From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com ([192.55.52.93]:24124 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755356AbdKMUqO (ORCPT ); Mon, 13 Nov 2017 15:46:14 -0500 Date: Mon, 13 Nov 2017 22:46:10 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Rodrigo Vivi Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix init_clock_gating for resume Message-ID: <20171113204610.GX10981@intel.com> References: <20171113145028.10362-1-ville.syrjala@linux.intel.com> <20171113190131.t4673jf5mbokqe22@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20171113190131.t4673jf5mbokqe22@intel.com> Sender: stable-owner@vger.kernel.org List-ID: 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� > > > > 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 > > Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was") > > Signed-off-by: Ville Syrj�l� > > --- > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix init_clock_gating for resume Date: Mon, 13 Nov 2017 22:46:10 +0200 Message-ID: <20171113204610.GX10981@intel.com> References: <20171113145028.10362-1-ville.syrjala@linux.intel.com> <20171113190131.t4673jf5mbokqe22@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <20171113190131.t4673jf5mbokqe22@intel.com> Sender: stable-owner@vger.kernel.org To: Rodrigo Vivi Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org 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ä > > > > 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 > > Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was") > > Signed-off-by: Ville Syrjälä > > --- > > 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