[Intel-gfx] [PATCH 4/7] dma-buf: Document DMA_BUF_IOCTL_SYNC

[Jason Ekstrand]

On Thu, May 27, 2021 at 5:38 AM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Tue, May 25, 2021 at 04:17:50PM -0500, Jason Ekstrand wrote:
> > This adds a new "DMA Buffer ioctls" section to the dma-buf docs and adds
> > documentation for DMA_BUF_IOCTL_SYNC.
> >
> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Christian König <christian.koenig at amd.com>
> > Cc: Sumit Semwal <sumit.semwal at linaro.org>
>
> We're still missing the doc for the SET_NAME ioctl, but maybe Sumit can be
> motivated to fix that?
>
> > ---
> >  Documentation/driver-api/dma-buf.rst |  8 +++++++
> >  include/uapi/linux/dma-buf.h         | 32 +++++++++++++++++++++++++++-
> >  2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
> > index 7f37ec30d9fd7..784f84fe50a5e 100644
> > --- a/Documentation/driver-api/dma-buf.rst
> > +++ b/Documentation/driver-api/dma-buf.rst
> > @@ -88,6 +88,9 @@ consider though:
> >  - The DMA buffer FD is also pollable, see `Implicit Fence Poll Support`_ below for
> >    details.
> >
> > +- The DMA buffer FD also supports a few dma-buf-specific ioctls, see
> > +  `DMA Buffer ioctls`_ below for details.
> > +
> >  Basic Operation and Device DMA Access
> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > @@ -106,6 +109,11 @@ Implicit Fence Poll Support
> >  .. kernel-doc:: drivers/dma-buf/dma-buf.c
> >     :doc: implicit fence polling
> >
> > +DMA Buffer ioctls
> > +~~~~~~~~~~~~~~~~~
> > +
> > +.. kernel-doc:: include/uapi/linux/dma-buf.h
> > +
> >  Kernel Functions and Structures Reference
> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h
> > index 7f30393b92c3b..1f67ced853b14 100644
> > --- a/include/uapi/linux/dma-buf.h
> > +++ b/include/uapi/linux/dma-buf.h
> > @@ -22,8 +22,38 @@
> >
> >  #include <linux/types.h>
> >
> > -/* begin/end dma-buf functions used for userspace mmap. */
> > +/**
> > + * struct dma_buf_sync - Synchronize with CPU access.
> > + *
> > + * When a DMA buffer is accessed from the CPU via mmap, it is not always
> > + * possible to guarantee coherency between the CPU-visible map and underlying
> > + * memory.  To manage coherency, DMA_BUF_IOCTL_SYNC must be used to bracket
> > + * any CPU access to give the kernel the chance to shuffle memory around if
> > + * needed.
> > + *
> > + * Prior to accessing the map, the client should call DMA_BUF_IOCTL_SYNC
>
> s/should/must/
>
> > + * with DMA_BUF_SYNC_START and the appropriate read/write flags.  Once the
> > + * access is complete, the client should call DMA_BUF_IOCTL_SYNC with
> > + * DMA_BUF_SYNC_END and the same read/write flags.
>
> I think we should make it really clear here that this is _only_ for cache
> coherency, and that furthermore if you want coherency with gpu access you
> either need to use poll() for implicit sync (link to the relevant section)
> or handle explicit sync with sync_file (again link would be awesome).

I've added such a comment.  I encourage you to look at v2 which I'll
be sending shortly.  I'm not sure how to get the poll() reference to
hyperlink, though.

> > + */
> >  struct dma_buf_sync {
> > +     /**
> > +      * @flags: Set of access flags
> > +      *
> > +      * - DMA_BUF_SYNC_START: Indicates the start of a map access
>
> Bikeshed, but I think the item list format instead of bullet point list
> looks neater, e.g.  DOC: standard plane properties in drm_plane.c.

Yeah, that's better.

> > +      *   session.
> > +      *
> > +      * - DMA_BUF_SYNC_END: Indicates the end of a map access session.
> > +      *
> > +      * - DMA_BUF_SYNC_READ: Indicates that the mapped DMA buffer will
> > +      *   be read by the client via the CPU map.
> > +      *
> > +      * - DMA_BUF_SYNC_READ: Indicates that the mapped DMA buffer will
>
> s/READ/WRITE/

Oops.

> > +      *   be written by the client via the CPU map.
> > +      *
> > +      * - DMA_BUF_SYNC_RW: An alias for DMA_BUF_SYNC_READ |
> > +      *   DMA_BUF_SYNC_WRITE.
> > +      */
>
> With the nits addressed: Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Thanks!

> >       __u64 flags;
> >  };
> >
> > --
> > 2.31.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch