Patch Detail
Show a patch.
GET /api/1.1/patches/12091/?format=api
{ "id": 12091, "url": "https://patchwork.libcamera.org/api/1.1/patches/12091/?format=api", "web_url": "https://patchwork.libcamera.org/patch/12091/", "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": "<20210423040738.1227220-2-hiroh@chromium.org>", "date": "2021-04-23T04:07:37", "name": "[libcamera-devel,v2,1/2] android: CameraDevice: Prevent race due to concurrent HAL calls", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "87f8fe199ff870e217adbe4240ae44541ad9dd68", "submitter": { "id": 63, "url": "https://patchwork.libcamera.org/api/1.1/people/63/?format=api", "name": "Hirokazu Honda", "email": "hiroh@chromium.org" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/12091/mbox/", "series": [ { "id": 1968, "url": "https://patchwork.libcamera.org/api/1.1/series/1968/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1968", "date": "2021-04-23T04:07:36", "name": "Support HAL3 API flush", "version": 2, "mbox": "https://patchwork.libcamera.org/series/1968/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/12091/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/12091/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 3DBB0BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Apr 2021 04:07:49 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECD7B68878;\n\tFri, 23 Apr 2021 06:07:48 +0200 (CEST)", "from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com\n\t[IPv6:2607:f8b0:4864:20::1036])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD38468875\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Apr 2021 06:07:46 +0200 (CEST)", "by mail-pj1-x1036.google.com with SMTP id nk8so9272891pjb.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Apr 2021 21:07:46 -0700 (PDT)", "from hiroh2.tok.corp.google.com\n\t([2401:fa00:8f:2:8537:2d4f:8d35:5777])\n\tby smtp.gmail.com with ESMTPSA id\n\tj23sm3229670pfh.179.2021.04.22.21.07.43\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 22 Apr 2021 21:07:44 -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=\"adUF+8KN\"; 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=MsN40bmC3F41Rpw+7esE5zEV1IyiFFeClODJYRACcm4=;\n\tb=adUF+8KNWM5oWQCKSQydN+hszR+gJjPMWBGhOgZeqlS545Hte5j/hovj6S0ohWHWL9\n\txAO+uhyun/SmcnEpyzEnOqKruatK4R7D33fB1sZqkdXI33woV55dAx5tMOH01jpniucY\n\t2Lk4uusnu0yZBx2jIVNgZ7g/FNDRNVzjNA4T4=", "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=MsN40bmC3F41Rpw+7esE5zEV1IyiFFeClODJYRACcm4=;\n\tb=Dx5i+pvm76zWWDNtALZNYLuMBJTpo59H2OXHX/b06ItJX6E+iYSekpcKDRm7OCpgqr\n\tTIuGxRK3owpa/VwDm+7gyjkhQqQd7DGEQOYAnDGE/Iyb78r6UFJtJcthK/MvUE5NaPyz\n\tDvDKcIZszKtx2kWo0yp4Ndjg9yDdpV4rlMPkw6KxxBVOAQVLta1TR0bvFpq4QXCb93pO\n\tys6GSYI8GYcJg7OreQo0zYjQ6NQLUrZGqU9vS9keYwouPbdiGiKU6nd2/vloSMcMy5gv\n\t6I8vLm4TjCzVRB2sLxtWjeJOq2scKo6s0TstIeMM6sS1JdoNtfxVe0WiKvaungR9tfL+\n\tGZYg==", "X-Gm-Message-State": "AOAM532URXpVFdAAF15fb9IMONrOnInCoTg+CnHcmg98QQimtOW9Qp5l\n\t3YKqS1aBAFjr8H5s6b5GTYJQm4uWzYJ1NA==", "X-Google-Smtp-Source": "ABdhPJwKxeFvtKW4dWxkc9gGAtr9afIjQAEUHP4U8GYmXfcEm7QYt29K4kf3vpsS8bJqv7Xy5uv66g==", "X-Received": "by 2002:a17:902:da86:b029:ec:ad63:5ab with SMTP id\n\tj6-20020a170902da86b02900ecad6305abmr1870284plx.28.1619150865264; \n\tThu, 22 Apr 2021 21:07:45 -0700 (PDT)", "From": "Hirokazu Honda <hiroh@chromium.org>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Fri, 23 Apr 2021 13:07:37 +0900", "Message-Id": "<20210423040738.1227220-2-hiroh@chromium.org>", "X-Mailer": "git-send-email 2.31.1.498.g6c1eba8ee3d-goog", "In-Reply-To": "<20210423040738.1227220-1-hiroh@chromium.org>", "References": "<20210423040738.1227220-1-hiroh@chromium.org>", "MIME-Version": "1.0", "Subject": "[libcamera-devel] [PATCH v2 1/2] 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 | 70 +++++++++++++++++++++--------------\n src/android/camera_device.h | 9 ++++-\n 2 files changed, 50 insertions(+), 29 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex a71aee2f..c74dc0e7 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -748,27 +748,40 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n \n void CameraDevice::close()\n {\n-\tstreams_.clear();\n-\n-\tstop();\n+\tstd::scoped_lock<std::mutex> lock(mutex_);\n \n-\tcamera_->release();\n+\tstop(/*releaseCamera=*/true);\n }\n \n-void CameraDevice::stop()\n+void CameraDevice::stop(bool releaseCamera)\n {\n-\tif (!running_)\n+\tstreams_.clear();\n+\n+\tif (!running_) {\n+\t\tif (releaseCamera) {\n+\t\t\tdescriptors_.clear();\n+\t\t\tcamera_->release();\n+\t\t}\n \t\treturn;\n+\t}\n+\n+\tstopping_ = true;\n \n \tworker_.stop();\n \tcamera_->stop();\n \n \tdescriptors_.clear();\n+\n \trunning_ = false;\n+\tstopping_ = false;\n+\n+\tif (releaseCamera)\n+\t\tcamera_->release();\n }\n \n void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)\n {\n+\tstd::scoped_lock<std::mutex> lock(mutex_);\n \tcallbacks_ = callbacks;\n }\n \n@@ -1581,6 +1594,8 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()\n */\n const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n {\n+\tstd::scoped_lock<std::mutex> lock(mutex_);\n+\n \tauto it = requestTemplates_.find(type);\n \tif (it != requestTemplates_.end())\n \t\treturn it->second->get();\n@@ -1648,8 +1663,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> lock(mutex_);\n \n \tif (stream_list->num_streams == 0) {\n \t\tLOG(HAL, Error) << \"No streams in configuration\";\n@@ -1661,6 +1675,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@@ -1672,11 +1689,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \t}\n \n \t/*\n-\t * Clear and remove any existing configuration from previous calls, and\n-\t * ensure the required entries are available without further\n+\t * Ensure the required entries are available without further\n \t * reallocation.\n \t */\n-\tstreams_.clear();\n+\tASSERT(streams_.empty());\n \tstreams_.reserve(stream_list->num_streams);\n \n \tstd::vector<Camera3StreamConfig> streamConfigs;\n@@ -1912,6 +1928,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \tif (!isValidRequest(camera3Request))\n \t\treturn -EINVAL;\n \n+\tstd::scoped_lock<std::mutex> lock(mutex_);\n+\n \t/* Start the camera if that's the first request we handle. */\n \tif (!running_) {\n \t\tworker_.start();\n@@ -2015,33 +2033,31 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \n \tworker_.queueRequest(descriptor.request_.get());\n \n-\t{\n-\t\tstd::scoped_lock<std::mutex> lock(mutex_);\n-\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n-\t}\n+\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n \n \treturn 0;\n }\n \n void CameraDevice::requestComplete(Request *request)\n {\n+\tif (stopping_)\n+\t\treturn;\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-\t{\n-\t\tstd::scoped_lock<std::mutex> lock(mutex_);\n-\t\tauto it = descriptors_.find(request->cookie());\n-\t\tif (it == descriptors_.end()) {\n-\t\t\tLOG(HAL, Fatal)\n-\t\t\t\t<< \"Unknown request: \" << request->cookie();\n-\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n-\t\t\treturn;\n-\t\t}\n-\n-\t\tnode = descriptors_.extract(it);\n+\tauto it = descriptors_.find(request->cookie());\n+\tif (it == descriptors_.end()) {\n+\t\tLOG(HAL, Debug)\n+\t\t\t<< \"Unknown request: \" << request->cookie();\n+\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n+\t\treturn;\n \t}\n+\n+\tauto node = descriptors_.extract(it);\n \tCamera3RequestDescriptor &descriptor = node.mapped();\n \n \tif (request->status() != Request::RequestComplete) {\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex c63e8e21..08553584 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -88,7 +88,7 @@ private:\n \t\tint androidFormat;\n \t};\n \n-\tvoid stop();\n+\tvoid stop(bool releaseCamera = false);\n \n \tint initializeStreamConfigurations();\n \tstd::vector<libcamera::Size>\n@@ -127,7 +127,12 @@ private:\n \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n \tstd::vector<CameraStream> streams_;\n \n-\tstd::mutex mutex_; /* Protect descriptors_ */\n+\t/*\n+\t * Protect descriptors_ and camera_, and also avoid races by concurrent\n+\t * Camera HAL API calls.\n+\t */\n+\tstd::mutex mutex_;\n+\tstd::atomic_bool stopping_;\n \tstd::map<uint64_t, Camera3RequestDescriptor> descriptors_;\n \n \tstd::string maker_;\n", "prefixes": [ "libcamera-devel", "v2", "1/2" ] }