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

[PATCH 2/4] drm/sched: Add callback and enable signaling on debug

Posted by Arvind Yadav 3 years, 7 months ago

Here's on debug adding an enable_signaling callback for finished
fences and enabling software signaling for finished fence.

Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
---
 drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++++++
 drivers/gpu/drm/scheduler/sched_main.c  |  4 +++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index 7fd869520ef2..ebd26cdb79a0 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -122,16 +122,28 @@ static void drm_sched_fence_release_finished(struct dma_fence *f)
 
 	dma_fence_put(&fence->scheduled);
 }
+#ifdef CONFIG_DEBUG_FS
+static bool drm_sched_enable_signaling(struct dma_fence *f)
+{
+	return true;
+}
+#endif
 
 static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
 	.get_driver_name = drm_sched_fence_get_driver_name,
 	.get_timeline_name = drm_sched_fence_get_timeline_name,
+#ifdef CONFIG_DEBUG_FS
+	.enable_signaling = drm_sched_enable_signaling,
+#endif
 	.release = drm_sched_fence_release_scheduled,
 };
 
 static const struct dma_fence_ops drm_sched_fence_ops_finished = {
 	.get_driver_name = drm_sched_fence_get_driver_name,
 	.get_timeline_name = drm_sched_fence_get_timeline_name,
+#ifdef CONFIG_DEBUG_FS
+	.enable_signaling = drm_sched_enable_signaling,
+#endif
 	.release = drm_sched_fence_release_finished,
 };
 
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index e0ab14e0fb6b..140e3d8646e2 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -961,7 +961,9 @@ static int drm_sched_main(void *param)
 			s_fence->parent = dma_fence_get(fence);
 			/* Drop for original kref_init of the fence */
 			dma_fence_put(fence);
-
+#ifdef CONFIG_DEBUG_FS
+			dma_fence_enable_sw_signaling(&s_fence->finished);
+#endif
 			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_job_done_cb);
 			if (r == -ENOENT)
-- 
2.25.1

Re: [PATCH 2/4] drm/sched: Add callback and enable signaling on debug

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


Am 05.09.22 um 12:56 schrieb Arvind Yadav:
> Here's on debug adding an enable_signaling callback for finished
> fences and enabling software signaling for finished fence.
>
> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++++++
>   drivers/gpu/drm/scheduler/sched_main.c  |  4 +++-
>   2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index 7fd869520ef2..ebd26cdb79a0 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -122,16 +122,28 @@ static void drm_sched_fence_release_finished(struct dma_fence *f)
>   
>   	dma_fence_put(&fence->scheduled);
>   }
> +#ifdef CONFIG_DEBUG_FS
> +static bool drm_sched_enable_signaling(struct dma_fence *f)
> +{
> +	return true;
> +}
> +#endif
>   
>   static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
>   	.get_driver_name = drm_sched_fence_get_driver_name,
>   	.get_timeline_name = drm_sched_fence_get_timeline_name,
> +#ifdef CONFIG_DEBUG_FS
> +	.enable_signaling = drm_sched_enable_signaling,
> +#endif
>   	.release = drm_sched_fence_release_scheduled,
>   };
>   
>   static const struct dma_fence_ops drm_sched_fence_ops_finished = {
>   	.get_driver_name = drm_sched_fence_get_driver_name,
>   	.get_timeline_name = drm_sched_fence_get_timeline_name,
> +#ifdef CONFIG_DEBUG_FS
> +	.enable_signaling = drm_sched_enable_signaling,
> +#endif

Adding the callback should not be necessary.

>   	.release = drm_sched_fence_release_finished,
>   };
>   
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index e0ab14e0fb6b..140e3d8646e2 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -961,7 +961,9 @@ static int drm_sched_main(void *param)
>   			s_fence->parent = dma_fence_get(fence);
>   			/* Drop for original kref_init of the fence */
>   			dma_fence_put(fence);

