Hi Hillf,

Le mardi 04 avril 2023 à 09:59 +0800, Hillf Danton a écrit :

On 3 Apr 2023 17:47:50 +0200 Paul Cercueil paul@crapouillou.net

...

This function can be used to initiate a scatter-gather DMA transfer where the DMA addresses and lengths are located inside arrays.

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.

Given sg's wayout and conceptually iovec and kvec (in include/linux/uio.h), what you add should have been dma_vec to ease people making use of it.

struct dma_vec {                 dma_addr_t      addr;                 size_t          len;         };

Well it's not too late ;)

Thanks for the feedback.

Cheers, -Paul

...

Signed-off-by: Paul Cercueil paul@crapouillou.net

---

v3: New patch

include/linux/dmaengine.h | 16 ++++++++++++++++  1 file changed, 16 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index c3656e590213..62efa28c009a 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -912,6 +912,11 @@ 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_slave_dma_array)( +               struct dma_chan *chan, dma_addr_t *addrs, +               size_t *lengths, size_t nb, +               enum dma_transfer_direction direction, +               unsigned long flags);

Then the callback looks like

struct dma_async_tx_descriptor *(*device_prep_slave_vec)(                 struct dma_chan *chan,                 struct dma_vec *vec,                 int nvec,                 enum dma_transfer_direction direction,                 unsigned long flags);

On 03-04-23, 17:47, Paul Cercueil wrote:

This function can be used to initiate a scatter-gather DMA transfer where the DMA addresses and lengths are located inside arrays.

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.

Do we need a new API for this? why not use device_prep_interleaved_dma?

Signed-off-by: Paul Cercueil paul@crapouillou.net

---

v3: New patch

include/linux/dmaengine.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index c3656e590213..62efa28c009a 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -912,6 +912,11 @@ 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_slave_dma_array)(

• struct dma_chan *chan, dma_addr_t *addrs,
  

• size_t *lengths, size_t nb,
  

• enum dma_transfer_direction direction,
  

• 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,

@@ -974,6 +979,17 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single( dir, flags, NULL); } +static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_dma_array(

• struct dma_chan *chan, dma_addr_t *addrs, size_t *lengths,
• size_t nb, enum dma_transfer_direction dir, unsigned long flags)

