From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm/i915: Fix locking for intel_enable_pipe_a() Date: Mon, 11 Aug 2014 13:29:56 +0200 Message-ID: <20140811112956.GM8727@phenom.ffwll.local> References: <1407752136-2973-1-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wi0-f170.google.com (mail-wi0-f170.google.com [209.85.212.170]) by gabe.freedesktop.org (Postfix) with ESMTP id 1E5646E3BA for ; Mon, 11 Aug 2014 04:29:44 -0700 (PDT) Received: by mail-wi0-f170.google.com with SMTP id f8so5371711wiw.5 for ; Mon, 11 Aug 2014 04:29:44 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1407752136-2973-1-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Aug 11, 2014 at 01:15:35PM +0300, ville.syrjala@linux.intel.com wro= te: > From: Ville Syrj=E4l=E4 > = > 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=E4l=E4 Reviewed-by: Daniel Vetter Cc: stable@vger.kernel.org (for 3.16) > --- > 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/inte= l_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, b= ool 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 =3D connector_status_connected; > else > status =3D intel_crt_load_detect(crt); > - intel_release_load_detect_pipe(connector, &tmp, &ctx); > + intel_release_load_detect_pipe(connector, &tmp); > } else > status =3D 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_connecto= r *connector, > connector->base.id, connector->name, > encoder->base.id, encoder->name); > = > - drm_modeset_acquire_init(ctx, 0); > - > retry: > ret =3D 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 =3D > intel_attached_encoder(connector); > @@ -8584,17 +8578,12 @@ void intel_release_load_detect_pipe(struct drm_co= nnector *connector, > drm_framebuffer_unreference(old->release_fb); > } > = > - goto unlock; > return; > } > = > /* Switch crtc and encoder back off if necessary */ > if (old->dpms_mode !=3D 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 =3D NULL; > struct intel_load_detect_pipe load_detect_temp; > - struct drm_modeset_acquire_ctx ctx; > + struct drm_modeset_acquire_ctx *ctx =3D 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_devic= e *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/inte= l_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 =3D 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