Claude review: drm/v3d: Clear queue->active_job when v3d_fence_create() fails

Claude review: drm/v3d: Clear queue->active_job when v3d_fence_create() fails

[Claude Code Review Bot]

Patch Review

Good fix. Previously, if `v3d_fence_create()` failed after `queue->active_job` was assigned, the pointer was left dangling. The fix consolidates all early-exit paths into `out_clean_job:` labels.

The BIN path correctly takes `queue->queue_lock` to protect `active_job` against concurrent access from `v3d_overflow_mem_work()`. RENDER/TFU/CSD don't need the lock since their `active_job` has no concurrent reader. This asymmetry is well-explained in the commit message.

No issues.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 00/14] drm/v3d: Scheduler and submission fixes and refactoring

[Maíra Canal]

This series addresses several issues in the v3d scheduler and submission
code. Most of the fixes were motivated by feedback in the vc4 scheduler
series [1], which inherited issues from v3d. Based on the issues found
there, this series addresses the issues in the v3d driver as well.

This series has cleanup patches, fixes, and finally, a refactoring of
the submission code, which allowed us to fix the atomicity of a submission.

- Cleanups and small improvements:
  - PATCH 1/14: "drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h"
  - PATCH 3/14: "drm/v3d: Use inline lock for dma fence initialization"
  - PATCH 4/14: "drm/v3d: Replace spin_lock_irqsave() with spin_lock()"

- Fixes:
  - PATCH 2/14: "drm/v3d: Clear queue->active_job when v3d_fence_create() fails"
  - PATCH 5/14: "drm/v3d: Extract v3d_job_add_syncobjs() helper"
  - PATCH 6/14: "drm/v3d: Reject invalid syncobj handles in submit ioctls"
  - PATCH 13/14: "drm/v3d: Reject invalid out_sync handles in submit ioctls"

- Submission refactoring:
  - PATCH 7/14: "drm/v3d: Migrate BO reservation locking to DRM exec"
  - PATCH 8/14: "drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls"
  - PATCH 9/14: "drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser"
  - PATCH 10/14: "drm/v3d: Convert submit helpers to operate on struct v3d_submit"
  - PATCH 11/14: "drm/v3d: Refactor CPU ioctl into unified submission chain"
  - PATCH 12/14: "drm/v3d: Split BO fence attach from syncobj output handling"
  - PATCH 14/14: "drm/v3d: Ensure atomic submissions in v3d_submit_jobs()" (Also a fix)

During the refactoring, I tried to break the conversion into small steps.
On the one hand, this helps reviewability; on the other hand, some things
may look unfinished at the end of a given commit. I tried to balance the
trade-off, but I'm happy to take split or squash requests during review.

[1] https://lore.kernel.org/dri-devel/20260205-vc4-drm-scheduler-v1-0-c6174fd7bbc1@igalia.com/T/

Best regards,
- Maíra

---
v1 -> v2: https://lore.kernel.org/r/20260413-v3d-sched-misc-fixes-v1-0-bac63a8ceb6c@igalia.com

- [2/14, 3/14, 5/14, 6/14] Add Tvrtko's R-b tag (Tvrtko Ursulin)
- [4/14] Use spin_lock() instead of spin_lock_irq() and change the commit message
         (Tvrtko Ursulin)
- [4/14] Use scoped_guard() instead of open-coding spin_(un)lock()
- [6/14] s/NULL/zero in the comments (Tvrtko Ursulin)
- [8/14] s/kcalloc(1, size, GFP_KERNEL)/kzalloc (Tvrtko Ursulin)
- [8/14] Save one atomic in v3d_attach_perfmon_to_jobs() by adding a conditional
         in the loop (Tvrtko Ursulin)
- [8/14] Zero is implied when initializing the struct v3d_submit (Tvrtko Ursulin)
- [8/14] Create v3d_submit_put_jobs() and v3d_submit_cleanup_jobs() (Tvrtko Ursulin)
- [9/14, 10/14, 11/14] NEW PATCHES: Trying to make the original patch
                       "[PATCH 09/10] drm/v3d: Refactor CPU ioctl into unified
                       submission chain" a bit easier to review.
- [10/14] Wrap drm_exec_fini() in a function v3d_submit_unlock_reservations()
          (Tvrtko Ursulin)
- [12/14] NEW PATCH: "drm/v3d: Split BO fence attach from syncobj output handling"
          - Split v3d_attach_fences_and_unlock_reservation() into different functions
            and fold parts of it into v3d_submit_jobs (Tvrtko Ursulin)
- [13/14] NEW PATCH: "drm/v3d: Reject invalid out_sync handles in submit ioctls"
          - Stop silently ignoring invalid syncobj handles (Tvrtko Ursulin)
- [14/14] Move the error path to a goto
- [14/14] Skip fence attachment and syncobj exportation to user-space if the
          submission has failed (Tvrtko Ursulin)

v2 -> v3: https://lore.kernel.org/r/20260510-v3d-sched-misc-fixes-v2-0-ca4aba343ef6@igalia.com

- Rebased on top of drm-misc-next.
- [1/14] Add Iago's R-b tag (Iago Toral)
- [4/14, 7/14, 9/14] Add Tvrtko's R-b tag (Tvrtko Ursulin)
- [5/14] Create a job->queue variable to be used by v3d_job_add_syncobjs()
- [8/14] v3d_submit_add_job() returns struct v3d_job * instead of using
         void **container as argument (Tvrtko Ursulin)
- [10/14] Pull v3d_submit_lock_reservations() out of
          v3d_setup_csd_jobs_and_bos() (Tvrtko Ursulin)
- [11/14] Assert that jobs[1] is actually a CSD job (Tvrtko Ursulin)
- [11/14] v3d_submit_lock_reservations() is now called unconditionally
          in v3d_submit_cpu_ioctl() (Tvrtko Ursulin)
- [12/14, 13/14] Fold v3d_submit_process_post_deps() and v3d_submit_put_jobs()
                 into v3d_submit_jobs() (Tvrtko Ursulin)

---
Maíra Canal (14):
      drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h
      drm/v3d: Clear queue->active_job when v3d_fence_create() fails
      drm/v3d: Use inline lock for dma fence initialization
      drm/v3d: Replace spin_lock_irqsave() with spin_lock()
      drm/v3d: Extract v3d_job_add_syncobjs() helper
      drm/v3d: Reject invalid syncobj handles in submit ioctls
      drm/v3d: Migrate BO reservation locking to DRM exec
      drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls
      drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser
      drm/v3d: Convert submit helpers to operate on struct v3d_submit
      drm/v3d: Refactor CPU ioctl into unified submission chain
      drm/v3d: Split BO fence attach from syncobj output handling
      drm/v3d: Reject invalid out_sync handles in submit ioctls
      drm/v3d: Ensure atomic submissions in v3d_submit_jobs()

 drivers/gpu/drm/v3d/Kconfig      |   1 +
 drivers/gpu/drm/v3d/v3d_drv.h    |  35 +-
 drivers/gpu/drm/v3d/v3d_fence.c  |   2 +-
 drivers/gpu/drm/v3d/v3d_irq.c    |  17 +-
 drivers/gpu/drm/v3d/v3d_sched.c  |  73 ++--
 drivers/gpu/drm/v3d/v3d_submit.c | 758 +++++++++++++++++++--------------------
 6 files changed, 447 insertions(+), 439 deletions(-)
---
base-commit: 6e7eb171ac96fefa32197afa658e7d9da238be89
change-id: 20260407-v3d-sched-misc-fixes-623739017e53

[PATCH v3 01/14] drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h

[Maíra Canal]

The V3D driver has no display pipeline, so nothing in the driver requires
drm_encoder.h. Remove the stale include.

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 4ebe175a8c6b..5a0b9da2c3aa 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -7,7 +7,6 @@
 #include <linux/spinlock_types.h>
 #include <linux/workqueue.h>
 
-#include <drm/drm_encoder.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/gpu_scheduler.h>

-- 
2.54.0

[PATCH v3 02/14] drm/v3d: Clear queue->active_job when v3d_fence_create() fails

[Maíra Canal]

The run_job() callbacks for BIN, RENDER, TFU and CSD assign the incoming
job to queue->active_job before calling v3d_fence_create(). If
v3d_fence_create() fails, the callback returns NULL without clearing
active_job, leaving a dangling pointer.

Create a failure path in all run_job() callbacks that clears the active
job before returning NULL. The BIN path takes queue->queue_lock around the
clear as it races against v3d_overflow_mem_work(); RENDER, TFU and CSD
paths have no concurrent reader, so the clear is lock-free.

Fixes: a783a09ee76d ("drm/v3d: Refactor job management.")
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c | 52 ++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 0e266b29317f..46f4fc09c59e 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -192,12 +192,8 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
 	struct dma_fence *fence;
 	unsigned long irqflags;
 
-	if (unlikely(job->base.base.s_fence->finished.error)) {
-		spin_lock_irqsave(&queue->queue_lock, irqflags);
-		queue->active_job = NULL;
-		spin_unlock_irqrestore(&queue->queue_lock, irqflags);
-		return NULL;
-	}
+	if (unlikely(job->base.base.s_fence->finished.error))
+		goto out_clean_job;
 
 	/* Lock required around bin_job update vs
 	 * v3d_overflow_mem_work().
@@ -214,7 +210,7 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
 
 	fence = v3d_fence_create(v3d, V3D_BIN);
 	if (IS_ERR(fence))
-		return NULL;
+		goto out_clean_job;
 
 	if (job->base.irq_fence)
 		dma_fence_put(job->base.irq_fence);
@@ -242,6 +238,12 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
 	V3D_CORE_WRITE(0, V3D_CLE_CT0QEA, job->end);
 
 	return fence;
+
+out_clean_job:
+	spin_lock_irqsave(&queue->queue_lock, irqflags);
+	queue->active_job = NULL;
+	spin_unlock_irqrestore(&queue->queue_lock, irqflags);
+	return NULL;
 }
 
 static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
@@ -251,10 +253,8 @@ static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 
-	if (unlikely(job->base.base.s_fence->finished.error)) {
-		v3d->queue[V3D_RENDER].active_job = NULL;
-		return NULL;
-	}
+	if (unlikely(job->base.base.s_fence->finished.error))
+		goto out_clean_job;
 
 	v3d->queue[V3D_RENDER].active_job = &job->base;
 
@@ -268,7 +268,7 @@ static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
 
 	fence = v3d_fence_create(v3d, V3D_RENDER);
 	if (IS_ERR(fence))
-		return NULL;
+		goto out_clean_job;
 
 	if (job->base.irq_fence)
 		dma_fence_put(job->base.irq_fence);
@@ -289,6 +289,10 @@ static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
 	V3D_CORE_WRITE(0, V3D_CLE_CT1QEA, job->end);
 
 	return fence;
+
+out_clean_job:
+	v3d->queue[V3D_RENDER].active_job = NULL;
+	return NULL;
 }
 
 static struct dma_fence *
@@ -299,16 +303,14 @@ v3d_tfu_job_run(struct drm_sched_job *sched_job)
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 
-	if (unlikely(job->base.base.s_fence->finished.error)) {
-		v3d->queue[V3D_TFU].active_job = NULL;
-		return NULL;
-	}
+	if (unlikely(job->base.base.s_fence->finished.error))
+		goto out_clean_job;
 
 	v3d->queue[V3D_TFU].active_job = &job->base;
 
 	fence = v3d_fence_create(v3d, V3D_TFU);
 	if (IS_ERR(fence))
-		return NULL;
+		goto out_clean_job;
 
 	if (job->base.irq_fence)
 		dma_fence_put(job->base.irq_fence);
@@ -336,6 +338,10 @@ v3d_tfu_job_run(struct drm_sched_job *sched_job)
 	V3D_WRITE(V3D_TFU_ICFG(v3d->ver), job->args.icfg | V3D_TFU_ICFG_IOC);
 
 	return fence;
+
+out_clean_job:
+	v3d->queue[V3D_TFU].active_job = NULL;
+	return NULL;
 }
 
 static struct dma_fence *
@@ -347,10 +353,8 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
 	struct dma_fence *fence;
 	int i, csd_cfg0_reg;
 
-	if (unlikely(job->base.base.s_fence->finished.error)) {
-		v3d->queue[V3D_CSD].active_job = NULL;
-		return NULL;
-	}
+	if (unlikely(job->base.base.s_fence->finished.error))
+		goto out_clean_job;
 
 	v3d->queue[V3D_CSD].active_job = &job->base;
 
@@ -358,7 +362,7 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
 
 	fence = v3d_fence_create(v3d, V3D_CSD);
 	if (IS_ERR(fence))
-		return NULL;
+		goto out_clean_job;
 
 	if (job->base.irq_fence)
 		dma_fence_put(job->base.irq_fence);
@@ -385,6 +389,10 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
 	V3D_CORE_WRITE(0, csd_cfg0_reg, job->args.cfg[0]);
 
 	return fence;
+
+out_clean_job:
+	v3d->queue[V3D_CSD].active_job = NULL;
+	return NULL;
 }
 
 static void

-- 
2.54.0

[PATCH v3 03/14] drm/v3d: Use inline lock for dma fence initialization

[Maíra Canal]

Since commit 1f32f310a13c ("dma-buf: inline spinlock for fence protection
v5"), dma_fence_init() accepts a NULL external lock and will fall back to
an inline spinlock embedded in the fence itself. The embedded spinlock
allows the decoupling the lock and the fence lifetime.

Pass NULL so each v3d fence uses its own inline lock. This will allow
queue_lock to use spin_(un)lock() in all its uses instead of
spin_(un)lock_irqsave().

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
index c500136d0455..9b1a882a4c15 100644
--- a/drivers/gpu/drm/v3d/v3d_fence.c
+++ b/drivers/gpu/drm/v3d/v3d_fence.c
@@ -15,7 +15,7 @@ struct dma_fence *v3d_fence_create(struct v3d_dev *v3d, enum v3d_queue q)
 	fence->dev = &v3d->drm;
 	fence->queue = q;
 	fence->seqno = ++queue->emit_seqno;
-	dma_fence_init(&fence->base, &v3d_fence_ops, &queue->queue_lock,
+	dma_fence_init(&fence->base, &v3d_fence_ops, NULL,
 		       queue->fence_context, fence->seqno);
 
 	return &fence->base;

-- 
2.54.0

[PATCH v3 04/14] drm/v3d: Replace spin_lock_irqsave() with spin_lock()

[Maíra Canal]

queue->queue_lock is only acquired from v3d_overflow_mem_work() and
v3d_bin_job_run(), both of which run from workqueue context. The hard
IRQ handler does not take this lock, so disabling interrupts while
holding it is unnecessary.

Drop the spin_lock_irqsave() and use plain spin_(un)lock() instead.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_irq.c   | 17 +++++++----------
 drivers/gpu/drm/v3d/v3d_sched.c | 21 ++++++++++-----------
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 86efaef2722c..754a969b862b 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -47,7 +47,6 @@ v3d_overflow_mem_work(struct work_struct *work)
 	struct v3d_queue_state *queue = &v3d->queue[V3D_BIN];
 	struct v3d_bin_job *bin_job;
 	struct drm_gem_object *obj;
-	unsigned long irqflags;
 
 	if (IS_ERR(bo)) {
 		drm_err(dev, "Couldn't allocate binner overflow mem\n");
@@ -64,18 +63,16 @@ v3d_overflow_mem_work(struct work_struct *work)
 	 * bin job got scheduled, that's fine.  We'll just give them
 	 * some binner pool anyway.
 	 */
