[{"id":29974,"web_url":"https://patchwork.libcamera.org/comment/29974/","msgid":"<171861531757.2248009.3345344069445577683@ping.linuxembedded.co.uk>","date":"2024-06-17T09:08:37","subject":"Re: [PATCH 04/12] ipa: rkisp1: Document all AGC parameters in\n\tIPAActiveState","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-16 17:39:02)\n> The IPAActiveState AGC documentation is lagging behind the\n> implementation and misses many variables. Document them.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/ipa_context.cpp | 40 +++++++++++++++++++++++++++++-----\n> 1 file changed, 35 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 65e0c58cb2ca..c4895479204c 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -137,13 +137,43 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAActiveState::agc\n> * \\brief State for the Automatic Gain Control algorithm\n> *\n> - * The exposure and gain are the latest values computed by the AGC algorithm.\n> + * The \\a automatic variables track the latest values computed by algorithm\n> + * based on the latest processed statistics. All other variables track the\n> + * consolidated controls requested in queued requests.\n\nI wonder if we should make the state structures convey this somehow by\nseparating the current request context vs the current process context\n... but not now ...\n\n> *\n> - * \\var IPAActiveState::agc.exposure\n> - * \\brief Exposure time expressed as a number of lines\n> + * \\struct IPAActiveState::agc.manual\n> + * \\brief Manual exposure time and analog gain (set through requests)\n> *\n> - * \\var IPAActiveState::agc.gain\n> - * \\brief Analogue gain multiplier\n> + * \\var IPAActiveState::agc.manual.exposure\n> + * \\brief Manual exposure time expressed as a number of lines as set by the\n> + * ExposureTime control\n> + *\n> + * \\var IPAActiveState::agc.manual.gain\n> + * \\brief Manual analogue gain as set by the AnalogueGain control\n> + *\n> + * \\struct IPAActiveState::agc.automatic\n> + * \\brief Automatic exposure time and analog gain (computed by the algorithm)\n> + *\n> + * \\var IPAActiveState::agc.automatic.exposure\n> + * \\brief Automatic exposure time expressed as a number of lines\n> + *\n> + * \\var IPAActiveState::agc.automatic.gain\n> + * \\brief Automatic analogue gain multiplier\n> + *\n> + * \\var IPAActiveState::agc.autoEnabled\n> + * \\brief Manual/automatic AGC state as set by the AeEnable control\n> + *\n> + * \\var IPAActiveState::agc.constraintMode\n> + * \\brief Constraint mode as set by the AeConstraintMode control\n> + *\n> + * \\var IPAActiveState::agc.exposureMode\n> + * \\brief Exposure mode as set by the AeExposureMode control\n> + *\n> + * \\var IPAActiveState::agc.meteringMode\n> + * \\brief Metering mode as set by the AeMeteringMode control\n> + *\n> + * \\var IPAActiveState::agc.maxShutterSpeed\n> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n\n\nReviewed-by: Kieran Bingham \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 37E31C3237\n\tfor ;\n\tMon, 17 Jun 2024 09:08:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 090376548F;\n\tMon, 17 Jun 2024 11:08:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B761B65489\n\tfor ;\n\tMon, 17 Jun 2024 11:08:40 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ECBD739F;\n\tMon, 17 Jun 2024 11:08:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bPg2nFXc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718615304;\n\tbh=WMSgXkqoHnIMZ8JpnfNtGwiSWZE59YtCXK4uXjA4Nyk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=bPg2nFXcvXubJHnZLInuhDT45jm25TFw1MIXEIELj28uZ7RZoE00xl7FUv9+j4qGX\n\tEJSmR+9BURVNmAZrDxsRpATg6B6+PLO1vASYQI63FI+e4N3Fx1XozBDXH/QNLw4pid\n\tY+zcz8cm2DtC2bi2OvlCQoQNN8xk6JmYUHDPHyUI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240616163910.5506-5-laurent.pinchart@ideasonboard.com>","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH 04/12] ipa: rkisp1: Document all AGC parameters in\n\tIPAActiveState","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 17 Jun 2024 10:08:37 +0100","Message-ID":"<171861531757.2248009.3345344069445577683@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29978,"web_url":"https://patchwork.libcamera.org/comment/29978/","msgid":"<20240617092428.GI30324@pendragon.ideasonboard.com>","date":"2024-06-17T09:24:28","subject":"Re: [PATCH 04/12] ipa: rkisp1: Document all AGC parameters in\n\tIPAActiveState","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 17, 2024 at 10:08:37AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-06-16 17:39:02)\n> > The IPAActiveState AGC documentation is lagging behind the\n> > implementation and misses many variables. Document them.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/ipa_context.cpp | 40 +++++++++++++++++++++++++++++-----\n> > 1 file changed, 35 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 65e0c58cb2ca..c4895479204c 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -137,13 +137,43 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\var IPAActiveState::agc\n> > * \\brief State for the Automatic Gain Control algorithm\n> > *\n> > - * The exposure and gain are the latest values computed by the AGC algorithm.\n> > + * The \\a automatic variables track the latest values computed by algorithm\n> > + * based on the latest processed statistics. All other variables track the\n> > + * consolidated controls requested in queued requests.\n> \n> I wonder if we should make the state structures convey this somehow by\n> separating the current request context vs the current process context\n> ... but not now ...\n\nYes, I think we should. I've discussed that with Stefan before, and I\nthink we'll take that direction.\n\nWe have quite a bit of code in algorithms along the lines of\n\n const auto &exposureMode = controls.get(controls::AeExposureMode);\n if (exposureMode) {\n frameContext.agc.update = agc.exposureMode != *exposureMode;\n agc.exposureMode =\n static_cast(*exposureMode);\n }\n frameContext.agc.exposureMode = agc.exposureMode;\n\nI wonder if we could reorganize structures in a way that would allow\nautomating some of this.\n\n> > *\n> > - * \\var IPAActiveState::agc.exposure\n> > - * \\brief Exposure time expressed as a number of lines\n> > + * \\struct IPAActiveState::agc.manual\n> > + * \\brief Manual exposure time and analog gain (set through requests)\n> > *\n> > - * \\var IPAActiveState::agc.gain\n> > - * \\brief Analogue gain multiplier\n> > + * \\var IPAActiveState::agc.manual.exposure\n> > + * \\brief Manual exposure time expressed as a number of lines as set by the\n> > + * ExposureTime control\n> > + *\n> > + * \\var IPAActiveState::agc.manual.gain\n> > + * \\brief Manual analogue gain as set by the AnalogueGain control\n> > + *\n> > + * \\struct IPAActiveState::agc.automatic\n> > + * \\brief Automatic exposure time and analog gain (computed by the algorithm)\n> > + *\n> > + * \\var IPAActiveState::agc.automatic.exposure\n> > + * \\brief Automatic exposure time expressed as a number of lines\n> > + *\n> > + * \\var IPAActiveState::agc.automatic.gain\n> > + * \\brief Automatic analogue gain multiplier\n> > + *\n> > + * \\var IPAActiveState::agc.autoEnabled\n> > + * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > + *\n> > + * \\var IPAActiveState::agc.constraintMode\n> > + * \\brief Constraint mode as set by the AeConstraintMode control\n> > + *\n> > + * \\var IPAActiveState::agc.exposureMode\n> > + * \\brief Exposure mode as set by the AeExposureMode control\n> > + *\n> > + * \\var IPAActiveState::agc.meteringMode\n> > + * \\brief Metering mode as set by the AeMeteringMode control\n> > + *\n> > + * \\var IPAActiveState::agc.maxShutterSpeed\n> > + * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> \n> Reviewed-by: Kieran Bingham \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 358DCBD87C\n\tfor ;\n\tMon, 17 Jun 2024 09:24:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2912965493;\n\tMon, 17 Jun 2024 11:24:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD80B6548D\n\tfor ;\n\tMon, 17 Jun 2024 11:24:49 +0200 (CEST)","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 C916C39F;\n\tMon, 17 Jun 2024 11:24:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JQ4e7gde\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718616273;\n\tbh=T5ePx1RFzYlDjWEruJY+Mr8XJbrRaKtlDmu039bwZYQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JQ4e7gdeRdMDtqc6twum+S29WmHUVLPUV8KzBdyoF/AjZ5Jh4qIj7tKFIez2QZ935\n\tNUE4bThNRF90h6yHrPJVaaurqn+J5f9AXgKXWLPYLhtV3XyX3C4sWWzNx1WNxO6AuY\n\tiHeIErzLzqjsHa69hs1OT9kK0KiFS2bHSR2dP9ZI=","Date":"Mon, 17 Jun 2024 12:24:28 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 04/12] ipa: rkisp1: Document all AGC parameters in\n\tIPAActiveState","Message-ID":"<20240617092428.GI30324@pendragon.ideasonboard.com>","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-5-laurent.pinchart@ideasonboard.com>\n\t<171861531757.2248009.3345344069445577683@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171861531757.2248009.3345344069445577683@ping.linuxembedded.co.uk>","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":30001,"web_url":"https://patchwork.libcamera.org/comment/30001/","msgid":"","date":"2024-06-17T13:09:42","subject":"Re: [PATCH 04/12] ipa: rkisp1: Document all AGC parameters in\n\tIPAActiveState","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Sun, Jun 16, 2024 at 07:39:02PM +0300, Laurent Pinchart wrote:\n> The IPAActiveState AGC documentation is lagging behind the\n> implementation and misses many variables. Document them.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/rkisp1/ipa_context.cpp | 40 +++++++++++++++++++++++++++++-----\n> 1 file changed, 35 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 65e0c58cb2ca..c4895479204c 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -137,13 +137,43 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAActiveState::agc\n> * \\brief State for the Automatic Gain Control algorithm\n> *\n> - * The exposure and gain are the latest values computed by the AGC algorithm.\n> + * The \\a automatic variables track the latest values computed by algorithm\n> + * based on the latest processed statistics. All other variables track the\n> + * consolidated controls requested in queued requests.\n> *\n> - * \\var IPAActiveState::agc.exposure\n> - * \\brief Exposure time expressed as a number of lines\n> + * \\struct IPAActiveState::agc.manual\n> + * \\brief Manual exposure time and analog gain (set through requests)\n> *\n> - * \\var IPAActiveState::agc.gain\n> - * \\brief Analogue gain multiplier\n> + * \\var IPAActiveState::agc.manual.exposure\n> + * \\brief Manual exposure time expressed as a number of lines as set by the\n> + * ExposureTime control\n> + *\n> + * \\var IPAActiveState::agc.manual.gain\n> + * \\brief Manual analogue gain as set by the AnalogueGain control\n> + *\n> + * \\struct IPAActiveState::agc.automatic\n> + * \\brief Automatic exposure time and analog gain (computed by the algorithm)\n> + *\n> + * \\var IPAActiveState::agc.automatic.exposure\n> + * \\brief Automatic exposure time expressed as a number of lines\n> + *\n> + * \\var IPAActiveState::agc.automatic.gain\n> + * \\brief Automatic analogue gain multiplier\n> + *\n> + * \\var IPAActiveState::agc.autoEnabled\n> + * \\brief Manual/automatic AGC state as set by the AeEnable control\n> + *\n> + * \\var IPAActiveState::agc.constraintMode\n> + * \\brief Constraint mode as set by the AeConstraintMode control\n> + *\n> + * \\var IPAActiveState::agc.exposureMode\n> + * \\brief Exposure mode as set by the AeExposureMode control\n> + *\n> + * \\var IPAActiveState::agc.meteringMode\n> + * \\brief Metering mode as set by the AeMeteringMode control\n> + *\n> + * \\var IPAActiveState::agc.maxShutterSpeed\n> + * \\brief Maximum frame duration as set by the FrameDurationLimits control\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 59DF2BD87C\n\tfor ;\n\tMon, 17 Jun 2024 13:09:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F84065497;\n\tMon, 17 Jun 2024 15:09:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D970E6548B\n\tfor ;\n\tMon, 17 Jun 2024 15:09:49 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CBB0A39F;\n\tMon, 17 Jun 2024 15:09:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ahtuYdAZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718629773;\n\tbh=HaMLw2oPEt056kQ3KSitp/JJQmS8Og6ayKZijkz6Q5w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ahtuYdAZZRPUyZLVOGapcrtFIRSwSi28KV8WXvNJkJYgbMgdYBF/Uk2aOyOefoGUO\n\tmF0tiAiUNcsxhrR+su1Ga1C6Re/QwBWZNFBJaifZDxdvcrx0o81r58jOkyres7C+P4\n\tP5ofnzsrKUpBkjEpHFDrOxqFXxFNRT78RbmMS2KQ=","Date":"Mon, 17 Jun 2024 22:09:42 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 04/12] ipa: rkisp1: Document all AGC parameters in\n\tIPAActiveState","Message-ID":"","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240616163910.5506-5-laurent.pinchart@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\" "}}]