Re: [PATCH v3 07/11] iio: core: Add new DMABUF interface infrastructure

From: "Nuno Sá" <noname.nuno@gmail.com>
To: "Paul Cercueil" <paul@crapouillou.net>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Vinod Koul" <vkoul@kernel.org>,
	"Michael Hennerich" <Michael.Hennerich@analog.com>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Christian König" <christian.koenig@amd.com>
Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
	dmaengine@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [PATCH v3 07/11] iio: core: Add new DMABUF interface infrastructure
Date: Tue, 04 Apr 2023 10:21:27 +0200	[thread overview]
Message-ID: <9f5cac46e38897f54c5246bf400e8888324abad8.camel@gmail.com> (raw)
In-Reply-To: <2dac030470ffe74b6d21a1e6510afcefaf58cd6a.camel@crapouillou.net>

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 <paul@crapouillou.net>
> > > 
> > > ---
> > > 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á