| Message ID | 20230328095534.3584-2-harveyycyang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Tue, Mar 28, 2023 at 05:55:32PM +0800, 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> A few questions on the design before getting to the actual implementation > --- > include/libcamera/dma_heap.h | 20 +++++++++ > include/libcamera/heap.h | 27 ++++++++++++ > include/libcamera/heap_allocator.h | 30 +++++++++++++ Is the HeapAllocator meant to be used by applications or is it only for internal library components usage ? If it's only for internal usage, should these headers be moved to include/libcamera/internal/ ? > include/libcamera/meson.build | 3 ++ > .../dma_heaps.cpp => dma_heap.cpp} | 35 +++++++-------- > src/libcamera/heap_allocator.cpp | 43 +++++++++++++++++++ > src/libcamera/meson.build | 2 + > .../pipeline/raspberrypi/dma_heaps.h | 32 -------------- > .../pipeline/raspberrypi/meson.build | 1 - > .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++--- > 10 files changed, 146 insertions(+), 57 deletions(-) > create mode 100644 include/libcamera/dma_heap.h > create mode 100644 include/libcamera/heap.h > create mode 100644 include/libcamera/heap_allocator.h > rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp => dma_heap.cpp} (69%) > create mode 100644 src/libcamera/heap_allocator.cpp > delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h > > diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h > new file mode 100644 > index 00000000..a663c317 > --- /dev/null > +++ b/include/libcamera/dma_heap.h If I'm not mistaken the DmaHeap and UdmaHeap classes are not meant to be used directly, they always go through the HeapAllocator class. Do we need three separate headers for: 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; }; class UdmaHeap : public Heap { public: UdmaHeap(); ~UdmaHeap(); UniqueFD alloc(const char *name, std::size_t size) override; }; I wonder if we even need to define an header for the Heap and derived subclasses at all or they could be defined inside the heap_allocator.cpp file (assuming no other component is meant to use them). > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2020, Raspberry Pi Ltd > + * > + * dma_heap.h - Dma Heap implementation. > + */ > + > +#include <libcamera/heap.h> > + > +namespace libcamera { > + > +class DmaHeap : public Heap > +{ > +public: > + DmaHeap(); > + ~DmaHeap(); > + UniqueFD alloc(const char *name, std::size_t size) override; > +}; > + > +} /* namespace libcamera */ > diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h > new file mode 100644 > index 00000000..c49a2ac3 > --- /dev/null > +++ b/include/libcamera/heap.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2023, Google Inc. > + * > + * heap.h - Heap interface. > + */ > + > +#pragma once > + > +#include <stddef.h> > + > +#include <libcamera/base/unique_fd.h> > + > +namespace libcamera { > + > +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_; > +}; > + > +} /* namespace libcamera */ > diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h > new file mode 100644 > index 00000000..cd7ed1a3 > --- /dev/null > +++ b/include/libcamera/heap_allocator.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2023, Google Inc. > + * > + * heap_allocator.h - Helper class for heap buffer allocations. > + */ > + > +#pragma once > + > +#include <stddef.h> > + > +#include <libcamera/base/unique_fd.h> > + > +#include <libcamera/heap.h> > + > +namespace libcamera { > + > +class HeapAllocator > +{ > +public: > + HeapAllocator(); > + ~HeapAllocator(); > + bool isValid() const { return heap_->isValid(); } > + UniqueFD alloc(const char *name, std::size_t size); > + > +private: > + std::unique_ptr<Heap> heap_; > +}; > + > +} /* namespace libcamera */ > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > index 408b7acf..f486630a 100644 > --- a/include/libcamera/meson.build > +++ b/include/libcamera/meson.build > @@ -7,10 +7,13 @@ libcamera_public_headers = files([ > 'camera_manager.h', > 'color_space.h', > 'controls.h', > + 'dma_heap.h', > 'fence.h', > 'framebuffer.h', > 'framebuffer_allocator.h', > 'geometry.h', > + 'heap.h', > + 'heap_allocator.h', > 'logging.h', > 'pixel_format.h', > 'request.h', > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/dma_heap.cpp > similarity index 69% > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > rename to src/libcamera/dma_heap.cpp > index 6b644406..02415975 100644 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > +++ b/src/libcamera/dma_heap.cpp > @@ -2,18 +2,19 @@ > /* > * Copyright (C) 2020, Raspberry Pi Ltd > * > - * dma_heaps.h - Helper class for dma-heap allocations. > + * dma_heap.h - Dma Heap implementation. > */ > > -#include "dma_heaps.h" > +#include <libcamera/dma_heap.h> > > #include <array> > #include <fcntl.h> > -#include <linux/dma-buf.h> > -#include <linux/dma-heap.h> > #include <sys/ioctl.h> > #include <unistd.h> > > +#include <linux/dma-buf.h> > +#include <linux/dma-heap.h> > + > #include <libcamera/base/log.h> > > /* > @@ -30,9 +31,7 @@ static constexpr std::array<const char *, 2> heapNames = { > > namespace libcamera { > > -LOG_DECLARE_CATEGORY(RPI) > - > -namespace RPi { > +LOG_DEFINE_CATEGORY(DmaHeap) > > DmaHeap::DmaHeap() > { > @@ -40,17 +39,17 @@ DmaHeap::DmaHeap() > int ret = ::open(name, O_RDWR, 0); > if (ret < 0) { > ret = errno; > - LOG(RPI, Debug) << "Failed to open " << name << ": " > - << strerror(ret); > + LOG(DmaHeap, Debug) << "Failed to open " << name << ": " > + << strerror(ret); > continue; > } > > - dmaHeapHandle_ = UniqueFD(ret); > + handle_ = UniqueFD(ret); > break; > } > > - if (!dmaHeapHandle_.isValid()) > - LOG(RPI, Error) << "Could not open any dmaHeap device"; > + if (!handle_.isValid()) > + LOG(DmaHeap, Error) << "Could not open any dmaHeap device"; > } > > DmaHeap::~DmaHeap() = default; > @@ -67,24 +66,22 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size) > alloc.len = size; > alloc.fd_flags = O_CLOEXEC | O_RDWR; > > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > if (ret < 0) { > - LOG(RPI, Error) << "dmaHeap allocation failure for " > - << name; > + LOG(DmaHeap, 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; > + LOG(DmaHeap, Error) << "dmaHeap naming failure for " > + << name; > return {}; > } > > return allocFd; > } > > -} /* namespace RPi */ > - > } /* namespace libcamera */ > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp > new file mode 100644 > index 00000000..594b1d6a > --- /dev/null > +++ b/src/libcamera/heap_allocator.cpp > @@ -0,0 +1,43 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2023, Google Inc. > + * > + * heap_allocator.cpp - Helper class for heap buffer allocations. > + */ > + > +#include <libcamera/heap_allocator.h> > + > +#include <array> > +#include <fcntl.h> > +#include <sys/ioctl.h> Are these headers used ? > +#include <unistd.h> > + > +#include <linux/dma-buf.h> > +#include <linux/dma-heap.h> > + > +#include <libcamera/base/log.h> > + > +#include <libcamera/dma_heap.h> > + > +namespace libcamera { > + > +LOG_DEFINE_CATEGORY(HeapAllocator) > + > +HeapAllocator::HeapAllocator() > +{ > + heap_ = std::make_unique<DmaHeap>(); > +} > + > +HeapAllocator::~HeapAllocator() = default; > + > +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 9869bfe7..ee586c0d 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -17,11 +17,13 @@ libcamera_sources = files([ > 'delayed_controls.cpp', > 'device_enumerator.cpp', > 'device_enumerator_sysfs.cpp', > + 'dma_heap.cpp', > 'fence.cpp', > 'formats.cpp', > '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/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > deleted file mode 100644 > index 0a4a8d86..00000000 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h > +++ /dev/null > @@ -1,32 +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. > - */ > - > -#pragma once > - > -#include <stddef.h> > - > -#include <libcamera/base/unique_fd.h> > - > -namespace libcamera { > - > -namespace RPi { > - > -class DmaHeap > -{ > -public: > - DmaHeap(); > - ~DmaHeap(); > - bool isValid() const { return dmaHeapHandle_.isValid(); } > - UniqueFD alloc(const char *name, std::size_t size); > - > -private: > - UniqueFD dmaHeapHandle_; > -}; > - > -} /* namespace RPi */ > - > -} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build > index 59e8fb18..7322f0e8 100644 > --- a/src/libcamera/pipeline/raspberrypi/meson.build > +++ b/src/libcamera/pipeline/raspberrypi/meson.build > @@ -2,7 +2,6 @@ > > libcamera_sources += files([ > 'delayed_controls.cpp', > - 'dma_heaps.cpp', > 'raspberrypi.cpp', > 'rpi_stream.cpp', > ]) > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 00600441..198dcc9d 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -21,6 +21,7 @@ > #include <libcamera/camera.h> > #include <libcamera/control_ids.h> > #include <libcamera/formats.h> > +#include <libcamera/heap_allocator.h> > #include <libcamera/ipa/raspberrypi_ipa_interface.h> > #include <libcamera/ipa/raspberrypi_ipa_proxy.h> > #include <libcamera/logging.h> > @@ -44,7 +45,6 @@ > #include "libcamera/internal/yaml_parser.h" > > #include "delayed_controls.h" > -#include "dma_heaps.h" > #include "rpi_stream.h" > > using namespace std::chrono_literals; > @@ -246,7 +246,7 @@ public: > std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, MediaLink *>> bridgeDevices_; > > /* DMAHEAP allocation helper. */ > - RPi::DmaHeap dmaHeap_; > + HeapAllocator heapAllocator_; > SharedFD lsTable_; > > std::unique_ptr<RPi::DelayedControls> delayedCtrls_; > @@ -1304,7 +1304,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > { > std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this); > > - if (!data->dmaHeap_.isValid()) > + if (!data->heapAllocator_.isValid()) > return -ENOMEM; > > MediaEntity *unicamImage = unicam->getEntityByName("unicam-image"); > @@ -1692,9 +1692,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA > /* Always send the user transform to the IPA. */ > ipaConfig.transform = static_cast<unsigned int>(config->transform); > > - /* 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.0 >
Thanks Jacopo for the review! On Sun, May 14, 2023 at 12:12 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Harvey > > On Tue, Mar 28, 2023 at 05:55:32PM +0800, 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> > > A few questions on the design before getting to the actual > implementation > > > --- > > include/libcamera/dma_heap.h | 20 +++++++++ > > include/libcamera/heap.h | 27 ++++++++++++ > > include/libcamera/heap_allocator.h | 30 +++++++++++++ > > Is the HeapAllocator meant to be used by applications or is it only > for internal library components usage ? If it's only for internal > usage, should these headers be moved to include/libcamera/internal/ ? > > I see. Thanks for pointing it out. Updated. Only one concern: the copyright. I'll mark |heap_allocator.cpp| belonging to "Raspberry Pi Ltd" then. Please check if it works. > include/libcamera/meson.build | 3 ++ > > .../dma_heaps.cpp => dma_heap.cpp} | 35 +++++++-------- > > src/libcamera/heap_allocator.cpp | 43 +++++++++++++++++++ > > src/libcamera/meson.build | 2 + > > .../pipeline/raspberrypi/dma_heaps.h | 32 -------------- > > .../pipeline/raspberrypi/meson.build | 1 - > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++--- > > 10 files changed, 146 insertions(+), 57 deletions(-) > > create mode 100644 include/libcamera/dma_heap.h > > create mode 100644 include/libcamera/heap.h > > create mode 100644 include/libcamera/heap_allocator.h > > rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp => > dma_heap.cpp} (69%) > > create mode 100644 src/libcamera/heap_allocator.cpp > > delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h > > > > diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h > > new file mode 100644 > > index 00000000..a663c317 > > --- /dev/null > > +++ b/include/libcamera/dma_heap.h > > If I'm not mistaken the DmaHeap and UdmaHeap classes are not meant to be > used > directly, they always go through the HeapAllocator class. Do we need > three separate headers for: > > 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; > }; > > class UdmaHeap : public Heap > { > public: > UdmaHeap(); > ~UdmaHeap(); > UniqueFD alloc(const char *name, std::size_t size) override; > }; > > I wonder if we even need to define an header for the Heap and derived > subclasses at all or they could be defined inside the > heap_allocator.cpp file (assuming no other component is meant to use > them). > > Right. Moved the classes to heap_allocator.cpp. > > > @@ -0,0 +1,20 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi Ltd > > + * > > + * dma_heap.h - Dma Heap implementation. > > + */ > > + > > +#include <libcamera/heap.h> > > + > > +namespace libcamera { > > + > > +class DmaHeap : public Heap > > +{ > > +public: > > + DmaHeap(); > > + ~DmaHeap(); > > + UniqueFD alloc(const char *name, std::size_t size) override; > > +}; > > + > > +} /* namespace libcamera */ > > diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h > > new file mode 100644 > > index 00000000..c49a2ac3 > > --- /dev/null > > +++ b/include/libcamera/heap.h > > @@ -0,0 +1,27 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * heap.h - Heap interface. > > + */ > > + > > +#pragma once > > + > > +#include <stddef.h> > > + > > +#include <libcamera/base/unique_fd.h> > > + > > +namespace libcamera { > > + > > +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_; > > +}; > > + > > +} /* namespace libcamera */ > > diff --git a/include/libcamera/heap_allocator.h > b/include/libcamera/heap_allocator.h > > new file mode 100644 > > index 00000000..cd7ed1a3 > > --- /dev/null > > +++ b/include/libcamera/heap_allocator.h > > @@ -0,0 +1,30 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * heap_allocator.h - Helper class for heap buffer allocations. > > + */ > > + > > +#pragma once > > + > > +#include <stddef.h> > > + > > +#include <libcamera/base/unique_fd.h> > > + > > +#include <libcamera/heap.h> > > + > > +namespace libcamera { > > + > > +class HeapAllocator > > +{ > > +public: > > + HeapAllocator(); > > + ~HeapAllocator(); > > + bool isValid() const { return heap_->isValid(); } > > + UniqueFD alloc(const char *name, std::size_t size); > > + > > +private: > > + std::unique_ptr<Heap> heap_; > > +}; > > + > > +} /* namespace libcamera */ > > diff --git a/include/libcamera/meson.build > b/include/libcamera/meson.build > > index 408b7acf..f486630a 100644 > > --- a/include/libcamera/meson.build > > +++ b/include/libcamera/meson.build > > @@ -7,10 +7,13 @@ libcamera_public_headers = files([ > > 'camera_manager.h', > > 'color_space.h', > > 'controls.h', > > + 'dma_heap.h', > > 'fence.h', > > 'framebuffer.h', > > 'framebuffer_allocator.h', > > 'geometry.h', > > + 'heap.h', > > + 'heap_allocator.h', > > 'logging.h', > > 'pixel_format.h', > > 'request.h', > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > b/src/libcamera/dma_heap.cpp > > similarity index 69% > > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > > rename to src/libcamera/dma_heap.cpp > > index 6b644406..02415975 100644 > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > > +++ b/src/libcamera/dma_heap.cpp > > @@ -2,18 +2,19 @@ > > /* > > * Copyright (C) 2020, Raspberry Pi Ltd > > * > > - * dma_heaps.h - Helper class for dma-heap allocations. > > + * dma_heap.h - Dma Heap implementation. > > */ > > > > -#include "dma_heaps.h" > > +#include <libcamera/dma_heap.h> > > > > #include <array> > > #include <fcntl.h> > > -#include <linux/dma-buf.h> > > -#include <linux/dma-heap.h> > > #include <sys/ioctl.h> > > #include <unistd.h> > > > > +#include <linux/dma-buf.h> > > +#include <linux/dma-heap.h> > > + > > #include <libcamera/base/log.h> > > > > /* > > @@ -30,9 +31,7 @@ static constexpr std::array<const char *, 2> heapNames > = { > > > > namespace libcamera { > > > > -LOG_DECLARE_CATEGORY(RPI) > > - > > -namespace RPi { > > +LOG_DEFINE_CATEGORY(DmaHeap) > > > > DmaHeap::DmaHeap() > > { > > @@ -40,17 +39,17 @@ DmaHeap::DmaHeap() > > int ret = ::open(name, O_RDWR, 0); > > if (ret < 0) { > > ret = errno; > > - LOG(RPI, Debug) << "Failed to open " << name << ": > " > > - << strerror(ret); > > + LOG(DmaHeap, Debug) << "Failed to open " << name > << ": " > > + << strerror(ret); > > continue; > > } > > > > - dmaHeapHandle_ = UniqueFD(ret); > > + handle_ = UniqueFD(ret); > > break; > > } > > > > - if (!dmaHeapHandle_.isValid()) > > - LOG(RPI, Error) << "Could not open any dmaHeap device"; > > + if (!handle_.isValid()) > > + LOG(DmaHeap, Error) << "Could not open any dmaHeap device"; > > } > > > > DmaHeap::~DmaHeap() = default; > > @@ -67,24 +66,22 @@ UniqueFD DmaHeap::alloc(const char *name, > std::size_t size) > > alloc.len = size; > > alloc.fd_flags = O_CLOEXEC | O_RDWR; > > > > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); > > if (ret < 0) { > > - LOG(RPI, Error) << "dmaHeap allocation failure for " > > - << name; > > + LOG(DmaHeap, 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; > > + LOG(DmaHeap, Error) << "dmaHeap naming failure for " > > + << name; > > return {}; > > } > > > > return allocFd; > > } > > > > -} /* namespace RPi */ > > - > > } /* namespace libcamera */ > > diff --git a/src/libcamera/heap_allocator.cpp > b/src/libcamera/heap_allocator.cpp > > new file mode 100644 > > index 00000000..594b1d6a > > --- /dev/null > > +++ b/src/libcamera/heap_allocator.cpp > > @@ -0,0 +1,43 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2023, Google Inc. > > + * > > + * heap_allocator.cpp - Helper class for heap buffer allocations. > > + */ > > + > > +#include <libcamera/heap_allocator.h> > > + > > +#include <array> > > +#include <fcntl.h> > > +#include <sys/ioctl.h> > > Are these headers used ? > > You're right, while now that the heap implementations are moved into this file, yes they're used :) > > +#include <unistd.h> > > + > > +#include <linux/dma-buf.h> > > +#include <linux/dma-heap.h> > > + > > +#include <libcamera/base/log.h> > > + > > +#include <libcamera/dma_heap.h> > > + > > +namespace libcamera { > > + > > +LOG_DEFINE_CATEGORY(HeapAllocator) > > + > > +HeapAllocator::HeapAllocator() > > +{ > > + heap_ = std::make_unique<DmaHeap>(); > > +} > > + > > +HeapAllocator::~HeapAllocator() = default; > > + > > +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 9869bfe7..ee586c0d 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -17,11 +17,13 @@ libcamera_sources = files([ > > 'delayed_controls.cpp', > > 'device_enumerator.cpp', > > 'device_enumerator_sysfs.cpp', > > + 'dma_heap.cpp', > > 'fence.cpp', > > 'formats.cpp', > > '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/raspberrypi/dma_heaps.h > b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > > deleted file mode 100644 > > index 0a4a8d86..00000000 > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h > > +++ /dev/null > > @@ -1,32 +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. > > - */ > > - > > -#pragma once > > - > > -#include <stddef.h> > > - > > -#include <libcamera/base/unique_fd.h> > > - > > -namespace libcamera { > > - > > -namespace RPi { > > - > > -class DmaHeap > > -{ > > -public: > > - DmaHeap(); > > - ~DmaHeap(); > > - bool isValid() const { return dmaHeapHandle_.isValid(); } > > - UniqueFD alloc(const char *name, std::size_t size); > > - > > -private: > > - UniqueFD dmaHeapHandle_; > > -}; > > - > > -} /* namespace RPi */ > > - > > -} /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build > b/src/libcamera/pipeline/raspberrypi/meson.build > > index 59e8fb18..7322f0e8 100644 > > --- a/src/libcamera/pipeline/raspberrypi/meson.build > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build > > @@ -2,7 +2,6 @@ > > > > libcamera_sources += files([ > > 'delayed_controls.cpp', > > - 'dma_heaps.cpp', > > 'raspberrypi.cpp', > > 'rpi_stream.cpp', > > ]) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 00600441..198dcc9d 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -21,6 +21,7 @@ > > #include <libcamera/camera.h> > > #include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > +#include <libcamera/heap_allocator.h> > > #include <libcamera/ipa/raspberrypi_ipa_interface.h> > > #include <libcamera/ipa/raspberrypi_ipa_proxy.h> > > #include <libcamera/logging.h> > > @@ -44,7 +45,6 @@ > > #include "libcamera/internal/yaml_parser.h" > > > > #include "delayed_controls.h" > > -#include "dma_heaps.h" > > #include "rpi_stream.h" > > > > using namespace std::chrono_literals; > > @@ -246,7 +246,7 @@ public: > > std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, MediaLink > *>> bridgeDevices_; > > > > /* DMAHEAP allocation helper. */ > > - RPi::DmaHeap dmaHeap_; > > + HeapAllocator heapAllocator_; > > SharedFD lsTable_; > > > > std::unique_ptr<RPi::DelayedControls> delayedCtrls_; > > @@ -1304,7 +1304,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice > *unicam, MediaDevice *isp, Me > > { > > std::unique_ptr<RPiCameraData> data = > std::make_unique<RPiCameraData>(this); > > > > - if (!data->dmaHeap_.isValid()) > > + if (!data->heapAllocator_.isValid()) > > return -ENOMEM; > > > > MediaEntity *unicamImage = unicam->getEntityByName("unicam-image"); > > @@ -1692,9 +1692,9 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config, ipa::RPi::IPA > > /* Always send the user transform to the IPA. */ > > ipaConfig.transform = static_cast<unsigned int>(config->transform); > > > > - /* 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.0 > > >
diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h new file mode 100644 index 00000000..a663c317 --- /dev/null +++ b/include/libcamera/dma_heap.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2020, Raspberry Pi Ltd + * + * dma_heap.h - Dma Heap implementation. + */ + +#include <libcamera/heap.h> + +namespace libcamera { + +class DmaHeap : public Heap +{ +public: + DmaHeap(); + ~DmaHeap(); + UniqueFD alloc(const char *name, std::size_t size) override; +}; + +} /* namespace libcamera */ diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h new file mode 100644 index 00000000..c49a2ac3 --- /dev/null +++ b/include/libcamera/heap.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Google Inc. + * + * heap.h - Heap interface. + */ + +#pragma once + +#include <stddef.h> + +#include <libcamera/base/unique_fd.h> + +namespace libcamera { + +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_; +}; + +} /* namespace libcamera */ diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h new file mode 100644 index 00000000..cd7ed1a3 --- /dev/null +++ b/include/libcamera/heap_allocator.h @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Google Inc. + * + * heap_allocator.h - Helper class for heap buffer allocations. + */ + +#pragma once + +#include <stddef.h> + +#include <libcamera/base/unique_fd.h> + +#include <libcamera/heap.h> + +namespace libcamera { + +class HeapAllocator +{ +public: + HeapAllocator(); + ~HeapAllocator(); + bool isValid() const { return heap_->isValid(); } + UniqueFD alloc(const char *name, std::size_t size); + +private: + std::unique_ptr<Heap> heap_; +}; + +} /* namespace libcamera */ diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build index 408b7acf..f486630a 100644 --- a/include/libcamera/meson.build +++ b/include/libcamera/meson.build @@ -7,10 +7,13 @@ libcamera_public_headers = files([ 'camera_manager.h', 'color_space.h', 'controls.h', + 'dma_heap.h', 'fence.h', 'framebuffer.h', 'framebuffer_allocator.h', 'geometry.h', + 'heap.h', + 'heap_allocator.h', 'logging.h', 'pixel_format.h', 'request.h', diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/dma_heap.cpp similarity index 69% rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp rename to src/libcamera/dma_heap.cpp index 6b644406..02415975 100644 --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp +++ b/src/libcamera/dma_heap.cpp @@ -2,18 +2,19 @@ /* * Copyright (C) 2020, Raspberry Pi Ltd * - * dma_heaps.h - Helper class for dma-heap allocations. + * dma_heap.h - Dma Heap implementation. */ -#include "dma_heaps.h" +#include <libcamera/dma_heap.h> #include <array> #include <fcntl.h> -#include <linux/dma-buf.h> -#include <linux/dma-heap.h> #include <sys/ioctl.h> #include <unistd.h> +#include <linux/dma-buf.h> +#include <linux/dma-heap.h> + #include <libcamera/base/log.h> /* @@ -30,9 +31,7 @@ static constexpr std::array<const char *, 2> heapNames = { namespace libcamera { -LOG_DECLARE_CATEGORY(RPI) - -namespace RPi { +LOG_DEFINE_CATEGORY(DmaHeap) DmaHeap::DmaHeap() { @@ -40,17 +39,17 @@ DmaHeap::DmaHeap() int ret = ::open(name, O_RDWR, 0); if (ret < 0) { ret = errno; - LOG(RPI, Debug) << "Failed to open " << name << ": " - << strerror(ret); + LOG(DmaHeap, Debug) << "Failed to open " << name << ": " + << strerror(ret); continue; } - dmaHeapHandle_ = UniqueFD(ret); + handle_ = UniqueFD(ret); break; } - if (!dmaHeapHandle_.isValid()) - LOG(RPI, Error) << "Could not open any dmaHeap device"; + if (!handle_.isValid()) + LOG(DmaHeap, Error) << "Could not open any dmaHeap device"; } DmaHeap::~DmaHeap() = default; @@ -67,24 +66,22 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size) alloc.len = size; alloc.fd_flags = O_CLOEXEC | O_RDWR; - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc); if (ret < 0) { - LOG(RPI, Error) << "dmaHeap allocation failure for " - << name; + LOG(DmaHeap, 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; + LOG(DmaHeap, Error) << "dmaHeap naming failure for " + << name; return {}; } return allocFd; } -} /* namespace RPi */ - } /* namespace libcamera */ diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp new file mode 100644 index 00000000..594b1d6a --- /dev/null +++ b/src/libcamera/heap_allocator.cpp @@ -0,0 +1,43 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Google Inc. + * + * heap_allocator.cpp - Helper class for heap buffer allocations. + */ + +#include <libcamera/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> + +#include <libcamera/dma_heap.h> + +namespace libcamera { + +LOG_DEFINE_CATEGORY(HeapAllocator) + +HeapAllocator::HeapAllocator() +{ + heap_ = std::make_unique<DmaHeap>(); +} + +HeapAllocator::~HeapAllocator() = default; + +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 9869bfe7..ee586c0d 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -17,11 +17,13 @@ libcamera_sources = files([ 'delayed_controls.cpp', 'device_enumerator.cpp', 'device_enumerator_sysfs.cpp', + 'dma_heap.cpp', 'fence.cpp', 'formats.cpp', '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/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h deleted file mode 100644 index 0a4a8d86..00000000 --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h +++ /dev/null @@ -1,32 +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. - */ - -#pragma once - -#include <stddef.h> - -#include <libcamera/base/unique_fd.h> - -namespace libcamera { - -namespace RPi { - -class DmaHeap -{ -public: - DmaHeap(); - ~DmaHeap(); - bool isValid() const { return dmaHeapHandle_.isValid(); } - UniqueFD alloc(const char *name, std::size_t size); - -private: - UniqueFD dmaHeapHandle_; -}; - -} /* namespace RPi */ - -} /* namespace libcamera */ diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build index 59e8fb18..7322f0e8 100644 --- a/src/libcamera/pipeline/raspberrypi/meson.build +++ b/src/libcamera/pipeline/raspberrypi/meson.build @@ -2,7 +2,6 @@ libcamera_sources += files([ 'delayed_controls.cpp', - 'dma_heaps.cpp', 'raspberrypi.cpp', 'rpi_stream.cpp', ]) diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 00600441..198dcc9d 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -21,6 +21,7 @@ #include <libcamera/camera.h> #include <libcamera/control_ids.h> #include <libcamera/formats.h> +#include <libcamera/heap_allocator.h> #include <libcamera/ipa/raspberrypi_ipa_interface.h> #include <libcamera/ipa/raspberrypi_ipa_proxy.h> #include <libcamera/logging.h> @@ -44,7 +45,6 @@ #include "libcamera/internal/yaml_parser.h" #include "delayed_controls.h" -#include "dma_heaps.h" #include "rpi_stream.h" using namespace std::chrono_literals; @@ -246,7 +246,7 @@ public: std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, MediaLink *>> bridgeDevices_; /* DMAHEAP allocation helper. */ - RPi::DmaHeap dmaHeap_; + HeapAllocator heapAllocator_; SharedFD lsTable_; std::unique_ptr<RPi::DelayedControls> delayedCtrls_; @@ -1304,7 +1304,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me { std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this); - if (!data->dmaHeap_.isValid()) + if (!data->heapAllocator_.isValid()) return -ENOMEM; MediaEntity *unicamImage = unicam->getEntityByName("unicam-image"); @@ -1692,9 +1692,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA /* Always send the user transform to the IPA. */ ipaConfig.transform = static_cast<unsigned int>(config->transform); - /* 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;