[{"id":16166,"web_url":"https://patchwork.libcamera.org/comment/16166/","msgid":"<20210410083109.qyxdzrrrlwwa57rf@uno.localdomain>","date":"2021-04-10T08:31:09","subject":"Re: [libcamera-devel] [RFC PATCH 3/3] libcamera: pipeline: rkisp1:\n\tMake fixed amount of internal buffer allocation more explicit","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\nOn Fri, Apr 09, 2021 at 06:38:15PM -0300, Nícolas F. R. A. Prado wrote:\n> Now that bufferCount can be increased and there is a lc-compliance test\n> in place to check if the pipeline can handle more requests than it has\n> internal buffers, we need to be able to test it.\n>\n> Make the internal buffer allocation always constant, so that by\n> increasing bufferCount we can simulate this scenario. Scaling the\n> internal buffers amount with bufferCount only hides the issue, that\n> should be handled by queueing the overflowing requests.\n>\n\nNot a 100% sure I got it: is this patch meant to trigger an error just\nfor the purpose of testing ?\n\nThanks\n j\n\n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++-------\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++--\n> 2 files changed, 4 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c7b93b2804c7..0dc474f343fc 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -685,16 +685,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> \tunsigned int ipaBufferId = 1;\n> \tint ret;\n>\n> -\tunsigned int maxCount = std::max({\n> -\t\tdata->mainPathStream_.configuration().bufferCount,\n> -\t\tdata->selfPathStream_.configuration().bufferCount,\n> -\t});\n> -\n> -\tret = param_->allocateBuffers(maxCount, ¶mBuffers_);\n> +\tret = param_->allocateBuffers(RKISP1_MIN_BUFFER_COUNT, ¶mBuffers_);\n> \tif (ret < 0)\n> \t\tgoto error;\n>\n> -\tret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> +\tret = stat_->allocateBuffers(RKISP1_MIN_BUFFER_COUNT, &statBuffers_);\n> \tif (ret < 0)\n> \t\tgoto error;\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 13291da89dc5..4ab4c0516ebc 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -21,6 +21,8 @@\n>\n> namespace libcamera {\n>\n> +static constexpr unsigned int RKISP1_MIN_BUFFER_COUNT = 4;\n> +\n> class MediaDevice;\n> class V4L2Subdevice;\n> struct StreamConfiguration;\n> @@ -56,8 +58,6 @@ public:\n> \tSignal &bufferReady() { return video_->bufferReady; }\n>\n> private:\n> -\tstatic constexpr unsigned int RKISP1_MIN_BUFFER_COUNT = 4;\n> -\n> \tconst char *name_;\n> \tbool running_;\n>\n> --\n> 2.31.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 C165EBD1F6\n\tfor ;\n\tSat, 10 Apr 2021 08:30:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26DB7687F8;\n\tSat, 10 Apr 2021 10:30:36 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 76331687EC\n\tfor ;\n\tSat, 10 Apr 2021 10:30:34 +0200 (CEST)","from uno.localdomain (host-82-57-193-192.retail.telecomitalia.it\n\t[82.57.193.192]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 46D9640005;\n\tSat, 10 Apr 2021 08:30:32 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Sat, 10 Apr 2021 10:31:09 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210410083109.qyxdzrrrlwwa57rf@uno.localdomain>","References":"<20210409213815.356837-1-nfraprado@collabora.com>\n\t<20210409213815.356837-4-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210409213815.356837-4-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 3/3] libcamera: pipeline: rkisp1:\n\tMake fixed amount of internal buffer allocation more explicit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org,\n\tCollabora Kernel ML ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16185,"web_url":"https://patchwork.libcamera.org/comment/16185/","msgid":"","date":"2021-04-12T13:45:17","subject":"Re: [libcamera-devel] [RFC PATCH 3/3] libcamera: pipeline: rkisp1:\n\tMake fixed amount of internal buffer allocation more explicit","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Em 2021-04-10 05:31, Jacopo Mondi escreveu:\n\n> Hi Nicolas,\n>\n> On Fri, Apr 09, 2021 at 06:38:15PM -0300, Nícolas F. R. A. Prado wrote:\n> > Now that bufferCount can be increased and there is a lc-compliance test\n> > in place to check if the pipeline can handle more requests than it has\n> > internal buffers, we need to be able to test it.\n> >\n> > Make the internal buffer allocation always constant, so that by\n> > increasing bufferCount we can simulate this scenario. Scaling the\n> > internal buffers amount with bufferCount only hides the issue, that\n> > should be handled by queueing the overflowing requests.\n> >\n>\n> Not a 100% sure I got it: is this patch meant to trigger an error just\n> for the purpose of testing ?\n\nMore or less, yes. I think actually that this is something that would make sense\nto merge only after other changes, but I sent it as a discussion starter.\n\nBefore this patch, a buffer overrun can happen on the internal buffer only if\nthe user allocates the buffers without using the FrameBufferAllocator. After the\npatch, that can also happen using the FrameBufferAllocator if the bufferCount\nvalue configured is above RKISP1_MIN_BUFFER_COUNT. So the way this is, it\nactually increases the scenarios of failure, although in a way that is tested by\nthe new lc-compliance test.\n\nFrom my discussion with Laurent on IRC, my understanding is that the issue\nshould be fixed by queuing the requests until there are free internal buffers\nrather than allocating more of them. Additionally, it seems the idea in the\nfuture is to remove bufferCount. With that in mind it makes sense to me to have\na fixed amount of internal buffers and only rely on queuing the overflowing\nrequests internally. But that also probably means that this change should be\nmade after the internal request queueing is implemented.\n\nI think a change like this would make a lot more sense right now on the IPU3\npipeline since that already has a patch for solving the issue in the works [1],\nbut I made it for the rkisp1 since it's what I have and can test on.\n\nThanks,\nNícolas\n\n[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019108.html","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 A16E7BD224\n\tfor ;\n\tMon, 12 Apr 2021 14:09:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65235687F3;\n\tMon, 12 Apr 2021 16:09:53 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E83BF602C8\n\tfor ;\n\tMon, 12 Apr 2021 16:09:50 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:77f9:56be:21f2:e14e])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 3B8C21F4519F;\n\tMon, 12 Apr 2021 15:09:39 +0100 (BST)"],"Mime-Version":"1.0","To":"\"Jacopo Mondi\" ","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","Date":"Mon, 12 Apr 2021 10:45:17 -0300","Message-Id":"","In-Reply-To":"<20210410083109.qyxdzrrrlwwa57rf@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 3/3] libcamera: pipeline: rkisp1:\n\tMake fixed amount of internal buffer allocation more explicit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org,\n\tCollabora Kernel ML ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]