[{"id":23674,"web_url":"https://patchwork.libcamera.org/comment/23674/","msgid":"<9bca8c41-db86-66a4-8891-88e0b93beed8@ideasonboard.com>","date":"2022-06-30T08:34:54","subject":"Re: [libcamera-devel] [PATCH v2 08/12] gstreamer: Use dedicated\n\tlock for request queues","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch,\n\nOn 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:\n> Add a new lock to the GstLibcameraSrcState class to protect the queued\n> and completed requests queues. This replaces the GstObject lock, and\n> minimizes the lock contention between the request completion handler and\n> the task run handler as the former must run as fast as possible.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Nicolas Dufresne \n\n\nReviewed-by: Umang Jain \n\n> ---\n> Changes since v1:\n>\n> - Add comment about lock_\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 44 +++++++++++++++++++++++--------\n> 1 file changed, 33 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 8c1cd7017d98..6f9a03c515d2 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -32,6 +32,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> #include \n> @@ -111,8 +113,18 @@ struct GstLibcameraSrcState {\n> \tstd::shared_ptr cam_;\n> \tstd::unique_ptr config_;\n> \tstd::vector srcpads_;\n> -\tstd::queue> queuedRequests_;\n> -\tstd::queue> completedRequests_;\n> +\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 */\n> +\tMutex lock_;\n> +\tstd::queue> queuedRequests_\n> +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> +\tstd::queue> completedRequests_\n> +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> +\n> \tguint group_id_;\n> \n> \tvoid requestCompleted(Request *request);\n> @@ -155,12 +167,15 @@ GstStaticPadTemplate request_src_template = {\n> void\n> GstLibcameraSrcState::requestCompleted(Request *request)\n> {\n> -\tGLibLocker lock(GST_OBJECT(src_));\n> -\n> \tGST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> \n> -\tstd::unique_ptr wrap = std::move(queuedRequests_.front());\n> -\tqueuedRequests_.pop();\n> +\tstd::unique_ptr wrap;\n> +\n> +\t{\n> +\t\tMutexLocker locker(lock_);\n> +\t\twrap = std::move(queuedRequests_.front());\n> +\t\tqueuedRequests_.pop();\n> +\t}\n> \n> \tg_return_if_fail(wrap->request_.get() == request);\n> \n> @@ -183,7 +198,10 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> \t\twrap->latency_ = sys_now - timestamp;\n> \t}\n> \n> -\tcompletedRequests_.push(std::move(wrap));\n> +\t{\n> +\t\tMutexLocker locker(lock_);\n> +\t\tcompletedRequests_.push(std::move(wrap));\n> +\t}\n> \n> \tgst_task_resume(src_->task);\n> }\n> @@ -289,16 +307,17 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t}\n> \n> \tif (wrap) {\n> -\t\tGLibLocker lock(GST_OBJECT(self));\n> \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> \t\tstate->cam_->queueRequest(wrap->request_.get());\n> +\n> +\t\tMutexLocker locker(state->lock_);\n> \t\tstate->queuedRequests_.push(std::move(wrap));\n> \n> \t\t/* The RequestWrap will be deleted in the completion handler. */\n> \t}\n> \n> \t{\n> -\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\tMutexLocker locker(state->lock_);\n> \n> \t\tif (!state->completedRequests_.empty()) {\n> \t\t\twrap = std::move(state->completedRequests_.front());\n> @@ -358,7 +377,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t\tbool do_pause;\n> \n> \t\t{\n> -\t\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\t\tMutexLocker locker(state->lock_);\n> \t\t\tdo_pause = state->completedRequests_.empty();\n> \t\t}\n> \n> @@ -513,7 +532,10 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,\n> \n> \tstate->cam_->stop();\n> \n> -\tstate->completedRequests_ = {};\n> +\t{\n> +\t\tMutexLocker locker(state->lock_);\n> +\t\tstate->completedRequests_ = {};\n> +\t}\n> \n> \tfor (GstPad *srcpad : state->srcpads_)\n> \t\tgst_libcamera_pad_set_pool(srcpad, nullptr);","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 A28AFBD808\n\tfor ;\n\tThu, 30 Jun 2022 08:35:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 096C36564E;\n\tThu, 30 Jun 2022 10:35:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07A016054C\n\tfor ;\n\tThu, 30 Jun 2022 10:35:00 +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 EAD2E45F;\n\tThu, 30 Jun 2022 10:34:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656578102;\n\tbh=AivNuueVPN94v9AnUSCuQtZDQ35SNlhQMGimH2Of11c=;\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=O/K/2F5g2L85lzq+flN79Cm9vpxSjplnxWvBFRScmLAKZwjoiHDKXO6FWsHI6xRwo\n\t9c1fuVlGIdmM8T/3TSkRACD7lc1b2zkdSHf3SxbCWD10FQ/+HWj05gMONt4f8EJC/z\n\tJkbpcT79h+32ZDal6KoQKEj1wzynMNaobdP+AWeRmYja6qECDCRRkgt5V3kVUfPcrQ\n\tAk7mXWlsD52OGM/otozdHyAedpHTN5YM+ge3S0D2CP0Nq3q9Qy5zk4RafnhGU/6qWJ\n\tnF09fX7B7cgigpYrYQAzsS+0eQGfjwXpzwj4htprWioOX3YqW4JnJ7AfjF0ZqI3Gy7\n\taSLNMw6uOmGEg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656578099;\n\tbh=AivNuueVPN94v9AnUSCuQtZDQ35SNlhQMGimH2Of11c=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=JJYsUNg67YS7Z7200LXBZj2S43eEWnNWQsYT7TLL47q2bFpCqLVuIj2st6n6AVlGE\n\tu4Ptt9uVsX4FlLYMn/5wa4xLtdN0tZRmlo6Y21jKUgi/PHMx0ZX6tPKgIvsCRVPB9r\n\tPCS6ptie0UlM3blq2INW2rSxwsx7XNkPbjBmf7Qw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JJYsUNg6\"; dkim-atps=neutral","Message-ID":"<9bca8c41-db86-66a4-8891-88e0b93beed8@ideasonboard.com>","Date":"Thu, 30 Jun 2022 14:04:54 +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-9-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20220630000251.31295-9-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] gstreamer: Use dedicated\n\tlock for request queues","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\" "}}]