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

[Mikhail Rudenko]

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?

Best regards,
Mikhail

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