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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
