Claude review: drm/v3d: Introduce Runtime Power Management

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

Patch Review

This is the largest and most complex patch in the series. Let me go through the key aspects.

**PM runtime get/put wrappers:**

> +static __always_inline int v3d_pm_runtime_get(struct v3d_dev *v3d)
> +{
> +	return pm_runtime_resume_and_get(v3d->drm.dev);
> +}
> +
> +static __always_inline int v3d_pm_runtime_put(struct v3d_dev *v3d)
> +{
> +	return pm_runtime_put_autosuspend(v3d->drm.dev);
> +}

Clean wrappers. The 50ms autosuspend delay is set in probe.

**Job submission PM reference:**

> +	/* CPU jobs don't require hardware resources */
> +	if (queue != V3D_CPU) {
> +		ret = v3d_pm_runtime_get(v3d);
> +		if (ret)
> +			goto fail_job_init;
> +		job->has_pm_ref = true;
> +	}

This takes a PM reference at job init time and releases it when the job's refcount drops to zero:

> +	if (job->has_pm_ref)
> +		v3d_pm_runtime_put(job->v3d);

This means the GPU stays powered as long as any submitted job exists, even after it finishes execution but before its refcount drops to zero. That's a reasonable approach -- it ensures the autosuspend timer starts ticking after the last job reference is released.

**v3d_reset PM reference:**

> +	ret = v3d_pm_runtime_get(v3d);
> +	if (ret)
> +		return;

`v3d_reset()` is called from the scheduler timeout handler (`v3d_gpu_reset_for_timeout`), which is triggered when a submitted job times out. Since submitted jobs hold a PM reference via `has_pm_ref`, the GPU should already be powered when the timeout handler runs. The extra `pm_runtime_get` here is still correct for safety, and the early return on failure is reasonable since a reset can't proceed without hardware access. However, the scheduler timeout handlers (e.g., `v3d_cl_job_timedout`) also read hardware registers *before* calling `v3d_gpu_reset_for_timeout`:

```c
u32 ctca = V3D_CORE_READ(0, V3D_CLE_CTNCA(q));
u32 ctra = V3D_CORE_READ(0, V3D_CLE_CTNRA(q));
```

These register reads in `v3d_cl_job_timedout()` and `v3d_csd_job_timedout()` happen without an explicit PM reference. They rely on the timed-out job's `has_pm_ref` keeping the GPU powered. Since the job must still be alive for the timeout to fire, this is safe.

**v3d_mmu_flush_all PM guard:**

> +	/* Flush the PTs only if we're already awake */
> +	if (!pm_runtime_get_if_active(v3d->drm.dev))
> +		return 0;

This is correct -- if the GPU is suspended, the MMU doesn't need flushing since state will be fully restored on resume. `pm_runtime_get_if_active()` returns 0 when the device is not active, and the `!0` causes the early return. When active (return 1), the reference is incremented and the flush proceeds, followed by `v3d_pm_runtime_put()`. The `-EINVAL` case (runtime PM disabled) would skip the early return and proceed to flush without taking a reference, but `v3d_pm_runtime_put()` is also effectively a no-op in that scenario, so it works out.

**Perfmon PM guards:**

> +	if (!pm_runtime_get_if_active(v3d->drm.dev))
> +		return;

In `v3d_perfmon_start()`, the early return without setting `v3d->active_perfmon` means the perfmon won't be started if the GPU is suspended. This is called from `v3d_switch_perfmon()` in the scheduler's job run path, where the job already holds a PM reference, so the GPU should be active. The guard is defensive here.

In `v3d_perfmon_stop()`:

> +	if (!pm_runtime_get_if_active(v3d->drm.dev))
> +		goto out_clear;

If the GPU is not active during stop, it skips reading the counter values and writing the disable register, but still clears `v3d->active_perfmon`. This is correct -- if the GPU is powered off, the counters are lost anyway, and we need to clear the active perfmon state.

**Probe sequence with PM:**

> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		goto gem_destroy;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto gem_destroy;
> +
> +	/* If PM is disabled, we need to call v3d_power_resume() manually. */
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		ret = v3d_power_resume(dev);
> +		if (ret)
> +			goto gem_destroy;
> +	}

