[{"id":29077,"web_url":"https://patchwork.libcamera.org/comment/29077/","msgid":"<20240327103429.GB8623@pendragon.ideasonboard.com>","date":"2024-03-27T10:34:29","subject":"Re: [PATCH v6 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\nIn the subject line, s/extend/Extend/\n\nOn Tue, Mar 19, 2024 at 01:35:50PM +0100, 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 | 67 ++++++++++++++++++++------\n> 2 files changed, 63 insertions(+), 16 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 38ef175a..d0e33ce6 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\nI like that we're making this more generic :-)\n\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,42 +31,77 @@\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> + * \\struct DmaHeapInfo\n> + * \\brief Tells what type of dma-heap the dma-heap represented by the device node name is\n> + * \\var DmaHeapInfo::flag\n> + * \\brief The type of the dma-heap\n> + * \\var DmaHeapInfo::name\n> + * \\brief The dma-heap's device node name\n> + */\n\nThis is internal, no need to generate documentation. \n\n> +struct DmaHeapInfo {\n> +\tDmaHeap::DmaHeapFlag flag;\n> +\tconst char *name;\n> };\n> \n> -namespace libcamera {\n> +static constexpr std::array heapInfos = {\n> +\t{ /* CMA heap names first */\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\nstatic constexpr std::array heapInfos = { {\n\t/* CMA heap names first */\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> \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 * \\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> /**\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\n\n * \\brief Allocate from a CMA dma-heap, providing physically-contiguous memory\n\n> + * \\var DmaHeap::System\n> + * \\brief Allocate from the system dma-heap\n\n * \\brief Allocate from the system dma-heap, using the page allocator\n\n> + */\n> +\n> +/**\n> + * \\typedef DmaHeap::DmaHeapFlags\n> + * \\brief A bitwise combination of DmaHeap::DmaHeapFlag values\n> + */\n> +\n> +/**\n> + * \\brief Construct a DmaHeap that owns a CMA or system dma-heap file descriptor\n\nJust \"Construct a DmaHeap of a given type\" is enough.\n\n> + * \\param [in] flags The type(s) of the dma-heap(s) to allocate from\n\nAs this indicates the desired DMA heap type, let's call the argument\ntype, not flags.\n\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\nGiven that this patch rewrites the documentation of 02/18, I suppose I\ncan close my eyes and merge 02/18 without the review comments related to\ndocumentation being addressed :-)\n\n> + * By default \\a flags are set to DmaHeap::DmaHeapFlag::Cma. The constructor goes\n> + * through the internal list of possible names of the CMA and system dma-heap devices\n> + * until the dma-heap device of the requested type is successfully opened. If more\n> + * than one dma-heap type is specified in flags the CMA heap is tried first. If it\n> + * fails to open any dma-heap device an invalid DmaHeap object is constructed.\n> + * A valid DmaHeap object owns a wrapped dma-heap file descriptor.\n> *\n> * Please check the new DmaHeap object with \\ref DmaHeap::isValid before using it.\n\nThat's even more internal implementation details. Let me try to help.\n\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 * 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> */\n> -DmaHeap::DmaHeap()\n> +DmaHeap::DmaHeap(DmaHeapFlags flags)\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 (!(flags & info.flag))\n> +\t\t\tcontinue;\n> +\n> +\t\tint ret = ::open(info.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<< \"Failed to open \" << info.name << \": \"\n> \t\t\t\t<< strerror(ret);\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tLOG(DmaHeap, Debug) << \"Using \" << info.name;\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 6B157C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 10:34:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6286263339;\n\tWed, 27 Mar 2024 11:34:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5427962CA2\n\tfor ;\n\tWed, 27 Mar 2024 11:34:39 +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 DE677675;\n\tWed, 27 Mar 2024 11:34:05 +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=\"VKSlgXsI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711535646;\n\tbh=H0c6iLTdz8JLmYUI/tLp1QE0fAGxYH2BUYMhIV4g/1E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VKSlgXsI05W4UDvAJPp4jKY8W56ELHuv1k+eLmyCDdt6qaqkTGI09YCNERVZaVt3/\n\tlS04ISRkE0A9u3jERiyMsnueirkyyTicVwZF4yqL8BQ+Wi0X3w444qLhQ+pIApo2mP\n\t9r4zYTQW3l/TQZTei6n86Ty645DDdCvoV1DCS7Gc=","Date":"Wed, 27 Mar 2024 12:34:29 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","Message-ID":"<20240327103429.GB8623@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-4-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-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\" "}},{"id":29095,"web_url":"https://patchwork.libcamera.org/comment/29095/","msgid":"<87jzlny8d5.fsf@redhat.com>","date":"2024-03-27T15:58:30","subject":"Re: [PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for the reviews.\n\nLaurent Pinchart writes:\n\n>> + * By default \\a flags are set to DmaHeap::DmaHeapFlag::Cma. The constructor goes\n>> + * through the internal list of possible names of the CMA and system dma-heap devices\n>> + * until the dma-heap device of the requested type is successfully opened. If more\n>> + * than one dma-heap type is specified in flags the CMA heap is tried first. If it\n>> + * fails to open any dma-heap device an invalid DmaHeap object is constructed.\n>> + * A valid DmaHeap object owns a wrapped dma-heap file descriptor.\n>> *\n>> * Please check the new DmaHeap object with \\ref DmaHeap::isValid before using it.\n>\n> That's even more internal implementation details. Let me try to help.\n>\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> * 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\nI'm not sure we want the generalization described in the last paragraph. The\ncase \"use whatever heap is available, but use CMA one if available\" would have\nto be split in two DmaHeap construction attempts (one asking for CMA, the other\none asking for anything if the first one doesn't provide a valid instance).\n\nI suppose it may make sense to ask for a CMA heap and use a system heap if no\nCMA heap is available. While if someone really wants \"use whatever heap is\navailable, I don't care at all which one\", applying \"use whatever heap is\navailable, but use CMA one if available\" is still compatible.\n\nI admit that using two separate calls would be slightly cleaner but it'd be also\nless convenient (and I think the current code should be changed if the docstring\nis changed as suggested, since I suppose the CMA preference is on purpose).\n\nOpinions?\n\nRegards,\nMilan","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 90328BD160\n\tfor ;\n\tWed, 27 Mar 2024 15:58:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 702B363331;\n\tWed, 27 Mar 2024 16:58:37 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B881261C35\n\tfor ;\n\tWed, 27 Mar 2024 16:58:35 +0100 (CET)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-116-Jd95cU2YP8W9KHoqNwZ-7Q-1; Wed, 27 Mar 2024 11:58:33 -0400","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-33eca7270ccso3062f8f.1\n\tfor ;\n\tWed, 27 Mar 2024 08:58:33 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\taz7-20020adfe187000000b00341ce80ea66sm8093496wrb.82.2024.03.27.08.58.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 27 Mar 2024 08:58:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Lq/4Ri4A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1711555114;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=nw6i2s6UHXT5WakZARrMIcxoQcG9CF/rdrrKcgvw+Yw=;\n\tb=Lq/4Ri4AZLXN7ItSMqTnUUZe/XUJpwCUEhPVvQpw76bKo+alBGu4NdYDzx8Zu+86aInR5I\n\tcSaWgh/IRx2isTIhfu5LmJNevjnbI636Riiye8VbIMiHGjl5Qu/EwTJJiNyccYof29Fkyw\n\tn2OcCGzHYtI+lH5j4vweZr1o1As04ME=","X-MC-Unique":"Jd95cU2YP8W9KHoqNwZ-7Q-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711555112; x=1712159912;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=nw6i2s6UHXT5WakZARrMIcxoQcG9CF/rdrrKcgvw+Yw=;\n\tb=VWwBO6v57IBLzeDgO8hvkiAL9oPUel1FEnWXC+/7tuUSEiMz6GVdFtSJ2I9OcrVj9a\n\twu210uZYGEH6I9BSmAO1fyZfQ0wkUXZC/WH1tSYuR0TjwR72G/E/UZfcE1Ss1aInYjSe\n\tAP6ZsMTvadyXr2L/QH+h3MOgeiN6iM3QW4SQMeQJZ7pMXOTIvLKDLioRB/WFHHnIooHw\n\tRXi+gDU3ZAupys48OhrhydTkZv+SvZkdh2N0uwrjLCi7E8ENis2kAZgpVaMgx6lTsmYr\n\tZl4AIc4Q50WSS0US46vRKBzAJLQg0J0ov/TfVXsFQn4lkus+UKHXB4xqhq5LRXEwCZki\n\tAcSQ==","X-Gm-Message-State":"AOJu0YwJeS8IE8BdugGRrHfimy+p8p4IXRy9myeGKYDG0pufu3dFNhMS\n\tB4Zp+yvyFNHNPmNOSTeWETujkd50+Dc2R0ImB7YrV7u7AeOlOaBb38d+9MqqRO4y3ODHRM5ljYB\n\tMpcL923+i+SFCLvRP0326S2/i+iCXY27+crH1JYNQSPdG7iyb8rQMbSXKhqF+YmWCYpYVu+Q=","X-Received":["by 2002:a5d:680a:0:b0:33e:7991:6105 with SMTP id\n\tw10-20020a5d680a000000b0033e79916105mr227907wru.16.1711555112240; \n\tWed, 27 Mar 2024 08:58:32 -0700 (PDT)","by 2002:a5d:680a:0:b0:33e:7991:6105 with SMTP id\n\tw10-20020a5d680a000000b0033e79916105mr227888wru.16.1711555111802; \n\tWed, 27 Mar 2024 08:58:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGOo+DBQ5/7t2/+s03wUHSzlA5QFVDCrVDIPgl6Y+j9Ycn/AN49Yed4w16mWPlj9wBp9jl4Yg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Andrey Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham ","Subject":"Re: [PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","In-Reply-To":"<20240327103429.GB8623@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Mar 2024 12:34:29 +0200\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-4-mzamazal@redhat.com>\n\t<20240327103429.GB8623@pendragon.ideasonboard.com>","Date":"Wed, 27 Mar 2024 16:58:30 +0100","Message-ID":"<87jzlny8d5.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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\" "}},{"id":29107,"web_url":"https://patchwork.libcamera.org/comment/29107/","msgid":"<079a7b42-cbf2-446f-859b-bff647779213@gmail.com>","date":"2024-03-27T18:54:30","subject":"Re: [PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","submitter":{"id":179,"url":"https://patchwork.libcamera.org/api/people/179/","name":"Andrei Konovalov","email":"andrey.konovalov.ynk@gmail.com"},"content":"Hi Laurent and Milan,\n\nand thank you for the review and comments.\n\nOn 27.03.2024 18:58, Milan Zamazal wrote:\n> Hi Laurent,\n> \n> thank you for the reviews.\n> \n> Laurent Pinchart writes:\n> \n>>> + * By default \\a flags are set to DmaHeap::DmaHeapFlag::Cma. The constructor goes\n>>> + * through the internal list of possible names of the CMA and system dma-heap devices\n>>> + * until the dma-heap device of the requested type is successfully opened. If more\n>>> + * than one dma-heap type is specified in flags the CMA heap is tried first. If it\n>>> + * fails to open any dma-heap device an invalid DmaHeap object is constructed.\n>>> + * A valid DmaHeap object owns a wrapped dma-heap file descriptor.\n>>> *\n>>> * Please check the new DmaHeap object with \\ref DmaHeap::isValid before using it.\n>>\n>> That's even more internal implementation details. Let me try to help.\n>>\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>> * 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> I'm not sure we want the generalization described in the last paragraph.\n\nThis generalization can make more sense.\nIf the caller selects multiple types then it must be able to use any of the selected ones.\n\nThe DmaHeap class has no means to determine which of the types selected by the user are\n\"better\" in each particular case (is faster, or doesn't use the precious memory type,\nor something else).\n\nThe current code tries the CMA heap first just because this is a safer choice (system heap\nwouldn't work for all the hardware; e.g. on RPi only the CMA heap works).\n\nSo we should better not to claim in the API documentation that the DmaHeap class has its own\npreferences among the heap types the caller requested.\n\n> The\n> case \"use whatever heap is available, but use CMA one if available\" would have\n> to be split in two DmaHeap construction attempts (one asking for CMA, the other\n> one asking for anything if the first one doesn't provide a valid instance).\n> \n> I suppose it may make sense to ask for a CMA heap and use a system heap if no\n> CMA heap is available.\n\nI would prefer not to do that to avoid providing the caller with the heap type\nthe particular hardware can't work with.\n\n> While if someone really wants \"use whatever heap is\n> available, I don't care at all which one\", applying \"use whatever heap is\n> available, but use CMA one if available\" is still compatible.\n> \n> I admit that using two separate calls would be slightly cleaner but it'd be also\n> less convenient (and I think the current code should be changed if the docstring\n> is changed as suggested, since I suppose the CMA preference is on purpose\n\nNot sure I see the reason for changing the current code.\nThe only reason for trying the CMA heap first is the biggest chance to work with any\nhardware.\n\nThanks,\nAndrei\n\n).\n> \n> Opinions?\n> \n> Regards,\n> Milan\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 762DEBD160\n\tfor ;\n\tWed, 27 Mar 2024 18:54:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86AC663331;\n\tWed, 27 Mar 2024 19:54:34 +0100 (CET)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1F9461C35\n\tfor ;\n\tWed, 27 Mar 2024 19:54:32 +0100 (CET)","by mail-ej1-x62f.google.com with SMTP id\n\ta640c23a62f3a-a46cca2a979so7675966b.3\n\tfor ;\n\tWed, 27 Mar 2024 11:54:32 -0700 (PDT)","from [192.168.118.26] ([87.116.165.87])\n\tby smtp.gmail.com with ESMTPSA id\n\tn1-20020aa7c781000000b0056bb65f4a1esm5598109eds.94.2024.03.27.11.54.31\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 27 Mar 2024 11:54:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"EglYuGJK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1711565672; x=1712170472;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=XP8MshOevpsDptYlr5B1DEEKhoIUinued/yLeKuKozo=;\n\tb=EglYuGJKbh0i+QRdf8l/dCWbiltedNAK5g8/dtxmUYuvkMf6M2V5gpXxeUkz5JhU77\n\tfxYWf39A51h1notOkgim3dRQpx1Ej4UR3J30bt0ZMvKx1b0oAtyzEhkiM/zyiek24cD3\n\tuzR7C505XWVTrhUm2iPsgZXAofiAnxW8D+z04EUswnVmeUp3vDzF23KSW4drWhRSuIyx\n\tgB6kUEjDuFHmYBlS9bDCs5wyV20PMDD0lGY10mNFkO6aZql3duo86Hj0s6ocTxVcmBVD\n\t764bslp+jp8OMaWIIyI8qgHqm9mB4KnYe6QoLkSvS2dg7EQzCpZVR/DRAOsIRcZaU65E\n\tvYiw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711565672; x=1712170472;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=XP8MshOevpsDptYlr5B1DEEKhoIUinued/yLeKuKozo=;\n\tb=Yu72ZyfyUi43TPlfNYoVo0hZy4tL7Aa3gSMxV45bmCUl/uJqk2s1djUY7Cu6EJG68J\n\t/ZDDC3RoxnQ681iPqVohcTMzai7Ggt1/JmSn0P+9MS1LtoRo0MWWAxz+AjkAK6sWlIqm\n\tLLRkXEaC7ZPRUAnismJaea62pk+8PKVJd8hC+z7ip2hgqa7mZJmomT+Q0dRJ4N2Leup9\n\tKAA9qcwQYdkjIxKsB75UYqokYh7JoNHkdBjUWtrr81dYdEflGv8c8mKqS4IMQ/T7JT0e\n\tMRXApSszNNIJ1OD64qPV5s99Fl6upPYDA4GY/fyxKOcVEWlGqZwP/bX6JrGme0f9jdJb\n\tWahg==","X-Gm-Message-State":"AOJu0YwIQHouyX4lSZVoM6/CnE+HnreFiJ35b9Ca/n9eXOnQWbN4UlHm\n\tSQuMlYdg54U9dr10hpo1iIk26Sz2GeyuLTmYpmLiOLMTqt91RY/yhi7mvK82GDQ=","X-Google-Smtp-Source":"AGHT+IHP5PHzO5zUf/cv+jzrrTxnjZjWWehdBMI1maEbzjFMQHQ1PQI4I/ERUhRp5Z1LBj6Q+c+UPA==","X-Received":"by 2002:a50:c349:0:b0:56b:f54a:8485 with SMTP id\n\tq9-20020a50c349000000b0056bf54a8485mr744296edb.0.1711565672129; \n\tWed, 27 Mar 2024 11:54:32 -0700 (PDT)","Message-ID":"<079a7b42-cbf2-446f-859b-bff647779213@gmail.com>","Date":"Wed, 27 Mar 2024 21:54:30 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","To":"Milan Zamazal ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-4-mzamazal@redhat.com>\n\t<20240327103429.GB8623@pendragon.ideasonboard.com>\n\t<87jzlny8d5.fsf@redhat.com>","Content-Language":"en-US","From":"Andrei Konovalov ","In-Reply-To":"<87jzlny8d5.fsf@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}},{"id":29110,"web_url":"https://patchwork.libcamera.org/comment/29110/","msgid":"<20240327191629.GP4721@pendragon.ideasonboard.com>","date":"2024-03-27T19:16:29","subject":"Re: [PATCH v6 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":"Hello,\n\nOn Wed, Mar 27, 2024 at 09:54:30PM +0300, Andrei Konovalov wrote:\n> On 27.03.2024 18:58, Milan Zamazal wrote:\n> > Laurent Pinchart writes:\n> > \n> >>> + * By default \\a flags are set to DmaHeap::DmaHeapFlag::Cma. The constructor goes\n> >>> + * through the internal list of possible names of the CMA and system dma-heap devices\n> >>> + * until the dma-heap device of the requested type is successfully opened. If more\n> >>> + * than one dma-heap type is specified in flags the CMA heap is tried first. If it\n> >>> + * fails to open any dma-heap device an invalid DmaHeap object is constructed.\n> >>> + * A valid DmaHeap object owns a wrapped dma-heap file descriptor.\n> >>> *\n> >>> * Please check the new DmaHeap object with \\ref DmaHeap::isValid before using it.\n> >>\n> >> That's even more internal implementation details. Let me try to help.\n> >>\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> >> * 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> > I'm not sure we want the generalization described in the last paragraph.\n> \n> This generalization can make more sense.\n> If the caller selects multiple types then it must be able to use any of the selected ones.\n> \n> The DmaHeap class has no means to determine which of the types selected by the user are\n> \"better\" in each particular case (is faster, or doesn't use the precious memory type,\n> or something else).\n> \n> The current code tries the CMA heap first just because this is a safer choice (system heap\n> wouldn't work for all the hardware; e.g. on RPi only the CMA heap works).\n> \n> So we should better not to claim in the API documentation that the DmaHeap class has its own\n> preferences among the heap types the caller requested.\n\nThat's my reasoning too, I would prefer not guaranteeing a particular\nbehaviour here if it's needed by one specific caller, when other\nbehaviours could also make sense. If we want to make CMA allocation with\na system heap fallback a prime use case, then let's design a good API\nthat makes this simple to do for a user.\n\n> > The\n> > case \"use whatever heap is available, but use CMA one if available\" would have\n> > to be split in two DmaHeap construction attempts (one asking for CMA, the other\n> > one asking for anything if the first one doesn't provide a valid instance).\n> > \n> > I suppose it may make sense to ask for a CMA heap and use a system heap if no\n> > CMA heap is available.\n> \n> I would prefer not to do that to avoid providing the caller with the heap type\n> the particular hardware can't work with.\n> \n> > While if someone really wants \"use whatever heap is\n> > available, I don't care at all which one\", applying \"use whatever heap is\n> > available, but use CMA one if available\" is still compatible.\n> > \n> > I admit that using two separate calls would be slightly cleaner but it'd be also\n> > less convenient (and I think the current code should be changed if the docstring\n> > is changed as suggested, since I suppose the CMA preference is on purpose\n> \n> Not sure I see the reason for changing the current code.\n> The only reason for trying the CMA heap first is the biggest chance to work with any\n> hardware.\n> \n> > Opinions?","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 592E7BD160\n\tfor ;\n\tWed, 27 Mar 2024 19:16:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C98863360;\n\tWed, 27 Mar 2024 20:16:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 298056308D\n\tfor ;\n\tWed, 27 Mar 2024 20:16:38 +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 2D12F899;\n\tWed, 27 Mar 2024 20:16:05 +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=\"aTXPS+0m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711566965;\n\tbh=Pkumuz31t8Zn3whJTS9LRgdb/ypNw4aRgWTBS4yA40E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aTXPS+0mhNLQupMvIjGiESG+OSJHhRuTnVmIM9pQ346g1vM5A8XsJbjFnzVGmhNt0\n\tWpI/1sE8mz6X+0/s8liXT8ZToQjuoZAIUfYvYIBwiZeK1RjYmdChn3rgx9JwMQmjRv\n\tA03dG/lfi7nnB5IKOUDYaLOGiO710qoVaf6/ccSs=","Date":"Wed, 27 Mar 2024 21:16:29 +0200","From":"Laurent Pinchart ","To":"Andrei Konovalov ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to\n\tsupport system heap","Message-ID":"<20240327191629.GP4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-4-mzamazal@redhat.com>\n\t<20240327103429.GB8623@pendragon.ideasonboard.com>\n\t<87jzlny8d5.fsf@redhat.com>\n\t<079a7b42-cbf2-446f-859b-bff647779213@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<079a7b42-cbf2-446f-859b-bff647779213@gmail.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\" "}}]