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

[Ryan Brue <ryanbrue.dev@gmail.com>]

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