<div dir="ltr">Hi Dave,<div><br></div><div>Thanks for pointing it out. Will update in the next patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 8, 2023 at 6:47 PM Dave Stevenson <<a href="mailto:dave.stevenson@raspberrypi.com">dave.stevenson@raspberrypi.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>
On Wed, 8 Feb 2023 at 09:59, Harvey Yang via libcamera-devel<br>
<<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<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>
> include/libcamera/dma_heap.h | 20 +++++++<br>
> include/libcamera/heap.h | 27 ++++++++++<br>
> include/libcamera/heap_allocator.h | 30 +++++++++++<br>
> include/libcamera/meson.build | 3 ++<br>
> .../dma_heaps.cpp => dma_heap.cpp} | 54 ++++++++++++-------<br>
> src/libcamera/heap_allocator.cpp | 43 +++++++++++++++<br>
> src/libcamera/meson.build | 2 +<br>
> .../pipeline/raspberrypi/dma_heaps.h | 32 -----------<br>
> .../pipeline/raspberrypi/meson.build | 1 -<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++--<br>
> 10 files changed, 164 insertions(+), 58 deletions(-)<br>
> create mode 100644 include/libcamera/dma_heap.h<br>
> create mode 100644 include/libcamera/heap.h<br>
> create mode 100644 include/libcamera/heap_allocator.h<br>
> rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp => dma_heap.cpp} (56%)<br>
> create mode 100644 src/libcamera/heap_allocator.cpp<br>
> delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h<br>
><br>
> diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h<br>
> new file mode 100644<br>
> index 00000000..96e311d3<br>
> --- /dev/null<br>
> +++ b/include/libcamera/dma_heap.h<br>
> @@ -0,0 +1,20 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2023, Google Inc.<br>
> + *<br>
> + * dma_heap.h - Dma Heap implementation.<br>
> + */<br>
> +<br>
> +#include <libcamera/heap.h><br>
> +<br>
> +namespace libcamera {<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>
> +} /* namespace libcamera */<br>
> diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h<br>
> new file mode 100644<br>
> index 00000000..c49a2ac3<br>
> --- /dev/null<br>
> +++ b/include/libcamera/heap.h<br>
> @@ -0,0 +1,27 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2023, Google Inc.<br>
> + *<br>
> + * heap.h - Heap interface.<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <stddef.h><br>
> +<br>
> +#include <libcamera/base/unique_fd.h><br>
> +<br>
> +namespace libcamera {<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>
> +} /* namespace libcamera */<br>
> diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h<br>
> new file mode 100644<br>
> index 00000000..cd7ed1a3<br>
> --- /dev/null<br>
> +++ b/include/libcamera/heap_allocator.h<br>
> @@ -0,0 +1,30 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2023, Google Inc.<br>
> + *<br>
> + * heap_allocator.h - Helper class for heap buffer allocations.<br>
> + */<br>
> +<br>
> +#pragma once<br>
> +<br>
> +#include <stddef.h><br>
> +<br>
> +#include <libcamera/base/unique_fd.h><br>
> +<br>
> +#include <libcamera/heap.h><br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +class HeapAllocator<br>
> +{<br>
> +public:<br>
> + HeapAllocator();<br>
> + ~HeapAllocator();<br>
> + bool isValid() const { return heap_->isValid(); }<br>
> + UniqueFD alloc(const char *name, std::size_t size);<br>
> +<br>
> +private:<br>
> + std::unique_ptr<Heap> heap_;<br>
> +};<br>
> +<br>
> +} /* namespace libcamera */<br>
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build<br>
> index 408b7acf..f486630a 100644<br>
> --- a/include/libcamera/meson.build<br>
> +++ b/include/libcamera/meson.build<br>
> @@ -7,10 +7,13 @@ libcamera_public_headers = files([<br>
> 'camera_manager.h',<br>
> 'color_space.h',<br>
> 'controls.h',<br>
> + 'dma_heap.h',<br>
> 'fence.h',<br>
> 'framebuffer.h',<br>
> 'framebuffer_allocator.h',<br>
> 'geometry.h',<br>
> + 'heap.h',<br>
> + 'heap_allocator.h',<br>
> 'logging.h',<br>
> 'pixel_format.h',<br>
> 'request.h',<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/dma_heap.cpp<br>
> similarity index 56%<br>
> rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp<br>
> rename to src/libcamera/dma_heap.cpp<br>
> index 6b644406..8b67e9d0 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp<br>
> +++ b/src/libcamera/dma_heap.cpp<br>
> @@ -1,19 +1,20 @@<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>
Moving and then editing a file warrants claiming sole copyright?<br>
Sounds legally dubious to me.<br>
<br>
Dave<br>
<br>
> *<br>
> - * dma_heaps.h - Helper class for dma-heap allocations.<br>
> + * dma_heap.h - Dma Heap implementation.<br>
> */<br>
><br>
> -#include "dma_heaps.h"<br>
> +#include <libcamera/dma_heap.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 <linux/dma-buf.h><br>
> +#include <linux/dma-heap.h><br>
> +<br>
> #include <libcamera/base/log.h><br>
><br>
> /*<br>
> @@ -30,9 +31,7 @@ static constexpr std::array<const char *, 2> heapNames = {<br>
><br>
> namespace libcamera {<br>
><br>
> -LOG_DECLARE_CATEGORY(RPI)<br>
> -<br>
> -namespace RPi {<br>
> +LOG_DEFINE_CATEGORY(DmaHeap)<br>
><br>
> DmaHeap::DmaHeap()<br>
> {<br>
> @@ -40,17 +39,34 @@ DmaHeap::DmaHeap()<br>
> int ret = ::open(name, O_RDWR, 0);<br>
> if (ret < 0) {<br>
> ret = errno;<br>
> - LOG(RPI, Debug) << "Failed to open " << name << ": "<br>
> - << strerror(ret);<br>
> + LOG(DmaHeap, Debug) << "Failed to open " << name << ": "<br>
> + << strerror(ret);<br>
> continue;<br>
> }<br>
><br>
> - dmaHeapHandle_ = UniqueFD(ret);<br>
> + handle_ = UniqueFD(ret);<br>
> break;<br>
> }<br>
><br>
> - if (!dmaHeapHandle_.isValid())<br>
> - LOG(RPI, Error) << "Could not open any dmaHeap device";<br>
> + if (!handle_.isValid())<br>
> + LOG(DmaHeap, Error) << "Could not open any dmaHeap device";<br>
> +<br>
> + int ret = ::open("/dev/udmabuf", O_RDWR, 0);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(DmaHeap, Error)<br>
> + << "Failed to open allocator: " << strerror(ret);<br>
> +<br>
> + if (ret == EACCES) {<br>
> + LOG(DmaHeap, Info)<br>
> + << "Consider making /dev/udmabuf accessible by the video group";<br>
> + LOG(DmaHeap, Info)<br>
> + << "Alternatively, add your user to the kvm group.";<br>
> + }<br>
> +<br>
> + } else {<br>
> + handle_ = UniqueFD(ret);<br>
> + }<br>
> }<br>
><br>
> DmaHeap::~DmaHeap() = default;<br>
> @@ -67,24 +83,22 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)<br>
> alloc.len = size;<br>
> alloc.fd_flags = O_CLOEXEC | O_RDWR;<br>
><br>
> - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);<br>
> + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);<br>
> if (ret < 0) {<br>
> - LOG(RPI, Error) << "dmaHeap allocation failure for "<br>
> - << name;<br>
> + LOG(DmaHeap, 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>
> + LOG(DmaHeap, 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/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp<br>
> new file mode 100644<br>
> index 00000000..594b1d6a<br>
> --- /dev/null<br>
> +++ b/src/libcamera/heap_allocator.cpp<br>
> @@ -0,0 +1,43 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2023, Google Inc.<br>
> + *<br>
> + * heap_allocator.cpp - Helper class for heap buffer allocations.<br>
> + */<br>
> +<br>
> +#include <libcamera/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>
> +#include <libcamera/dma_heap.h><br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +LOG_DEFINE_CATEGORY(HeapAllocator)<br>
> +<br>
> +HeapAllocator::HeapAllocator()<br>
> +{<br>
> + heap_ = std::make_unique<DmaHeap>();<br>
> +}<br>
> +<br>
> +HeapAllocator::~HeapAllocator() = default;<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 9869bfe7..ee586c0d 100644<br>
> --- a/src/libcamera/meson.build<br>
> +++ b/src/libcamera/meson.build<br>
> @@ -17,11 +17,13 @@ libcamera_sources = files([<br>
> 'delayed_controls.cpp',<br>
> 'device_enumerator.cpp',<br>
> 'device_enumerator_sysfs.cpp',<br>
> + 'dma_heap.cpp',<br>
> 'fence.cpp',<br>
> 'formats.cpp',<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/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h<br>
> deleted file mode 100644<br>
> index 0a4a8d86..00000000<br>
> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h<br>
> +++ /dev/null<br>
> @@ -1,32 +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>
> -#pragma once<br>
> -<br>
> -#include <stddef.h><br>
> -<br>
> -#include <libcamera/base/unique_fd.h><br>
> -<br>
> -namespace libcamera {<br>
> -<br>
> -namespace RPi {<br>
> -<br>
> -class DmaHeap<br>
> -{<br>
> -public:<br>
> - DmaHeap();<br>
> - ~DmaHeap();<br>
> - bool isValid() const { return dmaHeapHandle_.isValid(); }<br>
> - UniqueFD alloc(const char *name, std::size_t size);<br>
> -<br>
> -private:<br>
> - UniqueFD dmaHeapHandle_;<br>
> -};<br>
> -<br>
> -} /* namespace RPi */<br>
> -<br>
> -} /* namespace libcamera */<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build<br>
> index 6064a3f0..42b96286 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/meson.build<br>
> +++ b/src/libcamera/pipeline/raspberrypi/meson.build<br>
> @@ -2,7 +2,6 @@<br>
><br>
> libcamera_sources += files([<br>
> 'delayed_controls.cpp',<br>
> - 'dma_heaps.cpp',<br>
> 'raspberrypi.cpp',<br>
> 'rpi_stream.cpp',<br>
> ])<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 8569df17..18325faa 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -20,6 +20,7 @@<br>
> #include <libcamera/camera.h><br>
> #include <libcamera/control_ids.h><br>
> #include <libcamera/formats.h><br>
> +#include <libcamera/heap_allocator.h><br>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h><br>
> #include <libcamera/ipa/raspberrypi_ipa_proxy.h><br>
> #include <libcamera/logging.h><br>
> @@ -41,7 +42,6 @@<br>
> #include "libcamera/internal/v4l2_videodevice.h"<br>
><br>
> #include "delayed_controls.h"<br>
> -#include "dma_heaps.h"<br>
> #include "rpi_stream.h"<br>
><br>
> using namespace std::chrono_literals;<br>
> @@ -240,7 +240,7 @@ public:<br>
> std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, MediaLink *>> bridgeDevices_;<br>
><br>
> /* DMAHEAP allocation helper. */<br>
> - RPi::DmaHeap dmaHeap_;<br>
> + HeapAllocator heapAllocator_;<br>
> SharedFD lsTable_;<br>
><br>
> std::unique_ptr<RPi::DelayedControls> delayedCtrls_;<br>
> @@ -1206,7 +1206,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> {<br>
> std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);<br>
><br>
> - if (!data->dmaHeap_.isValid())<br>
> + if (!data->heapAllocator_.isValid())<br>
> return -ENOMEM;<br>
><br>
> MediaEntity *unicamImage = unicam->getEntityByName("unicam-image");<br>
> @@ -1595,9 +1595,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA<br>
> /* Always send the user transform to the IPA. */<br>
> ipaConfig.transform = static_cast<unsigned int>(config->transform);<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>
> 2.39.1.519.gcb327c4b5f-goog<br>
><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>BR,</div>Harvey Yang</div></div>