[libcamera-devel,4/4] ipa: rkisp1: Raise maximum analogue gain
diff mbox series

Message ID 20230119155905.464995-5-mike.rudenko@gmail.com
State Accepted
Headers show
Series
  • Add Omnivision OV4689 support
Related show

Commit Message

Mikhail Rudenko Jan. 19, 2023, 3:59 p.m. UTC 
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@gmail.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jacopo Mondi Jan. 19, 2023, 5:14 p.m. UTC | #1 
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@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

>
>  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
>  static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> --
> 2.39.0
>
Jacopo Mondi Jan. 19, 2023, 5:26 p.m. UTC | #2 
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@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 ?


> >
> >  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> >  static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> > --
> > 2.39.0
> >
Mikhail Rudenko Jan. 20, 2023, 2:27 p.m. UTC | #3 
Hi Jacopo,

On 2023-01-19 at 18:26 +01, Jacopo Mondi <jacopo.mondi@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@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
>> >
Jacopo Mondi Jan. 20, 2023, 3:09 p.m. UTC | #4 
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@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@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
> >> >

Patch
diff mbox series 

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;
 
 /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
 static constexpr utils::Duration kMaxShutterSpeed = 60ms;