[PATCH 04/12] ipa: rkisp1: Document all AGC parameters in IPAActiveState

[Laurent Pinchart]

On Mon, Jun 17, 2024 at 10:08:37AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-16 17:39:02)
> > The IPAActiveState AGC documentation is lagging behind the
> > implementation and misses many variables. Document them.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/ipa_context.cpp | 40 +++++++++++++++++++++++++++++-----
> >  1 file changed, 35 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 65e0c58cb2ca..c4895479204c 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -137,13 +137,43 @@ namespace libcamera::ipa::rkisp1 {
> >   * \var IPAActiveState::agc
> >   * \brief State for the Automatic Gain Control algorithm
> >   *
> > - * The exposure and gain are the latest values computed by the AGC algorithm.
> > + * The \a automatic variables track the latest values computed by algorithm
> > + * based on the latest processed statistics. All other variables track the
> > + * consolidated controls requested in queued requests.
> 
> I wonder if we should make the state structures convey this somehow by
> separating the current request context vs the current process context
> ... but not now ...

Yes, I think we should. I've discussed that with Stefan before, and I
think we'll take that direction.

We have quite a bit of code in algorithms along the lines of

        const auto &exposureMode = controls.get(controls::AeExposureMode);
        if (exposureMode) {
                frameContext.agc.update = agc.exposureMode != *exposureMode;
                agc.exposureMode =
                        static_cast<controls::AeExposureModeEnum>(*exposureMode);
        }
        frameContext.agc.exposureMode = agc.exposureMode;

I wonder if we could reorganize structures in a way that would allow
automating some of this.

> >   *
> > - * \var IPAActiveState::agc.exposure
> > - * \brief Exposure time expressed as a number of lines
> > + * \struct IPAActiveState::agc.manual
> > + * \brief Manual exposure time and analog gain (set through requests)
> >   *
> > - * \var IPAActiveState::agc.gain
> > - * \brief Analogue gain multiplier
> > + * \var IPAActiveState::agc.manual.exposure
> > + * \brief Manual exposure time expressed as a number of lines as set by the
> > + * ExposureTime control
> > + *
> > + * \var IPAActiveState::agc.manual.gain
> > + * \brief Manual analogue gain as set by the AnalogueGain control
> > + *
> > + * \struct IPAActiveState::agc.automatic
> > + * \brief Automatic exposure time and analog gain (computed by the algorithm)
> > + *
> > + * \var IPAActiveState::agc.automatic.exposure
> > + * \brief Automatic exposure time expressed as a number of lines
> > + *
> > + * \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.constraintMode
> > + * \brief Constraint mode as set by the AeConstraintMode control
> > + *
> > + * \var IPAActiveState::agc.exposureMode
> > + * \brief Exposure mode as set by the AeExposureMode control
> > + *
> > + * \var IPAActiveState::agc.meteringMode
> > + * \brief Metering mode as set by the AeMeteringMode control
> > + *
> > + * \var IPAActiveState::agc.maxShutterSpeed
> > + * \brief Maximum frame duration as set by the FrameDurationLimits control
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >   */
> >  
> >  /**

-- 
Regards,

Laurent Pinchart