[libcamera-devel] [RFC PATCH v2 1/7] libcamera: ipa_module_info: add license field

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jul 03, 2019 at 05:00:01PM +0900, Paul Elder wrote:
> Add a field to IPAModuleInfo to contain the license of the module.

Should you briefly describe here what the license field will be used for
? I haven't read the rest of the series, but I expect that libcamera
will use it to allow running open-source IPAs without process isolation
*if* enabled by the user (any IPA should be possible to run in
isolation, and libcamera would never allow running the closed-source
ones without isolation).

> Update the dummy IPA and IPA test to conform to the new struct layout
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> New patch in v2
> - this replaces the isolate flag that was used in v1
> 
>  include/libcamera/ipa/ipa_module_info.h | 2 ++
>  src/ipa/ipa_dummy.cpp                   | 1 +
>  src/libcamera/ipa_module.cpp            | 3 +++
>  test/ipa/ipa_test.cpp                   | 1 +
>  4 files changed, 7 insertions(+)
> 
> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h
> index 585f753..39dca1a 100644
> --- a/include/libcamera/ipa/ipa_module_info.h
> +++ b/include/libcamera/ipa/ipa_module_info.h
> @@ -18,6 +18,8 @@ struct IPAModuleInfo {
>  	uint32_t pipelineVersion;
>  	char pipelineName[256];
>  	char name[256];
> +	char license[64];
> +

Unneeded blank line.

>  } __attribute__((packed));
>  
>  extern "C" {
> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> index ee7a3a8..2e6ff71 100644
> --- a/src/ipa/ipa_dummy.cpp
> +++ b/src/ipa/ipa_dummy.cpp
> @@ -34,6 +34,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	0,
>  	"PipelineHandlerVimc",
>  	"Dummy IPA for Vimc",
> +	"LGPLv2.1",
>  };
>  
>  IPAInterface *ipaCreate()
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index d82ac69..f786c16 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -215,6 +215,9 @@ elfLoadSymbol(void *map, size_t soSize, const char *symbol)
>   *
>   * \var IPAModuleInfo::name
>   * \brief The name of the IPA module
> + *
> + * \var IPAModuleInfo::license
> + * \brief License of the IPA module

I think this requires more documentation. In particular you should
documentation what license strings are allowed. One option could be to
use the SPDX license strings, available from https://spdx.org/licenses/.
You would then need to explicitly define what string should be used for
proprietary licenses.

The longest license string currently defined in SPDX is 36 for
characters long (BSD-3-Clause-No-Nuclear-License-2014), with the second
contender 32 characters long (BSD-3-Clause-No-Nuclear-Warranty)
(slightly out of topic, but the concept of no nuclear warranty is
interesting :-)). With the use of license expressions
(https://spdx.org/spdx-specification-21-web-version#h.jxpfx0ykyb60) this
could become much longer, but I think 64 is a safe value for now.

Another option would be to use a numerical identifier. We would then
likely restrict that to a few common license types, otherwise it would
become a mess to handle. That would be a simpler mechanism, but not as
powerful.

>   */
>  
>  /**
> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp
> index bbef069..4c51034 100644
> --- a/test/ipa/ipa_test.cpp
> +++ b/test/ipa/ipa_test.cpp
> @@ -59,6 +59,7 @@ protected:
>  			0,
>  			"PipelineHandlerVimc",
>  			"Dummy IPA for Vimc",
> +			"LGPLv2.1"

This (and the above other license above) would thus become
"LGPL-2.1-or-later".

>  		};
>  
>  		count += runTest("src/ipa/ipa_dummy.so", testInfo);

-- 
Regards,

Laurent Pinchart