[PATCH v7 0/6] iio: new DMABUF based API

[PATCH v7 0/6] iio: new DMABUF based API

[Nuno Sa]

Hi Jonathan, likely you're wondering why I'm sending v7. Well, to be
honest, we're hoping to get this merged this for the 6.9 merge window.
Main reason is because the USB part is already in (so it would be nice
to get the whole thing in). Moreover, the changes asked in v6 were simple
(even though I'm not quite sure in one of them) and Paul has no access to
it's laptop so he can't send v7 himself. So he kind of said/asked for me to do it.

v6:
 * https://lore.kernel.org/linux-iio/20240129170201.133785-1-paul@crapouillou.net/

v7:
 - Patch 1
  * Renamed *device_prep_slave_dma_vec() -> device_prep_peripheral_dma_vec();
  * Added a new flag parameter to the function as agreed between Paul
    and Vinod. I renamed the first parameter to prep_flags as it's supposed to
    be used (I think) with enum dma_ctrl_flags. I'm not really sure how that API
    can grow but I was thinking in just having a bool cyclic parameter (as the
    first intention of the flags is to support cyclic transfers) but ended up
    "respecting" the previously agreed approach.
- Patch 2
  * Adapted patch for the changes made in patch 1.
- Patch 5
  * Adapted patch for the changes made in patch 1.

Patchset based on next-20240223.

---
Paul Cercueil (6):
      dmaengine: Add API function dmaengine_prep_peripheral_dma_vec()
      dmaengine: dma-axi-dmac: Implement device_prep_peripheral_dma_vec
      iio: core: Add new DMABUF interface infrastructure
      iio: buffer-dma: Enable support for DMABUFs
      iio: buffer-dmaengine: Support new DMABUF based userspace API
      Documentation: iio: Document high-speed DMABUF based API

 Documentation/iio/dmabuf_api.rst                   |  54 +++
 Documentation/iio/index.rst                        |   2 +
 drivers/dma/dma-axi-dmac.c                         |  40 ++
 drivers/iio/buffer/industrialio-buffer-dma.c       | 181 +++++++-
 drivers/iio/buffer/industrialio-buffer-dmaengine.c |  59 ++-
 drivers/iio/industrialio-buffer.c                  | 462 +++++++++++++++++++++
 include/linux/dmaengine.h                          |  27 ++
 include/linux/iio/buffer-dma.h                     |  31 ++
 include/linux/iio/buffer_impl.h                    |  33 ++
 include/uapi/linux/iio/buffer.h                    |  22 +
 10 files changed, 894 insertions(+), 17 deletions(-)
---
base-commit: 33e1d31873f87d119e5120b88cd350efa68ef276
change-id: 20240223-iio-dmabuf-5ee0530195ca
--

Thanks!
- Nuno Sá

[PATCH v7 1/6] dmaengine: Add API function dmaengine_prep_peripheral_dma_vec()

[Nuno Sa]

From: Paul Cercueil <paul@crapouillou.net>

This function can be used to initiate a scatter-gather DMA transfer,
where the address and size of each segment is located in one entry of
the dma_vec array.

The major difference with dmaengine_prep_slave_sg() is that it supports
specifying the lengths of each DMA transfer; as trying to override the
length of the transfer with dmaengine_prep_slave_sg() is a very tedious
process. The introduction of a new API function is also justified by the
fact that scatterlists are on their way out.

Note that dmaengine_prep_interleaved_dma() is not helpful either in that
case, as it assumes that the address of each segment will be higher than
the one of the previous segment, which we just cannot guarantee in case
of a scatter-gather transfer.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/dmaengine.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 752dbde4cec1..856df8cd9a4e 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -160,6 +160,16 @@ struct dma_interleaved_template {
 	struct data_chunk sgl[];
 };
 
+/**
+ * struct dma_vec - DMA vector
+ * @addr: Bus address of the start of the vector
+ * @len: Length in bytes of the DMA vector
+ */
+struct dma_vec {
+	dma_addr_t addr;
+	size_t len;
+};
+
 /**
  * enum dma_ctrl_flags - DMA flags to augment operation preparation,
  *  control completion, and communicate status.
@@ -910,6 +920,10 @@ struct dma_device {
 	struct dma_async_tx_descriptor *(*device_prep_dma_interrupt)(
 		struct dma_chan *chan, unsigned long flags);
 
+	struct dma_async_tx_descriptor *(*device_prep_peripheral_dma_vec)(
+		struct dma_chan *chan, const struct dma_vec *vecs,
+		size_t nents, enum dma_transfer_direction direction,
+		unsigned long prep_flags, unsigned long flags);
 	struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
 		struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned int sg_len, enum dma_transfer_direction direction,
@@ -973,6 +987,19 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
 						  dir, flags, NULL);
 }
 
+static inline struct dma_async_tx_descriptor *dmaengine_prep_peripheral_dma_vec(
+	struct dma_chan *chan, const struct dma_vec *vecs, size_t nents,
+	enum dma_transfer_direction dir, unsigned long prep_flags,
+	unsigned long flags)
+{
+	if (!chan || !chan->device || !chan->device->device_prep_peripheral_dma_vec)
+		return NULL;
+
+	return chan->device->device_prep_peripheral_dma_vec(chan, vecs, nents,
+							    dir, prep_flags,
+							    flags);
+}
+
 static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_sg(
 	struct dma_chan *chan, struct scatterlist *sgl,	unsigned int sg_len,
 	enum dma_transfer_direction dir, unsigned long flags)

-- 
2.43.2

[PATCH v7 2/6] dmaengine: dma-axi-dmac: Implement device_prep_peripheral_dma_vec

[Nuno Sa]

From: Paul Cercueil <paul@crapouillou.net>

Add implementation of the .device_prep_peripheral_dma_vec() callback.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/dma/dma-axi-dmac.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index 4e339c04fc1e..d03cbf1d28e8 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -620,6 +620,45 @@ static struct axi_dmac_sg *axi_dmac_fill_linear_sg(struct axi_dmac_chan *chan,
 	return sg;
 }
 
+static struct dma_async_tx_descriptor *
+axi_dmac_prep_peripheral_dma_vec(struct dma_chan *c, const struct dma_vec *vecs,
+				 size_t nb, enum dma_transfer_direction direction,
+				 unsigned long prep_flags, unsigned long flags)
+{
+	struct axi_dmac_chan *chan = to_axi_dmac_chan(c);
+	struct axi_dmac_desc *desc;
+	unsigned int num_sgs = 0;
+	struct axi_dmac_sg *dsg;
+	size_t i;
+
+	if (direction != chan->direction)
+		return NULL;
+
+	for (i = 0; i < nb; i++)
+		num_sgs += DIV_ROUND_UP(vecs[i].len, chan->max_length);
+
+	desc = axi_dmac_alloc_desc(chan, num_sgs);
+	if (!desc)
+		return NULL;
+
+	dsg = desc->sg;
+
+	for (i = 0; i < nb; i++) {
+		if (!axi_dmac_check_addr(chan, vecs[i].addr) ||
+		    !axi_dmac_check_len(chan, vecs[i].len)) {
+			kfree(desc);
+			return NULL;
+		}
+
+		dsg = axi_dmac_fill_linear_sg(chan, direction, vecs[i].addr, 1,
+					      vecs[i].len, dsg);
+	}
+
+	desc->cyclic = false;
+
+	return vchan_tx_prep(&chan->vchan, &desc->vdesc, prep_flags);
+}
+
 static struct dma_async_tx_descriptor *axi_dmac_prep_slave_sg(
 	struct dma_chan *c, struct scatterlist *sgl,
 	unsigned int sg_len, enum dma_transfer_direction direction,
@@ -1055,6 +1094,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
 	dma_dev->device_tx_status = dma_cookie_status;
 	dma_dev->device_issue_pending = axi_dmac_issue_pending;
 	dma_dev->device_prep_slave_sg = axi_dmac_prep_slave_sg;
+	dma_dev->device_prep_peripheral_dma_vec = axi_dmac_prep_peripheral_dma_vec;
 	dma_dev->device_prep_dma_cyclic = axi_dmac_prep_dma_cyclic;
 	dma_dev->device_prep_interleaved_dma = axi_dmac_prep_interleaved;
 	dma_dev->device_terminate_all = axi_dmac_terminate_all;

-- 
2.43.2

[PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Nuno Sa]

From: Paul Cercueil <paul@crapouillou.net>

Add the necessary infrastructure to the IIO core to support a new
optional DMABUF based interface.

With this new interface, DMABUF objects (externally created) can be
attached to a IIO buffer, and subsequently used for data transfer.

A userspace application can then use this interface to share DMABUF
objects between several interfaces, allowing it to transfer data in a
zero-copy fashion, for instance between IIO and the USB stack.

The userspace application can also memory-map the DMABUF objects, and
access the sample data directly. The advantage of doing this vs. the
read() interface is that it avoids an extra copy of the data between the
kernel and userspace. This is particularly userful for high-speed
devices which produce several megabytes or even gigabytes of data per
second.

As part of the interface, 3 new IOCTLs have been added:

IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
 Attach the DMABUF object identified by the given file descriptor to the
 buffer.

IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
 Detach the DMABUF object identified by the given file descriptor from
 the buffer. Note that closing the IIO buffer's file descriptor will
 automatically detach all previously attached DMABUF objects.

IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
 Request a data transfer to/from the given DMABUF object. Its file
 descriptor, as well as the transfer size and flags are provided in the
 "iio_dmabuf" structure.

These three IOCTLs have to be performed on the IIO buffer's file
descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-buffer.c | 462 ++++++++++++++++++++++++++++++++++++++
 include/linux/iio/buffer_impl.h   |  33 +++
 include/uapi/linux/iio/buffer.h   |  22 ++
 3 files changed, 517 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index b581a7e80566..0e63a09fa90a 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -9,14 +9,19 @@
  * - Better memory allocation techniques?
  * - Alternative access techniques?
  */
+#include <linux/atomic.h>
 #include <linux/anon_inodes.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/device.h>
+#include <linux/dma-buf.h>
+#include <linux/dma-fence.h>
+#include <linux/dma-resv.h>
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/cdev.h>
 #include <linux/slab.h>
+#include <linux/mm.h>
 #include <linux/poll.h>
 #include <linux/sched/signal.h>
 
@@ -28,6 +33,32 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 
+#define DMABUF_ENQUEUE_TIMEOUT_MS 5000
+
+struct iio_dma_fence;
+
+struct iio_dmabuf_priv {
+	struct list_head entry;
+	struct kref ref;
+
+	struct iio_buffer *buffer;
+	struct iio_dma_buffer_block *block;
+
+	u64 context;
+	spinlock_t lock;
+
+	struct dma_buf_attachment *attach;
+	struct sg_table *sgt;
+	enum dma_data_direction dir;
+	atomic_t seqno;
+};
+
+struct iio_dma_fence {
+	struct dma_fence base;
+	struct iio_dmabuf_priv *priv;
+	struct work_struct work;
+};
+
 static const char * const iio_endian_prefix[] = {
 	[IIO_BE] = "be",
 	[IIO_LE] = "le",
@@ -332,6 +363,8 @@ void iio_buffer_init(struct iio_buffer *buffer)
 {
 	INIT_LIST_HEAD(&buffer->demux_list);
 	INIT_LIST_HEAD(&buffer->buffer_list);
+	INIT_LIST_HEAD(&buffer->dmabufs);
+	mutex_init(&buffer->dmabufs_mutex);
 	init_waitqueue_head(&buffer->pollq);
 	kref_init(&buffer->ref);
 	if (!buffer->watermark)
@@ -1519,14 +1552,62 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
 	kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
 }
 
+static void iio_buffer_dmabuf_release(struct kref *ref)
+{
+	struct iio_dmabuf_priv *priv = container_of(ref, struct iio_dmabuf_priv, ref);
+	struct dma_buf_attachment *attach = priv->attach;
+	struct iio_buffer *buffer = priv->buffer;
+	struct dma_buf *dmabuf = attach->dmabuf;
+
+	dma_resv_lock(dmabuf->resv, NULL);
+	dma_buf_unmap_attachment(attach, priv->sgt, priv->dir);
+	dma_resv_unlock(dmabuf->resv);
+
+	buffer->access->detach_dmabuf(buffer, priv->block);
+
+	dma_buf_detach(attach->dmabuf, attach);
+	dma_buf_put(dmabuf);
+	kfree(priv);
+}
+
+void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach)
+{
+	struct iio_dmabuf_priv *priv = attach->importer_priv;
+
+	kref_get(&priv->ref);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_get);
+
+void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach)
+{
+	struct iio_dmabuf_priv *priv = attach->importer_priv;
+
+	kref_put(&priv->ref, iio_buffer_dmabuf_release);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_put);
+
 static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
 {
 	struct iio_dev_buffer_pair *ib = filep->private_data;
 	struct iio_dev *indio_dev = ib->indio_dev;
 	struct iio_buffer *buffer = ib->buffer;
+	struct iio_dmabuf_priv *priv, *tmp;
 
 	wake_up(&buffer->pollq);
 
+	mutex_lock(&buffer->dmabufs_mutex);
+
+	/* Close all attached DMABUFs */
+	list_for_each_entry_safe(priv, tmp, &buffer->dmabufs, entry) {
+		list_del_init(&priv->entry);
+		iio_buffer_dmabuf_put(priv->attach);
+	}
+
+	if (!list_empty(&buffer->dmabufs))
+		dev_warn(&indio_dev->dev, "Buffer FD closed with active transfers\n");
+
+	mutex_unlock(&buffer->dmabufs_mutex);
+
 	kfree(ib);
 	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
 	iio_device_put(indio_dev);
@@ -1534,11 +1615,391 @@ static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
 	return 0;
 }
 
