Re: [PATCH v4 12/19] drm/msm: Inhibit IFPC for state snapshot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: race re-enabling IFPC during gpu_state_get() in msm_gpu_open()
- [High] drm/msm: premature GMU_OOB_PERFCOUNTER_SET before msm_gpu_hw_init()
--

commit 158363591a2450fd461587eb02116600b0d3c278
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Inhibit IFPC for state snapshot

This commit adds an ifpc_disable guard in msm_gpu_open() to keep the GPU
awake while capturing a GPU state snapshot via debugfs.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index e1b872576c7b1..14b588aeeb562 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -81,6 +81,8 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  		return -ENOMEM;
>  
>  	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +		guard(ifpc_disable)(gpu);
> +
>  		scoped_cond_guard (mutex_intr, return PTR_ERR(scope.lock), &gpu->lock) {
>  			msm_gpu_hw_init(gpu);
>  			show_priv->state = gpu->funcs->gpu_state_get(gpu);

[Severity: High]
As reported by Sashiko on v3 [1], can concurrent callers clear the
GMU_OOB_PERFCOUNTER_SET vote and re-enable IFPC while gpu_state_get() or
sample_worker() is still reading GPU GX registers?

In drivers/gpu/drm/msm/msm_gpu.h, the ifpc_disable guard toggles
ifpc_update() with a boolean rather than a reference count:

DEFINE_GUARD(ifpc_disable, struct msm_gpu *,
	     if (_T->funcs->ifpc_update) _T->funcs->ifpc_update(_T, true),
	     if (_T->funcs->ifpc_update) _T->funcs->ifpc_update(_T, false));

And in drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_ifpc_update() tracks
the OOB request with a single bit (GMU_STATUS_OOB_PERF_SET) in 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 (!force_on && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);

When sysprof is not active (refcount_read(&gpu->sysprof_active) == 1),
several paths can clear GMU_STATUS_OOB_PERF_SET and drop
GMU_OOB_PERFCOUNTER_SET while another thread is inside gpu_state_get():

1. Because guard(ifpc_disable)(gpu) in msm_gpu_open() runs outside
   gpu->lock, a second process opening the debugfs file enters
   guard(ifpc_disable)(gpu) (which is a no-op since GMU_STATUS_OOB_PERF_SET
   is already set) and blocks on gpu->lock. If that waiter is interrupted by
   a signal, or when the first caller unlocks gpu->lock and exits its
   ifpc_disable guard after the second caller has entered gpu_state_get(),
   a6xx_gmu_ifpc_update(gpu, false) clears GMU_OOB_PERFCOUNTER_SET while
   gpu_state_get() is actively accessing GX registers.

2. sample_worker() in drivers/gpu/drm/msm/msm_perfcntr.c runs on gpu->worker
   without gpu->lock and also uses guard(ifpc_disable)(gpu). When either
   sample_worker() or msm_gpu_open() finishes first, its guard cleanup clears
   GMU_OOB_PERFCOUNTER_SET for the other.

3. msm_context_set_sysprof() in drivers/gpu/drm/msm/msm_submitqueue.c runs
   without gpu->lock (including on every DRM fd close via msm_postclose())
   and unconditionally calls gpu->funcs->ifpc_update(gpu, false), which also
   clears GMU_OOB_PERFCOUNTER_SET mid-snapshot.

4. On A8xx, hw_init() in drivers/gpu/drm/msm/adreno/a8xx_gpu.c sets
   GMU_OOB_PERFCOUNTER_SET and GMU_STATUS_OOB_PERF_SET when
   msm_gpu_sysprof_no_perfcntr_zap(gpu) is true (such as when
   gpu->perfcntrs->stream is active with sysprof_active == 1), whereas
   a6xx_gmu_ifpc_update() only checks msm_gpu_sysprof_no_ifpc(gpu). Exiting
   guard(ifpc_disable)(gpu) in msm_gpu_open() will then clear the active
   perfcntr stream's OOB vote.

[Severity: High]
Also in msm_gpu_open(), does placing guard(ifpc_disable)(gpu) before
msm_gpu_hw_init(gpu) assert GMU_OOB_PERFCOUNTER_SET before the initial
GMU_OOB_GPU_SET handshake when waking from runtime suspend?

When the GPU is runtime-suspended, entering scoped_guard(pm_runtime_active,
&gpu->pdev->dev) runs a6xx_gmu_pm_resume(), which sets
gpu->needs_hw_init = true and boots the GMU while deferring the
GMU_OOB_GPU_SET (and legacy GMU_OOB_BOOT_SLUMBER) handshake until
msm_gpu_hw_init(gpu).

Calling guard(ifpc_disable)(gpu) before msm_gpu_hw_init(gpu) causes
a6xx_gmu_ifpc_update(gpu, true) to set GMU_STATUS_OOB_PERF_SET and call
a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) while gpu->needs_hw_init is
still true and before hw_init() has performed the GMU_OOB_GPU_SET handshake.

Furthermore, when msm_gpu_hw_init(gpu) subsequently calls hw_init() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c or
drivers/gpu/drm/msm/adreno/a8xx_gpu.c while msm_gpu_sysprof_no_ifpc(gpu) or
msm_gpu_sysprof_no_perfcntr_zap(gpu) is true:

	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);
	}

hw_init() does not check whether GMU_STATUS_OOB_PERF_SET is already set and
calls a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) a second time while the
OOB bit is already asserted. Should guard(ifpc_disable)(gpu) be placed inside
the gpu->lock scope after msm_gpu_hw_init(gpu)?

[1] https://lore.kernel.org/r/20261029201730.151754-12-robin.clark@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030211928.45761-1-robin.clark@oss.qualcomm.com?part=12