Re: [Intel-gfx] [PATCH 5/7] dma-buf: Add an API for exporting sync files (v11)

From: Jason Ekstrand <jason@jlekstrand.net>
To: "Christian König" <christian.koenig@amd.com>,
	"Maling list - DRI developers" <dri-devel@lists.freedesktop.org>,
	"Simon Ser" <contact@emersion.fr>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Intel GFX" <intel-gfx@lists.freedesktop.org>,
	"Daniel Stone" <daniels@collabora.com>
Subject: Re: [Intel-gfx] [PATCH 5/7] dma-buf: Add an API for exporting sync files (v11)
Date: Thu, 27 May 2021 10:39:09 -0500	[thread overview]
Message-ID: <CAOFGe96XX6Sg0htVExu0hv5bkq-++5+1tRG4Wb17PXb5aaCqEg@mail.gmail.com> (raw)
In-Reply-To: <fef50d81-399a-af09-1d13-de4db1b3fab8@amd.com>

On Thu, May 27, 2021 at 4:49 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 26.05.21 um 19:42 schrieb Jason Ekstrand:
> > On Wed, May 26, 2021 at 6:02 AM Christian König
> > <christian.koenig@amd.com> wrote:
> >> Regarding that, why do we actually use a syncfile and not a drm_syncobj
> >> here?
> > A sync file is a userspace handle to a dma_fence which is exactly what
> > we're grabbing from the dma-buf so it's a better match.  A syncobj, on
> > the other hand, is a container for fences.  If we really want it in a
> > syncobj (which we may), we can use another ioctl to stuff it in there.
> > The only thing making this work in terms of syncobj would save us is a
> > little ioctl overhead.  In my Mesa patches, we do stuff it in a
> > syncobj but, instead of acting on the syncobj directly, we go through
> > vkSemaphoreImportFd() which is way more convenient for generic WSI
> > code.
>
> Yeah, that is a really good argument.
>
> > It also works nicer because a sync_file is a pretty generic construct
> > but a syncobj is a DRM construct.  This lets us do the export in an
> > entirely driver-generic way without even having access to a DRM fd.
> > It all happens as an ioctl on the dma-buf.
>
> Well that's an even better argument and I think the killer argument here.

Cool.

> We should probably mention that on the commit message as a single
> sentence somewhere.

Happy to do so.  How does this sound:

By making this an ioctl on the dma-buf itself, it allows this new
functionality to be used in an entirely driver-agnostic way without
having access to a DRM fd. This makes it ideal for use in
driver-generic code in Mesa or in a client such as a compositor where
the DRM fd may be hard to reach.

> BTW: You replied exclusively to me. Was that intentionally? I don't
> think so :)

Oops...  I've re-added the whole lot in this reply.

--Jason

> Regards,
> Christian.
>
> >
> > On Wed, May 26, 2021 at 8:23 AM Christian König
> > <christian.koenig@amd.com> wrote:
> >
> >> Am 26.05.21 um 15:12 schrieb Daniel Stone:
> >>> On Wed, 26 May 2021 at 13:46, Christian König <christian.koenig@amd.com> wrote:
> >>>> Am 26.05.21 um 13:31 schrieb Daniel Stone:
> >>>>> How would we insert a syncobj+val into a resv though? Like, if we pass
> >>>>> an unmaterialised syncobj+val here to insert into the resv, then an
> >>>>> implicit-only media user (or KMS) goes to sync against the resv, what
> >>>>> happens?
> >>>> Well this is for exporting, not importing. So we don't need to worry
> >>>> about that.
> >>>>
> >>>> It's just my thinking because the drm_syncobj is the backing object on
> >>>> VkSemaphore implementations these days, isn't it?
> >>> Yeah, I can see that to an extent. But then binary vs. timeline
> >>> syncobjs are very different in use (which is unfortunate tbh), and
> >>> then we have an asymmetry between syncobj export & sync_file import.
> >>>
> >>> You're right that we do want a syncobj though.
> > I'm not convinced.  Some people seem to think that we have a direct
> > progression of superiority: sync_file < syncobj < timeline syncobj.  I
> > don't think this is true.  They each serve different purposes.  A
> > sync_file is a handle to a single immutable fence operation (a
> > dma_fence*).  A syncobj is a container for a fence and is, by
> > definition, mutable (a dma_fence**).  A timeline syncobj is a
> > construct that lets us implement VK_KHR_timeline_semaphore with only
> > immutable finite-time (not future) fences.
> >
> >  From a WSI protocol PoV, it's sometimes nicer to pass a container
> > object once and then pass a serial or a simple "I just updated it"
> > signal every time instead of a new FD.  It certainly makes all the
> > "who closes this, again?" semantics easier.  But we shouldn't think of
> > syncobj as being better than sync_file.  With binary syncobj, it
> > really is the difference between passing a dma_fence* vs. a
> > dma_fence**.  Sometimes you want one and sometimes you want the other.
> > The real pity, IMO, is that the uAPI is scattered everywhere.
> >
> >>> This is probably not
> >>> practical due to smashing uAPI to bits, but if we could wind the clock
> >>> back a couple of years, I suspect the interface we want is that export
> >>> can either export a sync_file or a binary syncobj, and further that
> >>> binary syncobjs could transparently act as timeline semaphores by
> >>> mapping any value (either wait or signal) to the binary signal. In
> >>> hindsight, we should probably just never have had binary syncobj. Oh
> >>> well.
> >> Well the later is the case IIRC. Don't ask me for the detail semantics,
> >> but in general the drm_syncobj in timeline mode is compatible to the
> >> binary mode.
> >>
> >> The sync_file is also import/exportable to a certain drm_syncobj
> >> timeline point (or as binary signal). So no big deal, we are all
> >> compatible here :)
> >>
> >> I just thought that it might be more appropriate to return a drm_syncobj
> >> directly instead of a sync_file.
> > Maybe.  I'm not convinced that's better.  In the current Mesa WSI
> > code, it'd actually be quite a pain.
> >
> > --Jason
>