[{"id":29075,"web_url":"https://patchwork.libcamera.org/comment/29075/","msgid":"<20240327092835.GA8623@pendragon.ideasonboard.com>","date":"2024-03-27T09:28:35","subject":"Re: [PATCH v6 02/18] libcamera: internal: Move dma_heaps.[h, cpp] to\n\tcommon directories","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan and Andrey,\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:35:49PM +0100, Milan Zamazal wrote:\n> From: Andrey Konovalov \n> \n> DmaHeap class is useful outside the RPi pipeline handler too.\n> \n> Move dma_heaps.h and dma_heaps.cpp to common directories. Update\n> the build files and RPi vc4 pipeline handler accordingly.\n> \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Naushir Patuck \n> Reviewed-by: Pavel Machek \n> Signed-off-by: Andrey Konovalov \n> Signed-off-by: Hans de Goede \n> Reviewed-by: Milan Zamazal \n> ---\n> .../libcamera/internal}/dma_heaps.h | 4 -\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/dma_heaps.cpp | 127 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp | 90 -------------\n> src/libcamera/pipeline/rpi/vc4/meson.build | 1 -\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 5 +-\n> 7 files changed, 131 insertions(+), 98 deletions(-)\n> rename {src/libcamera/pipeline/rpi/vc4 => include/libcamera/internal}/dma_heaps.h (92%)\n> create mode 100644 src/libcamera/dma_heaps.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/dma_heaps.h\n> similarity index 92%\n> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> rename to include/libcamera/internal/dma_heaps.h\n> index 0a4a8d86..cff8f140 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h\n> +++ b/include/libcamera/internal/dma_heaps.h\n> @@ -13,8 +13,6 @@\n> \n> namespace libcamera {\n> \n> -namespace RPi {\n> -\n> class DmaHeap\n> {\n> public:\n> @@ -27,6 +25,4 @@ private:\n> \tUniqueFD dmaHeapHandle_;\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 7f1f3440..33eb0fb3 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -25,6 +25,7 @@ libcamera_internal_headers = files([\n> 'device_enumerator.h',\n> 'device_enumerator_sysfs.h',\n> 'device_enumerator_udev.h',\n> + 'dma_heaps.h',\n> 'formats.h',\n> 'framebuffer.h',\n> 'ipa_manager.h',\n> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> new file mode 100644\n> index 00000000..38ef175a\n> --- /dev/null\n> +++ b/src/libcamera/dma_heaps.cpp\n> @@ -0,0 +1,127 @@\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 \"libcamera/internal/dma_heaps.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file dma_heaps.cpp\n> + * \\brief CMA dma-heap allocator\n> + */\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_DEFINE_CATEGORY(DmaHeap)\n> +\n> +/**\n> + * \\class DmaHeap\n> + * \\brief Helper class for CMA dma-heap allocations\n> + */\n> +\n> +/**\n> + * \\brief Construct a DmaHeap that owns a CMA dma-heap file descriptor\n> + *\n> + * Goes through the internal list of possible names of the CMA dma-heap devices\n> + * until a CMA dma-heap device is successfully opened. If it fails to open any\n> + * dma-heap device, an invalid DmaHeap object is constructed. A valid DmaHeap\n> + * object owns a wrapped dma-heap file descriptor.\n\nMost of that is internal details. You should document the\nexternally-visible behaviour.\n\n> + *\n> + * Please check the new DmaHeap object with \\ref DmaHeap::isValid before using it.\n\nNo need for \\ref, you can write\n\n * Please check the new DmaHeap object with isValid() before using it.\n\nand doxygen will figure it out. And while being polite is nice, \"please\"\ndoesn't fit in the documentation style :-)\n\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(DmaHeap, Debug)\n> +\t\t\t\t<< \"Failed to open \" << name << \": \"\n> +\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(DmaHeap, Error) << \"Could not open any dmaHeap device\";\n> +}\n> +\n> +/**\n> + * \\brief Destroy the DmaHeap instance\n> + *\n> + * Destroying a DmaHeap instance which owns a wrapped dma-heap file descriptor\n> + * closes the descriptor automatically.\n\nInternal details too.\n\n> + */\n> +DmaHeap::~DmaHeap() = default;\n> +\n> +/**\n> + * \\fn DmaHeap::isValid()\n> + * \\brief Check if the DmaHeap instance is valid\n> + * \\return True if the DmaHeap is valid, false otherwise\n> + */\n> +\n> +/**\n> + * \\brief Allocate a dma-buf from the DmaHeap\n> + * \\param [in] name The name to set for the allocated buffer\n> + * \\param [in] size The size of the buffer to allocate\n> + * \\return The \\ref UniqueFD of the allocated buffer\n\n\\return goes last, after the mail text.\n\n> + *\n> + * Allocates a dma-buf with read/write access.\n> + * If the allocation fails returns invalid UniqueFD.\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(DmaHeap, Error) << \"dmaHeap allocation failure for \" << 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(DmaHeap, Error) << \"dmaHeap naming failure for \" << name;\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn allocFd;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 2e7b0c77..dd8107fa 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -15,6 +15,7 @@ libcamera_sources = files([\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> + 'dma_heaps.cpp',\n> 'fence.cpp',\n> 'formats.cpp',\n> 'framebuffer.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..386e2296 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/meson.build\n> +++ b/src/libcamera/pipeline/rpi/vc4/meson.build\n> @@ -1,7 +1,6 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libcamera_sources += files([\n> - 'dma_heaps.cpp',\n> 'vc4.cpp',\n> ])\n> \n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index ad7dddde..947b1e73 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/dma_heaps.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> +\tDmaHeap dmaHeap_;\n> \tSharedFD lsTable_;\n> \n> \tstruct Config {","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 09CB0C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 09:28:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E75F63339;\n\tWed, 27 Mar 2024 10:28:46 +0100 (CET)","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 38A8D62CA2\n\tfor ;\n\tWed, 27 Mar 2024 10:28:44 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7835C505;\n\tWed, 27 Mar 2024 10:28:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iw0azPs9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711531691;\n\tbh=fAKribuxcNUk3vWapSaT5BHm2Jgb8FADLsMqxI05eLE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iw0azPs9Ls/u+1M8H3CSCY3XjuR1goCl9x9Pa7s8FnAvoYUkLTcRFVuCd9gEvxnRS\n\tyMfidteXQ4U31KJpGB5RkYsdQQZw5jh3z6b0hSsoBQyUuWImno1Kr2juaGlSrcTbEG\n\tAjobm64qC9fZl/oBsayTyfgCN5dgy55x9lSrr7T0=","Date":"Wed, 27 Mar 2024 11:28:35 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tNaushir Patuck ","Subject":"Re: [PATCH v6 02/18] libcamera: internal: Move dma_heaps.[h, cpp] to\n\tcommon directories","Message-ID":"<20240327092835.GA8623@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-3-mzamazal@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]