[libcamera-devel,RFC,06/10] libcamera: ipa_manager: add shims

1367 diff mbox series

Make IPAManager load shim shared objects in addition to IPA module
shared objects, and keep them in a shims list. When requested for an
IPA, if the IPA requires isolation, wrap the IPA in the first shim that
is available.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/include/ipa_manager.h |  1 +
 src/libcamera/ipa_manager.cpp       | 34 +++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)

Hi Paul,

Thank you for the patch.

On Wed, Jun 05, 2019 at 06:18:13PM -0400, Paul Elder wrote:
> Make IPAManager load shim shared objects in addition to IPA module
> shared objects, and keep them in a shims list. When requested for an
> IPA, if the IPA requires isolation, wrap the IPA in the first shim that
> is available.

I think we need to start documenting this somewhere. It deals with the
libcamera architecture, so Documentation/ could be an option. Otherwise
the file block at the top of ipa_manager.cpp is another candidate.

I also think that we should take a bit of time to evaluate the other
available options. Reading you patch I was wondering if we should build
the shims in libcamera instead of loading them dynamically. I'm not
saying it is a better option, but we should evaluate the pros and cons.
One point that we should keep in mind is that which shim to load will
likely come from a configuration file, but it could also be useful if we
could detect automatically which shims are or are not suitable for the
platform (for instance to automatically use the Android shim on
Android).

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/include/ipa_manager.h |  1 +
>  src/libcamera/ipa_manager.cpp       | 34 +++++++++++++++++++++++++++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
> index 310ce7c..a0fb8ad 100644
> --- a/src/libcamera/include/ipa_manager.h
> +++ b/src/libcamera/include/ipa_manager.h
> @@ -28,6 +28,7 @@ public:
>  
>  private:
>  	std::vector<IPAModule *> modules_;
> +	std::vector<IPAModule *> shims_;
>  
>  	IPAManager();
>  	~IPAManager();
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index f689aa6..3648ea6 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -110,7 +110,10 @@ int IPAManager::addDir(const char *libDir)
>  			continue;
>  		}
>  
> -		modules_.push_back(ipaModule);
> +		if (!strncmp(ipaModule->info().pipelineName, "Shim", 4))
> +			shims_.push_back(ipaModule);
> +		else
> +			modules_.push_back(ipaModule);
>  		count++;
>  	}
>  
> @@ -132,6 +135,7 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe,
>  						    uint32_t minVersion)
>  {
>  	IPAModule *m = nullptr;
> +	IPAModule *shim = nullptr;

You can declare this variable below when assigning it.

>  
>  	for (IPAModule *module : modules_) {
>  		if (module->match(pipe, minVersion, maxVersion)) {
> @@ -140,7 +144,33 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe,
>  		}
>  	}
>  
> -	if (!m || !m->load())
> +	if (!m)
> +		return nullptr;
> +
> +	if (m->info().isolate) {
> +		if (shims_.empty()) {
> +			LOG(IPAManager, Error) << "No shims available";
> +			return nullptr;
> +		}
> +
> +		shim = shims_.front();
> +		if (!shim || !shim->load()) {

I don't think shim can be nullptr here, otherwise the shims_.empty()
check above would have triggered.

> +			LOG(IPAManager, Error) << "Failed to obtain shim";
> +			return nullptr;
> +		}
> +
> +		auto shimIPAIntf = shim->createInstance();

We usually try to type out types explicitly instead of using auto,
except when it would be very inconvenient.

		std::unique_ptr<IPAInterface> shimIPAIntf = shim->createInstance();

doesn't look that bad. And I think I would even name the variable just
intf (or interface).

> +		if (!shimIPAIntf) {
> +			LOG(IPAManager, Error) << "Failed to load shim";
> +			return nullptr;
> +		}
> +
> +		shimIPAIntf->init(m->path());
> +
> +		return shimIPAIntf;
> +	}
> +
> +	if (!m->load())
>  		return nullptr;
>  
>  	return m->createInstance();