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

[Antoine, Peter]

Ignore this response, replied to the wrong thread.


On Tue, 2015-05-05 at 08:05 +0100, Peter Antoine wrote:
> On Mon, 2015-05-04 at 16:55 +0200, Daniel Vetter wrote:
> > 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
> 
> Ok. More work is required.
> 
> But, we have two issues here. The open and accessible (to any user)
> ioctl that causes the driver (and in testing the kernel) to misbehave
> and cause the system to become unresponsive need or to reboot.This is
> covered by a simple de-reference check that protects the driver and the
> system.
> 
> Secondly, a nice to have, which is the hw-lock/context code that seems
> to have more issues with it and should be turned off when it is not
> required. This code has subtle connections that will need more work. The
> un-niceness of this code has been known about for a while and it would
> be good to turn off.
> 
> The first I would suggest is kind of important as is simply exploitable
> security hole, the second is just probably full of security holes.
> 
> Can we split this into two jobs, fix the actionable security hole, and
> get back to the nice to have later.
> 
> Peter.