Re: [PATCH v9 06/16] drm/panfrost: Fix PM refcnt and autosuspend issues at device probe/remove

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

On Tue, 22 Sep 2026 20:51:48 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> On 14.09.2026 11:22, Boris Brezillon wrote:
> > On Sat, 12 Sep 2026 00:28:07 +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 outcome of both of the above meant 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.
> > > - Because MMU interrupts are only enabled when the device is reset,
> > >   the very first job targeting the tiler heap BO after device probe()
> > >   would always time out, since the driver's PM runtime resume callback
> > >   would not be invoked.
> > > 
> > > To fix the above:
> > > - Manually adjust the PM refcnt at device probe and removal time.
> > > - Ensure pm_runtime_dont_use_autosuspend is called in the wind-down path.
> > > - Call pm_runtime_put_autosuspend() when device is ready to accept jobs
> > > - Move pm_runtime_set_suspended() before panfrost_device_fini() so that
> > >   resource unwinding happens in the opposite order as initialisation.
> > > 
> > > 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 | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > index 55fc22e8d4d4..a3eff77add55 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > @@ -854,6 +854,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_get_noresume(pfdev->base.dev);
> > >  	pm_runtime_enable(pfdev->base.dev);
> > >  	pm_runtime_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
> > >  	pm_runtime_use_autosuspend(pfdev->base.dev);
> > > @@ -866,13 +867,16 @@ 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_dont_use_autosuspend(pfdev->base.dev);
> > >  	pm_runtime_disable(pfdev->base.dev);
> > > -	panfrost_device_fini(pfdev);
> > > +	pm_runtime_put_noidle(pfdev->base.dev);
> > >  	pm_runtime_set_suspended(pfdev->base.dev);
> > > +	panfrost_device_fini(pfdev);  
> > 
> > Not an issue per-se, because put_noidle() is a NOP, but I think it'd be
> > easier to reason about with this order:
> > 
> > 	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> >   	pm_runtime_disable(pfdev->base.dev);
> > 	panfrost_device_fini(pfdev);
> >   	pm_runtime_set_suspended(pfdev->base.dev);
> > 	pm_runtime_put_noidle(pfdev->base.dev);
> > 
> > This makes it clear that panfrost_device_fini() assumes the device is
> > resumed when it's called and suspended when it returns.  
> 
> Your suggestion feels more natural, which makes me wonder why I decided to move
> panfrost_device_fini() to the end of the block. 
> 
> IIRC we chatted about where to place pm_runtime_put_noidle() and pm_runtime_set_suspended()
> in panfrost_device_init() when one of the subsystem initialisations failed. My intuition
> had been to leave them until the very bottom of it, in imitation of what you hinted here,
> but then it'd go against the notion that winding down must be done in the opposite order
> as initialisation. And so because panfrost_device_init() happens before all the pm_runtime_*
> incantations, I thought panfrost_device_fini() should be placed at the end in the error
> and driver remove paths.
> 
> That said, I think a more intuitive init order would be one that imitates what happens in
> Panthor when runtime PM is enabled:
> 
> 	/* First thing done in pm_runtime_resume_and_get -> pm_runtime_get_active -> __pm_runtime_resume */
> 	pm_runtime_get_noresume(pfdev->base.dev);

Panthor is a bit different in that it doesn't use noresume/noidle() at
all. What it does instead is teach the resume/suspend hooks about the
uninitialized device use case, and it skips some operations when a
resume/suspend() is done on an uninitialized dev. Panfrost, on the other
hand, just assumes that clks are enabled manually before any of the
subsystem _init() helpers are called. TLDR; this
pm_runtime_get_noresume() can be moved after all the _init() without
any downsides, and it keeps all PM related-init in just one place,
which I find neat.

> 	
> 	/* Now come subsystem initialisations */
> 	panfrost_gpu_init(pfdev);
> 	panfrost_mmu_init(pfdev);
> 	panfrost_jm_init(pfdev);
> 	panfrost_perfcnt_init(pfdev);
> 	panfrost_gem_init(pfdev);
> 	
> 	/* __pm_runtime_resume -> rpm_resume first invokes the resume callback and then
> 	 * updates runtime status to RPM_ACTIVE. */
> 	pm_runtime_set_active(pfdev->base.dev);
> 	
> 	pm_runtime_enable(ptdev->base.dev);
>         pm_runtime_mark_last_busy(pfdev->base.dev);
> 	pm_runtime_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
> 	pm_runtime_use_autosuspend(pfdev->base.dev);
> 
> And then in the error and device remove path would be almost just like you suggest.
> 
>         pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> 	pm_runtime_disable(pfdev->base.dev);
> 	pm_runtime_put_noidle(pfdev->base.dev);
> 	panfrost_device_fini(pfdev);
> 	pm_runtime_set_suspended(pfdev->base.dev);
> 
> The only difference is that pm_runtime_put_noidle() happens before device_fini
> because that's the same order as in __pm_runtime_suspend().

I really think we should keep the init/fini order consisten, so
put_noidle() should be after the _fini() calls. But as I said above,
I'd probably have get_noresume() called after all the _init() functions
instead, since we can afford it in Panfrost, and it helps keeping all
the PM-related calls in just one block.

> 
> However, that means in the commit where I move all DRM initialisation into
> panfrost_device_init(), in order to keep things consistent, I should move
> pm_runtime_set_suspended() to the very end of the error path.

It probably doesn't matter much actually, because as soon as
pm_runtime_disable() is called, nothing can change the RPM state
anymore. So maybe scratch all of my comments, and we stick to what you
had. It's just a transition state anyway, so, as long as it doesn't
regress the probe/remove logic, it's probably fine.

> 
> > >  err_out0:
> > >  	return err;
> > >  }
> > > @@ -884,9 +888,11 @@ static void panfrost_remove(struct platform_device *pdev)
> > >  	drm_dev_unregister(&pfdev->base);
> > >  
> > >  	pm_runtime_get_sync(pfdev->base.dev);
> > > +	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> > > +	pm_runtime_put_noidle(pfdev->base.dev);
> > >  	pm_runtime_disable(pfdev->base.dev);  
> > 
> > Let's keep the order consistent with the probe path:
> > 
> > 	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> > 	pm_runtime_disable(pfdev->base.dev);
> > 	panfrost_device_fini(pfdev);
> > 	pm_runtime_set_suspended(pfdev->base.dev);
> > 	pm_runtime_put_noidle(pfdev->base.dev);
> > 
> > I also think this deserves comments to explain the noresume/noidle
> > dance (device_init/fini take care of clks internally, and when they
> > return the device is resumed/suspended, so all we have to is update the
> > state, and acquire a ref).  
> 
> I don't mind doing this, but I think the fact we're calling noresume/noidle
> functions already implies device power-up is being handled manually rather
> by having RPM invoke resume and suspend callbacks behind the scenes, so
> we've no choice other than adjusting PM user counter and status ourselves.

Fair enough.