On Mon, Nov 15, 2021 at 4:19 PM Paul Cercueil paul@crapouillou.net wrote:

The buffer-dma code was using two queues, incoming and outgoing, to manage the state of the blocks in use.

While this totally works, it adds some complexity to the code, especially since the code only manages 2 blocks. It is much easier to just check each block's state manually, and keep a counter for the next block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and outgoing queues anyway, getting rid of them now makes the upcoming changes simpler.

Reviewed-by: Alexandru Ardelean ardeleanalex@gmail.com

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

drivers/iio/buffer/industrialio-buffer-dma.c | 68 ++++++++++---------- include/linux/iio/buffer-dma.h | 7 +- 2 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index d348af8b9705..abac88f20104 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -191,16 +191,8 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(

static void _iio_dma_buffer_block_done(struct iio_dma_buffer_block *block) {

•   struct iio_dma_buffer_queue *queue = block->queue;
  

•   /*
  

•    * The buffer has already been freed by the application, just drop the
  

•    * reference.
  

•    */
  

•   if (block->state != IIO_BLOCK_STATE_DEAD) {
  

•   if (block->state != IIO_BLOCK_STATE_DEAD)
            block->state = IIO_BLOCK_STATE_DONE;
  

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

•   }
  

}

/** @@ -317,11 +309,8 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer) * dead. This means we can reset the lists without having to fear * corrution. */

•   INIT_LIST_HEAD(&queue->outgoing);
    spin_unlock_irq(&queue->list_lock);
  
  

•   INIT_LIST_HEAD(&queue->incoming);
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
            if (queue->fileio.blocks[i]) {
                    block = queue->fileio.blocks[i];
  

@@ -346,7 +335,6 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer) }

            block->state = IIO_BLOCK_STATE_QUEUED;

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

out_unlock: @@ -401,13 +389,18 @@ int iio_dma_buffer_enable(struct iio_buffer *buffer, struct iio_dev *indio_dev) { struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);

•   struct iio_dma_buffer_block *block, *_block;
  

•   struct iio_dma_buffer_block *block;
  

•   unsigned int i;
  
    mutex_lock(&queue->lock);
    queue->active = true;
  

•   list_for_each_entry_safe(block, _block, &queue->incoming, head) {
  

•           list_del(&block->head);
  

•           iio_dma_buffer_submit_block(queue, block);
  

•   queue->fileio.next_dequeue = 0;
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
  

•           block = queue->fileio.blocks[i];
  

•           if (block->state == IIO_BLOCK_STATE_QUEUED)
  

•                   iio_dma_buffer_submit_block(queue, block);
    }
    mutex_unlock(&queue->lock);
  
  

@@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue *queue, struct iio_dma_buffer_block *block) {

•   if (block->state == IIO_BLOCK_STATE_DEAD) {
  

•   if (block->state == IIO_BLOCK_STATE_DEAD)
            iio_buffer_block_put(block);
  

•   } else if (queue->active) {
  

•   else if (queue->active)
            iio_dma_buffer_submit_block(queue, block);
  

•   } else {
  

•   else
            block->state = IIO_BLOCK_STATE_QUEUED;
  

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

•   }
  

}

static struct iio_dma_buffer_block *iio_dma_buffer_dequeue( struct iio_dma_buffer_queue *queue) { struct iio_dma_buffer_block *block;

•   unsigned int idx;
  
    spin_lock_irq(&queue->list_lock);
  

•   block = list_first_entry_or_null(&queue->outgoing, struct
  

•           iio_dma_buffer_block, head);
  

•   if (block != NULL) {
  

•           list_del(&block->head);
  

•   idx = queue->fileio.next_dequeue;
  

•   block = queue->fileio.blocks[idx];
  

•   if (block->state == IIO_BLOCK_STATE_DONE) {
            block->state = IIO_BLOCK_STATE_DEQUEUED;
  

•           idx = (idx + 1) % ARRAY_SIZE(queue->fileio.blocks);
  

•           queue->fileio.next_dequeue = idx;
  

•   } else {
  

•           block = NULL;
    }
  

•   spin_unlock_irq(&queue->list_lock);
  
    return block;
  

@@ -539,6 +537,7 @@ size_t iio_dma_buffer_data_available(struct iio_buffer *buf) struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buf); struct iio_dma_buffer_block *block; size_t data_available = 0;

•   unsigned int i;
  
    /*
     * For counting the available bytes we'll use the size of the block not
  

@@ -552,8 +551,15 @@ size_t iio_dma_buffer_data_available(struct iio_buffer *buf) data_available += queue->fileio.active_block->size;

    spin_lock_irq(&queue->list_lock);

•   list_for_each_entry(block, &queue->outgoing, head)
  

•           data_available += block->size;
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
  

•           block = queue->fileio.blocks[i];
  

•           if (block != queue->fileio.active_block
  

•               && block->state == IIO_BLOCK_STATE_DONE)
  

•                   data_available += block->size;
  

•   }
  

•   spin_unlock_irq(&queue->list_lock);
    mutex_unlock(&queue->lock);
  
  

@@ -616,9 +622,6 @@ int iio_dma_buffer_init(struct iio_dma_buffer_queue *queue, queue->dev = dev; queue->ops = ops;

•   INIT_LIST_HEAD(&queue->incoming);
  

•   INIT_LIST_HEAD(&queue->outgoing);
  

•   mutex_init(&queue->lock);
    spin_lock_init(&queue->list_lock);
  
  

@@ -645,11 +648,8 @@ void iio_dma_buffer_exit(struct iio_dma_buffer_queue *queue) continue; queue->fileio.blocks[i]->state = IIO_BLOCK_STATE_DEAD; }

•   INIT_LIST_HEAD(&queue->outgoing);
    spin_unlock_irq(&queue->list_lock);
  
  

•   INIT_LIST_HEAD(&queue->incoming);
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
            if (!queue->fileio.blocks[i])
                    continue;
  

diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h index ff15c61bf319..d4ed5ff39d44 100644 --- a/include/linux/iio/buffer-dma.h +++ b/include/linux/iio/buffer-dma.h @@ -78,12 +78,15 @@ struct iio_dma_buffer_block {

• @active_block: Block being used in read()
• @pos: Read offset in the active block
• @block_size: Size of each block

• • @next_dequeue: index of next block that will be dequeued
  */

struct iio_dma_buffer_queue_fileio { struct iio_dma_buffer_block *blocks[2]; struct iio_dma_buffer_block *active_block; size_t pos; size_t block_size;

•   unsigned int next_dequeue;
  

};

/** @@ -97,8 +100,6 @@ struct iio_dma_buffer_queue_fileio {

• atomic context as well as blocks on those lists. This is the outgoing queue
• list and typically also a list of active blocks in the part that handles
• the DMA controller

• • @incoming: List of buffers on the incoming queue
• • @outgoing: List of buffers on the outgoing queue
  • @active: Whether the buffer is currently active
  • @fileio: FileIO state
  */

@@ -109,8 +110,6 @@ struct iio_dma_buffer_queue {

    struct mutex lock;
    spinlock_t list_lock;

•   struct list_head incoming;
  

•   struct list_head outgoing;
  
    bool active;
  
  

-- 2.33.0

On 11/15/21 3:19 PM, Paul Cercueil wrote:

The buffer-dma code was using two queues, incoming and outgoing, to manage the state of the blocks in use.

While this totally works, it adds some complexity to the code, especially since the code only manages 2 blocks. It is much easier to just check each block's state manually, and keep a counter for the next block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and outgoing queues anyway, getting rid of them now makes the upcoming changes simpler.

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

The outgoing queue is going to be replaced by fences, but I think we need to keep the incoming queue.

[...] @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue *queue, struct iio_dma_buffer_block *block) {

• if (block->state == IIO_BLOCK_STATE_DEAD) {

• if (block->state == IIO_BLOCK_STATE_DEAD) iio_buffer_block_put(block);

• } else if (queue->active) {

• else if (queue->active) iio_dma_buffer_submit_block(queue, block);

• } else {

• else block->state = IIO_BLOCK_STATE_QUEUED;

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

If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is not active, it will be marked as queued, but we don't actually keep a reference to it anywhere. It will never be submitted to the DMA, and it will never be signaled as completed.

On 11/21/21 6:52 PM, Paul Cercueil wrote:

Hi Lars,

Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen lars@metafoo.de a écrit :

...

On 11/15/21 3:19 PM, Paul Cercueil wrote:

...

The buffer-dma code was using two queues, incoming and outgoing, to manage the state of the blocks in use.

While this totally works, it adds some complexity to the code, especially since the code only manages 2 blocks. It is much easier to just check each block's state manually, and keep a counter for the next block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and outgoing queues anyway, getting rid of them now makes the upcoming changes simpler.

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

The outgoing queue is going to be replaced by fences, but I think we need to keep the incoming queue.

Blocks are always accessed in sequential order, so we now have a "queue->next_dequeue" that cycles between the buffers allocated for fileio.

...

...

[...] @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable);   static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue *queue,       struct iio_dma_buffer_block *block)   { -    if (block->state == IIO_BLOCK_STATE_DEAD) { +    if (block->state == IIO_BLOCK_STATE_DEAD)           iio_buffer_block_put(block); -    } else if (queue->active) { +    else if (queue->active)           iio_dma_buffer_submit_block(queue, block); -    } else { +    else           block->state = IIO_BLOCK_STATE_QUEUED; -        list_add_tail(&block->head, &queue->incoming);

If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is not active, it will be marked as queued, but we don't actually keep a reference to it anywhere. It will never be submitted to the DMA, and it will never be signaled as completed.

We do keep a reference to the buffers, in the queue->fileio.blocks array. When the buffer is enabled, all the blocks in that array that are in the "queued" state will be submitted to the DMA.

But not when used in combination with the DMA buf changes later in this series.

On 11/21/21 9:08 PM, Paul Cercueil wrote:

Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen lars@metafoo.de a écrit :

...

On 11/21/21 6:52 PM, Paul Cercueil wrote:

...

Hi Lars,

Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen lars@metafoo.de a écrit :

...

On 11/15/21 3:19 PM, Paul Cercueil wrote:

...

The buffer-dma code was using two queues, incoming and outgoing, to manage the state of the blocks in use.

While this totally works, it adds some complexity to the code, especially since the code only manages 2 blocks. It is much easier to just check each block's state manually, and keep a counter for the next block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and outgoing queues anyway, getting rid of them now makes the upcoming changes simpler.

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

The outgoing queue is going to be replaced by fences, but I think we need to keep the incoming queue.

Blocks are always accessed in sequential order, so we now have a "queue->next_dequeue" that cycles between the buffers allocated for fileio.

...

...

[...] @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable);   static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue *queue,       struct iio_dma_buffer_block *block)   { -    if (block->state == IIO_BLOCK_STATE_DEAD) { +    if (block->state == IIO_BLOCK_STATE_DEAD)           iio_buffer_block_put(block); -    } else if (queue->active) { +    else if (queue->active)           iio_dma_buffer_submit_block(queue, block); -    } else { +    else           block->state = IIO_BLOCK_STATE_QUEUED; -        list_add_tail(&block->head, &queue->incoming);

If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is not active, it will be marked as queued, but we don't actually keep a reference to it anywhere. It will never be submitted to the DMA, and it will never be signaled as completed.

We do keep a reference to the buffers, in the queue->fileio.blocks array. When the buffer is enabled, all the blocks in that array that are in the "queued" state will be submitted to the DMA.

But not when used in combination with the DMA buf changes later in this series.

That's still the case after the DMABUF changes of the series. Or can you point me exactly what you think is broken?

When you allocate a DMABUF with the allocate IOCTL and then submit it with the enqueue IOCTL before the buffer is enabled it will end up marked as queued, but not actually be queued anywhere.

On 11/22/21 4:16 PM, Paul Cercueil wrote:

Hi Lars,

Le lun., nov. 22 2021 at 16:08:51 +0100, Lars-Peter Clausen lars@metafoo.de a écrit :

...

On 11/21/21 9:08 PM, Paul Cercueil wrote:

...

Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen lars@metafoo.de a écrit :

...

On 11/21/21 6:52 PM, Paul Cercueil wrote:

...

Hi Lars,

Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen lars@metafoo.de a écrit :

...

On 11/15/21 3:19 PM, Paul Cercueil wrote: > The buffer-dma code was using two queues, incoming and outgoing, to > manage the state of the blocks in use. > > While this totally works, it adds some complexity to the code, > especially since the code only manages 2 blocks. It is much > easier to > just check each block's state manually, and keep a counter for > the next > block to dequeue. > > Since the new DMABUF based API wouldn't use these incoming and > outgoing > queues anyway, getting rid of them now makes the upcoming changes > simpler. > > Signed-off-by: Paul Cercueil paul@crapouillou.net The outgoing queue is going to be replaced by fences, but I think we need to keep the incoming queue.

Blocks are always accessed in sequential order, so we now have a "queue->next_dequeue" that cycles between the buffers allocated for fileio.

...

> [...] > @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); >   static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue > *queue, >       struct iio_dma_buffer_block *block) >   { > -    if (block->state == IIO_BLOCK_STATE_DEAD) { > +    if (block->state == IIO_BLOCK_STATE_DEAD) >           iio_buffer_block_put(block); > -    } else if (queue->active) { > +    else if (queue->active) >           iio_dma_buffer_submit_block(queue, block); > -    } else { > +    else >           block->state = IIO_BLOCK_STATE_QUEUED; > -        list_add_tail(&block->head, &queue->incoming); If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is not active, it will be marked as queued, but we don't actually keep a reference to it anywhere. It will never be submitted to the DMA, and it will never be signaled as completed.

We do keep a reference to the buffers, in the queue->fileio.blocks array. When the buffer is enabled, all the blocks in that array that are in the "queued" state will be submitted to the DMA.

But not when used in combination with the DMA buf changes later in this series.

That's still the case after the DMABUF changes of the series. Or can you point me exactly what you think is broken?

When you allocate a DMABUF with the allocate IOCTL and then submit it with the enqueue IOCTL before the buffer is enabled it will end up marked as queued, but not actually be queued anywhere.

Ok, it works for me because I never enqueue blocks before enabling the buffer. I can add a requirement that blocks must be enqueued only after the buffer is enabled.

I don't think that is a good idea. This way you are going to potentially drop data at the begining of your stream when the DMA isn't ready yet.

On Mon, 15 Nov 2021 14:19:13 +0000 Paul Cercueil paul@crapouillou.net wrote:

We know that the buffer's alignment will always be a power of two; therefore, we can use the faster round_down() macro.

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

*groan*. I don't want to know where the naming of these two came from but that is spectacular...

Anyhow, happy to pick up 1-3 now if you like as all are good cleanup of existing code.

Jonathan

---

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

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c index 1ac94c4e9792..f8ce26a24c57 100644 --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c @@ -67,7 +67,7 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue, dma_cookie_t cookie;

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

• block->bytes_used = rounddown(block->bytes_used,

• block->bytes_used = round_down(block->bytes_used, dmaengine_buffer->align);

  desc = dmaengine_prep_slave_single(dmaengine_buffer->chan,

On Mon, Nov 15, 2021 at 4:19 PM 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).

A few comments inline from me. Otherwise fine from me.

Reviewed-by: Alexandru Ardelean ardeleanalex@gmail.com

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

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

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index abac88f20104..eeeed6b2e0cf 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -179,7 +179,8 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( }

    block->size = size;

•   block->state = IIO_BLOCK_STATE_DEQUEUED;
  

•   block->bytes_used = size;
  

•   block->state = IIO_BLOCK_STATE_DONE;
  

This initial state change looks like it could use a comment in the git log. Maybe it's fine, but I can't remember 100% about it.

    block->queue = queue;
    INIT_LIST_HEAD(&block->head);
    kref_init(&block->kref);

@@ -195,6 +196,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 +225,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 +254,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);

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

•           block->state = IIO_BLOCK_STATE_QUEUED;
  

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

•                   block->state = IIO_BLOCK_STATE_QUEUED;
    }
  
  

out_unlock: @@ -467,20 +481,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_write)
  

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

@@ -503,8 +509,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_write)
  

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

