Re: [PATCH] drm/i915: Fix deadlock in i830_disable_pipe()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 28, 2017 at 09:05:00PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-28 15:48:53)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > i830_disable_pipe() gets called from the power well code, and thus
> > we're already holding the power domain mutex. That means we can't
> > call plane->get_hw_state() as it will also try to grab the
> > same mutex and will thus deadlock.
> > 
> > Replace the assert_plane() calls (which calls ->get_hw_state()) with
> > just raw register reads in i830_disable_pipe(). As a bonus we can
> > now get a warning if plane C is enabled even though we don't even
> > expose it as a drm plane.
> > 
> > Fixes: 51f5a0963984 ("drm/i915: Add .get_hw_state() method for planes")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d67c7c498b34..48d9332b196f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14731,8 +14731,11 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> >         DRM_DEBUG_KMS("disabling pipe %c due to force quirk\n",
> >                       pipe_name(pipe));
> >  
> > -       assert_planes_disabled(intel_get_crtc_for_pipe(dev_priv, PIPE_A));
> > -       assert_planes_disabled(intel_get_crtc_for_pipe(dev_priv, PIPE_B));
> > +       WARN_ON(I915_READ(DSPCNTR(PLANE_A)) & DISPLAY_PLANE_ENABLE ||
> > +               I915_READ(DSPCNTR(PLANE_B)) & DISPLAY_PLANE_ENABLE ||
> > +               I915_READ(DSPCNTR(PLANE_C)) & DISPLAY_PLANE_ENABLE ||
> > +               I915_READ(CURCNTR(PIPE_A)) & CURSOR_MODE ||
> > +               I915_READ(CURCNTR(PIPE_B)) & CURSOR_MODE);
> 
> Ok. Avoiding mutex recursion sounds sensible, but a recursion sounds
> like a layering violation. i830_disable_pipe is only used by the
> powerwell, and I guess you could make the i830_enable_pipe in
> i9xx_crtc_disable into a call to the powerdomain instead.

That wouldn't actully work. We already hold the power domain
reference so the power well enable hook wouldn't even be called,
and even if it was it would just be a nop as the pipe is already
up and running.

I guess it would be possible to redesign that stuff somehow to do what
we want, but that probably means we'd have to redesign the power domain
initialization as well to never call the enable hook if the power well
is already active on boot.

So this would involve a bunch of work, and I can't actually see any
benefit from taking a detour through the power well code.

> 
> That sounds like more work than desired to fix the immediate problem!
> 
> However, I think you will miss the loss in precision by putting all the
> checks into one WARN_ON. If it either fires, we've no idea what went
> wrong?
> 
> Would it be worth just making each of those a separate WARN_ON()? I
> think so.

Yeah, I guess having that extra bit of information might be somewhat
helpful. I'll split it up.

> 
> Either way, I've checked that your explanation matches the code...
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks.

> -Chris

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx