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

[Kieran Bingham]

Hi Laurent / Paul,

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:
>> Hi Kieran,
>>
>> 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 ...

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