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: Sat, 14 Feb 2026 07:21:53 +1000 Message-ID: In-Reply-To: <20260213-v3d-power-management-v5-7-7a8b381eb379@igalia.com> References: <20260213-v3d-power-management-v5-0-7a8b381eb379@igalia.com> <20260213-v3d-power-management-v5-7-7a8b381eb379@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 largest and most complex patch in the series. Let me go through t= he 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 !=3D V3D_CPU) { > + ret =3D v3d_pm_runtime_get(v3d); > + if (ret) > + goto fail_job_init; > + job->has_pm_ref =3D true; > + } This takes a PM reference at job init time and releases it when the job's ref= count 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 af= ter it finishes execution but before its refcount drops to zero. That's a rea= sonable approach -- it ensures the autosuspend timer starts ticking after the= last job reference is released. **v3d_reset PM reference:** > + ret =3D v3d_pm_runtime_get(v3d); > + if (ret) > + return; `v3d_reset()` is called from the scheduler timeout handler (`v3d_gpu_reset_fo= r_timeout`), which is triggered when a submitted job times out. Since submitt= ed jobs hold a PM reference via `has_pm_ref`, the GPU should already be power= ed when the timeout handler runs. The extra `pm_runtime_get` here is still co= rrect for safety, and the early return on failure is reasonable since a reset= can't proceed without hardware access. However, the scheduler timeout handle= rs (e.g., `v3d_cl_job_timedout`) also read hardware registers *before* callin= g `v3d_gpu_reset_for_timeout`: ```c u32 ctca =3D V3D_CORE_READ(0, V3D_CLE_CTNCA(q)); u32 ctra =3D 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 `ha= s_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 sin= ce state will be fully restored on resume. `pm_runtime_get_if_active()` retur= ns 0 when the device is not active, and the `!0` causes the early return. Whe= n active (return 1), the reference is incremented and the flush proceeds, fol= lowed by `v3d_pm_runtime_put()`. The `-EINVAL` case (runtime PM disabled) wou= ld 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 w= orks 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_perfm= on` means the perfmon won't be started if the GPU is suspended. This is calle= d 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 defen= sive 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 i= s 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 =3D devm_pm_runtime_enable(dev); > + if (ret) > + goto gem_destroy; > + > + ret =3D 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 =3D v3d_power_resume(dev); > + if (ret) > + goto gem_destroy; > + } When `CONFIG_PM` is enabled, `pm_runtime_resume_and_get()` calls `v3d_power_r= esume()` via the runtime PM callback. When `CONFIG_PM` is disabled, `pm_runti= me_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_de= stroy` skips the runtime PM put, which is fine since PM is disabled. But it a= lso skips clock disable -- `v3d_power_resume()` would have called `clk_prepar= e_enable()`, but there's no corresponding cleanup. This seems like a minor ga= p: 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_s= uspend()` which is a no-op when PM is disabled, so the clock won't be disable= d. Looking more carefully, the only things that can fail after the `!CONFIG_P= M` 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 a= dd 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 p= ut/mark_last_busy pattern) makes sense in the remove path since we want immed= iate suspension. **v3d_power_resume callback:** > + ret =3D 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 =3D 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 p= ath (still visible after this patch), `v3d_init_hw_state()` is called explici= tly. 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 invaria= nt 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 ca= lled on every power-on, or whether the L2T configuration survives power cycli= ng? **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 ra= te 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_enab= le()` are no longer called explicitly in probe -- they're handled by `v3d_pow= er_resume()` which is called by `pm_runtime_resume_and_get()`. So the `v3d_in= it_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_p= ower_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 runti= me PM. This means during system suspend, the runtime PM framework's generic h= andling (`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