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

[Kieran Bingham]

Hi Laurent,

On 26/03/2021 12:34, Laurent Pinchart wrote:
> 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.

I can *read* the sentence ... but not interpret the location that needs
updating ;-)

Can you provide additional context please?

Otherwise I'll leave this out, and let someone tackle it on top.

-- 
Regards
--
Kieran