+{

• if (!chan || !chan->device || !chan->device->device_prep_slave_dma_array)

• return NULL;
  

• return chan->device->device_prep_slave_dma_array(chan, addrs, lengths,

• 					 nb, dir, 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.39.2

On Mon, 3 Apr 2023 17:47:53 +0200 Paul Cercueil paul@crapouillou.net wrote:

Adding write support to the buffer-dma code is easy - the write() function basically needs to do the exact same thing as the read() function: dequeue a block, read or write the data, enqueue the block when entirely processed.

Therefore, the iio_buffer_dma_read() and the new iio_buffer_dma_write() now both call a function iio_buffer_dma_io(), which will perform this task.

The .space_available() callback can return the exact same value as the .data_available() callback for input buffers, since in both cases we count the exact same thing (the number of bytes in each available block).

As they are doing the same thing, I'd like that to be visible down where the callback is set. As such, do we need a wrapper to provide the space available version? Perhaps just give the data_available version a more generic name to make it seem appropriate for both usecases?

Otherwise LGTM.

Jonathan

Note that we preemptively reset block->bytes_used to the buffer's size in iio_dma_buffer_request_update(), as in the future the iio_dma_buffer_enqueue() function won't reset it.

Signed-off-by: Paul Cercueil paul@crapouillou.net Reviewed-by: Alexandru Ardelean ardeleanalex@gmail.com

---

v2: - Fix block->state not being reset in iio_dma_buffer_request_update() for output buffers. - Only update block->bytes_used once and add a comment about why we update it. - Add a comment about why we're setting a different state for output buffers in iio_dma_buffer_request_update() - Remove useless cast to bool (!!) in iio_dma_buffer_io()

v3: - Reorganize arguments to iio_dma_buffer_io() - Change 'is_write' argument to 'is_from_user' - Change (__force char *) to (__force __user char *), in iio_dma_buffer_write(), since we only want to drop the "const".

---

drivers/iio/buffer/industrialio-buffer-dma.c | 89 ++++++++++++++++---- include/linux/iio/buffer-dma.h | 7 ++ 2 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index 1fc91467d1aa..86eced458236 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -195,6 +195,18 @@ static void _iio_dma_buffer_block_done(struct iio_dma_buffer_block *block) block->state = IIO_BLOCK_STATE_DONE; } +static void iio_dma_buffer_queue_wake(struct iio_dma_buffer_queue *queue) +{

• __poll_t flags;
• if (queue->buffer.direction == IIO_BUFFER_DIRECTION_IN)

• flags = EPOLLIN | EPOLLRDNORM;
  

• else

• flags = EPOLLOUT | EPOLLWRNORM;
  

• wake_up_interruptible_poll(&queue->buffer.pollq, flags);

+}

/**

• iio_dma_buffer_block_done() - Indicate that a block has been completed
• @block: The completed block

@@ -212,7 +224,7 @@ void iio_dma_buffer_block_done(struct iio_dma_buffer_block *block) spin_unlock_irqrestore(&queue->list_lock, flags); iio_buffer_block_put_atomic(block);

• wake_up_interruptible_poll(&queue->buffer.pollq, EPOLLIN | EPOLLRDNORM);

• iio_dma_buffer_queue_wake(queue);

} EXPORT_SYMBOL_GPL(iio_dma_buffer_block_done); @@ -241,7 +253,7 @@ void iio_dma_buffer_block_list_abort(struct iio_dma_buffer_queue *queue, } spin_unlock_irqrestore(&queue->list_lock, flags);

• wake_up_interruptible_poll(&queue->buffer.pollq, EPOLLIN | EPOLLRDNORM);

• iio_dma_buffer_queue_wake(queue);

} EXPORT_SYMBOL_GPL(iio_dma_buffer_block_list_abort); @@ -335,8 +347,24 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer) queue->fileio.blocks[i] = block; }

• block->state = IIO_BLOCK_STATE_QUEUED;
  

• list_add_tail(&block->head, &queue->incoming);
  

• /*
  

•  * block->bytes_used may have been modified previously, e.g. by
  

•  * iio_dma_buffer_block_list_abort(). Reset it here to the
  

•  * block's so that iio_dma_buffer_io() will work.
  

•  */
  

• block->bytes_used = block->size;
  

• /*
  

•  * If it's an input buffer, mark the block as queued, and
  

•  * iio_dma_buffer_enable() will submit it. Otherwise mark it as
  

•  * done, which means it's ready to be dequeued.
  

•  */
  

• if (queue->buffer.direction == IIO_BUFFER_DIRECTION_IN) {
  

• 	block->state = IIO_BLOCK_STATE_QUEUED;
  

• 	list_add_tail(&block->head, &queue->incoming);
  

• } else {
  

• 	block->state = IIO_BLOCK_STATE_DONE;
  

• }
  

  }

out_unlock: @@ -465,20 +493,12 @@ static struct iio_dma_buffer_block *iio_dma_buffer_dequeue( return block; } -/**

• • iio_dma_buffer_read() - DMA buffer read callback
• • @buffer: Buffer to read form
• • @n: Number of bytes to read
• • @user_buffer: Userspace buffer to copy the data to
• • Should be used as the read callback for iio_buffer_access_ops
• • struct for DMA buffers.
• */

-int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n,

• char __user *user_buffer)

+static int iio_dma_buffer_io(struct iio_buffer *buffer, size_t n,

• 	     char __user *user_buffer, bool is_from_user)
  

{ struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer); struct iio_dma_buffer_block *block;

• void *addr; int ret;

if (n < buffer->bytes_per_datum) @@ -501,8 +521,13 @@ int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n, n = rounddown(n, buffer->bytes_per_datum); if (n > block->bytes_used - queue->fileio.pos) n = block->bytes_used - queue->fileio.pos;

• addr = block->vaddr + queue->fileio.pos;

• if (copy_to_user(user_buffer, block->vaddr + queue->fileio.pos, n)) {

• if (is_from_user)

• ret = copy_from_user(addr, user_buffer, n);
  

• else

• ret = copy_to_user(user_buffer, addr, n);
  

• if (ret) { ret = -EFAULT; goto out_unlock; }

@@ -521,8 +546,40 @@ int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n, return ret; }

