[libcamera-devel] [PATCH v2 06/11] android: camera_hal_manager: Add thread safety annotation

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Nov 29, 2021 at 08:44:48PM +0900, Hirokazu Honda wrote:
> This applies clang thread safety annotation to CameraHalManager.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/camera_hal_manager.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index c07684a1..5395d419 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -8,7 +8,6 @@
>  #pragma once
>  
>  #include <map>
> -#include <mutex>

I think this belongs to patch 02/11.

>  #include <stddef.h>
>  #include <tuple>
>  #include <vector>
> @@ -18,7 +17,8 @@
>  #include <system/camera_metadata.h>
>  
>  #include <libcamera/base/class.h>
> -#include <libcamera/base/thread.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread_annotations.h>

Given that mutex.h includes thread_annotations.h, and that it will
always do so (as our mutex.h is there only for the purpose of supporting
TSA), I wonder if we should include it explicitly here. Same comment for
the other patches in this series.

>  
>  #include <libcamera/camera_manager.h>
>  
> @@ -54,14 +54,14 @@ private:
>  	void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
>  	void cameraRemoved(std::shared_ptr<libcamera::Camera> cam);
>  
> -	CameraDevice *cameraDeviceFromHalId(unsigned int id);
> +	CameraDevice *cameraDeviceFromHalId(unsigned int id) LIBCAMERA_TSA_REQUIRES(mutex_);
>  
>  	std::unique_ptr<libcamera::CameraManager> cameraManager_;
>  	CameraHalConfig halConfig_;
>  
>  	const camera_module_callbacks_t *callbacks_;
> -	std::vector<std::unique_ptr<CameraDevice>> cameras_;
> -	std::map<std::string, unsigned int> cameraIdsMap_;
> +	std::vector<std::unique_ptr<CameraDevice>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> +	std::map<std::string, unsigned int> cameraIdsMap_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
>  	libcamera::Mutex mutex_;
>  
>  	unsigned int numInternalCameras_;

-- 
Regards,

Laurent Pinchart