[libcamera-devel] [PATCH v3 1/3] libcamera: v4l2_videodevice: Add a dequeue timer
Naushir Patuck
naush at raspberrypi.com
Wed Apr 6 09:03:59 CEST 2022
Hi Laurent,
Thank you for your feedback.
On Tue, 5 Apr 2022 at 16:24, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> On Tue, Apr 05, 2022 at 03:19:29PM +0100, Naushir Patuck wrote:
> > On Tue, 5 Apr 2022 at 14:56, Umang Jain wrote:
> > > On 3/29/22 16:59, 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 | 10 ++++
> > > > src/libcamera/v4l2_videodevice.cpp | 54
> +++++++++++++++++++
> > > > 2 files changed, 64 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h
> b/include/libcamera/internal/v4l2_videodevice.h
> > > > index cfeae7bd6c52..2a9ba1fe5c71 100644
> > > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > > @@ -9,6 +9,7 @@
> > > >
> > > > #include <array>
> > > > #include <atomic>
> > > > +#include <chrono>
> > > > #include <memory>
> > > > #include <optional>
> > > > #include <stdint.h>
> > > > @@ -20,6 +21,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>
> > > > @@ -217,6 +219,9 @@ public:
> > > > int streamOn();
> > > > int streamOff();
> > > >
> > > > + void setDequeueTimeout(std::chrono::milliseconds msec);
> > > > + Signal<> dequeueTimeout;
> > > > +
> > > > static std::unique_ptr<V4L2VideoDevice>
> > > > fromEntityName(const MediaDevice *media, const std::string
> &entity);
> > > >
> > > > @@ -253,6 +258,8 @@ private:
> > > > void bufferAvailable();
> > > > FrameBuffer *dequeueBuffer();
> > > >
> > > > + void watchdogExpired();
> > > > +
> > > > V4L2Capability caps_;
> > > > V4L2DeviceFormat format_;
> > > > const PixelFormatInfo *formatInfo_;
> > > > @@ -266,6 +273,9 @@ private:
> > > > EventNotifier *fdBufferNotifier_;
> > > >
> > > > State state_;
> > > > +
> > > > + Timer watchdog_;
> > > > + std::chrono::milliseconds watchdogDuration_;
> > >
> > > Any particular reason this is std::chrono::miiliseconds instead of
> using
> > > utils::Duration ?
> >
> > In the previous revision, I did use the utils::Duration type for this
> variable.
> > However, Laurent suggested that we might want to avoid that as he
> intends to
> > move utils::Duration to the "public" API, and would not be available to
> use
> > in this case. So I switched to std::chrono::milliseconds.
>
> That was about usage of utils::Duration in the libcamera-base layer
> itself (in the Timer class if I recall correctly). You can use it in
> here if desired, types defined in the libcamera API are accessible to
> libcamera internals.
>
Ah, sorry that was my misunderstanding. I'll switch to using
utils::Duration here then.
>
> > > > };
> > > >
> > > > class V4L2M2MDevice
> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp
> b/src/libcamera/v4l2_videodevice.cpp
> > > > index 009f6d55610f..22191cb9de4d 100644
> > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > @@ -539,6 +539,7 @@ V4L2VideoDevice::V4L2VideoDevice(const
> std::string &deviceNode)
> > > > V4L2VideoDevice::V4L2VideoDevice(const MediaEntity *entity)
> > > > : V4L2VideoDevice(entity->deviceNode())
> > > > {
> > > > + watchdog_.timeout.connect(this,
> &V4L2VideoDevice::watchdogExpired);
> > > > }
> > > >
> > > > V4L2VideoDevice::~V4L2VideoDevice()
> > > > @@ -1723,6 +1724,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > > > return nullptr;
> > > > }
> > > >
> > > > + if (watchdogDuration_.count())
> > > > + watchdog_.start(watchdogDuration_);
> > > > +
> > > > cache_->put(buf.index);
> > > >
> > > > FrameBuffer *buffer = it->second;
> > > > @@ -1825,6 +1829,8 @@ int V4L2VideoDevice::streamOn()
> > > > }
> > > >
> > > > state_ = State::Streaming;
> > > > + if (watchdogDuration_.count())
> > > > + watchdog_.start(watchdogDuration_);
> > > >
> > > > return 0;
> > > > }
> > > > @@ -1849,6 +1855,9 @@ int V4L2VideoDevice::streamOff()
> > > > if (state_ != State::Streaming && queuedBuffers_.empty())
> > > > return 0;
> > > >
> > > > + if (watchdogDuration_.count())
> > > > + watchdog_.stop();
> > > > +
> > > > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > > > if (ret < 0) {
> > > > LOG(V4L2, Error)
> > > > @@ -1876,6 +1885,51 @@ int V4L2VideoDevice::streamOff()
> > > > return 0;
> > > > }
> > > >
> > > > +/**
> > > > + * \brief Set the dequeue timeout value
> > > > + * \param[in] msec The timeout value to be used
>
> I'd call the parameter timeout instead of msec, the units are specified
> by the chrono type.
>
> > > > + *
> > > > + * Sets a timeout value, given by \a msec, that will be used by a
> watchdog timer
> > > > + * to ensure buffer dequeue events are periodically occurring when
> the device is
> > > > + * streaming. The watchdog timer is only active when the device is
> streaming, so
> > > > + * it is not necessary to disable it when the device stops
> streaming. The timeout
> > > > + * value can be safely updated at any time.
> > > > + *
> > > > + * 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.
> > > > + *
> > > > + * Set \a msec to 0 to disable the watchdog timer.
> > > > + */
> > > > +void V4L2VideoDevice::setDequeueTimeout(std::chrono::milliseconds
> msec)
> > >
> > > Same here as well. I guess you take in a `utils::Duration msec` and
> > > assign to watchdogDuration_ below (given you have utils::Duration
> > > watchdogDuration_ too).
> > >
> > > > +{
> > > > + watchdogDuration_ = msec;
> > > > +
> > > > + watchdog_.stop();
> > > > + if (watchdogDuration_.count() && state_ == State::Streaming)
> > > > + watchdog_.start(msec);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \var V4L2VideoDevice::dequeueTimeout
> > > > + * \brief A Signal emitted when the dequeue watchdog timer expires
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Slot to handle an expired dequeue timer.
>
> s/.$//
>
> > > > + *
> > > > + * When this slot is called, the time between successive dequeue
> events is over
> > > > + * the required timeout. Emit the \ref
> V4L2VideoDevice::dequeueTimeout signal.
> > > > + */
> > > > +void V4L2VideoDevice::watchdogExpired()
> > > > +{
> > > > + LOG(V4L2, Warning)
> > > > + << "Dequeue timer of " << watchdogDuration_.count()
> > > > + << " ms has expired!";
>
> If watchdogDuration_ was stored as a utils::Duration, you could write
>
> << "Dequeue timer of " << watchdogDuration_ << " has
> expired!";
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Would you like to submit a new version with utils::Duration here, or
> keep std::chrono::milliseconds ?
>
I'll submit a new version with utils::Duration + the other suggestions!
Regards,
Naush
>
> > > > +
> > > > + dequeueTimeout.emit();
> > > > +}
> > > > +
> > > > /**
> > > > * \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/20220406/ab3da792/attachment-0001.htm>
More information about the libcamera-devel
mailing list