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: Tue, 05 May 2026 08:41:52 +1000 Message-ID: In-Reply-To: <20260503163842.213489-3-clamor95@gmail.com> References: <20260503163842.213489-1-clamor95@gmail.com> <20260503163842.213489-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: Good, one typo and one observation.** This patch does three things in both gr2d.c and gr3d.c: 1. Adds `pm_runtime_enable()`, `pm_runtime_use_autosuspend()`, `pm_runtime_= set_autosuspend_delay()` to `gr*d_probe()`. 2. Removes the same three calls from `gr*d_runtime_resume()`. 3. Removes `pm_runtime_dont_use_autosuspend()` and `pm_runtime_force_suspen= d()` from `gr*d_exit()`. This is the correct standard pattern. The current code has `pm_runtime_enab= le()` inside the runtime resume callback, which is architecturally circular= =E2=80=94 the PM core can only call the runtime resume callback if runtime= PM is already enabled. On a second system suspend/resume cycle, `pm_runtim= e_enable()` gets called again with `disable_depth =3D=3D 0`, triggering a k= ernel warning and PM state corruption. **Typo in commit message:** > The current power management configuration causes GR2G/GR3D to malfunctio= n after resume. `GR2G` should be `GR2D`. **Observation =E2=80=94 removal of `pm_runtime_force_suspend()` from `exit(= )`:** The patch removes these lines from `gr2d_exit()` / `gr3d_exit()`: ```c pm_runtime_dont_use_autosuspend(client->dev); pm_runtime_force_suspend(client->dev); ``` This means during driver teardown, the hardware won't be explicitly suspend= ed by `exit()`. The sequence becomes: 1. `remove()`: `pm_runtime_disable()` =E2=80=94 disables PM tracking 2. `remove()`: `host1x_client_unregister()` =E2=86=92 triggers `exit()` =E2= =86=92 frees channels/syncpts/IOMMU 3. devm cleanup releases clocks and resets Since `pm_runtime_disable()` is called before `exit()`, and `gr*d_runtime_s= uspend()` won't be invoked (PM is disabled), the channel won't be explicitl= y stopped via `host1x_channel_stop()` that normally runs in `gr*d_runtime_s= uspend()`. The clocks and resets are devm-managed so they'll be cleaned up = automatically, but skipping `host1x_channel_stop()` during teardown is a be= havioral change worth being aware of. In practice this is likely fine since= all users have been torn down by this point, but it's worth confirming wit= h the Tegra maintainers. **Error path:** `pm_runtime_enable()` is placed as the last operation befor= e `return 0` in both probe functions, so there's no failure path that needs= a corresponding `pm_runtime_disable()` =E2=80=94 this is correct. --- Generated by Claude Code Patch Reviewer