[libcamera-devel,RFC/PATCH] libcamera: ipa_manager: Proxy open-source IPAs to a thread

3207 diff mbox series

While closed-source IPA modules will always be sandboxed, open-source
IPA modules may be run in the main libcamera process or be sandboxed,
depending on platform configuration. These two models exhibit very
different timings, which require extensive testing with both
configurations.

When run into the main libcamera process, IPA modules are executed in
the pipeline handler thread (which is currently a global CameraManager
thread). Time-consuming operations in the IPA may thus slow down the
pipeline handler and compromise real-time behaviour. At least some
pipeline handlers will thus likely spawn a thread to isolate the IPA,
leading to code duplication in pipeline handlers.

Solve both issues by always proxying IPA modules. For open-source IPA
modules that run in the libcamera process, a new IPAProxyThread class is
added to run the IPA in a separate thread.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Hello,

This patch is currently untested, I'm working on expanding the VIMC IPA
to provide unit tests. I've however decided to send it already to get
feedback on the idea.

 src/libcamera/ipa_manager.cpp            |  48 ++++-----
 src/libcamera/proxy/ipa_proxy_thread.cpp | 130 +++++++++++++++++++++++
 src/libcamera/proxy/meson.build          |   1 +
 3 files changed, 152 insertions(+), 27 deletions(-)
 create mode 100644 src/libcamera/proxy/ipa_proxy_thread.cpp

On Fri, Mar 20, 2020 at 03:16:18AM +0200, Laurent Pinchart wrote:
> While closed-source IPA modules will always be sandboxed, open-source
> IPA modules may be run in the main libcamera process or be sandboxed,
> depending on platform configuration. These two models exhibit very
> different timings, which require extensive testing with both
> configurations.
> 
> When run into the main libcamera process, IPA modules are executed in
> the pipeline handler thread (which is currently a global CameraManager
> thread). Time-consuming operations in the IPA may thus slow down the
> pipeline handler and compromise real-time behaviour. At least some
> pipeline handlers will thus likely spawn a thread to isolate the IPA,
> leading to code duplication in pipeline handlers.
> 
> Solve both issues by always proxying IPA modules. For open-source IPA
> modules that run in the libcamera process, a new IPAProxyThread class is
> added to run the IPA in a separate thread.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Hello,
> 
> This patch is currently untested, I'm working on expanding the VIMC IPA
> to provide unit tests. I've however decided to send it already to get
> feedback on the idea.

Quick update, thanks to Niklas' testing of the patch, we've noticed an
issue with unmapBuffers() racing with processEvent(). I'll work on a
fix, but the general idea holds.

>  src/libcamera/ipa_manager.cpp            |  48 ++++-----
>  src/libcamera/proxy/ipa_proxy_thread.cpp | 130 +++++++++++++++++++++++
>  src/libcamera/proxy/meson.build          |   1 +
>  3 files changed, 152 insertions(+), 27 deletions(-)
>  create mode 100644 src/libcamera/proxy/ipa_proxy_thread.cpp
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index bcaae3564ea1..6d23f4705065 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -12,7 +12,6 @@
>  #include <string.h>
>  #include <sys/types.h>
>  
> -#include "ipa_context_wrapper.h"
>  #include "ipa_module.h"
>  #include "ipa_proxy.h"
>  #include "log.h"
> @@ -271,40 +270,35 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe,
>  	if (!m)
>  		return nullptr;
>  
> -	if (!m->isOpenSource()) {
> -		IPAProxyFactory *pf = nullptr;
> -		std::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories();
> -
> -		for (IPAProxyFactory *factory : factories) {
> -			/* TODO: Better matching */
> -			if (!strcmp(factory->name().c_str(), "IPAProxyLinux")) {
> -				pf = factory;
> -				break;
> -			}
> -		}
> -
> -		if (!pf) {
> -			LOG(IPAManager, Error) << "Failed to get proxy factory";
> -			return nullptr;
> -		}
> +	/*
> +	 * Load and run the IPA module in a thread if it is open-source, or
> +	 * isolate it in a separate process otherwise.
> +	 *
> +	 * \todo Implement a better proxy selection
> +	 */
> +	const char *proxyName = m->isOpenSource()
> +			      ? "IPAProxyThread" : "IPAProxyLinux";
> +	IPAProxyFactory *pf = nullptr;
>  
> -		std::unique_ptr<IPAProxy> proxy = pf->create(m);
> -		if (!proxy->isValid()) {
> -			LOG(IPAManager, Error) << "Failed to load proxy";
> -			return nullptr;
> +	for (IPAProxyFactory *factory : IPAProxyFactory::factories()) {
> +		if (!strcmp(factory->name().c_str(), proxyName)) {
> +			pf = factory;
> +			break;
>  		}
> -
> -		return proxy;
>  	}
>  
> -	if (!m->load())
> +	if (!pf) {
> +		LOG(IPAManager, Error) << "Failed to get proxy factory";
>  		return nullptr;
> +	}
>  
> -	struct ipa_context *ctx = m->createContext();
> -	if (!ctx)
> +	std::unique_ptr<IPAProxy> proxy = pf->create(m);
> +	if (!proxy->isValid()) {
> +		LOG(IPAManager, Error) << "Failed to load proxy";
>  		return nullptr;
> +	}
>  
> -	return std::make_unique<IPAContextWrapper>(ctx);
> +	return proxy;
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> new file mode 100644
> index 000000000000..22006b3c98ee
> --- /dev/null
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_proxy_thread.cpp - Proxy running an Image Processing Algorithm in a thread
> + */
> +
> +#include <memory>
> +
> +#include <ipa/ipa_interface.h>
> +#include <ipa/ipa_module_info.h>
> +
> +#include "ipa_context_wrapper.h"
> +#include "ipa_module.h"
> +#include "ipa_proxy.h"
> +#include "log.h"
> +#include "thread.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAProxy)
> +
> +class IPAProxyThread : public IPAProxy, public Object
> +{
> +public:
> +	IPAProxyThread(IPAModule *ipam);
> +	~IPAProxyThread();
> +
> +	int init() override;
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> +	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> +	void processEvent(const IPAOperationData &event) override;
> +
> +private:
> +	void queueFrameAction(unsigned int frame, const IPAOperationData &data);
> +
> +	/* Helper class to invoke processEvent() in another thread. */
> +	class ThreadProxy : public Object
> +	{
> +	public:
> +		void setIPA(IPAInterface *ipa)
> +		{
> +			ipa_ = ipa;
> +		}
> +
> +		void processEvent(const IPAOperationData &event)
> +		{
> +			ipa_->processEvent(event);
> +		}
> +
> +	private:
> +		IPAInterface *ipa_;
> +	};
> +
> +	Thread thread_;
> +	ThreadProxy proxy_;
> +	std::unique_ptr<IPAInterface> ipa_;
> +};
> +
> +IPAProxyThread::IPAProxyThread(IPAModule *ipam)
> +{
> +	if (!ipam->load())
> +		return;
> +
> +	struct ipa_context *ctx = ipam->createContext();
> +	if (!ctx) {
> +		LOG(IPAProxy, Error)
> +			<< "Failed to create IPA context for " << ipam->path();
> +		return;
> +	}
> +
> +	ipa_ = std::make_unique<IPAContextWrapper>(ctx);
> +	proxy_.setIPA(ipa_.get());
> +
> +	/*
> +	 * Proxy the queueFrameAction signal to dispatch it in the caller's
> +	 * thread.
> +	 */
> +	ipa_->queueFrameAction.connect(this, &IPAProxyThread::queueFrameAction);
> +
> +	valid_ = true;
> +}
> +
> +IPAProxyThread::~IPAProxyThread()
> +{
> +	thread_.exit();
> +	thread_.wait();
> +}
> +
> +int IPAProxyThread::init()
> +{
> +	thread_.start();
> +	proxy_.moveToThread(&thread_);
> +
> +	return ipa_->init();
> +}
> +
> +void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +{
> +	ipa_->configure(streamConfig, entityControls);
> +}
> +
> +void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> +{
> +	ipa_->mapBuffers(buffers);
> +}
> +
> +void IPAProxyThread::unmapBuffers(const std::vector<unsigned int> &ids)
> +{
> +	ipa_->unmapBuffers(ids);
> +}
> +
> +void IPAProxyThread::processEvent(const IPAOperationData &event)
> +{
> +	/* Dispatch the processEvent() call to the thread. */
> +	proxy_.invokeMethod(&ThreadProxy::processEvent, ConnectionTypeQueued,
> +			    event);
> +}
> +
> +void IPAProxyThread::queueFrameAction(unsigned int frame, const IPAOperationData &data)
> +{
> +	IPAInterface::queueFrameAction.emit(frame, data);
> +}
> +
> +REGISTER_IPA_PROXY(IPAProxyThread)
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
> index efc113230217..6c00d5f30ad2 100644
> --- a/src/libcamera/proxy/meson.build
> +++ b/src/libcamera/proxy/meson.build
> @@ -1,3 +1,4 @@
>  libcamera_sources += files([
>      'ipa_proxy_linux.cpp',
> +    'ipa_proxy_thread.cpp',
>  ])
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel