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/msm/a6xx: Limit GXPD votes to recovery in A8x Date: Tue, 28 Apr 2026 15:05:24 +1000 Message-ID: In-Reply-To: <20260427-gfx-clk-fixes-v2-6-797e54b3d464@oss.qualcomm.com> References: <20260427-gfx-clk-fixes-v2-0-797e54b3d464@oss.qualcomm.com> <20260427-gfx-clk-fixes-v2-6-797e54b3d464@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Author:** Akhil P Oommen This is the largest and most complex patch. It introduces `a6xx_gmu_gxpd_get()` and `a6xx_gmu_gxpd_put()` helpers that behave differently for A8x vs older generations. **`a6xx_gmu_gxpd_get()`:** ```c + if (adreno_gpu->info->family >= ADRENO_8XX_GEN1) + return 0; + return pm_runtime_get_sync(gmu->gxpd); ``` For A8x, this is a no-op. For older gens, it does the existing `pm_runtime_get_sync()`. **`a6xx_gmu_gxpd_put()`:** ```c + if (adreno_gpu->info->family < ADRENO_8XX_GEN1) + return pm_runtime_put_sync(gmu->gxpd); + + if (adreno_gpu->funcs->gx_is_on(adreno_gpu)) { + pm_runtime_get_sync(gmu->gxpd); + dev_pm_genpd_synced_poweroff(gmu->gxpd); + pm_runtime_put_sync(gmu->gxpd); + } ``` For A8x during the "put" path, if GX is still on, it does a get/synced_poweroff/put sequence to force-collapse the GDSC. **Issue 1 -- Asymmetric get/put for A8x:** On A8x, `gxpd_get()` is a no-op but `gxpd_put()` may do `pm_runtime_get_sync() + pm_runtime_put_sync()`. This is fine from a refcount perspective since the get/put pair is balanced within `gxpd_put()` itself, but the naming is misleading. The function isn't really a "put" for A8x -- it's "force-collapse GX if it's still on". Consider a comment or renaming. **Issue 2 -- Error path in `a6xx_gmu_resume()`:** The error path at `rpm_put:` now calls: ```c + a6xx_gmu_gxpd_put(gmu); ``` For A8x, if GX happens to be on during this error path, `gxpd_put()` will do a `pm_runtime_get_sync()` + `dev_pm_genpd_synced_poweroff()` + `pm_runtime_put_sync()`. This seems overly aggressive for a probe/resume error path. In the original code, this error path called `pm_runtime_put(gmu->gxpd)` which just dropped the refcount obtained earlier. Since `gxpd_get()` is now a no-op on A8x, the error path should also be a no-op -- the force-collapse logic is only appropriate for the `a6xx_gmu_stop()` shutdown path. This could cause unexpected behavior on the error path. **Issue 3 -- Return value of `a6xx_gmu_gxpd_get()`:** On pre-A8x, `pm_runtime_get_sync()` can return a positive value on success or a negative error. The existing code at the call site in `a6xx_gmu_resume()` doesn't check the return value (it didn't before either), so this is a pre-existing issue, not introduced by this patch. **Issue 4 -- Removed blank line:** There's a removed blank line between the end of `gdsc_gx_do_nothing_enable` export and the new functions: ```c } EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable); + +static int a6xx_gmu_gxpd_get(...) ``` Wait, that's in gdsc.c (patch 1). In patch 6, there's also a stray blank line removal: ```c - int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu) ``` This looks like an extra blank line was removed, which is fine. **Issue 5 -- Stop path change:** In `a6xx_gmu_stop()`, the original code had: ```c if (!IS_ERR_OR_NULL(gmu->gxpd)) pm_runtime_put_sync(gmu->gxpd); ``` The new `a6xx_gmu_gxpd_put()` already checks `IS_ERR_OR_NULL(gmu->gxpd)`, so this is functionally equivalent for pre-A8x. For A8x, the stop path now does the force-collapse dance, which is the intended behavior during recovery (since `a6xx_gmu_stop()` is called from the recovery path when `gmu->hung` is true). Overall the design intent is sound, but the error-path behavior in resume deserves closer attention to ensure `gxpd_put()` on A8x doesn't do the force-collapse when it shouldn't. --- Generated by Claude Code Patch Reviewer