[libcamera-devel] [PATCH v1 1/2] ipa: raspberrypi: Limit the maximum sensor gain used

[David Plowman]

Hi everyone

On Tue, 1 Feb 2022 at 08:48, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi Kieran,
>
> Thanks for the review (this + all the other patches as well)!
>
> On Tue, 1 Feb 2022 at 00:00, Kieran Bingham <kieran.bingham at ideasonboard.com> wrote:
>>
>> Hi Naush,
>>
>> Quoting Naushir Patuck (2022-01-24 10:31:06)
>> > Limit the gain code to the maximum value reported by the sensor controls when
>> > sending to DelayedControls. The AGC algorithm will handle a lower gain used by
>> > the sensor, provided provided it knows the actual gain used. This change ensures
>> > that DelayedControls will never report an unclipped gain used.
>> >
>> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>> > ---
>> >  src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++
>> >  1 file changed, 12 insertions(+)
>> >
>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> > index 0ed41385018a..a72d516f84ee 100644
>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> > @@ -173,6 +173,9 @@ private:
>> >         /* Frame duration (1/fps) limits. */
>> >         Duration minFrameDuration_;
>> >         Duration maxFrameDuration_;
>> > +
>> > +       /* Maximum gain code for the sensor. */
>> > +       uint32_t maxSensorGainCode_;
>>
>> Any risk of needing to clamp the minimum value too?
>
>
> This is a good point.
> I made the assumption that a minimum gain of 1.0 is universal, but perhaps not...
> David, what do you think?  I recall you had played with devices where this may possible
> not be true?
>

It's a fair question, but I'm not inclined to clamp the minimum as
well. I think handling that would require extra code in the AGC (I
have other things to tackle right now...) or maybe editing exposure
profiles on a per-sensor basis (also undesirable,). Actually I have
one sensor which has a minimum gain of 1.12 for *some*, but not all,
readout modes, but I take care of all that in the cam helper. So
anyway, I'd rather fix the problem that we really have and leave this
question on the back burner for now. We could re-consider it if it
ever becomes a real thing.

David

> Regards,
> Naush
>
>>
>>
>> >  };
>> >
>> >  int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)
>> > @@ -357,6 +360,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>> >                 return -1;
>> >         }
>> >
>> > +       maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();
>> > +
>> >         /* Setup a metadata ControlList to output metadata. */
>> >         libcameraMetadata_ = ControlList(controls::controls);
>> >
>> > @@ -1113,6 +1118,13 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>> >  {
>> >         int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
>> >
>> > +       /*
>> > +        * Ensure anything larger than the max gain code will not be passed to
>> > +        * DelayedControls. The AGC will correctly handle a lower gain returned
>> > +        * by the sensor, provided it knows the actual gain used.
>> > +        */
>> > +       gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_);
>> > +
>>
>> Sounds ok to me though.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> >         /* GetVBlanking might clip exposure time to the fps limits. */
>> >         Duration exposure = agcStatus->shutter_time;
>> >         int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);
>> > --
>> > 2.25.1
>> >