dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Brost <matthew.brost@intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: <dri-devel@lists.freedesktop.org>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Steven Price <steven.price@arm.com>,
	"Liviu Dudau" <liviu.dudau@arm.com>,
	Chia-I Wu <olvaffe@gmail.com>, Danilo Krummrich <dakr@kernel.org>,
	Philipp Stanner <phasta@kernel.org>, <kernel-dev@igalia.com>
Subject: Re: [RFC 0/8] DRM scheduler kthread_worker for submission latency improvements
Date: Wed, 8 Jul 2026 15:39:55 -0700	[thread overview]
Message-ID: <ak7Ru+STL9knR1xS@gsse-cloud1.jf.intel.com> (raw)
In-Reply-To: <ak63Qzqxn1k9ezEd@gsse-cloud1.jf.intel.com>

On Wed, Jul 08, 2026 at 01:46:59PM -0700, Matthew Brost wrote:
> On Wed, Jul 08, 2026 at 06:01:55PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 07/07/2026 08:12, Matthew Brost wrote:
> > > On Mon, Jul 06, 2026 at 03:54:45PM -0700, Matthew Brost wrote:
> > > > On Mon, Jul 06, 2026 at 01:41:01PM +0100, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 03/07/2026 10:22, Matthew Brost wrote:
> > > > > > On Thu, Jul 02, 2026 at 03:37:37PM +0100, Tvrtko Ursulin wrote:
> > > > > > > The problem statement is explained quite well and succinctly at:
> > > > > > > https://gitlab.freedesktop.org/panfrost/linux/-/work_items/49
> > > > > > > 
> > > > > > > Essentially, on a system (over)loaded with a lot of runnable CPU processes, a
> > > > > > > high-priority DRM client gets latency injected into the GPU submission path due
> > > > > > > to the DRM scheduler use of workqueues.
> > > > > > > 
> > > > > > > This patch series proposes to replace the workqueues with kthread_work and
> > > > > > > priority inheritance to solve this problem.
> > > > > > > 
> > > > > > > In the above linked issue Chia-I benchmarked the submit latencies which
> > > > > > > show a striking improvement:
> > > > > > > 
> > > > > > > 		median	95%	99%
> > > > > > >     before	41us	1.5ms	2.6ms
> > > > > > >      after	15us	19us	24us
> > > > > > 
> > > > > > Can you give more information on these numbers? e.g., What you ran / how
> > > > > > you measured these. It is hard to argue with numbers.
> > > > > 
> > > > > I believe Chia-I observed latency on some production hw/sw and then wrote a
> > > > > synthetic benchmark to test it more easily. Details are in the above linked
> > > > > issue.
> > > > > 
> > > > 
> > > > Thanks, I see this now. Part of the problem, as far as I can tell, is
> > > > that fences are signaled from work items rather than directly from IRQ
> > > > context.
> > > > 
> > > > For example:
> > > > 
> > > > "There is another 2.5 ms of scheduling latency from
> > > > panthor_sched_report_fw_events() (running on irq/105-panthor, PID 257)
> > > > to process_fw_events_work() (running on kworker/u32:1, PID 62)."
> > > > 
> > > > I assume this is only addressing the scheduling portion of the latency,
> > > > but you also mentioned a 9.5 ms delay for vkQueueWaitIdle(), where part
> > > > of the latency appears to be on the signaling side due to the worker
> > > > thread.
> > > > 
> > > > > > > This is obviously really good for preventing compositors from missing frames and
> > > > > > 
> > > > > > Modern compositors do not pass render-job fences to draw jobs as input
> > > > > > dependencies. On Wayland, this functionality is provided by
> > > > > > linux-drm-syncobj-v1 and is enabled by default on Ubuntu 24.10 and
> > > > > > later. SurfaceFlinger has also operated this way for quite some time.
> > > > > > 
> > > > > > The obvious solution for compositors is to submit work directly through
> > > > > > the ioctl (i.e., bypass path in sched) when there are no input
> > > > > > dependencies, which should be the common case. The main exception is
> > > > > > when one of the BOs mapped into the compositor is migrating or otherwise
> > > > > > busy (i.e., a BO has a fence in a kernel dma-resv slot).
> > > > > 
> > > > > You mean the direct submit RFC you floated some time ago? What was the
> > > > > verdict on that one, wasn't it rejected?
> > > > > 
> > > > 
> > > > I had some patches for DRM sched that I never posted. It turned out to
> > > > be a little tricky because of some other quirks in DRM sched, but it was
> > > > still roughly 100 LoC plus an additional lock.
> > > > 
> > > 
> > > Side note: I quickly rebased DRM DEP here [1] and prototyped some RT
> > > solutions.
> > > 
> > > I added a DRM_DEP_QUEUE_FLAGS_RT flag to internally choose between a
> > > workqueue and kthread_work, and to enable FIFO scheduling in [2]. Most
> > > of the details are hidden internally, but I had to make a few small
> > > changes in Xe to support this on the driver side [3].
> > > 
> > > Putting aside whether or when DRM DEP will land, if DRM sched really
> > > wants FIFO scheduling instead of bypass (the IMO this is somewhat
> > > questionable), I think the approach I took in DRM DEP makes a lot more
> > > sense. I haven't looked into what changes would be required in DRM
> > > sched, though; hopefully it wouldn't be too messy.
> > > 
> > > Of course, kthreads are now directly exposed to userspace, but this
> > > would be limited to privileged userspace with FIFO scheduling
> > > capabilities, which seems reasonable. Additionally, this approach does
> > > not require the kind of large paradigm shifts proposed by this series.
> > 
> > This sounds plausible and TBH I also considered worker duality. If you look
> > at my series I wrap everything to drm_sched_work and drm_sched_worker so
> > drivers do not even have to know what is the underlying implementation.
> > 
> 
> You could probably abstract this to some extent, but any driver-side
> functions that process work on scheduler workers would still require
> dedicated function signatures. Perhaps some creative macros could do
> something like:
> 
> DRM_SCHED_DECL_DRIVER_WORK_FUNC(driver_sched, work_func);
> DRM_SCHED_INIT_DRIVER_WORK_FUNC(driver_sched, work_func);
> 
> However, this would only eliminate functions that perform a
> `container_of()` conversion or use an `if` statement to select between
> workqueues and `kthread_work`.
> 
> > It is definitely true this solution would simplify the implementation, as
> 
> Yea, I think when I pushed back supporting kthreads + work queues, I
> wasn't aware of kthread_work which sematicly matches work queues. Given
> the same semantics supporting both seems trivial.
> 
> > you say, problem of thread explosion is limited to RT tasks so basically a
> > non-issue. And the need to create own worker pool implementation also goes
> > away.
> > 
> > Why I was unsure about deciding at drm_sched_init time is a) runtime
> > priority changes, but perhaps that is not such a big deal and we could live
> 
> Xe only allow static priorities on queues, unsure about other drivers.
> 

