[PATCH v3 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups

[PATCH v3 0/2] drm/v3d: Fix indirect CSD jobs with zeroed workgroups

[Maíra Canal]

Hi,

Indirect CSD lets userspace defer the workgroup counts to a GPU buffer
that is only filled at runtime, so the counts are unknown at submission
time and can legitimately turn out to be zero.

However, exercing this case exposed two issues in the CSD path.

  1. Virtual address leaks when the indirect CSD has zeroed workgroups.

  2. CSD jobs with zeroed workgroups shouldn't be submitted to hardware.

This series intends to address both issues.

Best regards,
- Maíra

---
v1 -> v2: https://lore.kernel.org/r/20260530-v3d-fix-indirect-csd-v1-0-15533948663f@igalia.com

- [2/2] Don't check the whole cfg[0-2], check only the number of workgroups (Iago Toral)
- [2/2] Add a comment about how the HW interprets 0 (Iago Toral)

v2 -> v3: https://lore.kernel.org/r/20260601-v3d-fix-indirect-csd-v2-0-aaebf035b936@igalia.com

- [1/2, 2/2] Add Iago's R-b (Iago Toral)
- [2/2] Adjust the comment to make it more accurate (Iago Toral)

---
Maíra Canal (2):
      drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups
      drm/v3d: Skip CSD when it has zeroed workgroups

 drivers/gpu/drm/v3d/v3d_sched.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
---
base-commit: ae0383e5a9a4b12d68c76c4769857def4665deff
change-id: 20260516-v3d-fix-indirect-csd-6806cf20a57a

[PATCH v3 1/2] drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups

[Maíra Canal]

v3d_rewrite_csd_job_wg_counts_from_indirect() maps both the indirect
buffer and the workgroup buffer and is expected to release them before
returning. When any of the workgroup counts read from the buffer is zero,
the function bailed out early and skipped the cleanup, leaking the vaddr
mappings of both BOs.

Jump to the cleanup path instead of returning directly, so the mappings
are always dropped.

Cc: stable@vger.kernel.org
Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 94bf628dc91c..47f83936cd73 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -403,7 +403,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
 	wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
 
 	if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
-		return;
+		goto unmap_bo;
 
 	args->cfg[0] = wg_counts[0] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
 	args->cfg[1] = wg_counts[1] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
@@ -428,6 +428,7 @@ v3d_rewrite_csd_job_wg_counts_from_indirect(struct v3d_cpu_job *job)
 		}
 	}
 
+unmap_bo:
 	v3d_put_bo_vaddr(indirect);
 	v3d_put_bo_vaddr(bo);
 }

-- 
2.54.0

[PATCH v3 2/2] drm/v3d: Skip CSD when it has zeroed workgroups

[Maíra Canal]

A compute shader dispatch encodes its workgroup counts in the CFG0..CFG2
registers. Kicking off a dispatch with a zero count in any of the three
dimensions is invalid. First, the hardware will process 0 as 65536,
while the user-space driver exposes a maximum of 65535. Over that, a
submission with a zeroed workgroup dimension should be a no-op.

These zeroed counts can reach the dispatch path through an indirect CSD
job, whose workgroup counts are only known once the indirect buffer is
read and may legitimately be zero, but such scenario should only result in
a no-op.

Don't submit the job to the hardware when any of the workgroup counts is
zero, so the job completes immediately instead of running the shader.

Cc: stable@vger.kernel.org
Fixes: d223f98f0209 ("drm/v3d: Add support for compute shader dispatch.")
Suggested-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 47f83936cd73..6678d62e5bd0 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -352,6 +352,16 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
 		return NULL;
 	}
 
+	/* The HW interprets a workgroup size of 0 as 65536; however, the
+	 * user-space driver exposes a maximum of 65535. Therefore, a 0 in
+	 * any dimension means that we have no workgroups and the compute
+	 * shader should not be dispatched.
+	 */
+	if (!V3D_GET_FIELD(job->args.cfg[0], V3D_CSD_QUEUED_CFG0_NUM_WGS_X) ||
+	    !V3D_GET_FIELD(job->args.cfg[1], V3D_CSD_QUEUED_CFG1_NUM_WGS_Y) ||
+	    !V3D_GET_FIELD(job->args.cfg[2], V3D_CSD_QUEUED_CFG2_NUM_WGS_Z))
+		return NULL;
+
 	v3d->queue[V3D_CSD].active_job = &job->base;
 
 	v3d_invalidate_caches(v3d);

-- 
2.54.0

Claude review: drm/v3d: Skip CSD when it has zeroed workgroups

[Claude Code Review Bot]

Patch Review

**Status: Good, with one minor style observation.**

This patch addresses two things:

**Part A — Always rewrite CFG[0..2] from the indirect buffer.** In the rewrite function, the cfg writes are now done unconditionally *before* the zero check:

```c
 	args->cfg[0] = wg_counts[0] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
 	args->cfg[1] = wg_counts[1] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
 	args->cfg[2] = wg_counts[2] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
 
+	if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
+		goto unmap_bo;
```

This is the v3→v4 improvement. It ensures CFG[0..2] always reflect the actual indirect buffer contents rather than preserving stale userspace values. The `goto unmap_bo` still correctly skips the `num_batches` calculation, which would produce `0 - 1 = 0xFFFFFFFF` (triggering the existing `WARN_ON(args->cfg[4] == ~0)`) if any count is zero.

**Part B — Skip hardware dispatch for zero workgroups.** In `v3d_csd_job_run()`:

```c
+	if (!V3D_GET_FIELD(job->args.cfg[0], V3D_CSD_QUEUED_CFG0_NUM_WGS_X) ||
+	    !V3D_GET_FIELD(job->args.cfg[1], V3D_CSD_QUEUED_CFG1_NUM_WGS_Y) ||
+	    !V3D_GET_FIELD(job->args.cfg[2], V3D_CSD_QUEUED_CFG2_NUM_WGS_Z))
+		return NULL;
```

Using `V3D_GET_FIELD` with the proper register field macros is the right approach — it extracts bits [31:16] from each CFG register, which is exactly where the workgroup counts live (confirmed: `V3D_CSD_QUEUED_CFG0_NUM_WGS_X_SHIFT = 16` in `v3d_regs.h` matches `V3D_CSD_CFG012_WG_COUNT_SHIFT = 16`). This works for both indirect and direct CSD submissions. The comment explaining the hardware's 0→65536 interpretation is useful.

**Minor observation:** The existing error path in this function does `v3d->queue[V3D_CSD].active_job = NULL` before returning NULL:

```c
	if (unlikely(job->base.base.s_fence->finished.error)) {
		v3d->queue[V3D_CSD].active_job = NULL;
		return NULL;
	}
```

The new zero-workgroup path returns NULL without this. Since `active_job` is set only on the *next* line (`v3d->queue[V3D_CSD].active_job = &job->base`), the value at this point should already be NULL from the previous job's IRQ handler, so this is not a correctness issue. However, for consistency with the existing error path and defensive coding, adding `v3d->queue[V3D_CSD].active_job = NULL;` before the return would be slightly cleaner. Not a blocker.

No other issues. The Fixes tag (`d223f98f0209`) correctly references the original CSD dispatch support, as the zero-workgroup check belongs there at the hardware-facing level.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Fix indirect CSD jobs with zeroed workgroups

[Claude Code Review Bot]

Overall Series Review

Subject: drm/v3d: Fix indirect CSD jobs with zeroed workgroups
Author: =?utf-8?q?Ma=C3=ADra_Canal?= <mcanal@igalia.com>
Patches: 3
Reviewed: 2026-06-04T12:49:57.798267

---

This is a clean, well-motivated two-patch bugfix series for the V3D DRM driver's indirect CSD (compute shader dispatch) path. When userspace defers workgroup counts to a GPU buffer and those counts turn out to be zero, two bugs are exposed: (1) a vaddr leak from an early return that skips cleanup, and (2) the hardware misinterpreting 0 as 65536, causing incorrect shader execution instead of a no-op.

Both patches are small, focused, correctly ordered (fix the leak first so it doesn't mask the skip), carry proper Fixes/Cc-stable tags, and have been reviewed (Iago's R-b). The series is in good shape and ready to be applied.

**Verdict: No issues found. The series looks good.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Fix vaddr leak when indirect CSD has zeroed workgroups

[Claude Code Review Bot]

Patch Review

**Status: Good**

This patch fixes a resource leak in `v3d_rewrite_csd_job_wg_counts_from_indirect()`. Looking at the existing code in the kernel tree (`v3d_sched.c:400-406`):

```c
v3d_get_bo_vaddr(bo);
v3d_get_bo_vaddr(indirect);

wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);

if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
    return;
```

The bare `return` at line 406 skips the cleanup at lines 431-432 (`v3d_put_bo_vaddr(indirect)` / `v3d_put_bo_vaddr(bo)`), leaking both mappings. The fix correctly replaces `return` with `goto unmap_bo`, which jumps to the cleanup path:

```c
if (wg_counts[0] == 0 || wg_counts[1] == 0 || wg_counts[2] == 0)
    goto unmap_bo;
```

```c
unmap_bo:
	v3d_put_bo_vaddr(indirect);
	v3d_put_bo_vaddr(bo);
```

The label placement is correct — it sits right before both `v3d_put_bo_vaddr` calls, matching the acquisition order (bo, indirect acquired; indirect, bo released). The label name `unmap_bo` is slightly ambiguous since it unmaps both BOs, but this is a minor style nit that doesn't affect correctness.

One observation: when the zero-workgroup path is taken, `args->cfg[0..4]` are left untouched — they retain whatever userspace submitted. This is fine because patch 2 ensures the CSD job is never submitted to hardware when counts are zero.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Skip CSD when it has zeroed workgroups

[Claude Code Review Bot]

Patch Review

**Status: Good**

This patch adds a check in `v3d_csd_job_run()` to skip hardware submission when any workgroup dimension is zero. The check is placed after the existing error check and before setting `active_job`:

```c
if (!V3D_GET_FIELD(job->args.cfg[0], V3D_CSD_QUEUED_CFG0_NUM_WGS_X) ||
    !V3D_GET_FIELD(job->args.cfg[1], V3D_CSD_QUEUED_CFG1_NUM_WGS_Y) ||
    !V3D_GET_FIELD(job->args.cfg[2], V3D_CSD_QUEUED_CFG2_NUM_WGS_Z))
	return NULL;
```

The use of `V3D_GET_FIELD` with the proper field macros (bits [31:16] per `v3d_regs.h:416-429`) is the right approach, extracting only the workgroup count bits rather than checking the whole register value. This correctly handles `cfg[]` values that might have non-zero bits in the lower half but a zero workgroup count in the upper half.

The comment explaining the hardware behavior (0 is interpreted as 65536) is accurate and provides good justification for why this check is needed.

The placement before `v3d->queue[V3D_CSD].active_job = &job->base` is correct — by returning `NULL` before setting `active_job`, the function avoids leaving the queue in a state where it thinks a job is active but no fence was created and no hardware submission occurred. This matches the existing pattern used by the error-path check above it (lines 350-353 in the current tree).

The `Fixes` tag references the original CSD support commit, which is appropriate since the zero-workgroup case was always a latent bug — it just wasn't reachable until indirect CSD was added.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Skip CSD when it has zeroed workgroups

[Claude Code Review Bot]

Patch Review

**Status: Good, one minor observation**

This patch prevents the hardware from seeing a dispatch with zero workgroup counts. As the comment explains, the hardware interprets 0 in the NUM_WGS fields as 65536 (since the field is 16-bit and 0 wraps), which would cause an illegitimate submission.

The check uses `V3D_GET_FIELD` to extract only the workgroup count fields from `cfg[0..2]`:

```c
if (!V3D_GET_FIELD(job->args.cfg[0], V3D_CSD_QUEUED_CFG0_NUM_WGS_X) ||
    !V3D_GET_FIELD(job->args.cfg[1], V3D_CSD_QUEUED_CFG1_NUM_WGS_Y) ||
    !V3D_GET_FIELD(job->args.cfg[2], V3D_CSD_QUEUED_CFG2_NUM_WGS_Z))
    return NULL;
```

This is correct. The `V3D_GET_FIELD` macro extracts bits [31:16] from each register (the workgroup count), ignoring the offset in bits [15:0]. This was improved in v2 per reviewer feedback — checking the full register word would have falsely triggered if an offset field happened to be non-zero while the count was zero, or failed to trigger if the whole word was non-zero but the count field was zero.

The placement of the check is correct — it's after the `s_fence->finished.error` check but before `active_job` assignment, `v3d_job_start_stats`, `v3d_switch_perfmon`, and register writes. This means none of those side effects occur when the job is skipped, so there's no resource or state imbalance.

Returning `NULL` from `v3d_csd_job_run` follows the existing pattern (the error path just above does the same) and tells the scheduler the job completed immediately with no hardware fence to wait on.

**Minor observation**: The existing error path defensively clears `v3d->queue[V3D_CSD].active_job = NULL` before returning NULL. The new zero-count path does not. This is technically fine because `active_job` hasn't been assigned to this job yet at this point, but for consistency with the existing error path's defensive style, adding the same `active_job = NULL` assignment could be considered. This is not a bug — just a minor consistency point.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Skip CSD when it has zeroed workgroups

[Claude Code Review Bot]

Patch Review

**Assessment: Correct in principle, one minor concern.**

The core idea is sound. The V3D CSD hardware treats a zero workgroup count as 65536, so submitting a zero-count dispatch causes an illegitimate execution. Returning `NULL` from `run_job` is the correct mechanism — the DRM scheduler treats a `NULL` return as "job completed synchronously with success" (calls `drm_sched_job_done(sched_job, 0)`), which is the right semantic for a no-op dispatch. No fences, perf counters, or IRQ infrastructure are set up, which is all appropriate since no hardware work occurs.

The placement of the check is correct — it's after the error-fence check but before `active_job` is set and before any hardware interaction:

```c
if (unlikely(job->base.base.s_fence->finished.error)) {
    v3d->queue[V3D_CSD].active_job = NULL;
    return NULL;
}

if (!job->args.cfg[0] || !job->args.cfg[1] || !job->args.cfg[2])
    return NULL;

v3d->queue[V3D_CSD].active_job = &job->base;
```

**Minor concern — cfg[0..2] may contain more than the workgroup count.** Since `V3D_CSD_CFG012_WG_COUNT_SHIFT` is 16, the workgroup count occupies bits [31:16] of each cfg register. The check `!job->args.cfg[0]` tests the entire 32-bit value, not just the count field. This works for the indirect CSD path because `v3d_rewrite_csd_job_wg_counts_from_indirect()` does a full assignment (`args->cfg[0] = wg_counts[0] << 16`), so the lower 16 bits are always zero after rewrite. And in the zero-count case (with patch 1), the rewrite is skipped entirely, so cfg[0..2] retain their userspace-provided values — which must be zero-initialized for this check to trigger.

This is presumably fine because Mesa would initialize cfg[0..2] to zero for indirect CSD (the real values come from the indirect buffer), but it might be worth a brief comment or, alternatively, masking to check only the count field explicitly:

```c
#define V3D_CSD_CFG012_WG_COUNT_MASK  GENMASK(31, V3D_CSD_CFG012_WG_COUNT_SHIFT)

if (!(job->args.cfg[0] & V3D_CSD_CFG012_WG_COUNT_MASK) || ...)
```

This would make the check robust regardless of what userspace puts in the lower bits. That said, given the rewrite function's full assignment behavior, the current check works correctly in practice.

**Nit — `Fixes:` tag.** The tag references `d223f98f0209 ("drm/v3d: Add support for compute shader dispatch.")` (the original CSD commit), while the actual trigger scenario (zero counts from an indirect buffer) was only introduced later with indirect CSD support (`18b8413b25b7`). The Fixes tag is defensible since the hardware's 0-as-65536 behavior was always a latent issue, but it does stretch the stable backport scope. Not wrong, just worth noting.

---
Generated by Claude Code Patch Reviewer