[PATCH v1 1/5] ipa: rkisp1: awb: Clamp gains to machine limits

[Kieran Bingham]

Quoting Stefan Klug (2024-07-12 15:32:02)
> When the color gains where set manually it was possible to specify a
> gain that wrapped the hardware limits. It would also be possible to
> further tune the floating point limits, but that is an error prone
> approach. So the limits are imposed on the integers, just before writing
> to the hardware. This noticeably reduces some oscillations in the awb
> regulation.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index a01fe5d90973..1a5d4776970a 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -120,10 +120,14 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,
>                 frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue;
>         }
>  
> -       params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green;
> -       params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue;
> -       params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red;
> -       params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green;
> +       params->others.awb_gain_config.gain_green_b =

Are these parameters clamped/verified by the kernel ? Is that where the
oscillations come from (if the kernel applies different values than we
track/believe/expect?)

Should we be clamping frameContext.awb.gains.green so we track what
value actually is applied in the frame context?

> +               std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff);
> +       params->others.awb_gain_config.gain_blue =
> +               std::clamp<int>(256 * frameContext.awb.gains.blue, 0, 0x3ff);
> +       params->others.awb_gain_config.gain_red =
> +               std::clamp<int>(256 * frameContext.awb.gains.red, 0, 0x3ff);
> +       params->others.awb_gain_config.gain_green_r =
> +               std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff);
>  

Though - I don't object to ensuring we clamp before setting in the param
buffer - but it scares me thinking there are a lot more values we need
to perhaps validate before storing in the parameter buffers?


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

>         /* Update the gains. */
>         params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN;
> -- 
> 2.43.0
>