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

[Imre Deak]

On ma, 2015-11-09 at 13:25 +0000, Chris Wilson wrote:
> On Mon, Nov 09, 2015 at 03:09:18PM +0200, Imre Deak wrote:
> > Looked through it, it seems only i915_gem_set_tiling() could
> > release
> > the PTE's without waking up the hardware (if no need to unbind the
> > object). Otherwise it's true that all callers hold (or should hold)
> > already an RPM ref. To fix the set tiling case to work after your
> > optimization we could wake up the HW unconditionally there, use a
> > no_resume RPM ref+and RPM barrier or a separate new lock for the
> > fault
> > list.
> 
> I was suggesting we move to the model where writes through gsm took
> the
> rpm reference itself.

Yes, but even then you want to have a lock around updating the new
fault list, no? So if we go with your way and push down the RPM ref
where GSM is written, we wouldn't have a lock around the fault_list
update in i915_gem_set_tiling() (via i915_gem_release_mmap()). That's
where I meant we need an extra ref/lock above.
 
> > > From the rpm point of view, this should improve the success of 
> > > runtime suspend, and reduce wakelocks.
> > 
> > Yes, seems like a worthy optimization, since I assume struct_mutex
> > can
> > be held for a long time without the need to wake up the hardware.
> 
> Admittedly most of the time we hold struct_mutex, the hw will be
> awake
> for other reasons. But there are many times where we do take the
> struct_mutex for 10s (if not 100s!) of milliseconds where the hw is
> completely idle, and so every chance to reduce usage/contention on
> struct_mutex is a little victory.
>  
> > Are you ok to first have the fix I posted and a similar one for
> >  i915_gem_set_tiling()? And then to follow-up with your plan.
> 
> Yes, adding the extra reference to that ioctl, juggling with the
> struct_mutex and then moving the rpm reference to where it is
> required
> lgtm.

Ok, will post a new one for set_tiling.

--Imre