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

[Laurent Pinchart]

Hi Harvey,

Thank you for the patch.

On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:
> From: Cheng-Hao Yang <chenghaoyang at chromium.org>
> 
> If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.

udmabuf and the DMA heaps are two completely different things, meant for
different use cases. An allocator that would use "whatever is available"
isn't a good generic helper.

I expect writing the documentation will make you realize that there's a
design issue here, as it's difficult to document clearly an API that
doesn't have a clear design.

> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp
> index 69f65062..8f99f590 100644
> --- a/src/libcamera/heap_allocator.cpp
> +++ b/src/libcamera/heap_allocator.cpp
> @@ -11,10 +11,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>
>  
> @@ -54,6 +58,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) {
> @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()
>  			continue;
>  		}
>  
> +		LOG(HeapAllocator, Info) << "Using DmaHeap allocator";
>  		handle_ = UniqueFD(ret);
>  		break;
>  	}
> @@ -105,9 +118,96 @@ 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);
> +	} else {
> +		LOG(HeapAllocator, Info) << "Using UdmaHeap allocator";
> +		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())
> +		heap_ = std::make_unique<UdmaHeap>();
>  }
>  
>  HeapAllocator::~HeapAllocator() = default;

-- 
Regards,

Laurent Pinchart