Re: [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness

[Rob Clark]

On Tue, Mar 21, 2023 at 6:24 AM Jonas Ådahl <jadahl@xxxxxxxxx> wrote:
>
> On Fri, Mar 17, 2023 at 08:59:48AM -0700, Rob Clark wrote:
> > On Fri, Mar 17, 2023 at 3:23 AM Jonas Ådahl <jadahl@xxxxxxxxx> wrote:
> > >
> > > On Thu, Mar 16, 2023 at 09:28:55AM -0700, Rob Clark wrote:
> > > > On Thu, Mar 16, 2023 at 2:26 AM Jonas Ådahl <jadahl@xxxxxxxxx> wrote:
> > > > >
> > > > > On Wed, Mar 15, 2023 at 09:19:49AM -0700, Rob Clark wrote:
> > > > > > On Wed, Mar 15, 2023 at 6:53 AM Jonas Ådahl <jadahl@xxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Fri, Mar 10, 2023 at 09:38:18AM -0800, Rob Clark wrote:
> > > > > > > > On Fri, Mar 10, 2023 at 7:45 AM Jonas Ådahl <jadahl@xxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Mar 08, 2023 at 07:52:52AM -0800, Rob Clark wrote:
> > > > > > > > > > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > > > > > > >
> > > > > > > > > > Add a way to hint to the fence signaler of an upcoming deadline, such as
> > > > > > > > > > vblank, which the fence waiter would prefer not to miss.  This is to aid
> > > > > > > > > > the fence signaler in making power management decisions, like boosting
> > > > > > > > > > frequency as the deadline approaches and awareness of missing deadlines
> > > > > > > > > > so that can be factored in to the frequency scaling.
> > > > > > > > > >
> > > > > > > > > > v2: Drop dma_fence::deadline and related logic to filter duplicate
> > > > > > > > > >     deadlines, to avoid increasing dma_fence size.  The fence-context
> > > > > > > > > >     implementation will need similar logic to track deadlines of all
> > > > > > > > > >     the fences on the same timeline.  [ckoenig]
> > > > > > > > > > v3: Clarify locking wrt. set_deadline callback
> > > > > > > > > > v4: Clarify in docs comment that this is a hint
> > > > > > > > > > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT.
> > > > > > > > > > v6: More docs
> > > > > > > > > > v7: Fix typo, clarify past deadlines
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > > > > > > > Reviewed-by: Christian König <christian.koenig@xxxxxxx>
> > > > > > > > > > Acked-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> > > > > > > > > > Reviewed-by: Bagas Sanjaya <bagasdotme@xxxxxxxxx>
> > > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Hi Rob!
> > > > > > > > >
> > > > > > > > > >  Documentation/driver-api/dma-buf.rst |  6 +++
> > > > > > > > > >  drivers/dma-buf/dma-fence.c          | 59 ++++++++++++++++++++++++++++
> > > > > > > > > >  include/linux/dma-fence.h            | 22 +++++++++++
> > > > > > > > > >  3 files changed, 87 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
> > > > > > > > > > index 622b8156d212..183e480d8cea 100644
> > > > > > > > > > --- a/Documentation/driver-api/dma-buf.rst
> > > > > > > > > > +++ b/Documentation/driver-api/dma-buf.rst
> > > > > > > > > > @@ -164,6 +164,12 @@ DMA Fence Signalling Annotations
> > > > > > > > > >  .. kernel-doc:: drivers/dma-buf/dma-fence.c
> > > > > > > > > >     :doc: fence signalling annotation
> > > > > > > > > >
> > > > > > > > > > +DMA Fence Deadline Hints
> > > > > > > > > > +~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > > > > +
> > > > > > > > > > +.. kernel-doc:: drivers/dma-buf/dma-fence.c
> > > > > > > > > > +   :doc: deadline hints
> > > > > > > > > > +
> > > > > > > > > >  DMA Fences Functions Reference
> > > > > > > > > >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > > > > > > > index 0de0482cd36e..f177c56269bb 100644
> > > > > > > > > > --- a/drivers/dma-buf/dma-fence.c
> > > > > > > > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > > > > > > > @@ -912,6 +912,65 @@ dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
> > > > > > > > > >  }
> > > > > > > > > >  EXPORT_SYMBOL(dma_fence_wait_any_timeout);
> > > > > > > > > >
> > > > > > > > > > +/**
> > > > > > > > > > + * DOC: deadline hints
> > > > > > > > > > + *
> > > > > > > > > > + * In an ideal world, it would be possible to pipeline a workload sufficiently
> > > > > > > > > > + * that a utilization based device frequency governor could arrive at a minimum
> > > > > > > > > > + * frequency that meets the requirements of the use-case, in order to minimize
> > > > > > > > > > + * power consumption.  But in the real world there are many workloads which
> > > > > > > > > > + * defy this ideal.  For example, but not limited to:
> > > > > > > > > > + *
> > > > > > > > > > + * * Workloads that ping-pong between device and CPU, with alternating periods
> > > > > > > > > > + *   of CPU waiting for device, and device waiting on CPU.  This can result in
> > > > > > > > > > + *   devfreq and cpufreq seeing idle time in their respective domains and in
> > > > > > > > > > + *   result reduce frequency.
> > > > > > > > > > + *
> > > > > > > > > > + * * Workloads that interact with a periodic time based deadline, such as double
> > > > > > > > > > + *   buffered GPU rendering vs vblank sync'd page flipping.  In this scenario,
> > > > > > > > > > + *   missing a vblank deadline results in an *increase* in idle time on the GPU
> > > > > > > > > > + *   (since it has to wait an additional vblank period), sending a signal to
> > > > > > > > > > + *   the GPU's devfreq to reduce frequency, when in fact the opposite is what is
> > > > > > > > > > + *   needed.
> > > > > > > > >
> > > > > > > > > This is the use case I'd like to get some better understanding about how
> > > > > > > > > this series intends to work, as the problematic scheduling behavior
> > > > > > > > > triggered by missed deadlines has plagued compositing display servers
> > > > > > > > > for a long time.
> > > > > > > > >
> > > > > > > > > I apologize, I'm not a GPU driver developer, nor an OpenGL driver
> > > > > > > > > developer, so I will need some hand holding when it comes to
> > > > > > > > > understanding exactly what piece of software is responsible for
> > > > > > > > > communicating what piece of information.
> > > > > > > > >
> > > > > > > > > > + *
> > > > > > > > > > + * To this end, deadline hint(s) can be set on a &dma_fence via &dma_fence_set_deadline.
> > > > > > > > > > + * The deadline hint provides a way for the waiting driver, or userspace, to
> > > > > > > > > > + * convey an appropriate sense of urgency to the signaling driver.
> > > > > > > > > > + *
> > > > > > > > > > + * A deadline hint is given in absolute ktime (CLOCK_MONOTONIC for userspace
> > > > > > > > > > + * facing APIs).  The time could either be some point in the future (such as
> > > > > > > > > > + * the vblank based deadline for page-flipping, or the start of a compositor's
> > > > > > > > > > + * composition cycle), or the current time to indicate an immediate deadline
> > > > > > > > > > + * hint (Ie. forward progress cannot be made until this fence is signaled).
> > > > > > > > >
> > > > > > > > > Is it guaranteed that a GPU driver will use the actual start of the
> > > > > > > > > vblank as the effective deadline? I have some memories of seing
> > > > > > > > > something about vblank evasion browsing driver code, which I might have
> > > > > > > > > misunderstood, but I have yet to find whether this is something
> > > > > > > > > userspace can actually expect to be something it can rely on.
> > > > > > > >
> > > > > > > > I guess you mean s/GPU driver/display driver/ ?  It makes things more
> > > > > > > > clear if we talk about them separately even if they happen to be the
> > > > > > > > same device.
> > > > > > >
> > > > > > > Sure, sorry about being unclear about that.
> > > > > > >
> > > > > > > >
> > > > > > > > Assuming that is what you mean, nothing strongly defines what the
> > > > > > > > deadline is.  In practice there is probably some buffering in the
> > > > > > > > display controller.  For ex, block based (including bandwidth
> > > > > > > > compressed) formats, you need to buffer up a row of blocks to
> > > > > > > > efficiently linearize for scanout.  So you probably need to latch some
> > > > > > > > time before you start sending pixel data to the display.  But details
> > > > > > > > like this are heavily implementation dependent.  I think the most
> > > > > > > > reasonable thing to target is start of vblank.
> > > > > > >
> > > > > > > The driver exposing those details would be quite useful for userspace
> > > > > > > though, so that it can delay committing updates to late, but not too
> > > > > > > late. Setting a deadline to be the vblank seems easy enough, but it
> > > > > > > isn't enough for scheduling the actual commit.
> > > > > >
> > > > > > I'm not entirely sure how that would even work.. but OTOH I think you
> > > > > > are talking about something on the order of 100us?  But that is a bit
> > > > > > of another topic.
> > > > >
> > > > > Yes, something like that. But yea, it's not really related. Scheduling
> > > > > commits closer to the deadline has more complex behavior than that too,
> > > > > e.g. the need for real time scheduling, and knowing how long it usually
> > > > > takes to create and commit and for the kernel to process.
> > > > >
> > > > > >
> > > > >
> > > > > 8-< *snip* 8-<
> > > > >
> > > > > > > >
> > > > > > > > You need a fence to set the deadline, and for that work needs to be
> > > > > > > > flushed.  But you can't associate a deadline with work that the kernel
> > > > > > > > is unaware of anyways.
> > > > > > >
> > > > > > > That makes sense, but it might also a bit inadequate to have it as the
> > > > > > > only way to tell the kernel it should speed things up. Even with the
> > > > > > > trick i915 does, with GNOME Shell, we still end up with the feedback
> > > > > > > loop this series aims to mitigate. Doing triple buffering, i.e. delaying
> > > > > > > or dropping the first frame is so far the best work around that works,
> > > > > > > except doing other tricks that makes the kernel to ramp up its clock.
> > > > > > > Having to rely on choosing between latency and frame drops should
> > > > > > > ideally not have to be made.
> > > > > >
> > > > > > Before you have a fence, the thing you want to be speeding up is the
> > > > > > CPU, not the GPU.  There are existing mechanisms for that.
> > > > >
> > > > > Is there no benefit to let the GPU know earlier that it should speed up,
> > > > > so that when the job queue arrives, it's already up to speed?
> > > >
> > > > Downstream we have input notifier that resumes the GPU so we can
> > > > pipeline the 1-2ms it takes to boot up the GPU with userspace.  But we
> > > > wait to boost freq until we have cmdstream to submit, since that
> > > > doesn't take as long.  What needs help initially after input is all
> > > > the stuff that happens on the CPU before the GPU can start to do
> > > > anything ;-)
> > >
> > > How do you deal with boosting CPU speeds downstream? Does the input
> > > notifier do that too?
> >
> > Yes.. actually currently downstream (depending on device) we have 1 to
> > 3 input notifiers, one for CPU boost, one for early-PSR-exit, and one
> > to get a head start on booting up the GPU.
>
> Would be really nice to upstream these, one way or the other, be it
> actually input event based, or via some uapi to just poke the kernel. I
> realize it's not related to this thread, so this is just me wishing
> things into the void.

There was a drm/input_helper proposed maybe a year or so back, mainly
for the early-PSR-exit but I was planning to build on that for early
GPU wake-up.  I guess we should revisit it.  Might not be the right
place for cpu boost, but it solves some problems so it's a start.

As far as uapi, I think sysfs already gives you everything or at least
most everything you need.  For ex,
/sys/devices/system/cpu/cpufreq/policy*/scaling_min_freq .. on the gpu
side, for drivers using devfreq (ie. panfrost/msm/etc) there is
similar sysfs.  I'm not sure what sort of knobs are avail on
intel/amd.

