[{"id":21882,"web_url":"https://patchwork.libcamera.org/comment/21882/","msgid":"<20211227085646.f724haspbmiiixo3@uno.localdomain>","date":"2021-12-27T08:56:46","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline_handler:\n\tMake lock() and unlock() thread-safe","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Dec 27, 2021 at 01:12:55AM +0200, Laurent Pinchart wrote:\n> The PipelineHandler lock() and unlock() functions are documented as\n> thread-safe, but they're not. Fix them using a mutex.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> ---\n> Changes since v1:\n>\n> - Add thread safety annotation\n> - Comment that lockOwner_ does not indicate ownership of lock_\n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/pipeline_handler.cpp | 5 +++++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index a7331cedfb16..e5b8ffb4db3d 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n>\n> +#include \n> #include \n>\n> #include \n> @@ -88,7 +89,8 @@ private:\n>\n> \tconst char *name_;\n>\n> -\tbool lockOwner_;\n> +\tMutex lock_;\n> +\tbool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */\n\nTook me a while to find out what the comment means..\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n>\n> \tfriend class PipelineHandlerFactory;\n> };\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index a9241ac62979..03e4b9e66aa6 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -11,6 +11,7 @@\n> #include \n>\n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> */\n> bool PipelineHandler::lock()\n> {\n> +\tMutexLocker locker(lock_);\n> +\n> \t/* Do not allow nested locking in the same libcamera instance. */\n> \tif (lockOwner_)\n> \t\treturn false;\n> @@ -183,6 +186,8 @@ bool PipelineHandler::lock()\n> */\n> void PipelineHandler::unlock()\n> {\n> +\tMutexLocker locker(lock_);\n> +\n> \tif (!lockOwner_)\n> \t\treturn;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 597D0BE080\n\tfor ;\n\tMon, 27 Dec 2021 08:55:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02AEE6090D;\n\tMon, 27 Dec 2021 09:55:52 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C1C4605A8\n\tfor ;\n\tMon, 27 Dec 2021 09:55:50 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 7890B20009;\n\tMon, 27 Dec 2021 08:55:49 +0000 (UTC)"],"Date":"Mon, 27 Dec 2021 09:56:46 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211227085646.f724haspbmiiixo3@uno.localdomain>","References":"<20211226231255.18653-1-laurent.pinchart@ideasonboard.com>\n\t<20211226231255.18653-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211226231255.18653-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline_handler:\n\tMake lock() and unlock() thread-safe","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21888,"web_url":"https://patchwork.libcamera.org/comment/21888/","msgid":"","date":"2021-12-27T16:03:18","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline_handler:\n\tMake lock() and unlock() thread-safe","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Dec 27, 2021 at 09:56:46AM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 27, 2021 at 01:12:55AM +0200, Laurent Pinchart wrote:\n> > The PipelineHandler lock() and unlock() functions are documented as\n> > thread-safe, but they're not. Fix them using a mutex.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > Changes since v1:\n> >\n> > - Add thread safety annotation\n> > - Comment that lockOwner_ does not indicate ownership of lock_\n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > src/libcamera/pipeline_handler.cpp | 5 +++++\n> > 2 files changed, 8 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index a7331cedfb16..e5b8ffb4db3d 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -14,6 +14,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -88,7 +89,8 @@ private:\n> >\n> > \tconst char *name_;\n> >\n> > -\tbool lockOwner_;\n> > +\tMutex lock_;\n> > +\tbool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */\n> \n> Took me a while to find out what the comment means..\n\nI'm sure it could be improved, but as this will be reworked soon, I\ndecided not to spend too much time on it.\n\n> Reviewed-by: Jacopo Mondi \n> \n> > \tfriend class PipelineHandlerFactory;\n> > };\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index a9241ac62979..03e4b9e66aa6 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> > */\n> > bool PipelineHandler::lock()\n> > {\n> > +\tMutexLocker locker(lock_);\n> > +\n> > \t/* Do not allow nested locking in the same libcamera instance. */\n> > \tif (lockOwner_)\n> > \t\treturn false;\n> > @@ -183,6 +186,8 @@ bool PipelineHandler::lock()\n> > */\n> > void PipelineHandler::unlock()\n> > {\n> > +\tMutexLocker locker(lock_);\n> > +\n> > \tif (!lockOwner_)\n> > \t\treturn;\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 4334FBF415\n\tfor ;\n\tMon, 27 Dec 2021 16:03:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A96296090D;\n\tMon, 27 Dec 2021 17:03:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B47A360115\n\tfor ;\n\tMon, 27 Dec 2021 17:03:20 +0100 (CET)","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 273EF8EB;\n\tMon, 27 Dec 2021 17:03:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BFy0d0LX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1640621000;\n\tbh=UrS6w9NKkY0n1DKBzsAoWIiDzBfcFJhXGgxL39Jb41E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BFy0d0LX4rkDqqJoC2PEgQFDVcU7/qIHs/D9JG4Kk/dcB24suMuBTGVxoZRwRu2NU\n\tjh2FsMdbB+1fQDEcH4vOkFVDHTjydw6XMdmZLqHDaM+JIS2SZo5mraFU5CZmU0wbtJ\n\t0NR+ZLdpZCUZZggWc2zaxRnVBVw1ADujw2085aNM=","Date":"Mon, 27 Dec 2021 18:03:18 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211226231255.18653-1-laurent.pinchart@ideasonboard.com>\n\t<20211226231255.18653-3-laurent.pinchart@ideasonboard.com>\n\t<20211227085646.f724haspbmiiixo3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211227085646.f724haspbmiiixo3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline_handler:\n\tMake lock() and unlock() thread-safe","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21922,"web_url":"https://patchwork.libcamera.org/comment/21922/","msgid":"<15f3f499-7903-3e74-f0cf-f80e078db026@ideasonboard.com>","date":"2022-01-03T02:50:45","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline_handler:\n\tMake lock() and unlock() thread-safe","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nThank you for the patch\n\nOn 12/27/21 4:42 AM, Laurent Pinchart wrote:\n> The PipelineHandler lock() and unlock() functions are documented as\n> thread-safe, but they're not. Fix them using a mutex.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n\n\nReviewed-by: Umang Jain \n\n> ---\n> Changes since v1:\n>\n> - Add thread safety annotation\n> - Comment that lockOwner_ does not indicate ownership of lock_\n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/pipeline_handler.cpp | 5 +++++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index a7331cedfb16..e5b8ffb4db3d 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -88,7 +89,8 @@ private:\n> \n> \tconst char *name_;\n> \n> -\tbool lockOwner_;\n> +\tMutex lock_;\n> +\tbool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */\n> \n> \tfriend class PipelineHandlerFactory;\n> };\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index a9241ac62979..03e4b9e66aa6 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> */\n> bool PipelineHandler::lock()\n> {\n> +\tMutexLocker locker(lock_);\n> +\n> \t/* Do not allow nested locking in the same libcamera instance. */\n> \tif (lockOwner_)\n> \t\treturn false;\n> @@ -183,6 +186,8 @@ bool PipelineHandler::lock()\n> */\n> void PipelineHandler::unlock()\n> {\n> +\tMutexLocker locker(lock_);\n> +\n> \tif (!lockOwner_)\n> \t\treturn;\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 A07E6BF415\n\tfor ;\n\tMon, 3 Jan 2022 02:50:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFD8060868;\n\tMon, 3 Jan 2022 03:50:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8291660219\n\tfor ;\n\tMon, 3 Jan 2022 03:50:50 +0100 (CET)","from [IPv6:2401:4900:1f3e:193e:9a73:f356:8c6a:a1aa] (unknown\n\t[IPv6:2401:4900:1f3e:193e:9a73:f356:8c6a:a1aa])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 75F15CC;\n\tMon, 3 Jan 2022 03:50:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZlmZocga\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641178250;\n\tbh=D2CmN1NgaSHPi46Sc94gs9NidoVl7cb9kwpwTdUsqJg=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ZlmZocgayRQg4A9bRBBFSqTKv042DdZxkuFI4uebzua3niAqMQerXjJEP94IXJyJo\n\tSHS6kWZ0aierOyCjLuSpa3k0t57FhcGK09jJD5WKYxN8QtmSh+SqHK0JOHdetSn9h9\n\tLUUQZQbUF9CH2kg247IhrxjvsPdkL4P6K3TdTE8M=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211226231255.18653-1-laurent.pinchart@ideasonboard.com>\n\t<20211226231255.18653-3-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<15f3f499-7903-3e74-f0cf-f80e078db026@ideasonboard.com>","Date":"Mon, 3 Jan 2022 08:20:45 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211226231255.18653-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline_handler:\n\tMake lock() and unlock() thread-safe","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]