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 <[email protected]> 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/[email protected]?part=1
