Claude review: drm/v3d: Introduce Runtime Power Management

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

Patch Review

**Status: Mostly good, a few concerns.**

This is the main patch. It introduces `v3d_power.c` with suspend/resume callbacks, wraps HW register accesses throughout the driver with `v3d_pm_runtime_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 — 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 != V3D_CPU`)
- The `!IS_ENABLED(CONFIG_PM)` fallbacks are proper

**Concerns:**

1. **`v3d_perfmon_start()` silently returns on PM failure**: 

```c
+	if (!pm_runtime_get_if_active(v3d->drm.dev))
+		return;
+
 	ncounters = 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 job should already hold a PM ref (via `has_pm_ref`). So the device should always 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 could 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()` — asymmetric get/put with `out_clear` label**:

```c
+	if (!pm_runtime_get_if_active(v3d->drm.dev))
+		goto out_clear;
...
+	v3d_pm_runtime_put(v3d);
+
+out_clear:
 	v3d->active_perfmon = NULL;
+out:
 	mutex_unlock(&perfmon->lock);
```

When `pm_runtime_get_if_active()` returns 0, it jumps to `out_clear` which sets `active_perfmon = NULL` without capturing counter values or disabling 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 = v3d_suspend_sms(v3d);
+	if (ret) {
+		v3d_irq_enable(v3d);
+		return ret;
+	}
```

This is good error recovery. However, `v3d_suspend_sms()` already wrote `V3D_SMS_POWER_OFF` to the SMS register before the timeout. On failure, the SMS may be in an inconsistent state. Re-enabling IRQs without attempting to recover 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 much that can be done. A comment explaining this would be helpful.

4. **Probe path — `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 fails, this drops the reference and synchronously suspends. Since `devm_pm_runtime_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)`**: 

```c
+	ret = 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 measurement. This is by design (measuring the clock requires it), but it's worth noting that reading this debugfs file will prevent autosuspend for 1 second. Acceptable for a debug interface.

6. **`v3d_get_param_ioctl()` takes PM ref for register reads**: This is correct — the device needs to be powered to read HW registers.

7. **Copyright line**: `Copyright (C) 2026 Raspberry Pi` — the author 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 points noted above.

---
Generated by Claude Code Patch Reviewer