[libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA shared library loader
Paul Elder
paul.elder at ideasonboard.com
Wed May 15 17:02:53 CEST 2019
Hi Kieran,
On Wed, May 15, 2019 at 10:28:46AM +0100, Kieran Bingham wrote:
> 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 ...
Thank you for the review.
> 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.
Yes, I agree.
> > + */
> > +#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
Yeah.
Then, where should class IPAModule go?
> > +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.
Yeah, good point; I agree.
How would we get the API version of libcamera?
> 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...
Yeah, I think that's a good idea.
> 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 ? ...
> */
> };
Should the info entry point be called ipaModuleInfo?
> 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?
Ah yeah, I totally missed that.
> > + 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";
And this.
> > + 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?
I'm not sure what you mean by "parse"?
> Can we do all of the 'work' we need to do on the file, and then close it
> at the destructor?
I don't want to keep the file open/mapped over multiple method calls.
That's why in my first version there was a constructor that did nothing
and an init() that did everything (which got moved to the constructor in
this version).
> 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 loadIPAModuleInfo() is to be called from within the constructor
(after open, stat, and mmap), then I agree. I'm not sure map and soSize
need to be privates though. It could go either way.
>
> > + 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',
> >
Thanks,
Paul
More information about the libcamera-devel
mailing list