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

[PATCH] 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 vendor driver arrives at the same rule 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.

The reset-path deadlock itself (drm_dev_unplug() waiting on an SRCU section
held by the ioctl that called pvr_power_reset()) is a separate problem and
is not addressed here; this change only stops the system-sleep path from
triggering it.

Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 5c965ef0274f..23d353141589 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(pm_runtime_force_suspend, pm_runtime_force_resume)
 };
 
 static struct platform_driver pvr_driver = {

---
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] drm/imagination: suspend the GPU for system sleep, not just runtime PM

[sashiko-bot]

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

New issues:
- [High] Delegating system suspend to pm_runtime_force_suspend causes system sleep to abort if the device is lost/unplugged.

Pre-existing issues:
- [Critical] Unkillable D-state deadlock when drm_dev_unplug() is called from within a drm_dev_enter() critical section.
- [High] Disabling a shared IRQ line during runtime suspend breaks other devices on the same interrupt line.
- [High] Hardware registers are accessed in a shared IRQ handler without verifying the runtime PM state, causing spurious wakeups and infinite loops.
- [High] Failing to suspend leaves the GPU watchdog permanently disabled.
- [High] drm_dev_unplug() is called after device teardown, creating a Use-After-Free race window with userspace ioctls.
- [High] The watchdog worker is cancelled after device teardown, allowing it to access freed firmware structures.
--

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

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

This commit addresses a system suspend bug where the GPU is not informed 
of the suspend if it is runtime-active. By delegating system suspend to 
the runtime PM force helpers, the GPU firmware is properly disabled 
during system sleep.

>    The reset-path deadlock itself (drm_dev_unplug() waiting on an SRCU section
>    held by the ioctl that called pvr_power_reset()) is a separate problem and
>    is not addressed here; this change only stops the system-sleep path from
>    triggering it.

[Severity: Critical]
This is a pre-existing issue, but the commit message acknowledges a deadlock
where drm_dev_unplug() blocks synchronously waiting for the SRCU lock held by
the caller, leaving the process in an unkillable D-state.

Can this still be triggered by local userspace submitting a job that hangs the
firmware or by an organic firmware hang?

If pvr_mmu_flush_exec() takes an SRCU read lock via drm_dev_enter(), and a
firmware lockup causes a flush timeout:

drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_flush_exec()
	err = pvr_power_reset(pvr_dev, true);

drivers/gpu/drm/imagination/pvr_power.c:pvr_power_reset()
err_device_lost:
	drm_err(from_pvr_device(pvr_dev), "GPU device lost");
	pvr_device_lost(pvr_dev);

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

Will drm_dev_unplug() cause a deadlock by calling synchronize_srcu() to wait
for all drm_dev_enter() readers to exit, while the thread is waiting on itself?

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c965ef0274ff..23d353141589f 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(pm_runtime_force_suspend, pm_runtime_force_resume)

[Severity: High]
Does delegating system suspend to pm_runtime_force_suspend() cause the system
sleep to abort if the device is lost or unplugged?

If the GPU crashes and is marked lost via drm_dev_unplug(), the PM core calls
the .suspend callback, which delegates to pvr_power_device_suspend():

drivers/gpu/drm/imagination/pvr_power.c:pvr_power_device_suspend()
	if (!drm_dev_enter(drm_dev, &idx))
		return -EIO;

Since the device is unplugged, drm_dev_enter() fails, and the function returns
-EIO. Will this permanently break system suspend for the entire system until
reboot, since the PM core aborts system suspend if any device's .suspend
callback returns an error?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does disabling a shared IRQ line
during runtime suspend break other devices on the same interrupt line?

The driver registers its interrupt with IRQF_SHARED:

drivers/gpu/drm/imagination/pvr_device.c:pvr_device_irq_init()
				    IRQF_SHARED | IRQF_ONESHOT, "gpu", pvr_dev);

When the GPU enters runtime suspend organically:

drivers/gpu/drm/imagination/pvr_power.c:pvr_power_fw_disable()
		/* This also waits for late processing of GPU or firmware IRQs in other cores */
		disable_irq(pvr_dev->irq);

Will disabling a shared IRQ at the controller level silently stop other devices
on the same line from receiving interrupts, potentially breaking their
functionality or hanging the system? Should this use synchronize_irq() instead,
as mandated by the PM guidelines?

[Severity: High]
This is a pre-existing issue, but are hardware registers accessed in a shared
IRQ handler without verifying the runtime PM state, potentially causing spurious
wakeups and infinite loops?

If another device on the shared interrupt line triggers an IRQ while the GPU is
in D3cold (powered off), the handler reads hardware registers:

drivers/gpu/drm/imagination/pvr_device.c:pvr_device_irq_handler()
	if (!pvr_fw_irq_pending(pvr_dev) && !safety_irq_pending)
		return IRQ_NONE; /* Spurious IRQ - ignore. */

	return IRQ_WAKE_THREAD;

drivers/gpu/drm/imagination/pvr_fw_meta.c:pvr_meta_irq_pending()
	return pvr_cr_read32(pvr_dev, ROGUE_CR_META_SP_MSLVIRQSTATUS) &
	       ROGUE_CR_META_SP_MSLVIRQSTATUS_TRIGVECT2_EN;

On powered-off hardware, reads typically return 0xffffffff. Since this matches
the _EN bitmasks, will it falsely evaluate to true and return IRQ_WAKE_THREAD,
leaving the threaded IRQ handler stuck in an infinite loop attempting to clear
non-existent interrupts? Should this check pm_runtime_get_if_active() before
accessing registers in shared handlers?

[Severity: High]
This isn't a bug introduced by this patch, but does failing to suspend leave
the GPU watchdog permanently disabled?

When the driver attempts runtime suspend:

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;

If the firmware idle request times out, it returns the error immediately. Does
this leave the watchdog disabled without restarting it in the error path,
meaning a firmware hang will never be detected or recovered?

[Severity: High]
This is a pre-existing issue, but is drm_dev_unplug() called after device
teardown, creating a use-after-free race window with userspace ioctls?

drivers/gpu/drm/imagination/pvr_drv.c:pvr_remove()
	pvr_device_fini(pvr_dev);
	drm_dev_unplug(drm_dev);

If the device is unbound (e.g., via sysfs), pvr_device_fini() tears down
hardware state and frees structures. Because drm_dev_unplug() is only called
after teardown, drm_dev_enter() will still succeed inside concurrent ioctls.
Can this allow userspace ioctls to operate on freed firmware structures and
VM contexts?

[Severity: High]
This isn't a bug introduced by this patch, but is the async watchdog worker
cancelled after device teardown, allowing it to access freed firmware
structures?

drivers/gpu/drm/imagination/pvr_drv.c:pvr_remove()
	pvr_device_fini(pvr_dev);
	drm_dev_unplug(drm_dev);
	pvr_watchdog_fini(pvr_dev);

Because drm_dev_unplug() is ordered incorrectly, concurrent ioctls can cause
pm_runtime_suspend() to return -EAGAIN, leaving the device active.
pvr_device_fini() then frees firmware structures.

Since the watchdog delayed work is only cancelled later via
pvr_watchdog_fini(), if it fires in the meantime, can it access freed memory?

drivers/gpu/drm/imagination/pvr_power.c:pvr_watchdog_kccb_stalled()
	u32 kccb_cmds_executed = pvr_dev->fw_dev.fwif_osdata->kccb_cmds_executed;

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

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

[Ryan Brue]

On 9/9/26 2:26 PM, sashiko-bot@kernel.org wrote:

> [Severity: High]
> Does delegating system suspend to pm_runtime_force_suspend() cause the system
> sleep to abort if the device is lost or unplugged?
>
> If the GPU crashes and is marked lost via drm_dev_unplug(), the PM core calls
> the .suspend callback, which delegates to pvr_power_device_suspend():
>
> drivers/gpu/drm/imagination/pvr_power.c:pvr_power_device_suspend()
> 	if (!drm_dev_enter(drm_dev, &idx))
> 		return -EIO;
>
> Since the device is unplugged, drm_dev_enter() fails, and the function returns
> -EIO. Will this permanently break system suspend for the entire system until
> reboot, since the PM core aborts system suspend if any device's .suspend
> callback returns an error?
>
> [ ... ]
Yes, this is a real regression in v1. Thanks for catching it. The chain is:

   pvr_device_lost() sets pvr_dev->lost and calls drm_dev_unplug(), which
   sets dev->unplugged (drm_drv.c);
   drm_dev_enter() then returns false;
   pvr_power_device_suspend() returns -EIO (pvr_power.c);
   pm_runtime_force_suspend() propagates it -- "if (ret) goto err;", and
   the error path is pm_runtime_enable(dev); return ret; (runtime.c);
   the PM core aborts the system suspend.

pm_runtime_force_suspend() only reaches the callback if the device is 
runtime-active, and a lost device cannot runtime-suspend either, for the 
same -EIO. As a result, it stays active and every subsequent system 
suspend fails the same way. All that can fix it at that point is a 
reboot or a rebind.

v2 wraps the force helpers and returns 0 early when the device is 
already unplugged, because at that point there's nothing left to power 
down. I test drm_dev_is_unplugged() rather than pvr_dev->lost so the 
guard matches the exact condition the -EIO comes from, therefore also 
covering an unplug arriving by any other path.

I verified v2 on my Amazon Fire HD 10 (2017) tablet, with no patch as 
the control. The same kernel, same config, same tree, with the only 
different being this patch applied and powervr.ko rebuilt and swapped (I 
have the module as =m, so no reflash was needed; vermagic identical). 
The control failed, and the v2 patch worked.

The test pins the GPU runtime-active -- the condition that fails -- then
system-suspends for 35 s and touches the GPU.

With the patch:

   runtime_status before suspend: active
   state write rc=0, uptime 606.04 -> 642.39
   runtime_status after resume: active
   vulkaninfo --summary: rc=0, deviceName = PowerVR Rogue GX6250

Without it, the same run, GPU working beforehand (rc=0, same deviceName) 
and the suspend itself succeeding (uptime 52.62 -> 87.82):

   VERDICT: FAIL -- GPU op still running after 100s
   pid 2862 state: D

and the kernel's hung-task detector produced two stacks, from two 
different ioctls, both wedged in the same place:

   pvr_power_reset+0x64/0x534 [powervr]
   pvr_mmu_flush_exec+0xec/0x18c [powervr]
   pvr_mmu_op_context_destroy+0x58/0x1ec [powervr]
   pvr_vm_bind_op_fini+0xa4/0xd0 [powervr]
   pvr_vm_unmap_obj_locked+0x1f4/0x260 [powervr]
   pvr_vm_unmap+0x5c/0x90 [powervr]
   pvr_ioctl_vm_unmap+0x50/0x80 [powervr]
   drm_ioctl_kernel+0xec/0x13c [drm]
   drm_ioctl+0x254/0x3c4 [drm]

   pvr_power_reset+0x64/0x534 [powervr]
   pvr_mmu_flush_exec+0xec/0x18c [powervr]
   pvr_submit_jobs+0x838/0xa60 [powervr]
   pvr_ioctl_submit_jobs+0x6c/0x19c [powervr]
   drm_ioctl_kernel+0xec/0x13c [drm]
   drm_ioctl+0x254/0x3c4 [drm]

That's the chain from the commit message, and it also answers your 
question about the deadlock: both stacks are under drm_ioctl, so 
pvr_power_reset() is calling drm_dev_unplug() from inside the very SRCU 
read section the ioctl holds. The box needed a reboot afterwards, and 
the reboot itself was slow because the D-state task cannot be killed.

One note on this: this is a check-then-act, and kernel workqueues are 
not frozen during dpm_suspend, so the watchdog worker could in principle 
lose the device between the test and the callback. The window is small 
and the consequence is one failed suspend rather than a permanent one, 
but it is not zero. The alternative is to stop 
pvr_power_device_suspend() reporting failure for hardware that is gone, 
which removes the race and is arguably more correct for the runtime path 
too -- but it changes existing behaviour, so I didn't fold it into this 
fix. I can do it that way instead if it's preferred, just let me know in 
v2.
> [Severity: High]
> This isn't a bug introduced by this patch, but does failing to suspend leave
> the GPU watchdog permanently disabled?
Confirmed, pvr_power_fw_disable() does 
cancel_delayed_work_sync(&watchdog.work) and then returns early if 
pvr_power_request_idle() fails, without requeueing. The only requeue is 
the queue_delayed_work() in pvr_power_fw_enable(), on the resume path, 
and the PM core doesn't call resume for a device whose suspend failed. 
So the watchdog stays cancelled and a firmware hang later on would go 
undetected.

It's pre-existing, but my patch adds a new way to reach it through 
system sleep, so I mentioned it in the v2 commit message. I can also 
send a separate patch restarting the watchdog on that error path, if 
that's wanted.

---

Everything else seems to be pre-existing and probably not worth 
addressing in my v2.

For the v2, I'm not including sarah.walker@imgtec.com and 
donald.robson@imgtec.com -- the sends to them bounced. b4 --auto-to-cc 
included them, but I'll just remove them for v2.

I'll also Cc Chen-Yu Tsai, Icenowy Zheng, and YoungJoon Lee, who are 
also working on the same GPU and are much more experienced with this 
hardware than I am :) I have an Amazon Fire HD 10 with the same GPU, but 
I believe they are working on Chromebooks.

Thanks again,
Ryan