From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/v3d: Skip CSD when it has zeroed workgroups Date: Thu, 04 Jun 2026 15:15:37 +1000 Message-ID: In-Reply-To: <20260530-v3d-fix-indirect-csd-v1-2-15533948663f@igalia.com> References: <20260530-v3d-fix-indirect-csd-v1-0-15533948663f@igalia.com> <20260530-v3d-fix-indirect-csd-v1-2-15533948663f@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 **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 execut= ion. Returning `NULL` from `run_job` is the correct mechanism =E2=80=94 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 sem= antic 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 =E2=80=94 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 =3D NULL; return NULL; } if (!job->args.cfg[0] || !job->args.cfg[1] || !job->args.cfg[2]) return NULL; v3d->queue[V3D_CSD].active_job =3D &job->base; ``` **Minor concern =E2=80=94 cfg[0..2] may contain more than the workgroup cou= nt.** Since `V3D_CSD_CFG012_WG_COUNT_SHIFT` is 16, the workgroup count occu= pies 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 indi= rect CSD path because `v3d_rewrite_csd_job_wg_counts_from_indirect()` does = a full assignment (`args->cfg[0] =3D 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-pr= ovided values =E2=80=94 which must be zero-initialized for this check to tr= igger. 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 coun= t field explicitly: ```c #define V3D_CSD_CFG012_WG_COUNT_MASK GENMASK(31, V3D_CSD_CFG012_WG_COUNT_S= HIFT) if (!(job->args.cfg[0] & V3D_CSD_CFG012_WG_COUNT_MASK) || ...) ``` This would make the check robust regardless of what userspace puts in the l= ower bits. That said, given the rewrite function's full assignment behavior= , the current check works correctly in practice. **Nit =E2=80=94 `Fixes:` tag.** The tag references `d223f98f0209 ("drm/v3d:= Add support for compute shader dispatch.")` (the original CSD commit), whi= le the actual trigger scenario (zero counts from an indirect buffer) was on= ly introduced later with indirect CSD support (`18b8413b25b7`). The Fixes t= ag is defensible since the hardware's 0-as-65536 behavior was always a late= nt issue, but it does stretch the stable backport scope. Not wrong, just wo= rth noting. --- Generated by Claude Code Patch Reviewer