<div dir="ltr"><div dir="ltr">Thanks Kieran and Jacopo for another round of review!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 16, 2023 at 7:27 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@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">Quoting Jacopo Mondi via libcamera-devel (2023-05-16 11:24:33)<br>
> Hi Harvey<br>
> On Tue, May 16, 2023 at 08:03:17AM +0000, 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 | 18 ++-<br>
> > include/libcamera/internal/meson.build | 1 +<br>
> > src/libcamera/heap_allocator.cpp | 128 ++++++++++++++++++<br>
> > src/libcamera/meson.build | 1 +<br>
> > src/libcamera/pipeline/meson.build | 5 +-<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>
> > 8 files changed, 145 insertions(+), 114 deletions(-)<br>
> > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (50%)<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 50%<br>
> > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h<br>
> > rename to include/libcamera/internal/heap_allocator.h<br>
> > index 0a4a8d86..d061fdce 100644<br>
> > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h<br>
> > +++ b/include/libcamera/internal/heap_allocator.h<br>
> > @@ -1,8 +1,8 @@<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>
> Why can't you keep both ? If the code comes from RPi they copyright<br>
> should be retained.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'm sure we've been here before ;-)<br>
<br>
Indeed, please don't drop copyrights when moving code around.<br>
<br></blockquote><div><br></div><div><div><br>I see. I thought keeping RPi copyright in heap_allocator.cpp is enough, as</div><div>that's where DmaHeap stays in this patch. The git mv is just auto generated</div><div>as they look pretty much the same.</div><div><br></div></div><div>I'll keep both copyrights in heap_allocator.h & heap_allocator.cpp, as the APIs</div><div>of HeapAllocator are basically the same as those of DeaHeap.</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">
<br>
> <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 +13,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..e9476de5<br>
> > --- /dev/null<br>
> > +++ b/src/libcamera/heap_allocator.cpp<br>
> > @@ -0,0 +1,128 @@<br>
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> > +/*<br>
> > + * Copyright (C) 2020, Raspberry Pi Ltd<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 *, 2> dmaHeapNames = {<br>
> > + "/dev/dma_heap/linux,cma",<br>
> > + "/dev/dma_heap/reserved"<br>
<br>
On my x86 pc, I have "/dev/dma_heap/system" too. I don't know if we<br>
shoudl add that ... but maybe something to explore later.<br>
<br>
This is fine to keep as it is though here I think.<br>
<br></blockquote><div><br></div><div>Actually the mediatek device (the new board) uses "/dev/dma_heap/system" as well. Let's add it now :)</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">
> > +};<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>
> > +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>
> > +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/meson.build b/src/libcamera/pipeline/meson.build<br>
> > index 8a61991c..b6160d34 100644<br>
> > --- a/src/libcamera/pipeline/meson.build<br>
> > +++ b/src/libcamera/pipeline/meson.build<br>
> > @@ -12,9 +12,6 @@ foreach pipeline : pipelines<br>
> > continue<br>
> > endif<br>
> ><br>
> > - subdirs += pipeline<br>
> > subdir(pipeline)<br>
> > -<br>
> > - # Don't reuse the pipeline variable below, the subdirectory may have<br>
> > - # overwritten it.<br>
> > + subdirs += pipeline<br>
> <br>
> The comment you removed says exactly not to do this :)<br>
> <br>
> Why do you need this change ?<br>
<br>
I presume this is just a bad merge conflict ?<br>
<br></blockquote><div><br></div><div>Yeah Kieran guess it right... Fixed.</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">
> <br>
> <br>
> > endforeach<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>
> > - 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>
> > 2.40.1.606.ga4b1b128d6-goog<br>
> ><br>
</blockquote></div></div>