ipa: simple: Fix againMinStep calculation

26819 diff mbox series

Use the difference between againMin + 1 and againMin as the againMinStep
value. This is better than a heuristic of 1% of the whole range, which
doesn't match the actual allowed values by the V4L2 control, especially
for sensors with an exponential gain model (that map the control values
to equal-sized dB increments).

Without this change I saw a lot of oscillations with IMX678. While the
oscillations did go away after I narrowed the gain control to only map
to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain
range, I still think we should use the minimum step allowed by V4L2
rather than a heuristic of 1%.

Even for sensors with a linear gain model like IMX219, that allow
setting more than 100 values, this should make the AGC algorithm
smoother.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 src/ipa/simple/soft_simple.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Hi Jai,


Quoting Jai Luthra (2026-06-03 15:08:18)
> Use the difference between againMin + 1 and againMin as the againMinStep
> value. This is better than a heuristic of 1% of the whole range, which
> doesn't match the actual allowed values by the V4L2 control, especially
> for sensors with an exponential gain model (that map the control values
> to equal-sized dB increments).
> 
> Without this change I saw a lot of oscillations with IMX678. While the
> oscillations did go away after I narrowed the gain control to only map
> to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain
> range, I still think we should use the minimum step allowed by V4L2
> rather than a heuristic of 1%.

This sounds good, but there's some existing work on the list that we
should also consider here:

Can you take a look at
https://patchwork.libcamera.org/project/libcamera/list/?series=5915 and
in particular https://patchwork.libcamera.org/patch/26662/ please ?

I also imagine lots of this changing as part of our AGC rework for
libipa - so if there's a quick win here already I'm fine merging it -
but I think there could be bigger changes imminent too.

--
Kieran


> 
> Even for sensors with a linear gain model like IMX219, that allow
> setting more than 100 values, this should make the AGC algorithm
> smoother.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  src/ipa/simple/soft_simple.cpp | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 629e1a32d..e463e38af 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                 context_.configuration.agc.againMin = camHelper_->gain(againMin);
>                 context_.configuration.agc.againMax = camHelper_->gain(againMax);
>                 context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0);
> +               double againNext = camHelper_->gain(std::min(againMin + 1, againMax));
>                 context_.configuration.agc.againMinStep =
> -                       (context_.configuration.agc.againMax -
> -                        context_.configuration.agc.againMin) /
> -                       100.0;
> +                       againNext - context_.configuration.agc.againMin;
>                 if (camHelper_->blackLevel().has_value()) {
>                         /*
>                          * The black level from camHelper_ is a 16 bit value, software ISP
> -- 
> 2.54.0
>