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

[Jacopo Mondi]

Hello,

On Fri, May 19, 2023 at 04:51:09PM +0200, Benjamin Bara via libcamera-devel wrote:
> 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>

We had received a similar patch in the past but it probably got
dropped in between different versions of a series if I recall
correctly.

The limits are arbitraty, the IPU3 has them set to 1 and 8, who knows
why, so yeah, let's drop them, the cam helper should ensure this is
safe.

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

And I'll send a similar patch to remove the limits from ipu3 too.

Thanks
  j

> ---
>  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);
> -	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
>