Re: [PATCH v3 1/4] drm/msm: Rework IFPC disable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: unpowered GMU MMIO access in a6xx_gmu_ifpc_update()
- [High] drm/msm: IFPC disable state lost across GMU shutdown in hw_init()
- [High] drm/msm: deadlock on gpu->lock in a6xx_gmu_ifpc_update()
--

commit 237527664421f3d36abbf31e05d83ea8e6da23e7
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Rework IFPC disable

This commit decouples IFPC inhibition from the sysprof refcount by
introducing a dedicated gpu->ifpc_disable refcount and replacing
gpu->funcs->sysprof_setup() with gpu->funcs->ifpc_update().

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 662f09bec9d52..28d93c60929c8 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -2128,31 +2128,33 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, struct platform_device *pdev,
>  	return irq;
>  }
>  
> -void a6xx_gmu_sysprof_setup(struct msm_gpu *gpu, bool force_on)
> +void a6xx_gmu_ifpc_update(struct msm_gpu *gpu)
>  {
> -	bool sysprof = msm_gpu_sysprof_no_ifpc(gpu) || force_on;
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>  	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>  	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>  
> -	/* Nothing to do if GPU is suspended. We will handle this during GMU resume */
> -	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
> -		return;
> +	/* Caller must hold a runpm ref: */
> +	drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev));
>  
> -	mutex_lock(&gmu->lock);
> +	guard(mutex)(&gpu->lock);
> +	guard(mutex)(&gmu->lock);

[Severity: High]
Can acquiring gpu->lock in a6xx_gmu_ifpc_update() cause a deadlock with
stream->sample_work?

In drivers/gpu/drm/msm/msm_perfcntr.c, sample_worker() now calls
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), which invoke
a6xx_gmu_ifpc_update() and take gpu->lock.

However, msm_perfcntr_suspend_locked() synchronously cancels sample_work
while holding gpu->perfcntr_lock (and while running in the runtime suspend
path):

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_suspend_locked() {
	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);
}

With sample_worker() also acquiring gpu->lock, doesn't
kthread_cancel_work_sync(&stream->sample_work) hit the same two deadlock
scenarios described in that comment (against callers holding gpu->lock across
runtime PM transitions such as msm_gpu_fault_crashstate_capture() or
msm_gpu_submit(), and against sel_worker() taking gpu->lock before
gpu->perfcntr_lock)?

> +
> +	bool ifpc_disable = refcount_read(&gpu->ifpc_disable) > 1;
>  
>  	/*
> -	 * 'Perfcounter select' register values are lost during IFPC collapse. To avoid that,
> -	 * use the currently unused perfcounter oob vote to block IFPC when sysprof is active
> +	 * But the caller need-not ensure hw_init is done.  If we need_hw_init,
> +	 * simply update the gmu->status mask, and hw_init() will re-apply the
> +	 * OOB request when it is done.  This way we preserve the ordering of
> +	 * OOB requests that the fw expects.
>  	 */
> -	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);
> -
> -	mutex_unlock(&gmu->lock);
> -
> -	pm_runtime_put(&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);
> +	}
>  }
>  
>  void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 92e8c8abb86da..83f87ae160b50 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1537,10 +1537,9 @@ static int hw_init(struct msm_gpu *gpu)
>  		a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_BOOT_SLUMBER);
>  	}
>  
> -	if (!ret && msm_gpu_sysprof_no_ifpc(gpu)) {
> +	/* 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 (!ret)
> -			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
>  	}
>  
>  	return ret;

[Severity: High]
Will GMU_STATUS_OOB_PERF_SET still be set in gmu->status when hw_init() runs
after a GMU stop/resume cycle (both here in a6xx_gpu.c and in
a8xx_gpu.c:hw_init())?

When the GMU stops during runtime suspend, system suspend, or GPU recovery,
a6xx_gmu_stop() -> a6xx_gmu_shutdown() 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);
}

When the GPU later resumes and runs hw_init(), test_bit(GMU_STATUS_OOB_PERF_SET,
&gmu->status) will evaluate to false even though refcount_read(&gpu->ifpc_disable)
is still greater than 1, leaving IFPC enabled in hardware.

Conversely, if a6xx_gmu_ifpc_update() sets GMU_STATUS_OOB_PERF_SET while
gpu->needs_hw_init is true (skipping a6xx_gmu_set_oob()) and the GPU suspends
before hw_init() runs, won't a6xx_gmu_shutdown() see GMU_STATUS_OOB_PERF_SET
and call a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET) for an OOB vote
that was never set in hardware?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index d307cdd17d908..8434a09319bcf 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -24,7 +24,10 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
>  		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;
> @@ -36,16 +39,15 @@ 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: */
> +		msm_gpu_disable_ifpc_put(gpu);
> +		/* Drop our vote for disabling perfcntr zapping: */
>  		refcount_dec(&gpu->sysprof_active);
>  		fallthrough;
>  	case 0:
>  		break;
>  	}

[Severity: High]
What ensures the GPU is runtime-resumed when msm_context_set_sysprof() calls
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put()?

In msm_context_set_sysprof(), only case 2 takes a runtime PM reference via
pm_runtime_get_sync(&gpu->pdev->dev). When userspace sets sysprof to 1 (or
unwinds ctx->sysprof == 1 on ioctl or DRM file close), case 1 calls
msm_gpu_disable_ifpc_get() or msm_gpu_disable_ifpc_put() without holding a
runtime PM reference. Even when unwinding ctx->sysprof == 2,
pm_runtime_put_autosuspend() is called before falling through to case 1.

In a6xx_gmu_ifpc_update(), pm_runtime_get_if_active() was removed and the
OOB register access is now gated only on !gpu->needs_hw_init:

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 set to true at the beginning of
a6xx_gmu_pm_resume() rather than in a6xx_gmu_pm_suspend(), gpu->needs_hw_init
remains false while the GPU is runtime-suspended after its initial boot. If
msm_context_set_sysprof() is called while the GPU is runtime-suspended, won't
a6xx_gmu_ifpc_update() trigger drm_WARN_ON_ONCE() and then perform unclocked
MMIO reads and writes in a6xx_gmu_set_oob() or a6xx_gmu_clear_oob()?

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