Re: [PATCH 3/4] drm/msm/adreno: drop redundant pm_runtime_disable()

[Johan Hovold <johan@kernel.org>]

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