Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm: premature GMU_OOB_PERFCOUNTER_SET before msm_gpu_hw_init() - [High] drm/msm/adreno: stale GMU_STATUS_OOB_PERF_SET across a6xx_gmu_force_off() - [High] drm/msm: pm_runtime_active outside gpu->lock in msm_gpu_open() blocks recovery --
commit c139e04b0443554ac873bfd8c98898fb001e8ce3 Author: Rob Clark <[email protected]> drm/msm: Inhibit IFPC for state snapshot This commit adds an ifpc_disable guard in msm_gpu_open() to keep the GPU awake while capturing the debugfs GPU state snapshot. > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c > b/drivers/gpu/drm/msm/msm_debugfs.c > index 9373ac19ee953..89d5c52e04678 100644 > --- a/drivers/gpu/drm/msm/msm_debugfs.c > +++ b/drivers/gpu/drm/msm/msm_debugfs.c > @@ -81,6 +81,8 @@ static int msm_gpu_open(struct inode *inode, struct file > *file) > return -ENOMEM; > > scoped_guard (pm_runtime_active, &gpu->pdev->dev) { > + guard(ifpc_disable)(gpu); [Severity: High] As reported by Sashiko on v1 [1], can placing guard(ifpc_disable)(gpu) before msm_gpu_hw_init(gpu) send a premature GMU_OOB_PERFCOUNTER_SET vote before the initial GMU_OOB_GPU_SET handshake completes? When msm_gpu_open() is called while the GPU is runtime-suspended, scoped_guard(pm_runtime_active, &gpu->pdev->dev) invokes a6xx_gmu_pm_resume(), which sets gpu->needs_hw_init = true, boots the GMU firmware, and transitions &gpu->pdev->dev to RPM_ACTIVE without running hw_init(). Immediately afterward, guard(ifpc_disable)(gpu) calls: msm_gpu_disable_ifpc_get(gpu) -> a6xx_gmu_ifpc_update(gpu) -> a6xx_gmu_ifpc_update_locked(gpu) In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update(): /* Nothing to do if GPU is suspended. We will handle this during GMU resume */ if (pm_runtime_get_if_active(&gpu->pdev->dev) <= 0) return; scoped_guard (mutex, &gmu->lock) a6xx_gmu_ifpc_update_locked(gpu); Because the outer pm_runtime_active guard already resumed the device, pm_runtime_get_if_active() succeeds even though gpu->needs_hw_init is still true. a6xx_gmu_ifpc_update_locked() then sets GMU_STATUS_OOB_PERF_SET in gmu->status and asserts GMU_OOB_PERFCOUNTER_SET before msm_gpu_hw_init() -> hw_init() sets and clears GMU_OOB_GPU_SET (and GMU_OOB_BOOT_SLUMBER on legacy GMUs). If mutex_intr fails with -EINTR, GMU_OOB_PERFCOUNTER_SET is asserted and cleared without msm_gpu_hw_init() ever running. Furthermore, when msm_gpu_hw_init() subsequently runs hw_init() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c: a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET); if (a6xx_gpu->gmu.legacy) { /* Take the GMU out of its special boot mode */ a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_BOOT_SLUMBER); } a6xx_gmu_ifpc_update_locked(gpu); test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) in a6xx_gmu_ifpc_update_locked() finds the bit already set and becomes a no-op instead of asserting GMU_OOB_PERFCOUNTER_SET after GMU_OOB_GPU_SET is cleared. As noted in commit d34b6919798c ("drm/msm/a6xx: Correct OOB usage"), issuing GMU_OOB_PERFCOUNTER_SET before the GMU_OOB_GPU_SET handshake completes confuses the GMU firmware internal power state. The same sequence also happens in drivers/gpu/drm/msm/msm_submitqueue.c:msm_context_set_sysprof() when sysprof == 2 wakes a suspended GPU: case 2: pm_runtime_get_sync(&gpu->pdev->dev); fallthrough; case 1: /* Disable zapping perfcntrs on context switch: */ refcount_inc(&gpu->sysprof_active); /* Disable IFPC: */ msm_gpu_disable_ifpc_get(gpu); [Severity: High] In a related path, can GMU_STATUS_OOB_PERF_SET remain stale across a forced GMU power-off in a6xx_gmu_force_off() and leave IFPC uninhibited after recovery? In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update_locked(): if (force_on && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET); else if (!force_on && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET); Prior to commit 15aefb4a3ef6 ("drm/msm: Rework IFPC disable"), hw_init() in a6xx_gpu.c and a8xx_gpu.c unconditionally called a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) whenever IFPC was disabled, regardless of the prior state of GMU_STATUS_OOB_PERF_SET. Now hw_init() calls a6xx_gmu_ifpc_update_locked(gpu), which skips a6xx_gmu_set_oob() if GMU_STATUS_OOB_PERF_SET is already set. However, GMU_STATUS_OOB_PERF_SET is only cleared on the graceful shutdown path in a6xx_gmu_shutdown(). When a6xx_gmu_stop() takes the gmu->hung branch to a6xx_gmu_force_off(gmu), or when a6xx_gmu_shutdown() jumps to force_off on an OOB failure, a6xx_gmu_force_off() and a6xx_rpmh_stop() clear GMU_STATUS_FW_START but do not clear GMU_STATUS_OOB_PERF_SET: /* Reset GPU core blocks */ a6xx_gpu_sw_reset(gpu, true); a6xx_rpmh_stop(gmu); If a hang recovery or forced GMU shutdown occurs while gpu->ifpc_disable > 1, won't GMU_STATUS_OOB_PERF_SET stay set in gmu->status across the hardware reset, causing a6xx_gmu_ifpc_update_locked() in hw_init() to skip re-asserting GMU_OOB_PERFCOUNTER_SET on the restarted GMU? > + > scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) { > msm_gpu_hw_init(gpu); > show_priv->state = gpu->funcs->gpu_state_get(gpu); [Severity: High] Does holding pm_runtime_active outside gpu->lock in msm_gpu_open() prevent the GPU from suspending during hang recovery in a6xx_recover() and a8xx_recover()? Prior to commit 8504c24acea0 ("drm/msm/debugfs: Use cleanup.h for gpu state dump"), msm_gpu_open() acquired gpu->lock first and called pm_runtime_get_sync() and pm_runtime_put_sync() inside gpu->lock. With scoped_guard(pm_runtime_active, &gpu->pdev->dev) outside scoped_cond_guard(mutex_intr, return -EINTR, &gpu->lock), msm_gpu_open() acquires an active runtime PM reference before waiting on gpu->lock and releases it asynchronously via pm_runtime_put() after unlocking gpu->lock. During hang recovery, recover_worker() holds gpu->lock and calls a6xx_recover() (or a8xx_recover()), which drops its runtime PM references expecting the device usage count to reach zero so the GMU suspends and the CX GDSC collapses: /* Drop the rpm refcount from active submits */ if (active_submits) pm_runtime_put(&gpu->pdev->dev); /* And the final one from recover worker */ pm_runtime_put_sync(&gpu->pdev->dev); if (!wait_for_completion_timeout(&gmu->pd_gate, msecs_to_jiffies(1000))) DRM_DEV_ERROR(&gpu->pdev->dev, "cx gdsc didn't collapse\n"); If a user or debugging tool opens the debugfs gpu file while recover_worker() holds gpu->lock (or right before recover_worker() takes gpu->lock while the asynchronous pm_runtime_put() from msm_gpu_open() is still pending), won't the extra runtime PM reference keep the usage count above zero, causing gmu->pd_gate to time out with "cx gdsc didn't collapse" and leaving gpu->needs_hw_init false so msm_gpu_hw_init() skips re-initializing the hung GPU? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