+/**

• • iio_dma_buffer_read() - DMA buffer read callback
• • @buffer: Buffer to read form
• • @n: Number of bytes to read
• • @user_buffer: Userspace buffer to copy the data to
• • Should be used as the read callback for iio_buffer_access_ops
• • struct for DMA buffers.
• */

+int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n,

• char __user *user_buffer)

+{

• return iio_dma_buffer_io(buffer, n, user_buffer, false);

+} EXPORT_SYMBOL_GPL(iio_dma_buffer_read); +/**

• • iio_dma_buffer_write() - DMA buffer write callback
• • @buffer: Buffer to read form
• • @n: Number of bytes to read
• • @user_buffer: Userspace buffer to copy the data from
• • Should be used as the write callback for iio_buffer_access_ops
• • struct for DMA buffers.
• */

+int iio_dma_buffer_write(struct iio_buffer *buffer, size_t n,

• 	 const char __user *user_buffer)
  

+{

• return iio_dma_buffer_io(buffer, n,

• 		 (__force __user char *)user_buffer, true);
  

+} +EXPORT_SYMBOL_GPL(iio_dma_buffer_write);

/**

• iio_dma_buffer_data_available() - DMA buffer data_available callback
• @buf: Buffer to check for data availability

diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h index 18d3702fa95d..490b93f76fa8 100644 --- a/include/linux/iio/buffer-dma.h +++ b/include/linux/iio/buffer-dma.h @@ -132,6 +132,8 @@ int iio_dma_buffer_disable(struct iio_buffer *buffer, struct iio_dev *indio_dev); int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n, char __user *user_buffer); +int iio_dma_buffer_write(struct iio_buffer *buffer, size_t n,

• 	 const char __user *user_buffer);
  

size_t iio_dma_buffer_data_available(struct iio_buffer *buffer); int iio_dma_buffer_set_bytes_per_datum(struct iio_buffer *buffer, size_t bpd); int iio_dma_buffer_set_length(struct iio_buffer *buffer, unsigned int length); @@ -142,4 +144,9 @@ 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); +static inline size_t iio_dma_buffer_space_available(struct iio_buffer *buffer) +{

• return iio_dma_buffer_data_available(buffer);

As mentioned above, I don't see having this trivial wrapper as beneficial.

+}

#endif

On Mon, 3 Apr 2023 17:47:54 +0200 Paul Cercueil paul@crapouillou.net wrote:

Update the devm_iio_dmaengine_buffer_setup() function to support specifying the buffer direction.

Update the iio_dmaengine_buffer_submit() function to handle input buffers as well as output buffers.

Signed-off-by: Paul Cercueil paul@crapouillou.net Reviewed-by: Alexandru Ardelean ardeleanalex@gmail.com

Just one trivial question inline.

Jonathan

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c index 5f85ba38e6f6..592d2aa9044c 100644 --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c @@ -64,14 +64,25 @@ 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;

• enum dma_transfer_direction dma_dir;
• size_t max_size; dma_cookie_t cookie;

• block->bytes_used = min(block->size, dmaengine_buffer->max_size);
• block->bytes_used = round_down(block->bytes_used,

• 	dmaengine_buffer->align);
  

• max_size = min(block->size, dmaengine_buffer->max_size);
• max_size = round_down(max_size, dmaengine_buffer->align);
• if (queue->buffer.direction == IIO_BUFFER_DIRECTION_IN) {

• block->bytes_used = max_size;
  

• dma_dir = DMA_DEV_TO_MEM;
  

• } else {

• dma_dir = DMA_MEM_TO_DEV;
  

• }
• if (!block->bytes_used || block->bytes_used > max_size)

• return -EINVAL;
  

Two paths to here. Either DIRECTION_IN in which we just set things up so conditions being checked are always fine (unless max_size == 0? Can that happen?), or !DIRECTION_IN. So why not move this into the else {} branch above?

desc = dmaengine_prep_slave_single(dmaengine_buffer->chan,

• block->phys_addr, block->bytes_used, DMA_DEV_TO_MEM,
  

• block->phys_addr, block->bytes_used, dma_dir,
  

  DMA_PREP_INTERRUPT); if (!desc) return -ENOMEM;

On Mon, 3 Apr 2023 17:47:55 +0200 Paul Cercueil paul@crapouillou.net wrote:

Use the iio_dma_buffer_write() and iio_dma_buffer_space_available() functions provided by the buffer-dma core, to enable write support in the buffer-dmaengine code.

Signed-off-by: Paul Cercueil paul@crapouillou.net Reviewed-by: Alexandru Ardelean ardeleanalex@gmail.com

---

drivers/iio/buffer/industrialio-buffer-dmaengine.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c index 592d2aa9044c..866c8b84bb24 100644 --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c @@ -123,12 +123,14 @@ static void iio_dmaengine_buffer_release(struct iio_buffer *buf) static const struct iio_buffer_access_funcs iio_dmaengine_buffer_ops = { .read = iio_dma_buffer_read,

• .write = iio_dma_buffer_write, .set_bytes_per_datum = iio_dma_buffer_set_bytes_per_datum, .set_length = iio_dma_buffer_set_length, .request_update = iio_dma_buffer_request_update, .enable = iio_dma_buffer_enable, .disable = iio_dma_buffer_disable, .data_available = iio_dma_buffer_data_available,
• .space_available = iio_dma_buffer_space_available,

Follow through from earlier patch. I would prefer it to be obvious hers that the two callbacks above are identical. Easiest way to expose that detail is to set the callbacks to the same function, but then it perhaps needs a rename so it isn't specific to one of the other.

Jonathan

.release = iio_dmaengine_buffer_release, .modes = INDIO_BUFFER_HARDWARE,

On Mon, 2023-04-03 at 17:47 +0200, Paul Cercueil wrote:

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

---

v2: Only allow the new IOCTLs on the buffer FD created with     IIO_BUFFER_GET_FD_IOCTL().

v3: - Get rid of the old IOCTLs. The IIO subsystem does not create or     manage DMABUFs anymore, and only attaches/detaches externally     created DMABUFs.     - Add IIO_BUFFER_DMABUF_CYCLIC to the supported flags.

---

drivers/iio/industrialio-buffer.c | 402 ++++++++++++++++++++++++++++++  include/linux/iio/buffer_impl.h   |  22 ++  include/uapi/linux/iio/buffer.h   |  22 ++  3 files changed, 446 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 80c78bd6bbef..5d88e098b3e7 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -13,10 +13,14 @@  #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,11 +32,41 @@  #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 iio_dma_fence *fence; +};

+struct iio_dma_fence { +       struct dma_fence base; +       struct iio_dmabuf_priv *priv; +       struct sg_table *sgt; +       enum dma_data_direction dir; +};

static const char * const iio_endian_prefix[] = {         [IIO_BE] = "be",         [IIO_LE] = "le",  };   +static inline struct iio_dma_fence *to_iio_dma_fence(struct dma_fence *fence) +{ +       return container_of(fence, struct iio_dma_fence, base); +}

Kind of a nitpick but I only see this being used once so I would maybe use plain 'container_of()' as you are already doing for:

... = container_of(ref, struct iio_dmabuf_priv, ref);

So I would at least advocate for consistency. I would also probably ditch the inline but I guess that is more a matter of style/preference.

static bool iio_buffer_is_active(struct iio_buffer *buf)  {         return !list_empty(&buf->buffer_list); @@ -329,6 +363,7 @@ void iio_buffer_init(struct iio_buffer *buffer)  {

...

+       priv = attach->importer_priv; +       list_del_init(&priv->entry);

+       iio_buffer_dmabuf_put(attach); +       iio_buffer_dmabuf_put(attach);

Is this intended? Looks suspicious...

+out_dmabuf_put: +       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 = to_iio_dma_fence(fence);

+       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; +       enum dma_data_direction dir; +       struct iio_dma_fence *fence; +       struct dma_buf *dmabuf; +       struct sg_table *sgt; +       unsigned long timeout; +       bool dma_to_ram; +       bool cyclic; +       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->indio_dev, dmabuf); +       if (IS_ERR(attach)) { +               ret = PTR_ERR(attach); +               goto err_dmabuf_put; +       }

+       priv = attach->importer_priv;

+       dma_to_ram = buffer->direction == IIO_BUFFER_DIRECTION_IN; +       dir = dma_to_ram ? DMA_FROM_DEVICE : DMA_TO_DEVICE;

+       sgt = dma_buf_map_attachment(attach, dir); +       if (IS_ERR(sgt)) { +               ret = PTR_ERR(sgt); +               pr_err("Unable to map attachment: %d\n", ret);

dev_err()? We should be able to reach the iio_dev

+               goto err_attachment_put; +       }

+       fence = kmalloc(sizeof(*fence), GFP_KERNEL); +       if (!fence) { +               ret = -ENOMEM; +               goto err_unmap_attachment; +       }

...

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,  };

Hmmm, what about the legacy buffer? We should also support this interface using it, right? Otherwise, using one of the new IOCTL in iio_device_buffer_ioctl() (or /dev/iio:device0) will error out.

- Nuno Sá

On Tue, 2023-04-04 at 09:55 +0200, Paul Cercueil wrote:

Hi Nuno,

Le mardi 04 avril 2023 à 09:32 +0200, Nuno Sá a écrit :

...

On Mon, 2023-04-03 at 17:47 +0200, Paul Cercueil wrote:

...

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

---

v2: Only allow the new IOCTLs on the buffer FD created with     IIO_BUFFER_GET_FD_IOCTL().

v3: - Get rid of the old IOCTLs. The IIO subsystem does not create or     manage DMABUFs anymore, and only attaches/detaches externally     created DMABUFs.     - Add IIO_BUFFER_DMABUF_CYCLIC to the supported flags.

---

drivers/iio/industrialio-buffer.c | 402 ++++++++++++++++++++++++++++++  include/linux/iio/buffer_impl.h   |  22 ++  include/uapi/linux/iio/buffer.h   |  22 ++  3 files changed, 446 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 80c78bd6bbef..5d88e098b3e7 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -13,10 +13,14 @@  #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,11 +32,41 @@  #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 iio_dma_fence *fence; +};

+struct iio_dma_fence { +       struct dma_fence base; +       struct iio_dmabuf_priv *priv; +       struct sg_table *sgt; +       enum dma_data_direction dir; +};

static const char * const iio_endian_prefix[] = {         [IIO_BE] = "be",         [IIO_LE] = "le",  };   +static inline struct iio_dma_fence *to_iio_dma_fence(struct dma_fence *fence) +{ +       return container_of(fence, struct iio_dma_fence, base); +}

Kind of a nitpick but I only see this being used once so I would maybe use plain 'container_of()' as you are already doing for:

... = container_of(ref, struct iio_dmabuf_priv, ref);

So I would at least advocate for consistency. I would also probably ditch the inline but I guess that is more a matter of style/preference.

Yep, at least it should be consistent.

...

...

static bool iio_buffer_is_active(struct iio_buffer *buf)  {         return !list_empty(&buf->buffer_list); @@ -329,6 +363,7 @@ void iio_buffer_init(struct iio_buffer *buffer)  {

...

...

+       priv = attach->importer_priv; +       list_del_init(&priv->entry);

+       iio_buffer_dmabuf_put(attach); +       iio_buffer_dmabuf_put(attach);

Is this intended? Looks suspicious...

It is intended, yes. You want to release the dma_buf_attachment that's created in iio_buffer_attach_dmabuf(), and you need to call iio_buffer_find_attachment() to get a pointer to it, which also gets a second reference - so it needs to unref twice.

I see..

...

...

...

+out_dmabuf_put: +       dma_buf_put(dmabuf);

+       return ret; +}

Hmmm, what about the legacy buffer? We should also support this interface using it, right? Otherwise, using one of the new IOCTL in iio_device_buffer_ioctl() (or /dev/iio:device0) will error out.

According to Jonathan the old chardev route is deprecated, and it's fine not to support the IOCTL there.

Oh, alright then... Better that way indeed!

- Nuno Sá

On Mon, 3 Apr 2023 17:47:56 +0200 Paul Cercueil paul@crapouillou.net wrote:

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.

I like numbers to support a patch. Any nice ones to throw in here as examples of expected rates?

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

Trivial comments from me. I don't (yet) understand dmabuf well enough to know if that part is right or not. Not sure I will ever find the time so relying on those who are more familiar with it to tell me if that code is correct.

Thanks,

Jonathan

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);

• /* 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);
  

• }
• /* TODO: Is it safe? Can "ib" be freed here? */

