[libcamera-devel] [PATCH 1/2] libipa: Round gain code before returning the value

[Jean-Michel Hautbois]

On 19/11/2021 14:27, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> On Fri, Nov 19, 2021 at 02:22:23PM +0100, Jean-Michel Hautbois wrote:
>> On 19/11/2021 13:47, Laurent Pinchart wrote:
>>> On Fri, Nov 19, 2021 at 12:14:20PM +0100, Jean-Michel Hautbois wrote:
>>>> On 19/11/2021 12:11, Kieran Bingham wrote:
>>>>> Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)
>>>>>> We can have the case where the gain is very close to 1, as coded in
>>>>>> double, and after gainCode() returns, the value as an uint32_t is 0. In
>>>>>> order to avoid that, always round the value halfway cases away from zero
>>>>>> before returning.
>>>
>>> Why do we want to avoid that, why is rounding to the closest integer
>>> better than rounding down ?
>>>
>>>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>>>>
>>>>> This seems to make sense.
>>>>>
>>>>> Are there any issues with other values being rounded? I presume not.
>>>>> Is there any requirement to further protect or warn if gainCode is
>>>>> returned as zero? or can that still be valid in some situations?
>>>>
>>>> That is a tough question... Can we have a gainCode set to 0 in a linear
>>>> analogue gain controlled sensor... I am not sure of that at all...
>>>
>>> It may depends on the constaints of the linear model. For IMX219, which
>>> uses and inverse gain model (gain = 256 / (256 - gain code)), a gain
>>> code set to 0 is valid and produces a gain of 1.0.
>>
>> The minimum gainCode is 1, which produces a gain of 1.00392.
>> When this gain is applied the other way around, gainCode returns 0 and
>> not 1 as expected.
> 
> Why is the minimum gain code 1 ? The sensor seems to support 0, at least
> according to the datasheet.
> 

The driver sets it to 0...
The real issue being in configure():
minGain_ = std::max<uint32_t>(itGain->second.min().get<int32_t>(), 1);

I will remove this patch :-) and have another one instead, thanks !

>>>> BTW, investigating it (before introducing std::round) I found that using
>>>> float instead of double made it return 1...
>>>>
>>>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>>>
>>>>>> ---
>>>>>>     src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--
>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>>>>>> index 0b0eb503..67cf6913 100644
>>>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>>>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>>>>>> @@ -7,6 +7,8 @@
>>>>>>      */
>>>>>>     #include "camera_sensor_helper.h"
>>>>>>     
>>>>>> +#include <cmath>
>>>>>> +
>>>>>>     #include <libcamera/base/log.h>
>>>>>>     
>>>>>>     /**
>>>>>> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
>>>>>>            ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);
>>>>>>            ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
>>>>>>     
>>>>>> -       return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
>>>>>> -              (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);
>>>>>> +       return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
>>>>>> +                         (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));
>>>>>>     }
>>>>>>     
>>>>>>     /**
>