•   else
  

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

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

@@ -513,6 +524,7 @@ int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n,

    if (queue->fileio.pos == block->bytes_used) {
            queue->fileio.active_block = NULL;

•           block->bytes_used = block->size;
            iio_dma_buffer_enqueue(queue, block);
    }
  
  

@@ -523,8 +535,39 @@ 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 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 a65a005c4a19..09c07d5563c0 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);
  

+}

Maybe re-using iio_dma_buffer_data_available() (and updating the return type) would have been a little neater than wrapping this. Also fine like this though.

#endif

2.33.0

Hi Jonathan,

Le dim., nov. 21 2021 at 14:20:49 +0000, Jonathan Cameron jic23@kernel.org a écrit :

On Mon, 15 Nov 2021 14:19:14 +0000 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).

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

Hi Paul,

There are a few changes in here, such as the bytes_used value being set that I'm not following the reasoning behind. More info on those? Also good to provide something about those in this patch description.

Thanks,

Jonathan

...

---

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

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index abac88f20104..eeeed6b2e0cf 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -179,7 +179,8 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( }

block->size = size;

• block->state = IIO_BLOCK_STATE_DEQUEUED;

• block->bytes_used = size;
• block->state = IIO_BLOCK_STATE_DONE;

I don't know why these are here - some more info?

When using an input buffer the block->bytes_used is unconditionally reset in iio_dmaengine_buffer_submit_block(), so this was fine until now.

When using an output buffer the block->bytes_used can actually (with the new API) be specified by the user, so we don't want iio_dmaengine_buffer_submit_block() to unconditionally override it. Which means that in the case where we have an output buffer in fileio mode, we do need block->bytes_used to be initialized to the buffer's size since it won't be set anywhere else.

About the change in block->state: in patch [01/15] we removed the incoming/outgoing queues, and while the "enqueued" state is still useful to know which buffers have to be submitted when the buffer is enabled, the "dequeued" state is not useful anymore since there is no more distinction vs. the "done" state.

I believe this change should be moved to patch [01/15] then, and I should go further and remove the IIO_BLOCK_STATE_DEQUEUED completely.

...

block->queue = queue; INIT_LIST_HEAD(&block->head); kref_init(&block->kref); @@ -195,6 +196,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 +225,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 +254,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);

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

• block->state = IIO_BLOCK_STATE_QUEUED;
  

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

• 	block->state = IIO_BLOCK_STATE_QUEUED;
  

Possibly worth a comment on the state being set here. I figured it out, but might save some brain cells in future if it's stated in the code.

Ok.

...

}

out_unlock: @@ -467,20 +481,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_write)
  

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

@@ -503,8 +509,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_write)

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

• else

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

What is the !! gaining us here? We only care about == 0 vs != 0 so forcing it to be 0 or 1 isn't useful.

Right.

...

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

@@ -513,6 +524,7 @@ int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n,

if (queue->fileio.pos == block->bytes_used) { queue->fileio.active_block = NULL;

• block->bytes_used = block->size;
  

This seems to be a functional change that isn't called out in the patch description.

See the explanation above. Although I most likely don't need to set it at two different spots... I'll check that in detail next week.

Cheers, -Paul

...

iio_dma_buffer_enqueue(queue, block);

}

@@ -523,8 +535,39 @@ 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 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 a65a005c4a19..09c07d5563c0 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);

+}

#endif

On Tue, 16 Nov 2021 12:59:30 +0200 Alexandru Ardelean ardeleanalex@gmail.com wrote:

On Mon, Nov 15, 2021 at 4:20 PM Paul Cercueil paul@crapouillou.net wrote:

...

From: Alexandru Ardelean alexandru.ardelean@analog.com

A part of the logic in the iio_dma_buffer_exit() is required for the change to add mmap support to IIO buffers. This change splits the logic into a separate function, which will be re-used later.

Not sure how the protocol is here, since my old @analog.com email isn't working anymore.

But:

Signed-off-by: Alexandru Ardelean ardeleanalex@gmail.com

If you want to both document your email address change and make various scripts find you more easily, send a patch for .mailmap

In meantime we can probably leave your original sign off in place but add a Cc: with an up to date email address. That will reflect that the sign off for this was (I assume) given back when you were at Analog.

Thanks :) Alex

...

Signed-off-by: Alexandru Ardelean alexandru.ardelean@analog.com Signed-off-by: Paul Cercueil paul@crapouillou.net

---

drivers/iio/buffer/industrialio-buffer-dma.c | 39 +++++++++++--------- 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index eeeed6b2e0cf..eb8cfd3af030 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -358,6 +358,27 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer) } EXPORT_SYMBOL_GPL(iio_dma_buffer_request_update);

+static void iio_dma_buffer_fileio_free(struct iio_dma_buffer_queue *queue) +{

•   unsigned int i;
  

•   spin_lock_irq(&queue->list_lock);
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
  

•           if (!queue->fileio.blocks[i])
  

•                   continue;
  

•           queue->fileio.blocks[i]->state = IIO_BLOCK_STATE_DEAD;
  

•   }
  

•   spin_unlock_irq(&queue->list_lock);
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
  

•           if (!queue->fileio.blocks[i])
  

•                   continue;
  

•           iio_buffer_block_put(queue->fileio.blocks[i]);
  

•           queue->fileio.blocks[i] = NULL;
  

•   }
  

•   queue->fileio.active_block = NULL;
  

+}

static void iio_dma_buffer_submit_block(struct iio_dma_buffer_queue *queue, struct iio_dma_buffer_block *block) { @@ -681,25 +702,9 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_init); */ void iio_dma_buffer_exit(struct iio_dma_buffer_queue *queue) {

•   unsigned int i;
  

•   mutex_lock(&queue->lock);
  
  

•   spin_lock_irq(&queue->list_lock);
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
  

•           if (!queue->fileio.blocks[i])
  

•                   continue;
  

•           queue->fileio.blocks[i]->state = IIO_BLOCK_STATE_DEAD;
  

•   }
  

•   spin_unlock_irq(&queue->list_lock);
  

•   for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
  

•           if (!queue->fileio.blocks[i])
  

•                   continue;
  

•           iio_buffer_block_put(queue->fileio.blocks[i]);
  

•           queue->fileio.blocks[i] = NULL;
  

•   }
  

•   queue->fileio.active_block = NULL;
  

•   iio_dma_buffer_fileio_free(queue);
    queue->ops = NULL;
  
    mutex_unlock(&queue->lock);
  

-- 2.33.0

Hi,

Le lun., nov. 15 2021 at 14:19:21 +0000, Paul Cercueil paul@crapouillou.net a écrit :

We can be certain that the input buffers will only be accessed by userspace for reading, and output buffers will mostly be accessed by userspace for writing.

Therefore, it makes more sense to use only fully cached input buffers, and to use the write-combine cache coherency setting for output buffers.

This boosts performance, as the data written to the output buffers does not have to be sync'd for coherency. It will halve performance if the userspace application tries to read from the output buffer, but this should never happen.

Since we don't need to sync the cache when disabling CPU access either for input buffers or output buffers, the .end_cpu_access() callback can be dropped completely.

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

drivers/iio/buffer/industrialio-buffer-dma.c | 82 +++++++++++++------- 1 file changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index 92356ee02f30..fb39054d8c15 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -229,8 +229,33 @@ static int iio_buffer_dma_buf_mmap(struct dma_buf *dbuf, if (vma->vm_ops->open) vma->vm_ops->open(vma);

• return dma_mmap_pages(dev, vma, vma->vm_end - vma->vm_start,

• 	      virt_to_page(block->vaddr));
  

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

• /*
  

•  * With an input buffer, userspace will only read the data and
  

•  * never write. We can mmap the buffer fully cached.
  

•  */
  

• return dma_mmap_pages(dev, vma, vma->vm_end - vma->vm_start,
  

• 		      virt_to_page(block->vaddr));
  

• } else {

• /*
  

•  * With an output buffer, userspace will only write the data
  

•  * and should rarely (if never) read from it. It is better to
  

•  * use write-combine in this case.
  

•  */
  

• return dma_mmap_wc(dev, vma, block->vaddr, block->phys_addr,
  

• 		   vma->vm_end - vma->vm_start);
  

• }

+}

+static void iio_dma_buffer_free_dmamem(struct iio_dma_buffer_block *block) +{

• struct device *dev = block->queue->dev;
• size_t size = PAGE_ALIGN(block->size);
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_IN)

• dma_free_coherent(dev, size, block->vaddr, block->phys_addr);
  

• else

• dma_free_wc(dev, size, block->vaddr, block->phys_addr);
  

}

static void iio_buffer_dma_buf_release(struct dma_buf *dbuf) @@ -243,9 +268,7 @@ static void iio_buffer_dma_buf_release(struct dma_buf *dbuf)

mutex_lock(&queue->lock);

• dma_free_coherent(queue->dev, PAGE_ALIGN(block->size),

• 	  block->vaddr, block->phys_addr);
  

• iio_dma_buffer_free_dmamem(block); kfree(block);

  queue->num_blocks--;

@@ -268,19 +291,6 @@ static int iio_buffer_dma_buf_begin_cpu_access(struct dma_buf *dbuf, return 0; }

-static int iio_buffer_dma_buf_end_cpu_access(struct dma_buf *dbuf,

• 			     enum dma_data_direction dma_dir)
  

-{

• struct iio_dma_buffer_block *block = dbuf->priv;
• struct device *dev = block->queue->dev;
• /* We only need to sync the cache for output buffers */
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_OUT)

• dma_sync_single_for_device(dev, block->phys_addr, block->size, 
  

dma_dir);

• return 0;

-}

static const struct dma_buf_ops iio_dma_buffer_dmabuf_ops = { .attach = iio_buffer_dma_buf_attach, .map_dma_buf = iio_buffer_dma_buf_map, @@ -288,9 +298,28 @@ static const struct dma_buf_ops iio_dma_buffer_dmabuf_ops = { .mmap = iio_buffer_dma_buf_mmap, .release = iio_buffer_dma_buf_release, .begin_cpu_access = iio_buffer_dma_buf_begin_cpu_access,

• .end_cpu_access = iio_buffer_dma_buf_end_cpu_access,

};

+static int iio_dma_buffer_alloc_dmamem(struct iio_dma_buffer_block *block) +{

• struct device *dev = block->queue->dev;
• size_t size = PAGE_ALIGN(block->size);
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_IN) {

• block->vaddr = dma_alloc_coherent(dev, size,
  

• 				  &block->phys_addr,
  

• 				  GFP_KERNEL);
  

I'm so used to dma_alloc_noncoherent() that I didn't even notice that it was dma_alloc_coherent() here. The code I added meant to work with non-coherent memory - hence the dma_sync_* operations and the use of dma_mmap_pages().

I'll fix that in V2.

Cheers, -Paul

• } else {

• block->vaddr = dma_alloc_wc(dev, size,
  

• 			    &block->phys_addr,
  

• 			    GFP_KERNEL);
  

• }
• if (!block->vaddr)

• return -ENOMEM;
  

• return 0;

+}

static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( struct iio_dma_buffer_queue *queue, size_t size, bool fileio) { @@ -303,12 +332,12 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( if (!block) return ERR_PTR(-ENOMEM);

• block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size),

• &block->phys_addr, GFP_KERNEL);
  

• if (!block->vaddr) {

• err = -ENOMEM;
  

• block->size = size;
• block->queue = queue;
• err = iio_dma_buffer_alloc_dmamem(block);
• if (err) goto err_free_block;

• }

  einfo.ops = &iio_dma_buffer_dmabuf_ops; einfo.size = PAGE_ALIGN(size);

@@ -322,10 +351,8 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( }

block->dmabuf = dmabuf;

• block->size = size; block->bytes_used = size; block->state = IIO_BLOCK_STATE_DONE;
• block->queue = queue; block->fileio = fileio; INIT_LIST_HEAD(&block->head);

@@ -338,8 +365,7 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( return block;

err_free_dma:

• dma_free_coherent(queue->dev, PAGE_ALIGN(size),

• 	  block->vaddr, block->phys_addr);
  

• iio_dma_buffer_free_dmamem(block);

err_free_block: kfree(block); return ERR_PTR(err); -- 2.33.0

Hi Jonathan,

Le dim., nov. 21 2021 at 15:00:37 +0000, Jonathan Cameron jic23@kernel.org a écrit :

On Mon, 15 Nov 2021 14:19:21 +0000 Paul Cercueil paul@crapouillou.net wrote:

...

We can be certain that the input buffers will only be accessed by userspace for reading, and output buffers will mostly be accessed by userspace for writing.

Mostly? Perhaps a little more info on why that's not 'only'.

Just like with a framebuffer, it really depends on what the application does. Most of the cases it will just read sequentially an input buffer, or write sequentially an output buffer. But then you get the exotic application that will try to do something like alpha blending, which means read+write. Hence "mostly".

...

Therefore, it makes more sense to use only fully cached input buffers, and to use the write-combine cache coherency setting for output buffers.

This boosts performance, as the data written to the output buffers does not have to be sync'd for coherency. It will halve performance if the userspace application tries to read from the output buffer, but this should never happen.

Since we don't need to sync the cache when disabling CPU access either for input buffers or output buffers, the .end_cpu_access() callback can be dropped completely.

We have an odd mix of coherent and non coherent DMA in here as you noted, but are you sure this is safe on all platforms?

The mix isn't safe, but using only coherent or only non-coherent should be safe, yes.

...

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

Any numbers to support this patch? The mapping types are performance optimisations so nice to know how much of a difference they make.

Output buffers are definitely faster in write-combine mode. On a ZedBoard with a AD9361 transceiver set to 66 MSPS, and buffer/size set to 8192, I would get about 185 MiB/s before, 197 MiB/s after.

Input buffers... early results are mixed. On ARM32 it does look like it is slightly faster to read from *uncached* memory than reading from cached memory. The cache sync does take a long time.

Other architectures might have a different result, for instance on MIPS invalidating the cache is a very fast operation, so using cached buffers would be a huge win in performance.

Setups where the DMA operations are coherent also wouldn't require any cache sync and this patch would give a huge win in performance.

I'll run some more tests next week to have some fresh numbers.

Cheers, -Paul

...

---

drivers/iio/buffer/industrialio-buffer-dma.c | 82 +++++++++++++------- 1 file changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index 92356ee02f30..fb39054d8c15 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -229,8 +229,33 @@ static int iio_buffer_dma_buf_mmap(struct dma_buf *dbuf, if (vma->vm_ops->open) vma->vm_ops->open(vma);

• return dma_mmap_pages(dev, vma, vma->vm_end - vma->vm_start,

• 	      virt_to_page(block->vaddr));
  

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

• /*
  

•  * With an input buffer, userspace will only read the data and
  

•  * never write. We can mmap the buffer fully cached.
  

•  */
  

• return dma_mmap_pages(dev, vma, vma->vm_end - vma->vm_start,
  

• 		      virt_to_page(block->vaddr));
  

• } else {

• /*
  

•  * With an output buffer, userspace will only write the data
  

•  * and should rarely (if never) read from it. It is better to
  

•  * use write-combine in this case.
  

•  */
  

• return dma_mmap_wc(dev, vma, block->vaddr, block->phys_addr,
  

• 		   vma->vm_end - vma->vm_start);
  

• }

+}

+static void iio_dma_buffer_free_dmamem(struct iio_dma_buffer_block *block) +{

• struct device *dev = block->queue->dev;
• size_t size = PAGE_ALIGN(block->size);
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_IN)

• dma_free_coherent(dev, size, block->vaddr, block->phys_addr);
  

• else

• dma_free_wc(dev, size, block->vaddr, block->phys_addr);
  

}

static void iio_buffer_dma_buf_release(struct dma_buf *dbuf) @@ -243,9 +268,7 @@ static void iio_buffer_dma_buf_release(struct dma_buf *dbuf)

mutex_lock(&queue->lock);

• dma_free_coherent(queue->dev, PAGE_ALIGN(block->size),

• 	  block->vaddr, block->phys_addr);
  

• iio_dma_buffer_free_dmamem(block); kfree(block);

  queue->num_blocks--;

@@ -268,19 +291,6 @@ static int iio_buffer_dma_buf_begin_cpu_access(struct dma_buf *dbuf, return 0; }

-static int iio_buffer_dma_buf_end_cpu_access(struct dma_buf *dbuf,

• 			     enum dma_data_direction dma_dir)
  

-{

• struct iio_dma_buffer_block *block = dbuf->priv;
• struct device *dev = block->queue->dev;
• /* We only need to sync the cache for output buffers */
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_OUT)

• dma_sync_single_for_device(dev, block->phys_addr, block->size, 
  

dma_dir);

• return 0;

-}

static const struct dma_buf_ops iio_dma_buffer_dmabuf_ops = { .attach = iio_buffer_dma_buf_attach, .map_dma_buf = iio_buffer_dma_buf_map, @@ -288,9 +298,28 @@ static const struct dma_buf_ops iio_dma_buffer_dmabuf_ops = { .mmap = iio_buffer_dma_buf_mmap, .release = iio_buffer_dma_buf_release, .begin_cpu_access = iio_buffer_dma_buf_begin_cpu_access,

• .end_cpu_access = iio_buffer_dma_buf_end_cpu_access,

};

+static int iio_dma_buffer_alloc_dmamem(struct iio_dma_buffer_block *block) +{

• struct device *dev = block->queue->dev;
• size_t size = PAGE_ALIGN(block->size);
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_IN) {

• block->vaddr = dma_alloc_coherent(dev, size,
  

• 				  &block->phys_addr,
  

• 				  GFP_KERNEL);
  

• } else {

• block->vaddr = dma_alloc_wc(dev, size,
  

• 			    &block->phys_addr,
  

• 			    GFP_KERNEL);
  

• }
• if (!block->vaddr)

• return -ENOMEM;
  

• return 0;

+}

static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( struct iio_dma_buffer_queue *queue, size_t size, bool fileio) { @@ -303,12 +332,12 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( if (!block) return ERR_PTR(-ENOMEM);

• block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size),

• &block->phys_addr, GFP_KERNEL);
  

• if (!block->vaddr) {

• err = -ENOMEM;
  

• block->size = size;
• block->queue = queue;
• err = iio_dma_buffer_alloc_dmamem(block);
• if (err) goto err_free_block;

• }

  einfo.ops = &iio_dma_buffer_dmabuf_ops; einfo.size = PAGE_ALIGN(size);

@@ -322,10 +351,8 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( }

block->dmabuf = dmabuf;

• block->size = size; block->bytes_used = size; block->state = IIO_BLOCK_STATE_DONE;
• block->queue = queue; block->fileio = fileio; INIT_LIST_HEAD(&block->head);

@@ -338,8 +365,7 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( return block;

err_free_dma:

• dma_free_coherent(queue->dev, PAGE_ALIGN(size),

• 	  block->vaddr, block->phys_addr);
  

• iio_dma_buffer_free_dmamem(block);

err_free_block: kfree(block); return ERR_PTR(err);

On Sun, 21 Nov 2021 17:43:20 +0000 Paul Cercueil paul@crapouillou.net wrote:

Hi Jonathan,

Le dim., nov. 21 2021 at 15:00:37 +0000, Jonathan Cameron jic23@kernel.org a écrit :

...

On Mon, 15 Nov 2021 14:19:21 +0000 Paul Cercueil paul@crapouillou.net wrote:

...

We can be certain that the input buffers will only be accessed by userspace for reading, and output buffers will mostly be accessed by userspace for writing.

Mostly? Perhaps a little more info on why that's not 'only'.

Just like with a framebuffer, it really depends on what the application does. Most of the cases it will just read sequentially an input buffer, or write sequentially an output buffer. But then you get the exotic application that will try to do something like alpha blending, which means read+write. Hence "mostly".

Ok. That makes sense though I hope no one actually does it, we can't prevent them doing so.

...

...

Therefore, it makes more sense to use only fully cached input buffers, and to use the write-combine cache coherency setting for output buffers.

This boosts performance, as the data written to the output buffers does not have to be sync'd for coherency. It will halve performance if the userspace application tries to read from the output buffer, but this should never happen.

Since we don't need to sync the cache when disabling CPU access either for input buffers or output buffers, the .end_cpu_access() callback can be dropped completely.

We have an odd mix of coherent and non coherent DMA in here as you noted, but are you sure this is safe on all platforms?

The mix isn't safe, but using only coherent or only non-coherent should be safe, yes.

yup

...

...

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

Any numbers to support this patch? The mapping types are performance optimisations so nice to know how much of a difference they make.

Output buffers are definitely faster in write-combine mode. On a ZedBoard with a AD9361 transceiver set to 66 MSPS, and buffer/size set to 8192, I would get about 185 MiB/s before, 197 MiB/s after.

Input buffers... early results are mixed. On ARM32 it does look like it is slightly faster to read from *uncached* memory than reading from cached memory. The cache sync does take a long time.

Other architectures might have a different result, for instance on MIPS invalidating the cache is a very fast operation, so using cached buffers would be a huge win in performance.

Setups where the DMA operations are coherent also wouldn't require any cache sync and this patch would give a huge win in performance.

I'll run some more tests next week to have some fresh numbers.

Great.

Thanks,

Jonathan

Cheers, -Paul

...

...

---

drivers/iio/buffer/industrialio-buffer-dma.c | 82 +++++++++++++------- 1 file changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index 92356ee02f30..fb39054d8c15 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -229,8 +229,33 @@ static int iio_buffer_dma_buf_mmap(struct dma_buf *dbuf, if (vma->vm_ops->open) vma->vm_ops->open(vma);

• return dma_mmap_pages(dev, vma, vma->vm_end - vma->vm_start,

• 	      virt_to_page(block->vaddr));
  

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

• /*
  

•  * With an input buffer, userspace will only read the data and
  

•  * never write. We can mmap the buffer fully cached.
  

•  */
  

• return dma_mmap_pages(dev, vma, vma->vm_end - vma->vm_start,
  

• 		      virt_to_page(block->vaddr));
  

• } else {

• /*
  

•  * With an output buffer, userspace will only write the data
  

•  * and should rarely (if never) read from it. It is better to
  

•  * use write-combine in this case.
  

•  */
  

• return dma_mmap_wc(dev, vma, block->vaddr, block->phys_addr,
  

• 		   vma->vm_end - vma->vm_start);
  

• }

+}

+static void iio_dma_buffer_free_dmamem(struct iio_dma_buffer_block *block) +{

• struct device *dev = block->queue->dev;
• size_t size = PAGE_ALIGN(block->size);
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_IN)

• dma_free_coherent(dev, size, block->vaddr, block->phys_addr);
  

• else

• dma_free_wc(dev, size, block->vaddr, block->phys_addr);
  

}

static void iio_buffer_dma_buf_release(struct dma_buf *dbuf) @@ -243,9 +268,7 @@ static void iio_buffer_dma_buf_release(struct dma_buf *dbuf)

mutex_lock(&queue->lock);

• dma_free_coherent(queue->dev, PAGE_ALIGN(block->size),

• 	  block->vaddr, block->phys_addr);
  

• iio_dma_buffer_free_dmamem(block); kfree(block);

  queue->num_blocks--;

@@ -268,19 +291,6 @@ static int iio_buffer_dma_buf_begin_cpu_access(struct dma_buf *dbuf, return 0; }

-static int iio_buffer_dma_buf_end_cpu_access(struct dma_buf *dbuf,

• 			     enum dma_data_direction dma_dir)
  

-{

• struct iio_dma_buffer_block *block = dbuf->priv;
• struct device *dev = block->queue->dev;
• /* We only need to sync the cache for output buffers */
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_OUT)

• dma_sync_single_for_device(dev, block->phys_addr, block->size, 
  

dma_dir);

• return 0;

-}

