From patchwork Fri Apr 23 04:07:37 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hirokazu Honda X-Patchwork-Id: 12091 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 3DBB0BDB15 for ; Fri, 23 Apr 2021 04:07:49 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id ECD7B68878; Fri, 23 Apr 2021 06:07:48 +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="adUF+8KN"; dkim-atps=neutral Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id CD38468875 for ; Fri, 23 Apr 2021 06:07:46 +0200 (CEST) Received: by mail-pj1-x1036.google.com with SMTP id nk8so9272891pjb.3 for ; Thu, 22 Apr 2021 21:07:46 -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=MsN40bmC3F41Rpw+7esE5zEV1IyiFFeClODJYRACcm4=; b=adUF+8KNWM5oWQCKSQydN+hszR+gJjPMWBGhOgZeqlS545Hte5j/hovj6S0ohWHWL9 xAO+uhyun/SmcnEpyzEnOqKruatK4R7D33fB1sZqkdXI33woV55dAx5tMOH01jpniucY 2Lk4uusnu0yZBx2jIVNgZ7g/FNDRNVzjNA4T4= 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=MsN40bmC3F41Rpw+7esE5zEV1IyiFFeClODJYRACcm4=; b=Dx5i+pvm76zWWDNtALZNYLuMBJTpo59H2OXHX/b06ItJX6E+iYSekpcKDRm7OCpgqr TIuGxRK3owpa/VwDm+7gyjkhQqQd7DGEQOYAnDGE/Iyb78r6UFJtJcthK/MvUE5NaPyz DvDKcIZszKtx2kWo0yp4Ndjg9yDdpV4rlMPkw6KxxBVOAQVLta1TR0bvFpq4QXCb93pO ys6GSYI8GYcJg7OreQo0zYjQ6NQLUrZGqU9vS9keYwouPbdiGiKU6nd2/vloSMcMy5gv 6I8vLm4TjCzVRB2sLxtWjeJOq2scKo6s0TstIeMM6sS1JdoNtfxVe0WiKvaungR9tfL+ GZYg== X-Gm-Message-State: AOAM532URXpVFdAAF15fb9IMONrOnInCoTg+CnHcmg98QQimtOW9Qp5l 3YKqS1aBAFjr8H5s6b5GTYJQm4uWzYJ1NA== X-Google-Smtp-Source: ABdhPJwKxeFvtKW4dWxkc9gGAtr9afIjQAEUHP4U8GYmXfcEm7QYt29K4kf3vpsS8bJqv7Xy5uv66g== X-Received: by 2002:a17:902:da86:b029:ec:ad63:5ab with SMTP id j6-20020a170902da86b02900ecad6305abmr1870284plx.28.1619150865264; Thu, 22 Apr 2021 21:07:45 -0700 (PDT) Received: from hiroh2.tok.corp.google.com ([2401:fa00:8f:2:8537:2d4f:8d35:5777]) by smtp.gmail.com with ESMTPSA id j23sm3229670pfh.179.2021.04.22.21.07.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Apr 2021 21:07:44 -0700 (PDT) From: Hirokazu Honda 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 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 | 70 +++++++++++++++++++++-------------- src/android/camera_device.h | 9 ++++- 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a71aee2f..c74dc0e7 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -748,27 +748,40 @@ int CameraDevice::open(const hw_module_t *hardwareModule) void CameraDevice::close() { - streams_.clear(); - - stop(); + std::scoped_lock lock(mutex_); - camera_->release(); + stop(/*releaseCamera=*/true); } -void CameraDevice::stop() +void CameraDevice::stop(bool releaseCamera) { - if (!running_) + streams_.clear(); + + if (!running_) { + if (releaseCamera) { + descriptors_.clear(); + camera_->release(); + } return; + } + + stopping_ = true; worker_.stop(); camera_->stop(); descriptors_.clear(); + running_ = false; + stopping_ = false; + + if (releaseCamera) + camera_->release(); } void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks) { + std::scoped_lock lock(mutex_); callbacks_ = callbacks; } @@ -1581,6 +1594,8 @@ std::unique_ptr CameraDevice::requestTemplateVideo() */ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type) { + std::scoped_lock lock(mutex_); + auto it = requestTemplates_.find(type); if (it != requestTemplates_.end()) return it->second->get(); @@ -1648,8 +1663,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 lock(mutex_); if (stream_list->num_streams == 0) { LOG(HAL, Error) << "No streams in configuration"; @@ -1661,6 +1675,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. @@ -1672,11 +1689,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) } /* - * Clear and remove any existing configuration from previous calls, and - * ensure the required entries are available without further + * Ensure the required entries are available without further * reallocation. */ - streams_.clear(); + ASSERT(streams_.empty()); streams_.reserve(stream_list->num_streams); std::vector streamConfigs; @@ -1912,6 +1928,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques if (!isValidRequest(camera3Request)) return -EINVAL; + std::scoped_lock lock(mutex_); + /* Start the camera if that's the first request we handle. */ if (!running_) { worker_.start(); @@ -2015,33 +2033,31 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques worker_.queueRequest(descriptor.request_.get()); - { - std::scoped_lock lock(mutex_); - descriptors_[descriptor.request_->cookie()] = std::move(descriptor); - } + descriptors_[descriptor.request_->cookie()] = std::move(descriptor); return 0; } void CameraDevice::requestComplete(Request *request) { + if (stopping_) + return; + + 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::scoped_lock lock(mutex_); - auto it = descriptors_.find(request->cookie()); - if (it == descriptors_.end()) { - LOG(HAL, Fatal) - << "Unknown request: " << request->cookie(); - status = CAMERA3_BUFFER_STATUS_ERROR; - return; - } - - node = descriptors_.extract(it); + auto it = descriptors_.find(request->cookie()); + if (it == descriptors_.end()) { + LOG(HAL, Debug) + << "Unknown request: " << request->cookie(); + status = CAMERA3_BUFFER_STATUS_ERROR; + return; } + + auto node = descriptors_.extract(it); Camera3RequestDescriptor &descriptor = node.mapped(); if (request->status() != Request::RequestComplete) { diff --git a/src/android/camera_device.h b/src/android/camera_device.h index c63e8e21..08553584 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -88,7 +88,7 @@ private: int androidFormat; }; - void stop(); + void stop(bool releaseCamera = false); int initializeStreamConfigurations(); std::vector @@ -127,7 +127,12 @@ private: std::map formatsMap_; std::vector streams_; - std::mutex mutex_; /* Protect descriptors_ */ + /* + * Protect descriptors_ and camera_, and also avoid races by concurrent + * Camera HAL API calls. + */ + std::mutex mutex_; + std::atomic_bool stopping_; std::map descriptors_; std::string maker_;