[{"id":5223,"web_url":"https://patchwork.libcamera.org/comment/5223/","msgid":"<20200616212709.GG913@pendragon.ideasonboard.com>","date":"2020-06-16T21:27:09","subject":"Re: [libcamera-devel] [PATCH v5 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Tue, Jun 16, 2020 at 07:45:33PM +0000, Umang Jain wrote:\n> The pipes_ vector was initially used to store pipeline handlers\n> instances with the CameraManager when it cannot be referenced from\n> anywhere else. It was used to retrieve cameras and deleting pipeline\n> handlers when stopping the camera manager.\n> \n> In f3695e9b09ce (\"libcamera: camera_manager: Register cameras with the\n> camera manager\"), cameras started to get registered directly with camera\n> manager and in 5b02e03199b7 (\"libcamera: camera: Associate cameras with\n> their pipeline handler\") pipeline handler started to get stored in a\n\ns/handler/handlers/\n\n> std::shared_ptr<> with each camera starting to hold a strong reference\n> to its associated pipeline-handler. At this point, both the camera\n\ns/pipeline-handler/pipeline handler/\n\n> manager and the camera held a strong reference to the pipeline handler.\n> \n> Since the additional reference held by the camera manager gets released\n> only on cleanup(), this lurking reference held on pipeline handler, did\n\ns/handler,/handler/\n\n> not allow it to get destroyed the even when cameras instances have been\n\ns/the //\n\n> destroyed. This situation of having a pipeline handler instance around\n> without having a camera, may lead to problems (one of them explained\n\ns/camera,/camera/\n\n> below) especially when the camera manager is still running.\n> \n> It was noticed that, there was a dangling driver directory issue (tested\n> for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'\n> operation while the CameraManager is running. The directories were still\n> kept around even after 'unbind' because of the lurking reference of\n> pipeline handler holding onto them. That reference would clear if and\n> only if the CameraManager is stopped and then only directories were\n> getting removed in the above stated path.\n> \n> Rather than writing a fix to release the pipeline handlers' reference\n> from camera manager on camera disconnection, it is decided to eliminate\n> the pipes_ vector from CameraManager moving forwards. There is no\n> point in holding a reference to it from camera manager's point-of-view\n> at this stage. It also helps us to fix the issue as explained above.\n> \n> Now that the pipeline handler instances are referenced via cameras\n> only, it can happen that the destruction of last camera instance may\n\ns/of last/of the last/\n\n> result into destruction of pipeline handler itself. Such a possibility\n\ns/into/in/\ns/pipeline/the pipeline/\n\n> exists PipelineHandler::disconnect(), where the pipeline handler itself\n\ns/exists/exists in/\n\n> can get destroyed while removing the camera. This is acceptable as long\n> as we make sure that there is no access of pipeline handler's members\n\ns/of/to the/\n\n> later on in the code-path. Address this situation and also add a\n\ns/code-path/code path/\n\n> detailed comment about it.\n> \n> Signed-off-by: Umang Jain \n\nA commit message longer than the commit itself is often the sign a\nproblem well understood :-)\n\n> ---\n> src/libcamera/camera_manager.cpp | 8 ++------\n> src/libcamera/pipeline_handler.cpp | 18 +++++++++++++++---\n> 2 files changed, 17 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 576856a..dbdc78e 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -63,7 +63,6 @@ private:\n> \tbool initialized_;\n> \tint status_;\n> \n> -\tstd::vector> pipes_;\n> \tstd::unique_ptr enumerator_;\n> \n> \tIPAManager ipaManager_;\n> @@ -144,7 +143,6 @@ int CameraManager::Private::init()\n> \t\t\tLOG(Camera, Debug)\n> \t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> \t\t\t\t<< \"\\\" matched\";\n> -\t\t\tpipes_.push_back(std::move(pipe));\n> \t\t}\n> \t}\n> \n> @@ -158,11 +156,9 @@ void CameraManager::Private::cleanup()\n> \t/* TODO: unregister hot-plug callback here */\n> \n> \t/*\n> -\t * Release all references to cameras and pipeline handlers to ensure\n> -\t * they all get destroyed before the device enumerator deletes the\n> -\t * media devices.\n> +\t * Release all references to cameras to ensure they all get destroyed\n> +\t * before the device enumerator deletes the media devices.\n> \t */\n> -\tpipes_.clear();\n> \tcameras_.clear();\n> \n> \tenumerator_.reset(nullptr);\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index a0f6b0f..c457be9 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -559,7 +559,21 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n> */\n> void PipelineHandler::disconnect()\n> {\n> -\tfor (std::weak_ptr ptr : cameras_) {\n> +\t/*\n> +\t * Each camera holds a reference to its associated pipeline handler\n> +\t * instance. Hence, when the last camera is dropped, the pipeline\n> +\t * handler will get destroyed by the last manager_->removeCamera(camera)\n> +\t * call in the loop below.\n> +\t *\n> +\t * This is acceptable as long as we make sure that the code path does not\n> +\t * access any member of the (already destroyed)pipeline handler instance\n\ns/pipeline/ pipeline/\n\nReviewed-by: Laurent Pinchart \n\n> +\t * afterwards. Therefore, we move the cameras_ vector to a local temporary\n> +\t * container to avoid accessing freed memory later i.e. to explicitly run\n> +\t * cameras_.clear().\n> +\t */\n> +\tstd::vector> cameras{ std::move(cameras_) };\n> +\n> +\tfor (std::weak_ptr ptr : cameras) {\n> \t\tstd::shared_ptr camera = ptr.lock();\n> \t\tif (!camera)\n> \t\t\tcontinue;\n> @@ -567,8 +581,6 @@ void PipelineHandler::disconnect()\n> \t\tcamera->disconnect();\n> \t\tmanager_->removeCamera(camera.get());\n> \t}\n> -\n> -\tcameras_.clear();\n> }\n> \n> /**","headers":{"Return-Path":"","Received":["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 343C2603C4\n\tfor ;\n\tTue, 16 Jun 2020 23:27:33 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D736F9;\n\tTue, 16 Jun 2020 23:27:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Wy/fdNNi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592342852;\n\tbh=ZgcEIRc2y1oDl1dZCfp971KAbzodHHmblAHRpDnNY94=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wy/fdNNiA5XNICpfJtNceHumEeOb0P0UZGON5m1eRo1Kn0DdDxOYRdOXxXRBSL1Um\n\tsUZrPjxbEtgZzUEyYHmLqDbbo8c33/fYhKyfb6En1sCbQlCe5aoT/pOCMaiRuVURqb\n\tb4jzK4MsZnPdFge7eVtcChzc9erjAw4dM65jA1BA=","Date":"Wed, 17 Jun 2020 00:27:09 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"kieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org","Message-ID":"<20200616212709.GG913@pendragon.ideasonboard.com>","References":"<20200616194523.23268-1-email@uajain.com>\n\t<20200616194523.23268-2-email@uajain.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200616194523.23268-2-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","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","X-List-Received-Date":"Tue, 16 Jun 2020 21:27:33 -0000"}}]