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

[Naushir Patuck]

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.


>
> > +
> >       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_;
> > +     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 :)


>
> > +
> >       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);
> > +}
> > +
> > +/**
> > + * \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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220323/9fdde01c/attachment-0001.htm>