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 13:44:21 +1000 Message-ID: In-Reply-To: <20260601-v3d-fix-indirect-csd-v2-2-aaebf035b936@igalia.com> References: <20260601-v3d-fix-indirect-csd-v2-0-aaebf035b936@igalia.com> <20260601-v3d-fix-indirect-csd-v2-2-aaebf035b936@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, 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 f= rom `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 wa= s improved in v2 per reviewer feedback =E2=80=94 checking the full register= word would have falsely triggered if an offset field happened to be non-ze= ro 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 =E2=80=94 it's after the `s_fence->fi= nished.error` check but before `active_job` assignment, `v3d_job_start_stat= s`, `v3d_switch_perfmon`, and register writes. This means none of those sid= e effects occur when the job is skipped, so there's no resource or state im= balance. Returning `NULL` from `v3d_csd_job_run` follows the existing pattern (the e= rror path just above does the same) and tells the scheduler the job complet= ed immediately with no hardware fence to wait on. **Minor observation**: The existing error path defensively clears `v3d->que= ue[V3D_CSD].active_job =3D NULL` before returning NULL. The new zero-count = path does not. This is technically fine because `active_job` hasn't been as= signed to this job yet at this point, but for consistency with the existing= error path's defensive style, adding the same `active_job =3D NULL` assign= ment could be considered. This is not a bug =E2=80=94 just a minor consiste= ncy point. --- Generated by Claude Code Patch Reviewer