Re: [PATCH v2] drm/i915: Avoid GPU hang when coming out of S3 or S4

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	"Antoine, Peter" <peter.antoine@intel.com>,
	Deepak S <deepak.s@linux.intel.com>,
	"S, Deepak" <deepak.s@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Tian, YeX" <yex.tian@intel.com>
Subject: Re: [PATCH v2] drm/i915: Avoid GPU hang when coming out of S3 or S4
Date: Mon, 4 May 2015 16:55:09 +0200	[thread overview]
Message-ID: <20150504145509.GQ30184@phenom.ffwll.local> (raw)
In-Reply-To: <20150429113917.GQ599@nuc-i3427.alporthouse.com>

On Wed, Apr 29, 2015 at 12:39:17PM +0100, Chris Wilson wrote:
> 
> On Wed, Apr 29, 2015 at 02:07:19PM +0300, David Weinehall wrote:
> > On Tue, Apr 28, 2015 at 03:46:46PM +0100, Chris Wilson wrote:
> > > On Tue, Apr 28, 2015 at 02:38:25PM +0000, Antoine, Peter wrote:
> > > > So is the plan to push these patches and have follow-on work to cover the other paths?
> > > > As this fixes the Bugzilla issue that has been raised.
> > > 
> > > You've identified an issue, but I think your patch is incomplete.
> > 
> > I've tried my best to go through the remaining similar-looking code,
> > but the rest seems fine (I might've missed something though).
> > 
> > The only thing I reacted on was that in intel_runtime_resume() the call
> > to intel_init_pch_refclk() is conditional on IS_GEN6(), but none of the
> > other invocations of intel_init_pch_refclk() are.  The commit message
> > doesn't seem to provide a sufficient explanation for why this is so.
> 
> The explanation for moving init_hw() was that it setups clock_gating. If
> any in that path are required for enabling the rings, those should be move to
> init_render_ring() or the init_context.

Yeah we've had this fun before. init_clock_gating is _not_ for GT
workarounds, only for modeset workarounds. We might need to rename that
hook to avoid getting bitten for eternity, but moving init_hw because of
clock_gating to get the rings up and running is bogus.

As Chris said we need to identify which bits need to get moved to the ring
init or w/a bb code and do that (and in a separate patch from enabling the
interrupts early enough like this one here does).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx