[{"id":1717,"web_url":"https://patchwork.libcamera.org/comment/1717/","msgid":"<20190528154231.GG14336@pendragon.ideasonboard.com>","date":"2019-05-28T15:42:31","subject":"Re: [libcamera-devel] [PATCH 4/8] libcamera: ipa_module: allow\n\tinstantiation of IPAInterface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, May 27, 2019 at 06:35:36PM -0400, Paul Elder wrote:\n> Add functions for loading the IPAInterface factory function from an IPA\n> module shared object, and for creating an instance of an IPAInterface.\n> These functions will be used by IPAManager, from which a PipelineHandler\n> will request an IPAInterface.\n> \n> Also update meson to add the \"-ldl\" linker argument, to allow loading of\n> the factory function from a shared object.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/include/ipa_module.h | 15 +++++-\n> src/libcamera/ipa_module.cpp | 80 ++++++++++++++++++++++++++++--\n> src/libcamera/meson.build | 3 +-\n> 3 files changed, 92 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> index a4c6dbd..bb03407 100644\n> --- a/src/libcamera/include/ipa_module.h\n> +++ b/src/libcamera/include/ipa_module.h\n> @@ -7,9 +7,10 @@\n> #ifndef __LIBCAMERA_IPA_MODULE_H__\n> #define __LIBCAMERA_IPA_MODULE_H__\n> \n> -#include \n> -\n> +#include \n> #include \n> +#include \n> +#include \n\nWe usually put the C++ headers first, followed by a blank line, followed\nby the libcamera public headers.\n\n> namespace libcamera {\n> \n> @@ -17,16 +18,26 @@ class IPAModule\n> {\n> public:\n> \texplicit IPAModule(const std::string &libPath);\n> +\t~IPAModule();\n> \n> \tbool isValid() const;\n> \n> \tconst struct IPAModuleInfo &info() const;\n> \n> +\tbool load();\n> +\n> +\tstd::unique_ptr createInstance();\n> +\n> private:\n> \tstruct IPAModuleInfo info_;\n> \n> \tstd::string libPath_;\n> \tbool valid_;\n> +\tbool loaded_;\n> +\n> +\tvoid *dlhandle_;\n\ns/dlhandle_/dlHandle_/ ?\n\n> +\ttypedef IPAInterface *(*ipaiFactory)(void);\n\nWhat does the second i in ipai stand for ?\n\n> +\tipaiFactory ipaCreate_;\n> \n> \tint loadIPAModuleInfo();\n> };\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 6e68934..9bb0594 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \"ipa_module.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -242,13 +243,11 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n> * The IPA module shared object file must be of the same endianness and\n> * bitness as libcamera.\n> *\n> - * \\todo load funtions from the IPA to be used by pipelines\n> - *\n> * The caller shall call the isValid() method after constructing an\n> * IPAModule instance to verify the validity of the IPAModule.\n> */\n> IPAModule::IPAModule(const std::string &libPath)\n> -\t: libPath_(libPath), valid_(false)\n> +\t: libPath_(libPath), valid_(false), loaded_(false)\n\nYou should initialise dlhandle_ too.\n\n> {\n> \tif (loadIPAModuleInfo() < 0)\n> \t\treturn;\n> @@ -256,6 +255,12 @@ IPAModule::IPAModule(const std::string &libPath)\n> \tvalid_ = true;\n> }\n> \n> +IPAModule::~IPAModule()\n> +{\n> +\tif (dlhandle_)\n> +\t\tdlclose(dlhandle_);\n> +}\n> +\n> int IPAModule::loadIPAModuleInfo()\n> {\n> \tint fd = open(libPath_.c_str(), O_RDONLY);\n> @@ -325,4 +330,73 @@ const struct IPAModuleInfo &IPAModule::info() const\n> \treturn info_;\n> }\n> \n> +/**\n> + * \\brief Load the IPA implementation factory from the shared object\n> + *\n> + * The IPA module shared object implements an IPAInterface class to be used\n> + * by pipeline handlers. This function loads the factory function from the\n> + * shared object. Later, createInstance() can be called to instantiate the\n> + * IPAInterface.\n> + *\n> + * This function only needs to be called successfully once, after which\n\nWe usually use method instead of function for member functions.\n\n> + * createInstance can be called as many times as IPAInterface instances are\n> + * needed.\n> + *\n> + * Calling this function on an invalid module (as returned by isValid()) is\n> + * an error.\n> + *\n> + * \\return true if load was successful, or already loaded, and false otherwise\n> + */\n> +bool IPAModule::load()\n> +{\n> +\tif (!valid_)\n> +\t\treturn false;\n> +\n> +\tif (loaded_)\n> +\t\treturn true;\n> +\n> +\tdlhandle_ = dlopen(libPath_.c_str(), RTLD_LAZY);\n> +\tif (!dlhandle_) {\n> +\t\tLOG(IPAModule, Error) << \"Failed to open IPA module shared object\"\n\ns/object/object /\n\n> +\t\t\t\t << dlerror();\n\nTo shorten the lines, we usually use the following style\n\n\t\tLOG(IPAModule, Error)\n\t\t\t<< \"Failed to open IPA module shared object: \"\n\t\t\t<< dlerror();\n\n> +\t\treturn false;\n> +\t}\n> +\n> +\tvoid *symbol = dlsym(dlhandle_, \"ipaCreate\");\n> +\tif (!symbol) {\n> +\t\tLOG(IPAModule, Error) << \"Failed to load ipaCreate() from IPA module shared object\"\n> +\t\t\t\t << dlerror();\n\nSame here.\n\n> +\t\tdlclose(dlhandle_);\n\n\t\tdlhandle_ = nullptr;\n\notherwise you will have a double-close.\n\n> +\t\treturn false;\n> +\t}\n> +\n> +\tipaCreate_ = (ipaiFactory)symbol;\n> +\n> +\tloaded_ = true;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Instantiate an IPAInterface\n> + *\n> + * After the IPAInterface implementation factory has been loaded (with load()),\n> + * an instance can be created with this function.\n\nTo avoid showing too much of the internal details in the documentation,\nhow about\n\n * After loading the IPA module with load(), this method creates an\n * instance of the IPA module interface.\n\n> + *\n> + * Calling this function on a module that has not yet been loaded, or an\n> + * invalid module (as returned by load() and isValid(), respectively) is\n> + * an error.\n> + *\n> + * \\return the IPA implementation as a new IPAInterface instance\n\non success, or nullptr on error.\n\n> + */\n> +std::unique_ptr IPAModule::createInstance()\n> +{\n> +\tif (!valid_ || !loaded_)\n> +\t\treturn nullptr;\n> +\n> +\tstd::unique_ptr ipai(ipaCreate_());\n> +\n> +\treturn ipai;\n\nYou can simplify this with\n\n\treturn std::unique_ptr(ipaCreate_());\n\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 07335e5..32f7da4 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -65,7 +65,8 @@ libcamera = shared_library('camera',\n> libcamera_sources,\n> install : true,\n> include_directories : includes,\n> - dependencies : libudev)\n> + dependencies : libudev,\n> + link_args : '-ldl')\n> \n> libcamera_dep = declare_dependency(sources : [libcamera_api, libcamera_h],\n> include_directories : libcamera_includes,","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4086A600EA\n\tfor ;\n\tTue, 28 May 2019 17:42:54 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-96-53-nat.elisa-mobile.fi\n\t[85.76.96.53])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15405D85;\n\tTue, 28 May 2019 17:42:50 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559058174;\n\tbh=KO0eRTJ5TEARxsNdbuUDLJhe4/Pi4XL0R7FQUsYKFWg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r+Ie7fz6RDoMGK3KoGvZw7eVMmR+RA/3aYpVBFFa9+JO6VcmBI4vI18Z6U+cV0jX4\n\tcXZ6NkF3P+99YI0dTbZrqkzQ1uQdzHeckIia9932XmusDFtaB2wfAvjU+pEfvaMaD2\n\tkDCA9LgWyRd5+jpV1gcbFac7mDOIpxHL2BAHGoi0=","Date":"Tue, 28 May 2019 18:42:31 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190528154231.GG14336@pendragon.ideasonboard.com>","References":"<20190527223540.21855-1-paul.elder@ideasonboard.com>\n\t<20190527223540.21855-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190527223540.21855-5-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/8] libcamera: ipa_module: allow\n\tinstantiation of IPAInterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 28 May 2019 15:42:54 -0000"}}]