[{"id":33415,"web_url":"https://patchwork.libcamera.org/comment/33415/","msgid":"<174023334144.2200270.6784239118171985137@ping.linuxembedded.co.uk>","date":"2025-02-22T14:09:01","subject":"Re: [PATCH v2 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2025-02-21 09:20:45)\n> Add support to rkisp1 for controlling the framerate via the\n> FrameDurationLimits control.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>\n> \n> ---\n> Changes in v2:\n> - recover from bitrot\n> - fix setting frame duration limits in raw mode\n> ---\n>  src/ipa/rkisp1/algorithms/agc.cpp        | 60 +++++++++++++++++++-----\n>  src/ipa/rkisp1/algorithms/agc.h          |  3 ++\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 |  1 +\n>  6 files changed, 74 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 5fece545677e..4e0e3734117e 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -190,6 +190,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>  \n>         /* Limit the frame duration to match current initialisation */\n>         ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> +       context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());\n>         context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());\n>  \n>         /*\n> @@ -307,10 +308,21 @@ void Agc::queueRequest(IPAContext &context,\n>  \n>         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n>         if (frameDurationLimits) {\n> -               utils::Duration maxFrameDuration =\n> -                       std::chrono::milliseconds((*frameDurationLimits).back());\n> -               agc.maxFrameDuration = maxFrameDuration;\n> +               /* Limit the control value to the limits in ControlInfo */\n> +               ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> +               int64_t minFrameDuration =\n> +                       std::clamp((*frameDurationLimits).front(),\n> +                                  limits.min().get<int64_t>(),\n> +                                  limits.max().get<int64_t>());\n> +               int64_t maxFrameDuration =\n> +                       std::clamp((*frameDurationLimits).back(),\n> +                                  limits.min().get<int64_t>(),\n> +                                  limits.max().get<int64_t>());\n> +\n> +               agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> +               agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n>         }\n> +       frameContext.agc.minFrameDuration = agc.minFrameDuration;\n>         frameContext.agc.maxFrameDuration = agc.maxFrameDuration;\n>  }\n>  \n> @@ -387,6 +399,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>                                      * frameContext.sensor.exposure;\n>         metadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n>         metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());\n> +       metadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get<std::micro>());\n>         metadata.set(controls::ExposureTimeMode,\n>                      frameContext.agc.autoExposureEnabled\n>                      ? controls::ExposureTimeModeAuto\n> @@ -396,13 +409,6 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>                      ? controls::AnalogueGainModeAuto\n>                      : controls::AnalogueGainModeManual);\n>  \n> -       /* \\todo Use VBlank value calculated from each frame exposure. */\n> -       uint32_t vTotal = context.configuration.sensor.size.height\n> -                       + context.configuration.sensor.defVBlank;\n> -       utils::Duration frameDuration = context.configuration.sensor.lineDuration\n> -                                     * vTotal;\n> -       metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());\n\nAm I missing where the FrameDuration is now reported in the metadata ?\nor has it been removed completely.\n\nI think we should still be reporting the FrameDuration which has been\ndetermined. It should come from\n  frameContext.agc.frameDuration.get<std::micro>()\n\nnow shouldn't it ?\n\n\n\n> -\n>         metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n>         metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n>         metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> @@ -444,6 +450,27 @@ double Agc::estimateLuminance(double gain) const\n>         return ySum / expMeans_.size() / 255;\n>  }\n>  \n> +/**\n> + * \\brief Process frame duration and compute vblank\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] frameContext The current frame context\n> + * \\param[in] frameDuration The target frame duration\n> + *\n> + * Compute and populate vblank from the target frame duration.\n> + */\n> +void Agc::processFrameDuration(IPAContext &context,\n> +                              IPAFrameContext &frameContext,\n> +                              utils::Duration frameDuration)\n> +{\n> +       IPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> +       utils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> +\n> +       frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> +\n> +       /* Update frame duration accounting for line length quantization. */\n> +       frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> +}\n> +\n>  /**\n>   * \\brief Process RkISP1 statistics, and run AGC operations\n>   * \\param[in] context The shared IPA context\n> @@ -460,6 +487,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>                   ControlList &metadata)\n>  {\n>         if (!stats) {\n> +               processFrameDuration(context, frameContext,\n> +                                    frameContext.agc.minFrameDuration);\n>                 fillMetadata(context, frameContext, metadata);\n>                 return;\n>         }\n> @@ -497,7 +526,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>         double maxAnalogueGain;\n>  \n>         if (frameContext.agc.autoExposureEnabled) {\n> -               minExposureTime = context.configuration.sensor.minExposureTime;\n> +               minExposureTime = std::clamp(frameContext.agc.minFrameDuration,\n> +                                            context.configuration.sensor.minExposureTime,\n> +                                            context.configuration.sensor.maxExposureTime);\n>                 maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,\n>                                              context.configuration.sensor.minExposureTime,\n>                                              context.configuration.sensor.maxExposureTime);\n> @@ -541,6 +572,13 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>         activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n>         activeState.agc.automatic.gain = aGain;\n>  \n> +       /*\n> +        * Expand the target frame duration so that we do not run faster than\n> +        * the minimum frame duration when we have short exposures.\n> +        */\n> +       processFrameDuration(context, frameContext,\n> +                            std::max(frameContext.agc.minFrameDuration, newExposureTime));\n> +\n>         fillMetadata(context, frameContext, metadata);\n>         expMeans_ = {};\n>  }\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index aa86f2c5bc21..62bcde999fe3 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -50,6 +50,9 @@ private:\n>         void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>                           ControlList &metadata);\n>         double estimateLuminance(double gain) const override;\n> +       void processFrameDuration(IPAContext &context,\n> +                                 IPAFrameContext &frameContext,\n> +                                 utils::Duration frameDuration);\n>  \n>         Span<const uint8_t> expMeans_;\n>  \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 261c0472a4fc..99611bd5b390 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -180,6 +180,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> @@ -282,7 +285,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> @@ -292,6 +297,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.autoExposureEnabled\n>   * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n>   *\n> @@ -307,9 +315,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 c765b928a55f..474f7036f003 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -84,6 +84,7 @@ struct IPAActiveState {\n>                 controls::AeConstraintModeEnum constraintMode;\n>                 controls::AeExposureModeEnum exposureMode;\n>                 controls::AeMeteringModeEnum meteringMode;\n> +               utils::Duration minFrameDuration;\n>                 utils::Duration maxFrameDuration;\n>         } agc;\n>  \n> @@ -125,12 +126,15 @@ struct IPAFrameContext : public FrameContext {\n>         struct {\n>                 uint32_t exposure;\n>                 double gain;\n> +               uint32_t vblank;\n>                 bool autoExposureEnabled;\n>                 bool autoGainEnabled;\n>                 controls::AeConstraintModeEnum constraintMode;\n>                 controls::AeExposureModeEnum exposureMode;\n>                 controls::AeMeteringModeEnum meteringMode;\n> +               utils::Duration minFrameDuration;\n>                 utils::Duration maxFrameDuration;\n> +               utils::Duration frameDuration;\n>                 bool updateMetering;\n>                 bool autoExposureModeChange;\n>                 bool autoGainModeChange;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 0e761249d27c..7547d2f274f4 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -453,10 +453,12 @@ void IPARkISP1::setControls(unsigned int frame)\n>         IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>         uint32_t exposure = frameContext.agc.exposure;\n>         uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> +       uint32_t vblank = frameContext.agc.vblank;\n>  \n>         ControlList ctrls(sensorControls_);\n>         ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));\n>         ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));\n> +       ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));\n>  \n>         setSensorControls.emit(frame, ctrls);\n>  }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 1ac8d8ae7ed9..52633fe3cb85 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1325,6 +1325,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>         std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {\n>                 { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n>                 { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n> +               { V4L2_CID_VBLANK, { 1, false } },\n>         };\n>  \n>         data->delayedCtrls_ =\n> -- \n> 2.39.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 64159BDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 22 Feb 2025 14:09:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E7B2686B3;\n\tSat, 22 Feb 2025 15:09:07 +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 B3ADC6185A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 22 Feb 2025 15:09:04 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8080A3A2;\n\tSat, 22 Feb 2025 15:07:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V7244sk/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740233259;\n\tbh=1SlIZBUz42ELIjCHl1R2V/gdnt85Vl/SCSJoDK+wXyk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=V7244sk/ZHB8icDbEsjdUx8TDpKLzGL6oaRYw87j2bW+f+17P+yJJyOTnRleb4pEh\n\tq2UABGM59X0WNMV2Z+ulH1uODTJ0YliFnGRUdwrVx4j8zYomDUq5rAC7TdYVzaQhtE\n\tVVX5+6sMHF+0CiPGsAt6+V6+V3vAzHszrpBQC2B0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250221092045.3896021-4-paul.elder@ideasonboard.com>","References":"<20250221092045.3896021-1-paul.elder@ideasonboard.com>\n\t<20250221092045.3896021-4-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v2 3/3] rkisp1: Honor the FrameDurationLimits control","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Paul Elder <paul.elder@ideasonboard.com>, isaac.scott@ideasonboard.com, \n\tUmang Jain <umang.jain@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 22 Feb 2025 14:09:01 +0000","Message-ID":"<174023334144.2200270.6784239118171985137@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33424,"web_url":"https://patchwork.libcamera.org/comment/33424/","msgid":"<Z7vV006L4c3P_--Z@pyrite.rasen.tech>","date":"2025-02-24T02:13:39","subject":"Re: [PATCH v2 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 Sat, Feb 22, 2025 at 02:09:01PM +0000, Kieran Bingham wrote:\n> Quoting Paul Elder (2025-02-21 09:20:45)\n> > Add support to rkisp1 for controlling the framerate via the\n> > FrameDurationLimits control.\n> > \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>\n> > \n> > ---\n> > Changes in v2:\n> > - recover from bitrot\n> > - fix setting frame duration limits in raw mode\n> > ---\n> >  src/ipa/rkisp1/algorithms/agc.cpp        | 60 +++++++++++++++++++-----\n> >  src/ipa/rkisp1/algorithms/agc.h          |  3 ++\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 |  1 +\n> >  6 files changed, 74 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 5fece545677e..4e0e3734117e 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -190,6 +190,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> >  \n> >         /* Limit the frame duration to match current initialisation */\n> >         ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> > +       context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());\n> >         context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());\n> >  \n> >         /*\n> > @@ -307,10 +308,21 @@ void Agc::queueRequest(IPAContext &context,\n> >  \n> >         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> >         if (frameDurationLimits) {\n> > -               utils::Duration maxFrameDuration =\n> > -                       std::chrono::milliseconds((*frameDurationLimits).back());\n> > -               agc.maxFrameDuration = maxFrameDuration;\n> > +               /* Limit the control value to the limits in ControlInfo */\n> > +               ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> > +               int64_t minFrameDuration =\n> > +                       std::clamp((*frameDurationLimits).front(),\n> > +                                  limits.min().get<int64_t>(),\n> > +                                  limits.max().get<int64_t>());\n> > +               int64_t maxFrameDuration =\n> > +                       std::clamp((*frameDurationLimits).back(),\n> > +                                  limits.min().get<int64_t>(),\n> > +                                  limits.max().get<int64_t>());\n> > +\n> > +               agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> > +               agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> >         }\n> > +       frameContext.agc.minFrameDuration = agc.minFrameDuration;\n> >         frameContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> >  }\n> >  \n> > @@ -387,6 +399,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> >                                      * frameContext.sensor.exposure;\n> >         metadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> >         metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());\n> > +       metadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get<std::micro>());\n\n(It's here!)\n\n> >         metadata.set(controls::ExposureTimeMode,\n> >                      frameContext.agc.autoExposureEnabled\n> >                      ? controls::ExposureTimeModeAuto\n> > @@ -396,13 +409,6 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> >                      ? controls::AnalogueGainModeAuto\n> >                      : controls::AnalogueGainModeManual);\n> >  \n> > -       /* \\todo Use VBlank value calculated from each frame exposure. */\n> > -       uint32_t vTotal = context.configuration.sensor.size.height\n> > -                       + context.configuration.sensor.defVBlank;\n> > -       utils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > -                                     * vTotal;\n> > -       metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());\n> \n> Am I missing where the FrameDuration is now reported in the metadata ?\n> or has it been removed completely.\n> \n> I think we should still be reporting the FrameDuration which has been\n> determined. It should come from\n>   frameContext.agc.frameDuration.get<std::micro>()\n> \n> now shouldn't it ?\n\n(Yes)\n\n\n(Paul)\n\n> \n> \n> \n> > -\n> >         metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> >         metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> >         metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > @@ -444,6 +450,27 @@ double Agc::estimateLuminance(double gain) const\n> >         return ySum / expMeans_.size() / 255;\n> >  }\n> >  \n> > +/**\n> > + * \\brief Process frame duration and compute vblank\n> > + * \\param[in] context The shared IPA context\n> > + * \\param[in] frameContext The current frame context\n> > + * \\param[in] frameDuration The target frame duration\n> > + *\n> > + * Compute and populate vblank from the target frame duration.\n> > + */\n> > +void Agc::processFrameDuration(IPAContext &context,\n> > +                              IPAFrameContext &frameContext,\n> > +                              utils::Duration frameDuration)\n> > +{\n> > +       IPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> > +       utils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> > +\n> > +       frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > +\n> > +       /* Update frame duration accounting for line length quantization. */\n> > +       frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > +}\n> > +\n> >  /**\n> >   * \\brief Process RkISP1 statistics, and run AGC operations\n> >   * \\param[in] context The shared IPA context\n> > @@ -460,6 +487,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> >                   ControlList &metadata)\n> >  {\n> >         if (!stats) {\n> > +               processFrameDuration(context, frameContext,\n> > +                                    frameContext.agc.minFrameDuration);\n> >                 fillMetadata(context, frameContext, metadata);\n> >                 return;\n> >         }\n> > @@ -497,7 +526,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> >         double maxAnalogueGain;\n> >  \n> >         if (frameContext.agc.autoExposureEnabled) {\n> > -               minExposureTime = context.configuration.sensor.minExposureTime;\n> > +               minExposureTime = std::clamp(frameContext.agc.minFrameDuration,\n> > +                                            context.configuration.sensor.minExposureTime,\n> > +                                            context.configuration.sensor.maxExposureTime);\n> >                 maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,\n> >                                              context.configuration.sensor.minExposureTime,\n> >                                              context.configuration.sensor.maxExposureTime);\n> > @@ -541,6 +572,13 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> >         activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> >         activeState.agc.automatic.gain = aGain;\n> >  \n> > +       /*\n> > +        * Expand the target frame duration so that we do not run faster than\n> > +        * the minimum frame duration when we have short exposures.\n> > +        */\n> > +       processFrameDuration(context, frameContext,\n> > +                            std::max(frameContext.agc.minFrameDuration, newExposureTime));\n> > +\n> >         fillMetadata(context, frameContext, metadata);\n> >         expMeans_ = {};\n> >  }\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index aa86f2c5bc21..62bcde999fe3 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -50,6 +50,9 @@ private:\n> >         void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> >                           ControlList &metadata);\n> >         double estimateLuminance(double gain) const override;\n> > +       void processFrameDuration(IPAContext &context,\n> > +                                 IPAFrameContext &frameContext,\n> > +                                 utils::Duration frameDuration);\n> >  \n> >         Span<const uint8_t> expMeans_;\n> >  \n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 261c0472a4fc..99611bd5b390 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -180,6 +180,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> > @@ -282,7 +285,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> > @@ -292,6 +297,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.autoExposureEnabled\n> >   * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> >   *\n> > @@ -307,9 +315,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 c765b928a55f..474f7036f003 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -84,6 +84,7 @@ struct IPAActiveState {\n> >                 controls::AeConstraintModeEnum constraintMode;\n> >                 controls::AeExposureModeEnum exposureMode;\n> >                 controls::AeMeteringModeEnum meteringMode;\n> > +               utils::Duration minFrameDuration;\n> >                 utils::Duration maxFrameDuration;\n> >         } agc;\n> >  \n> > @@ -125,12 +126,15 @@ struct IPAFrameContext : public FrameContext {\n> >         struct {\n> >                 uint32_t exposure;\n> >                 double gain;\n> > +               uint32_t vblank;\n> >                 bool autoExposureEnabled;\n> >                 bool autoGainEnabled;\n> >                 controls::AeConstraintModeEnum constraintMode;\n> >                 controls::AeExposureModeEnum exposureMode;\n> >                 controls::AeMeteringModeEnum meteringMode;\n> > +               utils::Duration minFrameDuration;\n> >                 utils::Duration maxFrameDuration;\n> > +               utils::Duration frameDuration;\n> >                 bool updateMetering;\n> >                 bool autoExposureModeChange;\n> >                 bool autoGainModeChange;\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 0e761249d27c..7547d2f274f4 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -453,10 +453,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> >         IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >         uint32_t exposure = frameContext.agc.exposure;\n> >         uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> > +       uint32_t vblank = frameContext.agc.vblank;\n> >  \n> >         ControlList ctrls(sensorControls_);\n> >         ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));\n> >         ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));\n> > +       ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));\n> >  \n> >         setSensorControls.emit(frame, ctrls);\n> >  }\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 1ac8d8ae7ed9..52633fe3cb85 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1325,6 +1325,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >         std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {\n> >                 { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n> >                 { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n> > +               { V4L2_CID_VBLANK, { 1, false } },\n> >         };\n> >  \n> >         data->delayedCtrls_ =\n> > -- \n> > 2.39.2\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3F54DC32A9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 24 Feb 2025 02:13:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CE4C686C4;\n\tMon, 24 Feb 2025 03:13:49 +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 7F4DE61855\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 24 Feb 2025 03:13:48 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:2a10:bc6b:cb7f:b9f7])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD877353;\n\tMon, 24 Feb 2025 03:12:20 +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=\"VnkV/zJJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740363142;\n\tbh=/dCGYqy5HGNijLXO/nCPLsr8PDLdwrD+pd9FqxLGghE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VnkV/zJJ38Pex5JS/V3UoldzNw1HpSl9tJlAwvn6q10j7EcK/SaoQsqQxxMbW4mcy\n\t5jIoVRAupaCKs9mHlrKQw52FDqqqOW9LU+zgXBZdbX+C4zejjkWlBlMLK2uLB4B5mi\n\tr+kKEtbukqxp5RRzfJYJohQ7pPbIyzAHYzk0WU6M=","Date":"Mon, 24 Feb 2025 11:13:39 +0900","From":"Paul Elder <paul.elder@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, isaac.scott@ideasonboard.com,\n\tUmang Jain <umang.jain@ideasonboard.com>","Subject":"Re: [PATCH v2 3/3] rkisp1: Honor the FrameDurationLimits control","Message-ID":"<Z7vV006L4c3P_--Z@pyrite.rasen.tech>","References":"<20250221092045.3896021-1-paul.elder@ideasonboard.com>\n\t<20250221092045.3896021-4-paul.elder@ideasonboard.com>\n\t<174023334144.2200270.6784239118171985137@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<174023334144.2200270.6784239118171985137@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33451,"web_url":"https://patchwork.libcamera.org/comment/33451/","msgid":"<174040880620.2357762.10927745699680354094@ping.linuxembedded.co.uk>","date":"2025-02-24T14:53:26","subject":"Re: [PATCH v2 3/3] rkisp1: Honor the FrameDurationLimits control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2025-02-24 02:13:39)\n> On Sat, Feb 22, 2025 at 02:09:01PM +0000, Kieran Bingham wrote:\n> > Quoting Paul Elder (2025-02-21 09:20:45)\n> > > Add support to rkisp1 for controlling the framerate via the\n> > > FrameDurationLimits control.\n> > > \n> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>\n> > > \n> > > ---\n> > > Changes in v2:\n> > > - recover from bitrot\n> > > - fix setting frame duration limits in raw mode\n> > > ---\n> > >  src/ipa/rkisp1/algorithms/agc.cpp        | 60 +++++++++++++++++++-----\n> > >  src/ipa/rkisp1/algorithms/agc.h          |  3 ++\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 |  1 +\n> > >  6 files changed, 74 insertions(+), 12 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 5fece545677e..4e0e3734117e 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -190,6 +190,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > >  \n> > >         /* Limit the frame duration to match current initialisation */\n> > >         ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > +       context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());\n> > >         context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());\n> > >  \n> > >         /*\n> > > @@ -307,10 +308,21 @@ void Agc::queueRequest(IPAContext &context,\n> > >  \n> > >         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > >         if (frameDurationLimits) {\n> > > -               utils::Duration maxFrameDuration =\n> > > -                       std::chrono::milliseconds((*frameDurationLimits).back());\n> > > -               agc.maxFrameDuration = maxFrameDuration;\n> > > +               /* Limit the control value to the limits in ControlInfo */\n> > > +               ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];\n> > > +               int64_t minFrameDuration =\n> > > +                       std::clamp((*frameDurationLimits).front(),\n> > > +                                  limits.min().get<int64_t>(),\n> > > +                                  limits.max().get<int64_t>());\n> > > +               int64_t maxFrameDuration =\n> > > +                       std::clamp((*frameDurationLimits).back(),\n> > > +                                  limits.min().get<int64_t>(),\n> > > +                                  limits.max().get<int64_t>());\n> > > +\n> > > +               agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);\n> > > +               agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);\n> > >         }\n> > > +       frameContext.agc.minFrameDuration = agc.minFrameDuration;\n> > >         frameContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> > >  }\n> > >  \n> > > @@ -387,6 +399,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > >                                      * frameContext.sensor.exposure;\n> > >         metadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > >         metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());\n> > > +       metadata.set(controls::FrameDuration, frameContext.agc.frameDuration.get<std::micro>());\n> \n> (It's here!)\n> \n> > >         metadata.set(controls::ExposureTimeMode,\n> > >                      frameContext.agc.autoExposureEnabled\n> > >                      ? controls::ExposureTimeModeAuto\n> > > @@ -396,13 +409,6 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > >                      ? controls::AnalogueGainModeAuto\n> > >                      : controls::AnalogueGainModeManual);\n> > >  \n> > > -       /* \\todo Use VBlank value calculated from each frame exposure. */\n> > > -       uint32_t vTotal = context.configuration.sensor.size.height\n> > > -                       + context.configuration.sensor.defVBlank;\n> > > -       utils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > > -                                     * vTotal;\n> > > -       metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());\n> > \n> > Am I missing where the FrameDuration is now reported in the metadata ?\n> > or has it been removed completely.\n> > \n> > I think we should still be reporting the FrameDuration which has been\n> > determined. It should come from\n> >   frameContext.agc.frameDuration.get<std::micro>()\n> > \n> > now shouldn't it ?\n> \n> (Yes)\n\nAha, sorry - I did miss it!\n\n\nThen, \n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n\n> \n> \n> (Paul)\n> \n> > \n> > \n> > \n> > > -\n> > >         metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > >         metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > >         metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > > @@ -444,6 +450,27 @@ double Agc::estimateLuminance(double gain) const\n> > >         return ySum / expMeans_.size() / 255;\n> > >  }\n> > >  \n> > > +/**\n> > > + * \\brief Process frame duration and compute vblank\n> > > + * \\param[in] context The shared IPA context\n> > > + * \\param[in] frameContext The current frame context\n> > > + * \\param[in] frameDuration The target frame duration\n> > > + *\n> > > + * Compute and populate vblank from the target frame duration.\n> > > + */\n> > > +void Agc::processFrameDuration(IPAContext &context,\n> > > +                              IPAFrameContext &frameContext,\n> > > +                              utils::Duration frameDuration)\n> > > +{\n> > > +       IPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> > > +       utils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> > > +\n> > > +       frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > > +\n> > > +       /* Update frame duration accounting for line length quantization. */\n> > > +       frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\brief Process RkISP1 statistics, and run AGC operations\n> > >   * \\param[in] context The shared IPA context\n> > > @@ -460,6 +487,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > >                   ControlList &metadata)\n> > >  {\n> > >         if (!stats) {\n> > > +               processFrameDuration(context, frameContext,\n> > > +                                    frameContext.agc.minFrameDuration);\n> > >                 fillMetadata(context, frameContext, metadata);\n> > >                 return;\n> > >         }\n> > > @@ -497,7 +526,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > >         double maxAnalogueGain;\n> > >  \n> > >         if (frameContext.agc.autoExposureEnabled) {\n> > > -               minExposureTime = context.configuration.sensor.minExposureTime;\n> > > +               minExposureTime = std::clamp(frameContext.agc.minFrameDuration,\n> > > +                                            context.configuration.sensor.minExposureTime,\n> > > +                                            context.configuration.sensor.maxExposureTime);\n> > >                 maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,\n> > >                                              context.configuration.sensor.minExposureTime,\n> > >                                              context.configuration.sensor.maxExposureTime);\n> > > @@ -541,6 +572,13 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > >         activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > >         activeState.agc.automatic.gain = aGain;\n> > >  \n> > > +       /*\n> > > +        * Expand the target frame duration so that we do not run faster than\n> > > +        * the minimum frame duration when we have short exposures.\n> > > +        */\n> > > +       processFrameDuration(context, frameContext,\n> > > +                            std::max(frameContext.agc.minFrameDuration, newExposureTime));\n> > > +\n> > >         fillMetadata(context, frameContext, metadata);\n> > >         expMeans_ = {};\n> > >  }\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > index aa86f2c5bc21..62bcde999fe3 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > @@ -50,6 +50,9 @@ private:\n> > >         void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > >                           ControlList &metadata);\n> > >         double estimateLuminance(double gain) const override;\n> > > +       void processFrameDuration(IPAContext &context,\n> > > +                                 IPAFrameContext &frameContext,\n> > > +                                 utils::Duration frameDuration);\n> > >  \n> > >         Span<const uint8_t> expMeans_;\n> > >  \n> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > index 261c0472a4fc..99611bd5b390 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > @@ -180,6 +180,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> > > @@ -282,7 +285,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> > > @@ -292,6 +297,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.autoExposureEnabled\n> > >   * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> > >   *\n> > > @@ -307,9 +315,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 c765b928a55f..474f7036f003 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -84,6 +84,7 @@ struct IPAActiveState {\n> > >                 controls::AeConstraintModeEnum constraintMode;\n> > >                 controls::AeExposureModeEnum exposureMode;\n> > >                 controls::AeMeteringModeEnum meteringMode;\n> > > +               utils::Duration minFrameDuration;\n> > >                 utils::Duration maxFrameDuration;\n> > >         } agc;\n> > >  \n> > > @@ -125,12 +126,15 @@ struct IPAFrameContext : public FrameContext {\n> > >         struct {\n> > >                 uint32_t exposure;\n> > >                 double gain;\n> > > +               uint32_t vblank;\n> > >                 bool autoExposureEnabled;\n> > >                 bool autoGainEnabled;\n> > >                 controls::AeConstraintModeEnum constraintMode;\n> > >                 controls::AeExposureModeEnum exposureMode;\n> > >                 controls::AeMeteringModeEnum meteringMode;\n> > > +               utils::Duration minFrameDuration;\n> > >                 utils::Duration maxFrameDuration;\n> > > +               utils::Duration frameDuration;\n> > >                 bool updateMetering;\n> > >                 bool autoExposureModeChange;\n> > >                 bool autoGainModeChange;\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 0e761249d27c..7547d2f274f4 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -453,10 +453,12 @@ void IPARkISP1::setControls(unsigned int frame)\n> > >         IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > >         uint32_t exposure = frameContext.agc.exposure;\n> > >         uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);\n> > > +       uint32_t vblank = frameContext.agc.vblank;\n> > >  \n> > >         ControlList ctrls(sensorControls_);\n> > >         ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));\n> > >         ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));\n> > > +       ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));\n> > >  \n> > >         setSensorControls.emit(frame, ctrls);\n> > >  }\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 1ac8d8ae7ed9..52633fe3cb85 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -1325,6 +1325,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > >         std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {\n> > >                 { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n> > >                 { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n> > > +               { V4L2_CID_VBLANK, { 1, false } },\n> > >         };\n> > >  \n> > >         data->delayedCtrls_ =\n> > > -- \n> > > 2.39.2\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 F2A2FC32A9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 24 Feb 2025 14:53:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D364686F4;\n\tMon, 24 Feb 2025 15:53:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DC1761856\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 24 Feb 2025 15:53:29 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 97C1F55A;\n\tMon, 24 Feb 2025 15:52: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=\"wWDfLZN2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740408722;\n\tbh=sS1se/OeJQkxDXoPAqHwGlKawU4DQk/6shduysOl8Do=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=wWDfLZN2LGSFMAIFBCMXRIItwq/I6rnavqyDRmsQgldJ7XcU4Vzy58MWEMFMxjoyZ\n\t+8Ey9r44Pw5K/xGDvGpe2SzPXLQLRD7z8kzkgsTY3rc9616+z0liccXkPhuJ5XODCl\n\tGSS75Nvu46fvO7hiHmkseuX8AC/akaJmasVvXL3s=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<Z7vV006L4c3P_--Z@pyrite.rasen.tech>","References":"<20250221092045.3896021-1-paul.elder@ideasonboard.com>\n\t<20250221092045.3896021-4-paul.elder@ideasonboard.com>\n\t<174023334144.2200270.6784239118171985137@ping.linuxembedded.co.uk>\n\t<Z7vV006L4c3P_--Z@pyrite.rasen.tech>","Subject":"Re: [PATCH v2 3/3] rkisp1: Honor the FrameDurationLimits control","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, isaac.scott@ideasonboard.com,\n\tUmang Jain <umang.jain@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Date":"Mon, 24 Feb 2025 14:53:26 +0000","Message-ID":"<174040880620.2357762.10927745699680354094@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]