Patch Detail
Show a patch.
GET /api/patches/2703/?format=api
{ "id": 2703, "url": "https://patchwork.libcamera.org/api/patches/2703/?format=api", "web_url": "https://patchwork.libcamera.org/patch/2703/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/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-17-laurent.pinchart@ideasonboard.com>", "date": "2020-01-20T00:24:34", "name": "[libcamera-devel,16/19] libcamera: camera: Implement the threading model", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "552d1518cbe0b3f4027a7294f6780bfe4ed3b553", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/2703/mbox/", "series": [ { "id": 641, "url": "https://patchwork.libcamera.org/api/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/2703/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/2703/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "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 50FF06080E\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 EFF27504\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=1579479888;\n\tbh=KseSN/qBI8Px+ooqDyDbzUfVgIBJOOrxYiYiBXJXC+M=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=vmxkiPAs+sEJ1lIZtosnkrpHz48TvTp3gqyl0X8xjQzErtZP16nblxJ684zLyfRoS\n\t/Dpw+ILQUFJzOsSJBBzjXP34gQBoEAVtDY7mFRBq1eduMaqPP7uAk9mJkrlVmmkXTa\n\tXua3iOdEp7pbpLurGAmiaKEs5NgSMwHXB5wVjPLo=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Mon, 20 Jan 2020 02:24:34 +0200", "Message-Id": "<20200120002437.6633-17-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 16/19] libcamera: camera: Implement the\n\tthreading model", "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": "Document the threading model of the Camera class and implement it.\nSelected functions become thread-safe, and require a few functions of\nthe PipelineHandler class to be called through cross-thread invocation\nas the pipeline handlers live in the camera manager thread, while the\nCamera class is mostly accessed from the application thread. The\nPipelineHandler is made to inherit from the Object class to support\nthis.\n\nDisconnection is currently left out as it is not implemented in pipeline\nhandlers, and isn't fully supported in the Camera class either. This\nwill be revisited when implementing proper hotplug support.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n src/libcamera/camera.cpp | 101 +++++++++++++++++------\n src/libcamera/include/pipeline_handler.h | 4 +-\n 2 files changed, 78 insertions(+), 27 deletions(-)", "diff": "diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\nindex 83c2249b8897..f281a4cd346b 100644\n--- a/src/libcamera/camera.cpp\n+++ b/src/libcamera/camera.cpp\n@@ -8,6 +8,7 @@\n #include <libcamera/camera.h>\n \n #include <array>\n+#include <atomic>\n #include <iomanip>\n \n #include <libcamera/framebuffer_allocator.h>\n@@ -282,7 +283,7 @@ public:\n \n private:\n \tbool disconnected_;\n-\tState state_;\n+\tstd::atomic<State> state_;\n };\n \n Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n@@ -294,7 +295,7 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n \n Camera::Private::~Private()\n {\n-\tif (state_ != Private::CameraAvailable)\n+\tif (state_.load(std::memory_order_acquire) != Private::CameraAvailable)\n \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n }\n \n@@ -310,12 +311,13 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const\n \tif (!allowDisconnected && disconnected_)\n \t\treturn -ENODEV;\n \n-\tif (state_ == state)\n+\tState currentState = state_.load(std::memory_order_acquire);\n+\tif (currentState == 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+\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[currentState]\n \t\t\t << \" state trying operation requiring state \"\n \t\t\t << camera_state_names[state];\n \n@@ -328,13 +330,14 @@ int Camera::Private::isAccessAllowed(State low, State high,\n \tif (!allowDisconnected && disconnected_)\n \t\treturn -ENODEV;\n \n-\tif (state_ >= low && state_ <= high)\n+\tState currentState = state_.load(std::memory_order_acquire);\n+\tif (currentState >= low && currentState <= high)\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 \n-\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n+\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[currentState]\n \t\t\t << \" state trying operation requiring state between \"\n \t\t\t << camera_state_names[low] << \" and \"\n \t\t\t << camera_state_names[high];\n@@ -349,15 +352,15 @@ void Camera::Private::disconnect()\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+\tif (state_.load(std::memory_order_acquire) == Private::CameraRunning)\n+\t\tstate_.store(Private::CameraConfigured, std::memory_order_release);\n \n \tdisconnected_ = true;\n }\n \n void Camera::Private::setState(State state)\n {\n-\tstate_ = state;\n+\tstate_.store(state, std::memory_order_release);\n }\n \n /**\n@@ -389,12 +392,17 @@ void Camera::Private::setState(State state)\n * An application may start and stop a camera multiple times as long as it is\n * not released. The camera may also be reconfigured.\n *\n+ * Functions that affect the camera state as defined below are generally not\n+ * synchronized with each other by the Camera class. The caller is responsible\n+ * for ensuring their synchronization if necessary.\n+ *\n * \\subsection Camera States\n *\n * To help manage the sequence of operations needed to control the camera a set\n * of states are defined. Each state describes which operations may be performed\n- * on the camera. Operations not listed in the state diagram are allowed in all\n- * states.\n+ * on the camera. Performing an operation not allowed in the camera state\n+ * results in undefined behaviour. Operations not listed at all in the state\n+ * diagram are allowed in all states.\n *\n * \\dot\n * digraph camera_state_machine {\n@@ -467,6 +475,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n \n /**\n * \\brief Retrieve the name of the camera\n+ * \\context This function is \\threadsafe.\n * \\return Name of the camera device\n */\n const std::string &Camera::name() const\n@@ -540,7 +549,9 @@ int Camera::exportFrameBuffers(Stream *stream,\n \tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end())\n \t\treturn -EINVAL;\n \n-\treturn p_->pipe_->exportFrameBuffers(this, stream, buffers);\n+\treturn p_->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n+\t\t\t\t ConnectionTypeBlocking, this, stream,\n+\t\t\t\t buffers);\n }\n \n int Camera::freeFrameBuffers(Stream *stream)\n@@ -549,7 +560,8 @@ int Camera::freeFrameBuffers(Stream *stream)\n \tif (ret < 0)\n \t\treturn ret;\n \n-\tp_->pipe_->freeFrameBuffers(this, stream);\n+\tp_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,\n+\t\t\t\tConnectionTypeBlocking, this, stream);\n \n \treturn 0;\n }\n@@ -571,7 +583,8 @@ int Camera::freeFrameBuffers(Stream *stream)\n * Once exclusive access isn't needed anymore, the device should be released\n * with a call to the release() function.\n *\n- * This function affects the state of the camera, see \\ref camera_operation.\n+ * \\context This function is \\threadsafe. It may only be called when the camera\n+ * is in the Available state as defined in \\ref camera_operation.\n *\n * \\return 0 on success or a negative error code otherwise\n * \\retval -ENODEV The camera has been disconnected from the system\n@@ -579,6 +592,10 @@ int Camera::freeFrameBuffers(Stream *stream)\n */\n int Camera::acquire()\n {\n+\t/*\n+\t * No manual locking is required as PipelineHandler::lock() is\n+\t * thread-safe.\n+\t */\n \tint ret = p_->isAccessAllowed(Private::CameraAvailable);\n \tif (ret < 0)\n \t\treturn ret == -EACCES ? -EBUSY : ret;\n@@ -600,7 +617,10 @@ int Camera::acquire()\n * Releasing the camera device allows other users to acquire exclusive access\n * with the acquire() function.\n *\n- * This function affects the state of the camera, see \\ref camera_operation.\n+ * \\context This function may only be called when the camera is in the\n+ * Available or Configured state as defined in \\ref camera_operation, and shall\n+ * be synchronized by the caller with other functions that affect the camera\n+ * state.\n *\n * \\return 0 on success or a negative error code otherwise\n * \\retval -EBUSY The camera is running and can't be released\n@@ -634,6 +654,8 @@ int Camera::release()\n *\n * Camera controls remain constant through the lifetime of the camera.\n *\n+ * \\context This function is \\threadsafe.\n+ *\n * \\return A ControlInfoMap listing the controls supported by the camera\n */\n const ControlInfoMap &Camera::controls()\n@@ -648,6 +670,8 @@ const ControlInfoMap &Camera::controls()\n * information describes among other things how many streams the camera\n * supports and the capabilities of each stream.\n *\n+ * \\context This function is \\threadsafe.\n+ *\n * \\return An array of all the camera's streams.\n */\n const std::set<Stream *> &Camera::streams() const\n@@ -665,6 +689,8 @@ const std::set<Stream *> &Camera::streams() const\n * empty list of roles is valid, and will generate an empty configuration that\n * can be filled by the caller.\n *\n+ * \\context This function is \\threadsafe.\n+ *\n * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n * null pointer otherwise. The ownership of the returned configuration is\n * passed to the caller.\n@@ -715,7 +741,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR\n * Exclusive access to the camera shall be ensured by a call to acquire() prior\n * to calling this function, otherwise an -EACCES error will be returned.\n *\n- * This function affects the state of the camera, see \\ref camera_operation.\n+ * \\context This function may only be called when the camera is in the Acquired\n+ * or Configured state as defined in \\ref camera_operation, and shall be\n+ * synchronized by the caller with other functions that affect the camera\n+ * state.\n *\n * Upon return the StreamConfiguration entries in \\a config are associated with\n * Stream instances which can be retrieved with StreamConfiguration::stream().\n@@ -754,7 +783,8 @@ int Camera::configure(CameraConfiguration *config)\n \n \tLOG(Camera, Info) << msg.str();\n \n-\tret = p_->pipe_->configure(this, config);\n+\tret = p_->pipe_->invokeMethod(&PipelineHandler::configure,\n+\t\t\t\t ConnectionTypeBlocking, this, config);\n \tif (ret)\n \t\treturn ret;\n \n@@ -789,8 +819,8 @@ int Camera::configure(CameraConfiguration *config)\n * The ownership of the returned request is passed to the caller, which is\n * responsible for either queueing the request or deleting it.\n *\n- * This function shall only be called when the camera is in the Configured\n- * or Running state, see \\ref camera_operation.\n+ * \\context This function is \\threadsafe. It may only be called when the camera\n+ * is in the Configured or Running state as defined in \\ref camera_operation.\n *\n * \\return A pointer to the newly created request, or nullptr on error\n */\n@@ -820,6 +850,9 @@ Request *Camera::createRequest(uint64_t cookie)\n * Ownership of the request is transferred to the camera. It will be deleted\n * automatically after it completes.\n *\n+ * \\context This function is \\threadsafe. It may only be called when the camera\n+ * is in the Running state as defined in \\ref camera_operation.\n+ *\n * \\return 0 on success or a negative error code otherwise\n * \\retval -ENODEV The camera has been disconnected from the system\n * \\retval -EACCES The camera is not running so requests can't be queued\n@@ -832,6 +865,12 @@ int Camera::queueRequest(Request *request)\n \tif (ret < 0)\n \t\treturn ret;\n \n+\t/*\n+\t * The camera state may chance until the end of the function. No locking\n+\t * is however needed as PipelineHandler::queueRequest() will handle\n+\t * this.\n+\t */\n+\n \tif (request->buffers().empty()) {\n \t\tLOG(Camera, Error) << \"Request contains no buffers\";\n \t\treturn -EINVAL;\n@@ -846,7 +885,8 @@ int Camera::queueRequest(Request *request)\n \t\t}\n \t}\n \n-\treturn p_->pipe_->queueRequest(this, request);\n+\treturn p_->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n+\t\t\t\t ConnectionTypeQueued, this, request);\n }\n \n /**\n@@ -856,7 +896,10 @@ int Camera::queueRequest(Request *request)\n * can queue requests to the camera to process and return to the application\n * until the capture session is terminated with \\a stop().\n *\n- * This function affects the state of the camera, see \\ref camera_operation.\n+ * \\context This function may only be called when the camera is in the\n+ * Configured state as defined in \\ref camera_operation, and shall be\n+ * synchronized by the caller with other functions that affect the camera\n+ * state.\n *\n * \\return 0 on success or a negative error code otherwise\n * \\retval -ENODEV The camera has been disconnected from the system\n@@ -874,10 +917,12 @@ int Camera::start()\n \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n \t\t\tcontinue;\n \n-\t\tp_->pipe_->importFrameBuffers(this, stream);\n+\t\tp_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers,\n+\t\t\t\t\tConnectionTypeDirect, this, stream);\n \t}\n \n-\tret = p_->pipe_->start(this);\n+\tret = p_->pipe_->invokeMethod(&PipelineHandler::start,\n+\t\t\t\t ConnectionTypeBlocking, this);\n \tif (ret)\n \t\treturn ret;\n \n@@ -892,7 +937,9 @@ int Camera::start()\n * This method stops capturing and processing requests immediately. All pending\n * requests are cancelled and complete synchronously in an error state.\n *\n- * This function affects the state of the camera, see \\ref camera_operation.\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 *\n * \\return 0 on success or a negative error code otherwise\n * \\retval -ENODEV The camera has been disconnected from the system\n@@ -908,13 +955,15 @@ int Camera::stop()\n \n \tp_->setState(Private::CameraConfigured);\n \n-\tp_->pipe_->stop(this);\n+\tp_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n+\t\t\t\tthis);\n \n \tfor (Stream *stream : p_->activeStreams_) {\n \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n \t\t\tcontinue;\n \n-\t\tp_->pipe_->freeFrameBuffers(this, stream);\n+\t\tp_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,\n+\t\t\t\t\tConnectionTypeBlocking, this, stream);\n \t}\n \n \treturn 0;\ndiff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\nindex a6c1e1fbae38..db0ec6ac00d7 100644\n--- a/src/libcamera/include/pipeline_handler.h\n+++ b/src/libcamera/include/pipeline_handler.h\n@@ -17,6 +17,7 @@\n \n #include <ipa/ipa_interface.h>\n #include <libcamera/controls.h>\n+#include <libcamera/object.h>\n #include <libcamera/stream.h>\n \n namespace libcamera {\n@@ -51,7 +52,8 @@ private:\n \tCameraData &operator=(const CameraData &) = delete;\n };\n \n-class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>\n+class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>,\n+\t\t\tpublic Object\n {\n public:\n \tPipelineHandler(CameraManager *manager);\n", "prefixes": [ "libcamera-devel", "16/19" ] }