Uff, not related to your patch but that looks wrong to me. The reference 
can only be dropped after the call to dma_fence_add_callback().

> -
> +#ifdef CONFIG_DEBUG_FS
> +			dma_fence_enable_sw_signaling(&s_fence->finished);
> +#endif

This should always be called, independent of the config options set.

Christian.

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

Re: [PATCH 2/4] drm/sched: Add callback and enable signaling on debug

Posted by Yadav, Arvind 3 years, 7 months ago

On 9/5/2022 4:55 PM, Christian König wrote:
>
>
> Am 05.09.22 um 12:56 schrieb Arvind Yadav:
>> Here's on debug adding an enable_signaling callback for finished
>> fences and enabling software signaling for finished fence.
>>
>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++++++
>>   drivers/gpu/drm/scheduler/sched_main.c  |  4 +++-
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c 
>> b/drivers/gpu/drm/scheduler/sched_fence.c
>> index 7fd869520ef2..ebd26cdb79a0 100644
>> --- a/drivers/gpu/drm/scheduler/sched_fence.c
>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
>> @@ -122,16 +122,28 @@ static void 
>> drm_sched_fence_release_finished(struct dma_fence *f)
>>         dma_fence_put(&fence->scheduled);
>>   }
>> +#ifdef CONFIG_DEBUG_FS
>> +static bool drm_sched_enable_signaling(struct dma_fence *f)
>> +{
>> +    return true;
>> +}
>> +#endif
>>     static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
>>       .get_driver_name = drm_sched_fence_get_driver_name,
>>       .get_timeline_name = drm_sched_fence_get_timeline_name,
>> +#ifdef CONFIG_DEBUG_FS
>> +    .enable_signaling = drm_sched_enable_signaling,
>> +#endif
>>       .release = drm_sched_fence_release_scheduled,
>>   };
>>     static const struct dma_fence_ops drm_sched_fence_ops_finished = {
>>       .get_driver_name = drm_sched_fence_get_driver_name,
>>       .get_timeline_name = drm_sched_fence_get_timeline_name,
>> +#ifdef CONFIG_DEBUG_FS
>> +    .enable_signaling = drm_sched_enable_signaling,
>> +#endif
>
> Adding the callback should not be necessary.
sure, I will remove this change.
>
>>       .release = drm_sched_fence_release_finished,
>>   };
>>   diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index e0ab14e0fb6b..140e3d8646e2 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -961,7 +961,9 @@ static int drm_sched_main(void *param)
>>               s_fence->parent = dma_fence_get(fence);
>>               /* Drop for original kref_init of the fence */
>>               dma_fence_put(fence);
>
> Uff, not related to your patch but that looks wrong to me. The 
> reference can only be dropped after the call to dma_fence_add_callback().
>
Shall I take care with this patch or I will submit separate one ?
>> -
>> +#ifdef CONFIG_DEBUG_FS
>> + dma_fence_enable_sw_signaling(&s_fence->finished);
>> +#endif
>
> This should always be called, independent of the config options set.
>
> Christian.

sure, I will remove the Config check.

~arvind

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

Re: [PATCH 2/4] drm/sched: Add callback and enable signaling on debug

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

Am 05.09.22 um 15:46 schrieb Yadav, Arvind:
> On 9/5/2022 4:55 PM, Christian König wrote:
>> [SNIP]
>>
>> Am 05.09.22 um 12:56 schrieb Arvind Yadav:
>>>       .release = drm_sched_fence_release_finished,
>>>   };
>>>   diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index e0ab14e0fb6b..140e3d8646e2 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -961,7 +961,9 @@ static int drm_sched_main(void *param)
>>>               s_fence->parent = dma_fence_get(fence);
>>>               /* Drop for original kref_init of the fence */
>>>               dma_fence_put(fence);
>>
>> Uff, not related to your patch but that looks wrong to me. The 
>> reference can only be dropped after the call to 
>> dma_fence_add_callback().
>>
> Shall I take care with this patch or I will submit separate one ?


