[{"id":22215,"web_url":"https://patchwork.libcamera.org/comment/22215/","msgid":"<164630803440.3471399.2692655364189567761@Monstersaurus>","date":"2022-03-03T11:47:14","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2022-03-01 07:59:19)\n> The exposure and gain limits are required for AGC configuration\n> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n> already. Therefore the max/min private members in IPAIPU3 class for\n> exposure/gain serve no use except setting initial values of exposure_\n> and gain_ members.\n> \n> Drop the max/min private members from IPAIPU3 class and set initial\n> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n\n\nGreat, even better to drop them if they aren't used.\n\nThis looks like we're on the path to removing the private variables for\ncontrols from the IPAIPU3 now, as they should all be stored in the\ncontext if there's any relationship with the algorithms.\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Umang Jain \n> ---\n> Changes in v2:\n> - Split into separate patch\n> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n> - Don't include max/min exposure/gain members in\n> IPASessionConfiguration, as they aren't used anywhere. \n> ---\n> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n> 1 file changed, 2 insertions(+), 24 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 17324616..4b6852a7 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -167,11 +167,7 @@ private:\n> /* Camera sensor controls. */\n> uint32_t defVBlank_;\n> uint32_t exposure_;\n> - uint32_t minExposure_;\n> - uint32_t maxExposure_;\n> uint32_t gain_;\n> - uint32_t minGain_;\n> - uint32_t maxGain_;\n\nWhat's required to get rid of gain_, exposure_ and defVBlank_ from here\ntoo?\n\n--\nKieran\n\n\n> \n> /* Interface to the Camera Helper */\n> std::unique_ptr camHelper_;\n> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> int32_t minExposure = v4l2Exposure.min().get();\n> int32_t maxExposure = v4l2Exposure.max().get();\n> + exposure_ = minExposure;\n> \n> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> int32_t minGain = v4l2Gain.min().get();\n> int32_t maxGain = v4l2Gain.max().get();\n> + gain_ = minGain;\n> \n> /*\n> * When the AGC computes the new exposure values for a frame, it needs\n> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> */\n> ctrls_ = configInfo.sensorControls;\n> \n> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> - if (itExp == ctrls_.end()) {\n> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n> - return -EINVAL;\n> - }\n> -\n> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> - if (itGain == ctrls_.end()) {\n> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n> - return -EINVAL;\n> - }\n> -\n> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n> if (itVBlank == ctrls_.end()) {\n> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n> return -EINVAL;\n> }\n> \n> - minExposure_ = itExp->second.min().get();\n> - maxExposure_ = itExp->second.max().get();\n> - exposure_ = minExposure_;\n> -\n> - minGain_ = itGain->second.min().get();\n> - maxGain_ = itGain->second.max().get();\n> - gain_ = minGain_;\n> -\n> defVBlank_ = itVBlank->second.def().get();\n> \n> calculateBdsGrid(configInfo.bdsOutputSize);\n> -- \n> 2.31.1\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 8C3C2BF415\n\tfor ;\n\tThu, 3 Mar 2022 11:47:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 025BC60475;\n\tThu, 3 Mar 2022 12:47:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E230601FF\n\tfor ;\n\tThu, 3 Mar 2022 12:47:17 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9DB5B101E;\n\tThu, 3 Mar 2022 12:47:16 +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=\"Hv4wn6Hx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646308036;\n\tbh=Q2kc06cXvZ79xLa/c9uzZR874WD+4Qp4QNDC0fEVYpk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Hv4wn6Hx5ksWbSpDZ1ag3MrGdgMw9VW/BS/LC8nd8OhpW1Dj6U9jH7LsSrvD3e1nn\n\teGgDswqb9CTTwObN3EgwBina5GcW/BADUk1oCo2Hh0ZnYCAxhX1qr+T+E3as2YLeoU\n\tJHF6pDfdU4XdYQYfMlEP85tawK5V/7myl1IbNKI8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220301075919.201968-1-umang.jain@ideasonboard.com>","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>","From":"Kieran Bingham ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Mar 2022 11:47:14 +0000","Message-ID":"<164630803440.3471399.2692655364189567761@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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":22217,"web_url":"https://patchwork.libcamera.org/comment/22217/","msgid":"","date":"2022-03-03T12:15:44","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 3/3/22 17:17, Kieran Bingham wrote:\n> Quoting Umang Jain (2022-03-01 07:59:19)\n>> The exposure and gain limits are required for AGC configuration\n>> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n>> already. Therefore the max/min private members in IPAIPU3 class for\n>> exposure/gain serve no use except setting initial values of exposure_\n>> and gain_ members.\n>>\n>> Drop the max/min private members from IPAIPU3 class and set initial\n>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n>\n> Great, even better to drop them if they aren't used.\n>\n> This looks like we're on the path to removing the private variables for\n> controls from the IPAIPU3 now, as they should all be stored in the\n> context if there's any relationship with the algorithms.\n\n\nyeah, we are on that path.\n\n>\n> Reviewed-by: Kieran Bingham \n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> Changes in v2:\n>> - Split into separate patch\n>> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n>> - Don't include max/min exposure/gain members in\n>> IPASessionConfiguration, as they aren't used anywhere.\n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n>> 1 file changed, 2 insertions(+), 24 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 17324616..4b6852a7 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -167,11 +167,7 @@ private:\n>> /* Camera sensor controls. */\n>> uint32_t defVBlank_;\n>> uint32_t exposure_;\n>> - uint32_t minExposure_;\n>> - uint32_t maxExposure_;\n>> uint32_t gain_;\n>> - uint32_t minGain_;\n>> - uint32_t maxGain_;\n> What's required to get rid of gain_, exposure_ and defVBlank_ from here\n> too?\n\n\nwe can drop gain_, exposure_ and replace them with local vars, no issue \nin that.\n\ndefVBlank_ seems to be used IPAIPU3 class functions directly, but i \nthink we can move the defVBlank_ to JM's introduction of \nIPASessionConfiguration.sensor structure\n\nWith this plan i think we can drop these remaining private members from \nIPAIPU3.\n\n>\n> --\n> Kieran\n>\n>\n>> \n>> /* Interface to the Camera Helper */\n>> std::unique_ptr camHelper_;\n>> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>> int32_t minExposure = v4l2Exposure.min().get();\n>> int32_t maxExposure = v4l2Exposure.max().get();\n>> + exposure_ = minExposure;\n>> \n>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>> int32_t minGain = v4l2Gain.min().get();\n>> int32_t maxGain = v4l2Gain.max().get();\n>> + gain_ = minGain;\n>> \n>> /*\n>> * When the AGC computes the new exposure values for a frame, it needs\n>> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>> */\n>> ctrls_ = configInfo.sensorControls;\n>> \n>> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>> - if (itExp == ctrls_.end()) {\n>> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n>> - return -EINVAL;\n>> - }\n>> -\n>> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n>> - if (itGain == ctrls_.end()) {\n>> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n>> - return -EINVAL;\n>> - }\n>> -\n>> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n>> if (itVBlank == ctrls_.end()) {\n>> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n>> return -EINVAL;\n>> }\n>> \n>> - minExposure_ = itExp->second.min().get();\n>> - maxExposure_ = itExp->second.max().get();\n>> - exposure_ = minExposure_;\n>> -\n>> - minGain_ = itGain->second.min().get();\n>> - maxGain_ = itGain->second.max().get();\n>> - gain_ = minGain_;\n>> -\n>> defVBlank_ = itVBlank->second.def().get();\n>> \n>> calculateBdsGrid(configInfo.bdsOutputSize);\n>> -- \n>> 2.31.1\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 EEACEBE08A\n\tfor ;\n\tThu, 3 Mar 2022 12:15:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46D7861160;\n\tThu, 3 Mar 2022 13:15:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B788601FF\n\tfor ;\n\tThu, 3 Mar 2022 13:15:51 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1525885;\n\tThu, 3 Mar 2022 13:15:49 +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=\"XPCT9n6L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646309750;\n\tbh=7GKWFlN45cur0cAr/s34iTRh+WRkiJPCS05oKivzABE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=XPCT9n6LvOk1Al2rnyQGGXL2QKVkHuX0vEuYWWqqlR+aiZGJ9DLuLBH8r320koC8r\n\tK8f+08yA3t0BXFcSA2jQcWP0saTNaQlHSA0Cy9FP3peEDbNcarHCOX0o2hxjBBT7vR\n\t9ROzIASa0NFn1B+geQCCAJ1YDBuOyLK3ng7+yYsI=","Message-ID":"","Date":"Thu, 3 Mar 2022 17:45:44 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>\n\t<164630803440.3471399.2692655364189567761@Monstersaurus>","From":"Umang Jain ","In-Reply-To":"<164630803440.3471399.2692655364189567761@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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":22219,"web_url":"https://patchwork.libcamera.org/comment/22219/","msgid":"<164631640038.3492470.11256900790981757289@Monstersaurus>","date":"2022-03-03T14:06:40","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2022-03-03 12:15:44)\n> Hi Kieran,\n> \n> On 3/3/22 17:17, Kieran Bingham wrote:\n> > Quoting Umang Jain (2022-03-01 07:59:19)\n> >> The exposure and gain limits are required for AGC configuration\n> >> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n> >> already. Therefore the max/min private members in IPAIPU3 class for\n> >> exposure/gain serve no use except setting initial values of exposure_\n> >> and gain_ members.\n> >>\n> >> Drop the max/min private members from IPAIPU3 class and set initial\n> >> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n> >\n> > Great, even better to drop them if they aren't used.\n> >\n> > This looks like we're on the path to removing the private variables for\n> > controls from the IPAIPU3 now, as they should all be stored in the\n> > context if there's any relationship with the algorithms.\n> \n> \n> yeah, we are on that path.\n> \n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> Changes in v2:\n> >> - Split into separate patch\n> >> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n> >> - Don't include max/min exposure/gain members in\n> >> IPASessionConfiguration, as they aren't used anywhere.\n> >> ---\n> >> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n> >> 1 file changed, 2 insertions(+), 24 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 17324616..4b6852a7 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -167,11 +167,7 @@ private:\n> >> /* Camera sensor controls. */\n> >> uint32_t defVBlank_;\n> >> uint32_t exposure_;\n> >> - uint32_t minExposure_;\n> >> - uint32_t maxExposure_;\n> >> uint32_t gain_;\n> >> - uint32_t minGain_;\n> >> - uint32_t maxGain_;\n> > What's required to get rid of gain_, exposure_ and defVBlank_ from here\n> > too?\n> \n> \n> we can drop gain_, exposure_ and replace them with local vars, no issue \n> in that.\n> \n> defVBlank_ seems to be used IPAIPU3 class functions directly, but i \n> think we can move the defVBlank_ to JM's introduction of \n> IPASessionConfiguration.sensor structure\n> \n> With this plan i think we can drop these remaining private members from \n> IPAIPU3.\n\nGreat, can easily be patches on top - doesn't need to be in this one.\n\n--\nKieran\n\n\n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> >> \n> >> /* Interface to the Camera Helper */\n> >> std::unique_ptr camHelper_;\n> >> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> >> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> >> int32_t minExposure = v4l2Exposure.min().get();\n> >> int32_t maxExposure = v4l2Exposure.max().get();\n> >> + exposure_ = minExposure;\n> >> \n> >> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> >> int32_t minGain = v4l2Gain.min().get();\n> >> int32_t maxGain = v4l2Gain.max().get();\n> >> + gain_ = minGain;\n> >> \n> >> /*\n> >> * When the AGC computes the new exposure values for a frame, it needs\n> >> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >> */\n> >> ctrls_ = configInfo.sensorControls;\n> >> \n> >> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> >> - if (itExp == ctrls_.end()) {\n> >> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n> >> - return -EINVAL;\n> >> - }\n> >> -\n> >> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> >> - if (itGain == ctrls_.end()) {\n> >> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n> >> - return -EINVAL;\n> >> - }\n> >> -\n> >> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n> >> if (itVBlank == ctrls_.end()) {\n> >> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n> >> return -EINVAL;\n> >> }\n> >> \n> >> - minExposure_ = itExp->second.min().get();\n> >> - maxExposure_ = itExp->second.max().get();\n> >> - exposure_ = minExposure_;\n> >> -\n> >> - minGain_ = itGain->second.min().get();\n> >> - maxGain_ = itGain->second.max().get();\n> >> - gain_ = minGain_;\n> >> -\n> >> defVBlank_ = itVBlank->second.def().get();\n> >> \n> >> calculateBdsGrid(configInfo.bdsOutputSize);\n> >> -- \n> >> 2.31.1\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 4ACB9BF415\n\tfor ;\n\tThu, 3 Mar 2022 14:06:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9495E60475;\n\tThu, 3 Mar 2022 15:06:44 +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 2EFD1601FF\n\tfor ;\n\tThu, 3 Mar 2022 15:06:43 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC11F885;\n\tThu, 3 Mar 2022 15:06:42 +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=\"nqfgwyQ7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646316402;\n\tbh=nwYV6bkInFvjIJ+5UBigdajg6Ci6VsQymiFXnE8Ak58=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=nqfgwyQ74zq8peexD+JWleGD2W/DxPSyGG2UY5X5jc1dFYvy/COWfsKnNTxuVNIsp\n\tTNY1xZ+uPzimJHVZ0zMjWBF4Ze4UzKB+MYoRjGwqu1szk+0LY8UN0ETeVFP4oZBtoR\n\t4I8ydk3h5itxVmufA01cM8+hoUctMzjEvZDT9E5Y=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>\n\t<164630803440.3471399.2692655364189567761@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Mar 2022 14:06:40 +0000","Message-ID":"<164631640038.3492470.11256900790981757289@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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":22220,"web_url":"https://patchwork.libcamera.org/comment/22220/","msgid":"","date":"2022-03-03T14:14:06","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 3/3/22 19:36, Kieran Bingham wrote:\n> Quoting Umang Jain (2022-03-03 12:15:44)\n>> Hi Kieran,\n>>\n>> On 3/3/22 17:17, Kieran Bingham wrote:\n>>> Quoting Umang Jain (2022-03-01 07:59:19)\n>>>> The exposure and gain limits are required for AGC configuration\n>>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n>>>> already. Therefore the max/min private members in IPAIPU3 class for\n>>>> exposure/gain serve no use except setting initial values of exposure_\n>>>> and gain_ members.\n>>>>\n>>>> Drop the max/min private members from IPAIPU3 class and set initial\n>>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n>>> Great, even better to drop them if they aren't used.\n>>>\n>>> This looks like we're on the path to removing the private variables for\n>>> controls from the IPAIPU3 now, as they should all be stored in the\n>>> context if there's any relationship with the algorithms.\n>>\n>> yeah, we are on that path.\n>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> Changes in v2:\n>>>> - Split into separate patch\n>>>> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n>>>> - Don't include max/min exposure/gain members in\n>>>> IPASessionConfiguration, as they aren't used anywhere.\n>>>> ---\n>>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n>>>> 1 file changed, 2 insertions(+), 24 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 17324616..4b6852a7 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -167,11 +167,7 @@ private:\n>>>> /* Camera sensor controls. */\n>>>> uint32_t defVBlank_;\n>>>> uint32_t exposure_;\n>>>> - uint32_t minExposure_;\n>>>> - uint32_t maxExposure_;\n>>>> uint32_t gain_;\n>>>> - uint32_t minGain_;\n>>>> - uint32_t maxGain_;\n>>> What's required to get rid of gain_, exposure_ and defVBlank_ from here\n>>> too?\n>>\n>> we can drop gain_, exposure_ and replace them with local vars, no issue\n>> in that.\n>>\n>> defVBlank_ seems to be used IPAIPU3 class functions directly, but i\n>> think we can move the defVBlank_ to JM's introduction of\n>> IPASessionConfiguration.sensor structure\n>>\n>> With this plan i think we can drop these remaining private members from\n>> IPAIPU3.\n> Great, can easily be patches on top - doesn't need to be in this one.\n\n\nOk.\n\nI have been thinking to introduce similar control-not-found error \nchecks(as removed in this patch) in updateSessionConfiguration(), since \nwe ll end up querying the exposure, gain, vblank controls there /only/ . \nMight as well introduce the control-not-found error checks there.\n\n>\n> --\n> Kieran\n>\n>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>> \n>>>> /* Interface to the Camera Helper */\n>>>> std::unique_ptr camHelper_;\n>>>> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n>>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>>>> int32_t minExposure = v4l2Exposure.min().get();\n>>>> int32_t maxExposure = v4l2Exposure.max().get();\n>>>> + exposure_ = minExposure;\n>>>> \n>>>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>>> int32_t minGain = v4l2Gain.min().get();\n>>>> int32_t maxGain = v4l2Gain.max().get();\n>>>> + gain_ = minGain;\n>>>> \n>>>> /*\n>>>> * When the AGC computes the new exposure values for a frame, it needs\n>>>> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>>>> */\n>>>> ctrls_ = configInfo.sensorControls;\n>>>> \n>>>> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>>>> - if (itExp == ctrls_.end()) {\n>>>> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n>>>> - return -EINVAL;\n>>>> - }\n>>>> -\n>>>> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n>>>> - if (itGain == ctrls_.end()) {\n>>>> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n>>>> - return -EINVAL;\n>>>> - }\n>>>> -\n>>>> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n>>>> if (itVBlank == ctrls_.end()) {\n>>>> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n>>>> return -EINVAL;\n>>>> }\n>>>> \n>>>> - minExposure_ = itExp->second.min().get();\n>>>> - maxExposure_ = itExp->second.max().get();\n>>>> - exposure_ = minExposure_;\n>>>> -\n>>>> - minGain_ = itGain->second.min().get();\n>>>> - maxGain_ = itGain->second.max().get();\n>>>> - gain_ = minGain_;\n>>>> -\n>>>> defVBlank_ = itVBlank->second.def().get();\n>>>> \n>>>> calculateBdsGrid(configInfo.bdsOutputSize);\n>>>> -- \n>>>> 2.31.1\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 3AEB5BE08A\n\tfor ;\n\tThu, 3 Mar 2022 14:14:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C9B46115E;\n\tThu, 3 Mar 2022 15:14:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D01E1601FF\n\tfor ;\n\tThu, 3 Mar 2022 15:14:12 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9C298885;\n\tThu, 3 Mar 2022 15:14:11 +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=\"NN7pvmFW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646316852;\n\tbh=gF0JuGP4qoMEHP+7twPYAuVJ/5DAJLSSkb/37JVdyPs=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=NN7pvmFW0DEc29wGQ+XZeL44LSka2mmc4+IsQqeRh5dmFRc7umCitrGBMjqLoVYvZ\n\tiUDYP0ysxV+ElUSUzhifo7REJAxkha1H+MvJ43zxUZ9UcLzlQHdyX419wMdRZVDMEM\n\t+bKFKRCdVPGCGXnui8bYMtcHi7u7HvhTgut8Rs2s=","Message-ID":"","Date":"Thu, 3 Mar 2022 19:44:06 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>\n\t<164630803440.3471399.2692655364189567761@Monstersaurus>\n\t\n\t<164631640038.3492470.11256900790981757289@Monstersaurus>","From":"Umang Jain ","In-Reply-To":"<164631640038.3492470.11256900790981757289@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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":22222,"web_url":"https://patchwork.libcamera.org/comment/22222/","msgid":"<164631735575.3492470.15375691681593611409@Monstersaurus>","date":"2022-03-03T14:22:35","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2022-03-03 14:14:06)\n> Hi,\n> \n> On 3/3/22 19:36, Kieran Bingham wrote:\n> > Quoting Umang Jain (2022-03-03 12:15:44)\n> >> Hi Kieran,\n> >>\n> >> On 3/3/22 17:17, Kieran Bingham wrote:\n> >>> Quoting Umang Jain (2022-03-01 07:59:19)\n> >>>> The exposure and gain limits are required for AGC configuration\n> >>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n> >>>> already. Therefore the max/min private members in IPAIPU3 class for\n> >>>> exposure/gain serve no use except setting initial values of exposure_\n> >>>> and gain_ members.\n> >>>>\n> >>>> Drop the max/min private members from IPAIPU3 class and set initial\n> >>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n> >>> Great, even better to drop them if they aren't used.\n> >>>\n> >>> This looks like we're on the path to removing the private variables for\n> >>> controls from the IPAIPU3 now, as they should all be stored in the\n> >>> context if there's any relationship with the algorithms.\n> >>\n> >> yeah, we are on that path.\n> >>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>> Changes in v2:\n> >>>> - Split into separate patch\n> >>>> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n> >>>> - Don't include max/min exposure/gain members in\n> >>>> IPASessionConfiguration, as they aren't used anywhere.\n> >>>> ---\n> >>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n> >>>> 1 file changed, 2 insertions(+), 24 deletions(-)\n> >>>>\n> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>>> index 17324616..4b6852a7 100644\n> >>>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>>> @@ -167,11 +167,7 @@ private:\n> >>>> /* Camera sensor controls. */\n> >>>> uint32_t defVBlank_;\n> >>>> uint32_t exposure_;\n> >>>> - uint32_t minExposure_;\n> >>>> - uint32_t maxExposure_;\n> >>>> uint32_t gain_;\n> >>>> - uint32_t minGain_;\n> >>>> - uint32_t maxGain_;\n> >>> What's required to get rid of gain_, exposure_ and defVBlank_ from here\n> >>> too?\n> >>\n> >> we can drop gain_, exposure_ and replace them with local vars, no issue\n> >> in that.\n> >>\n> >> defVBlank_ seems to be used IPAIPU3 class functions directly, but i\n> >> think we can move the defVBlank_ to JM's introduction of\n> >> IPASessionConfiguration.sensor structure\n> >>\n> >> With this plan i think we can drop these remaining private members from\n> >> IPAIPU3.\n> > Great, can easily be patches on top - doesn't need to be in this one.\n> \n> \n> Ok.\n> \n> I have been thinking to introduce similar control-not-found error \n> checks(as removed in this patch) in updateSessionConfiguration(), since \n> we ll end up querying the exposure, gain, vblank controls there /only/ . \n> Might as well introduce the control-not-found error checks there.\n\nAha, I assumed the checks were already there.\n\nWe should certainly have checks that the controls are available,\notherwise it can crash when it tries to access them.\n\nI wonder if we can skip the checks if they are Mandatory Controls as\ndefined by the CameraSensor class though ...\n\n--\nKieran\n\n\n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> >>> --\n> >>> Kieran\n> >>>\n> >>>\n> >>>> \n> >>>> /* Interface to the Camera Helper */\n> >>>> std::unique_ptr camHelper_;\n> >>>> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> >>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> >>>> int32_t minExposure = v4l2Exposure.min().get();\n> >>>> int32_t maxExposure = v4l2Exposure.max().get();\n> >>>> + exposure_ = minExposure;\n> >>>> \n> >>>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> >>>> int32_t minGain = v4l2Gain.min().get();\n> >>>> int32_t maxGain = v4l2Gain.max().get();\n> >>>> + gain_ = minGain;\n> >>>> \n> >>>> /*\n> >>>> * When the AGC computes the new exposure values for a frame, it needs\n> >>>> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >>>> */\n> >>>> ctrls_ = configInfo.sensorControls;\n> >>>> \n> >>>> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> >>>> - if (itExp == ctrls_.end()) {\n> >>>> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n> >>>> - return -EINVAL;\n> >>>> - }\n> >>>> -\n> >>>> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> >>>> - if (itGain == ctrls_.end()) {\n> >>>> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n> >>>> - return -EINVAL;\n> >>>> - }\n> >>>> -\n> >>>> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n> >>>> if (itVBlank == ctrls_.end()) {\n> >>>> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n> >>>> return -EINVAL;\n> >>>> }\n> >>>> \n> >>>> - minExposure_ = itExp->second.min().get();\n> >>>> - maxExposure_ = itExp->second.max().get();\n> >>>> - exposure_ = minExposure_;\n> >>>> -\n> >>>> - minGain_ = itGain->second.min().get();\n> >>>> - maxGain_ = itGain->second.max().get();\n> >>>> - gain_ = minGain_;\n> >>>> -\n> >>>> defVBlank_ = itVBlank->second.def().get();\n> >>>> \n> >>>> calculateBdsGrid(configInfo.bdsOutputSize);\n> >>>> -- \n> >>>> 2.31.1\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 B4377BF415\n\tfor ;\n\tThu, 3 Mar 2022 14:22:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22F2061166;\n\tThu, 3 Mar 2022 15:22: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 7A2BA601FF\n\tfor ;\n\tThu, 3 Mar 2022 15:22:38 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1631F885;\n\tThu, 3 Mar 2022 15:22:38 +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=\"P8V54Prl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646317358;\n\tbh=Ov3r/N7CPcalxAAlsGyqSMen1ZTAFp1izdcxdM9h7cA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=P8V54PrldRUsWK/BI3XqdBWV6btc5wPrYTnUHptKyN8Ji4RwFh0WRh9ppAJ33/gyQ\n\t/zC2/8vrBoMp2QUaHxeVcKuWvtVqmSiaPxveGROyU9ahnLhj96OEaNtzRiZMgjEDds\n\tHgrf/KQOVVl4Zwud+ww99Wz5tGKmFrvmkBtFjoIg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>\n\t<164630803440.3471399.2692655364189567761@Monstersaurus>\n\t\n\t<164631640038.3492470.11256900790981757289@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Mar 2022 14:22:35 +0000","Message-ID":"<164631735575.3492470.15375691681593611409@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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":22224,"web_url":"https://patchwork.libcamera.org/comment/22224/","msgid":"<67732b09-cc82-a696-92fa-cad757ba9a3b@ideasonboard.com>","date":"2022-03-03T14:55:34","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 3/3/22 19:52, Kieran Bingham wrote:\n> Quoting Umang Jain (2022-03-03 14:14:06)\n>> Hi,\n>>\n>> On 3/3/22 19:36, Kieran Bingham wrote:\n>>> Quoting Umang Jain (2022-03-03 12:15:44)\n>>>> Hi Kieran,\n>>>>\n>>>> On 3/3/22 17:17, Kieran Bingham wrote:\n>>>>> Quoting Umang Jain (2022-03-01 07:59:19)\n>>>>>> The exposure and gain limits are required for AGC configuration\n>>>>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n>>>>>> already. Therefore the max/min private members in IPAIPU3 class for\n>>>>>> exposure/gain serve no use except setting initial values of exposure_\n>>>>>> and gain_ members.\n>>>>>>\n>>>>>> Drop the max/min private members from IPAIPU3 class and set initial\n>>>>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n>>>>> Great, even better to drop them if they aren't used.\n>>>>>\n>>>>> This looks like we're on the path to removing the private variables for\n>>>>> controls from the IPAIPU3 now, as they should all be stored in the\n>>>>> context if there's any relationship with the algorithms.\n>>>> yeah, we are on that path.\n>>>>\n>>>>> Reviewed-by: Kieran Bingham \n>>>>>\n>>>>>> Signed-off-by: Umang Jain \n>>>>>> ---\n>>>>>> Changes in v2:\n>>>>>> - Split into separate patch\n>>>>>> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n>>>>>> - Don't include max/min exposure/gain members in\n>>>>>> IPASessionConfiguration, as they aren't used anywhere.\n>>>>>> ---\n>>>>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n>>>>>> 1 file changed, 2 insertions(+), 24 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>>>> index 17324616..4b6852a7 100644\n>>>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>>>> @@ -167,11 +167,7 @@ private:\n>>>>>> /* Camera sensor controls. */\n>>>>>> uint32_t defVBlank_;\n>>>>>> uint32_t exposure_;\n>>>>>> - uint32_t minExposure_;\n>>>>>> - uint32_t maxExposure_;\n>>>>>> uint32_t gain_;\n>>>>>> - uint32_t minGain_;\n>>>>>> - uint32_t maxGain_;\n>>>>> What's required to get rid of gain_, exposure_ and defVBlank_ from here\n>>>>> too?\n>>>> we can drop gain_, exposure_ and replace them with local vars, no issue\n>>>> in that.\n>>>>\n>>>> defVBlank_ seems to be used IPAIPU3 class functions directly, but i\n>>>> think we can move the defVBlank_ to JM's introduction of\n>>>> IPASessionConfiguration.sensor structure\n>>>>\n>>>> With this plan i think we can drop these remaining private members from\n>>>> IPAIPU3.\n>>> Great, can easily be patches on top - doesn't need to be in this one.\n>>\n>> Ok.\n>>\n>> I have been thinking to introduce similar control-not-found error\n>> checks(as removed in this patch) in updateSessionConfiguration(), since\n>> we ll end up querying the exposure, gain, vblank controls there /only/ .\n>> Might as well introduce the control-not-found error checks there.\n> Aha, I assumed the checks were already there.\n>\n> We should certainly have checks that the controls are available,\n> otherwise it can crash when it tries to access them.\n>\n> I wonder if we can skip the checks if they are Mandatory Controls as\n> defined by the CameraSensor class though ...\n\n\nGood point, like on a general level throughout the code-base should we \nadd individuals checks for the mandatory controls ? or the mandatory \ncontrol check in CameraSensor is a good gate-keeper to rely on..\n\nBy the way, I checked that V4L2_CID_GAIN isn't mandatory ... so I guess \natleast for that, we need to.\n\n>\n> --\n> Kieran\n>\n>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>>> --\n>>>>> Kieran\n>>>>>\n>>>>>\n>>>>>> \n>>>>>> /* Interface to the Camera Helper */\n>>>>>> std::unique_ptr camHelper_;\n>>>>>> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n>>>>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>>>>>> int32_t minExposure = v4l2Exposure.min().get();\n>>>>>> int32_t maxExposure = v4l2Exposure.max().get();\n>>>>>> + exposure_ = minExposure;\n>>>>>> \n>>>>>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>>>>> int32_t minGain = v4l2Gain.min().get();\n>>>>>> int32_t maxGain = v4l2Gain.max().get();\n>>>>>> + gain_ = minGain;\n>>>>>> \n>>>>>> /*\n>>>>>> * When the AGC computes the new exposure values for a frame, it needs\n>>>>>> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>>>>>> */\n>>>>>> ctrls_ = configInfo.sensorControls;\n>>>>>> \n>>>>>> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>>>>>> - if (itExp == ctrls_.end()) {\n>>>>>> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n>>>>>> - return -EINVAL;\n>>>>>> - }\n>>>>>> -\n>>>>>> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n>>>>>> - if (itGain == ctrls_.end()) {\n>>>>>> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n>>>>>> - return -EINVAL;\n>>>>>> - }\n>>>>>> -\n>>>>>> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n>>>>>> if (itVBlank == ctrls_.end()) {\n>>>>>> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n>>>>>> return -EINVAL;\n>>>>>> }\n>>>>>> \n>>>>>> - minExposure_ = itExp->second.min().get();\n>>>>>> - maxExposure_ = itExp->second.max().get();\n>>>>>> - exposure_ = minExposure_;\n>>>>>> -\n>>>>>> - minGain_ = itGain->second.min().get();\n>>>>>> - maxGain_ = itGain->second.max().get();\n>>>>>> - gain_ = minGain_;\n>>>>>> -\n>>>>>> defVBlank_ = itVBlank->second.def().get();\n>>>>>> \n>>>>>> calculateBdsGrid(configInfo.bdsOutputSize);\n>>>>>> -- \n>>>>>> 2.31.1\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 6E04CBF415\n\tfor ;\n\tThu, 3 Mar 2022 14:55:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4A5F60475;\n\tThu, 3 Mar 2022 15:55:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6557C601FF\n\tfor ;\n\tThu, 3 Mar 2022 15:55:41 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 411C8885;\n\tThu, 3 Mar 2022 15:55:39 +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=\"QHnGvGC9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646319341;\n\tbh=W/l8UVxeGPcbKd7/vc7CfbFZv7+Ru3n/kn0MiK/gqio=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=QHnGvGC9SC2WeHmW4aVweiY9AfGSdrwDg5giZx7kC2vyBoNjTqA+aIvPr7Jli3QJJ\n\thMHWO3/87e73XadQmSrr1juCdJSL70ULlvExGH/XmaQFtBncNqgyEhxrW6pmNoBtQA\n\tp5ZB1ANXRXqflg5Q6lqMGP6DED+kW6PNACeuEVHI=","Message-ID":"<67732b09-cc82-a696-92fa-cad757ba9a3b@ideasonboard.com>","Date":"Thu, 3 Mar 2022 20:25:34 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>\n\t<164630803440.3471399.2692655364189567761@Monstersaurus>\n\t\n\t<164631640038.3492470.11256900790981757289@Monstersaurus>\n\t\n\t<164631735575.3492470.15375691681593611409@Monstersaurus>","From":"Umang Jain ","In-Reply-To":"<164631735575.3492470.15375691681593611409@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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":22225,"web_url":"https://patchwork.libcamera.org/comment/22225/","msgid":"<164632079580.3554540.2357496247594428912@Monstersaurus>","date":"2022-03-03T15:19:55","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2022-03-03 14:55:34)\n> Hi,\n> \n> >>>> On 3/3/22 17:17, Kieran Bingham wrote:\n> >>>>> Quoting Umang Jain (2022-03-01 07:59:19)\n> >>>>>> The exposure and gain limits are required for AGC configuration\n> >>>>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n> >>>>>> already. Therefore the max/min private members in IPAIPU3 class for\n> >>>>>> exposure/gain serve no use except setting initial values of exposure_\n> >>>>>> and gain_ members.\n> >>>>>>\n> >>>>>> Drop the max/min private members from IPAIPU3 class and set initial\n> >>>>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n> >>>>> Great, even better to drop them if they aren't used.\n> >>>>>\n> >>>>> This looks like we're on the path to removing the private variables for\n> >>>>> controls from the IPAIPU3 now, as they should all be stored in the\n> >>>>> context if there's any relationship with the algorithms.\n> >>>> yeah, we are on that path.\n> >>>>\n> >>>>> Reviewed-by: Kieran Bingham \n> >>>>>\n> >>>>>> Signed-off-by: Umang Jain \n> >>>>>> ---\n> >>>>>> Changes in v2:\n> >>>>>> - Split into separate patch\n> >>>>>> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n> >>>>>> - Don't include max/min exposure/gain members in\n> >>>>>> IPASessionConfiguration, as they aren't used anywhere.\n> >>>>>> ---\n> >>>>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n> >>>>>> 1 file changed, 2 insertions(+), 24 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>>>>> index 17324616..4b6852a7 100644\n> >>>>>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>>>>> @@ -167,11 +167,7 @@ private:\n> >>>>>> /* Camera sensor controls. */\n> >>>>>> uint32_t defVBlank_;\n> >>>>>> uint32_t exposure_;\n> >>>>>> - uint32_t minExposure_;\n> >>>>>> - uint32_t maxExposure_;\n> >>>>>> uint32_t gain_;\n> >>>>>> - uint32_t minGain_;\n> >>>>>> - uint32_t maxGain_;\n> >>>>> What's required to get rid of gain_, exposure_ and defVBlank_ from here\n> >>>>> too?\n> >>>> we can drop gain_, exposure_ and replace them with local vars, no issue\n> >>>> in that.\n> >>>>\n> >>>> defVBlank_ seems to be used IPAIPU3 class functions directly, but i\n> >>>> think we can move the defVBlank_ to JM's introduction of\n> >>>> IPASessionConfiguration.sensor structure\n> >>>>\n> >>>> With this plan i think we can drop these remaining private members from\n> >>>> IPAIPU3.\n> >>> Great, can easily be patches on top - doesn't need to be in this one.\n> >>\n> >> Ok.\n> >>\n> >> I have been thinking to introduce similar control-not-found error\n> >> checks(as removed in this patch) in updateSessionConfiguration(), since\n> >> we ll end up querying the exposure, gain, vblank controls there /only/ .\n> >> Might as well introduce the control-not-found error checks there.\n> > Aha, I assumed the checks were already there.\n> >\n> > We should certainly have checks that the controls are available,\n> > otherwise it can crash when it tries to access them.\n> >\n> > I wonder if we can skip the checks if they are Mandatory Controls as\n> > defined by the CameraSensor class though ...\n> \n> \n> Good point, like on a general level throughout the code-base should we \n> add individuals checks for the mandatory controls ? or the mandatory \n> control check in CameraSensor is a good gate-keeper to rely on..\n> \n> By the way, I checked that V4L2_CID_GAIN isn't mandatory ... so I guess \n> atleast for that, we need to.\n\nAhhh ... I think some sensors provide V4L2_CID_GAIN, and some provide\nV4L2_CID_ANALOGUE_GAIN ...\n\nThey /should/ provide V4L2_CID_ANALOGUE_GAIN ... but it's not\nconsistent. which makes me think this needs to be handled in the\nCameraSensor class, which makes me further feel that we need to move to\nusing libcamera controls between the IPA and the pipeline handler\nsooner. Then the translation from libcamera to v4l2 control would happen\nunder the CameraSensor class, and deal with preferring ANALOGUE_GAIN,\nbut falling back to GAIN if it exists....\n\n--\nKieran\n\n\n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> >>> --\n> >>> Kieran\n> >>>\n> >>>\n> >>>>> --\n> >>>>> Kieran\n> >>>>>\n> >>>>>\n> >>>>>> \n> >>>>>> /* Interface to the Camera Helper */\n> >>>>>> std::unique_ptr camHelper_;\n> >>>>>> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> >>>>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> >>>>>> int32_t minExposure = v4l2Exposure.min().get();\n> >>>>>> int32_t maxExposure = v4l2Exposure.max().get();\n> >>>>>> + exposure_ = minExposure;\n> >>>>>> \n> >>>>>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> >>>>>> int32_t minGain = v4l2Gain.min().get();\n> >>>>>> int32_t maxGain = v4l2Gain.max().get();\n> >>>>>> + gain_ = minGain;\n> >>>>>> \n> >>>>>> /*\n> >>>>>> * When the AGC computes the new exposure values for a frame, it needs\n> >>>>>> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >>>>>> */\n> >>>>>> ctrls_ = configInfo.sensorControls;\n> >>>>>> \n> >>>>>> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> >>>>>> - if (itExp == ctrls_.end()) {\n> >>>>>> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n> >>>>>> - return -EINVAL;\n> >>>>>> - }\n> >>>>>> -\n> >>>>>> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> >>>>>> - if (itGain == ctrls_.end()) {\n> >>>>>> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n> >>>>>> - return -EINVAL;\n> >>>>>> - }\n> >>>>>> -\n> >>>>>> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n> >>>>>> if (itVBlank == ctrls_.end()) {\n> >>>>>> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n> >>>>>> return -EINVAL;\n> >>>>>> }\n> >>>>>> \n> >>>>>> - minExposure_ = itExp->second.min().get();\n> >>>>>> - maxExposure_ = itExp->second.max().get();\n> >>>>>> - exposure_ = minExposure_;\n> >>>>>> -\n> >>>>>> - minGain_ = itGain->second.min().get();\n> >>>>>> - maxGain_ = itGain->second.max().get();\n> >>>>>> - gain_ = minGain_;\n> >>>>>> -\n> >>>>>> defVBlank_ = itVBlank->second.def().get();\n> >>>>>> \n> >>>>>> calculateBdsGrid(configInfo.bdsOutputSize);\n> >>>>>> -- \n> >>>>>> 2.31.1\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 C6087BE08A\n\tfor ;\n\tThu, 3 Mar 2022 15:20:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B9A26115E;\n\tThu, 3 Mar 2022 16:20:00 +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 BEF2D601FF\n\tfor ;\n\tThu, 3 Mar 2022 16:19:58 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5AB9E885;\n\tThu, 3 Mar 2022 16:19:58 +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=\"XMS9j4Yo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646320798;\n\tbh=ydJ5TfTcUzGnIPwC1U55h8/E1Ws6ChVn5xjpQuKHiKg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XMS9j4Yo17zh8V3tCfY1kozSP2CqDrkTLvRmq8T8GKnCNF8o3xDF4kc+kdE2ub9yz\n\tlqRPtpsnsuJZ0SgcZGTzn0ZWHZlkq/DV0ZfGDhNStdp9NyhgSrjr8bfEdgKdgw+J87\n\t3aHuMPgvys7sMnCXB7hcEurgOwCCKD2PaKEq/Nww=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<67732b09-cc82-a696-92fa-cad757ba9a3b@ideasonboard.com>","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>\n\t<164630803440.3471399.2692655364189567761@Monstersaurus>\n\t\n\t<164631640038.3492470.11256900790981757289@Monstersaurus>\n\t\n\t<164631735575.3492470.15375691681593611409@Monstersaurus>\n\t<67732b09-cc82-a696-92fa-cad757ba9a3b@ideasonboard.com>","From":"Kieran Bingham ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Mar 2022 15:19:55 +0000","Message-ID":"<164632079580.3554540.2357496247594428912@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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":22226,"web_url":"https://patchwork.libcamera.org/comment/22226/","msgid":"<420613df-f09c-91e2-24f2-f068bfa611c4@ideasonboard.com>","date":"2022-03-03T15:55:11","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 3/3/22 20:49, Kieran Bingham wrote:\n> Quoting Umang Jain (2022-03-03 14:55:34)\n>> Hi,\n>>\n>>>>>> On 3/3/22 17:17, Kieran Bingham wrote:\n>>>>>>> Quoting Umang Jain (2022-03-01 07:59:19)\n>>>>>>>> The exposure and gain limits are required for AGC configuration\n>>>>>>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening\n>>>>>>>> already. Therefore the max/min private members in IPAIPU3 class for\n>>>>>>>> exposure/gain serve no use except setting initial values of exposure_\n>>>>>>>> and gain_ members.\n>>>>>>>>\n>>>>>>>> Drop the max/min private members from IPAIPU3 class and set initial\n>>>>>>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration().\n>>>>>>> Great, even better to drop them if they aren't used.\n>>>>>>>\n>>>>>>> This looks like we're on the path to removing the private variables for\n>>>>>>> controls from the IPAIPU3 now, as they should all be stored in the\n>>>>>>> context if there's any relationship with the algorithms.\n>>>>>> yeah, we are on that path.\n>>>>>>\n>>>>>>> Reviewed-by: Kieran Bingham \n>>>>>>>\n>>>>>>>> Signed-off-by: Umang Jain \n>>>>>>>> ---\n>>>>>>>> Changes in v2:\n>>>>>>>> - Split into separate patch\n>>>>>>>> - Prepped over \"v4 ipa: ipu3: Misc clean up\"\n>>>>>>>> - Don't include max/min exposure/gain members in\n>>>>>>>> IPASessionConfiguration, as they aren't used anywhere.\n>>>>>>>> ---\n>>>>>>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------\n>>>>>>>> 1 file changed, 2 insertions(+), 24 deletions(-)\n>>>>>>>>\n>>>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>>>>>> index 17324616..4b6852a7 100644\n>>>>>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>>>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>>>>>> @@ -167,11 +167,7 @@ private:\n>>>>>>>> /* Camera sensor controls. */\n>>>>>>>> uint32_t defVBlank_;\n>>>>>>>> uint32_t exposure_;\n>>>>>>>> - uint32_t minExposure_;\n>>>>>>>> - uint32_t maxExposure_;\n>>>>>>>> uint32_t gain_;\n>>>>>>>> - uint32_t minGain_;\n>>>>>>>> - uint32_t maxGain_;\n>>>>>>> What's required to get rid of gain_, exposure_ and defVBlank_ from here\n>>>>>>> too?\n>>>>>> we can drop gain_, exposure_ and replace them with local vars, no issue\n>>>>>> in that.\n>>>>>>\n>>>>>> defVBlank_ seems to be used IPAIPU3 class functions directly, but i\n>>>>>> think we can move the defVBlank_ to JM's introduction of\n>>>>>> IPASessionConfiguration.sensor structure\n>>>>>>\n>>>>>> With this plan i think we can drop these remaining private members from\n>>>>>> IPAIPU3.\n>>>>> Great, can easily be patches on top - doesn't need to be in this one.\n>>>> Ok.\n>>>>\n>>>> I have been thinking to introduce similar control-not-found error\n>>>> checks(as removed in this patch) in updateSessionConfiguration(), since\n>>>> we ll end up querying the exposure, gain, vblank controls there /only/ .\n>>>> Might as well introduce the control-not-found error checks there.\n>>> Aha, I assumed the checks were already there.\n>>>\n>>> We should certainly have checks that the controls are available,\n>>> otherwise it can crash when it tries to access them.\n>>>\n>>> I wonder if we can skip the checks if they are Mandatory Controls as\n>>> defined by the CameraSensor class though ...\n>>\n>> Good point, like on a general level throughout the code-base should we\n>> add individuals checks for the mandatory controls ? or the mandatory\n>> control check in CameraSensor is a good gate-keeper to rely on..\n>>\n>> By the way, I checked that V4L2_CID_GAIN isn't mandatory ... so I guess\n>> atleast for that, we need to.\n> Ahhh ... I think some sensors provide V4L2_CID_GAIN, and some provide\n> V4L2_CID_ANALOGUE_GAIN ...\n>\n> They /should/ provide V4L2_CID_ANALOGUE_GAIN ... but it's not\n> consistent. which makes me think this needs to be handled in the\n> CameraSensor class, which makes me further feel that we need to move to\n> using libcamera controls between the IPA and the pipeline handler\n> sooner. Then the translation from libcamera to v4l2 control would happen\n> under the CameraSensor class, and deal with preferring ANALOGUE_GAIN,\n> but falling back to GAIN if it exists....\n\nFurther discussion needed for this maybe... i.e. the gains\n\nFor earlier part of this thread,  I added 2 patches to summarise on this \nparent patch itself.\n\n\n>\n> --\n> Kieran\n>\n>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>>> --\n>>>>> Kieran\n>>>>>\n>>>>>\n>>>>>>> --\n>>>>>>> Kieran\n>>>>>>>\n>>>>>>>\n>>>>>>>> \n>>>>>>>> /* Interface to the Camera Helper */\n>>>>>>>> std::unique_ptr camHelper_;\n>>>>>>>> @@ -192,10 +188,12 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n>>>>>>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>>>>>>>> int32_t minExposure = v4l2Exposure.min().get();\n>>>>>>>> int32_t maxExposure = v4l2Exposure.max().get();\n>>>>>>>> + exposure_ = minExposure;\n>>>>>>>> \n>>>>>>>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>>>>>>> int32_t minGain = v4l2Gain.min().get();\n>>>>>>>> int32_t maxGain = v4l2Gain.max().get();\n>>>>>>>> + gain_ = minGain;\n>>>>>>>> \n>>>>>>>> /*\n>>>>>>>> * When the AGC computes the new exposure values for a frame, it needs\n>>>>>>>> @@ -429,32 +427,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>>>>>>>> */\n>>>>>>>> ctrls_ = configInfo.sensorControls;\n>>>>>>>> \n>>>>>>>> - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>>>>>>>> - if (itExp == ctrls_.end()) {\n>>>>>>>> - LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n>>>>>>>> - return -EINVAL;\n>>>>>>>> - }\n>>>>>>>> -\n>>>>>>>> - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n>>>>>>>> - if (itGain == ctrls_.end()) {\n>>>>>>>> - LOG(IPAIPU3, Error) << \"Can't find gain control\";\n>>>>>>>> - return -EINVAL;\n>>>>>>>> - }\n>>>>>>>> -\n>>>>>>>> const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);\n>>>>>>>> if (itVBlank == ctrls_.end()) {\n>>>>>>>> LOG(IPAIPU3, Error) << \"Can't find VBLANK control\";\n>>>>>>>> return -EINVAL;\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - minExposure_ = itExp->second.min().get();\n>>>>>>>> - maxExposure_ = itExp->second.max().get();\n>>>>>>>> - exposure_ = minExposure_;\n>>>>>>>> -\n>>>>>>>> - minGain_ = itGain->second.min().get();\n>>>>>>>> - maxGain_ = itGain->second.max().get();\n>>>>>>>> - gain_ = minGain_;\n>>>>>>>> -\n>>>>>>>> defVBlank_ = itVBlank->second.def().get();\n>>>>>>>> \n>>>>>>>> calculateBdsGrid(configInfo.bdsOutputSize);\n>>>>>>>> -- \n>>>>>>>> 2.31.1\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 1B77EBE08A\n\tfor ;\n\tThu, 3 Mar 2022 15:55:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7103C61160;\n\tThu, 3 Mar 2022 16:55: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 E00D4601FF\n\tfor ;\n\tThu, 3 Mar 2022 16:55:18 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C4C9C885;\n\tThu, 3 Mar 2022 16:55:17 +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=\"nlzSX44l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646322918;\n\tbh=+SKwFX8wQxq3XP7VfoB8HyDE+tWXGW/WZcueukFl3Zo=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=nlzSX44lL9DKVudq9XLKzO6QL9ktLFB7CqhUfmDFgLrjBcgNMJscxKwBw1VTTf96H\n\tiR2jRGnPckdRdddSk9cIvDLV279I7qNnWySPQpDdaIDtSkmHGPVT6RONBjUDcMGHnj\n\tia+3QGQTT5hWuIrDLkQwzd3060dC1FZXxI0+gPak=","Message-ID":"<420613df-f09c-91e2-24f2-f068bfa611c4@ideasonboard.com>","Date":"Thu, 3 Mar 2022 21:25:11 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220301075919.201968-1-umang.jain@ideasonboard.com>\n\t<164630803440.3471399.2692655364189567761@Monstersaurus>\n\t\n\t<164631640038.3492470.11256900790981757289@Monstersaurus>\n\t\n\t<164631735575.3492470.15375691681593611409@Monstersaurus>\n\t<67732b09-cc82-a696-92fa-cad757ba9a3b@ideasonboard.com>\n\t<164632079580.3554540.2357496247594428912@Monstersaurus>","From":"Umang Jain ","In-Reply-To":"<164632079580.3554540.2357496247594428912@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of\n\texposure and gain limits","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\" "}}]