[libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add thread safety annotation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 12 00:27:30 CET 2021
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 at 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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list