[libcamera-devel] [PATCH 3/8] libcamera: ipa_module_info: update struct to allow IPA matching

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, May 27, 2019 at 06:35:35PM -0400, Paul Elder wrote:
> We need a way to match pipelines with IPA modules, so add fields in
> IPAModuleInfo to hold the IPA API version number, the pipeline name, and
> the pipeline major and minor version.
> 
> Also update IPA module tests and Doxygen accordingly. Doxygen needs to
> be updated to accomodate __attribute__((packed)).
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  Documentation/Doxyfile.in               |  7 ++--
>  include/libcamera/ipa/ipa_module_info.h | 11 ++++--
>  src/libcamera/ipa_module.cpp            | 48 +++++++++++++++++++++----
>  test/ipa/ipa_test.cpp                   | 45 ++++++++++++++++++-----
>  test/ipa/shared_test.cpp                |  4 ++-
>  5 files changed, 94 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 67eaded..ac70efb 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -2016,7 +2016,7 @@ ENABLE_PREPROCESSING   = YES
>  # The default value is: NO.
>  # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
>  
> -MACRO_EXPANSION        = NO
> +MACRO_EXPANSION        = YES
>  
>  # If the EXPAND_ONLY_PREDEF and MACRO_EXPANSION tags are both set to YES then
>  # the macro expansion is limited to the macros specified with the PREDEFINED and
> @@ -2024,7 +2024,7 @@ MACRO_EXPANSION        = NO
>  # The default value is: NO.
>  # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
>  
> -EXPAND_ONLY_PREDEF     = NO
> +EXPAND_ONLY_PREDEF     = YES
>  
>  # If the SEARCH_INCLUDES tag is set to YES, the include files in the
>  # INCLUDE_PATH will be searched if a #include is found.
> @@ -2057,7 +2057,8 @@ INCLUDE_FILE_PATTERNS  = *.h
>  # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
>  
>  PREDEFINED             = __DOXYGEN__ \
> -                         __cplusplus
> +                         __cplusplus \
> +                         __attribute__(x)=
>  
>  # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
>  # tag can be used to specify a list of macro names that should be expanded. The
> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h
> index 4e0d668..84492ed 100644
> --- a/include/libcamera/ipa/ipa_module_info.h
> +++ b/include/libcamera/ipa/ipa_module_info.h
> @@ -7,14 +7,21 @@
>  #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__
>  #define __LIBCAMERA_IPA_MODULE_INFO_H__
>  
> +#include <stdint.h>
> +
> +#define IPA_MODULE_API_VERSION 1
> +
>  #ifdef __cplusplus
>  namespace libcamera {
>  #endif
>  
>  struct IPAModuleInfo {
> +	int moduleAPIVersion;
> +	int pipelineVersionMajor;
> +	int pipelineVersionMinor;

Please use integer types from stdint.h. These should be uint32_t.

I think you can combine the minor and major in a single version field,
with a macro to create it from major and minor numbers.

> +	char pipelineName[256];
>  	char name[256];
> -	unsigned int version;
> -};
> +} __attribute__((packed));
>  
>  #ifdef __cplusplus
>  extern "C" {
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 86cbe71..6e68934 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -169,6 +169,17 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>  
>  } /* namespace */
>  
> +/**
> + * \def IPA_MODULE_API_VERSION
> + * \brief The IPA API version
> + *
> + * This version number specifies the version for the layout of
> + * struct IPAModuleInfo, and the API between pipeline handlers and the IPA.

The module API version is just the former, not the latter, which are
reported through the pipelineVersion.

> + * The IPA module should use this macro to define its moduleAPIVersion field.

s/should/shall/
s/define/set/

> + *
> + * \sa libcamera::IPAModuleInfo::moduleAPIVersion
> + */
> +
>  /**
>   * \struct IPAModuleInfo
>   * \brief Information of an IPA module
> @@ -176,14 +187,39 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>   * This structure contains the information of an IPA module. It is loaded,
>   * read, and validated before anything else is loaded from the shared object.
>   *
> - * \var IPAModuleInfo::name
> - * \brief The name of the IPA module
> + * \var IPAModuleInfo::moduleAPIVersion
> + * \brief The IPA API version that the IPA module was made with

s/was made with/implements/ ?

> + *
> + * This version number specifies the version for the layout of
> + * struct IPAModuleInfo, and the API between pipeline handlers and the IPA.

s/, .*/./

