Re: [PATCH v10 3/6] iio: core: Add new DMABUF interface infrastructure
Hi Markus,
Le lundi 17 juin 2024 à 08:56 +0200, Markus Elfring a écrit :
> …
> > +++ b/drivers/iio/industrialio-buffer.c
> …
> > static int iio_buffer_chrdev_release(struct inode *inode, struct
> > file *filep)
> > {
> …
> > wake_up(&buffer->pollq);
> >
> > + mutex_lock(&buffer->dmabufs_mutex);
> > +
> > + /* Close all attached DMABUFs */
> …
> > + mutex_unlock(&buffer->dmabufs_mutex);
> > +
> > kfree(ib);
> …
>
> Would you become interested to apply a statement like
> “guard(mutex)(&buffer->dmabufs_mutex);”?
> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196
I'll update the patch to use it.
Cheers,
-Paul
Re: [PATCH v10 3/6] iio: core: Add new DMABUF interface infrastructure
…
> +++ b/drivers/iio/industrialio-buffer.c
…
> static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
> {
…
> wake_up(&buffer->pollq);
>
> + mutex_lock(&buffer->dmabufs_mutex);
> +
> + /* Close all attached DMABUFs */
…
> + mutex_unlock(&buffer->dmabufs_mutex);
> +
> kfree(ib);
…
Would you become interested to apply a statement like
“guard(mutex)(&buffer->dmabufs_mutex);”?
https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196
Regards,
Markus
Re: [PATCH v10 3/6] iio: core: Add new DMABUF interface infrastructure
On Sat, 2024-06-15 at 13:07 +0200, Paul Cercueil wrote: > Le dimanche 09 juin 2024 à 10:53 +0100, Jonathan Cameron a écrit : > > On Wed, 5 Jun 2024 13:08:42 +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 > > > Signed-off-by: Nuno Sa > > > > Need a brief note on the sign off chain. > > What is Nuno's role in this series as he's not sending the emails and > > not > > marked with Co-developed-by > > That's a good question. I think he sent one revision of the patchset > (v7 or something like that) so he added his SoB. > > (Nuno: you confirm?) exactly... > > I'll add his Co-developed-by then. Not sure if that is really deserved :)... Maybe just remove my tag. - Nuno Sá
Re: [PATCH v10 3/6] iio: core: Add new DMABUF interface infrastructure
Le dimanche 09 juin 2024 à 10:53 +0100, Jonathan Cameron a écrit :
> On Wed, 5 Jun 2024 13:08:42 +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
> > Signed-off-by: Nuno Sa
>
> Need a brief note on the sign off chain.
> What is Nuno's role in this series as he's not sending the emails and
> not
> marked with Co-developed-by
That's a good question. I think he sent one revision of the patchset
(v7 or something like that) so he added his SoB.
(Nuno: you confirm?)
I'll add his Co-developed-by then.
Cheers,
-Paul
> I gave this a much more thorough look in earlier versions than I have
> today but
> a few really minor things inline (that I might have fixed up whilst
> applying)
> but looks like you'll be done a v11 for Randy's docs comments anyway
> :(
>
> Jonathan
>
>
> > diff --git a/drivers/iio/industrialio-buffer.c
> > b/drivers/iio/industrialio-buffer.c
> > index 0138b21b244f..c98c8ac83785 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
>
> > +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;
>
> Given you are going to have a v11, please add a comment to this lock
> to say what data it is protecting.
>
> > +
> > + struct dma_buf_attachment *attach;
> > + struct sg_table *sgt;
> > + enum dma_data_direction dir;
> > + atomic_t seqno;
> > +};
>
>
> > diff --git a/include/linux/iio/buffer_impl.h
> > b/include/linux/iio/buffer_impl.h
> > index 89c3fd7c29ca..1a221c1d7736 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -9,8 +9,12 @@
> > #include
> > #include
> >
> > +struct dma_buf_attachment;
> > +struct dma_fence;
> > struct iio_dev;
> > +struct iio_dma_buffer_block;
> > struct iio_buffer;
> > +struct sg_table;
> >
> > /**
> > * INDIO_BUFFER_FLAG_FIXED_WATERMARK - Watermark level of the
> > buffer can not be
> > @@ -39,6 +43,13 @@ struct iio_buffer;
> > * device stops sampling. Calles are balanced
> > with @enable.
> > * @release: called when the last reference to the
> > buffer is dropped,
> > * should free all resources allocated by the
> > buffer.
> > + * @attach_dmabuf: called from userspace via ioctl to attach
> > one external
> > + * DMABUF.
> > + * @detach_dmabuf: called from userspace via ioctl to detach
> > one previously
> > + * attached DMABUF.
> > + * @enqueue_dmabuf:called from userspace via ioctl to queue
> > this DMABUF
> > + * object to this buffer. Requires a valid
> > DMABUF fd, that
> > + * was previouly attached to this buffer.
>
> Missing docs for lock_queue() and unlock_queue()
>
> Kernel-doc must be complete or bots are going to moan at us :(
>
> > * @modes: Supported operating modes by this buffer
> > type
> > * @flags: A bitmask combination of
> > INDIO_BUFFER_FLAG_*
> > *
> > @@ -68,6 +79,17 @@ struct iio_buffer_access_funcs {
> >
> > void (*release)(struct iio_buffer *buffer);
> >
> > + struct ii
Re: [PATCH v10 3/6] iio: core: Add new DMABUF interface infrastructure
On Wed, 5 Jun 2024 13:08:42 +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
> Signed-off-by: Nuno Sa
Need a brief note on the sign off chain.
What is Nuno's role in this series as he's not sending the emails and not
marked with Co-developed-by
I gave this a much more thorough look in earlier versions than I have today but
a few really minor things inline (that I might have fixed up whilst applying)
but looks like you'll be done a v11 for Randy's docs comments anyway :(
Jonathan
> diff --git a/drivers/iio/industrialio-buffer.c
> b/drivers/iio/industrialio-buffer.c
> index 0138b21b244f..c98c8ac83785 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> +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;
Given you are going to have a v11, please add a comment to this lock
to say what data it is protecting.
> +
> + struct dma_buf_attachment *attach;
> + struct sg_table *sgt;
> + enum dma_data_direction dir;
> + atomic_t seqno;
> +};
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 89c3fd7c29ca..1a221c1d7736 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -9,8 +9,12 @@
> #include
> #include
>
> +struct dma_buf_attachment;
> +struct dma_fence;
> struct iio_dev;
> +struct iio_dma_buffer_block;
> struct iio_buffer;
> +struct sg_table;
>
> /**
> * INDIO_BUFFER_FLAG_FIXED_WATERMARK - Watermark level of the buffer can not
> be
> @@ -39,6 +43,13 @@ struct iio_buffer;
> * device stops sampling. Calles are balanced with
> @enable.
> * @release: called when the last reference to the buffer is dropped,
> * should free all resources allocated by the buffer.
> + * @attach_dmabuf: called from userspace via ioctl to attach one external
> + * DMABUF.
> + * @detach_dmabuf: called from userspace via ioctl to detach one previously
> + * attached DMABUF.
> + * @enqueue_dmabuf: called from userspace via ioctl to queue this DMABUF
> + * object to this buffer. Requires a valid DMABUF fd, that
> + * was previouly attached to this buffer.
Missing docs for lock_queue() and unlock_queue()
Kernel-doc must be complete or bots are going to moan at us :(
> * @modes: Supported operating modes by this buffer type
> * @flags: A bitmask combination of INDIO_BUFFER_FLAG_*
> *
> @@ -68,6 +79,17 @@ struct iio_buffer_access_funcs {
>
> void (*release)(struct iio_buffer *buffer);
>
> + struct iio_dma_buffer_block * (*attach_dmabuf)(struct iio_buffer
> *buffer,
> +struct
> dma_buf_attachment *attach);
> + void (*detach_dmabuf)(struct iio_buffer *buffer,
> + struct iio_dma_buffer_block *block);
> + int (*enqueue_dmabuf)(struct iio_buffer *buffer,
> + struct iio_dma_buffer_block *block,
> + struct dma_fence *fence, struct sg_table *sgt,
> + size_t size, bool cyclic);
> +
[PATCH v10 3/6] iio: core: Add new DMABUF interface infrastructure
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
Signed-off-by: Nuno Sa
---
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.
v5: - Use dev_err() instead of pr_err()
- Inline to_iio_dma_fence()
- Add comment to explain why we unref twice when detaching dmabuf
- Remove TODO comment. It is actually safe to free the file's
private data even when transfers are still pending because it
won't be accessed.
- Fix documentation of new fields in struct iio_buffer_access_funcs
- iio_dma_resv_lock() does not need to be exported, make it static
v6: - Remove dead code in iio_dma_resv_lock()
- Fix non-block actually blocking
- Cache dma_buf_attachment instead of mapping/unmapping it for every
transfer
- Return -EINVAL instead of IIO_IOCTL_UNHANDLED for unknown ioctl
- Make .block_enqueue() callback take a dma_fence pointer, which
will be passed to iio_buffer_signal_dmabuf_done() instead of the
dma_buf_attachment; and remove the backpointer from the priv
structure to the dma_fence.
- Use dma_fence_begin/end_signalling in the dma_fence critical
sections
- Unref dma_fence and dma_buf_attachment in worker, because they
might try to lock the dma_resv, which would deadlock.
- Add buffer ops to lock/unlock the queue. This is motivated by the
fact that once the dma_fence has been installed, we cannot lock
anything anymore - so the queue must be locked before the
dma_fence is installed.
- Use 'long retl' variable to handle the return value of
dma_resv_wait_timeout()
- Protect dmabufs list access with a mutex
- Rework iio_buffer_find_attachment() to use the internal dmabufs
list, instead of messing with dmabufs private data.
- Add an atomically-increasing sequence number for fences
v8 - Fix swapped fence direction
- Simplify fence wait mechanism
- Remove "Buffer closed with active transfers" print, as it was dead
code
- Un-export iio_buffer_dmabuf_{get,put}. They are not used anywhere
else so they can even be static.
- Prevent attaching already-attached DMABUFs
v9: - Select DMA_SHARED_BUFFER in Kconfig
- Remove useless forward declaration of 'iio_dma_fence'
- Import DMA-BUF namespace
- Add missing __user tag to iio_buffer_detach_dmabuf() argument
---
drivers/iio/Kconfig | 1 +
drivers/iio/industrialio-buffer.c | 462 ++
include/linux/iio/buffer_impl.h | 30 ++
include/uapi/linux/iio/buffer.h | 22 ++
4 files changed, 515 insertions(+)
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 9c351ffc7bed..661127aed2f9 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -14,6 +14,7 @@ if IIO
config IIO_BUFFER
bool "Enable buffer support within IIO"
+ select DMA_SHARED_BUFFER
help
Provide core support for various buffer based data
acquisition methods.
diff --git a/drivers/iio/industrialio-buffer.c
b/drivers/iio/industrialio-buffer.c
index 0138b21b244f..c98c8ac83785 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -9,15 +9,20 @@
