On Mon, 11 Aug 2014, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Mon, Aug 11, 2014 at 01:15:35PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx (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@xxxxxxxxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