+static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
+{
+	if (!nonblock)
+		return dma_resv_lock_interruptible(dmabuf->resv, NULL);
+
+	if (!dma_resv_trylock(dmabuf->resv))
+		return -EBUSY;
+
+	return 0;
+}
+
+static struct dma_buf_attachment *
+iio_buffer_find_attachment(struct iio_dev_buffer_pair *ib,
+			   struct dma_buf *dmabuf, bool nonblock)
+{
+	struct device *dev = ib->indio_dev->dev.parent;
+	struct iio_buffer *buffer = ib->buffer;
+	struct dma_buf_attachment *attach = NULL;
+	struct iio_dmabuf_priv *priv;
+
+	mutex_lock(&buffer->dmabufs_mutex);
+
+	list_for_each_entry(priv, &buffer->dmabufs, entry) {
+		if (priv->attach->dev == dev
+		    && priv->attach->dmabuf == dmabuf) {
+			attach = priv->attach;
+			break;
+		}
+	}
+
+	if (attach)
+		iio_buffer_dmabuf_get(attach);
+
+	mutex_unlock(&buffer->dmabufs_mutex);
+
+	return attach ?: ERR_PTR(-EPERM);
+}
+
+static int iio_buffer_attach_dmabuf(struct iio_dev_buffer_pair *ib,
+				    int __user *user_fd, bool nonblock)
+{
+	struct iio_dev *indio_dev = ib->indio_dev;
+	struct iio_buffer *buffer = ib->buffer;
+	struct dma_buf_attachment *attach;
+	struct iio_dmabuf_priv *priv;
+	struct dma_buf *dmabuf;
+	int err, fd;
+
+	if (!buffer->access->attach_dmabuf
+	    || !buffer->access->detach_dmabuf
+	    || !buffer->access->enqueue_dmabuf)
+		return -EPERM;
+
+	if (copy_from_user(&fd, user_fd, sizeof(fd)))
+		return -EFAULT;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	spin_lock_init(&priv->lock);
+	priv->context = dma_fence_context_alloc(1);
+
+	dmabuf = dma_buf_get(fd);
+	if (IS_ERR(dmabuf)) {
+		err = PTR_ERR(dmabuf);
+		goto err_free_priv;
+	}
+
+	attach = dma_buf_attach(dmabuf, indio_dev->dev.parent);
+	if (IS_ERR(attach)) {
+		err = PTR_ERR(attach);
+		goto err_dmabuf_put;
+	}
+
+	err = iio_dma_resv_lock(dmabuf, nonblock);
+	if (err)
+		goto err_dmabuf_detach;
+
+	priv->dir = buffer->direction == IIO_BUFFER_DIRECTION_IN
+		? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+
+	priv->sgt = dma_buf_map_attachment(attach, priv->dir);
+	if (IS_ERR(priv->sgt)) {
+		err = PTR_ERR(priv->sgt);
+		dev_err(&indio_dev->dev, "Unable to map attachment: %d\n", err);
+		goto err_resv_unlock;
+	}
+
+	kref_init(&priv->ref);
+	priv->buffer = buffer;
+	priv->attach = attach;
+	attach->importer_priv = priv;
+
+	priv->block = buffer->access->attach_dmabuf(buffer, attach);
+	if (IS_ERR(priv->block)) {
+		err = PTR_ERR(priv->block);
+		goto err_dmabuf_unmap_attachment;
+	}
+
+	dma_resv_unlock(dmabuf->resv);
+
+	mutex_lock(&buffer->dmabufs_mutex);
+	list_add(&priv->entry, &buffer->dmabufs);
+	mutex_unlock(&buffer->dmabufs_mutex);
+
+	return 0;
+
+err_dmabuf_unmap_attachment:
+	dma_buf_unmap_attachment(attach, priv->sgt, priv->dir);
+err_resv_unlock:
+	dma_resv_unlock(dmabuf->resv);
+err_dmabuf_detach:
+	dma_buf_detach(dmabuf, attach);
+err_dmabuf_put:
+	dma_buf_put(dmabuf);
+err_free_priv:
+	kfree(priv);
+
+	return err;
+}
+
+static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair *ib,
+				    int *user_req, bool nonblock)
+{
+	struct iio_buffer *buffer = ib->buffer;
+	struct iio_dev *indio_dev = ib->indio_dev;
+	struct iio_dmabuf_priv *priv;
+	struct dma_buf *dmabuf;
+	int dmabuf_fd, ret = -EPERM;
+
+	if (copy_from_user(&dmabuf_fd, user_req, sizeof(dmabuf_fd)))
+		return -EFAULT;
+
+	dmabuf = dma_buf_get(dmabuf_fd);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+
+	mutex_lock(&buffer->dmabufs_mutex);
+
+	list_for_each_entry(priv, &buffer->dmabufs, entry) {
+		if (priv->attach->dev == indio_dev->dev.parent
+		    && priv->attach->dmabuf == dmabuf) {
+			list_del(&priv->entry);
+
+			/* Unref the reference from iio_buffer_attach_dmabuf() */
+			iio_buffer_dmabuf_put(priv->attach);
+			ret = 0;
+			break;
+		}
+	}
+
+	mutex_unlock(&buffer->dmabufs_mutex);
+	dma_buf_put(dmabuf);
+
+	return ret;
+}
+
+static const char *
+iio_buffer_dma_fence_get_driver_name(struct dma_fence *fence)
+{
+	return "iio";
+}
+
+static void iio_buffer_dma_fence_release(struct dma_fence *fence)
+{
+	struct iio_dma_fence *iio_fence =
+		container_of(fence, struct iio_dma_fence, base);
+
+	kfree(iio_fence);
+}
+
+static const struct dma_fence_ops iio_buffer_dma_fence_ops = {
+	.get_driver_name	= iio_buffer_dma_fence_get_driver_name,
+	.get_timeline_name	= iio_buffer_dma_fence_get_driver_name,
+	.release		= iio_buffer_dma_fence_release,
+};
+
+static int iio_buffer_enqueue_dmabuf(struct iio_dev_buffer_pair *ib,
+				     struct iio_dmabuf __user *iio_dmabuf_req,
+				     bool nonblock)
+{
+	struct iio_buffer *buffer = ib->buffer;
+	struct iio_dmabuf iio_dmabuf;
+	struct dma_buf_attachment *attach;
+	struct iio_dmabuf_priv *priv;
+	struct iio_dma_fence *fence;
+	struct dma_buf *dmabuf;
+	unsigned long timeout;
+	bool cookie, cyclic, dma_to_ram;
+	long retl;
+	u32 seqno;
+	int ret;
+
+	if (copy_from_user(&iio_dmabuf, iio_dmabuf_req, sizeof(iio_dmabuf)))
+		return -EFAULT;
+
+	if (iio_dmabuf.flags & ~IIO_BUFFER_DMABUF_SUPPORTED_FLAGS)
+		return -EINVAL;
+
+	cyclic = iio_dmabuf.flags & IIO_BUFFER_DMABUF_CYCLIC;
+
+	/* Cyclic flag is only supported on output buffers */
+	if (cyclic && buffer->direction != IIO_BUFFER_DIRECTION_OUT)
+		return -EINVAL;
+
+	dmabuf = dma_buf_get(iio_dmabuf.fd);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+
+	if (!iio_dmabuf.bytes_used || iio_dmabuf.bytes_used > dmabuf->size) {
+		ret = -EINVAL;
+		goto err_dmabuf_put;
+	}
+
+	attach = iio_buffer_find_attachment(ib, dmabuf, nonblock);
+	if (IS_ERR(attach)) {
+		ret = PTR_ERR(attach);
+		goto err_dmabuf_put;
+	}
+
+	priv = attach->importer_priv;
+
+	fence = kmalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence) {
+		ret = -ENOMEM;
+		goto err_attachment_put;
+	}
+
+	fence->priv = priv;
+
+	seqno = atomic_add_return(1, &priv->seqno);
+
+	/*
+	 * The transfers are guaranteed to be processed in the order they are
+	 * enqueued, so we can use a simple incrementing sequence number for
+	 * the dma_fence.
+	 */
+	dma_fence_init(&fence->base, &iio_buffer_dma_fence_ops,
+		       &priv->lock, priv->context, seqno);
+
+	ret = iio_dma_resv_lock(dmabuf, nonblock);
+	if (ret)
+		goto err_fence_put;
+
+	timeout = nonblock ? 0 : msecs_to_jiffies(DMABUF_ENQUEUE_TIMEOUT_MS);
+
+	/* Make sure we don't have writers */
+	retl = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_WRITE,
+				     true, timeout);
+	if (retl == 0)
+		retl = -EBUSY;
+	if (retl < 0) {
+		ret = (int)retl;
+		goto err_resv_unlock;
+	}
+
+	dma_to_ram = buffer->direction == IIO_BUFFER_DIRECTION_IN;
+
+	if (dma_to_ram) {
+		/*
+		 * If we're writing to the DMABUF, make sure we don't have
+		 * readers
+		 */
+		retl = dma_resv_wait_timeout(dmabuf->resv,
+					     DMA_RESV_USAGE_READ, true,
+					     timeout);
+		if (retl == 0)
+			retl = -EBUSY;
+		if (retl < 0) {
+			ret = (int)retl;
+			goto err_resv_unlock;
+		}
+	}
+
+	if (buffer->access->lock_queue)
+		buffer->access->lock_queue(buffer);
+
+	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
+	if (ret)
+		goto err_queue_unlock;
+
+	dma_resv_add_fence(dmabuf->resv, &fence->base,
+			   dma_resv_usage_rw(dma_to_ram));
+	dma_resv_unlock(dmabuf->resv);
+
+	cookie = dma_fence_begin_signalling();
+
+	ret = buffer->access->enqueue_dmabuf(buffer, priv->block, &fence->base,
+					     priv->sgt, iio_dmabuf.bytes_used,
+					     cyclic);
+	if (ret) {
+		/*
+		 * DMABUF enqueue failed, but we already added the fence.
+		 * Signal the error through the fence completion mechanism.
+		 */
+		iio_buffer_signal_dmabuf_done(&fence->base, ret);
+	}
+
+	if (buffer->access->unlock_queue)
+		buffer->access->unlock_queue(buffer);
+
+	dma_fence_end_signalling(cookie);
+	dma_buf_put(dmabuf);
+
+	return ret;
+
+err_queue_unlock:
+	if (buffer->access->unlock_queue)
+		buffer->access->unlock_queue(buffer);
+err_resv_unlock:
+	dma_resv_unlock(dmabuf->resv);
+err_fence_put:
+	dma_fence_put(&fence->base);
+err_attachment_put:
+	iio_buffer_dmabuf_put(attach);
+err_dmabuf_put:
+	dma_buf_put(dmabuf);
+
+	return ret;
+}
+
+static void iio_buffer_cleanup(struct work_struct *work)
+{
+	struct iio_dma_fence *fence =
+		container_of(work, struct iio_dma_fence, work);
+	struct iio_dmabuf_priv *priv = fence->priv;
+	struct dma_buf_attachment *attach = priv->attach;
+
+	dma_fence_put(&fence->base);
+	iio_buffer_dmabuf_put(attach);
+}
+
+void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int ret)
+{
+	struct iio_dma_fence *iio_fence =
+		container_of(fence, struct iio_dma_fence, base);
+	bool cookie = dma_fence_begin_signalling();
+
+	/*
+	 * Get a reference to the fence, so that it's not freed as soon as
+	 * it's signaled.
+	 */
+	dma_fence_get(fence);
+
+	fence->error = ret;
+	dma_fence_signal(fence);
+	dma_fence_end_signalling(cookie);
+
+	/*
+	 * The fence will be unref'd in iio_buffer_cleanup.
+	 * It can't be done here, as the unref functions might try to lock the
+	 * resv object, which can deadlock.
+	 */
+	INIT_WORK(&iio_fence->work, iio_buffer_cleanup);
+	schedule_work(&iio_fence->work);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_signal_dmabuf_done);
+
+static long iio_buffer_chrdev_ioctl(struct file *filp,
+				    unsigned int cmd, unsigned long arg)
+{
+	struct iio_dev_buffer_pair *ib = filp->private_data;
+	void __user *_arg = (void __user *)arg;
+	bool nonblock = filp->f_flags & O_NONBLOCK;
+
+	switch (cmd) {
+	case IIO_BUFFER_DMABUF_ATTACH_IOCTL:
+		return iio_buffer_attach_dmabuf(ib, _arg, nonblock);
+	case IIO_BUFFER_DMABUF_DETACH_IOCTL:
+		return iio_buffer_detach_dmabuf(ib, _arg, nonblock);
+	case IIO_BUFFER_DMABUF_ENQUEUE_IOCTL:
+		return iio_buffer_enqueue_dmabuf(ib, _arg, nonblock);
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct file_operations iio_buffer_chrdev_fileops = {
 	.owner = THIS_MODULE,
 	.llseek = noop_llseek,
 	.read = iio_buffer_read,
 	.write = iio_buffer_write,
+	.unlocked_ioctl = iio_buffer_chrdev_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
 	.poll = iio_buffer_poll,
 	.release = iio_buffer_chrdev_release,
 };
@@ -1953,6 +2414,7 @@ static void iio_buffer_release(struct kref *ref)
 {
 	struct iio_buffer *buffer = container_of(ref, struct iio_buffer, ref);
 
+	mutex_destroy(&buffer->dmabufs_mutex);
 	buffer->access->release(buffer);
 }
 
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 89c3fd7c29ca..f4b1147291e5 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -9,8 +9,12 @@
 #include <uapi/linux/iio/buffer.h>
 #include <linux/iio/buffer.h>
 
+struct dma_buf_attachment;
+struct dma_fence;
 struct iio_dev;
+struct iio_dma_buffer_block;
 struct iio_buffer;
+struct sg_table;
 
 /**
  * INDIO_BUFFER_FLAG_FIXED_WATERMARK - Watermark level of the buffer can not be
@@ -39,6 +43,13 @@ struct iio_buffer;
  *                      device stops sampling. Calles are balanced with @enable.
  * @release:		called when the last reference to the buffer is dropped,
  *			should free all resources allocated by the buffer.
+ * @attach_dmabuf:	called from userspace via ioctl to attach one external
+ *			DMABUF.
+ * @detach_dmabuf:	called from userspace via ioctl to detach one previously
+ *			attached DMABUF.
+ * @enqueue_dmabuf:	called from userspace via ioctl to queue this DMABUF
+ *			object to this buffer. Requires a valid DMABUF fd, that
+ *			was previouly attached to this buffer.
  * @modes:		Supported operating modes by this buffer type
  * @flags:		A bitmask combination of INDIO_BUFFER_FLAG_*
  *
@@ -68,6 +79,17 @@ struct iio_buffer_access_funcs {
 
 	void (*release)(struct iio_buffer *buffer);
 
+	struct iio_dma_buffer_block * (*attach_dmabuf)(struct iio_buffer *buffer,
+						       struct dma_buf_attachment *attach);
+	void (*detach_dmabuf)(struct iio_buffer *buffer,
+			      struct iio_dma_buffer_block *block);
+	int (*enqueue_dmabuf)(struct iio_buffer *buffer,
+			      struct iio_dma_buffer_block *block,
+			      struct dma_fence *fence, struct sg_table *sgt,
+			      size_t size, bool cyclic);
+	void (*lock_queue)(struct iio_buffer *buffer);
+	void (*unlock_queue)(struct iio_buffer *buffer);
+
 	unsigned int modes;
 	unsigned int flags;
 };
@@ -136,6 +158,12 @@ struct iio_buffer {
 
 	/* @ref: Reference count of the buffer. */
 	struct kref ref;
+
+	/* @dmabufs: List of DMABUF attachments */
+	struct list_head dmabufs; /* P: dmabufs_mutex */
+
+	/* @dmabufs_mutex: Protects dmabufs */
+	struct mutex dmabufs_mutex;
 };
 
 /**
@@ -156,9 +184,14 @@ int iio_update_buffers(struct iio_dev *indio_dev,
  **/
 void iio_buffer_init(struct iio_buffer *buffer);
 
+void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach);
+void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach);
+
 struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer);
 void iio_buffer_put(struct iio_buffer *buffer);
 
+void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int ret);
+
 #else /* CONFIG_IIO_BUFFER */
 
 static inline void iio_buffer_get(struct iio_buffer *buffer) {}
diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
index 13939032b3f6..c666aa95e532 100644
--- a/include/uapi/linux/iio/buffer.h
+++ b/include/uapi/linux/iio/buffer.h
@@ -5,6 +5,28 @@
 #ifndef _UAPI_IIO_BUFFER_H_
 #define _UAPI_IIO_BUFFER_H_
 
+#include <linux/types.h>
+
+/* Flags for iio_dmabuf.flags */
+#define IIO_BUFFER_DMABUF_CYCLIC		(1 << 0)
+#define IIO_BUFFER_DMABUF_SUPPORTED_FLAGS	0x00000001
+
+/**
+ * struct iio_dmabuf - Descriptor for a single IIO DMABUF object
+ * @fd:		file descriptor of the DMABUF object
+ * @flags:	one or more IIO_BUFFER_DMABUF_* flags
+ * @bytes_used:	number of bytes used in this DMABUF for the data transfer.
+ *		Should generally be set to the DMABUF's size.
+ */
+struct iio_dmabuf {
+	__u32 fd;
+	__u32 flags;
+	__u64 bytes_used;
+};
+
 #define IIO_BUFFER_GET_FD_IOCTL			_IOWR('i', 0x91, int)
+#define IIO_BUFFER_DMABUF_ATTACH_IOCTL		_IOW('i', 0x92, int)
+#define IIO_BUFFER_DMABUF_DETACH_IOCTL		_IOW('i', 0x93, int)
+#define IIO_BUFFER_DMABUF_ENQUEUE_IOCTL		_IOW('i', 0x94, struct iio_dmabuf)
 
 #endif /* _UAPI_IIO_BUFFER_H_ */

-- 
2.43.2

[PATCH v7 4/6] iio: buffer-dma: Enable support for DMABUFs

[Nuno Sa]

From: Paul Cercueil <paul@crapouillou.net>

Implement iio_dma_buffer_attach_dmabuf(), iio_dma_buffer_detach_dmabuf()
and iio_dma_buffer_transfer_dmabuf(), which can then be used by the IIO
DMA buffer implementations.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/buffer/industrialio-buffer-dma.c | 181 +++++++++++++++++++++++++--
 include/linux/iio/buffer-dma.h               |  31 +++++
 2 files changed, 201 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index 5610ba67925e..c0f539af98f9 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -4,6 +4,8 @@
  *  Author: Lars-Peter Clausen <lars@metafoo.de>
  */
 
+#include <linux/atomic.h>
+#include <linux/cleanup.h>
 #include <linux/slab.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -14,6 +16,8 @@
 #include <linux/poll.h>
 #include <linux/iio/buffer_impl.h>
 #include <linux/iio/buffer-dma.h>
+#include <linux/dma-buf.h>
+#include <linux/dma-fence.h>
 #include <linux/dma-mapping.h>
 #include <linux/sizes.h>
 
@@ -94,13 +98,18 @@ static void iio_buffer_block_release(struct kref *kref)
 {
 	struct iio_dma_buffer_block *block = container_of(kref,
 		struct iio_dma_buffer_block, kref);
+	struct iio_dma_buffer_queue *queue = block->queue;
 
-	WARN_ON(block->state != IIO_BLOCK_STATE_DEAD);
+	WARN_ON(block->fileio && block->state != IIO_BLOCK_STATE_DEAD);
 
-	dma_free_coherent(block->queue->dev, PAGE_ALIGN(block->size),
-					block->vaddr, block->phys_addr);
+	if (block->fileio) {
+		dma_free_coherent(queue->dev, PAGE_ALIGN(block->size),
+				  block->vaddr, block->phys_addr);
+	} else {
+		atomic_dec(&queue->num_dmabufs);
+	}
 
-	iio_buffer_put(&block->queue->buffer);
+	iio_buffer_put(&queue->buffer);
 	kfree(block);
 }
 
@@ -163,7 +172,7 @@ static struct iio_dma_buffer_queue *iio_buffer_to_queue(struct iio_buffer *buf)
 }
 
 static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(
-	struct iio_dma_buffer_queue *queue, size_t size)
+	struct iio_dma_buffer_queue *queue, size_t size, bool fileio)
 {
 	struct iio_dma_buffer_block *block;
 
@@ -171,13 +180,16 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(
 	if (!block)
 		return NULL;
 
-	block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size),
-		&block->phys_addr, GFP_KERNEL);
-	if (!block->vaddr) {
-		kfree(block);
-		return NULL;
+	if (fileio) {
+		block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size),
+						  &block->phys_addr, GFP_KERNEL);
+		if (!block->vaddr) {
+			kfree(block);
+			return NULL;
+		}
 	}
 
+	block->fileio = fileio;
 	block->size = size;
 	block->state = IIO_BLOCK_STATE_DONE;
 	block->queue = queue;
@@ -186,6 +198,9 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(
 
 	iio_buffer_get(&queue->buffer);
 
+	if (!fileio)
+		atomic_inc(&queue->num_dmabufs);
+
 	return block;
 }
 
@@ -206,13 +221,21 @@ void iio_dma_buffer_block_done(struct iio_dma_buffer_block *block)
 {
 	struct iio_dma_buffer_queue *queue = block->queue;
 	unsigned long flags;
+	bool cookie;
+
+	cookie = dma_fence_begin_signalling();
 
 	spin_lock_irqsave(&queue->list_lock, flags);
 	_iio_dma_buffer_block_done(block);
 	spin_unlock_irqrestore(&queue->list_lock, flags);
 
+	if (!block->fileio)
+		iio_buffer_signal_dmabuf_done(block->fence, 0);
+
 	iio_buffer_block_put_atomic(block);
 	wake_up_interruptible_poll(&queue->buffer.pollq, EPOLLIN | EPOLLRDNORM);
+
+	dma_fence_end_signalling(cookie);
 }
 EXPORT_SYMBOL_GPL(iio_dma_buffer_block_done);
 
@@ -231,17 +254,27 @@ void iio_dma_buffer_block_list_abort(struct iio_dma_buffer_queue *queue,
 {
 	struct iio_dma_buffer_block *block, *_block;
 	unsigned long flags;
+	bool cookie;
+
+	cookie = dma_fence_begin_signalling();
 
 	spin_lock_irqsave(&queue->list_lock, flags);
 	list_for_each_entry_safe(block, _block, list, head) {
 		list_del(&block->head);
 		block->bytes_used = 0;
 		_iio_dma_buffer_block_done(block);
+
+		if (!block->fileio)
+			iio_buffer_signal_dmabuf_done(block->fence, -EINTR);
 		iio_buffer_block_put_atomic(block);
 	}
 	spin_unlock_irqrestore(&queue->list_lock, flags);
 
+	if (queue->fileio.enabled)
+		queue->fileio.enabled = false;
+
 	wake_up_interruptible_poll(&queue->buffer.pollq, EPOLLIN | EPOLLRDNORM);
+	dma_fence_end_signalling(cookie);
 }
 EXPORT_SYMBOL_GPL(iio_dma_buffer_block_list_abort);
 
@@ -261,6 +294,16 @@ static bool iio_dma_block_reusable(struct iio_dma_buffer_block *block)
 	}
 }
 
+static bool iio_dma_buffer_can_use_fileio(struct iio_dma_buffer_queue *queue)
+{
+	/*
+	 * Note that queue->num_dmabufs cannot increase while the queue is
+	 * locked, it can only decrease, so it does not race against
+	 * iio_dma_buffer_alloc_block().
+	 */
+	return queue->fileio.enabled || !atomic_read(&queue->num_dmabufs);
+}
+
 /**
  * iio_dma_buffer_request_update() - DMA buffer request_update callback
  * @buffer: The buffer which to request an update
@@ -287,6 +330,12 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer)
 
 	mutex_lock(&queue->lock);
 
+	queue->fileio.enabled = iio_dma_buffer_can_use_fileio(queue);
+
+	/* If DMABUFs were created, disable fileio interface */
+	if (!queue->fileio.enabled)
+		goto out_unlock;
+
 	/* Allocations are page aligned */
 	if (PAGE_ALIGN(queue->fileio.block_size) == PAGE_ALIGN(size))
 		try_reuse = true;
