[libcamera-devel] [PATCH v1 1/3] libcamera: Move DmaHeap to HeapAllocator as a base class
Cheng-Hao Yang
chenghaoyang at google.com
Wed Feb 8 12:58:50 CET 2023
Hi Dave,
Thanks for pointing it out. Will update in the next patch.
On Wed, Feb 8, 2023 at 6:47 PM Dave Stevenson <
dave.stevenson at raspberrypi.com> wrote:
> Hi Harvey
>
> On Wed, 8 Feb 2023 at 09:59, Harvey Yang via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > 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>
> > ---
> > include/libcamera/dma_heap.h | 20 +++++++
> > include/libcamera/heap.h | 27 ++++++++++
> > include/libcamera/heap_allocator.h | 30 +++++++++++
> > include/libcamera/meson.build | 3 ++
> > .../dma_heaps.cpp => dma_heap.cpp} | 54 ++++++++++++-------
> > 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, 164 insertions(+), 58 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} (56%)
> > 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..96e311d3
> > --- /dev/null
> > +++ b/include/libcamera/dma_heap.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Google Inc.
> > + *
> > + * 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 56%
> > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > rename to src/libcamera/dma_heap.cpp
> > index 6b644406..8b67e9d0 100644
> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > +++ b/src/libcamera/dma_heap.cpp
> > @@ -1,19 +1,20 @@
> > /* SPDX-License-Identifier: LGPL-2.1-or-later */
> > /*
> > - * Copyright (C) 2020, Raspberry Pi Ltd
> > + * Copyright (C) 2023, Google Inc.
>
> Moving and then editing a file warrants claiming sole copyright?
> Sounds legally dubious to me.
>
> Dave
>
> > *
> > - * 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,34 @@ 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";
> > +
> > + int ret = ::open("/dev/udmabuf", O_RDWR, 0);
> > + if (ret < 0) {
> > + ret = errno;
> > + LOG(DmaHeap, Error)
> > + << "Failed to open allocator: " << strerror(ret);
> > +
> > + if (ret == EACCES) {
> > + LOG(DmaHeap, Info)
> > + << "Consider making /dev/udmabuf
> accessible by the video group";
> > + LOG(DmaHeap, Info)
> > + << "Alternatively, add your user to the
> kvm group.";
> > + }
> > +
> > + } else {
> > + handle_ = UniqueFD(ret);
> > + }
> > }
> >
> > DmaHeap::~DmaHeap() = default;
> > @@ -67,24 +83,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 6064a3f0..42b96286 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 8569df17..18325faa 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -20,6 +20,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>
> > @@ -41,7 +42,6 @@
> > #include "libcamera/internal/v4l2_videodevice.h"
> >
> > #include "delayed_controls.h"
> > -#include "dma_heaps.h"
> > #include "rpi_stream.h"
> >
> > using namespace std::chrono_literals;
> > @@ -240,7 +240,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_;
> > @@ -1206,7 +1206,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");
> > @@ -1595,9 +1595,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.39.1.519.gcb327c4b5f-goog
> >
>
--
BR,
Harvey Yang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230208/d9cb7182/attachment.htm>
More information about the libcamera-devel
mailing list