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

[Naushir Patuck]

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?

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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220201/b7d171a2/attachment.htm>