[PATCH] libcamera: dma_heaps: Add support for using udmabuf to alloc DMA heaps
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue May 28 13:32:10 CEST 2024
Hi Hans,
Thanks for refreshing this work!
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.
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.
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
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...
> } };
>
> 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 */
> + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> + size = (size + pageMask) & ~pageMask;
> +
> + int ret = memfd_create(name, MFD_ALLOW_SEALING);
> + if (ret < 0) {
> + ret = errno;
> + LOG(DmaHeap, Error)
> + << "UdmaHeap failed to allocate memfd storage: "
> + << 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 */
> + 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?
> + return {};
> + }
> +
> + if (create.size != size)
> + LOG(DmaHeap, Warning)
> + << "UdmaHeap allocated " << create.size << " bytes, "
> + << "which is greater than requested : " << size << " bytes";
> +
> + /* Fail if not suitable, the allocation will be free'd by UniqueFD */
> + 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.
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 */
> --
> 2.45.1
>
More information about the libcamera-devel
mailing list