Re: [PATCH v2 07/12] iio: buffer-dma: Use DMABUFs instead of custom solution

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Mar 28, 2022 at 11:30 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lun., mars 28 2022 at 18:54:25 +0100, Jonathan Cameron
> <jic23@kernel.org> a écrit :
> > On Mon,  7 Feb 2022 12:59:28 +0000
> > Paul Cercueil <paul@crapouillou.net> wrote:
> >
> >>  Enhance the current fileio code by using DMABUF objects instead of
> >>  custom buffers.
> >>
> >>  This adds more code than it removes, but:
> >>  - a lot of the complexity can be dropped, e.g. custom kref and
> >>    iio_buffer_block_put_atomic() are not needed anymore;
> >>  - it will be much easier to introduce an API to export these DMABUF
> >>    objects to userspace in a following patch.

> > I'm a bit rusty on dma mappings, but you seem to have
> > a mixture of streaming and coherent mappings going on in here.
>
> That's OK, so am I. What do you call "streaming mappings"?

dma_*_coherent() are for coherent mappings (usually you do it once and
cache coherency is guaranteed by accessing this memory by device or
CPU).
dma_map_*() are for streaming, which means that you often want to map
arbitrary pages during the transfer (usually used for the cases when
you want to keep previous data and do something with a new coming, or
when a new coming data is supplied by different virtual address, and
hence has to be mapped for DMA).

> > Is it the case that the current code is using the coherent mappings
> > and a potential 'other user' of the dma buffer might need
> > streaming mappings?
>
> Something like that. There are two different things; on both cases,
> userspace needs to create a DMABUF with IIO_BUFFER_DMABUF_ALLOC_IOCTL,
> and the backing memory is allocated with dma_alloc_coherent().
>
> - For the userspace interface, you then have a "cpu access" IOCTL
> (DMA_BUF_IOCTL_SYNC), that allows userspace to inform when it will
> start/finish to process the buffer in user-space (which will
> sync/invalidate the data cache if needed). A buffer can then be
> enqueued for DMA processing (TX or RX) with the new
> IIO_BUFFER_DMABUF_ENQUEUE_IOCTL.
>
> - When the DMABUF created via the IIO core is sent to another driver
> through the driver's custom DMABUF import function, this driver will
> call dma_buf_attach(), which will call iio_buffer_dma_buf_map(). Since
> it has to return a "struct sg_table *", this function then simply
> creates a sgtable with one entry that points to the backing memory.

...

> >>  +   ret = dma_map_sgtable(at->dev, &dba->sg_table, dma_dir, 0);
> >>  +   if (ret) {
> >>  +           kfree(dba);
> >>  +           return ERR_PTR(ret);
> >>  +   }

Missed DMA mapping error check.

> >>  +
> >>  +   return &dba->sg_table;
> >>  +}

...

> >>  -   /* Must not be accessed outside the core. */
> >>  -   struct kref kref;


> >>  +   struct dma_buf *dmabuf;

Is it okay to access outside the core? If no, why did you remove
(actually not modify) the comment?

-- 
With Best Regards,
Andy Shevchenko