BR,
-R

> >
> > > >
> > > > Btw, I guess I haven't made this clear, dma-fence deadline is trying
> > > > to help the steady-state situation, rather than the input-latency
> > > > situation.  It might take a frame or two of missed deadlines for
> > > > gpufreq to arrive at a good steady-state freq.
> > >
> > > I'm just not sure it will help. Missed deadlines set at commit hasn't
> > > been enough in the past to let the kernel understand it should speed
> > > things up before the next frame (which will be a whole frame late
> > > without any triple buffering which should be a last resort), so I don't
> > > see how it will help by adding a userspace hook to do the same thing.
> >
> > So deadline is just a superset of "right now" and "sometime in the
> > future".. and this has been useful enough for i915 that they have both
> > forms, when waiting on GPU via i915 specific ioctls and when pageflip
> > (assuming userspace isn't deferring composition decision and instead
> > just pushing it all down to the kernel).  But this breaks down in a
> > few cases:
> >
> > 1) non pageflip (for ex. ping-ponging between cpu and gpu) use cases
> > when you wait via polling on fence fd or wait via drm_syncobj instead
> > of DRM_IOCTL_I915_GEM_WAIT
> > 2) when userspace decides late in frame to not pageflip because app
> > fence isn't signaled yet
>
> It breaks down in practice today, because we do entering the low-freq
> feedback loop that triple buffering today effectively works around.
> That is even with non-delayed page flipping, and a single pipeline
> source (compositor only rendering) or only using already signaled ready
> client buffers when compositing.
>
> Anyway, I don't doubt its usefulness, just a bit pessimistic.
>
> >
> > And this is all done in a way that doesn't help for situations where
> > you have separate kms and render devices.  Or the kms driver doesn't
> > bypass atomic helpers (ie. uses drm_atomic_helper_wait_for_fences()).
> > So the technique has already proven to be useful.  This series just
> > extends it beyond driver specific primitives (ie.
> > dma_fence/drm_syncojb)
> >
> > > I think input latency and steady state target frequency here is tightly
> > > linked; what we should aim for is to provide enough information at the
> > > right time so that it does *not* take a frame or two to of missed
> > > deadlines to arrive at the target frequency, as those missed deadlines
> > > either means either stuttering and/or lag.
> >
> > If you have some magic way for a gl/vk driver to accurately predict
> > how many cycles it will take to execute a sequence of draws, I'm all
> > ears.
> >
> > Realistically, the best solution on sudden input is to overshoot and
> > let freqs settle back down.
> >
> > But there is a lot more to input latency than GPU freq.  In UI
> > workloads, even fullscreen animation, I don't really see the GPU going
> > above the 2nd lowest OPP even on relatively small things like a618.
> > UI input latency (touch scrolling, on-screen stylus / low-latency-ink,
> > animations) are a separate issue from what this series addresses, and
> > aren't too much to do with GPU freq.
> >
> > > That it helps with the deliberately late commit I do understand, but we
> > > don't do that yet, but intend to when there is kernel uapi to lets us do
> > > so without negative consequences.
> > >
> > > >
> > > > > >
> > > > > > TBF I'm of the belief that there is still a need for input based cpu
> > > > > > boost (and early wake-up trigger for GPU).. we have something like
> > > > > > this in CrOS kernel.  That is a bit of a different topic, but my point
> > > > > > is that fence deadlines are just one of several things we need to
> > > > > > optimize power/perf and responsiveness, rather than the single thing
> > > > > > that solves every problem under the sun ;-)
> > > > >
> > > > > Perhaps; but I believe it's a bit of a back channel of intent; the piece
> > > > > of the puzzle that has the information to know whether there is need
> > > > > actually speed up is the compositor, not the kernel.
> > > > >
> > > > > For example, pressing 'p' while a terminal is focused does not need high
> > > > > frequency clocks, it just needs the terminal emulator to draw a 'p' and
> > > > > the compositor to composite that update. Pressing <Super> may however
> > > > > trigger a non-trivial animation moving a lot of stuff around on screen,
> > > > > maybe triggering Wayland clients to draw and what not, and should most
> > > > > arguably have the ability to "warn" the kernel about the upcoming flood
> > > > > of work before it is already knocking on its door step.
> > > >
> > > > The super key is problematic, but not for the reason you think.  It is
> > > > because it is a case where we should boost on key-up instead of
> > > > key-down.. and the second key-up event comes after the cpu-boost is
> > > > already in it's cool-down period.  But even if suboptimal in cases
> > > > like this, it is still useful for touch/stylus cases where the
> > > > slightest of lag is much more perceptible.
> > >
> > > Other keys are even more problematic. Alt, for example, does nothing,
> > > Alt + Tab does some light rendering, but Alt + KeyAboveTab will,
> > > depending on the current active applications, suddenly trigger N Wayland
> > > surfaces to start rendering at the same time.
> > >
> > > >
> > > > This is getting off topic but I kinda favor coming up with some sort
> > > > of static definition that userspace could give the kernel to let the
> > > > kernel know what input to boost on.  Or maybe something could be done
> > > > with BPF?
> > >
> > > I have hard time seeing any static information can be enough, it's
> > > depends too much on context what is expected to happen. And can a BPF
> > > program really help? Unless BPF programs that pulls some internal kernel
> > > strings to speed things up whenever userspace wants I don't see how it
> > > is that much better.
> > >
> > > I don't think userspace is necessarily too slow to actively particitpate
> > > in providing direct scheduling hints either. Input processing can, for
> > > example, be off loaded to a real time scheduled thread, and plumbing any
> > > hints about future expectations from rendering, windowing and layout
> > > subsystems will be significantly easier to plumb to a real time input
> > > thread than translated into static informations or BPF programs.
> >
> > I mean, the kernel side input handler is called from irq context long
> > before even the scheduler gets involved..
> >
> > But I think you are over-thinking the Alt + SomeOtherKey case.  The
> > important thing isn't what the other key is, it is just to know that
> > Alt is a modifier key (ie. handle it on key-up instead of key-down).
> > No need to over-complicate things.  It's probably enough to give the
> > kernel a list of modifier+key combo's that do _something_..
>
> Perhaps I'm over thinking it, it just seems all so unnecessary to
> complicate the kernel so that it's able to predict when GUI animations
> will happen instead of the GUI itself doing it when it is actually
> beneficial. All it'd take (naively) is uapi for the three kind of boosts
> downstream now does automatically from input events.
>
> >
> > And like I've said before, keyboard input is the least problematic in
> > terms of latency.  It is a _lot_ easier to notice lag with touch
> > scrolling or stylus (on screen).  (The latter case, I think wayland
> > has some catching up to do compared to CrOS or android.. you really
> > need a way to allow the app to do front buffer rendering to an overlay
> > for the stylus case, because even just 16ms delay is _very_
> > noticeable.)
>
> Sure, but here too userpsace (rt thread in the compositor) is probably a
> good enough place to predict when to boost since it will be the one
> proxies e.g. the stylus input events to the application.
>
> Front buffering on the other hand is a very different topic ;)
>
>
> Jonas
>
> >
> > BR,
> > -R