[PATCH] drm/probe-helpers: Drop locking from poll_enable

[PATCH] drm/probe-helpers: Drop locking from poll_enable

[Daniel Vetter]

It was only needed to protect the connector_list walking, see

commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 9 23:44:26 2015 +0200

    drm/probe-helper: Grab mode_config.mutex in poll_init/enable

Unfortunately the commit message of that patch fails to mention that
the new locking check was for the connector_list.

But that requirement disappeared in

commit c36a3254f7857f1ad9badbe3578ccc92be541a8e
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Dec 15 16:58:43 2016 +0100

    drm: Convert all helpers to drm_connector_list_iter

and so we can drop this again.

This fixes a locking inversion on nouveau, where the rpm code needs to
re-enable. But in other places the rpm_get() calls are nested within
the big modeset locks.

While at it, also improve the kerneldoc for these two functions a
notch.

Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_probe_helper.c   | 41 ++++++++++--------------------------
 drivers/gpu/drm/i915/intel_hotplug.c |  4 ++--
 include/drm/drm_crtc_helper.h        |  1 -
 3 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 060211ac74a1..7c70f2a52250 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -115,25 +115,24 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
 /**
- * drm_kms_helper_poll_enable_locked - re-enable output polling.
+ * drm_kms_helper_poll_enable - re-enable output polling.
  * @dev: drm_device
  *
- * This function re-enables the output polling work without
- * locking the mode_config mutex.
+ * This function re-enables the output polling work, after it has been
+ * temporarily disabled using drm_kms_helper_poll_disable(), for example over
+ * suspend/resume.
  *
- * This is like drm_kms_helper_poll_enable() however it is to be
- * called from a context where the mode_config mutex is locked
- * already.
+ * Drivers can call this helper from their device resume implementation. It is
+ * an error to call this when the output polling support has not yet been set
+ * up.
  */
-void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
+void drm_kms_helper_poll_enable(struct drm_device *dev)
 {
 	bool poll = false;
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
 
-	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
-
 	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
 		return;
 
@@ -153,7 +152,7 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 	if (poll)
 		schedule_delayed_work(&dev->mode_config.output_poll_work, delay);
 }
-EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
+EXPORT_SYMBOL(drm_kms_helper_poll_enable);
 
 static enum drm_connector_status
 drm_connector_detect(struct drm_connector *connector, bool force)
@@ -280,7 +279,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 
 	/* 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_locked(dev);
+		drm_kms_helper_poll_enable(dev);
 
 	dev->mode_config.poll_running = drm_kms_helper_poll;
 
@@ -475,7 +474,7 @@ static void output_poll_execute(struct work_struct *work)
  *
  * Drivers can call this helper from their device suspend implementation. It is
  * not an error to call this even when output polling isn't enabled or arlready
- * disabled.
+ * disabled. Polling is re-enabled by calling drm_kms_helper_poll_enable().
  */
 void drm_kms_helper_poll_disable(struct drm_device *dev)
 {
@@ -486,24 +485,6 @@ void drm_kms_helper_poll_disable(struct drm_device *dev)
 EXPORT_SYMBOL(drm_kms_helper_poll_disable);
 
 /**
- * drm_kms_helper_poll_enable - re-enable output polling.
- * @dev: drm_device
- *
- * This function re-enables the output polling work.
- *
- * Drivers can call this helper from their device resume implementation. It is
- * an error to call this when the output polling support has not yet been set
- * up.
- */
-void drm_kms_helper_poll_enable(struct drm_device *dev)
-{
-	mutex_lock(&dev->mode_config.mutex);
-	drm_kms_helper_poll_enable_locked(dev);
-	mutex_unlock(&dev->mode_config.mutex);
-}
-EXPORT_SYMBOL(drm_kms_helper_poll_enable);
-
-/**
  * drm_kms_helper_poll_init - initialize and enable output polling
  * @dev: drm_device
  *
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 2ddc9e5842ec..5122d4bfb70e 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -182,7 +182,7 @@ static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
 
 	/* Enable polling and queue hotplug re-enabling. */
 	if (hpd_disabled) {
-		drm_kms_helper_poll_enable_locked(dev);
+		drm_kms_helper_poll_enable(dev);
 		mod_delayed_work(system_wq, &dev_priv->hotplug.reenable_work,
 				 msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
 	}
@@ -519,7 +519,7 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
 	drm_connector_list_iter_put(&conn_iter);
 
 	if (enabled)
-		drm_kms_helper_poll_enable_locked(dev);
+		drm_kms_helper_poll_enable(dev);
 
 	mutex_unlock(&dev->mode_config.mutex);
 
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 982c299e435a..d026f5017c33 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -73,6 +73,5 @@ 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);
-extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
 
 #endif
-- 
2.11.0

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

Re: [PATCH] drm/probe-helpers: Drop locking from poll_enable

[Chris Wilson]

On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote:
> It was only needed to protect the connector_list walking, see
> 
> commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jul 9 23:44:26 2015 +0200
> 
>     drm/probe-helper: Grab mode_config.mutex in poll_init/enable
> 
> Unfortunately the commit message of that patch fails to mention that
> the new locking check was for the connector_list.
> 
> But that requirement disappeared in
> 
> commit c36a3254f7857f1ad9badbe3578ccc92be541a8e
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Dec 15 16:58:43 2016 +0100
> 
>     drm: Convert all helpers to drm_connector_list_iter
> 
> and so we can drop this again.
> 
> This fixes a locking inversion on nouveau, where the rpm code needs to
> re-enable. But in other places the rpm_get() calls are nested within
> the big modeset locks.
> 
> While at it, also improve the kerneldoc for these two functions a
> notch.
> 
> Cc: Dave Airlie <airlied@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_probe_helper.c   | 41 ++++++++++--------------------------
>  drivers/gpu/drm/i915/intel_hotplug.c |  4 ++--
>  include/drm/drm_crtc_helper.h        |  1 -
>  3 files changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 060211ac74a1..7c70f2a52250 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -115,25 +115,24 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  
>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
>  /**
> - * drm_kms_helper_poll_enable_locked - re-enable output polling.
> + * drm_kms_helper_poll_enable - re-enable output polling.
>   * @dev: drm_device
>   *
> - * This function re-enables the output polling work without
> - * locking the mode_config mutex.
> + * This function re-enables the output polling work, after it has been
> + * temporarily disabled using drm_kms_helper_poll_disable(), for example over
> + * suspend/resume.
>   *
> - * This is like drm_kms_helper_poll_enable() however it is to be
> - * called from a context where the mode_config mutex is locked
> - * already.
> + * Drivers can call this helper from their device resume implementation. It is
> + * an error to call this when the output polling support has not yet been set
> + * up.
>   */
> -void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
> +void drm_kms_helper_poll_enable(struct drm_device *dev)
>  {
>  	bool poll = false;
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
>  
> -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> -
>  	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
>  		return;

Hmm, output_poll_execute() itself is not checking this correctly,

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 84b75709af90..cb3febc6e719 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work)
        changed = dev->mode_config.delayed_event;
        dev->mode_config.delayed_event = false;
 
-       if (!drm_kms_helper_poll)
+       if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
                goto out;
 
        if (!mutex_trylock(&dev->mode_config.mutex)) {

The connector list is locked, but the other reads are all racy
(connector->polled, delayed_event). Those races seem intrinsic and handled
by e.g. intel_hotplug.c.

Since the locking here was only covering the connector list and that now
has its own lock,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/probe-helpers: Drop locking from poll_enable

[Daniel Vetter]

On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote:
> On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote:
> > It was only needed to protect the connector_list walking, see
> > 
> > commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Thu Jul 9 23:44:26 2015 +0200
> > 
> >     drm/probe-helper: Grab mode_config.mutex in poll_init/enable
> > 
> > Unfortunately the commit message of that patch fails to mention that
> > the new locking check was for the connector_list.
> > 
> > But that requirement disappeared in
> > 
> > commit c36a3254f7857f1ad9badbe3578ccc92be541a8e
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Thu Dec 15 16:58:43 2016 +0100
> > 
> >     drm: Convert all helpers to drm_connector_list_iter
> > 
> > and so we can drop this again.
> > 
> > This fixes a locking inversion on nouveau, where the rpm code needs to
> > re-enable. But in other places the rpm_get() calls are nested within
> > the big modeset locks.
> > 
> > While at it, also improve the kerneldoc for these two functions a
> > notch.
> > 
> > Cc: Dave Airlie <airlied@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_probe_helper.c   | 41 ++++++++++--------------------------
> >  drivers/gpu/drm/i915/intel_hotplug.c |  4 ++--
> >  include/drm/drm_crtc_helper.h        |  1 -
> >  3 files changed, 13 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 060211ac74a1..7c70f2a52250 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -115,25 +115,24 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
> >  
> >  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> >  /**
> > - * drm_kms_helper_poll_enable_locked - re-enable output polling.
> > + * drm_kms_helper_poll_enable - re-enable output polling.
> >   * @dev: drm_device
> >   *
> > - * This function re-enables the output polling work without
> > - * locking the mode_config mutex.
> > + * This function re-enables the output polling work, after it has been
> > + * temporarily disabled using drm_kms_helper_poll_disable(), for example over
> > + * suspend/resume.
> >   *
> > - * This is like drm_kms_helper_poll_enable() however it is to be
> > - * called from a context where the mode_config mutex is locked
> > - * already.
> > + * Drivers can call this helper from their device resume implementation. It is
> > + * an error to call this when the output polling support has not yet been set
> > + * up.
> >   */
> > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
> > +void drm_kms_helper_poll_enable(struct drm_device *dev)
> >  {
> >  	bool poll = false;
> >  	struct drm_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
> >  
> > -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> > -
> >  	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> >  		return;
> 
> Hmm, output_poll_execute() itself is not checking this correctly,
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 84b75709af90..cb3febc6e719 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work)
>         changed = dev->mode_config.delayed_event;
>         dev->mode_config.delayed_event = false;
>  
> -       if (!drm_kms_helper_poll)
> +       if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
>                 goto out;

The cancel_work_sync in poll_disable should make this impossible, but it
might be a good thing to check this with a WARN_ON? Care to type that
patch as a follow up please?

>         if (!mutex_trylock(&dev->mode_config.mutex)) {
> 
> The connector list is locked, but the other reads are all racy
> (connector->polled, delayed_event). Those races seem intrinsic and handled
> by e.g. intel_hotplug.c.
> 
> Since the locking here was only covering the connector list and that now
> has its own lock,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the review, I think I'll wait for Dave to supply bugzilla link
or confirmation it solves his lockdep issue before merging.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/probe-helpers: Drop locking from poll_enable

[Chris Wilson]

On Tue, Jan 10, 2017 at 05:34:08PM +0100, Daniel Vetter wrote:
> On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote:
> > On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote:
> > > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
> > > +void drm_kms_helper_poll_enable(struct drm_device *dev)
> > >  {
> > >  	bool poll = false;
> > >  	struct drm_connector *connector;
> > >  	struct drm_connector_list_iter conn_iter;
> > >  	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
> > >  
> > > -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> > > -
> > >  	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> > >  		return;
> > 
> > Hmm, output_poll_execute() itself is not checking this correctly,
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 84b75709af90..cb3febc6e719 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work)
> >         changed = dev->mode_config.delayed_event;
> >         dev->mode_config.delayed_event = false;
> >  
> > -       if (!drm_kms_helper_poll)
> > +       if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> >                 goto out;
> 
> The cancel_work_sync in poll_disable should make this impossible, but it
> might be a good thing to check this with a WARN_ON? Care to type that
> patch as a follow up please?

Imagine a drm_kms_helper_poll_disable() run concurrently with
drm_kms_helper_poll_enable(). The enable() gets past the is-enabled? check
and begins iterating the list, meanwhile the disable() cancels the work
and turns off the helper. The enable() is unaware of this and so
reschedules the work, and as output_poll_execute() doesn't check for
dev->mode_config.poll_enabled it stays active.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/probe-helpers: Drop locking from poll_enable

[Patchwork]

== Series Details ==

Series: drm/probe-helpers: Drop locking from poll_enable
URL   : https://patchwork.freedesktop.org/series/17768/
State : failure

== Summary ==

Series 17768v1 drm/probe-helpers: Drop locking from poll_enable
https://patchwork.freedesktop.org/api/1.0/series/17768/revisions/1/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> INCOMPLETE (fi-hsw-4770)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:118  pass:109  dwarn:0   dfail:0   fail:0   skip:8  
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

44b1ad71b67ecfae6ba9b816c6f3a6ebe1fd182e drm-tip: 2017y-01m-10d-16h-32m-29s UTC integration manifest
daa3d60 drm/probe-helpers: Drop locking from poll_enable

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3467/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/probe-helpers: Drop locking from poll_enable

[Daniel Vetter]

On Tue, Jan 10, 2017 at 05:10:50PM +0000, Chris Wilson wrote:
> On Tue, Jan 10, 2017 at 05:34:08PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote:
> > > On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote:
> > > > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
> > > > +void drm_kms_helper_poll_enable(struct drm_device *dev)
> > > >  {
> > > >  	bool poll = false;
> > > >  	struct drm_connector *connector;
> > > >  	struct drm_connector_list_iter conn_iter;
> > > >  	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
> > > >  
> > > > -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> > > > -
> > > >  	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> > > >  		return;
> > > 
> > > Hmm, output_poll_execute() itself is not checking this correctly,
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 84b75709af90..cb3febc6e719 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work)
> > >         changed = dev->mode_config.delayed_event;
> > >         dev->mode_config.delayed_event = false;
> > >  
> > > -       if (!drm_kms_helper_poll)
> > > +       if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> > >                 goto out;
> > 
> > The cancel_work_sync in poll_disable should make this impossible, but it
> > might be a good thing to check this with a WARN_ON? Care to type that
> > patch as a follow up please?
> 
> Imagine a drm_kms_helper_poll_disable() run concurrently with
> drm_kms_helper_poll_enable(). The enable() gets past the is-enabled? check
> and begins iterating the list, meanwhile the disable() cancels the work
> and turns off the helper. The enable() is unaware of this and so
> reschedules the work, and as output_poll_execute() doesn't check for
> dev->mode_config.poll_enabled it stays active.

I thought I also added that this case is a driver bug to the kernel-docs,
but I failed. Driver needs to ensure this is all ordered reasonably well
(which it is, if you only call it from suspend/resume callbacks). I'll
respin with that fixed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/probe-helpers: Drop locking from poll_enable

[Daniel Vetter]

It was only needed to protect the connector_list walking, see

commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 9 23:44:26 2015 +0200

    drm/probe-helper: Grab mode_config.mutex in poll_init/enable

Unfortunately the commit message of that patch fails to mention that
the new locking check was for the connector_list.

But that requirement disappeared in

commit c36a3254f7857f1ad9badbe3578ccc92be541a8e
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Dec 15 16:58:43 2016 +0100

    drm: Convert all helpers to drm_connector_list_iter

and so we can drop this again.

This fixes a locking inversion on nouveau, where the rpm code needs to
re-enable. But in other places the rpm_get() calls are nested within
the big modeset locks.

While at it, also improve the kerneldoc for these two functions a
notch.

v2: Update the kerneldoc even more to explain that these functions
can't be called concurrently, or bad things happen (Chris).

Cc: Dave Airlie <airlied@gmail.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_probe_helper.c   | 51 ++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_hotplug.c |  4 +--
 include/drm/drm_crtc_helper.h        |  1 -
 3 files changed, 22 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 20f48d1e2785..93381454bdf7 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -115,25 +115,28 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
 /**
- * drm_kms_helper_poll_enable_locked - re-enable output polling.
+ * drm_kms_helper_poll_enable - re-enable output polling.
  * @dev: drm_device
  *
- * This function re-enables the output polling work without
- * locking the mode_config mutex.
+ * This function re-enables the output polling work, after it has been
+ * temporarily disabled using drm_kms_helper_poll_disable(), for example over
+ * suspend/resume.
  *
- * This is like drm_kms_helper_poll_enable() however it is to be
- * called from a context where the mode_config mutex is locked
- * already.
+ * Drivers can call this helper from their device resume implementation. It is
+ * an error to call this when the output polling support has not yet been set
+ * up.
+ *
+ * Note that calls to enable and disable polling must be strictly ordered, which
+ * is automatically the case when they're only call from suspend/resume
+ * callbacks.
  */
-void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
+void drm_kms_helper_poll_enable(struct drm_device *dev)
 {
 	bool poll = false;
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
 
-	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
-
 	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
 		return;
 
@@ -163,7 +166,7 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 	if (poll)
 		schedule_delayed_work(&dev->mode_config.output_poll_work, delay);
 }
-EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
+EXPORT_SYMBOL(drm_kms_helper_poll_enable);
 
 static enum drm_connector_status
 drm_connector_detect(struct drm_connector *connector, bool force)
@@ -290,7 +293,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 
 	/* 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_locked(dev);
+		drm_kms_helper_poll_enable(dev);
 
 	dev->mode_config.poll_running = drm_kms_helper_poll;
 
@@ -484,8 +487,12 @@ static void output_poll_execute(struct work_struct *work)
  * This function disables the output polling work.
  *
  * Drivers can call this helper from their device suspend implementation. It is
- * not an error to call this even when output polling isn't enabled or arlready
- * disabled.
+ * not an error to call this even when output polling isn't enabled or already
+ * disabled. Polling is re-enabled by calling drm_kms_helper_poll_enable().
+ *
+ * Note that calls to enable and disable polling must be strictly ordered, which
+ * is automatically the case when they're only call from suspend/resume
+ * callbacks.
  */
 void drm_kms_helper_poll_disable(struct drm_device *dev)
 {
@@ -496,24 +503,6 @@ void drm_kms_helper_poll_disable(struct drm_device *dev)
 EXPORT_SYMBOL(drm_kms_helper_poll_disable);
 
 /**
- * drm_kms_helper_poll_enable - re-enable output polling.
- * @dev: drm_device
- *
- * This function re-enables the output polling work.
- *
- * Drivers can call this helper from their device resume implementation. It is
- * an error to call this when the output polling support has not yet been set
- * up.
- */
-void drm_kms_helper_poll_enable(struct drm_device *dev)
-{
-	mutex_lock(&dev->mode_config.mutex);
-	drm_kms_helper_poll_enable_locked(dev);
-	mutex_unlock(&dev->mode_config.mutex);
-}
-EXPORT_SYMBOL(drm_kms_helper_poll_enable);
-
-/**
  * drm_kms_helper_poll_init - initialize and enable output polling
  * @dev: drm_device
  *
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 2ddc9e5842ec..5122d4bfb70e 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -182,7 +182,7 @@ static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
 
 	/* Enable polling and queue hotplug re-enabling. */
 	if (hpd_disabled) {
-		drm_kms_helper_poll_enable_locked(dev);
+		drm_kms_helper_poll_enable(dev);
 		mod_delayed_work(system_wq, &dev_priv->hotplug.reenable_work,
 				 msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
 	}
@@ -519,7 +519,7 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
 	drm_connector_list_iter_put(&conn_iter);
 
 	if (enabled)
-		drm_kms_helper_poll_enable_locked(dev);
+		drm_kms_helper_poll_enable(dev);
 
 	mutex_unlock(&dev->mode_config.mutex);
 
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 982c299e435a..d026f5017c33 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -73,6 +73,5 @@ 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);
-extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
 
 #endif
-- 
2.11.0

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

Re: [PATCH] drm/probe-helpers: Drop locking from poll_enable

[Lyude Paul]

Fixes locking issues I've witnessed on the W541.

Tested-by: Lyude <lyude@redhat.com>
Reviewed-by: Lyude <lyude@redhat.com>

On Wed, 2017-01-11 at 10:01 +0100, Daniel Vetter wrote:
> It was only needed to protect the connector_list walking, see
> 
> commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jul 9 23:44:26 2015 +0200
> 
>     drm/probe-helper: Grab mode_config.mutex in poll_init/enable
> 
> Unfortunately the commit message of that patch fails to mention that
> the new locking check was for the connector_list.
> 
> But that requirement disappeared in
> 
> commit c36a3254f7857f1ad9badbe3578ccc92be541a8e
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Dec 15 16:58:43 2016 +0100
> 
>     drm: Convert all helpers to drm_connector_list_iter
> 
> and so we can drop this again.
> 
> This fixes a locking inversion on nouveau, where the rpm code needs
> to
> re-enable. But in other places the rpm_get() calls are nested within
> the big modeset locks.
> 
> While at it, also improve the kerneldoc for these two functions a
> notch.
> 
> v2: Update the kerneldoc even more to explain that these functions
> can't be called concurrently, or bad things happen (Chris).
> 
> Cc: Dave Airlie <airlied@gmail.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_probe_helper.c   | 51 ++++++++++++++----------
> ------------
>  drivers/gpu/drm/i915/intel_hotplug.c |  4 +--
>  include/drm/drm_crtc_helper.h        |  1 -
>  3 files changed, 22 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 20f48d1e2785..93381454bdf7 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -115,25 +115,28 @@ static int
> drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  
>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
>  /**
> - * drm_kms_helper_poll_enable_locked - re-enable output polling.
> + * drm_kms_helper_poll_enable - re-enable output polling.
>   * @dev: drm_device
>   *
> - * This function re-enables the output polling work without
> - * locking the mode_config mutex.
> + * This function re-enables the output polling work, after it has
> been
> + * temporarily disabled using drm_kms_helper_poll_disable(), for
> example over
> + * suspend/resume.
>   *
> - * This is like drm_kms_helper_poll_enable() however it is to be
> - * called from a context where the mode_config mutex is locked
> - * already.
> + * Drivers can call this helper from their device resume
> implementation. It is
> + * an error to call this when the output polling support has not yet
> been set
> + * up.
> + *
> + * Note that calls to enable and disable polling must be strictly
> ordered, which
> + * is automatically the case when they're only call from
> suspend/resume
> + * callbacks.
>   */
> -void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
> +void drm_kms_helper_poll_enable(struct drm_device *dev)
>  {
>  	bool poll = false;
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
>  
> -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> -
>  	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
>  		return;
>  
> @@ -163,7 +166,7 @@ void drm_kms_helper_poll_enable_locked(struct
> drm_device *dev)
>  	if (poll)
>  		schedule_delayed_work(&dev-
> >mode_config.output_poll_work, delay);
>  }
> -EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
> +EXPORT_SYMBOL(drm_kms_helper_poll_enable);
>  
>  static enum drm_connector_status
>  drm_connector_detect(struct drm_connector *connector, bool force)
> @@ -290,7 +293,7 @@ int
> drm_helper_probe_single_connector_modes(struct drm_connector
> *connector,
>  
>  	/* 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_locked(dev);
> +		drm_kms_helper_poll_enable(dev);
>  
>  	dev->mode_config.poll_running = drm_kms_helper_poll;
>  
> @@ -484,8 +487,12 @@ static void output_poll_execute(struct
> work_struct *work)
>   * This function disables the output polling work.
>   *
>   * Drivers can call this helper from their device suspend
> implementation. It is
> - * not an error to call this even when output polling isn't enabled
> or arlready
> - * disabled.
> + * not an error to call this even when output polling isn't enabled
> or already
> + * disabled. Polling is re-enabled by calling
> drm_kms_helper_poll_enable().
> + *
> + * Note that calls to enable and disable polling must be strictly
> ordered, which
> + * is automatically the case when they're only call from
> suspend/resume
> + * callbacks.
>   */
>  void drm_kms_helper_poll_disable(struct drm_device *dev)
>  {
> @@ -496,24 +503,6 @@ void drm_kms_helper_poll_disable(struct
> drm_device *dev)
>  EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>  
>  /**
> - * drm_kms_helper_poll_enable - re-enable output polling.
> - * @dev: drm_device
> - *
> - * This function re-enables the output polling work.
> - *
> - * Drivers can call this helper from their device resume
> implementation. It is
> - * an error to call this when the output polling support has not yet
> been set
> - * up.
> - */
> -void drm_kms_helper_poll_enable(struct drm_device *dev)
> -{
> -	mutex_lock(&dev->mode_config.mutex);
> -	drm_kms_helper_poll_enable_locked(dev);
> -	mutex_unlock(&dev->mode_config.mutex);
> -}
> -EXPORT_SYMBOL(drm_kms_helper_poll_enable);
> -
> -/**
>   * drm_kms_helper_poll_init - initialize and enable output polling
>   * @dev: drm_device
>   *
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> b/drivers/gpu/drm/i915/intel_hotplug.c
> index 2ddc9e5842ec..5122d4bfb70e 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -182,7 +182,7 @@ static void intel_hpd_irq_storm_disable(struct
> drm_i915_private *dev_priv)
>  
>  	/* Enable polling and queue hotplug re-enabling. */
>  	if (hpd_disabled) {
> -		drm_kms_helper_poll_enable_locked(dev);
> +		drm_kms_helper_poll_enable(dev);
>  		mod_delayed_work(system_wq, &dev_priv-
> >hotplug.reenable_work,
>  				 msecs_to_jiffies(HPD_STORM_REENABLE
> _DELAY));
>  	}
> @@ -519,7 +519,7 @@ static void i915_hpd_poll_init_work(struct
> work_struct *work)
>  	drm_connector_list_iter_put(&conn_iter);
>  
>  	if (enabled)
> -		drm_kms_helper_poll_enable_locked(dev);
> +		drm_kms_helper_poll_enable(dev);
>  
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> diff --git a/include/drm/drm_crtc_helper.h
> b/include/drm/drm_crtc_helper.h
> index 982c299e435a..d026f5017c33 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -73,6 +73,5 @@ 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);
> -extern void drm_kms_helper_poll_enable_locked(struct drm_device
> *dev);
>  
>  #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx