[libcamera-devel,4/8] libcamera: ipa_module: allow instantiation of IPAInterface

1322 diff mbox series

Add functions for loading the IPAInterface factory function from an IPA
module shared object, and for creating an instance of an IPAInterface.
These functions will be used by IPAManager, from which a PipelineHandler
will request an IPAInterface.

Also update meson to add the "-ldl" linker argument, to allow loading of
the factory function from a shared object.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/include/ipa_module.h | 15 +++++-
 src/libcamera/ipa_module.cpp       | 80 ++++++++++++++++++++++++++++--
 src/libcamera/meson.build          |  3 +-
 3 files changed, 92 insertions(+), 6 deletions(-)

Hi Paul,

Thank you for the patch.

On Mon, May 27, 2019 at 06:35:36PM -0400, Paul Elder wrote:
> Add functions for loading the IPAInterface factory function from an IPA
> module shared object, and for creating an instance of an IPAInterface.
> These functions will be used by IPAManager, from which a PipelineHandler
> will request an IPAInterface.
> 
> Also update meson to add the "-ldl" linker argument, to allow loading of
> the factory function from a shared object.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/include/ipa_module.h | 15 +++++-
>  src/libcamera/ipa_module.cpp       | 80 ++++++++++++++++++++++++++++--
>  src/libcamera/meson.build          |  3 +-
>  3 files changed, 92 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h
> index a4c6dbd..bb03407 100644
> --- a/src/libcamera/include/ipa_module.h
> +++ b/src/libcamera/include/ipa_module.h
> @@ -7,9 +7,10 @@
>  #ifndef __LIBCAMERA_IPA_MODULE_H__
>  #define __LIBCAMERA_IPA_MODULE_H__
>  
> -#include <string>
> -
> +#include <libcamera/ipa/ipa_interface.h>
>  #include <libcamera/ipa/ipa_module_info.h>
> +#include <memory>
> +#include <string>

We usually put the C++ headers first, followed by a blank line, followed
by the libcamera public headers.

>  namespace libcamera {
>  
> @@ -17,16 +18,26 @@ class IPAModule
>  {
>  public:
>  	explicit IPAModule(const std::string &libPath);
> +	~IPAModule();
>  
>  	bool isValid() const;
>  
>  	const struct IPAModuleInfo &info() const;
>  
> +	bool load();
> +
> +	std::unique_ptr<IPAInterface> createInstance();
> +
>  private:
>  	struct IPAModuleInfo info_;
>  
>  	std::string libPath_;
>  	bool valid_;
> +	bool loaded_;
> +
> +	void *dlhandle_;

s/dlhandle_/dlHandle_/ ?

> +	typedef IPAInterface *(*ipaiFactory)(void);

What does the second i in ipai stand for ?

> +	ipaiFactory ipaCreate_;
>  
>  	int loadIPAModuleInfo();
>  };
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 6e68934..9bb0594 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -7,6 +7,7 @@
>  
>  #include "ipa_module.h"
>  
> +#include <dlfcn.h>
>  #include <elf.h>
>  #include <errno.h>
>  #include <fcntl.h>
> @@ -242,13 +243,11 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>   * The IPA module shared object file must be of the same endianness and
>   * bitness as libcamera.
>   *
> - * \todo load funtions from the IPA to be used by pipelines
> - *
>   * The caller shall call the isValid() method after constructing an
>   * IPAModule instance to verify the validity of the IPAModule.
>   */
>  IPAModule::IPAModule(const std::string &libPath)
> -	: libPath_(libPath), valid_(false)
> +	: libPath_(libPath), valid_(false), loaded_(false)

You should initialise dlhandle_ too.

>  {
>  	if (loadIPAModuleInfo() < 0)
>  		return;
> @@ -256,6 +255,12 @@ IPAModule::IPAModule(const std::string &libPath)
>  	valid_ = true;
>  }
>  
> +IPAModule::~IPAModule()
> +{
> +	if (dlhandle_)
> +		dlclose(dlhandle_);
> +}
> +
>  int IPAModule::loadIPAModuleInfo()
>  {
>  	int fd = open(libPath_.c_str(), O_RDONLY);
> @@ -325,4 +330,73 @@ const struct IPAModuleInfo &IPAModule::info() const
>  	return info_;
>  }
>  
> +/**
> + * \brief Load the IPA implementation factory from the shared object
> + *
> + * The IPA module shared object implements an IPAInterface class to be used
> + * by pipeline handlers. This function loads the factory function from the
> + * shared object. Later, createInstance() can be called to instantiate the
> + * IPAInterface.
> + *
> + * This function only needs to be called successfully once, after which

We usually use method instead of function for member functions.

> + * createInstance can be called as many times as IPAInterface instances are
> + * needed.
> + *
> + * Calling this function on an invalid module (as returned by isValid()) is
> + * an error.
> + *
> + * \return true if load was successful, or already loaded, and false otherwise
> + */
> +bool IPAModule::load()
> +{
> +	if (!valid_)
> +		return false;
> +
> +	if (loaded_)
> +		return true;
> +
> +	dlhandle_ = dlopen(libPath_.c_str(), RTLD_LAZY);
> +	if (!dlhandle_) {
> +		LOG(IPAModule, Error) << "Failed to open IPA module shared object"

s/object/object /

> +				      << dlerror();

To shorten the lines, we usually use the following style

		LOG(IPAModule, Error)
			<< "Failed to open IPA module shared object: "
			<< dlerror();

> +		return false;
> +	}
> +
> +	void *symbol = dlsym(dlhandle_, "ipaCreate");
> +	if (!symbol) {
> +		LOG(IPAModule, Error) << "Failed to load ipaCreate() from IPA module shared object"
> +				      << dlerror();

Same here.

> +		dlclose(dlhandle_);

		dlhandle_ = nullptr;

otherwise you will have a double-close.

> +		return false;
> +	}
> +
> +	ipaCreate_ = (ipaiFactory)symbol;
> +
> +	loaded_ = true;
> +
> +	return true;
> +}
> +
> +/**
> + * \brief Instantiate an IPAInterface
> + *
> + * After the IPAInterface implementation factory has been loaded (with load()),
> + * an instance can be created with this function.

To avoid showing too much of the internal details in the documentation,
how about

 * After loading the IPA module with load(), this method creates an
 * instance of the IPA module interface.

> + *
> + * Calling this function on a module that has not yet been loaded, or an
> + * invalid module (as returned by load() and isValid(), respectively) is
> + * an error.
> + *
> + * \return the IPA implementation as a new IPAInterface instance

on success, or nullptr on error.

> + */
> +std::unique_ptr<IPAInterface> IPAModule::createInstance()
> +{
> +	if (!valid_ || !loaded_)
> +		return nullptr;
> +
> +	std::unique_ptr<IPAInterface> ipai(ipaCreate_());
> +
> +	return ipai;

You can simplify this with

	return std::unique_ptr<IPAInterface>(ipaCreate_());

> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 07335e5..32f7da4 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -65,7 +65,8 @@ libcamera = shared_library('camera',
>                             libcamera_sources,
>                             install : true,
>                             include_directories : includes,
> -                           dependencies : libudev)
> +                           dependencies : libudev,
> +                           link_args : '-ldl')
>  
>  libcamera_dep = declare_dependency(sources : [libcamera_api, libcamera_h],
>                                     include_directories : libcamera_includes,