[libcamera-devel] [PATCH v3 26/38] ipa: Add core.mojom
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 4 11:33:04 CEST 2020
Hi Paul,
Thank you for the patch.
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 ?
> +[hasFd] struct IPABuffer {};
> +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/
> + *
> + * 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.
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.
> + *
> + * 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.
> + *
> + * All input parameters will become const references, except for primitives,
What's a primitive ? Is it a concept defined by mojo ?
> + * 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 ?
> + *
> + * 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 ?
I would s/Controls/controls/ as we start variable names with a
lower-case letter.
> + *
> + * 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.
> + *
> + * 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.
Do I also understand correctly that specifying the [async] tag will not
cause any issue ?
> + *
> + * 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.
> +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 :-(
> +
> 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