[Linaro-mm-sig] Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

Am 04.03.24 um 14:59 schrieb Paul Cercueil:

[SNIP]

...

...

• dma_to_ram = buffer->direction == IIO_BUFFER_DIRECTION_IN;
• if (dma_to_ram) {

• /*
  

•  * If we're writing to the DMABUF, make sure we
  

don't have

•  * readers
  

•  */
  

• retl = dma_resv_wait_timeout(dmabuf->resv,
  

• 			     DMA_RESV_USAGE_READ,
  

true,

• 			     timeout);
  

• if (retl == 0)
  

• 	retl = -EBUSY;
  

• if (retl < 0) {
  

• 	ret = (int)retl;
  

• 	goto err_resv_unlock;
  

• }
  

• }
• if (buffer->access->lock_queue)

• buffer->access->lock_queue(buffer);
  

• ret = dma_resv_reserve_fences(dmabuf->resv, 1);
• if (ret)

• goto err_queue_unlock;
  

• dma_resv_add_fence(dmabuf->resv, &fence->base,

• 	   dma_resv_usage_rw(dma_to_ram));
  

That is incorrect use of the function dma_resv_usage_rw(). That function is for retrieving fences and not adding them.

See the function implementation and comments, when you use it like this you get exactly what you don't want.

No, I get exactly what I want. If "dma_to_ram", I get DMA_RESV_USAGE_READ, otherwise I get DMA_RESV_USAGE_WRITE.

Ah, so basically !dma_to_ram means that you have a write access to the buffer?

If you really don't like the macro, I can inline things here.

Yeah, that would still be incorrect use.

The dma__resv_usage_rw() is for retrieving the fences to sync to for read and write operations and should never be used together with dma_fence_resv_add().

Regards, Christian.

Cheers, -Paul

...

Regards, Christian.

...

• dma_resv_unlock(dmabuf->resv);
• cookie = dma_fence_begin_signalling();
• ret = buffer->access->enqueue_dmabuf(buffer, priv->block,

&fence->base,

• 			     priv->sgt,
  

iio_dmabuf.bytes_used,

• 			     cyclic);
  

• if (ret) {

• /*
  

•  * DMABUF enqueue failed, but we already added the
  

fence.

•  * Signal the error through the fence completion
  

mechanism.

•  */
  

• iio_buffer_signal_dmabuf_done(&fence->base, ret);
  

• }
• if (buffer->access->unlock_queue)

• buffer->access->unlock_queue(buffer);
  

• dma_fence_end_signalling(cookie);
• dma_buf_put(dmabuf);
• return ret;

+err_queue_unlock:

• if (buffer->access->unlock_queue)

• buffer->access->unlock_queue(buffer);
  

+err_resv_unlock:

• dma_resv_unlock(dmabuf->resv);

+err_fence_put:

• dma_fence_put(&fence->base);

+err_attachment_put:

• iio_buffer_dmabuf_put(attach);

+err_dmabuf_put:

• dma_buf_put(dmabuf);
• return ret;

+}

+static void iio_buffer_cleanup(struct work_struct *work) +{

• struct iio_dma_fence *fence =

• container_of(work, struct iio_dma_fence, work);
  

• struct iio_dmabuf_priv *priv = fence->priv;
• struct dma_buf_attachment *attach = priv->attach;
• dma_fence_put(&fence->base);
• iio_buffer_dmabuf_put(attach);

+}

+void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int ret) +{

• struct iio_dma_fence *iio_fence =

• container_of(fence, struct iio_dma_fence, base);
  

• bool cookie = dma_fence_begin_signalling();
• /*

• * Get a reference to the fence, so that it's not freed as
  

soon as

• * it's signaled.
  

• */
  

• dma_fence_get(fence);
• fence->error = ret;
• dma_fence_signal(fence);
• dma_fence_end_signalling(cookie);
• /*

• * The fence will be unref'd in iio_buffer_cleanup.
  

• * It can't be done here, as the unref functions might try
  

to lock the

• * resv object, which can deadlock.
  

• */
  

• INIT_WORK(&iio_fence->work, iio_buffer_cleanup);
• schedule_work(&iio_fence->work);

+} +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;
• bool nonblock = filp->f_flags & O_NONBLOCK;
• switch (cmd) {
• case IIO_BUFFER_DMABUF_ATTACH_IOCTL:

• return iio_buffer_attach_dmabuf(ib, _arg,
  

nonblock);

• case IIO_BUFFER_DMABUF_DETACH_IOCTL:

• return iio_buffer_detach_dmabuf(ib, _arg,
  

nonblock);

• case IIO_BUFFER_DMABUF_ENQUEUE_IOCTL:

• return iio_buffer_enqueue_dmabuf(ib, _arg,
  

nonblock);

• default:

• return -EINVAL;
  

• }

+}

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,   }; @@ -1953,6 +2414,7 @@ static void iio_buffer_release(struct kref *ref)   {    struct iio_buffer *buffer = container_of(ref, struct iio_buffer, ref);

• mutex_destroy(&buffer->dmabufs_mutex);

buffer->access->release(buffer);   } diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h index 89c3fd7c29ca..f4b1147291e5 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.
    

* @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);

unsigned int modes;    unsigned int flags;   }; @@ -136,6 +158,12 @@ struct iio_buffer {    /* @ref: Reference count of the buffer. */    struct kref ref;

• /* @dmabufs: List of DMABUF attachments */
• struct list_head dmabufs; /* P: dmabufs_mutex */
• /* @dmabufs_mutex: Protects dmabufs */
• struct mutex dmabufs_mutex;

};   /** @@ -156,9 +184,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_fence *fence, 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_ */