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

[Naushir Patuck]

Hi,

On Sat, 13 Jan 2024 at 14:22, Hans de Goede via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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.

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 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),
> +       };
> +
> +       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
>