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

[Kieran Bingham]

Quoting Hans de Goede (2024-05-29 18:07:01)
> Hi,
> 
> On 5/29/24 6:43 PM, Kieran Bingham wrote:
> > 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,
> 
> Ok, I'll drop it.

I dug up the original WIP:
 - https://github.com/kbingham/libcamera/commit/8d79af53eaf461bab246c2fdeb1d9fdcbb288743#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R102-R109

So it probably started there indeed. Anyway - I think this code has come
a long way (for the better) since then ;-)


> > which
> > probably led to the comment "size must be a multiple of the page-size
> > round it up" at the top.
> 
> The page-rounding of size is actually an addition by me, otherwise the
> UDMABUF_CREATE call fails with errno=EINVAL.

Aha, well then it's a great addition ;-)

Thanks!

--
Kieran

> 
> Regards,
> 
> Hans
> 
> 
>