Re: [PATCH v2 06/11] drm/panthor: Prepare the scheduler logic for FW events in IRQ context

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

On Wed, 13 May 2026 10:47:28 -0700
Chia-I Wu <olvaffe@gmail.com> wrote:

> > >
> > > Multiple things happen in this commit. I try to identify things that
> > > can be separate commits. If this does not make sense, feel free to
> > > ignore.
> > >  
> > > >         /** @tiler_oom_work: Work used to process tiler OOM events happening on this group. */
> > > >         struct work_struct tiler_oom_work;
> > > >  
> >
> > [...]
> >  
> > > >  /**
> > > >   * panthor_sched_report_fw_events() - Report FW events to the scheduler.
> > > >   * @ptdev: Device.
> > > > @@ -1902,8 +1953,19 @@ void panthor_sched_report_fw_events(struct panthor_device *ptdev, u32 events)  
> > > This can be renamed to panthor_sched_handle_fw_events.  
> >
> > It's not quite handling events though. For most of them, it's really
> > just deferring the processing to work items, SYNC_UPDATE is the
> > exception.  
> panthor_sched_report_fw_events no longer just queues
> process_fw_events_work. It processes fw events immediately. If
> "handle" is not the right verb, perhaps we can go with "process".

I guess "demux" would be more accurate, but do we need to rename this
function in the first place? I mean, panthor_sched_report_fw_events()
doesn't imply that events are processed/handled, it just reflects the
fact FW events are reported to the scheduler. Up to the scheduler to
do what it wants with this piece of information (process some of them
immediately, defer the processing for others, etc).

> 
> 
> >  
> > >  
> > > >         if (!ptdev->scheduler)
> > > >                 return;
> > > >
> > > > -       atomic_or(events, &ptdev->scheduler->fw_events);
> > > > -       sched_queue_work(ptdev->scheduler, fw_events);
> > > > +       guard(spinlock_irqsave)(&ptdev->scheduler->events_lock);
> > > > +
> > > > +       if (events & JOB_INT_GLOBAL_IF) {
> > > > +               sched_process_global_irq_locked(ptdev);
> > > > +               events &= ~JOB_INT_GLOBAL_IF;
> > > > +       }
> > > > +
> > > > +       while (events) {
> > > > +               u32 csg_id = ffs(events) - 1;
> > > > +
> > > > +               sched_process_csg_irq_locked(ptdev, csg_id);
> > > > +               events &= ~BIT(csg_id);
> > > > +       }  
> > > This handles all fw events in the irq context. Are there concerns that
> > > it may take too long? I might be wrong, but it seems possible to
> > > handle only CSG_SYNC_UPDATE and defer the rest as before.  
> >
> > I started with just the SYNC_UPDATE processing done in the hard-irq
> > context, but after auditing the other stuff done in the handler, I
> > realized it's basically just deferring all actual processing to work
> > items. Yes, there's the overhead of demuxing the events from the
> > ack/req regs, but part of this is already done to get to SYNC_UPDATE
> > anyway, so at this point we're probably better off demuxing everything
> > and scheduling works for all kind of events.
> >
> > I also compared the perfs between the two approaches (though I didn't
> > do as much testing as I did with the new version, so I might have
> > missed something), and it didn't seem to matter at all, because the
> > interrupts we receive the most are SYNC_UPDATE and IDLE events, and
> > those are at the same level.  
> Looking at ftrace irq events, when there is one active csg,
> panthor-job takes 6us (median) / 17us (95%) / 27us (slowest).
> 
> I don't have a good sense if that's considered normal in hardirq. But
> if that is ever an issue, and if the majority of the time is spent in
> CSG_SYNC_UPDATE anyway, we can always revert the last patch to move
> processing to threaded handler.

Actually, the threaded -> hard transition (patch 9) is where the perf
gain is.