From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc() Date: Mon, 9 Jun 2014 11:30:26 +0300 Message-ID: <20140609083025.GL27580@intel.com> References: <5394EE4E.3040402@rus.uni-stuttgart.de> <1402296430-5065-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 68FE16E4A1 for ; Mon, 9 Jun 2014 01:30:49 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1402296430-5065-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: Thomas Richter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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(). > = > Reported-by: Thomas Richter > Signed-off-by: Chris Wilson > Cc: Thomas Richter > Cc: Daniel Vetter > Cc: Ville Syrj=E4l=E4 > --- > 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 =3D > @@ -8405,7 +8406,7 @@ bool intel_get_load_detect_pipe(struct drm_connecto= r *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_connecto= r *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_connec= tor *connector, > else > intel_crtc->new_config =3D 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 =3D > intel_attached_encoder(connector); > @@ -8518,6 +8518,8 @@ void intel_release_load_detect_pipe(struct drm_conn= ector *connector, > struct drm_crtc *crtc =3D encoder->crtc; > struct intel_crtc *intel_crtc =3D 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_co= nnector *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 !=3D 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 !=3D 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_devic= e *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=E4l=E4 Intel OTC