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

[Laurent Pinchart]

Hi Kieran,

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.

> >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> >>> index efb09a180b90..e33c26492d91 100644
> >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> >>> @@ -104,7 +104,13 @@ private:
> >>>  		{{interface_name}} *ipa_;
> >>>  	};
> >>>  
> >>> -	bool running_;
> >>> +	enum {{proxy_name}}States {
> >>> +		Stopped,
> >>> +		Stopping,
> >>> +		Running,
> >>> +	};
> >>> +	enum {{proxy_name}}States state_;
> >>> +
> >>
> >> Since these are the same for all IPAProxies, should we put them in
> >> ipa_proxy.h in the base IPAProxy class instead?
> > 
> > I like this.
> 
> I started with that and then moved it here for some reason, that I can
> no longer recall.
> 
> But keeping it common is better I agree, so I'll move it back.
> 
> >> With or without the suggested change,
> >>
> >> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> >>
> >>>  	Thread thread_;
> >>>  	ThreadProxy proxy_;
> >>>  	std::unique_ptr<{{interface_name}}> ipa_;
> >>> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> >>> index 8addc2fad0a8..09394a4fec83 100644
> >>> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> >>> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> >>> @@ -23,9 +23,12 @@
> >>>   # \brief Generate function body for IPA stop() function for thread
> >>>   #}
> >>>  {%- macro stop_thread_body() -%}
> >>> -	if (!running_)
> >>> +	ASSERT(state_ != Stopping);
> >>> +	if (state_ != Running)
> >>>  		return;
> >>>  
> >>> +	state_ = Stopping;
> >>> +
> >>>  	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
> >>>  
> >>>  	thread_.exit();
> >>> @@ -33,7 +36,7 @@
> >>>  
> >>>  	Thread::current()->dispatchMessages(Message::Type::InvokeMessage);
> >>>  
> >>> -	running_ = false;
> >>> +	state_ = Stopped;
> >>>  {%- endmacro -%}
> >>>  
> >>>  

-- 
Regards,

Laurent Pinchart