[libcamera-devel,RFC,3/6] android: camera_hal_manager: Add thread safety annotation
diff mbox series

Message ID 20211029041424.1430886-4-hiroh@chromium.org
State Superseded
Headers show
Series
  • Introduce clang thread safety annotations
Related show

Commit Message

Hirokazu Honda Oct. 29, 2021, 4:14 a.m. UTC 
This applies clang thread safety annotation to CameraHalManager.
Mutex and MutexLocker there are replaced with Mutex2 and
MutexLocer2.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_hal_manager.cpp | 10 +++++-----
 src/android/camera_hal_manager.h   | 14 ++++++--------
 2 files changed, 11 insertions(+), 13 deletions(-)

Comments

Umang Jain Nov. 11, 2021, 5:10 p.m. UTC | #1 
Hi Hiro

On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> This applies clang thread safety annotation to CameraHalManager.
> Mutex and MutexLocker there are replaced with Mutex2 and
> MutexLocer2.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>


Patch looks good to me

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/android/camera_hal_manager.cpp | 10 +++++-----
>   src/android/camera_hal_manager.h   | 14 ++++++--------
>   2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 5f7bfe26..26715a5f 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -75,7 +75,7 @@ int CameraHalManager::init()
>   std::tuple<CameraDevice *, int>
>   CameraHalManager::open(unsigned int id, const hw_module_t *hardwareModule)
>   {
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	if (!callbacks_) {
>   		LOG(HAL, Error) << "Can't open camera before callbacks are set";
> @@ -103,7 +103,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>   	bool isCameraExternal = false;
>   	bool isCameraNew = false;
>   
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	/*
>   	 * Each camera is assigned a unique integer ID when it is seen for the
> @@ -198,7 +198,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>   
>   void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
>   {
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
>   				 [&cam](const std::unique_ptr<CameraDevice> &camera) {
> @@ -257,7 +257,7 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
>   	if (!info)
>   		return -EINVAL;
>   
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	CameraDevice *camera = cameraDeviceFromHalId(id);
>   	if (!camera) {
> @@ -280,7 +280,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
>   {
>   	callbacks_ = callbacks;
>   
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	/*
>   	 * Some external cameras may have been identified before the callbacks_
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index 3f6d302a..9b8c3a1f 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -8,7 +8,6 @@
>   #define __ANDROID_CAMERA_MANAGER_H__
>   
>   #include <map>
> -#include <mutex>
>   #include <stddef.h>
>   #include <tuple>
>   #include <vector>
> @@ -18,6 +17,8 @@
>   #include <system/camera_metadata.h>
>   
>   #include <libcamera/base/class.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread_annotations.h>
>   
>   #include <libcamera/camera_manager.h>
>   
> @@ -44,9 +45,6 @@ public:
>   private:
>   	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraHalManager)
>   
> -	using Mutex = std::mutex;
> -	using MutexLocker = std::unique_lock<std::mutex>;
> -
>   	static constexpr unsigned int firstExternalCameraId_ = 1000;
>   
>   	CameraHalManager();
> @@ -56,15 +54,15 @@ 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) 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_;
> -	Mutex mutex_;
> +	std::vector<std::unique_ptr<CameraDevice>> cameras_ GUARDED_BY(mutex_);
> +	std::map<std::string, unsigned int> cameraIdsMap_ GUARDED_BY(mutex_);
> +	libcamera::Mutex2 mutex_;
>   
>   	unsigned int numInternalCameras_;
>   	unsigned int nextExternalCameraId_;
Laurent Pinchart Nov. 11, 2021, 10:54 p.m. UTC | #2 
Hi Hiro,

Thank you for the patch.

On Fri, Oct 29, 2021 at 01:14:21PM +0900, Hirokazu Honda wrote:
> This applies clang thread safety annotation to CameraHalManager.
> Mutex and MutexLocker there are replaced with Mutex2 and
> MutexLocer2.

s/Locer/Locker/

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

This looks fine, except for the *2 suffix that I'd like to avoid. This
is discussed in 2/6, so for this patch,

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

> ---
>  src/android/camera_hal_manager.cpp | 10 +++++-----
>  src/android/camera_hal_manager.h   | 14 ++++++--------
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 5f7bfe26..26715a5f 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -75,7 +75,7 @@ int CameraHalManager::init()
>  std::tuple<CameraDevice *, int>
>  CameraHalManager::open(unsigned int id, const hw_module_t *hardwareModule)
>  {
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>  
>  	if (!callbacks_) {
>  		LOG(HAL, Error) << "Can't open camera before callbacks are set";
> @@ -103,7 +103,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>  	bool isCameraExternal = false;
>  	bool isCameraNew = false;
>  
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>  
>  	/*
>  	 * Each camera is assigned a unique integer ID when it is seen for the
> @@ -198,7 +198,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>  
>  void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
>  {
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>  
>  	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
>  				 [&cam](const std::unique_ptr<CameraDevice> &camera) {
> @@ -257,7 +257,7 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
>  	if (!info)
>  		return -EINVAL;
>  
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>  
>  	CameraDevice *camera = cameraDeviceFromHalId(id);
>  	if (!camera) {
> @@ -280,7 +280,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
>  {
>  	callbacks_ = callbacks;
>  
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>  
>  	/*
>  	 * Some external cameras may have been identified before the callbacks_
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index 3f6d302a..9b8c3a1f 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -8,7 +8,6 @@
>  #define __ANDROID_CAMERA_MANAGER_H__
>  
>  #include <map>
> -#include <mutex>
>  #include <stddef.h>
>  #include <tuple>
>  #include <vector>
> @@ -18,6 +17,8 @@
>  #include <system/camera_metadata.h>
>  
>  #include <libcamera/base/class.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread_annotations.h>
>  
>  #include <libcamera/camera_manager.h>
>  
> @@ -44,9 +45,6 @@ public:
>  private:
>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraHalManager)
>  
> -	using Mutex = std::mutex;
> -	using MutexLocker = std::unique_lock<std::mutex>;
> -
>  	static constexpr unsigned int firstExternalCameraId_ = 1000;
>  
>  	CameraHalManager();
> @@ -56,15 +54,15 @@ 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) 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_;
> -	Mutex mutex_;
> +	std::vector<std::unique_ptr<CameraDevice>> cameras_ GUARDED_BY(mutex_);
> +	std::map<std::string, unsigned int> cameraIdsMap_ GUARDED_BY(mutex_);
> +	libcamera::Mutex2 mutex_;
>  
>  	unsigned int numInternalCameras_;
>  	unsigned int nextExternalCameraId_;

Patch
diff mbox series 

diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
index 5f7bfe26..26715a5f 100644
--- a/src/android/camera_hal_manager.cpp
+++ b/src/android/camera_hal_manager.cpp
@@ -75,7 +75,7 @@  int CameraHalManager::init()
 std::tuple<CameraDevice *, int>
 CameraHalManager::open(unsigned int id, const hw_module_t *hardwareModule)
 {
-	MutexLocker locker(mutex_);
+	MutexLocker2 locker(mutex_);
 
 	if (!callbacks_) {
 		LOG(HAL, Error) << "Can't open camera before callbacks are set";
@@ -103,7 +103,7 @@  void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
 	bool isCameraExternal = false;
 	bool isCameraNew = false;
 
-	MutexLocker locker(mutex_);
+	MutexLocker2 locker(mutex_);
 
 	/*
 	 * Each camera is assigned a unique integer ID when it is seen for the
@@ -198,7 +198,7 @@  void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
 
 void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
 {
-	MutexLocker locker(mutex_);
+	MutexLocker2 locker(mutex_);
 
 	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
 				 [&cam](const std::unique_ptr<CameraDevice> &camera) {
@@ -257,7 +257,7 @@  int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
 	if (!info)
 		return -EINVAL;
 
-	MutexLocker locker(mutex_);
+	MutexLocker2 locker(mutex_);
 
 	CameraDevice *camera = cameraDeviceFromHalId(id);
 	if (!camera) {
@@ -280,7 +280,7 @@  void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
 {
 	callbacks_ = callbacks;
 
-	MutexLocker locker(mutex_);
+	MutexLocker2 locker(mutex_);
 
 	/*
 	 * Some external cameras may have been identified before the callbacks_
diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
index 3f6d302a..9b8c3a1f 100644
--- a/src/android/camera_hal_manager.h
+++ b/src/android/camera_hal_manager.h
@@ -8,7 +8,6 @@ 
 #define __ANDROID_CAMERA_MANAGER_H__
 
 #include <map>
-#include <mutex>
 #include <stddef.h>
 #include <tuple>
 #include <vector>
@@ -18,6 +17,8 @@ 
 #include <system/camera_metadata.h>
 
 #include <libcamera/base/class.h>
+#include <libcamera/base/mutex.h>
+#include <libcamera/base/thread_annotations.h>
 
 #include <libcamera/camera_manager.h>
 
@@ -44,9 +45,6 @@  public:
 private:
 	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraHalManager)
 
-	using Mutex = std::mutex;
-	using MutexLocker = std::unique_lock<std::mutex>;
-
 	static constexpr unsigned int firstExternalCameraId_ = 1000;
 
 	CameraHalManager();
@@ -56,15 +54,15 @@  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) 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_;
-	Mutex mutex_;
+	std::vector<std::unique_ptr<CameraDevice>> cameras_ GUARDED_BY(mutex_);
+	std::map<std::string, unsigned int> cameraIdsMap_ GUARDED_BY(mutex_);
+	libcamera::Mutex2 mutex_;
 
 	unsigned int numInternalCameras_;
 	unsigned int nextExternalCameraId_;