[libcamera-devel] [PATCH v2 04/10] libcamera: ipa_module: allow instantiation of IPAInterface

[Paul Elder]

Hi Laurent,

On Tue, Jun 04, 2019 at 01:49:06PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.

Thank you for the review.

> On Mon, Jun 03, 2019 at 07:16:31PM -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>
> > ---
> > Changes in v2:
> > - mostly just documentation
> > 
> >  src/libcamera/include/ipa_module.h | 12 +++++
> >  src/libcamera/ipa_module.cpp       | 83 ++++++++++++++++++++++++++++--
> >  src/libcamera/meson.build          |  3 +-
> >  3 files changed, 94 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h
> > index a4c6dbd..557435c 100644
> > --- a/src/libcamera/include/ipa_module.h
> > +++ b/src/libcamera/include/ipa_module.h
> > @@ -7,8 +7,10 @@
> >  #ifndef __LIBCAMERA_IPA_MODULE_H__
> >  #define __LIBCAMERA_IPA_MODULE_H__
> >  
> > +#include <memory>
> >  #include <string>
> >  
> > +#include <libcamera/ipa/ipa_interface.h>
> >  #include <libcamera/ipa/ipa_module_info.h>
> >  
> >  namespace libcamera {
> > @@ -17,16 +19,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_;
> > +	typedef IPAInterface *(*ipaIntfFactory)(void);
> 
> This is a type so it should start with an uppercase letter. Furthermore,

Ah, right.

> as the type name is defined in the context of the IPAModule class, it
> doesn't have to start with IPA. How about just FactoryFunction ?

No, I'd prefer to have IPAIntfFactory. That way it's really clear what
the factory function is meant to produce. IntfFactory becomes kind of
vague too.

> > +	ipaIntfFactory ipaCreate_;
> >  
> >  	int loadIPAModuleInfo();
> >  };
> > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> > index 067f868..91d97ae 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>
> > @@ -257,13 +258,12 @@ 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),
> > +	  dlHandle_(nullptr), ipaCreate_(nullptr)
> >  {
> >  	if (loadIPAModuleInfo() < 0)
> >  		return;
> > @@ -271,6 +271,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);
> > @@ -340,4 +346,75 @@ 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
> 
> s/This function/This method/
> 
> ("factory function" is correct as it's a pure C function)
> 
> > + * shared object. Later, createInstance() can be called to instantiate the
> > + * IPAInterface.
> > + *
> > + * This method only needs to be called successfully once, after which
> > + * createInstance can be called as many times as IPAInterface instances are
> 
> createInstance()
> 
> > + * 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
> 
> s/true/True/
> 
> > + */
> > +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();
> > +		return false;
> > +	}
> > +
> > +	void *symbol = dlsym(dlHandle_, "ipaCreate");
> > +	if (!symbol) {
> > +		LOG(IPAModule, Error)
> > +			<< "Failed to load ipaCreate() from IPA module shared object "
> 
> s/object/object:/
> 
> > +			<< dlerror();
> > +		dlclose(dlHandle_);
> > +		dlHandle_ = nullptr;
> > +		return false;
> > +	}
> > +
> > +	ipaCreate_ = (ipaIntfFactory)symbol;
> 
> Let's use C++-style casts.
> 
> 	ipaCreate_ = reinterpret_cast<ipaIntfFactory>(symbol);
> 
> > +
> > +	loaded_ = true;
> > +
> > +	return true;
> > +}
> > +
> > +/**
> > + * \brief Instantiate an IPAInterface
> > + *
> > + * 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,
> 
> s/the/The/
> 
> With those small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > + * or nullptr on error
> > + */
> > +std::unique_ptr<IPAInterface> IPAModule::createInstance()
> > +{
> > +	if (!valid_ || !loaded_)
> > +		return nullptr;
> > +
> > +	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,
> 

Thanks,

Paul