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: Move pm_runtime_enable to gr*d_probe Date: Tue, 28 Apr 2026 14:52:59 +1000 Message-ID: In-Reply-To: <20260427075856.85208-2-clamor95@gmail.com> References: <20260427075856.85208-1-clamor95@gmail.com> <20260427075856.85208-2-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 **Issue: Wrong ordering of pm_runtime calls (moderate)** In both gr2d and gr3d, the patch adds: ```c pm_runtime_enable(dev); pm_runtime_use_autosuspend(dev); pm_runtime_set_autosuspend_delay(dev, 500); ``` The standard kernel pattern is the reverse =E2=80=94 configure autosuspend = *before* enabling runtime PM: ```c pm_runtime_set_autosuspend_delay(dev, 500); pm_runtime_use_autosuspend(dev); pm_runtime_enable(dev); ``` Once `pm_runtime_enable()` is called, the runtime PM subsystem is live and = can immediately attempt to suspend the device. If autosuspend isn't configu= red yet, there's a window where a `pm_runtime_put()` could trigger an immed= iate suspend rather than the intended 500ms delayed autosuspend. The delay = should be set first, then autosuspend enabled, then finally runtime PM enab= led. This ordering issue existed in the original `runtime_resume` code too, but = there it was less consequential since the device was already actively resum= ing. In `probe()`, with PM freshly enabled, it matters more. **Observation: Placement relative to host1x_client_register (minor)** The `pm_runtime_enable()` is placed after `host1x_client_register()`. If th= e host1x framework synchronously invokes `gr*d_init()` during registration,= and that callback (or anything it triggers) does `pm_runtime_get_sync()`, = the resume callback won't fire because runtime PM isn't enabled yet. In pra= ctice this is likely safe because `host1x_client_register` just adds the cl= ient to a list, but it's worth considering whether enabling PM before regis= tration would be more robust. That would require adding `pm_runtime_disable= ()` to the error path for `host1x_client_register()` failure though. **Observation: Missing Fixes tag** If this addresses a real bug (runtime PM never properly enabled on first bo= ot without a sleep/resume cycle), a `Fixes:` tag referencing the commit tha= t originally placed `pm_runtime_enable` inside `runtime_resume` would help = stable backport triage. **Style nit: gr3d uses `&pdev->dev` vs gr2d uses `dev`** In `gr2d_probe`, there's a local `struct device *dev =3D &pdev->dev;`, so t= he patch correctly uses `dev`. In `gr3d_probe`, there's no such local varia= ble, so `&pdev->dev` is used. This is consistent with the existing code in = each file =E2=80=94 no issue, just noting the difference. **Summary**: The change is correct in principle and fixes a real bug. Pleas= e reorder the three calls so `pm_runtime_enable()` comes last (after config= uring autosuspend), and consider adding a `Fixes:` tag. --- Generated by Claude Code Patch Reviewer