Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm/adreno: stale GMU_STATUS_OOB_PERF_SET after a6xx_gmu_force_off() - [Medium] drm/msm/adreno: premature GMU_OOB_PERFCOUNTER_SET in a6xx_gmu_ifpc_update() - [Medium] drm/msm/adreno: unhandled OOB errors in a6xx_gmu_ifpc_update_locked() --
commit 15aefb4a3ef6c261aac3a46fc7b4e9cde2215554 Author: Rob Clark <[email protected]> drm/msm: Rework IFPC disable This commit decouples IFPC inhibition from the sysprof refcount by introducing a dedicated gpu->ifpc_disable refcount and replacing gpu->funcs->sysprof_setup() with gpu->funcs->ifpc_update(). > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 662f09bec9d52..bba5468db42e9 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -2128,29 +2128,33 @@ 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_locked(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)) > - return; > - > - mutex_lock(&gmu->lock); > + WARN_ON_ONCE(!mutex_is_locked(&gmu->lock)); > > - /* > - * '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); > +} [Severity: High] As reported by Sashiko on v1 [1], can a6xx_gmu_ifpc_update_locked() skip re-asserting GMU_OOB_PERFCOUNTER_SET during hw_init() after a forced GMU shutdown? GMU_STATUS_OOB_PERF_SET in gmu->status is normally cleared during a graceful shutdown in a6xx_gmu_shutdown(). However, if a6xx_gmu_stop() takes the a6xx_gmu_force_off() path because gmu->hung is true (or if a6xx_gmu_shutdown() jumps to force_off when GMU_OOB_GPU_SET fails), a6xx_gmu_force_off() resets the GMU CM3 core and powers down RPMh without clearing GMU_STATUS_OOB_PERF_SET: drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_stop() { if (gmu->hung) a6xx_gmu_force_off(gmu); else a6xx_gmu_shutdown(gmu); } Previously, hw_init() in a6xx_gpu.c and a8xx_gpu.c unconditionally called a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) and set GMU_STATUS_OOB_PERF_SET when IFPC inhibition was active. With hw_init() now calling a6xx_gmu_ifpc_update_locked(), test_and_set_bit() sees the stale GMU_STATUS_OOB_PERF_SET bit and skips sending GMU_OOB_PERFCOUNTER_SET to the freshly reset GMU, leaving IFPC enabled in hardware while gpu->ifpc_disable > 1. > > - mutex_unlock(&gmu->lock); > +void a6xx_gmu_ifpc_update(struct msm_gpu *gpu) > +{ > + 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; > + > + /* Nothing to do if GPU is suspended. We will handle this during GMU > resume */ > + if (pm_runtime_get_if_active(&gpu->pdev->dev) <= 0) > + return; > + > + scoped_guard (mutex, &gmu->lock) > + a6xx_gmu_ifpc_update_locked(gpu); > > pm_runtime_put(&gpu->pdev->dev); > } [Severity: Medium] Can a6xx_gmu_ifpc_update() send GMU_OOB_PERFCOUNTER_SET after runtime resume before hw_init() has completed the initial GMU_OOB_GPU_SET handshake? When the GPU is runtime-resumed via adreno_runtime_resume() -> a6xx_gmu_pm_resume(), the GMU is booted and gpu->needs_hw_init is set to true, while msm_gpu_hw_init() is deferred until later. At that point, pm_runtime_get_if_active(&gpu->pdev->dev) returns 1 even though hw_init() has not run yet. In msm_context_set_sysprof(): drivers/gpu/drm/msm/msm_submitqueue.c:msm_context_set_sysprof() { case 2: pm_runtime_get_sync(&gpu->pdev->dev); fallthrough; case 1: /* Disable zapping perfcntrs on context switch: */ refcount_inc(&gpu->sysprof_active); /* Disable IFPC: */ msm_gpu_disable_ifpc_get(gpu); } If the GPU was runtime-suspended, pm_runtime_get_sync() wakes the GMU with gpu->needs_hw_init == true, and msm_gpu_disable_ifpc_get() immediately calls a6xx_gmu_ifpc_update() -> a6xx_gmu_ifpc_update_locked(). This issues GMU_OOB_PERFCOUNTER_SET before hw_init() performs the initial GMU_OOB_GPU_SET handshake (which commit d34b6919798c noted confuses GMU firmware state), and sets GMU_STATUS_OOB_PERF_SET in gmu->status so that the subsequent hw_init() call to a6xx_gmu_ifpc_update_locked() skips sending GMU_OOB_PERFCOUNTER_SET after clearing GMU_OOB_GPU_SET. Should a6xx_gmu_ifpc_update_locked() (or a6xx_gmu_ifpc_update()) check !gpu->needs_hw_init before touching GMU OOB state? > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 92e8c8abb86da..773c1727dd31a 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_locked(gpu); > > return ret; > } [Severity: Medium] As reported by Sashiko on v1 [2], is it intentional that hw_init() in both a6xx_gpu.c and a8xx_gpu.c now calls a6xx_gmu_ifpc_update_locked() at the out label even when ret != 0, and ignores errors from a6xx_gmu_set_oob()? Previously, if adreno_hw_init() or CP initialization failed and jumped to out with a non-zero ret, GMU_OOB_PERFCOUNTER_SET was skipped because of the !ret check. In addition, if a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) timed out, GMU_STATUS_OOB_PERF_SET was not set and the error was returned from hw_init(). With this change, a6xx_gmu_ifpc_update_locked() sets GMU_STATUS_OOB_PERF_SET via test_and_set_bit() before calling a6xx_gmu_set_oob() and discards its return value, leaving GMU_STATUS_OOB_PERF_SET set even if the OOB request fails. [1] https://lore.kernel.org/r/[email protected] [2] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
