[{"id":25977,"web_url":"https://patchwork.libcamera.org/comment/25977/","msgid":"","date":"2022-12-02T13:44:06","subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote:\n> If the pipeline handler is required to drop startup frames by the IPA, do not\n> call StreamOn on the ISP Output0 and Output1 device nodes from\n> PipelineHandlerRPi::start. This stops the ISP from generating output on those\n> channels giving improving latency, and more crucially does not require internal\n> buffers to be allocated to deal with this condition.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/data/default.json | 2 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---\n> 2 files changed, 31 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n> index d709e31850af..a7ea735c87f4 100644\n> --- a/src/libcamera/pipeline/raspberrypi/data/default.json\n> +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\n> @@ -14,7 +14,7 @@\n> # min_total_unicam_buffers - external buffer count)\n> \"min_total_unicam_buffers\": 4,\n> \n> - # The number of internal buffers used for ISP Output0. This must be set to 1.\n> + # The number of internal buffers used for ISP Output0.\n\nCan you explain this change in the commit message ?\n\n> \"num_output0_buffers\": 1\n> }\n> }\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index ce411453db0a..86eb43a3a3c5 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> \n> \tdata->state_ = RPiCameraData::State::Idle;\n> \n> -\t/* Start all streams. */\n> +\t/*\n> +\t * Start all streams with the exception of ISP Output0 and Output1 if\n> +\t * we are dropping some start-up frames. This saves a tiny bit of latency\n> +\t * and avoids the need for allocating an Output0 buffer only to handle\n> +\t * startup drop frame conditions.\n> +\t */\n\nI'm not sure to follow you here. Wouldn't it be sufficient to drop the\nbuffers at unicam's output without pushing them to the ISP ? Why does\ndelaying streamOn() help ?\n\n> \tfor (auto const stream : data->streams_) {\n> +\t\tif (data->dropFrameCount_ &&\n> +\t\t (stream == &data->isp_[Isp::Output0] ||\n> +\t\t stream == &data->isp_[Isp::Output1]))\n> +\t\t\tcontinue;\n> +\n> \t\tret = stream->dev()->streamOn();\n> \t\tif (ret) {\n> \t\t\tstop(camera);\n> @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> \t\t\tif (ret < 0)\n> \t\t\t\treturn ret;\n> \t\t} else {\n> +\t\t\t/*\n> +\t\t\t * We don't enable streaming for Output0 and Output1 for\n> +\t\t\t * startup frame drops, so don't queue any buffers.\n> +\t\t\t */\n> +\t\t\tif (stream == &data->isp_[Isp::Output0] ||\n> +\t\t\t stream == &data->isp_[Isp::Output1])\n> +\t\t\t\tcontinue;\n> \t\t\t/*\n> \t\t\t * For external streams, we must queue up a set of internal\n> \t\t\t * buffers to handle the number of drop frames requested by\n> @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()\n> \t}\n> \n> \t/*\n> -\t * Make sure we have three outputs completed in the case of a dropped\n> -\t * frame.\n> +\t * Only the ISP statistics output is generated when we are dropping\n> +\t * frames on startup.\n> \t */\n> \tif (state_ == State::IpaComplete &&\n> -\t ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {\n> +\t ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) {\n> \t\tstate_ = State::Idle;\n> \t\tif (dropFrameCount_) {\n> \t\t\tdropFrameCount_--;\n> \t\t\tLOG(RPI, Debug) << \"Dropping frame at the request of the IPA (\"\n> \t\t\t\t\t<< dropFrameCount_ << \" left)\";\n> +\t\t\t/*\n> +\t\t\t * If we have finished dropping startup frames, start\n> +\t\t\t * streaming on the ISP Output0 and Output1 nodes for\n> +\t\t\t * normal operation.\n> +\t\t\t */\n> +\t\t\tif (!dropFrameCount_) {\n> +\t\t\t\tisp_[Isp::Output0].dev()->streamOn();\n> +\t\t\t\tisp_[Isp::Output1].dev()->streamOn();\n> +\t\t\t}\n> \t\t}\n> \t}\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 B3F3EC3284\n\tfor ;\n\tFri, 2 Dec 2022 13:44:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D14EB60483;\n\tFri, 2 Dec 2022 14:44:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF8B260483\n\tfor ;\n\tFri, 2 Dec 2022 14:44:08 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3BC476E0;\n\tFri, 2 Dec 2022 14:44:08 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669988649;\n\tbh=YsWHpw6EdIEZ2cf5lkXJU0jT4eHaKQs40Z5UXIs0gyg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=tYXoKpPbm5kATlgohNBrJULEF3S422WW9Y3OvwpiLfAwtm2oX88TKEON6arNj0zfB\n\tlxhPDbVHx3zYlv19pZewD1zVwj5wvkLv4dRsMbH/B3L+7epEkji42/TjCSJlXYxTTl\n\tALC0aPpKGB7FMd2ZxOBffyxS/Ic7TidVrrBxnCUBDi0VSdhVdgWT0GD/XqHbHIFUg5\n\tC9m6SBzQAKlV88308haQw/Jwr8Z3eobKV2mVtj/VfxG9HfxkweR+kXihUj7nBc2JXT\n\toYLP03epOG5PPnysn+LYAMIQbplP5PAPpjqQ7Mic8d/gAPi3w/my7R1VJytOwxppJm\n\tCd1Ik71FKgW1A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669988648;\n\tbh=YsWHpw6EdIEZ2cf5lkXJU0jT4eHaKQs40Z5UXIs0gyg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lhcO83H+/WpXM2MKWbsk5mYlds68QgLcckg0x1QvrXD/iyUqVfpVKPd6n2z1ENWBP\n\t0s9B4s77NVhizeowyj5+p5wZ0zNp3V21UtiXebJhevXmLi8fs9gCaVjEtUlgVNMEWK\n\t6oV9qM0F7pJCsJsDoHHyZpjBH2YJbjacT8nsusWU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lhcO83H+\"; dkim-atps=neutral","Date":"Fri, 2 Dec 2022 15:44:06 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-6-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221129134534.2933-6-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25979,"web_url":"https://patchwork.libcamera.org/comment/25979/","msgid":"","date":"2022-12-02T13:52:08","subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Fri, 2 Dec 2022 at 13:44, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > If the pipeline handler is required to drop startup frames by the IPA,\n> do not\n> > call StreamOn on the ISP Output0 and Output1 device nodes from\n> > PipelineHandlerRPi::start. This stops the ISP from generating output on\n> those\n> > channels giving improving latency, and more crucially does not require\n> internal\n> > buffers to be allocated to deal with this condition.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/data/default.json | 2 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---\n> > 2 files changed, 31 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json\n> b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > index d709e31850af..a7ea735c87f4 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/data/default.json\n> > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > @@ -14,7 +14,7 @@\n> > # min_total_unicam_buffers\n> - external buffer count)\n> > \"min_total_unicam_buffers\": 4,\n> >\n> > - # The number of internal buffers used for ISP Output0.\n> This must be set to 1.\n> > + # The number of internal buffers used for ISP Output0.\n>\n> Can you explain this change in the commit message ?\n>\n> > \"num_output0_buffers\": 1\n> > }\n> > }\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index ce411453db0a..86eb43a3a3c5 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera,\n> const ControlList *controls)\n> >\n> > data->state_ = RPiCameraData::State::Idle;\n> >\n> > - /* Start all streams. */\n> > + /*\n> > + * Start all streams with the exception of ISP Output0 and Output1\n> if\n> > + * we are dropping some start-up frames. This saves a tiny bit of\n> latency\n> > + * and avoids the need for allocating an Output0 buffer only to\n> handle\n> > + * startup drop frame conditions.\n> > + */\n>\n> I'm not sure to follow you here. Wouldn't it be sufficient to drop the\n> buffers at unicam's output without pushing them to the ISP ? Why does\n> delaying streamOn() help ?\n>\n\nYou caught me :-)\n\nI would have done exactly this, only our FW interface does not work that\nway.\nIf the kernel device node is opened, we must provide it with an Output0\nbuffer\nto function correctly. This is purely down to the FW and not the kernel\ndriver.\nGiven the fragile nature of the FW, I don't want to make any large scale\nchanges to fix this in FW land.\n\nNaush\n\n\n\n>\n> > for (auto const stream : data->streams_) {\n> > + if (data->dropFrameCount_ &&\n> > + (stream == &data->isp_[Isp::Output0] ||\n> > + stream == &data->isp_[Isp::Output1]))\n> > + continue;\n> > +\n> > ret = stream->dev()->streamOn();\n> > if (ret) {\n> > stop(camera);\n> > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera\n> *camera)\n> > if (ret < 0)\n> > return ret;\n> > } else {\n> > + /*\n> > + * We don't enable streaming for Output0 and\n> Output1 for\n> > + * startup frame drops, so don't queue any buffers.\n> > + */\n> > + if (stream == &data->isp_[Isp::Output0] ||\n> > + stream == &data->isp_[Isp::Output1])\n> > + continue;\n> > /*\n> > * For external streams, we must queue up a set of\n> internal\n> > * buffers to handle the number of drop frames\n> requested by\n> > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()\n> > }\n> >\n> > /*\n> > - * Make sure we have three outputs completed in the case of a\n> dropped\n> > - * frame.\n> > + * Only the ISP statistics output is generated when we are dropping\n> > + * frames on startup.\n> > */\n> > if (state_ == State::IpaComplete &&\n> > - ((ispOutputCount_ == 3 && dropFrameCount_) ||\n> requestCompleted)) {\n> > + ((ispOutputCount_ == 1 && dropFrameCount_) ||\n> requestCompleted)) {\n> > state_ = State::Idle;\n> > if (dropFrameCount_) {\n> > dropFrameCount_--;\n> > LOG(RPI, Debug) << \"Dropping frame at the request\n> of the IPA (\"\n> > << dropFrameCount_ << \" left)\";\n> > + /*\n> > + * If we have finished dropping startup frames,\n> start\n> > + * streaming on the ISP Output0 and Output1 nodes\n> for\n> > + * normal operation.\n> > + */\n> > + if (!dropFrameCount_) {\n> > + isp_[Isp::Output0].dev()->streamOn();\n> > + isp_[Isp::Output1].dev()->streamOn();\n> > + }\n> > }\n> > }\n> > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0BD6FBDE6B\n\tfor ;\n\tFri, 2 Dec 2022 13:52:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6DED16333F;\n\tFri, 2 Dec 2022 14:52:26 +0100 (CET)","from mail-io1-xd33.google.com (mail-io1-xd33.google.com\n\t[IPv6:2607:f8b0:4864:20::d33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1B5A60483\n\tfor ;\n\tFri, 2 Dec 2022 14:52:24 +0100 (CET)","by mail-io1-xd33.google.com with SMTP id c7so3069787iof.13\n\tfor ;\n\tFri, 02 Dec 2022 05:52:24 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669989146;\n\tbh=fJRIQyCf7dWuSd5XrubxkhNKwvj/HMjQhXjqKoNp4vU=;\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=AJc7mjKeeUUoXNafEVmG3CPsb7xIKoLZEoxFzo87HLpnHbHWIFCifmF8bNRiT0q5m\n\tXoSd5470Gf954ti6ZbfARqTjNwPwre8oT5k8FlF9DqHU3AButFQ/J5po0F8y4nOdzv\n\tAKXJBSgqGgATjT4RrjwYVAkf0gapTnMQgkS0aID8NhU5fb/fYXqP95fR8vYluCstYD\n\t4Uw2gXfyBA2Mz1Zcms0/NC0s2wKqN7mtAuhqcpDRd6KqUXC4PnnnFvVIQJlT00080z\n\tZOkMA+VbN58dPHTNcCjES+yzn7OgO7g38neCGJ3bMxdTfzivt1Z1oX5vmdpgjN+N4a\n\tlAXXGyCv+kBxg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=cU1Z0LDroMcsKBFI8RxEl3Ny0Re0r5LkDf/fOxqeLaw=;\n\tb=ZOmX+OeMNKcnjIdw0hdwUtPpHlHt28110qgrndEkV9wlf6+m/SHf8j7Jm6ravkCakc\n\t7x8PygP7wym1OPnnBAGjtjSIZYUAH0JXr1xnU3VNrD39w6lJDjM6cSekbuB8/GAnM5t0\n\tC21Bu8/Ygo3tBeeII28ExiGBi7NbXD8J+B1fqC+LqJ1VH+AdaUepq3j74n5yGBSTHox2\n\tkkxeJMDrSZKv5OQr6osOkLCTDoI5LLcM1mko28WOqQAPwrUEbwCP88uSivYTlTpAY3hF\n\t4pIaYxXebwkLUmsQ9Sd5HYDc9WI/XrOEiCOl+xYX9gra7uICYHQRonLrGRYstwcWkk8m\n\tzc5g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ZOmX+OeM\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=cU1Z0LDroMcsKBFI8RxEl3Ny0Re0r5LkDf/fOxqeLaw=;\n\tb=Tl6zR1JJyvrGjGlYetyZkRUIW8YElK/zPQx239nj3qx0ufZWJx+7ohYUPyjoLKg9Ut\n\tHRyPxtjfG5OL12c4eY9g2bKWKiWpnInXMhZZZj8vWFDD1vxgLBdr/nC7yUDUBPuxYRPX\n\t38cE3nDrykPy+b7HZv/diS3PS5B7XNrAIzZe4sdXlUuU+XmhQhd7Dibsz88hijowTI0x\n\tIUIwgluZh3VDHXWg6XFliFQ2XUymZK5eOItSiB3ptXjAYZQZ7a9oLDhnvEIT0GAZ/0HS\n\tKuNDYUyBeMh/7tccElfVwHlRs4E7gkdq6U3xeMWBWE20J+74Mxul+Kk6xcy7kVMQBejV\n\tWG0A==","X-Gm-Message-State":"ANoB5plEVnQN74XkdctmNenZrICHHUC9oKrw1GUnhm/mF9iCMZLFLDut\n\tHPgTsjRINw6u5eMYk1g5NG1HsinxL+VifviN3AfH4rxLWBz2CQ==","X-Google-Smtp-Source":"AA0mqf6qhhimMjf8t0d9moVBxnp1TjL6zhSAV/7AEzU8qlcblM2WwaKGS6Zxf7MJ5OeF0Rs5lpDNT4Da9a7kfIRO9Ug=","X-Received":"by 2002:a02:6a43:0:b0:375:4725:4b4f with SMTP id\n\tm3-20020a026a43000000b0037547254b4fmr33238066jaf.52.1669989143409;\n\tFri, 02 Dec 2022 05:52:23 -0800 (PST)","MIME-Version":"1.0","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Fri, 2 Dec 2022 13:52:08 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000ac0fc105eed8a56f\"","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","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":25990,"web_url":"https://patchwork.libcamera.org/comment/25990/","msgid":"","date":"2022-12-02T20:06:36","subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote:\n> On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote:\n> > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > > If the pipeline handler is required to drop startup frames by the IPA, do not\n> > > call StreamOn on the ISP Output0 and Output1 device nodes from\n> > > PipelineHandlerRPi::start. This stops the ISP from generating output on those\n> > > channels giving improving latency, and more crucially does not require internal\n> > > buffers to be allocated to deal with this condition.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > .../pipeline/raspberrypi/data/default.json | 2 +-\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---\n> > > 2 files changed, 31 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > index d709e31850af..a7ea735c87f4 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > @@ -14,7 +14,7 @@\n> > > # min_total_unicam_buffers - external buffer count)\n> > > \"min_total_unicam_buffers\": 4,\n> > >\n> > > - # The number of internal buffers used for ISP Output0. This must be set to 1.\n> > > + # The number of internal buffers used for ISP Output0.\n> >\n> > Can you explain this change in the commit message ?\n> >\n> > > \"num_output0_buffers\": 1\n> > > }\n> > > }\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index ce411453db0a..86eb43a3a3c5 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > >\n> > > data->state_ = RPiCameraData::State::Idle;\n> > >\n> > > - /* Start all streams. */\n> > > + /*\n> > > + * Start all streams with the exception of ISP Output0 and Output1 if\n> > > + * we are dropping some start-up frames. This saves a tiny bit of latency\n\nWhat latency does this save, and won't we repay that later anyway ?\n\n> > > + * and avoids the need for allocating an Output0 buffer only to handle\n> > > + * startup drop frame conditions.\n> > > + */\n> >\n> > I'm not sure to follow you here. Wouldn't it be sufficient to drop the\n> > buffers at unicam's output without pushing them to the ISP ? Why does\n> > delaying streamOn() help ?\n> \n> You caught me :-)\n> \n> I would have done exactly this, only our FW interface does not work that way.\n> If the kernel device node is opened, we must provide it with an Output0 buffer\n> to function correctly.\n\nThis patch addresses VIDIOC_STREAMON, do you then mean if the video\ndevice node is streaming, not if it is open ?\n\n> This is purely down to the FW and not the kernel driver.\n> Given the fragile nature of the FW, I don't want to make any large scale\n> changes to fix this in FW land.\n\nSometimes I think it could make things easier if the firmware was\nopen-source ;-)\n\nMore seriously, it seems this could be addressed in kernelspace, by\ndelaying the vchiq_mmal_port_enable() call until the first buffer is\nqueued, which may be as easy as setting vb2_queue.min_buffers_needed to\n1. A quick look at videobuf2 doesn't show any immediate ill side\neffects, but I may have overlooked something. I also understand that\nchanging the kernel would be more complex (as it would need to be synced\nwith merging this series), and thinking more about it, it may be a bit\nof a hack as it would only work as expect for the startup frame drop.\n\n> > > for (auto const stream : data->streams_) {\n> > > + if (data->dropFrameCount_ &&\n> > > + (stream == &data->isp_[Isp::Output0] ||\n> > > + stream == &data->isp_[Isp::Output1]))\n> > > + continue;\n> > > +\n> > > ret = stream->dev()->streamOn();\n> > > if (ret) {\n> > > stop(camera);\n> > > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > if (ret < 0)\n> > > return ret;\n> > > } else {\n> > > + /*\n> > > + * We don't enable streaming for Output0 and Output1 for\n\nMaybe \"We haven't enabled streaming yet\" to make this clearer ?\n\n> > > + * startup frame drops, so don't queue any buffers.\n> > > + */\n> > > + if (stream == &data->isp_[Isp::Output0] ||\n> > > + stream == &data->isp_[Isp::Output1])\n> > > + continue;\n\nI'd add a blank line here.\n\n> > > /*\n> > > * For external streams, we must queue up a set of internal\n> > > * buffers to handle the number of drop frames requested by\n> > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()\n> > > }\n> > >\n> > > /*\n> > > - * Make sure we have three outputs completed in the case of a dropped\n> > > - * frame.\n> > > + * Only the ISP statistics output is generated when we are dropping\n> > > + * frames on startup.\n> > > */\n> > > if (state_ == State::IpaComplete &&\n> > > - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {\n> > > + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) {\n> > > state_ = State::Idle;\n> > > if (dropFrameCount_) {\n> > > dropFrameCount_--;\n> > > LOG(RPI, Debug) << \"Dropping frame at the request of the IPA (\"\n> > > << dropFrameCount_ << \" left)\";\n\nHere too.\n\n> > > + /*\n> > > + * If we have finished dropping startup frames, start\n> > > + * streaming on the ISP Output0 and Output1 nodes for\n> > > + * normal operation.\n> > > + */\n> > > + if (!dropFrameCount_) {\n> > > + isp_[Isp::Output0].dev()->streamOn();\n> > > + isp_[Isp::Output1].dev()->streamOn();\n> > > + }\n\nThis looks good.\n\nI think there's a potential issue though. Given that the output queues\nare not started in start(), but buffers are still queued in\nqueueRequestDevice(), what will happen if the application calls stop()\nbefore the video nodes are started ? Won't the buffers stay queued ?\n\n> > > }\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 970EFBDE6B\n\tfor ;\n\tFri, 2 Dec 2022 20:06:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3CA763336;\n\tFri, 2 Dec 2022 21:06:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7700160483\n\tfor ;\n\tFri, 2 Dec 2022 21:06:39 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD6226E0;\n\tFri, 2 Dec 2022 21:06:38 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670011601;\n\tbh=7uaBGvKA1c2HNkGQSBpOkJZzriOXGI3ionhODfZ95Eg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kppP2atLP3Z3COEUCQRsZtbf5MFf4aaBHEZX9ZRfd8mRQqRaVmAQxi+lbGlVyhmgq\n\tAk3ye4tLp/dIYk1CMyh96cPu9Pq+Ff+8lP0M9bolIsMR9GETwSlQYq721q1nRTZm/6\n\tkoSZ6DKB+2gt2MooSOlhhd/TAwczw6F9of8a/3qVxGtSc2dI4N/15BWHdIt29tPq3E\n\tjEaa7kiVtCZeP+lL1wCMj5Pfw2O/MidPIt26WirM9p+vKfzdQA3vh8z7GeDwE4QJQc\n\tmqa1RM8dNUVkyncow9qYq9t1mvfv/5nrB4DWw3Uh0Rhtsnrq7dXMUCKXu4ev4YioRg\n\tNe0zSLz3hkj7A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670011599;\n\tbh=7uaBGvKA1c2HNkGQSBpOkJZzriOXGI3ionhODfZ95Eg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eFZAg0AIGXAxB8QbGMqi17Hfe2/zldSmC3QRenqZbpii6n/s0ZC6p4AX2HVQg22oQ\n\tWAWUutKPX9aeVnKRfZhJKxqAra3ouccx0LWI6AvNrdOLAsAkJb6XqgWYkMFOTJDnSB\n\t4P+cW7jeGo4ZKZ83Ldk0F5sFCFIG/aSuhOl7PGFg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eFZAg0AI\"; dkim-atps=neutral","Date":"Fri, 2 Dec 2022 22:06:36 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-6-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26003,"web_url":"https://patchwork.libcamera.org/comment/26003/","msgid":"","date":"2022-12-05T11:11:27","subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Fri, 2 Dec 2022 at 20:06, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote:\n> > On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote:\n> > > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > > > If the pipeline handler is required to drop startup frames by the\n> IPA, do not\n> > > > call StreamOn on the ISP Output0 and Output1 device nodes from\n> > > > PipelineHandlerRPi::start. This stops the ISP from generating output\n> on those\n> > > > channels giving improving latency, and more crucially does not\n> require internal\n> > > > buffers to be allocated to deal with this condition.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > .../pipeline/raspberrypi/data/default.json | 2 +-\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 34\n> ++++++++++++++++---\n> > > > 2 files changed, 31 insertions(+), 5 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json\n> b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > > index d709e31850af..a7ea735c87f4 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > > @@ -14,7 +14,7 @@\n> > > > #\n> min_total_unicam_buffers - external buffer count)\n> > > > \"min_total_unicam_buffers\": 4,\n> > > >\n> > > > - # The number of internal buffers used for ISP\n> Output0. This must be set to 1.\n> > > > + # The number of internal buffers used for ISP\n> Output0.\n> > >\n> > > Can you explain this change in the commit message ?\n> > >\n> > > > \"num_output0_buffers\": 1\n> > > > }\n> > > > }\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index ce411453db0a..86eb43a3a3c5 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera,\n> const ControlList *controls)\n> > > >\n> > > > data->state_ = RPiCameraData::State::Idle;\n> > > >\n> > > > - /* Start all streams. */\n> > > > + /*\n> > > > + * Start all streams with the exception of ISP Output0 and\n> Output1 if\n> > > > + * we are dropping some start-up frames. This saves a tiny bit\n> of latency\n>\n> What latency does this save, and won't we repay that later anyway ?\n>\n\nI don't have an exact time value, but since the stats are generated at the\nfront of\nthe pipeline, no other processing blocks are run if there is no output.\nMultiply that\nby the number (5-8) of convergence frames, and we get a measurable (though\nstill very small) improvement.\n\n\n>\n> > > > + * and avoids the need for allocating an Output0 buffer only\n> to handle\n> > > > + * startup drop frame conditions.\n> > > > + */\n> > >\n> > > I'm not sure to follow you here. Wouldn't it be sufficient to drop the\n> > > buffers at unicam's output without pushing them to the ISP ? Why does\n> > > delaying streamOn() help ?\n> >\n> > You caught me :-)\n> >\n> > I would have done exactly this, only our FW interface does not work that\n> way.\n> > If the kernel device node is opened, we must provide it with an Output0\n> buffer\n> > to function correctly.\n>\n> This patch addresses VIDIOC_STREAMON, do you then mean if the video\n> device node is streaming, not if it is open ?\n>\n\nSorry, I did mean VIDIOC_STREAMON!\n\n\n>\n> > This is purely down to the FW and not the kernel driver.\n> > Given the fragile nature of the FW, I don't want to make any large scale\n> > changes to fix this in FW land.\n>\n> Sometimes I think it could make things easier if the firmware was\n> open-source ;-)\n>\n> More seriously, it seems this could be addressed in kernelspace, by\n> delaying the vchiq_mmal_port_enable() call until the first buffer is\n> queued, which may be as easy as setting vb2_queue.min_buffers_needed to\n> 1. A quick look at videobuf2 doesn't show any immediate ill side\n> effects, but I may have overlooked something. I also understand that\n> changing the kernel would be more complex (as it would need to be synced\n> with merging this series), and thinking more about it, it may be a bit\n> of a hack as it would only work as expect for the startup frame drop.\n>\n\nIndeed, I opted for the userland fix as it does simplify things quite a\nbit.\n\n\n>\n> > > > for (auto const stream : data->streams_) {\n> > > > + if (data->dropFrameCount_ &&\n> > > > + (stream == &data->isp_[Isp::Output0] ||\n> > > > + stream == &data->isp_[Isp::Output1]))\n> > > > + continue;\n> > > > +\n> > > > ret = stream->dev()->streamOn();\n> > > > if (ret) {\n> > > > stop(camera);\n> > > > @@ -1500,6 +1510,13 @@ int\n> PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > if (ret < 0)\n> > > > return ret;\n> > > > } else {\n> > > > + /*\n> > > > + * We don't enable streaming for Output0 and\n> Output1 for\n>\n> Maybe \"We haven't enabled streaming yet\" to make this clearer ?\n>\n\nAck.\n\n\n>\n> > > > + * startup frame drops, so don't queue any\n> buffers.\n> > > > + */\n> > > > + if (stream == &data->isp_[Isp::Output0] ||\n> > > > + stream == &data->isp_[Isp::Output1])\n> > > > + continue;\n>\n> I'd add a blank line here.\n>\n> > > > /*\n> > > > * For external streams, we must queue up a\n> set of internal\n> > > > * buffers to handle the number of drop frames\n> requested by\n> > > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()\n> > > > }\n> > > >\n> > > > /*\n> > > > - * Make sure we have three outputs completed in the case of a\n> dropped\n> > > > - * frame.\n> > > > + * Only the ISP statistics output is generated when we are\n> dropping\n> > > > + * frames on startup.\n> > > > */\n> > > > if (state_ == State::IpaComplete &&\n> > > > - ((ispOutputCount_ == 3 && dropFrameCount_) ||\n> requestCompleted)) {\n> > > > + ((ispOutputCount_ == 1 && dropFrameCount_) ||\n> requestCompleted)) {\n> > > > state_ = State::Idle;\n> > > > if (dropFrameCount_) {\n> > > > dropFrameCount_--;\n> > > > LOG(RPI, Debug) << \"Dropping frame at the\n> request of the IPA (\"\n> > > > << dropFrameCount_ << \" left)\";\n>\n> Here too.\n>\n> > > > + /*\n> > > > + * If we have finished dropping startup\n> frames, start\n> > > > + * streaming on the ISP Output0 and Output1\n> nodes for\n> > > > + * normal operation.\n> > > > + */\n> > > > + if (!dropFrameCount_) {\n> > > > + isp_[Isp::Output0].dev()->streamOn();\n> > > > + isp_[Isp::Output1].dev()->streamOn();\n> > > > + }\n>\n> This looks good.\n>\n> I think there's a potential issue though. Given that the output queues\n> are not started in start(), but buffers are still queued in\n> queueRequestDevice(), what will happen if the application calls stop()\n> before the video nodes are started ? Won't the buffers stay queued ?\n>\n\nPipelineHandlerRPi::stopDevice() unconditionally\ncalls V4L2VideoDevice::streamOff()\nfor the ISP nodes even if they were not started. I was assuming this would\nunqueue\nthe buffers and clean up correctly. Do you think this might not be correct?\n\nRegards,\nNaush\n\n\n>\n> > > > }\n> > > > }\n> > > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AE525BDE6B\n\tfor ;\n\tMon, 5 Dec 2022 11:11:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E2136333F;\n\tMon, 5 Dec 2022 12:11:43 +0100 (CET)","from mail-il1-x131.google.com (mail-il1-x131.google.com\n\t[IPv6:2607:f8b0:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5996061F22\n\tfor ;\n\tMon, 5 Dec 2022 12:11:41 +0100 (CET)","by mail-il1-x131.google.com with SMTP id x13so4924912ilp.8\n\tfor ;\n\tMon, 05 Dec 2022 03:11:41 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670238703;\n\tbh=EBtmxdQ7zeWk0a/XI/gtquGwBQZ0ueLP3M77YLiR8hM=;\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=no+ze2nPq9H41UUHgpIG7vdAR/65zld6t9GC4hy8bdrTP3Lxuc0/pjXDX5ddqR9G8\n\tksmNZh+8LSo55W4aKOKZsHH7Vch5fIZu7H7jNOyfgTkm6wYatTckGsF6WCOrzjxLlz\n\tjVsZBi8PlhpBhGntEARz9XqVDxuQzHUKHSwo1mat+Ii/tWDj8W6W+wdLLqQKOm050m\n\tz5e993Cd0TQILJ7CiQyWxy5i2Fd28KnRzz/kNz/wm36S29VFLuHtsh1PUnTfwP66da\n\tXWwPMTlQVFXrqbIEM3AVBc7ySCbb7Y8Bd1ZDTqamTy+QXe1i9TtI7LetfN+YHG1t0B\n\tu0XX4A8FRS5aQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=F1OIFA2J2G6BNM9wiIHyJ35Ubwy8sRYG7qyaJq8HZZM=;\n\tb=k5jlox2MXOw5jaiApbPaHu5L+i7ecAXJpdEMk1wpxmXoZoUq/yB9GZJgGz1ZpW4j1I\n\t/x9ZPkhllBGFIwqIu/UDq4xA2AHtGiWr+bfO1LzuLbQyWZ9wfagSWOBP6U+pQbv0u3xr\n\t32QYuuSoqjOHld7ZZtyVhFbrXo7CBR3DTVj92sZOTuoXS5wvf4avy78tUcF2GpGmacw4\n\tZY2fBZpuOIfyNwionNkrtYvXkUTnDx2vaIaixjg2a69yIeD+hhkv8kbG2whu1DD89DEY\n\t78VAXhAK5o+WORzSK1Ti7aqlsZC8r/gaTFLGxsgdDVzZH4B76AHE5oYXoA2nEuTVwUth\n\tlRWQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"k5jlox2M\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=F1OIFA2J2G6BNM9wiIHyJ35Ubwy8sRYG7qyaJq8HZZM=;\n\tb=Ss9Blf1mWvUDU1BYcQcALDKl9QLUixSh1jxaoxgWU93QO7qIDbXpdzWYQZfmPnUQ5Q\n\tNYkuI+6qMFLxgVsys8X1PMd2Wmt+mrD8hNT/yr05BaoTgsL9VFeAsZyUtisx4j2WCPrF\n\tsOWK7QLCSCMB/rW+dNQr6tE2OQwVvzRh0bFj+APZOlo5RepytTbgZQodMXG7tHd6i9ZR\n\tvfQHTO6Nw6dC/taZp86RpnSLsrmau78UxR6nkKFmZOXd36KC+ZWhaV9PJMacYLBO8vfi\n\trf2LvYbpj8QCXFUzZNp0x0l94FwCk9H3cvhOaLWveAW4zIzpHrWtMBDl+6BVHELHkh6p\n\tFKEg==","X-Gm-Message-State":"ANoB5plUxI5B2+lHHeDAj/+bwRTJ6heer2/LeaghrdGKtB3fW5ZAhbda\n\tzvVDNmskljLCJyscNxizyKoSTX+MCEDhnvXk8PQOUYd5kkZduG9W","X-Google-Smtp-Source":"AA0mqf7FpxuAZSLpOjB7SzkjsGrpdeET/3ksc8XsWSs1xQPpBRqQ+XnZpGXGxMHe5QgoNq7HJlbks9bxnf9mvDwQpQA=","X-Received":"by 2002:a92:1912:0:b0:302:5c57:c19d with SMTP id\n\t18-20020a921912000000b003025c57c19dmr29389778ilz.226.1670238700053;\n\tMon, 05 Dec 2022 03:11:40 -0800 (PST)","MIME-Version":"1.0","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-6-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Mon, 5 Dec 2022 11:11:27 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000683e2c05ef12c067\"","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","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":26004,"web_url":"https://patchwork.libcamera.org/comment/26004/","msgid":"","date":"2022-12-05T12:31:16","subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Dec 05, 2022 at 11:11:27AM +0000, Naushir Patuck wrote:\n> On Fri, 2 Dec 2022 at 20:06, Laurent Pinchart wrote:\n> > On Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote:\n> > > On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote:\n> > > > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > > > > If the pipeline handler is required to drop startup frames by the IPA, do not\n> > > > > call StreamOn on the ISP Output0 and Output1 device nodes from\n> > > > > PipelineHandlerRPi::start. This stops the ISP from generating output on those\n> > > > > channels giving improving latency, and more crucially does not require internal\n> > > > > buffers to be allocated to deal with this condition.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Reviewed-by: David Plowman \n> > > > > ---\n> > > > > .../pipeline/raspberrypi/data/default.json | 2 +-\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---\n> > > > > 2 files changed, 31 insertions(+), 5 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > > > index d709e31850af..a7ea735c87f4 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > > > @@ -14,7 +14,7 @@\n> > > > > # min_total_unicam_buffers - external buffer count)\n> > > > > \"min_total_unicam_buffers\": 4,\n> > > > >\n> > > > > - # The number of internal buffers used for ISP Output0. This must be set to 1.\n> > > > > + # The number of internal buffers used for ISP Output0.\n> > > >\n> > > > Can you explain this change in the commit message ?\n> > > >\n> > > > > \"num_output0_buffers\": 1\n> > > > > }\n> > > > > }\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index ce411453db0a..86eb43a3a3c5 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > >\n> > > > > data->state_ = RPiCameraData::State::Idle;\n> > > > >\n> > > > > - /* Start all streams. */\n> > > > > + /*\n> > > > > + * Start all streams with the exception of ISP Output0 and Output1 if\n> > > > > + * we are dropping some start-up frames. This saves a tiny bit of latency\n> >\n> > What latency does this save, and won't we repay that later anyway ?\n> \n> I don't have an exact time value, but since the stats are generated at the front of\n> the pipeline, no other processing blocks are run if there is no output. Multiply that\n> by the number (5-8) of convergence frames, and we get a measurable (though\n> still very small) improvement.\n\nAh, I was considering the start() latency, but you're talking about the\nprocessing latency. Makes sense. Could you mention that here ? Something\nalong the lines of \"a tiny bit of ISP processing latency\".\n\n> > > > > + * and avoids the need for allocating an Output0 buffer only to handle\n> > > > > + * startup drop frame conditions.\n> > > > > + */\n> > > >\n> > > > I'm not sure to follow you here. Wouldn't it be sufficient to drop the\n> > > > buffers at unicam's output without pushing them to the ISP ? Why does\n> > > > delaying streamOn() help ?\n> > >\n> > > You caught me :-)\n> > >\n> > > I would have done exactly this, only our FW interface does not work that way.\n> > > If the kernel device node is opened, we must provide it with an Output0 buffer\n> > > to function correctly.\n> >\n> > This patch addresses VIDIOC_STREAMON, do you then mean if the video\n> > device node is streaming, not if it is open ?\n> \n> Sorry, I did mean VIDIOC_STREAMON!\n> \n> > > This is purely down to the FW and not the kernel driver.\n> > > Given the fragile nature of the FW, I don't want to make any large scale\n> > > changes to fix this in FW land.\n> >\n> > Sometimes I think it could make things easier if the firmware was\n> > open-source ;-)\n> >\n> > More seriously, it seems this could be addressed in kernelspace, by\n> > delaying the vchiq_mmal_port_enable() call until the first buffer is\n> > queued, which may be as easy as setting vb2_queue.min_buffers_needed to\n> > 1. A quick look at videobuf2 doesn't show any immediate ill side\n> > effects, but I may have overlooked something. I also understand that\n> > changing the kernel would be more complex (as it would need to be synced\n> > with merging this series), and thinking more about it, it may be a bit\n> > of a hack as it would only work as expect for the startup frame drop.\n> \n> Indeed, I opted for the userland fix as it does simplify things quite a\n> bit.\n> \n> > > > > for (auto const stream : data->streams_) {\n> > > > > + if (data->dropFrameCount_ &&\n> > > > > + (stream == &data->isp_[Isp::Output0] ||\n> > > > > + stream == &data->isp_[Isp::Output1]))\n> > > > > + continue;\n> > > > > +\n> > > > > ret = stream->dev()->streamOn();\n> > > > > if (ret) {\n> > > > > stop(camera);\n> > > > > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > > if (ret < 0)\n> > > > > return ret;\n> > > > > } else {\n> > > > > + /*\n> > > > > + * We don't enable streaming for Output0 and Output1 for\n> >\n> > Maybe \"We haven't enabled streaming yet\" to make this clearer ?\n> \n> Ack.\n> \n> > > > > + * startup frame drops, so don't queue any buffers.\n> > > > > + */\n> > > > > + if (stream == &data->isp_[Isp::Output0] ||\n> > > > > + stream == &data->isp_[Isp::Output1])\n> > > > > + continue;\n> >\n> > I'd add a blank line here.\n> >\n> > > > > /*\n> > > > > * For external streams, we must queue up a set of internal\n> > > > > * buffers to handle the number of drop frames requested by\n> > > > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()\n> > > > > }\n> > > > >\n> > > > > /*\n> > > > > - * Make sure we have three outputs completed in the case of a dropped\n> > > > > - * frame.\n> > > > > + * Only the ISP statistics output is generated when we are dropping\n> > > > > + * frames on startup.\n> > > > > */\n> > > > > if (state_ == State::IpaComplete &&\n> > > > > - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {\n> > > > > + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) {\n> > > > > state_ = State::Idle;\n> > > > > if (dropFrameCount_) {\n> > > > > dropFrameCount_--;\n> > > > > LOG(RPI, Debug) << \"Dropping frame at the request of the IPA (\"\n> > > > > << dropFrameCount_ << \" left)\";\n> >\n> > Here too.\n> >\n> > > > > + /*\n> > > > > + * If we have finished dropping startup frames, start\n> > > > > + * streaming on the ISP Output0 and Output1 nodes for\n> > > > > + * normal operation.\n> > > > > + */\n> > > > > + if (!dropFrameCount_) {\n> > > > > + isp_[Isp::Output0].dev()->streamOn();\n> > > > > + isp_[Isp::Output1].dev()->streamOn();\n> > > > > + }\n> >\n> > This looks good.\n> >\n> > I think there's a potential issue though. Given that the output queues\n> > are not started in start(), but buffers are still queued in\n> > queueRequestDevice(), what will happen if the application calls stop()\n> > before the video nodes are started ? Won't the buffers stay queued ?\n> \n> PipelineHandlerRPi::stopDevice() unconditionally calls V4L2VideoDevice::streamOff()\n> for the ISP nodes even if they were not started. I was assuming this would unqueue\n> the buffers and clean up correctly. Do you think this might not be correct?\n\nI've had another look at the kernel code and the\nV4L2VideoDevice::streamOff() implementation, and I think it's fine. It\nwould still be nice if you could test it though.\n\n> > > > > }\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 2D32EBE08B\n\tfor ;\n\tMon, 5 Dec 2022 12:31:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DD4B6333F;\n\tMon, 5 Dec 2022 13:31:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78A0861F22\n\tfor ;\n\tMon, 5 Dec 2022 13:31:19 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D851A3D7;\n\tMon, 5 Dec 2022 13:31:18 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670243480;\n\tbh=9CxbVPr/Qy0iYiCOjU1myjrMBiZY0YODII0+XUc/ZvM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=LNPOApxap/puShqZoA7VsTNfSU7bDwT69mKNTI+qTBX7Z7yPxcUzWaEiiyLSJZc9K\n\trOiTMCGBDDWUGMjHImj1PvFsCbrXz4M4KtSZ1GDqv8SB0qT/8xxgW8dA3/UQnNVC7Y\n\tI1V+QHdflv+eEm5JAx0KuroFhzwF3cobTEuJy7A+NJlzdF4yOQS3Q4CcXGpAaMgOuS\n\tG3qTUTdL3TVyOUFFGepGh82QdLEQnM1wFTMItLQiqYKE5V68C7MhKhQMJ7pqBTDUlg\n\td5QbefNOQGvs8qrkopNY2w8YnDQsufheKOpYpwcOK2HRb7Di6aUYdjXcfDscGqLUIS\n\tIPpHetYO6Sb/w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670243479;\n\tbh=9CxbVPr/Qy0iYiCOjU1myjrMBiZY0YODII0+XUc/ZvM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TzNjOKurUmztIt2LTUux0wIKhohbrrrzdywYXlSUtO4W8SBzdS9N2pKN7i7KX1aGD\n\tOFv+4GANIdEQ8r/UMp5X2O4KJCAcxvv2xgBsK5Q8HO3IJKZ+mdEWjaHUf3rbuvrXIf\n\t/H9/+eQJB9q1pxEooL4JmwTJqGMePw/kx9sljILw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TzNjOKur\"; dkim-atps=neutral","Date":"Mon, 5 Dec 2022 14:31:16 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-6-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi:\n\tDisable StreamOn for ISP Output0/1 when dropping frames","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]