by Daniel Vetter

[permalink] [raw]

Subject: Re: [PATCH v5 1/6] dma-buf: Add dma_buf_{begin,end}_access()

On Fri, Jan 19, 2024 at 03:13:57PM +0100, Paul Cercueil wrote:
> These functions should be used by device drivers when they start and
> stop accessing the data of DMABUF. It allows DMABUF importers to cache
> the dma_buf_attachment while ensuring that the data they want to access
> is available for their device when the DMA transfers take place.
>
> Signed-off-by: Paul Cercueil <[email protected]>

Putting my detailed review comments here just so I don't have to remember
them any longer. We need to reach consensus on the big picture direction
first.

>
> ---
> v5: New patch
> ---
> drivers/dma-buf/dma-buf.c | 66 +++++++++++++++++++++++++++++++++++++++
> include/linux/dma-buf.h | 37 ++++++++++++++++++++++
> 2 files changed, 103 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 8fe5aa67b167..a8bab6c18fcd 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -830,6 +830,8 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
> * - dma_buf_mmap()
> * - dma_buf_begin_cpu_access()
> * - dma_buf_end_cpu_access()
> + * - dma_buf_begin_access()
> + * - dma_buf_end_access()
> * - dma_buf_map_attachment_unlocked()
> * - dma_buf_unmap_attachment_unlocked()
> * - dma_buf_vmap_unlocked()
> @@ -1602,6 +1604,70 @@ void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
> }
> EXPORT_SYMBOL_NS_GPL(dma_buf_vunmap_unlocked, DMA_BUF);
>
> +/**
> + * @dma_buf_begin_access - Call before any hardware access from/to the DMABUF
> + * @attach: [in] attachment used for hardware access
> + * @sg_table: [in] scatterlist used for the DMA transfer
> + * @direction: [in] direction of DMA transfer

I think for the kerneldoc would be good to point at the other function
here, explain why this might be needed and that for most reasonable
devices it's probably not, and link between the function pairs.

Also we need to document that dma_buf_map does an implied
dma_buf_begin_access (because dma_sg_map does an implied
dma_sg_sync_for_device) and vice versa for dma_buf_end_access. Which also
means that dma_buf_map/unmap should link to these functions in their
kerneldoc too.

Finally I think we should document here that it's ok to call these from
dma_fence signalling critical section and link to the relevant discussion
in the dma_fence docs for that.

> + */
> +int dma_buf_begin_access(struct dma_buf_attachment *attach,
> + struct sg_table *sgt, enum dma_data_direction dir)
> +{
> + struct dma_buf *dmabuf;
> + bool cookie;
> + int ret;
> +
> + if (WARN_ON(!attach))
> + return -EINVAL;
> +
> + dmabuf = attach->dmabuf;
> +
> + if (!dmabuf->ops->begin_access)
> + return 0;
> +
> + cookie = dma_fence_begin_signalling();
> + ret = dmabuf->ops->begin_access(attach, sgt, dir);
> + dma_fence_end_signalling(cookie);
> +
> + if (WARN_ON_ONCE(ret))
> + return ret;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(dma_buf_begin_access, DMA_BUF);

So explicit device side coherency management is not going to be very
compatible with dynamic buffer managament where the exporter can move the
buffer around. The reason for that is that for a dynamic exporter we cache
the sg mapping, which means any device-side coherency management which
dma_buf_map/unmap would do will not happen (since it's cached),
potentially breaking things for importers that rely on the assumption that
dma_buf_map/unmap already implies dma_buf_begin/end_device_access.

I think for now it's sufficient to put a WARN_ON(dma_buf_is_dymamic() &&
ops->begin|end_access) or similar into dma_buf_export and bail out with an
error to catch that.

Aside from the nits I do think this is roughly what we brievely discussed
well over a decade ago in the original dma-buf kickoff meeting at a linaro
connect in Budapest :-)

Cheers, Sima

> +
> +/**
> + * @dma_buf_end_access - Call after any hardware access from/to the DMABUF
> + * @attach: [in] attachment used for hardware access
> + * @sg_table: [in] scatterlist used for the DMA transfer
> + * @direction: [in] direction of DMA transfer
> + */
> +int dma_buf_end_access(struct dma_buf_attachment *attach,
> + struct sg_table *sgt, enum dma_data_direction dir)
> +{
> + struct dma_buf *dmabuf;
> + bool cookie;
> + int ret;
> +
> + if (WARN_ON(!attach))
> + return -EINVAL;
> +
> + dmabuf = attach->dmabuf;
> +
> + if (!dmabuf->ops->end_access)
> + return 0;
> +
> + cookie = dma_fence_begin_signalling();
> + ret = dmabuf->ops->end_access(attach, sgt, dir);
> + dma_fence_end_signalling(cookie);
> +
> + if (WARN_ON_ONCE(ret))
> + return ret;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(dma_buf_end_access, DMA_BUF);
> +
> #ifdef CONFIG_DEBUG_FS
> static int dma_buf_debug_show(struct seq_file *s, void *unused)
> {
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 8ff4add71f88..8ba612c7cc16 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -246,6 +246,38 @@ struct dma_buf_ops {
> */
> int (*end_cpu_access)(struct dma_buf *, enum dma_data_direction);
>
> + /**
> + * @begin_access:
> + *
> + * This is called from dma_buf_begin_access() when a device driver
> + * wants to access the data of the DMABUF. The exporter can use this
> + * to flush/sync the caches if needed.
> + *
> + * This callback is optional.
> + *
> + * Returns:
> + *
> + * 0 on success or a negative error code on failure.
> + */
> + int (*begin_access)(struct dma_buf_attachment *, struct sg_table *,
> + enum dma_data_direction);
> +
> + /**
> + * @end_access:
> + *
> + * This is called from dma_buf_end_access() when a device driver is
> + * done accessing the data of the DMABUF. The exporter can use this
> + * to flush/sync the caches if needed.
> + *
> + * This callback is optional.
> + *
> + * Returns:
> + *
> + * 0 on success or a negative error code on failure.
> + */
> + int (*end_access)(struct dma_buf_attachment *, struct sg_table *,
> + enum dma_data_direction);
> +
> /**
> * @mmap:
> *
> @@ -606,6 +638,11 @@ void dma_buf_detach(struct dma_buf *dmabuf,
> int dma_buf_pin(struct dma_buf_attachment *attach);
> void dma_buf_unpin(struct dma_buf_attachment *attach);
>
> +int dma_buf_begin_access(struct dma_buf_attachment *attach,
> + struct sg_table *sgt, enum dma_data_direction dir);
> +int dma_buf_end_access(struct dma_buf_attachment *attach,
> + struct sg_table *sgt, enum dma_data_direction dir);
> +
> struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info);
>
> int dma_buf_fd(struct dma_buf *dmabuf, int flags);
> --
> 2.43.0
>
>

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

2024-02-07 20:46:16

by Daniel Vetter

[permalink] [raw]

Subject: Re: [Linaro-mm-sig] [PATCH v5 2/6] dma-buf: udmabuf: Implement .{begin,end}_access

On Fri, Jan 19, 2024 at 03:13:58PM +0100, Paul Cercueil wrote:
> Implement .begin_access() and .end_access() callbacks.
>
> For now these functions will simply sync/flush the CPU cache when
> needed.
>
> Signed-off-by: Paul Cercueil <[email protected]>
>
> ---
> v5: New patch
> ---
> drivers/dma-buf/udmabuf.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index c40645999648..a87d89b58816 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -179,6 +179,31 @@ static int end_cpu_udmabuf(struct dma_buf *buf,
> return 0;
> }
>
> +static int begin_udmabuf(struct dma_buf_attachment *attach,
> + struct sg_table *sgt,
> + enum dma_data_direction dir)
> +{
> + struct dma_buf *buf = attach->dmabuf;
> + struct udmabuf *ubuf = buf->priv;
> + struct device *dev = ubuf->device->this_device;
> +
> + dma_sync_sg_for_device(dev, sgt->sgl, sg_nents(sgt->sgl), dir);

So one thing I've just wondered is whether we've made sure that this is
only doing cache coherency maintenance, and not swiotlb bounce buffer
copying. The latter would really not be suitable for dma-buf anymore I
think.

Not sure how to best check for that since it's all in the depths of the
dma-api code, but I guess the best way to really make sure is to disable
CONFIG_SWIOTLB. Otherwise I guess the way to absolutely make sure is to
trace swiotlb_sync_single_for_device/cpu.

It would be kinda neat if dma-buf.c code could make sure you never ever
get an swiotlb entry from a dma_buf_map_attachment call, but I don't think
we can enforce that. There's sg_dma_is_swiotlb, but that won't catch all
implementations, only the generic dma-iommu.c one.

Cheers, Sima

> + return 0;
> +}
> +
> +static int end_udmabuf(struct dma_buf_attachment *attach,
> + struct sg_table *sgt,
> + enum dma_data_direction dir)
> +{
> + struct dma_buf *buf = attach->dmabuf;
> + struct udmabuf *ubuf = buf->priv;
> + struct device *dev = ubuf->device->this_device;
> +
> + if (dir != DMA_TO_DEVICE)
> + dma_sync_sg_for_cpu(dev, sgt->sgl, sg_nents(sgt->sgl), dir);
> + return 0;
> +}
> +
> static const struct dma_buf_ops udmabuf_ops = {
> .cache_sgt_mapping = true,
> .map_dma_buf = map_udmabuf,
> @@ -189,6 +214,8 @@ static const struct dma_buf_ops udmabuf_ops = {
> .vunmap = vunmap_udmabuf,
> .begin_cpu_access = begin_cpu_udmabuf,
> .end_cpu_access = end_cpu_udmabuf,
> + .begin_access = begin_udmabuf,
> + .end_access = end_udmabuf,
> };
>
> #define SEALS_WANTED (F_SEAL_SHRINK)
> --
> 2.43.0
>
> _______________________________________________
> Linaro-mm-sig mailing list -- [email protected]
> To unsubscribe send an email to [email protected]

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