Re: [PATCH v4 6/6] 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: recursive deadlock on perfcntr_lock in sel_worker()
- [High] drm/msm: deadlock on fault_coredump_done in msm_gpu_open()
--

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

drm/msm: Inhibit IFPC for state snapshot

This commit inhibits IFPC around gpu_state_get() in msm_gpu_open() so that the
GPU remains 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 9373ac19ee953..394d37ca90711 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -83,6 +83,7 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
>  		scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
>  			msm_gpu_hw_init(gpu);

[Severity: High]
With commit 5d6d3def9b62 ("drm/msm: Resume perfcntrs after hw_init") moving
msm_perfcntr_resume(gpu) into msm_gpu_hw_init(gpu), can sel_worker() in
drivers/gpu/drm/msm/msm_perfcntr.c hit a recursive deadlock on
gpu->perfcntr_lock?

In drivers/gpu/drm/msm/msm_perfcntr.c, sel_worker() acquires
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 (for example, while the GPU is runtime-active
in the autosuspend window after adreno_load_gpu(), MSM_PARAM_TIMESTAMP, or
msm_postclose(), or when racing with a thread between pm_runtime_get_sync()
and acquiring gpu->lock in msm_gpu_open() or msm_context_set_sysprof()),
msm_gpu_hw_init() calls msm_perfcntr_resume(gpu):

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

And msm_perfcntr_resume() unconditionally tries to acquire gpu->perfcntr_lock
again:

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

Won't this self-deadlock on gpu->perfcntr_lock while holding gpu->lock with
gpu->irq disabled?

> +			guard(ifpc_disable)(gpu);

[Severity: High]
Could acquiring ifpc_disable while holding gpu->lock in msm_gpu_open() (as
well as in msm_context_set_sysprof() from commit "drm/msm: Rework IFPC
disable" earlier in this series) cause an ABBA deadlock with
adreno_gpu->fault_coredump_done?

If the GPU is actively executing a submit (so gpu->needs_hw_init is false),
msm_gpu_open() acquires gpu->lock and invokes:

  guard(ifpc_disable)(gpu)
    -> msm_gpu_disable_ifpc_get(gpu)
      -> a6xx_gmu_ifpc_update(gpu)
        -> a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET)

If an active GPU submit concurrently triggers an SMMU page fault with
stall-on-fault enabled (ARM_SMMU_FSR_SS), arm_smmu_context_fault() calls
report_iommu_fault() -> msm_gpu_fault_handler() -> a6xx_fault_handler() ->
adreno_fault_handler() before writing ARM_SMMU_CB_RESUME to un-stall the SMMU:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_fault_handler() {
	...
		reinit_completion(&adreno_gpu->fault_coredump_done);

		fault_info.ttbr0 = info->ttbr0;
		fault_info.iova  = iova;
		fault_info.flags = flags;
		fault_info.type  = type;
		fault_info.block = block;

		msm_gpu_fault_crashstate_capture(gpu, &fault_info);

		complete_all(&adreno_gpu->fault_coredump_done);
	...
}

Inside msm_gpu_fault_crashstate_capture(), the fault handler thread blocks on
mutex_lock(&gpu->lock) because msm_gpu_open() is holding gpu->lock.

Meanwhile, because the SMMU is stalled on the fault, the GMU is wedged and
gmu_poll_timeout() in a6xx_gmu_set_oob() times out and waits on
fault_coredump_done while holding gpu->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 on fault_coredump_done, and
adreno_fault_handler() cannot complete fault_coredump_done or resume the SMMU
until it acquires gpu->lock, won't both threads permanently deadlock?

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