diff mbox series

[18/26] drm/i915: use new iterator in i915_gem_object_last_write_engine

Message ID 20210913131707.45639-19-christian.koenig@amd.com (mailing list archive)
State New, archived
Headers show
Series [01/26] dma-buf: add dma_resv_for_each_fence_unlocked | expand

Commit Message

Christian König Sept. 13, 2021, 1:16 p.m. UTC
This is maybe even a fix since the RCU usage here looks incorrect.

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

Comments

Tvrtko Ursulin Sept. 14, 2021, 12:27 p.m. UTC | #1
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?

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.

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;
>   }
>   
>
Christian König Sept. 14, 2021, 12:32 p.m. UTC | #2
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.

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

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;
>>   }
>>
Tvrtko Ursulin Sept. 14, 2021, 12:47 p.m. UTC | #3
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;
>>>   }
>>>
>
Christian König Sept. 15, 2021, 11:19 a.m. UTC | #4
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;
>>>>   }
>>>>
>>
diff mbox series

Patch

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