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: Fri, 13 Mar 2026 13:47:08 +1000 Message-ID: In-Reply-To: <20260312-v3d-power-management-v7-5-9f006a1d4c55@igalia.com> References: <20260312-v3d-power-management-v7-0-9f006a1d4c55@igalia.com> <20260312-v3d-power-management-v7-5-9f006a1d4c55@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 This is the main patch. Several observations: 1. **`v3d_power_suspend` error handling**: If `reset_control_assert()` fail= s, the clock is still disabled: ```c if (v3d->reset) ret =3D reset_control_assert(v3d->reset); clk_disable_unprepare(v3d->clk); return ret; ``` If the assert fails but we still disable the clock, the hardware could be l= eft in an inconsistent state (reset not asserted but clock off). Consider w= hether the clock should remain enabled if the reset assert fails, or whethe= r this is actually fine in practice. 2. **`v3d_perfmon_start` PM get/put asymmetry**: The function does `pm_runt= ime_get_if_active()` at the top and `v3d_pm_runtime_put()` at the bottom, b= ut in between it sets `v3d->active_perfmon =3D perfmon`. The perfmon gets a= PM reference and immediately drops it. If the autosuspend timer fires betw= een start and the next GPU job, the device could suspend while the perfmon = is considered "active". This seems intentional (the device will be re-resum= ed when the next job arrives), but it means perfmon counter values could be= lost during an intermediate suspend. The `v3d_power_suspend` does call `v3= d_perfmon_stop(v3d, v3d->active_perfmon, false)` with `capture=3Dfalse`, so= values accumulated since the last stop will indeed be lost. This might be = acceptable for runtime PM but is worth noting. 3. **`v3d_measure_clock` holds PM ref across `msleep(1000)`**:=20 ```c ret =3D v3d_pm_runtime_get(v3d); ... msleep(measure_ms); /* 1000ms! */ ... v3d_pm_runtime_put(v3d); ``` This keeps the GPU powered for a full second during the debugfs measurement= . This is correct behavior for a clock measurement, but could be surprising= . Not a bug, just a consequence of the measurement approach. 4. **Probe `!CONFIG_PM` handling**:=20 ```c if (!IS_ENABLED(CONFIG_PM)) { ret =3D v3d_power_resume(dev); if (ret) goto gem_destroy; } ``` When PM is disabled, `pm_runtime_resume_and_get()` is a no-op that returns = 0 without actually calling the resume callback, so the manual call to `v3d_= power_resume()` is correct. However, the `goto gem_destroy` on failure does= n't undo `pm_runtime_resume_and_get` (which was a no-op anyway, so this is = fine). 5. **Remove path**: `pm_runtime_suspend(dev)` is called explicitly before `= v3d_power_suspend()` for !CONFIG_PM. The `pm_runtime_suspend()` will trigge= r the callback if PM is enabled, or be a no-op if disabled. The `!IS_ENABLE= D(CONFIG_PM)` check then handles the disabled case. This is correct. 6. **50ms autosuspend delay**: Reasonable default. Short enough to save pow= er during idle, long enough to avoid excessive power cycling during bursty = workloads. 7. **`v3d_mmu_flush_all` pattern**: Using `pm_runtime_get_if_active()` to s= kip the flush when the device is suspended is correct =E2=80=94 page table = changes will be picked up on the next resume via `v3d_mmu_set_page_table()`= . This is a good optimization. 8. **Job submission PM ref lifecycle**: The `has_pm_ref` flag pattern in `v= 3d_job` is clean =E2=80=94 PM ref is acquired during `v3d_job_init` and rel= eased in `v3d_job_free`. CPU jobs correctly skip PM as they don't access ha= rdware. Overall this is a solid series at v7. The main concern is point 1 (suspend = continuing to disable the clock after a failed reset assert) and point 2 (p= erfmon values being lost on intermediate suspends). Both may be acceptable = tradeoffs but deserve explicit acknowledgment. --- Generated by Claude Code Patch Reviewer