[libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a Unicam dequeue timeout

[Laurent Pinchart]

Hi Naush,

On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote:
> On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote:
> > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via
> > libcamera-devel wrote:
> > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and
> > > connect the timeout signal to a slot in the pipeline handler. This slot will
> > > log a fatal error message informing the user of a possible hardware stall.
> > >
> > > The timeout is calculated as 2x the maximum frame length possible for a given
> > > mode, returned by the IPA.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.mojom           |  1 +
> > >  src/ipa/raspberrypi/raspberrypi.cpp               |  2 ++
> > >  .../pipeline/raspberrypi/raspberrypi.cpp          | 15 +++++++++++++++
> > >  3 files changed, 18 insertions(+)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > index acd3cafe6c91..5a228b75cd2f 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 maxSensorFrameLengthMs;
> >
> > We really need duration types in mojo.
> 
> Yes please!! :-)
> 
> > >  };
> > >
> > >  interface IPARPiInterface {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index fd8fecb07f81..675f9ba1b350 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
> > >       }
> > >
> > >       startConfig->dropFrameCount = dropFrameCount_;
> > > +     const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
> > > +     startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();
> > >
> > >       firstStart_ = false;
> > >       lastRunTimestamp_ = 0;
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index c2230199fed7..50b39f1adf93 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -45,6 +45,8 @@
> > >  #include "dma_heaps.h"
> > >  #include "rpi_stream.h"
> > >
> > > +using namespace std::literals::chrono_literals;
> > > +
> > >  namespace libcamera {
> > >
> > >  LOG_DEFINE_CATEGORY(RPI)
> > > @@ -202,6 +204,7 @@ public:
> > >       void setIspControls(const ControlList &controls);
> > >       void setDelayedControls(const ControlList &controls);
> > >       void setSensorControls(ControlList &controls);
> > > +     void unicamTimeout(V4L2VideoDevice *dev);
> > >
> > >       /* bufferComplete signal handlers. */
> > >       void unicamBufferDequeue(FrameBuffer *buffer);
> > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >               }
> > >       }
> > >
> > > +     /* Set the dequeue timeout to 2x the maximum possible frame duration. */
> > > +     utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms;
> > > +     data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);
> > > +     data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout);
> > > +     LOG(RPI, Debug) << "Setting sensor timeout to " << duration;
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
> > >       RPiCameraData *data = cameraData(camera);
> > >
> > >       data->state_ = RPiCameraData::State::Stopped;
> > > +     data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();
> >
> > No need to connect and disconnect the signal when starting and stopping,
> > you can connect it at init time and keep it connected forever.
> >
> > >       /* Disable SOF event generation. */
> > >       data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);
> > > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls)
> > >       sensor_->setControls(&controls);
> > >  }
> > >
> > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev)
> > > +{
> > > +     LOG(RPI, Fatal) << "Unicam has timed out!";
> >
> > That's harsh, and is likely to trigger if we ever miss a frame due to
> > high CPU load. If the goal is to detect a complete stall, I'd increase
> > the timeout to at least 10 frames.
> >
> 
> Yes, you are right.
> 
> I actually meant to use Error here - Fatal was a bit leftover from my testing
> to see the timeout hit as expected.

Maybe the error message in the V4L2VideoDevice class is enough then ?

> > How will this work with sensors that use an external trigger ?
> 
> Short answer is it won't and cannot right now.  Given this is a purely debug feature,
> the only effect it will have is to dump a log message to the console which
> I think is fine.

Won't the message be repeated at regular intervals though ? That may not
be very nice.

> In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we
> are in "bulb" mode and we must disable timeouts. Or perhaps the application can pass
> a config flag?

Or perhaps libcamera should handle the internal/external sync
configuration itself, I wouldn't be surprised if the IPA module could
benefit from more knowledge of how the sensor is triggered. We'll of
course need a trigger mode control in that case.

> > > +}
> > > +
> > >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > >  {
> > >       RPi::Stream *stream = nullptr;

-- 
Regards,

Laurent Pinchart