| Message ID | 20170111090117.5134-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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(-)