[libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice: Add a dequeue timer

[Laurent Pinchart]

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;

> +
>  	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.

> +
>  	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).

> + *
> + * 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