[libcamera-devel] [PATCH v2 1/7] IPAManager: make IPAManager lifetime explicitly managed

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jun 05, 2020 at 06:01:00PM +0900, Paul Elder wrote:
> 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 at ideasonboard.com>
> 
> ---
> Changes in v2: Move the IPAManager instance into
> 	       CameraManager::Private from CameraManager
> ---
>  include/libcamera/camera_manager.h       |  1 -
>  include/libcamera/internal/ipa_manager.h |  6 ++++--
>  src/libcamera/camera_manager.cpp         |  3 +++
>  src/libcamera/ipa_manager.cpp            | 20 ++++++++++++++++++--
>  test/ipa/ipa_interface_test.cpp          |  5 +++++
>  5 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848a..00658a70 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -18,7 +18,6 @@ namespace libcamera {
>  
>  class Camera;
>  class EventDispatcher;
> -
>  class CameraManager : public Object
>  {
>  public:
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 16d74291..43f700d3 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..abb681a3 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,12 +147,16 @@ 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;
>  }
>  
>  /**
> @@ -153,8 +170,7 @@ IPAManager::~IPAManager()
>   */
>  IPAManager *IPAManager::instance()

I think the documentation needs to be updated too. With this handled,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  {
> -	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_;

-- 
Regards,

Laurent Pinchart