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: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls
Date: Thu, 04 Jun 2026 11:24:41 +1000
Message-ID: <review-patch8-20260603-v3d-sched-misc-fixes-v3-8-d7114bba55a0@igalia.com>
In-Reply-To: <20260603-v3d-sched-misc-fixes-v3-8-d7114bba55a0@igalia.com>
References: <20260603-v3d-sched-misc-fixes-v3-0-d7114bba55a0@igalia.com> <20260603-v3d-sched-misc-fixes-v3-8-d7114bba55a0@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the largest patch in the series and introduces the core `struct v3d_submit` abstraction. Key observations:

**Missing bounds check in `v3d_submit_add_job()`**: The function increments `submit->job_count` and writes to `submit->jobs[]` without checking against `V3D_MAX_JOBS_PER_SUBMISSION` (which is 3):

```c
submit->jobs[submit->job_count++] = job;
```

While the current callers won't exceed 3 jobs, this is a latent out-of-bounds write waiting to happen if a future caller adds a 4th job. An `if (submit->job_count >= V3D_MAX_JOBS_PER_SUBMISSION) return ERR_PTR(-EINVAL);` check should be added before the array write.

**CSD ioctl path**: After `v3d_setup_csd_jobs_and_bos()` creates the jobs via the old `v3d_job_init()` path, they're manually added to the submit array:
```c
submit.jobs[submit.job_count++] = &job->base;
submit.jobs[submit.job_count++] = clean_job;
```
This bypasses `v3d_submit_add_job()` and its initialization, which is fine since these jobs were already initialized via the old path. But it means the CSD ioctl temporarily uses two patterns. This is cleaned up in patch 10 where `v3d_setup_csd_jobs_and_bos()` is converted to use `v3d_submit_add_job()` internally.

**`v3d_job_types[]` table**: Nice approach to centralizing job size and free callback. The array is indexed by `enum v3d_queue`, and the bounds check at the top (`queue >= V3D_MAX_QUEUES`) protects against out-of-range access.

**v3d_submit_jobs() error handling**: If `drm_sched_job_add_dependency()` fails midway through the loop, already-pushed jobs are scheduler-owned. The current code just unlocks the mutex and returns the error. This is addressed in patch 14, but at this point in the series there's a window where partial submissions can occur. This is acceptable as an intermediate state.

Overall a well-decomposed refactoring, but the missing bounds check should be addressed.

---
Generated by Claude Code Patch Reviewer