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

[paul.elder at ideasonboard.com]

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 at 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 at 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