-	spin_lock_irqsave(&queue->queue_lock, irqflags);
-	bin_job = (struct v3d_bin_job *)queue->active_job;
+	scoped_guard(spinlock, &queue->queue_lock) {
+		bin_job = (struct v3d_bin_job *)queue->active_job;
 
-	if (!bin_job) {
-		spin_unlock_irqrestore(&queue->queue_lock, irqflags);
-		goto out;
+		if (!bin_job)
+			goto out;
+
+		drm_gem_object_get(obj);
+		list_add_tail(&bo->unref_head, &bin_job->render->unref_list);
 	}
 
-	drm_gem_object_get(obj);
-	list_add_tail(&bo->unref_head, &bin_job->render->unref_list);
-	spin_unlock_irqrestore(&queue->queue_lock, irqflags);
-
 	v3d_mmu_flush_all(v3d);
 
 	V3D_CORE_WRITE(0, V3D_PTB_BPOA, bo->node.start << V3D_MMU_PAGE_SHIFT);
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 46f4fc09c59e..04fd1a365576 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -190,7 +190,6 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
 	struct v3d_queue_state *queue = &v3d->queue[V3D_BIN];
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
-	unsigned long irqflags;
 
 	if (unlikely(job->base.base.s_fence->finished.error))
 		goto out_clean_job;
@@ -198,13 +197,13 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
 	/* Lock required around bin_job update vs
 	 * v3d_overflow_mem_work().
 	 */
-	spin_lock_irqsave(&queue->queue_lock, irqflags);
-	queue->active_job = &job->base;
-	/* Clear out the overflow allocation, so we don't
-	 * reuse the overflow attached to a previous job.
-	 */
-	V3D_CORE_WRITE(0, V3D_PTB_BPOS, 0);
-	spin_unlock_irqrestore(&queue->queue_lock, irqflags);
+	scoped_guard(spinlock, &queue->queue_lock) {
+		queue->active_job = &job->base;
+		/* Clear out the overflow allocation, so we don't
+		 * reuse the overflow attached to a previous job.
+		 */
+		V3D_CORE_WRITE(0, V3D_PTB_BPOS, 0);
+	}
 
 	v3d_invalidate_caches(v3d);
 
@@ -240,9 +239,9 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
 	return fence;
 
 out_clean_job:
-	spin_lock_irqsave(&queue->queue_lock, irqflags);
-	queue->active_job = NULL;
-	spin_unlock_irqrestore(&queue->queue_lock, irqflags);
+	scoped_guard(spinlock, &queue->queue_lock) {
+		queue->active_job = NULL;
+	}
 	return NULL;
 }
 

-- 
2.54.0

[PATCH v3 05/14] drm/v3d: Extract v3d_job_add_syncobjs() helper

[Maíra Canal]

Move the syncobj dependency setup out of v3d_job_init() into its own
v3d_job_add_syncobjs() helper and make the queue that the job was
submitted a variable in struct v3d_job, so that v3d_job_add_syncobjs()
can use it. No functional change.

This prepares for the next commit which changes the error handling, and
for a later consolidation that separates job allocation from syncobj
attachment.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.h    |  3 ++
 drivers/gpu/drm/v3d/v3d_submit.c | 72 ++++++++++++++++++++++++----------------
 2 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 5a0b9da2c3aa..79672abef70d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -294,6 +294,9 @@ struct v3d_job {
 
 	struct v3d_dev *v3d;
 
+	/* The queue that the job was submitted on. */
+	enum v3d_queue queue;
+
 	/* This is the array of BOs that were looked up at the start
 	 * of submission.
 	 */
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 1db43c6a078d..8250376d104c 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -180,17 +180,56 @@ v3d_job_deallocate(void **container)
 	*container = NULL;
 }
 
+static int
+v3d_job_add_syncobjs(struct v3d_job *job, struct drm_file *file_priv,
+		     u32 in_sync, struct v3d_submit_ext *se)
+{
+	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
+	struct v3d_dev *v3d = job->v3d;
+	int ret = 0;
+
+	if (!has_multisync) {
+		ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv,
+							   in_sync, 0);
+		// TODO: Investigate why this was filtered out for the IOCTL.
+		if (ret && ret != -ENOENT)
+			return ret;
+		return 0;
+	}
+
+	if (se->in_sync_count && se->wait_stage == job->queue) {
+		struct drm_v3d_sem __user *handle = u64_to_user_ptr(se->in_syncs);
+
+		for (int i = 0; i < se->in_sync_count; i++) {
+			struct drm_v3d_sem in;
+
+			if (copy_from_user(&in, handle++, sizeof(in))) {
+				drm_dbg(&v3d->drm, "Failed to copy wait dep handle.\n");
+				return -EFAULT;
+			}
+
+			ret = drm_sched_job_add_syncobj_dependency(&job->base,
+								   file_priv, in.handle, 0);
+			// TODO: Investigate why this was filtered out for the IOCTL.
+			if (ret && ret != -ENOENT)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 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, struct v3d_submit_ext *se, enum v3d_queue queue)
 {
 	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
-	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
-	int ret, i;
+	int ret;
 
 	job->v3d = v3d;
 	job->free = free;
+	job->queue = queue;
 	job->file_priv = v3d_priv;
 
 	ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue],
@@ -198,32 +237,9 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 	if (ret)
 		return ret;
 
-	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;
-
-				if (copy_from_user(&in, handle++, sizeof(in))) {
-					ret = -EFAULT;
-					drm_dbg(&v3d->drm, "Failed to copy wait dep handle.\n");
-					goto fail_job_init;
-				}
-				ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in.handle, 0);
-
-				// TODO: Investigate why this was filtered out for the IOCTL.
-				if (ret && ret != -ENOENT)
-					goto fail_job_init;
-			}
-		}
-	} else {
-		ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in_sync, 0);
-
-		// TODO: Investigate why this was filtered out for the IOCTL.
-		if (ret && ret != -ENOENT)
-			goto fail_job_init;
-	}
+	ret = v3d_job_add_syncobjs(job, file_priv, in_sync, se);
+	if (ret)
+		goto fail_job_init;
 
 	/* CPU jobs don't require hardware resources */
 	if (queue != V3D_CPU) {

-- 
2.54.0

[PATCH v3 06/14] drm/v3d: Reject invalid syncobj handles in submit ioctls

[Maíra Canal]

drm_sched_job_add_syncobj_dependency() returns -ENOENT both when the
handle is zero and when the handle is non-zero but does not find a
corresponding existing syncobj (userspace bug). The driver previously
ignored -ENOENT in both cases, silently accepting broken handles.

Distinguish the two: skip the call entirely when the handle is zero, as
there is no dependency, and let -ENOENT propagate for non-zero handles
that don't resolve, turning the error into a proper return to userspace.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_submit.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 8250376d104c..0babe2e67266 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -189,12 +189,11 @@ v3d_job_add_syncobjs(struct v3d_job *job, struct drm_file *file_priv,
 	int ret = 0;
 
 	if (!has_multisync) {
-		ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv,
-							   in_sync, 0);
-		// TODO: Investigate why this was filtered out for the IOCTL.
-		if (ret && ret != -ENOENT)
-			return ret;
-		return 0;
+		/* Ignore syncobj if its handle is zero */
+		if (in_sync)
+			ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv,
+								   in_sync, 0);
+		return ret;
 	}
 
 	if (se->in_sync_count && se->wait_stage == job->queue) {
@@ -208,11 +207,13 @@ v3d_job_add_syncobjs(struct v3d_job *job, struct drm_file *file_priv,
 				return -EFAULT;
 			}
 
-			ret = drm_sched_job_add_syncobj_dependency(&job->base,
-								   file_priv, in.handle, 0);
-			// TODO: Investigate why this was filtered out for the IOCTL.
-			if (ret && ret != -ENOENT)
-				return ret;
+			/* Ignore syncobj if its handle is zero */
+			if (in.handle) {
+				ret = drm_sched_job_add_syncobj_dependency(&job->base,
+									   file_priv, in.handle, 0);
+				if (ret)
+					return ret;
+			}
 		}
 	}
 

-- 
2.54.0

[PATCH v3 07/14] drm/v3d: Migrate BO reservation locking to DRM exec

[Maíra Canal]

Replace the drm_gem_(un)lock_reservations() + ww_acquire_ctx pattern with
DRM exec across all submit ioctls. Just a straightforward conversion; no
functional change.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/Kconfig      |  1 +
 drivers/gpu/drm/v3d/v3d_drv.h    |  3 +-
 drivers/gpu/drm/v3d/v3d_submit.c | 69 +++++++++++++++++-----------------------
 3 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/v3d/Kconfig b/drivers/gpu/drm/v3d/Kconfig
index ce62c5908e1d..6a33e0ab30de 100644
--- a/drivers/gpu/drm/v3d/Kconfig
+++ b/drivers/gpu/drm/v3d/Kconfig
@@ -5,6 +5,7 @@ config DRM_V3D
 	depends on DRM
 	depends on COMMON_CLK
 	depends on MMU
+	select DRM_EXEC
 	select DRM_SCHED
 	select DRM_GEM_SHMEM_HELPER
 	help
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 79672abef70d..07bbdf52c33c 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -7,6 +7,7 @@
 #include <linux/spinlock_types.h>
 #include <linux/workqueue.h>
 
+#include <drm/drm_exec.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/gpu_scheduler.h>
@@ -421,7 +422,7 @@ struct v3d_indirect_csd_info {
 	struct drm_gem_object *indirect;
 
 	/* Context of the Indirect CSD job */
-	struct ww_acquire_ctx acquire_ctx;
+	struct drm_exec exec;
 };
 
 struct v3d_timestamp_query_info {
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 0babe2e67266..d80afbf2c03b 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -20,20 +20,19 @@
  * to v3d, so we don't attach dma-buf fences to them.
  */
 static int
-v3d_lock_bo_reservations(struct v3d_job *job,
-			 struct ww_acquire_ctx *acquire_ctx)
+v3d_lock_bo_reservations(struct v3d_job *job, struct drm_exec *exec)
 {
 	int i, ret;
 
-	ret = drm_gem_lock_reservations(job->bo, job->bo_count, acquire_ctx);
+	drm_exec_init(exec, DRM_EXEC_INTERRUPTIBLE_WAIT, job->bo_count);
+	drm_exec_until_all_locked(exec) {
+		ret = drm_exec_prepare_array(exec, job->bo, job->bo_count, 1);
+	}
+
 	if (ret)
-		return ret;
+		goto fail;
 
 	for (i = 0; i < job->bo_count; i++) {
-		ret = dma_resv_reserve_fences(job->bo[i]->resv, 1);
-		if (ret)
-			goto fail;
-
 		ret = drm_sched_job_add_implicit_dependencies(&job->base,
 							      job->bo[i], true);
 		if (ret)
@@ -43,7 +42,7 @@ v3d_lock_bo_reservations(struct v3d_job *job,
 	return 0;
 
 fail:
-	drm_gem_unlock_reservations(job->bo, job->bo_count, acquire_ctx);
+	drm_exec_fini(exec);
 	return ret;
 }
 
@@ -278,7 +277,7 @@ v3d_push_job(struct v3d_job *job)
 static void
 v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 					 struct v3d_job *job,
-					 struct ww_acquire_ctx *acquire_ctx,
+					 struct drm_exec *exec,
 					 u32 out_sync,
 					 struct v3d_submit_ext *se,
 					 struct dma_fence *done_fence)
@@ -293,7 +292,7 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 				   DMA_RESV_USAGE_WRITE);
 	}
 
-	drm_gem_unlock_reservations(job->bo, job->bo_count, acquire_ctx);
+	drm_exec_fini(exec);
 
 	/* Update the return sync object for the job */
 	/* If it only supports a single signal semaphore*/
@@ -324,7 +323,7 @@ v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
 			   struct v3d_csd_job **job,
 			   struct v3d_job **clean_job,
 			   struct v3d_submit_ext *se,
