| Message ID | 20211029041424.1430886-6-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch On 10/29/21 9:44 AM, Hirokazu Honda wrote: > This applies clang thread safety annotation to CameraDevice. > Mutex and MutexLocker there are replaced with Mutex2 and > MutexLocer2. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 26 ++++++++++++++------------ > src/android/camera_device.h | 18 +++++++++--------- > 2 files changed, 23 insertions(+), 21 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index f2e0bdbd..e05b5767 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -14,7 +14,6 @@ > #include <vector> > > #include <libcamera/base/log.h> > -#include <libcamera/base/thread.h> Ok, so this removal because we are no longer need to use Mutex and MutexLocker from thread.h, makes sense > #include <libcamera/base/utils.h> > > #include <libcamera/control_ids.h> > @@ -399,7 +398,7 @@ void CameraDevice::close() > void CameraDevice::flush() > { > { > - MutexLocker stateLock(stateMutex_); > + MutexLocker2 stateLock(stateMutex_); > if (state_ != State::Running) > return; > > @@ -409,20 +408,23 @@ void CameraDevice::flush() > worker_.stop(); > camera_->stop(); > > - MutexLocker stateLock(stateMutex_); > + MutexLocker2 stateLock(stateMutex_); > state_ = State::Stopped; > } > > void CameraDevice::stop() > { > - MutexLocker stateLock(stateMutex_); > + MutexLocker2 stateLock(stateMutex_); > if (state_ == State::Stopped) > return; > > worker_.stop(); > camera_->stop(); > > - descriptors_ = {}; > + { > + MutexLocker2 descriptorsLock(descriptorsMutex_); > + descriptors_ = {}; > + } oh, we were resetting descriptors_ without taking the lock here. I am curious, did you notice this as WARNING from annotation and then introduced this change? If yes, then annotation is already proving useful to us. > streams_.clear(); > > state_ = State::Stopped; > @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > */ > FrameBuffer *frameBuffer = nullptr; > int acquireFence = -1; > + MutexLocker2 lock(descriptor->streamsProcessMutex_); aha, one more change as result warning I suppose? > switch (cameraStream->type()) { > case CameraStream::Type::Mapped: > /* > @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * Request. > */ > LOG(HAL, Debug) << ss.str() << " (mapped)"; > - ? > descriptor->pendingStreamsToProcess_.insert( > { cameraStream, &buffer }); > continue; > @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * on the queue to be later completed. If the camera has been stopped we > * have to re-start it to be able to process the request. > */ > - MutexLocker stateLock(stateMutex_); > + MutexLocker2 stateLock(stateMutex_); > > if (state_ == State::Flushing) { > Camera3RequestDescriptor *rawDescriptor = descriptor.get(); > { > - MutexLocker descriptorsLock(descriptorsMutex_); > + MutexLocker2 descriptorsLock(descriptorsMutex_); > descriptors_.push(std::move(descriptor)); > } > abortRequest(rawDescriptor); > @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > CaptureRequest *request = descriptor->request_.get(); > > { > - MutexLocker descriptorsLock(descriptorsMutex_); > + MutexLocker2 descriptorsLock(descriptorsMutex_); > descriptors_.push(std::move(descriptor)); > } > > @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request) > } > > /* Handle post-processing. */ > - MutexLocker locker(descriptor->streamsProcessMutex_); > + MutexLocker2 locker(descriptor->streamsProcessMutex_); > > /* > * Queue all the post-processing streams request at once. The completion > @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request) > > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor) > { > - MutexLocker lock(descriptorsMutex_); > + MutexLocker2 lock(descriptorsMutex_); > descriptor->complete_ = true; > > sendCaptureResults(); > @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff > Camera3RequestDescriptor *request = streamBuffer->request; > > { > - MutexLocker locker(request->streamsProcessMutex_); > + MutexLocker2 locker(request->streamsProcessMutex_); > > request->pendingStreamsToProcess_.erase(streamBuffer->stream); > if (!request->pendingStreamsToProcess_.empty()) > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 2a414020..9feb287e 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -9,7 +9,6 @@ > > #include <map> > #include <memory> > -#include <mutex> > #include <queue> > #include <vector> > > @@ -18,7 +17,8 @@ > #include <libcamera/base/class.h> > #include <libcamera/base/log.h> > #include <libcamera/base/message.h> > -#include <libcamera/base/thread.h> > +#include <libcamera/base/mutex.h> > +#include <libcamera/base/thread_annotations.h> > > #include <libcamera/camera.h> > #include <libcamera/framebuffer.h> > @@ -83,7 +83,7 @@ private: > Running, > }; > > - void stop(); > + void stop() EXCLUDES(stateMutex_); > > std::unique_ptr<libcamera::FrameBuffer> > createFrameBuffer(const buffer_handle_t camera3buffer, > @@ -95,8 +95,8 @@ private: > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > camera3_error_msg_code code) const; > int processControls(Camera3RequestDescriptor *descriptor); > - void completeDescriptor(Camera3RequestDescriptor *descriptor); > - void sendCaptureResults(); > + void completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_); Am I right to infer that the EXCLUDES here means, descriptorsMutex_ is not held (i.e. is not locked) beforehand, calling to completeDescriptor? Since the completeDescriptor will actually lock descriptorMutex_ > + void sendCaptureResults() REQUIRES(descriptorsMutex_); And this requires descriptorsMutex_ to be locked, which makes sense. > void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer, > Camera3RequestDescriptor::Status status); > std::unique_ptr<CameraMetadata> getResultMetadata( > @@ -107,8 +107,8 @@ private: > > CameraWorker worker_; > > - libcamera::Mutex stateMutex_; /* Protects access to the camera state. */ > - State state_; > + libcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */ > + State state_ GUARDED_BY(stateMutex_); > > std::shared_ptr<libcamera::Camera> camera_; > std::unique_ptr<libcamera::CameraConfiguration> config_; > @@ -119,8 +119,8 @@ private: > > std::vector<CameraStream> streams_; > > - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ > - std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; > + libcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_); Well, the document currently states that ACQUIRED_AFTER is currently un-implemented. Secondly, I don't think we enforce a design interaction between the two mutexes currently, that requires this macro. For e.g. completeDescriptor() on a requestcomplete() path, will acquire descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any strong reasoning I am missing which led to use of ACQUIRED_AFTER for descriptorsMutex_? > + std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_ GUARDED_BY(descriptorsMutex_); This is becoming a bit harder to read (not your fault, probably mine). I should spend some time tinkering on naming these members/classes. > > std::string maker_; > std::string model_;
Hello, On Fri, Nov 12, 2021 at 12:12:57AM +0530, Umang Jain wrote: > On 10/29/21 9:44 AM, Hirokazu Honda wrote: > > This applies clang thread safety annotation to CameraDevice. > > Mutex and MutexLocker there are replaced with Mutex2 and > > MutexLocer2. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 26 ++++++++++++++------------ > > src/android/camera_device.h | 18 +++++++++--------- > > 2 files changed, 23 insertions(+), 21 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index f2e0bdbd..e05b5767 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -14,7 +14,6 @@ > > #include <vector> > > > > #include <libcamera/base/log.h> > > -#include <libcamera/base/thread.h> > > Ok, so this removal because we are no longer need to use Mutex and > MutexLocker from thread.h, makes sense > > > #include <libcamera/base/utils.h> > > > > #include <libcamera/control_ids.h> > > @@ -399,7 +398,7 @@ void CameraDevice::close() > > void CameraDevice::flush() > > { > > { > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > if (state_ != State::Running) > > return; > > > > @@ -409,20 +408,23 @@ void CameraDevice::flush() > > worker_.stop(); > > camera_->stop(); > > > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > state_ = State::Stopped; > > } > > > > void CameraDevice::stop() > > { > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > if (state_ == State::Stopped) > > return; > > > > worker_.stop(); > > camera_->stop(); > > > > - descriptors_ = {}; > > + { > > + MutexLocker2 descriptorsLock(descriptorsMutex_); > > + descriptors_ = {}; > > + } > > oh, we were resetting descriptors_ without taking the lock here. > > I am curious, did you notice this as WARNING from annotation and then > introduced this change? If yes, then annotation is already proving > useful to us. Such fixes should be split to a separate patch. Given that the camera is stopped, accessing descriptors_ without taking the lock should be safe here. The performance impact of the lock should be negligible, so it's fine to be pedantic, but assuming there would be a performance impact, would there be a way to avoid warnings without taking the lock ? > > streams_.clear(); > > > > state_ = State::Stopped; > > @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > */ > > FrameBuffer *frameBuffer = nullptr; > > int acquireFence = -1; > > + MutexLocker2 lock(descriptor->streamsProcessMutex_); > > aha, one more change as result warning I suppose? It seems to belong to patch 6/6 (or rather to a separate patch). I don't think the lock is needed (but it's probably harmless from a performance point of view). > > switch (cameraStream->type()) { > > case CameraStream::Type::Mapped: > > /* > > @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * Request. > > */ > > LOG(HAL, Debug) << ss.str() << " (mapped)"; > > - > > ? > > > descriptor->pendingStreamsToProcess_.insert( > > { cameraStream, &buffer }); > > continue; > > @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * on the queue to be later completed. If the camera has been stopped we > > * have to re-start it to be able to process the request. > > */ > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > > > if (state_ == State::Flushing) { > > Camera3RequestDescriptor *rawDescriptor = descriptor.get(); > > { > > - MutexLocker descriptorsLock(descriptorsMutex_); > > + MutexLocker2 descriptorsLock(descriptorsMutex_); > > descriptors_.push(std::move(descriptor)); > > } > > abortRequest(rawDescriptor); > > @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > CaptureRequest *request = descriptor->request_.get(); > > > > { > > - MutexLocker descriptorsLock(descriptorsMutex_); > > + MutexLocker2 descriptorsLock(descriptorsMutex_); > > descriptors_.push(std::move(descriptor)); > > } > > > > @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request) > > } > > > > /* Handle post-processing. */ > > - MutexLocker locker(descriptor->streamsProcessMutex_); > > + MutexLocker2 locker(descriptor->streamsProcessMutex_); > > > > /* > > * Queue all the post-processing streams request at once. The completion > > @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request) > > > > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor) > > { > > - MutexLocker lock(descriptorsMutex_); > > + MutexLocker2 lock(descriptorsMutex_); > > descriptor->complete_ = true; > > > > sendCaptureResults(); > > @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff > > Camera3RequestDescriptor *request = streamBuffer->request; > > > > { > > - MutexLocker locker(request->streamsProcessMutex_); > > + MutexLocker2 locker(request->streamsProcessMutex_); > > > > request->pendingStreamsToProcess_.erase(streamBuffer->stream); > > if (!request->pendingStreamsToProcess_.empty()) > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 2a414020..9feb287e 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -9,7 +9,6 @@ > > > > #include <map> > > #include <memory> > > -#include <mutex> > > #include <queue> > > #include <vector> > > > > @@ -18,7 +17,8 @@ > > #include <libcamera/base/class.h> > > #include <libcamera/base/log.h> > > #include <libcamera/base/message.h> > > -#include <libcamera/base/thread.h> > > +#include <libcamera/base/mutex.h> > > +#include <libcamera/base/thread_annotations.h> > > > > #include <libcamera/camera.h> > > #include <libcamera/framebuffer.h> > > @@ -83,7 +83,7 @@ private: > > Running, > > }; > > > > - void stop(); > > + void stop() EXCLUDES(stateMutex_); > > > > std::unique_ptr<libcamera::FrameBuffer> > > createFrameBuffer(const buffer_handle_t camera3buffer, > > @@ -95,8 +95,8 @@ private: > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > > camera3_error_msg_code code) const; > > int processControls(Camera3RequestDescriptor *descriptor); > > - void completeDescriptor(Camera3RequestDescriptor *descriptor); > > - void sendCaptureResults(); > > + void completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_); > > Am I right to infer that the EXCLUDES here means, descriptorsMutex_ is > not held (i.e. is not locked) beforehand, calling to completeDescriptor? > Since the completeDescriptor will actually lock descriptorMutex_ > > > + void sendCaptureResults() REQUIRES(descriptorsMutex_); > > And this requires descriptorsMutex_ to be locked, which makes sense. > > > void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer, > > Camera3RequestDescriptor::Status status); > > std::unique_ptr<CameraMetadata> getResultMetadata( > > @@ -107,8 +107,8 @@ private: > > > > CameraWorker worker_; > > > > - libcamera::Mutex stateMutex_; /* Protects access to the camera state. */ > > - State state_; > > + libcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */ > > + State state_ GUARDED_BY(stateMutex_); > > > > std::shared_ptr<libcamera::Camera> camera_; > > std::unique_ptr<libcamera::CameraConfiguration> config_; > > @@ -119,8 +119,8 @@ private: > > > > std::vector<CameraStream> streams_; > > > > - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ > > - std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; > > + libcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_); > > Well, the document currently states that ACQUIRED_AFTER is currently > un-implemented. > > Secondly, I don't think we enforce a design interaction between the two > mutexes currently, that requires this macro. For e.g. > completeDescriptor() on a requestcomplete() path, will acquire > descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any > strong reasoning I am missing which led to use of ACQUIRED_AFTER for > descriptorsMutex_? Acquiring locks in random orders lead to deadlocks, so it's good to document the order. > > + std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_ GUARDED_BY(descriptorsMutex_); > > This is becoming a bit harder to read (not your fault, probably mine). I > should spend some time tinkering on naming these members/classes. > > > > > std::string maker_; > > std::string model_;
Hi Umang, On Fri, Nov 12, 2021 at 3:43 AM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch > > On 10/29/21 9:44 AM, Hirokazu Honda wrote: > > This applies clang thread safety annotation to CameraDevice. > > Mutex and MutexLocker there are replaced with Mutex2 and > > MutexLocer2. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 26 ++++++++++++++------------ > > src/android/camera_device.h | 18 +++++++++--------- > > 2 files changed, 23 insertions(+), 21 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index f2e0bdbd..e05b5767 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -14,7 +14,6 @@ > > #include <vector> > > > > #include <libcamera/base/log.h> > > -#include <libcamera/base/thread.h> > > > Ok, so this removal because we are no longer need to use Mutex and > MutexLocker from thread.h, makes sense > > > #include <libcamera/base/utils.h> > > > > #include <libcamera/control_ids.h> > > @@ -399,7 +398,7 @@ void CameraDevice::close() > > void CameraDevice::flush() > > { > > { > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > if (state_ != State::Running) > > return; > > > > @@ -409,20 +408,23 @@ void CameraDevice::flush() > > worker_.stop(); > > camera_->stop(); > > > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > state_ = State::Stopped; > > } > > > > void CameraDevice::stop() > > { > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > if (state_ == State::Stopped) > > return; > > > > worker_.stop(); > > camera_->stop(); > > > > - descriptors_ = {}; > > + { > > + MutexLocker2 descriptorsLock(descriptorsMutex_); > > + descriptors_ = {}; > > + } > > > oh, we were resetting descriptors_ without taking the lock here. > > I am curious, did you notice this as WARNING from annotation and then > introduced this change? If yes, then annotation is already proving > useful to us. > Yes, if we have mistakes like this, a compilation fails. > > streams_.clear(); > > > > state_ = State::Stopped; > > @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > */ > > FrameBuffer *frameBuffer = nullptr; > > int acquireFence = -1; > > + MutexLocker2 lock(descriptor->streamsProcessMutex_); > > > aha, one more change as result warning I suppose? > > > switch (cameraStream->type()) { > > case CameraStream::Type::Mapped: > > /* > > @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * Request. > > */ > > LOG(HAL, Debug) << ss.str() << " (mapped)"; > > - > ? > > descriptor->pendingStreamsToProcess_.insert( > > { cameraStream, &buffer }); > > continue; > > @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * on the queue to be later completed. If the camera has been stopped we > > * have to re-start it to be able to process the request. > > */ > > - MutexLocker stateLock(stateMutex_); > > + MutexLocker2 stateLock(stateMutex_); > > > > if (state_ == State::Flushing) { > > Camera3RequestDescriptor *rawDescriptor = descriptor.get(); > > { > > - MutexLocker descriptorsLock(descriptorsMutex_); > > + MutexLocker2 descriptorsLock(descriptorsMutex_); > > descriptors_.push(std::move(descriptor)); > > } > > abortRequest(rawDescriptor); > > @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > CaptureRequest *request = descriptor->request_.get(); > > > > { > > - MutexLocker descriptorsLock(descriptorsMutex_); > > + MutexLocker2 descriptorsLock(descriptorsMutex_); > > descriptors_.push(std::move(descriptor)); > > } > > > > @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request) > > } > > > > /* Handle post-processing. */ > > - MutexLocker locker(descriptor->streamsProcessMutex_); > > + MutexLocker2 locker(descriptor->streamsProcessMutex_); > > > > /* > > * Queue all the post-processing streams request at once. The completion > > @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request) > > > > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor) > > { > > - MutexLocker lock(descriptorsMutex_); > > + MutexLocker2 lock(descriptorsMutex_); > > descriptor->complete_ = true; > > > > sendCaptureResults(); > > @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff > > Camera3RequestDescriptor *request = streamBuffer->request; > > > > { > > - MutexLocker locker(request->streamsProcessMutex_); > > + MutexLocker2 locker(request->streamsProcessMutex_); > > > > request->pendingStreamsToProcess_.erase(streamBuffer->stream); > > if (!request->pendingStreamsToProcess_.empty()) > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 2a414020..9feb287e 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -9,7 +9,6 @@ > > > > #include <map> > > #include <memory> > > -#include <mutex> > > #include <queue> > > #include <vector> > > > > @@ -18,7 +17,8 @@ > > #include <libcamera/base/class.h> > > #include <libcamera/base/log.h> > > #include <libcamera/base/message.h> > > -#include <libcamera/base/thread.h> > > +#include <libcamera/base/mutex.h> > > +#include <libcamera/base/thread_annotations.h> > > > > #include <libcamera/camera.h> > > #include <libcamera/framebuffer.h> > > @@ -83,7 +83,7 @@ private: > > Running, > > }; > > > > - void stop(); > > + void stop() EXCLUDES(stateMutex_); > > > > std::unique_ptr<libcamera::FrameBuffer> > > createFrameBuffer(const buffer_handle_t camera3buffer, > > @@ -95,8 +95,8 @@ private: > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > > camera3_error_msg_code code) const; > > int processControls(Camera3RequestDescriptor *descriptor); > > - void completeDescriptor(Camera3RequestDescriptor *descriptor); > > - void sendCaptureResults(); > > + void completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_); > > > Am I right to infer that the EXCLUDES here means, descriptorsMutex_ is > not held (i.e. is not locked) beforehand, calling to completeDescriptor? > Since the completeDescriptor will actually lock descriptorMutex_ > Yes, this makes sure this function is not called while descriptorsMutex_ is held, which causes deadlock. > > + void sendCaptureResults() REQUIRES(descriptorsMutex_); > > > And this requires descriptorsMutex_ to be locked, which makes sense. > > > void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer, > > Camera3RequestDescriptor::Status status); > > std::unique_ptr<CameraMetadata> getResultMetadata( > > @@ -107,8 +107,8 @@ private: > > > > CameraWorker worker_; > > > > - libcamera::Mutex stateMutex_; /* Protects access to the camera state. */ > > - State state_; > > + libcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */ > > + State state_ GUARDED_BY(stateMutex_); > > > > std::shared_ptr<libcamera::Camera> camera_; > > std::unique_ptr<libcamera::CameraConfiguration> config_; > > @@ -119,8 +119,8 @@ private: > > > > std::vector<CameraStream> streams_; > > > > - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ > > - std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; > > + libcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_); > > > Well, the document currently states that ACQUIRED_AFTER is currently > un-implemented. > Oh I don't know that. What document do you refer? I couldnm't find it in https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-acquired-after. > Secondly, I don't think we enforce a design interaction between the two > mutexes currently, that requires this macro. For e.g. > completeDescriptor() on a requestcomplete() path, will acquire > descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any > strong reasoning I am missing which led to use of ACQUIRED_AFTER for > descriptorsMutex_? > No strong reason. I think it is nicer to clarify the order. From the code, I think this is the current order used in code. Thanks, -Hiro > > + std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_ GUARDED_BY(descriptorsMutex_); > > > This is becoming a bit harder to read (not your fault, probably mine). I > should spend some time tinkering on naming these members/classes. > > > > > std::string maker_; > > std::string model_;
Hi Hiro, On 11/29/21 5:08 PM, Hirokazu Honda wrote: <snip> > >> >> Well, the document currently states that ACQUIRED_AFTER is currently >> un-implemented. >> > Oh I don't know that. What document do you refer? > I couldnm't find it in > https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-acquired-after. https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-and-acquired-after-are-currently-unimplemented > >> Secondly, I don't think we enforce a design interaction between the two >> mutexes currently, that requires this macro. For e.g. >> completeDescriptor() on a requestcomplete() path, will acquire >> descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any >> strong reasoning I am missing which led to use of ACQUIRED_AFTER for >> descriptorsMutex_? >> > No strong reason. I think it is nicer to clarify the order. > > From the code, I think this is the current order used in code. Makes sense. > > Thanks, > -Hiro >>> + std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_ GUARDED_BY(descriptorsMutex_); >> >> This is becoming a bit harder to read (not your fault, probably mine). I >> should spend some time tinkering on naming these members/classes. >> >>> std::string maker_; >>> std::string model_;
Hi Umang, On Tue, Nov 30, 2021 at 3:28 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Hiro, > > On 11/29/21 5:08 PM, Hirokazu Honda wrote: > > <snip> > > > >> > >> Well, the document currently states that ACQUIRED_AFTER is currently > >> un-implemented. > >> > > Oh I don't know that. What document do you refer? > > I couldnm't find it in > > https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-acquired-after. > > > https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-and-acquired-after-are-currently-unimplemented > Thanks! I definitely missed it. -Hiro > > > >> Secondly, I don't think we enforce a design interaction between the two > >> mutexes currently, that requires this macro. For e.g. > >> completeDescriptor() on a requestcomplete() path, will acquire > >> descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any > >> strong reasoning I am missing which led to use of ACQUIRED_AFTER for > >> descriptorsMutex_? > >> > > No strong reason. I think it is nicer to clarify the order. > > > > From the code, I think this is the current order used in code. > > > Makes sense. > > > > > Thanks, > > -Hiro > >>> + std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_ GUARDED_BY(descriptorsMutex_); > >> > >> This is becoming a bit harder to read (not your fault, probably mine). I > >> should spend some time tinkering on naming these members/classes. > >> > >>> std::string maker_; > >>> std::string model_;
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index f2e0bdbd..e05b5767 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -14,7 +14,6 @@ #include <vector> #include <libcamera/base/log.h> -#include <libcamera/base/thread.h> #include <libcamera/base/utils.h> #include <libcamera/control_ids.h> @@ -399,7 +398,7 @@ void CameraDevice::close() void CameraDevice::flush() { { - MutexLocker stateLock(stateMutex_); + MutexLocker2 stateLock(stateMutex_); if (state_ != State::Running) return; @@ -409,20 +408,23 @@ void CameraDevice::flush() worker_.stop(); camera_->stop(); - MutexLocker stateLock(stateMutex_); + MutexLocker2 stateLock(stateMutex_); state_ = State::Stopped; } void CameraDevice::stop() { - MutexLocker stateLock(stateMutex_); + MutexLocker2 stateLock(stateMutex_); if (state_ == State::Stopped) return; worker_.stop(); camera_->stop(); - descriptors_ = {}; + { + MutexLocker2 descriptorsLock(descriptorsMutex_); + descriptors_ = {}; + } streams_.clear(); state_ = State::Stopped; @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques */ FrameBuffer *frameBuffer = nullptr; int acquireFence = -1; + MutexLocker2 lock(descriptor->streamsProcessMutex_); switch (cameraStream->type()) { case CameraStream::Type::Mapped: /* @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * Request. */ LOG(HAL, Debug) << ss.str() << " (mapped)"; - descriptor->pendingStreamsToProcess_.insert( { cameraStream, &buffer }); continue; @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * on the queue to be later completed. If the camera has been stopped we * have to re-start it to be able to process the request. */ - MutexLocker stateLock(stateMutex_); + MutexLocker2 stateLock(stateMutex_); if (state_ == State::Flushing) { Camera3RequestDescriptor *rawDescriptor = descriptor.get(); { - MutexLocker descriptorsLock(descriptorsMutex_); + MutexLocker2 descriptorsLock(descriptorsMutex_); descriptors_.push(std::move(descriptor)); } abortRequest(rawDescriptor); @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques CaptureRequest *request = descriptor->request_.get(); { - MutexLocker descriptorsLock(descriptorsMutex_); + MutexLocker2 descriptorsLock(descriptorsMutex_); descriptors_.push(std::move(descriptor)); } @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request) } /* Handle post-processing. */ - MutexLocker locker(descriptor->streamsProcessMutex_); + MutexLocker2 locker(descriptor->streamsProcessMutex_); /* * Queue all the post-processing streams request at once. The completion @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request) void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor) { - MutexLocker lock(descriptorsMutex_); + MutexLocker2 lock(descriptorsMutex_); descriptor->complete_ = true; sendCaptureResults(); @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff Camera3RequestDescriptor *request = streamBuffer->request; { - MutexLocker locker(request->streamsProcessMutex_); + MutexLocker2 locker(request->streamsProcessMutex_); request->pendingStreamsToProcess_.erase(streamBuffer->stream); if (!request->pendingStreamsToProcess_.empty()) diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 2a414020..9feb287e 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -9,7 +9,6 @@ #include <map> #include <memory> -#include <mutex> #include <queue> #include <vector> @@ -18,7 +17,8 @@ #include <libcamera/base/class.h> #include <libcamera/base/log.h> #include <libcamera/base/message.h> -#include <libcamera/base/thread.h> +#include <libcamera/base/mutex.h> +#include <libcamera/base/thread_annotations.h> #include <libcamera/camera.h> #include <libcamera/framebuffer.h> @@ -83,7 +83,7 @@ private: Running, }; - void stop(); + void stop() EXCLUDES(stateMutex_); std::unique_ptr<libcamera::FrameBuffer> createFrameBuffer(const buffer_handle_t camera3buffer, @@ -95,8 +95,8 @@ private: void notifyError(uint32_t frameNumber, camera3_stream_t *stream, camera3_error_msg_code code) const; int processControls(Camera3RequestDescriptor *descriptor); - void completeDescriptor(Camera3RequestDescriptor *descriptor); - void sendCaptureResults(); + void completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_); + void sendCaptureResults() REQUIRES(descriptorsMutex_); void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer, Camera3RequestDescriptor::Status status); std::unique_ptr<CameraMetadata> getResultMetadata( @@ -107,8 +107,8 @@ private: CameraWorker worker_; - libcamera::Mutex stateMutex_; /* Protects access to the camera state. */ - State state_; + libcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */ + State state_ GUARDED_BY(stateMutex_); std::shared_ptr<libcamera::Camera> camera_; std::unique_ptr<libcamera::CameraConfiguration> config_; @@ -119,8 +119,8 @@ private: std::vector<CameraStream> streams_; - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ - std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; + libcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_); + std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_ GUARDED_BY(descriptorsMutex_); std::string maker_; std::string model_;
This applies clang thread safety annotation to CameraDevice. Mutex and MutexLocker there are replaced with Mutex2 and MutexLocer2. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_device.cpp | 26 ++++++++++++++------------ src/android/camera_device.h | 18 +++++++++--------- 2 files changed, 23 insertions(+), 21 deletions(-)