| Message ID | 20230522083546.2465448-2-chenghaoyang@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Mon, May 22, 2023 at 08:35:06AM +0000, Harvey Yang via libcamera-devel wrote: > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > As other components (like the WIP virtual pipeline handler) also need a > heap allocator, move DmaHeap from raspberry pi pipeline handler to a > general HeapAllocator as a base class. > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > .../libcamera/internal/heap_allocator.h | 17 ++- > include/libcamera/internal/meson.build | 1 + > src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++ > src/libcamera/meson.build | 1 + > src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------ > src/libcamera/pipeline/rpi/vc4/meson.build | 5 +- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +- > 7 files changed, 146 insertions(+), 109 deletions(-) > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (57%) > create mode 100644 src/libcamera/heap_allocator.cpp > delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/heap_allocator.h > similarity index 57% > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h > rename to include/libcamera/internal/heap_allocator.h > index 0a4a8d86..92d4488a 100644 > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h > +++ b/include/libcamera/internal/heap_allocator.h > @@ -1,8 +1,9 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > * Copyright (C) 2020, Raspberry Pi Ltd > + * Copyright (C) 2023, Google Inc. > * > - * dma_heaps.h - Helper class for dma-heap allocations. > + * heap_allocator.h - Helper class for heap buffer allocations. > */ > > #pragma once > @@ -13,20 +14,18 @@ > > namespace libcamera { > > -namespace RPi { > +class Heap; > > -class DmaHeap > +class HeapAllocator > { > public: > - DmaHeap(); > - ~DmaHeap(); > - bool isValid() const { return dmaHeapHandle_.isValid(); } > + HeapAllocator(); > + ~HeapAllocator(); > + bool isValid() const; > UniqueFD alloc(const char *name, std::size_t size); > > private: > - UniqueFD dmaHeapHandle_; > + std::unique_ptr<Heap> heap_; > }; > > -} /* namespace RPi */ > - > } /* namespace libcamera */ > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > index d7508805..9d25c289 100644 > --- a/include/libcamera/internal/meson.build > +++ b/include/libcamera/internal/meson.build > @@ -26,6 +26,7 @@ libcamera_internal_headers = files([ > 'device_enumerator_udev.h', > 'formats.h', > 'framebuffer.h', > + 'heap_allocator.h', > 'ipa_manager.h', > 'ipa_module.h', > 'ipa_proxy.h', > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp > new file mode 100644 > index 00000000..69f65062 > --- /dev/null > +++ b/src/libcamera/heap_allocator.cpp > @@ -0,0 +1,130 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2020, Raspberry Pi Ltd > + * Copyright (C) 2023, Google Inc. > + * > + * heap_allocator.cpp - Helper class for heap buffer allocations. > + */ > + > +#include "libcamera/internal/heap_allocator.h" > + > +#include <array> > +#include <fcntl.h> > +#include <sys/ioctl.h> > +#include <unistd.h> > + > +#include <linux/dma-buf.h> > +#include <linux/dma-heap.h> > + > +#include <libcamera/base/log.h> > + > +namespace libcamera { > + > +/* > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma > + * to only have to worry about importing. > + * > + * 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 *, 3> dmaHeapNames = { > + "/dev/dma_heap/linux,cma", > + "/dev/dma_heap/reserved", > + "/dev/dma_heap/system" > +}; > + > +LOG_DEFINE_CATEGORY(HeapAllocator) > + > +class Heap > +{ > +public: > + virtual ~Heap() = default; > + bool isValid() const { return handle_.isValid(); } > + virtual UniqueFD alloc(const char *name, std::size_t size) = 0; > + > +protected: > + UniqueFD handle_; > +}; > + > +class DmaHeap : public Heap > +{ > +public: > + DmaHeap(); > + ~DmaHeap(); > + UniqueFD alloc(const char *name, std::size_t size) override; > +}; > + > +DmaHeap::DmaHeap() > +{ > + for (const char *name : dmaHeapNames) { > + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); Do you need the third argument ? Isn't it meaningful only if O_CREAT is part of the flags ? > + if (ret < 0) { > + ret = errno; > + LOG(HeapAllocator, Debug) << "DmaHeap failed to open " << name << ": " > + << strerror(ret); > + continue; > + } > + > + handle_ = UniqueFD(ret); > + break; > + } > + > + if (!handle_.isValid()) > + LOG(HeapAllocator, Error) << "DmaHeap could not open any dmaHeap device"; > +} > + > +DmaHeap::~DmaHeap() = default; > + > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size) > +{ > + int ret; > + > + if (!name) > + return {}; > + > + struct dma_heap_allocation_data alloc = {}; > + > + alloc.len = size; > + alloc.fd_flags = O_CLOEXEC | O_RDWR; > + > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > + if (ret < 0) { > + LOG(HeapAllocator, Error) << "DmaHeap allocation failure for " > + << name; > + return {}; > + } > + > + UniqueFD allocFd(alloc.fd); > + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); > + if (ret < 0) { > + LOG(HeapAllocator, Error) << "DmaHeap naming failure for " > + << name; > + return {}; > + } > + > + return allocFd; > +} > + > +HeapAllocator::HeapAllocator() > +{ > + heap_ = std::make_unique<DmaHeap>(); > +} > + > +HeapAllocator::~HeapAllocator() = default; > + > +bool HeapAllocator::isValid() const > +{ > + return heap_->isValid(); > +} > + > +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size) > +{ > + if (!isValid()) { > + LOG(HeapAllocator, Fatal) << "Allocation attempted without allocator" << name; > + return {}; > + } > + > + return heap_->alloc(name, size); > +} This all looks nice, but you're missing all the documentation for this and the next patches. This is the list of Doxygen warnings I get [3/13] Generating Documentation/doxygen with a custom command libcamera.git/src/libcamera/heap_allocator.cpp:59: warning: Compound libcamera::DmaHeap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:48: warning: Compound libcamera::Heap is not documented. libcamera.git/include/libcamera/internal/heap_allocator.h:23: warning: Compound libcamera::HeapAllocator is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:67: warning: Compound libcamera::UdmaHeap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:64: warning: Member alloc(const char *name, std::size_t size) override (function) of class libcamera::DmaHeap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:52: warning: Member isValid() const (function) of class libcamera::Heap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:53: warning: Member alloc(const char *name, std::size_t size)=0 (function) of class libcamera::Heap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:56: warning: Member handle_ (variable) of class libcamera::Heap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:52: warning: Member isValid() const (function) of class libcamera::Heap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:53: warning: Member alloc(const char *name, std::size_t size)=0 (function) of class libcamera::Heap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:56: warning: Member handle_ (variable) of class libcamera::Heap is not documented. libcamera.git/include/libcamera/internal/heap_allocator.h:28: warning: Member isValid() const (function) of class libcamera::HeapAllocator is not documented. libcamera.git/include/libcamera/internal/heap_allocator.h:29: warning: Member alloc(const char *name, std::size_t size) (function) of class libcamera::HeapAllocator is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:72: warning: Member alloc(const char *name, std::size_t size) override (function) of class libcamera::UdmaHeap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:52: warning: Member isValid() const (function) of class libcamera::Heap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:53: warning: Member alloc(const char *name, std::size_t size)=0 (function) of class libcamera::Heap is not documented. libcamera.git/src/libcamera/heap_allocator.cpp:56: warning: Member handle_ (variable) of class libcamera::Heap is not documented. Thanks j > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index d4385041..aa2d32a0 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -22,6 +22,7 @@ libcamera_sources = files([ > 'framebuffer.cpp', > 'framebuffer_allocator.cpp', > 'geometry.cpp', > + 'heap_allocator.cpp', > 'ipa_controls.cpp', > 'ipa_data_serializer.cpp', > 'ipa_interface.cpp', > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > deleted file mode 100644 > index 317b1fc1..00000000 > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > +++ /dev/null > @@ -1,90 +0,0 @@ > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > -/* > - * Copyright (C) 2020, Raspberry Pi Ltd > - * > - * dma_heaps.h - Helper class for dma-heap allocations. > - */ > - > -#include "dma_heaps.h" > - > -#include <array> > -#include <fcntl.h> > -#include <linux/dma-buf.h> > -#include <linux/dma-heap.h> > -#include <sys/ioctl.h> > -#include <unistd.h> > - > -#include <libcamera/base/log.h> > - > -/* > - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma > - * to only have to worry about importing. > - * > - * 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" > -}; > - > -namespace libcamera { > - > -LOG_DECLARE_CATEGORY(RPI) > - > -namespace RPi { > - > -DmaHeap::DmaHeap() > -{ > - for (const char *name : heapNames) { > - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); > - if (ret < 0) { > - ret = errno; > - LOG(RPI, Debug) << "Failed to open " << name << ": " > - << strerror(ret); > - continue; > - } > - > - dmaHeapHandle_ = UniqueFD(ret); > - break; > - } > - > - if (!dmaHeapHandle_.isValid()) > - LOG(RPI, Error) << "Could not open any dmaHeap device"; > -} > - > -DmaHeap::~DmaHeap() = default; > - > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size) > -{ > - int ret; > - > - if (!name) > - return {}; > - > - struct dma_heap_allocation_data alloc = {}; > - > - alloc.len = size; > - alloc.fd_flags = O_CLOEXEC | O_RDWR; > - > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > - if (ret < 0) { > - LOG(RPI, Error) << "dmaHeap allocation failure for " > - << name; > - return {}; > - } > - > - UniqueFD allocFd(alloc.fd); > - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); > - if (ret < 0) { > - LOG(RPI, Error) << "dmaHeap naming failure for " > - << name; > - return {}; > - } > - > - return allocFd; > -} > - > -} /* namespace RPi */ > - > -} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build > index cdb049c5..b2b79735 100644 > --- a/src/libcamera/pipeline/rpi/vc4/meson.build > +++ b/src/libcamera/pipeline/rpi/vc4/meson.build > @@ -1,8 +1,5 @@ > # SPDX-License-Identifier: CC0-1.0 > > -libcamera_sources += files([ > - 'dma_heaps.cpp', > - 'vc4.cpp', > -]) > +libcamera_sources += files(['vc4.cpp']) > > subdir('data') > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index 018cf488..410ecfaf 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -12,12 +12,11 @@ > #include <libcamera/formats.h> > > #include "libcamera/internal/device_enumerator.h" > +#include "libcamera/internal/heap_allocator.h" > > #include "../common/pipeline_base.h" > #include "../common/rpi_stream.h" > > -#include "dma_heaps.h" > - > using namespace std::chrono_literals; > > namespace libcamera { > @@ -87,7 +86,7 @@ public: > RPi::Device<Isp, 4> isp_; > > /* DMAHEAP allocation helper. */ > - RPi::DmaHeap dmaHeap_; > + HeapAllocator heapAllocator_; > SharedFD lsTable_; > > struct Config { > @@ -296,7 +295,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > { > Vc4CameraData *data = static_cast<Vc4CameraData *>(cameraData.get()); > > - if (!data->dmaHeap_.isValid()) > + if (!data->heapAllocator_.isValid()) > return -ENOMEM; > > MediaEntity *unicamImage = unicam->getEntityByName("unicam-image"); > @@ -670,9 +669,9 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) > { > params.ispControls = isp_[Isp::Input].dev()->controls(); > > - /* Allocate the lens shading table via dmaHeap and pass to the IPA. */ > + /* Allocate the lens shading table via heapAllocator and pass to the IPA. */ > if (!lsTable_.isValid()) { > - lsTable_ = SharedFD(dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize)); > + lsTable_ = SharedFD(heapAllocator_.alloc("ls_grid", ipa::RPi::MaxLsGridSize)); > if (!lsTable_.isValid()) > return -ENOMEM; > > -- > 2.40.1.698.g37aff9b760-goog >
Hi Harvey, Thank you for the patch. On 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote: > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > As other components (like the WIP virtual pipeline handler) also need a > heap allocator, move DmaHeap from raspberry pi pipeline handler to a > general HeapAllocator as a base class. > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > .../libcamera/internal/heap_allocator.h | 17 ++- > include/libcamera/internal/meson.build | 1 + > src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++ > src/libcamera/meson.build | 1 + > src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------ > src/libcamera/pipeline/rpi/vc4/meson.build | 5 +- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +- > 7 files changed, 146 insertions(+), 109 deletions(-) > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (57%) > create mode 100644 src/libcamera/heap_allocator.cpp > delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/heap_allocator.h > similarity index 57% > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h > rename to include/libcamera/internal/heap_allocator.h > index 0a4a8d86..92d4488a 100644 > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h > +++ b/include/libcamera/internal/heap_allocator.h > @@ -1,8 +1,9 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > * Copyright (C) 2020, Raspberry Pi Ltd > + * Copyright (C) 2023, Google Inc. > * > - * dma_heaps.h - Helper class for dma-heap allocations. > + * heap_allocator.h - Helper class for heap buffer allocations. > */ > > #pragma once > @@ -13,20 +14,18 @@ > > namespace libcamera { > > -namespace RPi { > +class Heap; > > -class DmaHeap > +class HeapAllocator > { > public: > - DmaHeap(); > - ~DmaHeap(); > - bool isValid() const { return dmaHeapHandle_.isValid(); } > + HeapAllocator(); > + ~HeapAllocator(); > + bool isValid() const; > UniqueFD alloc(const char *name, std::size_t size); > > private: > - UniqueFD dmaHeapHandle_; > + std::unique_ptr<Heap> heap_; > }; > > -} /* namespace RPi */ > - > } /* namespace libcamera */ > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > index d7508805..9d25c289 100644 > --- a/include/libcamera/internal/meson.build > +++ b/include/libcamera/internal/meson.build > @@ -26,6 +26,7 @@ libcamera_internal_headers = files([ > 'device_enumerator_udev.h', > 'formats.h', > 'framebuffer.h', > + 'heap_allocator.h', > 'ipa_manager.h', > 'ipa_module.h', > 'ipa_proxy.h', > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp > new file mode 100644 > index 00000000..69f65062 > --- /dev/null > +++ b/src/libcamera/heap_allocator.cpp > @@ -0,0 +1,130 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2020, Raspberry Pi Ltd > + * Copyright (C) 2023, Google Inc. > + * > + * heap_allocator.cpp - Helper class for heap buffer allocations. > + */ > + > +#include "libcamera/internal/heap_allocator.h" > + > +#include <array> > +#include <fcntl.h> > +#include <sys/ioctl.h> > +#include <unistd.h> > + > +#include <linux/dma-buf.h> > +#include <linux/dma-heap.h> > + > +#include <libcamera/base/log.h> > + > +namespace libcamera { > + > +/* > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma > + * to only have to worry about importing. > + * > + * 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 *, 3> dmaHeapNames = { > + "/dev/dma_heap/linux,cma", > + "/dev/dma_heap/reserved", > + "/dev/dma_heap/system" > +}; > + > +LOG_DEFINE_CATEGORY(HeapAllocator) > + > +class Heap > +{ > +public: > + virtual ~Heap() = default; > + bool isValid() const { return handle_.isValid(); } > + virtual UniqueFD alloc(const char *name, std::size_t size) = 0; > + > +protected: > + UniqueFD handle_; > +}; > + > +class DmaHeap : public Heap > +{ > +public: > + DmaHeap(); > + ~DmaHeap(); > + UniqueFD alloc(const char *name, std::size_t size) override; > +}; > + > +DmaHeap::DmaHeap() > +{ > + for (const char *name : dmaHeapNames) { > + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); > + if (ret < 0) { > + ret = errno; > + LOG(HeapAllocator, Debug) << "DmaHeap failed to open " << name << ": " > + << strerror(ret); > + continue; > + } > + > + handle_ = UniqueFD(ret); > + break; > + } > + > + if (!handle_.isValid()) > + LOG(HeapAllocator, Error) << "DmaHeap could not open any dmaHeap device"; > +} > + > +DmaHeap::~DmaHeap() = default; This should be automatically generated no? > + > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size) > +{ > + int ret; > + > + if (!name) > + return {}; > + > + struct dma_heap_allocation_data alloc = {}; > + > + alloc.len = size; > + alloc.fd_flags = O_CLOEXEC | O_RDWR; > + > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > + if (ret < 0) { > + LOG(HeapAllocator, Error) << "DmaHeap allocation failure for " > + << name; > + return {}; > + } > + > + UniqueFD allocFd(alloc.fd); > + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); > + if (ret < 0) { > + LOG(HeapAllocator, Error) << "DmaHeap naming failure for " > + << name; > + return {}; > + } > + > + return allocFd; > +} > + > +HeapAllocator::HeapAllocator() > +{ > + heap_ = std::make_unique<DmaHeap>(); > +} > + > +HeapAllocator::~HeapAllocator() = default; Same here > + > +bool HeapAllocator::isValid() const > +{ > + return heap_->isValid(); > +} > + > +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size) > +{ > + if (!isValid()) { > + LOG(HeapAllocator, Fatal) << "Allocation attempted without allocator" << name; > + return {}; > + } > + > + return heap_->alloc(name, size); > +} > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index d4385041..aa2d32a0 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -22,6 +22,7 @@ libcamera_sources = files([ > 'framebuffer.cpp', > 'framebuffer_allocator.cpp', > 'geometry.cpp', > + 'heap_allocator.cpp', > 'ipa_controls.cpp', > 'ipa_data_serializer.cpp', > 'ipa_interface.cpp', > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > deleted file mode 100644 > index 317b1fc1..00000000 > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > +++ /dev/null > @@ -1,90 +0,0 @@ > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > -/* > - * Copyright (C) 2020, Raspberry Pi Ltd > - * > - * dma_heaps.h - Helper class for dma-heap allocations. > - */ > - > -#include "dma_heaps.h" > - > -#include <array> > -#include <fcntl.h> > -#include <linux/dma-buf.h> > -#include <linux/dma-heap.h> > -#include <sys/ioctl.h> > -#include <unistd.h> > - > -#include <libcamera/base/log.h> > - > -/* > - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma > - * to only have to worry about importing. > - * > - * 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" > -}; > - > -namespace libcamera { > - > -LOG_DECLARE_CATEGORY(RPI) > - > -namespace RPi { > - > -DmaHeap::DmaHeap() > -{ > - for (const char *name : heapNames) { > - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); > - if (ret < 0) { > - ret = errno; > - LOG(RPI, Debug) << "Failed to open " << name << ": " > - << strerror(ret); > - continue; > - } > - > - dmaHeapHandle_ = UniqueFD(ret); > - break; > - } > - > - if (!dmaHeapHandle_.isValid()) > - LOG(RPI, Error) << "Could not open any dmaHeap device"; > -} > - > -DmaHeap::~DmaHeap() = default; > - > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size) > -{ > - int ret; > - > - if (!name) > - return {}; > - > - struct dma_heap_allocation_data alloc = {}; > - > - alloc.len = size; > - alloc.fd_flags = O_CLOEXEC | O_RDWR; > - > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > - if (ret < 0) { > - LOG(RPI, Error) << "dmaHeap allocation failure for " > - << name; > - return {}; > - } > - > - UniqueFD allocFd(alloc.fd); > - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); > - if (ret < 0) { > - LOG(RPI, Error) << "dmaHeap naming failure for " > - << name; > - return {}; > - } > - > - return allocFd; > -} > - > -} /* namespace RPi */ > - > -} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build > index cdb049c5..b2b79735 100644 > --- a/src/libcamera/pipeline/rpi/vc4/meson.build > +++ b/src/libcamera/pipeline/rpi/vc4/meson.build > @@ -1,8 +1,5 @@ > # SPDX-License-Identifier: CC0-1.0 > > -libcamera_sources += files([ > - 'dma_heaps.cpp', > - 'vc4.cpp', > -]) > +libcamera_sources += files(['vc4.cpp']) > > subdir('data') > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index 018cf488..410ecfaf 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -12,12 +12,11 @@ > #include <libcamera/formats.h> > > #include "libcamera/internal/device_enumerator.h" > +#include "libcamera/internal/heap_allocator.h" > > #include "../common/pipeline_base.h" > #include "../common/rpi_stream.h" > > -#include "dma_heaps.h" > - > using namespace std::chrono_literals; > > namespace libcamera { > @@ -87,7 +86,7 @@ public: > RPi::Device<Isp, 4> isp_; > > /* DMAHEAP allocation helper. */ > - RPi::DmaHeap dmaHeap_; > + HeapAllocator heapAllocator_; > SharedFD lsTable_; > > struct Config { > @@ -296,7 +295,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > { > Vc4CameraData *data = static_cast<Vc4CameraData *>(cameraData.get()); > > - if (!data->dmaHeap_.isValid()) > + if (!data->heapAllocator_.isValid()) > return -ENOMEM; > > MediaEntity *unicamImage = unicam->getEntityByName("unicam-image"); > @@ -670,9 +669,9 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) > { > params.ispControls = isp_[Isp::Input].dev()->controls(); > > - /* Allocate the lens shading table via dmaHeap and pass to the IPA. */ > + /* Allocate the lens shading table via heapAllocator and pass to the IPA. */ > if (!lsTable_.isValid()) { > - lsTable_ = SharedFD(dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize)); > + lsTable_ = SharedFD(heapAllocator_.alloc("ls_grid", ipa::RPi::MaxLsGridSize)); > if (!lsTable_.isValid()) > return -ENOMEM; >
Hi Harvey On 5/30/23 11:38 AM, Umang Jain via libcamera-devel wrote: > Hi Harvey, > > Thank you for the patch. > > On 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote: >> From: Cheng-Hao Yang <chenghaoyang@chromium.org> >> >> As other components (like the WIP virtual pipeline handler) also need a >> heap allocator, move DmaHeap from raspberry pi pipeline handler to a >> general HeapAllocator as a base class. >> >> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> >> --- >> .../libcamera/internal/heap_allocator.h | 17 ++- >> include/libcamera/internal/meson.build | 1 + >> src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++ >> src/libcamera/meson.build | 1 + >> src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------ >> src/libcamera/pipeline/rpi/vc4/meson.build | 5 +- >> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +- >> 7 files changed, 146 insertions(+), 109 deletions(-) >> rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => >> include/libcamera/internal/heap_allocator.h (57%) >> create mode 100644 src/libcamera/heap_allocator.cpp >> delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp >> >> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h >> b/include/libcamera/internal/heap_allocator.h >> similarity index 57% >> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h >> rename to include/libcamera/internal/heap_allocator.h >> index 0a4a8d86..92d4488a 100644 >> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h >> +++ b/include/libcamera/internal/heap_allocator.h >> @@ -1,8 +1,9 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> * Copyright (C) 2020, Raspberry Pi Ltd >> + * Copyright (C) 2023, Google Inc. >> * >> - * dma_heaps.h - Helper class for dma-heap allocations. >> + * heap_allocator.h - Helper class for heap buffer allocations. >> */ >> #pragma once >> @@ -13,20 +14,18 @@ >> namespace libcamera { >> -namespace RPi { >> +class Heap; >> -class DmaHeap >> +class HeapAllocator >> { >> public: >> - DmaHeap(); >> - ~DmaHeap(); >> - bool isValid() const { return dmaHeapHandle_.isValid(); } >> + HeapAllocator(); >> + ~HeapAllocator(); >> + bool isValid() const; >> UniqueFD alloc(const char *name, std::size_t size); >> private: >> - UniqueFD dmaHeapHandle_; >> + std::unique_ptr<Heap> heap_; >> }; >> -} /* namespace RPi */ >> - >> } /* namespace libcamera */ >> diff --git a/include/libcamera/internal/meson.build >> b/include/libcamera/internal/meson.build >> index d7508805..9d25c289 100644 >> --- a/include/libcamera/internal/meson.build >> +++ b/include/libcamera/internal/meson.build >> @@ -26,6 +26,7 @@ libcamera_internal_headers = files([ >> 'device_enumerator_udev.h', >> 'formats.h', >> 'framebuffer.h', >> + 'heap_allocator.h', >> 'ipa_manager.h', >> 'ipa_module.h', >> 'ipa_proxy.h', >> diff --git a/src/libcamera/heap_allocator.cpp >> b/src/libcamera/heap_allocator.cpp >> new file mode 100644 >> index 00000000..69f65062 >> --- /dev/null >> +++ b/src/libcamera/heap_allocator.cpp >> @@ -0,0 +1,130 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2020, Raspberry Pi Ltd >> + * Copyright (C) 2023, Google Inc. >> + * >> + * heap_allocator.cpp - Helper class for heap buffer allocations. >> + */ >> + >> +#include "libcamera/internal/heap_allocator.h" >> + >> +#include <array> >> +#include <fcntl.h> >> +#include <sys/ioctl.h> >> +#include <unistd.h> >> + >> +#include <linux/dma-buf.h> >> +#include <linux/dma-heap.h> >> + >> +#include <libcamera/base/log.h> >> + >> +namespace libcamera { >> + >> +/* >> + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows >> dmaheap-cma >> + * to only have to worry about importing. >> + * >> + * 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 *, 3> dmaHeapNames = { >> + "/dev/dma_heap/linux,cma", >> + "/dev/dma_heap/reserved", >> + "/dev/dma_heap/system" >> +}; >> + >> +LOG_DEFINE_CATEGORY(HeapAllocator) >> + >> +class Heap >> +{ >> +public: >> + virtual ~Heap() = default; >> + bool isValid() const { return handle_.isValid(); } >> + virtual UniqueFD alloc(const char *name, std::size_t size) = 0; >> + >> +protected: >> + UniqueFD handle_; >> +}; >> + >> +class DmaHeap : public Heap >> +{ >> +public: >> + DmaHeap(); >> + ~DmaHeap(); >> + UniqueFD alloc(const char *name, std::size_t size) override; >> +}; >> + >> +DmaHeap::DmaHeap() >> +{ >> + for (const char *name : dmaHeapNames) { >> + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); >> + if (ret < 0) { >> + ret = errno; >> + LOG(HeapAllocator, Debug) << "DmaHeap failed to open " >> << name << ": " >> + << strerror(ret); >> + continue; >> + } >> + >> + handle_ = UniqueFD(ret); >> + break; >> + } >> + >> + if (!handle_.isValid()) >> + LOG(HeapAllocator, Error) << "DmaHeap could not open any >> dmaHeap device"; >> +} >> + >> +DmaHeap::~DmaHeap() = default; > > This should be automatically generated no? Sorry, this is actually correct. I got confused between implicitly-declared destructor vs implicit user-defined desctructor. >> + >> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size) >> +{ >> + int ret; >> + >> + if (!name) >> + return {}; >> + >> + struct dma_heap_allocation_data alloc = {}; >> + >> + alloc.len = size; >> + alloc.fd_flags = O_CLOEXEC | O_RDWR; >> + >> + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); >> + if (ret < 0) { >> + LOG(HeapAllocator, Error) << "DmaHeap allocation failure for " >> + << name; >> + return {}; >> + } >> + >> + UniqueFD allocFd(alloc.fd); >> + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); >> + if (ret < 0) { >> + LOG(HeapAllocator, Error) << "DmaHeap naming failure for " >> + << name; >> + return {}; >> + } >> + >> + return allocFd; >> +} >> + >> +HeapAllocator::HeapAllocator() >> +{ >> + heap_ = std::make_unique<DmaHeap>(); >> +} >> + >> +HeapAllocator::~HeapAllocator() = default; > > Same here Same applies here. Sorry for the noise! >> + >> +bool HeapAllocator::isValid() const >> +{ >> + return heap_->isValid(); >> +} >> + >> +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size) >> +{ >> + if (!isValid()) { >> + LOG(HeapAllocator, Fatal) << "Allocation attempted without >> allocator" << name; >> + return {}; >> + } >> + >> + return heap_->alloc(name, size); >> +} >> + >> +} /* namespace libcamera */ >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build >> index d4385041..aa2d32a0 100644 >> --- a/src/libcamera/meson.build >> +++ b/src/libcamera/meson.build >> @@ -22,6 +22,7 @@ libcamera_sources = files([ >> 'framebuffer.cpp', >> 'framebuffer_allocator.cpp', >> 'geometry.cpp', >> + 'heap_allocator.cpp', >> 'ipa_controls.cpp', >> 'ipa_data_serializer.cpp', >> 'ipa_interface.cpp', >> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp >> b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp >> deleted file mode 100644 >> index 317b1fc1..00000000 >> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp >> +++ /dev/null >> @@ -1,90 +0,0 @@ >> -/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> -/* >> - * Copyright (C) 2020, Raspberry Pi Ltd >> - * >> - * dma_heaps.h - Helper class for dma-heap allocations. >> - */ >> - >> -#include "dma_heaps.h" >> - >> -#include <array> >> -#include <fcntl.h> >> -#include <linux/dma-buf.h> >> -#include <linux/dma-heap.h> >> -#include <sys/ioctl.h> >> -#include <unistd.h> >> - >> -#include <libcamera/base/log.h> >> - >> -/* >> - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows >> dmaheap-cma >> - * to only have to worry about importing. >> - * >> - * 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" >> -}; >> - >> -namespace libcamera { >> - >> -LOG_DECLARE_CATEGORY(RPI) >> - >> -namespace RPi { >> - >> -DmaHeap::DmaHeap() >> -{ >> - for (const char *name : heapNames) { >> - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); >> - if (ret < 0) { >> - ret = errno; >> - LOG(RPI, Debug) << "Failed to open " << name << ": " >> - << strerror(ret); >> - continue; >> - } >> - >> - dmaHeapHandle_ = UniqueFD(ret); >> - break; >> - } >> - >> - if (!dmaHeapHandle_.isValid()) >> - LOG(RPI, Error) << "Could not open any dmaHeap device"; >> -} >> - >> -DmaHeap::~DmaHeap() = default; >> - >> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size) >> -{ >> - int ret; >> - >> - if (!name) >> - return {}; >> - >> - struct dma_heap_allocation_data alloc = {}; >> - >> - alloc.len = size; >> - alloc.fd_flags = O_CLOEXEC | O_RDWR; >> - >> - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); >> - if (ret < 0) { >> - LOG(RPI, Error) << "dmaHeap allocation failure for " >> - << name; >> - return {}; >> - } >> - >> - UniqueFD allocFd(alloc.fd); >> - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); >> - if (ret < 0) { >> - LOG(RPI, Error) << "dmaHeap naming failure for " >> - << name; >> - return {}; >> - } >> - >> - return allocFd; >> -} >> - >> -} /* namespace RPi */ >> - >> -} /* namespace libcamera */ >> diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build >> b/src/libcamera/pipeline/rpi/vc4/meson.build >> index cdb049c5..b2b79735 100644 >> --- a/src/libcamera/pipeline/rpi/vc4/meson.build >> +++ b/src/libcamera/pipeline/rpi/vc4/meson.build >> @@ -1,8 +1,5 @@ >> # SPDX-License-Identifier: CC0-1.0 >> -libcamera_sources += files([ >> - 'dma_heaps.cpp', >> - 'vc4.cpp', >> -]) >> +libcamera_sources += files(['vc4.cpp']) >> subdir('data') >> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> index 018cf488..410ecfaf 100644 >> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> @@ -12,12 +12,11 @@ >> #include <libcamera/formats.h> >> #include "libcamera/internal/device_enumerator.h" >> +#include "libcamera/internal/heap_allocator.h" >> #include "../common/pipeline_base.h" >> #include "../common/rpi_stream.h" >> -#include "dma_heaps.h" >> - >> using namespace std::chrono_literals; >> namespace libcamera { >> @@ -87,7 +86,7 @@ public: >> RPi::Device<Isp, 4> isp_; >> /* DMAHEAP allocation helper. */ >> - RPi::DmaHeap dmaHeap_; >> + HeapAllocator heapAllocator_; >> SharedFD lsTable_; >> struct Config { >> @@ -296,7 +295,7 @@ int >> PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> >> &camer >> { >> Vc4CameraData *data = static_cast<Vc4CameraData >> *>(cameraData.get()); >> - if (!data->dmaHeap_.isValid()) >> + if (!data->heapAllocator_.isValid()) >> return -ENOMEM; >> MediaEntity *unicamImage = >> unicam->getEntityByName("unicam-image"); >> @@ -670,9 +669,9 @@ int >> Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) >> { >> params.ispControls = isp_[Isp::Input].dev()->controls(); >> - /* Allocate the lens shading table via dmaHeap and pass to the >> IPA. */ >> + /* Allocate the lens shading table via heapAllocator and pass to >> the IPA. */ >> if (!lsTable_.isValid()) { >> - lsTable_ = SharedFD(dmaHeap_.alloc("ls_grid", >> ipa::RPi::MaxLsGridSize)); >> + lsTable_ = SharedFD(heapAllocator_.alloc("ls_grid", >> ipa::RPi::MaxLsGridSize)); >> if (!lsTable_.isValid()) >> return -ENOMEM; >
Thanks Umang and Jacopo! On Tue, May 30, 2023 at 2:08 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > Hi Harvey, > > Thank you for the patch. > > On 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote: > > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > > > As other components (like the WIP virtual pipeline handler) also need a > > heap allocator, move DmaHeap from raspberry pi pipeline handler to a > > general HeapAllocator as a base class. > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > .../libcamera/internal/heap_allocator.h | 17 ++- > > include/libcamera/internal/meson.build | 1 + > > src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++ > > src/libcamera/meson.build | 1 + > > src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------ > > src/libcamera/pipeline/rpi/vc4/meson.build | 5 +- > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +- > > 7 files changed, 146 insertions(+), 109 deletions(-) > > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => > include/libcamera/internal/heap_allocator.h (57%) > > create mode 100644 src/libcamera/heap_allocator.cpp > > delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h > b/include/libcamera/internal/heap_allocator.h > > similarity index 57% > > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h > > rename to include/libcamera/internal/heap_allocator.h > > index 0a4a8d86..92d4488a 100644 > > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h > > +++ b/include/libcamera/internal/heap_allocator.h > > @@ -1,8 +1,9 @@ > > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > > /* > > * Copyright (C) 2020, Raspberry Pi Ltd > > + * Copyright (C) 2023, Google Inc. > > * > > - * dma_heaps.h - Helper class for dma-heap allocations. > > + * heap_allocator.h - Helper class for heap buffer allocations. > > */ > > > > #pragma once > > @@ -13,20 +14,18 @@ > > > > namespace libcamera { > > > > -namespace RPi { > > +class Heap; > > > > -class DmaHeap > > +class HeapAllocator > > { > > public: > > - DmaHeap(); > > - ~DmaHeap(); > > - bool isValid() const { return dmaHeapHandle_.isValid(); } > > + HeapAllocator(); > > + ~HeapAllocator(); > > + bool isValid() const; > > UniqueFD alloc(const char *name, std::size_t size); > > > > private: > > - UniqueFD dmaHeapHandle_; > > + std::unique_ptr<Heap> heap_; > > }; > > > > -} /* namespace RPi */ > > - > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/meson.build > b/include/libcamera/internal/meson.build > > index d7508805..9d25c289 100644 > > --- a/include/libcamera/internal/meson.build > > +++ b/include/libcamera/internal/meson.build > > @@ -26,6 +26,7 @@ libcamera_internal_headers = files([ > > 'device_enumerator_udev.h', > > 'formats.h', > > 'framebuffer.h', > > + 'heap_allocator.h', > > 'ipa_manager.h', > > 'ipa_module.h', > > 'ipa_proxy.h', > > diff --git a/src/libcamera/heap_allocator.cpp > b/src/libcamera/heap_allocator.cpp > > new file mode 100644 > > index 00000000..69f65062 > > --- /dev/null > > +++ b/src/libcamera/heap_allocator.cpp > > @@ -0,0 +1,130 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi Ltd > > + * Copyright (C) 2023, Google Inc. > > + * > > + * heap_allocator.cpp - Helper class for heap buffer allocations. > > + */ > > + > > +#include "libcamera/internal/heap_allocator.h" > > + > > +#include <array> > > +#include <fcntl.h> > > +#include <sys/ioctl.h> > > +#include <unistd.h> > > + > > +#include <linux/dma-buf.h> > > +#include <linux/dma-heap.h> > > + > > +#include <libcamera/base/log.h> > > + > > +namespace libcamera { > > + > > +/* > > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows > dmaheap-cma > > + * to only have to worry about importing. > > + * > > + * 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 *, 3> dmaHeapNames = { > > + "/dev/dma_heap/linux,cma", > > + "/dev/dma_heap/reserved", > > + "/dev/dma_heap/system" > > +}; > > + > > +LOG_DEFINE_CATEGORY(HeapAllocator) > > + > > +class Heap > > +{ > > +public: > > + virtual ~Heap() = default; > > + bool isValid() const { return handle_.isValid(); } > > + virtual UniqueFD alloc(const char *name, std::size_t size) = 0; > > + > > +protected: > > + UniqueFD handle_; > > +}; > > + > > +class DmaHeap : public Heap > > +{ > > +public: > > + DmaHeap(); > > + ~DmaHeap(); > > + UniqueFD alloc(const char *name, std::size_t size) override; > > +}; > > + > > +DmaHeap::DmaHeap() > > +{ > > + for (const char *name : dmaHeapNames) { > > + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); > > + if (ret < 0) { > > + ret = errno; > > + LOG(HeapAllocator, Debug) << "DmaHeap failed to > open " << name << ": " > > + << strerror(ret); > > + continue; > > + } > > + > > + handle_ = UniqueFD(ret); > > + break; > > + } > > + > > + if (!handle_.isValid()) > > + LOG(HeapAllocator, Error) << "DmaHeap could not open any > dmaHeap device"; > > +} > > + > > +DmaHeap::~DmaHeap() = default; > > This should be automatically generated no? > + > > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size) > > +{ > > + int ret; > > + > > + if (!name) > > + return {}; > > + > > + struct dma_heap_allocation_data alloc = {}; > > + > > + alloc.len = size; > > + alloc.fd_flags = O_CLOEXEC | O_RDWR; > > + > > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > > + if (ret < 0) { > > + LOG(HeapAllocator, Error) << "DmaHeap allocation failure > for " > > + << name; > > + return {}; > > + } > > + > > + UniqueFD allocFd(alloc.fd); > > + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); > > + if (ret < 0) { > > + LOG(HeapAllocator, Error) << "DmaHeap naming failure for " > > + << name; > > + return {}; > > + } > > + > > + return allocFd; > > +} > > + > > +HeapAllocator::HeapAllocator() > > +{ > > + heap_ = std::make_unique<DmaHeap>(); > > +} > > + > > +HeapAllocator::~HeapAllocator() = default; > > Same here > + > > +bool HeapAllocator::isValid() const > > +{ > > + return heap_->isValid(); > > +} > > + > > +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size) > > +{ > > + if (!isValid()) { > > + LOG(HeapAllocator, Fatal) << "Allocation attempted without > allocator" << name; > > + return {}; > > + } > > + > > + return heap_->alloc(name, size); > > +} > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index d4385041..aa2d32a0 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -22,6 +22,7 @@ libcamera_sources = files([ > > 'framebuffer.cpp', > > 'framebuffer_allocator.cpp', > > 'geometry.cpp', > > + 'heap_allocator.cpp', > > 'ipa_controls.cpp', > > 'ipa_data_serializer.cpp', > > 'ipa_interface.cpp', > > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > > deleted file mode 100644 > > index 317b1fc1..00000000 > > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp > > +++ /dev/null > > @@ -1,90 +0,0 @@ > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > -/* > > - * Copyright (C) 2020, Raspberry Pi Ltd > > - * > > - * dma_heaps.h - Helper class for dma-heap allocations. > > - */ > > - > > -#include "dma_heaps.h" > > - > > -#include <array> > > -#include <fcntl.h> > > -#include <linux/dma-buf.h> > > -#include <linux/dma-heap.h> > > -#include <sys/ioctl.h> > > -#include <unistd.h> > > - > > -#include <libcamera/base/log.h> > > - > > -/* > > - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows > dmaheap-cma > > - * to only have to worry about importing. > > - * > > - * 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" > > -}; > > - > > -namespace libcamera { > > - > > -LOG_DECLARE_CATEGORY(RPI) > > - > > -namespace RPi { > > - > > -DmaHeap::DmaHeap() > > -{ > > - for (const char *name : heapNames) { > > - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); > Removed the third argument based on Jacopo's comment. (I only copied and pasted from the previous implementation though :p) > > - if (ret < 0) { > > - ret = errno; > > - LOG(RPI, Debug) << "Failed to open " << name << ": > " > > - << strerror(ret); > > - continue; > > - } > > - > > - dmaHeapHandle_ = UniqueFD(ret); > > - break; > > - } > > - > > - if (!dmaHeapHandle_.isValid()) > > - LOG(RPI, Error) << "Could not open any dmaHeap device"; > > -} > > - > > -DmaHeap::~DmaHeap() = default; > > - > > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size) > > -{ > > - int ret; > > - > > - if (!name) > > - return {}; > > - > > - struct dma_heap_allocation_data alloc = {}; > > - > > - alloc.len = size; > > - alloc.fd_flags = O_CLOEXEC | O_RDWR; > > - > > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > > - if (ret < 0) { > > - LOG(RPI, Error) << "dmaHeap allocation failure for " > > - << name; > > - return {}; > > - } > > - > > - UniqueFD allocFd(alloc.fd); > > - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); > > - if (ret < 0) { > > - LOG(RPI, Error) << "dmaHeap naming failure for " > > - << name; > > - return {}; > > - } > > - > > - return allocFd; > > -} > > - > > -} /* namespace RPi */ > > - > > -} /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build > b/src/libcamera/pipeline/rpi/vc4/meson.build > > index cdb049c5..b2b79735 100644 > > --- a/src/libcamera/pipeline/rpi/vc4/meson.build > > +++ b/src/libcamera/pipeline/rpi/vc4/meson.build > > @@ -1,8 +1,5 @@ > > # SPDX-License-Identifier: CC0-1.0 > > > > -libcamera_sources += files([ > > - 'dma_heaps.cpp', > > - 'vc4.cpp', > > -]) > > +libcamera_sources += files(['vc4.cpp']) > > > > subdir('data') > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > index 018cf488..410ecfaf 100644 > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > @@ -12,12 +12,11 @@ > > #include <libcamera/formats.h> > > > > #include "libcamera/internal/device_enumerator.h" > > +#include "libcamera/internal/heap_allocator.h" > > > > #include "../common/pipeline_base.h" > > #include "../common/rpi_stream.h" > > > > -#include "dma_heaps.h" > > - > > using namespace std::chrono_literals; > > > > namespace libcamera { > > @@ -87,7 +86,7 @@ public: > > RPi::Device<Isp, 4> isp_; > > > > /* DMAHEAP allocation helper. */ > > - RPi::DmaHeap dmaHeap_; > > + HeapAllocator heapAllocator_; > > SharedFD lsTable_; > > > > struct Config { > > @@ -296,7 +295,7 @@ int > PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > { > > Vc4CameraData *data = static_cast<Vc4CameraData > *>(cameraData.get()); > > > > - if (!data->dmaHeap_.isValid()) > > + if (!data->heapAllocator_.isValid()) > > return -ENOMEM; > > > > MediaEntity *unicamImage = unicam->getEntityByName("unicam-image"); > > @@ -670,9 +669,9 @@ int > Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) > > { > > params.ispControls = isp_[Isp::Input].dev()->controls(); > > > > - /* Allocate the lens shading table via dmaHeap and pass to the > IPA. */ > > + /* Allocate the lens shading table via heapAllocator and pass to > the IPA. */ > > if (!lsTable_.isValid()) { > > - lsTable_ = SharedFD(dmaHeap_.alloc("ls_grid", > ipa::RPi::MaxLsGridSize)); > > + lsTable_ = SharedFD(heapAllocator_.alloc("ls_grid", > ipa::RPi::MaxLsGridSize)); > > if (!lsTable_.isValid()) > > return -ENOMEM; > > > > Updated with some documents. Please feel free to directly modify it when landing. I'm very bad at documenting code... BR, Harvey
diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/heap_allocator.h similarity index 57% rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h rename to include/libcamera/internal/heap_allocator.h index 0a4a8d86..92d4488a 100644 --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h +++ b/include/libcamera/internal/heap_allocator.h @@ -1,8 +1,9 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* * Copyright (C) 2020, Raspberry Pi Ltd + * Copyright (C) 2023, Google Inc. * - * dma_heaps.h - Helper class for dma-heap allocations. + * heap_allocator.h - Helper class for heap buffer allocations. */ #pragma once @@ -13,20 +14,18 @@ namespace libcamera { -namespace RPi { +class Heap; -class DmaHeap +class HeapAllocator { public: - DmaHeap(); - ~DmaHeap(); - bool isValid() const { return dmaHeapHandle_.isValid(); } + HeapAllocator(); + ~HeapAllocator(); + bool isValid() const; UniqueFD alloc(const char *name, std::size_t size); private: - UniqueFD dmaHeapHandle_; + std::unique_ptr<Heap> heap_; }; -} /* namespace RPi */ - } /* namespace libcamera */ diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build index d7508805..9d25c289 100644 --- a/include/libcamera/internal/meson.build +++ b/include/libcamera/internal/meson.build @@ -26,6 +26,7 @@ libcamera_internal_headers = files([ 'device_enumerator_udev.h', 'formats.h', 'framebuffer.h', + 'heap_allocator.h', 'ipa_manager.h', 'ipa_module.h', 'ipa_proxy.h', diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp new file mode 100644 index 00000000..69f65062 --- /dev/null +++ b/src/libcamera/heap_allocator.cpp @@ -0,0 +1,130 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2020, Raspberry Pi Ltd + * Copyright (C) 2023, Google Inc. + * + * heap_allocator.cpp - Helper class for heap buffer allocations. + */ + +#include "libcamera/internal/heap_allocator.h" + +#include <array> +#include <fcntl.h> +#include <sys/ioctl.h> +#include <unistd.h> + +#include <linux/dma-buf.h> +#include <linux/dma-heap.h> + +#include <libcamera/base/log.h> + +namespace libcamera { + +/* + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma + * to only have to worry about importing. + * + * 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 *, 3> dmaHeapNames = { + "/dev/dma_heap/linux,cma", + "/dev/dma_heap/reserved", + "/dev/dma_heap/system" +}; + +LOG_DEFINE_CATEGORY(HeapAllocator) + +class Heap +{ +public: + virtual ~Heap() = default; + bool isValid() const { return handle_.isValid(); } + virtual UniqueFD alloc(const char *name, std::size_t size) = 0; + +protected: + UniqueFD handle_; +}; + +class DmaHeap : public Heap +{ +public: + DmaHeap(); + ~DmaHeap(); + UniqueFD alloc(const char *name, std::size_t size) override; +}; + +DmaHeap::DmaHeap() +{ + for (const char *name : dmaHeapNames) { + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); + if (ret < 0) { + ret = errno; + LOG(HeapAllocator, Debug) << "DmaHeap failed to open " << name << ": " + << strerror(ret); + continue; + } + + handle_ = UniqueFD(ret); + break; + } + + if (!handle_.isValid()) + LOG(HeapAllocator, Error) << "DmaHeap could not open any dmaHeap device"; +} + +DmaHeap::~DmaHeap() = default; + +UniqueFD DmaHeap::alloc(const char *name, std::size_t size) +{ + int ret; + + if (!name) + return {}; + + struct dma_heap_allocation_data alloc = {}; + + alloc.len = size; + alloc.fd_flags = O_CLOEXEC | O_RDWR; + + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); + if (ret < 0) { + LOG(HeapAllocator, Error) << "DmaHeap allocation failure for " + << name; + return {}; + } + + UniqueFD allocFd(alloc.fd); + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); + if (ret < 0) { + LOG(HeapAllocator, Error) << "DmaHeap naming failure for " + << name; + return {}; + } + + return allocFd; +} + +HeapAllocator::HeapAllocator() +{ + heap_ = std::make_unique<DmaHeap>(); +} + +HeapAllocator::~HeapAllocator() = default; + +bool HeapAllocator::isValid() const +{ + return heap_->isValid(); +} + +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size) +{ + if (!isValid()) { + LOG(HeapAllocator, Fatal) << "Allocation attempted without allocator" << name; + return {}; + } + + return heap_->alloc(name, size); +} + +} /* namespace libcamera */ diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index d4385041..aa2d32a0 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -22,6 +22,7 @@ libcamera_sources = files([ 'framebuffer.cpp', 'framebuffer_allocator.cpp', 'geometry.cpp', + 'heap_allocator.cpp', 'ipa_controls.cpp', 'ipa_data_serializer.cpp', 'ipa_interface.cpp', diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp deleted file mode 100644 index 317b1fc1..00000000 --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp +++ /dev/null @@ -1,90 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Copyright (C) 2020, Raspberry Pi Ltd - * - * dma_heaps.h - Helper class for dma-heap allocations. - */ - -#include "dma_heaps.h" - -#include <array> -#include <fcntl.h> -#include <linux/dma-buf.h> -#include <linux/dma-heap.h> -#include <sys/ioctl.h> -#include <unistd.h> - -#include <libcamera/base/log.h> - -/* - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma - * to only have to worry about importing. - * - * 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" -}; - -namespace libcamera { - -LOG_DECLARE_CATEGORY(RPI) - -namespace RPi { - -DmaHeap::DmaHeap() -{ - for (const char *name : heapNames) { - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0); - if (ret < 0) { - ret = errno; - LOG(RPI, Debug) << "Failed to open " << name << ": " - << strerror(ret); - continue; - } - - dmaHeapHandle_ = UniqueFD(ret); - break; - } - - if (!dmaHeapHandle_.isValid()) - LOG(RPI, Error) << "Could not open any dmaHeap device"; -} - -DmaHeap::~DmaHeap() = default; - -UniqueFD DmaHeap::alloc(const char *name, std::size_t size) -{ - int ret; - - if (!name) - return {}; - - struct dma_heap_allocation_data alloc = {}; - - alloc.len = size; - alloc.fd_flags = O_CLOEXEC | O_RDWR; - - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); - if (ret < 0) { - LOG(RPI, Error) << "dmaHeap allocation failure for " - << name; - return {}; - } - - UniqueFD allocFd(alloc.fd); - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name); - if (ret < 0) { - LOG(RPI, Error) << "dmaHeap naming failure for " - << name; - return {}; - } - - return allocFd; -} - -} /* namespace RPi */ - -} /* namespace libcamera */ diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build index cdb049c5..b2b79735 100644 --- a/src/libcamera/pipeline/rpi/vc4/meson.build +++ b/src/libcamera/pipeline/rpi/vc4/meson.build @@ -1,8 +1,5 @@ # SPDX-License-Identifier: CC0-1.0 -libcamera_sources += files([ - 'dma_heaps.cpp', - 'vc4.cpp', -]) +libcamera_sources += files(['vc4.cpp']) subdir('data') diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp index 018cf488..410ecfaf 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -12,12 +12,11 @@ #include <libcamera/formats.h> #include "libcamera/internal/device_enumerator.h" +#include "libcamera/internal/heap_allocator.h" #include "../common/pipeline_base.h" #include "../common/rpi_stream.h" -#include "dma_heaps.h" - using namespace std::chrono_literals; namespace libcamera { @@ -87,7 +86,7 @@ public: RPi::Device<Isp, 4> isp_; /* DMAHEAP allocation helper. */ - RPi::DmaHeap dmaHeap_; + HeapAllocator heapAllocator_; SharedFD lsTable_; struct Config { @@ -296,7 +295,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer { Vc4CameraData *data = static_cast<Vc4CameraData *>(cameraData.get()); - if (!data->dmaHeap_.isValid()) + if (!data->heapAllocator_.isValid()) return -ENOMEM; MediaEntity *unicamImage = unicam->getEntityByName("unicam-image"); @@ -670,9 +669,9 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) { params.ispControls = isp_[Isp::Input].dev()->controls(); - /* Allocate the lens shading table via dmaHeap and pass to the IPA. */ + /* Allocate the lens shading table via heapAllocator and pass to the IPA. */ if (!lsTable_.isValid()) { - lsTable_ = SharedFD(dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize)); + lsTable_ = SharedFD(heapAllocator_.alloc("ls_grid", ipa::RPi::MaxLsGridSize)); if (!lsTable_.isValid()) return -ENOMEM;