[PATCH 1/2] drm/probe-helper: don't lose hotplug event

[PATCH 1/2] drm/probe-helper: don't lose hotplug event

[Daniel Vetter]

There's a race window (small for hpd, 10s large for polled outputs)
where userspace could sneak in with an unrelated connnector probe
ioctl call and eat the hotplug event (since neither the hpd nor the
poll code see a state change).

To avoid this, check whether the connector state changes in all other
->detect calls (in the current helper code that's only probe_single)
and if that's the case, fire off a hotplug event. Note that we can't
directly call the hotplug event handler, since that expects that no
locks are held (due to reentrancy with the fb code to update the kms
console).

Also, this requires that drivers using the probe_single helper
function set up the poll work. All current drivers do that already,
and with the reworked hpd handling there'll be no downside to
unconditionally setting up the poll work any more.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_probe_helper.c | 32 +++++++++++++++++++++++++++++++-
 include/drm/drm_crtc.h             |  1 +
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 2fbdcca7ca9a..be9eca7f41d2 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -103,6 +103,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 	int count = 0;
 	int mode_flags = 0;
 	bool verbose_prune = true;
+	enum drm_connector_status old_status;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 
@@ -121,7 +122,32 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 		if (connector->funcs->force)
 			connector->funcs->force(connector);
 	} else {
+		old_status = connector->status;
+
 		connector->status = connector->funcs->detect(connector, true);
+
+		/*
+		 * Normally either the driver's hpd code or the poll loop should
+		 * pick up any changes and fire the hotplug event. But if
+		 * userspace sneaks in a probe, we might miss a change. Hence
+		 * check here, and if anything changed start the hotplug code.
+		 */
+		if (old_status != connector->status) {
+			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
+				      connector->base.id,
+				      connector->name,
+				      old_status, connector->status);
+
+			/*
+			 * The hotplug event code might call into the fb
+			 * helpers, and so expects that we do not hold any
+			 * locks. Fire up the poll struct instead, it will
+			 * disable itself again.
+			 */
+			dev->mode_config.delayed_event = true;
+			schedule_delayed_work(&dev->mode_config.output_poll_work,
+					      0);
+		}
 	}
 
 	/* Re-enable polling in case the global poll config changed. */
@@ -274,7 +300,11 @@ static void output_poll_execute(struct work_struct *work)
 	struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
 	struct drm_connector *connector;
 	enum drm_connector_status old_status;
-	bool repoll = false, changed = false;
+	bool repoll = false, changed;
+
+	/* Pick up any changes detected by the probe functions. */
+	changed = dev->mode_config.delayed_event;
+	dev->mode_config.delayed_event = false;
 
 	if (!drm_kms_helper_poll)
 		return;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8b626ff3d3bd..397203647f4a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1112,6 +1112,7 @@ struct drm_mode_config {
 	/* output poll support */
 	bool poll_enabled;
 	bool poll_running;
+	bool delayed_event;
 	struct delayed_work output_poll_work;
 
 	/* pointers to standard properties */
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/probe-helper: clamp unknown connector status in the poll work

[Daniel Vetter]

On some chipset we try to avoid possibly invasive output detection
methods (like load detect which can cause flickering elsewhere) in the
output poll work. Drivers could hence return unknown when a previous
full ->detect call returned a different state.

This change will generate a hotplug event, forcing userspace to do a
full scan. This in turn updates the connector->status field so that we
will _again_ get a state change when the hotplug work re-runs in 10
seconds.

To avoid this ping-pong loop detect this situation and clamp the
connector state to the old value.

Patch is inspired by a patch from Knut Peterson. Knut's patch
completely ignored connector state changes if either the old or new
status was unknown, which seemed to be a bit too agressive to me.

v2: Rebased onto the drm_probe_helper.c extraction.

References: http://lists.freedesktop.org/archives/dri-devel/2012-August/025975.html
Cc: Knut Petersen <Knut_Petersen@t-online.de>
Cc: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_probe_helper.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index be9eca7f41d2..776a48c5de74 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -334,6 +334,24 @@ static void output_poll_execute(struct work_struct *work)
 		if (old_status != connector->status) {
 			const char *old, *new;
 
+			/*
+			 * The poll work sets force=false when calling detect so
+			 * that drivers can avoid to do disruptive tests (e.g.
+			 * when load detect cycles could cause flickering on
+			 * other, running displays). This bears the risk that we
+			 * flip-flop between unknown here in the poll work and
+			 * the real state when userspace forces a full detect
+			 * call after receiving a hotplug event due to this
+			 * change.
+			 *
+			 * Hence clamp an unknown detect status to the old
+			 * value.
+			 */
+			if (connector->status == connector_status_unknown) {
+				connector->status = old_status;
+				continue;
+			}
+
 			old = drm_get_connector_status_name(old_status);
 			new = drm_get_connector_status_name(connector->status);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/probe-helper: don't lose hotplug event

[Daniel Vetter]

There's a race window (small for hpd, 10s large for polled outputs)
where userspace could sneak in with an unrelated connnector probe
ioctl call and eat the hotplug event (since neither the hpd nor the
poll code see a state change).

To avoid this, check whether the connector state changes in all other
->detect calls (in the current helper code that's only probe_single)
and if that's the case, fire off a hotplug event. Note that we can't
directly call the hotplug event handler, since that expects that no
locks are held (due to reentrancy with the fb code to update the kms
console).

Also, this requires that drivers using the probe_single helper
function set up the poll work. All current drivers do that already,
and with the reworked hpd handling there'll be no downside to
unconditionally setting up the poll work any more.

v2: Review from Rob Clark
- Don't bail out of the output poll work immediately if it's disabled
  to make sure we deliver the delayed hoptplug events. Instead just
  jump to the tail.
- Don't scheduel the work when it's not set up. Would be a driver bug
  since using probe helpers for anything dynamic without them
  initialized makes them all noops.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_probe_helper.c | 36 ++++++++++++++++++++++++++++++++++--
 include/drm/drm_crtc.h             |  1 +
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 2fbdcca7ca9a..33bf550a1d3f 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -103,6 +103,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 	int count = 0;
 	int mode_flags = 0;
 	bool verbose_prune = true;
+	enum drm_connector_status old_status;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 
@@ -121,7 +122,33 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 		if (connector->funcs->force)
 			connector->funcs->force(connector);
 	} else {
+		old_status = connector->status;
+
 		connector->status = connector->funcs->detect(connector, true);
+
+		/*
+		 * Normally either the driver's hpd code or the poll loop should
+		 * pick up any changes and fire the hotplug event. But if
+		 * userspace sneaks in a probe, we might miss a change. Hence
+		 * check here, and if anything changed start the hotplug code.
+		 */
+		if (old_status != connector->status) {
+			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
+				      connector->base.id,
+				      connector->name,
+				      old_status, connector->status);
+
+			/*
+			 * The hotplug event code might call into the fb
+			 * helpers, and so expects that we do not hold any
+			 * locks. Fire up the poll struct instead, it will
+			 * disable itself again.
+			 */
+			dev->mode_config.delayed_event = true;
+			if (dev->mode_config.poll_enabled)
+				schedule_delayed_work(&dev->mode_config.output_poll_work,
+						      0);
+		}
 	}
 
 	/* Re-enable polling in case the global poll config changed. */
@@ -274,10 +301,14 @@ static void output_poll_execute(struct work_struct *work)
 	struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
 	struct drm_connector *connector;
 	enum drm_connector_status old_status;
-	bool repoll = false, changed = false;
+	bool repoll = false, changed;
+
+	/* Pick up any changes detected by the probe functions. */
+	changed = dev->mode_config.delayed_event;
+	dev->mode_config.delayed_event = false;
 
 	if (!drm_kms_helper_poll)
-		return;
+		goto out;
 
 	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -319,6 +350,7 @@ static void output_poll_execute(struct work_struct *work)
 
 	mutex_unlock(&dev->mode_config.mutex);
 
+out:
 	if (changed)
 		drm_kms_helper_hotplug_event(dev);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f444263055c5..65da9fb939a7 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1089,6 +1089,7 @@ struct drm_mode_config {
 	/* output poll support */
 	bool poll_enabled;
 	bool poll_running;
+	bool delayed_event;
 	struct delayed_work output_poll_work;
 
 	/* pointers to standard properties */
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/probe-helper: don't lose hotplug event

[Rob Clark]

On Wed, Jan 21, 2015 at 9:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> There's a race window (small for hpd, 10s large for polled outputs)
> where userspace could sneak in with an unrelated connnector probe
> ioctl call and eat the hotplug event (since neither the hpd nor the
> poll code see a state change).
>
> To avoid this, check whether the connector state changes in all other
> ->detect calls (in the current helper code that's only probe_single)
> and if that's the case, fire off a hotplug event. Note that we can't
> directly call the hotplug event handler, since that expects that no
> locks are held (due to reentrancy with the fb code to update the kms
> console).
>
> Also, this requires that drivers using the probe_single helper
> function set up the poll work. All current drivers do that already,
> and with the reworked hpd handling there'll be no downside to
> unconditionally setting up the poll work any more.
>
> v2: Review from Rob Clark
> - Don't bail out of the output poll work immediately if it's disabled
>   to make sure we deliver the delayed hoptplug events. Instead just
>   jump to the tail.
> - Don't scheduel the work when it's not set up. Would be a driver bug
>   since using probe helpers for anything dynamic without them
>   initialized makes them all noops.
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

and with the monitor that started all this, plus the addition of:
http://lists.freedesktop.org/archives/dri-devel/2015-January/075901.html

Tested-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_probe_helper.c | 36 ++++++++++++++++++++++++++++++++++--
>  include/drm/drm_crtc.h             |  1 +
>  2 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 2fbdcca7ca9a..33bf550a1d3f 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -103,6 +103,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>         int count = 0;
>         int mode_flags = 0;
>         bool verbose_prune = true;
> +       enum drm_connector_status old_status;
>
>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>
> @@ -121,7 +122,33 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>                 if (connector->funcs->force)
>                         connector->funcs->force(connector);
>         } else {
> +               old_status = connector->status;
> +
>                 connector->status = connector->funcs->detect(connector, true);
> +
> +               /*
> +                * Normally either the driver's hpd code or the poll loop should
> +                * pick up any changes and fire the hotplug event. But if
> +                * userspace sneaks in a probe, we might miss a change. Hence
> +                * check here, and if anything changed start the hotplug code.
> +                */
> +               if (old_status != connector->status) {
> +                       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
> +                                     connector->base.id,
> +                                     connector->name,
> +                                     old_status, connector->status);
> +
> +                       /*
> +                        * The hotplug event code might call into the fb
> +                        * helpers, and so expects that we do not hold any
> +                        * locks. Fire up the poll struct instead, it will
> +                        * disable itself again.
> +                        */
> +                       dev->mode_config.delayed_event = true;
> +                       if (dev->mode_config.poll_enabled)
> +                               schedule_delayed_work(&dev->mode_config.output_poll_work,
> +                                                     0);
> +               }
>         }
>
>         /* Re-enable polling in case the global poll config changed. */
> @@ -274,10 +301,14 @@ static void output_poll_execute(struct work_struct *work)
>         struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
>         struct drm_connector *connector;
>         enum drm_connector_status old_status;
> -       bool repoll = false, changed = false;
> +       bool repoll = false, changed;
> +
> +       /* Pick up any changes detected by the probe functions. */
> +       changed = dev->mode_config.delayed_event;
> +       dev->mode_config.delayed_event = false;
>
>         if (!drm_kms_helper_poll)
> -               return;
> +               goto out;
>
>         mutex_lock(&dev->mode_config.mutex);
>         list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> @@ -319,6 +350,7 @@ static void output_poll_execute(struct work_struct *work)
>
>         mutex_unlock(&dev->mode_config.mutex);
>
> +out:
>         if (changed)
>                 drm_kms_helper_hotplug_event(dev);
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f444263055c5..65da9fb939a7 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1089,6 +1089,7 @@ struct drm_mode_config {
>         /* output poll support */
>         bool poll_enabled;
>         bool poll_running;
> +       bool delayed_event;
>         struct delayed_work output_poll_work;
>
>         /* pointers to standard properties */
> --
> 2.1.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/probe-helper: clamp unknown connector status in the poll work

[Rob Clark]

On Wed, Jan 21, 2015 at 2:45 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On some chipset we try to avoid possibly invasive output detection
> methods (like load detect which can cause flickering elsewhere) in the
> output poll work. Drivers could hence return unknown when a previous
> full ->detect call returned a different state.
>
> This change will generate a hotplug event, forcing userspace to do a
> full scan. This in turn updates the connector->status field so that we
> will _again_ get a state change when the hotplug work re-runs in 10
> seconds.
>
> To avoid this ping-pong loop detect this situation and clamp the
> connector state to the old value.
>
> Patch is inspired by a patch from Knut Peterson. Knut's patch
> completely ignored connector state changes if either the old or new
> status was unknown, which seemed to be a bit too agressive to me.
>
> v2: Rebased onto the drm_probe_helper.c extraction.
>
> References: http://lists.freedesktop.org/archives/dri-devel/2012-August/025975.html
> Cc: Knut Petersen <Knut_Petersen@t-online.de>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_probe_helper.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index be9eca7f41d2..776a48c5de74 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -334,6 +334,24 @@ static void output_poll_execute(struct work_struct *work)
>                 if (old_status != connector->status) {
>                         const char *old, *new;
>
> +                       /*
> +                        * The poll work sets force=false when calling detect so
> +                        * that drivers can avoid to do disruptive tests (e.g.
> +                        * when load detect cycles could cause flickering on
> +                        * other, running displays). This bears the risk that we
> +                        * flip-flop between unknown here in the poll work and
> +                        * the real state when userspace forces a full detect
> +                        * call after receiving a hotplug event due to this
> +                        * change.
> +                        *
> +                        * Hence clamp an unknown detect status to the old
> +                        * value.
> +                        */
> +                       if (connector->status == connector_status_unknown) {
> +                               connector->status = old_status;
> +                               continue;
> +                       }
> +
>                         old = drm_get_connector_status_name(old_status);
>                         new = drm_get_connector_status_name(connector->status);
>
> --
> 2.1.4
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/probe-helper: clamp unknown connector status in the poll work

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5619
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  353/353              353/353
SNB                                  400/422              400/422
IVB                                  487/487              487/487
BYT                                  296/296              296/296
HSW                                  508/508              508/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/probe-helper: don't lose hotplug event

[Rob Clark]

On Wed, Jan 21, 2015 at 4:40 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Jan 21, 2015 at 9:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> There's a race window (small for hpd, 10s large for polled outputs)
>> where userspace could sneak in with an unrelated connnector probe
>> ioctl call and eat the hotplug event (since neither the hpd nor the
>> poll code see a state change).
>>
>> To avoid this, check whether the connector state changes in all other
>> ->detect calls (in the current helper code that's only probe_single)
>> and if that's the case, fire off a hotplug event. Note that we can't
>> directly call the hotplug event handler, since that expects that no
>> locks are held (due to reentrancy with the fb code to update the kms
>> console).
>>
>> Also, this requires that drivers using the probe_single helper
>> function set up the poll work. All current drivers do that already,
>> and with the reworked hpd handling there'll be no downside to
>> unconditionally setting up the poll work any more.
>>
>> v2: Review from Rob Clark
>> - Don't bail out of the output poll work immediately if it's disabled
>>   to make sure we deliver the delayed hoptplug events. Instead just
>>   jump to the tail.
>> - Don't scheduel the work when it's not set up. Would be a driver bug
>>   since using probe helpers for anything dynamic without them
>>   initialized makes them all noops.
>>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
>> Cc: Rob Clark <robdclark@gmail.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Reviewed-by: Rob Clark <robdclark@gmail.com>
>
> and with the monitor that started all this, plus the addition of:
> http://lists.freedesktop.org/archives/dri-devel/2015-January/075901.html
>
> Tested-by: Rob Clark <robdclark@gmail.com>

bleh, but on latest upstream (plus drm-next, etc).. possibly just not
caught earlier due to insufficient debug stuff enabled..

I could probably solve this by moving drm_kms_helper_poll_init()
earlier.. otoh that might cause other surprises and other drivers also
seem to do this late in their load..

[    4.053197] WARNING: CPU: 3 PID: 52 at ../kernel/workqueue.c:1427
__queue_delayed_work+0x14c/0x15c()
[    4.058040] Modules linked in:
[    4.070017] CPU: 3 PID: 52 Comm: kworker/u8:1 Tainted: G     U
   3.19.0-rc5-00821-g415f9e0-dirty #1113
[    4.070107] Hardware name: Qualcomm (Flattened Device Tree)
[    4.080016] Workqueue: deferwq deferred_probe_work_func
[    4.090549] [<c021654c>] (unwind_backtrace) from [<c0211f68>]
(show_stack+0x10/0x14)
[    4.090714] [<c0211f68>] (show_stack) from [<c091d3bc>]
(dump_stack+0x8c/0x9c)
[    4.098519] [<c091d3bc>] (dump_stack) from [<c0246a2c>]
(warn_slowpath_common+0x84/0xb4)
[    4.105547] [<c0246a2c>] (warn_slowpath_common) from [<c0246af8>]
(warn_slowpath_null+0x1c/0x24)
[    4.113790] [<c0246af8>] (warn_slowpath_null) from [<c0259b5c>]
(__queue_delayed_work+0x14c/0x15c)
[    4.122556] [<c0259b5c>] (__queue_delayed_work) from [<c0259bbc>]
(queue_delayed_work_on+0x50/0x5c)
[    4.131337] [<c0259bbc>] (queue_delayed_work_on) from [<c0561114>]
(drm_helper_probe_single_connector_modes_merge_bits+0x2fc/0x49c)
[    4.140290] [<c0561114>]
(drm_helper_probe_single_connector_modes_merge_bits) from [<c0569824>]
(drm_fb_helper_probe_connector_modes.isra.5+0x4c/0x6c)
[    4.152089] [<c0569824>]
(drm_fb_helper_probe_connector_modes.isra.5) from [<c056a8a8>]
(drm_fb_helper_initial_config+0x34/0x3d8)
[    4.165624] [<c056a8a8>] (drm_fb_helper_initial_config) from
[<c05a62dc>] (msm_fbdev_init+0x70/0xa0)
[    4.177342] [<c05a62dc>] (msm_fbdev_init) from [<c05a1614>]
(msm_load+0x268/0x36c)
[    4.186537] [<c05a1614>] (msm_load) from [<c0573af4>]
(drm_dev_register+0xa8/0x104)
[    4.193912] [<c0573af4>] (drm_dev_register) from [<c05759dc>]
(drm_platform_init+0x44/0xd8)
[    4.201475] [<c05759dc>] (drm_platform_init) from [<c05c1654>]
(try_to_bring_up_master.part.3+0xc8/0x108)
[    4.209804] [<c05c1654>] (try_to_bring_up_master.part.3) from
[<c05c1740>] (component_master_add_with_match+0xac/0x124)
[    4.219525] [<c05c1740>] (component_master_add_with_match) from
[<c05a116c>] (msm_pdev_probe+0x64/0x70)
[    4.230116] [<c05a116c>] (msm_pdev_probe) from [<c05c73c8>]
(platform_drv_probe+0x44/0xa4)
[    4.239485] [<c05c73c8>] (platform_drv_probe) from [<c05c5c48>]
(driver_probe_device+0x108/0x23c)
[    4.247813] [<c05c5c48>] (driver_probe_device) from [<c05c42ac>]
(bus_for_each_drv+0x64/0x98)
[    4.256752] [<c05c42ac>] (bus_for_each_drv) from [<c05c5b10>]
(device_attach+0x74/0x88)
[    4.265259] [<c05c5b10>] (device_attach) from [<c05c51fc>]
(bus_probe_device+0x84/0xa8)
[    4.273071] [<c05c51fc>] (bus_probe_device) from [<c05c561c>]
(deferred_probe_work_func+0x64/0x94)
[    4.281064] [<c05c561c>] (deferred_probe_work_func) from
[<c025a354>] (process_one_work+0x134/0x334)
[    4.290091] [<c025a354>] (process_one_work) from [<c025ac30>]
(worker_thread+0x4c/0x4b0)
[    4.299383] [<c025ac30>] (worker_thread) from [<c025ecfc>]
(kthread+0xdc/0xf4)
[    4.307459] [<c025ecfc>] (kthread) from [<c020e978>]
(ret_from_fork+0x14/0x3c)

BR,
-R

>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c | 36 ++++++++++++++++++++++++++++++++++--
>>  include/drm/drm_crtc.h             |  1 +
>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 2fbdcca7ca9a..33bf550a1d3f 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -103,6 +103,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>>         int count = 0;
>>         int mode_flags = 0;
>>         bool verbose_prune = true;
>> +       enum drm_connector_status old_status;
>>
>>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>>
>> @@ -121,7 +122,33 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>>                 if (connector->funcs->force)
>>                         connector->funcs->force(connector);
>>         } else {
>> +               old_status = connector->status;
>> +
>>                 connector->status = connector->funcs->detect(connector, true);
>> +
>> +               /*
>> +                * Normally either the driver's hpd code or the poll loop should
>> +                * pick up any changes and fire the hotplug event. But if
>> +                * userspace sneaks in a probe, we might miss a change. Hence
>> +                * check here, and if anything changed start the hotplug code.
>> +                */
>> +               if (old_status != connector->status) {
>> +                       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
>> +                                     connector->base.id,
>> +                                     connector->name,
>> +                                     old_status, connector->status);
>> +
>> +                       /*
>> +                        * The hotplug event code might call into the fb
>> +                        * helpers, and so expects that we do not hold any
>> +                        * locks. Fire up the poll struct instead, it will
>> +                        * disable itself again.
>> +                        */
>> +                       dev->mode_config.delayed_event = true;
>> +                       if (dev->mode_config.poll_enabled)
>> +                               schedule_delayed_work(&dev->mode_config.output_poll_work,
>> +                                                     0);
>> +               }
>>         }
>>
>>         /* Re-enable polling in case the global poll config changed. */
>> @@ -274,10 +301,14 @@ static void output_poll_execute(struct work_struct *work)
>>         struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
>>         struct drm_connector *connector;
>>         enum drm_connector_status old_status;
>> -       bool repoll = false, changed = false;
>> +       bool repoll = false, changed;
>> +
>> +       /* Pick up any changes detected by the probe functions. */
>> +       changed = dev->mode_config.delayed_event;
>> +       dev->mode_config.delayed_event = false;
>>
>>         if (!drm_kms_helper_poll)
>> -               return;
>> +               goto out;
>>
>>         mutex_lock(&dev->mode_config.mutex);
>>         list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>> @@ -319,6 +350,7 @@ static void output_poll_execute(struct work_struct *work)
>>
>>         mutex_unlock(&dev->mode_config.mutex);
>>
>> +out:
>>         if (changed)
>>                 drm_kms_helper_hotplug_event(dev);
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index f444263055c5..65da9fb939a7 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1089,6 +1089,7 @@ struct drm_mode_config {
>>         /* output poll support */
>>         bool poll_enabled;
>>         bool poll_running;
>> +       bool delayed_event;
>>         struct delayed_work output_poll_work;
>>
>>         /* pointers to standard properties */
>> --
>> 2.1.4
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/probe-helper: don't lose hotplug event

[Daniel Vetter]

On Sun, Jan 25, 2015 at 12:37:31PM -0500, Rob Clark wrote:
> On Wed, Jan 21, 2015 at 4:40 PM, Rob Clark <robdclark@gmail.com> wrote:
> > On Wed, Jan 21, 2015 at 9:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >> There's a race window (small for hpd, 10s large for polled outputs)
> >> where userspace could sneak in with an unrelated connnector probe
> >> ioctl call and eat the hotplug event (since neither the hpd nor the
> >> poll code see a state change).
> >>
> >> To avoid this, check whether the connector state changes in all other
> >> ->detect calls (in the current helper code that's only probe_single)
> >> and if that's the case, fire off a hotplug event. Note that we can't
> >> directly call the hotplug event handler, since that expects that no
> >> locks are held (due to reentrancy with the fb code to update the kms
> >> console).
> >>
> >> Also, this requires that drivers using the probe_single helper
> >> function set up the poll work. All current drivers do that already,
> >> and with the reworked hpd handling there'll be no downside to
> >> unconditionally setting up the poll work any more.
> >>
> >> v2: Review from Rob Clark
> >> - Don't bail out of the output poll work immediately if it's disabled
> >>   to make sure we deliver the delayed hoptplug events. Instead just
> >>   jump to the tail.
> >> - Don't scheduel the work when it's not set up. Would be a driver bug
> >>   since using probe helpers for anything dynamic without them
> >>   initialized makes them all noops.
> >>
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Reviewed-by: Rob Clark <robdclark@gmail.com>
> >
> > and with the monitor that started all this, plus the addition of:
> > http://lists.freedesktop.org/archives/dri-devel/2015-January/075901.html
> >
> > Tested-by: Rob Clark <robdclark@gmail.com>
> 
> bleh, but on latest upstream (plus drm-next, etc).. possibly just not
> caught earlier due to insufficient debug stuff enabled..
> 
> I could probably solve this by moving drm_kms_helper_poll_init()
> earlier.. otoh that might cause other surprises and other drivers also
> seem to do this late in their load..

Well I'm confused - I check for poll_enabled and that is only set _after_
INIT_DELAYED_WORK. Which should prevent schedule_delayed_work on an
uninitialized work and was the entire point of v2. Nothing else writes to
poll_enabled. What do I miss?

Totally confused ...
-Daniel

> 
> [    4.053197] WARNING: CPU: 3 PID: 52 at ../kernel/workqueue.c:1427
> __queue_delayed_work+0x14c/0x15c()
> [    4.058040] Modules linked in:
> [    4.070017] CPU: 3 PID: 52 Comm: kworker/u8:1 Tainted: G     U
>    3.19.0-rc5-00821-g415f9e0-dirty #1113
> [    4.070107] Hardware name: Qualcomm (Flattened Device Tree)
> [    4.080016] Workqueue: deferwq deferred_probe_work_func
> [    4.090549] [<c021654c>] (unwind_backtrace) from [<c0211f68>]
> (show_stack+0x10/0x14)
> [    4.090714] [<c0211f68>] (show_stack) from [<c091d3bc>]
> (dump_stack+0x8c/0x9c)
> [    4.098519] [<c091d3bc>] (dump_stack) from [<c0246a2c>]
> (warn_slowpath_common+0x84/0xb4)
> [    4.105547] [<c0246a2c>] (warn_slowpath_common) from [<c0246af8>]
> (warn_slowpath_null+0x1c/0x24)
> [    4.113790] [<c0246af8>] (warn_slowpath_null) from [<c0259b5c>]
> (__queue_delayed_work+0x14c/0x15c)
> [    4.122556] [<c0259b5c>] (__queue_delayed_work) from [<c0259bbc>]
> (queue_delayed_work_on+0x50/0x5c)
> [    4.131337] [<c0259bbc>] (queue_delayed_work_on) from [<c0561114>]
> (drm_helper_probe_single_connector_modes_merge_bits+0x2fc/0x49c)
> [    4.140290] [<c0561114>]
> (drm_helper_probe_single_connector_modes_merge_bits) from [<c0569824>]
> (drm_fb_helper_probe_connector_modes.isra.5+0x4c/0x6c)
> [    4.152089] [<c0569824>]
> (drm_fb_helper_probe_connector_modes.isra.5) from [<c056a8a8>]
> (drm_fb_helper_initial_config+0x34/0x3d8)
> [    4.165624] [<c056a8a8>] (drm_fb_helper_initial_config) from
> [<c05a62dc>] (msm_fbdev_init+0x70/0xa0)
> [    4.177342] [<c05a62dc>] (msm_fbdev_init) from [<c05a1614>]
> (msm_load+0x268/0x36c)
> [    4.186537] [<c05a1614>] (msm_load) from [<c0573af4>]
> (drm_dev_register+0xa8/0x104)
> [    4.193912] [<c0573af4>] (drm_dev_register) from [<c05759dc>]
> (drm_platform_init+0x44/0xd8)
> [    4.201475] [<c05759dc>] (drm_platform_init) from [<c05c1654>]
> (try_to_bring_up_master.part.3+0xc8/0x108)
> [    4.209804] [<c05c1654>] (try_to_bring_up_master.part.3) from
> [<c05c1740>] (component_master_add_with_match+0xac/0x124)
> [    4.219525] [<c05c1740>] (component_master_add_with_match) from
> [<c05a116c>] (msm_pdev_probe+0x64/0x70)
> [    4.230116] [<c05a116c>] (msm_pdev_probe) from [<c05c73c8>]
> (platform_drv_probe+0x44/0xa4)
> [    4.239485] [<c05c73c8>] (platform_drv_probe) from [<c05c5c48>]
> (driver_probe_device+0x108/0x23c)
> [    4.247813] [<c05c5c48>] (driver_probe_device) from [<c05c42ac>]
> (bus_for_each_drv+0x64/0x98)
> [    4.256752] [<c05c42ac>] (bus_for_each_drv) from [<c05c5b10>]
> (device_attach+0x74/0x88)
> [    4.265259] [<c05c5b10>] (device_attach) from [<c05c51fc>]
> (bus_probe_device+0x84/0xa8)
> [    4.273071] [<c05c51fc>] (bus_probe_device) from [<c05c561c>]
> (deferred_probe_work_func+0x64/0x94)
> [    4.281064] [<c05c561c>] (deferred_probe_work_func) from
> [<c025a354>] (process_one_work+0x134/0x334)
> [    4.290091] [<c025a354>] (process_one_work) from [<c025ac30>]
> (worker_thread+0x4c/0x4b0)
> [    4.299383] [<c025ac30>] (worker_thread) from [<c025ecfc>]
> (kthread+0xdc/0xf4)
> [    4.307459] [<c025ecfc>] (kthread) from [<c020e978>]
> (ret_from_fork+0x14/0x3c)
> 
> BR,
> -R
> 
> >
> >> ---
> >>  drivers/gpu/drm/drm_probe_helper.c | 36 ++++++++++++++++++++++++++++++++++--
> >>  include/drm/drm_crtc.h             |  1 +
> >>  2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> >> index 2fbdcca7ca9a..33bf550a1d3f 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c
> >> @@ -103,6 +103,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
> >>         int count = 0;
> >>         int mode_flags = 0;
> >>         bool verbose_prune = true;
> >> +       enum drm_connector_status old_status;
> >>
> >>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> >>
> >> @@ -121,7 +122,33 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
> >>                 if (connector->funcs->force)
> >>                         connector->funcs->force(connector);
> >>         } else {
> >> +               old_status = connector->status;
> >> +
> >>                 connector->status = connector->funcs->detect(connector, true);
> >> +
> >> +               /*
> >> +                * Normally either the driver's hpd code or the poll loop should
> >> +                * pick up any changes and fire the hotplug event. But if
> >> +                * userspace sneaks in a probe, we might miss a change. Hence
> >> +                * check here, and if anything changed start the hotplug code.
> >> +                */
> >> +               if (old_status != connector->status) {
> >> +                       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
> >> +                                     connector->base.id,
> >> +                                     connector->name,
> >> +                                     old_status, connector->status);
> >> +
> >> +                       /*
> >> +                        * The hotplug event code might call into the fb
> >> +                        * helpers, and so expects that we do not hold any
> >> +                        * locks. Fire up the poll struct instead, it will
> >> +                        * disable itself again.
> >> +                        */
> >> +                       dev->mode_config.delayed_event = true;
> >> +                       if (dev->mode_config.poll_enabled)
> >> +                               schedule_delayed_work(&dev->mode_config.output_poll_work,
> >> +                                                     0);
> >> +               }
> >>         }
> >>
> >>         /* Re-enable polling in case the global poll config changed. */
> >> @@ -274,10 +301,14 @@ static void output_poll_execute(struct work_struct *work)
> >>         struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
> >>         struct drm_connector *connector;
> >>         enum drm_connector_status old_status;
> >> -       bool repoll = false, changed = false;
> >> +       bool repoll = false, changed;
> >> +
> >> +       /* Pick up any changes detected by the probe functions. */
> >> +       changed = dev->mode_config.delayed_event;
> >> +       dev->mode_config.delayed_event = false;
> >>
> >>         if (!drm_kms_helper_poll)
> >> -               return;
> >> +               goto out;
> >>
> >>         mutex_lock(&dev->mode_config.mutex);
> >>         list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> >> @@ -319,6 +350,7 @@ static void output_poll_execute(struct work_struct *work)
> >>
> >>         mutex_unlock(&dev->mode_config.mutex);
> >>
> >> +out:
> >>         if (changed)
> >>                 drm_kms_helper_hotplug_event(dev);
> >>
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index f444263055c5..65da9fb939a7 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -1089,6 +1089,7 @@ struct drm_mode_config {
> >>         /* output poll support */
> >>         bool poll_enabled;
> >>         bool poll_running;
> >> +       bool delayed_event;
> >>         struct delayed_work output_poll_work;
> >>
> >>         /* pointers to standard properties */
> >> --
> >> 2.1.4
> >>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx