[{"id":27812,"web_url":"https://patchwork.libcamera.org/comment/27812/","msgid":"","date":"2023-09-18T09:37:22","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi William,\n\nThank you for this patch.\n\nOn Mon, 18 Sept 2023 at 10:31, William Vinnicombe via libcamera-devel\n wrote:\n>\n> If the pipeline runs out of embedded data buffers, then it will pass\n> the frame to the IPA without the metadata. The IPA then has to use the\n> delayed controls as inputs to the algorithms. This can cause problems\n> with the subsequent algorithms if the sensor did not action the\n> controls, especially with the autofocus as that doesn't have controls\n> which can be passed in lieu of the metadata.\n>\n> Reduce the likelihood of this by increasing the number of embedded data\n> buffers, as they are small so a generous number can be allocated.\n>\n> Signed-off-by: William Vinnicombe \n\nMakes sense!\n\nReviewed-by: Naushir Patuck \n\n> ---\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index 018cf488..6777c697 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera *camera)\n> } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> /*\n> * Embedded data buffers are (currently) for internal use,\n> - * so allocate the minimum required to avoid frame drops.\n> + * so allocate a generous number as they are small.\n> */\n> - numBuffers = minBuffers;\n> + numBuffers = 12;\n> } else {\n> /*\n> * Since the ISP runs synchronous with the IPA and requests,\n> --\n> 2.39.2\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 1B369BE080\n\tfor ;\n\tMon, 18 Sep 2023 09:38:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79E2D6293A;\n\tMon, 18 Sep 2023 11:38:01 +0200 (CEST)","from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com\n\t[IPv6:2607:f8b0:4864:20::b29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5308662938\n\tfor ;\n\tMon, 18 Sep 2023 11:37:59 +0200 (CEST)","by mail-yb1-xb29.google.com with SMTP id\n\t3f1490d57ef6-d81afd5273eso3403497276.3\n\tfor ;\n\tMon, 18 Sep 2023 02:37:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695029881;\n\tbh=H/HMYIaL1AqG0sQGFW/T8QVU1C0V0y8N/To0ZwGF4JQ=;\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=ftpyLky8wpI8+DC57LOfSWctnj4rXtcCOdstA0hRi5Gj6WKwU9S/ZYWHVCmBE/bX3\n\tfXyfMBsmGM1wsgXhYi3zr+TMvprat2nx9lS+IeCJt9Vd7lLXDoIuf6lfB1z2LU61mu\n\tIUTskJsGQTHhNHsy63dEgwrKupNbM7CsgdNpIX9N3Ygx7d+uXtewOpQwuznfVvNKH0\n\t+I4kv+L3xCDXkkodMgL/zE0p42RH2AtlHy0+nUqMFS1E1J2RcW1Ddz8rGkQn7I4AUa\n\taPaEz95BA/uvzP75AlDHsTEV8adl1VeV0XtLYhJgVD/v5WqfCkADXwVD60rlSEYxjZ\n\tvl+tV+OEhIUXA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1695029878; x=1695634678;\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=2qzIWwj/Je0kF0lH6YyIEpfB/QmNhqA5KUKsbCckqdc=;\n\tb=sBGGb4Tmf3KxC0oLMHYZJW1fImKqJXRJkHjLUhOo8BBP/sYpoa834M3wOSdLKDgpKC\n\tDFCLkChsrPToUxZqIv2lJSvHA3pLwAyB9atYv6djk4PkL4mWA7v79RuYUp6rainIjho+\n\tuHtrALEUHIg+5e2JrZUp7I+SwdV0xkHXvzbdPapzzyN4+N918rA7HwHSm3RRGAGxU4Pa\n\t7MgWvtZJjIIyE0uNBW19bKaxfYMa19k1ZtpK8bTKJReXGQpHWU0yazB+KQI+4rpmCiHA\n\tDbrpxJAK+Ja+LpmIJNnCC58+uWgwsrtf0Jb2H2/MFKF+CYKCdYyyErXfvozeP4j8/3GZ\n\tqvVQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sBGGb4Tm\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1695029878; x=1695634678;\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=2qzIWwj/Je0kF0lH6YyIEpfB/QmNhqA5KUKsbCckqdc=;\n\tb=jutYzHKMuK5acfjd+r8OiAJv+dqSJ7hPkwULJelkenA+puU0M/A8HzUOHF7s7BwrR7\n\tfok8YBvYrhtcCQCN8Uo8vFLE2zoxEIuLU8p+kitHFB2Uslbl7nBkwjwzhHt45GBmcB6C\n\tPm7rvaFnLjpeFMAYYxHIt8MhA/MMVKVQQOzqk6uXEq7XMHFI9XWDMeu5UHSil5zP5d2p\n\tA4J3NBqmnHwGLCjNsK4DLbbIFaWerEahq1f+KjrDvDEghHeZrlnZYjgICit27QoAkwtm\n\tWu1qM3+BQqlX7zaEB0cn4dS5PaD9/uO5liom1NcQWipoXnc1TNnw22MUPec0FiCBfNYi\n\tP8bg==","X-Gm-Message-State":"AOJu0Yx1Czx/V/NkC5hMd+xTmdytjDsAYf2T4giLXdeS34akxcfbGkGN\n\tIPk7Uy8rt/AuStHFG2lyAMwjDXAYTPmtYgbyedM8sA==","X-Google-Smtp-Source":"AGHT+IFpJJ4/DAF9LuBRbYzB+OqdD9EHAqMtZZboh54Eh/4Ba2/J7fCYosdBB0MAYed8uExuYZnDF3d+tq/yNE7Wero=","X-Received":"by 2002:a25:3289:0:b0:d81:78cb:9267 with SMTP id\n\ty131-20020a253289000000b00d8178cb9267mr7783308yby.62.1695029878146;\n\tMon, 18 Sep 2023 02:37:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>","In-Reply-To":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>","Date":"Mon, 18 Sep 2023 10:37:22 +0100","Message-ID":"","To":"William Vinnicombe ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27813,"web_url":"https://patchwork.libcamera.org/comment/27813/","msgid":"<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>","date":"2023-09-18T10:40:59","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)\n> If the pipeline runs out of embedded data buffers, then it will pass\n> the frame to the IPA without the metadata. The IPA then has to use the\n> delayed controls as inputs to the algorithms. This can cause problems\n> with the subsequent algorithms if the sensor did not action the\n> controls, especially with the autofocus as that doesn't have controls\n> which can be passed in lieu of the metadata.\n\nI assume the implication here is that you've detected this happening at\ntimes ?\n\n> Reduce the likelihood of this by increasing the number of embedded data\n> buffers, as they are small so a generous number can be allocated.\n> \n> Signed-off-by: William Vinnicombe \n> ---\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index 018cf488..6777c697 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera *camera)\n> } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> /*\n> * Embedded data buffers are (currently) for internal use,\n> - * so allocate the minimum required to avoid frame drops.\n> + * so allocate a generous number as they are small.\n\nCan we improve on 'Generous number' at all? What aspects can we consider\nhere?\n\nThe pipeline depth should be a known value (4 frames?) so doubling that\nwould perhaps already be 'generous'?\n\nOr basing it on the number of raw stream buffers we may have available\nto queue to unicam? We can't ever capture embedded data without also\ncapturing a raw image can we ? (or maybe we can? )\n\n\"\"\"\n * Embedded data buffers are (currently) for internal use, and are small\n * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid\n * causing problems in the IPA when we can not supply the metadata.\n *\n * We may have up to 6 raw buffers in the pipeline, so use double this\n * value to ensure we always have sufficient resources.\n\"\"\"\n\n(Of course I just made up the comment details so I don't expect it to be\ncorrect at all, just an example)\n\n> */\n> - numBuffers = minBuffers;\n> + numBuffers = 12;\n\nNormally we'd put constants like this into a constexpr too and define at\nthe top of the file - but I think with a large comment like above that\nwould be overkill here. Best to keep the code in one place in this\ninstance IMO.\n\n> } else {\n> /*\n> * Since the ISP runs synchronous with the IPA and requests,\n> -- \n> 2.39.2\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 EBB2CC326B\n\tfor ;\n\tMon, 18 Sep 2023 10:41:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 29CCD61DEB;\n\tMon, 18 Sep 2023 12:41:04 +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 3ED0A61DE9\n\tfor ;\n\tMon, 18 Sep 2023 12:41:02 +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 D29C71257;\n\tMon, 18 Sep 2023 12:39:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695033664;\n\tbh=97cqesRHu0aS6/KK+07yXs6HVk846yoWc4ArrAGebDs=;\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:\n\tFrom;\n\tb=cmffFKkTs+cHTBSETGlAtVCFZdstZ/L7cHFpxW0sBAZ6/cNpeva3J2BxqggGwpsJX\n\ttCZDsFFhAZixCji6LUbcRdh+8skGQxLq1r35c1Flncetr+kL7Mxs12p2S1MQ47j84c\n\tA2F6EoFM3j/eDinRupYmD9gGlMqEaqyrmXk080Z4iTXxFXoILFxocGTrzI8MpNbLSI\n\tBSt7DyA17vCWDsUC3jy07BQdYRJbtZYv/urqITf8FIou6W3rDXFY5ESUxDJIHbKWJ9\n\tMTQi1j6JigF8GxUFpl0QvKBWwuI6WmxJrairiyPuelVuWFKOHSkpuMEy6XLva2fIjg\n\tiQEK7jvtxawZA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1695033566;\n\tbh=97cqesRHu0aS6/KK+07yXs6HVk846yoWc4ArrAGebDs=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=RX8Ja38x2oK2V+EOLrToqdOWx3Lpsk1vTZdSnfEFE0CLPeNyLGRAJL6mFFhTkPc6/\n\tPv5pkASRXcsl7h+nklytJno9+tDPSDjiJRFriwhPu8SnV3+mTi5jTkxcOrr3bQbn6M\n\thanOWeVZPL4gdFyz/cG8PbfUZrJa+MYcBq8+yWqg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RX8Ja38x\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>","To":"William Vinnicombe ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 18 Sep 2023 11:40:59 +0100","Message-ID":"<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27832,"web_url":"https://patchwork.libcamera.org/comment/27832/","msgid":"","date":"2023-09-21T13:44:38","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","submitter":{"id":131,"url":"https://patchwork.libcamera.org/api/people/131/","name":"William Vinnicombe","email":"william.vinnicombe@raspberrypi.com"},"content":"Hi Kieran,\n\nThanks for your comments\n\nOn Mon, 18 Sept 2023 at 11:41, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)\n> > If the pipeline runs out of embedded data buffers, then it will pass\n> > the frame to the IPA without the metadata. The IPA then has to use the\n> > delayed controls as inputs to the algorithms. This can cause problems\n> > with the subsequent algorithms if the sensor did not action the\n> > controls, especially with the autofocus as that doesn't have controls\n> > which can be passed in lieu of the metadata.\n>\n> I assume the implication here is that you've detected this happening at\n> times ?\n>\n> > Reduce the likelihood of this by increasing the number of embedded data\n> > buffers, as they are small so a generous number can be allocated.\n> >\n> > Signed-off-by: William Vinnicombe \n> > ---\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > index 018cf488..6777c697 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera\n> *camera)\n> > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > /*\n> > * Embedded data buffers are (currently) for\n> internal use,\n> > - * so allocate the minimum required to avoid\n> frame drops.\n> > + * so allocate a generous number as they are\n> small.\n>\n> Can we improve on 'Generous number' at all? What aspects can we consider\n> here?\n>\n> The pipeline depth should be a known value (4 frames?) so doubling that\n> would perhaps already be 'generous'?\n>\n> Or basing it on the number of raw stream buffers we may have available\n> to queue to unicam? We can't ever capture embedded data without also\n> capturing a raw image can we ? (or maybe we can? )\n>\n> \"\"\"\n> * Embedded data buffers are (currently) for internal use, and are small\n> * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid\n> * causing problems in the IPA when we can not supply the metadata.\n> *\n> * We may have up to 6 raw buffers in the pipeline, so use double this\n> * value to ensure we always have sufficient resources.\n> \"\"\"\n>\n> (Of course I just made up the comment details so I don't expect it to be\n> correct at all, just an example)\n>\n\n12 was selected as an arbitrary number, which is more than the number of\ninput buffers (which is application dependent, but is typically 8-10). As\nthey are much smaller than the image buffers (1-2kB rather than MB), it's\nmuch simpler here to just set it to 12 rather than querying the number of\ninput buffers, as the extra memory used is small. If that explanation\nsounds good, I can submit a v2 with the comment edited to that effect?\n\n\n> > */\n> > - numBuffers = minBuffers;\n> > + numBuffers = 12;\n>\n> Normally we'd put constants like this into a constexpr too and define at\n> the top of the file - but I think with a large comment like above that\n> would be overkill here. Best to keep the code in one place in this\n> instance IMO.\n>\n> > } else {\n> > /*\n> > * Since the ISP runs synchronous with the IPA\n> and requests,\n> > --\n> > 2.39.2\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 7BFE1BE080\n\tfor ;\n\tThu, 21 Sep 2023 13:44:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE72D62944;\n\tThu, 21 Sep 2023 15:44:52 +0200 (CEST)","from mail-pl1-x62c.google.com (mail-pl1-x62c.google.com\n\t[IPv6:2607:f8b0:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1222628D8\n\tfor ;\n\tThu, 21 Sep 2023 15:44:51 +0200 (CEST)","by mail-pl1-x62c.google.com with SMTP id\n\td9443c01a7336-1c451541f23so7657985ad.2\n\tfor ;\n\tThu, 21 Sep 2023 06:44:51 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695303893;\n\tbh=wvGQUFdQKE54EEJSt4Sw1nIqIwHDNrI4fjl0rvRb2Jg=;\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:\n\tFrom;\n\tb=iNd9/5tWBTp6F6V5C35FHV4tRhpHuGZ+Wg8YMcm09ncn+1adPNiH9D2zgL3SGKpfr\n\tbjNijqvpEyG1x36ldaVUqw/MM6+8/AwGDxqH9JGV9gY06y0I7B4hBdw4mLgQ03UcQ0\n\t/Ib2FtzIv0I1DOmqnmEB1k4/SqUslg66d2ZTh2mzXifkbG3FAm7lt5ljQ6Y+UMgb1E\n\t5DU5F3iU68eOCNAbdM1IsYrFkYzB47hSssQYPxogY3Xb/J52pyXDKspc3f8wPJY7ZL\n\tJUDsJAG/KXgQTHLzuux9xBqZfkq5yetrfHTySS6Rn8BayKopEsve3zEeGOcLCWlgba\n\tWtCjr0KIXZ/FA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1695303890; x=1695908690;\n\tdarn=lists.libcamera.org; \n\th=to:subject:message-id:date:from:in-reply-to:references:mime-version\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=68SOf6hv3mVVUT5r64W6+ndW/vLVzjL4pATP7rA3kUY=;\n\tb=is8llaDu+1HTkxMWj5oHJwdkQHkjN/ceBFzlIP9aH8HVYRc6xQAxv0JomqniFCAzMY\n\tAmis/pQZtiQ+NihM39Rwz5wRhVMkUsHj6oxWzvg6WZjP4K81cIZPWcHVHVecca+5PbL/\n\tRq43suf/SkEpMfJSd5FtWHsaNe0EcZZb3X33Joel8XtAacN4qNFiCk3U+a2tVwPN1/BD\n\tTFYFb4Gc3e5sawYiVSIKgkCfK1F4xsAKh8EczFKr2BVM/VAalFk56kywHO8lPAe1n1qE\n\txgHVpuj3Q/WLpDUhiOZ/9PBMBdG8rhjl4Ns4yh0aOvHsEiyo32Eh7PDbfCSeP6nRJzQe\n\tSP7w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"is8llaDu\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1695303890; x=1695908690;\n\th=to:subject:message-id:date:from:in-reply-to:references:mime-version\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=68SOf6hv3mVVUT5r64W6+ndW/vLVzjL4pATP7rA3kUY=;\n\tb=ABkUW/I1Qh2XMk5a8Og6fFfkUZO3WFLD8ww5qB033HpjTniw4zarYA8X8AxclPV3F2\n\tS08G+cIBLr1/ErATvutbBxROGK9VNXgnHnGE1pyhqhGlTSv4mYSOCin/fmI0ZnDW1JFj\n\tMQDQ+F67nZIe98Rs3FLOn+Ve1ZRnt1Gx2Dzd4pe/rcCR0Hp783mhe2nMjLMKeO9AT2tj\n\taYTJ17QSh8roxcvpNxaHlA6BVKNpnNXNujORym0DN16WIP8QnialwIeiPO1tIFIvNK4t\n\tntt0rGmccrqvKYSnP+moni5yzWPxLKb/PFsTcYD+wnEUSVKpclBROXdq+6UQp6GCB5HQ\n\th9Uw==","X-Gm-Message-State":"AOJu0YyNwM7g/yuNUvej7xkKUSNgEBW80SW9JpZ1P4Q0whmgNHetAjNG\n\tIPs7ScrCR2tvuI5bNhwb6fI1b0F3XyuoWJ5qgB4GuA==","X-Google-Smtp-Source":"AGHT+IF0Q9yWrFZVjNyNK7nlhHhQyebeyflHipWs1rnCOrQncW6m7XGJ6VXNl/Smd+a9MNv6E5bPjX0KEovYSpwrHrM=","X-Received":"by 2002:a17:90a:b78e:b0:26d:3d86:9a8e with SMTP id\n\tm14-20020a17090ab78e00b0026d3d869a8emr5830149pjr.25.1695303889689;\n\tThu, 21 Sep 2023 06:44:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>\n\t<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>","In-Reply-To":"<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>","Date":"Thu, 21 Sep 2023 14:44:38 +0100","Message-ID":"","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"00000000000021b5840605deb285\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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":"William Vinnicombe via libcamera-devel\n\t","Reply-To":"William Vinnicombe ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27835,"web_url":"https://patchwork.libcamera.org/comment/27835/","msgid":"<169530902223.2915094.16592105649605718872@ping.linuxembedded.co.uk>","date":"2023-09-21T15:10:22","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting William Vinnicombe (2023-09-21 14:44:38)\n> Hi Kieran,\n> \n> Thanks for your comments\n> \n> On Mon, 18 Sept 2023 at 11:41, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)\n> > > If the pipeline runs out of embedded data buffers, then it will pass\n> > > the frame to the IPA without the metadata. The IPA then has to use the\n> > > delayed controls as inputs to the algorithms. This can cause problems\n> > > with the subsequent algorithms if the sensor did not action the\n> > > controls, especially with the autofocus as that doesn't have controls\n> > > which can be passed in lieu of the metadata.\n> >\n> > I assume the implication here is that you've detected this happening at\n> > times ?\n> >\n> > > Reduce the likelihood of this by increasing the number of embedded data\n> > > buffers, as they are small so a generous number can be allocated.\n> > >\n> > > Signed-off-by: William Vinnicombe \n> > > ---\n> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > index 018cf488..6777c697 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera\n> > *camera)\n> > > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > > /*\n> > > * Embedded data buffers are (currently) for\n> > internal use,\n> > > - * so allocate the minimum required to avoid\n> > frame drops.\n> > > + * so allocate a generous number as they are\n> > small.\n> >\n> > Can we improve on 'Generous number' at all? What aspects can we consider\n> > here?\n> >\n> > The pipeline depth should be a known value (4 frames?) so doubling that\n> > would perhaps already be 'generous'?\n> >\n> > Or basing it on the number of raw stream buffers we may have available\n> > to queue to unicam? We can't ever capture embedded data without also\n> > capturing a raw image can we ? (or maybe we can? )\n> >\n> > \"\"\"\n> > * Embedded data buffers are (currently) for internal use, and are small\n> > * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid\n> > * causing problems in the IPA when we can not supply the metadata.\n> > *\n> > * We may have up to 6 raw buffers in the pipeline, so use double this\n> > * value to ensure we always have sufficient resources.\n> > \"\"\"\n> >\n> > (Of course I just made up the comment details so I don't expect it to be\n> > correct at all, just an example)\n> >\n> \n> 12 was selected as an arbitrary number, which is more than the number of\n> input buffers (which is application dependent, but is typically 8-10). As\n> they are much smaller than the image buffers (1-2kB rather than MB), it's\n> much simpler here to just set it to 12 rather than querying the number of\n> input buffers, as the extra memory used is small. If that explanation\n> sounds good, I can submit a v2 with the comment edited to that effect?\n\nDo we know how many application buffers there are? Is that a value we\ncan determine when we import buffers? or during configuration?\n\nWhat happens if I have an application which allocates 16 buffers, and\nruns at 120 FPS? Will 12 still be sufficient?\n\n--\nKieran\n\n\n\n> \n> \n> > > */\n> > > - numBuffers = minBuffers;\n> > > + numBuffers = 12;\n> >\n> > Normally we'd put constants like this into a constexpr too and define at\n> > the top of the file - but I think with a large comment like above that\n> > would be overkill here. Best to keep the code in one place in this\n> > instance IMO.\n> >\n> > > } else {\n> > > /*\n> > > * Since the ISP runs synchronous with the IPA\n> > and requests,\n> > > --\n> > > 2.39.2\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 D9D6FC326B\n\tfor ;\n\tThu, 21 Sep 2023 15:10:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB5D462944;\n\tThu, 21 Sep 2023 17:10:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF1B9628D8\n\tfor ;\n\tThu, 21 Sep 2023 17:10:24 +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 5AB06880E;\n\tThu, 21 Sep 2023 17:08:47 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695309026;\n\tbh=Wpjk+6WuFSnVo/382jKA25oLMzZTNlLminZ5U30qEWI=;\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:\n\tFrom;\n\tb=w8KOxxaRRuJ72W2ElJX+W8ga6jcm0OeQVHsUJm/lqrVhgxyIxmYZyVvE2l2z5GoFW\n\tg7vmeYsM5IABPg1e63YVrzim/oofYrbyhsZF/SCStYeKMV6BEq/EA5xcJgSyHOVj0x\n\tmTCKb4m0E+2oq2CsSYHx+8pBVGuwpd2Hz+mg6reWKbmel4UFa0MQoUpokpWaXQPX11\n\t5N+N+1cHqP0qbR7Ma3AKecz208QX0UwEC80BT+OR0oEbchCPmfXvWZLEIlmSNgKj3v\n\tTLzVC3J7SSGfUXe3U4gmU5oNsfUWhXM8zXKpiZ3LsRQP2CbsAXScw5NGIhGwio8jl1\n\tfDpiY6z0SLJcQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1695308927;\n\tbh=Wpjk+6WuFSnVo/382jKA25oLMzZTNlLminZ5U30qEWI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=VZBIrsG4RP11+47GVedojQGfYWxlq2PX4NOPxks4DXqsqBpp8HbErbI3vT2SPC58m\n\txwWHj04qWE9Qta7JkeE3N4O6QU0ggHgXdKJGGh8E1kwn9EHun1lXeayBjhPFR4I9KP\n\tftajtLcV1R++Lr/1d19b19fAvnEzmGQZpuHRfROg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VZBIrsG4\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>\n\t<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>\n\t","To":"William Vinnicombe ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 21 Sep 2023 16:10:22 +0100","Message-ID":"<169530902223.2915094.16592105649605718872@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27836,"web_url":"https://patchwork.libcamera.org/comment/27836/","msgid":"","date":"2023-09-21T15:14:40","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Thu, 21 Sept 2023 at 16:10, Kieran Bingham via libcamera-devel\n wrote:\n>\n> Quoting William Vinnicombe (2023-09-21 14:44:38)\n> > Hi Kieran,\n> >\n> > Thanks for your comments\n> >\n> > On Mon, 18 Sept 2023 at 11:41, Kieran Bingham <\n> > kieran.bingham@ideasonboard.com> wrote:\n> >\n> > > Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)\n> > > > If the pipeline runs out of embedded data buffers, then it will pass\n> > > > the frame to the IPA without the metadata. The IPA then has to use the\n> > > > delayed controls as inputs to the algorithms. This can cause problems\n> > > > with the subsequent algorithms if the sensor did not action the\n> > > > controls, especially with the autofocus as that doesn't have controls\n> > > > which can be passed in lieu of the metadata.\n> > >\n> > > I assume the implication here is that you've detected this happening at\n> > > times ?\n> > >\n> > > > Reduce the likelihood of this by increasing the number of embedded data\n> > > > buffers, as they are small so a generous number can be allocated.\n> > > >\n> > > > Signed-off-by: William Vinnicombe \n> > > > ---\n> > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > index 018cf488..6777c697 100644\n> > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera\n> > > *camera)\n> > > > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > > > /*\n> > > > * Embedded data buffers are (currently) for\n> > > internal use,\n> > > > - * so allocate the minimum required to avoid\n> > > frame drops.\n> > > > + * so allocate a generous number as they are\n> > > small.\n> > >\n> > > Can we improve on 'Generous number' at all? What aspects can we consider\n> > > here?\n> > >\n> > > The pipeline depth should be a known value (4 frames?) so doubling that\n> > > would perhaps already be 'generous'?\n> > >\n> > > Or basing it on the number of raw stream buffers we may have available\n> > > to queue to unicam? We can't ever capture embedded data without also\n> > > capturing a raw image can we ? (or maybe we can? )\n> > >\n> > > \"\"\"\n> > > * Embedded data buffers are (currently) for internal use, and are small\n> > > * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid\n> > > * causing problems in the IPA when we can not supply the metadata.\n> > > *\n> > > * We may have up to 6 raw buffers in the pipeline, so use double this\n> > > * value to ensure we always have sufficient resources.\n> > > \"\"\"\n> > >\n> > > (Of course I just made up the comment details so I don't expect it to be\n> > > correct at all, just an example)\n> > >\n> >\n> > 12 was selected as an arbitrary number, which is more than the number of\n> > input buffers (which is application dependent, but is typically 8-10). As\n> > they are much smaller than the image buffers (1-2kB rather than MB), it's\n> > much simpler here to just set it to 12 rather than querying the number of\n> > input buffers, as the extra memory used is small. If that explanation\n> > sounds good, I can submit a v2 with the comment edited to that effect?\n>\n> Do we know how many application buffers there are? Is that a value we\n> can determine when we import buffers? or during configuration?\n\nWe can (sort of), but it's not entirely trivial hence keeping it\nsimple with a constant of 12 buffers with this change.\n\n>\n> What happens if I have an application which allocates 16 buffers, and\n> runs at 120 FPS? Will 12 still be sufficient?\n\nIt should be sufficient even if we have > 12 RAW buffers. This is\nbecause the lifetime of the embedded buffers is much smaller than the\nRAW buffers and they will be recycled quicker. This is why the\ncurrent number of 4 worked for the most part, but not always at the\nfaster farmerates...\n\nRegards,\nNaush\n\n\n>\n> --\n> Kieran\n>\n>\n>\n> >\n> >\n> > > > */\n> > > > - numBuffers = minBuffers;\n> > > > + numBuffers = 12;\n> > >\n> > > Normally we'd put constants like this into a constexpr too and define at\n> > > the top of the file - but I think with a large comment like above that\n> > > would be overkill here. Best to keep the code in one place in this\n> > > instance IMO.\n> > >\n> > > > } else {\n> > > > /*\n> > > > * Since the ISP runs synchronous with the IPA\n> > > and requests,\n> > > > --\n> > > > 2.39.2\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 A9B8BBE080\n\tfor ;\n\tThu, 21 Sep 2023 15:15:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 164AD62931;\n\tThu, 21 Sep 2023 17:15:19 +0200 (CEST)","from mail-yw1-x112b.google.com (mail-yw1-x112b.google.com\n\t[IPv6:2607:f8b0:4864:20::112b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 605A6628D8\n\tfor ;\n\tThu, 21 Sep 2023 17:15:17 +0200 (CEST)","by mail-yw1-x112b.google.com with SMTP id\n\t00721157ae682-59ed7094255so13834257b3.3\n\tfor ;\n\tThu, 21 Sep 2023 08:15:17 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695309319;\n\tbh=3b4VOoQkfEVBoivQ4HDvh4qbZAUhQIHSfoYgdTObfRM=;\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=XXM8xdqgBrSbd+oXcJJKzjwSvxJxVyThDpk4v0uGrywzdd5T01nVdz0w0t6rVKfUv\n\t1eeM+j8YQl1IlZYWn5G8mBDpcujKeKTHfGwrrko8Q5WbEKcotD7vN1dKxfh84Hgu/T\n\tREyf9CwfIMmTljQV84MxuZGVnsjdCMR1j2cnuvwkCgoQP1DF/uNtdHzto86Ca3LHE+\n\tHN9AvnG1/kc8+0mwJrC87K87h5mE5E+4IoHc+cVXlFT4XLipfwm313+L13GoqVRez0\n\tj14LZ1x3u1PzTqQUf+WVl3FtpC07GNKA2Yg1DDvRlEE9IuXAbcR2ZPueo8FxceZ1x6\n\tFRP5xGSJrNONA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1695309316; x=1695914116;\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=Q8BlU917n2Vhz4jBe6w/GB+GQ3hrEct4BRxwrLtexL4=;\n\tb=BVHFsJZw+E/TSAjhvlJpssx0flqMDQ61hBFsomntgDBkNqHxFEe0QaDhdrFCCZSvht\n\tOQl1+AHO200QMpZ6mQCFTRIo6YZiVykut8LkTo2RRDi0f170P8hjOq6xhL5uwlIKas9B\n\tM9Gi8fw5vBRRaJqJ71nkbYUVfwDKURm1zFgs9KzyZE70vlPAUGrkd7E4+eYmM+5wikOJ\n\tWmKpccpAczqfLCrohWDoURmYaGyDZ4PGdJjantysOo7E40115iJw0Ckc4010VwcMr3MT\n\t+b88fxccfhkv/kSoh7T+xciKzIk+9b2ce+UsdxJrOy9oLySKIrVmm7RMZIzZla99bN5s\n\tFwjA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"BVHFsJZw\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1695309316; x=1695914116;\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=Q8BlU917n2Vhz4jBe6w/GB+GQ3hrEct4BRxwrLtexL4=;\n\tb=T0PnuDat0/1zyC+ByPY5iwvd2h+9qq3P698su2yzBM29+N9hzhn9mEVwKBrceCY7n2\n\tA6nFrjmmZE3NSQUvralRAJr6yHzssDOsaZMvq5ukCIYv5DDJbEGb/IBGbmgZECWAw+hb\n\tpHl6e7WNJDebl6Kbctsgz6DwROV0UqD25wjzMRcWiAgI3IXBGVIM6aTGBi06OBsUNGc/\n\tNSYpqQeSoTU1qtA2o186EseR4m5zH/0nlkQtOPGsD0VUj+uvXFzSeWcXjsQOl53YfLXG\n\tsLfQT6u+I9dbvYgkaUcQvV1dMrt8nWUhB2mocgweFX+qrYsknJmNAFTU9qBvBUZGWAqv\n\tQhNg==","X-Gm-Message-State":"AOJu0YzP9B6EXYLiqRYNhQwQbLqZygeJqqpTYa+3io4z1i0+M2OfOuGv\n\twxsjOSFJ1pE9pUHoPT+E0ayxS1nNvX9lLFIy6B+aDWDCJTxwjotkX3Q=","X-Google-Smtp-Source":"AGHT+IErY0xQMN8PpT7fq/6ujvAUkh/j0g0yMXuaggU3ZCTEOsW8Xp1x11i91/a/81LzLnfCfCG66P3QqWXlH//gvm4=","X-Received":"by 2002:a81:4707:0:b0:592:9035:8356 with SMTP id\n\tu7-20020a814707000000b0059290358356mr5571868ywa.26.1695309316206;\n\tThu, 21 Sep 2023 08:15:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>\n\t<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>\n\t\n\t<169530902223.2915094.16592105649605718872@ping.linuxembedded.co.uk>","In-Reply-To":"<169530902223.2915094.16592105649605718872@ping.linuxembedded.co.uk>","Date":"Thu, 21 Sep 2023 16:14:40 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27837,"web_url":"https://patchwork.libcamera.org/comment/27837/","msgid":"<169531163228.2915094.8263234780258603625@ping.linuxembedded.co.uk>","date":"2023-09-21T15:53:52","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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-21 16:14:40)\n> Hi Kieran,\n> \n> On Thu, 21 Sept 2023 at 16:10, Kieran Bingham via libcamera-devel\n> wrote:\n> >\n> > Quoting William Vinnicombe (2023-09-21 14:44:38)\n> > > Hi Kieran,\n> > >\n> > > Thanks for your comments\n> > >\n> > > On Mon, 18 Sept 2023 at 11:41, Kieran Bingham <\n> > > kieran.bingham@ideasonboard.com> wrote:\n> > >\n> > > > Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)\n> > > > > If the pipeline runs out of embedded data buffers, then it will pass\n> > > > > the frame to the IPA without the metadata. The IPA then has to use the\n> > > > > delayed controls as inputs to the algorithms. This can cause problems\n> > > > > with the subsequent algorithms if the sensor did not action the\n> > > > > controls, especially with the autofocus as that doesn't have controls\n> > > > > which can be passed in lieu of the metadata.\n> > > >\n> > > > I assume the implication here is that you've detected this happening at\n> > > > times ?\n> > > >\n> > > > > Reduce the likelihood of this by increasing the number of embedded data\n> > > > > buffers, as they are small so a generous number can be allocated.\n> > > > >\n> > > > > Signed-off-by: William Vinnicombe \n> > > > > ---\n> > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > index 018cf488..6777c697 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera\n> > > > *camera)\n> > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > > > > /*\n> > > > > * Embedded data buffers are (currently) for\n> > > > internal use,\n> > > > > - * so allocate the minimum required to avoid\n> > > > frame drops.\n> > > > > + * so allocate a generous number as they are\n> > > > small.\n> > > >\n> > > > Can we improve on 'Generous number' at all? What aspects can we consider\n> > > > here?\n> > > >\n> > > > The pipeline depth should be a known value (4 frames?) so doubling that\n> > > > would perhaps already be 'generous'?\n> > > >\n> > > > Or basing it on the number of raw stream buffers we may have available\n> > > > to queue to unicam? We can't ever capture embedded data without also\n> > > > capturing a raw image can we ? (or maybe we can? )\n> > > >\n> > > > \"\"\"\n> > > > * Embedded data buffers are (currently) for internal use, and are small\n> > > > * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid\n> > > > * causing problems in the IPA when we can not supply the metadata.\n> > > > *\n> > > > * We may have up to 6 raw buffers in the pipeline, so use double this\n> > > > * value to ensure we always have sufficient resources.\n> > > > \"\"\"\n> > > >\n> > > > (Of course I just made up the comment details so I don't expect it to be\n> > > > correct at all, just an example)\n> > > >\n> > >\n> > > 12 was selected as an arbitrary number, which is more than the number of\n> > > input buffers (which is application dependent, but is typically 8-10). As\n> > > they are much smaller than the image buffers (1-2kB rather than MB), it's\n> > > much simpler here to just set it to 12 rather than querying the number of\n> > > input buffers, as the extra memory used is small. If that explanation\n> > > sounds good, I can submit a v2 with the comment edited to that effect?\n> >\n> > Do we know how many application buffers there are? Is that a value we\n> > can determine when we import buffers? or during configuration?\n> \n> We can (sort of), but it's not entirely trivial hence keeping it\n> simple with a constant of 12 buffers with this change.\n> \n> >\n> > What happens if I have an application which allocates 16 buffers, and\n> > runs at 120 FPS? Will 12 still be sufficient?\n> \n> It should be sufficient even if we have > 12 RAW buffers. This is\n> because the lifetime of the embedded buffers is much smaller than the\n> RAW buffers and they will be recycled quicker. This is why the\n> current number of 4 worked for the most part, but not always at the\n> faster farmerates...\n\nCapturing the rationale that the lifetime of the embedded buffers is\nsmaller than the raw buffers sounds helpful to capture in the comment\ntoo then. That's what I was looking for!\n\n\n--\nKieran\n\n\n> \n> Regards,\n> Naush\n> \n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> >\n> > >\n> > >\n> > > > > */\n> > > > > - numBuffers = minBuffers;\n> > > > > + numBuffers = 12;\n> > > >\n> > > > Normally we'd put constants like this into a constexpr too and define at\n> > > > the top of the file - but I think with a large comment like above that\n> > > > would be overkill here. Best to keep the code in one place in this\n> > > > instance IMO.\n> > > >\n> > > > > } else {\n> > > > > /*\n> > > > > * Since the ISP runs synchronous with the IPA\n> > > > and requests,\n> > > > > --\n> > > > > 2.39.2\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 DB42FC326B\n\tfor ;\n\tThu, 21 Sep 2023 15:53:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 442F162944;\n\tThu, 21 Sep 2023 17:53:56 +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 5820E628D8\n\tfor ;\n\tThu, 21 Sep 2023 17:53:55 +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 B8025880E;\n\tThu, 21 Sep 2023 17:52:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695311636;\n\tbh=y8McwQQnphcci+ZX0s0AjtTX/f3SV42WTvJazROZ/rw=;\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=AlIgZGA0mQKE+dN/MsCtVhK3OEO396z8nWhE1V9MrYVdN86LP/1DMLg0RAbFHNiRC\n\tnDdzYLzbdTCEommZnjKvY/ib/Zv9ARcx02vOMM3XcH6ROSn/Ze5Ta/S9NdexZOmy3X\n\thSACU+zavLDOHVMFTiBYRnGEo0tInrd8r4VGVvt4lBvnQPS4gFT1fCEML3znSuKssx\n\tbD/h6bcWgTqTxlGC4cq9hNIx2SCRanXeDpyFHv827tdLO/Y2J+1ne3tKJXzkCRFuIp\n\t72VHcDJQGtDajliI2VPxYKHRz399gslP7PXiGB1fjGc/Nd9K/wsAV4ZWLI4hxv59NM\n\tzCWWXjGwOvxRw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1695311537;\n\tbh=y8McwQQnphcci+ZX0s0AjtTX/f3SV42WTvJazROZ/rw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=e0RNIMMJfkmSgM8ZCCg1fpUT3RnqToW3i7eS/R3CPD4peeNu5KjrT1moXgarLpasB\n\trs8XS9DWTHJXkfPAAMGEjrTDJaUOjqTP42Js7P1LUsHFCbWArHI6kPoulHOYMCbEyA\n\tspCVL5ae3vDGQarp9ewmDh3v8LxHWMrnpsRM/n/U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"e0RNIMMJ\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>\n\t<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>\n\t\n\t<169530902223.2915094.16592105649605718872@ping.linuxembedded.co.uk>\n\t","To":"Naushir Patuck ","Date":"Thu, 21 Sep 2023 16:53:52 +0100","Message-ID":"<169531163228.2915094.8263234780258603625@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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":27976,"web_url":"https://patchwork.libcamera.org/comment/27976/","msgid":"","date":"2023-10-18T12:16:40","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nOn Thu, 21 Sept 2023 at 16:53, Kieran Bingham\n wrote:\n>\n> Quoting Naushir Patuck (2023-09-21 16:14:40)\n> > Hi Kieran,\n> >\n> > On Thu, 21 Sept 2023 at 16:10, Kieran Bingham via libcamera-devel\n> > wrote:\n> > >\n> > > Quoting William Vinnicombe (2023-09-21 14:44:38)\n> > > > Hi Kieran,\n> > > >\n> > > > Thanks for your comments\n> > > >\n> > > > On Mon, 18 Sept 2023 at 11:41, Kieran Bingham <\n> > > > kieran.bingham@ideasonboard.com> wrote:\n> > > >\n> > > > > Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)\n> > > > > > If the pipeline runs out of embedded data buffers, then it will pass\n> > > > > > the frame to the IPA without the metadata. The IPA then has to use the\n> > > > > > delayed controls as inputs to the algorithms. This can cause problems\n> > > > > > with the subsequent algorithms if the sensor did not action the\n> > > > > > controls, especially with the autofocus as that doesn't have controls\n> > > > > > which can be passed in lieu of the metadata.\n> > > > >\n> > > > > I assume the implication here is that you've detected this happening at\n> > > > > times ?\n> > > > >\n> > > > > > Reduce the likelihood of this by increasing the number of embedded data\n> > > > > > buffers, as they are small so a generous number can be allocated.\n> > > > > >\n> > > > > > Signed-off-by: William Vinnicombe \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > index 018cf488..6777c697 100644\n> > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera\n> > > > > *camera)\n> > > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > > > > > /*\n> > > > > > * Embedded data buffers are (currently) for\n> > > > > internal use,\n> > > > > > - * so allocate the minimum required to avoid\n> > > > > frame drops.\n> > > > > > + * so allocate a generous number as they are\n> > > > > small.\n> > > > >\n> > > > > Can we improve on 'Generous number' at all? What aspects can we consider\n> > > > > here?\n> > > > >\n> > > > > The pipeline depth should be a known value (4 frames?) so doubling that\n> > > > > would perhaps already be 'generous'?\n> > > > >\n> > > > > Or basing it on the number of raw stream buffers we may have available\n> > > > > to queue to unicam? We can't ever capture embedded data without also\n> > > > > capturing a raw image can we ? (or maybe we can? )\n> > > > >\n> > > > > \"\"\"\n> > > > > * Embedded data buffers are (currently) for internal use, and are small\n> > > > > * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid\n> > > > > * causing problems in the IPA when we can not supply the metadata.\n> > > > > *\n> > > > > * We may have up to 6 raw buffers in the pipeline, so use double this\n> > > > > * value to ensure we always have sufficient resources.\n> > > > > \"\"\"\n> > > > >\n> > > > > (Of course I just made up the comment details so I don't expect it to be\n> > > > > correct at all, just an example)\n> > > > >\n> > > >\n> > > > 12 was selected as an arbitrary number, which is more than the number of\n> > > > input buffers (which is application dependent, but is typically 8-10). As\n> > > > they are much smaller than the image buffers (1-2kB rather than MB), it's\n> > > > much simpler here to just set it to 12 rather than querying the number of\n> > > > input buffers, as the extra memory used is small. If that explanation\n> > > > sounds good, I can submit a v2 with the comment edited to that effect?\n> > >\n> > > Do we know how many application buffers there are? Is that a value we\n> > > can determine when we import buffers? or during configuration?\n> >\n> > We can (sort of), but it's not entirely trivial hence keeping it\n> > simple with a constant of 12 buffers with this change.\n> >\n> > >\n> > > What happens if I have an application which allocates 16 buffers, and\n> > > runs at 120 FPS? Will 12 still be sufficient?\n> >\n> > It should be sufficient even if we have > 12 RAW buffers. This is\n> > because the lifetime of the embedded buffers is much smaller than the\n> > RAW buffers and they will be recycled quicker. This is why the\n> > current number of 4 worked for the most part, but not always at the\n> > faster farmerates...\n>\n> Capturing the rationale that the lifetime of the embedded buffers is\n> smaller than the raw buffers sounds helpful to capture in the comment\n> too then. That's what I was looking for!\n\nJust coming back to this one as it does not seem to be merged yet. Is\nit sufficient to update the commit message to get this merged, or do\nwe need anything more?\n\nRegards,\nNaush\n\n>\n>\n> --\n> Kieran\n>\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> > >\n> > > --\n> > > Kieran\n> > >\n> > >\n> > >\n> > > >\n> > > >\n> > > > > > */\n> > > > > > - numBuffers = minBuffers;\n> > > > > > + numBuffers = 12;\n> > > > >\n> > > > > Normally we'd put constants like this into a constexpr too and define at\n> > > > > the top of the file - but I think with a large comment like above that\n> > > > > would be overkill here. Best to keep the code in one place in this\n> > > > > instance IMO.\n> > > > >\n> > > > > > } else {\n> > > > > > /*\n> > > > > > * Since the ISP runs synchronous with the IPA\n> > > > > and requests,\n> > > > > > --\n> > > > > > 2.39.2\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 DB9AEC3272\n\tfor ;\n\tWed, 18 Oct 2023 12:17:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18FA86297F;\n\tWed, 18 Oct 2023 14:17:18 +0200 (CEST)","from mail-yw1-x1136.google.com (mail-yw1-x1136.google.com\n\t[IPv6:2607:f8b0:4864:20::1136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9FBF061DD1\n\tfor ;\n\tWed, 18 Oct 2023 14:17:16 +0200 (CEST)","by mail-yw1-x1136.google.com with SMTP id\n\t00721157ae682-5a7ac4c3666so82851647b3.3\n\tfor ;\n\tWed, 18 Oct 2023 05:17:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697631438;\n\tbh=L9dn+kbaXTiNlL/GPFHKGV1LNxOPssx0js3Gzkdj77E=;\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=f7lH1LACYCIwk575B6wIuol31CxQlkchSSGv5A0SsL43EuUI3CvtmHmx0f6ieK46B\n\taMllQDDi77vdqdCqiQILoysw0xTnLaQSZXO9rgshI8Cdu6aJHezDpBSsXB610wjkRd\n\tGnXLITNgisYqyDq4QyI//l5O969HLhHyYMMrjh/ZCScgO8YjDF7cOpQAAxoKAj+WJr\n\tjcFI58sgFFpg/gGr0KjgoVMf3APP9JHHdr8ZXsVh8eejy8FxiySym1WUAR9KNPPqj8\n\t65fXHgdYrGeedpqFZkUQCG14J/59vYc1V4Ymj6SYmekzjqu+9H0nkbk3d1VMvN5dLG\n\tLdO/Wazcc+bAA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1697631435; x=1698236235;\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=Keat+Xk+3NaurIfAPtm7E7Wh4/kgBadQ/+yAqIJCWPQ=;\n\tb=sapAabxq3MnuNZFszzPsGFun8UkiOu2hZEbcB1K1rV7C69Gt1Qy+wtGOQX/yofr2AU\n\tR/kx0fYULPgND3HLpePFbFmOn0exFC1M7Iq5rkjb26trxUxEBLw1dfWviKeYABlWa7Y+\n\tap2AV8VQiHoZFGe3gyFFyVZu7WjUt6S78d+wyy+dyW6eGMHJlPQfqgR3Z4dx3fSgYSg9\n\tslW+3sfiPhepfs3VoEpcHQKGcx+AHNQGiVNPCo8oAPyc3jNeYc0jlH+l6/NRD4ynvU7S\n\tIPeMpOdEFYw+tNMI8jcK7sh2FClrHlw4H/gmmOw79a9Q3NA8udfmRB+4ODIOvLnQl0Nv\n\tDGNA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sapAabxq\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1697631435; x=1698236235;\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=Keat+Xk+3NaurIfAPtm7E7Wh4/kgBadQ/+yAqIJCWPQ=;\n\tb=dWG1X2fdR9oqqnxc1UB5VL48l0hZuc+dI3GbyDJeegUDBw+X9pXogqk5nglv7UNpwH\n\taMPWjgaI2jVzvysR75lnOSjkSQ2VHfkSGkwjJcCyu9LCQt0qudLpNLRpjIPEnMgpgDhF\n\tpfKZ4oPC0JMdn08w0jifw2uRhbtD+6023I+rtZpr0x9X0prcEJrX0qdrUNPpB9zT7SD3\n\tTlxcDPg1+Y+mF3c7z0pOY4KbhM5oTX30Gh/RpBQHxcQY0eWQCj91/q1Kgueqe+yQwSzu\n\tSxbzpFsb+TKGB/AF4uA1kRCqbBq1Wf8jfEpQBZfMVaw5836chxVH6k5mFMoRsE8+CPog\n\tSVMw==","X-Gm-Message-State":"AOJu0Yx3zPzBSyOSyAdTs61gLKwRp5Pwq3wkXDKsNoNXls6I53v9wCIx\n\t8BTkTB6g4bPM2p/whmltnCLcH5jUUP6dZLQ0X5wnww==","X-Google-Smtp-Source":"AGHT+IGhYNcoJ/2Hq3iJOZC4+mqgmKOQqvUNzee6U/ndxej3yWuNcpziVQXQukVr7InI+Y27FEk9G0iJtvIG8t/m/RI=","X-Received":"by 2002:a25:8312:0:b0:d8b:1407:f61d with SMTP id\n\ts18-20020a258312000000b00d8b1407f61dmr4990154ybk.26.1697631435244;\n\tWed, 18 Oct 2023 05:17:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>\n\t<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>\n\t\n\t<169530902223.2915094.16592105649605718872@ping.linuxembedded.co.uk>\n\t\n\t<169531163228.2915094.8263234780258603625@ping.linuxembedded.co.uk>","In-Reply-To":"<169531163228.2915094.8263234780258603625@ping.linuxembedded.co.uk>","Date":"Wed, 18 Oct 2023 13:16:40 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27977,"web_url":"https://patchwork.libcamera.org/comment/27977/","msgid":"<169763261659.3049594.5176072266001710555@ping.linuxembedded.co.uk>","date":"2023-10-18T12:36:56","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2023-10-18 13:16:40)\n> Hi all,\n> \n> On Thu, 21 Sept 2023 at 16:53, Kieran Bingham\n> wrote:\n> >\n> > Quoting Naushir Patuck (2023-09-21 16:14:40)\n> > > Hi Kieran,\n> > >\n> > > On Thu, 21 Sept 2023 at 16:10, Kieran Bingham via libcamera-devel\n> > > wrote:\n> > > >\n> > > > Quoting William Vinnicombe (2023-09-21 14:44:38)\n> > > > > Hi Kieran,\n> > > > >\n> > > > > Thanks for your comments\n> > > > >\n> > > > > On Mon, 18 Sept 2023 at 11:41, Kieran Bingham <\n> > > > > kieran.bingham@ideasonboard.com> wrote:\n> > > > >\n> > > > > > Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)\n> > > > > > > If the pipeline runs out of embedded data buffers, then it will pass\n> > > > > > > the frame to the IPA without the metadata. The IPA then has to use the\n> > > > > > > delayed controls as inputs to the algorithms. This can cause problems\n> > > > > > > with the subsequent algorithms if the sensor did not action the\n> > > > > > > controls, especially with the autofocus as that doesn't have controls\n> > > > > > > which can be passed in lieu of the metadata.\n> > > > > >\n> > > > > > I assume the implication here is that you've detected this happening at\n> > > > > > times ?\n> > > > > >\n> > > > > > > Reduce the likelihood of this by increasing the number of embedded data\n> > > > > > > buffers, as they are small so a generous number can be allocated.\n> > > > > > >\n> > > > > > > Signed-off-by: William Vinnicombe \n> > > > > > > ---\n> > > > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--\n> > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > > index 018cf488..6777c697 100644\n> > > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > > @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera\n> > > > > > *camera)\n> > > > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > > > > > > /*\n> > > > > > > * Embedded data buffers are (currently) for\n> > > > > > internal use,\n> > > > > > > - * so allocate the minimum required to avoid\n> > > > > > frame drops.\n> > > > > > > + * so allocate a generous number as they are\n> > > > > > small.\n> > > > > >\n> > > > > > Can we improve on 'Generous number' at all? What aspects can we consider\n> > > > > > here?\n> > > > > >\n> > > > > > The pipeline depth should be a known value (4 frames?) so doubling that\n> > > > > > would perhaps already be 'generous'?\n> > > > > >\n> > > > > > Or basing it on the number of raw stream buffers we may have available\n> > > > > > to queue to unicam? We can't ever capture embedded data without also\n> > > > > > capturing a raw image can we ? (or maybe we can? )\n> > > > > >\n> > > > > > \"\"\"\n> > > > > > * Embedded data buffers are (currently) for internal use, and are small\n> > > > > > * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid\n> > > > > > * causing problems in the IPA when we can not supply the metadata.\n> > > > > > *\n> > > > > > * We may have up to 6 raw buffers in the pipeline, so use double this\n> > > > > > * value to ensure we always have sufficient resources.\n> > > > > > \"\"\"\n> > > > > >\n> > > > > > (Of course I just made up the comment details so I don't expect it to be\n> > > > > > correct at all, just an example)\n> > > > > >\n> > > > >\n> > > > > 12 was selected as an arbitrary number, which is more than the number of\n> > > > > input buffers (which is application dependent, but is typically 8-10). As\n> > > > > they are much smaller than the image buffers (1-2kB rather than MB), it's\n> > > > > much simpler here to just set it to 12 rather than querying the number of\n> > > > > input buffers, as the extra memory used is small. If that explanation\n> > > > > sounds good, I can submit a v2 with the comment edited to that effect?\n> > > >\n> > > > Do we know how many application buffers there are? Is that a value we\n> > > > can determine when we import buffers? or during configuration?\n> > >\n> > > We can (sort of), but it's not entirely trivial hence keeping it\n> > > simple with a constant of 12 buffers with this change.\n> > >\n> > > >\n> > > > What happens if I have an application which allocates 16 buffers, and\n> > > > runs at 120 FPS? Will 12 still be sufficient?\n> > >\n> > > It should be sufficient even if we have > 12 RAW buffers. This is\n> > > because the lifetime of the embedded buffers is much smaller than the\n> > > RAW buffers and they will be recycled quicker. This is why the\n> > > current number of 4 worked for the most part, but not always at the\n> > > faster farmerates...\n> >\n> > Capturing the rationale that the lifetime of the embedded buffers is\n> > smaller than the raw buffers sounds helpful to capture in the comment\n> > too then. That's what I was looking for!\n> \n> Just coming back to this one as it does not seem to be merged yet. Is\n> it sufficient to update the commit message to get this merged, or do\n> we need anything more?\n\nMy suggestion was to capture some of the rationale above into the comment\nwhere the value is used so that it's not just an opaque \"12 is a magic\nnumber here\".\n\nIf you'd rather leave this as is, let me know and I can merge. You've\nalready provided an RB tag.\n\n--\nKieran\n\n\n> \n> Regards,\n> Naush\n> \n> >\n> >\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > > >\n> > > > --\n> > > > Kieran\n> > > >\n> > > >\n> > > >\n> > > > >\n> > > > >\n> > > > > > > */\n> > > > > > > - numBuffers = minBuffers;\n> > > > > > > + numBuffers = 12;\n> > > > > >\n> > > > > > Normally we'd put constants like this into a constexpr too and define at\n> > > > > > the top of the file - but I think with a large comment like above that\n> > > > > > would be overkill here. Best to keep the code in one place in this\n> > > > > > instance IMO.\n> > > > > >\n> > > > > > > } else {\n> > > > > > > /*\n> > > > > > > * Since the ISP runs synchronous with the IPA\n> > > > > > and requests,\n> > > > > > > --\n> > > > > > > 2.39.2\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 F1EE2BDCBD\n\tfor ;\n\tWed, 18 Oct 2023 12:37:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 518C56297F;\n\tWed, 18 Oct 2023 14:37:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 768AB61DD1\n\tfor ;\n\tWed, 18 Oct 2023 14:36:59 +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 16DE15AA;\n\tWed, 18 Oct 2023 14:36:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697632620;\n\tbh=S19q4jgRxF03+3yDIdY+ZYrNXbyUG5V1SvSOnvTMi+4=;\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=oqAMadK3k+x5uRHgUbGKJ5huWRU+DZ5bcSRpOujOVqVyXtjNeZ1xdeOcGTWzoMwfK\n\tzplFm475oMAw6vFqoFoYK4qnlrzCvX7nfPbNlas+bFrhdGosgl/9pJob9fUoJ4S+O6\n\tyUrhxRT0MEir+qIy2g5QfSs2/OvJhLXrqwm9wXLaCJIoVDojC19100gkHirz1diZXI\n\t+nqEydtUW9lQBzRqlJecAdpbz1eh55IbHh6UzhjfRnEL2bxLjU+uQdT6qF7N/KvGbe\n\tZaYKtvZVKQ0E6CmU2Yrw6Jr/k7oCXI+hbXrIyKurd3Q+CiUBq2uQ9PGm+/MPmhNiw7\n\tSaTFCMN8Ly/9Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697632612;\n\tbh=S19q4jgRxF03+3yDIdY+ZYrNXbyUG5V1SvSOnvTMi+4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=qN7UOY5wOEyEgjpiv0wlC14qFE3sJDTVotzud2+w5Rm8gViYDA4ArnPT2Rb7iRbrd\n\t0n/a/knCLI2CZFsrIo9ePJ3hMlbzoS6u0DQTrygVQ6We+lg0vcm7O/Cyw/r7p4ujuW\n\tE4uWdb0gsFheYJkb/Sp1ncq79JNqgrOtVTUdW2u0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qN7UOY5w\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230918093016.22948-1-william.vinnicombe@raspberrypi.com>\n\t<169503365972.2427060.10650979731403390150@ping.linuxembedded.co.uk>\n\t\n\t<169530902223.2915094.16592105649605718872@ping.linuxembedded.co.uk>\n\t\n\t<169531163228.2915094.8263234780258603625@ping.linuxembedded.co.uk>\n\t","To":"Naushir Patuck ","Date":"Wed, 18 Oct 2023 13:36:56 +0100","Message-ID":"<169763261659.3049594.5176072266001710555@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more\n\tembedded data buffers","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\" "}}]