@@ -327,7 +376,7 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer)
 		}
 
 		if (!block) {
-			block = iio_dma_buffer_alloc_block(queue, size);
+			block = iio_dma_buffer_alloc_block(queue, size, true);
 			if (!block) {
 				ret = -ENOMEM;
 				goto out_unlock;
@@ -384,8 +433,12 @@ static void iio_dma_buffer_submit_block(struct iio_dma_buffer_queue *queue,
 
 	block->state = IIO_BLOCK_STATE_ACTIVE;
 	iio_buffer_block_get(block);
+
 	ret = queue->ops->submit(queue, block);
 	if (ret) {
+		if (!block->fileio)
+			iio_buffer_signal_dmabuf_done(block->fence, ret);
+
 		/*
 		 * This is a bit of a problem and there is not much we can do
 		 * other then wait for the buffer to be disabled and re-enabled
@@ -588,6 +641,112 @@ size_t iio_dma_buffer_data_available(struct iio_buffer *buf)
 }
 EXPORT_SYMBOL_GPL(iio_dma_buffer_data_available);
 
+struct iio_dma_buffer_block *
+iio_dma_buffer_attach_dmabuf(struct iio_buffer *buffer,
+			     struct dma_buf_attachment *attach)
+{
+	struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
+	struct iio_dma_buffer_block *block;
+
+	guard(mutex)(&queue->lock);
+
+	/*
+	 * If the buffer is enabled and in fileio mode new blocks can't be
+	 * allocated.
+	 */
+	if (queue->fileio.enabled)
+		return ERR_PTR(-EBUSY);
+
+	block = iio_dma_buffer_alloc_block(queue, attach->dmabuf->size, false);
+	if (!block)
+		return ERR_PTR(-ENOMEM);
+
+	block->attach = attach;
+
+	/* Free memory that might be in use for fileio mode */
+	iio_dma_buffer_fileio_free(queue);
+
+	return block;
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_attach_dmabuf);
+
+void iio_dma_buffer_detach_dmabuf(struct iio_buffer *buffer,
+				  struct iio_dma_buffer_block *block)
+{
+	block->state = IIO_BLOCK_STATE_DEAD;
+	iio_buffer_block_put_atomic(block);
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_detach_dmabuf);
+
+static int iio_dma_can_enqueue_block(struct iio_dma_buffer_block *block)
+{
+	struct iio_dma_buffer_queue *queue = block->queue;
+
+	/* If in fileio mode buffers can't be enqueued. */
+	if (queue->fileio.enabled)
+		return -EBUSY;
+
+	switch (block->state) {
+	case IIO_BLOCK_STATE_QUEUED:
+		return -EPERM;
+	case IIO_BLOCK_STATE_ACTIVE:
+	case IIO_BLOCK_STATE_DEAD:
+		return -EBUSY;
+	case IIO_BLOCK_STATE_DONE:
+		break;
+	}
+
+	return 0;
+}
+
+int iio_dma_buffer_enqueue_dmabuf(struct iio_buffer *buffer,
+				  struct iio_dma_buffer_block *block,
+				  struct dma_fence *fence,
+				  struct sg_table *sgt,
+				  size_t size, bool cyclic)
+{
+	struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
+	bool cookie;
+	int ret;
+
+	WARN_ON(!mutex_is_locked(&queue->lock));
+
+	cookie = dma_fence_begin_signalling();
+
+	ret = iio_dma_can_enqueue_block(block);
+	if (ret < 0)
+		goto out_end_signalling;
+
+	block->bytes_used = size;
+	block->cyclic = cyclic;
+	block->sg_table = sgt;
+	block->fence = fence;
+
+	iio_dma_buffer_enqueue(queue, block);
+
+out_end_signalling:
+	dma_fence_end_signalling(cookie);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_enqueue_dmabuf);
+
+void iio_dma_buffer_lock_queue(struct iio_buffer *buffer)
+{
+	struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
+
+	mutex_lock(&queue->lock);
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_lock_queue);
+
+void iio_dma_buffer_unlock_queue(struct iio_buffer *buffer)
+{
+	struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
+
+	mutex_unlock(&queue->lock);
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_unlock_queue);
+
 /**
  * iio_dma_buffer_set_bytes_per_datum() - DMA buffer set_bytes_per_datum callback
  * @buffer: Buffer to set the bytes-per-datum for
diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
index 18d3702fa95d..b62ff2a3f554 100644
--- a/include/linux/iio/buffer-dma.h
+++ b/include/linux/iio/buffer-dma.h
@@ -7,6 +7,7 @@
 #ifndef __INDUSTRIALIO_DMA_BUFFER_H__
 #define __INDUSTRIALIO_DMA_BUFFER_H__
 
+#include <linux/atomic.h>
 #include <linux/list.h>
 #include <linux/kref.h>
 #include <linux/spinlock.h>
@@ -16,6 +17,9 @@
 struct iio_dma_buffer_queue;
 struct iio_dma_buffer_ops;
 struct device;
+struct dma_buf_attachment;
+struct dma_fence;
+struct sg_table;
 
 /**
  * enum iio_block_state - State of a struct iio_dma_buffer_block
@@ -41,6 +45,8 @@ enum iio_block_state {
  * @queue: Parent DMA buffer queue
  * @kref: kref used to manage the lifetime of block
  * @state: Current state of the block
+ * @cyclic: True if this is a cyclic buffer
+ * @fileio: True if this buffer is used for fileio mode
  */
 struct iio_dma_buffer_block {
 	/* May only be accessed by the owner of the block */
@@ -63,6 +69,14 @@ struct iio_dma_buffer_block {
 	 * queue->list_lock if the block is not owned by the core.
 	 */
 	enum iio_block_state state;
+
+	bool cyclic;
+	bool fileio;
+
+	struct dma_buf_attachment *attach;
+	struct sg_table *sg_table;
+
+	struct dma_fence *fence;
 };
 
 /**
@@ -72,6 +86,7 @@ struct iio_dma_buffer_block {
  * @pos: Read offset in the active block
  * @block_size: Size of each block
  * @next_dequeue: index of next block that will be dequeued
+ * @enabled: Whether the buffer is operating in fileio mode
  */
 struct iio_dma_buffer_queue_fileio {
 	struct iio_dma_buffer_block *blocks[2];
@@ -80,6 +95,7 @@ struct iio_dma_buffer_queue_fileio {
 	size_t block_size;
 
 	unsigned int next_dequeue;
+	bool enabled;
 };
 
 /**
@@ -95,6 +111,7 @@ struct iio_dma_buffer_queue_fileio {
  *   the DMA controller
  * @incoming: List of buffers on the incoming queue
  * @active: Whether the buffer is currently active
+ * @num_dmabufs: Total number of DMABUFs attached to this queue
  * @fileio: FileIO state
  */
 struct iio_dma_buffer_queue {
@@ -107,6 +124,7 @@ struct iio_dma_buffer_queue {
 	struct list_head incoming;
 
 	bool active;
+	atomic_t num_dmabufs;
 
 	struct iio_dma_buffer_queue_fileio fileio;
 };
@@ -142,4 +160,17 @@ int iio_dma_buffer_init(struct iio_dma_buffer_queue *queue,
 void iio_dma_buffer_exit(struct iio_dma_buffer_queue *queue);
 void iio_dma_buffer_release(struct iio_dma_buffer_queue *queue);
 
+struct iio_dma_buffer_block *
+iio_dma_buffer_attach_dmabuf(struct iio_buffer *buffer,
+			     struct dma_buf_attachment *attach);
+void iio_dma_buffer_detach_dmabuf(struct iio_buffer *buffer,
+				  struct iio_dma_buffer_block *block);
+int iio_dma_buffer_enqueue_dmabuf(struct iio_buffer *buffer,
+				  struct iio_dma_buffer_block *block,
+				  struct dma_fence *fence,
+				  struct sg_table *sgt,
+				  size_t size, bool cyclic);
+void iio_dma_buffer_lock_queue(struct iio_buffer *buffer);
+void iio_dma_buffer_unlock_queue(struct iio_buffer *buffer);
+
 #endif

-- 
2.43.2

[PATCH v7 5/6] iio: buffer-dmaengine: Support new DMABUF based userspace API

[Nuno Sa]

From: Paul Cercueil <paul@crapouillou.net>

Use the functions provided by the buffer-dma core to implement the
DMABUF userspace API in the buffer-dmaengine IIO buffer implementation.

Since we want to be able to transfer an arbitrary number of bytes and
not necesarily the full DMABUF, the associated scatterlist is converted
to an array of DMA addresses + lengths, which is then passed to
dmaengine_prep_slave_dma_array().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/buffer/industrialio-buffer-dmaengine.c | 59 +++++++++++++++++++---
 1 file changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index a18c1da292af..3b7b649f0a89 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -64,15 +64,55 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue,
 	struct dmaengine_buffer *dmaengine_buffer =
 		iio_buffer_to_dmaengine_buffer(&queue->buffer);
 	struct dma_async_tx_descriptor *desc;
+	struct scatterlist *sgl;
+	struct dma_vec *vecs;
 	dma_cookie_t cookie;
+	size_t len_total;
+	size_t max_size;
+	unsigned int i;
+	int nents;
 
-	block->bytes_used = min(block->size, dmaengine_buffer->max_size);
-	block->bytes_used = round_down(block->bytes_used,
-			dmaengine_buffer->align);
+	if (queue->buffer.direction != IIO_BUFFER_DIRECTION_IN) {
+		/* We do not yet support output buffers. */
+		return -EINVAL;
+	}
 
-	desc = dmaengine_prep_slave_single(dmaengine_buffer->chan,
-		block->phys_addr, block->bytes_used, DMA_DEV_TO_MEM,
-		DMA_PREP_INTERRUPT);
+	if (block->sg_table) {
+		sgl = block->sg_table->sgl;
+		nents = sg_nents_for_len(sgl, block->bytes_used);
+		if (nents < 0)
+			return nents;
+
+		vecs = kmalloc_array(nents, sizeof(*vecs), GFP_ATOMIC);
+		if (!vecs)
+			return -ENOMEM;
+
+		len_total = block->bytes_used;
+
+		for (i = 0; i < nents; i++) {
+			vecs[i].addr = sg_dma_address(sgl);
+			vecs[i].len = min(sg_dma_len(sgl), len_total);
+			len_total -= vecs[i].len;
+
+			sgl = sg_next(sgl);
+		}
+
+		desc = dmaengine_prep_peripheral_dma_vec(dmaengine_buffer->chan,
+							 vecs, nents,
+							 DMA_DEV_TO_MEM,
+							 DMA_PREP_INTERRUPT, 0);
+		kfree(vecs);
+	} else {
+		max_size = min(block->size, dmaengine_buffer->max_size);
+		max_size = round_down(max_size, dmaengine_buffer->align);
+		block->bytes_used = max_size;
+
+		desc = dmaengine_prep_slave_single(dmaengine_buffer->chan,
+						   block->phys_addr,
+						   block->bytes_used,
+						   DMA_DEV_TO_MEM,
+						   DMA_PREP_INTERRUPT);
+	}
 	if (!desc)
 		return -ENOMEM;
 
@@ -120,6 +160,13 @@ static const struct iio_buffer_access_funcs iio_dmaengine_buffer_ops = {
 	.data_available = iio_dma_buffer_data_available,
 	.release = iio_dmaengine_buffer_release,
 
+	.enqueue_dmabuf = iio_dma_buffer_enqueue_dmabuf,
+	.attach_dmabuf = iio_dma_buffer_attach_dmabuf,
+	.detach_dmabuf = iio_dma_buffer_detach_dmabuf,
+
+	.lock_queue = iio_dma_buffer_lock_queue,
+	.unlock_queue = iio_dma_buffer_unlock_queue,
+
 	.modes = INDIO_BUFFER_HARDWARE,
 	.flags = INDIO_BUFFER_FLAG_FIXED_WATERMARK,
 };

-- 
2.43.2

[PATCH v7 6/6] Documentation: iio: Document high-speed DMABUF based API

[Nuno Sa]

From: Paul Cercueil <paul@crapouillou.net>

Document the new DMABUF based API.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 Documentation/iio/dmabuf_api.rst | 54 ++++++++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst      |  2 ++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/iio/dmabuf_api.rst b/Documentation/iio/dmabuf_api.rst
new file mode 100644
index 000000000000..1cd6cd51a582
--- /dev/null
+++ b/Documentation/iio/dmabuf_api.rst
@@ -0,0 +1,54 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===================================
+High-speed DMABUF interface for IIO
+===================================
+
+1. Overview
+===========
+
+The Industrial I/O subsystem supports access to buffers through a
+file-based interface, with read() and write() access calls through the
+IIO device's dev node.
+
+It additionally supports a DMABUF based interface, where the userspace
+can attach DMABUF objects (externally created) to a IIO buffer, and
+subsequently use them for data transfers.
+
+A userspace application can then use this interface to share DMABUF
+objects between several interfaces, allowing it to transfer data in a
+zero-copy fashion, for instance between IIO and the USB stack.
+
+The userspace application can also memory-map the DMABUF objects, and
+access the sample data directly. The advantage of doing this vs. the
+read() interface is that it avoids an extra copy of the data between the
+kernel and userspace. This is particularly useful for high-speed devices
+which produce several megabytes or even gigabytes of data per second.
+It does however increase the userspace-kernelspace synchronization
+overhead, as the DMA_BUF_SYNC_START and DMA_BUF_SYNC_END IOCTLs have to
+be used for data integrity.
+
+2. User API
+===========
+
+As part of this interface, three new IOCTLs have been added. These three
+IOCTLs have to be performed on the IIO buffer's file descriptor,
+obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.
+
+  ``IIO_BUFFER_DMABUF_ATTACH_IOCTL(int)``
+    Attach the DMABUF object, identified by its file descriptor, to the
+    IIO buffer. Returns zero on success, and a negative errno value on
+    error.
+
+  ``IIO_BUFFER_DMABUF_DETACH_IOCTL(int)``
+    Detach the given DMABUF object, identified by its file descriptor,
+    from the IIO buffer. Returns zero on success, and a negative errno
+    value on error.
+
+    Note that closing the IIO buffer's file descriptor will
+    automatically detach all previously attached DMABUF objects.
+
+  ``IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *iio_dmabuf)``
+    Enqueue a previously attached DMABUF object to the buffer queue.
+    Enqueued DMABUFs will be read from (if output buffer) or written to
+    (if input buffer) as long as the buffer is enabled.
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 1b7292c58cd0..3eae8fcb1938 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -9,6 +9,8 @@ Industrial I/O
 
    iio_configfs
 
+   dmabuf_api
+
    ep93xx_adc
 
    bno055

-- 
2.43.2

Re: [PATCH v7 0/6] iio: new DMABUF based API

[Jonathan Cameron]

On Fri, 23 Feb 2024 13:13:58 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> Hi Jonathan, likely you're wondering why I'm sending v7. Well, to be
> honest, we're hoping to get this merged this for the 6.9 merge window.
> Main reason is because the USB part is already in (so it would be nice
> to get the whole thing in). Moreover, the changes asked in v6 were simple
> (even though I'm not quite sure in one of them) and Paul has no access to
> it's laptop so he can't send v7 himself. So he kind of said/asked for me to do it.

So, we are cutting this very fine. If Linus hints strongly at an rc8 maybe we
can sneak this in. However, I need an Ack from Vinod for the dma engine changes first.

Also I'd love a final 'looks ok' comment from DMABUF folk (Ack even better!)

Seems that the other side got resolved in the USB gadget, but last we heard form
Daniel and Christian looks to have been back on v5. I'd like them to confirm
they are fine with the changes made as a result. 

I've been happy with the IIO parts for a few versions now but my ability to review
the DMABUF and DMA engine bits is limited.

A realistic path to get this in is rc8 is happening, is all Acks in place by Wednesday,
I get apply it and hits Linux-next Thursday, Pull request to Greg on Saturday and Greg
is feeling particularly generous to take one on the day he normally closes his trees.

Whilst I'll cross my fingers, looks like 6.10 material to me :(

I'd missed the progress on the USB side so wasn't paying enough attention. Sorry!

Jonathan

> 
> v6:
>  * https://lore.kernel.org/linux-iio/20240129170201.133785-1-paul@crapouillou.net/
> 
> v7:
>  - Patch 1
>   * Renamed *device_prep_slave_dma_vec() -> device_prep_peripheral_dma_vec();
>   * Added a new flag parameter to the function as agreed between Paul
>     and Vinod. I renamed the first parameter to prep_flags as it's supposed to
>     be used (I think) with enum dma_ctrl_flags. I'm not really sure how that API
>     can grow but I was thinking in just having a bool cyclic parameter (as the
>     first intention of the flags is to support cyclic transfers) but ended up
>     "respecting" the previously agreed approach.
> - Patch 2
>   * Adapted patch for the changes made in patch 1.
> - Patch 5
>   * Adapted patch for the changes made in patch 1.
> 
> Patchset based on next-20240223.
> 
> ---
> Paul Cercueil (6):
>       dmaengine: Add API function dmaengine_prep_peripheral_dma_vec()
>       dmaengine: dma-axi-dmac: Implement device_prep_peripheral_dma_vec
>       iio: core: Add new DMABUF interface infrastructure
>       iio: buffer-dma: Enable support for DMABUFs
>       iio: buffer-dmaengine: Support new DMABUF based userspace API
>       Documentation: iio: Document high-speed DMABUF based API
> 
>  Documentation/iio/dmabuf_api.rst                   |  54 +++
>  Documentation/iio/index.rst                        |   2 +
>  drivers/dma/dma-axi-dmac.c                         |  40 ++
>  drivers/iio/buffer/industrialio-buffer-dma.c       | 181 +++++++-
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c |  59 ++-
>  drivers/iio/industrialio-buffer.c                  | 462 +++++++++++++++++++++
>  include/linux/dmaengine.h                          |  27 ++
>  include/linux/iio/buffer-dma.h                     |  31 ++
>  include/linux/iio/buffer_impl.h                    |  33 ++
>  include/uapi/linux/iio/buffer.h                    |  22 +
>  10 files changed, 894 insertions(+), 17 deletions(-)
> ---
> base-commit: 33e1d31873f87d119e5120b88cd350efa68ef276
> change-id: 20240223-iio-dmabuf-5ee0530195ca
> --
> 
> Thanks!
> - Nuno Sá
> 

Re: [PATCH v7 0/6] iio: new DMABUF based API

[Nuno Sá]

On Sun, 2024-03-03 at 17:42 +0000, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 13:13:58 +0100
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Hi Jonathan, likely you're wondering why I'm sending v7. Well, to be
> > honest, we're hoping to get this merged this for the 6.9 merge window.
> > Main reason is because the USB part is already in (so it would be nice
> > to get the whole thing in). Moreover, the changes asked in v6 were simple
> > (even though I'm not quite sure in one of them) and Paul has no access to
> > it's laptop so he can't send v7 himself. So he kind of said/asked for me to
> > do it.
> 
> So, we are cutting this very fine. If Linus hints strongly at an rc8 maybe we
> can sneak this in. However, I need an Ack from Vinod for the dma engine
> changes first.
> 
> Also I'd love a final 'looks ok' comment from DMABUF folk (Ack even better!)
> 
> Seems that the other side got resolved in the USB gadget, but last we heard
> form
> Daniel and Christian looks to have been back on v5. I'd like them to confirm
> they are fine with the changes made as a result. 
> 

I can ask Christian or Daniel for some acks but my feeling (I still need, at
some point, to get really familiar with all of this) is that this should be
pretty similar to the USB series (from a DMABUF point of view) as they are both
importers.

> I've been happy with the IIO parts for a few versions now but my ability to
> review
> the DMABUF and DMA engine bits is limited.
> 
> A realistic path to get this in is rc8 is happening, is all Acks in place by
> Wednesday,
> I get apply it and hits Linux-next Thursday, Pull request to Greg on Saturday
> and Greg
> is feeling particularly generous to take one on the day he normally closes his
> trees.
> 

Well, it looks like we still have a shot. I'll try to see if Vinod is fine with
the DMAENGINE stuff.

- Nuno Sá

Re: [PATCH v7 1/6] dmaengine: Add API function dmaengine_prep_peripheral_dma_vec()

[Nuno Sá]

On Fri, 2024-02-23 at 13:13 +0100, Nuno Sa wrote:
> From: Paul Cercueil <paul@crapouillou.net>
> 
> This function can be used to initiate a scatter-gather DMA transfer,
> where the address and size of each segment is located in one entry of
> the dma_vec array.
> 
> The major difference with dmaengine_prep_slave_sg() is that it supports
> specifying the lengths of each DMA transfer; as trying to override the
> length of the transfer with dmaengine_prep_slave_sg() is a very tedious
> process. The introduction of a new API function is also justified by the
> fact that scatterlists are on their way out.
> 
> Note that dmaengine_prep_interleaved_dma() is not helpful either in that
> case, as it assumes that the address of each segment will be higher than
> the one of the previous segment, which we just cannot guarantee in case
> of a scatter-gather transfer.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---

Hi Vinod,

Is this already good for you? I do not want to be pushy but we're trying to see
if we can have this in the 6.9 cycle and Jonathan definitely wants an ack from
you before merging this in his tree. I've more or less till Wednesday so that's
why I'm asking already today so I still have time to re-spin if you want some
changes.

- Nuno Sá

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Christian König]

Am 23.02.24 um 13:14 schrieb Nuno Sa:
> From: Paul Cercueil <paul@crapouillou.net>
>
> Add the necessary infrastructure to the IIO core to support a new
> optional DMABUF based interface.
>
> With this new interface, DMABUF objects (externally created) can be
> attached to a IIO buffer, and subsequently used for data transfer.
>
> A userspace application can then use this interface to share DMABUF
> objects between several interfaces, allowing it to transfer data in a
> zero-copy fashion, for instance between IIO and the USB stack.
>
> The userspace application can also memory-map the DMABUF objects, and
> access the sample data directly. The advantage of doing this vs. the
> read() interface is that it avoids an extra copy of the data between the
> kernel and userspace. This is particularly userful for high-speed
> devices which produce several megabytes or even gigabytes of data per
> second.
>
> As part of the interface, 3 new IOCTLs have been added:
>
> IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
>   Attach the DMABUF object identified by the given file descriptor to the
>   buffer.
>
> IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
>   Detach the DMABUF object identified by the given file descriptor from
>   the buffer. Note that closing the IIO buffer's file descriptor will
>   automatically detach all previously attached DMABUF objects.
>
> IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
>   Request a data transfer to/from the given DMABUF object. Its file
>   descriptor, as well as the transfer size and flags are provided in the
>   "iio_dmabuf" structure.
>
> These three IOCTLs have to be performed on the IIO buffer's file
> descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.

A few nit picks and one bug below, apart from that looks good to me.

>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>   drivers/iio/industrialio-buffer.c | 462 ++++++++++++++++++++++++++++++++++++++
>   include/linux/iio/buffer_impl.h   |  33 +++
>   include/uapi/linux/iio/buffer.h   |  22 ++
>   3 files changed, 517 insertions(+)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index b581a7e80566..0e63a09fa90a 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -9,14 +9,19 @@
>    * - Better memory allocation techniques?
>    * - Alternative access techniques?
>    */
> +#include <linux/atomic.h>
>   #include <linux/anon_inodes.h>
>   #include <linux/kernel.h>
>   #include <linux/export.h>
>   #include <linux/device.h>
> +#include <linux/dma-buf.h>
> +#include <linux/dma-fence.h>
> +#include <linux/dma-resv.h>
>   #include <linux/file.h>
>   #include <linux/fs.h>
>   #include <linux/cdev.h>
>   #include <linux/slab.h>
> +#include <linux/mm.h>
>   #include <linux/poll.h>
>   #include <linux/sched/signal.h>
>   
> @@ -28,6 +33,32 @@
>   #include <linux/iio/buffer.h>
>   #include <linux/iio/buffer_impl.h>
>   
> +#define DMABUF_ENQUEUE_TIMEOUT_MS 5000
> +
> +struct iio_dma_fence;
> +
> +struct iio_dmabuf_priv {
> +	struct list_head entry;
> +	struct kref ref;
> +
> +	struct iio_buffer *buffer;
> +	struct iio_dma_buffer_block *block;
> +
> +	u64 context;
> +	spinlock_t lock;
> +
> +	struct dma_buf_attachment *attach;
> +	struct sg_table *sgt;
> +	enum dma_data_direction dir;
> +	atomic_t seqno;
> +};
> +
> +struct iio_dma_fence {
> +	struct dma_fence base;
> +	struct iio_dmabuf_priv *priv;
> +	struct work_struct work;
> +};
> +
>   static const char * const iio_endian_prefix[] = {
>   	[IIO_BE] = "be",
>   	[IIO_LE] = "le",
> @@ -332,6 +363,8 @@ void iio_buffer_init(struct iio_buffer *buffer)
>   {
>   	INIT_LIST_HEAD(&buffer->demux_list);
>   	INIT_LIST_HEAD(&buffer->buffer_list);
> +	INIT_LIST_HEAD(&buffer->dmabufs);
> +	mutex_init(&buffer->dmabufs_mutex);
>   	init_waitqueue_head(&buffer->pollq);
>   	kref_init(&buffer->ref);
>   	if (!buffer->watermark)
> @@ -1519,14 +1552,62 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
>   	kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
>   }
>   
> +static void iio_buffer_dmabuf_release(struct kref *ref)
> +{
> +	struct iio_dmabuf_priv *priv = container_of(ref, struct iio_dmabuf_priv, ref);
> +	struct dma_buf_attachment *attach = priv->attach;
> +	struct iio_buffer *buffer = priv->buffer;
> +	struct dma_buf *dmabuf = attach->dmabuf;
> +
> +	dma_resv_lock(dmabuf->resv, NULL);
> +	dma_buf_unmap_attachment(attach, priv->sgt, priv->dir);
> +	dma_resv_unlock(dmabuf->resv);
> +
> +	buffer->access->detach_dmabuf(buffer, priv->block);
> +
> +	dma_buf_detach(attach->dmabuf, attach);
> +	dma_buf_put(dmabuf);
> +	kfree(priv);
> +}
> +
> +void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach)
> +{
> +	struct iio_dmabuf_priv *priv = attach->importer_priv;
> +
> +	kref_get(&priv->ref);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_get);
> +
> +void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach)
> +{
> +	struct iio_dmabuf_priv *priv = attach->importer_priv;
> +
> +	kref_put(&priv->ref, iio_buffer_dmabuf_release);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_put);
> +
>   static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
>   {
>   	struct iio_dev_buffer_pair *ib = filep->private_data;
>   	struct iio_dev *indio_dev = ib->indio_dev;
>   	struct iio_buffer *buffer = ib->buffer;
> +	struct iio_dmabuf_priv *priv, *tmp;
>   
>   	wake_up(&buffer->pollq);
>   
> +	mutex_lock(&buffer->dmabufs_mutex);
> +
> +	/* Close all attached DMABUFs */
> +	list_for_each_entry_safe(priv, tmp, &buffer->dmabufs, entry) {
> +		list_del_init(&priv->entry);
> +		iio_buffer_dmabuf_put(priv->attach);
> +	}
> +
> +	if (!list_empty(&buffer->dmabufs))
> +		dev_warn(&indio_dev->dev, "Buffer FD closed with active transfers\n");

That check here smells fishy.

Either the list_for_each_entry_safe() above has removed all entries and 
then it can never happen or the buffer list is not always properly 
protected by the mutex.

Do you really need that? If yes then please justify.

BTW: When the transfers are async it's perfectly possible that there are 
active transfers on close (when the application is just killed for example).

> +
> +	mutex_unlock(&buffer->dmabufs_mutex);
> +
>   	kfree(ib);
>   	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
>   	iio_device_put(indio_dev);
> @@ -1534,11 +1615,391 @@ static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
>   	return 0;
>   }
>   
> +static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
> +{
> +	if (!nonblock)
> +		return dma_resv_lock_interruptible(dmabuf->resv, NULL);
> +
> +	if (!dma_resv_trylock(dmabuf->resv))
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +static struct dma_buf_attachment *
> +iio_buffer_find_attachment(struct iio_dev_buffer_pair *ib,
> +			   struct dma_buf *dmabuf, bool nonblock)
> +{
> +	struct device *dev = ib->indio_dev->dev.parent;
> +	struct iio_buffer *buffer = ib->buffer;
> +	struct dma_buf_attachment *attach = NULL;
> +	struct iio_dmabuf_priv *priv;
> +
> +	mutex_lock(&buffer->dmabufs_mutex);
> +
> +	list_for_each_entry(priv, &buffer->dmabufs, entry) {
> +		if (priv->attach->dev == dev
> +		    && priv->attach->dmabuf == dmabuf) {
> +			attach = priv->attach;
> +			break;
> +		}
> +	}
> +
> +	if (attach)
> +		iio_buffer_dmabuf_get(attach);
> +
> +	mutex_unlock(&buffer->dmabufs_mutex);
> +
> +	return attach ?: ERR_PTR(-EPERM);
> +}
> +
> +static int iio_buffer_attach_dmabuf(struct iio_dev_buffer_pair *ib,
> +				    int __user *user_fd, bool nonblock)
> +{
> +	struct iio_dev *indio_dev = ib->indio_dev;
> +	struct iio_buffer *buffer = ib->buffer;
> +	struct dma_buf_attachment *attach;
> +	struct iio_dmabuf_priv *priv;
> +	struct dma_buf *dmabuf;
> +	int err, fd;
> +
> +	if (!buffer->access->attach_dmabuf
> +	    || !buffer->access->detach_dmabuf
> +	    || !buffer->access->enqueue_dmabuf)
> +		return -EPERM;
> +
> +	if (copy_from_user(&fd, user_fd, sizeof(fd)))
> +		return -EFAULT;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&priv->lock);
> +	priv->context = dma_fence_context_alloc(1);
> +
> +	dmabuf = dma_buf_get(fd);
> +	if (IS_ERR(dmabuf)) {
> +		err = PTR_ERR(dmabuf);
> +		goto err_free_priv;
> +	}
> +
> +	attach = dma_buf_attach(dmabuf, indio_dev->dev.parent);
> +	if (IS_ERR(attach)) {
> +		err = PTR_ERR(attach);
> +		goto err_dmabuf_put;
> +	}
> +
> +	err = iio_dma_resv_lock(dmabuf, nonblock);
> +	if (err)
> +		goto err_dmabuf_detach;
> +
> +	priv->dir = buffer->direction == IIO_BUFFER_DIRECTION_IN
> +		? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +
> +	priv->sgt = dma_buf_map_attachment(attach, priv->dir);
> +	if (IS_ERR(priv->sgt)) {
> +		err = PTR_ERR(priv->sgt);
> +		dev_err(&indio_dev->dev, "Unable to map attachment: %d\n", err);
> +		goto err_resv_unlock;
> +	}
> +
> +	kref_init(&priv->ref);
> +	priv->buffer = buffer;
> +	priv->attach = attach;
> +	attach->importer_priv = priv;
> +
> +	priv->block = buffer->access->attach_dmabuf(buffer, attach);
> +	if (IS_ERR(priv->block)) {
> +		err = PTR_ERR(priv->block);
> +		goto err_dmabuf_unmap_attachment;
> +	}
> +
> +	dma_resv_unlock(dmabuf->resv);
> +

> +	mutex_lock(&buffer->dmabufs_mutex);
> +	list_add(&priv->entry, &buffer->dmabufs);
> +	mutex_unlock(&buffer->dmabufs_mutex);

Is it valid to attach the same dma_buf multiple times?

If not that code here should probably check that.

> +
> +	return 0;
> +
> +err_dmabuf_unmap_attachment:
> +	dma_buf_unmap_attachment(attach, priv->sgt, priv->dir);
> +err_resv_unlock:
> +	dma_resv_unlock(dmabuf->resv);
> +err_dmabuf_detach:
> +	dma_buf_detach(dmabuf, attach);
> +err_dmabuf_put:
> +	dma_buf_put(dmabuf);
> +err_free_priv:
> +	kfree(priv);
> +
> +	return err;
> +}
> +
> +static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair *ib,
> +				    int *user_req, bool nonblock)
> +{
> +	struct iio_buffer *buffer = ib->buffer;
> +	struct iio_dev *indio_dev = ib->indio_dev;
> +	struct iio_dmabuf_priv *priv;
> +	struct dma_buf *dmabuf;
> +	int dmabuf_fd, ret = -EPERM;

ENOENT is more common to use when something can't be found I think, but 
perfectly up to you what you prefer.

> +
> +	if (copy_from_user(&dmabuf_fd, user_req, sizeof(dmabuf_fd)))
> +		return -EFAULT;
> +
> +	dmabuf = dma_buf_get(dmabuf_fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	mutex_lock(&buffer->dmabufs_mutex);
> +
> +	list_for_each_entry(priv, &buffer->dmabufs, entry) {
> +		if (priv->attach->dev == indio_dev->dev.parent
> +		    && priv->attach->dmabuf == dmabuf) {
> +			list_del(&priv->entry);
> +
> +			/* Unref the reference from iio_buffer_attach_dmabuf() */
> +			iio_buffer_dmabuf_put(priv->attach);
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&buffer->dmabufs_mutex);
> +	dma_buf_put(dmabuf);
> +
> +	return ret;
> +}
> +
> +static const char *
> +iio_buffer_dma_fence_get_driver_name(struct dma_fence *fence)
> +{
> +	return "iio";
> +}
> +
> +static void iio_buffer_dma_fence_release(struct dma_fence *fence)
> +{
> +	struct iio_dma_fence *iio_fence =
> +		container_of(fence, struct iio_dma_fence, base);
> +
> +	kfree(iio_fence);
> +}
> +
> +static const struct dma_fence_ops iio_buffer_dma_fence_ops = {
> +	.get_driver_name	= iio_buffer_dma_fence_get_driver_name,
> +	.get_timeline_name	= iio_buffer_dma_fence_get_driver_name,
> +	.release		= iio_buffer_dma_fence_release,
> +};
> +
> +static int iio_buffer_enqueue_dmabuf(struct iio_dev_buffer_pair *ib,
> +				     struct iio_dmabuf __user *iio_dmabuf_req,
> +				     bool nonblock)
> +{
> +	struct iio_buffer *buffer = ib->buffer;
> +	struct iio_dmabuf iio_dmabuf;
> +	struct dma_buf_attachment *attach;
> +	struct iio_dmabuf_priv *priv;
> +	struct iio_dma_fence *fence;
> +	struct dma_buf *dmabuf;
> +	unsigned long timeout;
> +	bool cookie, cyclic, dma_to_ram;
> +	long retl;
> +	u32 seqno;
> +	int ret;
> +
> +	if (copy_from_user(&iio_dmabuf, iio_dmabuf_req, sizeof(iio_dmabuf)))
> +		return -EFAULT;
> +
> +	if (iio_dmabuf.flags & ~IIO_BUFFER_DMABUF_SUPPORTED_FLAGS)
> +		return -EINVAL;
> +
> +	cyclic = iio_dmabuf.flags & IIO_BUFFER_DMABUF_CYCLIC;
> +
> +	/* Cyclic flag is only supported on output buffers */
> +	if (cyclic && buffer->direction != IIO_BUFFER_DIRECTION_OUT)
> +		return -EINVAL;
> +
> +	dmabuf = dma_buf_get(iio_dmabuf.fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	if (!iio_dmabuf.bytes_used || iio_dmabuf.bytes_used > dmabuf->size) {
> +		ret = -EINVAL;
> +		goto err_dmabuf_put;
> +	}
> +
> +	attach = iio_buffer_find_attachment(ib, dmabuf, nonblock);
> +	if (IS_ERR(attach)) {
> +		ret = PTR_ERR(attach);
> +		goto err_dmabuf_put;
> +	}
> +
> +	priv = attach->importer_priv;
> +
> +	fence = kmalloc(sizeof(*fence), GFP_KERNEL);
> +	if (!fence) {
> +		ret = -ENOMEM;
> +		goto err_attachment_put;
> +	}
> +
> +	fence->priv = priv;
> +
> +	seqno = atomic_add_return(1, &priv->seqno);
> +
> +	/*
> +	 * The transfers are guaranteed to be processed in the order they are
> +	 * enqueued, so we can use a simple incrementing sequence number for
> +	 * the dma_fence.
> +	 */
> +	dma_fence_init(&fence->base, &iio_buffer_dma_fence_ops,
> +		       &priv->lock, priv->context, seqno);
> +
> +	ret = iio_dma_resv_lock(dmabuf, nonblock);
> +	if (ret)
> +		goto err_fence_put;
> +
> +	timeout = nonblock ? 0 : msecs_to_jiffies(DMABUF_ENQUEUE_TIMEOUT_MS);
> +
> +	/* Make sure we don't have writers */
> +	retl = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_WRITE,
> +				     true, timeout);
> +	if (retl == 0)
> +		retl = -EBUSY;
> +	if (retl < 0) {
> +		ret = (int)retl;
> +		goto err_resv_unlock;
> +	}
> +
> +	dma_to_ram = buffer->direction == IIO_BUFFER_DIRECTION_IN;
> +
> +	if (dma_to_ram) {
> +		/*
> +		 * If we're writing to the DMABUF, make sure we don't have
> +		 * readers
> +		 */
> +		retl = dma_resv_wait_timeout(dmabuf->resv,
> +					     DMA_RESV_USAGE_READ, true,
> +					     timeout);
> +		if (retl == 0)
> +			retl = -EBUSY;
> +		if (retl < 0) {
> +			ret = (int)retl;
> +			goto err_resv_unlock;
> +		}
> +	}
> +
> +	if (buffer->access->lock_queue)
> +		buffer->access->lock_queue(buffer);
> +
> +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> +	if (ret)
> +		goto err_queue_unlock;
> +
> +	dma_resv_add_fence(dmabuf->resv, &fence->base,
> +			   dma_resv_usage_rw(dma_to_ram));

That is incorrect use of the function dma_resv_usage_rw(). That function 
is for retrieving fences and not adding them.

See the function implementation and comments, when you use it like this 
you get exactly what you don't want.

Regards,
Christian.

> +	dma_resv_unlock(dmabuf->resv);
> +
> +	cookie = dma_fence_begin_signalling();
> +
> +	ret = buffer->access->enqueue_dmabuf(buffer, priv->block, &fence->base,
> +					     priv->sgt, iio_dmabuf.bytes_used,
> +					     cyclic);
> +	if (ret) {
> +		/*
> +		 * DMABUF enqueue failed, but we already added the fence.
> +		 * Signal the error through the fence completion mechanism.
> +		 */
> +		iio_buffer_signal_dmabuf_done(&fence->base, ret);
> +	}
> +
> +	if (buffer->access->unlock_queue)
> +		buffer->access->unlock_queue(buffer);
> +
> +	dma_fence_end_signalling(cookie);
> +	dma_buf_put(dmabuf);
> +
> +	return ret;
> +
> +err_queue_unlock:
> +	if (buffer->access->unlock_queue)
> +		buffer->access->unlock_queue(buffer);
> +err_resv_unlock:
> +	dma_resv_unlock(dmabuf->resv);
> +err_fence_put:
> +	dma_fence_put(&fence->base);
> +err_attachment_put:
> +	iio_buffer_dmabuf_put(attach);
> +err_dmabuf_put:
> +	dma_buf_put(dmabuf);
> +
> +	return ret;
> +}
> +
> +static void iio_buffer_cleanup(struct work_struct *work)
> +{
> +	struct iio_dma_fence *fence =
> +		container_of(work, struct iio_dma_fence, work);
> +	struct iio_dmabuf_priv *priv = fence->priv;
> +	struct dma_buf_attachment *attach = priv->attach;
> +
> +	dma_fence_put(&fence->base);
> +	iio_buffer_dmabuf_put(attach);
> +}
> +
> +void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int ret)
> +{
> +	struct iio_dma_fence *iio_fence =
> +		container_of(fence, struct iio_dma_fence, base);
> +	bool cookie = dma_fence_begin_signalling();
> +
> +	/*
> +	 * Get a reference to the fence, so that it's not freed as soon as
> +	 * it's signaled.
> +	 */
> +	dma_fence_get(fence);
> +
> +	fence->error = ret;
> +	dma_fence_signal(fence);
> +	dma_fence_end_signalling(cookie);
> +
> +	/*
> +	 * The fence will be unref'd in iio_buffer_cleanup.
> +	 * It can't be done here, as the unref functions might try to lock the
> +	 * resv object, which can deadlock.
> +	 */
> +	INIT_WORK(&iio_fence->work, iio_buffer_cleanup);
> +	schedule_work(&iio_fence->work);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_signal_dmabuf_done);
> +
> +static long iio_buffer_chrdev_ioctl(struct file *filp,
> +				    unsigned int cmd, unsigned long arg)
> +{
> +	struct iio_dev_buffer_pair *ib = filp->private_data;
> +	void __user *_arg = (void __user *)arg;
> +	bool nonblock = filp->f_flags & O_NONBLOCK;
> +
> +	switch (cmd) {
> +	case IIO_BUFFER_DMABUF_ATTACH_IOCTL:
> +		return iio_buffer_attach_dmabuf(ib, _arg, nonblock);
> +	case IIO_BUFFER_DMABUF_DETACH_IOCTL:
> +		return iio_buffer_detach_dmabuf(ib, _arg, nonblock);
> +	case IIO_BUFFER_DMABUF_ENQUEUE_IOCTL:
> +		return iio_buffer_enqueue_dmabuf(ib, _arg, nonblock);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>   static const struct file_operations iio_buffer_chrdev_fileops = {
>   	.owner = THIS_MODULE,
>   	.llseek = noop_llseek,
>   	.read = iio_buffer_read,
>   	.write = iio_buffer_write,
> +	.unlocked_ioctl = iio_buffer_chrdev_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
>   	.poll = iio_buffer_poll,
>   	.release = iio_buffer_chrdev_release,
>   };
> @@ -1953,6 +2414,7 @@ static void iio_buffer_release(struct kref *ref)
>   {
>   	struct iio_buffer *buffer = container_of(ref, struct iio_buffer, ref);
>   
> +	mutex_destroy(&buffer->dmabufs_mutex);
>   	buffer->access->release(buffer);
>   }
>   
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 89c3fd7c29ca..f4b1147291e5 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -9,8 +9,12 @@
>   #include <uapi/linux/iio/buffer.h>
>   #include <linux/iio/buffer.h>
>   
> +struct dma_buf_attachment;
> +struct dma_fence;
>   struct iio_dev;
> +struct iio_dma_buffer_block;
>   struct iio_buffer;
> +struct sg_table;
>   
>   /**
>    * INDIO_BUFFER_FLAG_FIXED_WATERMARK - Watermark level of the buffer can not be
> @@ -39,6 +43,13 @@ struct iio_buffer;
>    *                      device stops sampling. Calles are balanced with @enable.
>    * @release:		called when the last reference to the buffer is dropped,
>    *			should free all resources allocated by the buffer.
> + * @attach_dmabuf:	called from userspace via ioctl to attach one external
> + *			DMABUF.
> + * @detach_dmabuf:	called from userspace via ioctl to detach one previously
> + *			attached DMABUF.
> + * @enqueue_dmabuf:	called from userspace via ioctl to queue this DMABUF
> + *			object to this buffer. Requires a valid DMABUF fd, that
> + *			was previouly attached to this buffer.
>    * @modes:		Supported operating modes by this buffer type
>    * @flags:		A bitmask combination of INDIO_BUFFER_FLAG_*
>    *
> @@ -68,6 +79,17 @@ struct iio_buffer_access_funcs {
>   
>   	void (*release)(struct iio_buffer *buffer);
>   
> +	struct iio_dma_buffer_block * (*attach_dmabuf)(struct iio_buffer *buffer,
> +						       struct dma_buf_attachment *attach);
> +	void (*detach_dmabuf)(struct iio_buffer *buffer,
> +			      struct iio_dma_buffer_block *block);
> +	int (*enqueue_dmabuf)(struct iio_buffer *buffer,
> +			      struct iio_dma_buffer_block *block,
> +			      struct dma_fence *fence, struct sg_table *sgt,
> +			      size_t size, bool cyclic);
> +	void (*lock_queue)(struct iio_buffer *buffer);
> +	void (*unlock_queue)(struct iio_buffer *buffer);
> +
>   	unsigned int modes;
>   	unsigned int flags;
>   };
> @@ -136,6 +158,12 @@ struct iio_buffer {
>   
>   	/* @ref: Reference count of the buffer. */
>   	struct kref ref;
> +
> +	/* @dmabufs: List of DMABUF attachments */
> +	struct list_head dmabufs; /* P: dmabufs_mutex */
> +
> +	/* @dmabufs_mutex: Protects dmabufs */
> +	struct mutex dmabufs_mutex;
>   };
>   
>   /**
> @@ -156,9 +184,14 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>    **/
>   void iio_buffer_init(struct iio_buffer *buffer);
>   
> +void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach);
> +void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach);
> +
>   struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer);
>   void iio_buffer_put(struct iio_buffer *buffer);
>   
> +void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int ret);
> +
>   #else /* CONFIG_IIO_BUFFER */
>   
>   static inline void iio_buffer_get(struct iio_buffer *buffer) {}
> diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
> index 13939032b3f6..c666aa95e532 100644
> --- a/include/uapi/linux/iio/buffer.h
> +++ b/include/uapi/linux/iio/buffer.h
> @@ -5,6 +5,28 @@
>   #ifndef _UAPI_IIO_BUFFER_H_
>   #define _UAPI_IIO_BUFFER_H_
>   
> +#include <linux/types.h>
> +
> +/* Flags for iio_dmabuf.flags */
> +#define IIO_BUFFER_DMABUF_CYCLIC		(1 << 0)
> +#define IIO_BUFFER_DMABUF_SUPPORTED_FLAGS	0x00000001
> +
> +/**
> + * struct iio_dmabuf - Descriptor for a single IIO DMABUF object
> + * @fd:		file descriptor of the DMABUF object
> + * @flags:	one or more IIO_BUFFER_DMABUF_* flags
> + * @bytes_used:	number of bytes used in this DMABUF for the data transfer.
> + *		Should generally be set to the DMABUF's size.
> + */
> +struct iio_dmabuf {
> +	__u32 fd;
> +	__u32 flags;
> +	__u64 bytes_used;
> +};
> +
>   #define IIO_BUFFER_GET_FD_IOCTL			_IOWR('i', 0x91, int)
> +#define IIO_BUFFER_DMABUF_ATTACH_IOCTL		_IOW('i', 0x92, int)
> +#define IIO_BUFFER_DMABUF_DETACH_IOCTL		_IOW('i', 0x93, int)
> +#define IIO_BUFFER_DMABUF_ENQUEUE_IOCTL		_IOW('i', 0x94, struct iio_dmabuf)
>   
>   #endif /* _UAPI_IIO_BUFFER_H_ */
>

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Nuno Sá]

On Mon, 2024-03-04 at 13:44 +0100, Christian König wrote:
> Am 23.02.24 um 13:14 schrieb Nuno Sa:
> > From: Paul Cercueil <paul@crapouillou.net>
> > 
> > Add the necessary infrastructure to the IIO core to support a new
> > optional DMABUF based interface.
> > 
> > With this new interface, DMABUF objects (externally created) can be
> > attached to a IIO buffer, and subsequently used for data transfer.
> > 
> > A userspace application can then use this interface to share DMABUF
> > objects between several interfaces, allowing it to transfer data in a
> > zero-copy fashion, for instance between IIO and the USB stack.
> > 
> > The userspace application can also memory-map the DMABUF objects, and
> > access the sample data directly. The advantage of doing this vs. the
> > read() interface is that it avoids an extra copy of the data between the
> > kernel and userspace. This is particularly userful for high-speed
> > devices which produce several megabytes or even gigabytes of data per
> > second.
> > 
> > As part of the interface, 3 new IOCTLs have been added:
> > 
> > IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
> >   Attach the DMABUF object identified by the given file descriptor to the
> >   buffer.
> > 
> > IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
> >   Detach the DMABUF object identified by the given file descriptor from
> >   the buffer. Note that closing the IIO buffer's file descriptor will
> >   automatically detach all previously attached DMABUF objects.
> > 
> > IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
> >   Request a data transfer to/from the given DMABUF object. Its file
> >   descriptor, as well as the transfer size and flags are provided in the
> >   "iio_dmabuf" structure.
> > 
> > These three IOCTLs have to be performed on the IIO buffer's file
> > descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.
> 
> A few nit picks and one bug below, apart from that looks good to me.

Hi Christian,

Thanks for looking at it. I'll just add some comment on the bug below and for
the other stuff I hope Paul is already able to step in and reply to it. My
assumption (which seems to be wrong) is that the dmabuf bits should be already
good to go as they should be pretty similar to the USB part of it.

...

> 
> > +	if (dma_to_ram) {
> > +		/*
> > +		 * If we're writing to the DMABUF, make sure we don't have
> > +		 * readers
> > +		 */
> > +		retl = dma_resv_wait_timeout(dmabuf->resv,
> > +					     DMA_RESV_USAGE_READ, true,
> > +					     timeout);
> > +		if (retl == 0)
> > +			retl = -EBUSY;
> > +		if (retl < 0) {
> > +			ret = (int)retl;
> > +			goto err_resv_unlock;
> > +		}
> > +	}
> > +
> > +	if (buffer->access->lock_queue)
> > +		buffer->access->lock_queue(buffer);
> > +
> > +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > +	if (ret)
> > +		goto err_queue_unlock;
> > +
> > +	dma_resv_add_fence(dmabuf->resv, &fence->base,
> > +			   dma_resv_usage_rw(dma_to_ram));
> 
> That is incorrect use of the function dma_resv_usage_rw(). That function 
> is for retrieving fences and not adding them.
> 
> See the function implementation and comments, when you use it like this 
> you get exactly what you don't want.
> 

Does that mean that the USB stuff [1] is also wrong?

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/tree/drivers/usb/gadget/function/f_fs.c?h=usb-next#n1669

- Nuno Sá

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Christian König]

Trying to send this once more as text only.

Am 04.03.24 um 14:40 schrieb Christian König:
> Am 04.03.24 um 14:28 schrieb Nuno Sá:
>> On Mon, 2024-03-04 at 13:44 +0100, Christian König wrote:
>>> Am 23.02.24 um 13:14 schrieb Nuno Sa:
>>>> From: Paul Cercueil<paul@crapouillou.net>
>>>>
>>>> Add the necessary infrastructure to the IIO core to support a new
>>>> optional DMABUF based interface.
>>>>
>>>> With this new interface, DMABUF objects (externally created) can be
>>>> attached to a IIO buffer, and subsequently used for data transfer.
>>>>
>>>> A userspace application can then use this interface to share DMABUF
>>>> objects between several interfaces, allowing it to transfer data in a
>>>> zero-copy fashion, for instance between IIO and the USB stack.
>>>>
>>>> The userspace application can also memory-map the DMABUF objects, and
>>>> access the sample data directly. The advantage of doing this vs. the
>>>> read() interface is that it avoids an extra copy of the data between the
>>>> kernel and userspace. This is particularly userful for high-speed
>>>> devices which produce several megabytes or even gigabytes of data per
>>>> second.
>>>>
>>>> As part of the interface, 3 new IOCTLs have been added:
>>>>
>>>> IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
>>>>    Attach the DMABUF object identified by the given file descriptor to the
>>>>    buffer.
>>>>
>>>> IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
>>>>    Detach the DMABUF object identified by the given file descriptor from
>>>>    the buffer. Note that closing the IIO buffer's file descriptor will
>>>>    automatically detach all previously attached DMABUF objects.
>>>>
>>>> IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
>>>>    Request a data transfer to/from the given DMABUF object. Its file
>>>>    descriptor, as well as the transfer size and flags are provided in the
>>>>    "iio_dmabuf" structure.
>>>>
>>>> These three IOCTLs have to be performed on the IIO buffer's file
>>>> descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.
>>> A few nit picks and one bug below, apart from that looks good to me.
>> Hi Christian,
>>
>> Thanks for looking at it. I'll just add some comment on the bug below and for
>> the other stuff I hope Paul is already able to step in and reply to it. My
>> assumption (which seems to be wrong) is that the dmabuf bits should be already
>> good to go as they should be pretty similar to the USB part of it.
>>
>> ...
>>
>>>> +	if (dma_to_ram) {
>>>> +		/*
>>>> +		 * If we're writing to the DMABUF, make sure we don't have
>>>> +		 * readers
>>>> +		 */
>>>> +		retl = dma_resv_wait_timeout(dmabuf->resv,
>>>> +					     DMA_RESV_USAGE_READ, true,
>>>> +					     timeout);
>>>> +		if (retl == 0)
>>>> +			retl = -EBUSY;
>>>> +		if (retl < 0) {
>>>> +			ret = (int)retl;
>>>> +			goto err_resv_unlock;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (buffer->access->lock_queue)
>>>> +		buffer->access->lock_queue(buffer);
>>>> +
>>>> +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
>>>> +	if (ret)
>>>> +		goto err_queue_unlock;
>>>> +
>>>> +	dma_resv_add_fence(dmabuf->resv, &fence->base,
>>>> +			   dma_resv_usage_rw(dma_to_ram));
>>> That is incorrect use of the function dma_resv_usage_rw(). That function
>>> is for retrieving fences and not adding them.
>>>
>>> See the function implementation and comments, when you use it like this
>>> you get exactly what you don't want.
>>>
>> Does that mean that the USB stuff [1] is also wrong?
>>
>> [1]:https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/tree/drivers/usb/gadget/function/f_fs.c?h=usb-next#n1669
>
> Yes, that's broken as well. The dma_resv_usage_rw() function is 
> supposed to be used while retrieving fences.
>
> In other words your "if (dma_to_ram) ..." above is incorrect as well. 
> That one should look more like this:
> /*
>   * Writes needs to wait for other writes and reads, but readers only have to wait for writers.
>   */
>
> retl = dma_resv_wait_timeout(dmabuf->resv, dma_resv_usage_rw(dma_to_ram), timeout);
>
> Regards,
> Christian.
>
>> - Nuno Sá
>>
>>
>

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Paul Cercueil]

Hi Christian,

Le lundi 04 mars 2024 à 13:44 +0100, Christian König a écrit :
> Am 23.02.24 um 13:14 schrieb Nuno Sa:
> > From: Paul Cercueil <paul@crapouillou.net>
> > 
> > Add the necessary infrastructure to the IIO core to support a new
> > optional DMABUF based interface.
> > 
> > With this new interface, DMABUF objects (externally created) can be
> > attached to a IIO buffer, and subsequently used for data transfer.
> > 
> > A userspace application can then use this interface to share DMABUF
> > objects between several interfaces, allowing it to transfer data in
> > a
> > zero-copy fashion, for instance between IIO and the USB stack.
> > 
> > The userspace application can also memory-map the DMABUF objects,
> > and
> > access the sample data directly. The advantage of doing this vs.
> > the
> > read() interface is that it avoids an extra copy of the data
> > between the
> > kernel and userspace. This is particularly userful for high-speed
> > devices which produce several megabytes or even gigabytes of data
> > per
> > second.
> > 
> > As part of the interface, 3 new IOCTLs have been added:
> > 
> > IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
> >   Attach the DMABUF object identified by the given file descriptor
> > to the
> >   buffer.
> > 
> > IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
> >   Detach the DMABUF object identified by the given file descriptor
> > from
> >   the buffer. Note that closing the IIO buffer's file descriptor
> > will
> >   automatically detach all previously attached DMABUF objects.
> > 
> > IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
> >   Request a data transfer to/from the given DMABUF object. Its file
> >   descriptor, as well as the transfer size and flags are provided
> > in the
> >   "iio_dmabuf" structure.
> > 
> > These three IOCTLs have to be performed on the IIO buffer's file
> > descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.
> 
> A few nit picks and one bug below, apart from that looks good to me.
> 
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >   drivers/iio/industrialio-buffer.c | 462
> > ++++++++++++++++++++++++++++++++++++++
> >   include/linux/iio/buffer_impl.h   |  33 +++
> >   include/uapi/linux/iio/buffer.h   |  22 ++
> >   3 files changed, 517 insertions(+)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c
> > b/drivers/iio/industrialio-buffer.c
> > index b581a7e80566..0e63a09fa90a 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -9,14 +9,19 @@
> >    * - Better memory allocation techniques?
> >    * - Alternative access techniques?
> >    */
> > +#include <linux/atomic.h>
> >   #include <linux/anon_inodes.h>
> >   #include <linux/kernel.h>
> >   #include <linux/export.h>
> >   #include <linux/device.h>
> > +#include <linux/dma-buf.h>
> > +#include <linux/dma-fence.h>
> > +#include <linux/dma-resv.h>
> >   #include <linux/file.h>
> >   #include <linux/fs.h>
> >   #include <linux/cdev.h>
> >   #include <linux/slab.h>
> > +#include <linux/mm.h>
> >   #include <linux/poll.h>
> >   #include <linux/sched/signal.h>
> >   
> > @@ -28,6 +33,32 @@
> >   #include <linux/iio/buffer.h>
> >   #include <linux/iio/buffer_impl.h>
> >   
> > +#define DMABUF_ENQUEUE_TIMEOUT_MS 5000
> > +
> > +struct iio_dma_fence;
> > +
> > +struct iio_dmabuf_priv {
> > +	struct list_head entry;
> > +	struct kref ref;
> > +
> > +	struct iio_buffer *buffer;
> > +	struct iio_dma_buffer_block *block;
> > +
> > +	u64 context;
> > +	spinlock_t lock;
> > +
> > +	struct dma_buf_attachment *attach;
> > +	struct sg_table *sgt;
> > +	enum dma_data_direction dir;
> > +	atomic_t seqno;
> > +};
> > +
> > +struct iio_dma_fence {
> > +	struct dma_fence base;
> > +	struct iio_dmabuf_priv *priv;
> > +	struct work_struct work;
> > +};
> > +
> >   static const char * const iio_endian_prefix[] = {
> >   	[IIO_BE] = "be",
> >   	[IIO_LE] = "le",
> > @@ -332,6 +363,8 @@ void iio_buffer_init(struct iio_buffer *buffer)
> >   {
> >   	INIT_LIST_HEAD(&buffer->demux_list);
> >   	INIT_LIST_HEAD(&buffer->buffer_list);
> > +	INIT_LIST_HEAD(&buffer->dmabufs);
> > +	mutex_init(&buffer->dmabufs_mutex);
> >   	init_waitqueue_head(&buffer->pollq);
> >   	kref_init(&buffer->ref);
> >   	if (!buffer->watermark)
> > @@ -1519,14 +1552,62 @@ static void
> > iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev
> > *indio_dev)
> >   	kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> >   }
> >   
> > +static void iio_buffer_dmabuf_release(struct kref *ref)
> > +{
> > +	struct iio_dmabuf_priv *priv = container_of(ref, struct
> > iio_dmabuf_priv, ref);
> > +	struct dma_buf_attachment *attach = priv->attach;
> > +	struct iio_buffer *buffer = priv->buffer;
> > +	struct dma_buf *dmabuf = attach->dmabuf;
> > +
> > +	dma_resv_lock(dmabuf->resv, NULL);
> > +	dma_buf_unmap_attachment(attach, priv->sgt, priv->dir);
> > +	dma_resv_unlock(dmabuf->resv);
> > +
> > +	buffer->access->detach_dmabuf(buffer, priv->block);
> > +
> > +	dma_buf_detach(attach->dmabuf, attach);
> > +	dma_buf_put(dmabuf);
> > +	kfree(priv);
> > +}
> > +
> > +void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach)
> > +{
> > +	struct iio_dmabuf_priv *priv = attach->importer_priv;
> > +
> > +	kref_get(&priv->ref);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_get);
> > +
> > +void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach)
> > +{
> > +	struct iio_dmabuf_priv *priv = attach->importer_priv;
> > +
> > +	kref_put(&priv->ref, iio_buffer_dmabuf_release);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_put);
> > +
> >   static int iio_buffer_chrdev_release(struct inode *inode, struct
> > file *filep)
> >   {
> >   	struct iio_dev_buffer_pair *ib = filep->private_data;
> >   	struct iio_dev *indio_dev = ib->indio_dev;
> >   	struct iio_buffer *buffer = ib->buffer;
> > +	struct iio_dmabuf_priv *priv, *tmp;
> >   
> >   	wake_up(&buffer->pollq);
> >   
> > +	mutex_lock(&buffer->dmabufs_mutex);
> > +
> > +	/* Close all attached DMABUFs */
> > +	list_for_each_entry_safe(priv, tmp, &buffer->dmabufs,
> > entry) {
> > +		list_del_init(&priv->entry);
> > +		iio_buffer_dmabuf_put(priv->attach);
> > +	}
> > +
> > +	if (!list_empty(&buffer->dmabufs))
> > +		dev_warn(&indio_dev->dev, "Buffer FD closed with
> > active transfers\n");
> 
> That check here smells fishy.
> 
> Either the list_for_each_entry_safe() above has removed all entries
> and 
> then it can never happen or the buffer list is not always properly 
> protected by the mutex.
> 
> Do you really need that? If yes then please justify.

The dev_warn() can be removed, the buffers->dmabufs list will always be
empty at that point because of the loop above.

> BTW: When the transfers are async it's perfectly possible that there
> are 
> active transfers on close (when the application is just killed for
> example).
> 
> > +
> > +	mutex_unlock(&buffer->dmabufs_mutex);
> > +
> >   	kfree(ib);
> >   	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> >   	iio_device_put(indio_dev);
> > @@ -1534,11 +1615,391 @@ static int
> > iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
> >   	return 0;
> >   }
> >   
> > +static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool
> > nonblock)
> > +{
> > +	if (!nonblock)
> > +		return dma_resv_lock_interruptible(dmabuf->resv,
> > NULL);
> > +
> > +	if (!dma_resv_trylock(dmabuf->resv))
> > +		return -EBUSY;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct dma_buf_attachment *
> > +iio_buffer_find_attachment(struct iio_dev_buffer_pair *ib,
> > +			   struct dma_buf *dmabuf, bool nonblock)
> > +{
> > +	struct device *dev = ib->indio_dev->dev.parent;
> > +	struct iio_buffer *buffer = ib->buffer;
> > +	struct dma_buf_attachment *attach = NULL;
> > +	struct iio_dmabuf_priv *priv;
> > +
> > +	mutex_lock(&buffer->dmabufs_mutex);
> > +
> > +	list_for_each_entry(priv, &buffer->dmabufs, entry) {
> > +		if (priv->attach->dev == dev
> > +		    && priv->attach->dmabuf == dmabuf) {
> > +			attach = priv->attach;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (attach)
> > +		iio_buffer_dmabuf_get(attach);
> > +
> > +	mutex_unlock(&buffer->dmabufs_mutex);
> > +
> > +	return attach ?: ERR_PTR(-EPERM);
> > +}
> > +
> > +static int iio_buffer_attach_dmabuf(struct iio_dev_buffer_pair
> > *ib,
> > +				    int __user *user_fd, bool
> > nonblock)
> > +{
> > +	struct iio_dev *indio_dev = ib->indio_dev;
> > +	struct iio_buffer *buffer = ib->buffer;
> > +	struct dma_buf_attachment *attach;
> > +	struct iio_dmabuf_priv *priv;
> > +	struct dma_buf *dmabuf;
> > +	int err, fd;
> > +
> > +	if (!buffer->access->attach_dmabuf
> > +	    || !buffer->access->detach_dmabuf
> > +	    || !buffer->access->enqueue_dmabuf)
> > +		return -EPERM;
> > +
> > +	if (copy_from_user(&fd, user_fd, sizeof(fd)))
> > +		return -EFAULT;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_init(&priv->lock);
> > +	priv->context = dma_fence_context_alloc(1);
> > +
> > +	dmabuf = dma_buf_get(fd);
> > +	if (IS_ERR(dmabuf)) {
> > +		err = PTR_ERR(dmabuf);
> > +		goto err_free_priv;
> > +	}
> > +
> > +	attach = dma_buf_attach(dmabuf, indio_dev->dev.parent);
> > +	if (IS_ERR(attach)) {
> > +		err = PTR_ERR(attach);
> > +		goto err_dmabuf_put;
> > +	}
> > +
> > +	err = iio_dma_resv_lock(dmabuf, nonblock);
> > +	if (err)
> > +		goto err_dmabuf_detach;
> > +
> > +	priv->dir = buffer->direction == IIO_BUFFER_DIRECTION_IN
> > +		? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> > +
> > +	priv->sgt = dma_buf_map_attachment(attach, priv->dir);
> > +	if (IS_ERR(priv->sgt)) {
> > +		err = PTR_ERR(priv->sgt);
> > +		dev_err(&indio_dev->dev, "Unable to map
> > attachment: %d\n", err);
> > +		goto err_resv_unlock;
> > +	}
> > +
> > +	kref_init(&priv->ref);
> > +	priv->buffer = buffer;
> > +	priv->attach = attach;
> > +	attach->importer_priv = priv;
> > +
> > +	priv->block = buffer->access->attach_dmabuf(buffer,
> > attach);
> > +	if (IS_ERR(priv->block)) {
> > +		err = PTR_ERR(priv->block);
> > +		goto err_dmabuf_unmap_attachment;
> > +	}
> > +
> > +	dma_resv_unlock(dmabuf->resv);
> > +
> 
> > +	mutex_lock(&buffer->dmabufs_mutex);
> > +	list_add(&priv->entry, &buffer->dmabufs);
> > +	mutex_unlock(&buffer->dmabufs_mutex);
> 
> Is it valid to attach the same dma_buf multiple times?
> 
> If not that code here should probably check that.

Hmm, I remember that dma_buf_attach() wouldn't allow attaching multiple
times to the same device, but looking at the code, I can't find where
it was checked. Maybe it changed (or I just remember wrong).

I can add code to check that.

> 
> > +
> > +	return 0;
> > +
> > +err_dmabuf_unmap_attachment:
> > +	dma_buf_unmap_attachment(attach, priv->sgt, priv->dir);
> > +err_resv_unlock:
> > +	dma_resv_unlock(dmabuf->resv);
> > +err_dmabuf_detach:
> > +	dma_buf_detach(dmabuf, attach);
> > +err_dmabuf_put:
> > +	dma_buf_put(dmabuf);
> > +err_free_priv:
> > +	kfree(priv);
> > +
> > +	return err;
> > +}
> > +
> > +static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair
> > *ib,
> > +				    int *user_req, bool nonblock)
> > +{
> > +	struct iio_buffer *buffer = ib->buffer;
> > +	struct iio_dev *indio_dev = ib->indio_dev;
> > +	struct iio_dmabuf_priv *priv;
> > +	struct dma_buf *dmabuf;
> > +	int dmabuf_fd, ret = -EPERM;
> 
> ENOENT is more common to use when something can't be found I think,
> but 
> perfectly up to you what you prefer.

The way I see it, the detach is an "operation not permitted" if the
DMABUF hasn't been attached before.

> 
> > +
> > +	if (copy_from_user(&dmabuf_fd, user_req,
> > sizeof(dmabuf_fd)))
> > +		return -EFAULT;
> > +
> > +	dmabuf = dma_buf_get(dmabuf_fd);
> > +	if (IS_ERR(dmabuf))
> > +		return PTR_ERR(dmabuf);
> > +
> > +	mutex_lock(&buffer->dmabufs_mutex);
> > +
> > +	list_for_each_entry(priv, &buffer->dmabufs, entry) {
> > +		if (priv->attach->dev == indio_dev->dev.parent
> > +		    && priv->attach->dmabuf == dmabuf) {
> > +			list_del(&priv->entry);
> > +
> > +			/* Unref the reference from
> > iio_buffer_attach_dmabuf() */
> > +			iio_buffer_dmabuf_put(priv->attach);
> > +			ret = 0;
> > +			break;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&buffer->dmabufs_mutex);
> > +	dma_buf_put(dmabuf);
> > +
> > +	return ret;
> > +}
> > +
> > +static const char *
> > +iio_buffer_dma_fence_get_driver_name(struct dma_fence *fence)
> > +{
> > +	return "iio";
> > +}
> > +
> > +static void iio_buffer_dma_fence_release(struct dma_fence *fence)
> > +{
> > +	struct iio_dma_fence *iio_fence =
> > +		container_of(fence, struct iio_dma_fence, base);
> > +
> > +	kfree(iio_fence);
> > +}
> > +
> > +static const struct dma_fence_ops iio_buffer_dma_fence_ops = {
> > +	.get_driver_name	=
> > iio_buffer_dma_fence_get_driver_name,
> > +	.get_timeline_name	=
> > iio_buffer_dma_fence_get_driver_name,
> > +	.release		= iio_buffer_dma_fence_release,
> > +};
> > +
> > +static int iio_buffer_enqueue_dmabuf(struct iio_dev_buffer_pair
> > *ib,
> > +				     struct iio_dmabuf __user
> > *iio_dmabuf_req,
> > +				     bool nonblock)
> > +{
> > +	struct iio_buffer *buffer = ib->buffer;
> > +	struct iio_dmabuf iio_dmabuf;
> > +	struct dma_buf_attachment *attach;
> > +	struct iio_dmabuf_priv *priv;
> > +	struct iio_dma_fence *fence;
> > +	struct dma_buf *dmabuf;
> > +	unsigned long timeout;
> > +	bool cookie, cyclic, dma_to_ram;
> > +	long retl;
> > +	u32 seqno;
> > +	int ret;
> > +
> > +	if (copy_from_user(&iio_dmabuf, iio_dmabuf_req,
> > sizeof(iio_dmabuf)))
> > +		return -EFAULT;
> > +
> > +	if (iio_dmabuf.flags & ~IIO_BUFFER_DMABUF_SUPPORTED_FLAGS)
> > +		return -EINVAL;
> > +
> > +	cyclic = iio_dmabuf.flags & IIO_BUFFER_DMABUF_CYCLIC;
> > +
> > +	/* Cyclic flag is only supported on output buffers */
> > +	if (cyclic && buffer->direction !=
> > IIO_BUFFER_DIRECTION_OUT)
> > +		return -EINVAL;
> > +
> > +	dmabuf = dma_buf_get(iio_dmabuf.fd);
> > +	if (IS_ERR(dmabuf))
> > +		return PTR_ERR(dmabuf);
> > +
> > +	if (!iio_dmabuf.bytes_used || iio_dmabuf.bytes_used >
> > dmabuf->size) {
> > +		ret = -EINVAL;
> > +		goto err_dmabuf_put;
> > +	}
> > +
> > +	attach = iio_buffer_find_attachment(ib, dmabuf, nonblock);
> > +	if (IS_ERR(attach)) {
> > +		ret = PTR_ERR(attach);
> > +		goto err_dmabuf_put;
> > +	}
> > +
> > +	priv = attach->importer_priv;
> > +
> > +	fence = kmalloc(sizeof(*fence), GFP_KERNEL);
> > +	if (!fence) {
> > +		ret = -ENOMEM;
> > +		goto err_attachment_put;
> > +	}
> > +
> > +	fence->priv = priv;
> > +
> > +	seqno = atomic_add_return(1, &priv->seqno);
> > +
> > +	/*
> > +	 * The transfers are guaranteed to be processed in the
> > order they are
> > +	 * enqueued, so we can use a simple incrementing sequence
> > number for
> > +	 * the dma_fence.
> > +	 */
> > +	dma_fence_init(&fence->base, &iio_buffer_dma_fence_ops,
> > +		       &priv->lock, priv->context, seqno);
> > +
> > +	ret = iio_dma_resv_lock(dmabuf, nonblock);
> > +	if (ret)
> > +		goto err_fence_put;
> > +
> > +	timeout = nonblock ? 0 :
> > msecs_to_jiffies(DMABUF_ENQUEUE_TIMEOUT_MS);
> > +
> > +	/* Make sure we don't have writers */
> > +	retl = dma_resv_wait_timeout(dmabuf->resv,
> > DMA_RESV_USAGE_WRITE,
> > +				     true, timeout);
> > +	if (retl == 0)
> > +		retl = -EBUSY;
> > +	if (retl < 0) {
> > +		ret = (int)retl;
> > +		goto err_resv_unlock;
> > +	}
> > +
> > +	dma_to_ram = buffer->direction == IIO_BUFFER_DIRECTION_IN;
> > +
> > +	if (dma_to_ram) {
> > +		/*
> > +		 * If we're writing to the DMABUF, make sure we
> > don't have
> > +		 * readers
> > +		 */
> > +		retl = dma_resv_wait_timeout(dmabuf->resv,
> > +					     DMA_RESV_USAGE_READ,
> > true,
> > +					     timeout);
> > +		if (retl == 0)
> > +			retl = -EBUSY;
> > +		if (retl < 0) {
> > +			ret = (int)retl;
> > +			goto err_resv_unlock;
> > +		}
> > +	}
> > +
> > +	if (buffer->access->lock_queue)
> > +		buffer->access->lock_queue(buffer);
> > +
> > +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > +	if (ret)
> > +		goto err_queue_unlock;
> > +
> > +	dma_resv_add_fence(dmabuf->resv, &fence->base,
> > +			   dma_resv_usage_rw(dma_to_ram));
> 
> That is incorrect use of the function dma_resv_usage_rw(). That
> function 
> is for retrieving fences and not adding them.
> 
> See the function implementation and comments, when you use it like
> this 
> you get exactly what you don't want.

No, I get exactly what I want. If "dma_to_ram", I get
DMA_RESV_USAGE_READ, otherwise I get DMA_RESV_USAGE_WRITE.

If you really don't like the macro, I can inline things here.

Cheers,
-Paul

> 
> Regards,
> Christian.
> 
> > +	dma_resv_unlock(dmabuf->resv);
> > +
> > +	cookie = dma_fence_begin_signalling();
> > +
> > +	ret = buffer->access->enqueue_dmabuf(buffer, priv->block,
> > &fence->base,
> > +					     priv->sgt,
> > iio_dmabuf.bytes_used,
> > +					     cyclic);
> > +	if (ret) {
> > +		/*
> > +		 * DMABUF enqueue failed, but we already added the
> > fence.
> > +		 * Signal the error through the fence completion
> > mechanism.
> > +		 */
> > +		iio_buffer_signal_dmabuf_done(&fence->base, ret);
> > +	}
> > +
> > +	if (buffer->access->unlock_queue)
> > +		buffer->access->unlock_queue(buffer);
> > +
> > +	dma_fence_end_signalling(cookie);
> > +	dma_buf_put(dmabuf);
> > +
> > +	return ret;
> > +
> > +err_queue_unlock:
> > +	if (buffer->access->unlock_queue)
> > +		buffer->access->unlock_queue(buffer);
> > +err_resv_unlock:
> > +	dma_resv_unlock(dmabuf->resv);
> > +err_fence_put:
> > +	dma_fence_put(&fence->base);
> > +err_attachment_put:
> > +	iio_buffer_dmabuf_put(attach);
> > +err_dmabuf_put:
> > +	dma_buf_put(dmabuf);
> > +
> > +	return ret;
> > +}
> > +
> > +static void iio_buffer_cleanup(struct work_struct *work)
> > +{
> > +	struct iio_dma_fence *fence =
> > +		container_of(work, struct iio_dma_fence, work);
> > +	struct iio_dmabuf_priv *priv = fence->priv;
> > +	struct dma_buf_attachment *attach = priv->attach;
> > +
> > +	dma_fence_put(&fence->base);
> > +	iio_buffer_dmabuf_put(attach);
> > +}
> > +
> > +void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int
> > ret)
> > +{
> > +	struct iio_dma_fence *iio_fence =
> > +		container_of(fence, struct iio_dma_fence, base);
> > +	bool cookie = dma_fence_begin_signalling();
> > +
> > +	/*
> > +	 * Get a reference to the fence, so that it's not freed as
> > soon as
> > +	 * it's signaled.
> > +	 */
> > +	dma_fence_get(fence);
> > +
> > +	fence->error = ret;
> > +	dma_fence_signal(fence);
> > +	dma_fence_end_signalling(cookie);
> > +
> > +	/*
> > +	 * The fence will be unref'd in iio_buffer_cleanup.
> > +	 * It can't be done here, as the unref functions might try
> > to lock the
> > +	 * resv object, which can deadlock.
> > +	 */
> > +	INIT_WORK(&iio_fence->work, iio_buffer_cleanup);
> > +	schedule_work(&iio_fence->work);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_signal_dmabuf_done);
> > +
> > +static long iio_buffer_chrdev_ioctl(struct file *filp,
> > +				    unsigned int cmd, unsigned
> > long arg)
> > +{
> > +	struct iio_dev_buffer_pair *ib = filp->private_data;
> > +	void __user *_arg = (void __user *)arg;
> > +	bool nonblock = filp->f_flags & O_NONBLOCK;
> > +
> > +	switch (cmd) {
> > +	case IIO_BUFFER_DMABUF_ATTACH_IOCTL:
> > +		return iio_buffer_attach_dmabuf(ib, _arg,
> > nonblock);
> > +	case IIO_BUFFER_DMABUF_DETACH_IOCTL:
> > +		return iio_buffer_detach_dmabuf(ib, _arg,
> > nonblock);
> > +	case IIO_BUFFER_DMABUF_ENQUEUE_IOCTL:
> > +		return iio_buffer_enqueue_dmabuf(ib, _arg,
> > nonblock);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >   static const struct file_operations iio_buffer_chrdev_fileops = {
> >   	.owner = THIS_MODULE,
> >   	.llseek = noop_llseek,
> >   	.read = iio_buffer_read,
> >   	.write = iio_buffer_write,
> > +	.unlocked_ioctl = iio_buffer_chrdev_ioctl,
> > +	.compat_ioctl = compat_ptr_ioctl,
> >   	.poll = iio_buffer_poll,
> >   	.release = iio_buffer_chrdev_release,
> >   };
> > @@ -1953,6 +2414,7 @@ static void iio_buffer_release(struct kref
> > *ref)
> >   {
> >   	struct iio_buffer *buffer = container_of(ref, struct
> > iio_buffer, ref);
> >   
> > +	mutex_destroy(&buffer->dmabufs_mutex);
> >   	buffer->access->release(buffer);
> >   }
> >   
> > diff --git a/include/linux/iio/buffer_impl.h
> > b/include/linux/iio/buffer_impl.h
> > index 89c3fd7c29ca..f4b1147291e5 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -9,8 +9,12 @@
> >   #include <uapi/linux/iio/buffer.h>
> >   #include <linux/iio/buffer.h>
> >   
> > +struct dma_buf_attachment;
> > +struct dma_fence;
> >   struct iio_dev;
> > +struct iio_dma_buffer_block;
> >   struct iio_buffer;
> > +struct sg_table;
> >   
> >   /**
> >    * INDIO_BUFFER_FLAG_FIXED_WATERMARK - Watermark level of the
> > buffer can not be
> > @@ -39,6 +43,13 @@ struct iio_buffer;
> >    *                      device stops sampling. Calles are
> > balanced with @enable.
> >    * @release:		called when the last reference to the
> > buffer is dropped,
> >    *			should free all resources allocated by the
> > buffer.
> > + * @attach_dmabuf:	called from userspace via ioctl to attach
> > one external
> > + *			DMABUF.
> > + * @detach_dmabuf:	called from userspace via ioctl to detach
> > one previously
> > + *			attached DMABUF.
> > + * @enqueue_dmabuf:	called from userspace via ioctl to queue
> > this DMABUF
> > + *			object to this buffer. Requires a valid
> > DMABUF fd, that
> > + *			was previouly attached to this buffer.
> >    * @modes:		Supported operating modes by this buffer
> > type
> >    * @flags:		A bitmask combination of
> > INDIO_BUFFER_FLAG_*
> >    *
> > @@ -68,6 +79,17 @@ struct iio_buffer_access_funcs {
> >   
> >   	void (*release)(struct iio_buffer *buffer);
> >   
> > +	struct iio_dma_buffer_block * (*attach_dmabuf)(struct
> > iio_buffer *buffer,
> > +						       struct
> > dma_buf_attachment *attach);
> > +	void (*detach_dmabuf)(struct iio_buffer *buffer,
> > +			      struct iio_dma_buffer_block *block);
> > +	int (*enqueue_dmabuf)(struct iio_buffer *buffer,
> > +			      struct iio_dma_buffer_block *block,
> > +			      struct dma_fence *fence, struct
> > sg_table *sgt,
> > +			      size_t size, bool cyclic);
> > +	void (*lock_queue)(struct iio_buffer *buffer);
> > +	void (*unlock_queue)(struct iio_buffer *buffer);
> > +
> >   	unsigned int modes;
> >   	unsigned int flags;
> >   };
> > @@ -136,6 +158,12 @@ struct iio_buffer {
> >   
> >   	/* @ref: Reference count of the buffer. */
> >   	struct kref ref;
> > +
> > +	/* @dmabufs: List of DMABUF attachments */
> > +	struct list_head dmabufs; /* P: dmabufs_mutex */
> > +
> > +	/* @dmabufs_mutex: Protects dmabufs */
> > +	struct mutex dmabufs_mutex;
> >   };
> >   
> >   /**
> > @@ -156,9 +184,14 @@ int iio_update_buffers(struct iio_dev
> > *indio_dev,
> >    **/
> >   void iio_buffer_init(struct iio_buffer *buffer);
> >   
> > +void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach);
> > +void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach);
> > +
> >   struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer);
> >   void iio_buffer_put(struct iio_buffer *buffer);
> >   
> > +void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int
> > ret);
> > +
> >   #else /* CONFIG_IIO_BUFFER */
> >   
> >   static inline void iio_buffer_get(struct iio_buffer *buffer) {}
> > diff --git a/include/uapi/linux/iio/buffer.h
> > b/include/uapi/linux/iio/buffer.h
> > index 13939032b3f6..c666aa95e532 100644
> > --- a/include/uapi/linux/iio/buffer.h
> > +++ b/include/uapi/linux/iio/buffer.h
> > @@ -5,6 +5,28 @@
> >   #ifndef _UAPI_IIO_BUFFER_H_
> >   #define _UAPI_IIO_BUFFER_H_
> >   
> > +#include <linux/types.h>
> > +
> > +/* Flags for iio_dmabuf.flags */
> > +#define IIO_BUFFER_DMABUF_CYCLIC		(1 << 0)
> > +#define IIO_BUFFER_DMABUF_SUPPORTED_FLAGS	0x00000001
> > +
> > +/**
> > + * struct iio_dmabuf - Descriptor for a single IIO DMABUF object
> > + * @fd:		file descriptor of the DMABUF object
> > + * @flags:	one or more IIO_BUFFER_DMABUF_* flags
> > + * @bytes_used:	number of bytes used in this DMABUF for
> > the data transfer.
> > + *		Should generally be set to the DMABUF's size.
> > + */
> > +struct iio_dmabuf {
> > +	__u32 fd;
> > +	__u32 flags;
> > +	__u64 bytes_used;
> > +};
> > +
> >   #define IIO_BUFFER_GET_FD_IOCTL			_IOWR('i',
> > 0x91, int)
> > +#define IIO_BUFFER_DMABUF_ATTACH_IOCTL		_IOW('i', 0x92,
> > int)
> > +#define IIO_BUFFER_DMABUF_DETACH_IOCTL		_IOW('i', 0x93,
> > int)
> > +#define IIO_BUFFER_DMABUF_ENQUEUE_IOCTL		_IOW('i',
> > 0x94, struct iio_dmabuf)
> >   
> >   #endif /* _UAPI_IIO_BUFFER_H_ */
> > 
> 

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Paul Cercueil]

