[V3 was: "iio: new DMABUF based API, v3"][1]
Hi Jonathan,
This is a subset of my patchset that introduced a new interface based on
DMABUF objects [1]. It adds write() support to the IIO buffer
infrastructure.
The reason it is not the full IIO-DMABUF patchset, is because you
requested performance benchmarks - and our current numbers are barely
better (~ +10%) than the fileio interface. There is a good reason for
that: V3 of the patchset switched from having the IIO core creating the
DMABUFs backed by physically contiguous memory, to having the IIO core
being a simple DMABUF importer, and having the DMABUFs created
externally. We now use the udmabuf driver to create those, and they are
allocated from paged memory. While this works perfectly fine, our
buffers are now cut in 4 KiB chunks (pages), non-contiguous in memory,
which causes the DMA hardware to create an IRQ storm, as it raises an
interrupt after each 4 KiB in the worst case scenario.
Anyway, this is not directly a problem of the IIO-DMABUF code - but I
can't really upstream a shiny new interface that I claim is much faster,
without giving numbers.
So while we fix this (either by updating the DMA IP and driver to
support scatter-gather, or by hacking something quick to give us
physically contiguous DMABUFs just for the benchmark), I thought it
would make sense to upstream the few patches of the V3 patchset that are
needed for the IIO-DMABUF interface but aren't directly related.
As for write() support, Nuno (Cc'd) said he will work on upstreaming the
DAC counterpart of adc/adi-axi-adc.c in the next few weeks, so there
will be a user for the buffer write() support. I hope you are okay with
this - otherwise, we can just wait until this work is done and submit it
all at once.
Changelog since v3:
- [PATCH 2/6] is new;
- [PATCH 3/6]: Drop iio_dma_buffer_space_available() function, and
update patch description accordingly;
- [PATCH 6/6]: .space_available is now set to iio_dma_buffer_usage
(which is functionally the exact same).
Cheers,
-Paul
[1] https://lore.kernel.org/all/[email protected]/
Alexandru Ardelean (1):
iio: buffer-dma: split iio_dma_buffer_fileio_free() function
Paul Cercueil (5):
iio: buffer-dma: Get rid of outgoing queue
iio: buffer-dma: Rename iio_dma_buffer_data_available()
iio: buffer-dma: Enable buffer write support
iio: buffer-dmaengine: Support specifying buffer direction
iio: buffer-dmaengine: Enable write support
drivers/iio/adc/adi-axi-adc.c | 3 +-
drivers/iio/buffer/industrialio-buffer-dma.c | 187 ++++++++++++------
.../buffer/industrialio-buffer-dmaengine.c | 28 ++-
include/linux/iio/buffer-dma.h | 11 +-
include/linux/iio/buffer-dmaengine.h | 5 +-
5 files changed, 160 insertions(+), 74 deletions(-)
--
2.40.1
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.
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 <[email protected]>
Reviewed-by: Alexandru Ardelean <[email protected]>
---
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".
v4: Drop iio_dma_buffer_space_available() function, and update patch
description accordingly.
---
drivers/iio/buffer/industrialio-buffer-dma.c | 89 ++++++++++++++++----
include/linux/iio/buffer-dma.h | 2 +
2 files changed, 75 insertions(+), 16 deletions(-)
diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index 764f1400a545..e00b704941b6 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_usage() - DMA buffer data_available and
* space_available callback
diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
index 52a838ec0e57..6e27e47077d5 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_usage(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);
--
2.40.1
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 <[email protected]>
Reviewed-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/adi-axi-adc.c | 3 ++-
.../buffer/industrialio-buffer-dmaengine.c | 24 +++++++++++++++----
include/linux/iio/buffer-dmaengine.h | 5 +++-
3 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index e8a8ea4140f1..d33574b5417a 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -114,7 +114,8 @@ static int adi_axi_adc_config_dma_buffer(struct device *dev,
dma_name = "rx";
return devm_iio_dmaengine_buffer_setup(indio_dev->dev.parent,
- indio_dev, dma_name);
+ indio_dev, dma_name,
+ IIO_BUFFER_DIRECTION_IN);
}
static int adi_axi_adc_read_raw(struct iio_dev *indio_dev,
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index 7b49f85af064..deae5a4ac03d 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;
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;
@@ -275,7 +286,8 @@ static struct iio_buffer *devm_iio_dmaengine_buffer_alloc(struct device *dev,
*/
int devm_iio_dmaengine_buffer_setup(struct device *dev,
struct iio_dev *indio_dev,
- const char *channel)
+ const char *channel,
+ enum iio_buffer_direction dir)
{
struct iio_buffer *buffer;
@@ -286,6 +298,8 @@ int devm_iio_dmaengine_buffer_setup(struct device *dev,
indio_dev->modes |= INDIO_BUFFER_HARDWARE;
+ buffer->direction = dir;
+
return iio_device_attach_buffer(indio_dev, buffer);
}
EXPORT_SYMBOL_GPL(devm_iio_dmaengine_buffer_setup);
diff --git a/include/linux/iio/buffer-dmaengine.h b/include/linux/iio/buffer-dmaengine.h
index 5c355be89814..538d0479cdd6 100644
--- a/include/linux/iio/buffer-dmaengine.h
+++ b/include/linux/iio/buffer-dmaengine.h
@@ -7,11 +7,14 @@
#ifndef __IIO_DMAENGINE_H__
#define __IIO_DMAENGINE_H__
+#include <linux/iio/buffer.h>
+
struct iio_dev;
struct device;
int devm_iio_dmaengine_buffer_setup(struct device *dev,
struct iio_dev *indio_dev,
- const char *channel);
+ const char *channel,
+ enum iio_buffer_direction dir);
#endif
--
2.40.1
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 <[email protected]>
Reviewed-by: Alexandru Ardelean <[email protected]>
---
v4: .space_available is now set to iio_dma_buffer_usage (which is
functionally the exact same).
---
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 deae5a4ac03d..ef9d890ed3c9 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_usage,
+ .space_available = iio_dma_buffer_usage,
.release = iio_dmaengine_buffer_release,
.modes = INDIO_BUFFER_HARDWARE,
--
2.40.1
On Mon, 2023-08-07 at 13:21 +0200, Paul Cercueil wrote:
> [V3 was: "iio: new DMABUF based API, v3"][1]
>
> Hi Jonathan,
>
> This is a subset of my patchset that introduced a new interface based on
> DMABUF objects [1]. It adds write() support to the IIO buffer
> infrastructure.
>
> The reason it is not the full IIO-DMABUF patchset, is because you
> requested performance benchmarks - and our current numbers are barely
> better (~ +10%) than the fileio interface. There is a good reason for
> that: V3 of the patchset switched from having the IIO core creating the
> DMABUFs backed by physically contiguous memory, to having the IIO core
> being a simple DMABUF importer, and having the DMABUFs created
> externally. We now use the udmabuf driver to create those, and they are
> allocated from paged memory. While this works perfectly fine, our
> buffers are now cut in 4 KiB chunks (pages), non-contiguous in memory,
> which causes the DMA hardware to create an IRQ storm, as it raises an
> interrupt after each 4 KiB in the worst case scenario.
>
> Anyway, this is not directly a problem of the IIO-DMABUF code - but I
> can't really upstream a shiny new interface that I claim is much faster,
> without giving numbers.
>
> So while we fix this (either by updating the DMA IP and driver to
> support scatter-gather, or by hacking something quick to give us
> physically contiguous DMABUFs just for the benchmark), I thought it
> would make sense to upstream the few patches of the V3 patchset that are
> needed for the IIO-DMABUF interface but aren't directly related.
>
> As for write() support, Nuno (Cc'd) said he will work on upstreaming the
> DAC counterpart of adc/adi-axi-adc.c in the next few weeks, so there
> will be a user for the buffer write() support. I hope you are okay with
> this - otherwise, we can just wait until this work is done and submit it
> all at once.
>
Yeah, I've started that process last week:
https://lore.kernel.org/linux-iio/[email protected]/
the dac counterpart is actually missing in the RFC (as the goal of the RFC is
not review those drivers but the framework) but I do state that my plan is to
have it in the actual series where I actually mention we would need this work
:).
- Nuno Sá
On Wed, 2023-08-30 at 17:18 +0100, Jonathan Cameron wrote:
> On Wed, 30 Aug 2023 17:11:18 +0100
> Jonathan Cameron <[email protected]> wrote:
>
> > On Mon, 7 Aug 2023 13:21:07 +0200
> > Paul Cercueil <[email protected]> wrote:
> >
> > > [V3 was: "iio: new DMABUF based API, v3"][1]
> > >
> > > Hi Jonathan,
> > >
> > > This is a subset of my patchset that introduced a new interface based on
> > > DMABUF objects [1]. It adds write() support to the IIO buffer
> > > infrastructure.
> > >
> > > The reason it is not the full IIO-DMABUF patchset, is because you
> > > requested performance benchmarks - and our current numbers are barely
> > > better (~ +10%) than the fileio interface. There is a good reason for
> > > that: V3 of the patchset switched from having the IIO core creating the
> > > DMABUFs backed by physically contiguous memory, to having the IIO core
> > > being a simple DMABUF importer, and having the DMABUFs created
> > > externally. We now use the udmabuf driver to create those, and they are
> > > allocated from paged memory. While this works perfectly fine, our
> > > buffers are now cut in 4 KiB chunks (pages), non-contiguous in memory,
> > > which causes the DMA hardware to create an IRQ storm, as it raises an
> > > interrupt after each 4 KiB in the worst case scenario.
> >
> > Interesting. I'm guessing you don't necessarily need contiguous memory
> > and huge pages would get rid of most of that overhead?
> >
> > Given embedded target those huge pages are hard to get so you need
> > hugetlb support to improve the chances of it working. Some quick searching
> > suggests there is possible support on the way.
> > https://lore.kernel.org/linux-mm/[email protected]/
> >
> >
> > >
> > > Anyway, this is not directly a problem of the IIO-DMABUF code - but I
> > > can't really upstream a shiny new interface that I claim is much faster,
> > > without giving numbers.
> > >
> > > So while we fix this (either by updating the DMA IP and driver to
> > > support scatter-gather)
> >
> > Long run you almost always end up needing that unless contig requirements
> > are small and you want a robust solution. I'm guessing no IOMMU to pretend
> > it's all contiguous...
> >
> > > or by hacking something quick to give us
> > > physically contiguous DMABUFs just for the benchmark), I thought it
> > > would make sense to upstream the few patches of the V3 patchset that are
> > > needed for the IIO-DMABUF interface but aren't directly related.
> >
> > Good idea.
> >
> > >
> > > As for write() support, Nuno (Cc'd) said he will work on upstreaming the
> > > DAC counterpart of adc/adi-axi-adc.c in the next few weeks, so there
> > > will be a user for the buffer write() support. I hope you are okay with
> > > this - otherwise, we can just wait until this work is done and submit it
> > > all at once.
> >
> > Absolutely fine, though I won't pick this up without the user also being
> > ready to go.
>
>
> Having looked through these again, they are straight forward so no changes
> requested from me. Nuno, if you can add this set into appropriate
> point in your series that will make use of it that will make my life easier
> and ensure and minor rebasing etc happens without having to bother Paul.
>
Sure...
- Nuno Sá
>