[{"id":22361,"web_url":"https://patchwork.libcamera.org/comment/22361/","msgid":"","date":"2022-03-22T21:47:25","subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Mar 22, 2022 at 01:16:34PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Add a timer that gets reset on every buffer dequeue event. If the timeout\n> expires, optionally call a slot in the pipeline handler to handle this\n> condition. This may be useful in detecting and handling stalls in either the\n> hardware or device driver.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 9 ++++\n> src/libcamera/v4l2_videodevice.cpp | 47 +++++++++++++++++++\n> 2 files changed, 56 insertions(+)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 2d2ccc477c91..dd6e96438637 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -20,6 +20,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -216,6 +217,9 @@ public:\n> \tint streamOn();\n> \tint streamOff();\n> \n> +\tvoid setDequeueTimeout(utils::Duration duration);\n> +\tSignal dequeueTimeout;\n\nWe stopped passing the pointer to the emitter object when emitting a\nsignal, so this should be just Signal<> dequeueTimeout;\n\n> +\n> \tstatic std::unique_ptr\n> \tfromEntityName(const MediaDevice *media, const std::string &entity);\n> \n> @@ -246,6 +250,8 @@ private:\n> \tvoid bufferAvailable();\n> \tFrameBuffer *dequeueBuffer();\n> \n> +\tvoid timerExpired();\n> +\n> \tV4L2Capability caps_;\n> \tV4L2DeviceFormat format_;\n> \tconst PixelFormatInfo *formatInfo_;\n> @@ -259,6 +265,9 @@ private:\n> \tEventNotifier *fdBufferNotifier_;\n> \n> \tbool streaming_;\n> +\n> +\tTimer timer_;\n> +\tutils::Duration timerDuration_;\n> };\n> \n> class V4L2M2MDevice\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 5f36ee20710d..25bce5475e07 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -600,6 +600,8 @@ int V4L2VideoDevice::open()\n> \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> \tfdBufferNotifier_->setEnabled(false);\n> \n> +\ttimer_.timeout.connect(this, &V4L2VideoDevice::timerExpired);\n\nYou should do this in the constructor, or you'll end up with multiple\nconnections if the device is closed and reopened.\n\n> +\n> \tLOG(V4L2, Debug)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> @@ -1695,6 +1697,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \t\treturn nullptr;\n> \t}\n> \n> +\tif (timerDuration_.count())\n> +\t\ttimer_.start(timerDuration_);\n> +\n> \tcache_->put(buf.index);\n> \n> \tFrameBuffer *buffer = it->second;\n> @@ -1797,6 +1802,8 @@ int V4L2VideoDevice::streamOn()\n> \t}\n> \n> \tstreaming_ = true;\n> +\tif (timerDuration_.count())\n> +\t\ttimer_.start(timerDuration_);\n> \n> \treturn 0;\n> }\n> @@ -1821,6 +1828,9 @@ int V4L2VideoDevice::streamOff()\n> \tif (!streaming_ && queuedBuffers_.empty())\n> \t\treturn 0;\n> \n> +\tif (timerDuration_.count())\n> +\t\ttimer_.stop();\n> +\n> \tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> \tif (ret < 0) {\n> \t\tLOG(V4L2, Error)\n> @@ -1843,6 +1853,43 @@ int V4L2VideoDevice::streamOff()\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Set the dequeue timeout value\n> + * \\param[in] duration The timeout value to be used\n> + *\n> + * Sets a timeout value, given by \\a duration, that will be used by a timer to\n> + * ensure buffer dequeue events are periodically occurring. If the timer\n> + * expires, a slot in the pipeline handler may be optionally called to handle\n> + * this condition through the \\a dequeueTimeout signal.\n\nThis should be \"\\ref V4L2VideoDevice::dequeueTimeout\" to generate a\nhyperlink. \\a is for function parameters, and only serves to emphesize\nthem.\n\nIt boils down to the same thing in the end, but from the point of view\nof the V4L2VideoDevice, we're emitting a signal, not calling into the\npipeline handler. The signal could be connected to anything. You can\nwrite\n\n\"If the timer expires, the \\ref V4L2VideoDevice::dequeueTimeout signal\nis emitted. This can typically be used by pipeline handlers to be\nnotified of stalled devices.\"\n\nIt would also be nice to expand this to tell that stall detection only\noccurs when the device is streaming (that is, it is safe to set the\ntimeout at initialization time and not set it back to 0 when stopping\nstreaming).\n\n> + *\n> + * Set \\a duration to 0 to disable the timeout.\n> + *\n> + */\n> +void V4L2VideoDevice::setDequeueTimeout(utils::Duration duration)\n> +{\n> +\ttimerDuration_ = duration;\n> +\n> +\ttimer_.stop();\n> +\tif (duration.count() && streaming_)\n> +\t\ttimer_.start(duration);\n> +}\n> +\n> +/**\n> + * \\brief Slot to handle an expired dequeue timer.\n> + *\n> + * When this slot is called, the the time between successive dequeue events\n\ns/the the/the/\n\n> + * is over the required timeout. Optionally call a slot in the pipeline handler\n> + * given by the \\a dequeueTimeout signal.\n\nAnd here, just \"Emit the dequeueTimeout signal.\" as it's internal\ndocumentation, for a private function.\n\n> + */\n> +void V4L2VideoDevice::timerExpired()\n> +{\n> +\tLOG(V4L2, Info)\n\nShouldn't this be at least a Warn ?\n\n> +\t\t<< \"Dequeue timer of \" << timerDuration_\n> +\t\t<< \" has expired!\";\n> +\n> +\tdequeueTimeout.emit(this);\n> +}\n> +\n> /**\n> * \\brief Create a new video device instance from \\a entity in media device\n> * \\a media","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 47706BD80A\n\tfor ;\n\tTue, 22 Mar 2022 21:47:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FC7A604DB;\n\tTue, 22 Mar 2022 22:47:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD135604C5\n\tfor ;\n\tTue, 22 Mar 2022 22:47:42 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2BDD7DFA;\n\tTue, 22 Mar 2022 22:47:42 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647985664;\n\tbh=vYucz48OT0hzlh8r4WVMnfxUj6wE/6zJ+Z27KH49N+k=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=0HfuM9HLWAoatgQvUhOdTSKWeBzKdZ15HlMIZC3emDKVx4Z3D689pUotg1N6W7bw9\n\tfM0QsWyz352dVXFFFX1V4Huqd9IZ4BjlnU5MM39if29bstRNcNVmQZMf6r2rxZH6Kc\n\teXgCeXzEzH+UEZhv41d/j8W2q1yZfSuyr4nlD3IXU2lyr49rT2l1THIIhu61BVP/fM\n\t/q0/ihvq9gAHQPG6dQWdbVVz7BHEqEQ8yFhJHYGKVqYFbGqULBtJTo+vVkFDBLQOOK\n\tyJDKMkIw6SdxPoNrBfSBFSeu3wM8p7naWA87ERuq6imSFrNZAMcJBVVP+7/f8WTZio\n\tFj3frgkDhnYxw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647985662;\n\tbh=vYucz48OT0hzlh8r4WVMnfxUj6wE/6zJ+Z27KH49N+k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v7yDF1zIgRA4XWSjCyCNhxA1PvkD2rQmM7uRXdhw8saXL0MGbgwhz6mHDyP5/dLQv\n\thCjp/7meeQo7ok/Y1xtaTepTO3XjoznmJxVsEakNF4F7e+595jTATIvK/y/eCe/Pql\n\twpoBbdJ78DKcaBYVc/y9SYe4CxIRl/mIsLrgyhDs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"v7yDF1zI\"; dkim-atps=neutral","Date":"Tue, 22 Mar 2022 23:47:25 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220322131635.2869526-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22387,"web_url":"https://patchwork.libcamera.org/comment/22387/","msgid":"","date":"2022-03-23T09:11:48","subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 22 Mar 2022 at 21:47, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 22, 2022 at 01:16:34PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > Add a timer that gets reset on every buffer dequeue event. If the timeout\n> > expires, optionally call a slot in the pipeline handler to handle this\n> > condition. This may be useful in detecting and handling stalls in either\n> the\n> > hardware or device driver.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 9 ++++\n> > src/libcamera/v4l2_videodevice.cpp | 47 +++++++++++++++++++\n> > 2 files changed, 56 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h\n> b/include/libcamera/internal/v4l2_videodevice.h\n> > index 2d2ccc477c91..dd6e96438637 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -20,6 +20,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -216,6 +217,9 @@ public:\n> > int streamOn();\n> > int streamOff();\n> >\n> > + void setDequeueTimeout(utils::Duration duration);\n> > + Signal dequeueTimeout;\n>\n> We stopped passing the pointer to the emitter object when emitting a\n> signal, so this should be just Signal<> dequeueTimeout;\n>\n\nThe intention here was to pass the V4L2VideoDevice instance to the\npipeline handler's slot callback. This way, the pipeline handler can have\na single slot for all devices it wants to monitor, and can distinguish which\ndevice timed out. If you think that may not be necessary, I will remove it.\n\n\n>\n> > +\n> > static std::unique_ptr\n> > fromEntityName(const MediaDevice *media, const std::string\n> &entity);\n> >\n> > @@ -246,6 +250,8 @@ private:\n> > void bufferAvailable();\n> > FrameBuffer *dequeueBuffer();\n> >\n> > + void timerExpired();\n> > +\n> > V4L2Capability caps_;\n> > V4L2DeviceFormat format_;\n> > const PixelFormatInfo *formatInfo_;\n> > @@ -259,6 +265,9 @@ private:\n> > EventNotifier *fdBufferNotifier_;\n> >\n> > bool streaming_;\n> > +\n> > + Timer timer_;\n> > + utils::Duration timerDuration_;\n> > };\n> >\n> > class V4L2M2MDevice\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp\n> b/src/libcamera/v4l2_videodevice.cpp\n> > index 5f36ee20710d..25bce5475e07 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -600,6 +600,8 @@ int V4L2VideoDevice::open()\n> > fdBufferNotifier_->activated.connect(this,\n> &V4L2VideoDevice::bufferAvailable);\n> > fdBufferNotifier_->setEnabled(false);\n> >\n> > + timer_.timeout.connect(this, &V4L2VideoDevice::timerExpired);\n>\n> You should do this in the constructor, or you'll end up with multiple\n> connections if the device is closed and reopened.\n>\n\nAck. We just fixed a problem with exactly this for Requests :)\n\n\n>\n> > +\n> > LOG(V4L2, Debug)\n> > << \"Opened device \" << caps_.bus_info() << \": \"\n> > << caps_.driver() << \": \" << caps_.card();\n> > @@ -1695,6 +1697,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > return nullptr;\n> > }\n> >\n> > + if (timerDuration_.count())\n> > + timer_.start(timerDuration_);\n> > +\n> > cache_->put(buf.index);\n> >\n> > FrameBuffer *buffer = it->second;\n> > @@ -1797,6 +1802,8 @@ int V4L2VideoDevice::streamOn()\n> > }\n> >\n> > streaming_ = true;\n> > + if (timerDuration_.count())\n> > + timer_.start(timerDuration_);\n> >\n> > return 0;\n> > }\n> > @@ -1821,6 +1828,9 @@ int V4L2VideoDevice::streamOff()\n> > if (!streaming_ && queuedBuffers_.empty())\n> > return 0;\n> >\n> > + if (timerDuration_.count())\n> > + timer_.stop();\n> > +\n> > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > if (ret < 0) {\n> > LOG(V4L2, Error)\n> > @@ -1843,6 +1853,43 @@ int V4L2VideoDevice::streamOff()\n> > return 0;\n> > }\n> >\n> > +/**\n> > + * \\brief Set the dequeue timeout value\n> > + * \\param[in] duration The timeout value to be used\n> > + *\n> > + * Sets a timeout value, given by \\a duration, that will be used by a\n> timer to\n> > + * ensure buffer dequeue events are periodically occurring. If the timer\n> > + * expires, a slot in the pipeline handler may be optionally called to\n> handle\n> > + * this condition through the \\a dequeueTimeout signal.\n>\n> This should be \"\\ref V4L2VideoDevice::dequeueTimeout\" to generate a\n> hyperlink. \\a is for function parameters, and only serves to emphesize\n> them.\n>\n> It boils down to the same thing in the end, but from the point of view\n> of the V4L2VideoDevice, we're emitting a signal, not calling into the\n> pipeline handler. The signal could be connected to anything. You can\n> write\n>\n> \"If the timer expires, the \\ref V4L2VideoDevice::dequeueTimeout signal\n> is emitted. This can typically be used by pipeline handlers to be\n> notified of stalled devices.\"\n>\n> It would also be nice to expand this to tell that stall detection only\n> occurs when the device is streaming (that is, it is safe to set the\n> timeout at initialization time and not set it back to 0 when stopping\n> streaming).\n>\n\nSure, I'll reword it to be more descriptive.\n\nRegards,\nNaush\n\n\n>\n> > + *\n> > + * Set \\a duration to 0 to disable the timeout.\n> > + *\n> > + */\n> > +void V4L2VideoDevice::setDequeueTimeout(utils::Duration duration)\n> > +{\n> > + timerDuration_ = duration;\n> > +\n> > + timer_.stop();\n> > + if (duration.count() && streaming_)\n> > + timer_.start(duration);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Slot to handle an expired dequeue timer.\n> > + *\n> > + * When this slot is called, the the time between successive dequeue\n> events\n>\n> s/the the/the/\n>\n> > + * is over the required timeout. Optionally call a slot in the pipeline\n> handler\n> > + * given by the \\a dequeueTimeout signal.\n>\n> And here, just \"Emit the dequeueTimeout signal.\" as it's internal\n> documentation, for a private function.\n>\n> > + */\n> > +void V4L2VideoDevice::timerExpired()\n> > +{\n> > + LOG(V4L2, Info)\n>\n> Shouldn't this be at least a Warn ?\n\n\n> > + << \"Dequeue timer of \" << timerDuration_\n> > + << \" has expired!\";\n> > +\n> > + dequeueTimeout.emit(this);\n> > +}\n> > +\n> > /**\n> > * \\brief Create a new video device instance from \\a entity in media\n> device\n> > * \\a media\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B5A3BBD80A\n\tfor ;\n\tWed, 23 Mar 2022 09:12:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2887B604DC;\n\tWed, 23 Mar 2022 10:12:06 +0100 (CET)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E18A601F8\n\tfor ;\n\tWed, 23 Mar 2022 10:12:05 +0100 (CET)","by mail-lj1-x233.google.com with SMTP id 17so946543ljw.8\n\tfor ;\n\tWed, 23 Mar 2022 02:12:05 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648026726;\n\tbh=leZEsbeqm5LGfprU7wBFonx2nmMH/wL5vjtXcOGZWRE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=pbReLKMVaOOjLxeD8AfXUqrGc9vJiPebxWWtwKdEVSRUcaX09jc+h4QQhHsUGHDas\n\tivM+bJf5mtwH5dxI/k7TzjEpB+m5dBMFWaaJUb325vZDv/Mr1ji/EGgMXj851q2O3O\n\tc1Bi8qsckrXyg82w5hXInBu16QqtjkkKUdGD/UZ6Z8ozJpic7b2rM2G9rwUP4iRx9R\n\t/i5sqYm1+8HUoBL2hMFG1eWL1qPKCgS9UX5hLFehxsXDABtAFrFBNRIk7n1+hw+qqL\n\tdNJWL2M2a8KUNLtjgfmsatPh3KLU7ARvE7FHN2jknxH1CoP9C647YEEytaC/L+Oc1+\n\tNPk7t1kCKV7Zg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=tlBHpFPZIh7hrocpV/xGWsSzAhmaXUeyE75VRXo7mK4=;\n\tb=VPr6v1Ma/mD+KgK2BfUWa+S0GnNwXWK6c9PMQFYgkTXorQVDQJXqWw76HI2UOEDgRO\n\t2YImS2zkQDQRL7mXxVjXXnhca+N38WLPEKULscHGbKDpbM7hpVPuTQ6xlRJLhhGSsxzz\n\t5gxQn6r0OG79+wdKeArrndB87OaEYwJE1IunovcqsPbqaq5sppA2kYO+0XJVFENpxx6m\n\tinWXmiBwbVASElh7grbKwwcOaELB6gPhnTYm5yarnlOwiXp2V00ldTPaoOVlgWKjGjvg\n\tR95JFNLooA19lUpOh83svuLqCgmcYDErFPnAYFsyMl1aRl3lEdKGcn9TVXiVeDWtK16i\n\tnrDQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"VPr6v1Ma\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=tlBHpFPZIh7hrocpV/xGWsSzAhmaXUeyE75VRXo7mK4=;\n\tb=QH2HCpVVC7U8VvTy0jPDX175otGUy3vi+diqCIll7xuSrm9Y06onJaVSwe+9RUtgps\n\tbszk8sz3nBKkPbTeaWeaHP//nm/kjA1pqBngqtKYvuUviRLN+tKRugqqE0PnTrbgBTvp\n\tmMu8W05WhvJmSU0U22fcOFgFM8R9zqnBh89wLyVq4R8tW/mI9yiosITUr11ZEaQSWed9\n\tv07pg/ZByiqBXLZ8sQdv0smqfYtfX/p/XkKoimQujcxz7JLGNlSWoEqzMhng2KB37lz9\n\t3QPeYRDcFBmkbw73EFl2CYBKclDjXyGp3cARgJEgcQgpD9Ycm7kTDMaUmWWNTuVjUZPf\n\tA6hg==","X-Gm-Message-State":"AOAM532y25OpXXhg/HJSjH+lUO2iuum7IZoHxeDVgnu5vo6WHa3wYpZp\n\ttB3hpV54zDjbnLlhNJeolk29NjlD6h/tRAMQdrMyOg==","X-Google-Smtp-Source":"ABdhPJyrA5DGOQb9QgelDG0PP47gWoRQBZCEHxYmkC0ojd+VlToK9/JlFqBMW5Y1egsBBkjszsYUjicMG161hAsEjnM=","X-Received":"by 2002:a2e:bf25:0:b0:247:d216:43fc with SMTP id\n\tc37-20020a2ebf25000000b00247d21643fcmr22578220ljr.520.1648026724503;\n\tWed, 23 Mar 2022 02:12:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Wed, 23 Mar 2022 09:11:48 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000007ea96205dadf1fde\"","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22395,"web_url":"https://patchwork.libcamera.org/comment/22395/","msgid":"<164803067410.2130830.3515514940735622524@Monstersaurus>","date":"2022-03-23T10:17:54","subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:11:48)\n> Hi Laurent,\n> \n> Thank you for your feedback.\n> \n> On Tue, 22 Mar 2022 at 21:47, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n> \n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Mar 22, 2022 at 01:16:34PM +0000, Naushir Patuck via\n> > libcamera-devel wrote:\n> > > Add a timer that gets reset on every buffer dequeue event. If the timeout\n> > > expires, optionally call a slot in the pipeline handler to handle this\n> > > condition. This may be useful in detecting and handling stalls in either\n> > the\n> > > hardware or device driver.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 9 ++++\n> > > src/libcamera/v4l2_videodevice.cpp | 47 +++++++++++++++++++\n> > > 2 files changed, 56 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h\n> > b/include/libcamera/internal/v4l2_videodevice.h\n> > > index 2d2ccc477c91..dd6e96438637 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -20,6 +20,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -216,6 +217,9 @@ public:\n> > > int streamOn();\n> > > int streamOff();\n> > >\n> > > + void setDequeueTimeout(utils::Duration duration);\n> > > + Signal dequeueTimeout;\n> >\n> > We stopped passing the pointer to the emitter object when emitting a\n> > signal, so this should be just Signal<> dequeueTimeout;\n> >\n> \n> The intention here was to pass the V4L2VideoDevice instance to the\n> pipeline handler's slot callback. This way, the pipeline handler can have\n> a single slot for all devices it wants to monitor, and can distinguish which\n> device timed out. If you think that may not be necessary, I will remove it.\n> \n\nFor this use case, being able to report which device has stalled could\nbe useful, though I imagine it will already be done by the\nV4L2VideoDevice itself before emitting this signal so it will already be\nidentifiable?\n\n\n> >\n> > > +\n> > > static std::unique_ptr\n> > > fromEntityName(const MediaDevice *media, const std::string\n> > &entity);\n> > >\n> > > @@ -246,6 +250,8 @@ private:\n> > > void bufferAvailable();\n> > > FrameBuffer *dequeueBuffer();\n> > >\n> > > + void timerExpired();\n> > > +\n> > > V4L2Capability caps_;\n> > > V4L2DeviceFormat format_;\n> > > const PixelFormatInfo *formatInfo_;\n> > > @@ -259,6 +265,9 @@ private:\n> > > EventNotifier *fdBufferNotifier_;\n> > >\n> > > bool streaming_;\n> > > +\n> > > + Timer timer_;\n\nGiven the Watchdog comments below - would calling this a Timer watchdog_\nmake it's purpose clearer rather than it's type?\n\n(Of course if we had a Watchdog wrapper, it would then be Watchdog\nwatchdog_, so then what would it be watching?)\n\n> > > + utils::Duration timerDuration_;\n> > > };\n> > >\n> > > class V4L2M2MDevice\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp\n> > b/src/libcamera/v4l2_videodevice.cpp\n> > > index 5f36ee20710d..25bce5475e07 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -600,6 +600,8 @@ int V4L2VideoDevice::open()\n> > > fdBufferNotifier_->activated.connect(this,\n> > &V4L2VideoDevice::bufferAvailable);\n> > > fdBufferNotifier_->setEnabled(false);\n> > >\n> > > + timer_.timeout.connect(this, &V4L2VideoDevice::timerExpired);\n> >\n> > You should do this in the constructor, or you'll end up with multiple\n> > connections if the device is closed and reopened.\n> >\n> \n> Ack. We just fixed a problem with exactly this for Requests :)\n\nI hope the assert we added would catch it then! I'd be interested if the\nunit tests would trigger it - and if not - perhaps we need a multiple\nclose/open test on a V4L2 device or something like that.\n\nBut that's probably just another yak for another day, unless it's\ninteresting to you, particularly as that would then come with a whole\nset of unit tests to make sure it barks ... and it may not be needed\nanywhere else.\n\n\n> >\n> > > +\n> > > LOG(V4L2, Debug)\n> > > << \"Opened device \" << caps_.bus_info() << \": \"\n> > > << caps_.driver() << \": \" << caps_.card();\n> > > @@ -1695,6 +1697,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > return nullptr;\n> > > }\n> > >\n> > > + if (timerDuration_.count())\n> > > + timer_.start(timerDuration_);\n> > > +\n> > > cache_->put(buf.index);\n> > >\n> > > FrameBuffer *buffer = it->second;\n> > > @@ -1797,6 +1802,8 @@ int V4L2VideoDevice::streamOn()\n> > > }\n> > >\n> > > streaming_ = true;\n> > > + if (timerDuration_.count())\n> > > + timer_.start(timerDuration_);\n> > >\n> > > return 0;\n> > > }\n> > > @@ -1821,6 +1828,9 @@ int V4L2VideoDevice::streamOff()\n> > > if (!streaming_ && queuedBuffers_.empty())\n> > > return 0;\n> > >\n> > > + if (timerDuration_.count())\n> > > + timer_.stop();\n> > > +\n> > > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > > if (ret < 0) {\n> > > LOG(V4L2, Error)\n> > > @@ -1843,6 +1853,43 @@ int V4L2VideoDevice::streamOff()\n> > > return 0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Set the dequeue timeout value\n> > > + * \\param[in] duration The timeout value to be used\n> > > + *\n> > > + * Sets a timeout value, given by \\a duration, that will be used by a\n> > timer to\n> > > + * ensure buffer dequeue events are periodically occurring. If the timer\n> > > + * expires, a slot in the pipeline handler may be optionally called to\n> > handle\n> > > + * this condition through the \\a dequeueTimeout signal.\n> >\n> > This should be \"\\ref V4L2VideoDevice::dequeueTimeout\" to generate a\n> > hyperlink. \\a is for function parameters, and only serves to emphesize\n> > them.\n> >\n> > It boils down to the same thing in the end, but from the point of view\n> > of the V4L2VideoDevice, we're emitting a signal, not calling into the\n> > pipeline handler. The signal could be connected to anything. You can\n> > write\n> >\n> > \"If the timer expires, the \\ref V4L2VideoDevice::dequeueTimeout signal\n> > is emitted. This can typically be used by pipeline handlers to be\n> > notified of stalled devices.\"\n> >\n> > It would also be nice to expand this to tell that stall detection only\n> > occurs when the device is streaming (that is, it is safe to set the\n> > timeout at initialization time and not set it back to 0 when stopping\n> > streaming).\n> >\n> \n> Sure, I'll reword it to be more descriptive.\n> \n> Regards,\n> Naush\n> \n> \n> >\n> > > + *\n> > > + * Set \\a duration to 0 to disable the timeout.\n> > > + *\n> > > + */\n> > > +void V4L2VideoDevice::setDequeueTimeout(utils::Duration duration)\n> > > +{\n> > > + timerDuration_ = duration;\n> > > +\n> > > + timer_.stop();\n> > > + if (duration.count() && streaming_)\n> > > + timer_.start(duration);\n\nA timer with a duration that gets reset is a Watchdog ... I wonder if\nthat should get wrapped into the same helpers header as Timer ... but\nthat's yet more yaks that we can shave later - I think this is fine for\nnow. (Unless you like shaving them of course).\n\n\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Slot to handle an expired dequeue timer.\n> > > + *\n> > > + * When this slot is called, the the time between successive dequeue\n> > events\n> >\n> > s/the the/the/\n> >\n> > > + * is over the required timeout. Optionally call a slot in the pipeline\n> > handler\n> > > + * given by the \\a dequeueTimeout signal.\n> >\n> > And here, just \"Emit the dequeueTimeout signal.\" as it's internal\n> > documentation, for a private function.\n> >\n> > > + */\n> > > +void V4L2VideoDevice::timerExpired()\n> > > +{\n> > > + LOG(V4L2, Info)\n> >\n> > Shouldn't this be at least a Warn ?\n> \n> \n> > > + << \"Dequeue timer of \" << timerDuration_\n> > > + << \" has expired!\";\n> > > +\n> > > + dequeueTimeout.emit(this);\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Create a new video device instance from \\a entity in media\n> > device\n> > > * \\a media\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9B2DABD80A\n\tfor ;\n\tWed, 23 Mar 2022 10:17:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 105BA604DC;\n\tWed, 23 Mar 2022 11:17:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45AAC601F8\n\tfor ;\n\tWed, 23 Mar 2022 11:17:57 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D63599DE;\n\tWed, 23 Mar 2022 11:17:56 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648030679;\n\tbh=GyVnT0y3EMxYH+lIBlnALlnCfk8t13tsMKjXhN8Dts8=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=wWMvJ2i+VD5OlQz33WN+BHWvqSxXjzOkcXKUNtgKfSTgqwmN+gtlyzWSfTpDNcC9n\n\tCKziqXJQSNVOUNlLNIJsPrNm7mh2kN5Y+ukpZ/q/jYFDS1EjE14HR/vmYD+tRutAmG\n\taz3sJLIXffdH4RGWabu492470pCReceuydrIQ4bBHRnmXK44KVVmYb6AKTX+vjH9tm\n\tc5s9gQzkYU8khE5f98/QIO+u/zFSvxF5eg57fsAbR7sD6dZ1B3vHJ5FlGdj3KSTIWl\n\t+AJVh74pcn+WrQmQdZxUPTy+ylkBqmD1/wVjviSToOP4gWLpzdDzs4fGBAPxl5aJzF\n\tidF2M1yVioqVA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648030676;\n\tbh=GyVnT0y3EMxYH+lIBlnALlnCfk8t13tsMKjXhN8Dts8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=U4NDmBpTdV2zRrn3Ryv62iuMwjUZzVHoL3OKhAuWL67+3kxdR2pxOGXFYc4DO/Lhs\n\tStFuIgoqi8weoFL5i+7Kyzu4UCuTwBUlODuaGRwTFtQMwafuJLBe3+JLKRly0eaN1h\n\tRCp9IPk2woT8dppAQsRUr9Jn0v4hqhZw6HIv0MfI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"U4NDmBpT\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-3-naush@raspberrypi.com>\n\t\n\t","To":"Laurent Pinchart ,\n\tNaushir Patuck ,\n\tNaushir Patuck via libcamera-devel ","Date":"Wed, 23 Mar 2022 10:17:54 +0000","Message-ID":"<164803067410.2130830.3515514940735622524@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22400,"web_url":"https://patchwork.libcamera.org/comment/22400/","msgid":"","date":"2022-03-23T10:47:48","subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Mar 23, 2022 at 10:17:54AM +0000, Kieran Bingham wrote:\n> Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:11:48)\n> > On Tue, 22 Mar 2022 at 21:47, Laurent Pinchart wrote:\n> > > On Tue, Mar 22, 2022 at 01:16:34PM +0000, Naushir Patuck via\n> > > libcamera-devel wrote:\n> > > > Add a timer that gets reset on every buffer dequeue event. If the timeout\n> > > > expires, optionally call a slot in the pipeline handler to handle this\n> > > > condition. This may be useful in detecting and handling stalls in either\n> > > the\n> > > > hardware or device driver.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > include/libcamera/internal/v4l2_videodevice.h | 9 ++++\n> > > > src/libcamera/v4l2_videodevice.cpp | 47 +++++++++++++++++++\n> > > > 2 files changed, 56 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h\n> > > b/include/libcamera/internal/v4l2_videodevice.h\n> > > > index 2d2ccc477c91..dd6e96438637 100644\n> > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > @@ -20,6 +20,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \n> > > > @@ -216,6 +217,9 @@ public:\n> > > > int streamOn();\n> > > > int streamOff();\n> > > >\n> > > > + void setDequeueTimeout(utils::Duration duration);\n> > > > + Signal dequeueTimeout;\n> > >\n> > > We stopped passing the pointer to the emitter object when emitting a\n> > > signal, so this should be just Signal<> dequeueTimeout;\n> > \n> > The intention here was to pass the V4L2VideoDevice instance to the\n> > pipeline handler's slot callback. This way, the pipeline handler can have\n> > a single slot for all devices it wants to monitor, and can distinguish which\n> > device timed out. If you think that may not be necessary, I will remove it.\n> \n> For this use case, being able to report which device has stalled could\n> be useful, though I imagine it will already be done by the\n> V4L2VideoDevice itself before emitting this signal so it will already be\n> identifiable?\n\nInstead of adding a pointer to the emitter to the signal, in case a user\nmay need it, you can connect the signal to a lambda that captures the\ncontext. See the implementation of Request::Private::prepare() for an\nexample.\n\n> > > > +\n> > > > static std::unique_ptr\n> > > > fromEntityName(const MediaDevice *media, const std::string\n> > > &entity);\n> > > >\n> > > > @@ -246,6 +250,8 @@ private:\n> > > > void bufferAvailable();\n> > > > FrameBuffer *dequeueBuffer();\n> > > >\n> > > > + void timerExpired();\n> > > > +\n> > > > V4L2Capability caps_;\n> > > > V4L2DeviceFormat format_;\n> > > > const PixelFormatInfo *formatInfo_;\n> > > > @@ -259,6 +265,9 @@ private:\n> > > > EventNotifier *fdBufferNotifier_;\n> > > >\n> > > > bool streaming_;\n> > > > +\n> > > > + Timer timer_;\n> \n> Given the Watchdog comments below - would calling this a Timer watchdog_\n> make it's purpose clearer rather than it's type?\n\nI like that.\n\n> (Of course if we had a Watchdog wrapper, it would then be Watchdog\n> watchdog_, so then what would it be watching?)\n> \n> > > > + utils::Duration timerDuration_;\n> > > > };\n> > > >\n> > > > class V4L2M2MDevice\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp\n> > > b/src/libcamera/v4l2_videodevice.cpp\n> > > > index 5f36ee20710d..25bce5475e07 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -600,6 +600,8 @@ int V4L2VideoDevice::open()\n> > > > fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> > > > fdBufferNotifier_->setEnabled(false);\n> > > >\n> > > > + timer_.timeout.connect(this, &V4L2VideoDevice::timerExpired);\n> > >\n> > > You should do this in the constructor, or you'll end up with multiple\n> > > connections if the device is closed and reopened.\n> > \n> > Ack. We just fixed a problem with exactly this for Requests :)\n> \n> I hope the assert we added would catch it then! I'd be interested if the\n> unit tests would trigger it - and if not - perhaps we need a multiple\n> close/open test on a V4L2 device or something like that.\n> \n> But that's probably just another yak for another day, unless it's\n> interesting to you, particularly as that would then come with a whole\n> set of unit tests to make sure it barks ... and it may not be needed\n> anywhere else.\n> \n> > > > +\n> > > > LOG(V4L2, Debug)\n> > > > << \"Opened device \" << caps_.bus_info() << \": \"\n> > > > << caps_.driver() << \": \" << caps_.card();\n> > > > @@ -1695,6 +1697,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > > return nullptr;\n> > > > }\n> > > >\n> > > > + if (timerDuration_.count())\n> > > > + timer_.start(timerDuration_);\n> > > > +\n> > > > cache_->put(buf.index);\n> > > >\n> > > > FrameBuffer *buffer = it->second;\n> > > > @@ -1797,6 +1802,8 @@ int V4L2VideoDevice::streamOn()\n> > > > }\n> > > >\n> > > > streaming_ = true;\n> > > > + if (timerDuration_.count())\n> > > > + timer_.start(timerDuration_);\n> > > >\n> > > > return 0;\n> > > > }\n> > > > @@ -1821,6 +1828,9 @@ int V4L2VideoDevice::streamOff()\n> > > > if (!streaming_ && queuedBuffers_.empty())\n> > > > return 0;\n> > > >\n> > > > + if (timerDuration_.count())\n> > > > + timer_.stop();\n> > > > +\n> > > > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > > > if (ret < 0) {\n> > > > LOG(V4L2, Error)\n> > > > @@ -1843,6 +1853,43 @@ int V4L2VideoDevice::streamOff()\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Set the dequeue timeout value\n> > > > + * \\param[in] duration The timeout value to be used\n> > > > + *\n> > > > + * Sets a timeout value, given by \\a duration, that will be used by a> timer to\n> > > > + * ensure buffer dequeue events are periodically occurring. If the timer\n> > > > + * expires, a slot in the pipeline handler may be optionally called to handle\n> > > > + * this condition through the \\a dequeueTimeout signal.\n> > >\n> > > This should be \"\\ref V4L2VideoDevice::dequeueTimeout\" to generate a\n> > > hyperlink. \\a is for function parameters, and only serves to emphesize\n> > > them.\n> > >\n> > > It boils down to the same thing in the end, but from the point of view\n> > > of the V4L2VideoDevice, we're emitting a signal, not calling into the\n> > > pipeline handler. The signal could be connected to anything. You can\n> > > write\n> > >\n> > > \"If the timer expires, the \\ref V4L2VideoDevice::dequeueTimeout signal\n> > > is emitted. This can typically be used by pipeline handlers to be\n> > > notified of stalled devices.\"\n> > >\n> > > It would also be nice to expand this to tell that stall detection only\n> > > occurs when the device is streaming (that is, it is safe to set the\n> > > timeout at initialization time and not set it back to 0 when stopping\n> > > streaming).\n> > \n> > Sure, I'll reword it to be more descriptive.\n> > \n> > > > + *\n> > > > + * Set \\a duration to 0 to disable the timeout.\n> > > > + *\n> > > > + */\n> > > > +void V4L2VideoDevice::setDequeueTimeout(utils::Duration duration)\n> > > > +{\n> > > > + timerDuration_ = duration;\n> > > > +\n> > > > + timer_.stop();\n> > > > + if (duration.count() && streaming_)\n> > > > + timer_.start(duration);\n> \n> A timer with a duration that gets reset is a Watchdog ... I wonder if\n> that should get wrapped into the same helpers header as Timer ... but\n> that's yet more yaks that we can shave later - I think this is fine for\n> now. (Unless you like shaving them of course).\n> \n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Slot to handle an expired dequeue timer.\n> > > > + *\n> > > > + * When this slot is called, the the time between successive dequeue events\n> > >\n> > > s/the the/the/\n> > >\n> > > > + * is over the required timeout. Optionally call a slot in the pipeline handler\n> > > > + * given by the \\a dequeueTimeout signal.\n> > >\n> > > And here, just \"Emit the dequeueTimeout signal.\" as it's internal\n> > > documentation, for a private function.\n> > >\n> > > > + */\n> > > > +void V4L2VideoDevice::timerExpired()\n> > > > +{\n> > > > + LOG(V4L2, Info)\n> > >\n> > > Shouldn't this be at least a Warn ?\n> > \n> > \n> > > > + << \"Dequeue timer of \" << timerDuration_\n> > > > + << \" has expired!\";\n> > > > +\n> > > > + dequeueTimeout.emit(this);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Create a new video device instance from \\a entity in media device\n> > > > * \\a media","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D7EF5C0F1B\n\tfor ;\n\tWed, 23 Mar 2022 10:48:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30839604C6;\n\tWed, 23 Mar 2022 11:48:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 29152601F8\n\tfor ;\n\tWed, 23 Mar 2022 11:48:06 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9E0689DE;\n\tWed, 23 Mar 2022 11:48:05 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648032488;\n\tbh=+dBLjpLF0WP3Zih6yDHlYhU66idm2IDPzRwfnepv+jY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yhyb4si5wQEU8wrzcZqdIj0k3ejNfUT4eW835BKPtR3W+uZhqGvzv8vQFYA0YWti+\n\trwZTlMvwZFXxa9+5LIavN1IMzBmXA9yt43QhQSrr1hVUbcfPn+WJG+lgavd3b+hh4v\n\tsWOhBrZXEmn0D1ZvqBz0zKk54cP67JOjitih8p2K5RaNqv08maAd7wLH95g9ERTRd3\n\t3qfsSyKXVwaqqraVOa+mSkzRlFffb1y6nHN0jhF0GLm2q/8KZhfkaMdZy7Qm9uPAe6\n\txOt3BkbJ48OHhk3+W95SOeFxNB/tLqzbF8+rCK5mpQ/VDPiAUiFMQ5IaCdPvxyQ9Ky\n\tTmZgOz6iokHqw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648032485;\n\tbh=+dBLjpLF0WP3Zih6yDHlYhU66idm2IDPzRwfnepv+jY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gYZYSd/KH4hfuhXjAKGN5lzVB2DDTWTY7Iphv20x1z0qLaoNCFA/sQ6LzEo3ocx/2\n\twd2stZ5Z7+CtUibceCvDS28Z/PSkyMDWIX6sF+NRYN/EichudYuDd9T7DKz9mPo50E\n\t2YMmGHTavVRQNLYfgboJ0O7QYBCjiSzZUVKQSfWs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gYZYSd/K\"; dkim-atps=neutral","Date":"Wed, 23 Mar 2022 12:47:48 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-3-naush@raspberrypi.com>\n\t\n\t\n\t<164803067410.2130830.3515514940735622524@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164803067410.2130830.3515514940735622524@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] libcamera: v4l2_videodevice:\n\tAdd a dequeue timer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Naushir Patuck via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]