[libcamera-devel,01/13] meson: ipa: Add mapping for pipeline handler to mojom interface file

18549 diff mbox series

Allow an arbitrary mapping between the pipeline handler and IPA mojom
interface file in the build system. This removes the 1:1 mapping of
pipeline handler name to mojom filename, and allows more flexibility to
pipeline developers.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 Documentation/guides/ipa.rst      | 19 ++++++++--------
 include/libcamera/ipa/meson.build | 36 ++++++++++++++++++++-----------
 2 files changed, 33 insertions(+), 22 deletions(-)

Hi Naush

On Wed, Apr 26, 2023 at 02:10:45PM +0100, Naushir Patuck via libcamera-devel wrote:
> Allow an arbitrary mapping between the pipeline handler and IPA mojom
> interface file in the build system. This removes the 1:1 mapping of
> pipeline handler name to mojom filename, and allows more flexibility to
> pipeline developers.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  Documentation/guides/ipa.rst      | 19 ++++++++--------
>  include/libcamera/ipa/meson.build | 36 ++++++++++++++++++++-----------
>  2 files changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst
> index fc0317451e24..89839408672a 100644
> --- a/Documentation/guides/ipa.rst
> +++ b/Documentation/guides/ipa.rst
> @@ -269,35 +269,36 @@ The following is an example of an event interface definition:
>  Compiling the IPA interface
>  ---------------------------
>
> -After the IPA interface is defined in include/libcamera/ipa/{pipeline_name}.mojom,
> +After the IPA interface is defined in include/libcamera/ipa/{interface_name}.mojom,
>  an entry for it must be added in meson so that it can be compiled. The filename
> -must be added to the ipa_mojom_files object in include/libcamera/ipa/meson.build.
> +must be added to the pipeline_ipa_mojom_mapping object in include/libcamera/ipa/meson.build.
> +This object maps the pipeline handler name with an ipa interface file.
>
>  For example, adding the raspberrypi.mojom file to meson:
>
>  .. code-block:: none
>
> -        ipa_mojom_files = [
> -            'raspberrypi.mojom',
> +        pipeline_ipa_mojom_mapping = [
> +            'raspberrypi': 'raspberrypi.mojom',
>          ]
>
>  This will cause the mojo data definition file to be compiled. Specifically, it
>  generates five files:
>
>  - a header describing the custom data structures, and the complete IPA
> -  interface (at {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_interface.h)
> +  interface (at {$build_dir}/include/libcamera/ipa/{interface}_ipa_interface.h)
>
>  - a serializer implementing de/serialization for the custom data structures (at
> -  {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_serializer.h)
> +  {$build_dir}/include/libcamera/ipa/{interface}_ipa_serializer.h)
>
>  - a proxy header describing a specialized IPA proxy (at
> -  {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_proxy.h)
> +  {$build_dir}/include/libcamera/ipa/{interface}_ipa_proxy.h)
>
>  - a proxy source implementing the IPA proxy (at
> -  {$build_dir}/src/libcamera/proxy/{pipeline}_ipa_proxy.cpp)
> +  {$build_dir}/src/libcamera/proxy/{interface}_ipa_proxy.cpp)
>
>  - a proxy worker source implementing the other end of the IPA proxy (at
> -  {$build_dir}/src/libcamera/proxy/worker/{pipeline}_ipa_proxy_worker.cpp)
> +  {$build_dir}/src/libcamera/proxy/worker/{interface}_ipa_proxy_worker.cpp)
>
>  The IPA proxy serves as the layer between the pipeline handler and the IPA, and
>  handles threading vs isolation transparently. The pipeline handler and the IPA
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index 442ca3dd7e1c..67c31cb04ccf 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -60,13 +60,15 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h',
>                        './' +'@INPUT@'
>                    ])
>
> -ipa_mojom_files = [
> -    'ipu3.mojom',
> -    'raspberrypi.mojom',
> -    'rkisp1.mojom',
> -    'vimc.mojom',
> -]
> -
> +# Mapping from pipeline handler name to mojom file
> +pipeline_ipa_mojom_mapping = {
> +    'ipu3': 'ipu3.mojom',
> +    'rkisp1': 'rkisp1.mojom',
> +    'raspberrypi': 'raspberrypi.mojom',
> +    'vimc': 'vimc.mojom',
> +}
> +
> +ipa_mojom_files = []
>  ipa_mojoms = []
>
>  #
> @@ -75,14 +77,22 @@ ipa_mojoms = []
>
>  # TODO Define per-pipeline ControlInfoMap with yaml?
>
> -foreach file : ipa_mojom_files
> +foreach pipeline, file : pipeline_ipa_mojom_mapping
> +
>      name = file.split('.')[0]
>
> -    if name not in pipelines
> +    # Ensure we do not build duplicate mojom modules
> +    if (file in ipa_mojom_files)
> +        continue
> +    endif
> +
> +    ipa_mojom_files += file
> +
> +    if pipeline not in pipelines
>          continue
>      endif
>
> -    # {pipeline}.mojom-module
> +    # {interface}.mojom-module
>      mojom = custom_target(name + '_mojom_module',
>                            input : file,
>                            output : file + '-module',
> @@ -94,7 +104,7 @@ foreach file : ipa_mojom_files
>                                '--mojoms', '@INPUT@'
>                            ])
>
> -    # {pipeline}_ipa_interface.h
> +    # {interface}_ipa_interface.h
>      header = custom_target(name + '_ipa_interface_h',
>                             input : mojom,
>                             output : name + '_ipa_interface.h',
> @@ -110,7 +120,7 @@ foreach file : ipa_mojom_files
>                                 './' +'@INPUT@'
>                             ])
>
> -    # {pipeline}_ipa_serializer.h
> +    # {interface}_ipa_serializer.h
>      serializer = custom_target(name + '_ipa_serializer_h',
>                                 input : mojom,
>                                 output : name + '_ipa_serializer.h',
> @@ -124,7 +134,7 @@ foreach file : ipa_mojom_files
>                                     './' +'@INPUT@'
>                                 ])
>
> -    # {pipeline}_ipa_proxy.h
> +    # {interface}_ipa_proxy.h
>      proxy_header = custom_target(name + '_proxy_h',
>                                   input : mojom,
>                                   output : name + '_ipa_proxy.h',
> --
> 2.34.1
>

Hi Naush,

Thank you for the patch.

On Wed, Apr 26, 2023 at 02:10:45PM +0100, Naushir Patuck via libcamera-devel wrote:
> Allow an arbitrary mapping between the pipeline handler and IPA mojom
> interface file in the build system. This removes the 1:1 mapping of
> pipeline handler name to mojom filename, and allows more flexibility to
> pipeline developers.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  Documentation/guides/ipa.rst      | 19 ++++++++--------
>  include/libcamera/ipa/meson.build | 36 ++++++++++++++++++++-----------
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst
> index fc0317451e24..89839408672a 100644
> --- a/Documentation/guides/ipa.rst
> +++ b/Documentation/guides/ipa.rst
> @@ -269,35 +269,36 @@ The following is an example of an event interface definition:
>  Compiling the IPA interface
>  ---------------------------
>  
> -After the IPA interface is defined in include/libcamera/ipa/{pipeline_name}.mojom,
> +After the IPA interface is defined in include/libcamera/ipa/{interface_name}.mojom,

There are three references to {pipeline_name} above that need to be
addressed.

Just replacing {pipeline_name} with {interface_name} there would be
confusing I think, the text should be extended to explain that each
pipeline handler maps to an interface name, as to describe the rules
that govern the choice of the interface name (both the hard
requirements, and the recommended naming scheme).

I think this change is interesting. I envision it could enable us to
share the same IPA modules between different pipeline handlers. For
instance, the rkisp1 pipeline handler supports both the RK3399 and the
i.MX8MP as they integrate the same ISP. However, the i.MX8MP has a more
complex camera pipeline, which may call for a dedicated pipeline handler
in the future. As the ISP is the same as for RK3399, I think we could
use the same IPA module for both pipeline handlers, and would thus need
the same IPA interface.

Could you capture this in the documentation when explaining the concept
of IPA interface ? Otherwise, decoupling the pipeline handler name and
the IPA interface name without explaining why would lead to confusion.

>  an entry for it must be added in meson so that it can be compiled. The filename
> -must be added to the ipa_mojom_files object in include/libcamera/ipa/meson.build.
> +must be added to the pipeline_ipa_mojom_mapping object in include/libcamera/ipa/meson.build.
> +This object maps the pipeline handler name with an ipa interface file.
>  
>  For example, adding the raspberrypi.mojom file to meson:
>  
>  .. code-block:: none
>  
> -        ipa_mojom_files = [
> -            'raspberrypi.mojom',
> +        pipeline_ipa_mojom_mapping = [
> +            'raspberrypi': 'raspberrypi.mojom',
>          ]
>  
>  This will cause the mojo data definition file to be compiled. Specifically, it
>  generates five files:
>  
>  - a header describing the custom data structures, and the complete IPA
> -  interface (at {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_interface.h)
> +  interface (at {$build_dir}/include/libcamera/ipa/{interface}_ipa_interface.h)
>  
>  - a serializer implementing de/serialization for the custom data structures (at
> -  {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_serializer.h)
> +  {$build_dir}/include/libcamera/ipa/{interface}_ipa_serializer.h)
>  
>  - a proxy header describing a specialized IPA proxy (at
> -  {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_proxy.h)
> +  {$build_dir}/include/libcamera/ipa/{interface}_ipa_proxy.h)
>  
>  - a proxy source implementing the IPA proxy (at
> -  {$build_dir}/src/libcamera/proxy/{pipeline}_ipa_proxy.cpp)
> +  {$build_dir}/src/libcamera/proxy/{interface}_ipa_proxy.cpp)
>  
>  - a proxy worker source implementing the other end of the IPA proxy (at
> -  {$build_dir}/src/libcamera/proxy/worker/{pipeline}_ipa_proxy_worker.cpp)
> +  {$build_dir}/src/libcamera/proxy/worker/{interface}_ipa_proxy_worker.cpp)
>  
>  The IPA proxy serves as the layer between the pipeline handler and the IPA, and
>  handles threading vs isolation transparently. The pipeline handler and the IPA
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index 442ca3dd7e1c..67c31cb04ccf 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -60,13 +60,15 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h',
>                        './' +'@INPUT@'
>                    ])
>  
> -ipa_mojom_files = [
> -    'ipu3.mojom',
> -    'raspberrypi.mojom',
> -    'rkisp1.mojom',
> -    'vimc.mojom',
> -]
> -
> +# Mapping from pipeline handler name to mojom file
> +pipeline_ipa_mojom_mapping = {
> +    'ipu3': 'ipu3.mojom',
> +    'rkisp1': 'rkisp1.mojom',
> +    'raspberrypi': 'raspberrypi.mojom',
> +    'vimc': 'vimc.mojom',
> +}
> +
> +ipa_mojom_files = []
>  ipa_mojoms = []
>  
>  #
> @@ -75,14 +77,22 @@ ipa_mojoms = []
>  
>  # TODO Define per-pipeline ControlInfoMap with yaml?
>  
> -foreach file : ipa_mojom_files
> +foreach pipeline, file : pipeline_ipa_mojom_mapping
> +
>      name = file.split('.')[0]
>  
> -    if name not in pipelines
> +    # Ensure we do not build duplicate mojom modules

