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

[Umang Jain]

Hi,

On 4/5/22 19:49, Naushir Patuck wrote:
> Hi Umang,
>
> On Tue, 5 Apr 2022 at 14:56, Umang Jain <umang.jain at ideasonboard.com> wrote:
>
>> 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 ?
>>
> In the previous revision, I did use the utils::Duration type for this
> variable.
> However, Laurent suggested that we might want to avoid that as he intends to
> move utils::Duration to the "public" API, and would not be available to use
> in this case.  So I switched to std::chrono::milliseconds.


err, didn't read the previous iterations, sorry for the noise then :S

>
> Regards,
> Naush
>
>
>>>    };
>>>
>>>    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