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

[Tvrtko Ursulin]

On 19/09/2019 14:26, Chris Wilson wrote:
> 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.

But here it is under the engine->active.lock with interrupts disabled 
and the requests are fetched from execlists ports. Timeline is not 
guaranteed to be kept alive under these conditions? intel_context 
reference will be held until process_csb schedules it out so I'd expect 
timeline and hwsp to be there. But I could be lost in the new scheme of 
things.

Regards,

Tvrtko

>>> 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
>