[libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice: Add a dequeue timer

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:11:48)
> Hi Laurent,
> 
> Thank you for your feedback.
> 
> On Tue, 22 Mar 2022 at 21:47, Laurent Pinchart <
> laurent.pinchart at ideasonboard.com> wrote:
> 
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Tue, Mar 22, 2022 at 01:16:34PM +0000, Naushir Patuck via
> > libcamera-devel wrote:
> > > Add a timer that gets reset on every buffer dequeue event. If the timeout
> > > expires, optionally call a slot in the pipeline handler to handle this
> > > condition. This may be useful in detecting and handling stalls in either
> > the
> > > hardware or device driver.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/v4l2_videodevice.h |  9 ++++
> > >  src/libcamera/v4l2_videodevice.cpp            | 47 +++++++++++++++++++
> > >  2 files changed, 56 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h
> > b/include/libcamera/internal/v4l2_videodevice.h
> > > index 2d2ccc477c91..dd6e96438637 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -20,6 +20,7 @@
> > >  #include <libcamera/base/class.h>
> > >  #include <libcamera/base/log.h>
> > >  #include <libcamera/base/signal.h>
> > > +#include <libcamera/base/timer.h>
> > >  #include <libcamera/base/unique_fd.h>
> > >
> > >  #include <libcamera/color_space.h>
> > > @@ -216,6 +217,9 @@ public:
> > >       int streamOn();
> > >       int streamOff();
> > >
> > > +     void setDequeueTimeout(utils::Duration duration);
> > > +     Signal<V4L2VideoDevice *> dequeueTimeout;
> >
> > We stopped passing the pointer to the emitter object when emitting a
> > signal, so this should be just Signal<> dequeueTimeout;
> >
> 
> The intention here was to pass the V4L2VideoDevice instance to the
> pipeline handler's slot callback.  This way, the pipeline handler can have
> a single slot for all devices it wants to monitor, and can distinguish which
> device timed out.  If you think that may not be necessary, I will remove it.
> 

For this use case, being able to report which device has stalled could
be useful, though I imagine it will already be done by the
V4L2VideoDevice itself before emitting this signal so it will already be
identifiable?


> >
> > > +
> > >       static std::unique_ptr<V4L2VideoDevice>
> > >       fromEntityName(const MediaDevice *media, const std::string
> > &entity);
> > >
> > > @@ -246,6 +250,8 @@ private:
> > >       void bufferAvailable();
> > >       FrameBuffer *dequeueBuffer();
> > >
> > > +     void timerExpired();
> > > +
> > >       V4L2Capability caps_;
> > >       V4L2DeviceFormat format_;
> > >       const PixelFormatInfo *formatInfo_;
> > > @@ -259,6 +265,9 @@ private:
> > >       EventNotifier *fdBufferNotifier_;
> > >
> > >       bool streaming_;
> > > +
> > > +     Timer timer_;

Given the Watchdog comments below - would calling this a Timer watchdog_
make it's purpose clearer rather than it's type?

(Of course if we had a Watchdog wrapper, it would then be Watchdog
watchdog_, so then what would it be watching?)

> > > +     utils::Duration timerDuration_;
> > >  };
> > >
> > >  class V4L2M2MDevice
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp
> > b/src/libcamera/v4l2_videodevice.cpp
> > > index 5f36ee20710d..25bce5475e07 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -600,6 +600,8 @@ int V4L2VideoDevice::open()
> > >       fdBufferNotifier_->activated.connect(this,
> > &V4L2VideoDevice::bufferAvailable);
> > >       fdBufferNotifier_->setEnabled(false);
> > >
> > > +     timer_.timeout.connect(this, &V4L2VideoDevice::timerExpired);
> >
> > You should do this in the constructor, or you'll end up with multiple
> > connections if the device is closed and reopened.
> >
> 
> Ack. We just fixed a problem with exactly this for Requests :)

I hope the assert we added would catch it then! I'd be interested if the
unit tests would trigger it - and if not - perhaps we need a multiple
close/open test on a V4L2 device or something like that.

But that's probably just another yak for another day, unless it's
interesting to you, particularly as that would then come with a whole
set of unit tests to make sure it barks ... and it may not be needed
anywhere else.


