Re: [PATCH v4 0/6] iio: Add buffer write() support

[Jonathan Cameron]

On Wed, 30 Aug 2023 17:11:18 +0100
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:

> On Mon,  7 Aug 2023 13:21:07 +0200
> Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> 
> > [V3 was: "iio: new DMABUF based API, v3"][1]
> > 
> > Hi Jonathan,
> > 
> > This is a subset of my patchset that introduced a new interface based on
> > DMABUF objects [1]. It adds write() support to the IIO buffer
> > infrastructure.
> > 
> > The reason it is not the full IIO-DMABUF patchset, is because you
> > requested performance benchmarks - and our current numbers are barely
> > better (~ +10%) than the fileio interface. There is a good reason for
> > that: V3 of the patchset switched from having the IIO core creating the
> > DMABUFs backed by physically contiguous memory, to having the IIO core
> > being a simple DMABUF importer, and having the DMABUFs created
> > externally. We now use the udmabuf driver to create those, and they are
> > allocated from paged memory. While this works perfectly fine, our
> > buffers are now cut in 4 KiB chunks (pages), non-contiguous in memory,
> > which causes the DMA hardware to create an IRQ storm, as it raises an
> > interrupt after each 4 KiB in the worst case scenario.  
> 
> Interesting. I'm guessing you don't necessarily need contiguous memory
> and huge pages would get rid of most of that overhead?
> 
> Given embedded target those huge pages are hard to get so you need
> hugetlb support to improve the chances of it working.  Some quick searching
> suggests there is possible support on the way.
> https://lore.kernel.org/linux-mm/20230817064623.3424348-1-vivek.kasireddy@xxxxxxxxx/
> 
> 
> > 
> > Anyway, this is not directly a problem of the IIO-DMABUF code - but I
> > can't really upstream a shiny new interface that I claim is much faster,
> > without giving numbers.
> > 
> > So while we fix this (either by updating the DMA IP and driver to
> > support scatter-gather)  
> 
> Long run you almost always end up needing that unless contig requirements
> are small and you want a robust solution.  I'm guessing no IOMMU to pretend
> it's all contiguous... 
> 
> > or by hacking something quick to give us
> > physically contiguous DMABUFs just for the benchmark), I thought it
> > would make sense to upstream the few patches of the V3 patchset that are
> > needed for the IIO-DMABUF interface but aren't directly related.  
> 
> Good idea.
> 
> > 
> > As for write() support, Nuno (Cc'd) said he will work on upstreaming the
> > DAC counterpart of adc/adi-axi-adc.c in the next few weeks, so there
> > will be a user for the buffer write() support. I hope you are okay with
> > this - otherwise, we can just wait until this work is done and submit it
> > all at once.  
> 
> Absolutely fine, though I won't pick this up without the user also being
> ready to go.


Having looked through these again, they are straight forward so no changes
requested from me.  Nuno, if you can add this set into appropriate
point in your series that will make use of it that will make my life easier
and ensure and minor rebasing etc happens without having to bother Paul.

Thanks,

Jonathan

> 
> > 
> > Changelog since v3:
> > - [PATCH 2/6] is new;
> > - [PATCH 3/6]: Drop iio_dma_buffer_space_available() function, and
> >   update patch description accordingly;
> > - [PATCH 6/6]: .space_available is now set to iio_dma_buffer_usage
> >   (which is functionally the exact same).
> > 
> > Cheers,
> > -Paul
> > 
> > [1] https://lore.kernel.org/all/20230403154800.215924-1-paul@xxxxxxxxxxxxxxx/
> > 
> > Alexandru Ardelean (1):
> >   iio: buffer-dma: split iio_dma_buffer_fileio_free() function
> > 
> > Paul Cercueil (5):
> >   iio: buffer-dma: Get rid of outgoing queue
> >   iio: buffer-dma: Rename iio_dma_buffer_data_available()
> >   iio: buffer-dma: Enable buffer write support
> >   iio: buffer-dmaengine: Support specifying buffer direction
> >   iio: buffer-dmaengine: Enable write support
> > 
> >  drivers/iio/adc/adi-axi-adc.c                 |   3 +-
> >  drivers/iio/buffer/industrialio-buffer-dma.c  | 187 ++++++++++++------
> >  .../buffer/industrialio-buffer-dmaengine.c    |  28 ++-
> >  include/linux/iio/buffer-dma.h                |  11 +-
> >  include/linux/iio/buffer-dmaengine.h          |   5 +-
> >  5 files changed, 160 insertions(+), 74 deletions(-)
> >   
>