Le lundi 04 mars 2024 à 15:07 +0100, Christian König a écrit :
>  Am 04.03.24 um 14:59 schrieb Paul Cercueil:
>  
> > [SNIP]
> >  
> > >  
> > > >  
> > > > +	dma_to_ram = buffer->direction ==
> > > > IIO_BUFFER_DIRECTION_IN;
> > > > +
> > > > +	if (dma_to_ram) {
> > > > +		/*
> > > > +		 * If we're writing to the DMABUF, make sure
> > > > we
> > > > don't have
> > > > +		 * readers
> > > > +		 */
> > > > +		retl = dma_resv_wait_timeout(dmabuf->resv,
> > > > +					    
> > > > DMA_RESV_USAGE_READ,
> > > > true,
> > > > +					     timeout);
> > > > +		if (retl == 0)
> > > > +			retl = -EBUSY;
> > > > +		if (retl < 0) {
> > > > +			ret = (int)retl;
> > > > +			goto err_resv_unlock;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (buffer->access->lock_queue)
> > > > +		buffer->access->lock_queue(buffer);
> > > > +
> > > > +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > > > +	if (ret)
> > > > +		goto err_queue_unlock;
> > > > +
> > > > +	dma_resv_add_fence(dmabuf->resv, &fence->base,
> > > > +			   dma_resv_usage_rw(dma_to_ram));
> > > >  
> > >  
> > > That is incorrect use of the function dma_resv_usage_rw(). That
> > > function 
> > > is for retrieving fences and not adding them.
> > > 
> > > See the function implementation and comments, when you use it
> > > like
> > > this 
> > > you get exactly what you don't want.
> > > 
> > 
> > No, I get exactly what I want. If "dma_to_ram", I get
> > DMA_RESV_USAGE_READ, otherwise I get DMA_RESV_USAGE_WRITE.
> > 
> 
>  Ah, so basically !dma_to_ram means that you have a write access to
> the buffer?
>  

"dma_to_ram" means the data flows from the DMA to the RAM.

... Which means that it writes the buffer, so you are right, this is
wrong.

> > 
> > If you really don't like the macro, I can inline things here.
> 
>  Yeah, that would still be incorrect use.
>  
>  The dma__resv_usage_rw() is for retrieving the fences to sync to for
> read and write operations and should never be used together with
> dma_fence_resv_add().
> 

Ok, I'll inline it (and fix it) then.

Cheers,
-Paul

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Paul Cercueil]

Le lundi 04 mars 2024 à 14:41 +0100, Christian König a écrit :
> Trying to send this once more as text only.
> 
> Am 04.03.24 um 14:40 schrieb Christian König:
> > Am 04.03.24 um 14:28 schrieb Nuno Sá:
> > > On Mon, 2024-03-04 at 13:44 +0100, Christian König wrote:
> > > > Am 23.02.24 um 13:14 schrieb Nuno Sa:
> > > > > From: Paul Cercueil<paul@crapouillou.net>
> > > > > 
> > > > > Add the necessary infrastructure to the IIO core to support a
> > > > > new
> > > > > optional DMABUF based interface.
> > > > > 
> > > > > With this new interface, DMABUF objects (externally created)
> > > > > can be
> > > > > attached to a IIO buffer, and subsequently used for data
> > > > > transfer.
> > > > > 
> > > > > A userspace application can then use this interface to share
> > > > > DMABUF
> > > > > objects between several interfaces, allowing it to transfer
> > > > > data in a
> > > > > zero-copy fashion, for instance between IIO and the USB
> > > > > stack.
> > > > > 
> > > > > The userspace application can also memory-map the DMABUF
> > > > > objects, and
> > > > > access the sample data directly. The advantage of doing this
> > > > > vs. the
> > > > > read() interface is that it avoids an extra copy of the data
> > > > > between the
> > > > > kernel and userspace. This is particularly userful for high-
> > > > > speed
> > > > > devices which produce several megabytes or even gigabytes of
> > > > > data per
> > > > > second.
> > > > > 
> > > > > As part of the interface, 3 new IOCTLs have been added:
> > > > > 
> > > > > IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
> > > > >    Attach the DMABUF object identified by the given file
> > > > > descriptor to the
> > > > >    buffer.
> > > > > 
> > > > > IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
> > > > >    Detach the DMABUF object identified by the given file
> > > > > descriptor from
> > > > >    the buffer. Note that closing the IIO buffer's file
> > > > > descriptor will
> > > > >    automatically detach all previously attached DMABUF
> > > > > objects.
> > > > > 
> > > > > IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
> > > > >    Request a data transfer to/from the given DMABUF object.
> > > > > Its file
> > > > >    descriptor, as well as the transfer size and flags are
> > > > > provided in the
> > > > >    "iio_dmabuf" structure.
> > > > > 
> > > > > These three IOCTLs have to be performed on the IIO buffer's
> > > > > file
> > > > > descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL()
> > > > > ioctl.
> > > > A few nit picks and one bug below, apart from that looks good
> > > > to me.
> > > Hi Christian,
> > > 
> > > Thanks for looking at it. I'll just add some comment on the bug
> > > below and for
> > > the other stuff I hope Paul is already able to step in and reply
> > > to it. My
> > > assumption (which seems to be wrong) is that the dmabuf bits
> > > should be already
> > > good to go as they should be pretty similar to the USB part of
> > > it.
> > > 
> > > ...
> > > 
> > > > > +	if (dma_to_ram) {
> > > > > +		/*
> > > > > +		 * If we're writing to the DMABUF, make sure
> > > > > we don't have
> > > > > +		 * readers
> > > > > +		 */
> > > > > +		retl = dma_resv_wait_timeout(dmabuf->resv,
> > > > > +					    
> > > > > DMA_RESV_USAGE_READ, true,
> > > > > +					     timeout);
> > > > > +		if (retl == 0)
> > > > > +			retl = -EBUSY;
> > > > > +		if (retl < 0) {
> > > > > +			ret = (int)retl;
> > > > > +			goto err_resv_unlock;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	if (buffer->access->lock_queue)
> > > > > +		buffer->access->lock_queue(buffer);
> > > > > +
> > > > > +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > > > > +	if (ret)
> > > > > +		goto err_queue_unlock;
> > > > > +
> > > > > +	dma_resv_add_fence(dmabuf->resv, &fence->base,
> > > > > +			   dma_resv_usage_rw(dma_to_ram));
> > > > That is incorrect use of the function dma_resv_usage_rw(). That
> > > > function
> > > > is for retrieving fences and not adding them.
> > > > 
> > > > See the function implementation and comments, when you use it
> > > > like this
> > > > you get exactly what you don't want.
> > > > 
> > > Does that mean that the USB stuff [1] is also wrong?
> > > 
> > > [1]:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/tr
> > > ee/drivers/usb/gadget/function/f_fs.c?h=usb-next#n1669
> > 
> > Yes, that's broken as well. The dma_resv_usage_rw() function is 
> > supposed to be used while retrieving fences.

Ok, I'll fix it there too.

> > 
> > In other words your "if (dma_to_ram) ..." above is incorrect as
> > well. 
> > That one should look more like this:
> > /*
> >   * Writes needs to wait for other writes and reads, but readers
> > only have to wait for writers.
> >   */
> > 
> > retl = dma_resv_wait_timeout(dmabuf->resv,
> > dma_resv_usage_rw(dma_to_ram), timeout);

When writing the DMABUF, the USB code (and the IIO code above) will
wait for writers/readers, but it does so through two consecutive calls
to dma_resv_wait_timeout (because I did not know the proper usage - I
thought I had to check both manually).

So this code block should technically be correct; but I'll update this
code nonetheless.

> > Regards,
> > Christian.

Cheers,
-Paul

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

[Christian König]

Am 04.03.24 um 15:29 schrieb Paul Cercueil:
> Le lundi 04 mars 2024 à 14:41 +0100, Christian König a écrit :
>> Trying to send this once more as text only.
>>
>> Am 04.03.24 um 14:40 schrieb Christian König:
>>> Am 04.03.24 um 14:28 schrieb Nuno Sá:
>>>> On Mon, 2024-03-04 at 13:44 +0100, Christian König wrote:
>>>>> Am 23.02.24 um 13:14 schrieb Nuno Sa:
>>>>>> From: Paul Cercueil<paul@crapouillou.net>
>>>>>>
>>>>>> Add the necessary infrastructure to the IIO core to support a
>>>>>> new
>>>>>> optional DMABUF based interface.
>>>>>>
>>>>>> With this new interface, DMABUF objects (externally created)
>>>>>> can be
>>>>>> attached to a IIO buffer, and subsequently used for data
>>>>>> transfer.
>>>>>>
>>>>>> A userspace application can then use this interface to share
>>>>>> DMABUF
>>>>>> objects between several interfaces, allowing it to transfer
>>>>>> data in a
>>>>>> zero-copy fashion, for instance between IIO and the USB
>>>>>> stack.
>>>>>>
>>>>>> The userspace application can also memory-map the DMABUF
>>>>>> objects, and
>>>>>> access the sample data directly. The advantage of doing this
>>>>>> vs. the
>>>>>> read() interface is that it avoids an extra copy of the data
>>>>>> between the
>>>>>> kernel and userspace. This is particularly userful for high-
>>>>>> speed
>>>>>> devices which produce several megabytes or even gigabytes of
>>>>>> data per
>>>>>> second.
>>>>>>
>>>>>> As part of the interface, 3 new IOCTLs have been added:
>>>>>>
>>>>>> IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
>>>>>>     Attach the DMABUF object identified by the given file
>>>>>> descriptor to the
>>>>>>     buffer.
>>>>>>
>>>>>> IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
>>>>>>     Detach the DMABUF object identified by the given file
>>>>>> descriptor from
>>>>>>     the buffer. Note that closing the IIO buffer's file
>>>>>> descriptor will
>>>>>>     automatically detach all previously attached DMABUF
>>>>>> objects.
>>>>>>
>>>>>> IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
>>>>>>     Request a data transfer to/from the given DMABUF object.
>>>>>> Its file
>>>>>>     descriptor, as well as the transfer size and flags are
>>>>>> provided in the
>>>>>>     "iio_dmabuf" structure.
>>>>>>
>>>>>> These three IOCTLs have to be performed on the IIO buffer's
>>>>>> file
>>>>>> descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL()
>>>>>> ioctl.
>>>>> A few nit picks and one bug below, apart from that looks good
>>>>> to me.
>>>> Hi Christian,
>>>>
>>>> Thanks for looking at it. I'll just add some comment on the bug
>>>> below and for
>>>> the other stuff I hope Paul is already able to step in and reply
>>>> to it. My
>>>> assumption (which seems to be wrong) is that the dmabuf bits
>>>> should be already
>>>> good to go as they should be pretty similar to the USB part of
>>>> it.
>>>>
>>>> ...
>>>>
>>>>>> +	if (dma_to_ram) {
>>>>>> +		/*
>>>>>> +		 * If we're writing to the DMABUF, make sure
>>>>>> we don't have
>>>>>> +		 * readers
>>>>>> +		 */
>>>>>> +		retl = dma_resv_wait_timeout(dmabuf->resv,
>>>>>> +					
>>>>>> DMA_RESV_USAGE_READ, true,
>>>>>> +					     timeout);
>>>>>> +		if (retl == 0)
>>>>>> +			retl = -EBUSY;
>>>>>> +		if (retl < 0) {
>>>>>> +			ret = (int)retl;
>>>>>> +			goto err_resv_unlock;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	if (buffer->access->lock_queue)
>>>>>> +		buffer->access->lock_queue(buffer);
>>>>>> +
>>>>>> +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
>>>>>> +	if (ret)
>>>>>> +		goto err_queue_unlock;
>>>>>> +
>>>>>> +	dma_resv_add_fence(dmabuf->resv, &fence->base,
>>>>>> +			   dma_resv_usage_rw(dma_to_ram));
>>>>> That is incorrect use of the function dma_resv_usage_rw(). That
>>>>> function
>>>>> is for retrieving fences and not adding them.
>>>>>
>>>>> See the function implementation and comments, when you use it
>>>>> like this
>>>>> you get exactly what you don't want.
>>>>>
>>>> Does that mean that the USB stuff [1] is also wrong?
>>>>
>>>> [1]:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/tr
>>>> ee/drivers/usb/gadget/function/f_fs.c?h=usb-next#n1669
>>> Yes, that's broken as well. The dma_resv_usage_rw() function is
>>> supposed to be used while retrieving fences.
> Ok, I'll fix it there too.
>
>>> In other words your "if (dma_to_ram) ..." above is incorrect as
>>> well.
>>> That one should look more like this:
>>> /*
>>>    * Writes needs to wait for other writes and reads, but readers
>>> only have to wait for writers.
>>>    */
>>>
>>> retl = dma_resv_wait_timeout(dmabuf->resv,
>>> dma_resv_usage_rw(dma_to_ram), timeout);
> When writing the DMABUF, the USB code (and the IIO code above) will
> wait for writers/readers, but it does so through two consecutive calls
> to dma_resv_wait_timeout (because I did not know the proper usage - I
> thought I had to check both manually).

Yeah, see the documentation on the dma_resv_usage enum. Basically you 
have KERNEL>WRITE>READ>BOOKKEEP.

When waiting for READ you automatically wait for WRITE and KERNEL as 
well. So no need for two calls to the wait function.

If you have any idea how to improve the documentation feel free to 
suggest, it's certainly not obvious how that works :)

Cheers,
Christian.

>
> So this code block should technically be correct; but I'll update this
> code nonetheless.
>
>>> Regards,
>>> Christian.
> Cheers,
> -Paul

Re: [PATCH v7 0/6] iio: new DMABUF based API

[Jonathan Cameron]

On Mon, 04 Mar 2024 08:59:47 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sun, 2024-03-03 at 17:42 +0000, Jonathan Cameron wrote:
> > On Fri, 23 Feb 2024 13:13:58 +0100
> > Nuno Sa <nuno.sa@analog.com> wrote:
> >   
> > > Hi Jonathan, likely you're wondering why I'm sending v7. Well, to be
> > > honest, we're hoping to get this merged this for the 6.9 merge window.
> > > Main reason is because the USB part is already in (so it would be nice
> > > to get the whole thing in). Moreover, the changes asked in v6 were simple
> > > (even though I'm not quite sure in one of them) and Paul has no access to
> > > it's laptop so he can't send v7 himself. So he kind of said/asked for me to
> > > do it.  
> > 
> > So, we are cutting this very fine. If Linus hints strongly at an rc8 maybe we
> > can sneak this in. However, I need an Ack from Vinod for the dma engine
> > changes first.
> > 
> > Also I'd love a final 'looks ok' comment from DMABUF folk (Ack even better!)
> > 
> > Seems that the other side got resolved in the USB gadget, but last we heard
> > form
> > Daniel and Christian looks to have been back on v5. I'd like them to confirm
> > they are fine with the changes made as a result. 
> >   
> 
> I can ask Christian or Daniel for some acks but my feeling (I still need, at
> some point, to get really familiar with all of this) is that this should be
> pretty similar to the USB series (from a DMABUF point of view) as they are both
> importers.
> 
> > I've been happy with the IIO parts for a few versions now but my ability to
> > review
> > the DMABUF and DMA engine bits is limited.
> > 
> > A realistic path to get this in is rc8 is happening, is all Acks in place by
> > Wednesday,
> > I get apply it and hits Linux-next Thursday, Pull request to Greg on Saturday
> > and Greg
> > is feeling particularly generous to take one on the day he normally closes his
> > trees.
> >   
> 
> Well, it looks like we still have a shot. I'll try to see if Vinod is fine with
> the DMAENGINE stuff.
> 

Sadly, looks like rc7 was at the end of a quiet week, so almost certain to not
be an rc8 in the end. Let's aim to get this in at the start of the next cycle
so we can build on it from there.


Jonathan

> - Nuno Sá
> 
> 

Re: [PATCH v7 0/6] iio: new DMABUF based API

[Paul Cercueil]

Hi Jonathan,

Le mardi 05 mars 2024 à 10:07 +0000, Jonathan Cameron a écrit :
> On Mon, 04 Mar 2024 08:59:47 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Sun, 2024-03-03 at 17:42 +0000, Jonathan Cameron wrote:
> > > On Fri, 23 Feb 2024 13:13:58 +0100
> > > Nuno Sa <nuno.sa@analog.com> wrote:
> > >   
> > > > Hi Jonathan, likely you're wondering why I'm sending v7. Well,
> > > > to be
> > > > honest, we're hoping to get this merged this for the 6.9 merge
> > > > window.
> > > > Main reason is because the USB part is already in (so it would
> > > > be nice
> > > > to get the whole thing in). Moreover, the changes asked in v6
> > > > were simple
> > > > (even though I'm not quite sure in one of them) and Paul has no
> > > > access to
> > > > it's laptop so he can't send v7 himself. So he kind of
> > > > said/asked for me to
> > > > do it.  
> > > 
> > > So, we are cutting this very fine. If Linus hints strongly at an
> > > rc8 maybe we
> > > can sneak this in. However, I need an Ack from Vinod for the dma
> > > engine
> > > changes first.
> > > 
> > > Also I'd love a final 'looks ok' comment from DMABUF folk (Ack
> > > even better!)
> > > 
> > > Seems that the other side got resolved in the USB gadget, but
> > > last we heard
> > > form
> > > Daniel and Christian looks to have been back on v5. I'd like them
> > > to confirm
> > > they are fine with the changes made as a result. 
> > >   
> > 
> > I can ask Christian or Daniel for some acks but my feeling (I still
> > need, at
> > some point, to get really familiar with all of this) is that this
> > should be
> > pretty similar to the USB series (from a DMABUF point of view) as
> > they are both
> > importers.
> > 
> > > I've been happy with the IIO parts for a few versions now but my
> > > ability to
> > > review
> > > the DMABUF and DMA engine bits is limited.
> > > 
> > > A realistic path to get this in is rc8 is happening, is all Acks
> > > in place by
> > > Wednesday,
> > > I get apply it and hits Linux-next Thursday, Pull request to Greg
> > > on Saturday
> > > and Greg
> > > is feeling particularly generous to take one on the day he
> > > normally closes his
> > > trees.
> > >   
> > 
> > Well, it looks like we still have a shot. I'll try to see if Vinod
> > is fine with
> > the DMAENGINE stuff.
> > 
> 
> Sadly, looks like rc7 was at the end of a quiet week, so almost
> certain to not
> be an rc8 in the end. Let's aim to get this in at the start of the
> next cycle
> so we can build on it from there.

And it looks like I'll need a V8 for the few things noted by Christian.

Having it in 6.9 would have been great but having it eventually merged
is all that matters - so I'm fine to have it queued for 6.10 instead.

Cheers,
-Paul