[PATCH 0/5] drm/sched: Create wrapper to add a syncobj dependency to job

[PATCH 0/5] drm/sched: Create wrapper to add a syncobj dependency to job

[Maíra Canal]

Some drivers perform the same operation to add a syncobj's fence to the sched
as a dependency: first, call drm_syncobj_find_fence() to find the fence and
then, call drm_sched_job_add_dependency(). Therefore, create a wrapper to
encapsulate those steps in one single function.

The first patch creates the wrapper for the operation and the following
patches make the drivers use the new function drm_sched_job_add_syncobj_dependency().

Best Regards,
- Maíra Canal

Maíra Canal (5):
  drm/sched: Create wrapper to add a syncobj dependency to job
  drm/lima: Use drm_sched_job_add_syncobj_dependency()
  drm/msm: Use drm_sched_job_add_syncobj_dependency()
  drm/panfrost: Use drm_sched_job_add_syncobj_dependency()
  drm/v3d: Use drm_sched_job_add_syncobj_dependency()

 drivers/gpu/drm/lima/lima_gem.c         | 12 ++--------
 drivers/gpu/drm/msm/msm_gem_submit.c    |  8 ++-----
 drivers/gpu/drm/panfrost/panfrost_drv.c | 11 ++--------
 drivers/gpu/drm/scheduler/sched_main.c  | 29 +++++++++++++++++++++++++
 drivers/gpu/drm/v3d/v3d_gem.c           |  9 +-------
 include/drm/gpu_scheduler.h             |  6 +++++
 6 files changed, 42 insertions(+), 33 deletions(-)

-- 
2.39.1

[PATCH 1/5] drm/sched: Create wrapper to add a syncobj dependency to job

[Maíra Canal]

In order to add a syncobj's fence as a dependency to a job, it is
necessary to call drm_syncobj_find_fence() to find the fence and then
add the dependency with drm_sched_job_add_dependency(). So, wrap these
steps in one single function, drm_sched_job_add_syncobj_dependency().

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 29 ++++++++++++++++++++++++++
 include/drm/gpu_scheduler.h            |  6 ++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 0e4378420271..d5331b1877a3 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -53,6 +53,7 @@
 
 #include <drm/drm_print.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_syncobj.h>
 #include <drm/gpu_scheduler.h>
 #include <drm/spsc_queue.h>
 
@@ -718,6 +719,34 @@ int drm_sched_job_add_dependency(struct drm_sched_job *job,
 }
 EXPORT_SYMBOL(drm_sched_job_add_dependency);
 
+/**
+ * drm_sched_job_add_syncobj_dependency - adds a syncobj's fence as a job dependency
+ * @job: scheduler job to add the dependencies to
+ * @file_private: drm file private pointer
+ * @handle: syncobj handle to lookup
+ * @point: timeline point
+ *
+ * This adds the fence matching the given syncobj to @job.
+ *
+ * Returns:
+ * 0 on success, or an error on failing to expand the array.
+ */
+int drm_sched_job_add_syncobj_dependency(struct drm_sched_job *job,
+					 struct drm_file *file,
+					 u32 handle,
+					 u32 point)
+{
+	struct dma_fence *fence;
+	int ret = 0;
+
+	ret = drm_syncobj_find_fence(file, handle, point, 0, &fence);
+	if (ret)
+		return ret;
+
+	return drm_sched_job_add_dependency(job, fence);
+}
+EXPORT_SYMBOL(drm_sched_job_add_syncobj_dependency);
+
 /**
  * drm_sched_job_add_resv_dependencies - add all fences from the resv to the job
  * @job: scheduler job to add the dependencies to
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9935d1e2ff69..4cc54f8b57b4 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -48,6 +48,8 @@ struct drm_gem_object;
 struct drm_gpu_scheduler;
 struct drm_sched_rq;
 
+struct drm_file;
+
 /* These are often used as an (initial) index
  * to an array, and as such should start at 0.
  */
