Hi all, Thanks for the review. All three of you are right, and v1 was wrong.
On Sun Nov 1, 2026 at 12:34 AM CST, Gyeyoung Baek wrote: > Hi Baihui, Troy, > > > On Fri, Oct 30, 2026 at 10:55 AM Troy Mitchell <[email protected]> wrote: > > > > Hi Baihui, > > > > On Thu Oct 29, 2026 at 4:38 PM +0800, Baihui Liang wrote: > > > Returning -EBUSY from ->runtime_idle() prevents the PM core from > > > retrying the idle check. Schedule an autosuspend so the idle state > > > can be re-evaluated once the firmware reaches ROGUE_FWIF_POW_IDLE. > > > > > > Signed-off-by: Baihui Liang <[email protected]> > > > --- > > > Tested on SpacemiT K3 (PowerVR Rogue BXM-4-64) by running vulkaninfo > > > repeatedly. Without this change the device intermittently stayed > > > runtime-active after the last user went away, because the firmware had > > > not yet reached ROGUE_FWIF_POW_IDLE when the idle check ran. With the > > > change the device reliably suspends. > > > --- > > > drivers/gpu/drm/imagination/pvr_power.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/imagination/pvr_power.c > > > b/drivers/gpu/drm/imagination/pvr_power.c > > > index eb4b6ecdf4f..6f38249ffcb 100644 > > > --- a/drivers/gpu/drm/imagination/pvr_power.c > > > +++ b/drivers/gpu/drm/imagination/pvr_power.c > > > @@ -434,7 +434,16 @@ pvr_power_device_idle(struct 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; > > > + if (pvr_power_is_idle(pvr_dev)) > > > + return 0; > > > + > > > + /* > > > + * Firmware not yet idle; reject but schedule autosuspend so > > > + * runtime_idle() can be retried. > > > + */ > > > + pm_runtime_autosuspend(dev); > > > > If I read the PM core correctly, when this autosuspend timer fires it > > calls ->runtime_suspend() directly rather than retrying > > ->runtime_idle(), so this looks equivalent to returning 0. That's right. It merely delays unconditional FW teardown without re-checking idle state. I'll send v2 with a different approach. > > Could the > > real issue be the pm_runtime_put() in pvr_watchdog_worker(), which > > perhaps should be pm_runtime_put_autosuspend()? That put is only reached when pm_runtime_get_if_in_use() actually returned a reference. Our stuck-active case was the opposite case: the device was active with a zero usage count, which jumps directly to out_requeue and skips pm_runtime_put entirely. Changing this call won't help. > I think sashiko's point is valid. > > Calling pm_runtime_autosuspend() from pvr_power_device_idle() when the > FW is busy only adds a delay, and the device is then suspended without > re-checking idle, and this is effectively the same as > returning 0. I guess it worked in testing because the FW was busy at > pvr_power_device_idle() time but reached idle before the autosuspend > delay expired. > > How about moving the pvr_power_is_idle() check into > pvr_power_device_suspend() (and dropping pvr_power_device_idle()), and > returning -EBUSY after pm_runtime_mark_last_busy() if the FW is busy? > The PM core then reschedules the autosuspend regardless of whether the > last put comes from the watchdog or elsewhere, so suspend is retried > until the FW becomes idle. Agreed, v2 will follow this approach. > I think this is the pattern described in > Documentation/power/runtime_pm.rst, section 9 ("If the > ->runtime_suspend() callback returns -EAGAIN or -EBUSY ..."). > -- > Thanks, > Gyeyoung Thanks, Baihui
