[{"id":27618,"web_url":"https://patchwork.libcamera.org/comment/27618/","msgid":"","date":"2023-07-27T09:31:28","subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Hardcode the maximum number of buffers imported to the V4L2 video device\n> to 32. This only has a minor disadvantage of over-allocating cache slots\n> and V4L2 buffer indexes, but does allow more headroom for using dma\n> buffers allocated from outside of libcamera.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/rpi/common/rpi_stream.cpp | 33 +++++--------------\n> 1 file changed, 8 insertions(+), 25 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> index c158843cb0ed..e9ad1e6f0848 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)\n> \tint ret;\n>\n> \tif (!(flags_ & StreamFlag::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> -\t\t\tresetBuffers();\n> -\t\t}\n> +\t\t/* Export some frame buffers for internal use. */\n> +\t\tret = dev_->exportBuffers(count, &internalBuffers_);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n>\n> -\t\t/* We must import all internal/external exported buffers. */\n> -\t\tcount = bufferMap_.size();\n> +\t\t/* Add these exported buffers to the internal/external buffer list. */\n> +\t\tsetExportedBuffers(&internalBuffers_);\n> +\t\tresetBuffers();\n> \t}\n>\n> -\t/*\n> -\t * If this is an external stream, we must allocate slots for buffers that\n> -\t * might be externally allocated. We have no indication of how many buffers\n> -\t * may be used, so this might overallocate slots in the buffer cache.\n> -\t * Similarly, if this stream is only importing buffers, we do the same.\n> -\t *\n> -\t * \\todo Find a better heuristic, or, even better, an exact solution to\n> -\t * this issue.\n> -\t */\n> -\tif ((flags_ & StreamFlag::External) || (flags_ & StreamFlag::ImportOnly))\n> -\t\tcount = count * 2;\n> -\n> -\treturn dev_->importBuffers(count);\n> +\treturn dev_->importBuffers(32);\n\nMight be good to define 32 somewhere as constexpr to avoid magic\nnumbers, but apart from this\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> }\n>\n> int Stream::queueBuffer(FrameBuffer *buffer)\n> --\n> 2.34.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 66099BDB13\n\tfor ;\n\tThu, 27 Jul 2023 09:31:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6711628C0;\n\tThu, 27 Jul 2023 11:31:34 +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 D351261E24\n\tfor ;\n\tThu, 27 Jul 2023 11:31:32 +0200 (CEST)","from ideasonboard.com (mob-5-91-19-250.net.vodafone.it\n\t[5.91.19.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 324534A9;\n\tThu, 27 Jul 2023 11:30:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690450294;\n\tbh=oAKR/NxAR7rxhMJmdOR+oe2Qb9mIqhVtQU3OSKSHlUQ=;\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=jozZ69rcmLSy2Eft9fNZhSA//vqKo0cuCrCw9QtJU8clNUuch03X/v1Uqn310TA2I\n\tc9Ic+jlPYaDGVafcdTVdteALq65aLFaMscjbuIfaAZ3cgF/YL/wHwRuVWlkV75DUXh\n\t6jpb+f7GoIxXpMGmNfQ/a/PTaFzSpEkTMlaAo58+jyWCyXJE+rljAiFbIK3I1NiBkd\n\tdQbz+QQbC9by7lQHo9GweVfdrp0nOCnaoK413oTYU+3vxzIbS0LXf6o+A8J+64KX7k\n\tafERkYS3GU1YziMkXrDtbQtgr9LpWuaPnMTy1WLHBL1p0VuYX2RgNp2fpKt2FQzWut\n\tXhFCXDE0CauEA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1690450233;\n\tbh=oAKR/NxAR7rxhMJmdOR+oe2Qb9mIqhVtQU3OSKSHlUQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rt8o4L5cVSfF+biRtOFdPsWLsBfz1kKYFjrEKIh7kNB0S80b/m7e3Z/e7o0uOtBy6\n\trjSSdSFgCA5SSGxjdbHH9Kuvj0Zu5AqHmZSVxfTLtEkKHK11xhCiktZVqhuWsMogX+\n\tSiQcE8Gyafer8axQBqksxnjJN5uVaqg/9tJjAaAM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rt8o4L5c\"; dkim-atps=neutral","Date":"Thu, 27 Jul 2023 11:31:28 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230725085540.24863-1-naush@raspberrypi.com>\n\t<20230725085540.24863-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230725085540.24863-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","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":27721,"web_url":"https://patchwork.libcamera.org/comment/27721/","msgid":"<169381704120.277971.2656309906740376842@ping.linuxembedded.co.uk>","date":"2023-09-04T08:44:01","subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2023-07-27 10:31:28)\n> Hi Naush\n> \n> On Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Hardcode the maximum number of buffers imported to the V4L2 video device\n> > to 32. This only has a minor disadvantage of over-allocating cache slots\n> > and V4L2 buffer indexes, but does allow more headroom for using dma\n> > buffers allocated from outside of libcamera.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/rpi/common/rpi_stream.cpp | 33 +++++--------------\n> > 1 file changed, 8 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > index c158843cb0ed..e9ad1e6f0848 100644\n> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)\n> > int ret;\n> >\n> > if (!(flags_ & StreamFlag::ImportOnly)) {\n> > - if (count) {\n> > - /* Export some frame buffers for internal use. */\n> > - ret = dev_->exportBuffers(count, &internalBuffers_);\n> > - if (ret < 0)\n> > - return ret;\n> > -\n> > - /* Add these exported buffers to the internal/external buffer list. */\n> > - setExportedBuffers(&internalBuffers_);\n> > - resetBuffers();\n> > - }\n> > + /* Export some frame buffers for internal use. */\n> > + ret = dev_->exportBuffers(count, &internalBuffers_);\n> > + if (ret < 0)\n> > + return ret;\n> >\n> > - /* We must import all internal/external exported buffers. */\n> > - count = bufferMap_.size();\n> > + /* Add these exported buffers to the internal/external buffer list. */\n> > + setExportedBuffers(&internalBuffers_);\n> > + resetBuffers();\n> > }\n> >\n> > - /*\n> > - * If this is an external stream, we must allocate slots for buffers that\n> > - * might be externally allocated. We have no indication of how many buffers\n> > - * may be used, so this might overallocate slots in the buffer cache.\n> > - * Similarly, if this stream is only importing buffers, we do the same.\n> > - *\n> > - * \\todo Find a better heuristic, or, even better, an exact solution to\n> > - * this issue.\n> > - */\n> > - if ((flags_ & StreamFlag::External) || (flags_ & StreamFlag::ImportOnly))\n> > - count = count * 2;\n> > -\n> > - return dev_->importBuffers(count);\n> > + return dev_->importBuffers(32);\n> \n> Might be good to define 32 somewhere as constexpr to avoid magic\n> numbers, but apart from this\n\nAgreed here. Something like\n\n/** The maximum buffer allocation possible with V4L2 */\nconstexpr maximumV4L2Buffers = 32;\n\nto express that this is the maximum possible with V4L2....\n\nBut otherwise\n\nReviewed-by: Kieran Bingham \n\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n> > }\n> >\n> > int Stream::queueBuffer(FrameBuffer *buffer)\n> > --\n> > 2.34.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 BCB8BBE080\n\tfor ;\n\tMon, 4 Sep 2023 08:44:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A83B628E4;\n\tMon, 4 Sep 2023 10:44:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 107E661DF7\n\tfor ;\n\tMon, 4 Sep 2023 10:44:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 26A4575A;\n\tMon, 4 Sep 2023 10:42:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693817046;\n\tbh=9O6+JAlx7tdLUKwt2dTwv42sqQiipT+DTczwza5tq6k=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=QpWEhOczdHjR+nybK17pGKx1na4+SRuJtkXunaWu+0NcOeLCb76fTf92L6ClXF/3M\n\t8Gf/JbjGM9KZwXo0chEbkOBkIFBg2UVWCtXpqnX3j1+bf4eMu7BSAodWgCTZLciEOY\n\tl/wHwjd1Zn8WFziw+1pvO81rj/4sgJDdeVegI2yeUXjhcvi84O/GwnZxLai3AiuWSe\n\tcO92dAoBh1PRLRt6WWyQt8n863cZn0HNI7vDWyQc/LUoJvWyCszV/oO8FqlatMMVzG\n\tq7qjpQWWy2BqoSrT8n7A2Tn1IuHvCTpPrIBhCx+O9v1/f+P8HgzEzm0nPw3WqOUv1M\n\tlA0/7LTUF+C8g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693816958;\n\tbh=9O6+JAlx7tdLUKwt2dTwv42sqQiipT+DTczwza5tq6k=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=cOGnHAbS3bQmvqeEApk42A15nuj07zpySiyi/lem3bosG7ZV/6Ma2SE4vtLbtQgQI\n\tXd3S27ShQQQmvYGjwRAOawUjOkg9aGGTQBGpjLCkDl+4w3Rxg38jZZ4gbfk7CvhK6K\n\tipaqRLu7Pioo2siqA3lSoRxcRTOfN4qtRlsd22e4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cOGnHAbS\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230725085540.24863-1-naush@raspberrypi.com>\n\t<20230725085540.24863-2-naush@raspberrypi.com>\n\t","To":"Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel , \n\tNaushir Patuck ","Date":"Mon, 04 Sep 2023 09:44:01 +0100","Message-ID":"<169381704120.277971.2656309906740376842@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27724,"web_url":"https://patchwork.libcamera.org/comment/27724/","msgid":"","date":"2023-09-04T09:04:41","subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThanks for the review.\n\nOn Mon, 4 Sept 2023 at 09:44, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Jacopo Mondi via libcamera-devel (2023-07-27 10:31:28)\n> > Hi Naush\n> >\n> > On Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > > Hardcode the maximum number of buffers imported to the V4L2 video\n> device\n> > > to 32. This only has a minor disadvantage of over-allocating cache\n> slots\n> > > and V4L2 buffer indexes, but does allow more headroom for using dma\n> > > buffers allocated from outside of libcamera.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/rpi/common/rpi_stream.cpp | 33 +++++--------------\n> > > 1 file changed, 8 insertions(+), 25 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > index c158843cb0ed..e9ad1e6f0848 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)\n> > > int ret;\n> > >\n> > > if (!(flags_ & StreamFlag::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> > > - resetBuffers();\n> > > - }\n> > > + /* Export some frame buffers for internal use. */\n> > > + ret = dev_->exportBuffers(count, &internalBuffers_);\n> > > + if (ret < 0)\n> > > + return ret;\n> > >\n> > > - /* We must import all internal/external exported\n> buffers. */\n> > > - count = bufferMap_.size();\n> > > + /* Add these exported buffers to the internal/external\n> buffer list. */\n> > > + setExportedBuffers(&internalBuffers_);\n> > > + resetBuffers();\n> > > }\n> > >\n> > > - /*\n> > > - * If this is an external stream, we must allocate slots for\n> buffers that\n> > > - * might be externally allocated. We have no indication of how\n> many buffers\n> > > - * may be used, so this might overallocate slots in the buffer\n> cache.\n> > > - * Similarly, if this stream is only importing buffers, we do\n> the same.\n> > > - *\n> > > - * \\todo Find a better heuristic, or, even better, an exact\n> solution to\n> > > - * this issue.\n> > > - */\n> > > - if ((flags_ & StreamFlag::External) || (flags_ &\n> StreamFlag::ImportOnly))\n> > > - count = count * 2;\n> > > -\n> > > - return dev_->importBuffers(count);\n> > > + return dev_->importBuffers(32);\n> >\n> > Might be good to define 32 somewhere as constexpr to avoid magic\n> > numbers, but apart from this\n>\n> Agreed here. Something like\n>\n> /** The maximum buffer allocation possible with V4L2 */\n> constexpr maximumV4L2Buffers = 32;\n>\n\nAck, I'll make this change for v3.\n\nNaush\n\n\n>\n> to express that this is the maximum possible with V4L2....\n>\n> But otherwise\n>\n> Reviewed-by: Kieran Bingham \n>\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> > > }\n> > >\n> > > int Stream::queueBuffer(FrameBuffer *buffer)\n> > > --\n> > > 2.34.1\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 3307DBE080\n\tfor ;\n\tMon, 4 Sep 2023 09:05:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 643FE628EB;\n\tMon, 4 Sep 2023 11:05:20 +0200 (CEST)","from mail-yw1-x1135.google.com (mail-yw1-x1135.google.com\n\t[IPv6:2607:f8b0:4864:20::1135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 95F8F628E4\n\tfor ;\n\tMon, 4 Sep 2023 11:05:18 +0200 (CEST)","by mail-yw1-x1135.google.com with SMTP id\n\t00721157ae682-58df8cab1f2so11602057b3.3\n\tfor ;\n\tMon, 04 Sep 2023 02:05:18 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693818320;\n\tbh=CLNpAOQvTrZ5H0b89axIkgoEYB8QZeihSO1lv73+Frc=;\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=f4HSkS8ckX1bRFSQLwvO1BT1VeYccY8nnTqcAI1gWIXSYty9XRuHykzJqxzNvYdnv\n\tbvV6l4JeT15otPBmEV3utnh7flKlQKVk5CLQ4HEcivk0sFllJteXNY2NbCd3GbcDNa\n\trwqlSvngvMmlCccjuYYf64Yk5q+fbyjAn0TJy66y8S2AG9QGbyZ7ertCS7W4n3ha67\n\tLz6aAZlKsV8gMPU1IbhFEUNq8XhdfVde9keZHxvdF9VPST9tGm6LI6c/ttfda++efU\n\t5MJeog6x+0FEvnmX+qyAJfbbxI+Y8uOOkfvkFREB0v4tpE0Qpk4HrvW+OpBdKum3hX\n\tgR7YrGNnIa3Zw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1693818317; x=1694423117;\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=05wSz5zYSeXiCVOpGirOZNNu9PBtKGf+mSIIyHonNdc=;\n\tb=puBBTX0ERD4hB9QGH9xhO/4Xqp3WNIMxskUPdkYucWr2/uUOwGPf0ViiYEab4fMECY\n\tGXLbh1D5g5ju3h06hhODomHohquwWIzTxnCSqPu+pM5fjeStVrX6Ct7RsHFMnKIU7uI8\n\t0zJLaLdy/st2mmoEys1Hl/0J23OmNi08QbafiWfcbLjhi2vPkvnSXJPPvyqlRG5B0GeV\n\thCCanau4EkWYvPUXw7hq4tozmRsYxzb3KYjTSjXi1hPU8Yp10KtqnTrcdLm5bBcyMAex\n\tE5IF5ZAlH+xugVOThfxRzCZJgtGpUvmDs/9vz/UM5LGy3R3sgsY900pHdxjbjk2hDsFd\n\tZ8Zg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"puBBTX0E\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1693818317; x=1694423117;\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=05wSz5zYSeXiCVOpGirOZNNu9PBtKGf+mSIIyHonNdc=;\n\tb=dWPyXNAXr9Y4EzB4IUyU7BKEpxmc5vs0pd4cjIP7RiGnGgeIyy/oAceE4/2xH+FgHT\n\twqwktRYyVoEvUnksYvgv5Qu4nGG1GylpoRj0MNa5eG/Ty0j2hUjqYTAqBBZhEjXc7Dqt\n\t3MF6nuQFN0yzFSbNu39Ne0WNIaZTCGu+H/gFLyKLVmN3hqhGSCCHR5xlW9CcuRmIeaAk\n\tK9yz59PeZhcWTBbXKJUtJV1lkXvDd/P1uFEp0R7nnoEwcoupBjA3iyGiCQDsF6r+iRKc\n\tIWb7okmiT0TXCtmT/+sttA0cGWzHL3oRjXzdqtO1a6Z78vEk6b+7Su0ecuns1UgZEKGx\n\tKHWQ==","X-Gm-Message-State":"AOJu0YwZvm2KxPjA8pF3COJttAyVmuyE5NBjV4eqqwO2eAtXB68DfY23\n\tBHQHFT1J2F3xNHOBHw+MYKJ6FCs2F1dnDkd7THnc8RQsezNjZCXCv5M=","X-Google-Smtp-Source":"AGHT+IFAQdXLzPUuRMTv0MvykGjlWvVIos0ln3JOWlqtvPSvsLQb6nREaW1ddGBkgFflGmkamK+kZo9DCSWgDKsNtOk=","X-Received":"by 2002:a0d:d50c:0:b0:570:6d74:21d5 with SMTP id\n\tx12-20020a0dd50c000000b005706d7421d5mr10664790ywd.13.1693818317365;\n\tMon, 04 Sep 2023 02:05:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20230725085540.24863-1-naush@raspberrypi.com>\n\t<20230725085540.24863-2-naush@raspberrypi.com>\n\t\n\t<169381704120.277971.2656309906740376842@ping.linuxembedded.co.uk>","In-Reply-To":"<169381704120.277971.2656309906740376842@ping.linuxembedded.co.uk>","Date":"Mon, 4 Sep 2023 10:04:41 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000001ef724060484cf6f\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","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":"Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27729,"web_url":"https://patchwork.libcamera.org/comment/27729/","msgid":"<169382663099.137962.15641349021600465863@ping.linuxembedded.co.uk>","date":"2023-09-04T11:23:50","subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2023-09-04 10:04:41)\n> Hi Kieran,\n> \n> Thanks for the review.\n> \n> On Mon, 4 Sept 2023 at 09:44, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Jacopo Mondi via libcamera-devel (2023-07-27 10:31:28)\n> > > Hi Naush\n> > >\n> > > On Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via\n> > libcamera-devel wrote:\n> > > > Hardcode the maximum number of buffers imported to the V4L2 video\n> > device\n> > > > to 32. This only has a minor disadvantage of over-allocating cache\n> > slots\n> > > > and V4L2 buffer indexes, but does allow more headroom for using dma\n> > > > buffers allocated from outside of libcamera.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > .../pipeline/rpi/common/rpi_stream.cpp | 33 +++++--------------\n> > > > 1 file changed, 8 insertions(+), 25 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > index c158843cb0ed..e9ad1e6f0848 100644\n> > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)\n> > > > int ret;\n> > > >\n> > > > if (!(flags_ & StreamFlag::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> > > > - resetBuffers();\n> > > > - }\n> > > > + /* Export some frame buffers for internal use. */\n> > > > + ret = dev_->exportBuffers(count, &internalBuffers_);\n> > > > + if (ret < 0)\n> > > > + return ret;\n> > > >\n> > > > - /* We must import all internal/external exported\n> > buffers. */\n> > > > - count = bufferMap_.size();\n> > > > + /* Add these exported buffers to the internal/external\n> > buffer list. */\n> > > > + setExportedBuffers(&internalBuffers_);\n> > > > + resetBuffers();\n> > > > }\n> > > >\n> > > > - /*\n> > > > - * If this is an external stream, we must allocate slots for\n> > buffers that\n> > > > - * might be externally allocated. We have no indication of how\n> > many buffers\n> > > > - * may be used, so this might overallocate slots in the buffer\n> > cache.\n> > > > - * Similarly, if this stream is only importing buffers, we do\n> > the same.\n> > > > - *\n> > > > - * \\todo Find a better heuristic, or, even better, an exact\n> > solution to\n> > > > - * this issue.\n> > > > - */\n> > > > - if ((flags_ & StreamFlag::External) || (flags_ &\n> > StreamFlag::ImportOnly))\n> > > > - count = count * 2;\n> > > > -\n> > > > - return dev_->importBuffers(count);\n> > > > + return dev_->importBuffers(32);\n> > >\n> > > Might be good to define 32 somewhere as constexpr to avoid magic\n> > > numbers, but apart from this\n> >\n> > Agreed here. Something like\n> >\n> > /** The maximum buffer allocation possible with V4L2 */\n> > constexpr maximumV4L2Buffers = 32;\n> >\n> \n> Ack, I'll make this change for v3.\n\nNo point sending a v3 just for this change I don't think.\n\nYou could go direct to a PR with this fixed.\n\n--\nKieran\n\n\n> \n> Naush\n> \n> \n> >\n> > to express that this is the maximum possible with V4L2....\n> >\n> > But otherwise\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > >\n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > }\n> > > >\n> > > > int Stream::queueBuffer(FrameBuffer *buffer)\n> > > > --\n> > > > 2.34.1\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 CCADEC0F1B\n\tfor ;\n\tMon, 4 Sep 2023 11:23:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF236628E9;\n\tMon, 4 Sep 2023 13:23:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E75AA628D7\n\tfor ;\n\tMon, 4 Sep 2023 13:23:53 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E34F2CCE;\n\tMon, 4 Sep 2023 13:22:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693826636;\n\tbh=tNcA7KZaP1FXGTROi19FJNdkBpHr+eH51UvwWF4wUE4=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=t8qpBp4MjunLjw2pS4N5CRJ0g+N3aaORXodsocR9qBOJUvymIsaRBfwyASccFyEgS\n\tuu7BpuRfSjFunXRh7YW1zULCW6BthZFytHmyPgBEA3SGRUlaUG+ZDWMxc7zsMbtOoh\n\tddvnJI8hZJ9/wwVAiezSjHMfITwWQ/NsHvUENRDvj27V2ENLiWRtqbD4SbJ0iBkFEq\n\t7kL8fE9+0I9oiOpeZQrLNHvkCZQYOu6h8LTsHgtCaQLyevthfwvRfUZExibkiA2f0c\n\tLuzVT4JY4DSEMCmSn1yHjVd2Fy4KHayOWqm1dOa8I5MW0ZUWD9cX6Vo3AJG5v9m62M\n\t4VQyughYXwsSg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693826548;\n\tbh=tNcA7KZaP1FXGTROi19FJNdkBpHr+eH51UvwWF4wUE4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=jmoBKgUNsygt0RDOrcH/W53EtlhNcWAhFu+aufRXJ1cc+NyhXdiCLfws2Tuoqi2iW\n\t9XckTySjZr/5vpNp+IDDIeUAHkydmVLMoFYgM2dnqmWjouwNxR6LJ86IVor4LyneJP\n\t3Vr4JsB8J1BE//+Cr6J30JJQENPy68S4xIWz58Jo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jmoBKgUN\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230725085540.24863-1-naush@raspberrypi.com>\n\t<20230725085540.24863-2-naush@raspberrypi.com>\n\t\n\t<169381704120.277971.2656309906740376842@ping.linuxembedded.co.uk>\n\t","To":"Naushir Patuck ","Date":"Mon, 04 Sep 2023 12:23:50 +0100","Message-ID":"<169382663099.137962.15641349021600465863@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27743,"web_url":"https://patchwork.libcamera.org/comment/27743/","msgid":"<20230907152918.GA19752@pendragon.ideasonboard.com>","date":"2023-09-07T15:29:18","subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","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 Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Hardcode the maximum number of buffers imported to the V4L2 video device\n> to 32. This only has a minor disadvantage of over-allocating cache slots\n> and V4L2 buffer indexes, but does allow more headroom for using dma\n> buffers allocated from outside of libcamera.\n\nI fear there's an additional disadvantage :-S\n\nDmabuf instances are imported implicitly in V4L2, when preparing a\nbuffer (either with VIDIOC_PREPARE_BUF or VIDIOC_QBUF). At that point,\nthe reference count to the buffer is increased. The reference is dropped\neither when the V4L2 buffer is deleted (currently only possible with\nVIDIOC_REQBUFS(0)), or when the dmabuf instance is evicted by importing\na differ dmabuf (again with VIDIOC_PREPARE_BUF or VIDIOC_QBUF) for the\nsame buffer slot.\n\nThe eviction mechanism makes it very expensive to cycle through a pool\nof dmabuf buffers if you queue them in different buffer slots all the\ntime, so the V4L2VideoDevice class tries hard to reuse a buffer slot\nthat is already associated with the dmabuf instances when queuing a\nFrameBuffer. The downside, however, is that a large number of buffer\nslots means that dmabuf instances will be held on for longer before they\nget evicted.\n\nIf an application preallocates a limited set of buffers and cycles\nthrough them, it's likely all fine. But if it destroys existing buffers\nand allocates new ones (not necessarily all the time, it could do so for\nstill images only for instance), then the buffers that are freed by the\napplication will still have references in the kernel, and their memory\nwon't actually be freed. You may then end run out of memory.\n\nI think this is an issue with the V4L2 API and its implicit dmabuf\nimport, there's likely nothing we can do about it in userspace to really\nfix the problem, but increasing the number of buffer slots\nunconditionally can make things much worse.\n\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/rpi/common/rpi_stream.cpp | 33 +++++--------------\n> 1 file changed, 8 insertions(+), 25 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> index c158843cb0ed..e9ad1e6f0848 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)\n> \tint ret;\n> \n> \tif (!(flags_ & StreamFlag::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> -\t\t\tresetBuffers();\n> -\t\t}\n> +\t\t/* Export some frame buffers for internal use. */\n> +\t\tret = dev_->exportBuffers(count, &internalBuffers_);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> \n> -\t\t/* We must import all internal/external exported buffers. */\n> -\t\tcount = bufferMap_.size();\n> +\t\t/* Add these exported buffers to the internal/external buffer list. */\n> +\t\tsetExportedBuffers(&internalBuffers_);\n> +\t\tresetBuffers();\n> \t}\n> \n> -\t/*\n> -\t * If this is an external stream, we must allocate slots for buffers that\n> -\t * might be externally allocated. We have no indication of how many buffers\n> -\t * may be used, so this might overallocate slots in the buffer cache.\n> -\t * Similarly, if this stream is only importing buffers, we do the same.\n> -\t *\n> -\t * \\todo Find a better heuristic, or, even better, an exact solution to\n> -\t * this issue.\n> -\t */\n> -\tif ((flags_ & StreamFlag::External) || (flags_ & StreamFlag::ImportOnly))\n> -\t\tcount = count * 2;\n> -\n> -\treturn dev_->importBuffers(count);\n> +\treturn dev_->importBuffers(32);\n> }\n> \n> int Stream::queueBuffer(FrameBuffer *buffer)","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 73FD1BE080\n\tfor ;\n\tThu, 7 Sep 2023 15:29:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1FB0628EC;\n\tThu, 7 Sep 2023 17:29:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7444627DF\n\tfor ;\n\tThu, 7 Sep 2023 17:29:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(ftip006315900.acc1.colindale.21cn-nte.bt.net [81.134.214.249])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C56D88C2;\n\tThu, 7 Sep 2023 17:27:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694100546;\n\tbh=z30Ya8u/2js5HjtRuyT8TtgM+109ugxu/hCPiFgarWc=;\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=wuJVbbFiKxFt1izYh4cd2z0fHJIkRv5sWG5Ph8NXLeVyMP4p7z4tFL9EgKovlQPfp\n\t724SJa1S/70jEeaai5041s7HqBVAiaWRcBq1Tqk09l+QsFwdfPcYsb0ERo5GBo8sSU\n\tqf1NOdMADmTZlSC0tvp+1qLLW/VYqvRtAqDi7xAKcox9BRiV1WMFSOeF+ToYQgef8e\n\tJ/4+PemYeVqqxcvmPsupZTmQl+XB7vncXZZVbUfNULLimV3m1e7j8Uga7NiWRg2m6y\n\tS7JncRVrldrTMIkq+I0eL20fJVnDA2l0ydMnq1DNZavDzn05iPR2Vtlt5s5zQEelZy\n\tsueEw4MsgS+Cw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694100456;\n\tbh=z30Ya8u/2js5HjtRuyT8TtgM+109ugxu/hCPiFgarWc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jK5VzuZMftio0XONeky2dHuzaG77eTUIZVIB1aPaFRHxDYio60WAnPv9/M1CKH/9T\n\tYmjkmkBKhuhvD5IYgxZ9rfFdlzTbODOgSFemaqHN2Pcizesiy3kCISVzlgK7BNXzxO\n\tPUTNuIk7zsMvLXAwbLH/OF6VgCFnfBuq2bN3Inmc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jK5VzuZM\"; dkim-atps=neutral","Date":"Thu, 7 Sep 2023 18:29:18 +0300","To":"Naushir Patuck ","Message-ID":"<20230907152918.GA19752@pendragon.ideasonboard.com>","References":"<20230725085540.24863-1-naush@raspberrypi.com>\n\t<20230725085540.24863-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230725085540.24863-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer\n\timport count to 32","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\" "}}]