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 <paul@crapouillou.net>
---
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
+ */
+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);
+
+/**
+ * @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.0On 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 <paul@crapouillou.net>
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
Am 19.01.24 um 15:13 schrieb Paul Cercueil:
> 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.
As Daniel already noted as well this is a complete no-go from the
DMA-buf design point of view.
Regards,
Christian.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>
> ---
> 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
> + */
> +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);
> +
> +/**
> + * @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);
Hi Christian,
Le lundi 22 janvier 2024 à 11:35 +0100, Christian König a écrit :
> Am 19.01.24 um 15:13 schrieb Paul Cercueil:
> > 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.
>
> As Daniel already noted as well this is a complete no-go from the
> DMA-buf design point of view.
What do you mean "as Daniel already noted"? It was him who suggested
this.
>
> Regards,
> Christian.
Cheers,
-Paul
>
> >
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >
> > ---
> > 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
> > + */
> > +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);
> > +
> > +/**
> > + * @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);
>
Am 22.01.24 um 12:01 schrieb Paul Cercueil:
> Hi Christian,
>
> Le lundi 22 janvier 2024 à 11:35 +0100, Christian König a écrit :
>> Am 19.01.24 um 15:13 schrieb Paul Cercueil:
>>> 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.
>> As Daniel already noted as well this is a complete no-go from the
>> DMA-buf design point of view.
> What do you mean "as Daniel already noted"? It was him who suggested
> this.
Sorry, I haven't fully catched up to the discussion then.
In general DMA-buf is build around the idea that the data can be
accessed coherently by the involved devices.
Having a begin/end of access for devices was brought up multiple times
but so far rejected for good reasons.
That an exporter has to call extra functions to access his own buffers
is a complete no-go for the design since this forces exporters into
doing extra steps for allowing importers to access their data.
That in turn is pretty much un-testable unless you have every possible
importer around while testing the exporter.
Regards,
Christian.
>
>> Regards,
>> Christian.
> Cheers,
> -Paul
>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>
>>> ---
>>> 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
>>> + */
>>> +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);
>>> +
>>> +/**
>>> + * @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);
Hi Christian,
Le lundi 22 janvier 2024 à 14:41 +0100, Christian König a écrit :
> Am 22.01.24 um 12:01 schrieb Paul Cercueil:
> > Hi Christian,
> >
> > Le lundi 22 janvier 2024 à 11:35 +0100, Christian König a écrit :
> > > Am 19.01.24 um 15:13 schrieb Paul Cercueil:
> > > > 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.
> > > As Daniel already noted as well this is a complete no-go from the
> > > DMA-buf design point of view.
> > What do you mean "as Daniel already noted"? It was him who
> > suggested
> > this.
>
> Sorry, I haven't fully catched up to the discussion then.
>
> In general DMA-buf is build around the idea that the data can be
> accessed coherently by the involved devices.
>
> Having a begin/end of access for devices was brought up multiple
> times
> but so far rejected for good reasons.
I would argue that if it was brought up multiple times, then there are
also good reasons to support such a mechanism.
> That an exporter has to call extra functions to access his own
> buffers
> is a complete no-go for the design since this forces exporters into
> doing extra steps for allowing importers to access their data.
Then what about we add these dma_buf_{begin,end}_access(), with only
implementations for "dumb" exporters e.g. udmabuf or the dmabuf heaps?
And only importers (who cache the mapping and actually care about non-
coherency) would have to call these.
At the very least, is there a way to check that "the data can be
accessed coherently by the involved devices"? So that my importer can
EPERM if there is no coherency vs. a device that's already attached.
Cheers,
-Paul
> That in turn is pretty much un-testable unless you have every
> possible
> importer around while testing the exporter.
>
> Regards,
> Christian.
>
> >
> > > Regards,
> > > Christian.
> > Cheers,
> > -Paul
> >
> > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > >
> > > > ---
> > > > 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
> > > > + */
> > > > +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);
> > > > +
> > > > +/**
> > > > + * @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);
>
Am 23.01.24 um 11:10 schrieb Paul Cercueil:
> Hi Christian,
>
> Le lundi 22 janvier 2024 à 14:41 +0100, Christian König a écrit :
>> Am 22.01.24 um 12:01 schrieb Paul Cercueil:
>>> Hi Christian,
>>>
>>> Le lundi 22 janvier 2024 à 11:35 +0100, Christian König a écrit :
>>>> Am 19.01.24 um 15:13 schrieb Paul Cercueil:
>>>>> 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.
>>>> As Daniel already noted as well this is a complete no-go from the
>>>> DMA-buf design point of view.
>>> What do you mean "as Daniel already noted"? It was him who
>>> suggested
>>> this.
>> Sorry, I haven't fully catched up to the discussion then.
>>
>> In general DMA-buf is build around the idea that the data can be
>> accessed coherently by the involved devices.
>>
>> Having a begin/end of access for devices was brought up multiple
>> times
>> but so far rejected for good reasons.
> I would argue that if it was brought up multiple times, then there are
> also good reasons to support such a mechanism.
>
>> That an exporter has to call extra functions to access his own
>> buffers
>> is a complete no-go for the design since this forces exporters into
>> doing extra steps for allowing importers to access their data.
> Then what about we add these dma_buf_{begin,end}_access(), with only
> implementations for "dumb" exporters e.g. udmabuf or the dmabuf heaps?
> And only importers (who cache the mapping and actually care about non-
> coherency) would have to call these.
No, the problem is still that you would have to change all importers to
mandatory use dma_buf_begin/end.
But going a step back caching the mapping is irrelevant for coherency.
Even if you don't cache the mapping you don't get coherency.
In other words exporters are not require to call sync_to_cpu or
sync_to_device when you create a mapping.
What exactly is your use case here? And why does coherency matters?
> At the very least, is there a way to check that "the data can be
> accessed coherently by the involved devices"? So that my importer can
> EPERM if there is no coherency vs. a device that's already attached.
Yeah, there is functionality for this in the DMA subsystem. I've once
created prototype patches for enforcing the same coherency approach
between importer and exporter, but we never got around to upstream them.
>
> Cheers,
> -Paul
>
>> That in turn is pretty much un-testable unless you have every
>> possible
>> importer around while testing the exporter.
>>
>> Regards,
>> Christian.
>>
>>>> Regards,
>>>> Christian.
>>> Cheers,
>>> -Paul
>>>
>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>>
>>>>> ---
>>>>> 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
>>>>> + */
>>>>> +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);
>>>>> +
>>>>> +/**
>>>>> + * @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);
Le mardi 23 janvier 2024 à 12:52 +0100, Christian König a écrit :
> Am 23.01.24 um 11:10 schrieb Paul Cercueil:
> > Hi Christian,
> >
> > Le lundi 22 janvier 2024 à 14:41 +0100, Christian König a écrit :
> > > Am 22.01.24 um 12:01 schrieb Paul Cercueil:
> > > > Hi Christian,
> > > >
> > > > Le lundi 22 janvier 2024 à 11:35 +0100, Christian König a
> > > > écrit :
> > > > > Am 19.01.24 um 15:13 schrieb Paul Cercueil:
> > > > > > 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.
> > > > > As Daniel already noted as well this is a complete no-go from
> > > > > the
> > > > > DMA-buf design point of view.
> > > > What do you mean "as Daniel already noted"? It was him who
> > > > suggested
> > > > this.
> > > Sorry, I haven't fully catched up to the discussion then.
> > >
> > > In general DMA-buf is build around the idea that the data can be
> > > accessed coherently by the involved devices.
> > >
> > > Having a begin/end of access for devices was brought up multiple
> > > times
> > > but so far rejected for good reasons.
> > I would argue that if it was brought up multiple times, then there
> > are
> > also good reasons to support such a mechanism.
> >
> > > That an exporter has to call extra functions to access his own
> > > buffers
> > > is a complete no-go for the design since this forces exporters
> > > into
> > > doing extra steps for allowing importers to access their data.
> > Then what about we add these dma_buf_{begin,end}_access(), with
> > only
> > implementations for "dumb" exporters e.g. udmabuf or the dmabuf
> > heaps?
> > And only importers (who cache the mapping and actually care about
> > non-
> > coherency) would have to call these.
>
> No, the problem is still that you would have to change all importers
> to
> mandatory use dma_buf_begin/end.
>
> But going a step back caching the mapping is irrelevant for
> coherency.
> Even if you don't cache the mapping you don't get coherency.
You actually do - at least with udmabuf, as in that case
dma_buf_map_attachment() / dma_buf_unmap_attachment() will handle cache
coherency when the SGs are mapped/unmapped.
The problem was then that dma_buf_unmap_attachment cannot be called
before the dma_fence is signaled, and calling it after is already too
late (because the fence would be signaled before the data is sync'd).
Daniel / Sima suggested then that I cache the mapping and add new
functions to ensure cache coherency, which is what these patches are
about.
> In other words exporters are not require to call sync_to_cpu or
> sync_to_device when you create a mapping.
>
> What exactly is your use case here? And why does coherency matters?
My use-case is, I create DMABUFs with udmabuf, that I attach to
USB/functionfs with the interface introduced by this patchset. I attach
them to IIO with a similar interface (being upstreamed in parallel),
and transfer data from USB to IIO and vice-versa in a zero-copy
fashion.
This works perfectly fine as long as the USB and IIO hardware are
coherent between themselves, which is the case on most of our boards.
However I do have a board (with a Xilinx Ultrascale SoC) where it is
not the case, and cache flushes/sync are needed. So I was trying to
rework these new interfaces to work on that system too.
If this really is a no-no, then I am fine with the assumption that
devices sharing a DMABUF must be coherent between themselves; but
that's something that should probably be enforced rather than assumed.
(and I *think* there is a way to force coherency in the Ultrascale's
interconnect - we're investigating it)
Cheers,
-Paul
> > At the very least, is there a way to check that "the data can be
> > accessed coherently by the involved devices"? So that my importer
> > can
> > EPERM if there is no coherency vs. a device that's already
> > attached.
>
> Yeah, there is functionality for this in the DMA subsystem. I've once
> created prototype patches for enforcing the same coherency approach
> between importer and exporter, but we never got around to upstream
> them.
>
>
>
> >
> > Cheers,
> > -Paul
> >
> > > That in turn is pretty much un-testable unless you have every
> > > possible
> > > importer around while testing the exporter.
> > >
> > > Regards,
> > > Christian.
> > >
> > > > > Regards,
> > > > > Christian.
> > > > Cheers,
> > > > -Paul
> > > >
> > > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > > > >
> > > > > > ---
> > > > > > 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
> > > > > > + */
> > > > > > +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);
> > > > > > +
> > > > > > +/**
> > > > > > + * @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);
>
Hi Paul,
kernel test robot noticed the following build warnings:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus drm-misc/drm-misc-next lwn/docs-next linus/master v6.7 next-20240119]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Paul-Cercueil/dma-buf-Add-dma_buf_-begin-end-_access/20240119-221604
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20240119141402.44262-2-paul%40crapouillou.net
patch subject: [PATCH v5 1/6] dma-buf: Add dma_buf_{begin,end}_access()
config: arm-randconfig-001-20240120 (https://download.01.org/0day-ci/archive/20240121/202401210406.YYgVcAC1-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project d92ce344bf641e6bb025b41b3f1a77dd25e2b3e9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240121/202401210406.YYgVcAC1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401210406.YYgVcAC1-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/dma-buf/dma-buf.c:1608: warning: Cannot understand * @dma_buf_begin_access - Call before any hardware access from/to the DMABUF
on line 1608 - I thought it was a doc line
>> drivers/dma-buf/dma-buf.c:1640: warning: Cannot understand * @dma_buf_end_access - Call after any hardware access from/to the DMABUF
on line 1640 - I thought it was a doc line
vim +1608 drivers/dma-buf/dma-buf.c
1606
1607 /**
> 1608 * @dma_buf_begin_access - Call before any hardware access from/to the DMABUF
1609 * @attach: [in] attachment used for hardware access
1610 * @sg_table: [in] scatterlist used for the DMA transfer
1611 * @direction: [in] direction of DMA transfer
1612 */
1613 int dma_buf_begin_access(struct dma_buf_attachment *attach,
1614 struct sg_table *sgt, enum dma_data_direction dir)
1615 {
1616 struct dma_buf *dmabuf;
1617 bool cookie;
1618 int ret;
1619
1620 if (WARN_ON(!attach))
1621 return -EINVAL;
1622
1623 dmabuf = attach->dmabuf;
1624
1625 if (!dmabuf->ops->begin_access)
1626 return 0;
1627
1628 cookie = dma_fence_begin_signalling();
1629 ret = dmabuf->ops->begin_access(attach, sgt, dir);
1630 dma_fence_end_signalling(cookie);
1631
1632 if (WARN_ON_ONCE(ret))
1633 return ret;
1634
1635 return 0;
1636 }
1637 EXPORT_SYMBOL_NS_GPL(dma_buf_begin_access, DMA_BUF);
1638
1639 /**
> 1640 * @dma_buf_end_access - Call after any hardware access from/to the DMABUF
1641 * @attach: [in] attachment used for hardware access
1642 * @sg_table: [in] scatterlist used for the DMA transfer
1643 * @direction: [in] direction of DMA transfer
1644 */
1645 int dma_buf_end_access(struct dma_buf_attachment *attach,
1646 struct sg_table *sgt, enum dma_data_direction dir)
1647 {
1648 struct dma_buf *dmabuf;
1649 bool cookie;
1650 int ret;
1651
1652 if (WARN_ON(!attach))
1653 return -EINVAL;
1654
1655 dmabuf = attach->dmabuf;
1656
1657 if (!dmabuf->ops->end_access)
1658 return 0;
1659
1660 cookie = dma_fence_begin_signalling();
1661 ret = dmabuf->ops->end_access(attach, sgt, dir);
1662 dma_fence_end_signalling(cookie);
1663
1664 if (WARN_ON_ONCE(ret))
1665 return ret;
1666
1667 return 0;
1668 }
1669 EXPORT_SYMBOL_NS_GPL(dma_buf_end_access, DMA_BUF);
1670
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2025 Red Hat, Inc.