Hi all,
I've got fed up with our sorry state of connector detection and rampant edid re and rere-reading.
This patch series lays the groundwork in the drm helpers so that drivers can avoid all this madness (at least on working hw) and properly cache the edid.
With the additional changes for drm/i915, the edid is now read _once_ per plug event (or at boot-up/resume time). A further step would be to integrate the hotplug handling into the driver itself and only call ->detect on the connectors for which the irq handler received a hotplug event.
By adding POLL_FORCE drivers can get back the old behaviour of calling ->detect every time probe_single_connector is called from userspace. I've splattered that over all drivers where I've thought it might be required.
Note though that setting this doesn't avoid all regressions - the regular output poll work will still ignore any connectors with POLL_HPD set. If a driver/hw with broken hpd got away due to this, this will break stuff. But that should be easy to fix by admitting the defeat and setting POLL_CONNECT|DISCONNECT directly.
If other people want to convert over their drivers, the following steps are required: - Ensure that the connector status is unknown every time the driver could have missed a hpd event (e.g. after resume). drm_mode_config_reset will do that for you. - Drop the POLL_FORCE flag for connectors where hdp is fully reliable. - Implement edid caching - that's a nice way to figure out whether hpd is actually reliable, because if it isn't, this step will ensure that you get bug reports because the the edid won't ever get updated ;-) - Optionally teach the driver some smarts about which specific connectors actually got a hotplug event. Mostly useful on cheap hw (like intel's) that can't distinguish between hdmi and dp without trying some aux channel transfers.
As you can guess from the patch series, I've discovered the hard way that i915 sdvo support is totally broken. Tested on most of the intel machines I have and also quickly on my radeon hd5000.
Comments, flames, ideas and test reports highly welcome.
Cheers, Daniel
Daniel Vetter (14): drm: extract drm_kms_helper_hotplug_event drm: handle HDP and polled connectors separately drm: introduce DRM_CONNECTOR_POLL_FORCE drm/i915: set POLL_FORCE for sdvo outputs drm: properly init/reset connector status drm: kill unnecessary calls to connector->detect drm: don't start the poll engine in probe_single_connector drm: don't unnecessarily enable the polling work drm: don't poll forced connectors drm/i915: cache crt edid drm/i915: cache dp edid drm/i915: cache hdmi edid drm/i915/sdvo: implement correct return value for ->get_modes drm/i915: s/mdelay/msleep/ in the sdvo detect function
drivers/gpu/drm/drm_crtc.c | 6 ++- drivers/gpu/drm/drm_crtc_helper.c | 76 ++++++++++++++++++------- drivers/gpu/drm/exynos/exynos_drm_connector.c | 2 + drivers/gpu/drm/gma500/cdv_intel_crt.c | 2 + drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 2 + drivers/gpu/drm/gma500/mdfld_dsi_output.c | 1 + drivers/gpu/drm/gma500/oaktrail_hdmi.c | 2 + drivers/gpu/drm/gma500/psb_intel_sdvo.c | 2 + drivers/gpu/drm/i915/intel_crt.c | 28 +++++++-- drivers/gpu/drm/i915/intel_dp.c | 47 +++++++-------- drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_hdmi.c | 48 ++++++++++------ drivers/gpu/drm/i915/intel_modes.c | 18 +++++- drivers/gpu/drm/i915/intel_sdvo.c | 45 +++++++++------ drivers/gpu/drm/i915/intel_tv.c | 3 +- drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + drivers/gpu/drm/radeon/radeon_connectors.c | 5 ++ drivers/gpu/drm/udl/udl_connector.c | 2 + drivers/staging/omapdrm/omap_connector.c | 2 + include/drm/drm_crtc.h | 5 ++ include/drm/drm_crtc_helper.h | 1 + 21 files changed, 208 insertions(+), 92 deletions(-)
Useful if drivers want to be slightly more clever about hotplug handling.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 17 +++++++++++------ include/drm/drm_crtc_helper.h | 1 + 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 974196a..909a85c 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -918,6 +918,15 @@ int drm_helper_resume_force_mode(struct drm_device *dev) } EXPORT_SYMBOL(drm_helper_resume_force_mode);
+void drm_kms_helper_hotplug_event(struct drm_device *dev) +{ + /* send a uevent + call fbdev */ + drm_sysfs_hotplug_event(dev); + if (dev->mode_config.funcs->output_poll_changed) + dev->mode_config.funcs->output_poll_changed(dev); +} +EXPORT_SYMBOL(drm_kms_helper_hotplug_event); + #define DRM_OUTPUT_POLL_PERIOD (10*HZ) static void output_poll_execute(struct work_struct *work) { @@ -960,12 +969,8 @@ static void output_poll_execute(struct work_struct *work)
mutex_unlock(&dev->mode_config.mutex);
- if (changed) { - /* send a uevent + call fbdev */ - drm_sysfs_hotplug_event(dev); - if (dev->mode_config.funcs->output_poll_changed) - dev->mode_config.funcs->output_poll_changed(dev); - } + if (changed) + drm_kms_helper_hotplug_event(dev);
if (repoll) queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD); diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 3add00e..9a9288d 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -141,6 +141,7 @@ extern int drm_helper_resume_force_mode(struct drm_device *dev); extern void drm_kms_helper_poll_init(struct drm_device *dev); extern void drm_kms_helper_poll_fini(struct drm_device *dev); extern void drm_helper_hpd_irq_event(struct drm_device *dev); +extern void drm_kms_helper_hotplug_event(struct drm_device *dev);
extern void drm_kms_helper_poll_disable(struct drm_device *dev); extern void drm_kms_helper_poll_enable(struct drm_device *dev);
Instead of reusing the polling code for hdp handling, split them up. This has a few consequences: - Don't touch HDP capable connectors in the poll loop. - Only touch HDP capable connectors in drm_helper_hpd_irq_event. - Run the HDP handling directly instead of going through a work item - all callers already call drm_helper_hpd_irq_event from process context without holding the mode_config mutex.
The ultimate goal is that drivers grow some smarts about which connectors have received a hotplug event and only call the detect code of that connector. But that's a second step.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 39 ++++++++++++++++++++++++++++-------- 1 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 909a85c..b1d643d 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -942,9 +942,9 @@ static void output_poll_execute(struct work_struct *work) mutex_lock(&dev->mode_config.mutex); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
- /* if this is HPD or polled don't check it - - TV out for instance */ - if (!connector->polled) + /* Ignore HDP capable connectors and connectors where we don't + * want any hotplug detection at all for polling. */ + if (!connector->polled || connector->polled == DRM_CONNECTOR_POLL_HPD) continue;
else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) @@ -954,8 +954,7 @@ static void output_poll_execute(struct work_struct *work) /* if we are connected and don't want to poll for disconnect skip it */ if (old_status == connector_status_connected && - !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT) && - !(connector->polled & DRM_CONNECTOR_POLL_HPD)) + !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) continue;
connector->status = connector->funcs->detect(connector, false); @@ -1019,12 +1018,34 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
void drm_helper_hpd_irq_event(struct drm_device *dev) { + struct drm_connector *connector; + enum drm_connector_status old_status; + bool changed = false; + if (!dev->mode_config.poll_enabled) return;
- /* kill timer and schedule immediate execution, this doesn't block */ - cancel_delayed_work(&dev->mode_config.output_poll_work); - if (drm_kms_helper_poll) - queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, 0); + mutex_lock(&dev->mode_config.mutex); + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + + /* Only handle HPD capable connectors. */ + if (connector->polled != DRM_CONNECTOR_POLL_HPD) + continue; + + old_status = connector->status; + + connector->status = connector->funcs->detect(connector, false); + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", + connector->base.id, + drm_get_connector_name(connector), + old_status, connector->status); + if (old_status != connector->status) + changed = true; + } + + mutex_unlock(&dev->mode_config.mutex); + + if (changed) + drm_kms_helper_hotplug_event(dev); } EXPORT_SYMBOL(drm_helper_hpd_irq_event);
On 5/24/12 3:26 PM, Daniel Vetter wrote:
Instead of reusing the polling code for hdp handling, split them up. This has a few consequences:
- Don't touch HDP capable connectors in the poll loop.
- Only touch HDP capable connectors in drm_helper_hpd_irq_event.
- Run the HDP handling directly instead of going through a work item - all callers already call drm_helper_hpd_irq_event from process context without holding the mode_config mutex.
"HPD".
/* if this is HPD or polled don't check it -TV out for instance */if (!connector->polled)
/* Ignore HDP capable connectors and connectors where we don't* want any hotplug detection at all for polling. */if (!connector->polled || connector->polled == DRM_CONNECTOR_POLL_HPD)
Here too. Also in the subject.
- ajax
Instead of reusing the polling code for hpd handling, split them up. This has a few consequences: - Don't touch HPD capable connectors in the poll loop. - Only touch HPD capable connectors in drm_helper_hpd_irq_event. - Run the HPD handling directly instead of going through a work item - all callers already call drm_helper_hpd_irq_event from process context without holding the mode_config mutex.
The ultimate goal is that drivers grow some smarts about which connectors have received a hotplug event and only call the detect code of that connector. But that's a second step.
v2: s/hdp/hpd/, noticed by Adam Jackson. I can't type.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 39 ++++++++++++++++++++++++++++-------- 1 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 909a85c..5461ee6 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -942,9 +942,9 @@ static void output_poll_execute(struct work_struct *work) mutex_lock(&dev->mode_config.mutex); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
- /* if this is HPD or polled don't check it - - TV out for instance */ - if (!connector->polled) + /* Ignore HPD capable connectors and connectors where we don't + * want any hotplug detection at all for polling. */ + if (!connector->polled || connector->polled == DRM_CONNECTOR_POLL_HPD) continue;
else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) @@ -954,8 +954,7 @@ static void output_poll_execute(struct work_struct *work) /* if we are connected and don't want to poll for disconnect skip it */ if (old_status == connector_status_connected && - !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT) && - !(connector->polled & DRM_CONNECTOR_POLL_HPD)) + !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) continue;
connector->status = connector->funcs->detect(connector, false); @@ -1019,12 +1018,34 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
void drm_helper_hpd_irq_event(struct drm_device *dev) { + struct drm_connector *connector; + enum drm_connector_status old_status; + bool changed = false; + if (!dev->mode_config.poll_enabled) return;
- /* kill timer and schedule immediate execution, this doesn't block */ - cancel_delayed_work(&dev->mode_config.output_poll_work); - if (drm_kms_helper_poll) - queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, 0); + mutex_lock(&dev->mode_config.mutex); + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + + /* Only handle HPD capable connectors. */ + if (connector->polled != DRM_CONNECTOR_POLL_HPD) + continue; + + old_status = connector->status; + + connector->status = connector->funcs->detect(connector, false); + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", + connector->base.id, + drm_get_connector_name(connector), + old_status, connector->status); + if (old_status != connector->status) + changed = true; + } + + mutex_unlock(&dev->mode_config.mutex); + + if (changed) + drm_kms_helper_hotplug_event(dev); } EXPORT_SYMBOL(drm_helper_hpd_irq_event);
Instead of reusing the polling code for hpd handling, split them up. This has a few consequences: - Don't touch HPD capable connectors in the poll loop. - Only touch HPD capable connectors in drm_helper_hpd_irq_event. - We could run the HPD handling directly (because all callers already use their own work item), but for easier bisect that happens in it's own patch.
The ultimate goal is that drivers grow some smarts about which connectors have received a hotplug event and only call the detect code of that connector. But that's a second step.
v2: s/hdp/hpd/, noticed by Adam Jackson. I can't type.
v3: Split out the work item removal as requested by Dave Airlie. This results in a temporary mode_config.hpd_irq_work item to keep things the same.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 51 ++++++++++++++++++++++++++++++------ include/drm/drm_crtc.h | 1 + 2 files changed, 43 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 909a85c..74da342 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -942,9 +942,9 @@ static void output_poll_execute(struct work_struct *work) mutex_lock(&dev->mode_config.mutex); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
- /* if this is HPD or polled don't check it - - TV out for instance */ - if (!connector->polled) + /* Ignore HPD capable connectors and connectors where we don't + * want any hotplug detection at all for polling. */ + if (!connector->polled || connector->polled == DRM_CONNECTOR_POLL_HPD) continue;
else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) @@ -954,8 +954,7 @@ static void output_poll_execute(struct work_struct *work) /* if we are connected and don't want to poll for disconnect skip it */ if (old_status == connector_status_connected && - !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT) && - !(connector->polled & DRM_CONNECTOR_POLL_HPD)) + !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) continue;
connector->status = connector->funcs->detect(connector, false); @@ -1002,9 +1001,12 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_enable);
+static void hpd_irq_event_execute(struct work_struct *work); + void drm_kms_helper_poll_init(struct drm_device *dev) { INIT_DELAYED_WORK(&dev->mode_config.output_poll_work, output_poll_execute); + INIT_DELAYED_WORK(&dev->mode_config.hpd_irq_work, hpd_irq_event_execute); dev->mode_config.poll_enabled = true;
drm_kms_helper_poll_enable(dev); @@ -1017,14 +1019,45 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini);
-void drm_helper_hpd_irq_event(struct drm_device *dev) +static void hpd_irq_event_execute(struct work_struct *work) { + struct delayed_work *delayed_work = to_delayed_work(work); + struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.hpd_irq_work); + struct drm_connector *connector; + enum drm_connector_status old_status; + bool changed = false; + if (!dev->mode_config.poll_enabled) return;
- /* kill timer and schedule immediate execution, this doesn't block */ - cancel_delayed_work(&dev->mode_config.output_poll_work); + mutex_lock(&dev->mode_config.mutex); + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + + /* Only handle HPD capable connectors. */ + if (connector->polled != DRM_CONNECTOR_POLL_HPD) + continue; + + old_status = connector->status; + + connector->status = connector->funcs->detect(connector, false); + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", + connector->base.id, + drm_get_connector_name(connector), + old_status, connector->status); + if (old_status != connector->status) + changed = true; + } + + mutex_unlock(&dev->mode_config.mutex); + + if (changed) + drm_kms_helper_hotplug_event(dev); +} + +void drm_helper_hpd_irq_event(struct drm_device *dev) +{ + cancel_delayed_work(&dev->mode_config.hpd_irq_work); if (drm_kms_helper_poll) - queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, 0); + queue_delayed_work(system_nrt_wq, &dev->mode_config.hpd_irq_work, 0); } EXPORT_SYMBOL(drm_helper_hpd_irq_event); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d59bb7d..03b08f7 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -780,6 +780,7 @@ struct drm_mode_config { /* output poll support */ bool poll_enabled; struct delayed_work output_poll_work; + struct delayed_work hpd_irq_work;
/* pointers to standard properties */ struct list_head property_blob_list;
All drivers already have a work item to run the hpd code, so we don't need to launch a new one in the helper code. Dave Airlie mentioned that the cancel+re-queue might paper over DP related hpd ping-pongs, hence why this is split out.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 14 +------------- include/drm/drm_crtc.h | 1 - 2 files changed, 1 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 74da342..5461ee6 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -1001,12 +1001,9 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_enable);
-static void hpd_irq_event_execute(struct work_struct *work); - void drm_kms_helper_poll_init(struct drm_device *dev) { INIT_DELAYED_WORK(&dev->mode_config.output_poll_work, output_poll_execute); - INIT_DELAYED_WORK(&dev->mode_config.hpd_irq_work, hpd_irq_event_execute); dev->mode_config.poll_enabled = true;
drm_kms_helper_poll_enable(dev); @@ -1019,10 +1016,8 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini);
-static void hpd_irq_event_execute(struct work_struct *work) +void drm_helper_hpd_irq_event(struct drm_device *dev) { - struct delayed_work *delayed_work = to_delayed_work(work); - struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.hpd_irq_work); struct drm_connector *connector; enum drm_connector_status old_status; bool changed = false; @@ -1053,11 +1048,4 @@ static void hpd_irq_event_execute(struct work_struct *work) if (changed) drm_kms_helper_hotplug_event(dev); } - -void drm_helper_hpd_irq_event(struct drm_device *dev) -{ - cancel_delayed_work(&dev->mode_config.hpd_irq_work); - if (drm_kms_helper_poll) - queue_delayed_work(system_nrt_wq, &dev->mode_config.hpd_irq_work, 0); -} EXPORT_SYMBOL(drm_helper_hpd_irq_event); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 03b08f7..d59bb7d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -780,7 +780,6 @@ struct drm_mode_config { /* output poll support */ bool poll_enabled; struct delayed_work output_poll_work; - struct delayed_work hpd_irq_work;
/* pointers to standard properties */ struct list_head property_blob_list;
Useful for ->detect functions that have different behaviour if force is set. This way probe_single_connector can avoid to do the expensive edid dance on connectors where this is not needed.
I've checked through all drivers and set this flag everywhere where the connector->detect function has different behaviour if force is set. For nouveau and radeon I've got lost in the code traces, so I've set this flag unconditionally.
Note that we also need to update the poll_execute function to now also ignore connectors which have only this new flag set.
v2: Change POLL_HDP checks so that they ignore POLL_FORCE for both the poll and the hpd handling code.
v3: Sprinkle POLL_FORCE more liberally over drivers. It should be now everywhere where a non-intel driver can return anything else than connector_status_connected in its detect callback.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 6 ++++-- drivers/gpu/drm/exynos/exynos_drm_connector.c | 2 ++ drivers/gpu/drm/gma500/cdv_intel_crt.c | 2 ++ drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 2 ++ drivers/gpu/drm/gma500/mdfld_dsi_output.c | 1 + drivers/gpu/drm/gma500/oaktrail_hdmi.c | 2 ++ drivers/gpu/drm/gma500/psb_intel_sdvo.c | 2 ++ drivers/gpu/drm/i915/intel_crt.c | 3 ++- drivers/gpu/drm/i915/intel_tv.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + drivers/gpu/drm/radeon/radeon_connectors.c | 5 +++++ drivers/gpu/drm/udl/udl_connector.c | 2 ++ drivers/staging/omapdrm/omap_connector.c | 2 ++ include/drm/drm_crtc.h | 4 ++++ 14 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index b1d643d..8ea1c1e 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -944,7 +944,9 @@ static void output_poll_execute(struct work_struct *work)
/* Ignore HDP capable connectors and connectors where we don't * want any hotplug detection at all for polling. */ - if (!connector->polled || connector->polled == DRM_CONNECTOR_POLL_HPD) + if (!connector->polled || + connector->polled == DRM_CONNECTOR_POLL_FORCE || + (connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) @@ -1029,7 +1031,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
/* Only handle HPD capable connectors. */ - if (connector->polled != DRM_CONNECTOR_POLL_HPD) + if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
old_status = connector->status; diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c index bf791fa..e5a8a27 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c @@ -327,6 +327,8 @@ struct drm_connector *exynos_drm_connector_create(struct drm_device *dev, drm_connector_init(dev, connector, &exynos_connector_funcs, type); drm_connector_helper_add(connector, &exynos_connector_helper_funcs);
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + err = drm_sysfs_connector_add(connector); if (err) goto err_connector; diff --git a/drivers/gpu/drm/gma500/cdv_intel_crt.c b/drivers/gpu/drm/gma500/cdv_intel_crt.c index 1874220..e6b2e49 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_crt.c +++ b/drivers/gpu/drm/gma500/cdv_intel_crt.c @@ -313,6 +313,8 @@ void cdv_intel_crt_init(struct drm_device *dev, drm_connector_helper_add(connector, &cdv_intel_crt_connector_helper_funcs);
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector);
return; diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c index 88b59d4..766aec8 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c @@ -373,6 +373,8 @@ void cdv_hdmi_init(struct drm_device *dev, hdmi_priv->hdmi_i2c_adapter = &(psb_intel_encoder->i2c_bus->adapter); hdmi_priv->dev = dev; + connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector); return;
diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_output.c b/drivers/gpu/drm/gma500/mdfld_dsi_output.c index 5675d93..0e97a91 100644 --- a/drivers/gpu/drm/gma500/mdfld_dsi_output.c +++ b/drivers/gpu/drm/gma500/mdfld_dsi_output.c @@ -604,6 +604,7 @@ void mdfld_dsi_output_init(struct drm_device *dev, dsi_config->encoder = encoder; encoder->base.type = (pipe == 0) ? INTEL_OUTPUT_MIPI : INTEL_OUTPUT_MIPI2; + connector->polled |= DRM_CONNECTOR_POLL_FORCE; drm_sysfs_connector_add(connector); return;
diff --git a/drivers/gpu/drm/gma500/oaktrail_hdmi.c b/drivers/gpu/drm/gma500/oaktrail_hdmi.c index c10899c..23b8d52 100644 --- a/drivers/gpu/drm/gma500/oaktrail_hdmi.c +++ b/drivers/gpu/drm/gma500/oaktrail_hdmi.c @@ -349,6 +349,8 @@ void oaktrail_hdmi_init(struct drm_device *dev, connector->display_info.subpixel_order = SubPixelHorizontalRGB; connector->interlace_allowed = false; connector->doublescan_allowed = false; + connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector);
return; diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c index d39b15b..e86d8ba 100644 --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c @@ -2028,6 +2028,8 @@ psb_intel_sdvo_connector_init(struct psb_intel_sdvo_connector *connector, connector->base.base.display_info.subpixel_order = SubPixelHorizontalRGB;
psb_intel_connector_attach_encoder(&connector->base, &encoder->base); + connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(&connector->base.base); }
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 75a70c4..a60d131 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -639,7 +639,8 @@ void intel_crt_init(struct drm_device *dev) if (I915_HAS_HOTPLUG(dev)) connector->polled = DRM_CONNECTOR_POLL_HPD; else - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_FORCE;
/* * Configure the automatic hotplug detection stuff diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index a233a51..dad7d8e 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1614,7 +1614,8 @@ intel_tv_init(struct drm_device *dev) * * More recent chipsets favour HDMI rather than integrated S-Video. */ - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_FORCE;
drm_connector_init(dev, connector, &intel_tv_connector_funcs, DRM_MODE_CONNECTOR_SVIDEO); diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index fa86035..780d608 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -1085,6 +1085,7 @@ nouveau_connector_create(struct drm_device *dev, int index) if (ret == 0) connector->polled = DRM_CONNECTOR_POLL_HPD; } + connector->polled |= DRM_CONNECTOR_POLL_FORCE;
drm_sysfs_connector_add(connector); return connector; diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 2914c57..c64ecc9 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -1830,6 +1830,8 @@ radeon_add_atom_connector(struct drm_device *dev, } else connector->polled = DRM_CONNECTOR_POLL_HPD;
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + connector->display_info.subpixel_order = subpixel_order; drm_sysfs_connector_add(connector); return; @@ -1987,6 +1989,9 @@ radeon_add_legacy_connector(struct drm_device *dev, connector->polled = DRM_CONNECTOR_POLL_CONNECT; } else connector->polled = DRM_CONNECTOR_POLL_HPD; + + connector->polled |= DRM_CONNECTOR_POLL_FORCE; + connector->display_info.subpixel_order = subpixel_order; drm_sysfs_connector_add(connector); if (connector_type == DRM_MODE_CONNECTOR_LVDS) { diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index ba055e9..838dc71 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -131,6 +131,8 @@ int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder) drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_DVII); drm_connector_helper_add(connector, &udl_connector_helper_funcs);
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector); drm_mode_connector_attach_encoder(connector, encoder);
diff --git a/drivers/staging/omapdrm/omap_connector.c b/drivers/staging/omapdrm/omap_connector.c index 5e2856c..bb8b134 100644 --- a/drivers/staging/omapdrm/omap_connector.c +++ b/drivers/staging/omapdrm/omap_connector.c @@ -358,6 +358,8 @@ struct drm_connector *omap_connector_init(struct drm_device *dev, connector->interlace_allowed = 1; connector->doublescan_allowed = 0;
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector);
return connector; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d59bb7d..4417da2 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -514,6 +514,10 @@ enum drm_connector_force { /* can cleanly poll for disconnections without flickering the screen */ /* DACs should rarely do this without a lot of testing */ #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) +/* connector's ->detect function needs to be called when userspace forces + * detection. Should only be used if the detect function has special handling + * when force is set to avoid spurious re-reading of the edid. */ +#define DRM_CONNECTOR_POLL_FORCE (1 << 3)
#define MAX_ELD_BYTES 128
Useful for ->detect functions that have different behaviour if force is set. This way probe_single_connector can avoid to do the expensive edid dance on connectors where this is not needed.
I've checked through all drivers and set this flag everywhere where the connector->detect function has different behaviour if force is set. For nouveau and radeon I've got lost in the code traces, so I've set this flag unconditionally.
Note that we also need to update the poll_execute function to now also ignore connectors which have only this new flag set.
v2: Change POLL_HDP checks so that they ignore POLL_FORCE for both the poll and the hpd handling code.
v3: Sprinkle POLL_FORCE more liberally over drivers. It should be now everywhere where a non-intel driver can return anything else than connector_status_connected in its detect callback.
v4: Fixup psb compile fail.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 6 ++++-- drivers/gpu/drm/exynos/exynos_drm_connector.c | 2 ++ drivers/gpu/drm/gma500/cdv_intel_crt.c | 2 ++ drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 2 ++ drivers/gpu/drm/gma500/mdfld_dsi_output.c | 1 + drivers/gpu/drm/gma500/oaktrail_hdmi.c | 2 ++ drivers/gpu/drm/gma500/psb_intel_sdvo.c | 2 ++ drivers/gpu/drm/i915/intel_crt.c | 3 ++- drivers/gpu/drm/i915/intel_tv.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + drivers/gpu/drm/radeon/radeon_connectors.c | 5 +++++ drivers/gpu/drm/udl/udl_connector.c | 2 ++ drivers/staging/omapdrm/omap_connector.c | 2 ++ include/drm/drm_crtc.h | 4 ++++ 14 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index b1d643d..8ea1c1e 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -944,7 +944,9 @@ static void output_poll_execute(struct work_struct *work)
/* Ignore HDP capable connectors and connectors where we don't * want any hotplug detection at all for polling. */ - if (!connector->polled || connector->polled == DRM_CONNECTOR_POLL_HPD) + if (!connector->polled || + connector->polled == DRM_CONNECTOR_POLL_FORCE || + (connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) @@ -1029,7 +1031,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
/* Only handle HPD capable connectors. */ - if (connector->polled != DRM_CONNECTOR_POLL_HPD) + if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
old_status = connector->status; diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c index bf791fa..e5a8a27 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c @@ -327,6 +327,8 @@ struct drm_connector *exynos_drm_connector_create(struct drm_device *dev, drm_connector_init(dev, connector, &exynos_connector_funcs, type); drm_connector_helper_add(connector, &exynos_connector_helper_funcs);
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + err = drm_sysfs_connector_add(connector); if (err) goto err_connector; diff --git a/drivers/gpu/drm/gma500/cdv_intel_crt.c b/drivers/gpu/drm/gma500/cdv_intel_crt.c index 1874220..e6b2e49 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_crt.c +++ b/drivers/gpu/drm/gma500/cdv_intel_crt.c @@ -313,6 +313,8 @@ void cdv_intel_crt_init(struct drm_device *dev, drm_connector_helper_add(connector, &cdv_intel_crt_connector_helper_funcs);
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector);
return; diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c index 88b59d4..766aec8 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c @@ -373,6 +373,8 @@ void cdv_hdmi_init(struct drm_device *dev, hdmi_priv->hdmi_i2c_adapter = &(psb_intel_encoder->i2c_bus->adapter); hdmi_priv->dev = dev; + connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector); return;
diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_output.c b/drivers/gpu/drm/gma500/mdfld_dsi_output.c index 5675d93..0e97a91 100644 --- a/drivers/gpu/drm/gma500/mdfld_dsi_output.c +++ b/drivers/gpu/drm/gma500/mdfld_dsi_output.c @@ -604,6 +604,7 @@ void mdfld_dsi_output_init(struct drm_device *dev, dsi_config->encoder = encoder; encoder->base.type = (pipe == 0) ? INTEL_OUTPUT_MIPI : INTEL_OUTPUT_MIPI2; + connector->polled |= DRM_CONNECTOR_POLL_FORCE; drm_sysfs_connector_add(connector); return;
diff --git a/drivers/gpu/drm/gma500/oaktrail_hdmi.c b/drivers/gpu/drm/gma500/oaktrail_hdmi.c index c10899c..23b8d52 100644 --- a/drivers/gpu/drm/gma500/oaktrail_hdmi.c +++ b/drivers/gpu/drm/gma500/oaktrail_hdmi.c @@ -349,6 +349,8 @@ void oaktrail_hdmi_init(struct drm_device *dev, connector->display_info.subpixel_order = SubPixelHorizontalRGB; connector->interlace_allowed = false; connector->doublescan_allowed = false; + connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector);
return; diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c index d39b15b..fdfc4a0 100644 --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c @@ -2028,6 +2028,8 @@ psb_intel_sdvo_connector_init(struct psb_intel_sdvo_connector *connector, connector->base.base.display_info.subpixel_order = SubPixelHorizontalRGB;
psb_intel_connector_attach_encoder(&connector->base, &encoder->base); + connector->base.base.polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(&connector->base.base); }
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 75a70c4..a60d131 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -639,7 +639,8 @@ void intel_crt_init(struct drm_device *dev) if (I915_HAS_HOTPLUG(dev)) connector->polled = DRM_CONNECTOR_POLL_HPD; else - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_FORCE;
/* * Configure the automatic hotplug detection stuff diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index a233a51..dad7d8e 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1614,7 +1614,8 @@ intel_tv_init(struct drm_device *dev) * * More recent chipsets favour HDMI rather than integrated S-Video. */ - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_FORCE;
drm_connector_init(dev, connector, &intel_tv_connector_funcs, DRM_MODE_CONNECTOR_SVIDEO); diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index fa86035..780d608 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -1085,6 +1085,7 @@ nouveau_connector_create(struct drm_device *dev, int index) if (ret == 0) connector->polled = DRM_CONNECTOR_POLL_HPD; } + connector->polled |= DRM_CONNECTOR_POLL_FORCE;
drm_sysfs_connector_add(connector); return connector; diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 2914c57..c64ecc9 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -1830,6 +1830,8 @@ radeon_add_atom_connector(struct drm_device *dev, } else connector->polled = DRM_CONNECTOR_POLL_HPD;
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + connector->display_info.subpixel_order = subpixel_order; drm_sysfs_connector_add(connector); return; @@ -1987,6 +1989,9 @@ radeon_add_legacy_connector(struct drm_device *dev, connector->polled = DRM_CONNECTOR_POLL_CONNECT; } else connector->polled = DRM_CONNECTOR_POLL_HPD; + + connector->polled |= DRM_CONNECTOR_POLL_FORCE; + connector->display_info.subpixel_order = subpixel_order; drm_sysfs_connector_add(connector); if (connector_type == DRM_MODE_CONNECTOR_LVDS) { diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index ba055e9..838dc71 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -131,6 +131,8 @@ int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder) drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_DVII); drm_connector_helper_add(connector, &udl_connector_helper_funcs);
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector); drm_mode_connector_attach_encoder(connector, encoder);
diff --git a/drivers/staging/omapdrm/omap_connector.c b/drivers/staging/omapdrm/omap_connector.c index 5e2856c..bb8b134 100644 --- a/drivers/staging/omapdrm/omap_connector.c +++ b/drivers/staging/omapdrm/omap_connector.c @@ -358,6 +358,8 @@ struct drm_connector *omap_connector_init(struct drm_device *dev, connector->interlace_allowed = 1; connector->doublescan_allowed = 0;
+ connector->polled |= DRM_CONNECTOR_POLL_FORCE; + drm_sysfs_connector_add(connector);
return connector; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d59bb7d..4417da2 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -514,6 +514,10 @@ enum drm_connector_force { /* can cleanly poll for disconnections without flickering the screen */ /* DACs should rarely do this without a lot of testing */ #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) +/* connector's ->detect function needs to be called when userspace forces + * detection. Should only be used if the detect function has special handling + * when force is set to avoid spurious re-reading of the edid. */ +#define DRM_CONNECTOR_POLL_FORCE (1 << 3)
#define MAX_ELD_BYTES 128
The detection function is simply too unreliable - it doesn't properly pick up changes right away.
For now, just set the compat flag and ignore this, because on a quick look fixing this properly is a very big fish to fry.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_sdvo.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index a658207..fdc0574 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2012,6 +2012,8 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector, connector->base.base.doublescan_allowed = 0; connector->base.base.display_info.subpixel_order = SubPixelHorizontalRGB;
+ connector->base.base.polled |= DRM_CONNECTOR_POLL_FORCE; + intel_connector_attach_encoder(&connector->base, &encoder->base); drm_sysfs_connector_add(&connector->base.base); }
We need this because otherwise the improved connector code has no idea when it needs to run the ->detect callback after boot/resume on all connectors.
Because drm/i915 is the only driver that properly calls mode_config_reset at resume time, this will horribly blow up everywhere else.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a177d0a..82eaff0 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -495,6 +495,7 @@ int drm_connector_init(struct drm_device *dev, INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL; + connector->status = connector_status_unknown;
list_add_tail(&connector->head, &dev->mode_config.connector_list); dev->mode_config.num_connector++; @@ -3521,9 +3522,12 @@ void drm_mode_config_reset(struct drm_device *dev) if (encoder->funcs->reset) encoder->funcs->reset(encoder);
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + connector->status = connector_status_unknown; + if (connector->funcs->reset) connector->funcs->reset(connector); + } } EXPORT_SYMBOL(drm_mode_config_reset);
Only call that function if something has actually changed (i.e. in the output polling or hdp handling functions) or when userspace asks for the information and DRM_CONNECTOR_POLL_FORCE is set.
Let's see how many bugs this uncovers.
v2: Run ->detect if the current connector status is 'unknown' - otherwise we won't ever detect the boot-up/resume state correctly.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 8ea1c1e..db93e4d 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -107,7 +107,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, connector->status = connector_status_disconnected; if (connector->funcs->force) connector->funcs->force(connector); - } else { + } else if (connector->polled & DRM_CONNECTOR_POLL_FORCE || + connector->status == connector_status_unknown) { connector->status = connector->funcs->detect(connector, true); drm_kms_helper_poll_enable(dev); }
Actually there's a reason this stuff is there, and it's called
commit e58f637bb96d5a0ae0919b9998b891d1ba7e47c9 Author: Chris Wilson chris@chris-wilson.co.uk Date: Fri Aug 20 09:13:36 2010 +0100
drm/kms: Add a module parameter to disable polling
The idea has been that users can enable/disable polling at runtime. So the quick hack has been to just re-enable the output polling if xrandr asks for the latest state of the connectors.
The problem with that hack is that when we force connectors to another state than what would be detected, we nicely ping-pong: - Userspace calls probe, gets the forced state, but polling starts again. - Polling notices that the state is actually different, wakes up userspace. - Repeat.
As that commit already explains, the right fix would be to make the locking more fine-grained, so that hotplug detection on one output does not interfere with cursor updates on another crtc.
But that is way too much work. So let's just safe this gross hack by caching the last-seen state of drm_kms_helper_poll for that driver, and only fire up the poll engine again if it changed from off to on.
v2: Fixup the edge detection of drm_kms_helper_poll.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49907 Tested-by: Tvrtko Ursulin tvrtko.ursulin@onelan.co.uk Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 7 ++++++- include/drm/drm_crtc.h | 1 + 2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index db93e4d..f17953e 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -110,9 +110,14 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, } else if (connector->polled & DRM_CONNECTOR_POLL_FORCE || connector->status == connector_status_unknown) { connector->status = connector->funcs->detect(connector, true); - drm_kms_helper_poll_enable(dev); }
+ /* Re-enable polling in case the global poll config changed. */ + if (drm_kms_helper_poll != dev->mode_config.poll_running) + drm_kms_helper_poll_enable(dev); + + dev->mode_config.poll_running = drm_kms_helper_poll; + if (connector->status == connector_status_disconnected) { DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n", connector->base.id, drm_get_connector_name(connector)); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4417da2..fb21121 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -783,6 +783,7 @@ struct drm_mode_config {
/* output poll support */ bool poll_enabled; + bool poll_running; struct delayed_work output_poll_work;
/* pointers to standard properties */
... by properly checking connector->polled. This doesn't matter too much because the polling work itself gets this slightly more right and doesn't set repoll if there's nothing to do. But we can do better.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index f17953e..87a45de 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -955,9 +955,6 @@ static void output_poll_execute(struct work_struct *work) (connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
- else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) - repoll = true; - old_status = connector->status; /* if we are connected and don't want to poll for disconnect skip it */ @@ -1000,7 +997,8 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) return;
list_for_each_entry(connector, &dev->mode_config.connector_list, head) { - if (connector->polled) + if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT)) poll = true; }
Otherwise if the detect callback reports a different state than what the user forced (rather likely), we continously annoy userspace about a hotplug uevent.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 87a45de..9214612 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -948,6 +948,10 @@ static void output_poll_execute(struct work_struct *work) mutex_lock(&dev->mode_config.mutex); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+ /* Ignore forced connectors. */ + if (connector->force) + continue; + /* Ignore HDP capable connectors and connectors where we don't * want any hotplug detection at all for polling. */ if (!connector->polled ||
Let's put all this new&neat output detection infrastructure and rework to some good use and cache the crt edid. Given that the drm helpers now only call ->detect when actually required, we only need to reset the edid there and can keep it otherwise.
Slashes xrandr time on systems that are hotplug capable if there's something connected to the VGA connector.
v2: Remember to clean up the cached edid on destroy.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_crt.c | 25 ++++++++++++++++++++----- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_modes.c | 18 ++++++++++++++---- 3 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index a60d131..46a0716 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -47,6 +47,7 @@ struct intel_crt { struct intel_encoder base; bool force_hotplug_required; + struct edid *cached_edid; };
static struct intel_crt *intel_attached_crt(struct drm_connector *connector) @@ -310,7 +311,7 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector) if (edid != NULL) { is_digital = edid->input & DRM_EDID_INPUT_DIGITAL; connector->display_info.raw_edid = NULL; - kfree(edid); + crt->cached_edid = edid; }
if (!is_digital) { @@ -452,6 +453,10 @@ intel_crt_detect(struct drm_connector *connector, bool force) enum drm_connector_status status; struct intel_load_detect_pipe tmp;
+ /* Clean the edid cache. */ + kfree(crt->cached_edid); + crt->cached_edid = NULL; + if (I915_HAS_HOTPLUG(dev)) { if (intel_crt_detect_hotplug(connector)) { DRM_DEBUG_KMS("CRT detected via hotplug\n"); @@ -485,8 +490,11 @@ intel_crt_detect(struct drm_connector *connector, bool force)
static void intel_crt_destroy(struct drm_connector *connector) { + struct intel_crt *crt = intel_attached_crt(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); + kfree(crt->cached_edid); kfree(connector); }
@@ -494,17 +502,24 @@ static int intel_crt_get_modes(struct drm_connector *connector) { struct drm_device *dev = connector->dev; struct drm_i915_private *dev_priv = dev->dev_private; - int ret; + struct intel_crt *crt = intel_attached_crt(connector); + int ret = 0; struct i2c_adapter *i2c;
- i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin); - ret = intel_ddc_get_modes(connector, i2c); + if (!crt->cached_edid) { + i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin); + crt->cached_edid = drm_get_edid(connector, i2c); + } + + ret = intel_edid_get_modes(connector, crt->cached_edid); if (ret || !IS_G4X(dev)) return ret;
/* Try to probe digital port for output in DVI-I -> VGA mode. */ i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB); - return intel_ddc_get_modes(connector, i2c); + kfree(crt->cached_edid); + crt->cached_edid = drm_get_edid(connector, i2c); + return intel_edid_get_modes(connector, crt->cached_edid); }
static int intel_crt_set_property(struct drm_connector *connector, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3e09188..3b6f716 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -335,6 +335,7 @@ struct intel_fbc_work { };
int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); +int intel_edid_get_modes(struct drm_connector *connector, struct edid *edid); extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus);
extern void intel_attach_force_audio_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index d67ec3a..6c723d9 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -60,6 +60,19 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus) msgs, 2) == 2; }
+int intel_edid_get_modes(struct drm_connector *connector, + struct edid *edid) +{ + int ret; + + drm_mode_connector_update_edid_property(connector, edid); + ret = drm_add_edid_modes(connector, edid); + drm_edid_to_eld(connector, edid); + connector->display_info.raw_edid = NULL; + + return ret; +} + /** * intel_ddc_get_modes - get modelist from monitor * @connector: DRM connector device to use @@ -75,10 +88,7 @@ int intel_ddc_get_modes(struct drm_connector *connector,
edid = drm_get_edid(connector, adapter); if (edid) { - drm_mode_connector_update_edid_property(connector, edid); - ret = drm_add_edid_modes(connector, edid); - drm_edid_to_eld(connector, edid); - connector->display_info.raw_edid = NULL; + intel_edid_get_modes(connector, edid); kfree(edid); }
Again, let's be slightly more clever here.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_dp.c | 47 ++++++++++++++++++-------------------- 1 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 3bbd754..1c84a97 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -67,6 +67,7 @@ struct intel_dp { struct drm_display_mode *panel_fixed_mode; /* for eDP */ struct delayed_work panel_vdd_work; bool want_panel_vdd; + struct edid *cached_edid; };
/** @@ -2089,30 +2090,19 @@ g4x_dp_detect(struct intel_dp *intel_dp) }
static struct edid * -intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) +intel_dp_get_edid(struct drm_connector *connector) { struct intel_dp *intel_dp = intel_attached_dp(connector); - struct edid *edid; - - ironlake_edp_panel_vdd_on(intel_dp); - edid = drm_get_edid(connector, adapter); - ironlake_edp_panel_vdd_off(intel_dp, false); - return edid; -}
-static int -intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter) -{ - struct intel_dp *intel_dp = intel_attached_dp(connector); - int ret; + if (!intel_dp->cached_edid) { + ironlake_edp_panel_vdd_on(intel_dp); + intel_dp->cached_edid = drm_get_edid(connector, &intel_dp->adapter); + ironlake_edp_panel_vdd_off(intel_dp, false); + }
- ironlake_edp_panel_vdd_on(intel_dp); - ret = intel_ddc_get_modes(connector, adapter); - ironlake_edp_panel_vdd_off(intel_dp, false); - return ret; + return intel_dp->cached_edid; }
- /** * Uses CRT_HOTPLUG_EN and CRT_HOTPLUG_STAT to detect DP connection. * @@ -2127,6 +2117,10 @@ intel_dp_detect(struct drm_connector *connector, bool force) enum drm_connector_status status; struct edid *edid = NULL;
+ /* Clean the edid cache. */ + kfree(intel_dp->cached_edid); + intel_dp->cached_edid = NULL; + intel_dp->has_audio = false;
if (HAS_PCH_SPLIT(dev)) @@ -2145,11 +2139,10 @@ intel_dp_detect(struct drm_connector *connector, bool force) if (intel_dp->force_audio != HDMI_AUDIO_AUTO) { intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON); } else { - edid = intel_dp_get_edid(connector, &intel_dp->adapter); + edid = intel_dp_get_edid(connector); if (edid) { intel_dp->has_audio = drm_detect_monitor_audio(edid); connector->display_info.raw_edid = NULL; - kfree(edid); } }
@@ -2161,12 +2154,16 @@ static int intel_dp_get_modes(struct drm_connector *connector) struct intel_dp *intel_dp = intel_attached_dp(connector); struct drm_device *dev = intel_dp->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - int ret; + struct edid *edid; + int ret = 0;
/* We should parse the EDID data and find out if it has an audio sink */
- ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter); + edid = intel_dp_get_edid(connector); + if (edid) + ret = intel_edid_get_modes(connector, edid); + if (ret) { if (is_edp(intel_dp) && !intel_dp->panel_fixed_mode) { struct drm_display_mode *newmode; @@ -2206,16 +2203,14 @@ static int intel_dp_get_modes(struct drm_connector *connector) static bool intel_dp_detect_audio(struct drm_connector *connector) { - struct intel_dp *intel_dp = intel_attached_dp(connector); struct edid *edid; bool has_audio = false;
- edid = intel_dp_get_edid(connector, &intel_dp->adapter); + edid = intel_dp_get_edid(connector); if (edid) { has_audio = drm_detect_monitor_audio(edid);
connector->display_info.raw_edid = NULL; - kfree(edid); }
return has_audio; @@ -2279,6 +2274,7 @@ done: static void intel_dp_destroy(struct drm_connector *connector) { + struct intel_dp *intel_dp = intel_attached_dp(connector); struct drm_device *dev = connector->dev;
if (intel_dpd_is_edp(dev)) @@ -2286,6 +2282,7 @@ intel_dp_destroy(struct drm_connector *connector)
drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); + kfree(intel_dp->cached_edid); kfree(connector); }
Like the previous patches.
While at it also kill a stale comment - we've moved hdmi audio detection from ->get_modes to ->detect and the audio property handling functions.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 48 +++++++++++++++++++++++-------------- 2 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3b6f716..8693551 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -301,6 +301,7 @@ struct intel_hdmi { enum hdmi_force_audio force_audio; void (*write_infoframe)(struct drm_encoder *encoder, struct dip_infoframe *frame); + struct edid *cached_edid; };
static inline struct drm_crtc * diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 2ead3bf..373d252 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -452,19 +452,37 @@ static bool intel_hdmi_mode_fixup(struct drm_encoder *encoder, return true; }
+struct edid * +intel_hdmi_get_edid(struct drm_connector *connector) +{ + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); + struct drm_i915_private *dev_priv = connector->dev->dev_private; + + if (!intel_hdmi->cached_edid) { + struct i2c_adapter *adapter; + + adapter = intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus); + intel_hdmi->cached_edid = drm_get_edid(connector, adapter); + } + + return intel_hdmi->cached_edid; +} + static enum drm_connector_status intel_hdmi_detect(struct drm_connector *connector, bool force) { struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - struct drm_i915_private *dev_priv = connector->dev->dev_private; struct edid *edid; enum drm_connector_status status = connector_status_disconnected;
+ /* Clean the edid cache. */ + kfree(intel_hdmi->cached_edid); + intel_hdmi->cached_edid = NULL; + intel_hdmi->has_hdmi_sink = false; intel_hdmi->has_audio = false; - edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + edid = intel_hdmi_get_edid(connector);
if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) { @@ -477,6 +495,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) connector->display_info.raw_edid = NULL; kfree(edid); } + intel_hdmi->cached_edid = edid;
if (status == connector_status_connected) { if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO) @@ -489,29 +508,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
static int intel_hdmi_get_modes(struct drm_connector *connector) { - struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - struct drm_i915_private *dev_priv = connector->dev->dev_private; - - /* We should parse the EDID data and find out if it's an HDMI sink so - * we can send audio to it. - */ + struct edid *edid;
- return intel_ddc_get_modes(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + edid = intel_hdmi_get_edid(connector); + return intel_edid_get_modes(connector, edid); }
static bool intel_hdmi_detect_audio(struct drm_connector *connector) { - struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - struct drm_i915_private *dev_priv = connector->dev->dev_private; struct edid *edid; bool has_audio = false;
- edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + edid = intel_hdmi_get_edid(connector); if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) has_audio = drm_detect_monitor_audio(edid); @@ -580,8 +589,11 @@ done:
static void intel_hdmi_destroy(struct drm_connector *connector) { + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); + drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); + kfree(intel_hdmi->cached_edid); kfree(connector); }
We should return the number of added modes. Luckily no one really cares, but it kinda sticked out compared to the other ->get_modes functions I've looked at recently.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_sdvo.c | 41 ++++++++++++++++++++++-------------- 1 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index fdc0574..6056603 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1428,8 +1428,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force) return ret; }
-static void intel_sdvo_get_ddc_modes(struct drm_connector *connector) +static int intel_sdvo_get_ddc_modes(struct drm_connector *connector) { + int ret = 0; struct edid *edid;
/* set the bus switch and get the modes */ @@ -1448,12 +1449,14 @@ static void intel_sdvo_get_ddc_modes(struct drm_connector *connector) if (intel_sdvo_connector_matches_edid(to_intel_sdvo_connector(connector), edid)) { drm_mode_connector_update_edid_property(connector, edid); - drm_add_edid_modes(connector, edid); + ret = drm_add_edid_modes(connector, edid); }
connector->display_info.raw_edid = NULL; kfree(edid); } + + return ret; }
/* @@ -1521,12 +1524,12 @@ static const struct drm_display_mode sdvo_tv_modes[] = { DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) }, };
-static void intel_sdvo_get_tv_modes(struct drm_connector *connector) +static int intel_sdvo_get_tv_modes(struct drm_connector *connector) { struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector); struct intel_sdvo_sdtv_resolution_request tv_res; uint32_t reply = 0, format_map = 0; - int i; + int i, ret = 0;
/* Read the list of supported input resolutions for the selected TV * format. @@ -1536,39 +1539,44 @@ static void intel_sdvo_get_tv_modes(struct drm_connector *connector) min(sizeof(format_map), sizeof(struct intel_sdvo_sdtv_resolution_request)));
if (!intel_sdvo_set_target_output(intel_sdvo, intel_sdvo->attached_output)) - return; + return 0;
BUILD_BUG_ON(sizeof(tv_res) != 3); if (!intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_SDTV_RESOLUTION_SUPPORT, &tv_res, sizeof(tv_res))) - return; + return 0; if (!intel_sdvo_read_response(intel_sdvo, &reply, 3)) - return; + return 0;
for (i = 0; i < ARRAY_SIZE(sdvo_tv_modes); i++) if (reply & (1 << i)) { struct drm_display_mode *nmode; nmode = drm_mode_duplicate(connector->dev, &sdvo_tv_modes[i]); - if (nmode) + if (nmode) { drm_mode_probed_add(connector, nmode); + ret++; + } } + + return ret; }
-static void intel_sdvo_get_lvds_modes(struct drm_connector *connector) +static int intel_sdvo_get_lvds_modes(struct drm_connector *connector) { struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector); struct drm_i915_private *dev_priv = connector->dev->dev_private; struct drm_display_mode *newmode; + int ret;
/* * Attempt to get the mode list from DDC. * Assume that the preferred modes are * arranged in priority order. */ - intel_ddc_get_modes(connector, intel_sdvo->i2c); - if (list_empty(&connector->probed_modes) == false) + ret = intel_ddc_get_modes(connector, intel_sdvo->i2c); + if (ret) goto end;
/* Fetch modes from VBT */ @@ -1580,6 +1588,8 @@ static void intel_sdvo_get_lvds_modes(struct drm_connector *connector) newmode->type = (DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER); drm_mode_probed_add(connector, newmode); + + ret++; } }
@@ -1594,6 +1604,7 @@ end: } }
+ return ret; }
static int intel_sdvo_get_modes(struct drm_connector *connector) @@ -1601,13 +1612,11 @@ static int intel_sdvo_get_modes(struct drm_connector *connector) struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector);
if (IS_TV(intel_sdvo_connector)) - intel_sdvo_get_tv_modes(connector); + return intel_sdvo_get_tv_modes(connector); else if (IS_LVDS(intel_sdvo_connector)) - intel_sdvo_get_lvds_modes(connector); + return intel_sdvo_get_lvds_modes(connector); else - intel_sdvo_get_ddc_modes(connector); - - return !list_empty(&connector->probed_modes); + return intel_sdvo_get_ddc_modes(connector); }
static void
A 30 ms delay is simply way too big to waste cpu cycles on.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_sdvo.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 6056603..efa0d17 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1370,7 +1370,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
/* add 30ms delay when the output type might be TV */ if (intel_sdvo->caps.output_flags & SDVO_TV_MASK) - mdelay(30); + msleep(30);
if (!intel_sdvo_read_response(intel_sdvo, &response, 2)) return connector_status_unknown;
On Thu, May 24, 2012 at 09:26:49PM +0200, Daniel Vetter wrote:
A 30 ms delay is simply way too big to waste cpu cycles on.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
I've queued this patch here for -next with Chris' irc ack added. -Daniel
Is there a reason for the 30 ms delay in the first place? -Satyeshwar
-----Original Message----- From: dri-devel-bounces+satyeshwar.singh=intel.com@lists.freedesktop.org [mailto:dri-devel-bounces+satyeshwar.singh=intel.com@lists.freedesktop.org] On Behalf Of Daniel Vetter Sent: Thursday, May 31, 2012 1:29 AM To: Intel Graphics Development; DRI Development Cc: Daniel Vetter Subject: Re: [PATCH 14/14] drm/i915: s/mdelay/msleep/ in the sdvo detect function
On Thu, May 24, 2012 at 09:26:49PM +0200, Daniel Vetter wrote:
A 30 ms delay is simply way too big to waste cpu cycles on.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
I've queued this patch here for -next with Chris' irc ack added. -Daniel
On Thu, May 31, 2012 at 9:24 PM, Singh, Satyeshwar satyeshwar.singh@intel.com wrote:
Is there a reason for the 30 ms delay in the first place?
git blame says there is, it supposedely fixes a bug with tv detection. -Daniel
On Thu, May 24, 2012 at 3:26 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Hi all,
I've got fed up with our sorry state of connector detection and rampant edid re and rere-reading.
This patch series lays the groundwork in the drm helpers so that drivers can avoid all this madness (at least on working hw) and properly cache the edid.
With the additional changes for drm/i915, the edid is now read _once_ per plug event (or at boot-up/resume time). A further step would be to integrate the hotplug handling into the driver itself and only call ->detect on the connectors for which the irq handler received a hotplug event.
By adding POLL_FORCE drivers can get back the old behaviour of calling ->detect every time probe_single_connector is called from userspace. I've splattered that over all drivers where I've thought it might be required.
Note though that setting this doesn't avoid all regressions - the regular output poll work will still ignore any connectors with POLL_HPD set. If a driver/hw with broken hpd got away due to this, this will break stuff. But that should be easy to fix by admitting the defeat and setting POLL_CONNECT|DISCONNECT directly.
If other people want to convert over their drivers, the following steps are required:
- Ensure that the connector status is unknown every time the driver could have missed a hpd event (e.g. after resume). drm_mode_config_reset will do that for you.
- Drop the POLL_FORCE flag for connectors where hdp is fully reliable.
- Implement edid caching - that's a nice way to figure out whether hpd is actually reliable, because if it isn't, this step will ensure that you get bug reports because the the edid won't ever get updated ;-)
- Optionally teach the driver some smarts about which specific connectors actually got a hotplug event. Mostly useful on cheap hw (like intel's) that can't distinguish between hdmi and dp without trying some aux channel transfers.
As you can guess from the patch series, I've discovered the hard way that i915 sdvo support is totally broken. Tested on most of the intel machines I have and also quickly on my radeon hd5000.
Comments, flames, ideas and test reports highly welcome.
This set looks good to me. Do you have plans to revive this? At least the common drm ones are much better than what we currently have.
Alex
Cheers, Daniel
Daniel Vetter (14): drm: extract drm_kms_helper_hotplug_event drm: handle HDP and polled connectors separately drm: introduce DRM_CONNECTOR_POLL_FORCE drm/i915: set POLL_FORCE for sdvo outputs drm: properly init/reset connector status drm: kill unnecessary calls to connector->detect drm: don't start the poll engine in probe_single_connector drm: don't unnecessarily enable the polling work drm: don't poll forced connectors drm/i915: cache crt edid drm/i915: cache dp edid drm/i915: cache hdmi edid drm/i915/sdvo: implement correct return value for ->get_modes drm/i915: s/mdelay/msleep/ in the sdvo detect function
drivers/gpu/drm/drm_crtc.c | 6 ++- drivers/gpu/drm/drm_crtc_helper.c | 76 ++++++++++++++++++------- drivers/gpu/drm/exynos/exynos_drm_connector.c | 2 + drivers/gpu/drm/gma500/cdv_intel_crt.c | 2 + drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 2 + drivers/gpu/drm/gma500/mdfld_dsi_output.c | 1 + drivers/gpu/drm/gma500/oaktrail_hdmi.c | 2 + drivers/gpu/drm/gma500/psb_intel_sdvo.c | 2 + drivers/gpu/drm/i915/intel_crt.c | 28 +++++++-- drivers/gpu/drm/i915/intel_dp.c | 47 +++++++-------- drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_hdmi.c | 48 ++++++++++------ drivers/gpu/drm/i915/intel_modes.c | 18 +++++- drivers/gpu/drm/i915/intel_sdvo.c | 45 +++++++++------ drivers/gpu/drm/i915/intel_tv.c | 3 +- drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + drivers/gpu/drm/radeon/radeon_connectors.c | 5 ++ drivers/gpu/drm/udl/udl_connector.c | 2 + drivers/staging/omapdrm/omap_connector.c | 2 + include/drm/drm_crtc.h | 5 ++ include/drm/drm_crtc_helper.h | 1 + 21 files changed, 208 insertions(+), 92 deletions(-)
-- 1.7.7.6
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Oct 04, 2012 at 12:07:01PM -0400, Alex Deucher wrote:
On Thu, May 24, 2012 at 3:26 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Hi all,
I've got fed up with our sorry state of connector detection and rampant edid re and rere-reading.
This patch series lays the groundwork in the drm helpers so that drivers can avoid all this madness (at least on working hw) and properly cache the edid.
With the additional changes for drm/i915, the edid is now read _once_ per plug event (or at boot-up/resume time). A further step would be to integrate the hotplug handling into the driver itself and only call ->detect on the connectors for which the irq handler received a hotplug event.
By adding POLL_FORCE drivers can get back the old behaviour of calling ->detect every time probe_single_connector is called from userspace. I've splattered that over all drivers where I've thought it might be required.
Note though that setting this doesn't avoid all regressions - the regular output poll work will still ignore any connectors with POLL_HPD set. If a driver/hw with broken hpd got away due to this, this will break stuff. But that should be easy to fix by admitting the defeat and setting POLL_CONNECT|DISCONNECT directly.
If other people want to convert over their drivers, the following steps are required:
- Ensure that the connector status is unknown every time the driver could have missed a hpd event (e.g. after resume). drm_mode_config_reset will do that for you.
- Drop the POLL_FORCE flag for connectors where hdp is fully reliable.
- Implement edid caching - that's a nice way to figure out whether hpd is actually reliable, because if it isn't, this step will ensure that you get bug reports because the the edid won't ever get updated ;-)
- Optionally teach the driver some smarts about which specific connectors actually got a hotplug event. Mostly useful on cheap hw (like intel's) that can't distinguish between hdmi and dp without trying some aux channel transfers.
As you can guess from the patch series, I've discovered the hard way that i915 sdvo support is totally broken. Tested on most of the intel machines I have and also quickly on my radeon hd5000.
Comments, flames, ideas and test reports highly welcome.
This set looks good to me. Do you have plans to revive this? At least the common drm ones are much better than what we currently have.
The issue I've meanwhile discovered with hpd handling is that we really need the smarts in the driver to only do probing on native hdmi/dp outputs if we have a hpd irq for that specific port: Since we have 2 encoders for the same port, if e.g. dp is enabled and we do edid probing on the hdmi encoder/connector the display might get unhappy, which can happen even with these patches applied e.g. when pluggin in a 2nd display.
So the new plan (for 3.8 hopefully) is to revamp the i915-internal hpd handling and keep on using the crtc helper stuff. For connectors with reliable hpd we'd simply return the tracked stated in ->detect without touching the hw at all. So I don't think we need any changes in the hotplug/polling helper code. And looking again at these patches, they're a rather decent kludge and it's probably better to do this all in the drivers. Since the helper code sets the force parameter to false for all the background polling, you can just return any driver-internal cached state. And when force=true you could invalidate that cached state or do some additional (more expensive) detection.
In short: No plans to revive this, but certainly plans to improve the drm/i915 hpd handling.
Cheers, Daniel
On Thu, Oct 4, 2012 at 1:16 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Oct 04, 2012 at 12:07:01PM -0400, Alex Deucher wrote:
On Thu, May 24, 2012 at 3:26 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Hi all,
I've got fed up with our sorry state of connector detection and rampant edid re and rere-reading.
This patch series lays the groundwork in the drm helpers so that drivers can avoid all this madness (at least on working hw) and properly cache the edid.
With the additional changes for drm/i915, the edid is now read _once_ per plug event (or at boot-up/resume time). A further step would be to integrate the hotplug handling into the driver itself and only call ->detect on the connectors for which the irq handler received a hotplug event.
By adding POLL_FORCE drivers can get back the old behaviour of calling ->detect every time probe_single_connector is called from userspace. I've splattered that over all drivers where I've thought it might be required.
Note though that setting this doesn't avoid all regressions - the regular output poll work will still ignore any connectors with POLL_HPD set. If a driver/hw with broken hpd got away due to this, this will break stuff. But that should be easy to fix by admitting the defeat and setting POLL_CONNECT|DISCONNECT directly.
If other people want to convert over their drivers, the following steps are required:
- Ensure that the connector status is unknown every time the driver could have missed a hpd event (e.g. after resume). drm_mode_config_reset will do that for you.
- Drop the POLL_FORCE flag for connectors where hdp is fully reliable.
- Implement edid caching - that's a nice way to figure out whether hpd is actually reliable, because if it isn't, this step will ensure that you get bug reports because the the edid won't ever get updated ;-)
- Optionally teach the driver some smarts about which specific connectors actually got a hotplug event. Mostly useful on cheap hw (like intel's) that can't distinguish between hdmi and dp without trying some aux channel transfers.
As you can guess from the patch series, I've discovered the hard way that i915 sdvo support is totally broken. Tested on most of the intel machines I have and also quickly on my radeon hd5000.
Comments, flames, ideas and test reports highly welcome.
This set looks good to me. Do you have plans to revive this? At least the common drm ones are much better than what we currently have.
The issue I've meanwhile discovered with hpd handling is that we really need the smarts in the driver to only do probing on native hdmi/dp outputs if we have a hpd irq for that specific port: Since we have 2 encoders for the same port, if e.g. dp is enabled and we do edid probing on the hdmi encoder/connector the display might get unhappy, which can happen even with these patches applied e.g. when pluggin in a 2nd display.
So the new plan (for 3.8 hopefully) is to revamp the i915-internal hpd handling and keep on using the crtc helper stuff. For connectors with reliable hpd we'd simply return the tracked stated in ->detect without touching the hw at all. So I don't think we need any changes in the hotplug/polling helper code. And looking again at these patches, they're a rather decent kludge and it's probably better to do this all in the drivers. Since the helper code sets the force parameter to false for all the background polling, you can just return any driver-internal cached state. And when force=true you could invalidate that cached state or do some additional (more expensive) detection.
Well the current code skips all hotplug event propagation if the poll option is disabled (even for hpd capable connectors) which is fail so the first few patches still seem like a win. I guess you are not planning to use any of the current common hpd code? If so, do you mind if we pull at least the first few patches in in the interim for other drivers?
Alex
In short: No plans to revive this, but certainly plans to improve the drm/i915 hpd handling.
Cheers, Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Adam Jackson -
Alex Deucher -
Daniel Vetter -
Daniel Vetter -
Singh, Satyeshwar