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

[PATCH v2 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().

v1 -> v2: https://lore.kernel.org/dri-devel/20230208194817.199932-1-mcanal@igalia.com/T/

- Don't initialize any local return variables if it isn't necessary (Christian König).
- Remove unused "fence" variable from msm.
- Handle the -ENOENT case in v3d (Melissa Wen).
- Add Christian's, Alyssa's, and Luben's Reviewed-by.

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    |  9 ++------
 drivers/gpu/drm/panfrost/panfrost_drv.c | 11 ++--------
 drivers/gpu/drm/scheduler/sched_main.c  | 29 +++++++++++++++++++++++++
 drivers/gpu/drm/v3d/v3d_gem.c           | 22 ++++---------------
 include/drm/gpu_scheduler.h             |  6 +++++
 6 files changed, 45 insertions(+), 44 deletions(-)

-- 
2.39.1

[PATCH v2 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().

Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
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..9b16480686f6 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;
+
+	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

[PATCH v2 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 use 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 v2 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 use the DRM function.

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

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 73a2ca122c57..d360277809f2 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -550,7 +550,6 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit,
 
 	for (i = 0; i < nr_in_syncobjs; ++i) {
 		uint64_t address = in_syncobjs_addr + i * syncobj_stride;
-		struct dma_fence *fence;
 
 		if (copy_from_user(&syncobj_desc,
 			           u64_to_user_ptr(address),
@@ -570,12 +569,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 v2 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 use the DRM function.

Reviewed-by: Alyssa Rosenzweig <alyssa@collabora.com>
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 v2 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 use the DRM function.

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

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 5da1806f3969..24cc65ffb4df 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -396,20 +396,6 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data,
 	return ret;
 }
 
-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);
-}
-
 static int
 v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 	     void **container, size_t size, void (*free)(struct kref *ref),
@@ -447,14 +433,14 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 					DRM_DEBUG("Failed to copy wait dep handle.\n");
 					goto fail_deps;
 				}
-				ret = v3d_job_add_deps(file_priv, job, in.handle, 0);
-				if (ret)
+				ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in.handle, 0);
+				if (ret && ret != -ENOENT)
 					goto fail_deps;
 			}
 		}
 	} else {
-		ret = v3d_job_add_deps(file_priv, job, in_sync, 0);
-		if (ret)
+		ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, 0);
+		if (ret && ret != -ENOENT)
 			goto fail_deps;
 	}
 
-- 
2.39.1

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

[Melissa Wen]

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

On 02/09, 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 use the DRM function.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/v3d/v3d_gem.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index 5da1806f3969..24cc65ffb4df 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -396,20 +396,6 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data,
>  	return ret;
>  }
>  
> -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);
> -}
> -
>  static int
>  v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
>  	     void **container, size_t size, void (*free)(struct kref *ref),
> @@ -447,14 +433,14 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
>  					DRM_DEBUG("Failed to copy wait dep handle.\n");
>  					goto fail_deps;
>  				}
> -				ret = v3d_job_add_deps(file_priv, job, in.handle, 0);
> -				if (ret)
> +				ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in.handle, 0);
> +				if (ret && ret != -ENOENT)
>  					goto fail_deps;
>  			}
>  		}
>  	} else {
> -		ret = v3d_job_add_deps(file_priv, job, in_sync, 0);
> -		if (ret)
> +		ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, 0);
> +		if (ret && ret != -ENOENT)
>  			goto fail_deps;
Hi Maíra,

LGTM. Can you add the comment suggested by Christian?

With that, for v3d:

Reviewed-by: Melissa Wen <mwen@igalia.com>

Thanks,

Melissa

>  	}
>  
> -- 
> 2.39.1
> 

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

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

[Steven Price]

On 09/02/2023 12:44, 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 use the DRM function.
> 
> Reviewed-by: Alyssa Rosenzweig <alyssa@collabora.com>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Reviewed-by: Steven Price <steven.price@arm.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;
>  	}

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

[Maíra Canal]

On 2/9/23 09:44, 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().
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
> 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(+)
> 

Applied to drm-misc-next.

Best Regards,
- Maíra Canal

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

[Maíra Canal]

On 2/9/23 09:44, 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 use the DRM function.
> 
> Reviewed-by: Alyssa Rosenzweig <alyssa@collabora.com>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_drv.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 

Applied to drm-misc-next.

Best Regards,
- Maíra Canal

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

[Maíra Canal]

On 2/9/23 09:44, 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 use the DRM function.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   drivers/gpu/drm/v3d/v3d_gem.c | 22 ++++------------------
>   1 file changed, 4 insertions(+), 18 deletions(-)
> 

Applied to drm-misc-next.

Best Regards,
- Maíra Canal