From patchwork Mon Oct 11 07:35:04 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 14081 X-Patchwork-Delegate: umang.jain@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 2A31BC323E for ; Mon, 11 Oct 2021 07:35:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id E0F0B68F58; Mon, 11 Oct 2021 09:35:28 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="sDOQFpqn"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 61BF868F51 for ; Mon, 11 Oct 2021 09:35:26 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.251.226.107]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 748872BD; Mon, 11 Oct 2021 09:35:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1633937726; bh=lZjbcbQa+fNMcgjQZHeZxVksBy9hBGcRD5K59GP0k70=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=sDOQFpqnO6q3xTR4PaCgVV0rSu2Ss6DNAp+2aqCZH9DzelkktUe/KALCldyrogT22 s+jH6xQVIxaPs1jcwe+j58dneyLks6t0ZDE5h1SgGj1RjXuvRgmi/tLsq0SiRa1aJX CKWMKx7Si2b9M4Q1prxYjIWPQbxari7yvr0BYXAc= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Mon, 11 Oct 2021 13:05:04 +0530 Message-Id: <20211011073505.243864-7-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20211011073505.243864-1-umang.jain@ideasonboard.com> References: <20211011073505.243864-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 6/7] android: camera_device: Protect descriptor status_ with lock X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Laurent Pinchart The Camera3RequestDescriptor::status_ is checked as a stop condition for the sendCaptureResults() loop, protected by the descriptorsMutex_. The status is however not set with the mutex locked, which can cause a race condition with a concurrent sendCaptureResults() call (from the post-processor thread for instance). This should be harmless in practice, as the reader thread will either see the old status (Pending) and stop iterating over descriptors, or the new status and continue. Still, if the Camera3RequestDescriptor state machine were to change in the future, this could introduce hard to debug issues. Close the race window by always setting the status with the lock taken. Signed-off-by: Laurent Pinchart --- src/android/camera_device.cpp | 27 +++++++++++++++++++-------- src/android/camera_device.h | 4 +++- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 61b902ad..3541a74b 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -877,8 +877,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const buffer.status = CAMERA3_BUFFER_STATUS_ERROR; } result.output_buffers = resultBuffers.data(); - - descriptor->status_ = Camera3RequestDescriptor::Status::Error; } bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const @@ -1058,8 +1056,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques if (state_ == State::Flushing) { abortRequest(descriptor.get()); + { MutexLocker descriptorsLock(descriptorsMutex_); + descriptor->status_ = Camera3RequestDescriptor::Status::Error; descriptors_.push(std::move(descriptor)); } @@ -1154,8 +1154,7 @@ void CameraDevice::requestComplete(Request *request) buffer.status = CAMERA3_BUFFER_STATUS_ERROR; } - descriptor->status_ = Camera3RequestDescriptor::Status::Error; - sendCaptureResults(); + completeDescriptor(descriptor, Camera3RequestDescriptor::Status::Error); return; } @@ -1215,13 +1214,24 @@ void CameraDevice::requestComplete(Request *request) } captureResult.result = descriptor->resultMetadata_->get(); - descriptor->status_ = Camera3RequestDescriptor::Status::Success; + + completeDescriptor(descriptor, Camera3RequestDescriptor::Status::Success); +} + +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor, + Camera3RequestDescriptor::Status status) +{ + MutexLocker lock(descriptorsMutex_); + descriptor->status_ = status; + lock.unlock(); + sendCaptureResults(); } void CameraDevice::sendCaptureResults() { MutexLocker lock(descriptorsMutex_); + while (!descriptors_.empty() && !descriptors_.front()->isPending()) { auto descriptor = std::move(descriptors_.front()); descriptors_.pop(); @@ -1262,12 +1272,13 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, if (cameraStream->type() == CameraStream::Type::Internal) cameraStream->putBuffer(request->internalBuffer_); + Camera3RequestDescriptor::Status descriptorStatus; if (status == PostProcessor::Status::Success) - request->status_ = Camera3RequestDescriptor::Status::Success; + descriptorStatus = Camera3RequestDescriptor::Status::Success; else - request->status_ = Camera3RequestDescriptor::Status::Error; + descriptorStatus = Camera3RequestDescriptor::Status::Error; - sendCaptureResults(); + completeDescriptor(request, descriptorStatus); } std::string CameraDevice::logPrefix() const diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 725a0618..0ce6caeb 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -126,6 +126,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, + Camera3RequestDescriptor::Status status); void sendCaptureResults(); std::unique_ptr getResultMetadata( const Camera3RequestDescriptor &descriptor) const; @@ -147,7 +149,7 @@ private: std::vector streams_; - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ + libcamera::Mutex descriptorsMutex_; /* Protects descriptors_ and Camera3RequestDescriptor::status_. */ std::queue> descriptors_; std::string maker_;