Quoting Tvrtko Ursulin (2019-07-08 14:40:18)
>
> On 08/07/2019 14:32, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-07-08 13:22:01)
> >> Quoting Tvrtko Ursulin (2019-07-08 13:18:02)
> >>>
> >>> On 08/07/2019 12:39, Chris Wilson wrote:
> >>>> v2: Though it doesn't affect the current users, contexts may share
> >>>> timelines so check if we already hold the right mutex.
> >>
> >>>> +int intel_context_prepare_remote_request(struct intel_context *ce,
> >>>> + struct i915_request *rq)
> >>>> +{
> >>>> + struct intel_timeline *tl = ce->ring->timeline;
> >>>> + int err;
> >>>> +
> >>>> + /* Only suitable for use in remotely modifying this context */
> >>>> + GEM_BUG_ON(rq->hw_context == ce);
> >>>> +
> >>>> + if (rq->timeline != tl) { /* beware timeline sharing */
> >>>> + err = mutex_lock_interruptible_nested(&tl->mutex,
> >>>> + SINGLE_DEPTH_NESTING);
> >>>
> >>> Which caller is holding tl->mutex?
> >>
> >> None today, but it is conceivable. So rather than have a mysterious
> >> deadlock not reported by lockdep in the future, nip the problem in the
> >> bud.
> >
> > Just in case, creating a request takes the timeline->mutex and holds it
> > until submitted. So the scenario I considered was if one user context
> > wanted to modify another one (which could work), but they happen to be
> > on a common timeline.
>
> Maybe my forward vision is too limited. :) But since code consolidation
> is worth it, how about you keep that aspect in this patch, and leave the
> tl->mutex taking for later?
In the current set of patches on the list for removing struct_mutex
around requests, it is already illegal to do the operation on
tl->last_request without holding the tl->mutex.
I'm considering how best to add lockdep to highlight that. I think if I
add a struct mutex *lock to i915_active_request, and make it only exist
under debug?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
