Patch Detail
Show a patch.
GET /api/1.1/patches/2702/?format=api
{ "id": 2702, "url": "https://patchwork.libcamera.org/api/1.1/patches/2702/?format=api", "web_url": "https://patchwork.libcamera.org/patch/2702/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20200120002437.6633-16-laurent.pinchart@ideasonboard.com>", "date": "2020-01-20T00:24:33", "name": "[libcamera-devel,15/19] libcamera: camera: Centralize state checks in Private class", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "0c2b5876dedf6a0e11f812c1cb5db832d5646a4e", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/1.1/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/2702/mbox/", "series": [ { "id": 641, "url": "https://patchwork.libcamera.org/api/1.1/series/641/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=641", "date": "2020-01-20T00:24:19", "name": "Initial libcamera threading model", "version": 1, "mbox": "https://patchwork.libcamera.org/series/641/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/2702/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/2702/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C6AE607FF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jan 2020 01:24:48 +0100 (CET)", "from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CEDF529\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jan 2020 01:24:47 +0100 (CET)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579479887;\n\tbh=YwaW9Tnt9QaqLvr9FxIJGGpmvEuR0T7OyVyrG3GYQWk=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=jhhVw2Ox4EQBPbe14feXsCmu2ExeufqiiTIBenJchuB6abTsfgaE4eUx5W8oI/qUv\n\t//QS0HCAPcIPkewNqFaJieJodKGWw5eGwxWW25C++U9FT5G5Z66fR/LJOSfjtb3wxw\n\tssadATJSSoN/GIvEuwfgzJiQCZ7XcnrSLWW5wrMQ=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Mon, 20 Jan 2020 02:24:33 +0200", "Message-Id": "<20200120002437.6633-16-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.24.1", "In-Reply-To": "<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>", "References": "<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize state\n\tchecks in Private class", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Mon, 20 Jan 2020 00:24:50 -0000" }, "content": "Move all accesses to the state_ and disconnected_ members to functions\nof the Private class. This will make it easier to implement\nsynchronization, and simplifies the Camera class implementation.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n src/libcamera/camera.cpp | 163 ++++++++++++++++-------------\n src/libcamera/pipeline_handler.cpp | 5 +-\n 2 files changed, 95 insertions(+), 73 deletions(-)", "diff": "diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\nindex 6fe802f375a3..83c2249b8897 100644\n--- a/src/libcamera/camera.cpp\n+++ b/src/libcamera/camera.cpp\n@@ -266,15 +266,21 @@ public:\n \n \tPrivate(PipelineHandler *pipe, const std::string &name,\n \t\tconst std::set<Stream *> &streams);\n+\t~Private();\n \n-\tbool stateBetween(State low, State high) const;\n-\tbool stateIs(State state) const;\n+\tint isAccessAllowed(State state, bool allowDisconnected = false) const;\n+\tint isAccessAllowed(State low, State high,\n+\t\t\t bool allowDisconnected = false) const;\n+\n+\tvoid disconnect();\n+\tvoid setState(State state);\n \n \tstd::shared_ptr<PipelineHandler> pipe_;\n \tstd::string name_;\n \tstd::set<Stream *> streams_;\n \tstd::set<Stream *> activeStreams_;\n \n+private:\n \tbool disconnected_;\n \tState state_;\n };\n@@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n {\n }\n \n+Camera::Private::~Private()\n+{\n+\tif (state_ != Private::CameraAvailable)\n+\t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n+}\n+\n static constexpr std::array<const char *, 4> camera_state_names = {\n \t\"Available\",\n \t\"Acquired\",\n@@ -293,10 +305,31 @@ static constexpr std::array<const char *, 4> camera_state_names = {\n \t\"Running\",\n };\n \n-bool Camera::Private::stateBetween(State low, State high) const\n+int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const\n {\n+\tif (!allowDisconnected && disconnected_)\n+\t\treturn -ENODEV;\n+\n+\tif (state_ == state)\n+\t\treturn 0;\n+\n+\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n+\n+\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n+\t\t\t << \" state trying operation requiring state \"\n+\t\t\t << camera_state_names[state];\n+\n+\treturn -EACCES;\n+}\n+\n+int Camera::Private::isAccessAllowed(State low, State high,\n+\t\t\t\t bool allowDisconnected) const\n+{\n+\tif (!allowDisconnected && disconnected_)\n+\t\treturn -ENODEV;\n+\n \tif (state_ >= low && state_ <= high)\n-\t\treturn true;\n+\t\treturn 0;\n \n \tASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&\n \t static_cast<unsigned int>(high) < camera_state_names.size());\n@@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const\n \t\t\t << camera_state_names[low] << \" and \"\n \t\t\t << camera_state_names[high];\n \n-\treturn false;\n+\treturn -EACCES;\n }\n \n-bool Camera::Private::stateIs(State state) const\n+void Camera::Private::disconnect()\n {\n-\tif (state_ == state)\n-\t\treturn true;\n-\n-\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n+\t/*\n+\t * If the camera was running when the hardware was removed force the\n+\t * state to Configured state to allow applications to free resources\n+\t * and call release() before deleting the camera.\n+\t */\n+\tif (state_ == Private::CameraRunning)\n+\t\tstate_ = Private::CameraConfigured;\n \n-\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n-\t\t\t << \" state trying operation requiring state \"\n-\t\t\t << camera_state_names[state];\n+\tdisconnected_ = true;\n+}\n \n-\treturn false;\n+void Camera::Private::setState(State state)\n+{\n+\tstate_ = state;\n }\n \n /**\n@@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name,\n \n Camera::~Camera()\n {\n-\tif (!p_->stateIs(Private::CameraAvailable))\n-\t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n }\n \n /**\n@@ -488,23 +523,16 @@ void Camera::disconnect()\n {\n \tLOG(Camera, Debug) << \"Disconnecting camera \" << name();\n \n-\t/*\n-\t * If the camera was running when the hardware was removed force the\n-\t * state to Configured state to allow applications to free resources\n-\t * and call release() before deleting the camera.\n-\t */\n-\tif (p_->state_ == Private::CameraRunning)\n-\t\tp_->state_ = Private::CameraConfigured;\n-\n-\tp_->disconnected_ = true;\n+\tp_->disconnect();\n \tdisconnected.emit(this);\n }\n \n int Camera::exportFrameBuffers(Stream *stream,\n \t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n {\n-\tif (!p_->stateIs(Private::CameraConfigured))\n-\t\treturn -EACCES;\n+\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tif (streams().find(stream) == streams().end())\n \t\treturn -EINVAL;\n@@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream,\n \n int Camera::freeFrameBuffers(Stream *stream)\n {\n-\tif (!p_->stateIs(Private::CameraConfigured))\n-\t\treturn -EACCES;\n+\tint ret = p_->isAccessAllowed(Private::CameraConfigured, true);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tp_->pipe_->freeFrameBuffers(this, stream);\n \n@@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream)\n */\n int Camera::acquire()\n {\n-\tif (p_->disconnected_)\n-\t\treturn -ENODEV;\n-\n-\tif (!p_->stateIs(Private::CameraAvailable))\n-\t\treturn -EBUSY;\n+\tint ret = p_->isAccessAllowed(Private::CameraAvailable);\n+\tif (ret < 0)\n+\t\treturn ret == -EACCES ? -EBUSY : ret;\n \n \tif (!p_->pipe_->lock()) {\n \t\tLOG(Camera, Info)\n@@ -562,7 +589,7 @@ int Camera::acquire()\n \t\treturn -EBUSY;\n \t}\n \n-\tp_->state_ = Private::CameraAcquired;\n+\tp_->setState(Private::CameraAcquired);\n \n \treturn 0;\n }\n@@ -580,9 +607,10 @@ int Camera::acquire()\n */\n int Camera::release()\n {\n-\tif (!p_->stateBetween(Private::CameraAvailable,\n-\t\t\t Private::CameraConfigured))\n-\t\treturn -EBUSY;\n+\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n+\t\t\t\t Private::CameraConfigured, true);\n+\tif (ret < 0)\n+\t\treturn ret == -EACCES ? -EBUSY : ret;\n \n \tif (allocator_) {\n \t\t/*\n@@ -596,7 +624,7 @@ int Camera::release()\n \n \tp_->pipe_->unlock();\n \n-\tp_->state_ = Private::CameraAvailable;\n+\tp_->setState(Private::CameraAvailable);\n \n \treturn 0;\n }\n@@ -643,7 +671,12 @@ const std::set<Stream *> &Camera::streams() const\n */\n std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)\n {\n-\tif (p_->disconnected_ || roles.size() > streams().size())\n+\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n+\t\t\t\t Private::CameraRunning);\n+\tif (ret < 0)\n+\t\treturn nullptr;\n+\n+\tif (roles.size() > streams().size())\n \t\treturn nullptr;\n \n \tCameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);\n@@ -694,14 +727,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR\n */\n int Camera::configure(CameraConfiguration *config)\n {\n-\tint ret;\n-\n-\tif (p_->disconnected_)\n-\t\treturn -ENODEV;\n-\n-\tif (!p_->stateBetween(Private::CameraAcquired,\n-\t\t\t Private::CameraConfigured))\n-\t\treturn -EACCES;\n+\tint ret = p_->isAccessAllowed(Private::CameraAcquired,\n+\t\t\t\t Private::CameraConfigured);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tif (allocator_ && allocator_->allocated()) {\n \t\tLOG(Camera, Error)\n@@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config)\n \t\tp_->activeStreams_.insert(stream);\n \t}\n \n-\tp_->state_ = Private::CameraConfigured;\n+\tp_->setState(Private::CameraConfigured);\n \n \treturn 0;\n }\n@@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config)\n */\n Request *Camera::createRequest(uint64_t cookie)\n {\n-\tif (p_->disconnected_ ||\n-\t !p_->stateBetween(Private::CameraConfigured,\n-\t\t\t Private::CameraRunning))\n+\tint ret = p_->isAccessAllowed(Private::CameraConfigured,\n+\t\t\t\t Private::CameraRunning);\n+\tif (ret < 0)\n \t\treturn nullptr;\n \n \treturn new Request(this, cookie);\n@@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie)\n */\n int Camera::queueRequest(Request *request)\n {\n-\tif (p_->disconnected_)\n-\t\treturn -ENODEV;\n-\n-\tif (!p_->stateIs(Private::CameraRunning))\n-\t\treturn -EACCES;\n+\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tif (request->buffers().empty()) {\n \t\tLOG(Camera, Error) << \"Request contains no buffers\";\n@@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request)\n */\n int Camera::start()\n {\n-\tif (p_->disconnected_)\n-\t\treturn -ENODEV;\n-\n-\tif (!p_->stateIs(Private::CameraConfigured))\n-\t\treturn -EACCES;\n+\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tLOG(Camera, Debug) << \"Starting capture\";\n \n@@ -852,11 +877,11 @@ int Camera::start()\n \t\tp_->pipe_->importFrameBuffers(this, stream);\n \t}\n \n-\tint ret = p_->pipe_->start(this);\n+\tret = p_->pipe_->start(this);\n \tif (ret)\n \t\treturn ret;\n \n-\tp_->state_ = Private::CameraRunning;\n+\tp_->setState(Private::CameraRunning);\n \n \treturn 0;\n }\n@@ -875,15 +900,13 @@ int Camera::start()\n */\n int Camera::stop()\n {\n-\tif (p_->disconnected_)\n-\t\treturn -ENODEV;\n-\n-\tif (!p_->stateIs(Private::CameraRunning))\n-\t\treturn -EACCES;\n+\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tLOG(Camera, Debug) << \"Stopping capture\";\n \n-\tp_->state_ = Private::CameraConfigured;\n+\tp_->setState(Private::CameraConfigured);\n \n \tp_->pipe_->stop(this);\n \ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex 669097f609ab..3091971d5da0 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n * exportFrameBuffers() and importFrameBuffers() for the streams contained in\n * any camera configuration.\n *\n- * The only intended caller is the FrameBufferAllocator helper.\n+ * The only intended caller is Camera::exportFrameBuffers().\n *\n * \\return 0 on success or a negative error code otherwise\n */\n@@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n * called only after a successful call to either of these two methods, and only\n * once per stream.\n *\n- * The only intended callers are Camera::stop() and the FrameBufferAllocator\n- * helper.\n+ * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().\n */\n \n /**\n", "prefixes": [ "libcamera-devel", "15/19" ] }