[libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better heruistics for calculating Unicam timeout

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.


On Mon, 6 Mar 2023 at 17:25, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via
> libcamera-devel wrote:
> >
> > The existing mechanism of setting a timeout value simply uses the
> > maximum possible frame length advertised by the sensor mode. This can be
> > problematic when, for example, the IMX477 sensor can use a frame length
> > of over 600 seconds. However, for typical usage the frame length will
> > never go over several 100s of milliseconds, making the timeout very
> > impractical.
> >
> > Store a list of the last 10 frame length values requested by the AGC. On
> > startup, and at the end of every frame, take the maximum frame length
> > value from this list and return that to the pipeline handler through the
> > setCameraTimeoutValue() signal. This allows the timeout value to better
> > track the actual sensor usage.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++++++++++++++++++++--
> >  1 file changed, 41 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f6826bf27fe1..7b5aed644a67 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -8,6 +8,7 @@
> >  #include <algorithm>
> >  #include <array>
> >  #include <cstring>
> > +#include <deque>
> >  #include <fcntl.h>
> >  #include <math.h>
> >  #include <stdint.h>
> > @@ -64,6 +65,9 @@ using utils::Duration;
> >  /* Number of metadata objects available in the context list. */
> >  constexpr unsigned int numMetadataContexts = 16;
> >
> > +/* Number of frame length times to hold in the queue. */
> > +constexpr unsigned int FrameLengthsQueueSize = 10;
> > +
> >  /* Configure the sensor with these values initially. */
> >  constexpr double defaultAnalogueGain = 1.0;
> >  constexpr Duration defaultExposureTime = 20.0ms;
> > @@ -121,7 +125,8 @@ class IPARPi : public IPARPiInterface
> >  public:
> >       IPARPi()
> >               : controller_(), frameCount_(0), checkCount_(0),
> mistrustCount_(0),
> > -               lastRunTimestamp_(0), lsTable_(nullptr),
> firstStart_(true)
> > +               lastRunTimestamp_(0), lsTable_(nullptr),
> firstStart_(true),
> > +               lastTimeout_(0s)
> >       {
> >       }
> >
> > @@ -155,6 +160,7 @@ private:
> >       void fillDeviceStatus(const ControlList &sensorControls, unsigned
> int ipaContext);
> >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats
> *stats) const;
> >       void processStats(unsigned int bufferId, unsigned int ipaContext);
> > +     void setCameraTimeoutValue();
> >       void applyFrameDurations(Duration minFrameDuration, Duration
> maxFrameDuration);
> >       void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> >       void applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls);
> > @@ -220,6 +226,10 @@ private:
> >
> >       /* Maximum gain code for the sensor. */
> >       uint32_t maxSensorGainCode_;
> > +
> > +     /* Track the frame length times over FrameLengthsQueueSize frames.
> */
> > +     std::deque<Duration> frameLengths_;
>
> Another note to self: create a generic ring buffer class.
>
> > +     Duration lastTimeout_;
> >  };
> >
> >  int IPARPi::init(const IPASettings &settings, bool lensPresent,
> IPAInitResult *result)
> > @@ -284,6 +294,10 @@ void IPARPi::start(const ControlList &controls,
> StartConfig *startConfig)
> >
> >       controller_.switchMode(mode_, &metadata);
> >
> > +     /* Reset the frame lengths queue */
>
> s/queue/queue./
>
> > +     frameLengths_.clear();
> > +     frameLengths_.resize(FrameLengthsQueueSize, 0s);
>
> Should you also reset lastTimeout_ to 0s to ensure a consistent
> behaviour at start time ?
>

Good catch, I will reset lastTimeout_ here.


>
> > +
> >       /* SwitchMode may supply updated exposure/gain values to use. */
> >       AgcStatus agcStatus;
> >       agcStatus.shutterTime = 0.0s;
> > @@ -294,6 +308,7 @@ void IPARPi::start(const ControlList &controls,
> StartConfig *startConfig)
> >               ControlList ctrls(sensorCtrls_);
> >               applyAGC(&agcStatus, ctrls);
> >               startConfig->controls = std::move(ctrls);
> > +             setCameraTimeoutValue();
> >       }
> >
> >       /*
> > @@ -340,8 +355,6 @@ void IPARPi::start(const ControlList &controls,
> StartConfig *startConfig)
> >       }
> >
> >       startConfig->dropFrameCount = dropFrameCount_;
> > -     const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.maxLineLength;
> > -     setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());
> >
> >       firstStart_ = false;
> >       lastRunTimestamp_ = 0;
> > @@ -1434,6 +1447,22 @@ void IPARPi::processStats(unsigned int bufferId,
> unsigned int ipaContext)
>
> Adding more context:
>
>         if (agcStatus.shutterTime && agcStatus.analogueGain) {
>                 ControlList ctrls(sensorCtrls_);
>
> >               applyAGC(&agcStatus, ctrls);
> >
> >               setDelayedControls.emit(ctrls, ipaContext);
> > +             setCameraTimeoutValue();
>
> If the above condition is false, setCameraTimeoutValue() won't be
> called. When can that happen, and could it cause any issue ?
>

Short answer is no.  Really the test is a bit redundant, agc can never ever
return with agcStatus.shutterTime or agcStatus.analogueGain set to 0.  I
ought
to remove that if () clause in a future tidy up.

Regards,
Naush


>
> > +     }
> > +}
> > +
> > +void IPARPi::setCameraTimeoutValue()
> > +{
> > +     /*
> > +      * Take the maximum value of the exposure queue as the camera
> timeout
> > +      * value to pass back to the pipeline handler. Only signal if it
> has changed
>
> Line wrap.
>
> > +      * from the last set value.
> > +      */
> > +     auto max = std::max_element(frameLengths_.begin(),
> frameLengths_.end());
> > +
> > +     if (*max != lastTimeout_) {
> > +             setCameraTimeout.emit(max->get<std::milli>());
> > +             lastTimeout_ = *max;
> >       }
> >  }
> >
> > @@ -1522,6 +1551,15 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> >        */
> >       if (mode_.minLineLength != mode_.maxLineLength)
> >               ctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));
> > +
> > +     /*
> > +      * Store the frame length times in a circular queue, up-to
> FrameLengthsQueueSize
> > +      * elements. This will be used to advertise a camera timeout value
> to the
> > +      * pipeline handler.
>
> Line wraps here too.
>
> > +      */
> > +     frameLengths_.pop_front();
> > +     frameLengths_.push_back(helper_->exposure(vblank + mode_.height,
> > +
>  helper_->hblankToLineLength(hblank)));
> >  }
> >
> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList
> &ctrls)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230307/f60c3ba3/attachment.htm>