static const struct dma_buf_ops iio_dma_buffer_dmabuf_ops = { .attach = iio_buffer_dma_buf_attach, .map_dma_buf = iio_buffer_dma_buf_map, @@ -288,9 +298,28 @@ static const struct dma_buf_ops iio_dma_buffer_dmabuf_ops = { .mmap = iio_buffer_dma_buf_mmap, .release = iio_buffer_dma_buf_release, .begin_cpu_access = iio_buffer_dma_buf_begin_cpu_access,

• .end_cpu_access = iio_buffer_dma_buf_end_cpu_access,

};

+static int iio_dma_buffer_alloc_dmamem(struct iio_dma_buffer_block *block) +{

• struct device *dev = block->queue->dev;
• size_t size = PAGE_ALIGN(block->size);
• if (block->queue->buffer.direction == IIO_BUFFER_DIRECTION_IN) {

• block->vaddr = dma_alloc_coherent(dev, size,
  

• 				  &block->phys_addr,
  

• 				  GFP_KERNEL);
  

• } else {

• block->vaddr = dma_alloc_wc(dev, size,
  

• 			    &block->phys_addr,
  

• 			    GFP_KERNEL);
  

• }
• if (!block->vaddr)

• return -ENOMEM;
  

• return 0;

+}

static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( struct iio_dma_buffer_queue *queue, size_t size, bool fileio) { @@ -303,12 +332,12 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( if (!block) return ERR_PTR(-ENOMEM);

• block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size),

• &block->phys_addr, GFP_KERNEL);
  

• if (!block->vaddr) {

• err = -ENOMEM;
  

• block->size = size;
• block->queue = queue;
• err = iio_dma_buffer_alloc_dmamem(block);
• if (err) goto err_free_block;

• }

  einfo.ops = &iio_dma_buffer_dmabuf_ops; einfo.size = PAGE_ALIGN(size);

@@ -322,10 +351,8 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( }

block->dmabuf = dmabuf;

• block->size = size; block->bytes_used = size; block->state = IIO_BLOCK_STATE_DONE;
• block->queue = queue; block->fileio = fileio; INIT_LIST_HEAD(&block->head);

@@ -338,8 +365,7 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block( return block;

err_free_dma:

• dma_free_coherent(queue->dev, PAGE_ALIGN(size),

• 	  block->vaddr, block->phys_addr);
  

• iio_dma_buffer_free_dmamem(block);

err_free_block: kfree(block); return ERR_PTR(err);

Introduce a new flag IIO_BUFFER_DMABUF_CYCLIC in the "flags" field of the iio_dmabuf uapi structure.

When set, the DMABUF enqueued with the enqueue ioctl will be endlessly repeated on the TX output, until the buffer is disabled.

Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/iio/industrialio-buffer.c | 5 +++++ include/uapi/linux/iio/buffer.h | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 30910e6c2346..41bc51c88002 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -1600,6 +1600,11 @@ static int iio_buffer_enqueue_dmabuf(struct iio_buffer *buffer, if (dmabuf.flags & ~IIO_BUFFER_DMABUF_SUPPORTED_FLAGS) return -EINVAL;

+ /* Cyclic flag is only supported on output buffers */ + if ((dmabuf.flags & IIO_BUFFER_DMABUF_CYCLIC) && + buffer->direction != IIO_BUFFER_DIRECTION_OUT) + return -EINVAL; + return buffer->access->enqueue_dmabuf(buffer, &dmabuf); }

diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h index e4621b926262..2d541d038c02 100644 --- a/include/uapi/linux/iio/buffer.h +++ b/include/uapi/linux/iio/buffer.h @@ -7,7 +7,8 @@

#include <linux/types.h>

-#define IIO_BUFFER_DMABUF_SUPPORTED_FLAGS 0x00000000 +#define IIO_BUFFER_DMABUF_CYCLIC (1 << 0) +#define IIO_BUFFER_DMABUF_SUPPORTED_FLAGS 0x00000001

/** * struct iio_dmabuf_alloc_req - Descriptor for allocating IIO DMABUFs

Handle the IIO_BUFFER_DMABUF_CYCLIC flag to support cyclic buffers.

Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/iio/buffer/industrialio-buffer-dma.c | 1 + .../iio/buffer/industrialio-buffer-dmaengine.c | 15 ++++++++++++--- include/linux/iio/buffer-dma.h | 3 +++ 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c index fb39054d8c15..6658f103ee17 100644 --- a/drivers/iio/buffer/industrialio-buffer-dma.c +++ b/drivers/iio/buffer/industrialio-buffer-dma.c @@ -933,6 +933,7 @@ int iio_dma_buffer_enqueue_dmabuf(struct iio_buffer *buffer, }

dma_block->bytes_used = iio_dmabuf->bytes_used ?: dma_block->size; + dma_block->cyclic = iio_dmabuf->flags & IIO_BUFFER_DMABUF_CYCLIC;

switch (dma_block->state) { case IIO_BLOCK_STATE_QUEUED: diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c index 57a8b2e4ba3c..952e2160a11e 100644 --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c @@ -81,9 +81,18 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue, if (!block->bytes_used || 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); + if (block->cyclic) { + desc = dmaengine_prep_dma_cyclic(dmaengine_buffer->chan, + block->phys_addr, + block->size, + block->bytes_used, + dma_dir, 0); + } else { + desc = dmaengine_prep_slave_single(dmaengine_buffer->chan, + block->phys_addr, + block->bytes_used, dma_dir, + DMA_PREP_INTERRUPT); + } if (!desc) return -ENOMEM;

diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h index 85e55fe35282..27639fdf7b54 100644 --- a/include/linux/iio/buffer-dma.h +++ b/include/linux/iio/buffer-dma.h @@ -42,6 +42,7 @@ enum iio_block_state { * @phys_addr: Physical address of the blocks memory * @queue: Parent DMA buffer queue * @state: Current state of the block + * @cyclic: True if this is a cyclic buffer * @fileio: True if this buffer is used for fileio mode * @dmabuf: Underlying DMABUF object */ @@ -65,6 +66,8 @@ struct iio_dma_buffer_block { */ enum iio_block_state state;

+ bool cyclic; + bool fileio; struct dma_buf *dmabuf; };

Document the new DMABUF based API.

Signed-off-by: Paul Cercueil paul@crapouillou.net --- Documentation/driver-api/dma-buf.rst | 2 + Documentation/iio/dmabuf_api.rst | 94 ++++++++++++++++++++++++++++ Documentation/iio/index.rst | 2 + 3 files changed, 98 insertions(+) create mode 100644 Documentation/iio/dmabuf_api.rst

diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index 2cd7db82d9fe..d3c9b58d2706 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -1,3 +1,5 @@ +.. _dma-buf: + Buffer Sharing and Synchronization ==================================

diff --git a/Documentation/iio/dmabuf_api.rst b/Documentation/iio/dmabuf_api.rst new file mode 100644 index 000000000000..b4e120a4ef0c --- /dev/null +++ b/Documentation/iio/dmabuf_api.rst @@ -0,0 +1,94 @@ +=================================== +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 +application can allocate and append DMABUF objects to the buffer's queue. + +The advantage of this DMABUF based interface vs. the fileio +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. + +The data in this DMABUF interface is managed at the granularity of +DMABUF objects. Reducing the granularity from byte level to block level +is done to reduce the userspace-kernelspace synchronization overhead +since performing syscalls for each byte at a few Mbps is just not +feasible. + +This of course leads to a slightly increased latency. For this reason an +application can choose the size of the DMABUFs as well as how many it +allocates. E.g. two DMABUFs would be a traditional double buffering +scheme. But using a higher number might be necessary to avoid +underflow/overflow situations in the presence of scheduling latencies. + +2. User API +=========== + +``IIO_BUFFER_DMABUF_ALLOC_IOCTL(struct iio_dmabuf_alloc_req *)`` +---------------------------------------------------------------- + +Each call will allocate a new DMABUF object. The return value (if not +a negative errno value as error) will be the file descriptor of the new +DMABUF. + +``IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *)`` +-------------------------------------------------------- + +Place the DMABUF object into the queue pending for hardware process. + +These two IOCTLs have to be performed on the IIO buffer's file +descriptor (either opened from the corresponding /dev/iio:deviceX, or +obtained using the `IIO_BUFFER_GET_FD_IOCTL` ioctl). + +3. Usage +======== + +To access the data stored in a block by userspace the block must be +mapped to the process's memory. This is done by calling mmap() on the +DMABUF's file descriptor. + +Before accessing the data through the map, you must use the +DMA_BUF_IOCTL_SYNC(struct dma_buf_sync *) ioctl, with the +DMA_BUF_SYNC_START flag, to make sure that the data is available. +This call may block until the hardware is done with this block. Once +you are done reading or writing the data, you must use this ioctl again +with the DMA_BUF_SYNC_END flag, before enqueueing the DMABUF to the +kernel's queue. + +If you need to know when the hardware is done with a DMABUF, you can +poll its file descriptor for the EPOLLOUT event. + +Finally, to destroy a DMABUF object, simply call close() on its file +descriptor. + +For more information about manipulating DMABUF objects, see: :ref:`dma-buf`. + +A typical workflow for the new interface is: + + for block in blocks: + DMABUF_ALLOC block + mmap block + + enable buffer + + while !done + for block in blocks: + DMABUF_ENQUEUE block + + DMABUF_SYNC_START block + process data + DMABUF_SYNC_END block + + disable buffer + + for block in blocks: + close block diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst index 58b7a4ebac51..9ce799fbf262 100644 --- a/Documentation/iio/index.rst +++ b/Documentation/iio/index.rst @@ -10,3 +10,5 @@ Industrial I/O iio_configfs

ep93xx_adc + + dmabuf_api

Hi Jonathan,

Le dim., nov. 21 2021 at 15:10:26 +0000, Jonathan Cameron jic23@kernel.org a écrit :

On Mon, 15 Nov 2021 14:22:43 +0000 Paul Cercueil paul@crapouillou.net wrote:

...

Document the new DMABUF based API.

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

Hi Paul,

A few trivial things inline but looks good to me if we do end up using DMABUF anyway.

Jonathan

...

---

Documentation/driver-api/dma-buf.rst | 2 + Documentation/iio/dmabuf_api.rst | 94 ++++++++++++++++++++++++++++ Documentation/iio/index.rst | 2 + 3 files changed, 98 insertions(+) create mode 100644 Documentation/iio/dmabuf_api.rst

diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index 2cd7db82d9fe..d3c9b58d2706 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -1,3 +1,5 @@ +.. _dma-buf:

Why this change?

I have this line in the file: For more information about manipulating DMABUF objects, see: :ref:`dma-buf`.

For the :ref: to work I need a label at the reference point, if I understood correctly.

...

Buffer Sharing and Synchronization

diff --git a/Documentation/iio/dmabuf_api.rst b/Documentation/iio/dmabuf_api.rst new file mode 100644 index 000000000000..b4e120a4ef0c --- /dev/null +++ b/Documentation/iio/dmabuf_api.rst @@ -0,0 +1,94 @@ +=================================== +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 +application can allocate and append DMABUF objects to the buffer's queue.

I would note somewhere that this interface is optional for a given IIO driver. I don't want people to start assuming their i2c ADC will support this and wondering why it doesn't work :)

Their I2C ADC will support it, as long as the driver supports the dmaengine buffer interface. I can make that explicit, yes.

...

+The advantage of this DMABUF based interface vs. the fileio +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.

+The data in this DMABUF interface is managed at the granularity of +DMABUF objects. Reducing the granularity from byte level to block level +is done to reduce the userspace-kernelspace synchronization overhead +since performing syscalls for each byte at a few Mbps is just not +feasible.

+This of course leads to a slightly increased latency. For this reason an +application can choose the size of the DMABUFs as well as how many it +allocates. E.g. two DMABUFs would be a traditional double buffering +scheme. But using a higher number might be necessary to avoid +underflow/overflow situations in the presence of scheduling latencies.

+2. User API +===========

+``IIO_BUFFER_DMABUF_ALLOC_IOCTL(struct iio_dmabuf_alloc_req *)`` +----------------------------------------------------------------

+Each call will allocate a new DMABUF object. The return value (if not +a negative errno value as error) will be the file descriptor of the new +DMABUF.

+``IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *)`` +--------------------------------------------------------

+Place the DMABUF object into the queue pending for hardware process.

+These two IOCTLs have to be performed on the IIO buffer's file +descriptor (either opened from the corresponding /dev/iio:deviceX, or +obtained using the `IIO_BUFFER_GET_FD_IOCTL` ioctl).

+3. Usage +========

+To access the data stored in a block by userspace the block must be +mapped to the process's memory. This is done by calling mmap() on the +DMABUF's file descriptor.

+Before accessing the data through the map, you must use the +DMA_BUF_IOCTL_SYNC(struct dma_buf_sync *) ioctl, with the +DMA_BUF_SYNC_START flag, to make sure that the data is available. +This call may block until the hardware is done with this block. Once +you are done reading or writing the data, you must use this ioctl again +with the DMA_BUF_SYNC_END flag, before enqueueing the DMABUF to the +kernel's queue.

+If you need to know when the hardware is done with a DMABUF, you can +poll its file descriptor for the EPOLLOUT event.

+Finally, to destroy a DMABUF object, simply call close() on its file +descriptor.

+For more information about manipulating DMABUF objects, see: :ref:`dma-buf`.

+A typical workflow for the new interface is:

• for block in blocks:

•  DMABUF_ALLOC block
  

•  mmap block
  

• enable buffer
• while !done

•  for block in blocks:
  

•    DMABUF_ENQUEUE block
  

•    DMABUF_SYNC_START block
  

•    process data
  

•    DMABUF_SYNC_END block
  

• disable buffer
• for block in blocks:

•  close block
  

diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst index 58b7a4ebac51..9ce799fbf262 100644 --- a/Documentation/iio/index.rst +++ b/Documentation/iio/index.rst @@ -10,3 +10,5 @@ Industrial I/O iio_configfs

 ep93xx_adc

• dmabuf_api

Given this is core stuff rather than driver specific, perhaps move it up a few lines?

Alright.

Cheers, -Paul