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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> 
> ---
> 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á





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux