[{"id":1688,"web_url":"https://patchwork.libcamera.org/comment/1688/","msgid":"<20190523200642.GC21601@pendragon.ideasonboard.com>","date":"2019-05-23T20:06:42","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera:\n\tipa_module_info: update 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 Thu, May 23, 2019 at 12:42:06PM -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 and the pipeline\n> matching string.\n> \n> Also update IPA module tests and Doxygen accordingly.\n\nYou may why to explain why the doxygen updates are needed, it's not\nimmediately evident from the code.\n\n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - make Doxygen stop complaining about undocumented __attribute__((packed))\n> \n> Documentation/Doxyfile.in | 7 +++--\n> include/libcamera/ipa/ipa_module_info.h | 8 ++++--\n> test/ipa/ipa_test.cpp | 37 +++++++++++++++++--------\n> test/ipa/shared_test.c | 4 ++-\n> test/ipa/shared_test.cpp | 6 ++--\n> 5 files changed, 43 insertions(+), 19 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 4e0d668..f3ae97d 100644\n> --- a/include/libcamera/ipa/ipa_module_info.h\n> +++ b/include/libcamera/ipa/ipa_module_info.h\n> @@ -7,14 +7,18 @@\n> #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__\n> #define __LIBCAMERA_IPA_MODULE_INFO_H__\n> \n> +#include \n> +\n> #ifdef __cplusplus\n> namespace libcamera {\n> #endif\n> \n> struct IPAModuleInfo {\n> +\tuint32_t ipaAPIVersion;\n\nI wonder if we need the ipa prefix here, as the field is defined in the\ncontext of the IPAModuleInfo structure. apiVersion may be enough. Still\nnit-picking, API may also not be the best word. As discussed previously,\nwe need two version numbers, one to identify the version of this\nstructure, and thus the top-level API exposed by the IPA module, and one\nto identify the version of the pipeline handler-specific API. Both could\nqualify to be called APIs.\n\nOne option would be to rename the fields to moduleAPIVersion and\npipelineAPIVersion. Please feel free to propose other names.\n\n> +\tuint32_t pipelineVersion;\n> +\tchar pipelineName[256];\n> \tchar name[256];\n\nThis is growing fast :-) Maybe we should reduce them slightly ? As\ndiscussed privately on IRC a const char * would be nice, but that would\nrequire implementing support for rellocation in our ELF parser, which I\ndon't think is a good idea.\n\n> -\tunsigned int version;\n> -};\n> +} __attribute__((packed));\n\nAll fields of this structure need to be documented, even more so as\nthey're part of the external API of libcamera. I know how much\ndevelopers usually like to write documentation, but we try to take this\nseriously in libcamera. For external APIs we really need to go to the\ndetails, not just a single short sentence that duplicates the variable\nname.\n\n> #ifdef __cplusplus\n> extern \"C\" {\n> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp\n> index 9861ee2..ca15fbd 100644\n> --- a/test/ipa/ipa_test.cpp\n> +++ b/test/ipa/ipa_test.cpp\n> @@ -33,6 +33,19 @@ protected:\n> \n> \t\tconst struct IPAModuleInfo &info = ll->info();\n> \n> +\t\tif (info.ipaAPIVersion != testInfo.ipaAPIVersion) {\n> +\t\t\tcerr << \"test IPA module has incorrect ipaAPIVersion\" << endl;\n> +\t\t\tcerr << \"expected \\\"\" << testInfo.ipaAPIVersion << \"\\\", got \\\"\"\n> +\t\t\t << info.ipaAPIVersion << \"\\\"\" << endl;\n> +\t\t\tret = -1;\n> +\t\t}\n> +\t\tif (info.pipelineVersion != testInfo.pipelineVersion) {\n> +\t\t\tcerr << \"test IPA module has incorrect pipelineVersion\" << endl;\n> +\t\t\tcerr << \"expected \\\"\" << testInfo.pipelineVersion << \"\\\", got \\\"\"\n> +\t\t\t << info.pipelineVersion << \"\\\"\" << endl;\n> +\t\t\tret = -1;\n> +\t\t}\n\nDo we really need to duplicate all the error messages ? Could we group\nall the checks and print a single message in case of a mismatch ?\n\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> @@ -40,13 +53,6 @@ protected:\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\t}\n> -\n> \t\tdelete ll;\n> \t\treturn ret;\n> \t}\n> @@ -55,14 +61,23 @@ protected:\n> \t{\n> \t\tint count = 0;\n> \n> -\t\tconst struct IPAModuleInfo testInfo = {\n> -\t\t\t\"It's over nine thousand!\",\n> +\t\tconst struct IPAModuleInfo testInfo1 = {\n> +\t\t\t1,\n> \t\t\t9001,\n> +\t\t\t\"bloop\",\n> +\t\t\t\"It's over nine thousand!\",\n> +\t\t};\n> +\n> +\t\tconst struct IPAModuleInfo testInfo2 = {\n> +\t\t\t1,\n> +\t\t\t8999,\n> +\t\t\t\"bleep\",\n> +\t\t\t\"It's under nine thousand!\",\n> \t\t};\n> \n> -\t\tcount += runTest(\"test/ipa/ipa-dummy-c.so\", testInfo);\n> +\t\tcount += runTest(\"test/ipa/ipa-dummy-c.so\", testInfo1);\n> \n> -\t\tcount += runTest(\"test/ipa/ipa-dummy-cpp.so\", testInfo);\n> +\t\tcount += runTest(\"test/ipa/ipa-dummy-cpp.so\", testInfo2);\n> \n> \t\tif (count < 0)\n> \t\t\treturn TestFail;\n> diff --git a/test/ipa/shared_test.c b/test/ipa/shared_test.c\n> index 87d182b..0046ace 100644\n> --- a/test/ipa/shared_test.c\n> +++ b/test/ipa/shared_test.c\n> @@ -1,6 +1,8 @@\n> #include \n> \n> const struct IPAModuleInfo ipaModuleInfo = {\n> +\t.ipaAPIVersion = 1,\n> +\t.pipelineVersion = 9001,\n> +\t.pipelineName = \"bloop\",\n> \t.name = \"It's over nine thousand!\",\n> -\t.version = 9001,\n> };\n> diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp\n> index 4e5c976..78405b1 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> -\t\"It's over nine thousand!\",\n> -\t9001,\n> +\t1,\n> +\t8999,\n> +\t\"bleep\",\n> +\t\"It's under nine thousand!\",\n\nIs there a reason to use different values ?\n\nI think I would also make the values a bit more realistic, to better\nshow what we expect.\n\n> };\n> };\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 340A360E9A\n\tfor ;\n\tThu, 23 May 2019 22:07:01 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 806B75A9;\n\tThu, 23 May 2019 22:07:00 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558642020;\n\tbh=f8Co//fMr8OfIEQ+FV+94Iv6ePUs13kGgb8TElUk25M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sDP1tfGLXzEXgtrskc1G8rascQORcvd6Opxg72SQyavK3KNJGf9EaYGg+BGWxcAv1\n\tjEDqF5I1/ZDrzaf5t1Z2Qqaryus+bITNC0uxoJSObbcS+Zw3fkTOu/88nBsFMDJS5p\n\tK7n0P2UrTouQXAsp4aM2iuabjqXMKKqgXqPpq+r8=","Date":"Thu, 23 May 2019 23:06:42 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190523200642.GC21601@pendragon.ideasonboard.com>","References":"<20190523164210.2105-1-paul.elder@ideasonboard.com>\n\t<20190523164210.2105-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190523164210.2105-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera:\n\tipa_module_info: update 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":"Thu, 23 May 2019 20:07:01 -0000"}}]