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

[Laurent Pinchart]

Hi Paul,

On Wed, Mar 03, 2021 at 07:26:13PM +0900, paul.elder at 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 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.

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 :)

-- 
Regards,

Laurent Pinchart