Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/msm/adreno: self-deadlock on gmu.lock in hw_init() - [High] drm/msm/adreno: unlocked ifpc_disable read in a6xx_gmu_ifpc_update() - [High] drm/msm/adreno: a8xx hw_init() not updated to check ifpc_disable - [Medium] drm/msm/adreno: stale GMU_STATUS_OOB_PERF_SET after a6xx_gmu_force_off() --
commit 14094c96f7c739970280de8ad207fe78e212a6f7 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..d1fe6c40da83b 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -2128,12 +2128,12 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, > struct platform_device *pdev, > return irq; > } > > -void a6xx_gmu_sysprof_setup(struct msm_gpu *gpu, bool force_on) > +void a6xx_gmu_ifpc_update(struct msm_gpu *gpu) > { > - bool sysprof = msm_gpu_sysprof_no_ifpc(gpu) || force_on; > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > struct a6xx_gmu *gmu = &a6xx_gpu->gmu; > + bool force_on = refcount_read(&gpu->ifpc_disable) > 1; > > /* Nothing to do if GPU is suspended. We will handle this during GMU > resume */ > if (!pm_runtime_get_if_active(&gpu->pdev->dev)) > @@ -2145,9 +2145,9 @@ void a6xx_gmu_sysprof_setup(struct msm_gpu *gpu, bool > force_on) > * 'Perfcounter select' register values are lost during IFPC collapse. > To avoid that, > * use the currently unused perfcounter oob vote to block IFPC when > sysprof is active > */ > - if (sysprof && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &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 (!sysprof && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, > &gmu->status)) > + else if (!force_on && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, > &gmu->status)) > a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET); > > mutex_unlock(&gmu->lock); [Severity: High] Should force_on be evaluated inside the gmu->lock critical section in a6xx_gmu_ifpc_update() rather than before acquiring gmu->lock? Callers of msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), such as sample_worker() and msm_context_set_sysprof(), update gpu->ifpc_disable and invoke gpu->funcs->ifpc_update(gpu) without holding a shared outer lock: drivers/gpu/drm/msm/msm_gpu.h: static inline void msm_gpu_disable_ifpc_get(struct msm_gpu *gpu) { refcount_inc(&gpu->ifpc_disable); if (gpu->funcs->ifpc_update) gpu->funcs->ifpc_update(gpu); } static inline void msm_gpu_disable_ifpc_put(struct msm_gpu *gpu) { refcount_dec(&gpu->ifpc_disable); if (gpu->funcs->ifpc_update) gpu->funcs->ifpc_update(gpu); } Because a6xx_gmu_ifpc_update() reads gpu->ifpc_disable before calling mutex_lock(&gmu->lock), two concurrent callers can race: Thread A (msm_gpu_disable_ifpc_put) refcount_dec(&gpu->ifpc_disable); /* 2 -> 1 */ a6xx_gmu_ifpc_update(gpu) force_on = refcount_read(&gpu->ifpc_disable) > 1; /* false */ Thread B (msm_gpu_disable_ifpc_get) refcount_inc(&gpu->ifpc_disable); /* 1 -> 2 */ a6xx_gmu_ifpc_update(gpu) force_on = refcount_read(&gpu->ifpc_disable) > 1; /* true */ mutex_lock(&gmu->lock); /* sets GMU_OOB_PERFCOUNTER_SET or sees GMU_STATUS_OOB_PERF_SET set */ mutex_unlock(&gmu->lock); /* proceeds to access GX registers in sample_worker() */ Thread A mutex_lock(&gmu->lock); /* uses stale force_on == false and clears GMU_OOB_PERFCOUNTER_SET */ mutex_unlock(&gmu->lock); This leaves IFPC enabled while gpu->ifpc_disable is 2 and Thread B is actively reading GX registers (or conversely leaves GMU_OOB_PERFCOUNTER_SET asserted when gpu->ifpc_disable is 1). > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 92e8c8abb86da..39a1472e0def0 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -1537,11 +1537,7 @@ static int hw_init(struct msm_gpu *gpu) > a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_BOOT_SLUMBER); > } > > - 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); > - } > + a6xx_gmu_ifpc_update(gpu); > > return ret; > } [Severity: High] Does calling a6xx_gmu_ifpc_update(gpu) inside hw_init() cause a self-deadlock on a6xx_gpu->gmu.lock? In drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a6xx_hw_init() already holds a6xx_gpu->gmu.lock when calling hw_init(): static int a6xx_hw_init(struct msm_gpu *gpu) { ... mutex_lock(&a6xx_gpu->gmu.lock); ret = hw_init(gpu); mutex_unlock(&a6xx_gpu->gmu.lock); return ret; } All callers of msm_gpu_hw_init() -> a6xx_hw_init() -> hw_init() hold an active runtime PM reference on &gpu->pdev->dev, so inside a6xx_gmu_ifpc_update(), pm_runtime_get_if_active(&gpu->pdev->dev) succeeds and mutex_lock(&gmu->lock) attempts to acquire the same mutex a second time. Also, since this call is after the out: label, hw_init() now calls a6xx_gmu_ifpc_update(gpu) even when an earlier step failed with ret != 0, and no longer propagates errors from a6xx_gmu_set_oob(). [Severity: Medium] In addition to the locking issue above, can replacing the unconditional a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) in hw_init() with a6xx_gmu_ifpc_update(gpu) skip re-applying the OOB vote after a forced GMU reset? During a graceful shutdown, a6xx_gmu_shutdown() clears GMU_STATUS_OOB_PERF_SET in gmu->status. However, if the GMU is hung during recovery in a6xx_recover() -> a6xx_gmu_stop() -> a6xx_gmu_force_off(), or if a6xx_gmu_shutdown() fails early and jumps to force_off: drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() { if (adreno_gpu->base.needs_hw_init) { if (a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET)) goto force_off; ... } if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET); } a6xx_gmu_force_off() powers off and resets the GMU without clearing GMU_STATUS_OOB_PERF_SET in gmu->status. When hw_init() subsequently runs to reinitialize the GPU while gpu->ifpc_disable > 1, the !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) check in a6xx_gmu_ifpc_update() will see the stale bit still set and skip sending GMU_OOB_PERFCOUNTER_SET to the freshly reset GMU. [ ... ] > @@ -2925,7 +2921,7 @@ const struct adreno_gpu_funcs a8xx_gpu_funcs = { > .create_private_vm = a6xx_create_private_vm, > .get_rptr = a6xx_get_rptr, > .progress = a8xx_progress, > - .sysprof_setup = a6xx_gmu_sysprof_setup, > + .ifpc_update = a6xx_gmu_ifpc_update, > .perfcntr_configure = a6xx_perfcntr_configure, > .perfcntr_flush = a8xx_perfcntr_flush, > }, [Severity: High] While a8xx_gpu_funcs is updated here to use a6xx_gmu_ifpc_update(), it looks like hw_init() in drivers/gpu/drm/msm/adreno/a8xx_gpu.c still checks msm_gpu_sysprof_no_perfcntr_zap(gpu): drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init() { ... a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET); if (!ret && msm_gpu_sysprof_no_perfcntr_zap(gpu)) { ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET); if (!ret) set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status); } return ret; } Should a8xx_gpu.c:hw_init() also be updated to check gpu->ifpc_disable so that non-sysprof votes on gpu->ifpc_disable are honored across A8xx hardware init (and an active global perfcntr stream with gpu->ifpc_disable == 1 does not leave GMU_OOB_PERFCOUNTER_SET set on resume)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
