Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
Le lundi 29 janvier 2024 à 14:32 +0100, Paul Cercueil a écrit : > Le lundi 29 janvier 2024 à 14:17 +0100, Christian König a écrit : > > Am 29.01.24 um 14:06 schrieb Paul Cercueil: > > > Hi Christian, > > > > > > Le lundi 29 janvier 2024 à 13:52 +0100, Christian König a écrit : > > > > Am 27.01.24 um 17:50 schrieb Jonathan Cameron: > > > > > > > > + iio_buffer_dmabuf_put(attach); > > > > > > > > + > > > > > > > > +out_dmabuf_put: > > > > > > > > + dma_buf_put(dmabuf); > > > > > > > As below. Feels like a __free(dma_buf_put) bit of magic > > > > > > > would > > > > > > > be a > > > > > > > nice to have. > > > > > > I'm working on the patches right now, just one quick > > > > > > question. > > > > > > > > > > > > Having a __free(dma_buf_put) requires that dma_buf_put is > > > > > > first > > > > > > "registered" as a freeing function using DEFINE_FREE() in > > > > > > > > > > > buf.h>, which has not been done yet. > > > > > > > > > > > > That would mean carrying a dma-buf specific patch in your > > > > > > tree, > > > > > > are you > > > > > > OK with that? > > > > > Needs an ACK from appropriate maintainer, but otherwise I'm > > > > > fine > > > > > doing > > > > > so. Alternative is to circle back to this later after this > > > > > code is > > > > > upstream. > > > > Separate patches for that please, the autocleanup feature is so > > > > new > > > > that > > > > I'm not 100% convinced that everything works out smoothly from > > > > the > > > > start. > > > Separate patches is a given, did you mean outside this patchset? > > > Because I can send a separate patchset that introduces scope- > > > based > > > management for dma_fence and dma_buf, but then it won't have > > > users. > > > > Outside of the patchset, this is essentially brand new stuff. > > > > IIRC we have quite a number of dma_fence selftests and sw_sync > > which > > is > > basically code inside the drivers/dma-buf directory only there for > > testing DMA-buf functionality. > > > > Convert those over as well and I'm more than happy to upstream this > > change. > > Well there is very little to convert there; you can use scope-based > management when the unref is done in all exit points of the > functional > block, and the only place I could find that does that in drivers/dma- > buf/ was in dma_fence_chain_enable_signaling() in dma-fence-chain.c. Actually - not even that, since it doesn't call dma_fence_get() and dma_fence_put() on the same fence. So I cannot use it anywhere in drivers/dma-buf/. -Paul
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
Le lundi 29 janvier 2024 à 14:17 +0100, Christian König a écrit : > Am 29.01.24 um 14:06 schrieb Paul Cercueil: > > Hi Christian, > > > > Le lundi 29 janvier 2024 à 13:52 +0100, Christian König a écrit : > > > Am 27.01.24 um 17:50 schrieb Jonathan Cameron: > > > > > > > + iio_buffer_dmabuf_put(attach); > > > > > > > + > > > > > > > +out_dmabuf_put: > > > > > > > + dma_buf_put(dmabuf); > > > > > > As below. Feels like a __free(dma_buf_put) bit of magic > > > > > > would > > > > > > be a > > > > > > nice to have. > > > > > I'm working on the patches right now, just one quick > > > > > question. > > > > > > > > > > Having a __free(dma_buf_put) requires that dma_buf_put is > > > > > first > > > > > "registered" as a freeing function using DEFINE_FREE() in > > > > > > > > > buf.h>, which has not been done yet. > > > > > > > > > > That would mean carrying a dma-buf specific patch in your > > > > > tree, > > > > > are you > > > > > OK with that? > > > > Needs an ACK from appropriate maintainer, but otherwise I'm > > > > fine > > > > doing > > > > so. Alternative is to circle back to this later after this > > > > code is > > > > upstream. > > > Separate patches for that please, the autocleanup feature is so > > > new > > > that > > > I'm not 100% convinced that everything works out smoothly from > > > the > > > start. > > Separate patches is a given, did you mean outside this patchset? > > Because I can send a separate patchset that introduces scope-based > > management for dma_fence and dma_buf, but then it won't have users. > > Outside of the patchset, this is essentially brand new stuff. > > IIRC we have quite a number of dma_fence selftests and sw_sync which > is > basically code inside the drivers/dma-buf directory only there for > testing DMA-buf functionality. > > Convert those over as well and I'm more than happy to upstream this > change. Well there is very little to convert there; you can use scope-based management when the unref is done in all exit points of the functional block, and the only place I could find that does that in drivers/dma- buf/ was in dma_fence_chain_enable_signaling() in dma-fence-chain.c. Cheers, -Paul
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
Am 29.01.24 um 14:06 schrieb Paul Cercueil: Hi Christian, Le lundi 29 janvier 2024 à 13:52 +0100, Christian König a écrit : Am 27.01.24 um 17:50 schrieb Jonathan Cameron: + iio_buffer_dmabuf_put(attach); + +out_dmabuf_put: + dma_buf_put(dmabuf); As below. Feels like a __free(dma_buf_put) bit of magic would be a nice to have. I'm working on the patches right now, just one quick question. Having a __free(dma_buf_put) requires that dma_buf_put is first "registered" as a freeing function using DEFINE_FREE() in , which has not been done yet. That would mean carrying a dma-buf specific patch in your tree, are you OK with that? Needs an ACK from appropriate maintainer, but otherwise I'm fine doing so. Alternative is to circle back to this later after this code is upstream. Separate patches for that please, the autocleanup feature is so new that I'm not 100% convinced that everything works out smoothly from the start. Separate patches is a given, did you mean outside this patchset? Because I can send a separate patchset that introduces scope-based management for dma_fence and dma_buf, but then it won't have users. Outside of the patchset, this is essentially brand new stuff. IIRC we have quite a number of dma_fence selftests and sw_sync which is basically code inside the drivers/dma-buf directory only there for testing DMA-buf functionality. Convert those over as well and I'm more than happy to upstream this change. Thanks, Christian. Cheers, -Paul
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
Hi Christian, Le lundi 29 janvier 2024 à 13:52 +0100, Christian König a écrit : > Am 27.01.24 um 17:50 schrieb Jonathan Cameron: > > > > > + iio_buffer_dmabuf_put(attach); > > > > > + > > > > > +out_dmabuf_put: > > > > > + dma_buf_put(dmabuf); > > > > As below. Feels like a __free(dma_buf_put) bit of magic would > > > > be a > > > > nice to have. > > > I'm working on the patches right now, just one quick question. > > > > > > Having a __free(dma_buf_put) requires that dma_buf_put is first > > > "registered" as a freeing function using DEFINE_FREE() in > > > > > buf.h>, which has not been done yet. > > > > > > That would mean carrying a dma-buf specific patch in your tree, > > > are you > > > OK with that? > > Needs an ACK from appropriate maintainer, but otherwise I'm fine > > doing > > so. Alternative is to circle back to this later after this code is > > upstream. > > Separate patches for that please, the autocleanup feature is so new > that > I'm not 100% convinced that everything works out smoothly from the > start. Separate patches is a given, did you mean outside this patchset? Because I can send a separate patchset that introduces scope-based management for dma_fence and dma_buf, but then it won't have users. Cheers, -Paul
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
Am 27.01.24 um 17:50 schrieb Jonathan Cameron: + iio_buffer_dmabuf_put(attach); + +out_dmabuf_put: + dma_buf_put(dmabuf); As below. Feels like a __free(dma_buf_put) bit of magic would be a nice to have. I'm working on the patches right now, just one quick question. Having a __free(dma_buf_put) requires that dma_buf_put is first "registered" as a freeing function using DEFINE_FREE() in , which has not been done yet. That would mean carrying a dma-buf specific patch in your tree, are you OK with that? Needs an ACK from appropriate maintainer, but otherwise I'm fine doing so. Alternative is to circle back to this later after this code is upstream. Separate patches for that please, the autocleanup feature is so new that I'm not 100% convinced that everything works out smoothly from the start. Regards, Christian. Cheers, -Paul
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
> > > + iio_buffer_dmabuf_put(attach); > > > + > > > +out_dmabuf_put: > > > + dma_buf_put(dmabuf); > > As below. Feels like a __free(dma_buf_put) bit of magic would be a > > nice to have. > > I'm working on the patches right now, just one quick question. > > Having a __free(dma_buf_put) requires that dma_buf_put is first > "registered" as a freeing function using DEFINE_FREE() in buf.h>, which has not been done yet. > > That would mean carrying a dma-buf specific patch in your tree, are you > OK with that? Needs an ACK from appropriate maintainer, but otherwise I'm fine doing so. Alternative is to circle back to this later after this code is upstream. > > Cheers, > -Paul >
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
Hi Jonathan,
Le jeudi 21 décembre 2023 à 12:06 +, Jonathan Cameron a écrit :
> On Tue, 19 Dec 2023 18:50:06 +0100
> 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.
> >
>
> Fair enough - so they don't apply to the 'legacy' buffer which
> simplifies
> things but in one place you assume that logic is used (given error
> return
> values).
>
> > Signed-off-by: Paul Cercueil
> >
> This is big and complex and I'm out of time for now, so I've made
> some
> comments but should revisit it.
> I'm also looking for review from those more familiar with dmabuf side
> of things than I am!
>
> Jonathan
>
>
> >
> > +static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool
> > nonblock)
> > +{
> > + int ret;
> > +
> > + ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
> > + if (ret) {
> > + if (ret != -EDEADLK)
> > + goto out;
> > + if (nonblock) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > +
> > + ret = dma_resv_lock_slow_interruptible(dmabuf-
> > >resv, NULL);
> > + }
> > +
> > +out:
> > + return ret;
>
> I'm not a fan gotos that do nothing. Just return in appropriate
> places above.
>
> > +}
> >
> > +static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair
> > *ib, int *user_req)
> > +{
> > + struct dma_buf_attachment *attach;
> > + struct iio_dmabuf_priv *priv;
> > + struct dma_buf *dmabuf;
> > + int dmabuf_fd, ret = 0;
> > +
> > + if (copy_from_user(_fd, user_req,
> > sizeof(dmabuf_fd)))
> > + return -EFAULT;
> > +
> > + dmabuf = dma_buf_get(dmabuf_fd);
> > + if (IS_ERR(dmabuf))
> > + return PTR_ERR(dmabuf);
> > +
> > + attach = iio_buffer_find_attachment(ib->indio_dev,
> > dmabuf);
> > + if (IS_ERR(attach)) {
> > + ret = PTR_ERR(attach);
> > + goto out_dmabuf_put;
> > + }
> > +
> > + priv = attach->importer_priv;
> > + list_del_init(>entry);
> > +
> > + /*
> > +* Unref twice to release the reference obtained with
> > +* iio_buffer_find_attachment() above, and the one
> > obtained in
> > +* iio_buffer_attach_dmabuf().
> > +*/
> > + iio_buffer_dmabuf_put(attach);
> > + iio_buffer_dmabuf_put(attach);
> > +
> > +out_dmabuf_put:
> > + dma_buf_put(dmabuf);
> As below. Feels like a __free(dma_buf_put) bit of magic would be a
> nice to have.
I'm working on the patches right now, just one quick question.
Having a __free(dma_buf_put) requires that dma_buf_put is first
"registered" as a freeing function using DEFINE_FREE() in , which has not been done yet.
That would mean carrying a dma-buf specific patch in your tree, are you
OK with that?
Cheers,
-Paul
> > +
> > + 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 =
> > + container_of(fence, struct iio_dma_fence, base);
> > +
> > + kfree(iio_fence);
> > +}
> > +
> > +static const struct dma_fence_ops iio_buffer_dma_fence_ops = {
> > + .get_driver_name=
> > iio_buffer_dma_fence_get_driver_name,
> > +
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
On Tue, Dec 19, 2023 at 06:50:06PM +0100, 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
>
> ---
> 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
> ---
> drivers/iio/industrialio-buffer.c | 402 ++
> include/linux/iio/buffer_impl.h | 26 ++
> include/uapi/linux/iio/buffer.h | 22 ++
> 3 files changed, 450 insertions(+)
>
> diff --git a/drivers/iio/industrialio-buffer.c
> b/drivers/iio/industrialio-buffer.c
> index 09c41e9ccf87..24c040e073a7 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -13,10 +13,14 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
> #include
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -28,6 +32,31 @@
> #include
> #include
>
> +#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",
> @@ -332,6 +361,7 @@ void iio_buffer_init(struct iio_buffer *buffer)
> {
> INIT_LIST_HEAD(>demux_list);
> INIT_LIST_HEAD(>buffer_list);
> + INIT_LIST_HEAD(>dmabufs);
> init_waitqueue_head(>pollq);
> kref_init(>ref);
> if (!buffer->watermark)
> @@ -1519,14 +1549,54 @@ static void
> iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
> kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> }
>
> +static void iio_buffer_dmabuf_release(struct kref *ref)
> +{
> + struct iio_dmabuf_priv *priv = container_of(ref, struct
> iio_dmabuf_priv, ref);
> + struct dma_buf_attachment *attach = priv->attach;
> + struct iio_buffer *buffer = priv->buffer;
> + struct dma_buf *dmabuf = attach->dmabuf;
> +
> + buffer->access->detach_dmabuf(buffer, priv->block);
> +
> + dma_buf_detach(attach->dmabuf, attach);
> + dma_buf_put(dmabuf);
> + kfree(priv);
> +}
> +
> +void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach)
> +{
> + struct iio_dmabuf_priv *priv = attach->importer_priv;
> +
> + kref_get(>ref);
> +}
>
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
Hi Jonathan,
Le jeudi 21 décembre 2023 à 12:06 +, Jonathan Cameron a écrit :
> On Tue, 19 Dec 2023 18:50:06 +0100
> 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.
> >
>
> Fair enough - so they don't apply to the 'legacy' buffer which
> simplifies
> things but in one place you assume that logic is used (given error
> return
> values).
>
> > Signed-off-by: Paul Cercueil
> >
> This is big and complex and I'm out of time for now, so I've made
> some
> comments but should revisit it.
> I'm also looking for review from those more familiar with dmabuf side
> of things than I am!
>
> Jonathan
>
>
> >
> > +static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool
> > nonblock)
> > +{
> > + int ret;
> > +
> > + ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
> > + if (ret) {
> > + if (ret != -EDEADLK)
> > + goto out;
> > + if (nonblock) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > +
> > + ret = dma_resv_lock_slow_interruptible(dmabuf-
> > >resv, NULL);
> > + }
> > +
> > +out:
> > + return ret;
>
> I'm not a fan gotos that do nothing. Just return in appropriate
> places above.
>
> > +}
> >
> > +static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair
> > *ib, int *user_req)
> > +{
> > + struct dma_buf_attachment *attach;
> > + struct iio_dmabuf_priv *priv;
> > + struct dma_buf *dmabuf;
> > + int dmabuf_fd, ret = 0;
> > +
> > + if (copy_from_user(_fd, user_req,
> > sizeof(dmabuf_fd)))
> > + return -EFAULT;
> > +
> > + dmabuf = dma_buf_get(dmabuf_fd);
> > + if (IS_ERR(dmabuf))
> > + return PTR_ERR(dmabuf);
> > +
> > + attach = iio_buffer_find_attachment(ib->indio_dev,
> > dmabuf);
> > + if (IS_ERR(attach)) {
> > + ret = PTR_ERR(attach);
> > + goto out_dmabuf_put;
> > + }
> > +
> > + priv = attach->importer_priv;
> > + list_del_init(>entry);
> > +
> > + /*
> > +* Unref twice to release the reference obtained with
> > +* iio_buffer_find_attachment() above, and the one
> > obtained in
> > +* iio_buffer_attach_dmabuf().
> > +*/
> > + iio_buffer_dmabuf_put(attach);
> > + iio_buffer_dmabuf_put(attach);
> > +
> > +out_dmabuf_put:
> > + dma_buf_put(dmabuf);
> As below. Feels like a __free(dma_buf_put) bit of magic would be a
> nice to have.
Whoa, never heard about this. That looks great!
>
> > +
> > + 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 =
> > + container_of(fence, struct iio_dma_fence, base);
> > +
> > + 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
Re: [PATCH v5 5/8] iio: core: Add new DMABUF interface infrastructure
On Tue, 19 Dec 2023 18:50:06 +0100
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.
>
Fair enough - so they don't apply to the 'legacy' buffer which simplifies
things but in one place you assume that logic is used (given error return
values).
> Signed-off-by: Paul Cercueil
>
This is big and complex and I'm out of time for now, so I've made some
comments but should revisit it.
I'm also looking for review from those more familiar with dmabuf side
of things than I am!
Jonathan
>
> +static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock)
> +{
> + int ret;
> +
> + ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
> + if (ret) {
> + if (ret != -EDEADLK)
> + goto out;
> + if (nonblock) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + ret = dma_resv_lock_slow_interruptible(dmabuf->resv, NULL);
> + }
> +
> +out:
> + return ret;
I'm not a fan gotos that do nothing. Just return in appropriate places above.
> +}
>
> +static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair *ib, int
> *user_req)
> +{
> + struct dma_buf_attachment *attach;
> + struct iio_dmabuf_priv *priv;
> + struct dma_buf *dmabuf;
> + int dmabuf_fd, ret = 0;
> +
> + if (copy_from_user(_fd, user_req, sizeof(dmabuf_fd)))
> + return -EFAULT;
> +
> + dmabuf = dma_buf_get(dmabuf_fd);
> + if (IS_ERR(dmabuf))
> + return PTR_ERR(dmabuf);
> +
> + attach = iio_buffer_find_attachment(ib->indio_dev, dmabuf);
> + if (IS_ERR(attach)) {
> + ret = PTR_ERR(attach);
> + goto out_dmabuf_put;
> + }
> +
> + priv = attach->importer_priv;
> + list_del_init(>entry);
> +
> + /*
> + * Unref twice to release the reference obtained with
> + * iio_buffer_find_attachment() above, and the one obtained in
> + * iio_buffer_attach_dmabuf().
> + */
> + iio_buffer_dmabuf_put(attach);
> + iio_buffer_dmabuf_put(attach);
> +
> +out_dmabuf_put:
> + dma_buf_put(dmabuf);
As below. Feels like a __free(dma_buf_put) bit of magic would be a nice to have.
> +
> + 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 =
> + container_of(fence, struct iio_dma_fence, base);
> +
> + 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_dev *indio_dev = ib->indio_dev;
> + 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;
[PATCH v5 5/8] 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
---
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
---
drivers/iio/industrialio-buffer.c | 402 ++
include/linux/iio/buffer_impl.h | 26 ++
include/uapi/linux/iio/buffer.h | 22 ++
3 files changed, 450 insertions(+)
diff --git a/drivers/iio/industrialio-buffer.c
b/drivers/iio/industrialio-buffer.c
index 09c41e9ccf87..24c040e073a7 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -13,10 +13,14 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
#include
#include
+#include
#include
#include
@@ -28,6 +32,31 @@
#include
#include
+#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",
@@ -332,6 +361,7 @@ void iio_buffer_init(struct iio_buffer *buffer)
{
INIT_LIST_HEAD(>demux_list);
INIT_LIST_HEAD(>buffer_list);
+ INIT_LIST_HEAD(>dmabufs);
init_waitqueue_head(>pollq);
kref_init(>ref);
if (!buffer->watermark)
@@ -1519,14 +1549,54 @@ static void
iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
}
+static void iio_buffer_dmabuf_release(struct kref *ref)
+{
+ struct iio_dmabuf_priv *priv = container_of(ref, struct
iio_dmabuf_priv, ref);
+ struct dma_buf_attachment *attach = priv->attach;
+ struct iio_buffer *buffer = priv->buffer;
+ struct dma_buf *dmabuf = attach->dmabuf;
+
+ buffer->access->detach_dmabuf(buffer, priv->block);
+
+ dma_buf_detach(attach->dmabuf, attach);
+ dma_buf_put(dmabuf);
+ kfree(priv);
+}
+
+void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach)
+{
+ struct iio_dmabuf_priv *priv = attach->importer_priv;
+
+ kref_get(>ref);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_get);
+
+void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach)
+{
+ struct iio_dmabuf_priv *priv = attach->importer_priv;
+
+ kref_put(>ref, iio_buffer_dmabuf_release);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_put);
+
static int iio_buffer_chrdev_release(struct
