[PATCH] libcamera: dma_heaps: Add support for using udmabuf to alloc DMA heaps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 28 23:37:46 CEST 2024
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".
> 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(-)
> >
> > diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> > index f0a8aa5d..7e1271ed 100644
> > --- a/include/libcamera/internal/dma_heaps.h
> > +++ b/include/libcamera/internal/dma_heaps.h
> > @@ -30,7 +30,10 @@ public:
> > UniqueFD alloc(const char *name, std::size_t size);
> >
> > private:
> > + UniqueFD allocFromHeap(const char *name, std::size_t size);
> > + UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
> > UniqueFD dmaHeapHandle_;
> > + bool useUDmaBuf_;
> > };
> >
> > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
> > diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> > index d4cb880b..bb707786 100644
> > --- a/src/libcamera/dma_heaps.cpp
> > +++ b/src/libcamera/dma_heaps.cpp
> > @@ -10,10 +10,14 @@
> > #include <array>
> > #include <fcntl.h>
> > #include <sys/ioctl.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > #include <unistd.h>
> >
> > #include <linux/dma-buf.h>
> > #include <linux/dma-heap.h>
> > +#include <linux/udmabuf.h>
> >
> > #include <libcamera/base/log.h>
> >
> > @@ -36,13 +40,15 @@ namespace libcamera {
> > struct DmaHeapInfo {
> > DmaHeap::DmaHeapFlag type;
> > const char *deviceNodeName;
> > + bool useUDmaBuf;
> > };
> > #endif
> >
> > -static constexpr std::array<DmaHeapInfo, 3> heapInfos = { {
> > - { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
> > - { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
> > - { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" },
> > +static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
> > + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
> > + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
> > + { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
> > + { DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },
>
> Should we distinguise UDMA with a different flag? or just simply keep it
> as a fall back?
>
> If we do use a different DmaHeapFlag then I think we don't need the
> boolean...
Exposing the difference to the user may alleviate some of my concerns.
Should we then rename the DmaHeap class to DmaBufAllocator ?
> > } };
> >
> > LOG_DEFINE_CATEGORY(DmaHeap)
> > @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)
> >
> > LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName;
> > dmaHeapHandle_ = UniqueFD(ret);
> > + useUDmaBuf_ = info.useUDmaBuf;
> > break;
> > }
> >
> > @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;
> > * \return True if the DmaHeap is valid, false otherwise
> > */
> >
> > -/**
> > - * \brief Allocate a dma-buf from the DmaHeap
> > - * \param [in] name The name to set for the allocated buffer
> > - * \param [in] size The size of the buffer to allocate
> > - *
> > - * Allocates a dma-buf with read/write access.
> > - *
> > - * If the allocation fails, return an invalid UniqueFD.
> > - *
> > - * \return The UniqueFD of the allocated buffer
> > - */
> > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> > +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)
> > {
> > - int ret;
> > -
> > - if (!name)
> > - return {};
> > -
> > struct dma_heap_allocation_data alloc = {};
> > + int ret;
> >
> > alloc.len = size;
> > alloc.fd_flags = O_CLOEXEC | O_RDWR;
> > @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> > return allocFd;
> > }
> >
> > +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./
> > + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> > + size = (size + pageMask) & ~pageMask;
> > +
> > + int ret = memfd_create(name, MFD_ALLOW_SEALING);
Needs sys/mman.h.
> > + if (ret < 0) {
> > + ret = errno;
> > + LOG(DmaHeap, Error)
> > + << "UdmaHeap failed to allocate memfd storage: "
UdmaHeap sounds like a frankenstein monster :-) Did you mean UDmaBuf
here ? I would just drop the prefix from this message and the ones below
actually.
> > + << strerror(ret);
>
> Completely optional/just an idea - I wonder if we should use 'name' in
> the log messages here and below.
>
> Would probably apply to allocFromHeap() too so likely a patch on top -
> not an update here.
>
> > + return {};
> > + }
> > +
> > + UniqueFD memfd(ret);
> > +
> > + ret = ftruncate(memfd.get(), size);
> > + if (ret < 0) {
> > + ret = errno;
> > + LOG(DmaHeap, Error)
> > + << "UdmaHeap failed to set memfd size: " << strerror(ret);
> > + return {};
> > + }
> > +
> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */
s/seal/seal./
> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);
> > + if (ret < 0) {
> > + ret = errno;
> > + LOG(DmaHeap, Error)
> > + << "UdmaHeap failed to seal the memfd: " << strerror(ret);
> > + return {};
> > + }
> > +
> > + struct udmabuf_create create;
> > +
> > + create.memfd = memfd.get();
> > + create.flags = UDMABUF_FLAGS_CLOEXEC;
> > + create.offset = 0;
> > + create.size = size;
> > +
> > + ret = ::ioctl(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);
> > + if (ret < 0) {
> > + ret = errno;
> > + LOG(DmaHeap, Error)
> > + << "UdmaHeap failed to allocate " << size << " bytes: "
> > + << strerror(ret);
> > + return {};
> > + }
> > +
> > + if (create.size < size) {
> > + LOG(DmaHeap, Error)
> > + << "UdmaHeap allocated " << create.size << " bytes instead of "
> > + << size << " bytes";
>
> Hrm, I think this path doesn't handle the now successfully created
> UDMABuf handle. Do we need to free it?
>
> Maybe we should move the 'UniqueFD uDma(ret);' above this statement so
> that the UniqueFD handles it correctly for both paths?
Sounds good.
> > + 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 ?
> > +
> > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */
s/UniqueFD/UniqueFD./
That comment doesn't seem correct though, there are no further checks.
Is it a leftover ?
> > + LOG(DmaHeap, Debug) << "UdmaHeap allocated " << create.size << " bytes";
> > +
> > + /* The underlying memfd is kept as as a reference in the kernel */
> > + UniqueFD uDma(ret);
> > +
> > + return uDma;
> > +}
> > +
> > +/**
> > + * \brief Allocate a dma-buf from the DmaHeap
>
> I think this would now change from 'the DmaHeap' to 'a DmaHeap' but
> that's just a nit.
I don't think that's correct. The allocator still allocates the buffer
from one specific backend, selected when the allocator is constructed.
> Otherwise / with the above considered I'd put my name to this ;-)
> (err... I think I co-wrote it in the past so I'm biased ;D)
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > + * \param [in] name The name to set for the allocated buffer
> > + * \param [in] size The size of the buffer to allocate
> > + *
> > + * Allocates a dma-buf with read/write access.
> > + *
> > + * If the allocation fails, return an invalid UniqueFD.
> > + *
> > + * \return The UniqueFD of the allocated buffer
> > + */
> > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> > +{
> > + if (!name)
> > + return {};
> > +
> > + if (useUDmaBuf_)
> > + return allocFromUDmaBuf(name, size);
> > + else
> > + return allocFromHeap(name, size);
> > +}
> > +
> > } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list