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