[Intel-gfx] [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 at 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 at 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 at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre