Patch Detail
Show a patch.
GET /api/patches/2721/?format=api
{ "id": 2721, "url": "https://patchwork.libcamera.org/api/patches/2721/?format=api", "web_url": "https://patchwork.libcamera.org/patch/2721/", "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": "<20200122205723.8865-6-laurent.pinchart@ideasonboard.com>", "date": "2020-01-22T20:57:15", "name": "[libcamera-devel,v2,05/13] libcamera: camera: Centralize state checks in Private class", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "05443fccdbd44ec097f3eec7f9238eb86d448308", "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/2721/mbox/", "series": [ { "id": 645, "url": "https://patchwork.libcamera.org/api/series/645/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=645", "date": "2020-01-22T20:57:10", "name": "Initial libcamera threading model", "version": 2, "mbox": "https://patchwork.libcamera.org/series/645/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/2721/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/2721/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 6F0E260877\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 21:57:44 +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 205BA2F9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 21:57:44 +0100 (CET)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579726664;\n\tbh=+UVWjheWtVGjdlQ6DFm06uYCb9pKg14xuBRDDbB/PzA=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=ZaFGfo8iGWesHwOXSoBx1Sx/PDh9fWfoiSfU5zACKQH99lxKGNeNaVwN/56nZoJcJ\n\tvRggHQCrt7AoBnsDAYXpXUFJP90z/nOKbdBVHdQqUD6kfhGMypKDmvQkQ82X0vDfUF\n\tK+vaCV3xZgV4r2LGtFm82b+j1iyg+d8jTdFCmp/w=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Wed, 22 Jan 2020 22:57:15 +0200", "Message-Id": "<20200122205723.8865-6-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.24.1", "In-Reply-To": "<20200122205723.8865-1-laurent.pinchart@ideasonboard.com>", "References": "<20200122205723.8865-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=UTF-8", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v2 05/13] libcamera: camera: Centralize\n\tstate checks 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": "Wed, 22 Jan 2020 20:57:46 -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>\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n---\n src/libcamera/camera.cpp | 158 ++++++++++++++++-------------\n src/libcamera/pipeline_handler.cpp | 5 +-\n 2 files changed, 90 insertions(+), 73 deletions(-)", "diff": "diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\nindex d567148c36b1..802e7fd0d12f 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 const char *const camera_state_names[] = {\n \t\"Available\",\n \t\"Acquired\",\n@@ -293,10 +305,31 @@ static const char *const 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) < ARRAY_SIZE(camera_state_names));\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) < ARRAY_SIZE(camera_state_names) &&\n \t static_cast<unsigned int>(high) < ARRAY_SIZE(camera_state_names));\n@@ -306,21 +339,20 @@ 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) < ARRAY_SIZE(camera_state_names));\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 +500,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 +518,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 +540,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 +574,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 +584,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 +602,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 +619,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 +666,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 +722,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 +764,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 +791,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 +823,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 +859,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 +872,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 +895,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 01b9ede34b53..5476dbab74d5 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 The number of allocated buffers on success or a negative error code\n * otherwise\n@@ -358,8 +358,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", "v2", "05/13" ] }