[{"id":23680,"web_url":"https://patchwork.libcamera.org/comment/23680/","msgid":"","date":"2022-06-30T10:01:57","subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\n(still working through this patch)...\n\nOn 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:\n> The task run function races with two other threads that want to resume\n> the task: the requestCompleted() handler and the buffer-notify signal\n\n\nWhat do you mean by \"two other threads\" ?\n\nI think the \"buffer-notify\" signal is emitted in the task-thread itself. \nAre gst_libcamera_src_task_enter() and the task function itself (i.e. \ngst_libcamera_src_task_run()) are different threads? I think it's the \nsame thread.\n\nIf my understanding above is correct, the resume of task races with \nrequestCompleted() handler thread only.\n\nOfcourse, I need to see how the task_resume is affecting the task_run \nfrom different threads but I am still trying to grasp the issue.\n\n> handler. If the former queues completed requests or the latter queues\n> back buffers to the pool, and then resume the task, after the task run\n> handler checks the queues but before it attemps to pause the task, then\n> the task may be paused without noticing that more work is available.\n>\n> The most immediate way to fix this is to take the stream_lock in the\n> requestCompleted() and buffer-notify signal handlers, or cover the whole\n> task run handler with the GstLibcameraSrcState lock. This could cause\n> long delays in the requestCompleted() handler, so that's not a good\n> option.\n>\n> Instead, add a wakeup flag, preotected by the GstLibcameraSrcState lock,\n> that allows detection of a lost race, and retry the task run.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n>\n> - Fix incorrect wakeup and pause logic\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 84 +++++++++++++++++++++++--------\n> 1 file changed, 63 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 9ea59631a9f2..5471ab951252 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -118,7 +118,7 @@ struct GstLibcameraSrcState {\n> \t/*\n> \t * Contention on this lock_ must be minimized, as it has to be taken in\n> \t * the realtime-sensitive requestCompleted() handler to protect\n> -\t * queuedRequests_ and completedRequests_.\n> +\t * queuedRequests_, completedRequests_ and wakeup_.\n> \t *\n> \t * stream_lock must be taken before lock_ in contexts where both locks\n> \t * need to be taken. In particular, this means that the lock_ must not\n> @@ -130,6 +130,7 @@ struct GstLibcameraSrcState {\n> \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> \tstd::queue> completedRequests_\n> \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> +\tbool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n> \n> \tguint group_id_;\n> \n> @@ -250,15 +251,17 @@ 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> /* Must be called with stream_lock held. */\n> int GstLibcameraSrcState::processRequest()\n> {\n> \tstd::unique_ptr wrap;\n> +\tint err = 0;\n> \n> \t{\n> \t\tMutexLocker locker(lock_);\n> @@ -267,10 +270,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> @@ -310,7 +316,7 @@ int GstLibcameraSrcState::processRequest()\n> \t\treturn -EPIPE;\n> \t}\n> \n> -\treturn 0;\n> +\treturn err;\n> }\n> \n> static bool\n> @@ -374,53 +380,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_ = false;\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> @@ -531,7 +572,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);","headers":{"Return-Path":"","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 61D50BD808\n\tfor ;\n\tThu, 30 Jun 2022 10:02:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1D8A65633;\n\tThu, 30 Jun 2022 12:02:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B93F6559A\n\tfor ;\n\tThu, 30 Jun 2022 12:02:10 +0200 (CEST)","from [IPV6:2401:4900:1f3f:ca21:e286:106b:5da4:9482] (unknown\n\t[IPv6:2401:4900:1f3f:ca21:e286:106b:5da4:9482])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A346045F;\n\tThu, 30 Jun 2022 12:02:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656583331;\n\tbh=iKy1OUncf3ABXPlnG9MCbc/MoK4XpdzwNrk3V1vtIv8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=A9cvRWGqNEcdmM2zHLghtsriVNrWJ1GNexMMRU3xjJmFi2l6SZgWYNPMly3HG6lPt\n\tNyS/HRxZSY7P7301YCdDDwxomwdFNLYRMJA0LbaSwELTlCt/HbHbzPFebVcgjqkTLo\n\tC+AZ9rekbeaEcqoy41NyqnXegHGpB9mkFQz6nbl93DWUYcDd45RDI0bpV8Uit1d9kk\n\tDX5SST/gczhO+dF9Ck5xx64eOLWHkpH/bkU3dfx/E3QbscYrtnWTxZv3yfTuACC6cJ\n\ttLzQq28+Bk8+qlsRVk+3HDWyxhjfvfx0rr3/KcDJVJZWIVKTBc4qUU14bWJ6mLJKiL\n\ttZIXAgNk+ELyQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656583330;\n\tbh=iKy1OUncf3ABXPlnG9MCbc/MoK4XpdzwNrk3V1vtIv8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=tvCrz6e114nwQEOUXULkFW3h4ckOSoI/RyWawTRa3IsfnZ+DLrSRZhYcgA5InWwUm\n\tXFMI6xLbG5hEohojdhNI+fHuxpQYaouLa0rfhMYkE0lsRPqQe9IxyPQ5XIiuNOeuB8\n\tYLo7lDd7yza8krilvnFaaYc6aRiHN3fnKLGUJRCs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tvCrz6e1\"; dkim-atps=neutral","Message-ID":"","Date":"Thu, 30 Jun 2022 15:31:57 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220630000251.31295-1-laurent.pinchart@ideasonboard.com>\n\t<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23683,"web_url":"https://patchwork.libcamera.org/comment/23683/","msgid":"","date":"2022-06-30T10:48:21","subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jun 30, 2022 at 03:31:57PM +0530, Umang Jain wrote:\n> Hi Laurent,\n> \n> (still working through this patch)...\n> \n> On 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:\n> > The task run function races with two other threads that want to resume\n> > the task: the requestCompleted() handler and the buffer-notify signal\n> \n> What do you mean by \"two other threads\" ?\n> \n> I think the \"buffer-notify\" signal is emitted in the task-thread itself. \n> Are gst_libcamera_src_task_enter() and the task function itself (i.e. \n> gst_libcamera_src_task_run()) are different threads? I think it's the \n> same thread.\n\nThe buffer-notify signal is emitted by\ngst_libcamera_pool_release_buffer(), which is called by GStreamer from a\ndifferent thread.\n\n> If my understanding above is correct, the resume of task races with \n> requestCompleted() handler thread only.\n> \n> Ofcourse, I need to see how the task_resume is affecting the task_run \n> from different threads but I am still trying to grasp the issue.\n> \n> > handler. If the former queues completed requests or the latter queues\n> > back buffers to the pool, and then resume the task, after the task run\n> > handler checks the queues but before it attemps to pause the task, then\n> > the task may be paused without noticing that more work is available.\n> >\n> > The most immediate way to fix this is to take the stream_lock in the\n> > requestCompleted() and buffer-notify signal handlers, or cover the whole\n> > task run handler with the GstLibcameraSrcState lock. This could cause\n> > long delays in the requestCompleted() handler, so that's not a good\n> > option.\n> >\n> > Instead, add a wakeup flag, preotected by the GstLibcameraSrcState lock,\n> > that allows detection of a lost race, and retry the task run.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> >\n> > - Fix incorrect wakeup and pause logic\n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 84 +++++++++++++++++++++++--------\n> > 1 file changed, 63 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 9ea59631a9f2..5471ab951252 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -118,7 +118,7 @@ struct GstLibcameraSrcState {\n> > \t/*\n> > \t * Contention on this lock_ must be minimized, as it has to be taken in\n> > \t * the realtime-sensitive requestCompleted() handler to protect\n> > -\t * queuedRequests_ and completedRequests_.\n> > +\t * queuedRequests_, completedRequests_ and wakeup_.\n> > \t *\n> > \t * stream_lock must be taken before lock_ in contexts where both locks\n> > \t * need to be taken. In particular, this means that the lock_ must not\n> > @@ -130,6 +130,7 @@ struct GstLibcameraSrcState {\n> > \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > \tstd::queue> completedRequests_\n> > \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > +\tbool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n> > \n> > \tguint group_id_;\n> > \n> > @@ -250,15 +251,17 @@ 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> > /* Must be called with stream_lock held. */\n> > int GstLibcameraSrcState::processRequest()\n> > {\n> > \tstd::unique_ptr wrap;\n> > +\tint err = 0;\n> > \n> > \t{\n> > \t\tMutexLocker locker(lock_);\n> > @@ -267,10 +270,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> > @@ -310,7 +316,7 @@ int GstLibcameraSrcState::processRequest()\n> > \t\treturn -EPIPE;\n> > \t}\n> > \n> > -\treturn 0;\n> > +\treturn err;\n> > }\n> > \n> > static bool\n> > @@ -374,53 +380,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_ = false;\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> > @@ -531,7 +572,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);","headers":{"Return-Path":"","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 1510BBE173\n\tfor ;\n\tThu, 30 Jun 2022 10:48:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49D406564E;\n\tThu, 30 Jun 2022 12:48:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD67B6559A\n\tfor ;\n\tThu, 30 Jun 2022 12:48:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 408A245F;\n\tThu, 30 Jun 2022 12:48:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656586123;\n\tbh=6nQWnHFOX/RZP/8jz2JK5h6k22d625bk2204rEPuDic=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=wBNbEp8oJ5Raoc6w9wEYv2L02S+/SRjNZmIDK9zLGIRuXm9owz0ocX2pLFQgAykh4\n\thBWADpN8ftkBJeMAZqAHPM5674zsHS2ACOryZt0tMSEgvk3pQW+EkZhuQxHz9DL6Au\n\tLGI92v2oEGWKmVu/q0f5cHXjTC8dHNDpP3tAn3vk4FJkDNiOTf1sT1FTzR2J6kkxRs\n\tyyOTTP1QKhtUAEKjjkQP5y9pfRllpfEVViP/+Jf2AlptWVjQ2G+6/ZQpxb5aBT/hkY\n\tQnfhHJf6T06yicH7e6ZNRqHXRVRMblDavYN7Clkjf9VSalRwaV8Smrg43iPve5Tc70\n\tryQcc6/kXSAeg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656586121;\n\tbh=6nQWnHFOX/RZP/8jz2JK5h6k22d625bk2204rEPuDic=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I9X5xf3vN1Ta8pc5kr3fkhLeg4EDOW/b1NBvIHxWsnYeLVjPPl2ErX9HlfvTJzaE4\n\t7kmTIpm754T9cOp9WNgThnHGS7R5ewEUG0RjJz9oFjwxr3JmF4ENB6U9n7vkEEZHdf\n\tlHxFI0QQlpGD0F3a85a01QipEzHneAM9fT8kAqis="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"I9X5xf3v\"; dkim-atps=neutral","Date":"Thu, 30 Jun 2022 13:48:21 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220630000251.31295-1-laurent.pinchart@ideasonboard.com>\n\t<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23689,"web_url":"https://patchwork.libcamera.org/comment/23689/","msgid":"<4640cabc56e963855f6fb2c39dec190afce6b01b.camel@collabora.com>","date":"2022-06-30T19:38:34","subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi Laurent,\n\nthis was too fast for me ;-D\n\nLe jeudi 30 juin 2022 à 03:02 +0300, Laurent Pinchart a écrit :\n> The task run function races with two other threads that want to resume\n> the task: the requestCompleted() handler and the buffer-notify signal\n> handler. If the former queues completed requests or the latter queues\n> back buffers to the pool, and then resume the task, after the task run\n> handler checks the queues but before it attemps to pause the task, then\n> the task may be paused without noticing that more work is available.\n> \n> The most immediate way to fix this is to take the stream_lock in the\n> requestCompleted() and buffer-notify signal handlers, or cover the whole\n> task run handler with the GstLibcameraSrcState lock. This could cause\n> long delays in the requestCompleted() handler, so that's not a good\n> option.\n> \n> Instead, add a wakeup flag, preotected by the GstLibcameraSrcState lock,\n> that allows detection of a lost race, and retry the task run.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n> \n> - Fix incorrect wakeup and pause logic\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 84 +++++++++++++++++++++++--------\n> 1 file changed, 63 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 9ea59631a9f2..5471ab951252 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -118,7 +118,7 @@ struct GstLibcameraSrcState {\n> \t/*\n> \t * Contention on this lock_ must be minimized, as it has to be taken in\n> \t * the realtime-sensitive requestCompleted() handler to protect\n> -\t * queuedRequests_ and completedRequests_.\n> +\t * queuedRequests_, completedRequests_ and wakeup_.\n> \t *\n> \t * stream_lock must be taken before lock_ in contexts where both locks\n> \t * need to be taken. In particular, this means that the lock_ must not\n> @@ -130,6 +130,7 @@ struct GstLibcameraSrcState {\n> \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> \tstd::queue> completedRequests_\n> \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> +\tbool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n\nThe method I described was all about removing this wakeup_ boolean. If the run()\nfunction suspend itself right at the start (perhaps holding the locker lock (I\nstill don't like that lock name)), it shouldn't conflict with requestComplete()\nresume. In the worst scenario, once in a while, run() will be called and will\nsuspend without doing anything. I do believe wakeup_ bool is error prone, and I\nwould prefer to see this removed.\n\nregards,\nNicolas\n\n> \n> \tguint group_id_;\n> \n> @@ -250,15 +251,17 @@ 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> /* Must be called with stream_lock held. */\n> int GstLibcameraSrcState::processRequest()\n> {\n> \tstd::unique_ptr wrap;\n> +\tint err = 0;\n> \n> \t{\n> \t\tMutexLocker locker(lock_);\n> @@ -267,10 +270,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> @@ -310,7 +316,7 @@ int GstLibcameraSrcState::processRequest()\n> \t\treturn -EPIPE;\n> \t}\n> \n> -\treturn 0;\n> +\treturn err;\n> }\n> \n> static bool\n> @@ -374,53 +380,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_ = false;\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> @@ -531,7 +572,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);","headers":{"Return-Path":"","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 BEDE0BE173\n\tfor ;\n\tThu, 30 Jun 2022 19:38:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 115546564F;\n\tThu, 30 Jun 2022 21:38:46 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC56E65633\n\tfor ;\n\tThu, 30 Jun 2022 21:38:44 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id ED3776601969;\n\tThu, 30 Jun 2022 20:38:43 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656617926;\n\tbh=YdSoyTjUC+LTJtKiik2QnM/fqVdjeOO6VfencF5SDU8=;\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:\n\tFrom;\n\tb=j56/IdzFroTb8vh2Bt3wpZIqX2JonDejd81iWnIckQsSIh76dW2z01ZaiK05e/ItS\n\t06IohJAhsrcREgfhnwTRTWpvP/1f8xFTNQVYZl+2kXbtR7vuHVEYoUE9Bn4RjyqYk8\n\tnCu7jwyQHyFZK7+8xzBcI8OdKfMBU1nqvz+Eof595sGvS+BZxtJk+RFqHoe01gEgQc\n\t7MQ/3ReXiu3VxNvIYQNTEzXkPM9bhGQ6r/pRlYs2qHup0c5egYZ66pwzbgqn4xqpew\n\t7YXTvD52of4GqAhIHFm9651Hb/rd7oti+FiR1gioirNjDLx0lZQwI+mZIu+freACFW\n\tK6MkkkdUFg86g==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1656617924;\n\tbh=YdSoyTjUC+LTJtKiik2QnM/fqVdjeOO6VfencF5SDU8=;\n\th=Subject:From:To:Date:In-Reply-To:References:From;\n\tb=ni1k0di5gmN72nqXkCkKAz57LoiFaH0Xm0qI8Oityj1XnZyQ3iNveuSQ5nXvOoDHR\n\t4ADEb6X84cjK8gga8dkCQGU2W79VR7NTm4v6bWiZQxSbicomJp3PMdC3doYyXcbm8z\n\tuLg13mhWYe5apDDhE3TuySqDRzJ3i0wU45etEXkHmDtc+FygLj3KcP5y7TRgRV75sG\n\tHOQOsGwa4eA9460i+ztQuODY0dkULxFbolK3CtrUQVXoHOKEHEonJB+RF3f9QeM6/H\n\t17VBY6mfYFOnznwzAmf0eTMTmIPlj76dRG4Fei07CC9ONEzFHC1WhzEFK2GqYwJf6h\n\tRAmCoNJOqdS8A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"ni1k0di5\"; dkim-atps=neutral","Message-ID":"<4640cabc56e963855f6fb2c39dec190afce6b01b.camel@collabora.com>","To":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 30 Jun 2022 15:38:34 -0400","In-Reply-To":"<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>","References":"<20220630000251.31295-1-laurent.pinchart@ideasonboard.com>\n\t<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23690,"web_url":"https://patchwork.libcamera.org/comment/23690/","msgid":"","date":"2022-06-30T19:41:36","subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le jeudi 30 juin 2022 à 13:48 +0300, Laurent Pinchart a écrit :\n> On Thu, Jun 30, 2022 at 03:31:57PM +0530, Umang Jain wrote:\n> > Hi Laurent,\n> > \n> > (still working through this patch)...\n> > \n> > On 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:\n> > > The task run function races with two other threads that want to resume\n> > > the task: the requestCompleted() handler and the buffer-notify signal\n> > \n> > What do you mean by \"two other threads\" ?\n> > \n> > I think the \"buffer-notify\" signal is emitted in the task-thread itself. \n> > Are gst_libcamera_src_task_enter() and the task function itself (i.e. \n> > gst_libcamera_src_task_run()) are different threads? I think it's the \n> > same thread.\n> \n> The buffer-notify signal is emitted by\n> gst_libcamera_pool_release_buffer(), which is called by GStreamer from a\n> different thread.\n\nIts more complex then this of course, since its GStreamer. But the buffer-notify\nis called from a streaming thread, so it may be re-entrant to the gst_pad_push()\ncall, or its from another thread. This callback holds one of more stream lock,\nincluding ours, though stream locks are recursive for that reason.\n\n> \n> > If my understanding above is correct, the resume of task races with \n> > requestCompleted() handler thread only.\n> > \n> > Ofcourse, I need to see how the task_resume is affecting the task_run \n> > from different threads but I am still trying to grasp the issue.\n> > \n> > > handler. If the former queues completed requests or the latter queues\n> > > back buffers to the pool, and then resume the task, after the task run\n> > > handler checks the queues but before it attemps to pause the task, then\n> > > the task may be paused without noticing that more work is available.\n> > > \n> > > The most immediate way to fix this is to take the stream_lock in the\n> > > requestCompleted() and buffer-notify signal handlers, or cover the whole\n> > > task run handler with the GstLibcameraSrcState lock. This could cause\n> > > long delays in the requestCompleted() handler, so that's not a good\n> > > option.\n> > > \n> > > Instead, add a wakeup flag, preotected by the GstLibcameraSrcState lock,\n> > > that allows detection of a lost race, and retry the task run.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Changes since v1:\n> > > \n> > > - Fix incorrect wakeup and pause logic\n> > > ---\n> > > src/gstreamer/gstlibcamerasrc.cpp | 84 +++++++++++++++++++++++--------\n> > > 1 file changed, 63 insertions(+), 21 deletions(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 9ea59631a9f2..5471ab951252 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -118,7 +118,7 @@ struct GstLibcameraSrcState {\n> > > \t/*\n> > > \t * Contention on this lock_ must be minimized, as it has to be taken in\n> > > \t * the realtime-sensitive requestCompleted() handler to protect\n> > > -\t * queuedRequests_ and completedRequests_.\n> > > +\t * queuedRequests_, completedRequests_ and wakeup_.\n> > > \t *\n> > > \t * stream_lock must be taken before lock_ in contexts where both locks\n> > > \t * need to be taken. In particular, this means that the lock_ must not\n> > > @@ -130,6 +130,7 @@ struct GstLibcameraSrcState {\n> > > \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > > \tstd::queue> completedRequests_\n> > > \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > > +\tbool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n> > > \n> > > \tguint group_id_;\n> > > \n> > > @@ -250,15 +251,17 @@ 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> > > /* Must be called with stream_lock held. */\n> > > int GstLibcameraSrcState::processRequest()\n> > > {\n> > > \tstd::unique_ptr wrap;\n> > > +\tint err = 0;\n> > > \n> > > \t{\n> > > \t\tMutexLocker locker(lock_);\n> > > @@ -267,10 +270,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> > > @@ -310,7 +316,7 @@ int GstLibcameraSrcState::processRequest()\n> > > \t\treturn -EPIPE;\n> > > \t}\n> > > \n> > > -\treturn 0;\n> > > +\treturn err;\n> > > }\n> > > \n> > > static bool\n> > > @@ -374,53 +380,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_ = false;\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> > > @@ -531,7 +572,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>","headers":{"Return-Path":"","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 5D018BD808\n\tfor ;\n\tThu, 30 Jun 2022 19:41:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B281F6564C;\n\tThu, 30 Jun 2022 21:41:48 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[46.235.227.172])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5F4965633\n\tfor ;\n\tThu, 30 Jun 2022 21:41:46 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits))\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id DFBE4660196E;\n\tThu, 30 Jun 2022 20:41:45 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656618108;\n\tbh=c/gSN/kO2GxkfBzK5gIdq9PQFFSwEuzWCtkf4Y0t5y8=;\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=YLA7goMaVsz6RpPWZqSFdT50IoOkf8pOHRsF708XTmN46kffcn4z92P3qLwElBazT\n\tx+7Y+Iek4P7gYYhlImZvNdYdlD9zonlVldXUGRJCWDac3LwqhHBUZOROTKMjk4ZXuD\n\to7n0xcdeA0AHBhp2XoIavn6MAQtCYD+GK9CD6TRVUKBsT1PiYS0wjDT3xkMhPcaduw\n\tOzEsXhMi1vnZ4Jqlzd9oy75woI5P/txTuUljb1Uzkl0cNTVdscOo90900uImt9hbB8\n\t6oU+Ht2gSy5leJKIg6MV3FS8pZWn47r9XHlRfipyE7MzJaOuUCwJSoY5xWfWTlDHF0\n\tPWlDT/IG2J0Rw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1656618106;\n\tbh=c/gSN/kO2GxkfBzK5gIdq9PQFFSwEuzWCtkf4Y0t5y8=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=c67P5Crkke0FvjYF91lnKSo4buCPzq9L2X1YrTfBnZzTV/apB5ik4raF5AEWi0Q7B\n\tOUYZjs8/SLOyKQQqDrvTL8eyYXr/Xwk7buS2YqBCDIbo+S9OMOl5gKGZGhoJAKPRo0\n\t1IcUSuf6G7trPHmUByV00cCEuZaB/+ljBFm6U7CGwLRWCyrPiw625bGyy1//x/bluk\n\tk5eWBL2GQpFDGmfJrIisMgO2I3TtFpuPe0/8vBXdEZGDFqdtTDwkMyNkarx7wvXAUD\n\t9qfoLi8uscA4eysr5D5N+AktLhztVvmWnvR7RzzEcEd6/EEioMog8D4I1J8YYdfUsb\n\tTKMSjv+hyxUxw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"c67P5Crk\"; dkim-atps=neutral","Message-ID":"","To":"Laurent Pinchart , Umang Jain\n\t","Date":"Thu, 30 Jun 2022 15:41:36 -0400","In-Reply-To":"","References":"<20220630000251.31295-1-laurent.pinchart@ideasonboard.com>\n\t<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>\n\t\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23691,"web_url":"https://patchwork.libcamera.org/comment/23691/","msgid":"","date":"2022-06-30T20:17:07","subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Thu, Jun 30, 2022 at 03:38:34PM -0400, Nicolas Dufresne wrote:\n> Hi Laurent,\n> \n> this was too fast for me ;-D\n\nSorry :-)\n\n> Le jeudi 30 juin 2022 à 03:02 +0300, Laurent Pinchart a écrit :\n> > The task run function races with two other threads that want to resume\n> > the task: the requestCompleted() handler and the buffer-notify signal\n> > handler. If the former queues completed requests or the latter queues\n> > back buffers to the pool, and then resume the task, after the task run\n> > handler checks the queues but before it attemps to pause the task, then\n> > the task may be paused without noticing that more work is available.\n> > \n> > The most immediate way to fix this is to take the stream_lock in the\n> > requestCompleted() and buffer-notify signal handlers, or cover the whole\n> > task run handler with the GstLibcameraSrcState lock. This could cause\n> > long delays in the requestCompleted() handler, so that's not a good\n> > option.\n> > \n> > Instead, add a wakeup flag, preotected by the GstLibcameraSrcState lock,\n> > that allows detection of a lost race, and retry the task run.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> > \n> > - Fix incorrect wakeup and pause logic\n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 84 +++++++++++++++++++++++--------\n> > 1 file changed, 63 insertions(+), 21 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 9ea59631a9f2..5471ab951252 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -118,7 +118,7 @@ struct GstLibcameraSrcState {\n> > \t/*\n> > \t * Contention on this lock_ must be minimized, as it has to be taken in\n> > \t * the realtime-sensitive requestCompleted() handler to protect\n> > -\t * queuedRequests_ and completedRequests_.\n> > +\t * queuedRequests_, completedRequests_ and wakeup_.\n> > \t *\n> > \t * stream_lock must be taken before lock_ in contexts where both locks\n> > \t * need to be taken. In particular, this means that the lock_ must not\n> > @@ -130,6 +130,7 @@ struct GstLibcameraSrcState {\n> > \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > \tstd::queue> completedRequests_\n> > \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > +\tbool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n> \n> The method I described was all about removing this wakeup_ boolean. If the run()\n> function suspend itself right at the start (perhaps holding the locker lock (I\n> still don't like that lock name)), it shouldn't conflict with requestComplete()\n> resume. In the worst scenario, once in a while, run() will be called and will\n> suspend without doing anything. I do believe wakeup_ bool is error prone, and I\n> would prefer to see this removed.\n\nI'm giving it a try and will post a v3.\n\n> > \n> > \tguint group_id_;\n> > \n> > @@ -250,15 +251,17 @@ 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> > /* Must be called with stream_lock held. */\n> > int GstLibcameraSrcState::processRequest()\n> > {\n> > \tstd::unique_ptr wrap;\n> > +\tint err = 0;\n> > \n> > \t{\n> > \t\tMutexLocker locker(lock_);\n> > @@ -267,10 +270,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> > @@ -310,7 +316,7 @@ int GstLibcameraSrcState::processRequest()\n> > \t\treturn -EPIPE;\n> > \t}\n> > \n> > -\treturn 0;\n> > +\treturn err;\n> > }\n> > \n> > static bool\n> > @@ -374,53 +380,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_ = false;\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> > @@ -531,7 +572,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>","headers":{"Return-Path":"","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 6CCDFBE173\n\tfor ;\n\tThu, 30 Jun 2022 20:17:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C122A6564F;\n\tThu, 30 Jun 2022 22:17:29 +0200 (CEST)","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 38F8365633\n\tfor ;\n\tThu, 30 Jun 2022 22:17:28 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A1C1B45F;\n\tThu, 30 Jun 2022 22:17:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656620249;\n\tbh=WwKqma1EgonoKLDMKKzLUBNOwkSdifUOE5Hlivsp6M4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ptZDdZpy5D9yBZBzegZtqRmFD1QM5bAnwQu7F/wE6jDocZBzeMnO7of1WzFJ8Q8+h\n\tSdHKW/5T0nf0/9gtJMGOQLB/Q8LhU7F8hpwhHR3Y1Tvq1jJ+MpeUcr/vVooGGFV3vt\n\t+rEXFN69WB02hhY8VCMKceULxmBtOEgnH/1iX3/PWXg9CVyZDKSlaJ0yAm2s4X1Nma\n\tSitzI7ZIYR7/13aUXlxB/kmCQJS2zA37v9TVaiabtBTy9OV1gmjY4Zto1rUba/44cI\n\tLuUfTzT0Ly20VZrhK//ZBB+E/xLVgHoCp//i2mAFonH7tYq8imbmauPaI1aCwoUW1Y\n\tESgo6XrwfxOcw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656620247;\n\tbh=WwKqma1EgonoKLDMKKzLUBNOwkSdifUOE5Hlivsp6M4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NJLYuey/R7KQRcDJLiijjzWUk8jWFp6acqpybWkbI92JE1qj54rD2y/rVQp+6eugr\n\tctm2gQUa9u/8rccueAh6ZYTodUAB2yaqxQx58P9UcU7i75XBcndCNtSta27/E6NWIS\n\tOz82iTAstK7PCN5/shOmTmPm4LTOLRpV9w78qrEg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NJLYuey/\"; dkim-atps=neutral","Date":"Thu, 30 Jun 2022 23:17:07 +0300","To":"Nicolas Dufresne ","Message-ID":"","References":"<20220630000251.31295-1-laurent.pinchart@ideasonboard.com>\n\t<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>\n\t<4640cabc56e963855f6fb2c39dec190afce6b01b.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4640cabc56e963855f6fb2c39dec190afce6b01b.camel@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23702,"web_url":"https://patchwork.libcamera.org/comment/23702/","msgid":"<3e0fd660-98f5-b07d-92d5-01c67ca03c26@ideasonboard.com>","date":"2022-07-01T07:02:18","subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 7/1/22 01:11, Nicolas Dufresne wrote:\n> Le jeudi 30 juin 2022 à 13:48 +0300, Laurent Pinchart a écrit :\n>> On Thu, Jun 30, 2022 at 03:31:57PM +0530, Umang Jain wrote:\n>>> Hi Laurent,\n>>>\n>>> (still working through this patch)...\n>>>\n>>> On 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:\n>>>> The task run function races with two other threads that want to resume\n>>>> the task: the requestCompleted() handler and the buffer-notify signal\n>>> What do you mean by \"two other threads\" ?\n>>>\n>>> I think the \"buffer-notify\" signal is emitted in the task-thread itself.\n>>> Are gst_libcamera_src_task_enter() and the task function itself (i.e.\n>>> gst_libcamera_src_task_run()) are different threads? I think it's the\n>>> same thread.\n>> The buffer-notify signal is emitted by\n>> gst_libcamera_pool_release_buffer(), which is called by GStreamer from a\n>> different thread.\n> Its more complex then this of course, since its GStreamer. But the buffer-notify\n> is called from a streaming thread, so it may be re-entrant to the gst_pad_push()\n> call, or its from another thread. This callback holds one of more stream lock,\n> including ours, though stream locks are recursive for that reason.\n\n\nOk - then I need to see 'where' is this different thread coming from.\n\nTo me, \"buffer-notify\" is something we (as in GstLibcameraPool) define\nand a GstLibcameraPool is created inside gst_libcamera_src_task_enter() \n- signals\nand callbacks are attached here ... so it does give an \"impression\" that \nit would be\nthe same thread as the task function's one.\n\nSo, something for me to go through once I am have time...\n\n>\n>>> If my understanding above is correct, the resume of task races with\n>>> requestCompleted() handler thread only.\n>>>\n>>> Ofcourse, I need to see how the task_resume is affecting the task_run\n>>> from different threads but I am still trying to grasp the issue.\n>>>\n>>>> handler. If the former queues completed requests or the latter queues\n>>>> back buffers to the pool, and then resume the task, after the task run\n>>>> handler checks the queues but before it attemps to pause the task, then\n>>>> the task may be paused without noticing that more work is available.\n>>>>\n>>>> The most immediate way to fix this is to take the stream_lock in the\n>>>> requestCompleted() and buffer-notify signal handlers, or cover the whole\n>>>> task run handler with the GstLibcameraSrcState lock. This could cause\n>>>> long delays in the requestCompleted() handler, so that's not a good\n>>>> option.\n>>>>\n>>>> Instead, add a wakeup flag, preotected by the GstLibcameraSrcState lock,\n>>>> that allows detection of a lost race, and retry the task run.\n>>>>\n>>>> Signed-off-by: Laurent Pinchart \n>>>> ---\n>>>> Changes since v1:\n>>>>\n>>>> - Fix incorrect wakeup and pause logic\n>>>> ---\n>>>> src/gstreamer/gstlibcamerasrc.cpp | 84 +++++++++++++++++++++++--------\n>>>> 1 file changed, 63 insertions(+), 21 deletions(-)\n>>>>\n>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>>>> index 9ea59631a9f2..5471ab951252 100644\n>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>>> @@ -118,7 +118,7 @@ struct GstLibcameraSrcState {\n>>>> \t/*\n>>>> \t * Contention on this lock_ must be minimized, as it has to be taken in\n>>>> \t * the realtime-sensitive requestCompleted() handler to protect\n>>>> -\t * queuedRequests_ and completedRequests_.\n>>>> +\t * queuedRequests_, completedRequests_ and wakeup_.\n>>>> \t *\n>>>> \t * stream_lock must be taken before lock_ in contexts where both locks\n>>>> \t * need to be taken. In particular, this means that the lock_ must not\n>>>> @@ -130,6 +130,7 @@ struct GstLibcameraSrcState {\n>>>> \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n>>>> \tstd::queue> completedRequests_\n>>>> \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n>>>> +\tbool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n>>>> \n>>>> \tguint group_id_;\n>>>> \n>>>> @@ -250,15 +251,17 @@ 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>>>> /* Must be called with stream_lock held. */\n>>>> int GstLibcameraSrcState::processRequest()\n>>>> {\n>>>> \tstd::unique_ptr wrap;\n>>>> +\tint err = 0;\n>>>> \n>>>> \t{\n>>>> \t\tMutexLocker locker(lock_);\n>>>> @@ -267,10 +270,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>>>> @@ -310,7 +316,7 @@ int GstLibcameraSrcState::processRequest()\n>>>> \t\treturn -EPIPE;\n>>>> \t}\n>>>> \n>>>> -\treturn 0;\n>>>> +\treturn err;\n>>>> }\n>>>> \n>>>> static bool\n>>>> @@ -374,53 +380,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_ = false;\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>>>> @@ -531,7 +572,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);","headers":{"Return-Path":"","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 E5DFDBE173\n\tfor ;\n\tFri, 1 Jul 2022 07:02:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22C156564E;\n\tFri, 1 Jul 2022 09:02:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 098556054A\n\tfor ;\n\tFri, 1 Jul 2022 09:02:25 +0200 (CEST)","from [IPV6:2401:4900:1f3f:ca21:e286:106b:5da4:9482] (unknown\n\t[IPv6:2401:4900:1f3f:ca21:e286:106b:5da4:9482])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7090425C;\n\tFri, 1 Jul 2022 09:02:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656658946;\n\tbh=l2Ia1EMADYk/UHJnBNHTyOJiHBIpPfDm85vD5iVgElU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yvVsZIyjM3YYEXiU52WTfv0oOcZQ7ZCgHIY3h2nakv9kko4qrASB9oWZ+brYadBJY\n\tx9VwGYCVqevtbdrzwu/KhOXfu31IJLRo3Um+2gElIRUChdHi0UOu5GWN4wnvyfCBaZ\n\t46BvYfuZK/+Aysp6WWiN8yChEoAq2aueHOZX9K1go+Y9Hag8Ys86jXdC11aMrvJhIs\n\tdtlBZua6M1A5mat4OheEKHJ69hI1h4UDqtBN/mCxE46EslNRDj3YNZlUV3OKm4wPO+\n\tVVUINfScU8iRFLQ2fuGSsXkktCXFN4Jsd9lyz9P5TqQaxeYYZGv6trjtcLVx1QPs2U\n\tBuHUJJPlBuQ3w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656658944;\n\tbh=l2Ia1EMADYk/UHJnBNHTyOJiHBIpPfDm85vD5iVgElU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=lFS0X7bpaMLyNHEwKaL82vh/gAdVWwDXtfJU5Vb5coYn83mNvKZ0Vxc1yXyP1+clw\n\tvTOIpwj4K6v6ajN6+96ADDoQAG1J8RZ2qMC4o1immCoa4lh8xCNonL/jYjYdvtfJMj\n\tq96PbYvS1c2UJstPFg3Jhfllol3YrY9Rki0ikJmo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lFS0X7bp\"; dkim-atps=neutral","Message-ID":"<3e0fd660-98f5-b07d-92d5-01c67ca03c26@ideasonboard.com>","Date":"Fri, 1 Jul 2022 12:32:18 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Nicolas Dufresne ,\n\tLaurent Pinchart ","References":"<20220630000251.31295-1-laurent.pinchart@ideasonboard.com>\n\t<20220630000251.31295-13-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] gstreamer: Fix race\n\tconditions in task pause/resume","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]