[{"id":27149,"web_url":"https://patchwork.libcamera.org/comment/27149/","msgid":"<4j2ae6q6hsny3hyda4gpposehsfkldhxik6esttteefcyd2wud@vghoxwijzg6q>","date":"2023-05-29T07:58:05","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:\n> From: Cheng-Hao Yang \n>\n> If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n>\n> Signed-off-by: Harvey Yang \n> ---\n> src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> 1 file changed, 100 insertions(+)\n>\n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> index 69f65062..8f99f590 100644\n> --- a/src/libcamera/heap_allocator.cpp\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -11,10 +11,14 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> +#include \n> #include \n>\n> #include \n> #include \n> +#include \n>\n> #include \n>\n> @@ -54,6 +58,14 @@ public:\n> \tUniqueFD alloc(const char *name, std::size_t size) override;\n> };\n>\n> +class UdmaHeap : public Heap\n> +{\n> +public:\n> +\tUdmaHeap();\n> +\t~UdmaHeap();\n> +\tUniqueFD alloc(const char *name, std::size_t size) override;\n> +};\n> +\n> DmaHeap::DmaHeap()\n> {\n> \tfor (const char *name : dmaHeapNames) {\n> @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> \t\t\tcontinue;\n> \t\t}\n>\n> +\t\tLOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> \t\thandle_ = UniqueFD(ret);\n> \t\tbreak;\n> \t}\n> @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> \treturn allocFd;\n> }\n>\n> +UdmaHeap::UdmaHeap()\n> +{\n> +\tint ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n\nShould you add O_CLOEXEC or does it generate issues ? Same question as\nin 1/3 on the third argument\n\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to open allocator: \" << strerror(ret);\n\nYou could return here and save an...\n\n> +\t} else {\n\n... indendation level here\n\n> +\t\tLOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> +\t\thandle_ = UniqueFD(ret);\n> +\t}\n> +}\n> +\n> +UdmaHeap::~UdmaHeap() = default;\n> +\n> +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> +{\n> +\tif (!isValid()) {\n> +\t\tLOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation attempted without allocator\" << name;\n> +\t\treturn {};\n> +\t}\n\nUdmaHeap::alloc() is called by HeapAllocator::alloc() which already\nchecks for validity with a Fatal error. You can drop this change\n> +\n> +\tint ret;\n> +\n> +\tret = memfd_create(name, MFD_ALLOW_SEALING);\n\nYou can declare and assign ret in one line\n\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to allocate memfd storage: \"\n> +\t\t\t<< strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tUniqueFD memfd(ret);\n> +\n> +\tret = ftruncate(memfd.get(), size);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> +\tret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tstruct udmabuf_create create;\n> +\n> +\tcreate.memfd = memfd.get();\n> +\tcreate.flags = UDMABUF_FLAGS_CLOEXEC;\n> +\tcreate.offset = 0;\n> +\tcreate.size = size;\n> +\n> +\tret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n> +\t\t\t<< strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* The underlying memfd is kept as as a reference in the kernel */\n> +\tUniqueFD uDma(ret);\n> +\n> +\tif (create.size != size)\n> +\t\tLOG(HeapAllocator, Warning)\n> +\t\t\t<< \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> +\t\t\t<< size << \" bytes\";\n\nShould we continue or is it an error ? Also I would move this check\nbefore the creation of uDma(ret);\n\n> +\n> +\t/* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> +\tif (create.size < size)\n> +\t\treturn {};\n\nAh so larger size is ok, smaller it's not.\n\nSo I would check for this before the previous one\n\n> +\n> +\tLOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> +\n> +\treturn uDma;\n> +}\n> +\n> HeapAllocator::HeapAllocator()\n> {\n> \theap_ = std::make_unique();\n> +\tif (!isValid())\n> +\t\theap_ = std::make_unique();\n> }\n>\n> HeapAllocator::~HeapAllocator() = default;\n> --\n> 2.40.1.698.g37aff9b760-goog\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 77871C3213\n\tfor ;\n\tMon, 29 May 2023 07:58:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2530626F8;\n\tMon, 29 May 2023 09:58:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10A5E60599\n\tfor ;\n\tMon, 29 May 2023 09:58:09 +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 15C02836;\n\tMon, 29 May 2023 09:57:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685347090;\n\tbh=YT9VEO/0rzuYqsbmBYtaAW9RlLZwtjL+Gqw549JV4Lw=;\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=xAK1gHt2LyXsM+qHnRVLjL77diCcQgfXygrR+IQY77Xx0+NY8xjTO7WgGqtyL0T4U\n\t8i2yi1HLweB95HFM3uO7fC+e1A/nXahFbtbO+l3tJABdFyhBsnwNilXvjTUFj+qJjX\n\tCgnZseaZMXDh+z85U0uc3XbhE8BiPxe2TAF1VkTeVALxtYqQzwovaw3XcrPCdywffb\n\t9XSPKigGnlPdxYckL/B1wKrYP2cOrs9RHduQ4CLtkrkboFFtV3JV2j88ddiE9HP5pa\n\tS3F3A/AGamob1wY2TniTwgiGsUKxKOunhc/RgLm7ce+f8v8pcWNRtyW1UFuJAoaW/o\n\tmLwQmlln8ljqA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685347069;\n\tbh=YT9VEO/0rzuYqsbmBYtaAW9RlLZwtjL+Gqw549JV4Lw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=grs3mLlKeiahNHocIwdybOWN1MPGMqWHso71Llv+eU+dEMsQPVqEA3y8rKcMq772C\n\tLTvmld8H60B190UcaRwN26xWdR1uyFeavCUxufh66UbGzCSZD4JcEsb1rcuwoUsGl/\n\tWRH/1h6LzazruXJ9vlFg6IVTtCR2UUw6FZ/xcoJI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"grs3mLlK\"; dkim-atps=neutral","Date":"Mon, 29 May 2023 09:58:05 +0200","To":"Harvey Yang ","Message-ID":"<4j2ae6q6hsny3hyda4gpposehsfkldhxik6esttteefcyd2wud@vghoxwijzg6q>","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230522083546.2465448-3-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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":27168,"web_url":"https://patchwork.libcamera.org/comment/27168/","msgid":"<20230530064250.GB6865@pendragon.ideasonboard.com>","date":"2023-05-30T06:42:50","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nThank you for the patch.\n\nOn Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:\n> From: Cheng-Hao Yang \n> \n> If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n\nudmabuf and the DMA heaps are two completely different things, meant for\ndifferent use cases. An allocator that would use \"whatever is available\"\nisn't a good generic helper.\n\nI expect writing the documentation will make you realize that there's a\ndesign issue here, as it's difficult to document clearly an API that\ndoesn't have a clear design.\n\n> Signed-off-by: Harvey Yang \n> ---\n> src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> 1 file changed, 100 insertions(+)\n> \n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> index 69f65062..8f99f590 100644\n> --- a/src/libcamera/heap_allocator.cpp\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -11,10 +11,14 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> +#include \n> #include \n> \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -54,6 +58,14 @@ public:\n> \tUniqueFD alloc(const char *name, std::size_t size) override;\n> };\n> \n> +class UdmaHeap : public Heap\n> +{\n> +public:\n> +\tUdmaHeap();\n> +\t~UdmaHeap();\n> +\tUniqueFD alloc(const char *name, std::size_t size) override;\n> +};\n> +\n> DmaHeap::DmaHeap()\n> {\n> \tfor (const char *name : dmaHeapNames) {\n> @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tLOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> \t\thandle_ = UniqueFD(ret);\n> \t\tbreak;\n> \t}\n> @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> \treturn allocFd;\n> }\n> \n> +UdmaHeap::UdmaHeap()\n> +{\n> +\tint ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to open allocator: \" << strerror(ret);\n> +\t} else {\n> +\t\tLOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> +\t\thandle_ = UniqueFD(ret);\n> +\t}\n> +}\n> +\n> +UdmaHeap::~UdmaHeap() = default;\n> +\n> +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> +{\n> +\tif (!isValid()) {\n> +\t\tLOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation attempted without allocator\" << name;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tint ret;\n> +\n> +\tret = memfd_create(name, MFD_ALLOW_SEALING);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to allocate memfd storage: \"\n> +\t\t\t<< strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tUniqueFD memfd(ret);\n> +\n> +\tret = ftruncate(memfd.get(), size);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> +\tret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tstruct udmabuf_create create;\n> +\n> +\tcreate.memfd = memfd.get();\n> +\tcreate.flags = UDMABUF_FLAGS_CLOEXEC;\n> +\tcreate.offset = 0;\n> +\tcreate.size = size;\n> +\n> +\tret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n> +\t\t\t<< strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* The underlying memfd is kept as as a reference in the kernel */\n> +\tUniqueFD uDma(ret);\n> +\n> +\tif (create.size != size)\n> +\t\tLOG(HeapAllocator, Warning)\n> +\t\t\t<< \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> +\t\t\t<< size << \" bytes\";\n> +\n> +\t/* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> +\tif (create.size < size)\n> +\t\treturn {};\n> +\n> +\tLOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> +\n> +\treturn uDma;\n> +}\n> +\n> HeapAllocator::HeapAllocator()\n> {\n> \theap_ = std::make_unique();\n> +\tif (!isValid())\n> +\t\theap_ = std::make_unique();\n> }\n> \n> HeapAllocator::~HeapAllocator() = default;","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 7EB05C31E9\n\tfor ;\n\tTue, 30 May 2023 06:42:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6881D626F9;\n\tTue, 30 May 2023 08:42:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC6A96038F\n\tfor ;\n\tTue, 30 May 2023 08:42:49 +0200 (CEST)","from pendragon.ideasonboard.com (om126205206011.34.openmobile.ne.jp\n\t[126.205.206.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B8AAE4;\n\tTue, 30 May 2023 08:42:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685428971;\n\tbh=TgFOsYNXp/2/BM2bFU2bm4BX/M13TOJLuad7BoE6mR4=;\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=RRpOxSm4+kSEwEm+tn38fat1OnZFYBqWEWJGxTdmB7JmCWnSIkhSm8CIvTHdS8H6A\n\t/cDC8dOJzkwZ6OpC0TRFe4QK0QTuBpXi9J+P0099a15inWan0WIsf1vOhx8sqG+lHc\n\taUhvOJIm601dBb7o3pJKLVr5WiiSfHFitlAXxMf3HUXN7s1hCKKXOCut5xekk5rNPn\n\tZSeSFjhwjEJ47kLS/pNcR9o0BpTvn6xLez4KSkX/QcLBYb1NmUh4YccHRBrK6QkCGo\n\t6dviVlVhNpOk6iKg0nh5QmWbkrv5/0ZXqehBfhfckJjwyFPsvoq3MXvhTZOptJmEAt\n\tivmPOdtPGy1pw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685428949;\n\tbh=TgFOsYNXp/2/BM2bFU2bm4BX/M13TOJLuad7BoE6mR4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Kn/TKf2PC1ENRnBqZmU5rZwqIbDwYKNpgwefBejU6iBZw32xNuFHb+XYpGCuK9YDq\n\tUR8pBmmmeUCTSTa86oA+0kYuDwSg74slc0gluiFN/kXJQ/dZEwpEXx0NRKa/58BYuD\n\tFesZGEAyMlTBVcGCFUz8ReAanwNMH8tV4tHfDIww="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Kn/TKf2P\"; dkim-atps=neutral","Date":"Tue, 30 May 2023 09:42:50 +0300","To":"Harvey Yang ","Message-ID":"<20230530064250.GB6865@pendragon.ideasonboard.com>","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230522083546.2465448-3-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27169,"web_url":"https://patchwork.libcamera.org/comment/27169/","msgid":"","date":"2023-05-30T06:47:48","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Harvey,\n\nOn 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote:\n> From: Cheng-Hao Yang \n>\n> If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n\nDoes udmaheap requires any addition to the udev rules? I noticed that \nthe dma_heap requires:\n\nhttps://github.com/RPi-Distro/raspberrypi-sys-mods/blob/master/etc.armhf/udev/rules.d/99-com.rules#L8\n\nsuch a rule, so the Allocator possibility can't be used out of the box \n(unless libcamera takes care of installing such a rule).\n\n> Signed-off-by: Harvey Yang \n> ---\n> src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> 1 file changed, 100 insertions(+)\n>\n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> index 69f65062..8f99f590 100644\n> --- a/src/libcamera/heap_allocator.cpp\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -11,10 +11,14 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> +#include \n> #include \n> \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -54,6 +58,14 @@ public:\n> \tUniqueFD alloc(const char *name, std::size_t size) override;\n> };\n> \n> +class UdmaHeap : public Heap\n> +{\n> +public:\n> +\tUdmaHeap();\n> +\t~UdmaHeap();\n> +\tUniqueFD alloc(const char *name, std::size_t size) override;\n> +};\n> +\n> DmaHeap::DmaHeap()\n> {\n> \tfor (const char *name : dmaHeapNames) {\n> @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tLOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> \t\thandle_ = UniqueFD(ret);\n> \t\tbreak;\n> \t}\n> @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> \treturn allocFd;\n> }\n> \n> +UdmaHeap::UdmaHeap()\n> +{\n> +\tint ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to open allocator: \" << strerror(ret);\n> +\t} else {\n> +\t\tLOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> +\t\thandle_ = UniqueFD(ret);\n> +\t}\n> +}\n> +\n> +UdmaHeap::~UdmaHeap() = default;\n> +\n> +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> +{\n> +\tif (!isValid()) {\n> +\t\tLOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation attempted without allocator\" << name;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tint ret;\n> +\n> +\tret = memfd_create(name, MFD_ALLOW_SEALING);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to allocate memfd storage: \"\n> +\t\t\t<< strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tUniqueFD memfd(ret);\n> +\n> +\tret = ftruncate(memfd.get(), size);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> +\tret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tstruct udmabuf_create create;\n> +\n> +\tcreate.memfd = memfd.get();\n> +\tcreate.flags = UDMABUF_FLAGS_CLOEXEC;\n> +\tcreate.offset = 0;\n> +\tcreate.size = size;\n> +\n> +\tret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(HeapAllocator, Error)\n> +\t\t\t<< \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n> +\t\t\t<< strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* The underlying memfd is kept as as a reference in the kernel */\n> +\tUniqueFD uDma(ret);\n> +\n> +\tif (create.size != size)\n> +\t\tLOG(HeapAllocator, Warning)\n> +\t\t\t<< \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> +\t\t\t<< size << \" bytes\";\n> +\n> +\t/* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> +\tif (create.size < size)\n> +\t\treturn {};\n> +\n> +\tLOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> +\n> +\treturn uDma;\n> +}\n> +\n> HeapAllocator::HeapAllocator()\n> {\n> \theap_ = std::make_unique();\n> +\tif (!isValid())\n> +\t\theap_ = std::make_unique();\n> }\n> \n> HeapAllocator::~HeapAllocator() = default;","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 536ADC3213\n\tfor ;\n\tTue, 30 May 2023 06:47:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7DEC6038F;\n\tTue, 30 May 2023 08:47:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E86E46038F\n\tfor ;\n\tTue, 30 May 2023 08:47:54 +0200 (CEST)","from [192.168.1.108] (unknown [103.86.18.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF841E4;\n\tTue, 30 May 2023 08:47:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685429276;\n\tbh=TN138JsJJq+RgOOhAUCbx8so1BsNELapKHpvwvN0Ivs=;\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:\n\tFrom;\n\tb=TsaJ4oe4/pjn1dSvrNjbqjp+a5w12jGI2XbR6YsXqd76vLiTqe9QuZxw3Ee9z0J2w\n\tVTSQrd6Qq7shBO0FsjwwxnsKnK8F1BBLKXLaNBDduflBI3iuJaN3QkOyfek2zdhwBj\n\tzYYOqE9Y3Fdf8y4NcsolLFJreHcI+TtPsvIjA4ZqXcWaLnkFu5TdPOtSkdHjr98Waq\n\t/jwinZw7yk9AH1lSvrZhuVmM0BvztB3VRGFghA42F0ac96iiYY1GiGjoSDP/cu60gl\n\tqkM5QfTnGuCjblVhYLXMhdIdoJnKyp6qIiFm+GLW0VTZLFbn8zUg8ZVksM79dNtssA\n\tInXUuJq/dZojQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685429254;\n\tbh=TN138JsJJq+RgOOhAUCbx8so1BsNELapKHpvwvN0Ivs=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=g0OyE9TBgCnucKBTOqGdEadh8vOhsB16aemvuMoG+hhr39WLGUqaJDJvEN9DuZl/S\n\tSptTd2DzydajF1QV5f/tiJ2+/wms+81lSVZrYHb2ALIo5Eo2OJzs3ePTDAqgYG+yHw\n\t4Pyz9OrfhJ1XD/LQgY1e4tR5hfPLNAEVrNtOF9MU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"g0OyE9TB\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 30 May 2023 12:17:48 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>","In-Reply-To":"<20230522083546.2465448-3-chenghaoyang@google.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27172,"web_url":"https://patchwork.libcamera.org/comment/27172/","msgid":"","date":"2023-05-30T10:14:54","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent, thanks for pointing it out. I'm a newbie to buffer allocation\nin linux, so here are my confusions before updating the patch:\n\nOn Tue, May 30, 2023 at 2:42 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey,\n>\n> Thank you for the patch.\n>\n> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel\n> wrote:\n> > From: Cheng-Hao Yang \n> >\n> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n>\n> udmabuf and the DMA heaps are two completely different things, meant for\n> different use cases. An allocator that would use \"whatever is available\"\n> isn't a good generic helper.\n>\n>\nI assume you mean udmabuf and DMA buffers, unless you mean the\ndifference between buffers and heaps:\nI saw [1], which states udmabuf as \"User space mappable DMA buffer\",\nso I assume that it's one kind of DMA buffer, especially accessible for\nuserspace.\n\nI agree that we shouldn't use \"whatever is available\" logic though. How\nabout an enum passed into HeapAllocator's c'tor that lets the user to\nspecify which heap(s) (with an order or not) to be used?\n\n[1]:\nhttps://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md\n\n\n> I expect writing the documentation will make you realize that there's a\n> design issue here, as it's difficult to document clearly an API that\n> doesn't have a clear design.\n>\n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> > 1 file changed, 100 insertions(+)\n> >\n> > diff --git a/src/libcamera/heap_allocator.cpp\n> b/src/libcamera/heap_allocator.cpp\n> > index 69f65062..8f99f590 100644\n> > --- a/src/libcamera/heap_allocator.cpp\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -11,10 +11,14 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > +#include \n> > +#include \n> > #include \n> >\n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -54,6 +58,14 @@ public:\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> > DmaHeap::DmaHeap()\n> > {\n> > for (const char *name : dmaHeapNames) {\n> > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> > continue;\n> > }\n> >\n> > + LOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> > handle_ = UniqueFD(ret);\n> > break;\n> > }\n> > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name,\n> std::size_t size)\n> > return allocFd;\n> > }\n> >\n> > +UdmaHeap::UdmaHeap()\n> > +{\n> > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to open allocator: \" <<\n> strerror(ret);\n> > + } else {\n> > + LOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> > + handle_ = UniqueFD(ret);\n> > + }\n> > +}\n> > +\n> > +UdmaHeap::~UdmaHeap() = default;\n> > +\n> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> > +{\n> > + if (!isValid()) {\n> > + LOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation\n> attempted without allocator\" << name;\n> > + return {};\n> > + }\n> > +\n> > + int ret;\n> > +\n> > + ret = memfd_create(name, MFD_ALLOW_SEALING);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to allocate memfd storage: \"\n> > + << strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + UniqueFD memfd(ret);\n> > +\n> > + ret = ftruncate(memfd.get(), size);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to set memfd size: \" <<\n> strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to seal the memfd: \" <<\n> strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + struct udmabuf_create create;\n> > +\n> > + create.memfd = memfd.get();\n> > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> > + create.offset = 0;\n> > + create.size = size;\n> > +\n> > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to allocate \" << size << \"\n> bytes: \"\n> > + << strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + /* The underlying memfd is kept as as a reference in the kernel */\n> > + UniqueFD uDma(ret);\n> > +\n> > + if (create.size != size)\n> > + LOG(HeapAllocator, Warning)\n> > + << \"UdmaHeap allocated \" << create.size << \" bytes\n> instead of \"\n> > + << size << \" bytes\";\n> > +\n> > + /* Fail if not suitable, the allocation will be free'd by UniqueFD\n> */\n> > + if (create.size < size)\n> > + return {};\n> > +\n> > + LOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size\n> << \" bytes\";\n> > +\n> > + return uDma;\n> > +}\n> > +\n> > HeapAllocator::HeapAllocator()\n> > {\n> > heap_ = std::make_unique();\n> > + if (!isValid())\n> > + heap_ = std::make_unique();\n> > }\n> >\n> > HeapAllocator::~HeapAllocator() = default;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 BDC91C31E9\n\tfor ;\n\tTue, 30 May 2023 10:15:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 295B660599;\n\tTue, 30 May 2023 12:15:08 +0200 (CEST)","from mail-ua1-x929.google.com (mail-ua1-x929.google.com\n\t[IPv6:2607:f8b0:4864:20::929])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F1C060595\n\tfor ;\n\tTue, 30 May 2023 12:15:06 +0200 (CEST)","by mail-ua1-x929.google.com with SMTP id\n\ta1e0cc1a2514c-7868d32ace2so2863932241.3\n\tfor ;\n\tTue, 30 May 2023 03:15:06 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685441708;\n\tbh=zvCcfk1NPMwKwtwRp4ySuMyRAdrae31tiDBFECp2wUc=;\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=f6oDlvb51pn4lPtOj62MBfJrwsEEdJ+Wpjc/ZjIZSx4B8Gx+XxZjntxoS/iNAT0ZS\n\t/DWAxHpWHORCNDRoCdkfTDgUghHsvCOPOTq0aIIkkk6nqCMwct2Zn+AZvJyhb/5Im1\n\tNGpVLaWBDYmqJuCKALYg9dB/IMtbfmZZpkWSpZlF11qxyl4AzfUATvmLY7nEb9avW8\n\ta87THB9B1chKKIuKhrJd8ECzafN8wHaw4PURrEdv/w1+yOpVkdSNz0FvO+eeE7nOT2\n\tgOK4O73X8fAaBF9KfQBl8oP2m4C0ujcHN/3uRnPVTCJEC18BX48v/RjO8p9NcgkGH1\n\tMp2Iq6nldjZTw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1685441705; x=1688033705;\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=aW7otCYelPW99cEh5VajahF6SjlE4IykL44N7JxUHpE=;\n\tb=HArwnt+al3PRDuVJDBjzRtj+dArYQ9SbLZdgLQRj/PYig9NJ7Wm3jXeiV7dMk9rmMn\n\tR0IRIeeDVsDAh40vFZgpK8/1OQXxuC+l4+j2w429fzA4k21UFe+wXF5BCCPLzWIgiFWF\n\tVheXiuy7ScDpvF0WS4bWatLYsOjmkDOCH4nac="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"HArwnt+a\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685441705; x=1688033705;\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=aW7otCYelPW99cEh5VajahF6SjlE4IykL44N7JxUHpE=;\n\tb=PJNweo/v3nP44+S5UTwFhVzJvA9JI554S5KMl7QnAGfwJKZRGZEKC/Sdn779o2k+oM\n\tCdDxwGshmNy4qXVvp9ylcRISGtVc/SK2lzJutaDp9h3segatJoXZVcUZqiei6Q7TMJO5\n\tAXBWB9fSqDLSF1JSUeraatjrdqfXqmdiOWGh3X3YokZqG0Uus/BqGcOCd/SkA0CnCw3O\n\tbqhpn6vjVT+//HXxK6brddLXnark+PJVocdvXSTGofkke0VF8OsPWWyPNJphofHOoAPr\n\tFMXWyCPLnkCNsFtJ+7wuYrnltcsCdEuaohlppvuHL2szipHxa0azaOvcF1bvXIRcKyF5\n\t4KSg==","X-Gm-Message-State":"AC+VfDyAEmBKe1eUDtVIc6bvahZQsLMhCeYjpNDvf9WdjVGfjwKMCZiJ\n\tunDkUVQRULmTQG6N2WiYmRhsLG/JVxDBmZwO6YOTLqJ/1bU4fD4qJ/c+iA==","X-Google-Smtp-Source":"ACHHUZ6qhpTNSk/OY53XY3feTHhWRBHIRriDdAW0sAc1eTporwHAnWdaNK2hucRV71riKB/wq9vpWQLJA8PqtsB++2k=","X-Received":"by 2002:a67:e208:0:b0:434:6dcf:5e13 with SMTP id\n\tg8-20020a67e208000000b004346dcf5e13mr560442vsa.18.1685441704966;\n\tTue, 30 May 2023 03:15:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>\n\t<20230530064250.GB6865@pendragon.ideasonboard.com>","In-Reply-To":"<20230530064250.GB6865@pendragon.ideasonboard.com>","Date":"Tue, 30 May 2023 18:14:54 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000001d51fb05fce67af5\"","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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\" "}},{"id":27216,"web_url":"https://patchwork.libcamera.org/comment/27216/","msgid":"","date":"2023-06-02T06:39:33","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Also, if we let users decide which heap implementations to be used\ndirectly, maybe there's no point having class HeapAllocator at all.\nWe can just give users (pipeline handlers) access to each Heap's\nimplementation directly.\n\nWDYT?\n\nOn Tue, May 30, 2023 at 6:14 PM Cheng-Hao Yang \nwrote:\n\n> Hi Laurent, thanks for pointing it out. I'm a newbie to buffer allocation\n> in linux, so here are my confusions before updating the patch:\n>\n> On Tue, May 30, 2023 at 2:42 PM Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi Harvey,\n>>\n>> Thank you for the patch.\n>>\n>> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel\n>> wrote:\n>> > From: Cheng-Hao Yang \n>> >\n>> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n>>\n>> udmabuf and the DMA heaps are two completely different things, meant for\n>> different use cases. An allocator that would use \"whatever is available\"\n>> isn't a good generic helper.\n>>\n>>\n> I assume you mean udmabuf and DMA buffers, unless you mean the\n> difference between buffers and heaps:\n> I saw [1], which states udmabuf as \"User space mappable DMA buffer\",\n> so I assume that it's one kind of DMA buffer, especially accessible for\n> userspace.\n>\n> I agree that we shouldn't use \"whatever is available\" logic though. How\n> about an enum passed into HeapAllocator's c'tor that lets the user to\n> specify which heap(s) (with an order or not) to be used?\n>\n> [1]:\n> https://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md\n>\n>\n>> I expect writing the documentation will make you realize that there's a\n>> design issue here, as it's difficult to document clearly an API that\n>> doesn't have a clear design.\n>>\n>> > Signed-off-by: Harvey Yang \n>> > ---\n>> > src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n>> > 1 file changed, 100 insertions(+)\n>> >\n>> > diff --git a/src/libcamera/heap_allocator.cpp\n>> b/src/libcamera/heap_allocator.cpp\n>> > index 69f65062..8f99f590 100644\n>> > --- a/src/libcamera/heap_allocator.cpp\n>> > +++ b/src/libcamera/heap_allocator.cpp\n>> > @@ -11,10 +11,14 @@\n>> > #include \n>> > #include \n>> > #include \n>> > +#include \n>> > +#include \n>> > +#include \n>> > #include \n>> >\n>> > #include \n>> > #include \n>> > +#include \n>> >\n>> > #include \n>> >\n>> > @@ -54,6 +58,14 @@ public:\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>> > DmaHeap::DmaHeap()\n>> > {\n>> > for (const char *name : dmaHeapNames) {\n>> > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n>> > continue;\n>> > }\n>> >\n>> > + LOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n>> > handle_ = UniqueFD(ret);\n>> > break;\n>> > }\n>> > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name,\n>> std::size_t size)\n>> > return allocFd;\n>> > }\n>> >\n>> > +UdmaHeap::UdmaHeap()\n>> > +{\n>> > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n>> > + if (ret < 0) {\n>> > + ret = errno;\n>> > + LOG(HeapAllocator, Error)\n>> > + << \"UdmaHeap failed to open allocator: \" <<\n>> strerror(ret);\n>> > + } else {\n>> > + LOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n>> > + handle_ = UniqueFD(ret);\n>> > + }\n>> > +}\n>> > +\n>> > +UdmaHeap::~UdmaHeap() = default;\n>> > +\n>> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n>> > +{\n>> > + if (!isValid()) {\n>> > + LOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation\n>> attempted without allocator\" << name;\n>> > + return {};\n>> > + }\n>> > +\n>> > + int ret;\n>> > +\n>> > + ret = memfd_create(name, MFD_ALLOW_SEALING);\n>> > + if (ret < 0) {\n>> > + ret = errno;\n>> > + LOG(HeapAllocator, Error)\n>> > + << \"UdmaHeap failed to allocate memfd storage: \"\n>> > + << strerror(ret);\n>> > + return {};\n>> > + }\n>> > +\n>> > + UniqueFD memfd(ret);\n>> > +\n>> > + ret = ftruncate(memfd.get(), size);\n>> > + if (ret < 0) {\n>> > + ret = errno;\n>> > + LOG(HeapAllocator, Error)\n>> > + << \"UdmaHeap failed to set memfd size: \" <<\n>> strerror(ret);\n>> > + return {};\n>> > + }\n>> > +\n>> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n>> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n>> > + if (ret < 0) {\n>> > + ret = errno;\n>> > + LOG(HeapAllocator, Error)\n>> > + << \"UdmaHeap failed to seal the memfd: \" <<\n>> strerror(ret);\n>> > + return {};\n>> > + }\n>> > +\n>> > + struct udmabuf_create create;\n>> > +\n>> > + create.memfd = memfd.get();\n>> > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n>> > + create.offset = 0;\n>> > + create.size = size;\n>> > +\n>> > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n>> > + if (ret < 0) {\n>> > + ret = errno;\n>> > + LOG(HeapAllocator, Error)\n>> > + << \"UdmaHeap failed to allocate \" << size << \"\n>> bytes: \"\n>> > + << strerror(ret);\n>> > + return {};\n>> > + }\n>> > +\n>> > + /* The underlying memfd is kept as as a reference in the kernel */\n>> > + UniqueFD uDma(ret);\n>> > +\n>> > + if (create.size != size)\n>> > + LOG(HeapAllocator, Warning)\n>> > + << \"UdmaHeap allocated \" << create.size << \"\n>> bytes instead of \"\n>> > + << size << \" bytes\";\n>> > +\n>> > + /* Fail if not suitable, the allocation will be free'd by\n>> UniqueFD */\n>> > + if (create.size < size)\n>> > + return {};\n>> > +\n>> > + LOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size\n>> << \" bytes\";\n>> > +\n>> > + return uDma;\n>> > +}\n>> > +\n>> > HeapAllocator::HeapAllocator()\n>> > {\n>> > heap_ = std::make_unique();\n>> > + if (!isValid())\n>> > + heap_ = std::make_unique();\n>> > }\n>> >\n>> > HeapAllocator::~HeapAllocator() = default;\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\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 EFA83C3200\n\tfor ;\n\tFri, 2 Jun 2023 06:39:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55DA462728;\n\tFri, 2 Jun 2023 08:39:46 +0200 (CEST)","from mail-vk1-xa2a.google.com (mail-vk1-xa2a.google.com\n\t[IPv6:2607:f8b0:4864:20::a2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 563CB61EA5\n\tfor ;\n\tFri, 2 Jun 2023 08:39:45 +0200 (CEST)","by mail-vk1-xa2a.google.com with SMTP id\n\t71dfb90a1353d-456fc318dd4so520327e0c.0\n\tfor ;\n\tThu, 01 Jun 2023 23:39:45 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685687986;\n\tbh=SarzwuO7B178qYvmEYA0Eeal13qog2WAPV37xAiY5ts=;\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=mDbnSOG/RmHvHU8ISY9410il7Uo4UG4I6xI9ditXJxv/KzMSBHuvnp/rAPVvLtLYB\n\tr0hj4SNKcAtOzMWG9EzkguDtL/Fx7mmh5Cw4EwRXvx+f9k/GQi1WlFnoKJU4cvtxtt\n\tzhi4bMdW9lWEk0sUmHUBgiHU9ia85Gdkt0Dc4xgJ2TpOhvkQBFojWDncd3/pT4q2d1\n\tWnKVPjltbVvkXmROWe7Aeb1cqt0DC8a27foIGKm0sNyt/KdV5S2leGEBYJsAKVwcLE\n\t6Q2lgfh2TXH+lvXGXmAdEUQXvEnbZey5r1LVrXW4rQmXe8YJSOtsWCRe0YmoIVD9Vk\n\txXYZ9uqkMqo8g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1685687984; x=1688279984;\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=+Qk/dxw4yj7KgOpBEg+bjpJ36aDtOguDQ1Vl8+ZMgoE=;\n\tb=OzC2/WOop65+Wvi+znCVLc2drNRZD7q5d1gEXEULo7126S+gqkiKvzcB+wLfN5KZjy\n\tEJz/Ecjjx2Lx46YP7PEYzb9f35uHpvW/TUl/fZxsl6+bFJMLft6tDQgQ8juqQY0lnMTd\n\teynNvCsdAoDmmnfeEqbR7rNNxkHu59QlKQc+k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"OzC2/WOo\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685687984; x=1688279984;\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=+Qk/dxw4yj7KgOpBEg+bjpJ36aDtOguDQ1Vl8+ZMgoE=;\n\tb=LzYfAR4sRVtTWORfiPzlDtBZM5rNhyaL3jxsYZwTDkbGsGYVs+/DJrv8Kserf9yvrm\n\t4vz0C5iPge4tkY8AGS3ZDpkfMCaq37rApYKi2HJ2lLjW+UowGSMmexXIPnt1sRRqVmj3\n\txq5z+RDPMrx4IbkIRXqrsrK84Z1P2A4xhkf11m9AYX5xAbzOnXT9yGfZ0d3J/ZmHsaPt\n\tEV/ersT2Us39MfGlAgOUG5pmNPNjpf5JQtIF9W+beMwivj2hBTrDnjikeEMPe5iEsKzf\n\t3xpr+JNiEkhX1tTRXi0VnT2FecSlksDULyKFo3oHvfi3M6AaLE40cpBJjGO0EEo+D/xP\n\t8wjQ==","X-Gm-Message-State":"AC+VfDxlcpfBZoHQTQjhPipbki7IWd2fVUAqdh8w48e5hGNb7BsI4khT\n\tsasbc71NxZjaRXk7jx9ZahhlHl9NrtKlqbnu12YXLMq3/bLwBSwvOUA=","X-Google-Smtp-Source":"ACHHUZ56m7oXyW3s2ca5tCT+MY85nqq9i7E6KxIIjxXmcvYCciUHkt4T1DOo8KfoJERMSxWLA96oezy+r5GI3LkEsAQ=","X-Received":"by 2002:a1f:d1c5:0:b0:457:5379:daed with SMTP id\n\ti188-20020a1fd1c5000000b004575379daedmr2117278vkg.1.1685687984088;\n\tThu, 01 Jun 2023 23:39:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>\n\t<20230530064250.GB6865@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","Date":"Fri, 2 Jun 2023 14:39:33 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000007e820f05fd1fd1c4\"","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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\" "}},{"id":27220,"web_url":"https://patchwork.libcamera.org/comment/27220/","msgid":"<20230602083234.GC19463@pendragon.ideasonboard.com>","date":"2023-06-02T08:32:34","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nOn Fri, Jun 02, 2023 at 02:39:33PM +0800, Cheng-Hao Yang wrote:\n> Also, if we let users decide which heap implementations to be used\n> directly, maybe there's no point having class HeapAllocator at all.\n> We can just give users (pipeline handlers) access to each Heap's\n> implementation directly.\n> \n> WDYT?\n\nThe first question to answer is what use case(s) you're trying to\naddress. Let's assume we would give users of the allocator class a\nchoice regarding what heap they allocate from (system vs. cma, those are\nthe only two options available in mainline). How would the user decide\nwhich heap to use ? You can use the virtual pipeline handler as one use\ncase to answer that question (try to keep the big picture in mind, what,\nfor instance, if the virtual pipeline handler were to use a hardware\ndevice - such as the GPU - to post-process images generated by the CPU,\nor what if we wanted to achieve zero-copy when display images generated\nby the virtual pipeline handler ?), and considering other use cases\nwould be helpful to design a good solution.\n\n> On Tue, May 30, 2023 at 6:14 PM Cheng-Hao Yang wrote:\n> \n> > Hi Laurent, thanks for pointing it out. I'm a newbie to buffer allocation\n> > in linux, so here are my confusions before updating the patch:\n> >\n> > On Tue, May 30, 2023 at 2:42 PM Laurent Pinchart wrote:\n> >> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:\n> >> > From: Cheng-Hao Yang \n> >> >\n> >> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n> >>\n> >> udmabuf and the DMA heaps are two completely different things, meant for\n> >> different use cases. An allocator that would use \"whatever is available\"\n> >> isn't a good generic helper.\n> >\n> > I assume you mean udmabuf and DMA buffers, unless you mean the\n> > difference between buffers and heaps:\n> > I saw [1], which states udmabuf as \"User space mappable DMA buffer\",\n> > so I assume that it's one kind of DMA buffer, especially accessible for\n> > userspace.\n\nThat's a different udmabuf than the one present in the mainline kernel\nas far as I can tell. It seems to be an out-of-tree implementation, from\nhttps://github.com/ikwzm/udmabuf.\n\nThe mainline udmabuf is a kernel API that allows wrapping a memfd into a\ndmabuf object. Userspace gets a dmabuf FD, usable with all kernel APIs\nthat expect a dmabuf FD. memfd creates an anonymous file, backed by\nanonymous memory, so you will essentially get discontiguous pages. If\nI'm not mistaken, those pages are pinned to memory by calling\nshmem_read_mapping_page() when the udmabuf is created, but don't quote\nme on that.\n\nDMA heaps, on the other hand, are a set of memory allocators aimed to\nprovide memory regions suitable for devices to use for DMA. The\nallocators also expose the buffers as dmabuf FDs to userspace, and\nthat's the only thing they have in common with udmabuf. With DMA heaps,\nuserspace picks one of the allocators provided by the system, and asks\nthe kernel to allocate buffers using that allocator.\n\nThe \"system\" heap uses alloc_pages() and tries to split the allocation\nin the smallest number of page sets, with each of the sets using the\nhighest possible page order. From a device DMA point of view, this is\nsimilar to udmabuf, neither are suitable for usage with devices that\nrequire DMA-contiguous memory and don't have an IOMMU. \n\nThe \"cma\" heap uses cma_alloc(), which guarantees physically-contiguous\nmemory. The memory is suitable for DMA with many (but not all) devices.\nThe downside is that the CMA area is limited in size, and physically\ncontiguous memory shouldn't be used when the device doesn't require it.\n\nThere is unfortunately no DMA heap that allocates DMA-capable buffers\nfor a specific device. Lots of work is still needed to provide Linux\nsystems with a unified device memory allocator.\n\nBased on the above, the issue isn't so much about DMA heaps vs. udmabuf\nas I initially implied, but more about system heap and udmabuf vs. CMA\nheap. I think the system heap and udmabuf are likely to provide similar\ntypes of allocations from the point of view of DMA. The page allocation\norder, however, may be significantly different. I haven't investigated\nthat.\n\n> > I agree that we shouldn't use \"whatever is available\" logic though. How\n> > about an enum passed into HeapAllocator's c'tor that lets the user to\n> > specify which heap(s) (with an order or not) to be used?\n> >\n> > [1]: https://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md\n> >\n> >> I expect writing the documentation will make you realize that there's a\n> >> design issue here, as it's difficult to document clearly an API that\n> >> doesn't have a clear design.\n> >>\n> >> > Signed-off-by: Harvey Yang \n> >> > ---\n> >> > src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> >> > 1 file changed, 100 insertions(+)\n> >> >\n> >> > diff --git a/src/libcamera/heap_allocator.cpp\n> >> b/src/libcamera/heap_allocator.cpp\n> >> > index 69f65062..8f99f590 100644\n> >> > --- a/src/libcamera/heap_allocator.cpp\n> >> > +++ b/src/libcamera/heap_allocator.cpp\n> >> > @@ -11,10 +11,14 @@\n> >> > #include \n> >> > #include \n> >> > #include \n> >> > +#include \n> >> > +#include \n> >> > +#include \n> >> > #include \n> >> >\n> >> > #include \n> >> > #include \n> >> > +#include \n> >> >\n> >> > #include \n> >> >\n> >> > @@ -54,6 +58,14 @@ public:\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> >> > DmaHeap::DmaHeap()\n> >> > {\n> >> > for (const char *name : dmaHeapNames) {\n> >> > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> >> > continue;\n> >> > }\n> >> >\n> >> > + LOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> >> > handle_ = UniqueFD(ret);\n> >> > break;\n> >> > }\n> >> > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> >> > return allocFd;\n> >> > }\n> >> >\n> >> > +UdmaHeap::UdmaHeap()\n> >> > +{\n> >> > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> >> > + if (ret < 0) {\n> >> > + ret = errno;\n> >> > + LOG(HeapAllocator, Error)\n> >> > + << \"UdmaHeap failed to open allocator: \" << strerror(ret);\n> >> > + } else {\n> >> > + LOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> >> > + handle_ = UniqueFD(ret);\n> >> > + }\n> >> > +}\n> >> > +\n> >> > +UdmaHeap::~UdmaHeap() = default;\n> >> > +\n> >> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> >> > +{\n> >> > + if (!isValid()) {\n> >> > + LOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation attempted without allocator\" << name;\n> >> > + return {};\n> >> > + }\n> >> > +\n> >> > + int ret;\n> >> > +\n> >> > + ret = memfd_create(name, MFD_ALLOW_SEALING);\n> >> > + if (ret < 0) {\n> >> > + ret = errno;\n> >> > + LOG(HeapAllocator, Error)\n> >> > + << \"UdmaHeap failed to allocate memfd storage: \"\n> >> > + << strerror(ret);\n> >> > + return {};\n> >> > + }\n> >> > +\n> >> > + UniqueFD memfd(ret);\n> >> > +\n> >> > + ret = ftruncate(memfd.get(), size);\n> >> > + if (ret < 0) {\n> >> > + ret = errno;\n> >> > + LOG(HeapAllocator, Error)\n> >> > + << \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> >> > + return {};\n> >> > + }\n> >> > +\n> >> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> >> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> >> > + if (ret < 0) {\n> >> > + ret = errno;\n> >> > + LOG(HeapAllocator, Error)\n> >> > + << \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> >> > + return {};\n> >> > + }\n> >> > +\n> >> > + struct udmabuf_create create;\n> >> > +\n> >> > + create.memfd = memfd.get();\n> >> > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> >> > + create.offset = 0;\n> >> > + create.size = size;\n> >> > +\n> >> > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> >> > + if (ret < 0) {\n> >> > + ret = errno;\n> >> > + LOG(HeapAllocator, Error)\n> >> > + << \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n> >> > + << strerror(ret);\n> >> > + return {};\n> >> > + }\n> >> > +\n> >> > + /* The underlying memfd is kept as as a reference in the kernel */\n> >> > + UniqueFD uDma(ret);\n> >> > +\n> >> > + if (create.size != size)\n> >> > + LOG(HeapAllocator, Warning)\n> >> > + << \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> >> > + << size << \" bytes\";\n> >> > +\n> >> > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> >> > + if (create.size < size)\n> >> > + return {};\n> >> > +\n> >> > + LOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> >> > +\n> >> > + return uDma;\n> >> > +}\n> >> > +\n> >> > HeapAllocator::HeapAllocator()\n> >> > {\n> >> > heap_ = std::make_unique();\n> >> > + if (!isValid())\n> >> > + heap_ = std::make_unique();\n> >> > }\n> >> >\n> >> > HeapAllocator::~HeapAllocator() = default;","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 1833FC3200\n\tfor ;\n\tFri, 2 Jun 2023 08:32:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D5A3626F5;\n\tFri, 2 Jun 2023 10:32:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75B8C626F5\n\tfor ;\n\tFri, 2 Jun 2023 10:32:36 +0200 (CEST)","from pendragon.ideasonboard.com (om126156168104.26.openmobile.ne.jp\n\t[126.156.168.104])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0060127C;\n\tFri, 2 Jun 2023 10:32:12 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685694757;\n\tbh=AVZDuemJrtgr+cHw1oliMabteAwjF0h4SxIyOqAXuqQ=;\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=ZOMtdiJQXVqmxQry/cZI/f24GtEF8itaxsToVDgt1wY1OKbVCebt9OEFLnBZ3nYVB\n\tyd/fim67uQfsZ5764x2/hGB/TOIIx49SVP8B5Egb2Oif3sciyk9Uib53lf6ANL+zeL\n\t735MFH1NcH91GQEaCij9KqQic54yxjqoyjuCKJrvgyEnIxZv347dLyPuLxkuwmt6nw\n\th/mkEIi6gQz7fo4HMATSAInnHepuOfEGEUn6SOlqQr4jlA2fVG3IyTFiw2mYlI/ZJg\n\tDeXRiY7AOne0tRE3NhXn0WMRJXcDA8iDStZLJmI1TugK4NPiwCdEkJEsOgGqhAD3fx\n\tcJuRsZR9kIO1Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685694733;\n\tbh=AVZDuemJrtgr+cHw1oliMabteAwjF0h4SxIyOqAXuqQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qgvIkCFSOZSG7oBrVADYS+drPHNE4U9+qAsY91FJRbEd2PsAZ/V1UGb0QLNPUTae0\n\tJSGl5D8MRr5kEb+0ng6Znf5W/wH6OsfPQEWkQsr6Kp0Y1LpFGqHXIFpGJgLu6S/D1p\n\thoZbOcIraZ2m9efBYw7zgHfbcp1bwSz4yOciuXN4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qgvIkCFS\"; dkim-atps=neutral","Date":"Fri, 2 Jun 2023 11:32:34 +0300","To":"Cheng-Hao Yang ","Message-ID":"<20230602083234.GC19463@pendragon.ideasonboard.com>","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>\n\t<20230530064250.GB6865@pendragon.ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27385,"web_url":"https://patchwork.libcamera.org/comment/27385/","msgid":"","date":"2023-06-19T05:55:10","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nAs discussed in the libcamera weekly meeting before, have you brought out\nthe discussion in irc.oftc.net with people from Debian and Fedora about\nsupporting udmabuf (or DMA buf) in some common linux distributions? I'd\nlike to know the updates if any.\n\nThanks!\n\n\nOn Fri, Jun 2, 2023 at 4:32 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey,\n>\n> On Fri, Jun 02, 2023 at 02:39:33PM +0800, Cheng-Hao Yang wrote:\n> > Also, if we let users decide which heap implementations to be used\n> > directly, maybe there's no point having class HeapAllocator at all.\n> > We can just give users (pipeline handlers) access to each Heap's\n> > implementation directly.\n> >\n> > WDYT?\n>\n> The first question to answer is what use case(s) you're trying to\n> address. Let's assume we would give users of the allocator class a\n> choice regarding what heap they allocate from (system vs. cma, those are\n> the only two options available in mainline). How would the user decide\n> which heap to use ? You can use the virtual pipeline handler as one use\n> case to answer that question (try to keep the big picture in mind, what,\n> for instance, if the virtual pipeline handler were to use a hardware\n> device - such as the GPU - to post-process images generated by the CPU,\n> or what if we wanted to achieve zero-copy when display images generated\n> by the virtual pipeline handler ?), and considering other use cases\n> would be helpful to design a good solution.\n>\n> > On Tue, May 30, 2023 at 6:14 PM Cheng-Hao Yang wrote:\n> >\n> > > Hi Laurent, thanks for pointing it out. I'm a newbie to buffer\n> allocation\n> > > in linux, so here are my confusions before updating the patch:\n> > >\n> > > On Tue, May 30, 2023 at 2:42 PM Laurent Pinchart wrote:\n> > >> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via\n> libcamera-devel wrote:\n> > >> > From: Cheng-Hao Yang \n> > >> >\n> > >> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n> > >>\n> > >> udmabuf and the DMA heaps are two completely different things, meant\n> for\n> > >> different use cases. An allocator that would use \"whatever is\n> available\"\n> > >> isn't a good generic helper.\n> > >\n> > > I assume you mean udmabuf and DMA buffers, unless you mean the\n> > > difference between buffers and heaps:\n> > > I saw [1], which states udmabuf as \"User space mappable DMA buffer\",\n> > > so I assume that it's one kind of DMA buffer, especially accessible for\n> > > userspace.\n>\n> That's a different udmabuf than the one present in the mainline kernel\n> as far as I can tell. It seems to be an out-of-tree implementation, from\n> https://github.com/ikwzm/udmabuf.\n>\n> The mainline udmabuf is a kernel API that allows wrapping a memfd into a\n> dmabuf object. Userspace gets a dmabuf FD, usable with all kernel APIs\n> that expect a dmabuf FD. memfd creates an anonymous file, backed by\n> anonymous memory, so you will essentially get discontiguous pages. If\n> I'm not mistaken, those pages are pinned to memory by calling\n> shmem_read_mapping_page() when the udmabuf is created, but don't quote\n> me on that.\n>\n> DMA heaps, on the other hand, are a set of memory allocators aimed to\n> provide memory regions suitable for devices to use for DMA. The\n> allocators also expose the buffers as dmabuf FDs to userspace, and\n> that's the only thing they have in common with udmabuf. With DMA heaps,\n> userspace picks one of the allocators provided by the system, and asks\n> the kernel to allocate buffers using that allocator.\n>\n> The \"system\" heap uses alloc_pages() and tries to split the allocation\n> in the smallest number of page sets, with each of the sets using the\n> highest possible page order. From a device DMA point of view, this is\n> similar to udmabuf, neither are suitable for usage with devices that\n> require DMA-contiguous memory and don't have an IOMMU.\n>\n> The \"cma\" heap uses cma_alloc(), which guarantees physically-contiguous\n> memory. The memory is suitable for DMA with many (but not all) devices.\n> The downside is that the CMA area is limited in size, and physically\n> contiguous memory shouldn't be used when the device doesn't require it.\n>\n> There is unfortunately no DMA heap that allocates DMA-capable buffers\n> for a specific device. Lots of work is still needed to provide Linux\n> systems with a unified device memory allocator.\n>\n> Based on the above, the issue isn't so much about DMA heaps vs. udmabuf\n> as I initially implied, but more about system heap and udmabuf vs. CMA\n> heap. I think the system heap and udmabuf are likely to provide similar\n> types of allocations from the point of view of DMA. The page allocation\n> order, however, may be significantly different. I haven't investigated\n> that.\n>\n> > > I agree that we shouldn't use \"whatever is available\" logic though. How\n> > > about an enum passed into HeapAllocator's c'tor that lets the user to\n> > > specify which heap(s) (with an order or not) to be used?\n> > >\n> > > [1]:\n> https://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md\n> > >\n> > >> I expect writing the documentation will make you realize that there's\n> a\n> > >> design issue here, as it's difficult to document clearly an API that\n> > >> doesn't have a clear design.\n> > >>\n> > >> > Signed-off-by: Harvey Yang \n> > >> > ---\n> > >> > src/libcamera/heap_allocator.cpp | 100\n> +++++++++++++++++++++++++++++++\n> > >> > 1 file changed, 100 insertions(+)\n> > >> >\n> > >> > diff --git a/src/libcamera/heap_allocator.cpp\n> > >> b/src/libcamera/heap_allocator.cpp\n> > >> > index 69f65062..8f99f590 100644\n> > >> > --- a/src/libcamera/heap_allocator.cpp\n> > >> > +++ b/src/libcamera/heap_allocator.cpp\n> > >> > @@ -11,10 +11,14 @@\n> > >> > #include \n> > >> > #include \n> > >> > #include \n> > >> > +#include \n> > >> > +#include \n> > >> > +#include \n> > >> > #include \n> > >> >\n> > >> > #include \n> > >> > #include \n> > >> > +#include \n> > >> >\n> > >> > #include \n> > >> >\n> > >> > @@ -54,6 +58,14 @@ public:\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> > >> > DmaHeap::DmaHeap()\n> > >> > {\n> > >> > for (const char *name : dmaHeapNames) {\n> > >> > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> > >> > continue;\n> > >> > }\n> > >> >\n> > >> > + LOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> > >> > handle_ = UniqueFD(ret);\n> > >> > break;\n> > >> > }\n> > >> > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name,\n> std::size_t size)\n> > >> > return allocFd;\n> > >> > }\n> > >> >\n> > >> > +UdmaHeap::UdmaHeap()\n> > >> > +{\n> > >> > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> > >> > + if (ret < 0) {\n> > >> > + ret = errno;\n> > >> > + LOG(HeapAllocator, Error)\n> > >> > + << \"UdmaHeap failed to open allocator: \" <<\n> strerror(ret);\n> > >> > + } else {\n> > >> > + LOG(HeapAllocator, Info) << \"Using UdmaHeap\n> allocator\";\n> > >> > + handle_ = UniqueFD(ret);\n> > >> > + }\n> > >> > +}\n> > >> > +\n> > >> > +UdmaHeap::~UdmaHeap() = default;\n> > >> > +\n> > >> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> > >> > +{\n> > >> > + if (!isValid()) {\n> > >> > + LOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation\n> attempted without allocator\" << name;\n> > >> > + return {};\n> > >> > + }\n> > >> > +\n> > >> > + int ret;\n> > >> > +\n> > >> > + ret = memfd_create(name, MFD_ALLOW_SEALING);\n> > >> > + if (ret < 0) {\n> > >> > + ret = errno;\n> > >> > + LOG(HeapAllocator, Error)\n> > >> > + << \"UdmaHeap failed to allocate memfd\n> storage: \"\n> > >> > + << strerror(ret);\n> > >> > + return {};\n> > >> > + }\n> > >> > +\n> > >> > + UniqueFD memfd(ret);\n> > >> > +\n> > >> > + ret = ftruncate(memfd.get(), size);\n> > >> > + if (ret < 0) {\n> > >> > + ret = errno;\n> > >> > + LOG(HeapAllocator, Error)\n> > >> > + << \"UdmaHeap failed to set memfd size: \" <<\n> strerror(ret);\n> > >> > + return {};\n> > >> > + }\n> > >> > +\n> > >> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> > >> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> > >> > + if (ret < 0) {\n> > >> > + ret = errno;\n> > >> > + LOG(HeapAllocator, Error)\n> > >> > + << \"UdmaHeap failed to seal the memfd: \" <<\n> strerror(ret);\n> > >> > + return {};\n> > >> > + }\n> > >> > +\n> > >> > + struct udmabuf_create create;\n> > >> > +\n> > >> > + create.memfd = memfd.get();\n> > >> > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> > >> > + create.offset = 0;\n> > >> > + create.size = size;\n> > >> > +\n> > >> > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> > >> > + if (ret < 0) {\n> > >> > + ret = errno;\n> > >> > + LOG(HeapAllocator, Error)\n> > >> > + << \"UdmaHeap failed to allocate \" << size <<\n> \" bytes: \"\n> > >> > + << strerror(ret);\n> > >> > + return {};\n> > >> > + }\n> > >> > +\n> > >> > + /* The underlying memfd is kept as as a reference in the\n> kernel */\n> > >> > + UniqueFD uDma(ret);\n> > >> > +\n> > >> > + if (create.size != size)\n> > >> > + LOG(HeapAllocator, Warning)\n> > >> > + << \"UdmaHeap allocated \" << create.size << \"\n> bytes instead of \"\n> > >> > + << size << \" bytes\";\n> > >> > +\n> > >> > + /* Fail if not suitable, the allocation will be free'd by\n> UniqueFD */\n> > >> > + if (create.size < size)\n> > >> > + return {};\n> > >> > +\n> > >> > + LOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" <<\n> create.size << \" bytes\";\n> > >> > +\n> > >> > + return uDma;\n> > >> > +}\n> > >> > +\n> > >> > HeapAllocator::HeapAllocator()\n> > >> > {\n> > >> > heap_ = std::make_unique();\n> > >> > + if (!isValid())\n> > >> > + heap_ = std::make_unique();\n> > >> > }\n> > >> >\n> > >> > HeapAllocator::~HeapAllocator() = default;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 20EA1C3237\n\tfor ;\n\tMon, 19 Jun 2023 05:55:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65E6E628C3;\n\tMon, 19 Jun 2023 07:55:25 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1524B61E3F\n\tfor ;\n\tMon, 19 Jun 2023 07:55:23 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2b475b54253so11374881fa.2\n\tfor ;\n\tSun, 18 Jun 2023 22:55:22 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687154125;\n\tbh=Y9eL+wbIo/1yfpfj4jeTqP6y2ODSg3pWTjFKvgQulE4=;\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=LDCazblFNTDDkBqodDIge3jjQJhNfwaTsHfnvK8hoIsVin5LW+75E74QNNlU6mfKJ\n\td4zkKTaPfXWKqt5cvPGi4PSPkXewXdsXp7tBKXXVLKt0vb0OuCMrrxgJQY/0ZLFMJR\n\trLqj/0mGhUk8CE7NMMQTsM7k/5McSuCs7cF81pYVLoNQeeWZraVc/hPvx1c1QFCArf\n\tQK/YrV2UV81iCWC/xLLAqotI2d0mCtNbhBhL6UZo1jaSS/QhSbVg0MjI79E4Kr4F0h\n\tbyn58qsTgwXSHI/jz/Gtq71CPH/k6XlYjNoRjMIhPTpDonPHHg854DUtaEdz6JqAnq\n\tUQr7WPHvWs+Ng==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1687154122; x=1689746122;\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=50kWQDnXeGsVBWX1u00JzHrnZn83eGVOh6eO5yVpTSE=;\n\tb=fq9SS2C7S9rtFbHiSztIqRCZda756OtvD5JlGPigECygvf53mAHO9I1m85BNHTzzji\n\t6oPxwt5qywwNCIteKJIuJeZWeGMzI7eSdgzUDVxANM4n7vwdBKzdLNHVYUeoOHW8Hfer\n\tdfP23+6kKzhQuq1kVTIQcfTm7isHbL8nVBTiM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"fq9SS2C7\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687154122; x=1689746122;\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=50kWQDnXeGsVBWX1u00JzHrnZn83eGVOh6eO5yVpTSE=;\n\tb=QXa9HRnbO4QbLphY3Ku3g7G9Uj7bgtZbb84IhcCpbHtfE7Yn5r84M+SrmS+Q1RKmsB\n\t0/L5CH+DacZ9L9k751m3WbpcO1iZZdHah5GXt2E2BvMQR3aZ07/jKHhNJyP2x6Dmp0Sm\n\tBOzOx7a4gwq+5vlq8gRjhpq17jTFaKtJ4HqhPFxdrTEOqNIQyk1OTvBPb9qvbJfRqRqD\n\tfJcBGBHkOWsKIZuhog9XLHbb+jt9eQ5HrEKLSq9yrEGr33S8bXi80y5fnDMpEyXxjHsB\n\tfjpC2GcaXA/PmrT/tHXocR1jAEVGEPJG2cbEPFrTwBp3g/rxbyUTXDlSBP2rDD473aqC\n\tu7ow==","X-Gm-Message-State":"AC+VfDwgFek4tTPFCx33KSY11vMbk+o9AGSWrZXnnrhp8Dg5hpJ/RWdX\n\tasOOy9li2L3VX1PlFpzfXwvZKvM2L8tP2DQ03sZSKezaZP1Ob+C3Kws=","X-Google-Smtp-Source":"ACHHUZ4tlReJDARmHb2h40H0LeKdxsIeSwTNslb6qqNiJJxtVeDSnG9SjLHeS3Gv2dr4NMzkVtJNOoBQdcMAEnSykD4=","X-Received":"by 2002:a2e:7819:0:b0:2b4:7fc5:21ab with SMTP id\n\tt25-20020a2e7819000000b002b47fc521abmr336182ljc.51.1687154121905;\n\tSun, 18 Jun 2023 22:55:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>\n\t<20230530064250.GB6865@pendragon.ideasonboard.com>\n\t\n\t\n\t<20230602083234.GC19463@pendragon.ideasonboard.com>","In-Reply-To":"<20230602083234.GC19463@pendragon.ideasonboard.com>","Date":"Mon, 19 Jun 2023 13:55:10 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000001e2b3905fe752e9b\"","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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\" "}},{"id":27651,"web_url":"https://patchwork.libcamera.org/comment/27651/","msgid":"","date":"2023-08-02T07:07:16","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Jacopo,\n\nOn Mon, May 29, 2023 at 3:58 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel\n> wrote:\n> > From: Cheng-Hao Yang \n> >\n> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> > 1 file changed, 100 insertions(+)\n> >\n> > diff --git a/src/libcamera/heap_allocator.cpp\n> b/src/libcamera/heap_allocator.cpp\n> > index 69f65062..8f99f590 100644\n> > --- a/src/libcamera/heap_allocator.cpp\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -11,10 +11,14 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > +#include \n> > +#include \n> > #include \n> >\n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -54,6 +58,14 @@ public:\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> > DmaHeap::DmaHeap()\n> > {\n> > for (const char *name : dmaHeapNames) {\n> > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> > continue;\n> > }\n> >\n> > + LOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> > handle_ = UniqueFD(ret);\n> > break;\n> > }\n> > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name,\n> std::size_t size)\n> > return allocFd;\n> > }\n> >\n> > +UdmaHeap::UdmaHeap()\n> > +{\n> > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n>\n> Should you add O_CLOEXEC or does it generate issues ? Same question as\n> in 1/3 on the third argument\n>\n>\nRemoved the third argument for now.\nKieran, I copied and pasted from your code [1]. Maybe you know better than I\ndo?\n\n[1]:\nhttps://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R29\n\n\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to open allocator: \" <<\n> strerror(ret);\n>\n> You could return here and save an...\n>\n> > + } else {\n>\n> ... indendation level here\n>\n>\nYeah thanks!\n\n\n> > + LOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> > + handle_ = UniqueFD(ret);\n> > + }\n> > +}\n> > +\n> > +UdmaHeap::~UdmaHeap() = default;\n> > +\n> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> > +{\n> > + if (!isValid()) {\n> > + LOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation\n> attempted without allocator\" << name;\n> > + return {};\n> > + }\n>\n> UdmaHeap::alloc() is called by HeapAllocator::alloc() which already\n> checks for validity with a Fatal error. You can drop this change\n>\n\nRight, removed.\n\n\n> > +\n> > + int ret;\n> > +\n> > + ret = memfd_create(name, MFD_ALLOW_SEALING);\n>\n> You can declare and assign ret in one line\n>\n>\nDone\n\n\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to allocate memfd storage: \"\n> > + << strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + UniqueFD memfd(ret);\n> > +\n> > + ret = ftruncate(memfd.get(), size);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to set memfd size: \" <<\n> strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to seal the memfd: \" <<\n> strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + struct udmabuf_create create;\n> > +\n> > + create.memfd = memfd.get();\n> > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> > + create.offset = 0;\n> > + create.size = size;\n> > +\n> > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(HeapAllocator, Error)\n> > + << \"UdmaHeap failed to allocate \" << size << \"\n> bytes: \"\n> > + << strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + /* The underlying memfd is kept as as a reference in the kernel */\n> > + UniqueFD uDma(ret);\n> > +\n> > + if (create.size != size)\n> > + LOG(HeapAllocator, Warning)\n> > + << \"UdmaHeap allocated \" << create.size << \" bytes\n> instead of \"\n> > + << size << \" bytes\";\n>\n> Should we continue or is it an error ? Also I would move this check\n> before the creation of uDma(ret);\n>\n>\nYeah creating |uDma| when we're returning should be better.\n\n\n> > +\n> > + /* Fail if not suitable, the allocation will be free'd by UniqueFD\n> */\n> > + if (create.size < size)\n> > + return {};\n>\n> Ah so larger size is ok, smaller it's not.\n>\n> So I would check for this before the previous one\n>\n>\nHmm, do you mean we should generate different logs for the two cases?\nUpdated the order and the logs. Please check.\n\n\n> > +\n> > + LOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size\n> << \" bytes\";\n> > +\n> > + return uDma;\n> > +}\n> > +\n> > HeapAllocator::HeapAllocator()\n> > {\n> > heap_ = std::make_unique();\n> > + if (!isValid())\n> > + heap_ = std::make_unique();\n> > }\n> >\n> > HeapAllocator::~HeapAllocator() = default;\n> > --\n> > 2.40.1.698.g37aff9b760-goog\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 57724BDB13\n\tfor ;\n\tWed, 2 Aug 2023 07:07:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C2CB627EC;\n\tWed, 2 Aug 2023 09:07:30 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97090627E6\n\tfor ;\n\tWed, 2 Aug 2023 09:07:28 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2b9cbaee7a9so84259621fa.0\n\tfor ;\n\tWed, 02 Aug 2023 00:07:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690960050;\n\tbh=WqiOnZH8AIjQpv4/8SqYKITu0UV8TrdB3nmUTd77rWk=;\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=O0jujDQbNahJ/on0sMKqtY2zZ4bGem0+iiwqnlrXV4jM7zygugwperNrAbRBzTwPp\n\t6+RsatQ0cjYwf2mGq4qv+o7jRepRc2JqGltmRsLm/r+4OGI0JPmqC7FnL321UoAs7O\n\t+OlEp4Rp4oThN83CgTzIwNbWNEON5LWIY4jAdf+5dlT4Q7knojjsdreUM1ttrWVLWy\n\tJLyWPRmFqagY20hl7TNYTsDtbuTrdRpFYvFKXddVQsohoA6f9NoCXiMhlAFYoe4W28\n\t/Gyf3z/d003/aAA8atM5T4mS0Rf4CsGgE3576wR24eMiT1quKz4e0kH4r3ixTLpHcP\n\toCq0Db/qLxIzw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1690960048; x=1691564848;\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=EXq7VAtzgoeMmw6IMEvruvQyF8xIUbqxX2vaZlW+DY4=;\n\tb=V2vAvoLRgu+/4FQp0+DEFuJQANqx5BhbfjTYGEn2YjOxEE1OgHIC8NDuIVl01ibszl\n\tKqiW7Lv7If+TX6SWtC0PIYOq6szJH8OAtzKj2HuQCsGfyaaki8E4X46EPmkp4ARPJ1az\n\ti1HiDNrKRQzHKOJSLCuQaa0NRpgv7eOxmif2A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"V2vAvoLR\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690960048; x=1691564848;\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=EXq7VAtzgoeMmw6IMEvruvQyF8xIUbqxX2vaZlW+DY4=;\n\tb=S507b/RSrQzzfIF7X8HLBLvMK+bnsSn3kUHZEJ9mkF4doZnGxe1hLG37bcbK8I/SZ7\n\tGrbsqqoXMQVfdNAH8XclkFKaTbvEOw7p9Y3wmwx8DfPnkYfDnAVObw5zSXNAqPu1wO6r\n\t3XlCPGQJs8N8HQGTpz2Hu9W0trkH99GuXRRET9GS37ZpT9fWjjViwFA8tkYuXDBzhOuQ\n\tVpwT7GpQ2V/NNnJrv5GORsmqcU7bmqzBRVYFk7yzMGMso1/MUFprdCzgNYCuPI598Uu+\n\tc+5Jez6dWoVMXy8OnX2hs1fBK+No2sn6imPoRrFh9gfTuQlJ2RcBE04jFNnOrsOQNIfF\n\t6tOw==","X-Gm-Message-State":"ABy/qLZa5Jq3qN9Is3Nrs18E7HYrD9EQMNnQkgzxUF68DvPMTQzvW7g0\n\tuSUg5rOPCd/bQZaX711u5tKXigFb25SaMa/z5+q3Pq1Tt+cg40ve","X-Google-Smtp-Source":"APBJJlEMJewUO4PTcGMvShiz7+cPt6K6nXLwT5eLlmQLmuqIwspiGnTXZHYRJb4SqpriqWDww+0d0vn3FIXw2+mAKXE=","X-Received":"by 2002:a2e:9c94:0:b0:2b9:dd3b:cf43 with SMTP id\n\tx20-20020a2e9c94000000b002b9dd3bcf43mr4138547lji.13.1690960047691;\n\tWed, 02 Aug 2023 00:07:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>\n\t<4j2ae6q6hsny3hyda4gpposehsfkldhxik6esttteefcyd2wud@vghoxwijzg6q>","In-Reply-To":"<4j2ae6q6hsny3hyda4gpposehsfkldhxik6esttteefcyd2wud@vghoxwijzg6q>","Date":"Wed, 2 Aug 2023 15:07:16 +0800","Message-ID":"","To":"Jacopo Mondi , \n\tKieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000f8eee10601eb5075\"","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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\" "}},{"id":27653,"web_url":"https://patchwork.libcamera.org/comment/27653/","msgid":"<20230802200124.GB29718@pendragon.ideasonboard.com>","date":"2023-08-02T20:01:24","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 02, 2023 at 03:07:16PM +0800, Cheng-Hao Yang via libcamera-devel wrote:\n> On Mon, May 29, 2023 at 3:58 PM Jacopo Mondi wrote:\n> > On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:\n> > > From: Cheng-Hao Yang \n> > >\n> > > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n> > >\n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> > > 1 file changed, 100 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> > > index 69f65062..8f99f590 100644\n> > > --- a/src/libcamera/heap_allocator.cpp\n> > > +++ b/src/libcamera/heap_allocator.cpp\n> > > @@ -11,10 +11,14 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -54,6 +58,14 @@ public:\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> > > DmaHeap::DmaHeap()\n> > > {\n> > > for (const char *name : dmaHeapNames) {\n> > > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> > > continue;\n> > > }\n> > >\n> > > + LOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> > > handle_ = UniqueFD(ret);\n> > > break;\n> > > }\n> > > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name,\n> > std::size_t size)\n> > > return allocFd;\n> > > }\n> > >\n> > > +UdmaHeap::UdmaHeap()\n> > > +{\n> > > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> >\n> > Should you add O_CLOEXEC or does it generate issues ? Same question as\n> > in 1/3 on the third argument\n>\n> Removed the third argument for now.\n> Kieran, I copied and pasted from your code [1]. Maybe you know better than I\n> do?\n> \n> [1]: https://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R29\n\nKieran's code predates the commit that added O_CLOEXEC to\nsrc/libcamera/pipeline/raspberrypi/dma_heaps.cpp. It should be used\nhere too.\n\n> > > + if (ret < 0) {\n> > > + ret = errno;\n> > > + LOG(HeapAllocator, Error)\n> > > + << \"UdmaHeap failed to open allocator: \" << strerror(ret);\n> >\n> > You could return here and save an...\n> >\n> > > + } else {\n> >\n> > ... indendation level here\n>\n> Yeah thanks!\n> \n> > > + LOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> > > + handle_ = UniqueFD(ret);\n> > > + }\n> > > +}\n> > > +\n> > > +UdmaHeap::~UdmaHeap() = default;\n> > > +\n> > > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> > > +{\n> > > + if (!isValid()) {\n> > > + LOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation attempted without allocator\" << name;\n> > > + return {};\n> > > + }\n> >\n> > UdmaHeap::alloc() is called by HeapAllocator::alloc() which already\n> > checks for validity with a Fatal error. You can drop this change\n> \n> Right, removed.\n> \n> > > +\n> > > + int ret;\n> > > +\n> > > + ret = memfd_create(name, MFD_ALLOW_SEALING);\n> >\n> > You can declare and assign ret in one line\n>\n> Done\n> \n> > > + if (ret < 0) {\n> > > + ret = errno;\n> > > + LOG(HeapAllocator, Error)\n> > > + << \"UdmaHeap failed to allocate memfd storage: \"\n> > > + << strerror(ret);\n> > > + return {};\n> > > + }\n> > > +\n> > > + UniqueFD memfd(ret);\n> > > +\n> > > + ret = ftruncate(memfd.get(), size);\n> > > + if (ret < 0) {\n> > > + ret = errno;\n> > > + LOG(HeapAllocator, Error)\n> > > + << \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> > > + return {};\n> > > + }\n> > > +\n> > > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> > > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> > > + if (ret < 0) {\n> > > + ret = errno;\n> > > + LOG(HeapAllocator, Error)\n> > > + << \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> > > + return {};\n> > > + }\n> > > +\n> > > + struct udmabuf_create create;\n> > > +\n> > > + create.memfd = memfd.get();\n> > > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> > > + create.offset = 0;\n> > > + create.size = size;\n> > > +\n> > > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> > > + if (ret < 0) {\n> > > + ret = errno;\n> > > + LOG(HeapAllocator, Error)\n> > > + << \"UdmaHeap failed to allocate \" << size << \"bytes: \"\n> > > + << strerror(ret);\n> > > + return {};\n> > > + }\n> > > +\n> > > + /* The underlying memfd is kept as as a reference in the kernel */\n> > > + UniqueFD uDma(ret);\n> > > +\n> > > + if (create.size != size)\n> > > + LOG(HeapAllocator, Warning)\n> > > + << \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> > > + << size << \" bytes\";\n> >\n> > Should we continue or is it an error ? Also I would move this check\n> > before the creation of uDma(ret);\n>\n> Yeah creating |uDma| when we're returning should be better.\n> \n> > > +\n> > > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> > > + if (create.size < size)\n> > > + return {};\n> >\n> > Ah so larger size is ok, smaller it's not.\n> >\n> > So I would check for this before the previous one\n>\n> Hmm, do you mean we should generate different logs for the two cases?\n> Updated the order and the logs. Please check.\n> \n> > > +\n> > > + LOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> > > +\n> > > + return uDma;\n> > > +}\n> > > +\n> > > HeapAllocator::HeapAllocator()\n> > > {\n> > > heap_ = std::make_unique();\n> > > + if (!isValid())\n> > > + heap_ = std::make_unique();\n> > > }\n> > >\n> > > HeapAllocator::~HeapAllocator() = default;","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 F1541BDB13\n\tfor ;\n\tWed, 2 Aug 2023 20:01:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 299A1627EC;\n\tWed, 2 Aug 2023 22:01:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94570627E7\n\tfor ;\n\tWed, 2 Aug 2023 22:01:19 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 572D6137C;\n\tWed, 2 Aug 2023 22:00:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1691006481;\n\tbh=LBCdzKVB4Cdc62412Cl8b6jpvpMoS9ZfkpYjEEGImM8=;\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=deWcFqXE3qRV3AQHj5el2yWbqvfY49KTrJYzA+tiuTxMLsTv9aVzgbHgxYrC5j5B2\n\txaKAK3/LXc4+LuSV9q1FInuMavaRuGEwstpqOZisYDTf/f71c5HUyqSrSRaV4PkskL\n\tzpki9rCCLYV/yVwXBSA4K1yeV2VPuA6LlgxK08qc+1qkJaQ51lu3mWorVB+H5xTkJv\n\tl8+bq3mvNbIG7huCYNJov81Xz/1ml7iZ/M5HPxogxkM8zmc4i4dIkWAjahrwS05y0r\n\t6jKq2MV49qs005yLsW8RbDV+35/jrNudCAf1L8XA87DpqVxRXjG3Ywc9/KhSjxzi/C\n\tCHoo3TfEgR8Pg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1691006415;\n\tbh=LBCdzKVB4Cdc62412Cl8b6jpvpMoS9ZfkpYjEEGImM8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R9PZLNU8Ty+r5JUL8fij9CrGepV1W1V4pmU3G11fYeOWx1Wwx/XL+mhwVZYhCHyQP\n\tHvOxttD/owMC7BDwU5hocV2ya9qO2aWdlzdlcIHbWGkYmOeaLEVHKjCsdGTy6j/KE0\n\t2Dku8zbyBhQRgymVdJr7xrmOfyf2J5M7xLNWlRD8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"R9PZLNU8\"; dkim-atps=neutral","Date":"Wed, 2 Aug 2023 23:01:24 +0300","To":"Cheng-Hao Yang ","Message-ID":"<20230802200124.GB29718@pendragon.ideasonboard.com>","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>\n\t<4j2ae6q6hsny3hyda4gpposehsfkldhxik6esttteefcyd2wud@vghoxwijzg6q>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27656,"web_url":"https://patchwork.libcamera.org/comment/27656/","msgid":"<20230802203201.GD29718@pendragon.ideasonboard.com>","date":"2023-08-02T20:32:01","subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\n(CC'ing Javier)\n\nOn Mon, Jun 19, 2023 at 01:55:10PM +0800, Cheng-Hao Yang wrote:\n> Hi Laurent,\n> \n> As discussed in the libcamera weekly meeting before, have you brought out\n> the discussion in irc.oftc.net with people from Debian and Fedora about\n> supporting udmabuf (or DMA buf) in some common linux distributions? I'd\n> like to know the updates if any.\n\nAccording to the discussions I've had with Javier in the #libcamera IRC\nchannel, Fedora ships both udmabuf and DMA heaps. The device node for\nthe former is owned by root:kvm, while the latter are owned by\nroot:root.\n\nI believe Debian doesn't ship udmabuf or DMA heaps, according to the\nkernel config ([1], I have checked amd64 and arm64). I'm a bit surprised\nby the lack of udmabuf support, as it gets used by qemu, so I may be\nmissing something. I haven't contacted the Debian kernel team about\nthis, but found a corresponding bug that went unanswered ([2].\n\nThe ownership for /dev/udmabuf is controlled by the systemd-udev default\nrules, see [3]. This was authored four years ago ([4]), to fix an issue\n([5]). How to make the device accessible to libcamera is an open\nquestion. One option would be to create a new dmabuf group, and add\nlocal users to it automatically based on system policies. The user under\nwhich the pipewire daemon gets run would be added to that group, should\nit be different that the account for logged users (as in pipewire\nrunning as a system deamon instead of a user daemon). Someone should\nengage with the relevant communities to discuss this. Would you\nvolunteer ? :-)\n\nWhile at it, we should also discuss permissions on DMA heaps, I think\nthat would be a better path forward.\n\n[1] https://packages.debian.org/bookworm/linux-config-6.1\n[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=994449\n[3] https://github.com/systemd/systemd/blob/main/rules.d/50-udev-default.rules.in#L116\n[4] https://github.com/systemd/systemd/commit/0fde5a3591d7bc7f689937aa86bd8b30e40d9bf8\n[5] https://github.com/systemd/systemd/issues/12283\n\n> On Fri, Jun 2, 2023 at 4:32 PM Laurent Pinchart wrote:\n> > On Fri, Jun 02, 2023 at 02:39:33PM +0800, Cheng-Hao Yang wrote:\n> > > Also, if we let users decide which heap implementations to be used\n> > > directly, maybe there's no point having class HeapAllocator at all.\n> > > We can just give users (pipeline handlers) access to each Heap's\n> > > implementation directly.\n> > >\n> > > WDYT?\n> >\n> > The first question to answer is what use case(s) you're trying to\n> > address. Let's assume we would give users of the allocator class a\n> > choice regarding what heap they allocate from (system vs. cma, those are\n> > the only two options available in mainline). How would the user decide\n> > which heap to use ? You can use the virtual pipeline handler as one use\n> > case to answer that question (try to keep the big picture in mind, what,\n> > for instance, if the virtual pipeline handler were to use a hardware\n> > device - such as the GPU - to post-process images generated by the CPU,\n> > or what if we wanted to achieve zero-copy when display images generated\n> > by the virtual pipeline handler ?), and considering other use cases\n> > would be helpful to design a good solution.\n> >\n> > > On Tue, May 30, 2023 at 6:14 PM Cheng-Hao Yang wrote:\n> > >\n> > > > Hi Laurent, thanks for pointing it out. I'm a newbie to buffer allocation\n> > > > in linux, so here are my confusions before updating the patch:\n> > > >\n> > > > On Tue, May 30, 2023 at 2:42 PM Laurent Pinchart wrote:\n> > > >> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:\n> > > >> > From: Cheng-Hao Yang \n> > > >> >\n> > > >> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.\n> > > >>\n> > > >> udmabuf and the DMA heaps are two completely different things, meant for\n> > > >> different use cases. An allocator that would use \"whatever is available\"\n> > > >> isn't a good generic helper.\n> > > >\n> > > > I assume you mean udmabuf and DMA buffers, unless you mean the\n> > > > difference between buffers and heaps:\n> > > > I saw [1], which states udmabuf as \"User space mappable DMA buffer\",\n> > > > so I assume that it's one kind of DMA buffer, especially accessible for\n> > > > userspace.\n> >\n> > That's a different udmabuf than the one present in the mainline kernel\n> > as far as I can tell. It seems to be an out-of-tree implementation, from\n> > https://github.com/ikwzm/udmabuf.\n> >\n> > The mainline udmabuf is a kernel API that allows wrapping a memfd into a\n> > dmabuf object. Userspace gets a dmabuf FD, usable with all kernel APIs\n> > that expect a dmabuf FD. memfd creates an anonymous file, backed by\n> > anonymous memory, so you will essentially get discontiguous pages. If\n> > I'm not mistaken, those pages are pinned to memory by calling\n> > shmem_read_mapping_page() when the udmabuf is created, but don't quote\n> > me on that.\n> >\n> > DMA heaps, on the other hand, are a set of memory allocators aimed to\n> > provide memory regions suitable for devices to use for DMA. The\n> > allocators also expose the buffers as dmabuf FDs to userspace, and\n> > that's the only thing they have in common with udmabuf. With DMA heaps,\n> > userspace picks one of the allocators provided by the system, and asks\n> > the kernel to allocate buffers using that allocator.\n> >\n> > The \"system\" heap uses alloc_pages() and tries to split the allocation\n> > in the smallest number of page sets, with each of the sets using the\n> > highest possible page order. From a device DMA point of view, this is\n> > similar to udmabuf, neither are suitable for usage with devices that\n> > require DMA-contiguous memory and don't have an IOMMU.\n> >\n> > The \"cma\" heap uses cma_alloc(), which guarantees physically-contiguous\n> > memory. The memory is suitable for DMA with many (but not all) devices.\n> > The downside is that the CMA area is limited in size, and physically\n> > contiguous memory shouldn't be used when the device doesn't require it.\n> >\n> > There is unfortunately no DMA heap that allocates DMA-capable buffers\n> > for a specific device. Lots of work is still needed to provide Linux\n> > systems with a unified device memory allocator.\n> >\n> > Based on the above, the issue isn't so much about DMA heaps vs. udmabuf\n> > as I initially implied, but more about system heap and udmabuf vs. CMA\n> > heap. I think the system heap and udmabuf are likely to provide similar\n> > types of allocations from the point of view of DMA. The page allocation\n> > order, however, may be significantly different. I haven't investigated\n> > that.\n> >\n> > > > I agree that we shouldn't use \"whatever is available\" logic though. How\n> > > > about an enum passed into HeapAllocator's c'tor that lets the user to\n> > > > specify which heap(s) (with an order or not) to be used?\n> > > >\n> > > > [1]: https://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md\n> > > >\n> > > >> I expect writing the documentation will make you realize that there's a\n> > > >> design issue here, as it's difficult to document clearly an API that\n> > > >> doesn't have a clear design.\n> > > >>\n> > > >> > Signed-off-by: Harvey Yang \n> > > >> > ---\n> > > >> > src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++\n> > > >> > 1 file changed, 100 insertions(+)\n> > > >> >\n> > > >> > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> > > >> > index 69f65062..8f99f590 100644\n> > > >> > --- a/src/libcamera/heap_allocator.cpp\n> > > >> > +++ b/src/libcamera/heap_allocator.cpp\n> > > >> > @@ -11,10 +11,14 @@\n> > > >> > #include \n> > > >> > #include \n> > > >> > #include \n> > > >> > +#include \n> > > >> > +#include \n> > > >> > +#include \n> > > >> > #include \n> > > >> >\n> > > >> > #include \n> > > >> > #include \n> > > >> > +#include \n> > > >> >\n> > > >> > #include \n> > > >> >\n> > > >> > @@ -54,6 +58,14 @@ public:\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> > > >> > DmaHeap::DmaHeap()\n> > > >> > {\n> > > >> > for (const char *name : dmaHeapNames) {\n> > > >> > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()\n> > > >> > continue;\n> > > >> > }\n> > > >> >\n> > > >> > + LOG(HeapAllocator, Info) << \"Using DmaHeap allocator\";\n> > > >> > handle_ = UniqueFD(ret);\n> > > >> > break;\n> > > >> > }\n> > > >> > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > > >> > return allocFd;\n> > > >> > }\n> > > >> >\n> > > >> > +UdmaHeap::UdmaHeap()\n> > > >> > +{\n> > > >> > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> > > >> > + if (ret < 0) {\n> > > >> > + ret = errno;\n> > > >> > + LOG(HeapAllocator, Error)\n> > > >> > + << \"UdmaHeap failed to open allocator: \" << strerror(ret);\n> > > >> > + } else {\n> > > >> > + LOG(HeapAllocator, Info) << \"Using UdmaHeap allocator\";\n> > > >> > + handle_ = UniqueFD(ret);\n> > > >> > + }\n> > > >> > +}\n> > > >> > +\n> > > >> > +UdmaHeap::~UdmaHeap() = default;\n> > > >> > +\n> > > >> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)\n> > > >> > +{\n> > > >> > + if (!isValid()) {\n> > > >> > + LOG(HeapAllocator, Fatal) << \"UdmaHeap: Allocation attempted without allocator\" << name;\n> > > >> > + return {};\n> > > >> > + }\n> > > >> > +\n> > > >> > + int ret;\n> > > >> > +\n> > > >> > + ret = memfd_create(name, MFD_ALLOW_SEALING);\n> > > >> > + if (ret < 0) {\n> > > >> > + ret = errno;\n> > > >> > + LOG(HeapAllocator, Error)\n> > > >> > + << \"UdmaHeap failed to allocate memfd storage: \"\n> > > >> > + << strerror(ret);\n> > > >> > + return {};\n> > > >> > + }\n> > > >> > +\n> > > >> > + UniqueFD memfd(ret);\n> > > >> > +\n> > > >> > + ret = ftruncate(memfd.get(), size);\n> > > >> > + if (ret < 0) {\n> > > >> > + ret = errno;\n> > > >> > + LOG(HeapAllocator, Error)\n> > > >> > + << \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> > > >> > + return {};\n> > > >> > + }\n> > > >> > +\n> > > >> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> > > >> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> > > >> > + if (ret < 0) {\n> > > >> > + ret = errno;\n> > > >> > + LOG(HeapAllocator, Error)\n> > > >> > + << \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> > > >> > + return {};\n> > > >> > + }\n> > > >> > +\n> > > >> > + struct udmabuf_create create;\n> > > >> > +\n> > > >> > + create.memfd = memfd.get();\n> > > >> > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> > > >> > + create.offset = 0;\n> > > >> > + create.size = size;\n> > > >> > +\n> > > >> > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);\n> > > >> > + if (ret < 0) {\n> > > >> > + ret = errno;\n> > > >> > + LOG(HeapAllocator, Error)\n> > > >> > + << \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n> > > >> > + << strerror(ret);\n> > > >> > + return {};\n> > > >> > + }\n> > > >> > +\n> > > >> > + /* The underlying memfd is kept as as a reference in the kernel */\n> > > >> > + UniqueFD uDma(ret);\n> > > >> > +\n> > > >> > + if (create.size != size)\n> > > >> > + LOG(HeapAllocator, Warning)\n> > > >> > + << \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> > > >> > + << size << \" bytes\";\n> > > >> > +\n> > > >> > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> > > >> > + if (create.size < size)\n> > > >> > + return {};\n> > > >> > +\n> > > >> > + LOG(HeapAllocator, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> > > >> > +\n> > > >> > + return uDma;\n> > > >> > +}\n> > > >> > +\n> > > >> > HeapAllocator::HeapAllocator()\n> > > >> > {\n> > > >> > heap_ = std::make_unique();\n> > > >> > + if (!isValid())\n> > > >> > + heap_ = std::make_unique();\n> > > >> > }\n> > > >> >\n> > > >> > HeapAllocator::~HeapAllocator() = default;","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 937BBBDB13\n\tfor ;\n\tWed, 2 Aug 2023 20:31:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0901627E7;\n\tWed, 2 Aug 2023 22:31:58 +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 1953461E1C\n\tfor ;\n\tWed, 2 Aug 2023 22:31:57 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EA7AE9CA;\n\tWed, 2 Aug 2023 22:30:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1691008319;\n\tbh=bAV+P1f/ynMLRKS8IF489ez3IBrYeQMSQDsw1/Y/518=;\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=MpbqSAJI3sURlEncv8wfXJ8su07wIb6a7+bsDqmOqwOmSekHhQomIINPjfIPDV+vp\n\tPcPu40O+dGhIf64dLH7d48nVWvVGVOfeGxTaGxj+dUEFvKT3Hw7adc+dFndQfKsrMu\n\ts3Y4RVOxU2LobhH/RZ9I4DH8WPiTfes3i2RfLKKTiwjHSDeBK0RgKTgWqum1OlA30S\n\tmN04QdC7NiKpZ3tjsISyjVubEDS1q8ktyvMo0Jw7E601k6AU2pUxB+YwAFPv1/alJD\n\tkFNrs6NqISyQaFTjW+R1l3gqhYaIGAE6pyYmWV+WSyLmytc84XUyj2Rydf6Uk8NT36\n\t9B1tS7ityrJ2A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1691008253;\n\tbh=bAV+P1f/ynMLRKS8IF489ez3IBrYeQMSQDsw1/Y/518=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HiA6n/UP9haOf9AkUU7QViKzh8+NZhEC21FutqLqJaTb7O8DsfwYu/TsvZtkaMPpB\n\ty2sYA3JB4bmeQI6nof13skPa5khyygwCW3SREF/qd5WnQ2fY0g3E7iet/NbR+JtAaF\n\t5sj11bh0FNG9gySCwKP8v5XEy7lBjYSQl2MPRcyA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HiA6n/UP\"; dkim-atps=neutral","Date":"Wed, 2 Aug 2023 23:32:01 +0300","To":"Cheng-Hao Yang ","Message-ID":"<20230802203201.GD29718@pendragon.ideasonboard.com>","References":"<20230522083546.2465448-1-chenghaoyang@google.com>\n\t<20230522083546.2465448-3-chenghaoyang@google.com>\n\t<20230530064250.GB6865@pendragon.ideasonboard.com>\n\t\n\t\n\t<20230602083234.GC19463@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if\n\tDmaHeap is not valid","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]