From patchwork Mon Sep 6 14:01:48 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 13631 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 0D679BDC71 for ; Mon, 6 Sep 2021 14:01:11 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C02FC69168; Mon, 6 Sep 2021 16:01:10 +0200 (CEST) Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id EE3C360137 for ; Mon, 6 Sep 2021 16:01:08 +0200 (CEST) Received: (Authenticated sender: jacopo@jmondi.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id ED2D44000E; Mon, 6 Sep 2021 14:01:07 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Mon, 6 Sep 2021 16:01:48 +0200 Message-Id: <20210906140152.636883-2-jacopo@jmondi.org> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20210906140152.636883-1-jacopo@jmondi.org> References: <20210906140152.636883-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 1/5] android: camera_device: Fix crash in calling CameraDevice::close() 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" From: Hirokazu Honda The problem is happening because we seem to add a CameraStream associated buffer (depending on the CameraStream::Type) to the Request, in CameraDevice::processCaptureRequest(). However, when the camera stops, all the current buffers are marked with FrameMetadata::FrameCancelled and proceed to completion. But the buffer associated with the CameraStream (that was previously added to the request) has now been cleared out with a part of streams_.clear(), even before the camera stop() has been invoked. Any access to those request buffers after they have been cleared, will result in a crash. Signed-off-by: Hirokazu Honda Reviewed-by: Laurent Pinchart Reviewed-by: Umang Jain Reviewed-by: Jacopo Mondi --- src/android/camera_device.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 8ca76719a50f..fda77db4540c 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule) void CameraDevice::close() { - streams_.clear(); - stop(); camera_->release(); @@ -457,6 +455,8 @@ void CameraDevice::stop() camera_->stop(); descriptors_.clear(); + streams_.clear(); + state_ = State::Stopped; } From patchwork Mon Sep 6 14:01:51 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 13632 X-Patchwork-Delegate: jacopo@jmondi.org 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 918F0BEFBE for ; Mon, 6 Sep 2021 14:01:15 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 4EB436916A; Mon, 6 Sep 2021 16:01:14 +0200 (CEST) Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id CBCE36916F for ; Mon, 6 Sep 2021 16:01:10 +0200 (CEST) Received: (Authenticated sender: jacopo@jmondi.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 6454A40011; Mon, 6 Sep 2021 14:01:10 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Mon, 6 Sep 2021 16:01:51 +0200 Message-Id: <20210906140152.636883-5-jacopo@jmondi.org> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20210906140152.636883-1-jacopo@jmondi.org> References: <20210906140152.636883-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 4/5] android: camera_worker: Notify fence wait failures 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" The CameraDevice class stores one CaptureRequestDescriptor instance for each capture request queued to the Camera. Such descriptors are cleared when the Camera completes the Request in the CameraDevice::requestComplete() slot. The CameraDevice class relies on an internal worker to off-load waiting on synchronization fences, and does unconditionally store the descriptor in the descriptors_ class member map to handle its lifetime. If waiting on a fence fails, the created descriptor remains in the descriptors_ map until the next call to stop() or close(), as requestComplete() will never be called, and the camera service is never notified about the queueing failure. Fix that by allowing the CameraWorker to notify to the CameraDevice if waiting on a fence has failed, by installing a new requestQueueFailed Signal. The Signal is emitted by the CameraWorker internal worker if waiting on a fence has failed. The CameraDevice is augmented with a slot connected to the Signal that removes the descriptor from the map and notifies the camera framework about the failure. Signed-off-by: Jacopo Mondi Reviewed-by: Hirokazu Honda Reviewed-by: Umang Jain --- src/android/camera_device.cpp | 22 +++++++++++++++++++++- src/android/camera_device.h | 1 + src/android/camera_worker.cpp | 10 ++++++---- src/android/camera_worker.h | 9 +++++++-- 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a0ea138d9499..0ce9b699bfae 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -244,7 +244,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor( * Create the CaptureRequest, stored as a unique_ptr<> to tie its * lifetime to the descriptor. */ - request_ = std::make_unique(camera); + request_ = std::make_unique(camera, camera3Request); } /* @@ -264,6 +264,7 @@ CameraDevice::CameraDevice(unsigned int id, std::shared_ptr camera) : id_(id), state_(State::Stopped), camera_(std::move(camera)), facing_(CAMERA_FACING_FRONT), orientation_(0) { + worker_.requestQueueFailed.connect(this, &CameraDevice::requestQueueFailed); camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); maker_ = "libcamera"; @@ -1060,6 +1061,25 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques return 0; } +void CameraDevice::requestQueueFailed(CaptureRequest *request) +{ + /* + * Remove the descriptor from the map if the worker has failed + * to process it and notify the camera service about the failure. + */ + MutexLocker descriptorsLock(descriptorsMutex_); + auto it = descriptors_.find(request->cookie()); + if (it == descriptors_.end()) { + LOG(HAL, Fatal) + << "Unknown request: " << request->cookie(); + return; + } + + descriptors_.extract(it); + + abortRequest(request->camera3Request()); +} + void CameraDevice::requestComplete(Request *request) { decltype(descriptors_)::node_type node; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 54c4cb9ab499..65456ecca7e3 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -83,6 +83,7 @@ private: std::vector> frameBuffers_; CameraMetadata settings_; std::unique_ptr request_; + const camera3_capture_request_t *camera3Request_; }; enum class State { diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp index 98dddd9eb13b..7f39fab01638 100644 --- a/src/android/camera_worker.cpp +++ b/src/android/camera_worker.cpp @@ -27,8 +27,9 @@ LOG_DECLARE_CATEGORY(HAL) * by the CameraWorker which queues it to the libcamera::Camera after handling * fences. */ -CaptureRequest::CaptureRequest(libcamera::Camera *camera) - : camera_(camera) +CaptureRequest::CaptureRequest(libcamera::Camera *camera, + const camera3_capture_request_t *camera3Request) + : camera_(camera), camera3Request_(camera3Request) { request_ = camera_->createRequest(reinterpret_cast(this)); } @@ -77,7 +78,7 @@ void CameraWorker::queueRequest(CaptureRequest *request) { /* Async process the request on the worker which runs its own thread. */ worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, - request); + this, request); } /* @@ -109,7 +110,7 @@ int CameraWorker::Worker::waitFence(int fence) return -errno; } -void CameraWorker::Worker::processRequest(CaptureRequest *request) +void CameraWorker::Worker::processRequest(CameraWorker *context, CaptureRequest *request) { /* Wait on all fences before queuing the Request. */ for (int fence : request->fences()) { @@ -121,6 +122,7 @@ void CameraWorker::Worker::processRequest(CaptureRequest *request) if (ret < 0) { LOG(HAL, Error) << "Failed waiting for fence: " << fence << ": " << strerror(-ret); + context->requestQueueFailed.emit(request); return; } } diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h index 67ae50bd9288..86f20f741e54 100644 --- a/src/android/camera_worker.h +++ b/src/android/camera_worker.h @@ -10,6 +10,7 @@ #include #include +#include #include #include @@ -22,7 +23,8 @@ class CameraDevice; class CaptureRequest { public: - CaptureRequest(libcamera::Camera *camera); + CaptureRequest(libcamera::Camera *camera, + const camera3_capture_request_t *camera3Request); const std::vector &fences() const { return acquireFences_; } libcamera::ControlList &controls() { return request_->controls(); } @@ -31,6 +33,7 @@ public: return request_->metadata(); } unsigned long cookie() const { return request_->cookie(); } + const camera3_capture_request_t *camera3Request() const { return camera3Request_; } void addBuffer(libcamera::Stream *stream, libcamera::FrameBuffer *buffer, int fence); @@ -40,6 +43,7 @@ private: libcamera::Camera *camera_; std::vector acquireFences_; std::unique_ptr request_; + const camera3_capture_request_t *camera3Request_; }; class CameraWorker : private libcamera::Thread @@ -51,6 +55,7 @@ public: void stop(); void queueRequest(CaptureRequest *request); + libcamera::Signal requestQueueFailed; protected: void run() override; @@ -59,7 +64,7 @@ private: class Worker : public libcamera::Object { public: - void processRequest(CaptureRequest *request); + void processRequest(CameraWorker *context, CaptureRequest *request); private: int waitFence(int fence);