[{"id":20791,"web_url":"https://patchwork.libcamera.org/comment/20791/","msgid":"<163654026901.1896795.14837710823582506340@Monstersaurus>","date":"2021-11-10T10:31:09","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2021-11-10 10:08:01)\n> For simplicity, the pipeline handler would look at the maximum number of\n> buffers set in the StreamConfiguration and allocate the same number of internal\n> buffers for all device nodes. This would likely overallocate buffers for some\n> nodes. Rework this logic to try and minimise overallcations without compromising\n> performance.\n> \n> For ISP nodes, we only ever need 1 set of internal buffers, as the hardware runs\n> synchronous with the requests and IPA.\n> \n> For Unicam nodes, allocate a minimum for 4 buffers (exported + internal), but\n> also require at least 1 internal buffer.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++-----\n> 1 file changed, 33 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 11d3c2b120dd..5f0f00aacd59 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> {\n> RPiCameraData *data = cameraData(camera);\n> + unsigned int numBuffers;\n> + bool rawStream = false;\n> int ret;\n> \n> - /*\n> - * Decide how many internal buffers to allocate. For now, simply look\n> - * at how many external buffers will be provided. We'll need to improve\n> - * this logic. However, we really must have all streams allocate the same\n> - * number of buffers to simplify error handling in queueRequestDevice().\n> - */\n> - unsigned int maxBuffers = 0;\n> - for (const Stream *s : camera->streams())\n> - if (static_cast(s)->isExternal())\n> - maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> + for (Stream *s : camera->streams()) {\n> + if (isRaw(s->configuration().pixelFormat)) {\n> + numBuffers = s->configuration().bufferCount;\n> + rawStream = true;\n> + break;\n> + }\n> + }\n> \n> + /* Decide how many internal buffers to allocate. */\n> for (auto const stream : data->streams_) {\n> - ret = stream->prepareBuffers(maxBuffers);\n> + if (stream == &data->unicam_[Unicam::Image] ||\n> + stream == &data->unicam_[Unicam::Embedded]) {\n> + /*\n> + * For Unicam, allocate a minimum of 4 buffers as we want\n> + * to avoid any frame drops. If an application has configured\n> + * a RAW stream, allocate additional buffers to make up the\n> + * minimum, but ensure we have at least 1 set of internal\n> + * buffers to use to minimise frame drops.\n> + */\n> + constexpr unsigned int minBuffers = 4;\n> + numBuffers = rawStream ? std::max(1, minBuffers - numBuffers)\n> + : minBuffers;\n> + } else {\n> + /*\n> + * Since the ISP runs synchronous with the IPA and requests,\n> + * we only ever need one set of internal buffers. Any buffers\n> + * the application wants to hold onto will already be exported\n> + * through PipelineHandlerRPi::exportFrameBuffers().\n> + */\n\nOk, I see this is for the 4 device streams of the ISP. These don't\nrepresent the buffers that go to the user application though do they?\n\nOh - I see - it's /just/ any extra internal allocation. If a user buffer\nis sent in, it would be used on Output0, Output1 ... so we're not going\nto starve here.\n\nReviewed-by: Kieran Bingham \n\n> + numBuffers = 1;\n> + }\n> +\n> + ret = stream->prepareBuffers(numBuffers);\n> if (ret < 0)\n> return ret;\n> }\n> -- \n> 2.25.1\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 47DD4BF415\n\tfor ;\n\tWed, 10 Nov 2021 10:31:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C8246035A;\n\tWed, 10 Nov 2021 11:31:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A9E8D60128\n\tfor ;\n\tWed, 10 Nov 2021 11:31:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21D41D8B;\n\tWed, 10 Nov 2021 11:31:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"a04zoFIJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636540271;\n\tbh=rj2uVvOO2Yr1NEMY1iBKcz0pOUi0zYcNuCrZCm8jZmI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=a04zoFIJBDkdEzA9YId60T7a73MyOnOITn0TzebKT1kQYECeqt3dDoaC8MB7pl4Qh\n\tXUHbQXsRJkzin+Kj1oOMHSFet5Im2vlUPQB5OITil+XhPxjug/x4f/VbzHuRDgfw3T\n\tq5Xs5q+dKcMRiy9pu2F2Kb/zJBtwP/AnWVDvJ0E4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211110100802.349623-2-naush@raspberrypi.com>","References":"<20211110100802.349623-1-naush@raspberrypi.com>\n\t<20211110100802.349623-2-naush@raspberrypi.com>","From":"Kieran Bingham ","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 10 Nov 2021 10:31:09 +0000","Message-ID":"<163654026901.1896795.14837710823582506340@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","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":"Roman Stratiienko ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20819,"web_url":"https://patchwork.libcamera.org/comment/20819/","msgid":"<26d2cd00-ba85-7dae-7b90-27a48336a936@ideasonboard.com>","date":"2021-11-10T17:27:38","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Naush\n\nOn 11/10/21 3:38 PM, Naushir Patuck wrote:\n> For simplicity, the pipeline handler would look at the maximum number of\n> buffers set in the StreamConfiguration and allocate the same number of internal\n> buffers for all device nodes. This would likely overallocate buffers for some\n> nodes. Rework this logic to try and minimise overallcations without compromising\n> performance.\n>\n> For ISP nodes, we only ever need 1 set of internal buffers, as the hardware runs\n> synchronous with the requests and IPA.\n>\n> For Unicam nodes, allocate a minimum for 4 buffers (exported + internal), but\n> also require at least 1 internal buffer.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++-----\n> 1 file changed, 33 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 11d3c2b120dd..5f0f00aacd59 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> +\tunsigned int numBuffers;\n\nI would initialise this value to 0, it might stand a chance in future \n(though unlikely) that it is used uninitialised below\n\n         std::max(1, minBuffers - numBuffers)\n\nSo just future-proof the usage\n\nReviewed-by: Umang Jain \n\n> +\tbool rawStream = false;\n> \tint ret;\n> \n> -\t/*\n> -\t * Decide how many internal buffers to allocate. For now, simply look\n> -\t * at how many external buffers will be provided. We'll need to improve\n> -\t * this logic. However, we really must have all streams allocate the same\n> -\t * number of buffers to simplify error handling in queueRequestDevice().\n> -\t */\n> -\tunsigned int maxBuffers = 0;\n> -\tfor (const Stream *s : camera->streams())\n> -\t\tif (static_cast(s)->isExternal())\n> -\t\t\tmaxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> +\tfor (Stream *s : camera->streams()) {\n> +\t\tif (isRaw(s->configuration().pixelFormat)) {\n> +\t\t\tnumBuffers = s->configuration().bufferCount;\n> +\t\t\trawStream = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> \n> +\t/* Decide how many internal buffers to allocate. */\n> \tfor (auto const stream : data->streams_) {\n> -\t\tret = stream->prepareBuffers(maxBuffers);\n> +\t\tif (stream == &data->unicam_[Unicam::Image] ||\n> +\t\t stream == &data->unicam_[Unicam::Embedded]) {\n> +\t\t\t/*\n> +\t\t\t * For Unicam, allocate a minimum of 4 buffers as we want\n> +\t\t\t * to avoid any frame drops. If an application has configured\n> +\t\t\t * a RAW stream, allocate additional buffers to make up the\n> +\t\t\t * minimum, but ensure we have at least 1 set of internal\n> +\t\t\t * buffers to use to minimise frame drops.\n> +\t\t\t */\n> +\t\t\tconstexpr unsigned int minBuffers = 4;\n> +\t\t\tnumBuffers = rawStream ? std::max(1, minBuffers - numBuffers)\n> +\t\t\t\t\t : minBuffers;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * Since the ISP runs synchronous with the IPA and requests,\n> +\t\t\t * we only ever need one set of internal buffers. Any buffers\n> +\t\t\t * the application wants to hold onto will already be exported\n> +\t\t\t * through PipelineHandlerRPi::exportFrameBuffers().\n> +\t\t\t */\n> +\t\t\tnumBuffers = 1;\n> +\t\t}\n> +\n> +\t\tret = stream->prepareBuffers(numBuffers);\n> \t\tif (ret < 0)\n> \t\t\treturn ret;\n> \t}","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8DB0DBDB1C\n\tfor ;\n\tWed, 10 Nov 2021 17:27:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD5846035D;\n\tWed, 10 Nov 2021 18:27:44 +0100 (CET)","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 8FB1D6033C\n\tfor ;\n\tWed, 10 Nov 2021 18:27:43 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 738908BB;\n\tWed, 10 Nov 2021 18:27:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HbB4MwDV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636565263;\n\tbh=70UQli+7XYD/BNqH7Dw9nq12vizjBXy3yNwAC/oV88I=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=HbB4MwDV1qKavEYY0FDUWn2hgdXYkf9nCqII+ETlzu7rxkza2ANx0B0EIPeYo6Bgy\n\tnJ/F0ZzVNFHXFOVipf19IouQcx7jUskR7dGCBDcgpcxrRGZXbX25ZQntI+gq23NbGo\n\tUyzXlPOSHg2AW7FwIhGt280oMEBqKqZx86hQaO3s=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211110100802.349623-1-naush@raspberrypi.com>\n\t<20211110100802.349623-2-naush@raspberrypi.com>","From":"Umang Jain ","Message-ID":"<26d2cd00-ba85-7dae-7b90-27a48336a936@ideasonboard.com>","Date":"Wed, 10 Nov 2021 22:57:38 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211110100802.349623-2-naush@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","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":"Roman Stratiienko ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20863,"web_url":"https://patchwork.libcamera.org/comment/20863/","msgid":"","date":"2021-11-11T14:35:20","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Wed, Nov 10, 2021 at 10:08:01AM +0000, Naushir Patuck wrote:\n> For simplicity, the pipeline handler would look at the maximum number of\n> buffers set in the StreamConfiguration and allocate the same number of internal\n> buffers for all device nodes. This would likely overallocate buffers for some\n> nodes. Rework this logic to try and minimise overallcations without compromising\n> performance.\n> \n> For ISP nodes, we only ever need 1 set of internal buffers, as the hardware runs\n> synchronous with the requests and IPA.\n> \n> For Unicam nodes, allocate a minimum for 4 buffers (exported + internal), but\n> also require at least 1 internal buffer.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++-----\n> 1 file changed, 33 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 11d3c2b120dd..5f0f00aacd59 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> +\tunsigned int numBuffers;\n\nI'd name the variable numRawBuffers.\n\n> +\tbool rawStream = false;\n> \tint ret;\n> \n> -\t/*\n> -\t * Decide how many internal buffers to allocate. For now, simply look\n> -\t * at how many external buffers will be provided. We'll need to improve\n> -\t * this logic. However, we really must have all streams allocate the same\n> -\t * number of buffers to simplify error handling in queueRequestDevice().\n\nDoes the error logic still work after this change ?\n\n> -\t */\n> -\tunsigned int maxBuffers = 0;\n> -\tfor (const Stream *s : camera->streams())\n> -\t\tif (static_cast(s)->isExternal())\n> -\t\t\tmaxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> +\tfor (Stream *s : camera->streams()) {\n> +\t\tif (isRaw(s->configuration().pixelFormat)) {\n> +\t\t\tnumBuffers = s->configuration().bufferCount;\n> +\t\t\trawStream = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> \n> +\t/* Decide how many internal buffers to allocate. */\n> \tfor (auto const stream : data->streams_) {\n> -\t\tret = stream->prepareBuffers(maxBuffers);\n\nAnd here add a numBuffers local variable.\n\n> +\t\tif (stream == &data->unicam_[Unicam::Image] ||\n> +\t\t stream == &data->unicam_[Unicam::Embedded]) {\n> +\t\t\t/*\n> +\t\t\t * For Unicam, allocate a minimum of 4 buffers as we want\n> +\t\t\t * to avoid any frame drops. If an application has configured\n> +\t\t\t * a RAW stream, allocate additional buffers to make up the\n> +\t\t\t * minimum, but ensure we have at least 1 set of internal\n> +\t\t\t * buffers to use to minimise frame drops.\n> +\t\t\t */\n> +\t\t\tconstexpr unsigned int minBuffers = 4;\n> +\t\t\tnumBuffers = rawStream ? std::max(1, minBuffers - numBuffers)\n> +\t\t\t\t\t : minBuffers;\n\nYou could initialize numRawBuffers to 0 and drop rawStream, as if\n!rawStream, then numRawBuffers will be equal to 0, so\n\n\t\t\tnumBuffers = std::max(1, minBuffers - numRawBuffers);\n\nwould do the right thing.\n\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * Since the ISP runs synchronous with the IPA and requests,\n> +\t\t\t * we only ever need one set of internal buffers. Any buffers\n> +\t\t\t * the application wants to hold onto will already be exported\n> +\t\t\t * through PipelineHandlerRPi::exportFrameBuffers().\n> +\t\t\t */\n> +\t\t\tnumBuffers = 1;\n> +\t\t}\n> +\n> +\t\tret = stream->prepareBuffers(numBuffers);\n\nI have a hard time follow this. Before the patch, the pipeline handler\ncalls prepareBuffers() with the number of buffers requested by the\napplication. Now it calls it with a number of internal buffers only,\nwithout any change to the prepareBuffers() function itself. This would\nbenefit from an explanation in the commit message, as it's not clear why\nit's correct.\n\nFurthermore, what will happen if an application requests 4 buffers for\nthe raw stream and any number of buffers for a processed streams, and\nrepeatedly queues requests with no buffer for the raw stream ? It seems\nto me that you'll now allocate a single extra buffer for the raw stream,\nwhich won't be enough to keep the unicam queue running.\n\n> \t\tif (ret < 0)\n> \t\t\treturn ret;\n> \t}","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 87F41BDB1C\n\tfor ;\n\tThu, 11 Nov 2021 14:35:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A29A66035A;\n\tThu, 11 Nov 2021 15:35:43 +0100 (CET)","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 597D6600B5\n\tfor ;\n\tThu, 11 Nov 2021 15:35:42 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E931DDEE;\n\tThu, 11 Nov 2021 15:35:41 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"llUx5Pzg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636641342;\n\tbh=KonGKtFGTGqniN3L/B5133p3w7U564n94BlS8i7bbq4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=llUx5PzgOqCyYVCNe1V5qo7BNGK7rJrSw+afxeI9vRTcisabnaMVHbt9BO4ek4nt8\n\tVECXDyuhc/eR//pM69l1HbLN4YqD+BGZDuPhA6gisz7+NrAOzb+EN4cRIeN5P4to7P\n\ty35m/KNDXcHyzcDiI3cJSoP16jYCNBNM+phHqC+A=","Date":"Thu, 11 Nov 2021 16:35:20 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211110100802.349623-1-naush@raspberrypi.com>\n\t<20211110100802.349623-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211110100802.349623-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","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":"libcamera-devel@lists.libcamera.org,\n\tRoman Stratiienko ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20865,"web_url":"https://patchwork.libcamera.org/comment/20865/","msgid":"","date":"2021-11-11T14:58:21","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback!\n\nOn Thu, 11 Nov 2021 at 14:35, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Wed, Nov 10, 2021 at 10:08:01AM +0000, Naushir Patuck wrote:\n> > For simplicity, the pipeline handler would look at the maximum number of\n> > buffers set in the StreamConfiguration and allocate the same number of\n> internal\n> > buffers for all device nodes. This would likely overallocate buffers for\n> some\n> > nodes. Rework this logic to try and minimise overallcations without\n> compromising\n> > performance.\n> >\n> > For ISP nodes, we only ever need 1 set of internal buffers, as the\n> hardware runs\n> > synchronous with the requests and IPA.\n> >\n> > For Unicam nodes, allocate a minimum for 4 buffers (exported +\n> internal), but\n> > also require at least 1 internal buffer.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++-----\n> > 1 file changed, 33 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 11d3c2b120dd..5f0f00aacd59 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera\n> *camera)\n> > int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > + unsigned int numBuffers;\n>\n> I'd name the variable numRawBuffers.\n>\n> > + bool rawStream = false;\n> > int ret;\n> >\n> > - /*\n> > - * Decide how many internal buffers to allocate. For now, simply\n> look\n> > - * at how many external buffers will be provided. We'll need to\n> improve\n> > - * this logic. However, we really must have all streams allocate\n> the same\n> > - * number of buffers to simplify error handling in\n> queueRequestDevice().\n>\n> Does the error logic still work after this change ?\n>\n\nYes, the comment above has been bit-rotted, and this is handled correctly\nnow.\n\n\n>\n> > - */\n> > - unsigned int maxBuffers = 0;\n> > - for (const Stream *s : camera->streams())\n> > - if (static_cast(s)->isExternal())\n> > - maxBuffers = std::max(maxBuffers,\n> s->configuration().bufferCount);\n> > + for (Stream *s : camera->streams()) {\n> > + if (isRaw(s->configuration().pixelFormat)) {\n> > + numBuffers = s->configuration().bufferCount;\n> > + rawStream = true;\n> > + break;\n> > + }\n> > + }\n> >\n> > + /* Decide how many internal buffers to allocate. */\n> > for (auto const stream : data->streams_) {\n> > - ret = stream->prepareBuffers(maxBuffers);\n>\n> And here add a numBuffers local variable.\n>\n> > + if (stream == &data->unicam_[Unicam::Image] ||\n> > + stream == &data->unicam_[Unicam::Embedded]) {\n> > + /*\n> > + * For Unicam, allocate a minimum of 4 buffers as\n> we want\n> > + * to avoid any frame drops. If an application has\n> configured\n> > + * a RAW stream, allocate additional buffers to\n> make up the\n> > + * minimum, but ensure we have at least 1 set of\n> internal\n> > + * buffers to use to minimise frame drops.\n> > + */\n> > + constexpr unsigned int minBuffers = 4;\n> > + numBuffers = rawStream ? std::max(1,\n> minBuffers - numBuffers)\n> > + : minBuffers;\n>\n> You could initialize numRawBuffers to 0 and drop rawStream, as if\n> !rawStream, then numRawBuffers will be equal to 0, so\n>\n> numBuffers = std::max(1, minBuffers -\n> numRawBuffers);\n>\n> would do the right thing.\n>\n> > + } else {\n> > + /*\n> > + * Since the ISP runs synchronous with the IPA and\n> requests,\n> > + * we only ever need one set of internal buffers.\n> Any buffers\n> > + * the application wants to hold onto will already\n> be exported\n> > + * through\n> PipelineHandlerRPi::exportFrameBuffers().\n> > + */\n> > + numBuffers = 1;\n> > + }\n> > +\n> > + ret = stream->prepareBuffers(numBuffers);\n>\n> I have a hard time follow this. Before the patch, the pipeline handler\n> calls prepareBuffers() with the number of buffers requested by the\n> application. Now it calls it with a number of internal buffers only,\n> without any change to the prepareBuffers() function itself. This would\n> benefit from an explanation in the commit message, as it's not clear why\n> it's correct.\n>\n\nFor clarification, prepareBuffers() would (and still will) allocate\ninternal use\nbuffers, and call importBuffers() to allocate the buffer cache with the\ninternal\n+ user requested buffer count. Buffer allocations for the user requested\nbuffer\ncount goes through PipelineHandler::exportFrameBuffers().\n\nSo this change (somewhat) decouples the user count from the internal count.\nI will clarify this in the commit message.\n\n\n>\n> Furthermore, what will happen if an application requests 4 buffers for\n> the raw stream and any number of buffers for a processed streams, and\n> repeatedly queues requests with no buffer for the raw stream ? It seems\n> to me that you'll now allocate a single extra buffer for the raw stream,\n> which won't be enough to keep the unicam queue running.\n>\n\nThis is a tricky one. Without any hints from the application as to what it\nintends\nto do, I have to balance memory usage with performance. You are right that\nwith only a single internal buffer allocated, the performance may be\ndegraded\nif the application does not provide buffers through the Request. However,\nIf the application has allocated 4x 18Mbyte buffers (12mpix 12-bit), I\nreally don't\nwant to be allocating 2x more of them when they possibly may never be used\n(assuming the application is circulating all these buffers constantly). I\nwould hope\napplication writers would be using those buffers sensibly :-)\n\nSome of our platforms have very small CMA heaps to work with, memory will\nget very tight for these use-cases, so I do want to limit memory usage at\nthe\nexpense of performance. Some hints from the application on how it intends\nto\nuse buffers might help here with this balance.\n\nDavid, do you think we can bump the internal allocation up to 2 buffers?\n\nRegards,\nNaush\n\n\n>\n> > if (ret < 0)\n> > return ret;\n> > }\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 3A1C8BDB1C\n\tfor ;\n\tThu, 11 Nov 2021 14:58:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92FF960368;\n\tThu, 11 Nov 2021 15:58:39 +0100 (CET)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB2C2600B5\n\tfor ;\n\tThu, 11 Nov 2021 15:58:38 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id 207so12418958ljf.10\n\tfor ;\n\tThu, 11 Nov 2021 06:58:38 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"h5zFtNjW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=VeRKbh42ovTuOv0h2kkM/bMfJgq2nY0xX7eBOYG9LPo=;\n\tb=h5zFtNjWpTRXxqBzVhL3zFD6nSemAgXlJr95SVdsl/8tLMzo09Noog6hj0AdbbyGFH\n\tXzn9omvT2ZFMiHQmW0avi00CsbhlCfomuvskzZnmGAg0YVuZrJD7tfAzjsowSzWzErEc\n\t9lo8IHRcOmexpV+WQFx+CE4fs8p+kKb3Rt5Iso2byXiaYgON2CA1LaEoilYPOXnyexjO\n\tVM0hBS72Lyla5TE26OOwovB0V6GSe9EC7FtTC2h8YYzhYKX3sugqyMkc3Mu3JqTKfODb\n\tIdRcgfwJxU3j4cbFF5f2dAxPa2y6iD6cQAK4VoJg3PT9tfLH9w89naf+2IgUybnu06L2\n\t8mzQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=VeRKbh42ovTuOv0h2kkM/bMfJgq2nY0xX7eBOYG9LPo=;\n\tb=NIDbpL6oMMXJyBYGj+Q15swAx6+YMOBJsqhklDQxUDT6uWc4Kd1saYCWQhS8ihoE4r\n\tNFaLeZR3/sPU65NEy1jAKTdX09X8MeRnFoYhT3M+Fq2Qutlu0kLt17wivG1B4EEqxuKF\n\tE+tA7A+5N7qAQQQBl2h219rF3prW7vsqMkf179vrtph5X+3YK465A6rorn5rCJ3ILsnl\n\tD0YLmW8he08qEnB8gNTOovaR1tmOIYMjbZtY4Z346CB7rzyxWt0HAJk5x2OODuB7DehM\n\tvIP+tU3nim0OPrMpuT0cJcsosCo9kD25V7JmTn5n8onXhcAbPlvYNvzT2b5+Crw5L183\n\tePAg==","X-Gm-Message-State":"AOAM532A9LsM11oYRnDYlEIVSB2KQTB36r78qcJhSYNU9TjhDiDXDUgB\n\tbiyr938SbRn7HY0PzYo6RF8wKuSNs+0s9iXbasAo3A==","X-Google-Smtp-Source":"ABdhPJyg8QAcLHxczIUGGi11mSoRWNPz3hvf5j6EL3sxkpXljB8BbGiLxPXE7/r5PJ0SfrT5OsQX2Sx1OKc7dUphJrU=","X-Received":"by 2002:a2e:a22a:: with SMTP id i10mr7635034ljm.16.1636642718004;\n\tThu, 11 Nov 2021 06:58:38 -0800 (PST)","MIME-Version":"1.0","References":"<20211110100802.349623-1-naush@raspberrypi.com>\n\t<20211110100802.349623-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 11 Nov 2021 14:58:21 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000d4cb2505d08493d6\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","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":"libcamera devel ,\n\tRoman Stratiienko ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20867,"web_url":"https://patchwork.libcamera.org/comment/20867/","msgid":"","date":"2021-11-11T15:48:58","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, Nov 11, 2021 at 02:58:21PM +0000, Naushir Patuck wrote:\n> On Thu, 11 Nov 2021 at 14:35, Laurent Pinchart wrote:\n> > On Wed, Nov 10, 2021 at 10:08:01AM +0000, Naushir Patuck wrote:\n> > > For simplicity, the pipeline handler would look at the maximum number of\n> > > buffers set in the StreamConfiguration and allocate the same number of internal\n> > > buffers for all device nodes. This would likely overallocate buffers for some\n> > > nodes. Rework this logic to try and minimise overallcations without compromising\n> > > performance.\n> > >\n> > > For ISP nodes, we only ever need 1 set of internal buffers, as the hardware runs\n> > > synchronous with the requests and IPA.\n> > >\n> > > For Unicam nodes, allocate a minimum for 4 buffers (exported + internal), but\n> > > also require at least 1 internal buffer.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++-----\n> > > 1 file changed, 33 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 11d3c2b120dd..5f0f00aacd59 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > {\n> > > RPiCameraData *data = cameraData(camera);\n> > > + unsigned int numBuffers;\n> > I'd name the variable numRawBuffers.\n> >\n> > > + bool rawStream = false;\n> > > int ret;\n> > >\n> > > - /*\n> > > - * Decide how many internal buffers to allocate. For now, simply look\n> > > - * at how many external buffers will be provided. We'll need to improve\n> > > - * this logic. However, we really must have all streams allocate the same\n> > > - * number of buffers to simplify error handling in queueRequestDevice().\n> >\n> > Does the error logic still work after this change ?\n> \n> Yes, the comment above has been bit-rotted, and this is handled correctly\n> now.\n> \n> > > - */\n> > > - unsigned int maxBuffers = 0;\n> > > - for (const Stream *s : camera->streams())\n> > > - if (static_cast(s)->isExternal())\n> > > - maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> > > + for (Stream *s : camera->streams()) {\n> > > + if (isRaw(s->configuration().pixelFormat)) {\n> > > + numBuffers = s->configuration().bufferCount;\n> > > + rawStream = true;\n> > > + break;\n> > > + }\n> > > + }\n> > >\n> > > + /* Decide how many internal buffers to allocate. */\n> > > for (auto const stream : data->streams_) {\n> > > - ret = stream->prepareBuffers(maxBuffers);\n> >\n> > And here add a numBuffers local variable.\n> >\n> > > + if (stream == &data->unicam_[Unicam::Image] ||\n> > > + stream == &data->unicam_[Unicam::Embedded]) {\n> > > + /*\n> > > + * For Unicam, allocate a minimum of 4 buffers as we want\n> > > + * to avoid any frame drops. If an application has configured\n> > > + * a RAW stream, allocate additional buffers to make up the\n> > > + * minimum, but ensure we have at least 1 set of internal\n> > > + * buffers to use to minimise frame drops.\n> > > + */\n> > > + constexpr unsigned int minBuffers = 4;\n> > > + numBuffers = rawStream ? std::max(1, minBuffers - numBuffers)\n> > > + : minBuffers;\n> >\n> > You could initialize numRawBuffers to 0 and drop rawStream, as if\n> > !rawStream, then numRawBuffers will be equal to 0, so\n> >\n> > numBuffers = std::max(1, minBuffers - numRawBuffers);\n> >\n> > would do the right thing.\n> >\n> > > + } else {\n> > > + /*\n> > > + * Since the ISP runs synchronous with the IPA and requests,\n> > > + * we only ever need one set of internal buffers. Any buffers\n> > > + * the application wants to hold onto will already be exported\n> > > + * through PipelineHandlerRPi::exportFrameBuffers().\n> > > + */\n> > > + numBuffers = 1;\n> > > + }\n> > > +\n> > > + ret = stream->prepareBuffers(numBuffers);\n> >\n> > I have a hard time follow this. Before the patch, the pipeline handler\n> > calls prepareBuffers() with the number of buffers requested by the\n> > application. Now it calls it with a number of internal buffers only,\n> > without any change to the prepareBuffers() function itself. This would\n> > benefit from an explanation in the commit message, as it's not clear why\n> > it's correct.\n> \n> For clarification, prepareBuffers() would (and still will) allocate internal use\n> buffers, and call importBuffers() to allocate the buffer cache with the internal\n> + user requested buffer count. Buffer allocations for the user requested buffer\n> count goes through PipelineHandler::exportFrameBuffers().\n\nI'm looking at prepareBuffers():\n\nint Stream::prepareBuffers(unsigned int count)\n{\n\tint ret;\n\n\tif (!importOnly_) {\n\t\tif (count) {\n\t\t\t/* Export some frame buffers for internal use. */\n\t\t\tret = dev_->exportBuffers(count, &internalBuffers_);\n\t\t\tif (ret < 0)\n\t\t\t\treturn ret;\n\n\t\t\t/* Add these exported buffers to the internal/external buffer list. */\n\t\t\tsetExportedBuffers(&internalBuffers_);\n\n\t\t\t/* Add these buffers to the queue of internal usable buffers. */\n\t\t\tfor (auto const &buffer : internalBuffers_)\n\t\t\t\tavailableBuffers_.push(buffer.get());\n\t\t}\n\n\t\t/* We must import all internal/external exported buffers. */\n\t\tcount = bufferMap_.size();\n\t}\n\n\treturn dev_->importBuffers(count);\n}\n\nIn the !importOnly_ case, we add count buffers to bufferMap_ by a call\nto setExportedBuffers(). setExportedBuffers() is also called by\nPipelineHandlerRPi::exportFrameBuffers(), so we currently add\nmax(s->configuration().bufferCount) buffers to any buffers already\nexported by the application. This certainly overallocates, as we\nessentially allocations bufferCount * 2 buffers.\n\nWith this patch, overallocation is reduced. However, there's no\nrequirement for applications to use the FrameBufferAllocator (which ends\nup calling PipelineHandlerRPi::exportFrameBuffers()) to allocate\nbuffers. If an application allocates buffers through different means\n(for instance exporting them from a display device), then we will call\ndev_->importBuffers() with a very low count.\n\n> So this change (somewhat) decouples the user count from the internal count.\n> I will clarify this in the commit message.\n> \n> > Furthermore, what will happen if an application requests 4 buffers for\n> > the raw stream and any number of buffers for a processed streams, and\n> > repeatedly queues requests with no buffer for the raw stream ? It seems\n> > to me that you'll now allocate a single extra buffer for the raw stream,\n> > which won't be enough to keep the unicam queue running.\n> \n> This is a tricky one. Without any hints from the application as to what it intends\n> to do, I have to balance memory usage with performance. You are right that\n> with only a single internal buffer allocated, the performance may be degraded\n> if the application does not provide buffers through the Request. However,\n> If the application has allocated 4x 18Mbyte buffers (12mpix 12-bit), I really don't\n> want to be allocating 2x more of them when they possibly may never be used\n> (assuming the application is circulating all these buffers constantly). I would hope\n> application writers would be using those buffers sensibly :-)\n> \n> Some of our platforms have very small CMA heaps to work with, memory will\n> get very tight for these use-cases, so I do want to limit memory usage at the\n> expense of performance. Some hints from the application on how it intends to\n> use buffers might help here with this balance.\n\nI agree with you on the problem statement, but I'm wondering if we\nshouldn't start by implementing the safe option to avoid frame drops,\nand then improve performance on top (possibly with a hint API).\n\n> David, do you think we can bump the internal allocation up to 2 buffers?\n> \n> > > if (ret < 0)\n> > > return ret;\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 C40E2BDB1C\n\tfor ;\n\tThu, 11 Nov 2021 15:49:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2F596035A;\n\tThu, 11 Nov 2021 16:49:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F0A9600B5\n\tfor ;\n\tThu, 11 Nov 2021 16:49:21 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D1AAFDEE;\n\tThu, 11 Nov 2021 16:49:20 +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=\"kwscQEdt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636645760;\n\tbh=ZwO9Z6TMvO/8zTPt/i5vHNHTMhn3nEtgwMgff5SQox0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kwscQEdt5U7/iiyo4JxqV3TnbTuIVIpjNu/pLAjTipvS4Gf9KI7R7hhB56yMOQ45U\n\tmVPvo0rn/pdyXmw7c7BXuXvYEndeQS/353uPfsvF9mouBmqNsYI74BW66XQb+R5vIL\n\t9CMFrG8ymelxm/cJzSR5KUdQdtXxt3CfSwfF0OZ8=","Date":"Thu, 11 Nov 2021 17:48:58 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211110100802.349623-1-naush@raspberrypi.com>\n\t<20211110100802.349623-2-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","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":"libcamera devel ,\n\tRoman Stratiienko ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20874,"web_url":"https://patchwork.libcamera.org/comment/20874/","msgid":"","date":"2021-11-11T16:42:32","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Thu, 11 Nov 2021 at 15:49, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Thu, Nov 11, 2021 at 02:58:21PM +0000, Naushir Patuck wrote:\n> > On Thu, 11 Nov 2021 at 14:35, Laurent Pinchart wrote:\n> > > On Wed, Nov 10, 2021 at 10:08:01AM +0000, Naushir Patuck wrote:\n> > > > For simplicity, the pipeline handler would look at the maximum\n> number of\n> > > > buffers set in the StreamConfiguration and allocate the same number\n> of internal\n> > > > buffers for all device nodes. This would likely overallocate buffers\n> for some\n> > > > nodes. Rework this logic to try and minimise overallcations without\n> compromising\n> > > > performance.\n> > > >\n> > > > For ISP nodes, we only ever need 1 set of internal buffers, as the\n> hardware runs\n> > > > synchronous with the requests and IPA.\n> > > >\n> > > > For Unicam nodes, allocate a minimum for 4 buffers (exported +\n> internal), but\n> > > > also require at least 1 internal buffer.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 44\n> ++++++++++++++-----\n> > > > 1 file changed, 33 insertions(+), 11 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 11d3c2b120dd..5f0f00aacd59 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1211,21 +1211,43 @@ int\n> PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > > {\n> > > > RPiCameraData *data = cameraData(camera);\n> > > > + unsigned int numBuffers;\n> > > I'd name the variable numRawBuffers.\n> > >\n> > > > + bool rawStream = false;\n> > > > int ret;\n> > > >\n> > > > - /*\n> > > > - * Decide how many internal buffers to allocate. For now,\n> simply look\n> > > > - * at how many external buffers will be provided. We'll need\n> to improve\n> > > > - * this logic. However, we really must have all streams\n> allocate the same\n> > > > - * number of buffers to simplify error handling in\n> queueRequestDevice().\n> > >\n> > > Does the error logic still work after this change ?\n> >\n> > Yes, the comment above has been bit-rotted, and this is handled correctly\n> > now.\n> >\n> > > > - */\n> > > > - unsigned int maxBuffers = 0;\n> > > > - for (const Stream *s : camera->streams())\n> > > > - if (static_cast(s)->isExternal())\n> > > > - maxBuffers = std::max(maxBuffers,\n> s->configuration().bufferCount);\n> > > > + for (Stream *s : camera->streams()) {\n> > > > + if (isRaw(s->configuration().pixelFormat)) {\n> > > > + numBuffers = s->configuration().bufferCount;\n> > > > + rawStream = true;\n> > > > + break;\n> > > > + }\n> > > > + }\n> > > >\n> > > > + /* Decide how many internal buffers to allocate. */\n> > > > for (auto const stream : data->streams_) {\n> > > > - ret = stream->prepareBuffers(maxBuffers);\n> > >\n> > > And here add a numBuffers local variable.\n> > >\n> > > > + if (stream == &data->unicam_[Unicam::Image] ||\n> > > > + stream == &data->unicam_[Unicam::Embedded]) {\n> > > > + /*\n> > > > + * For Unicam, allocate a minimum of 4 buffers\n> as we want\n> > > > + * to avoid any frame drops. If an application\n> has configured\n> > > > + * a RAW stream, allocate additional buffers\n> to make up the\n> > > > + * minimum, but ensure we have at least 1 set\n> of internal\n> > > > + * buffers to use to minimise frame drops.\n> > > > + */\n> > > > + constexpr unsigned int minBuffers = 4;\n> > > > + numBuffers = rawStream ? std::max(1,\n> minBuffers - numBuffers)\n> > > > + : minBuffers;\n> > >\n> > > You could initialize numRawBuffers to 0 and drop rawStream, as if\n> > > !rawStream, then numRawBuffers will be equal to 0, so\n> > >\n> > > numBuffers = std::max(1, minBuffers -\n> numRawBuffers);\n> > >\n> > > would do the right thing.\n> > >\n> > > > + } else {\n> > > > + /*\n> > > > + * Since the ISP runs synchronous with the IPA\n> and requests,\n> > > > + * we only ever need one set of internal\n> buffers. Any buffers\n> > > > + * the application wants to hold onto will\n> already be exported\n> > > > + * through\n> PipelineHandlerRPi::exportFrameBuffers().\n> > > > + */\n> > > > + numBuffers = 1;\n> > > > + }\n> > > > +\n> > > > + ret = stream->prepareBuffers(numBuffers);\n> > >\n> > > I have a hard time follow this. Before the patch, the pipeline handler\n> > > calls prepareBuffers() with the number of buffers requested by the\n> > > application. Now it calls it with a number of internal buffers only,\n> > > without any change to the prepareBuffers() function itself. This would\n> > > benefit from an explanation in the commit message, as it's not clear\n> why\n> > > it's correct.\n> >\n> > For clarification, prepareBuffers() would (and still will) allocate\n> internal use\n> > buffers, and call importBuffers() to allocate the buffer cache with the\n> internal\n> > + user requested buffer count. Buffer allocations for the user requested\n> buffer\n> > count goes through PipelineHandler::exportFrameBuffers().\n>\n> I'm looking at prepareBuffers():\n>\n> int Stream::prepareBuffers(unsigned int count)\n> {\n> int ret;\n>\n> if (!importOnly_) {\n> if (count) {\n> /* Export some frame buffers for internal use. */\n> ret = dev_->exportBuffers(count,\n> &internalBuffers_);\n> if (ret < 0)\n> return ret;\n>\n> /* Add these exported buffers to the\n> internal/external buffer list. */\n> setExportedBuffers(&internalBuffers_);\n>\n> /* Add these buffers to the queue of internal\n> usable buffers. */\n> for (auto const &buffer : internalBuffers_)\n> availableBuffers_.push(buffer.get());\n> }\n>\n> /* We must import all internal/external exported buffers.\n> */\n> count = bufferMap_.size();\n> }\n>\n> return dev_->importBuffers(count);\n> }\n>\n> In the !importOnly_ case, we add count buffers to bufferMap_ by a call\n> to setExportedBuffers(). setExportedBuffers() is also called by\n> PipelineHandlerRPi::exportFrameBuffers(), so we currently add\n> max(s->configuration().bufferCount) buffers to any buffers already\n> exported by the application. This certainly overallocates, as we\n> essentially allocations bufferCount * 2 buffers.\n>\n> With this patch, overallocation is reduced. However, there's no\n> requirement for applications to use the FrameBufferAllocator (which ends\n> up calling PipelineHandlerRPi::exportFrameBuffers()) to allocate\n> buffers. If an application allocates buffers through different means\n> (for instance exporting them from a display device), then we will call\n> dev_->importBuffers() with a very low count.\n>\n\nYes, I see how this could go wrong. Should I just assume the worst, and\npass VIDEO_MAX_FRAME into importBuffers. Would that be advisable?\n\n\n>\n> > So this change (somewhat) decouples the user count from the internal\n> count.\n> > I will clarify this in the commit message.\n> >\n> > > Furthermore, what will happen if an application requests 4 buffers for\n> > > the raw stream and any number of buffers for a processed streams, and\n> > > repeatedly queues requests with no buffer for the raw stream ? It seems\n> > > to me that you'll now allocate a single extra buffer for the raw\n> stream,\n> > > which won't be enough to keep the unicam queue running.\n> >\n> > This is a tricky one. Without any hints from the application as to what\n> it intends\n> > to do, I have to balance memory usage with performance. You are right\n> that\n> > with only a single internal buffer allocated, the performance may be\n> degraded\n> > if the application does not provide buffers through the Request.\n> However,\n> > If the application has allocated 4x 18Mbyte buffers (12mpix 12-bit), I\n> really don't\n> > want to be allocating 2x more of them when they possibly may never be\n> used\n> > (assuming the application is circulating all these buffers constantly).\n> I would hope\n> > application writers would be using those buffers sensibly :-)\n> >\n> > Some of our platforms have very small CMA heaps to work with, memory will\n> > get very tight for these use-cases, so I do want to limit memory usage\n> at the\n> > expense of performance. Some hints from the application on how it\n> intends to\n> > use buffers might help here with this balance.\n>\n> I agree with you on the problem statement, but I'm wondering if we\n> shouldn't start by implementing the safe option to avoid frame drops,\n> and then improve performance on top (possibly with a hint API).\n>\n\nI think maybe we can start with 2 internal buffer allocations, then see if\nthat may ever need reducing.\n\nRegards,\nNaush\n\n\n>\n> > David, do you think we can bump the internal allocation up to 2 buffers?\n> >\n> > > > if (ret < 0)\n> > > > return ret;\n> > > > }\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 BDC50BF415\n\tfor ;\n\tThu, 11 Nov 2021 16:42:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 106DC6035F;\n\tThu, 11 Nov 2021 17:42:51 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67026600B5\n\tfor ;\n\tThu, 11 Nov 2021 17:42:49 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id c32so15577830lfv.4\n\tfor ;\n\tThu, 11 Nov 2021 08:42:49 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"W1KXSYt0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=SYg6qweN/FJPkRhwfxmSXbPhSJQ4R3eEBEec8SD80NM=;\n\tb=W1KXSYt0/14MtGcBAdiSXYT6N7nXIY7mnMB0NUqHEkOTY9XAc1hDIdETFBvIfRFjaD\n\tsMPn+daGupKD56hXfA8yvRAeWAE6/Euygv4TnVGYzg6NAcmO607L86xbPEanueYJf3fF\n\tTWX1ZTBEmZ5pY4e9hpJItGB6kSOEPgTKcpVoVspP0sFx06Nf2C8Z1KunFSvI24FCxZWo\n\t7IbQjHHaP5x3pBJeePFVEPyNHCTsrj7ECathWbFlofK4BKF7C2tVxRgJk6KD0M6APmZ3\n\t2INq8zIB93eomjSmUM2yuagIYXu9l2t5kprVT58dbmadWH1hDnMOzd9+EcEZ8Zo8ztqS\n\tAYmg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=SYg6qweN/FJPkRhwfxmSXbPhSJQ4R3eEBEec8SD80NM=;\n\tb=Ke0PTXW2G9OoOpr+WHl1+sUNhJNcUVfu3/8kombII3qdpVxBjnay/kfgUGWCs8H0gL\n\tVtM+CC6nwoGXG7qUrrFfpW7jLJ+X8JEngV4a3Gft75TXLsVlCaJx4C8ezaSkT/hf+/+D\n\tRzIstHWSOLVJ1joUZwuYxlvFSq/P1xEfPzrM0CXVSjrKB/GJ7nNOwykOZN65vXy+Scld\n\tttcMvKO7cEGxF2gHwy+XtldTKKA+kCjLiuLA56P/UBzF19EYI8eRH2L0S+bznSM0FVky\n\txQKmM8D6weimR8wFuOKqKe7QwKIeOH7aWcYz0YMc7C6e1Tb1KUJbqdVnHcjCUkuiBHxW\n\tynvQ==","X-Gm-Message-State":"AOAM5320pFnPJanCcNBWFCtzQNbGE5ByeoYBnrj3E+zpS94ZIlUko+Uu\n\t0gHJKXOL30j/rKWteOoiiUZcX/6hw7Gemjp2vgCeKQ==","X-Google-Smtp-Source":"ABdhPJycA+zTQyxwuyDfLIlDtVTTd3lKTElKpXXsoVfr3Mrs2Uflxo100tr3n2ORYSkjeZLUNpk4x7jiCdQH6V2N7/4=","X-Received":"by 2002:a05:6512:3763:: with SMTP id\n\tz3mr7468534lft.315.1636648968533; \n\tThu, 11 Nov 2021 08:42:48 -0800 (PST)","MIME-Version":"1.0","References":"<20211110100802.349623-1-naush@raspberrypi.com>\n\t<20211110100802.349623-2-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 11 Nov 2021 16:42:32 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000644c0205d086087f\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the\n\tinternal buffer allocation scheme","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":"libcamera devel ,\n\tRoman Stratiienko ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]