[libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice: Add a dequeue timer
Naushir Patuck
naush at raspberrypi.com
Wed Mar 23 10:11:48 CET 2022
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>
More information about the libcamera-devel
mailing list