[Intel-gfx] [PATCH] drm/i915: get runtime PM reference around GEM set_caching IOCTL

[Chris Wilson]

On Fri, Nov 06, 2015 at 12:57:35AM +0200, Imre Deak wrote:
> On Thu, 2015-11-05 at 11:56 +0000, Chris Wilson wrote:
> > On Thu, Nov 05, 2015 at 01:28:32PM +0200, Imre Deak wrote:
> > > On ke, 2015-11-04 at 20:57 +0000, Chris Wilson wrote:
> > > > On Wed, Nov 04, 2015 at 09:25:32PM +0200, Imre Deak wrote:
> > > > > After Damien's D3 fix I started to get runtime suspend residency for the
> > > > > first time and that revealed a breakage on the set_caching IOCTL path
> > > > > that accesses the HW but doesn't take an RPM ref. Fix this up.
> > > > 
> > > > Why here (and in so many random places) and not around the PTE write
> > > > itself?
> > > 
> > > Imo we should take the RPM ref outside of any of our locks. Otherwise we
> > > need hacks like we have currently in the runtime suspend handler to work
> > > around lock inversions. It works now, but we couldn't do the same trick
> > > if we needed to take struct_mutex for example in the resume handler too
> > > for some reason.
> > 
> > On the other hand, it leads to hard to track down bugs and improper
> > documentation. Neither solution is perfect.
> 
> I think I understand the documentation part, not sure how that could be
> solved. Perhaps RPM-held asserts right before the point where the HW
> needs to be on?
> 
> What do you mean by hard to track down bugs? Couldn't that also be
> tackled by asserts?
> 
> > Note since intel_runtime_suspend has ample barriers of its own, you can
> > avoid the struct_mutex if you have a dedicated dev_priv->mm.fault_list.
> > 
> > Something along the lines of:
> 
> Ok, looks interesting. But as you said we would have to then make
> callers of i915_gem_release_mmap() wake up the device, which isn't
> strictly necessary. (and imo as such goes somewhat against the
> documentation argument)

Outside of suspend, we only revoke the CPU PTE mapping when we change
the hardware (as doing so is very expensive). So all callers should
already have a reason (and be holding) the rpm wakelock, the only
complication from my point of view is enforcing that and reviewing that
what I said is true.

>From the rpm point of view, this should improve the success of runtime
suspend, and reduce wakelocks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre