[PATCH] drm/i915: Fix init_clock_gating for resume

[PATCH] drm/i915: Fix init_clock_gating for resume

[Ville Syrjala]

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);
 
 	spin_lock_irq(&dev_priv->irq_lock);
-- 
2.13.6

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

[Rodrigo Vivi]

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?

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?

Also, do we still need now the call on vlv_resume_prepare?
and on i915_gem_init?

Thanks,
Rodrigo.

>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

Re: [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@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.

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@intel.com>

Thanks for all explanations.

> 
> -- 
> Ville Syrj�l�
> Intel OTC

[PATCH v2] drm/i915: Fix init_clock_gating for resume

[Ville Syrjala]

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. At least
the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.

v2: Reorder the init_clock_gating vs. modeset_init_hw to match
    the display reset path (Rodrigo)

Cc: stable@vger.kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 3423d873123a..fb584b821fb3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1709,6 +1709,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	i915_gem_resume(dev_priv);
 
 	intel_modeset_init_hw(dev);
+	intel_init_clock_gating(dev_priv);
 
 	spin_lock_irq(&dev_priv->irq_lock);
 	if (dev_priv->display.hpd_irq_setup)
-- 
2.13.6

Re: [PATCH v2] drm/i915: Fix init_clock_gating for resume

[Chris Wilson]

Quoting Ville Syrjala (2017-11-16 16:02:15)
> 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. At least
> the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
> didn't make a comeback, even after a hibernate cycle.
> 
> v2: Reorder the init_clock_gating vs. modeset_init_hw to match
>     the display reset path (Rodrigo)
> 
> Cc: stable@vger.kernel.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 3423d873123a..fb584b821fb3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1709,6 +1709,7 @@ static int i915_drm_resume(struct drm_device *dev)
>         i915_gem_resume(dev_priv);
>  
>         intel_modeset_init_hw(dev);
> +       intel_init_clock_gating(dev_priv);

The repetition of GT stuff here shouldn't be a problem, since they
should match the values in the reloaded context. (If not, we have bigger
problems!) And the bits that aren't in the context do need to be
restored.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH v2] drm/i915: Fix init_clock_gating for resume

[Ville Syrjälä]

On Thu, Nov 16, 2017 at 04:14:50PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-16 16:02:15)
> > 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. At least
> > the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
> > didn't make a comeback, even after a hibernate cycle.
> > 
> > v2: Reorder the init_clock_gating vs. modeset_init_hw to match
> >     the display reset path (Rodrigo)
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 3423d873123a..fb584b821fb3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1709,6 +1709,7 @@ static int i915_drm_resume(struct drm_device *dev)
> >         i915_gem_resume(dev_priv);
> >  
> >         intel_modeset_init_hw(dev);
> > +       intel_init_clock_gating(dev_priv);
> 
> The repetition of GT stuff here shouldn't be a problem, since they
> should match the values in the reloaded context. (If not, we have bigger
> problems!) And the bits that aren't in the context do need to be
> restored.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed to dinq. Thanks for the reviews.

-- 
Ville Syrj�l�
Intel OTC