[{"id":1753,"web_url":"https://patchwork.libcamera.org/comment/1753/","msgid":"<20190604104047.GF4771@pendragon.ideasonboard.com>","date":"2019-06-04T10:40:47","subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: ipa_module_info:\n\tupdate struct to allow IPA matching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Jun 03, 2019 at 07:16:30PM -0400, Paul Elder wrote:\n> We need a way to match pipelines with IPA modules, so add fields in\n> IPAModuleInfo to hold the IPA API version number, the pipeline name, and\n> the pipeline version.\n> \n> Also update IPA module tests and Doxygen accordingly. Doxygen needs to\n> be updated to accomodate __attribute__((packed)).\n> \n> Signed-off-by: Paul Elder \n> ---\n> - combine pipeline major and minor versions into one using the\n> PIPELINE_VERSION macro, and update tests accordingly\n> \n> Documentation/Doxyfile.in | 7 ++--\n> include/libcamera/ipa/ipa_module_info.h | 10 ++++--\n> src/libcamera/ipa_module.cpp | 46 +++++++++++++++++++++----\n> test/ipa/ipa_test.cpp | 30 +++++++++-------\n> test/ipa/shared_test.cpp | 4 ++-\n> 5 files changed, 72 insertions(+), 25 deletions(-)\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 67eaded..ac70efb 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -2016,7 +2016,7 @@ ENABLE_PREPROCESSING = YES\n> # The default value is: NO.\n> # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.\n> \n> -MACRO_EXPANSION = NO\n> +MACRO_EXPANSION = YES\n> \n> # If the EXPAND_ONLY_PREDEF and MACRO_EXPANSION tags are both set to YES then\n> # the macro expansion is limited to the macros specified with the PREDEFINED and\n> @@ -2024,7 +2024,7 @@ MACRO_EXPANSION = NO\n> # The default value is: NO.\n> # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.\n> \n> -EXPAND_ONLY_PREDEF = NO\n> +EXPAND_ONLY_PREDEF = YES\n> \n> # If the SEARCH_INCLUDES tag is set to YES, the include files in the\n> # INCLUDE_PATH will be searched if a #include is found.\n> @@ -2057,7 +2057,8 @@ INCLUDE_FILE_PATTERNS = *.h\n> # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.\n> \n> PREDEFINED = __DOXYGEN__ \\\n> - __cplusplus\n> + __cplusplus \\\n> + __attribute__(x)=\n> \n> # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this\n> # tag can be used to specify a list of macro names that should be expanded. The\n> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h\n> index e5f2a7e..df9b47e 100644\n> --- a/include/libcamera/ipa/ipa_module_info.h\n> +++ b/include/libcamera/ipa/ipa_module_info.h\n> @@ -7,6 +7,10 @@\n> #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__\n> #define __LIBCAMERA_IPA_MODULE_INFO_H__\n> \n> +#include \n> +\n> +#define IPA_MODULE_API_VERSION 1\n> +\n> #define PIPELINE_VERSION(majorV, minorV) (majorV * 1000 + minorV)\n\nThis is something I missed in a previous patch, I would store the major\nin the upper 16 bits and the minor in the lower 16 bits. You also need\nto enclose majorV and minorV in parentheses, otherwise\n\n\tPIPELINE_VERSION(1+1, 3)\n\nwill not be equivalent to\n\n\tPIPELINE_VERSION(2, 3)\n\n> \n> #ifdef __cplusplus\n> @@ -14,9 +18,11 @@ namespace libcamera {\n> #endif\n> \n> struct IPAModuleInfo {\n> +\tint moduleAPIVersion;\n> +\tuint32_t pipelineVersion;\n> +\tchar pipelineName[256];\n> \tchar name[256];\n> -\tunsigned int version;\n> -};\n> +} __attribute__((packed));\n> \n> #ifdef __cplusplus\n> extern \"C\" {\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index db56415..067f868 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -169,6 +169,17 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n> \n> } /* namespace */\n> \n> +/**\n> + * \\def IPA_MODULE_API_VERSION\n> + * \\brief The IPA API version\n\nThis is not the API version but the version of the module info\nstructure layout, so I would describe it as \"The IPA module API\nversion\".\n\n> + *\n> + * This version number specifies the version for the layout of\n> + * struct IPAModuleInfo.\n> + * The IPA module shall use this macro to set its moduleAPIVersion field.\n\nIf you want a single paragraph there's no need for a line break after\nthe first sentence. Otherwise you need a blank line.\n\n> + *\n> + * \\sa libcamera::IPAModuleInfo::moduleAPIVersion\n\nYou are in the libcamera namespace so I think you can drop the\nlibcamera:: prefix.\n\n> + */\n> +\n> /**\n> * \\def PIPELINE_VERSION\n> * \\brief Convert a major and minor version pair to a single version number\n> @@ -193,14 +204,37 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n> * This structure contains the information of an IPA module. It is loaded,\n> * read, and validated before anything else is loaded from the shared object.\n> *\n> - * \\var IPAModuleInfo::name\n> - * \\brief The name of the IPA module\n> + * \\var IPAModuleInfo::moduleAPIVersion\n> + * \\brief The IPA API version that the IPA module implements\n\nSame as above, s/IPA API/IPA module API/ ?\n\n> + *\n> + * This version number specifies the version for the layout of\n> + * struct IPAModuleInfo.\n\nSame comment abot the line break.\n\n> + * The IPA module shall report here the version that it was built for,\n> + * using the macro IPA_MODULE_API_VERSION.\n> *\n> - * \\var IPAModuleInfo::version\n> - * \\brief The version of the IPA module\n> + * \\sa IPA_MODULE_API_VERSION\n\nDoesn't doxygen generate a link for IPA_MODULE_API_VERSION in the\nprevious sentence, without the need for an additional \\sa here ? If it\ndoesn't, I would force link generation with \\ref instead of adding a\n\\sa.\n\n> *\n> - * \\todo abi compatability version\n> - * \\todo pipeline compatability matcher\n> + * \\var IPAModuleInfo::pipelineVersion\n> + * \\brief The pipeline handler version that the IPA module is for\n> + *\n> + * This field shall be constructed from a major and minor version using the\n> + * macro PIPELINE_VERSION.\n> + *\n> + * The major version of the module and pipeline handler must be equal in\n> + * order for the pipeline handler and the module to be matched.\n> + *, and the API between pipeline handlers and the IPA\n\nDid you forget to remove this sentence ?\n\n> + *\n> + * The minor version of the module must be equal to or greater than\n> + * the pipeline handler minor version in order for the pipeline handler\n> + * and the module to be matched.\n> + *\n> + * \\var IPAModuleInfo::pipelineName\n> + * \\brief The name of the pipeline handler that the IPA module is for\n> + *\n> + * This name is used to match a pipeline handler with the module.\n> + *\n> + * \\var IPAModuleInfo::name\n> + * \\brief The name of the IPA module\n> */\n> \n> /**\n> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp\n> index 2dbc702..d7b18c7 100644\n> --- a/test/ipa/ipa_test.cpp\n> +++ b/test/ipa/ipa_test.cpp\n> @@ -33,18 +33,20 @@ protected:\n> \n> \t\tconst struct IPAModuleInfo &info = ll->info();\n> \n> -\t\tif (strcmp(info.name, testInfo.name)) {\n> -\t\t\tcerr << \"test IPA module has incorrect name\" << endl;\n> -\t\t\tcerr << \"expected \\\"\" << testInfo.name << \"\\\", got \\\"\"\n> -\t\t\t << info.name << \"\\\"\" << endl;\n> -\t\t\tret = -1;\n> -\t\t}\n> -\n> -\t\tif (info.version != testInfo.version) {\n> -\t\t\tcerr << \"test IPA module has incorrect version\" << endl;\n> -\t\t\tcerr << \"expected \\\"\" << testInfo.version << \"\\\", got \\\"\"\n> -\t\t\t << info.version << \"\\\"\" << endl;\n> -\t\t\tret = -1;\n> +\t\tif (info.moduleAPIVersion != testInfo.moduleAPIVersion ||\n> +\t\t info.pipelineVersion != testInfo.pipelineVersion ||\n> +\t\t strcmp(info.pipelineName, testInfo.pipelineName) ||\n> +\t\t strcmp(info.name, testInfo.name)) {\n\n\t\tif (memcmp(&info, &testInfo, sizeof info)) {\n\n> +\t\t\tcerr << \"IPA module information mismatch: expected:\" << endl\n> +\t\t\t << \"moduleAPIVersion = \" << testInfo.moduleAPIVersion << endl\n> +\t\t\t << \"pipelineVersion = \" << testInfo.pipelineVersion << endl\n> +\t\t\t << \"pipelineName = \" << testInfo.pipelineName << endl\n> +\t\t\t << \"name = \" << testInfo.name\n> +\t\t\t << \"got: \" << endl\n> +\t\t\t << \"moduleAPIVersion = \" << info.moduleAPIVersion << endl\n> +\t\t\t << \"pipelineVersion = \" << info.pipelineVersion << endl\n> +\t\t\t << \"pipelineName = \" << info.pipelineName << endl\n> +\t\t\t << \"name = \" << info.name << endl;\n> \t\t}\n> \n> \t\tdelete ll;\n> @@ -56,8 +58,10 @@ protected:\n> \t\tint count = 0;\n> \n> \t\tconst struct IPAModuleInfo testInfo = {\n> +\t\t\tIPA_MODULE_API_VERSION,\n> +\t\t\tPIPELINE_VERSION(0, 9001),\n> +\t\t\t\"bleep\",\n> \t\t\t\"It's over nine thousand!\",\n> -\t\t\t9001,\n> \t\t};\n> \n> \t\tcount += runTest(\"test/ipa/ipa-dummy-cpp.so\", testInfo);\n> diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp\n> index 4e5c976..5afc135 100644\n> --- a/test/ipa/shared_test.cpp\n> +++ b/test/ipa/shared_test.cpp\n> @@ -4,8 +4,10 @@ namespace libcamera {\n> \n> extern \"C\" {\n> const struct libcamera::IPAModuleInfo ipaModuleInfo = {\n> +\tIPA_MODULE_API_VERSION,\n> +\tPIPELINE_VERSION(0, 9001),\n> +\t\"bleep\",\n> \t\"It's over nine thousand!\",\n> -\t9001,\n> };\n> };\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 832D261EEB\n\tfor ;\n\tTue, 4 Jun 2019 12:41:00 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:2788:668:163:5bb7:9f6c:564c:d55e])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F4792D2;\n\tTue, 4 Jun 2019 12:41:00 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559644860;\n\tbh=AWEfgH7QgtCmmKsBy3oVmJ1hiphQArR2ZJI0eNLcW9Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tjnyejpF7f95oL1OyK57io58R5o9fT2loYomQdHk+XAcITDTpZKxCJ6sZXH2iMzYr\n\tndfvso8s4lV0Mm8vEhFcGt3U8yiAp/m6UeUuDdhwvRSTBmjjf5tkOVVVPw2Lj9UwHt\n\thIUmmN9LRdSAi6w4EKkriRdH74wTLJDJnmtvXdZ4=","Date":"Tue, 4 Jun 2019 13:40:47 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604104047.GF4771@pendragon.ideasonboard.com>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190603231637.28554-4-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: ipa_module_info:\n\tupdate struct to allow IPA matching","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 04 Jun 2019 10:41:00 -0000"}}]