| Message ID | 20171113145028.10362-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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().
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
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);