| Message ID | 20221011105859.457567-9-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Tue, Oct 11, 2022 at 07:58:58PM +0900, Paul Elder via libcamera-devel wrote: > Currently, enums that are passed between pipeline handlers and their IPA > must be defined in a mojom file. However, there is a use case for > enum/flags to be defined in a C++ header, such that the enum can be used > in a component other than the pipeline handler and its IPA. > > To support this, add support for the skipHeader attribute for enums. > Like structs, it is only allowed in core.mojom. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > New in v5 > --- > include/libcamera/ipa/core.mojom | 4 +++- > include/libcamera/ipa/ipa_interface.h | 4 ++-- > .../generators/libcamera_templates/core_ipa_interface.h.tmpl | 2 +- > utils/ipc/generators/mojom_libcamera_generator.py | 2 +- > 4 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom > index ef28ff2d..1ff674b0 100644 > --- a/include/libcamera/ipa/core.mojom > +++ b/include/libcamera/ipa/core.mojom > @@ -14,7 +14,7 @@ module libcamera; > * - structs > * > * Attributes: > - * - skipHeader - structs only, and only in core.mojom > + * - skipHeader - allowed only for structs and enums in core.mojom > * - Do not generate a C++ definition for the structure s/structure/structure or enum/ It seems so simple now that it's implemented :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > * - Any type used in a mojom interface definition must have a corresponding > * definition in a mojom file for the code generator to accept it, except > @@ -52,6 +52,8 @@ module libcamera; > * then the type definition in the core.mojom file should have the > * [skipHeader] attribute only > * - A (de)serializer will be generated for the type > + * - enums that are defined in a libcamera C++ header also fall in this > + * category > * - If a type definition has [skipHeader], then the header where the type is > * defined must be included in ipa_interface.h > * - Types that are solely used as array/map members do not require a mojom > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h > index 8afcfe21..8884f0ed 100644 > --- a/include/libcamera/ipa/ipa_interface.h > +++ b/include/libcamera/ipa/ipa_interface.h > @@ -23,8 +23,8 @@ > namespace libcamera { > > /* > - * Structs that are defined in core.mojom and have the skipHeader tag must be > - * #included here. > + * Structs and enums that are defined in core.mojom that have the skipHeader > + * tag must be #included here. > */ > > class IPAInterface > diff --git a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > index a565b59a..c60b99b8 100644 > --- a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > +++ b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > @@ -26,7 +26,7 @@ namespace libcamera { > static const {{const.kind|name}} {{const.mojom_name}} = {{const.value}}; > {% endfor %} > > -{% for enum in enums %} > +{% for enum in enums_gen_header %} > {{funcs.define_enum(enum)}} > {% endfor %} > > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py > index 6c176aba..64987ccd 100644 > --- a/utils/ipc/generators/mojom_libcamera_generator.py > +++ b/utils/ipc/generators/mojom_libcamera_generator.py > @@ -483,7 +483,7 @@ class Generator(generator.Generator): > def _GetJinjaExportsForCore(self): > return { > 'consts': self.module.constants, > - 'enums': self.module.enums, > + 'enums_gen_header': [x for x in self.module.enums if x.attributes is None or 'skipHeader' not in x.attributes], > 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0, > 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0, > 'structs_gen_header': [x for x in self.module.structs if x.attributes is None or 'skipHeader' not in x.attributes],
Hi Paul On Tue, Oct 11, 2022 at 07:58:58PM +0900, Paul Elder via libcamera-devel wrote: > Currently, enums that are passed between pipeline handlers and their IPA > must be defined in a mojom file. However, there is a use case for > enum/flags to be defined in a C++ header, such that the enum can be used > in a component other than the pipeline handler and its IPA. > > To support this, add support for the skipHeader attribute for enums. > Like structs, it is only allowed in core.mojom. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > New in v5 > --- > include/libcamera/ipa/core.mojom | 4 +++- > include/libcamera/ipa/ipa_interface.h | 4 ++-- > .../generators/libcamera_templates/core_ipa_interface.h.tmpl | 2 +- > utils/ipc/generators/mojom_libcamera_generator.py | 2 +- > 4 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom > index ef28ff2d..1ff674b0 100644 > --- a/include/libcamera/ipa/core.mojom > +++ b/include/libcamera/ipa/core.mojom > @@ -14,7 +14,7 @@ module libcamera; > * - structs > * > * Attributes: > - * - skipHeader - structs only, and only in core.mojom > + * - skipHeader - allowed only for structs and enums in core.mojom > * - Do not generate a C++ definition for the structure s/structure/type ? > * - Any type used in a mojom interface definition must have a corresponding > * definition in a mojom file for the code generator to accept it, except I guess we don't want [skipSerdes] for enums ? Or do we ? > @@ -52,6 +52,8 @@ module libcamera; > * then the type definition in the core.mojom file should have the > * [skipHeader] attribute only > * - A (de)serializer will be generated for the type > + * - enums that are defined in a libcamera C++ header also fall in this > + * category Is this necessary, as we already claimed the attribute applies to struct and enums ? > * - If a type definition has [skipHeader], then the header where the type is > * defined must be included in ipa_interface.h > * - Types that are solely used as array/map members do not require a mojom > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h > index 8afcfe21..8884f0ed 100644 > --- a/include/libcamera/ipa/ipa_interface.h > +++ b/include/libcamera/ipa/ipa_interface.h > @@ -23,8 +23,8 @@ > namespace libcamera { > > /* > - * Structs that are defined in core.mojom and have the skipHeader tag must be > - * #included here. > + * Structs and enums that are defined in core.mojom that have the skipHeader > + * tag must be #included here. I would take the occasion to re-phrase this: * The header file where symbols used in core.mojom are defined * must be #included here If my understanding is correct > */ > > class IPAInterface > diff --git a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > index a565b59a..c60b99b8 100644 > --- a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > +++ b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > @@ -26,7 +26,7 @@ namespace libcamera { > static const {{const.kind|name}} {{const.mojom_name}} = {{const.value}}; > {% endfor %} > > -{% for enum in enums %} > +{% for enum in enums_gen_header %} > {{funcs.define_enum(enum)}} > {% endfor %} > > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py > index 6c176aba..64987ccd 100644 > --- a/utils/ipc/generators/mojom_libcamera_generator.py > +++ b/utils/ipc/generators/mojom_libcamera_generator.py > @@ -483,7 +483,7 @@ class Generator(generator.Generator): > def _GetJinjaExportsForCore(self): > return { > 'consts': self.module.constants, > - 'enums': self.module.enums, > + 'enums_gen_header': [x for x in self.module.enums if x.attributes is None or 'skipHeader' not in x.attributes], > 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0, > 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0, > 'structs_gen_header': [x for x in self.module.structs if x.attributes is None or 'skipHeader' not in x.attributes], > -- > 2.30.2 >
On Wed, Oct 12, 2022 at 03:55:15PM +0200, Jacopo Mondi wrote: > Hi Paul > > On Tue, Oct 11, 2022 at 07:58:58PM +0900, Paul Elder via libcamera-devel wrote: > > Currently, enums that are passed between pipeline handlers and their IPA > > must be defined in a mojom file. However, there is a use case for > > enum/flags to be defined in a C++ header, such that the enum can be used > > in a component other than the pipeline handler and its IPA. > > > > To support this, add support for the skipHeader attribute for enums. > > Like structs, it is only allowed in core.mojom. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > New in v5 > > --- > > include/libcamera/ipa/core.mojom | 4 +++- > > include/libcamera/ipa/ipa_interface.h | 4 ++-- > > .../generators/libcamera_templates/core_ipa_interface.h.tmpl | 2 +- > > utils/ipc/generators/mojom_libcamera_generator.py | 2 +- > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom > > index ef28ff2d..1ff674b0 100644 > > --- a/include/libcamera/ipa/core.mojom > > +++ b/include/libcamera/ipa/core.mojom > > @@ -14,7 +14,7 @@ module libcamera; > > * - structs > > * > > * Attributes: > > - * - skipHeader - structs only, and only in core.mojom > > + * - skipHeader - allowed only for structs and enums in core.mojom > > * - Do not generate a C++ definition for the structure > > s/structure/type ? > > > * - Any type used in a mojom interface definition must have a corresponding > > * definition in a mojom file for the code generator to accept it, except > > I guess we don't want [skipSerdes] for enums ? Or do we ? enums, like PODs, already get skipSerdes-ed. > > > @@ -52,6 +52,8 @@ module libcamera; > > * then the type definition in the core.mojom file should have the > > * [skipHeader] attribute only > > * - A (de)serializer will be generated for the type > > + * - enums that are defined in a libcamera C++ header also fall in this > > + * category > > Is this necessary, as we already claimed the attribute applies to > struct and enums ? > > > > * - If a type definition has [skipHeader], then the header where the type is > > * defined must be included in ipa_interface.h > > * - Types that are solely used as array/map members do not require a mojom > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h > > index 8afcfe21..8884f0ed 100644 > > --- a/include/libcamera/ipa/ipa_interface.h > > +++ b/include/libcamera/ipa/ipa_interface.h > > @@ -23,8 +23,8 @@ > > namespace libcamera { > > > > /* > > - * Structs that are defined in core.mojom and have the skipHeader tag must be > > - * #included here. > > + * Structs and enums that are defined in core.mojom that have the skipHeader > > + * tag must be #included here. > > I would take the occasion to re-phrase this: > > * The header file where symbols used in core.mojom are defined > * must be #included here > > If my understanding is correct It's not wrong, I just think it's less precise. Paul > > > */ > > > > class IPAInterface > > diff --git a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > > index a565b59a..c60b99b8 100644 > > --- a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > > +++ b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > > @@ -26,7 +26,7 @@ namespace libcamera { > > static const {{const.kind|name}} {{const.mojom_name}} = {{const.value}}; > > {% endfor %} > > > > -{% for enum in enums %} > > +{% for enum in enums_gen_header %} > > {{funcs.define_enum(enum)}} > > {% endfor %} > > > > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py > > index 6c176aba..64987ccd 100644 > > --- a/utils/ipc/generators/mojom_libcamera_generator.py > > +++ b/utils/ipc/generators/mojom_libcamera_generator.py > > @@ -483,7 +483,7 @@ class Generator(generator.Generator): > > def _GetJinjaExportsForCore(self): > > return { > > 'consts': self.module.constants, > > - 'enums': self.module.enums, > > + 'enums_gen_header': [x for x in self.module.enums if x.attributes is None or 'skipHeader' not in x.attributes], > > 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0, > > 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0, > > 'structs_gen_header': [x for x in self.module.structs if x.attributes is None or 'skipHeader' not in x.attributes], > > -- > > 2.30.2 > >
diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom index ef28ff2d..1ff674b0 100644 --- a/include/libcamera/ipa/core.mojom +++ b/include/libcamera/ipa/core.mojom @@ -14,7 +14,7 @@ module libcamera; * - structs * * Attributes: - * - skipHeader - structs only, and only in core.mojom + * - skipHeader - allowed only for structs and enums in core.mojom * - Do not generate a C++ definition for the structure * - Any type used in a mojom interface definition must have a corresponding * definition in a mojom file for the code generator to accept it, except @@ -52,6 +52,8 @@ module libcamera; * then the type definition in the core.mojom file should have the * [skipHeader] attribute only * - A (de)serializer will be generated for the type + * - enums that are defined in a libcamera C++ header also fall in this + * category * - If a type definition has [skipHeader], then the header where the type is * defined must be included in ipa_interface.h * - Types that are solely used as array/map members do not require a mojom diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h index 8afcfe21..8884f0ed 100644 --- a/include/libcamera/ipa/ipa_interface.h +++ b/include/libcamera/ipa/ipa_interface.h @@ -23,8 +23,8 @@ namespace libcamera { /* - * Structs that are defined in core.mojom and have the skipHeader tag must be - * #included here. + * Structs and enums that are defined in core.mojom that have the skipHeader + * tag must be #included here. */ class IPAInterface diff --git a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl index a565b59a..c60b99b8 100644 --- a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl +++ b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl @@ -26,7 +26,7 @@ namespace libcamera { static const {{const.kind|name}} {{const.mojom_name}} = {{const.value}}; {% endfor %} -{% for enum in enums %} +{% for enum in enums_gen_header %} {{funcs.define_enum(enum)}} {% endfor %} diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py index 6c176aba..64987ccd 100644 --- a/utils/ipc/generators/mojom_libcamera_generator.py +++ b/utils/ipc/generators/mojom_libcamera_generator.py @@ -483,7 +483,7 @@ class Generator(generator.Generator): def _GetJinjaExportsForCore(self): return { 'consts': self.module.constants, - 'enums': self.module.enums, + 'enums_gen_header': [x for x in self.module.enums if x.attributes is None or 'skipHeader' not in x.attributes], 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0, 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0, 'structs_gen_header': [x for x in self.module.structs if x.attributes is None or 'skipHeader' not in x.attributes],
Currently, enums that are passed between pipeline handlers and their IPA must be defined in a mojom file. However, there is a use case for enum/flags to be defined in a C++ header, such that the enum can be used in a component other than the pipeline handler and its IPA. To support this, add support for the skipHeader attribute for enums. Like structs, it is only allowed in core.mojom. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- New in v5 --- include/libcamera/ipa/core.mojom | 4 +++- include/libcamera/ipa/ipa_interface.h | 4 ++-- .../generators/libcamera_templates/core_ipa_interface.h.tmpl | 2 +- utils/ipc/generators/mojom_libcamera_generator.py | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-)