Re: [PATCH v2 6/7] drm/panfrost: Fix PM usage_count mishandling

[Boris Brezillon <boris.brezillon@collabora.com>]

On Tue, 16 Jun 2026 21:17:53 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> On 04.06.2026 20:36, Boris Brezillon wrote:
> > On Thu, 04 Jun 2026 18:35:25 +0100
> > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> >   
> > > During device probe(), failure to do a PM get() will leave the usage_count
> > > set to 0, which is the value assigned at device creation time. That means
> > > when the autosuspend delay expires, runtime suspend callback won't be
> > > invoked, so the device will remain powered on forever.
> > > 
> > > On top of that, failure to call PM put() during device unplug means
> > > Panfrost device's PM usage_count increases monotonically for every new
> > > module reload.
> > > 
> > > The combined outcome of both of the above was that devfreq OPP transition
> > > notifications would be printed all the time, even when no jobs are being
> > > submitted. This quickly fills the kernel ring buffer with junk.
> > > 
> > > Even direr than that was the fact MMU interrupts are only enabled when
> > > the device is reset, so after device probe() the very first job targeting
> > > the tiler heap BO would always time out, because the driver's PM runtime
> > > resume callback would not be invoked.
> > > 
> > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > Fixes: 635430797d3f ("drm/panfrost: Rework runtime PM initialization")
> > > Fixes: 876b15d2c88d ("drm/panfrost: Fix module unload")
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > index 2d4b6aa95c66..545fbf2c8d0c 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > @@ -989,6 +989,7 @@ static int panfrost_probe(struct platform_device *pdev)
> > >  	pm_runtime_set_active(pfdev->base.dev);
> > >  	pm_runtime_mark_last_busy(pfdev->base.dev);
> > >  	pm_runtime_enable(pfdev->base.dev);
> > > +	pm_runtime_get_noresume(pfdev->base.dev);
> > >  	pm_runtime_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
> > >  	pm_runtime_use_autosuspend(pfdev->base.dev);
> > >  
> > > @@ -1000,10 +1001,12 @@ static int panfrost_probe(struct platform_device *pdev)
> > >  	if (err < 0)
> > >  		goto err_out1;
> > >  
> > > +	pm_runtime_put_autosuspend(pfdev->base.dev);
> > >  
> > >  	return 0;
> > >  
> > >  err_out1:
> > > +	pm_runtime_put_noidle(pfdev->base.dev);  
> > 
> > Do we really need this get_noresume/put_noidle dance, can't use call
> > pm_runtime_dont_use_autosuspend() instead like is done in panthor, or
> > is panthor broken too?  
> 
> We need get_noresume() because after panfrost_device_init(), the device is powered
> up but that is not reflected in the device's PM refcnt. Then pm_runtime_put_autosuspend()
> will decrement the refcnt back to 0 and let the autosuspend window expire before
> suspending the device, unless someone starts using it immediately.

That's more or less what I'm questioning. Can't we make it so
panfrost_clk_init() doesn't call clk_prepare_enable() on its clks, and
then have a pm_runtime_resume_and_get() in panfrost_device_init() just
before we start interacting with the HW components (so basically before
panfrost_gpu_init()). Then you don't need this
get_noresume/put_noidle() dance, because the init path is just
considered like any other path that needs to interact with the HW.

> 
> pm_runtime_dont_use_autosuspend() is not mandatory if we manually call pm_runtime_disable()
> at device unplug time or when probe() fails. pm_runtime_use_autosuspend()'s docs say:
> 
> pm_runtime_use_autosuspend - Allow autosuspend to be used for a device.
> @dev: Target device.
> Allow the runtime PM autosuspend mechanism to be used for @dev whenever
> requested (or "autosuspend" will be handled as direct runtime-suspend for
> it).
> NOTE: It's important to undo this with pm_runtime_dont_use_autosuspend()
> at driver exit time unless your driver initially enabled pm_runtime
> with devm_pm_runtime_enable() (which handles it for you).
> 
> Panthor uses devm_pm_runtime_enable(), which makes me suspect perhaps it doesn't need
> to explicitly call pm_runtime_dont_use_autosuspend().

Yep, devm_pm_runtime_enable() takes care of that, indeed.

> 
> Panthor also manages PM refcnt fine at driver probe(), inside panthor_device_init():
> 
> ``` c
> ret = pm_runtime_resume_and_get(ptdev->base.dev);
> if (ret)
> 	return ret;
> 
> /* If PM is disabled, we need to call panthor_device_resume() manually. */
> if (!IS_ENABLED(CONFIG_PM)) {
> 	ret = panthor_device_resume(ptdev->base.dev);
> 	if (ret)
> 		return ret;
> }
> ```
> 
> If we want a similar thing in Panfrost, then we should move all clock and devfreq enablement
> into panfrost_device_runtime_resume() and do pm_runtime_resume_and_get() right before
> panfrost_gpu_init().

This ^. You probably want to keep clk_prepare() in the clk_init() path,
since resume/suspend() only enable/disable the clks.