Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

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

On Thu, Jun 01, 2017 at 07:05:41PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 01, 2017 at 05:48:16PM +0300, Jani Nikula wrote:
> > On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > If intel_crtc_disable_noatomic() were to ever get called during resume
> > > e'd end up deadlocking since resume has its own acqcuire_ctx but
> > > intel_crtc_disable_noatomic() still tries to use the
> > > mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.
> > 
> > Same here, have we seen this, or "just" theoretical?
> 
> Happens on my 830 after adding the pipes powerwell. Before that the
> machine resumed with both pipes off and the pipe A quirk left
> crtc->active as false so intel_crtc_disable_noatomic() didn't get
> called.
> 
> Hmm. Actually, now that I think about it more, I believe this should
> happen for S4 even without the new powerwell since both pipes should
> have been enabled by the BIOS when resuming from S4. I'll have to
> run a proper to test to verify that.

Indeed. In fact it deadlocks already during the thaw phase between
creating the hibernation image and writing to it disk.

> 
> > 
> > BR,
> > Jani.
> > 
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 2f76a63efe8c..4e3c64ed4131 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> > >  		intel_update_watermarks(intel_crtc);
> > >  }
> > >  
> > > -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > > +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > > +					struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct intel_encoder *encoder;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > >  		return;
> > >  	}
> > >  
> > > -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> > > +	state->acquire_ctx = ctx;
> > >  
> > >  	/* Everything's already locked, -EDEADLK can't happen. */
> > >  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> > > @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  		plane = crtc->plane;
> > >  		crtc->base.primary->state->visible = true;
> > >  		crtc->plane = !plane;
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  		crtc->plane = plane;
> > >  	}
> > >  
> > > @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  	/* Adjust the state of the output pipe according to whether we
> > >  	 * have active connectors/encoders. */
> > >  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  
> > >  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
> > >  		/*
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC