[libcamera-devel] [PATCH] pipeline: raspberrypi: Add a sensor dequeue timeout

[Laurent Pinchart]

Hi Naush,

On Mon, Mar 21, 2022 at 01:22:43PM +0000, Naushir Patuck via libcamera-devel wrote:
> On Mon, 21 Mar 2022 at 13:13, Kieran Bingham wrote:
> > Quoting Naushir Patuck via libcamera-devel (2022-03-21 12:48:28)
> > > Add a timer to the pipeline handler that get set on start(), and subsequently
> > > reset on each frame dequeue from the sensor. If the timeout expires, log an
> > > error message indicating so. This is useful for debugging sensor failures were
> > > the device just stops streaming frames to Unicam, or the pipeline handler has
> > > failed to queue buffers to the Unicam device driver.
> > >
> > > The timeout is calculated as 2x the maximum frame length possible for a given
> > > mode.
> >
> > This is an interesting idea, and I think will prove useful for
> > identifying stalls in streams.
> >
> > Would it make sense to make this a feature of the V4L2VideoDevice? Then
> > it would (automatically?) cover ISP hangs as well as sensor hangs...
> 
> It would be nice to add this to a level above, and allow all pipeline handlers
> to gain this functionality!  I can look to do that if we want to go this way.

That would be nice indeed. Let's be careful not to generate false
positives though. I suppose the mechanism would need to be enabled by
the V4L2VideoDevice user by setting a timeout, so the risk shouldn't be
too high.

> > Perhaps the hard part is in making sure the correct timeout is
> > determined or set?
> 
> Well, I've already done it for the sensor in this patch :-) Adding it for the
> ISP is a bit simpler, as it is (normally) not mode dependent.
> 
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.mojom       |  1 +
> > >  src/ipa/raspberrypi/raspberrypi.cpp           |  7 ++++++
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 22 +++++++++++++++++++
> > >  3 files changed, 30 insertions(+)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > index acd3cafe6c91..33d2a97ca916 100644
> > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > @@ -41,6 +41,7 @@ struct IPAConfig {
> > >  struct StartConfig {
> > >         libcamera.ControlList controls;
> > >         int32 dropFrameCount;
> > > +       uint32 sensorTimeoutMs;
> > >  };
> > >
> > >  interface IPARPiInterface {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index fd8fecb07f81..983d6e998b4c 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -281,6 +281,13 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
> > >
> > >         startConfig->dropFrameCount = dropFrameCount_;
> > >
> > > +       /*
> > > +        * Set the pipeline handler's sensor timeout to 2x the maximum possible
> > > +        * frame duration for this mode.
> > > +        */
> > > +       const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
> > > +       startConfig->sensorTimeoutMs = 2 * maxSensorFrameDuration.get<std::milli>();

I don't like having the IPA instruct the pipeline handler. It may be
considered as nitpicking, but I'd rather expose data from the API, (such
as the frame duration for instance), and let the pipeline handler
determine the timeout. Doesn't the pipeline handler already have access
to the frame duration, as it's configurable by applications through
libcamera controls ?

> > > +
> > >         firstStart_ = false;
> > >         lastRunTimestamp_ = 0;
> > >  }
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index c2230199fed7..86d952b52aed 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -15,6 +15,7 @@
> > >  #include <utility>
> > >
> > >  #include <libcamera/base/shared_fd.h>
> > > +#include <libcamera/base/timer.h>
> > >  #include <libcamera/base/utils.h>
> > >
> > >  #include <libcamera/camera.h>
> > > @@ -202,6 +203,7 @@ public:
> > >         void setIspControls(const ControlList &controls);
> > >         void setDelayedControls(const ControlList &controls);
> > >         void setSensorControls(ControlList &controls);
> > > +       void sensorTimeout();
> > >
> > >         /* bufferComplete signal handlers. */
> > >         void unicamBufferDequeue(FrameBuffer *buffer);
> > > @@ -279,6 +281,10 @@ public:
> > >          */
> > >         std::optional<int32_t> notifyGainsUnity_;
> > >
> > > +       /* Timer to ensure the sensor is producing frames for the pipeline handler. */
> > > +       Timer sensorTimeout_;
> > > +       std::chrono::milliseconds sensorTimeoutDuration_;
> > > +
> > >  private:
> > >         void checkRequestCompleted();
> > >         void fillRequestMetadata(const ControlList &bufferControls,
> > > @@ -1032,6 +1038,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >                 }
> > >         }
> > >
> > > +       LOG(RPI, Debug) << "Setting sensor timeout to " << startConfig.sensorTimeoutMs << " ms";
> > > +       data->sensorTimeoutDuration_ = std::chrono::milliseconds(startConfig.sensorTimeoutMs);
> > > +       data->sensorTimeout_.start(data->sensorTimeoutDuration_);
> > > +       data->sensorTimeout_.timeout.connect(data, &RPiCameraData::sensorTimeout);
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -1040,6 +1051,8 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
> > >         RPiCameraData *data = cameraData(camera);
> > >
> > >         data->state_ = RPiCameraData::State::Stopped;
> > > +       data->sensorTimeout_.timeout.disconnect();

You don't have to connect/disconnect the signal when starting and
stopping the camera, you can connect it when creating the RPiCameraData
instance and keep it connected.

> > > +       data->sensorTimeout_.stop();
> > >
> > >         /* Disable SOF event generation. */
> > >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);
> > > @@ -1757,6 +1770,12 @@ void RPiCameraData::setSensorControls(ControlList &controls)
> > >         sensor_->setControls(&controls);
> > >  }
> > >
> > > +void RPiCameraData::sensorTimeout()
> > > +{
> > > +       LOG(RPI, Error) << "Sensor has timed out after "
> > > +                       << sensorTimeoutDuration_.count() << " ms!";
> > > +}
> > > +
> > >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > >  {
> > >         RPi::Stream *stream = nullptr;
> > > @@ -1792,6 +1811,9 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > >                  */
> > >                 ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp);
> > >                 bayerQueue_.push({ buffer, std::move(ctrl) });
> > > +
> > > +               /* Restart the sensor timer. */
> > > +               sensorTimeout_.start(sensorTimeoutDuration_);
> > >         } else {
> > >                 embeddedQueue_.push(buffer);
> > >         }

-- 
Regards,

Laurent Pinchart