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

[Laurent Pinchart]

On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28)
> > 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.
> 
> No longer a fatal error message.
> 
> > The timeout is calculated as 2x the maximum frame length possible for a given
> > mode, returned by the IPA.

I mentioned in the review of v1 that we should probably increase the
timeout (to 10 times the frame duration). That was based on the error
message being fatal though, but maybe it's still good to avoid a short
watchdog timeout ?

> > 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;
> >  };
> >  
> >  interface IPARPiInterface {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 1bf4e270b062..89767a9db471 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 8fd79be67988..93931aaf3b55 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -208,6 +208,7 @@ public:
> >         void setIspControls(const ControlList &controls);
> >         void setDelayedControls(const ControlList &controls);
> >         void setSensorControls(ControlList &controls);
> > +       void unicamTimeout();
> >  
> >         /* bufferComplete signal handlers. */
> >         void unicamBufferDequeue(FrameBuffer *buffer);
> > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> >                 }
> >         }
> >  
> > +       /* Set the dequeue timeout to 2x the maximum possible frame duration. */
> > +       std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2);

s/msec/timeout/

> > +       data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec);
> > +       LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " ms.";

I may have dropped this message, I'm not sure it helps much.

> > +
> >         return 0;
> >  }
> >  
> > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> >         data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
> >  
> >         /* Wire up all the buffer connections. */
> > +       data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);
> >         data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
> >         data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);
> >         data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);
> > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls)
> >         sensor_->setControls(&controls);
> >  }
> >  
> > +void RPiCameraData::unicamTimeout()
> > +{
> > +       LOG(RPI, Error)
> > +               << "Unicam has timed out!\n"
> > +                  "Please check that your camera sensor connector is attached securely.\n"
> 
> The '\n' should probably be std::endl ... but I don't think that makes a
> big difference here?

It would still be good, for consistency.

> Both of those could be fixed while applying, there's nothing complex
> there.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > +                  "Alternatively, try another cable and/or sensor.";
> > +}
> > +
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >         RPi::Stream *stream = nullptr;

-- 
Regards,

Laurent Pinchart