[libcamera-devel] [PATCH 1/2] ipa: ipu3: agc: Drop hard-codec analogue gain max

[Jean-Michel Hautbois]

Hi guys,

On 29/05/2023 14:44, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, May 29, 2023 at 02:39:25PM +0200, Jacopo Mondi via libcamera-devel wrote:
>> As the sensor's analogue gain range is known, drop the arbitrary
>> maximum limit for the sensor analogue gain.
>>
>> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> I'm pretty sure that as soon as we'll merge this patch we'll get a
> regression report and will need to implement proper AGC tuning. Oh well
> :-)

This limit was, if I recall correctly, set to limit the gain as we could 
for instance have a driver (or sensor) setting a maximum of 16 while 
after 8 it is not behaving correctly (hello Omnivision). On the sgo2, 
the ov sensor is going full white for a too high gain (again, if I 
recall correctly). One could argue this is a driver issue, but the 
datasheet says it can do it.

The other reason was that multiplying by more than 8 is really a lot of 
amplification for such sensors. Maybe having the possibility to add a 
tuning value for this would make sense (ie, having a maximum default 
which can be overridden) ?

Wait and see then, if there is a regression, this should be seen quite 
easily (just put your device in a black room :-D).

JM

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index b5309bdbea25..606a237a4a59 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -47,9 +47,8 @@ namespace ipa::ipu3::algorithms {
>>   
>>   LOG_DEFINE_CATEGORY(IPU3Agc)
>>   
>> -/* Limits for analogue gain values */
>> +/* Minimum limit for analogue gain value */
>>   static constexpr double kMinAnalogueGain = 1.0;
>> -static constexpr double kMaxAnalogueGain = 8.0;
>>   
>>   /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
>>   static constexpr utils::Duration kMaxShutterSpeed = 60ms;
>> @@ -97,10 +96,10 @@ int Agc::configure(IPAContext &context,
>>   				    kMaxShutterSpeed);
>>   
>>   	minAnalogueGain_ = std::max(configuration.agc.minAnalogueGain, kMinAnalogueGain);
>> -	maxAnalogueGain_ = std::min(configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
>> +	maxAnalogueGain_ = configuration.agc.maxAnalogueGain;
>>   
>>   	/* Configure the default exposure and gain. */
>> -	activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
>> +	activeState.agc.gain = minAnalogueGain_;
>>   	activeState.agc.exposure = 10ms / configuration.sensor.lineDuration;
>>   
>>   	frameCount_ = 0;
>