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: Refactor perfmon locking
Date: Thu, 04 Jun 2026 14:44:42 +1000
Message-ID: <review-patch2-20260531-v3d-perfmon-lifetime-v2-2-60ed4485a203@igalia.com>
In-Reply-To: <20260531-v3d-perfmon-lifetime-v2-2-60ed4485a203@igalia.com>
References: <20260531-v3d-perfmon-lifetime-v2-0-60ed4485a203@igalia.com> <20260531-v3d-perfmon-lifetime-v2-2-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**: Generally good, a couple of observations.

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

**Positive design decisions:**

1. Moving perfmon stop into `v3d_irq_signal_fence()` is correct =E2=80=94 t=
his is the natural job-completion boundary and solves the "perfmon never st=
opped 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 co=
rrect =E2=80=94 you can't hold a spinlock across `copy_to_user()`.

**Observations:**

1. **`v3d_perfmon_start()` calls `pm_runtime_get_if_active()` under spinloc=
k_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 =3D perfmon;

    v3d_pm_runtime_put(v3d);
}
```

`pm_runtime_get_if_active()` is documented as safe to call from atomic cont=
ext (it uses `spin_lock_irqsave` internally on the power domain lock), so t=
his should be fine. `v3d_pm_runtime_put()` likely wraps `pm_runtime_put_aut=
osuspend()` which is also atomic-safe. Just worth confirming the `v3d_pm_ru=
ntime_put` wrapper doesn't do anything sleepable.

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

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

This is correct for per-job perfmons (they shouldn't override the global on=
e), but it means the `v3d_perfmon_start()` call from `run_job()` in the sch=
eduler 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 s=
eparation 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 =E2=80=94 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 cle=
ared `perfmon_state.active`, so `v3d_perfmon_resume()` will find `active =
=3D=3D 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 =3D v3d->global_perfmon;
    if (!old)
        return -EINVAL;
    ...
}
```

The `scoped_guard` macro correctly handles early returns (the lock is relea=
sed 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()` i=
n the same spinlock section**. After `v3d_pm_runtime_put()`, a suspend coul=
d race in and call `v3d_perfmon_suspend()`, which also takes the spinlock. =
Since we're still holding the lock, this is safe =E2=80=94 the suspend will=
 spin until we release. But it means `v3d_pm_runtime_put()` must not trigge=
r a synchronous suspend while holding the spinlock. `pm_runtime_put_autosus=
pend()` is deferred, so this should be fine.

**No blocking issues.**

---

---
Generated by Claude Code Patch Reviewer