Patch Detail
Show a patch.
GET /api/1.1/patches/667/?format=api
{ "id": 667, "url": "https://patchwork.libcamera.org/api/1.1/patches/667/?format=api", "web_url": "https://patchwork.libcamera.org/patch/667/", "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": "<20190228185126.32475-4-niklas.soderlund@ragnatech.se>", "date": "2019-02-28T18:51:26", "name": "[libcamera-devel,v3,3/3] libcamera: camera: add state machine to control access from applications", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "cdd0f88eab3052af6df7feb2c89d93093dd363dc", "submitter": { "id": 5, "url": "https://patchwork.libcamera.org/api/1.1/people/5/?format=api", "name": "Niklas Söderlund", "email": "niklas.soderlund@ragnatech.se" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/667/mbox/", "series": [ { "id": 197, "url": "https://patchwork.libcamera.org/api/1.1/series/197/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=197", "date": "2019-02-28T18:51:24", "name": "libcamera: improve validation of camera operations", "version": 3, "mbox": "https://patchwork.libcamera.org/series/197/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/667/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/667/checks/", "tags": {}, "headers": { "Return-Path": "<niklas.soderlund@ragnatech.se>", "Received": [ "from vsp-unauthed02.binero.net (vsp-unauthed02.binero.net\n\t[195.74.38.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 856BC610BB\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 28 Feb 2019 19:51:40 +0100 (CET)", "from bismarck.berto.se (unknown [89.233.230.99])\n\tby bin-vsp-out-03.atm.binero.net (Halon) with ESMTPA\n\tid e01af104-3b89-11e9-8144-0050569116f7;\n\tThu, 28 Feb 2019 19:51:36 +0100 (CET)" ], "X-Halon-ID": "e01af104-3b89-11e9-8144-0050569116f7", "Authorized-sender": "niklas@soderlund.pp.se", "From": "=?utf-8?q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Thu, 28 Feb 2019 19:51:26 +0100", "Message-Id": "<20190228185126.32475-4-niklas.soderlund@ragnatech.se>", "X-Mailer": "git-send-email 2.20.1", "In-Reply-To": "<20190228185126.32475-1-niklas.soderlund@ragnatech.se>", "References": "<20190228185126.32475-1-niklas.soderlund@ragnatech.se>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=UTF-8", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v3 3/3] libcamera: camera: add state\n\tmachine to control access from applications", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.23", "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": "Thu, 28 Feb 2019 18:51:40 -0000" }, "content": "There is a need to better control the order of operations an application\nperforms on a camera for it to function correctly. Add a basic state\nmachine to ensure applications perform operations on the camera in good\norder.\n\nInternal to the Camera states are added; Available, Acquired,\nConfigured, Prepared and Running. Each state represents a higher state\nof configuration of the camera ultimately leading to the highest state\nwhere the camera is capturing frames. Each state supports a subset of\noperations the application may perform.\n\nSigned-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n---\n include/libcamera/camera.h | 18 ++-\n src/libcamera/camera.cpp | 266 +++++++++++++++++++++++++++++++------\n 2 files changed, 238 insertions(+), 46 deletions(-)", "diff": "diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\nindex 9c8ae01ed5c607f1..e5212cf05d221279 100644\n--- a/include/libcamera/camera.h\n+++ b/include/libcamera/camera.h\n@@ -39,7 +39,7 @@ public:\n \tSignal<Camera *> disconnected;\n \n \tint acquire();\n-\tvoid release();\n+\tint release();\n \n \tconst std::set<Stream *> &streams() const;\n \tstd::map<Stream *, StreamConfiguration>\n@@ -47,7 +47,7 @@ public:\n \tint configureStreams(std::map<Stream *, StreamConfiguration> &config);\n \n \tint allocateBuffers();\n-\tvoid freeBuffers();\n+\tint freeBuffers();\n \n \tRequest *createRequest();\n \tint queueRequest(Request *request);\n@@ -56,20 +56,30 @@ public:\n \tint stop();\n \n private:\n+\tenum State {\n+\t\tCameraAvailable,\n+\t\tCameraAcquired,\n+\t\tCameraConfigured,\n+\t\tCameraPrepared,\n+\t\tCameraRunning,\n+\t};\n+\n \tCamera(PipelineHandler *pipe, const std::string &name);\n \t~Camera();\n \n+\tbool stateBetween(State low, State high) const;\n+\tbool stateIs(State state) const;\n+\n \tfriend class PipelineHandler;\n \tvoid disconnect();\n-\tint exclusiveAccess();\n \n \tstd::shared_ptr<PipelineHandler> pipe_;\n \tstd::string name_;\n \tstd::set<Stream *> streams_;\n \tstd::set<Stream *> activeStreams_;\n \n-\tbool acquired_;\n \tbool disconnected_;\n+\tState state_;\n };\n \n } /* namespace libcamera */\ndiff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\nindex 84b97b5c2ce94ecf..0b06c0d838b356f3 100644\n--- a/src/libcamera/camera.cpp\n+++ b/src/libcamera/camera.cpp\n@@ -11,6 +11,7 @@\n \n #include \"log.h\"\n #include \"pipeline_handler.h\"\n+#include \"utils.h\"\n \n /**\n * \\file camera.h\n@@ -42,6 +43,9 @@ LOG_DECLARE_CATEGORY(Camera)\n * \\class Camera\n * \\brief Camera device\n *\n+ * \\todo Add documentation for camera start timings. What exactly does the\n+ * camera expect the pipeline handler to do when start() is called?\n+ *\n * The Camera class models a camera capable of producing one or more image\n * streams from a single image source. It provides the main interface to\n * configuring and controlling the device, and capturing image streams. It is\n@@ -52,6 +56,78 @@ LOG_DECLARE_CATEGORY(Camera)\n * created with the create() function which returns a shared pointer. The\n * Camera constructors and destructor are private, to prevent instances from\n * being constructed and destroyed manually.\n+ *\n+ * \\section camera_operation Operating the Camera\n+ *\n+ * An application needs to perform a sequence of operations on a camera before\n+ * it is ready to process requests. The camera needs to be acquired, configured\n+ * and resources allocated or imported to prepare the camera for capture. Once\n+ * started the camera can process requests until it is stopped. When an\n+ * application is done with a camera all resources allocated needs to be freed\n+ * and the camera released.\n+ *\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 provided that all\n+ * resources allocated are freed prior to the reconfiguration.\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+ *\n+ * \\dot\n+ * digraph camera_state_machine {\n+ * node [shape = doublecircle ]; Available;\n+ * node [shape = circle ]; Acquired;\n+ * node [shape = circle ]; Configured;\n+ * node [shape = circle ]; Prepared;\n+ * node [shape = circle ]; Running;\n+ *\n+ * Available -> Available [label = \"release()\"];\n+ * Available -> Acquired [label = \"acquire()\"];\n+ *\n+ * Acquired -> Available [label = \"release()\"];\n+ * Acquired -> Configured [label = \"configureStreams()\"];\n+ *\n+ * Configured -> Available [label = \"release()\"];\n+ * Configured -> Configured [label = \"configureStreams()\"];\n+ * Configured -> Prepared [label = \"allocateBuffers()\"];\n+ *\n+ * Prepared -> Configured [label = \"freeBuffers()\"];\n+ * Prepared -> Prepared [label = \"createRequest()\"];\n+ * Prepared -> Running [label = \"start()\"];\n+ *\n+ * Running -> Prepared [label = \"stop()\"];\n+ * Running -> Running [label = \"createRequest(), queueRequest()\"];\n+ * }\n+ * \\enddot\n+ *\n+ * \\subsubsection Available\n+ * The base state of a camera, an application can inspect the properties of the\n+ * camera to determine if it wishes to use it. If an application wishes to use\n+ * a camera it should acquire() it to proceed to the Acquired state.\n+ *\n+ * \\subsubsection Acquired\n+ * In the acquired state an application has exclusive access to the camera and\n+ * may modify the camera's parameters to configure it and proceed to the\n+ * Configured state.\n+ *\n+ * \\subsubsection Configured\n+ * The camera is configured and ready for the application to prepare it with\n+ * resources. The camera may be reconfigured multiple times until resources\n+ * are provided and the state progresses to Prepared.\n+ *\n+ * \\subsubsection Prepared\n+ * The camera has been configured and provided with resources and is ready to be\n+ * started. The application may free the camera's resources to get back to the\n+ * Configured state or start it to progress to the Running state.\n+ *\n+ * \\subsubsection Running\n+ * The camera is running and ready to process requests queued by the\n+ * application. The camera remains in this state until it is stopped and moved\n+ * to the Prepared state.\n */\n \n /**\n@@ -116,17 +192,55 @@ const std::string &Camera::name() const\n */\n \n Camera::Camera(PipelineHandler *pipe, const std::string &name)\n-\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n-\t disconnected_(false)\n+\t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n+\t state_(CameraAvailable)\n {\n }\n \n Camera::~Camera()\n {\n-\tif (acquired_)\n+\tif (!stateIs(CameraAvailable))\n \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n }\n \n+static const char *const camera_state_name[] = {\n+\t\"Available\",\n+\t\"Acquired\",\n+\t\"Configured\",\n+\t\"Prepared\",\n+\t\"Running\",\n+};\n+\n+bool Camera::stateBetween(State low, State high) const\n+{\n+\tif (state_ >= low && state_ <= high)\n+\t\treturn true;\n+\n+\tASSERT(static_cast<unsigned int>(low) < ARRAY_SIZE(camera_state_name) &&\n+\t static_cast<unsigned int>(high) < ARRAY_SIZE(camera_state_name));\n+\n+\tLOG(Camera, Debug) << \"Camera in \" << camera_state_name[state_]\n+\t\t\t << \" state trying operation requiring state between \"\n+\t\t\t << camera_state_name[low] << \" and \"\n+\t\t\t << camera_state_name[high];\n+\n+\treturn false;\n+}\n+\n+bool Camera::stateIs(State state) const\n+{\n+\tif (state_ == state)\n+\t\treturn true;\n+\n+\tASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_name));\n+\n+\tLOG(Camera, Debug) << \"Camera in \" << camera_state_name[state_]\n+\t\t\t << \" state trying operation requiring state \"\n+\t\t\t << camera_state_name[state];\n+\n+\treturn false;\n+}\n+\n /**\n * \\brief Notify camera disconnection\n *\n@@ -135,11 +249,24 @@ Camera::~Camera()\n * instance notifies the application by emitting the #disconnected signal, and\n * ensures that all new calls to the application-facing Camera API return an\n * error immediately.\n+ *\n+ * \\todo: Deal with pending requests if the camera is disconnected in a\n+ * running state.\n+ * \\todo: Update comment about Running state when importing buffers as well as\n+ * allocating them are supported.\n */\n 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 Prepared to allow applications to call freeBuffers() and\n+\t * release() before deleting the camera.\n+\t */\n+\tif (state_ == CameraRunning)\n+\t\tstate_ = CameraPrepared;\n+\n \tdisconnected_ = true;\n \tdisconnected.emit(this);\n }\n@@ -155,16 +282,24 @@ void Camera::disconnect()\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 effects the state of the camera, see \\ref camera_operation.\n+ *\n * \\todo Implement exclusive access across processes.\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 -EBUSY The camera is not free and can't be acquired by the caller\n */\n int Camera::acquire()\n {\n-\tif (acquired_)\n+\tif (disconnected_)\n+\t\treturn -ENODEV;\n+\n+\tif (!stateIs(CameraAvailable))\n \t\treturn -EBUSY;\n \n-\tacquired_ = true;\n+\tstate_ = CameraAcquired;\n+\n \treturn 0;\n }\n \n@@ -173,10 +308,20 @@ int Camera::acquire()\n *\n * Releasing the camera device allows other users to acquire exclusive access\n * with the acquire() function.\n+ *\n+ * This function effects the state of the camera, see \\ref camera_operation.\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 */\n-void Camera::release()\n+int Camera::release()\n {\n-\tacquired_ = false;\n+\tif (!stateBetween(CameraAvailable, CameraConfigured))\n+\t\treturn -EBUSY;\n+\n+\tstate_ = CameraAvailable;\n+\n+\treturn 0;\n }\n \n /**\n@@ -235,18 +380,22 @@ Camera::streamConfiguration(std::set<Stream *> &streams)\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 effects the state of the camera, see \\ref camera_operation.\n+ *\n * \\return 0 on success or a negative error code otherwise\n- * \\retval -ENODEV The camera is not connected to any hardware\n- * \\retval -EACCES The user has not acquired exclusive access to the camera\n+ * \\retval -ENODEV The camera has been disconnected from the system\n+ * \\retval -EACCES The camera is not in a state where it can be configured\n * \\retval -EINVAL The configuration is not valid\n */\n int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)\n {\n \tint ret;\n \n-\tret = exclusiveAccess();\n-\tif (ret)\n-\t\treturn ret;\n+\tif (disconnected_)\n+\t\treturn -ENODEV;\n+\n+\tif (!stateBetween(CameraAvailable, CameraConfigured))\n+\t\treturn -EACCES;\n \n \tif (!config.size()) {\n \t\tLOG(Camera, Error)\n@@ -273,20 +422,28 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)\n \t\tstream->bufferPool().createBuffers(cfg.bufferCount);\n \t}\n \n+\tstate_ = CameraConfigured;\n+\n \treturn 0;\n }\n \n /**\n * \\brief Allocate buffers for all configured streams\n+ *\n+ * This function effects the state of the camera, see \\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 in a state where buffers can be allocated\n+ * \\retval -EINVAL The configuration is not valid\n */\n int Camera::allocateBuffers()\n {\n-\tint ret;\n+\tif (disconnected_)\n+\t\treturn -ENODEV;\n \n-\tret = exclusiveAccess();\n-\tif (ret)\n-\t\treturn ret;\n+\tif (!stateIs(CameraConfigured))\n+\t\treturn -EACCES;\n \n \tif (activeStreams_.empty()) {\n \t\tLOG(Camera, Error)\n@@ -295,7 +452,7 @@ int Camera::allocateBuffers()\n \t}\n \n \tfor (Stream *stream : activeStreams_) {\n-\t\tret = pipe_->allocateBuffers(this, stream);\n+\t\tint ret = pipe_->allocateBuffers(this, stream);\n \t\tif (ret) {\n \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n \t\t\tfreeBuffers();\n@@ -303,14 +460,24 @@ int Camera::allocateBuffers()\n \t\t}\n \t}\n \n+\tstate_ = CameraPrepared;\n+\n \treturn 0;\n }\n \n /**\n * \\brief Release all buffers from allocated pools in each stream\n+ *\n+ * This function effects the state of the camera, see \\ref camera_operation.\n+ *\n+ * \\return 0 on success or a negative error code otherwise\n+ * \\retval -EACCES The camera is not in a state where buffers can be freed\n */\n-void Camera::freeBuffers()\n+int Camera::freeBuffers()\n {\n+\tif (!stateIs(CameraPrepared))\n+\t\treturn -EACCES;\n+\n \tfor (Stream *stream : activeStreams_) {\n \t\tif (!stream->bufferPool().count())\n \t\t\tcontinue;\n@@ -318,6 +485,10 @@ void Camera::freeBuffers()\n \t\tpipe_->freeBuffers(this, stream);\n \t\tstream->bufferPool().destroyBuffers();\n \t}\n+\n+\tstate_ = CameraConfigured;\n+\n+\treturn 0;\n }\n \n /**\n@@ -333,7 +504,7 @@ void Camera::freeBuffers()\n */\n Request *Camera::createRequest()\n {\n-\tif (exclusiveAccess())\n+\tif (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))\n \t\treturn nullptr;\n \n \treturn new Request(this);\n@@ -351,16 +522,18 @@ Request *Camera::createRequest()\n * automatically after it completes.\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 */\n int Camera::queueRequest(Request *request)\n {\n-\tint ret;\n+\tif (disconnected_)\n+\t\treturn -ENODEV;\n \n-\tret = exclusiveAccess();\n-\tif (ret)\n-\t\treturn ret;\n+\tif (!stateIs(CameraRunning))\n+\t\treturn -EACCES;\n \n-\tret = request->prepare();\n+\tint ret = request->prepare();\n \tif (ret) {\n \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n \t\treturn ret;\n@@ -376,17 +549,29 @@ 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 effects the state of the camera, see \\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 in a state where it can be started\n */\n int Camera::start()\n {\n-\tint ret = exclusiveAccess();\n-\tif (ret)\n-\t\treturn ret;\n+\tif (disconnected_)\n+\t\treturn -ENODEV;\n+\n+\tif (!stateIs(CameraPrepared))\n+\t\treturn -EACCES;\n \n \tLOG(Camera, Debug) << \"Starting capture\";\n \n-\treturn pipe_->start(this);\n+\tint ret = pipe_->start(this);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tstate_ = CameraRunning;\n+\n+\treturn 0;\n }\n \n /**\n@@ -395,30 +580,27 @@ 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 effects the state of the camera, see \\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 can't be stopped\n */\n int Camera::stop()\n {\n-\tint ret = exclusiveAccess();\n-\tif (ret)\n-\t\treturn ret;\n+\tif (disconnected_)\n+\t\treturn -ENODEV;\n+\n+\tif (!stateIs(CameraRunning))\n+\t\treturn -EACCES;\n \n \tLOG(Camera, Debug) << \"Stopping capture\";\n \n+\tstate_ = CameraPrepared;\n+\n \tpipe_->stop(this);\n \n \treturn 0;\n }\n \n-int Camera::exclusiveAccess()\n-{\n-\tif (disconnected_)\n-\t\treturn -ENODEV;\n-\n-\tif (!acquired_)\n-\t\treturn -EACCES;\n-\n-\treturn 0;\n-}\n-\n } /* namespace libcamera */\n", "prefixes": [ "libcamera-devel", "v3", "3/3" ] }