Re: [RFC] Add DMA_RESV_USAGE flags

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 17, 2021 at 9:38 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 17.05.21 um 17:04 schrieb Daniel Vetter:
> > On Mon, May 17, 2021 at 04:11:18PM +0200, Christian König wrote:
> >> We had a long outstanding problem in amdgpu that buffers exported to
> >> user drivers by DMA-buf serialize all command submissions using them.
> >>
> >> In other words we can't compose the buffer with different engines and
> >> then send it to another driver for display further processing.
> >>
> >> This was added to work around the fact that i915 didn't wanted to wait
> >> for shared fences in the dma_resv objects before displaying a buffer.
> >>
> >> Since this problem is now causing issues with Vulkan we need to find a
> >> better solution for that.
> >>
> >> The patch set here tries to do this by adding an usage flag to the
> >> shared fences noting when and how they should participate in implicit
> >> synchronization.
> > So the way this is fixed in every other vulkan driver is that vulkan
> > userspace sets flags in the CS ioctl when it wants to synchronize with
> > implicit sync. This gets you mostly there. Last time I checked amdgpu
> > isn't doing this, and yes that's broken.
>
> And exactly that is a really bad approach as far as I can see. The
> Vulkan stack on top simply doesn't know when to set this flag during CS.

Adding Jason for the Vulkan side of things, because this isn't how I
understand this works.

But purely form a kernel pov your patches are sketchy for two reasons:

- we reinstate the amdgpu special case of not setting exclusive fences

- you only fix the single special case of i915 display, nothing else

That's not how a cross driver interface works. And if you'd do this
properly, you'd be back to all the same sync fun you've orignally had,
with all the same fallout.

> That's also the reason the Valve guys came up with a solution where each
> BO gets a flag for explicit sync, but that only works for exports and
> not for imports.
>
> > I915 and iirc msm has explicit flags for this, panfrost was designed to
> > support this correctly from the start (also with flags I think). That's at
> > least what I remember from all the discussions at XDC and #dri-devel, but
> > didn't check the code again to give you the list of uapi flags you need
> > for each driver.
> >
> > The other piece is making sure you're only picking up implicit fences when
> > you should, and not any later ones, for which Jason has a solution:
> >
> > https://lore.kernel.org/dri-devel/20210317221940.2146688-1-jason@jlekstrand.net/
>
> Yes, I helped with that as well. But I think that this is just another
> workaround without really addressing the underlying problem.
>
> > If amdgpu isn't using those, then you will suffer from
> > over-synchronization in vulkan and pay a price. The entire point of vulkan
> > is that you pick up sync points very explicitly, and we also need to have
> > very explicit uapi for userspace to pick up/set the implicit fences.
> >
> > Trying to paper over this with more implicit magic is imo just wrong, and
> > definitely not the long term explicit sync model we want.
>
> I completely disagree.
>
> In my opinion the implicit sync model we have for dma_resv currently is
> just not well designed at all, since it always requires cooperation from
> userspace.
>
> In other words you need to know when to enable implicit sync in
> userspace and that information is simply not present all of the time.
>
> What we have done here is just keeping the old reader/writer flags i915,
> radeon and nouveau once had and pushed that out to everybody else making
> the assumption that everybody would follow that without documenting the
> actual rules of engagement you need to follow here.
>
> That was a really big mistake and we should try to fix that sooner or
> later. The only other clean alternative I see is to use a flag on the
> exporter to tell the importer if it should sync to shared fences or not.
>
> Additional to that I'm perfectly fine with implicit sync. Explicit sync
> certainly has some use cases as well, but I don't see it as an absolute
> advantage over the implicit model.

Ok this stops making sense. Somehow you claim userspace doesn't know
when to sync, but somehow the kernel does? By guessing, and getting it
wrong mostly, except for the one case that you benchmarked?

Aside from silly userspace which exports a buffer to a dma-buf, but
then never imports it anywhere else, there isn't a case I know of
where the kernel actually knows more than userspace. But there's lots
of cases where the kernel definitely knows less, especially if
userspace doesn't tell it about what's going on with each rendering
and buffer.

So here's the 2 things you need to make this work like every other driver:

1. A way to set the explicit fence on a buffer. CS ioctl is perfectly
fine, but also can be seperate. Userspace uses this only on a) shared
buffers b) when there's a flush/swap on that shared buffer. Not when
rendering any of the interim stuff, that only leads to oversync.
Anything non-shared is handled explicitly in userspace (at least for
modern-ish drivers). This is the only thing that ever sets an
exclusive fence (aside from ttm moving buffers around ofc).

2. A way to sync with the implicit fences, either all of them (for
upcoming write access) or just the write fence (for read access). At
first we thought it's good enough to do this in the CS ioctl, but
that's a wee bit too late, hence the patches from Jason. My
understanding is that vulkan converts this into an vk syncobj/fence of
some sorts, so really can't make this more explicit and intentional
than that.

None of this is something the kernel has the slightest idea about when
it happens, so you have to have explicit uapi for it. Trying to fake
it in the kernel just doesn't work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch