[v2] dma-buf: To check enable signaling before signaled

[PATCH v2 1/4] drm/sched: Enable signaling for finished fence

Posted by Arvind Yadav 3 years, 7 months ago

Here's enabling software signaling for finished fence.

Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
---

Changes in v1 :
1- Addressing Christian's comment to remove CONFIG_DEBUG_FS check from 
this patch. 
2- The version of this patch is also changed and previously
it was [PATCH 2/4]

---
 drivers/gpu/drm/scheduler/sched_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index e0ab14e0fb6b..fe72de0e2911 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -962,6 +962,8 @@ static int drm_sched_main(void *param)
 			/* Drop for original kref_init of the fence */
 			dma_fence_put(fence);
 
+			dma_fence_enable_sw_signaling(&s_fence->finished);
+
 			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_job_done_cb);
 			if (r == -ENOENT)
-- 
2.25.1

Re: [PATCH v2 1/4] drm/sched: Enable signaling for finished fence

Posted by Christian König 3 years, 7 months ago

Am 05.09.22 um 18:34 schrieb Arvind Yadav:
> Here's enabling software signaling for finished fence.
>
> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
> ---
>
> Changes in v1 :
> 1- Addressing Christian's comment to remove CONFIG_DEBUG_FS check from
> this patch.
> 2- The version of this patch is also changed and previously
> it was [PATCH 2/4]
>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index e0ab14e0fb6b..fe72de0e2911 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -962,6 +962,8 @@ static int drm_sched_main(void *param)
>   			/* Drop for original kref_init of the fence */
>   			dma_fence_put(fence);
>   
> +			dma_fence_enable_sw_signaling(&s_fence->finished);

Ok, this makes it a lot clearer. Previously I though that we have some 
bug in dma_fence_add_callback().

This is essentially the wrong place to call this, the finished fence 
should be enabled by the caller and not here.

There is also another problem in dma_fence_enable_sw_signaling(), it 
returns early when the fence is already signaled:

         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
                 return;

Please remove that one first.

Thanks,
Christian.


> +
>   			r = dma_fence_add_callback(fence, &sched_job->cb,
>   						   drm_sched_job_done_cb);
>   			if (r == -ENOENT)

Re: [PATCH v2 1/4] drm/sched: Enable signaling for finished fence

Posted by Andrey Grodzovsky 3 years, 7 months ago

On 2022-09-06 02:34, Christian König wrote:
> Am 05.09.22 um 18:34 schrieb Arvind Yadav:
>> Here's enabling software signaling for finished fence.
>>
>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>> ---
>>
>> Changes in v1 :
>> 1- Addressing Christian's comment to remove CONFIG_DEBUG_FS check from
>> this patch.
>> 2- The version of this patch is also changed and previously
>> it was [PATCH 2/4]
>>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index e0ab14e0fb6b..fe72de0e2911 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -962,6 +962,8 @@ static int drm_sched_main(void *param)
>>               /* Drop for original kref_init of the fence */
>>               dma_fence_put(fence);
>>   + dma_fence_enable_sw_signaling(&s_fence->finished);
>
> Ok, this makes it a lot clearer. Previously I though that we have some 
> bug in dma_fence_add_callback().
>
> This is essentially the wrong place to call this, the finished fence 
> should be enabled by the caller and not here.
>
> There is also another problem in dma_fence_enable_sw_signaling(), it 
> returns early when the fence is already signaled:
>
>         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>                 return;
>
> Please remove that one first.


Why we even need this explicit call if dma_fence_add_callback calls 
__dma_fence_enable_signaling anyway ?

Andrey


>
> Thanks,
> Christian.
>
>
>> +
>>               r = dma_fence_add_callback(fence, &sched_job->cb,
>>                              drm_sched_job_done_cb);
>>               if (r == -ENOENT)
>

Re: [PATCH v2 1/4] drm/sched: Enable signaling for finished fence

Posted by Christian König 3 years, 7 months ago

