[libcamera-devel] [PATCH 3/8] libcamera: ipa_module_info: update struct to allow IPA matching
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 28 17:25:31 CEST 2019
Hi Paul,
Thank you for the patch.
On Mon, May 27, 2019 at 06:35:35PM -0400, Paul Elder wrote:
> We need a way to match pipelines with IPA modules, so add fields in
> IPAModuleInfo to hold the IPA API version number, the pipeline name, and
> the pipeline major and minor version.
>
> Also update IPA module tests and Doxygen accordingly. Doxygen needs to
> be updated to accomodate __attribute__((packed)).
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Documentation/Doxyfile.in | 7 ++--
> include/libcamera/ipa/ipa_module_info.h | 11 ++++--
> src/libcamera/ipa_module.cpp | 48 +++++++++++++++++++++----
> test/ipa/ipa_test.cpp | 45 ++++++++++++++++++-----
> test/ipa/shared_test.cpp | 4 ++-
> 5 files changed, 94 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 67eaded..ac70efb 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -2016,7 +2016,7 @@ ENABLE_PREPROCESSING = YES
> # The default value is: NO.
> # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
>
> -MACRO_EXPANSION = NO
> +MACRO_EXPANSION = YES
>
> # If the EXPAND_ONLY_PREDEF and MACRO_EXPANSION tags are both set to YES then
> # the macro expansion is limited to the macros specified with the PREDEFINED and
> @@ -2024,7 +2024,7 @@ MACRO_EXPANSION = NO
> # The default value is: NO.
> # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
>
> -EXPAND_ONLY_PREDEF = NO
> +EXPAND_ONLY_PREDEF = YES
>
> # If the SEARCH_INCLUDES tag is set to YES, the include files in the
> # INCLUDE_PATH will be searched if a #include is found.
> @@ -2057,7 +2057,8 @@ INCLUDE_FILE_PATTERNS = *.h
> # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
>
> PREDEFINED = __DOXYGEN__ \
> - __cplusplus
> + __cplusplus \
> + __attribute__(x)=
>
> # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
> # tag can be used to specify a list of macro names that should be expanded. The
> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h
> index 4e0d668..84492ed 100644
> --- a/include/libcamera/ipa/ipa_module_info.h
> +++ b/include/libcamera/ipa/ipa_module_info.h
> @@ -7,14 +7,21 @@
> #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__
> #define __LIBCAMERA_IPA_MODULE_INFO_H__
>
> +#include <stdint.h>
> +
> +#define IPA_MODULE_API_VERSION 1
> +
> #ifdef __cplusplus
> namespace libcamera {
> #endif
>
> struct IPAModuleInfo {
> + int moduleAPIVersion;
> + int pipelineVersionMajor;
> + int pipelineVersionMinor;
Please use integer types from stdint.h. These should be uint32_t.
I think you can combine the minor and major in a single version field,
with a macro to create it from major and minor numbers.
> + char pipelineName[256];
> char name[256];
> - unsigned int version;
> -};
> +} __attribute__((packed));
>
> #ifdef __cplusplus
> extern "C" {
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 86cbe71..6e68934 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -169,6 +169,17 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>
> } /* namespace */
>
> +/**
> + * \def IPA_MODULE_API_VERSION
> + * \brief The IPA API version
> + *
> + * This version number specifies the version for the layout of
> + * struct IPAModuleInfo, and the API between pipeline handlers and the IPA.
The module API version is just the former, not the latter, which are
reported through the pipelineVersion.
> + * The IPA module should use this macro to define its moduleAPIVersion field.
s/should/shall/
s/define/set/
> + *
> + * \sa libcamera::IPAModuleInfo::moduleAPIVersion
> + */
> +
> /**
> * \struct IPAModuleInfo
> * \brief Information of an IPA module
> @@ -176,14 +187,39 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
> * This structure contains the information of an IPA module. It is loaded,
> * read, and validated before anything else is loaded from the shared object.
> *
> - * \var IPAModuleInfo::name
> - * \brief The name of the IPA module
> + * \var IPAModuleInfo::moduleAPIVersion
> + * \brief The IPA API version that the IPA module was made with
s/was made with/implements/ ?
> + *
> + * This version number specifies the version for the layout of
> + * struct IPAModuleInfo, and the API between pipeline handlers and the IPA.
s/, .*/./
> + * The IPA module shall report here the version that it was built for,
> + * using the macro IPA_MODULE_API_VERSION.
> *
> - * \var IPAModuleInfo::version
> - * \brief The version of the IPA module
> + * \sa IPA_MODULE_API_VERSION
> *
> - * \todo abi compatability version
> - * \todo pipeline compatability matcher
> + * \var IPAModuleInfo::pipelineVersionMajor
> + * \brief The pipeline handler version (major) that the IPA module is for
> + * \todo actually match the pipeline handler against this
> + *
> + * The major version of the module and pipeline handler must be equal in
> + * order for the pipeline handler and the module to be matched.
> + *
> + * \var IPAModuleInfo::pipelineVersionMinor
> + * \brief The pipeline handler version (minor) that the IPA module is for
> + *
> + * The minor version of the module must be equal to or greater than
> + * the pipeline handler minor version in order for the pipeline handler
> + * and the module to be matched.
> + *
> + * \var IPAModuleInfo::pipelineName
> + * \brief The name of the pipeline handler that the IPA module is for
> + *
> + * This name is used to match a pipeline handler with the module.
> + * The name of the pipeline handler is declared in the pipeline handler
> + * with the macro REGISTER_PIPELINE_HANDLER.
I would drop the second sentence, as REGISTER_PIPELINE_HANDLER is an
internal API, and should thus not be referenced from external API files.
> + *
> + * \var IPAModuleInfo::name
> + * \brief The name of the IPA module
> */
>
> /**
> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp
> index 2dbc702..f50880e 100644
> --- a/test/ipa/ipa_test.cpp
> +++ b/test/ipa/ipa_test.cpp
> @@ -33,20 +33,45 @@ protected:
>
> const struct IPAModuleInfo &info = ll->info();
>
> - if (strcmp(info.name, testInfo.name)) {
> - cerr << "test IPA module has incorrect name" << endl;
> - cerr << "expected \"" << testInfo.name << "\", got \""
> - << info.name << "\"" << endl;
> + stringstream errString;
> +
> + if (info.moduleAPIVersion != testInfo.moduleAPIVersion) {
> + errString << "incorrect name: expected \""
"name" ? :-)
You can simplify all this with
if (info.moduleAPIVersion != testInfo.moduleAPIVersion ||
info.version != testInfo.version ||
...) {
cerr << "IPA module information mismatch: expected "
<< "moduleAPIVersion=" << testInfo.moduleApiVersion
<< ...
<< ", got "
<< ...
}
What do you think ?
> + << testInfo.moduleAPIVersion << "\", got \""
> + << info.moduleAPIVersion << "\"" << endl;
> ret = -1;
> }
>
> - if (info.version != testInfo.version) {
> - cerr << "test IPA module has incorrect version" << endl;
> - cerr << "expected \"" << testInfo.version << "\", got \""
> - << info.version << "\"" << endl;
> + if (info.pipelineVersionMajor != testInfo.pipelineVersionMajor) {
> + errString << "incorrect name: expected \""
> + << testInfo.pipelineVersionMajor << "\", got \""
> + << info.pipelineVersionMajor << "\"" << endl;
> ret = -1;
> }
>
> + if (info.pipelineVersionMinor != testInfo.pipelineVersionMinor) {
> + errString << "incorrect name: expected \""
> + << testInfo.pipelineVersionMinor << "\", got \""
> + << info.pipelineVersionMinor << "\"" << endl;
> + ret = -1;
> + }
> +
> + if (strcmp(info.pipelineName, testInfo.pipelineName)) {
> + errString << "incorrect name: expected \""
> + << testInfo.pipelineName << "\", got \""
> + << info.pipelineName << "\"" << endl;
> + ret = -1;
> + }
> +
> + if (strcmp(info.name, testInfo.name)) {
> + errString << "incorrect name: expected \""
> + << testInfo.name << "\", got \""
> + << info.name << "\"" << endl;
> + ret = -1;
> + }
> +
> + cerr << errString.str();
> +
> delete ll;
> return ret;
> }
> @@ -56,8 +81,10 @@ protected:
> int count = 0;
>
> const struct IPAModuleInfo testInfo = {
> + 1,
Should you use IPA_MODULE_API_VERSION as recommended by the
documentation ? Same for the shared_test.cpp file below.
> + 0, 9001,
> + "bleep",
> "It's over nine thousand!",
> - 9001,
> };
>
> count += runTest("test/ipa/ipa-dummy-cpp.so", testInfo);
> diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp
> index 4e5c976..d932a9b 100644
> --- a/test/ipa/shared_test.cpp
> +++ b/test/ipa/shared_test.cpp
> @@ -4,8 +4,10 @@ namespace libcamera {
>
> extern "C" {
> const struct libcamera::IPAModuleInfo ipaModuleInfo = {
> + 1,
> + 0, 9001,
I would go for one field per line. It would be really great if we could
use named initialisers :-S
> + "bleep",
> "It's over nine thousand!",
Let's give those fields more plausible values :-)
> - 9001,
> };
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list