[libcamera-devel] [PATCH] ipa: libipa: Fix ov5670 analogue gain parameters

[Jean-Michel Hautbois]

Hi Laurent,

On 08/07/2021 10:13, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Mon, Jul 05, 2021 at 06:01:20PM +0200, Jean-Michel Hautbois wrote:
>> Based on datasheet, the gain multiplier is 128 and not 256 (the
>> fraction bits are the 7 lower bits). Fix it.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> Change-Id: I87e4b078f65c68da005a3ad4f1216256b8487df7
> 
> You know about this already :-)

Indeed :-).

>> ---
>>  src/ipa/libipa/camera_sensor_helper.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>> index 23335ed6..cd0a19bb 100644
>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>> @@ -294,7 +294,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper
>>  public:
>>  	CameraSensorHelperOv5670()
>>  	{
>> -		analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };
>> +		analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };
> 
> Interestingly, the OV5670 supports two different gain models at the
> hardware level, with a register bit to select which model to use. Bit 2
> in register 0x3503 selects the "real gain format" (linear) when set to
> 0, and the "sensor gain format" (exponential) when set to 1. I really
> wonder why.
> 
> I've had a look at the driver, and quite interestingly, it selects the
> linear gain model in all cases except for the 2560x1440 mode, where the
> exponential gain model is used. Register 0x3503 is set to 0x04 towards
> the beginning of the mode-specific register arrays for all modes, and
> then set to 0x00 towards the end, except for 2560x1440 where the 0x00
> entry is missing.
> 
> https://lore.kernel.org/linux-media/YOay2tIJxpBzYKzW@pendragon.ideasonboard.com/

Nice catch !
I wonder if other OV* drivers may have the same issue...

> I believe that's a driver bug, and this patch looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>>  	}
>>  };
>>  REGISTER_CAMERA_SENSOR_HELPER("ov5670", CameraSensorHelperOv5670)
>