@@ -515,6 +517,10 @@ int drm_sched_job_init(struct drm_sched_job *job,
 void drm_sched_job_arm(struct drm_sched_job *job);
 int drm_sched_job_add_dependency(struct drm_sched_job *job,
 				 struct dma_fence *fence);
+int drm_sched_job_add_syncobj_dependency(struct drm_sched_job *job,
+					 struct drm_file *file,
+					 u32 handle,
+					 u32 point);
 int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
 					struct dma_resv *resv,
 					enum dma_resv_usage usage);
-- 
2.39.1

Re: [PATCH 1/5] drm/sched: Create wrapper to add a syncobj dependency to job

[Christian König]

Am 08.02.23 um 20:48 schrieb Maíra Canal:
> In order to add a syncobj's fence as a dependency to a job, it is
> necessary to call drm_syncobj_find_fence() to find the fence and then
> add the dependency with drm_sched_job_add_dependency(). So, wrap these
> steps in one single function, drm_sched_job_add_syncobj_dependency().
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Just one nit pick below, with that fixed Reviewed-by: Christian König 
<christian.koenig@amd.com>

I'm pretty sure we have the exact same function now in amdgpu cause I 
cleaned that up just a few weeks ago to look the same as the other drivers.

Would be nice to have that new function applied there as well.

Thanks,
Christian.

> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 29 ++++++++++++++++++++++++++
>   include/drm/gpu_scheduler.h            |  6 ++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 0e4378420271..d5331b1877a3 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -53,6 +53,7 @@
>   
>   #include <drm/drm_print.h>
>   #include <drm/drm_gem.h>
> +#include <drm/drm_syncobj.h>
>   #include <drm/gpu_scheduler.h>
>   #include <drm/spsc_queue.h>
>   
> @@ -718,6 +719,34 @@ int drm_sched_job_add_dependency(struct drm_sched_job *job,
>   }
>   EXPORT_SYMBOL(drm_sched_job_add_dependency);
>   
> +/**
> + * drm_sched_job_add_syncobj_dependency - adds a syncobj's fence as a job dependency
> + * @job: scheduler job to add the dependencies to
> + * @file_private: drm file private pointer
> + * @handle: syncobj handle to lookup
> + * @point: timeline point
> + *
> + * This adds the fence matching the given syncobj to @job.
> + *
> + * Returns:
> + * 0 on success, or an error on failing to expand the array.
> + */
> +int drm_sched_job_add_syncobj_dependency(struct drm_sched_job *job,
> +					 struct drm_file *file,
> +					 u32 handle,
> +					 u32 point)
> +{
> +	struct dma_fence *fence;
> +	int ret = 0;

Please don't initialize any local return variables if it isn't necessary.

This just suppresses uninitialized variables from the compiler which 
quite often have helped finding more wider bugs.

Regards,
Christian.

> +
> +	ret = drm_syncobj_find_fence(file, handle, point, 0, &fence);
> +	if (ret)
> +		return ret;
> +
> +	return drm_sched_job_add_dependency(job, fence);
> +}
> +EXPORT_SYMBOL(drm_sched_job_add_syncobj_dependency);
> +
>   /**
>    * drm_sched_job_add_resv_dependencies - add all fences from the resv to the job
>    * @job: scheduler job to add the dependencies to
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 9935d1e2ff69..4cc54f8b57b4 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -48,6 +48,8 @@ struct drm_gem_object;
>   struct drm_gpu_scheduler;
>   struct drm_sched_rq;
>   
> +struct drm_file;
> +
>   /* These are often used as an (initial) index
>    * to an array, and as such should start at 0.
>    */
> @@ -515,6 +517,10 @@ int drm_sched_job_init(struct drm_sched_job *job,
>   void drm_sched_job_arm(struct drm_sched_job *job);
>   int drm_sched_job_add_dependency(struct drm_sched_job *job,
>   				 struct dma_fence *fence);
> +int drm_sched_job_add_syncobj_dependency(struct drm_sched_job *job,
> +					 struct drm_file *file,
> +					 u32 handle,
> +					 u32 point);
>   int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>   					struct dma_resv *resv,
>   					enum dma_resv_usage usage);

