drm/imagination: re-check firmware idle state before runtime suspend

PATCH New Baihui Liang 2 participants 2 messages Nov 11

Subsystems (5)

DRM drivers DRM misc RISC-V RISC-V SpacemiT SoC Support +1 more subsystems

lore

Share & tools

lore

Baihui Liang

author

2026-11-11 09:29 UTC #0 lore raw

PATCH v2 1/1 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 <[email protected]>

---
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/[email protected]

---
 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(-)

drivers/gpu/drm/imagination/pvr_drv.c +1 -1

index 5c965ef0274..541351d4bb6 100644
@@ -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 = {

drivers/gpu/drm/imagination/pvr_power.c +12 ~~-10~~

index eb4b6ecdf4f..71267f714c7 100644
@@ -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)
 {

drivers/gpu/drm/imagination/pvr_power.h +0 -1

index c34252bda07..7d8df45c7b1 100644
@@ -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,

Signature · 1 line

Baihui Liang <[email protected]>

[email protected]

AI review

2026-11-11 09:47 UTC #1 lore raw

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…

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 <[email protected]>

Quoted 7 lines

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.

Quoted 5 lines

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 <[email protected]>

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

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

[ ... ]

Quoted 9 lines

drivers/gpu/drm/imagination/pvr_power.c +1 -0

index eb4b6ecdf4f4d..71267f714c736 100644
@@ -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)) {

drivers/gpu/drm/imagination/pvr_power.c pvr_power_device_suspend(struct device *dev) · line 391

Quoted 10 lines

drivers/gpu/drm/imagination/pvr_power.c +10 -0

+		/*
+		 * 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.

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1