[PATCH] libcamera: dma_heaps: Add support for using udmabuf to alloc DMA heaps

[Kieran Bingham]

Quoting Hans de Goede (2024-05-29 17:26:50)
> Hi,
> 
> On 5/28/24 11:37 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:
> >> Quoting Hans de Goede (2024-05-27 15:16:47)
> >>> Add support for using udmabuf to alloc DMA heaps, this is basically:
> >>> https://patchwork.libcamera.org/patch/18922/
> >>>
> >>> ported from the never merged HeapAllocator class to the current DmaHeap
> >>> class.
> >>
> >> True statements, but not really specific to the content of the patch?
> >>
> >> Perhaps:
> >>
> >> """
> >> The DMA heap allocator currently allocates from CMA and system heaps.
> >>
> >> These provide contiguous device memory but require dedicated and
> >> reserved memory regions to be configured on the platform and may require
> >> elevated privilidges to access.
> > 
> > The system heap doesn't require reserved memory regions, and also
> > doesn't provide contiguous device memory. The CMA heap also doesn't
> > required reserved memory regions actually.
> > 
> >> Extend the dma_heap allocator to support falling back to the memfd
> >> allocator backed by the udmabuf framework to prepare buffers that are
> >> compatible with linux drivers and the libcamera API.
> > 
> > When reading the patch, my very first thought is that udmabuf doesn't
> > make use of DMA heaps, so the allocator should belong to a different
> > class.
> > 
> > Thinking a bit more about it, I understand the appeal of hiding this
> > fallback from the DmaHeap user. I'm however a bit concerned that the
> > dmabuf instances allocated from the DMA system heap and through udmabuf
> > will not operate in a fully identical way. Returning one or the other
> > without the user being aware of what has been allocated may lead to
> > issues.
> > 
> > I may worry needlessly though. I haven't compared the system heap and
> > the udmabuf implementations, to see if the dmabuf operations differ in
> > ways that are significant. I think this should at least be checked
> > before we make a decision.
> > 
> >> The buffers allocated through memfd/udmabuf will not be contiguous and
> >> are likely not suitable for directly rendering to a display or encoder
> >> but may facilitate more usecases for allocating memory in pipelines that
> > 
> > Let's make a stronger statement : "are not suitable for zero-copy buffer
> > sharing with other devices".
> 
> Sounds good I'll come up with a new commit message for v2 taking all the various
> remarks into account.
> 
> >> make use of the CPU for processing including virtual pipeline handlers
> >> or the SoftISP.
> >> """
> >>
> >> Feel free to adapt the above of course if I've made any errors.
> >>
> >>> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> >>> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> >>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >>> ---
> >>>  include/libcamera/internal/dma_heaps.h |   3 +
> >>>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
> >>>  2 files changed, 109 insertions(+), 21 deletions(-)

... snip ...

> >>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
> >>> +{
> >>> +       /* size must be a multiple of the page-size round it up */
> > 
> > s/size/Size/
> > s/ up/ up./
> 
> Ack.
> 
> >>> +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> >>> +       size = (size + pageMask) & ~pageMask;
> >>> +
> >>> +       int ret = memfd_create(name, MFD_ALLOW_SEALING);
> > 
> > Needs sys/mman.h.
> 
> Ack.

... snip ...

> 
> >>> +               return {};
> >>> +       }
> >>> +
> >>> +       if (create.size != size)
> >>> +               LOG(DmaHeap, Warning)
> >>> +                       << "UdmaHeap allocated " << create.size << " bytes, "
> >>> +                       << "which is greater than requested : " << size << " bytes";
> > 
> > Why/when does this happen ?
> 
> This was taken from the original patch which I based this on. Shall I drop
> the check ?

I'm fine dropping it. I bet it was from my prototyping code where I was
likely investigating why I didn't get the size I requested, which
probably led to the comment "size must be a multiple of the page-size
round it up" at the top.

--
Kieran