From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/v3d: Skip CSD when it has zeroed workgroups
Date: Thu, 04 Jun 2026 12:23:40 +1000
Message-ID: <review-patch2-20260602-v3d-fix-indirect-csd-v4-2-654309e32bc0@igalia.com>
In-Reply-To: <20260602-v3d-fix-indirect-csd-v4-2-654309e32bc0@igalia.com>
References: <20260602-v3d-fix-indirect-csd-v4-0-654309e32bc0@igalia.com> <20260602-v3d-fix-indirect-csd-v4-2-654309e32bc0@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

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

This patch addresses two things:

**Part A =E2=80=94 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] =3D wg_counts[0] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
 	args->cfg[1] =3D wg_counts[1] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
 	args->cfg[2] =3D wg_counts[2] << V3D_CSD_CFG012_WG_COUNT_SHIFT;
=20
+	if (wg_counts[0] =3D=3D 0 || wg_counts[1] =3D=3D 0 || wg_counts[2] =3D=3D=
 0)
+		goto unmap_bo;
```

This is the v3=E2=86=92v4 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` calcula=
tion, which would produce `0 - 1 =3D 0xFFFFFFFF` (triggering the existing `=
WARN_ON(args->cfg[4] =3D=3D ~0)`) if any count is zero.

**Part B =E2=80=94 Skip hardware dispatch for zero workgroups.** In `v3d_cs=
d_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 ap=
proach =E2=80=94 it extracts bits [31:16] from each CFG register, which is =
exactly where the workgroup counts live (confirmed: `V3D_CSD_QUEUED_CFG0_NU=
M_WGS_X_SHIFT =3D 16` in `v3d_regs.h` matches `V3D_CSD_CFG012_WG_COUNT_SHIF=
T =3D 16`). This works for both indirect and direct CSD submissions. The co=
mment explaining the hardware's 0=E2=86=9265536 interpretation is useful.

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

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

The new zero-workgroup path returns NULL without this. Since `active_job` i=
s set only on the *next* line (`v3d->queue[V3D_CSD].active_job =3D &job->ba=
se`), 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 =3D NULL;` before the return would be slightly cleaner. Not=
 a blocker.

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

---
Generated by Claude Code Patch Reviewer