drm/msm/adreno: fix runtime PM imbalance at unbind

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

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

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 36f062c7582f..c5c4c93b3689 100644
@@ -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

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'.

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 3 ---
 1 file changed, 3 deletions(-)

index c5c4c93b3689..cd009d56d35d 100644
@@ -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

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.

---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 -----
 1 file changed, 5 deletions(-)

index ce6b76c45b6f..1101b8234b49 100644
@@ -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

Hi Rob,

Sorry about the late follow-up on this. Went down a bit of a DRM rabbit hole this week.

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.

I've found another related runtime PM issue so I'll send a v2 anyway.

Johan

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

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index cd009d56d35d..80947420ac19 100644
@@ -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