diff mbox series

[15/26] drm/i915: use the new iterator in i915_request_await_object

Message ID 20210913131707.45639-16-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
Simplifying the code a bit.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/i915/i915_request.c | 36 ++++++-----------------------
 1 file changed, 7 insertions(+), 29 deletions(-)

Comments

Tvrtko Ursulin Sept. 14, 2021, 10:26 a.m. UTC | #1
On 13/09/2021 14:16, Christian König wrote:
> Simplifying the code a bit.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 36 ++++++-----------------------
>   1 file changed, 7 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 37aef1308573..b81045ceb619 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request *to,
>   			  struct drm_i915_gem_object *obj,
>   			  bool write)
>   {
> -	struct dma_fence *excl;
> +	struct dma_resv_cursor cursor;
> +	struct dma_fence *fence;
>   	int ret = 0;
>   
> -	if (write) {
> -		struct dma_fence **shared;
> -		unsigned int count, i;
> -
> -		ret = dma_resv_get_fences(obj->base.resv, &excl, &count,
> -					  &shared);
> -		if (ret)
> -			return ret;
> -
> -		for (i = 0; i < count; i++) {
> -			ret = i915_request_await_dma_fence(to, shared[i]);
> -			if (ret)
> -				break;
> -
> -			dma_fence_put(shared[i]);
> +	dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, fence) {

I think callers have the object locked for this one. At least if you 
haven't tried it it's worth asking CI (you have the assert already so it 
will tell you). But I think it's important to have an atomic snapshot here.

Regards,

Tvrtko

> +		ret = i915_request_await_dma_fence(to, fence);
> +		if (ret) {
> +			dma_fence_put(fence);
> +			break;
>   		}
> -
> -		for (; i < count; i++)
> -			dma_fence_put(shared[i]);
> -		kfree(shared);
> -	} else {
> -		excl = dma_resv_get_excl_unlocked(obj->base.resv);
> -	}
> -
> -	if (excl) {
> -		if (ret == 0)
> -			ret = i915_request_await_dma_fence(to, excl);
> -
> -		dma_fence_put(excl);
>   	}
>   
>   	return ret;
>
Christian König Sept. 14, 2021, 10:39 a.m. UTC | #2
Am 14.09.21 um 12:26 schrieb Tvrtko Ursulin:
>
> On 13/09/2021 14:16, Christian König wrote:
>> Simplifying the code a bit.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/i915/i915_request.c | 36 ++++++-----------------------
>>   1 file changed, 7 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_request.c 
>> b/drivers/gpu/drm/i915/i915_request.c
>> index 37aef1308573..b81045ceb619 100644
>> --- a/drivers/gpu/drm/i915/i915_request.c
>> +++ b/drivers/gpu/drm/i915/i915_request.c
>> @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request 
>> *to,
>>                 struct drm_i915_gem_object *obj,
>>                 bool write)
>>   {
>> -    struct dma_fence *excl;
>> +    struct dma_resv_cursor cursor;
>> +    struct dma_fence *fence;
>>       int ret = 0;
>>   -    if (write) {
>> -        struct dma_fence **shared;
>> -        unsigned int count, i;
>> -
>> -        ret = dma_resv_get_fences(obj->base.resv, &excl, &count,
>> -                      &shared);
>> -        if (ret)
>> -            return ret;
>> -
>> -        for (i = 0; i < count; i++) {
>> -            ret = i915_request_await_dma_fence(to, shared[i]);
>> -            if (ret)
>> -                break;
>> -
>> -            dma_fence_put(shared[i]);
>> +    dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, 
>> fence) {
>
> I think callers have the object locked for this one. At least if you 
> haven't tried it it's worth asking CI (you have the assert already so 
> it will tell you). But I think it's important to have an atomic 
> snapshot here.

Thanks for the info. In this case I'm just going to use the locked 
variant of the iterator here for the next round.

Could you point me to the place where the lock is grabed/released for 
reference?

Thanks,
Christian.

>
> Regards,
>
> Tvrtko
>
>> +        ret = i915_request_await_dma_fence(to, fence);
>> +        if (ret) {
>> +            dma_fence_put(fence);
>> +            break;
>>           }
>> -
>> -        for (; i < count; i++)
>> -            dma_fence_put(shared[i]);
>> -        kfree(shared);
>> -    } else {
>> -        excl = dma_resv_get_excl_unlocked(obj->base.resv);
>> -    }
>> -
>> -    if (excl) {
>> -        if (ret == 0)
>> -            ret = i915_request_await_dma_fence(to, excl);
>> -
>> -        dma_fence_put(excl);
>>       }
>>         return ret;
>>
Tvrtko Ursulin Sept. 14, 2021, 10:59 a.m. UTC | #3
On 14/09/2021 11:39, Christian König wrote:
> Am 14.09.21 um 12:26 schrieb Tvrtko Ursulin:
>>
>> On 13/09/2021 14:16, Christian König wrote:
>>> Simplifying the code a bit.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_request.c | 36 ++++++-----------------------
>>>   1 file changed, 7 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c 
>>> b/drivers/gpu/drm/i915/i915_request.c
>>> index 37aef1308573..b81045ceb619 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request 
>>> *to,
>>>                 struct drm_i915_gem_object *obj,
>>>                 bool write)
>>>   {
>>> -    struct dma_fence *excl;
>>> +    struct dma_resv_cursor cursor;
>>> +    struct dma_fence *fence;
>>>       int ret = 0;
>>>   -    if (write) {
>>> -        struct dma_fence **shared;
>>> -        unsigned int count, i;
>>> -
>>> -        ret = dma_resv_get_fences(obj->base.resv, &excl, &count,
>>> -                      &shared);
>>> -        if (ret)
>>> -            return ret;
>>> -
>>> -        for (i = 0; i < count; i++) {
>>> -            ret = i915_request_await_dma_fence(to, shared[i]);
>>> -            if (ret)
>>> -                break;
>>> -
>>> -            dma_fence_put(shared[i]);
>>> +    dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, 
>>> fence) {
>>
>> I think callers have the object locked for this one. At least if you 
>> haven't tried it it's worth asking CI (you have the assert already so 
>> it will tell you). But I think it's important to have an atomic 
>> snapshot here.
> 
> Thanks for the info. In this case I'm just going to use the locked 
> variant of the iterator here for the next round.
> 
> Could you point me to the place where the lock is grabed/released for 
> reference?

There is quite a few callers and I haven't audited all of them. But I 
think, given the function is used for setting up tracking of implicit 
dependencies, that it has to be true.

In the case of execbuf for instance the flow is relatively complicated:

i915_gem_do_execbuffer
   eb_relocate_parse
     eb_validate_vmas
       eb_lock_vmas
         i915_gem_object_lock
   eb_submit
     eb_move_to_gpu
       i915_request_await_object
   i915_gem_ww_ctx_fini
     i915_gem_ww_ctx_unlock_all
       i915_gem_object_unlock

Other call sites have simpler flows but there is a lot of them so I 
think using CI is easiest.

Regards,

Tvrtko

> Thanks,
> Christian.
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>> +        ret = i915_request_await_dma_fence(to, fence);
>>> +        if (ret) {
>>> +            dma_fence_put(fence);
>>> +            break;
>>>           }
>>> -
>>> -        for (; i < count; i++)
>>> -            dma_fence_put(shared[i]);
>>> -        kfree(shared);
>>> -    } else {
>>> -        excl = dma_resv_get_excl_unlocked(obj->base.resv);
>>> -    }
>>> -
>>> -    if (excl) {
>>> -        if (ret == 0)
>>> -            ret = i915_request_await_dma_fence(to, excl);
>>> -
>>> -        dma_fence_put(excl);
>>>       }
>>>         return ret;
>>>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 37aef1308573..b81045ceb619 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1583,38 +1583,16 @@  i915_request_await_object(struct i915_request *to,
 			  struct drm_i915_gem_object *obj,
 			  bool write)
 {
-	struct dma_fence *excl;
+	struct dma_resv_cursor cursor;
+	struct dma_fence *fence;
 	int ret = 0;
 
-	if (write) {
-		struct dma_fence **shared;
-		unsigned int count, i;
-
-		ret = dma_resv_get_fences(obj->base.resv, &excl, &count,
-					  &shared);
-		if (ret)
-			return ret;
-
-		for (i = 0; i < count; i++) {
-			ret = i915_request_await_dma_fence(to, shared[i]);
-			if (ret)
-				break;
-
-			dma_fence_put(shared[i]);
+	dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, fence) {
+		ret = i915_request_await_dma_fence(to, fence);
+		if (ret) {
+			dma_fence_put(fence);
+			break;
 		}
-
-		for (; i < count; i++)
-			dma_fence_put(shared[i]);
-		kfree(shared);
-	} else {
-		excl = dma_resv_get_excl_unlocked(obj->base.resv);
-	}
-
-	if (excl) {
-		if (ret == 0)
-			ret = i915_request_await_dma_fence(to, excl);
-
-		dma_fence_put(excl);
 	}
 
 	return ret;