Re: [RFC 2/8] drm/panthor: Use separate workqueue for DRM scheduler

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

On Mon, 6 Jul 2026 13:03:33 +0100
Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:

> On 02/07/2026 16:31, Boris Brezillon wrote:
> > On Thu,  2 Jul 2026 15:37:39 +0100
> > Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
> >   
> >> Currently an unordered workqueue is used for the DRM scheduler which means
> >> its concurrency is externally managed, and given there is one scheduler
> >> instance per userspace queue, that means workqueue management logic is
> >> within its rights to spawn many kernel threads to submit their respective
> >> jobs.
> >>
> >> Problem there is that all run job callbacks are serialized on the device
> >> global mutex,  
> > 
> > I think we should address that instead, and either shorten the scope of
> > the locked section, or make it so we don't make it a contention point
> > for concurrent job submission from different contexts (with a rwsem
> > instead of a lock, for instance).
> >   
> >> making the potential thread storm just causing lock
> >> contention.
> >>
> >> If we add a separate ordered workqueue for the DRM scheduler integration
> >> we can avoid this problem, since the ordered property directly expresses
> >> the nature of the submission backend implementation.  
> > 
> > Yep, except that's not how it was meant to work. The goal was to allow
> > contexts to submit their jobs concurrently to the FW. The only reason we
> > take the lock is to:
> > 
> > 1. make sure the context is still allowed to take jobs
> > 2. kick the group scheduler if the context is not resident
> > 
> > For #1, I believe we can come up with either a lockless solution, or a
> > solution where the lock protecting the state belongs to the group
> > instead of being externally protected by the device-wide scheduler lock.
> > 
> > For #2, the rwsem approach, and narrowing down the locked section to
> > just this part of the code should do the trick.  
> 
> Out of curiosity how much CPU side parallelism you think is required to 
> keep these GPUs fed? Both today (with the greater lock contention) and 
> in the future (with the reduced contention) I guess would be interesting 
> data points.

The maximum is known: it's the amount of FW CSG slot we have available.
I think the theoretical limit is 16, but IIRC, we never had more than 8
exposed by the FW.

Also, adjusting the priority of the thread used for job submission is
not enough (at least not for panthor), because the processing of FW
events is still going through this single-prio workqueue (signaling is
about to be moved to the threaded IRQ handler, but we still defer the
processing of some events to work items, and those can block progress
on a queue until they are processed). If we're processing events coming
from low prio queues before those coming from high prio queues, we're
back to the same problem. Not to mention that now, high prio submission
threads can completely starve FW event processing.

This is just my 2-cts, but I think we need both side of the equation
addressed at the same time, which is why I believe we should convert
panthor_scheduler::wq to the same kthread_worker mechanism.

> 
> >> And considering the other user of this workqueue, the free job callback,
> >> which is not globally serialized in this manner so could be thought to
> >> potentially regress with this change, it should not be the case since
> >> commit
> >> a58f317c1ca0 ("drm/sched: Free all finished jobs at once")
> >> made the DRM scheduler handle the cleanup of finished jobs more promptly.  
> > 
> > I don't know if this change is a hard dep for what's coming next, but
> > if it's not, I'd drop it. If it is, and the new kthread_work solution
> > relies on this serialization, I guess I need to read more to understand
> > why.  
> 
> You mean this patch, "drm/panthor: Use separate workqueue for DRM 
> scheduler"?
> 
> It is not strictly required. I could for example use the concurrent 
> flavour of the drm_sched_worker I add later in the series. That would 
> align with WQ_UNBOUND which is currently used, but then there is the 
> question of how much parallelism is required and the fact dumb worker 
> pool implementation from this RFC makes no attempt to spawn/retire 
> threads on demand. It just picks four out of thin air kind of. Priority 
> inheritance would still work but if picking a fix number of threads 
> based on some criteria wouldn't be feasible I would need to work on a 
> smarter worker pool implementation.

If that's easier, we can let the driver define the max number of
threads to spawn.

> 
> I could drop it, although the question would be when do you expect 
> locking rework could realistically happen and,

If that's a blocker for this series, I can try to prioritize this
change.

> if not quick, what is the 
> advantage of keeping the false parallelism.

The advantage is that we don't start relying on the artificial
serialization provided by single-threaded workqueues, because the more
we rely on that, the harder it gets to go back to a solution where GPU
contexts can submit jobs concurrently.