dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] hpd rework, relaunched
@ 2012-10-23 18:23 Daniel Vetter
  2012-10-23 18:23 ` [PATCH 1/7] drm: extract drm_kms_helper_hotplug_event Daniel Vetter
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

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(-)

-- 
1.7.11.7

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/7] drm: extract drm_kms_helper_hotplug_event
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
@ 2012-10-23 18:23 ` Daniel Vetter
  2012-10-23 18:23 ` [PATCH 2/7] drm: handle HPD and polled connectors separately Daniel Vetter
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

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);
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/7] drm: handle HPD and polled connectors separately
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
  2012-10-23 18:23 ` [PATCH 1/7] drm: extract drm_kms_helper_hotplug_event Daniel Vetter
@ 2012-10-23 18:23 ` Daniel Vetter
  2012-10-23 18:23 ` [PATCH 3/7] drm: run the hpd irq event code directly Daniel Vetter
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

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;
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/7] drm: run the hpd irq event code directly
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
  2012-10-23 18:23 ` [PATCH 1/7] drm: extract drm_kms_helper_hotplug_event Daniel Vetter
  2012-10-23 18:23 ` [PATCH 2/7] drm: handle HPD and polled connectors separately Daniel Vetter
@ 2012-10-23 18:23 ` Daniel Vetter
  2012-10-23 18:23 ` [PATCH 4/7] drm: properly init/reset connector status Daniel Vetter
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

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;
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/7] drm: properly init/reset connector status
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
                   ` (2 preceding siblings ...)
  2012-10-23 18:23 ` [PATCH 3/7] drm: run the hpd irq event code directly Daniel Vetter
@ 2012-10-23 18:23 ` Daniel Vetter
  2012-10-23 18:23 ` [PATCH 5/7] drm: don't start the poll engine in probe_single_connector Daniel Vetter
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

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);
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/7] drm: don't start the poll engine in probe_single_connector
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
                   ` (3 preceding siblings ...)
  2012-10-23 18:23 ` [PATCH 4/7] drm: properly init/reset connector status Daniel Vetter
@ 2012-10-23 18:23 ` Daniel Vetter
  2012-10-23 18:23 ` [PATCH 6/7] drm: don't unnecessarily enable the polling work Daniel Vetter
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

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 */
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 6/7] drm: don't unnecessarily enable the polling work
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
                   ` (4 preceding siblings ...)
  2012-10-23 18:23 ` [PATCH 5/7] drm: don't start the poll engine in probe_single_connector Daniel Vetter
@ 2012-10-23 18:23 ` Daniel Vetter
  2012-10-24 13:35   ` [PATCH] " Daniel Vetter
  2012-10-23 18:23 ` [PATCH 7/7] drm: don't poll forced connectors Daniel Vetter
  2012-10-23 20:41 ` [PATCH 0/7] hpd rework, relaunched Alex Deucher
  7 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

... 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;
 	}
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 7/7] drm: don't poll forced connectors
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
                   ` (5 preceding siblings ...)
  2012-10-23 18:23 ` [PATCH 6/7] drm: don't unnecessarily enable the polling work Daniel Vetter
@ 2012-10-23 18:23 ` Daniel Vetter
  2012-10-23 20:41 ` [PATCH 0/7] hpd rework, relaunched Alex Deucher
  7 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-23 18:23 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

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)
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/7] hpd rework, relaunched
  2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
                   ` (6 preceding siblings ...)
  2012-10-23 18:23 ` [PATCH 7/7] drm: don't poll forced connectors Daniel Vetter
@ 2012-10-23 20:41 ` Alex Deucher
  7 siblings, 0 replies; 10+ messages in thread
From: Alex Deucher @ 2012-10-23 20:41 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: DRI Development

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
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] drm: don't unnecessarily enable the polling work
  2012-10-23 18:23 ` [PATCH 6/7] drm: don't unnecessarily enable the polling work Daniel Vetter
@ 2012-10-24 13:35   ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-24 13:35 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter

... 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;
 	}
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-10-24 14:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-23 18:23 [PATCH 0/7] hpd rework, relaunched Daniel Vetter
2012-10-23 18:23 ` [PATCH 1/7] drm: extract drm_kms_helper_hotplug_event Daniel Vetter
2012-10-23 18:23 ` [PATCH 2/7] drm: handle HPD and polled connectors separately Daniel Vetter
2012-10-23 18:23 ` [PATCH 3/7] drm: run the hpd irq event code directly Daniel Vetter
2012-10-23 18:23 ` [PATCH 4/7] drm: properly init/reset connector status Daniel Vetter
2012-10-23 18:23 ` [PATCH 5/7] drm: don't start the poll engine in probe_single_connector Daniel Vetter
2012-10-23 18:23 ` [PATCH 6/7] drm: don't unnecessarily enable the polling work Daniel Vetter
2012-10-24 13:35   ` [PATCH] " Daniel Vetter
2012-10-23 18:23 ` [PATCH 7/7] drm: don't poll forced connectors Daniel Vetter
2012-10-23 20:41 ` [PATCH 0/7] hpd rework, relaunched Alex Deucher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).