From patchwork Mon Jan 20 00:24:19 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2687 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id E2FFA607A2 for ; Mon, 20 Jan 2020 01:24:42 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 6F7B4529 for ; Mon, 20 Jan 2020 01:24:42 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479882; bh=P9OoRePHre0KNNhl+EZscIO2aiOkwmaJW4o5E68bEl8=; h=From:To:Subject:Date:In-Reply-To:References:From; b=agc/yUowOinYDihsZhkjLgLi7/GfmR7qMXmmmLuJ+4ujZXLRZgNkuHmWwxHrnNbhZ 9niaAo6fI28FIqleuUU0neXzPXSEOVriNxjogx0eHyQ7pQ2Mdrg3NdDCMVHBfXPUXJ Zujcn2G+Nc6iNmu5qCN+c+o+6ZgQUtZnRJXnZeUc= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:19 +0200 Message-Id: <20200120002437.6633-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 01/19] test: buffer_import: Propagate status code from buffer allocation 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:43 -0000 The BufferSource::allocate() return value isn't propagated correctly, resulting in a test failure when the test should be skipped due to a missing vivid device. Fix it. While at it, return valid status codes from BufferSource::allocate() in all error cases, with proper diagnostic messages. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund Reviewed-by: Jacopo Mondi --- test/camera/buffer_import.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp index e7048335e031..16bfca9beeea 100644 --- a/test/camera/buffer_import.cpp +++ b/test/camera/buffer_import.cpp @@ -89,15 +89,23 @@ public: ret = video_->getFormat(&format); if (ret) { std::cout << "Failed to get format on output device" << std::endl; - return ret; + return TestFail; } format.size = config.size; format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false); - if (video_->setFormat(&format)) + if (video_->setFormat(&format)) { + std::cout << "Failed to set format on output device" << std::endl; return TestFail; + } - return video_->exportBuffers(config.bufferCount, &buffers_); + ret = video_->exportBuffers(config.bufferCount, &buffers_); + if (ret < 0) { + std::cout << "Failed to export buffers" << std::endl; + return TestFail; + } + + return TestPass; } const std::vector> &buffers() @@ -178,8 +186,8 @@ protected: BufferSource source; int ret = source.allocate(cfg); - if (ret < 0) - return TestFail; + if (ret != TestPass) + return ret; std::vector requests; for (const std::unique_ptr &buffer : source.buffers()) { From patchwork Mon Jan 20 00:24:20 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2689 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 2260A607C4 for ; Mon, 20 Jan 2020 01:24:43 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id BD9A6563 for ; Mon, 20 Jan 2020 01:24:42 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479882; bh=NQ6JOWDLOr67iSP/Erm1ZtqOU7E7ppsS0wuyFcdBQBA=; h=From:To:Subject:Date:In-Reply-To:References:From; b=dAxbqBKFDHJiZUpdVhU+E5qMnALCINBcU2lwstC5pyHhVpeyXywWvf5c8GrK9zJP3 KjerVz7ae8vESyyfZpf/ZHUhZnBZ7izfHlrsRQNh4c2t37ILzcesn978SuCuHYFIro NOYOALD6AB/YL6DzVAFVjk776eP9rX4HNiRa6NfA= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:20 +0200 Message-Id: <20200120002437.6633-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid deadlock with ConnectionTypeBlocking 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:43 -0000 ConnectionTypeBlocking always invokes the method through inter-thread message passing, which results in deadlocks if the sender and receiver live in the same thread. The deadlock can easily be avoided by turning the invocation into a direct call in this case. Do so to make ConnectionTypeBlocking easier to use when some of the senders live in the same thread as the receiver while the other senders don't. Extend the object-invoke test to cover this usage. While at it reformat the documentation to avoid long \brief lines. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund Reviewed-by: Jacopo Mondi --- src/libcamera/bound_method.cpp | 22 ++++++++++++++-------- test/object-invoke.cpp | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp index e18c2eb4c68e..9aa59dc3678f 100644 --- a/src/libcamera/bound_method.cpp +++ b/src/libcamera/bound_method.cpp @@ -35,16 +35,19 @@ namespace libcamera { * thread. * * \var ConnectionType::ConnectionTypeQueued - * \brief The receiver is invoked asynchronously in its thread when control - * returns to the thread's event loop. The sender proceeds without waiting for - * the invocation to complete. + * \brief The receiver is invoked asynchronously + * + * Invoke the receiver asynchronously in its thread when control returns to the + * thread's event loop. The sender proceeds without waiting for the invocation + * to complete. * * \var ConnectionType::ConnectionTypeBlocking - * \brief The receiver is invoked asynchronously in its thread when control - * returns to the thread's event loop. The sender blocks until the receiver - * signals the completion of the invocation. This connection type shall not be - * used when the sender and receiver live in the same thread, otherwise - * deadlock will occur. + * \brief The receiver is invoked synchronously + * + * If the sender and the receiver live in the same thread, this is equivalent to + * ConnectionTypeDirect. Otherwise, the receiver is invoked asynchronously in + * its thread when control returns to the thread's event loop. The sender + * blocks until the receiver signals the completion of the invocation. */ /** @@ -71,6 +74,9 @@ bool BoundMethodBase::activatePack(std::shared_ptr pack, type = ConnectionTypeDirect; else type = ConnectionTypeQueued; + } else if (type == ConnectionTypeBlocking) { + if (Thread::current() == object_->thread()) + type = ConnectionTypeDirect; } switch (type) { diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp index 8e2055ca620f..fa162c838c78 100644 --- a/test/object-invoke.cpp +++ b/test/object-invoke.cpp @@ -100,6 +100,26 @@ protected: return TestFail; } + /* + * Test that blocking invocation is delivered directly when the + * caller and callee live in the same thread. + */ + object_.reset(); + + object_.invokeMethod(&InvokedObject::method, + ConnectionTypeBlocking, 42); + + switch (object_.status()) { + case InvokedObject::NoCall: + cout << "Method not invoked for main thread (blocking)" << endl; + return TestFail; + case InvokedObject::InvalidThread: + cout << "Method invoked in incorrect thread for main thread (blocking)" << endl; + return TestFail; + default: + break; + } + /* * Move the object to a thread and verify that auto method * invocation is delivered in the correct thread. From patchwork Mon Jan 20 00:24:21 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2690 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C78F607A2 for ; Mon, 20 Jan 2020 01:24:43 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A854504 for ; Mon, 20 Jan 2020 01:24:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479883; bh=U27LJqHdUKh2eK/z9aPBMZOJpM+0sfi7/dMwnBwccGg=; h=From:To:Subject:Date:In-Reply-To:References:From; b=pPx0U0KNaDKGknCg0vGPNLpfSKhrHmF5iWqgEIrPaxYxS8Mf3lE3GGrp5lma9nPx3 mh0gSz+sStoPVoiavgXp3PSSUy9ym8XlB1u+sbzY8lW5PX4nUqNlKqUICRvP3izqgg 0MJwncuxoOcrsZYYIFMziVqRZyQdDtUZf38cCakk= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:21 +0200 Message-Id: <20200120002437.6633-4-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 03/19] libcamera: thread: Add a method to return the ID of the current thread 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:43 -0000 The current thread ID is useful when logging message to debug concurrency issues. Add a method to retrieve it. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund Reviewed-by: Jacopo Mondi --- src/libcamera/include/thread.h | 2 ++ src/libcamera/thread.cpp | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h index 37edd4f5138b..819a9879ac56 100644 --- a/src/libcamera/include/thread.h +++ b/src/libcamera/include/thread.h @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -39,6 +40,7 @@ public: Signal finished; static Thread *current(); + static pid_t currentId(); EventDispatcher *eventDispatcher(); void setEventDispatcher(std::unique_ptr dispatcher); diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index 18ebd16a7e2f..fe32cd677596 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -9,6 +9,9 @@ #include #include +#include +#include +#include #include @@ -62,6 +65,7 @@ private: Thread *thread_; bool running_; + pid_t tid_; Mutex mutex_; @@ -108,6 +112,7 @@ ThreadData *ThreadData::current() * started, set it here. */ ThreadData *data = mainThread.data_; + data->tid_ = syscall(SYS_gettid); currentThreadData = data; return data; } @@ -189,6 +194,7 @@ void Thread::startThread() */ thread_local ThreadCleaner cleaner(this, &Thread::finishThread); + data_->tid_ = syscall(SYS_gettid); currentThreadData = data_; run(); @@ -308,6 +314,20 @@ Thread *Thread::current() return data->thread_; } +/** + * \brief Retrieve the ID of the current thread + * + * The thread ID corresponds to the Linux thread ID (TID) as returned by the + * gettid system call. + * + * \return The ID of the current thread + */ +pid_t Thread::currentId() +{ + ThreadData *data = ThreadData::current(); + return data->tid_; +} + /** * \brief Set the event dispatcher * \param[in] dispatcher Pointer to the event dispatcher From patchwork Mon Jan 20 00:24:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2691 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id C3513607DB for ; Mon, 20 Jan 2020 01:24:43 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 6AA16529 for ; Mon, 20 Jan 2020 01:24:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479883; bh=Fi21uGDu0oFxJxnP0nwK/btgHGo7AKOL9YN71odSnMo=; h=From:To:Subject:Date:In-Reply-To:References:From; b=lH7l+Kv3zPSR9q/zG06Ck/hp6N4fEn20GlS/CtdnbnlutiS2stVSzP88/n4/D+ggn fv5EyMItUx++X1wO7kYb+UzmPhZ0wnaSeqR0vBo18bG3UAdkHEGHhscMkEjBeJJOIj stb+6gnjJD0HKJxVnveCruauJAXlPIaeuvkKu47I= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:22 +0200 Message-Id: <20200120002437.6633-5-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 04/19] libcamera: log: Print the thread ID in the log 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:44 -0000 The current thread ID is useful when debugging concurrency issues. Print it in log messages. The syslog target is left out as the thread ID would have little use there, and partly duplicates the process ID. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund Reviewed-by: Jacopo Mondi --- src/libcamera/log.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp index f4eb8c11adc3..1dac4666b435 100644 --- a/src/libcamera/log.cpp +++ b/src/libcamera/log.cpp @@ -22,6 +22,7 @@ #include +#include "thread.h" #include "utils.h" /** @@ -196,7 +197,8 @@ void LogOutput::write(const LogMessage &msg) break; case LoggingTargetStream: case LoggingTargetFile: - str = "[" + utils::time_point_to_string(msg.timestamp()) + "]" + str = "[" + utils::time_point_to_string(msg.timestamp()) + "] [" + + std::to_string(Thread::currentId()) + "]" + log_severity_name(msg.severity()) + " " + msg.category().name() + " " + msg.fileInfo() + " " + msg.msg(); From patchwork Mon Jan 20 00:24:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2692 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 829DE607E2 for ; Mon, 20 Jan 2020 01:24:44 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id BB29B504 for ; Mon, 20 Jan 2020 01:24:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479883; bh=siEXXoSdcqSOVH1xn3Cc+kKBHDs2MhxzxQOo7xyszdM=; h=From:To:Subject:Date:In-Reply-To:References:From; b=izEhKFB0YKlbKgWeeorg00jeTkJQ3SXZXcQlNgKJ9L2ERZyHRgVOrWnDqZ1S4NI3C V7O09+bgdqxCsbKHFWNfjlV95DtygR55EVA7ezjngFGly49dtUTrlc2e6OT5yxJNoA VeFRbmLhFY/XMGROUTdBzRZy5/u0YD7EDsNteFj8= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:23 +0200 Message-Id: <20200120002437.6633-6-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE with std::array 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:46 -0000 Replace the C-style arrays with std::array wherever the ARRAY_SIZE macro is used, removing the need for the macro completely. std::array combines the performance and accessibility of C-style arrays with the benefits of a standard container, which is shown here through the ability to carry its size. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Jacopo Mondi Reviewed-by: Kieran Bingham --- src/libcamera/camera.cpp | 10 +++++----- src/libcamera/include/utils.h | 2 -- src/libcamera/ipa_module.cpp | 8 ++++---- src/libcamera/log.cpp | 15 ++++++++------- src/libcamera/utils.cpp | 5 ----- src/libcamera/v4l2_videodevice.cpp | 7 ++++--- test/ipc/unixsocket.cpp | 8 ++++---- 7 files changed, 25 insertions(+), 30 deletions(-) diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 79a5f994f9bb..3385c08778b8 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -7,6 +7,7 @@ #include +#include #include #include @@ -15,7 +16,6 @@ #include "log.h" #include "pipeline_handler.h" -#include "utils.h" /** * \file camera.h @@ -404,7 +404,7 @@ Camera::~Camera() LOG(Camera, Error) << "Removing camera while still in use"; } -static const char *const camera_state_names[] = { +static constexpr std::array camera_state_names = { "Available", "Acquired", "Configured", @@ -416,8 +416,8 @@ bool Camera::stateBetween(State low, State high) const if (state_ >= low && state_ <= high) return true; - ASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) && - static_cast(high) < ARRAY_SIZE(camera_state_names)); + ASSERT(static_cast(low) < camera_state_names.size() && + static_cast(high) < camera_state_names.size()); LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] << " state trying operation requiring state between " @@ -432,7 +432,7 @@ bool Camera::stateIs(State state) const if (state_ == state) return true; - ASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names)); + ASSERT(static_cast(state) < camera_state_names.size()); LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] << " state trying operation requiring state " diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h index e467eb21c518..f55c52f72bd5 100644 --- a/src/libcamera/include/utils.h +++ b/src/libcamera/include/utils.h @@ -15,8 +15,6 @@ #include #include -#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0])) - #ifndef __DOXYGEN__ /* uClibc and uClibc-ng don't provide O_TMPFILE */ diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp index 2c355ea8b5e5..9c927a62b308 100644 --- a/src/libcamera/ipa_module.cpp +++ b/src/libcamera/ipa_module.cpp @@ -22,7 +22,6 @@ #include "log.h" #include "pipeline_handler.h" -#include "utils.h" /** * \file ipa_module.h @@ -480,7 +479,7 @@ bool IPAModule::match(PipelineHandler *pipe, */ bool IPAModule::isOpenSource() const { - static const char *osLicenses[] = { + static constexpr std::array osLicenses = { "GPL-2.0-only", "GPL-2.0-or-later", "GPL-3.0-only", @@ -491,9 +490,10 @@ bool IPAModule::isOpenSource() const "LGPL-3.0-or-later", }; - for (unsigned int i = 0; i < ARRAY_SIZE(osLicenses); i++) - if (!strcmp(osLicenses[i], info_.license)) + for (const char *license : osLicenses) { + if (!strcmp(license, info_.license)) return true; + } return false; } diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp index 1dac4666b435..ef0b81f77131 100644 --- a/src/libcamera/log.cpp +++ b/src/libcamera/log.cpp @@ -7,6 +7,7 @@ #include "log.h" +#include #if HAVE_BACKTRACE #include #endif @@ -83,7 +84,7 @@ static int log_severity_to_syslog(LogSeverity severity) static const char *log_severity_name(LogSeverity severity) { - static const char *const names[] = { + static constexpr std::array names = { " DBG", " INFO", " WARN", @@ -91,7 +92,7 @@ static const char *log_severity_name(LogSeverity severity) "FATAL", }; - if (static_cast(severity) < ARRAY_SIZE(names)) + if (static_cast(severity) < names.size()) return names[severity]; else return "UNKWN"; @@ -405,9 +406,9 @@ void Logger::backtrace() if (!output) return; - void *buffer[32]; - int num_entries = ::backtrace(buffer, ARRAY_SIZE(buffer)); - char **strings = backtrace_symbols(buffer, num_entries); + std::array buffer; + int num_entries = ::backtrace(buffer.data(), buffer.size()); + char **strings = backtrace_symbols(buffer.data(), num_entries); if (!strings) return; @@ -603,7 +604,7 @@ void Logger::parseLogLevels() */ LogSeverity Logger::parseLogLevel(const std::string &level) { - static const char *const names[] = { + static constexpr std::array names = { "DEBUG", "INFO", "WARN", @@ -620,7 +621,7 @@ LogSeverity Logger::parseLogLevel(const std::string &level) severity = LogInvalid; } else { severity = LogInvalid; - for (unsigned int i = 0; i < ARRAY_SIZE(names); ++i) { + for (unsigned int i = 0; i < names.size(); ++i) { if (names[i] == level) { severity = i; break; diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp index 4beffdab5eb6..74576797ee77 100644 --- a/src/libcamera/utils.cpp +++ b/src/libcamera/utils.cpp @@ -22,11 +22,6 @@ namespace libcamera { namespace utils { -/** - * \def ARRAY_SIZE(array) - * \brief Determine the number of elements in the static array. - */ - /** * \brief Strip the directory prefix from the path * \param[in] path The path to process diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 18220b81af21..51be1dcd7fff 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -7,6 +7,7 @@ #include "v4l2_videodevice.h" +#include #include #include #include @@ -991,13 +992,13 @@ int V4L2VideoDevice::exportBuffers(unsigned int count, for (unsigned i = 0; i < count; ++i) { struct v4l2_buffer buf = {}; - struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; + std::array planes = {}; buf.index = i; buf.type = bufferType_; buf.memory = memoryType_; - buf.length = ARRAY_SIZE(planes); - buf.m.planes = planes; + buf.length = planes.size(); + buf.m.planes = planes.data(); ret = ioctl(VIDIOC_QUERYBUF, &buf); if (ret < 0) { diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp index f53042b88720..5bf543c197fa 100644 --- a/test/ipc/unixsocket.cpp +++ b/test/ipc/unixsocket.cpp @@ -6,6 +6,7 @@ */ #include +#include #include #include #include @@ -21,7 +22,6 @@ #include "ipc_unixsocket.h" #include "test.h" #include "thread.h" -#include "utils.h" #define CMD_CLOSE 0 #define CMD_REVERSE 1 @@ -303,13 +303,13 @@ protected: IPCUnixSocket::Payload message, response; int ret; - static const char *strings[2] = { + static constexpr std::array strings = { "Foo", "Bar", }; int fds[2]; - for (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) { + for (unsigned int i = 0; i < strings.size(); i++) { unsigned int len = strlen(strings[i]); fds[i] = open("/tmp", O_TMPFILE | O_RDWR, @@ -331,7 +331,7 @@ protected: if (ret) return ret; - for (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) { + for (unsigned int i = 0; i < strings.size(); i++) { unsigned int len = strlen(strings[i]); char buf[len]; From patchwork Mon Jan 20 00:24:24 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2693 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id E9DAA607C4 for ; Mon, 20 Jan 2020 01:24:44 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8A625529 for ; Mon, 20 Jan 2020 01:24:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479884; bh=8HAk4qCr0p0EA2DP7p4x/yStMqE64E+iuD7/sZsNGtE=; h=From:To:Subject:Date:In-Reply-To:References:From; b=eXAT2eybpvGP29QXxe+hot9TFWuNntOYAN5FBjlrL3pWUHKw6GzA+Z+XVDGGM/1cy nQAlfxTMJG0SrjcuYMxLna3wD8Y0cJADMpaY+oFcZsJP/1Eca0CRoTT7wnty/OL7cX DbKRm0TpU+8CSrZ7XqnP01RvmYMTYp9PNbqJZeso= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:24 +0200 Message-Id: <20200120002437.6633-7-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 06/19] libcamera: bound_method: Use std::index_sequence 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:46 -0000 Now that we're using C++-14, replace the manual implementation of std::integer_sequence with std::index_sequence, a specialization of std::integer_sequence with the integer type equal to std::size_t. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- include/libcamera/bound_method.h | 38 +++++++++----------------------- 1 file changed, 10 insertions(+), 28 deletions(-) diff --git a/include/libcamera/bound_method.h b/include/libcamera/bound_method.h index ca501493bce2..7ffd2e426e18 100644 --- a/include/libcamera/bound_method.h +++ b/include/libcamera/bound_method.h @@ -10,6 +10,7 @@ #include #include #include +#include namespace libcamera { @@ -71,25 +72,6 @@ public: virtual void invokePack(BoundMethodPackBase *pack) = 0; protected: -#ifndef __DOXYGEN__ - /* - * This is a cheap partial implementation of std::integer_sequence<> - * from C++14. - */ - template - struct sequence { - }; - - template - struct generator : generator { - }; - - template - struct generator<0, S...> { - typedef sequence type; - }; -#endif - bool activatePack(std::shared_ptr pack, bool deleteMethod); @@ -107,11 +89,11 @@ public: using PackType = BoundMethodPack; private: - template - void invokePack(BoundMethodPackBase *pack, BoundMethodBase::sequence) + template + void invokePack(BoundMethodPackBase *pack, std::index_sequence) { PackType *args = static_cast(pack); - args->ret_ = invoke(std::get(args->args_)...); + args->ret_ = invoke(std::get(args->args_)...); } public: @@ -120,7 +102,7 @@ public: void invokePack(BoundMethodPackBase *pack) override { - invokePack(pack, typename BoundMethodBase::generator::type()); + invokePack(pack, std::make_index_sequence{}); } virtual R activate(Args... args, bool deleteMethod = false) = 0; @@ -134,12 +116,12 @@ public: using PackType = BoundMethodPack; private: - template - void invokePack(BoundMethodPackBase *pack, BoundMethodBase::sequence) + template + void invokePack(BoundMethodPackBase *pack, std::index_sequence) { - /* args is effectively unused when the sequence S is empty. */ + /* args is effectively unused when the sequence I is empty. */ PackType *args [[gnu::unused]] = static_cast(pack); - invoke(std::get(args->args_)...); + invoke(std::get(args->args_)...); } public: @@ -148,7 +130,7 @@ public: void invokePack(BoundMethodPackBase *pack) override { - invokePack(pack, typename BoundMethodBase::generator::type()); + invokePack(pack, std::make_index_sequence{}); } virtual void activate(Args... args, bool deleteMethod = false) = 0; From patchwork Mon Jan 20 00:24:25 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2694 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A5A7607D9 for ; Mon, 20 Jan 2020 01:24:45 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E6369563 for ; Mon, 20 Jan 2020 01:24:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479885; bh=xEe7fvbCYCHn301mk4SENmNkHS6ZAU4WulFUlUu0RBw=; h=From:To:Subject:Date:In-Reply-To:References:From; b=EcZCGBnABkuWH42FaE2B1XRuf3LpLDkPNEEFwWMtiCibGZKPR2l8tTVk7ospM67EH srKhmK0APS7VTjykXSt+AViu1/nFu6UBnizCEg2hxCOAr2YoE+MRxRrxBifpPqSHI3 cr3o2Rw5QmtUywki1/NCqNKUxP7ovB+uCh+udBvM= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:25 +0200 Message-Id: <20200120002437.6633-8-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 07/19] libcamera: Declare static local variables as const where applicable 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:47 -0000 We use static local variables to indicate errors in methods that return a const reference. The local variables can thus be const, make them so. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- src/libcamera/controls.cpp | 2 +- src/libcamera/formats.cpp | 2 +- src/libcamera/framebuffer_allocator.cpp | 2 +- src/libcamera/log.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 7d8a0e97ee3a..34a8c8dd9458 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -752,7 +752,7 @@ bool ControlList::contains(unsigned int id) const */ const ControlValue &ControlList::get(unsigned int id) const { - static ControlValue zero; + static const ControlValue zero; const ControlValue *val = find(id); if (!val) diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp index 6f0ffb6dc5a8..5f6552a4e06c 100644 --- a/src/libcamera/formats.cpp +++ b/src/libcamera/formats.cpp @@ -86,7 +86,7 @@ std::vector ImageFormats::formats() const */ const std::vector &ImageFormats::sizes(unsigned int format) const { - static std::vector empty; + static const std::vector empty; auto const &it = data_.find(format); if (it == data_.end()) diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index 207a13bd841d..a7588c7fe4c2 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -203,7 +203,7 @@ int FrameBufferAllocator::free(Stream *stream) const std::vector> & FrameBufferAllocator::buffers(Stream *stream) const { - static std::vector> empty; + static const std::vector> empty; auto iter = buffers_.find(stream); if (iter == buffers_.end()) diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp index ef0b81f77131..2bb09ac7a7bc 100644 --- a/src/libcamera/log.cpp +++ b/src/libcamera/log.cpp @@ -750,7 +750,7 @@ void LogCategory::setSeverity(LogSeverity severity) */ const LogCategory &LogCategory::defaultCategory() { - static LogCategory category("default"); + static const LogCategory category("default"); return category; } From patchwork Mon Jan 20 00:24:26 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2695 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id D8E31607F4 for ; Mon, 20 Jan 2020 01:24:45 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 420BE529 for ; Mon, 20 Jan 2020 01:24:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479885; bh=381ksBZRWm51n+Ev4Slj5OQOiaQATFkU/Qfy81qRFG4=; h=From:To:Subject:Date:In-Reply-To:References:From; b=i9r5lGEdnX62A3aV/AuF31JeMoxmONaFwqdNJUFTjYkMC+fXSgflMFLgDn2Ptbe+S 4gLJiGC7VOrI/K2mhw+7XI6ECuIw+81S8Jnf5vuUk1qJ3G9aVHJtBr266UOHoM49XN EFaZLDR5ipFxZ8rDpupCgOXv6x4ZNaYGNSedLxs8= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:26 +0200 Message-Id: <20200120002437.6633-9-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 08/19] test: signal: Add additional disconnection tests for Object 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:47 -0000 Add two tests that exercise the Signal::disconnect(Object *) and Signal::disconnect() methods, to verify that they correctly remove the signal from the connected object's list of signals. This triggers an issue that was detected through manual code inspection, and is expected to crash or at least generate valgrind warnings. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- test/signal.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/signal.cpp b/test/signal.cpp index 0054ed5a380d..f83ceb05f091 100644 --- a/test/signal.cpp +++ b/test/signal.cpp @@ -220,6 +220,30 @@ protected: delete dynamicSignal; delete slotObject; + /* + * Test that signal manual disconnection from Object removes + * the signal for the object. This shall not generate any + * valgrind warning. + */ + dynamicSignal = new Signal<>(); + slotObject = new SlotObject(); + dynamicSignal->connect(slotObject, &SlotObject::slot); + dynamicSignal->disconnect(slotObject); + delete dynamicSignal; + delete slotObject; + + /* + * Test that signal manual disconnection from all slots removes + * the signal for the object. This shall not generate any + * valgrind warning. + */ + dynamicSignal = new Signal<>(); + slotObject = new SlotObject(); + dynamicSignal->connect(slotObject, &SlotObject::slot); + dynamicSignal->disconnect(); + delete dynamicSignal; + delete slotObject; + /* Exercise the Object slot code paths. */ slotObject = new SlotObject(); signalVoid_.connect(slotObject, &SlotObject::slot); From patchwork Mon Jan 20 00:24:27 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2696 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 073AE607DD for ; Mon, 20 Jan 2020 01:24:46 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 931C6B57 for ; Mon, 20 Jan 2020 01:24:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479885; bh=5/f77IEbrZNeopV8uxm1wG8GJJMXo/vUPIp2sDTZG6c=; h=From:To:Subject:Date:In-Reply-To:References:From; b=wW0kFYMyDSt5tYKQDyd6FYE5WrwO55o6l9y/J0Y5JebkUgxB13gstDPa3x3xhVOIA Q4HLELsWEXr+dOqMiIfe66QHhE1hOnBHVh15ZTTYuh5WC17Z6hS6yhOp/oUz1uRiYN b/1zpyP2BngSq04xgRzfm13zYLgu/Rpw61J+2wOg= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:27 +0200 Message-Id: <20200120002437.6633-10-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 09/19] libcamera: signal: Make slots list private 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:47 -0000 The slots list is touched from most of the Signal template functions. In order to prepare for thread-safety, move handling of the lists list to a small number of non-template functions in the SignalBase class. This incidently fixes a bug in signal disconnection handling where the signal wasn't removed from the object's signals list, as pointed out by the signals unit test. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- include/libcamera/object.h | 4 +- include/libcamera/signal.h | 88 ++++++++++++++++---------------------- src/libcamera/object.cpp | 7 ++- src/libcamera/signal.cpp | 36 ++++++++++++++++ 4 files changed, 81 insertions(+), 54 deletions(-) diff --git a/include/libcamera/object.h b/include/libcamera/object.h index 9344af30bc79..4d16f3f2b610 100644 --- a/include/libcamera/object.h +++ b/include/libcamera/object.h @@ -48,9 +48,7 @@ protected: virtual void message(Message *msg); private: - template - friend class Signal; - friend class BoundMethodBase; + friend class SignalBase; friend class Thread; void notifyThreadMove(); diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h index 432d95d0ed1c..c13bb30f0636 100644 --- a/include/libcamera/signal.h +++ b/include/libcamera/signal.h @@ -7,6 +7,7 @@ #ifndef __LIBCAMERA_SIGNAL_H__ #define __LIBCAMERA_SIGNAL_H__ +#include #include #include #include @@ -19,23 +20,18 @@ namespace libcamera { class SignalBase { public: - template - void disconnect(T *obj) - { - for (auto iter = slots_.begin(); iter != slots_.end(); ) { - BoundMethodBase *slot = *iter; - if (slot->match(obj)) { - iter = slots_.erase(iter); - delete slot; - } else { - ++iter; - } - } - } + void disconnect(Object *object); protected: - friend class Object; - std::list slots_; + using SlotList = std::list; + + void connect(BoundMethodBase *slot); + void disconnect(std::function match); + + SlotList slots(); + +private: + SlotList slots_; }; template @@ -45,12 +41,7 @@ public: Signal() {} ~Signal() { - for (BoundMethodBase *slot : slots_) { - Object *object = slot->object(); - if (object) - object->disconnect(this); - delete slot; - } + disconnect(); } #ifndef __DOXYGEN__ @@ -59,8 +50,7 @@ public: ConnectionType type = ConnectionTypeAuto) { Object *object = static_cast(obj); - object->connect(this); - slots_.push_back(new BoundMethodMember(obj, object, func, type)); + SignalBase::connect(new BoundMethodMember(obj, object, func, type)); } template::value>::type * = nullptr> @@ -69,63 +59,62 @@ public: #endif void connect(T *obj, R (T::*func)(Args...)) { - slots_.push_back(new BoundMethodMember(obj, nullptr, func)); + SignalBase::connect(new BoundMethodMember(obj, nullptr, func)); } template void connect(R (*func)(Args...)) { - slots_.push_back(new BoundMethodStatic(func)); + SignalBase::connect(new BoundMethodStatic(func)); } void disconnect() { - for (BoundMethodBase *slot : slots_) - delete slot; - slots_.clear(); + SignalBase::disconnect([](SlotList::iterator &iter) { + return true; + }); } template void disconnect(T *obj) { - SignalBase::disconnect(obj); + SignalBase::disconnect([obj](SlotList::iterator &iter) { + return (*iter)->match(obj); + }); } template void disconnect(T *obj, R (T::*func)(Args...)) { - for (auto iter = slots_.begin(); iter != slots_.end(); ) { + SignalBase::disconnect([obj, func](SlotList::iterator &iter) { BoundMethodArgs *slot = static_cast *>(*iter); + + if (!slot->match(obj)) + return false; + /* * If the object matches the slot, the slot is * guaranteed to be a member slot, so we can safely * cast it to BoundMethodMember to match * func. */ - if (slot->match(obj) && - static_cast *>(slot)->match(func)) { - iter = slots_.erase(iter); - delete slot; - } else { - ++iter; - } - } + return static_cast *>(slot)->match(func); + }); } template void disconnect(R (*func)(Args...)) { - for (auto iter = slots_.begin(); iter != slots_.end(); ) { - BoundMethodArgs *slot = *iter; - if (slot->match(nullptr) && - static_cast *>(slot)->match(func)) { - iter = slots_.erase(iter); - delete slot; - } else { - ++iter; - } - } + SignalBase::disconnect([func](SlotList::iterator &iter) { + BoundMethodArgs *slot = + static_cast *>(*iter); + + if (!slot->match(nullptr)) + return false; + + return static_cast *>(slot)->match(func); + }); } void emit(Args... args) @@ -134,8 +123,7 @@ public: * Make a copy of the slots list as the slot could call the * disconnect operation, invalidating the iterator. */ - std::vector slots{ slots_.begin(), slots_.end() }; - for (BoundMethodBase *slot : slots) + for (BoundMethodBase *slot : slots()) static_cast *>(slot)->activate(args...); } }; diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp index 21aad5652b38..f2a8be172547 100644 --- a/src/libcamera/object.cpp +++ b/src/libcamera/object.cpp @@ -76,7 +76,12 @@ Object::Object(Object *parent) */ Object::~Object() { - for (SignalBase *signal : signals_) + /* + * Move signals to a private list to avoid concurrent iteration and + * deletion of items from Signal::disconnect(). + */ + std::list signals(std::move(signals_)); + for (SignalBase *signal : signals) signal->disconnect(this); if (pendingMessages_) diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp index 190033317c72..9bb7eca8ed6e 100644 --- a/src/libcamera/signal.cpp +++ b/src/libcamera/signal.cpp @@ -14,6 +14,42 @@ namespace libcamera { +void SignalBase::connect(BoundMethodBase *slot) +{ + Object *object = slot->object(); + if (object) + object->connect(this); + slots_.push_back(slot); +} + +void SignalBase::disconnect(Object *object) +{ + disconnect([object](SlotList::iterator &iter) { + return (*iter)->match(object); + }); +} + +void SignalBase::disconnect(std::function match) +{ + for (auto iter = slots_.begin(); iter != slots_.end(); ) { + if (match(iter)) { + Object *object = (*iter)->object(); + if (object) + object->disconnect(this); + + delete *iter; + iter = slots_.erase(iter); + } else { + ++iter; + } + } +} + +SignalBase::SlotList SignalBase::slots() +{ + return slots_; +} + /** * \class Signal * \brief Generic signal and slot communication mechanism From patchwork Mon Jan 20 00:24:28 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2697 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DC78607DF for ; Mon, 20 Jan 2020 01:24:46 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E34A7529 for ; Mon, 20 Jan 2020 01:24:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479886; bh=B55uMI/61cz8qwfQFgZpeVW8M2AhxylTUV0IPlyUnkA=; h=From:To:Subject:Date:In-Reply-To:References:From; b=BZq9KRZO0Q9pr8FUPLOyNE5jpBZlR8TMRmIHi9B3ZjQtW5LFO6grQ6vCfxdvR413c jCvDRA8bvthhYHmkqRp0pyqn8vaM8wvkCpo/G0/WH7HMHH7mEjXZNd+BkK5+U+LBkE U8K5QunDPwSJC+JeRUHAJ+AhBgHwIl3nnrAk5BR4= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:28 +0200 Message-Id: <20200120002437.6633-11-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 10/19] libcamera: Define the threading model 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:47 -0000 Document the design of libcamera's threading support, and prepare to document thread-safety of classes and functions with a doxygen alias command. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund Reviewed-by: Kieran Bingham --- Documentation/Doxyfile.in | 4 +- src/libcamera/thread.cpp | 82 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in index 8e6fbdbb92b6..734672ed15dc 100644 --- a/Documentation/Doxyfile.in +++ b/Documentation/Doxyfile.in @@ -239,7 +239,9 @@ TAB_SIZE = 4 # newlines (in the resulting output). You can put ^^ in the value part of an # alias to insert a newline as if a physical newline was in the original file. -ALIASES = +ALIASES = "context=\xrefitem context \"Thread Safety\" \"Thread Safety\"" +ALIASES += "threadbound=\ref thread-bound \"thread-bound\"" +ALIASES += "threadsafe=\ref thread-safe \"thread-safe\"" # This tag can be used to specify a number of word-keyword mappings (TCL only). # A mapping has the form "name=value". For example adding "class=itcl::class" diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index fe32cd677596..c1698d469a6c 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -19,6 +19,88 @@ #include "log.h" #include "message.h" +/** + * \page thread Thread Support + * + * libcamera supports multi-threaded applications through a threading model that + * sets precise rules to guarantee thread-safe usage of the API. Additionally, + * libcamera makes internal use of threads, and offers APIs that simplify + * interactions with application threads. Careful compliance with the threading + * model will ensure avoidance of race conditions. + * + * \section thread-reentrancy Reentrancy and Thread-Safety + * + * Through the documentation, several terms are used to define how classes and + * their member functions can be used from multiple threads. + * + * - A **reentrant** function may be called simultaneously from multiple + * threads if and only if each invocation uses a different instance of the + * class. This is the default for all member functions not explictly marked + * otherwise. + * + * - \anchor thread-safe A **thread-safe** function may be called + * simultaneously from multiple threads on the same instance of a class. A + * thread-safe function is thus reentrant. + * + * - \anchor thread-bound A **thread-bound** function may be called only from + * the thread that the class instances lives in (see section \ref + * thread-objects). For instances of classes that do not derive from the + * Object class, this is the thread in which the instance was created. A + * thread-bound function is not thread-safe, and may or may not be reentrant. + * + * Neither reentrancy nor thread-safety, in this context, mean that a function + * may be called simultaneously from the same thread, for instance from a + * callback invoked by the the function. This may deadlock and isn't allowed + * unless separately documented. + * + * A class is defined as reentrant, thread-safe or thread-bound if all its + * member functions are reentrant, thread-safe or thread-bound respectively. + * Some member functions may additionally be documented as having additional + * thread-related attributes. + * + * Most classes are reentrant but not thread-safe, as making them fully + * thread-safe would incur locking costs considered prohibitive for the + * expected use cases. + * + * \section thread-objects Threads and Objects + * + * Instances of the Object class and all its derived classes are thread-aware + * and are bound to the thread they are created in. They are said to *live* in + * a thread, and they interact with the event loop of their thread for the + * purpose of message passing and signal delivery. Messages posted to the + * object with Object::postMessage() will be delivered from the event loop of + * the thread that the object lives in. Signals delivered to the object, unless + * explicitly connected with ConnectionTypeDirect, will also be delivered from + * the object thread's event loop. + * + * All Object instances created by libcamera are bound to an internal thread, + * an applications don't need to provide an event loop to support them. Object + * instances created by applications require an event loop. It is the + * responsibility of applications to provide that event loop, either explicitly + * through CameraManager::setEventDispatcher(), or by running the default event + * loop provided by CameraManager::eventDispatcher() in their main thread. The + * main thread of an application is the one that calls CameraManager::start(). + * + * \section thread-signals Threads and Signals + * + * When sent to a receiver that does not inherit from the Object class, signals + * are delivered synchronously in the thread of the sender. When the receiver + * inherits from the Object class, delivery is by default asynchronous if the + * sender and receiver live in different threads. In that case, the signal is + * posted to the receiver's message queue and will be delivered from the + * receiver's event loop, running in the receiver's thread. This mechanism can + * be overridden by selecting a different connection type when calling + * Signal::connect(). + * + * Asynchronous signal delivery is used internally in libcamera, but is also + * available to applications if desired. To use this feature, applications + * shall create receiver classes that inherit from the Object class, and + * provide an event loop to the CameraManager as explained above. Note that + * Object instance created by the application are limited to living in the + * application's main thread. Creating Object instances from another thread of + * an application causes undefined behaviour. + */ + /** * \file thread.h * \brief Thread support From patchwork Mon Jan 20 00:24:29 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2698 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id D1AF6607F6 for ; Mon, 20 Jan 2020 01:24:46 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C3631176 for ; Mon, 20 Jan 2020 01:24:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479886; bh=JIkyqxH76PuuyQ9kB4LgACPD2JIYVU9OciQYvnKIeNY=; h=From:To:Subject:Date:In-Reply-To:References:From; b=gQ+AU2yXKYGxFX+LCqepELUvgEDMPN+e+mYZr78GshpXG2MN1o92jcMwsDUAPU4Oh gwY0Xa5TjOiPlskF7EJ4ewlQnff/cJdp+pyr3k33ehcILCFIr6PfKaFWtYt202bQX5 c8vCXBicB5kJqoinyyFgRC6TLIJrOdbz7y+xVYUI= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:29 +0200 Message-Id: <20200120002437.6633-12-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 11/19] libcamera: Document thread-safety attributes of core classes 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:47 -0000 Define the thread-safety attributes of the classes and methods that are either thread-safe or thread-bound. The CameraManager, Camera and PipelineHandler will be addressed separately. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/device_enumerator.cpp | 2 ++ src/libcamera/event_notifier.cpp | 2 ++ src/libcamera/ipc_unixsocket.cpp | 2 ++ src/libcamera/object.cpp | 10 ++++++++-- src/libcamera/thread.cpp | 6 +++++- src/libcamera/timer.cpp | 20 ++++++++++++-------- src/libcamera/v4l2_videodevice.cpp | 2 ++ 7 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp index a8b5c90f5a5d..64cdc132308a 100644 --- a/src/libcamera/device_enumerator.cpp +++ b/src/libcamera/device_enumerator.cpp @@ -190,6 +190,8 @@ DeviceEnumerator::~DeviceEnumerator() * with a warning message logged, without returning an error. Only errors that * prevent enumeration altogether shall be fatal. * + * \context This function is \threadbound. + * * \return 0 on success or a negative error code otherwise */ diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp index 4326b0b413e2..a9be686f79ae 100644 --- a/src/libcamera/event_notifier.cpp +++ b/src/libcamera/event_notifier.cpp @@ -99,6 +99,8 @@ EventNotifier::~EventNotifier() * * This function enables or disables the notifier. A disabled notifier ignores * events and does not emit the \ref activated signal. + * + * \context This function is \threadbound. */ void EventNotifier::setEnabled(bool enable) { diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp index eb1a50239188..6e5cab894a93 100644 --- a/src/libcamera/ipc_unixsocket.cpp +++ b/src/libcamera/ipc_unixsocket.cpp @@ -62,6 +62,8 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket) * communication method. The remote side then instantiates a socket, and binds * it to the other side by passing the file descriptor to bind(). At that point * the channel is operation and communication is bidirectional and symmmetrical. + * + * \context This class is \threadbound. */ IPCUnixSocket::IPCUnixSocket() diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp index f2a8be172547..99c3bf9a709b 100644 --- a/src/libcamera/object.cpp +++ b/src/libcamera/object.cpp @@ -110,6 +110,8 @@ Object::~Object() * Messages are delivered through the thread's event loop. If the thread is not * running its event loop the message will not be delivered until the event * loop gets started. + * + * \context This function is \threadsafe. */ void Object::postMessage(std::unique_ptr msg) { @@ -162,6 +164,8 @@ void Object::message(Message *msg) * are passed untouched. The caller shall ensure that any pointer argument * remains valid until the method is invoked. * + * \context This function is \threadsafe. + * * \return For connection types ConnectionTypeDirect and * ConnectionTypeBlocking, return the return value of the invoked method. For * connection type ConnectionTypeQueued, return a default-constructed R value. @@ -170,6 +174,7 @@ void Object::message(Message *msg) /** * \fn Object::thread() * \brief Retrieve the thread the object is bound to + * \context This function is \threadsafe. * \return The thread the object is bound to */ @@ -178,8 +183,7 @@ void Object::message(Message *msg) * \param[in] thread The target thread * * This method moves the object and all its children from the current thread to - * the new \a thread. It shall be called from the thread in which the object - * currently lives, otherwise the behaviour is undefined. + * the new \a thread. * * Before the object is moved, a Message::ThreadMoveMessage message is sent to * it. The message() method can be reimplement in derived classes to be notified @@ -187,6 +191,8 @@ void Object::message(Message *msg) * * Moving an object that has a parent is not allowed, and causes undefined * behaviour. + * + * \context This function is thread-bound. */ void Object::moveToThread(Thread *thread) { diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index c1698d469a6c..0cf6ff691315 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -40,7 +40,9 @@ * * - \anchor thread-safe A **thread-safe** function may be called * simultaneously from multiple threads on the same instance of a class. A - * thread-safe function is thus reentrant. + * thread-safe function is thus reentrant. Thread-safe functions may also be + * called simultaneously with any other reentrant function of the same class + * on the same instance. * * - \anchor thread-bound A **thread-bound** function may be called only from * the thread that the class instances lives in (see section \ref @@ -220,6 +222,8 @@ ThreadData *ThreadData::current() * called. A custom event dispatcher may be installed with * setEventDispatcher(), otherwise a poll-based event dispatcher is used. This * behaviour can be overriden by overloading the run() method. + * + * \context This class is \threadsafe. */ /** diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp index 4c68883204e8..24da51524efb 100644 --- a/src/libcamera/timer.cpp +++ b/src/libcamera/timer.cpp @@ -67,16 +67,18 @@ Timer::~Timer() * \brief Start or restart the timer with a timeout of \a msec * \param[in] msec The timer duration in milliseconds * - * This method shall be called from the thread the timer is associated with. If - * the timer is already running it will be stopped and restarted. + * If the timer is already running it will be stopped and restarted. + * + * \context This function is \threadbound. */ /** * \brief Start or restart the timer with a timeout of \a duration * \param[in] duration The timer duration in milliseconds * - * This method shall be called from the thread the timer is associated with. If - * the timer is already running it will be stopped and restarted. + * If the timer is already running it will be stopped and restarted. + * + * \context This function is \threadbound. */ void Timer::start(std::chrono::milliseconds duration) { @@ -87,8 +89,9 @@ void Timer::start(std::chrono::milliseconds duration) * \brief Start or restart the timer with a \a deadline * \param[in] deadline The timer deadline * - * This method shall be called from the thread the timer is associated with. If - * the timer is already running it will be stopped and restarted. + * If the timer is already running it will be stopped and restarted. + * + * \context This function is \threadbound. */ void Timer::start(std::chrono::steady_clock::time_point deadline) { @@ -115,8 +118,9 @@ void Timer::start(std::chrono::steady_clock::time_point deadline) * After this function returns the timer is guaranteed not to emit the * \ref timeout signal. * - * This method shall be called from the thread the timer is associated with. If - * the timer is not running this function performs no operation. + * If the timer is not running this function performs no operation. + * + * \context This function is \threadbound. */ void Timer::stop() { diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 51be1dcd7fff..cb166c795abd 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -404,6 +404,8 @@ const std::string V4L2DeviceFormat::toString() const * * Upon destruction any device left open will be closed, and any resources * released. + * + * \context This class is \threadbound. */ /** From patchwork Mon Jan 20 00:24:30 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2699 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id E66CE607C4 for ; Mon, 20 Jan 2020 01:24:46 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8AF34504 for ; Mon, 20 Jan 2020 01:24:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479886; bh=51/YLrAB8DXWNr/g/YSoeq08tZX7TNIoH3L7qTNvrCc=; h=From:To:Subject:Date:In-Reply-To:References:From; b=pptF5bsc/7BBVl5QEWBemumLzV38G+XXssLMHGdSP97zArUmguqP0XYAVL0c2tk8a u4n8d0/+4K0ubNSdk+nPARoR8/ICzR5ZPMKEO7JcAg5iBscuy0ks/qkIqvtnnp8TMT y8SJ0UCFKgd8U4WyHxXK45Nwb3juoXpdKPsAT2bk= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:30 +0200 Message-Id: <20200120002437.6633-13-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 12/19] libcamera: signal: Make connection and disconnection thread-safe 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:47 -0000 Make the signal connection and disconnection thread-safe, and document them as such. This is required to make objects connectable from different threads. The connect(), disconnect() and emit() methods are now all protected by a global mutex, which may generate a high lock contention. This could be improved with finer-grained locks or with a pool of mutexes. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- src/libcamera/signal.cpp | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp index 9bb7eca8ed6e..6eab1fa74d42 100644 --- a/src/libcamera/signal.cpp +++ b/src/libcamera/signal.cpp @@ -7,6 +7,8 @@ #include +#include "thread.h" + /** * \file signal.h * \brief Signal & slot implementation @@ -14,8 +16,21 @@ namespace libcamera { +namespace { + +/* + * Mutex to protect the SignalBase::slots_ and Object::signals_ lists. If lock + * contention needs to be decreased, this could be replaced with locks in + * Object and SignalBase, or with a mutex pool. + */ +Mutex signalsLock; + +} /* namespace */ + void SignalBase::connect(BoundMethodBase *slot) { + MutexLocker locker(signalsLock); + Object *object = slot->object(); if (object) object->connect(this); @@ -31,6 +46,8 @@ void SignalBase::disconnect(Object *object) void SignalBase::disconnect(std::function match) { + MutexLocker locker(signalsLock); + for (auto iter = slots_.begin(); iter != slots_.end(); ) { if (match(iter)) { Object *object = (*iter)->object(); @@ -47,6 +64,7 @@ void SignalBase::disconnect(std::function match) SignalBase::SlotList SignalBase::slots() { + MutexLocker locker(signalsLock); return slots_; } @@ -99,23 +117,31 @@ SignalBase::SlotList SignalBase::slots() * disconnected from the \a func slot of \a object when \a object is destroyed. * Otherwise the caller shall disconnect signals manually before destroying \a * object. + * + * \context This function is \threadsafe. */ /** * \fn Signal::connect(R (*func)(Args...)) * \brief Connect the signal to a static function slot * \param[in] func The slot static function + * + * \context This function is \threadsafe. */ /** * \fn Signal::disconnect() * \brief Disconnect the signal from all slots + * + * \context This function is \threadsafe. */ /** * \fn Signal::disconnect(T *object) * \brief Disconnect the signal from all slots of the \a object * \param[in] object The object pointer whose slots to disconnect + * + * \context This function is \threadsafe. */ /** @@ -123,12 +149,16 @@ SignalBase::SlotList SignalBase::slots() * \brief Disconnect the signal from the \a object slot member function \a func * \param[in] object The object pointer whose slots to disconnect * \param[in] func The slot member function to disconnect + * + * \context This function is \threadsafe. */ /** * \fn Signal::disconnect(R (*func)(Args...)) * \brief Disconnect the signal from the slot static function \a func * \param[in] func The slot static function to disconnect + * + * \context This function is \threadsafe. */ /** @@ -141,6 +171,9 @@ SignalBase::SlotList SignalBase::slots() * function are passed to the slot functions unchanged. If a slot modifies one * of the arguments (when passed by pointer or reference), the modification is * thus visible to all subsequently called slots. + * + * This function is not \threadsafe, but thread-safety is guaranteed against + * concurrent connect() and disconnect() calls. */ } /* namespace libcamera */ From patchwork Mon Jan 20 00:24:31 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2700 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 49D9F60454 for ; Mon, 20 Jan 2020 01:24:47 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id DEB76529 for ; Mon, 20 Jan 2020 01:24:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479887; bh=cE/6Q8cXp+khS6Q2njjhgxZTloE72CLePidnvjviz6c=; h=From:To:Subject:Date:In-Reply-To:References:From; b=rCEMeOKRnqRptrksxyHwepFfl+anwe9m6QFNuOMNSVsXQm+7QhKkjiAE1SDTrUl1+ juKZaBDTgMI42lJfodwnExI8+8qqoXp24iBnZavJFyQhR+VL6cnL7dFsRO0kO3HxD/ wdVSmZV4KwnJ6ZzwaptVEeDB6t6sRsqV3EHBIPVQ= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:31 +0200 Message-Id: <20200120002437.6633-14-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 13/19] libcamera: camera_manager: Run the camera manager in a thread 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:47 -0000 Relying on the application event loop to process all our internal events is a bad idea for multiple reasons. In many cases the user of libcamera can't provide an event loop, for instance when running through one of the adaptation layers. The Android camera HAL and V4L2 compatibility layer create a thread for this reason, and the GStreamer element would need to do so as well. Furthermore, relying on the application event loop pushes libcamera's realtime constraints to the application, which isn't manageable. For these reasons it's desirable to always run the camera manager, the pipeline handlers and the cameras in a separate thread. Doing so isn't too complicated, it only involves creating the thread internally when starting the camera manager, and synchronizing a few methods of the Camera class. Do so as a first step towards defining the threading model of libcamera. Making CameraManager::cameras() thread-safe requires returning a copy of the cameras vector instead of a reference. This is also required for hot-plugging support and is thus desirable. The event dispatcher interface is still exposed to applications, to enable cross-thread signal delivery if desired. Signed-off-by: Laurent Pinchart --- Documentation/Doxyfile.in | 1 + include/libcamera/camera_manager.h | 13 +- src/libcamera/camera_manager.cpp | 295 +++++++++++++++++++++-------- 3 files changed, 224 insertions(+), 85 deletions(-) diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in index 734672ed15dc..1c46b04b3f7e 100644 --- a/Documentation/Doxyfile.in +++ b/Documentation/Doxyfile.in @@ -879,6 +879,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \ libcamera::BoundMethodPackBase \ libcamera::BoundMethodStatic \ libcamera::SignalBase \ + libcamera::*::Private \ std::* # The EXAMPLE_PATH tag can be used to specify one or more files or directories diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 094197668698..079f848aec79 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -7,7 +7,6 @@ #ifndef __LIBCAMERA_CAMERA_MANAGER_H__ #define __LIBCAMERA_CAMERA_MANAGER_H__ -#include #include #include #include @@ -18,9 +17,7 @@ namespace libcamera { class Camera; -class DeviceEnumerator; class EventDispatcher; -class PipelineHandler; class CameraManager : public Object { @@ -33,7 +30,7 @@ public: int start(); void stop(); - const std::vector> &cameras() const { return cameras_; } + std::vector> cameras() const; std::shared_ptr get(const std::string &name); std::shared_ptr get(dev_t devnum); @@ -46,13 +43,11 @@ public: EventDispatcher *eventDispatcher(); private: - std::unique_ptr enumerator_; - std::vector> pipes_; - std::vector> cameras_; - std::map> camerasByDevnum_; - static const std::string version_; static CameraManager *self_; + + class Private; + std::unique_ptr p_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index a71caf8536bb..64aa89975701 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -7,6 +7,9 @@ #include +#include +#include + #include #include @@ -26,6 +29,184 @@ namespace libcamera { LOG_DEFINE_CATEGORY(Camera) +class CameraManager::Private : public Thread +{ +public: + Private(CameraManager *cm); + + int start(); + void addCamera(std::shared_ptr &camera, dev_t devnum); + void removeCamera(Camera *camera); + + /* + * This mutex protects + * + * - initialized_ and status_ during initialization + * - cameras_ and camerasByDevnum_ after initialization + */ + Mutex mutex_; + std::vector> cameras_; + std::map> camerasByDevnum_; + +protected: + void run() override; + +private: + int init(); + void cleanup(); + + CameraManager *cm_; + + std::condition_variable cv_; + bool initialized_; + int status_; + + std::vector> pipes_; + std::unique_ptr enumerator_; +}; + +CameraManager::Private::Private(CameraManager *cm) + : cm_(cm), initialized_(false) +{ +} + +int CameraManager::Private::start() +{ + /* Start the thread and wait for initialization to complete. */ + Thread::start(); + + { + MutexLocker locker(mutex_); + cv_.wait(locker, [&] { return initialized_; }); + } + + /* If a failure happened during initialization, stop the thread. */ + if (status_ < 0) { + exit(); + wait(); + return status_; + } + + return 0; +} + +void CameraManager::Private::addCamera(std::shared_ptr &camera, + dev_t devnum) +{ + MutexLocker locker(mutex_); + + for (std::shared_ptr c : cameras_) { + if (c->name() == camera->name()) { + LOG(Camera, Warning) + << "Registering camera with duplicate name '" + << camera->name() << "'"; + break; + } + } + + cameras_.push_back(std::move(camera)); + + if (devnum) { + unsigned int index = cameras_.size() - 1; + camerasByDevnum_[devnum] = cameras_[index]; + } +} + +void CameraManager::Private::removeCamera(Camera *camera) +{ + MutexLocker locker(mutex_); + + auto iter = std::find_if(cameras_.begin(), cameras_.end(), + [camera](std::shared_ptr &c) { + return c.get() == camera; + }); + if (iter == cameras_.end()) + return; + + LOG(Camera, Debug) + << "Unregistering camera '" << camera->name() << "'"; + + auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(), + [camera](const std::pair> &p) { + return p.second.lock().get() == camera; + }); + if (iter_d != camerasByDevnum_.end()) + camerasByDevnum_.erase(iter_d); + + cameras_.erase(iter); +} + +void CameraManager::Private::run() +{ + LOG(Camera, Debug) << "Starting camera manager"; + + int ret = init(); + + mutex_.lock(); + status_ = ret; + initialized_ = true; + mutex_.unlock(); + cv_.notify_one(); + + if (ret < 0) + return; + + /* Now start processing events and messages. */ + exec(); + + cleanup(); +} + +int CameraManager::Private::init() +{ + enumerator_ = DeviceEnumerator::create(); + if (!enumerator_ || enumerator_->enumerate()) + return -ENODEV; + + /* + * TODO: Try to read handlers and order from configuration + * file and only fallback on all handlers if there is no + * configuration file. + */ + std::vector &factories = PipelineHandlerFactory::factories(); + + for (PipelineHandlerFactory *factory : factories) { + /* + * Try each pipeline handler until it exhaust + * all pipelines it can provide. + */ + while (1) { + std::shared_ptr pipe = factory->create(cm_); + if (!pipe->match(enumerator_.get())) + break; + + LOG(Camera, Debug) + << "Pipeline handler \"" << factory->name() + << "\" matched"; + pipes_.push_back(std::move(pipe)); + } + } + + /* TODO: register hot-plug callback here */ + + return 0; +} + +void CameraManager::Private::cleanup() +{ + /* TODO: unregister hot-plug callback here */ + + /* + * Release all references to cameras and pipeline handlers to ensure + * they all get destroyed before the device enumerator deletes the + * media devices. + */ + pipes_.clear(); + cameras_.clear(); + + enumerator_.reset(nullptr); +} + /** * \class CameraManager * \brief Provide access and manage all cameras in the system @@ -62,7 +243,7 @@ LOG_DEFINE_CATEGORY(Camera) CameraManager *CameraManager::self_ = nullptr; CameraManager::CameraManager() - : enumerator_(nullptr) + : p_(new CameraManager::Private(this)) { if (self_) LOG(Camera, Fatal) @@ -73,6 +254,8 @@ CameraManager::CameraManager() CameraManager::~CameraManager() { + stop(); + self_ = nullptr; } @@ -88,41 +271,16 @@ CameraManager::~CameraManager() */ int CameraManager::start() { - if (enumerator_) - return -EBUSY; - LOG(Camera, Info) << "libcamera " << version_; - enumerator_ = DeviceEnumerator::create(); - if (!enumerator_ || enumerator_->enumerate()) - return -ENODEV; - - /* - * TODO: Try to read handlers and order from configuration - * file and only fallback on all handlers if there is no - * configuration file. - */ - std::vector &factories = PipelineHandlerFactory::factories(); + int ret = p_->start(); + if (ret < 0) { + LOG(Camera, Error) << "Failed to start camera manager: " + << strerror(-ret); - for (PipelineHandlerFactory *factory : factories) { - /* - * Try each pipeline handler until it exhaust - * all pipelines it can provide. - */ - while (1) { - std::shared_ptr pipe = factory->create(this); - if (!pipe->match(enumerator_.get())) - break; - - LOG(Camera, Debug) - << "Pipeline handler \"" << factory->name() - << "\" matched"; - pipes_.push_back(std::move(pipe)); - } + return ret; } - /* TODO: register hot-plug callback here */ - return 0; } @@ -138,17 +296,8 @@ int CameraManager::start() */ void CameraManager::stop() { - /* TODO: unregister hot-plug callback here */ - - /* - * Release all references to cameras and pipeline handlers to ensure - * they all get destroyed before the device enumerator deletes the - * media devices. - */ - pipes_.clear(); - cameras_.clear(); - - enumerator_.reset(nullptr); + p_->exit(); + p_->wait(); } /** @@ -158,8 +307,16 @@ void CameraManager::stop() * Before calling this function the caller is responsible for ensuring that * the camera manager is running. * + * \context This function is \threadsafe. + * * \return List of all available cameras */ +std::vector> CameraManager::cameras() const +{ + MutexLocker locker(p_->mutex_); + + return p_->cameras_; +} /** * \brief Get a camera based on name @@ -168,11 +325,15 @@ void CameraManager::stop() * Before calling this function the caller is responsible for ensuring that * the camera manager is running. * + * \context This function is \threadsafe. + * * \return Shared pointer to Camera object or nullptr if camera not found */ std::shared_ptr CameraManager::get(const std::string &name) { - for (std::shared_ptr camera : cameras_) { + MutexLocker locker(p_->mutex_); + + for (std::shared_ptr camera : p_->cameras_) { if (camera->name() == name) return camera; } @@ -191,13 +352,17 @@ std::shared_ptr CameraManager::get(const std::string &name) * Before calling this function the caller is responsible for ensuring that * the camera manager is running. * + * \context This function is \threadsafe. + * * \return Shared pointer to Camera object, which is empty if the camera is * not found */ std::shared_ptr CameraManager::get(dev_t devnum) { - auto iter = camerasByDevnum_.find(devnum); - if (iter == camerasByDevnum_.end()) + MutexLocker locker(p_->mutex_); + + auto iter = p_->camerasByDevnum_.find(devnum); + if (iter == p_->camerasByDevnum_.end()) return nullptr; return iter->second.lock(); @@ -214,24 +379,14 @@ std::shared_ptr CameraManager::get(dev_t devnum) * * \a devnum is used by the V4L2 compatibility layer to map V4L2 device nodes * to Camera instances. + * + * \context This function shall be called from the CameraManager thread. */ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum) { - for (std::shared_ptr c : cameras_) { - if (c->name() == camera->name()) { - LOG(Camera, Warning) - << "Registering camera with duplicate name '" - << camera->name() << "'"; - break; - } - } - - cameras_.push_back(std::move(camera)); + ASSERT(Thread::current() == p_.get()); - if (devnum) { - unsigned int index = cameras_.size() - 1; - camerasByDevnum_[devnum] = cameras_[index]; - } + p_->addCamera(camera, devnum); } /** @@ -241,32 +396,20 @@ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum) * This function is called by pipeline handlers to unregister cameras from the * camera manager. Unregistered cameras won't be reported anymore by the * cameras() and get() calls, but references may still exist in applications. + * + * \context This function shall be called from the CameraManager thread. */ void CameraManager::removeCamera(Camera *camera) { - auto iter = std::find_if(cameras_.begin(), cameras_.end(), - [camera](std::shared_ptr &c) { - return c.get() == camera; - }); - if (iter == cameras_.end()) - return; - - LOG(Camera, Debug) - << "Unregistering camera '" << camera->name() << "'"; - - auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(), - [camera](const std::pair> &p) { - return p.second.lock().get() == camera; - }); - if (iter_d != camerasByDevnum_.end()) - camerasByDevnum_.erase(iter_d); + ASSERT(Thread::current() == p_.get()); - cameras_.erase(iter); + p_->removeCamera(camera); } /** * \fn const std::string &CameraManager::version() * \brief Retrieve the libcamera version string + * \context This function is \a threadsafe. * \return The libcamera version string */ From patchwork Mon Jan 20 00:24:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2701 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id D5158607EC for ; Mon, 20 Jan 2020 01:24:47 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A4DA504 for ; Mon, 20 Jan 2020 01:24:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479887; bh=biG6RcQKVHJgIhFHeFwbZz0F4ULtzgUJp36cqcQg95s=; h=From:To:Subject:Date:In-Reply-To:References:From; b=QZ5IxpYr9xg8Zgpz7WEhpisvgqaKCTsdHf38m7VDBPkRW4HzuLtX5UArZLNFmGvap HpYTwUaUZYWAQOuFUKfULRVmk0o20g7u0kskqf5jPvPZEPyMnZO/vHscyb90XH/CEl JUe/dN2+29RD4Jjind1LhgRtGwrUJ4PClTBaX/MQ= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:32 +0200 Message-Id: <20200120002437.6633-15-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 14/19] libcamera: camera: Move private data members to private implementation 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:48 -0000 Use the pimpl paradigm ([1]) to hide the private data members from the Camera class interface. This will ease maintaining ABI compatibility, and prepares for the implementation of the Camera class threading model. The FrameBufferAllocator class accesses the Camera private data members directly. In order to hide them, this pattern is replaced with new private member functions in the Camera class, and the FrameBufferAllocator is updated accordingly. [1] https://en.cppreference.com/w/cpp/language/pimpl Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- include/libcamera/camera.h | 28 +-- src/libcamera/camera.cpp | 224 +++++++++++++++--------- src/libcamera/framebuffer_allocator.cpp | 47 ++--- 3 files changed, 161 insertions(+), 138 deletions(-) diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 6597ade83288..c37319eda2dc 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -98,34 +98,22 @@ public: int stop(); private: - enum State { - CameraAvailable, - CameraAcquired, - CameraConfigured, - CameraRunning, - }; - - Camera(PipelineHandler *pipe, const std::string &name); + Camera(PipelineHandler *pipe, const std::string &name, + const std::set &streams); ~Camera(); - bool stateBetween(State low, State high) const; - bool stateIs(State state) const; + class Private; + std::unique_ptr p_; friend class PipelineHandler; void disconnect(); - void requestComplete(Request *request); - std::shared_ptr pipe_; - std::string name_; - std::set streams_; - std::set activeStreams_; - - bool disconnected_; - State state_; - - /* Needed to update allocator_ and to read state_ and activeStreams_. */ friend class FrameBufferAllocator; + int exportFrameBuffers(Stream *stream, + std::vector> *buffers); + int freeFrameBuffers(Stream *stream); + /* \todo Remove allocator_ from the exposed API */ FrameBufferAllocator *allocator_; }; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 3385c08778b8..6fe802f375a3 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -254,6 +254,75 @@ std::size_t CameraConfiguration::size() const * \brief The vector of stream configurations */ +class Camera::Private +{ +public: + enum State { + CameraAvailable, + CameraAcquired, + CameraConfigured, + CameraRunning, + }; + + Private(PipelineHandler *pipe, const std::string &name, + const std::set &streams); + + bool stateBetween(State low, State high) const; + bool stateIs(State state) const; + + std::shared_ptr pipe_; + std::string name_; + std::set streams_; + std::set activeStreams_; + + bool disconnected_; + State state_; +}; + +Camera::Private::Private(PipelineHandler *pipe, const std::string &name, + const std::set &streams) + : pipe_(pipe->shared_from_this()), name_(name), streams_(streams), + disconnected_(false), state_(CameraAvailable) +{ +} + +static constexpr std::array camera_state_names = { + "Available", + "Acquired", + "Configured", + "Running", +}; + +bool Camera::Private::stateBetween(State low, State high) const +{ + if (state_ >= low && state_ <= high) + return true; + + ASSERT(static_cast(low) < camera_state_names.size() && + static_cast(high) < camera_state_names.size()); + + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + << " state trying operation requiring state between " + << camera_state_names[low] << " and " + << camera_state_names[high]; + + return false; +} + +bool Camera::Private::stateIs(State state) const +{ + if (state_ == state) + return true; + + ASSERT(static_cast(state) < camera_state_names.size()); + + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + << " state trying operation requiring state " + << camera_state_names[state]; + + return false; +} + /** * \class Camera * \brief Camera device @@ -354,8 +423,7 @@ std::shared_ptr Camera::create(PipelineHandler *pipe, } }; - Camera *camera = new Camera(pipe, name); - camera->streams_ = streams; + Camera *camera = new Camera(pipe, name, streams); return std::shared_ptr(camera, Deleter()); } @@ -366,7 +434,7 @@ std::shared_ptr Camera::create(PipelineHandler *pipe, */ const std::string &Camera::name() const { - return name_; + return p_->name_; } /** @@ -392,55 +460,18 @@ const std::string &Camera::name() const * application API calls by returning errors immediately. */ -Camera::Camera(PipelineHandler *pipe, const std::string &name) - : pipe_(pipe->shared_from_this()), name_(name), disconnected_(false), - state_(CameraAvailable), allocator_(nullptr) +Camera::Camera(PipelineHandler *pipe, const std::string &name, + const std::set &streams) + : p_(new Private(pipe, name, streams)), allocator_(nullptr) { } Camera::~Camera() { - if (!stateIs(CameraAvailable)) + if (!p_->stateIs(Private::CameraAvailable)) LOG(Camera, Error) << "Removing camera while still in use"; } -static constexpr std::array camera_state_names = { - "Available", - "Acquired", - "Configured", - "Running", -}; - -bool Camera::stateBetween(State low, State high) const -{ - if (state_ >= low && state_ <= high) - return true; - - ASSERT(static_cast(low) < camera_state_names.size() && - static_cast(high) < camera_state_names.size()); - - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] - << " state trying operation requiring state between " - << camera_state_names[low] << " and " - << camera_state_names[high]; - - return false; -} - -bool Camera::stateIs(State state) const -{ - if (state_ == state) - return true; - - ASSERT(static_cast(state) < camera_state_names.size()); - - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] - << " state trying operation requiring state " - << camera_state_names[state]; - - return false; -} - /** * \brief Notify camera disconnection * @@ -455,20 +486,45 @@ bool Camera::stateIs(State state) const */ void Camera::disconnect() { - LOG(Camera, Debug) << "Disconnecting camera " << name_; + LOG(Camera, Debug) << "Disconnecting camera " << name(); /* * If the camera was running when the hardware was removed force the * state to Configured state to allow applications to free resources * and call release() before deleting the camera. */ - if (state_ == CameraRunning) - state_ = CameraConfigured; + if (p_->state_ == Private::CameraRunning) + p_->state_ = Private::CameraConfigured; - disconnected_ = true; + p_->disconnected_ = true; disconnected.emit(this); } +int Camera::exportFrameBuffers(Stream *stream, + std::vector> *buffers) +{ + if (!p_->stateIs(Private::CameraConfigured)) + return -EACCES; + + if (streams().find(stream) == streams().end()) + return -EINVAL; + + if (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) + return -EINVAL; + + return p_->pipe_->exportFrameBuffers(this, stream, buffers); +} + +int Camera::freeFrameBuffers(Stream *stream) +{ + if (!p_->stateIs(Private::CameraConfigured)) + return -EACCES; + + p_->pipe_->freeFrameBuffers(this, stream); + + return 0; +} + /** * \brief Acquire the camera device for exclusive access * @@ -494,19 +550,19 @@ void Camera::disconnect() */ int Camera::acquire() { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraAvailable)) + if (!p_->stateIs(Private::CameraAvailable)) return -EBUSY; - if (!pipe_->lock()) { + if (!p_->pipe_->lock()) { LOG(Camera, Info) << "Pipeline handler in use by another process"; return -EBUSY; } - state_ = CameraAcquired; + p_->state_ = Private::CameraAcquired; return 0; } @@ -524,7 +580,8 @@ int Camera::acquire() */ int Camera::release() { - if (!stateBetween(CameraAvailable, CameraConfigured)) + if (!p_->stateBetween(Private::CameraAvailable, + Private::CameraConfigured)) return -EBUSY; if (allocator_) { @@ -537,9 +594,9 @@ int Camera::release() return -EBUSY; } - pipe_->unlock(); + p_->pipe_->unlock(); - state_ = CameraAvailable; + p_->state_ = Private::CameraAvailable; return 0; } @@ -553,7 +610,7 @@ int Camera::release() */ const ControlInfoMap &Camera::controls() { - return pipe_->controls(this); + return p_->pipe_->controls(this); } /** @@ -567,7 +624,7 @@ const ControlInfoMap &Camera::controls() */ const std::set &Camera::streams() const { - return streams_; + return p_->streams_; } /** @@ -586,10 +643,10 @@ const std::set &Camera::streams() const */ std::unique_ptr Camera::generateConfiguration(const StreamRoles &roles) { - if (disconnected_ || roles.size() > streams_.size()) + if (p_->disconnected_ || roles.size() > streams().size()) return nullptr; - CameraConfiguration *config = pipe_->generateConfiguration(this, roles); + CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles); if (!config) { LOG(Camera, Debug) << "Pipeline handler failed to generate configuration"; @@ -639,10 +696,11 @@ int Camera::configure(CameraConfiguration *config) { int ret; - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateBetween(CameraAcquired, CameraConfigured)) + if (!p_->stateBetween(Private::CameraAcquired, + Private::CameraConfigured)) return -EACCES; if (allocator_ && allocator_->allocated()) { @@ -667,11 +725,11 @@ int Camera::configure(CameraConfiguration *config) LOG(Camera, Info) << msg.str(); - ret = pipe_->configure(this, config); + ret = p_->pipe_->configure(this, config); if (ret) return ret; - activeStreams_.clear(); + p_->activeStreams_.clear(); for (const StreamConfiguration &cfg : *config) { Stream *stream = cfg.stream(); if (!stream) @@ -679,10 +737,10 @@ int Camera::configure(CameraConfiguration *config) << "Pipeline handler failed to update stream configuration"; stream->configuration_ = cfg; - activeStreams_.insert(stream); + p_->activeStreams_.insert(stream); } - state_ = CameraConfigured; + p_->state_ = Private::CameraConfigured; return 0; } @@ -709,7 +767,9 @@ int Camera::configure(CameraConfiguration *config) */ Request *Camera::createRequest(uint64_t cookie) { - if (disconnected_ || !stateBetween(CameraConfigured, CameraRunning)) + if (p_->disconnected_ || + !p_->stateBetween(Private::CameraConfigured, + Private::CameraRunning)) return nullptr; return new Request(this, cookie); @@ -739,10 +799,10 @@ Request *Camera::createRequest(uint64_t cookie) */ int Camera::queueRequest(Request *request) { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraRunning)) + if (!p_->stateIs(Private::CameraRunning)) return -EACCES; if (request->buffers().empty()) { @@ -753,13 +813,13 @@ int Camera::queueRequest(Request *request) for (auto const &it : request->buffers()) { Stream *stream = it.first; - if (activeStreams_.find(stream) == activeStreams_.end()) { + if (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) { LOG(Camera, Error) << "Invalid request"; return -EINVAL; } } - return pipe_->queueRequest(this, request); + return p_->pipe_->queueRequest(this, request); } /** @@ -777,26 +837,26 @@ int Camera::queueRequest(Request *request) */ int Camera::start() { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraConfigured)) + if (!p_->stateIs(Private::CameraConfigured)) return -EACCES; LOG(Camera, Debug) << "Starting capture"; - for (Stream *stream : activeStreams_) { + for (Stream *stream : p_->activeStreams_) { if (allocator_ && !allocator_->buffers(stream).empty()) continue; - pipe_->importFrameBuffers(this, stream); + p_->pipe_->importFrameBuffers(this, stream); } - int ret = pipe_->start(this); + int ret = p_->pipe_->start(this); if (ret) return ret; - state_ = CameraRunning; + p_->state_ = Private::CameraRunning; return 0; } @@ -815,23 +875,23 @@ int Camera::start() */ int Camera::stop() { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraRunning)) + if (!p_->stateIs(Private::CameraRunning)) return -EACCES; LOG(Camera, Debug) << "Stopping capture"; - state_ = CameraConfigured; + p_->state_ = Private::CameraConfigured; - pipe_->stop(this); + p_->pipe_->stop(this); - for (Stream *stream : activeStreams_) { + for (Stream *stream : p_->activeStreams_) { if (allocator_ && !allocator_->buffers(stream).empty()) continue; - pipe_->freeFrameBuffers(this, stream); + p_->pipe_->freeFrameBuffers(this, stream); } return 0; diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index a7588c7fe4c2..4f442d0d92e7 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -89,7 +89,7 @@ FrameBufferAllocator::~FrameBufferAllocator() { for (auto &value : buffers_) { Stream *stream = value.first; - camera_->pipe_->freeFrameBuffers(camera_.get(), stream); + camera_->freeFrameBuffers(stream); } buffers_.clear(); @@ -116,36 +116,17 @@ FrameBufferAllocator::~FrameBufferAllocator() */ int FrameBufferAllocator::allocate(Stream *stream) { - if (camera_->state_ != Camera::CameraConfigured) { - LOG(Allocator, Error) - << "Camera must be in the configured state to allocate buffers"; - return -EACCES; - } - - if (camera_->streams().find(stream) == camera_->streams().end()) { - LOG(Allocator, Error) - << "Stream does not belong to " << camera_->name(); - return -EINVAL; - } - - if (camera_->activeStreams_.find(stream) == camera_->activeStreams_.end()) { - LOG(Allocator, Error) - << "Stream is not part of " << camera_->name() - << " active configuration"; - return -EINVAL; - } - if (buffers_.count(stream)) { LOG(Allocator, Error) << "Buffers already allocated for stream"; return -EBUSY; } - int ret = camera_->pipe_->exportFrameBuffers(camera_.get(), stream, - &buffers_[stream]); - if (ret) - return ret; - - return 0; + int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]); + if (ret == -EINVAL) + LOG(Allocator, Error) + << "Stream is not part of " << camera_->name() + << " active configuration"; + return ret; } /** @@ -162,22 +143,16 @@ int FrameBufferAllocator::allocate(Stream *stream) */ int FrameBufferAllocator::free(Stream *stream) { - if (camera_->state_ != Camera::CameraConfigured) { - LOG(Allocator, Error) - << "Camera must be in the configured state to free buffers"; - return -EACCES; - } - auto iter = buffers_.find(stream); if (iter == buffers_.end()) return -EINVAL; - std::vector> &buffers = iter->second; + int ret = camera_->freeFrameBuffers(stream); + if (ret < 0) + return ret; + std::vector> &buffers = iter->second; buffers.clear(); - - camera_->pipe_->freeFrameBuffers(camera_.get(), stream); - buffers_.erase(iter); return 0; From patchwork Mon Jan 20 00:24:33 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2702 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C6AE607FF for ; Mon, 20 Jan 2020 01:24:48 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CEDF529 for ; Mon, 20 Jan 2020 01:24:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479887; bh=YwaW9Tnt9QaqLvr9FxIJGGpmvEuR0T7OyVyrG3GYQWk=; h=From:To:Subject:Date:In-Reply-To:References:From; b=jhhVw2Ox4EQBPbe14feXsCmu2ExeufqiiTIBenJchuB6abTsfgaE4eUx5W8oI/qUv //QS0HCAPcIPkewNqFaJieJodKGWw5eGwxWW25C++U9FT5G5Z66fR/LJOSfjtb3wxw ssadATJSSoN/GIvEuwfgzJiQCZ7XcnrSLWW5wrMQ= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:33 +0200 Message-Id: <20200120002437.6633-16-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize state checks in Private class 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:50 -0000 Move all accesses to the state_ and disconnected_ members to functions of the Private class. This will make it easier to implement synchronization, and simplifies the Camera class implementation. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/camera.cpp | 163 ++++++++++++++++------------- src/libcamera/pipeline_handler.cpp | 5 +- 2 files changed, 95 insertions(+), 73 deletions(-) diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 6fe802f375a3..83c2249b8897 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -266,15 +266,21 @@ public: Private(PipelineHandler *pipe, const std::string &name, const std::set &streams); + ~Private(); - bool stateBetween(State low, State high) const; - bool stateIs(State state) const; + int isAccessAllowed(State state, bool allowDisconnected = false) const; + int isAccessAllowed(State low, State high, + bool allowDisconnected = false) const; + + void disconnect(); + void setState(State state); std::shared_ptr pipe_; std::string name_; std::set streams_; std::set activeStreams_; +private: bool disconnected_; State state_; }; @@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name, { } +Camera::Private::~Private() +{ + if (state_ != Private::CameraAvailable) + LOG(Camera, Error) << "Removing camera while still in use"; +} + static constexpr std::array camera_state_names = { "Available", "Acquired", @@ -293,10 +305,31 @@ static constexpr std::array camera_state_names = { "Running", }; -bool Camera::Private::stateBetween(State low, State high) const +int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const { + if (!allowDisconnected && disconnected_) + return -ENODEV; + + if (state_ == state) + return 0; + + ASSERT(static_cast(state) < camera_state_names.size()); + + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + << " state trying operation requiring state " + << camera_state_names[state]; + + return -EACCES; +} + +int Camera::Private::isAccessAllowed(State low, State high, + bool allowDisconnected) const +{ + if (!allowDisconnected && disconnected_) + return -ENODEV; + if (state_ >= low && state_ <= high) - return true; + return 0; ASSERT(static_cast(low) < camera_state_names.size() && static_cast(high) < camera_state_names.size()); @@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const << camera_state_names[low] << " and " << camera_state_names[high]; - return false; + return -EACCES; } -bool Camera::Private::stateIs(State state) const +void Camera::Private::disconnect() { - if (state_ == state) - return true; - - ASSERT(static_cast(state) < camera_state_names.size()); + /* + * If the camera was running when the hardware was removed force the + * state to Configured state to allow applications to free resources + * and call release() before deleting the camera. + */ + if (state_ == Private::CameraRunning) + state_ = Private::CameraConfigured; - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] - << " state trying operation requiring state " - << camera_state_names[state]; + disconnected_ = true; +} - return false; +void Camera::Private::setState(State state) +{ + state_ = state; } /** @@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name, Camera::~Camera() { - if (!p_->stateIs(Private::CameraAvailable)) - LOG(Camera, Error) << "Removing camera while still in use"; } /** @@ -488,23 +523,16 @@ void Camera::disconnect() { LOG(Camera, Debug) << "Disconnecting camera " << name(); - /* - * If the camera was running when the hardware was removed force the - * state to Configured state to allow applications to free resources - * and call release() before deleting the camera. - */ - if (p_->state_ == Private::CameraRunning) - p_->state_ = Private::CameraConfigured; - - p_->disconnected_ = true; + p_->disconnect(); disconnected.emit(this); } int Camera::exportFrameBuffers(Stream *stream, std::vector> *buffers) { - if (!p_->stateIs(Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraConfigured); + if (ret < 0) + return ret; if (streams().find(stream) == streams().end()) return -EINVAL; @@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream, int Camera::freeFrameBuffers(Stream *stream) { - if (!p_->stateIs(Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraConfigured, true); + if (ret < 0) + return ret; p_->pipe_->freeFrameBuffers(this, stream); @@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream) */ int Camera::acquire() { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraAvailable)) - return -EBUSY; + int ret = p_->isAccessAllowed(Private::CameraAvailable); + if (ret < 0) + return ret == -EACCES ? -EBUSY : ret; if (!p_->pipe_->lock()) { LOG(Camera, Info) @@ -562,7 +589,7 @@ int Camera::acquire() return -EBUSY; } - p_->state_ = Private::CameraAcquired; + p_->setState(Private::CameraAcquired); return 0; } @@ -580,9 +607,10 @@ int Camera::acquire() */ int Camera::release() { - if (!p_->stateBetween(Private::CameraAvailable, - Private::CameraConfigured)) - return -EBUSY; + int ret = p_->isAccessAllowed(Private::CameraAvailable, + Private::CameraConfigured, true); + if (ret < 0) + return ret == -EACCES ? -EBUSY : ret; if (allocator_) { /* @@ -596,7 +624,7 @@ int Camera::release() p_->pipe_->unlock(); - p_->state_ = Private::CameraAvailable; + p_->setState(Private::CameraAvailable); return 0; } @@ -643,7 +671,12 @@ const std::set &Camera::streams() const */ std::unique_ptr Camera::generateConfiguration(const StreamRoles &roles) { - if (p_->disconnected_ || roles.size() > streams().size()) + int ret = p_->isAccessAllowed(Private::CameraAvailable, + Private::CameraRunning); + if (ret < 0) + return nullptr; + + if (roles.size() > streams().size()) return nullptr; CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles); @@ -694,14 +727,10 @@ std::unique_ptr Camera::generateConfiguration(const StreamR */ int Camera::configure(CameraConfiguration *config) { - int ret; - - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateBetween(Private::CameraAcquired, - Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraAcquired, + Private::CameraConfigured); + if (ret < 0) + return ret; if (allocator_ && allocator_->allocated()) { LOG(Camera, Error) @@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config) p_->activeStreams_.insert(stream); } - p_->state_ = Private::CameraConfigured; + p_->setState(Private::CameraConfigured); return 0; } @@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config) */ Request *Camera::createRequest(uint64_t cookie) { - if (p_->disconnected_ || - !p_->stateBetween(Private::CameraConfigured, - Private::CameraRunning)) + int ret = p_->isAccessAllowed(Private::CameraConfigured, + Private::CameraRunning); + if (ret < 0) return nullptr; return new Request(this, cookie); @@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie) */ int Camera::queueRequest(Request *request) { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraRunning)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraRunning); + if (ret < 0) + return ret; if (request->buffers().empty()) { LOG(Camera, Error) << "Request contains no buffers"; @@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request) */ int Camera::start() { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraConfigured); + if (ret < 0) + return ret; LOG(Camera, Debug) << "Starting capture"; @@ -852,11 +877,11 @@ int Camera::start() p_->pipe_->importFrameBuffers(this, stream); } - int ret = p_->pipe_->start(this); + ret = p_->pipe_->start(this); if (ret) return ret; - p_->state_ = Private::CameraRunning; + p_->setState(Private::CameraRunning); return 0; } @@ -875,15 +900,13 @@ int Camera::start() */ int Camera::stop() { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraRunning)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraRunning); + if (ret < 0) + return ret; LOG(Camera, Debug) << "Stopping capture"; - p_->state_ = Private::CameraConfigured; + p_->setState(Private::CameraConfigured); p_->pipe_->stop(this); diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 669097f609ab..3091971d5da0 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * exportFrameBuffers() and importFrameBuffers() for the streams contained in * any camera configuration. * - * The only intended caller is the FrameBufferAllocator helper. + * The only intended caller is Camera::exportFrameBuffers(). * * \return 0 on success or a negative error code otherwise */ @@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * called only after a successful call to either of these two methods, and only * once per stream. * - * The only intended callers are Camera::stop() and the FrameBufferAllocator - * helper. + * The only intended callers are Camera::stop() and Camera::freeFrameBuffers(). */ /** From patchwork Mon Jan 20 00:24:34 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2703 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 50FF06080E for ; Mon, 20 Jan 2020 01:24:48 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id EFF27504 for ; Mon, 20 Jan 2020 01:24:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479888; bh=KseSN/qBI8Px+ooqDyDbzUfVgIBJOOrxYiYiBXJXC+M=; h=From:To:Subject:Date:In-Reply-To:References:From; b=vmxkiPAs+sEJ1lIZtosnkrpHz48TvTp3gqyl0X8xjQzErtZP16nblxJ684zLyfRoS /Dpw+ILQUFJzOsSJBBzjXP34gQBoEAVtDY7mFRBq1eduMaqPP7uAk9mJkrlVmmkXTa Xua3iOdEp7pbpLurGAmiaKEs5NgSMwHXB5wVjPLo= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:34 +0200 Message-Id: <20200120002437.6633-17-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 16/19] libcamera: camera: Implement the threading model 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:50 -0000 Document the threading model of the Camera class and implement it. Selected functions become thread-safe, and require a few functions of the PipelineHandler class to be called through cross-thread invocation as the pipeline handlers live in the camera manager thread, while the Camera class is mostly accessed from the application thread. The PipelineHandler is made to inherit from the Object class to support this. Disconnection is currently left out as it is not implemented in pipeline handlers, and isn't fully supported in the Camera class either. This will be revisited when implementing proper hotplug support. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/camera.cpp | 101 +++++++++++++++++------ src/libcamera/include/pipeline_handler.h | 4 +- 2 files changed, 78 insertions(+), 27 deletions(-) diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 83c2249b8897..f281a4cd346b 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -282,7 +283,7 @@ public: private: bool disconnected_; - State state_; + std::atomic state_; }; Camera::Private::Private(PipelineHandler *pipe, const std::string &name, @@ -294,7 +295,7 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name, Camera::Private::~Private() { - if (state_ != Private::CameraAvailable) + if (state_.load(std::memory_order_acquire) != Private::CameraAvailable) LOG(Camera, Error) << "Removing camera while still in use"; } @@ -310,12 +311,13 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const if (!allowDisconnected && disconnected_) return -ENODEV; - if (state_ == state) + State currentState = state_.load(std::memory_order_acquire); + if (currentState == state) return 0; ASSERT(static_cast(state) < camera_state_names.size()); - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] << " state trying operation requiring state " << camera_state_names[state]; @@ -328,13 +330,14 @@ int Camera::Private::isAccessAllowed(State low, State high, if (!allowDisconnected && disconnected_) return -ENODEV; - if (state_ >= low && state_ <= high) + State currentState = state_.load(std::memory_order_acquire); + if (currentState >= low && currentState <= high) return 0; ASSERT(static_cast(low) < camera_state_names.size() && static_cast(high) < camera_state_names.size()); - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] << " state trying operation requiring state between " << camera_state_names[low] << " and " << camera_state_names[high]; @@ -349,15 +352,15 @@ void Camera::Private::disconnect() * state to Configured state to allow applications to free resources * and call release() before deleting the camera. */ - if (state_ == Private::CameraRunning) - state_ = Private::CameraConfigured; + if (state_.load(std::memory_order_acquire) == Private::CameraRunning) + state_.store(Private::CameraConfigured, std::memory_order_release); disconnected_ = true; } void Camera::Private::setState(State state) { - state_ = state; + state_.store(state, std::memory_order_release); } /** @@ -389,12 +392,17 @@ void Camera::Private::setState(State state) * An application may start and stop a camera multiple times as long as it is * not released. The camera may also be reconfigured. * + * Functions that affect the camera state as defined below are generally not + * synchronized with each other by the Camera class. The caller is responsible + * for ensuring their synchronization if necessary. + * * \subsection Camera States * * To help manage the sequence of operations needed to control the camera a set * of states are defined. Each state describes which operations may be performed - * on the camera. Operations not listed in the state diagram are allowed in all - * states. + * on the camera. Performing an operation not allowed in the camera state + * results in undefined behaviour. Operations not listed at all in the state + * diagram are allowed in all states. * * \dot * digraph camera_state_machine { @@ -467,6 +475,7 @@ std::shared_ptr Camera::create(PipelineHandler *pipe, /** * \brief Retrieve the name of the camera + * \context This function is \threadsafe. * \return Name of the camera device */ const std::string &Camera::name() const @@ -540,7 +549,9 @@ int Camera::exportFrameBuffers(Stream *stream, if (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) return -EINVAL; - return p_->pipe_->exportFrameBuffers(this, stream, buffers); + return p_->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers, + ConnectionTypeBlocking, this, stream, + buffers); } int Camera::freeFrameBuffers(Stream *stream) @@ -549,7 +560,8 @@ int Camera::freeFrameBuffers(Stream *stream) if (ret < 0) return ret; - p_->pipe_->freeFrameBuffers(this, stream); + p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, + ConnectionTypeBlocking, this, stream); return 0; } @@ -571,7 +583,8 @@ int Camera::freeFrameBuffers(Stream *stream) * Once exclusive access isn't needed anymore, the device should be released * with a call to the release() function. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function is \threadsafe. It may only be called when the camera + * is in the Available state as defined in \ref camera_operation. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -579,6 +592,10 @@ int Camera::freeFrameBuffers(Stream *stream) */ int Camera::acquire() { + /* + * No manual locking is required as PipelineHandler::lock() is + * thread-safe. + */ int ret = p_->isAccessAllowed(Private::CameraAvailable); if (ret < 0) return ret == -EACCES ? -EBUSY : ret; @@ -600,7 +617,10 @@ int Camera::acquire() * Releasing the camera device allows other users to acquire exclusive access * with the acquire() function. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the + * Available or Configured state as defined in \ref camera_operation, and shall + * be synchronized by the caller with other functions that affect the camera + * state. * * \return 0 on success or a negative error code otherwise * \retval -EBUSY The camera is running and can't be released @@ -634,6 +654,8 @@ int Camera::release() * * Camera controls remain constant through the lifetime of the camera. * + * \context This function is \threadsafe. + * * \return A ControlInfoMap listing the controls supported by the camera */ const ControlInfoMap &Camera::controls() @@ -648,6 +670,8 @@ const ControlInfoMap &Camera::controls() * information describes among other things how many streams the camera * supports and the capabilities of each stream. * + * \context This function is \threadsafe. + * * \return An array of all the camera's streams. */ const std::set &Camera::streams() const @@ -665,6 +689,8 @@ const std::set &Camera::streams() const * empty list of roles is valid, and will generate an empty configuration that * can be filled by the caller. * + * \context This function is \threadsafe. + * * \return A CameraConfiguration if the requested roles can be satisfied, or a * null pointer otherwise. The ownership of the returned configuration is * passed to the caller. @@ -715,7 +741,10 @@ std::unique_ptr Camera::generateConfiguration(const StreamR * Exclusive access to the camera shall be ensured by a call to acquire() prior * to calling this function, otherwise an -EACCES error will be returned. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the Acquired + * or Configured state as defined in \ref camera_operation, and shall be + * synchronized by the caller with other functions that affect the camera + * state. * * Upon return the StreamConfiguration entries in \a config are associated with * Stream instances which can be retrieved with StreamConfiguration::stream(). @@ -754,7 +783,8 @@ int Camera::configure(CameraConfiguration *config) LOG(Camera, Info) << msg.str(); - ret = p_->pipe_->configure(this, config); + ret = p_->pipe_->invokeMethod(&PipelineHandler::configure, + ConnectionTypeBlocking, this, config); if (ret) return ret; @@ -789,8 +819,8 @@ int Camera::configure(CameraConfiguration *config) * The ownership of the returned request is passed to the caller, which is * responsible for either queueing the request or deleting it. * - * This function shall only be called when the camera is in the Configured - * or Running state, see \ref camera_operation. + * \context This function is \threadsafe. It may only be called when the camera + * is in the Configured or Running state as defined in \ref camera_operation. * * \return A pointer to the newly created request, or nullptr on error */ @@ -820,6 +850,9 @@ Request *Camera::createRequest(uint64_t cookie) * Ownership of the request is transferred to the camera. It will be deleted * automatically after it completes. * + * \context This function is \threadsafe. It may only be called when the camera + * is in the Running state as defined in \ref camera_operation. + * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system * \retval -EACCES The camera is not running so requests can't be queued @@ -832,6 +865,12 @@ int Camera::queueRequest(Request *request) if (ret < 0) return ret; + /* + * The camera state may chance until the end of the function. No locking + * is however needed as PipelineHandler::queueRequest() will handle + * this. + */ + if (request->buffers().empty()) { LOG(Camera, Error) << "Request contains no buffers"; return -EINVAL; @@ -846,7 +885,8 @@ int Camera::queueRequest(Request *request) } } - return p_->pipe_->queueRequest(this, request); + return p_->pipe_->invokeMethod(&PipelineHandler::queueRequest, + ConnectionTypeQueued, this, request); } /** @@ -856,7 +896,10 @@ int Camera::queueRequest(Request *request) * can queue requests to the camera to process and return to the application * until the capture session is terminated with \a stop(). * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the + * Configured state as defined in \ref camera_operation, and shall be + * synchronized by the caller with other functions that affect the camera + * state. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -874,10 +917,12 @@ int Camera::start() if (allocator_ && !allocator_->buffers(stream).empty()) continue; - p_->pipe_->importFrameBuffers(this, stream); + p_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers, + ConnectionTypeDirect, this, stream); } - ret = p_->pipe_->start(this); + ret = p_->pipe_->invokeMethod(&PipelineHandler::start, + ConnectionTypeBlocking, this); if (ret) return ret; @@ -892,7 +937,9 @@ int Camera::start() * This method stops capturing and processing requests immediately. All pending * requests are cancelled and complete synchronously in an error state. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the Running + * state as defined in \ref camera_operation, and shall be synchronized by the + * caller with other functions that affect the camera state. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -908,13 +955,15 @@ int Camera::stop() p_->setState(Private::CameraConfigured); - p_->pipe_->stop(this); + p_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, + this); for (Stream *stream : p_->activeStreams_) { if (allocator_ && !allocator_->buffers(stream).empty()) continue; - p_->pipe_->freeFrameBuffers(this, stream); + p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, + ConnectionTypeBlocking, this, stream); } return 0; diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index a6c1e1fbae38..db0ec6ac00d7 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -17,6 +17,7 @@ #include #include +#include #include namespace libcamera { @@ -51,7 +52,8 @@ private: CameraData &operator=(const CameraData &) = delete; }; -class PipelineHandler : public std::enable_shared_from_this +class PipelineHandler : public std::enable_shared_from_this, + public Object { public: PipelineHandler(CameraManager *manager); From patchwork Mon Jan 20 00:24:35 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2704 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id E569E6081D for ; Mon, 20 Jan 2020 01:24:48 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 4997D529 for ; Mon, 20 Jan 2020 01:24:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479888; bh=7EeTbDavZAtKxKnTSfJe6Y8a0TcoEmxVK0F1j7tjqIY=; h=From:To:Subject:Date:In-Reply-To:References:From; b=dMYF8IkVXxdI2eeYfLATYnhHYDSLCMtpu9fMhKDcvtfuLLXES8LAaJzQyWAUqhlDE 19GTW7lMQAn0hghYR9F66+BaoDYO5AYbKmkBm3hods+cdt81yktqlJ0i8t2b1o1F6W /g9bo5tBUAcnTZxKaEBpWTz6uog6w7rpwOzpCTao= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:35 +0200 Message-Id: <20200120002437.6633-18-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 17/19] libcamera: pipeline_handler: Implement the threading model 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:50 -0000 Document the threading model of the PipelineHandler class (and all its derived classes). The model is already enforced by the Camera class, so not much is required. As for the Camera class, disconnection is currently left out. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/pipeline_handler.cpp | 36 ++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 3091971d5da0..2ebf73b76e6b 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -166,6 +166,8 @@ PipelineHandler::~PipelineHandler() * If this function returns true, a new instance of the pipeline handler will * be created and its match() function called. * + * \context This function is called from the CameraManager thread. + * * \return true if media devices have been acquired and camera instances * created, or false otherwise */ @@ -182,6 +184,8 @@ PipelineHandler::~PipelineHandler() * device explicitly, it will be automatically released when the pipeline * handler is destroyed. * + * \context This function shall be called from the CameraManager thread. + * * \return A pointer to the matching MediaDevice, or nullptr if no match is found */ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, @@ -205,6 +209,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, * This method shall not be called from pipeline handler implementation, as the * Camera class handles locking directly. * + * \context This function is \threadsafe. + * * \return True if the devices could be locked, false otherwise * \sa unlock() * \sa MediaDevice::lock() @@ -227,6 +233,8 @@ bool PipelineHandler::lock() * This method shall not be called from pipeline handler implementation, as the * Camera class handles locking directly. * + * \context This function is \threadsafe. + * * \sa lock() */ void PipelineHandler::unlock() @@ -238,6 +246,7 @@ void PipelineHandler::unlock() /** * \brief Retrieve the list of controls for a camera * \param[in] camera The camera + * \context This function is \threadsafe. * \return A ControlInfoMap listing the controls support by \a camera */ const ControlInfoMap &PipelineHandler::controls(Camera *camera) @@ -261,6 +270,10 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * The intended companion to this is \a configure() which can be used to change * the group of streams parameters. * + * \context This function may be called from any thread and shall be + * \threadsafe. It shall not modify the state of the \a camera in the pipeline + * handler. + * * \return A valid CameraConfiguration if the requested roles can be satisfied, * or a null pointer otherwise. The ownership of the returned configuration is * passed to the caller. @@ -286,6 +299,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * instance to each StreamConfiguration entry in the CameraConfiguration using * the StreamConfiguration::setStream() method. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -316,6 +331,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * * The only intended caller is Camera::exportFrameBuffers(). * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -343,6 +360,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * * The only intended caller is Camera::start(). * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -358,6 +377,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * once per stream. * * The only intended callers are Camera::stop() and Camera::freeFrameBuffers(). + * + * \context This function is called from the CameraManager thread. */ /** @@ -370,6 +391,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * will in turn be called from the application to indicate that it has * configured the streams and is ready to capture. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -380,6 +403,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * * This method stops capturing and processing requests immediately. All pending * requests are cancelled and complete immediately in an error state. + * + * \context This function is called from the CameraManager thread. */ /** @@ -396,6 +421,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * when the pipeline handler is stopped with stop(). Request completion shall be * signalled by the pipeline handler using the completeRequest() method. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ int PipelineHandler::queueRequest(Camera *camera, Request *request) @@ -422,6 +449,8 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request) * parameters will be applied to the frames captured in the buffers provided in * the request. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -438,6 +467,8 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request) * pipeline handlers a chance to perform any operation that may still be * needed. They shall complete requests explicitly with completeRequest(). * + * \context This function shall be called from the CameraManager thread. + * * \return True if all buffers contained in the request have completed, false * otherwise */ @@ -460,6 +491,8 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request, * This method ensures that requests will be returned to the application in * submission order, the pipeline handler may call it on any complete request * without any ordering constraint. + * + * \context This function shall be called from the CameraManager thread. */ void PipelineHandler::completeRequest(Camera *camera, Request *request) { @@ -493,6 +526,8 @@ void PipelineHandler::completeRequest(Camera *camera, Request *request) * is to be associated with. This is for the V4L2 compatibility layer to map * device nodes to Camera instances based on the device number * registered by this method in \a devnum. + * + * \context This function shall be called from the CameraManager thread. */ void PipelineHandler::registerCamera(std::shared_ptr camera, std::unique_ptr data, @@ -586,6 +621,7 @@ CameraData *PipelineHandler::cameraData(const Camera *camera) /** * \fn PipelineHandler::name() * \brief Retrieve the pipeline handler name + * \context This function shall be \threadsafe. * \return The pipeline handler name */ From patchwork Mon Jan 20 00:24:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2705 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B34660808 for ; Mon, 20 Jan 2020 01:24:49 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 9ADD5563 for ; Mon, 20 Jan 2020 01:24:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479888; bh=pWndmu897QEt+JmjUPmC3KVu5gu85bjQmoA5v2SMfEc=; h=From:To:Subject:Date:In-Reply-To:References:From; b=dXv/NXzwQ+qQzrbjvFJx4+O+61IkduKSajipni3N0RGyHqcJKddFbuqu7KCHnyV51 VZ60ML8CXGcTQUQ2uFcs9zjJWpWhHgssKSfRIWgwXhC5FH8annUb7bG9fF6e6hnT/N drLpiATcQu4wE1tOVZVhwhJYxBBKHBGAzoYo4xKE= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:36 +0200 Message-Id: <20200120002437.6633-19-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 18/19] v4l2: Remove internal thread 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:51 -0000 Now that libcamera creates threads internally and doesn't rely on an application-provided event loop, remove the thread from the V4L2 compatibility layer. The split between the V4L2CameraProxy and V4L2Camera classes is still kept to separate the V4L2 adaptation from camera operation. This may be further refactored later. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/v4l2/v4l2_camera.h | 2 +- src/v4l2/v4l2_camera_proxy.cpp | 43 ++++++++++------------------ src/v4l2/v4l2_compat_manager.cpp | 48 +++++++++----------------------- src/v4l2/v4l2_compat_manager.h | 13 ++------- 4 files changed, 31 insertions(+), 75 deletions(-) diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index f1f04d9ef6ed..37bd358462db 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -21,7 +21,7 @@ using namespace libcamera; -class V4L2Camera : public Object +class V4L2Camera { public: struct Buffer { diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index e58fd6a0d8b5..622520479be0 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -41,8 +41,7 @@ int V4L2CameraProxy::open(bool nonBlocking) { LOG(V4L2Compat, Debug) << "Servicing open"; - int ret = vcam_->invokeMethod(&V4L2Camera::open, - ConnectionTypeBlocking); + int ret = vcam_->open(); if (ret < 0) { errno = -ret; return -1; @@ -50,8 +49,7 @@ int V4L2CameraProxy::open(bool nonBlocking) nonBlocking_ = nonBlocking; - vcam_->invokeMethod(&V4L2Camera::getStreamConfig, - ConnectionTypeBlocking, &streamConfig_); + vcam_->getStreamConfig(&streamConfig_); setFmtFromConfig(streamConfig_); sizeimage_ = calculateSizeImage(streamConfig_); @@ -72,7 +70,7 @@ void V4L2CameraProxy::close() if (--refcount_ > 0) return; - vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking); + vcam_->close(); } void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags, @@ -284,11 +282,9 @@ int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg) tryFormat(arg); Size size(arg->fmt.pix.width, arg->fmt.pix.height); - int ret = vcam_->invokeMethod(&V4L2Camera::configure, - ConnectionTypeBlocking, - &streamConfig_, size, - v4l2ToDrm(arg->fmt.pix.pixelformat), - bufferCount_); + int ret = vcam_->configure(&streamConfig_, size, + v4l2ToDrm(arg->fmt.pix.pixelformat), + bufferCount_); if (ret < 0) return -EINVAL; @@ -319,13 +315,12 @@ int V4L2CameraProxy::freeBuffers() { LOG(V4L2Compat, Debug) << "Freeing libcamera bufs"; - int ret = vcam_->invokeMethod(&V4L2Camera::streamOff, - ConnectionTypeBlocking); + int ret = vcam_->streamOff(); if (ret < 0) { LOG(V4L2Compat, Error) << "Failed to stop stream"; return ret; } - vcam_->invokeMethod(&V4L2Camera::freeBuffers, ConnectionTypeBlocking); + vcam_->freeBuffers(); bufferCount_ = 0; return 0; @@ -349,11 +344,9 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg) return freeBuffers(); Size size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height); - ret = vcam_->invokeMethod(&V4L2Camera::configure, - ConnectionTypeBlocking, - &streamConfig_, size, - v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat), - arg->count); + ret = vcam_->configure(&streamConfig_, size, + v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat), + arg->count); if (ret < 0) return -EINVAL; @@ -366,8 +359,7 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg) arg->count = streamConfig_.bufferCount; bufferCount_ = arg->count; - ret = vcam_->invokeMethod(&V4L2Camera::allocBuffers, - ConnectionTypeBlocking, arg->count); + ret = vcam_->allocBuffers(arg->count); if (ret < 0) { arg->count = 0; return ret; @@ -415,8 +407,7 @@ int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg) arg->index >= bufferCount_) return -EINVAL; - int ret = vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking, - arg->index); + int ret = vcam_->qbuf(arg->index); if (ret < 0) return ret; @@ -459,10 +450,7 @@ int V4L2CameraProxy::vidioc_streamon(int *arg) if (!validateBufferType(*arg)) return -EINVAL; - int ret = vcam_->invokeMethod(&V4L2Camera::streamOn, - ConnectionTypeBlocking); - - return ret; + return vcam_->streamOn(); } int V4L2CameraProxy::vidioc_streamoff(int *arg) @@ -472,8 +460,7 @@ int V4L2CameraProxy::vidioc_streamoff(int *arg) if (!validateBufferType(*arg)) return -EINVAL; - int ret = vcam_->invokeMethod(&V4L2Camera::streamOff, - ConnectionTypeBlocking); + int ret = vcam_->streamOff(); for (struct v4l2_buffer &buf : buffers_) buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE); diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index f5a7b2ac4229..961d06b3e39a 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -37,7 +37,7 @@ void get_symbol(T &func, const char *name) } /* namespace */ V4L2CompatManager::V4L2CompatManager() - : cm_(nullptr), initialized_(false) + : cm_(nullptr) { get_symbol(fops_.openat, "openat"); get_symbol(fops_.dup, "dup"); @@ -52,24 +52,15 @@ V4L2CompatManager::~V4L2CompatManager() devices_.clear(); mmaps_.clear(); - if (isRunning()) { - exit(0); - /* \todo Wait with a timeout, just in case. */ - wait(); + if (cm_) { + proxies_.clear(); + cm_->stop(); + delete cm_; + cm_ = nullptr; } } -int V4L2CompatManager::init() -{ - start(); - - MutexLocker locker(mutex_); - cv_.wait(locker, [&] { return initialized_; }); - - return 0; -} - -void V4L2CompatManager::run() +int V4L2CompatManager::start() { cm_ = new CameraManager(); @@ -77,7 +68,9 @@ void V4L2CompatManager::run() if (ret) { LOG(V4L2Compat, Error) << "Failed to start camera manager: " << strerror(-ret); - return; + delete cm_; + cm_ = nullptr; + return ret; } LOG(V4L2Compat, Debug) << "Started camera manager"; @@ -93,22 +86,7 @@ void V4L2CompatManager::run() ++index; } - /* - * libcamera has been initialized. Unlock the init() caller as we're - * now ready to handle calls from the framework. - */ - mutex_.lock(); - initialized_ = true; - mutex_.unlock(); - cv_.notify_one(); - - /* Now start processing events and messages. */ - exec(); - - proxies_.clear(); - cm_->stop(); - delete cm_; - cm_ = nullptr; + return 0; } V4L2CompatManager *V4L2CompatManager::instance() @@ -159,8 +137,8 @@ int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mod major(statbuf.st_rdev) != 81) return fd; - if (!isRunning()) - init(); + if (!cm_) + start(); ret = getCameraIndex(fd); if (ret < 0) { diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h index d51b5953d930..872c7c3b10e8 100644 --- a/src/v4l2/v4l2_compat_manager.h +++ b/src/v4l2/v4l2_compat_manager.h @@ -8,22 +8,19 @@ #ifndef __V4L2_COMPAT_MANAGER_H__ #define __V4L2_COMPAT_MANAGER_H__ -#include #include #include #include -#include #include #include #include -#include "thread.h" #include "v4l2_camera_proxy.h" using namespace libcamera; -class V4L2CompatManager : public Thread +class V4L2CompatManager { public: struct FileOperations { @@ -46,8 +43,6 @@ public: static V4L2CompatManager *instance(); - int init(); - V4L2CameraProxy *getProxy(int fd); const FileOperations &fops() const { return fops_; } @@ -64,17 +59,13 @@ private: V4L2CompatManager(); ~V4L2CompatManager(); - void run() override; + int start(); int getCameraIndex(int fd); FileOperations fops_; CameraManager *cm_; - std::mutex mutex_; - std::condition_variable cv_; - bool initialized_; - std::vector> proxies_; std::map devices_; std::map mmaps_; From patchwork Mon Jan 20 00:24:37 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2706 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 30D55607FD for ; Mon, 20 Jan 2020 01:24:50 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8EBFEB57 for ; Mon, 20 Jan 2020 01:24:49 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579479889; bh=02Lg/Q2FV3R3R1AOSNuNmtefAEGx4cnoSYiW8rLSuSg=; h=From:To:Subject:Date:In-Reply-To:References:From; b=kz3IP2qSMN6KZw8OHUd7EHQPCasDENOtg5UfsG4Ouq3soMHxTuGIqzAOT0SmWqPFL NFyC4hMmjbqarMI0OZvhT6lcKVQ0D64tlBTinfCnQXJrHpStJPYFZuZI1vai7KERIV F5fui5brubhrFw76WGZFRnHopNKaeoA+VHtjSQSA= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 20 Jan 2020 02:24:37 +0200 Message-Id: <20200120002437.6633-20-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> References: <20200120002437.6633-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 19/19] android: Remove internal thread 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: , X-List-Received-Date: Mon, 20 Jan 2020 00:24:51 -0000 Now that libcamera creates threads internally and doesn't rely on an application-provided event loop, remove the thread from the Android Camera HAL layer. The CameraProxy class becomes meaningless, remove it and communicate directly from the CameraHalManager to the CameraDevice. Signed-off-by: Laurent Pinchart --- src/android/camera3_hal.cpp | 8 +- src/android/camera_device.cpp | 48 +++++--- src/android/camera_device.h | 17 ++- src/android/camera_hal_manager.cpp | 78 +++++-------- src/android/camera_hal_manager.h | 16 +-- src/android/camera_ops.cpp | 96 +++++++++++++++ src/android/camera_ops.h | 15 +++ src/android/camera_proxy.cpp | 180 ----------------------------- src/android/camera_proxy.h | 42 ------- src/android/meson.build | 2 +- 10 files changed, 188 insertions(+), 314 deletions(-) create mode 100644 src/android/camera_ops.cpp create mode 100644 src/android/camera_ops.h delete mode 100644 src/android/camera_proxy.cpp delete mode 100644 src/android/camera_proxy.h diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp index 8d2629ca356c..d6fc1ecc01bf 100644 --- a/src/android/camera3_hal.cpp +++ b/src/android/camera3_hal.cpp @@ -7,8 +7,8 @@ #include +#include "camera_device.h" #include "camera_hal_manager.h" -#include "camera_proxy.h" #include "log.h" using namespace libcamera; @@ -71,14 +71,14 @@ static int hal_dev_open(const hw_module_t *module, const char *name, LOG(HAL, Debug) << "Open camera " << name; int id = atoi(name); - CameraProxy *proxy = cameraManager.open(id, module); - if (!proxy) { + CameraDevice *camera = cameraManager.open(id, module); + if (!camera) { LOG(HAL, Error) << "Failed to open camera module '" << id << "'"; return -ENODEV; } - *device = &proxy->camera3Device()->common; + *device = &camera->camera3Device()->common; return 0; } diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 67c1d47e67ed..b460d499375b 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -6,6 +6,7 @@ */ #include "camera_device.h" +#include "camera_ops.h" #include "log.h" #include "utils.h" @@ -39,13 +40,13 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() * \class CameraDevice * * The CameraDevice class wraps a libcamera::Camera instance, and implements - * the camera_device_t interface by handling RPC requests received from its - * associated CameraProxy. + * the camera3_device_t interface, bridging calls received from the Android + * camera service to the CameraDevice. * - * It translate parameters and operations from Camera HALv3 API to the libcamera - * ones to provide static information for a Camera, create request templates - * for it, process capture requests and then deliver capture results back - * to the framework using the designated callbacks. + * The class translates parameters and operations from the Camera HALv3 API to + * the libcamera API to provide static information for a Camera, create request + * templates for it, process capture requests and then deliver capture results + * back to the framework using the designated callbacks. */ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera) @@ -63,7 +64,7 @@ CameraDevice::~CameraDevice() delete it.second; } -int CameraDevice::open() +int CameraDevice::open(const hw_module_t *hardwareModule) { int ret = camera_->acquire(); if (ret) { @@ -71,6 +72,19 @@ int CameraDevice::open() return ret; } + /* Initialize the hw_device_t in the instance camera3_module_t. */ + camera3Device_.common.tag = HARDWARE_DEVICE_TAG; + camera3Device_.common.version = CAMERA_DEVICE_API_VERSION_3_3; + camera3Device_.common.module = (hw_module_t *)hardwareModule; + camera3Device_.common.close = hal_dev_close; + + /* + * The camera device operations. These actually implement + * the Android Camera HALv3 interface. + */ + camera3Device_.ops = &hal_dev_ops; + camera3Device_.priv = this; + return 0; } @@ -90,7 +104,7 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks) /* * Return static information for the camera. */ -camera_metadata_t *CameraDevice::getStaticMetadata() +const camera_metadata_t *CameraDevice::getStaticMetadata() { if (staticMetadata_) return staticMetadata_->get(); @@ -675,7 +689,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) return 0; } -void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) { StreamConfiguration *streamConfiguration = &config_->at(0); Stream *stream = streamConfiguration->stream(); @@ -683,7 +697,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque if (camera3Request->num_output_buffers != 1) { LOG(HAL, Error) << "Invalid number of output buffers: " << camera3Request->num_output_buffers; - return; + return -EINVAL; } /* Start the camera if that's the first request we handle. */ @@ -691,7 +705,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque int ret = camera_->start(); if (ret) { LOG(HAL, Error) << "Failed to start camera"; - return; + return ret; } running_ = true; @@ -747,7 +761,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque if (!buffer) { LOG(HAL, Error) << "Failed to create buffer"; delete descriptor; - return; + return -ENOMEM; } Request *request = @@ -757,14 +771,12 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque int ret = camera_->queueRequest(request); if (ret) { LOG(HAL, Error) << "Failed to queue request"; - goto error; + delete request; + delete descriptor; + return ret; } - return; - -error: - delete request; - delete descriptor; + return 0; } void CameraDevice::requestComplete(Request *request) diff --git a/src/android/camera_device.h b/src/android/camera_device.h index caa617dcbfe1..8c2d9d971cc8 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -13,7 +13,6 @@ #include #include -#include #include #include @@ -21,19 +20,23 @@ class CameraMetadata; -class CameraDevice : public libcamera::Object +class CameraDevice { public: CameraDevice(unsigned int id, const std::shared_ptr &camera); ~CameraDevice(); - int open(); + int open(const hw_module_t *hardwareModule); void close(); + + unsigned int id() const { return id_; } + camera3_device_t *camera3Device() { return &camera3Device_; } + void setCallbacks(const camera3_callback_ops_t *callbacks); - camera_metadata_t *getStaticMetadata(); + const camera_metadata_t *getStaticMetadata(); const camera_metadata_t *constructDefaultRequestSettings(int type); int configureStreams(camera3_stream_configuration_t *stream_list); - void processCaptureRequest(camera3_capture_request_t *request); + int processCaptureRequest(camera3_capture_request_t *request); void requestComplete(libcamera::Request *request); private: @@ -52,6 +55,10 @@ private: std::unique_ptr getResultMetadata(int frame_number, int64_t timestamp); + unsigned int id_; + CameraDevice *cameraDevice_; + camera3_device_t camera3Device_; + bool running_; std::shared_ptr camera_; std::unique_ptr config_; diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index 22f0323b3ff0..5bd3bdba8a55 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -12,7 +12,6 @@ #include "log.h" #include "camera_device.h" -#include "camera_proxy.h" using namespace libcamera; @@ -28,92 +27,67 @@ LOG_DECLARE_CATEGORY(HAL); * their static information and to open camera devices. */ -CameraHalManager::~CameraHalManager() +CameraHalManager::CameraHalManager() + : cameraManager_(nullptr) { - if (isRunning()) { - exit(0); - /* \todo Wait with a timeout, just in case. */ - wait(); - } } -int CameraHalManager::init() +CameraHalManager::~CameraHalManager() { - /* - * Start the camera HAL manager thread and wait until its - * initialisation completes to be fully operational before - * receiving calls from the camera stack. - */ - start(); - - MutexLocker locker(mutex_); - cv_.wait(locker); + cameras_.clear(); - return 0; + if (cameraManager_) { + cameraManager_->stop(); + delete cameraManager_; + cameraManager_ = nullptr; + } } -void CameraHalManager::run() +int CameraHalManager::init() { - /* - * All the libcamera components must be initialised here, in - * order to bind them to the camera HAL manager thread that - * executes the event dispatcher. - */ cameraManager_ = new CameraManager(); int ret = cameraManager_->start(); if (ret) { LOG(HAL, Error) << "Failed to start camera manager: " << strerror(-ret); - return; + delete cameraManager_; + cameraManager_ = nullptr; + return ret; } /* - * For each Camera registered in the system, a CameraProxy - * gets created here to wraps a camera device. + * For each Camera registered in the system, a CameraDevice + * gets created here to wraps a libcamera Camera instance. * * \todo Support camera hotplug. */ unsigned int index = 0; - for (auto &camera : cameraManager_->cameras()) { - CameraProxy *proxy = new CameraProxy(index, camera); - proxies_.emplace_back(proxy); + for (auto &cam : cameraManager_->cameras()) { + CameraDevice *camera = new CameraDevice(index, cam); + cameras_.emplace_back(camera); ++index; } - /* - * libcamera has been initialized. Unlock the init() caller - * as we're now ready to handle calls from the framework. - */ - cv_.notify_one(); - - /* Now start processing events and messages. */ - exec(); - - /* Clean up the resources we have allocated. */ - proxies_.clear(); - - cameraManager_->stop(); - delete cameraManager_; - cameraManager_ = nullptr; + return 0; } -CameraProxy *CameraHalManager::open(unsigned int id, - const hw_module_t *hardwareModule) +CameraDevice *CameraHalManager::open(unsigned int id, + const hw_module_t *hardwareModule) { if (id >= numCameras()) { LOG(HAL, Error) << "Invalid camera id '" << id << "'"; return nullptr; } - CameraProxy *proxy = proxies_[id].get(); - if (proxy->open(hardwareModule)) + CameraDevice *camera = cameras_[id].get(); + if (camera->open(hardwareModule)) return nullptr; LOG(HAL, Info) << "Open camera '" << id << "'"; - return proxy; + return camera; } unsigned int CameraHalManager::numCameras() const @@ -131,14 +105,14 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info) return -EINVAL; } - CameraProxy *proxy = proxies_[id].get(); + CameraDevice *camera = cameras_[id].get(); /* \todo Get these info dynamically inspecting the camera module. */ info->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK; info->orientation = 0; info->device_version = 0; info->resource_cost = 0; - info->static_camera_characteristics = proxy->getStaticMetadata(); + info->static_camera_characteristics = camera->getStaticMetadata(); info->conflicting_devices = nullptr; info->conflicting_devices_length = 0; diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h index c23abd1c00af..7e48516286a1 100644 --- a/src/android/camera_hal_manager.h +++ b/src/android/camera_hal_manager.h @@ -7,8 +7,6 @@ #ifndef __ANDROID_CAMERA_MANAGER_H__ #define __ANDROID_CAMERA_MANAGER_H__ -#include -#include #include #include @@ -16,33 +14,27 @@ #include -#include "thread.h" - class CameraDevice; -class CameraProxy; -class CameraHalManager : public libcamera::Thread +class CameraHalManager { public: + CameraHalManager(); ~CameraHalManager(); int init(); - CameraProxy *open(unsigned int id, const hw_module_t *module); + CameraDevice *open(unsigned int id, const hw_module_t *module); unsigned int numCameras() const; int getCameraInfo(unsigned int id, struct camera_info *info); private: - void run() override; camera_metadata_t *getStaticMetadata(unsigned int id); libcamera::CameraManager *cameraManager_; - std::mutex mutex_; - std::condition_variable cv_; - - std::vector> proxies_; + std::vector> cameras_; }; #endif /* __ANDROID_CAMERA_MANAGER_H__ */ diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp new file mode 100644 index 000000000000..9dfc2e655e83 --- /dev/null +++ b/src/android/camera_ops.cpp @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * camera_ops.h - Android Camera HAL Operations + */ + +#include "camera_ops.h" + +#include + +#include "camera_device.h" + +using namespace libcamera; + +/* + * Translatation layer between the Android Camera HAL device operations and the + * CameraDevice. + */ + +static int hal_dev_initialize(const struct camera3_device *dev, + const camera3_callback_ops_t *callback_ops) +{ + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast(dev->priv); + camera->setCallbacks(callback_ops); + + return 0; +} + +static int hal_dev_configure_streams(const struct camera3_device *dev, + camera3_stream_configuration_t *stream_list) +{ + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast(dev->priv); + return camera->configureStreams(stream_list); +} + +static const camera_metadata_t * +hal_dev_construct_default_request_settings(const struct camera3_device *dev, + int type) +{ + if (!dev) + return nullptr; + + CameraDevice *camera = reinterpret_cast(dev->priv); + return camera->constructDefaultRequestSettings(type); +} + +static int hal_dev_process_capture_request(const struct camera3_device *dev, + camera3_capture_request_t *request) +{ + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast(dev->priv); + return camera->processCaptureRequest(request); +} + +static void hal_dev_dump(const struct camera3_device *dev, int fd) +{ +} + +static int hal_dev_flush(const struct camera3_device *dev) +{ + return 0; +} + +int hal_dev_close(hw_device_t *hw_device) +{ + if (!hw_device) + return -EINVAL; + + camera3_device_t *dev = reinterpret_cast(hw_device); + CameraDevice *camera = reinterpret_cast(dev->priv); + + camera->close(); + + return 0; +} + +camera3_device_ops hal_dev_ops = { + .initialize = hal_dev_initialize, + .configure_streams = hal_dev_configure_streams, + .register_stream_buffers = nullptr, + .construct_default_request_settings = hal_dev_construct_default_request_settings, + .process_capture_request = hal_dev_process_capture_request, + .get_metadata_vendor_tag_ops = nullptr, + .dump = hal_dev_dump, + .flush = hal_dev_flush, + .reserved = { nullptr }, +}; diff --git a/src/android/camera_ops.h b/src/android/camera_ops.h new file mode 100644 index 000000000000..304e7b856e81 --- /dev/null +++ b/src/android/camera_ops.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * camera_ops.h - Android Camera HAL Operations + */ +#ifndef __ANDROID_CAMERA_OPS_H__ +#define __ANDROID_CAMERA_OPS_H__ + +#include + +int hal_dev_close(hw_device_t *hw_device); +extern camera3_device_ops hal_dev_ops; + +#endif /* __ANDROID_CAMERA_OPS_H__ */ diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp deleted file mode 100644 index 3964b5665e2f..000000000000 --- a/src/android/camera_proxy.cpp +++ /dev/null @@ -1,180 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Copyright (C) 2019, Google Inc. - * - * camera_proxy.cpp - Proxy to camera devices - */ - -#include "camera_proxy.h" - -#include - -#include - -#include "log.h" -#include "message.h" -#include "utils.h" - -#include "camera_device.h" - -using namespace libcamera; - -LOG_DECLARE_CATEGORY(HAL); - -/* - * \class CameraProxy - * - * The CameraProxy wraps a CameraDevice and implements the camera3_device_t - * API, bridging calls received from the camera framework to the CameraDevice. - * - * Bridging operation calls between the framework and the CameraDevice is - * required as the two run in two different threads and certain operations, - * such as queueing a new capture request to the camera, shall be called in - * the thread that dispatches events. Other operations do not require any - * bridging and resolve to direct function calls on the CameraDevice instance - * instead. - */ - -static int hal_dev_initialize(const struct camera3_device *dev, - const camera3_callback_ops_t *callback_ops) -{ - if (!dev) - return -EINVAL; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - proxy->initialize(callback_ops); - - return 0; -} - -static int hal_dev_configure_streams(const struct camera3_device *dev, - camera3_stream_configuration_t *stream_list) -{ - if (!dev) - return -EINVAL; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - return proxy->configureStreams(stream_list); -} - -static const camera_metadata_t * -hal_dev_construct_default_request_settings(const struct camera3_device *dev, - int type) -{ - if (!dev) - return nullptr; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - return proxy->constructDefaultRequestSettings(type); -} - -static int hal_dev_process_capture_request(const struct camera3_device *dev, - camera3_capture_request_t *request) -{ - if (!dev) - return -EINVAL; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - return proxy->processCaptureRequest(request); -} - -static void hal_dev_dump(const struct camera3_device *dev, int fd) -{ -} - -static int hal_dev_flush(const struct camera3_device *dev) -{ - return 0; -} - -static int hal_dev_close(hw_device_t *hw_device) -{ - if (!hw_device) - return -EINVAL; - - camera3_device_t *dev = reinterpret_cast(hw_device); - CameraProxy *proxy = reinterpret_cast(dev->priv); - - proxy->close(); - - return 0; -} - -static camera3_device_ops hal_dev_ops = { - .initialize = hal_dev_initialize, - .configure_streams = hal_dev_configure_streams, - .register_stream_buffers = nullptr, - .construct_default_request_settings = hal_dev_construct_default_request_settings, - .process_capture_request = hal_dev_process_capture_request, - .get_metadata_vendor_tag_ops = nullptr, - .dump = hal_dev_dump, - .flush = hal_dev_flush, - .reserved = { nullptr }, -}; - -CameraProxy::CameraProxy(unsigned int id, const std::shared_ptr &camera) - : id_(id) -{ - cameraDevice_ = new CameraDevice(id, camera); -} - -CameraProxy::~CameraProxy() -{ - delete cameraDevice_; -} - -int CameraProxy::open(const hw_module_t *hardwareModule) -{ - int ret = cameraDevice_->open(); - if (ret) - return ret; - - /* Initialize the hw_device_t in the instance camera3_module_t. */ - camera3Device_.common.tag = HARDWARE_DEVICE_TAG; - camera3Device_.common.version = CAMERA_DEVICE_API_VERSION_3_3; - camera3Device_.common.module = (hw_module_t *)hardwareModule; - camera3Device_.common.close = hal_dev_close; - - /* - * The camera device operations. These actually implement - * the Android Camera HALv3 interface. - */ - camera3Device_.ops = &hal_dev_ops; - camera3Device_.priv = this; - - return 0; -} - -void CameraProxy::close() -{ - cameraDevice_->invokeMethod(&CameraDevice::close, - ConnectionTypeBlocking); -} - -void CameraProxy::initialize(const camera3_callback_ops_t *callbacks) -{ - cameraDevice_->setCallbacks(callbacks); -} - -const camera_metadata_t *CameraProxy::getStaticMetadata() -{ - return cameraDevice_->getStaticMetadata(); -} - -const camera_metadata_t *CameraProxy::constructDefaultRequestSettings(int type) -{ - return cameraDevice_->constructDefaultRequestSettings(type); -} - -int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list) -{ - return cameraDevice_->configureStreams(stream_list); -} - -int CameraProxy::processCaptureRequest(camera3_capture_request_t *request) -{ - cameraDevice_->invokeMethod(&CameraDevice::processCaptureRequest, - ConnectionTypeBlocking, request); - - return 0; -} diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h deleted file mode 100644 index e8cfbc9dd526..000000000000 --- a/src/android/camera_proxy.h +++ /dev/null @@ -1,42 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Copyright (C) 2019, Google Inc. - * - * camera_proxy.h - Proxy to camera devices - */ -#ifndef __ANDROID_CAMERA_PROXY_H__ -#define __ANDROID_CAMERA_PROXY_H__ - -#include - -#include - -#include - -class CameraDevice; - -class CameraProxy -{ -public: - CameraProxy(unsigned int id, const std::shared_ptr &camera); - ~CameraProxy(); - - int open(const hw_module_t *hardwareModule); - void close(); - - void initialize(const camera3_callback_ops_t *callbacks); - const camera_metadata_t *getStaticMetadata(); - const camera_metadata_t *constructDefaultRequestSettings(int type); - int configureStreams(camera3_stream_configuration_t *stream_list); - int processCaptureRequest(camera3_capture_request_t *request); - - unsigned int id() const { return id_; } - camera3_device_t *camera3Device() { return &camera3Device_; } - -private: - unsigned int id_; - CameraDevice *cameraDevice_; - camera3_device_t camera3Device_; -}; - -#endif /* __ANDROID_CAMERA_PROXY_H__ */ diff --git a/src/android/meson.build b/src/android/meson.build index 70dfcc1df27a..5a5a332e6a6f 100644 --- a/src/android/meson.build +++ b/src/android/meson.build @@ -3,7 +3,7 @@ android_hal_sources = files([ 'camera_hal_manager.cpp', 'camera_device.cpp', 'camera_metadata.cpp', - 'camera_proxy.cpp', + 'camera_ops.cpp', ]) android_camera_metadata_sources = files([