When `CONFIG_PM` is enabled, `pm_runtime_resume_and_get()` calls `v3d_power_resume()` via the runtime PM callback. When `CONFIG_PM` is disabled, `pm_runtime_resume_and_get()` returns 1 (from the stub), so `v3d_power_resume()` needs to be called manually. However, in the `!CONFIG_PM` error path, `goto gem_destroy` skips the runtime PM put, which is fine since PM is disabled. But it also skips clock disable -- `v3d_power_resume()` would have called `clk_prepare_enable()`, but there's no corresponding cleanup. This seems like a minor gap: if the `!CONFIG_PM` path succeeds with `v3d_power_resume()` but something later in probe fails, the `runtime_pm_put` label calls `pm_runtime_put_sync_suspend()` which is a no-op when PM is disabled, so the clock won't be disabled. Looking more carefully, the only things that can fail after the `!CONFIG_PM` block are `dma_set_mask_and_coherent`, `drm_dev_register`, and `v3d_sysfs_init`. On those failures, the error path goes to `runtime_pm_put` (no-op) -> `gem_destroy` -> `dma_free`, and the clock remains enabled. You may want to add an explicit `v3d_power_suspend()` call in the `!CONFIG_PM` error cleanup.

**Remove sequence:**

> +	pm_runtime_suspend(dev);
> +
> +	/* If PM is disabled, we need to call v3d_power_suspend() manually. */
> +	if (!IS_ENABLED(CONFIG_PM))
> +		v3d_power_suspend(dev);

`pm_runtime_suspend()` will be a no-op when `!CONFIG_PM`, so the manual call is needed. Calling `pm_runtime_suspend()` directly (rather than through the put/mark_last_busy pattern) makes sense in the remove path since we want immediate suspension.

**v3d_power_resume callback:**

> +	ret = clk_prepare_enable(v3d->clk);
> +	if (ret)
> +		return ret;
> +
> +	if (v3d->clk)
> +		clk_set_rate(v3d->clk, v3d->max_clk_rate);
> +
> +	if (v3d->reset) {
> +		ret = reset_control_deassert(v3d->reset);
> +		if (ret)
> +			goto clk_disable;
> +	}
> +
> +	v3d_resume_sms(v3d);
> +	v3d_mmu_set_page_table(v3d);
> +	v3d_irq_enable(v3d);

One question: should `v3d_init_hw_state()` be called here as well? After the GPU is power-cycled through runtime suspend/resume, the core hardware state (L2T cache configuration set by `v3d_init_core`) would be lost. In the probe path (still visible after this patch), `v3d_init_hw_state()` is called explicitly. But in the runtime resume callback, it's not called. This means after a runtime suspend/resume cycle, the L2T cache flush registers and other invariant state set by `v3d_init_core()` may not be restored. The `v3d_reset()` path does call `v3d_init_hw_state()` (via the patched version that adds a PM get), but the normal resume path does not.

Could you verify whether the hardware requires `v3d_init_hw_state()` to be called on every power-on, or whether the L2T configuration survives power cycling?

**v3d_power_suspend callback:**

> +	v3d_irq_disable(v3d);
> +	v3d_suspend_sms(v3d);
> +
> +	if (v3d->reset)
> +		reset_control_assert(v3d->reset);
> +
> +	if (v3d->clk)
> +		clk_set_rate(v3d->clk, 0);
> +
> +	clk_disable_unprepare(v3d->clk);

The ordering here is: disable IRQs, power off SMS, assert reset, set clock rate to minimum, then disable clock. This is reasonable. The `clk_set_rate(v3d->clk, 0)` before `clk_disable_unprepare()` addresses the firmware quirk where disabling the clock doesn't actually power it off.

**Probe init_hw_state placement:**

> 	v3d_init_hw_state(v3d);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 50);
> +	pm_runtime_use_autosuspend(dev);

`v3d_init_hw_state()` is called after `pm_runtime_resume_and_get()` in probe, which is correct. But note that `v3d_mmu_set_page_table()` and `v3d_irq_enable()` are no longer called explicitly in probe -- they're handled by `v3d_power_resume()` which is called by `pm_runtime_resume_and_get()`. So the `v3d_init_hw_state()` in probe is the only call that initializes core hardware state during first boot. As noted above, this same init is missing from the `v3d_power_resume()` callback for subsequent runtime resumes.

**DEFINE_RUNTIME_DEV_PM_OPS:**

> +static DEFINE_RUNTIME_DEV_PM_OPS(v3d_pm_ops, v3d_power_suspend,
> +				 v3d_power_resume, NULL);

The NULL third argument means no system suspend/resume callbacks beyond runtime PM. This means during system suspend, the runtime PM framework's generic handling (`pm_runtime_force_suspend`/`pm_runtime_force_resume`) will be used. This should work correctly with the runtime PM callbacks already defined.

---
Generated by Claude Code Patch Reviewer