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

[Imre Deak]

On Tue, 2015-11-17 at 22:38 +0000, Chris Wilson wrote:
> On Tue, Nov 17, 2015 at 11:16:09PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 5, 2015 at 12:56 PM, Chris Wilson <chris at chris-wilson.c
> > o.uk> wrote:
> > > The tricky part is reviewing the i915_gem_release_mmap() callers
> > > to
> > > ensure that they have the right barrier. If you make
> > > i915_gem_release_mmap() assert it holds an rpm ref, and then make
> > > the
> > > i915_gem_release_all_mmaps() unlink the node directly that should
> > > do the
> > > trick.
> > 
> > I think the easier solution would be to add a mutex for the
> > fault_list. We call release_mmap from a lot of places,
> 
> We don't though. The only times we do are when we touching hw
> registers
> (or gsm):
> 
> set_tiling_ioctl - which also may unbind and so needs rpm
> fence_write - which needs rpm to write the reigsters
> vma_unbind - which needs rpm to write through the gsm
> set_caching - which needs rpm to write through the gsm
> 
> and currently by rpm itself.
> 
> I think it is certainly reasonable to use the rpm barriers for the
> faulted list. The only one where we have to actually ensure we hold
> rpm
> is the set_tiling_ioctl.

Btw, since this would be a bigger change shouldn't we first add the new
RPM wakelock asserts? That's mostly reviewed already anyway, I still
have to check the RPS work which would need the same handling as the
hang check work (Chris' idea to use rpm_try_get there).

So how about the following until that, which is the correct way in any
case imo:

 void __suspend_report_result(const char *function, void *fn, int ret)
 {
-       if (ret)
+       if (ret == -EBUSY || ret == -EAGAIN)
+               printk(KERN_DEBUG "%s(): %pF returns %d\n", function, fn, ret);
+       else if (ret)
                printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
 }


--Imre