[{"id":33033,"web_url":"https://patchwork.libcamera.org/comment/33033/","msgid":"<20250112223102.GD13236@pendragon.ideasonboard.com>","date":"2025-01-12T22:31:02","subject":"Re: [RFC PATCH 7/7] ipa: rkisp1: Set frameContext.agc in\n\tqueueRequest for auto mode also","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Fri, Dec 20, 2024 at 05:26:53PM +0100, Stefan Klug wrote:\n> If the agc is in auto mode, exposure time and gain used to be set on the\n> frame context within prepare(). As exposure time and gain are used by\n> getSensorControls(0) from within start() that is too late (prepare()\n> hasn't run yet). Also prepare() is documented as the place to initialize\n> the params buffer, not the frame context. From the pipeline point of\n> view, prepare() gets called immediately after queueRequest(). Therefore\n> we can safely move setting the frame context into queueRequest() to fix\n> the sensor controls for start().\n\nI don't think that's right. When queueRequest() is called the active\nstate doesn't contain recent enough agc values.\n\nIt may be that computeParams() is called right after queueRequest() in\nthe pipeline handler, but that's not how it should be.\n\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++------\n> 1 file changed, 4 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 40e5a8f481b2..46be1413a728 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -243,7 +243,10 @@ void Agc::queueRequest(IPAContext &context,\n> \n> \tframeContext.agc.autoEnabled = agc.autoEnabled;\n> \n> -\tif (!frameContext.agc.autoEnabled) {\n> +\tif (frameContext.agc.autoEnabled) {\n> +\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> +\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> +\t} else {\n> \t\tframeContext.agc.exposure = agc.manual.exposure;\n> \t\tframeContext.agc.gain = agc.manual.gain;\n> \t}\n> @@ -283,11 +286,6 @@ void Agc::queueRequest(IPAContext &context,\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> {\n> -\tif (frameContext.agc.autoEnabled) {\n> -\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> -\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> -\t}\n> -\n> \tif (frame > 0 && !frameContext.agc.updateMetering)\n> \t\treturn;\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 11009C326C\n\tfor ;\n\tSun, 12 Jan 2025 22:31:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D855684E0;\n\tSun, 12 Jan 2025 23:31:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA4B661882\n\tfor ;\n\tSun, 12 Jan 2025 23:31:06 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 62C75250;\n\tSun, 12 Jan 2025 23:30:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Y1NSolQY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736721010;\n\tbh=8lepxJxEV/KiOTOPRb/s4MfGzZfO5zZ6xfh6Fyv7dvA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y1NSolQYISTZtoC6C0Z2/qeA7tXotQBxakkQCxDc5itaSY6W3MBAL0Ruo58jupmis\n\tklyb2XFvV3cpN9pw4B1LsXDamTPWKMHt9Rec1P7gQVdpX+cg51Z+rH4DQhEdYGXJAT\n\tSglpbhVKVtEVzdiLvUQQ0o5JQH3LCLu7R/zwULek=","Date":"Mon, 13 Jan 2025 00:31:02 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 7/7] ipa: rkisp1: Set frameContext.agc in\n\tqueueRequest for auto mode also","Message-ID":"<20250112223102.GD13236@pendragon.ideasonboard.com>","References":"<20241220162724.756494-1-stefan.klug@ideasonboard.com>\n\t<20241220162724.756494-8-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241220162724.756494-8-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33047,"web_url":"https://patchwork.libcamera.org/comment/33047/","msgid":"","date":"2025-01-13T13:40:30","subject":"Re: [RFC PATCH 7/7] ipa: rkisp1: Set frameContext.agc in\n\tqueueRequest for auto mode also","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Mon, Jan 13, 2025 at 12:31:02AM +0200, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Fri, Dec 20, 2024 at 05:26:53PM +0100, Stefan Klug wrote:\n> > If the agc is in auto mode, exposure time and gain used to be set on the\n> > frame context within prepare(). As exposure time and gain are used by\n> > getSensorControls(0) from within start() that is too late (prepare()\n> > hasn't run yet). Also prepare() is documented as the place to initialize\n> > the params buffer, not the frame context. From the pipeline point of\n> > view, prepare() gets called immediately after queueRequest(). Therefore\n> > we can safely move setting the frame context into queueRequest() to fix\n> > the sensor controls for start().\n> \n> I don't think that's right. When queueRequest() is called the active\n> state doesn't contain recent enough agc values.\n> \n> It may be that computeParams() is called right after queueRequest() in\n> the pipeline handler, but that's not how it should be.\n\nHmm, also a place where more conceptual work is needed (or at least I\nneed to draw some graphs for me). In my understanding computeParams()\nshould be called right before the isp starts to process the given frame.\nThis is at least maxSensorDelay too late for sending out the sensor\ncontrols for that frame. So setting frameContext.agc.exposure in\ncomputeParams()/prepare() is the wrong place. I agree that\nqueueRequest() is also not the best place, but still better in the sense\nthat it is not too late but maybe not the most up to date value we could\nget.\n\nCheers,\nStefan\n\n> \n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++------\n> > 1 file changed, 4 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 40e5a8f481b2..46be1413a728 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -243,7 +243,10 @@ void Agc::queueRequest(IPAContext &context,\n> > \n> > \tframeContext.agc.autoEnabled = agc.autoEnabled;\n> > \n> > -\tif (!frameContext.agc.autoEnabled) {\n> > +\tif (frameContext.agc.autoEnabled) {\n> > +\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> > +\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > +\t} else {\n> > \t\tframeContext.agc.exposure = agc.manual.exposure;\n> > \t\tframeContext.agc.gain = agc.manual.gain;\n> > \t}\n> > @@ -283,11 +286,6 @@ void Agc::queueRequest(IPAContext &context,\n> > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> > {\n> > -\tif (frameContext.agc.autoEnabled) {\n> > -\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> > -\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > -\t}\n> > -\n> > \tif (frame > 0 && !frameContext.agc.updateMetering)\n> > \t\treturn;\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 914C9C326C\n\tfor ;\n\tMon, 13 Jan 2025 13:40:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B939E68503;\n\tMon, 13 Jan 2025 14:40:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D401684E7\n\tfor ;\n\tMon, 13 Jan 2025 14:40:34 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3c15:8eea:8742:2e5a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A41374A;\n\tMon, 13 Jan 2025 14:39:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YOsiAVmB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736775577;\n\tbh=B6q1u8IQvrks818Lg1l58U3ukpVh2vPlcLn0o8Qb4CI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YOsiAVmB97dDpCB/L7xepAg15FwVCQJQFI3I/ZoUp/ouiaDhoGZfOuc37RScTaTVh\n\t1rgxwm6AB4HcaYdakIqsFpmGzDBHGhDMF8x4SNi8vvqmYtAOvXMAJ5yTetPP4hZfB5\n\trx9ppycIGS5pD3nCdCGNC9cvZ+1jmAxC4+24OxcM=","Date":"Mon, 13 Jan 2025 14:40:30 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 7/7] ipa: rkisp1: Set frameContext.agc in\n\tqueueRequest for auto mode also","Message-ID":"","References":"<20241220162724.756494-1-stefan.klug@ideasonboard.com>\n\t<20241220162724.756494-8-stefan.klug@ideasonboard.com>\n\t<20250112223102.GD13236@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250112223102.GD13236@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33402,"web_url":"https://patchwork.libcamera.org/comment/33402/","msgid":"","date":"2025-02-20T08:44:08","subject":"Re: [RFC PATCH 7/7] ipa: rkisp1: Set frameContext.agc in\n\tqueueRequest for auto mode also","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Jan 13, 2025 at 02:40:30PM +0100, Stefan Klug wrote:\n> Hi Laurent,\n> \n> Thank you for the review. \n> \n> On Mon, Jan 13, 2025 at 12:31:02AM +0200, Laurent Pinchart wrote:\n> > Hi Stefan,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Dec 20, 2024 at 05:26:53PM +0100, Stefan Klug wrote:\n> > > If the agc is in auto mode, exposure time and gain used to be set on the\n> > > frame context within prepare(). As exposure time and gain are used by\n> > > getSensorControls(0) from within start() that is too late (prepare()\n> > > hasn't run yet). Also prepare() is documented as the place to initialize\n> > > the params buffer, not the frame context. From the pipeline point of\n> > > view, prepare() gets called immediately after queueRequest(). Therefore\n> > > we can safely move setting the frame context into queueRequest() to fix\n> > > the sensor controls for start().\n> > \n> > I don't think that's right. When queueRequest() is called the active\n> > state doesn't contain recent enough agc values.\n> > \n> > It may be that computeParams() is called right after queueRequest() in\n> > the pipeline handler, but that's not how it should be.\n> \n> Hmm, also a place where more conceptual work is needed (or at least I\n> need to draw some graphs for me). In my understanding computeParams()\n> should be called right before the isp starts to process the given frame.\n> This is at least maxSensorDelay too late for sending out the sensor\n> controls for that frame. So setting frameContext.agc.exposure in\n> computeParams()/prepare() is the wrong place. I agree that\n> queueRequest() is also not the best place, but still better in the sense\n> that it is not too late but maybe not the most up to date value we could\n> get.\n\nMy understanding was that the active state is written after the stats\nbuffer comes in (process()), but it needs to be read out right before\nit's time to queue the params buffer (prepare()) and that's why for\npopulating the params buffer in auto mode we set it in prepare().\n\n(Unless we don't have proper synchronization yet so in practice\nqueueRequest() and prepare() don't make much of a difference in terms of\ntiming.)\n\n\nPaul\n\n> > \n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++------\n> > > 1 file changed, 4 insertions(+), 6 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 40e5a8f481b2..46be1413a728 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -243,7 +243,10 @@ void Agc::queueRequest(IPAContext &context,\n> > > \n> > > \tframeContext.agc.autoEnabled = agc.autoEnabled;\n> > > \n> > > -\tif (!frameContext.agc.autoEnabled) {\n> > > +\tif (frameContext.agc.autoEnabled) {\n> > > +\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> > > +\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > > +\t} else {\n> > > \t\tframeContext.agc.exposure = agc.manual.exposure;\n> > > \t\tframeContext.agc.gain = agc.manual.gain;\n> > > \t}\n> > > @@ -283,11 +286,6 @@ void Agc::queueRequest(IPAContext &context,\n> > > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > \t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> > > {\n> > > -\tif (frameContext.agc.autoEnabled) {\n> > > -\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> > > -\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > > -\t}\n> > > -\n> > > \tif (frame > 0 && !frameContext.agc.updateMetering)\n> > > \t\treturn;\n> > > \n> > \n> > -- \n> > Regards,\n> > \n> > Laurent Pinchart","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 9F9A2BE175\n\tfor ;\n\tThu, 20 Feb 2025 08:44:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B244968695;\n\tThu, 20 Feb 2025 09:44:18 +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 9F4746185D\n\tfor ;\n\tThu, 20 Feb 2025 09:44:16 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:4292:75df:25cd:1836])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D7B23526;\n\tThu, 20 Feb 2025 09:42:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tj69zAXR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740040973;\n\tbh=1QcMdijeNyFX97KuOmL0ovBp8yJStfsPY44TelfdPqc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tj69zAXRJcnNIX/zoT0pKzjCw0EAblA4bwjRUgJO//hE9FxJFbBowiBWPKWWllZLV\n\tjxxFijbLcMl/uzV+Y8MtM0zUlhOq/5mixajj4lXDgcuLvMvqhKFRRbqKWqNKMZ873q\n\tI4F3yqB0nENXf5oR7kRWwiRPmuQogrsudyMzTWM0=","Date":"Thu, 20 Feb 2025 17:44:08 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 7/7] ipa: rkisp1: Set frameContext.agc in\n\tqueueRequest for auto mode also","Message-ID":"","References":"<20241220162724.756494-1-stefan.klug@ideasonboard.com>\n\t<20241220162724.756494-8-stefan.klug@ideasonboard.com>\n\t<20250112223102.GD13236@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]