Message-ID: <CAKMK7uE34xvL8-9TS1i8FtrFiaw5A4ZeTSaOvHnn0UGp1dj4MQ@mail.gmail.com>
Date:   Mon, 15 Mar 2021 22:30:31 +0100
From:   Daniel Vetter <daniel.vetter@...ll.ch>
To:     Jason Ekstrand <jason@...kstrand.net>
Cc:     Michel Dänzer <michel@...nzer.net>,
        Chenbo Feng <fengc@...gle.com>,
        Daniel Stone <daniels@...labora.com>,
        James Jones <jajones@...dia.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Hackmann <ghackmann@...gle.com>,
        "moderated list:DMA BUFFER SHARING FRAMEWORK" 
        <linaro-mm-sig@...ts.linaro.org>,
        Kristian Høgsberg <hoegsberg@...gle.com>,
        Maling list - DRI developers 
        <dri-devel@...ts.freedesktop.org>,
        Jesse Hall <jessehall@...gle.com>,
        Dave Airlie <airlied@...hat.com>,
        Christian König <christian.koenig@....com>,
        "open list:DMA BUFFER SHARING FRAMEWORK" 
        <linux-media@...r.kernel.org>
Subject: Re: [PATCH 3/3] RFC: dma-buf: Add an API for importing and exporting
 sync files (v5)

On Mon, Mar 15, 2021 at 10:11 PM Jason Ekstrand <jason@...kstrand.net> wrote:
>
> On Wed, Sep 30, 2020 at 4:55 AM Daniel Vetter <daniel@...ll.ch> wrote:
> >
> > On Wed, Sep 30, 2020 at 11:39:06AM +0200, Michel Dänzer wrote:
> > > On 2020-03-17 10:21 p.m., Jason Ekstrand wrote:
> > > > Explicit synchronization is the future.  At least, that seems to be what
> > > > most userspace APIs are agreeing on at this point.  However, most of our
> > > > Linux APIs (both userspace and kernel UAPI) are currently built around
> > > > implicit synchronization with dma-buf.  While work is ongoing to change
> > > > many of the userspace APIs and protocols to an explicit synchronization
> > > > model, switching over piecemeal is difficult due to the number of
> > > > potential components involved.  On the kernel side, many drivers use
> > > > dma-buf including GPU (3D/compute), display, v4l, and others.  In
> > > > userspace, we have X11, several Wayland compositors, 3D drivers, compute
> > > > drivers (OpenCL etc.), media encode/decode, and the list goes on.
> > > >
> > > > This patch provides a path forward by allowing userspace to manually
> > > > manage the fences attached to a dma-buf.  Alternatively, one can think
> > > > of this as making dma-buf's implicit synchronization simply a carrier
> > > > for an explicit fence.  This is accomplished by adding two IOCTLs to
> > > > dma-buf for importing and exporting a sync file to/from the dma-buf.
> > > > This way a userspace component which is uses explicit synchronization,
> > > > such as a Vulkan driver, can manually set the write fence on a buffer
> > > > before handing it off to an implicitly synchronized component such as a
> > > > Wayland compositor or video encoder.  In this way, each of the different
> > > > components can be upgraded to an explicit synchronization model one at a
> > > > time as long as the userspace pieces connecting them are aware of it and
> > > > import/export fences at the right times.
> > > >
> > > > There is a potential race condition with this API if userspace is not
> > > > careful.  A typical use case for implicit synchronization is to wait for
> > > > the dma-buf to be ready, use it, and then signal it for some other
> > > > component.  Because a sync_file cannot be created until it is guaranteed
> > > > to complete in finite time, userspace can only signal the dma-buf after
> > > > it has already submitted the work which uses it to the kernel and has
> > > > received a sync_file back.  There is no way to atomically submit a
> > > > wait-use-signal operation.  This is not, however, really a problem with
> > > > this API so much as it is a problem with explicit synchronization
> > > > itself.  The way this is typically handled is to have very explicit
> > > > ownership transfer points in the API or protocol which ensure that only
> > > > one component is using it at any given time.  Both X11 (via the PRESENT
> > > > extension) and Wayland provide such ownership transfer points via
> > > > explicit present and idle messages.
> > > >
> > > > The decision was intentionally made in this patch to make the import and
> > > > export operations IOCTLs on the dma-buf itself rather than as a DRM
> > > > IOCTL.  This makes it the import/export operation universal across all
> > > > components which use dma-buf including GPU, display, v4l, and others.
> > > > It also means that a userspace component can do the import/export
> > > > without access to the DRM fd which may be tricky to get in cases where
> > > > the client communicates with DRM via a userspace API such as OpenGL or
> > > > Vulkan.  At a future date we may choose to add direct import/export APIs
> > > > to components such as drm_syncobj to avoid allocating a file descriptor
> > > > and going through two ioctls.  However, that seems to be something of a
> > > > micro-optimization as import/export operations are likely to happen at a
> > > > rate of a few per frame of rendered or decoded video.
> > > >
> > > > v2 (Jason Ekstrand):
> > > >   - Use a wrapper dma_fence_array of all fences including the new one
> > > >     when importing an exclusive fence.
> > > >
> > > > v3 (Jason Ekstrand):
> > > >   - Lock around setting shared fences as well as exclusive
> > > >   - Mark SIGNAL_SYNC_FILE as a read-write ioctl.
> > > >   - Initialize ret to 0 in dma_buf_wait_sync_file
> > > >
> > > > v4 (Jason Ekstrand):
> > > >   - Use the new dma_resv_get_singleton helper
> > > >
> > > > v5 (Jason Ekstrand):
> > > >   - Rename the IOCTLs to import/export rather than wait/signal
> > > >   - Drop the WRITE flag and always get/set the exclusive fence
> > > >
> > > > Signed-off-by: Jason Ekstrand <jason@...kstrand.net>
> > >
> > > What's the status of this? DMA_BUF_IOCTL_EXPORT_SYNC_FILE would be useful
> > > for Wayland compositors to wait for client buffers to become ready without
> > > being prone to getting delayed by later HW access to them, so it would be
> > > nice to merge that at least (if DMA_BUF_IOCTL_IMPORT_SYNC_FILE is still
> > > controversial).
> >
> > I think the missing bits are just the usual stuff
> > - igt testcases
> > - userspace using the new ioctls
> > - review of the entire pile
> >
> > I don't think there's any fundamental objections aside from "no one ever
> > pushed this over the finish line".
>
> I just re-upped the patch series and you should have been on the CC
> for the cover letter.  The Mesa MR is here:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4037  I'm
> going to try and knock out an IGT test quick but I don't know how much
> is really practical to test in that environment besides a basic fuzz
> for "does the IOCTL return a sync file".

With vgem you should be able to control the interface fully, since
that allows you to control dma_fences we attach to stuff. Or at least
it has some of the ingredients, and a bunch of igts test it. E.g. we
have one that creates a fence with vgem and then checks (using crc)
that your display driver works correctly with the page flip.

> I've dropped all the sync_file import stuff in the latest version.  It
> would have been useful for testing but it's also where all the scary
> stuff lives and I'm no longer convinced it solves any real problems
> for userspace.

Yeah vgem should allow you to get at least some of that sorted.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists