[PATCH 0/4] drm/msm/adreno: fix runtime PM imbalance at unbind

[PATCH 0/4] drm/msm/adreno: fix runtime PM imbalance at unbind

[Johan Hovold]

As reported by Bjorn, we can end up with an unbalanced runtime PM
disable count if unbind() is called before the drm device is opened
(e.g. if component bind fails due to the panel driver not having been
loaded yet).

As runtime PM must currently stay disabled until the firmware has been
loaded, fix this by making the runtime PM disable call at unbind()
conditional.

The rest of the series removes a bogus pm_runtime_set_active() call and
drops the redundant pm_runtime_disable() from adreno_gpu_cleanup().
Included is also a related indentation cleanup.

Johan


Johan Hovold (4):
  drm/msm/adreno: fix runtime PM imbalance at unbind
  drm/msm/adreno: drop bogus pm_runtime_set_active()
  drm/msm/adreno: drop redundant pm_runtime_disable()
  drm/msm/adreno: clean up component ops indentation

 drivers/gpu/drm/msm/adreno/adreno_device.c | 10 ++++------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  5 -----
 2 files changed, 4 insertions(+), 11 deletions(-)

-- 
2.39.2

[PATCH 1/4] drm/msm/adreno: fix runtime PM imbalance at unbind

[Johan Hovold]

A recent commit moved enabling of runtime PM from adreno_gpu_init() to
adreno_load_gpu() (called on first open()), which means that unbind()
may now be called with runtime PM disabled in case the device was never
opened in between.

Make sure to only forcibly suspend and disable runtime PM at unbind() in
case runtime PM has been enabled to prevent a disable count imbalance.

This specifically avoids leaving runtime PM disabled when the device
is later opened after a successful bind:

	msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* Couldn't power up the GPU: -13

Fixes: 4b18299b3365 ("drm/msm/adreno: Defer enabling runpm until hw_init()")
Reported-by: Bjorn Andersson <quic_bjorande@quicinc.com>
Link: https://lore.kernel.org/lkml/20230203181245.3523937-1-quic_bjorande@quicinc.com
Cc: stable@vger.kernel.org	# 6.0
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 36f062c7582f..c5c4c93b3689 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -558,7 +558,8 @@ static void adreno_unbind(struct device *dev, struct device *master,
 	struct msm_drm_private *priv = dev_get_drvdata(master);
 	struct msm_gpu *gpu = dev_to_gpu(dev);
 
-	WARN_ON_ONCE(adreno_system_suspend(dev));
+	if (pm_runtime_enabled(dev))
+		WARN_ON_ONCE(adreno_system_suspend(dev));
 	gpu->funcs->destroy(gpu);
 
 	priv->gpu_pdev = NULL;
-- 
2.39.2

[PATCH 2/4] drm/msm/adreno: drop bogus pm_runtime_set_active()

[Johan Hovold]

The runtime PM status can only be updated while runtime PM is disabled.

Drop the bogus pm_runtime_set_active() call that was made after enabling
runtime PM and which (incidentally but correctly) left the runtime PM
status set to 'suspended'.

Fixes: 2c087a336676 ("drm/msm/adreno: Load the firmware before bringing up the hardware")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index c5c4c93b3689..cd009d56d35d 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -438,9 +438,6 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
 	 */
 	pm_runtime_enable(&pdev->dev);
 
-	/* Make sure pm runtime is active and reset any previous errors */
-	pm_runtime_set_active(&pdev->dev);
-
 	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0) {
 		pm_runtime_put_sync(&pdev->dev);
-- 
2.39.2

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

[Johan Hovold]

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);
-
 	msm_gpu_cleanup(&adreno_gpu->base);
 }
-- 
2.39.2

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

[Rob Clark]

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
>

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

[Johan Hovold]

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

[PATCH 4/4] drm/msm/adreno: clean up component ops indentation

[Johan Hovold]

Clean up the component ops initialisers which were indented one level
too far.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index cd009d56d35d..80947420ac19 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -563,8 +563,8 @@ static void adreno_unbind(struct device *dev, struct device *master,
 }
 
 static const struct component_ops a3xx_ops = {
-		.bind   = adreno_bind,
-		.unbind = adreno_unbind,
+	.bind   = adreno_bind,
+	.unbind = adreno_unbind,
 };
 
 static void adreno_device_register_headless(void)
-- 
2.39.2