Patch Detail
Show a patch.
GET /api/1.1/patches/16359/?format=api
{ "id": 16359, "url": "https://patchwork.libcamera.org/api/1.1/patches/16359/?format=api", "web_url": "https://patchwork.libcamera.org/patch/16359/", "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": "<20220623232210.18742-14-laurent.pinchart@ideasonboard.com>", "date": "2022-06-23T23:22:10", "name": "[libcamera-devel,13/13] gstreamer: Fix race conditions in task pause/resume", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "909d3e852861266a274f2b13dc1da23936c01700", "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/16359/mbox/", "series": [ { "id": 3214, "url": "https://patchwork.libcamera.org/api/1.1/series/3214/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=3214", "date": "2022-06-23T23:21:57", "name": "gstreamer: Queue multiple requests", "version": 1, "mbox": "https://patchwork.libcamera.org/series/3214/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/16359/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/16359/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 6CFF7BD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 23 Jun 2022 23:22:51 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D29A66565C;\n\tFri, 24 Jun 2022 01:22:50 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E372765646\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 24 Jun 2022 01:22:34 +0200 (CEST)", "from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 725BD6BB;\n\tFri, 24 Jun 2022 01:22:34 +0200 (CEST)" ], "DKIM-Signature": [ "v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656026571;\n\tbh=NPCkwNXRJdjGrG38Wc76cbEo79KkSu6n0yqhqAccaj0=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yCJlKeJU2LBlc5t/red5m940H+Yq8iDEYRZXWxP+Qz8vp6LUEi7+b57VaJ5AZSMI0\n\tCbDLu8tr0Qjcy5CcRS40SsDmRLaZ6Nm5vlzWHZSey3vakm5QHWUR1dXtVljuIxIRmf\n\tonIG1Rx1jPCrLlGRCcf+9wo3FcfGxk+T+tf2nM36ZV+klu97YwI3nz+QPG0TBQRjj9\n\tI3WeZCyZfRatVS4mnipNoSTmx8zD4Mq8GPwmJoL4XwhlWLigrCi8FUMW2gFkVNhsPz\n\tcMGr/cKTaIOGZErOLfhtddB/qi19T9j+p+mJIeq7/TPPJJ/Y4fQ8yNlKoSEXSUoZn6\n\tyzlj76BiqvZ7Q==", "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656026554;\n\tbh=NPCkwNXRJdjGrG38Wc76cbEo79KkSu6n0yqhqAccaj0=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=LiOqdHn5Uq3hcWU2dldkYYioiOYyMsEHbLDwmMF6G9CTip8Zv38I9XhxXbq0/3RWK\n\tOFZZkpnynUIDikyWTskk6s73Fwh6b/P0H4ASlKb2CFuNiy1WNhRdRT7ia2KGqDuzqz\n\txhIP877V5FPeD4NkWZMrsWoIfpAhO+zvs7AndNuk=" ], "Authentication-Results": "lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LiOqdHn5\"; dkim-atps=neutral", "To": "libcamera-devel@lists.libcamera.org", "Date": "Fri, 24 Jun 2022 02:22:10 +0300", "Message-Id": "<20220623232210.18742-14-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.35.1", "In-Reply-To": "<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>", "References": "<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH 13/13] gstreamer: Fix race conditions in\n\ttask pause/resume", "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>", "From": "Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>", "Reply-To": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "Cc": "Nicolas Dufresne <nicolas.dufresne@collabora.com>,\n\tVedant Paranjape <vedantparanjape160201@gmail.com>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "The task run function races with two other threads that want to resume\nthe task: the requestCompleted() handler and the buffer-notify signal\nhandler. If the former queues completed requests or the latter queues\nback buffers to the pool, and then resume the task, after the task run\nhandler checks the queues but before it attemps to pause the task, then\nthe task may be paused without noticing that more work is available.\n\nThe most immediate way to fix this is to take the stream_lock in the\nrequestCompleted() and buffer-notify signal handlers, or cover the whole\ntask run handler with the GstLibcameraSrcState lock. This could cause\nlong delays in the requestCompleted() handler, so that's not a good\noption.\n\nInstead, add a wakeup flag, preotected by the GstLibcameraSrcState lock,\nthat allows detection of a lost race, and retry the task run.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n src/gstreamer/gstlibcamerasrc.cpp | 82 +++++++++++++++++++++++--------\n 1 file changed, 62 insertions(+), 20 deletions(-)", "diff": "diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\nindex 3feb87254916..59400f17ae85 100644\n--- a/src/gstreamer/gstlibcamerasrc.cpp\n+++ b/src/gstreamer/gstlibcamerasrc.cpp\n@@ -120,6 +120,7 @@ struct GstLibcameraSrcState {\n \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n \tstd::queue<std::unique_ptr<RequestWrap>> completedRequests_\n \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n+\tbool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n \n \tguint group_id_;\n \n@@ -237,14 +238,16 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n \t{\n \t\tMutexLocker locker(lock_);\n \t\tcompletedRequests_.push(std::move(wrap));\n-\t}\n+\t\twakeup_ = true;\n \n-\tgst_task_resume(src_->task);\n+\t\tgst_task_resume(src_->task);\n+\t}\n }\n \n int GstLibcameraSrcState::processRequest()\n {\n \tstd::unique_ptr<RequestWrap> wrap;\n+\tint err = 0;\n \n \t{\n \t\tMutexLocker locker(lock_);\n@@ -253,10 +256,13 @@ int GstLibcameraSrcState::processRequest()\n \t\t\twrap = std::move(completedRequests_.front());\n \t\t\tcompletedRequests_.pop();\n \t\t}\n+\n+\t\tif (completedRequests_.empty())\n+\t\t\terr = -ENOBUFS;\n \t}\n \n \tif (!wrap)\n-\t\treturn -ENODATA;\n+\t\treturn -ENOBUFS;\n \n \tGstFlowReturn ret = GST_FLOW_OK;\n \tgst_flow_combiner_reset(src_->flow_combiner);\n@@ -296,7 +302,7 @@ int GstLibcameraSrcState::processRequest()\n \t\treturn -EPIPE;\n \t}\n \n-\treturn 0;\n+\treturn err;\n }\n \n static bool\n@@ -360,53 +366,88 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n \treturn true;\n }\n \n+static void\n+gst_libcamera_src_task_resume(gpointer user_data)\n+{\n+\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n+\tGstLibcameraSrcState *state = self->state;\n+\n+\tMutexLocker locker(state->lock_);\n+\tstate->wakeup_ = true;\n+\tgst_task_resume(self->task);\n+}\n+\n static void\n gst_libcamera_src_task_run(gpointer user_data)\n {\n \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n \tGstLibcameraSrcState *state = self->state;\n \n+\t{\n+\t\tMutexLocker locker(state->lock_);\n+\t\tstate->wakeup_ = true;\n+\t}\n+\n+\tbool doPause = true;\n+\n \t/*\n \t * Create and queue one request. If no buffers are available the\n \t * function returns -ENOBUFS, which we ignore here as that's not a\n \t * fatal error.\n \t */\n \tint ret = state->queueRequest();\n-\tif (ret == -ENOMEM) {\n+\tswitch (ret) {\n+\tcase 0:\n+\t\t/*\n+\t\t * The request was successfully queued, there may be enough\n+\t\t * buffers to create a new one. Don't pause the task to give it\n+\t\t * another try.\n+\t\t */\n+\t\tdoPause = false;\n+\t\tbreak;\n+\n+\tcase -ENOMEM:\n \t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n \t\t\t\t (\"Failed to allocate request for camera '%s'.\",\n \t\t\t\t state->cam_->id().c_str()),\n \t\t\t\t (\"libcamera::Camera::createRequest() failed\"));\n \t\tgst_task_stop(self->task);\n \t\treturn;\n+\n+\tcase -ENOBUFS:\n+\tdefault:\n+\t\tbreak;\n \t}\n \n-\t/* Process one completed request, if available. */\n+\t/*\n+\t * Process one completed request, if available, and record if further\n+\t * requests are ready for processing.\n+\t */\n \tret = state->processRequest();\n \tswitch (ret) {\n+\tcase -ENOBUFS:\n+\t\tdoPause = false;\n+\t\tbreak;\n+\n \tcase -EPIPE:\n \t\tgst_task_stop(self->task);\n \t\treturn;\n \n-\tcase -ENODATA:\n-\t\tgst_task_pause(self->task);\n-\t\treturn;\n+\tcase 0:\n+\tdefault:\n+\t\tbreak;\n \t}\n \n \t/*\n-\t * Here we need to decide if we want to pause. This needs to\n-\t * happen in lock step with the callback thread which may want\n-\t * to resume the task and might push pending buffers.\n+\t * Here we need to decide if we want to pause. This needs to happen in\n+\t * lock step with the requestCompleted callback and the buffer-notify\n+\t * signal handler that resume the task.\n \t */\n-\tbool do_pause;\n-\n-\t{\n+\tif (doPause) {\n \t\tMutexLocker locker(state->lock_);\n-\t\tdo_pause = state->completedRequests_.empty();\n+\t\tif (!state->wakeup_)\n+\t\t\tgst_task_pause(self->task);\n \t}\n-\n-\tif (do_pause)\n-\t\tgst_task_pause(self->task);\n }\n \n static void\n@@ -517,7 +558,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n \t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,\n \t\t\t\t\t\t\t\tstream_cfg.stream());\n \t\tg_signal_connect_swapped(pool, \"buffer-notify\",\n-\t\t\t\t\t G_CALLBACK(gst_task_resume), task);\n+\t\t\t\t\t G_CALLBACK(gst_libcamera_src_task_resume),\n+\t\t\t\t\t self);\n \n \t\tgst_libcamera_pad_set_pool(srcpad, pool);\n \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n", "prefixes": [ "libcamera-devel", "13/13" ] }