[{"id":21884,"web_url":"https://patchwork.libcamera.org/comment/21884/","msgid":"<20211227090343.4kmpe6shflm5cyxe@uno.localdomain>","date":"2021-12-27T09:03:43","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","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:54AM +0200, Laurent Pinchart wrote:\n> The MediaDevice lock is meant to prevent concurrent usage of multiple\n> cameras from the same pipeline handlers. As media devices are acquired\n> by pipeline handlers, we can't have multiple pipeline handlers trying to\n> lock the same media device. The recursive locking detection can thus be\n> moved to the pipeline handler. This simplifies the media device\n> implementation that now implements true lock semantics, and prepares for\n> support of concurrent camera usage.\n\nOut of curiosity: what do we talk about recursive locking ?\nWhere is the recursive part ? I though this is just about preventing\nto lock the same media device twice.\n\nHow do you envision per-camera locking going forward ? Currently\nCamera::acquire() locks the pipeline handlers, if the locking should\nbe moved to be per-camera, shouldn't the whole pipeline handler\nlocking mechanism be reworked ?\n\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n>\n> - Fix lock owner check un PipelineHandler::unlock()\n> ---\n> include/libcamera/internal/media_device.h | 1 -\n> include/libcamera/internal/pipeline_handler.h | 2 ++\n> src/libcamera/media_device.cpp | 14 +-------------\n> src/libcamera/pipeline_handler.cpp | 13 ++++++++++++-\n> 4 files changed, 15 insertions(+), 15 deletions(-)\n>\n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index af0b25b731eb..6e2a63f38229 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -86,7 +86,6 @@ private:\n> \tUniqueFD fd_;\n> \tbool valid_;\n> \tbool acquired_;\n> -\tbool lockOwner_;\n>\n> \tstd::map objects_;\n> \tstd::vector entities_;\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index ec986a518b5c..a7331cedfb16 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -88,6 +88,8 @@ private:\n>\n> \tconst char *name_;\n>\n> +\tbool lockOwner_;\n> +\n> \tfriend class PipelineHandlerFactory;\n> };\n>\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 0b7940182d0c..941f86c25f66 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> * populate() before the media graph can be queried.\n> */\n> MediaDevice::MediaDevice(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), valid_(false), acquired_(false),\n> -\t lockOwner_(false)\n> +\t: deviceNode_(deviceNode), valid_(false), acquired_(false)\n> {\n> }\n>\n> @@ -145,15 +144,9 @@ bool MediaDevice::lock()\n> \tif (!fd_.isValid())\n> \t\treturn false;\n>\n> -\t/* Do not allow nested locking in the same libcamera instance. */\n> -\tif (lockOwner_)\n> -\t\treturn false;\n> -\n> \tif (lockf(fd_.get(), F_TLOCK, 0))\n> \t\treturn false;\n>\n> -\tlockOwner_ = true;\n> -\n> \treturn true;\n> }\n>\n> @@ -171,11 +164,6 @@ void MediaDevice::unlock()\n> \tif (!fd_.isValid())\n> \t\treturn;\n>\n> -\tif (!lockOwner_)\n> -\t\treturn;\n> -\n> -\tlockOwner_ = false;\n> -\n> \tlockf(fd_.get(), F_ULOCK, 0);\n> }\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 0bc0e341b9e7..a9241ac62979 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * respective factories.\n> */\n> PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager)\n> +\t: manager_(manager), lockOwner_(false)\n> {\n> }\n>\n> @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> */\n> bool PipelineHandler::lock()\n> {\n> +\t/* Do not allow nested locking in the same libcamera instance. */\n> +\tif (lockOwner_)\n> +\t\treturn false;\n> +\n> \tfor (std::shared_ptr &media : mediaDevices_) {\n> \t\tif (!media->lock()) {\n> \t\t\tunlock();\n> @@ -162,6 +166,8 @@ bool PipelineHandler::lock()\n> \t\t}\n> \t}\n>\n> +\tlockOwner_ = true;\n> +\n> \treturn true;\n> }\n>\n> @@ -177,8 +183,13 @@ bool PipelineHandler::lock()\n> */\n> void PipelineHandler::unlock()\n> {\n> +\tif (!lockOwner_)\n> +\t\treturn;\n> +\n> \tfor (std::shared_ptr &media : mediaDevices_)\n> \t\tmedia->unlock();\n> +\n> +\tlockOwner_ = false;\n> }\n>\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 0B3DBBF415\n\tfor ;\n\tMon, 27 Dec 2021 09:02:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D56A605A8;\n\tMon, 27 Dec 2021 10:02:48 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B942605A8\n\tfor ;\n\tMon, 27 Dec 2021 10:02:47 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 92D2F4000C;\n\tMon, 27 Dec 2021 09:02:46 +0000 (UTC)"],"Date":"Mon, 27 Dec 2021 10:03:43 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211227090343.4kmpe6shflm5cyxe@uno.localdomain>","References":"<20211226231255.18653-1-laurent.pinchart@ideasonboard.com>\n\t<20211226231255.18653-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211226231255.18653-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","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":21887,"web_url":"https://patchwork.libcamera.org/comment/21887/","msgid":"","date":"2021-12-27T15:33:55","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","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 10:03:43AM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 27, 2021 at 01:12:54AM +0200, Laurent Pinchart wrote:\n> > The MediaDevice lock is meant to prevent concurrent usage of multiple\n> > cameras from the same pipeline handlers. As media devices are acquired\n> > by pipeline handlers, we can't have multiple pipeline handlers trying to\n> > lock the same media device. The recursive locking detection can thus be\n> > moved to the pipeline handler. This simplifies the media device\n> > implementation that now implements true lock semantics, and prepares for\n> > support of concurrent camera usage.\n> \n> Out of curiosity: what do we talk about recursive locking ?\n> Where is the recursive part ? I though this is just about preventing\n> to lock the same media device twice.\n\nIt's recursive in the sense that the lock is taken again (with a call to\nPipelineHandler::lock()) for a second camera while already held for a\nfirst camera. That corresponds to the definition of\nstd::recursive_mutex, but it's not recursive in the sense of calling\nlock() from within a lock() call.\n\n> How do you envision per-camera locking going forward ? Currently\n> Camera::acquire() locks the pipeline handlers, if the locking should\n> be moved to be per-camera, shouldn't the whole pipeline handler\n> locking mechanism be reworked ?\n\nYes, it will need to be reworked. This patch only prepares for that by\ngiving true lock semantics to MediaDevice::lock(). The rest will need to\nbe modified, with an API for pipeline handlers to expose mutual\nexclusion constraints between different cameras to applications. I'm not\nsure yet how that will look like.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> >\n> > - Fix lock owner check un PipelineHandler::unlock()\n> > ---\n> > include/libcamera/internal/media_device.h | 1 -\n> > include/libcamera/internal/pipeline_handler.h | 2 ++\n> > src/libcamera/media_device.cpp | 14 +-------------\n> > src/libcamera/pipeline_handler.cpp | 13 ++++++++++++-\n> > 4 files changed, 15 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> > index af0b25b731eb..6e2a63f38229 100644\n> > --- a/include/libcamera/internal/media_device.h\n> > +++ b/include/libcamera/internal/media_device.h\n> > @@ -86,7 +86,6 @@ private:\n> > \tUniqueFD fd_;\n> > \tbool valid_;\n> > \tbool acquired_;\n> > -\tbool lockOwner_;\n> >\n> > \tstd::map objects_;\n> > \tstd::vector entities_;\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index ec986a518b5c..a7331cedfb16 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -88,6 +88,8 @@ private:\n> >\n> > \tconst char *name_;\n> >\n> > +\tbool lockOwner_;\n> > +\n> > \tfriend class PipelineHandlerFactory;\n> > };\n> >\n> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > index 0b7940182d0c..941f86c25f66 100644\n> > --- a/src/libcamera/media_device.cpp\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> > * populate() before the media graph can be queried.\n> > */\n> > MediaDevice::MediaDevice(const std::string &deviceNode)\n> > -\t: deviceNode_(deviceNode), valid_(false), acquired_(false),\n> > -\t lockOwner_(false)\n> > +\t: deviceNode_(deviceNode), valid_(false), acquired_(false)\n> > {\n> > }\n> >\n> > @@ -145,15 +144,9 @@ bool MediaDevice::lock()\n> > \tif (!fd_.isValid())\n> > \t\treturn false;\n> >\n> > -\t/* Do not allow nested locking in the same libcamera instance. */\n> > -\tif (lockOwner_)\n> > -\t\treturn false;\n> > -\n> > \tif (lockf(fd_.get(), F_TLOCK, 0))\n> > \t\treturn false;\n> >\n> > -\tlockOwner_ = true;\n> > -\n> > \treturn true;\n> > }\n> >\n> > @@ -171,11 +164,6 @@ void MediaDevice::unlock()\n> > \tif (!fd_.isValid())\n> > \t\treturn;\n> >\n> > -\tif (!lockOwner_)\n> > -\t\treturn;\n> > -\n> > -\tlockOwner_ = false;\n> > -\n> > \tlockf(fd_.get(), F_ULOCK, 0);\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 0bc0e341b9e7..a9241ac62979 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * respective factories.\n> > */\n> > PipelineHandler::PipelineHandler(CameraManager *manager)\n> > -\t: manager_(manager)\n> > +\t: manager_(manager), lockOwner_(false)\n> > {\n> > }\n> >\n> > @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> > */\n> > bool PipelineHandler::lock()\n> > {\n> > +\t/* Do not allow nested locking in the same libcamera instance. */\n> > +\tif (lockOwner_)\n> > +\t\treturn false;\n> > +\n> > \tfor (std::shared_ptr &media : mediaDevices_) {\n> > \t\tif (!media->lock()) {\n> > \t\t\tunlock();\n> > @@ -162,6 +166,8 @@ bool PipelineHandler::lock()\n> > \t\t}\n> > \t}\n> >\n> > +\tlockOwner_ = true;\n> > +\n> > \treturn true;\n> > }\n> >\n> > @@ -177,8 +183,13 @@ bool PipelineHandler::lock()\n> > */\n> > void PipelineHandler::unlock()\n> > {\n> > +\tif (!lockOwner_)\n> > +\t\treturn;\n> > +\n> > \tfor (std::shared_ptr &media : mediaDevices_)\n> > \t\tmedia->unlock();\n> > +\n> > +\tlockOwner_ = false;\n> > }\n> >\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 8217DBE080\n\tfor ;\n\tMon, 27 Dec 2021 15:33:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5AE76090D;\n\tMon, 27 Dec 2021 16:33:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D12660115\n\tfor ;\n\tMon, 27 Dec 2021 16:33:57 +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 E76468EB;\n\tMon, 27 Dec 2021 16:33:56 +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=\"VROhBIUg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1640619237;\n\tbh=dFw56ZqgA8UFOTEAnwWWOom7d7Fd0Jqz2A8BVVCZGV0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VROhBIUg/2MK5tjlh2JtygrbAxKzfEXH4Qe3naJP3KQ4lwqJnH7pOe42a8IYnygvJ\n\tavfQ0YUaQHQNihA8R7eIy7dAQ9Znq8fizAVBxGCBUL1iob1GO4FSCIZdYmhGVxcfQS\n\twwEG+tMYIMwyKQfi7krjilWP3HFasij6nqeKVuQU=","Date":"Mon, 27 Dec 2021 17:33:55 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211226231255.18653-1-laurent.pinchart@ideasonboard.com>\n\t<20211226231255.18653-2-laurent.pinchart@ideasonboard.com>\n\t<20211227090343.4kmpe6shflm5cyxe@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211227090343.4kmpe6shflm5cyxe@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","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":21920,"web_url":"https://patchwork.libcamera.org/comment/21920/","msgid":"<164113635127.2704318.16282438355506231998@Monstersaurus>","date":"2022-01-02T15:12:31","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-12-26 23:12:54)\n> The MediaDevice lock is meant to prevent concurrent usage of multiple\n> cameras from the same pipeline handlers. As media devices are acquired\n> by pipeline handlers, we can't have multiple pipeline handlers trying to\n> lock the same media device. The recursive locking detection can thus be\n> moved to the pipeline handler. This simplifies the media device\n> implementation that now implements true lock semantics, and prepares for\n> support of concurrent camera usage.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Kieran Bingham \n\n> ---\n> Changes since v1:\n> \n> - Fix lock owner check un PipelineHandler::unlock()\n> ---\n> include/libcamera/internal/media_device.h | 1 -\n> include/libcamera/internal/pipeline_handler.h | 2 ++\n> src/libcamera/media_device.cpp | 14 +-------------\n> src/libcamera/pipeline_handler.cpp | 13 ++++++++++++-\n> 4 files changed, 15 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index af0b25b731eb..6e2a63f38229 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -86,7 +86,6 @@ private:\n> UniqueFD fd_;\n> bool valid_;\n> bool acquired_;\n> - bool lockOwner_;\n> \n> std::map objects_;\n> std::vector entities_;\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index ec986a518b5c..a7331cedfb16 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -88,6 +88,8 @@ private:\n> \n> const char *name_;\n> \n> + bool lockOwner_;\n> +\n> friend class PipelineHandlerFactory;\n> };\n> \n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 0b7940182d0c..941f86c25f66 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> * populate() before the media graph can be queried.\n> */\n> MediaDevice::MediaDevice(const std::string &deviceNode)\n> - : deviceNode_(deviceNode), valid_(false), acquired_(false),\n> - lockOwner_(false)\n> + : deviceNode_(deviceNode), valid_(false), acquired_(false)\n> {\n> }\n> \n> @@ -145,15 +144,9 @@ bool MediaDevice::lock()\n> if (!fd_.isValid())\n> return false;\n> \n> - /* Do not allow nested locking in the same libcamera instance. */\n> - if (lockOwner_)\n> - return false;\n> -\n> if (lockf(fd_.get(), F_TLOCK, 0))\n> return false;\n> \n> - lockOwner_ = true;\n> -\n> return true;\n> }\n> \n> @@ -171,11 +164,6 @@ void MediaDevice::unlock()\n> if (!fd_.isValid())\n> return;\n> \n> - if (!lockOwner_)\n> - return;\n> -\n> - lockOwner_ = false;\n> -\n> lockf(fd_.get(), F_ULOCK, 0);\n> }\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 0bc0e341b9e7..a9241ac62979 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * respective factories.\n> */\n> PipelineHandler::PipelineHandler(CameraManager *manager)\n> - : manager_(manager)\n> + : manager_(manager), lockOwner_(false)\n> {\n> }\n> \n> @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> */\n> bool PipelineHandler::lock()\n> {\n> + /* Do not allow nested locking in the same libcamera instance. */\n> + if (lockOwner_)\n> + return false;\n> +\n> for (std::shared_ptr &media : mediaDevices_) {\n> if (!media->lock()) {\n> unlock();\n> @@ -162,6 +166,8 @@ bool PipelineHandler::lock()\n> }\n> }\n> \n> + lockOwner_ = true;\n> +\n> return true;\n> }\n> \n> @@ -177,8 +183,13 @@ bool PipelineHandler::lock()\n> */\n> void PipelineHandler::unlock()\n> {\n> + if (!lockOwner_)\n> + return;\n> +\n> for (std::shared_ptr &media : mediaDevices_)\n> media->unlock();\n> +\n> + lockOwner_ = false;\n> }\n> \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 8856EBF415\n\tfor ;\n\tSun, 2 Jan 2022 15:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97E60605A8;\n\tSun, 2 Jan 2022 16:12:36 +0100 (CET)","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 7A864604F8\n\tfor ;\n\tSun, 2 Jan 2022 16:12:34 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02CBECC;\n\tSun, 2 Jan 2022 16:12:33 +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=\"PtkiCj0Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641136354;\n\tbh=tSLcfQdXYfjiGeSeECdScEwSFiux/euZDTjAkAyYcMI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=PtkiCj0Qz9r206LdU5+ycuZcDDbOpVrBQfnTjiH0g/fe3Xo405+oVXn8wucK8O7qT\n\t4UKs2CLV+jqsfQ4gdmcjpGHUqk2Nm8HELK81EfBy+8cY4AlVMnt8K0n4jygO6LKrnR\n\tG1ANBpD/Wp3Prml/NT5RkhlX4VYWlUm2zbUrWOtA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211226231255.18653-2-laurent.pinchart@ideasonboard.com>","References":"<20211226231255.18653-1-laurent.pinchart@ideasonboard.com>\n\t<20211226231255.18653-2-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sun, 02 Jan 2022 15:12:31 +0000","Message-ID":"<164113635127.2704318.16282438355506231998@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","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\" "}},{"id":21923,"web_url":"https://patchwork.libcamera.org/comment/21923/","msgid":"","date":"2022-01-03T02:53:21","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","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 12/27/21 4:42 AM, Laurent Pinchart wrote:\n> The MediaDevice lock is meant to prevent concurrent usage of multiple\n> cameras from the same pipeline handlers. As media devices are acquired\n> by pipeline handlers, we can't have multiple pipeline handlers trying to\n> lock the same media device. The recursive locking detection can thus be\n> moved to the pipeline handler. This simplifies the media device\n> implementation that now implements true lock semantics, and prepares for\n> support of concurrent camera usage.\n>\n> Signed-off-by: Laurent Pinchart \n\n\nReviewed-by: Umang Jain \n\n> ---\n> Changes since v1:\n>\n> - Fix lock owner check un PipelineHandler::unlock()\n> ---\n> include/libcamera/internal/media_device.h | 1 -\n> include/libcamera/internal/pipeline_handler.h | 2 ++\n> src/libcamera/media_device.cpp | 14 +-------------\n> src/libcamera/pipeline_handler.cpp | 13 ++++++++++++-\n> 4 files changed, 15 insertions(+), 15 deletions(-)\n>\n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index af0b25b731eb..6e2a63f38229 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -86,7 +86,6 @@ private:\n> \tUniqueFD fd_;\n> \tbool valid_;\n> \tbool acquired_;\n> -\tbool lockOwner_;\n> \n> \tstd::map objects_;\n> \tstd::vector entities_;\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index ec986a518b5c..a7331cedfb16 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -88,6 +88,8 @@ private:\n> \n> \tconst char *name_;\n> \n> +\tbool lockOwner_;\n> +\n> \tfriend class PipelineHandlerFactory;\n> };\n> \n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 0b7940182d0c..941f86c25f66 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> * populate() before the media graph can be queried.\n> */\n> MediaDevice::MediaDevice(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), valid_(false), acquired_(false),\n> -\t lockOwner_(false)\n> +\t: deviceNode_(deviceNode), valid_(false), acquired_(false)\n> {\n> }\n> \n> @@ -145,15 +144,9 @@ bool MediaDevice::lock()\n> \tif (!fd_.isValid())\n> \t\treturn false;\n> \n> -\t/* Do not allow nested locking in the same libcamera instance. */\n> -\tif (lockOwner_)\n> -\t\treturn false;\n> -\n> \tif (lockf(fd_.get(), F_TLOCK, 0))\n> \t\treturn false;\n> \n> -\tlockOwner_ = true;\n> -\n> \treturn true;\n> }\n> \n> @@ -171,11 +164,6 @@ void MediaDevice::unlock()\n> \tif (!fd_.isValid())\n> \t\treturn;\n> \n> -\tif (!lockOwner_)\n> -\t\treturn;\n> -\n> -\tlockOwner_ = false;\n> -\n> \tlockf(fd_.get(), F_ULOCK, 0);\n> }\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 0bc0e341b9e7..a9241ac62979 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * respective factories.\n> */\n> PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager)\n> +\t: manager_(manager), lockOwner_(false)\n> {\n> }\n> \n> @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> */\n> bool PipelineHandler::lock()\n> {\n> +\t/* Do not allow nested locking in the same libcamera instance. */\n> +\tif (lockOwner_)\n> +\t\treturn false;\n> +\n> \tfor (std::shared_ptr &media : mediaDevices_) {\n> \t\tif (!media->lock()) {\n> \t\t\tunlock();\n> @@ -162,6 +166,8 @@ bool PipelineHandler::lock()\n> \t\t}\n> \t}\n> \n> +\tlockOwner_ = true;\n> +\n> \treturn true;\n> }\n> \n> @@ -177,8 +183,13 @@ bool PipelineHandler::lock()\n> */\n> void PipelineHandler::unlock()\n> {\n> +\tif (!lockOwner_)\n> +\t\treturn;\n> +\n> \tfor (std::shared_ptr &media : mediaDevices_)\n> \t\tmedia->unlock();\n> +\n> +\tlockOwner_ = false;\n> }\n> \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 357B6BF415\n\tfor ;\n\tMon, 3 Jan 2022 02:53:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7881960219;\n\tMon, 3 Jan 2022 03:53:30 +0100 (CET)","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 8597460219\n\tfor ;\n\tMon, 3 Jan 2022 03:53:29 +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 4A211CC;\n\tMon, 3 Jan 2022 03:53:28 +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=\"ew4VJGFx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641178409;\n\tbh=tdQ0sMsn1UIM/TiUOoUlyHRMaY5hiOI0icqQhYaDYGA=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ew4VJGFxWupcR9vMlAr+POz8ZfPkdbak9FdqvTI414ythm//1Ju2lduJIeed/7Uv2\n\toh/pYfkrXVoyJajWPaQkD9ejAl6rkVjYWKHCOYLMJTMXGCjQiltOfOQA24eWkyalWv\n\tLcFaCX+HWwLGt4bgMkTw7dZpDlocPNjizIbohsG4=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211226231255.18653-1-laurent.pinchart@ideasonboard.com>\n\t<20211226231255.18653-2-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 3 Jan 2022 08:23:21 +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-2-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 1/2] libcamera: media_device: Move\n\trecursive lock handling to pipeline handler","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\" "}}]