Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/msm/adreno: unmatched OOB clear in a6xx_gmu_shutdown() when uninitialized - [Medium] drm/msm: unchecked msm_gpu_hw_init() in msm_context_set_sysprof() - [Medium] drm/msm: runtime PM ref held before gpu->lock in msm_context_set_sysprof() --
commit 1f9338984ef58bb04f98d64fbb7e1fb8870c9a2f Author: Rob Clark <[email protected]> drm/msm: Rework IFPC disable This commit decouples IFPC inhibition from the sysprof_active refcount by introducing a dedicated ifpc_disable refcount on struct msm_gpu and replacing sysprof_setup with an ifpc_update callback that updates the GMU_OOB_PERFCOUNTER_SET vote. > 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); [Severity: Medium] As reported by Sashiko on v4 [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 without hw_init() running? Previously, GMU_STATUS_OOB_PERF_SET tracked whether GMU_OOB_PERFCOUNTER_SET was currently asserted on the GMU hardware: a6xx_gmu_shutdown() cleared the bit via test_and_clear_bit(), and hw_init() set the bit after re-asserting GMU_OOB_PERFCOUNTER_SET. With this change, GMU_STATUS_OOB_PERF_SET stays set across suspend while GMU_OOB_PERFCOUNTER_SET is only re-asserted at the end of hw_init() in a6xx_gpu.c and a8xx_gpu.c: /* 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 with sysprof == 1), and the GPU runtime-suspends and later resumes and suspends without msm_gpu_hw_init() running (such as when userspace queries MSM_PARAM_TIMESTAMP in adreno_get_param(), or when msm_context_set_sysprof() is called with sysprof == 0 while ctx->sysprof == 0), adreno_gpu->base.needs_hw_init remains true. When a6xx_gmu_shutdown() runs, GMU_OOB_PERFCOUNTER_SET was never set during that wakeup cycle, but test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) is still true and triggers an unpaired OOB clear. Additionally, if a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) fails in a6xx_gmu_ifpc_update() or hw_init(), GMU_STATUS_OOB_PERF_SET remains set while the hardware OOB vote is not active, preventing subsequent a6xx_gmu_ifpc_update() calls from asserting the vote and causing a6xx_gmu_shutdown() to clear an unset OOB vote. And when dropping the last sysprof == 1 vote in msm_context_set_sysprof() while gpu->needs_hw_init is true, msm_gpu_hw_init() will assert GMU_OOB_PERFCOUNTER_SET in hw_init() only for msm_gpu_disable_ifpc_put() to immediately clear it. > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c > b/drivers/gpu/drm/msm/msm_submitqueue.c > index e3e7c401fa5c5..d41756c0152bb 100644 > --- a/drivers/gpu/drm/msm/msm_submitqueue.c > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c > @@ -25,7 +25,13 @@ int msm_context_set_sysprof(struct msm_context *ctx, > struct msm_gpu *gpu, int sy > pm_runtime_get(&gpu->pdev->dev); > fallthrough; > case 1: > + /* Disable zapping perfcntrs on context switch: */ > refcount_inc(&gpu->sysprof_active); > + /* Disable IFPC: */ > + scoped_guard (mutex, &gpu->lock) { > + msm_gpu_hw_init(gpu); > + msm_gpu_disable_ifpc_get(gpu); > + } > fallthrough; > case 0: > break; > @@ -37,16 +43,18 @@ int msm_context_set_sysprof(struct msm_context *ctx, > struct msm_gpu *gpu, int sy > pm_runtime_put_autosuspend(&gpu->pdev->dev); > fallthrough; > case 1: > + /* Drop our vote for inhibiting IFPC: */ > + scoped_guard (mutex, &gpu->lock) { > + msm_gpu_hw_init(gpu); > + msm_gpu_disable_ifpc_put(gpu); > + } [Severity: Medium] What happens if msm_gpu_hw_init(gpu) fails here (or in the case 1 block above)? In msm_gpu_hw_init(), gpu->needs_hw_init is only cleared when gpu->funcs->hw_init(gpu) returns 0: ret = gpu->funcs->hw_init(gpu); if (!ret) { msm_perfcntr_resume(gpu); gpu->needs_hw_init = false; } Because msm_context_set_sysprof() does not check the return value of msm_gpu_hw_init(gpu) (or whether runtime resume via guard(pm_runtime_active) succeeded), a failure leaves gpu->needs_hw_init set to true and still calls msm_gpu_disable_ifpc_get() or msm_gpu_disable_ifpc_put(). That calls a6xx_gmu_ifpc_update(), which hits drm_WARN_ON_ONCE() and proceeds to touch GMU registers via a6xx_gmu_set_oob() or a6xx_gmu_clear_oob() on an uninitialized or suspended GPU/GMU: /* Caller must hold a runpm ref: */ drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev)); drm_WARN_ON_ONCE(gpu->dev, READ_ONCE(gpu->needs_hw_init)); guard(mutex)(&gmu->lock); bool ifpc_disable = refcount_read(&gpu->ifpc_disable) > 1; if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) { a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET); } else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) { a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET); } [Severity: Medium] Also in msm_context_set_sysprof(), can acquiring gpu->lock while holding the pm_runtime_active reference prevent GPU hang recovery from collapsing the CX GDSC? At the start of msm_context_set_sysprof(), a runtime PM reference is acquired before taking gpu->lock: guard(pm_runtime_active)(&gpu->pdev->dev); guard(rwsem_write)(&ctx->ctxlock); When a GPU hang occurs, recover_worker() holds gpu->lock and invokes a6xx_recover() or a8xx_recover(), which drops its runtime PM references and waits for the device to runtime-suspend so the CX GDSC collapses: /* 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 another thread concurrently enters msm_context_set_sysprof() (for example via MSM_PARAM_SYSPROF or via msm_postclose() when a profiling process exits during a hang), it will hold the runtime PM reference from guard(pm_runtime_active) while blocked on scoped_guard(mutex, &gpu->lock). This keeps the runtime PM usage count above zero during recovery, causing gmu->pd_gate to time out and leaving gpu->needs_hw_init false so msm_gpu_hw_init() skips reinitializing the hardware. Other paths that take both (such as msm_gpu_open(), msm_job_run(), and recover_worker(), as noted in commit 917e9b7c2350 ("drm/msm/adreno: Hold gpu-lock acrossWait for recovery before narratives")) acquire gpu->lock before getting the runtime PM reference. [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
