<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, 22 Mar 2022 at 21:47, 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>
Thank you for the patch.<br>
<br>
On Tue, Mar 22, 2022 at 01:16:34PM +0000, 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 | 9 ++++<br>
> src/libcamera/v4l2_videodevice.cpp | 47 +++++++++++++++++++<br>
> 2 files changed, 56 insertions(+)<br>
> <br>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h<br>
> index 2d2ccc477c91..dd6e96438637 100644<br>
> --- a/include/libcamera/internal/v4l2_videodevice.h<br>
> +++ b/include/libcamera/internal/v4l2_videodevice.h<br>
> @@ -20,6 +20,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>
> @@ -216,6 +217,9 @@ public:<br>
> int streamOn();<br>
> int streamOff();<br>
> <br>
> + void setDequeueTimeout(utils::Duration duration);<br>
> + Signal<V4L2VideoDevice *> dequeueTimeout;<br>
<br>
We stopped passing the pointer to the emitter object when emitting a<br>
signal, so this should be just Signal<> dequeueTimeout;<br></blockquote><div><br>The intention here was to pass the V4L2VideoDevice instance to the</div><div>pipeline handler's slot callback. This way, the pipeline handler can have</div><div>a single slot for all devices it wants to monitor, and can distinguish which</div><div>device timed out. If you think that may not be necessary, I will remove it.</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>
> static std::unique_ptr<V4L2VideoDevice><br>
> fromEntityName(const MediaDevice *media, const std::string &entity);<br>
> <br>
> @@ -246,6 +250,8 @@ private:<br>
> void bufferAvailable();<br>
> FrameBuffer *dequeueBuffer();<br>
> <br>
> + void timerExpired();<br>
> +<br>
> V4L2Capability caps_;<br>
> V4L2DeviceFormat format_;<br>
> const PixelFormatInfo *formatInfo_;<br>
> @@ -259,6 +265,9 @@ private:<br>
> EventNotifier *fdBufferNotifier_;<br>
> <br>
> bool streaming_;<br>
> +<br>
> + Timer timer_;<br>
> + utils::Duration timerDuration_;<br>
> };<br>
> <br>
> class V4L2M2MDevice<br>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp<br>
> index 5f36ee20710d..25bce5475e07 100644<br>
> --- a/src/libcamera/v4l2_videodevice.cpp<br>
> +++ b/src/libcamera/v4l2_videodevice.cpp<br>
> @@ -600,6 +600,8 @@ int V4L2VideoDevice::open()<br>
> fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);<br>
> fdBufferNotifier_->setEnabled(false);<br>
> <br>
> + timer_.timeout.connect(this, &V4L2VideoDevice::timerExpired);<br>
<br>
You should do this in the constructor, or you'll end up with multiple<br>
connections if the device is closed and reopened.<br></blockquote><div><br></div><div>Ack. We just fixed a problem with exactly this for Requests :)</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>
> LOG(V4L2, Debug)<br>
> << "Opened device " << caps_.bus_info() << ": "<br>
> << caps_.driver() << ": " << caps_.card();<br>
> @@ -1695,6 +1697,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()<br>
> return nullptr;<br>
> }<br>
> <br>
> + if (timerDuration_.count())<br>
> + timer_.start(timerDuration_);<br>
> +<br>
> cache_->put(buf.index);<br>
> <br>
> FrameBuffer *buffer = it->second;<br>
> @@ -1797,6 +1802,8 @@ int V4L2VideoDevice::streamOn()<br>
> }<br>
> <br>
> streaming_ = true;<br>
> + if (timerDuration_.count())<br>
> + timer_.start(timerDuration_);<br>
> <br>
> return 0;<br>
> }<br>
> @@ -1821,6 +1828,9 @@ int V4L2VideoDevice::streamOff()<br>
> if (!streaming_ && queuedBuffers_.empty())<br>
> return 0;<br>
> <br>
> + if (timerDuration_.count())<br>
> + timer_.stop();<br>
> +<br>
> ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);<br>
> if (ret < 0) {<br>
> LOG(V4L2, Error)<br>
> @@ -1843,6 +1853,43 @@ int V4L2VideoDevice::streamOff()<br>
> return 0;<br>
> }<br>
> <br>
> +/**<br>
> + * \brief Set the dequeue timeout value<br>
> + * \param[in] duration The timeout value to be used<br>
> + *<br>
> + * Sets a timeout value, given by \a duration, that will be used by a timer to<br>
> + * ensure buffer dequeue events are periodically occurring. If the timer<br>
> + * expires, a slot in the pipeline handler may be optionally called to handle<br>
> + * this condition through the \a dequeueTimeout signal.<br>
<br>
This should be "\ref V4L2VideoDevice::dequeueTimeout" to generate a<br>
hyperlink. \a is for function parameters, and only serves to emphesize<br>
them.<br>
<br>
It boils down to the same thing in the end, but from the point of view<br>
of the V4L2VideoDevice, we're emitting a signal, not calling into the<br>
pipeline handler. The signal could be connected to anything. You can<br>
write<br>
<br>
"If the timer expires, the \ref V4L2VideoDevice::dequeueTimeout signal<br>
is emitted. This can typically be used by pipeline handlers to be<br>
notified of stalled devices."<br>
<br>
It would also be nice to expand this to tell that stall detection only<br>
occurs when the device is streaming (that is, it is safe to set the<br>
timeout at initialization time and not set it back to 0 when stopping<br>
streaming).<br></blockquote><div><br></div><div>Sure, I'll reword it to be more descriptive.</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>
> + * Set \a duration to 0 to disable the timeout.<br>
> + *<br>
> + */<br>
> +void V4L2VideoDevice::setDequeueTimeout(utils::Duration duration)<br>
> +{<br>
> + timerDuration_ = duration;<br>
> +<br>
> + timer_.stop();<br>
> + if (duration.count() && streaming_)<br>
> + timer_.start(duration);<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Slot to handle an expired dequeue timer.<br>
> + *<br>
> + * When this slot is called, the the time between successive dequeue events<br>
<br>
s/the the/the/<br>
<br>
> + * is over the required timeout. Optionally call a slot in the pipeline handler<br>
> + * given by the \a dequeueTimeout signal.<br>
<br>
And here, just "Emit the dequeueTimeout signal." as it's internal<br>
documentation, for a private function.<br>
<br>
> + */<br>
> +void V4L2VideoDevice::timerExpired()<br>
> +{<br>
> + LOG(V4L2, Info)<br>
<br>
Shouldn't this be at least a Warn ?</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + << "Dequeue timer of " << timerDuration_<br>
> + << " has expired!";<br>
> +<br>
> + dequeueTimeout.emit(this);<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>