Claude review: drm/v3d: Refactor perfmon locking

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

Patch Review

**Status**: Generally good, a couple of observations.

This is the core patch. It replaces the per-perfmon mutex with a device-wide `perfmon_state.lock` spinlock, which is the correct granularity for protecting a device-wide singleton (`active_perfmon`). The spinlock also enables stopping the perfmon from IRQ context (the job-completion IRQ handler), which is the natural place to do it.

**Positive design decisions:**

1. Moving perfmon stop into `v3d_irq_signal_fence()` is correct — this is the natural job-completion boundary and solves the "perfmon never stopped if nothing queued behind" problem.

2. The suspend/resume split (`v3d_perfmon_suspend` / `v3d_perfmon_resume`) that captures HW counters without clearing `perfmon_state.active` is clean. This means the global perfmon survives power transitions correctly.

3. The `v3d_perfmon_get_values_ioctl()` change to snapshot values under the spinlock into a stack-local `values[]` array before `copy_to_user()` is correct — you can't hold a spinlock across `copy_to_user()`.

**Observations:**

1. **`v3d_perfmon_start()` calls `pm_runtime_get_if_active()` under spinlock_irqsave**:

```c
void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon)
{
    guard(spinlock_irqsave)(&v3d->perfmon_state.lock);
    ...
    if (!pm_runtime_get_if_active(v3d->drm.dev))
        return;

    v3d_perfmon_hw_start(v3d, perfmon);
    v3d->perfmon_state.active = perfmon;

    v3d_pm_runtime_put(v3d);
}
```

`pm_runtime_get_if_active()` is documented as safe to call from atomic context (it uses `spin_lock_irqsave` internally on the power domain lock), so this should be fine. `v3d_pm_runtime_put()` likely wraps `pm_runtime_put_autosuspend()` which is also atomic-safe. Just worth confirming the `v3d_pm_runtime_put` wrapper doesn't do anything sleepable.

2. **`v3d_perfmon_start()` silently returns when `v3d->global_perfmon` is set**:

```c
if (!perfmon || v3d->global_perfmon)
    return;
```

This is correct for per-job perfmons (they shouldn't override the global one), but it means the `v3d_perfmon_start()` call from `run_job()` in the scheduler becomes a no-op when a global perfmon is active. The global perfmon is started in `v3d_perfmon_set_global_ioctl()` instead. This is a cleaner separation of concerns.

3. **`v3d_reset()` now calls `v3d_perfmon_resume()` instead of `v3d_perfmon_stop()`**:

```c
/* Re-arm the global perfmon HW counters that the reset zeroed. */
v3d_perfmon_resume(v3d);
```

This makes sense for the global perfmon case — a reset zeros the HW, so you need to reprogram the counters. For a per-job perfmon, the stop in `v3d_gpu_reset_for_timeout()` (added just above `v3d_reset()`) already cleared `perfmon_state.active`, so `v3d_perfmon_resume()` will find `active == NULL` and be a no-op. The ordering is correct: stop perfmon -> reset HW -> resume (reprograms if global active).

4. **Return from inside `scoped_guard` in `v3d_perfmon_set_global_ioctl()`**:

```c
scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock) {
    old = v3d->global_perfmon;
    if (!old)
        return -EINVAL;
    ...
}
```

The `scoped_guard` macro correctly handles early returns (the lock is released via the cleanup attribute), so this is fine. Just noting it since it's a pattern that can confuse readers.

5. **`v3d_perfmon_start()` sets `active` and calls `v3d_pm_runtime_put()` in the same spinlock section**. After `v3d_pm_runtime_put()`, a suspend could race in and call `v3d_perfmon_suspend()`, which also takes the spinlock. Since we're still holding the lock, this is safe — the suspend will spin until we release. But it means `v3d_pm_runtime_put()` must not trigger a synchronous suspend while holding the spinlock. `pm_runtime_put_autosuspend()` is deferred, so this should be fine.

**No blocking issues.**

---

---
Generated by Claude Code Patch Reviewer