From patchwork Mon Apr 26 08:38:29 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hirokazu Honda X-Patchwork-Id: 12111 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 8B09CBDC99 for ; Mon, 26 Apr 2021 08:38:43 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 12C8F688AF; Mon, 26 Apr 2021 10:38:43 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="EJQz4C8/"; dkim-atps=neutral Received: from mail-pg1-x531.google.com (mail-pg1-x531.google.com [IPv6:2607:f8b0:4864:20::531]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 926AE688AF for ; Mon, 26 Apr 2021 10:38:41 +0200 (CEST) Received: by mail-pg1-x531.google.com with SMTP id s22so18495899pgk.6 for ; Mon, 26 Apr 2021 01:38:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=xpJ1iUDfDDXHcnvCiqwegzXaebN0Vojm3qpeP8L5ZPg=; b=EJQz4C8/VgpVE4wvb+udZIbNTeIOIA69lWHdsusBGqcFOzKt/1neW3MP4ULJJdOOkU CG7s2vYLG/nB8GKyJy8CYpogxZQoo8Rq5GjeyviAJlDyANBBu7KMGuQDaLcwBWaNjS7Y m7VCotglQKt+unKDANl70ENQHob818gXfNU/0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=xpJ1iUDfDDXHcnvCiqwegzXaebN0Vojm3qpeP8L5ZPg=; b=DMuZseBzhtN6FWy7CaLEt2uXpQ9XtNyH5kXWwc+FL8xqD72xo/WPHPuLPXOvSyiWSc gjCgIoCjAgGDjuhByEWMJI6ZZyF5nPqN8SvcmstZsVxe4XhwnwNs0Rk081WvsO1A2TK5 nJ5eDM876gYmMJZcSmT1PSTvgHTHUZ0EtfATGj+Ad2NeZdQcCJM1z2zeKoRc9JDHCWGc m8DlqdxdUVhwnjs3iIxzvycYnDPqwHx3sySCEfQH0VwfxUM9ju/jL9gCazQ4hgu0kiys 81lQI8ilKC+L/xcgMhpwus73AqFMgvJ4K613g7dsyA/jiGDLlO64DfircTlhVj65K2vD mgug== X-Gm-Message-State: AOAM531eg2ZG63VC0n4+LBI0+Bdyl3lGHh86irpr/LDgko5+LGEyn+at 55PB1p7XXAo/RS4TCChjydkQzMzzArFLBA== X-Google-Smtp-Source: ABdhPJzQcTvXzQ2r0aau+zOgI/YMS56qZEW2JZZHzMsjYTnQUGk8ZpJoK/CqcGjMz3xk0TJ+ShtYjw== X-Received: by 2002:a63:8948:: with SMTP id v69mr15891684pgd.264.1619426318878; Mon, 26 Apr 2021 01:38:38 -0700 (PDT) Received: from hiroh2.tok.corp.google.com ([2401:fa00:8f:2:a407:b8c2:d8b:a449]) by smtp.gmail.com with ESMTPSA id e18sm4873728pgr.8.2021.04.26.01.38.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Apr 2021 01:38:38 -0700 (PDT) From: Hirokazu Honda 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 race due to concurrent HAL calls 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" HAL API functions might be called by different threads in Android Camera HAL3 API, while process_capture_request() must be called by a single thread. Furthermore, close() and flush() can be called any time. Therefore, races to variables can occur among the HAL API functions. This prevents the races by enclosing HAL calls by mutex. It might not be the best in terms of the performance, but the simplicity is good as a first step and also further development. Signed-off-by: Hirokazu Honda --- src/android/camera_device.cpp | 59 ++++++++++++++++++++--------------- src/android/camera_device.h | 7 +++-- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index b3def04b..96c8d4a9 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -743,7 +743,7 @@ int CameraDevice::open(const hw_module_t *hardwareModule) void CameraDevice::close() { - streams_.clear(); + std::scoped_lock halLock(halMutex_); stop(); @@ -758,12 +758,17 @@ void CameraDevice::stop() worker_.stop(); camera_->stop(); - descriptors_.clear(); + { + std::scoped_lock lock(mutex_); + descriptors_.clear(); + } + running_ = false; } void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks) { + std::scoped_lock halLock(halMutex_); callbacks_ = callbacks; } @@ -1643,8 +1648,7 @@ PixelFormat CameraDevice::toPixelFormat(int format) const */ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) { - /* Before any configuration attempt, stop the camera. */ - stop(); + std::scoped_lock halLock(halMutex_); if (stream_list->num_streams == 0) { LOG(HAL, Error) << "No streams in configuration"; @@ -1656,6 +1660,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) return -EINVAL; #endif + /* Before any configuration attempt, stop the camera. */ + stop(); + /* * Generate an empty configuration, and construct a StreamConfiguration * for each camera3_stream to add to it. @@ -1671,6 +1678,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) * ensure the required entries are available without further * reallocation. */ + + std::scoped_lock lock(mutex_); streams_.clear(); streams_.reserve(stream_list->num_streams); @@ -1907,6 +1916,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques if (!isValidRequest(camera3Request)) return -EINVAL; + std::scoped_lock halLock(halMutex_); + /* Start the camera if that's the first request we handle. */ if (!running_) { worker_.start(); @@ -2022,29 +2033,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques void CameraDevice::requestComplete(Request *request) { + std::scoped_lock lock(mutex_); + const Request::BufferMap &buffers = request->buffers(); camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK; std::unique_ptr resultMetadata; - decltype(descriptors_)::node_type node; - std::unique_ptr captureRequest; - { - std::scoped_lock lock(mutex_); - auto requestIt = requests_.find(request->cookie()); - if (requestIt == requests_.end()) { - LOG(HAL, Fatal) - << "Unknown request: " << request->cookie(); - return; - } - captureRequest = std::move(requestIt->second); - requests_.erase(requestIt); + auto requestIt = requests_.find(request->cookie()); + if (requestIt == requests_.end()) { + LOG(HAL, Fatal) + << "Unknown request: " << request->cookie(); + return; + } + auto captureRequest = std::move(requestIt->second); + requests_.erase(requestIt); - auto it = descriptors_.find(request->cookie()); - if (it == descriptors_.end()) - return; + auto it = descriptors_.find(request->cookie()); + if (it == descriptors_.end()) + return; - node = descriptors_.extract(it); - } + auto node = descriptors_.extract(it); Camera3RequestDescriptor &descriptor = node.mapped(); if (request->status() != Request::RequestComplete) { @@ -2149,11 +2157,12 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) camera3_notify_msg_t notify = {}; /* - * \todo Report and identify the stream number or configuration to - * clarify the stream that failed. + * \todo Report and identify the stream number or configuration + * to clarify the stream that failed. */ - LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " (" - << toPixelFormat(stream->format).toString() << ")"; + LOG(HAL, Error) + << "Error occurred on frame " << descriptor.frameNumber_ << " (" + << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")"; notify.type = CAMERA3_MSG_ERROR; notify.message.error.error_stream = stream; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 95d77890..325fb967 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -125,9 +125,12 @@ private: std::vector streamConfigurations_; std::map formatsMap_; - std::vector streams_; - std::mutex mutex_; /* Protect descriptors_ and requests_. */ + /* Avoid races by concurrent Camera HAL API calls. */ + std::mutex halMutex_; + + std::mutex mutex_; /* Protect streams_, descriptors_ and requests_. */ + std::vector streams_; std::map descriptors_; std::map> requests_;