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 11:30:26AM +0300, Ville Syrjälä wrote:
> 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.
> 
> It can still try to grab crtc->mutex twice. Looks like Danial undid my
> fix to not take all the modeset locks around
> intel_modeset_setup_hw_state().

Hm, I didn't find anything in git logs and we've had places where we used
modeset_lock_all since a long time. And I don't see how Chris' patch
wouldn't address this here. Can you please explain?

Thanks, Daniel

> 
> > 
> > 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);
> > +	}
> > +	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
> 
> -- 
> Ville Syrjälä
> Intel OTC

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