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