[{"id":27093,"web_url":"https://patchwork.libcamera.org/comment/27093/","msgid":"<20230513161224.og5gmck7c7ucijhe@lati>","date":"2023-05-13T16:12:24","subject":"Re: [libcamera-devel] [PATCH v4 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 Tue, Mar 28, 2023 at 05:55:32PM +0800, 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\nA few questions on the design before getting to the actual\nimplementation\n\n> ---\n> include/libcamera/dma_heap.h | 20 +++++++++\n> include/libcamera/heap.h | 27 ++++++++++++\n> include/libcamera/heap_allocator.h | 30 +++++++++++++\n\nIs the HeapAllocator meant to be used by applications or is it only\nfor internal library components usage ? If it's only for internal\nusage, should these headers be moved to include/libcamera/internal/ ?\n\n> include/libcamera/meson.build | 3 ++\n> .../dma_heaps.cpp => dma_heap.cpp} | 35 +++++++--------\n> src/libcamera/heap_allocator.cpp | 43 +++++++++++++++++++\n> src/libcamera/meson.build | 2 +\n> .../pipeline/raspberrypi/dma_heaps.h | 32 --------------\n> .../pipeline/raspberrypi/meson.build | 1 -\n> .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++---\n> 10 files changed, 146 insertions(+), 57 deletions(-)\n> create mode 100644 include/libcamera/dma_heap.h\n> create mode 100644 include/libcamera/heap.h\n> create mode 100644 include/libcamera/heap_allocator.h\n> rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp => dma_heap.cpp} (69%)\n> create mode 100644 src/libcamera/heap_allocator.cpp\n> delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h\n>\n> diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h\n> new file mode 100644\n> index 00000000..a663c317\n> --- /dev/null\n> +++ b/include/libcamera/dma_heap.h\n\nIf I'm not mistaken the DmaHeap and UdmaHeap classes are not meant to be used\ndirectly, they always go through the HeapAllocator class. Do we need\nthree separate headers for:\n\nclass Heap\n{\npublic:\n\tvirtual ~Heap() = default;\n\tbool isValid() const { return handle_.isValid(); }\n\tvirtual UniqueFD alloc(const char *name, std::size_t size) = 0;\n\nprotected:\n\tUniqueFD handle_;\n};\n\nclass DmaHeap : public Heap\n{\npublic:\n\tDmaHeap();\n\t~DmaHeap();\n\tUniqueFD alloc(const char *name, std::size_t size) override;\n};\n\nclass UdmaHeap : public Heap\n{\npublic:\n\tUdmaHeap();\n\t~UdmaHeap();\n\tUniqueFD alloc(const char *name, std::size_t size) override;\n};\n\nI wonder if we even need to define an header for the Heap and derived\nsubclasses at all or they could be defined inside the\nheap_allocator.cpp file (assuming no other component is meant to use\nthem).\n\n\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi Ltd\n> + *\n> + * dma_heap.h - Dma Heap implementation.\n> + */\n> +\n> +#include \n> +\n> +namespace libcamera {\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> +} /* namespace libcamera */\n> diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h\n> new file mode 100644\n> index 00000000..c49a2ac3\n> --- /dev/null\n> +++ b/include/libcamera/heap.h\n> @@ -0,0 +1,27 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * heap.h - Heap interface.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\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> +} /* namespace libcamera */\n> diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h\n> new file mode 100644\n> index 00000000..cd7ed1a3\n> --- /dev/null\n> +++ b/include/libcamera/heap_allocator.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * heap_allocator.h - Helper class for heap buffer allocations.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class HeapAllocator\n> +{\n> +public:\n> +\tHeapAllocator();\n> +\t~HeapAllocator();\n> +\tbool isValid() const { return heap_->isValid(); }\n> +\tUniqueFD alloc(const char *name, std::size_t size);\n> +\n> +private:\n> +\tstd::unique_ptr heap_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 408b7acf..f486630a 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -7,10 +7,13 @@ libcamera_public_headers = files([\n> 'camera_manager.h',\n> 'color_space.h',\n> 'controls.h',\n> + 'dma_heap.h',\n> 'fence.h',\n> 'framebuffer.h',\n> 'framebuffer_allocator.h',\n> 'geometry.h',\n> + 'heap.h',\n> + 'heap_allocator.h',\n> 'logging.h',\n> 'pixel_format.h',\n> 'request.h',\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/dma_heap.cpp\n> similarity index 69%\n> rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> rename to src/libcamera/dma_heap.cpp\n> index 6b644406..02415975 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heap.cpp\n> @@ -2,18 +2,19 @@\n> /*\n> * Copyright (C) 2020, Raspberry Pi Ltd\n> *\n> - * dma_heaps.h - Helper class for dma-heap allocations.\n> + * dma_heap.h - Dma Heap implementation.\n> */\n>\n> -#include \"dma_heaps.h\"\n> +#include \n>\n> #include \n> #include \n> -#include \n> -#include \n> #include \n> #include \n>\n> +#include \n> +#include \n> +\n> #include \n>\n> /*\n> @@ -30,9 +31,7 @@ static constexpr std::array heapNames = {\n>\n> namespace libcamera {\n>\n> -LOG_DECLARE_CATEGORY(RPI)\n> -\n> -namespace RPi {\n> +LOG_DEFINE_CATEGORY(DmaHeap)\n>\n> DmaHeap::DmaHeap()\n> {\n> @@ -40,17 +39,17 @@ DmaHeap::DmaHeap()\n> \t\tint ret = ::open(name, O_RDWR, 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\tLOG(DmaHeap, 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\thandle_ = UniqueFD(ret);\n> \t\tbreak;\n> \t}\n>\n> -\tif (!dmaHeapHandle_.isValid())\n> -\t\tLOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> +\tif (!handle_.isValid())\n> +\t\tLOG(DmaHeap, Error) << \"Could not open any dmaHeap device\";\n> }\n>\n> DmaHeap::~DmaHeap() = default;\n> @@ -67,24 +66,22 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> \talloc.len = size;\n> \talloc.fd_flags = O_CLOEXEC | O_RDWR;\n>\n> -\tret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> +\tret = ::ioctl(handle_.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\tLOG(DmaHeap, 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\tLOG(DmaHeap, 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/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> new file mode 100644\n> index 00000000..594b1d6a\n> --- /dev/null\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * heap_allocator.cpp - Helper class for heap buffer allocations.\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n\nAre these headers used ?\n\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(HeapAllocator)\n> +\n> +HeapAllocator::HeapAllocator()\n> +{\n> +\theap_ = std::make_unique();\n> +}\n> +\n> +HeapAllocator::~HeapAllocator() = default;\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 9869bfe7..ee586c0d 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -17,11 +17,13 @@ libcamera_sources = files([\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> + 'dma_heap.cpp',\n> 'fence.cpp',\n> 'formats.cpp',\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/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> deleted file mode 100644\n> index 0a4a8d86..00000000\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> +++ /dev/null\n> @@ -1,32 +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> -#pragma once\n> -\n> -#include \n> -\n> -#include \n> -\n> -namespace libcamera {\n> -\n> -namespace RPi {\n> -\n> -class DmaHeap\n> -{\n> -public:\n> -\tDmaHeap();\n> -\t~DmaHeap();\n> -\tbool isValid() const { return dmaHeapHandle_.isValid(); }\n> -\tUniqueFD alloc(const char *name, std::size_t size);\n> -\n> -private:\n> -\tUniqueFD dmaHeapHandle_;\n> -};\n> -\n> -} /* namespace RPi */\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index 59e8fb18..7322f0e8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -2,7 +2,6 @@\n>\n> libcamera_sources += files([\n> 'delayed_controls.cpp',\n> - 'dma_heaps.cpp',\n> 'raspberrypi.cpp',\n> 'rpi_stream.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 00600441..198dcc9d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -21,6 +21,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -44,7 +45,6 @@\n> #include \"libcamera/internal/yaml_parser.h\"\n>\n> #include \"delayed_controls.h\"\n> -#include \"dma_heaps.h\"\n> #include \"rpi_stream.h\"\n>\n> using namespace std::chrono_literals;\n> @@ -246,7 +246,7 @@ public:\n> \tstd::vector, MediaLink *>> bridgeDevices_;\n>\n> \t/* DMAHEAP allocation helper. */\n> -\tRPi::DmaHeap dmaHeap_;\n> +\tHeapAllocator heapAllocator_;\n> \tSharedFD lsTable_;\n>\n> \tstd::unique_ptr delayedCtrls_;\n> @@ -1304,7 +1304,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> {\n> \tstd::unique_ptr data = std::make_unique(this);\n>\n> -\tif (!data->dmaHeap_.isValid())\n> +\tif (!data->heapAllocator_.isValid())\n> \t\treturn -ENOMEM;\n>\n> \tMediaEntity *unicamImage = unicam->getEntityByName(\"unicam-image\");\n> @@ -1692,9 +1692,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> \t/* Always send the user transform to the IPA. */\n> \tipaConfig.transform = static_cast(config->transform);\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.0\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 578F8BE08B\n\tfor ;\n\tSat, 13 May 2023 16:12:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD459627DE;\n\tSat, 13 May 2023 18:12:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE8F860539\n\tfor ;\n\tSat, 13 May 2023 18:12:28 +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 21CD36C8;\n\tSat, 13 May 2023 18:12:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683994349;\n\tbh=KhR/n3r7LJxW59w/32X6Id2IcpbNljLTmBSMyPd2wL0=;\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=K4MRic9XRSSHOeeGtVzalKea8QF2EXWF28c26+bo8m0CETkv9amNjGV6Uq+UiKuky\n\tKn9Lqmqqp2l8Zf6972k9SWohVei+N+LyeOKqfn7FzC0MDhYS8nNM7yNl43UBziDMZO\n\tyOn4wm6176YF7OnFBgFvjIdGDt/fzsWTa5LaDSgYbGYrQUBR72j2VbY6G8c1/d91mz\n\tM/oK7pst1Pb/DvaH09NxlstrX8gZksEp8pIeCS3V4eCCXMDWqEOzryNJ7lC4vPaGV9\n\t/RFD18fLB7Tfo0CHDq7bLtHzBCFlJ6gmxWPmQqh/WAjojoesW6359wjXgoHfJlC8Ql\n\toU3QUmguQQywg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1683994339;\n\tbh=KhR/n3r7LJxW59w/32X6Id2IcpbNljLTmBSMyPd2wL0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VjiP1SJiAVDKSjq2BmTEa17YD5bz2ti6Yf1lbyoMINYhQ2iBR1ukheVxUR2ytb461\n\tuOzxKRuCe8+jcifdwZutEPsnv1eDWZFP7O8frbEFlqgj/Ew4ZXWY8eZsBKTJ7pumTd\n\tlH56WiDKBxRWuyHjivne2TA90jIwuW7vrn8bYKfY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VjiP1SJi\"; dkim-atps=neutral","Date":"Sat, 13 May 2023 18:12:24 +0200","To":"Harvey Yang ","Message-ID":"<20230513161224.og5gmck7c7ucijhe@lati>","References":"<20230328095534.3584-1-harveyycyang@gmail.com>\n\t<20230328095534.3584-2-harveyycyang@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230328095534.3584-2-harveyycyang@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":27104,"web_url":"https://patchwork.libcamera.org/comment/27104/","msgid":"","date":"2023-05-16T08:12:02","subject":"Re: [libcamera-devel] [PATCH v4 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 Jacopo for the review!\n\n\n\nOn Sun, May 14, 2023 at 12:12 AM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Tue, Mar 28, 2023 at 05:55:32PM +0800, Harvey Yang via libcamera-devel\n> 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> A few questions on the design before getting to the actual\n> implementation\n>\n> > ---\n> > include/libcamera/dma_heap.h | 20 +++++++++\n> > include/libcamera/heap.h | 27 ++++++++++++\n> > include/libcamera/heap_allocator.h | 30 +++++++++++++\n>\n> Is the HeapAllocator meant to be used by applications or is it only\n> for internal library components usage ? If it's only for internal\n> usage, should these headers be moved to include/libcamera/internal/ ?\n>\n>\nI see. Thanks for pointing it out. Updated.\nOnly one concern: the copyright. I'll mark |heap_allocator.cpp| belonging\nto\n\"Raspberry Pi Ltd\" then. Please check if it works.\n\n> include/libcamera/meson.build | 3 ++\n> > .../dma_heaps.cpp => dma_heap.cpp} | 35 +++++++--------\n> > src/libcamera/heap_allocator.cpp | 43 +++++++++++++++++++\n> > src/libcamera/meson.build | 2 +\n> > .../pipeline/raspberrypi/dma_heaps.h | 32 --------------\n> > .../pipeline/raspberrypi/meson.build | 1 -\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++---\n> > 10 files changed, 146 insertions(+), 57 deletions(-)\n> > create mode 100644 include/libcamera/dma_heap.h\n> > create mode 100644 include/libcamera/heap.h\n> > create mode 100644 include/libcamera/heap_allocator.h\n> > rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp =>\n> dma_heap.cpp} (69%)\n> > create mode 100644 src/libcamera/heap_allocator.cpp\n> > delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> >\n> > diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h\n> > new file mode 100644\n> > index 00000000..a663c317\n> > --- /dev/null\n> > +++ b/include/libcamera/dma_heap.h\n>\n> If I'm not mistaken the DmaHeap and UdmaHeap classes are not meant to be\n> used\n> directly, they always go through the HeapAllocator class. Do we need\n> three separate headers for:\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> class UdmaHeap : public Heap\n> {\n> public:\n> UdmaHeap();\n> ~UdmaHeap();\n> UniqueFD alloc(const char *name, std::size_t size) override;\n> };\n>\n> I wonder if we even need to define an header for the Heap and derived\n> subclasses at all or they could be defined inside the\n> heap_allocator.cpp file (assuming no other component is meant to use\n> them).\n>\n>\nRight. Moved the classes to heap_allocator.cpp.\n\n\n>\n> > @@ -0,0 +1,20 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi Ltd\n> > + *\n> > + * dma_heap.h - Dma Heap implementation.\n> > + */\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\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> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h\n> > new file mode 100644\n> > index 00000000..c49a2ac3\n> > --- /dev/null\n> > +++ b/include/libcamera/heap.h\n> > @@ -0,0 +1,27 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * heap.h - Heap interface.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\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> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/heap_allocator.h\n> b/include/libcamera/heap_allocator.h\n> > new file mode 100644\n> > index 00000000..cd7ed1a3\n> > --- /dev/null\n> > +++ b/include/libcamera/heap_allocator.h\n> > @@ -0,0 +1,30 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * heap_allocator.h - Helper class for heap buffer allocations.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class HeapAllocator\n> > +{\n> > +public:\n> > + HeapAllocator();\n> > + ~HeapAllocator();\n> > + bool isValid() const { return heap_->isValid(); }\n> > + UniqueFD alloc(const char *name, std::size_t size);\n> > +\n> > +private:\n> > + std::unique_ptr heap_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/meson.build\n> b/include/libcamera/meson.build\n> > index 408b7acf..f486630a 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -7,10 +7,13 @@ libcamera_public_headers = files([\n> > 'camera_manager.h',\n> > 'color_space.h',\n> > 'controls.h',\n> > + 'dma_heap.h',\n> > 'fence.h',\n> > 'framebuffer.h',\n> > 'framebuffer_allocator.h',\n> > 'geometry.h',\n> > + 'heap.h',\n> > + 'heap_allocator.h',\n> > 'logging.h',\n> > 'pixel_format.h',\n> > 'request.h',\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> b/src/libcamera/dma_heap.cpp\n> > similarity index 69%\n> > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > rename to src/libcamera/dma_heap.cpp\n> > index 6b644406..02415975 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > +++ b/src/libcamera/dma_heap.cpp\n> > @@ -2,18 +2,19 @@\n> > /*\n> > * Copyright (C) 2020, Raspberry Pi Ltd\n> > *\n> > - * dma_heaps.h - Helper class for dma-heap allocations.\n> > + * dma_heap.h - Dma Heap implementation.\n> > */\n> >\n> > -#include \"dma_heaps.h\"\n> > +#include \n> >\n> > #include \n> > #include \n> > -#include \n> > -#include \n> > #include \n> > #include \n> >\n> > +#include \n> > +#include \n> > +\n> > #include \n> >\n> > /*\n> > @@ -30,9 +31,7 @@ static constexpr std::array heapNames\n> = {\n> >\n> > namespace libcamera {\n> >\n> > -LOG_DECLARE_CATEGORY(RPI)\n> > -\n> > -namespace RPi {\n> > +LOG_DEFINE_CATEGORY(DmaHeap)\n> >\n> > DmaHeap::DmaHeap()\n> > {\n> > @@ -40,17 +39,17 @@ DmaHeap::DmaHeap()\n> > int ret = ::open(name, O_RDWR, 0);\n> > if (ret < 0) {\n> > ret = errno;\n> > - LOG(RPI, Debug) << \"Failed to open \" << name << \":\n> \"\n> > - << strerror(ret);\n> > + LOG(DmaHeap, Debug) << \"Failed to open \" << name\n> << \": \"\n> > + << strerror(ret);\n> > continue;\n> > }\n> >\n> > - dmaHeapHandle_ = UniqueFD(ret);\n> > + handle_ = UniqueFD(ret);\n> > break;\n> > }\n> >\n> > - if (!dmaHeapHandle_.isValid())\n> > - LOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> > + if (!handle_.isValid())\n> > + LOG(DmaHeap, Error) << \"Could not open any dmaHeap device\";\n> > }\n> >\n> > DmaHeap::~DmaHeap() = default;\n> > @@ -67,24 +66,22 @@ UniqueFD DmaHeap::alloc(const char *name,\n> std::size_t size)\n> > alloc.len = size;\n> > alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> >\n> > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > if (ret < 0) {\n> > - LOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> > - << name;\n> > + LOG(DmaHeap, 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> > + LOG(DmaHeap, 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/heap_allocator.cpp\n> b/src/libcamera/heap_allocator.cpp\n> > new file mode 100644\n> > index 00000000..594b1d6a\n> > --- /dev/null\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * heap_allocator.cpp - Helper class for heap buffer allocations.\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n>\n> Are these headers used ?\n>\n>\nYou're right, while now that the heap implementations are moved into this\nfile, yes they're used :)\n\n\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(HeapAllocator)\n> > +\n> > +HeapAllocator::HeapAllocator()\n> > +{\n> > + heap_ = std::make_unique();\n> > +}\n> > +\n> > +HeapAllocator::~HeapAllocator() = default;\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 9869bfe7..ee586c0d 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -17,11 +17,13 @@ libcamera_sources = files([\n> > 'delayed_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > + 'dma_heap.cpp',\n> > 'fence.cpp',\n> > 'formats.cpp',\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/raspberrypi/dma_heaps.h\n> b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > deleted file mode 100644\n> > index 0a4a8d86..00000000\n> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > +++ /dev/null\n> > @@ -1,32 +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> > -#pragma once\n> > -\n> > -#include \n> > -\n> > -#include \n> > -\n> > -namespace libcamera {\n> > -\n> > -namespace RPi {\n> > -\n> > -class DmaHeap\n> > -{\n> > -public:\n> > - DmaHeap();\n> > - ~DmaHeap();\n> > - bool isValid() const { return dmaHeapHandle_.isValid(); }\n> > - UniqueFD alloc(const char *name, std::size_t size);\n> > -\n> > -private:\n> > - UniqueFD dmaHeapHandle_;\n> > -};\n> > -\n> > -} /* namespace RPi */\n> > -\n> > -} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build\n> b/src/libcamera/pipeline/raspberrypi/meson.build\n> > index 59e8fb18..7322f0e8 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > @@ -2,7 +2,6 @@\n> >\n> > libcamera_sources += files([\n> > 'delayed_controls.cpp',\n> > - 'dma_heaps.cpp',\n> > 'raspberrypi.cpp',\n> > 'rpi_stream.cpp',\n> > ])\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 00600441..198dcc9d 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -21,6 +21,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -44,7 +45,6 @@\n> > #include \"libcamera/internal/yaml_parser.h\"\n> >\n> > #include \"delayed_controls.h\"\n> > -#include \"dma_heaps.h\"\n> > #include \"rpi_stream.h\"\n> >\n> > using namespace std::chrono_literals;\n> > @@ -246,7 +246,7 @@ public:\n> > std::vector, MediaLink\n> *>> bridgeDevices_;\n> >\n> > /* DMAHEAP allocation helper. */\n> > - RPi::DmaHeap dmaHeap_;\n> > + HeapAllocator heapAllocator_;\n> > SharedFD lsTable_;\n> >\n> > std::unique_ptr delayedCtrls_;\n> > @@ -1304,7 +1304,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> > {\n> > std::unique_ptr data =\n> std::make_unique(this);\n> >\n> > - if (!data->dmaHeap_.isValid())\n> > + if (!data->heapAllocator_.isValid())\n> > return -ENOMEM;\n> >\n> > MediaEntity *unicamImage = unicam->getEntityByName(\"unicam-image\");\n> > @@ -1692,9 +1692,9 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config, ipa::RPi::IPA\n> > /* Always send the user transform to the IPA. */\n> > ipaConfig.transform = static_cast(config->transform);\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.0\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 A1BD5BDE6B\n\tfor ;\n\tTue, 16 May 2023 08:12:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03177627DC;\n\tTue, 16 May 2023 10:12:15 +0200 (CEST)","from mail-vk1-xa2e.google.com (mail-vk1-xa2e.google.com\n\t[IPv6:2607:f8b0:4864:20::a2e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB9E7627D1\n\tfor ;\n\tTue, 16 May 2023 10:12:14 +0200 (CEST)","by mail-vk1-xa2e.google.com with SMTP id\n\t71dfb90a1353d-44fa6454576so4885844e0c.0\n\tfor ;\n\tTue, 16 May 2023 01:12:14 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684224736;\n\tbh=kL3PIG/7/mpTEBLMwofkr5Adi0qXdq666Tr3jleEDXs=;\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=lvdnKWGcIbXXWEJ1elPGI98wSuttG5Z0AtU5ip0JOjckewzVaKBbq4QDid/6bfnTW\n\tw7eMRH7qpC8PZXTNBz4XEnACQuZWuNn8r5Gobjl4KeQug0SoTaXCF8nKSSb89HaiNI\n\tIsxS2epdVuNFD48cZeKkMfdRVEE5sied4bNB9TspvrLdqwBW9ygLxlb230Li25ly6q\n\tVfvWs0EoLdmtyHGSQdNReXdoi5O/57IZeedBsKv+yROp7IkdWafKg7FucWPy3tURhP\n\tTbWp9zxyB91CfLDEqmoxW8WFzD4myZf+HXhGXa2wCd8Km/Bu6v2S7d4rN2O5kFK5U6\n\tSB0Xjyhiqx2JQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1684224733; x=1686816733;\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=udNJlquItcv82kmGLikh/k8FckIyhf1fVSEFi8/hiS4=;\n\tb=a/QIod0GQl/hXcfWZUVXJ9E7Usr5QbZD8cbJ25ONVb8Ks+7B8CVIFcEbKtriyoVGcI\n\tgYqIBb1llyt763Yc6/jdJ3VZ93FCLhFFj2iLRlVAmiZlE7I5Gt2+3nNSkzeRjg/zcK+3\n\t2yGLzBWqfTx5q9iHDzsQAvTRB4cTa0hFUWTIQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"a/QIod0G\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1684224733; x=1686816733;\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=udNJlquItcv82kmGLikh/k8FckIyhf1fVSEFi8/hiS4=;\n\tb=ZcsjC/pbieJFKumy2N9ENYzYGuRC4fX3H/tSVrMcTvfCOM7BOQrVrzu9hu9KVNkMUd\n\ttdMmkMEkP0bg2MHN7uOsaPs0XxjH+zuByglPBuUKnqOYfx1gstfWGDkDBJwIZeuynhwK\n\tD8z8OBTE2/I9SWZaIBqZUvO4z1Sc/vYrch7NcyrlW1n6uBnIpGIVhJ3e2fejotet3hw6\n\tYFXv0IHaQNI28PkD8r6XM9wysdTpXq9szqnqKRr6UXNn6bRAqqg4D9yS70k6IKETh6Hc\n\tsHiDd2C2ODqmuQSI+5IReB51yvykwHXaq3UySaJyKqEft5nowgJC7NvuhhU9Uzuwq5Hq\n\trIfg==","X-Gm-Message-State":"AC+VfDwjnVVxDCFG+fJi6mwyXGjo3j5M+uSHomubYUL4tZ8pEBIlVDLO\n\tDzN6IshAw2frMdMox8/tbhqfa3dfghvYF0N1YZE+1Q==","X-Google-Smtp-Source":"ACHHUZ7dxZPt4or7Q4Zv0hlxHoKIyZTKKFd9ffKLVFz66quzFgbqdpnjXNkeLkAJg0soC/JQ3clkY73yldNqRZPZRp0=","X-Received":"by 2002:a1f:e601:0:b0:3ea:7af1:9ea4 with SMTP id\n\td1-20020a1fe601000000b003ea7af19ea4mr11545467vkh.12.1684224733510;\n\tTue, 16 May 2023 01:12:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20230328095534.3584-1-harveyycyang@gmail.com>\n\t<20230328095534.3584-2-harveyycyang@gmail.com>\n\t<20230513161224.og5gmck7c7ucijhe@lati>","In-Reply-To":"<20230513161224.og5gmck7c7ucijhe@lati>","Date":"Tue, 16 May 2023 16:12:02 +0800","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000f6a67f05fbcb20b3\"","Subject":"Re: [libcamera-devel] [PATCH v4 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\" "}}]