[Intel-gfx] [PATCH] drm/i915: Lock signaler timeline while navigating

[Tvrtko Ursulin]

On 17/09/2019 16:04, Chris Wilson wrote:
> 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 at chris-wilson.co.uk>
>>> ---
>>>    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);

Yeah that helps.

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

I did not worry about the list_is_first check, just about 
dma_fence_get_rcu. At least I'm happy that one is safe and v2 is even 
better in this respect.

Regards,

Tvrtko