Re: [Linaro-mm-sig] [RFC 1/3] dma-fence: Add boost fence op

[Daniel Vetter]

On Fri, May 21, 2021 at 09:43:59AM +0200, Christian König wrote:
> Am 20.05.21 um 19:08 schrieb Daniel Vetter:
> > [SNIP]
> > > AH! So we are basically telling the fence backend that we have just
> > > missed an event we waited for.
> > > 
> > > So what we want to know is how long the frontend wanted to wait instead
> > > of how long the backend took for rendering.
> > tbh I'm not sure the timestamp matters at all. What we do in i915 is
> > boost quite aggressively, and then let the usual clock tuning wittle
> > it down if we overshot. Plus soom cool-down to prevent
> > abuse/continuous boosting. I think we also differentiate between
> > display boost and userspace waits.
> 
> I was not thinking about time stamps here, but more like which information
> we need at which place.
> 
> > On the display side we also wait until the vblank has passed we aimed
> > for (atm always the next, we don't have target_frame support like
> > amdgpu), to avoid boosting when there's no point.
> > 
> > > > So boosting right when you've missed your frame (not what Rob implements
> > > > currently, but fixable) is the right semantics.
> > > > 
> > > > The other issue is that for cpu waits, we want to differentiate from fence
> > > > waits that userspace does intentially (e.g. wait ioctl) and waits that
> > > > random other things are doing within the kernel to keep track of progress.
> > > > 
> > > > For the former we know that userspace is stuck waiting for the gpu, and we
> > > > probably want to boost. For the latter we most definitely do _not_ want to
> > > > boost.
> > > > 
> > > > Otoh I do agree with you that the current api is a bit awkward, so perhaps
> > > > we do need a dma_fence_userspace_wait wrapper which boosts automatically
> > > > after a bit. And similarly perhaps a drm_vblank_dma_fence_wait, where you
> > > > give it a vblank target, and if the fence isn't signalled by then, we kick
> > > > it real hard.
> > > Yeah, something like an use case driven API would be nice to have.
> > > 
> > > For this particular case I suggest that we somehow extend the enable
> > > signaling callback.
> > > 
> > > > But otherwise yes this is absolutely a thing that matters a ton. If you
> > > > look at Matt Brost's scheduler rfc, there's also a line item in there
> > > > about adding this kind of boosting to drm/scheduler.
> > > BTW: I still can't see this in my inbox.
> > You've replied already:
> > 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20210518235830.133834-1-matthew.brost%40intel.com%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Ce4f3688b832842c4236e08d91bb1e148%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637571273080820910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=uk3Gs%2FW42BDqMuMJtujcAH5GvN8mOlDnmywK8x1I%2F0k%3D&reserved=0
> 
> Yeah, but doesn't that also require some changes to the DRM scheduler?
> 
> I was expecting that this is a bit more than just two patches.

It's just the rfc document, per the new rfc process:

https://dri.freedesktop.org/docs/drm/gpu/rfc/

It's rather obviously not any piece of code in there, but just meant to
check rough direction before we go rewrite the entire i915 execbuf
frontend.
-Daniel

> 
> Christian.
> 
> > 
> > It's just the big picture plan of what areas we're all trying to
> > tackle with some why, so that everyone knows what's coming in the next
> > half year at least. Probably longer until this is all sorted. I think
> > Matt has some poc hacked-up pile, but nothing really to show.
> > -Daniel
> > 
> > > Do you have a link?
> > > 
> > > Christian.
> > > 
> > > > -Daniel
> > > > 
> > > > 
> > > > > Regards,
> > > > > Christian.
> > > > > 
> > > > > > BR,
> > > > > > -R
> > > > > > 
> > > > > > > Thanks,
> > > > > > > Christian.
> > > > > > > 
> > > > > > > > BR,
> > > > > > > > -R
> > > > > > > > 
> > > > > > > > > Christian.
> > > > > > > > > 
> > > > > > > > > Am 19.05.21 um 20:38 schrieb Rob Clark:
> > > > > > > > > > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > > > > > > > 
> > > > > > > > > > Add a way to hint to the fence signaler that a fence waiter has missed a
> > > > > > > > > > deadline waiting on the fence.
> > > > > > > > > > 
> > > > > > > > > > In some cases, missing a vblank can result in lower gpu utilization,
> > > > > > > > > > when really we want to go in the opposite direction and boost gpu freq.
> > > > > > > > > > The boost callback gives some feedback to the fence signaler that we
> > > > > > > > > > are missing deadlines, so it can take this into account in it's freq/
> > > > > > > > > > utilization calculations.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > > > > > > > ---
> > > > > > > > > >       include/linux/dma-fence.h | 26 ++++++++++++++++++++++++++
> > > > > > > > > >       1 file changed, 26 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > > > > > > > > > index 9f12efaaa93a..172702521acc 100644
> > > > > > > > > > --- a/include/linux/dma-fence.h
> > > > > > > > > > +++ b/include/linux/dma-fence.h
> > > > > > > > > > @@ -231,6 +231,17 @@ struct dma_fence_ops {
> > > > > > > > > >           signed long (*wait)(struct dma_fence *fence,
> > > > > > > > > >                               bool intr, signed long timeout);
> > > > > > > > > > 
> > > > > > > > > > +     /**
> > > > > > > > > > +      * @boost:
> > > > > > > > > > +      *
> > > > > > > > > > +      * Optional callback, to indicate that a fence waiter missed a deadline.
> > > > > > > > > > +      * This can serve as a signal that (if possible) whatever signals the
> > > > > > > > > > +      * fence should boost it's clocks.
> > > > > > > > > > +      *
> > > > > > > > > > +      * This can be called in any context that can call dma_fence_wait().
> > > > > > > > > > +      */
> > > > > > > > > > +     void (*boost)(struct dma_fence *fence);
> > > > > > > > > > +
> > > > > > > > > >           /**
> > > > > > > > > >            * @release:
> > > > > > > > > >            *
> > > > > > > > > > @@ -586,6 +597,21 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr)
> > > > > > > > > >           return ret < 0 ? ret : 0;
> > > > > > > > > >       }
> > > > > > > > > > 
> > > > > > > > > > +/**
> > > > > > > > > > + * dma_fence_boost - hint from waiter that it missed a deadline
> > > > > > > > > > + *
> > > > > > > > > > + * @fence: the fence that caused the missed deadline
> > > > > > > > > > + *
> > > > > > > > > > + * This function gives a hint from a fence waiter that a deadline was
> > > > > > > > > > + * missed, so that the fence signaler can factor this in to device
> > > > > > > > > > + * power state decisions
> > > > > > > > > > + */
> > > > > > > > > > +static inline void dma_fence_boost(struct dma_fence *fence)
> > > > > > > > > > +{
> > > > > > > > > > +     if (fence->ops->boost)
> > > > > > > > > > +             fence->ops->boost(fence);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >       struct dma_fence *dma_fence_get_stub(void);
> > > > > > > > > >       u64 dma_fence_context_alloc(unsigned num);
> > > > > > > > > > 
> > > > > > _______________________________________________
> > > > > > Linaro-mm-sig mailing list
> > > > > > Linaro-mm-sig@xxxxxxxxxxxxxxxx
> > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linaro.org%2Fmailman%2Flistinfo%2Flinaro-mm-sig&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7Ce4f3688b832842c4236e08d91bb1e148%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637571273080820910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=lOOKD4J4h7byys2ifx0Ibn5vVr9gwZGGGsgrNmaymc4%3D&amp;reserved=0
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch