[libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA shared library loader
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 15 17:26:48 CEST 2019
Hi Paul,
On 15/05/2019 16:02, Paul Elder wrote:
> 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?
Stays here in this file I think :-)
>
>>> +class IPAModule
I'm wondering if we have two sides if this class should be calls
IPAModule though.
Surely an IPAModule would implement a class called IPAModule, and
perhaps 'this' class is an IPAModuleParser? IPAModuleLoader?
Probably a Loader :D
>>> +{
>>> +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?
You get to define it :-)
To start with it's probably just:
#define LIBCAMERA_IPA_API_VERSION 1
This would be a 'local' IPAModule API version number or such I think, as
our IPA modules must go through a defined interface...
Any time an ABI break occurs in the IPAModule 'API' we would have to
bump up the apiversion.
>> 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?
I think variables are in camelCase with a lowerCase first letter in the
project right? or was this not quite your question?
>
>> 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.
No worries.
>
>>> + 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"?
I mean once you find the symbol and obtain the pointer to it ('data'
here) then you have a pointer to memory which contains the structure.
You could return that pointer, and cast it to the type. Then you can
read the structure members directly.
But that requires knowing that the symbol doesn't require any further
relocations ... which is fine for our simple info structure.
It just seems logical in my head to have a function called findSymbol()
which returns a pointer to the symbol, and then (if you're going to copy
it) a function called loadStructure(), which finds the symbol, checks
the size then copies it or such. Maybe it's generalising the functions
more than we care if we will only use them for one thing
>
>> 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).
The only things we'll do on this structure is read it and decide if this
library is OK for loading, perhaps even printing the name string in debug.
Once that has occurred, can't the IPAModuleInfo structure be discarded?
The next thing that we'll do is spawn a container/namespace/safe-thingy
and use the normal dl_open methods to re-open the library there...
>
>> 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.
Well, I leave that all up to you now :-)
>
>>
>>> + 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
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list