[{"id":4276,"web_url":"https://patchwork.libcamera.org/comment/4276/","msgid":"<20200325104154.GA19171@pendragon.ideasonboard.com>","date":"2020-03-25T10:41:54","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not\n\twait for param buffer it it's not used","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote:\n> If the parameter buffer is not filled in by the IPA and therefor not\n\ns/therefor/therefore/\n\n> used, mark it as already dequeued. If not a request without a parameter\n\ns/If not/Otherwise,/ (or \"If not,\")\n\n> buffer will never complete.\n\nI'm not sure to follow you there, parameters buffers are not provided in\nrequests. Is this about the IPA not providing a parameters buffer in\ntime ? Doesn't that introduce a bad race condition if we don't provide a\nparameters buffer for every request ?\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -351,12 +351,15 @@ protected:\n> \t\tif (!info)\n> \t\t\tLOG(RkISP1, Fatal) << \"Frame not known\";\n> \n> -\t\tif (info->paramFilled)\n> +\t\tif (info->paramFilled) {\n> \t\t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> -\t\telse\n> +\t\t} else {\n> +\t\t\t/* No need to dequeue param if it's never queued. */\n> +\t\t\tinfo->paramDequeued = true;\n> \t\t\tLOG(RkISP1, Error)\n> \t\t\t\t<< \"Parameters not ready on time for frame \"\n> \t\t\t\t<< frame() << \", ignore parameters.\";\n\nHow often does this occur ?\n\n> +\t\t}\n\nI think the logic should be simplified. Do we really have a need to\ndequeue the parameters buffer to complete the request ? It seems we\nshould be able to just dequeue them when they're signalled, and put them\nin a list of free parameters buffer for later use.\n\n> \n> \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> \t\tpipe_->video_->queueBuffer(info->videoBuffer);","headers":{"Return-Path":"","Received":["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 62A0060412\n\tfor ;\n\tWed, 25 Mar 2020 11:41:58 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 890E580C;\n\tWed, 25 Mar 2020 11:41:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cpAtCCcv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585132918;\n\tbh=N8MN6psckeru6fhjr1hZqR36KobqPvLw6I+sECELcEQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cpAtCCcvXCcY0SPfBQRKrWUMVaO7KQpyPi8dYKcTUk0b6YbOylK9VkT9jGXwIcmon\n\tv0/DtpRAs5itzi/+i0DsSHUSRpiRBHdxN3M4dz7jbpEIwA/6/4C5Sc+Le215xp2bsL\n\tHmmXnVkxh7QxIfZLhCTTHhRlmiwc7nnmOo5696JE=","Date":"Wed, 25 Mar 2020 12:41:54 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200325104154.GA19171@pendragon.ideasonboard.com>","References":"<20200325102355.3969245-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200325102355.3969245-1-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not\n\twait for param buffer it it's not used","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","X-List-Received-Date":"Wed, 25 Mar 2020 10:41:58 -0000"}},{"id":4282,"web_url":"https://patchwork.libcamera.org/comment/4282/","msgid":"<20200325120357.GA1042491@oden.dyn.berto.se>","date":"2020-03-25T12:03:57","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not\n\twait for param buffer it it's not used","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-03-25 12:41:54 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote:\n> > If the parameter buffer is not filled in by the IPA and therefor not\n> \n> s/therefor/therefore/\n> \n> > used, mark it as already dequeued. If not a request without a parameter\n> \n> s/If not/Otherwise,/ (or \"If not,\")\n> \n> > buffer will never complete.\n> \n> I'm not sure to follow you there, parameters buffers are not provided in\n> requests. Is this about the IPA not providing a parameters buffer in\n> time ? Doesn't that introduce a bad race condition if we don't provide a\n> parameters buffer for every request ?\n\nFor RkISP1 when a request is queued to the pipeline it contains no \nbuffers for stats or params. So the first thing that happens at this \npoint is to internally associate buffers for this and inform the IPA \nthat for frame # please use this param buffer.\n\nThen the timeline is used to schedule a point in time when this request \nneeds to be queued to hardware. When that time comes and the IPA have \nnot filled in the param buffer it should not be queued to hardware as we \ndon't know what parameters are in it. This is no problem for the \nhardware as it don't need a param buffer queued to operate. But as we \nhave already associated a param buffer with the request we need mark \nthat we shall not wait for it to dequeue as we never queue it to \nhardware.\n\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++--\n> > 1 file changed, 5 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -351,12 +351,15 @@ protected:\n> > \t\tif (!info)\n> > \t\t\tLOG(RkISP1, Fatal) << \"Frame not known\";\n> > \n> > -\t\tif (info->paramFilled)\n> > +\t\tif (info->paramFilled) {\n> > \t\t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> > -\t\telse\n> > +\t\t} else {\n> > +\t\t\t/* No need to dequeue param if it's never queued. */\n> > +\t\t\tinfo->paramDequeued = true;\n> > \t\t\tLOG(RkISP1, Error)\n> > \t\t\t\t<< \"Parameters not ready on time for frame \"\n> > \t\t\t\t<< frame() << \", ignore parameters.\";\n> \n> How often does this occur ?\n\nWithout the ipa thread series I have never seen it.\n\nWith the ipa thread series I don't see it for cam runs, but for qcam I \nsee them at least every other frame.\n\nIf I add the opengl rendering series ontop (fps ~3 -> 30) I see them for \nthe first ~10 frames and they are gone.\n\n> \n> > +\t\t}\n> \n> I think the logic should be simplified. Do we really have a need to\n> dequeue the parameters buffer to complete the request ? It seems we\n> should be able to just dequeue them when they're signalled, and put them\n> in a list of free parameters buffer for later use.\n\nI think the current design is good. It makes sure all resources \nassociated with a request when it enters the pipeline are available \nuntil the request is marked as completed and \"exits\" the pipeline \nhandler.\n\nI however think we should at some point create a helper class to track \nrequests and additional buffers added to it inside a pipeline handler, \nas we have a similar need in IPU3.\n\n> \n> > \n> > \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> > \t\tpipe_->video_->queueBuffer(info->videoBuffer);\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E32096193A\n\tfor ;\n\tWed, 25 Mar 2020 13:03:59 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id t16so727856lfl.2\n\tfor ;\n\tWed, 25 Mar 2020 05:03:59 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tu30sm2739450lfn.2.2020.03.25.05.03.57\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 25 Mar 2020 05:03:57 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"0yUJRMoo\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=TR44C0AMHXziv+607H1ce34YPzvnPx7BxiB8rV8hNGs=;\n\tb=0yUJRMooyklb3slq5Z1S+vmjDy6ZAUlIAwtTzoWn0OmJdaJhPZFcHKyo3WfCHBr+RU\n\tO0+Jwm4VSa76C7Aq962JppjNwuYrvHapJSTCB78VR/LymWWOYyvxBCrPZGqA6rxW4pvA\n\tcl4aH3fa9wGBszbMQasXlMd6ZnlE5xaCS/3bgRj0vLXZjc8ywUooJLagQLTS0sj1Z2/V\n\tAUbLZRdxd77jPlWHns/Llx9CpqAPrdDuc8H/op9vRwzHZrVb/KU9hTfAbBB2pUDHTi/s\n\t5vKTZetYgnpf+5uBE2bNm7+xA0KFLIsVgv+iKr2C03b/5HhbkVsEnS9Uilds/Kb0Cewv\n\tft4A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=TR44C0AMHXziv+607H1ce34YPzvnPx7BxiB8rV8hNGs=;\n\tb=AejfN9BljHC2yqZI9Q1wN2DuzzMuMCPf0UMa8H7zAp5g0HOjJJcZlUyq+YwKhx1weJ\n\tvqX99+kq8xZnAjiEOs8Sn4PIEj7rDGVPNs01utjqdUdJSjqu0m0GoiG67xPOTrHVqc5E\n\tCORQpTBUaHH/oIDwnLrUhePHV1jqxfZsPTstA+kHobgzktP0MfNlFcFJ5xzW84RVFKu2\n\tqGEyGji5eOM+2lf2aR+7HhUY6mcEQ9nAzaboDTwQPWnUmn5ELvUf6W65GBw4jVUkIKUq\n\twIq2chlOmdSsI4wUe+CpwgyClDVFZ5GP97ScN5hGFhDyg2644MHt+DzKj2MVx3Yl7zpK\n\tppRw==","X-Gm-Message-State":"ANhLgQ26fIIPcuYMta9uJOM5GOg3lqiVDYEQ2H8vweruhDXJGs51Urca\n\tQT/mIz7FSENzRFhKsXWhQq95sQ==","X-Google-Smtp-Source":"ADFU+vv3LdnPZR4NX5K1jZSRFbZZydjoGMKVhopgFgZO3rSr0hmqtFhTqRcPLE8X4DYWo/xlYQ3QxQ==","X-Received":"by 2002:a19:953:: with SMTP id 80mr2141345lfj.15.1585137839020; \n\tWed, 25 Mar 2020 05:03:59 -0700 (PDT)","Date":"Wed, 25 Mar 2020 13:03:57 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200325120357.GA1042491@oden.dyn.berto.se>","References":"<20200325102355.3969245-1-niklas.soderlund@ragnatech.se>\n\t<20200325104154.GA19171@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200325104154.GA19171@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not\n\twait for param buffer it it's not used","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","X-List-Received-Date":"Wed, 25 Mar 2020 12:04:00 -0000"}},{"id":4283,"web_url":"https://patchwork.libcamera.org/comment/4283/","msgid":"<20200325121403.GC19171@pendragon.ideasonboard.com>","date":"2020-03-25T12:14:03","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not\n\twait for param buffer it it's not used","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, Mar 25, 2020 at 01:03:57PM +0100, Niklas Söderlund wrote:\n> On 2020-03-25 12:41:54 +0200, Laurent Pinchart wrote:\n> > On Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote:\n> > > If the parameter buffer is not filled in by the IPA and therefor not\n> > \n> > s/therefor/therefore/\n> > \n> > > used, mark it as already dequeued. If not a request without a parameter\n> > \n> > s/If not/Otherwise,/ (or \"If not,\")\n> > \n> > > buffer will never complete.\n> > \n> > I'm not sure to follow you there, parameters buffers are not provided in\n> > requests. Is this about the IPA not providing a parameters buffer in\n> > time ? Doesn't that introduce a bad race condition if we don't provide a\n> > parameters buffer for every request ?\n> \n> For RkISP1 when a request is queued to the pipeline it contains no \n> buffers for stats or params. So the first thing that happens at this \n> point is to internally associate buffers for this and inform the IPA \n> that for frame # please use this param buffer.\n> \n> Then the timeline is used to schedule a point in time when this request \n> needs to be queued to hardware. When that time comes and the IPA have \n> not filled in the param buffer it should not be queued to hardware as we \n> don't know what parameters are in it. This is no problem for the \n> hardware as it don't need a param buffer queued to operate. But as we \n> have already associated a param buffer with the request we need mark \n> that we shall not wait for it to dequeue as we never queue it to \n> hardware.\n\nWhat happens if\n\n- request N is queued to the driver with no params buffer\n- request N+1 is queued to the driver with a param buffer\n- the driver executes request N\n\nWon't the driver take the params buffer of request N+1 for request N ?\n\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++--\n> > > 1 file changed, 5 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -351,12 +351,15 @@ protected:\n> > > \t\tif (!info)\n> > > \t\t\tLOG(RkISP1, Fatal) << \"Frame not known\";\n> > > \n> > > -\t\tif (info->paramFilled)\n> > > +\t\tif (info->paramFilled) {\n> > > \t\t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> > > -\t\telse\n> > > +\t\t} else {\n> > > +\t\t\t/* No need to dequeue param if it's never queued. */\n> > > +\t\t\tinfo->paramDequeued = true;\n> > > \t\t\tLOG(RkISP1, Error)\n> > > \t\t\t\t<< \"Parameters not ready on time for frame \"\n> > > \t\t\t\t<< frame() << \", ignore parameters.\";\n> > \n> > How often does this occur ?\n> \n> Without the ipa thread series I have never seen it.\n> \n> With the ipa thread series I don't see it for cam runs, but for qcam I \n> see them at least every other frame.\n> \n> If I add the opengl rendering series ontop (fps ~3 -> 30) I see them for \n> the first ~10 frames and they are gone.\n\nThat's still pretty bad though, it should be a very uncommon case.\n\n> > \n> > > +\t\t}\n> > \n> > I think the logic should be simplified. Do we really have a need to\n> > dequeue the parameters buffer to complete the request ? It seems we\n> > should be able to just dequeue them when they're signalled, and put them\n> > in a list of free parameters buffer for later use.\n> \n> I think the current design is good. It makes sure all resources \n> associated with a request when it enters the pipeline are available \n> until the request is marked as completed and \"exits\" the pipeline \n> handler.\n\nAcquiring a params buffer at the time the request is queued sounds good\nto me, but I don't see a reason to delay request completion until the\nparams buffer is dequeued, as it's not needed. This will just delay\nrequest completion for no real reason.\n\n> I however think we should at some point create a helper class to track \n> requests and additional buffers added to it inside a pipeline handler, \n> as we have a similar need in IPU3.\n> \n> > > \n> > > \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> > > \t\tpipe_->video_->queueBuffer(info->videoBuffer);","headers":{"Return-Path":"","Received":["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 6FF6F6193A\n\tfor ;\n\tWed, 25 Mar 2020 13:14:06 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B6DA280C;\n\tWed, 25 Mar 2020 13:14:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YsZ6umWa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585138446;\n\tbh=1b/u1HYwQIDnkwAuPTTROEfDojibmD1dtrePDsC9EFQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YsZ6umWaWgA2WbbjPzK0gNn923NABqzEvtTtdCI+KMqMoh6K1x7dyD2/Uz5+k6AcR\n\tRhT7mZa6T4J0mvboRn++ElBmBX/3c4AywrD1PYqkiYbucepVgE4H7RMIw0FhSo4fGQ\n\tCAhsiRcEMyqLWGQGHqz3oHjaoQgJ1iNZE5it0Yi8=","Date":"Wed, 25 Mar 2020 14:14:03 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200325121403.GC19171@pendragon.ideasonboard.com>","References":"<20200325102355.3969245-1-niklas.soderlund@ragnatech.se>\n\t<20200325104154.GA19171@pendragon.ideasonboard.com>\n\t<20200325120357.GA1042491@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200325120357.GA1042491@oden.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not\n\twait for param buffer it it's not used","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","X-List-Received-Date":"Wed, 25 Mar 2020 12:14:06 -0000"}}]