[libcamera-devel] [PATCH] ipa: rkisp1: agc: drop hard-coded analogue gain range

[Laurent Pinchart]

On Fri, May 19, 2023 at 04:17:40PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Benjamin Bara via libcamera-devel (2023-05-19 15:51:09)
> > From: Benjamin Bara <benjamin.bara at skidata.com>
> > 
> > As the sensor's analogue gain range is known (read-out in
> > IPARkISP1::configure()), drop the limiting hard-coded range.
> > This enables better performance in low-light conditions for sensors with
> > a higher gain (e.g. the imx327).
> > 
> > Signed-off-by: Benjamin Bara <benjamin.bara at skidata.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 19 +++----------------
> >  1 file changed, 3 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 22f70aba..a4e5500e 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -36,15 +36,6 @@ namespace ipa::rkisp1::algorithms {
> >  
> >  LOG_DEFINE_CATEGORY(RkISP1Agc)
> >  
> > -/*
> > - * Limits for analogue gain values
> > - *
> > - * \todo Remove the hard-coded limits and let the sensor helper specify
> > - * the minimum and maximum allowed gain values.
> > - */
> > -static constexpr double kMinAnalogueGain = 1.0;
> > -static constexpr double kMaxAnalogueGain = 16.0;
> > -
> >  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> >  static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> >  
> > @@ -80,9 +71,7 @@ Agc::Agc()
> >  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >  {
> >         /* Configure the default exposure and gain. */
> > -       context.activeState.agc.automatic.gain =
> > -               std::max(context.configuration.sensor.minAnalogueGain,
> > -                        kMinAnalogueGain);
> > +       context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;
> >         context.activeState.agc.automatic.exposure =
> >                 10ms / context.configuration.sensor.lineDuration;
> >         context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> > @@ -265,10 +254,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> >         utils::Duration maxShutterSpeed = std::min(configuration.sensor.maxShutterSpeed,
> >                                                    kMaxShutterSpeed);
> >  
> > -       double minAnalogueGain = std::max(configuration.sensor.minAnalogueGain,
> > -                                         kMinAnalogueGain);
> 
> This seems reasonable. Do we need to do anything to ensure we don't go
> below 1.0 here?
> 
> I guess that's the responsiblity of the CameraSensor class to validate
> though, or at least it should be checked during IPARkISP1::configure()
> ... so I think this is fine.

No, I think it should be handled in the IPA module. While I don't expect
many camera sensors to support gains smaller than 1.0, some may, and the
CameraSensor class should expose the real capabilities of the sensor.
The IPA module is responsible for setting the policies of the
algorithms.

I see that this patch has been merged as-is. Who will submit a fix to
restore the behaviour for the minimum gain ?

For the maximum gain, there's a policy decision to consider as well, but
that should come from the tuning data, not be hardcoded here. I'm thus
OK with the change for the maximum gain, but I think there's a risk it
will cause regressions for sensors that have a very high maximum gain
value.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > -       double maxAnalogueGain = std::min(configuration.sensor.maxAnalogueGain,
> > -                                         kMaxAnalogueGain);
> > +       double minAnalogueGain = configuration.sensor.minAnalogueGain;
> > +       double maxAnalogueGain = configuration.sensor.maxAnalogueGain;
> >  
> >         /* Consider within 1% of the target as correctly exposed. */
> >         if (utils::abs_diff(evGain, 1.0) < 0.01)

-- 
Regards,

Laurent Pinchart