[PATCH v3 0/4] drm/v3d: add multiple in/out syncobjs support

[PATCH v3 0/4] drm/v3d: add multiple in/out syncobjs support

[Melissa Wen]

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

Currently, v3d only supports single in/out syncobj per submission (in
v3d_submit_cl, there are two in_sync, one for bin and another for render
job); however, Vulkan queue submit operations expect multiples wait and
signal semaphores. This series extends v3d interface and job dependency
operations to handle more than one in/out syncobj.

The first patch just decouples the steps to lookup and add job
dependency from the job init code, since the operation repeats for every
syncobj that a job should wait before starting. So, the fourth patch of
this series will reuse it to handle multiples wait for semaphores.

The second patch moves job memory allocation to v3d_job_init() for any
type of v3d_job-based job. The main goal is to prevent errors when
handling job initialization failures (doing a proper cleanup). 

The third patch extends our interface by using a generic extension.
This approach was inspired by i915_user_extension[1] and
amd_cs_chunks[2] to give a little more flexibility in adding other
submission features in the future. Therefore, the list of extensions
would work as a hub of features that use an id to determine the
corresponding feature data type.

With this base, the fourth patch adds multiple wait/signal semaphores
support. For this, we add to the list of the generic extensions a new
data type (drm_v3d_multi_sync) that points to two arrays of syncobjs
(in/out) and also determines (wait_stage) the job to add wait
dependencies (v3d_queue). An auxiliary struct (v3d_submit_ext) is used
when parsing submission extensions. Finally, we reserve some space in
the semaphore struct (drm_v3d_sem) to accommodate timeline semaphores
that we aim to add support soon (same reason for already defining
v3d_submit_outsync).

[1] https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/i915/i915_user_extensions.c?id=9d1305ef80b95dde0337106ed8b826604e2155ad
[2] https://cgit.freedesktop.org/drm/drm-misc/tree/include/uapi/drm/amdgpu_drm.h#n556

In the mesa side, the work related to this series is in progress at
https://gitlab.freedesktop.org/mwen/mesa/-/commit/6c340bb35203a0418af87a7921d7295a4047b77f

v2:
- move job mem alloc to v3d_job_init (Iago)
- simplify and subclass the generic extension struct (Daniel)
- make job dependency conditions more understandable (Iago)

v3 (Iago):
- job should be initialized NULL
- fix error handling for pm ops in v3d_job_init
- remove irrelevant comment
- fix multisync conditions to multiple vs single sempahores

Melissa Wen (4):
  drm/v3d: decouple adding job dependencies steps from job init
  drm/v3d: alloc and init job in one shot
  drm/v3d: add generic ioctl extension
  drm/v3d: add multiple syncobjs support

 drivers/gpu/drm/v3d/v3d_drv.c |  10 +-
 drivers/gpu/drm/v3d/v3d_drv.h |  24 ++-
 drivers/gpu/drm/v3d/v3d_gem.c | 390 +++++++++++++++++++++++++---------
 include/uapi/drm/v3d_drm.h    |  78 +++++++
 4 files changed, 389 insertions(+), 113 deletions(-)

-- 
2.30.2


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

[PATCH v3 1/4] drm/v3d: decouple adding job dependencies steps from job init

[Melissa Wen]

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

Prep work to enable a job to wait for more than one syncobj before
start. Also get rid of old checkpatch warnings in the v3d_gem file.
No functional changes.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_gem.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index ead0be8d48a7..e60fbc28ef29 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -416,7 +416,7 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 
 	ret = drm_gem_dma_resv_wait(file_priv, args->handle,
-					      true, timeout_jiffies);
+				    true, timeout_jiffies);
 
 	/* Decrement the user's timeout, in case we got interrupted
 	 * such that the ioctl will be restarted.
@@ -434,12 +434,25 @@ 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,
 	     struct v3d_job *job, void (*free)(struct kref *ref),
 	     u32 in_sync, enum v3d_queue queue)
 {
-	struct dma_fence *in_fence = NULL;
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
 	int ret;
 
@@ -455,11 +468,7 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 	if (ret)
 		goto fail;
 
-	ret = drm_syncobj_find_fence(file_priv, in_sync, 0, 0, &in_fence);
-	if (ret == -EINVAL)
-		goto fail_job;
-
-	ret = drm_sched_job_add_dependency(&job->base, in_fence);
+	ret = v3d_job_add_deps(file_priv, job, in_sync, 0);
 	if (ret)
 		goto fail_job;
 
@@ -499,7 +508,7 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 	for (i = 0; i < job->bo_count; i++) {
 		/* XXX: Use shared fences for read-only objects. */
 		dma_resv_add_excl_fence(job->bo[i]->resv,
-						  job->done_fence);
+					job->done_fence);
 	}
 
 	drm_gem_unlock_reservations(job->bo, job->bo_count, acquire_ctx);
@@ -903,8 +912,7 @@ v3d_gem_init(struct drm_device *dev)
 	if (!v3d->pt) {
 		drm_mm_takedown(&v3d->mm);
 		dev_err(v3d->drm.dev,
-			"Failed to allocate page tables. "
-			"Please ensure you have CMA enabled.\n");
+			"Failed to allocate page tables. Please ensure you have CMA enabled.\n");
 		return -ENOMEM;
 	}
 
-- 
2.30.2


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

[PATCH v3 2/4] drm/v3d: alloc and init job in one shot

[Melissa Wen]

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

Move job memory allocation to v3d_job_init function. This aim to facilitate
error handling in job initialization, since cleanup steps are similar for all
(struct v3d_job)-based types of job involved in a command submission. To
generalize v3d_job_init(), this change takes into account that all job structs
have the first element a struct v3d_job (bin, render, tfu, csd) or it is a
v3d_job itself (clean_job) for pointer casting.

v3:
- explicitly init job as NULL (Iago)
- fix pm failure handling on v3_job_init (Iago)

Suggested-by: Iago Toral <itoral@igalia.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_gem.c | 135 +++++++++++++---------------------
 1 file changed, 53 insertions(+), 82 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index e60fbc28ef29..f932e73fb5c6 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -392,6 +392,9 @@ v3d_render_job_free(struct kref *ref)
 
 void v3d_job_cleanup(struct v3d_job *job)
 {
+	if (!job)
+		return;
+
 	drm_sched_job_cleanup(&job->base);
 	v3d_job_put(job);
 }
@@ -450,35 +453,48 @@ v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
 
 static int
 v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
-	     struct v3d_job *job, void (*free)(struct kref *ref),
+	     void **container, size_t size, void (*free)(struct kref *ref),
 	     u32 in_sync, enum v3d_queue queue)
 {
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
+	struct v3d_job *job;
 	int ret;
 
+	*container = kcalloc(1, size, GFP_KERNEL);
+	if (!*container) {
+		DRM_ERROR("Cannot allocate memory for v3d job.");
+		return -ENOMEM;
+	}
+
+	job = *container;
 	job->v3d = v3d;
 	job->free = free;
 
 	ret = pm_runtime_get_sync(v3d->drm.dev);
 	if (ret < 0)
-		return ret;
+		goto fail;
 
 	ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue],
 				 v3d_priv);
 	if (ret)
-		goto fail;
+		goto fail_job;
 
 	ret = v3d_job_add_deps(file_priv, job, in_sync, 0);
 	if (ret)
-		goto fail_job;
+		goto fail_deps;
 
 	kref_init(&job->refcount);
 
 	return 0;
-fail_job:
+
+fail_deps:
 	drm_sched_job_cleanup(&job->base);
-fail:
+fail_job:
 	pm_runtime_put_autosuspend(v3d->drm.dev);
+fail:
+	kfree(*container);
+	*container = NULL;
+
 	return ret;
 }
 
@@ -541,7 +557,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
 	struct drm_v3d_submit_cl *args = data;
 	struct v3d_bin_job *bin = NULL;
-	struct v3d_render_job *render;
+	struct v3d_render_job *render = NULL;
 	struct v3d_job *clean_job = NULL;
 	struct v3d_job *last_job;
 	struct ww_acquire_ctx acquire_ctx;
@@ -558,35 +574,20 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	render = kcalloc(1, sizeof(*render), GFP_KERNEL);
-	if (!render)
-		return -ENOMEM;
+	ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render),
+			   v3d_render_job_free, args->in_sync_rcl, V3D_RENDER);
+	if (ret)
+		goto fail;
 
 	render->start = args->rcl_start;
 	render->end = args->rcl_end;
 	INIT_LIST_HEAD(&render->unref_list);
 
-	ret = v3d_job_init(v3d, file_priv, &render->base,
-			   v3d_render_job_free, args->in_sync_rcl, V3D_RENDER);
-	if (ret) {
-		kfree(render);
-		return ret;
-	}
-
 	if (args->bcl_start != args->bcl_end) {
-		bin = kcalloc(1, sizeof(*bin), GFP_KERNEL);
-		if (!bin) {
-			v3d_job_cleanup(&render->base);
-			return -ENOMEM;
-		}
-
-		ret = v3d_job_init(v3d, file_priv, &bin->base,
+		ret = v3d_job_init(v3d, file_priv, (void *)&bin, sizeof(*bin),
 				   v3d_job_free, args->in_sync_bcl, V3D_BIN);
-		if (ret) {
-			v3d_job_cleanup(&render->base);
-			kfree(bin);
-			return ret;
-		}
+		if (ret)
+			goto fail;
 
 		bin->start = args->bcl_start;
 		bin->end = args->bcl_end;
@@ -597,18 +598,10 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
-		clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL);
-		if (!clean_job) {
-			ret = -ENOMEM;
-			goto fail;
-		}
-
-		ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0, V3D_CACHE_CLEAN);
-		if (ret) {
-			kfree(clean_job);
-			clean_job = NULL;
+		ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job),
+				   v3d_job_free, 0, V3D_CACHE_CLEAN);
+		if (ret)
 			goto fail;
