[{"id":27148,"web_url":"https://patchwork.libcamera.org/comment/27148/","msgid":"","date":"2023-05-29T07:50:33","subject":"Re: [libcamera-devel] [PATCH v6 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\n\nOn Mon, May 22, 2023 at 08:35:06AM +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 | 17 ++-\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\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> 7 files changed, 146 insertions(+), 109 deletions(-)\n> rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (57%)\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 57%\n> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> rename to include/libcamera/internal/heap_allocator.h\n> index 0a4a8d86..92d4488a 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> +++ b/include/libcamera/internal/heap_allocator.h\n> @@ -1,8 +1,9 @@\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> - * 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 +14,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..69f65062\n> --- /dev/null\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -0,0 +1,130 @@\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> + * 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> +\t\"/dev/dma_heap/system\"\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\nDo you need the third argument ?\nIsn't it meaningful only if O_CREAT is part of the flags ?\n\n\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\nThis all looks nice, but you're missing all the documentation for this\nand the next patches.\n\nThis is the list of Doxygen warnings I get\n\n[3/13] Generating Documentation/doxygen with a custom command\nlibcamera.git/src/libcamera/heap_allocator.cpp:59: warning: Compound libcamera::DmaHeap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:48: warning: Compound libcamera::Heap is not documented.\nlibcamera.git/include/libcamera/internal/heap_allocator.h:23: warning: Compound libcamera::HeapAllocator is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:67: warning: Compound libcamera::UdmaHeap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:64: warning: Member alloc(const char *name, std::size_t size) override (function) of class libcamera::DmaHeap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:52: warning: Member isValid() const (function) of class libcamera::Heap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:53: warning: Member alloc(const char *name, std::size_t size)=0 (function) of class libcamera::Heap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:56: warning: Member handle_ (variable) of class libcamera::Heap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:52: warning: Member isValid() const (function) of class libcamera::Heap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:53: warning: Member alloc(const char *name, std::size_t size)=0 (function) of class libcamera::Heap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:56: warning: Member handle_ (variable) of class libcamera::Heap is not documented.\nlibcamera.git/include/libcamera/internal/heap_allocator.h:28: warning: Member isValid() const (function) of class libcamera::HeapAllocator is not documented.\nlibcamera.git/include/libcamera/internal/heap_allocator.h:29: warning: Member alloc(const char *name, std::size_t size) (function) of class libcamera::HeapAllocator is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:72: warning: Member alloc(const char *name, std::size_t size) override (function) of class libcamera::UdmaHeap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:52: warning: Member isValid() const (function) of class libcamera::Heap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:53: warning: Member alloc(const char *name, std::size_t size)=0 (function) of class libcamera::Heap is not documented.\nlibcamera.git/src/libcamera/heap_allocator.cpp:56: warning: Member handle_ (variable) of class libcamera::Heap is not documented.\n\nThanks\n j\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/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.698.g37aff9b760-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 4B65AC31E9\n\tfor ;\n\tMon, 29 May 2023 07:50:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F305626FA;\n\tMon, 29 May 2023 09:50:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BAEC60599\n\tfor ;\n\tMon, 29 May 2023 09:50: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 7C252836;\n\tMon, 29 May 2023 09:50:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685346639;\n\tbh=DkEqxJm7XCvDy9IrhZNwVXwA4/LBLF2zzSXmQztWX64=;\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=dTKa0SbWWBrGe8tpJsLg+DeWyQHJd2Autbp9NwgI2YuqjgV1PKo9VSBYRQ0vvIVrL\n\tj5PcCkQ7rD0JsLJChbuWLpgLnD0gywKP3tf7KgTipZzeVV6/GvRjU+Bo3PlQdzZF4A\n\tAXTTWMpCEpaalh6k8UPpSq1xUZs3BZyOIKh9kJujiGNHvHBsn+qadduWbT9b+9nLDg\n\tRR+ow2wFWM4OVPTAexngSQt8rJ2Wy/f5l3NH+TYcQJrTDbdFAnkK78G+kj7QpLqdzY\n\tmvZj/9E9nPTNGV+20RHiOTKUpfC1NzsgMsXYQ883cnl+xbnKUa8Av41aAk3z3fF+OD\n\t8nkldoQS44rqg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685346617;\n\tbh=DkEqxJm7XCvDy9IrhZNwVXwA4/LBLF2zzSXmQztWX64=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mhodHxpL6qF9hFkO9hrGuV+EG8A5Xt2g5vfi2lIOBxiCu35M4lIAxq56CKEJza4N0\n\ti9D5yDOI+VIP4KSjDsSMNcl09T6JgoQ1k68pymtBzriedbwKAkL6D2xgkini9Fqyxg\n\tShUSoTuB2Hiz38g1keqCooEwj0Op/v78HMoN1THc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mhodHxpL\"; dkim-atps=neutral","Date":"Mon, 29 May 2023 09:50:33 +0200","To":"Harvey Yang ","Message-ID":"","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-2-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230522083546.2465448-2-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v6 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":27167,"web_url":"https://patchwork.libcamera.org/comment/27167/","msgid":"<4311e3be-d23f-d16a-ea11-c3991ec22086@ideasonboard.com>","date":"2023-05-30T06:08:33","subject":"Re: [libcamera-devel] [PATCH v6 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Harvey,\n\nThank you for the patch.\n\nOn 5/22/23 2:05 PM, 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 | 17 ++-\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\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> 7 files changed, 146 insertions(+), 109 deletions(-)\n> rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => include/libcamera/internal/heap_allocator.h (57%)\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 57%\n> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> rename to include/libcamera/internal/heap_allocator.h\n> index 0a4a8d86..92d4488a 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> +++ b/include/libcamera/internal/heap_allocator.h\n> @@ -1,8 +1,9 @@\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> - * 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 +14,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..69f65062\n> --- /dev/null\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -0,0 +1,130 @@\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> + * 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> +\t\"/dev/dma_heap/system\"\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\nThis should be automatically generated no?\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\nSame here\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/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>","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 2831FC3213\n\tfor ;\n\tTue, 30 May 2023 06:08:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CA616038F;\n\tTue, 30 May 2023 08:08:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B931E6038F\n\tfor ;\n\tTue, 30 May 2023 08:08:38 +0200 (CEST)","from [192.168.1.108] (unknown [103.86.18.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B4A65E4;\n\tTue, 30 May 2023 08:08:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685426921;\n\tbh=YGIomkJUU3Wdw5X7sNI8HMkKkcYMN+CqIWgUWGRxhv0=;\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:\n\tFrom;\n\tb=nOEx6SwdT5SXgbdSKjx4iGPYw2iXokAroBO79pU7Lda4w5ydsmxppFBG+EcJBigAR\n\tkiOVbdGjdIyoekRvxkb6Oal6Ax4FpdeF8cYSmH4PKL8eOtpv0AnmdQtL8Iaf3/vQH8\n\tcnDqkuahVCEInZLsrxVVav3E/Gku8Mu8opX5b2zKyMtoPn3XonwHfd7h85K4in8pBq\n\tLK8agCmsw4BQ/eT1cfYj/cZxAwh3uMHrrLQ6fX/PJOavSvAM8ACrp/5WToXxZ2xOBu\n\twll7t4op+gRTAFOt8NosXBOwuR7tTVuhXAtCITL5SJN/a4EImJTwXvahSpHsoOosyr\n\t3gaXaCpOjk32A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685426898;\n\tbh=YGIomkJUU3Wdw5X7sNI8HMkKkcYMN+CqIWgUWGRxhv0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=qMrWFkqM4WrnXsPeoLHGtfeaMnSBZfoad0xH/GNXbkAuLngtMwyto8orcRho52iwn\n\tFXhMCRI1FHEkvlYBHkbsf3xaBvCpTjs580o+g5GHgccfVGBhAEKsNc0zTBYTs9mTT8\n\ts3GXXGPVfLeH5TuisxEOu0no3Q8FGdalLCfYZTxE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qMrWFkqM\"; dkim-atps=neutral","Message-ID":"<4311e3be-d23f-d16a-ea11-c3991ec22086@ideasonboard.com>","Date":"Tue, 30 May 2023 11:38:33 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-2-chenghaoyang@google.com>","In-Reply-To":"<20230522083546.2465448-2-chenghaoyang@google.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v6 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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27188,"web_url":"https://patchwork.libcamera.org/comment/27188/","msgid":"<44d405ab-fbdd-2600-490f-106aad334a5e@ideasonboard.com>","date":"2023-05-31T06:23:24","subject":"Re: [libcamera-devel] [PATCH v6 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Harvey\n\nOn 5/30/23 11:38 AM, Umang Jain via libcamera-devel wrote:\n> Hi Harvey,\n>\n> Thank you for the patch.\n>\n> On 5/22/23 2:05 PM, 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       |  17 ++-\n>>   include/libcamera/internal/meson.build        |   1 +\n>>   src/libcamera/heap_allocator.cpp              | 130 ++++++++++++++++++\n>>   src/libcamera/meson.build                     |   1 +\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>>   7 files changed, 146 insertions(+), 109 deletions(-)\n>>   rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h => \n>> include/libcamera/internal/heap_allocator.h (57%)\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 57%\n>> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n>> rename to include/libcamera/internal/heap_allocator.h\n>> index 0a4a8d86..92d4488a 100644\n>> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n>> +++ b/include/libcamera/internal/heap_allocator.h\n>> @@ -1,8 +1,9 @@\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>> - * dma_heaps.h - Helper class for dma-heap allocations.\n>> + * heap_allocator.h - Helper class for heap buffer allocations.\n>>    */\n>>     #pragma once\n>> @@ -13,20 +14,18 @@\n>>     namespace libcamera {\n>>   -namespace RPi {\n>> +class Heap;\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>>     private:\n>> -    UniqueFD dmaHeapHandle_;\n>> +    std::unique_ptr heap_;\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..69f65062\n>> --- /dev/null\n>> +++ b/src/libcamera/heap_allocator.cpp\n>> @@ -0,0 +1,130 @@\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>> + * 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>> +    \"/dev/dma_heap/system\"\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 \" \n>> << 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> This should be automatically generated no?\n\nSorry, this is actually correct. I got confused between \nimplicitly-declared  destructor vs  implicit user-defined desctructor.\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> Same here\n\nSame applies here. Sorry for the noise!\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 \n>> 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/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>> -                    << 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 \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>>   -libcamera_sources += files([\n>> -    'dma_heaps.cpp',\n>> -    'vc4.cpp',\n>> -])\n>> +libcamera_sources += files(['vc4.cpp'])\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>>     #include \"libcamera/internal/device_enumerator.h\"\n>> +#include \"libcamera/internal/heap_allocator.h\"\n>>     #include \"../common/pipeline_base.h\"\n>>   #include \"../common/rpi_stream.h\"\n>>   -#include \"dma_heaps.h\"\n>> -\n>>   using namespace std::chrono_literals;\n>>     namespace libcamera {\n>> @@ -87,7 +86,7 @@ public:\n>>       RPi::Device isp_;\n>>         /* DMAHEAP allocation helper. */\n>> -    RPi::DmaHeap dmaHeap_;\n>> +    HeapAllocator heapAllocator_;\n>>       SharedFD lsTable_;\n>>         struct Config {\n>> @@ -296,7 +295,7 @@ int \n>> PipelineHandlerVc4::platformRegister(std::unique_ptr \n>> &camer\n>>   {\n>>       Vc4CameraData *data = static_cast> *>(cameraData.get());\n>>   -    if (!data->dmaHeap_.isValid())\n>> +    if (!data->heapAllocator_.isValid())\n>>           return -ENOMEM;\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>>   -    /* 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>","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 E144FC3213\n\tfor ;\n\tWed, 31 May 2023 06:23:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F9BB6055A;\n\tWed, 31 May 2023 08:23:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2FD86055A\n\tfor ;\n\tWed, 31 May 2023 08:23:30 +0200 (CEST)","from [192.168.1.108] (unknown [103.86.18.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B5E2FB;\n\tWed, 31 May 2023 08:23:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685514213;\n\tbh=+7zG50UFZhkMGL2/cXBFb2MB+Y1kdMC691ZTJE3x9QI=;\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:\n\tFrom;\n\tb=ue0Gfo9DqKeradEBJ2RVENjiAN7Aa2HYzQNWVOVwHJTRPNPcuthqiuFNn0xOjiiuG\n\tBIg2/NPmBfMHunNGgurNkgd7WbvsYMH7cDnfSqxC+fDVkxKQOTPEv/oEMMWb3zco60\n\tpLIEeq3gVviJoHWOKp25JZ+1nCwctoWWNKtY8A16A1JlN2Pts2h+Y8sWtrOCTt2lzj\n\tizwYmppLunL9s/o3Ywkn2OiYMCMgwhhsYHPgKkC8VEyC75e0BtqB29w7pPnJrbuHDg\n\tysSKHLA19MrsfwZAODeIFSsYn5dhCQx94h4wwEzUaFUOZbsv+W0CUiU0RWkQDlAkgq\n\t+llqw8rwhbPlg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685514189;\n\tbh=+7zG50UFZhkMGL2/cXBFb2MB+Y1kdMC691ZTJE3x9QI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=PhWr1sGelJNeqPCQTu9uz/AQ0SMdcG5naUa9l+JFCIvzjHMoJr9PaiCvwRePX+Km7\n\tJ31NjEHckfh0HGXH/6cSuMzfYjwlnjsOob9zpERTqN3m9o5/WAmORxaMhAwsgwv5Y4\n\t1eewYH97tlbYk5/IUnGCHrG51cNu0mYuE3nOTsrw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PhWr1sGe\"; dkim-atps=neutral","Message-ID":"<44d405ab-fbdd-2600-490f-106aad334a5e@ideasonboard.com>","Date":"Wed, 31 May 2023 11:53:24 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-2-chenghaoyang@google.com>\n\t<4311e3be-d23f-d16a-ea11-c3991ec22086@ideasonboard.com>","Content-Language":"en-US","In-Reply-To":"<4311e3be-d23f-d16a-ea11-c3991ec22086@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27650,"web_url":"https://patchwork.libcamera.org/comment/27650/","msgid":"","date":"2023-08-02T07:06:23","subject":"Re: [libcamera-devel] [PATCH v6 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 Umang and Jacopo!\n\nOn Tue, May 30, 2023 at 2:08 PM Umang Jain \nwrote:\n\n> Hi Harvey,\n>\n> Thank you for the patch.\n>\n> On 5/22/23 2:05 PM, 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 | 17 ++-\n> > include/libcamera/internal/meson.build | 1 +\n> > src/libcamera/heap_allocator.cpp | 130 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\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> > 7 files changed, 146 insertions(+), 109 deletions(-)\n> > rename src/libcamera/pipeline/rpi/vc4/dma_heaps.h =>\n> include/libcamera/internal/heap_allocator.h (57%)\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 57%\n> > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> > rename to include/libcamera/internal/heap_allocator.h\n> > index 0a4a8d86..92d4488a 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> > +++ b/include/libcamera/internal/heap_allocator.h\n> > @@ -1,8 +1,9 @@\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> > - * 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 +14,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..69f65062\n> > --- /dev/null\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -0,0 +1,130 @@\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> > + * 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> > + \"/dev/dma_heap/system\"\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> This should be automatically generated no?\n\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> > + << 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> Same here\n\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\n> 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/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>\n\nRemoved the third argument based on Jacopo's comment.\n(I only copied and pasted from the previous implementation though :p)\n\n\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, &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 = 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>\nUpdated with some documents. Please feel free to directly modify it when\nlanding. I'm very bad at documenting code...\n\n\nBR,\nHarvey","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 1D925BDB13\n\tfor ;\n\tWed, 2 Aug 2023 07:06:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7620A627E7;\n\tWed, 2 Aug 2023 09:06:37 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C45D627E6\n\tfor ;\n\tWed, 2 Aug 2023 09:06:35 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id\n\t38308e7fff4ca-2b95d5ee18dso99512761fa.1\n\tfor ;\n\tWed, 02 Aug 2023 00:06:35 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690959997;\n\tbh=1o9/vzCeT5MLxKsDhvmD7CTstq3X3cyRoOeTiDI3PDE=;\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=GjPc5UjZQA8TlZ+12somwwgwLVtSV/18ueRdH9ocqB5qx6IO5xQdugEUjC4zna32b\n\t3wo/BQ13zUweNpXt5hAt6DoVvfA8rwj/9maZ1uVtv7NRRjnWpFNxhg0Q0/9dgIilxs\n\tbiKr9YawEtO7KCVGdPX6qWf19nd28sVldrzWZxRA0MmeH7lhcOaFCqXrssBz4T/RG2\n\tRIR3JPTLeQzp9mrSfNJ3Eo7YJC4hLtIhdIRBU61epgjcRFic2cewZZTCObMDWLabO3\n\tEKhgFI+3EGEaZdmYV2UEKSG1fTHLNMQFjxumkHDg/0hZeyFVXjChc140y3GaNVkmrO\n\t2j7Y/kl74HjUg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1690959994; x=1691564794;\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=X3yJ6pKB7upvXD0K7c9g4QtYcSbpCF2Xfc5M2df+GPE=;\n\tb=GDOJWXAGjK7nQYF/WxCRdOIqKEjEUh4Og/cHdWn9gFDxE8i1mRgIZfgbFaqave/A+O\n\tGhH850dp/olDcC9aBrPeMpgH4k+K5zfM/Ii5cjfKsUJcNL8OLHXzM2x++UhtpzNb5/dw\n\tWDy3eNjkDuQAi9J6LANwQooZ3J+5AfBoKloo0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"GDOJWXAG\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690959994; x=1691564794;\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=X3yJ6pKB7upvXD0K7c9g4QtYcSbpCF2Xfc5M2df+GPE=;\n\tb=ZLUcIY/r43T6lHNhEU247mN4cfI36xFBLv2qQk8G9nZKFlnUdohuus+pCqfMPibedp\n\tOfwNg8whGG0JyDlw8Bc6S1Cu6sFciZANqXzUSnfLmp6pU98om5PzMd258O1ME91EPsoK\n\t7HOcYRVad0bVKgDAo/28KXGS7e+v+onKkyvg5WxmnVC0yzUaVh4uyG+5QTAPYxMx2kty\n\t45c4IywafCgdy+nsBCl62th6ZnRow0FBZ52iIcvcpiK5NqCasbQNZwphhaFwA090W3bk\n\tAMvXGqt3qZiHtuegCH4HplbXHkZ0B6aOgWRvQtsaoqj2/yessYZoA/DnBGfi9rA3zzVB\n\tzf+w==","X-Gm-Message-State":"ABy/qLbRndso3IAqWq072E7hHw/1gJ6wRHl3xbfUh3T4y7zKS5n78nB7\n\tuP2Wyjnr45Ndc6kJnR+8zDmTrJU5yo2JHoPZu2n6D2o3pGOUTAfA","X-Google-Smtp-Source":"APBJJlHARZE91dVxIOAqHPsA23YjaSe3xfuWGZ2CgB3uzYtjial7ZnErk+OdEEd8FBfi5uQTrrEzPRk26s9oMHcd998=","X-Received":"by 2002:a2e:3307:0:b0:2b9:d71c:b49d with SMTP id\n\td7-20020a2e3307000000b002b9d71cb49dmr4201861ljc.14.1690959994250;\n\tWed, 02 Aug 2023 00:06:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-2-chenghaoyang@google.com>\n\t<4311e3be-d23f-d16a-ea11-c3991ec22086@ideasonboard.com>","In-Reply-To":"<4311e3be-d23f-d16a-ea11-c3991ec22086@ideasonboard.com>","Date":"Wed, 2 Aug 2023 15:06:23 +0800","Message-ID":"","To":"Umang Jain , \n\tJacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000c97c800601eb4d2c\"","Subject":"Re: [libcamera-devel] [PATCH v6 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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]