From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/v3d: Introduce Runtime Power Management Date: Wed, 01 Apr 2026 07:45:10 +1000 Message-ID: In-Reply-To: <20260331-v3d-power-management-v9-3-f52ff87bfd36@igalia.com> References: <20260331-v3d-power-management-v9-0-f52ff87bfd36@igalia.com> <20260331-v3d-power-management-v9-3-f52ff87bfd36@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: Mostly good, a few concerns.** This is the main patch. It introduces `v3d_power.c` with suspend/resume cal= lbacks, wraps HW register accesses throughout the driver with `v3d_pm_runti= me_get()`/`v3d_pm_runtime_put()`, and holds a PM reference for the duration= of GPU jobs. **Positive aspects:** - Clean separation into `v3d_power.c` - The `v3d_pm_runtime_get()`/`v3d_pm_runtime_put()` inline wrappers in the = header are a good pattern - Using `pm_runtime_get_if_active()` for `v3d_mmu_flush_all()` (skip flush = if device is suspended =E2=80=94 PTs will be reloaded on resume) is correct - Using `pm_runtime_get_if_active()` for `v3d_perfmon_start()`/`v3d_perfmon= _stop()` is reasonable since these are called from scheduler context where = the device should already be active - `has_pm_ref` flag on jobs to track whether a PM reference was taken, with= automatic put in `v3d_job_free()`, is a clean design - CPU jobs correctly skip taking a PM reference (`queue !=3D V3D_CPU`) - The `!IS_ENABLED(CONFIG_PM)` fallbacks are proper **Concerns:** 1. **`v3d_perfmon_start()` silently returns on PM failure**:=20 ```c + if (!pm_runtime_get_if_active(v3d->drm.dev)) + return; + ncounters =3D perfmon->ncounters; ``` If `pm_runtime_get_if_active()` returns 0 (device suspended), `v3d_perfmon_= start()` returns without setting `v3d->active_perfmon`. The scheduler calls= `v3d_perfmon_start()` right before running a job, but by that point the jo= b should already hold a PM ref (via `has_pm_ref`). So the device should alw= ays be active here. The `pm_runtime_get_if_active()` acts as a safety net. = However, if it *did* fire, the perfmon would silently not start, which coul= d be confusing. A `WARN_ON` or `drm_warn` might be more appropriate than a = silent return, since this would indicate a logic error. 2. **`v3d_perfmon_stop()` =E2=80=94 asymmetric get/put with `out_clear` lab= el**: ```c + if (!pm_runtime_get_if_active(v3d->drm.dev)) + goto out_clear; ... + v3d_pm_runtime_put(v3d); + +out_clear: v3d->active_perfmon =3D NULL; +out: mutex_unlock(&perfmon->lock); ``` When `pm_runtime_get_if_active()` returns 0, it jumps to `out_clear` which = sets `active_perfmon =3D NULL` without capturing counter values or disablin= g the counters. This is correct for the suspend case (counters are already = dead), but it means `active_perfmon` tracking stays consistent. This looks = fine. 3. **`v3d_power_suspend()` error recovery**: If `v3d_suspend_sms()` fails, = it re-enables IRQs: ```c + v3d_irq_disable(v3d); + + ret =3D v3d_suspend_sms(v3d); + if (ret) { + v3d_irq_enable(v3d); + return ret; + } ``` This is good error recovery. However, `v3d_suspend_sms()` already wrote `V3= D_SMS_POWER_OFF` to the SMS register before the timeout. On failure, the SM= S may be in an inconsistent state. Re-enabling IRQs without attempting to r= ecover SMS state could be problematic. That said, if the SMS is failing to = power off, the HW is likely in a bad state already, so there may not be muc= h that can be done. A comment explaining this would be helpful. 4. **Probe path =E2=80=94 `pm_runtime_put_sync_suspend()` in error cleanup*= *: ```c +runtime_pm_put: + pm_runtime_put_sync_suspend(dev); ``` After `pm_runtime_resume_and_get()` succeeds in probe, if a later step fail= s, this drops the reference and synchronously suspends. Since `devm_pm_runt= ime_enable()` was already called, the runtime PM core will handle disabling= on driver detach. This is correct. 5. **`v3d_measure_clock()` holds PM ref across `msleep(1000)`**:=20 ```c + ret =3D v3d_pm_runtime_get(v3d); + if (ret) + return ret; ... msleep(measure_ms); ... + v3d_pm_runtime_put(v3d); ``` This keeps the GPU powered for a full second during the debugfs clock measu= rement. This is by design (measuring the clock requires it), but it's worth= noting that reading this debugfs file will prevent autosuspend for 1 secon= d. Acceptable for a debug interface. 6. **`v3d_get_param_ioctl()` takes PM ref for register reads**: This is cor= rect =E2=80=94 the device needs to be powered to read HW registers. 7. **Copyright line**: `Copyright (C) 2026 Raspberry Pi` =E2=80=94 the auth= or is from Igalia, not Raspberry Pi. Might want `Copyright (C) 2026 Igalia,= S.L.` instead, or whatever is appropriate. Minor. Overall, this is a solid series at v9 and ready to merge with the minor poi= nts noted above. --- Generated by Claude Code Patch Reviewer