Re: [PATCH 1/5] drm/sched: Create wrapper to add a syncobj dependency to job

[Luben Tuikov]

On 2023-02-08 14:54, Christian König wrote:
> Am 08.02.23 um 20:48 schrieb Maíra Canal:
>> In order to add a syncobj's fence as a dependency to a job, it is
>> necessary to call drm_syncobj_find_fence() to find the fence and then
>> add the dependency with drm_sched_job_add_dependency(). So, wrap these
>> steps in one single function, drm_sched_job_add_syncobj_dependency().
>>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> Just one nit pick below, with that fixed Reviewed-by: Christian König 
> <christian.koenig@amd.com>
>
> I'm pretty sure we have the exact same function now in amdgpu cause I 
> cleaned that up just a few weeks ago to look the same as the other drivers.
>
> Would be nice to have that new function applied there as well.

Hi Christian,

Is that R-B for the series or just this patch?
-- 
Regards,
Luben

Re: [PATCH 1/5] drm/sched: Create wrapper to add a syncobj dependency to job

[Christian König]

Am 08.02.23 um 23:20 schrieb Luben Tuikov:
> On 2023-02-08 14:54, Christian König wrote:
>> Am 08.02.23 um 20:48 schrieb Maíra Canal:
>>> In order to add a syncobj's fence as a dependency to a job, it is
>>> necessary to call drm_syncobj_find_fence() to find the fence and then
>>> add the dependency with drm_sched_job_add_dependency(). So, wrap these
>>> steps in one single function, drm_sched_job_add_syncobj_dependency().
>>>
>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> Just one nit pick below, with that fixed Reviewed-by: Christian König
>> <christian.koenig@amd.com>
>>
>> I'm pretty sure we have the exact same function now in amdgpu cause I
>> cleaned that up just a few weeks ago to look the same as the other drivers.
>>
>> Would be nice to have that new function applied there as well.
> Hi Christian,
>
> Is that R-B for the series or just this patch?

Just this patch, haven't looked at the driver implementations in detail.

Regards,
Christian.

Re: [PATCH 1/5] drm/sched: Create wrapper to add a syncobj dependency to job

[Luben Tuikov]

On 2023-02-08 14:48, Maíra Canal wrote:
> In order to add a syncobj's fence as a dependency to a job, it is
> necessary to call drm_syncobj_find_fence() to find the fence and then
> add the dependency with drm_sched_job_add_dependency(). So, wrap these
> steps in one single function, drm_sched_job_add_syncobj_dependency().

Yes, that's a good change--thanks!

Patch is: Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
-- 
Regards,
Luben

[PATCH 2/5] drm/lima: Use drm_sched_job_add_syncobj_dependency()

[Maíra Canal]

As lima_gem_add_deps() performs the same steps as
drm_sched_job_add_syncobj_dependency(), replace the open-coded
implementation in Lima in order to simply, using the DRM function.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/lima/lima_gem.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 0f1ca0b0db49..10252dc11a22 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -277,21 +277,13 @@ static int lima_gem_add_deps(struct drm_file *file, struct lima_submit *submit)
 	int i, err;
 
 	for (i = 0; i < ARRAY_SIZE(submit->in_sync); i++) {
-		struct dma_fence *fence = NULL;
-
 		if (!submit->in_sync[i])
 			continue;
 
-		err = drm_syncobj_find_fence(file, submit->in_sync[i],
-					     0, 0, &fence);
+		err = drm_sched_job_add_syncobj_dependency(&submit->task->base, file,
+							   submit->in_sync[i], 0);
 		if (err)
 			return err;
-
-		err = drm_sched_job_add_dependency(&submit->task->base, fence);
-		if (err) {
-			dma_fence_put(fence);
-			return err;
-		}
 	}
 
 	return 0;
-- 
2.39.1

[PATCH 3/5] drm/msm: Use drm_sched_job_add_syncobj_dependency()

[Maíra Canal]

As msm_parse_deps() performs the same steps as
drm_sched_job_add_syncobj_dependency(), replace the open-coded
implementation in msm in order to simply, using the DRM function.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 73a2ca122c57..622f1e27fcca 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -570,12 +570,8 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit,
 			break;
 		}
 
-		ret = drm_syncobj_find_fence(file, syncobj_desc.handle,
-		                             syncobj_desc.point, 0, &fence);
-		if (ret)
-			break;
-
-		ret = drm_sched_job_add_dependency(&submit->base, fence);
+		ret = drm_sched_job_add_syncobj_dependency(&submit->base, file,
+							   syncobj_desc.handle, syncobj_desc.point);
 		if (ret)
 			break;
 
-- 
2.39.1

[PATCH 4/5] drm/panfrost: Use drm_sched_job_add_syncobj_dependency()

[Maíra Canal]

As panfrost_copy_in_sync() performs the same steps as
drm_sched_job_add_syncobj_dependency(), replace the open-coded
implementation in Panfrost in order to simply, using the DRM function.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index abb0dadd8f63..f49096f53141 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -220,15 +220,8 @@ panfrost_copy_in_sync(struct drm_device *dev,
 	}
 
 	for (i = 0; i < in_fence_count; i++) {
-		struct dma_fence *fence;
-
-		ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0,
-					     &fence);
-		if (ret)
-			goto fail;
-
-		ret = drm_sched_job_add_dependency(&job->base, fence);
-
+		ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv,
+							   handles[i], 0);
 		if (ret)
 			goto fail;
 	}
-- 
2.39.1

Re: [PATCH 4/5] drm/panfrost: Use drm_sched_job_add_syncobj_dependency()

[Alyssa Rosenzweig]

R-b, thanks

On Wed, Feb 08, 2023 at 04:48:16PM -0300, Ma??ra Canal wrote:
> As panfrost_copy_in_sync() performs the same steps as
> drm_sched_job_add_syncobj_dependency(), replace the open-coded
> implementation in Panfrost in order to simply, using the DRM function.
> 
> Signed-off-by: Ma??ra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index abb0dadd8f63..f49096f53141 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -220,15 +220,8 @@ panfrost_copy_in_sync(struct drm_device *dev,
>  	}
>  
>  	for (i = 0; i < in_fence_count; i++) {
> -		struct dma_fence *fence;
> -
> -		ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0,
> -					     &fence);
> -		if (ret)
> -			goto fail;
> -
> -		ret = drm_sched_job_add_dependency(&job->base, fence);
> -
> +		ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv,
> +							   handles[i], 0);
>  		if (ret)
>  			goto fail;
>  	}
> -- 
> 2.39.1
> 

[PATCH 5/5] drm/v3d: Use drm_sched_job_add_syncobj_dependency()

[Maíra Canal]

As v3d_job_add_deps() performs the same steps as
drm_sched_job_add_syncobj_dependency(), replace the open-coded
implementation in v3d in order to simply, using the DRM function.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_gem.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 5da1806f3969..f149526ec971 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -400,14 +400,7 @@ static int
 v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
 		 u32 in_sync, u32 point)
 {
-	struct dma_fence *in_fence = NULL;
-	int ret;
-
-	ret = drm_syncobj_find_fence(file_priv, in_sync, point, 0, &in_fence);
-	if (ret == -EINVAL)
-		return ret;
-
-	return drm_sched_job_add_dependency(&job->base, in_fence);
+	return drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, point);
 }
 
 static int
-- 
2.39.1

Re: [PATCH 5/5] drm/v3d: Use drm_sched_job_add_syncobj_dependency()

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

On 02/08, Maíra Canal wrote:
> As v3d_job_add_deps() performs the same steps as
> drm_sched_job_add_syncobj_dependency(), replace the open-coded
> implementation in v3d in order to simply, using the DRM function.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/v3d/v3d_gem.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index 5da1806f3969..f149526ec971 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -400,14 +400,7 @@ static int
>  v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
>  		 u32 in_sync, u32 point)
>  {
> -	struct dma_fence *in_fence = NULL;
> -	int ret;
> -
> -	ret = drm_syncobj_find_fence(file_priv, in_sync, point, 0, &in_fence);
> -	if (ret == -EINVAL)
> -		return ret;
> -
> -	return drm_sched_job_add_dependency(&job->base, in_fence);
> +	return drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, point);

Hi Maíra,

First, thanks for making this function a common-code.

There are two issues to address before moving v3d to the new
drm_sche_job_add_syncobj_dependency():

1. We don't need the v3d_job_add_deps one line function just wrapping
the new drm_sched_job_add_syncobj_dependency() with the same parameters.
We can just replace all occurrences of v3d function with drm_sched
function. Except if we use v3d_job_add_deps to address the second issue:

2. Currently, v3d_job_add_deps returns 0 (success) if
drm_syncobj_find_fence() doesn't find the syncobj from handle (-ENOENT),
but drm_sched_job_add_syncobj_dependency() returns a negative value on
this case. If it happens, job submissions will fail (and may cause a
regression). One way to solve it is checking the return value of
drm_sched_job_add_syncobj_dependency in v3d_job_add_deps.  The second
way is to replace v3d_job_add_deps by
drm_sched_job_add_syncobj_dependency and check expected return values
there.

Melissa

>  }
>  
>  static int
> -- 
> 2.39.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] drm/v3d: Use drm_sched_job_add_syncobj_dependency()

[Christian König]

Am 09.02.23 um 12:27 schrieb Melissa Wen:
> On 02/08, Maíra Canal wrote:
>> As v3d_job_add_deps() performs the same steps as
>> drm_sched_job_add_syncobj_dependency(), replace the open-coded
>> implementation in v3d in order to simply, using the DRM function.
>>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   drivers/gpu/drm/v3d/v3d_gem.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
>> index 5da1806f3969..f149526ec971 100644
>> --- a/drivers/gpu/drm/v3d/v3d_gem.c
>> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>> @@ -400,14 +400,7 @@ static int
>>   v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
>>   		 u32 in_sync, u32 point)
>>   {
>> -	struct dma_fence *in_fence = NULL;
>> -	int ret;
>> -
>> -	ret = drm_syncobj_find_fence(file_priv, in_sync, point, 0, &in_fence);
>> -	if (ret == -EINVAL)
>> -		return ret;
>> -
>> -	return drm_sched_job_add_dependency(&job->base, in_fence);
>> +	return drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, point);
> Hi Maíra,
>
> First, thanks for making this function a common-code.
>
> There are two issues to address before moving v3d to the new
> drm_sche_job_add_syncobj_dependency():
>
> 1. We don't need the v3d_job_add_deps one line function just wrapping
> the new drm_sched_job_add_syncobj_dependency() with the same parameters.
> We can just replace all occurrences of v3d function with drm_sched
> function. Except if we use v3d_job_add_deps to address the second issue:
>
> 2. Currently, v3d_job_add_deps returns 0 (success) if
> drm_syncobj_find_fence() doesn't find the syncobj from handle (-ENOENT),
> but drm_sched_job_add_syncobj_dependency() returns a negative value on
> this case. If it happens, job submissions will fail (and may cause a
> regression). One way to solve it is checking the return value of
> drm_sched_job_add_syncobj_dependency in v3d_job_add_deps.  The second
> way is to replace v3d_job_add_deps by
> drm_sched_job_add_syncobj_dependency and check expected return values
> there.

