Re: [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
> Thomas found that his machine would deadlock reloading the i915.ko
> module after resume. He identified that this was caused by the
> reacquisition of the connection mutex inside intel_enable_pipe_a()
> during the CRTC sanitization routine. This will only affect machines
> that quirk PIPE A, i.e. the original 830m chipsets.
> 
> This patch move the locking into a wrapper function so that
> intel_enable_pipe_a() can bypass the locking knowing that it already
> holds the correct locks.
> 
> Reported-by: Thomas Richter <richter@rus.uni-stuttgart.de>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Thomas Richter <richter@rus.uni-stuttgart.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   54 ++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c1f79a1..26d3424 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8387,9 +8387,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
>  #endif
>  }
>  
> -bool intel_get_load_detect_pipe(struct drm_connector *connector,
> -				struct drm_display_mode *mode,
> -				struct intel_load_detect_pipe *old)
> +static bool
> +__intel_get_load_detect_pipe(struct drm_connector *connector,
> +			     struct drm_display_mode *mode,
> +			     struct intel_load_detect_pipe *old)
>  {
>  	struct intel_crtc *intel_crtc;
>  	struct intel_encoder *intel_encoder =
> @@ -8405,7 +8406,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  		      connector->base.id, connector->name,
>  		      encoder->base.id, encoder->name);
>  
> -	mutex_lock(&dev->mode_config.connection_mutex);
> +	lockdep_assert_held(&dev->mode_config.connection_mutex);
>  
>  	/*
>  	 * Algorithm gets a little messy:
> @@ -8449,7 +8450,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  	 */
>  	if (!crtc) {
>  		DRM_DEBUG_KMS("no pipe available for load-detect\n");
> -		goto fail_unlock_connector;
> +		return false;
>  	}
>  
>  	mutex_lock(&crtc->mutex);
> @@ -8503,14 +8504,13 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  	else
>  		intel_crtc->new_config = NULL;
>  	mutex_unlock(&crtc->mutex);
> -fail_unlock_connector:
> -	mutex_unlock(&dev->mode_config.connection_mutex);
>  
>  	return false;
>  }
>  
> -void intel_release_load_detect_pipe(struct drm_connector *connector,
> -				    struct intel_load_detect_pipe *old)
> +static void
> +__intel_release_load_detect_pipe(struct drm_connector *connector,
> +				 struct intel_load_detect_pipe *old)
>  {
>  	struct intel_encoder *intel_encoder =
>  		intel_attached_encoder(connector);
> @@ -8518,6 +8518,8 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  	struct drm_crtc *crtc = encoder->crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> +	lockdep_assert_held(&connector->dev->mode_config.connection_mutex);
> +
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>  		      connector->base.id, connector->name,
>  		      encoder->base.id, encoder->name);
> @@ -8533,17 +8535,32 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  			drm_framebuffer_unregister_private(old->release_fb);
>  			drm_framebuffer_unreference(old->release_fb);
>  		}
> +	} else {
> +		/* Switch crtc and encoder back off if necessary */
> +		if (old->dpms_mode != DRM_MODE_DPMS_ON)
> +			connector->funcs->dpms(connector, old->dpms_mode);
> +	}

This part looks like an unrelated change?
-Daniel

> +	mutex_unlock(&crtc->mutex);
> +}
>  
> -		mutex_unlock(&crtc->mutex);
> +bool intel_get_load_detect_pipe(struct drm_connector *connector,
> +				struct drm_display_mode *mode,
> +				struct intel_load_detect_pipe *old)
> +{
> +	mutex_lock(&connector->dev->mode_config.connection_mutex);
> +	if (!__intel_get_load_detect_pipe(connector, mode, old)) {
>  		mutex_unlock(&connector->dev->mode_config.connection_mutex);
> -		return;
> +		return false;
>  	}
>  
> -	/* Switch crtc and encoder back off if necessary */
> -	if (old->dpms_mode != DRM_MODE_DPMS_ON)
> -		connector->funcs->dpms(connector, old->dpms_mode);
> +	/* lock will be released by intel_release_load_detect_pipe() */
> +	return true;
> +}
>  
> -	mutex_unlock(&crtc->mutex);
> +void intel_release_load_detect_pipe(struct drm_connector *connector,
> +				    struct intel_load_detect_pipe *old)
> +{
> +	__intel_release_load_detect_pipe(connector, old);
>  	mutex_unlock(&connector->dev->mode_config.connection_mutex);
>  }
>  
> @@ -12354,10 +12371,9 @@ static void intel_enable_pipe_a(struct drm_device *dev)
>  	if (!crt)
>  		return;
>  
> -	if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp))
> -		intel_release_load_detect_pipe(crt, &load_detect_temp);
> -
> -
> +	lockdep_assert_held(&dev->mode_config.connection_mutex);
> +	if (__intel_get_load_detect_pipe(crt, NULL, &load_detect_temp))
> +		__intel_release_load_detect_pipe(crt, &load_detect_temp);
>  }
>  
>  static bool
> -- 
> 1.7.9.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch