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

From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org,
	linux-media@vger.kernel.org
Cc: daniel@ffwll.ch, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 18/26] drm/i915: use new iterator in i915_gem_object_last_write_engine
Date: Wed, 15 Sep 2021 13:19:51 +0200	[thread overview]
Message-ID: <be670538-cba3-1362-c79d-910a0bb54b3d@gmail.com> (raw)
In-Reply-To: <f9b65430-de19-7c3b-c77f-5a88114037c5@linux.intel.com>

Am 14.09.21 um 14:47 schrieb Tvrtko Ursulin:
>
> 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!

Feel free to add an Acked-by: Christian König <christian.koenig@amd.com> 
to that one.

Regards,
Christian.

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