Re: [Intel-gfx] [PATCH 14/26] drm/i915: use the new iterator in i915_sw_fence_await_reservation v3

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 20.09.21 um 10:47 schrieb Tvrtko Ursulin:
>
> On 20/09/2021 09:45, Tvrtko Ursulin wrote:
>>
>> On 17/09/2021 13:35, Christian König wrote:
>>> Simplifying the code a bit.
>>>
>>> v2: use dma_resv_for_each_fence instead, according to Tvrtko the 
>>> lock is
>>>      held here anyway.
>>> v3: back to using dma_resv_for_each_fence_unlocked.
>>
>> It did not work out - what happened?
> Wait, my suggestion to try the locked iterator was against 
> i915_request_await_object. I haven't looked at this one at the time or 
> even now.

Exactly! I've mixed the two up and this one here is really called 
without holding a lock.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>
>> Regards,
>>
>> Tvrtko
>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_sw_fence.c | 57 
>>> ++++++++--------------------
>>>   1 file changed, 15 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c 
>>> b/drivers/gpu/drm/i915/i915_sw_fence.c
>>> index c589a681da77..7635b0478ea5 100644
>>> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
>>> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
>>> @@ -572,56 +572,29 @@ int i915_sw_fence_await_reservation(struct 
>>> i915_sw_fence *fence,
>>>                       unsigned long timeout,
>>>                       gfp_t gfp)
>>>   {
>>> -    struct dma_fence *excl;
>>> +    struct dma_resv_iter cursor;
>>> +    struct dma_fence *f;
>>>       int ret = 0, pending;
>>>       debug_fence_assert(fence);
>>>       might_sleep_if(gfpflags_allow_blocking(gfp));
>>> -    if (write) {
>>> -        struct dma_fence **shared;
>>> -        unsigned int count, i;
>>> -
>>> -        ret = dma_resv_get_fences(resv, &excl, &count, &shared);
>>> -        if (ret)
>>> -            return ret;
>>> -
>>> -        for (i = 0; i < count; i++) {
>>> -            if (shared[i]->ops == exclude)
>>> -                continue;
>>> -
>>> -            pending = i915_sw_fence_await_dma_fence(fence,
>>> -                                shared[i],
>>> -                                timeout,
>>> -                                gfp);
>>> -            if (pending < 0) {
>>> -                ret = pending;
>>> -                break;
>>> -            }
>>> -
>>> -            ret |= pending;
>>> -        }
>>> -
>>> -        for (i = 0; i < count; i++)
>>> -            dma_fence_put(shared[i]);
>>> -        kfree(shared);
>>> -    } else {
>>> -        excl = dma_resv_get_excl_unlocked(resv);
>>> -    }
>>> -
>>> -    if (ret >= 0 && excl && excl->ops != exclude) {
>>> -        pending = i915_sw_fence_await_dma_fence(fence,
>>> -                            excl,
>>> -                            timeout,
>>> +    rcu_read_lock();
>>> +    dma_resv_iter_begin(&cursor, resv, write);
>>> +    dma_resv_for_each_fence_unlocked(&cursor, f) {
>>> +        rcu_read_unlock();
>>> +        pending = i915_sw_fence_await_dma_fence(fence, f, timeout,
>>>                               gfp);
>>> -        if (pending < 0)
>>> +        rcu_read_lock();
>>> +        if (pending < 0) {
>>>               ret = pending;
>>> -        else
>>> -            ret |= pending;
>>> -    }
>>> -
>>> -    dma_fence_put(excl);
>>> +            break;
>>> +        }
>>> +        ret |= pending;
>>> +    }
>>> +    dma_resv_iter_end(&cursor);
>>> +    rcu_read_unlock();
>>>       return ret;
>>>   }
>>>