[libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after their source directory

[Kieran Bingham]

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 at ideasonboard.com>



> 
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran