Re: [PATCH] drm/imagination: retry runtime idle when firmware is not yet idle

["Baihui Liang" <liangbaihui@linux.spacemit.com>]

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 <troy@kernel.org> 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 <liangbaihui@linux.spacemit.com>
> > > ---
> > > 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