[libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA shared library loader
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed May 15 23:19:42 CEST 2019
Hello,
On Wed, May 15, 2019 at 04:26:48PM +0100, Kieran Bingham wrote:
> On 15/05/2019 16:02, Paul Elder wrote:
> > 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 :-)
That's what I had envisioned, so it sounds good to me.
> >
> >>> +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
I was wondering about this. One option would indeed to provide an
IPAModule class with pure virtual functions that the module would need
to implement. Another option would be to use a structure with C-style
function pointers. I was thinking about the latter, but the former is
not a bad idea. However, we need to take care about backward
compatibility. Can a module compiled with an old version of g++ (or
clang++) be incompatible with libcamera built with a newer version if
using a C++ ABI ?
Ie we go the C++ way, we will indeed need two different names for the
classes. I was thinking about IPAModule for this class and
IPAModuleInterface for the new one, in which case no change would be
needed here. Note that IPAModule would inherit from IPAModuleInterface
as it would provide the IPA API to pipeline handlers. I don't like the
name IPAModuleLoader much, as from a pipeline handler point of view
we're using modules, not module loaders. Other options may be possible.
> >>> +{
> >>> +public:
> >>> + explicit IPAModule(const std::string &libPath, const std::string &symbol);
No need to pass the symbol name here, you can hardcode it in the
implementation. All modules will use the same symbol name.
> >>> +
> >>> + bool isValid() const;
> >>> +
> >>> + struct IPAModuleInfo const &IPAModuleInfo() const;
We usually write this const struct IPAModuleInfo &IPAModuleInfo() const.
And I would name the function info(), as IPAModule::info() makes it
clear that we're retrieving the information of an IPA module.
> >>> +
> >>> +private:
> >>> + struct IPAModuleInfo info_;
> >>> +
> >>> + bool loaded_;
Let's name this valid_ to match isValid(). We will later have a load()
function to dlopen() the module, and likely a loaded_ flag to match
that.
> >>> + 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
I think we can already update the description to "Image Processing
Algorithm module" or similar
> >>> + */
> >>> +
> >>> +#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
Same here.
> >>> + */
> >>> +
> >>> +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.
I agree, but I think we can defer this. The IPAModuleInfo structure is
currently a mock up to start implementing the IPAModule class, I expect
it to change a lot.
> >> 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>
I think we will need that :-)
Paul, could you record there open items ?
> >>> + */
> >>> +
> >>> +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);
> >>> +}
This function should be static. The C way of doing so is using the
static keyword, the C++ way is to put it in an anonymous namespace.
namespace {
template<typename T>
typename std::remove_extent<T>::type *elfPointer(void *map, off_t offset,
size_t fileSize)
{
...
}
};
> >>> +
> >>> +/**
> >>> + * \class IPAModule
> >>> + * \brief Load IPA module information from an IPA plugin
To match the suggestion above, "IPA module wrapper" ? I think we can do
better than that, maybe "Wrapper around an IPA module shared object" ?
Suggestions are welcome.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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)
I've discussed this with Paul, and I think that in this case this
implementation makes sense. There's no need to delay loading, and as the
IPAModule class is a wrapper around a .so, constructing an instance on
which on can call isValid() to check if the IPA is valid makes sense to
me.
Maybe we need to reconsider our other classes indeed. Any
suggestion/preference ?
> >>> + */
> >>> +
> >>> +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 :)
So we agree :-)
> >>> +{
> >>> + 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).
Note that this only loads the module info, not the module itself (I
foresee a load() function that will dlopen() the module). I'm not sure
what you mean by merging the loading of the object and the parsing of
the symbol.
> >> 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 you can't know it's a valid IPA unless you can load the
IPAModuleInfo successfully (and later maybe even cryptographically
verifying the module).
> >> 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 sounds good to me.
> >> This could even then be turned into a macro:
> >>
> >> #define LIBCAMERA_IPA_MODULE(__NAME) \
> >> const struct IPAModuleInfo ipaModuleInfo = { \
> >> .name = __NAME, \
> >> .apiversion = 1, \
> >> }
We'll have to throw an extern "C" here, otherwise your symbol name will
be _ZL13ipaModuleInfo with g++.
> >>
> >> 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)
Something like that. If the number of fields grows, and if some of them
become complex, we may want to fill the structure manually, be we should
still use a macro to instantiate the right structure with the right name
and the extern "C" keyword.
> >>> + if (ret < 0) {
> >>> + loaded_ = false;
This isn't needed, you initialise loaded_ to false above.
> >>> + return;
> >>> + }
> >>> +
> >>> + loaded_ = true;
> >>> +}
> >>> +
> >>> +int IPAModule::loadELFIdent(int fd)
This function doesn't actually load much, how about renaming it to
verifyELFIdent() ?
> >>> +{
> >>> + int ret = lseek(fd, 0, SEEK_SET);
> >>> + if (ret < 0)
> >>> + return -errno;
> >>> +
As we're mmap()ing the whole .so, you could mmap() before calling
loadELFIdent(), and pass the void *map and size to the function, instead
of using read.
> >>> + 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;
How about
bitClass_ = sizeof(unsigned long) == 4 ? ELFCLASS32 : ELFCLASS64;
if (e_ident[EI_CLASS] != bitClass_)
return -ENOEXEC;
> >>> +
> >>> + int a = 1;
> >>> + unsigned char endianness = *reinterpret_cast<char *>(&a) == 1
> >>> + ? ELFDATA2LSB : ELFDATA2MSB;
So you didn't like my union-based solution ? :-)
> >>> + 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);
This should be elfPointer<char[8]> as you want to ensure that at least 8
bytes are available (sizeof(".dynsym"), including the terminating 0).
> >>> + if (!name)
> >>> + return -ENOEXEC;
> >>> +
> >>> + if (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, ".dynsym")) {
> >>> + found = true;
> >>> + dynsym = sHdr;
You could initialise dynsym to nullptr and remove found.
> >>> + 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;
targetSymbol ?
> >>> + 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);
Same here, you want elfPointer<char[strlen(symbol) + 1]>. I hope this
will compile :-)
> >>> + 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;
You could also remove found here.
> >>> + 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. */
As a sanity check you may want to verify that target_symbol->st_shndx <
eHdr->e_shnum. Otherwise none of the elfPointer<> checks below may
trigger, and you will load an invalid symbol. This can happen for
instance if the symbol is absolute (SHN_ABS).
> >>> + 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);
elfPointer<char[size]>
> >>> + 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
I have no particular opinion on this. We could indeed rename this
function to findSymbol() and move the copy outside. It may actually be a
good idea, but then this function should return both the address (or
offset) and size (one or both through output arguments).
> >> 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 agree, we shouldn't keep it mapped.
> 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...
No, we need it at least to match the module with pipeline handlers, so
it needs to be copied. I'm sure we'll have other fields that we will
also need to access later.
> >> 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)
I would store libPath in a member variable as we will need it for
dlopen(), and use that member variable in this function.
> >>> +{
> >>> + int fd = open(libPath, O_RDONLY);
> >>> + if (fd < 0) {
> >>> + LOG(IPAModule, Error) << "Failed to open IPA library: "
> >>> + << strerror(errno);
You need
ret = -errno;
LOG(IPAModule, Error) << "Failed to open IPA library: "
<< strerror(-ret);
return ret;
ass errno may be changed by LOG(IPAModule, Error).
> >>> + 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));
struct stat st;
ret = fstat(fd, &st);
if (ret < 0)
goto close;
size_t soSize = st.st_size;
char *map = static_cast<char *>(mmap(NULL, soSize, PROT_READ,
MAP_PRIVATE, fd, 0));
I think I would make it a void *map, pass it as a void * to all the
functions, and let elfPointer<> do the case.
> >> *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.
I thought about it, but it the address and size are really temporary
variables, as we unmap the file after the parsing, so I think it's best
to pass them explicitly to functions.
> 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);
As we don't need to support a module with a different bit class than
libcamera, would we avoid compiling in both version of the function ?
Maybe something like
#if sizeof(long) == 4
ret = loadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
(&info_, sizeof(info_), map, soSize, symbol);
#else
ret = loadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
(&info_, sizeof(info_), map, soSize, symbol);
#endif
and given that we only need one version of the function, I even wonder
if we need to keep it defined as a template function, maybe we could
have a
#if sizeof(long) == 4
#define Elf_Ehdr Elf32_Ehdr
#define Elf_Shdr Elf32_Shdr
#define Elf_Sym Elf32_Sym
#else
#define Elf_Ehdr Elf64_Ehdr
#define Elf_Shdr Elf64_Shdr
#define Elf_Sym Elf64_Sym
#endif
just before the function definition. This would however require moving
the function away from the class to a global function, private to this
file, to avoid exposing it in the ipa_module.h header, otherwise we
would have to move the above #if's there, which isn't neat. If we do
this we should move the loadElfIdent function too. Neither function use
any of the class members (except for bitClass_, which isn't needed
anymore), so they're essentially static, and separating them from the
IPAModule class would open the door to sharing them with other classes
if needed later. elfPointer<> is already separate. If you prefer to keep
them as class members, they should be declared as static in the class
definition.
> >>> + if (ret)
> >>> + goto unmap;
> >>> +
> >>> +unmap:
> >>> + munmap(map, soSize);
> >>> +close:
> >>> + close(fd);
> >>> + return ret;
> >>> +}
> >>> +
> >>> +/**
> >>> + * \brief Check if construction of the IPAModule instance succeeded
I would say "Check if the IPAModule instance is valid" and then add a
paragraph to detail what a valid (or invalid) IPAModule is.
> >>> + *
> >>> + * \return true if the constructor succeeded with no errors, false otherwise
true if the IPAModule is valid, false otherwise
> >>> + */
> >>> +
No need for this blank line.
> >>> +bool IPAModule::isValid() const
> >>> +{
> >>> + return loaded_;
> >>> +}
> >>> +
> >>> +/**
> >>> + * \brief Get the loaded IPAModuleInfo
"Get the IPA module information"
> >>> + *
> >>> + * Check isValid() before calling this.
"The content of the IPA module information is loaded from the module,
and is valid only if the module is valid (as returned by isValid()).
Calling this function on an invalid module is an error."
> >>> + *
> >>> + * \return IPAModuleInfo
> >>> + */
> >>> +
No need for a blank line here either
> >>> +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,
Laurent Pinchart
More information about the libcamera-devel
mailing list