> + * The IPA module shall report here the version that it was built for,
> + * using the macro IPA_MODULE_API_VERSION.
>   *
> - * \var IPAModuleInfo::version
> - * \brief The version of the IPA module
> + * \sa IPA_MODULE_API_VERSION
>   *
> - * \todo abi compatability version
> - * \todo pipeline compatability matcher
> + * \var IPAModuleInfo::pipelineVersionMajor
> + * \brief The pipeline handler version (major) that the IPA module is for
> + * \todo actually match the pipeline handler against this
> + *
> + * The major version of the module and pipeline handler must be equal in
> + * order for the pipeline handler and the module to be matched.
> + *
> + * \var IPAModuleInfo::pipelineVersionMinor
> + * \brief The pipeline handler version (minor) that the IPA module is for
> + *
> + * The minor version of the module must be equal to or greater than
> + * the pipeline handler minor version in order for the pipeline handler
> + * and the module to be matched.
> + *
> + * \var IPAModuleInfo::pipelineName
> + * \brief The name of the pipeline handler that the IPA module is for
> + *
> + * This name is used to match a pipeline handler with the module.
> + * The name of the pipeline handler is declared in the pipeline handler
> + * with the macro REGISTER_PIPELINE_HANDLER.

I would drop the second sentence, as REGISTER_PIPELINE_HANDLER is an
internal API, and should thus not be referenced from external API files.

> + *
> + * \var IPAModuleInfo::name
> + * \brief The name of the IPA module
>   */
>  
>  /**
> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp
> index 2dbc702..f50880e 100644
> --- a/test/ipa/ipa_test.cpp
> +++ b/test/ipa/ipa_test.cpp
> @@ -33,20 +33,45 @@ protected:
>  
>  		const struct IPAModuleInfo &info = ll->info();
>  
> -		if (strcmp(info.name, testInfo.name)) {
> -			cerr << "test IPA module has incorrect name" << endl;
> -			cerr << "expected \"" << testInfo.name << "\", got \""
> -			     << info.name << "\"" << endl;
> +		stringstream errString;
> +
> +		if (info.moduleAPIVersion != testInfo.moduleAPIVersion) {
> +			errString << "incorrect name: expected \""

"name" ? :-)

You can simplify all this with

		if (info.moduleAPIVersion != testInfo.moduleAPIVersion ||
		    info.version != testInfo.version ||
		    ...) {
		    	cerr << "IPA module information mismatch: expected "
			     << "moduleAPIVersion=" << testInfo.moduleApiVersion
			     << ...
			     << ", got "
			     << ...
		}

What do you think ?


> +				  << testInfo.moduleAPIVersion << "\", got \""
> +				  << info.moduleAPIVersion << "\"" << endl;
>  			ret = -1;
>  		}
>  
> -		if (info.version != testInfo.version) {
> -			cerr << "test IPA module has incorrect version" << endl;
> -			cerr << "expected \"" << testInfo.version << "\", got \""
> -			     << info.version << "\"" << endl;
> +		if (info.pipelineVersionMajor != testInfo.pipelineVersionMajor) {
> +			errString << "incorrect name: expected \""
> +				  << testInfo.pipelineVersionMajor << "\", got \""
> +				  << info.pipelineVersionMajor << "\"" << endl;
>  			ret = -1;
>  		}
>  
> +		if (info.pipelineVersionMinor != testInfo.pipelineVersionMinor) {
> +			errString << "incorrect name: expected \""
> +				  << testInfo.pipelineVersionMinor << "\", got \""
> +				  << info.pipelineVersionMinor << "\"" << endl;
> +			ret = -1;
> +		}
> +
> +		if (strcmp(info.pipelineName, testInfo.pipelineName)) {
> +			errString << "incorrect name: expected \""
> +				  << testInfo.pipelineName << "\", got \""
> +				  << info.pipelineName << "\"" << endl;
> +			ret = -1;
> +		}
> +
> +		if (strcmp(info.name, testInfo.name)) {
> +			errString << "incorrect name: expected \""
> +				  << testInfo.name << "\", got \""
> +				  << info.name << "\"" << endl;
> +			ret = -1;
> +		}
> +
> +		cerr << errString.str();
> +
>  		delete ll;
>  		return ret;
>  	}
> @@ -56,8 +81,10 @@ protected:
>  		int count = 0;
>  
>  		const struct IPAModuleInfo testInfo = {
> +			1,

Should you use IPA_MODULE_API_VERSION as recommended by the
documentation ? Same for the shared_test.cpp file below.

> +			0, 9001,
> +			"bleep",
>  			"It's over nine thousand!",
> -			9001,
>  		};
>  
>  		count += runTest("test/ipa/ipa-dummy-cpp.so", testInfo);
> diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp
> index 4e5c976..d932a9b 100644
> --- a/test/ipa/shared_test.cpp
> +++ b/test/ipa/shared_test.cpp
> @@ -4,8 +4,10 @@ namespace libcamera {
>  
>  extern "C" {
>  const struct libcamera::IPAModuleInfo ipaModuleInfo = {
> +	1,
> +	0, 9001,

I would go for one field per line. It would be really great if we could
use named initialisers :-S

> +	"bleep",
>  	"It's over nine thousand!",

Let's give those fields more plausible values :-)

> -	9001,
>  };
>  };
>  

-- 
Regards,

Laurent Pinchart