-			   struct ww_acquire_ctx *acquire_ctx)
+			   struct drm_exec *exec)
 {
 	int ret;
 
@@ -357,7 +356,7 @@ v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
 	if (ret)
 		return ret;
 
-	return v3d_lock_bo_reservations(*clean_job, acquire_ctx);
+	return v3d_lock_bo_reservations(*clean_job, exec);
 }
 
 static void
@@ -515,7 +514,7 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
 
 	return v3d_setup_csd_jobs_and_bos(file_priv, v3d, &indirect_csd.submit,
 					  &info->job, &info->clean_job,
-					  NULL, &info->acquire_ctx);
+					  NULL, &info->exec);
 }
 
 /* Get data for the query timestamp job submission. */
@@ -930,7 +929,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	struct v3d_render_job *render = NULL;
 	struct v3d_job *clean_job = NULL;
 	struct v3d_job *last_job;
-	struct ww_acquire_ctx acquire_ctx;
+	struct drm_exec exec;
 	int ret = 0;
 
 	trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args->rcl_end);
@@ -1010,7 +1009,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_lock_bo_reservations(last_job, &acquire_ctx);
+	ret = v3d_lock_bo_reservations(last_job, &exec);
 	if (ret)
 		goto fail;
 
@@ -1059,7 +1058,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 
 	v3d_attach_fences_and_unlock_reservation(file_priv,
 						 last_job,
-						 &acquire_ctx,
+						 &exec,
 						 args->out_sync,
 						 &se,
 						 last_job->done_fence);
@@ -1073,8 +1072,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 fail_unreserve:
 	mutex_unlock(&v3d->sched_lock);
 fail_perfmon:
-	drm_gem_unlock_reservations(last_job->bo,
-				    last_job->bo_count, &acquire_ctx);
+	drm_exec_fini(&exec);
 fail:
 	v3d_job_cleanup((void *)bin);
 	v3d_job_cleanup((void *)render);
@@ -1101,7 +1099,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	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;
+	struct drm_exec exec;
 	int ret = 0;
 
 	trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);
@@ -1157,7 +1155,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 		job->base.bo[job->base.bo_count] = bo;
 	}
 
-	ret = v3d_lock_bo_reservations(&job->base, &acquire_ctx);
+	ret = v3d_lock_bo_reservations(&job->base, &exec);
 	if (ret)
 		goto fail;
 
@@ -1166,7 +1164,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	mutex_unlock(&v3d->sched_lock);
 
 	v3d_attach_fences_and_unlock_reservation(file_priv,
-						 &job->base, &acquire_ctx,
+						 &job->base, &exec,
 						 args->out_sync,
 						 &se,
 						 job->base.done_fence);
@@ -1201,7 +1199,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *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;
+	struct drm_exec exec;
 	int ret;
 
 	trace_v3d_submit_csd_ioctl(&v3d->drm, args->cfg[5], args->cfg[6]);
@@ -1228,8 +1226,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	}
 
 	ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
-					 &job, &clean_job, &se,
-					 &acquire_ctx);
+					 &job, &clean_job, &se, &exec);
 	if (ret)
 		goto fail;
 
@@ -1260,7 +1257,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 
 	v3d_attach_fences_and_unlock_reservation(file_priv,
 						 clean_job,
-						 &acquire_ctx,
+						 &exec,
 						 args->out_sync,
 						 &se,
 						 clean_job->done_fence);
@@ -1273,8 +1270,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 fail_unreserve:
 	mutex_unlock(&v3d->sched_lock);
 fail_perfmon:
-	drm_gem_unlock_reservations(clean_job->bo, clean_job->bo_count,
-				    &acquire_ctx);
+	drm_exec_fini(&exec);
 fail:
 	v3d_job_cleanup((void *)job);
 	v3d_job_cleanup(clean_job);
@@ -1312,7 +1308,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 	struct v3d_cpu_job *cpu_job = NULL;
 	struct v3d_csd_job *csd_job = NULL;
 	struct v3d_job *clean_job = NULL;
-	struct ww_acquire_ctx acquire_ctx;
+	struct drm_exec exec;
 	int ret;
 
 	if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {
@@ -1363,7 +1359,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 		if (ret)
 			goto fail;
 
-		ret = v3d_lock_bo_reservations(&cpu_job->base, &acquire_ctx);
+		ret = v3d_lock_bo_reservations(&cpu_job->base, &exec);
 		if (ret)
 			goto fail;
 	}
@@ -1397,14 +1393,14 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 
 	v3d_attach_fences_and_unlock_reservation(file_priv,
 						 &cpu_job->base,
-						 &acquire_ctx, 0,
+						 &exec, 0,
 						 out_se, cpu_job->base.done_fence);
 
 	switch (cpu_job->job_type) {
 	case V3D_CPU_JOB_TYPE_INDIRECT_CSD:
 		v3d_attach_fences_and_unlock_reservation(file_priv,
 							 clean_job,
-							 &cpu_job->indirect_csd.acquire_ctx,
+							 &cpu_job->indirect_csd.exec,
 							 0, &se, clean_job->done_fence);
 		break;
 	default:
@@ -1419,13 +1415,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 
 fail_unreserve:
 	mutex_unlock(&v3d->sched_lock);
-
-	drm_gem_unlock_reservations(cpu_job->base.bo, cpu_job->base.bo_count,
-				    &acquire_ctx);
-
-	drm_gem_unlock_reservations(clean_job->bo, clean_job->bo_count,
-				    &cpu_job->indirect_csd.acquire_ctx);
-
+	drm_exec_fini(&exec);
+	drm_exec_fini(&cpu_job->indirect_csd.exec);
 fail:
 	v3d_job_cleanup((void *)cpu_job);
 	v3d_job_cleanup((void *)csd_job);

-- 
2.54.0

[PATCH v3 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls

[Maíra Canal]

As the V3D driver grew with time, different types of submission were added
and the submission code grew more complex, but the driver stuck with the
same abstractions.

Nowadays, the submission ioctls don't submit a single job, but a
chain of jobs:

1. v3d_submit_cl_ioctl() submits a BIN job (optional), RENDER job
   (mandatory), and a CLEAN_CACHE job (optional).
2. v3d_submit_csd_ioctl() submits a CSD, and a CLEAN_CACHE job.
3. v3d_submit_tfu_ioctl() submits a TFU job.

Therefore, each ioctl submits a chain of jobs in which each job depends on
the previous one. However, this concept is not well represented in software
at the moment.

To address this, introduce a new concept: the struct v3d_submit, which
groups the submission state and represents the submission chain formed by
an ordered array of jobs.

Add new helpers to allocate, add jobs to the chain and submit jobs to
the scheduler, all based on the new struct. Convert v3d_submit_cl_ioctl(),
v3d_submit_tfu_ioctl() and v3d_submit_csd_ioctl() to the new pattern. Each
ioctl now follows the same flow: add jobs -> attach perfmon -> lookup BOs
-> lock reservations -> submit chain -> attach fences -> put jobs.

The CPU ioctl is left on the old helpers for now; its indirect CSD path
requires some restructuring that will be addressed in the next few
commits.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.h    |  21 +++
 drivers/gpu/drm/v3d/v3d_submit.c | 386 ++++++++++++++++++++++-----------------
 2 files changed, 241 insertions(+), 166 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 07bbdf52c33c..fae0c53e7aa3 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -288,6 +288,27 @@ to_v3d_fence(struct dma_fence *fence)
 #define V3D_CORE_READ(core, offset) readl(v3d->core_regs[core] + offset)
 #define V3D_CORE_WRITE(core, offset, val) writel(val, v3d->core_regs[core] + offset)
 
+#define V3D_MAX_JOBS_PER_SUBMISSION 3
+
+/* Per-ioctl submission context */
+struct v3d_submit {
+	struct v3d_dev *v3d;
+
+	struct drm_file *file_priv;
+
+	/* DRM exec context for this submission. */
+	struct drm_exec exec;
+
+	/* Ordered array of jobs forming the submission chain. Jobs are
+	 * appended via v3d_submit_add_job(), then chained and pushed to
+	 * the scheduler by v3d_submit_jobs().
+	 */
+	struct v3d_job *jobs[V3D_MAX_JOBS_PER_SUBMISSION];
+
+	/* Number of jobs currently in @jobs. */
+	u32 job_count;
+};
+
 struct v3d_job {
 	struct drm_sched_job base;
 
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index d80afbf2c03b..fcaf3a6cfddc 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -261,17 +261,105 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 	return ret;
 }
 
-static void
-v3d_push_job(struct v3d_job *job)
+static const struct {
+	size_t size;
+	void (*free)(struct kref *ref);
+} v3d_job_types[] = {
+	[V3D_BIN]		= { sizeof(struct v3d_bin_job), v3d_job_free },
+	[V3D_RENDER]		= { sizeof(struct v3d_render_job), v3d_render_job_free },
+	[V3D_TFU]		= { sizeof(struct v3d_tfu_job), v3d_job_free },
+	[V3D_CSD]		= { sizeof(struct v3d_csd_job), v3d_job_free },
+	[V3D_CACHE_CLEAN]	= { sizeof(struct v3d_job), v3d_job_free },
+	[V3D_CPU]		= { sizeof(struct v3d_cpu_job), v3d_cpu_job_free },
+};
+
+static struct v3d_job *
+v3d_submit_add_job(struct v3d_submit *submit, enum v3d_queue queue)
 {
-	drm_sched_job_arm(&job->base);
+	struct v3d_file_priv *v3d_priv = submit->file_priv->driver_priv;
+	struct v3d_dev *v3d = submit->v3d;
+	struct v3d_job *job;
+	int ret;
 
-	job->done_fence = dma_fence_get(&job->base.s_fence->finished);
+	if (queue >= V3D_MAX_QUEUES)
+		return ERR_PTR(-EINVAL);
 
-	/* put by scheduler job completion */
-	kref_get(&job->refcount);
+	job = kzalloc(v3d_job_types[queue].size, GFP_KERNEL);
+	if (!job)
+		return ERR_PTR(-ENOMEM);
 
-	drm_sched_entity_push_job(&job->base);
+	job->v3d = v3d;
+	job->queue = queue;
+	job->file_priv = v3d_priv;
+	job->free = v3d_job_types[queue].free;
+
+	ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue],
+				 1, v3d_priv, submit->file_priv->client_id);
+	if (ret)
+		goto fail_free;
+
+	/* CPU jobs don't require hardware resources */
+	if (queue != V3D_CPU) {
+		ret = v3d_pm_runtime_get(v3d);
+		if (ret)
+			goto fail_sched_job;
+		job->has_pm_ref = true;
+	}
+
+	kref_init(&job->refcount);
+
+	job->client_stats = v3d_stats_get(v3d_priv->stats[queue]);
+	job->global_stats = v3d_stats_get(v3d->queue[queue].stats);
+
+	submit->jobs[submit->job_count++] = job;
+
+	return job;
+
+fail_sched_job:
+	drm_sched_job_cleanup(&job->base);
+fail_free:
+	kfree(job);
+	return ERR_PTR(ret);
+}
+
+static void
+v3d_submit_put_jobs(struct v3d_submit *submit)
+{
+	for (int i = 0; i < submit->job_count; i++)
+		v3d_job_put(submit->jobs[i]);
+}
+
+static void
+v3d_submit_cleanup_jobs(struct v3d_submit *submit)
+{
+	for (int i = 0; i < submit->job_count; i++)
+		v3d_job_cleanup(submit->jobs[i]);
+}
+
+static int
+v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, u32 perfmon_id)
+{
+	struct v3d_file_priv *v3d_priv = submit->file_priv->driver_priv;
+	struct v3d_dev *v3d = submit->v3d;
+	struct v3d_perfmon *perfmon;
+
+	if (!perfmon_id)
+		return 0;
+
+	if (v3d->global_perfmon)
+		return -EAGAIN;
+
+	perfmon = v3d_perfmon_find(v3d_priv, perfmon_id);
+	if (!perfmon)
+		return -ENOENT;
+
+	for (int i = 0; i < submit->job_count; i++) {
+		submit->jobs[i]->perfmon = perfmon;
+		if (i != 0)
+			v3d_perfmon_get(perfmon);
+	}
+
+	return 0;
 }
 
 static void
@@ -316,6 +404,45 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 	}
 }
 
+static void
+v3d_push_job(struct v3d_job *job)
+{
+	drm_sched_job_arm(&job->base);
+
+	job->done_fence = dma_fence_get(&job->base.s_fence->finished);
+
+	/* put by scheduler job completion */
+	kref_get(&job->refcount);
+
+	drm_sched_entity_push_job(&job->base);
+}
+
+static int
+v3d_submit_jobs(struct v3d_submit *submit)
+{
+	struct v3d_dev *v3d = submit->v3d;
+	int ret = 0;
+
+	mutex_lock(&v3d->sched_lock);
+
+	for (int i = 0; i < submit->job_count; i++) {
+		struct v3d_job *job = submit->jobs[i];
+
+		v3d_push_job(job);
+
+		if (i + 1 < submit->job_count) {
+			ret = drm_sched_job_add_dependency(&submit->jobs[i + 1]->base,
+							   dma_fence_get(job->done_fence));
+			if (ret)
+				goto err;
+		}
+	}
+
+err:
+	mutex_unlock(&v3d->sched_lock);
+	return ret;
+}
+
 static int
 v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
 			   struct v3d_dev *v3d,
@@ -921,18 +1048,15 @@ int
 v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 		    struct drm_file *file_priv)
 {
-	struct v3d_dev *v3d = to_v3d_dev(dev);
-	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
+	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_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;
-	struct v3d_job *last_job;
-	struct drm_exec exec;
-	int ret = 0;
+	int ret;
 
-	trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args->rcl_end);
+	trace_v3d_submit_cl_ioctl(dev, args->rcl_start, args->rcl_end);
 
 	if (args->pad)
 		return -EINVAL;
@@ -952,30 +1076,10 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 		}
 	}
 
-	ret = v3d_job_allocate(v3d, (void *)&render, sizeof(*render));
-	if (ret)
-		return ret;
-
-	ret = v3d_job_init(v3d, file_priv, &render->base,
-			   v3d_render_job_free, args->in_sync_rcl, &se, V3D_RENDER);
-	if (ret) {
-		v3d_job_deallocate((void *)&render);
-		goto fail;
-	}
-
-	render->start = args->rcl_start;
-	render->end = args->rcl_end;
-	INIT_LIST_HEAD(&render->unref_list);
-
 	if (args->bcl_start != args->bcl_end) {
-		ret = v3d_job_allocate(v3d, (void *)&bin, sizeof(*bin));
-		if (ret)
-			goto fail;
-
-		ret = v3d_job_init(v3d, file_priv, &bin->base,
-				   v3d_job_free, args->in_sync_bcl, &se, V3D_BIN);
-		if (ret) {
-			v3d_job_deallocate((void *)&bin);
+		bin = (struct v3d_bin_job *) v3d_submit_add_job(&submit, V3D_BIN);
+		if (IS_ERR(bin)) {
+			ret = PTR_ERR(bin);
 			goto fail;
 		}
 
@@ -984,99 +1088,71 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 		bin->qma = args->qma;
 		bin->qms = args->qms;
 		bin->qts = args->qts;
-		bin->render = render;
-	}
 
-	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
-		ret = v3d_job_allocate(v3d, (void *)&clean_job, sizeof(*clean_job));
+		ret = v3d_job_add_syncobjs(&bin->base, file_priv, args->in_sync_bcl,
+					   &se);
 		if (ret)
 			goto fail;
-
-		ret = v3d_job_init(v3d, file_priv, clean_job,
-				   v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
-		if (ret) {
-			v3d_job_deallocate((void *)&clean_job);
-			goto fail;
-		}
-
-		last_job = clean_job;
-	} else {
-		last_job = &render->base;
 	}
 
-	ret = v3d_lookup_bos(dev, file_priv, last_job,
+	render = (struct v3d_render_job *) v3d_submit_add_job(&submit, V3D_RENDER);
+	if (IS_ERR(render)) {
+		ret = PTR_ERR(render);
+		goto fail;
+	}
+
+	INIT_LIST_HEAD(&render->unref_list);
+	render->start = args->rcl_start;
+	render->end = args->rcl_end;
+
+	if (bin)
+		bin->render = render;
+
+	ret = v3d_job_add_syncobjs(&render->base, file_priv, args->in_sync_rcl, &se);
+	if (ret)
+		goto fail;
+
+	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
+		clean_job = v3d_submit_add_job(&submit, V3D_CACHE_CLEAN);
+		if (IS_ERR(clean_job)) {
+			ret = PTR_ERR(clean_job);
+			goto fail;
+		}
+	}
+
+	ret = v3d_attach_perfmon_to_jobs(&submit, args->perfmon_id);
+	if (ret)
+		goto fail;
+
+	ret = v3d_lookup_bos(dev, file_priv,
+			     submit.jobs[submit.job_count - 1],
 			     args->bo_handles, args->bo_handle_count);
 	if (ret)
 		goto fail;
 
-	ret = v3d_lock_bo_reservations(last_job, &exec);
+	ret = v3d_lock_bo_reservations(submit.jobs[submit.job_count - 1],
+				       &submit.exec);
 	if (ret)
 		goto fail;
 
-	if (args->perfmon_id) {
-		if (v3d->global_perfmon) {
-			ret = -EAGAIN;
-			goto fail_perfmon;
-		}
-
-		render->base.perfmon = v3d_perfmon_find(v3d_priv,
-							args->perfmon_id);
-
-		if (!render->base.perfmon) {
-			ret = -ENOENT;
-			goto fail_perfmon;
-		}
-	}
-
-	mutex_lock(&v3d->sched_lock);
-	if (bin) {
-		bin->base.perfmon = render->base.perfmon;
-		v3d_perfmon_get(bin->base.perfmon);
-		v3d_push_job(&bin->base);
-
-		ret = drm_sched_job_add_dependency(&render->base.base,
-						   dma_fence_get(bin->base.done_fence));
-		if (ret)
-			goto fail_unreserve;
-	}
-
-	v3d_push_job(&render->base);
-
-	if (clean_job) {
-		struct dma_fence *render_fence =
-			dma_fence_get(render->base.done_fence);
-		ret = drm_sched_job_add_dependency(&clean_job->base,
-						   render_fence);
-		if (ret)
-			goto fail_unreserve;
-		clean_job->perfmon = render->base.perfmon;
-		v3d_perfmon_get(clean_job->perfmon);
-		v3d_push_job(clean_job);
-	}
-
-	mutex_unlock(&v3d->sched_lock);
+	ret = v3d_submit_jobs(&submit);
+	if (ret)
+		goto fail_unreserve;
 
 	v3d_attach_fences_and_unlock_reservation(file_priv,
-						 last_job,
-						 &exec,
-						 args->out_sync,
-						 &se,
-						 last_job->done_fence);
+						 submit.jobs[submit.job_count - 1],
+						 &submit.exec,
+						 args->out_sync, &se,
+						 submit.jobs[submit.job_count - 1]->done_fence);
 
-	v3d_job_put(&bin->base);
-	v3d_job_put(&render->base);
-	v3d_job_put(clean_job);
+	v3d_submit_put_jobs(&submit);
 
 	return 0;
 
 fail_unreserve:
-	mutex_unlock(&v3d->sched_lock);
-fail_perfmon:
-	drm_exec_fini(&exec);
+	drm_exec_fini(&submit.exec);
 fail:
-	v3d_job_cleanup((void *)bin);
-	v3d_job_cleanup((void *)render);
-	v3d_job_cleanup(clean_job);
+	v3d_submit_cleanup_jobs(&submit);
 	v3d_put_multisync_post_deps(&se);
 
 	return ret;
@@ -1095,14 +1171,13 @@ int
 v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *file_priv)
 {
-	struct v3d_dev *v3d = to_v3d_dev(dev);
+	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
 	struct drm_v3d_submit_tfu *args = data;
 	struct v3d_submit_ext se = {0};
 	struct v3d_tfu_job *job = NULL;
-	struct drm_exec exec;
 	int ret = 0;
 
-	trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);
+	trace_v3d_submit_tfu_ioctl(dev, args->iia);
 
 	if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {
 		drm_dbg(dev, "invalid flags: %d\n", args->flags);
@@ -1117,17 +1192,16 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 		}
 	}
 
-	ret = v3d_job_allocate(v3d, (void *)&job, sizeof(*job));
-	if (ret)
-		return ret;
-
-	ret = v3d_job_init(v3d, file_priv, &job->base,
-			   v3d_job_free, args->in_sync, &se, V3D_TFU);
-	if (ret) {
-		v3d_job_deallocate((void *)&job);
+	job = (struct v3d_tfu_job *) v3d_submit_add_job(&submit, V3D_TFU);
+	if (IS_ERR(job)) {
+		ret = PTR_ERR(job);
 		goto fail;
 	}
 
+	ret = v3d_job_add_syncobjs(&job->base, file_priv, args->in_sync, &se);
+	if (ret)
+		goto fail;
+
 	job->base.bo = kzalloc_objs(*job->base.bo, ARRAY_SIZE(args->bo_handles));
 	if (!job->base.bo) {
 		ret = -ENOMEM;
@@ -1155,26 +1229,27 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 		job->base.bo[job->base.bo_count] = bo;
 	}
 
-	ret = v3d_lock_bo_reservations(&job->base, &exec);
+	ret = v3d_lock_bo_reservations(&job->base, &submit.exec);
 	if (ret)
 		goto fail;
 
-	mutex_lock(&v3d->sched_lock);
-	v3d_push_job(&job->base);
-	mutex_unlock(&v3d->sched_lock);
+	ret = v3d_submit_jobs(&submit);
+	if (ret)
+		goto fail_unreserve;
 
 	v3d_attach_fences_and_unlock_reservation(file_priv,
-						 &job->base, &exec,
-						 args->out_sync,
-						 &se,
+						 &job->base, &submit.exec,
+						 args->out_sync, &se,
 						 job->base.done_fence);
 
-	v3d_job_put(&job->base);
+	v3d_submit_put_jobs(&submit);
 
 	return 0;
 
+fail_unreserve:
+	drm_exec_fini(&submit.exec);
 fail:
-	v3d_job_cleanup((void *)job);
+	v3d_submit_cleanup_jobs(&submit);
 	v3d_put_multisync_post_deps(&se);
 
 	return ret;
@@ -1193,21 +1268,19 @@ int
 v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *file_priv)
 {
-	struct v3d_dev *v3d = to_v3d_dev(dev);
-	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
+	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_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 drm_exec exec;
 	int ret;
 
-	trace_v3d_submit_csd_ioctl(&v3d->drm, args->cfg[5], args->cfg[6]);
+	trace_v3d_submit_csd_ioctl(dev, args->cfg[5], args->cfg[6]);
 
 	if (args->pad)
 		return -EINVAL;
 
-	if (!v3d_has_csd(v3d)) {
+	if (!v3d_has_csd(submit.v3d)) {
 		drm_warn(dev, "Attempting CSD submit on non-CSD hardware\n");
 		return -EINVAL;
 	}
@@ -1225,55 +1298,36 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 		}
 	}
 
-	ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
-					 &job, &clean_job, &se, &exec);
+	ret = v3d_setup_csd_jobs_and_bos(file_priv, submit.v3d, args,
+					 &job, &clean_job, &se, &submit.exec);
 	if (ret)
 		goto fail;
 
-	if (args->perfmon_id) {
-		if (v3d->global_perfmon) {
-			ret = -EAGAIN;
-			goto fail_perfmon;
-		}
+	submit.jobs[submit.job_count++] = &job->base;
+	submit.jobs[submit.job_count++] = clean_job;
 
-		job->base.perfmon = v3d_perfmon_find(v3d_priv,
-						     args->perfmon_id);
-		if (!job->base.perfmon) {
-			ret = -ENOENT;
-			goto fail_perfmon;
-		}
-	}
-
-	mutex_lock(&v3d->sched_lock);
-	v3d_push_job(&job->base);
-
-	ret = drm_sched_job_add_dependency(&clean_job->base,
-					   dma_fence_get(job->base.done_fence));
+	ret = v3d_attach_perfmon_to_jobs(&submit, args->perfmon_id);
 	if (ret)
 		goto fail_unreserve;
 
-	v3d_push_job(clean_job);
-	mutex_unlock(&v3d->sched_lock);
+	ret = v3d_submit_jobs(&submit);
+	if (ret)
+		goto fail_unreserve;
 
 	v3d_attach_fences_and_unlock_reservation(file_priv,
 						 clean_job,
-						 &exec,
-						 args->out_sync,
-						 &se,
+						 &submit.exec,
+						 args->out_sync, &se,
 						 clean_job->done_fence);
 
-	v3d_job_put(&job->base);
-	v3d_job_put(clean_job);
+	v3d_submit_put_jobs(&submit);
 
 	return 0;
 
 fail_unreserve:
-	mutex_unlock(&v3d->sched_lock);
-fail_perfmon:
-	drm_exec_fini(&exec);
+	drm_exec_fini(&submit.exec);
 fail:
-	v3d_job_cleanup((void *)job);
-	v3d_job_cleanup(clean_job);
+	v3d_submit_cleanup_jobs(&submit);
 	v3d_put_multisync_post_deps(&se);
 
 	return ret;

-- 
2.54.0

[PATCH v3 09/14] drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser

[Maíra Canal]

v3d_get_cpu_indirect_csd_params() currently does double duty: it parses
the indirect CSD extension and, while still inside the extension parser,
also creates the CSD/clean jobs and locks their BOs through a separate
DRM exec context. This nested submission deviates from the standard flow
and makes it hard to fold the indirect CSD path into the unified submit
chain.

Stash the parsed drm_v3d_submit_csd args in struct v3d_indirect_csd_info
and have the parser only fill in the parameters. Then, move job creation
(v3d_setup_csd_jobs_and_bos()) into v3d_submit_cpu_ioctl(), where is the
proper place to create jobs.

No functional change, but prepares to move the CPU ioctl into the
unified submission chain.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.h    |  5 +++++
 drivers/gpu/drm/v3d/v3d_submit.c | 16 +++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index fae0c53e7aa3..52ed0af67f5e 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -428,6 +428,11 @@ struct v3d_indirect_csd_info {
 	/* Clean cache job associated to the Indirect CSD job */
 	struct v3d_job *clean_job;
 
+	/* Indirect CSD args, stashed by the extension parser and later used
+	 * to create the CSD job from them.
+	 */
+	struct drm_v3d_submit_csd args;
+
 	/* Offset within the BO where the workgroup counts are stored */
 	u32 offset;
 
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index fcaf3a6cfddc..64eba912dc64 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -632,6 +632,7 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
 	}
 
 	job->job_type = V3D_CPU_JOB_TYPE_INDIRECT_CSD;
+	info->args = indirect_csd.submit;
 	info->offset = indirect_csd.offset;
 	info->wg_size = indirect_csd.wg_size;
 	memcpy(&info->wg_uniform_offsets, &indirect_csd.wg_uniform_offsets,
@@ -639,9 +640,7 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
 
 	info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);
 
-	return v3d_setup_csd_jobs_and_bos(file_priv, v3d, &indirect_csd.submit,
-					  &info->job, &info->clean_job,
-					  NULL, &info->exec);
+	return 0;
 }
 
 /* Get data for the query timestamp job submission. */
@@ -1404,6 +1403,17 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 		goto fail;
 	}
 
+	if (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) {
+		ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d,
+						 &cpu_job->indirect_csd.args,
+						 &cpu_job->indirect_csd.job,
+						 &cpu_job->indirect_csd.clean_job,
+						 NULL,
+						 &cpu_job->indirect_csd.exec);
+		if (ret)
+			goto fail;
+	}
+
 	clean_job = cpu_job->indirect_csd.clean_job;
 	csd_job = cpu_job->indirect_csd.job;
 

-- 
2.54.0

[PATCH v3 10/14] drm/v3d: Convert submit helpers to operate on struct v3d_submit

[Maíra Canal]

Generalize the submission helpers so they act on a whole struct v3d_submit
(the entire job chain) rather than on individual jobs and a drm_exec. This
lets a submission of several chained jobs be locked, fenced, and finalized
as a single unit, and is the groundwork for collapsing the indirect CSD
path into one chain.

The following helpers were generalized:

  - v3d_lookup_bos()
  - v3d_lock_bo_reservations() (renamed to v3d_submit_lock_reservations()):
  - v3d_attach_fences_and_unlock_reservation()
  - v3d_setup_csd_jobs_and_bos()

Now, the locking helper now iterates over all jobs and locks the union of
their BOs under one DRM exec, using DRM_EXEC_IGNORE_DUPLICATES to tolerate
shared BO references. The fence-attach helper similarly walks every job
and attaches the chain's last fence to all touched BOs.

Also, v3d_submit_jobs() becomes the single submit-and-finalize entry
point and callers no longer need to open-code fence attachment,
reservation unlocking, etc.

Update CL/TFU/CSD/CPU ioctls to use the new helper signatures. The CPU
ioctl still uses two struct v3d_submit instances (one for the CPU job,
one for the indirect CSD jobs) and keeps its manual two-pass fence-attach
flow. Converting the indirect CSD path into the unified chain is done in
the next commit.

No functional change.

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

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 64eba912dc64..62c23feb8fbb 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -20,32 +20,51 @@
  * to v3d, so we don't attach dma-buf fences to them.
  */
 static int
-v3d_lock_bo_reservations(struct v3d_job *job, struct drm_exec *exec)
+v3d_submit_lock_reservations(struct v3d_submit *submit)
 {
-	int i, ret;
+	int i, j, ret;
 
-	drm_exec_init(exec, DRM_EXEC_INTERRUPTIBLE_WAIT, job->bo_count);
-	drm_exec_until_all_locked(exec) {
-		ret = drm_exec_prepare_array(exec, job->bo, job->bo_count, 1);
-	}
+	drm_exec_init(&submit->exec,
+		      DRM_EXEC_INTERRUPTIBLE_WAIT | DRM_EXEC_IGNORE_DUPLICATES, 0);
+	drm_exec_until_all_locked(&submit->exec) {
+		for (i = 0; i < submit->job_count; i++) {
+			struct v3d_job *job = submit->jobs[i];
 
-	if (ret)
-		goto fail;
-
-	for (i = 0; i < job->bo_count; i++) {
-		ret = drm_sched_job_add_implicit_dependencies(&job->base,
-							      job->bo[i], true);
+			ret = drm_exec_prepare_array(&submit->exec, job->bo,
+						     job->bo_count, 1);
+			if (ret)
+				break;
+		}
+		drm_exec_retry_on_contention(&submit->exec);
 		if (ret)
 			goto fail;
 	}
 
+	for (i = 0; i < submit->job_count; i++) {
+		struct v3d_job *job = submit->jobs[i];
+
+		for (j = 0; j < job->bo_count; j++) {
+			ret = drm_sched_job_add_implicit_dependencies(&job->base,
+								      job->bo[j],
+								      true);
+			if (ret)
+				goto fail;
+		}
+	}
+
 	return 0;
 
 fail:
-	drm_exec_fini(exec);
+	drm_exec_fini(&submit->exec);
 	return ret;
 }
 
+static void
+v3d_submit_unlock_reservations(struct v3d_submit *submit)
+{
+	drm_exec_fini(&submit->exec);
+}
+
 /**
  * v3d_lookup_bos() - Sets up job->bo[] with the GEM objects
  * referenced by the job.
@@ -63,25 +82,23 @@ v3d_lock_bo_reservations(struct v3d_job *job, struct drm_exec *exec)
  * failure, because that will happen at `v3d_job_free()`.
  */
 static int
-v3d_lookup_bos(struct drm_device *dev,
-	       struct drm_file *file_priv,
-	       struct v3d_job *job,
-	       u64 bo_handles,
-	       u32 bo_count)
+v3d_lookup_bos(struct v3d_submit *submit, u64 bo_handles, u32 bo_count)
 {
-	job->bo_count = bo_count;
+	struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
 
-	if (!job->bo_count) {
+	last_job->bo_count = bo_count;
+
+	if (!last_job->bo_count) {
 		/* See comment on bo_index for why we have to check
 		 * this.
 		 */
-		drm_warn(dev, "Rendering requires BOs\n");
+		drm_warn(&submit->v3d->drm, "Rendering requires BOs\n");
 		return -EINVAL;
 	}
 
-	return drm_gem_objects_lookup(file_priv,
+	return drm_gem_objects_lookup(submit->file_priv,
 				      (void __user *)(uintptr_t)bo_handles,
-				      job->bo_count, &job->bo);
+				      last_job->bo_count, &last_job->bo);
 }
 
 static void
@@ -160,25 +177,6 @@ void v3d_job_put(struct v3d_job *job)
 	kref_put(&job->refcount, job->free);
 }
 
-static int
-v3d_job_allocate(struct v3d_dev *v3d, void **container, size_t size)
-{
-	*container = kcalloc(1, size, GFP_KERNEL);
-	if (!*container) {
-		drm_err(&v3d->drm, "Cannot allocate memory for V3D job.\n");
-		return -ENOMEM;
-	}
-
-	return 0;
-}
-
-static void
-v3d_job_deallocate(void **container)
-{
-	kfree(*container);
-	*container = NULL;
-}
-
 static int
 v3d_job_add_syncobjs(struct v3d_job *job, struct drm_file *file_priv,
 		     u32 in_sync, struct v3d_submit_ext *se)
@@ -219,48 +217,6 @@ v3d_job_add_syncobjs(struct v3d_job *job, struct drm_file *file_priv,
 	return 0;
 }
 
-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, struct v3d_submit_ext *se, enum v3d_queue queue)
-{
-	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
-	int ret;
-
-	job->v3d = v3d;
-	job->free = free;
-	job->queue = queue;
-	job->file_priv = v3d_priv;
-
-	ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue],
-				 1, v3d_priv, file_priv->client_id);
-	if (ret)
-		return ret;
-
-	ret = v3d_job_add_syncobjs(job, file_priv, in_sync, se);
-	if (ret)
-		goto fail_job_init;
-
-	/* CPU jobs don't require hardware resources */
-	if (queue != V3D_CPU) {
-		ret = v3d_pm_runtime_get(v3d);
-		if (ret)
-			goto fail_job_init;
-		job->has_pm_ref = true;
-	}
-
-	kref_init(&job->refcount);
-
-	job->client_stats = v3d_stats_get(v3d_priv->stats[queue]);
-	job->global_stats = v3d_stats_get(v3d->queue[queue].stats);
-
-	return 0;
-
-fail_job_init:
-	drm_sched_job_cleanup(&job->base);
-	return ret;
-}
-
 static const struct {
 	size_t size;
 	void (*free)(struct kref *ref);
@@ -363,31 +319,34 @@ v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, u32 perfmon_id)
 }
 
 static void
-v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
-					 struct v3d_job *job,
-					 struct drm_exec *exec,
-					 u32 out_sync,
-					 struct v3d_submit_ext *se,
-					 struct dma_fence *done_fence)
+v3d_attach_fences_and_unlock_reservation(struct v3d_submit *submit,
+					 u32 out_sync, struct v3d_submit_ext *se)
 {
-	struct drm_syncobj *sync_out;
 	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
-	int i;
+	struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
+	struct drm_syncobj *sync_out;
 
-	for (i = 0; i < job->bo_count; i++) {
-		/* XXX: Use shared fences for read-only objects. */
-		dma_resv_add_fence(job->bo[i]->resv, job->done_fence,
-				   DMA_RESV_USAGE_WRITE);
+	/* The submission's last fence covers the entire submission. Attach it
+	 * to every BO touched by any job in the submission.
+	 */
+	for (int i = 0; i < submit->job_count; i++) {
+		struct v3d_job *job = submit->jobs[i];
+
+		for (int j = 0; j < job->bo_count; j++) {
+			/* XXX: Use shared fences for read-only objects. */
+			dma_resv_add_fence(job->bo[j]->resv, last_job->done_fence,
+					   DMA_RESV_USAGE_WRITE);
+		}
 	}
 
-	drm_exec_fini(exec);
+	v3d_submit_unlock_reservations(submit);
 
 	/* Update the return sync object for the job */
 	/* If it only supports a single signal semaphore*/
 	if (!has_multisync) {
-		sync_out = drm_syncobj_find(file_priv, out_sync);
+		sync_out = drm_syncobj_find(submit->file_priv, out_sync);
 		if (sync_out) {
-			drm_syncobj_replace_fence(sync_out, done_fence);
+			drm_syncobj_replace_fence(sync_out, last_job->done_fence);
 			drm_syncobj_put(sync_out);
 		}
 		return;
@@ -395,9 +354,9 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
 
 	/* If multiple semaphores extension is supported */
 	if (se->out_sync_count) {
-		for (i = 0; i < se->out_sync_count; i++) {
+		for (int i = 0; i < se->out_sync_count; i++) {
 			drm_syncobj_replace_fence(se->out_syncs[i].syncobj,
-						  done_fence);
+						  last_job->done_fence);
 			drm_syncobj_put(se->out_syncs[i].syncobj);
 		}
 		kvfree(se->out_syncs);
@@ -418,7 +377,8 @@ v3d_push_job(struct v3d_job *job)
 }
 
 static int
-v3d_submit_jobs(struct v3d_submit *submit)
+v3d_submit_jobs(struct v3d_submit *submit, u32 out_sync,
+		struct v3d_submit_ext *se)
 {
 	struct v3d_dev *v3d = submit->v3d;
 	int ret = 0;
@@ -438,52 +398,42 @@ v3d_submit_jobs(struct v3d_submit *submit)
 		}
 	}
 
+	mutex_unlock(&v3d->sched_lock);
+
+	v3d_attach_fences_and_unlock_reservation(submit, out_sync, se);
+	v3d_submit_put_jobs(submit);
+
+	return 0;
+
 err:
 	mutex_unlock(&v3d->sched_lock);
 	return ret;
 }
 
 static int
-v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
-			   struct v3d_dev *v3d,
+v3d_setup_csd_jobs_and_bos(struct v3d_submit *submit,
 			   struct drm_v3d_submit_csd *args,
-			   struct v3d_csd_job **job,
-			   struct v3d_job **clean_job,
-			   struct v3d_submit_ext *se,
-			   struct drm_exec *exec)
+			   struct v3d_submit_ext *se)
 {
+	struct v3d_csd_job *job;
+	struct v3d_job *clean_job;
 	int ret;
 
-	ret = v3d_job_allocate(v3d, (void *)job, sizeof(**job));
+	job = (struct v3d_csd_job *) v3d_submit_add_job(submit, V3D_CSD);
+	if (IS_ERR(job))
+		return PTR_ERR(job);
+
+	ret = v3d_job_add_syncobjs(&job->base, submit->file_priv, args->in_sync, se);
 	if (ret)
 		return ret;
 
-	ret = v3d_job_init(v3d, file_priv, &(*job)->base,
-			   v3d_job_free, args->in_sync, se, V3D_CSD);
-	if (ret) {
-		v3d_job_deallocate((void *)job);
-		return ret;
-	}
+	job->args = *args;
 
-	ret = v3d_job_allocate(v3d, (void *)clean_job, sizeof(**clean_job));
-	if (ret)
-		return ret;
+	clean_job = v3d_submit_add_job(submit, V3D_CACHE_CLEAN);
+	if (IS_ERR(clean_job))
+		return PTR_ERR(clean_job);
 
-	ret = v3d_job_init(v3d, file_priv, *clean_job,
-			   v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
-	if (ret) {
-		v3d_job_deallocate((void *)clean_job);
-		return ret;
-	}
-
-	(*job)->args = *args;
-
-	ret = v3d_lookup_bos(&v3d->drm, file_priv, *clean_job,
-			     args->bo_handles, args->bo_handle_count);
-	if (ret)
-		return ret;
-
-	return v3d_lock_bo_reservations(*clean_job, exec);
+	return v3d_lookup_bos(submit, args->bo_handles, args->bo_handle_count);
 }
 
 static void
@@ -1123,33 +1073,22 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_lookup_bos(dev, file_priv,
-			     submit.jobs[submit.job_count - 1],
-			     args->bo_handles, args->bo_handle_count);
+	ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);
 	if (ret)
 		goto fail;
 
-	ret = v3d_lock_bo_reservations(submit.jobs[submit.job_count - 1],
-				       &submit.exec);
+	ret = v3d_submit_lock_reservations(&submit);
 	if (ret)
 		goto fail;
 
-	ret = v3d_submit_jobs(&submit);
+	ret = v3d_submit_jobs(&submit, args->out_sync, &se);
 	if (ret)
 		goto fail_unreserve;
 
-	v3d_attach_fences_and_unlock_reservation(file_priv,
-						 submit.jobs[submit.job_count - 1],
-						 &submit.exec,
-						 args->out_sync, &se,
-						 submit.jobs[submit.job_count - 1]->done_fence);
-
-	v3d_submit_put_jobs(&submit);
-
 	return 0;
 
 fail_unreserve:
-	drm_exec_fini(&submit.exec);
+	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
 	v3d_put_multisync_post_deps(&se);
@@ -1228,25 +1167,18 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 		job->base.bo[job->base.bo_count] = bo;
 	}
 
-	ret = v3d_lock_bo_reservations(&job->base, &submit.exec);
+	ret = v3d_submit_lock_reservations(&submit);
 	if (ret)
 		goto fail;
 
-	ret = v3d_submit_jobs(&submit);
+	ret = v3d_submit_jobs(&submit, args->out_sync, &se);
 	if (ret)
 		goto fail_unreserve;
 
-	v3d_attach_fences_and_unlock_reservation(file_priv,
-						 &job->base, &submit.exec,
-						 args->out_sync, &se,
-						 job->base.done_fence);
-
-	v3d_submit_put_jobs(&submit);
-
 	return 0;
 
 fail_unreserve:
-	drm_exec_fini(&submit.exec);
+	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
 	v3d_put_multisync_post_deps(&se);
@@ -1270,8 +1202,6 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_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;
 	int ret;
 
 	trace_v3d_submit_csd_ioctl(dev, args->cfg[5], args->cfg[6]);
@@ -1297,34 +1227,26 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 		}
 	}
 
-	ret = v3d_setup_csd_jobs_and_bos(file_priv, submit.v3d, args,
-					 &job, &clean_job, &se, &submit.exec);
+	ret = v3d_setup_csd_jobs_and_bos(&submit, args, &se);
 	if (ret)
 		goto fail;
 
-	submit.jobs[submit.job_count++] = &job->base;
-	submit.jobs[submit.job_count++] = clean_job;
-
 	ret = v3d_attach_perfmon_to_jobs(&submit, args->perfmon_id);
 	if (ret)
-		goto fail_unreserve;
+		goto fail;
 
-	ret = v3d_submit_jobs(&submit);
+	ret = v3d_submit_lock_reservations(&submit);
+	if (ret)
+		goto fail;
+
+	ret = v3d_submit_jobs(&submit, args->out_sync, &se);
 	if (ret)
 		goto fail_unreserve;
 
-	v3d_attach_fences_and_unlock_reservation(file_priv,
-						 clean_job,
-						 &submit.exec,
-						 args->out_sync, &se,
-						 clean_job->done_fence);
-
-	v3d_submit_put_jobs(&submit);
-
 	return 0;
 
 fail_unreserve:
-	drm_exec_fini(&submit.exec);
+	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
 	v3d_put_multisync_post_deps(&se);
@@ -1355,13 +1277,14 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *file_priv)
 {
 	struct v3d_dev *v3d = to_v3d_dev(dev);
+	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
+	struct v3d_submit indirect_submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
 	struct drm_v3d_submit_cpu *args = data;
 	struct v3d_submit_ext se = {0};
 	struct v3d_submit_ext *out_se = NULL;
 	struct v3d_cpu_job *cpu_job = NULL;
 	struct v3d_csd_job *csd_job = NULL;
 	struct v3d_job *clean_job = NULL;
-	struct drm_exec exec;
 	int ret;
 
 	if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {
@@ -1369,9 +1292,9 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	ret = v3d_job_allocate(v3d, (void *)&cpu_job, sizeof(*cpu_job));
-	if (ret)
-		return ret;
+	cpu_job = (struct v3d_cpu_job *) v3d_submit_add_job(&submit, V3D_CPU);
+	if (IS_ERR(cpu_job))
+		return PTR_ERR(cpu_job);
 
 	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
 		ret = v3d_get_extensions(file_priv, args->extensions, &se, cpu_job);
@@ -1396,34 +1319,35 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 
 	trace_v3d_submit_cpu_ioctl(&v3d->drm, cpu_job->job_type);
 
-	ret = v3d_job_init(v3d, file_priv, &cpu_job->base,
-			   v3d_cpu_job_free, 0, &se, V3D_CPU);
-	if (ret) {
-		v3d_job_deallocate((void *)&cpu_job);
+	ret = v3d_job_add_syncobjs(&cpu_job->base, file_priv, 0, &se);
+	if (ret)
 		goto fail;
-	}
 
 	if (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) {
-		ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d,
+		ret = v3d_setup_csd_jobs_and_bos(&indirect_submit,
 						 &cpu_job->indirect_csd.args,
-						 &cpu_job->indirect_csd.job,
-						 &cpu_job->indirect_csd.clean_job,
-						 NULL,
-						 &cpu_job->indirect_csd.exec);
+						 NULL);
 		if (ret)
 			goto fail;
+
+		ret = v3d_submit_lock_reservations(&indirect_submit);
+		if (ret)
+			goto fail;
+
+		cpu_job->indirect_csd.job = container_of(indirect_submit.jobs[0],
+							 struct v3d_csd_job, base);
+		cpu_job->indirect_csd.clean_job = indirect_submit.jobs[1];
+
+		clean_job = cpu_job->indirect_csd.clean_job;
+		csd_job = cpu_job->indirect_csd.job;
 	}
 
-	clean_job = cpu_job->indirect_csd.clean_job;
-	csd_job = cpu_job->indirect_csd.job;
-
 	if (args->bo_handle_count) {
-		ret = v3d_lookup_bos(dev, file_priv, &cpu_job->base,
-				     args->bo_handles, args->bo_handle_count);
+		ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);
 		if (ret)
 			goto fail;
 
-		ret = v3d_lock_bo_reservations(&cpu_job->base, &exec);
+		ret = v3d_submit_lock_reservations(&submit);
 		if (ret)
 			goto fail;
 	}
@@ -1455,36 +1379,28 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 
 	out_se = (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) ? NULL : &se;
 
-	v3d_attach_fences_and_unlock_reservation(file_priv,
-						 &cpu_job->base,
-						 &exec, 0,
-						 out_se, cpu_job->base.done_fence);
+	v3d_attach_fences_and_unlock_reservation(&submit, 0, out_se);
 
 	switch (cpu_job->job_type) {
 	case V3D_CPU_JOB_TYPE_INDIRECT_CSD:
-		v3d_attach_fences_and_unlock_reservation(file_priv,
-							 clean_job,
-							 &cpu_job->indirect_csd.exec,
-							 0, &se, clean_job->done_fence);
+		v3d_attach_fences_and_unlock_reservation(&indirect_submit, 0, &se);
 		break;
 	default:
 		break;
 	}
 
-	v3d_job_put(&cpu_job->base);
-	v3d_job_put(&csd_job->base);
-	v3d_job_put(clean_job);
+	v3d_submit_put_jobs(&submit);
+	v3d_submit_put_jobs(&indirect_submit);
 
 	return 0;
 
 fail_unreserve:
 	mutex_unlock(&v3d->sched_lock);
-	drm_exec_fini(&exec);
-	drm_exec_fini(&cpu_job->indirect_csd.exec);
+	v3d_submit_unlock_reservations(&submit);
+	v3d_submit_unlock_reservations(&indirect_submit);
 fail:
-	v3d_job_cleanup((void *)cpu_job);
-	v3d_job_cleanup((void *)csd_job);
-	v3d_job_cleanup(clean_job);
+	v3d_submit_cleanup_jobs(&submit);
+	v3d_submit_cleanup_jobs(&indirect_submit);
 	v3d_put_multisync_post_deps(&se);
 
 	return ret;

-- 
2.54.0

[PATCH v3 11/14] drm/v3d: Refactor CPU ioctl into unified submission chain

[Maíra Canal]

Restructure the CPU ioctl so that all job types, including indirect CSD,
use a single struct v3d_submit chain and a single DRM exec context.

Now that v3d_get_cpu_indirect_csd_params() is a pure parser and the
submit helpers operate on struct v3d_submit, fold the indirect CSD path
into the standard flow by appending the CSD and CLEAN_CACHE jobs to the
same struct v3d_submit as the CPU job and locking the union of all jobs'
BOs under one drm_exec. This eliminates the second drm_exec, the nested
submission, and the conditional two-pass fence attachment that the CPU
ioctl previously required for the indirect CSD path.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.h    |  6 ---
 drivers/gpu/drm/v3d/v3d_submit.c | 90 ++++++++++------------------------------
 2 files changed, 23 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 52ed0af67f5e..e91b1b83dc23 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -425,9 +425,6 @@ struct v3d_indirect_csd_info {
 	/* Indirect CSD */
 	struct v3d_csd_job *job;
 
-	/* Clean cache job associated to the Indirect CSD job */
-	struct v3d_job *clean_job;
-
 	/* Indirect CSD args, stashed by the extension parser and later used
 	 * to create the CSD job from them.
 	 */
@@ -446,9 +443,6 @@ struct v3d_indirect_csd_info {
 
 	/* Indirect BO */
 	struct drm_gem_object *indirect;
-
-	/* Context of the Indirect CSD job */
-	struct drm_exec exec;
 };
 
 struct v3d_timestamp_query_info {
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 62c23feb8fbb..1df7ab528422 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -1276,15 +1276,10 @@ int
 v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *file_priv)
 {
-	struct v3d_dev *v3d = to_v3d_dev(dev);
 	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
-	struct v3d_submit indirect_submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
 	struct drm_v3d_submit_cpu *args = data;
 	struct v3d_submit_ext se = {0};
-	struct v3d_submit_ext *out_se = NULL;
 	struct v3d_cpu_job *cpu_job = NULL;
-	struct v3d_csd_job *csd_job = NULL;
-	struct v3d_job *clean_job = NULL;
 	int ret;
 
 	if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {
@@ -1317,90 +1312,51 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 		goto fail;
 	}
 
-	trace_v3d_submit_cpu_ioctl(&v3d->drm, cpu_job->job_type);
+	trace_v3d_submit_cpu_ioctl(dev, cpu_job->job_type);
 
 	ret = v3d_job_add_syncobjs(&cpu_job->base, file_priv, 0, &se);
 	if (ret)
 		goto fail;
 
-	if (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) {
-		ret = v3d_setup_csd_jobs_and_bos(&indirect_submit,
-						 &cpu_job->indirect_csd.args,
-						 NULL);
-		if (ret)
-			goto fail;
-
-		ret = v3d_submit_lock_reservations(&indirect_submit);
-		if (ret)
-			goto fail;
-
-		cpu_job->indirect_csd.job = container_of(indirect_submit.jobs[0],
-							 struct v3d_csd_job, base);
-		cpu_job->indirect_csd.clean_job = indirect_submit.jobs[1];
-
-		clean_job = cpu_job->indirect_csd.clean_job;
-		csd_job = cpu_job->indirect_csd.job;
-	}
-
+	/* Look up the CPU jobs' BOs before v3d_setup_csd_jobs_and_bos() appends
+	 * the CSD and clean jobs in the case of indirect CSD job.
+	 */
 	if (args->bo_handle_count) {
 		ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);
 		if (ret)
 			goto fail;
+	}
 
-		ret = v3d_submit_lock_reservations(&submit);
+	if (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) {
+		ret = v3d_setup_csd_jobs_and_bos(&submit, &cpu_job->indirect_csd.args,
+						 NULL);
 		if (ret)
 			goto fail;
+
+		/* The CSD job was appended at jobs[1] */
+		if (WARN_ON(submit.jobs[1]->queue != V3D_CSD)) {
+			ret = -EINVAL;
+			goto fail;
+		}
+
+		cpu_job->indirect_csd.job = container_of(submit.jobs[1], struct v3d_csd_job,
+							 base);
 	}
 
-	mutex_lock(&v3d->sched_lock);
-	v3d_push_job(&cpu_job->base);
+	ret = v3d_submit_lock_reservations(&submit);
+	if (ret)
+		goto fail;
 
-	switch (cpu_job->job_type) {
-	case V3D_CPU_JOB_TYPE_INDIRECT_CSD:
-		ret = drm_sched_job_add_dependency(&csd_job->base.base,
-						   dma_fence_get(cpu_job->base.done_fence));
-		if (ret)
-			goto fail_unreserve;
-
-		v3d_push_job(&csd_job->base);
-
-		ret = drm_sched_job_add_dependency(&clean_job->base,
-						   dma_fence_get(csd_job->base.done_fence));
-		if (ret)
-			goto fail_unreserve;
-
-		v3d_push_job(clean_job);
-
-		break;
-	default:
-		break;
-	}
-	mutex_unlock(&v3d->sched_lock);
-
-	out_se = (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) ? NULL : &se;
-
-	v3d_attach_fences_and_unlock_reservation(&submit, 0, out_se);
-
-	switch (cpu_job->job_type) {
-	case V3D_CPU_JOB_TYPE_INDIRECT_CSD:
-		v3d_attach_fences_and_unlock_reservation(&indirect_submit, 0, &se);
-		break;
-	default:
-		break;
-	}
-
-	v3d_submit_put_jobs(&submit);
-	v3d_submit_put_jobs(&indirect_submit);
+	ret = v3d_submit_jobs(&submit, 0, &se);
+	if (ret)
+		goto fail_unreserve;
 
 	return 0;
 
 fail_unreserve:
-	mutex_unlock(&v3d->sched_lock);
 	v3d_submit_unlock_reservations(&submit);
-	v3d_submit_unlock_reservations(&indirect_submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
-	v3d_submit_cleanup_jobs(&indirect_submit);
 	v3d_put_multisync_post_deps(&se);
 
 	return ret;

-- 
2.54.0

[PATCH v3 12/14] drm/v3d: Split BO fence attach from syncobj output handling

[Maíra Canal]

v3d_attach_fences_and_unlock_reservation() does three different things:
(1) attaches the submission's last fence to every BO, (2) releases
drm_exec, and (3) replaces the userspace out_sync syncobjs. Decouple
these three behaviors into different functions, so that each function
has a more self-contained behavior.

v3d_submit_jobs() now invokes the three steps explicitly, which makes the
submission sequence self-documenting and keeps each helper self-contained.

No functional change; just code consolidation.

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

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 1df7ab528422..2beb99a25104 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -319,12 +319,9 @@ v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, u32 perfmon_id)
 }
 
 static void
