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

20332 diff mbox series

The IPAActiveState AGC documentation is lagging behind the
implementation and misses many variables. Document them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/rkisp1/ipa_context.cpp | 40 +++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

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@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 ...

>   *
> - * \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@ideasonboard.com>

>   */
>  
>  /**
> -- 
> Regards,
> 
> 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@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@ideasonboard.com>
> 
> >   */
> >  
> >  /**

On Sun, Jun 16, 2024 at 07:39:02PM +0300, Laurent Pinchart wrote:
> The IPAActiveState AGC documentation is lagging behind the
> implementation and misses many variables. Document them.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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.
>   *
> - * \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
>   */
>  
>  /**