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@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
Thanks for all explanations.
>
> --
> Ville Syrjälä
> Intel OTC