| Message ID | 20220505084824.4104296-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | c39b52c1840545bf89ff048764a4b550db465624 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Quoting Naushir Patuck via libcamera-devel (2022-05-05 09:48:24) > Only enable/reset the watchdog timer when there are buffers queued in the V4L2 > device. Otherwise, we may trigger spurious warnings when the watchdog times out > even if there are no buffers queued in the device. aha yes - I can see how that was tripping up on python interactive sessions or otherwise underflowing when it's not at all a fault of the V4L2VideoDevice. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 5b4637b1a39e..430715afd554 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > return ret; > } > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(true); > + if (watchdogDuration_) > + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > + } I guess this could also be set outside of the if (queuedBuffer_.empty()) - but it would artificially delay the watchdog every time a buffer was queued. In the event that more than one buffer is required to be queued (to satisfy internal requirements perhaps?) that might actually be beneficial ... But I think either way is fine. > > queuedBuffers_[buf.index] = buffer; > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > return nullptr; > } > > - if (watchdogDuration_.count()) > - watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > - > cache_->put(buf.index); > > FrameBuffer *buffer = it->second; > queuedBuffers_.erase(it); > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(false); > + watchdog_.stop(); > + } else if (watchdogDuration_) { > + /* > + * Restart the watchdog timer if there are buffers still queued > + * in the device. > + */ > + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); Why do we need/have all these casts? Is either watchdogDuration_ not a duration? or is watchdog_.start() not accepting a duration? Either of those would remove a lot of casting surely? watchdogDuration_ is a utils::Duration which stores std::nano, and I see Timer start takes a std::chrono::milliseconds. I think it would make sense to add a 'void start(std::chrono::duration)' to the timer class and simplify these casts. But that doesn't have to be fixed in this patch. Would you like to do that as a patch on top? If you don't want to let me know and I'll handle it after this patch is merged. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + } > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR > ? FrameMetadata::FrameError > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() > } > > state_ = State::Streaming; > - if (watchdogDuration_.count()) > + if (watchdogDuration_ && !queuedBuffers_.empty()) > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > return 0; > @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) > watchdogDuration_ = timeout; > > watchdog_.stop(); > - if (watchdogDuration_.count() && state_ == State::Streaming) > + if (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty()) > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); > } > > -- > 2.25.1 >
Hi Kieran, Thank you for the feedback! On Thu, 5 May 2022 at 10:36, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > Quoting Naushir Patuck via libcamera-devel (2022-05-05 09:48:24) > > Only enable/reset the watchdog timer when there are buffers queued in > the V4L2 > > device. Otherwise, we may trigger spurious warnings when the watchdog > times out > > even if there are no buffers queued in the device. > > aha yes - I can see how that was tripping up on python interactive > sessions or otherwise underflowing when it's not at all a fault of the > V4L2VideoDevice. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp > b/src/libcamera/v4l2_videodevice.cpp > > index 5b4637b1a39e..430715afd554 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer > *buffer) > > return ret; > > } > > > > - if (queuedBuffers_.empty()) > > + if (queuedBuffers_.empty()) { > > fdBufferNotifier_->setEnabled(true); > > + if (watchdogDuration_) > > + > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > + } > > I guess this could also be set outside of the if (queuedBuffer_.empty()) > - but it would artificially delay the watchdog every time a buffer was > queued. In the event that more than one buffer is required to be queued > (to satisfy internal requirements perhaps?) that might actually be > beneficial ... But I think either way is fine. > It could... I thought it might be ever so slightly more logically correct if this was inside the if() block. I'll keep it in there unless there are further objections. > > > > > queuedBuffers_[buf.index] = buffer; > > > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > return nullptr; > > } > > > > - if (watchdogDuration_.count()) > > - > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > - > > cache_->put(buf.index); > > > > FrameBuffer *buffer = it->second; > > queuedBuffers_.erase(it); > > > > - if (queuedBuffers_.empty()) > > + if (queuedBuffers_.empty()) { > > fdBufferNotifier_->setEnabled(false); > > + watchdog_.stop(); > > + } else if (watchdogDuration_) { > > + /* > > + * Restart the watchdog timer if there are buffers still > queued > > + * in the device. > > + */ > > + > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > Why do we need/have all these casts? Is either watchdogDuration_ not a > duration? or is watchdog_.start() not accepting a duration? Either of > those would remove a lot of casting surely? > This is a bit of a minor annoyance because of how I defined the underlying type of utils::Duration to a double. The std::chrono library forbids implicit casts from integer to float types. > watchdogDuration_ is a utils::Duration which stores std::nano, and I see > Timer start takes a std::chrono::milliseconds. I think it would make > sense to add a 'void start(std::chrono::duration)' to the timer class > and simplify these casts. But that doesn't have to be fixed in this > patch. > > Would you like to do that as a patch on top? If you don't want to let me > know and I'll handle it after this patch is merged. > What we've talked about doing is to switch utils::Duration to use an integer underlying type, which would allow implicit conversions and remove the ugly casts like the above. I believe Laurent was still thinking this through as there were also plans to make utils::Duration part of the public API... Regards, Naush > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > + } > > > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR > > ? FrameMetadata::FrameError > > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() > > } > > > > state_ = State::Streaming; > > - if (watchdogDuration_.count()) > > + if (watchdogDuration_ && !queuedBuffers_.empty()) > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > > return 0; > > @@ -1924,7 +1932,7 @@ void > V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) > > watchdogDuration_ = timeout; > > > > watchdog_.stop(); > > - if (watchdogDuration_.count() && state_ == State::Streaming) > > + if (watchdogDuration_ && state_ == State::Streaming && > !queuedBuffers_.empty()) > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); > > } > > > > -- > > 2.25.1 > > >
Quoting Naushir Patuck (2022-05-05 11:15:44) > Hi Kieran, > > Thank you for the feedback! > > On Thu, 5 May 2022 at 10:36, Kieran Bingham <kieran.bingham@ideasonboard.com> > wrote: > > > Hi Naush, > > > > Quoting Naushir Patuck via libcamera-devel (2022-05-05 09:48:24) > > > Only enable/reset the watchdog timer when there are buffers queued in > > the V4L2 > > > device. Otherwise, we may trigger spurious warnings when the watchdog > > times out > > > even if there are no buffers queued in the device. > > > > aha yes - I can see how that was tripping up on python interactive > > sessions or otherwise underflowing when it's not at all a fault of the > > V4L2VideoDevice. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- > > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp > > b/src/libcamera/v4l2_videodevice.cpp > > > index 5b4637b1a39e..430715afd554 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer > > *buffer) > > > return ret; > > > } > > > > > > - if (queuedBuffers_.empty()) > > > + if (queuedBuffers_.empty()) { > > > fdBufferNotifier_->setEnabled(true); > > > + if (watchdogDuration_) > > > + > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > + } > > > > I guess this could also be set outside of the if (queuedBuffer_.empty()) > > - but it would artificially delay the watchdog every time a buffer was > > queued. In the event that more than one buffer is required to be queued > > (to satisfy internal requirements perhaps?) that might actually be > > beneficial ... But I think either way is fine. > > > > It could... I thought it might be ever so slightly more logically correct > if this > was inside the if() block. I'll keep it in there unless there are further > objections. > > > > > > > > > > queuedBuffers_[buf.index] = buffer; > > > > > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > return nullptr; > > > } > > > > > > - if (watchdogDuration_.count()) > > > - > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > - > > > cache_->put(buf.index); > > > > > > FrameBuffer *buffer = it->second; > > > queuedBuffers_.erase(it); > > > > > > - if (queuedBuffers_.empty()) > > > + if (queuedBuffers_.empty()) { > > > fdBufferNotifier_->setEnabled(false); > > > + watchdog_.stop(); > > > + } else if (watchdogDuration_) { > > > + /* > > > + * Restart the watchdog timer if there are buffers still > > queued > > > + * in the device. > > > + */ > > > + > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > > Why do we need/have all these casts? Is either watchdogDuration_ not a > > duration? or is watchdog_.start() not accepting a duration? Either of > > those would remove a lot of casting surely? > > > > This is a bit of a minor annoyance because of how I defined the underlying > type > of utils::Duration to a double. The std::chrono library forbids implicit > casts > from integer to float types. > > > > watchdogDuration_ is a utils::Duration which stores std::nano, and I see > > Timer start takes a std::chrono::milliseconds. I think it would make > > sense to add a 'void start(std::chrono::duration)' to the timer class > > and simplify these casts. But that doesn't have to be fixed in this > > patch. > > > > Would you like to do that as a patch on top? If you don't want to let me > > know and I'll handle it after this patch is merged. > > > > What we've talked about doing is to switch utils::Duration to use an integer > underlying type, which would allow implicit conversions and remove the ugly > casts like the above. I believe Laurent was still thinking this through as > there > were also plans to make utils::Duration part of the public API... Ayeee - I see - not as simple and obvious as I thought it should be (which explains why it isn't so ...) Well - I don't think it should affect this patch anyway. -- Kieran > > Regards, > Naush > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > + } > > > > > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR > > > ? FrameMetadata::FrameError > > > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() > > > } > > > > > > state_ = State::Streaming; > > > - if (watchdogDuration_.count()) > > > + if (watchdogDuration_ && !queuedBuffers_.empty()) > > > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > > > > return 0; > > > @@ -1924,7 +1932,7 @@ void > > V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) > > > watchdogDuration_ = timeout; > > > > > > watchdog_.stop(); > > > - if (watchdogDuration_.count() && state_ == State::Streaming) > > > + if (watchdogDuration_ && state_ == State::Streaming && > > !queuedBuffers_.empty()) > > > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); > > > } > > > > > > -- > > > 2.25.1 > > > > >
Hi Naush Thanks for fixing this! On Thu, 5 May 2022 at 09:48, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Only enable/reset the watchdog timer when there are buffers queued in the V4L2 > device. Otherwise, we may trigger spurious warnings when the watchdog times out > even if there are no buffers queued in the device. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Works for me, and on the Picamera2 auto test machine: Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > --- > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 5b4637b1a39e..430715afd554 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > return ret; > } > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(true); > + if (watchdogDuration_) > + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > + } > > queuedBuffers_[buf.index] = buffer; > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > return nullptr; > } > > - if (watchdogDuration_.count()) > - watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > - > cache_->put(buf.index); > > FrameBuffer *buffer = it->second; > queuedBuffers_.erase(it); > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(false); > + watchdog_.stop(); > + } else if (watchdogDuration_) { > + /* > + * Restart the watchdog timer if there are buffers still queued > + * in the device. > + */ > + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > + } > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR > ? FrameMetadata::FrameError > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() > } > > state_ = State::Streaming; > - if (watchdogDuration_.count()) > + if (watchdogDuration_ && !queuedBuffers_.empty()) > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > return 0; > @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) > watchdogDuration_ = timeout; > > watchdog_.stop(); > - if (watchdogDuration_.count() && state_ == State::Streaming) > + if (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty()) > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); > } > > -- > 2.25.1 >
Hi, On Thu, 5 May 2022 at 09:48, Naushir Patuck <naush@raspberrypi.com> wrote: > Only enable/reset the watchdog timer when there are buffers queued in the > V4L2 > device. Otherwise, we may trigger spurious warnings when the watchdog > times out > even if there are no buffers queued in the device. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Gentle ping. This needs one more R-B tag to get merged. Regards, Naush > --- > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp > b/src/libcamera/v4l2_videodevice.cpp > index 5b4637b1a39e..430715afd554 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer > *buffer) > return ret; > } > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(true); > + if (watchdogDuration_) > + > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > + } > > queuedBuffers_[buf.index] = buffer; > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > return nullptr; > } > > - if (watchdogDuration_.count()) > - > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > - > cache_->put(buf.index); > > FrameBuffer *buffer = it->second; > queuedBuffers_.erase(it); > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(false); > + watchdog_.stop(); > + } else if (watchdogDuration_) { > + /* > + * Restart the watchdog timer if there are buffers still > queued > + * in the device. > + */ > + > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > + } > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR > ? FrameMetadata::FrameError > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() > } > > state_ = State::Streaming; > - if (watchdogDuration_.count()) > + if (watchdogDuration_ && !queuedBuffers_.empty()) > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > return 0; > @@ -1924,7 +1932,7 @@ void > V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) > watchdogDuration_ = timeout; > > watchdog_.stop(); > - if (watchdogDuration_.count() && state_ == State::Streaming) > + if (watchdogDuration_ && state_ == State::Streaming && > !queuedBuffers_.empty()) > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); > } > > -- > 2.25.1 > >
Quoting Naushir Patuck via libcamera-devel (2022-05-10 08:58:39) > Hi, > > On Thu, 5 May 2022 at 09:48, Naushir Patuck <naush@raspberrypi.com> wrote: > > > Only enable/reset the watchdog timer when there are buffers queued in the > > V4L2 > > device. Otherwise, we may trigger spurious warnings when the watchdog > > times out > > even if there are no buffers queued in the device. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Gentle ping. This needs one more R-B tag to get merged. David, can we upgrade your Tested-by: to a Reviewed-by: ? (Or maybe TB is sufficient?) > > Regards, > Naush > > > > --- > > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp > > b/src/libcamera/v4l2_videodevice.cpp > > index 5b4637b1a39e..430715afd554 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer > > *buffer) > > return ret; > > } > > > > - if (queuedBuffers_.empty()) > > + if (queuedBuffers_.empty()) { > > fdBufferNotifier_->setEnabled(true); > > + if (watchdogDuration_) > > + > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > + } > > > > queuedBuffers_[buf.index] = buffer; > > > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > return nullptr; > > } > > > > - if (watchdogDuration_.count()) > > - > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > - > > cache_->put(buf.index); > > > > FrameBuffer *buffer = it->second; > > queuedBuffers_.erase(it); > > > > - if (queuedBuffers_.empty()) > > + if (queuedBuffers_.empty()) { > > fdBufferNotifier_->setEnabled(false); > > + watchdog_.stop(); > > + } else if (watchdogDuration_) { > > + /* > > + * Restart the watchdog timer if there are buffers still > > queued > > + * in the device. > > + */ > > + > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > + } > > > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR > > ? FrameMetadata::FrameError > > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() > > } > > > > state_ = State::Streaming; > > - if (watchdogDuration_.count()) > > + if (watchdogDuration_ && !queuedBuffers_.empty()) > > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > > > return 0; > > @@ -1924,7 +1932,7 @@ void > > V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) > > watchdogDuration_ = timeout; > > > > watchdog_.stop(); > > - if (watchdogDuration_.count() && state_ == State::Streaming) > > + if (watchdogDuration_ && state_ == State::Streaming && > > !queuedBuffers_.empty()) > > > > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); > > } > > > > -- > > 2.25.1 > > > >
Hi Naush, Thank you for the patch. On Thu, May 05, 2022 at 09:48:24AM +0100, Naushir Patuck via libcamera-devel wrote: > Only enable/reset the watchdog timer when there are buffers queued in the V4L2 > device. Otherwise, we may trigger spurious warnings when the watchdog times out > even if there are no buffers queued in the device. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 5b4637b1a39e..430715afd554 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > return ret; > } > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(true); > + if (watchdogDuration_) > + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > + } > > queuedBuffers_[buf.index] = buffer; > > @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > return nullptr; > } > > - if (watchdogDuration_.count()) > - watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > - > cache_->put(buf.index); > > FrameBuffer *buffer = it->second; > queuedBuffers_.erase(it); > > - if (queuedBuffers_.empty()) > + if (queuedBuffers_.empty()) { > fdBufferNotifier_->setEnabled(false); > + watchdog_.stop(); > + } else if (watchdogDuration_) { > + /* > + * Restart the watchdog timer if there are buffers still queued > + * in the device. > + */ > + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > + } > > buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR > ? FrameMetadata::FrameError > @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() > } > > state_ = State::Streaming; > - if (watchdogDuration_.count()) > + if (watchdogDuration_ && !queuedBuffers_.empty()) > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); > > return 0; > @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) > watchdogDuration_ = timeout; > > watchdog_.stop(); > - if (watchdogDuration_.count() && state_ == State::Streaming) > + if (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty()) > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); > } >
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 5b4637b1a39e..430715afd554 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) return ret; } - if (queuedBuffers_.empty()) + if (queuedBuffers_.empty()) { fdBufferNotifier_->setEnabled(true); + if (watchdogDuration_) + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); + } queuedBuffers_[buf.index] = buffer; @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() return nullptr; } - if (watchdogDuration_.count()) - watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); - cache_->put(buf.index); FrameBuffer *buffer = it->second; queuedBuffers_.erase(it); - if (queuedBuffers_.empty()) + if (queuedBuffers_.empty()) { fdBufferNotifier_->setEnabled(false); + watchdog_.stop(); + } else if (watchdogDuration_) { + /* + * Restart the watchdog timer if there are buffers still queued + * in the device. + */ + watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); + } buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR ? FrameMetadata::FrameError @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn() } state_ = State::Streaming; - if (watchdogDuration_.count()) + if (watchdogDuration_ && !queuedBuffers_.empty()) watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_)); return 0; @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout) watchdogDuration_ = timeout; watchdog_.stop(); - if (watchdogDuration_.count() && state_ == State::Streaming) + if (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty()) watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout)); }
Only enable/reset the watchdog timer when there are buffers queued in the V4L2 device. Otherwise, we may trigger spurious warnings when the watchdog times out even if there are no buffers queued in the device. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)