Re: [PATCH v7 08/17] drm/panfrost: Split subsystem init/reset from interrupt enablement

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

On Wed, 2 Sep 2026 16:41:40 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> On 01.09.2026 15:08, Boris Brezillon wrote:
> > On Fri, 28 Aug 2026 21:56:48 +0100
> > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> >   
> > > Because MMU interrupts are only enabled when the device is reset, it
> > > happened that after DRM device registration, the very first job targeting
> > > the tiler heap BO would always time out. The reason is the reset sequence
> > > is only part of PM runtime resume, which is not called explicitly at driver
> > > probe time, and an actual reset work item manually triggered after a HW
> > > error.
> > > 
> > > I have attempted a somewhat drastic solution, which is completely
> > > decoupling GPU/MMU/JM subsystem initialisation and reset from interrupt
> > > enablement, so that we can handle IRQ toggling a bit more flexibly.
> > > 
> > > To this end:
> > > - Ensure every subsystem with its own IRQ has an 'enable interrupts'
> > > method, and that it doesn't enable them anywhere else.
> > > - Force IRQ masking at MMU reset time. Up until, now, panfrost_mmu_reset()
> > > was clearing the MMU IRQ suspension bit, but at no point that is set during
> > > the reset sequence.
> > > 
> > > Then manually enable all interrupts when the device is fully initialised at
> > > probe time, right before DRM device registration, or after the reset
> > > sequence is complete. Also disable all interrupts at device remove time,
> > > so that their IRQs can be sync'ed right before tearing the device down.
> > > 
> > > Fixes: 635430797d3f ("drm/panfrost: Rework runtime PM initialization")
> > > Fixes: 876b15d2c88d ("drm/panfrost: Fix module unload")
> > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_device.c | 40 ++++++++++++++++++++++--------
> > >  drivers/gpu/drm/panfrost/panfrost_device.h |  3 ++-
> > >  drivers/gpu/drm/panfrost/panfrost_gpu.c    | 19 ++++++++------
> > >  drivers/gpu/drm/panfrost/panfrost_gpu.h    |  2 ++
> > >  drivers/gpu/drm/panfrost/panfrost_job.c    |  7 +++---
> > >  drivers/gpu/drm/panfrost/panfrost_mmu.c    |  9 +++++--
> > >  drivers/gpu/drm/panfrost/panfrost_mmu.h    |  2 ++
> > >  7 files changed, 56 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > index 9e02fb5f73c8..99f7da2180f9 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > @@ -226,6 +226,27 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
> > >  	return err;
> > >  }
> > >  
> > > +void panfrost_device_enable_int(struct panfrost_device *pfdev)
> > > +{
> > > +	panfrost_gpu_enable_interrupts(pfdev);
> > > +	panfrost_mmu_enable_interrupts(pfdev);
> > > +	panfrost_jm_enable_interrupts(pfdev);
> > > +}
> > > +
> > > +static void panfrost_device_enable_hw(struct panfrost_device *pfdev)
> > > +{
> > > +	panfrost_device_enable_int(pfdev);
> > > +	panfrost_devfreq_resume(pfdev);
> > > +}
> > > +
> > > +static void panfrost_device_disable_hw(struct panfrost_device *pfdev)
> > > +{
> > > +	panfrost_devfreq_suspend(pfdev);
> > > +	panfrost_jm_suspend_irq(pfdev);
> > > +	panfrost_mmu_suspend_irq(pfdev);
> > > +	panfrost_gpu_suspend_irq(pfdev);  
> > 
> > Hm, I think I'd prefer if those suspend/resume_irq() were hidden in
> > some subcomponent panfrost_<subcomp>_suspend,resume() helpers. And
> > then we just have to resume/suspend component in the right order
> > instead of treating IRQs as a standalone object (enabling/disabling
> > only makes sense if the subcomponent handling those interrupts is
> > resumed/suspended).  
> 
> I thought it would only make sense to enable interupts for a given subsystem
> when all the other subsystems are also resumed or initialised. This was prompted
> by Sashiko warning of the possibility of spurious interrupts causing a handler
> to be run when one of the subsystems it touches on hasn't yet been initialised.

Well, in practice things tend to be well isolated, for instance, an
MMU IRQ should be processed entirely inside panfrost_mmu.c, with no
particular interaction with the other subsystems. So, if an MMU
interrupt fires before, say, the JM subsystem is up and running, that
shouldn't be a problem. In panthor, we have a few cases where events
get propagated between subsystems, and for those we have some
is_initialized checks. I'm not sure this applies to panfrost though.

The other advantage with this approach is that it's one step towards a
better subsystem isolation like we have in panthor, where subsystems
only see their internal state/data plus the general state exposed by
panthor_device, instead of having everything in panfrost_device, and
everyone having the ability to modify/check the state of other
subsystems. panfrost_device.c then just acts as a glue layer that knows
about the order things should be executed in, but doesn't have all the
internal details about subsystem initialization/teardown steps.