[libcamera-devel] [PATCH v4 24/37] ipa: Add core.mojom

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Nov 06, 2020 at 07:36:54PM +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 v4:
> - move docs to IPA guide
> 
> 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       | 18 ++++++++++++++++++
>  include/libcamera/ipa/meson.build      | 18 +++++++++++++++---
>  src/libcamera/proxy/meson.build        |  2 +-
>  src/libcamera/proxy/worker/meson.build |  2 +-
>  4 files changed, 35 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..ed26aaad
> --- /dev/null
> +++ b/include/libcamera/ipa/core.mojom
> @@ -0,0 +1,18 @@
> +/* 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,

s/de\/serializer/(de)serializer/ ?
s/in IPADataSerializer/as a template specialization of IPADataSerializer/ ?

> + * the corresponding header file (or forward-declarations) must be placed in

s/placed/included/

> + * {pipeline_name}.h.
> + *
> + * For libcamera types, the [hasFd] attribute is needed to notify the compiler
> + * that the struct embeds a FileDescriptor.
> + */
> +struct CameraSensorInfo {};
> +struct ControlInfoMap {};
> +struct ControlList {};
> +struct FileDescriptor {};
> +[hasFd] struct IPABuffer {};
> +struct IPASettings {};
> +struct IPAStream {};

I think the last three structures can be defined fully in core.mojom.
The comment above will need to be updated, to reflect the fact that
specializations of IPADataSerializer are only needed for opaque
structures, not structures defined in mojom.

> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index d7a2b6b2..b7caec95 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'
> +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@'
> +                               ])

It would be nice if the mojo parser was able to handle imports
automatically :-S

> +
>  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 + '_ipa_interface_h',
>                             input : mojom,
>                             output : name + '_ipa_interface.h',
> -                           depends : mojom_templates,
> +                           depends : [mojom_templates, ipa_mojom_core],

Do we need this, given that input is set to mojom, which depends on
ipa_mojom_core already ? The dependency is for the parsing step, not the
generation step. I think all the dependencies below can be dropped.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>                             command : [
>                                 mojom_generator, 'generate',
>                                 '-g', 'libcamera',
> @@ -55,7 +67,7 @@ foreach file : ipa_mojom_files
>      serializer = custom_target(name + '_ipa_serializer_h',
>                                 input : mojom,
>                                 output : name + '_ipa_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 8e0b978a..628bb050 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