Message ID | 20240404084657.353464-4-mzamazal@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Milan and Andrey, Thank you for the patch. On Thu, Apr 04, 2024 at 10:46:40AM +0200, Milan Zamazal 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. > > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s > Tested-by: Pavel Machek <pavel@ucw.cz> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > Reviewed-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > include/libcamera/internal/dma_heaps.h | 12 +++- > src/libcamera/dma_heaps.cpp | 76 +++++++++++++++++++++----- > 2 files changed, 73 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h > index cff8f140..80bf29e7 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 70da6f3a..2503e4c5 100644 > --- a/src/libcamera/dma_heaps.cpp > +++ b/src/libcamera/dma_heaps.cpp > @@ -19,9 +19,11 @@ > > /** > * \file dma_heaps.cpp > - * \brief CMA dma-heap allocator > + * \brief dma-heap allocator > */ > > +namespace libcamera { > + > /* > * /dev/dma_heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma > * to only have to worry about importing. > @@ -29,40 +31,86 @@ > * 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" > + > +/** > + * \brief Specification of a heap info entry > + */ As this structure is internal to the file, it shouldn't end up in generated documentation. That was the point of my comment in v6, which I haven't expressed clearly enough. You can just replace the /** with /*, or also drop the \brief. Or possibly drop the comments, I'm not sure they add much. Up to you. > +struct DmaHeapInfo { > + /** > + * \brief Flag to match for considering the entry > + */ > + DmaHeap::DmaHeapFlag type; > + /** > + * \brief Path to a heap device > + */ > + const char *deviceNodeName; > }; > > -namespace libcamera { > +static constexpr std::array<DmaHeapInfo, 3> heapInfos = { { > + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" }, > + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" }, > + { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" }, > +} }; > > LOG_DEFINE_CATEGORY(DmaHeap) > > /** > * \class DmaHeap > - * \brief Helper class for CMA dma-heap allocations > + * \brief Helper class for dma-heap allocations > + * > + * DMA heaps are kernel devices that provide an API to allocate memory from > + * different pools called "heaps", wrap each allocated piece of memory in a > + * dmabuf object, and return the dmabuf file descriptor to userspace. Multiple > + * heaps can be provided by the system, with different properties for the > + * underlying memory. > + * > + * This class wraps a DMA heap selected at construction time, and exposes > + * functions to manage memory allocation. > */ > > /** > - * \brief Construct a DmaHeap that owns a CMA dma-heap file descriptor > + * \enum DmaHeap::DmaHeapFlag > + * \brief Type of the dma-heap > + * \var DmaHeap::Cma > + * \brief Allocate from a CMA dma-heap, providing physically-contiguous memory > + * \var DmaHeap::System > + * \brief Allocate from the system dma-heap, using the page allocator > + */ > + > +/** > + * \typedef DmaHeap::DmaHeapFlags > + * \brief A bitwise combination of DmaHeap::DmaHeapFlag values > + */ > + > +/** > + * \brief Construct a DmaHeap of a given type > + * \param [in] type The type(s) of the dma-heap(s) to allocate from No space between "param" and "[in]". Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > * > - * Looks for a CMA dma-heap device to use. If it fails to open any dma-heap > - * device, an invalid DmaHeap object is constructed. > + * The DMA heap type is selected with the \a type parameter, which defaults to > + * the CMA heap. If no heap of the given type can be accessed, the constructed > + * DmaHeap instance is invalid as indicated by the isValid() function. > * > - * Check the new DmaHeap object with DmaHeap::isValid before using it. > + * Multiple types can be selected by combining type flags, in which case the > + * constructed DmaHeap will match one of the types. If the system provides > + * multiple heaps that match the requested types, which heap is used is > + * undefined. > */ > -DmaHeap::DmaHeap() > +DmaHeap::DmaHeap(DmaHeapFlags type) > { > - for (const char *name : heapNames) { > - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); > + for (const auto &info : heapInfos) { > + if (!(type & info.type)) > + continue; > + > + int ret = ::open(info.deviceNodeName, O_RDWR | O_CLOEXEC, 0); > if (ret < 0) { > ret = errno; > LOG(DmaHeap, Debug) > - << "Failed to open " << name << ": " > + << "Failed to open " << info.deviceNodeName << ": " > << strerror(ret); > continue; > } > > + LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName; > dmaHeapHandle_ = UniqueFD(ret); > break; > }
diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h index cff8f140..80bf29e7 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 70da6f3a..2503e4c5 100644 --- a/src/libcamera/dma_heaps.cpp +++ b/src/libcamera/dma_heaps.cpp @@ -19,9 +19,11 @@ /** * \file dma_heaps.cpp - * \brief CMA dma-heap allocator + * \brief dma-heap allocator */ +namespace libcamera { + /* * /dev/dma_heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma * to only have to worry about importing. @@ -29,40 +31,86 @@ * 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" + +/** + * \brief Specification of a heap info entry + */ +struct DmaHeapInfo { + /** + * \brief Flag to match for considering the entry + */ + DmaHeap::DmaHeapFlag type; + /** + * \brief Path to a heap device + */ + const char *deviceNodeName; }; -namespace libcamera { +static constexpr std::array<DmaHeapInfo, 3> heapInfos = { { + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" }, + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" }, + { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" }, +} }; LOG_DEFINE_CATEGORY(DmaHeap) /** * \class DmaHeap - * \brief Helper class for CMA dma-heap allocations + * \brief Helper class for dma-heap allocations + * + * DMA heaps are kernel devices that provide an API to allocate memory from + * different pools called "heaps", wrap each allocated piece of memory in a + * dmabuf object, and return the dmabuf file descriptor to userspace. Multiple + * heaps can be provided by the system, with different properties for the + * underlying memory. + * + * This class wraps a DMA heap selected at construction time, and exposes + * functions to manage memory allocation. */ /** - * \brief Construct a DmaHeap that owns a CMA dma-heap file descriptor + * \enum DmaHeap::DmaHeapFlag + * \brief Type of the dma-heap + * \var DmaHeap::Cma + * \brief Allocate from a CMA dma-heap, providing physically-contiguous memory + * \var DmaHeap::System + * \brief Allocate from the system dma-heap, using the page allocator + */ + +/** + * \typedef DmaHeap::DmaHeapFlags + * \brief A bitwise combination of DmaHeap::DmaHeapFlag values + */ + +/** + * \brief Construct a DmaHeap of a given type + * \param [in] type The type(s) of the dma-heap(s) to allocate from * - * Looks for a CMA dma-heap device to use. If it fails to open any dma-heap - * device, an invalid DmaHeap object is constructed. + * The DMA heap type is selected with the \a type parameter, which defaults to + * the CMA heap. If no heap of the given type can be accessed, the constructed + * DmaHeap instance is invalid as indicated by the isValid() function. * - * Check the new DmaHeap object with DmaHeap::isValid before using it. + * Multiple types can be selected by combining type flags, in which case the + * constructed DmaHeap will match one of the types. If the system provides + * multiple heaps that match the requested types, which heap is used is + * undefined. */ -DmaHeap::DmaHeap() +DmaHeap::DmaHeap(DmaHeapFlags type) { - for (const char *name : heapNames) { - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); + for (const auto &info : heapInfos) { + if (!(type & info.type)) + continue; + + int ret = ::open(info.deviceNodeName, O_RDWR | O_CLOEXEC, 0); if (ret < 0) { ret = errno; LOG(DmaHeap, Debug) - << "Failed to open " << name << ": " + << "Failed to open " << info.deviceNodeName << ": " << strerror(ret); continue; } + LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName; dmaHeapHandle_ = UniqueFD(ret); break; }