[{"id":22439,"web_url":"https://patchwork.libcamera.org/comment/22439/","msgid":"","date":"2022-03-24T15:57:50","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Ensure controls\n\texists in updateSessionConfiguration()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Thu, Mar 24, 2022 at 07:50:35PM +0530, Umang Jain via libcamera-devel wrote:\n> Add a control-not-found error checking for the controls being\n> queried in IPAIPU3::updateSessionConfiguration(). If the control\n> is not found, progagate the error back to the caller. This requires\n> a change in the function signature of private member function\n> IPAIPU3::updateSessionConfiguration().\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/ipu3.cpp | 44 +++++++++++++++++++++++++++++++------------\n> 1 file changed, 32 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 3717d893..66346728 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -149,7 +149,7 @@ private:\n> \tvoid updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t const ControlInfoMap &sensorControls,\n> \t\t\t ControlInfoMap *ipaControls);\n> -\tvoid updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> +\tint updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> \tvoid processControls(unsigned int frame, const ControlList &controls);\n> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> \tvoid parseStatistics(unsigned int frame,\n> @@ -180,18 +180,33 @@ private:\n> * \\brief Compute IPASessionConfiguration using the sensor information and the\n> * sensor V4L2 controls\n> */\n> -void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> +int IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> {\n> -\tconst ControlInfo vBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> -\tcontext_.configuration.sensor.defVBlank = vBlank.def().get();\n> +\tconst auto itVBlank = sensorControls.find(V4L2_CID_VBLANK);\n> +\tif (itVBlank == sensorControls.end()) {\n> +\t\tLOG(IPAIPU3, Error) << \"Can't find vblank control\";\n> +\t\treturn -EINVAL;\n> +\t}\n\nThis answers my question in 2/3 :-)\n\nI think it's good to be defensive.\n\nReviewed-by: Laurent Pinchart \n\n> \n> -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> -\tint32_t minExposure = v4l2Exposure.min().get();\n> -\tint32_t maxExposure = v4l2Exposure.max().get();\n> +\tcontext_.configuration.sensor.defVBlank = itVBlank->second.def().get();\n> +\n> +\tconst auto itExp = sensorControls.find(V4L2_CID_EXPOSURE);\n> +\tif (itExp == sensorControls.end()) {\n> +\t\tLOG(IPAIPU3, Error) << \"Can't find exposure control\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tint32_t minExposure = itExp->second.min().get();\n> +\tint32_t maxExposure = itExp->second.max().get();\n> \n> -\tconst ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> -\tint32_t minGain = v4l2Gain.min().get();\n> -\tint32_t maxGain = v4l2Gain.max().get();\n> +\tconst auto itGain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (itGain == sensorControls.end()) {\n> +\t\tLOG(IPAIPU3, Error) << \"Can't find analogue gain control\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tint32_t minGain = itGain->second.min().get();\n> +\tint32_t maxGain = itGain->second.max().get();\n> \n> \t/*\n> \t * When the AGC computes the new exposure values for a frame, it needs\n> @@ -204,6 +219,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> \tcontext_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;\n> \tcontext_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> \tcontext_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -437,10 +454,13 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \tupdateControls(sensorInfo_, sensorCtrls_, ipaControls);\n> \n> \t/* Update the IPASessionConfiguration using the sensor settings. */\n> -\tupdateSessionConfiguration(sensorCtrls_);\n> +\tint ret = updateSessionConfiguration(sensorCtrls_);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> \n> \tfor (auto const &algo : algorithms_) {\n> -\t\tint ret = algo->configure(context_, configInfo);\n> +\t\tret = algo->configure(context_, configInfo);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t}","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 D15D4C0F1B\n\tfor ;\n\tThu, 24 Mar 2022 15:57:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39D71604D5;\n\tThu, 24 Mar 2022 16:57:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F7C560397\n\tfor ;\n\tThu, 24 Mar 2022 16:57:52 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E793B1852;\n\tThu, 24 Mar 2022 16:57:51 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648137474;\n\tbh=0PDg8nn9vgwYFJX97ATimzeWLcDGvOYB0hiG8XMfc+w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ruyltp/eJ45NfQkF+ZuyAU6OGLpMhW0UWPQJf1Uht/KQ2TJ8iNy85cOEgmTKCcKBT\n\t8gsF0lUKVQ8bJYQY60EciLkkFxWDUo2Gr9S+kz9yI2Ddhfi9KSrr3u0it+0C6G1Mzi\n\tCjFugYfOXWBk2IbYCdsXJH3wrG7qVTjTWpqHyiAfPhA14iJfJO1ahh+iVpfjIkjdty\n\tXEZr/eLj8culs2vKS/f2RNAgFkpF6ICNS/F/FCgqeEn2odjuqUc94cbjDp6PC/empt\n\tiqeZ+h16VMHWiGEwmZG84zCofsLW5Zz8EfjMxvMaFSVbeQejCnl2jhHb5075ui8ihg\n\t9v8bKyofKK8FA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648137472;\n\tbh=0PDg8nn9vgwYFJX97ATimzeWLcDGvOYB0hiG8XMfc+w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WVGpjlRvXGRwEWI7gUCOLQdzVeg82NjViIdcTTBSqrWRrNTQqPQjTSnxsrRg98IcL\n\tWVI5Dod+G1cwuxCqz/pII5WPSADeNHoGEnbPnmOwiRRHHsICrH3nDe5kypAAMfiv3r\n\tzA/Wd6+3wfum62xpeSQ9GeBTxrRYWYwBJM0XKtbI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WVGpjlRv\"; dkim-atps=neutral","Date":"Thu, 24 Mar 2022 17:57:50 +0200","To":"Umang Jain ","Message-ID":"","References":"<20220324142035.47338-1-umang.jain@ideasonboard.com>\n\t<20220324142035.47338-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220324142035.47338-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Ensure controls\n\texists in updateSessionConfiguration()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22448,"web_url":"https://patchwork.libcamera.org/comment/22448/","msgid":"<164819933474.1783612.3187149589611292301@Monstersaurus>","date":"2022-03-25T09:08:54","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Ensure controls\n\texists in updateSessionConfiguration()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nQuoting Umang Jain via libcamera-devel (2022-03-24 14:20:35)\n> Add a control-not-found error checking for the controls being\n> queried in IPAIPU3::updateSessionConfiguration(). If the control\n> is not found, progagate the error back to the caller. This requires\n> a change in the function signature of private member function\n> IPAIPU3::updateSessionConfiguration().\n\nIn RPi IPA this is handled with a dedicated function that can check a\nlist of controls, which helps make it extendable and keep the list of\nrequired controls centralised.\n\nLook at\n IPARPi::validateSensorControls()\n IPARPi::validateIspControls()\n IPARPi::validateLensControls()\n\nA loop or dedicated validate function might make sense, but I think this\nis also valid. At least this implementation means once the ctrl is\nfound, it's used without having to search again later.\n\nSo either way:\n\nReviewed-by: Kieran Bingham \n\n\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/ipu3.cpp | 44 +++++++++++++++++++++++++++++++------------\n> 1 file changed, 32 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 3717d893..66346728 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -149,7 +149,7 @@ private:\n> void updateControls(const IPACameraSensorInfo &sensorInfo,\n> const ControlInfoMap &sensorControls,\n> ControlInfoMap *ipaControls);\n> - void updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> + int updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> void processControls(unsigned int frame, const ControlList &controls);\n> void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> void parseStatistics(unsigned int frame,\n> @@ -180,18 +180,33 @@ private:\n> * \\brief Compute IPASessionConfiguration using the sensor information and the\n> * sensor V4L2 controls\n> */\n> -void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> +int IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> {\n> - const ControlInfo vBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> - context_.configuration.sensor.defVBlank = vBlank.def().get();\n> + const auto itVBlank = sensorControls.find(V4L2_CID_VBLANK);\n> + if (itVBlank == sensorControls.end()) {\n> + LOG(IPAIPU3, Error) << \"Can't find vblank control\";\n> + return -EINVAL;\n> + }\n> \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> + context_.configuration.sensor.defVBlank = itVBlank->second.def().get();\n> +\n> + const auto itExp = sensorControls.find(V4L2_CID_EXPOSURE);\n> + if (itExp == sensorControls.end()) {\n> + LOG(IPAIPU3, Error) << \"Can't find exposure control\";\n> + return -EINVAL;\n> + }\n> +\n> + int32_t minExposure = itExp->second.min().get();\n> + int32_t maxExposure = itExp->second.max().get();\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> + const auto itGain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);\n> + if (itGain == sensorControls.end()) {\n> + LOG(IPAIPU3, Error) << \"Can't find analogue gain control\";\n> + return -EINVAL;\n> + }\n> +\n> + int32_t minGain = itGain->second.min().get();\n> + int32_t maxGain = itGain->second.max().get();\n> \n> /*\n> * When the AGC computes the new exposure values for a frame, it needs\n> @@ -204,6 +219,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;\n> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> +\n> + return 0;\n> }\n> \n> /**\n> @@ -437,10 +454,13 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> updateControls(sensorInfo_, sensorCtrls_, ipaControls);\n> \n> /* Update the IPASessionConfiguration using the sensor settings. */\n> - updateSessionConfiguration(sensorCtrls_);\n> + int ret = updateSessionConfiguration(sensorCtrls_);\n> + if (ret < 0)\n> + return ret;\n> +\n> \n> for (auto const &algo : algorithms_) {\n> - int ret = algo->configure(context_, configInfo);\n> + ret = algo->configure(context_, configInfo);\n> if (ret)\n> return ret;\n> }\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 7F397C0F1B\n\tfor ;\n\tFri, 25 Mar 2022 09:09:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ABBA2604D4;\n\tFri, 25 Mar 2022 10:08:59 +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 3887A60136\n\tfor ;\n\tFri, 25 Mar 2022 10:08: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 B932B6F3;\n\tFri, 25 Mar 2022 10:08:57 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648199339;\n\tbh=8J8xY9qhSzsrmGytX3IGct4cZRCCRoYjMXf+aDc1t4s=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=qKIWoE6SQJ+brUDIqP2wv7gPdaFAH8u1ESx5EwVy4fncrcosNzRIee6fgbEmuhFYp\n\t66+clvrGhO/8Qw7eLe02ahE5WaxYS61+ViFMdZUJWSZ1jauuztpoX7kIseuxAE8osc\n\t5w+VT11AawHlLxlsjAozDEb6CpUTnydMity4y13VwpZTCuQ+U34+Qb9KpHWCFzbJUA\n\tWy0zdTLPYe4/n+P3GxRnvaf5MkgNo0RLdDa+xwdGFUD1wyZYR15HUfdHt9ANjjCyvR\n\tSSbNMzNxzDzpIpb/HiR4QcdsYQVJRoviCbpE4UjMbqmp+z1WDFnhZ31dcehXVYI3/e\n\t1dpIhXdiL3BTQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648199337;\n\tbh=8J8xY9qhSzsrmGytX3IGct4cZRCCRoYjMXf+aDc1t4s=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ukP7Aeq3bv4D+fvqQ3X+DaH5ehtxdrs6kTCrBWtVaOJPhfg2g/gpBVQa3pqQzIxWD\n\tg8h14r6aIE7gvX3O9bh3KFQgfy/ie5d6t3ZEvCb/6MIawcCumxYsNc6dR2LyZQ+wY7\n\trBlgPFRslaVqvXsWtPPKgC5QhLfCJPOo/dwlGPvg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ukP7Aeq3\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220324142035.47338-4-umang.jain@ideasonboard.com>","References":"<20220324142035.47338-1-umang.jain@ideasonboard.com>\n\t<20220324142035.47338-4-umang.jain@ideasonboard.com>","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 25 Mar 2022 09:08:54 +0000","Message-ID":"<164819933474.1783612.3187149589611292301@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Ensure controls\n\texists in updateSessionConfiguration()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]