| Message ID | 20190523164210.2105-2-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Thu, May 23, 2019 at 12:42:06PM -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 and the pipeline > matching string. > > Also update IPA module tests and Doxygen accordingly. You may why to explain why the doxygen updates are needed, it's not immediately evident from the code. > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > Changes in v2: > - make Doxygen stop complaining about undocumented __attribute__((packed)) > > Documentation/Doxyfile.in | 7 +++-- > include/libcamera/ipa/ipa_module_info.h | 8 ++++-- > test/ipa/ipa_test.cpp | 37 +++++++++++++++++-------- > test/ipa/shared_test.c | 4 ++- > test/ipa/shared_test.cpp | 6 ++-- > 5 files changed, 43 insertions(+), 19 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..f3ae97d 100644 > --- a/include/libcamera/ipa/ipa_module_info.h > +++ b/include/libcamera/ipa/ipa_module_info.h > @@ -7,14 +7,18 @@ > #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__ > #define __LIBCAMERA_IPA_MODULE_INFO_H__ > > +#include <stdint.h> > + > #ifdef __cplusplus > namespace libcamera { > #endif > > struct IPAModuleInfo { > + uint32_t ipaAPIVersion; I wonder if we need the ipa prefix here, as the field is defined in the context of the IPAModuleInfo structure. apiVersion may be enough. Still nit-picking, API may also not be the best word. As discussed previously, we need two version numbers, one to identify the version of this structure, and thus the top-level API exposed by the IPA module, and one to identify the version of the pipeline handler-specific API. Both could qualify to be called APIs. One option would be to rename the fields to moduleAPIVersion and pipelineAPIVersion. Please feel free to propose other names. > + uint32_t pipelineVersion; > + char pipelineName[256]; > char name[256]; This is growing fast :-) Maybe we should reduce them slightly ? As discussed privately on IRC a const char * would be nice, but that would require implementing support for rellocation in our ELF parser, which I don't think is a good idea. > - unsigned int version; > -}; > +} __attribute__((packed)); All fields of this structure need to be documented, even more so as they're part of the external API of libcamera. I know how much developers usually like to write documentation, but we try to take this seriously in libcamera. For external APIs we really need to go to the details, not just a single short sentence that duplicates the variable name. > #ifdef __cplusplus > extern "C" { > diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp > index 9861ee2..ca15fbd 100644 > --- a/test/ipa/ipa_test.cpp > +++ b/test/ipa/ipa_test.cpp > @@ -33,6 +33,19 @@ protected: > > const struct IPAModuleInfo &info = ll->info(); > > + if (info.ipaAPIVersion != testInfo.ipaAPIVersion) { > + cerr << "test IPA module has incorrect ipaAPIVersion" << endl; > + cerr << "expected \"" << testInfo.ipaAPIVersion << "\", got \"" > + << info.ipaAPIVersion << "\"" << endl; > + ret = -1; > + } > + if (info.pipelineVersion != testInfo.pipelineVersion) { > + cerr << "test IPA module has incorrect pipelineVersion" << endl; > + cerr << "expected \"" << testInfo.pipelineVersion << "\", got \"" > + << info.pipelineVersion << "\"" << endl; > + ret = -1; > + } Do we really need to duplicate all the error messages ? Could we group all the checks and print a single message in case of a mismatch ? > + > if (strcmp(info.name, testInfo.name)) { > cerr << "test IPA module has incorrect name" << endl; > cerr << "expected \"" << testInfo.name << "\", got \"" > @@ -40,13 +53,6 @@ protected: > 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; > - } > - > delete ll; > return ret; > } > @@ -55,14 +61,23 @@ protected: > { > int count = 0; > > - const struct IPAModuleInfo testInfo = { > - "It's over nine thousand!", > + const struct IPAModuleInfo testInfo1 = { > + 1, > 9001, > + "bloop", > + "It's over nine thousand!", > + }; > + > + const struct IPAModuleInfo testInfo2 = { > + 1, > + 8999, > + "bleep", > + "It's under nine thousand!", > }; > > - count += runTest("test/ipa/ipa-dummy-c.so", testInfo); > + count += runTest("test/ipa/ipa-dummy-c.so", testInfo1); > > - count += runTest("test/ipa/ipa-dummy-cpp.so", testInfo); > + count += runTest("test/ipa/ipa-dummy-cpp.so", testInfo2); > > if (count < 0) > return TestFail; > diff --git a/test/ipa/shared_test.c b/test/ipa/shared_test.c > index 87d182b..0046ace 100644 > --- a/test/ipa/shared_test.c > +++ b/test/ipa/shared_test.c > @@ -1,6 +1,8 @@ > #include <libcamera/ipa/ipa_module_info.h> > > const struct IPAModuleInfo ipaModuleInfo = { > + .ipaAPIVersion = 1, > + .pipelineVersion = 9001, > + .pipelineName = "bloop", > .name = "It's over nine thousand!", > - .version = 9001, > }; > diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp > index 4e5c976..78405b1 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 = { > - "It's over nine thousand!", > - 9001, > + 1, > + 8999, > + "bleep", > + "It's under nine thousand!", Is there a reason to use different values ? I think I would also make the values a bit more realistic, to better show what we expect. > }; > }; >
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..f3ae97d 100644 --- a/include/libcamera/ipa/ipa_module_info.h +++ b/include/libcamera/ipa/ipa_module_info.h @@ -7,14 +7,18 @@ #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__ #define __LIBCAMERA_IPA_MODULE_INFO_H__ +#include <stdint.h> + #ifdef __cplusplus namespace libcamera { #endif struct IPAModuleInfo { + uint32_t ipaAPIVersion; + uint32_t pipelineVersion; + char pipelineName[256]; char name[256]; - unsigned int version; -}; +} __attribute__((packed)); #ifdef __cplusplus extern "C" { diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp index 9861ee2..ca15fbd 100644 --- a/test/ipa/ipa_test.cpp +++ b/test/ipa/ipa_test.cpp @@ -33,6 +33,19 @@ protected: const struct IPAModuleInfo &info = ll->info(); + if (info.ipaAPIVersion != testInfo.ipaAPIVersion) { + cerr << "test IPA module has incorrect ipaAPIVersion" << endl; + cerr << "expected \"" << testInfo.ipaAPIVersion << "\", got \"" + << info.ipaAPIVersion << "\"" << endl; + ret = -1; + } + if (info.pipelineVersion != testInfo.pipelineVersion) { + cerr << "test IPA module has incorrect pipelineVersion" << endl; + cerr << "expected \"" << testInfo.pipelineVersion << "\", got \"" + << info.pipelineVersion << "\"" << endl; + ret = -1; + } + if (strcmp(info.name, testInfo.name)) { cerr << "test IPA module has incorrect name" << endl; cerr << "expected \"" << testInfo.name << "\", got \"" @@ -40,13 +53,6 @@ protected: 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; - } - delete ll; return ret; } @@ -55,14 +61,23 @@ protected: { int count = 0; - const struct IPAModuleInfo testInfo = { - "It's over nine thousand!", + const struct IPAModuleInfo testInfo1 = { + 1, 9001, + "bloop", + "It's over nine thousand!", + }; + + const struct IPAModuleInfo testInfo2 = { + 1, + 8999, + "bleep", + "It's under nine thousand!", }; - count += runTest("test/ipa/ipa-dummy-c.so", testInfo); + count += runTest("test/ipa/ipa-dummy-c.so", testInfo1); - count += runTest("test/ipa/ipa-dummy-cpp.so", testInfo); + count += runTest("test/ipa/ipa-dummy-cpp.so", testInfo2); if (count < 0) return TestFail; diff --git a/test/ipa/shared_test.c b/test/ipa/shared_test.c index 87d182b..0046ace 100644 --- a/test/ipa/shared_test.c +++ b/test/ipa/shared_test.c @@ -1,6 +1,8 @@ #include <libcamera/ipa/ipa_module_info.h> const struct IPAModuleInfo ipaModuleInfo = { + .ipaAPIVersion = 1, + .pipelineVersion = 9001, + .pipelineName = "bloop", .name = "It's over nine thousand!", - .version = 9001, }; diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp index 4e5c976..78405b1 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 = { - "It's over nine thousand!", - 9001, + 1, + 8999, + "bleep", + "It's under nine thousand!", }; };
We need a way to match pipelines with IPA modules, so add fields in IPAModuleInfo to hold the IPA API version number and the pipeline matching string. Also update IPA module tests and Doxygen accordingly. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v2: - make Doxygen stop complaining about undocumented __attribute__((packed)) Documentation/Doxyfile.in | 7 +++-- include/libcamera/ipa/ipa_module_info.h | 8 ++++-- test/ipa/ipa_test.cpp | 37 +++++++++++++++++-------- test/ipa/shared_test.c | 4 ++- test/ipa/shared_test.cpp | 6 ++-- 5 files changed, 43 insertions(+), 19 deletions(-)