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

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Sat, Jan 13, 2024 at 03:22:03PM +0100, 📷-dev 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.
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel at ucw.cz>
> ---
>  include/libcamera/internal/dma_heaps.h | 12 +++++++-
>  src/libcamera/dma_heaps.cpp            | 39 +++++++++++++++-----------
>  2 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> index cff8f140..22aa1007 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),

No need for parentheses.

> +	};
> +
> +	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 7444d9c2..177de31b 100644
> --- a/src/libcamera/dma_heaps.cpp
> +++ b/src/libcamera/dma_heaps.cpp
> @@ -16,6 +16,8 @@

You need to include <utility> for std::pair.

>  
>  #include "libcamera/internal/dma_heaps.h"
>  
> +namespace libcamera {
> +
>  /*
>   * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
>   * to only have to worry about importing.
> @@ -23,28 +25,33 @@
>   * 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"
> +static constexpr std::array<std::pair<DmaHeap::DmaHeapFlag, const char *>, 3> heapNames = {

I'm not a big fan of std::pair for this kind of use case, it leads to
it->first and it->second in the code, which are not very readable.

struct DmaHeapInfo {
	DmaHeap::DmaHeapFlag flag;
	const char *name;
};

static constexpr std::array<DmaHeapInfo, 3> heapNames

will make the code more readable.

I would also, while at it, rename it to heapsInfo or something similar,
as it's not the just names anymore.

> +	/* CMA heap names first */
> +	std::make_pair(DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma"),
> +	std::make_pair(DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved"),
> +	std::make_pair(DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system")
>  };
>  
> -namespace libcamera {
> -
>  LOG_DEFINE_CATEGORY(DmaHeap)
>  
> -DmaHeap::DmaHeap()
> +DmaHeap::DmaHeap(DmaHeapFlags flags)

The new argument will need to be documented.

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

		if (!(flags & name.first))
			continue;

You won't have to touch the indentation of the code below.

> +			ret = ::open(name.second, O_RDWR | O_CLOEXEC, 0);
> +			if (ret < 0) {
> +				ret = errno;
> +				LOG(DmaHeap, Debug) << "Failed to open " << name.second << ": "
> +						    << strerror(ret);
> +				continue;
> +			}
> +
> +			LOG(DmaHeap, Debug) << "Using " << name.second;
> +			dmaHeapHandle_ = UniqueFD(ret);
> +			break;
> +		}
>  	}
>  
>  	if (!dmaHeapHandle_.isValid())

-- 
Regards,

Laurent Pinchart