Re: [PATCH v10 3/6] iio: core: Add new DMABUF interface infrastructure

[Paul Cercueil <paul@crapouillou.net>]

Le dimanche 09 juin 2024 à 10:53 +0100, Jonathan Cameron a écrit :
> On Wed,  5 Jun 2024 13:08:42 +0200
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
> > Add the necessary infrastructure to the IIO core to support a new
> > optional DMABUF based interface.
> > 
> > With this new interface, DMABUF objects (externally created) can be
> > attached to a IIO buffer, and subsequently used for data transfer.
> > 
> > A userspace application can then use this interface to share DMABUF
> > objects between several interfaces, allowing it to transfer data in
> > a
> > zero-copy fashion, for instance between IIO and the USB stack.
> > 
> > The userspace application can also memory-map the DMABUF objects,
> > and
> > access the sample data directly. The advantage of doing this vs.
> > the
> > read() interface is that it avoids an extra copy of the data
> > between the
> > kernel and userspace. This is particularly userful for high-speed
> > devices which produce several megabytes or even gigabytes of data
> > per
> > second.
> > 
> > As part of the interface, 3 new IOCTLs have been added:
> > 
> > IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
> >  Attach the DMABUF object identified by the given file descriptor
> > to the
> >  buffer.
> > 
> > IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
> >  Detach the DMABUF object identified by the given file descriptor
> > from
> >  the buffer. Note that closing the IIO buffer's file descriptor
> > will
> >  automatically detach all previously attached DMABUF objects.
> > 
> > IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
> >  Request a data transfer to/from the given DMABUF object. Its file
> >  descriptor, as well as the transfer size and flags are provided in
> > the
> >  "iio_dmabuf" structure.
> > 
> > These three IOCTLs have to be performed on the IIO buffer's file
> > descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> 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 <uapi/linux/iio/buffer.h>
> >  #include <linux/iio/buffer.h>
> >  
> > +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);
> > +	void (*lock_queue)(struct iio_buffer *buffer);
> > +	void (*unlock_queue)(struct iio_buffer *buffer);
> > +
> 
>