[{"id":1651,"web_url":"https://patchwork.libcamera.org/comment/1651/","msgid":"<20190521210149.GK5674@pendragon.ideasonboard.com>","date":"2019-05-21T21:01:49","subject":"Re: [libcamera-devel] [PATCH v5 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":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, May 21, 2019 at 04:54:28PM -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 v5:\n> - overloaded elfPointer use the other elfPointer in implementation\n> \n> Changes in v4:\n> - added overloaded elfPointer to specify the size to check for in the\n>   case that it cannot be obtained from the type (eg. char[size])\n> - documentation formalization\n> - other cosmetic changes\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            | 285 ++++++++++++++++++++++++\n>  src/libcamera/meson.build               |   2 +\n>  5 files changed, 354 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..4e0d668\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_info.h - Image Processing Algorithm module information\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> +\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..671b5a9\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\nNitpicking, we usually include the C++ headers first then then libcamera\nheaders, with a blank line between the two.\n\nWith this fixed,\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\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();\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..aa0fee6\n> --- /dev/null\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -0,0 +1,285 @@\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, size_t objSize)\n> +{\n> +\tsize_t size = offset + objSize;\n> +\tif (size > fileSize || size < objSize)\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> +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> +\treturn elfPointer<T>(map, offset, fileSize, sizeof(T));\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> +\t\t\t\t\t      strlen(symbol) + 1);\n> +\t\tif (!name)\n> +\t\t\treturn -ENOEXEC;\n> +\n> +\t\tif (!strcmp(name, symbol) &&\n> +\t\t    sym->st_info & STB_GLOBAL && sym->st_size == size) {\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, size);\n> +\tif (!data)\n> +\t\treturn -ENOEXEC;\n> +\n> +\tmemcpy(dst, data, size);\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\struct IPAModuleInfo\n> + * \\brief Information of an IPA module\n> + *\n> + * This structure contains the information of an IPA module. It is loaded,\n> + * read, and validated before anything else is loaded from the shared object.\n> + *\n> + * \\var IPAModuleInfo::name\n> + * \\brief The name of the IPA module\n> + *\n> + * \\var IPAModuleInfo::version\n> + * \\brief The version of the IPA module\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 module shared object\n> + *\n> + * Loads the IPAModuleInfo from the IPA module shared object at libPath.\n> + * The IPA module shared object file must be of the same endianness and\n> + * bitness as libcamera.\n> + *\n> + * \\todo load funtions from the IPA to be used by pipelines\n> + *\n> + * The caller shall call the isValid() method after constructing an\n> + * IPAModule instance to verify the validity of the IPAModule.\n> + */\n> +IPAModule::IPAModule(const std::string &libPath)\n> +\t: libPath_(libPath), valid_(false)\n> +{\n> +\tif (loadIPAModuleInfo() < 0)\n> +\t\treturn;\n> +\n> +\tvalid_ = true;\n> +}\n> +\n> +int IPAModule::loadIPAModuleInfo()\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, \"ipaModuleInfo\");\n> +\telse\n> +\t\tret = elfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>\n> +\t\t\t\t   (&info_, sizeof(info_), map, soSize, \"ipaModuleInfo\");\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 exists and\n> + * the IPA module information it contains was successfully retrieved and\n> + * validated.\n> + *\n> + * \\return true if the the IPAModule is valid, false otherwise\n> + */\n> +bool IPAModule::isValid() const\n> +{\n> +\treturn valid_;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the IPA module information\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 the IPA module information\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 6667360C40\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 May 2019 23:02:07 +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 C209452C;\n\tTue, 21 May 2019 23:02:06 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558472526;\n\tbh=o0iTJiXwjzUK/UrRqVJ4OjOff65W5nMfQqojX/ICpMo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=va4R9za1ztK32jqAsV00+jK3xAuI30terQmd1xzL0Xht6jfOJd7KOYWctuR/Qp+oN\n\tYYha3YxKuXrJ7+qrOgLn4tHUGqUJYHTuPWpopeEog7mDU0KAmMfuv4wnIUNq/RqNCd\n\t1Re1GPfV+SvWu1zuqSQhhWMrY03JNGgfY/tBILqs=","Date":"Wed, 22 May 2019 00:01:49 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190521210149.GK5674@pendragon.ideasonboard.com>","References":"<20190521205429.6529-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190521205429.6529-1-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 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 21:02:07 -0000"}}]