[PATCH v2 4/5] libcamera: DmaBufAllocator: Support allocating from /dev/udmabuf

Laurent Pinchart laurent.pinchart at ideasonboard.com
Sun Jun 2 01:37:21 CEST 2024


Hi Hans,

Thank you for the patch.

On Thu, May 30, 2024 at 07:15:59PM +0200, Hans de Goede wrote:
> The dma-buf allocator currently allocates from CMA and system heaps.
> 
> Extend the dma-buf allocator to support allocating dma-buffers by creating
> memfd-s and turning those into dma-buffers using /dev/udmabuf.
> 
> The buffers allocated through memfd/udmabuf are not suitable for zero-copy
> buffer sharing with other devices.
> 
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
> Changes in v2:
> - Reword the commit message
> - Add a new DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf type for udmabuf
> - Drop unnecessary size != size check
> - Reword log messages to be more like the DMA heap alloc path
> - Move UniqueFD(ret) up so as to not leak the fd on errors
> ---
>  .../libcamera/internal/dma_buf_allocator.h    |   4 +
>  src/libcamera/dma_buf_allocator.cpp           | 111 +++++++++++++++---
>  2 files changed, 100 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> index a881042e..36ec1696 100644
> --- a/include/libcamera/internal/dma_buf_allocator.h
> +++ b/include/libcamera/internal/dma_buf_allocator.h
> @@ -20,6 +20,7 @@ public:
>  	enum class DmaBufAllocatorFlag {
>  		CmaHeap = 1 << 0,
>  		SystemHeap = 1 << 1,
> +		UDmaBuf = 1 << 2,
>  	};
>  
>  	using DmaBufAllocatorFlags = Flags<DmaBufAllocatorFlag>;
> @@ -30,7 +31,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 providerHandle_;
> +	DmaBufAllocatorFlag type_;
>  };
>  
>  LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag)
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index bc0d78ef..369a419a 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_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>
>  
> @@ -32,7 +36,7 @@ struct DmaBufAllocatorInfo {
>  };
>  #endif
>  
> -static constexpr std::array<DmaBufAllocatorInfo, 3> providerInfos = { {
> +static constexpr std::array<DmaBufAllocatorInfo, 4> providerInfos = { {
>  	/*
>  	 * /dev/dma_heap/linux,cma is the CMA dma-heap. When the cma heap size is
>  	 * specified on the kernel command line, this gets renamed to "reserved".
> @@ -40,6 +44,7 @@ static constexpr std::array<DmaBufAllocatorInfo, 3> providerInfos = { {
>  	{ DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap, "/dev/dma_heap/linux,cma" },
>  	{ DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap, "/dev/dma_heap/reserved" },
>  	{ DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap, "/dev/dma_heap/system" },
> +	{ DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf, "/dev/udmabuf" },
>  } };
>  
>  LOG_DEFINE_CATEGORY(DmaBufAllocator)
> @@ -63,6 +68,8 @@ LOG_DEFINE_CATEGORY(DmaBufAllocator)
>   * \brief Allocate from a CMA dma-heap, providing physically-contiguous memory
>   * \var DmaBufAllocator::SystemHeap
>   * \brief Allocate from the system dma-heap, using the page allocator
> + * \var DmaBufAllocator::UDmaBuf
> + * \brief Allocate using a memfd + /dev/udmabuf
>   */
>  
>  /**
> @@ -99,6 +106,7 @@ DmaBufAllocator::DmaBufAllocator(DmaBufAllocatorFlags type)
>  
>  		LOG(DmaBufAllocator, Debug) << "Using " << info.deviceNodeName;
>  		providerHandle_ = UniqueFD(ret);
> +		type_ = info.type;
>  		break;
>  	}
>  
> @@ -117,25 +125,76 @@ DmaBufAllocator::~DmaBufAllocator() = default;
>   * \return True if the DmaBufAllocator is valid, false otherwise
>   */
>  
> -/**
> - * \brief Allocate a dma-buf from the DmaBufAllocator
> - * \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 DmaBufAllocator::alloc(const char *name, std::size_t size)
> +UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
>  {
> -	int ret;
> +	/* Size must be a multiple of the page-size round it up. */

	/* Size must be a multiple of the page size. Round it up. */

> +	std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> +	size = (size + pageMask) & ~pageMask;
>  
> -	if (!name)
> +	int ret = memfd_create(name, MFD_ALLOW_SEALING);
> +	if (ret < 0) {
> +		ret = errno;
> +		LOG(DmaBufAllocator, Error)
> +			<< "Failed to allocate memfd storage for " << name
> +			<< ": " << strerror(ret);
>  		return {};
> +	}
>  
> +	UniqueFD memfd(ret);
> +
> +	ret = ftruncate(memfd.get(), size);
> +	if (ret < 0) {
> +		ret = errno;
> +		LOG(DmaBufAllocator, Error)
> +			<< "Failed to set memfd size for " << name
> +			<< ": " << strerror(ret);
> +		return {};
> +	}
> +
> +	/* udmabuf dma-buffers *must* have the F_SEAL_SHRINK seal. */
> +	ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);
> +	if (ret < 0) {
> +		ret = errno;
> +		LOG(DmaBufAllocator, Error)
> +			<< "Failed to seal the memfd for " << name
> +			<< ": " << strerror(ret);
> +		return {};
> +	}
> +
> +	struct udmabuf_create create;
> +
> +	create.memfd = memfd.get();
> +	create.flags = UDMABUF_FLAGS_CLOEXEC;
> +	create.offset = 0;
> +	create.size = size;
> +
> +	ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);
> +	if (ret < 0) {
> +		ret = errno;
> +		LOG(DmaBufAllocator, Error)
> +			<< "Failed to allocate " << size << " bytes for "
> +			<< name << ": " << strerror(ret);

This doesn't allocate memory, did you mean

		LOG(DmaBufAllocator, Error)
			<< "Failed to create dma buf for " << name << ": "
			<< strerror(ret);

?

> +		return {};
> +	}
> +
> +	/* The underlying memfd is kept as as a reference in the kernel */

s/kernel/kernel./

> +	UniqueFD uDma(ret);
> +
> +	/* Fail if not suitable, the allocation will be free'd by UniqueFD */

s/UniqueFD/UniqueFD./

> +	if (create.size < size) {

This can't happen, at least with the current implementation in the
kernel, as UDMABUF_CREATE doesn't update the struct udmabuf_create
argument but instead returns an error if something goes wrong. I think
you can drop this check, and just do

	return UniqueFD(ret);

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		LOG(DmaBufAllocator, Error)
> +			<< "UDMABUF_CREATE for " << name << " allocated "
> +			<< create.size << " bytes instead of " << size << " bytes";
> +		return {};
> +	}
> +
> +	return uDma;
> +}
> +
> +UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)
> +{
>  	struct dma_heap_allocation_data alloc = {};
> +	int ret;
>  
>  	alloc.len = size;
>  	alloc.fd_flags = O_CLOEXEC | O_RDWR;
> @@ -156,4 +215,26 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)
>  	return allocFd;
>  }
>  
> +/**
> + * \brief Allocate a dma-buf from the DmaBufAllocator
> + * \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 DmaBufAllocator::alloc(const char *name, std::size_t size)
> +{
> +	if (!name)
> +		return {};
> +
> +	if (type_ == DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)
> +		return allocFromUDmaBuf(name, size);
> +	else
> +		return allocFromHeap(name, size);
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list