[{"id":29216,"web_url":"https://patchwork.libcamera.org/comment/29216/","msgid":"<20240414202427.GE12561@pendragon.ideasonboard.com>","date":"2024-04-14T20:24:27","subject":"Re: [PATCH v7 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","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:40AM +0200, Milan Zamazal wrote:\n> From: Andrey Konovalov \n> \n> Add an argument to the constructor to specify dma heaps type(s)\n> to use. Can be DmaHeapFlag::Cma and/or DmaHeapFlag::System.\n> By default DmaHeapFlag::Cma is used. If both DmaHeapFlag::Cma and\n> DmaHeapFlag::System are set, CMA heap is tried first.\n> \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Milan Zamazal \n> Reviewed-by: Pavel Machek \n> Signed-off-by: Andrey Konovalov \n> Signed-off-by: Hans de Goede \n> ---\n> include/libcamera/internal/dma_heaps.h | 12 +++-\n> src/libcamera/dma_heaps.cpp | 76 +++++++++++++++++++++-----\n> 2 files changed, 73 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n> index cff8f140..80bf29e7 100644\n> --- a/include/libcamera/internal/dma_heaps.h\n> +++ b/include/libcamera/internal/dma_heaps.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -16,7 +17,14 @@ namespace libcamera {\n> class DmaHeap\n> {\n> public:\n> -\tDmaHeap();\n> +\tenum class DmaHeapFlag {\n> +\t\tCma = 1 << 0,\n> +\t\tSystem = 1 << 1,\n> +\t};\n> +\n> +\tusing DmaHeapFlags = Flags;\n> +\n> +\tDmaHeap(DmaHeapFlags flags = DmaHeapFlag::Cma);\n> \t~DmaHeap();\n> \tbool isValid() const { return dmaHeapHandle_.isValid(); }\n> \tUniqueFD alloc(const char *name, std::size_t size);\n> @@ -25,4 +33,6 @@ private:\n> \tUniqueFD dmaHeapHandle_;\n> };\n> \n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> index 70da6f3a..2503e4c5 100644\n> --- a/src/libcamera/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heaps.cpp\n> @@ -19,9 +19,11 @@\n> \n> /**\n> * \\file dma_heaps.cpp\n> - * \\brief CMA dma-heap allocator\n> + * \\brief dma-heap allocator\n> */\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> @@ -29,40 +31,86 @@\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> + * \\brief Specification of a heap info entry\n> + */\n\nAs this structure is internal to the file, it shouldn't end up in\ngenerated documentation. That was the point of my comment in v6, which I\nhaven't expressed clearly enough.\n\nYou can just replace the /** with /*, or also drop the \\brief. Or\npossibly drop the comments, I'm not sure they add much. Up to you.\n\n> +struct DmaHeapInfo {\n> +\t/**\n> +\t * \\brief Flag to match for considering the entry\n> +\t */\n> +\tDmaHeap::DmaHeapFlag type;\n> +\t/**\n> +\t * \\brief Path to a heap device\n> +\t */\n> +\tconst char *deviceNodeName;\n> };\n> \n> -namespace libcamera {\n> +static constexpr std::array heapInfos = { {\n> +\t{ DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\" },\n> +\t{ DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\" },\n> +\t{ DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\" },\n> +} };\n> \n> LOG_DEFINE_CATEGORY(DmaHeap)\n> \n> /**\n> * \\class DmaHeap\n> - * \\brief Helper class for CMA dma-heap allocations\n> + * \\brief Helper class for dma-heap allocations\n> + *\n> + * DMA heaps are kernel devices that provide an API to allocate memory from\n> + * different pools called \"heaps\", wrap each allocated piece of memory in a\n> + * dmabuf object, and return the dmabuf file descriptor to userspace. Multiple\n> + * heaps can be provided by the system, with different properties for the\n> + * underlying memory.\n> + *\n> + * This class wraps a DMA heap selected at construction time, and exposes\n> + * functions to manage memory allocation.\n> */\n> \n> /**\n> - * \\brief Construct a DmaHeap that owns a CMA dma-heap file descriptor\n> + * \\enum DmaHeap::DmaHeapFlag\n> + * \\brief Type of the dma-heap\n> + * \\var DmaHeap::Cma\n> + * \\brief Allocate from a CMA dma-heap, providing physically-contiguous memory\n> + * \\var DmaHeap::System\n> + * \\brief Allocate from the system dma-heap, using the page allocator\n> + */\n> +\n> +/**\n> + * \\typedef DmaHeap::DmaHeapFlags\n> + * \\brief A bitwise combination of DmaHeap::DmaHeapFlag values\n> + */\n> +\n> +/**\n> + * \\brief Construct a DmaHeap of a given type\n> + * \\param [in] type The type(s) of the dma-heap(s) to allocate from\n\nNo space between \"param\" and \"[in]\".\n\nReviewed-by: Laurent Pinchart \n\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> + * The DMA heap type is selected with the \\a type parameter, which defaults to\n> + * the CMA heap. If no heap of the given type can be accessed, the constructed\n> + * DmaHeap instance is invalid as indicated by the isValid() function.\n> *\n> - * Check the new DmaHeap object with DmaHeap::isValid before using it.\n> + * Multiple types can be selected by combining type flags, in which case the\n> + * constructed DmaHeap will match one of the types. If the system provides\n> + * multiple heaps that match the requested types, which heap is used is\n> + * undefined.\n> */\n> -DmaHeap::DmaHeap()\n> +DmaHeap::DmaHeap(DmaHeapFlags type)\n> {\n> -\tfor (const char *name : heapNames) {\n> -\t\tint ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> +\tfor (const auto &info : heapInfos) {\n> +\t\tif (!(type & info.type))\n> +\t\t\tcontinue;\n> +\n> +\t\tint ret = ::open(info.deviceNodeName, 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<< \"Failed to open \" << info.deviceNodeName << \": \"\n> \t\t\t\t<< strerror(ret);\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tLOG(DmaHeap, Debug) << \"Using \" << info.deviceNodeName;\n> \t\tdmaHeapHandle_ = UniqueFD(ret);\n> \t\tbreak;\n> \t}","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 67D9EBE08B\n\tfor ;\n\tSun, 14 Apr 2024 20:24:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E48B63352;\n\tSun, 14 Apr 2024 22:24:39 +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 5C0856333B\n\tfor ;\n\tSun, 14 Apr 2024 22:24:37 +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 C4AA27E4;\n\tSun, 14 Apr 2024 22:23:51 +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=\"qO0Hn64H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713126231;\n\tbh=Nt58h4jrjqqwDpADRzmY+Wph9T7mY794UI3Y7FZLrnc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qO0Hn64HwPvdJPr3mdYlZCjffQvGm1bgVysmRf1PF7p2WPDYV+XH3lPaek5+KPpxh\n\tON7jVOn5KoVZFZ026QDImAc3CuGtwiVTNKIUvxtOQuyT46NDImRXui+SbJzWp8FxF8\n\tVmIlNwinSpQhqcE1wusKD8qH1s4EG6onYnSBTpwI=","Date":"Sun, 14 Apr 2024 23:24:27 +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 ","Subject":"Re: [PATCH v7 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","Message-ID":"<20240414202427.GE12561@pendragon.ideasonboard.com>","References":"<20240404084657.353464-1-mzamazal@redhat.com>\n\t<20240404084657.353464-4-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240404084657.353464-4-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\" "}}]