[{"id":1754,"web_url":"https://patchwork.libcamera.org/comment/1754/","msgid":"<20190604104906.GG4771@pendragon.ideasonboard.com>","date":"2019-06-04T10:49:06","subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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, Jun 03, 2019 at 07:16:31PM -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> Changes in v2:\n> - mostly just documentation\n> \n> src/libcamera/include/ipa_module.h | 12 +++++\n> src/libcamera/ipa_module.cpp | 83 ++++++++++++++++++++++++++++--\n> src/libcamera/meson.build | 3 +-\n> 3 files changed, 94 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> index a4c6dbd..557435c 100644\n> --- a/src/libcamera/include/ipa_module.h\n> +++ b/src/libcamera/include/ipa_module.h\n> @@ -7,8 +7,10 @@\n> #ifndef __LIBCAMERA_IPA_MODULE_H__\n> #define __LIBCAMERA_IPA_MODULE_H__\n> \n> +#include \n> #include \n> \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -17,16 +19,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> +\ttypedef IPAInterface *(*ipaIntfFactory)(void);\n\nThis is a type so it should start with an uppercase letter. Furthermore,\nas the type name is defined in the context of the IPAModule class, it\ndoesn't have to start with IPA. How about just FactoryFunction ?\n\n> +\tipaIntfFactory ipaCreate_;\n> \n> \tint loadIPAModuleInfo();\n> };\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 067f868..91d97ae 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> @@ -257,13 +258,12 @@ 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> +\t dlHandle_(nullptr), ipaCreate_(nullptr)\n> {\n> \tif (loadIPAModuleInfo() < 0)\n> \t\treturn;\n> @@ -271,6 +271,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> @@ -340,4 +346,75 @@ 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\ns/This function/This method/\n\n(\"factory function\" is correct as it's a pure C function)\n\n> + * shared object. Later, createInstance() can be called to instantiate the\n> + * IPAInterface.\n> + *\n> + * This method only needs to be called successfully once, after which\n> + * createInstance can be called as many times as IPAInterface instances are\n\ncreateInstance()\n\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\ns/true/True/\n\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)\n> +\t\t\t<< \"Failed to open IPA module shared object \"\n\ns/object/object:/\n\n> +\t\t\t<< dlerror();\n> +\t\treturn false;\n> +\t}\n> +\n> +\tvoid *symbol = dlsym(dlHandle_, \"ipaCreate\");\n> +\tif (!symbol) {\n> +\t\tLOG(IPAModule, Error)\n> +\t\t\t<< \"Failed to load ipaCreate() from IPA module shared object \"\n\ns/object/object:/\n\n> +\t\t\t<< dlerror();\n> +\t\tdlclose(dlHandle_);\n> +\t\tdlHandle_ = nullptr;\n> +\t\treturn false;\n> +\t}\n> +\n> +\tipaCreate_ = (ipaIntfFactory)symbol;\n\nLet's use C++-style casts.\n\n\tipaCreate_ = reinterpret_cast(symbol);\n\n> +\n> +\tloaded_ = true;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Instantiate an IPAInterface\n> + *\n> + * After loading the IPA module with load(), this method creates an\n> + * instance of the IPA module interface.\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 on success,\n\ns/the/The/\n\nWith those small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\n> + * or nullptr on error\n> + */\n> +std::unique_ptr IPAModule::createInstance()\n> +{\n> +\tif (!valid_ || !loaded_)\n> +\t\treturn nullptr;\n> +\n> +\treturn std::unique_ptr(ipaCreate_());\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[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7324061F56\n\tfor ;\n\tTue, 4 Jun 2019 12:49:19 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:2788:668:163:5bb7:9f6c:564c:d55e])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 07B032D2;\n\tTue, 4 Jun 2019 12:49:18 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559645359;\n\tbh=8YIL27ooPOHFW682f0tyB8Mu8v7xKnbxiPpKFLvfWWM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KjbVNmjcbpcCPfeLh2d1dq87jYyIFWOlLQx91Sd4b8AxEQQgTM1xn8/J7TCFFPO6F\n\tblvMAg/hcSkzRdMjbCHuvEkIV5gDBDjvkd13eXsiVCUKhWg0kiTWm1hHLuhCh96zU6\n\tlCeW1PjEd6Ddh9bqWLoR44Z2xEfUGURtsvqH42Jc=","Date":"Tue, 4 Jun 2019 13:49:06 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604104906.GG4771@pendragon.ideasonboard.com>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190603231637.28554-5-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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, 04 Jun 2019 10:49:19 -0000"}},{"id":1768,"web_url":"https://patchwork.libcamera.org/comment/1768/","msgid":"<20190604204500.GO32191@localhost.localdomain>","date":"2019-06-04T20:45:00","subject":"Re: [libcamera-devel] [PATCH v2 04/10] libcamera: ipa_module: allow\n\tinstantiation of IPAInterface","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Tue, Jun 04, 2019 at 01:49:06PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n\nThank you for the review.\n\n> On Mon, Jun 03, 2019 at 07:16:31PM -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> > Changes in v2:\n> > - mostly just documentation\n> > \n> > src/libcamera/include/ipa_module.h | 12 +++++\n> > src/libcamera/ipa_module.cpp | 83 ++++++++++++++++++++++++++++--\n> > src/libcamera/meson.build | 3 +-\n> > 3 files changed, 94 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> > index a4c6dbd..557435c 100644\n> > --- a/src/libcamera/include/ipa_module.h\n> > +++ b/src/libcamera/include/ipa_module.h\n> > @@ -7,8 +7,10 @@\n> > #ifndef __LIBCAMERA_IPA_MODULE_H__\n> > #define __LIBCAMERA_IPA_MODULE_H__\n> > \n> > +#include \n> > #include \n> > \n> > +#include \n> > #include \n> > \n> > namespace libcamera {\n> > @@ -17,16 +19,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> > +\ttypedef IPAInterface *(*ipaIntfFactory)(void);\n> \n> This is a type so it should start with an uppercase letter. Furthermore,\n\nAh, right.\n\n> as the type name is defined in the context of the IPAModule class, it\n> doesn't have to start with IPA. How about just FactoryFunction ?\n\nNo, I'd prefer to have IPAIntfFactory. That way it's really clear what\nthe factory function is meant to produce. IntfFactory becomes kind of\nvague too.\n\n> > +\tipaIntfFactory ipaCreate_;\n> > \n> > \tint loadIPAModuleInfo();\n> > };\n> > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> > index 067f868..91d97ae 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> > @@ -257,13 +258,12 @@ 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> > +\t dlHandle_(nullptr), ipaCreate_(nullptr)\n> > {\n> > \tif (loadIPAModuleInfo() < 0)\n> > \t\treturn;\n> > @@ -271,6 +271,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> > @@ -340,4 +346,75 @@ 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> \n> s/This function/This method/\n> \n> (\"factory function\" is correct as it's a pure C function)\n> \n> > + * shared object. Later, createInstance() can be called to instantiate the\n> > + * IPAInterface.\n> > + *\n> > + * This method only needs to be called successfully once, after which\n> > + * createInstance can be called as many times as IPAInterface instances are\n> \n> createInstance()\n> \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> s/true/True/\n> \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)\n> > +\t\t\t<< \"Failed to open IPA module shared object \"\n> \n> s/object/object:/\n> \n> > +\t\t\t<< dlerror();\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tvoid *symbol = dlsym(dlHandle_, \"ipaCreate\");\n> > +\tif (!symbol) {\n> > +\t\tLOG(IPAModule, Error)\n> > +\t\t\t<< \"Failed to load ipaCreate() from IPA module shared object \"\n> \n> s/object/object:/\n> \n> > +\t\t\t<< dlerror();\n> > +\t\tdlclose(dlHandle_);\n> > +\t\tdlHandle_ = nullptr;\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tipaCreate_ = (ipaIntfFactory)symbol;\n> \n> Let's use C++-style casts.\n> \n> \tipaCreate_ = reinterpret_cast(symbol);\n> \n> > +\n> > +\tloaded_ = true;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Instantiate an IPAInterface\n> > + *\n> > + * After loading the IPA module with load(), this method creates an\n> > + * instance of the IPA module interface.\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 on success,\n> \n> s/the/The/\n> \n> With those small issues fixed,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > + * or nullptr on error\n> > + */\n> > +std::unique_ptr IPAModule::createInstance()\n> > +{\n> > +\tif (!valid_ || !loaded_)\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn std::unique_ptr(ipaCreate_());\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,\n> \n\nThanks,\n\nPaul","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 A00136190A\n\tfor ;\n\tTue, 4 Jun 2019 22:45:08 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CFC882D1;\n\tTue, 4 Jun 2019 22:45:07 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559681108;\n\tbh=eu9+sHP9tySBrmgUWL1GztG4i4TqMGIFr9D6JHubLBM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SEtOTTuPPN6Pms7JALAvXXOHPjhi97wlv5FZvv7ct2HuyA0xtGnJ+6wUPhyLR3Ox8\n\tEC3XCtvfBD2twmSYvACtWxBytL/ArTPF/1RaqKcTgE5BFlYZfG9W0MYtkTsyaR8bl/\n\tHKfp+iCLv6AS/gQ2QBpj7eQ2/0cSWj3avG0TMjp0=","Date":"Tue, 4 Jun 2019 16:45:00 -0400","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604204500.GO32191@localhost.localdomain>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-5-paul.elder@ideasonboard.com>\n\t<20190604104906.GG4771@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20190604104906.GG4771@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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, 04 Jun 2019 20:45:08 -0000"}}]