[libcamera-devel] [PATCH v2 03/10] libcamera: ipa_module_info: update struct to allow IPA matching
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 4 12:40:47 CEST 2019
Hi Paul,
Thank you for the patch.
On Mon, Jun 03, 2019 at 07:16:30PM -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 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>
> ---
> - combine pipeline major and minor versions into one using the
> PIPELINE_VERSION macro, and update tests accordingly
>
> Documentation/Doxyfile.in | 7 ++--
> include/libcamera/ipa/ipa_module_info.h | 10 ++++--
> src/libcamera/ipa_module.cpp | 46 +++++++++++++++++++++----
> test/ipa/ipa_test.cpp | 30 +++++++++-------
> test/ipa/shared_test.cpp | 4 ++-
> 5 files changed, 72 insertions(+), 25 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 e5f2a7e..df9b47e 100644
> --- a/include/libcamera/ipa/ipa_module_info.h
> +++ b/include/libcamera/ipa/ipa_module_info.h
> @@ -7,6 +7,10 @@
> #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__
> #define __LIBCAMERA_IPA_MODULE_INFO_H__
>
> +#include <stdint.h>
> +
> +#define IPA_MODULE_API_VERSION 1
> +
> #define PIPELINE_VERSION(majorV, minorV) (majorV * 1000 + minorV)
This is something I missed in a previous patch, I would store the major
in the upper 16 bits and the minor in the lower 16 bits. You also need
to enclose majorV and minorV in parentheses, otherwise
PIPELINE_VERSION(1+1, 3)
will not be equivalent to
PIPELINE_VERSION(2, 3)
>
> #ifdef __cplusplus
> @@ -14,9 +18,11 @@ namespace libcamera {
> #endif
>
> struct IPAModuleInfo {
> + int moduleAPIVersion;
> + uint32_t pipelineVersion;
> + 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 db56415..067f868 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 is not the API version but the version of the module info
structure layout, so I would describe it as "The IPA module API
version".
> + *
> + * This version number specifies the version for the layout of
> + * struct IPAModuleInfo.
> + * The IPA module shall use this macro to set its moduleAPIVersion field.
If you want a single paragraph there's no need for a line break after
the first sentence. Otherwise you need a blank line.
> + *
> + * \sa libcamera::IPAModuleInfo::moduleAPIVersion
You are in the libcamera namespace so I think you can drop the
libcamera:: prefix.
> + */
> +
> /**
> * \def PIPELINE_VERSION
> * \brief Convert a major and minor version pair to a single version number
> @@ -193,14 +204,37 @@ 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 implements
Same as above, s/IPA API/IPA module API/ ?
> + *
> + * This version number specifies the version for the layout of
> + * struct IPAModuleInfo.
Same comment abot the line break.
> + * 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
Doesn't doxygen generate a link for IPA_MODULE_API_VERSION in the
previous sentence, without the need for an additional \sa here ? If it
doesn't, I would force link generation with \ref instead of adding a
\sa.
> *
> - * \todo abi compatability version
> - * \todo pipeline compatability matcher
> + * \var IPAModuleInfo::pipelineVersion
> + * \brief The pipeline handler version that the IPA module is for
> + *
> + * This field shall be constructed from a major and minor version using the
> + * macro PIPELINE_VERSION.
> + *
> + * The major version of the module and pipeline handler must be equal in
> + * order for the pipeline handler and the module to be matched.
> + *, and the API between pipeline handlers and the IPA
Did you forget to remove this sentence ?
> + *
> + * 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.
> + *
> + * \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..d7b18c7 100644
> --- a/test/ipa/ipa_test.cpp
> +++ b/test/ipa/ipa_test.cpp
> @@ -33,18 +33,20 @@ 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;
> - ret = -1;
> - }
> -
> - if (info.version != testInfo.version) {
> - cerr << "test IPA module has incorrect version" << endl;
> - cerr << "expected \"" << testInfo.version << "\", got \""
> - << info.version << "\"" << endl;
> - ret = -1;
> + if (info.moduleAPIVersion != testInfo.moduleAPIVersion ||
> + info.pipelineVersion != testInfo.pipelineVersion ||
> + strcmp(info.pipelineName, testInfo.pipelineName) ||
> + strcmp(info.name, testInfo.name)) {
if (memcmp(&info, &testInfo, sizeof info)) {
> + cerr << "IPA module information mismatch: expected:" << endl
> + << "moduleAPIVersion = " << testInfo.moduleAPIVersion << endl
> + << "pipelineVersion = " << testInfo.pipelineVersion << endl
> + << "pipelineName = " << testInfo.pipelineName << endl
> + << "name = " << testInfo.name
> + << "got: " << endl
> + << "moduleAPIVersion = " << info.moduleAPIVersion << endl
> + << "pipelineVersion = " << info.pipelineVersion << endl
> + << "pipelineName = " << info.pipelineName << endl
> + << "name = " << info.name << endl;
> }
>
> delete ll;
> @@ -56,8 +58,10 @@ protected:
> int count = 0;
>
> const struct IPAModuleInfo testInfo = {
> + IPA_MODULE_API_VERSION,
> + PIPELINE_VERSION(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..5afc135 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 = {
> + IPA_MODULE_API_VERSION,
> + PIPELINE_VERSION(0, 9001),
> + "bleep",
> "It's over nine thousand!",
> - 9001,
> };
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list