From 46d544345cbae270da533a737e334880720ceea5 Mon Sep 17 00:00:00 2001
From: Paul Elder <paul.elder@ideasonboard.com>
Date: Tue, 2 Jun 2020 11:15:37 +0900
Subject: libcamera: IPAManager: make IPAManager lifetime explicitly managed
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If any ipa_context instances are destroyed after the IPAManager is
destroyed, then a segfault will occur, since the modules have been
unloaded by the IPAManager and the context function pointers have been
freed.

Fix this by making the lifetime of the IPAManager explicit, and make the
CameraManager construct and deconstruct (automatically, via a unique
pointer) the IPAManager.

Also update the IPA interface test to do the construction and
deconstruction of the IPAManager, as it does not use the CameraManager.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/internal/ipa_manager.h |  6 ++++--
 src/libcamera/camera_manager.cpp         |  3 +++
 src/libcamera/ipa_manager.cpp            | 25 ++++++++++++++++++++-----
 test/ipa/ipa_interface_test.cpp          |  5 +++++
 4 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index 9f35b16d..f17fd0a8 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -22,6 +22,9 @@ namespace libcamera {
 class IPAManager
 {
 public:
+	IPAManager();
+	~IPAManager();
+
 	static IPAManager *instance();
 
 	std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
@@ -29,8 +32,7 @@ public:
 					    uint32_t minVersion);
 
 private:
-	IPAManager();
-	~IPAManager();
+	static IPAManager *self_;
 
 	void parseDir(const char *libDir, unsigned int maxDepth,
 		      std::vector<std::string> &files);
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 849377ad..da806fa7 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -15,6 +15,7 @@
 
 #include "libcamera/internal/device_enumerator.h"
 #include "libcamera/internal/event_dispatcher_poll.h"
+#include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/log.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/thread.h"
@@ -63,6 +64,8 @@ private:
 
 	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
 	std::unique_ptr<DeviceEnumerator> enumerator_;
+
+	IPAManager ipaManager_;
 };
 
 CameraManager::Private::Private(CameraManager *cm)
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index 505cf610..e7fddf42 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -93,8 +93,21 @@ LOG_DEFINE_CATEGORY(IPAManager)
  * plain C API, or to transmit the data to the isolated process through IPC.
  */
 
+IPAManager *IPAManager::self_ = nullptr;
+
+/**
+ * \brief Construct an IPAManager instance
+ *
+ * The IPAManager class is meant to only be instantiated once, by the
+ * CameraManager. Pipeline handlers shall use the instance() function to access
+ * the IPAManager instance.
+ */
 IPAManager::IPAManager()
 {
+	if (self_)
+		LOG(IPAManager, Fatal)
+			<< "Multiple IPAManager objects are not allowed";
+
 	unsigned int ipaCount = 0;
 
 	/* User-specified paths take precedence. */
@@ -134,27 +147,29 @@ IPAManager::IPAManager()
 	if (!ipaCount)
 		LOG(IPAManager, Warning)
 			<< "No IPA found in '" IPA_MODULE_DIR "'";
+
+	self_ = this;
 }
 
 IPAManager::~IPAManager()
 {
 	for (IPAModule *module : modules_)
 		delete module;
+
+	self_ = nullptr;
 }
 
 /**
  * \brief Retrieve the IPA manager instance
  *
- * The IPAManager is a singleton and can't be constructed manually. This
- * function shall instead be used to retrieve the single global instance of the
- * manager.
+ * The IPAManager is constructed by the CameraManager. This function shall be
+ * used to retrieve the single instance of the manager.
  *
  * \return The IPA manager instance
  */
 IPAManager *IPAManager::instance()
 {
-	static IPAManager ipaManager;
-	return &ipaManager;
+	return self_;
 }
 
 /**
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
index 2f02af49..153493ba 100644
--- a/test/ipa/ipa_interface_test.cpp
+++ b/test/ipa/ipa_interface_test.cpp
@@ -39,11 +39,15 @@ public:
 	~IPAInterfaceTest()
 	{
 		delete notifier_;
+		ipa_.reset();
+		ipaManager_.reset();
 	}
 
 protected:
 	int init() override
 	{
+		ipaManager_ = make_unique<IPAManager>();
+
 		/* Create a pipeline handler for vimc. */
 		std::vector<PipelineHandlerFactory *> &factories =
 			PipelineHandlerFactory::factories();
@@ -161,6 +165,7 @@ private:
 
 	std::shared_ptr<PipelineHandler> pipe_;
 	std::unique_ptr<IPAProxy> ipa_;
+	std::unique_ptr<IPAManager> ipaManager_;
 	enum IPAOperationCode trace_;
 	EventNotifier *notifier_;
 	int fd_;
-- 
cgit v1.2.1