[libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to HeapAllocator as a base class
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue May 16 13:27:01 CEST 2023
Quoting Jacopo Mondi via libcamera-devel (2023-05-16 11:24:33)
> Hi Harvey
> On Tue, May 16, 2023 at 08:03:17AM +0000, Harvey Yang via libcamera-devel wrote:
> > From: Cheng-Hao Yang <chenghaoyang at 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 at chromium.org>
> > ---
> > .../libcamera/internal/heap_allocator.h | 18 ++-
> > include/libcamera/internal/meson.build | 1 +
> > src/libcamera/heap_allocator.cpp | 128 ++++++++++++++++++
> > src/libcamera/meson.build | 1 +
> > src/libcamera/pipeline/meson.build | 5 +-
> > 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 +-
> > 8 files changed, 145 insertions(+), 114 deletions(-)
> > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (50%)
> > 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 50%
> > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> > rename to include/libcamera/internal/heap_allocator.h
> > index 0a4a8d86..d061fdce 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> > +++ b/include/libcamera/internal/heap_allocator.h
> > @@ -1,8 +1,8 @@
> > /* SPDX-License-Identifier: LGPL-2.1-or-later */
> > /*
> > - * Copyright (C) 2020, Raspberry Pi Ltd
> > + * Copyright (C) 2023, Google Inc.
>
> Why can't you keep both ? If the code comes from RPi they copyright
> should be retained.
I'm sure we've been here before ;-)
Indeed, please don't drop copyrights when moving code around.
>
> > *
> > - * dma_heaps.h - Helper class for dma-heap allocations.
> > + * heap_allocator.h - Helper class for heap buffer allocations.
> > */
> >
> > #pragma once
> > @@ -13,20 +13,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..e9476de5
> > --- /dev/null
> > +++ b/src/libcamera/heap_allocator.cpp
> > @@ -0,0 +1,128 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi Ltd
> > + *
> > + * 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 *, 2> dmaHeapNames = {
> > + "/dev/dma_heap/linux,cma",
> > + "/dev/dma_heap/reserved"
On my x86 pc, I have "/dev/dma_heap/system" too. I don't know if we
shoudl add that ... but maybe something to explore later.
This is fine to keep as it is though here I think.
> > +};
> > +
> > +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/meson.build b/src/libcamera/pipeline/meson.build
> > index 8a61991c..b6160d34 100644
> > --- a/src/libcamera/pipeline/meson.build
> > +++ b/src/libcamera/pipeline/meson.build
> > @@ -12,9 +12,6 @@ foreach pipeline : pipelines
> > continue
> > endif
> >
> > - subdirs += pipeline
> > subdir(pipeline)
> > -
> > - # Don't reuse the pipeline variable below, the subdirectory may have
> > - # overwritten it.
> > + subdirs += pipeline
>
> The comment you removed says exactly not to do this :)
>
> Why do you need this change ?
I presume this is just a bad merge conflict ?
>
>
> > endforeach
> > 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.606.ga4b1b128d6-goog
> >
More information about the libcamera-devel
mailing list