[libcamera-devel] [PATCH v5 2/3] libcamera: Add UdmaHeap if DmaHeap is not valid

[Kieran Bingham]

Quoting Harvey Yang via libcamera-devel (2023-05-16 09:03:18)
> From: Cheng-Hao Yang <chenghaoyang at chromium.org>
> 
> If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.
> 
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  src/libcamera/heap_allocator.cpp | 106 +++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp
> index e9476de5..682a7e01 100644
> --- a/src/libcamera/heap_allocator.cpp
> +++ b/src/libcamera/heap_allocator.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>
>  
> @@ -52,6 +56,14 @@ public:
>         UniqueFD alloc(const char *name, std::size_t size) override;
>  };
>  
> +class UdmaHeap : public Heap
> +{
> +public:
> +       UdmaHeap();
> +       ~UdmaHeap();
> +       UniqueFD alloc(const char *name, std::size_t size) override;
> +};
> +
>  DmaHeap::DmaHeap()
>  {
>         for (const char *name : dmaHeapNames) {
> @@ -103,9 +115,103 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>         return allocFd;
>  }
>  
> +UdmaHeap::UdmaHeap()
> +{
> +       int ret = ::open("/dev/udmabuf", O_RDWR, 0);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(HeapAllocator, Error)
> +                       << "UdmaHeap failed to open allocator: " << strerror(ret);
> +
> +               if (ret == EACCES) {
> +                       LOG(HeapAllocator, Info)
> +                               << "UdmaHeap: Consider making /dev/udmabuf accessible by the video group";
> +                       LOG(HeapAllocator, Info)
> +                               << "UdmaHeap: Alternatively, add your user to the kvm group.";

I think I wrote this code, but I'm torn here for upstream. These are
probably very 'distro' specific comments which we probably shouldn't
include here.

I like that they help inform the user what they need to do - but I fear
maybe we should drop these comments for now.

> +               }
> +
> +       } else {
> +               handle_ = UniqueFD(ret);
> +       }
> +}
> +
> +UdmaHeap::~UdmaHeap() = default;
> +
> +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)
> +{
> +       if (!isValid()) {
> +               LOG(HeapAllocator, Fatal) << "UdmaHeap: Allocation attempted without allocator" << name;
> +               return {};
> +       }
> +
> +       int ret;
> +
> +       ret = memfd_create(name, MFD_ALLOW_SEALING);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(HeapAllocator, Error)
> +                       << "UdmaHeap failed to allocate memfd storage: "
> +                       << strerror(ret);
> +               return {};
> +       }
> +
> +       UniqueFD memfd(ret);
> +
> +       ret = ftruncate(memfd.get(), size);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(HeapAllocator, 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(HeapAllocator, 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(handle_.get(), UDMABUF_CREATE, &create);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(HeapAllocator, Error)
> +                       << "UdmaHeap failed to allocate " << size << " bytes: "
> +                       << strerror(ret);
> +               return {};
> +       }
> +
> +       /* The underlying memfd is kept as as a reference in the kernel */
> +       UniqueFD uDma(ret);
> +
> +       if (create.size != size)
> +               LOG(HeapAllocator, Warning)
> +                       << "UdmaHeap allocated " << create.size << " bytes instead of "
> +                       << size << " bytes";
> +
> +       /* Fail if not suitable, the allocation will be free'd by UniqueFD */
> +       if (create.size < size)
> +               return {};
> +
> +       LOG(HeapAllocator, Debug) << "UdmaHeap allocated " << create.size << " bytes";
> +
> +       return uDma;
> +}
> +
>  HeapAllocator::HeapAllocator()
>  {
>         heap_ = std::make_unique<DmaHeap>();
> +       if (!isValid())

I think if we've failed to create a DmaHeap - that's probably important
to report to the user.

I'm weary of this error path being taken for instance on an RPi or real
device that wouldn't necessarily work with UdmaHeap ? (Maybe it will, I
don't know) - but the fact we're using a different heap should probably
be reported in that instance.

Perhaps at least just a
		LOG(HeapAllocator, Info) << "Using UdmaHeap allocator";

Which would give us an indicator if we have issues reported on hardware
pipelines that can't use the buffers.

Otherwise, the allocation code all looks about how I remember it being
so 


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +               heap_ = std::make_unique<UdmaHeap>();
>  }
>  
>  HeapAllocator::~HeapAllocator() = default;
> -- 
> 2.40.1.606.ga4b1b128d6-goog
>