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

[Laurent Pinchart]

Hi Jean-Michel,

On Thu, Jun 01, 2023 at 07:32:08AM +0200, Jean-Michel Hautbois wrote:
> On 29/05/2023 14:44, Laurent Pinchart via libcamera-devel wrote:
> > 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.

Regardless of whether this is a hardware issue or a driver issue, it's
an issue. I don't think a hardcoded gain in the IPA module is the right
solution though, if gains above a certain value don't work, we can
either disallow that on the driver side, or have a sensor-specific
maximum in the camera sensor helpers.

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

Too high gains will cause lots of noise, but again that is
device-specific, and should thus be handled in the tuning data, no as a
hardcoded maximum gain in the IPA module. Tuning is performed based on
maximum acceptable noise levels (hello ISO-12232), so we could even have
different tuning files for the same platform for different use cases.

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

That's a good idea. Jacopo, could you try this ?

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

-- 
Regards,

Laurent Pinchart