[PATCH v2] drm/imagination: re-check firmware idle state before runtime suspend

[PATCH v2] drm/imagination: re-check firmware idle state before runtime suspend

[Baihui Liang]

pvr_power_device_idle() rejects runtime suspend while the firmware
hasn't reached ROGUE_FWIF_POW_IDLE. The PM core doesn't reschedule
idle notification after rejection, so no further pm_runtime_put()
arrives on an idle GPU and the device stays powered indefinitely.

Move the idle check into pvr_power_device_suspend() and return -EBUSY
after pm_runtime_mark_last_busy() so the PM core reschedules the
autosuspend timer and retries until the firmware is idle.

Signed-off-by: Baihui Liang <liangbaihui@linux.spacemit.com>
---
Changes in v2:
- Move the pvr_power_is_idle() check into pvr_power_device_suspend()
  and return -EBUSY after pm_runtime_mark_last_busy().
- Drop pvr_power_device_idle() and pass NULL in RUNTIME_PM_OPS().
- Link to v1: https://patch.msgid.link/20261029-img-pm-autosuspend-v1-1-26851840a34e@linux.spacemit.com
---
 drivers/gpu/drm/imagination/pvr_drv.c   |  2 +-
 drivers/gpu/drm/imagination/pvr_power.c | 22 ++++++++++++----------
 drivers/gpu/drm/imagination/pvr_power.h |  1 -
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 5c965ef0274..541351d4bb6 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1516,7 +1516,7 @@ static const struct of_device_id dt_match[] = {
 MODULE_DEVICE_TABLE(of, dt_match);
 
 static const struct dev_pm_ops pvr_pm_ops = {
-	RUNTIME_PM_OPS(pvr_power_device_suspend, pvr_power_device_resume, pvr_power_device_idle)
+	RUNTIME_PM_OPS(pvr_power_device_suspend, pvr_power_device_resume, NULL)
 };
 
 static struct platform_driver pvr_driver = {
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index eb4b6ecdf4f..71267f714c7 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -378,6 +378,18 @@ pvr_power_device_suspend(struct device *dev)
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
+	if (READ_ONCE(pvr_dev->fw_dev.initialised) && !pvr_power_is_idle(pvr_dev)) {
+		/*
+		 * The firmware has not drained the GPU yet. Refresh last_busy so
+		 * the PM core reschedules the autosuspend and retries this callback
+		 * until the firmware becomes idle.
+		 */
+		pm_runtime_mark_last_busy(dev);
+		drm_dev_exit(idx);
+
+		return -EBUSY;
+	}
+
 	if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
 		err = pvr_power_fw_disable(pvr_dev, false, true);
 		if (err)
@@ -427,16 +439,6 @@ pvr_power_device_resume(struct device *dev)
 	return err;
 }
 
-int
-pvr_power_device_idle(struct device *dev)
-{
-	struct platform_device *plat_dev = to_platform_device(dev);
-	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
-	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
-
-	return pvr_power_is_idle(pvr_dev) ? 0 : -EBUSY;
-}
-
 static int
 pvr_power_clear_error(struct pvr_device *pvr_dev)
 {
diff --git a/drivers/gpu/drm/imagination/pvr_power.h b/drivers/gpu/drm/imagination/pvr_power.h
index c34252bda07..7d8df45c7b1 100644
--- a/drivers/gpu/drm/imagination/pvr_power.h
+++ b/drivers/gpu/drm/imagination/pvr_power.h
@@ -18,7 +18,6 @@ bool pvr_power_is_idle(struct pvr_device *pvr_dev);
 
 int pvr_power_device_suspend(struct device *dev);
 int pvr_power_device_resume(struct device *dev);
-int pvr_power_device_idle(struct device *dev);
 
 int pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset);
 

---
base-commit: 067c5c24cef23d913b67eaaf71ae67e7169a6620
change-id: 20261029-img-pm-autosuspend-7772d83416f7

Best regards,
--  
Baihui Liang <liangbaihui@linux.spacemit.com>

Re: [PATCH v2] drm/imagination: re-check firmware idle state before runtime suspend

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/imagination: pvr_remove() fails to stop FW on non-idle
  suspend
- [Low] drm/imagination: missing Fixes trailer on runtime PM bug fix
--

commit 819e68458c31e07fc8a39ed92b923866cc2d2009
Author: Baihui Liang <liangbaihui@linux.spacemit.com>

> drm/imagination: re-check firmware idle state before runtime suspend
>
> pvr_power_device_idle() rejects runtime suspend while the firmware
> hasn't reached ROGUE_FWIF_POW_IDLE. The PM core doesn't reschedule
> idle notification after rejection, so no further pm_runtime_put()
> arrives on an idle GPU and the device stays powered indefinitely.
>
> Move the idle check into pvr_power_device_suspend() and return -EBUSY
> after pm_runtime_mark_last_busy() so the PM core reschedules the
> autosuspend timer and retries until the firmware is idle.
>
> Signed-off-by: Baihui Liang <liangbaihui@linux.spacemit.com>

[Severity: Low]
This fixes a bug in runtime PM idle handling.  Should this include:

  Fixes: 727538a4bbff ("drm/imagination: Implement power management")

[ ... ]

> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> index eb4b6ecdf4f4d..71267f714c736 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.c
> +++ b/drivers/gpu/drm/imagination/pvr_power.c
> @@ -378,6 +378,18 @@ pvr_power_device_suspend(struct device *dev)
>  	if (!drm_dev_enter(drm_dev, &idx))
>  		return -EIO;
>  
> +	if (READ_ONCE(pvr_dev->fw_dev.initialised) && !pvr_power_is_idle(pvr_dev)) {
> +		/*
> +		 * The firmware has not drained the GPU yet. Refresh last_busy so
> +		 * the PM core reschedules the autosuspend and retries this callback
> +		 * until the firmware becomes idle.
> +		 */
> +		pm_runtime_mark_last_busy(dev);
> +		drm_dev_exit(idx);
> +
> +		return -EBUSY;
> +	}

[Severity: High]
Can this leave the firmware processor running and the GPU powered on during
pvr_remove()?

Before this change, pvr_power_is_idle() was only checked in .runtime_idle,
so synchronous pm_runtime_suspend() in pvr_remove() bypassed that check and
called pvr_power_fw_disable() to force the firmware idle and stop it.

During driver unbind, __device_release_driver() calls pm_runtime_get_sync()
followed by pm_runtime_put_sync() right before pvr_remove():

drivers/gpu/drm/imagination/pvr_drv.c:pvr_remove() {
	...
	pm_runtime_suspend(drm_dev->dev);
	pvr_device_fini(pvr_dev);
	drm_dev_unplug(drm_dev);
	pvr_watchdog_fini(pvr_dev);
}

If the firmware has not yet transitioned to ROGUE_FWIF_POW_IDLE (for example,
right after pm_runtime_get_sync() resumed the GPU or after a KCCB command),
pvr_power_device_suspend() now returns -EBUSY.  Because pm_runtime_suspend()
does not pass RPM_AUTO, rpm_suspend() does not retry and returns -EBUSY to
pvr_remove(), which ignores the error and runs pvr_device_fini() ->
pvr_fw_fini().

That skips pvr_power_fw_disable() and pwr_ops->power_off(), so pvr_fw_fini()
unmaps and frees the firmware structures, CCBs, code/data GEM objects, and
page tables while the firmware processor is still running, and leaves the GPU
clocks and power enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-img-pm-autosuspend-v2-1-427689f6f2f9@linux.spacemit.com?part=1