[PATCH v2 2/2] libipa: camera_sensor_helper: Fix rounding of gainCode

[David Plowman]

On Fri, 24 May 2024 at 15:32, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi Kieran,
>
> On Fri, 24 May 2024, 2:53 pm Kieran Bingham, <kieran.bingham at ideasonboard.com> wrote:
>>
>> The implementation of gainCode for both Exponential and Linear gain
>> 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 only rounding down, rather than to the closest
>> integer.
>>
>> Correct the rounding error and ensure that gainCode translation
>> produces accurate bi-directional conversions from the perspective
>> of the gainCode.
>
>
> Just to cause a bit of controversy, some IPAs might prefer to round down the gain code. This would allow a stable total exposure by compensating with digital gain. If we round up, our total exposure will be higher than what was requested, which is harder to compensate for.
>
> However the visual differences will be extremely marginal.

Good spot, Naush. Indeed, I would quite deeply dislike anything that
gives me more gain than I asked for! Less is ok, because you can
compensate somewhere else. I know we aren't using any of these
helpers, but no sense in doing anything that would make adopting them
harder...

Maybe it's a case of fixing the test?

David

>
> Naush
>
>
>
>>
>> This fixes the IMX290, IMX296, IMX327 and IMX335 which use the
>> Exponential gain model helpers, as well as IMX219, IMX258, IMX283, and
>> IMX477 which use the Linear gain model helpers.
>>
>>
>> This updates the corresponding test which was previously marked as
>> expected to fail.
>>
>> Fixes: 8ad9249fb09d ("libipa: camera_sensor_helper: Implement exponential gain model")
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/ipa/libipa/camera_sensor_helper.cpp | 6 +++---
>>  test/ipa/meson.build                    | 3 +--
>>  2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>> index 2cd61fccfbb9..4a92ead88b23 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);
>>
>>         default:
>>                 ASSERT(false);
>> diff --git a/test/ipa/meson.build b/test/ipa/meson.build
>> index abc2456fc341..0b22c7576e56 100644
>> --- a/test/ipa/meson.build
>> +++ b/test/ipa/meson.build
>> @@ -1,8 +1,7 @@
>>  # SPDX-License-Identifier: CC0-1.0
>>
>>  ipa_test = [
>> -    {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp'],
>> -     'should_fail': true},
>> +    {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},
>>      {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},
>>      {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},
>>  ]
>> --
>> 2.34.1
>>