[PATCH v2] drm/imagination: suspend the GPU for system sleep, not just runtime PM

[PATCH v2] drm/imagination: suspend the GPU for system sleep, not just runtime PM

[Ryan Brue]

pvr_pm_ops has only RUNTIME_PM_OPS, so the GPU gets no callback for system
suspend at all. That is fine only while the GPU happens to be
runtime-suspended when the system suspends -- then its own runtime PM has
already disabled the firmware and powered the domain off. If it is
runtime-ACTIVE, genpd powers the domain down during the system suspend
without the driver ever being told, so on resume the driver still believes
its firmware is running. The first firmware operation then never completes:

  pvr_mmu_flush_exec -> timeout -> pvr_power_reset -> drm_dev_unplug

and pvr_power_reset() deadlocks, because drm_dev_unplug() waits in
synchronize_srcu() for every drm_dev_enter() section to exit while being
called from inside a DRM ioctl that holds one. The result is an unkillable
D-state process and a GPU that stays dead until reboot.

On mt8173 (amazon-suez) this is deterministic:

  echo on > /sys/devices/platform/soc/13000000.gpu/power/control
  echo mem > /sys/power/state          # wakes on an RTC alarm
  vulkaninfo --summary                 # hangs in D state, every time

and with an autosuspend delay of 50 ms it is also what makes the failure
look intermittent in normal use -- it depends purely on whether the GPU was
idle at the instant of suspend.

pvr_power_device_suspend() already disables the firmware and calls the
platform power_off, and pvr_power_device_resume() is its exact inverse, so
force them for system sleep.

The force helpers cannot be used directly, though. Once the GPU has been
lost -- pvr_device_lost() sets pvr_dev->lost and calls drm_dev_unplug() --
drm_dev_enter() fails, so pvr_power_device_suspend() returns -EIO, and
pm_runtime_force_suspend() propagates that to the PM core, which aborts the
system suspend. Nothing ever clears the condition: a lost device cannot
runtime-suspend either, for the same reason, so it stays runtime-active and
every later attempt fails the same way. Wiring the force helpers in raw
would therefore trade a GPU that dies on suspend for a machine that cannot
suspend at all. Wrap them instead and skip the transition for a device
that is already gone, which is also the truthful answer: there is nothing
left to power down.

The wrappers test the same condition the -EIO comes from rather than
pvr_dev->lost, so they also cover an unplug from any other path.

The vendor driver arrives at the same rule about system sleep by a
different route: it holds a wakelock while the MFG domain is powered
("prevent suspend when gpu active") so the system can never suspend with
the GPU on.

Two related problems are left alone here. The reset-path deadlock above
(drm_dev_unplug() waiting on an SRCU section held by the ioctl that called
pvr_power_reset()) is a separate bug; this change only stops the
system-sleep path from reaching it. And pvr_power_fw_disable() cancels the
watchdog work before it can fail, and does not restart it on the error
path, so a suspend that fails part-way leaves the watchdog dead until the
next successful resume -- pre-existing, but system sleep is a new way to
reach it.

Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
Changes in v2:
- Don't wire pm_runtime_force_suspend/resume in directly. Once the GPU
  has been lost, drm_dev_enter() fails, pvr_power_device_suspend() returns
  -EIO, and the force helper hands that to the PM core, which aborts the
  system suspend -- permanently, since a lost device never becomes
  suspendable again. v1 would have traded a GPU that dies on suspend for a
  machine that cannot suspend at all. v2 wraps the helpers and skips the
  transition when the device is already unplugged. Caught by the Sashiko
  AI review bot.
- Commit message: say why the wrappers exist, and note that
  pvr_power_fw_disable() leaves the watchdog cancelled on its error path
  -- pre-existing, but system sleep is a new way to reach it.
- Cc'd Chen-Yu Tsai, Icenowy Zheng and YoungJoon Lee, who are testing this
  same GPU in the MT8173 powervr thread [1] and are the people best placed
  to say whether this reproduces on a Chromebook. That thread doesn't
  mention suspend at all.
- Dropped sarah.walker@imgtec.com and donald.robson@imgtec.com, both of
  which bounced on v1. get_maintainer.pl offers them via blamed_fixes on
  the Fixes: commit, but neither has committed since 2025-04 and 2023-12
  respectively and neither has a newer address in kernel mail.
[1] https://lore.kernel.org/all/20260728091804.382753-1-wenst@chromium.org/

- Link to v1: https://patch.msgid.link/20260909-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v1-1-f45847bc951b@gmail.com
---
 drivers/gpu/drm/imagination/pvr_drv.c   |  1 +
 drivers/gpu/drm/imagination/pvr_power.c | 31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/imagination/pvr_power.h |  3 +++
 3 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 5c965ef0274f..09eb162353aa 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1517,6 +1517,7 @@ 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)
+	SYSTEM_SLEEP_PM_OPS(pvr_power_system_suspend, pvr_power_system_resume)
 };
 
 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 eb4b6ecdf4f4..55a47bc4e925 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -427,6 +427,37 @@ pvr_power_device_resume(struct device *dev)
 	return err;
 }
 
+int
+pvr_power_system_suspend(struct device *dev)
+{
+	struct platform_device *plat_dev = to_platform_device(dev);
+	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
+
+	/*
+	 * A device that has been lost cannot be talked to, and there is
+	 * nothing left to power down. pvr_power_device_suspend() answers
+	 * -EIO for it, which the PM core turns into a failed system suspend
+	 * for the whole machine -- and because nothing clears the condition,
+	 * it would fail that way on every subsequent attempt.
+	 */
+	if (drm_dev_is_unplugged(drm_dev))
+		return 0;
+
+	return pm_runtime_force_suspend(dev);
+}
+
+int
+pvr_power_system_resume(struct device *dev)
+{
+	struct platform_device *plat_dev = to_platform_device(dev);
+	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
+
+	if (drm_dev_is_unplugged(drm_dev))
+		return 0;
+
+	return pm_runtime_force_resume(dev);
+}
+
 int
 pvr_power_device_idle(struct device *dev)
 {
diff --git a/drivers/gpu/drm/imagination/pvr_power.h b/drivers/gpu/drm/imagination/pvr_power.h
index c34252bda078..39d18ee9a3bd 100644
--- a/drivers/gpu/drm/imagination/pvr_power.h
+++ b/drivers/gpu/drm/imagination/pvr_power.h
@@ -20,6 +20,9 @@ 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_system_suspend(struct device *dev);
+int pvr_power_system_resume(struct device *dev);
+
 int pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset);
 
 static __always_inline int

---
base-commit: df2908090cda368b01ff43709f51890076c56157
change-id: 20260909-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-bfb9dfd7007b

Best regards,
--  
Ryan Brue <ryanbrue.dev@gmail.com>

Re: [PATCH v2] drm/imagination: suspend the GPU for system sleep, not just runtime PM

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] System suspend introduces an AB-BA deadlock between the SRCU read lock and the watchdog synchronization.
- [High] TOCTOU race in `pvr_power_system_suspend()` allows a concurrent unplug to cause a spurious system suspend failure.

Pre-existing issues:
- [High] Error path in `pvr_power_fw_disable()` fails to requeue watchdog work, permanently breaking watchdog monitoring.
- [High] Device structures are destroyed before `drm_dev_unplug()`, creating a Use-After-Free for concurrent ioctls.
--

commit dc6377c63d9f22c52f5a09300ba23dc0ca1af753
Author: Ryan Brue <ryanbrue.dev@gmail.com>

    drm/imagination: suspend the GPU for system sleep, not just runtime PM

    pvr_pm_ops has only RUNTIME_PM_OPS, so the GPU gets no callback for
    system suspend at all. This patch adds SYSTEM_SLEEP_PM_OPS to map
    system suspend and resume to pm_runtime_force_suspend and
    pm_runtime_force_resume, wrapped to prevent system suspend failures if
    the GPU is already unplugged.

> ...
> And pvr_power_fw_disable() cancels the watchdog work before it can fail,
> and does not restart it on the error path, so a suspend that fails
> part-way leaves the watchdog dead until the next successful resume --
> pre-existing, but system sleep is a new way to reach it.
> ...

[Severity: High]
This isn't a bug introduced by this patch, but since system sleep now
provides a reachable path for this to happen, could this leave the GPU
permanently dead on subsequent hangs?

If pvr_power_fw_disable() cancels the work, and then bails out early
before requeuing it:

drivers/gpu/drm/imagination/pvr_power.c:pvr_power_fw_disable() {
    cancel_delayed_work_sync(&pvr_dev->watchdog.work);

    err = pvr_power_request_idle(pvr_dev);
    if (err)
        return err;
    ...
}

Will the watchdog remain permanently disabled without a subsequent
successful resume to restart it?

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c965ef0274ff..09eb162353aac 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1517,6 +1517,7 @@ 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)
> +	SYSTEM_SLEEP_PM_OPS(pvr_power_system_suspend, pvr_power_system_resume)
>  };
>  
>  static struct platform_driver pvr_driver = {
>  	.probe = pvr_probe,
>  	.remove = pvr_remove,

[Severity: High]
This is a pre-existing issue, but does pvr_remove() create a
use-after-free condition for concurrent ioctls?

Looking at drivers/gpu/drm/imagination/pvr_drv.c:pvr_remove():

    xa_destroy(&pvr_dev->job_ids);
    xa_destroy(&pvr_dev->free_list_ids);

    pm_runtime_suspend(drm_dev->dev);
    pvr_device_fini(pvr_dev);
    drm_dev_unplug(drm_dev);

Since drm_dev_unplug() is called after destroying the xarrays, could a
concurrent ioctl still successfully call drm_dev_enter() and access the
freed data structures? Should drm_dev_unplug() be called before tearing
down the internal data structures?

> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> index eb4b6ecdf4f4d..55a47bc4e925e 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.c
> +++ b/drivers/gpu/drm/imagination/pvr_power.c
[ ... ]
> @@ -427,6 +427,37 @@ pvr_power_device_resume(struct device *dev)
>  	return err;
>  }
>  
> +int
> +pvr_power_system_suspend(struct device *dev)
> +{
> +	struct platform_device *plat_dev = to_platform_device(dev);
> +	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> +
> +	/*
> +	 * A device that has been lost cannot be talked to, and there is
> +	 * nothing left to power down. pvr_power_device_suspend() answers
> +	 * -EIO for it, which the PM core turns into a failed system suspend
> +	 * for the whole machine -- and because nothing clears the condition,
> +	 * it would fail that way on every subsequent attempt.
> +	 */
> +	if (drm_dev_is_unplugged(drm_dev))
> +		return 0;
> +
> +	return pm_runtime_force_suspend(dev);

[Severity: High]
Does this create a TOCTOU race window?

Because drm_dev_is_unplugged() acquires and then drops the lock, and
pvr_dev->sched_wq is not allocated with WQ_FREEZABLE, can the watchdog
worker run concurrently and unplug the device between the check and
the call to pm_runtime_force_suspend(dev)?

If pm_runtime_force_suspend() is called on a device that was lost in that
window, wouldn't it result in a spurious system suspend failure?


[Severity: Critical]
Does this bypass runtime PM usage counts and introduce an AB-BA deadlock
during system suspend?

pm_runtime_force_suspend() calls pvr_power_device_suspend(), which
acquires the SRCU read lock via drm_dev_enter():

drivers/gpu/drm/imagination/pvr_power.c:pvr_power_device_suspend() {
    if (!drm_dev_enter(drm_dev, &idx))
        return -EIO;
    ...
    if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
        err = pvr_power_fw_disable(pvr_dev, true);

It then blocks waiting for the watchdog to complete while holding the lock:

drivers/gpu/drm/imagination/pvr_power.c:pvr_power_fw_disable() {
    cancel_delayed_work_sync(&pvr_dev->watchdog.work);

If the watchdog worker is concurrently executing and triggers a reset that
fails, it calls drm_dev_unplug():

drivers/gpu/drm/imagination/pvr_power.c:pvr_device_lost() {
    drm_dev_unplug(from_pvr_device(pvr_dev));

drm_dev_unplug() will block on synchronize_srcu() waiting for the read
lock held by the suspend path, causing a complete system freeze. Can we
avoid holding the SRCU read lock while synchronizing with the watchdog?

> +}
> +
> +int
> +pvr_power_system_resume(struct device *dev)
> +{
> +	struct platform_device *plat_dev = to_platform_device(dev);
> +	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> +
> +	if (drm_dev_is_unplugged(drm_dev))
> +		return 0;
> +
> +	return pm_runtime_force_resume(dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v2-1-66a938e92b87@gmail.com?part=1