Re: [PATCH v5 6/6] drm/msm: Inhibit IFPC for state snapshot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: deadlock on fault_coredump_done in msm_gpu_open() and set_sysprof()
- [High] drm/msm: runtime PM reference leak in sel_worker() when needs_hw_init is set
- [High] drm/msm: msm_gpu_open() halts CP SQE via a6xx_gpu_state_get() without recovery
--

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

drm/msm: Inhibit IFPC for state snapshot

This series reworks IFPC inhibition into a refcounted helper, moves
performance counter resumption after hardware initialization, and inhibits
IFPC while capturing a GPU state snapshot in debugfs.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index d05307bfa1235..e6c872ed1a017 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -70,43 +70,37 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  	struct drm_device *dev = inode->i_private;
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_gpu *gpu = priv->gpu;
> -	struct msm_gpu_show_priv *show_priv;
>  	int ret;
>  
>  	if (!gpu || !gpu->funcs->gpu_state_get)
>  		return -ENODEV;
>  
> -	show_priv = kmalloc_obj(*show_priv);
> +	struct msm_gpu_show_priv *show_priv __free(kfree)
> +		= kmalloc_obj(*show_priv);
>  	if (!show_priv)
>  		return -ENOMEM;
>  
> -	ret = mutex_lock_interruptible(&gpu->lock);
> -	if (ret)
> -		goto free_priv;
> -
> -	pm_runtime_get_sync(&gpu->pdev->dev);
> -	msm_gpu_hw_init(gpu);
> -	show_priv->state = gpu->funcs->gpu_state_get(gpu);
> -	pm_runtime_put_sync(&gpu->pdev->dev);
> -
> -	mutex_unlock(&gpu->lock);
> +	scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
> +		scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +			msm_gpu_hw_init(gpu);
> +			guard(ifpc_disable)(gpu);

[Severity: High]
As reported by Sashiko on v4 [1], calling guard(ifpc_disable)(gpu) inside
msm_gpu_open() (in drivers/gpu/drm/msm/msm_debugfs.c) and calling
msm_gpu_disable_ifpc_get(gpu) in msm_context_set_sysprof() (in
drivers/gpu/drm/msm/msm_submitqueue.c) while holding gpu->lock can deadlock
with adreno_fault_handler() when an active GPU submission triggers an SMMU
page fault.

In msm_gpu_open() and msm_context_set_sysprof(), gpu->lock is acquired first,
followed by msm_gpu_disable_ifpc_get() -> a6xx_gmu_ifpc_update(), which takes
gmu->lock and calls a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET).

If an active GPU submission concurrently triggers a stalled SMMU fault,
arm_smmu_context_fault() invokes adreno_fault_handler() (in
drivers/gpu/drm/msm/adreno/adreno_gpu.c):

	reinit_completion(&adreno_gpu->fault_coredump_done);
	...
	msm_gpu_fault_crashstate_capture(gpu, &fault_info);
	complete_all(&adreno_gpu->fault_coredump_done);

Inside msm_gpu_fault_crashstate_capture() (in drivers/gpu/drm/msm/msm_gpu.c),
the fault handler attempts to acquire gpu->lock:

	mutex_lock(&gpu->lock);

Meanwhile, because the SMMU is stalled on the fault, the GMU cannot service
OOB interrupts and gmu_poll_timeout() in a6xx_gmu_set_oob() (in
drivers/gpu/drm/msm/adreno/a6xx_gmu.c) times out and waits on
fault_coredump_done while holding gmu->lock and gpu->lock:

	if (completion_done(&a6xx_gpu->base.fault_coredump_done))
		break;

	wait_for_completion(&a6xx_gpu->base.fault_coredump_done);

Can this cause an unrecoverable deadlock where msm_gpu_open() or
msm_context_set_sysprof() holds gpu->lock while waiting for
fault_coredump_done, while adreno_fault_handler() cannot complete
fault_coredump_done or resume the SMMU until it acquires gpu->lock?

> +			show_priv->state = gpu->funcs->gpu_state_get(gpu);
> +		}
> +	}

[Severity: High]
With guard(ifpc_disable)(gpu) waking the GX power domain via
GMU_OOB_PERFCOUNTER_SET before calling gpu->funcs->gpu_state_get(gpu) in
msm_gpu_open(), a6xx_gpu_state_get() (in
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c) will see gx_is_on() return true
and halt the CP SQE before running the hardware crashdumper:

	if (!adreno_gpu->funcs->gx_is_on(adreno_gpu))
		return &a6xx_state->base;

	/* Halt SQE first */
	gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 3);

When msm_gpu_open() finishes, REG_A6XX_CP_SQE_CNTL is not restored and
gpu->needs_hw_init remains false (having already been cleared by
msm_gpu_hw_init(gpu) prior to gpu_state_get()).

If the GPU does not go through a full runtime PM suspend and resume before
the next submission (for example, when submissions are active, before the
asynchronous pm_runtime_put() worker runs, within the autosuspend window, or
while sysprof or performance counters hold a runtime PM or ifpc_disable
reference), msm_gpu_submit() will see gpu->needs_hw_init as false and skip
hw_init.

Won't subsequent GPU submissions stall against the halted CP SQE until the
hangcheck timer fires and forces a GPU recovery?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..e6f4f9b098442 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -199,10 +199,17 @@ sel_worker(struct work_struct *w)
>  		return;
>  
>  	scoped_guard (mutex, &gpu->lock) {
> +		/*
> +		 * If gpu still needs hw_init, bail.  We'll be rescheduled
> +		 * after hw_init, and there is no point to collect counters
> +		 * beforehand
> +		 */
> +		if (gpu->needs_hw_init)
> +			return;

[Severity: High]
In sel_worker() (in drivers/gpu/drm/msm/msm_perfcntr.c),
pm_runtime_get_if_active(&gpu->pdev->dev) acquires a runtime PM reference
before entering scoped_guard(mutex, &gpu->lock):

	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
		return;

	scoped_guard (mutex, &gpu->lock) {
		if (gpu->needs_hw_init)
			return;
		...
	}

	pm_runtime_put_autosuspend(&gpu->pdev->dev);

If the device is runtime-active while gpu->needs_hw_init is still true (such
as during the autosuspend window after adreno_get_param() handles
MSM_PARAM_TIMESTAMP, or between pm_runtime_get_sync() and msm_gpu_hw_init()),
doesn't this early return bypass pm_runtime_put_autosuspend(&gpu->pdev->dev)
and leak the runtime PM reference on &gpu->pdev->dev?

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

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