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

[Kieran Bingham]

Quoting Laurent Pinchart (2022-04-06 12:50:49)
> 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.

Argh, I wondered if this was an issue in git-pw ... but I've just
upgraded to git-pw 2.3.0 which is the latest so it's definitely still
something happening server side.

I'll investigate ...



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