* [PATCH v2 01/14] drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
@ 2026-05-10 22:11 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:11 ` [PATCH v2 02/14] drm/v3d: Clear queue->active_job when v3d_fence_create() fails Maíra Canal
` (13 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:11 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, Maíra Canal
The V3D driver has no display pipeline, so nothing in the driver requires
drm_encoder.h. Remove the stale include.
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 02/14] drm/v3d: Clear queue->active_job when v3d_fence_create() fails
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
2026-05-10 22:11 ` [PATCH v2 01/14] drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h Maíra Canal
@ 2026-05-10 22:11 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 03/14] drm/v3d: Use inline lock for dma fence initialization Maíra Canal
` (12 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:11 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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 c01fa90def4c..66569b538e4e 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -206,12 +206,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().
@@ -228,7 +224,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);
@@ -256,6 +252,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)
@@ -265,10 +267,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;
@@ -282,7 +282,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);
@@ -303,6 +303,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 *
@@ -313,16 +317,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);
@@ -350,6 +352,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 *
@@ -361,10 +367,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;
@@ -372,7 +376,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);
@@ -399,6 +403,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Clear queue->active_job when v3d_fence_create() fails
2026-05-10 22:11 ` [PATCH v2 02/14] drm/v3d: Clear queue->active_job when v3d_fence_create() fails Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 03/14] drm/v3d: Use inline lock for dma fence initialization
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
2026-05-10 22:11 ` [PATCH v2 01/14] drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h Maíra Canal
2026-05-10 22:11 ` [PATCH v2 02/14] drm/v3d: Clear queue->active_job when v3d_fence_create() fails Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 04/14] drm/v3d: Replace spin_lock_irqsave() with spin_lock() Maíra Canal
` (11 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 04/14] drm/v3d: Replace spin_lock_irqsave() with spin_lock()
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (2 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 03/14] drm/v3d: Use inline lock for dma fence initialization Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 05/14] drm/v3d: Extract v3d_job_add_syncobjs() helper Maíra Canal
` (10 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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.
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 66569b538e4e..cc3212e2cb5d 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -204,7 +204,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;
@@ -212,13 +211,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);
@@ -254,9 +253,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Replace spin_lock_irqsave() with spin_lock()
2026-05-10 22:12 ` [PATCH v2 04/14] drm/v3d: Replace spin_lock_irqsave() with spin_lock() Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
After patch 3 decoupled the fence lock, `queue_lock` is no longer shared with any IRQ handler, so plain `spin_lock()` suffices. The conversion to `scoped_guard(spinlock, ...)` is clean.
One minor style observation: the `out_clean_job:` path in `v3d_bin_job_run` uses `scoped_guard` for a single assignment:
```c
scoped_guard(spinlock, &queue->queue_lock) {
queue->active_job = NULL;
}
```
This is fine but slightly verbose for a one-liner. Not worth changing.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 05/14] drm/v3d: Extract v3d_job_add_syncobjs() helper
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (3 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 04/14] drm/v3d: Replace spin_lock_irqsave() with spin_lock() Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 06/14] drm/v3d: Reject invalid syncobj handles in submit ioctls Maíra Canal
` (9 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, Maíra Canal
Move the syncobj dependency setup out of v3d_job_init() into its own
v3d_job_add_syncobjs() helper. 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_submit.c | 71 ++++++++++++++++++++++++----------------
1 file changed, 43 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index d0f949a49dd2..f9231a87b963 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -162,14 +162,52 @@ 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, enum v3d_queue queue)
+{
+ 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 == 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;
@@ -180,32 +218,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, queue);
+ if (ret)
+ goto fail_job_init;
/* CPU jobs don't require hardware resources */
if (queue != V3D_CPU) {
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 06/14] drm/v3d: Reject invalid syncobj handles in submit ioctls
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (4 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 05/14] drm/v3d: Extract v3d_job_add_syncobjs() helper Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 07/14] drm/v3d: Migrate BO reservation locking to DRM exec Maíra Canal
` (8 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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 f9231a87b963..30cb94ee1a2a 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -171,12 +171,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 == queue) {
@@ -190,11 +189,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Reject invalid syncobj handles in submit ioctls
2026-05-10 22:12 ` [PATCH v2 06/14] drm/v3d: Reject invalid syncobj handles in submit ioctls Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Good fix. Previously, `-ENOENT` from `drm_sched_job_add_syncobj_dependency()` was silently swallowed for both zero and non-zero handles. Now zero handles are skipped (no dependency to add), and non-zero invalid handles properly propagate the error.
**Minor note on UAPI compatibility**: Userspace that previously passed non-zero invalid in_sync handles and relied on silent success will now get `-ENOENT`. This is the correct behavior but is technically a UAPI change. The commit message acknowledges this is a "userspace bug" scenario, and this seems like the right call — silently ignoring broken handles masks real bugs.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 07/14] drm/v3d: Migrate BO reservation locking to DRM exec
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (5 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 06/14] drm/v3d: Reject invalid syncobj handles in submit ioctls Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls Maíra Canal
` (7 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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.
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 5a0b9da2c3aa..788a45c60290 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>
@@ -418,7 +419,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 30cb94ee1a2a..0ac88e5763b5 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;
}
@@ -259,7 +258,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)
@@ -274,7 +273,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*/
@@ -305,7 +304,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;
@@ -338,7 +337,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
@@ -496,7 +495,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. */
@@ -911,7 +910,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);
@@ -991,7 +990,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;
@@ -1040,7 +1039,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);
@@ -1054,8 +1053,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);
@@ -1082,7 +1080,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);
@@ -1138,7 +1136,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;
@@ -1147,7 +1145,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);
@@ -1182,7 +1180,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]);
@@ -1209,8 +1207,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;
@@ -1241,7 +1238,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);
@@ -1254,8 +1251,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);
@@ -1293,7 +1289,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)) {
@@ -1344,7 +1340,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;
}
@@ -1378,14 +1374,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:
@@ -1400,13 +1396,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (6 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 07/14] drm/v3d: Migrate BO reservation locking to DRM exec Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 09/14] drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser Maíra Canal
` (6 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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 -> lookup BOs -> lock
reservations -> attach perfmon -> 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 | 355 ++++++++++++++++++++++-----------------
2 files changed, 218 insertions(+), 158 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 788a45c60290..fc12e5215fb0 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 0ac88e5763b5..c51474403d2d 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -242,6 +242,80 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
return ret;
}
+static int
+v3d_submit_add_job(struct v3d_submit *submit, void **container, size_t size,
+ void (*free)(struct kref *ref), enum v3d_queue queue)
+{
+ struct v3d_file_priv *v3d_priv = submit->file_priv->driver_priv;
+ struct v3d_dev *v3d = submit->v3d;
+ struct v3d_job *job;
+ int ret;
+
+ *container = kzalloc(size, GFP_KERNEL);
+ if (!*container)
+ return -ENOMEM;
+
+ job = *container;
+ job->v3d = v3d;
+ job->free = free;
+ job->file_priv = v3d_priv;
+
+ 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 0;
+
+fail_sched_job:
+ drm_sched_job_cleanup(&job->base);
+fail_free:
+ kfree(*container);
+ *container = NULL;
+ return ret;
+}
+
+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
v3d_push_job(struct v3d_job *job)
{
@@ -255,6 +329,46 @@ v3d_push_job(struct v3d_job *job)
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 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 void
v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
struct v3d_job *job,
@@ -902,18 +1016,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;
@@ -933,131 +1044,82 @@ 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));
+ ret = v3d_submit_add_job(&submit, (void **)&bin, sizeof(*bin),
+ v3d_job_free, V3D_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);
- goto fail;
- }
-
bin->start = args->bcl_start;
bin->end = args->bcl_end;
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, V3D_BIN);
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,
+ ret = v3d_submit_add_job(&submit, (void **)&render, sizeof(*render),
+ v3d_render_job_free, V3D_RENDER);
+ if (ret)
+ 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, V3D_RENDER);
+ if (ret)
+ goto fail;
+
+ if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
+ ret = v3d_submit_add_job(&submit, (void **)&clean_job,
+ sizeof(*clean_job), v3d_job_free,
+ V3D_CACHE_CLEAN);
+ 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;
- }
+ ret = v3d_attach_perfmon_to_jobs(&submit, args->perfmon_id);
+ if (ret)
+ goto fail_unreserve;
- 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;
@@ -1076,14 +1138,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);
@@ -1098,16 +1159,14 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
}
}
- ret = v3d_job_allocate(v3d, (void *)&job, sizeof(*job));
+ ret = v3d_submit_add_job(&submit, (void **)&job, sizeof(*job),
+ v3d_job_free, V3D_TFU);
+ if (ret)
+ goto fail;
+
+ ret = v3d_job_add_syncobjs(&job->base, file_priv, args->in_sync, &se, V3D_TFU);
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);
goto fail;
- }
job->base.bo = kzalloc_objs(*job->base.bo, ARRAY_SIZE(args->bo_handles));
if (!job->base.bo) {
@@ -1136,26 +1195,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;
@@ -1174,21 +1234,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;
}
@@ -1206,55 +1264,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls
2026-05-10 22:12 ` [PATCH v2 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the core abstraction patch. `struct v3d_submit` groups `v3d_dev`, `file_priv`, `drm_exec`, and a fixed-size jobs array.
**Concern: Missing bounds check in v3d_submit_add_job()**
`v3d_submit_add_job()` does:
```c
submit->jobs[submit->job_count++] = job;
```
without checking `submit->job_count < V3D_MAX_JOBS_PER_SUBMISSION`. All current callers respect the limit (CL submits at most 3, CSD 2, TFU 1, CPU indirect CSD 3), but a missing bounds check on a stack-allocated fixed-size array is a latent buffer overflow waiting for someone to add a fourth job. I'd recommend adding:
```c
if (submit->job_count >= V3D_MAX_JOBS_PER_SUBMISSION)
return -EINVAL;
```
**CSD ioctl manually adds jobs to the array**: In the CSD ioctl, after calling the old `v3d_setup_csd_jobs_and_bos()`:
```c
submit.jobs[submit.job_count++] = &job->base;
submit.jobs[submit.job_count++] = clean_job;
```
These jobs were created by the old `v3d_job_init()` path, not by `v3d_submit_add_job()`. This is a transitional state that gets cleaned up in patch 10 — acceptable for bisectability.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 09/14] drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (7 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 10/14] drm/v3d: Convert submit helpers to operate on struct v3d_submit Maíra Canal
` (5 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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.
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 fc12e5215fb0..ec01668bacb6 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -425,6 +425,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 c51474403d2d..ef003ca6b42a 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -600,6 +600,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,
@@ -607,9 +608,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. */
@@ -1370,6 +1369,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 10/14] drm/v3d: Convert submit helpers to operate on struct v3d_submit
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (8 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 09/14] drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 11/14] drm/v3d: Refactor CPU ioctl into unified submission chain Maíra Canal
` (4 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, Maíra Canal
Generalize v3d_lookup_bos(), v3d_lock_bo_reservations() (renamed to
v3d_submit_lock_reservations()), v3d_attach_fences_and_unlock_reservation()
and v3d_setup_csd_jobs_and_bos() so they all take a struct v3d_submit
and operate on every job in the chain.
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.
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 mutex/push/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 | 295 ++++++++++++++-------------------------
1 file changed, 108 insertions(+), 187 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index ef003ca6b42a..899128fc2bd5 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
@@ -142,25 +159,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, enum v3d_queue queue)
@@ -201,47 +199,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->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, queue);
- 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 int
v3d_submit_add_job(struct v3d_submit *submit, void **container, size_t size,
void (*free)(struct kref *ref), enum v3d_queue queue)
@@ -370,31 +327,34 @@ v3d_submit_cleanup_jobs(struct v3d_submit *submit)
}
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;
@@ -402,9 +362,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);
@@ -412,46 +372,36 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
}
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 = NULL;
+ struct v3d_job *clean_job = NULL;
int ret;
- ret = v3d_job_allocate(v3d, (void *)job, sizeof(**job));
+ ret = v3d_submit_add_job(submit, (void **)&job, sizeof(*job),
+ v3d_job_free, V3D_CSD);
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;
- }
-
- ret = v3d_job_allocate(v3d, (void *)clean_job, sizeof(**clean_job));
+ ret = v3d_job_add_syncobjs(&job->base, submit->file_priv, args->in_sync,
+ se, V3D_CSD);
if (ret)
return ret;
- 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;
- (*job)->args = *args;
-
- ret = v3d_lookup_bos(&v3d->drm, file_priv, *clean_job,
- args->bo_handles, args->bo_handle_count);
+ ret = v3d_submit_add_job(submit, (void **)&clean_job, sizeof(*clean_job),
+ v3d_job_free, V3D_CACHE_CLEAN);
if (ret)
return ret;
- return v3d_lock_bo_reservations(*clean_job, exec);
+ ret = v3d_lookup_bos(submit, args->bo_handles, args->bo_handle_count);
+ if (ret)
+ return ret;
+
+ return v3d_submit_lock_reservations(submit);
}
static void
@@ -1086,14 +1036,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
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;
@@ -1105,18 +1052,14 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
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_attach_fences_and_unlock_reservation(&submit, args->out_sync, &se);
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);
@@ -1194,7 +1137,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, &submit.exec);
+ ret = v3d_submit_lock_reservations(&submit);
if (ret)
goto fail;
@@ -1202,17 +1145,14 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
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_attach_fences_and_unlock_reservation(&submit, args->out_sync, &se);
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);
@@ -1236,8 +1176,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]);
@@ -1263,14 +1201,10 @@ 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;
@@ -1279,18 +1213,14 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
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_attach_fences_and_unlock_reservation(&submit, args->out_sync, &se);
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);
@@ -1321,13 +1251,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)) {
@@ -1335,7 +1266,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
return -EINVAL;
}
- ret = v3d_job_allocate(v3d, (void *)&cpu_job, sizeof(*cpu_job));
+ ret = v3d_submit_add_job(&submit, (void **)&cpu_job, sizeof(*cpu_job),
+ v3d_job_free, V3D_CPU);
if (ret)
return ret;
@@ -1362,34 +1294,31 @@ 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_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, V3D_CPU);
+ 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;
+
+ 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;
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;
}
@@ -1421,36 +1350,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);
kvfree(cpu_job->timestamp_query.queries);
kvfree(cpu_job->performance_query.queries);
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Convert submit helpers to operate on struct v3d_submit
2026-05-10 22:12 ` [PATCH v2 10/14] drm/v3d: Convert submit helpers to operate on struct v3d_submit Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Large patch that generalizes the helper functions. Key changes:
1. `v3d_submit_lock_reservations()` iterates all jobs and locks the union of BOs under one `drm_exec` with `DRM_EXEC_IGNORE_DUPLICATES`.
2. `v3d_attach_fences_and_unlock_reservation()` attaches the *last* job's fence to all BOs across all jobs.
3. `v3d_lookup_bos()` now takes `struct v3d_submit` and operates on the last job.
The CPU ioctl uses two `struct v3d_submit` instances (`submit` and `indirect_submit`) — a transitional state cleaned up in patch 11.
The `v3d_job_allocate()`, `v3d_job_deallocate()`, and `v3d_job_init()` functions are removed since `v3d_submit_add_job()` subsumes them. Good cleanup.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 11/14] drm/v3d: Refactor CPU ioctl into unified submission chain
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (9 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 10/14] drm/v3d: Convert submit helpers to operate on struct v3d_submit Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 12/14] drm/v3d: Split BO fence attach from syncobj output handling Maíra Canal
` (3 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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 | 80 ++++++++++------------------------------
2 files changed, 20 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index ec01668bacb6..071d919fe860 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -422,9 +422,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.
*/
@@ -443,9 +440,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 899128fc2bd5..6506acb6992e 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -1250,15 +1250,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)) {
@@ -1292,86 +1287,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, V3D_CPU);
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;
-
- 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 first, so v3d_setup_csd_jobs_and_bos(), which
+ * locks all jobs' BOs at its end, picks them up too in the case of indirect
+ * CSD.
+ */
if (args->bo_handle_count) {
ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);
if (ret)
goto fail;
+ }
+ 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] */
+ cpu_job->indirect_csd.job = container_of(submit.jobs[1], struct v3d_csd_job,
+ base);
+ } else {
ret = v3d_submit_lock_reservations(&submit);
if (ret)
goto fail;
}
- mutex_lock(&v3d->sched_lock);
- v3d_push_job(&cpu_job->base);
+ ret = v3d_submit_jobs(&submit);
+ if (ret)
+ goto fail_unreserve;
- 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_attach_fences_and_unlock_reservation(&submit, 0, &se);
v3d_submit_put_jobs(&submit);
- v3d_submit_put_jobs(&indirect_submit);
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);
kvfree(cpu_job->timestamp_query.queries);
kvfree(cpu_job->performance_query.queries);
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Refactor CPU ioctl into unified submission chain
2026-05-10 22:12 ` [PATCH v2 11/14] drm/v3d: Refactor CPU ioctl into unified submission chain Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Merges the indirect CSD path into a single `struct v3d_submit`. The two-pass fence attachment is eliminated.
There's a subtlety worth noting: `v3d_lookup_bos()` is called *before* `v3d_setup_csd_jobs_and_bos()`, and the comment explains why:
```c
/* Look up the CPU jobs' BOs first, so v3d_setup_csd_jobs_and_bos(), which
* locks all jobs' BOs at its end, picks them up too in the case of indirect
* CSD.
*/
```
This ordering dependency is correct — `v3d_setup_csd_jobs_and_bos()` calls `v3d_submit_lock_reservations()` internally, which iterates all jobs' BOs.
**Observation**: After this patch, the `v3d_indirect_csd_info` struct still has `struct v3d_csd_job *job` and `struct drm_gem_object *indirect` members. The `job` pointer is set via `container_of(submit.jobs[1], ...)`. This cross-reference between the submit array and the indirect_csd struct is fine since both are stack-local and have the same lifetime.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 12/14] drm/v3d: Split BO fence attach from syncobj output handling
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (10 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 11/14] drm/v3d: Refactor CPU ioctl into unified submission chain Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 13/14] drm/v3d: Reject invalid out_sync handles in submit ioctls Maíra Canal
` (2 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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.
Also, move fence attachment and reservation unlocking to v3d_submit_jobs(),
so that callers no longer have to deal with this sequence. Now, after
submitting the jobs, callers are only responsible for calling
v3d_submit_process_post_deps().
No functional change; just code consolidation.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/v3d/v3d_submit.c | 57 ++++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 6506acb6992e..08a7fa9bc366 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -273,6 +273,25 @@ v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, u32 perfmon_id)
return 0;
}
+static void
+v3d_submit_attach_object_fences(struct v3d_submit *submit)
+{
+ struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
+
+ /* 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);
+ }
+ }
+}
+
static void
v3d_push_job(struct v3d_job *job)
{
@@ -307,6 +326,13 @@ v3d_submit_jobs(struct v3d_submit *submit)
}
}
+ mutex_unlock(&v3d->sched_lock);
+
+ v3d_submit_attach_object_fences(submit);
+ v3d_submit_unlock_reservations(submit);
+
+ return 0;
+
err:
mutex_unlock(&v3d->sched_lock);
return ret;
@@ -327,28 +353,13 @@ v3d_submit_cleanup_jobs(struct v3d_submit *submit)
}
static void
-v3d_attach_fences_and_unlock_reservation(struct v3d_submit *submit,
- u32 out_sync, struct v3d_submit_ext *se)
+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;
- /* 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);
- }
- }
-
- v3d_submit_unlock_reservations(submit);
-
/* Update the return sync object for the job */
/* If it only supports a single signal semaphore*/
if (!has_multisync) {
@@ -1052,8 +1063,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_attach_fences_and_unlock_reservation(&submit, args->out_sync, &se);
-
+ v3d_submit_process_post_deps(&submit, args->out_sync, &se);
v3d_submit_put_jobs(&submit);
return 0;
@@ -1145,8 +1155,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_attach_fences_and_unlock_reservation(&submit, args->out_sync, &se);
-
+ v3d_submit_process_post_deps(&submit, args->out_sync, &se);
v3d_submit_put_jobs(&submit);
return 0;
@@ -1213,8 +1222,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_attach_fences_and_unlock_reservation(&submit, args->out_sync, &se);
-
+ v3d_submit_process_post_deps(&submit, args->out_sync, &se);
v3d_submit_put_jobs(&submit);
return 0;
@@ -1322,8 +1330,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_attach_fences_and_unlock_reservation(&submit, 0, &se);
-
+ v3d_submit_process_post_deps(&submit, 0, &se);
v3d_submit_put_jobs(&submit);
return 0;
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Split BO fence attach from syncobj output handling
2026-05-10 22:12 ` [PATCH v2 12/14] drm/v3d: Split BO fence attach from syncobj output handling Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Clean decomposition of `v3d_attach_fences_and_unlock_reservation()` into:
1. `v3d_submit_attach_object_fences()` — BO fence attachment
2. `v3d_submit_unlock_reservations()` — drm_exec cleanup
3. `v3d_submit_process_post_deps()` — syncobj output handling
Both (1) and (2) are folded into `v3d_submit_jobs()`, so callers only need to call `v3d_submit_process_post_deps()` afterward.
**Observation on v3d_submit_jobs() control flow**: After this patch, on success, `v3d_submit_jobs()` unlocks reservations before returning. On error (the `err:` label), it does *not* unlock reservations, leaving that to the caller. The callers all have `fail_unreserve:` labels that call `v3d_submit_unlock_reservations()`. This is correct but asymmetric — patch 14 resolves this by having the error path also unlock.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 13/14] drm/v3d: Reject invalid out_sync handles in submit ioctls
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (11 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 12/14] drm/v3d: Split BO fence attach from syncobj output handling Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-10 22:12 ` [PATCH v2 14/14] drm/v3d: Ensure atomic submissions in v3d_submit_jobs() Maíra Canal
2026-05-16 5:59 ` Claude review: drm/v3d: Scheduler and submission fixes and refactoring Claude Code Review Bot
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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 | 46 +++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 08a7fa9bc366..afe54e73a461 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -353,17 +353,15 @@ v3d_submit_cleanup_jobs(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);
@@ -416,10 +414,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;
@@ -978,6 +979,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;
@@ -1004,6 +1006,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) {
ret = v3d_submit_add_job(&submit, (void **)&bin, sizeof(*bin),
v3d_job_free, V3D_BIN);
@@ -1063,7 +1071,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_submit_process_post_deps(&submit, args->out_sync, &se);
+ v3d_submit_process_post_deps(&submit, sync_out, &se);
v3d_submit_put_jobs(&submit);
return 0;
@@ -1072,7 +1080,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;
}
@@ -1092,6 +1100,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;
@@ -1111,6 +1120,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;
+ }
+
ret = v3d_submit_add_job(&submit, (void **)&job, sizeof(*job),
v3d_job_free, V3D_TFU);
if (ret)
@@ -1155,7 +1170,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_submit_process_post_deps(&submit, args->out_sync, &se);
+ v3d_submit_process_post_deps(&submit, sync_out, &se);
v3d_submit_put_jobs(&submit);
return 0;
@@ -1164,7 +1179,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;
}
@@ -1184,6 +1199,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;
@@ -1210,6 +1226,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;
@@ -1222,7 +1244,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_submit_process_post_deps(&submit, args->out_sync, &se);
+ v3d_submit_process_post_deps(&submit, sync_out, &se);
v3d_submit_put_jobs(&submit);
return 0;
@@ -1231,7 +1253,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;
}
@@ -1330,7 +1352,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail_unreserve;
- v3d_submit_process_post_deps(&submit, 0, &se);
+ v3d_submit_process_post_deps(&submit, NULL, &se);
v3d_submit_put_jobs(&submit);
return 0;
@@ -1339,7 +1361,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);
kvfree(cpu_job->timestamp_query.queries);
kvfree(cpu_job->performance_query.queries);
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Reject invalid out_sync handles in submit ioctls
2026-05-10 22:12 ` [PATCH v2 13/14] drm/v3d: Reject invalid out_sync handles in submit ioctls Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Mirrors patch 6's treatment of in_sync for the out_sync path. Previously, a non-zero invalid `out_sync` handle was silently ignored. Now, `drm_syncobj_find()` is done up-front and returns `-ENOENT` on failure.
`v3d_put_multisync_post_deps()` is generalized into `v3d_submit_put_post_deps()` which handles both the single-sync case (`sync_out`) and multi-sync case (`se->out_syncs`).
**Same UAPI note as patch 6**: Userspace passing invalid non-zero `out_sync` handles will now get `-ENOENT` instead of silent success. Correct behavior, potential compat concern if buggy userspace exists.
**Minor observation**: The early return paths for `sync_out` lookup failure happen before any job allocation, so there's no cleanup needed — this is correct:
```c
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;
}
```
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 14/14] drm/v3d: Ensure atomic submissions in v3d_submit_jobs()
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (12 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 13/14] drm/v3d: Reject invalid out_sync handles in submit ioctls Maíra Canal
@ 2026-05-10 22:12 ` Maíra Canal
2026-05-16 5:59 ` Claude review: " Claude Code Review Bot
2026-05-16 5:59 ` Claude review: drm/v3d: Scheduler and submission fixes and refactoring Claude Code Review Bot
14 siblings, 1 reply; 30+ messages in thread
From: Maíra Canal @ 2026-05-10 22:12 UTC (permalink / raw)
To: Melissa Wen, Iago Toral, Tvrtko Ursulin, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Christian König
Cc: kernel-dev, dri-devel, 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 | 76 +++++++++++++++++++++++-----------------
2 files changed, 50 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index cc3212e2cb5d..6e890b8547e5 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -670,6 +670,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;
@@ -693,6 +696,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 afe54e73a461..11ba3c977e2d 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -292,19 +292,6 @@ v3d_submit_attach_object_fences(struct v3d_submit *submit)
}
}
-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)
{
@@ -316,16 +303,23 @@ v3d_submit_jobs(struct v3d_submit *submit)
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);
@@ -334,7 +328,17 @@ v3d_submit_jobs(struct v3d_submit *submit)
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);
+
return ret;
}
@@ -1068,8 +1072,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
goto fail_unreserve;
ret = v3d_submit_jobs(&submit);
- if (ret)
- goto fail_unreserve;
+ if (ret) {
+ v3d_submit_put_jobs(&submit);
+ v3d_submit_put_post_deps(sync_out, &se);
+ return ret;
+ }
v3d_submit_process_post_deps(&submit, sync_out, &se);
v3d_submit_put_jobs(&submit);
@@ -1167,16 +1174,17 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
goto fail;
ret = v3d_submit_jobs(&submit);
- if (ret)
- goto fail_unreserve;
+ if (ret) {
+ v3d_submit_put_jobs(&submit);
+ v3d_submit_put_post_deps(sync_out, &se);
+ return ret;
+ }
v3d_submit_process_post_deps(&submit, sync_out, &se);
v3d_submit_put_jobs(&submit);
return 0;
-fail_unreserve:
- v3d_submit_unlock_reservations(&submit);
fail:
v3d_submit_cleanup_jobs(&submit);
v3d_submit_put_post_deps(sync_out, &se);
@@ -1241,8 +1249,11 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
goto fail_unreserve;
ret = v3d_submit_jobs(&submit);
- if (ret)
- goto fail_unreserve;
+ if (ret) {
+ v3d_submit_put_jobs(&submit);
+ v3d_submit_put_post_deps(sync_out, &se);
+ return ret;
+ }
v3d_submit_process_post_deps(&submit, sync_out, &se);
v3d_submit_put_jobs(&submit);
@@ -1349,16 +1360,17 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
}
ret = v3d_submit_jobs(&submit);
- if (ret)
- goto fail_unreserve;
+ if (ret) {
+ v3d_submit_put_jobs(&submit);
+ v3d_submit_put_post_deps(NULL, &se);
+ return ret;
+ }
v3d_submit_process_post_deps(&submit, NULL, &se);
v3d_submit_put_jobs(&submit);
return 0;
-fail_unreserve:
- v3d_submit_unlock_reservations(&submit);
fail:
v3d_submit_cleanup_jobs(&submit);
v3d_submit_put_post_deps(NULL, &se);
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Claude review: drm/v3d: Ensure atomic submissions in v3d_submit_jobs()
2026-05-10 22:12 ` [PATCH v2 14/14] drm/v3d: Ensure atomic submissions in v3d_submit_jobs() Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
0 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
The key correctness fix. Restructures `v3d_submit_jobs()` into three phases:
1. **Arm** all jobs (`drm_sched_job_arm()`)
2. **Wire dependencies** between consecutive jobs
3. **Push** all jobs to the scheduler
If phase 2 fails, all armed jobs get their finished fence marked with an error and are pushed anyway. The `run_job()` callbacks (BIN, RENDER, TFU, CSD, CPU, CACHE_CLEAN) all check `s_fence->finished.error` and skip execution.
This patch also adds the error check to `v3d_cpu_job_run()` and `v3d_cache_clean_job_run()`, which previously lacked it (the other queue types already had it from patch 2's pre-existing code).
**One observation about the error path**: On failure, `v3d_submit_jobs()` calls `v3d_submit_unlock_reservations()` but does *not* call `v3d_submit_attach_object_fences()`. This means on error, the BOs' reservation objects don't get the errored fence attached. This is correct — on error, the caller returns the error to userspace, and the errored jobs will complete via the scheduler without touching hardware. No BO fences are needed.
**Error path cleanup in callers**: The callers now handle `v3d_submit_jobs()` failure differently than other failures:
```c
ret = v3d_submit_jobs(&submit);
if (ret) {
v3d_submit_put_jobs(&submit);
v3d_submit_put_post_deps(sync_out, &se);
return ret;
}
```
This is because after `v3d_submit_jobs()` (even on error), the jobs have been armed+pushed and are now owned by the scheduler. The caller uses `v3d_submit_put_jobs()` (kref_put) instead of `v3d_submit_cleanup_jobs()` (drm_sched_job_cleanup + kfree). This is correct.
**Potential concern**: The `v3d_submit_cpu_ioctl()` error-after-submit path does:
```c
if (ret) {
v3d_submit_put_jobs(&submit);
v3d_submit_put_post_deps(NULL, &se);
return ret;
}
```
This skips the `kvfree(cpu_job->timestamp_query.queries)` and `kvfree(cpu_job->performance_query.queries)` cleanup that happens in the `fail:` label. However, since the jobs are now scheduler-owned and will be freed through the scheduler's cleanup path (via `v3d_job_free`), those allocations should be freed when the job's kref drops to zero. I'd verify that `v3d_job_free()` (the `free` callback) handles freeing these sub-allocations. If it does, this is correct. If not, this would be a memory leak on this specific error path.
Overall, this is a solid and well-organized series. The bounds check in `v3d_submit_add_job()` is the main thing I'd want added before merging.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread
* Claude review: drm/v3d: Scheduler and submission fixes and refactoring
2026-05-10 22:11 [PATCH v2 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
` (13 preceding siblings ...)
2026-05-10 22:12 ` [PATCH v2 14/14] drm/v3d: Ensure atomic submissions in v3d_submit_jobs() Maíra Canal
@ 2026-05-16 5:59 ` Claude Code Review Bot
14 siblings, 0 replies; 30+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:59 UTC (permalink / raw)
To: dri-devel-reviews
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-05-16T15:59:26.532315
---
This is a well-structured 14-patch series from Maíra Canal that cleans up, fixes, and refactors the V3D scheduler and submission code. The series progresses logically: trivial cleanups first (patches 1-4), then preparatory refactors (5-6), a mechanical API migration (7), the core abstraction introduction (8), and incremental conversions (9-12) leading to the key correctness fixes (13-14, atomic submissions).
The overall design is sound. The `struct v3d_submit` abstraction is well-motivated — the existing code managed multi-job submission chains with ad-hoc local variables, and this centralizes the pattern cleanly. The final atomicity fix in patch 14 (arm-all-then-push) is the main payoff and is correct in approach.
I have one concern worth flagging and a few minor observations, but nothing that I'd consider a blocker.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 30+ messages in thread