[libcamera-devel,01/11] ipa: Name IPA modules after their source directory
diff mbox series

Message ID 20200427031713.14013-2-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Add support for IPA configuration
Related show

Commit Message

Laurent Pinchart April 27, 2020, 3:17 a.m. UTC 
The IPAModuleInfo::name field is currently a free-formed string that has
little use. Tighten its usage rules to make it suitable for building
file system paths to IPA-specific resources by matching the directory
name of the IPA module.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/rkisp1/rkisp1.cpp    |  2 +-
 src/ipa/vimc/vimc.cpp        |  2 +-
 src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++
 test/ipa/ipa_module_test.cpp |  2 +-
 4 files changed, 23 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi April 27, 2020, 7:46 a.m. UTC | #1 
Hi Laurent,

On Mon, Apr 27, 2020 at 06:17:03AM +0300, Laurent Pinchart wrote:
> The IPAModuleInfo::name field is currently a free-formed string that has
> little use. Tighten its usage rules to make it suitable for building
> file system paths to IPA-specific resources by matching the directory
> name of the IPA module.
>

I wonder, should we use 'name' for this purpose, or should we get the
file name directly, strip the .cpp extension out and add a field for
that purpose ? I feel maintaining the name in synch with the file name
is a (relatively small) burden we could avoid.

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/rkisp1/rkisp1.cpp    |  2 +-
>  src/ipa/vimc/vimc.cpp        |  2 +-
>  src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++
>  test/ipa/ipa_module_test.cpp |  2 +-
>  4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index acbbe58c7a2e..7f0ffb0a791f 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -274,7 +274,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	IPA_MODULE_API_VERSION,
>  	1,
>  	"PipelineHandlerRkISP1",
> -	"RkISP1 IPA",
> +	"rkisp1",
>  };
>
>  struct ipa_context *ipaCreate()
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index d2267e11737f..cfdbd6f99155 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -126,7 +126,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	IPA_MODULE_API_VERSION,
>  	0,
>  	"PipelineHandlerVimc",
> -	"Dummy IPA for Vimc",
> +	"vimc",
>  };
>
>  struct ipa_context *ipaCreate()
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 958ede74688e..52823e88a508 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -9,6 +9,7 @@
>
>  #include <algorithm>
>  #include <array>
> +#include <ctype.h>
>  #include <dlfcn.h>
>  #include <elf.h>
>  #include <errno.h>
> @@ -216,6 +217,11 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
>   * \var IPAModuleInfo::name
>   * \brief The name of the IPA module
>   *
> + * The name may be used to build file system paths to IPA-specific resources.
> + * It shall only contain printable characters, and may not contain '/', '*',
> + * '?' or '\'. For IPA modules included in libcamera, it shall match the
> + * directory of the IPA module in the source tree.
> + *
>   * \todo Allow user to choose to isolate open source IPAs
>   */
>
> @@ -287,6 +293,20 @@ int IPAModule::loadIPAModuleInfo()
>  		return -EINVAL;
>  	}
>
> +	/* Validate the IPA module name. */
> +	std::string ipaName = info_.name;
> +	auto iter = std::find_if_not(ipaName.begin(), ipaName.end(),
> +				     [](unsigned char c) -> bool {
> +					     return isprint(c) && c != '/' &&
> +						    c != '?' && c != '*' &&
> +						    c != '\\';
> +				     });
> +	if (iter != ipaName.end()) {
> +		LOG(IPAModule, Error)
> +			<< "Invalid IPA module name '" << ipaName << "'";
> +		return -EINVAL;
> +	}
> +
>  	/* Load the signature. Failures are not fatal. */
>  	File sign{ libPath_ + ".sign" };
>  	if (!sign.open(File::ReadOnly)) {
> diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp
> index d22c302fa726..e3aee190b410 100644
> --- a/test/ipa/ipa_module_test.cpp
> +++ b/test/ipa/ipa_module_test.cpp
> @@ -58,7 +58,7 @@ protected:
>  			IPA_MODULE_API_VERSION,
>  			0,
>  			"PipelineHandlerVimc",
> -			"Dummy IPA for Vimc",
> +			"vimc",
>  		};
>
>  		count += runTest("src/ipa/vimc/ipa_vimc.so", testInfo);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham April 27, 2020, 10:25 a.m. UTC | #2 
Hi Laurent,

