Hi Jonathan,
Here's the v3 of my patchset that introduces a new interface based on
DMABUF objects to complement the fileio API, and adds write() support to
the existing fileio API.
It changed quite a lot since V2; the IIO subsystem is now just a DMABUF
importer, and all the complexity related to handling creation, deletion
and export of DMABUFs (including DMA mapping etc.) is gone.
This new interface will be used by Libiio. The code is ready[1] and will
be merged to the main branch as soon as the kernel bits are accepted
upstream.
Note that Libiio (and its server counterpart, iiod) use this new
interface in two different ways:
- by memory-mapping the DMABUFs to access the sample data directly,
which is much faster than using the existing fileio API as the sample
data does not need to be copied;
- by passing the DMABUFs around directly to the USB stack, in a
device-to-device zero-copy fashion, using a new DMABUF interface for
the USB (FunctionFS to be exact) stack, which is being upstreamed in
parallel of this patchset [2].
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 I still
benefit from sending this patchset early to get feedback.
Finally, the dmaengine implementation for this new interface requires a
new dmaengine API function, since dmaengine_prep_slave_sg() will always
transfer the full scatterlist unconditionally, while we want to be able
to transfer an arbitrary amount of bytes from/to the DMABUF. Since
scatterlists seem to be going away soon, the new API function will take
an array of DMA addresses + lengths. I am open to suggestions if anybody
(especially Vinod) have a better design in mind.
Cheers,
-Paul
[1]: https://github.com/analogdevicesinc/libiio/pull/928
[2]: https://lore.kernel.org/linux-usb/[email protected]/T/#t
---
Changelog:
* Patches 01-02 are new;
* Patches [03/11], [05/11] didn't change;
* Patch [04/11]:
- 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".
* Patch [07/11]:
- 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.
* Patch [09/11]:
Update code to provide the functions that will be used as callbacks
for the new IOCTLs.
* Patch [10/11]:
Use the new dmaengine_prep_slave_dma_array(), and adapt the code to
work with the new functions introduced in industrialio-buffer-dma.c.
* Patch [11/11]:
Update the documentation to reflect the new API.
---
Alexandru Ardelean (1):
iio: buffer-dma: split iio_dma_buffer_fileio_free() function
Paul Cercueil (10):
dmaengine: Add API function dmaengine_prep_slave_dma_array()
dmaengine: dma-axi-dmac: Implement device_prep_slave_dma_array
iio: buffer-dma: Get rid of outgoing queue
iio: buffer-dma: Enable buffer write support
iio: buffer-dmaengine: Support specifying buffer direction
iio: buffer-dmaengine: Enable write support
iio: core: Add new DMABUF interface infrastructure
iio: buffer-dma: Enable support for DMABUFs
iio: buffer-dmaengine: Support new DMABUF based userspace API
Documentation: iio: Document high-speed DMABUF based API
Documentation/iio/dmabuf_api.rst | 59 +++
Documentation/iio/index.rst | 2 +
drivers/dma/dma-axi-dmac.c | 41 ++
drivers/iio/adc/adi-axi-adc.c | 3 +-
drivers/iio/buffer/industrialio-buffer-dma.c | 331 +++++++++++---
.../buffer/industrialio-buffer-dmaengine.c | 77 +++-
drivers/iio/industrialio-buffer.c | 402 ++++++++++++++++++
include/linux/dmaengine.h | 16 +
include/linux/iio/buffer-dma.h | 40 +-
include/linux/iio/buffer-dmaengine.h | 5 +-
include/linux/iio/buffer_impl.h | 22 +
include/uapi/linux/iio/buffer.h | 22 +
12 files changed, 939 insertions(+), 81 deletions(-)
create mode 100644 Documentation/iio/dmabuf_api.rst
--
2.39.2
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.
Signed-off-by: Paul Cercueil <[email protected]>
---
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
Add implementation of the .device_prep_slave_dma_array() callback.
Signed-off-by: Paul Cercueil <[email protected]>
---
v3: New patch
---
drivers/dma/dma-axi-dmac.c | 41 ++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index a812b9b00e6b..61a796aca631 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -536,6 +536,46 @@ static struct axi_dmac_sg *axi_dmac_fill_linear_sg(struct axi_dmac_chan *chan,
return sg;
}
+static struct dma_async_tx_descriptor *
+axi_dmac_prep_slave_dma_array(struct dma_chan *c, dma_addr_t *addrs,
+ size_t *lengths, size_t nb,
+ enum dma_transfer_direction direction,
+ unsigned long flags)
+{
+ struct axi_dmac_chan *chan = to_axi_dmac_chan(c);
+ struct axi_dmac_desc *desc;
+ unsigned int num_sgs = 0;
+ struct axi_dmac_sg *dsg;
+ size_t i;
+
+ if (direction != chan->direction)
+ return NULL;
+
+ for (i = 0; i < nb; i++)
+ num_sgs += DIV_ROUND_UP(lengths[i], chan->max_length);
+
+ desc = axi_dmac_alloc_desc(num_sgs);
+ if (!desc)
+ return NULL;
+
+ dsg = desc->sg;
+
+ for (i = 0; i < nb; i++) {
+ if (!axi_dmac_check_addr(chan, addrs[i]) ||
+ !axi_dmac_check_len(chan, lengths[i])) {
+ kfree(desc);
+ return NULL;
+ }
+
+ dsg = axi_dmac_fill_linear_sg(chan, direction, addrs[i], 1,
+ lengths[i], dsg);
+ }
+
+ desc->cyclic = false;
+
+ return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
+}
+
static struct dma_async_tx_descriptor *axi_dmac_prep_slave_sg(
struct dma_chan *c, struct scatterlist *sgl,
unsigned int sg_len, enum dma_transfer_direction direction,
@@ -958,6 +998,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
dma_dev->device_tx_status = dma_cookie_status;
dma_dev->device_issue_pending = axi_dmac_issue_pending;
dma_dev->device_prep_slave_sg = axi_dmac_prep_slave_sg;
+ dma_dev->device_prep_slave_dma_array = axi_dmac_prep_slave_dma_array;
dma_dev->device_prep_dma_cyclic = axi_dmac_prep_dma_cyclic;
dma_dev->device_prep_interleaved_dma = axi_dmac_prep_interleaved;
dma_dev->device_terminate_all = axi_dmac_terminate_all;
--
2.39.2
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 the outgoing queue anyway,
getting rid of it now makes the upcoming changes simpler.
With this change, the IIO_BLOCK_STATE_DEQUEUED is now useless, and can
be removed.
Signed-off-by: Paul Cercueil <[email protected]>
---
v2: - Only remove the outgoing queue, and keep the incoming queue, as we
want the buffer to start streaming data as soon as it is enabled.
- Remove IIO_BLOCK_STATE_DEQUEUED, since it is now functionally the
same as IIO_BLOCK_STATE_DONE.
---
drivers/iio/buffer/industrialio-buffer-dma.c | 44 ++++++++++----------
include/linux/iio/buffer-dma.h | 7 ++--
2 files changed, 26 insertions(+), 25 deletions(-)
diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index d348af8b9705..1fc91467d1aa 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -179,7 +179,7 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(
}
block->size = size;
- block->state = IIO_BLOCK_STATE_DEQUEUED;
+ block->state = IIO_BLOCK_STATE_DONE;
block->queue = queue;
INIT_LIST_HEAD(&block->head);
kref_init(&block->kref);
@@ -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);
- }
}
/**
@@ -261,7 +253,6 @@ static bool iio_dma_block_reusable(struct iio_dma_buffer_block *block)
* not support abort and has not given back the block yet.
*/
switch (block->state) {
- case IIO_BLOCK_STATE_DEQUEUED:
case IIO_BLOCK_STATE_QUEUED:
case IIO_BLOCK_STATE_DONE:
return true;
@@ -317,7 +308,6 @@ 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);
@@ -456,14 +446,20 @@ 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);
- block->state = IIO_BLOCK_STATE_DEQUEUED;
+
+ idx = queue->fileio.next_dequeue;
+ block = queue->fileio.blocks[idx];
+
+ if (block->state == IIO_BLOCK_STATE_DONE) {
+ 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 +535,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 +549,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);
@@ -617,7 +621,6 @@ int iio_dma_buffer_init(struct iio_dma_buffer_queue *queue,
queue->ops = ops;
INIT_LIST_HEAD(&queue->incoming);
- INIT_LIST_HEAD(&queue->outgoing);
mutex_init(&queue->lock);
spin_lock_init(&queue->list_lock);
@@ -645,7 +648,6 @@ 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);
diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
index 6564bdcdac66..18d3702fa95d 100644
--- a/include/linux/iio/buffer-dma.h
+++ b/include/linux/iio/buffer-dma.h
@@ -19,14 +19,12 @@ struct device;
/**
* enum iio_block_state - State of a struct iio_dma_buffer_block
- * @IIO_BLOCK_STATE_DEQUEUED: Block is not queued
* @IIO_BLOCK_STATE_QUEUED: Block is on the incoming queue
* @IIO_BLOCK_STATE_ACTIVE: Block is currently being processed by the DMA
* @IIO_BLOCK_STATE_DONE: Block is on the outgoing queue
* @IIO_BLOCK_STATE_DEAD: Block has been marked as to be freed
*/
enum iio_block_state {
- IIO_BLOCK_STATE_DEQUEUED,
IIO_BLOCK_STATE_QUEUED,
IIO_BLOCK_STATE_ACTIVE,
IIO_BLOCK_STATE_DONE,
@@ -73,12 +71,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;
};
/**
@@ -93,7 +94,6 @@ struct iio_dma_buffer_queue_fileio {
* 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
*/
@@ -105,7 +105,6 @@ struct iio_dma_buffer_queue {
struct mutex lock;
spinlock_t list_lock;
struct list_head incoming;
- struct list_head outgoing;
bool active;
--
2.39.2
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).
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".
---
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);
+}
+
#endif
--
2.39.2
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 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;
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.39.2
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]>
---
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,
.release = iio_dmaengine_buffer_release,
.modes = INDIO_BUFFER_HARDWARE,
--
2.39.2
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 <[email protected]>
---
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);
+}
+
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)
{
INIT_LIST_HEAD(&buffer->demux_list);
INIT_LIST_HEAD(&buffer->buffer_list);
+ INIT_LIST_HEAD(&buffer->dmabufs);
init_waitqueue_head(&buffer->pollq);
kref_init(&buffer->ref);
if (!buffer->watermark)
@@ -1500,14 +1535,55 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
}
+static void iio_buffer_dmabuf_release(struct kref *ref)
+{
+ struct iio_dmabuf_priv *priv = container_of(ref, struct iio_dmabuf_priv, ref);
+ struct dma_buf_attachment *attach = priv->attach;
+ struct iio_buffer *buffer = priv->buffer;
+ struct dma_buf *dmabuf = attach->dmabuf;
+
+ buffer->access->detach_dmabuf(buffer, priv->block);
+
+ dma_buf_detach(attach->dmabuf, attach);
+ dma_buf_put(dmabuf);
+ kfree(priv);
+}
+
+void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach)
+{
+ struct iio_dmabuf_priv *priv = attach->importer_priv;
+
+ kref_get(&priv->ref);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_get);
+
+void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach)
+{
+ struct iio_dmabuf_priv *priv = attach->importer_priv;
+
+ kref_put(&priv->ref, iio_buffer_dmabuf_release);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_put);
+
static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
{
struct iio_dev_buffer_pair *ib = filep->private_data;
struct iio_dev *indio_dev = ib->indio_dev;
struct iio_buffer *buffer = ib->buffer;
+ struct iio_dmabuf_priv *priv, *tmp;
wake_up(&buffer->pollq);
+ /* 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? */
+ 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;
}
+int iio_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
+{
+ int ret;
+
+ ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
+ if (ret) {
+ if (ret != -EDEADLK)
+ goto out;
+ if (nonblock) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ ret = dma_resv_lock_slow_interruptible(dmabuf->resv, NULL);
+ }
+
+out:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(iio_dma_resv_lock);
+
+static struct dma_buf_attachment *
+iio_buffer_find_attachment(struct iio_dev *indio_dev, struct dma_buf *dmabuf)
+{
+ struct dma_buf_attachment *elm, *attach = NULL;
+ int ret;
+
+ ret = iio_dma_resv_lock(dmabuf, false);
+ if (ret)
+ return ERR_PTR(ret);
+
+ list_for_each_entry(elm, &dmabuf->attachments, node) {
+ if (elm->dev == indio_dev->dev.parent) {
+ attach = elm;
+ break;
+ }
+ }
+
+ if (attach)
+ iio_buffer_dmabuf_get(elm);
+
+ dma_resv_unlock(dmabuf->resv);
+
+ return attach ?: ERR_PTR(-EPERM);
+}
+
+static int iio_buffer_attach_dmabuf(struct iio_dev_buffer_pair *ib,
+ int __user *user_fd)
+{
+ struct iio_dev *indio_dev = ib->indio_dev;
+ struct iio_buffer *buffer = ib->buffer;
+ struct dma_buf_attachment *attach;
+ struct iio_dmabuf_priv *priv;
+ struct dma_buf *dmabuf;
+ int err, fd;
+
+ if (!buffer->access->attach_dmabuf
+ || !buffer->access->detach_dmabuf
+ || !buffer->access->enqueue_dmabuf)
+ return -EPERM;
+
+ if (copy_from_user(&fd, user_fd, sizeof(fd)))
+ return -EFAULT;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ spin_lock_init(&priv->lock);
+ priv->context = dma_fence_context_alloc(1);
+
+ dmabuf = dma_buf_get(fd);
+ if (IS_ERR(dmabuf)) {
+ err = PTR_ERR(dmabuf);
+ goto err_free_priv;
+ }
+
+ attach = dma_buf_attach(dmabuf, indio_dev->dev.parent);
+ if (IS_ERR(attach)) {
+ err = PTR_ERR(attach);
+ goto err_dmabuf_put;
+ }
+
+ kref_init(&priv->ref);
+ priv->buffer = buffer;
+ priv->attach = attach;
+ attach->importer_priv = priv;
+
+ priv->block = buffer->access->attach_dmabuf(buffer, attach);
+ if (IS_ERR(priv->block)) {
+ err = PTR_ERR(priv->block);
+ goto err_dmabuf_detach;
+ }
+
+ list_add(&priv->entry, &buffer->dmabufs);
+
+ return 0;
+
+err_dmabuf_detach:
+ dma_buf_detach(dmabuf, attach);
+err_dmabuf_put:
+ dma_buf_put(dmabuf);
+err_free_priv:
+ kfree(priv);
+
+ return err;
+}
+
+static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair *ib, int *user_req)
+{
+ struct dma_buf_attachment *attach;
+ struct iio_dmabuf_priv *priv;
+ struct dma_buf *dmabuf;
+ int dmabuf_fd, ret = 0;
+
+ if (copy_from_user(&dmabuf_fd, user_req, sizeof(dmabuf_fd)))
+ return -EFAULT;
+
+ dmabuf = dma_buf_get(dmabuf_fd);
+ if (IS_ERR(dmabuf))
+ return PTR_ERR(dmabuf);
+
+ attach = iio_buffer_find_attachment(ib->indio_dev, dmabuf);
+ if (IS_ERR(attach)) {
+ ret = PTR_ERR(attach);
+ goto out_dmabuf_put;
+ }
+
+ priv = attach->importer_priv;
+ list_del_init(&priv->entry);
+
+ iio_buffer_dmabuf_put(attach);
+ iio_buffer_dmabuf_put(attach);
+
+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);
+ goto err_attachment_put;
+ }
+
+ fence = kmalloc(sizeof(*fence), GFP_KERNEL);
+ if (!fence) {
+ ret = -ENOMEM;
+ goto err_unmap_attachment;
+ }
+
+ fence->priv = priv;
+ fence->sgt = sgt;
+ fence->dir = dir;
+ priv->fence = fence;
+
+ dma_fence_init(&fence->base, &iio_buffer_dma_fence_ops,
+ &priv->lock, priv->context, 0);
+
+ ret = iio_dma_resv_lock(dmabuf, nonblock);
+ if (ret)
+ goto err_fence_put;
+
+ timeout = nonblock ? 0 : msecs_to_jiffies(DMABUF_ENQUEUE_TIMEOUT_MS);
+
+ /* Make sure we don't have writers */
+ ret = (int) dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_WRITE,
+ true, timeout);
+ if (ret == 0)
+ ret = -EBUSY;
+ if (ret < 0)
+ goto err_resv_unlock;
+
+ if (dma_to_ram) {
+ /*
+ * If we're writing to the DMABUF, make sure we don't have
+ * readers
+ */
+ ret = (int) dma_resv_wait_timeout(dmabuf->resv,
+ DMA_RESV_USAGE_READ, true,
+ timeout);
+ if (ret == 0)
+ ret = -EBUSY;
+ if (ret < 0)
+ goto err_resv_unlock;
+ }
+
+ ret = dma_resv_reserve_fences(dmabuf->resv, 1);
+ if (ret)
+ goto err_resv_unlock;
+
+ dma_resv_add_fence(dmabuf->resv, &fence->base,
+ dma_resv_usage_rw(dma_to_ram));
+ dma_resv_unlock(dmabuf->resv);
+
+ ret = buffer->access->enqueue_dmabuf(buffer, priv->block, sgt,
+ iio_dmabuf.bytes_used, cyclic);
+ if (ret)
+ 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);
+
+static long iio_buffer_chrdev_ioctl(struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ struct iio_dev_buffer_pair *ib = filp->private_data;
+ void __user *_arg = (void __user *)arg;
+
+ switch (cmd) {
+ case IIO_BUFFER_DMABUF_ATTACH_IOCTL:
+ return iio_buffer_attach_dmabuf(ib, _arg);
+ case IIO_BUFFER_DMABUF_DETACH_IOCTL:
+ return iio_buffer_detach_dmabuf(ib, _arg);
+ case IIO_BUFFER_DMABUF_ENQUEUE_IOCTL:
+ return iio_buffer_enqueue_dmabuf(ib, _arg,
+ filp->f_flags & O_NONBLOCK);
+ default:
+ return IIO_IOCTL_UNHANDLED;
+ }
+}
+
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,
};
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.
+ * @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;
};
@@ -136,6 +150,9 @@ struct iio_buffer {
/* @ref: Reference count of the buffer. */
struct kref ref;
+
+ /* @dmabufs: List of DMABUF attachments */
+ struct list_head dmabufs;
};
/**
@@ -156,9 +173,14 @@ int iio_update_buffers(struct iio_dev *indio_dev,
**/
void iio_buffer_init(struct iio_buffer *buffer);
+void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach);
+void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach);
+
struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer);
void iio_buffer_put(struct iio_buffer *buffer);
+void iio_buffer_signal_dmabuf_done(struct dma_buf_attachment *attach, int ret);
+
#else /* CONFIG_IIO_BUFFER */
static inline void iio_buffer_get(struct iio_buffer *buffer) {}
diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
index 13939032b3f6..c666aa95e532 100644
--- a/include/uapi/linux/iio/buffer.h
+++ b/include/uapi/linux/iio/buffer.h
@@ -5,6 +5,28 @@
#ifndef _UAPI_IIO_BUFFER_H_
#define _UAPI_IIO_BUFFER_H_
+#include <linux/types.h>
+
+/* Flags for iio_dmabuf.flags */
+#define IIO_BUFFER_DMABUF_CYCLIC (1 << 0)
+#define IIO_BUFFER_DMABUF_SUPPORTED_FLAGS 0x00000001
+
+/**
+ * struct iio_dmabuf - Descriptor for a single IIO DMABUF object
+ * @fd: file descriptor of the DMABUF object
+ * @flags: one or more IIO_BUFFER_DMABUF_* flags
+ * @bytes_used: number of bytes used in this DMABUF for the data transfer.
+ * Should generally be set to the DMABUF's size.
+ */
+struct iio_dmabuf {
+ __u32 fd;
+ __u32 flags;
+ __u64 bytes_used;
+};
+
#define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int)
+#define IIO_BUFFER_DMABUF_ATTACH_IOCTL _IOW('i', 0x92, int)
+#define IIO_BUFFER_DMABUF_DETACH_IOCTL _IOW('i', 0x93, int)
+#define IIO_BUFFER_DMABUF_ENQUEUE_IOCTL _IOW('i', 0x94, struct iio_dmabuf)
#endif /* _UAPI_IIO_BUFFER_H_ */
--
2.39.2
From: Alexandru Ardelean <[email protected]>
This change splits the logic into a separate function, which will be
re-used later.
Signed-off-by: Alexandru Ardelean <[email protected]>
Cc: Alexandru Ardelean <[email protected]>
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/iio/buffer/industrialio-buffer-dma.c | 43 +++++++++++---------
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index 86eced458236..e14814e0d4c8 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -374,6 +374,29 @@ 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);
+
+ INIT_LIST_HEAD(&queue->incoming);
+
+ 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)
{
@@ -695,27 +718,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);
-
- INIT_LIST_HEAD(&queue->incoming);
-
- 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.39.2
Implement iio_dma_buffer_attach_dmabuf(), iio_dma_buffer_detach_dmabuf()
and iio_dma_buffer_transfer_dmabuf(), which can then be used by the IIO
DMA buffer implementations.
Signed-off-by: Paul Cercueil <[email protected]>
---
v3: Update code to provide the functions that will be used as callbacks
for the new IOCTLs.
---
drivers/iio/buffer/industrialio-buffer-dma.c | 157 +++++++++++++++++--
include/linux/iio/buffer-dma.h | 24 +++
2 files changed, 168 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index e14814e0d4c8..422bd784fd1e 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -14,6 +14,7 @@
#include <linux/poll.h>
#include <linux/iio/buffer_impl.h>
#include <linux/iio/buffer-dma.h>
+#include <linux/dma-buf.h>
#include <linux/dma-mapping.h>
#include <linux/sizes.h>
@@ -94,14 +95,24 @@ static void iio_buffer_block_release(struct kref *kref)
{
struct iio_dma_buffer_block *block = container_of(kref,
struct iio_dma_buffer_block, kref);
+ struct iio_dma_buffer_queue *queue = block->queue;
- WARN_ON(block->state != IIO_BLOCK_STATE_DEAD);
+ WARN_ON(block->fileio && block->state != IIO_BLOCK_STATE_DEAD);
- dma_free_coherent(block->queue->dev, PAGE_ALIGN(block->size),
- block->vaddr, block->phys_addr);
+ mutex_lock(&queue->lock);
- iio_buffer_put(&block->queue->buffer);
+ if (block->fileio) {
+ dma_free_coherent(queue->dev, PAGE_ALIGN(block->size),
+ block->vaddr, block->phys_addr);
+ queue->num_fileio_blocks--;
+ }
+
+ queue->num_blocks--;
kfree(block);
+
+ mutex_unlock(&queue->lock);
+
+ iio_buffer_put(&queue->buffer);
}
static void iio_buffer_block_get(struct iio_dma_buffer_block *block)
@@ -163,7 +174,7 @@ static struct iio_dma_buffer_queue *iio_buffer_to_queue(struct iio_buffer *buf)
}
static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(
- struct iio_dma_buffer_queue *queue, size_t size)
+ struct iio_dma_buffer_queue *queue, size_t size, bool fileio)
{
struct iio_dma_buffer_block *block;
@@ -171,13 +182,16 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(
if (!block)
return NULL;
- block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size),
- &block->phys_addr, GFP_KERNEL);
- if (!block->vaddr) {
- kfree(block);
- return NULL;
+ if (fileio) {
+ block->vaddr = dma_alloc_coherent(queue->dev, PAGE_ALIGN(size),
+ &block->phys_addr, GFP_KERNEL);
+ if (!block->vaddr) {
+ kfree(block);
+ return NULL;
+ }
}
+ block->fileio = fileio;
block->size = size;
block->state = IIO_BLOCK_STATE_DONE;
block->queue = queue;
@@ -186,6 +200,9 @@ static struct iio_dma_buffer_block *iio_dma_buffer_alloc_block(
iio_buffer_get(&queue->buffer);
+ queue->num_blocks++;
+ queue->num_fileio_blocks += fileio;
+
return block;
}
@@ -223,6 +240,9 @@ void iio_dma_buffer_block_done(struct iio_dma_buffer_block *block)
_iio_dma_buffer_block_done(block);
spin_unlock_irqrestore(&queue->list_lock, flags);
+ if (!block->fileio)
+ iio_buffer_signal_dmabuf_done(block->attach, 0);
+
iio_buffer_block_put_atomic(block);
iio_dma_buffer_queue_wake(queue);
}
@@ -249,10 +269,14 @@ void iio_dma_buffer_block_list_abort(struct iio_dma_buffer_queue *queue,
list_del(&block->head);
block->bytes_used = 0;
_iio_dma_buffer_block_done(block);
+
+ if (!block->fileio)
+ iio_buffer_signal_dmabuf_done(block->attach, -EINTR);
iio_buffer_block_put_atomic(block);
}
spin_unlock_irqrestore(&queue->list_lock, flags);
+ queue->fileio.enabled = false;
iio_dma_buffer_queue_wake(queue);
}
EXPORT_SYMBOL_GPL(iio_dma_buffer_block_list_abort);
@@ -273,6 +297,12 @@ static bool iio_dma_block_reusable(struct iio_dma_buffer_block *block)
}
}
+static bool iio_dma_buffer_fileio_mode(struct iio_dma_buffer_queue *queue)
+{
+ return queue->fileio.enabled ||
+ queue->num_blocks == queue->num_fileio_blocks;
+}
+
/**
* iio_dma_buffer_request_update() - DMA buffer request_update callback
* @buffer: The buffer which to request an update
@@ -299,6 +329,12 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer)
mutex_lock(&queue->lock);
+ queue->fileio.enabled = iio_dma_buffer_fileio_mode(queue);
+
+ /* If DMABUFs were created, disable fileio interface */
+ if (!queue->fileio.enabled)
+ goto out_unlock;
+
/* Allocations are page aligned */
if (PAGE_ALIGN(queue->fileio.block_size) == PAGE_ALIGN(size))
try_reuse = true;
@@ -329,7 +365,7 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer)
block = queue->fileio.blocks[i];
if (block->state == IIO_BLOCK_STATE_DEAD) {
/* Could not reuse it */
- iio_buffer_block_put(block);
+ iio_buffer_block_put_atomic(block);
block = NULL;
} else {
block->size = size;
@@ -339,7 +375,7 @@ int iio_dma_buffer_request_update(struct iio_buffer *buffer)
}
if (!block) {
- block = iio_dma_buffer_alloc_block(queue, size);
+ block = iio_dma_buffer_alloc_block(queue, size, true);
if (!block) {
ret = -ENOMEM;
goto out_unlock;
@@ -391,7 +427,7 @@ static void iio_dma_buffer_fileio_free(struct iio_dma_buffer_queue *queue)
for (i = 0; i < ARRAY_SIZE(queue->fileio.blocks); i++) {
if (!queue->fileio.blocks[i])
continue;
- iio_buffer_block_put(queue->fileio.blocks[i]);
+ iio_buffer_block_put_atomic(queue->fileio.blocks[i]);
queue->fileio.blocks[i] = NULL;
}
queue->fileio.active_block = NULL;
@@ -412,8 +448,12 @@ static void iio_dma_buffer_submit_block(struct iio_dma_buffer_queue *queue,
block->state = IIO_BLOCK_STATE_ACTIVE;
iio_buffer_block_get(block);
+
ret = queue->ops->submit(queue, block);
if (ret) {
+ if (!block->fileio)
+ iio_buffer_signal_dmabuf_done(block->attach, ret);
+
/*
* This is a bit of a problem and there is not much we can do
* other then wait for the buffer to be disabled and re-enabled
@@ -645,6 +685,97 @@ size_t iio_dma_buffer_data_available(struct iio_buffer *buf)
}
EXPORT_SYMBOL_GPL(iio_dma_buffer_data_available);
+struct iio_dma_buffer_block *
+iio_dma_buffer_attach_dmabuf(struct iio_buffer *buffer,
+ struct dma_buf_attachment *attach)
+{
+ struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
+ struct iio_dma_buffer_block *block;
+ int err;
+
+ mutex_lock(&queue->lock);
+
+ /*
+ * If the buffer is enabled and in fileio mode new blocks can't be
+ * allocated.
+ */
+ if (queue->fileio.enabled) {
+ err = -EBUSY;
+ goto err_unlock;
+ }
+
+ block = iio_dma_buffer_alloc_block(queue, attach->dmabuf->size, false);
+ if (!block) {
+ err = -ENOMEM;
+ goto err_unlock;
+ }
+
+ block->attach = attach;
+
+ /* Free memory that might be in use for fileio mode */
+ iio_dma_buffer_fileio_free(queue);
+
+ mutex_unlock(&queue->lock);
+
+ return block;
+
+err_unlock:
+ mutex_unlock(&queue->lock);
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_attach_dmabuf);
+
+void iio_dma_buffer_detach_dmabuf(struct iio_buffer *buffer,
+ struct iio_dma_buffer_block *block)
+{
+ block->state = IIO_BLOCK_STATE_DEAD;
+ iio_buffer_block_put_atomic(block);
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_detach_dmabuf);
+
+static int iio_dma_can_enqueue_block(struct iio_dma_buffer_block *block)
+{
+ struct iio_dma_buffer_queue *queue = block->queue;
+
+ /* If in fileio mode buffers can't be enqueued. */
+ if (queue->fileio.enabled)
+ return -EBUSY;
+
+ switch (block->state) {
+ case IIO_BLOCK_STATE_QUEUED:
+ return -EPERM;
+ case IIO_BLOCK_STATE_DONE:
+ return 0;
+ default:
+ return -EBUSY;
+ }
+}
+
+int iio_dma_buffer_enqueue_dmabuf(struct iio_buffer *buffer,
+ struct iio_dma_buffer_block *block,
+ struct sg_table *sgt,
+ size_t size, bool cyclic)
+{
+ struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
+ int ret = 0;
+
+ mutex_lock(&queue->lock);
+ ret = iio_dma_can_enqueue_block(block);
+ if (ret < 0)
+ goto out_mutex_unlock;
+
+ block->bytes_used = size;
+ block->cyclic = cyclic;
+ block->sg_table = sgt;
+
+ iio_dma_buffer_enqueue(queue, block);
+
+out_mutex_unlock:
+ mutex_unlock(&queue->lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(iio_dma_buffer_enqueue_dmabuf);
+
/**
* iio_dma_buffer_set_bytes_per_datum() - DMA buffer set_bytes_per_datum callback
* @buffer: Buffer to set the bytes-per-datum for
diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
index 490b93f76fa8..e5e5817e99db 100644
--- a/include/linux/iio/buffer-dma.h
+++ b/include/linux/iio/buffer-dma.h
@@ -16,6 +16,8 @@
struct iio_dma_buffer_queue;
struct iio_dma_buffer_ops;
struct device;
+struct dma_buf_attachment;
+struct sg_table;
/**
* enum iio_block_state - State of a struct iio_dma_buffer_block
@@ -41,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
+ * @fileio: True if this buffer is used for fileio mode
*/
struct iio_dma_buffer_block {
/* May only be accessed by the owner of the block */
@@ -63,6 +66,11 @@ struct iio_dma_buffer_block {
* queue->list_lock if the block is not owned by the core.
*/
enum iio_block_state state;
+
+ bool fileio;
+
+ struct dma_buf_attachment *attach;
+ struct sg_table *sg_table;
};
/**
@@ -72,6 +80,7 @@ struct iio_dma_buffer_block {
* @pos: Read offset in the active block
* @block_size: Size of each block
* @next_dequeue: index of next block that will be dequeued
+ * @enabled: Whether the buffer is operating in fileio mode
*/
struct iio_dma_buffer_queue_fileio {
struct iio_dma_buffer_block *blocks[2];
@@ -80,6 +89,7 @@ struct iio_dma_buffer_queue_fileio {
size_t block_size;
unsigned int next_dequeue;
+ bool enabled;
};
/**
@@ -95,6 +105,8 @@ struct iio_dma_buffer_queue_fileio {
* the DMA controller
* @incoming: List of buffers on the incoming queue
* @active: Whether the buffer is currently active
+ * @num_blocks: Total number of DMA blocks
+ * @num_fileio_blocks: Number of DMA blocks for fileio mode
* @fileio: FileIO state
*/
struct iio_dma_buffer_queue {
@@ -107,6 +119,8 @@ struct iio_dma_buffer_queue {
struct list_head incoming;
bool active;
+ unsigned int num_blocks;
+ unsigned int num_fileio_blocks;
struct iio_dma_buffer_queue_fileio fileio;
};
@@ -149,4 +163,14 @@ static inline size_t iio_dma_buffer_space_available(struct iio_buffer *buffer)
return iio_dma_buffer_data_available(buffer);
}
+struct iio_dma_buffer_block *
+iio_dma_buffer_attach_dmabuf(struct iio_buffer *buffer,
+ struct dma_buf_attachment *attach);
+void iio_dma_buffer_detach_dmabuf(struct iio_buffer *buffer,
+ struct iio_dma_buffer_block *block);
+int iio_dma_buffer_enqueue_dmabuf(struct iio_buffer *buffer,
+ struct iio_dma_buffer_block *block,
+ struct sg_table *sgt,
+ size_t size, bool cyclic);
+
#endif
--
2.39.2
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 <[email protected]>
---
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;
+ 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);
+ 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);
+ }
if (!desc)
return -ENOMEM;
@@ -133,6 +176,10 @@ static const struct iio_buffer_access_funcs iio_dmaengine_buffer_ops = {
.space_available = iio_dma_buffer_space_available,
.release = iio_dmaengine_buffer_release,
+ .enqueue_dmabuf = iio_dma_buffer_enqueue_dmabuf,
+ .attach_dmabuf = iio_dma_buffer_attach_dmabuf,
+ .detach_dmabuf = iio_dma_buffer_detach_dmabuf,
+
.modes = INDIO_BUFFER_HARDWARE,
.flags = INDIO_BUFFER_FLAG_FIXED_WATERMARK,
};
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
*/
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;
--
2.39.2
Document the new DMABUF based API.
Signed-off-by: Paul Cercueil <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: [email protected]
---
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.
+
+``IIO_BUFFER_DMABUF_DETACH_IOCTL(int)``
+--------------------------------------------------------
+
+Detach the given DMABUF object, identified by its file descriptor, from
+the IIO buffer. Returns zero on success, and a negative errno value on
+error.
+
+Note that closing the IIO buffer's file descriptor will automatically
+detach all previously attached DMABUF objects.
+
+``IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *iio_dmabuf)``
+--------------------------------------------------------
+
+Enqueue a previously attached DMABUF object to the buffer queue.
+Enqueued DMABUFs will be read from (if output buffer) or written to
+(if input buffer) as long as the buffer is enabled.
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 1b7292c58cd0..3eae8fcb1938 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -9,6 +9,8 @@ Industrial I/O
iio_configfs
+ dmabuf_api
+
ep93xx_adc
bno055
--
2.39.2
Paul Cercueil <[email protected]> writes:
One nit:
> Document the new DMABUF based API.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: [email protected]
>
> ---
> 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
Hi Jonathan,
Le lundi 03 avril 2023 à 10:05 -0600, Jonathan Corbet a écrit :
> Paul Cercueil <[email protected]> writes:
>
> One nit:
>
> > Document the new DMABUF based API.
> >
> > Signed-off-by: Paul Cercueil <[email protected]>
> > Cc: Jonathan Corbet <[email protected]>
> > Cc: [email protected]
> >
> > ---
> > 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.
Noted, thanks.
Cheers,
-Paul
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 <[email protected]>
>
> ---
> 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 Mon, 2023-04-03 at 17:47 +0200, Paul Cercueil wrote:
> Hi Jonathan,
>
> Here's the v3 of my patchset that introduces a new interface based on
> DMABUF objects to complement the fileio API, and adds write() support to
> the existing fileio API.
>
> It changed quite a lot since V2; the IIO subsystem is now just a DMABUF
> importer, and all the complexity related to handling creation, deletion
> and export of DMABUFs (including DMA mapping etc.) is gone.
>
> This new interface will be used by Libiio. The code is ready[1] and will
> be merged to the main branch as soon as the kernel bits are accepted
> upstream.
>
> Note that Libiio (and its server counterpart, iiod) use this new
> interface in two different ways:
> - by memory-mapping the DMABUFs to access the sample data directly,
> which is much faster than using the existing fileio API as the sample
> data does not need to be copied;
> - by passing the DMABUFs around directly to the USB stack, in a
> device-to-device zero-copy fashion, using a new DMABUF interface for
> the USB (FunctionFS to be exact) stack, which is being upstreamed in
> parallel of this patchset [2].
>
> 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 I still
> benefit from sending this patchset early to get feedback.
>
Indeed, I already started a discussion [1] since what we have now for
adc/adi-axi-adc.c has some major flaws (IMHO). So I'm hopping to get some
feedback/discussion to get "righter" from the beginning.
[1]: https://lore.kernel.org/linux-iio/[email protected]/T/#u
- Nuno Sá
Hi Hillf,
Le mardi 04 avril 2023 à 09:59 +0800, Hillf Danton a écrit :
> On 3 Apr 2023 17:47:50 +0200 Paul Cercueil <[email protected]>
> > 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 <[email protected]>
> >
> > ---
> > 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);
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 <[email protected]>
> >
> > ---
> > 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.
>
> > +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
Should work with (&ib->indio_dev->dev), yes.
>
> > + 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.
According to Jonathan the old chardev route is deprecated, and it's
fine not to support the IOCTL there.
Cheers,
-Paul
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 <[email protected]>
> > >
> > > ---
> > > 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á
Am 04.04.23 um 09:42 schrieb Paul Cercueil:
> Hi Hillf,
>
> Le mardi 04 avril 2023 à 09:59 +0800, Hillf Danton a écrit :
>> On 3 Apr 2023 17:47:50 +0200 Paul Cercueil <[email protected]>
>>> 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 ;)
Yeah adding that is pretty much the job I have on my TODO list for quite
some time.
I wouldn't mind if you start adding that and provide helper functions in
DMA-buf to convert from/to an sg_table.
This way we can migrate the interface over to a new design over time.
Regards,
Christian.
>
> Thanks for the feedback.
>
> Cheers,
> -Paul
>
>>> Signed-off-by: Paul Cercueil <[email protected]>
>>>
>>> ---
>>> 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 4/4/23 00:55, Paul Cercueil wrote:
> [...]
>>> + 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.
Let's add a comment documenting that.
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 <[email protected]>
>
> ---
> 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
--
~Vinod
Hi Vinod,
Le mercredi 12 avril 2023 à 22:53 +0530, Vinod Koul a écrit :
> 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?
I admit that I discarded the interleaved DMA without trying it, because
reading the doc, e.g. the one for "struct data_chunk", It looked like
it was not usable for when the DMA addresses are scattered in memory;
it assumes that the following DMA addresses will always come after the
previous one.
Cheers,
-Paul
> >
> > Signed-off-by: Paul Cercueil <[email protected]>
> >
> > ---
> > 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:52 +0200
Paul Cercueil <[email protected]> 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 the outgoing queue anyway,
> getting rid of it now makes the upcoming changes simpler.
>
> With this change, the IIO_BLOCK_STATE_DEQUEUED is now useless, and can
> be removed.
>
> Signed-off-by: Paul Cercueil <[email protected]>
>
> ---
> v2: - Only remove the outgoing queue, and keep the incoming queue, as we
> want the buffer to start streaming data as soon as it is enabled.
> - Remove IIO_BLOCK_STATE_DEQUEUED, since it is now functionally the
> same as IIO_BLOCK_STATE_DONE.
I'm not that familiar with this code, but with my understanding this makes
sense. I think it is independent of the earlier patches and is a useful
change in it's own right. As such, does it make sense to pick this up
ahead of the rest of the series? I'm assuming that discussion on the
rest will take a while. No great rush as too late for the coming merge
window anyway.
Thanks,
Jonathan
On Mon, 3 Apr 2023 17:47:53 +0200
Paul Cercueil <[email protected]> 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 <[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".
> ---
> 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:55 +0200
Paul Cercueil <[email protected]> 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 <[email protected]>
> Reviewed-by: Alexandru Ardelean <[email protected]>
> ---
> 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, 3 Apr 2023 17:47:54 +0200
Paul Cercueil <[email protected]> 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 <[email protected]>
> Reviewed-by: Alexandru Ardelean <[email protected]>
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:56 +0200
Paul Cercueil <[email protected]> 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 <[email protected]>
>
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;
> };
On Mon, 3 Apr 2023 17:47:58 +0200
Paul Cercueil <[email protected]> wrote:
> Implement iio_dma_buffer_attach_dmabuf(), iio_dma_buffer_detach_dmabuf()
> and iio_dma_buffer_transfer_dmabuf(), which can then be used by the IIO
> DMA buffer implementations.
>
> Signed-off-by: Paul Cercueil <[email protected]>
Hi Paul,
A few superficially comments.
Jonathan
>
> ---
> v3: Update code to provide the functions that will be used as callbacks
> for the new IOCTLs.
> ---
> drivers/iio/buffer/industrialio-buffer-dma.c | 157 +++++++++++++++++--
> include/linux/iio/buffer-dma.h | 24 +++
> 2 files changed, 168 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
> index e14814e0d4c8..422bd784fd1e 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
...
> @@ -412,8 +448,12 @@ static void iio_dma_buffer_submit_block(struct iio_dma_buffer_queue *queue,
>
> block->state = IIO_BLOCK_STATE_ACTIVE;
> iio_buffer_block_get(block);
> +
Trivial, but I'd rather not see unrelated white space changes in a patch
doing anything else.
> ret = queue->ops->submit(queue, block);
> if (ret) {
> + if (!block->fileio)
> + iio_buffer_signal_dmabuf_done(block->attach, ret);
> +
> /*
> * This is a bit of a problem and there is not much we can do
> * other then wait for the buffer to be disabled and re-enabled
> @@ -645,6 +685,97 @@ size_t iio_dma_buffer_data_available(struct iio_buffer *buf)
> }
> EXPORT_SYMBOL_GPL(iio_dma_buffer_data_available);
...
> +int iio_dma_buffer_enqueue_dmabuf(struct iio_buffer *buffer,
> + struct iio_dma_buffer_block *block,
> + struct sg_table *sgt,
> + size_t size, bool cyclic)
> +{
> + struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
> + int ret = 0;
No need to init.
> +
> + mutex_lock(&queue->lock);
> + ret = iio_dma_can_enqueue_block(block);
> + if (ret < 0)
> + goto out_mutex_unlock;
> +
> + block->bytes_used = size;
> + block->cyclic = cyclic;
> + block->sg_table = sgt;
> +
> + iio_dma_buffer_enqueue(queue, block);
> +
> +out_mutex_unlock:
> + mutex_unlock(&queue->lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_dma_buffer_enqueue_dmabuf);
Obviously an unrelated activity but good to namespace these
in a future patch set.
> +
> /**
> * iio_dma_buffer_set_bytes_per_datum() - DMA buffer set_bytes_per_datum callback
> * @buffer: Buffer to set the bytes-per-datum for
> diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
> index 490b93f76fa8..e5e5817e99db 100644
> --- a/include/linux/iio/buffer-dma.h
> +++ b/include/linux/iio/buffer-dma.h
> /**
> * enum iio_block_state - State of a struct iio_dma_buffer_block
> @@ -41,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
> + * @fileio: True if this buffer is used for fileio mode
Docs need update for the other two new elements.
> */
> struct iio_dma_buffer_block {
> /* May only be accessed by the owner of the block */
> @@ -63,6 +66,11 @@ struct iio_dma_buffer_block {
> * queue->list_lock if the block is not owned by the core.
> */
> enum iio_block_state state;
> +
> + bool fileio;
> +
> + struct dma_buf_attachment *attach;
> + struct sg_table *sg_table;
> };
On Mon, 3 Apr 2023 17:49:54 +0200
Paul Cercueil <[email protected]> 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 <[email protected]>
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;
Hi Jonathan,
Le dimanche 16 avril 2023 à 15:24 +0100, Jonathan Cameron a écrit :
> On Mon, 3 Apr 2023 17:47:52 +0200
> Paul Cercueil <[email protected]> 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 the outgoing queue
> > anyway,
> > getting rid of it now makes the upcoming changes simpler.
> >
> > With this change, the IIO_BLOCK_STATE_DEQUEUED is now useless, and
> > can
> > be removed.
> >
> > Signed-off-by: Paul Cercueil <[email protected]>
> >
> > ---
> > v2: - Only remove the outgoing queue, and keep the incoming queue,
> > as we
> > want the buffer to start streaming data as soon as it is
> > enabled.
> > - Remove IIO_BLOCK_STATE_DEQUEUED, since it is now functionally
> > the
> > same as IIO_BLOCK_STATE_DONE.
>
> I'm not that familiar with this code, but with my understanding this
> makes
> sense. I think it is independent of the earlier patches and is a
> useful
> change in it's own right. As such, does it make sense to pick this
> up
> ahead of the rest of the series? I'm assuming that discussion on the
> rest will take a while. No great rush as too late for the coming
> merge
> window anyway.
Actually, you can pick patches 3 to 6 (when all have been acked). They
add write support for buffer-dma implementations; which is a dependency
for the rest of the patchset, but they can live on their own.
Cheers,
-Paul
On Tue, 18 Apr 2023 10:08:21 +0200
Paul Cercueil <[email protected]> wrote:
> Hi Jonathan,
>
> Le dimanche 16 avril 2023 à 15:24 +0100, Jonathan Cameron a écrit :
> > On Mon, 3 Apr 2023 17:47:52 +0200
> > Paul Cercueil <[email protected]> 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 the outgoing queue
> > > anyway,
> > > getting rid of it now makes the upcoming changes simpler.
> > >
> > > With this change, the IIO_BLOCK_STATE_DEQUEUED is now useless, and
> > > can
> > > be removed.
> > >
> > > Signed-off-by: Paul Cercueil <[email protected]>
> > >
> > > ---
> > > v2: - Only remove the outgoing queue, and keep the incoming queue,
> > > as we
> > > want the buffer to start streaming data as soon as it is
> > > enabled.
> > > - Remove IIO_BLOCK_STATE_DEQUEUED, since it is now functionally
> > > the
> > > same as IIO_BLOCK_STATE_DONE.
> >
> > I'm not that familiar with this code, but with my understanding this
> > makes
> > sense. I think it is independent of the earlier patches and is a
> > useful
> > change in it's own right. As such, does it make sense to pick this
> > up
> > ahead of the rest of the series? I'm assuming that discussion on the
> > rest will take a while. No great rush as too late for the coming
> > merge
> > window anyway.
>
> Actually, you can pick patches 3 to 6 (when all have been acked). They
> add write support for buffer-dma implementations; which is a dependency
> for the rest of the patchset, but they can live on their own.
Remind me of that in the cover letter for v4.
Thanks,
Jonathan
>
> Cheers,
> -Paul