[PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to support system heap

[Laurent Pinchart]

Hi Milan and Andrey,

Thank you for the patch.

In the subject line, s/extend/Extend/

On Tue, Mar 19, 2024 at 01:35:50PM +0100, Milan Zamazal wrote:
> From: Andrey Konovalov <andrey.konovalov at linaro.org>
> 
> Add an argument to the constructor to specify dma heaps type(s)
> to use. Can be DmaHeapFlag::Cma and/or DmaHeapFlag::System.
> By default DmaHeapFlag::Cma is used. If both DmaHeapFlag::Cma and
> DmaHeapFlag::System are set, CMA heap is tried first.
> 
> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel at ucw.cz>
> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
> Reviewed-by: Pavel Machek <pavel at ucw.cz>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  include/libcamera/internal/dma_heaps.h | 12 ++++-
>  src/libcamera/dma_heaps.cpp            | 67 ++++++++++++++++++++------
>  2 files changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> index cff8f140..80bf29e7 100644
> --- a/include/libcamera/internal/dma_heaps.h
> +++ b/include/libcamera/internal/dma_heaps.h
> @@ -9,6 +9,7 @@
>  
>  #include <stddef.h>
>  
> +#include <libcamera/base/flags.h>
>  #include <libcamera/base/unique_fd.h>
>  
>  namespace libcamera {
> @@ -16,7 +17,14 @@ namespace libcamera {
>  class DmaHeap
>  {
>  public:
> -	DmaHeap();
> +	enum class DmaHeapFlag {
> +		Cma = 1 << 0,
> +		System = 1 << 1,
> +	};
> +
> +	using DmaHeapFlags = Flags<DmaHeapFlag>;
> +
> +	DmaHeap(DmaHeapFlags flags = DmaHeapFlag::Cma);
>  	~DmaHeap();
>  	bool isValid() const { return dmaHeapHandle_.isValid(); }
>  	UniqueFD alloc(const char *name, std::size_t size);
> @@ -25,4 +33,6 @@ private:
>  	UniqueFD dmaHeapHandle_;
>  };
>  
> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> index 38ef175a..d0e33ce6 100644
> --- a/src/libcamera/dma_heaps.cpp
> +++ b/src/libcamera/dma_heaps.cpp
> @@ -19,9 +19,11 @@
>  
>  /**
>   * \file dma_heaps.cpp
> - * \brief CMA dma-heap allocator
> + * \brief dma-heap allocator

I like that we're making this more generic :-)

>   */
>  
> +namespace libcamera {
> +
>  /*
>   * /dev/dma_heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
>   * to only have to worry about importing.
> @@ -29,42 +31,77 @@
>   * Annoyingly, should the cma heap size be specified on the kernel command line
>   * instead of DT, the heap gets named "reserved" instead.
>   */
> -static constexpr std::array<const char *, 2> heapNames = {
> -	"/dev/dma_heap/linux,cma",
> -	"/dev/dma_heap/reserved"
> +
> +/**
> + * \struct DmaHeapInfo
> + * \brief Tells what type of dma-heap the dma-heap represented by the device node name is
> + * \var DmaHeapInfo::flag
> + * \brief The type of the dma-heap
> + * \var DmaHeapInfo::name
> + * \brief The dma-heap's device node name
> + */

This is internal, no need to generate documentation. 

> +struct DmaHeapInfo {
> +	DmaHeap::DmaHeapFlag flag;
> +	const char *name;
>  };
>  
> -namespace libcamera {
> +static constexpr std::array<DmaHeapInfo, 3> heapInfos = {
> +	{ /* CMA heap names first */
> +	  { 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, 3> heapInfos = { {
	/* CMA heap names first */
	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
	{ DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" },
} };

>  
>  LOG_DEFINE_CATEGORY(DmaHeap)
>  
>  /**
>   * \class DmaHeap
> - * \brief Helper class for CMA dma-heap allocations
> + * \brief Helper class for dma-heap allocations

 * \brief Helper class for dma-heap allocations
 *
 * DMA heaps are kernel devices that provide an API to allocate memory from
 * different pools called "heaps", wrap each allocated piece of memory in a
 * dmabuf object, and return the dmabuf file descriptor to userspace. Multiple
 * heaps can be provided by the system, with different properties for the
 * underlying memory.
 *
 * This class wraps a DMA heap selected at construction time, and exposes
 * functions to manage memory allocation.

>   */
>  
>  /**
> - * \brief Construct a DmaHeap that owns a CMA dma-heap file descriptor
> + * \enum DmaHeap::DmaHeapFlag
> + * \brief Type of the dma-heap
> + * \var DmaHeap::Cma
> + * \brief Allocate from a CMA dma-heap

 * \brief Allocate from a CMA dma-heap, providing physically-contiguous memory

> + * \var DmaHeap::System
> + * \brief Allocate from the system dma-heap

 * \brief Allocate from the system dma-heap, using the page allocator

> + */
> +
> +/**
> + * \typedef DmaHeap::DmaHeapFlags
> + * \brief A bitwise combination of DmaHeap::DmaHeapFlag values
> + */
> +
> +/**
> + * \brief Construct a DmaHeap that owns a CMA or system dma-heap file descriptor

Just "Construct a DmaHeap of a given type" is enough.

> + * \param [in] flags The type(s) of the dma-heap(s) to allocate from

As this indicates the desired DMA heap type, let's call the argument
type, not flags.

>   *
> - * Goes through the internal list of possible names of the CMA dma-heap devices
> - * until a CMA dma-heap device is successfully opened. If it fails to open any
> - * dma-heap device, an invalid DmaHeap object is constructed. A valid DmaHeap
> - * object owns a wrapped dma-heap file descriptor.

Given that this patch rewrites the documentation of 02/18, I suppose I
can close my eyes and merge 02/18 without the review comments related to
documentation being addressed :-)

> + * By default \a flags are set to DmaHeap::DmaHeapFlag::Cma. The constructor goes
> + * through the internal list of possible names of the CMA and system dma-heap devices
> + * until the dma-heap device of the requested type is successfully opened. If more
> + * than one dma-heap type is specified in flags the CMA heap is tried first. If it
> + * fails to open any dma-heap device an invalid DmaHeap object is constructed.
> + * A valid DmaHeap object owns a wrapped dma-heap file descriptor.
>   *
>   * Please check the new DmaHeap object with \ref DmaHeap::isValid before using it.

That's even more internal implementation details. Let me try to help.

 * The DMA heap type is selected with the \a type parameter, which defaults to
 * the CMA heap. If no heap of the given type can be accessed, the constructed
 * DmaHeap instance is invalid as indicated by the isValid() function.
 *
 * Multiple types can be selected by combining type flags, in which case the
 * constructed DmaHeap will match one of the types. If the system provides
 * multiple heaps that match the requested types, which heap is used is
 * undefined.

>   */
> -DmaHeap::DmaHeap()
> +DmaHeap::DmaHeap(DmaHeapFlags flags)
>  {
> -	for (const char *name : heapNames) {
> -		int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);
> +	for (const auto &info : heapInfos) {
> +		if (!(flags & info.flag))
> +			continue;
> +
> +		int ret = ::open(info.name, O_RDWR | O_CLOEXEC, 0);
>  		if (ret < 0) {
>  			ret = errno;
>  			LOG(DmaHeap, Debug)
> -				<< "Failed to open " << name << ": "
> +				<< "Failed to open " << info.name << ": "
>  				<< strerror(ret);
>  			continue;
>  		}
>  
> +		LOG(DmaHeap, Debug) << "Using " << info.name;
>  		dmaHeapHandle_ = UniqueFD(ret);
>  		break;
>  	}

-- 
Regards,

Laurent Pinchart