[libcamera-devel,1/2] ipa: ipu3: agc: Drop hard-codec analogue gain max
diff mbox series

Message ID 20230529123926.74775-1-jacopo.mondi@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,1/2] ipa: ipu3: agc: Drop hard-codec analogue gain max
Related show

Commit Message

Jacopo Mondi May 29, 2023, 12:39 p.m. UTC
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@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart May 29, 2023, 12:44 p.m. UTC | #1
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@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
:-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
Jean-Michel Hautbois June 1, 2023, 5:32 a.m. UTC | #2
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@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@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;
>
Laurent Pinchart June 1, 2023, 7:26 a.m. UTC | #3
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@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@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;
Dan Scally June 6, 2023, 2:54 p.m. UTC | #4
Hello

On 29/05/2023 13:39, 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@ideasonboard.com>


I tested this on my Surface Go 2 and couldn't see any regressions from it. With the typo in the 
subject line fixed:


Reviewed-and-tested-by: Daniel Scally <dan.scally@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;

Patch
diff mbox series

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;