Re: [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2019-09-19 14:02:19)
> 
> On 19/09/2019 12:19, Chris Wilson wrote:
> > +static struct intel_timeline *get_timeline(struct i915_request *rq)
> > +{
> > +     struct intel_timeline *tl;
> > +
> > +     /*
> > +      * Even though we are holding the engine->active.lock here, there
> > +      * is no control over the submission queue per-se and we are
> > +      * inspecting the active state at a random point in time, with an
> > +      * unknown queue. Play safe and make sure the timeline remains valid.
> > +      * (Only being used for pretty printing, one extra kref shouldn't
> > +      * cause a camel stampede!)
> > +      */
> > +     rcu_read_lock();
> > +     tl = rcu_dereference(rq->timeline);
> > +     if (!kref_get_unless_zero(&tl->kref))
> > +             tl = NULL;
> > +     rcu_read_unlock();
> 
> How can it be NULL under the active lock? Isn't that the same assertion 
> from i915_timeline_get_active.

Not NULL, but retired. The difference is that during submission we know
that this request's context/timeline must be currently pinned until
a subsequent request (containing the idle-barriers) is submitted. The
danger I worry about here is that subsequent idle request may be already
submitted and since the queued requests may *already* have been retired,
the timeline may be unpinned and indeed dropped it's last reference.

> > diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
> > index 9d1ea26c7a2d..4ce1e25433d2 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_context.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
> > @@ -14,22 +14,28 @@
> >   
> >   static int request_sync(struct i915_request *rq)
> >   {
> > +     struct intel_timeline *tl = i915_request_timeline(rq);
> >       long timeout;
> >       int err = 0;
> >   
> > +     intel_timeline_get(tl);
> >       i915_request_get(rq);
> >   
> > -     i915_request_add(rq);
> > +     /* Opencode i915_request_add() so we can keep the timeline locked. */
> > +     __i915_request_commit(rq);
> > +     __i915_request_queue(rq, NULL);
> 
> Looking at i915_request_add here we also have tasklet kicking but I 
> guess for selftests it's not important.

Yup, and immediately before a wait, that tasklet should be scheduled
naturally in the near future.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx