[libcamera-devel] [PATCH 4/4] ipa: rkisp1: Raise maximum analogue gain

[Jacopo Mondi]

Hi Mikhail

On Fri, Jan 20, 2023 at 05:27:52PM +0300, Mikhail Rudenko via libcamera-devel wrote:
>
> Hi Jacopo,
>
> On 2023-01-19 at 18:26 +01, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
>
> > Hello again
> >
> > On Thu, Jan 19, 2023 at 06:14:31PM +0100, Jacopo Mondi via libcamera-devel wrote:
> >> Hi Mikhail
> >>
> >> On Thu, Jan 19, 2023 at 06:59:05PM +0300, Mikhail Rudenko via libcamera-devel wrote:
> >> > Omnivision OV4689 sensor driver exposes maximum analogue gain of
> >> > 16x. Raise kMaxAnalogueGain to 16.0, so that the full gain range can
> >> > be used.
> >> >
> >> > Signed-off-by: Mikhail Rudenko <mike.rudenko at gmail.com>
> >> > ---
> >> >  src/ipa/rkisp1/algorithms/agc.cpp | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> >> > index e3470e25..e4cb2fc7 100644
> >> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> >> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> >> > @@ -38,7 +38,7 @@ LOG_DEFINE_CATEGORY(RkISP1Agc)
> >> >
> >> >  /* Limits for analogue gain values */
> >> >  static constexpr double kMinAnalogueGain = 1.0;
> >> > -static constexpr double kMaxAnalogueGain = 8.0;
> >> > +static constexpr double kMaxAnalogueGain = 16.0;
> >>
> >> I'm very surprised we have such an hard limit..
> >>
> >> Should it be made configurable ? Should we allow the sensor tuning
> >> file to provide this value ? Not something required for you to do to
> >> fix this ofc
> >>
> >
> > In facts, this is already overriden using the sensor's provided max
> > gain as returned from the CameraHelper
> >
> > src/ipa/rkisp1/rkisp1.cpp:      context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);
> >
> > But we limit it to 8.0
> >
> > src/ipa/rkisp1/algorithms/agc.cpp:      double maxAnalogueGain = std::min(configuration.sensor.maxAnalogueGain,
> > src/ipa/rkisp1/algorithms/agc.cpp-                                        kMaxAnalogueGain);
> >
> > Should the camera sensor/helper be let express their max gain as they
> > like ?
>
> Do I understand correctly that you suggest dropping 4/4 as it is and
> removing kMaxAnalogueGain and analogue gain limiting in
> src/ipa/rkisp1/algorithms/agc.cpp instead?

Looking at the history, that values is there since the very beginning,
which makes me wonder if it's just a leftover from early developments.

We should let the sensor express it's maximum gain value in my
opinion (and give it a way to tune it from configuration file
eventually, but not for this patch).

Same for kMinAnalogueValue.

True that

        static constexpr double kMinAnalogueGain = 1.0;
        static constexpr double kMaxAnalogueGain = 8.0;

	double minAnalogueGain = std::max(configuration.sensor.minAnalogueGain,
					  kMinAnalogueGain);
	double maxAnalogueGain = std::min(configuration.sensor.maxAnalogueGain,
					  kMaxAnalogueGain);

	double stepGain = std::clamp(exposureValue / shutterTime,
				     minAnalogueGain, maxAnalogueGain);

Guarantees that the CameraSensorHelper receives  gain guaranteed to be
within a know interval. If we remove such clamp we would possibly feed
to CameraSensorHelper values not previously tested (and which only
depend on the sensor driver implementation).

I'm looking at
        uint32_t CameraSensorHelper::gainCode(double gain) const

and I'm trying to figure out if it's safe or we need a safety clamp
mechanism

>
> Best regards,
> Mikhail
>
> >
> >> >
> >> >  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> >> >  static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> >> > --
> >> > 2.39.0
> >> >