[libcamera-devel] [PATCH v5 1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Tue, Jun 16, 2020 at 07:45:33PM +0000, Umang Jain wrote:
> 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

s/handler/handlers/

> std::shared_ptr<> with each camera starting to hold a strong reference
> to its associated pipeline-handler. At this point, both the camera

s/pipeline-handler/pipeline handler/

> 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

s/handler,/handler/

> not allow it to get destroyed the even when cameras instances have been

s/the //

> destroyed. This situation of having a pipeline handler instance around
> without having a camera, may lead to problems (one of them explained

s/camera,/camera/

> 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

s/of last/of the last/

> result into destruction of pipeline handler itself. Such a possibility

s/into/in/
s/pipeline/the pipeline/

> exists PipelineHandler::disconnect(), where the pipeline handler itself

s/exists/exists in/

> 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

s/of/to the/

> later on in the code-path. Address this situation and also add a

s/code-path/code path/

> detailed comment about it.
> 
> Signed-off-by: Umang Jain <email at uajain.com>

A commit message longer than the commit itself is often the sign a
problem well understood :-)

> ---
>  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<std::shared_ptr<PipelineHandler>> pipes_;
>  	std::unique_ptr<DeviceEnumerator> 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<Camera> 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

s/pipeline/ pipeline/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	 * 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<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
> +
> +	for (std::weak_ptr<Camera> ptr : cameras) {
>  		std::shared_ptr<Camera> camera = ptr.lock();
>  		if (!camera)
>  			continue;
> @@ -567,8 +581,6 @@ void PipelineHandler::disconnect()
>  		camera->disconnect();
>  		manager_->removeCamera(camera.get());
>  	}
> -
> -	cameras_.clear();
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart