public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
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	[thread overview]
Message-ID: <review-patch8-20260603-v3d-sched-misc-fixes-v3-8-d7114bba55a0@igalia.com> (raw)
In-Reply-To: <20260603-v3d-sched-misc-fixes-v3-8-d7114bba55a0@igalia.com>

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

  reply	other threads:[~2026-06-04  1:24 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-03 22:25 [PATCH v3 00/14] drm/v3d: Scheduler and submission fixes and refactoring Maíra Canal
2026-06-03 22:25 ` [PATCH v3 01/14] drm/v3d: Drop unused drm_encoder.h include from v3d_drv.h Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 02/14] drm/v3d: Clear queue->active_job when v3d_fence_create() fails Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 03/14] drm/v3d: Use inline lock for dma fence initialization Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 04/14] drm/v3d: Replace spin_lock_irqsave() with spin_lock() Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 05/14] drm/v3d: Extract v3d_job_add_syncobjs() helper Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 06/14] drm/v3d: Reject invalid syncobj handles in submit ioctls Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 07/14] drm/v3d: Migrate BO reservation locking to DRM exec Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls Maíra Canal
2026-06-04  1:24   ` Claude Code Review Bot [this message]
2026-06-03 22:25 ` [PATCH v3 09/14] drm/v3d: Make v3d_get_cpu_indirect_csd_params() a pure parser Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 10/14] drm/v3d: Convert submit helpers to operate on struct v3d_submit Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 11/14] drm/v3d: Refactor CPU ioctl into unified submission chain Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 12/14] drm/v3d: Split BO fence attach from syncobj output handling Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 13/14] drm/v3d: Reject invalid out_sync handles in submit ioctls Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-03 22:25 ` [PATCH v3 14/14] drm/v3d: Ensure atomic submissions in v3d_submit_jobs() Maíra Canal
2026-06-04  1:24   ` Claude review: " Claude Code Review Bot
2026-06-04  1:24 ` Claude review: drm/v3d: Scheduler and submission fixes and refactoring Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-05-10 22:11 [PATCH v2 00/14] " Maíra Canal
2026-05-10 22:12 ` [PATCH v2 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls Maíra Canal
2026-05-16  5:59   ` Claude review: " Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch8-20260603-v3d-sched-misc-fixes-v3-8-d7114bba55a0@igalia.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox