[libcamera-devel] [PATCH 07/13] ipa: ipu3: agc: Change analogue gain limits

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-10-13 16:41:19)
> The gains is currently set as a uint32_t while the analogue gain is

'gains are currently' ?

> passed as a double. We also have a default maximum analogue gain of 15
> which is quite high for a number of sensors.
> 
> Use a maximum value of 8 which should really be configured by the IPA
> and not fixed as it is now. While at it make it a double.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 2dd5c5c1..e58a8a8d 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -30,14 +30,10 @@ static constexpr uint32_t kInitialFrameMinAECount = 4;
>  /* Number of frames to wait between new gain/exposure estimations */
>  static constexpr uint32_t kFrameSkipCount = 6;
>  
> -/* Maximum ISO value for analogue gain */
> -static constexpr uint32_t kMinISO = 100;
> -static constexpr uint32_t kMaxISO = 1500;
> -

Do the ISO values no longer make sense to be used here?

As this needs to come from a camera helper anyway, I guess it doesn't
hurt to reduce it as it should be changed later all the same.

Do you reduce it to prevent some overexposure issue? or because the
sensors you've tested only have x8 as a maximum?

Anyway, I don't object to this patch otherwise.


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

>  /* Maximum analogue gain value
>   * \todo grab it from a camera helper */
> -static constexpr uint32_t kMinGain = kMinISO / 100;
> -static constexpr uint32_t kMaxGain = kMaxISO / 100;
> +static constexpr double kMinGain = 1.0;
> +static constexpr double kMaxGain = 8.0;
>  
>  /* \todo use calculated value based on sensor */
>  static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us;
> @@ -161,9 +157,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>                 if (currentShutter < kMaxShutterSpeed) {
>                         exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_);
>                         newExposure = currentExposure_ / exposure;
> -                       gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> +                       gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
>                 } else if (currentShutter >= kMaxShutterSpeed) {
> -                       gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
> +                       gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
>                         newExposure = currentExposure_ / gain;
>                         exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_);
>                 }
> -- 
> 2.30.2
>