> >
> > > +
> > >       LOG(V4L2, Debug)
> > >               << "Opened device " << caps_.bus_info() << ": "
> > >               << caps_.driver() << ": " << caps_.card();
> > > @@ -1695,6 +1697,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >               return nullptr;
> > >       }
> > >
> > > +     if (timerDuration_.count())
> > > +             timer_.start(timerDuration_);
> > > +
> > >       cache_->put(buf.index);
> > >
> > >       FrameBuffer *buffer = it->second;
> > > @@ -1797,6 +1802,8 @@ int V4L2VideoDevice::streamOn()
> > >       }
> > >
> > >       streaming_ = true;
> > > +     if (timerDuration_.count())
> > > +             timer_.start(timerDuration_);
> > >
> > >       return 0;
> > >  }
> > > @@ -1821,6 +1828,9 @@ int V4L2VideoDevice::streamOff()
> > >       if (!streaming_ && queuedBuffers_.empty())
> > >               return 0;
> > >
> > > +     if (timerDuration_.count())
> > > +             timer_.stop();
> > > +
> > >       ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > >       if (ret < 0) {
> > >               LOG(V4L2, Error)
> > > @@ -1843,6 +1853,43 @@ int V4L2VideoDevice::streamOff()
> > >       return 0;
> > >  }
> > >
> > > +/**
> > > + * \brief Set the dequeue timeout value
> > > + * \param[in] duration The timeout value to be used
> > > + *
> > > + * Sets a timeout value, given by \a duration, that will be used by a
> > timer to
> > > + * ensure buffer dequeue events are periodically occurring. If the timer
> > > + * expires, a slot in the pipeline handler may be optionally called to
> > handle
> > > + * this condition through the \a dequeueTimeout signal.
> >
> > This should be "\ref V4L2VideoDevice::dequeueTimeout" to generate a
> > hyperlink. \a is for function parameters, and only serves to emphesize
> > them.
> >
> > It boils down to the same thing in the end, but from the point of view
> > of the V4L2VideoDevice, we're emitting a signal, not calling into the
> > pipeline handler. The signal could be connected to anything. You can
> > write
> >
> > "If the timer expires, the \ref V4L2VideoDevice::dequeueTimeout signal
> > is emitted. This can typically be used by pipeline handlers to be
> > notified of stalled devices."
> >
> > It would also be nice to expand this to tell that stall detection only
> > occurs when the device is streaming (that is, it is safe to set the
> > timeout at initialization time and not set it back to 0 when stopping
> > streaming).
> >
> 
> Sure, I'll reword it to be more descriptive.
> 
> Regards,
> Naush
> 
> 
> >
> > > + *
> > > + * Set \a duration to 0 to disable the timeout.
> > > + *
> > > + */
> > > +void V4L2VideoDevice::setDequeueTimeout(utils::Duration duration)
> > > +{
> > > +     timerDuration_ = duration;
> > > +
> > > +     timer_.stop();
> > > +     if (duration.count() && streaming_)
> > > +             timer_.start(duration);

A timer with a duration that gets reset is a Watchdog ... I wonder if
that should get wrapped into the same helpers header as Timer ... but
that's yet more yaks that we can shave later - I think this is fine for
now.  (Unless you like shaving them of course).


> > > +}
> > > +
> > > +/**
> > > + * \brief Slot to handle an expired dequeue timer.
> > > + *
> > > + * When this slot is called, the the time between successive dequeue
> > events
> >
> > s/the the/the/
> >
> > > + * is over the required timeout. Optionally call a slot in the pipeline
> > handler
> > > + * given by the \a dequeueTimeout signal.
> >
> > And here, just "Emit the dequeueTimeout signal." as it's internal
> > documentation, for a private function.
> >
> > > + */
> > > +void V4L2VideoDevice::timerExpired()
> > > +{
> > > +     LOG(V4L2, Info)
> >
> > Shouldn't this be at least a Warn ?
> 
> 
> > > +             << "Dequeue timer of " << timerDuration_
> > > +             << " has expired!";
> > > +
> > > +     dequeueTimeout.emit(this);
> > > +}
> > > +
> > >  /**
> > >   * \brief Create a new video device instance from \a entity in media
> > device
> > >   * \a media
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >