[{"id":31897,"web_url":"https://patchwork.libcamera.org/comment/31897/","msgid":"","date":"2024-10-24T03:58:58","subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran, Paul\n\nThank you for the patch.\n\nOn 14/10/24 9:17 pm, Kieran Bingham wrote:\n> From: Paul Elder \n>\n> Add support to rkisp1 for controlling the framerate via the\n> FrameDurationLimits control.\n>\n> Signed-off-by: Paul Elder \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------\n> src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-\n> src/ipa/rkisp1/ipa_context.h | 4 ++\n> src/ipa/rkisp1/rkisp1.cpp | 2 +\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++\n> 5 files changed, 68 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index e23ab120b3e2..088ecf42c480 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \n> \t/* Limit the frame duration to match current initialisation */\n> \tControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> +\tcontext.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get());\n> \tcontext.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get());\n> \n> \t/*\n> @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,\n> \n> \tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> \tif (frameDurationLimits) {\n> -\t\tutils::Duration maxFrameDuration =\n> -\t\t\tstd::chrono::milliseconds((*frameDurationLimits).back());\n> -\t\tagc.maxFrameDuration = maxFrameDuration;\n> +\t\t/* Limit the control value to the limits in ControlInfo */\n> +\t\tControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> +\t\tint64_t minFrameDuration =\n> +\t\t\tstd::clamp((*frameDurationLimits).front(),\n> +\t\t\t\t limits.min().get(),\n> +\t\t\t\t limits.max().get());\n> +\t\tint64_t maxFrameDuration =\n> +\t\t\tstd::clamp((*frameDurationLimits).back(),\n> +\t\t\t\t limits.min().get(),\n> +\t\t\t\t limits.max().get());\n> +\n> +\t\tagc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> +\t\tagc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> \t}\n> +\tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n> \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> }\n> \n> @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t\t * frameContext.sensor.exposure;\n> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> +\tmetadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get());\n> \tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> -\n> -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> -\tuint32_t vTotal = context.configuration.sensor.size.height\n> -\t\t\t+ context.configuration.sensor.defVBlank;\n> -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> -\t\t\t\t * vTotal;\n> -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> -\n> \tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\treturn;\n> \t}\n> \n> +\tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> \n> \t/*\n> @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t [](uint32_t x) { return x >> 4; });\n> \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> \n> +\t/*\n> +\t * Limit the allowed shutter speeds for the exposure helper based on\n> +\t * the frame duration limits.\n> +\t */\n> +\tutils::Duration minShutterSpeed =\n> +\t\tstd::clamp(frameContext.agc.minFrameDuration,\n> +\t\t\t context.configuration.sensor.minShutterSpeed,\n> +\t\t\t context.configuration.sensor.maxShutterSpeed);\n> \tutils::Duration maxShutterSpeed =\n> \t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> \t\t\t context.configuration.sensor.minShutterSpeed,\n> \t\t\t context.configuration.sensor.maxShutterSpeed);\n> -\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> +\tsetLimits(minShutterSpeed,\n> \t\t maxShutterSpeed,\n> \t\t context.configuration.sensor.minAnalogueGain,\n> \t\t context.configuration.sensor.maxAnalogueGain);\n> @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tactiveState.agc.automatic.exposure = shutterTime / lineDuration;\n> \tactiveState.agc.automatic.gain = aGain;\n> \n> +\t/*\n> +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n> +\t * this if we need to expand the shutter time calculated to fill the\n> +\t * remaining time so that we do not run faster than the minimum frame\n> +\t * duration (maximum requested frame rate) when we have short exposures.\n> +\t */\n\nFeels like this should be broken down to atleast two sentences.\n\n +\t/*\n +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n +\t * it. If we need to expand the shutter time, calculate VBLANK to fill the\n +\t * remaining time so that we do not run faster than the minimum frame\n +\t * duration (maximum requested frame rate) when we have short exposures.\n +\t */\n\n\nOther than this,\n\nReviewed-by: Umang Jain \n> +\tutils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,\n> +\t\t\t\t\t\t shutterTime);\n> +\n> +\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> +\n> +\t/* Update accounting for line length quantization. */\n> +\tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> +\n> \tfillMetadata(context, frameContext, metadata);\n> \texpMeans_ = {};\n> }\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 14d0c02a2b32..c5eb0d64ec29 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAActiveState::agc.meteringMode\n> * \\brief Metering mode as set by the AeMeteringMode control\n> *\n> + * \\var IPAActiveState::agc.minFrameDuration\n> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> + *\n> * \\var IPAActiveState::agc.maxFrameDuration\n> * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> */\n> @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Automatic Gain Control parameters for this frame\n> *\n> * The exposure and gain are provided by the AGC algorithm, and are to be\n> - * applied to the sensor in order to take effect for this frame.\n> + * applied to the sensor in order to take effect for this frame. Additionally\n> + * the vertical blanking period is determined to maintain a consistent frame\n> + * rate matched to the FrameDurationLimits as set by the user.\n> *\n> * \\var IPAFrameContext::agc.exposure\n> * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * The gain should be adapted to the sensor specific gain code before applying.\n> *\n> + * \\var IPAFrameContext::agc.vblank\n> + * \\brief Vertical blanking parameter computed by the algorithm\n> + *\n> * \\var IPAFrameContext::agc.autoEnabled\n> * \\brief Manual/automatic AGC state as set by the AeEnable control\n> *\n> @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAFrameContext::agc.meteringMode\n> * \\brief Metering mode as set by the AeMeteringMode control\n> *\n> + * \\var IPAFrameContext::agc.minFrameDuration\n> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> + *\n> * \\var IPAFrameContext::agc.maxFrameDuration\n> * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> *\n> + * \\var IPAFrameContext::agc.frameDuration\n> + * \\brief The actual FrameDuration used by the algorithm for the frame\n> + *\n> * \\var IPAFrameContext::agc.updateMetering\n> * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> */\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index e274d9b01e1c..60c4d647f1ef 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -79,6 +79,7 @@ struct IPAActiveState {\n> \t\tcontrols::AeConstraintModeEnum constraintMode;\n> \t\tcontrols::AeExposureModeEnum exposureMode;\n> \t\tcontrols::AeMeteringModeEnum meteringMode;\n> +\t\tutils::Duration minFrameDuration;\n> \t\tutils::Duration maxFrameDuration;\n> \t} agc;\n> \n> @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> +\t\tuint32_t vblank;\n> \t\tbool autoEnabled;\n> \t\tcontrols::AeConstraintModeEnum constraintMode;\n> \t\tcontrols::AeExposureModeEnum exposureMode;\n> \t\tcontrols::AeMeteringModeEnum meteringMode;\n> +\t\tutils::Duration minFrameDuration;\n> \t\tutils::Duration maxFrameDuration;\n> +\t\tutils::Duration frameDuration;\n> \t\tbool updateMetering;\n> \t} agc;\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 47777ece783f..17d56fb4e901 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \tuint32_t exposure = frameContext.agc.exposure;\n> \tuint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> +\tuint32_t vblank = frameContext.agc.vblank;\n> \n> \tControlList ctrls(sensorControls_);\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> \n> \tsetSensorControls.emit(frame, ctrls);\n> }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 42961c120036..1ec12185bb78 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> \t\t\t\t\t const ControlList &sensorControls)\n> {\n> +\tif (frame == 0) {\n> +\t\tControlList controls = sensorControls;\n> +\t\tsensor_->setControls(&controls);\n> +\t\treturn;\n> +\t}\n> +\n> \tdelayedCtrls_->push(sensorControls);\n> }\n> \n> @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tstd::unordered_map params = {\n> \t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> \t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n> +\t\t{ V4L2_CID_VBLANK, { 1, false } },\n> \t};\n> \n> \tdata->delayedCtrls_ =","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 9878BBE08B\n\tfor ;\n\tThu, 24 Oct 2024 03:59:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B482165397;\n\tThu, 24 Oct 2024 05:59:04 +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 04CE365394\n\tfor ;\n\tThu, 24 Oct 2024 05:59:03 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ABE493D5;\n\tThu, 24 Oct 2024 05:57: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=\"KHuBINUM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729742235;\n\tbh=EH1vS0/WYlfvNPlXBReFyzNsKXmibKGEQCAiQS7wh1c=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=KHuBINUMkHN0LZmVOGQt9zaROcGN2QqegXZLAlrffa8KSYT2VzRKud8OdMXBWzFvU\n\tJvRyfl8/22+0f0+jaJ+1PDrJZNiDV9CvAs+ldAXSeteClcLqS48dMdvzn5na79gwnL\n\tCp2R3OJh7B+KdOt5GxnRk+CZfgaVDYKDdWrvsoq4=","Message-ID":"","Date":"Thu, 24 Oct 2024 09:28:58 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","To":"Kieran Bingham ,\n\tlibcamera devel ","Cc":"Paul Elder ","References":"<20241014154747.2295253-1-kieran.bingham@ideasonboard.com>\n\t<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":31910,"web_url":"https://patchwork.libcamera.org/comment/31910/","msgid":"","date":"2024-10-24T15:53:01","subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:\n> From: Paul Elder \n>\n> Add support to rkisp1 for controlling the framerate via the\n> FrameDurationLimits control.\n>\n> Signed-off-by: Paul Elder \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------\n> src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-\n> src/ipa/rkisp1/ipa_context.h | 4 ++\n> src/ipa/rkisp1/rkisp1.cpp | 2 +\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++\n> 5 files changed, 68 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index e23ab120b3e2..088ecf42c480 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>\n> \t/* Limit the frame duration to match current initialisation */\n> \tControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> +\tcontext.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get());\n> \tcontext.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get());\n>\n> \t/*\n> @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,\n>\n> \tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> \tif (frameDurationLimits) {\n> -\t\tutils::Duration maxFrameDuration =\n> -\t\t\tstd::chrono::milliseconds((*frameDurationLimits).back());\n> -\t\tagc.maxFrameDuration = maxFrameDuration;\n> +\t\t/* Limit the control value to the limits in ControlInfo */\n> +\t\tControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> +\t\tint64_t minFrameDuration =\n> +\t\t\tstd::clamp((*frameDurationLimits).front(),\n> +\t\t\t\t limits.min().get(),\n> +\t\t\t\t limits.max().get());\n> +\t\tint64_t maxFrameDuration =\n> +\t\t\tstd::clamp((*frameDurationLimits).back(),\n> +\t\t\t\t limits.min().get(),\n> +\t\t\t\t limits.max().get());\n> +\n\nWe operate on the assumption the control is well-formed, right ? iow\nthat frameDurationLimits[0] < frameDurationLimits[1] ?\n\n> +\t\tagc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> +\t\tagc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> \t}\n> +\tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n> \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> }\n>\n> @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t\t * frameContext.sensor.exposure;\n> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> +\tmetadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get());\n> \tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> -\n> -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> -\tuint32_t vTotal = context.configuration.sensor.size.height\n> -\t\t\t+ context.configuration.sensor.defVBlank;\n> -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> -\t\t\t\t * vTotal;\n> -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> -\n> \tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\treturn;\n> \t}\n>\n> +\tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n>\n> \t/*\n> @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t [](uint32_t x) { return x >> 4; });\n> \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n>\n> +\t/*\n> +\t * Limit the allowed shutter speeds for the exposure helper based on\n> +\t * the frame duration limits.\n> +\t */\n> +\tutils::Duration minShutterSpeed =\n> +\t\tstd::clamp(frameContext.agc.minFrameDuration,\n> +\t\t\t context.configuration.sensor.minShutterSpeed,\n> +\t\t\t context.configuration.sensor.maxShutterSpeed);\n> \tutils::Duration maxShutterSpeed =\n> \t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> \t\t\t context.configuration.sensor.minShutterSpeed,\n> \t\t\t context.configuration.sensor.maxShutterSpeed);\n> -\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> +\tsetLimits(minShutterSpeed,\n> \t\t maxShutterSpeed,\n> \t\t context.configuration.sensor.minAnalogueGain,\n> \t\t context.configuration.sensor.maxAnalogueGain);\n> @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tactiveState.agc.automatic.exposure = shutterTime / lineDuration;\n> \tactiveState.agc.automatic.gain = aGain;\n>\n> +\t/*\n> +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n> +\t * this if we need to expand the shutter time calculated to fill the\n> +\t * remaining time so that we do not run faster than the minimum frame\n> +\t * duration (maximum requested frame rate) when we have short exposures.\n> +\t */\n> +\tutils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,\n> +\t\t\t\t\t\t shutterTime);\n> +\n> +\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> +\n> +\t/* Update accounting for line length quantization. */\n> +\tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> +\n> \tfillMetadata(context, frameContext, metadata);\n> \texpMeans_ = {};\n> }\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 14d0c02a2b32..c5eb0d64ec29 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAActiveState::agc.meteringMode\n> * \\brief Metering mode as set by the AeMeteringMode control\n> *\n> + * \\var IPAActiveState::agc.minFrameDuration\n> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> + *\n> * \\var IPAActiveState::agc.maxFrameDuration\n> * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> */\n> @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Automatic Gain Control parameters for this frame\n> *\n> * The exposure and gain are provided by the AGC algorithm, and are to be\n> - * applied to the sensor in order to take effect for this frame.\n> + * applied to the sensor in order to take effect for this frame. Additionally\n> + * the vertical blanking period is determined to maintain a consistent frame\n> + * rate matched to the FrameDurationLimits as set by the user.\n> *\n> * \\var IPAFrameContext::agc.exposure\n> * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * The gain should be adapted to the sensor specific gain code before applying.\n> *\n> + * \\var IPAFrameContext::agc.vblank\n> + * \\brief Vertical blanking parameter computed by the algorithm\n> + *\n> * \\var IPAFrameContext::agc.autoEnabled\n> * \\brief Manual/automatic AGC state as set by the AeEnable control\n> *\n> @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAFrameContext::agc.meteringMode\n> * \\brief Metering mode as set by the AeMeteringMode control\n> *\n> + * \\var IPAFrameContext::agc.minFrameDuration\n> + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> + *\n> * \\var IPAFrameContext::agc.maxFrameDuration\n> * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> *\n> + * \\var IPAFrameContext::agc.frameDuration\n> + * \\brief The actual FrameDuration used by the algorithm for the frame\n> + *\n> * \\var IPAFrameContext::agc.updateMetering\n> * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> */\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index e274d9b01e1c..60c4d647f1ef 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -79,6 +79,7 @@ struct IPAActiveState {\n> \t\tcontrols::AeConstraintModeEnum constraintMode;\n> \t\tcontrols::AeExposureModeEnum exposureMode;\n> \t\tcontrols::AeMeteringModeEnum meteringMode;\n> +\t\tutils::Duration minFrameDuration;\n> \t\tutils::Duration maxFrameDuration;\n> \t} agc;\n>\n> @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> +\t\tuint32_t vblank;\n> \t\tbool autoEnabled;\n> \t\tcontrols::AeConstraintModeEnum constraintMode;\n> \t\tcontrols::AeExposureModeEnum exposureMode;\n> \t\tcontrols::AeMeteringModeEnum meteringMode;\n> +\t\tutils::Duration minFrameDuration;\n> \t\tutils::Duration maxFrameDuration;\n> +\t\tutils::Duration frameDuration;\n> \t\tbool updateMetering;\n> \t} agc;\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 47777ece783f..17d56fb4e901 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \tuint32_t exposure = frameContext.agc.exposure;\n> \tuint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> +\tuint32_t vblank = frameContext.agc.vblank;\n>\n> \tControlList ctrls(sensorControls_);\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>\n> \tsetSensorControls.emit(frame, ctrls);\n> }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 42961c120036..1ec12185bb78 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> \t\t\t\t\t const ControlList &sensorControls)\n> {\n> +\tif (frame == 0) {\n> +\t\tControlList controls = sensorControls;\n> +\t\tsensor_->setControls(&controls);\n> +\t\treturn;\n> +\t}\n> +\n\nI might have missed what is this for :)\n\n> \tdelayedCtrls_->push(sensorControls);\n> }\n>\n> @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tstd::unordered_map params = {\n> \t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> \t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n> +\t\t{ V4L2_CID_VBLANK, { 1, false } },\n> \t};\n>\n> \tdata->delayedCtrls_ =\n> --\n> 2.34.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 18201C3213\n\tfor ;\n\tThu, 24 Oct 2024 15:53:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E79165394;\n\tThu, 24 Oct 2024 17:53:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E722618C1\n\tfor ;\n\tThu, 24 Oct 2024 17:53:05 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E869C59D;\n\tThu, 24 Oct 2024 17:51:16 +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=\"shaDxYrt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729785077;\n\tbh=kM4xQ3ZEpE6p0Noc5GXhuasUnBB7Oo3q6LY4cTf8WFU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=shaDxYrt8x4iCmfE+HHRKBTkchm1vb1dRn/gYAR9+eAiZzLIZ2/825bIfxY/W5lpb\n\trMQsXgRjJZ98AZV9r5t4PVZJLp7+dYv6jqO93uiCA+ZHfLNqZy6Vt92cPAQNORAXyK\n\tqdpTAZiSOLSObbNlbFfEpJZ82TBo3G0Emc72bDsQ=","Date":"Thu, 24 Oct 2024 17:53:01 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"libcamera devel , \n\tPaul Elder ","Subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","Message-ID":"","References":"<20241014154747.2295253-1-kieran.bingham@ideasonboard.com>\n\t<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32003,"web_url":"https://patchwork.libcamera.org/comment/32003/","msgid":"","date":"2024-11-04T11:25:02","subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:\n> Hi Kieran\n> \n> On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:\n> > From: Paul Elder \n> >\n> > Add support to rkisp1 for controlling the framerate via the\n> > FrameDurationLimits control.\n> >\n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------\n> > src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-\n> > src/ipa/rkisp1/ipa_context.h | 4 ++\n> > src/ipa/rkisp1/rkisp1.cpp | 2 +\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++\n> > 5 files changed, 68 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index e23ab120b3e2..088ecf42c480 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> >\n> > \t/* Limit the frame duration to match current initialisation */\n> > \tControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> > +\tcontext.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get());\n> > \tcontext.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get());\n> >\n> > \t/*\n> > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,\n> >\n> > \tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > \tif (frameDurationLimits) {\n> > -\t\tutils::Duration maxFrameDuration =\n> > -\t\t\tstd::chrono::milliseconds((*frameDurationLimits).back());\n> > -\t\tagc.maxFrameDuration = maxFrameDuration;\n> > +\t\t/* Limit the control value to the limits in ControlInfo */\n> > +\t\tControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> > +\t\tint64_t minFrameDuration =\n> > +\t\t\tstd::clamp((*frameDurationLimits).front(),\n> > +\t\t\t\t limits.min().get(),\n> > +\t\t\t\t limits.max().get());\n> > +\t\tint64_t maxFrameDuration =\n> > +\t\t\tstd::clamp((*frameDurationLimits).back(),\n> > +\t\t\t\t limits.min().get(),\n> > +\t\t\t\t limits.max().get());\n> > +\n> \n> We operate on the assumption the control is well-formed, right ? iow\n> that frameDurationLimits[0] < frameDurationLimits[1] ?\n\nYeah.\n\n> \n> > +\t\tagc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> > +\t\tagc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> > \t}\n> > +\tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n> > \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> > }\n> >\n> > @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > \t\t\t\t * frameContext.sensor.exposure;\n> > \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > +\tmetadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get());\n> > \tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > -\n> > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > -\tuint32_t vTotal = context.configuration.sensor.size.height\n> > -\t\t\t+ context.configuration.sensor.defVBlank;\n> > -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > -\t\t\t\t * vTotal;\n> > -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > -\n> > \tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \t\treturn;\n> > \t}\n> >\n> > +\tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> > \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> >\n> > \t/*\n> > @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \t\t [](uint32_t x) { return x >> 4; });\n> > \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> >\n> > +\t/*\n> > +\t * Limit the allowed shutter speeds for the exposure helper based on\n> > +\t * the frame duration limits.\n> > +\t */\n> > +\tutils::Duration minShutterSpeed =\n> > +\t\tstd::clamp(frameContext.agc.minFrameDuration,\n> > +\t\t\t context.configuration.sensor.minShutterSpeed,\n> > +\t\t\t context.configuration.sensor.maxShutterSpeed);\n> > \tutils::Duration maxShutterSpeed =\n> > \t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> > \t\t\t context.configuration.sensor.minShutterSpeed,\n> > \t\t\t context.configuration.sensor.maxShutterSpeed);\n> > -\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> > +\tsetLimits(minShutterSpeed,\n> > \t\t maxShutterSpeed,\n> > \t\t context.configuration.sensor.minAnalogueGain,\n> > \t\t context.configuration.sensor.maxAnalogueGain);\n> > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \tactiveState.agc.automatic.exposure = shutterTime / lineDuration;\n> > \tactiveState.agc.automatic.gain = aGain;\n> >\n> > +\t/*\n> > +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n> > +\t * this if we need to expand the shutter time calculated to fill the\n> > +\t * remaining time so that we do not run faster than the minimum frame\n> > +\t * duration (maximum requested frame rate) when we have short exposures.\n> > +\t */\n> > +\tutils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,\n> > +\t\t\t\t\t\t shutterTime);\n> > +\n> > +\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > +\n> > +\t/* Update accounting for line length quantization. */\n> > +\tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > +\n> > \tfillMetadata(context, frameContext, metadata);\n> > \texpMeans_ = {};\n> > }\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 14d0c02a2b32..c5eb0d64ec29 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\var IPAActiveState::agc.meteringMode\n> > * \\brief Metering mode as set by the AeMeteringMode control\n> > *\n> > + * \\var IPAActiveState::agc.minFrameDuration\n> > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > + *\n> > * \\var IPAActiveState::agc.maxFrameDuration\n> > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > */\n> > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\brief Automatic Gain Control parameters for this frame\n> > *\n> > * The exposure and gain are provided by the AGC algorithm, and are to be\n> > - * applied to the sensor in order to take effect for this frame.\n> > + * applied to the sensor in order to take effect for this frame. Additionally\n> > + * the vertical blanking period is determined to maintain a consistent frame\n> > + * rate matched to the FrameDurationLimits as set by the user.\n> > *\n> > * \\var IPAFrameContext::agc.exposure\n> > * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {\n> > *\n> > * The gain should be adapted to the sensor specific gain code before applying.\n> > *\n> > + * \\var IPAFrameContext::agc.vblank\n> > + * \\brief Vertical blanking parameter computed by the algorithm\n> > + *\n> > * \\var IPAFrameContext::agc.autoEnabled\n> > * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > *\n> > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\var IPAFrameContext::agc.meteringMode\n> > * \\brief Metering mode as set by the AeMeteringMode control\n> > *\n> > + * \\var IPAFrameContext::agc.minFrameDuration\n> > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > + *\n> > * \\var IPAFrameContext::agc.maxFrameDuration\n> > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > *\n> > + * \\var IPAFrameContext::agc.frameDuration\n> > + * \\brief The actual FrameDuration used by the algorithm for the frame\n> > + *\n> > * \\var IPAFrameContext::agc.updateMetering\n> > * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> > */\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index e274d9b01e1c..60c4d647f1ef 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -79,6 +79,7 @@ struct IPAActiveState {\n> > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > +\t\tutils::Duration minFrameDuration;\n> > \t\tutils::Duration maxFrameDuration;\n> > \t} agc;\n> >\n> > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {\n> > \tstruct {\n> > \t\tuint32_t exposure;\n> > \t\tdouble gain;\n> > +\t\tuint32_t vblank;\n> > \t\tbool autoEnabled;\n> > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > +\t\tutils::Duration minFrameDuration;\n> > \t\tutils::Duration maxFrameDuration;\n> > +\t\tutils::Duration frameDuration;\n> > \t\tbool updateMetering;\n> > \t} agc;\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 47777ece783f..17d56fb4e901 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > \tuint32_t exposure = frameContext.agc.exposure;\n> > \tuint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> > +\tuint32_t vblank = frameContext.agc.vblank;\n> >\n> > \tControlList ctrls(sensorControls_);\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> >\n> > \tsetSensorControls.emit(frame, ctrls);\n> > }\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 42961c120036..1ec12185bb78 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > \t\t\t\t\t const ControlList &sensorControls)\n> > {\n> > +\tif (frame == 0) {\n> > +\t\tControlList controls = sensorControls;\n> > +\t\tsensor_->setControls(&controls);\n> > +\t\treturn;\n> > +\t}\n> > +\n> \n> I might have missed what is this for :)\n\niirc I needed this because the vlbank wasn't being applied...\nuntil the next time the control was set.\n\n\nPaul\n\n> \n> > \tdelayedCtrls_->push(sensorControls);\n> > }\n> >\n> > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > \tstd::unordered_map params = {\n> > \t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> > \t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n> > +\t\t{ V4L2_CID_VBLANK, { 1, false } },\n> > \t};\n> >\n> > \tdata->delayedCtrls_ =\n> > --\n> > 2.34.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 DA255BDB1C\n\tfor ;\n\tMon, 4 Nov 2024 11:25:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F9CA653D0;\n\tMon, 4 Nov 2024 12:25:16 +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 0D9D265399\n\tfor ;\n\tMon, 4 Nov 2024 12:25:15 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:f74e:5d21:8053:f1b9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA90D22E;\n\tMon, 4 Nov 2024 12:25:06 +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=\"YpdJjRPJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730719508;\n\tbh=UjAKi+Ku+OW++tKPemn68kJmFmjQueWID++4+dcynjo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YpdJjRPJAdwG4greNcFRoI+FXZ3sSY893XiqviFACehI9QfgQMZjUrS1nE6RpbYdR\n\tONk6t1nHYSJdmuTn1M/X2XfjQ9x7sUw4UxCnmHYh4LMrdbk8H4EbjI9AKzZVIv1PeZ\n\tb0o2l8BPxseYDQqitrCD5mdNpLdH9YUhfZE69vxM=","Date":"Mon, 4 Nov 2024 20:25:02 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","Subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","Message-ID":"","References":"<20241014154747.2295253-1-kieran.bingham@ideasonboard.com>\n\t<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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":32005,"web_url":"https://patchwork.libcamera.org/comment/32005/","msgid":"","date":"2024-11-04T13:35:06","subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul\n\nOn Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:\n> On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:\n> > Hi Kieran\n> >\n> > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:\n> > > From: Paul Elder \n> > >\n> > > Add support to rkisp1 for controlling the framerate via the\n> > > FrameDurationLimits control.\n> > >\n> > > Signed-off-by: Paul Elder \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------\n> > > src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-\n> > > src/ipa/rkisp1/ipa_context.h | 4 ++\n> > > src/ipa/rkisp1/rkisp1.cpp | 2 +\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++\n> > > 5 files changed, 68 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index e23ab120b3e2..088ecf42c480 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > >\n> > > \t/* Limit the frame duration to match current initialisation */\n> > > \tControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > +\tcontext.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get());\n> > > \tcontext.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get());\n> > >\n> > > \t/*\n> > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,\n> > >\n> > > \tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > > \tif (frameDurationLimits) {\n> > > -\t\tutils::Duration maxFrameDuration =\n> > > -\t\t\tstd::chrono::milliseconds((*frameDurationLimits).back());\n> > > -\t\tagc.maxFrameDuration = maxFrameDuration;\n> > > +\t\t/* Limit the control value to the limits in ControlInfo */\n> > > +\t\tControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > +\t\tint64_t minFrameDuration =\n> > > +\t\t\tstd::clamp((*frameDurationLimits).front(),\n> > > +\t\t\t\t limits.min().get(),\n> > > +\t\t\t\t limits.max().get());\n> > > +\t\tint64_t maxFrameDuration =\n> > > +\t\t\tstd::clamp((*frameDurationLimits).back(),\n> > > +\t\t\t\t limits.min().get(),\n> > > +\t\t\t\t limits.max().get());\n> > > +\n> >\n> > We operate on the assumption the control is well-formed, right ? iow\n> > that frameDurationLimits[0] < frameDurationLimits[1] ?\n>\n> Yeah.\n>\n> >\n> > > +\t\tagc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> > > +\t\tagc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> > > \t}\n> > > +\tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n> > > \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> > > }\n> > >\n> > > @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > \t\t\t\t * frameContext.sensor.exposure;\n> > > \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > > \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > > +\tmetadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get());\n> > > \tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > > -\n> > > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > > -\tuint32_t vTotal = context.configuration.sensor.size.height\n> > > -\t\t\t+ context.configuration.sensor.defVBlank;\n> > > -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > > -\t\t\t\t * vTotal;\n> > > -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > > -\n> > > \tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > > \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > > \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > \t\treturn;\n> > > \t}\n> > >\n> > > +\tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> > > \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> > >\n> > > \t/*\n> > > @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > \t\t [](uint32_t x) { return x >> 4; });\n> > > \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > >\n> > > +\t/*\n> > > +\t * Limit the allowed shutter speeds for the exposure helper based on\n> > > +\t * the frame duration limits.\n> > > +\t */\n> > > +\tutils::Duration minShutterSpeed =\n> > > +\t\tstd::clamp(frameContext.agc.minFrameDuration,\n> > > +\t\t\t context.configuration.sensor.minShutterSpeed,\n> > > +\t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > \tutils::Duration maxShutterSpeed =\n> > > \t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> > > \t\t\t context.configuration.sensor.minShutterSpeed,\n> > > \t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > -\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> > > +\tsetLimits(minShutterSpeed,\n> > > \t\t maxShutterSpeed,\n> > > \t\t context.configuration.sensor.minAnalogueGain,\n> > > \t\t context.configuration.sensor.maxAnalogueGain);\n> > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > \tactiveState.agc.automatic.exposure = shutterTime / lineDuration;\n> > > \tactiveState.agc.automatic.gain = aGain;\n> > >\n> > > +\t/*\n> > > +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n> > > +\t * this if we need to expand the shutter time calculated to fill the\n> > > +\t * remaining time so that we do not run faster than the minimum frame\n> > > +\t * duration (maximum requested frame rate) when we have short exposures.\n> > > +\t */\n> > > +\tutils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,\n> > > +\t\t\t\t\t\t shutterTime);\n> > > +\n> > > +\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > > +\n> > > +\t/* Update accounting for line length quantization. */\n> > > +\tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > > +\n> > > \tfillMetadata(context, frameContext, metadata);\n> > > \texpMeans_ = {};\n> > > }\n> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > index 14d0c02a2b32..c5eb0d64ec29 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\var IPAActiveState::agc.meteringMode\n> > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > *\n> > > + * \\var IPAActiveState::agc.minFrameDuration\n> > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > + *\n> > > * \\var IPAActiveState::agc.maxFrameDuration\n> > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > */\n> > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\brief Automatic Gain Control parameters for this frame\n> > > *\n> > > * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > - * applied to the sensor in order to take effect for this frame.\n> > > + * applied to the sensor in order to take effect for this frame. Additionally\n> > > + * the vertical blanking period is determined to maintain a consistent frame\n> > > + * rate matched to the FrameDurationLimits as set by the user.\n> > > *\n> > > * \\var IPAFrameContext::agc.exposure\n> > > * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > *\n> > > * The gain should be adapted to the sensor specific gain code before applying.\n> > > *\n> > > + * \\var IPAFrameContext::agc.vblank\n> > > + * \\brief Vertical blanking parameter computed by the algorithm\n> > > + *\n> > > * \\var IPAFrameContext::agc.autoEnabled\n> > > * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > > *\n> > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\var IPAFrameContext::agc.meteringMode\n> > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > *\n> > > + * \\var IPAFrameContext::agc.minFrameDuration\n> > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > + *\n> > > * \\var IPAFrameContext::agc.maxFrameDuration\n> > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > *\n> > > + * \\var IPAFrameContext::agc.frameDuration\n> > > + * \\brief The actual FrameDuration used by the algorithm for the frame\n> > > + *\n> > > * \\var IPAFrameContext::agc.updateMetering\n> > > * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> > > */\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index e274d9b01e1c..60c4d647f1ef 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -79,6 +79,7 @@ struct IPAActiveState {\n> > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > +\t\tutils::Duration minFrameDuration;\n> > > \t\tutils::Duration maxFrameDuration;\n> > > \t} agc;\n> > >\n> > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {\n> > > \tstruct {\n> > > \t\tuint32_t exposure;\n> > > \t\tdouble gain;\n> > > +\t\tuint32_t vblank;\n> > > \t\tbool autoEnabled;\n> > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > +\t\tutils::Duration minFrameDuration;\n> > > \t\tutils::Duration maxFrameDuration;\n> > > +\t\tutils::Duration frameDuration;\n> > > \t\tbool updateMetering;\n> > > \t} agc;\n> > >\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 47777ece783f..17d56fb4e901 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > \tuint32_t exposure = frameContext.agc.exposure;\n> > > \tuint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> > > +\tuint32_t vblank = frameContext.agc.vblank;\n> > >\n> > > \tControlList ctrls(sensorControls_);\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> > >\n> > > \tsetSensorControls.emit(frame, ctrls);\n> > > }\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 42961c120036..1ec12185bb78 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> > > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > > \t\t\t\t\t const ControlList &sensorControls)\n> > > {\n> > > +\tif (frame == 0) {\n> > > +\t\tControlList controls = sensorControls;\n> > > +\t\tsensor_->setControls(&controls);\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> >\n> > I might have missed what is this for :)\n>\n> iirc I needed this because the vlbank wasn't being applied...\n> until the next time the control was set.\n>\n\nI see. It's then an issue that is worth fixing but it's not related to\nthis patch ?\n\nHow does this happen ? Do we push the control list to delayed controls\n-after- we get the frameStart event notification ? Are controls for\nthe first frame lost ?\n\n\n>\n> Paul\n>\n> >\n> > > \tdelayedCtrls_->push(sensorControls);\n> > > }\n> > >\n> > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > \tstd::unordered_map params = {\n> > > \t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> > > \t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n> > > +\t\t{ V4L2_CID_VBLANK, { 1, false } },\n> > > \t};\n> > >\n> > > \tdata->delayedCtrls_ =\n> > > --\n> > > 2.34.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 4139BBDB1C\n\tfor ;\n\tMon, 4 Nov 2024 13:35:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3534C653D0;\n\tMon, 4 Nov 2024 14:35:11 +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 3AC0565399\n\tfor ;\n\tMon, 4 Nov 2024 14:35:09 +0100 (CET)","from ideasonboard.com (mob-5-90-48-188.net.vodafone.it\n\t[5.90.48.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D110526;\n\tMon, 4 Nov 2024 14:35:02 +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=\"GAvnfj2u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730727302;\n\tbh=i+aiEKOMnLDdNQQshCMmGuFnsqDLpPtK+8/AF6e1tOE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GAvnfj2u/oP9NeCC7pu21Ky7IX2pyc1gLPDaNBkKR2psmauoqaScBwHl9nMYSfyjL\n\twcqcYGP2/RGgLyZw3ipGATmOdR+zqHg5SFQc6cMhN1r4IMpfv/hZBbgNpCqAxgx0+I\n\turjqOQZmvxCbMvS6h7eVklqt23jhwgVfiEXc1Sfc=","Date":"Mon, 4 Nov 2024 14:35:06 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"Jacopo Mondi , \n\tKieran Bingham ,\n\tlibcamera devel ","Subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","Message-ID":"","References":"<20241014154747.2295253-1-kieran.bingham@ideasonboard.com>\n\t<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":32235,"web_url":"https://patchwork.libcamera.org/comment/32235/","msgid":"","date":"2024-11-18T14:51:21","subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Nov 04, 2024 at 02:35:06PM +0100, Jacopo Mondi wrote:\n> Hi Paul\n> \n> On Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:\n> > On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:\n> > > Hi Kieran\n> > >\n> > > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:\n> > > > From: Paul Elder \n> > > >\n> > > > Add support to rkisp1 for controlling the framerate via the\n> > > > FrameDurationLimits control.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------\n> > > > src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-\n> > > > src/ipa/rkisp1/ipa_context.h | 4 ++\n> > > > src/ipa/rkisp1/rkisp1.cpp | 2 +\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++\n> > > > 5 files changed, 68 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index e23ab120b3e2..088ecf42c480 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > >\n> > > > \t/* Limit the frame duration to match current initialisation */\n> > > > \tControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > > +\tcontext.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get());\n> > > > \tcontext.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get());\n> > > >\n> > > > \t/*\n> > > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,\n> > > >\n> > > > \tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > > > \tif (frameDurationLimits) {\n> > > > -\t\tutils::Duration maxFrameDuration =\n> > > > -\t\t\tstd::chrono::milliseconds((*frameDurationLimits).back());\n> > > > -\t\tagc.maxFrameDuration = maxFrameDuration;\n> > > > +\t\t/* Limit the control value to the limits in ControlInfo */\n> > > > +\t\tControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > > +\t\tint64_t minFrameDuration =\n> > > > +\t\t\tstd::clamp((*frameDurationLimits).front(),\n> > > > +\t\t\t\t limits.min().get(),\n> > > > +\t\t\t\t limits.max().get());\n> > > > +\t\tint64_t maxFrameDuration =\n> > > > +\t\t\tstd::clamp((*frameDurationLimits).back(),\n> > > > +\t\t\t\t limits.min().get(),\n> > > > +\t\t\t\t limits.max().get());\n> > > > +\n> > >\n> > > We operate on the assumption the control is well-formed, right ? iow\n> > > that frameDurationLimits[0] < frameDurationLimits[1] ?\n> >\n> > Yeah.\n> >\n> > >\n> > > > +\t\tagc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> > > > +\t\tagc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> > > > \t}\n> > > > +\tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n> > > > \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> > > > }\n> > > >\n> > > > @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > > \t\t\t\t * frameContext.sensor.exposure;\n> > > > \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > > > \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > > > +\tmetadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get());\n> > > > \tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > > > -\n> > > > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > > > -\tuint32_t vTotal = context.configuration.sensor.size.height\n> > > > -\t\t\t+ context.configuration.sensor.defVBlank;\n> > > > -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > > > -\t\t\t\t * vTotal;\n> > > > -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > > > -\n> > > > \tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > > > \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > > > \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > \t\treturn;\n> > > > \t}\n> > > >\n> > > > +\tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> > > > \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> > > >\n> > > > \t/*\n> > > > @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > \t\t [](uint32_t x) { return x >> 4; });\n> > > > \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > > >\n> > > > +\t/*\n> > > > +\t * Limit the allowed shutter speeds for the exposure helper based on\n> > > > +\t * the frame duration limits.\n> > > > +\t */\n> > > > +\tutils::Duration minShutterSpeed =\n> > > > +\t\tstd::clamp(frameContext.agc.minFrameDuration,\n> > > > +\t\t\t context.configuration.sensor.minShutterSpeed,\n> > > > +\t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > > \tutils::Duration maxShutterSpeed =\n> > > > \t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> > > > \t\t\t context.configuration.sensor.minShutterSpeed,\n> > > > \t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > > -\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> > > > +\tsetLimits(minShutterSpeed,\n> > > > \t\t maxShutterSpeed,\n> > > > \t\t context.configuration.sensor.minAnalogueGain,\n> > > > \t\t context.configuration.sensor.maxAnalogueGain);\n> > > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > \tactiveState.agc.automatic.exposure = shutterTime / lineDuration;\n> > > > \tactiveState.agc.automatic.gain = aGain;\n> > > >\n> > > > +\t/*\n> > > > +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n> > > > +\t * this if we need to expand the shutter time calculated to fill the\n> > > > +\t * remaining time so that we do not run faster than the minimum frame\n> > > > +\t * duration (maximum requested frame rate) when we have short exposures.\n> > > > +\t */\n> > > > +\tutils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,\n> > > > +\t\t\t\t\t\t shutterTime);\n> > > > +\n> > > > +\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > > > +\n> > > > +\t/* Update accounting for line length quantization. */\n> > > > +\tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > > > +\n> > > > \tfillMetadata(context, frameContext, metadata);\n> > > > \texpMeans_ = {};\n> > > > }\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > index 14d0c02a2b32..c5eb0d64ec29 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > * \\var IPAActiveState::agc.meteringMode\n> > > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > > *\n> > > > + * \\var IPAActiveState::agc.minFrameDuration\n> > > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > > + *\n> > > > * \\var IPAActiveState::agc.maxFrameDuration\n> > > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > > */\n> > > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > * \\brief Automatic Gain Control parameters for this frame\n> > > > *\n> > > > * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > > - * applied to the sensor in order to take effect for this frame.\n> > > > + * applied to the sensor in order to take effect for this frame. Additionally\n> > > > + * the vertical blanking period is determined to maintain a consistent frame\n> > > > + * rate matched to the FrameDurationLimits as set by the user.\n> > > > *\n> > > > * \\var IPAFrameContext::agc.exposure\n> > > > * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> > > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > *\n> > > > * The gain should be adapted to the sensor specific gain code before applying.\n> > > > *\n> > > > + * \\var IPAFrameContext::agc.vblank\n> > > > + * \\brief Vertical blanking parameter computed by the algorithm\n> > > > + *\n> > > > * \\var IPAFrameContext::agc.autoEnabled\n> > > > * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > > > *\n> > > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {\n> > > > * \\var IPAFrameContext::agc.meteringMode\n> > > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > > *\n> > > > + * \\var IPAFrameContext::agc.minFrameDuration\n> > > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > > + *\n> > > > * \\var IPAFrameContext::agc.maxFrameDuration\n> > > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > > *\n> > > > + * \\var IPAFrameContext::agc.frameDuration\n> > > > + * \\brief The actual FrameDuration used by the algorithm for the frame\n> > > > + *\n> > > > * \\var IPAFrameContext::agc.updateMetering\n> > > > * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> > > > */\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index e274d9b01e1c..60c4d647f1ef 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -79,6 +79,7 @@ struct IPAActiveState {\n> > > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > > +\t\tutils::Duration minFrameDuration;\n> > > > \t\tutils::Duration maxFrameDuration;\n> > > > \t} agc;\n> > > >\n> > > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {\n> > > > \tstruct {\n> > > > \t\tuint32_t exposure;\n> > > > \t\tdouble gain;\n> > > > +\t\tuint32_t vblank;\n> > > > \t\tbool autoEnabled;\n> > > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > > +\t\tutils::Duration minFrameDuration;\n> > > > \t\tutils::Duration maxFrameDuration;\n> > > > +\t\tutils::Duration frameDuration;\n> > > > \t\tbool updateMetering;\n> > > > \t} agc;\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 47777ece783f..17d56fb4e901 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > \tuint32_t exposure = frameContext.agc.exposure;\n> > > > \tuint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> > > > +\tuint32_t vblank = frameContext.agc.vblank;\n> > > >\n> > > > \tControlList ctrls(sensorControls_);\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> > > >\n> > > > \tsetSensorControls.emit(frame, ctrls);\n> > > > }\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 42961c120036..1ec12185bb78 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> > > > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > > > \t\t\t\t\t const ControlList &sensorControls)\n> > > > {\n> > > > +\tif (frame == 0) {\n> > > > +\t\tControlList controls = sensorControls;\n> > > > +\t\tsensor_->setControls(&controls);\n> > > > +\t\treturn;\n> > > > +\t}\n> > > > +\n> > >\n> > > I might have missed what is this for :)\n> >\n> > iirc I needed this because the vlbank wasn't being applied...\n> > until the next time the control was set.\n> >\n> \n> I see. It's then an issue that is worth fixing but it's not related to\n> this patch ?\n> \n> How does this happen ? Do we push the control list to delayed controls\n> -after- we get the frameStart event notification ? Are controls for\n> the first frame lost ?\n\nI'm pretty sure it's that the vblank isn't ever set until process(), so\nuntil a stats buffer comes in. Before that, at start() time, it's just\nset to the initial default value... which is zero and therefore out of\nbounds and does nothing.\n\n\nPaul\n\n> >\n> > >\n> > > > \tdelayedCtrls_->push(sensorControls);\n> > > > }\n> > > >\n> > > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > \tstd::unordered_map params = {\n> > > > \t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> > > > \t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n> > > > +\t\t{ V4L2_CID_VBLANK, { 1, false } },\n> > > > \t};\n> > > >\n> > > > \tdata->delayedCtrls_ =\n> > > > --\n> > > > 2.34.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 198F4C32EA\n\tfor ;\n\tMon, 18 Nov 2024 14:51:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21247658E1;\n\tMon, 18 Nov 2024 15:51:32 +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 2318F60532\n\tfor ;\n\tMon, 18 Nov 2024 15:51:29 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:fb8e:b30c:9a4a:eed0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3BA66A57;\n\tMon, 18 Nov 2024 15:51:10 +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=\"cza2y9pe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731941472;\n\tbh=YRm86ddnEXyjYMuW+gZ+06FJsOD4/QRGRJ1PbQN/ekU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cza2y9peo6Vyj7huJtvCnl8NAHd5NzHLkJChPK0xBYMeMkTEMtkI0a9LxvWRyScSM\n\tLsKXqgyoORB4wedL9P6CE35EsKQVhmPh/PLjntY4AF3ugdKRMvZgNG4EKn0JbK9OPT\n\twSSlW7zddBjd17Xq1mMh5CKJojXOfTsvjNep9ONo=","Date":"Mon, 18 Nov 2024 23:51:21 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","Subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","Message-ID":"","References":"<20241014154747.2295253-1-kieran.bingham@ideasonboard.com>\n\t<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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":32238,"web_url":"https://patchwork.libcamera.org/comment/32238/","msgid":"","date":"2024-11-18T15:30:23","subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul\n\nOn Mon, Nov 18, 2024 at 11:51:21PM +0900, Paul Elder wrote:\n> On Mon, Nov 04, 2024 at 02:35:06PM +0100, Jacopo Mondi wrote:\n> > Hi Paul\n> >\n> > On Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:\n> > > On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:\n> > > > Hi Kieran\n> > > >\n> > > > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:\n> > > > > From: Paul Elder \n> > > > >\n> > > > > Add support to rkisp1 for controlling the framerate via the\n> > > > > FrameDurationLimits control.\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > > Signed-off-by: Kieran Bingham \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------\n> > > > > src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-\n> > > > > src/ipa/rkisp1/ipa_context.h | 4 ++\n> > > > > src/ipa/rkisp1/rkisp1.cpp | 2 +\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++\n> > > > > 5 files changed, 68 insertions(+), 13 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > index e23ab120b3e2..088ecf42c480 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > >\n> > > > > \t/* Limit the frame duration to match current initialisation */\n> > > > > \tControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > > > +\tcontext.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get());\n> > > > > \tcontext.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get());\n> > > > >\n> > > > > \t/*\n> > > > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,\n> > > > >\n> > > > > \tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > > > > \tif (frameDurationLimits) {\n> > > > > -\t\tutils::Duration maxFrameDuration =\n> > > > > -\t\t\tstd::chrono::milliseconds((*frameDurationLimits).back());\n> > > > > -\t\tagc.maxFrameDuration = maxFrameDuration;\n> > > > > +\t\t/* Limit the control value to the limits in ControlInfo */\n> > > > > +\t\tControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > > > +\t\tint64_t minFrameDuration =\n> > > > > +\t\t\tstd::clamp((*frameDurationLimits).front(),\n> > > > > +\t\t\t\t limits.min().get(),\n> > > > > +\t\t\t\t limits.max().get());\n> > > > > +\t\tint64_t maxFrameDuration =\n> > > > > +\t\t\tstd::clamp((*frameDurationLimits).back(),\n> > > > > +\t\t\t\t limits.min().get(),\n> > > > > +\t\t\t\t limits.max().get());\n> > > > > +\n> > > >\n> > > > We operate on the assumption the control is well-formed, right ? iow\n> > > > that frameDurationLimits[0] < frameDurationLimits[1] ?\n> > >\n> > > Yeah.\n> > >\n> > > >\n> > > > > +\t\tagc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> > > > > +\t\tagc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> > > > > \t}\n> > > > > +\tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n> > > > > \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> > > > > }\n> > > > >\n> > > > > @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > > > \t\t\t\t * frameContext.sensor.exposure;\n> > > > > \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > > > > \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > > > > +\tmetadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get());\n> > > > > \tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > > > > -\n> > > > > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > > > > -\tuint32_t vTotal = context.configuration.sensor.size.height\n> > > > > -\t\t\t+ context.configuration.sensor.defVBlank;\n> > > > > -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > > > > -\t\t\t\t * vTotal;\n> > > > > -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > > > > -\n> > > > > \tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > > > > \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > > > > \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > > > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > \t\treturn;\n> > > > > \t}\n> > > > >\n> > > > > +\tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> > > > > \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> > > > >\n> > > > > \t/*\n> > > > > @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > \t\t [](uint32_t x) { return x >> 4; });\n> > > > > \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > > > >\n> > > > > +\t/*\n> > > > > +\t * Limit the allowed shutter speeds for the exposure helper based on\n> > > > > +\t * the frame duration limits.\n> > > > > +\t */\n> > > > > +\tutils::Duration minShutterSpeed =\n> > > > > +\t\tstd::clamp(frameContext.agc.minFrameDuration,\n> > > > > +\t\t\t context.configuration.sensor.minShutterSpeed,\n> > > > > +\t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > > > \tutils::Duration maxShutterSpeed =\n> > > > > \t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> > > > > \t\t\t context.configuration.sensor.minShutterSpeed,\n> > > > > \t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > > > -\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> > > > > +\tsetLimits(minShutterSpeed,\n> > > > > \t\t maxShutterSpeed,\n> > > > > \t\t context.configuration.sensor.minAnalogueGain,\n> > > > > \t\t context.configuration.sensor.maxAnalogueGain);\n> > > > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > \tactiveState.agc.automatic.exposure = shutterTime / lineDuration;\n> > > > > \tactiveState.agc.automatic.gain = aGain;\n> > > > >\n> > > > > +\t/*\n> > > > > +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n> > > > > +\t * this if we need to expand the shutter time calculated to fill the\n> > > > > +\t * remaining time so that we do not run faster than the minimum frame\n> > > > > +\t * duration (maximum requested frame rate) when we have short exposures.\n> > > > > +\t */\n> > > > > +\tutils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,\n> > > > > +\t\t\t\t\t\t shutterTime);\n> > > > > +\n> > > > > +\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > > > > +\n> > > > > +\t/* Update accounting for line length quantization. */\n> > > > > +\tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > > > > +\n> > > > > \tfillMetadata(context, frameContext, metadata);\n> > > > > \texpMeans_ = {};\n> > > > > }\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > index 14d0c02a2b32..c5eb0d64ec29 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > * \\var IPAActiveState::agc.meteringMode\n> > > > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > > > *\n> > > > > + * \\var IPAActiveState::agc.minFrameDuration\n> > > > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > > > + *\n> > > > > * \\var IPAActiveState::agc.maxFrameDuration\n> > > > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > > > */\n> > > > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > * \\brief Automatic Gain Control parameters for this frame\n> > > > > *\n> > > > > * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > > > - * applied to the sensor in order to take effect for this frame.\n> > > > > + * applied to the sensor in order to take effect for this frame. Additionally\n> > > > > + * the vertical blanking period is determined to maintain a consistent frame\n> > > > > + * rate matched to the FrameDurationLimits as set by the user.\n> > > > > *\n> > > > > * \\var IPAFrameContext::agc.exposure\n> > > > > * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> > > > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > *\n> > > > > * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > *\n> > > > > + * \\var IPAFrameContext::agc.vblank\n> > > > > + * \\brief Vertical blanking parameter computed by the algorithm\n> > > > > + *\n> > > > > * \\var IPAFrameContext::agc.autoEnabled\n> > > > > * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > > > > *\n> > > > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > * \\var IPAFrameContext::agc.meteringMode\n> > > > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > > > *\n> > > > > + * \\var IPAFrameContext::agc.minFrameDuration\n> > > > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > > > + *\n> > > > > * \\var IPAFrameContext::agc.maxFrameDuration\n> > > > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > > > *\n> > > > > + * \\var IPAFrameContext::agc.frameDuration\n> > > > > + * \\brief The actual FrameDuration used by the algorithm for the frame\n> > > > > + *\n> > > > > * \\var IPAFrameContext::agc.updateMetering\n> > > > > * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> > > > > */\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index e274d9b01e1c..60c4d647f1ef 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > @@ -79,6 +79,7 @@ struct IPAActiveState {\n> > > > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > > > +\t\tutils::Duration minFrameDuration;\n> > > > > \t\tutils::Duration maxFrameDuration;\n> > > > > \t} agc;\n> > > > >\n> > > > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {\n> > > > > \tstruct {\n> > > > > \t\tuint32_t exposure;\n> > > > > \t\tdouble gain;\n> > > > > +\t\tuint32_t vblank;\n> > > > > \t\tbool autoEnabled;\n> > > > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > > > +\t\tutils::Duration minFrameDuration;\n> > > > > \t\tutils::Duration maxFrameDuration;\n> > > > > +\t\tutils::Duration frameDuration;\n> > > > > \t\tbool updateMetering;\n> > > > > \t} agc;\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > index 47777ece783f..17d56fb4e901 100644\n> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > \tuint32_t exposure = frameContext.agc.exposure;\n> > > > > \tuint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> > > > > +\tuint32_t vblank = frameContext.agc.vblank;\n> > > > >\n> > > > > \tControlList ctrls(sensorControls_);\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> > > > >\n> > > > > \tsetSensorControls.emit(frame, ctrls);\n> > > > > }\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index 42961c120036..1ec12185bb78 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> > > > > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > > > > \t\t\t\t\t const ControlList &sensorControls)\n> > > > > {\n> > > > > +\tif (frame == 0) {\n> > > > > +\t\tControlList controls = sensorControls;\n> > > > > +\t\tsensor_->setControls(&controls);\n> > > > > +\t\treturn;\n> > > > > +\t}\n> > > > > +\n> > > >\n> > > > I might have missed what is this for :)\n> > >\n> > > iirc I needed this because the vlbank wasn't being applied...\n> > > until the next time the control was set.\n> > >\n> >\n> > I see. It's then an issue that is worth fixing but it's not related to\n> > this patch ?\n> >\n> > How does this happen ? Do we push the control list to delayed controls\n> > -after- we get the frameStart event notification ? Are controls for\n> > the first frame lost ?\n>\n> I'm pretty sure it's that the vblank isn't ever set until process(), so\n\nmmm, maybe I'm confused.\n\nIPARkISP1::start() calls setControls(0) which emits setSensorControls.\n\nSo we get here just after start() from\n\nvoid IPARkISP1::setControls(unsigned int frame)\n{\n\tuint32_t vblank = frameContext.agc.vblank;\n\tctrls.set(V4L2_CID_VBLANK, static_cast(vblank));\n\tsetSensorControls.emit(frame, ctrls);\n}\n\nSo either\n1) frameContext.agc.vblank is not initialized to the right value\n - what's \"right\" ? The value set at Camera::start ? This is\n currently ignored by\n\n int PipelineHandlerRkISP1::start(Camera *camera,\n [[maybe_unused]] const ControlList *controls)\n\n2) vblank has the right value, but DelayedControls has already\n received the frame start event for frame#0 so the control is lost ?\n\nCould you help clarify this, and if this change is related to this\nseries or it can be fixed separately ?\n\nThanks\n\n> until a stats buffer comes in. Before that, at start() time, it's just\n> set to the initial default value... which is zero and therefore out of\n> bounds and does nothing.\n>\n>\n> Paul\n>\n> > >\n> > > >\n> > > > > \tdelayedCtrls_->push(sensorControls);\n> > > > > }\n> > > > >\n> > > > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > \tstd::unordered_map params = {\n> > > > > \t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> > > > > \t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n> > > > > +\t\t{ V4L2_CID_VBLANK, { 1, false } },\n> > > > > \t};\n> > > > >\n> > > > > \tdata->delayedCtrls_ =\n> > > > > --\n> > > > > 2.34.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 234D6C32DD\n\tfor ;\n\tMon, 18 Nov 2024 15:30:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CF42658E6;\n\tMon, 18 Nov 2024 16:30:31 +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 90BDF658DC\n\tfor ;\n\tMon, 18 Nov 2024 16:30:28 +0100 (CET)","from ideasonboard.com (net-93-150-224-74.cust.vodafonedsl.it\n\t[93.150.224.74])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D326834D;\n\tMon, 18 Nov 2024 16:30:10 +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=\"Wyf1Bi3Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731943811;\n\tbh=6Bd8rXwi3ZHg4gFsAUdzI7gslqvx7otayR8fSlcsLCg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wyf1Bi3YTYdK0GjRB1eD2jsLzk0h1ppeO5GGAO7To8LQTcdV3waltgoe3/pSLyGGJ\n\tROZAHsxova0OL/2+cy1EUcmXPM05HEYjuknA8Vx4YPHf4IP9yqPzkN9f3SFcOMb2GX\n\tvXjWz1rvlWT2uieBSkrtWHTizFxnsnkB/1GizNCI=","Date":"Mon, 18 Nov 2024 16:30:23 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"Jacopo Mondi , \n\tKieran Bingham ,\n\tlibcamera devel ","Subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","Message-ID":"","References":"<20241014154747.2295253-1-kieran.bingham@ideasonboard.com>\n\t<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":32241,"web_url":"https://patchwork.libcamera.org/comment/32241/","msgid":"","date":"2024-11-18T15:59:17","subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Nov 18, 2024 at 04:30:23PM +0100, Jacopo Mondi wrote:\n> Hi Paul\n> \n> On Mon, Nov 18, 2024 at 11:51:21PM +0900, Paul Elder wrote:\n> > On Mon, Nov 04, 2024 at 02:35:06PM +0100, Jacopo Mondi wrote:\n> > > Hi Paul\n> > >\n> > > On Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:\n> > > > On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:\n> > > > > Hi Kieran\n> > > > >\n> > > > > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:\n> > > > > > From: Paul Elder \n> > > > > >\n> > > > > > Add support to rkisp1 for controlling the framerate via the\n> > > > > > FrameDurationLimits control.\n> > > > > >\n> > > > > > Signed-off-by: Paul Elder \n> > > > > > Signed-off-by: Kieran Bingham \n> > > > > > ---\n> > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 52 ++++++++++++++++++------\n> > > > > > src/ipa/rkisp1/ipa_context.cpp | 16 +++++++-\n> > > > > > src/ipa/rkisp1/ipa_context.h | 4 ++\n> > > > > > src/ipa/rkisp1/rkisp1.cpp | 2 +\n> > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++++\n> > > > > > 5 files changed, 68 insertions(+), 13 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > index e23ab120b3e2..088ecf42c480 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > > >\n> > > > > > \t/* Limit the frame duration to match current initialisation */\n> > > > > > \tControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > > > > +\tcontext.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get());\n> > > > > > \tcontext.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get());\n> > > > > >\n> > > > > > \t/*\n> > > > > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,\n> > > > > >\n> > > > > > \tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > > > > > \tif (frameDurationLimits) {\n> > > > > > -\t\tutils::Duration maxFrameDuration =\n> > > > > > -\t\t\tstd::chrono::milliseconds((*frameDurationLimits).back());\n> > > > > > -\t\tagc.maxFrameDuration = maxFrameDuration;\n> > > > > > +\t\t/* Limit the control value to the limits in ControlInfo */\n> > > > > > +\t\tControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > > > > +\t\tint64_t minFrameDuration =\n> > > > > > +\t\t\tstd::clamp((*frameDurationLimits).front(),\n> > > > > > +\t\t\t\t limits.min().get(),\n> > > > > > +\t\t\t\t limits.max().get());\n> > > > > > +\t\tint64_t maxFrameDuration =\n> > > > > > +\t\t\tstd::clamp((*frameDurationLimits).back(),\n> > > > > > +\t\t\t\t limits.min().get(),\n> > > > > > +\t\t\t\t limits.max().get());\n> > > > > > +\n> > > > >\n> > > > > We operate on the assumption the control is well-formed, right ? iow\n> > > > > that frameDurationLimits[0] < frameDurationLimits[1] ?\n> > > >\n> > > > Yeah.\n> > > >\n> > > > >\n> > > > > > +\t\tagc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> > > > > > +\t\tagc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> > > > > > \t}\n> > > > > > +\tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n> > > > > > \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -330,15 +342,8 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > > > > \t\t\t\t * frameContext.sensor.exposure;\n> > > > > > \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > > > > > \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > > > > > +\tmetadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get());\n> > > > > > \tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > > > > > -\n> > > > > > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > > > > > -\tuint32_t vTotal = context.configuration.sensor.size.height\n> > > > > > -\t\t\t+ context.configuration.sensor.defVBlank;\n> > > > > > -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > > > > > -\t\t\t\t * vTotal;\n> > > > > > -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > > > > > -\n> > > > > > \tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > > > > > \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > > > > > \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > > > > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > \t\treturn;\n> > > > > > \t}\n> > > > > >\n> > > > > > +\tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> > > > > > \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> > > > > >\n> > > > > > \t/*\n> > > > > > @@ -418,11 +424,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > \t\t [](uint32_t x) { return x >> 4; });\n> > > > > > \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > > > > >\n> > > > > > +\t/*\n> > > > > > +\t * Limit the allowed shutter speeds for the exposure helper based on\n> > > > > > +\t * the frame duration limits.\n> > > > > > +\t */\n> > > > > > +\tutils::Duration minShutterSpeed =\n> > > > > > +\t\tstd::clamp(frameContext.agc.minFrameDuration,\n> > > > > > +\t\t\t context.configuration.sensor.minShutterSpeed,\n> > > > > > +\t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > > > > \tutils::Duration maxShutterSpeed =\n> > > > > > \t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> > > > > > \t\t\t context.configuration.sensor.minShutterSpeed,\n> > > > > > \t\t\t context.configuration.sensor.maxShutterSpeed);\n> > > > > > -\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> > > > > > +\tsetLimits(minShutterSpeed,\n> > > > > > \t\t maxShutterSpeed,\n> > > > > > \t\t context.configuration.sensor.minAnalogueGain,\n> > > > > > \t\t context.configuration.sensor.maxAnalogueGain);\n> > > > > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > \tactiveState.agc.automatic.exposure = shutterTime / lineDuration;\n> > > > > > \tactiveState.agc.automatic.gain = aGain;\n> > > > > >\n> > > > > > +\t/*\n> > > > > > +\t * Determine what our FrameDuration must be and adapt VBLANK to suit\n> > > > > > +\t * this if we need to expand the shutter time calculated to fill the\n> > > > > > +\t * remaining time so that we do not run faster than the minimum frame\n> > > > > > +\t * duration (maximum requested frame rate) when we have short exposures.\n> > > > > > +\t */\n> > > > > > +\tutils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,\n> > > > > > +\t\t\t\t\t\t shutterTime);\n> > > > > > +\n> > > > > > +\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > > > > > +\n> > > > > > +\t/* Update accounting for line length quantization. */\n> > > > > > +\tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > > > > > +\n> > > > > > \tfillMetadata(context, frameContext, metadata);\n> > > > > > \texpMeans_ = {};\n> > > > > > }\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > index 14d0c02a2b32..c5eb0d64ec29 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > * \\var IPAActiveState::agc.meteringMode\n> > > > > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > > > > *\n> > > > > > + * \\var IPAActiveState::agc.minFrameDuration\n> > > > > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > > > > + *\n> > > > > > * \\var IPAActiveState::agc.maxFrameDuration\n> > > > > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > > > > */\n> > > > > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > * \\brief Automatic Gain Control parameters for this frame\n> > > > > > *\n> > > > > > * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > > > > - * applied to the sensor in order to take effect for this frame.\n> > > > > > + * applied to the sensor in order to take effect for this frame. Additionally\n> > > > > > + * the vertical blanking period is determined to maintain a consistent frame\n> > > > > > + * rate matched to the FrameDurationLimits as set by the user.\n> > > > > > *\n> > > > > > * \\var IPAFrameContext::agc.exposure\n> > > > > > * \\brief Exposure time expressed as a number of lines computed by the algorithm\n> > > > > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > *\n> > > > > > * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > > *\n> > > > > > + * \\var IPAFrameContext::agc.vblank\n> > > > > > + * \\brief Vertical blanking parameter computed by the algorithm\n> > > > > > + *\n> > > > > > * \\var IPAFrameContext::agc.autoEnabled\n> > > > > > * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > > > > > *\n> > > > > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > * \\var IPAFrameContext::agc.meteringMode\n> > > > > > * \\brief Metering mode as set by the AeMeteringMode control\n> > > > > > *\n> > > > > > + * \\var IPAFrameContext::agc.minFrameDuration\n> > > > > > + * \\brief Minimum frame duration as set by the FrameDurationLimits control\n> > > > > > + *\n> > > > > > * \\var IPAFrameContext::agc.maxFrameDuration\n> > > > > > * \\brief Maximum frame duration as set by the FrameDurationLimits control\n> > > > > > *\n> > > > > > + * \\var IPAFrameContext::agc.frameDuration\n> > > > > > + * \\brief The actual FrameDuration used by the algorithm for the frame\n> > > > > > + *\n> > > > > > * \\var IPAFrameContext::agc.updateMetering\n> > > > > > * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> > > > > > */\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > index e274d9b01e1c..60c4d647f1ef 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > @@ -79,6 +79,7 @@ struct IPAActiveState {\n> > > > > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > > > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > > > > +\t\tutils::Duration minFrameDuration;\n> > > > > > \t\tutils::Duration maxFrameDuration;\n> > > > > > \t} agc;\n> > > > > >\n> > > > > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {\n> > > > > > \tstruct {\n> > > > > > \t\tuint32_t exposure;\n> > > > > > \t\tdouble gain;\n> > > > > > +\t\tuint32_t vblank;\n> > > > > > \t\tbool autoEnabled;\n> > > > > > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > > > > > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > > > > > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > > > > > +\t\tutils::Duration minFrameDuration;\n> > > > > > \t\tutils::Duration maxFrameDuration;\n> > > > > > +\t\tutils::Duration frameDuration;\n> > > > > > \t\tbool updateMetering;\n> > > > > > \t} agc;\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > index 47777ece783f..17d56fb4e901 100644\n> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > > > \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > > \tuint32_t exposure = frameContext.agc.exposure;\n> > > > > > \tuint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> > > > > > +\tuint32_t vblank = frameContext.agc.vblank;\n> > > > > >\n> > > > > > \tControlList ctrls(sensorControls_);\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> > > > > >\n> > > > > > \tsetSensorControls.emit(frame, ctrls);\n> > > > > > }\n> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > index 42961c120036..1ec12185bb78 100644\n> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> > > > > > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > > > > > \t\t\t\t\t const ControlList &sensorControls)\n> > > > > > {\n> > > > > > +\tif (frame == 0) {\n> > > > > > +\t\tControlList controls = sensorControls;\n> > > > > > +\t\tsensor_->setControls(&controls);\n> > > > > > +\t\treturn;\n> > > > > > +\t}\n> > > > > > +\n> > > > >\n> > > > > I might have missed what is this for :)\n> > > >\n> > > > iirc I needed this because the vlbank wasn't being applied...\n> > > > until the next time the control was set.\n> > > >\n> > >\n> > > I see. It's then an issue that is worth fixing but it's not related to\n> > > this patch ?\n> > >\n> > > How does this happen ? Do we push the control list to delayed controls\n> > > -after- we get the frameStart event notification ? Are controls for\n> > > the first frame lost ?\n> >\n> > I'm pretty sure it's that the vblank isn't ever set until process(), so\n> \n> mmm, maybe I'm confused.\n> \n> IPARkISP1::start() calls setControls(0) which emits setSensorControls.\n> \n> So we get here just after start() from\n> \n> void IPARkISP1::setControls(unsigned int frame)\n> {\n> \tuint32_t vblank = frameContext.agc.vblank;\n> \tctrls.set(V4L2_CID_VBLANK, static_cast(vblank));\n> \tsetSensorControls.emit(frame, ctrls);\n> }\n> \n> So either\n> 1) frameContext.agc.vblank is not initialized to the right value\n\nYes, this is the case atm.\n\n> - what's \"right\" ? The value set at Camera::start ? This is\n> currently ignored by\n\nActually, it should probably be initialized at configure() time, when the\nFrameDurationLimits ControlInfo limits are computed, and defaulted to\nthat value. That way setControls() should work as expected.\n\n> \n> int PipelineHandlerRkISP1::start(Camera *camera,\n> [[maybe_unused]] const ControlList *controls)\n\nAnd then in addition, these should be forwarded in I think.\n\n> \n> 2) vblank has the right value, but DelayedControls has already\n> received the frame start event for frame#0 so the control is lost ?\n> \n> Could you help clarify this, and if this change is related to this\n> series or it can be fixed separately ?\n\nIt should indeed probably be fixed in this patch, because as you pointed\nout that hunk isn't very nice/correct.\n\n\nPaul\n\n> \n> Thanks\n> \n> > until a stats buffer comes in. Before that, at start() time, it's just\n> > set to the initial default value... which is zero and therefore out of\n> > bounds and does nothing.\n> >\n> >\n> > Paul\n> >\n> > > >\n> > > > >\n> > > > > > \tdelayedCtrls_->push(sensorControls);\n> > > > > > }\n> > > > > >\n> > > > > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > > \tstd::unordered_map params = {\n> > > > > > \t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> > > > > > \t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n> > > > > > +\t\t{ V4L2_CID_VBLANK, { 1, false } },\n> > > > > > \t};\n> > > > > >\n> > > > > > \tdata->delayedCtrls_ =\n> > > > > > --\n> > > > > > 2.34.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 910ABC32DD\n\tfor ;\n\tMon, 18 Nov 2024 15:59:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3956B658EC;\n\tMon, 18 Nov 2024 16:59:27 +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 9B711658E2\n\tfor ;\n\tMon, 18 Nov 2024 16:59:24 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:fb8e:b30c:9a4a:eed0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63DC734D;\n\tMon, 18 Nov 2024 16:59:06 +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=\"R83/ICPw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731945547;\n\tbh=+0Mws3JwUQg5VA+9iQP8NTWm37sdxfRHB5DjQvnyR6I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R83/ICPw0gztF3q/Onprn+GZOI6yeZ7eMJgKNs4doIkvvfSY93sWj/ehC8/F08vjA\n\tGav/eondfE4eJdxuVfs0s9Lblh3xRpOtsQaFu/jN0re9PMsagMzTSZk2p/AsGiiiMg\n\tRBlLF6JmDdLNZD+dWDBMMT4+x5iLEJ8VPf4P+7G4=","Date":"Tue, 19 Nov 2024 00:59:17 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","Subject":"Re: [PATCH 3/3] rkisp1: Honor the FrameDurationLimits control","Message-ID":"","References":"<20241014154747.2295253-1-kieran.bingham@ideasonboard.com>\n\t<20241014154747.2295253-4-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]