[{"id":1597,"web_url":"https://patchwork.libcamera.org/comment/1597/","msgid":"<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>","date":"2019-05-15T09:28:46","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for your patch, This is looking good and it's going in the\nright direction but I have a few design comments ... feel free to\ndisagree though ...\n\n\nOn 14/05/2019 23:38, Paul Elder wrote:\n> Implement a class to load a struct IPAModuleInfo of a given symbol name\n> from a .so shared object library.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - renamed LibLoader class to IPAModule\n> - added documentation\n> - added logging\n> - check that bitness of the shared object is the same as libcamera\n> - moved symbol loading (\"init\") to the constructor, and added isValid()\n> - added elfPointer() to prevent segfaults when reading data from mmap\n> - moved struct IPAModuleInfo out of IPAModule\n> - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a\n> const reference\n> - added munmap after the mmap\n> \n> src/libcamera/include/ipa_module.h | 43 +++++\n> src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 2 +\n> 3 files changed, 322 insertions(+)\n> create mode 100644 src/libcamera/include/ipa_module.h\n> create mode 100644 src/libcamera/ipa_module.cpp\n> \n> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> new file mode 100644\n> index 0000000..9eb0094\n> --- /dev/null\n> +++ b/src/libcamera/include/ipa_module.h\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_module.h - load IPA module information from shared library at runtime\n\nHrm ... we have two sides here.\n\nWe need a public header which defines the interface between an IPA\nmodule and libcamera. That would include a struct IPAModuleInfo and any\nregistration details, but should not include any internal libcamera\nprivate details regarding how the module is loaded.\n\n\n> + */\n> +#ifndef __LIBCAMERA_IPA_MODULE_H__\n> +#define __LIBCAMERA_IPA_MODULE_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +struct IPAModuleInfo {\n> +\tchar name[256];\n> +\tunsigned int version;\n> +};\n> +\n\nSo IPModuleInfo (and then later, the class definition for how a\ndeveloper would construct an IPA Module) should live in the public\nheaders at:\n\n /include/libcamera/ipa_module.h\n\n\n> +class IPAModule\n> +{\n> +public:\n> +\texplicit IPAModule(const std::string &libPath, const std::string &symbol);\n> +\n> +\tbool isValid() const;\n> +\n> +\tstruct IPAModuleInfo const &IPAModuleInfo() const;\n> +\n> +private:\n> +\tstruct IPAModuleInfo info_;\n> +\n> +\tbool loaded_;\n> +\tint bitClass_;\n> +\n> +\tint loadELFIdent(int fd);\n> +\ttemplate\n> +\tint loadSymbol(void *data, size_t size, char *map, size_t soSize,\n> +\t\t const char *symbol);\n> +\tint loadIPAModuleInfo(const char *libPath, const char *symbol);\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_MODULE_H__ */\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> new file mode 100644\n> index 0000000..5ca16e8\n> --- /dev/null\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -0,0 +1,277 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_module.cpp - load IPA module information from shared library at runtime\n> + */\n> +\n> +#include \"ipa_module.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file ipa_module.h\n> + * \\brief IPA module information loading\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\struct IPAModuleInfo\n> + * \\brief Information of an IPA plugin\n> + *\n> + * This structure contains the information of an IPA plugin. It is loaded,\n> + * read, and validated before anything else is loaded from the plugin.\n> + *\n> + * \\var IPAModuleInfo::name\n> + * \\brief The name of the IPA plugin\n> + *\n> + * \\var IPAModuleInfo::version\n> + * \\brief The version of the IPA plugin\n\nI don't think we need to store the 'version' of the plugin, so much as\nthe version of the API it was compiled against to ensure that it is\ncompatible with this 'running' instance of libcamera.\n\nI.e. We don't care that it's the 3rd iteration of the rk3399 module -\nbut we do care that it is compatible with the API defined in Libcamera 1.0.\n\nMaybe we should add compatible strings to match against pipeline\nhandlers as some point too :-) \n\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(IPAModule)\n> +\n> +template\n> +typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n> +\t\t\t\t\t\t size_t fileSize)\n> +{\n> +\tsize_t size = offset + sizeof(T);\n> +\tif (size > fileSize || size < sizeof(T))\n> +\t\treturn nullptr;\n> +\n> +\treturn reinterpret_cast::type *>\n> +\t\t(static_cast(map) + offset);\n> +}\n> +\n> +/**\n> + * \\class IPAModule\n> + * \\brief Load IPA module information from an IPA plugin\n> + */\n> +\n> +/**\n> + * \\brief Construct an IPAModule instance\n> + * \\param[in] libPath path to IPA plugin\n> + * \\param[in] symbol name of IPAModuleInfo to load from libPath\n> + *\n> + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.\n> + * The IPA plugin shared object file must be of the same endianness and\n> + * bitness as libcamera.\n> + *\n> + * Note that isValid() should be called to check if this loading was successful\n> + * or not.\n\nHrm ... this is a slightly different design pattern than the rest of our\nobjects which are always constructable, and then actions later can fail.\n\nI don't mind - but I wonder if we should add some documentation\nregarding our design patterns somewhere.\n\n(not an action on this patch)\n\n> + */\n> +\n> +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)\n> +\t: loaded_(false)\n\nSo actually, from my text below - what I'm really saying is that I don't\nthink you should provide a &symbol to this IPAModule constructor :)\n\n> +{\n> +\tint ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());\n\n\nI see that you have moved the load into this constructor based on\ncomments from Laurent in v1. I think that's fine, but I think merging\nthe loading of the object, and the parsing of the symbol might be\ncombining too much. But it depends (as ever).\n\n\nFor efficiency, the constructor can load the file once and when created\nand .isValid() is true, then we know we can successfully parse symbols.\n\n\nBut I think loadIPAModuleInfo() should be 'hardcoded' to know exactly\nwhich symbol name it will expect for an IPAModule. Think of it like a\n'main()' entry point for our IPAModule. You would always expect a C\nbinary to have a main() symbol...\n\nSo I would expect a libcamera IPA module to have a registration\nsomething like:\n\n\n/* Register our module with Libcamera */\nconst struct IPAModuleInfo ipaModuleInfo = {\n\t.name = \"Image Processing for the RK3399\",\n\t.version = 1, /* Perhaps this should be apiversion to prevent\n * people thinking it describes the version of the\n * module ? ...\n */\n};\n\nThis could even then be turned into a macro:\n\n#define LIBCAMERA_IPA_MODULE(__NAME) \\\nconst struct IPAModuleInfo ipaModuleInfo = { \\\n\t.name = __NAME,\t\\\n\t.apiversion = 1, \\\n}\n\nso that a module rk3399_ipa.cpp could define:\n\nLIBCAMERA_IPA_MODULE(\"Image Processing Algorithms for the RK3399\");\n\n(apiversion would be hardcoded by the macro because it defines what\nversion of the API/libraries it's compiled against..., and we check it's\ncompatible at runtime)\n\n\n\n> +\tif (ret < 0) {\n> +\t\tloaded_ = false;\n> +\t\treturn;\n> +\t}\n> +\n> +\tloaded_ = true;\n> +}\n> +\n> +int IPAModule::loadELFIdent(int fd)\n> +{\n> +\tint ret = lseek(fd, 0, SEEK_SET);\n> +\tif (ret < 0)\n> +\t\treturn -errno;\n> +\n> +\tunsigned char e_ident[EI_NIDENT];\n> +\tret = read(fd, e_ident, EI_NIDENT);\n> +\tif (ret < 0)\n> +\t\treturn -errno;\n> +\n> +\tif (e_ident[EI_MAG0] != ELFMAG0 ||\n> +\t e_ident[EI_MAG1] != ELFMAG1 ||\n> +\t e_ident[EI_MAG2] != ELFMAG2 ||\n> +\t e_ident[EI_MAG3] != ELFMAG3 ||\n> +\t e_ident[EI_VERSION] != EV_CURRENT)\n> +\t\treturn -ENOEXEC;\n> +\n> +\tif ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||\n> +\t (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))\n> +\t\tbitClass_ = e_ident[EI_CLASS];\n> +\telse\n> +\t\treturn -ENOEXEC;\n> +\n> +\tint a = 1;\n> +\tunsigned char endianness = *reinterpret_cast(&a) == 1\n> +\t\t\t\t ? ELFDATA2LSB : ELFDATA2MSB;\n> +\tif (e_ident[EI_DATA] != endianness)\n> +\t\treturn -ENOEXEC;\n> +\n> +\treturn 0;\n> +}\n> +\n> +template\n> +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,\n> +\t\t\t const char *symbol)\n> +{> +\tElfHeader *eHdr = elfPointer(map, 0, soSize);\n> +\tif (!eHdr)\n> +\t\treturn -ENOEXEC;\n> +\n> +\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n> +\tSecHeader *sHdr = elfPointer(map, offset, soSize);\n> +\tif (!sHdr)\n> +\t\treturn -ENOEXEC;\n> +\toff_t shnameoff = sHdr->sh_offset;\n> +\n> +\t/* Locate .dynsym section header. */\n> +\tbool found = false;\n> +\tSecHeader *dynsym;\n> +\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n> +\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n> +\t\tsHdr = elfPointer(map, offset, soSize);\n> +\t\tif (!sHdr)\n> +\t\t\treturn -ENOEXEC;\n> +\n> +\t\toffset = shnameoff + sHdr->sh_name;\n> +\t\tchar *name = elfPointer(map, offset, soSize);\n> +\t\tif (!name)\n> +\t\t\treturn -ENOEXEC;\n> +\n> +\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n> +\t\t\tfound = true;\n> +\t\t\tdynsym = sHdr;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!found) {\n> +\t\tLOG(IPAModule, Error) << \"ELF has no .dynsym section\";\n> +\t\treturn -ENOEXEC;\n> +\t}\n> +\n> +\toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n> +\tsHdr = elfPointer(map, offset, soSize);\n> +\tif (!sHdr)\n> +\t\treturn -ENOEXEC;\n> +\toff_t dynsym_nameoff = sHdr->sh_offset;\n> +\n> +\t/* Locate symbol in the .dynsym section. */\n> +\tfound = false;\n> +\tSymHeader *target_symbol;\n> +\tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n> +\tfor (unsigned int i = 0; i < dynsym_num; i++) {\n> +\t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n> +\t\tSymHeader *sym = elfPointer(map, offset, soSize);\n> +\t\tif (!sym)\n> +\t\t\treturn -ENOEXEC;\n> +\n> +\t\toffset = dynsym_nameoff + sym->st_name;\n> +\t\tchar *name = elfPointer(map, offset, soSize);\n> +\t\tif (!name)\n> +\t\t\treturn -ENOEXEC;\n> +\n> +\t\tif (!strcmp(name, symbol) &&\n> +\t\t sym->st_info & STB_GLOBAL &&\n> +\t\t sym->st_size == sizeof(struct IPAModuleInfo)) {\n\nI think this should be a check on the size_t size passed in?\n\n> +\t\t\tfound = true;\n> +\t\t\ttarget_symbol = sym;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!found) {\n> +\t\tLOG(IPAModule, Error) << \"IPAModuleInfo symbol not found\";\n\nThis function is called loadSymbol, it doesn't 'know' that it's loading\nan IPAModuleInfo symbol.\n\nI'd change this error to \"Symbol \" << symbol << \" not found\";\n\n> +\t\treturn -ENOEXEC;\n> +\t}\n> +\n> +\t/* Locate and return data of symbol. */\n> +\toffset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;\n> +\tsHdr = elfPointer(map, offset, soSize);\n> +\tif (!sHdr)\n> +\t\treturn -ENOEXEC;\n> +\toffset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);\n> +\tchar *data = elfPointer(map, offset, soSize);\n> +\tif (!data)\n> +\t\treturn -ENOEXEC;\n> +\tmemcpy(dst, data, size);\n\nOh - interesting, you're copying the symbol out. Once it's mmapped ...\nwhy not parse it directly?\n\nCan we do all of the 'work' we need to do on the file, and then close it\nat the destructor?\n\nI guess actually this might then keep the whole file mapped in memory\nfor longer which might not be desirable...\n\n\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)\n> +{\n> +\tint fd = open(libPath, O_RDONLY);\n> +\tif (fd < 0) {\n> +\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> +\t\t\t\t << strerror(errno);\n> +\t\treturn fd;\n> +\t}\n> +\n> +\tint ret = loadELFIdent(fd);\n> +\tif (ret)\n> +\t\tgoto close;\n> +\n> +\tsize_t soSize;\n> +\tchar *map;\n> +\tstruct stat st;\n> +\tret = fstat(fd, &st);\n> +\tif (ret < 0)\n> +\t\tgoto close;\n> +\tsoSize = st.st_size;\n> +\tmap = static_cast(mmap(NULL, soSize, PROT_READ,\n> +\t\t\t\t MAP_PRIVATE, fd, 0));\n\n*iff* the constructor did the open, and the symbol parsing was\nseparated, then I think soSize and map would go to class privates. Then\nthey could be accessed directly by loadSymbol too. But that's an only if...\n\nEssentially, it would be\n\nIPAModule(\"SO-path\")\n - Constructor opens the file, performs stat, and mmap.\n\t (or those become an open() call)\n - loadIPAModuleInfo() calls loadSymbol(\"ipaModuleInfo\"); and deals only\nwith the ipaModule info structure and checking or such.\n\n\n\n\n> +\tif (map == MAP_FAILED) {\n> +\t\tret = -errno;\n> +\t\tgoto close;\n> +\t}\n> +\n> +\tif (bitClass_ == ELFCLASS32)\n> +\t\tret = loadSymbol\n> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> +\telse if (bitClass_ == ELFCLASS64)\n> +\t\tret = loadSymbol\n> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> +\tif (ret)\n> +\t\tgoto unmap;\n> +\n> +unmap:\n> +\tmunmap(map, soSize);\n> +close:\n> +\tclose(fd);\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\brief Check if construction of the IPAModule instance succeeded\n> + *\n> + * \\return true if the constructor succeeded with no errors, false otherwise\n> + */\n> +\n> +bool IPAModule::isValid() const\n> +{\n> +\treturn loaded_;\n> +}\n> +\n> +/**\n> + * \\brief Get the loaded IPAModuleInfo\n> + *\n> + * Check isValid() before calling this.\n> + *\n> + * \\return IPAModuleInfo\n> + */\n> +\n> +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const\n> +{\n> +\treturn info_;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 8796f49..e5b48f2 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -10,6 +10,7 @@ libcamera_sources = files([\n> 'event_notifier.cpp',\n> 'formats.cpp',\n> 'geometry.cpp',\n> + 'ipa_module.cpp',\n> 'log.cpp',\n> 'media_device.cpp',\n> 'media_object.cpp',\n> @@ -31,6 +32,7 @@ libcamera_headers = files([\n> 'include/device_enumerator_udev.h',\n> 'include/event_dispatcher_poll.h',\n> 'include/formats.h',\n> + 'include/ipa_module.h',\n> 'include/log.h',\n> 'include/media_device.h',\n> 'include/media_object.h',\n>","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 643FC60E4A\n\tfor ;\n\tWed, 15 May 2019 11:28:50 +0200 (CEST)","from [IPv6:2a02:c7f:1887:5d00:b4d8:e934:94c9:8ec0] (unknown\n\t[IPv6:2a02:c7f:1887:5d00:b4d8:e934:94c9:8ec0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93151E2;\n\tWed, 15 May 2019 11:28:49 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557912529;\n\tbh=FBH2lYZaLAwn5wLqv9bGyxHu9EsmiGX9XG9EtBUr6xY=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=wKizP9/dwE20vO7vf8wWhGVm87n9Aaymh2D6uMhvgdLYRdL3UpR2o13Y0pKoPDoqU\n\th2YuI1PROBBhbs/Y7DeqZfDo/VvkKmlE3Vl75adFhK+Ifm90b6BxLS3zw6Y7RuFq8F\n\t5CRZnjsDzdbv0j1e/WmGhdg9vPgGgH+KTfogpyUE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190514223808.27351-1-paul.elder@ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>","Date":"Wed, 15 May 2019 10:28:46 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.6.1","MIME-Version":"1.0","In-Reply-To":"<20190514223808.27351-1-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","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":"Wed, 15 May 2019 09:28:50 -0000"}},{"id":1598,"web_url":"https://patchwork.libcamera.org/comment/1598/","msgid":"<20190515150253.GF12325@localhost.localdomain>","date":"2019-05-15T15:02:53","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, May 15, 2019 at 10:28:46AM +0100, Kieran Bingham wrote:\n> Hi Paul,\n> \n> Thank you for your patch, This is looking good and it's going in the\n> right direction but I have a few design comments ... feel free to\n> disagree though ...\n\nThank you for the review.\n\n> On 14/05/2019 23:38, Paul Elder wrote:\n> > Implement a class to load a struct IPAModuleInfo of a given symbol name\n> > from a .so shared object library.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > Changes in v2:\n> > - renamed LibLoader class to IPAModule\n> > - added documentation\n> > - added logging\n> > - check that bitness of the shared object is the same as libcamera\n> > - moved symbol loading (\"init\") to the constructor, and added isValid()\n> > - added elfPointer() to prevent segfaults when reading data from mmap\n> > - moved struct IPAModuleInfo out of IPAModule\n> > - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a\n> > const reference\n> > - added munmap after the mmap\n> > \n> > src/libcamera/include/ipa_module.h | 43 +++++\n> > src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 2 +\n> > 3 files changed, 322 insertions(+)\n> > create mode 100644 src/libcamera/include/ipa_module.h\n> > create mode 100644 src/libcamera/ipa_module.cpp\n> > \n> > diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> > new file mode 100644\n> > index 0000000..9eb0094\n> > --- /dev/null\n> > +++ b/src/libcamera/include/ipa_module.h\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * ipa_module.h - load IPA module information from shared library at runtime\n> \n> Hrm ... we have two sides here.\n> \n> We need a public header which defines the interface between an IPA\n> module and libcamera. That would include a struct IPAModuleInfo and any\n> registration details, but should not include any internal libcamera\n> private details regarding how the module is loaded.\n\nYes, I agree.\n\n> > + */\n> > +#ifndef __LIBCAMERA_IPA_MODULE_H__\n> > +#define __LIBCAMERA_IPA_MODULE_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +struct IPAModuleInfo {\n> > +\tchar name[256];\n> > +\tunsigned int version;\n> > +};\n> > +\n> \n> So IPModuleInfo (and then later, the class definition for how a\n> developer would construct an IPA Module) should live in the public\n> headers at:\n> \n> /include/libcamera/ipa_module.h\n\nYeah.\n\nThen, where should class IPAModule go?\n\n> > +class IPAModule\n> > +{\n> > +public:\n> > +\texplicit IPAModule(const std::string &libPath, const std::string &symbol);\n> > +\n> > +\tbool isValid() const;\n> > +\n> > +\tstruct IPAModuleInfo const &IPAModuleInfo() const;\n> > +\n> > +private:\n> > +\tstruct IPAModuleInfo info_;\n> > +\n> > +\tbool loaded_;\n> > +\tint bitClass_;\n> > +\n> > +\tint loadELFIdent(int fd);\n> > +\ttemplate\n> > +\tint loadSymbol(void *data, size_t size, char *map, size_t soSize,\n> > +\t\t const char *symbol);\n> > +\tint loadIPAModuleInfo(const char *libPath, const char *symbol);\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPA_MODULE_H__ */\n> > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> > new file mode 100644\n> > index 0000000..5ca16e8\n> > --- /dev/null\n> > +++ b/src/libcamera/ipa_module.cpp\n> > @@ -0,0 +1,277 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * ipa_module.cpp - load IPA module information from shared library at runtime\n> > + */\n> > +\n> > +#include \"ipa_module.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +\n> > +/**\n> > + * \\file ipa_module.h\n> > + * \\brief IPA module information loading\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\struct IPAModuleInfo\n> > + * \\brief Information of an IPA plugin\n> > + *\n> > + * This structure contains the information of an IPA plugin. It is loaded,\n> > + * read, and validated before anything else is loaded from the plugin.\n> > + *\n> > + * \\var IPAModuleInfo::name\n> > + * \\brief The name of the IPA plugin\n> > + *\n> > + * \\var IPAModuleInfo::version\n> > + * \\brief The version of the IPA plugin\n> \n> I don't think we need to store the 'version' of the plugin, so much as\n> the version of the API it was compiled against to ensure that it is\n> compatible with this 'running' instance of libcamera.\n> \n> I.e. We don't care that it's the 3rd iteration of the rk3399 module -\n> but we do care that it is compatible with the API defined in Libcamera 1.0.\n\nYeah, good point; I agree.\n\nHow would we get the API version of libcamera?\n\n> Maybe we should add compatible strings to match against pipeline\n> handlers as some point too :-) \n> \n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(IPAModule)\n> > +\n> > +template\n> > +typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n> > +\t\t\t\t\t\t size_t fileSize)\n> > +{\n> > +\tsize_t size = offset + sizeof(T);\n> > +\tif (size > fileSize || size < sizeof(T))\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn reinterpret_cast::type *>\n> > +\t\t(static_cast(map) + offset);\n> > +}\n> > +\n> > +/**\n> > + * \\class IPAModule\n> > + * \\brief Load IPA module information from an IPA plugin\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct an IPAModule instance\n> > + * \\param[in] libPath path to IPA plugin\n> > + * \\param[in] symbol name of IPAModuleInfo to load from libPath\n> > + *\n> > + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.\n> > + * The IPA plugin shared object file must be of the same endianness and\n> > + * bitness as libcamera.\n> > + *\n> > + * Note that isValid() should be called to check if this loading was successful\n> > + * or not.\n> \n> Hrm ... this is a slightly different design pattern than the rest of our\n> objects which are always constructable, and then actions later can fail.\n> \n> I don't mind - but I wonder if we should add some documentation\n> regarding our design patterns somewhere.\n> \n> (not an action on this patch)\n> \n> > + */\n> > +\n> > +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)\n> > +\t: loaded_(false)\n> \n> So actually, from my text below - what I'm really saying is that I don't\n> think you should provide a &symbol to this IPAModule constructor :)\n> \n> > +{\n> > +\tint ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());\n> \n> \n> I see that you have moved the load into this constructor based on\n> comments from Laurent in v1. I think that's fine, but I think merging\n> the loading of the object, and the parsing of the symbol might be\n> combining too much. But it depends (as ever).\n> \n> \n> For efficiency, the constructor can load the file once and when created\n> and .isValid() is true, then we know we can successfully parse symbols.\n> \n> \n> But I think loadIPAModuleInfo() should be 'hardcoded' to know exactly\n> which symbol name it will expect for an IPAModule. Think of it like a\n> 'main()' entry point for our IPAModule. You would always expect a C\n> binary to have a main() symbol...\n\nYeah, I think that's a good idea.\n\n> So I would expect a libcamera IPA module to have a registration\n> something like:\n> \n> \n> /* Register our module with Libcamera */\n> const struct IPAModuleInfo ipaModuleInfo = {\n> \t.name = \"Image Processing for the RK3399\",\n> \t.version = 1, /* Perhaps this should be apiversion to prevent\n> * people thinking it describes the version of the\n> * module ? ...\n> */\n> };\n\nShould the info entry point be called ipaModuleInfo?\n\n> This could even then be turned into a macro:\n> \n> #define LIBCAMERA_IPA_MODULE(__NAME) \\\n> const struct IPAModuleInfo ipaModuleInfo = { \\\n> \t.name = __NAME,\t\\\n> \t.apiversion = 1, \\\n> }\n> \n> so that a module rk3399_ipa.cpp could define:\n> \n> LIBCAMERA_IPA_MODULE(\"Image Processing Algorithms for the RK3399\");\n> \n> (apiversion would be hardcoded by the macro because it defines what\n> version of the API/libraries it's compiled against..., and we check it's\n> compatible at runtime)\n> \n> \n> \n> > +\tif (ret < 0) {\n> > +\t\tloaded_ = false;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tloaded_ = true;\n> > +}\n> > +\n> > +int IPAModule::loadELFIdent(int fd)\n> > +{\n> > +\tint ret = lseek(fd, 0, SEEK_SET);\n> > +\tif (ret < 0)\n> > +\t\treturn -errno;\n> > +\n> > +\tunsigned char e_ident[EI_NIDENT];\n> > +\tret = read(fd, e_ident, EI_NIDENT);\n> > +\tif (ret < 0)\n> > +\t\treturn -errno;\n> > +\n> > +\tif (e_ident[EI_MAG0] != ELFMAG0 ||\n> > +\t e_ident[EI_MAG1] != ELFMAG1 ||\n> > +\t e_ident[EI_MAG2] != ELFMAG2 ||\n> > +\t e_ident[EI_MAG3] != ELFMAG3 ||\n> > +\t e_ident[EI_VERSION] != EV_CURRENT)\n> > +\t\treturn -ENOEXEC;\n> > +\n> > +\tif ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||\n> > +\t (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))\n> > +\t\tbitClass_ = e_ident[EI_CLASS];\n> > +\telse\n> > +\t\treturn -ENOEXEC;\n> > +\n> > +\tint a = 1;\n> > +\tunsigned char endianness = *reinterpret_cast(&a) == 1\n> > +\t\t\t\t ? ELFDATA2LSB : ELFDATA2MSB;\n> > +\tif (e_ident[EI_DATA] != endianness)\n> > +\t\treturn -ENOEXEC;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +template\n> > +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,\n> > +\t\t\t const char *symbol)\n> > +{> +\tElfHeader *eHdr = elfPointer(map, 0, soSize);\n> > +\tif (!eHdr)\n> > +\t\treturn -ENOEXEC;\n> > +\n> > +\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n> > +\tSecHeader *sHdr = elfPointer(map, offset, soSize);\n> > +\tif (!sHdr)\n> > +\t\treturn -ENOEXEC;\n> > +\toff_t shnameoff = sHdr->sh_offset;\n> > +\n> > +\t/* Locate .dynsym section header. */\n> > +\tbool found = false;\n> > +\tSecHeader *dynsym;\n> > +\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n> > +\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n> > +\t\tsHdr = elfPointer(map, offset, soSize);\n> > +\t\tif (!sHdr)\n> > +\t\t\treturn -ENOEXEC;\n> > +\n> > +\t\toffset = shnameoff + sHdr->sh_name;\n> > +\t\tchar *name = elfPointer(map, offset, soSize);\n> > +\t\tif (!name)\n> > +\t\t\treturn -ENOEXEC;\n> > +\n> > +\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n> > +\t\t\tfound = true;\n> > +\t\t\tdynsym = sHdr;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!found) {\n> > +\t\tLOG(IPAModule, Error) << \"ELF has no .dynsym section\";\n> > +\t\treturn -ENOEXEC;\n> > +\t}\n> > +\n> > +\toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n> > +\tsHdr = elfPointer(map, offset, soSize);\n> > +\tif (!sHdr)\n> > +\t\treturn -ENOEXEC;\n> > +\toff_t dynsym_nameoff = sHdr->sh_offset;\n> > +\n> > +\t/* Locate symbol in the .dynsym section. */\n> > +\tfound = false;\n> > +\tSymHeader *target_symbol;\n> > +\tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n> > +\tfor (unsigned int i = 0; i < dynsym_num; i++) {\n> > +\t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n> > +\t\tSymHeader *sym = elfPointer(map, offset, soSize);\n> > +\t\tif (!sym)\n> > +\t\t\treturn -ENOEXEC;\n> > +\n> > +\t\toffset = dynsym_nameoff + sym->st_name;\n> > +\t\tchar *name = elfPointer(map, offset, soSize);\n> > +\t\tif (!name)\n> > +\t\t\treturn -ENOEXEC;\n> > +\n> > +\t\tif (!strcmp(name, symbol) &&\n> > +\t\t sym->st_info & STB_GLOBAL &&\n> > +\t\t sym->st_size == sizeof(struct IPAModuleInfo)) {\n> \n> I think this should be a check on the size_t size passed in?\n\nAh yeah, I totally missed that.\n\n> > +\t\t\tfound = true;\n> > +\t\t\ttarget_symbol = sym;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!found) {\n> > +\t\tLOG(IPAModule, Error) << \"IPAModuleInfo symbol not found\";\n> \n> This function is called loadSymbol, it doesn't 'know' that it's loading\n> an IPAModuleInfo symbol.\n> \n> I'd change this error to \"Symbol \" << symbol << \" not found\";\n\nAnd this.\n\n> > +\t\treturn -ENOEXEC;\n> > +\t}\n> > +\n> > +\t/* Locate and return data of symbol. */\n> > +\toffset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;\n> > +\tsHdr = elfPointer(map, offset, soSize);\n> > +\tif (!sHdr)\n> > +\t\treturn -ENOEXEC;\n> > +\toffset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);\n> > +\tchar *data = elfPointer(map, offset, soSize);\n> > +\tif (!data)\n> > +\t\treturn -ENOEXEC;\n> > +\tmemcpy(dst, data, size);\n> \n> Oh - interesting, you're copying the symbol out. Once it's mmapped ...\n> why not parse it directly?\n\nI'm not sure what you mean by \"parse\"?\n\n> Can we do all of the 'work' we need to do on the file, and then close it\n> at the destructor?\n\nI don't want to keep the file open/mapped over multiple method calls.\nThat's why in my first version there was a constructor that did nothing\nand an init() that did everything (which got moved to the constructor in\nthis version).\n\n> I guess actually this might then keep the whole file mapped in memory\n> for longer which might not be desirable...\n> \n> \n> \n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)\n> > +{\n> > +\tint fd = open(libPath, O_RDONLY);\n> > +\tif (fd < 0) {\n> > +\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> > +\t\t\t\t << strerror(errno);\n> > +\t\treturn fd;\n> > +\t}\n> > +\n> > +\tint ret = loadELFIdent(fd);\n> > +\tif (ret)\n> > +\t\tgoto close;\n> > +\n> > +\tsize_t soSize;\n> > +\tchar *map;\n> > +\tstruct stat st;\n> > +\tret = fstat(fd, &st);\n> > +\tif (ret < 0)\n> > +\t\tgoto close;\n> > +\tsoSize = st.st_size;\n> > +\tmap = static_cast(mmap(NULL, soSize, PROT_READ,\n> > +\t\t\t\t MAP_PRIVATE, fd, 0));\n> \n> *iff* the constructor did the open, and the symbol parsing was\n> separated, then I think soSize and map would go to class privates. Then\n> they could be accessed directly by loadSymbol too. But that's an only if...\n> \n> Essentially, it would be\n> \n> IPAModule(\"SO-path\")\n> - Constructor opens the file, performs stat, and mmap.\n> \t (or those become an open() call)\n> - loadIPAModuleInfo() calls loadSymbol(\"ipaModuleInfo\"); and deals only\n> with the ipaModule info structure and checking or such.\n\nIf loadIPAModuleInfo() is to be called from within the constructor\n(after open, stat, and mmap), then I agree. I'm not sure map and soSize\nneed to be privates though. It could go either way.\n\n> \n> > +\tif (map == MAP_FAILED) {\n> > +\t\tret = -errno;\n> > +\t\tgoto close;\n> > +\t}\n> > +\n> > +\tif (bitClass_ == ELFCLASS32)\n> > +\t\tret = loadSymbol\n> > +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> > +\telse if (bitClass_ == ELFCLASS64)\n> > +\t\tret = loadSymbol\n> > +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> > +\tif (ret)\n> > +\t\tgoto unmap;\n> > +\n> > +unmap:\n> > +\tmunmap(map, soSize);\n> > +close:\n> > +\tclose(fd);\n> > +\treturn ret;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Check if construction of the IPAModule instance succeeded\n> > + *\n> > + * \\return true if the constructor succeeded with no errors, false otherwise\n> > + */\n> > +\n> > +bool IPAModule::isValid() const\n> > +{\n> > +\treturn loaded_;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get the loaded IPAModuleInfo\n> > + *\n> > + * Check isValid() before calling this.\n> > + *\n> > + * \\return IPAModuleInfo\n> > + */\n> > +\n> > +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const\n> > +{\n> > +\treturn info_;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 8796f49..e5b48f2 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -10,6 +10,7 @@ libcamera_sources = files([\n> > 'event_notifier.cpp',\n> > 'formats.cpp',\n> > 'geometry.cpp',\n> > + 'ipa_module.cpp',\n> > 'log.cpp',\n> > 'media_device.cpp',\n> > 'media_object.cpp',\n> > @@ -31,6 +32,7 @@ libcamera_headers = files([\n> > 'include/device_enumerator_udev.h',\n> > 'include/event_dispatcher_poll.h',\n> > 'include/formats.h',\n> > + 'include/ipa_module.h',\n> > 'include/log.h',\n> > 'include/media_device.h',\n> > 'include/media_object.h',\n> > \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 7792A60103\n\tfor ;\n\tWed, 15 May 2019 17:03:01 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A24DE2;\n\tWed, 15 May 2019 17:03:00 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557932581;\n\tbh=BrOuaaAgWNB9fu9AJAKRo+hk6smNbqHADXVmMWzSjFM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sWEk6alu3ycHngRSw2RMN0t189Hpw/918GpwIKciTfX44daZY9hRe/ocLOQpVn+yk\n\tqnoDV7tyPGkw73RLuP3cPSEBO3rn0hhg8yDZodzNtz3lzziYgUEjURdeJqf4Ml/wOZ\n\tTDHnkfEWICzS/S1UQrFHIap65AHGQ1YvL0mP5up8=","Date":"Wed, 15 May 2019 11:02:53 -0400","From":"Paul Elder ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190515150253.GF12325@localhost.localdomain>","References":"<20190514223808.27351-1-paul.elder@ideasonboard.com>\n\t<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","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":"Wed, 15 May 2019 15:03:01 -0000"}},{"id":1599,"web_url":"https://patchwork.libcamera.org/comment/1599/","msgid":"<27f62fbf-1f80-af57-10c9-75f94d87487f@ideasonboard.com>","date":"2019-05-15T15:26:48","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Paul,\n\nOn 15/05/2019 16:02, Paul Elder wrote:\n> Hi Kieran,\n> \n> On Wed, May 15, 2019 at 10:28:46AM +0100, Kieran Bingham wrote:\n>> Hi Paul,\n>>\n>> Thank you for your patch, This is looking good and it's going in the\n>> right direction but I have a few design comments ... feel free to\n>> disagree though ...\n> \n> Thank you for the review.\n> \n>> On 14/05/2019 23:38, Paul Elder wrote:\n>>> Implement a class to load a struct IPAModuleInfo of a given symbol name\n>>> from a .so shared object library.\n>>>\n>>> Signed-off-by: Paul Elder \n>>> ---\n>>> Changes in v2:\n>>> - renamed LibLoader class to IPAModule\n>>> - added documentation\n>>> - added logging\n>>> - check that bitness of the shared object is the same as libcamera\n>>> - moved symbol loading (\"init\") to the constructor, and added isValid()\n>>> - added elfPointer() to prevent segfaults when reading data from mmap\n>>> - moved struct IPAModuleInfo out of IPAModule\n>>> - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a\n>>> const reference\n>>> - added munmap after the mmap\n>>>\n>>> src/libcamera/include/ipa_module.h | 43 +++++\n>>> src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++\n>>> src/libcamera/meson.build | 2 +\n>>> 3 files changed, 322 insertions(+)\n>>> create mode 100644 src/libcamera/include/ipa_module.h\n>>> create mode 100644 src/libcamera/ipa_module.cpp\n>>>\n>>> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n>>> new file mode 100644\n>>> index 0000000..9eb0094\n>>> --- /dev/null\n>>> +++ b/src/libcamera/include/ipa_module.h\n>>> @@ -0,0 +1,43 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2019, Google Inc.\n>>> + *\n>>> + * ipa_module.h - load IPA module information from shared library at runtime\n>>\n>> Hrm ... we have two sides here.\n>>\n>> We need a public header which defines the interface between an IPA\n>> module and libcamera. That would include a struct IPAModuleInfo and any\n>> registration details, but should not include any internal libcamera\n>> private details regarding how the module is loaded.\n> \n> Yes, I agree.\n> \n>>> + */\n>>> +#ifndef __LIBCAMERA_IPA_MODULE_H__\n>>> +#define __LIBCAMERA_IPA_MODULE_H__\n>>> +\n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +struct IPAModuleInfo {\n>>> +\tchar name[256];\n>>> +\tunsigned int version;\n>>> +};\n>>> +\n>>\n>> So IPModuleInfo (and then later, the class definition for how a\n>> developer would construct an IPA Module) should live in the public\n>> headers at:\n>>\n>> /include/libcamera/ipa_module.h\n> \n> Yeah.\n> \n> Then, where should class IPAModule go?\n\nStays here in this file I think :-)\n\n> \n>>> +class IPAModule\n\nI'm wondering if we have two sides if this class should be calls\nIPAModule though.\n\nSurely an IPAModule would implement a class called IPAModule, and\nperhaps 'this' class is an IPAModuleParser? IPAModuleLoader?\n\nProbably a Loader :D\n\n>>> +{\n>>> +public:\n>>> +\texplicit IPAModule(const std::string &libPath, const std::string &symbol);\n>>> +\n>>> +\tbool isValid() const;\n>>> +\n>>> +\tstruct IPAModuleInfo const &IPAModuleInfo() const;\n>>> +\n>>> +private:\n>>> +\tstruct IPAModuleInfo info_;\n>>> +\n>>> +\tbool loaded_;\n>>> +\tint bitClass_;\n>>> +\n>>> +\tint loadELFIdent(int fd);\n>>> +\ttemplate\n>>> +\tint loadSymbol(void *data, size_t size, char *map, size_t soSize,\n>>> +\t\t const char *symbol);\n>>> +\tint loadIPAModuleInfo(const char *libPath, const char *symbol);\n>>> +};\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_IPA_MODULE_H__ */\n>>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n>>> new file mode 100644\n>>> index 0000000..5ca16e8\n>>> --- /dev/null\n>>> +++ b/src/libcamera/ipa_module.cpp\n>>> @@ -0,0 +1,277 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2019, Google Inc.\n>>> + *\n>>> + * ipa_module.cpp - load IPA module information from shared library at runtime\n>>> + */\n>>> +\n>>> +#include \"ipa_module.h\"\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +#include \"log.h\"\n>>> +\n>>> +/**\n>>> + * \\file ipa_module.h\n>>> + * \\brief IPA module information loading\n>>> + */\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +/**\n>>> + * \\struct IPAModuleInfo\n>>> + * \\brief Information of an IPA plugin\n>>> + *\n>>> + * This structure contains the information of an IPA plugin. It is loaded,\n>>> + * read, and validated before anything else is loaded from the plugin.\n>>> + *\n>>> + * \\var IPAModuleInfo::name\n>>> + * \\brief The name of the IPA plugin\n>>> + *\n>>> + * \\var IPAModuleInfo::version\n>>> + * \\brief The version of the IPA plugin\n>>\n>> I don't think we need to store the 'version' of the plugin, so much as\n>> the version of the API it was compiled against to ensure that it is\n>> compatible with this 'running' instance of libcamera.\n>>\n>> I.e. We don't care that it's the 3rd iteration of the rk3399 module -\n>> but we do care that it is compatible with the API defined in Libcamera 1.0.\n> \n> Yeah, good point; I agree.\n> \n> How would we get the API version of libcamera?\n\nYou get to define it :-)\n\nTo start with it's probably just:\n\n #define LIBCAMERA_IPA_API_VERSION 1\n\nThis would be a 'local' IPAModule API version number or such I think, as\nour IPA modules must go through a defined interface...\n\nAny time an ABI break occurs in the IPAModule 'API' we would have to\nbump up the apiversion.\n\n\n\n>> Maybe we should add compatible strings to match against pipeline\n>> handlers as some point too :-) \n>>\n>>> + */\n>>> +\n>>> +LOG_DEFINE_CATEGORY(IPAModule)\n>>> +\n>>> +template\n>>> +typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n>>> +\t\t\t\t\t\t size_t fileSize)\n>>> +{\n>>> +\tsize_t size = offset + sizeof(T);\n>>> +\tif (size > fileSize || size < sizeof(T))\n>>> +\t\treturn nullptr;\n>>> +\n>>> +\treturn reinterpret_cast::type *>\n>>> +\t\t(static_cast(map) + offset);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\class IPAModule\n>>> + * \\brief Load IPA module information from an IPA plugin\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct an IPAModule instance\n>>> + * \\param[in] libPath path to IPA plugin\n>>> + * \\param[in] symbol name of IPAModuleInfo to load from libPath\n>>> + *\n>>> + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.\n>>> + * The IPA plugin shared object file must be of the same endianness and\n>>> + * bitness as libcamera.\n>>> + *\n>>> + * Note that isValid() should be called to check if this loading was successful\n>>> + * or not.\n>>\n>> Hrm ... this is a slightly different design pattern than the rest of our\n>> objects which are always constructable, and then actions later can fail.\n>>\n>> I don't mind - but I wonder if we should add some documentation\n>> regarding our design patterns somewhere.\n>>\n>> (not an action on this patch)\n>>\n>>> + */\n>>> +\n>>> +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)\n>>> +\t: loaded_(false)\n>>\n>> So actually, from my text below - what I'm really saying is that I don't\n>> think you should provide a &symbol to this IPAModule constructor :)\n>>\n>>> +{\n>>> +\tint ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());\n>>\n>>\n>> I see that you have moved the load into this constructor based on\n>> comments from Laurent in v1. I think that's fine, but I think merging\n>> the loading of the object, and the parsing of the symbol might be\n>> combining too much. But it depends (as ever).\n>>\n>>\n>> For efficiency, the constructor can load the file once and when created\n>> and .isValid() is true, then we know we can successfully parse symbols.\n>>\n>>\n>> But I think loadIPAModuleInfo() should be 'hardcoded' to know exactly\n>> which symbol name it will expect for an IPAModule. Think of it like a\n>> 'main()' entry point for our IPAModule. You would always expect a C\n>> binary to have a main() symbol...\n> \n> Yeah, I think that's a good idea.\n> \n>> So I would expect a libcamera IPA module to have a registration\n>> something like:\n>>\n>>\n>> /* Register our module with Libcamera */\n>> const struct IPAModuleInfo ipaModuleInfo = {\n>> \t.name = \"Image Processing for the RK3399\",\n>> \t.version = 1, /* Perhaps this should be apiversion to prevent\n>> * people thinking it describes the version of the\n>> * module ? ...\n>> */\n>> };\n> \n> Should the info entry point be called ipaModuleInfo?\n\nI think variables are in camelCase with a lowerCase first letter in the\nproject right? or was this not quite your question?\n\n\n> \n>> This could even then be turned into a macro:\n>>\n>> #define LIBCAMERA_IPA_MODULE(__NAME) \\\n>> const struct IPAModuleInfo ipaModuleInfo = { \\\n>> \t.name = __NAME,\t\\\n>> \t.apiversion = 1, \\\n>> }\n>>\n>> so that a module rk3399_ipa.cpp could define:\n>>\n>> LIBCAMERA_IPA_MODULE(\"Image Processing Algorithms for the RK3399\");\n>>\n>> (apiversion would be hardcoded by the macro because it defines what\n>> version of the API/libraries it's compiled against..., and we check it's\n>> compatible at runtime)\n>>\n>>\n>>\n>>> +\tif (ret < 0) {\n>>> +\t\tloaded_ = false;\n>>> +\t\treturn;\n>>> +\t}\n>>> +\n>>> +\tloaded_ = true;\n>>> +}\n>>> +\n>>> +int IPAModule::loadELFIdent(int fd)\n>>> +{\n>>> +\tint ret = lseek(fd, 0, SEEK_SET);\n>>> +\tif (ret < 0)\n>>> +\t\treturn -errno;\n>>> +\n>>> +\tunsigned char e_ident[EI_NIDENT];\n>>> +\tret = read(fd, e_ident, EI_NIDENT);\n>>> +\tif (ret < 0)\n>>> +\t\treturn -errno;\n>>> +\n>>> +\tif (e_ident[EI_MAG0] != ELFMAG0 ||\n>>> +\t e_ident[EI_MAG1] != ELFMAG1 ||\n>>> +\t e_ident[EI_MAG2] != ELFMAG2 ||\n>>> +\t e_ident[EI_MAG3] != ELFMAG3 ||\n>>> +\t e_ident[EI_VERSION] != EV_CURRENT)\n>>> +\t\treturn -ENOEXEC;\n>>> +\n>>> +\tif ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||\n>>> +\t (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))\n>>> +\t\tbitClass_ = e_ident[EI_CLASS];\n>>> +\telse\n>>> +\t\treturn -ENOEXEC;\n>>> +\n>>> +\tint a = 1;\n>>> +\tunsigned char endianness = *reinterpret_cast(&a) == 1\n>>> +\t\t\t\t ? ELFDATA2LSB : ELFDATA2MSB;\n>>> +\tif (e_ident[EI_DATA] != endianness)\n>>> +\t\treturn -ENOEXEC;\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +template\n>>> +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,\n>>> +\t\t\t const char *symbol)\n>>> +{> +\tElfHeader *eHdr = elfPointer(map, 0, soSize);\n>>> +\tif (!eHdr)\n>>> +\t\treturn -ENOEXEC;\n>>> +\n>>> +\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n>>> +\tSecHeader *sHdr = elfPointer(map, offset, soSize);\n>>> +\tif (!sHdr)\n>>> +\t\treturn -ENOEXEC;\n>>> +\toff_t shnameoff = sHdr->sh_offset;\n>>> +\n>>> +\t/* Locate .dynsym section header. */\n>>> +\tbool found = false;\n>>> +\tSecHeader *dynsym;\n>>> +\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n>>> +\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n>>> +\t\tsHdr = elfPointer(map, offset, soSize);\n>>> +\t\tif (!sHdr)\n>>> +\t\t\treturn -ENOEXEC;\n>>> +\n>>> +\t\toffset = shnameoff + sHdr->sh_name;\n>>> +\t\tchar *name = elfPointer(map, offset, soSize);\n>>> +\t\tif (!name)\n>>> +\t\t\treturn -ENOEXEC;\n>>> +\n>>> +\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n>>> +\t\t\tfound = true;\n>>> +\t\t\tdynsym = sHdr;\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\tif (!found) {\n>>> +\t\tLOG(IPAModule, Error) << \"ELF has no .dynsym section\";\n>>> +\t\treturn -ENOEXEC;\n>>> +\t}\n>>> +\n>>> +\toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n>>> +\tsHdr = elfPointer(map, offset, soSize);\n>>> +\tif (!sHdr)\n>>> +\t\treturn -ENOEXEC;\n>>> +\toff_t dynsym_nameoff = sHdr->sh_offset;\n>>> +\n>>> +\t/* Locate symbol in the .dynsym section. */\n>>> +\tfound = false;\n>>> +\tSymHeader *target_symbol;\n>>> +\tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n>>> +\tfor (unsigned int i = 0; i < dynsym_num; i++) {\n>>> +\t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n>>> +\t\tSymHeader *sym = elfPointer(map, offset, soSize);\n>>> +\t\tif (!sym)\n>>> +\t\t\treturn -ENOEXEC;\n>>> +\n>>> +\t\toffset = dynsym_nameoff + sym->st_name;\n>>> +\t\tchar *name = elfPointer(map, offset, soSize);\n>>> +\t\tif (!name)\n>>> +\t\t\treturn -ENOEXEC;\n>>> +\n>>> +\t\tif (!strcmp(name, symbol) &&\n>>> +\t\t sym->st_info & STB_GLOBAL &&\n>>> +\t\t sym->st_size == sizeof(struct IPAModuleInfo)) {\n>>\n>> I think this should be a check on the size_t size passed in?\n> \n> Ah yeah, I totally missed that.\n\nNo worries.\n\n> \n>>> +\t\t\tfound = true;\n>>> +\t\t\ttarget_symbol = sym;\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\tif (!found) {\n>>> +\t\tLOG(IPAModule, Error) << \"IPAModuleInfo symbol not found\";\n>>\n>> This function is called loadSymbol, it doesn't 'know' that it's loading\n>> an IPAModuleInfo symbol.\n>>\n>> I'd change this error to \"Symbol \" << symbol << \" not found\";\n> \n> And this.\n> \n>>> +\t\treturn -ENOEXEC;\n>>> +\t}\n>>> +\n>>> +\t/* Locate and return data of symbol. */\n>>> +\toffset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;\n>>> +\tsHdr = elfPointer(map, offset, soSize);\n>>> +\tif (!sHdr)\n>>> +\t\treturn -ENOEXEC;\n>>> +\toffset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);\n>>> +\tchar *data = elfPointer(map, offset, soSize);\n>>> +\tif (!data)\n>>> +\t\treturn -ENOEXEC;\n>>> +\tmemcpy(dst, data, size);\n>>\n>> Oh - interesting, you're copying the symbol out. Once it's mmapped ...\n>> why not parse it directly?\n> \n> I'm not sure what you mean by \"parse\"?\n\n\nI mean once you find the symbol and obtain the pointer to it ('data'\nhere) then you have a pointer to memory which contains the structure.\nYou could return that pointer, and cast it to the type. Then you can\nread the structure members directly.\n\nBut that requires knowing that the symbol doesn't require any further\nrelocations ... which is fine for our simple info structure.\n\nIt just seems logical in my head to have a function called findSymbol()\nwhich returns a pointer to the symbol, and then (if you're going to copy\nit) a function called loadStructure(), which finds the symbol, checks\nthe size then copies it or such. Maybe it's generalising the functions\nmore than we care if we will only use them for one thing\n\n> \n>> Can we do all of the 'work' we need to do on the file, and then close it\n>> at the destructor?\n> \n> I don't want to keep the file open/mapped over multiple method calls.\n> That's why in my first version there was a constructor that did nothing\n> and an init() that did everything (which got moved to the constructor in\n> this version).\n\nThe only things we'll do on this structure is read it and decide if this\nlibrary is OK for loading, perhaps even printing the name string in debug.\n\nOnce that has occurred, can't the IPAModuleInfo structure be discarded?\nThe next thing that we'll do is spawn a container/namespace/safe-thingy\nand use the normal dl_open methods to re-open the library there...\n\n\n\n\n> \n>> I guess actually this might then keep the whole file mapped in memory\n>> for longer which might not be desirable...\n>>\n>>\n>>\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)\n>>> +{\n>>> +\tint fd = open(libPath, O_RDONLY);\n>>> +\tif (fd < 0) {\n>>> +\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n>>> +\t\t\t\t << strerror(errno);\n>>> +\t\treturn fd;\n>>> +\t}\n>>> +\n>>> +\tint ret = loadELFIdent(fd);\n>>> +\tif (ret)\n>>> +\t\tgoto close;\n>>> +\n>>> +\tsize_t soSize;\n>>> +\tchar *map;\n>>> +\tstruct stat st;\n>>> +\tret = fstat(fd, &st);\n>>> +\tif (ret < 0)\n>>> +\t\tgoto close;\n>>> +\tsoSize = st.st_size;\n>>> +\tmap = static_cast(mmap(NULL, soSize, PROT_READ,\n>>> +\t\t\t\t MAP_PRIVATE, fd, 0));\n>>\n>> *iff* the constructor did the open, and the symbol parsing was\n>> separated, then I think soSize and map would go to class privates. Then\n>> they could be accessed directly by loadSymbol too. But that's an only if...\n>>\n>> Essentially, it would be\n>>\n>> IPAModule(\"SO-path\")\n>> - Constructor opens the file, performs stat, and mmap.\n>> \t (or those become an open() call)\n>> - loadIPAModuleInfo() calls loadSymbol(\"ipaModuleInfo\"); and deals only\n>> with the ipaModule info structure and checking or such.\n> \n> If loadIPAModuleInfo() is to be called from within the constructor\n> (after open, stat, and mmap), then I agree. I'm not sure map and soSize\n> need to be privates though. It could go either way.\n\nWell, I leave that all up to you now :-)\n\n> \n>>\n>>> +\tif (map == MAP_FAILED) {\n>>> +\t\tret = -errno;\n>>> +\t\tgoto close;\n>>> +\t}\n>>> +\n>>> +\tif (bitClass_ == ELFCLASS32)\n>>> +\t\tret = loadSymbol\n>>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n>>> +\telse if (bitClass_ == ELFCLASS64)\n>>> +\t\tret = loadSymbol\n>>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n>>> +\tif (ret)\n>>> +\t\tgoto unmap;\n>>> +\n>>> +unmap:\n>>> +\tmunmap(map, soSize);\n>>> +close:\n>>> +\tclose(fd);\n>>> +\treturn ret;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Check if construction of the IPAModule instance succeeded\n>>> + *\n>>> + * \\return true if the constructor succeeded with no errors, false otherwise\n>>> + */\n>>> +\n>>> +bool IPAModule::isValid() const\n>>> +{\n>>> +\treturn loaded_;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Get the loaded IPAModuleInfo\n>>> + *\n>>> + * Check isValid() before calling this.\n>>> + *\n>>> + * \\return IPAModuleInfo\n>>> + */\n>>> +\n>>> +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const\n>>> +{\n>>> +\treturn info_;\n>>> +}\n>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>> index 8796f49..e5b48f2 100644\n>>> --- a/src/libcamera/meson.build\n>>> +++ b/src/libcamera/meson.build\n>>> @@ -10,6 +10,7 @@ libcamera_sources = files([\n>>> 'event_notifier.cpp',\n>>> 'formats.cpp',\n>>> 'geometry.cpp',\n>>> + 'ipa_module.cpp',\n>>> 'log.cpp',\n>>> 'media_device.cpp',\n>>> 'media_object.cpp',\n>>> @@ -31,6 +32,7 @@ libcamera_headers = files([\n>>> 'include/device_enumerator_udev.h',\n>>> 'include/event_dispatcher_poll.h',\n>>> 'include/formats.h',\n>>> + 'include/ipa_module.h',\n>>> 'include/log.h',\n>>> 'include/media_device.h',\n>>> 'include/media_object.h',\n>>>\n> \n> \n> Thanks,\n> \n> Paul\n>","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 A045160103\n\tfor ;\n\tWed, 15 May 2019 17:26:52 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D34BAE2;\n\tWed, 15 May 2019 17:26:51 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557934012;\n\tbh=QS9RgvCSi51LSP0TNkhx3Z4cQR6oGk0TWzj0IPQsLHA=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=GCWX9ssF2OI9Wvzxok/50clHORCXBLUcxOyt29bEuRzz/8aaBhQw1I6oW9zTrlwd0\n\tKT2GZMi1yjuNM68UY6qbFBNfzpSiEsbY5/lojrFE2B9pFd2Ktc5XmqC/yRtD53vtoT\n\t9xKBJLliQDZX+olt7uiOtMM7NaLXvHZ6f/UUIXJo=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20190514223808.27351-1-paul.elder@ideasonboard.com>\n\t<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>\n\t<20190515150253.GF12325@localhost.localdomain>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<27f62fbf-1f80-af57-10c9-75f94d87487f@ideasonboard.com>","Date":"Wed, 15 May 2019 16:26:48 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.6.1","MIME-Version":"1.0","In-Reply-To":"<20190515150253.GF12325@localhost.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","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":"Wed, 15 May 2019 15:26:52 -0000"}},{"id":1600,"web_url":"https://patchwork.libcamera.org/comment/1600/","msgid":"<20190515211942.GA4991@pendragon.ideasonboard.com>","date":"2019-05-15T21:19:42","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, May 15, 2019 at 04:26:48PM +0100, Kieran Bingham wrote:\n> On 15/05/2019 16:02, Paul Elder wrote:\n> > On Wed, May 15, 2019 at 10:28:46AM +0100, Kieran Bingham wrote:\n> >> Hi Paul,\n> >>\n> >> Thank you for your patch, This is looking good and it's going in the\n> >> right direction but I have a few design comments ... feel free to\n> >> disagree though ...\n> > \n> > Thank you for the review.\n> > \n> >> On 14/05/2019 23:38, Paul Elder wrote:\n> >>> Implement a class to load a struct IPAModuleInfo of a given symbol name\n> >>> from a .so shared object library.\n> >>>\n> >>> Signed-off-by: Paul Elder \n> >>> ---\n> >>> Changes in v2:\n> >>> - renamed LibLoader class to IPAModule\n> >>> - added documentation\n> >>> - added logging\n> >>> - check that bitness of the shared object is the same as libcamera\n> >>> - moved symbol loading (\"init\") to the constructor, and added isValid()\n> >>> - added elfPointer() to prevent segfaults when reading data from mmap\n> >>> - moved struct IPAModuleInfo out of IPAModule\n> >>> - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a\n> >>> const reference\n> >>> - added munmap after the mmap\n> >>>\n> >>> src/libcamera/include/ipa_module.h | 43 +++++\n> >>> src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++\n> >>> src/libcamera/meson.build | 2 +\n> >>> 3 files changed, 322 insertions(+)\n> >>> create mode 100644 src/libcamera/include/ipa_module.h\n> >>> create mode 100644 src/libcamera/ipa_module.cpp\n> >>>\n> >>> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> >>> new file mode 100644\n> >>> index 0000000..9eb0094\n> >>> --- /dev/null\n> >>> +++ b/src/libcamera/include/ipa_module.h\n> >>> @@ -0,0 +1,43 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * ipa_module.h - load IPA module information from shared library at runtime\n> >>\n> >> Hrm ... we have two sides here.\n> >>\n> >> We need a public header which defines the interface between an IPA\n> >> module and libcamera. That would include a struct IPAModuleInfo and any\n> >> registration details, but should not include any internal libcamera\n> >> private details regarding how the module is loaded.\n> > \n> > Yes, I agree.\n> > \n> >>> + */\n> >>> +#ifndef __LIBCAMERA_IPA_MODULE_H__\n> >>> +#define __LIBCAMERA_IPA_MODULE_H__\n> >>> +\n> >>> +#include \n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +struct IPAModuleInfo {\n> >>> +\tchar name[256];\n> >>> +\tunsigned int version;\n> >>> +};\n> >>> +\n> >>\n> >> So IPModuleInfo (and then later, the class definition for how a\n> >> developer would construct an IPA Module) should live in the public\n> >> headers at:\n> >>\n> >> /include/libcamera/ipa_module.h\n> > \n> > Yeah.\n> > \n> > Then, where should class IPAModule go?\n> \n> Stays here in this file I think :-)\n\nThat's what I had envisioned, so it sounds good to me.\n\n> > \n> >>> +class IPAModule\n> \n> I'm wondering if we have two sides if this class should be calls\n> IPAModule though.\n> \n> Surely an IPAModule would implement a class called IPAModule, and\n> perhaps 'this' class is an IPAModuleParser? IPAModuleLoader?\n> \n> Probably a Loader :D\n\nI was wondering about this. One option would indeed to provide an\nIPAModule class with pure virtual functions that the module would need\nto implement. Another option would be to use a structure with C-style\nfunction pointers. I was thinking about the latter, but the former is\nnot a bad idea. However, we need to take care about backward\ncompatibility. Can a module compiled with an old version of g++ (or\nclang++) be incompatible with libcamera built with a newer version if\nusing a C++ ABI ?\n\nIe we go the C++ way, we will indeed need two different names for the\nclasses. I was thinking about IPAModule for this class and\nIPAModuleInterface for the new one, in which case no change would be\nneeded here. Note that IPAModule would inherit from IPAModuleInterface\nas it would provide the IPA API to pipeline handlers. I don't like the\nname IPAModuleLoader much, as from a pipeline handler point of view\nwe're using modules, not module loaders. Other options may be possible.\n\n> >>> +{\n> >>> +public:\n> >>> +\texplicit IPAModule(const std::string &libPath, const std::string &symbol);\n\nNo need to pass the symbol name here, you can hardcode it in the\nimplementation. All modules will use the same symbol name.\n\n> >>> +\n> >>> +\tbool isValid() const;\n> >>> +\n> >>> +\tstruct IPAModuleInfo const &IPAModuleInfo() const;\n\nWe usually write this const struct IPAModuleInfo &IPAModuleInfo() const.\nAnd I would name the function info(), as IPAModule::info() makes it\nclear that we're retrieving the information of an IPA module.\n\n> >>> +\n> >>> +private:\n> >>> +\tstruct IPAModuleInfo info_;\n> >>> +\n> >>> +\tbool loaded_;\n\nLet's name this valid_ to match isValid(). We will later have a load()\nfunction to dlopen() the module, and likely a loaded_ flag to match\nthat.\n\n> >>> +\tint bitClass_;\n> >>> +\n> >>> +\tint loadELFIdent(int fd);\n> >>> +\ttemplate\n> >>> +\tint loadSymbol(void *data, size_t size, char *map, size_t soSize,\n> >>> +\t\t const char *symbol);\n> >>> +\tint loadIPAModuleInfo(const char *libPath, const char *symbol);\n> >>> +};\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> +#endif /* __LIBCAMERA_IPA_MODULE_H__ */\n> >>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> >>> new file mode 100644\n> >>> index 0000000..5ca16e8\n> >>> --- /dev/null\n> >>> +++ b/src/libcamera/ipa_module.cpp\n> >>> @@ -0,0 +1,277 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * ipa_module.cpp - load IPA module information from shared library at runtime\n\nI think we can already update the description to \"Image Processing\nAlgorithm module\" or similar\n\n> >>> + */\n> >>> +\n> >>> +#include \"ipa_module.h\"\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \n> >>> +\n> >>> +#include \"log.h\"\n> >>> +\n> >>> +/**\n> >>> + * \\file ipa_module.h\n> >>> + * \\brief IPA module information loading\n\nSame here.\n\n> >>> + */\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +/**\n> >>> + * \\struct IPAModuleInfo\n> >>> + * \\brief Information of an IPA plugin\n> >>> + *\n> >>> + * This structure contains the information of an IPA plugin. It is loaded,\n> >>> + * read, and validated before anything else is loaded from the plugin.\n> >>> + *\n> >>> + * \\var IPAModuleInfo::name\n> >>> + * \\brief The name of the IPA plugin\n> >>> + *\n> >>> + * \\var IPAModuleInfo::version\n> >>> + * \\brief The version of the IPA plugin\n> >>\n> >> I don't think we need to store the 'version' of the plugin, so much as\n> >> the version of the API it was compiled against to ensure that it is\n> >> compatible with this 'running' instance of libcamera.\n> >>\n> >> I.e. We don't care that it's the 3rd iteration of the rk3399 module -\n> >> but we do care that it is compatible with the API defined in Libcamera 1.0.\n> > \n> > Yeah, good point; I agree.\n> > \n> > How would we get the API version of libcamera?\n> \n> You get to define it :-)\n> \n> To start with it's probably just:\n> \n> #define LIBCAMERA_IPA_API_VERSION 1\n> \n> This would be a 'local' IPAModule API version number or such I think, as\n> our IPA modules must go through a defined interface...\n> \n> Any time an ABI break occurs in the IPAModule 'API' we would have to\n> bump up the apiversion.\n\nI agree, but I think we can defer this. The IPAModuleInfo structure is\ncurrently a mock up to start implementing the IPAModule class, I expect\nit to change a lot.\n\n> >> Maybe we should add compatible strings to match against pipeline\n> >> handlers as some point too :-) \n\nI think we will need that :-)\n\nPaul, could you record there open items ?\n\n> >>> + */\n> >>> +\n> >>> +LOG_DEFINE_CATEGORY(IPAModule)\n> >>> +\n> >>> +template\n> >>> +typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n> >>> +\t\t\t\t\t\t size_t fileSize)\n> >>> +{\n> >>> +\tsize_t size = offset + sizeof(T);\n> >>> +\tif (size > fileSize || size < sizeof(T))\n> >>> +\t\treturn nullptr;\n> >>> +\n> >>> +\treturn reinterpret_cast::type *>\n> >>> +\t\t(static_cast(map) + offset);\n> >>> +}\n\nThis function should be static. The C way of doing so is using the\nstatic keyword, the C++ way is to put it in an anonymous namespace.\n\nnamespace {\n\ntemplate\ntypename std::remove_extent::type *elfPointer(void *map, off_t offset,\n\t\t\t\t\t\t size_t fileSize)\n{\n...\n}\n\n};\n\n> >>> +\n> >>> +/**\n> >>> + * \\class IPAModule\n> >>> + * \\brief Load IPA module information from an IPA plugin\n\nTo match the suggestion above, \"IPA module wrapper\" ? I think we can do\nbetter than that, maybe \"Wrapper around an IPA module shared object\" ?\nSuggestions are welcome.\n\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Construct an IPAModule instance\n> >>> + * \\param[in] libPath path to IPA plugin\n> >>> + * \\param[in] symbol name of IPAModuleInfo to load from libPath\n> >>> + *\n> >>> + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.\n> >>> + * The IPA plugin shared object file must be of the same endianness and\n> >>> + * bitness as libcamera.\n> >>> + *\n> >>> + * Note that isValid() should be called to check if this loading was successful\n> >>> + * or not.\n> >>\n> >> Hrm ... this is a slightly different design pattern than the rest of our\n> >> objects which are always constructable, and then actions later can fail.\n> >>\n> >> I don't mind - but I wonder if we should add some documentation\n> >> regarding our design patterns somewhere.\n> >>\n> >> (not an action on this patch)\n\nI've discussed this with Paul, and I think that in this case this\nimplementation makes sense. There's no need to delay loading, and as the\nIPAModule class is a wrapper around a .so, constructing an instance on\nwhich on can call isValid() to check if the IPA is valid makes sense to\nme.\n\nMaybe we need to reconsider our other classes indeed. Any\nsuggestion/preference ?\n\n> >>> + */\n> >>> +\n> >>> +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)\n> >>> +\t: loaded_(false)\n> >>\n> >> So actually, from my text below - what I'm really saying is that I don't\n> >> think you should provide a &symbol to this IPAModule constructor :)\n\nSo we agree :-)\n\n> >>> +{\n> >>> +\tint ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());\n> >>\n> >> I see that you have moved the load into this constructor based on\n> >> comments from Laurent in v1. I think that's fine, but I think merging\n> >> the loading of the object, and the parsing of the symbol might be\n> >> combining too much. But it depends (as ever).\n\nNote that this only loads the module info, not the module itself (I\nforesee a load() function that will dlopen() the module). I'm not sure\nwhat you mean by merging the loading of the object and the parsing of\nthe symbol.\n\n> >> For efficiency, the constructor can load the file once and when created\n> >> and .isValid() is true, then we know we can successfully parse symbols.\n\nBut you can't know it's a valid IPA unless you can load the\nIPAModuleInfo successfully (and later maybe even cryptographically\nverifying the module).\n\n> >> But I think loadIPAModuleInfo() should be 'hardcoded' to know exactly\n> >> which symbol name it will expect for an IPAModule. Think of it like a\n> >> 'main()' entry point for our IPAModule. You would always expect a C\n> >> binary to have a main() symbol...\n> > \n> > Yeah, I think that's a good idea.\n> > \n> >> So I would expect a libcamera IPA module to have a registration\n> >> something like:\n> >>\n> >>\n> >> /* Register our module with Libcamera */\n> >> const struct IPAModuleInfo ipaModuleInfo = {\n> >> \t.name = \"Image Processing for the RK3399\",\n> >> \t.version = 1, /* Perhaps this should be apiversion to prevent\n> >> * people thinking it describes the version of the\n> >> * module ? ...\n> >> */\n> >> };\n> > \n> > Should the info entry point be called ipaModuleInfo?\n> \n> I think variables are in camelCase with a lowerCase first letter in the\n> project right? or was this not quite your question?\n\nThis sounds good to me.\n\n> >> This could even then be turned into a macro:\n> >>\n> >> #define LIBCAMERA_IPA_MODULE(__NAME) \\\n> >> const struct IPAModuleInfo ipaModuleInfo = { \\\n> >> \t.name = __NAME,\t\\\n> >> \t.apiversion = 1, \\\n> >> }\n\nWe'll have to throw an extern \"C\" here, otherwise your symbol name will\nbe _ZL13ipaModuleInfo with g++.\n\n> >>\n> >> so that a module rk3399_ipa.cpp could define:\n> >>\n> >> LIBCAMERA_IPA_MODULE(\"Image Processing Algorithms for the RK3399\");\n> >>\n> >> (apiversion would be hardcoded by the macro because it defines what\n> >> version of the API/libraries it's compiled against..., and we check it's\n> >> compatible at runtime)\n\nSomething like that. If the number of fields grows, and if some of them\nbecome complex, we may want to fill the structure manually, be we should\nstill use a macro to instantiate the right structure with the right name\nand the extern \"C\" keyword.\n\n> >>> +\tif (ret < 0) {\n> >>> +\t\tloaded_ = false;\n\nThis isn't needed, you initialise loaded_ to false above.\n\n> >>> +\t\treturn;\n> >>> +\t}\n> >>> +\n> >>> +\tloaded_ = true;\n> >>> +}\n> >>> +\n> >>> +int IPAModule::loadELFIdent(int fd)\n\nThis function doesn't actually load much, how about renaming it to\nverifyELFIdent() ?\n\n> >>> +{\n> >>> +\tint ret = lseek(fd, 0, SEEK_SET);\n> >>> +\tif (ret < 0)\n> >>> +\t\treturn -errno;\n> >>> +\n\nAs we're mmap()ing the whole .so, you could mmap() before calling\nloadELFIdent(), and pass the void *map and size to the function, instead\nof using read.\n\n> >>> +\tunsigned char e_ident[EI_NIDENT];\n> >>> +\tret = read(fd, e_ident, EI_NIDENT);\n> >>> +\tif (ret < 0)\n> >>> +\t\treturn -errno;\n> >>> +\n> >>> +\tif (e_ident[EI_MAG0] != ELFMAG0 ||\n> >>> +\t e_ident[EI_MAG1] != ELFMAG1 ||\n> >>> +\t e_ident[EI_MAG2] != ELFMAG2 ||\n> >>> +\t e_ident[EI_MAG3] != ELFMAG3 ||\n> >>> +\t e_ident[EI_VERSION] != EV_CURRENT)\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\n> >>> +\tif ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||\n> >>> +\t (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))\n> >>> +\t\tbitClass_ = e_ident[EI_CLASS];\n> >>> +\telse\n> >>> +\t\treturn -ENOEXEC;\n\nHow about\n\n\tbitClass_ = sizeof(unsigned long) == 4 ? ELFCLASS32 : ELFCLASS64;\n\tif (e_ident[EI_CLASS] != bitClass_)\n\t\treturn -ENOEXEC;\n\n> >>> +\n> >>> +\tint a = 1;\n> >>> +\tunsigned char endianness = *reinterpret_cast(&a) == 1\n> >>> +\t\t\t\t ? ELFDATA2LSB : ELFDATA2MSB;\n\nSo you didn't like my union-based solution ? :-)\n\n> >>> +\tif (e_ident[EI_DATA] != endianness)\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +template\n> >>> +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,\n> >>> +\t\t\t const char *symbol)\n> >>> +{\n> >>> +\tElfHeader *eHdr = elfPointer(map, 0, soSize);\n> >>> +\tif (!eHdr)\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\n> >>> +\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n> >>> +\tSecHeader *sHdr = elfPointer(map, offset, soSize);\n> >>> +\tif (!sHdr)\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\toff_t shnameoff = sHdr->sh_offset;\n> >>> +\n> >>> +\t/* Locate .dynsym section header. */\n> >>> +\tbool found = false;\n> >>> +\tSecHeader *dynsym;\n> >>> +\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n> >>> +\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n> >>> +\t\tsHdr = elfPointer(map, offset, soSize);\n> >>> +\t\tif (!sHdr)\n> >>> +\t\t\treturn -ENOEXEC;\n> >>> +\n> >>> +\t\toffset = shnameoff + sHdr->sh_name;\n> >>> +\t\tchar *name = elfPointer(map, offset, soSize);\n\nThis should be elfPointer as you want to ensure that at least 8\nbytes are available (sizeof(\".dynsym\"), including the terminating 0).\n\n> >>> +\t\tif (!name)\n> >>> +\t\t\treturn -ENOEXEC;\n> >>> +\n> >>> +\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n> >>> +\t\t\tfound = true;\n> >>> +\t\t\tdynsym = sHdr;\n\nYou could initialise dynsym to nullptr and remove found.\n\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\tif (!found) {\n> >>> +\t\tLOG(IPAModule, Error) << \"ELF has no .dynsym section\";\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\t}\n> >>> +\n> >>> +\toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n> >>> +\tsHdr = elfPointer(map, offset, soSize);\n> >>> +\tif (!sHdr)\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\toff_t dynsym_nameoff = sHdr->sh_offset;\n> >>> +\n> >>> +\t/* Locate symbol in the .dynsym section. */\n> >>> +\tfound = false;\n> >>> +\tSymHeader *target_symbol;\n\ntargetSymbol ?\n\n> >>> +\tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n> >>> +\tfor (unsigned int i = 0; i < dynsym_num; i++) {\n> >>> +\t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n> >>> +\t\tSymHeader *sym = elfPointer(map, offset, soSize);\n> >>> +\t\tif (!sym)\n> >>> +\t\t\treturn -ENOEXEC;\n> >>> +\n> >>> +\t\toffset = dynsym_nameoff + sym->st_name;\n> >>> +\t\tchar *name = elfPointer(map, offset, soSize);\n\nSame here, you want elfPointer. I hope this\nwill compile :-)\n\n> >>> +\t\tif (!name)\n> >>> +\t\t\treturn -ENOEXEC;\n> >>> +\n> >>> +\t\tif (!strcmp(name, symbol) &&\n> >>> +\t\t sym->st_info & STB_GLOBAL &&\n> >>> +\t\t sym->st_size == sizeof(struct IPAModuleInfo)) {\n> >>\n> >> I think this should be a check on the size_t size passed in?\n> > \n> > Ah yeah, I totally missed that.\n> \n> No worries.\n> \n> >>> +\t\t\tfound = true;\n> >>> +\t\t\ttarget_symbol = sym;\n\nYou could also remove found here.\n\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\tif (!found) {\n> >>> +\t\tLOG(IPAModule, Error) << \"IPAModuleInfo symbol not found\";\n> >>\n> >> This function is called loadSymbol, it doesn't 'know' that it's loading\n> >> an IPAModuleInfo symbol.\n> >>\n> >> I'd change this error to \"Symbol \" << symbol << \" not found\";\n> > \n> > And this.\n> > \n> >>> +\t\treturn -ENOEXEC;\n> >>> +\t}\n> >>> +\n> >>> +\t/* Locate and return data of symbol. */\n\nAs a sanity check you may want to verify that target_symbol->st_shndx <\neHdr->e_shnum. Otherwise none of the elfPointer<> checks below may\ntrigger, and you will load an invalid symbol. This can happen for\ninstance if the symbol is absolute (SHN_ABS).\n\n> >>> +\toffset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;\n> >>> +\tsHdr = elfPointer(map, offset, soSize);\n> >>> +\tif (!sHdr)\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\toffset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);\n> >>> +\tchar *data = elfPointer(map, offset, soSize);\n\nelfPointer\n\n> >>> +\tif (!data)\n> >>> +\t\treturn -ENOEXEC;\n> >>> +\tmemcpy(dst, data, size);\n> >>\n> >> Oh - interesting, you're copying the symbol out. Once it's mmapped ...\n> >> why not parse it directly?\n> > \n> > I'm not sure what you mean by \"parse\"?\n> \n> I mean once you find the symbol and obtain the pointer to it ('data'\n> here) then you have a pointer to memory which contains the structure.\n> You could return that pointer, and cast it to the type. Then you can\n> read the structure members directly.\n> \n> But that requires knowing that the symbol doesn't require any further\n> relocations ... which is fine for our simple info structure.\n> \n> It just seems logical in my head to have a function called findSymbol()\n> which returns a pointer to the symbol, and then (if you're going to copy\n> it) a function called loadStructure(), which finds the symbol, checks\n> the size then copies it or such. Maybe it's generalising the functions\n> more than we care if we will only use them for one thing\n\nI have no particular opinion on this. We could indeed rename this\nfunction to findSymbol() and move the copy outside. It may actually be a\ngood idea, but then this function should return both the address (or\noffset) and size (one or both through output arguments).\n\n> >> Can we do all of the 'work' we need to do on the file, and then close it\n> >> at the destructor?\n> > \n> > I don't want to keep the file open/mapped over multiple method calls.\n> > That's why in my first version there was a constructor that did nothing\n> > and an init() that did everything (which got moved to the constructor in\n> > this version).\n\nI agree, we shouldn't keep it mapped.\n\n> The only things we'll do on this structure is read it and decide if this\n> library is OK for loading, perhaps even printing the name string in debug.\n> \n> Once that has occurred, can't the IPAModuleInfo structure be discarded?\n> The next thing that we'll do is spawn a container/namespace/safe-thingy\n> and use the normal dl_open methods to re-open the library there...\n\nNo, we need it at least to match the module with pipeline handlers, so\nit needs to be copied. I'm sure we'll have other fields that we will\nalso need to access later.\n\n> >> I guess actually this might then keep the whole file mapped in memory\n> >> for longer which might not be desirable...\n> >>\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)\n\nI would store libPath in a member variable as we will need it for\ndlopen(), and use that member variable in this function.\n\n> >>> +{\n> >>> +\tint fd = open(libPath, O_RDONLY);\n> >>> +\tif (fd < 0) {\n> >>> +\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> >>> +\t\t\t\t << strerror(errno);\n\nYou need\n\n\t\tret = -errno;\n\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n\t\t\t\t << strerror(-ret);\n\t\treturn ret;\n\nass errno may be changed by LOG(IPAModule, Error).\n\n> >>> +\t\treturn fd;\n> >>> +\t}\n> >>> +\n> >>> +\tint ret = loadELFIdent(fd);\n> >>> +\tif (ret)\n> >>> +\t\tgoto close;\n> >>> +\n> >>> +\tsize_t soSize;\n> >>> +\tchar *map;\n> >>> +\tstruct stat st;\n> >>> +\tret = fstat(fd, &st);\n> >>> +\tif (ret < 0)\n> >>> +\t\tgoto close;\n> >>> +\tsoSize = st.st_size;\n> >>> +\tmap = static_cast(mmap(NULL, soSize, PROT_READ,\n> >>> +\t\t\t\t MAP_PRIVATE, fd, 0));\n\n\tstruct stat st;\n\tret = fstat(fd, &st);\n\tif (ret < 0)\n\t\tgoto close;\n\n\tsize_t soSize = st.st_size;\n\tchar *map = static_cast(mmap(NULL, soSize, PROT_READ,\n\t\t\t\t\t MAP_PRIVATE, fd, 0));\n\nI think I would make it a void *map, pass it as a void * to all the\nfunctions, and let elfPointer<> do the case.\n\n> >> *iff* the constructor did the open, and the symbol parsing was\n> >> separated, then I think soSize and map would go to class privates. Then\n> >> they could be accessed directly by loadSymbol too. But that's an only if...\n> >>\n> >> Essentially, it would be\n> >>\n> >> IPAModule(\"SO-path\")\n> >> - Constructor opens the file, performs stat, and mmap.\n> >> \t (or those become an open() call)\n> >> - loadIPAModuleInfo() calls loadSymbol(\"ipaModuleInfo\"); and deals only\n> >> with the ipaModule info structure and checking or such.\n> > \n> > If loadIPAModuleInfo() is to be called from within the constructor\n> > (after open, stat, and mmap), then I agree. I'm not sure map and soSize\n> > need to be privates though. It could go either way.\n\nI thought about it, but it the address and size are really temporary\nvariables, as we unmap the file after the parsing, so I think it's best\nto pass them explicitly to functions.\n\n> Well, I leave that all up to you now :-)\n> \n> >>> +\tif (map == MAP_FAILED) {\n> >>> +\t\tret = -errno;\n> >>> +\t\tgoto close;\n> >>> +\t}\n> >>> +\n> >>> +\tif (bitClass_ == ELFCLASS32)\n> >>> +\t\tret = loadSymbol\n> >>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> >>> +\telse if (bitClass_ == ELFCLASS64)\n> >>> +\t\tret = loadSymbol\n> >>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n\nAs we don't need to support a module with a different bit class than\nlibcamera, would we avoid compiling in both version of the function ?\nMaybe something like\n\n#if sizeof(long) == 4\n\tret = loadSymbol\n\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n#else\n\tret = loadSymbol\n\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n#endif\n\nand given that we only need one version of the function, I even wonder\nif we need to keep it defined as a template function, maybe we could\nhave a\n\n#if sizeof(long) == 4\n#define Elf_Ehdr Elf32_Ehdr\n#define Elf_Shdr Elf32_Shdr\n#define Elf_Sym Elf32_Sym\n#else\n#define Elf_Ehdr Elf64_Ehdr\n#define Elf_Shdr Elf64_Shdr\n#define Elf_Sym Elf64_Sym\n#endif\n\njust before the function definition. This would however require moving\nthe function away from the class to a global function, private to this\nfile, to avoid exposing it in the ipa_module.h header, otherwise we\nwould have to move the above #if's there, which isn't neat. If we do\nthis we should move the loadElfIdent function too. Neither function use\nany of the class members (except for bitClass_, which isn't needed\nanymore), so they're essentially static, and separating them from the\nIPAModule class would open the door to sharing them with other classes\nif needed later. elfPointer<> is already separate. If you prefer to keep\nthem as class members, they should be declared as static in the class\ndefinition.\n\n> >>> +\tif (ret)\n> >>> +\t\tgoto unmap;\n> >>> +\n> >>> +unmap:\n> >>> +\tmunmap(map, soSize);\n> >>> +close:\n> >>> +\tclose(fd);\n> >>> +\treturn ret;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Check if construction of the IPAModule instance succeeded\n\nI would say \"Check if the IPAModule instance is valid\" and then add a\nparagraph to detail what a valid (or invalid) IPAModule is.\n\n> >>> + *\n> >>> + * \\return true if the constructor succeeded with no errors, false otherwise\n\ntrue if the IPAModule is valid, false otherwise\n\n> >>> + */\n> >>> +\n\nNo need for this blank line.\n\n> >>> +bool IPAModule::isValid() const\n> >>> +{\n> >>> +\treturn loaded_;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Get the loaded IPAModuleInfo\n\n\"Get the IPA module information\"\n\n> >>> + *\n> >>> + * Check isValid() before calling this.\n\n\"The content of the IPA module information is loaded from the module,\nand is valid only if the module is valid (as returned by isValid()).\nCalling this function on an invalid module is an error.\"\n\n> >>> + *\n> >>> + * \\return IPAModuleInfo\n> >>> + */\n> >>> +\n\nNo need for a blank line here either\n\n> >>> +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const\n> >>> +{\n> >>> +\treturn info_;\n> >>> +}\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>> index 8796f49..e5b48f2 100644\n> >>> --- a/src/libcamera/meson.build\n> >>> +++ b/src/libcamera/meson.build\n> >>> @@ -10,6 +10,7 @@ libcamera_sources = files([\n> >>> 'event_notifier.cpp',\n> >>> 'formats.cpp',\n> >>> 'geometry.cpp',\n> >>> + 'ipa_module.cpp',\n> >>> 'log.cpp',\n> >>> 'media_device.cpp',\n> >>> 'media_object.cpp',\n> >>> @@ -31,6 +32,7 @@ libcamera_headers = files([\n> >>> 'include/device_enumerator_udev.h',\n> >>> 'include/event_dispatcher_poll.h',\n> >>> 'include/formats.h',\n> >>> + 'include/ipa_module.h',\n> >>> 'include/log.h',\n> >>> 'include/media_device.h',\n> >>> 'include/media_object.h',","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 2DA1F60103\n\tfor ;\n\tWed, 15 May 2019 23:20:00 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4AE73443;\n\tWed, 15 May 2019 23:19:59 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557955199;\n\tbh=++RZRE7P09V9ltfUz3Ha0PJuU3PvbPUGHds6ZmXA4hM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HlzmcQIgUXQ+6fpR7lCz/kIQO3d98OrERvBGSd+ISQk9ScuKiThllSab0zAUVyrtQ\n\tTr9LUNafWos0l26rsSsUvHX2/vb4qxKq5U8v4rEL40FD1QmrhlXOWPtUTYYBTlgS3+\n\toc03OWTHFToaAOYp0+q6A0Uyqgu143SXIZgnQDfg=","Date":"Thu, 16 May 2019 00:19:42 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190515211942.GA4991@pendragon.ideasonboard.com>","References":"<20190514223808.27351-1-paul.elder@ideasonboard.com>\n\t<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>\n\t<20190515150253.GF12325@localhost.localdomain>\n\t<27f62fbf-1f80-af57-10c9-75f94d87487f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<27f62fbf-1f80-af57-10c9-75f94d87487f@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","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":"Wed, 15 May 2019 21:20:00 -0000"}},{"id":1602,"web_url":"https://patchwork.libcamera.org/comment/1602/","msgid":"<20190516175116.GG12325@localhost.localdomain>","date":"2019-05-16T17:51:16","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi,\n\nOn Thu, May 16, 2019 at 12:19:42AM +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Wed, May 15, 2019 at 04:26:48PM +0100, Kieran Bingham wrote:\n> > On 15/05/2019 16:02, Paul Elder wrote:\n> > > On Wed, May 15, 2019 at 10:28:46AM +0100, Kieran Bingham wrote:\n> > >> Hi Paul,\n> > >>\n> > >> Thank you for your patch, This is looking good and it's going in the\n> > >> right direction but I have a few design comments ... feel free to\n> > >> disagree though ...\n> > > \n> > > Thank you for the review.\n> > > \n> > >> On 14/05/2019 23:38, Paul Elder wrote:\n> > >>> Implement a class to load a struct IPAModuleInfo of a given symbol name\n> > >>> from a .so shared object library.\n> > >>>\n> > >>> Signed-off-by: Paul Elder \n> > >>> ---\n> > >>> Changes in v2:\n> > >>> - renamed LibLoader class to IPAModule\n> > >>> - added documentation\n> > >>> - added logging\n> > >>> - check that bitness of the shared object is the same as libcamera\n> > >>> - moved symbol loading (\"init\") to the constructor, and added isValid()\n> > >>> - added elfPointer() to prevent segfaults when reading data from mmap\n> > >>> - moved struct IPAModuleInfo out of IPAModule\n> > >>> - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a\n> > >>> const reference\n> > >>> - added munmap after the mmap\n> > >>>\n> > >>> src/libcamera/include/ipa_module.h | 43 +++++\n> > >>> src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++\n> > >>> src/libcamera/meson.build | 2 +\n> > >>> 3 files changed, 322 insertions(+)\n> > >>> create mode 100644 src/libcamera/include/ipa_module.h\n> > >>> create mode 100644 src/libcamera/ipa_module.cpp\n> > >>>\n> > >>> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> > >>> new file mode 100644\n> > >>> index 0000000..9eb0094\n> > >>> --- /dev/null\n> > >>> +++ b/src/libcamera/include/ipa_module.h\n> > >>> @@ -0,0 +1,43 @@\n> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >>> +/*\n> > >>> + * Copyright (C) 2019, Google Inc.\n> > >>> + *\n> > >>> + * ipa_module.h - load IPA module information from shared library at runtime\n> > >>\n> > >> Hrm ... we have two sides here.\n> > >>\n> > >> We need a public header which defines the interface between an IPA\n> > >> module and libcamera. That would include a struct IPAModuleInfo and any\n> > >> registration details, but should not include any internal libcamera\n> > >> private details regarding how the module is loaded.\n> > > \n> > > Yes, I agree.\n> > > \n> > >>> + */\n> > >>> +#ifndef __LIBCAMERA_IPA_MODULE_H__\n> > >>> +#define __LIBCAMERA_IPA_MODULE_H__\n> > >>> +\n> > >>> +#include \n> > >>> +\n> > >>> +namespace libcamera {\n> > >>> +\n> > >>> +struct IPAModuleInfo {\n> > >>> +\tchar name[256];\n> > >>> +\tunsigned int version;\n> > >>> +};\n> > >>> +\n> > >>\n> > >> So IPModuleInfo (and then later, the class definition for how a\n> > >> developer would construct an IPA Module) should live in the public\n> > >> headers at:\n> > >>\n> > >> /include/libcamera/ipa_module.h\n> > > \n> > > Yeah.\n> > > \n> > > Then, where should class IPAModule go?\n> > \n> > Stays here in this file I think :-)\n> \n> That's what I had envisioned, so it sounds good to me.\n> \n> > > \n> > >>> +class IPAModule\n> > \n> > I'm wondering if we have two sides if this class should be calls\n> > IPAModule though.\n> > \n> > Surely an IPAModule would implement a class called IPAModule, and\n> > perhaps 'this' class is an IPAModuleParser? IPAModuleLoader?\n> > \n> > Probably a Loader :D\n> \n> I was wondering about this. One option would indeed to provide an\n> IPAModule class with pure virtual functions that the module would need\n> to implement. Another option would be to use a structure with C-style\n> function pointers. I was thinking about the latter, but the former is\n> not a bad idea. However, we need to take care about backward\n> compatibility. Can a module compiled with an old version of g++ (or\n> clang++) be incompatible with libcamera built with a newer version if\n> using a C++ ABI ?\n> \n> Ie we go the C++ way, we will indeed need two different names for the\n> classes. I was thinking about IPAModule for this class and\n> IPAModuleInterface for the new one, in which case no change would be\n> needed here. Note that IPAModule would inherit from IPAModuleInterface\n> as it would provide the IPA API to pipeline handlers. I don't like the\n> name IPAModuleLoader much, as from a pipeline handler point of view\n> we're using modules, not module loaders. Other options may be possible.\n\nI don't know about C++ ABI compatability. I think to just avoid the risk\nitself we should go with C-style function pointers.\n\nIn which case Laurent's shim idea would work well for process isolation\nlater on. Since the only layers are Pipeline -> IPAModule there isn't\nreally anywhere else we can break off into another process without\nmaking the Pipeline -> IPAModule interface ugly.\n\n> > >>> +{\n> > >>> +public:\n> > >>> +\texplicit IPAModule(const std::string &libPath, const std::string &symbol);\n> \n> No need to pass the symbol name here, you can hardcode it in the\n> implementation. All modules will use the same symbol name.\n> \n> > >>> +\n> > >>> +\tbool isValid() const;\n> > >>> +\n> > >>> +\tstruct IPAModuleInfo const &IPAModuleInfo() const;\n> \n> We usually write this const struct IPAModuleInfo &IPAModuleInfo() const.\n> And I would name the function info(), as IPAModule::info() makes it\n> clear that we're retrieving the information of an IPA module.\n> \n> > >>> +\n> > >>> +private:\n> > >>> +\tstruct IPAModuleInfo info_;\n> > >>> +\n> > >>> +\tbool loaded_;\n> \n> Let's name this valid_ to match isValid(). We will later have a load()\n> function to dlopen() the module, and likely a loaded_ flag to match\n> that.\n> \n> > >>> +\tint bitClass_;\n> > >>> +\n> > >>> +\tint loadELFIdent(int fd);\n> > >>> +\ttemplate\n> > >>> +\tint loadSymbol(void *data, size_t size, char *map, size_t soSize,\n> > >>> +\t\t const char *symbol);\n> > >>> +\tint loadIPAModuleInfo(const char *libPath, const char *symbol);\n> > >>> +};\n> > >>> +\n> > >>> +} /* namespace libcamera */\n> > >>> +\n> > >>> +#endif /* __LIBCAMERA_IPA_MODULE_H__ */\n> > >>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> > >>> new file mode 100644\n> > >>> index 0000000..5ca16e8\n> > >>> --- /dev/null\n> > >>> +++ b/src/libcamera/ipa_module.cpp\n> > >>> @@ -0,0 +1,277 @@\n> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >>> +/*\n> > >>> + * Copyright (C) 2019, Google Inc.\n> > >>> + *\n> > >>> + * ipa_module.cpp - load IPA module information from shared library at runtime\n> \n> I think we can already update the description to \"Image Processing\n> Algorithm module\" or similar\n> \n> > >>> + */\n> > >>> +\n> > >>> +#include \"ipa_module.h\"\n> > >>> +\n> > >>> +#include \n> > >>> +#include \n> > >>> +#include \n> > >>> +#include \n> > >>> +#include \n> > >>> +#include \n> > >>> +#include \n> > >>> +#include \n> > >>> +\n> > >>> +#include \n> > >>> +\n> > >>> +#include \"log.h\"\n> > >>> +\n> > >>> +/**\n> > >>> + * \\file ipa_module.h\n> > >>> + * \\brief IPA module information loading\n> \n> Same here.\n> \n> > >>> + */\n> > >>> +\n> > >>> +namespace libcamera {\n> > >>> +\n> > >>> +/**\n> > >>> + * \\struct IPAModuleInfo\n> > >>> + * \\brief Information of an IPA plugin\n> > >>> + *\n> > >>> + * This structure contains the information of an IPA plugin. It is loaded,\n> > >>> + * read, and validated before anything else is loaded from the plugin.\n> > >>> + *\n> > >>> + * \\var IPAModuleInfo::name\n> > >>> + * \\brief The name of the IPA plugin\n> > >>> + *\n> > >>> + * \\var IPAModuleInfo::version\n> > >>> + * \\brief The version of the IPA plugin\n> > >>\n> > >> I don't think we need to store the 'version' of the plugin, so much as\n> > >> the version of the API it was compiled against to ensure that it is\n> > >> compatible with this 'running' instance of libcamera.\n> > >>\n> > >> I.e. We don't care that it's the 3rd iteration of the rk3399 module -\n> > >> but we do care that it is compatible with the API defined in Libcamera 1.0.\n> > > \n> > > Yeah, good point; I agree.\n> > > \n> > > How would we get the API version of libcamera?\n> > \n> > You get to define it :-)\n> > \n> > To start with it's probably just:\n> > \n> > #define LIBCAMERA_IPA_API_VERSION 1\n> > \n> > This would be a 'local' IPAModule API version number or such I think, as\n> > our IPA modules must go through a defined interface...\n> > \n> > Any time an ABI break occurs in the IPAModule 'API' we would have to\n> > bump up the apiversion.\n> \n> I agree, but I think we can defer this. The IPAModuleInfo structure is\n> currently a mock up to start implementing the IPAModule class, I expect\n> it to change a lot.\n> \n> > >> Maybe we should add compatible strings to match against pipeline\n> > >> handlers as some point too :-) \n> \n> I think we will need that :-)\n> \n> Paul, could you record there open items ?\n\nI'm not sure what you mean.\n\n> > >>> + */\n> > >>> +\n> > >>> +LOG_DEFINE_CATEGORY(IPAModule)\n> > >>> +\n> > >>> +template\n> > >>> +typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n> > >>> +\t\t\t\t\t\t size_t fileSize)\n> > >>> +{\n> > >>> +\tsize_t size = offset + sizeof(T);\n> > >>> +\tif (size > fileSize || size < sizeof(T))\n> > >>> +\t\treturn nullptr;\n> > >>> +\n> > >>> +\treturn reinterpret_cast::type *>\n> > >>> +\t\t(static_cast(map) + offset);\n> > >>> +}\n> \n> This function should be static. The C way of doing so is using the\n> static keyword, the C++ way is to put it in an anonymous namespace.\n> \n> namespace {\n> \n> template\n> typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n> \t\t\t\t\t\t size_t fileSize)\n> {\n> ...\n> }\n> \n> };\n> \n> > >>> +\n> > >>> +/**\n> > >>> + * \\class IPAModule\n> > >>> + * \\brief Load IPA module information from an IPA plugin\n> \n> To match the suggestion above, \"IPA module wrapper\" ? I think we can do\n> better than that, maybe \"Wrapper around an IPA module shared object\" ?\n> Suggestions are welcome.\n> \n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Construct an IPAModule instance\n> > >>> + * \\param[in] libPath path to IPA plugin\n> > >>> + * \\param[in] symbol name of IPAModuleInfo to load from libPath\n> > >>> + *\n> > >>> + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.\n> > >>> + * The IPA plugin shared object file must be of the same endianness and\n> > >>> + * bitness as libcamera.\n> > >>> + *\n> > >>> + * Note that isValid() should be called to check if this loading was successful\n> > >>> + * or not.\n> > >>\n> > >> Hrm ... this is a slightly different design pattern than the rest of our\n> > >> objects which are always constructable, and then actions later can fail.\n> > >>\n> > >> I don't mind - but I wonder if we should add some documentation\n> > >> regarding our design patterns somewhere.\n> > >>\n> > >> (not an action on this patch)\n> \n> I've discussed this with Paul, and I think that in this case this\n> implementation makes sense. There's no need to delay loading, and as the\n> IPAModule class is a wrapper around a .so, constructing an instance on\n> which on can call isValid() to check if the IPA is valid makes sense to\n> me.\n> \n> Maybe we need to reconsider our other classes indeed. Any\n> suggestion/preference ?\n\nDo you mean to make the other classes follow the same model? As long as\nwe document the exception (well the usage is already documented...) then\nshouldn't it be fine?\n\n> > >>> + */\n> > >>> +\n> > >>> +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)\n> > >>> +\t: loaded_(false)\n> > >>\n> > >> So actually, from my text below - what I'm really saying is that I don't\n> > >> think you should provide a &symbol to this IPAModule constructor :)\n> \n> So we agree :-)\n> \n> > >>> +{\n> > >>> +\tint ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());\n> > >>\n> > >> I see that you have moved the load into this constructor based on\n> > >> comments from Laurent in v1. I think that's fine, but I think merging\n> > >> the loading of the object, and the parsing of the symbol might be\n> > >> combining too much. But it depends (as ever).\n> \n> Note that this only loads the module info, not the module itself (I\n> foresee a load() function that will dlopen() the module). I'm not sure\n> what you mean by merging the loading of the object and the parsing of\n> the symbol.\n> \n> > >> For efficiency, the constructor can load the file once and when created\n> > >> and .isValid() is true, then we know we can successfully parse symbols.\n> \n> But you can't know it's a valid IPA unless you can load the\n> IPAModuleInfo successfully (and later maybe even cryptographically\n> verifying the module).\n> \n> > >> But I think loadIPAModuleInfo() should be 'hardcoded' to know exactly\n> > >> which symbol name it will expect for an IPAModule. Think of it like a\n> > >> 'main()' entry point for our IPAModule. You would always expect a C\n> > >> binary to have a main() symbol...\n> > > \n> > > Yeah, I think that's a good idea.\n> > > \n> > >> So I would expect a libcamera IPA module to have a registration\n> > >> something like:\n> > >>\n> > >>\n> > >> /* Register our module with Libcamera */\n> > >> const struct IPAModuleInfo ipaModuleInfo = {\n> > >> \t.name = \"Image Processing for the RK3399\",\n> > >> \t.version = 1, /* Perhaps this should be apiversion to prevent\n> > >> * people thinking it describes the version of the\n> > >> * module ? ...\n> > >> */\n> > >> };\n> > > \n> > > Should the info entry point be called ipaModuleInfo?\n> > \n> > I think variables are in camelCase with a lowerCase first letter in the\n> > project right? or was this not quite your question?\n> \n> This sounds good to me.\n\nMy question was about the entry point name, not the casing :p\n\n> > >> This could even then be turned into a macro:\n> > >>\n> > >> #define LIBCAMERA_IPA_MODULE(__NAME) \\\n> > >> const struct IPAModuleInfo ipaModuleInfo = { \\\n> > >> \t.name = __NAME,\t\\\n> > >> \t.apiversion = 1, \\\n> > >> }\n> \n> We'll have to throw an extern \"C\" here, otherwise your symbol name will\n> be _ZL13ipaModuleInfo with g++.\n> \n> > >>\n> > >> so that a module rk3399_ipa.cpp could define:\n> > >>\n> > >> LIBCAMERA_IPA_MODULE(\"Image Processing Algorithms for the RK3399\");\n> > >>\n> > >> (apiversion would be hardcoded by the macro because it defines what\n> > >> version of the API/libraries it's compiled against..., and we check it's\n> > >> compatible at runtime)\n> \n> Something like that. If the number of fields grows, and if some of them\n> become complex, we may want to fill the structure manually, be we should\n> still use a macro to instantiate the right structure with the right name\n> and the extern \"C\" keyword.\n\nYeah I think I'll do that.\n\n> > >>> +\tif (ret < 0) {\n> > >>> +\t\tloaded_ = false;\n> \n> This isn't needed, you initialise loaded_ to false above.\n> \n> > >>> +\t\treturn;\n> > >>> +\t}\n> > >>> +\n> > >>> +\tloaded_ = true;\n> > >>> +}\n> > >>> +\n> > >>> +int IPAModule::loadELFIdent(int fd)\n> \n> This function doesn't actually load much, how about renaming it to\n> verifyELFIdent() ?\n> \n> > >>> +{\n> > >>> +\tint ret = lseek(fd, 0, SEEK_SET);\n> > >>> +\tif (ret < 0)\n> > >>> +\t\treturn -errno;\n> > >>> +\n> \n> As we're mmap()ing the whole .so, you could mmap() before calling\n> loadELFIdent(), and pass the void *map and size to the function, instead\n> of using read.\n\nI didn't want to have to do elfPointer for every single byte that I read\nit, but I forgot that I could just use elfPointer on an array.\n\n> > >>> +\tunsigned char e_ident[EI_NIDENT];\n> > >>> +\tret = read(fd, e_ident, EI_NIDENT);\n> > >>> +\tif (ret < 0)\n> > >>> +\t\treturn -errno;\n> > >>> +\n> > >>> +\tif (e_ident[EI_MAG0] != ELFMAG0 ||\n> > >>> +\t e_ident[EI_MAG1] != ELFMAG1 ||\n> > >>> +\t e_ident[EI_MAG2] != ELFMAG2 ||\n> > >>> +\t e_ident[EI_MAG3] != ELFMAG3 ||\n> > >>> +\t e_ident[EI_VERSION] != EV_CURRENT)\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\n> > >>> +\tif ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||\n> > >>> +\t (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))\n> > >>> +\t\tbitClass_ = e_ident[EI_CLASS];\n> > >>> +\telse\n> > >>> +\t\treturn -ENOEXEC;\n> \n> How about\n> \n> \tbitClass_ = sizeof(unsigned long) == 4 ? ELFCLASS32 : ELFCLASS64;\n> \tif (e_ident[EI_CLASS] != bitClass_)\n> \t\treturn -ENOEXEC;\n\nYeah. I guess we're going to demote bitClass_ to a local variable.\n\n> > >>> +\n> > >>> +\tint a = 1;\n> > >>> +\tunsigned char endianness = *reinterpret_cast(&a) == 1\n> > >>> +\t\t\t\t ? ELFDATA2LSB : ELFDATA2MSB;\n> \n> So you didn't like my union-based solution ? :-)\n\nNot really :p\nimo this was more readable.\n\n> > >>> +\tif (e_ident[EI_DATA] != endianness)\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\n> > >>> +\treturn 0;\n> > >>> +}\n> > >>> +\n> > >>> +template\n> > >>> +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,\n> > >>> +\t\t\t const char *symbol)\n> > >>> +{\n> > >>> +\tElfHeader *eHdr = elfPointer(map, 0, soSize);\n> > >>> +\tif (!eHdr)\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\n> > >>> +\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n> > >>> +\tSecHeader *sHdr = elfPointer(map, offset, soSize);\n> > >>> +\tif (!sHdr)\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\toff_t shnameoff = sHdr->sh_offset;\n> > >>> +\n> > >>> +\t/* Locate .dynsym section header. */\n> > >>> +\tbool found = false;\n> > >>> +\tSecHeader *dynsym;\n> > >>> +\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n> > >>> +\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n> > >>> +\t\tsHdr = elfPointer(map, offset, soSize);\n> > >>> +\t\tif (!sHdr)\n> > >>> +\t\t\treturn -ENOEXEC;\n> > >>> +\n> > >>> +\t\toffset = shnameoff + sHdr->sh_name;\n> > >>> +\t\tchar *name = elfPointer(map, offset, soSize);\n> \n> This should be elfPointer as you want to ensure that at least 8\n> bytes are available (sizeof(\".dynsym\"), including the terminating 0).\n\nAh right, I forgot that sizeof(T) was there.\n\n> > >>> +\t\tif (!name)\n> > >>> +\t\t\treturn -ENOEXEC;\n> > >>> +\n> > >>> +\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n> > >>> +\t\t\tfound = true;\n> > >>> +\t\t\tdynsym = sHdr;\n> \n> You could initialise dynsym to nullptr and remove found.\n> \n> > >>> +\t\t\tbreak;\n> > >>> +\t\t}\n> > >>> +\t}\n> > >>> +\n> > >>> +\tif (!found) {\n> > >>> +\t\tLOG(IPAModule, Error) << \"ELF has no .dynsym section\";\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\t}\n> > >>> +\n> > >>> +\toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n> > >>> +\tsHdr = elfPointer(map, offset, soSize);\n> > >>> +\tif (!sHdr)\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\toff_t dynsym_nameoff = sHdr->sh_offset;\n> > >>> +\n> > >>> +\t/* Locate symbol in the .dynsym section. */\n> > >>> +\tfound = false;\n> > >>> +\tSymHeader *target_symbol;\n> \n> targetSymbol ?\n> \n> > >>> +\tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n> > >>> +\tfor (unsigned int i = 0; i < dynsym_num; i++) {\n> > >>> +\t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n> > >>> +\t\tSymHeader *sym = elfPointer(map, offset, soSize);\n> > >>> +\t\tif (!sym)\n> > >>> +\t\t\treturn -ENOEXEC;\n> > >>> +\n> > >>> +\t\toffset = dynsym_nameoff + sym->st_name;\n> > >>> +\t\tchar *name = elfPointer(map, offset, soSize);\n> \n> Same here, you want elfPointer. I hope this\n> will compile :-)\n> \n> > >>> +\t\tif (!name)\n> > >>> +\t\t\treturn -ENOEXEC;\n> > >>> +\n> > >>> +\t\tif (!strcmp(name, symbol) &&\n> > >>> +\t\t sym->st_info & STB_GLOBAL &&\n> > >>> +\t\t sym->st_size == sizeof(struct IPAModuleInfo)) {\n> > >>\n> > >> I think this should be a check on the size_t size passed in?\n> > > \n> > > Ah yeah, I totally missed that.\n> > \n> > No worries.\n> > \n> > >>> +\t\t\tfound = true;\n> > >>> +\t\t\ttarget_symbol = sym;\n> \n> You could also remove found here.\n> \n> > >>> +\t\t\tbreak;\n> > >>> +\t\t}\n> > >>> +\t}\n> > >>> +\n> > >>> +\tif (!found) {\n> > >>> +\t\tLOG(IPAModule, Error) << \"IPAModuleInfo symbol not found\";\n> > >>\n> > >> This function is called loadSymbol, it doesn't 'know' that it's loading\n> > >> an IPAModuleInfo symbol.\n> > >>\n> > >> I'd change this error to \"Symbol \" << symbol << \" not found\";\n> > > \n> > > And this.\n> > > \n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\t}\n> > >>> +\n> > >>> +\t/* Locate and return data of symbol. */\n> \n> As a sanity check you may want to verify that target_symbol->st_shndx <\n> eHdr->e_shnum. Otherwise none of the elfPointer<> checks below may\n> trigger, and you will load an invalid symbol. This can happen for\n> instance if the symbol is absolute (SHN_ABS).\n> \n> > >>> +\toffset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;\n> > >>> +\tsHdr = elfPointer(map, offset, soSize);\n> > >>> +\tif (!sHdr)\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\toffset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);\n> > >>> +\tchar *data = elfPointer(map, offset, soSize);\n> \n> elfPointer\n> \n> > >>> +\tif (!data)\n> > >>> +\t\treturn -ENOEXEC;\n> > >>> +\tmemcpy(dst, data, size);\n> > >>\n> > >> Oh - interesting, you're copying the symbol out. Once it's mmapped ...\n> > >> why not parse it directly?\n> > > \n> > > I'm not sure what you mean by \"parse\"?\n> > \n> > I mean once you find the symbol and obtain the pointer to it ('data'\n> > here) then you have a pointer to memory which contains the structure.\n> > You could return that pointer, and cast it to the type. Then you can\n> > read the structure members directly.\n> > \n> > But that requires knowing that the symbol doesn't require any further\n> > relocations ... which is fine for our simple info structure.\n> > \n> > It just seems logical in my head to have a function called findSymbol()\n> > which returns a pointer to the symbol, and then (if you're going to copy\n> > it) a function called loadStructure(), which finds the symbol, checks\n> > the size then copies it or such. Maybe it's generalising the functions\n> > more than we care if we will only use them for one thing\n> \n> I have no particular opinion on this. We could indeed rename this\n> function to findSymbol() and move the copy outside. It may actually be a\n> good idea, but then this function should return both the address (or\n> offset) and size (one or both through output arguments).\n\nI don't think we really need to separate it into find and load. I\nsuppose if findSymbol turns out to be useful somewhere down the road it\ncould be changed.\n\n> > >> Can we do all of the 'work' we need to do on the file, and then close it\n> > >> at the destructor?\n> > > \n> > > I don't want to keep the file open/mapped over multiple method calls.\n> > > That's why in my first version there was a constructor that did nothing\n> > > and an init() that did everything (which got moved to the constructor in\n> > > this version).\n> \n> I agree, we shouldn't keep it mapped.\n> \n> > The only things we'll do on this structure is read it and decide if this\n> > library is OK for loading, perhaps even printing the name string in debug.\n> > \n> > Once that has occurred, can't the IPAModuleInfo structure be discarded?\n> > The next thing that we'll do is spawn a container/namespace/safe-thingy\n> > and use the normal dl_open methods to re-open the library there...\n> \n> No, we need it at least to match the module with pipeline handlers, so\n> it needs to be copied. I'm sure we'll have other fields that we will\n> also need to access later.\n> \n> > >> I guess actually this might then keep the whole file mapped in memory\n> > >> for longer which might not be desirable...\n> > >>\n> > >>> +\n> > >>> +\treturn 0;\n> > >>> +}\n> > >>> +\n> > >>> +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)\n> \n> I would store libPath in a member variable as we will need it for\n> dlopen(), and use that member variable in this function.\n> \n> > >>> +{\n> > >>> +\tint fd = open(libPath, O_RDONLY);\n> > >>> +\tif (fd < 0) {\n> > >>> +\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> > >>> +\t\t\t\t << strerror(errno);\n> \n> You need\n> \n> \t\tret = -errno;\n> \t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> \t\t\t\t << strerror(-ret);\n> \t\treturn ret;\n> \n> ass errno may be changed by LOG(IPAModule, Error).\n> \n> > >>> +\t\treturn fd;\n> > >>> +\t}\n> > >>> +\n> > >>> +\tint ret = loadELFIdent(fd);\n> > >>> +\tif (ret)\n> > >>> +\t\tgoto close;\n> > >>> +\n> > >>> +\tsize_t soSize;\n> > >>> +\tchar *map;\n> > >>> +\tstruct stat st;\n> > >>> +\tret = fstat(fd, &st);\n> > >>> +\tif (ret < 0)\n> > >>> +\t\tgoto close;\n> > >>> +\tsoSize = st.st_size;\n> > >>> +\tmap = static_cast(mmap(NULL, soSize, PROT_READ,\n> > >>> +\t\t\t\t MAP_PRIVATE, fd, 0));\n> \n> \tstruct stat st;\n> \tret = fstat(fd, &st);\n> \tif (ret < 0)\n> \t\tgoto close;\n> \n> \tsize_t soSize = st.st_size;\n> \tchar *map = static_cast(mmap(NULL, soSize, PROT_READ,\n> \t\t\t\t\t MAP_PRIVATE, fd, 0));\n\nLast time I tried that the compiler was complaining about the goto\njumping over declarations, or something along those lines. I'll try it\nagain.\n\n> I think I would make it a void *map, pass it as a void * to all the\n> functions, and let elfPointer<> do the case.\n> \n> > >> *iff* the constructor did the open, and the symbol parsing was\n> > >> separated, then I think soSize and map would go to class privates. Then\n> > >> they could be accessed directly by loadSymbol too. But that's an only if...\n> > >>\n> > >> Essentially, it would be\n> > >>\n> > >> IPAModule(\"SO-path\")\n> > >> - Constructor opens the file, performs stat, and mmap.\n> > >> \t (or those become an open() call)\n> > >> - loadIPAModuleInfo() calls loadSymbol(\"ipaModuleInfo\"); and deals only\n> > >> with the ipaModule info structure and checking or such.\n> > > \n> > > If loadIPAModuleInfo() is to be called from within the constructor\n> > > (after open, stat, and mmap), then I agree. I'm not sure map and soSize\n> > > need to be privates though. It could go either way.\n> \n> I thought about it, but it the address and size are really temporary\n> variables, as we unmap the file after the parsing, so I think it's best\n> to pass them explicitly to functions.\n> \n> > Well, I leave that all up to you now :-)\n> > \n> > >>> +\tif (map == MAP_FAILED) {\n> > >>> +\t\tret = -errno;\n> > >>> +\t\tgoto close;\n> > >>> +\t}\n> > >>> +\n> > >>> +\tif (bitClass_ == ELFCLASS32)\n> > >>> +\t\tret = loadSymbol\n> > >>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> > >>> +\telse if (bitClass_ == ELFCLASS64)\n> > >>> +\t\tret = loadSymbol\n> > >>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> \n> As we don't need to support a module with a different bit class than\n> libcamera, would we avoid compiling in both version of the function ?\n> Maybe something like\n> \n> #if sizeof(long) == 4\n> \tret = loadSymbol\n> \t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> #else\n> \tret = loadSymbol\n> \t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> #endif\n> \n> and given that we only need one version of the function, I even wonder\n> if we need to keep it defined as a template function, maybe we could\n> have a\n> \n> #if sizeof(long) == 4\n> #define Elf_Ehdr Elf32_Ehdr\n> #define Elf_Shdr Elf32_Shdr\n> #define Elf_Sym Elf32_Sym\n> #else\n> #define Elf_Ehdr Elf64_Ehdr\n> #define Elf_Shdr Elf64_Shdr\n> #define Elf_Sym Elf64_Sym\n> #endif\n> \n> just before the function definition. This would however require moving\n> the function away from the class to a global function, private to this\n> file, to avoid exposing it in the ipa_module.h header, otherwise we\n> would have to move the above #if's there, which isn't neat. If we do\n> this we should move the loadElfIdent function too. Neither function use\n> any of the class members (except for bitClass_, which isn't needed\n> anymore), so they're essentially static, and separating them from the\n> IPAModule class would open the door to sharing them with other classes\n> if needed later. elfPointer<> is already separate. If you prefer to keep\n> them as class members, they should be declared as static in the class\n> definition.\n\nI was actually thinking about separating them from IPAModule before\nanyway, so I think I'll go this route.\n\n> > >>> +\tif (ret)\n> > >>> +\t\tgoto unmap;\n> > >>> +\n> > >>> +unmap:\n> > >>> +\tmunmap(map, soSize);\n> > >>> +close:\n> > >>> +\tclose(fd);\n> > >>> +\treturn ret;\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Check if construction of the IPAModule instance succeeded\n> \n> I would say \"Check if the IPAModule instance is valid\" and then add a\n> paragraph to detail what a valid (or invalid) IPAModule is.\n> \n> > >>> + *\n> > >>> + * \\return true if the constructor succeeded with no errors, false otherwise\n> \n> true if the IPAModule is valid, false otherwise\n> \n> > >>> + */\n> > >>> +\n> \n> No need for this blank line.\n\nHuh, I thought I saw some other file do this. Maybe I missaw :/\n\n> > >>> +bool IPAModule::isValid() const\n> > >>> +{\n> > >>> +\treturn loaded_;\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Get the loaded IPAModuleInfo\n> \n> \"Get the IPA module information\"\n> \n> > >>> + *\n> > >>> + * Check isValid() before calling this.\n> \n> \"The content of the IPA module information is loaded from the module,\n> and is valid only if the module is valid (as returned by isValid()).\n> Calling this function on an invalid module is an error.\"\n> \n> > >>> + *\n> > >>> + * \\return IPAModuleInfo\n> > >>> + */\n> > >>> +\n> \n> No need for a blank line here either\n> \n> > >>> +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const\n> > >>> +{\n> > >>> +\treturn info_;\n> > >>> +}\n> > >>> +\n> > >>> +} /* namespace libcamera */\n> > >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > >>> index 8796f49..e5b48f2 100644\n> > >>> --- a/src/libcamera/meson.build\n> > >>> +++ b/src/libcamera/meson.build\n> > >>> @@ -10,6 +10,7 @@ libcamera_sources = files([\n> > >>> 'event_notifier.cpp',\n> > >>> 'formats.cpp',\n> > >>> 'geometry.cpp',\n> > >>> + 'ipa_module.cpp',\n> > >>> 'log.cpp',\n> > >>> 'media_device.cpp',\n> > >>> 'media_object.cpp',\n> > >>> @@ -31,6 +32,7 @@ libcamera_headers = files([\n> > >>> 'include/device_enumerator_udev.h',\n> > >>> 'include/event_dispatcher_poll.h',\n> > >>> 'include/formats.h',\n> > >>> + 'include/ipa_module.h',\n> > >>> 'include/log.h',\n> > >>> 'include/media_device.h',\n> > >>> 'include/media_object.h',\n\nEverything else is either an ack or an affirmative.\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 76E7E60DE9\n\tfor ;\n\tThu, 16 May 2019 19:51:24 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35E772FD;\n\tThu, 16 May 2019 19:51:23 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558029083;\n\tbh=3+dWSf0UQNGGLOVIpztpwQBVjz7oFkLk8IPaKos1bu0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h6Y530uWbXCMkTPd6j9j0dyBXoXXTRN0yiYtIwB0DW5r8G2jxEdCk4Mu6r/v/ajJC\n\tyFsGSyPVo85JgsmLrikP6LzXq/Vfcn2nS0tm+FQ52yi9mv7iMSnw43kKfUVJzRiu2w\n\tBPFllkN6NKJeqhagXJlYNwB0QdbV1XIC6zeUtsuI=","Date":"Thu, 16 May 2019 13:51:16 -0400","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190516175116.GG12325@localhost.localdomain>","References":"<20190514223808.27351-1-paul.elder@ideasonboard.com>\n\t<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>\n\t<20190515150253.GF12325@localhost.localdomain>\n\t<27f62fbf-1f80-af57-10c9-75f94d87487f@ideasonboard.com>\n\t<20190515211942.GA4991@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20190515211942.GA4991@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","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":"Thu, 16 May 2019 17:51:24 -0000"}},{"id":1603,"web_url":"https://patchwork.libcamera.org/comment/1603/","msgid":"<20190516181655.GT14820@pendragon.ideasonboard.com>","date":"2019-05-16T18:16:55","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Thu, May 16, 2019 at 01:51:16PM -0400, Paul Elder wrote:\n> On Thu, May 16, 2019 at 12:19:42AM +0300, Laurent Pinchart wrote:\n> > On Wed, May 15, 2019 at 04:26:48PM +0100, Kieran Bingham wrote:\n> >> On 15/05/2019 16:02, Paul Elder wrote:\n> >>> On Wed, May 15, 2019 at 10:28:46AM +0100, Kieran Bingham wrote:\n> >>>> Hi Paul,\n> >>>>\n> >>>> Thank you for your patch, This is looking good and it's going in the\n> >>>> right direction but I have a few design comments ... feel free to\n> >>>> disagree though ...\n> >>> \n> >>> Thank you for the review.\n> >>> \n> >>>> On 14/05/2019 23:38, Paul Elder wrote:\n> >>>>> Implement a class to load a struct IPAModuleInfo of a given symbol name\n> >>>>> from a .so shared object library.\n> >>>>>\n> >>>>> Signed-off-by: Paul Elder \n> >>>>> ---\n> >>>>> Changes in v2:\n> >>>>> - renamed LibLoader class to IPAModule\n> >>>>> - added documentation\n> >>>>> - added logging\n> >>>>> - check that bitness of the shared object is the same as libcamera\n> >>>>> - moved symbol loading (\"init\") to the constructor, and added isValid()\n> >>>>> - added elfPointer() to prevent segfaults when reading data from mmap\n> >>>>> - moved struct IPAModuleInfo out of IPAModule\n> >>>>> - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a\n> >>>>> const reference\n> >>>>> - added munmap after the mmap\n> >>>>>\n> >>>>> src/libcamera/include/ipa_module.h | 43 +++++\n> >>>>> src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++\n> >>>>> src/libcamera/meson.build | 2 +\n> >>>>> 3 files changed, 322 insertions(+)\n> >>>>> create mode 100644 src/libcamera/include/ipa_module.h\n> >>>>> create mode 100644 src/libcamera/ipa_module.cpp\n> >>>>>\n> >>>>> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> >>>>> new file mode 100644\n> >>>>> index 0000000..9eb0094\n> >>>>> --- /dev/null\n> >>>>> +++ b/src/libcamera/include/ipa_module.h\n> >>>>> @@ -0,0 +1,43 @@\n> >>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>>> +/*\n> >>>>> + * Copyright (C) 2019, Google Inc.\n> >>>>> + *\n> >>>>> + * ipa_module.h - load IPA module information from shared library at runtime\n> >>>>\n> >>>> Hrm ... we have two sides here.\n> >>>>\n> >>>> We need a public header which defines the interface between an IPA\n> >>>> module and libcamera. That would include a struct IPAModuleInfo and any\n> >>>> registration details, but should not include any internal libcamera\n> >>>> private details regarding how the module is loaded.\n> >>> \n> >>> Yes, I agree.\n> >>> \n> >>>>> + */\n> >>>>> +#ifndef __LIBCAMERA_IPA_MODULE_H__\n> >>>>> +#define __LIBCAMERA_IPA_MODULE_H__\n> >>>>> +\n> >>>>> +#include \n> >>>>> +\n> >>>>> +namespace libcamera {\n> >>>>> +\n> >>>>> +struct IPAModuleInfo {\n> >>>>> +\tchar name[256];\n> >>>>> +\tunsigned int version;\n> >>>>> +};\n> >>>>> +\n> >>>>\n> >>>> So IPModuleInfo (and then later, the class definition for how a\n> >>>> developer would construct an IPA Module) should live in the public\n> >>>> headers at:\n> >>>>\n> >>>> /include/libcamera/ipa_module.h\n> >>> \n> >>> Yeah.\n> >>> \n> >>> Then, where should class IPAModule go?\n> >> \n> >> Stays here in this file I think :-)\n> > \n> > That's what I had envisioned, so it sounds good to me.\n> > \n> >>> \n> >>>>> +class IPAModule\n> >> \n> >> I'm wondering if we have two sides if this class should be calls\n> >> IPAModule though.\n> >> \n> >> Surely an IPAModule would implement a class called IPAModule, and\n> >> perhaps 'this' class is an IPAModuleParser? IPAModuleLoader?\n> >> \n> >> Probably a Loader :D\n> > \n> > I was wondering about this. One option would indeed to provide an\n> > IPAModule class with pure virtual functions that the module would need\n> > to implement. Another option would be to use a structure with C-style\n> > function pointers. I was thinking about the latter, but the former is\n> > not a bad idea. However, we need to take care about backward\n> > compatibility. Can a module compiled with an old version of g++ (or\n> > clang++) be incompatible with libcamera built with a newer version if\n> > using a C++ ABI ?\n> > \n> > Ie we go the C++ way, we will indeed need two different names for the\n> > classes. I was thinking about IPAModule for this class and\n> > IPAModuleInterface for the new one, in which case no change would be\n> > needed here. Note that IPAModule would inherit from IPAModuleInterface\n> > as it would provide the IPA API to pipeline handlers. I don't like the\n> > name IPAModuleLoader much, as from a pipeline handler point of view\n> > we're using modules, not module loaders. Other options may be possible.\n> \n> I don't know about C++ ABI compatability. I think to just avoid the risk\n> itself we should go with C-style function pointers.\n\nI think the entry point at least should be a factory C function pointer,\nbut it could return a C++ object pointer, if we can guarantee ABI\ncompatibility.\n\n> In which case Laurent's shim idea would work well for process isolation\n> later on. Since the only layers are Pipeline -> IPAModule there isn't\n> really anywhere else we can break off into another process without\n> making the Pipeline -> IPAModule interface ugly.\n> \n> >>>>> +{\n> >>>>> +public:\n> >>>>> +\texplicit IPAModule(const std::string &libPath, const std::string &symbol);\n> > \n> > No need to pass the symbol name here, you can hardcode it in the\n> > implementation. All modules will use the same symbol name.\n> > \n> >>>>> +\n> >>>>> +\tbool isValid() const;\n> >>>>> +\n> >>>>> +\tstruct IPAModuleInfo const &IPAModuleInfo() const;\n> > \n> > We usually write this const struct IPAModuleInfo &IPAModuleInfo() const.\n> > And I would name the function info(), as IPAModule::info() makes it\n> > clear that we're retrieving the information of an IPA module.\n> > \n> >>>>> +\n> >>>>> +private:\n> >>>>> +\tstruct IPAModuleInfo info_;\n> >>>>> +\n> >>>>> +\tbool loaded_;\n> > \n> > Let's name this valid_ to match isValid(). We will later have a load()\n> > function to dlopen() the module, and likely a loaded_ flag to match\n> > that.\n> > \n> >>>>> +\tint bitClass_;\n> >>>>> +\n> >>>>> +\tint loadELFIdent(int fd);\n> >>>>> +\ttemplate\n> >>>>> +\tint loadSymbol(void *data, size_t size, char *map, size_t soSize,\n> >>>>> +\t\t const char *symbol);\n> >>>>> +\tint loadIPAModuleInfo(const char *libPath, const char *symbol);\n> >>>>> +};\n> >>>>> +\n> >>>>> +} /* namespace libcamera */\n> >>>>> +\n> >>>>> +#endif /* __LIBCAMERA_IPA_MODULE_H__ */\n> >>>>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> >>>>> new file mode 100644\n> >>>>> index 0000000..5ca16e8\n> >>>>> --- /dev/null\n> >>>>> +++ b/src/libcamera/ipa_module.cpp\n> >>>>> @@ -0,0 +1,277 @@\n> >>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>>> +/*\n> >>>>> + * Copyright (C) 2019, Google Inc.\n> >>>>> + *\n> >>>>> + * ipa_module.cpp - load IPA module information from shared library at runtime\n> > \n> > I think we can already update the description to \"Image Processing\n> > Algorithm module\" or similar\n> > \n> >>>>> + */\n> >>>>> +\n> >>>>> +#include \"ipa_module.h\"\n> >>>>> +\n> >>>>> +#include \n> >>>>> +#include \n> >>>>> +#include \n> >>>>> +#include \n> >>>>> +#include \n> >>>>> +#include \n> >>>>> +#include \n> >>>>> +#include \n> >>>>> +\n> >>>>> +#include \n> >>>>> +\n> >>>>> +#include \"log.h\"\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\file ipa_module.h\n> >>>>> + * \\brief IPA module information loading\n> > \n> > Same here.\n> > \n> >>>>> + */\n> >>>>> +\n> >>>>> +namespace libcamera {\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\struct IPAModuleInfo\n> >>>>> + * \\brief Information of an IPA plugin\n> >>>>> + *\n> >>>>> + * This structure contains the information of an IPA plugin. It is loaded,\n> >>>>> + * read, and validated before anything else is loaded from the plugin.\n> >>>>> + *\n> >>>>> + * \\var IPAModuleInfo::name\n> >>>>> + * \\brief The name of the IPA plugin\n> >>>>> + *\n> >>>>> + * \\var IPAModuleInfo::version\n> >>>>> + * \\brief The version of the IPA plugin\n> >>>>\n> >>>> I don't think we need to store the 'version' of the plugin, so much as\n> >>>> the version of the API it was compiled against to ensure that it is\n> >>>> compatible with this 'running' instance of libcamera.\n> >>>>\n> >>>> I.e. We don't care that it's the 3rd iteration of the rk3399 module -\n> >>>> but we do care that it is compatible with the API defined in Libcamera 1.0.\n> >>> \n> >>> Yeah, good point; I agree.\n> >>> \n> >>> How would we get the API version of libcamera?\n> >> \n> >> You get to define it :-)\n> >> \n> >> To start with it's probably just:\n> >> \n> >> #define LIBCAMERA_IPA_API_VERSION 1\n> >> \n> >> This would be a 'local' IPAModule API version number or such I think, as\n> >> our IPA modules must go through a defined interface...\n> >> \n> >> Any time an ABI break occurs in the IPAModule 'API' we would have to\n> >> bump up the apiversion.\n> > \n> > I agree, but I think we can defer this. The IPAModuleInfo structure is\n> > currently a mock up to start implementing the IPAModule class, I expect\n> > it to change a lot.\n> > \n> >>>> Maybe we should add compatible strings to match against pipeline\n> >>>> handlers as some point too :-) \n> > \n> > I think we will need that :-)\n> > \n> > Paul, could you record there open items ?\n> \n> I'm not sure what you mean.\n\nSorry, s/there/the/\n\nCould you keep a list of the todo items (possibly as \\todo comments),\nsuch as adding compability strings to the IPA module info ?\n\n> >>>>> + */\n> >>>>> +\n> >>>>> +LOG_DEFINE_CATEGORY(IPAModule)\n> >>>>> +\n> >>>>> +template\n> >>>>> +typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n> >>>>> +\t\t\t\t\t\t size_t fileSize)\n> >>>>> +{\n> >>>>> +\tsize_t size = offset + sizeof(T);\n> >>>>> +\tif (size > fileSize || size < sizeof(T))\n> >>>>> +\t\treturn nullptr;\n> >>>>> +\n> >>>>> +\treturn reinterpret_cast::type *>\n> >>>>> +\t\t(static_cast(map) + offset);\n> >>>>> +}\n> > \n> > This function should be static. The C way of doing so is using the\n> > static keyword, the C++ way is to put it in an anonymous namespace.\n> > \n> > namespace {\n> > \n> > template\n> > typename std::remove_extent::type *elfPointer(void *map, off_t offset,\n> > \t\t\t\t\t\t size_t fileSize)\n> > {\n> > ...\n> > }\n> > \n> > };\n> > \n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\class IPAModule\n> >>>>> + * \\brief Load IPA module information from an IPA plugin\n> > \n> > To match the suggestion above, \"IPA module wrapper\" ? I think we can do\n> > better than that, maybe \"Wrapper around an IPA module shared object\" ?\n> > Suggestions are welcome.\n> > \n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Construct an IPAModule instance\n> >>>>> + * \\param[in] libPath path to IPA plugin\n> >>>>> + * \\param[in] symbol name of IPAModuleInfo to load from libPath\n> >>>>> + *\n> >>>>> + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.\n> >>>>> + * The IPA plugin shared object file must be of the same endianness and\n> >>>>> + * bitness as libcamera.\n> >>>>> + *\n> >>>>> + * Note that isValid() should be called to check if this loading was successful\n> >>>>> + * or not.\n> >>>>\n> >>>> Hrm ... this is a slightly different design pattern than the rest of our\n> >>>> objects which are always constructable, and then actions later can fail.\n> >>>>\n> >>>> I don't mind - but I wonder if we should add some documentation\n> >>>> regarding our design patterns somewhere.\n> >>>>\n> >>>> (not an action on this patch)\n> > \n> > I've discussed this with Paul, and I think that in this case this\n> > implementation makes sense. There's no need to delay loading, and as the\n> > IPAModule class is a wrapper around a .so, constructing an instance on\n> > which on can call isValid() to check if the IPA is valid makes sense to\n> > me.\n> > \n> > Maybe we need to reconsider our other classes indeed. Any\n> > suggestion/preference ?\n> \n> Do you mean to make the other classes follow the same model? As long as\n> we document the exception (well the usage is already documented...) then\n> shouldn't it be fine?\n\nSure, it's fine having two models when two models are needed, the\nquestion is whether they are actually needed :-) Some of our existing\nclasses may benefit from using the model used here.\n\n> >>>>> + */\n> >>>>> +\n> >>>>> +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)\n> >>>>> +\t: loaded_(false)\n> >>>>\n> >>>> So actually, from my text below - what I'm really saying is that I don't\n> >>>> think you should provide a &symbol to this IPAModule constructor :)\n> > \n> > So we agree :-)\n> > \n> >>>>> +{\n> >>>>> +\tint ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());\n> >>>>\n> >>>> I see that you have moved the load into this constructor based on\n> >>>> comments from Laurent in v1. I think that's fine, but I think merging\n> >>>> the loading of the object, and the parsing of the symbol might be\n> >>>> combining too much. But it depends (as ever).\n> > \n> > Note that this only loads the module info, not the module itself (I\n> > foresee a load() function that will dlopen() the module). I'm not sure\n> > what you mean by merging the loading of the object and the parsing of\n> > the symbol.\n> > \n> >>>> For efficiency, the constructor can load the file once and when created\n> >>>> and .isValid() is true, then we know we can successfully parse symbols.\n> > \n> > But you can't know it's a valid IPA unless you can load the\n> > IPAModuleInfo successfully (and later maybe even cryptographically\n> > verifying the module).\n> > \n> >>>> But I think loadIPAModuleInfo() should be 'hardcoded' to know exactly\n> >>>> which symbol name it will expect for an IPAModule. Think of it like a\n> >>>> 'main()' entry point for our IPAModule. You would always expect a C\n> >>>> binary to have a main() symbol...\n> >>> \n> >>> Yeah, I think that's a good idea.\n> >>> \n> >>>> So I would expect a libcamera IPA module to have a registration\n> >>>> something like:\n> >>>>\n> >>>>\n> >>>> /* Register our module with Libcamera */\n> >>>> const struct IPAModuleInfo ipaModuleInfo = {\n> >>>> \t.name = \"Image Processing for the RK3399\",\n> >>>> \t.version = 1, /* Perhaps this should be apiversion to prevent\n> >>>> * people thinking it describes the version of the\n> >>>> * module ? ...\n> >>>> */\n> >>>> };\n> >>> \n> >>> Should the info entry point be called ipaModuleInfo?\n> >> \n> >> I think variables are in camelCase with a lowerCase first letter in the\n> >> project right? or was this not quite your question?\n> > \n> > This sounds good to me.\n> \n> My question was about the entry point name, not the casing :p\n\nAh :-) yes, ipaModuleInfo seems good to me.\n\n> >>>> This could even then be turned into a macro:\n> >>>>\n> >>>> #define LIBCAMERA_IPA_MODULE(__NAME) \\\n> >>>> const struct IPAModuleInfo ipaModuleInfo = { \\\n> >>>> \t.name = __NAME,\t\\\n> >>>> \t.apiversion = 1, \\\n> >>>> }\n> > \n> > We'll have to throw an extern \"C\" here, otherwise your symbol name will\n> > be _ZL13ipaModuleInfo with g++.\n> > \n> >>>>\n> >>>> so that a module rk3399_ipa.cpp could define:\n> >>>>\n> >>>> LIBCAMERA_IPA_MODULE(\"Image Processing Algorithms for the RK3399\");\n> >>>>\n> >>>> (apiversion would be hardcoded by the macro because it defines what\n> >>>> version of the API/libraries it's compiled against..., and we check it's\n> >>>> compatible at runtime)\n> > \n> > Something like that. If the number of fields grows, and if some of them\n> > become complex, we may want to fill the structure manually, be we should\n> > still use a macro to instantiate the right structure with the right name\n> > and the extern \"C\" keyword.\n> \n> Yeah I think I'll do that.\n> \n> >>>>> +\tif (ret < 0) {\n> >>>>> +\t\tloaded_ = false;\n> > \n> > This isn't needed, you initialise loaded_ to false above.\n> > \n> >>>>> +\t\treturn;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tloaded_ = true;\n> >>>>> +}\n> >>>>> +\n> >>>>> +int IPAModule::loadELFIdent(int fd)\n> > \n> > This function doesn't actually load much, how about renaming it to\n> > verifyELFIdent() ?\n> > \n> >>>>> +{\n> >>>>> +\tint ret = lseek(fd, 0, SEEK_SET);\n> >>>>> +\tif (ret < 0)\n> >>>>> +\t\treturn -errno;\n> >>>>> +\n> > \n> > As we're mmap()ing the whole .so, you could mmap() before calling\n> > loadELFIdent(), and pass the void *map and size to the function, instead\n> > of using read.\n> \n> I didn't want to have to do elfPointer for every single byte that I read\n> it, but I forgot that I could just use elfPointer on an array.\n> \n> >>>>> +\tunsigned char e_ident[EI_NIDENT];\n> >>>>> +\tret = read(fd, e_ident, EI_NIDENT);\n> >>>>> +\tif (ret < 0)\n> >>>>> +\t\treturn -errno;\n> >>>>> +\n> >>>>> +\tif (e_ident[EI_MAG0] != ELFMAG0 ||\n> >>>>> +\t e_ident[EI_MAG1] != ELFMAG1 ||\n> >>>>> +\t e_ident[EI_MAG2] != ELFMAG2 ||\n> >>>>> +\t e_ident[EI_MAG3] != ELFMAG3 ||\n> >>>>> +\t e_ident[EI_VERSION] != EV_CURRENT)\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\n> >>>>> +\tif ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||\n> >>>>> +\t (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))\n> >>>>> +\t\tbitClass_ = e_ident[EI_CLASS];\n> >>>>> +\telse\n> >>>>> +\t\treturn -ENOEXEC;\n> > \n> > How about\n> > \n> > \tbitClass_ = sizeof(unsigned long) == 4 ? ELFCLASS32 : ELFCLASS64;\n> > \tif (e_ident[EI_CLASS] != bitClass_)\n> > \t\treturn -ENOEXEC;\n> \n> Yeah. I guess we're going to demote bitClass_ to a local variable.\n> \n> >>>>> +\n> >>>>> +\tint a = 1;\n> >>>>> +\tunsigned char endianness = *reinterpret_cast(&a) == 1\n> >>>>> +\t\t\t\t ? ELFDATA2LSB : ELFDATA2MSB;\n> > \n> > So you didn't like my union-based solution ? :-)\n> \n> Not really :p\n> imo this was more readable.\n\nYour code, your choice :-)\n\n> >>>>> +\tif (e_ident[EI_DATA] != endianness)\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\n> >>>>> +\treturn 0;\n> >>>>> +}\n> >>>>> +\n> >>>>> +template\n> >>>>> +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,\n> >>>>> +\t\t\t const char *symbol)\n> >>>>> +{\n> >>>>> +\tElfHeader *eHdr = elfPointer(map, 0, soSize);\n> >>>>> +\tif (!eHdr)\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\n> >>>>> +\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n> >>>>> +\tSecHeader *sHdr = elfPointer(map, offset, soSize);\n> >>>>> +\tif (!sHdr)\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\toff_t shnameoff = sHdr->sh_offset;\n> >>>>> +\n> >>>>> +\t/* Locate .dynsym section header. */\n> >>>>> +\tbool found = false;\n> >>>>> +\tSecHeader *dynsym;\n> >>>>> +\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n> >>>>> +\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n> >>>>> +\t\tsHdr = elfPointer(map, offset, soSize);\n> >>>>> +\t\tif (!sHdr)\n> >>>>> +\t\t\treturn -ENOEXEC;\n> >>>>> +\n> >>>>> +\t\toffset = shnameoff + sHdr->sh_name;\n> >>>>> +\t\tchar *name = elfPointer(map, offset, soSize);\n> > \n> > This should be elfPointer as you want to ensure that at least 8\n> > bytes are available (sizeof(\".dynsym\"), including the terminating 0).\n> \n> Ah right, I forgot that sizeof(T) was there.\n> \n> >>>>> +\t\tif (!name)\n> >>>>> +\t\t\treturn -ENOEXEC;\n> >>>>> +\n> >>>>> +\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n> >>>>> +\t\t\tfound = true;\n> >>>>> +\t\t\tdynsym = sHdr;\n> > \n> > You could initialise dynsym to nullptr and remove found.\n> > \n> >>>>> +\t\t\tbreak;\n> >>>>> +\t\t}\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tif (!found) {\n> >>>>> +\t\tLOG(IPAModule, Error) << \"ELF has no .dynsym section\";\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n> >>>>> +\tsHdr = elfPointer(map, offset, soSize);\n> >>>>> +\tif (!sHdr)\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\toff_t dynsym_nameoff = sHdr->sh_offset;\n> >>>>> +\n> >>>>> +\t/* Locate symbol in the .dynsym section. */\n> >>>>> +\tfound = false;\n> >>>>> +\tSymHeader *target_symbol;\n> > \n> > targetSymbol ?\n> > \n> >>>>> +\tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n> >>>>> +\tfor (unsigned int i = 0; i < dynsym_num; i++) {\n> >>>>> +\t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n> >>>>> +\t\tSymHeader *sym = elfPointer(map, offset, soSize);\n> >>>>> +\t\tif (!sym)\n> >>>>> +\t\t\treturn -ENOEXEC;\n> >>>>> +\n> >>>>> +\t\toffset = dynsym_nameoff + sym->st_name;\n> >>>>> +\t\tchar *name = elfPointer(map, offset, soSize);\n> > \n> > Same here, you want elfPointer. I hope this\n> > will compile :-)\n> > \n> >>>>> +\t\tif (!name)\n> >>>>> +\t\t\treturn -ENOEXEC;\n> >>>>> +\n> >>>>> +\t\tif (!strcmp(name, symbol) &&\n> >>>>> +\t\t sym->st_info & STB_GLOBAL &&\n> >>>>> +\t\t sym->st_size == sizeof(struct IPAModuleInfo)) {\n> >>>>\n> >>>> I think this should be a check on the size_t size passed in?\n> >>> \n> >>> Ah yeah, I totally missed that.\n> >> \n> >> No worries.\n> >> \n> >>>>> +\t\t\tfound = true;\n> >>>>> +\t\t\ttarget_symbol = sym;\n> > \n> > You could also remove found here.\n> > \n> >>>>> +\t\t\tbreak;\n> >>>>> +\t\t}\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tif (!found) {\n> >>>>> +\t\tLOG(IPAModule, Error) << \"IPAModuleInfo symbol not found\";\n> >>>>\n> >>>> This function is called loadSymbol, it doesn't 'know' that it's loading\n> >>>> an IPAModuleInfo symbol.\n> >>>>\n> >>>> I'd change this error to \"Symbol \" << symbol << \" not found\";\n> >>> \n> >>> And this.\n> >>> \n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\t/* Locate and return data of symbol. */\n> > \n> > As a sanity check you may want to verify that target_symbol->st_shndx <\n> > eHdr->e_shnum. Otherwise none of the elfPointer<> checks below may\n> > trigger, and you will load an invalid symbol. This can happen for\n> > instance if the symbol is absolute (SHN_ABS).\n> > \n> >>>>> +\toffset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;\n> >>>>> +\tsHdr = elfPointer(map, offset, soSize);\n> >>>>> +\tif (!sHdr)\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\toffset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);\n> >>>>> +\tchar *data = elfPointer(map, offset, soSize);\n> > \n> > elfPointer\n> > \n> >>>>> +\tif (!data)\n> >>>>> +\t\treturn -ENOEXEC;\n> >>>>> +\tmemcpy(dst, data, size);\n> >>>>\n> >>>> Oh - interesting, you're copying the symbol out. Once it's mmapped ...\n> >>>> why not parse it directly?\n> >>> \n> >>> I'm not sure what you mean by \"parse\"?\n> >> \n> >> I mean once you find the symbol and obtain the pointer to it ('data'\n> >> here) then you have a pointer to memory which contains the structure.\n> >> You could return that pointer, and cast it to the type. Then you can\n> >> read the structure members directly.\n> >> \n> >> But that requires knowing that the symbol doesn't require any further\n> >> relocations ... which is fine for our simple info structure.\n> >> \n> >> It just seems logical in my head to have a function called findSymbol()\n> >> which returns a pointer to the symbol, and then (if you're going to copy\n> >> it) a function called loadStructure(), which finds the symbol, checks\n> >> the size then copies it or such. Maybe it's generalising the functions\n> >> more than we care if we will only use them for one thing\n> > \n> > I have no particular opinion on this. We could indeed rename this\n> > function to findSymbol() and move the copy outside. It may actually be a\n> > good idea, but then this function should return both the address (or\n> > offset) and size (one or both through output arguments).\n> \n> I don't think we really need to separate it into find and load. I\n> suppose if findSymbol turns out to be useful somewhere down the road it\n> could be changed.\n\nI agree. Please however keep in mind that separating them already would\nmake the find function a bit smaller, so the code may become more\nreadable (especially as we would be closer to the \"one function, one\npurpose\" idiom). Up to you.\n\n> >>>> Can we do all of the 'work' we need to do on the file, and then close it\n> >>>> at the destructor?\n> >>> \n> >>> I don't want to keep the file open/mapped over multiple method calls.\n> >>> That's why in my first version there was a constructor that did nothing\n> >>> and an init() that did everything (which got moved to the constructor in\n> >>> this version).\n> > \n> > I agree, we shouldn't keep it mapped.\n> > \n> >> The only things we'll do on this structure is read it and decide if this\n> >> library is OK for loading, perhaps even printing the name string in debug.\n> >> \n> >> Once that has occurred, can't the IPAModuleInfo structure be discarded?\n> >> The next thing that we'll do is spawn a container/namespace/safe-thingy\n> >> and use the normal dl_open methods to re-open the library there...\n> > \n> > No, we need it at least to match the module with pipeline handlers, so\n> > it needs to be copied. I'm sure we'll have other fields that we will\n> > also need to access later.\n> > \n> >>>> I guess actually this might then keep the whole file mapped in memory\n> >>>> for longer which might not be desirable...\n> >>>>\n> >>>>> +\n> >>>>> +\treturn 0;\n> >>>>> +}\n> >>>>> +\n> >>>>> +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)\n> > \n> > I would store libPath in a member variable as we will need it for\n> > dlopen(), and use that member variable in this function.\n> > \n> >>>>> +{\n> >>>>> +\tint fd = open(libPath, O_RDONLY);\n> >>>>> +\tif (fd < 0) {\n> >>>>> +\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> >>>>> +\t\t\t\t << strerror(errno);\n> > \n> > You need\n> > \n> > \t\tret = -errno;\n> > \t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> > \t\t\t\t << strerror(-ret);\n> > \t\treturn ret;\n> > \n> > ass errno may be changed by LOG(IPAModule, Error).\n> > \n> >>>>> +\t\treturn fd;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tint ret = loadELFIdent(fd);\n> >>>>> +\tif (ret)\n> >>>>> +\t\tgoto close;\n> >>>>> +\n> >>>>> +\tsize_t soSize;\n> >>>>> +\tchar *map;\n> >>>>> +\tstruct stat st;\n> >>>>> +\tret = fstat(fd, &st);\n> >>>>> +\tif (ret < 0)\n> >>>>> +\t\tgoto close;\n> >>>>> +\tsoSize = st.st_size;\n> >>>>> +\tmap = static_cast(mmap(NULL, soSize, PROT_READ,\n> >>>>> +\t\t\t\t MAP_PRIVATE, fd, 0));\n> > \n> > \tstruct stat st;\n> > \tret = fstat(fd, &st);\n> > \tif (ret < 0)\n> > \t\tgoto close;\n> > \n> > \tsize_t soSize = st.st_size;\n> > \tchar *map = static_cast(mmap(NULL, soSize, PROT_READ,\n> > \t\t\t\t\t MAP_PRIVATE, fd, 0));\n> \n> Last time I tried that the compiler was complaining about the goto\n> jumping over declarations, or something along those lines. I'll try it\n> again.\n\nOops, you're right. That's a pesky error :-(\n\n> > I think I would make it a void *map, pass it as a void * to all the\n> > functions, and let elfPointer<> do the case.\n> > \n> >>>> *iff* the constructor did the open, and the symbol parsing was\n> >>>> separated, then I think soSize and map would go to class privates. Then\n> >>>> they could be accessed directly by loadSymbol too. But that's an only if...\n> >>>>\n> >>>> Essentially, it would be\n> >>>>\n> >>>> IPAModule(\"SO-path\")\n> >>>> - Constructor opens the file, performs stat, and mmap.\n> >>>> \t (or those become an open() call)\n> >>>> - loadIPAModuleInfo() calls loadSymbol(\"ipaModuleInfo\"); and deals only\n> >>>> with the ipaModule info structure and checking or such.\n> >>> \n> >>> If loadIPAModuleInfo() is to be called from within the constructor\n> >>> (after open, stat, and mmap), then I agree. I'm not sure map and soSize\n> >>> need to be privates though. It could go either way.\n> > \n> > I thought about it, but it the address and size are really temporary\n> > variables, as we unmap the file after the parsing, so I think it's best\n> > to pass them explicitly to functions.\n> > \n> >> Well, I leave that all up to you now :-)\n> >> \n> >>>>> +\tif (map == MAP_FAILED) {\n> >>>>> +\t\tret = -errno;\n> >>>>> +\t\tgoto close;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tif (bitClass_ == ELFCLASS32)\n> >>>>> +\t\tret = loadSymbol\n> >>>>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> >>>>> +\telse if (bitClass_ == ELFCLASS64)\n> >>>>> +\t\tret = loadSymbol\n> >>>>> +\t\t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> > \n> > As we don't need to support a module with a different bit class than\n> > libcamera, would we avoid compiling in both version of the function ?\n> > Maybe something like\n> > \n> > #if sizeof(long) == 4\n> > \tret = loadSymbol\n> > \t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> > #else\n> > \tret = loadSymbol\n> > \t\t\t(&info_, sizeof(info_), map, soSize, symbol);\n> > #endif\n> > \n> > and given that we only need one version of the function, I even wonder\n> > if we need to keep it defined as a template function, maybe we could\n> > have a\n> > \n> > #if sizeof(long) == 4\n> > #define Elf_Ehdr Elf32_Ehdr\n> > #define Elf_Shdr Elf32_Shdr\n> > #define Elf_Sym Elf32_Sym\n> > #else\n> > #define Elf_Ehdr Elf64_Ehdr\n> > #define Elf_Shdr Elf64_Shdr\n> > #define Elf_Sym Elf64_Sym\n> > #endif\n> > \n> > just before the function definition. This would however require moving\n> > the function away from the class to a global function, private to this\n> > file, to avoid exposing it in the ipa_module.h header, otherwise we\n> > would have to move the above #if's there, which isn't neat. If we do\n> > this we should move the loadElfIdent function too. Neither function use\n> > any of the class members (except for bitClass_, which isn't needed\n> > anymore), so they're essentially static, and separating them from the\n> > IPAModule class would open the door to sharing them with other classes\n> > if needed later. elfPointer<> is already separate. If you prefer to keep\n> > them as class members, they should be declared as static in the class\n> > definition.\n> \n> I was actually thinking about separating them from IPAModule before\n> anyway, so I think I'll go this route.\n> \n> >>>>> +\tif (ret)\n> >>>>> +\t\tgoto unmap;\n> >>>>> +\n> >>>>> +unmap:\n> >>>>> +\tmunmap(map, soSize);\n> >>>>> +close:\n> >>>>> +\tclose(fd);\n> >>>>> +\treturn ret;\n> >>>>> +}\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Check if construction of the IPAModule instance succeeded\n> > \n> > I would say \"Check if the IPAModule instance is valid\" and then add a\n> > paragraph to detail what a valid (or invalid) IPAModule is.\n> > \n> >>>>> + *\n> >>>>> + * \\return true if the constructor succeeded with no errors, false otherwise\n> > \n> > true if the IPAModule is valid, false otherwise\n> > \n> >>>>> + */\n> >>>>> +\n> > \n> > No need for this blank line.\n> \n> Huh, I thought I saw some other file do this. Maybe I missaw :/\n> \n> >>>>> +bool IPAModule::isValid() const\n> >>>>> +{\n> >>>>> +\treturn loaded_;\n> >>>>> +}\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Get the loaded IPAModuleInfo\n> > \n> > \"Get the IPA module information\"\n> > \n> >>>>> + *\n> >>>>> + * Check isValid() before calling this.\n> > \n> > \"The content of the IPA module information is loaded from the module,\n> > and is valid only if the module is valid (as returned by isValid()).\n> > Calling this function on an invalid module is an error.\"\n> > \n> >>>>> + *\n> >>>>> + * \\return IPAModuleInfo\n> >>>>> + */\n> >>>>> +\n> > \n> > No need for a blank line here either\n> > \n> >>>>> +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const\n> >>>>> +{\n> >>>>> +\treturn info_;\n> >>>>> +}\n> >>>>> +\n> >>>>> +} /* namespace libcamera */\n> >>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>>>> index 8796f49..e5b48f2 100644\n> >>>>> --- a/src/libcamera/meson.build\n> >>>>> +++ b/src/libcamera/meson.build\n> >>>>> @@ -10,6 +10,7 @@ libcamera_sources = files([\n> >>>>> 'event_notifier.cpp',\n> >>>>> 'formats.cpp',\n> >>>>> 'geometry.cpp',\n> >>>>> + 'ipa_module.cpp',\n> >>>>> 'log.cpp',\n> >>>>> 'media_device.cpp',\n> >>>>> 'media_object.cpp',\n> >>>>> @@ -31,6 +32,7 @@ libcamera_headers = files([\n> >>>>> 'include/device_enumerator_udev.h',\n> >>>>> 'include/event_dispatcher_poll.h',\n> >>>>> 'include/formats.h',\n> >>>>> + 'include/ipa_module.h',\n> >>>>> 'include/log.h',\n> >>>>> 'include/media_device.h',\n> >>>>> 'include/media_object.h',\n> \n> Everything else is either an ack or an affirmative.","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 5B36460DE9\n\tfor ;\n\tThu, 16 May 2019 20:17:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6ABBD2FD;\n\tThu, 16 May 2019 20:17:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558030632;\n\tbh=02M/BWiuHljE+tYEFIbdWQEnfn/n7r5o5O0blQcxGD0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iexBSisVo9MuI5z4VmrRLCdsdLBVW/3SKUKOtecTI8RWTRA76B7TWwGI7/RMV3svQ\n\ttV5SIjKELOzSIyAe3MNwTI31a3dEJc7dZAfJOEyFDf6z2KpPHr9Cn5eKnAC0t8rAjn\n\t88vvxSvg+PcJr1W3QRraDGCUOqyI3UlzfurWqj0k=","Date":"Thu, 16 May 2019 21:16:55 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190516181655.GT14820@pendragon.ideasonboard.com>","References":"<20190514223808.27351-1-paul.elder@ideasonboard.com>\n\t<41555e99-19d1-8949-1950-e5a111a9ddaf@ideasonboard.com>\n\t<20190515150253.GF12325@localhost.localdomain>\n\t<27f62fbf-1f80-af57-10c9-75f94d87487f@ideasonboard.com>\n\t<20190515211942.GA4991@pendragon.ideasonboard.com>\n\t<20190516175116.GG12325@localhost.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190516175116.GG12325@localhost.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA\n\tshared library loader","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":"Thu, 16 May 2019 18:17:13 -0000"}}]