[PATCH 9/9] ipa: rkisp1: agc: Implement ExposureValue control

[Stefan Klug]

Hi Kieran,

Thank you for the review. 

On Mon, Mar 31, 2025 at 05:50:54PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-03-31 15:43:48)
> > Now that agc_mean_luminance supports exposure correction, implement the
> > corresponding ExposureValue control for rkisp1.
> 
> Oh, that's an exciting development ...
> 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
> >  src/ipa/rkisp1/ipa_context.h      |  2 ++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index b3ac9400b74f..8e77455e7afd 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -158,6 +158,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> >                             ControlValue(controls::AnalogueGainModeAuto));
> >         /* \todo Move this to the Camera class */
> >         context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
> > +       context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> >         context.ctrlMap.merge(controls());
> >  
> >         return 0;
> > @@ -180,6 +181,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >         context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> >         context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
> >         context.activeState.agc.autoGainEnabled = !context.configuration.raw;
> > +       context.activeState.agc.exposureValue = 0.0;
> >  
> >         context.activeState.agc.constraintMode =
> >                 static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> > @@ -302,6 +304,11 @@ void Agc::queueRequest(IPAContext &context,
> >                         static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> >         frameContext.agc.constraintMode = agc.constraintMode;
> >  
> > +       const auto &exposureValue = controls.get(controls::ExposureValue);
> > +       if (exposureValue)
> > +               agc.exposureValue = *exposureValue;
> > +       frameContext.agc.exposureValue = agc.exposureValue;
> > +
> >         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> >         if (frameDurationLimits) {
> >                 /* Limit the control value to the limits in ControlInfo */
> > @@ -408,6 +415,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> >         metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> >         metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> >         metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> > +       metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
> >  }
> >  
> >  /**
> > @@ -557,6 +565,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >         double analogueGain = frameContext.sensor.gain;
> >         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> >  
> > +       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> > +
> 
> while the patch overall sounds and looks pretty good - I'm not sure I've
> seen setExposureCompensation() added to the helpers yet - so perhaps
> this patch is supposed to be on a different series?

setExposureCompensation() was added in the patch before. It works
because the algorithm is derived from AgcMeanLuminance.

Best regards,
Stefan

> 
> --
> Kieran
> 
> 
> >         utils::Duration newExposureTime;
> >         double aGain, dGain;
> >         std::tie(newExposureTime, aGain, dGain) =
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index f0d504215d34..7ccc7b501aff 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -81,6 +81,7 @@ struct IPAActiveState {
> >  
> >                 bool autoExposureEnabled;
> >                 bool autoGainEnabled;
> > +               double exposureValue;
> >                 controls::AeConstraintModeEnum constraintMode;
> >                 controls::AeExposureModeEnum exposureMode;
> >                 controls::AeMeteringModeEnum meteringMode;
> > @@ -129,6 +130,7 @@ struct IPAFrameContext : public FrameContext {
> >         struct {
> >                 uint32_t exposure;
> >                 double gain;
> > +               double exposureValue;
> >                 uint32_t vblank;
> >                 bool autoExposureEnabled;
> >                 bool autoGainEnabled;
> > -- 
> > 2.43.0
> >