Sorry for the double reply. Even dynamic priorities are likely workable.
If you stop a scheduler, you should be able to reinitialize it in a
different mode (e.g., switch between a workqueue and `kthread_work`)
and then restart it.

I think DRM sched would need slightly stronger stop semantics, though—
for example, a lock around the "stopped" state and work queuing—but it
seems possible if absolutely necessary.

Matt

> Matt
> 
> > with a static setup, and b) the priority inversion problems relating to
> > other workqueues used by various drivers (like the panthor issue Boris
> > raised).
> > 
> > Or we can cap the number of workqueues to some reasonable number and then
> > Tejun maybe can give us RT workers. There are many options on the table so
> > lets see how to discussion develops.
> > 
> > Regards,
> > 
> > Tvrtko
> > 
> > > [1] https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-svn-perf-6-15-2025/-/commits/drm-dep-rebase-7-6
> > > [2] https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-svn-perf-6-15-2025/-/commit/fdc38e4acefc9327637fd818b5b91fd6b2a6198f?file_path=include%2Fdrm%2Fdrm_dep.h#line_99bf000ae_A178
> > > [3] https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-svn-perf-6-15-2025/-/commit/f1483af15747a2d5d73262d2cdfc9b616230c5d9
> > > 
> > > > DRM dep has this built in without any of that complexity. I'm hoping to
> > > > get around to rebasing it soon, but of course there's always something
> > > > else to work on. Sooner or later, it will get rebased, and Xe will move
> > > > over to it given the latency and power-saving benefits I observed. Given
> > > > power savings, I'd think drivers for ARM based phones would be pretty
> > > > keen on moving over too.
> > > > 
> > > >  From an architecture point of view, bypass is the ultimate win because
> > > > the context switch is completely avoided. Again, this is primarily for
> > > > compositor use cases, since they do not wait on fences.
> > > > 
> > > > Also, in general, if Panthor selects PANTHOR_GROUP_PRIORITY_REALTIME, it
> > > > would be very odd to pass in fences. The RT priority then depends on
> > > > other work completing before the RT job can be scheduled, which
> > > > seemingly defeats the purpose of having RT priority in the first place.
> > > > 
> > > > > > > similar. Another good quote for the above issue, explaining the consequence of
> > > > > > > CPU starvation of the submit path, is this:
> > > > > > > 
> > > > > > > """
> > > > > > > As a result, vkQueueWaitIdle blocks for 9.5ms for a gpu job that takes 4.5ms
> > > > > > > gpu time.
> > > > > > > """
> > > > > > 
> > > > > > More details here?
> > > > > 
> > > > > This just illustrates how long can the workqueue wait for it's slice on the
> > > > > CPU if the cores are loaded with other tasks. In other words, how long since
> > > > > job is runnable to it actually being passed to the GPU.
> > > > > 
> > > > 
> > > > Yes. I do wonder if this is an Android quirk, though. While working on
> > > > some MM-related Android issues, I noticed it has quite a few quirks.
> > > > 
> > > > I've done a lot of profiling around workqueues in Xe (submission, page
> > > > faults, resuming after preempt fences, etc.), and I've seen latency
> > > > spikes of perhaps 10–20 µs, but never anything on the millisecond scale
> > > > on Linux.
> > > > 
> > > > I also recently fixed a workqueue bug [1] that showed up fairly often on
> > > > Android. In that case, workqueues could stop scheduling under the right
> > > > conditions. If I recall correctly, a flush_work() could get the work
> > > > item unstuck. It might be worth looking into whether that helps here.
> > > > 
> > > > [1] https://patchwork.freedesktop.org/series/164199/
> > > > 
> > > > > > > DRM scheduler was originally using kthreads but was converted workqueues due
> > > > > > > desire by xe to create thousands of schedulers. This series also questions
> > > > > > > whether that was needed, given how the submission is serialized by a device
> > > > > > > global lock (per GT, so almost device global). Panthor has a similar situation;
> > > > > > > hence the series contains two patches to move those two to a setup which matches
> > > > > > > the design of those drivers.
> > > > > > > 
> > > > > > > Other drivers, like for example amdgpu, v3d, etnaviv etc, which use the
> > > > > > > scheduler as a hardware scheduler, where number of instances follow the number
> > > > > > > of hardware blocks instead the number of userspace contexts, are completely
> > > > > > > fine.
> > > > > > > 
> > > > > > > There are use cases however which do currently track the number of userspace
> > > > > > > contexts and which do allow for more parallelism. For those a straight
> > > > > > > kthread_work conversion would be a problem due an explosion in number of
> > > > > > > threads.
> > > > > > > 
> > > > > > > The most direct example is panthor VM bind queue which creates a scheduler per
> > > > > > > userspace context and relies on work queue concurrency management to keep the
> > > > > > > number of threads in check.
> > > > > > > 
> > > > > > > This creates a challenge for the kthread_work conversion. To solve which I for
> > > > > > > now opted to create a trivial round-robin thread pool. For the RFC this is
> > > > > > > limited to four CPU threads and is something which will need to be discussed.
> > > > > > 
> > > > > > 4 CPU threads per device, per drm sched module?
> > > > > 
> > > > > 4 CPU threads per drm_sched_create_concurrent_worker(). So depends on the
> > > > 
> > > > Ugh, that is a pretty ugly API.
> > > > 
> > > > Matt
> > > > 
> > > > > driver how it wants to use it. For xe there are no usages of that, albeit
> > > > > you say single thread is not workable, we can discuss that in the other
> > > > > sub-thread. For panthor this RFC uses that flavour of the worker for the VM
> > > > > bind queues and in that case it is 4 CPU threads per device which handle VM
> > > > > bind requests from all userspace clients.
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Tvrtko
> > > > > 
> > > > > > 
> > > > > > I have more questions but let's get some clarification first.
> > > > > > 
> > > > > > Matt
> > > > > > 
> > > > > > > Ie. how much parallelsim those really need. The true answer is somewhere between
> > > > > > > "at most the number of active userspace contexts and the number of CPU cores".
> > > > > > > Or it could be less than that, since after all, VM BIND parallelism is
> > > > > > > eventually going to choke on a narrower gate of actual GPU execution. We could
> > > > > > > also allow drivers to pick their number.
> > > > > > > 
> > > > > > > In terms of how I implemented priority inheritance, the most important
> > > > > > > characteristic is that it is temporary. As many userspace clients may be
> > > > > > > submitting to a single DRM scheduler instance, a generic solution is to only
> > > > > > > elevate the submission worker priority while there are active high priority
> > > > > > > submitters. The mechanism is light weight and has a hysteresis built in to avoid
> > > > > > > frequent scheduler operations.
> > > > > > > 
> > > > > > > That's pretty much it for now apart for an important detail that this RFC will
> > > > > > > not build for all drivers! Out of those which directly use the DRM scheduler
> > > > > > > APIs changed, I converted only panthor and xe. Amdgpu will also work by the way.
> > > > > > > Others I have not tried to build.
> > > > > > > 
> > > > > > > Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > > Cc: Steven Price <steven.price@arm.com>
> > > > > > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > > > > > > Cc: Chia-I Wu <olvaffe@gmail.com>
> > > > > > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > > > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > > > > > Cc: Philipp Stanner <phasta@kernel.org>
> > > > > > > 
> > > > > > > Tvrtko Ursulin (8):
> > > > > > >     drm/panthor: Remove redundant drm_sched_job_cleanup() from the
> > > > > > >       .free_job callback
> > > > > > >     drm/panthor: Use separate workqueue for DRM scheduler
> > > > > > >     drm/sched: Use generic naming for workqueue helpers
> > > > > > >     drm/xe: Convert to per gt scheduler workers
> > > > > > >     drm: Wrap DRM scheduler worker in own abstraction
> > > > > > >     drm/sched: Convert the scheduler job submission to kthread_worker
> > > > > > >     drm/sched: Add ability to change drm_sched_worker priority
> > > > > > >     drm/sched: Notify worker of the entity submission priority
> > > > > > > 
> > > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |   8 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  |   4 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c     |   4 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    |   2 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c    |   8 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c     |   8 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c     |   2 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c     |   4 +-
> > > > > > >    drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c     |   4 +-
> > > > > > >    drivers/gpu/drm/msm/adreno/adreno_device.c  |   4 +-
> > > > > > >    drivers/gpu/drm/panthor/panthor_mmu.c       |  20 +-
> > > > > > >    drivers/gpu/drm/panthor/panthor_sched.c     |  23 ++-
> > > > > > >    drivers/gpu/drm/scheduler/sched_entity.c    |   3 +
> > > > > > >    drivers/gpu/drm/scheduler/sched_main.c      | 168 +++++++++++-----
> > > > > > >    drivers/gpu/drm/scheduler/sched_rq.c        | 202 ++++++++++++++++++++
> > > > > > >    drivers/gpu/drm/xe/xe_dep_scheduler.c       |   6 +-
> > > > > > >    drivers/gpu/drm/xe/xe_dep_scheduler.h       |   6 +-
> > > > > > >    drivers/gpu/drm/xe/xe_exec_queue.c          |   6 +-
> > > > > > >    drivers/gpu/drm/xe/xe_gpu_scheduler.c       |  21 +-
> > > > > > >    drivers/gpu/drm/xe/xe_gpu_scheduler.h       |   2 +-
> > > > > > >    drivers/gpu/drm/xe/xe_gpu_scheduler_types.h |   2 +-
> > > > > > >    drivers/gpu/drm/xe/xe_gt.c                  |   7 +
> > > > > > >    drivers/gpu/drm/xe/xe_gt_types.h            |   3 +
> > > > > > >    drivers/gpu/drm/xe/xe_guc_submit.c          |  18 +-
> > > > > > >    drivers/gpu/drm/xe/xe_tlb_inval.c           |  14 +-
> > > > > > >    drivers/gpu/drm/xe/xe_tlb_inval_types.h     |   5 +-
> > > > > > >    include/drm/gpu_scheduler.h                 | 131 ++++++++++++-
> > > > > > >    27 files changed, 551 insertions(+), 134 deletions(-)
> > > > > > > 
> > > > > > > -- 
> > > > > > > 2.54.0
> > > > > 
> > 

  reply	other threads:[~2026-07-08 22:40 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-02 14:37 [RFC 0/8] DRM scheduler kthread_worker for submission latency improvements Tvrtko Ursulin
2026-07-02 14:37 ` [RFC 1/8] drm/panthor: Remove redundant drm_sched_job_cleanup() from the .free_job callback Tvrtko Ursulin
2026-07-03 15:00   ` Steven Price
2026-07-09 10:05     ` Tvrtko Ursulin
2026-07-09 10:15       ` Boris Brezillon
2026-07-09 12:16         ` Tvrtko Ursulin
2026-07-02 14:37 ` [RFC 2/8] drm/panthor: Use separate workqueue for DRM scheduler Tvrtko Ursulin
2026-07-02 15:31   ` Boris Brezillon
2026-07-06 12:03     ` Tvrtko Ursulin
2026-07-06 14:18       ` Boris Brezillon
2026-07-08 16:47         ` Tvrtko Ursulin
2026-07-09  6:45           ` Boris Brezillon
2026-07-09 10:56             ` Tvrtko Ursulin
2026-07-09 12:19               ` Boris Brezillon
2026-07-09 14:33                 ` Tvrtko Ursulin
2026-07-09 15:24                   ` Boris Brezillon
2026-07-02 14:37 ` [RFC 3/8] drm/sched: Use generic naming for workqueue helpers Tvrtko Ursulin
2026-07-02 14:37 ` [RFC 4/8] drm/xe: Convert to per gt scheduler workers Tvrtko Ursulin
2026-07-03  9:06   ` Matthew Brost
2026-07-06 12:27     ` Tvrtko Ursulin
2026-07-06 23:14       ` Matthew Brost
2026-07-08 16:35         ` Tvrtko Ursulin
2026-07-08 19:32           ` Matthew Brost
2026-07-09 11:35             ` Tvrtko Ursulin
2026-07-09 11:45               ` Matthew Brost
2026-07-09 12:05                 ` Tvrtko Ursulin
2026-07-02 14:37 ` [RFC 5/8] drm: Wrap DRM scheduler worker in own abstraction Tvrtko Ursulin
2026-07-02 14:37 ` [RFC 6/8] drm/sched: Convert the scheduler job submission to kthread_worker Tvrtko Ursulin
2026-07-02 14:37 ` [RFC 7/8] drm/sched: Add ability to change drm_sched_worker priority Tvrtko Ursulin
2026-07-02 14:37 ` [RFC 8/8] drm/sched: Notify worker of the entity submission priority Tvrtko Ursulin
2026-07-03  8:52 ` [RFC 0/8] DRM scheduler kthread_worker for submission latency improvements Philipp Stanner
2026-07-06 12:20   ` Tvrtko Ursulin
2026-07-06 19:36     ` Tejun Heo
2026-07-08 16:24       ` Tvrtko Ursulin
2026-07-09  8:28         ` Tvrtko Ursulin
2026-07-10 12:29           ` Tvrtko Ursulin
2026-07-10 16:11             ` Tejun Heo
2026-07-10 18:29               ` Matthew Brost
2026-07-03  9:22 ` Matthew Brost
2026-07-06 12:41   ` Tvrtko Ursulin
2026-07-06 22:54     ` Matthew Brost
2026-07-07  7:12       ` Matthew Brost
2026-07-08 17:01         ` Tvrtko Ursulin
2026-07-08 20:46           ` Matthew Brost
2026-07-08 22:39             ` Matthew Brost [this message]
2026-07-09  6:52             ` Boris Brezillon
2026-07-09  7:25             ` Boris Brezillon
2026-07-09  7:55               ` Matthew Brost
2026-07-09 10:08                 ` Boris Brezillon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ak7Ru+STL9knR1xS@gsse-cloud1.jf.intel.com \
    --to=matthew.brost@intel.com \
    --cc=boris.brezillon@collabora.com \
    --cc=dakr@kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=kernel-dev@igalia.com \
    --cc=liviu.dudau@arm.com \
    --cc=olvaffe@gmail.com \
    --cc=phasta@kernel.org \
    --cc=steven.price@arm.com \
    --cc=tvrtko.ursulin@igalia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox