[{"id":19253,"web_url":"https://patchwork.libcamera.org/comment/19253/","msgid":"<20210901083357.GW968527@pyrite.rasen.tech>","date":"2021-09-01T08:33:57","subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Aug 27, 2021 at 02:07:57PM +0200, Jacopo Mondi wrote:\n> Limit the IPU3 frame rate to 30 FPS.\n> \n> The reason to do is to bring the IPU3 IPA in par with the Intel\n> HAL implementation on IPU3 platform, where 30FPS is the frame rate used\n> to perform quality tuning in the closed-source IPA module and has been\n> validated as the most efficient rate for the power/performace budget.\n> \n> Compute the vertical blanking to maintain such frame rate and configure\n> the sensor with that.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/ipu3/ipu3.cpp | 37 ++++++++++++++++++++++++++++++++-----\n> 1 file changed, 32 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index fc5f69ed5ddc..0e5d5e479e20 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -183,7 +183,7 @@ private:\n> \tIPACameraSensorInfo sensorInfo_;\n> \n> \t/* Camera sensor controls. */\n> -\tuint32_t defVBlank_;\n> +\tuint32_t vBlank_;\n> \tuint32_t exposure_;\n> \tuint32_t minExposure_;\n> \tuint32_t maxExposure_;\n> @@ -257,10 +257,39 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> \t}\n> \n> +\t/*\n> +\t * Cap minimum frame duration to 30FPS.\n> +\t *\n> +\t * 30 FPS has been validated in the closed source Intel 3A module as the\n> +\t * most opportune frame rate for quality tuning, and power\n> +\t * vs performances budget on Intel IPU3.\n> +\t *\n> +\t * Reduce the minimum achievable frame rate to 30 FPS and compute the\n\nSince it's a \"frame rate\" should this be the \"maximum achievable frame\nrate\"?\n\nReviewed-by: Paul Elder \n\n> +\t * vertical blanking to maintain that rate.\n> +\t */\n> +\tint64_t *minFrameDuration = &frameDurations[0];\n> +\tif (*minFrameDuration < 1e6 / 30.0)\n> +\t\t*minFrameDuration = 1e6 / 30.0;\n> +\n> \tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> \t\t\t\t\t\t\t frameDurations[1],\n> \t\t\t\t\t\t\t frameDurations[2]);\n> \n> +\t/*\n> +\t * Adjust the vertical blanking to obtain the desired frame duration.\n> +\t *\n> +\t * Assume a fixed line length as horizontal blanking is seldom\n> +\t * controllable.\n> +\t *\n> +\t * \\todo Support making this overridable by the application through\n> +\t * controls::FrameDuration.\n> +\t *\n> +\t * \\todo Clamp exposure to frame duration.\n> +\t */\n> +\tvBlank_ = *minFrameDuration * (sensorInfo.pixelRate / 1000000U);\n> +\tvBlank_ /= lineLength;\n> +\tvBlank_ -= sensorInfo.outputSize.height;\n> +\n> \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> }\n> \n> @@ -399,8 +428,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \tmaxGain_ = itGain->second.max().get();\n> \tgain_ = minGain_;\n> \n> -\tdefVBlank_ = itVBlank->second.def().get();\n> -\n> \t/* Clean context at configuration */\n> \tcontext_ = {};\n> \n> @@ -511,8 +538,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> \n> \tsetControls(frame);\n> \n> -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> -\tint64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /\n> +\tint64_t frameDuration = sensorInfo_.lineLength * (vBlank_ + sensorInfo_.outputSize.height) /\n> \t\t\t\t(sensorInfo_.pixelRate / 1e6);\n> \tctrls.set(controls::FrameDuration, frameDuration);\n> \n> @@ -534,6 +560,7 @@ void IPAIPU3::setControls(unsigned int frame)\n> \tControlList ctrls(ctrls_);\n> \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> +\tctrls.set(V4L2_CID_VBLANK, static_cast(vBlank_));\n> \top.controls = ctrls;\n> \n> \tqueueFrameAction.emit(frame, op);\n> -- \n> 2.32.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 4AB9FBDC71\n\tfor ;\n\tWed, 1 Sep 2021 08:34:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B79CD6916A;\n\tWed, 1 Sep 2021 10:34:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41F7E60253\n\tfor ;\n\tWed, 1 Sep 2021 10:34:05 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B3A43D7;\n\tWed, 1 Sep 2021 10:34:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YJpyNCZD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630485244;\n\tbh=AWWUDpOR7jdigrM7AFBrjKg4RnCOeUVvDZaMTpe4Hzc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YJpyNCZDNLp0OFLE93acuZ7yc/N/QUZscg9PpIEKkqUY6GA07us3HySc1tO0AukcM\n\tSAeJ+Sf1CirZuM3Sv2efVnEdmUGbgeMxuS4sJGT0qOoE/Jw06agFQCdyvoTVXwV9S2\n\tMeQFBHtgn0PZWQjdkPyG4fS6QCiHX9QjmbZB002c=","Date":"Wed, 1 Sep 2021 17:33:57 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210901083357.GW968527@pyrite.rasen.tech>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-17-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210827120757.110615-17-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19328,"web_url":"https://patchwork.libcamera.org/comment/19328/","msgid":"<20210903084751.2it2jmucvq7hosgq@uno.localdomain>","date":"2021-09-03T08:47:51","subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Wed, Sep 01, 2021 at 05:33:57PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Fri, Aug 27, 2021 at 02:07:57PM +0200, Jacopo Mondi wrote:\n> > Limit the IPU3 frame rate to 30 FPS.\n> >\n> > The reason to do is to bring the IPU3 IPA in par with the Intel\n> > HAL implementation on IPU3 platform, where 30FPS is the frame rate used\n> > to perform quality tuning in the closed-source IPA module and has been\n> > validated as the most efficient rate for the power/performace budget.\n> >\n> > Compute the vertical blanking to maintain such frame rate and configure\n> > the sensor with that.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipu3/ipu3.cpp | 37 ++++++++++++++++++++++++++++++++-----\n> > 1 file changed, 32 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index fc5f69ed5ddc..0e5d5e479e20 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -183,7 +183,7 @@ private:\n> > \tIPACameraSensorInfo sensorInfo_;\n> >\n> > \t/* Camera sensor controls. */\n> > -\tuint32_t defVBlank_;\n> > +\tuint32_t vBlank_;\n> > \tuint32_t exposure_;\n> > \tuint32_t minExposure_;\n> > \tuint32_t maxExposure_;\n> > @@ -257,10 +257,39 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > \t}\n> >\n> > +\t/*\n> > +\t * Cap minimum frame duration to 30FPS.\n> > +\t *\n> > +\t * 30 FPS has been validated in the closed source Intel 3A module as the\n> > +\t * most opportune frame rate for quality tuning, and power\n> > +\t * vs performances budget on Intel IPU3.\n> > +\t *\n> > +\t * Reduce the minimum achievable frame rate to 30 FPS and compute the\n>\n> Since it's a \"frame rate\" should this be the \"maximum achievable frame\n> rate\"?\n\nIndeed! Thanks for spotting, I'll fix!\n\n>\n> Reviewed-by: Paul Elder \n>\n> > +\t * vertical blanking to maintain that rate.\n> > +\t */\n> > +\tint64_t *minFrameDuration = &frameDurations[0];\n> > +\tif (*minFrameDuration < 1e6 / 30.0)\n> > +\t\t*minFrameDuration = 1e6 / 30.0;\n> > +\n> > \tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> > \t\t\t\t\t\t\t frameDurations[1],\n> > \t\t\t\t\t\t\t frameDurations[2]);\n> >\n> > +\t/*\n> > +\t * Adjust the vertical blanking to obtain the desired frame duration.\n> > +\t *\n> > +\t * Assume a fixed line length as horizontal blanking is seldom\n> > +\t * controllable.\n> > +\t *\n> > +\t * \\todo Support making this overridable by the application through\n> > +\t * controls::FrameDuration.\n> > +\t *\n> > +\t * \\todo Clamp exposure to frame duration.\n> > +\t */\n> > +\tvBlank_ = *minFrameDuration * (sensorInfo.pixelRate / 1000000U);\n> > +\tvBlank_ /= lineLength;\n> > +\tvBlank_ -= sensorInfo.outputSize.height;\n> > +\n> > \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> > }\n> >\n> > @@ -399,8 +428,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > \tmaxGain_ = itGain->second.max().get();\n> > \tgain_ = minGain_;\n> >\n> > -\tdefVBlank_ = itVBlank->second.def().get();\n> > -\n> > \t/* Clean context at configuration */\n> > \tcontext_ = {};\n> >\n> > @@ -511,8 +538,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> >\n> > \tsetControls(frame);\n> >\n> > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > -\tint64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /\n> > +\tint64_t frameDuration = sensorInfo_.lineLength * (vBlank_ + sensorInfo_.outputSize.height) /\n> > \t\t\t\t(sensorInfo_.pixelRate / 1e6);\n> > \tctrls.set(controls::FrameDuration, frameDuration);\n> >\n> > @@ -534,6 +560,7 @@ void IPAIPU3::setControls(unsigned int frame)\n> > \tControlList ctrls(ctrls_);\n> > \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > +\tctrls.set(V4L2_CID_VBLANK, static_cast(vBlank_));\n> > \top.controls = ctrls;\n> >\n> > \tqueueFrameAction.emit(frame, op);\n> > --\n> > 2.32.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 B7877BDC71\n\tfor ;\n\tFri, 3 Sep 2021 08:47:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 247096916B;\n\tFri, 3 Sep 2021 10:47:05 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3020E69165\n\tfor ;\n\tFri, 3 Sep 2021 10:47:03 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 935C3C0003;\n\tFri, 3 Sep 2021 08:47:02 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 10:47:51 +0200","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20210903084751.2it2jmucvq7hosgq@uno.localdomain>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-17-jacopo@jmondi.org>\n\t<20210901083357.GW968527@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210901083357.GW968527@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19422,"web_url":"https://patchwork.libcamera.org/comment/19422/","msgid":"<2df0c56d-b050-0a8a-09d6-6ef451dc783e@ideasonboard.com>","date":"2021-09-06T11:03:10","subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nI have a few questions regarding the todos\n\nOn 8/27/21 5:37 PM, Jacopo Mondi wrote:\n> Limit the IPU3 frame rate to 30 FPS.\n>\n> The reason to do is to bring the IPU3 IPA in par with the Intel\n> HAL implementation on IPU3 platform, where 30FPS is the frame rate used\n> to perform quality tuning in the closed-source IPA module and has been\n> validated as the most efficient rate for the power/performace budget.\n>\n> Compute the vertical blanking to maintain such frame rate and configure\n> the sensor with that.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/ipu3/ipu3.cpp | 37 ++++++++++++++++++++++++++++++++-----\n> 1 file changed, 32 insertions(+), 5 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index fc5f69ed5ddc..0e5d5e479e20 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -183,7 +183,7 @@ private:\n> \tIPACameraSensorInfo sensorInfo_;\n> \n> \t/* Camera sensor controls. */\n> -\tuint32_t defVBlank_;\n> +\tuint32_t vBlank_;\n> \tuint32_t exposure_;\n> \tuint32_t minExposure_;\n> \tuint32_t maxExposure_;\n> @@ -257,10 +257,39 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> \t}\n> \n> +\t/*\n> +\t * Cap minimum frame duration to 30FPS.\n> +\t *\n> +\t * 30 FPS has been validated in the closed source Intel 3A module as the\n> +\t * most opportune frame rate for quality tuning, and power\n> +\t * vs performances budget on Intel IPU3.\n> +\t *\n> +\t * Reduce the minimum achievable frame rate to 30 FPS and compute the\n> +\t * vertical blanking to maintain that rate.\n> +\t */\n> +\tint64_t *minFrameDuration = &frameDurations[0];\n> +\tif (*minFrameDuration < 1e6 / 30.0)\n> +\t\t*minFrameDuration = 1e6 / 30.0;\n> +\n> \tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> \t\t\t\t\t\t\t frameDurations[1],\n> \t\t\t\t\t\t\t frameDurations[2]);\n> \n> +\t/*\n> +\t * Adjust the vertical blanking to obtain the desired frame duration.\n> +\t *\n> +\t * Assume a fixed line length as horizontal blanking is seldom\n> +\t * controllable.\n> +\t *\n> +\t * \\todo Support making this overridable by the application through\n> +\t * controls::FrameDuration.\n\n\nIn general, are applications poised to alter frame duration ? I think \napplications are poised to request manual exposure control, correct me \nif I am wrong! Hence, I don't understand the concept of overridable \ncontrols::FrameDuration as such.\n\n> +\t *\n> +\t * \\todo Clamp exposure to frame duration.\n\n\nIf a manual exposure is requested, will the clamping be in effect? In my \nhead, the broader concept is - manual exposure -> set exposure (which \nrequires new vblank), calculate new vblank -> set new vblank (which is \nto (-) \\todo  of in patch, see below).\n\n> +\t */\n> +\tvBlank_ = *minFrameDuration * (sensorInfo.pixelRate / 1000000U);\n> +\tvBlank_ /= lineLength;\n> +\tvBlank_ -= sensorInfo.outputSize.height;\n> +\n> \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> }\n> \n> @@ -399,8 +428,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \tmaxGain_ = itGain->second.max().get();\n> \tgain_ = minGain_;\n> \n> -\tdefVBlank_ = itVBlank->second.def().get();\n> -\n> \t/* Clean context at configuration */\n> \tcontext_ = {};\n> \n> @@ -511,8 +538,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> \n> \tsetControls(frame);\n> \n> -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n\n\nI think what I am trying to ask is, this patch deletes one \\todo, and \nintroduces two other \\todos. The descriptions  of new \\todos are \nconfusing to me or maybe they need some general discussion.\n\nThis previous todo, seems to comply with Intel HAL i.e. The Vblank is \nset from exposure's results. (See SyncManager::applySensorParams and  \nSensorHwOp::setFrameDuration). These use exposure's results derived from \nIPA.\n\n\n> -\tint64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /\n> +\tint64_t frameDuration = sensorInfo_.lineLength * (vBlank_ + sensorInfo_.outputSize.height) /\n> \t\t\t\t(sensorInfo_.pixelRate / 1e6);\n> \tctrls.set(controls::FrameDuration, frameDuration);\n> \n> @@ -534,6 +560,7 @@ void IPAIPU3::setControls(unsigned int frame)\n> \tControlList ctrls(ctrls_);\n> \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> +\tctrls.set(V4L2_CID_VBLANK, static_cast(vBlank_));\n> \top.controls = ctrls;\n> \n> \tqueueFrameAction.emit(frame, op);","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 27126BD87D\n\tfor ;\n\tMon, 6 Sep 2021 11:03:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C3556916A;\n\tMon, 6 Sep 2021 13:03:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2BD360137\n\tfor ;\n\tMon, 6 Sep 2021 13:03:15 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.180])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A7AB38AD;\n\tMon, 6 Sep 2021 13:03:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cyVnWPbu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630926195;\n\tbh=nhco5IxGIMfbUX4GMxujMj42lZlDuzcJZEHiAfX+crI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=cyVnWPbuGoRSisxX9nBeYNnvdwGQOddixh3kGzG3n+fErccEVQH33NmDjm+7CB/M7\n\tpxl/tPKIK/bTh2hAlt97cdufrhABTndybFlSkOsmGH66cV7ewDSr8uA7fIscHiHHAv\n\t2lJnpLuNsmNLIhcUhKbbKP5Nip0C/E96ZnNXZFLg=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-17-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"<2df0c56d-b050-0a8a-09d6-6ef451dc783e@ideasonboard.com>","Date":"Mon, 6 Sep 2021 16:33:10 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210827120757.110615-17-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","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":19475,"web_url":"https://patchwork.libcamera.org/comment/19475/","msgid":"<20210906164752.odpk35ch7q7mcffz@uno.localdomain>","date":"2021-09-06T16:47:52","subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, Sep 06, 2021 at 04:33:10PM +0530, Umang Jain wrote:\n> Hi Jacopo,\n>\n> I have a few questions regarding the todos\n>\n> On 8/27/21 5:37 PM, Jacopo Mondi wrote:\n> > Limit the IPU3 frame rate to 30 FPS.\n> >\n> > The reason to do is to bring the IPU3 IPA in par with the Intel\n> > HAL implementation on IPU3 platform, where 30FPS is the frame rate used\n> > to perform quality tuning in the closed-source IPA module and has been\n> > validated as the most efficient rate for the power/performace budget.\n> >\n> > Compute the vertical blanking to maintain such frame rate and configure\n> > the sensor with that.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipu3/ipu3.cpp | 37 ++++++++++++++++++++++++++++++++-----\n> > 1 file changed, 32 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index fc5f69ed5ddc..0e5d5e479e20 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -183,7 +183,7 @@ private:\n> > \tIPACameraSensorInfo sensorInfo_;\n> > \t/* Camera sensor controls. */\n> > -\tuint32_t defVBlank_;\n> > +\tuint32_t vBlank_;\n> > \tuint32_t exposure_;\n> > \tuint32_t minExposure_;\n> > \tuint32_t maxExposure_;\n> > @@ -257,10 +257,39 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > \t}\n> > +\t/*\n> > +\t * Cap minimum frame duration to 30FPS.\n> > +\t *\n> > +\t * 30 FPS has been validated in the closed source Intel 3A module as the\n> > +\t * most opportune frame rate for quality tuning, and power\n> > +\t * vs performances budget on Intel IPU3.\n> > +\t *\n> > +\t * Reduce the minimum achievable frame rate to 30 FPS and compute the\n> > +\t * vertical blanking to maintain that rate.\n> > +\t */\n> > +\tint64_t *minFrameDuration = &frameDurations[0];\n> > +\tif (*minFrameDuration < 1e6 / 30.0)\n> > +\t\t*minFrameDuration = 1e6 / 30.0;\n> > +\n> > \tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> > \t\t\t\t\t\t\t frameDurations[1],\n> > \t\t\t\t\t\t\t frameDurations[2]);\n> > +\t/*\n> > +\t * Adjust the vertical blanking to obtain the desired frame duration.\n> > +\t *\n> > +\t * Assume a fixed line length as horizontal blanking is seldom\n> > +\t * controllable.\n> > +\t *\n> > +\t * \\todo Support making this overridable by the application through\n> > +\t * controls::FrameDuration.\n>\n>\n> In general, are applications poised to alter frame duration ? I think\n> applications are poised to request manual exposure control, correct me if I\n> am wrong! Hence, I don't understand the concept of overridable\n> controls::FrameDuration as such.\n>\n\nFrom control_id.yaml\n\n When provided by applications, the control specifies the sensor frame\n duration interval the pipeline has to use. This limits the largest\n exposure time the sensor can use. For example, if a maximum frame\n duration of 33ms is requested (corresponding to 30 frames per second),\n the sensor will not be able to raise the exposure time above 33ms.\n A fixed frame duration is achieved by setting the minimum and maximum\n values to be the same. Setting both values to 0 reverts to using the\n IPA provided defaults.\n\n\n> > +\t *\n> > +\t * \\todo Clamp exposure to frame duration.\n>\n>\n> If a manual exposure is requested, will the clamping be in effect? In my\n\nFrom control_ids.yaml\n\n The maximum frame duration provides the absolute limit to the shutter\n speed computed by the AE algorithm and it overrides any exposure mode\n setting specified with controls::AeExposureMode. Similarly, when a\n manual exposure time is set through controls::ExposureTime, it also\n gets clipped to the limits set by this control.\n\nYou know, I feel the hardest part in defining controls is not their\ndefinition in isolation but defining how they interact together. The\nnumber of \\todo entries in controls_ids.yaml should suggest that to\nyou as well.\n\nAs the AEGC related definitions are in rework, this might change as\nwell. So far it matches what has been implemented by RPi, where the\nexposure time (being it computed by the AEGC or manually set) is\nalways clamped in the frame duration limits.\n\n\n> head, the broader concept is - manual exposure -> set exposure (which\n> requires new vblank), calculate new vblank -> set new vblank (which is to\n> (-) \\todo  of in patch, see below).\n>\n> > +\t */\n> > +\tvBlank_ = *minFrameDuration * (sensorInfo.pixelRate / 1000000U);\n> > +\tvBlank_ /= lineLength;\n> > +\tvBlank_ -= sensorInfo.outputSize.height;\n> > +\n> > \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> > }\n> > @@ -399,8 +428,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > \tmaxGain_ = itGain->second.max().get();\n> > \tgain_ = minGain_;\n> > -\tdefVBlank_ = itVBlank->second.def().get();\n> > -\n> > \t/* Clean context at configuration */\n> > \tcontext_ = {};\n> > @@ -511,8 +538,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > \tsetControls(frame);\n> > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n>\n>\n> I think what I am trying to ask is, this patch deletes one \\todo, and\n> introduces two other \\todos. The descriptions  of new \\todos are confusing\n> to me or maybe they need some general discussion.\n>\n> This previous todo, seems to comply with Intel HAL i.e. The Vblank is set\n> from exposure's results. (See SyncManager::applySensorParams and \n> SensorHwOp::setFrameDuration). These use exposure's results derived from\n> IPA.\n\nI think the vblank calculated by the AEGC algorithm shuld be in the\nlimits defined by the frame durations.\n\nIt didn' get what you mean with:\n\"These use exposure's results derived from IPA\"\n\nDoes it mean the Intel IPA adaption later receives the exposure\nvalues from a black box and comptes frame duration limit from there ?\nIt doesn't comply with our definition of how frame duration and the\nAEGC algorithm interact, but I suspect we cannot do much about it.\n\nAs long as the definition of how our controls interact doesn't change, and\nshould changes for a good reason, I think the open IPA design should behave\naccording to what is there prescribed.\n\nI'm cutting a lot of corners both in the definition of how AEGC and\ndurations interact and in considering corner cases, but as reference have\na look at the RPi camera helper behaves:\n\n\t/*\n\t * Limit the exposure to the maximum frame duration requested, and\n\t * re-calculate if it has been clipped.\n\t */\n\texposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);\n\texposure = Exposure(exposureLines);\n\n\t/* Limit the vblank to the range allowed by the frame length limits. */\n\tvblank = std::clamp(exposureLines + frameIntegrationDiff_,\n\t\t\t frameLengthMin, frameLengthMax) - mode_.height;\n\treturn vblank;\n}\n\n>\n>\n> > -\tint64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /\n> > +\tint64_t frameDuration = sensorInfo_.lineLength * (vBlank_ + sensorInfo_.outputSize.height) /\n> > \t\t\t\t(sensorInfo_.pixelRate / 1e6);\n> > \tctrls.set(controls::FrameDuration, frameDuration);\n> > @@ -534,6 +560,7 @@ void IPAIPU3::setControls(unsigned int frame)\n> > \tControlList ctrls(ctrls_);\n> > \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > +\tctrls.set(V4L2_CID_VBLANK, static_cast(vBlank_));\n> > \top.controls = ctrls;\n> > \tqueueFrameAction.emit(frame, op);","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 574D3BD87D\n\tfor ;\n\tMon, 6 Sep 2021 16:47:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B78206916A;\n\tMon, 6 Sep 2021 18:47:06 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4E3C160137\n\tfor ;\n\tMon, 6 Sep 2021 18:47:05 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id D273B40003;\n\tMon, 6 Sep 2021 16:47:04 +0000 (UTC)"],"Date":"Mon, 6 Sep 2021 18:47:52 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210906164752.odpk35ch7q7mcffz@uno.localdomain>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-17-jacopo@jmondi.org>\n\t<2df0c56d-b050-0a8a-09d6-6ef451dc783e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2df0c56d-b050-0a8a-09d6-6ef451dc783e@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration\n\tto 30 FPS","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]