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

18639 diff mbox series

From: Benjamin Bara <benjamin.bara@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@skidata.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

Quoting Benjamin Bara via libcamera-devel (2023-05-19 15:51:09)
> From: Benjamin Bara <benjamin.bara@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@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.

Reviewed-by: Kieran Bingham <kieran.bingham@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)
> -- 
> 2.34.1
>

Hello,

On Fri, May 19, 2023 at 04:51:09PM +0200, Benjamin Bara via libcamera-devel wrote:
> From: Benjamin Bara <benjamin.bara@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@skidata.com>

We had received a similar patch in the past but it probably got
dropped in between different versions of a series if I recall
correctly.

The limits are arbitraty, the IPU3 has them set to 1 and 8, who knows
why, so yeah, let's drop them, the cam helper should ensure this is
safe.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

And I'll send a similar patch to remove the limits from ipu3 too.

Thanks
  j

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

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