-		}
 
 		last_job = clean_job;
 	} else {
@@ -681,11 +674,9 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	drm_gem_unlock_reservations(last_job->bo,
 				    last_job->bo_count, &acquire_ctx);
 fail:
-	if (bin)
-		v3d_job_cleanup(&bin->base);
-	v3d_job_cleanup(&render->base);
-	if (clean_job)
-		v3d_job_cleanup(clean_job);
+	v3d_job_cleanup((void *)bin);
+	v3d_job_cleanup((void *)render);
+	v3d_job_cleanup(clean_job);
 
 	return ret;
 }
@@ -705,28 +696,22 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 {
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 	struct drm_v3d_submit_tfu *args = data;
-	struct v3d_tfu_job *job;
+	struct v3d_tfu_job *job = NULL;
 	struct ww_acquire_ctx acquire_ctx;
 	int ret = 0;
 
 	trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);
 
-	job = kcalloc(1, sizeof(*job), GFP_KERNEL);
-	if (!job)
-		return -ENOMEM;
-
-	ret = v3d_job_init(v3d, file_priv, &job->base,
+	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
 			   v3d_job_free, args->in_sync, V3D_TFU);
-	if (ret) {
-		kfree(job);
-		return ret;
-	}
+	if (ret)
+		goto fail;
 
 	job->base.bo = kcalloc(ARRAY_SIZE(args->bo_handles),
 			       sizeof(*job->base.bo), GFP_KERNEL);
 	if (!job->base.bo) {
-		v3d_job_cleanup(&job->base);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto fail;
 	}
 
 	job->args = *args;
@@ -773,7 +758,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	return 0;
 
 fail:
-	v3d_job_cleanup(&job->base);
+	v3d_job_cleanup((void *)job);
 
 	return ret;
 }
@@ -794,8 +779,8 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
 	struct drm_v3d_submit_csd *args = data;
-	struct v3d_csd_job *job;
-	struct v3d_job *clean_job;
+	struct v3d_csd_job *job = NULL;
+	struct v3d_job *clean_job = NULL;
 	struct ww_acquire_ctx acquire_ctx;
 	int ret;
 
@@ -806,29 +791,15 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	job = kcalloc(1, sizeof(*job), GFP_KERNEL);
-	if (!job)
-		return -ENOMEM;
-
-	ret = v3d_job_init(v3d, file_priv, &job->base,
+	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
 			   v3d_job_free, args->in_sync, V3D_CSD);
-	if (ret) {
-		kfree(job);
-		return ret;
-	}
-
-	clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL);
-	if (!clean_job) {
-		v3d_job_cleanup(&job->base);
-		return -ENOMEM;
-	}
+	if (ret)
+		goto fail;
 
-	ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0, V3D_CACHE_CLEAN);
-	if (ret) {
-		v3d_job_cleanup(&job->base);
-		kfree(clean_job);
-		return ret;
-	}
+	ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job),
+			   v3d_job_free, 0, V3D_CACHE_CLEAN);
+	if (ret)
+		goto fail;
 
 	job->args = *args;
 
@@ -877,7 +848,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	drm_gem_unlock_reservations(clean_job->bo, clean_job->bo_count,
 				    &acquire_ctx);
 fail:
-	v3d_job_cleanup(&job->base);
+	v3d_job_cleanup((void *)job);
 	v3d_job_cleanup(clean_job);
 
 	return ret;
-- 
2.30.2


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

[PATCH v3 3/4] drm/v3d: add generic ioctl extension

[Melissa Wen]

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

Add support to attach generic extensions on job submission. This patch
is third prep work to enable multiple syncobjs on job submission. With
this work, when the job submission interface needs to be extended to
accomodate a new feature, we will use a generic extension struct where
an id determines the data type to be pointed. The first application is
to enable multiples in/out syncobj (next patch), but the base is
already done for future features. Therefore, to attach a new feature,
a specific extension struct should subclass drm_v3d_extension and
update the list of extensions in a job submission.

v2:
- remove redundant elements to subclass struct (Daniel)

v3:
- add comment for v3d_get_extensions

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.c |  4 +-
 drivers/gpu/drm/v3d/v3d_gem.c | 74 +++++++++++++++++++++++++++++++++--
 include/uapi/drm/v3d_drm.h    | 31 +++++++++++++++
 3 files changed, 103 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index c1deab2cf38d..3d6b9bcce2f7 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -83,7 +83,6 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data,
 		return 0;
 	}
 
-
 	switch (args->param) {
 	case DRM_V3D_PARAM_SUPPORTS_TFU:
 		args->value = 1;
@@ -147,7 +146,7 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file)
 DEFINE_DRM_GEM_FOPS(v3d_drm_fops);
 
 /* DRM_AUTH is required on SUBMIT_CL for now, while we don't have GMP
- * protection between clients.  Note that render nodes would be be
+ * protection between clients.  Note that render nodes would be
  * able to submit CLs that could access BOs from clients authenticated
  * with the master node.  The TFU doesn't use the GMP, so it would
  * need to stay DRM_AUTH until we do buffer size/offset validation.
@@ -219,7 +218,6 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	u32 mmu_debug;
 	u32 ident1;
 
-
 	v3d = devm_drm_dev_alloc(dev, &v3d_drm_driver, struct v3d_dev, drm);
 	if (IS_ERR(v3d))
 		return PTR_ERR(v3d);
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index f932e73fb5c6..93f130fb3a13 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -537,6 +537,36 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 	}
 }
 
+/* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data
+ * according to the extension id (name).
+ */
+static int
+v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles)
+{
+	struct drm_v3d_extension __user *user_ext;
+
+	user_ext = u64_to_user_ptr(ext_handles);
+	while (user_ext) {
+		struct drm_v3d_extension ext;
+
+		if (copy_from_user(&ext, user_ext, sizeof(ext))) {
+			DRM_DEBUG("Failed to copy submit extension\n");
+			return -EFAULT;
+		}
+
+		switch (ext.id) {
+		case 0:
+		default:
+			DRM_DEBUG_DRIVER("Unknown extension id: %d\n", ext.id);
+			return -EINVAL;
+		}
+
+		user_ext = u64_to_user_ptr(ext.next);
+	}
+
+	return 0;
+}
+
 /**
  * v3d_submit_cl_ioctl() - Submits a job (frame) to the V3D.
  * @dev: DRM device
@@ -565,15 +595,24 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 
 	trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args->rcl_end);
 
-	if (args->pad != 0)
+	if (args->pad)
 		return -EINVAL;
 
-	if (args->flags != 0 &&
-	    args->flags != DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
+	if (args->flags &&
+	    args->flags & ~(DRM_V3D_SUBMIT_CL_FLUSH_CACHE |
+			    DRM_V3D_SUBMIT_EXTENSION)) {
 		DRM_INFO("invalid flags: %d\n", args->flags);
 		return -EINVAL;
 	}
 
+	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
+		ret = v3d_get_extensions(file_priv, args->extensions);
+		if (ret) {
+			DRM_DEBUG("Failed to get extensions.\n");
+			return ret;
+		}
+	}
+
 	ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render),
 			   v3d_render_job_free, args->in_sync_rcl, V3D_RENDER);
 	if (ret)
@@ -702,6 +741,19 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 
 	trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);
 
+	if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {
+		DRM_DEBUG("invalid flags: %d\n", args->flags);
+		return -EINVAL;
+	}
+
+	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
+		ret = v3d_get_extensions(file_priv, args->extensions);
+		if (ret) {
+			DRM_DEBUG("Failed to get extensions.\n");
+			return ret;
+		}
+	}
+
 	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
 			   v3d_job_free, args->in_sync, V3D_TFU);
 	if (ret)
@@ -786,11 +838,27 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 
 	trace_v3d_submit_csd_ioctl(&v3d->drm, args->cfg[5], args->cfg[6]);
 
+	if (args->pad)
+		return -EINVAL;
+
 	if (!v3d_has_csd(v3d)) {
 		DRM_DEBUG("Attempting CSD submit on non-CSD hardware\n");
 		return -EINVAL;
 	}
 
+	if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {
+		DRM_INFO("invalid flags: %d\n", args->flags);
+		return -EINVAL;
+	}
+
+	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
+		ret = v3d_get_extensions(file_priv, args->extensions);
+		if (ret) {
+			DRM_DEBUG("Failed to get extensions.\n");
+			return ret;
+		}
+	}
+
 	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
 			   v3d_job_free, args->in_sync, V3D_CSD);
 	if (ret)
diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h
index 4104f22fb3d3..55b443ca6c0b 100644
--- a/include/uapi/drm/v3d_drm.h
+++ b/include/uapi/drm/v3d_drm.h
@@ -58,6 +58,20 @@ extern "C" {
 						   struct drm_v3d_perfmon_get_values)
 
 #define DRM_V3D_SUBMIT_CL_FLUSH_CACHE             0x01
+#define DRM_V3D_SUBMIT_EXTENSION		  0x02
+
+/* struct drm_v3d_extension - ioctl extensions
+ *
+ * Linked-list of generic extensions where the id identify which struct is
+ * pointed by ext_data. Therefore, DRM_V3D_EXT_ID_* is used on id to identify
+ * the extension type.
+ */
+struct drm_v3d_extension {
+	__u64 next;
+	__u32 id;
+#define DRM_V3D_EXT_ID_MULTI_SYNC		0x01
+	__u32 flags; /* mbz */
+};
 
 /**
  * struct drm_v3d_submit_cl - ioctl argument for submitting commands to the 3D
@@ -135,12 +149,16 @@ struct drm_v3d_submit_cl {
 	/* Number of BO handles passed in (size is that times 4). */
 	__u32 bo_handle_count;
 
