| Message ID | 20230221101430.14546-4-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/adreno: fix runtime PM imbalance at unbind | expand |
On Tue, Feb 21, 2023 at 2:16 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > Since commit 4b18299b3365 ("drm/msm/adreno: Defer enabling runpm until > hw_init()") runtime PM is no longer enabled at adreno_gpu_init(), which > means that there are no longer any bind() error paths for which > adreno_gpu_cleanup() is called with runtime PM enabled. > > As the runtime PM enable on first open() is balanced by the > pm_runtime_force_suspend() call at unbind(), adreno_gpu_cleanup() is now > always called with runtime PM disabled so that its pm_runtime_disable() > call can be removed. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index ce6b76c45b6f..1101b8234b49 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -1082,15 +1082,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) > { > - struct msm_gpu *gpu = &adreno_gpu->base; > - struct msm_drm_private *priv = gpu->dev ? gpu->dev->dev_private : NULL; > unsigned int i; > > for (i = 0; i < ARRAY_SIZE(adreno_gpu->info->fw); i++) > release_firmware(adreno_gpu->fw[i]); > > - if (priv && pm_runtime_enabled(&priv->gpu_pdev->dev)) > - pm_runtime_disable(&priv->gpu_pdev->dev); > - Maybe WARN_ON(priv && pm_runtime_enabled(&priv->gpu_pdev->dev))? BR, -R > msm_gpu_cleanup(&adreno_gpu->base); > } > -- > 2.39.2 >
Hi Rob, Sorry about the late follow-up on this. Went down a bit of a DRM rabbit hole this week. On Wed, Feb 22, 2023 at 11:09:16AM -0800, Rob Clark wrote: > On Tue, Feb 21, 2023 at 2:16 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > Since commit 4b18299b3365 ("drm/msm/adreno: Defer enabling runpm until > > hw_init()") runtime PM is no longer enabled at adreno_gpu_init(), which > > means that there are no longer any bind() error paths for which > > adreno_gpu_cleanup() is called with runtime PM enabled. > > > > As the runtime PM enable on first open() is balanced by the > > pm_runtime_force_suspend() call at unbind(), adreno_gpu_cleanup() is now > > always called with runtime PM disabled so that its pm_runtime_disable() > > call can be removed. > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > index ce6b76c45b6f..1101b8234b49 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > @@ -1082,15 +1082,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > > > void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) > > { > > - struct msm_gpu *gpu = &adreno_gpu->base; > > - struct msm_drm_private *priv = gpu->dev ? gpu->dev->dev_private : NULL; > > unsigned int i; > > > > for (i = 0; i < ARRAY_SIZE(adreno_gpu->info->fw); i++) > > release_firmware(adreno_gpu->fw[i]); > > > > - if (priv && pm_runtime_enabled(&priv->gpu_pdev->dev)) > > - pm_runtime_disable(&priv->gpu_pdev->dev); > > - > > Maybe WARN_ON(priv && pm_runtime_enabled(&priv->gpu_pdev->dev))? I'd rather not add warnings for something that can not happen, but it turns out there is indeed one corner case were this function could still end up being called with runtime PM enabled, namely if suspending the scheduler fails in adreno_system_suspend() during unbind: adreno_bind() info->init() // e.g. a6xx_gpu_init() adreno_gpu_init() msm_open() load_gpu() adreno_load_gpu() pm_runtime_enable() adreno_unbind() adreno_system_suspend() err = suspend_scheduler(gpu) if (!err) pm_runtime_force_suspend() pm_runtime_disable() gpu->funcs->destroy() // e.g. a6xx_destroy() adreno_gpu_cleanup() I assume we'd be in bigger troubles than just having an unbalanced disable count if that ever happens, but we should probably just keep the conditional disable in adreno_gpu_cleanup() in place for now. > > msm_gpu_cleanup(&adreno_gpu->base); > > } > > -- > > 2.39.2 I've found another related runtime PM issue so I'll send a v2 anyway. Johan
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index ce6b76c45b6f..1101b8234b49 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -1082,15 +1082,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) { - struct msm_gpu *gpu = &adreno_gpu->base; - struct msm_drm_private *priv = gpu->dev ? gpu->dev->dev_private : NULL; unsigned int i; for (i = 0; i < ARRAY_SIZE(adreno_gpu->info->fw); i++) release_firmware(adreno_gpu->fw[i]); - if (priv && pm_runtime_enabled(&priv->gpu_pdev->dev)) - pm_runtime_disable(&priv->gpu_pdev->dev); - msm_gpu_cleanup(&adreno_gpu->base); }
Since commit 4b18299b3365 ("drm/msm/adreno: Defer enabling runpm until hw_init()") runtime PM is no longer enabled at adreno_gpu_init(), which means that there are no longer any bind() error paths for which adreno_gpu_cleanup() is called with runtime PM enabled. As the runtime PM enable on first open() is balanced by the pm_runtime_force_suspend() call at unbind(), adreno_gpu_cleanup() is now always called with runtime PM disabled so that its pm_runtime_disable() call can be removed. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 ----- 1 file changed, 5 deletions(-)