[libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA shared library loader
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 15 11:28:46 CEST 2019
Hi Paul,
Thank you for your patch, This is looking good and it's going in the
right direction but I have a few design comments ... feel free to
disagree though ...
On 14/05/2019 23:38, Paul Elder wrote:
> Implement a class to load a struct IPAModuleInfo of a given symbol name
> from a .so shared object library.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - renamed LibLoader class to IPAModule
> - added documentation
> - added logging
> - check that bitness of the shared object is the same as libcamera
> - moved symbol loading ("init") to the constructor, and added isValid()
> - added elfPointer() to prevent segfaults when reading data from mmap
> - moved struct IPAModuleInfo out of IPAModule
> - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a
> const reference
> - added munmap after the mmap
>
> src/libcamera/include/ipa_module.h | 43 +++++
> src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++
> src/libcamera/meson.build | 2 +
> 3 files changed, 322 insertions(+)
> create mode 100644 src/libcamera/include/ipa_module.h
> create mode 100644 src/libcamera/ipa_module.cpp
>
> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h
> new file mode 100644
> index 0000000..9eb0094
> --- /dev/null
> +++ b/src/libcamera/include/ipa_module.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_module.h - load IPA module information from shared library at runtime
Hrm ... we have two sides here.
We need a public header which defines the interface between an IPA
module and libcamera. That would include a struct IPAModuleInfo and any
registration details, but should not include any internal libcamera
private details regarding how the module is loaded.
> + */
> +#ifndef __LIBCAMERA_IPA_MODULE_H__
> +#define __LIBCAMERA_IPA_MODULE_H__
> +
> +#include <string>
> +
> +namespace libcamera {
> +
> +struct IPAModuleInfo {
> + char name[256];
> + unsigned int version;
> +};
> +
So IPModuleInfo (and then later, the class definition for how a
developer would construct an IPA Module) should live in the public
headers at:
/include/libcamera/ipa_module.h
> +class IPAModule
> +{
> +public:
> + explicit IPAModule(const std::string &libPath, const std::string &symbol);
> +
> + bool isValid() const;
> +
> + struct IPAModuleInfo const &IPAModuleInfo() const;
> +
> +private:
> + struct IPAModuleInfo info_;
> +
> + bool loaded_;
> + int bitClass_;
> +
> + int loadELFIdent(int fd);
> + template<class ElfHeader, class SecHeader, class SymHeader>
> + int loadSymbol(void *data, size_t size, char *map, size_t soSize,
> + const char *symbol);
> + int loadIPAModuleInfo(const char *libPath, const char *symbol);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_MODULE_H__ */
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> new file mode 100644
> index 0000000..5ca16e8
> --- /dev/null
> +++ b/src/libcamera/ipa_module.cpp
> @@ -0,0 +1,277 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_module.cpp - load IPA module information from shared library at runtime
> + */
> +
> +#include "ipa_module.h"
> +
> +#include <elf.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <iostream>
> +
> +#include "log.h"
> +
> +/**
> + * \file ipa_module.h
> + * \brief IPA module information loading
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \struct IPAModuleInfo
> + * \brief Information of an IPA plugin
> + *
> + * This structure contains the information of an IPA plugin. It is loaded,
> + * read, and validated before anything else is loaded from the plugin.
> + *
> + * \var IPAModuleInfo::name
> + * \brief The name of the IPA plugin
> + *
> + * \var IPAModuleInfo::version
> + * \brief The version of the IPA plugin
I don't think we need to store the 'version' of the plugin, so much as
the version of the API it was compiled against to ensure that it is
compatible with this 'running' instance of libcamera.
I.e. We don't care that it's the 3rd iteration of the rk3399 module -
but we do care that it is compatible with the API defined in Libcamera 1.0.
Maybe we should add compatible strings to match against pipeline
handlers as some point too :-) <I'm not even sure if I'm joking now>
> + */
> +
> +LOG_DEFINE_CATEGORY(IPAModule)
> +
> +template<typename T>
> +typename std::remove_extent<T>::type *elfPointer(void *map, off_t offset,
> + size_t fileSize)
> +{
> + size_t size = offset + sizeof(T);
> + if (size > fileSize || size < sizeof(T))
> + return nullptr;
> +
> + return reinterpret_cast<typename std::remove_extent<T>::type *>
> + (static_cast<char *>(map) + offset);
> +}
> +
> +/**
> + * \class IPAModule
> + * \brief Load IPA module information from an IPA plugin
> + */
> +
> +/**
> + * \brief Construct an IPAModule instance
> + * \param[in] libPath path to IPA plugin
> + * \param[in] symbol name of IPAModuleInfo to load from libPath
> + *
> + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.
> + * The IPA plugin shared object file must be of the same endianness and
> + * bitness as libcamera.
> + *
> + * Note that isValid() should be called to check if this loading was successful
> + * or not.
Hrm ... this is a slightly different design pattern than the rest of our
objects which are always constructable, and then actions later can fail.
I don't mind - but I wonder if we should add some documentation
regarding our design patterns somewhere.
(not an action on this patch)
> + */
> +
> +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)
> + : loaded_(false)
So actually, from my text below - what I'm really saying is that I don't
think you should provide a &symbol to this IPAModule constructor :)
> +{
> + int ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());
I see that you have moved the load into this constructor based on
comments from Laurent in v1. I think that's fine, but I think merging
the loading of the object, and the parsing of the symbol might be
combining too much. But it depends (as ever).
For efficiency, the constructor can load the file once and when created
and .isValid() is true, then we know we can successfully parse symbols.
But I think loadIPAModuleInfo() should be 'hardcoded' to know exactly
which symbol name it will expect for an IPAModule. Think of it like a
'main()' entry point for our IPAModule. You would always expect a C
binary to have a main() symbol...
So I would expect a libcamera IPA module to have a registration
something like:
/* Register our module with Libcamera */
const struct IPAModuleInfo ipaModuleInfo = {
.name = "Image Processing for the RK3399",
.version = 1, /* Perhaps this should be apiversion to prevent
* people thinking it describes the version of the
* module ? ...
*/
};
This could even then be turned into a macro:
#define LIBCAMERA_IPA_MODULE(__NAME) \
const struct IPAModuleInfo ipaModuleInfo = { \
.name = __NAME, \
.apiversion = 1, \
}
so that a module rk3399_ipa.cpp could define:
LIBCAMERA_IPA_MODULE("Image Processing Algorithms for the RK3399");
(apiversion would be hardcoded by the macro because it defines what
version of the API/libraries it's compiled against..., and we check it's
compatible at runtime)
> + if (ret < 0) {
> + loaded_ = false;
> + return;
> + }
> +
> + loaded_ = true;
> +}
> +
> +int IPAModule::loadELFIdent(int fd)
> +{
> + int ret = lseek(fd, 0, SEEK_SET);
> + if (ret < 0)
> + return -errno;
> +
> + unsigned char e_ident[EI_NIDENT];
> + ret = read(fd, e_ident, EI_NIDENT);
> + if (ret < 0)
> + return -errno;
> +
> + if (e_ident[EI_MAG0] != ELFMAG0 ||
> + e_ident[EI_MAG1] != ELFMAG1 ||
> + e_ident[EI_MAG2] != ELFMAG2 ||
> + e_ident[EI_MAG3] != ELFMAG3 ||
> + e_ident[EI_VERSION] != EV_CURRENT)
> + return -ENOEXEC;
> +
> + if ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||
> + (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))
> + bitClass_ = e_ident[EI_CLASS];
> + else
> + return -ENOEXEC;
> +
> + int a = 1;
> + unsigned char endianness = *reinterpret_cast<char *>(&a) == 1
> + ? ELFDATA2LSB : ELFDATA2MSB;
> + if (e_ident[EI_DATA] != endianness)
> + return -ENOEXEC;
> +
> + return 0;
> +}
> +
> +template<class ElfHeader, class SecHeader, class SymHeader>
> +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,
> + const char *symbol)
> +{> + ElfHeader *eHdr = elfPointer<ElfHeader>(map, 0, soSize);
> + if (!eHdr)
> + return -ENOEXEC;
> +
> + off_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;
> + SecHeader *sHdr = elfPointer<SecHeader>(map, offset, soSize);
> + if (!sHdr)
> + return -ENOEXEC;
> + off_t shnameoff = sHdr->sh_offset;
> +
> + /* Locate .dynsym section header. */
> + bool found = false;
> + SecHeader *dynsym;
> + for (unsigned int i = 0; i < eHdr->e_shnum; i++) {
> + offset = eHdr->e_shoff + eHdr->e_shentsize * i;
> + sHdr = elfPointer<SecHeader>(map, offset, soSize);
> + if (!sHdr)
> + return -ENOEXEC;
> +
> + offset = shnameoff + sHdr->sh_name;
> + char *name = elfPointer<char>(map, offset, soSize);
> + if (!name)
> + return -ENOEXEC;
> +
> + if (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, ".dynsym")) {
> + found = true;
> + dynsym = sHdr;
> + break;
> + }
> + }
> +
> + if (!found) {
> + LOG(IPAModule, Error) << "ELF has no .dynsym section";
> + return -ENOEXEC;
> + }
> +
> + offset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;
> + sHdr = elfPointer<SecHeader>(map, offset, soSize);
> + if (!sHdr)
> + return -ENOEXEC;
> + off_t dynsym_nameoff = sHdr->sh_offset;
> +
> + /* Locate symbol in the .dynsym section. */
> + found = false;
> + SymHeader *target_symbol;
> + unsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;
> + for (unsigned int i = 0; i < dynsym_num; i++) {
> + offset = dynsym->sh_offset + dynsym->sh_entsize * i;
> + SymHeader *sym = elfPointer<SymHeader>(map, offset, soSize);
> + if (!sym)
> + return -ENOEXEC;
> +
> + offset = dynsym_nameoff + sym->st_name;
> + char *name = elfPointer<char>(map, offset, soSize);
> + if (!name)
> + return -ENOEXEC;
> +
> + if (!strcmp(name, symbol) &&
> + sym->st_info & STB_GLOBAL &&
> + sym->st_size == sizeof(struct IPAModuleInfo)) {
I think this should be a check on the size_t size passed in?
> + found = true;
> + target_symbol = sym;
> + break;
> + }
> + }
> +
> + if (!found) {
> + LOG(IPAModule, Error) << "IPAModuleInfo symbol not found";
This function is called loadSymbol, it doesn't 'know' that it's loading
an IPAModuleInfo symbol.
I'd change this error to "Symbol " << symbol << " not found";
> + return -ENOEXEC;
> + }
> +
> + /* Locate and return data of symbol. */
> + offset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;
> + sHdr = elfPointer<SecHeader>(map, offset, soSize);
> + if (!sHdr)
> + return -ENOEXEC;
> + offset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);
> + char *data = elfPointer<char>(map, offset, soSize);
> + if (!data)
> + return -ENOEXEC;
> + memcpy(dst, data, size);
Oh - interesting, you're copying the symbol out. Once it's mmapped ...
why not parse it directly?
Can we do all of the 'work' we need to do on the file, and then close it
at the destructor?
I guess actually this might then keep the whole file mapped in memory
for longer which might not be desirable...
> +
> + return 0;
> +}
> +
> +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)
> +{
> + int fd = open(libPath, O_RDONLY);
> + if (fd < 0) {
> + LOG(IPAModule, Error) << "Failed to open IPA library: "
> + << strerror(errno);
> + return fd;
> + }
> +
> + int ret = loadELFIdent(fd);
> + if (ret)
> + goto close;
> +
> + size_t soSize;
> + char *map;
> + struct stat st;
> + ret = fstat(fd, &st);
> + if (ret < 0)
> + goto close;
> + soSize = st.st_size;
> + map = static_cast<char *>(mmap(NULL, soSize, PROT_READ,
> + MAP_PRIVATE, fd, 0));
*iff* the constructor did the open, and the symbol parsing was
separated, then I think soSize and map would go to class privates. Then
they could be accessed directly by loadSymbol too. But that's an only if...
Essentially, it would be
IPAModule("SO-path")
- Constructor opens the file, performs stat, and mmap.
(or those become an open() call)
- loadIPAModuleInfo() calls loadSymbol("ipaModuleInfo"); and deals only
with the ipaModule info structure and checking or such.
> + if (map == MAP_FAILED) {
> + ret = -errno;
> + goto close;
> + }
> +
> + if (bitClass_ == ELFCLASS32)
> + ret = loadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
> + (&info_, sizeof(info_), map, soSize, symbol);
> + else if (bitClass_ == ELFCLASS64)
> + ret = loadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
> + (&info_, sizeof(info_), map, soSize, symbol);
> + if (ret)
> + goto unmap;
> +
> +unmap:
> + munmap(map, soSize);
> +close:
> + close(fd);
> + return ret;
> +}
> +
> +/**
> + * \brief Check if construction of the IPAModule instance succeeded
> + *
> + * \return true if the constructor succeeded with no errors, false otherwise
> + */
> +
> +bool IPAModule::isValid() const
> +{
> + return loaded_;
> +}
> +
> +/**
> + * \brief Get the loaded IPAModuleInfo
> + *
> + * Check isValid() before calling this.
> + *
> + * \return IPAModuleInfo
> + */
> +
> +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const
> +{
> + return info_;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 8796f49..e5b48f2 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -10,6 +10,7 @@ libcamera_sources = files([
> 'event_notifier.cpp',
> 'formats.cpp',
> 'geometry.cpp',
> + 'ipa_module.cpp',
> 'log.cpp',
> 'media_device.cpp',
> 'media_object.cpp',
> @@ -31,6 +32,7 @@ libcamera_headers = files([
> 'include/device_enumerator_udev.h',
> 'include/event_dispatcher_poll.h',
> 'include/formats.h',
> + 'include/ipa_module.h',
> 'include/log.h',
> 'include/media_device.h',
> 'include/media_object.h',
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list