| Message ID | 20190603231637.28554-4-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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, > }; > }; >
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) #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 version number specifies the version for the layout of + * struct IPAModuleInfo. + * The IPA module shall use this macro to set its moduleAPIVersion field. + * + * \sa libcamera::IPAModuleInfo::moduleAPIVersion + */ + /** * \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 + * + * This version number specifies the version for the layout of + * struct IPAModuleInfo. + * 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::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 + * + * 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)) { + 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, }; };
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@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(-)