Re: [PATCH 1/2] drm/i915: Fix locking for intel_enable_pipe_a()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 11 Aug 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Aug 11, 2014 at 01:15:35PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> intel_enable_pipe_a() gets called with all the modeset locks already
>> held (by drm_modeset_lock_all()), so trying to grab the same
>> locks using another drm_modeset_acquire_ctx is going to fail miserably.
>> 
>> Move most of the drm_modeset_acquire_ctx handling (init/drop/fini)
>> out from intel_{get,release}_load_detect_pipe() into the callers
>> (intel_{crt,tv}_detect()). Only the actual locking and backoff
>> handling is left in intel_get_load_detect_pipe(). And in
>> intel_enable_pipe_a() we just share the mode_config.acquire_ctx from
>> drm_modeset_lock_all() which is already holding all the relevant locks.
>> 
>> It's perfectly legal to lock the same ww_mutex multiple times using the
>> same ww_acquire_ctx. drm_modeset_lock() will convert the returned
>> -EALREADY into 0, so the caller doesn't need to do antyhing special.
>> 
>> Fixes a hang on resume on my 830.
>> 
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org (for 3.16)

Both patches picked up for -fixes, thanks for the patches and review.

BR,
Jani.


>
>> ---
>>  drivers/gpu/drm/i915/intel_crt.c     |  7 ++++++-
>>  drivers/gpu/drm/i915/intel_display.c | 21 ++++-----------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  3 +--
>>  drivers/gpu/drm/i915/intel_tv.c      |  7 ++++++-
>>  4 files changed, 17 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
>> index 2efaf8e..e8abfce 100644
>> --- a/drivers/gpu/drm/i915/intel_crt.c
>> +++ b/drivers/gpu/drm/i915/intel_crt.c
>> @@ -699,16 +699,21 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>>  		goto out;
>>  	}
>>  
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>>  	/* for pre-945g platforms use load detect */
>>  	if (intel_get_load_detect_pipe(connector, NULL, &tmp, &ctx)) {
>>  		if (intel_crt_detect_ddc(connector))
>>  			status = connector_status_connected;
>>  		else
>>  			status = intel_crt_load_detect(crt);
>> -		intel_release_load_detect_pipe(connector, &tmp, &ctx);
>> +		intel_release_load_detect_pipe(connector, &tmp);
>>  	} else
>>  		status = connector_status_unknown;
>>  
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +
>>  out:
>>  	intel_display_power_put(dev_priv, power_domain);
>>  	return status;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 51f48d9..7953b46 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8440,8 +8440,6 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>>  		      connector->base.id, connector->name,
>>  		      encoder->base.id, encoder->name);
>>  
>> -	drm_modeset_acquire_init(ctx, 0);
>> -
>>  retry:
>>  	ret = drm_modeset_lock(&config->connection_mutex, ctx);
>>  	if (ret)
>> @@ -8552,15 +8550,11 @@ fail_unlock:
>>  		goto retry;
>>  	}
>>  
>> -	drm_modeset_drop_locks(ctx);
>> -	drm_modeset_acquire_fini(ctx);
>> -
>>  	return false;
>>  }
>>  
>>  void intel_release_load_detect_pipe(struct drm_connector *connector,
>> -				    struct intel_load_detect_pipe *old,
>> -				    struct drm_modeset_acquire_ctx *ctx)
>> +				    struct intel_load_detect_pipe *old)
>>  {
>>  	struct intel_encoder *intel_encoder =
>>  		intel_attached_encoder(connector);
>> @@ -8584,17 +8578,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>>  			drm_framebuffer_unreference(old->release_fb);
>>  		}
>>  
>> -		goto unlock;
>>  		return;
>>  	}
>>  
>>  	/* Switch crtc and encoder back off if necessary */
>>  	if (old->dpms_mode != DRM_MODE_DPMS_ON)
>>  		connector->funcs->dpms(connector, old->dpms_mode);
>> -
>> -unlock:
>> -	drm_modeset_drop_locks(ctx);
>> -	drm_modeset_acquire_fini(ctx);
>>  }
>>  
>>  static int i9xx_pll_refclk(struct drm_device *dev,
>> @@ -12652,7 +12641,7 @@ static void intel_enable_pipe_a(struct drm_device *dev)
>>  	struct intel_connector *connector;
>>  	struct drm_connector *crt = NULL;
>>  	struct intel_load_detect_pipe load_detect_temp;
>> -	struct drm_modeset_acquire_ctx ctx;
>> +	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
>>  
>>  	/* We can't just switch on the pipe A, we need to set things up with a
>>  	 * proper mode and output configuration. As a gross hack, enable pipe A
>> @@ -12669,10 +12658,8 @@ static void intel_enable_pipe_a(struct drm_device *dev)
>>  	if (!crt)
>>  		return;
>>  
>> -	if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp, &ctx))
>> -		intel_release_load_detect_pipe(crt, &load_detect_temp, &ctx);
>> -
>> -
>> +	if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp, ctx))
>> +		intel_release_load_detect_pipe(crt, &load_detect_temp);
>>  }
>>  
>>  static bool
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 1b3d1d7..0dd23f1 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -834,8 +834,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>>  				struct intel_load_detect_pipe *old,
>>  				struct drm_modeset_acquire_ctx *ctx);
>>  void intel_release_load_detect_pipe(struct drm_connector *connector,
>> -				    struct intel_load_detect_pipe *old,
>> -				    struct drm_modeset_acquire_ctx *ctx);
>> +				    struct intel_load_detect_pipe *old);
>>  int intel_pin_and_fence_fb_obj(struct drm_device *dev,
>>  			       struct drm_i915_gem_object *obj,
>>  			       struct intel_engine_cs *pipelined);
>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>> index e211eef..32186a6 100644
>> --- a/drivers/gpu/drm/i915/intel_tv.c
>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>> @@ -1323,11 +1323,16 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>>  		struct intel_load_detect_pipe tmp;
>>  		struct drm_modeset_acquire_ctx ctx;
>>  
>> +		drm_modeset_acquire_init(&ctx, 0);
>> +
>>  		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
>>  			type = intel_tv_detect_type(intel_tv, connector);
>> -			intel_release_load_detect_pipe(connector, &tmp, &ctx);
>> +			intel_release_load_detect_pipe(connector, &tmp);
>>  		} else
>>  			return connector_status_unknown;
>> +
>> +		drm_modeset_drop_locks(&ctx);
>> +		drm_modeset_acquire_fini(&ctx);
>>  	} else
>>  		return connector->status;
>>  
>> -- 
>> 1.8.5.5
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx