[PATCH] libcamera: dma_heaps: Add support for using udmabuf to alloc DMA heaps
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 29 19:17:30 CEST 2024
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
>
>
>
More information about the libcamera-devel
mailing list