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

[Laurent Pinchart]

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 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