Claude review: drm/v3d: Serialize jobs across queues when a perfmon is attached

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Status**: The design is sound but there's a subtle concern worth discussing.

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 = !!v3d->global_perfmon;

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

**Observation 1 — TOCTOU on `global_perfmon` check**: The `is_global_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 only 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 = false`, proceeds 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 operation and the window is tiny), and the `sched_lock` assertion confirms submission serialization. But it's worth considering whether the SET_GLOBAL ioctl should also acquire `sched_lock` to close this gap. Given that the global perfmon case doesn't need serialization (concurrency is desired), the worst case is unnecessary serialization of one submission, not a correctness bug.

**Observation 2 — `last_hw_fence` update always happens (the `publish` label)**:

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

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

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

**Observation 3 — `done_fence` vs `irq_fence`**: The code uses `job->done_fence` for the serialization fences. This is the scheduler's finished 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 — 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, there should never be an active perfmon when a new perfmon-carrying job starts. If this fires, it means serialization failed.

**Observation 5 — Cleanup in `v3d_gem_destroy()`**:

```c
for (q = 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