Hi all,
I've always been a bit unhappy with the overall approach of my old hpd patches, I think they've tried to move too much clever logic into the helper code. Now we can still make the helper code a bit more smarter and flexible, but I think the really clever hpd handling code should be in the driver specific parts. This this series will allow: - Better separation of polling and hpd event handling. While still allowing a connector to be handled by both (for connectors where hpd is a bit unreliable). - Fixing up some corner cases in the polling, to avoid flip-flop behaviour and races. This should prevent spurious wakeups of userspace with hotplug events. - A tiny bit of code to make the unkown state a tad more useful.
Originally I wanted to wait with submitting this until I have the full i915 hpd rework ready. But Alex Deucher pinged me a few times too often on irc, and I agree, this pile should be useful in and of itself.
Comments, flames and review highly welcome.
Cheers, Daniel
Daniel Vetter (7): drm: extract drm_kms_helper_hotplug_event drm: handle HPD and polled connectors separately drm: run the hpd irq event code directly drm: properly init/reset connector status 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
drivers/gpu/drm/drm_crtc.c | 6 +++- drivers/gpu/drm/drm_crtc_helper.c | 71 ++++++++++++++++++++++++++++----------- include/drm/drm_crtc.h | 1 + include/drm/drm_crtc_helper.h | 1 + 4 files changed, 59 insertions(+), 20 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 1227adf..accfd3b 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) schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD); diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index e01cc80..8f2842c 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -162,6 +162,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 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.
v4: In the hpd_irq_event handler don't bail out if other bits than HPD are set. This is useful where e.g. hpd is unreliably, but mostly works. Drivers can then set both HPD and POLL flags, and users get the best of both worlds: Quick hotplug feedback if the hpd works, but still reliable detection with the polling. The poll loop already works the same, and doesn't bail if HPD is set.
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 accfd3b..7342f61 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) - schedule_delayed_work(&dev->mode_config.output_poll_work, 0); + schedule_delayed_work(&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 3fa18b7..1052348 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -793,6 +793,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 insertion(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 7342f61..654080b 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) - schedule_delayed_work(&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 1052348..3fa18b7 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -793,7 +793,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;
This can help drivers to make somewhat intelligent decisions in their ->detect callback: If the connector is hpd capable and in the unknown state, the driver needs to force a full detect cycle. Otherwise it could just (if it chooses so) to update the connector state from it's hpd handler directly, and always return that in the ->detect callback.
Atm only drm/i915 calls drm_mode_config_reset at resume time, so other drivers would need to add that call first before using this facility.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ef1b221..05c4e7b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -555,6 +555,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++; @@ -3656,9 +3657,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);
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 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 654080b..bb94b6d 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -109,9 +109,14 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, connector->funcs->force(connector); } else { 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 3fa18b7..89f8f7f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -792,6 +792,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 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index bb94b6d..bd0574a 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -952,9 +952,6 @@ static void output_poll_execute(struct work_struct *work) if (!connector->polled || 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 */ @@ -997,7 +994,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; }
... 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.
v2: Chris Wilson noticed that I broke polling, since repoll will never ever be set true. Fix this up, and simplify the logic a bit while at it.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc_helper.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index bb94b6d..6f03afb 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -952,8 +952,7 @@ static void output_poll_execute(struct work_struct *work) if (!connector->polled || connector->polled == DRM_CONNECTOR_POLL_HPD) continue;
- else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) - repoll = true; + repoll = true;
old_status = connector->status; /* if we are connected and don't want to poll for disconnect @@ -997,7 +996,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 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index bd0574a..e2967e6 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -947,6 +947,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 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)
On Tue, Oct 23, 2012 at 2:23 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Hi all,
I've always been a bit unhappy with the overall approach of my old hpd patches, I think they've tried to move too much clever logic into the helper code. Now we can still make the helper code a bit more smarter and flexible, but I think the really clever hpd handling code should be in the driver specific parts. This this series will allow:
- Better separation of polling and hpd event handling. While still allowing a connector to be handled by both (for connectors where hpd is a bit unreliable).
- Fixing up some corner cases in the polling, to avoid flip-flop behaviour and races. This should prevent spurious wakeups of userspace with hotplug events.
- A tiny bit of code to make the unkown state a tad more useful.
Most importantly, it allows you to still get hpd hotplug events when you have disabled polling. The old code disabled all hotplug events when polling was disabled since they shared the same code path.
Originally I wanted to wait with submitting this until I have the full i915 hpd rework ready. But Alex Deucher pinged me a few times too often on irc, and I agree, this pile should be useful in and of itself.
For the series:
Reviewed-by: Alex Deucher alexander.deucher@amd.com
Comments, flames and review highly welcome.
Cheers, Daniel
Daniel Vetter (7): drm: extract drm_kms_helper_hotplug_event drm: handle HPD and polled connectors separately drm: run the hpd irq event code directly drm: properly init/reset connector status 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
drivers/gpu/drm/drm_crtc.c | 6 +++- drivers/gpu/drm/drm_crtc_helper.c | 71 ++++++++++++++++++++++++++++----------- include/drm/drm_crtc.h | 1 + include/drm/drm_crtc_helper.h | 1 + 4 files changed, 59 insertions(+), 20 deletions(-)
-- 1.7.11.7
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter