On Tue, 12 May 2026 14:55:30 -0700 Chia-I Wu <[email protected]> wrote:
> On Tue, May 12, 2026 at 4:54 AM Boris Brezillon > <[email protected]> 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 <[email protected]> > WIth minor comments below, Reviewed-by: Chia-I Wu <[email protected]> > > --- > > 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).