No idea :) However that need resolving before we apply this.

• if (!list_empty(&buffer->dmabufs))

• dev_warn(&indio_dev->dev, "Buffer FD closed with active transfers\n");
  

• kfree(ib); clear_bit(IIO_BUSY_BIT_POS, &buffer->flags); iio_device_put(indio_dev);

@@ -1515,11 +1591,337 @@ static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep) return 0; }

+static int iio_buffer_enqueue_dmabuf(struct iio_dev_buffer_pair *ib,

• 		     struct iio_dmabuf __user *iio_dmabuf_req,
  

• 		     bool nonblock)
  

+{

...

• ret = buffer->access->enqueue_dmabuf(buffer, priv->block, sgt,

• 			     iio_dmabuf.bytes_used, cyclic);
  

• if (ret)

Hmm. Is there an easy way to perhaps avoid a function with multiple error handling paths like we have here. Perhaps drag the extra stuff from the the dmabuf_done() function into this if (ret) then goto err_fence_put;? I'm not sure if that would make this even harder to read however.

• iio_buffer_signal_dmabuf_done(attach, ret);
  

• dma_buf_put(dmabuf);
• return ret;

+err_resv_unlock:

• dma_resv_unlock(dmabuf->resv);

+err_fence_put:

• dma_fence_put(&fence->base);

+err_unmap_attachment:

• dma_buf_unmap_attachment(attach, sgt, dir);

+err_attachment_put:

• iio_buffer_dmabuf_put(attach);

+err_dmabuf_put:

• dma_buf_put(dmabuf);
• return ret;

+}

+void iio_buffer_signal_dmabuf_done(struct dma_buf_attachment *attach, int ret) +{

• struct iio_dmabuf_priv *priv = attach->importer_priv;
• struct iio_dma_fence *fence = priv->fence;
• enum dma_data_direction dir = fence->dir;
• struct sg_table *sgt = fence->sgt;
• dma_fence_get(&fence->base);
• fence->base.error = ret;
• dma_fence_signal(&fence->base);
• dma_fence_put(&fence->base);
• dma_buf_unmap_attachment(attach, sgt, dir);
• iio_buffer_dmabuf_put(attach);

+} +EXPORT_SYMBOL_GPL(iio_buffer_signal_dmabuf_done);

...

diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h index 89c3fd7c29ca..a8a490091277 100644 --- a/include/linux/iio/buffer_impl.h +++ b/include/linux/iio/buffer_impl.h @@ -9,8 +9,11 @@ #include <uapi/linux/iio/buffer.h> #include <linux/iio/buffer.h> +struct dma_buf_attachment; 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 +42,9 @@ 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
  

• • @alloc_dmabuf: called from userspace via ioctl to allocate one DMABUF.

Looks like you missed updating the docs.

• • @enqueue_dmabuf: called from userspace via ioctl to queue this DMABUF

• • 	object to this buffer. Requires a valid DMABUF fd.
    

  • @modes: Supported operating modes by this buffer type
  • @flags: A bitmask combination of INDIO_BUFFER_FLAG_*

