Re: [PATCH v4 3/6] drm/msm: Rework IFPC disable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: recursive mutex deadlock on gpu->perfcntr_lock in sel_worker()
- [Medium] drm/msm/adreno: unmatched OOB clear in a6xx_gmu_shutdown() when uninitialized
--

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

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..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);
>  
>  	ret = a6xx_gmu_wait_for_idle(gmu);

[Severity: Medium]
As reported by Sashiko on v3 [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 again without hw_init() running?

With this change, a6xx_gmu_shutdown() keeps GMU_STATUS_OOB_PERF_SET set in
gmu->status across suspend, and hw_init() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c
and drivers/gpu/drm/msm/adreno/a8xx_gpu.c only re-asserts
GMU_OOB_PERFCOUNTER_SET on the GMU at the end of hardware initialization:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() {
	...
	/* 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 when sysprof is set to 1) and the GPU runtime-suspends,
GMU_STATUS_OOB_PERF_SET remains set. If the GMU is later resumed (setting
gpu->needs_hw_init = true in a6xx_gmu_pm_resume()) and suspends again
without msm_gpu_hw_init() running, such as when querying MSM_PARAM_TIMESTAMP
in adreno_get_param() or closing another DRM file descriptor in
msm_postclose() -> msm_context_set_sysprof(ctx, priv->gpu, 0),
GMU_OOB_PERFCOUNTER_SET is never asserted during that wakeup cycle.

When a6xx_gmu_shutdown() then runs while adreno_gpu->base.needs_hw_init is
 still true, test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) evaluates to
true and sends an unpaired GMU_OOB_PERFCOUNTER_SET clear to the GMU. Should
clearing GMU_OOB_PERFCOUNTER_SET in a6xx_gmu_shutdown() be skipped when
adreno_gpu->base.needs_hw_init is true?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..bf73752dac3de 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -259,8 +259,7 @@ sample_worker(struct kthread_work *work)
>  	}
>  
>  	/* Inhibit IFPC while accessing registers: */
> -	if (gpu->funcs->sysprof_setup)
> -		gpu->funcs->sysprof_setup(gpu, true);
> +	msm_gpu_disable_ifpc_get(gpu);

[Severity: High]
Does calling msm_gpu_hw_init() from sel_worker() in
drivers/gpu/drm/msm/msm_perfcntr.c cause a recursive mutex deadlock on
gpu->perfcntr_lock when gpu->needs_hw_init is true?

In drivers/gpu/drm/msm/msm_perfcntr.c, sel_worker() acquires both gpu->lock
and gpu->perfcntr_lock before calling msm_gpu_hw_init(gpu):

drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() {
	...
	scoped_guard (mutex, &gpu->lock) {
		guard(mutex)(&gpu->perfcntr_lock);

		if (stream == gpu->perfcntrs->stream) {
			msm_gpu_hw_init(gpu);
			gpu->funcs->perfcntr_configure(gpu, ring, stream);
		}
	}
}

When gpu->needs_hw_init is true, msm_gpu_hw_init() in
drivers/gpu/drm/msm/msm_gpu.c runs gpu->funcs->hw_init(gpu) and then calls
msm_perfcntr_resume(gpu):

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_hw_init() {
	...
	disable_irq(gpu->irq);
	ret = gpu->funcs->hw_init(gpu);
	if (!ret) {
		msm_perfcntr_resume(gpu);
		gpu->needs_hw_init = false;
	}
	enable_irq(gpu->irq);
}

msm_perfcntr_resume() in drivers/gpu/drm/msm/msm_perfcntr.c then attempts
to acquire gpu->perfcntr_lock again on the same thread:

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_resume() {
	if (!gpu->perfcntrs)
		return 0;
	guard(mutex)(&gpu->perfcntr_lock);
	return msm_perfcntr_resume_locked(gpu->perfcntrs->stream);
}

This call chain sel_worker() -> msm_gpu_hw_init() -> msm_perfcntr_resume()
can be reached whenever sel_worker() runs while the GPU is runtime-active
and gpu->needs_hw_init is true, such as when a perfcntr stream is configured
via msm_ioctl_perfcntr_config() during the autosuspend window after a path
wakes the GPU without calling msm_gpu_hw_init() (for example
adreno_get_param() handling MSM_PARAM_TIMESTAMP, or msm_postclose() calling
msm_context_set_sysprof(ctx, priv->gpu, 0)). Would calling
msm_gpu_hw_init(gpu) under gpu->lock before acquiring gpu->perfcntr_lock in
sel_worker() avoid this deadlock?

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

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