Oh, wait a second. This behavior is most likely a bug in V3D.

When a syncobj can't find a timeline point aborting the IOCTL with 
-ENOENT is mandatory or otherwise you run into trouble with wait before 
signal handling for Vulkan.

This should be common behavior for all drivers which at some point want 
to support Vulkan.

Regards,
Christian.

>
> Melissa
>
>>   }
>>   
>>   static int
>> -- 
>> 2.39.1
>>

Re: [PATCH 5/5] drm/v3d: Use drm_sched_job_add_syncobj_dependency()

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 3442 bytes --]

On 02/09, Christian König wrote:
> Am 09.02.23 um 12:27 schrieb Melissa Wen:
> > On 02/08, Maíra Canal wrote:
> > > As v3d_job_add_deps() performs the same steps as
> > > drm_sched_job_add_syncobj_dependency(), replace the open-coded
> > > implementation in v3d in order to simply, using the DRM function.
> > > 
> > > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > > ---
> > >   drivers/gpu/drm/v3d/v3d_gem.c | 9 +--------
> > >   1 file changed, 1 insertion(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> > > index 5da1806f3969..f149526ec971 100644
> > > --- a/drivers/gpu/drm/v3d/v3d_gem.c
> > > +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> > > @@ -400,14 +400,7 @@ static int
> > >   v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
> > >   		 u32 in_sync, u32 point)
> > >   {
> > > -	struct dma_fence *in_fence = NULL;
> > > -	int ret;
> > > -
> > > -	ret = drm_syncobj_find_fence(file_priv, in_sync, point, 0, &in_fence);
> > > -	if (ret == -EINVAL)
> > > -		return ret;
> > > -
> > > -	return drm_sched_job_add_dependency(&job->base, in_fence);
> > > +	return drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, point);
> > Hi Maíra,
> > 
> > First, thanks for making this function a common-code.
> > 
> > There are two issues to address before moving v3d to the new
> > drm_sche_job_add_syncobj_dependency():
> > 
> > 1. We don't need the v3d_job_add_deps one line function just wrapping
> > the new drm_sched_job_add_syncobj_dependency() with the same parameters.
> > We can just replace all occurrences of v3d function with drm_sched
> > function. Except if we use v3d_job_add_deps to address the second issue:
> > 
> > 2. Currently, v3d_job_add_deps returns 0 (success) if
> > drm_syncobj_find_fence() doesn't find the syncobj from handle (-ENOENT),
> > but drm_sched_job_add_syncobj_dependency() returns a negative value on
> > this case. If it happens, job submissions will fail (and may cause a
> > regression). One way to solve it is checking the return value of
> > drm_sched_job_add_syncobj_dependency in v3d_job_add_deps.  The second
> > way is to replace v3d_job_add_deps by
> > drm_sched_job_add_syncobj_dependency and check expected return values
> > there.
> 
> Oh, wait a second. This behavior is most likely a bug in V3D.
> 
> When a syncobj can't find a timeline point aborting the IOCTL with -ENOENT
> is mandatory or otherwise you run into trouble with wait before signal
> handling for Vulkan.
> 
> This should be common behavior for all drivers which at some point want to
> support Vulkan.

So, v3d doesn't support timeline syncobj yet, and I took a look at
returning errors on drm_syncobj_find_fence, for timeline syncobj they
can be `-ETIME` and `-ERESTARTSYS`.

TBH, I don't exactly know the design reason for accepting a ENOENT from
drm_syncobj_find_fence. I suspect it's only trying to deal with the
`in_sync == 0` case (that would be better replaced by a `if then;
continue;`).

In any case, I think it isn't good to change this behavior in the same
time we are moving to another function. I'd prefer to preserve the
current behavior here and better investigate/address this issue in
another patch.

Melissa

> 
> Regards,
> Christian.
> 
> > 
> > Melissa
> > 
> > >   }
> > >   static int
> > > -- 
> > > 2.39.1
> > > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] drm/v3d: Use drm_sched_job_add_syncobj_dependency()

