[PATCH 3/3] libipa: camera_sensor_helper: Fix rounding of gainCode

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Feb 23, 2024 at 03:59:54PM +0000, Kieran Bingham wrote:
> The implementation of gainCode for both Exponential and Linear gain

s/gainCode/gainCode()/

> models does not generate a gainCode that matches the result of the
> reverse operation.
> 
> This can be seen by converting sequential gainCodes to a gain
> and converting that back to the gainCode. The values do not
> translate back due to rounding errors.

It's not due to rounding errors, but to the fact that we only round
down, isn't it ?

> Correct the rounding error and ensure that gainCode translation
> produces accurate bi-directional conversions from the perspective
> of the gainCode.

Even rounding to the closest integer (which I think is a good idea), are
you sure you'll guarantee that gainCode(gain(code)) will always be equal
to code, in *all* cases ?

> This fixes the IMX290, IMX296, IMX327 and IMX335 which use the
> Exponential gain model helpers, as well as IMX219 IMX258 and IMX477

Missing commas.

> which use the Linear gain model helpers.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index a925b37b9f7c..39e10d86a2c7 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -64,13 +64,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
>  	case AnalogueGainLinear:
>  		ASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);
>  
> -		return (k.linear.c0 - k.linear.c1 * gain) /
> -		       (k.linear.m1 * gain - k.linear.m0);
> +		return std::round((k.linear.c0 - k.linear.c1 * gain) /
> +				  (k.linear.m1 * gain - k.linear.m0));
>  
>  	case AnalogueGainExponential:
>  		ASSERT(k.exp.a != 0 && k.exp.m != 0);
>  
> -		return std::log2(gain / k.exp.a) / k.exp.m;
> +		return std::round(std::log2(gain / k.exp.a) / k.exp.m);

The code change seems fine, but the commit message may need to be
reworked.

>  
>  	default:
>  		ASSERT(false);

-- 
Regards,

Laurent Pinchart