[PATCH v3 6/8] ipa: rkisp1: Port to the new AEGC controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 21 04:27:41 CET 2024
On Thu, Nov 14, 2024 at 04:20:40PM +0100, Stefan Klug wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Wed, Nov 13, 2024 at 10:12:54PM +0900, Paul Elder wrote:
> > The newly introduced controls to drive the AEGC algorithm allow
> > controlling the computation of the exposure time and analogue gain
> > separately.
> >
> > Augument the RkISP1 AEGC implementation to handle the exposure and gain
> > controls separately using the new controls.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > New in v3
> >
> > Back 2 years ago in v2 RkISP1 didn't yet support AeEnable properly yet
> > so the AeEnable control was simply removed.
> > ---
> > src/ipa/rkisp1/algorithms/agc.cpp | 68 ++++++++++++++++++++++++-------
> > src/ipa/rkisp1/ipa_context.cpp | 14 +++++--
> > src/ipa/rkisp1/ipa_context.h | 6 ++-
> > 3 files changed, 67 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 301b7ec26508..0ad206621516 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -148,7 +148,12 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> > if (ret)
> > return ret;
> >
> > - context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);
> > + context.ctrlMap[&controls::ExposureTimeMode] =
> > + ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> > + static_cast<int32_t>(controls::ExposureTimeModeManual));
> > + context.ctrlMap[&controls::AnalogueGainMode] =
> > + ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> > + static_cast<int32_t>(controls::AnalogueGainModeManual));
>
> I believe these should also have default values.
They should, and I think they should default to auto. Can we mandate in
the controls documentation that a camera that supports auto mode must
default to auto ?
> > context.ctrlMap.merge(controls());
> >
> > return 0;
> > @@ -169,7 +174,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > 10ms / context.configuration.sensor.lineDuration;
> > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > - context.activeState.agc.autoEnabled = !context.configuration.raw;
> > + context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
> > + context.activeState.agc.autoGainEnabled = !context.configuration.raw;
> >
>
> Ahh this actually sets the defaults internally, but the defaults above
> are the ones seen by applications like camshark.
>
> > context.activeState.agc.constraintMode =
> > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> > @@ -215,18 +221,42 @@ void Agc::queueRequest(IPAContext &context,
> > auto &agc = context.activeState.agc;
> >
> > if (!context.configuration.raw) {
> > - const auto &agcEnable = controls.get(controls::AeEnable);
> > - if (agcEnable && *agcEnable != agc.autoEnabled) {
> > - agc.autoEnabled = *agcEnable;
> > + const auto &aeEnable = controls.get(controls::ExposureTimeMode);
> > + if (aeEnable &&
> > + (*aeEnable == controls::ExposureTimeModeAuto) != agc.autoExposureEnabled) {
> > + agc.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
> >
> > LOG(RkISP1Agc, Debug)
> > - << (agc.autoEnabled ? "Enabling" : "Disabling")
> > - << " AGC";
> > + << (agc.autoExposureEnabled ? "Enabling" : "Disabling")
> > + << " AGC (exposure)";
> > +
> > + /*
> > + * If we go from auto -> manual with no manual control
> > + * set, use the last computed value
> > + */
> > + if (!agc.autoExposureEnabled && !controls.get(controls::ExposureTime))
> > + frameContext.agc.exposure = agc.automatic.exposure;
>
> Does this work? It sets the exposure time of the future frame (that is
> currently beeing queued) to the current value from active state. But
> that value will change depending on how many frames are queued already.
> I guess we need to set a flag and fetch the exposure time from
> activeState when this frame gets processed.
>
> > + }
> > +
> > + const auto &agEnable = controls.get(controls::AnalogueGainMode);
> > + if (agEnable &&
> > + (*agEnable == controls::AnalogueGainModeAuto) != agc.autoGainEnabled) {
> > + agc.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
> > +
> > + LOG(RkISP1Agc, Debug)
> > + << (agc.autoGainEnabled ? "Enabling" : "Disabling")
> > + << " AGC (gain)";
> > + /*
> > + * If we go from auto -> manual with no manual control
> > + * set, use the last computed value
> > + */
> > + if (!agc.autoGainEnabled && !controls.get(controls::AnalogueGain))
> > + frameContext.agc.gain = agc.automatic.gain;
>
> As above.
>
> > }
> > }
> >
> > const auto &exposure = controls.get(controls::ExposureTime);
> > - if (exposure && !agc.autoEnabled) {
> > + if (exposure && !agc.autoExposureEnabled) {
> > agc.manual.exposure = *exposure * 1.0us
> > / context.configuration.sensor.lineDuration;
> >
> > @@ -235,18 +265,19 @@ void Agc::queueRequest(IPAContext &context,
> > }
> >
> > const auto &gain = controls.get(controls::AnalogueGain);
> > - if (gain && !agc.autoEnabled) {
> > + if (gain && !agc.autoGainEnabled) {
> > agc.manual.gain = *gain;
> >
> > LOG(RkISP1Agc, Debug) << "Set gain to " << agc.manual.gain;
> > }
> >
> > - frameContext.agc.autoEnabled = agc.autoEnabled;
> > + frameContext.agc.autoExposureEnabled = agc.autoExposureEnabled;
> > + frameContext.agc.autoGainEnabled = agc.autoGainEnabled;
> >
> > - if (!frameContext.agc.autoEnabled) {
> > + if (!frameContext.agc.autoExposureEnabled)
> > frameContext.agc.exposure = agc.manual.exposure;
> > + if (!frameContext.agc.autoGainEnabled)
> > frameContext.agc.gain = agc.manual.gain;
> > - }
> >
> > const auto &meteringMode = controls.get(controls::AeMeteringMode);
> > if (meteringMode) {
> > @@ -283,10 +314,10 @@ void Agc::queueRequest(IPAContext &context,
> > void Agc::prepare(IPAContext &context, const uint32_t frame,
> > IPAFrameContext &frameContext, RkISP1Params *params)
> > {
> > - if (frameContext.agc.autoEnabled) {
> > + if (frameContext.agc.autoExposureEnabled)
> > frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> > + if (frameContext.agc.autoGainEnabled)
> > frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > - }
> >
> > if (frame > 0 && !frameContext.agc.updateMetering)
> > return;
> > @@ -333,7 +364,14 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > * frameContext.sensor.exposure;
> > metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> > metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> > - metadata.set(controls::AeEnable, frameContext.agc.autoEnabled);
> > + metadata.set(controls::ExposureTimeMode,
> > + frameContext.agc.autoExposureEnabled
> > + ? controls::ExposureTimeModeAuto
> > + : controls::ExposureTimeModeManual);
> > + metadata.set(controls::AnalogueGainMode,
> > + frameContext.agc.autoGainEnabled
> > + ? controls::AnalogueGainModeAuto
> > + : controls::AnalogueGainModeManual);
> >
> > /* \todo Use VBlank value calculated from each frame exposure. */
> > uint32_t vTotal = context.configuration.sensor.size.height
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 14d0c02a2b32..a8e8e1c0d06e 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -165,8 +165,11 @@ namespace libcamera::ipa::rkisp1 {
> > * \var IPAActiveState::agc.automatic.gain
> > * \brief Automatic analogue gain multiplier
> > *
> > - * \var IPAActiveState::agc.autoEnabled
> > - * \brief Manual/automatic AGC state as set by the AeEnable control
> > + * \var IPAActiveState::agc.autoExposureEnabled
> > + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> > + *
> > + * \var IPAActiveState::agc.autoGainEnabled
> > + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> > *
> > * \var IPAActiveState::agc.constraintMode
> > * \brief Constraint mode as set by the AeConstraintMode control
> > @@ -307,8 +310,11 @@ namespace libcamera::ipa::rkisp1 {
> > *
> > * The gain should be adapted to the sensor specific gain code before applying.
> > *
> > - * \var IPAFrameContext::agc.autoEnabled
> > - * \brief Manual/automatic AGC state as set by the AeEnable control
> > + * \var IPAFrameContext::agc.autoExposureEnabled
> > + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> > + *
> > + * \var IPAFrameContext::agc.autoGainEnabled
> > + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> > *
> > * \var IPAFrameContext::agc.constraintMode
> > * \brief Constraint mode as set by the AeConstraintMode control
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index e274d9b01e1c..7f819d31015e 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -75,7 +75,8 @@ struct IPAActiveState {
> > double gain;
> > } automatic;
> >
> > - bool autoEnabled;
> > + bool autoExposureEnabled;
> > + bool autoGainEnabled;
> > controls::AeConstraintModeEnum constraintMode;
> > controls::AeExposureModeEnum exposureMode;
> > controls::AeMeteringModeEnum meteringMode;
> > @@ -128,7 +129,8 @@ struct IPAFrameContext : public FrameContext {
> > struct {
> > uint32_t exposure;
> > double gain;
> > - bool autoEnabled;
> > + bool autoExposureEnabled;
> > + bool autoGainEnabled;
> > controls::AeConstraintModeEnum constraintMode;
> > controls::AeExposureModeEnum exposureMode;
> > controls::AeMeteringModeEnum meteringMode;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list