From patchwork Tue Jun 16 19:45:33 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 4064 Return-Path: Received: from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 01F0261167 for ; Tue, 16 Jun 2020 21:45:34 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=uajain.com header.i=@uajain.com header.b="VED8T0Gc"; dkim-atps=neutral DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com; h=from:subject:in-reply-to:references:mime-version:to:cc:content-type: content-transfer-encoding; s=s1; bh=yx6dU7ckU6ShtvwUOxgtaiD4W7+suRpe2YocURNhkf8=; b=VED8T0GcnyhgnatFQj4aUQSxS47llaeCDxpWcTkAWm/kdJztg9J6rbEJyYyresCP0vWz dl7IMswKa/Ru5Z/i5+RcJ1Y8uNVhQe7VmS796k1Yw8uCvRVwQ3Mno/xID4wCV2y2RlgHwj wjakyLSTACzXrH8o4j5eSvpY3787WrOG4= Received: by filterdrecv-p3mdw1-6f5df8956d-4d2bg with SMTP id filterdrecv-p3mdw1-6f5df8956d-4d2bg-21-5EE9215D-2A 2020-06-16 19:45:33.47457207 +0000 UTC m=+1121502.324567696 Received: from mail.uajain.com (unknown) by ismtpd0004p1maa1.sendgrid.net (SG) with ESMTP id _BnG-p6kSZuoZoFr7UImzA Tue, 16 Jun 2020 19:45:33.074 +0000 (UTC) From: Umang Jain Date: Tue, 16 Jun 2020 19:45:33 +0000 (UTC) Message-Id: <20200616194523.23268-2-email@uajain.com> In-Reply-To: <20200616194523.23268-1-email@uajain.com> References: <20200616194523.23268-1-email@uajain.com> Mime-Version: 1.0 X-SG-EID: 1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPc9vLW9W8H4bP+KZ0PPVtun2GUUTio/M2EKpbAqho5duHtKP8eDtaspFrCVhJ3PR4vH4mRMd6r9cvmCvIQ41w0V5s6xB89i6oWBMoIuY7NpmIkHD8QkD8HoD7yLL+opDFU9zWt6CvP0UsGm++XnnIalUSTSuKsRfmnw79WvGN0AY/sQXGanPBxpsGKAHATyFM45p9lFjfFgueV2J9k7IurlA== To: laurent.pinchart@ideasonboard.com, kieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org Subject: [libcamera-devel] [PATCH v5 1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Jun 2020 19:45:35 -0000 The pipes_ vector was initially used to store pipeline handlers instances with the CameraManager when it cannot be referenced from anywhere else. It was used to retrieve cameras and deleting pipeline handlers when stopping the camera manager. In f3695e9b09ce ("libcamera: camera_manager: Register cameras with the camera manager"), cameras started to get registered directly with camera manager and in 5b02e03199b7 ("libcamera: camera: Associate cameras with their pipeline handler") pipeline handler started to get stored in a std::shared_ptr<> with each camera starting to hold a strong reference to its associated pipeline-handler. At this point, both the camera manager and the camera held a strong reference to the pipeline handler. Since the additional reference held by the camera manager gets released only on cleanup(), this lurking reference held on pipeline handler, did not allow it to get destroyed the even when cameras instances have been destroyed. This situation of having a pipeline handler instance around without having a camera, may lead to problems (one of them explained below) especially when the camera manager is still running. It was noticed that, there was a dangling driver directory issue (tested for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind' operation while the CameraManager is running. The directories were still kept around even after 'unbind' because of the lurking reference of pipeline handler holding onto them. That reference would clear if and only if the CameraManager is stopped and then only directories were getting removed in the above stated path. Rather than writing a fix to release the pipeline handlers' reference from camera manager on camera disconnection, it is decided to eliminate the pipes_ vector from CameraManager moving forwards. There is no point in holding a reference to it from camera manager's point-of-view at this stage. It also helps us to fix the issue as explained above. Now that the pipeline handler instances are referenced via cameras only, it can happen that the destruction of last camera instance may result into destruction of pipeline handler itself. Such a possibility exists PipelineHandler::disconnect(), where the pipeline handler itself can get destroyed while removing the camera. This is acceptable as long as we make sure that there is no access of pipeline handler's members later on in the code-path. Address this situation and also add a detailed comment about it. Signed-off-by: Umang Jain Reviewed-by: Laurent Pinchart --- src/libcamera/camera_manager.cpp | 8 ++------ src/libcamera/pipeline_handler.cpp | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 576856a..dbdc78e 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -63,7 +63,6 @@ private: bool initialized_; int status_; - std::vector> pipes_; std::unique_ptr enumerator_; IPAManager ipaManager_; @@ -144,7 +143,6 @@ int CameraManager::Private::init() LOG(Camera, Debug) << "Pipeline handler \"" << factory->name() << "\" matched"; - pipes_.push_back(std::move(pipe)); } } @@ -158,11 +156,9 @@ void CameraManager::Private::cleanup() /* TODO: unregister hot-plug callback here */ /* - * Release all references to cameras and pipeline handlers to ensure - * they all get destroyed before the device enumerator deletes the - * media devices. + * Release all references to cameras to ensure they all get destroyed + * before the device enumerator deletes the media devices. */ - pipes_.clear(); cameras_.clear(); enumerator_.reset(nullptr); diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index a0f6b0f..c457be9 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -559,7 +559,21 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media) */ void PipelineHandler::disconnect() { - for (std::weak_ptr ptr : cameras_) { + /* + * Each camera holds a reference to its associated pipeline handler + * instance. Hence, when the last camera is dropped, the pipeline + * handler will get destroyed by the last manager_->removeCamera(camera) + * call in the loop below. + * + * This is acceptable as long as we make sure that the code path does not + * access any member of the (already destroyed)pipeline handler instance + * afterwards. Therefore, we move the cameras_ vector to a local temporary + * container to avoid accessing freed memory later i.e. to explicitly run + * cameras_.clear(). + */ + std::vector> cameras{ std::move(cameras_) }; + + for (std::weak_ptr ptr : cameras) { std::shared_ptr camera = ptr.lock(); if (!camera) continue; @@ -567,8 +581,6 @@ void PipelineHandler::disconnect() camera->disconnect(); manager_->removeCamera(camera.get()); } - - cameras_.clear(); } /**