[libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to HeapAllocator as a base class
Cheng-Hao Yang
chenghaoyang at chromium.org
Mon May 22 10:37:36 CEST 2023
Thanks Kieran and Jacopo for another round of review!
On Tue, May 16, 2023 at 7:27 PM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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.
>
>
I see. I thought keeping RPi copyright in heap_allocator.cpp is enough, as
that's where DmaHeap stays in this patch. The git mv is just auto generated
as they look pretty much the same.
I'll keep both copyrights in heap_allocator.h & heap_allocator.cpp, as the
APIs
of HeapAllocator are basically the same as those of DeaHeap.
>
> >
> > > *
> > > - * 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.
>
>
Actually the mediatek device (the new board) uses "/dev/dma_heap/system" as
well. Let's add it now :)
> > > +};
> > > +
> > > +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 ?
>
>
Yeah Kieran guess it right... Fixed.
> >
> >
> > > 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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230522/f7f1a072/attachment.htm>
More information about the libcamera-devel
mailing list