[libcamera-devel] [PATCH] ipa: rkisp1: agc: drop hard-coded analogue gain range

[Kieran Bingham]

Quoting Benjamin Bara via libcamera-devel (2023-05-19 15:51:09)
> From: Benjamin Bara <benjamin.bara at skidata.com>
> 
> As the sensor's analogue gain range is known (read-out in
> IPARkISP1::configure()), drop the limiting hard-coded range.
> This enables better performance in low-light conditions for sensors with
> a higher gain (e.g. the imx327).
> 
> Signed-off-by: Benjamin Bara <benjamin.bara at skidata.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 22f70aba..a4e5500e 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -36,15 +36,6 @@ namespace ipa::rkisp1::algorithms {
>  
>  LOG_DEFINE_CATEGORY(RkISP1Agc)
>  
> -/*
> - * Limits for analogue gain values
> - *
> - * \todo Remove the hard-coded limits and let the sensor helper specify
> - * the minimum and maximum allowed gain values.
> - */
> -static constexpr double kMinAnalogueGain = 1.0;
> -static constexpr double kMaxAnalogueGain = 16.0;
> -
>  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
>  static constexpr utils::Duration kMaxShutterSpeed = 60ms;
>  
> @@ -80,9 +71,7 @@ Agc::Agc()
>  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  {
>         /* Configure the default exposure and gain. */
> -       context.activeState.agc.automatic.gain =
> -               std::max(context.configuration.sensor.minAnalogueGain,
> -                        kMinAnalogueGain);
> +       context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;
>         context.activeState.agc.automatic.exposure =
>                 10ms / context.configuration.sensor.lineDuration;
>         context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> @@ -265,10 +254,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>         utils::Duration maxShutterSpeed = std::min(configuration.sensor.maxShutterSpeed,
>                                                    kMaxShutterSpeed);
>  
> -       double minAnalogueGain = std::max(configuration.sensor.minAnalogueGain,
> -                                         kMinAnalogueGain);

This seems reasonable. Do we need to do anything to ensure we don't go
below 1.0 here?

I guess that's the responsiblity of the CameraSensor class to validate
though, or at least it should be checked during IPARkISP1::configure()
... so I think this is fine.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> -       double maxAnalogueGain = std::min(configuration.sensor.maxAnalogueGain,
> -                                         kMaxAnalogueGain);
> +       double minAnalogueGain = configuration.sensor.minAnalogueGain;
> +       double maxAnalogueGain = configuration.sensor.maxAnalogueGain;
>  
>         /* Consider within 1% of the target as correctly exposed. */
>         if (utils::abs_diff(evGain, 1.0) < 0.01)
> -- 
> 2.34.1
>