Re: [PATCH v3 2/4] drm/msm: Add cleanup help for disabling IFPC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: deadlock on gpu->lock in sample_worker() via guard(ifpc_disable)
- [High] drm/msm: unclocked GMU access in msm_context_set_sysprof() when suspended
- [Medium] drm/msm: IFPC-disable OOB state lost across suspend in a6xx_gmu_shutdown()
--

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

drm/msm: Add cleanup help for disabling IFPC

This commit adds a cleanup guard helper for disabling IFPC via
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), and converts
sample_worker() in msm_perfcntr.c to use guard(ifpc_disable).

> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 0334a0f604ac1..e7912d2180a51 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -399,6 +399,10 @@ msm_gpu_disable_ifpc_put(struct msm_gpu *gpu)
>  		gpu->funcs->ifpc_update(gpu);
>  }
>  
> +DEFINE_GUARD(ifpc_disable, struct msm_gpu *,
> +	     msm_gpu_disable_ifpc_get(_T),
> +	     msm_gpu_disable_ifpc_put(_T))

[Severity: High]
Can calling msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put() from
msm_context_set_sysprof() access unclocked GMU registers when the GPU is
runtime-suspended?

In drivers/gpu/drm/msm/msm_submitqueue.c, msm_context_set_sysprof() (called
from adreno_set_param() via MSM_PARAM_SYSPROF and from msm_postclose() when
closing the DRM file) only acquires a runtime PM reference in case 2, and
drops it in case 2 before calling msm_gpu_disable_ifpc_put() when unwinding:

drivers/gpu/drm/msm/msm_submitqueue.c:msm_context_set_sysprof() {
	switch (sysprof) {
	default:
		return UERR(EINVAL, gpu->dev, "Invalid sysprof: %d", sysprof);
	case 2:
		pm_runtime_get_sync(&gpu->pdev->dev);
		fallthrough;
	case 1:
		/* Disable zapping perfcntrs on context switch: */
		refcount_inc(&gpu->sysprof_active);
		/* Disable IFPC: */
		msm_gpu_disable_ifpc_get(gpu);
		fallthrough;
	case 0:
		break;
	}

	/* unwind old value: */
	switch (ctx->sysprof) {
	case 2:
		pm_runtime_put_autosuspend(&gpu->pdev->dev);
		fallthrough;
	case 1:
		/* Drop our vote for inhibiting IFPC: */
		msm_gpu_disable_ifpc_put(gpu);
    ...
}

Both helpers call gpu->funcs->ifpc_update(gpu) -> a6xx_gmu_ifpc_update(gpu)
in drivers/gpu/drm/msm/adreno/a6xx_gmu.c:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
	/* Caller must hold a runpm ref: */
	drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev));
    ...
	if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	}
}

Because gpu->needs_hw_init is only set to true at the start of
a6xx_gmu_pm_resume() and a6xx_pm_resume() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c (and is not set during runtime
suspend), once the GPU has initialized and later entered runtime suspend,
gpu->needs_hw_init remains false while GMU clocks and power domains are off.

When sysprof is set to 1 (or unwound from 1 or 2) while the GPU is
runtime-suspended, won't a6xx_gmu_ifpc_update() trigger the
drm_WARN_ON_ONCE() and call a6xx_gmu_set_oob() or a6xx_gmu_clear_oob() on
powered-off GMU registers?

[Severity: Medium]
Also in the IFPC disable tracking, does GMU_STATUS_OOB_PERF_SET get lost
across GMU suspend and recovery?

During GMU shutdown (a6xx_gmu_pm_suspend() -> a6xx_gmu_stop() ->
a6xx_gmu_shutdown()), a6xx_gmu_shutdown() in
drivers/gpu/drm/msm/adreno/a6xx_gmu.c clears GMU_STATUS_OOB_PERF_SET:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() {
    ...
	if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
    ...
}

However, hw_init() in both drivers/gpu/drm/msm/adreno/a6xx_gpu.c and
drivers/gpu/drm/msm/adreno/a8xx_gpu.c checks GMU_STATUS_OOB_PERF_SET to
decide whether to re-apply the OOB request on resume:

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

Since a6xx_gmu_shutdown() cleared GMU_STATUS_OOB_PERF_SET when stopping the
GMU, won't hw_init() see GMU_STATUS_OOB_PERF_SET as cleared on resume and
fail to re-apply GMU_OOB_PERFCOUNTER_SET even when
refcount_read(&gpu->ifpc_disable) > 1?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index bf73752dac3de..0d33f0eae35d7 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -259,7 +259,7 @@ sample_worker(struct kthread_work *work)
>  	}
>  
>  	/* Inhibit IFPC while accessing registers: */
> -	msm_gpu_disable_ifpc_get(gpu);
> +	guard(ifpc_disable)(gpu);

[Severity: High]
Could calling guard(ifpc_disable)(gpu) in sample_worker() deadlock on
gpu->lock against msm_perfcntr_suspend_locked()?

Here guard(ifpc_disable)(gpu) calls msm_gpu_disable_ifpc_get(gpu) on entry
and msm_gpu_disable_ifpc_put(gpu) on scope exit, both of which invoke
gpu->funcs->ifpc_update(gpu) -> a6xx_gmu_ifpc_update(gpu) and acquire
gpu->lock:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
    ...
	guard(mutex)(&gpu->lock);
	guard(mutex)(&gmu->lock);
    ...
}

Meanwhile, msm_perfcntr_suspend_locked() in
drivers/gpu/drm/msm/msm_perfcntr.c (called with gpu->perfcntr_lock held from
msm_perfcntr_suspend() and msm_perfcntrs_stream_release()) waits
synchronously for sample_work:

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_suspend_locked() {
	if (!stream)
		return;

	hrtimer_cancel(&stream->sample_timer);
	kthread_cancel_work_sync(&stream->sample_work);

	/*
	 * We can't use cancel_work_sync() here, since sel_work acquires
	 * gpu->lock which (a) in suspend path can already be held, or
	 * (b) in release path would invert the order of gpu->lock and
	 * gpu->perfcntr_lock.  Either would cause deadlock.
	 */
	cancel_work(&stream->sel_work);
    ...
}

As explained in the comment for sel_work, synchronously canceling a worker
here that acquires gpu->lock appears to hit both deadlock scenarios:

1. Lock order inversion between gpu->lock and gpu->perfcntr_lock:
sel_worker() in drivers/gpu/drm/msm/msm_perfcntr.c and recover_worker() ->
adreno_recover() -> msm_perfcntr_suspend() acquire gpu->lock before
gpu->perfcntr_lock:

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

If msm_perfcntrs_stream_release() or msm_perfcntr_suspend() holds
gpu->perfcntr_lock and blocks in kthread_cancel_work_sync(&stream->sample_work)
while sample_worker() is waiting for gpu->lock in a6xx_gmu_ifpc_update(), a
concurrent thread holding gpu->lock and waiting for gpu->perfcntr_lock will
deadlock.

2. Runtime suspend while gpu->lock is held: paths that hold gpu->lock across
pm_runtime_put_sync(&gpu->pdev->dev) (such as
msm_gpu_fault_crashstate_capture() in drivers/gpu/drm/msm/msm_gpu.c), or a
concurrent msm_job_run() -> msm_gpu_submit() holding gpu->lock while waiting
in pm_runtime_get_sync(&gpu->pdev->dev) for an in-progress
adreno_runtime_suspend() -> msm_perfcntr_suspend() ->
kthread_cancel_work_sync(&stream->sample_work) to finish, will block forever
when sample_worker() tries to acquire gpu->lock in a6xx_gmu_ifpc_update().

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