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

[Lyude Paul <lyude@redhat.com>]

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