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/tegra: gr2d/gr3d: Contain PM in the gr*d_probe/gr*d_remove Date: Mon, 18 May 2026 16:14:38 +1000 Message-ID: In-Reply-To: <20260517091450.46728-3-clamor95@gmail.com> References: <20260517091450.46728-1-clamor95@gmail.com> <20260517091450.46728-3-clamor95@gmail.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 **Assessment: Correct fix with minor observations.** This patch makes two changes: 1. **Moves `pm_runtime_enable()`, `pm_runtime_use_autosuspend()`, and `pm_r= untime_set_autosuspend_delay()` from `gr*d_runtime_resume()` into `gr*d_pro= be()`.** Having `pm_runtime_enable()` inside the runtime_resume callback wa= s fundamentally wrong =E2=80=94 the PM core needs runtime PM to be enabled = *before* it can call the resume callback through normal runtime PM operatio= ns. Every resume would also redundantly re-configure autosuspend. Moving th= is to probe is correct. 2. **Removes `pm_runtime_dont_use_autosuspend()` and `pm_runtime_force_susp= end()` from `gr*d_exit()`.** The exit callback no longer manages PM lifecyc= le; this is handled by `pm_runtime_disable()` in `gr*d_remove()`. **Observations:** - **Typo in commit message:** `"GR2G/GR3D"` should be `"GR2D/GR3D"`. - **Ordering relative to other tegra drivers:** The patches place `pm_runti= me_enable()` *before* `host1x_client_register()`, which is the safer orderi= ng (avoids a race where userspace submits before PM is enabled). However, t= he sibling drivers `nvdec.c` and `vic.c` do the opposite =E2=80=94 they cal= l `host1x_client_register()` first, then `pm_runtime_enable()`. The new ord= ering in gr2d/gr3d is arguably more correct, but the inconsistency across t= he tegra driver family is worth being aware of. - **Missing `pm_runtime_dont_use_autosuspend()` in remove:** The patch remo= ves `pm_runtime_dont_use_autosuspend()` from `gr*d_exit()` but doesn't add = it to `gr*d_remove()` before `pm_runtime_disable()`. The common pattern for= drivers using autosuspend is to call `pm_runtime_dont_use_autosuspend()` t= hen `pm_runtime_disable()` in the remove path, to cancel any pending autosu= spend timer. In practice, `pm_runtime_disable()` handles cancellation inter= nally, so this is not a bug =E2=80=94 just a deviation from the typical pat= tern. The pre-existing `gr*d_remove()` also lacked it, so this is not a reg= ression. - **No explicit suspend on removal:** The original `gr*d_exit()` called `pm= _runtime_force_suspend()` to ensure the hardware was powered down when the = client was torn down. After this patch, if the device is runtime-active whe= n `gr*d_remove()` is called, `pm_runtime_disable()` leaves it active =E2=80= =94 the clocks stay enabled until devm cleanup. This is acceptable for driv= er removal but is a minor change in teardown behavior worth noting. - **Error path in probe:** The error path for `host1x_client_register()` fa= ilure correctly calls `pm_runtime_disable()`, which is good: ```c err =3D host1x_client_register(&gr2d->client.base); if (err < 0) { pm_runtime_disable(dev); ... } ``` Overall, this is a well-structured fix for a real resume regression. The mi= nor items above are observations, not blockers. --- Generated by Claude Code Patch Reviewer