| Message ID | 20220322131635.2869526-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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
Hi Laurent, Thank you for your feedback. On Tue, 22 Mar 2022 at 21:47, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > 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@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; > The intention here was to pass the V4L2VideoDevice instance to the pipeline handler's slot callback. This way, the pipeline handler can have a single slot for all devices it wants to monitor, and can distinguish which device timed out. If you think that may not be necessary, I will remove it. > > > + > > 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. > Ack. We just fixed a problem with exactly this for Requests :) > > > + > > 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). > Sure, I'll reword it to be more descriptive. Regards, Naush > > > + * > > + * 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 >
Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:11:48) > Hi Laurent, > > Thank you for your feedback. > > On Tue, 22 Mar 2022 at 21:47, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > > > 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@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; > > > > The intention here was to pass the V4L2VideoDevice instance to the > pipeline handler's slot callback. This way, the pipeline handler can have > a single slot for all devices it wants to monitor, and can distinguish which > device timed out. If you think that may not be necessary, I will remove it. > For this use case, being able to report which device has stalled could be useful, though I imagine it will already be done by the V4L2VideoDevice itself before emitting this signal so it will already be identifiable? > > > > > + > > > 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_; Given the Watchdog comments below - would calling this a Timer watchdog_ make it's purpose clearer rather than it's type? (Of course if we had a Watchdog wrapper, it would then be Watchdog watchdog_, so then what would it be watching?) > > > + 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. > > > > Ack. We just fixed a problem with exactly this for Requests :) I hope the assert we added would catch it then! I'd be interested if the unit tests would trigger it - and if not - perhaps we need a multiple close/open test on a V4L2 device or something like that. But that's probably just another yak for another day, unless it's interesting to you, particularly as that would then come with a whole set of unit tests to make sure it barks ... and it may not be needed anywhere else. > > > > > + > > > 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). > > > > Sure, I'll reword it to be more descriptive. > > Regards, > Naush > > > > > > > + * > > > + * 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); A timer with a duration that gets reset is a Watchdog ... I wonder if that should get wrapped into the same helpers header as Timer ... but that's yet more yaks that we can shave later - I think this is fine for now. (Unless you like shaving them of course). > > > +} > > > + > > > +/** > > > + * \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 > >
On Wed, Mar 23, 2022 at 10:17:54AM +0000, Kieran Bingham wrote: > Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:11:48) > > On Tue, 22 Mar 2022 at 21:47, Laurent Pinchart wrote: > > > 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@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; > > > > The intention here was to pass the V4L2VideoDevice instance to the > > pipeline handler's slot callback. This way, the pipeline handler can have > > a single slot for all devices it wants to monitor, and can distinguish which > > device timed out. If you think that may not be necessary, I will remove it. > > For this use case, being able to report which device has stalled could > be useful, though I imagine it will already be done by the > V4L2VideoDevice itself before emitting this signal so it will already be > identifiable? Instead of adding a pointer to the emitter to the signal, in case a user may need it, you can connect the signal to a lambda that captures the context. See the implementation of Request::Private::prepare() for an example. > > > > + > > > > 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_; > > Given the Watchdog comments below - would calling this a Timer watchdog_ > make it's purpose clearer rather than it's type? I like that. > (Of course if we had a Watchdog wrapper, it would then be Watchdog > watchdog_, so then what would it be watching?) > > > > > + 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. > > > > Ack. We just fixed a problem with exactly this for Requests :) > > I hope the assert we added would catch it then! I'd be interested if the > unit tests would trigger it - and if not - perhaps we need a multiple > close/open test on a V4L2 device or something like that. > > But that's probably just another yak for another day, unless it's > interesting to you, particularly as that would then come with a whole > set of unit tests to make sure it barks ... and it may not be needed > anywhere else. > > > > > + > > > > 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). > > > > Sure, I'll reword it to be more descriptive. > > > > > > + * > > > > + * 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); > > A timer with a duration that gets reset is a Watchdog ... I wonder if > that should get wrapped into the same helpers header as Timer ... but > that's yet more yaks that we can shave later - I think this is fine for > now. (Unless you like shaving them of course). > > > > > +} > > > > + > > > > +/** > > > > + * \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
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; + 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); + 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. + * + * 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 + * is over the required timeout. Optionally call a slot in the pipeline handler + * given by the \a dequeueTimeout signal. + */ +void V4L2VideoDevice::timerExpired() +{ + LOG(V4L2, Info) + << "Dequeue timer of " << timerDuration_ + << " has expired!"; + + dequeueTimeout.emit(this); +} + /** * \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 | 9 ++++ src/libcamera/v4l2_videodevice.cpp | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+)