[{"id":1645,"web_url":"https://patchwork.libcamera.org/comment/1645/","msgid":"<20190521144359.GH5674@pendragon.ideasonboard.com>","date":"2019-05-21T14:43:59","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: ipa_module: add IPA\n\tshared library module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, May 20, 2019 at 08:40:58PM -0400, Paul Elder wrote:\n> Implement a class to wrap around an IPA module shared object.\n> \n> For now, just load a struct IPAModuleInfo with symbol name\n> ipaModuleInfo from an IPA module .so shared object.\n> \n> Also provide a public header file including the struct IPAModuleInfo,\n> structured such that both C and C++ IPA modules are supported.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n> Changes in v3:\n> - created public header file for IPAModuleInfo (and for handling C vs\n>   C++ IPA modules)\n> - made ELF loading/parsing functions static\n> - got rid of bitClass_\n> - other cosmetic changes\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>  include/libcamera/ipa/ipa_module_info.h |  31 +++\n>  include/libcamera/meson.build           |   1 +\n>  src/libcamera/include/ipa_module.h      |  35 +++\n>  src/libcamera/ipa_module.cpp            | 280 ++++++++++++++++++++++++\n>  src/libcamera/meson.build               |   2 +\n>  5 files changed, 349 insertions(+)\n>  create mode 100644 include/libcamera/ipa/ipa_module_info.h\n>  create mode 100644 src/libcamera/include/ipa_module.h\n>  create mode 100644 src/libcamera/ipa_module.cpp\n> \n> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h\n> new file mode 100644\n> index 0000000..eae60f6\n> --- /dev/null\n> +++ b/include/libcamera/ipa/ipa_module_info.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_module.h - Image Processing Algorithm module\n\ns/ipa_module/ipa_module_info/\n\nand a similar change for the description that then follows.\n\n> + */\n> +#ifndef __LIBCAMERA_IPA_MODULE_INFO_H__\n> +#define __LIBCAMERA_IPA_MODULE_INFO_H__\n> +\n> +#ifdef __cplusplus\n> +namespace libcamera {\n> +#endif\n> +\n> +struct IPAModuleInfo {\n> +\tchar name[256];\n> +\tunsigned int version;\n> +};\n> +\n> +#ifdef __cplusplus\n> +extern \"C\" {\n> +#endif\n> +extern const struct IPAModuleInfo ipaModuleInfo;\n> +#ifdef __cplusplus\n> +};\n> +#endif\n> +\n> +#ifdef __cplusplus\n> +}; /* namespace libcamera */\n> +#endif\n\nAll this __cplusplus handling isn't neat, but I think we can live with\nit for now. We will later decide whether to mandate a C++ ABI for IPA\nmodules (as in a C-style generator function that returns a pointer to a\nC++ object), or stick with a pure C API (as in a structure of C function\npointers). In the former case we can drop __cplusplus from here, as IPAs\nwould have to be implemented in C++. Let's just keep this in mind.\n\n> +\n> +#endif /* __LIBCAMERA_IPA_MODULE_INFO_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 83d226a..cb64f0c 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_api = files([\n>      'event_dispatcher.h',\n>      'event_notifier.h',\n>      'geometry.h',\n> +    'ipa/ipa_module_info.h',\n>      'libcamera.h',\n>      'object.h',\n>      'request.h',\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..a13ea4a\n> --- /dev/null\n> +++ b/src/libcamera/include/ipa_module.h\n> @@ -0,0 +1,35 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_module.h - Image Processing Algorithm module\n> + */\n> +#ifndef __LIBCAMERA_IPA_MODULE_H__\n> +#define __LIBCAMERA_IPA_MODULE_H__\n> +\n> +#include <libcamera/ipa/ipa_module_info.h>\n> +#include <string>\n> +\n> +namespace libcamera {\n> +\n> +class IPAModule\n> +{\n> +public:\n> +\texplicit IPAModule(const std::string &libPath);\n> +\n> +\tbool isValid() const;\n> +\n> +\tconst struct IPAModuleInfo &info() const;\n> +\n> +private:\n> +\tstruct IPAModuleInfo info_;\n> +\n> +\tstd::string libPath_;\n> +\tbool valid_;\n> +\n> +\tint loadIPAModuleInfo(const char *libPath);\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..acfe1af\n> --- /dev/null\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -0,0 +1,280 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_module.cpp - Image Processing Algorithm module\n> + */\n> +\n> +#include \"ipa_module.h\"\n> +\n> +#include <elf.h>\n> +#include <errno.h>\n> +#include <fcntl.h>\n> +#include <string.h>\n> +#include <sys/mman.h>\n> +#include <sys/stat.h>\n> +#include <sys/types.h>\n> +#include <unistd.h>\n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file ipa_module.h\n> + * \\brief Image Processing Algorithm module\n> + */\n> +\n> +/**\n> + * \\file ipa_module_info.h\n> + * \\brief Image Processing Algorithm module information\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPAModule)\n> +\n> +namespace {\n> +\n> +template<typename T>\n> +typename std::remove_extent<T>::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<typename std::remove_extent<T>::type *>\n> +\t\t(static_cast<char *>(map) + offset);\n> +}\n> +\n> +int elfVerifyIdent(void *map, size_t soSize)\n> +{\n> +\tchar *e_ident = elfPointer<char[EI_NIDENT]>(map, 0, soSize);\n> +\tif (!e_ident)\n> +\t\treturn -ENOEXEC;\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> +\tint bitClass = sizeof(unsigned long) == 4 ? ELFCLASS32 : ELFCLASS64;\n> +\tif (e_ident[EI_CLASS] != bitClass)\n> +\t\treturn -ENOEXEC;\n> +\n> +\tint a = 1;\n> +\tunsigned char endianness = *reinterpret_cast<char *>(&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<class ElfHeader, class SecHeader, class SymHeader>\n> +int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n> +\t\t\t  const char *symbol)\n> +{\n> +\tElfHeader *eHdr = elfPointer<ElfHeader>(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<SecHeader>(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> +\tSecHeader *dynsym = nullptr;\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<SecHeader>(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<char[8]>(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\tdynsym = sHdr;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (dynsym == nullptr) {\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<SecHeader>(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> +\tSymHeader *targetSymbol = nullptr;\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<SymHeader>(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<char>(map, offset, soSize);\n\nYou need a char[strlen(symbol)+1] here.\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 == size) {\n\nThis could hold on two lines instead of three.\n\n> +\t\t\ttargetSymbol = sym;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (targetSymbol == nullptr) {\n> +\t\tLOG(IPAModule, Error) << \"Symbol \" << symbol << \" not found\";\n> +\t\treturn -ENOEXEC;\n> +\t}\n> +\n> +\t/* Locate and return data of symbol. */\n> +\tif (targetSymbol->st_shndx >= eHdr->e_shnum)\n> +\t\treturn -ENOEXEC;\n> +\toffset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;\n> +\tsHdr = elfPointer<SecHeader>(map, offset, soSize);\n> +\tif (!sHdr)\n> +\t\treturn -ENOEXEC;\n> +\toffset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);\n> +\tchar *data = elfPointer<char>(map, offset, soSize);\n\nAnd a char[size] here.\n\n> +\tif (!data)\n> +\t\treturn -ENOEXEC;\n> +\t/* \\todo move out memcpy? */\n\nYOu know my opinion on this :-)\n\n> +\tmemcpy(dst, data, size);\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace */\n\nnamespace libcamera ?\n\n> +\n> +/**\n> + * \\struct IPAModuleInfo\n> + * \\brief Information of an IPA plugin\n\nDo we standardise on \"IPA plugin\" to mean \"IPA module shared object\" ?\nIf so you should replace \"The IPA plugin shared object\" with \"The IPA\nplugin\" below. Or should we drop the word plugin and use module through\nthe whole documentation instead, to avoid having two different terms ?\nI'm tempted to go for the latter, with a global s/plugin/module/.\n\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> + * \\todo abi compatability version\n> + * \\todo pipeline compatability matcher\n> + */\n> +\n> +/**\n> + * \\class IPAModule\n> + * \\brief Wrapper around IPA module shared object\n> + */\n> +\n> +/**\n> + * \\brief Construct an IPAModule instance\n> + * \\param[in] libPath path to IPA plugin\n> + *\n> + * Loads the IPAModuleInfo 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> + * \\todo also load funtions and whatever from the IPA to be used by pipelines\n\nVery formal :-) We'll fix this soon anyway.\n\n> + *\n> + * Note that isValid() should be called to check if this loading was successful\n> + * or not.\n\nMaybe replace the last sentence with\n\n * The caller shall call the isValid() method after constructing an\n * IPAModule instance to verify the validity of the IPAModule.\n\nto make it a bit more formal ?\n\n> + */\n> +IPAModule::IPAModule(const std::string &libPath)\n> +\t: libPath_(libPath), valid_(false)\n> +{\n> +\tint ret = loadIPAModuleInfo(\"ipaModuleInfo\");\n> +\tif (ret < 0)\n> +\t\treturn;\n\nMaybe\n\n\tif (loadIPAModuleInfo(\"ipaModuleInfo\") < 0)\n\t\treturn;\n\n?\n\n> +\n> +\tvalid_ = true;\n> +}\n> +\n> +int IPAModule::loadIPAModuleInfo(const char *symbol)\n\nDon't pass the symbol name to this function, you can hardcode it\ninternally.\n\n> +{\n> +\tint fd = open(libPath_.c_str(), O_RDONLY);\n> +\tif (fd < 0) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> +\t\t\t\t      << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tsize_t soSize;\n> +\tvoid *map;\n> +\tstruct stat st;\n> +\tint ret = fstat(fd, &st);\n> +\tif (ret < 0)\n> +\t\tgoto close;\n> +\tsoSize = st.st_size;\n> +\tmap = mmap(NULL, soSize, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tif (map == MAP_FAILED) {\n> +\t\tret = -errno;\n> +\t\tgoto close;\n> +\t}\n> +\n> +\tret = elfVerifyIdent(map, soSize);\n> +\tif (ret)\n> +\t\tgoto unmap;\n> +\n> +\tif (sizeof(unsigned long) == 4)\n> +\t\tret = elfLoadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>\n> +\t\t\t\t   (&info_, sizeof(info_), map, soSize, symbol);\n> +\telse\n> +\t\tret = elfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>\n> +\t\t\t\t   (&info_, sizeof(info_), map, soSize, symbol);\n> +\tif (ret)\n> +\t\tgoto unmap;\n\nYou can remove those two lines.\n\n> +\n> +unmap:\n> +\tmunmap(map, soSize);\n> +close:\n> +\tclose(fd);\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\brief Check if the IPAModule instance is valid\n> + *\n> + * An IPAModule instance is valid if the IPA Module shared object was\n> + * successfully loaded, and the ipaModuleInfo in it was read.\n\nAs we will add a load() method that dlopen()s the .so later, I would\nwrite this as\n\nAn IPAModuleinstance is valid if the IPA module shared object exists and\nthe IPA module information it contains was successfully retrieved and\nvalidated.\n\n> + *\n> + * \\return true if the the IPAModule is  valid, false otherwise\n\ns/is  valid/is valid/\n\n> + */\n> +bool IPAModule::isValid() const\n> +{\n> +\treturn valid_;\n> +}\n> +\n> +/**\n> + * \\brief Get the IPA module information\n\nWe use Retrieve instead of Get through the livrary.\n\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> + * \\return IPAModuleInfo\n\n\\return The IPA module information\n\n> + */\n> +const struct IPAModuleInfo &IPAModule::info() 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":"<laurent.pinchart@ideasonboard.com>","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 C1AEE60C27\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 May 2019 16:44:16 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2199652C;\n\tTue, 21 May 2019 16:44:16 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558449856;\n\tbh=7S7GiQJpOHZtydMRhkVA+xhxUFCF8wxmhN5FfSzbrY8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lUBGtc1/QdQ7StGfJ9yXBGt+8uIKJX0bPkbvfjjrS3wN24zIp2rELV+E3c8Ch538K\n\tw2qBt64Ep31UVv1VXzJwu7mNMvLWhiIqiLye55TAskKraXZWYKeIAk8bkV2nNimFQJ\n\tU2GQI8DWkOl1L3p7pjyqCDO8sAZXFXltuaqsLfco=","Date":"Tue, 21 May 2019 17:43:59 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190521144359.GH5674@pendragon.ideasonboard.com>","References":"<20190521004059.7750-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190521004059.7750-1-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: ipa_module: add IPA\n\tshared library module","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 21 May 2019 14:44:17 -0000"}}]