Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm: recursive mutex deadlock on gpu->perfcntr_lock in sel_worker() - [Medium] drm/msm/adreno: unmatched OOB clear in a6xx_gmu_shutdown() when uninitialized --
commit da3fce8126509b0693832ae064a79a5dbbf0cc40 Author: Rob Clark <[email protected]> drm/msm: Rework IFPC disable This commit decouples IFPC inhibition from the sysprof refcount by introducing a dedicated ifpc_disable refcount on struct msm_gpu and replacing sysprof_setup() with ifpc_update(). > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 662f09bec9d52..b02bcaa4bd96f 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -1443,7 +1443,7 @@ static void a6xx_gmu_shutdown(struct a6xx_gmu *gmu) > a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET); > } > > - if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) > + if (test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) > a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET); > > ret = a6xx_gmu_wait_for_idle(gmu); [Severity: Medium] As reported by Sashiko on v3 [1], can a6xx_gmu_shutdown() issue an unmatched a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET) when the GMU wakes up and shuts down again without hw_init() running? With this change, a6xx_gmu_shutdown() keeps GMU_STATUS_OOB_PERF_SET set in gmu->status across suspend, and hw_init() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c and drivers/gpu/drm/msm/adreno/a8xx_gpu.c only re-asserts GMU_OOB_PERFCOUNTER_SET on the GMU at the end of hardware initialization: drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() { ... /* re-apply IFPC-disable OOB request, if needed: */ if (!ret && test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) { ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET); } } If IFPC disable is active without holding a persistent runtime PM reference (for example when sysprof is set to 1) and the GPU runtime-suspends, GMU_STATUS_OOB_PERF_SET remains set. If the GMU is later resumed (setting gpu->needs_hw_init = true in a6xx_gmu_pm_resume()) and suspends again without msm_gpu_hw_init() running, such as when querying MSM_PARAM_TIMESTAMP in adreno_get_param() or closing another DRM file descriptor in msm_postclose() -> msm_context_set_sysprof(ctx, priv->gpu, 0), GMU_OOB_PERFCOUNTER_SET is never asserted during that wakeup cycle. When a6xx_gmu_shutdown() then runs while adreno_gpu->base.needs_hw_init is still true, test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) evaluates to true and sends an unpaired GMU_OOB_PERFCOUNTER_SET clear to the GMU. Should clearing GMU_OOB_PERFCOUNTER_SET in a6xx_gmu_shutdown() be skipped when adreno_gpu->base.needs_hw_init is true? [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c > b/drivers/gpu/drm/msm/msm_perfcntr.c > index 7fa2e858bd082..bf73752dac3de 100644 > --- a/drivers/gpu/drm/msm/msm_perfcntr.c > +++ b/drivers/gpu/drm/msm/msm_perfcntr.c > @@ -259,8 +259,7 @@ sample_worker(struct kthread_work *work) > } > > /* Inhibit IFPC while accessing registers: */ > - if (gpu->funcs->sysprof_setup) > - gpu->funcs->sysprof_setup(gpu, true); > + msm_gpu_disable_ifpc_get(gpu); [Severity: High] Does calling msm_gpu_hw_init() from sel_worker() in drivers/gpu/drm/msm/msm_perfcntr.c cause a recursive mutex deadlock on gpu->perfcntr_lock when gpu->needs_hw_init is true? In drivers/gpu/drm/msm/msm_perfcntr.c, sel_worker() acquires both gpu->lock and gpu->perfcntr_lock before calling msm_gpu_hw_init(gpu): drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() { ... scoped_guard (mutex, &gpu->lock) { guard(mutex)(&gpu->perfcntr_lock); if (stream == gpu->perfcntrs->stream) { msm_gpu_hw_init(gpu); gpu->funcs->perfcntr_configure(gpu, ring, stream); } } } When gpu->needs_hw_init is true, msm_gpu_hw_init() in drivers/gpu/drm/msm/msm_gpu.c runs gpu->funcs->hw_init(gpu) and then calls msm_perfcntr_resume(gpu): drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_hw_init() { ... disable_irq(gpu->irq); ret = gpu->funcs->hw_init(gpu); if (!ret) { msm_perfcntr_resume(gpu); gpu->needs_hw_init = false; } enable_irq(gpu->irq); } msm_perfcntr_resume() in drivers/gpu/drm/msm/msm_perfcntr.c then attempts to acquire gpu->perfcntr_lock again on the same thread: drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_resume() { if (!gpu->perfcntrs) return 0; guard(mutex)(&gpu->perfcntr_lock); return msm_perfcntr_resume_locked(gpu->perfcntrs->stream); } This call chain sel_worker() -> msm_gpu_hw_init() -> msm_perfcntr_resume() can be reached whenever sel_worker() runs while the GPU is runtime-active and gpu->needs_hw_init is true, such as when a perfcntr stream is configured via msm_ioctl_perfcntr_config() during the autosuspend window after a path wakes the GPU without calling msm_gpu_hw_init() (for example adreno_get_param() handling MSM_PARAM_TIMESTAMP, or msm_postclose() calling msm_context_set_sysprof(ctx, priv->gpu, 0)). Would calling msm_gpu_hw_init(gpu) under gpu->lock before acquiring gpu->perfcntr_lock in sel_worker() avoid this deadlock? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
