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

[Jacopo Mondi]

Hi Harvey

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.
>
> 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);

Should you add O_CLOEXEC or does it generate issues ? Same question as
in 1/3 on the third argument

> +	if (ret < 0) {
> +		ret = errno;
> +		LOG(HeapAllocator, Error)
> +			<< "UdmaHeap failed to open allocator: " << strerror(ret);

You could return here and save an...

> +	} else {

... indendation level here

> +		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 {};
> +	}

UdmaHeap::alloc() is called by HeapAllocator::alloc() which already
checks for validity with a Fatal error. You can drop this change
> +
> +	int ret;
> +
> +	ret = memfd_create(name, MFD_ALLOW_SEALING);

You can declare and assign ret in one line

> +	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";

Should we continue or is it an error ? Also I would move this check
before the creation of uDma(ret);

> +
> +	/* Fail if not suitable, the allocation will be free'd by UniqueFD */
> +	if (create.size < size)
> +		return {};

Ah so larger size is ok, smaller it's not.

So I would check for this before the previous one

> +
> +	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;
> --
> 2.40.1.698.g37aff9b760-goog
>