[{"id":4543,"web_url":"https://patchwork.libcamera.org/comment/4543/","msgid":"<20200427074605.ikbkaivl4yp6lfsn@uno.localdomain>","date":"2020-04-27T07:46:05","subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after\n\ttheir source directory","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Apr 27, 2020 at 06:17:03AM +0300, Laurent Pinchart wrote:\n> The IPAModuleInfo::name field is currently a free-formed string that has\n> little use. Tighten its usage rules to make it suitable for building\n> file system paths to IPA-specific resources by matching the directory\n> name of the IPA module.\n>\n\nI wonder, should we use 'name' for this purpose, or should we get the\nfile name directly, strip the .cpp extension out and add a field for\nthat purpose ? I feel maintaining the name in synch with the file name\nis a (relatively small) burden we could avoid.\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> src/ipa/vimc/vimc.cpp | 2 +-\n> src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++\n> test/ipa/ipa_module_test.cpp | 2 +-\n> 4 files changed, 23 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index acbbe58c7a2e..7f0ffb0a791f 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -274,7 +274,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> \tIPA_MODULE_API_VERSION,\n> \t1,\n> \t\"PipelineHandlerRkISP1\",\n> -\t\"RkISP1 IPA\",\n> +\t\"rkisp1\",\n> };\n>\n> struct ipa_context *ipaCreate()\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index d2267e11737f..cfdbd6f99155 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -126,7 +126,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> \tIPA_MODULE_API_VERSION,\n> \t0,\n> \t\"PipelineHandlerVimc\",\n> -\t\"Dummy IPA for Vimc\",\n> +\t\"vimc\",\n> };\n>\n> struct ipa_context *ipaCreate()\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 958ede74688e..52823e88a508 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -216,6 +217,11 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n> * \\var IPAModuleInfo::name\n> * \\brief The name of the IPA module\n> *\n> + * The name may be used to build file system paths to IPA-specific resources.\n> + * It shall only contain printable characters, and may not contain '/', '*',\n> + * '?' or '\\'. For IPA modules included in libcamera, it shall match the\n> + * directory of the IPA module in the source tree.\n> + *\n> * \\todo Allow user to choose to isolate open source IPAs\n> */\n>\n> @@ -287,6 +293,20 @@ int IPAModule::loadIPAModuleInfo()\n> \t\treturn -EINVAL;\n> \t}\n>\n> +\t/* Validate the IPA module name. */\n> +\tstd::string ipaName = info_.name;\n> +\tauto iter = std::find_if_not(ipaName.begin(), ipaName.end(),\n> +\t\t\t\t [](unsigned char c) -> bool {\n> +\t\t\t\t\t return isprint(c) && c != '/' &&\n> +\t\t\t\t\t\t c != '?' && c != '*' &&\n> +\t\t\t\t\t\t c != '\\\\';\n> +\t\t\t\t });\n> +\tif (iter != ipaName.end()) {\n> +\t\tLOG(IPAModule, Error)\n> +\t\t\t<< \"Invalid IPA module name '\" << ipaName << \"'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> \t/* Load the signature. Failures are not fatal. */\n> \tFile sign{ libPath_ + \".sign\" };\n> \tif (!sign.open(File::ReadOnly)) {\n> diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp\n> index d22c302fa726..e3aee190b410 100644\n> --- a/test/ipa/ipa_module_test.cpp\n> +++ b/test/ipa/ipa_module_test.cpp\n> @@ -58,7 +58,7 @@ protected:\n> \t\t\tIPA_MODULE_API_VERSION,\n> \t\t\t0,\n> \t\t\t\"PipelineHandlerVimc\",\n> -\t\t\t\"Dummy IPA for Vimc\",\n> +\t\t\t\"vimc\",\n> \t\t};\n>\n> \t\tcount += runTest(\"src/ipa/vimc/ipa_vimc.so\", testInfo);\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C55A4603F8\n\tfor ;\n\tMon, 27 Apr 2020 09:42:56 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 6D1DF100014;\n\tMon, 27 Apr 2020 07:42:55 +0000 (UTC)"],"Date":"Mon, 27 Apr 2020 09:46:05 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427074605.ikbkaivl4yp6lfsn@uno.localdomain>","References":"<20200427031713.14013-1-laurent.pinchart@ideasonboard.com>\n\t<20200427031713.14013-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200427031713.14013-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after\n\ttheir source directory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 27 Apr 2020 07:42:57 -0000"}},{"id":4558,"web_url":"https://patchwork.libcamera.org/comment/4558/","msgid":"<48eabf44-b077-71b2-008c-ca62af786098@ideasonboard.com>","date":"2020-04-27T10:25:49","subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after\n\ttheir source directory","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 27/04/2020 08:46, Jacopo Mondi wrote:\n> Hi Laurent,\n> \n> On Mon, Apr 27, 2020 at 06:17:03AM +0300, Laurent Pinchart wrote:\n>> The IPAModuleInfo::name field is currently a free-formed string that has\n>> little use. Tighten its usage rules to make it suitable for building\n>> file system paths to IPA-specific resources by matching the directory\n>> name of the IPA module.\n>>\n> \n> I wonder, should we use 'name' for this purpose, or should we get the\n> file name directly, strip the .cpp extension out and add a field for\n> that purpose ? I feel maintaining the name in synch with the file name\n> is a (relatively small) burden we could avoid.\n\nHrm ... some interesting ideas on a google search to get the stripped\nfilename at compile time:\nhttps://gist.github.com/huangml/3221950a96154180467a ... but I think in\nthis instance it requires the directory name - not the file name.\n(though equally, perhaps they should be the same too).\n\nI also wonder if perhaps to get the actual desired effect required\n(location of the IPA tree) we couldn't use a method similar to the\nbuildPath() ... but maybe that's more work for little current gain, and\nmaking that path buildable is a simple solution for the time being.\n\n\nReviewed-by: Kieran Bingham \n\n\n\n> \n>> Signed-off-by: Laurent Pinchart \n>> ---\n>> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n>> src/ipa/vimc/vimc.cpp | 2 +-\n>> src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++\n>> test/ipa/ipa_module_test.cpp | 2 +-\n>> 4 files changed, 23 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index acbbe58c7a2e..7f0ffb0a791f 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -274,7 +274,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n>> \tIPA_MODULE_API_VERSION,\n>> \t1,\n>> \t\"PipelineHandlerRkISP1\",\n>> -\t\"RkISP1 IPA\",\n>> +\t\"rkisp1\",\n>> };\n>>\n>> struct ipa_context *ipaCreate()\n>> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n>> index d2267e11737f..cfdbd6f99155 100644\n>> --- a/src/ipa/vimc/vimc.cpp\n>> +++ b/src/ipa/vimc/vimc.cpp\n>> @@ -126,7 +126,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n>> \tIPA_MODULE_API_VERSION,\n>> \t0,\n>> \t\"PipelineHandlerVimc\",\n>> -\t\"Dummy IPA for Vimc\",\n>> +\t\"vimc\",\n>> };\n>>\n>> struct ipa_context *ipaCreate()\n>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n>> index 958ede74688e..52823e88a508 100644\n>> --- a/src/libcamera/ipa_module.cpp\n>> +++ b/src/libcamera/ipa_module.cpp\n>> @@ -9,6 +9,7 @@\n>>\n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -216,6 +217,11 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n>> * \\var IPAModuleInfo::name\n>> * \\brief The name of the IPA module\n>> *\n>> + * The name may be used to build file system paths to IPA-specific resources.\n>> + * It shall only contain printable characters, and may not contain '/', '*',\n>> + * '?' or '\\'. For IPA modules included in libcamera, it shall match the\n>> + * directory of the IPA module in the source tree.\n>> + *\n>> * \\todo Allow user to choose to isolate open source IPAs\n>> */\n>>\n>> @@ -287,6 +293,20 @@ int IPAModule::loadIPAModuleInfo()\n>> \t\treturn -EINVAL;\n>> \t}\n>>\n>> +\t/* Validate the IPA module name. */\n>> +\tstd::string ipaName = info_.name;\n>> +\tauto iter = std::find_if_not(ipaName.begin(), ipaName.end(),\n>> +\t\t\t\t [](unsigned char c) -> bool {\n>> +\t\t\t\t\t return isprint(c) && c != '/' &&\n>> +\t\t\t\t\t\t c != '?' && c != '*' &&\n>> +\t\t\t\t\t\t c != '\\\\';\n>> +\t\t\t\t });\n>> +\tif (iter != ipaName.end()) {\n>> +\t\tLOG(IPAModule, Error)\n>> +\t\t\t<< \"Invalid IPA module name '\" << ipaName << \"'\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> \t/* Load the signature. Failures are not fatal. */\n>> \tFile sign{ libPath_ + \".sign\" };\n>> \tif (!sign.open(File::ReadOnly)) {\n>> diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp\n>> index d22c302fa726..e3aee190b410 100644\n>> --- a/test/ipa/ipa_module_test.cpp\n>> +++ b/test/ipa/ipa_module_test.cpp\n>> @@ -58,7 +58,7 @@ protected:\n>> \t\t\tIPA_MODULE_API_VERSION,\n>> \t\t\t0,\n>> \t\t\t\"PipelineHandlerVimc\",\n>> -\t\t\t\"Dummy IPA for Vimc\",\n>> +\t\t\t\"vimc\",\n>> \t\t};\n>>\n>> \t\tcount += runTest(\"src/ipa/vimc/ipa_vimc.so\", testInfo);\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 B754F603F9\n\tfor ;\n\tMon, 27 Apr 2020 12:25:53 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 91DCB72C;\n\tMon, 27 Apr 2020 12:25:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iqolQ3WQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587983153;\n\tbh=0p3jUSZyh108HsrHdcbS7Qgmuroms7/RzRIS0qLEF+0=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=iqolQ3WQ5T4NHjouuymVEZeA7mYMg2179u3dh/baKtffdKWApgiwC+9y/Hdxnq44a\n\ttmMHtvnR2kSJro4qdsW8PKLIvuSWH1HpLvhBPZZrJ4LIi5TuzVk3iL9ORZ7H7/IKl3\n\tKfUi8nAFtIyU+7zrrQLP1wXaeA2AUTcLybR4dL2I=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20200427031713.14013-1-laurent.pinchart@ideasonboard.com>\n\t<20200427031713.14013-2-laurent.pinchart@ideasonboard.com>\n\t<20200427074605.ikbkaivl4yp6lfsn@uno.localdomain>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<48eabf44-b077-71b2-008c-ca62af786098@ideasonboard.com>","Date":"Mon, 27 Apr 2020 11:25:49 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200427074605.ikbkaivl4yp6lfsn@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after\n\ttheir source directory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 27 Apr 2020 10:25:53 -0000"}},{"id":4560,"web_url":"https://patchwork.libcamera.org/comment/4560/","msgid":"<20200427103544.GA5849@pendragon.ideasonboard.com>","date":"2020-04-27T10:35:44","subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after\n\ttheir source directory","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello Jacopo and Kieran,\n\nOn Mon, Apr 27, 2020 at 11:25:49AM +0100, Kieran Bingham wrote:\n> On 27/04/2020 08:46, Jacopo Mondi wrote:\n> > On Mon, Apr 27, 2020 at 06:17:03AM +0300, Laurent Pinchart wrote:\n> >> The IPAModuleInfo::name field is currently a free-formed string that has\n> >> little use. Tighten its usage rules to make it suitable for building\n> >> file system paths to IPA-specific resources by matching the directory\n> >> name of the IPA module.\n> > \n> > I wonder, should we use 'name' for this purpose, or should we get the\n> > file name directly, strip the .cpp extension out and add a field for\n> > that purpose ? I feel maintaining the name in synch with the file name\n> > is a (relatively small) burden we could avoid.\n\nI'm open to ideas to automate this, but I'd rather do so on top of this\nseries, as it's a minor improvement in my opinion.\n\n> Hrm ... some interesting ideas on a google search to get the stripped\n> filename at compile time:\n> https://gist.github.com/huangml/3221950a96154180467a ... but I think in\n> this instance it requires the directory name - not the file name.\n> (though equally, perhaps they should be the same too).\n\nIt shouldn't be too difficult to retrieve the directory name, but I'd\nrather try to improve the tooling in meson first, possibly using a\ngenerator, to help with that.\n\n> I also wonder if perhaps to get the actual desired effect required\n> (location of the IPA tree) we couldn't use a method similar to the\n> buildPath() ... but maybe that's more work for little current gain, and\n> making that path buildable is a simple solution for the time being.\n\nDon't forget that the name is loaded from the IPAModuleInfo structure\nwithout executing any code from the IPA module, so it has to be a static\nstring.\n\n> Reviewed-by: Kieran Bingham \n> \n> >> Signed-off-by: Laurent Pinchart \n> >> ---\n> >> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> >> src/ipa/vimc/vimc.cpp | 2 +-\n> >> src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++\n> >> test/ipa/ipa_module_test.cpp | 2 +-\n> >> 4 files changed, 23 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index acbbe58c7a2e..7f0ffb0a791f 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -274,7 +274,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> >> \tIPA_MODULE_API_VERSION,\n> >> \t1,\n> >> \t\"PipelineHandlerRkISP1\",\n> >> -\t\"RkISP1 IPA\",\n> >> +\t\"rkisp1\",\n> >> };\n> >>\n> >> struct ipa_context *ipaCreate()\n> >> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> >> index d2267e11737f..cfdbd6f99155 100644\n> >> --- a/src/ipa/vimc/vimc.cpp\n> >> +++ b/src/ipa/vimc/vimc.cpp\n> >> @@ -126,7 +126,7 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> >> \tIPA_MODULE_API_VERSION,\n> >> \t0,\n> >> \t\"PipelineHandlerVimc\",\n> >> -\t\"Dummy IPA for Vimc\",\n> >> +\t\"vimc\",\n> >> };\n> >>\n> >> struct ipa_context *ipaCreate()\n> >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> >> index 958ede74688e..52823e88a508 100644\n> >> --- a/src/libcamera/ipa_module.cpp\n> >> +++ b/src/libcamera/ipa_module.cpp\n> >> @@ -9,6 +9,7 @@\n> >>\n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> @@ -216,6 +217,11 @@ Span elfLoadSymbol(Span elf, const char *symbol)\n> >> * \\var IPAModuleInfo::name\n> >> * \\brief The name of the IPA module\n> >> *\n> >> + * The name may be used to build file system paths to IPA-specific resources.\n> >> + * It shall only contain printable characters, and may not contain '/', '*',\n> >> + * '?' or '\\'. For IPA modules included in libcamera, it shall match the\n> >> + * directory of the IPA module in the source tree.\n> >> + *\n> >> * \\todo Allow user to choose to isolate open source IPAs\n> >> */\n> >>\n> >> @@ -287,6 +293,20 @@ int IPAModule::loadIPAModuleInfo()\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >>\n> >> +\t/* Validate the IPA module name. */\n> >> +\tstd::string ipaName = info_.name;\n> >> +\tauto iter = std::find_if_not(ipaName.begin(), ipaName.end(),\n> >> +\t\t\t\t [](unsigned char c) -> bool {\n> >> +\t\t\t\t\t return isprint(c) && c != '/' &&\n> >> +\t\t\t\t\t\t c != '?' && c != '*' &&\n> >> +\t\t\t\t\t\t c != '\\\\';\n> >> +\t\t\t\t });\n> >> +\tif (iter != ipaName.end()) {\n> >> +\t\tLOG(IPAModule, Error)\n> >> +\t\t\t<< \"Invalid IPA module name '\" << ipaName << \"'\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> \t/* Load the signature. Failures are not fatal. */\n> >> \tFile sign{ libPath_ + \".sign\" };\n> >> \tif (!sign.open(File::ReadOnly)) {\n> >> diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp\n> >> index d22c302fa726..e3aee190b410 100644\n> >> --- a/test/ipa/ipa_module_test.cpp\n> >> +++ b/test/ipa/ipa_module_test.cpp\n> >> @@ -58,7 +58,7 @@ protected:\n> >> \t\t\tIPA_MODULE_API_VERSION,\n> >> \t\t\t0,\n> >> \t\t\t\"PipelineHandlerVimc\",\n> >> -\t\t\t\"Dummy IPA for Vimc\",\n> >> +\t\t\t\"vimc\",\n> >> \t\t};\n> >>\n> >> \t\tcount += runTest(\"src/ipa/vimc/ipa_vimc.so\", testInfo);","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 5A255603F9\n\tfor ;\n\tMon, 27 Apr 2020 12:36:00 +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 C351C72C;\n\tMon, 27 Apr 2020 12:35:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"e+LpOfEY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587983760;\n\tbh=8Ffh1a1JjaaJw+V8meTozaBqxOwANuYg06wrM9B2vZQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e+LpOfEYHWHsTPsXJIMfrggWEMiFMwSbuf5BVIozCX61mfVCZkGVb5pIN7fZWO/gM\n\tOcX7IOsS2ZwT7E+J3Lrr9XjN+4LspFwRz2j37Om80mO4yp3Bi0CFtvm9wMMU8LLXdn\n\taeZyX4ZXAG+Zrr3drJedG3j8uojyyAIGoAczPSbI=","Date":"Mon, 27 Apr 2020 13:35:44 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200427103544.GA5849@pendragon.ideasonboard.com>","References":"<20200427031713.14013-1-laurent.pinchart@ideasonboard.com>\n\t<20200427031713.14013-2-laurent.pinchart@ideasonboard.com>\n\t<20200427074605.ikbkaivl4yp6lfsn@uno.localdomain>\n\t<48eabf44-b077-71b2-008c-ca62af786098@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<48eabf44-b077-71b2-008c-ca62af786098@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after\n\ttheir source directory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 27 Apr 2020 10:36:00 -0000"}}]