Quoting Tvrtko Ursulin (2019-09-17 15:51:45)
>
> On 17/09/2019 08:43, Chris Wilson wrote:
> > As we need to take a walk back along the signaler timeline to find the
> > fence before upon which we want to wait, we need to lock that timeline
> > to prevent it being modified as we walk. Similarly, we also need to
> > acquire a reference to the earlier fence while it still exists!
> >
> > Though we lack the correct locking today, we are saved by the
> > overarching struct_mutex -- but that protection is being removed.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_request.c | 30 +++++++++++++++++++++++------
> > 1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index f12358150097..452ad7a8ff0c 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -767,16 +767,34 @@ i915_request_create(struct intel_context *ce)
> > static int
> > i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
> > {
> > - if (list_is_first(&signal->link, &signal->timeline->requests))
> > + struct intel_timeline *tl = signal->timeline;
> > + struct dma_fence *fence;
> > + int err;
> > +
> > + if (list_is_first(&signal->link, &tl->requests))
> > return 0;
> >
> > - signal = list_prev_entry(signal, link);
> > - if (intel_timeline_sync_is_later(rq->timeline, &signal->fence))
> > + if (mutex_lock_interruptible_nested(&tl->mutex, SINGLE_DEPTH_NESTING))
>
> We are getting more and more these nested ones and it will become
> unmanageable to remember which ones, why and on what paths. Perhaps we
> need a comment next to the member in the structure definition?
The timeline mutex is held for request construction and retire; entry
and exit of the timeline->requests. Since we have a request, it should
be holding its rq->timeline->mutex; is that what you wish documented?
Similarly that signal->timeline != rq->timeline.
GEM_BUG_ON(signal->timeline == rq->timeline);
lockdep_assert_held(&rq->timeline->mutex);
> > + return -EINTR;
> > +
> > + if (list_is_first(&signal->link, &tl->requests)) {
> > + fence = NULL;
> > + } else {
> > + signal = list_prev_entry(signal, link);
> > + fence = dma_fence_get_rcu(&signal->fence);
> > + }
> > + mutex_unlock(&tl->mutex);
> > + if (!fence)
>
> Can it be no fence when it was obtained under the mutex?
Yes. The previous fence may have been retired and so removed from the
tl->requests before we acquire the mutex. It should not be freed while
it is still in the list, i.e. dma_fence_get_rcu() should never return
NULL.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