-v3d_attach_fences_and_unlock_reservation(struct v3d_submit *submit,
-					 u32 out_sync, struct v3d_submit_ext *se)
+v3d_submit_attach_object_fences(struct v3d_submit *submit)
 {
-	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
 	struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
-	struct drm_syncobj *sync_out;
 
 	/* The submission's last fence covers the entire submission. Attach it
 	 * to every BO touched by any job in the submission.
@@ -338,8 +335,15 @@ v3d_attach_fences_and_unlock_reservation(struct v3d_submit *submit,
 					   DMA_RESV_USAGE_WRITE);
 		}
 	}
+}
 
-	v3d_submit_unlock_reservations(submit);
+static void
+v3d_submit_process_post_deps(struct v3d_submit *submit, u32 out_sync,
+			     struct v3d_submit_ext *se)
+{
+	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
+	struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
+	struct drm_syncobj *sync_out;
 
 	/* Update the return sync object for the job */
 	/* If it only supports a single signal semaphore*/
@@ -400,7 +404,10 @@ v3d_submit_jobs(struct v3d_submit *submit, u32 out_sync,
 
 	mutex_unlock(&v3d->sched_lock);
 
-	v3d_attach_fences_and_unlock_reservation(submit, out_sync, se);
+	v3d_submit_attach_object_fences(submit);
+	v3d_submit_unlock_reservations(submit);
+	v3d_submit_process_post_deps(submit, out_sync, se);
+
 	v3d_submit_put_jobs(submit);
 
 	return 0;

-- 
2.54.0

[PATCH v3 13/14] drm/v3d: Reject invalid out_sync handles in submit ioctls

[Maíra Canal]

v3d_submit_process_post_deps() looks up the out_sync syncobj via
drm_syncobj_find(), and if userspace passes a non-zero handle that doesn't
refer to a valid syncobj, the lookup silently returns NULL and the
post-deps step skips publishing the submission's last fence to it. The
ioctl still returns success, leaving userspace to wait on a invalid
syncobj.

Instead of silently ignoring an invalid non-zero out_sync, move the syncobj
lookup to the submission and make it fail with -ENOENT up front, mirroring
the syncobj validation already done for in_sync. Now,
v3d_submit_process_post_deps() only does the fence replacement.

Note that the lookup is skipped when the multi-sync extension is in use,
since args->out_sync is unused in that case.

To keep cleanup symmetric on error paths, convert the function
v3d_put_multisync_post_deps() into a single function that releases the
references that were acquired but never published for both single-sync
and multi-sync.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_submit.c | 50 +++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 2beb99a25104..dc27770d85fd 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -338,17 +338,15 @@ v3d_submit_attach_object_fences(struct v3d_submit *submit)
 }
 
 static void
-v3d_submit_process_post_deps(struct v3d_submit *submit, u32 out_sync,
+v3d_submit_process_post_deps(struct v3d_submit *submit, struct drm_syncobj *sync_out,
 			     struct v3d_submit_ext *se)
 {
 	bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC);
 	struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
-	struct drm_syncobj *sync_out;
 
 	/* Update the return sync object for the job */
 	/* If it only supports a single signal semaphore*/
 	if (!has_multisync) {
-		sync_out = drm_syncobj_find(submit->file_priv, out_sync);
 		if (sync_out) {
 			drm_syncobj_replace_fence(sync_out, last_job->done_fence);
 			drm_syncobj_put(sync_out);
@@ -381,7 +379,7 @@ v3d_push_job(struct v3d_job *job)
 }
 
 static int
-v3d_submit_jobs(struct v3d_submit *submit, u32 out_sync,
+v3d_submit_jobs(struct v3d_submit *submit, struct drm_syncobj *sync_out,
 		struct v3d_submit_ext *se)
 {
 	struct v3d_dev *v3d = submit->v3d;
@@ -406,7 +404,7 @@ v3d_submit_jobs(struct v3d_submit *submit, u32 out_sync,
 
 	v3d_submit_attach_object_fences(submit);
 	v3d_submit_unlock_reservations(submit);
-	v3d_submit_process_post_deps(submit, out_sync, se);
+	v3d_submit_process_post_deps(submit, sync_out, se);
 
 	v3d_submit_put_jobs(submit);
 
@@ -444,10 +442,13 @@ v3d_setup_csd_jobs_and_bos(struct v3d_submit *submit,
 }
 
 static void
-v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
+v3d_submit_put_post_deps(struct drm_syncobj *sync_out, struct v3d_submit_ext *se)
 {
 	unsigned int i;
 
+	if (sync_out)
+		drm_syncobj_put(sync_out);
+
 	if (!(se && se->out_sync_count))
 		return;
 
@@ -1006,6 +1007,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 {
 	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
 	struct drm_v3d_submit_cl *args = data;
+	struct drm_syncobj *sync_out = NULL;
 	struct v3d_submit_ext se = {0};
 	struct v3d_bin_job *bin = NULL;
 	struct v3d_render_job *render = NULL;
@@ -1032,6 +1034,12 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 		}
 	}
 
+	if (args->out_sync && !(se.flags & DRM_V3D_EXT_ID_MULTI_SYNC)) {
+		sync_out = drm_syncobj_find(file_priv, args->out_sync);
+		if (!sync_out)
+			return -ENOENT;
+	}
+
 	if (args->bcl_start != args->bcl_end) {
 		bin = (struct v3d_bin_job *) v3d_submit_add_job(&submit, V3D_BIN);
 		if (IS_ERR(bin)) {
@@ -1088,7 +1096,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_submit_jobs(&submit, args->out_sync, &se);
+	ret = v3d_submit_jobs(&submit, sync_out, &se);
 	if (ret)
 		goto fail_unreserve;
 
@@ -1098,7 +1106,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
-	v3d_put_multisync_post_deps(&se);
+	v3d_submit_put_post_deps(sync_out, &se);
 
 	return ret;
 }
@@ -1118,6 +1126,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 {
 	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
 	struct drm_v3d_submit_tfu *args = data;
+	struct drm_syncobj *sync_out = NULL;
 	struct v3d_submit_ext se = {0};
 	struct v3d_tfu_job *job = NULL;
 	int ret = 0;
@@ -1137,6 +1146,12 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 		}
 	}
 
+	if (args->out_sync && !(se.flags & DRM_V3D_EXT_ID_MULTI_SYNC)) {
+		sync_out = drm_syncobj_find(file_priv, args->out_sync);
+		if (!sync_out)
+			return -ENOENT;
+	}
+
 	job = (struct v3d_tfu_job *) v3d_submit_add_job(&submit, V3D_TFU);
 	if (IS_ERR(job)) {
 		ret = PTR_ERR(job);
@@ -1178,7 +1193,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_submit_jobs(&submit, args->out_sync, &se);
+	ret = v3d_submit_jobs(&submit, sync_out, &se);
 	if (ret)
 		goto fail_unreserve;
 
@@ -1188,7 +1203,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
-	v3d_put_multisync_post_deps(&se);
+	v3d_submit_put_post_deps(sync_out, &se);
 
 	return ret;
 }
@@ -1208,6 +1223,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 {
 	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
 	struct drm_v3d_submit_csd *args = data;
+	struct drm_syncobj *sync_out = NULL;
 	struct v3d_submit_ext se = {0};
 	int ret;
 
@@ -1234,6 +1250,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 		}
 	}
 
+	if (args->out_sync && !(se.flags & DRM_V3D_EXT_ID_MULTI_SYNC)) {
+		sync_out = drm_syncobj_find(file_priv, args->out_sync);
+		if (!sync_out)
+			return -ENOENT;
+	}
+
 	ret = v3d_setup_csd_jobs_and_bos(&submit, args, &se);
 	if (ret)
 		goto fail;
@@ -1246,7 +1268,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_submit_jobs(&submit, args->out_sync, &se);
+	ret = v3d_submit_jobs(&submit, sync_out, &se);
 	if (ret)
 		goto fail_unreserve;
 
@@ -1256,7 +1278,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
-	v3d_put_multisync_post_deps(&se);
+	v3d_submit_put_post_deps(sync_out, &se);
 
 	return ret;
 }
@@ -1354,7 +1376,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_submit_jobs(&submit, 0, &se);
+	ret = v3d_submit_jobs(&submit, NULL, &se);
 	if (ret)
 		goto fail_unreserve;
 
@@ -1364,7 +1386,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
-	v3d_put_multisync_post_deps(&se);
+	v3d_submit_put_post_deps(NULL, &se);
 
 	return ret;
 }

-- 
2.54.0

[PATCH v3 14/14] drm/v3d: Ensure atomic submissions in v3d_submit_jobs()

[Maíra Canal]

Currently, v3d_submit_jobs() arms and pushes each job one at a time,
wiring dependencies between consecutive jobs after each push. If
drm_sched_job_add_dependency() fails midway, the already-pushed jobs are
scheduler-owned and will be submitted to the GPU for execution, even though
the subsequent jobs won't be submitted.

This breaks the atomicity of the submissions, as only some of the jobs
from a submission would be submitted to the hardware, while the other part
fails.

Restructure v3d_submit_jobs() into three phases: (1) arm all jobs belonging
to a given submission, (2) wire inter-job dependencies, and (3) push all
jobs to the scheduler unconditionally. Phase (2) can fail; on failure,
it marks every armed job finished fence with an error, so that run_job()
callbacks skip hardware execution.

This guarantees that every armed job is always pushed, either to run
or to be skipped, and it also ensures the atomicity of a submission.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c  |  6 ++++
 drivers/gpu/drm/v3d/v3d_submit.c | 65 ++++++++++++++++++++--------------------
 2 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 04fd1a365576..c16a9d4d41e6 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -656,6 +656,9 @@ v3d_cpu_job_run(struct drm_sched_job *sched_job)
 	struct v3d_cpu_job *job = to_cpu_job(sched_job);
 	struct v3d_dev *v3d = job->base.v3d;
 
+	if (unlikely(job->base.base.s_fence->finished.error))
+		return NULL;
+
 	if (job->job_type >= ARRAY_SIZE(cpu_job_function)) {
 		drm_dbg(&v3d->drm, "Unknown CPU job: %d\n", job->job_type);
 		return NULL;
@@ -679,6 +682,9 @@ v3d_cache_clean_job_run(struct drm_sched_job *sched_job)
 	struct v3d_job *job = to_v3d_job(sched_job);
 	struct v3d_dev *v3d = job->v3d;
 
+	if (unlikely(job->base.s_fence->finished.error))
+		return NULL;
+
 	v3d_job_start_stats(job);
 
 	v3d_clean_caches(v3d);
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index dc27770d85fd..e118ba69e308 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -365,19 +365,6 @@ v3d_submit_process_post_deps(struct v3d_submit *submit, struct drm_syncobj *sync
 	}
 }
 
-static void
-v3d_push_job(struct v3d_job *job)
-{
-	drm_sched_job_arm(&job->base);
-
-	job->done_fence = dma_fence_get(&job->base.s_fence->finished);
-
-	/* put by scheduler job completion */
-	kref_get(&job->refcount);
-
-	drm_sched_entity_push_job(&job->base);
-}
-
 static int
 v3d_submit_jobs(struct v3d_submit *submit, struct drm_syncobj *sync_out,
 		struct v3d_submit_ext *se)
@@ -390,16 +377,23 @@ v3d_submit_jobs(struct v3d_submit *submit, struct drm_syncobj *sync_out,
 	for (int i = 0; i < submit->job_count; i++) {
 		struct v3d_job *job = submit->jobs[i];
 
-		v3d_push_job(job);
+		drm_sched_job_arm(&job->base);
+		job->done_fence = dma_fence_get(&job->base.s_fence->finished);
 
-		if (i + 1 < submit->job_count) {
-			ret = drm_sched_job_add_dependency(&submit->jobs[i + 1]->base,
-							   dma_fence_get(job->done_fence));
-			if (ret)
-				goto err;
-		}
+		/* put by scheduler job completion */
+		kref_get(&job->refcount);
 	}
 
+	for (int i = 0; i + 1 < submit->job_count; i++) {
+		ret = drm_sched_job_add_dependency(&submit->jobs[i + 1]->base,
+						   dma_fence_get(submit->jobs[i]->done_fence));
+		if (ret)
+			goto err;
+	}
+
+	for (int i = 0; i < submit->job_count; i++)
+		drm_sched_entity_push_job(&submit->jobs[i]->base);
+
 	mutex_unlock(&v3d->sched_lock);
 
 	v3d_submit_attach_object_fences(submit);
@@ -411,7 +405,18 @@ v3d_submit_jobs(struct v3d_submit *submit, struct drm_syncobj *sync_out,
 	return 0;
 
 err:
+	/* Mark every armed job as failed so run_job() skips execution */
+	for (int i = 0; i < submit->job_count; i++)
+		dma_fence_set_error(&submit->jobs[i]->base.s_fence->finished, ret);
+
+	for (int i = 0; i < submit->job_count; i++)
+		drm_sched_entity_push_job(&submit->jobs[i]->base);
+
 	mutex_unlock(&v3d->sched_lock);
+
+	v3d_submit_unlock_reservations(submit);
+	v3d_submit_put_jobs(submit);
+
 	return ret;
 }
 
@@ -1098,14 +1103,13 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 
 	ret = v3d_submit_jobs(&submit, sync_out, &se);
 	if (ret)
-		goto fail_unreserve;
+		goto fail_submit;
 
 	return 0;
 
-fail_unreserve:
-	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
+fail_submit:
 	v3d_submit_put_post_deps(sync_out, &se);
 
 	return ret;
@@ -1195,14 +1199,13 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 
 	ret = v3d_submit_jobs(&submit, sync_out, &se);
 	if (ret)
-		goto fail_unreserve;
+		goto fail_submit;
 
 	return 0;
 
-fail_unreserve:
-	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
+fail_submit:
 	v3d_submit_put_post_deps(sync_out, &se);
 
 	return ret;
@@ -1270,14 +1273,13 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
 
 	ret = v3d_submit_jobs(&submit, sync_out, &se);
 	if (ret)
-		goto fail_unreserve;
+		goto fail_submit;
 
 	return 0;
 
-fail_unreserve:
-	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
+fail_submit:
 	v3d_submit_put_post_deps(sync_out, &se);
 
 	return ret;
@@ -1378,14 +1380,13 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 
 	ret = v3d_submit_jobs(&submit, NULL, &se);
 	if (ret)
-		goto fail_unreserve;
+		goto fail_submit;
 
 	return 0;
 
-fail_unreserve:
-	v3d_submit_unlock_reservations(&submit);
 fail:
 	v3d_submit_cleanup_jobs(&submit);
+fail_submit:
 	v3d_submit_put_post_deps(NULL, &se);
 
 	return ret;

-- 
2.54.0

Claude review: drm/v3d: Scheduler and submission fixes and refactoring

[Claude Code Review Bot]

Overall Series Review

Subject: drm/v3d: Scheduler and submission fixes and refactoring
Author: =?utf-8?q?Ma=C3=ADra_Canal?= <mcanal@igalia.com>
Patches: 15
Reviewed: 2026-06-04T11:24:38.873719

---

This is a well-structured 14-patch series from Maira Canal that addresses scheduler and submission issues in the V3D driver. The series progresses logically from small cleanups/fixes (patches 1-6) through a DRM exec migration (patch 7), into a major refactoring of the submission code around a new `struct v3d_submit` abstraction (patches 8-12), adding out_sync validation (patch 13), and finally ensuring atomic submissions (patch 14).

The overall approach is sound. The `struct v3d_submit` abstraction is a clear improvement that unifies the previously ad-hoc per-ioctl submission patterns. The incremental decomposition makes the refactoring reviewable despite its scope.

There is one correctness issue (missing bounds check in `v3d_submit_add_job`), a few minor items, and the series is otherwise in good shape. Several patches already carry Reviewed-by tags from Tvrtko Ursulin and Iago Toral.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h

[Claude Code Review Bot]

Patch Review

Trivial cleanup removing an unused header. V3D has no display pipeline, so this is clearly correct.

Reviewed-by: Iago Toral. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Clear queue->active_job when v3d_fence_create() fails

[Claude Code Review Bot]

Patch Review

Good bugfix. Previously, if `v3d_fence_create()` failed, `queue->active_job` was left as a dangling pointer. The patch consolidates the early-return and fence-failure paths into a single `out_clean_job` label per function.

The BIN path correctly takes `queue->queue_lock` around the clear (because `v3d_overflow_mem_work()` can race), while RENDER/TFU/CSD paths clear lock-free (no concurrent reader). This distinction is well-documented in the commit message and correct.

Reviewed-by: Tvrtko Ursulin. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Use inline lock for dma fence initialization

[Claude Code Review Bot]

Patch Review

Passes `NULL` to `dma_fence_init()` so each fence uses its own inline spinlock, decoupling fence and lock lifetime. This relies on commit `1f32f310a13c` which added inline lock support. Enables patch 4's simplification.

Reviewed-by: Tvrtko Ursulin. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Replace spin_lock_irqsave() with spin_lock()

[Claude Code Review Bot]

Patch Review

Now that `queue_lock` is no longer used as the fence lock (patch 3), and the commit message correctly notes that only `v3d_overflow_mem_work()` and `v3d_bin_job_run()` (both workqueue context) take this lock, `irqsave` is unnecessary.

The conversion to `scoped_guard(spinlock, ...)` is clean. One subtlety worth noting: in `v3d_overflow_mem_work()`, the `goto out` inside the `scoped_guard` block correctly jumps past the guard's scope, which will automatically release the lock on scope exit. This is correct behavior with `scoped_guard`.

Reviewed-by: Tvrtko Ursulin. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Extract v3d_job_add_syncobjs() helper

[Claude Code Review Bot]

Patch Review

Clean extraction of syncobj dependency setup from `v3d_job_init()` into a separate helper. Adds `job->queue` field to `struct v3d_job` to enable the helper to check `se->wait_stage == job->queue`. No functional change, prepares for later patches.

The `// TODO: Investigate why this was filtered out for the IOCTL.` comments are carried forward from existing code (they're removed in patch 6).

Reviewed-by: Tvrtko Ursulin. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Reject invalid syncobj handles in submit ioctls

[Claude Code Review Bot]

Patch Review

Good fix. Previously, `-ENOENT` from `drm_sched_job_add_syncobj_dependency()` was silently swallowed for both zero handles (reasonable: no dependency) and invalid non-zero handles (userspace bug that should be reported). The patch distinguishes the two: skip the call when handle is zero, propagate the error otherwise. Removes the TODO comments.

Reviewed-by: Tvrtko Ursulin. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Migrate BO reservation locking to DRM exec

[Claude Code Review Bot]

Patch Review

Straightforward conversion from `drm_gem_(un)lock_reservations()` + `ww_acquire_ctx` to `drm_exec`. The `drm_exec_prepare_array()` call with `num_fences=1` replaces the manual `dma_resv_reserve_fences()` loop, which is a nice simplification.

One note: in the error path of `v3d_submit_cpu_ioctl()`, both `drm_exec_fini(&exec)` and `drm_exec_fini(&cpu_job->indirect_csd.exec)` are called unconditionally. Calling `drm_exec_fini()` on a zero-initialized `struct drm_exec` is safe (it checks internal state), so this is fine.

Reviewed-by: Tvrtko Ursulin. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls

[Claude Code Review Bot]

Patch Review

This is the largest patch in the series and introduces the core `struct v3d_submit` abstraction. Key observations:

**Missing bounds check in `v3d_submit_add_job()`**: The function increments `submit->job_count` and writes to `submit->jobs[]` without checking against `V3D_MAX_JOBS_PER_SUBMISSION` (which is 3):

```c
submit->jobs[submit->job_count++] = job;
```

While the current callers won't exceed 3 jobs, this is a latent out-of-bounds write waiting to happen if a future caller adds a 4th job. An `if (submit->job_count >= V3D_MAX_JOBS_PER_SUBMISSION) return ERR_PTR(-EINVAL);` check should be added before the array write.

**CSD ioctl path**: After `v3d_setup_csd_jobs_and_bos()` creates the jobs via the old `v3d_job_init()` path, they're manually added to the submit array:
```c
submit.jobs[submit.job_count++] = &job->base;
submit.jobs[submit.job_count++] = clean_job;
```
This bypasses `v3d_submit_add_job()` and its initialization, which is fine since these jobs were already initialized via the old path. But it means the CSD ioctl temporarily uses two patterns. This is cleaned up in patch 10 where `v3d_setup_csd_jobs_and_bos()` is converted to use `v3d_submit_add_job()` internally.

**`v3d_job_types[]` table**: Nice approach to centralizing job size and free callback. The array is indexed by `enum v3d_queue`, and the bounds check at the top (`queue >= V3D_MAX_QUEUES`) protects against out-of-range access.

**v3d_submit_jobs() error handling**: If `drm_sched_job_add_dependency()` fails midway through the loop, already-pushed jobs are scheduler-owned. The current code just unlocks the mutex and returns the error. This is addressed in patch 14, but at this point in the series there's a window where partial submissions can occur. This is acceptable as an intermediate state.

Overall a well-decomposed refactoring, but the missing bounds check should be addressed.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser

[Claude Code Review Bot]

Patch Review

Clean separation. The parsed args are stashed into `info->args`, and job creation is moved to `v3d_submit_cpu_ioctl()`. No functional change.

Reviewed-by: Tvrtko Ursulin. No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Convert submit helpers to operate on struct v3d_submit

[Claude Code Review Bot]

Patch Review

Large refactoring that generalizes the lock/fence/lookup helpers to operate on the full `struct v3d_submit`. Key changes:

- `v3d_submit_lock_reservations()` now iterates all jobs and locks the union of BOs under `DRM_EXEC_IGNORE_DUPLICATES`. The `drm_exec_retry_on_contention()` is correctly placed inside the `drm_exec_until_all_locked()` loop.
- `v3d_lookup_bos()` now takes `struct v3d_submit *` and always attaches BOs to the last job.
- `v3d_attach_fences_and_unlock_reservation()` walks all jobs and attaches the chain's last fence to every BO.
- `v3d_submit_jobs()` now also handles fence attachment and job put on success, becoming the single entry point.

The old `v3d_job_init()`, `v3d_job_allocate()`, and `v3d_job_deallocate()` are removed as dead code. Clean.

No issues beyond the inherited bounds check concern from patch 8.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Refactor CPU ioctl into unified submission chain

[Claude Code Review Bot]

Patch Review

Folds the indirect CSD path into the standard `struct v3d_submit` chain, eliminating the second `drm_exec` and the two-pass fence attachment. The CPU, CSD, and CLEAN_CACHE jobs all live in one `submit` instance.

The `WARN_ON(submit.jobs[1]->queue != V3D_CSD)` assertion is a good safety check:
```c
if (WARN_ON(submit.jobs[1]->queue != V3D_CSD)) {
    ret = -EINVAL;
    goto fail;
}
```

Removes `clean_job` and `exec` fields from `struct v3d_indirect_csd_info`, cleaning up the data structures.

No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Split BO fence attach from syncobj output handling

[Claude Code Review Bot]

Patch Review

Clean decomposition of `v3d_attach_fences_and_unlock_reservation()` into three distinct functions:
1. `v3d_submit_attach_object_fences()` - attach fences to BOs
2. `v3d_submit_unlock_reservations()` - release drm_exec
3. `v3d_submit_process_post_deps()` - replace out_sync syncobjs

`v3d_submit_jobs()` now calls these explicitly in sequence. No functional change.

No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Reject invalid out_sync handles in submit ioctls

[Claude Code Review Bot]

Patch Review

Moves out_sync syncobj lookup from the post-processing stage (where it was silently skipped on failure) to before submission, failing with `-ENOENT` for invalid non-zero handles. Mirrors the in_sync validation from patch 6.

The `v3d_put_multisync_post_deps()` is renamed to `v3d_submit_put_post_deps()` and now also handles the single `sync_out` cleanup.

**Minor note**: When the `sync_out` lookup fails early (e.g., in `v3d_submit_cl_ioctl()`), the function returns `-ENOENT` *before* any jobs have been allocated, so there's no need to clean up jobs. However, the multisync extension might have been parsed already, allocating `se.out_syncs`. The early `return -ENOENT` skips the `v3d_submit_put_post_deps()` cleanup. Let me verify...

Looking more carefully: the `sync_out` lookup is done after extension parsing but before job creation. If the lookup fails, `return -ENOENT` is used directly without going through the `fail` label. This leaks `se.out_syncs` if multi-sync extensions were partially parsed. However, the check `!(se.flags & DRM_V3D_EXT_ID_MULTI_SYNC)` ensures this code path only runs when multi-sync is NOT in use, so `se.out_syncs` would be NULL. This is correct.

No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Ensure atomic submissions in v3d_submit_jobs()

[Claude Code Review Bot]

Patch Review

This is the capstone fix. The submission is restructured into three phases:
1. **Arm** all jobs (get `done_fence`, take refcount)
2. **Wire** inter-job dependencies (can fail)
3. **Push** all jobs to scheduler (unconditional)

If phase 2 fails, all armed jobs have their finished fence marked with an error:
```c
for (int i = 0; i < submit->job_count; i++)
    dma_fence_set_error(&submit->jobs[i]->base.s_fence->finished, ret);
```
Then all jobs are pushed anyway, relying on the `run_job()` callbacks' existing `s_fence->finished.error` checks to skip execution.

The patch also adds the error check to `v3d_cpu_job_run()` and `v3d_cache_clean_job_run()`, which previously lacked them. BIN, RENDER, TFU, and CSD already had these checks (added/consolidated in patch 2).

The error path in `v3d_submit_jobs()` now handles reservation unlock and job put internally, so callers don't need `fail_unreserve` labels. The callers' error paths are updated accordingly with a new `fail_submit` label that skips job cleanup (since `v3d_submit_jobs` already did it on error).

**One concern with the error path**: On failure, `v3d_submit_jobs()` calls `v3d_submit_unlock_reservations(submit)` and `v3d_submit_put_jobs(submit)`, but does NOT call `v3d_submit_attach_object_fences()` or `v3d_submit_process_post_deps()`. This is correct -- on error, we don't want to attach fences to BOs or signal syncobjs to userspace. The jobs are pushed with error-marked fences, the scheduler will run them (no-op via the error check), and they'll be freed by the scheduler completion callback.

**Also correct**: The caller's `fail_submit` path still calls `v3d_submit_put_post_deps(sync_out, &se)` to release the sync_out reference and any multi-sync output references, which is the right thing since those were never consumed.

No issues. Well-designed error handling.

---

**Summary**: The series is well-decomposed and addresses real issues. The one actionable finding is:

1. **Patch 8**: `v3d_submit_add_job()` should check `submit->job_count < V3D_MAX_JOBS_PER_SUBMISSION` before writing to `submit->jobs[]` to prevent a potential out-of-bounds array write.

---
Generated by Claude Code Patch Reviewer