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_; From patchwork Fri Apr 23 04:07:38 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hirokazu Honda X-Patchwork-Id: 12092 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 02BA5BDB15 for ; Fri, 23 Apr 2021 04:07:52 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id B4CA368872; Fri, 23 Apr 2021 06:07:51 +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="fj2iZxD+"; dkim-atps=neutral Received: from mail-pj1-x1032.google.com (mail-pj1-x1032.google.com [IPv6:2607:f8b0:4864:20::1032]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D42468861 for ; Fri, 23 Apr 2021 06:07:48 +0200 (CEST) Received: by mail-pj1-x1032.google.com with SMTP id f6-20020a17090a6546b029015088cf4a1eso549347pjs.2 for ; Thu, 22 Apr 2021 21:07:48 -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=aYyEl85BGRXcwUXnJTysQs2J8kpH0ejwtwgIxJEe4R0=; b=fj2iZxD+b/Zg1Xpmuc3liLpcPsT0rti+ts1MUZHHJVM6xZshVSK8eSLbK6+Dftyavc 2VD3nBgc4on801FcEbZvcxOJfj5DDRShDOHI4BIpa3qOtnQPTQv5es+SL2NCCLQEMZ2f MoAAf8VjyrEsGWRNjbKBz5ZDN/4yxm/IGzpXI= 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=aYyEl85BGRXcwUXnJTysQs2J8kpH0ejwtwgIxJEe4R0=; b=Hi6aIf6O9wDhuSNYmeiuK8jWX3BAhuqaGUpVIw6iZPBemOC8FPYbS8XVvpDlAH70ho kf1AwxW0AUlaAMogckl68PnRxIMopFFsN1GTebgH8kR7YLGcQelMNaUHNJdJsn09mhyy AR1/PE9VRXYqgLTkRi+8xh9ubtOLSByLqjsj7v4ZnaegwlM/0gh9ZXk1aq3cGGmiZrCT jHKSEUXwi8L/b2PeQMzayXLWkywqhXJRpx0L+chrIeCyQ1wX812rLdD41ZSvqXbLmp72 73pG7MGpBU0BN6ZmPCJt2SmDZ2M0e+BzkHrrzL+pL0CZ/s1hYr6fUnce2vwGYYdDURqX /bnQ== X-Gm-Message-State: AOAM532kpwVozRaRBa1v00mZ509br/Dyg+dIkWWdIamfaj59OuQLdR56 g8bOvgLP9XxKJ+L25E7NkTjnQ4PKaT4h+A== X-Google-Smtp-Source: ABdhPJxb2QjyiY0it+Kbs2QjO7c40eJ64FNnzhiqyaymh4/zu+0mfwUfe41rquXdoxv1WNUzH3qs4Q== X-Received: by 2002:a17:90a:a613:: with SMTP id c19mr3579691pjq.117.1619150866680; Thu, 22 Apr 2021 21:07:46 -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.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Apr 2021 21:07:46 -0700 (PDT) From: Hirokazu Honda To: libcamera-devel@lists.libcamera.org Date: Fri, 23 Apr 2021 13:07:38 +0900 Message-Id: <20210423040738.1227220-3-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 2/2] android: CameraDevice: Implement HAL3 API flush 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" This implements flush(). It is mostly the same as close() though the canceled events are reported with error messages. Signed-off-by: Hirokazu Honda --- src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------ src/android/camera_device.h | 1 + src/android/camera_ops.cpp | 6 ++++- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index c74dc0e7..22a2e13c 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -753,6 +753,15 @@ void CameraDevice::close() stop(/*releaseCamera=*/true); } +int CameraDevice::flush() +{ + std::scoped_lock lock(mutex_); + + stop(); + + return 0; +} + void CameraDevice::stop(bool releaseCamera) { streams_.clear(); @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera) worker_.stop(); camera_->stop(); + for (auto &[cookie, descriptor] : descriptors_) { + LOG(HAL, Debug) << "request is canceled: " << cookie; + + camera3_capture_result_t captureResult = {}; + captureResult.frame_number = descriptor.frameNumber_; + captureResult.num_output_buffers = descriptor.buffers_.size(); + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + buffer.acquire_fence = -1; + buffer.release_fence = -1; + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; + } + captureResult.output_buffers = descriptor.buffers_.data(); + + notifyError(descriptor.frameNumber_, + descriptor.buffers_[0].stream); + callbacks_->process_capture_result(callbacks_, &captureResult); + } descriptors_.clear(); running_ = false; @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request) } if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) { + /* + * \todo Report and identify the stream number or configuration + * to clarify the stream that failed. + */ + LOG(HAL, Error) + << "Error occurred on frame " << descriptor.frameNumber_ << " (" + << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")"; + /* \todo Improve error handling. In case we notify an error * because the metadata generation fails, a shutter event has * already been notified for this frame number before the error @@ -2161,13 +2195,6 @@ 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. - */ - LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " (" - << toPixelFormat(stream->format).toString() << ")"; - notify.type = CAMERA3_MSG_ERROR; notify.message.error.error_stream = stream; notify.message.error.frame_number = frameNumber; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 08553584..7004c89a 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -41,6 +41,7 @@ public: int open(const hw_module_t *hardwareModule); void close(); + int flush(); unsigned int id() const { return id_; } camera3_device_t *camera3Device() { return &camera3Device_; } diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp index 696e8043..981a3d30 100644 --- a/src/android/camera_ops.cpp +++ b/src/android/camera_ops.cpp @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) { - return 0; + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast(dev->priv); + return camera->flush(); } int hal_dev_close(hw_device_t *hw_device)