[libcamera-devel,RFC,v2,1/7] libcamera: ipa_module_info: add license field
diff mbox series

Message ID 20190703080007.21376-2-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Add IPA process isolation
Related show

Commit Message

Paul Elder July 3, 2019, 8 a.m. UTC 
Add a field to IPAModuleInfo to contain the license of the module.

Update the dummy IPA and IPA test to conform to the new struct layout

Signed-off-by: Paul Elder <paul.elder@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(+)

Comments

Laurent Pinchart July 3, 2019, 2:04 p.m. UTC | #1 
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@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);
Niklas Söderlund July 3, 2019, 11:36 p.m. UTC | #2 
Hi Paul,

On 2019-07-03 17:04:02 +0300, Laurent Pinchart wrote:
> 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@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.

Will not a list of string SPDX identifiers be just as messy to maintain?  
Libcamera would still need to keep an internal list of either strings or 
numerical ids of licences which are OK to run without isolation.

The advantage I see with numerical ids is that the risk of typos in the
licensing "field" is eliminated, but I'm not so concerned that will be a 
problem as the developer of a out-of-tree IPA would notice the typo 
rather quickly.

I don't feel super strong about this issue but I'm tilting towards using 
SPDX strings in the licence field. And in libcamera start with a limited 
list of licences we think will be common and be open to add more from 
the SPDX list if needed down the road.

Another option is of course to have closed source IPAs set a "closed 
source bit", possibly done automatically when downloading the IPA using 
RFC3514 ;-P

> 
> >   */
> >  
> >  /**
> > 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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];
+
 } __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
  */
 
 /**
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"
 		};
 
 		count += runTest("src/ipa/ipa_dummy.so", testInfo);