+	/* DRM_V3D_SUBMIT_* properties */
 	__u32 flags;
 
 	/* ID of the perfmon to attach to this job. 0 means no perfmon. */
 	__u32 perfmon_id;
 
 	__u32 pad;
+
+	/* Pointer to an array of ioctl extensions*/
+	__u64 extensions;
 };
 
 /**
@@ -248,6 +266,12 @@ struct drm_v3d_submit_tfu {
 	__u32 in_sync;
 	/* Sync object to signal when the TFU job is done. */
 	__u32 out_sync;
+
+	__u32 flags;
+
+	/* Pointer to an array of ioctl extensions*/
+	__u64 extensions;
+
 };
 
 /* Submits a compute shader for dispatch.  This job will block on any
@@ -276,6 +300,13 @@ struct drm_v3d_submit_csd {
 
 	/* ID of the perfmon to attach to this job. 0 means no perfmon. */
 	__u32 perfmon_id;
+
+	/* Pointer to an array of ioctl extensions*/
+	__u64 extensions;
+
+	__u32 flags;
+
+	__u32 pad;
 };
 
 enum {
-- 
2.30.2


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

[PATCH v3 4/4] drm/v3d: add multiple syncobjs support

[Melissa Wen]

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

Using the generic extension from the previous patch, a specific multisync
extension enables more than one in/out binary syncobj per job submission.
Arrays of syncobjs are set in struct drm_v3d_multisync, that also cares
of determining the stage for sync (wait deps) according to the job
queue.

v2:
- subclass the generic extension struct (Daniel)
- simplify adding dependency conditions to make understandable (Iago)

v3:
- fix conditions to consider single or multiples in/out_syncs (Iago)
- remove irrelevant comment (Iago)

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.c |   6 +-
 drivers/gpu/drm/v3d/v3d_drv.h |  24 +++--
 drivers/gpu/drm/v3d/v3d_gem.c | 185 ++++++++++++++++++++++++++++++----
 include/uapi/drm/v3d_drm.h    |  49 ++++++++-
 4 files changed, 232 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 3d6b9bcce2f7..bd46396a1ae0 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data,
 	case DRM_V3D_PARAM_SUPPORTS_PERFMON:
 		args->value = (v3d->ver >= 40);
 		return 0;
+	case DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT:
+		args->value = 1;
+		return 0;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", args->param);
 		return -EINVAL;
@@ -135,9 +138,8 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file)
 	struct v3d_file_priv *v3d_priv = file->driver_priv;
 	enum v3d_queue q;
 
-	for (q = 0; q < V3D_MAX_QUEUES; q++) {
+	for (q = 0; q < V3D_MAX_QUEUES; q++)
 		drm_sched_entity_destroy(&v3d_priv->sched_entity[q]);
-	}
 
 	v3d_perfmon_close_file(v3d_priv);
 	kfree(v3d_priv);
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index b900a050d5e2..b74b1351bfc8 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -19,15 +19,6 @@ struct reset_control;
 
 #define GMP_GRANULARITY (128 * 1024)
 
-/* Enum for each of the V3D queues. */
-enum v3d_queue {
-	V3D_BIN,
-	V3D_RENDER,
-	V3D_TFU,
-	V3D_CSD,
-	V3D_CACHE_CLEAN,
-};
-
 #define V3D_MAX_QUEUES (V3D_CACHE_CLEAN + 1)
 
 struct v3d_queue_state {
@@ -294,6 +285,21 @@ struct v3d_csd_job {
 	struct drm_v3d_submit_csd args;
 };
 
+struct v3d_submit_outsync {
+	struct drm_syncobj *syncobj;
+};
+
+struct v3d_submit_ext {
+	u32 flags;
+	u32 wait_stage;
+
+	u32 in_sync_count;
+	u64 in_syncs;
+
+	u32 out_sync_count;
+	struct v3d_submit_outsync *out_syncs;
+};
+
 /**
  * __wait_for - magic wait macro
  *
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 93f130fb3a13..d344eae36b57 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -454,11 +454,12 @@ v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
 static int
 v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 	     void **container, size_t size, void (*free)(struct kref *ref),
-	     u32 in_sync, enum v3d_queue queue)
+	     u32 in_sync, struct v3d_submit_ext *se, enum v3d_queue queue)
 {
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
 	struct v3d_job *job;
-	int ret;
+	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
+	int ret, i;
 
 	*container = kcalloc(1, size, GFP_KERNEL);
 	if (!*container) {
@@ -479,9 +480,28 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 	if (ret)
 		goto fail_job;
 
-	ret = v3d_job_add_deps(file_priv, job, in_sync, 0);
-	if (ret)
-		goto fail_deps;
+	if (has_multisync) {
+		if (se->in_sync_count && se->wait_stage == queue) {
+			struct drm_v3d_sem __user *handle = u64_to_user_ptr(se->in_syncs);
+
+			for (i = 0; i < se->in_sync_count; i++) {
+				struct drm_v3d_sem in;
+
+				ret = copy_from_user(&in, handle++, sizeof(in));
+				if (ret) {
+					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)
+					goto fail_deps;
+			}
+		}
+	} else {
+		ret = v3d_job_add_deps(file_priv, job, in_sync, 0);
+		if (ret)
+			goto fail_deps;
+	}
 
 	kref_init(&job->refcount);
 
@@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 					 struct v3d_job *job,
 					 struct ww_acquire_ctx *acquire_ctx,
 					 u32 out_sync,
+					 struct v3d_submit_ext *se,
 					 struct dma_fence *done_fence)
 {
 	struct drm_syncobj *sync_out;
+	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
 	int i;
 
 	for (i = 0; i < job->bo_count; i++) {
@@ -530,20 +552,130 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 	drm_gem_unlock_reservations(job->bo, job->bo_count, acquire_ctx);
 
 	/* Update the return sync object for the job */
-	sync_out = drm_syncobj_find(file_priv, out_sync);
-	if (sync_out) {
-		drm_syncobj_replace_fence(sync_out, done_fence);
-		drm_syncobj_put(sync_out);
+	/* If it only supports a single signal semaphore*/
+	if (!has_multisync) {
+		sync_out = drm_syncobj_find(file_priv, out_sync);
+		if (sync_out) {
+			drm_syncobj_replace_fence(sync_out, done_fence);
+			drm_syncobj_put(sync_out);
+		}
+		return;
+	}
+
+	/* If multiple semaphores extension is supported */
+	if (se->out_sync_count) {
+		for (i = 0; i < se->out_sync_count; i++) {
+			drm_syncobj_replace_fence(se->out_syncs[i].syncobj,
+						  done_fence);
+			drm_syncobj_put(se->out_syncs[i].syncobj);
+		}
+		kvfree(se->out_syncs);
+	}
+}
+
+static void
+v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
+{
+	unsigned int i;
+
+	if (!se->out_sync_count)
+		return;
+
+	for (i = 0; i < se->out_sync_count; i++)
+		drm_syncobj_put(se->out_syncs[i].syncobj);
+	kvfree(se->out_syncs);
+}
+
+static int
+v3d_get_multisync_post_deps(struct drm_file *file_priv,
+			    struct v3d_submit_ext *se,
+			    u32 count, u64 handles)
+{
+	struct drm_v3d_sem __user *post_deps;
+	int i, ret;
+
+	if (!count)
+		return 0;
+
+	se->out_syncs = (struct v3d_submit_outsync *)
+			kvmalloc_array(count,
+				       sizeof(struct v3d_submit_outsync),
+				       GFP_KERNEL);
+	if (!se->out_syncs)
+		return -ENOMEM;
+
+	post_deps = u64_to_user_ptr(handles);
+
+	for (i = 0; i < count; i++) {
+		struct drm_v3d_sem out;
+
+		ret = copy_from_user(&out, post_deps++, sizeof(out));
+		if (ret) {
+			DRM_DEBUG("Failed to copy post dep handles\n");
+			goto fail;
+		}
+
+		se->out_syncs[i].syncobj = drm_syncobj_find(file_priv,
+							    out.handle);
+		if (!se->out_syncs[i].syncobj) {
+			ret = -EINVAL;
+			goto fail;
+		}
 	}
+	se->out_sync_count = count;
+
+	return 0;
+
+fail:
+	for (i--; i >= 0; i--)
+		drm_syncobj_put(se->out_syncs[i].syncobj);
+	kvfree(se->out_syncs);
+
+	return ret;
+}
+
+/* Get data for multiple binary semaphores synchronization. Parse syncobj
+ * to be signaled when job completes (out_sync).
+ */
+static int
+v3d_get_multisync_submit_deps(struct drm_file *file_priv,
+			      struct drm_v3d_extension __user *ext,
+			      void *data)
+{
+	struct drm_v3d_multi_sync multisync;
+	struct v3d_submit_ext *se = data;
+	int ret;
+
+	ret = copy_from_user(&multisync, ext, sizeof(multisync));
+	if (ret)
+		return ret;
+
+	if (multisync.pad)
+		return -EINVAL;
+
+	ret = v3d_get_multisync_post_deps(file_priv, data, multisync.out_sync_count,
+					  multisync.out_syncs);
+	if (ret)
+		return ret;
+
+	se->in_sync_count = multisync.in_sync_count;
+	se->in_syncs = multisync.in_syncs;
+	se->flags |= DRM_V3D_EXT_ID_MULTI_SYNC;
+	se->wait_stage = multisync.wait_stage;
+
+	return 0;
 }
 
 /* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data
  * according to the extension id (name).
  */
 static int
-v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles)
+v3d_get_extensions(struct drm_file *file_priv,
+		   u64 ext_handles,
+		   void *data)
 {
 	struct drm_v3d_extension __user *user_ext;
+	int ret;
 
 	user_ext = u64_to_user_ptr(ext_handles);
 	while (user_ext) {
@@ -555,7 +687,11 @@ v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles)
 		}
 
 		switch (ext.id) {
-		case 0:
+		case DRM_V3D_EXT_ID_MULTI_SYNC:
+			ret = v3d_get_multisync_submit_deps(file_priv, user_ext, data);
+			if (ret)
+				return ret;
+			break;
 		default:
 			DRM_DEBUG_DRIVER("Unknown extension id: %d\n", ext.id);
 			return -EINVAL;
@@ -586,6 +722,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
 	struct drm_v3d_submit_cl *args = data;
+	struct v3d_submit_ext se = {0};
 	struct v3d_bin_job *bin = NULL;
 	struct v3d_render_job *render = NULL;
 	struct v3d_job *clean_job = NULL;
@@ -606,7 +743,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
-		ret = v3d_get_extensions(file_priv, args->extensions);
+		ret = v3d_get_extensions(file_priv, args->extensions, &se);
 		if (ret) {
 			DRM_DEBUG("Failed to get extensions.\n");
 			return ret;
@@ -614,7 +751,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	}
 
 	ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render),
-			   v3d_render_job_free, args->in_sync_rcl, V3D_RENDER);
+			   v3d_render_job_free, args->in_sync_rcl, &se, V3D_RENDER);
 	if (ret)
 		goto fail;
 
@@ -624,7 +761,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 
 	if (args->bcl_start != args->bcl_end) {
 		ret = v3d_job_init(v3d, file_priv, (void *)&bin, sizeof(*bin),
-				   v3d_job_free, args->in_sync_bcl, V3D_BIN);
+				   v3d_job_free, args->in_sync_bcl, &se, V3D_BIN);
 		if (ret)
 			goto fail;
 
@@ -638,7 +775,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 
 	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
 		ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job),
-				   v3d_job_free, 0, V3D_CACHE_CLEAN);
+				   v3d_job_free, 0, 0, V3D_CACHE_CLEAN);
 		if (ret)
 			goto fail;
 
@@ -698,6 +835,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 						 last_job,
 						 &acquire_ctx,
 						 args->out_sync,
+						 &se,
 						 last_job->done_fence);
 
 	if (bin)
@@ -716,6 +854,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	v3d_job_cleanup((void *)bin);
 	v3d_job_cleanup((void *)render);
 	v3d_job_cleanup(clean_job);
+	v3d_put_multisync_post_deps(&se);
 
 	return ret;
 }
@@ -735,6 +874,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 {
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 	struct drm_v3d_submit_tfu *args = data;
+	struct v3d_submit_ext se = {0};
 	struct v3d_tfu_job *job = NULL;
 	struct ww_acquire_ctx acquire_ctx;
 	int ret = 0;
@@ -747,7 +887,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
-		ret = v3d_get_extensions(file_priv, args->extensions);
+		ret = v3d_get_extensions(file_priv, args->extensions, &se);
 		if (ret) {
 			DRM_DEBUG("Failed to get extensions.\n");
 			return ret;
@@ -755,7 +895,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	}
 
 	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
-			   v3d_job_free, args->in_sync, V3D_TFU);
+			   v3d_job_free, args->in_sync, &se, V3D_TFU);
 	if (ret)
 		goto fail;
 
@@ -803,6 +943,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	v3d_attach_fences_and_unlock_reservation(file_priv,
 						 &job->base, &acquire_ctx,
 						 args->out_sync,
+						 &se,
 						 job->base.done_fence);
 
 	v3d_job_put(&job->base);
@@ -811,6 +952,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 
 fail:
 	v3d_job_cleanup((void *)job);
+	v3d_put_multisync_post_deps(&se);
 
 	return ret;
 }
@@ -831,6 +973,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
 	struct drm_v3d_submit_csd *args = data;
+	struct v3d_submit_ext se = {0};
 	struct v3d_csd_job *job = NULL;
 	struct v3d_job *clean_job = NULL;
 	struct ww_acquire_ctx acquire_ctx;
@@ -852,7 +995,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
-		ret = v3d_get_extensions(file_priv, args->extensions);
+		ret = v3d_get_extensions(file_priv, args->extensions, &se);
 		if (ret) {
 			DRM_DEBUG("Failed to get extensions.\n");
 			return ret;
@@ -860,12 +1003,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	}
 
 	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
-			   v3d_job_free, args->in_sync, V3D_CSD);
+			   v3d_job_free, args->in_sync, &se, V3D_CSD);
 	if (ret)
 		goto fail;
 
 	ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job),
-			   v3d_job_free, 0, V3D_CACHE_CLEAN);
+			   v3d_job_free, 0, 0, V3D_CACHE_CLEAN);
 	if (ret)
 		goto fail;
 
@@ -904,6 +1047,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 						 clean_job,
 						 &acquire_ctx,
 						 args->out_sync,
+						 &se,
 						 clean_job->done_fence);
 
 	v3d_job_put(&job->base);
@@ -918,6 +1062,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 fail:
 	v3d_job_cleanup((void *)job);
 	v3d_job_cleanup(clean_job);
+	v3d_put_multisync_post_deps(&se);
 
 	return ret;
 }
diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h
index 55b443ca6c0b..3dfc0af8756a 100644
--- a/include/uapi/drm/v3d_drm.h
+++ b/include/uapi/drm/v3d_drm.h
@@ -73,6 +73,53 @@ struct drm_v3d_extension {
 	__u32 flags; /* mbz */
 };
 
+/* struct drm_v3d_sem - wait/signal semaphore
+ *
+ * If binary semaphore, it only takes syncobj handle and ignores flags and
+ * point fields. Point is defined for timeline syncobj feature.
+ */
+struct drm_v3d_sem {
+	__u32 handle; /* syncobj */
+	/* rsv below, for future uses */
+	__u32 flags;
+	__u64 point;  /* for timeline sem support */
+	__u64 mbz[2]; /* must be zero, rsv */
+};
+
+/* Enum for each of the V3D queues. */
+enum v3d_queue {
+	V3D_BIN,
+	V3D_RENDER,
+	V3D_TFU,
+	V3D_CSD,
+	V3D_CACHE_CLEAN,
+};
+
+/**
+ * struct drm_v3d_multi_sync - ioctl extension to add support multiples
+ * syncobjs for commands submission.
+ *
+ * When an extension of DRM_V3D_EXT_ID_MULTI_SYNC id is defined, it points to
+ * this extension to define wait and signal dependencies, instead of single
+ * in/out sync entries on submitting commands. The field flags is used to
+ * determine the stage to set wait dependencies.
+ */
+struct drm_v3d_multi_sync {
+	struct drm_v3d_extension base;
+	/* Array of wait and signal semaphores */
+	__u64 in_syncs;
+	__u64 out_syncs;
+
+	/* Number of entries */
+	__u32 in_sync_count;
+	__u32 out_sync_count;
+
+	/* set the stage (v3d_queue) to sync */
+	__u32 wait_stage;
+
+	__u32 pad; /* mbz */
+};
+
 /**
  * struct drm_v3d_submit_cl - ioctl argument for submitting commands to the 3D
  * engine.
@@ -228,6 +275,7 @@ enum drm_v3d_param {
 	DRM_V3D_PARAM_SUPPORTS_CSD,
 	DRM_V3D_PARAM_SUPPORTS_CACHE_FLUSH,
 	DRM_V3D_PARAM_SUPPORTS_PERFMON,
+	DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT,
 };
 
 struct drm_v3d_get_param {
@@ -271,7 +319,6 @@ struct drm_v3d_submit_tfu {
 
 	/* Pointer to an array of ioctl extensions*/
 	__u64 extensions;
