Claude review: drm/v3d: Introduce Runtime Power Management

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

Patch Review

This is the main patch. Several observations:

1. **`v3d_power_suspend` error handling**: If `reset_control_assert()` fails, the clock is still disabled:
```c
if (v3d->reset)
    ret = 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 left in an inconsistent state (reset not asserted but clock off). Consider whether the clock should remain enabled if the reset assert fails, or whether this is actually fine in practice.

2. **`v3d_perfmon_start` PM get/put asymmetry**: The function does `pm_runtime_get_if_active()` at the top and `v3d_pm_runtime_put()` at the bottom, but in between it sets `v3d->active_perfmon = perfmon`. The perfmon gets a PM reference and immediately drops it. If the autosuspend timer fires between start and the next GPU job, the device could suspend while the perfmon is considered "active". This seems intentional (the device will be re-resumed when the next job arrives), but it means perfmon counter values could be lost during an intermediate suspend. The `v3d_power_suspend` does call `v3d_perfmon_stop(v3d, v3d->active_perfmon, false)` with `capture=false`, 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)`**: 
```c
ret = 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**: 
```c
if (!IS_ENABLED(CONFIG_PM)) {
    ret = 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 doesn'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 trigger the callback if PM is enabled, or be a no-op if disabled. The `!IS_ENABLED(CONFIG_PM)` check then handles the disabled case. This is correct.

6. **50ms autosuspend delay**: Reasonable default. Short enough to save power 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 skip the flush when the device is suspended is correct — 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 `v3d_job` is clean — PM ref is acquired during `v3d_job_init` and released in `v3d_job_free`. CPU jobs correctly skip PM as they don't access hardware.

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 (perfmon values being lost on intermediate suspends). Both may be acceptable tradeoffs but deserve explicit acknowledgment.

---
Generated by Claude Code Patch Reviewer