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

[Laurent Pinchart]

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 at 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,

-- 
Regards,

Laurent Pinchart