On 27/04/2020 08:46, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Mon, Apr 27, 2020 at 06:17:03AM +0300, Laurent Pinchart wrote:
>> The IPAModuleInfo::name field is currently a free-formed string that has
>> little use. Tighten its usage rules to make it suitable for building
>> file system paths to IPA-specific resources by matching the directory
>> name of the IPA module.
>>
> 
> I wonder, should we use 'name' for this purpose, or should we get the
> file name directly, strip the .cpp extension out and add a field for
> that purpose ? I feel maintaining the name in synch with the file name
> is a (relatively small) burden we could avoid.

Hrm ... some interesting ideas on a google search to get the stripped
filename at compile time:
https://gist.github.com/huangml/3221950a96154180467a ... but I think in
this instance it requires the directory name - not the file name.
(though equally, perhaps they should be the same too).

I also wonder if perhaps to get the actual desired effect required
(location of the IPA tree) we couldn't use a method similar to the
buildPath() ... but maybe that's more work for little current gain, and
making that path buildable is a simple solution for the time being.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> 
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  src/ipa/rkisp1/rkisp1.cpp    |  2 +-
>>  src/ipa/vimc/vimc.cpp        |  2 +-
>>  src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++
>>  test/ipa/ipa_module_test.cpp |  2 +-
>>  4 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index acbbe58c7a2e..7f0ffb0a791f 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -274,7 +274,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
>>  	IPA_MODULE_API_VERSION,
>>  	1,
>>  	"PipelineHandlerRkISP1",
>> -	"RkISP1 IPA",
>> +	"rkisp1",
>>  };
>>
>>  struct ipa_context *ipaCreate()
>> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
>> index d2267e11737f..cfdbd6f99155 100644
>> --- a/src/ipa/vimc/vimc.cpp
>> +++ b/src/ipa/vimc/vimc.cpp
>> @@ -126,7 +126,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
>>  	IPA_MODULE_API_VERSION,
>>  	0,
>>  	"PipelineHandlerVimc",
>> -	"Dummy IPA for Vimc",
>> +	"vimc",
>>  };
>>
>>  struct ipa_context *ipaCreate()
>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
>> index 958ede74688e..52823e88a508 100644
>> --- a/src/libcamera/ipa_module.cpp
>> +++ b/src/libcamera/ipa_module.cpp
>> @@ -9,6 +9,7 @@
>>
>>  #include <algorithm>
>>  #include <array>
>> +#include <ctype.h>
>>  #include <dlfcn.h>
>>  #include <elf.h>
>>  #include <errno.h>
>> @@ -216,6 +217,11 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
>>   * \var IPAModuleInfo::name
>>   * \brief The name of the IPA module
>>   *
>> + * The name may be used to build file system paths to IPA-specific resources.
>> + * It shall only contain printable characters, and may not contain '/', '*',
>> + * '?' or '\'. For IPA modules included in libcamera, it shall match the
>> + * directory of the IPA module in the source tree.
>> + *
>>   * \todo Allow user to choose to isolate open source IPAs
>>   */
>>
>> @@ -287,6 +293,20 @@ int IPAModule::loadIPAModuleInfo()
>>  		return -EINVAL;
>>  	}
>>
>> +	/* Validate the IPA module name. */
>> +	std::string ipaName = info_.name;
>> +	auto iter = std::find_if_not(ipaName.begin(), ipaName.end(),
>> +				     [](unsigned char c) -> bool {
>> +					     return isprint(c) && c != '/' &&
>> +						    c != '?' && c != '*' &&
>> +						    c != '\\';
>> +				     });
>> +	if (iter != ipaName.end()) {
>> +		LOG(IPAModule, Error)
>> +			<< "Invalid IPA module name '" << ipaName << "'";
>> +		return -EINVAL;
>> +	}
>> +
>>  	/* Load the signature. Failures are not fatal. */
>>  	File sign{ libPath_ + ".sign" };
>>  	if (!sign.open(File::ReadOnly)) {
>> diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp
>> index d22c302fa726..e3aee190b410 100644
>> --- a/test/ipa/ipa_module_test.cpp
>> +++ b/test/ipa/ipa_module_test.cpp
>> @@ -58,7 +58,7 @@ protected:
>>  			IPA_MODULE_API_VERSION,
>>  			0,
>>  			"PipelineHandlerVimc",
>> -			"Dummy IPA for Vimc",
>> +			"vimc",
>>  		};
>>
>>  		count += runTest("src/ipa/vimc/ipa_vimc.so", testInfo);
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart April 27, 2020, 10:35 a.m. UTC | #3 
Hello Jacopo and Kieran,

On Mon, Apr 27, 2020 at 11:25:49AM +0100, Kieran Bingham wrote:
> On 27/04/2020 08:46, Jacopo Mondi wrote:
> > On Mon, Apr 27, 2020 at 06:17:03AM +0300, Laurent Pinchart wrote:
> >> The IPAModuleInfo::name field is currently a free-formed string that has
> >> little use. Tighten its usage rules to make it suitable for building
> >> file system paths to IPA-specific resources by matching the directory
> >> name of the IPA module.
> > 
> > I wonder, should we use 'name' for this purpose, or should we get the
> > file name directly, strip the .cpp extension out and add a field for
> > that purpose ? I feel maintaining the name in synch with the file name
> > is a (relatively small) burden we could avoid.

I'm open to ideas to automate this, but I'd rather do so on top of this
series, as it's a minor improvement in my opinion.

> Hrm ... some interesting ideas on a google search to get the stripped
> filename at compile time:
> https://gist.github.com/huangml/3221950a96154180467a ... but I think in
> this instance it requires the directory name - not the file name.
> (though equally, perhaps they should be the same too).

It shouldn't be too difficult to retrieve the directory name, but I'd
rather try to improve the tooling in meson first, possibly using a
generator, to help with that.

> I also wonder if perhaps to get the actual desired effect required
> (location of the IPA tree) we couldn't use a method similar to the
> buildPath() ... but maybe that's more work for little current gain, and
> making that path buildable is a simple solution for the time being.

Don't forget that the name is loaded from the IPAModuleInfo structure
without executing any code from the IPA module, so it has to be a static
string.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >>  src/ipa/rkisp1/rkisp1.cpp    |  2 +-
> >>  src/ipa/vimc/vimc.cpp        |  2 +-
> >>  src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++
> >>  test/ipa/ipa_module_test.cpp |  2 +-
> >>  4 files changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index acbbe58c7a2e..7f0ffb0a791f 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -274,7 +274,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
> >>  	IPA_MODULE_API_VERSION,
> >>  	1,
> >>  	"PipelineHandlerRkISP1",
> >> -	"RkISP1 IPA",
> >> +	"rkisp1",
> >>  };
> >>
> >>  struct ipa_context *ipaCreate()
> >> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> >> index d2267e11737f..cfdbd6f99155 100644
> >> --- a/src/ipa/vimc/vimc.cpp
> >> +++ b/src/ipa/vimc/vimc.cpp
> >> @@ -126,7 +126,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
> >>  	IPA_MODULE_API_VERSION,
> >>  	0,
> >>  	"PipelineHandlerVimc",
> >> -	"Dummy IPA for Vimc",
> >> +	"vimc",
> >>  };
> >>
> >>  struct ipa_context *ipaCreate()
> >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> >> index 958ede74688e..52823e88a508 100644
> >> --- a/src/libcamera/ipa_module.cpp
> >> +++ b/src/libcamera/ipa_module.cpp
> >> @@ -9,6 +9,7 @@
> >>
> >>  #include <algorithm>
> >>  #include <array>
> >> +#include <ctype.h>
> >>  #include <dlfcn.h>
> >>  #include <elf.h>
> >>  #include <errno.h>
> >> @@ -216,6 +217,11 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
> >>   * \var IPAModuleInfo::name
> >>   * \brief The name of the IPA module
> >>   *
> >> + * The name may be used to build file system paths to IPA-specific resources.
> >> + * It shall only contain printable characters, and may not contain '/', '*',
> >> + * '?' or '\'. For IPA modules included in libcamera, it shall match the
> >> + * directory of the IPA module in the source tree.
> >> + *
> >>   * \todo Allow user to choose to isolate open source IPAs
> >>   */
> >>
> >> @@ -287,6 +293,20 @@ int IPAModule::loadIPAModuleInfo()
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	/* Validate the IPA module name. */
> >> +	std::string ipaName = info_.name;
> >> +	auto iter = std::find_if_not(ipaName.begin(), ipaName.end(),
> >> +				     [](unsigned char c) -> bool {
> >> +					     return isprint(c) && c != '/' &&
> >> +						    c != '?' && c != '*' &&
> >> +						    c != '\\';
> >> +				     });
> >> +	if (iter != ipaName.end()) {
> >> +		LOG(IPAModule, Error)
> >> +			<< "Invalid IPA module name '" << ipaName << "'";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >>  	/* Load the signature. Failures are not fatal. */
> >>  	File sign{ libPath_ + ".sign" };
> >>  	if (!sign.open(File::ReadOnly)) {
> >> diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp
> >> index d22c302fa726..e3aee190b410 100644
> >> --- a/test/ipa/ipa_module_test.cpp
> >> +++ b/test/ipa/ipa_module_test.cpp
> >> @@ -58,7 +58,7 @@ protected:
> >>  			IPA_MODULE_API_VERSION,
> >>  			0,
> >>  			"PipelineHandlerVimc",
> >> -			"Dummy IPA for Vimc",
> >> +			"vimc",
> >>  		};
> >>
> >>  		count += runTest("src/ipa/vimc/ipa_vimc.so", testInfo);

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index acbbe58c7a2e..7f0ffb0a791f 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -274,7 +274,7 @@  const struct IPAModuleInfo ipaModuleInfo = {
 	IPA_MODULE_API_VERSION,
 	1,
 	"PipelineHandlerRkISP1",
-	"RkISP1 IPA",
+	"rkisp1",
 };
 
 struct ipa_context *ipaCreate()
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index d2267e11737f..cfdbd6f99155 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -126,7 +126,7 @@  const struct IPAModuleInfo ipaModuleInfo = {
 	IPA_MODULE_API_VERSION,
 	0,
 	"PipelineHandlerVimc",
-	"Dummy IPA for Vimc",
+	"vimc",
 };
 
 struct ipa_context *ipaCreate()
diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
index 958ede74688e..52823e88a508 100644
--- a/src/libcamera/ipa_module.cpp
+++ b/src/libcamera/ipa_module.cpp
@@ -9,6 +9,7 @@ 
 
 #include <algorithm>
 #include <array>
+#include <ctype.h>
 #include <dlfcn.h>
 #include <elf.h>
 #include <errno.h>
@@ -216,6 +217,11 @@  Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
  * \var IPAModuleInfo::name
  * \brief The name of the IPA module
  *
+ * The name may be used to build file system paths to IPA-specific resources.
+ * It shall only contain printable characters, and may not contain '/', '*',
+ * '?' or '\'. For IPA modules included in libcamera, it shall match the
+ * directory of the IPA module in the source tree.
+ *
  * \todo Allow user to choose to isolate open source IPAs
  */
 
@@ -287,6 +293,20 @@  int IPAModule::loadIPAModuleInfo()
 		return -EINVAL;
 	}
 
+	/* Validate the IPA module name. */
+	std::string ipaName = info_.name;
+	auto iter = std::find_if_not(ipaName.begin(), ipaName.end(),
+				     [](unsigned char c) -> bool {
+					     return isprint(c) && c != '/' &&
+						    c != '?' && c != '*' &&
+						    c != '\\';
+				     });
+	if (iter != ipaName.end()) {
+		LOG(IPAModule, Error)
+			<< "Invalid IPA module name '" << ipaName << "'";
+		return -EINVAL;
+	}
+
 	/* Load the signature. Failures are not fatal. */
 	File sign{ libPath_ + ".sign" };
 	if (!sign.open(File::ReadOnly)) {
diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp
index d22c302fa726..e3aee190b410 100644
--- a/test/ipa/ipa_module_test.cpp
+++ b/test/ipa/ipa_module_test.cpp
@@ -58,7 +58,7 @@  protected:
 			IPA_MODULE_API_VERSION,
 			0,
 			"PipelineHandlerVimc",
-			"Dummy IPA for Vimc",
+			"vimc",
 		};
 
 		count += runTest("src/ipa/vimc/ipa_vimc.so", testInfo);