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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/msm/adreno: unmatched OOB clear in a6xx_gmu_shutdown() when uninitialized
- [Medium] drm/msm: unchecked msm_gpu_hw_init() in msm_context_set_sysprof()
- [Medium] drm/msm: runtime PM ref held before gpu->lock in msm_context_set_sysprof()
--

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

drm/msm: Rework IFPC disable

This commit decouples IFPC inhibition from the sysprof_active refcount by
introducing a dedicated ifpc_disable refcount on struct msm_gpu and
replacing sysprof_setup with an ifpc_update callback that updates the
GMU_OOB_PERFCOUNTER_SET vote.

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

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

Previously, GMU_STATUS_OOB_PERF_SET tracked whether GMU_OOB_PERFCOUNTER_SET
was currently asserted on the GMU hardware: a6xx_gmu_shutdown() cleared the
bit via test_and_clear_bit(), and hw_init() set the bit after re-asserting
GMU_OOB_PERFCOUNTER_SET.

With this change, GMU_STATUS_OOB_PERF_SET stays set across suspend while
GMU_OOB_PERFCOUNTER_SET is only re-asserted at the end of hw_init() in
a6xx_gpu.c and a8xx_gpu.c:

	/* 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 with sysprof == 1), and the GPU runtime-suspends and later
resumes and suspends without msm_gpu_hw_init() running (such as when
userspace queries MSM_PARAM_TIMESTAMP in adreno_get_param(), or when
msm_context_set_sysprof() is called with sysprof == 0 while ctx->sysprof ==
0), adreno_gpu->base.needs_hw_init remains true. When a6xx_gmu_shutdown()
runs, GMU_OOB_PERFCOUNTER_SET was never set during that wakeup cycle, but
test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) is still true and triggers
an unpaired OOB clear.

Additionally, if a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) fails in
a6xx_gmu_ifpc_update() or hw_init(), GMU_STATUS_OOB_PERF_SET remains set
while the hardware OOB vote is not active, preventing subsequent
a6xx_gmu_ifpc_update() calls from asserting the vote and causing
a6xx_gmu_shutdown() to clear an unset OOB vote. And when dropping the last
sysprof == 1 vote in msm_context_set_sysprof() while gpu->needs_hw_init is
true, msm_gpu_hw_init() will assert GMU_OOB_PERFCOUNTER_SET in hw_init()
only for msm_gpu_disable_ifpc_put() to immediately clear it.

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index e3e7c401fa5c5..d41756c0152bb 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -25,7 +25,13 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
>  		pm_runtime_get(&gpu->pdev->dev);
>  		fallthrough;
>  	case 1:
> +		/* Disable zapping perfcntrs on context switch: */
>  		refcount_inc(&gpu->sysprof_active);
> +		/* Disable IFPC: */
> +		scoped_guard (mutex, &gpu->lock) {
> +			msm_gpu_hw_init(gpu);
> +			msm_gpu_disable_ifpc_get(gpu);
> +		}
>  		fallthrough;
>  	case 0:
>  		break;
> @@ -37,16 +43,18 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
>  		pm_runtime_put_autosuspend(&gpu->pdev->dev);
>  		fallthrough;
>  	case 1:
> +		/* Drop our vote for inhibiting IFPC: */
> +		scoped_guard (mutex, &gpu->lock) {
> +			msm_gpu_hw_init(gpu);
> +			msm_gpu_disable_ifpc_put(gpu);
> +		}

[Severity: Medium]
What happens if msm_gpu_hw_init(gpu) fails here (or in the case 1 block
above)?

In msm_gpu_hw_init(), gpu->needs_hw_init is only cleared when
gpu->funcs->hw_init(gpu) returns 0:

	ret = gpu->funcs->hw_init(gpu);
	if (!ret) {
		msm_perfcntr_resume(gpu);
		gpu->needs_hw_init = false;
	}

Because msm_context_set_sysprof() does not check the return value of
msm_gpu_hw_init(gpu) (or whether runtime resume via guard(pm_runtime_active)
succeeded), a failure leaves gpu->needs_hw_init set to true and still calls
msm_gpu_disable_ifpc_get() or msm_gpu_disable_ifpc_put(). That calls
a6xx_gmu_ifpc_update(), which hits drm_WARN_ON_ONCE() and proceeds to touch
GMU registers via a6xx_gmu_set_oob() or a6xx_gmu_clear_oob() on an
uninitialized or suspended GPU/GMU:

	/* Caller must hold a runpm ref: */
	drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev));
	drm_WARN_ON_ONCE(gpu->dev, READ_ONCE(gpu->needs_hw_init));

	guard(mutex)(&gmu->lock);

	bool ifpc_disable = refcount_read(&gpu->ifpc_disable) > 1;

	if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	}

[Severity: Medium]
Also in msm_context_set_sysprof(), can acquiring gpu->lock while holding the
pm_runtime_active reference prevent GPU hang recovery from collapsing the
CX GDSC?

At the start of msm_context_set_sysprof(), a runtime PM reference is
acquired before taking gpu->lock:

	guard(pm_runtime_active)(&gpu->pdev->dev);
	guard(rwsem_write)(&ctx->ctxlock);

When a GPU hang occurs, recover_worker() holds gpu->lock and invokes
a6xx_recover() or a8xx_recover(), which drops its runtime PM references and
waits for the device to runtime-suspend so the CX GDSC collapses:

	/* And the final one from recover worker */
	pm_runtime_put_sync(&gpu->pdev->dev);

	if (!wait_for_completion_timeout(&gmu->pd_gate, msecs_to_jiffies(1000)))
		DRM_DEV_ERROR(&gpu->pdev->dev, "cx gdsc didn't collapse\n");

If another thread concurrently enters msm_context_set_sysprof() (for
example via MSM_PARAM_SYSPROF or via msm_postclose() when a profiling
process exits during a hang), it will hold the runtime PM reference from
guard(pm_runtime_active) while blocked on scoped_guard(mutex, &gpu->lock).
This keeps the runtime PM usage count above zero during recovery, causing
gmu->pd_gate to time out and leaving gpu->needs_hw_init false so
msm_gpu_hw_init() skips reinitializing the hardware. Other paths that take
both (such as msm_gpu_open(), msm_job_run(), and recover_worker(), as noted
in commit 917e9b7c2350 ("drm/msm/adreno: Hold gpu-lock acrossWait for
recovery before narratives")) acquire gpu->lock before getting the runtime
PM reference.

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

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