[libcamera-devel,2/2] utils: ipc: Support extending IPA init() parameters

11460 diff mbox series

It can be useful for pipeline handlers to pass additional parameters to
the IPA init() method. Allow doing so.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../libcamera_templates/module_ipa_proxy.cpp.tmpl         | 2 +-
 .../generators/libcamera_templates/proxy_functions.tmpl   | 8 ++++++--
 utils/ipc/generators/mojom_libcamera_generator.py         | 3 ---
 3 files changed, 7 insertions(+), 6 deletions(-)

Hi Laurent,

Thank you for your patch.

On Tue, 2 Mar 2021 at 15:05, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> It can be useful for pipeline handlers to pass additional parameters to
> the IPA init() method. Allow doing so.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../libcamera_templates/module_ipa_proxy.cpp.tmpl         | 2 +-
>  .../generators/libcamera_templates/proxy_functions.tmpl   | 8 ++++++--
>  utils/ipc/generators/mojom_libcamera_generator.py         | 3 ---
>  3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git
> a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index d451fab35e83..f2f9128b056c 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -142,7 +142,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage
> &data)
>  {{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
>  {
>  {%- if method.mojom_name == "init" %}
> -       {{proxy_funcs.init_thread_body()}}
> +       {{proxy_funcs.init_thread_body(method)}}
>  {%- elif method.mojom_name == "stop" %}
>         {{proxy_funcs.stop_thread_body()}}
>  {%- elif method.mojom_name == "start" %}
> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index 40611feb179b..222a2a63764d 100644
> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -22,8 +22,12 @@
>  {#
>   # \brief Generate function body for IPA init() function for thread
>   #}
> -{%- macro init_thread_body() -%}
> -       int ret = ipa_->init(settings);
> +{%- macro init_thread_body(method) -%}
> +       int ret = ipa_->init(
> +       {%- for param in method|method_param_names -%}
> +               {{param}}{{- ", " if not loop.last}}
> +       {%- endfor -%}
> +);
>         if (ret)
>                 return ret;
>
> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py
> b/utils/ipc/generators/mojom_libcamera_generator.py
> index 438e41c649ad..2bfc4af23231 100644
> --- a/utils/ipc/generators/mojom_libcamera_generator.py
> +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> @@ -346,10 +346,7 @@ def ValidateInterfaces(interfaces):
>      f_stop  = f_stop[0]
>
>      # Validate parameters to init()
> -    ValidateSingleLength(f_init.parameters, 'input parameter to init()')
>      ValidateSingleLength(f_init.response_parameters, 'output parameter
> from init()')
> -    if f_init.parameters[0].kind.mojom_name != 'IPASettings':
> -        raise Exception('init() must have single IPASettings input
> parameter')
>      if f_init.response_parameters[0].kind.spec != 'i32':
>          raise Exception('init() must have single int32 output parameter')
>

As discussed, it would be really useful if the restriction on the return
value from init() could be relaxed as well.
Hopefully this will not be too complicated to achieve :-)

Regards,
Naush



>
> --
> Regards,
>
> Laurent Pinchart
>
>

Hi Laurent and Naush,

On Tue, Mar 02, 2021 at 03:16:43PM +0000, Naushir Patuck wrote:
> Hi Laurent,
> 
> Thank you for your patch.
> 
> On Tue, 2 Mar 2021 at 15:05, Laurent Pinchart <
> laurent.pinchart@ideasonboard.com> wrote:
> 
>     It can be useful for pipeline handlers to pass additional parameters to
>     the IPA init() method. Allow doing so.
> 
>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     ---
>      .../libcamera_templates/module_ipa_proxy.cpp.tmpl         | 2 +-
>      .../generators/libcamera_templates/proxy_functions.tmpl   | 8 ++++++--
>      utils/ipc/generators/mojom_libcamera_generator.py         | 3 ---
>      3 files changed, 7 insertions(+), 6 deletions(-)
> 
>     diff --git a/utils/ipc/generators/libcamera_templates/
>     module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/
>     module_ipa_proxy.cpp.tmpl
>     index d451fab35e83..f2f9128b056c 100644
>     --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>     +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>     @@ -142,7 +142,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &
>     data)
>      {{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
>      {
>      {%- if method.mojom_name == "init" %}
>     -       {{proxy_funcs.init_thread_body()}}
>     +       {{proxy_funcs.init_thread_body(method)}}
>      {%- elif method.mojom_name == "stop" %}
>             {{proxy_funcs.stop_thread_body()}}
>      {%- elif method.mojom_name == "start" %}

Marker (for reference in paragraph below).

>     diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>     b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>     index 40611feb179b..222a2a63764d 100644
>     --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>     +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>     @@ -22,8 +22,12 @@
>      {#
>       # \brief Generate function body for IPA init() function for thread
>       #}
>     -{%- macro init_thread_body() -%}
>     -       int ret = ipa_->init(settings);
>     +{%- macro init_thread_body(method) -%}
>     +       int ret = ipa_->init(
>     +       {%- for param in method|method_param_names -%}
>     +               {{param}}{{- ", " if not loop.last}}
>     +       {%- endfor -%}
>     +);
>             if (ret)
>                     return ret;

So, this is sufficient for custom input parameters. But what we're doing
(especially since we want to add support for custom output parameters as
well) is analogous to what we've already done to start(), so I think
it's better to put it at the marker above.

Like we can
- {%- elif not method|is_async %}
+ {%- elif not method|is_async or method.mojom_name == "init" %}

and then after the shared function body, conditionally (on
method.mojom_name == "init") put in proxy_.moveToThread(&thread_);

Then we can get rid of init_thread_body() too.

The IPC part, as you've noticed, should be able to handle the custom
definition as-is.

> 
>     diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/
>     generators/mojom_libcamera_generator.py
>     index 438e41c649ad..2bfc4af23231 100644
>     --- a/utils/ipc/generators/mojom_libcamera_generator.py
>     +++ b/utils/ipc/generators/mojom_libcamera_generator.py
>     @@ -346,10 +346,7 @@ def ValidateInterfaces(interfaces):
>          f_stop  = f_stop[0]
> 
>          # Validate parameters to init()
>     -    ValidateSingleLength(f_init.parameters, 'input parameter to init()')
>          ValidateSingleLength(f_init.response_parameters, 'output parameter
>     from init()')
>     -    if f_init.parameters[0].kind.mojom_name != 'IPASettings':
>     -        raise Exception('init() must have single IPASettings input
>     parameter')
>          if f_init.response_parameters[0].kind.spec != 'i32':
>              raise Exception('init() must have single int32 output parameter')

We can remove the whole stanza after we support totally customizable
parameters to init().

> 
> 
> As discussed, it would be really useful if the restriction on the return value
> from init() could be relaxed as well.
> Hopefully this will not be too complicated to achieve :-)

Yeah we can do that :)

I'll send a patch tomorrow if Laurent doesn't get nerd-sniped before
that :)


Paul

Hi Paul,

On Wed, Mar 03, 2021 at 07:26:13PM +0900, paul.elder@ideasonboard.com wrote:
> On Tue, Mar 02, 2021 at 03:16:43PM +0000, Naushir Patuck wrote:
> > On Tue, 2 Mar 2021 at 15:05, Laurent Pinchart wrote:
> > 
> > > It can be useful for pipeline handlers to pass additional parameters to
> > > the IPA init() method. Allow doing so.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  .../libcamera_templates/module_ipa_proxy.cpp.tmpl         | 2 +-
> > >  .../generators/libcamera_templates/proxy_functions.tmpl   | 8 ++++++--
> > >  utils/ipc/generators/mojom_libcamera_generator.py         | 3 ---
> > >  3 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/utils/ipc/generators/libcamera_templates/
> > > module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/
> > > module_ipa_proxy.cpp.tmpl
> > > index d451fab35e83..f2f9128b056c 100644
> > > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > @@ -142,7 +142,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &
> > > data)
> > >  {{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> > >  {
> > >  {%- if method.mojom_name == "init" %}
> > > -       {{proxy_funcs.init_thread_body()}}
> > > +       {{proxy_funcs.init_thread_body(method)}}
> > >  {%- elif method.mojom_name == "stop" %}
> > >         {{proxy_funcs.stop_thread_body()}}
> > >  {%- elif method.mojom_name == "start" %}
> 
> Marker (for reference in paragraph below).
> 
> > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > index 40611feb179b..222a2a63764d 100644
> > > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > @@ -22,8 +22,12 @@
> > >  {#
> > >   # \brief Generate function body for IPA init() function for thread
> > >   #}
> > > -{%- macro init_thread_body() -%}
> > > -       int ret = ipa_->init(settings);
> > > +{%- macro init_thread_body(method) -%}
> > > +       int ret = ipa_->init(
> > > +       {%- for param in method|method_param_names -%}
> > > +               {{param}}{{- ", " if not loop.last}}
> > > +       {%- endfor -%}
> > > +);
> > >         if (ret)
> > >                 return ret;
> 
> So, this is sufficient for custom input parameters. But what we're doing
> (especially since we want to add support for custom output parameters as
> well) is analogous to what we've already done to start(), so I think
> it's better to put it at the marker above.
> 
> Like we can
> - {%- elif not method|is_async %}
> + {%- elif not method|is_async or method.mojom_name == "init" %}
> 
> and then after the shared function body, conditionally (on
> method.mojom_name == "init") put in proxy_.moveToThread(&thread_);
> 
> Then we can get rid of init_thread_body() too.

Should we also improve the output parameters handling, so that if the
first output parameter is an int, it's returned from the function
instead of creating a void function with an int * output parameter ?

> The IPC part, as you've noticed, should be able to handle the custom
> definition as-is.
>
> > > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/
> > > generators/mojom_libcamera_generator.py
> > > index 438e41c649ad..2bfc4af23231 100644
> > > --- a/utils/ipc/generators/mojom_libcamera_generator.py
> > > +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> > > @@ -346,10 +346,7 @@ def ValidateInterfaces(interfaces):
> > >      f_stop  = f_stop[0]
> > > 
> > >      # Validate parameters to init()
> > > -    ValidateSingleLength(f_init.parameters, 'input parameter to init()')
> > >      ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')
> > > -    if f_init.parameters[0].kind.mojom_name != 'IPASettings':
> > > -        raise Exception('init() must have single IPASettings input parameter')
> > >      if f_init.response_parameters[0].kind.spec != 'i32':
> > >          raise Exception('init() must have single int32 output parameter')
> 
> We can remove the whole stanza after we support totally customizable
> parameters to init().
> 
> > As discussed, it would be really useful if the restriction on the return value
> > from init() could be relaxed as well.
> > Hopefully this will not be too complicated to achieve :-)
> 
> Yeah we can do that :)
> 
> I'll send a patch tomorrow if Laurent doesn't get nerd-sniped before
> that :)