[libcamera-devel] [PATCH v3 1/3] libcamera: v4l2_videodevice: Add a dequeue timer

[Umang Jain]

Hi Naushir,

Just a drive-by comment,

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 ?

>   };
>   
>   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
> + *
> + * 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.
> + *
> + * 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!";
> +
> +	dequeueTimeout.emit();
> +}
> +
>   /**
>    * \brief Create a new video device instance from \a entity in media device
>    * \a media