On Tue, Jan 12, 2016 at 02:21:51PM +0000, John Harrison wrote: > On 12/01/2016 14:04, Daniel Vetter wrote: > >On Tue, Jan 12, 2016 at 11:50:34AM +0000, John Harrison wrote: > >>On 12/01/2016 11:28, Chris Wilson wrote: > >>>On Tue, Jan 12, 2016 at 11:11:20AM +0000, John Harrison wrote: > >>>>On 12/01/2016 00:20, Chris Wilson wrote: > >>>>>On Mon, Jan 11, 2016 at 06:42:31PM +0000, John.C.Harrison@xxxxxxxxx wrote: > >>>>>>From: John Harrison <John.C.Harrison@xxxxxxxxx> > >>>>>> > >>>>>>A later patch in this series re-organises the batch buffer submission > >>>>>>code. Part of that is to reduce the scope of a pm_get/put pair. > >>>>>>Specifically, they previously wrapped the entire submission path from > >>>>>>the very start to the very end, now they only wrap the actual hardware > >>>>>>submission part in the back half. > >>>>>However, as you haven't fixed the ordering issue that requires rpm_get > >>>>>before struct_mutex, this is broken. > >>>>Why does 'intel_runtime_pm_get' require the struct mutex to be held? > >>>>It has certainly not complained at me about trying to do stuff > >>>>without it. > >>>Because it depends upon the struct_mutex and rpm doesn't have sufficient > >>>lockdep integration to be able to warn about using rpm from the > >>>incorrect contexts. > >>Where? What does the 'pm_runtime_get_sync' call turn into? There are already > >>other places in the driver which call intel_runtime_pm_get() immediately > >>after grabbing the mutex lock. Also, the description comment for _pm_get() > >>does not mention anything about mutexes at all. > >If you nest rpm_get within dev->struct_mutex that's a bug and could > >deadlock. Where does this happen? And for any such place we need a new > >subtest in pm_rpm. > > The first two hits when grepping the driver are in 'i915_gem_seqno_info()' > and 'i915_interrupt_info()' in i915_debugfs.c. Both say: > ret = mutex_lock_interruptible(&dev->struct_mutex); > if (ret) > return ret; > intel_runtime_pm_get(dev_priv); Yeah that's totally bonkers and will deadlock if the device is actually suspend. /me goes and files JIRAs Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
