<div dir="ltr"><div dir="ltr">Thanks Umang and Jacopo!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 30, 2023 at 2:08 PM Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey,<br>
<br>
Thank you for the patch.<br>
<br>
On 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote:<br>
> From: Cheng-Hao Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
><br>
> As other components (like the WIP virtual pipeline handler) also need a<br>
> heap allocator, move DmaHeap from raspberry pi pipeline handler to a<br>
> general HeapAllocator as a base class.<br>
><br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> .../libcamera/internal/heap_allocator.h | 17 ++-<br>
> include/libcamera/internal/meson.build | 1 +<br>
> src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++<br>
> src/libcamera/meson.build | 1 +<br>
> src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------<br>
> src/libcamera/pipeline/rpi/vc4/meson.build | 5 +-<br>
> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +-<br>
> 7 files changed, 146 insertions(+), 109 deletions(-)<br>
> rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (57%)<br>
> create mode 100644 src/libcamera/heap_allocator.cpp<br>
> delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp<br>
><br>
> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/heap_allocator.h<br>
> similarity index 57%<br>
> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h<br>
> rename to include/libcamera/internal/heap_allocator.h<br>
> index 0a4a8d86..92d4488a 100644<br>
> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h<br>
> +++ b/include/libcamera/internal/heap_allocator.h<br>
> @@ -1,8 +1,9 @@<br>
> /* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> /*<br>
> * Copyright (C) 2020, Raspberry Pi Ltd<br>
> + * Copyright (C) 2023, Google Inc.<br>
> *<br>
> - * dma_heaps.h - Helper class for dma-heap allocations.<br>
> + * heap_allocator.h - Helper class for heap buffer allocations.<br>
> */<br>
> <br>
> #pragma once<br>
> @@ -13,20 +14,18 @@<br>
> <br>
> namespace libcamera {<br>
> <br>
> -namespace RPi {<br>
> +class Heap;<br>
> <br>
> -class DmaHeap<br>
> +class HeapAllocator<br>
> {<br>
> public:<br>
> - DmaHeap();<br>
> - ~DmaHeap();<br>
> - bool isValid() const { return dmaHeapHandle_.isValid(); }<br>
> + HeapAllocator();<br>
> + ~HeapAllocator();<br>
> + bool isValid() const;<br>
> UniqueFD alloc(const char *name, std::size_t size);<br>
> <br>
> private:<br>
> - UniqueFD dmaHeapHandle_;<br>
> + std::unique_ptr<Heap> heap_;<br>
> };<br>
> <br>
> -} /* namespace RPi */<br>
> -<br>
> } /* namespace libcamera */<br>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build<br>
> index d7508805..9d25c289 100644<br>
> --- a/include/libcamera/internal/meson.build<br>
> +++ b/include/libcamera/internal/meson.build<br>
> @@ -26,6 +26,7 @@ libcamera_internal_headers = files([<br>
> 'device_enumerator_udev.h',<br>
> 'formats.h',<br>
> 'framebuffer.h',<br>
> + 'heap_allocator.h',<br>
> 'ipa_manager.h',<br>
> 'ipa_module.h',<br>
> 'ipa_proxy.h',<br>
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp<br>
> new file mode 100644<br>
> index 00000000..69f65062<br>
> --- /dev/null<br>
> +++ b/src/libcamera/heap_allocator.cpp<br>
> @@ -0,0 +1,130 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2020, Raspberry Pi Ltd<br>
> + * Copyright (C) 2023, Google Inc.<br>
> + *<br>
> + * heap_allocator.cpp - Helper class for heap buffer allocations.<br>
> + */<br>
> +<br>
> +#include "libcamera/internal/heap_allocator.h"<br>
> +<br>
> +#include <array><br>
> +#include <fcntl.h><br>
> +#include <sys/ioctl.h><br>
> +#include <unistd.h><br>
> +<br>
> +#include <linux/dma-buf.h><br>
> +#include <linux/dma-heap.h><br>
> +<br>
> +#include <libcamera/base/log.h><br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +/*<br>
> + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma<br>
> + * to only have to worry about importing.<br>
> + *<br>
> + * Annoyingly, should the cma heap size be specified on the kernel command line<br>
> + * instead of DT, the heap gets named "reserved" instead.<br>
> + */<br>
> +static constexpr std::array<const char *, 3> dmaHeapNames = {<br>
> + "/dev/dma_heap/linux,cma",<br>
> + "/dev/dma_heap/reserved",<br>
> + "/dev/dma_heap/system"<br>
> +};<br>
> +<br>
> +LOG_DEFINE_CATEGORY(HeapAllocator)<br>
> +<br>
> +class Heap<br>
> +{<br>
> +public:<br>
> + virtual ~Heap() = default;<br>
> + bool isValid() const { return handle_.isValid(); }<br>
> + virtual UniqueFD alloc(const char *name, std::size_t size) = 0;<br>
> +<br>
> +protected:<br>
> + UniqueFD handle_;<br>
> +};<br>
> +<br>
> +class DmaHeap : public Heap<br>
> +{<br>
> +public:<br>
> + DmaHeap();<br>
> + ~DmaHeap();<br>
> + UniqueFD alloc(const char *name, std::size_t size) override;<br>
> +};<br>
> +<br>
> +DmaHeap::DmaHeap()<br>
> +{<br>
> + for (const char *name : dmaHeapNames) {<br>
> + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(HeapAllocator, Debug) << "DmaHeap failed to open " << name << ": "<br>
> + << strerror(ret);<br>
> + continue;<br>
> + }<br>
> +<br>
> + handle_ = UniqueFD(ret);<br>
> + break;<br>
> + }<br>
> +<br>
> + if (!handle_.isValid())<br>
> + LOG(HeapAllocator, Error) << "DmaHeap could not open any dmaHeap device";<br>
> +}<br>
> +<br>
> +DmaHeap::~DmaHeap() = default;<br>
<br>
This should be automatically generated no?</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)<br>
> +{<br>
> + int ret;<br>
> +<br>
> + if (!name)<br>
> + return {};<br>
> +<br>
> + struct dma_heap_allocation_data alloc = {};<br>
> +<br>
> + alloc.len = size;<br>
> + alloc.fd_flags = O_CLOEXEC | O_RDWR;<br>
> +<br>
> + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);<br>
> + if (ret < 0) {<br>
> + LOG(HeapAllocator, Error) << "DmaHeap allocation failure for "<br>
> + << name;<br>
> + return {};<br>
> + }<br>
> +<br>
> + UniqueFD allocFd(alloc.fd);<br>
> + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);<br>
> + if (ret < 0) {<br>
> + LOG(HeapAllocator, Error) << "DmaHeap naming failure for "<br>
> + << name;<br>
> + return {};<br>
> + }<br>
> +<br>
> + return allocFd;<br>
> +}<br>
> +<br>
> +HeapAllocator::HeapAllocator()<br>
> +{<br>
> + heap_ = std::make_unique<DmaHeap>();<br>
> +}<br>
> +<br>
> +HeapAllocator::~HeapAllocator() = default;<br>
<br>
Same here</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +bool HeapAllocator::isValid() const<br>
> +{<br>
> + return heap_->isValid();<br>
> +}<br>
> +<br>
> +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)<br>
> +{<br>
> + if (!isValid()) {<br>
> + LOG(HeapAllocator, Fatal) << "Allocation attempted without allocator" << name;<br>
> + return {};<br>
> + }<br>
> +<br>
> + return heap_->alloc(name, size);<br>
> +}<br>
> +<br>
> +} /* namespace libcamera */<br>
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build<br>
> index d4385041..aa2d32a0 100644<br>
> --- a/src/libcamera/meson.build<br>
> +++ b/src/libcamera/meson.build<br>
> @@ -22,6 +22,7 @@ libcamera_sources = files([<br>
> 'framebuffer.cpp',<br>
> 'framebuffer_allocator.cpp',<br>
> 'geometry.cpp',<br>
> + 'heap_allocator.cpp',<br>
> 'ipa_controls.cpp',<br>
> 'ipa_data_serializer.cpp',<br>
> 'ipa_interface.cpp',<br>
> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp<br>
> deleted file mode 100644<br>
> index 317b1fc1..00000000<br>
> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp<br>
> +++ /dev/null<br>
> @@ -1,90 +0,0 @@<br>
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> -/*<br>
> - * Copyright (C) 2020, Raspberry Pi Ltd<br>
> - *<br>
> - * dma_heaps.h - Helper class for dma-heap allocations.<br>
> - */<br>
> -<br>
> -#include "dma_heaps.h"<br>
> -<br>
> -#include <array><br>
> -#include <fcntl.h><br>
> -#include <linux/dma-buf.h><br>
> -#include <linux/dma-heap.h><br>
> -#include <sys/ioctl.h><br>
> -#include <unistd.h><br>
> -<br>
> -#include <libcamera/base/log.h><br>
> -<br>
> -/*<br>
> - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma<br>
> - * to only have to worry about importing.<br>
> - *<br>
> - * Annoyingly, should the cma heap size be specified on the kernel command line<br>
> - * instead of DT, the heap gets named "reserved" instead.<br>
> - */<br>
> -static constexpr std::array<const char *, 2> heapNames = {<br>
> - "/dev/dma_heap/linux,cma",<br>
> - "/dev/dma_heap/reserved"<br>
> -};<br>
> -<br>
> -namespace libcamera {<br>
> -<br>
> -LOG_DECLARE_CATEGORY(RPI)<br>
> -<br>
> -namespace RPi {<br>
> -<br>
> -DmaHeap::DmaHeap()<br>
> -{<br>
> - for (const char *name : heapNames) {<br>
> - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);<br></blockquote><div><br></div><div>Removed the third argument based on Jacopo's comment.</div><div><span style="font-family:Roboto,Arial,sans-serif;font-variant-ligatures:none;letter-spacing:0.2px;white-space:pre-wrap;background-color:rgb(255,255,255)"><font color="#000000">(I only copied and pasted from the previous implementation though :p)</font></span><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> - if (ret < 0) {<br>
> - ret = errno;<br>
> - LOG(RPI, Debug) << "Failed to open " << name << ": "<br>
> - << strerror(ret);<br>
> - continue;<br>
> - }<br>
> -<br>
> - dmaHeapHandle_ = UniqueFD(ret);<br>
> - break;<br>
> - }<br>
> -<br>
> - if (!dmaHeapHandle_.isValid())<br>
> - LOG(RPI, Error) << "Could not open any dmaHeap device";<br>
> -}<br>
> -<br>
> -DmaHeap::~DmaHeap() = default;<br>
> -<br>
> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)<br>
> -{<br>
> - int ret;<br>
> -<br>
> - if (!name)<br>
> - return {};<br>
> -<br>
> - struct dma_heap_allocation_data alloc = {};<br>
> -<br>
> - alloc.len = size;<br>
> - alloc.fd_flags = O_CLOEXEC | O_RDWR;<br>
> -<br>
> - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);<br>
> - if (ret < 0) {<br>
> - LOG(RPI, Error) << "dmaHeap allocation failure for "<br>
> - << name;<br>
> - return {};<br>
> - }<br>
> -<br>
> - UniqueFD allocFd(alloc.fd);<br>
> - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);<br>
> - if (ret < 0) {<br>
> - LOG(RPI, Error) << "dmaHeap naming failure for "<br>
> - << name;<br>
> - return {};<br>
> - }<br>
> -<br>
> - return allocFd;<br>
> -}<br>
> -<br>
> -} /* namespace RPi */<br>
> -<br>
> -} /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build<br>
> index cdb049c5..b2b79735 100644<br>
> --- a/src/libcamera/pipeline/rpi/vc4/meson.build<br>
> +++ b/src/libcamera/pipeline/rpi/vc4/meson.build<br>
> @@ -1,8 +1,5 @@<br>
> # SPDX-License-Identifier: CC0-1.0<br>
> <br>
> -libcamera_sources += files([<br>
> - 'dma_heaps.cpp',<br>
> - 'vc4.cpp',<br>
> -])<br>
> +libcamera_sources += files(['vc4.cpp'])<br>
> <br>
> subdir('data')<br>
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>
> index 018cf488..410ecfaf 100644<br>
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>
> @@ -12,12 +12,11 @@<br>
> #include <libcamera/formats.h><br>
> <br>
> #include "libcamera/internal/device_enumerator.h"<br>
> +#include "libcamera/internal/heap_allocator.h"<br>
> <br>
> #include "../common/pipeline_base.h"<br>
> #include "../common/rpi_stream.h"<br>
> <br>
> -#include "dma_heaps.h"<br>
> -<br>
> using namespace std::chrono_literals;<br>
> <br>
> namespace libcamera {<br>
> @@ -87,7 +86,7 @@ public:<br>
> RPi::Device<Isp, 4> isp_;<br>
> <br>
> /* DMAHEAP allocation helper. */<br>
> - RPi::DmaHeap dmaHeap_;<br>
> + HeapAllocator heapAllocator_;<br>
> SharedFD lsTable_;<br>
> <br>
> struct Config {<br>
> @@ -296,7 +295,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer<br>
> {<br>
> Vc4CameraData *data = static_cast<Vc4CameraData *>(cameraData.get());<br>
> <br>
> - if (!data->dmaHeap_.isValid())<br>
> + if (!data->heapAllocator_.isValid())<br>
> return -ENOMEM;<br>
> <br>
> MediaEntity *unicamImage = unicam->getEntityByName("unicam-image");<br>
> @@ -670,9 +669,9 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)<br>
> {<br>
> params.ispControls = isp_[Isp::Input].dev()->controls();<br>
> <br>
> - /* Allocate the lens shading table via dmaHeap and pass to the IPA. */<br>
> + /* Allocate the lens shading table via heapAllocator and pass to the IPA. */<br>
> if (!lsTable_.isValid()) {<br>
> - lsTable_ = SharedFD(dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize));<br>
> + lsTable_ = SharedFD(heapAllocator_.alloc("ls_grid", ipa::RPi::MaxLsGridSize));<br>
> if (!lsTable_.isValid())<br>
> return -ENOMEM;<br>
> <br>
<br></blockquote><div><br><span style="background-color:rgb(255,255,255)"><font color="#000000"><span style="font-family:Roboto,Arial,sans-serif;font-variant-ligatures:none;letter-spacing:0.2px;white-space:pre-wrap">Updated with some documents. Please feel free to directly modify it when</span><span style="letter-spacing:0.2px"><br style="font-family:Roboto,Arial,sans-serif;font-variant-ligatures:none;letter-spacing:0.2px;white-space:pre-wrap"></span><span style="font-family:Roboto,Arial,sans-serif;font-variant-ligatures:none;letter-spacing:0.2px;white-space:pre-wrap">landing. I'm very bad at documenting code...</span></font></span><br><br><br>BR,<br>Harvey</div></div></div>