All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Brost <matthew.brost@intel.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>,
	<dri-devel@lists.freedesktop.org>,
	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: Thu, 9 Jul 2026 00:55:17 -0700	[thread overview]
Message-ID: <ak9T5V6BQE2bojZs@gsse-cloud1.jf.intel.com> (raw)
In-Reply-To: <20260709092552.256e2b3f@fedora-2.home>

On Thu, Jul 09, 2026 at 09:25:52AM +0200, Boris Brezillon wrote:
> On Wed, 8 Jul 2026 13:46:59 -0700
> Matthew Brost <matthew.brost@intel.com> 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`.
> 
> Honestly, I'm questioning the viability of such an hybrid design. Not
> only this makes the code base even harder to reason about (and thus,
> harder to maintain), but if you think about it, the only reason we got

If this is hard to maintain or reason about, then the scheduler is
broken. It took me roughly 10 minutes to modify a branch with DRM dep
and Xe to support a hybrid design, and another 20 minutes or so to
figure out how to run an Xe test with FIFO scheduling :). It shouldn't
be difficult to do the same in DRM sched either. If it is, that only
highlights why I think DRM dep is a good idea.

> here is because we pretended the full/monolithic drm_sched stack was a
> good fit for this FW-scheduler use case, and the reality is that it's
> not.
> 
> What we need for this FW-scheduler case is a simple per-context FIFO
> with dep tracking. The KMD can then decide when to pull jobs from
> this FIFO with its own deferred work model (workqueues, kthread or
> whatever). All it needs to have is:
> 
> - a way to initialize the job and its deps (so basically the
>   drm_sched_job_xxx() helpers)
> - a way to queue a job to this FIFO (something like
>   drm_sched_entity_push_job(), but maybe simpler)
> - a way to evaluate if the first job in the queue is ready for
>   execution
> - a way to register a callback for asynchronous queue/job readiness
>   re-evaluation. That would basically be called on a dep fence callback
>   call, and defers the responsibility of how the re-evaluation is done
>   to the driver. If you're using kthreads, you'll just be issuing a
>   kthread_queue_work() from this callback. If you want workqueues, you'd
>   be using queue_work_on(), etc.
> 
> The key here, is that the framework no longer decides the deferred work
> model the driver has to use. This push/pull model makes it so the driver
> is in control at all times, which also simplifies things like teardown,
> or rather, make it so drm_sched is not in the way of driver specific
> teardown sequences.

drm_sched teardowns are in fact awful.

> 
> I know some of these concepts have been discussed with Philipp and
> Danilo for the JobQueue implementation they intend to have on the
> rust-side. I also know that they are not particularly thrilled by this
> push/pull model, I think they're more going for a solution where jobs
> are pushed to the driver when they are ready for execution, which
> requires having their own worker/work to defer the run_job() call, so
> back to the problem that the framework enforces the execution model.
> 

This all sounds nice, but my initial reaction is that you're asking the
driver to do a lot of things that it will likely get wrong—for example,
stopping and starting queues, DMA-fence rules, tracking jobs to
completion, handling timeouts, managing object lifetimes, and
implementing teardown flows. Consequently, DRM sched has historically
gotten many of these things wrong as well, although the concepts you
want a scheduler to provide are there—they're just implemented poorly.

Another major problem with what you're suggesting is that you
essentially have to accept that a ready job may be pushed to the driver
from IRQ context, since fences can signal from IRQ context. That's a
massive can of worms, and I know firsthand what a terrible idea it can
be because i915 did exactly that.

So unless I'm misunderstansing you suggestion, this is something I'd
never use as common DRM idea in any driver I was involved with.

Matt

> Oh, and also, just because more is deferred to the driver doesn't mean
> we can't have generic helpers for kthread-based/workqueue-based
> deferral, the big difference being that, it's now up to the driver to
> decide which ones they want to use.

  reply	other threads:[~2026-07-09  7:55 UTC|newest]

Thread overview: 45+ 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-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-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
2026-07-09  6:52             ` Boris Brezillon
2026-07-09  7:25             ` Boris Brezillon
2026-07-09  7:55               ` Matthew Brost [this message]
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=ak9T5V6BQE2bojZs@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.