[libcamera-devel] [PATCH v6 1/3] libcamera: Move DmaHeap to HeapAllocator as a base class
Umang Jain
umang.jain at ideasonboard.com
Wed May 31 08:23:24 CEST 2023
Hi Harvey
On 5/30/23 11:38 AM, Umang Jain via libcamera-devel wrote:
> Hi Harvey,
>
> Thank you for the patch.
>
> On 5/22/23 2:05 PM, 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 | 17 ++-
>> include/libcamera/internal/meson.build | 1 +
>> src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++
>> src/libcamera/meson.build | 1 +
>> 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 +-
>> 7 files changed, 146 insertions(+), 109 deletions(-)
>> rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h =>
>> include/libcamera/internal/heap_allocator.h (57%)
>> 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 57%
>> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h
>> rename to include/libcamera/internal/heap_allocator.h
>> index 0a4a8d86..92d4488a 100644
>> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
>> +++ b/include/libcamera/internal/heap_allocator.h
>> @@ -1,8 +1,9 @@
>> /* SPDX-License-Identifier: LGPL-2.1-or-later */
>> /*
>> * Copyright (C) 2020, Raspberry Pi Ltd
>> + * Copyright (C) 2023, Google Inc.
>> *
>> - * dma_heaps.h - Helper class for dma-heap allocations.
>> + * heap_allocator.h - Helper class for heap buffer allocations.
>> */
>> #pragma once
>> @@ -13,20 +14,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..69f65062
>> --- /dev/null
>> +++ b/src/libcamera/heap_allocator.cpp
>> @@ -0,0 +1,130 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Raspberry Pi Ltd
>> + * Copyright (C) 2023, Google Inc.
>> + *
>> + * 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 *, 3> dmaHeapNames = {
>> + "/dev/dma_heap/linux,cma",
>> + "/dev/dma_heap/reserved",
>> + "/dev/dma_heap/system"
>> +};
>> +
>> +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;
>
> This should be automatically generated no?
Sorry, this is actually correct. I got confused between
implicitly-declared destructor vs implicit user-defined desctructor.
>> +
>> +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;
>
> Same here
Same applies here. Sorry for the noise!
>> +
>> +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/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;
>
More information about the libcamera-devel
mailing list