[libcamera-devel] [PATCH v3 26/38] ipa: Add core.mojom
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 6 02:35:48 CEST 2020
Hi Paul,
On Mon, Oct 05, 2020 at 05:48:42PM +0900, paul.elder at ideasonboard.com wrote:
> On Sun, Oct 04, 2020 at 12:33:04PM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 02, 2020 at 11:31:42PM +0900, Paul Elder wrote:
> > > Add a base mojom file to contain empty mojom definitions of libcamera
> > > objects, as well as documentation for the IPA interfaces that need to be
> > > defined in the mojom files.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v3:
> > > - add doc that structs need to be defined
> > > - add doc to recommend namespacing
> > > - change indentation
> > > - add requirement that base controls *must* be defined in
> > > libcamera::{pipeline_name}::Controls
> > >
> > > New in v2
> > > ---
> > > include/libcamera/ipa/core.mojom | 83 ++++++++++++++++++++++++++
> > > include/libcamera/ipa/meson.build | 18 +++++-
> > > src/libcamera/proxy/meson.build | 2 +-
> > > src/libcamera/proxy/worker/meson.build | 2 +-
> > > 4 files changed, 100 insertions(+), 5 deletions(-)
> > > create mode 100644 include/libcamera/ipa/core.mojom
> > >
> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > > new file mode 100644
> > > index 00000000..81d68bae
> > > --- /dev/null
> > > +++ b/include/libcamera/ipa/core.mojom
> > > @@ -0,0 +1,83 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +
> > > +/*
> > > + * Any libcamera objects that are used by any interfaces must be declared
> > > + * here, and a de/serializer be implemented in IPADataSerializer. In addition,
> > > + * the corresponding header file (or forward-declarations) must be placed in
> > > + * {pipeline_name}.h.
> > > + *
> > > + * For libcamera types, the [hasFd] attribute is needed to notify the compiler
> > > + * that the struct has an fd.
> > > + */
> > > +struct CameraSensorInfo {};
> > > +struct ControlInfoMap {};
> > > +struct ControlList {};
> > > +struct FileDescriptor {};
> >
> > Doesn't FileDescriptor contain a file descriptor ?
>
> Well yes but actually no. FileDescriptor *is* the file descriptor that
> the code generator searches for in the struct encapsulation tree, so it
> doesn't need to be tagged with [hasFd]. Just like how custom data
> structures in the mojom file don't need [hasFd] either, because mojo can
> see which stucts have FileDescriptor and which structs have those
> structs.
Makes sense. Maybe s/has an fd/embeds a FileDescriptor/ above to make
this explicit ?
> > > +[hasFd] struct IPABuffer {};
>
> This guy needs it because none of its mojo members have FileDescriptor,
> but its C++ members do.
>
> > > +struct IPASettings {};
> > > +struct IPAStream {};
> > > +
> > > +/*
> > > + * Any custom structs that the IPA interfaces use must be defined here,
> > > + * usin the mojo interface definition language.
> >
> > s/usin/using/
>
> ack
>
> > > + *
> > > + * It is recommended to use namespacing, to avoid potential collisions with
> > > + * libcamera types. Use mojom's module directive for this.
> > > + */
> >
> > I'm not entirely sure to follow you, there's no structure defined in
> > this file in the whole series. Is this comment meant to explain that an
> > IPA-specific .mojom file should define custom structures ? If so, I
> > think this would be best documented in Documentation/guides/. It doesn't
> > have to be handled in this series, but let's keep it in mind.
>
> Ah, Documentation/guides/ that's where I'm supposed to put it.
>
> I thought that, since I needed this file anyway for the common structs,
> I might as well but docs here too.
I think Documentation/guides/ would be best in this case, as it
documents what pipeline handler/IPA authors have to do, not the
structures and interfaces defined in this file.
> > If the comment means something else, please enlighten me :-)
> >
> > > +
> > > +/*
> > > + * \class IPACoreInterface
> > > + *
> > > + * This mojo interface describes the main interface of the IPA. It must be
> > > + * defined.
> > > + *
> > > + * The main interface must be named as IPA{pipeline_name}Interface.
> > > + * IPACoreInterface serves as an example and documentation, where
> > > + * {pipeline_name} is Core.
> >
> > OK, I think this confirms my understanding of the previous comment
> > block. All this should probably be moved to Documentation/guides/ in the
> > future. It's fine for now.
>
> Oh I can leave it here for now, cool :)
It would be great if you could move the documentation there already, it
would serve as a base for the complete IPA guide. If it's too much, we
can do so on top.
> > > + *
> > > + * At minimum, the following three functions must be present (and implemented):
> > > + * init(IPASettings settings) => (int32 ret);
> > > + * start() => (int32 ret);
> > > + * stop();
> >
> > It would be really nice if mojom supported inheritance for interfaces.
>
> Yeah :/
>
> > > + *
> > > + * All input parameters will become const references, except for primitives,
> >
> > What's a primitive ? Is it a concept defined by mojo ?
>
> I was referring to C++ primitives... Are they called PODs instead?
POD can also be a class or struct.
> bool, [u]int{8,16,32,64}_t, float, double
This is called arithmetic types in C++
(https://en.cppreference.com/w/cpp/language/type).
> > > + * which will simply become const. Output parameters will become pointers,
> > > + * unless there is only one primitive output parameter, in which case it will
> > > + * become a regular return value.
> > > + *
> > > + * const is not allowed inside of arrays and maps. mojo arrays will become C++
> > > + * vectors.
> >
> > s/vectors/std::vector<>/
> >
> > Do these limitations/constraints come from mojo, or from our code
> > generator ?
>
> From mojo.
>
> > > + *
> > > + * By default, all methods defined in the main interface are synchronous. This
> > > + * means that in the case of IPC (ie. isolated IPA), the function call will not
> > > + * return until the return value or output parameters are ready. To specify an
> > > + * asynchronous function, the [async] attribute can be used. Asynchronous
> > > + * methods must not have any return value or output parameters, since in the
> > > + * case of IPC the call needs to return immediately.
> > > + *
> > > + * In addition the following must be defined in {pipeline_name}.h in the
> > > + * libcamera namespace:
> > > + *
> > > + * static ControlInfoMap {pipeline_name}::Controls;
> >
> > Should we explain why, and what this is ?
>
> Yeah, probably.
>
> > I would s/Controls/controls/ as we start variable names with a
> > lower-case letter.
>
> Okay. raspberrypi.h has Controls right now though.
So this series will fix that problem, perfect :-)
> > > + *
> > > + * It may be empty.
> > > + */
> > > +
> > > +/*
> > > + * \class IPACoreCallbackInterface
> >
> > I wonder if this should be called IPACoreEventInterface. Callbacks imply
> > that the IPA can call back into the pipeline handler to drive it, which
> > we don't want. Of course it's just a semantic difference, one can still
> > implement an IPA that drives the pipeline handler with events, but
> > making clear what is acceptable and what isn't would be useful.
>
> Ooh yeah probably.
>
> > > + *
> > > + * This mojo interface describes the callback interface of the IPA. It must be
> > > + * defined. If there are no callback functions, then it may be empty.
> > > + *
> > > + * The callback interface must be named as IPA{pipeline_name}CallbackInterface.
> > > + * IPACoreCallbackInterface serves as an example and documentation, where
> > > + * {pipeline_name} is Core.
> > > + *
> > > + * Methods defined in the callback interface shall not return anything. This
> > > + * also means that these methods must be asynchronous and not synchronous.
> > > + * Therefore the [async] tag is not necessary.
> >
> > I find this slightly confusing, it's not entirely clear why [async]
> > isn't necessary. Would the following reflect a correct understanding ?
> >
> > * Methods defined in the callback interface are implictly asynchronous.
> > * They thus can't return any value. Specifying the [async] tag is not
> > * necessary.
>
> Ah yeah, that's better.
>
> > Do I also understand correctly that specifying the [async] tag will not
> > cause any issue ?
>
> Yeah, it won't. Callback methods are always async; if it has [async] tag
> it'll just be ignored.
>
> I don't have validation though, to enforce that callback methods return
> void. I guess Signals can't return anything anyway? Maybe it's fine.
Correct, signals can't return anything. Validating this at parsing time
would be nice, but can just be recorded as a todo item for now.
> > > + *
> > > + * Methods defined in the callback interface will become Signals in the IPA
> > > + * interface. The IPA can emit signals, while the pipeline handler can connect
> > > + * slots to them.
> > > + */
> > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> > > index 337948d2..590a1464 100644
> > > --- a/include/libcamera/ipa/meson.build
> > > +++ b/include/libcamera/ipa/meson.build
> > > @@ -13,6 +13,17 @@ install_headers(libcamera_ipa_headers,
> > > # Prepare IPA/IPC generation components
> > > #
> > >
> > > +core_mojom_file = 'core.mojom'
> >
> > It would be nice to handle the dependencies automatically. meson
> > supports a depfile argument in custom_target(), which we could use to
> > get mojom to output a list of dependencies parsed from the import
> > statements. Something for later, as we would need to extend the mojo
> > parser to support this feature.
>
> Hm if we want to avoid modifying mojo, I think we can define another
> generator (or extend the current one). It'll take the mojo module file
> as an input, and extract and output the import statements.
>
> Will IPA interfaces get so big that they'll span multiple files? In
> order to enforce pipeline handler-driven pipeline I would expect that
> they'd need to be short and simple.
You're right, it's probably overkill.
> > > +ipa_mojom_core = custom_target(core_mojom_file.split('.')[0] + '_mojom_module',
> > > + input : core_mojom_file,
> > > + output : core_mojom_file + '-module',
> > > + command : [
> > > + mojom_parser,
> > > + '--output-root', meson.build_root(),
> > > + '--input-root', meson.source_root(),
> > > + '--mojoms', '@INPUT@'
> > > + ])
> >
> > So the mojo parser requires compiling all .mojom files explicitly, it
> > doesn't compile the imports automatically ? Not very nice :-(
>
> Yeah :/
>
> > > +
> > > ipa_mojom_files = []
> > >
> > > ipa_mojoms = []
> > > @@ -30,6 +41,7 @@ foreach file : ipa_mojom_files
> > > mojom = custom_target(file.split('.')[0] + '_mojom_module',
> > > input : file,
> > > output : file + '-module',
> > > + depends : ipa_mojom_core,
> > > command : [
> > > mojom_parser,
> > > '--output-root', meson.build_root(),
> > > @@ -41,7 +53,7 @@ foreach file : ipa_mojom_files
> > > header = custom_target(name + '_generated_h',
> > > input : mojom,
> > > output : name + '_generated.h',
> > > - depends : mojom_templates,
> > > + depends : [mojom_templates, ipa_mojom_core],
> > > command : [
> > > mojom_generator, 'generate',
> > > '-g', 'libcamera',
> > > @@ -55,7 +67,7 @@ foreach file : ipa_mojom_files
> > > serializer = custom_target(name + '_serializer_h',
> > > input : mojom,
> > > output : name + '_serializer.h',
> > > - depends : mojom_templates,
> > > + depends : [mojom_templates, ipa_mojom_core],
> > > command : [
> > > mojom_generator, 'generate',
> > > '-g', 'libcamera',
> > > @@ -69,7 +81,7 @@ foreach file : ipa_mojom_files
> > > proxy_header = custom_target(name + '_proxy_h',
> > > input : mojom,
> > > output : 'ipa_proxy_' + name + '.h',
> > > - depends : mojom_templates,
> > > + depends : [mojom_templates, ipa_mojom_core],
> > > command : [
> > > mojom_generator, 'generate',
> > > '-g', 'libcamera',
> > > diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
> > > index ac68ad08..f27b453a 100644
> > > --- a/src/libcamera/proxy/meson.build
> > > +++ b/src/libcamera/proxy/meson.build
> > > @@ -10,7 +10,7 @@ foreach mojom : ipa_mojoms
> > > proxy = custom_target(mojom['name'] + '_proxy_cpp',
> > > input : mojom['mojom'],
> > > output : 'ipa_proxy_' + mojom['name'] + '.cpp',
> > > - depends : [mojom_templates],
> > > + depends : [mojom_templates, ipa_mojom_core],
> > > command : [
> > > mojom_generator, 'generate',
> > > '-g', 'libcamera',
> > > diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build
> > > index 5490811b..353e5cf1 100644
> > > --- a/src/libcamera/proxy/worker/meson.build
> > > +++ b/src/libcamera/proxy/worker/meson.build
> > > @@ -11,7 +11,7 @@ foreach mojom : ipa_mojoms
> > > worker = custom_target(mojom['name'] + '_proxy_worker',
> > > input : mojom['mojom'],
> > > output : 'ipa_proxy_' + mojom['name'] + '_worker.cpp',
> > > - depends : [mojom_templates],
> > > + depends : [mojom_templates, ipa_mojom_core],
> > > command : [
> > > mojom_generator, 'generate',
> > > '-g', 'libcamera',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list