| Message ID | 20240113142218.28063-4-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 >>
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())
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())