Re: [Intel-gfx] [PATCH 18/26] drm/i915: use new iterator in i915_gem_object_last_write_engine

Tvrtko Ursulin
2021-09-14T12:47:30Z

On 14/09/2021 13:32, Christian König wrote:
> Am 14.09.21 um 14:27 schrieb Tvrtko Ursulin:
>>
>> On 13/09/2021 14:16, Christian König wrote:
>>> This is maybe even a fix since the RCU usage here looks incorrect.
>>
>> What you think is incorrect? Pointless extra rcu locking?
>
> Yeah, exactly that. I also wondered for a second if rcu_read_lock() can
> nest or not. But obviously it either works or lockdep hasn't complained
> yet.
>
> But I've made a mistake here and at a couple of other places to remove
> to many rcu_read_lock() calls. Thanks for pointing that out, going to
> fix it as well.

Ack.

>> Also, FWIW, I submitted a patch to remove this function altogether
>> since its IMO pretty useless, just failed in getting anyone to ack it
>> so far.
>
> I was on the edge of suggesting that as well since it's only debugfs
> usage looked quite pointless to me.
>
> Feel free to CC me on the patch and you can have my acked-by.

Patch is here
https://patchwork.freedesktop.org/patch/451864/?series=94202&rev=1, thanks!

Regards,

Tvrtko

> Thanks,
> Christian.
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 +++++++--------
>>>   1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> index e9eecebf5c9d..3343922af4d6 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> @@ -500,16 +500,15 @@ static inline struct intel_engine_cs *
>>>   i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj)
>>>   {
>>>       struct intel_engine_cs *engine = NULL;
>>> +    struct dma_resv_cursor cursor;
>>>       struct dma_fence *fence;
>>>   -    rcu_read_lock();
>>> -    fence = dma_resv_get_excl_unlocked(obj->base.resv);
>>> -    rcu_read_unlock();
>>> -
>>> -    if (fence && dma_fence_is_i915(fence) &&
>>> !dma_fence_is_signaled(fence))
>>> -        engine = to_request(fence)->engine;
>>> -    dma_fence_put(fence);
>>> -
>>> +    dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false,
>>> +                     fence) {
>>> +        if (fence && dma_fence_is_i915(fence) &&
>>> +            !dma_fence_is_signaled(fence))
>>> +            engine = to_request(fence)->engine;
>>> +    }
>>>       return engine;
>>>   }
>>>
>