[{"id":27107,"web_url":"https://patchwork.libcamera.org/comment/27107/","msgid":"<20230516102433.mtr3h4zn5squgbbe@lati>","date":"2023-05-16T10:24:33","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\nOn Tue, May 16, 2023 at 08:03:17AM +0000, Harvey Yang via libcamera-devel wrote:\n> From: Cheng-Hao Yang \n>\n> As other components (like the WIP virtual pipeline handler) also need a\n> heap allocator, move DmaHeap from raspberry pi pipeline handler to a\n> general HeapAllocator as a base class.\n>\n> Signed-off-by: Harvey Yang \n> ---\n> .../libcamera/internal/heap_allocator.h | 18 ++-\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/heap_allocator.cpp | 128 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/pipeline/meson.build | 5 +-\n> src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------\n> src/libcamera/pipeline/rpi/vc4/meson.build | 5 +-\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +-\n> 8 files changed, 145 insertions(+), 114 deletions(-)\n> rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (50%)\n> create mode 100644 src/libcamera/heap_allocator.cpp\n> delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n>\n> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/heap_allocator.h\n> similarity index 50%\n> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> rename to include/libcamera/internal/heap_allocator.h\n> index 0a4a8d86..d061fdce 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> +++ b/include/libcamera/internal/heap_allocator.h\n> @@ -1,8 +1,8 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2020, Raspberry Pi Ltd\n> + * Copyright (C) 2023, Google Inc.\n\nWhy can't you keep both ? If the code comes from RPi they copyright\nshould be retained.\n\n> *\n> - * dma_heaps.h - Helper class for dma-heap allocations.\n> + * heap_allocator.h - Helper class for heap buffer allocations.\n> */\n>\n> #pragma once\n> @@ -13,20 +13,18 @@\n>\n> namespace libcamera {\n>\n> -namespace RPi {\n> +class Heap;\n>\n> -class DmaHeap\n> +class HeapAllocator\n> {\n> public:\n> -\tDmaHeap();\n> -\t~DmaHeap();\n> -\tbool isValid() const { return dmaHeapHandle_.isValid(); }\n> +\tHeapAllocator();\n> +\t~HeapAllocator();\n> +\tbool isValid() const;\n> \tUniqueFD alloc(const char *name, std::size_t size);\n>\n> private:\n> -\tUniqueFD dmaHeapHandle_;\n> +\tstd::unique_ptr heap_;\n> };\n>\n> -} /* namespace RPi */\n> -\n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index d7508805..9d25c289 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -26,6 +26,7 @@ libcamera_internal_headers = files([\n> 'device_enumerator_udev.h',\n> 'formats.h',\n> 'framebuffer.h',\n> + 'heap_allocator.h',\n> 'ipa_manager.h',\n> 'ipa_module.h',\n> 'ipa_proxy.h',\n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> new file mode 100644\n> index 00000000..e9476de5\n> --- /dev/null\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -0,0 +1,128 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi Ltd\n> + *\n> + * heap_allocator.cpp - Helper class for heap buffer allocations.\n> + */\n> +\n> +#include \"libcamera/internal/heap_allocator.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +/*\n> + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma\n> + * to only have to worry about importing.\n> + *\n> + * Annoyingly, should the cma heap size be specified on the kernel command line\n> + * instead of DT, the heap gets named \"reserved\" instead.\n> + */\n> +static constexpr std::array dmaHeapNames = {\n> +\t\"/dev/dma_heap/linux,cma\",\n> +\t\"/dev/dma_heap/reserved\"\n> +};\n> +\n> +LOG_DEFINE_CATEGORY(HeapAllocator)\n> +\n> +class Heap\n> +{\n> +public:\n> +\tvirtual ~Heap() = default;\n> +\tbool isValid() const { return handle_.isValid(); }\n> +\tvirtual UniqueFD alloc(const char *name, std::size_t size) = 0;\n> +\n> +protected:\n> +\tUniqueFD handle_;\n> +};\n> +\n> +class DmaHeap : public Heap\n> +{\n> +public:\n> +\tDmaHeap();\n> +\t~DmaHeap();\n> +\tUniqueFD alloc(const char *name, std::size_t size) override;\n> +};\n> +\n> +DmaHeap::DmaHeap()\n> +{\n> +\tfor (const char *name : dmaHeapNames) {\n> +\t\tint ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> +\t\tif (ret < 0) {\n> +\t\t\tret = errno;\n> +\t\t\tLOG(HeapAllocator, Debug) << \"DmaHeap failed to open \" << name << \": \"\n> +\t\t\t\t\t\t << strerror(ret);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\thandle_ = UniqueFD(ret);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (!handle_.isValid())\n> +\t\tLOG(HeapAllocator, Error) << \"DmaHeap could not open any dmaHeap device\";\n> +}\n> +\n> +DmaHeap::~DmaHeap() = default;\n> +\n> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> +{\n> +\tint ret;\n> +\n> +\tif (!name)\n> +\t\treturn {};\n> +\n> +\tstruct dma_heap_allocation_data alloc = {};\n> +\n> +\talloc.len = size;\n> +\talloc.fd_flags = O_CLOEXEC | O_RDWR;\n> +\n> +\tret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> +\tif (ret < 0) {\n> +\t\tLOG(HeapAllocator, Error) << \"DmaHeap allocation failure for \"\n> +\t\t\t\t\t << name;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tUniqueFD allocFd(alloc.fd);\n> +\tret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> +\tif (ret < 0) {\n> +\t\tLOG(HeapAllocator, Error) << \"DmaHeap naming failure for \"\n> +\t\t\t\t\t << name;\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn allocFd;\n> +}\n> +\n> +HeapAllocator::HeapAllocator()\n> +{\n> +\theap_ = std::make_unique();\n> +}\n> +\n> +HeapAllocator::~HeapAllocator() = default;\n> +\n> +bool HeapAllocator::isValid() const\n> +{\n> +\treturn heap_->isValid();\n> +}\n> +\n> +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)\n> +{\n> +\tif (!isValid()) {\n> +\t\tLOG(HeapAllocator, Fatal) << \"Allocation attempted without allocator\" << name;\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn heap_->alloc(name, size);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index d4385041..aa2d32a0 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -22,6 +22,7 @@ libcamera_sources = files([\n> 'framebuffer.cpp',\n> 'framebuffer_allocator.cpp',\n> 'geometry.cpp',\n> + 'heap_allocator.cpp',\n> 'ipa_controls.cpp',\n> 'ipa_data_serializer.cpp',\n> 'ipa_interface.cpp',\n> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> index 8a61991c..b6160d34 100644\n> --- a/src/libcamera/pipeline/meson.build\n> +++ b/src/libcamera/pipeline/meson.build\n> @@ -12,9 +12,6 @@ foreach pipeline : pipelines\n> continue\n> endif\n>\n> - subdirs += pipeline\n> subdir(pipeline)\n> -\n> - # Don't reuse the pipeline variable below, the subdirectory may have\n> - # overwritten it.\n> + subdirs += pipeline\n\nThe comment you removed says exactly not to do this :)\n\nWhy do you need this change ?\n\n\n> endforeach\n> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> deleted file mode 100644\n> index 317b1fc1..00000000\n> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> +++ /dev/null\n> @@ -1,90 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Raspberry Pi Ltd\n> - *\n> - * dma_heaps.h - Helper class for dma-heap allocations.\n> - */\n> -\n> -#include \"dma_heaps.h\"\n> -\n> -#include \n> -#include \n> -#include \n> -#include \n> -#include \n> -#include \n> -\n> -#include \n> -\n> -/*\n> - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma\n> - * to only have to worry about importing.\n> - *\n> - * Annoyingly, should the cma heap size be specified on the kernel command line\n> - * instead of DT, the heap gets named \"reserved\" instead.\n> - */\n> -static constexpr std::array heapNames = {\n> -\t\"/dev/dma_heap/linux,cma\",\n> -\t\"/dev/dma_heap/reserved\"\n> -};\n> -\n> -namespace libcamera {\n> -\n> -LOG_DECLARE_CATEGORY(RPI)\n> -\n> -namespace RPi {\n> -\n> -DmaHeap::DmaHeap()\n> -{\n> -\tfor (const char *name : heapNames) {\n> -\t\tint ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> -\t\tif (ret < 0) {\n> -\t\t\tret = errno;\n> -\t\t\tLOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> -\t\t\t\t\t<< strerror(ret);\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tdmaHeapHandle_ = UniqueFD(ret);\n> -\t\tbreak;\n> -\t}\n> -\n> -\tif (!dmaHeapHandle_.isValid())\n> -\t\tLOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> -}\n> -\n> -DmaHeap::~DmaHeap() = default;\n> -\n> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> -{\n> -\tint ret;\n> -\n> -\tif (!name)\n> -\t\treturn {};\n> -\n> -\tstruct dma_heap_allocation_data alloc = {};\n> -\n> -\talloc.len = size;\n> -\talloc.fd_flags = O_CLOEXEC | O_RDWR;\n> -\n> -\tret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> -\tif (ret < 0) {\n> -\t\tLOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> -\t\t\t\t<< name;\n> -\t\treturn {};\n> -\t}\n> -\n> -\tUniqueFD allocFd(alloc.fd);\n> -\tret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> -\tif (ret < 0) {\n> -\t\tLOG(RPI, Error) << \"dmaHeap naming failure for \"\n> -\t\t\t\t<< name;\n> -\t\treturn {};\n> -\t}\n> -\n> -\treturn allocFd;\n> -}\n> -\n> -} /* namespace RPi */\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build\n> index cdb049c5..b2b79735 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/meson.build\n> +++ b/src/libcamera/pipeline/rpi/vc4/meson.build\n> @@ -1,8 +1,5 @@\n> # SPDX-License-Identifier: CC0-1.0\n>\n> -libcamera_sources += files([\n> - 'dma_heaps.cpp',\n> - 'vc4.cpp',\n> -])\n> +libcamera_sources += files(['vc4.cpp'])\n>\n> subdir('data')\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index 018cf488..410ecfaf 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -12,12 +12,11 @@\n> #include \n>\n> #include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/heap_allocator.h\"\n>\n> #include \"../common/pipeline_base.h\"\n> #include \"../common/rpi_stream.h\"\n>\n> -#include \"dma_heaps.h\"\n> -\n> using namespace std::chrono_literals;\n>\n> namespace libcamera {\n> @@ -87,7 +86,7 @@ public:\n> \tRPi::Device isp_;\n>\n> \t/* DMAHEAP allocation helper. */\n> -\tRPi::DmaHeap dmaHeap_;\n> +\tHeapAllocator heapAllocator_;\n> \tSharedFD lsTable_;\n>\n> \tstruct Config {\n> @@ -296,7 +295,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> {\n> \tVc4CameraData *data = static_cast(cameraData.get());\n>\n> -\tif (!data->dmaHeap_.isValid())\n> +\tif (!data->heapAllocator_.isValid())\n> \t\treturn -ENOMEM;\n>\n> \tMediaEntity *unicamImage = unicam->getEntityByName(\"unicam-image\");\n> @@ -670,9 +669,9 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)\n> {\n> \tparams.ispControls = isp_[Isp::Input].dev()->controls();\n>\n> -\t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> +\t/* Allocate the lens shading table via heapAllocator and pass to the IPA. */\n> \tif (!lsTable_.isValid()) {\n> -\t\tlsTable_ = SharedFD(dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize));\n> +\t\tlsTable_ = SharedFD(heapAllocator_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize));\n> \t\tif (!lsTable_.isValid())\n> \t\t\treturn -ENOMEM;\n>\n> --\n> 2.40.1.606.ga4b1b128d6-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D8BE2BDE6B\n\tfor ;\n\tTue, 16 May 2023 10:24:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4ABD8627DE;\n\tTue, 16 May 2023 12:24:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 53C6560399\n\tfor ;\n\tTue, 16 May 2023 12:24:37 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D7C7755;\n\tTue, 16 May 2023 12:24:25 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684232678;\n\tbh=qY2IPPeJbHLGkDZRKozFbngjJmQCUrK8I4b+U66q/D0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=VJxj2CdUO+zBeIdkoO0vwmZQvBzsv7b+/R6CDOGPK00aYBL/dh0QXpqOek9UjUe3/\n\tERX9pp9xfZzuejvkSuOkKcPeJfy85JCNh15pRqb7Mbhc5t5ZjGGXnOYDN63BKifwwE\n\tVSJAqByBm4XNx+TRuoF6978XkBlLEKwuIqkXcLRy/0F49amaXvV+BrnJsd531Wq2oy\n\tIsKiMnEqOdAlHSdsyCq6nJ2ZrTHPgglEd8k1romOWuwWInuQYjvaWBp1X6osY2uE7L\n\tC1VN93ruRwFgG6ifwYX7VB9TJ+nG+8nSSFZvfcC0oyupGzGyLYGSLrmd7m5Oa3CWp5\n\tKbg3bCjSMIAAA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1684232666;\n\tbh=qY2IPPeJbHLGkDZRKozFbngjJmQCUrK8I4b+U66q/D0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l9VX7yklJYxJyg//YdKNk8IsWJviPYWDKL4WfBzDs31mkosdPx1pMnTb+ox7CHxp6\n\tTIGL77fhjwBKS/HysNXLiTLSrgDuNQ4zqWGfW0YsoIz7BqO+uyJNVsYG7eKBswnUnd\n\t1+lbncf48Rbr8gox0/JEC/kubdxcLfLL6jA/j6gU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"l9VX7ykl\"; dkim-atps=neutral","Date":"Tue, 16 May 2023 12:24:33 +0200","To":"Harvey Yang ","Message-ID":"<20230516102433.mtr3h4zn5squgbbe@lati>","References":"<20230516080459.711347-1-chenghaoyang@google.com>\n\t<20230516080459.711347-2-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230516080459.711347-2-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27110,"web_url":"https://patchwork.libcamera.org/comment/27110/","msgid":"<168423642196.3378917.13841249942170245375@Monstersaurus>","date":"2023-05-16T11:27:01","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2023-05-16 11:24:33)\n> Hi Harvey\n> On Tue, May 16, 2023 at 08:03:17AM +0000, Harvey Yang via libcamera-devel wrote:\n> > From: Cheng-Hao Yang \n> >\n> > As other components (like the WIP virtual pipeline handler) also need a\n> > heap allocator, move DmaHeap from raspberry pi pipeline handler to a\n> > general HeapAllocator as a base class.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > .../libcamera/internal/heap_allocator.h | 18 ++-\n> > include/libcamera/internal/meson.build | 1 +\n> > src/libcamera/heap_allocator.cpp | 128 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/pipeline/meson.build | 5 +-\n> > src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------\n> > src/libcamera/pipeline/rpi/vc4/meson.build | 5 +-\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +-\n> > 8 files changed, 145 insertions(+), 114 deletions(-)\n> > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (50%)\n> > create mode 100644 src/libcamera/heap_allocator.cpp\n> > delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/heap_allocator.h\n> > similarity index 50%\n> > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> > rename to include/libcamera/internal/heap_allocator.h\n> > index 0a4a8d86..d061fdce 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> > +++ b/include/libcamera/internal/heap_allocator.h\n> > @@ -1,8 +1,8 @@\n> > /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > /*\n> > - * Copyright (C) 2020, Raspberry Pi Ltd\n> > + * Copyright (C) 2023, Google Inc.\n> \n> Why can't you keep both ? If the code comes from RPi they copyright\n> should be retained.\n\nI'm sure we've been here before ;-)\n\nIndeed, please don't drop copyrights when moving code around.\n\n\n> \n> > *\n> > - * dma_heaps.h - Helper class for dma-heap allocations.\n> > + * heap_allocator.h - Helper class for heap buffer allocations.\n> > */\n> >\n> > #pragma once\n> > @@ -13,20 +13,18 @@\n> >\n> > namespace libcamera {\n> >\n> > -namespace RPi {\n> > +class Heap;\n> >\n> > -class DmaHeap\n> > +class HeapAllocator\n> > {\n> > public:\n> > - DmaHeap();\n> > - ~DmaHeap();\n> > - bool isValid() const { return dmaHeapHandle_.isValid(); }\n> > + HeapAllocator();\n> > + ~HeapAllocator();\n> > + bool isValid() const;\n> > UniqueFD alloc(const char *name, std::size_t size);\n> >\n> > private:\n> > - UniqueFD dmaHeapHandle_;\n> > + std::unique_ptr heap_;\n> > };\n> >\n> > -} /* namespace RPi */\n> > -\n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index d7508805..9d25c289 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -26,6 +26,7 @@ libcamera_internal_headers = files([\n> > 'device_enumerator_udev.h',\n> > 'formats.h',\n> > 'framebuffer.h',\n> > + 'heap_allocator.h',\n> > 'ipa_manager.h',\n> > 'ipa_module.h',\n> > 'ipa_proxy.h',\n> > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> > new file mode 100644\n> > index 00000000..e9476de5\n> > --- /dev/null\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -0,0 +1,128 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi Ltd\n> > + *\n> > + * heap_allocator.cpp - Helper class for heap buffer allocations.\n> > + */\n> > +\n> > +#include \"libcamera/internal/heap_allocator.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +/*\n> > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma\n> > + * to only have to worry about importing.\n> > + *\n> > + * Annoyingly, should the cma heap size be specified on the kernel command line\n> > + * instead of DT, the heap gets named \"reserved\" instead.\n> > + */\n> > +static constexpr std::array dmaHeapNames = {\n> > + \"/dev/dma_heap/linux,cma\",\n> > + \"/dev/dma_heap/reserved\"\n\nOn my x86 pc, I have \"/dev/dma_heap/system\" too. I don't know if we\nshoudl add that ... but maybe something to explore later.\n\nThis is fine to keep as it is though here I think.\n\n> > +};\n> > +\n> > +LOG_DEFINE_CATEGORY(HeapAllocator)\n> > +\n> > +class Heap\n> > +{\n> > +public:\n> > + virtual ~Heap() = default;\n> > + bool isValid() const { return handle_.isValid(); }\n> > + virtual UniqueFD alloc(const char *name, std::size_t size) = 0;\n> > +\n> > +protected:\n> > + UniqueFD handle_;\n> > +};\n> > +\n> > +class DmaHeap : public Heap\n> > +{\n> > +public:\n> > + DmaHeap();\n> > + ~DmaHeap();\n> > + UniqueFD alloc(const char *name, std::size_t size) override;\n> > +};\n> > +\n> > +DmaHeap::DmaHeap()\n> > +{\n> > + for (const char *name : dmaHeapNames) {\n> > + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Debug) << \"DmaHeap failed to open \" << name << \": \"\n> > + << strerror(ret);\n> > + continue;\n> > + }\n> > +\n> > + handle_ = UniqueFD(ret);\n> > + break;\n> > + }\n> > +\n> > + if (!handle_.isValid())\n> > + LOG(HeapAllocator, Error) << \"DmaHeap could not open any dmaHeap device\";\n> > +}\n> > +\n> > +DmaHeap::~DmaHeap() = default;\n> > +\n> > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > +{\n> > + int ret;\n> > +\n> > + if (!name)\n> > + return {};\n> > +\n> > + struct dma_heap_allocation_data alloc = {};\n> > +\n> > + alloc.len = size;\n> > + alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > +\n> > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > + if (ret < 0) {\n> > + LOG(HeapAllocator, Error) << \"DmaHeap allocation failure for \"\n> > + << name;\n> > + return {};\n> > + }\n> > +\n> > + UniqueFD allocFd(alloc.fd);\n> > + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> > + if (ret < 0) {\n> > + LOG(HeapAllocator, Error) << \"DmaHeap naming failure for \"\n> > + << name;\n> > + return {};\n> > + }\n> > +\n> > + return allocFd;\n> > +}\n> > +\n> > +HeapAllocator::HeapAllocator()\n> > +{\n> > + heap_ = std::make_unique();\n> > +}\n> > +\n> > +HeapAllocator::~HeapAllocator() = default;\n> > +\n> > +bool HeapAllocator::isValid() const\n> > +{\n> > + return heap_->isValid();\n> > +}\n> > +\n> > +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)\n> > +{\n> > + if (!isValid()) {\n> > + LOG(HeapAllocator, Fatal) << \"Allocation attempted without allocator\" << name;\n> > + return {};\n> > + }\n> > +\n> > + return heap_->alloc(name, size);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index d4385041..aa2d32a0 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -22,6 +22,7 @@ libcamera_sources = files([\n> > 'framebuffer.cpp',\n> > 'framebuffer_allocator.cpp',\n> > 'geometry.cpp',\n> > + 'heap_allocator.cpp',\n> > 'ipa_controls.cpp',\n> > 'ipa_data_serializer.cpp',\n> > 'ipa_interface.cpp',\n> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> > index 8a61991c..b6160d34 100644\n> > --- a/src/libcamera/pipeline/meson.build\n> > +++ b/src/libcamera/pipeline/meson.build\n> > @@ -12,9 +12,6 @@ foreach pipeline : pipelines\n> > continue\n> > endif\n> >\n> > - subdirs += pipeline\n> > subdir(pipeline)\n> > -\n> > - # Don't reuse the pipeline variable below, the subdirectory may have\n> > - # overwritten it.\n> > + subdirs += pipeline\n> \n> The comment you removed says exactly not to do this :)\n> \n> Why do you need this change ?\n\nI presume this is just a bad merge conflict ?\n\n> \n> \n> > endforeach\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> > deleted file mode 100644\n> > index 317b1fc1..00000000\n> > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> > +++ /dev/null\n> > @@ -1,90 +0,0 @@\n> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Raspberry Pi Ltd\n> > - *\n> > - * dma_heaps.h - Helper class for dma-heap allocations.\n> > - */\n> > -\n> > -#include \"dma_heaps.h\"\n> > -\n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -\n> > -#include \n> > -\n> > -/*\n> > - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma\n> > - * to only have to worry about importing.\n> > - *\n> > - * Annoyingly, should the cma heap size be specified on the kernel command line\n> > - * instead of DT, the heap gets named \"reserved\" instead.\n> > - */\n> > -static constexpr std::array heapNames = {\n> > - \"/dev/dma_heap/linux,cma\",\n> > - \"/dev/dma_heap/reserved\"\n> > -};\n> > -\n> > -namespace libcamera {\n> > -\n> > -LOG_DECLARE_CATEGORY(RPI)\n> > -\n> > -namespace RPi {\n> > -\n> > -DmaHeap::DmaHeap()\n> > -{\n> > - for (const char *name : heapNames) {\n> > - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> > - if (ret < 0) {\n> > - ret = errno;\n> > - LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> > - << strerror(ret);\n> > - continue;\n> > - }\n> > -\n> > - dmaHeapHandle_ = UniqueFD(ret);\n> > - break;\n> > - }\n> > -\n> > - if (!dmaHeapHandle_.isValid())\n> > - LOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> > -}\n> > -\n> > -DmaHeap::~DmaHeap() = default;\n> > -\n> > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > -{\n> > - int ret;\n> > -\n> > - if (!name)\n> > - return {};\n> > -\n> > - struct dma_heap_allocation_data alloc = {};\n> > -\n> > - alloc.len = size;\n> > - alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > -\n> > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > - if (ret < 0) {\n> > - LOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> > - << name;\n> > - return {};\n> > - }\n> > -\n> > - UniqueFD allocFd(alloc.fd);\n> > - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> > - if (ret < 0) {\n> > - LOG(RPI, Error) << \"dmaHeap naming failure for \"\n> > - << name;\n> > - return {};\n> > - }\n> > -\n> > - return allocFd;\n> > -}\n> > -\n> > -} /* namespace RPi */\n> > -\n> > -} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build\n> > index cdb049c5..b2b79735 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/meson.build\n> > +++ b/src/libcamera/pipeline/rpi/vc4/meson.build\n> > @@ -1,8 +1,5 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > -libcamera_sources += files([\n> > - 'dma_heaps.cpp',\n> > - 'vc4.cpp',\n> > -])\n> > +libcamera_sources += files(['vc4.cpp'])\n> >\n> > subdir('data')\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > index 018cf488..410ecfaf 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > @@ -12,12 +12,11 @@\n> > #include \n> >\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > +#include \"libcamera/internal/heap_allocator.h\"\n> >\n> > #include \"../common/pipeline_base.h\"\n> > #include \"../common/rpi_stream.h\"\n> >\n> > -#include \"dma_heaps.h\"\n> > -\n> > using namespace std::chrono_literals;\n> >\n> > namespace libcamera {\n> > @@ -87,7 +86,7 @@ public:\n> > RPi::Device isp_;\n> >\n> > /* DMAHEAP allocation helper. */\n> > - RPi::DmaHeap dmaHeap_;\n> > + HeapAllocator heapAllocator_;\n> > SharedFD lsTable_;\n> >\n> > struct Config {\n> > @@ -296,7 +295,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > {\n> > Vc4CameraData *data = static_cast(cameraData.get());\n> >\n> > - if (!data->dmaHeap_.isValid())\n> > + if (!data->heapAllocator_.isValid())\n> > return -ENOMEM;\n> >\n> > MediaEntity *unicamImage = unicam->getEntityByName(\"unicam-image\");\n> > @@ -670,9 +669,9 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)\n> > {\n> > params.ispControls = isp_[Isp::Input].dev()->controls();\n> >\n> > - /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > + /* Allocate the lens shading table via heapAllocator and pass to the IPA. */\n> > if (!lsTable_.isValid()) {\n> > - lsTable_ = SharedFD(dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize));\n> > + lsTable_ = SharedFD(heapAllocator_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize));\n> > if (!lsTable_.isValid())\n> > return -ENOMEM;\n> >\n> > --\n> > 2.40.1.606.ga4b1b128d6-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 12FE1C3284\n\tfor ;\n\tTue, 16 May 2023 11:27:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 945B4627D1;\n\tTue, 16 May 2023 13:27:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0DDD60399\n\tfor ;\n\tTue, 16 May 2023 13:27:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5A3AC7F8;\n\tTue, 16 May 2023 13:26:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684236426;\n\tbh=oZW+hvq/QAliURto7gry4F6qeSAa3S5muJjdnZnfjxQ=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=TIXvxAGH7FSxGWPtutfX7thGy4OHg+33ALmrdxb76Ka5ubzGSq0J3RN8mMnmyK0Zl\n\thgj7U2DrjEJfMSHQ26oNN0VwSiIHBKBsiHxM88qS360lqId6M77ZtSbED5etAXmxnB\n\tnMmGdRkVu3c7D3zsfSvrwadCdksS6/KUYuS5XwHP5pASWlez6pSLlcpopxAniENmjO\n\tfIQkfPz4dTqH2vwi4NexB1+N15ZnboYo1zxlTROCU/gtakPo8CHNs9KhAFYh6Tq+y/\n\tYvHSRej/c0BEjKI2jF2ymnRPB5lTIsDpxZSuLLS6Zx2ayAiRe16R6XtZw1r3atE9ZK\n\thVk+7eTkMb+Lg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1684236413;\n\tbh=oZW+hvq/QAliURto7gry4F6qeSAa3S5muJjdnZnfjxQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=m951sw6AUfAsIAoz/sUSx6QqLvZ795/eaXU+vSgG2IawUYiWikoK9EUU8peDbQ4pj\n\tVNxKEXjktNeisO+DNFdmu8i2nXOLT43QIZGAdmb89O4CV7ztwLeVXe6nj+zdb1L0xn\n\tiPFDVU3R/uYG9REJBrP4BxjR6+a3/rezcxi0cq3k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"m951sw6A\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230516102433.mtr3h4zn5squgbbe@lati>","References":"<20230516080459.711347-1-chenghaoyang@google.com>\n\t<20230516080459.711347-2-chenghaoyang@google.com>\n\t<20230516102433.mtr3h4zn5squgbbe@lati>","To":"Harvey Yang ,\n\tJacopo Mondi ,\n\tJacopo Mondi via libcamera-devel ","Date":"Tue, 16 May 2023 12:27:01 +0100","Message-ID":"<168423642196.3378917.13841249942170245375@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27126,"web_url":"https://patchwork.libcamera.org/comment/27126/","msgid":"","date":"2023-05-22T08:37:36","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Kieran and Jacopo for another round of review!\n\nOn Tue, May 16, 2023 at 7:27 PM Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Jacopo Mondi via libcamera-devel (2023-05-16 11:24:33)\n> > Hi Harvey\n> > On Tue, May 16, 2023 at 08:03:17AM +0000, Harvey Yang via\n> libcamera-devel wrote:\n> > > From: Cheng-Hao Yang \n> > >\n> > > As other components (like the WIP virtual pipeline handler) also need a\n> > > heap allocator, move DmaHeap from raspberry pi pipeline handler to a\n> > > general HeapAllocator as a base class.\n> > >\n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > .../libcamera/internal/heap_allocator.h | 18 ++-\n> > > include/libcamera/internal/meson.build | 1 +\n> > > src/libcamera/heap_allocator.cpp | 128 ++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > src/libcamera/pipeline/meson.build | 5 +-\n> > > src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 ------------\n> > > src/libcamera/pipeline/rpi/vc4/meson.build | 5 +-\n> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +-\n> > > 8 files changed, 145 insertions(+), 114 deletions(-)\n> > > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h =>\n> include/libcamera/internal/heap_allocator.h (50%)\n> > > create mode 100644 src/libcamera/heap_allocator.cpp\n> > > delete mode 100644 src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> b/include/libcamera/internal/heap_allocator.h\n> > > similarity index 50%\n> > > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> > > rename to include/libcamera/internal/heap_allocator.h\n> > > index 0a4a8d86..d061fdce 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> > > +++ b/include/libcamera/internal/heap_allocator.h\n> > > @@ -1,8 +1,8 @@\n> > > /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > /*\n> > > - * Copyright (C) 2020, Raspberry Pi Ltd\n> > > + * Copyright (C) 2023, Google Inc.\n> >\n> > Why can't you keep both ? If the code comes from RPi they copyright\n> > should be retained.\n>\nI'm sure we've been here before ;-)\n>\n> Indeed, please don't drop copyrights when moving code around.\n>\n>\n\nI see. I thought keeping RPi copyright in heap_allocator.cpp is enough, as\nthat's where DmaHeap stays in this patch. The git mv is just auto generated\nas they look pretty much the same.\n\nI'll keep both copyrights in heap_allocator.h & heap_allocator.cpp, as the\nAPIs\nof HeapAllocator are basically the same as those of DeaHeap.\n\n\n>\n> >\n> > > *\n> > > - * dma_heaps.h - Helper class for dma-heap allocations.\n> > > + * heap_allocator.h - Helper class for heap buffer allocations.\n> > > */\n> > >\n> > > #pragma once\n> > > @@ -13,20 +13,18 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > -namespace RPi {\n> > > +class Heap;\n> > >\n> > > -class DmaHeap\n> > > +class HeapAllocator\n> > > {\n> > > public:\n> > > - DmaHeap();\n> > > - ~DmaHeap();\n> > > - bool isValid() const { return dmaHeapHandle_.isValid(); }\n> > > + HeapAllocator();\n> > > + ~HeapAllocator();\n> > > + bool isValid() const;\n> > > UniqueFD alloc(const char *name, std::size_t size);\n> > >\n> > > private:\n> > > - UniqueFD dmaHeapHandle_;\n> > > + std::unique_ptr heap_;\n> > > };\n> > >\n> > > -} /* namespace RPi */\n> > > -\n> > > } /* namespace libcamera */\n> > > diff --git a/include/libcamera/internal/meson.build\n> b/include/libcamera/internal/meson.build\n> > > index d7508805..9d25c289 100644\n> > > --- a/include/libcamera/internal/meson.build\n> > > +++ b/include/libcamera/internal/meson.build\n> > > @@ -26,6 +26,7 @@ libcamera_internal_headers = files([\n> > > 'device_enumerator_udev.h',\n> > > 'formats.h',\n> > > 'framebuffer.h',\n> > > + 'heap_allocator.h',\n> > > 'ipa_manager.h',\n> > > 'ipa_module.h',\n> > > 'ipa_proxy.h',\n> > > diff --git a/src/libcamera/heap_allocator.cpp\n> b/src/libcamera/heap_allocator.cpp\n> > > new file mode 100644\n> > > index 00000000..e9476de5\n> > > --- /dev/null\n> > > +++ b/src/libcamera/heap_allocator.cpp\n> > > @@ -0,0 +1,128 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Raspberry Pi Ltd\n> > > + *\n> > > + * heap_allocator.cpp - Helper class for heap buffer allocations.\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/heap_allocator.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/*\n> > > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows\n> dmaheap-cma\n> > > + * to only have to worry about importing.\n> > > + *\n> > > + * Annoyingly, should the cma heap size be specified on the kernel\n> command line\n> > > + * instead of DT, the heap gets named \"reserved\" instead.\n> > > + */\n> > > +static constexpr std::array dmaHeapNames = {\n> > > + \"/dev/dma_heap/linux,cma\",\n> > > + \"/dev/dma_heap/reserved\"\n>\n> On my x86 pc, I have \"/dev/dma_heap/system\" too. I don't know if we\n> shoudl add that ... but maybe something to explore later.\n>\n> This is fine to keep as it is though here I think.\n>\n>\nActually the mediatek device (the new board) uses \"/dev/dma_heap/system\" as\nwell. Let's add it now :)\n\n\n> > > +};\n> > > +\n> > > +LOG_DEFINE_CATEGORY(HeapAllocator)\n> > > +\n> > > +class Heap\n> > > +{\n> > > +public:\n> > > + virtual ~Heap() = default;\n> > > + bool isValid() const { return handle_.isValid(); }\n> > > + virtual UniqueFD alloc(const char *name, std::size_t size) = 0;\n> > > +\n> > > +protected:\n> > > + UniqueFD handle_;\n> > > +};\n> > > +\n> > > +class DmaHeap : public Heap\n> > > +{\n> > > +public:\n> > > + DmaHeap();\n> > > + ~DmaHeap();\n> > > + UniqueFD alloc(const char *name, std::size_t size) override;\n> > > +};\n> > > +\n> > > +DmaHeap::DmaHeap()\n> > > +{\n> > > + for (const char *name : dmaHeapNames) {\n> > > + int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> > > + if (ret < 0) {\n> > > + ret = errno;\n> > > + LOG(HeapAllocator, Debug) << \"DmaHeap failed to\n> open \" << name << \": \"\n> > > + << strerror(ret);\n> > > + continue;\n> > > + }\n> > > +\n> > > + handle_ = UniqueFD(ret);\n> > > + break;\n> > > + }\n> > > +\n> > > + if (!handle_.isValid())\n> > > + LOG(HeapAllocator, Error) << \"DmaHeap could not open any\n> dmaHeap device\";\n> > > +}\n> > > +\n> > > +DmaHeap::~DmaHeap() = default;\n> > > +\n> > > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > > +{\n> > > + int ret;\n> > > +\n> > > + if (!name)\n> > > + return {};\n> > > +\n> > > + struct dma_heap_allocation_data alloc = {};\n> > > +\n> > > + alloc.len = size;\n> > > + alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > > +\n> > > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > > + if (ret < 0) {\n> > > + LOG(HeapAllocator, Error) << \"DmaHeap allocation failure\n> for \"\n> > > + << name;\n> > > + return {};\n> > > + }\n> > > +\n> > > + UniqueFD allocFd(alloc.fd);\n> > > + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> > > + if (ret < 0) {\n> > > + LOG(HeapAllocator, Error) << \"DmaHeap naming failure for\n> \"\n> > > + << name;\n> > > + return {};\n> > > + }\n> > > +\n> > > + return allocFd;\n> > > +}\n> > > +\n> > > +HeapAllocator::HeapAllocator()\n> > > +{\n> > > + heap_ = std::make_unique();\n> > > +}\n> > > +\n> > > +HeapAllocator::~HeapAllocator() = default;\n> > > +\n> > > +bool HeapAllocator::isValid() const\n> > > +{\n> > > + return heap_->isValid();\n> > > +}\n> > > +\n> > > +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)\n> > > +{\n> > > + if (!isValid()) {\n> > > + LOG(HeapAllocator, Fatal) << \"Allocation attempted\n> without allocator\" << name;\n> > > + return {};\n> > > + }\n> > > +\n> > > + return heap_->alloc(name, size);\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index d4385041..aa2d32a0 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -22,6 +22,7 @@ libcamera_sources = files([\n> > > 'framebuffer.cpp',\n> > > 'framebuffer_allocator.cpp',\n> > > 'geometry.cpp',\n> > > + 'heap_allocator.cpp',\n> > > 'ipa_controls.cpp',\n> > > 'ipa_data_serializer.cpp',\n> > > 'ipa_interface.cpp',\n> > > diff --git a/src/libcamera/pipeline/meson.build\n> b/src/libcamera/pipeline/meson.build\n> > > index 8a61991c..b6160d34 100644\n> > > --- a/src/libcamera/pipeline/meson.build\n> > > +++ b/src/libcamera/pipeline/meson.build\n> > > @@ -12,9 +12,6 @@ foreach pipeline : pipelines\n> > > continue\n> > > endif\n> > >\n> > > - subdirs += pipeline\n> > > subdir(pipeline)\n> > > -\n> > > - # Don't reuse the pipeline variable below, the subdirectory may\n> have\n> > > - # overwritten it.\n> > > + subdirs += pipeline\n> >\n> > The comment you removed says exactly not to do this :)\n> >\n> > Why do you need this change ?\n>\n> I presume this is just a bad merge conflict ?\n>\n>\nYeah Kieran guess it right... Fixed.\n\n\n> >\n> >\n> > > endforeach\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> > > deleted file mode 100644\n> > > index 317b1fc1..00000000\n> > > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> > > +++ /dev/null\n> > > @@ -1,90 +0,0 @@\n> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > -/*\n> > > - * Copyright (C) 2020, Raspberry Pi Ltd\n> > > - *\n> > > - * dma_heaps.h - Helper class for dma-heap allocations.\n> > > - */\n> > > -\n> > > -#include \"dma_heaps.h\"\n> > > -\n> > > -#include \n> > > -#include \n> > > -#include \n> > > -#include \n> > > -#include \n> > > -#include \n> > > -\n> > > -#include \n> > > -\n> > > -/*\n> > > - * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows\n> dmaheap-cma\n> > > - * to only have to worry about importing.\n> > > - *\n> > > - * Annoyingly, should the cma heap size be specified on the kernel\n> command line\n> > > - * instead of DT, the heap gets named \"reserved\" instead.\n> > > - */\n> > > -static constexpr std::array heapNames = {\n> > > - \"/dev/dma_heap/linux,cma\",\n> > > - \"/dev/dma_heap/reserved\"\n> > > -};\n> > > -\n> > > -namespace libcamera {\n> > > -\n> > > -LOG_DECLARE_CATEGORY(RPI)\n> > > -\n> > > -namespace RPi {\n> > > -\n> > > -DmaHeap::DmaHeap()\n> > > -{\n> > > - for (const char *name : heapNames) {\n> > > - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> > > - if (ret < 0) {\n> > > - ret = errno;\n> > > - LOG(RPI, Debug) << \"Failed to open \" << name <<\n> \": \"\n> > > - << strerror(ret);\n> > > - continue;\n> > > - }\n> > > -\n> > > - dmaHeapHandle_ = UniqueFD(ret);\n> > > - break;\n> > > - }\n> > > -\n> > > - if (!dmaHeapHandle_.isValid())\n> > > - LOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> > > -}\n> > > -\n> > > -DmaHeap::~DmaHeap() = default;\n> > > -\n> > > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > > -{\n> > > - int ret;\n> > > -\n> > > - if (!name)\n> > > - return {};\n> > > -\n> > > - struct dma_heap_allocation_data alloc = {};\n> > > -\n> > > - alloc.len = size;\n> > > - alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > > -\n> > > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC,\n> &alloc);\n> > > - if (ret < 0) {\n> > > - LOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> > > - << name;\n> > > - return {};\n> > > - }\n> > > -\n> > > - UniqueFD allocFd(alloc.fd);\n> > > - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> > > - if (ret < 0) {\n> > > - LOG(RPI, Error) << \"dmaHeap naming failure for \"\n> > > - << name;\n> > > - return {};\n> > > - }\n> > > -\n> > > - return allocFd;\n> > > -}\n> > > -\n> > > -} /* namespace RPi */\n> > > -\n> > > -} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build\n> b/src/libcamera/pipeline/rpi/vc4/meson.build\n> > > index cdb049c5..b2b79735 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/meson.build\n> > > +++ b/src/libcamera/pipeline/rpi/vc4/meson.build\n> > > @@ -1,8 +1,5 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > >\n> > > -libcamera_sources += files([\n> > > - 'dma_heaps.cpp',\n> > > - 'vc4.cpp',\n> > > -])\n> > > +libcamera_sources += files(['vc4.cpp'])\n> > >\n> > > subdir('data')\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > index 018cf488..410ecfaf 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > @@ -12,12 +12,11 @@\n> > > #include \n> > >\n> > > #include \"libcamera/internal/device_enumerator.h\"\n> > > +#include \"libcamera/internal/heap_allocator.h\"\n> > >\n> > > #include \"../common/pipeline_base.h\"\n> > > #include \"../common/rpi_stream.h\"\n> > >\n> > > -#include \"dma_heaps.h\"\n> > > -\n> > > using namespace std::chrono_literals;\n> > >\n> > > namespace libcamera {\n> > > @@ -87,7 +86,7 @@ public:\n> > > RPi::Device isp_;\n> > >\n> > > /* DMAHEAP allocation helper. */\n> > > - RPi::DmaHeap dmaHeap_;\n> > > + HeapAllocator heapAllocator_;\n> > > SharedFD lsTable_;\n> > >\n> > > struct Config {\n> > > @@ -296,7 +295,7 @@ int\n> PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > > {\n> > > Vc4CameraData *data = static_cast *>(cameraData.get());\n> > >\n> > > - if (!data->dmaHeap_.isValid())\n> > > + if (!data->heapAllocator_.isValid())\n> > > return -ENOMEM;\n> > >\n> > > MediaEntity *unicamImage =\n> unicam->getEntityByName(\"unicam-image\");\n> > > @@ -670,9 +669,9 @@ int\n> Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)\n> > > {\n> > > params.ispControls = isp_[Isp::Input].dev()->controls();\n> > >\n> > > - /* Allocate the lens shading table via dmaHeap and pass to the\n> IPA. */\n> > > + /* Allocate the lens shading table via heapAllocator and pass to\n> the IPA. */\n> > > if (!lsTable_.isValid()) {\n> > > - lsTable_ = SharedFD(dmaHeap_.alloc(\"ls_grid\",\n> ipa::RPi::MaxLsGridSize));\n> > > + lsTable_ = SharedFD(heapAllocator_.alloc(\"ls_grid\",\n> ipa::RPi::MaxLsGridSize));\n> > > if (!lsTable_.isValid())\n> > > return -ENOMEM;\n> > >\n> > > --\n> > > 2.40.1.606.ga4b1b128d6-goog\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B2CFAC31E9\n\tfor ;\n\tMon, 22 May 2023 08:37:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6962962861;\n\tMon, 22 May 2023 10:37:50 +0200 (CEST)","from mail-ua1-x92c.google.com (mail-ua1-x92c.google.com\n\t[IPv6:2607:f8b0:4864:20::92c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6350160536\n\tfor ;\n\tMon, 22 May 2023 10:37:48 +0200 (CEST)","by mail-ua1-x92c.google.com with SMTP id\n\ta1e0cc1a2514c-784205f0058so1042994241.1\n\tfor ;\n\tMon, 22 May 2023 01:37:48 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684744670;\n\tbh=RCWMeFp2NW4TQ1rileeHLaf7GdONSPsxBJfqDb1QdIs=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CfRT6N7DFXleH6PkTdO8tgcUl906PmTVsXn2ED9A36YjThT1JhN6Q2ZJEI/4STpmz\n\tuPmYcjGt1Gb0qoNVVLtfMyhsZQiBRcrDDKvZz7G7i73ZgngkGIZqypk7clLg4IU1ER\n\tWUxfaCLvRB3XEXLzTA3gE7uof34zbYLN3FEfCzTPF+VeR8/Byj8NGIiYi3p0df44SU\n\tdoT/+6ZkkGEFr2HtjPF5fXGw3ZTm1Kon5rOWXiGHthRZJau7fsNw+kJCXGQN7CpRMV\n\tmVZVBtHxISMLYA7Wvo7yr+rneqQUf86QpsgWxHV5FcVFz1eRAVQgz2QRXKBFhfH9Yj\n\tqd4fv91JIy6Fw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1684744667; x=1687336667;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=fgxRDSZjF4FScXX16cefsS5eE1iSmzI0iBlVdthjAxo=;\n\tb=ltCLkBG4xNNA/NKtcbaFOa9ISs4VXC52hVG8zfReYa0Jdws3YnCnpm72iqUiEl79da\n\tYBZ7TX2w5KwhHN/27ZBfBecfxcYeFh0Tk2rtGmNj+k7eFKmy9V+hatJlXwlDCd2RvNlb\n\ttTRU7OycaUBW76FjvrcPOgVo5X/pmRvagyr5g="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"ltCLkBG4\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1684744667; x=1687336667;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=fgxRDSZjF4FScXX16cefsS5eE1iSmzI0iBlVdthjAxo=;\n\tb=KCOqssOjPgJGmHNdU07bHSdx+Mn71E4t5ZpCBuayR6mKZwcMS4O3Og/KXzu2WC/q+1\n\tjHhrK/FzcOYKpgLNBxbXVTv/bRPE0Yyx783SitBxKHey9HVwFXPnDZyLj0JOYmAwnlvD\n\tP1bxJLH/hlreNo7QOJfhNB+d5WRlTxgkggwcKSDjyvT3OrmMOMYnUcwXM2sNY+aJkL4p\n\tipkJYxiV/XD9o1zs6TaeNWcexGcS9ReOG404hsaNxyFgEX9L+T6UZf6VbSw9dtGhJewv\n\tdGm5/fszw5BiY6Bpf0OWAkJEsVx/X4pZDzz4A+CcPzAkjEmcbAdKY3sic6/bRaXpR3bN\n\tIRWA==","X-Gm-Message-State":"AC+VfDxu1NCZTJmryVjP3qbZiYMSDFNqeNDy0i7D5IkiyNPcOd1bGuJD\n\trCm0IvaRWd0LGM6fxAs6BzF5yjxJs/QDYUrlcg8e/Z+Bh4Fzm5pcaN0=","X-Google-Smtp-Source":"ACHHUZ5vy84T45GHOjlncqn6FqwBqlb+TEeyhZy48j9n95NqxLziCzEdSePfeRGt4BPx9xOVq416LQ8ppbEM7m3mEYE=","X-Received":"by 2002:a67:eb47:0:b0:436:1156:ff5d with SMTP id\n\tx7-20020a67eb47000000b004361156ff5dmr2071767vso.29.1684744667254;\n\tMon, 22 May 2023 01:37:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20230516080459.711347-1-chenghaoyang@google.com>\n\t<20230516080459.711347-2-chenghaoyang@google.com>\n\t<20230516102433.mtr3h4zn5squgbbe@lati>\n\t<168423642196.3378917.13841249942170245375@Monstersaurus>","In-Reply-To":"<168423642196.3378917.13841249942170245375@Monstersaurus>","Date":"Mon, 22 May 2023 16:37:36 +0800","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000006de86405fc442f33\"","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]