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

18586 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>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 Documentation/guides/ipa.rst      | 39 ++++++++++++++++++++-----------
 include/libcamera/ipa/meson.build | 36 +++++++++++++++++-----------
 2 files changed, 48 insertions(+), 27 deletions(-)

Hi Naush,

Thank you for the patch.

On Wed, May 03, 2023 at 01:20:23PM +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>
> ---
>  Documentation/guides/ipa.rst      | 39 ++++++++++++++++++++-----------
>  include/libcamera/ipa/meson.build | 36 +++++++++++++++++-----------
>  2 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst
> index fc0317451e24..51dbe3aad5f5 100644
> --- a/Documentation/guides/ipa.rst
> +++ b/Documentation/guides/ipa.rst
> @@ -19,6 +19,16 @@ connect to, in order to receive data from the IPA asynchronously. In addition,
>  it contains any custom data structures that the pipeline handler and IPA may
>  pass to each other.
>  
> +It is possible to use the same IPA interface with multiple pipeline handlers
> +on different hardware platforms. Generally in such cases, these platforms would
> +have a common hardware ISP pipeline. 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,
> +the same IPA interface could be used for both pipeline handlers. The build files
> +provide a mapping from pipeline handler to the IPA interface name as detailed in
> +:ref:`compiling-section`.
> +
>  The IPA protocol refers to the agreement between the pipeline handler and the
>  IPA regarding the expected response(s) from the IPA for given calls to the IPA.
>  This protocol doesn't need to be declared anywhere in code, but it shall be
> @@ -43,7 +53,7 @@ interface definition is thus written by the pipeline handler author, based on
>  how they design the interactions between the pipeline handler and the IPA.
>  
>  The entire IPA interface, including the functions, signals, and any custom
> -structs shall be defined in a file named {pipeline_name}.mojom under
> +structs shall be defined in a file named {interface_name}.mojom under
>  include/libcamera/ipa/.
>  
>  .. _mojo Interface Definition Language: https://chromium.googlesource.com/chromium/src.git/+/master/mojo/public/tools/bindings/README.md
> @@ -150,8 +160,6 @@ and the Event IPA interface, which describes the signals received by the
>  pipeline handler that the IPA can emit. Both must be defined. This section
>  focuses on the Main IPA interface.
>  
> -The main interface must be named as IPA{pipeline_name}Interface.
> -

Did you drop this on purpose ?

>  The functions that the pipeline handler can call from the IPA may be
>  synchronous or asynchronous. Synchronous functions do not return until the IPA
>  returns from the function, while asynchronous functions return immediately
> @@ -243,7 +251,7 @@ then it may be empty. These emissions are meant to notify the pipeline handler
>  of some event, such as request data is ready, and *must not* be used to drive
>  the camera pipeline from the IPA.
>  
> -The event interface must be named as IPA{pipeline_name}EventInterface.
> +The event interface must be named as IPA{interface_name}EventInterface.
>  
>  Functions defined in the event interface are implicitly asynchronous.
>  Thus they cannot return any value. Specifying the [async] tag is not
> @@ -266,38 +274,41 @@ The following is an example of an event interface definition:
>                  setStaggered(libcamera.ControlList controls);
>          };
>  
> +.. _compiling-section:
> +
>  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.

It's not an object but a variable, I'll fix this while at it.

s/with an ipa/to its IPA/

