Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm: race re-enabling IFPC during gpu_state_get() in msm_gpu_open() - [High] drm/msm: premature GMU_OOB_PERFCOUNTER_SET before msm_gpu_hw_init() --
commit 158363591a2450fd461587eb02116600b0d3c278 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 a GPU state snapshot via debugfs. > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c > b/drivers/gpu/drm/msm/msm_debugfs.c > index e1b872576c7b1..14b588aeeb562 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); > + > scoped_cond_guard (mutex_intr, return PTR_ERR(scope.lock), > &gpu->lock) { > msm_gpu_hw_init(gpu); > show_priv->state = gpu->funcs->gpu_state_get(gpu); [Severity: High] As reported by Sashiko on v3 [1], can concurrent callers clear the GMU_OOB_PERFCOUNTER_SET vote and re-enable IFPC while gpu_state_get() or sample_worker() is still reading GPU GX registers? In drivers/gpu/drm/msm/msm_gpu.h, the ifpc_disable guard toggles ifpc_update() with a boolean rather than a reference count: DEFINE_GUARD(ifpc_disable, struct msm_gpu *, if (_T->funcs->ifpc_update) _T->funcs->ifpc_update(_T, true), if (_T->funcs->ifpc_update) _T->funcs->ifpc_update(_T, false)); And in drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_ifpc_update() tracks the OOB request with a single bit (GMU_STATUS_OOB_PERF_SET) in gmu->status: 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); When sysprof is not active (refcount_read(&gpu->sysprof_active) == 1), several paths can clear GMU_STATUS_OOB_PERF_SET and drop GMU_OOB_PERFCOUNTER_SET while another thread is inside gpu_state_get(): 1. Because guard(ifpc_disable)(gpu) in msm_gpu_open() runs outside gpu->lock, a second process opening the debugfs file enters guard(ifpc_disable)(gpu) (which is a no-op since GMU_STATUS_OOB_PERF_SET is already set) and blocks on gpu->lock. If that waiter is interrupted by a signal, or when the first caller unlocks gpu->lock and exits its ifpc_disable guard after the second caller has entered gpu_state_get(), a6xx_gmu_ifpc_update(gpu, false) clears GMU_OOB_PERFCOUNTER_SET while gpu_state_get() is actively accessing GX registers. 2. sample_worker() in drivers/gpu/drm/msm/msm_perfcntr.c runs on gpu->worker without gpu->lock and also uses guard(ifpc_disable)(gpu). When either sample_worker() or msm_gpu_open() finishes first, its guard cleanup clears GMU_OOB_PERFCOUNTER_SET for the other. 3. msm_context_set_sysprof() in drivers/gpu/drm/msm/msm_submitqueue.c runs without gpu->lock (including on every DRM fd close via msm_postclose()) and unconditionally calls gpu->funcs->ifpc_update(gpu, false), which also clears GMU_OOB_PERFCOUNTER_SET mid-snapshot. 4. On A8xx, hw_init() in drivers/gpu/drm/msm/adreno/a8xx_gpu.c sets GMU_OOB_PERFCOUNTER_SET and GMU_STATUS_OOB_PERF_SET when msm_gpu_sysprof_no_perfcntr_zap(gpu) is true (such as when gpu->perfcntrs->stream is active with sysprof_active == 1), whereas a6xx_gmu_ifpc_update() only checks msm_gpu_sysprof_no_ifpc(gpu). Exiting guard(ifpc_disable)(gpu) in msm_gpu_open() will then clear the active perfcntr stream's OOB vote. [Severity: High] Also in msm_gpu_open(), does placing guard(ifpc_disable)(gpu) before msm_gpu_hw_init(gpu) assert GMU_OOB_PERFCOUNTER_SET before the initial GMU_OOB_GPU_SET handshake when waking from runtime suspend? When the GPU is runtime-suspended, entering scoped_guard(pm_runtime_active, &gpu->pdev->dev) runs a6xx_gmu_pm_resume(), which sets gpu->needs_hw_init = true and boots the GMU while deferring the GMU_OOB_GPU_SET (and legacy GMU_OOB_BOOT_SLUMBER) handshake until msm_gpu_hw_init(gpu). Calling guard(ifpc_disable)(gpu) before msm_gpu_hw_init(gpu) causes a6xx_gmu_ifpc_update(gpu, true) to set GMU_STATUS_OOB_PERF_SET and call a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) while gpu->needs_hw_init is still true and before hw_init() has performed the GMU_OOB_GPU_SET handshake. Furthermore, when msm_gpu_hw_init(gpu) subsequently calls hw_init() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c or drivers/gpu/drm/msm/adreno/a8xx_gpu.c while msm_gpu_sysprof_no_ifpc(gpu) or msm_gpu_sysprof_no_perfcntr_zap(gpu) is true: if (!ret && msm_gpu_sysprof_no_ifpc(gpu)) { ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET); if (!ret) set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status); } hw_init() does not check whether GMU_STATUS_OOB_PERF_SET is already set and calls a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) a second time while the OOB bit is already asserted. Should guard(ifpc_disable)(gpu) be placed inside the gpu->lock scope after msm_gpu_hw_init(gpu)? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