Am 06.09.22 um 21:55 schrieb Andrey Grodzovsky:
>
> On 2022-09-06 02:34, Christian König wrote:
>> Am 05.09.22 um 18:34 schrieb Arvind Yadav:
>>> Here's enabling software signaling for finished fence.
>>>
>>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>>> ---
>>>
>>> Changes in v1 :
>>> 1- Addressing Christian's comment to remove CONFIG_DEBUG_FS check from
>>> this patch.
>>> 2- The version of this patch is also changed and previously
>>> it was [PATCH 2/4]
>>>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_main.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index e0ab14e0fb6b..fe72de0e2911 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -962,6 +962,8 @@ static int drm_sched_main(void *param)
>>>               /* Drop for original kref_init of the fence */
>>>               dma_fence_put(fence);
>>>   + dma_fence_enable_sw_signaling(&s_fence->finished);
>>
>> Ok, this makes it a lot clearer. Previously I though that we have 
>> some bug in dma_fence_add_callback().
>>
>> This is essentially the wrong place to call this, the finished fence 
>> should be enabled by the caller and not here.
>>
>> There is also another problem in dma_fence_enable_sw_signaling(), it 
>> returns early when the fence is already signaled:
>>
>>         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>                 return;
>>
>> Please remove that one first.
>
>
> Why we even need this explicit call if dma_fence_add_callback calls 
> __dma_fence_enable_signaling anyway ?

Two different fence objects.

The dma_fence_add_callback() is done on the hw fence we get in return of 
submitting the job.

The dma_fence_enable_sw_signaling() here is done on the finished fence 
we use to signal the completion externally.

Key point is the finished fence should be used by the frontend drivers 
which uses the scheduler and not by the scheduler itself.

Christian.

>
> Andrey
>
>
>>
>> Thanks,
>> Christian.
>>
>>
>>> +
>>>               r = dma_fence_add_callback(fence, &sched_job->cb,
>>>                              drm_sched_job_done_cb);
>>>               if (r == -ENOENT)
>>

Re: [PATCH v2 1/4] drm/sched: Enable signaling for finished fence

Posted by Andrey Grodzovsky 3 years, 7 months ago

On 2022-09-07 02:37, Christian König wrote:
> Am 06.09.22 um 21:55 schrieb Andrey Grodzovsky:
>>
>> On 2022-09-06 02:34, Christian König wrote:
>>> Am 05.09.22 um 18:34 schrieb Arvind Yadav:
>>>> Here's enabling software signaling for finished fence.
>>>>
>>>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>>>> ---
>>>>
>>>> Changes in v1 :
>>>> 1- Addressing Christian's comment to remove CONFIG_DEBUG_FS check from
>>>> this patch.
>>>> 2- The version of this patch is also changed and previously
>>>> it was [PATCH 2/4]
>>>>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/sched_main.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index e0ab14e0fb6b..fe72de0e2911 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -962,6 +962,8 @@ static int drm_sched_main(void *param)
>>>>               /* Drop for original kref_init of the fence */
>>>>               dma_fence_put(fence);
>>>>   + dma_fence_enable_sw_signaling(&s_fence->finished);
>>>
>>> Ok, this makes it a lot clearer. Previously I though that we have 
>>> some bug in dma_fence_add_callback().
>>>
>>> This is essentially the wrong place to call this, the finished fence 
>>> should be enabled by the caller and not here.
>>>
>>> There is also another problem in dma_fence_enable_sw_signaling(), it 
>>> returns early when the fence is already signaled:
>>>
>>>         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>>                 return;
>>>
>>> Please remove that one first.
>>
>>
>> Why we even need this explicit call if dma_fence_add_callback calls 
>> __dma_fence_enable_signaling anyway ?
>
> Two different fence objects.
>
> The dma_fence_add_callback() is done on the hw fence we get in return 
> of submitting the job.
>
> The dma_fence_enable_sw_signaling() here is done on the finished fence 
> we use to signal the completion externally.
>
> Key point is the finished fence should be used by the frontend drivers 
> which uses the scheduler and not by the scheduler itself.
>
> Christian.


Oh, so we need to explicitly call this because dma_fence_add_callback is 
not always used for finished fence right ?

Yea, then it makes sense that the client needs to manage this since each 
one has his own logic what to do with it.

Andrey


>
>>
>> Andrey
>>
>>
>>>
>>> Thanks,
>>> Christian.
>>>
>>>
>>>> +
>>>>               r = dma_fence_add_callback(fence, &sched_job->cb,
>>>>                              drm_sched_job_done_cb);
>>>>               if (r == -ENOENT)
>>>
>