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

[Laurent Pinchart]

Hi Naush,

On Wed, Apr 06, 2022 at 12:35:58PM +0100, Naushir Patuck wrote:
> On Wed, 6 Apr 2022 at 12:25, Laurent Pinchart wrote:
> > On Wed, Apr 06, 2022 at 08:58:58AM +0100, Naushir Patuck wrote:
> > > From: Naushir Patuck via libcamera-devel <libcamera-devel at lists.libcamera.org>
> >
> > Here too.
> 
> I don't really know what went wrong with this. I use my normal git-pw workflow to pull
> the tagged patches from patchworks onto my tree, and normally it does the right thing.

Kieran, it looks like patchwork correctly records the author in the
database, but the contents of the patches themselves still have the
wrong From: header.

> Oh well.... I'll fix it up.
> 
> > > 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 an 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>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.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..98c463e39548 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::chrono_literals;
> > > +
> > >  namespace libcamera {
> > >
> > >  LOG_DEFINE_CATEGORY(RPI)
> > > @@ -208,6 +210,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 +1053,10 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >               }
> > >       }
> > >
> > > +     /* Set the dequeue timeout to 2x the maximum possible frame duration. */
> > > +     utils::duration timeout(2 * startConfig.maxSensorFrameLengthMs * 1ms);
> >
> > This is a utils::duration, but setDequeueTimeout takes a
> > utils::Duration (what have we done... ?). It will compile and I think do
> > the right thing thanks to implicit conversion, but it would be better to
> > use utils::Duration.
> 
> Ouch good catch!
> 
> > I could fix this locally too, but I'd feel better if you could test it,
> > would you mind sending a v5 that would also take the comments on 1/3
> > into account ?
> 
> No problem.  I'll fix this and test before pushing another version.
> 
> > > +     data->unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -1192,6 +1199,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 +1781,13 @@ void RPiCameraData::setSensorControls(ControlList &controls)
> > >       sensor_->setControls(&controls);
> > >  }
> > >
> > > +void RPiCameraData::unicamTimeout()
> > > +{
> > > +     LOG(RPI, Error) << "Unicam has timed out!";
> > > +     LOG(RPI, Error) << "Please check that your camera sensor connector is attached securely.";
> > > +     LOG(RPI, Error) << "Alternatively, try another cable and/or sensor.";
> > > +}
> > > +
> > >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > >  {
> > >       RPi::Stream *stream = nullptr;

-- 
Regards,

Laurent Pinchart