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

From: Daniel Vetter <daniel@ffwll.ch>
To: Jason Ekstrand <jason@jlekstrand.net>
Cc: "Daniel Vetter" <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org,
	"Christian König" <christian.koenig@amd.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 4/7] dma-buf: Document DMA_BUF_IOCTL_SYNC
Date: Thu, 27 May 2021 12:38:07 +0200	[thread overview]
Message-ID: <YK92j8b8SZ61KTCO@phenom.ffwll.local> (raw)
In-Reply-To: <20210525211753.1086069-5-jason@jlekstrand.net>

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@jlekstrand.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Sumit Semwal <sumit.semwal@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).

> + */
>  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.

> +	 *   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/

> +	 *   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@ffwll.ch>

>  	__u64 flags;
>  };
>  
> -- 
> 2.31.1
> 

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