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

[Kieran Bingham]

Quoting Laurent Pinchart (2024-02-27 14:58:47)
> 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 ?

Isn't that a rounding error? An error when rounding (to integer)? ... :-)

> > 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 ?

Run the test with and without the fix...

I haven't done a formal proof here no. But that's also why the test ...
tests every CameraSensorHelper in more or less brute force.

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

Ok.

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