[{"id":22358,"web_url":"https://patchwork.libcamera.org/comment/22358/","msgid":"","date":"2022-03-22T20:08:19","subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","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 09:22:54AM +0000, Naushir Patuck via libcamera-devel wrote:\n> Replace the existing streaming_ state variable with an enum to track the\n> following three state: Streaming, Stopping, and Stopped. The alternate states\n> will be used in a subsequent commit.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 3 ++-\n> src/libcamera/v4l2_videodevice.cpp | 10 ++++++----\n> 2 files changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 2d2ccc477c91..32e022543385 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -258,7 +258,8 @@ private:\n> \n> \tEventNotifier *fdBufferNotifier_;\n> \n> -\tbool streaming_;\n> +\tenum class State { Streaming, Stopping, Stopped };\n\n\tenum class State {\n\t\tStreaming,\n\t\tStopping,\n\t\tStopped,\n\t};\n\nand this should go right after LIBCAMERA_DISABLE_COPY() as we declare\ntypes before functions and variables.\n\n> +\tState state_;\n> };\n> \n> class V4L2M2MDevice\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 5f36ee20710d..9cea6a608660 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const\n> */\n> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> \t: V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> -\t fdBufferNotifier_(nullptr), streaming_(false)\n> +\t fdBufferNotifier_(nullptr), state_(State::Stopped)\n> {\n> \t/*\n> \t * We default to an MMAP based CAPTURE video device, however this will\n> @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()\n> \t\treturn ret;\n> \t}\n> \n> -\tstreaming_ = true;\n> +\tstate_ = State::Streaming;\n> \n> \treturn 0;\n> }\n> @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()\n> {\n> \tint ret;\n> \n> -\tif (!streaming_ && queuedBuffers_.empty())\n> +\tif (state_ != State::Streaming && queuedBuffers_.empty())\n> \t\treturn 0;\n> \n> \tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()\n> \t\treturn ret;\n> \t}\n> \n> +\tstate_ = State::Stopping;\n> +\n> \t/* Send back all queued buffers. */\n> \tfor (auto it : queuedBuffers_) {\n> \t\tFrameBuffer *buffer = it.second;\n> @@ -1838,7 +1840,7 @@ int V4L2VideoDevice::streamOff()\n> \n> \tqueuedBuffers_.clear();\n> \tfdBufferNotifier_->setEnabled(false);\n> -\tstreaming_ = false;\n> +\tstate_ = State::Stopped;\n\nThis looks fine, but may depend on how it's then used by the next patch.\nProvided that's fine too, with the above change,\n\nReviewed-by: Laurent Pinchart \n\n> \n> \treturn 0;\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 77E7EBD80A\n\tfor ;\n\tTue, 22 Mar 2022 20:08:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7A26604DB;\n\tTue, 22 Mar 2022 21:08:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C570A604C5\n\tfor ;\n\tTue, 22 Mar 2022 21:08:36 +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 3EA80DFA;\n\tTue, 22 Mar 2022 21:08:36 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647979717;\n\tbh=psB7QBHxlN8NeCwq7k5qqHD49ZrkaZTVK6DZfI4DdRI=;\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=qNXNW75qVvWrogUteAMirYsBNDsn7a2mZGFQL+EuPTmJeOmN4yrwHRS7vZZkQiIKs\n\tixva4E1T/SL4IR8nXUsbROVJgNO/p6lDkQ0KmXEFTzdKdD99e9Jc994r/gD36cdJd6\n\thXAHbfMTuFC4BzFhOggs5kGA19lZWZhai72YH1xSLNsdED5u3A8K42gE6sqrTXATp5\n\tGF7Zawlr1KJqEzCcKMrlPLNjQ4B1El1VQmsbF0QHIyuYvxQeyY4j8Ab8qc9XOMOcDt\n\tv81JiVmlzRBr0YmG0JoEgZQy3LzS8ScO7VBbuHj5liQhLDZEXbku61H4yHjxuYMUPB\n\txI/kMvTM1MWsg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647979716;\n\tbh=psB7QBHxlN8NeCwq7k5qqHD49ZrkaZTVK6DZfI4DdRI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hDiIh53AY7iYqcTyDphiNuO2bQd6UYTbGUrP9gkLmFNYAX3oHQ6UgN1hQUMaCg3Bc\n\tcq8baeOfAecrBtrY2cZFmVx07icR2Glnlp0ofnf0eIhduns7bfaOk+8M+LZPVdc3ku\n\tbpFY9M/fW9PpkkXh/7QR4rhGnAetUb95RCKl5Lig="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hDiIh53A\"; dkim-atps=neutral","Date":"Tue, 22 Mar 2022 22:08:19 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20220322092257.2713521-1-naush@raspberrypi.com>\n\t<20220322092257.2713521-6-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220322092257.2713521-6-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","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":22392,"web_url":"https://patchwork.libcamera.org/comment/22392/","msgid":"<164802954215.2130830.3160044196190880871@Monstersaurus>","date":"2022-03-23T09:59:02","subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","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-22 09:22:54)\n> Replace the existing streaming_ state variable with an enum to track the\n> following three state: Streaming, Stopping, and Stopped. The alternate states\n> will be used in a subsequent commit.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 3 ++-\n> src/libcamera/v4l2_videodevice.cpp | 10 ++++++----\n> 2 files changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 2d2ccc477c91..32e022543385 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -258,7 +258,8 @@ private:\n> \n> EventNotifier *fdBufferNotifier_;\n> \n> - bool streaming_;\n> + enum class State { Streaming, Stopping, Stopped };\n> + State state_;\n> };\n> \n> class V4L2M2MDevice\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 5f36ee20710d..9cea6a608660 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const\n> */\n> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> - fdBufferNotifier_(nullptr), streaming_(false)\n> + fdBufferNotifier_(nullptr), state_(State::Stopped)\n> {\n> /*\n> * We default to an MMAP based CAPTURE video device, however this will\n> @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()\n> return ret;\n> }\n> \n> - streaming_ = true;\n> + state_ = State::Streaming;\n> \n> return 0;\n> }\n> @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()\n> {\n> int ret;\n> \n> - if (!streaming_ && queuedBuffers_.empty())\n> + if (state_ != State::Streaming && queuedBuffers_.empty())\n> return 0;\n> \n> ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()\n> return ret;\n> }\n> \n> + state_ = State::Stopping;\n> +\n\nShould this be before the call for VIDIOC_STREAMOFF?\nIt may not make a difference in practice, or it might cause\nV4L2VideoDevice to reject a buffer (/request) that is queued in parallel\n... but I think we already know the request can be queued while calling\nstreamOff ... so I 'think' it's safe to change the state before calling\nthe ioctl...\n\nWhich ever way you're happier with, or consensus goes to:\n\nReviewed-by: Kieran Bingham \n\n> /* Send back all queued buffers. */\n> for (auto it : queuedBuffers_) {\n> FrameBuffer *buffer = it.second;\n> @@ -1838,7 +1840,7 @@ int V4L2VideoDevice::streamOff()\n> \n> queuedBuffers_.clear();\n> fdBufferNotifier_->setEnabled(false);\n> - streaming_ = false;\n> + state_ = State::Stopped;\n> \n> return 0;\n> }\n> -- \n> 2.25.1\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 E4867BD80A\n\tfor ;\n\tWed, 23 Mar 2022 09:59:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52538604D5;\n\tWed, 23 Mar 2022 10:59:07 +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 32402601F8\n\tfor ;\n\tWed, 23 Mar 2022 10:59:05 +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 C55269DE;\n\tWed, 23 Mar 2022 10:59:04 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648029547;\n\tbh=mrCaKygFQAt8/LShFMhjzgrntXTLadUVEqTEnFJ/uOE=;\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:\n\tFrom;\n\tb=l40YeT5BKhtlj+PJH3YVHGeZJIOywzhTtlYA/JuBd/QcKpF0BYeh+fYspifDqPMG9\n\t+o2DET4RUwlYed5RP9S/kqt7GkAql9A/F8iKXvwKz7b6NE8SXS+nuu/ylMZ7BJGkaD\n\t013JSzzUNW/LHdC0P0FnDJoBp3ZSBNpErC9G4KZys1FMinQxrFrW0QQEjd7A91iujY\n\ttlH19r5f4OSwyVYi4OMtbXppdf8k8D+SChIaAqOrhr6DNo+3xlu9li8UDwpioANBac\n\tYAVoYmXATAznxJHEsM4sV2ylOgN45q/7u5jQUxKkZjAjyepJ0Nt0/3h398UcAcXKUW\n\tJsmQgNsW2VbSw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648029544;\n\tbh=mrCaKygFQAt8/LShFMhjzgrntXTLadUVEqTEnFJ/uOE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=T41nXf3BvAchYB5vVcLe5PXp+6EeT2wEVfjZUSFlVh+INbXGR2uWy7G+pg0iTyDZ+\n\t5bvlSyr61emnGwxf50UsP3zeuRqyTmRml80Ezpvc+2WW4V7lxMva/CP0F4ahElZaNI\n\teRfpgFRHxVM0EPeGF3Dkm6rvTWjjqp+VS4a2d1i4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T41nXf3B\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220322092257.2713521-6-naush@raspberrypi.com>","References":"<20220322092257.2713521-1-naush@raspberrypi.com>\n\t<20220322092257.2713521-6-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 23 Mar 2022 09:59:02 +0000","Message-ID":"<164802954215.2130830.3160044196190880871@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22394,"web_url":"https://patchwork.libcamera.org/comment/22394/","msgid":"","date":"2022-03-23T10:05:02","subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","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 09:59:02AM +0000, Kieran Bingham via libcamera-devel wrote:\n> Quoting Naushir Patuck via libcamera-devel (2022-03-22 09:22:54)\n> > Replace the existing streaming_ state variable with an enum to track the\n> > following three state: Streaming, Stopping, and Stopped. The alternate states\n> > will be used in a subsequent commit.\n> > \n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 3 ++-\n> > src/libcamera/v4l2_videodevice.cpp | 10 ++++++----\n> > 2 files changed, 8 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index 2d2ccc477c91..32e022543385 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -258,7 +258,8 @@ private:\n> > \n> > EventNotifier *fdBufferNotifier_;\n> > \n> > - bool streaming_;\n> > + enum class State { Streaming, Stopping, Stopped };\n> > + State state_;\n> > };\n> > \n> > class V4L2M2MDevice\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 5f36ee20710d..9cea6a608660 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const\n> > */\n> > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> > - fdBufferNotifier_(nullptr), streaming_(false)\n> > + fdBufferNotifier_(nullptr), state_(State::Stopped)\n> > {\n> > /*\n> > * We default to an MMAP based CAPTURE video device, however this will\n> > @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()\n> > return ret;\n> > }\n> > \n> > - streaming_ = true;\n> > + state_ = State::Streaming;\n> > \n> > return 0;\n> > }\n> > @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()\n> > {\n> > int ret;\n> > \n> > - if (!streaming_ && queuedBuffers_.empty())\n> > + if (state_ != State::Streaming && queuedBuffers_.empty())\n> > return 0;\n> > \n> > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()\n> > return ret;\n> > }\n> > \n> > + state_ = State::Stopping;\n> > +\n> \n> Should this be before the call for VIDIOC_STREAMOFF?\n> It may not make a difference in practice, or it might cause\n> V4L2VideoDevice to reject a buffer (/request) that is queued in parallel\n> ... but I think we already know the request can be queued while calling\n> streamOff ... so I 'think' it's safe to change the state before calling\n> the ioctl...\n\nAs the V4L2VideoDevice class isn't thread-safe, it will indeed make no\ndifference. I'd rather keep it here, or the state will stay Stopping\nforever if VIDIOC_STREAMOFF fails.\n\n> Which ever way you're happier with, or consensus goes to:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > /* Send back all queued buffers. */\n> > for (auto it : queuedBuffers_) {\n> > FrameBuffer *buffer = it.second;\n> > @@ -1838,7 +1840,7 @@ int V4L2VideoDevice::streamOff()\n> > \n> > queuedBuffers_.clear();\n> > fdBufferNotifier_->setEnabled(false);\n> > - streaming_ = false;\n> > + state_ = State::Stopped;\n> > \n> > return 0;\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 ED351C0F1B\n\tfor ;\n\tWed, 23 Mar 2022 10:06:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B28D8604C6;\n\tWed, 23 Mar 2022 11:05:22 +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 BE87D601F8\n\tfor ;\n\tWed, 23 Mar 2022 11:05:20 +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 32EF4FFD;\n\tWed, 23 Mar 2022 11:05:20 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648029922;\n\tbh=8p6RcLDZhMLLE9/eRdRUwCK4BYWoI4XjZomQ39EOguk=;\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=ZvihWIYA2dz8zJ1ndX32i4u49OU/rSG200yqUl/2A99ozw/XUFkbm4PzbU+r4a7me\n\tvDAYERSoBfL84txy2UTNWCUpqHjP0U+S9NZidJ9xKKpV1agQC2VvKPgBhMka6N1v/y\n\tSGv8w8WCABRZO/sY++gp+JH4w9mXTg+flQFGkuigWWiD0omIVOdhTQliPEbf0o5n52\n\tSyyuMLj16LzOjimmTFgFVtqhTZ0oeWLYMxE81VyBNrUgWrdAJq+ww0Jqn29NANusF7\n\tF2aTNCNrGEq6z6VHSKx+QfB0iyF5u1KViL7yRWxzjROY58m/7fl38pEX441FC9rE6G\n\tgA7Hx++TlodhA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648029920;\n\tbh=8p6RcLDZhMLLE9/eRdRUwCK4BYWoI4XjZomQ39EOguk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h81paX/aAyw1T78wX8Gfj+Sp2Ly8jrk/LKz6gyL5fof4N0f3EOtNzeyuTOIBXXMw9\n\twUrYlSQ2O/yVlW71Z5khEeqqv+/TDSCfVwLIEw/sf8eoh7sajmsg5LEj6zgZKAM7l2\n\tU+UrcOF8E5f/Nv1Nh3l37Nbw82Y8H9RhquUvdpyk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"h81paX/a\"; dkim-atps=neutral","Date":"Wed, 23 Mar 2022 12:05:02 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20220322092257.2713521-1-naush@raspberrypi.com>\n\t<20220322092257.2713521-6-naush@raspberrypi.com>\n\t<164802954215.2130830.3160044196190880871@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164802954215.2130830.3160044196190880871@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","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":22396,"web_url":"https://patchwork.libcamera.org/comment/22396/","msgid":"<164803134630.2130830.8309016603258109727@Monstersaurus>","date":"2022-03-23T10:29:06","subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-03-23 10:05:02)\n> On Wed, Mar 23, 2022 at 09:59:02AM +0000, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2022-03-22 09:22:54)\n> > > Replace the existing streaming_ state variable with an enum to track the\n> > > following three state: Streaming, Stopping, and Stopped. The alternate states\n> > > will be used in a subsequent commit.\n> > > \n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 3 ++-\n> > > src/libcamera/v4l2_videodevice.cpp | 10 ++++++----\n> > > 2 files changed, 8 insertions(+), 5 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index 2d2ccc477c91..32e022543385 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -258,7 +258,8 @@ private:\n> > > \n> > > EventNotifier *fdBufferNotifier_;\n> > > \n> > > - bool streaming_;\n> > > + enum class State { Streaming, Stopping, Stopped };\n> > > + State state_;\n> > > };\n> > > \n> > > class V4L2M2MDevice\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 5f36ee20710d..9cea6a608660 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const\n> > > */\n> > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),\n> > > - fdBufferNotifier_(nullptr), streaming_(false)\n> > > + fdBufferNotifier_(nullptr), state_(State::Stopped)\n> > > {\n> > > /*\n> > > * We default to an MMAP based CAPTURE video device, however this will\n> > > @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()\n> > > return ret;\n> > > }\n> > > \n> > > - streaming_ = true;\n> > > + state_ = State::Streaming;\n> > > \n> > > return 0;\n> > > }\n> > > @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()\n> > > {\n> > > int ret;\n> > > \n> > > - if (!streaming_ && queuedBuffers_.empty())\n> > > + if (state_ != State::Streaming && queuedBuffers_.empty())\n> > > return 0;\n> > > \n> > > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > > @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()\n> > > return ret;\n> > > }\n> > > \n> > > + state_ = State::Stopping;\n> > > +\n> > \n> > Should this be before the call for VIDIOC_STREAMOFF?\n> > It may not make a difference in practice, or it might cause\n> > V4L2VideoDevice to reject a buffer (/request) that is queued in parallel\n> > ... but I think we already know the request can be queued while calling\n> > streamOff ... so I 'think' it's safe to change the state before calling\n> > the ioctl...\n> \n> As the V4L2VideoDevice class isn't thread-safe, it will indeed make no\n> difference. I'd rather keep it here, or the state will stay Stopping\n> forever if VIDIOC_STREAMOFF fails.\n\nThat's a good enough reason to keep it where it is indeed ;-)\n--\nKieran\n\n\n> \n> > Which ever way you're happier with, or consensus goes to:\n> > \n> > Reviewed-by: Kieran Bingham \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 67C28C0F1B\n\tfor ;\n\tWed, 23 Mar 2022 10:29:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF49E604D5;\n\tWed, 23 Mar 2022 11:29:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DE72601F8\n\tfor ;\n\tWed, 23 Mar 2022 11:29:09 +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 E4A189DE;\n\tWed, 23 Mar 2022 11:29:08 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648031350;\n\tbh=1BSHTNl0QHmeCfICcSHrqyb4j4KsmGz7gfvyTnUOnPA=;\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=XwugH6M6rP86JbWPBfsB8/t1b/9V48hntLZ/7qTTPNK8pOU+Gw43gATNibPOVFfen\n\tCu9ZiAo3bGnpGi+fILMo+snRx/ai2ohIW7ZfhMK9NChESVaiRz0/kFr0JeEcasgsRy\n\tCW1h/iXrAY1NAkmNMbwK84Dhal/P5KRXUDihOLElxtGeohpV9Y09lZi8YmxWAF1EKE\n\tvgMvqm2Emrj7T16U4i3y7UYmcVqJJj7sQZlSPUBiFMm7iS/ko27Kyul/iKN+9u5kOc\n\tZ3S4kjhZDQB79S7qFRM2yrNc/Q6NVYnlXi46gW7JPfPEfGOs+9AUeb131EdJ2kHmuA\n\thSrY3xk8QWbqw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648031349;\n\tbh=1BSHTNl0QHmeCfICcSHrqyb4j4KsmGz7gfvyTnUOnPA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=oghE9nIi5dcAyEw7/CbqzP6SRVVUUVPP78vATtrzmwSiQslX5woy6n7RBAfMP99Ar\n\t2PsMQsDhL7nS2A4RumuOEndyI+Jyl7lSSZTyKKDNYIcptEnWRD2kQ3uxdQui999KLe\n\tkjyQJGsh5ZyUO2+jyBx2RtyblTGqn8K3DCR1KvjY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oghE9nIi\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220322092257.2713521-1-naush@raspberrypi.com>\n\t<20220322092257.2713521-6-naush@raspberrypi.com>\n\t<164802954215.2130830.3160044196190880871@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Wed, 23 Mar 2022 10:29:06 +0000","Message-ID":"<164803134630.2130830.8309016603258109727@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice:\n\tBetter tracking of the device state","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]