| Message ID | 20220329112929.465434-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:27) > 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@raspberrypi.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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_; > }; > > 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) > +{ > + 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 > -- > 2.25.1 >
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@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
Hi Umang, On Tue, 5 Apr 2022 at 14:56, Umang Jain <umang.jain@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@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. 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 >
Hi, On 4/5/22 19:49, Naushir Patuck wrote: > Hi Umang, > > On Tue, 5 Apr 2022 at 14:56, Umang Jain <umang.jain@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@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
Hi Naush, On Tue, Apr 05, 2022 at 03:19:29PM +0100, Naushir Patuck wrote: > On Tue, 5 Apr 2022 at 14:56, Umang Jain wrote: > > 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@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. That was about usage of utils::Duration in the libcamera-base layer itself (in the Timer class if I recall correctly). You can use it in here if desired, types defined in the libcamera API are accessible to libcamera internals. > > > }; > > > > > > 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 I'd call the parameter timeout instead of msec, the units are specified by the chrono type. > > > + * > > > + * 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. s/.$// > > > + * > > > + * 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!"; If watchdogDuration_ was stored as a utils::Duration, you could write << "Dequeue timer of " << watchdogDuration_ << " has expired!"; Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Would you like to submit a new version with utils::Duration here, or keep std::chrono::milliseconds ? > > > + > > > + dequeueTimeout.emit(); > > > +} > > > + > > > /** > > > * \brief Create a new video device instance from \a entity in media device > > > * \a media
Hi Laurent, Thank you for your feedback. On Tue, 5 Apr 2022 at 16:24, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Tue, Apr 05, 2022 at 03:19:29PM +0100, Naushir Patuck wrote: > > On Tue, 5 Apr 2022 at 14:56, Umang Jain wrote: > > > 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@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. > > That was about usage of utils::Duration in the libcamera-base layer > itself (in the Timer class if I recall correctly). You can use it in > here if desired, types defined in the libcamera API are accessible to > libcamera internals. > Ah, sorry that was my misunderstanding. I'll switch to using utils::Duration here then. > > > > > }; > > > > > > > > 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 > > I'd call the parameter timeout instead of msec, the units are specified > by the chrono type. > > > > > + * > > > > + * 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. > > s/.$// > > > > > + * > > > > + * 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!"; > > If watchdogDuration_ was stored as a utils::Duration, you could write > > << "Dequeue timer of " << watchdogDuration_ << " has > expired!"; > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Would you like to submit a new version with utils::Duration here, or > keep std::chrono::milliseconds ? > I'll submit a new version with utils::Duration + the other suggestions! Regards, Naush > > > > > + > > > > + dequeueTimeout.emit(); > > > > +} > > > > + > > > > /** > > > > * \brief Create a new video device instance from \a entity in > media device > > > > * \a media > > -- > Regards, > > Laurent Pinchart >
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_; }; 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) +{ + 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
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@raspberrypi.com> --- include/libcamera/internal/v4l2_videodevice.h | 10 ++++ src/libcamera/v4l2_videodevice.cpp | 54 +++++++++++++++++++ 2 files changed, 64 insertions(+)