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

[Naushir Patuck]

Hi David,

Thank you for the review!

On Fri, 24 Feb 2023 at 11:14, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi Naush
>
> Thanks for the patch.
>
> On Thu, 23 Feb 2023 at 12:49, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> 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.
>
> I guess just one thing I'm slightly curious about, what happens at
> startup if nothing has run and then the camera decides not to deliver
> any frames? I guess we still set initial values in the frame length
> list based on what we configure before starting the camera, so that we
> can't find ourselves in the "wait umpteen minutes" situation (unless
> the user expressly requested that).

As part of the startup process, we will have the first frame length put into the
list, and this will be the timeout value used for the calculation.

But you do raise an interesting scenario where if an application configures the
camera for a RAW stream, sets the stream hint to say it will pass all buffers
(so no internal buffers are allocated), then decides not to pass in any requests
for a period of time, it could cause the timeout to trigger.  This is all very
implausible, and I suppose we give the application an override in patch 3 that
could be used to "fix" this behavior.

Regards,
Naush

>
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 33 +++++++++++++++++++++++++++--
> >  1 file changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f6826bf27fe1..b8fe0c002a13 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;
> > @@ -155,6 +159,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 +225,9 @@ private:
> >
> >         /* Maximum gain code for the sensor. */
> >         uint32_t maxSensorGainCode_;
> > +
> > +       /* Track the frame length times over FrameLengthsQueueSize frames. */
> > +       std::deque<Duration> frameLengths_;
> >  };
> >
> >  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> > @@ -294,6 +302,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> >                 ControlList ctrls(sensorCtrls_);
> >                 applyAGC(&agcStatus, ctrls);
> >                 startConfig->controls = std::move(ctrls);
> > +               setCameraTimeoutValue();
> >         }
> >
> >         /*
> > @@ -340,8 +349,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,9 +1441,20 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
> >                 applyAGC(&agcStatus, ctrls);
> >
> >                 setDelayedControls.emit(ctrls, ipaContext);
> > +               setCameraTimeoutValue();
> >         }
> >  }
> >
> > +void IPARPi::setCameraTimeoutValue()
> > +{
> > +       /*
> > +        * Take the maximum value of the exposure queue as the camera timeout
> > +        * value to pass back to the pipe line handler.
> > +        */
> > +       auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());
> > +       setCameraTimeout.emit(max->get<std::milli>());
> > +}
> > +
> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> >  {
> >         LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: "
> > @@ -1522,6 +1540,17 @@ 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.
> > +        */
> > +       if (frameLengths_.size() == FrameLengthsQueueSize)
> > +               frameLengths_.pop_front();
> > +
> > +       frameLengths_.push_back(helper_->exposure(vblank + mode_.height,
> > +                                                 helper_->hblankToLineLength(hblank)));
> >  }
> >
> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> > --
> > 2.25.1
> >