Patch Detail
Show a patch.
GET /api/1.1/patches/14192/?format=api
{ "id": 14192, "url": "https://patchwork.libcamera.org/api/1.1/patches/14192/?format=api", "web_url": "https://patchwork.libcamera.org/patch/14192/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20211020104212.121743-5-umang.jain@ideasonboard.com>", "date": "2021-10-20T10:42:12", "name": "[libcamera-devel,v5,4/4] android: camera_device: Protect descriptor status_ with lock", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "0b178d415981c9198c35db04dfff0591eb704cef", "submitter": { "id": 86, "url": "https://patchwork.libcamera.org/api/1.1/people/86/?format=api", "name": "Umang Jain", "email": "umang.jain@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/14192/mbox/", "series": [ { "id": 2640, "url": "https://patchwork.libcamera.org/api/1.1/series/2640/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2640", "date": "2021-10-20T10:42:08", "name": "Async Post Processor", "version": 5, "mbox": "https://patchwork.libcamera.org/series/2640/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/14192/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/14192/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 72700BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 20 Oct 2021 10:42:29 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CDD568F5B;\n\tWed, 20 Oct 2021 12:42:29 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 400E968F61\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 20 Oct 2021 12:42:27 +0200 (CEST)", "from perceval.ideasonboard.com (unknown [103.251.226.185])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0ACEB2A5;\n\tWed, 20 Oct 2021 12:42:25 +0200 (CEST)" ], "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=\"fKAxxH5u\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634726547;\n\tbh=8wevhsEeDoT7GH/zuB8hQFildf6Fb6V/BSAK+h27oGE=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=fKAxxH5ugzGtiSUHzP1iFLnMAZp7f9E4fPD1jINyGTA/xJuMFqexCvq+GTKuZIDD1\n\t3Skri/rRBOQaRNq13TXH5eN00/cI8qVCx9Kx3PoXlXDclWRIQwS3pK7d5fLqtnahey\n\tbwxbzdyRyE0EV4uszTVUvnGLcchAc4pLa0moauQc=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Wed, 20 Oct 2021 16:12:12 +0530", "Message-Id": "<20211020104212.121743-5-umang.jain@ideasonboard.com>", "X-Mailer": "git-send-email 2.31.1", "In-Reply-To": "<20211020104212.121743-1-umang.jain@ideasonboard.com>", "References": "<20211020104212.121743-1-umang.jain@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v5 4/4] android: camera_device: Protect\n\tdescriptor status_ with lock", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nThe Camera3RequestDescriptor::status_ is checked as a stop condition for\nthe sendCaptureResults() loop, protected by the descriptorsMutex_. The\nstatus is however not set with the mutex locked, which can cause a race\ncondition with a concurrent sendCaptureResults() call (from the\npost-processor thread for instance).\n\nThis should be harmless in practice, as the reader thread will either\nsee the old status (Pending) and stop iterating over descriptors, or the\nnew status and continue. Still, if the Camera3RequestDescriptor state\nmachine were to change in the future, this could introduce hard to debug\nissues. Close the race window by always setting the status with the lock\ntaken.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\nSigned-off-by: Umang Jain <umang.jain@ideasonboard.com>\n---\n src/android/camera_device.cpp | 29 +++++++++++++++++++----------\n src/android/camera_device.h | 5 ++++-\n 2 files changed, 23 insertions(+), 11 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex b14416ce..4e8fb2ee 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -803,8 +803,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n \n \tfor (auto &buffer : descriptor->buffers_)\n \t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n-\n-\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n }\n \n bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n@@ -988,7 +986,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\tdescriptors_.push(std::move(descriptor));\n \t\t}\n \n-\t\tsendCaptureResults();\n+\t\tcompleteDescriptor(descriptor.get(),\n+\t\t\t\t Camera3RequestDescriptor::Status::Error);\n \n \t\treturn 0;\n \t}\n@@ -1058,7 +1057,8 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\t\t<< request->status();\n \n \t\tabortRequest(descriptor);\n-\t\tsendCaptureResults();\n+\t\tcompleteDescriptor(descriptor,\n+\t\t\t\t Camera3RequestDescriptor::Status::Error);\n \n \t\treturn;\n \t}\n@@ -1135,20 +1135,28 @@ void CameraDevice::requestComplete(Request *request)\n \n \tif (needsPostProcessing) {\n \t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n-\t\t\tdescriptor->status_ = processingStatus;\n \t\t\t/*\n \t\t\t * \\todo This is problematic when some streams are\n \t\t\t * queued successfully, but some fail to get queued.\n \t\t\t * We might end up with use-after-free situation for\n \t\t\t * descriptor in streamProcessingComplete().\n \t\t\t */\n-\t\t\tsendCaptureResults();\n+\t\t\tcompleteDescriptor(descriptor, processingStatus);\n \t\t}\n \n \t\treturn;\n \t}\n \n-\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n+\tcompleteDescriptor(descriptor, Camera3RequestDescriptor::Status::Success);\n+}\n+\n+void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,\n+\t\t\t\t Camera3RequestDescriptor::Status status)\n+{\n+\tMutexLocker lock(descriptorsMutex_);\n+\tdescriptor->status_ = status;\n+\tlock.unlock();\n+\n \tsendCaptureResults();\n }\n \n@@ -1254,14 +1262,15 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n \t\t\thasPostProcessingErrors = true;\n \t}\n \n+\tCamera3RequestDescriptor::Status descriptorStatus;\n \tif (hasPostProcessingErrors)\n-\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n+\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Error;\n \telse\n-\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n+\t\tdescriptorStatus = Camera3RequestDescriptor::Status::Success;\n \n \tlocker.unlock();\n \n-\tsendCaptureResults();\n+\tcompleteDescriptor(request, descriptorStatus);\n }\n \n std::string CameraDevice::logPrefix() const\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 1ef933da..46fb93ee 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -96,6 +96,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+\t\t\t\tCamera3RequestDescriptor::Status status);\n \tvoid sendCaptureResults();\n \tvoid setBufferStatus(CameraStream *cameraStream,\n \t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n@@ -121,7 +123,8 @@ private:\n \n \tstd::vector<CameraStream> streams_;\n \n-\tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n+\t/* Protects descriptors_ and Camera3RequestDescriptor::status_. */\n+\tlibcamera::Mutex descriptorsMutex_;\n \tstd::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;\n \n \tstd::string maker_;\n", "prefixes": [ "libcamera-devel", "v5", "4/4" ] }