@@ -68,6 +74,14 @@ 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 sg_table *sgt, size_t size, bool cyclic);
  

• unsigned int modes; unsigned int flags;

};

Paul Cercueil paul@crapouillou.net writes:

One nit:

Document the new DMABUF based API.

Signed-off-by: Paul Cercueil paul@crapouillou.net Cc: Jonathan Corbet corbet@lwn.net Cc: linux-doc@vger.kernel.org

---

v2: - Explicitly state that the new interface is optional and is not implemented by all drivers. - The IOCTLs can now only be called on the buffer FD returned by IIO_BUFFER_GET_FD_IOCTL. - Move the page up a bit in the index since it is core stuff and not driver-specific. v3: Update the documentation to reflect the new API.

---

Documentation/iio/dmabuf_api.rst | 59 ++++++++++++++++++++++++++++++++ Documentation/iio/index.rst | 2 ++ 2 files changed, 61 insertions(+) create mode 100644 Documentation/iio/dmabuf_api.rst

diff --git a/Documentation/iio/dmabuf_api.rst b/Documentation/iio/dmabuf_api.rst new file mode 100644 index 000000000000..4d70372c7ebd --- /dev/null +++ b/Documentation/iio/dmabuf_api.rst @@ -0,0 +1,59 @@ +.. 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.

Rather than abusing subsections, this would be better done as a description list:

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.

Thanks,

jon

On Mon, 3 Apr 2023 17:49:54 +0200 Paul Cercueil paul@crapouillou.net wrote:

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

A few things inline.

Thanks,

Jonathan

---

v3: Use the new dmaengine_prep_slave_dma_array(), and adapt the code to work with the new functions introduced in industrialio-buffer-dma.c.

---

.../buffer/industrialio-buffer-dmaengine.c | 69 ++++++++++++++++--- include/linux/iio/buffer-dma.h | 2 + 2 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c index 866c8b84bb24..faed9c2b089c 100644 --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c @@ -65,25 +65,68 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue, iio_buffer_to_dmaengine_buffer(&queue->buffer); struct dma_async_tx_descriptor *desc; enum dma_transfer_direction dma_dir;

• unsigned int i, nents, *lenghts;
• struct scatterlist *sgl;
• unsigned long flags;
• dma_addr_t *addrs; size_t max_size; dma_cookie_t cookie;
• size_t len_total;

• max_size = min(block->size, dmaengine_buffer->max_size);
• max_size = round_down(max_size, dmaengine_buffer->align);

• if (!block->bytes_used)

• return -EINVAL;
  

• if (queue->buffer.direction == IIO_BUFFER_DIRECTION_IN) {

• block->bytes_used = max_size;
  

• if (queue->buffer.direction == IIO_BUFFER_DIRECTION_IN) dma_dir = DMA_DEV_TO_MEM;

• } else {

• else dma_dir = DMA_MEM_TO_DEV;

• }

• if (!block->bytes_used || block->bytes_used > max_size)

• return -EINVAL;
  

Ah this is dropping the code I moaned about earlier. I'll probably forget though so maybe add a note to that patch saying it goes away later anyway so I don't keep moaning about it in future versions.

• if (block->sg_table) {

• sgl = block->sg_table->sgl;
  

• nents = sg_nents_for_len(sgl, block->bytes_used);
  

• addrs = kmalloc_array(nents, sizeof(*addrs), GFP_KERNEL);
  

• if (!addrs)
  

• 	return -ENOMEM;
  

• lenghts = kmalloc_array(nents, sizeof(*lenghts), GFP_KERNEL);
  

lengths?

• if (!lenghts) {
  

• 	kfree(addrs);
  

• 	return -ENOMEM;
  

• }
  

• len_total = block->bytes_used;
  

• desc = dmaengine_prep_slave_single(dmaengine_buffer->chan,

• block->phys_addr, block->bytes_used, dma_dir,
  

• DMA_PREP_INTERRUPT);
  

• for (i = 0; i < nents; i++) {
  

• 	addrs[i] = sg_dma_address(sgl);
  

• 	lenghts[i] = min(sg_dma_len(sgl), len_total);
  

• 	len_total -= lenghts[i];
  

• 	sgl = sg_next(sgl);
  

• }
  

• flags = block->cyclic ? DMA_PREP_REPEAT : DMA_PREP_INTERRUPT;
  

• desc = dmaengine_prep_slave_dma_array(dmaengine_buffer->chan,
  

• 				      addrs, lenghts, nents,
  

• 				      dma_dir, flags);
  

• kfree(addrs);
  

• kfree(lenghts);
  

• } else {

• max_size = min(block->size, dmaengine_buffer->max_size);
  

• max_size = round_down(max_size, dmaengine_buffer->align);
  

• if (queue->buffer.direction == IIO_BUFFER_DIRECTION_IN)
  

• 	block->bytes_used = max_size;
  

• if (block->bytes_used > max_size)
  

• 	return -EINVAL;
  

• desc = dmaengine_prep_slave_single(dmaengine_buffer->chan,
  

• 				   block->phys_addr,
  

• 				   block->bytes_used, dma_dir,
  

• 				   DMA_PREP_INTERRUPT);
  

• }

diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h index e5e5817e99db..48f7ffaf0867 100644 --- a/include/linux/iio/buffer-dma.h +++ b/include/linux/iio/buffer-dma.h @@ -43,6 +43,7 @@ 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

I might have commented on it earlier (I've lost track) but attach should be documented as well. Worth sanity checking by either building with W=1 or running kernel-doc over the files and fixing the warnings.

*/ struct iio_dma_buffer_block { @@ -67,6 +68,7 @@ struct iio_dma_buffer_block { */ enum iio_block_state state;

• bool cyclic; bool fileio;

struct dma_buf_attachment *attach;