Re: [PATCH v3 11/16] 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 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 <robin.clark@oss.qualcomm.com>

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/20261029201730.151754-1-robin.clark@oss.qualcomm.com?part=11