[Intel-gfx] [PATCH] drm/i915: Fix init_clock_gating for resume

[Rodrigo Vivi]

On Mon, Nov 13, 2017 at 08:46:10PM +0000, Ville Syrjälä wrote:
> On Mon, Nov 13, 2017 at 11:01:31AM -0800, Rodrigo Vivi wrote:
> > On Mon, Nov 13, 2017 at 02:50:28PM +0000, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Moving the init_clock_gating() call from intel_modeset_init_hw() to
> > > intel_modeset_gem_init() had an unintended effect of not applying
> > > some workarounds on resume. This, for example, cause some kind of
> > > corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS
> > > screen after hibernation. Fix the problem by explicitly calling
> > > init_clock_gating() from the resume path.
> > > 
> > > I really hope this doesn't break something else again...
> > > 
> > > Cc: stable at vger.kernel.org
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 9df7b5d59a94..0023fb17899f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1707,6 +1707,7 @@ static int i915_drm_resume(struct drm_device *dev)
> > >  
> > >  	intel_guc_resume(dev_priv);
> > >  
> > > +	intel_init_clock_gating(dev_priv);
> > >  	intel_modeset_init_hw(dev);
> > 
> > Few questions:
> > 
> > Any reason why here we have it before init_hw while on finish_reset
> > we have it after init_hw?
> 
> Just me being lazy and not checking where I put it in the display reset
> path.
> 
> > 
> > Also, what about the case on modeset_init? Do we need there?
> > If yes, shouldn't we move the call entirely to intel-modeset_init_hw?
> 
> I moved it there in commit b7048ea12fbb ("drm/i915: Do
> .init_clock_gating() earlier to avoid it clobbering watermarks")
> but that broke some GT workarounds, hence I had to move it back to
> somewhere later in commit 6ac43272768c ("drm/i915: Move init_clock_gating()
> back to where it was"). And that in turn broke my IVB LVDS :(
> 
> It's rather hard to win here.

yeap :(

> 
> > 
> > Also, do we still need now the call on vlv_resume_prepare?
> > and on i915_gem_init?
> 
> gem_init() yes. The vlv_resume_prepare() not sure. The theory there
> seems to be that on VLV the settings can get clobbered by a runtime
> suspend. Whereas with other platforms we seem to be assuming that
> they're preserved.
> 
> Whether that's what really happens I'm not 100% sure. I think it may
> very well be that VLV does lose a bunch of things in S0ix. We even have
> that vlv_{save,restore}_gunit_s0ix_state() thing to avoid some other
> things getting lost. CHV added a save/restore engine of some sort
> that should take care of more things automagically, so not sure if
> it actually needs the init_clock_gating either. OTOH maybe we should
> just start playing it safe an do the init_clock_gating() unconditionally
> in runtime resume? We already seem to be doing that with init_swizzle().

this seems a good idea...

But just in case you still want to go ahead with this patch for now feel
free to use:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

Thanks for all explanations.

> 
> -- 
> Ville Syrjälä
> Intel OTC