[{"id":29214,"web_url":"https://patchwork.libcamera.org/comment/29214/","msgid":"<20240413125944.GG23422@pendragon.ideasonboard.com>","date":"2024-04-13T12:59:44","subject":"Re: [PATCH v7 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 Thu, Apr 04, 2024 at 10:46:39AM +0200, 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> .../{pipeline/rpi/vc4 => }/dma_heaps.cpp | 63 ++++++++++++++-----\n> src/libcamera/meson.build | 1 +\n> src/libcamera/pipeline/rpi/vc4/meson.build | 1 -\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 5 +-\n> 6 files changed, 52 insertions(+), 23 deletions(-)\n> rename {src/libcamera/pipeline/rpi/vc4 => include/libcamera/internal}/dma_heaps.h (92%)\n> rename src/libcamera/{pipeline/rpi/vc4 => }/dma_heaps.cpp (51%)\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/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> similarity index 51%\n> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> rename to src/libcamera/dma_heaps.cpp\n> index 317b1fc1..70da6f3a 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heaps.cpp\n> @@ -5,19 +5,25 @@\n> * dma_heaps.h - Helper class for dma-heap allocations.\n\nWhile at it, s/dma_heaps.h/dma_heaps.cpp/\n\n> */\n> \n> -#include \"dma_heaps.h\"\n> +#include \"libcamera/internal/dma_heaps.h\"\n> \n> #include \n> #include \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> + * /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> @@ -30,18 +36,30 @@ static constexpr std::array heapNames = {\n> \n> namespace libcamera {\n> \n> -LOG_DECLARE_CATEGORY(RPI)\n> +LOG_DEFINE_CATEGORY(DmaHeap)\n> \n> -namespace RPi {\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> + * Looks for a CMA dma-heap device to use. If it fails to open any dma-heap\n> + * device, an invalid DmaHeap object is constructed.\n> + *\n> + * Check the new DmaHeap object with DmaHeap::isValid before using it.\n\ns/DmaHeap::isValid/isValid()/\n\ndoxygen should do the right thing.\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(RPI, Debug) << \"Failed to open \" << name << \": \"\n> -\t\t\t\t\t<< strerror(ret);\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> @@ -50,11 +68,30 @@ DmaHeap::DmaHeap()\n> \t}\n> \n> \tif (!dmaHeapHandle_.isValid())\n> -\t\tLOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> +\t\tLOG(DmaHeap, Error) << \"Could not open any dmaHeap device\";\n> }\n> \n> +/**\n> + * \\brief Destroy the DmaHeap instance\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> + *\n> + * Allocates a dma-buf with read/write access.\n> + * If the allocation fails returns invalid UniqueFD.\n\ns/ returns invalid/, return an invalid/\n\nI haven't found an easy way to check this in checkstyle.py yet, so I\nhave to repeat the comment myself for now: There should be no line break\nin the middle of a paragraph. Either you meant this text to be in a\nsingle paragraph, in which case it should be written as\n\n * Allocates a dma-buf with read/write access. If the allocation fails, return\n * an invalid UniqueFD.\n\nor you meant to have two separate paragraphs:\n\n * Allocates a dma-buf with read/write access.\n *\n * If the allocation fails, return an invalid UniqueFD.\n\nWith these small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\n> + *\n> + * \\return The UniqueFD of the allocated buffer\n> + */\n> UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> {\n> \tint ret;\n> @@ -69,22 +106,18 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\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\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(RPI, Error) << \"dmaHeap naming failure for \"\n> -\t\t\t\t<< name;\n> +\t\tLOG(DmaHeap, Error) << \"dmaHeap naming failure for \" << 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/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/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 A21CCC3213\n\tfor ;\n\tSat, 13 Apr 2024 12:59:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C73CD63340;\n\tSat, 13 Apr 2024 14:59:53 +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 DC69761B4D\n\tfor ;\n\tSat, 13 Apr 2024 14:59:51 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F9CA5AA;\n\tSat, 13 Apr 2024 14:59:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UgK/L+yF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713013147;\n\tbh=sfTpnSC5/PXlHcR8edutSMrZS382mXiPKrehJcvPL9E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UgK/L+yF6FtGcpDOSuwpjdFrPwuzjC6y3EEJxUZWcnI4aHIsoeOejGsN/HmeaqtKy\n\tYBavLnQ2VYA1liPLO5Z8DvlGvMRQgMMvx9K0pnhdbLEfBnZcW2NGFtmUqzsqEBG0bh\n\tXmIMorvIlO9yqe9Qdy+aw7kdT656DmR6RGDXK4WY=","Date":"Sat, 13 Apr 2024 15:59:44 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrei Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tNaushir Patuck ","Subject":"Re: [PATCH v7 02/18] libcamera: internal: Move dma_heaps.[h, cpp] to\n\tcommon directories","Message-ID":"<20240413125944.GG23422@pendragon.ideasonboard.com>","References":"<20240404084657.353464-1-mzamazal@redhat.com>\n\t<20240404084657.353464-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240404084657.353464-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\" "}}]