Patch Detail
Show a patch.
GET /api/patches/12111/?format=api
{ "id": 12111, "url": "https://patchwork.libcamera.org/api/patches/12111/?format=api", "web_url": "https://patchwork.libcamera.org/patch/12111/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/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": "<20210426083830.2965095-3-hiroh@chromium.org>", "date": "2021-04-26T08:38:29", "name": "[libcamera-devel,v3,2/3] android: CameraDevice: Prevent race due to concurrent HAL calls", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "926eb4a2a41b573e7ad9ae43fe90e874f7d1c14c", "submitter": { "id": 63, "url": "https://patchwork.libcamera.org/api/people/63/?format=api", "name": "Hirokazu Honda", "email": "hiroh@chromium.org" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/12111/mbox/", "series": [ { "id": 1976, "url": "https://patchwork.libcamera.org/api/series/1976/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1976", "date": "2021-04-26T08:38:27", "name": "Support HAL3 API flush", "version": 3, "mbox": "https://patchwork.libcamera.org/series/1976/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/12111/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/12111/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 8B09CBDC99\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 26 Apr 2021 08:38:43 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 12C8F688AF;\n\tMon, 26 Apr 2021 10:38:43 +0200 (CEST)", "from mail-pg1-x531.google.com (mail-pg1-x531.google.com\n\t[IPv6:2607:f8b0:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 926AE688AF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 26 Apr 2021 10:38:41 +0200 (CEST)", "by mail-pg1-x531.google.com with SMTP id s22so18495899pgk.6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 26 Apr 2021 01:38:41 -0700 (PDT)", "from hiroh2.tok.corp.google.com\n\t([2401:fa00:8f:2:a407:b8c2:d8b:a449])\n\tby smtp.gmail.com with ESMTPSA id\n\te18sm4873728pgr.8.2021.04.26.01.38.37\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 26 Apr 2021 01:38:38 -0700 (PDT)" ], "Authentication-Results": "lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"EJQz4C8/\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=from:to:cc:subject:date:message-id:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=xpJ1iUDfDDXHcnvCiqwegzXaebN0Vojm3qpeP8L5ZPg=;\n\tb=EJQz4C8/VgpVE4wvb+udZIbNTeIOIA69lWHdsusBGqcFOzKt/1neW3MP4ULJJdOOkU\n\tCG7s2vYLG/nB8GKyJy8CYpogxZQoo8Rq5GjeyviAJlDyANBBu7KMGuQDaLcwBWaNjS7Y\n\tm7VCotglQKt+unKDANl70ENQHob818gXfNU/0=", "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=xpJ1iUDfDDXHcnvCiqwegzXaebN0Vojm3qpeP8L5ZPg=;\n\tb=DMuZseBzhtN6FWy7CaLEt2uXpQ9XtNyH5kXWwc+FL8xqD72xo/WPHPuLPXOvSyiWSc\n\tgjCgIoCjAgGDjuhByEWMJI6ZZyF5nPqN8SvcmstZsVxe4XhwnwNs0Rk081WvsO1A2TK5\n\tnJ5eDM876gYmMJZcSmT1PSTvgHTHUZ0EtfATGj+Ad2NeZdQcCJM1z2zeKoRc9JDHCWGc\n\tm8DlqdxdUVhwnjs3iIxzvycYnDPqwHx3sySCEfQH0VwfxUM9ju/jL9gCazQ4hgu0kiys\n\t81lQI8ilKC+L/xcgMhpwus73AqFMgvJ4K613g7dsyA/jiGDLlO64DfircTlhVj65K2vD\n\tmgug==", "X-Gm-Message-State": "AOAM531eg2ZG63VC0n4+LBI0+Bdyl3lGHh86irpr/LDgko5+LGEyn+at\n\t55PB1p7XXAo/RS4TCChjydkQzMzzArFLBA==", "X-Google-Smtp-Source": "ABdhPJzQcTvXzQ2r0aau+zOgI/YMS56qZEW2JZZHzMsjYTnQUGk8ZpJoK/CqcGjMz3xk0TJ+ShtYjw==", "X-Received": "by 2002:a63:8948:: with SMTP id\n\tv69mr15891684pgd.264.1619426318878; \n\tMon, 26 Apr 2021 01:38:38 -0700 (PDT)", "From": "Hirokazu Honda <hiroh@chromium.org>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Mon, 26 Apr 2021 17:38:29 +0900", "Message-Id": "<20210426083830.2965095-3-hiroh@chromium.org>", "X-Mailer": "git-send-email 2.31.1.498.g6c1eba8ee3d-goog", "In-Reply-To": "<20210426083830.2965095-1-hiroh@chromium.org>", "References": "<20210426083830.2965095-1-hiroh@chromium.org>", "MIME-Version": "1.0", "Subject": "[libcamera-devel] [PATCH v3 2/3] android: CameraDevice: Prevent\n\trace due to concurrent HAL calls", "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>", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "HAL API functions might be called by different threads in Android\nCamera HAL3 API, while process_capture_request() must be called\nby a single thread. Furthermore, close() and flush() can be\ncalled any time. Therefore, races to variables can occur among\nthe HAL API functions.\nThis prevents the races by enclosing HAL calls by mutex. It\nmight not be the best in terms of the performance, but the\nsimplicity is good as a first step and also further development.\n\nSigned-off-by: Hirokazu Honda <hiroh@chromium.org>\n---\n src/android/camera_device.cpp | 59 ++++++++++++++++++++---------------\n src/android/camera_device.h | 7 +++--\n 2 files changed, 39 insertions(+), 27 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex b3def04b..96c8d4a9 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -743,7 +743,7 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n \n void CameraDevice::close()\n {\n-\tstreams_.clear();\n+\tstd::scoped_lock<std::mutex> halLock(halMutex_);\n \n \tstop();\n \n@@ -758,12 +758,17 @@ void CameraDevice::stop()\n \tworker_.stop();\n \tcamera_->stop();\n \n-\tdescriptors_.clear();\n+\t{\n+\t\tstd::scoped_lock<std::mutex> lock(mutex_);\n+\t\tdescriptors_.clear();\n+\t}\n+\n \trunning_ = false;\n }\n \n void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)\n {\n+\tstd::scoped_lock<std::mutex> halLock(halMutex_);\n \tcallbacks_ = callbacks;\n }\n \n@@ -1643,8 +1648,7 @@ PixelFormat CameraDevice::toPixelFormat(int format) const\n */\n int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n {\n-\t/* Before any configuration attempt, stop the camera. */\n-\tstop();\n+\tstd::scoped_lock<std::mutex> halLock(halMutex_);\n \n \tif (stream_list->num_streams == 0) {\n \t\tLOG(HAL, Error) << \"No streams in configuration\";\n@@ -1656,6 +1660,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \t\treturn -EINVAL;\n #endif\n \n+\t/* Before any configuration attempt, stop the camera. */\n+\tstop();\n+\n \t/*\n \t * Generate an empty configuration, and construct a StreamConfiguration\n \t * for each camera3_stream to add to it.\n@@ -1671,6 +1678,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \t * ensure the required entries are available without further\n \t * reallocation.\n \t */\n+\n+\tstd::scoped_lock<std::mutex> lock(mutex_);\n \tstreams_.clear();\n \tstreams_.reserve(stream_list->num_streams);\n \n@@ -1907,6 +1916,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \tif (!isValidRequest(camera3Request))\n \t\treturn -EINVAL;\n \n+\tstd::scoped_lock<std::mutex> halLock(halMutex_);\n+\n \t/* Start the camera if that's the first request we handle. */\n \tif (!running_) {\n \t\tworker_.start();\n@@ -2022,29 +2033,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \n void CameraDevice::requestComplete(Request *request)\n {\n+\tstd::scoped_lock<std::mutex> lock(mutex_);\n+\n \tconst Request::BufferMap &buffers = request->buffers();\n \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n \tstd::unique_ptr<CameraMetadata> resultMetadata;\n \n-\tdecltype(descriptors_)::node_type node;\n-\tstd::unique_ptr<CaptureRequest> captureRequest;\n-\t{\n-\t\tstd::scoped_lock<std::mutex> lock(mutex_);\n-\t\tauto requestIt = requests_.find(request->cookie());\n-\t\tif (requestIt == requests_.end()) {\n-\t\t\tLOG(HAL, Fatal)\n-\t\t\t\t<< \"Unknown request: \" << request->cookie();\n-\t\t\treturn;\n-\t\t}\n-\t\tcaptureRequest = std::move(requestIt->second);\n-\t\trequests_.erase(requestIt);\n+\tauto requestIt = requests_.find(request->cookie());\n+\tif (requestIt == requests_.end()) {\n+\t\tLOG(HAL, Fatal)\n+\t\t\t<< \"Unknown request: \" << request->cookie();\n+\t\treturn;\n+\t}\n+\tauto captureRequest = std::move(requestIt->second);\n+\trequests_.erase(requestIt);\n \n-\t\tauto it = descriptors_.find(request->cookie());\n-\t\tif (it == descriptors_.end())\n-\t\t\treturn;\n+\tauto it = descriptors_.find(request->cookie());\n+\tif (it == descriptors_.end())\n+\t\treturn;\n \n-\t\tnode = descriptors_.extract(it);\n-\t}\n+\tauto node = descriptors_.extract(it);\n \tCamera3RequestDescriptor &descriptor = node.mapped();\n \n \tif (request->status() != Request::RequestComplete) {\n@@ -2149,11 +2157,12 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n \tcamera3_notify_msg_t notify = {};\n \n \t/*\n-\t * \\todo Report and identify the stream number or configuration to\n-\t * clarify the stream that failed.\n+\t * \\todo Report and identify the stream number or configuration\n+\t * to clarify the stream that failed.\n \t */\n-\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n-\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n+\tLOG(HAL, Error)\n+\t\t<< \"Error occurred on frame \" << descriptor.frameNumber_ << \" (\"\n+\t\t<< toPixelFormat(descriptor.buffers_[0].stream->format).toString() << \")\";\n \n \tnotify.type = CAMERA3_MSG_ERROR;\n \tnotify.message.error.error_stream = stream;\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 95d77890..325fb967 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -125,9 +125,12 @@ private:\n \n \tstd::vector<Camera3StreamConfiguration> streamConfigurations_;\n \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n-\tstd::vector<CameraStream> streams_;\n \n-\tstd::mutex mutex_; /* Protect descriptors_ and requests_. */\n+\t/* Avoid races by concurrent Camera HAL API calls. */\n+\tstd::mutex halMutex_;\n+\n+\tstd::mutex mutex_; /* Protect streams_, descriptors_ and requests_. */\n+\tstd::vector<CameraStream> streams_;\n \tstd::map<uint64_t, Camera3RequestDescriptor> descriptors_;\n \tstd::map<uint64_t, std::unique_ptr<CaptureRequest>> requests_;\n \n", "prefixes": [ "libcamera-devel", "v3", "2/3" ] }