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

[Laurent Pinchart]

Hi Naush,

On Wed, Apr 06, 2022 at 08:12:26AM +0100, Naushir Patuck wrote:
> On Tue, 5 Apr 2022 at 16:34, Laurent Pinchart wrote:
> > 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 ?
> 
> I did wonder what to do here for the timeout limit.  Having 10x frame duration is going to
> be very long for imx477 that has a 200s frame length.  I think given this is only a log message
> I'm inclined to leave it short to catch the obvious failure case. If I get complaints of too many
> false positives we can bump this up a bit :-)

Ah right it's the maximum frame duration, not the current frame
duration. Well, in that case I wonder how useful it will be with the
IMX477, with a 400s timeout :-) That's fine, there's no need to worry
about it.

On the other hand, for sensors that have a small maximum frame duration,
would it make sense to set the timeout to, for instance, std::max(1s,
max frame duration * 2) ?

> I'll fix up all the suggested changes and post a new revision shortly.
> 
> > > > 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