-
 };
 
 /* Submits a compute shader for dispatch.  This job will block on any
-- 
2.30.2


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

Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support

[Iago Toral]

On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
> Using the generic extension from the previous patch, a specific
> multisync
> extension enables more than one in/out binary syncobj per job
> submission.
> Arrays of syncobjs are set in struct drm_v3d_multisync, that also
> cares
> of determining the stage for sync (wait deps) according to the job
> queue.
> 
> v2:
> - subclass the generic extension struct (Daniel)
> - simplify adding dependency conditions to make understandable (Iago)
> 
> v3:
> - fix conditions to consider single or multiples in/out_syncs (Iago)
> - remove irrelevant comment (Iago)
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>  drivers/gpu/drm/v3d/v3d_drv.c |   6 +-
>  drivers/gpu/drm/v3d/v3d_drv.h |  24 +++--
>  drivers/gpu/drm/v3d/v3d_gem.c | 185 ++++++++++++++++++++++++++++++
> ----
>  include/uapi/drm/v3d_drm.h    |  49 ++++++++-
>  4 files changed, 232 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c
> b/drivers/gpu/drm/v3d/v3d_drv.c
> index 3d6b9bcce2f7..bd46396a1ae0 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device 

(...)

> 
> @@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct
> drm_file *file_priv,
>  					 struct v3d_job *job,
>  					 struct ww_acquire_ctx
> *acquire_ctx,
>  					 u32 out_sync,
> +					 struct v3d_submit_ext *se,
>  					 struct dma_fence *done_fence)
>  {
>  	struct drm_syncobj *sync_out;
> +	bool has_multisync = se && (se->flags & 

We always pass the 'se' pointer from a local variable allocated in the
stack of the caller so it is never going to be NULL.

I am happy to keep the NULL check if you want to protect against future
changes just in case, but if we do that then...

> DRM_V3D_EXT_ID_MULTI_SYNC);
>  	int i;
>  
>  	for (i = 0; i < job->bo_count; i++) {

(...)

> +static void
> +v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
> +{
> +	unsigned int i;
> +
> +	if (!se->out_sync_count)

...we should also check for NULL here for consistency.

Also, I think there is another problem in the code: we always call
v3d_job_cleanup for failed jobs, but only call v3d_job_put for
successful jobs. However, reading the docs for drm_sched_job_init:

"Drivers must make sure drm_sched_job_cleanup() if this function
returns successfully, even when @job is aborted before
drm_sched_job_arm() is called."

So my understanding is that we should call v3d_job_cleanup instead of
v3d_job_put for successful jobs or we would be leaking sched resources
on every successful job, no?

Iago

> +		return;
> +
> +	for (i = 0; i < se->out_sync_count; i++)
> +		drm_syncobj_put(se->out_syncs[i].syncobj);
> +	kvfree(se->out_syncs);
> +}
> +
> +static int
> +v3d_get_multisync_post_deps(struct drm_file *file_priv,
> +			    struct v3d_submit_ext *se,
> +			    u32 count, u64 handles)
> +{
> +	struct drm_v3d_sem __user *post_deps;
> +	int i, ret;
> +
> +	if (!count)
> +		return 0;
> +
> +	se->out_syncs = (struct v3d_submit_outsync *)
> +			kvmalloc_array(count,
> +				       sizeof(struct
> v3d_submit_outsync),
> +				       GFP_KERNEL);
> +	if (!se->out_syncs)
> +		return -ENOMEM;
> +
> +	post_deps = u64_to_user_ptr(handles);
> +
> +	for (i = 0; i < count; i++) {
> +		struct drm_v3d_sem out;
> +
> +		ret = copy_from_user(&out, post_deps++, sizeof(out));
> +		if (ret) {
> +			DRM_DEBUG("Failed to copy post dep handles\n");
> +			goto fail;
> +		}
> +
> +		se->out_syncs[i].syncobj = drm_syncobj_find(file_priv,
> +							    out.handle)
> ;
> +		if (!se->out_syncs[i].syncobj) {
> +			ret = -EINVAL;
> +			goto fail;
> +		}
>  	}
> +	se->out_sync_count = count;
> +
> +	return 0;
> +
> +fail:
> +	for (i--; i >= 0; i--)
> +		drm_syncobj_put(se->out_syncs[i].syncobj);
> +	kvfree(se->out_syncs);
> +
> +	return ret;
> +}
> +
> +/* Get data for multiple binary semaphores synchronization. Parse
> syncobj
> + * to be signaled when job completes (out_sync).
> + */
> +static int
> +v3d_get_multisync_submit_deps(struct drm_file *file_priv,
> +			      struct drm_v3d_extension __user *ext,
> +			      void *data)
> +{
> +	struct drm_v3d_multi_sync multisync;
> +	struct v3d_submit_ext *se = data;
> +	int ret;
> +
> +	ret = copy_from_user(&multisync, ext, sizeof(multisync));
> +	if (ret)
> +		return ret;
> +
> +	if (multisync.pad)
> +		return -EINVAL;
> +
> +	ret = v3d_get_multisync_post_deps(file_priv, data,
> multisync.out_sync_count,
> +					  multisync.out_syncs);
> +	if (ret)
> +		return ret;
> +
> +	se->in_sync_count = multisync.in_sync_count;
> +	se->in_syncs = multisync.in_syncs;
> +	se->flags |= DRM_V3D_EXT_ID_MULTI_SYNC;
> +	se->wait_stage = multisync.wait_stage;
> +
> +	return 0;
>  }
>  
>  /* Whenever userspace sets ioctl extensions, v3d_get_extensions
> parses data
>   * according to the extension id (name).
>   */
>  static int
> -v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles)
> +v3d_get_extensions(struct drm_file *file_priv,
> +		   u64 ext_handles,
> +		   void *data)
>  {
>  	struct drm_v3d_extension __user *user_ext;
> +	int ret;
>  
>  	user_ext = u64_to_user_ptr(ext_handles);
>  	while (user_ext) {
> @@ -555,7 +687,11 @@ v3d_get_extensions(struct drm_file *file_priv,
> u64 ext_handles)
>  		}
>  
>  		switch (ext.id) {
> -		case 0:
> +		case DRM_V3D_EXT_ID_MULTI_SYNC:
> +			ret = v3d_get_multisync_submit_deps(file_priv,
> user_ext, data);
> +			if (ret)
> +				return ret;
> +			break;
>  		default:
>  			DRM_DEBUG_DRIVER("Unknown extension id: %d\n",
> ext.id);
>  			return -EINVAL;
> @@ -586,6 +722,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> *data,
>  	struct v3d_dev *v3d = to_v3d_dev(dev);
>  	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
>  	struct drm_v3d_submit_cl *args = data;
> +	struct v3d_submit_ext se = {0};
>  	struct v3d_bin_job *bin = NULL;
>  	struct v3d_render_job *render = NULL;
>  	struct v3d_job *clean_job = NULL;
> @@ -606,7 +743,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> *data,
>  	}
>  
>  	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
> -		ret = v3d_get_extensions(file_priv, args->extensions);
> +		ret = v3d_get_extensions(file_priv, args->extensions,
> &se);
>  		if (ret) {
>  			DRM_DEBUG("Failed to get extensions.\n");
>  			return ret;
> @@ -614,7 +751,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> *data,
>  	}
>  
>  	ret = v3d_job_init(v3d, file_priv, (void *)&render,
> sizeof(*render),
> -			   v3d_render_job_free, args->in_sync_rcl,
> V3D_RENDER);
> +			   v3d_render_job_free, args->in_sync_rcl, &se,
> V3D_RENDER);
>  	if (ret)
>  		goto fail;
>  
> @@ -624,7 +761,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> *data,
>  
>  	if (args->bcl_start != args->bcl_end) {
>  		ret = v3d_job_init(v3d, file_priv, (void *)&bin,
> sizeof(*bin),
> -				   v3d_job_free, args->in_sync_bcl,
> V3D_BIN);
> +				   v3d_job_free, args->in_sync_bcl,
> &se, V3D_BIN);
>  		if (ret)
>  			goto fail;
>  
> @@ -638,7 +775,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> *data,
>  
>  	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
>  		ret = v3d_job_init(v3d, file_priv, (void *)&clean_job,
> sizeof(*clean_job),
> -				   v3d_job_free, 0, V3D_CACHE_CLEAN);
> +				   v3d_job_free, 0, 0,
> V3D_CACHE_CLEAN);
>  		if (ret)
>  			goto fail;
>  
> @@ -698,6 +835,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> *data,
>  						 last_job,
>  						 &acquire_ctx,
>  						 args->out_sync,
> +						 &se,
>  						 last_job->done_fence);
>  
>  	if (bin)
> @@ -716,6 +854,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> *data,
>  	v3d_job_cleanup((void *)bin);
>  	v3d_job_cleanup((void *)render);
>  	v3d_job_cleanup(clean_job);
> +	v3d_put_multisync_post_deps(&se);
>  
>  	return ret;
>  }
> @@ -735,6 +874,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> *data,
>  {
>  	struct v3d_dev *v3d = to_v3d_dev(dev);
>  	struct drm_v3d_submit_tfu *args = data;
> +	struct v3d_submit_ext se = {0};
>  	struct v3d_tfu_job *job = NULL;
>  	struct ww_acquire_ctx acquire_ctx;
>  	int ret = 0;
> @@ -747,7 +887,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> *data,
>  	}
>  
>  	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
> -		ret = v3d_get_extensions(file_priv, args->extensions);
> +		ret = v3d_get_extensions(file_priv, args->extensions,
> &se);
>  		if (ret) {
>  			DRM_DEBUG("Failed to get extensions.\n");
>  			return ret;
> @@ -755,7 +895,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> *data,
>  	}
>  
>  	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
> -			   v3d_job_free, args->in_sync, V3D_TFU);
> +			   v3d_job_free, args->in_sync, &se, V3D_TFU);
>  	if (ret)
>  		goto fail;
>  
> @@ -803,6 +943,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> *data,
>  	v3d_attach_fences_and_unlock_reservation(file_priv,
>  						 &job->base,
> &acquire_ctx,
>  						 args->out_sync,
> +						 &se,
>  						 job->base.done_fence);
>  
>  	v3d_job_put(&job->base);
> @@ -811,6 +952,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> *data,
>  
>  fail:
>  	v3d_job_cleanup((void *)job);
> +	v3d_put_multisync_post_deps(&se);
>  
>  	return ret;
>  }
> @@ -831,6 +973,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void
> *data,
>  	struct v3d_dev *v3d = to_v3d_dev(dev);
>  	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
>  	struct drm_v3d_submit_csd *args = data;
> +	struct v3d_submit_ext se = {0};
>  	struct v3d_csd_job *job = NULL;
>  	struct v3d_job *clean_job = NULL;
>  	struct ww_acquire_ctx acquire_ctx;
> @@ -852,7 +995,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void
> *data,
>  	}
>  
>  	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
> -		ret = v3d_get_extensions(file_priv, args->extensions);
> +		ret = v3d_get_extensions(file_priv, args->extensions,
> &se);
>  		if (ret) {
>  			DRM_DEBUG("Failed to get extensions.\n");
>  			return ret;
> @@ -860,12 +1003,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> void *data,
>  	}
>  
>  	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
> -			   v3d_job_free, args->in_sync, V3D_CSD);
> +			   v3d_job_free, args->in_sync, &se, V3D_CSD);
>  	if (ret)
>  		goto fail;
>  
>  	ret = v3d_job_init(v3d, file_priv, (void *)&clean_job,
> sizeof(*clean_job),
> -			   v3d_job_free, 0, V3D_CACHE_CLEAN);
> +			   v3d_job_free, 0, 0, V3D_CACHE_CLEAN);
>  	if (ret)
>  		goto fail;
>  
> @@ -904,6 +1047,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> void *data,
>  						 clean_job,
>  						 &acquire_ctx,
>  						 args->out_sync,
> +						 &se,
>  						 clean_job-
> >done_fence);
>  
>  	v3d_job_put(&job->base);
> @@ -918,6 +1062,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> void *data,
>  fail:
>  	v3d_job_cleanup((void *)job);
>  	v3d_job_cleanup(clean_job);
> +	v3d_put_multisync_post_deps(&se);
>  
>  	return ret;
>  }
> diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h
> index 55b443ca6c0b..3dfc0af8756a 100644
> --- a/include/uapi/drm/v3d_drm.h
> +++ b/include/uapi/drm/v3d_drm.h
> @@ -73,6 +73,53 @@ struct drm_v3d_extension {
>  	__u32 flags; /* mbz */
>  };
>  
> +/* struct drm_v3d_sem - wait/signal semaphore
> + *
> + * If binary semaphore, it only takes syncobj handle and ignores
> flags and
> + * point fields. Point is defined for timeline syncobj feature.
> + */
> +struct drm_v3d_sem {
> +	__u32 handle; /* syncobj */
> +	/* rsv below, for future uses */
> +	__u32 flags;
> +	__u64 point;  /* for timeline sem support */
> +	__u64 mbz[2]; /* must be zero, rsv */
> +};
> +
> +/* Enum for each of the V3D queues. */
> +enum v3d_queue {
> +	V3D_BIN,
> +	V3D_RENDER,
> +	V3D_TFU,
> +	V3D_CSD,
> +	V3D_CACHE_CLEAN,
> +};
> +
> +/**
> + * struct drm_v3d_multi_sync - ioctl extension to add support
> multiples
> + * syncobjs for commands submission.
> + *
> + * When an extension of DRM_V3D_EXT_ID_MULTI_SYNC id is defined, it
> points to
> + * this extension to define wait and signal dependencies, instead of
> single
> + * in/out sync entries on submitting commands. The field flags is
> used to
> + * determine the stage to set wait dependencies.
> + */
> +struct drm_v3d_multi_sync {
> +	struct drm_v3d_extension base;
> +	/* Array of wait and signal semaphores */
> +	__u64 in_syncs;
> +	__u64 out_syncs;
> +
> +	/* Number of entries */
> +	__u32 in_sync_count;
> +	__u32 out_sync_count;
> +
> +	/* set the stage (v3d_queue) to sync */
> +	__u32 wait_stage;
> +
> +	__u32 pad; /* mbz */
> +};
> +
>  /**
>   * struct drm_v3d_submit_cl - ioctl argument for submitting commands
> to the 3D
>   * engine.
> @@ -228,6 +275,7 @@ enum drm_v3d_param {
>  	DRM_V3D_PARAM_SUPPORTS_CSD,
>  	DRM_V3D_PARAM_SUPPORTS_CACHE_FLUSH,
>  	DRM_V3D_PARAM_SUPPORTS_PERFMON,
> +	DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT,
>  };
>  
>  struct drm_v3d_get_param {
> @@ -271,7 +319,6 @@ struct drm_v3d_submit_tfu {
>  
>  	/* Pointer to an array of ioctl extensions*/
>  	__u64 extensions;
> -
>  };
>  
>  /* Submits a compute shader for dispatch.  This job will block on
> any

Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support

[Melissa Wen]

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

On 10/01, Iago Toral wrote:
> On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
> > Using the generic extension from the previous patch, a specific
> > multisync
> > extension enables more than one in/out binary syncobj per job
> > submission.
> > Arrays of syncobjs are set in struct drm_v3d_multisync, that also
> > cares
> > of determining the stage for sync (wait deps) according to the job
> > queue.
> > 
> > v2:
> > - subclass the generic extension struct (Daniel)
> > - simplify adding dependency conditions to make understandable (Iago)
> > 
> > v3:
> > - fix conditions to consider single or multiples in/out_syncs (Iago)
> > - remove irrelevant comment (Iago)
> > 
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >  drivers/gpu/drm/v3d/v3d_drv.c |   6 +-
> >  drivers/gpu/drm/v3d/v3d_drv.h |  24 +++--
> >  drivers/gpu/drm/v3d/v3d_gem.c | 185 ++++++++++++++++++++++++++++++
> > ----
> >  include/uapi/drm/v3d_drm.h    |  49 ++++++++-
> >  4 files changed, 232 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c
> > b/drivers/gpu/drm/v3d/v3d_drv.c
> > index 3d6b9bcce2f7..bd46396a1ae0 100644
> > --- a/drivers/gpu/drm/v3d/v3d_drv.c
> > +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> > @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device 
> 
> (...)
> 
> > 
> > @@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct
> > drm_file *file_priv,
> >  					 struct v3d_job *job,
> >  					 struct ww_acquire_ctx
> > *acquire_ctx,
> >  					 u32 out_sync,
> > +					 struct v3d_submit_ext *se,
> >  					 struct dma_fence *done_fence)
> >  {
> >  	struct drm_syncobj *sync_out;
> > +	bool has_multisync = se && (se->flags & 
> 
> We always pass the 'se' pointer from a local variable allocated in the
> stack of the caller so it is never going to be NULL.
> 
> I am happy to keep the NULL check if you want to protect against future
> changes just in case, but if we do that then...
> 
> > DRM_V3D_EXT_ID_MULTI_SYNC);
> >  	int i;
> >  
> >  	for (i = 0; i < job->bo_count; i++) {
> 
> (...)
> 
> > +static void
> > +v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
> > +{
> > +	unsigned int i;
> > +
> > +	if (!se->out_sync_count)
> 
> ...we should also check for NULL here for consistency.
yes, consistency makes sense here.
> 
> Also, I think there is another problem in the code: we always call
> v3d_job_cleanup for failed jobs, but only call v3d_job_put for
> successful jobs. However, reading the docs for drm_sched_job_init:
> 
> "Drivers must make sure drm_sched_job_cleanup() if this function
> returns successfully, even when @job is aborted before
> drm_sched_job_arm() is called."
> 
> So my understanding is that we should call v3d_job_cleanup instead of
> v3d_job_put for successful jobs or we would be leaking sched resources
> on every successful job, no?

When job_init is successful, v3d_job_cleanup is called by scheduler when
job is completed. drm_sched_job_cleanup describes how it works after
drm_sched_job_arm:

" After that point of no return @job is committed to be executed by the
scheduler, and this function should be called from the
&drm_sched_backend_ops.free_job callback."

On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in turn
calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it looks
ok.

Also, we can say that the very last v3d_job_put() is in charge to
decrement refcount initialized (set 1) in v3d_job_init(); while the
v3d_job_cleanup() from v3d_sched_job_free() callback decrements refcount
that was incremented when v3d_job_push() pushed the job to the
scheduler.

So, refcount pairs seem consistent, I mean, get and put. And about
drm_sched_job_cleanup, it is explicitly called when job_init fails or
after that by the scheduler.

Thanks,

Melissa

> 
> Iago
> 
> > +		return;
> > +
> > +	for (i = 0; i < se->out_sync_count; i++)
> > +		drm_syncobj_put(se->out_syncs[i].syncobj);
> > +	kvfree(se->out_syncs);
> > +}
> > +
> > +static int
> > +v3d_get_multisync_post_deps(struct drm_file *file_priv,
> > +			    struct v3d_submit_ext *se,
> > +			    u32 count, u64 handles)
> > +{
> > +	struct drm_v3d_sem __user *post_deps;
> > +	int i, ret;
> > +
> > +	if (!count)
> > +		return 0;
> > +
> > +	se->out_syncs = (struct v3d_submit_outsync *)
> > +			kvmalloc_array(count,
> > +				       sizeof(struct
> > v3d_submit_outsync),
> > +				       GFP_KERNEL);
> > +	if (!se->out_syncs)
> > +		return -ENOMEM;
> > +
> > +	post_deps = u64_to_user_ptr(handles);
> > +
> > +	for (i = 0; i < count; i++) {
> > +		struct drm_v3d_sem out;
> > +
> > +		ret = copy_from_user(&out, post_deps++, sizeof(out));
> > +		if (ret) {
> > +			DRM_DEBUG("Failed to copy post dep handles\n");
> > +			goto fail;
> > +		}
> > +
> > +		se->out_syncs[i].syncobj = drm_syncobj_find(file_priv,
> > +							    out.handle)
> > ;
> > +		if (!se->out_syncs[i].syncobj) {
> > +			ret = -EINVAL;
> > +			goto fail;
> > +		}
> >  	}
> > +	se->out_sync_count = count;
> > +
> > +	return 0;
> > +
> > +fail:
> > +	for (i--; i >= 0; i--)
> > +		drm_syncobj_put(se->out_syncs[i].syncobj);
> > +	kvfree(se->out_syncs);
> > +
> > +	return ret;
> > +}
> > +
> > +/* Get data for multiple binary semaphores synchronization. Parse
> > syncobj
> > + * to be signaled when job completes (out_sync).
> > + */
> > +static int
> > +v3d_get_multisync_submit_deps(struct drm_file *file_priv,
> > +			      struct drm_v3d_extension __user *ext,
> > +			      void *data)
> > +{
> > +	struct drm_v3d_multi_sync multisync;
> > +	struct v3d_submit_ext *se = data;
> > +	int ret;
> > +
> > +	ret = copy_from_user(&multisync, ext, sizeof(multisync));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (multisync.pad)
> > +		return -EINVAL;
> > +
> > +	ret = v3d_get_multisync_post_deps(file_priv, data,
> > multisync.out_sync_count,
> > +					  multisync.out_syncs);
> > +	if (ret)
> > +		return ret;
> > +
> > +	se->in_sync_count = multisync.in_sync_count;
> > +	se->in_syncs = multisync.in_syncs;
> > +	se->flags |= DRM_V3D_EXT_ID_MULTI_SYNC;
> > +	se->wait_stage = multisync.wait_stage;
> > +
> > +	return 0;
> >  }
> >  
> >  /* Whenever userspace sets ioctl extensions, v3d_get_extensions
> > parses data
> >   * according to the extension id (name).
> >   */
> >  static int
> > -v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles)
> > +v3d_get_extensions(struct drm_file *file_priv,
> > +		   u64 ext_handles,
> > +		   void *data)
> >  {
> >  	struct drm_v3d_extension __user *user_ext;
> > +	int ret;
> >  
> >  	user_ext = u64_to_user_ptr(ext_handles);
> >  	while (user_ext) {
> > @@ -555,7 +687,11 @@ v3d_get_extensions(struct drm_file *file_priv,
> > u64 ext_handles)
> >  		}
> >  
> >  		switch (ext.id) {
> > -		case 0:
> > +		case DRM_V3D_EXT_ID_MULTI_SYNC:
> > +			ret = v3d_get_multisync_submit_deps(file_priv,
> > user_ext, data);
> > +			if (ret)
> > +				return ret;
> > +			break;
> >  		default:
> >  			DRM_DEBUG_DRIVER("Unknown extension id: %d\n",
> > ext.id);
> >  			return -EINVAL;
> > @@ -586,6 +722,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> > *data,
> >  	struct v3d_dev *v3d = to_v3d_dev(dev);
> >  	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
> >  	struct drm_v3d_submit_cl *args = data;
> > +	struct v3d_submit_ext se = {0};
> >  	struct v3d_bin_job *bin = NULL;
> >  	struct v3d_render_job *render = NULL;
> >  	struct v3d_job *clean_job = NULL;
> > @@ -606,7 +743,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> > *data,
> >  	}
> >  
> >  	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
> > -		ret = v3d_get_extensions(file_priv, args->extensions);
> > +		ret = v3d_get_extensions(file_priv, args->extensions,
> > &se);
> >  		if (ret) {
> >  			DRM_DEBUG("Failed to get extensions.\n");
> >  			return ret;
> > @@ -614,7 +751,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> > *data,
> >  	}
> >  
> >  	ret = v3d_job_init(v3d, file_priv, (void *)&render,
> > sizeof(*render),
> > -			   v3d_render_job_free, args->in_sync_rcl,
> > V3D_RENDER);
> > +			   v3d_render_job_free, args->in_sync_rcl, &se,
> > V3D_RENDER);
> >  	if (ret)
> >  		goto fail;
> >  
> > @@ -624,7 +761,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> > *data,
> >  
> >  	if (args->bcl_start != args->bcl_end) {
> >  		ret = v3d_job_init(v3d, file_priv, (void *)&bin,
> > sizeof(*bin),
> > -				   v3d_job_free, args->in_sync_bcl,
> > V3D_BIN);
> > +				   v3d_job_free, args->in_sync_bcl,
> > &se, V3D_BIN);
> >  		if (ret)
> >  			goto fail;
> >  
> > @@ -638,7 +775,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> > *data,
> >  
> >  	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
> >  		ret = v3d_job_init(v3d, file_priv, (void *)&clean_job,
> > sizeof(*clean_job),
> > -				   v3d_job_free, 0, V3D_CACHE_CLEAN);
> > +				   v3d_job_free, 0, 0,
> > V3D_CACHE_CLEAN);
> >  		if (ret)
> >  			goto fail;
> >  
> > @@ -698,6 +835,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> > *data,
> >  						 last_job,
> >  						 &acquire_ctx,
> >  						 args->out_sync,
> > +						 &se,
> >  						 last_job->done_fence);
> >  
> >  	if (bin)
> > @@ -716,6 +854,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void
> > *data,
> >  	v3d_job_cleanup((void *)bin);
> >  	v3d_job_cleanup((void *)render);
> >  	v3d_job_cleanup(clean_job);
> > +	v3d_put_multisync_post_deps(&se);
> >  
> >  	return ret;
> >  }
> > @@ -735,6 +874,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> > *data,
> >  {
> >  	struct v3d_dev *v3d = to_v3d_dev(dev);
> >  	struct drm_v3d_submit_tfu *args = data;
> > +	struct v3d_submit_ext se = {0};
> >  	struct v3d_tfu_job *job = NULL;
> >  	struct ww_acquire_ctx acquire_ctx;
> >  	int ret = 0;
> > @@ -747,7 +887,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> > *data,
> >  	}
> >  
> >  	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
> > -		ret = v3d_get_extensions(file_priv, args->extensions);
> > +		ret = v3d_get_extensions(file_priv, args->extensions,
> > &se);
> >  		if (ret) {
> >  			DRM_DEBUG("Failed to get extensions.\n");
> >  			return ret;
> > @@ -755,7 +895,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> > *data,
> >  	}
> >  
> >  	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
> > -			   v3d_job_free, args->in_sync, V3D_TFU);
> > +			   v3d_job_free, args->in_sync, &se, V3D_TFU);
> >  	if (ret)
> >  		goto fail;
> >  
> > @@ -803,6 +943,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> > *data,
> >  	v3d_attach_fences_and_unlock_reservation(file_priv,
> >  						 &job->base,
> > &acquire_ctx,
> >  						 args->out_sync,
> > +						 &se,
> >  						 job->base.done_fence);
> >  
> >  	v3d_job_put(&job->base);
> > @@ -811,6 +952,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void
> > *data,
> >  
> >  fail:
> >  	v3d_job_cleanup((void *)job);
> > +	v3d_put_multisync_post_deps(&se);
> >  
> >  	return ret;
> >  }
> > @@ -831,6 +973,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void
> > *data,
> >  	struct v3d_dev *v3d = to_v3d_dev(dev);
> >  	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
> >  	struct drm_v3d_submit_csd *args = data;
> > +	struct v3d_submit_ext se = {0};
> >  	struct v3d_csd_job *job = NULL;
> >  	struct v3d_job *clean_job = NULL;
> >  	struct ww_acquire_ctx acquire_ctx;
> > @@ -852,7 +995,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void
> > *data,
> >  	}
> >  
> >  	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
> > -		ret = v3d_get_extensions(file_priv, args->extensions);
> > +		ret = v3d_get_extensions(file_priv, args->extensions,
> > &se);
> >  		if (ret) {
> >  			DRM_DEBUG("Failed to get extensions.\n");
> >  			return ret;
> > @@ -860,12 +1003,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> > void *data,
> >  	}
> >  
> >  	ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
> > -			   v3d_job_free, args->in_sync, V3D_CSD);
> > +			   v3d_job_free, args->in_sync, &se, V3D_CSD);
> >  	if (ret)
> >  		goto fail;
> >  
> >  	ret = v3d_job_init(v3d, file_priv, (void *)&clean_job,
> > sizeof(*clean_job),
> > -			   v3d_job_free, 0, V3D_CACHE_CLEAN);
> > +			   v3d_job_free, 0, 0, V3D_CACHE_CLEAN);
> >  	if (ret)
> >  		goto fail;
> >  
> > @@ -904,6 +1047,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> > void *data,
> >  						 clean_job,
> >  						 &acquire_ctx,
> >  						 args->out_sync,
> > +						 &se,
> >  						 clean_job-
> > >done_fence);
> >  
> >  	v3d_job_put(&job->base);
> > @@ -918,6 +1062,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
> > void *data,
> >  fail:
> >  	v3d_job_cleanup((void *)job);
> >  	v3d_job_cleanup(clean_job);
> > +	v3d_put_multisync_post_deps(&se);
> >  
> >  	return ret;
> >  }
> > diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h
> > index 55b443ca6c0b..3dfc0af8756a 100644
> > --- a/include/uapi/drm/v3d_drm.h
> > +++ b/include/uapi/drm/v3d_drm.h
> > @@ -73,6 +73,53 @@ struct drm_v3d_extension {
> >  	__u32 flags; /* mbz */
> >  };
> >  
> > +/* struct drm_v3d_sem - wait/signal semaphore
> > + *
> > + * If binary semaphore, it only takes syncobj handle and ignores
> > flags and
> > + * point fields. Point is defined for timeline syncobj feature.
> > + */
> > +struct drm_v3d_sem {
> > +	__u32 handle; /* syncobj */
> > +	/* rsv below, for future uses */
> > +	__u32 flags;
> > +	__u64 point;  /* for timeline sem support */
> > +	__u64 mbz[2]; /* must be zero, rsv */
> > +};
> > +
> > +/* Enum for each of the V3D queues. */
> > +enum v3d_queue {
> > +	V3D_BIN,
> > +	V3D_RENDER,
> > +	V3D_TFU,
> > +	V3D_CSD,
> > +	V3D_CACHE_CLEAN,
> > +};
> > +
> > +/**
> > + * struct drm_v3d_multi_sync - ioctl extension to add support
> > multiples
> > + * syncobjs for commands submission.
> > + *
> > + * When an extension of DRM_V3D_EXT_ID_MULTI_SYNC id is defined, it
> > points to
> > + * this extension to define wait and signal dependencies, instead of
> > single
> > + * in/out sync entries on submitting commands. The field flags is
> > used to
> > + * determine the stage to set wait dependencies.
> > + */
> > +struct drm_v3d_multi_sync {
> > +	struct drm_v3d_extension base;
> > +	/* Array of wait and signal semaphores */
> > +	__u64 in_syncs;
> > +	__u64 out_syncs;
> > +
> > +	/* Number of entries */
> > +	__u32 in_sync_count;
> > +	__u32 out_sync_count;
> > +
> > +	/* set the stage (v3d_queue) to sync */
> > +	__u32 wait_stage;
> > +
> > +	__u32 pad; /* mbz */
> > +};
> > +
> >  /**
> >   * struct drm_v3d_submit_cl - ioctl argument for submitting commands
> > to the 3D
> >   * engine.
> > @@ -228,6 +275,7 @@ enum drm_v3d_param {
> >  	DRM_V3D_PARAM_SUPPORTS_CSD,
> >  	DRM_V3D_PARAM_SUPPORTS_CACHE_FLUSH,
> >  	DRM_V3D_PARAM_SUPPORTS_PERFMON,
> > +	DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT,
> >  };
> >  
> >  struct drm_v3d_get_param {
> > @@ -271,7 +319,6 @@ struct drm_v3d_submit_tfu {
> >  
> >  	/* Pointer to an array of ioctl extensions*/
> >  	__u64 extensions;
> > -
> >  };
> >  
> >  /* Submits a compute shader for dispatch.  This job will block on
> > any
> 

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

Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support

[Iago Toral]

On Fri, 2021-10-01 at 09:37 +0100, Melissa Wen wrote:
> On 10/01, Iago Toral wrote:
> > On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
> > > Using the generic extension from the previous patch, a specific
> > > multisync
> > > extension enables more than one in/out binary syncobj per job
> > > submission.
> > > Arrays of syncobjs are set in struct drm_v3d_multisync, that also
> > > cares
> > > of determining the stage for sync (wait deps) according to the
> > > job
> > > queue.
> > > 
> > > v2:
> > > - subclass the generic extension struct (Daniel)
> > > - simplify adding dependency conditions to make understandable
> > > (Iago)
> > > 
> > > v3:
> > > - fix conditions to consider single or multiples in/out_syncs
> > > (Iago)
> > > - remove irrelevant comment (Iago)
> > > 
> > > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > > ---
> > >  drivers/gpu/drm/v3d/v3d_drv.c |   6 +-
> > >  drivers/gpu/drm/v3d/v3d_drv.h |  24 +++--
> > >  drivers/gpu/drm/v3d/v3d_gem.c | 185
> > > ++++++++++++++++++++++++++++++
> > > ----
> > >  include/uapi/drm/v3d_drm.h    |  49 ++++++++-
> > >  4 files changed, 232 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c
> > > b/drivers/gpu/drm/v3d/v3d_drv.c
> > > index 3d6b9bcce2f7..bd46396a1ae0 100644
> > > --- a/drivers/gpu/drm/v3d/v3d_drv.c
> > > +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> > > @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct
> > > drm_device 
> > 
> > (...)
> > 
> > > @@ -516,9 +536,11 @@
> > > v3d_attach_fences_and_unlock_reservation(struct
> > > drm_file *file_priv,
> > >  					 struct v3d_job *job,
> > >  					 struct ww_acquire_ctx
> > > *acquire_ctx,
> > >  					 u32 out_sync,
> > > +					 struct v3d_submit_ext *se,
> > >  					 struct dma_fence *done_fence)
> > >  {
> > >  	struct drm_syncobj *sync_out;
> > > +	bool has_multisync = se && (se->flags & 
> > 
> > We always pass the 'se' pointer from a local variable allocated in
> > the
> > stack of the caller so it is never going to be NULL.
> > 
> > I am happy to keep the NULL check if you want to protect against
> > future
> > changes just in case, but if we do that then...
> > 
> > > DRM_V3D_EXT_ID_MULTI_SYNC);
> > >  	int i;
> > >  
> > >  	for (i = 0; i < job->bo_count; i++) {
> > 
> > (...)
> > 
> > > +static void
> > > +v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	if (!se->out_sync_count)
> > 
> > ...we should also check for NULL here for consistency.
> yes, consistency makes sense here.
> > Also, I think there is another problem in the code: we always call
> > v3d_job_cleanup for failed jobs, but only call v3d_job_put for
> > successful jobs. However, reading the docs for drm_sched_job_init:
> > 
> > "Drivers must make sure drm_sched_job_cleanup() if this function
> > returns successfully, even when @job is aborted before
> > drm_sched_job_arm() is called."
> > 
> > So my understanding is that we should call v3d_job_cleanup instead
> > of
> > v3d_job_put for successful jobs or we would be leaking sched
> > resources
> > on every successful job, no?
> 
> When job_init is successful, v3d_job_cleanup is called by scheduler
> when
> job is completed. drm_sched_job_cleanup describes how it works after
> drm_sched_job_arm:
> 
> " After that point of no return @job is committed to be executed by
> the
> scheduler, and this function should be called from the
> &drm_sched_backend_ops.free_job callback."
> 
> On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in
> turn
> calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it
> looks
> ok.
> 
> Also, we can say that the very last v3d_job_put() is in charge to
> decrement refcount initialized (set 1) in v3d_job_init(); while the
> v3d_job_cleanup() from v3d_sched_job_free() callback decrements
> refcount
> that was incremented when v3d_job_push() pushed the job to the
> scheduler.
> 
> So, refcount pairs seem consistent, I mean, get and put. And about
> drm_sched_job_cleanup, it is explicitly called when job_init fails or
> after that by the scheduler.
> 

   A. Ah ok, thanks for explaining this!

With the consistency issue discussed above fixed, for the series:

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>

Iago

Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support

[Melissa Wen]

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

On 10/01, Iago Toral wrote:
> On Fri, 2021-10-01 at 09:37 +0100, Melissa Wen wrote:
> > On 10/01, Iago Toral wrote:
> > > On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
> > > > Using the generic extension from the previous patch, a specific
> > > > multisync
> > > > extension enables more than one in/out binary syncobj per job
> > > > submission.
> > > > Arrays of syncobjs are set in struct drm_v3d_multisync, that also
> > > > cares
> > > > of determining the stage for sync (wait deps) according to the
> > > > job
> > > > queue.
> > > > 
> > > > v2:
> > > > - subclass the generic extension struct (Daniel)
> > > > - simplify adding dependency conditions to make understandable
> > > > (Iago)
> > > > 
> > > > v3:
> > > > - fix conditions to consider single or multiples in/out_syncs
> > > > (Iago)
> > > > - remove irrelevant comment (Iago)
> > > > 
> > > > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > > > ---
> > > >  drivers/gpu/drm/v3d/v3d_drv.c |   6 +-
> > > >  drivers/gpu/drm/v3d/v3d_drv.h |  24 +++--
> > > >  drivers/gpu/drm/v3d/v3d_gem.c | 185
> > > > ++++++++++++++++++++++++++++++
> > > > ----
> > > >  include/uapi/drm/v3d_drm.h    |  49 ++++++++-
> > > >  4 files changed, 232 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c
> > > > b/drivers/gpu/drm/v3d/v3d_drv.c
> > > > index 3d6b9bcce2f7..bd46396a1ae0 100644
> > > > --- a/drivers/gpu/drm/v3d/v3d_drv.c
> > > > +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> > > > @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct
> > > > drm_device 
> > > 
> > > (...)
> > > 
> > > > @@ -516,9 +536,11 @@
> > > > v3d_attach_fences_and_unlock_reservation(struct
> > > > drm_file *file_priv,
> > > >  					 struct v3d_job *job,
> > > >  					 struct ww_acquire_ctx
> > > > *acquire_ctx,
> > > >  					 u32 out_sync,
> > > > +					 struct v3d_submit_ext *se,
> > > >  					 struct dma_fence *done_fence)
> > > >  {
> > > >  	struct drm_syncobj *sync_out;
> > > > +	bool has_multisync = se && (se->flags & 
> > > 
> > > We always pass the 'se' pointer from a local variable allocated in
> > > the
> > > stack of the caller so it is never going to be NULL.
> > > 
> > > I am happy to keep the NULL check if you want to protect against
> > > future
> > > changes just in case, but if we do that then...
> > > 
> > > > DRM_V3D_EXT_ID_MULTI_SYNC);
> > > >  	int i;
> > > >  
> > > >  	for (i = 0; i < job->bo_count; i++) {
> > > 
> > > (...)
> > > 
> > > > +static void
> > > > +v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
> > > > +{
> > > > +	unsigned int i;
> > > > +
> > > > +	if (!se->out_sync_count)
> > > 
> > > ...we should also check for NULL here for consistency.
> > yes, consistency makes sense here.
> > > Also, I think there is another problem in the code: we always call
> > > v3d_job_cleanup for failed jobs, but only call v3d_job_put for
> > > successful jobs. However, reading the docs for drm_sched_job_init:
> > > 
> > > "Drivers must make sure drm_sched_job_cleanup() if this function
> > > returns successfully, even when @job is aborted before
> > > drm_sched_job_arm() is called."
> > > 
> > > So my understanding is that we should call v3d_job_cleanup instead
> > > of
> > > v3d_job_put for successful jobs or we would be leaking sched
> > > resources
> > > on every successful job, no?
> > 
> > When job_init is successful, v3d_job_cleanup is called by scheduler
> > when
> > job is completed. drm_sched_job_cleanup describes how it works after
> > drm_sched_job_arm:
> > 
> > " After that point of no return @job is committed to be executed by
> > the
> > scheduler, and this function should be called from the
> > &drm_sched_backend_ops.free_job callback."
> > 
> > On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in
> > turn
> > calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it
> > looks
> > ok.
> > 
> > Also, we can say that the very last v3d_job_put() is in charge to
> > decrement refcount initialized (set 1) in v3d_job_init(); while the
> > v3d_job_cleanup() from v3d_sched_job_free() callback decrements
> > refcount
> > that was incremented when v3d_job_push() pushed the job to the
> > scheduler.
> > 
> > So, refcount pairs seem consistent, I mean, get and put. And about
> > drm_sched_job_cleanup, it is explicitly called when job_init fails or
> > after that by the scheduler.
> > 
> 
>    A. Ah ok, thanks for explaining this!
nice!
> 
> With the consistency issue discussed above fixed, for the series:
> 
> Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>

ok, thanks for reviewing and all improvement suggestions,

Melissa
> 
> Iago
> 

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