[{"id":20879,"web_url":"https://patchwork.libcamera.org/comment/20879/","msgid":"<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>","date":"2021-11-11T18:42:57","subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch\n\nOn 10/29/21 9:44 AM, Hirokazu Honda wrote:\n> This applies clang thread safety annotation to CameraDevice.\n> Mutex and MutexLocker there are replaced with Mutex2 and\n> MutexLocer2.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 26 ++++++++++++++------------\n> src/android/camera_device.h | 18 +++++++++---------\n> 2 files changed, 23 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f2e0bdbd..e05b5767 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -14,7 +14,6 @@\n> #include \n> \n> #include \n> -#include \n\n\nOk, so this removal because we are no longer need to use Mutex and \nMutexLocker from thread.h, makes sense\n\n> #include \n> \n> #include \n> @@ -399,7 +398,7 @@ void CameraDevice::close()\n> void CameraDevice::flush()\n> {\n> \t{\n> -\t\tMutexLocker stateLock(stateMutex_);\n> +\t\tMutexLocker2 stateLock(stateMutex_);\n> \t\tif (state_ != State::Running)\n> \t\t\treturn;\n> \n> @@ -409,20 +408,23 @@ void CameraDevice::flush()\n> \tworker_.stop();\n> \tcamera_->stop();\n> \n> -\tMutexLocker stateLock(stateMutex_);\n> +\tMutexLocker2 stateLock(stateMutex_);\n> \tstate_ = State::Stopped;\n> }\n> \n> void CameraDevice::stop()\n> {\n> -\tMutexLocker stateLock(stateMutex_);\n> +\tMutexLocker2 stateLock(stateMutex_);\n> \tif (state_ == State::Stopped)\n> \t\treturn;\n> \n> \tworker_.stop();\n> \tcamera_->stop();\n> \n> -\tdescriptors_ = {};\n> +\t{\n> +\t\tMutexLocker2 descriptorsLock(descriptorsMutex_);\n> +\t\tdescriptors_ = {};\n> +\t}\n\n\noh, we were resetting descriptors_ without taking the lock here.\n\nI am curious, did you notice this as WARNING from annotation and then \nintroduced this change? If yes, then annotation is already proving \nuseful to us.\n\n> \tstreams_.clear();\n> \n> \tstate_ = State::Stopped;\n> @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t */\n> \t\tFrameBuffer *frameBuffer = nullptr;\n> \t\tint acquireFence = -1;\n> +\t\tMutexLocker2 lock(descriptor->streamsProcessMutex_);\n\n\naha, one more change as result warning I suppose?\n\n> \t\tswitch (cameraStream->type()) {\n> \t\tcase CameraStream::Type::Mapped:\n> \t\t\t/*\n> @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * Request.\n> \t\t\t */\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> -\n?\n> \t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> \t\t\t\t{ cameraStream, &buffer });\n> \t\t\tcontinue;\n> @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t * on the queue to be later completed. If the camera has been stopped we\n> \t * have to re-start it to be able to process the request.\n> \t */\n> -\tMutexLocker stateLock(stateMutex_);\n> +\tMutexLocker2 stateLock(stateMutex_);\n> \n> \tif (state_ == State::Flushing) {\n> \t\tCamera3RequestDescriptor *rawDescriptor = descriptor.get();\n> \t\t{\n> -\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\t\t\tMutexLocker2 descriptorsLock(descriptorsMutex_);\n> \t\t\tdescriptors_.push(std::move(descriptor));\n> \t\t}\n> \t\tabortRequest(rawDescriptor);\n> @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tCaptureRequest *request = descriptor->request_.get();\n> \n> \t{\n> -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\t\tMutexLocker2 descriptorsLock(descriptorsMutex_);\n> \t\tdescriptors_.push(std::move(descriptor));\n> \t}\n> \n> @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \n> \t/* Handle post-processing. */\n> -\tMutexLocker locker(descriptor->streamsProcessMutex_);\n> +\tMutexLocker2 locker(descriptor->streamsProcessMutex_);\n> \n> \t/*\n> \t * Queue all the post-processing streams request at once. The completion\n> @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request)\n> \n> void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n> {\n> -\tMutexLocker lock(descriptorsMutex_);\n> +\tMutexLocker2 lock(descriptorsMutex_);\n> \tdescriptor->complete_ = true;\n> \n> \tsendCaptureResults();\n> @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n> \tCamera3RequestDescriptor *request = streamBuffer->request;\n> \n> \t{\n> -\t\tMutexLocker locker(request->streamsProcessMutex_);\n> +\t\tMutexLocker2 locker(request->streamsProcessMutex_);\n> \n> \t\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> \t\tif (!request->pendingStreamsToProcess_.empty())\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 2a414020..9feb287e 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -9,7 +9,6 @@\n> \n> #include \n> #include \n> -#include \n> #include \n> #include \n> \n> @@ -18,7 +17,8 @@\n> #include \n> #include \n> #include \n> -#include \n> +#include \n> +#include \n> \n> #include \n> #include \n> @@ -83,7 +83,7 @@ private:\n> \t\tRunning,\n> \t};\n> \n> -\tvoid stop();\n> +\tvoid stop() EXCLUDES(stateMutex_);\n> \n> \tstd::unique_ptr\n> \tcreateFrameBuffer(const buffer_handle_t camera3buffer,\n> @@ -95,8 +95,8 @@ private:\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> \t\t\t camera3_error_msg_code code) const;\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> -\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor);\n> -\tvoid sendCaptureResults();\n> +\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_);\n\n\nAm I right to infer that the EXCLUDES here means, descriptorsMutex_ is \nnot held (i.e. is not locked) beforehand, calling to completeDescriptor? \nSince the completeDescriptor will actually lock descriptorMutex_\n\n> +\tvoid sendCaptureResults() REQUIRES(descriptorsMutex_);\n\n\nAnd this requires descriptorsMutex_ to be locked, which makes sense.\n\n> \tvoid setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\n> \t\t\t Camera3RequestDescriptor::Status status);\n> \tstd::unique_ptr getResultMetadata(\n> @@ -107,8 +107,8 @@ private:\n> \n> \tCameraWorker worker_;\n> \n> -\tlibcamera::Mutex stateMutex_; /* Protects access to the camera state. */\n> -\tState state_;\n> +\tlibcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */\n> +\tState state_ GUARDED_BY(stateMutex_);\n> \n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> @@ -119,8 +119,8 @@ private:\n> \n> \tstd::vector streams_;\n> \n> -\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> -\tstd::queue> descriptors_;\n> +\tlibcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_);\n\n\nWell, the document currently states that ACQUIRED_AFTER is currently \nun-implemented.\n\nSecondly, I don't think we enforce a design interaction between the two \nmutexes currently, that requires this macro. For e.g. \ncompleteDescriptor() on a requestcomplete() path, will acquire \ndescriptorsMutex_ irrespective of acquiring stateMutex_. Is there any \nstrong reasoning I am missing which led to use of ACQUIRED_AFTER for \ndescriptorsMutex_?\n\n> +\tstd::queue> descriptors_ GUARDED_BY(descriptorsMutex_);\n\n\nThis is becoming a bit harder to read (not your fault, probably mine). I \nshould spend some time tinkering on naming these members/classes.\n\n> \n> \tstd::string maker_;\n> \tstd::string model_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D7124BF415\n\tfor ;\n\tThu, 11 Nov 2021 18:43:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8D776035F;\n\tThu, 11 Nov 2021 19:43:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 723E1600B5\n\tfor ;\n\tThu, 11 Nov 2021 19:43:03 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.254])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D937DEE;\n\tThu, 11 Nov 2021 19:43:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hdWaeIIa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636656183;\n\tbh=t1byONqQdQgKmjPnVdfCOhK2hgWIitzvtOipppHXaBc=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=hdWaeIIaG15QZCbW0LG0Kge51UN6fJAJP2g/e8VYGEnY73mfHNTfl/cAj3/QQ7eBp\n\tra4NbZtf/sczqNZr7U3GYMyR+0eyqwCZhrCZ7AntfpvRyunwargPfDsEpcQ9phn8xM\n\t6p9nFnmSF8lt+TqjO9/e2Xfo22TGyY+N9YtaNS40=","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-6-hiroh@chromium.org>","From":"Umang Jain ","Message-ID":"<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>","Date":"Fri, 12 Nov 2021 00:12:57 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211029041424.1430886-6-hiroh@chromium.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20886,"web_url":"https://patchwork.libcamera.org/comment/20886/","msgid":"","date":"2021-11-11T23:27:30","subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Nov 12, 2021 at 12:12:57AM +0530, Umang Jain wrote:\n> On 10/29/21 9:44 AM, Hirokazu Honda wrote:\n> > This applies clang thread safety annotation to CameraDevice.\n> > Mutex and MutexLocker there are replaced with Mutex2 and\n> > MutexLocer2.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 26 ++++++++++++++------------\n> > src/android/camera_device.h | 18 +++++++++---------\n> > 2 files changed, 23 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f2e0bdbd..e05b5767 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -14,7 +14,6 @@\n> > #include \n> > \n> > #include \n> > -#include \n> \n> Ok, so this removal because we are no longer need to use Mutex and \n> MutexLocker from thread.h, makes sense\n> \n> > #include \n> > \n> > #include \n> > @@ -399,7 +398,7 @@ void CameraDevice::close()\n> > void CameraDevice::flush()\n> > {\n> > \t{\n> > -\t\tMutexLocker stateLock(stateMutex_);\n> > +\t\tMutexLocker2 stateLock(stateMutex_);\n> > \t\tif (state_ != State::Running)\n> > \t\t\treturn;\n> > \n> > @@ -409,20 +408,23 @@ void CameraDevice::flush()\n> > \tworker_.stop();\n> > \tcamera_->stop();\n> > \n> > -\tMutexLocker stateLock(stateMutex_);\n> > +\tMutexLocker2 stateLock(stateMutex_);\n> > \tstate_ = State::Stopped;\n> > }\n> > \n> > void CameraDevice::stop()\n> > {\n> > -\tMutexLocker stateLock(stateMutex_);\n> > +\tMutexLocker2 stateLock(stateMutex_);\n> > \tif (state_ == State::Stopped)\n> > \t\treturn;\n> > \n> > \tworker_.stop();\n> > \tcamera_->stop();\n> > \n> > -\tdescriptors_ = {};\n> > +\t{\n> > +\t\tMutexLocker2 descriptorsLock(descriptorsMutex_);\n> > +\t\tdescriptors_ = {};\n> > +\t}\n> \n> oh, we were resetting descriptors_ without taking the lock here.\n> \n> I am curious, did you notice this as WARNING from annotation and then \n> introduced this change? If yes, then annotation is already proving \n> useful to us.\n\nSuch fixes should be split to a separate patch.\n\nGiven that the camera is stopped, accessing descriptors_ without taking\nthe lock should be safe here. The performance impact of the lock should\nbe negligible, so it's fine to be pedantic, but assuming there would be\na performance impact, would there be a way to avoid warnings without\ntaking the lock ?\n\n> > \tstreams_.clear();\n> > \n> > \tstate_ = State::Stopped;\n> > @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t */\n> > \t\tFrameBuffer *frameBuffer = nullptr;\n> > \t\tint acquireFence = -1;\n> > +\t\tMutexLocker2 lock(descriptor->streamsProcessMutex_);\n> \n> aha, one more change as result warning I suppose?\n\nIt seems to belong to patch 6/6 (or rather to a separate patch).\n\nI don't think the lock is needed (but it's probably harmless from a\nperformance point of view).\n\n> > \t\tswitch (cameraStream->type()) {\n> > \t\tcase CameraStream::Type::Mapped:\n> > \t\t\t/*\n> > @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\t * Request.\n> > \t\t\t */\n> > \t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> > -\n>\n> ?\n>\n> > \t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> > \t\t\t\t{ cameraStream, &buffer });\n> > \t\t\tcontinue;\n> > @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t * on the queue to be later completed. If the camera has been stopped we\n> > \t * have to re-start it to be able to process the request.\n> > \t */\n> > -\tMutexLocker stateLock(stateMutex_);\n> > +\tMutexLocker2 stateLock(stateMutex_);\n> > \n> > \tif (state_ == State::Flushing) {\n> > \t\tCamera3RequestDescriptor *rawDescriptor = descriptor.get();\n> > \t\t{\n> > -\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > +\t\t\tMutexLocker2 descriptorsLock(descriptorsMutex_);\n> > \t\t\tdescriptors_.push(std::move(descriptor));\n> > \t\t}\n> > \t\tabortRequest(rawDescriptor);\n> > @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \tCaptureRequest *request = descriptor->request_.get();\n> > \n> > \t{\n> > -\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > +\t\tMutexLocker2 descriptorsLock(descriptorsMutex_);\n> > \t\tdescriptors_.push(std::move(descriptor));\n> > \t}\n> > \n> > @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \t}\n> > \n> > \t/* Handle post-processing. */\n> > -\tMutexLocker locker(descriptor->streamsProcessMutex_);\n> > +\tMutexLocker2 locker(descriptor->streamsProcessMutex_);\n> > \n> > \t/*\n> > \t * Queue all the post-processing streams request at once. The completion\n> > @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \n> > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n> > {\n> > -\tMutexLocker lock(descriptorsMutex_);\n> > +\tMutexLocker2 lock(descriptorsMutex_);\n> > \tdescriptor->complete_ = true;\n> > \n> > \tsendCaptureResults();\n> > @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n> > \tCamera3RequestDescriptor *request = streamBuffer->request;\n> > \n> > \t{\n> > -\t\tMutexLocker locker(request->streamsProcessMutex_);\n> > +\t\tMutexLocker2 locker(request->streamsProcessMutex_);\n> > \n> > \t\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> > \t\tif (!request->pendingStreamsToProcess_.empty())\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 2a414020..9feb287e 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -9,7 +9,6 @@\n> > \n> > #include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > \n> > @@ -18,7 +17,8 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > +#include \n> > +#include \n> > \n> > #include \n> > #include \n> > @@ -83,7 +83,7 @@ private:\n> > \t\tRunning,\n> > \t};\n> > \n> > -\tvoid stop();\n> > +\tvoid stop() EXCLUDES(stateMutex_);\n> > \n> > \tstd::unique_ptr\n> > \tcreateFrameBuffer(const buffer_handle_t camera3buffer,\n> > @@ -95,8 +95,8 @@ private:\n> > \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > \t\t\t camera3_error_msg_code code) const;\n> > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > -\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor);\n> > -\tvoid sendCaptureResults();\n> > +\tvoid completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_);\n> \n> Am I right to infer that the EXCLUDES here means, descriptorsMutex_ is \n> not held (i.e. is not locked) beforehand, calling to completeDescriptor? \n> Since the completeDescriptor will actually lock descriptorMutex_\n> \n> > +\tvoid sendCaptureResults() REQUIRES(descriptorsMutex_);\n> \n> And this requires descriptorsMutex_ to be locked, which makes sense.\n> \n> > \tvoid setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\n> > \t\t\t Camera3RequestDescriptor::Status status);\n> > \tstd::unique_ptr getResultMetadata(\n> > @@ -107,8 +107,8 @@ private:\n> > \n> > \tCameraWorker worker_;\n> > \n> > -\tlibcamera::Mutex stateMutex_; /* Protects access to the camera state. */\n> > -\tState state_;\n> > +\tlibcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */\n> > +\tState state_ GUARDED_BY(stateMutex_);\n> > \n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;\n> > @@ -119,8 +119,8 @@ private:\n> > \n> > \tstd::vector streams_;\n> > \n> > -\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> > -\tstd::queue> descriptors_;\n> > +\tlibcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_);\n> \n> Well, the document currently states that ACQUIRED_AFTER is currently \n> un-implemented.\n> \n> Secondly, I don't think we enforce a design interaction between the two \n> mutexes currently, that requires this macro. For e.g. \n> completeDescriptor() on a requestcomplete() path, will acquire \n> descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any \n> strong reasoning I am missing which led to use of ACQUIRED_AFTER for \n> descriptorsMutex_?\n\nAcquiring locks in random orders lead to deadlocks, so it's good to\ndocument the order.\n\n> > +\tstd::queue> descriptors_ GUARDED_BY(descriptorsMutex_);\n> \n> This is becoming a bit harder to read (not your fault, probably mine). I \n> should spend some time tinkering on naming these members/classes.\n> \n> > \n> > \tstd::string maker_;\n> > \tstd::string model_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AE7D5BF415\n\tfor ;\n\tThu, 11 Nov 2021 23:27:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 199916035A;\n\tFri, 12 Nov 2021 00:27:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0ECB16033C\n\tfor ;\n\tFri, 12 Nov 2021 00:27:53 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7CA7674C;\n\tFri, 12 Nov 2021 00:27:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"U45aXJ1m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636673272;\n\tbh=bJWz8JrYk82vP6SDWfQp2VfkBPwSw9+WvBviSrD8EKA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U45aXJ1mYgb6sfXNFgHRXfZO3i1swKUg/WZiJ3st/1naMp0pk2YxJ196qGaAPyG6w\n\tPha7gcF/NEOApYzfs9gxa6ibn16x/TXxlsRWLsMuZk1SLvC0KHMagVSFbLi+VIso2u\n\tpRrKSl7fZtiB3ZVKOjjRDutr4pNufqcMVSe0uhl4=","Date":"Fri, 12 Nov 2021 01:27:30 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-6-hiroh@chromium.org>\n\t<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21310,"web_url":"https://patchwork.libcamera.org/comment/21310/","msgid":"","date":"2021-11-29T11:38:27","subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Fri, Nov 12, 2021 at 3:43 AM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch\n>\n> On 10/29/21 9:44 AM, Hirokazu Honda wrote:\n> > This applies clang thread safety annotation to CameraDevice.\n> > Mutex and MutexLocker there are replaced with Mutex2 and\n> > MutexLocer2.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 26 ++++++++++++++------------\n> > src/android/camera_device.h | 18 +++++++++---------\n> > 2 files changed, 23 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f2e0bdbd..e05b5767 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -14,7 +14,6 @@\n> > #include \n> >\n> > #include \n> > -#include \n>\n>\n> Ok, so this removal because we are no longer need to use Mutex and\n> MutexLocker from thread.h, makes sense\n>\n> > #include \n> >\n> > #include \n> > @@ -399,7 +398,7 @@ void CameraDevice::close()\n> > void CameraDevice::flush()\n> > {\n> > {\n> > - MutexLocker stateLock(stateMutex_);\n> > + MutexLocker2 stateLock(stateMutex_);\n> > if (state_ != State::Running)\n> > return;\n> >\n> > @@ -409,20 +408,23 @@ void CameraDevice::flush()\n> > worker_.stop();\n> > camera_->stop();\n> >\n> > - MutexLocker stateLock(stateMutex_);\n> > + MutexLocker2 stateLock(stateMutex_);\n> > state_ = State::Stopped;\n> > }\n> >\n> > void CameraDevice::stop()\n> > {\n> > - MutexLocker stateLock(stateMutex_);\n> > + MutexLocker2 stateLock(stateMutex_);\n> > if (state_ == State::Stopped)\n> > return;\n> >\n> > worker_.stop();\n> > camera_->stop();\n> >\n> > - descriptors_ = {};\n> > + {\n> > + MutexLocker2 descriptorsLock(descriptorsMutex_);\n> > + descriptors_ = {};\n> > + }\n>\n>\n> oh, we were resetting descriptors_ without taking the lock here.\n>\n> I am curious, did you notice this as WARNING from annotation and then\n> introduced this change? If yes, then annotation is already proving\n> useful to us.\n>\n\nYes, if we have mistakes like this, a compilation fails.\n\n> > streams_.clear();\n> >\n> > state_ = State::Stopped;\n> > @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > */\n> > FrameBuffer *frameBuffer = nullptr;\n> > int acquireFence = -1;\n> > + MutexLocker2 lock(descriptor->streamsProcessMutex_);\n>\n>\n> aha, one more change as result warning I suppose?\n>\n> > switch (cameraStream->type()) {\n> > case CameraStream::Type::Mapped:\n> > /*\n> > @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > * Request.\n> > */\n> > LOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> > -\n> ?\n> > descriptor->pendingStreamsToProcess_.insert(\n> > { cameraStream, &buffer });\n> > continue;\n> > @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > * on the queue to be later completed. If the camera has been stopped we\n> > * have to re-start it to be able to process the request.\n> > */\n> > - MutexLocker stateLock(stateMutex_);\n> > + MutexLocker2 stateLock(stateMutex_);\n> >\n> > if (state_ == State::Flushing) {\n> > Camera3RequestDescriptor *rawDescriptor = descriptor.get();\n> > {\n> > - MutexLocker descriptorsLock(descriptorsMutex_);\n> > + MutexLocker2 descriptorsLock(descriptorsMutex_);\n> > descriptors_.push(std::move(descriptor));\n> > }\n> > abortRequest(rawDescriptor);\n> > @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > CaptureRequest *request = descriptor->request_.get();\n> >\n> > {\n> > - MutexLocker descriptorsLock(descriptorsMutex_);\n> > + MutexLocker2 descriptorsLock(descriptorsMutex_);\n> > descriptors_.push(std::move(descriptor));\n> > }\n> >\n> > @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request)\n> > }\n> >\n> > /* Handle post-processing. */\n> > - MutexLocker locker(descriptor->streamsProcessMutex_);\n> > + MutexLocker2 locker(descriptor->streamsProcessMutex_);\n> >\n> > /*\n> > * Queue all the post-processing streams request at once. The completion\n> > @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request)\n> >\n> > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n> > {\n> > - MutexLocker lock(descriptorsMutex_);\n> > + MutexLocker2 lock(descriptorsMutex_);\n> > descriptor->complete_ = true;\n> >\n> > sendCaptureResults();\n> > @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff\n> > Camera3RequestDescriptor *request = streamBuffer->request;\n> >\n> > {\n> > - MutexLocker locker(request->streamsProcessMutex_);\n> > + MutexLocker2 locker(request->streamsProcessMutex_);\n> >\n> > request->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> > if (!request->pendingStreamsToProcess_.empty())\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 2a414020..9feb287e 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -9,7 +9,6 @@\n> >\n> > #include \n> > #include \n> > -#include \n> > #include \n> > #include \n> >\n> > @@ -18,7 +17,8 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > +#include \n> > +#include \n> >\n> > #include \n> > #include \n> > @@ -83,7 +83,7 @@ private:\n> > Running,\n> > };\n> >\n> > - void stop();\n> > + void stop() EXCLUDES(stateMutex_);\n> >\n> > std::unique_ptr\n> > createFrameBuffer(const buffer_handle_t camera3buffer,\n> > @@ -95,8 +95,8 @@ private:\n> > void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > camera3_error_msg_code code) const;\n> > int processControls(Camera3RequestDescriptor *descriptor);\n> > - void completeDescriptor(Camera3RequestDescriptor *descriptor);\n> > - void sendCaptureResults();\n> > + void completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_);\n>\n>\n> Am I right to infer that the EXCLUDES here means, descriptorsMutex_ is\n> not held (i.e. is not locked) beforehand, calling to completeDescriptor?\n> Since the completeDescriptor will actually lock descriptorMutex_\n>\n\nYes, this makes sure this function is not called while\ndescriptorsMutex_ is held, which causes deadlock.\n\n> > + void sendCaptureResults() REQUIRES(descriptorsMutex_);\n>\n>\n> And this requires descriptorsMutex_ to be locked, which makes sense.\n>\n> > void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\n> > Camera3RequestDescriptor::Status status);\n> > std::unique_ptr getResultMetadata(\n> > @@ -107,8 +107,8 @@ private:\n> >\n> > CameraWorker worker_;\n> >\n> > - libcamera::Mutex stateMutex_; /* Protects access to the camera state. */\n> > - State state_;\n> > + libcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */\n> > + State state_ GUARDED_BY(stateMutex_);\n> >\n> > std::shared_ptr camera_;\n> > std::unique_ptr config_;\n> > @@ -119,8 +119,8 @@ private:\n> >\n> > std::vector streams_;\n> >\n> > - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> > - std::queue> descriptors_;\n> > + libcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_);\n>\n>\n> Well, the document currently states that ACQUIRED_AFTER is currently\n> un-implemented.\n>\n\nOh I don't know that. What document do you refer?\nI couldnm't find it in\nhttps://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-acquired-after.\n\n> Secondly, I don't think we enforce a design interaction between the two\n> mutexes currently, that requires this macro. For e.g.\n> completeDescriptor() on a requestcomplete() path, will acquire\n> descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any\n> strong reasoning I am missing which led to use of ACQUIRED_AFTER for\n> descriptorsMutex_?\n>\n\nNo strong reason. I think it is nicer to clarify the order.\n\nFrom the code, I think this is the current order used in code.\n\nThanks,\n-Hiro\n> > + std::queue> descriptors_ GUARDED_BY(descriptorsMutex_);\n>\n>\n> This is becoming a bit harder to read (not your fault, probably mine). I\n> should spend some time tinkering on naming these members/classes.\n>\n> >\n> > std::string maker_;\n> > std::string model_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0BAC2BDB13\n\tfor ;\n\tMon, 29 Nov 2021 11:38:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E966605A1;\n\tMon, 29 Nov 2021 12:38:40 +0100 (CET)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B78B960592\n\tfor ;\n\tMon, 29 Nov 2021 12:38:38 +0100 (CET)","by mail-ed1-x529.google.com with SMTP id v1so70456394edx.2\n\tfor ;\n\tMon, 29 Nov 2021 03:38:38 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"EQqTtCt4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=HzCgnXVcxvqxd2HySQNTfmg+IGtQZa+MwmKHnG1VHi8=;\n\tb=EQqTtCt4wKYBEIhDNveZbEMpXYsyw4dfConFPuI72KwhqvO/4ZOIXir6j0qhVgl7dv\n\t3Rr7iy8D7HC0qtdBjJTmgHOWdAWWLYBmm//lryPVFX9hFBVyDbLjwbYwAdYZ/zDAfj2l\n\tocF7o8XmLNYf8T5jtyMgxd7HNdEp0mcibyjkY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=HzCgnXVcxvqxd2HySQNTfmg+IGtQZa+MwmKHnG1VHi8=;\n\tb=JtI9ArKMosY6PBeTp4YkOMfGxjV7jTZbA8UW47eWCq/EVxvuH6wo7qErs6uFVIK6v9\n\tGluvThH5WVc5IybRipW5qkGDgfjTy/+fgOCgkZDRBxONBJcI+D8o+JbtktgIAdH49QsN\n\tU3CtCW4a2EMWNIXm+671YJsCAffWhudy56ORjaHmbZptfIoEGzI4m1pIELS6szBOLUH2\n\t34J8z2lNgBYQPnLiNR/ZOmxmS1YMPiX++Mq3ml6334XGZD9ezUYn/Cabf/lcSRZpFCOJ\n\tvQVFUOd/QuYuBDQjK0DlfYj0uKQu8/7mM++U4JtO4K1gRAyIJ1JUO5hkMarihwkWWJFn\n\tZ7dw==","X-Gm-Message-State":"AOAM531JGybDEqG/6rVFyvIYQ/ALvZAK2SplAhAYeB+uI9zHNLSJoTEj\n\tIyyX+cYy1PYC/ZFcQ3688Vykw+7lg9lnViBV5i6nTNilhv6N1Q==","X-Google-Smtp-Source":"ABdhPJzBKjwQd7N3k7AT18oRIW3APkzPHkTV4MEizoinUJ3Uh2DRIRgw72NgD8PWsgqivrWWrf7cwL8W/GexS62Iql8=","X-Received":"by 2002:a05:6402:2552:: with SMTP id\n\tl18mr73828824edb.368.1638185918125; \n\tMon, 29 Nov 2021 03:38:38 -0800 (PST)","MIME-Version":"1.0","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-6-hiroh@chromium.org>\n\t<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>","In-Reply-To":"<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 20:38:27 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21412,"web_url":"https://patchwork.libcamera.org/comment/21412/","msgid":"<1f1ace6f-7ad2-f7e1-43cd-7d09729d2223@ideasonboard.com>","date":"2021-11-30T06:28:15","subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 11/29/21 5:08 PM, Hirokazu Honda wrote:\n\n\n>\n>>\n>> Well, the document currently states that ACQUIRED_AFTER is currently\n>> un-implemented.\n>>\n> Oh I don't know that. What document do you refer?\n> I couldnm't find it in\n> https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-acquired-after.\n\n\nhttps://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-and-acquired-after-are-currently-unimplemented\n\n>\n>> Secondly, I don't think we enforce a design interaction between the two\n>> mutexes currently, that requires this macro. For e.g.\n>> completeDescriptor() on a requestcomplete() path, will acquire\n>> descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any\n>> strong reasoning I am missing which led to use of ACQUIRED_AFTER for\n>> descriptorsMutex_?\n>>\n> No strong reason. I think it is nicer to clarify the order.\n>\n> From the code, I think this is the current order used in code.\n\n\nMakes sense.\n\n>\n> Thanks,\n> -Hiro\n>>> + std::queue> descriptors_ GUARDED_BY(descriptorsMutex_);\n>>\n>> This is becoming a bit harder to read (not your fault, probably mine). I\n>> should spend some time tinkering on naming these members/classes.\n>>\n>>> std::string maker_;\n>>> std::string model_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7DABBBDB13\n\tfor ;\n\tTue, 30 Nov 2021 06:28:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5802605B4;\n\tTue, 30 Nov 2021 07:28:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9747E604FC\n\tfor ;\n\tTue, 30 Nov 2021 07:28:22 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6DD8D8F0;\n\tTue, 30 Nov 2021 07:28:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wR4rGuRN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638253702;\n\tbh=W7v6PK+ZvE0ovT2MV5vTIIJHEWQaWR0B8XJlK0wHBRE=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=wR4rGuRNmLUJlrT2KFrGA7OmI8L+QQGEOqvMSIP4z8t0qeEJi/L3MQwQOirHN8/15\n\tc2DaMgnHVzIdWmcb7m8Vmw8NQG2M3dDk6FsFw4MJkPm05W2HXhP9kI3f6XkayUFUxU\n\t5yvXBoO26/bLmEWD/2bCAb7YsNOTc/MyuqFigcas=","To":"Hirokazu Honda ","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-6-hiroh@chromium.org>\n\t<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<1f1ace6f-7ad2-f7e1-43cd-7d09729d2223@ideasonboard.com>","Date":"Tue, 30 Nov 2021 11:58:15 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21449,"web_url":"https://patchwork.libcamera.org/comment/21449/","msgid":"","date":"2021-11-30T12:13:00","subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Tue, Nov 30, 2021 at 3:28 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 11/29/21 5:08 PM, Hirokazu Honda wrote:\n>\n> \n> >\n> >>\n> >> Well, the document currently states that ACQUIRED_AFTER is currently\n> >> un-implemented.\n> >>\n> > Oh I don't know that. What document do you refer?\n> > I couldnm't find it in\n> > https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-acquired-after.\n>\n>\n> https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquired-before-and-acquired-after-are-currently-unimplemented\n>\n\nThanks! I definitely missed it.\n-Hiro\n> >\n> >> Secondly, I don't think we enforce a design interaction between the two\n> >> mutexes currently, that requires this macro. For e.g.\n> >> completeDescriptor() on a requestcomplete() path, will acquire\n> >> descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any\n> >> strong reasoning I am missing which led to use of ACQUIRED_AFTER for\n> >> descriptorsMutex_?\n> >>\n> > No strong reason. I think it is nicer to clarify the order.\n> >\n> > From the code, I think this is the current order used in code.\n>\n>\n> Makes sense.\n>\n> >\n> > Thanks,\n> > -Hiro\n> >>> + std::queue> descriptors_ GUARDED_BY(descriptorsMutex_);\n> >>\n> >> This is becoming a bit harder to read (not your fault, probably mine). I\n> >> should spend some time tinkering on naming these members/classes.\n> >>\n> >>> std::string maker_;\n> >>> std::string model_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E44BDBF415\n\tfor ;\n\tTue, 30 Nov 2021 12:13:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A038605C1;\n\tTue, 30 Nov 2021 13:13:14 +0100 (CET)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC8EE60587\n\tfor ;\n\tTue, 30 Nov 2021 13:13:12 +0100 (CET)","by mail-ed1-x52f.google.com with SMTP id y12so85808832eda.12\n\tfor ;\n\tTue, 30 Nov 2021 04:13:12 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"OTFuRj5F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=BkVaW8WVKo3dQEqBGBiHRP6GYIUj6vXQZVcYS1+iJd0=;\n\tb=OTFuRj5Fg4J+UfG8BMM8kaZrFPo2rO/JE8vtACxvLrVp6jwPnaTIVjMlgYhmq8/C48\n\tqd2IMjz13XsYbDOpEbXFV166RRAKgjcxrVZ441s6Y0s3pFmOHI7w0EN2QEZWHRH7k5rg\n\te8T1+3+6AjRFWQ7sLPtxzyI6iMr/CkCk3q9Ak=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=BkVaW8WVKo3dQEqBGBiHRP6GYIUj6vXQZVcYS1+iJd0=;\n\tb=o2SvKkl9FntbjIRjqDV9EVs4Pfki4b+ucBvH3B1iLYbodztmvHOH8Vabsg0cGNW9yV\n\t27uA52j5+g6Wfugvwnna4mkCJ2yCmkAtdEF3K4Sfv1Hk9y8lvLtMwHKr9yjv+t78fC7E\n\t7uvHX8OyiCpcLGaa/nrP4/lRf1iMHcwbNE5DCUVUhPf72Zu6GeHAmRVaTS6sm6PKzEOA\n\t8v7/etdGh8Hw2n5kBQKykcEfgRxb5ZdBvO7Sm5DiVqFl1rT/NRFL14ZB0d5q5uLNAMSA\n\tcfycuXlnEYeVTffMDBBla3wRErwBwy3Xa7y5F7xvGjbUgT5X7D4QmxVF93+ACvDgMmAR\n\tIrVA==","X-Gm-Message-State":"AOAM531arYcRvD6ai0dSk+u3xThRPa6YM98ZBG3MJTvad8MAjUZn+oL1\n\t1CAgDJV2I8mIjHfSO3BDLKitUPSzVFHoHonOIjcgioOdNek=","X-Google-Smtp-Source":"ABdhPJyMnhZT654YylYK4rvDXUIKUTPXX0BGUO0h/kqV9jIvMnhb3C7P5PTzFTZlNqR/lahpZvPnIG9HhgOjJh9jEL0=","X-Received":"by 2002:a17:907:1c82:: with SMTP id\n\tnb2mr65712220ejc.218.1638274392277; \n\tTue, 30 Nov 2021 04:13:12 -0800 (PST)","MIME-Version":"1.0","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-6-hiroh@chromium.org>\n\t<71a6a634-68d7-6026-dc5c-5b5b895171b1@ideasonboard.com>\n\t\n\t<1f1ace6f-7ad2-f7e1-43cd-7d09729d2223@ideasonboard.com>","In-Reply-To":"<1f1ace6f-7ad2-f7e1-43cd-7d09729d2223@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 30 Nov 2021 21:13:00 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add\n\tthread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]