Re: [PATCH v2 08/11] drm/panthor: Automatically enable interrupts in panthor_fw_wait_acks()

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

On Tue, 12 May 2026 14:55:30 -0700
Chia-I Wu <olvaffe@gmail.com> wrote:

> On Tue, May 12, 2026 at 4:54 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > Rather than assuming an interrupt is always expected for request
> > acks, temporarily enable the relevant interrupts when the polling-wait
> > failed. This should hopefully reduce the number of interrupts the CPU
> > has to process.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> WIth minor comments below, Reviewed-by: Chia-I Wu <olvaffe@gmail.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_fw.c    | 34 +++++++++++++++++++--------------
> >  drivers/gpu/drm/panthor/panthor_sched.c |  5 +++--
> >  2 files changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index 8239a6951569..f5e0ceca4130 100644
> > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > @@ -1039,16 +1039,10 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
> >         glb_iface->input->progress_timer = PROGRESS_TIMEOUT_CYCLES >> PROGRESS_TIMEOUT_SCALE_SHIFT;
> >         glb_iface->input->idle_timer = panthor_fw_conv_timeout(ptdev, IDLE_HYSTERESIS_US);
> >
> > -       /* Enable interrupts we care about. */
> > -       glb_iface->input->ack_irq_mask = GLB_CFG_ALLOC_EN |
> > -                                        GLB_PING |
> > -                                        GLB_CFG_PROGRESS_TIMER |
> > -                                        GLB_CFG_POWEROFF_TIMER |
> > -                                        GLB_IDLE_EN |
> > -                                        GLB_IDLE;
> > -
> > -       if (panthor_fw_has_glb_state(ptdev))
> > -               glb_iface->input->ack_irq_mask |= GLB_STATE_MASK;
> > +       /* Enable interrupts for asynchronous events that are not
> > +        * triggered by request acks.
> > +        */
> > +       glb_iface->input->ack_irq_mask = GLB_IDLE;  
> We should static_assert or & with GLB_EVT_MASK. Same for CSG and CS.

Yep, good idea, I'll add a static_assert() in all places where
->ack_irq_mask is set.

> 
> >
> >         panthor_fw_update_reqs(glb_iface, req, GLB_IDLE_EN | GLB_COUNTER_EN,
> >                                GLB_IDLE_EN | GLB_COUNTER_EN);
> > @@ -1318,8 +1312,8 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> >   * Return: 0 on success, -ETIMEDOUT otherwise.
> >   */
> >  static int panthor_fw_wait_acks(const u32 *req_ptr, const u32 *ack_ptr,
> > -                               wait_queue_head_t *wq,
> > -                               u32 req_mask, u32 *acked,
> > +                               u32 *ack_irq_mask_ptr, spinlock_t *lock,
> > +                               wait_queue_head_t *wq, u32 req_mask, u32 *acked,
> >                                 u32 timeout_ms)
> >  {
> >         u32 ack, req = READ_ONCE(*req_ptr) & req_mask;
> > @@ -1334,8 +1328,16 @@ static int panthor_fw_wait_acks(const u32 *req_ptr, const u32 *ack_ptr,
> >         if (!ret)
> >                 return 0;
> >
> > -       if (wait_event_timeout(*wq, (READ_ONCE(*ack_ptr) & req_mask) == req,
> > -                              msecs_to_jiffies(timeout_ms)))
> > +       scoped_guard(spinlock_irqsave, lock)
> > +               *ack_irq_mask_ptr |= req_mask;
> > +
> > +       ret = wait_event_timeout(*wq, (READ_ONCE(*ack_ptr) & req_mask) == req,
> > +                                msecs_to_jiffies(timeout_ms));
> > +
> > +       scoped_guard(spinlock_irqsave, lock)
> > +               *ack_irq_mask_ptr &= ~req_mask;  
> We should add a comment saying that this is safe because
> {GLB,CSG,CS}_REQ_MASK and {GLB,CSG,CS}_EVT_MASK are disjoint, and thus
> req_mask and ack_irq_mask are disjoint.

You mean the ack_irq_mask set at init time? Because
xxx_iface->input->ack_irq_mask is moving target now.

Well, if we expand on safety matters, I'd say none of this is safe
since it relies on the caller knowing what it does and passing a valid
req_mask. But I'll add a comment mentioning that the original
ack_irq_mask shouldn't intersect with any of the bits that might be set
in req_mask (that's basically the static_assert() you suggested).