>  
>  For example, adding the raspberrypi.mojom file to meson:
>  
>  .. code-block:: none
>  
> -        ipa_mojom_files = [
> -            'raspberrypi.mojom',
> +        pipeline_ipa_mojom_mapping = [
> +            'rpi/vc4': '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
> @@ -312,7 +323,7 @@ file, the following header must be included:
>  
>  .. code-block:: C++
>  
> -   #include <libcamera/ipa/{pipeline_name}_ipa_interface.h>
> +   #include <libcamera/ipa/{interface_name}_ipa_interface.h>
>  
>  The POD types of the structs simply become their C++ counterparts, eg. uint32
>  in mojo will become uint32_t in C++. mojo map becomes C++ std::map, and mojo
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index 442ca3dd7e1c..c57b3a5e1570 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
> +

No need for a blank line.

I'll apply all these small changes, no need to resubmit.

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

>      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',

Hi Laurent,

On Thu, 4 May 2023 at 16:38, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, May 03, 2023 at 01:20:23PM +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>
> > ---
> >  Documentation/guides/ipa.rst      | 39 ++++++++++++++++++++-----------
> >  include/libcamera/ipa/meson.build | 36 +++++++++++++++++-----------
> >  2 files changed, 48 insertions(+), 27 deletions(-)
> >
> > diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst
> > index fc0317451e24..51dbe3aad5f5 100644
> > --- a/Documentation/guides/ipa.rst
> > +++ b/Documentation/guides/ipa.rst
> > @@ -19,6 +19,16 @@ connect to, in order to receive data from the IPA asynchronously. In addition,
> >  it contains any custom data structures that the pipeline handler and IPA may
> >  pass to each other.
> >
> > +It is possible to use the same IPA interface with multiple pipeline handlers
> > +on different hardware platforms. Generally in such cases, these platforms would
> > +have a common hardware ISP pipeline. 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,
> > +the same IPA interface could be used for both pipeline handlers. The build files
> > +provide a mapping from pipeline handler to the IPA interface name as detailed in
> > +:ref:`compiling-section`.
> > +
> >  The IPA protocol refers to the agreement between the pipeline handler and the
> >  IPA regarding the expected response(s) from the IPA for given calls to the IPA.
> >  This protocol doesn't need to be declared anywhere in code, but it shall be
> > @@ -43,7 +53,7 @@ interface definition is thus written by the pipeline handler author, based on
> >  how they design the interactions between the pipeline handler and the IPA.
> >
> >  The entire IPA interface, including the functions, signals, and any custom
> > -structs shall be defined in a file named {pipeline_name}.mojom under
> > +structs shall be defined in a file named {interface_name}.mojom under
> >  include/libcamera/ipa/.
> >
> >  .. _mojo Interface Definition Language: https://chromium.googlesource.com/chromium/src.git/+/master/mojo/public/tools/bindings/README.md
> > @@ -150,8 +160,6 @@ and the Event IPA interface, which describes the signals received by the
> >  pipeline handler that the IPA can emit. Both must be defined. This section
> >  focuses on the Main IPA interface.
> >
> > -The main interface must be named as IPA{pipeline_name}Interface.
> > -
>
> Did you drop this on purpose ?

Oops, no I didn't.  But we do need to replace pipeline_name with
interface_name.  Finger trouble on my part.

>
> >  The functions that the pipeline handler can call from the IPA may be
> >  synchronous or asynchronous. Synchronous functions do not return until the IPA
> >  returns from the function, while asynchronous functions return immediately
> > @@ -243,7 +251,7 @@ then it may be empty. These emissions are meant to notify the pipeline handler
> >  of some event, such as request data is ready, and *must not* be used to drive
> >  the camera pipeline from the IPA.
> >
> > -The event interface must be named as IPA{pipeline_name}EventInterface.
> > +The event interface must be named as IPA{interface_name}EventInterface.
> >
> >  Functions defined in the event interface are implicitly asynchronous.
> >  Thus they cannot return any value. Specifying the [async] tag is not
> > @@ -266,38 +274,41 @@ The following is an example of an event interface definition:
> >                  setStaggered(libcamera.ControlList controls);
> >          };
> >
> > +.. _compiling-section:
> > +
> >  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.
>
> It's not an object but a variable, I'll fix this while at it.
>
> s/with an ipa/to its IPA/
>
> >
> >  For example, adding the raspberrypi.mojom file to meson:
> >
> >  .. code-block:: none
> >
> > -        ipa_mojom_files = [
> > -            'raspberrypi.mojom',
> > +        pipeline_ipa_mojom_mapping = [
> > +            'rpi/vc4': '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
> > @@ -312,7 +323,7 @@ file, the following header must be included:
> >
> >  .. code-block:: C++
> >
> > -   #include <libcamera/ipa/{pipeline_name}_ipa_interface.h>
> > +   #include <libcamera/ipa/{interface_name}_ipa_interface.h>
> >
> >  The POD types of the structs simply become their C++ counterparts, eg. uint32
> >  in mojo will become uint32_t in C++. mojo map becomes C++ std::map, and mojo
> > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> > index 442ca3dd7e1c..c57b3a5e1570 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
> > +
>
> No need for a blank line.
>
> I'll apply all these small changes, no need to resubmit.

Thanks for that!

Regards,
Naush

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >      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',
>
> --
> Regards,
>
> Laurent Pinchart