Re: [PATCH v3 3/3] drm/panfrost: Synchronize and disable interrupts before powering off

[AngeloGioacchino Del Regno]

Il 01/12/23 13:34, Steven Price ha scritto:
> On 01/12/2023 11:14, Boris Brezillon wrote:
> > On Fri,  1 Dec 2023 11:40:27 +0100
> > AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > wrote:
> >
> > > To make sure that we don't unintentionally perform any unclocked and/or
> > > unpowered R/W operation on GPU registers, before turning off clocks and
> > > regulators we must make sure that no GPU, JOB or MMU ISR execution is
> > > pending: doing that required to add a mechanism to synchronize the
> >
> >                        ^ requires the addition of a mechanism...
> >
> > > interrupts on suspend.
> > >
> > > Add functions panfrost_{gpu,job,mmu}_suspend_irq() which will perform
> > > interrupts masking and ISR execution synchronization, and then call
> > > those in the panfrost_device_runtime_suspend() handler in the exact
> > > sequence of job (may require mmu!) -> mmu -> gpu.
> > >
> > > As a side note, JOB and MMU suspend_irq functions needed some special
> > > treatment: as their interrupt handlers will unmask interrupts, it was
> > > necessary to add a bitmap for `is_suspended` which is used to address
> >
> >              to add an `is_suspended` bitmap which is used...
> >
> > > the possible corner case of unintentional IRQ unmasking because of ISR
> > > execution after a call to synchronize_irq().
> >
> > Also fixes the case where the interrupt handler is called when the
> > device is suspended because the IRQ line is shared with another device.
> > No need to update the commit message for that though.
> >
> > > At resume, clear each is_suspended bit in the reset path of JOB/MMU
> > > to allow unmasking the interrupts.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > ---
>
> <snip>
>
> > >   static void panfrost_job_handle_err(struct panfrost_device *pfdev,
> > >   				    struct panfrost_job *job,
> > >   				    unsigned int js)
> > > @@ -792,9 +802,13 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
> > >   	struct panfrost_device *pfdev = data;
> > >   
> > >   	panfrost_job_handle_irqs(pfdev);
> > > -	job_write(pfdev, JOB_INT_MASK,
> > > -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> > > -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
> > > +
> > > +	/* Enable interrupts only if we're not about to get suspended */
> > > +	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
> > > +		job_write(pfdev, JOB_INT_MASK,
> > > +			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> > > +			  GENMASK(NUM_JOB_SLOTS - 1, 0));
> > > +
> >
> > Missing if (test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended)) in
> > panfrost_job_irq_handler(), to make sure you don't access the registers
> > if the GPU is suspended.
>
> I think generally these IRQ handler functions should simply check the
> is_suspended flag and early out if the flag is set. It's not the
> re-enabling of the interrupts specifically that we want to gate - it's
> any access to the hardware as in the shared-IRQ case the GPU might
> already have been powered down/unclocked.

Yes, in the thread handler we're still powered, because we are synchronizing
irqs - adding the test_bit in the hardirq handler will prevent scheduling the
irq_handler_thread.

What the test_bit() here does is to allow us to handle the last interrupt(s)
(synchronize_irqs() in the suspend function) before cutting off power, without
unwillingly re-enabling the job (or mmu in panfrost_mmu.c) interrupts.

Cheers,
Angelo