Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm: race on IFPC inhibition in msm_gpu_open() via sysprof_setup() - [High] drm/msm: deadlock in msm_gpu_open() calling sysprof_setup() under gpu->lock --
commit 8cfcd9503e85141418ee70c5995afa62adc06b2b Author: Rob Clark <[email protected]> drm/msm: Inhibit IFPC for state snapshot This commit inhibits inter-frame power collapse (IFPC) via gpu->funcs->sysprof_setup() around gpu->funcs->gpu_state_get() in msm_gpu_open() so the GPU stays awake while taking a debugfs state snapshot. > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c > b/drivers/gpu/drm/msm/msm_debugfs.c > index d05307bfa1235..c1d14d59e4d13 100644 > --- a/drivers/gpu/drm/msm/msm_debugfs.c > +++ b/drivers/gpu/drm/msm/msm_debugfs.c > @@ -86,7 +86,17 @@ static int msm_gpu_open(struct inode *inode, struct file > *file) > > pm_runtime_get_sync(&gpu->pdev->dev); > msm_gpu_hw_init(gpu); > + > + /* Inhibit IFPC while accessing registers: */ > + if (gpu->funcs->sysprof_setup) > + gpu->funcs->sysprof_setup(gpu, true); [Severity: High] Can calling gpu->funcs->sysprof_setup(gpu, true) while holding gpu->lock in msm_gpu_open() deadlock against adreno_fault_handler()? When an active GPU submit triggers a flood of IOMMU faults that stalls the SMMU and wedges the GMU, adreno_fault_handler() reinitializes fault_coredump_done and calls msm_gpu_fault_crashstate_capture(), which blocks on gpu->lock before completing fault_coredump_done: drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_fault_handler() { ... reinit_completion(&adreno_gpu->fault_coredump_done); ... msm_gpu_fault_crashstate_capture(gpu, &fault_info); complete_all(&adreno_gpu->fault_coredump_done); } drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_fault_crashstate_capture() { ... mutex_lock(&gpu->lock); ... } Meanwhile, if msm_gpu_open() already holds gpu->lock and calls gpu->funcs->sysprof_setup(gpu, true) -> a6xx_gmu_sysprof_setup() -> a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET), the GMU OOB poll can time out because the GMU is wedged by the pending GPU faults. When that happens, a6xx_gmu_set_oob() waits on fault_coredump_done while holding gpu->lock and gmu->lock: drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_set_oob() { ... if (completion_done(&a6xx_gpu->base.fault_coredump_done)) break; /* We may timeout because the GMU is temporarily wedged from * pending faults from the GPU and we are taking a devcoredump. * Wait until the MMU is resumed and try again. */ wait_for_completion(&a6xx_gpu->base.fault_coredump_done); } Since msm_gpu_open() holds gpu->lock while waiting for fault_coredump_done, and adreno_fault_handler() waits for gpu->lock before completing fault_coredump_done, wouldn't both threads hang permanently? > + > show_priv->state = gpu->funcs->gpu_state_get(gpu); > + > + /* Re-enable IFPC: */ > + if (gpu->funcs->sysprof_setup) > + gpu->funcs->sysprof_setup(gpu, false); [Severity: High] Could concurrent callers of gpu->funcs->sysprof_setup() race on the GMU_STATUS_OOB_PERF_SET vote and re-enable IFPC while registers are still being read? In drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_sysprof_setup() does not reference-count the force_on parameter, and instead tracks the OOB vote with a single bit in gmu->status: drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_sysprof_setup() { bool sysprof = msm_gpu_sysprof_no_ifpc(gpu) || force_on; ... if (sysprof && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET); else if (!sysprof && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET); } Although msm_gpu_open() holds gpu->lock, two other paths call gpu->funcs->sysprof_setup() without holding gpu->lock: 1. sample_worker() in drivers/gpu/drm/msm/msm_perfcntr.c runs on gpu->worker without gpu->lock and brackets performance counter reads with sysprof_setup(gpu, true) and sysprof_setup(gpu, false). 2. msm_context_set_sysprof() in drivers/gpu/drm/msm/msm_submitqueue.c runs under ctx->ctxlock without gpu->lock and calls sysprof_setup(gpu, false). This is also called unconditionally on every DRM file close via msm_postclose() -> msm_context_set_sysprof(ctx, priv->gpu, 0) in drivers/gpu/drm/msm/msm_drv.c, even when the context never enabled sysprof. When msm_gpu_sysprof_no_ifpc(gpu) is false (refcount_read(&gpu->sysprof_active) == 1), if sample_worker() finishes a sample or any DRM fd is closed in msm_postclose() while msm_gpu_open() is inside gpu->funcs->gpu_state_get(gpu), the concurrent sysprof_setup(gpu, false) call will clear GMU_STATUS_OOB_PERF_SET and drop GMU_OOB_PERFCOUNTER_SET mid-snapshot. Conversely, when msm_gpu_open() finishes and calls sysprof_setup(gpu, false), it will clear the OOB vote while sample_worker() may still be reading performance counter registers, allowing the GMU to power-collapse the GX domain during MMIO register access. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