It sounds like that would be a major bug in meson.build, so it should be
reported as an error if it happens. The risk sounds low though, so I'd
also be fine dropping the check.

> +    if (file in ipa_mojom_files)

No need for parentheses.

> +        continue
> +    endif
> +
> +    ipa_mojom_files += file
> +
> +    if pipeline not in pipelines
>          continue
>      endif
>  
> -    # {pipeline}.mojom-module
> +    # {interface}.mojom-module
>      mojom = custom_target(name + '_mojom_module',
>                            input : file,
>                            output : file + '-module',
> @@ -94,7 +104,7 @@ foreach file : ipa_mojom_files
>                                '--mojoms', '@INPUT@'
>                            ])
>  
> -    # {pipeline}_ipa_interface.h
> +    # {interface}_ipa_interface.h
>      header = custom_target(name + '_ipa_interface_h',
>                             input : mojom,
>                             output : name + '_ipa_interface.h',
> @@ -110,7 +120,7 @@ foreach file : ipa_mojom_files
>                                 './' +'@INPUT@'
>                             ])
>  
> -    # {pipeline}_ipa_serializer.h
> +    # {interface}_ipa_serializer.h
>      serializer = custom_target(name + '_ipa_serializer_h',
>                                 input : mojom,
>                                 output : name + '_ipa_serializer.h',
> @@ -124,7 +134,7 @@ foreach file : ipa_mojom_files
>                                     './' +'@INPUT@'
>                                 ])
>  
> -    # {pipeline}_ipa_proxy.h
> +    # {interface}_ipa_proxy.h
>      proxy_header = custom_target(name + '_proxy_h',
>                                   input : mojom,
>                                   output : name + '_ipa_proxy.h',