[libcamera-devel] [PATCH v4 1/3] libcamera: Move DmaHeap to HeapAllocator as a base class
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Sat May 13 18:12:24 CEST 2023
Hi Harvey
On Tue, Mar 28, 2023 at 05:55:32PM +0800, 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>
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
>
More information about the libcamera-devel
mailing list