[libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA with a state machine

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 29, 2021 at 12:31:49PM +0100, Kieran Bingham wrote:
> On 26/03/2021 12:34, Laurent Pinchart wrote:
> > On Fri, Mar 26, 2021 at 12:27:46PM +0000, Kieran Bingham wrote:
> >> On 26/03/2021 02:52, Laurent Pinchart wrote:
> >>> On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder at ideasonboard.com wrote:
> >>>> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:
> >>>>> Asynchronous tasks can only be submitted while the IPA is running.
> >>>>>
> >>>>> Further more, the shutdown sequence can not be tracked with a simple
> >>>>> running flag. We can also be in the state 'Stopping' where we have not
> >>>>> yet completed all events, but we must not commence anything new.
> >>>>>
> >>>>> Refactor the running_ boolean into a stateful enum to track this.
> >>>>
> >>>> Ooh, good idea!>>
> >>>>>
> >>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>>> ---
> >>>>>  .../libcamera_templates/module_ipa_proxy.cpp.tmpl         | 8 ++++----
> >>>>>  .../libcamera_templates/module_ipa_proxy.h.tmpl           | 8 +++++++-
> >>>>>  .../generators/libcamera_templates/proxy_functions.tmpl   | 7 +++++--
> >>>>>  3 files changed, 16 insertions(+), 7 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 e3b541db4e36..dd0f4d3f64b6 100644
> >>>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >>>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >>>>> @@ -45,7 +45,7 @@ namespace {{ns}} {
> >>>>>  {%- endif %}
> >>>>>  
> >>>>>  {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
> >>>>> -	: IPAProxy(ipam), running_(false),
> >>>>> +	: IPAProxy(ipam), state_(Stopped),
> >>>>>  	  isolate_(isolate), seq_(0)
> >>>>>  {
> >>>>>  	LOG(IPAProxy, Debug)
> >>>>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >>>>>  
> >>>>>  	return {{ "_ret" if method|method_return_value != "void" }};
> >>>>>  {%- elif method.mojom_name == "start" %}
> >>>>> -	running_ = true;
> >>>>> +	state_ = Running;
> >>>>>  	thread_.start();
> >>>>>  
> >>>>>  	{{ "return " if method|method_return_value != "void" -}}
> >>>>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >>>>>  	{%- endfor -%}
> >>>>>  );
> >>>>>  {% elif method|is_async %}
> >>>>> -	ASSERT(running_);
> >>>>> +	ASSERT(state_ == Running);
> >>>>>  	proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,
> >>>>>  	{%- for param in method|method_param_names -%}
> >>>>>  		{{param}}{{- ", " if not loop.last}}
> >>>>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >>>>>  {% for method in interface_event.methods %}
> >>>>>  {{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> >>>>>  {
> >>>>> -	ASSERT(running_);
> >>>>> +	ASSERT(state_ != Stopped);
> >>>>>  	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> >>>>>  }
> >>>>
> >>>> As mentioned on irc, I think we need to copy these over to the IPC
> >>>> versions too, but that can be done on top.
> >>
> >> I'm not sure I understand this here ...
> > 
> > Paul meant that asserts in the IPC case would be useful.
> 
> I can *read* the sentence ... but not interpret the location that needs
> updating ;-)

Sorry :-)

> Can you provide additional context please?
> 
> Otherwise I'll leave this out, and let someone tackle it on top.

It can go on top, no issue. It should actually go on top, as I expect we
may need a different state machine for the IPC case, given that we need
to keep processing messages while waiting for the reply to sync calls,
and that will be a larger development.

-- 
Regards,

Laurent Pinchart