[libcamera-devel] [RFC PATCH 2/6] libcamera: base: Add mutex classes with thread safety annotations

[Umang Jain]

Hi Hiro,

On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> Mutex2 and MutexLocker2 are annotated by clang thread safety


Not sure I like the naming here. but I don't have any better suggestion 
as of now, still contemplating on that.. maybe AMutex and AMutexLocker 
to denote they are annotated?

> annotations. So we can add annotation to code where the classes
> are used.
> In the future, they will replace Mutex and MutexLocker.


Ok so I see a patch following, that does this replacement. I went into 
the territory of thinking if this annotations can be used full libcamera 
codebase. Can it be done (not as part of the series) but as a separate 
series. I am not sure how much useful it would be since annotation is 
clang-only. Let's see.

So, the series really:

         In the future, they will replace Mutex and MutexLocker in the 
android HAL layer.

>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>   include/libcamera/base/meson.build |  1 +
>   include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++


Please refer to my comment on 1/6 about putting this in 
include/libcamera/internal, maybe that's a better place?

>   include/libcamera/base/thread.h    |  5 +--
>   3 files changed, 68 insertions(+), 4 deletions(-)
>   create mode 100644 include/libcamera/base/mutex.h
>
> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> index 1a71ce5a..37c4435a 100644
> --- a/include/libcamera/base/meson.build
> +++ b/include/libcamera/base/meson.build
> @@ -13,6 +13,7 @@ libcamera_base_headers = files([
>       'flags.h',
>       'log.h',
>       'message.h',
> +    'mutex.h',
>       'object.h',
>       'private.h',
>       'semaphore.h',
> diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
> new file mode 100644
> index 00000000..d130988e
> --- /dev/null
> +++ b/include/libcamera/base/mutex.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * thread.h - Thread support


oops it should be mutex.h


Patch itself looks good to me.

> + */
> +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> +#define __LIBCAMERA_BASE_MUTEX_H__
> +
> +#include <mutex>
> +
> +#include <libcamera/base/thread_annotations.h>
> +
> +namespace libcamera {
> +
> +using Mutex = std::mutex;
> +using MutexLocker = std::unique_lock<std::mutex>;
> +
> +class CAPABILITY("mutex") Mutex2 final
> +{
> +public:
> +	void lock() ACQUIRE()
> +	{
> +		mutex_.lock();
> +	}
> +
> +	void unlock() RELEASE()
> +	{
> +		mutex_.unlock();
> +	}
> +
> +	std::mutex &get() { return mutex_; }
> +private:
> +	std::mutex mutex_;
> +};
> +
> +class SCOPED_CAPABILITY MutexLocker2 final
> +{
> +public:
> +	MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> +		: lock_(mutex.get())
> +	{
> +	}
> +
> +	~MutexLocker2() RELEASE()
> +	{
> +	}
> +
> +	void lock() ACQUIRE()
> +	{
> +		lock_.lock();
> +	}
> +
> +	void unlock() RELEASE()
> +	{
> +		lock_.unlock();
> +	}
> +
> +	std::unique_lock<std::mutex> &get() { return lock_; }
> +private:
> +	std::unique_lock<std::mutex> lock_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> index e0ca0aea..ae630563 100644
> --- a/include/libcamera/base/thread.h
> +++ b/include/libcamera/base/thread.h
> @@ -8,13 +8,13 @@
>   #define __LIBCAMERA_BASE_THREAD_H__
>   
>   #include <memory>
> -#include <mutex>
>   #include <sys/types.h>
>   #include <thread>
>   
>   #include <libcamera/base/private.h>
>   
>   #include <libcamera/base/message.h>
> +#include <libcamera/base/mutex.h>
>   #include <libcamera/base/signal.h>
>   #include <libcamera/base/utils.h>
>   
> @@ -26,9 +26,6 @@ class Object;
>   class ThreadData;
>   class ThreadMain;
>   
> -using Mutex = std::mutex;
> -using MutexLocker = std::unique_lock<std::mutex>;
> -
>   class Thread
>   {
>   public: