| Message ID | 20230529123926.74775-1-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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;
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; >
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;
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;
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;
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(-)