[libcamera-devel,v2,03/18] libcamera: dma_heaps: extend DmaHeap class to support system heap
diff mbox series

Message ID 20240113142218.28063-4-hdegoede@redhat.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2,01/18] libcamera: pipeline: simple: fix size adjustment in validate()
Related show

Commit Message

Hans de Goede Jan. 13, 2024, 2:22 p.m. UTC
From: Andrey Konovalov <andrey.konovalov@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@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
---
 include/libcamera/internal/dma_heaps.h | 12 +++++++-
 src/libcamera/dma_heaps.cpp            | 39 +++++++++++++++-----------
 2 files changed, 34 insertions(+), 17 deletions(-)

Comments

Naushir Patuck Jan. 15, 2024, 9:07 a.m. UTC | #1
Hi,

On Sat, 13 Jan 2024 at 14:22, Hans de Goede via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> From: Andrey Konovalov <andrey.konovalov@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.

Just curious, what is the use case for trying an allocation in the CMA
first, then system help?  On the Raspberry Pi platforma at least, if
we are allocating through CMA, it's because of the hardware
requirements, and system heap allocation will not work at all.

Thanks,
Naush

>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@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),
> +       };
> +
> +       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 @@
>
>  #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 = {
> +       /* 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)
>  {
> -       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) {
> +                       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())
> --
> 2.43.0
>
Andrei Konovalov Jan. 15, 2024, 12:24 p.m. UTC | #2
Hi Naush,

On 15.01.2024 12:07, Naushir Patuck wrote:
> Hi,
> 
> On Sat, 13 Jan 2024 at 14:22, Hans de Goede via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
>>
>> From: Andrey Konovalov <andrey.konovalov@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.
> 
> Just curious, what is the use case for trying an allocation in the CMA
> first, then system help?  On the Raspberry Pi platforma at least, if
> we are allocating through CMA, it's because of the hardware
> requirements, and system heap allocation will not work at all.

The system dma-heap is used on the x86_64 targets where this is the only
dma-heap present by default.

On the Qualcomm platforms (sc8280xp, sm8250) both the CMA and the system
heaps work - at least in the current configuration without any fancy devices
like e.g hardware encoders behind the software ISP.

Thanks,
Andrei

> Thanks,
> Naush
> 
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel@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),
>> +       };
>> +
>> +       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 @@
>>
>>   #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 = {
>> +       /* 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)
>>   {
>> -       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) {
>> +                       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())
>> --
>> 2.43.0
>>
Laurent Pinchart Jan. 23, 2024, 2:26 p.m. UTC | #3
Hi Andrey,

Thank you for the patch.

On Sat, Jan 13, 2024 at 03:22:03PM +0100, 📷-dev wrote:
> From: Andrey Konovalov <andrey.konovalov@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@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@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())

Patch
diff mbox series

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),
+	};
+
+	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 @@ 
 
 #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 = {
+	/* 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)
 {
-	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) {
+			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())