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 <rich...@rus.uni-stuttgart.de>
> > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Thomas Richter <rich...@rus.uni-stuttgart.de>
> > Cc: Daniel Vetter <dan...@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrj...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
