[{"id":28474,"web_url":"https://patchwork.libcamera.org/comment/28474/","msgid":"","date":"2024-01-15T09:07:43","subject":"Re: [libcamera-devel] [PATCH v2 03/18] libcamera: dma_heaps: extend\n\tDmaHeap class to support system heap","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nOn Sat, 13 Jan 2024 at 14:22, Hans de Goede via libcamera-devel\n wrote:\n>\n> From: Andrey Konovalov \n>\n> Add an argument to the constructor to specify dma heaps type(s)\n> to use. Can be DmaHeapFlag::Cma and/or DmaHeapFlag::System.\n> By default DmaHeapFlag::Cma is used. If both DmaHeapFlag::Cma and\n> DmaHeapFlag::System are set, CMA heap is tried first.\n\nJust curious, what is the use case for trying an allocation in the CMA\nfirst, then system help? On the Raspberry Pi platforma at least, if\nwe are allocating through CMA, it's because of the hardware\nrequirements, and system heap allocation will not work at all.\n\nThanks,\nNaush\n\n>\n> Signed-off-by: Andrey Konovalov \n> Signed-off-by: Hans de Goede \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> ---\n> include/libcamera/internal/dma_heaps.h | 12 +++++++-\n> src/libcamera/dma_heaps.cpp | 39 +++++++++++++++-----------\n> 2 files changed, 34 insertions(+), 17 deletions(-)\n>\n> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n> index cff8f140..22aa1007 100644\n> --- a/include/libcamera/internal/dma_heaps.h\n> +++ b/include/libcamera/internal/dma_heaps.h\n> @@ -9,6 +9,7 @@\n>\n> #include \n>\n> +#include \n> #include \n>\n> namespace libcamera {\n> @@ -16,7 +17,14 @@ namespace libcamera {\n> class DmaHeap\n> {\n> public:\n> - DmaHeap();\n> + enum class DmaHeapFlag {\n> + Cma = (1 << 0),\n> + System = (1 << 1),\n> + };\n> +\n> + using DmaHeapFlags = Flags;\n> +\n> + DmaHeap(DmaHeapFlags flags = DmaHeapFlag::Cma);\n> ~DmaHeap();\n> bool isValid() const { return dmaHeapHandle_.isValid(); }\n> UniqueFD alloc(const char *name, std::size_t size);\n> @@ -25,4 +33,6 @@ private:\n> UniqueFD dmaHeapHandle_;\n> };\n>\n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> index 7444d9c2..177de31b 100644\n> --- a/src/libcamera/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heaps.cpp\n> @@ -16,6 +16,8 @@\n>\n> #include \"libcamera/internal/dma_heaps.h\"\n>\n> +namespace libcamera {\n> +\n> /*\n> * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma\n> * to only have to worry about importing.\n> @@ -23,28 +25,33 @@\n> * Annoyingly, should the cma heap size be specified on the kernel command line\n> * instead of DT, the heap gets named \"reserved\" instead.\n> */\n> -static constexpr std::array heapNames = {\n> - \"/dev/dma_heap/linux,cma\",\n> - \"/dev/dma_heap/reserved\"\n> +static constexpr std::array, 3> heapNames = {\n> + /* CMA heap names first */\n> + std::make_pair(DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\"),\n> + std::make_pair(DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\"),\n> + std::make_pair(DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\")\n> };\n>\n> -namespace libcamera {\n> -\n> LOG_DEFINE_CATEGORY(DmaHeap)\n>\n> -DmaHeap::DmaHeap()\n> +DmaHeap::DmaHeap(DmaHeapFlags flags)\n> {\n> - for (const char *name : heapNames) {\n> - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> - if (ret < 0) {\n> - ret = errno;\n> - LOG(DmaHeap, Debug) << \"Failed to open \" << name << \": \"\n> - << strerror(ret);\n> - continue;\n> - }\n> + int ret;\n>\n> - dmaHeapHandle_ = UniqueFD(ret);\n> - break;\n> + for (const auto &name : heapNames) {\n> + if (flags & name.first) {\n> + ret = ::open(name.second, O_RDWR | O_CLOEXEC, 0);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(DmaHeap, Debug) << \"Failed to open \" << name.second << \": \"\n> + << strerror(ret);\n> + continue;\n> + }\n> +\n> + LOG(DmaHeap, Debug) << \"Using \" << name.second;\n> + dmaHeapHandle_ = UniqueFD(ret);\n> + break;\n> + }\n> }\n>\n> if (!dmaHeapHandle_.isValid())\n> --\n> 2.43.0\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 8E58CC323E\n\tfor ;\n\tMon, 15 Jan 2024 09:08:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D33FE628B8;\n\tMon, 15 Jan 2024 10:08:20 +0100 (CET)","from mail-yw1-x112a.google.com (mail-yw1-x112a.google.com\n\t[IPv6:2607:f8b0:4864:20::112a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66363628AE\n\tfor ;\n\tMon, 15 Jan 2024 10:08:19 +0100 (CET)","by mail-yw1-x112a.google.com with SMTP id\n\t00721157ae682-5f68af028afso70682467b3.2\n\tfor ;\n\tMon, 15 Jan 2024 01:08:19 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705309700;\n\tbh=Uu4QQsMWr929aDD8AN94QH/TD2NhTQVZh094o0S79XU=;\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=IzM1rt3LlE/B3L88TEfuRF0E1aLvfEmlXqS0fkHqcsNc/6ecvWe0tqe7ZOm8i/Qbq\n\tgBRf8TM9XMnmv1YaKH52fhWu9nxssIgoCqreMrlrSkjk/nu4mq5OzfVf4Nop19QUIV\n\tgaejCfcQX0ttXpQNXJc/eXpC43XEsXR8ByWh1bOOVYlKmBCQRZw8B9qaXi5R90cqd8\n\tTehizCZlFLIOaTk0LnArdlLIfMIJtgOjaay6xZPt9axUeuf36sQZysJ5IpedPMXGZE\n\trjO+UBMSe0FLxknnIAUaCudIRqmfRwBALfhCkz7FRPWZtq88qv7Y9ghtI/83Tp5hJj\n\ta4fv9wKZrjh4Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1705309698; x=1705914498;\n\tdarn=lists.libcamera.org; \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=nHWkv7ltKVSOdzdJ3Tc5rI3U0pqtFyXstVrfsVggkiU=;\n\tb=plzM5yXUZPEdyyNVRqURxLX0LeZMYUDeBytYFxgmp3OOlfN38fhJ/h+l2xgLAajFly\n\t4jnShJegaB4wBI6TBbSUDRNQAZ2Ffox/znzZTz59BLhTGFxDQq5y/Ph3bZonQ0ltICNj\n\tqwnGehXdSS0+ijLaxqvWNbGhCuDoiD0zEEND3fksoWQRBvo7hozOr4gvw7+6zoVfs80+\n\tMsEFHeA9vuKYu3w01tOfw0m2c1apGqfO6JN0exXib3o7QYNCC9m9ZnxOjxmmm1JTXYTX\n\tGcVvXAwdt49xotzSLoJe7ysV42EzVqR2vfDIHWQM5/2bRCAXLHi2UT+5GfOhRzfN4dBk\n\tWcwg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"plzM5yXU\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1705309698; x=1705914498;\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=nHWkv7ltKVSOdzdJ3Tc5rI3U0pqtFyXstVrfsVggkiU=;\n\tb=pv6bgOy2JVWgeeI9PMTtGtYsZ7njTLHH+3y7M9vM4dtuPT8pDZncDfA8onPI7YaZXj\n\tckoof6Rk3ldRFZgsW73jRaDuu9mV11HosGqeILGByVoBOSvYNVT0tPGzE7gSasPDHJHF\n\tQZWU/pPay/fji1fsR/eonk4TuSBl12GdaIUi0Yi+D1sOXAClBwtz9Ow7q9I21KY6GRfL\n\tcQ3b3yKowC6ZC/m4a0YJriSy80qY+hXJ9LnQxkAMq1q0Nc67JVAJ8mGHCLrVAem4459B\n\t6tajvOdDXlLMGcddG3vJB8onXfNawJ5tCGrNVa2QE6gNKPMBMCDyhGi6n45W173aRYde\n\tHT5Q==","X-Gm-Message-State":"AOJu0YzO/eStItkuXOBjz757VwpubUblQ/l2bZDtxLgU3jzTekGqqOCF\n\thDe1Ex5os4ArjWz5n6Q/ycYH/C8rozlfwSKy9+xxWjcdiWwLfw==","X-Google-Smtp-Source":"AGHT+IFXdz8WegZ8Qnysq55QcRa2mjIsV9faadv0HNsZJ9xwOZSQys20BL0R3EvW3/W+MDD1ShwOt2R2ZK2ewGty56w=","X-Received":"by 2002:a81:b80c:0:b0:5fc:d074:73d2 with SMTP id\n\tv12-20020a81b80c000000b005fcd07473d2mr722474ywe.58.1705309698246;\n\tMon, 15 Jan 2024 01:08:18 -0800 (PST)","MIME-Version":"1.0","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-4-hdegoede@redhat.com>","In-Reply-To":"<20240113142218.28063-4-hdegoede@redhat.com>","Date":"Mon, 15 Jan 2024 09:07:43 +0000","Message-ID":"","To":"Hans de Goede ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 03/18] libcamera: dma_heaps: extend\n\tDmaHeap class to support system heap","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28476,"web_url":"https://patchwork.libcamera.org/comment/28476/","msgid":"<38fde066-9cdd-42c5-baa9-5645ca5bff20@gmail.com>","date":"2024-01-15T12:24:22","subject":"Re: [libcamera-devel] [PATCH v2 03/18] libcamera: dma_heaps: extend\n\tDmaHeap class to support system heap","submitter":{"id":179,"url":"https://patchwork.libcamera.org/api/people/179/","name":"Andrei Konovalov","email":"andrey.konovalov.ynk@gmail.com"},"content":"Hi Naush,\n\nOn 15.01.2024 12:07, Naushir Patuck wrote:\n> Hi,\n> \n> On Sat, 13 Jan 2024 at 14:22, Hans de Goede via libcamera-devel\n> wrote:\n>>\n>> From: Andrey Konovalov \n>>\n>> Add an argument to the constructor to specify dma heaps type(s)\n>> to use. Can be DmaHeapFlag::Cma and/or DmaHeapFlag::System.\n>> By default DmaHeapFlag::Cma is used. If both DmaHeapFlag::Cma and\n>> DmaHeapFlag::System are set, CMA heap is tried first.\n> \n> Just curious, what is the use case for trying an allocation in the CMA\n> first, then system help? On the Raspberry Pi platforma at least, if\n> we are allocating through CMA, it's because of the hardware\n> requirements, and system heap allocation will not work at all.\n\nThe system dma-heap is used on the x86_64 targets where this is the only\ndma-heap present by default.\n\nOn the Qualcomm platforms (sc8280xp, sm8250) both the CMA and the system\nheaps work - at least in the current configuration without any fancy devices\nlike e.g hardware encoders behind the software ISP.\n\nThanks,\nAndrei\n\n> Thanks,\n> Naush\n> \n>>\n>> Signed-off-by: Andrey Konovalov \n>> Signed-off-by: Hans de Goede \n>> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n>> Tested-by: Pavel Machek \n>> ---\n>> include/libcamera/internal/dma_heaps.h | 12 +++++++-\n>> src/libcamera/dma_heaps.cpp | 39 +++++++++++++++-----------\n>> 2 files changed, 34 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n>> index cff8f140..22aa1007 100644\n>> --- a/include/libcamera/internal/dma_heaps.h\n>> +++ b/include/libcamera/internal/dma_heaps.h\n>> @@ -9,6 +9,7 @@\n>>\n>> #include \n>>\n>> +#include \n>> #include \n>>\n>> namespace libcamera {\n>> @@ -16,7 +17,14 @@ namespace libcamera {\n>> class DmaHeap\n>> {\n>> public:\n>> - DmaHeap();\n>> + enum class DmaHeapFlag {\n>> + Cma = (1 << 0),\n>> + System = (1 << 1),\n>> + };\n>> +\n>> + using DmaHeapFlags = Flags;\n>> +\n>> + DmaHeap(DmaHeapFlags flags = DmaHeapFlag::Cma);\n>> ~DmaHeap();\n>> bool isValid() const { return dmaHeapHandle_.isValid(); }\n>> UniqueFD alloc(const char *name, std::size_t size);\n>> @@ -25,4 +33,6 @@ private:\n>> UniqueFD dmaHeapHandle_;\n>> };\n>>\n>> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n>> +\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n>> index 7444d9c2..177de31b 100644\n>> --- a/src/libcamera/dma_heaps.cpp\n>> +++ b/src/libcamera/dma_heaps.cpp\n>> @@ -16,6 +16,8 @@\n>>\n>> #include \"libcamera/internal/dma_heaps.h\"\n>>\n>> +namespace libcamera {\n>> +\n>> /*\n>> * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma\n>> * to only have to worry about importing.\n>> @@ -23,28 +25,33 @@\n>> * Annoyingly, should the cma heap size be specified on the kernel command line\n>> * instead of DT, the heap gets named \"reserved\" instead.\n>> */\n>> -static constexpr std::array heapNames = {\n>> - \"/dev/dma_heap/linux,cma\",\n>> - \"/dev/dma_heap/reserved\"\n>> +static constexpr std::array, 3> heapNames = {\n>> + /* CMA heap names first */\n>> + std::make_pair(DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\"),\n>> + std::make_pair(DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\"),\n>> + std::make_pair(DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\")\n>> };\n>>\n>> -namespace libcamera {\n>> -\n>> LOG_DEFINE_CATEGORY(DmaHeap)\n>>\n>> -DmaHeap::DmaHeap()\n>> +DmaHeap::DmaHeap(DmaHeapFlags flags)\n>> {\n>> - for (const char *name : heapNames) {\n>> - int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n>> - if (ret < 0) {\n>> - ret = errno;\n>> - LOG(DmaHeap, Debug) << \"Failed to open \" << name << \": \"\n>> - << strerror(ret);\n>> - continue;\n>> - }\n>> + int ret;\n>>\n>> - dmaHeapHandle_ = UniqueFD(ret);\n>> - break;\n>> + for (const auto &name : heapNames) {\n>> + if (flags & name.first) {\n>> + ret = ::open(name.second, O_RDWR | O_CLOEXEC, 0);\n>> + if (ret < 0) {\n>> + ret = errno;\n>> + LOG(DmaHeap, Debug) << \"Failed to open \" << name.second << \": \"\n>> + << strerror(ret);\n>> + continue;\n>> + }\n>> +\n>> + LOG(DmaHeap, Debug) << \"Using \" << name.second;\n>> + dmaHeapHandle_ = UniqueFD(ret);\n>> + break;\n>> + }\n>> }\n>>\n>> if (!dmaHeapHandle_.isValid())\n>> --\n>> 2.43.0\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 8E547C323E\n\tfor ;\n\tMon, 15 Jan 2024 12:24:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96813628B8;\n\tMon, 15 Jan 2024 13:24:26 +0100 (CET)","from mail-ej1-x632.google.com (mail-ej1-x632.google.com\n\t[IPv6:2a00:1450:4864:20::632])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5BEE1627F0\n\tfor ;\n\tMon, 15 Jan 2024 13:24:25 +0100 (CET)","by mail-ej1-x632.google.com with SMTP id\n\ta640c23a62f3a-a28b2e1a13fso913406366b.3\n\tfor ;\n\tMon, 15 Jan 2024 04:24:25 -0800 (PST)","from [192.168.118.26] ([87.116.160.134])\n\tby smtp.gmail.com with ESMTPSA id\n\tv24-20020a1709067d9800b00a2a4efe7d3dsm5249769ejo.79.2024.01.15.04.24.23\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 15 Jan 2024 04:24:24 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705321466;\n\tbh=1O6vnYrpjqFCknZ6p1cPqDVf+wsKLrB/LfXpGSXCPh8=;\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=Fe3X9QvgvDu14eDMTHLecAl0iJKmaa7b+8kLWnTWcPwcxy1vKgaeAPRDNJhEOLqg2\n\tQQz8/ymsJxHmsIRQNnr8L+jjhe7vq0wfImOXTbhwH3FEcbn2RuBRZc6irAilVX+nFY\n\tsAq5lqt8U1ADacN8GlyZPBlDLPeTgfOoSXdECTK9mb+x0iH8JN5NDlxjtG4Y9qbUe9\n\ty/T03wfZEjBFYzgsiQMBC3W49OaVd6Saf1YCIpu7WpXHdEZrcIs5NOJnvGmaTikCUT\n\t3IJFaNtWXgHBBxnmBeLN1qE2vrZXFNhFKMwenQnEcOMPcYrR0WtbKf05C+M1n4e0Oy\n\tSkQzDohliGTyQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1705321465; x=1705926265;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=kguv/YWkPE7NU+Zfk0RWRKNwr0XzpKJ3XoSlTieQxCQ=;\n\tb=Q2jx3EeNC7zBLtSB4R8D1LcpkMBui5YnVwjPz94GWMR3lfVk8N7i2/Lyk3J+If/lQ1\n\tVjXVv0UWfHsU8+11cb5HW/Ugzs5o3dBwhv49KnPWyVnZNqX1wVFmprsjqmpnF3rtiTMj\n\tomfTlsLfuRzOzxO/+fq82wFRvAkK9clzIFVA6fXo1zouNl1EwtnXkZWCrdjDvxOIsrmv\n\tuT+55WBshOxEqjuR5Zd6JfTkg0MdU3OZr6w0uGzY4yl23yf2/seImT+Q2nR4nyN7aoAe\n\t0lmyv3VI/BqCwVR/l7dW/9R2Wdz8fN26hxxhGWsCSE54LT47lwLbWmaD4blQ9rI1MAZD\n\tZqUQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"Q2jx3EeN\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1705321465; x=1705926265;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=kguv/YWkPE7NU+Zfk0RWRKNwr0XzpKJ3XoSlTieQxCQ=;\n\tb=j3RlgVAbNeLSd9ZHxRFHMPms6/LEMEb4Jx2NKYzStdZXG0pkYiIKtxQr0tQ2AokFA9\n\tT8c7xwwrA11pcEMaAc1Z8ycdlkljXNcGGT9IsuYEBLqtTtJdwIf/gLiRkJTMs+ops5RG\n\t3z0aim2cKZa8S0DWi8vNHtxMOyeKSBwtS4zwIzps4Jkc0KLZnqqsBxtS8UohD4CSCzjr\n\tOxn26msEOhc0ZEawpEW/W4rVbMuNsqQQFBOTCxxcJh1C7CH6A1NyNFYw+Gs8+ETstpxh\n\tFizTUJVUkLiDXmQq0LOrjfJT8diDcYYOcAxGzXD7wN8Da2zW5kr3r43ydooZbQ46mIab\n\tx/qA==","X-Gm-Message-State":"AOJu0Yx3RyUyDv+cLrAvviVCV8gqA4Oun7Mr0XiywmzynMoTD0cThOrS\n\tuqWI/w1mHjcy4oCSHAHJ75c=","X-Google-Smtp-Source":"AGHT+IFMZ2gpuXaB6WS0dSqFmHHD7RHP/LkOsePixUxK5yo6mmEPYL9IQCIlKqV9jfYd0rM/6fQsZg==","X-Received":"by 2002:a17:907:a78a:b0:a2d:359b:b880 with SMTP id\n\tvx10-20020a170907a78a00b00a2d359bb880mr1756216ejc.13.1705321464513; \n\tMon, 15 Jan 2024 04:24:24 -0800 (PST)","Message-ID":"<38fde066-9cdd-42c5-baa9-5645ca5bff20@gmail.com>","Date":"Mon, 15 Jan 2024 15:24:22 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","To":"Naushir Patuck ,\n\tHans de Goede ","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-4-hdegoede@redhat.com>\n\t","Content-Language":"en-US","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 03/18] libcamera: dma_heaps: extend\n\tDmaHeap class to support system heap","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":"Andrei Konovalov via libcamera-devel\n\t","Reply-To":"Andrei Konovalov ","Cc":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28598,"web_url":"https://patchwork.libcamera.org/comment/28598/","msgid":"<20240123142629.GC10679@pendragon.ideasonboard.com>","date":"2024-01-23T14:26:29","subject":"Re: [libcamera-devel] [PATCH v2 03/18] libcamera: dma_heaps: extend\n\tDmaHeap class to support system heap","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Andrey,\n\nThank you for the patch.\n\nOn Sat, Jan 13, 2024 at 03:22:03PM +0100, 📷-dev wrote:\n> From: Andrey Konovalov \n> \n> Add an argument to the constructor to specify dma heaps type(s)\n> to use. Can be DmaHeapFlag::Cma and/or DmaHeapFlag::System.\n> By default DmaHeapFlag::Cma is used. If both DmaHeapFlag::Cma and\n> DmaHeapFlag::System are set, CMA heap is tried first.\n> \n> Signed-off-by: Andrey Konovalov \n> Signed-off-by: Hans de Goede \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> ---\n> include/libcamera/internal/dma_heaps.h | 12 +++++++-\n> src/libcamera/dma_heaps.cpp | 39 +++++++++++++++-----------\n> 2 files changed, 34 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n> index cff8f140..22aa1007 100644\n> --- a/include/libcamera/internal/dma_heaps.h\n> +++ b/include/libcamera/internal/dma_heaps.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -16,7 +17,14 @@ namespace libcamera {\n> class DmaHeap\n> {\n> public:\n> -\tDmaHeap();\n> +\tenum class DmaHeapFlag {\n> +\t\tCma = (1 << 0),\n> +\t\tSystem = (1 << 1),\n\nNo need for parentheses.\n\n> +\t};\n> +\n> +\tusing DmaHeapFlags = Flags;\n> +\n> +\tDmaHeap(DmaHeapFlags flags = DmaHeapFlag::Cma);\n> \t~DmaHeap();\n> \tbool isValid() const { return dmaHeapHandle_.isValid(); }\n> \tUniqueFD alloc(const char *name, std::size_t size);\n> @@ -25,4 +33,6 @@ private:\n> \tUniqueFD dmaHeapHandle_;\n> };\n> \n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> index 7444d9c2..177de31b 100644\n> --- a/src/libcamera/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heaps.cpp\n> @@ -16,6 +16,8 @@\n\nYou need to include for std::pair.\n\n> \n> #include \"libcamera/internal/dma_heaps.h\"\n> \n> +namespace libcamera {\n> +\n> /*\n> * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma\n> * to only have to worry about importing.\n> @@ -23,28 +25,33 @@\n> * Annoyingly, should the cma heap size be specified on the kernel command line\n> * instead of DT, the heap gets named \"reserved\" instead.\n> */\n> -static constexpr std::array heapNames = {\n> -\t\"/dev/dma_heap/linux,cma\",\n> -\t\"/dev/dma_heap/reserved\"\n> +static constexpr std::array, 3> heapNames = {\n\nI'm not a big fan of std::pair for this kind of use case, it leads to\nit->first and it->second in the code, which are not very readable.\n\nstruct DmaHeapInfo {\n\tDmaHeap::DmaHeapFlag flag;\n\tconst char *name;\n};\n\nstatic constexpr std::array heapNames\n\nwill make the code more readable.\n\nI would also, while at it, rename it to heapsInfo or something similar,\nas it's not the just names anymore.\n\n> +\t/* CMA heap names first */\n> +\tstd::make_pair(DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\"),\n> +\tstd::make_pair(DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\"),\n> +\tstd::make_pair(DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\")\n> };\n> \n> -namespace libcamera {\n> -\n> LOG_DEFINE_CATEGORY(DmaHeap)\n> \n> -DmaHeap::DmaHeap()\n> +DmaHeap::DmaHeap(DmaHeapFlags flags)\n\nThe new argument will need to be documented.\n\n> {\n> -\tfor (const char *name : heapNames) {\n> -\t\tint ret = ::open(name, O_RDWR | O_CLOEXEC, 0);\n> -\t\tif (ret < 0) {\n> -\t\t\tret = errno;\n> -\t\t\tLOG(DmaHeap, Debug) << \"Failed to open \" << name << \": \"\n> -\t\t\t\t\t<< strerror(ret);\n> -\t\t\tcontinue;\n> -\t\t}\n> +\tint ret;\n> \n> -\t\tdmaHeapHandle_ = UniqueFD(ret);\n> -\t\tbreak;\n> +\tfor (const auto &name : heapNames) {\n> +\t\tif (flags & name.first) {\n\n\t\tif (!(flags & name.first))\n\t\t\tcontinue;\n\nYou won't have to touch the indentation of the code below.\n\n> +\t\t\tret = ::open(name.second, O_RDWR | O_CLOEXEC, 0);\n> +\t\t\tif (ret < 0) {\n> +\t\t\t\tret = errno;\n> +\t\t\t\tLOG(DmaHeap, Debug) << \"Failed to open \" << name.second << \": \"\n> +\t\t\t\t\t\t << strerror(ret);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\tLOG(DmaHeap, Debug) << \"Using \" << name.second;\n> +\t\t\tdmaHeapHandle_ = UniqueFD(ret);\n> +\t\t\tbreak;\n> +\t\t}\n> \t}\n> \n> \tif (!dmaHeapHandle_.isValid())","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 5D1D8C323E\n\tfor ;\n\tTue, 23 Jan 2024 14:26:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 986ED62944;\n\tTue, 23 Jan 2024 15:26:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88A6F628E9\n\tfor ;\n\tTue, 23 Jan 2024 15:26:31 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8CF031890;\n\tTue, 23 Jan 2024 15:25:17 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G6LfV5uw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1706019917;\n\tbh=DDFnDC4h4zdcx8U5/kjQFSzm0Qbz6cBE+9bFAFwlhoU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G6LfV5uwpIWnefY03HXvIfjURTQVYOGWkTv/vUF+z56hK83kRk9peWy+2k+93D2zH\n\tffiFxfynSCrRIqimtkTBgA7++pd40xmHc75GjQ00I91gvC3dPUOStt9BLxmVED6CY5\n\tWh4JeuonQZXlEDE1u7Lx48VLafYg4aG4+yphnRDY=","Date":"Tue, 23 Jan 2024 16:26:29 +0200","From":"Laurent Pinchart ","To":"Hans de Goede ","Subject":"Re: [libcamera-devel] [PATCH v2 03/18] libcamera: dma_heaps: extend\n\tDmaHeap class to support system heap","Message-ID":"<20240123142629.GC10679@pendragon.ideasonboard.com>","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-4-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20240113142218.28063-4-hdegoede@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]