[Christian König]

Am 09.02.23 um 13:36 schrieb Melissa Wen:
> On 02/09, Christian König wrote:
>> Am 09.02.23 um 12:27 schrieb Melissa Wen:
>>> On 02/08, Maíra Canal wrote:
>>>> As v3d_job_add_deps() performs the same steps as
>>>> drm_sched_job_add_syncobj_dependency(), replace the open-coded
>>>> implementation in v3d in order to simply, using the DRM function.
>>>>
>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>> ---
>>>>    drivers/gpu/drm/v3d/v3d_gem.c | 9 +--------
>>>>    1 file changed, 1 insertion(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
>>>> index 5da1806f3969..f149526ec971 100644
>>>> --- a/drivers/gpu/drm/v3d/v3d_gem.c
>>>> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>>>> @@ -400,14 +400,7 @@ static int
>>>>    v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
>>>>    		 u32 in_sync, u32 point)
>>>>    {
>>>> -	struct dma_fence *in_fence = NULL;
>>>> -	int ret;
>>>> -
>>>> -	ret = drm_syncobj_find_fence(file_priv, in_sync, point, 0, &in_fence);
>>>> -	if (ret == -EINVAL)
>>>> -		return ret;
>>>> -
>>>> -	return drm_sched_job_add_dependency(&job->base, in_fence);
>>>> +	return drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, point);
>>> Hi Maíra,
>>>
>>> First, thanks for making this function a common-code.
>>>
>>> There are two issues to address before moving v3d to the new
>>> drm_sche_job_add_syncobj_dependency():
>>>
>>> 1. We don't need the v3d_job_add_deps one line function just wrapping
>>> the new drm_sched_job_add_syncobj_dependency() with the same parameters.
>>> We can just replace all occurrences of v3d function with drm_sched
>>> function. Except if we use v3d_job_add_deps to address the second issue:
>>>
>>> 2. Currently, v3d_job_add_deps returns 0 (success) if
>>> drm_syncobj_find_fence() doesn't find the syncobj from handle (-ENOENT),
>>> but drm_sched_job_add_syncobj_dependency() returns a negative value on
>>> this case. If it happens, job submissions will fail (and may cause a
>>> regression). One way to solve it is checking the return value of
>>> drm_sched_job_add_syncobj_dependency in v3d_job_add_deps.  The second
>>> way is to replace v3d_job_add_deps by
>>> drm_sched_job_add_syncobj_dependency and check expected return values
>>> there.
>> Oh, wait a second. This behavior is most likely a bug in V3D.
>>
>> When a syncobj can't find a timeline point aborting the IOCTL with -ENOENT
>> is mandatory or otherwise you run into trouble with wait before signal
>> handling for Vulkan.
>>
>> This should be common behavior for all drivers which at some point want to
>> support Vulkan.
> So, v3d doesn't support timeline syncobj yet, and I took a look at
> returning errors on drm_syncobj_find_fence, for timeline syncobj they
> can be `-ETIME` and `-ERESTARTSYS`.

In this case forget what I've said. This just doesn't apply to V3D for now.

> TBH, I don't exactly know the design reason for accepting a ENOENT from
> drm_syncobj_find_fence. I suspect it's only trying to deal with the
> `in_sync == 0` case (that would be better replaced by a `if then;
> continue;`).
>
> In any case, I think it isn't good to change this behavior in the same
> time we are moving to another function. I'd prefer to preserve the
> current behavior here and better investigate/address this issue in
> another patch.

I would just keep the current behavior with an if and add something like 
"TODO: Investigate why this was filtered out for the IOCTL".

Christian.

>
> Melissa
>
>> Regards,
>> Christian.
>>
>>> Melissa
>>>
>>>>    }
>>>>    static int
>>>> -- 
>>>> 2.39.1
>>>>