[{"id":17011,"web_url":"https://patchwork.libcamera.org/comment/17011/","msgid":"<20210519073143.ibm2h5a4ha7iyxoi@uno.localdomain>","date":"2021-05-19T07:31:43","subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: camera: Make stop()\n\tidempotent","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\nOn Fri, May 14, 2021 at 10:16:49AM -0300, Nícolas F. R. A. Prado wrote:\n> Make Camera::stop() idempotent so that it can be called in any state and\n> consecutive times. When called in any state other than CameraRunning, it\n> is a no-op. This simplifies the cleanup path for applications.\n\nSorry for the late reply and the contradictory advices you've been\ngiven during the review of the previous iterations.\n\nI've managed to discuss this with Laurent and Kieran and I hope to be\nable to here summarize my understanding.\n\nIn a few words: please do not shortcut the camera state machine. It's\nfine to allow stop() to be called in more states than just 'Running'\n(probably on all states above 'Acquired') but that should be made part\nof the state machine.\n\nProbably adding these would be sufficient\n\n * Acquired -> Available [label = \"release()\"];\n * Acquired -> Configured [label = \"configure()\"];\n + * Acquired -> Acquired [label = \"stop()\"];\n *\n * Configured -> Available [label = \"release()\"];\n * Configured -> Configured [label = \"configure(), createRequest()\"];\n + * Configured -> Configured [label = \"stop()\"];\n * Configured -> Running [label = \"start()\"];\n *\n * Running -> Stopping [label = \"stop()\"];\n * Stopping -> Configured;\n + * Stopping -> Stopping [label = \"stop()\"];\n * Running -> Running [label = \"createRequest(), queueRequest()\"];\n\nThe motivation is that allowing stop() to be called in any state and\nreturn silently allows calls on a !acquired camera, and that\nmaintaining the camera state machine in place allows to later\nre-introduce debug statements if deemed useful for developers (ie\n\"calling stop() on an already stopped camera\").\n\nAll in all, there's a bit of work it might not be fair to require to\nget the series in, so if you don't feel like biting the bullet, a\n\\todo comment will do.\n\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> No changes in v3\n>\n> Changes in v2:\n> - Suggested by Kieran:\n> - Add new isRunning() function instead of relying on isAccessAllowed()\n> - Suggested by Laurent:\n> - Make stop()'s description clearer\n>\n> src/libcamera/camera.cpp | 20 +++++++++++++++++---\n> 1 file changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 1340c266cc5f..1c6c76c7c01e 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -347,6 +347,7 @@ public:\n> \t\tconst std::set &streams);\n> \t~Private();\n>\n> +\tbool isRunning() const;\n> \tint isAccessAllowed(State state, bool allowDisconnected = false,\n> \t\t\t const char *from = __builtin_FUNCTION()) const;\n> \tint isAccessAllowed(State low, State high,\n> @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = {\n> \t\"Running\",\n> };\n>\n> +bool Camera::Private::isRunning() const\n> +{\n> +\tState currentState = state_.load(std::memory_order_acquire);\n> +\tif (currentState == CameraRunning)\n> +\t\treturn true;\n> +\n> +\treturn false;\n> +}\n> +\n> int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,\n> \t\t\t\t const char *from) const\n> {\n> @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls)\n> * This method stops capturing and processing requests immediately. All pending\n> * requests are cancelled and complete synchronously in an error state.\n> *\n> - * \\context This function may only be called when the camera is in the Running\n> - * state as defined in \\ref camera_operation, and shall be synchronized by the\n> - * caller with other functions that affect the camera state.\n> + * \\context This function may be called in any camera state as defined in \\ref\n> + * camera_operation, and shall be synchronized by the caller with other\n> + * functions that affect the camera state. If called when the camera isn't\n> + * running, it is a no-op.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> * \\retval -ENODEV The camera has been disconnected from the system\n> @@ -1073,6 +1084,9 @@ int Camera::stop()\n> {\n> \tPrivate *const d = LIBCAMERA_D_PTR();\n>\n> +\tif (!d->isRunning())\n> +\t\treturn 0;\n> +\n> \tint ret = d->isAccessAllowed(Private::CameraRunning);\n> \tif (ret < 0)\n> \t\treturn ret;\n> --\n> 2.31.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 E5369C31FF\n\tfor ;\n\tWed, 19 May 2021 07:31:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2ECEB6891D;\n\tWed, 19 May 2021 09:31:00 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EA4D68911\n\tfor ;\n\tWed, 19 May 2021 09:30:58 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 8FC38E000A;\n\tWed, 19 May 2021 07:30:57 +0000 (UTC)"],"Date":"Wed, 19 May 2021 09:31:43 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210519073143.ibm2h5a4ha7iyxoi@uno.localdomain>","References":"<20210514131652.345486-1-nfraprado@collabora.com>\n\t<20210514131652.345486-2-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210514131652.345486-2-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: camera: Make stop()\n\tidempotent","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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]