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: Serialize jobs across queues when a perfmon is attached
Date: Thu, 04 Jun 2026 14:44:43 +1000
Message-ID: <review-patch3-20260531-v3d-perfmon-lifetime-v2-3-60ed4485a203@igalia.com>
In-Reply-To: <20260531-v3d-perfmon-lifetime-v2-3-60ed4485a203@igalia.com>
References: <20260531-v3d-perfmon-lifetime-v2-0-60ed4485a203@igalia.com> <20260531-v3d-perfmon-lifetime-v2-3-60ed4485a203@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**: The design is sound but there's a subtle concern worth discussi=
ng.

The approach is straightforward: when a non-global perfmon-carrying job is =
submitted, it depends on all in-flight jobs across all queues (via `last_hw=
_fence[]`). Conversely, when a non-perfmon job is submitted while a perfmon=
 job is in-flight, it depends on `perfmon_state.fence`.

**The `v3d_serialize_for_perfmon()` function:**

```c
static int
v3d_serialize_for_perfmon(struct v3d_job *job)
{
    ...
    lockdep_assert_held(&v3d->sched_lock);

    scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock)
        is_global_perfmon =3D !!v3d->global_perfmon;

    if (is_global_perfmon)
        goto publish;
    ...
```

**Observation 1 =E2=80=94 TOCTOU on `global_perfmon` check**: The `is_globa=
l_perfmon` flag is read under the perfmon spinlock, but after dropping it, =
the global perfmon state could change before we act on it. However, `sched_=
lock` is held, which serializes all submissions. The global perfmon can onl=
y be set/cleared by the SET_GLOBAL ioctl, which doesn't hold `sched_lock`. =
So there's a theoretical window where:

- Thread A: holds `sched_lock`, reads `is_global_perfmon =3D false`, procee=
ds to add fence dependencies
- Thread B: concurrently calls SET_GLOBAL, installs a global perfmon

This seems low-risk in practice (the global perfmon is a privileged operati=
on and the window is tiny), and the `sched_lock` assertion confirms submiss=
ion serialization. But it's worth considering whether the SET_GLOBAL ioctl =
should also acquire `sched_lock` to close this gap. Given that the global p=
erfmon case doesn't need serialization (concurrency is desired), the worst =
case is unnecessary serialization of one submission, not a correctness bug.

**Observation 2 =E2=80=94 `last_hw_fence` update always happens (the `publi=
sh` label)**:

```c
publish:
    dma_fence_put(v3d->perfmon_state.last_hw_fence[job->queue]);
    v3d->perfmon_state.last_hw_fence[job->queue] =3D dma_fence_get(job->don=
e_fence);

    if (job->perfmon && !is_global_perfmon) {
        dma_fence_put(v3d->perfmon_state.fence);
        v3d->perfmon_state.fence =3D dma_fence_get(job->done_fence);
    }
```

This correctly updates `last_hw_fence` for *every* job (needed so future pe=
rfmon jobs can wait on all in-flight work). The `perfmon_state.fence` is on=
ly set for perfmon-carrying jobs. This is sound.

**Observation 3 =E2=80=94 `done_fence` vs `irq_fence`**: The code uses `job=
->done_fence` for the serialization fences. This is the scheduler's finishe=
d fence, which signals when the job completes. This is the correct fence to=
 use (rather than `irq_fence` which is the HW interrupt fence).

**Observation 4 =E2=80=94 WARN_ON_ONCE in `v3d_perfmon_start()`**:

```c
if (WARN_ON_ONCE(v3d->perfmon_state.active))
    return;
```

Good defensive check. With cross-queue serialization working correctly, the=
re should never be an active perfmon when a new perfmon-carrying job starts=
. If this fires, it means serialization failed.

**Observation 5 =E2=80=94 Cleanup in `v3d_gem_destroy()`**:

```c
for (q =3D 0; q < V3D_MAX_QUEUES; q++) {
    WARN_ON(v3d->queue[q].active_job);
    v3d_stats_put(v3d->queue[q].stats);
    dma_fence_put(v3d->perfmon_state.last_hw_fence[q]);
}

dma_fence_put(v3d->perfmon_state.fence);
```

Correct cleanup of the new fence references.

**No blocking issues.**

---

---
Generated by Claude Code Patch Reviewer