[{"id":30966,"web_url":"https://patchwork.libcamera.org/comment/30966/","msgid":"<20240829214228.GH15799@pendragon.ideasonboard.com>","date":"2024-08-29T21:42:28","subject":"Re: [PATCH v2 2/3] camera: Use invokeMethod() for pipe_->acquire()\n\tand pipe_->release()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nThank you for the patch.\n\nOn Tue, Aug 27, 2024 at 06:42:54PM +0200, Hans de Goede wrote:\n> The uvcvideo driver needs to open / close its /dev/video# node from\n> pipe_->acquireDevices() / pipe_->releaseDevices().\n> \n> V4L2VideoDevice::open() creates an EventNotifier and this notifier needs\n> to be created from the CameraManager thread.\n> \n> Use invokeMethod() for pipe_->acquire() and pipe_->release() so that\n> the EventNotifiers are created from the CameraManager thread context.\n> \n> Running pipe_->acquire() and pipe_->release() from the CameraManager\n> thread context serializes all calls to them. Drop PipelineHandler::lock_\n> this now is no longer necessary and update the \"\\context\" part of\n> the documentation for acquire[Device]() and release[Device]() to match.\n> \n> Note the delayed opening of /dev/video# is a special case because\n> the kernel uvcvideo driver powers on the USB device as soon as /dev/video#\n> is opened. This behavior should *not* be copied by other pipeline handlers.\n\nThe real reason why this shouldn't be copied is not so much that the\nuvcvideo driver is a special case, but more that a proper API is needed\nin libcamera. This text is fine for now though, let's keep the ball\nrolling on the public API improvements, instead of nitpicking on commit\nmessages :-)\n\n> Signed-off-by: Hans de Goede \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> Changes in v2:\n> - Add a note to the commit message that opening/closing /dev/video# from\n> acquire()/release() as the uvcvideo pipeline handler does is an exception\n> and that this behavior should not be copied by other pipeline handlers\n> - Drop lock_, update \"\\context\" in doxygen docs\n> ---\n> include/libcamera/internal/pipeline_handler.h | 5 +----\n> src/libcamera/camera.cpp | 6 ++++--\n> src/libcamera/pipeline_handler.cpp | 12 ++++++------\n> 3 files changed, 11 insertions(+), 12 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 597f7c3e..c33cf715 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -14,7 +14,6 @@\n> #include \n> #include \n> \n> -#include \n> #include \n> \n> #include \n> @@ -99,9 +98,7 @@ private:\n> \tstd::queue waitingRequests_;\n> \n> \tconst char *name_;\n> -\n> -\tMutex lock_;\n> -\tunsigned int useCount_ LIBCAMERA_TSA_GUARDED_BY(lock_);\n> +\tunsigned int useCount_;\n> \n> \tfriend class PipelineHandlerFactoryBase;\n> };\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 4e393f89..61925e83 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -995,7 +995,8 @@ int Camera::acquire()\n> \tif (ret < 0)\n> \t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> -\tif (!d->pipe_->acquire(this)) {\n> +\tif (!d->pipe_->invokeMethod(&PipelineHandler::acquire,\n> +\t\t\t\t ConnectionTypeBlocking, this)) {\n> \t\tLOG(Camera, Info)\n> \t\t\t<< \"Pipeline handler in use by another process\";\n> \t\treturn -EBUSY;\n> @@ -1030,7 +1031,8 @@ int Camera::release()\n> \t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> \tif (d->isAcquired())\n> -\t\td->pipe_->release(this);\n> +\t\td->pipe_->invokeMethod(&PipelineHandler::release,\n> +\t\t\t\t ConnectionTypeBlocking, this);\n> \n> \td->setState(Private::CameraAvailable);\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 861815cb..b18b6d0b 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -157,7 +157,7 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> * Pipeline handlers shall not call this function directly as the Camera class\n> * handles access internally.\n> *\n> - * \\context This function is \\threadsafe.\n> + * \\context This function is called from the CameraManager thread.\n> *\n> * \\return True if the pipeline handler was acquired, false if another process\n> * has already acquired it\n> @@ -165,8 +165,6 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> */\n> bool PipelineHandler::acquire(Camera *camera)\n> {\n> -\tMutexLocker locker(lock_);\n> -\n> \tif (useCount_ == 0) {\n> \t\tfor (std::shared_ptr &media : mediaDevices_) {\n> \t\t\tif (!media->lock()) {\n> @@ -199,14 +197,12 @@ bool PipelineHandler::acquire(Camera *camera)\n> * Pipeline handlers shall not call this function directly as the Camera class\n> * handles access internally.\n> *\n> - * \\context This function is \\threadsafe.\n> + * \\context This function is called from the CameraManager thread.\n> *\n> * \\sa acquire()\n> */\n> void PipelineHandler::release(Camera *camera)\n> {\n> -\tMutexLocker locker(lock_);\n> -\n> \tASSERT(useCount_);\n> \n> \treleaseDevice(camera);\n> @@ -230,6 +226,8 @@ void PipelineHandler::release(Camera *camera)\n> * powers on the USB device as soon as /dev/video# is opened. This behavior\n> * should *not* be copied by other pipeline handlers.\n> *\n> + * \\context This function is called from the CameraManager thread.\n> + *\n> * \\return True on success, false on failure\n> * \\sa releaseDevice()\n> */\n> @@ -250,6 +248,8 @@ bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)\n> * release them until releaseDevice() has been called for all previously\n> * acquired cameras.\n> *\n> + * \\context This function is called from the CameraManager thread.\n> + *\n> * \\sa acquireDevice()\n> */\n> void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)","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 1A279C324C\n\tfor ;\n\tThu, 29 Aug 2024 21:43:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FA2563466;\n\tThu, 29 Aug 2024 23:43:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 69A3A63458\n\tfor ;\n\tThu, 29 Aug 2024 23:42:59 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D149226;\n\tThu, 29 Aug 2024 23:41:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"McTIY+1a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724967710;\n\tbh=lf+lTFs/fuUR7n3BiyncGUp6SxBmK6FWDlDSMw00qIs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=McTIY+1a3hy+wdu+NKmqFyGDs2FjIbkPrN/EmGS3YRzJCOGBQii+81k6RNjR66i8M\n\tiNNcEVD9GBNIK/3/bXpInlMlrRuRr35Sdqh3thaIHoJUwZER11asSkqoV644XZRCwi\n\t00xirgbaAauHiXZyI94SI1v9R/12dd8EdaPsp5bY=","Date":"Fri, 30 Aug 2024 00:42:28 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","Subject":"Re: [PATCH v2 2/3] camera: Use invokeMethod() for pipe_->acquire()\n\tand pipe_->release()","Message-ID":"<20240829214228.GH15799@pendragon.ideasonboard.com>","References":"<20240827164255.314432-1-hdegoede@redhat.com>\n\t<20240827164255.314432-3-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240827164255.314432-3-hdegoede@redhat.com>","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\" "}}]