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

[Andrei Konovalov]

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 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.

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 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
>>