Separate one. It's probably no big deal since we grab another reference 
right before, but still quite some broken coding.

Thanks,
Christian.

>
>>> -
>>> +#ifdef CONFIG_DEBUG_FS
>>> + dma_fence_enable_sw_signaling(&s_fence->finished);
>>> +#endif
>>
>> This should always be called, independent of the config options set.
>>
>> Christian.
>
> sure, I will remove the Config check.
>
> ~arvind
>
>>
>>>               r = dma_fence_add_callback(fence, &sched_job->cb,
>>>                              drm_sched_job_done_cb);
>>>               if (r == -ENOENT)
>>

Re: [PATCH 2/4] drm/sched: Add callback and enable signaling on debug

Posted by Yadav, Arvind 3 years, 7 months ago

On 9/5/2022 7:16 PM, Yadav, Arvind wrote:
>
> On 9/5/2022 4:55 PM, Christian König wrote:
>>
>>
>> Am 05.09.22 um 12:56 schrieb Arvind Yadav:
>>> Here's on debug adding an enable_signaling callback for finished
>>> fences and enabling software signaling for finished fence.
>>>
>>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++++++
>>>   drivers/gpu/drm/scheduler/sched_main.c  |  4 +++-
>>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c 
>>> b/drivers/gpu/drm/scheduler/sched_fence.c
>>> index 7fd869520ef2..ebd26cdb79a0 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_fence.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
>>> @@ -122,16 +122,28 @@ static void 
>>> drm_sched_fence_release_finished(struct dma_fence *f)
>>>         dma_fence_put(&fence->scheduled);
>>>   }
>>> +#ifdef CONFIG_DEBUG_FS
>>> +static bool drm_sched_enable_signaling(struct dma_fence *f)
>>> +{
>>> +    return true;
>>> +}
>>> +#endif
>>>     static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
>>>       .get_driver_name = drm_sched_fence_get_driver_name,
>>>       .get_timeline_name = drm_sched_fence_get_timeline_name,
>>> +#ifdef CONFIG_DEBUG_FS
>>> +    .enable_signaling = drm_sched_enable_signaling,
>>> +#endif
>>>       .release = drm_sched_fence_release_scheduled,
>>>   };
>>>     static const struct dma_fence_ops drm_sched_fence_ops_finished = {
>>>       .get_driver_name = drm_sched_fence_get_driver_name,
>>>       .get_timeline_name = drm_sched_fence_get_timeline_name,
>>> +#ifdef CONFIG_DEBUG_FS
>>> +    .enable_signaling = drm_sched_enable_signaling,
>>> +#endif
>>
>> Adding the callback should not be necessary.
> sure, I will remove this change.
>>
>>>       .release = drm_sched_fence_release_finished,
>>>   };
>>>   diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index e0ab14e0fb6b..140e3d8646e2 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -961,7 +961,9 @@ static int drm_sched_main(void *param)
>>>               s_fence->parent = dma_fence_get(fence);
>>>               /* Drop for original kref_init of the fence */
>>>               dma_fence_put(fence);
>>
>> Uff, not related to your patch but that looks wrong to me. The 
>> reference can only be dropped after the call to 
>> dma_fence_add_callback().
>>
> Shall I take care with this patch or I will submit separate one ?

This changes was recently added by  Andrey Grodzovsky (commit : 
45ecaea73) to fix the negative refcount.

>>> -
>>> +#ifdef CONFIG_DEBUG_FS
>>> + dma_fence_enable_sw_signaling(&s_fence->finished);
>>> +#endif
>>
>> This should always be called, independent of the config options set.
>>
>> Christian.
>
> sure, I will remove the Config check.
>
> ~arvind
>
>>
>>>               r = dma_fence_add_callback(fence, &sched_job->cb,
>>>                              drm_sched_job_done_cb);
>>>               if (r == -ENOENT)
>>