Re: [PATCH] drm/i915: get runtime PM reference around GEM set_caching IOCTL

Imre Deak
2015-11-17T22:49:46Z

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@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