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

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

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@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@vger.kernel.org
> > Cc: Chris Wilson <chris@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@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.

> 
> 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().

-- 
Ville Syrjälä
Intel OTC