Re: [PATCH v4 1/6] drm/msm: Hold runpm ref in set_sysprof()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm: unconditional GPU runtime resume in msm_context_set_sysprof()
--

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

drm/msm: Hold runpm ref in set_sysprof()

This commit adds a scoped pm_runtime_active guard in
msm_context_set_sysprof() so a runtime PM reference is held while setting
or clearing GMU OOB requests, and switches the case 2 reference acquisition
from pm_runtime_get_sync() to pm_runtime_get().

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index d307cdd17d908..e3e7c401fa5c5 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -9,6 +9,7 @@
>  
>  int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sysprof)
>  {
> +	guard(pm_runtime_active)(&gpu->pdev->dev);
>  	guard(rwsem_write)(&ctx->ctxlock);

[Severity: Medium]
Does placing guard(pm_runtime_active)(&gpu->pdev->dev) unconditionally at the
entry of msm_context_set_sysprof() cause unnecessary synchronous GPU and GMU
wakeups whenever any DRM file descriptor is closed while the GPU is
runtime-suspended?

In drivers/gpu/drm/msm/msm_drv.c, msm_postclose() unconditionally resets
sysprof to 0 for every closing context:

drivers/gpu/drm/msm/msm_drv.c:msm_postclose() {
	...
	if (priv->gpu)
		msm_context_set_sysprof(ctx, priv->gpu, 0);
	...
}

For normal contexts where ctx->sysprof is already 0, closing a DRM file
descriptor while the GPU is runtime-suspended previously did not wake the
device: neither switch statement matched case 1 or case 2, and
a6xx_gmu_sysprof_setup() returned early when pm_runtime_get_if_active()
returned 0.

With guard(pm_runtime_active)(&gpu->pdev->dev) at the top of
msm_context_set_sysprof(), pm_runtime_get_sync() now runs before checking
whether both sysprof and ctx->sysprof are 0 (or whether sysprof is an invalid
value that will immediately return -EINVAL). When the GPU is suspended, this
synchronously runs adreno_runtime_resume() -> a6xx_gmu_pm_resume() (powering
on GX/CX domains, enabling clocks, and booting GMU firmware with
gpu->needs_hw_init = true), does no work in msm_context_set_sysprof(), and
immediately calls pm_runtime_put() on scope exit.

When the GPU subsequently suspends again via a6xx_gmu_shutdown() in
drivers/gpu/drm/msm/adreno/a6xx_gmu.c, it also has to perform a dummy
GMU_OOB_GPU_SET handshake and increments gpu->suspend_count:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() {
	...
	if (adreno_gpu->base.needs_hw_init) {
		if (a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET))
			goto force_off;

		a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
	}
	...
}

Even after commit da3fce812650 ("drm/msm/a6xx: Split out helper to
disallow/allow IFPC") moves the IFPC updates into case 1 of the two switch
statements, guard(pm_runtime_active)(&gpu->pdev->dev) remains at the top of
msm_context_set_sysprof() and still wakes a suspended GPU on every
msm_postclose().

Could msm_context_set_sysprof() validate sysprof and return early when
ctx->sysprof == sysprof before acquiring the pm_runtime_active reference?

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