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

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> 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.


>
> 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.

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?

Regards,
Naush


>
> > +}
> > +
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >       RPi::Stream *stream = nullptr;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220323/5c135eb3/attachment.htm>