[{"id":22233,"web_url":"https://patchwork.libcamera.org/comment/22233/","msgid":"<164682727526.856468.13332092719988798974@Monstersaurus>","date":"2022-03-09T12:01:15","subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: ipu3: Drop sensor\n\tcontrols private members from IPAIPU3","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Quoting Umang Jain via libcamera-devel (2022-03-05 17:28:48)\n> Drop exposure, gain private members from IPAIPU3 because the values\n> are handled directly via IPAFrameContext.\n> \n> Move the default vblank value from IPAIPU3 to IPASessionContext\n> structure as it is a default static value not expected to change\n> for a session.\n\nI think the vblank can be updated if the frame rate changes. But that\nisn't something that happens now.\n\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/ipa_context.h | 1 +\n> src/ipa/ipu3/ipu3.cpp | 29 +++++++++--------------------\n> 2 files changed, 10 insertions(+), 20 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index e7c49828..d993359a 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -33,6 +33,7 @@ struct IPASessionConfiguration {\n> } agc;\n> \n> struct {\n> + int32_t defVBlank;\n\nI think I'd be tempted to call this vBlank rather than defVBlank.\n\nIn here it's not the 'default' vblank, it's the current/active vblank.\n\nIf it gets updated, (which can happen I think) then it should be updated\nhere. But I don't think this patch needs to handle updating it.\n\n\nI think with that:\n\nReviewed-by: Kieran Bingham \n\n\n> utils::Duration lineDuration;\n> } sensor;\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 4b6852a7..0df358ba 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -164,11 +164,6 @@ private:\n> \n> IPACameraSensorInfo sensorInfo_;\n> \n> - /* Camera sensor controls. */\n> - uint32_t defVBlank_;\n> - uint32_t exposure_;\n> - uint32_t gain_;\n> -\n> /* Interface to the Camera Helper */\n> std::unique_ptr camHelper_;\n> \n> @@ -185,15 +180,16 @@ private:\n> */\n> void 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> +\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> @@ -427,14 +423,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> */\n> ctrls_ = configInfo.sensorControls;\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> - defVBlank_ = itVBlank->second.def().get();\n> -\n> calculateBdsGrid(configInfo.bdsOutputSize);\n> \n> /* Clean frameContext at each reconfiguration. */\n> @@ -600,6 +588,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> const ipu3_uapi_stats_3a *stats)\n> {\n> double lineDuration = context_.configuration.sensor.lineDuration.get();\n> + int32_t vBlank = context_.configuration.sensor.defVBlank;\n> ControlList ctrls(controls::controls);\n> \n> for (auto const &algo : algorithms_)\n> @@ -608,7 +597,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> setControls(frame);\n> \n> /* \\todo Use VBlank value calculated from each frame exposure. */\n> - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration;\n> + int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;\n> ctrls.set(controls::FrameDuration, frameDuration);\n> \n> ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);\n> @@ -644,12 +633,12 @@ void IPAIPU3::setControls(unsigned int frame)\n> IPU3Action op;\n> op.op = ActionSetSensorControls;\n> \n> - exposure_ = context_.frameContext.agc.exposure;\n> - gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n> + int32_t exposure = context_.frameContext.agc.exposure;\n> + int32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n> \n> ControlList ctrls(ctrls_);\n> - ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> + ctrls.set(V4L2_CID_EXPOSURE, exposure);\n> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> op.sensorControls = ctrls;\n> \n> queueFrameAction.emit(frame, op);\n> -- \n> 2.31.0\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 90D1DBE08A\n\tfor ;\n\tWed, 9 Mar 2022 12:01:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C13566118B;\n\tWed, 9 Mar 2022 13:01: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 0EBFD60475\n\tfor ;\n\tWed, 9 Mar 2022 13:01:18 +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 91D758C4;\n\tWed, 9 Mar 2022 13:01:17 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646827278;\n\tbh=wQlJkPDqPmLwcfu7g73FPy14HIdy8j+oerU5Zikx04Q=;\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=snVOAVwq4+QSfMetrEUvGB4SKLW6W+EXgV6QAJi8bLU9a3bvzdzxaSYgKu5yQZKmQ\n\t6bk0RhlGeBk21a4lZ8qyLdPmivJujHmrPau1PXTbZl+NP7ntSFa+AcbB/bzj/Jsl8G\n\t9OZzk8YhOtXZYds89p7TKKzIPw1cfenCAxtJeRTI5fNtWziWtz/1FEZW/9Gyzd06hu\n\tAXhRDJCIJQeiRMoycWObE/0ORXE6fSo2rhURVeLcRfoDZecdjnOaOPWxrhVojaARLj\n\t5O0HJLdeEu4aKaDPxGVCxhk/inZQKt/PAW9ajtDqww6i/vt8rhIyzckU3RRd7W3RZ2\n\tVFXDJSpEco0OA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646827277;\n\tbh=wQlJkPDqPmLwcfu7g73FPy14HIdy8j+oerU5Zikx04Q=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=nwPGJ/E3e6r+Gp2C+3aCg+IGYgKcrK/xRieWZAzQeI914XIzFiKYAHjXICf3vAG8X\n\tFi+pwJTD+ozokZXC8GjuS4bQFJBxIsCeqgPsSIRKfpyx8vL88rMXkcJW8Rx1dpMo+p\n\t+VSiVtn4YfSQUaEKUtdcDzWSm2t4pUR+TIUu72no="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nwPGJ/E3\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220305172849.1022923-3-umang.jain@ideasonboard.com>","References":"<20220305172849.1022923-1-umang.jain@ideasonboard.com>\n\t<20220305172849.1022923-3-umang.jain@ideasonboard.com>","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 09 Mar 2022 12:01:15 +0000","Message-ID":"<164682727526.856468.13332092719988798974@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: ipu3: Drop sensor\n\tcontrols private members from IPAIPU3","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\" "}},{"id":22259,"web_url":"https://patchwork.libcamera.org/comment/22259/","msgid":"","date":"2022-03-11T09:27:07","subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: ipu3: Drop sensor\n\tcontrols private members from IPAIPU3","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Kirean,\n\nOn 3/9/22 17:31, Kieran Bingham wrote:\n> Quoting Umang Jain via libcamera-devel (2022-03-05 17:28:48)\n>> Drop exposure, gain private members from IPAIPU3 because the values\n>> are handled directly via IPAFrameContext.\n>>\n>> Move the default vblank value from IPAIPU3 to IPASessionContext\n>> structure as it is a default static value not expected to change\n>> for a session.\n> I think the vblank can be updated if the frame rate changes. But that\n> isn't something that happens now.\n\n\nyeah, but also, it shouldn't be a part of IPASessionConfiguration. \nNotice I named the member as 'def'Vblank; so a changing \nvblank/frame-rate-oriented can be separately introduced.\n\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/ipa/ipu3/ipa_context.h | 1 +\n>> src/ipa/ipu3/ipu3.cpp | 29 +++++++++--------------------\n>> 2 files changed, 10 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index e7c49828..d993359a 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -33,6 +33,7 @@ struct IPASessionConfiguration {\n>> } agc;\n>> \n>> struct {\n>> + int32_t defVBlank;\n> I think I'd be tempted to call this vBlank rather than defVBlank.\n>\n> In here it's not the 'default' vblank, it's the current/active vblank.\n\n\nah, I answered this above.\n\n>\n> If it gets updated, (which can happen I think) then it should be updated\n> here. But I don't think this patch needs to handle updating it.\n>\n>\n> I think with that:\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n>> utils::Duration lineDuration;\n>> } sensor;\n>> };\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 4b6852a7..0df358ba 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -164,11 +164,6 @@ private:\n>> \n>> IPACameraSensorInfo sensorInfo_;\n>> \n>> - /* Camera sensor controls. */\n>> - uint32_t defVBlank_;\n>> - uint32_t exposure_;\n>> - uint32_t gain_;\n>> -\n>> /* Interface to the Camera Helper */\n>> std::unique_ptr camHelper_;\n>> \n>> @@ -185,15 +180,16 @@ private:\n>> */\n>> void 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>> +\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>> @@ -427,14 +423,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>> */\n>> ctrls_ = configInfo.sensorControls;\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>> - defVBlank_ = itVBlank->second.def().get();\n>> -\n>> calculateBdsGrid(configInfo.bdsOutputSize);\n>> \n>> /* Clean frameContext at each reconfiguration. */\n>> @@ -600,6 +588,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>> const ipu3_uapi_stats_3a *stats)\n>> {\n>> double lineDuration = context_.configuration.sensor.lineDuration.get();\n>> + int32_t vBlank = context_.configuration.sensor.defVBlank;\n>> ControlList ctrls(controls::controls);\n>> \n>> for (auto const &algo : algorithms_)\n>> @@ -608,7 +597,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>> setControls(frame);\n>> \n>> /* \\todo Use VBlank value calculated from each frame exposure. */\n>> - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration;\n>> + int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;\n>> ctrls.set(controls::FrameDuration, frameDuration);\n>> \n>> ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);\n>> @@ -644,12 +633,12 @@ void IPAIPU3::setControls(unsigned int frame)\n>> IPU3Action op;\n>> op.op = ActionSetSensorControls;\n>> \n>> - exposure_ = context_.frameContext.agc.exposure;\n>> - gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n>> + int32_t exposure = context_.frameContext.agc.exposure;\n>> + int32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n>> \n>> ControlList ctrls(ctrls_);\n>> - ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n>> - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n>> + ctrls.set(V4L2_CID_EXPOSURE, exposure);\n>> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n>> op.sensorControls = ctrls;\n>> \n>> queueFrameAction.emit(frame, op);\n>> -- \n>> 2.31.0\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 8C7F2BE08A\n\tfor ;\n\tFri, 11 Mar 2022 09:27:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98CC16118A;\n\tFri, 11 Mar 2022 10:27:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5E7F604E6\n\tfor ;\n\tFri, 11 Mar 2022 10:27:12 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.65])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E45D488;\n\tFri, 11 Mar 2022 10:27:11 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646990834;\n\tbh=fOAq4sKk2RQX8SZO7BImdjrwnKmIeV8hY3B7FT5Yls0=;\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:\n\tFrom;\n\tb=pJ9Nw/5EKpP7AFBzY1Q+EIlSwDa2avE7t0rxixR6H1Y3eTzMDYrB8TEj2gyfPqqYX\n\tQUTMLp/jsgYLoWVNpdmUpmmaGDMqUUDE3fqit/wlJ80ra8LgHn/39/7Mmu1HBMZQo0\n\t3x58oCBD18uEZ5FQ5HAEkdkVK1b1m3nkgAMAl7IVXhUoNAzmwgqgaw+VpDvwE/cDN/\n\tGwVKIJDZuzPMm64BdI9P6aVzKyZobunud7rYax/nY8k+nYzNlduTMVARQdSAGc1x36\n\tDREd7+0+ucHSktQh0mVfqj0m258mQ/wf8oA64ZCMtMN2vAcnFdvmOk8DZQTD+ambpc\n\tHXj5AbqlLdRgA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646990832;\n\tbh=fOAq4sKk2RQX8SZO7BImdjrwnKmIeV8hY3B7FT5Yls0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=rXjeUrsWofzteguDWWwzCv7ahkzCp0ZGkEXWESoRxq61sq5etznelxQPZfZ1d+IWu\n\t7iZPBg7lUKglSY7wnRlEHxp8ASZWWtSHYQhOWz7mJ1CYbIujaRc9DmAVBuBNX/vBIO\n\tooQHlq/viiJKo/xqGpnD7N1GNaJh59zcIBqjHEfI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rXjeUrsW\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 11 Mar 2022 14:57:07 +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":"<20220305172849.1022923-1-umang.jain@ideasonboard.com>\n\t<20220305172849.1022923-3-umang.jain@ideasonboard.com>\n\t<164682727526.856468.13332092719988798974@Monstersaurus>","In-Reply-To":"<164682727526.856468.13332092719988798974@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: ipu3: Drop sensor\n\tcontrols private members from IPAIPU3","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]