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

[Laurent Pinchart]

On Wed, Mar 23, 2022 at 10:17:54AM +0000, Kieran Bingham wrote:
> Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:11:48)
> > On Tue, 22 Mar 2022 at 21:47, Laurent Pinchart wrote:
> > > 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?

Instead of adding a pointer to the emitter to the signal, in case a user
may need it, you can connect the signal to a lambda that captures the
context. See the implementation of Request::Private::prepare() for an
example.

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

I like that.

> (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.
> > 
> > > > + *
> > > > + * 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