[libcamera-devel] [PATCH v2 1/8] utils: ipc: proxy: Assert asynchronous calls execute in the running state

[Kieran Bingham]

Hi Laurent,

On 13/03/2021 21:36, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Mar 12, 2021 at 05:47:20AM +0000, Kieran Bingham wrote:
>> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> Signals and calls from the IPA should not occur after the IPA has been
>> put into the stopped state.
>>
>> Add assertions to catch and prevent any messages being processed after
>> this.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl  | 2 ++
>>  utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 4 ++--
>>  2 files changed, 4 insertions(+), 2 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 f75b1206cad4..e3b541db4e36 100644
>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>> @@ -173,6 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
>>  	{%- endfor -%}
>>  );
>>  {% elif method|is_async %}
>> +	ASSERT(running_);
> 
> This will fail to catch pipeline handlers calling into an IPA that is
> stopping, from the event handlers called by the dispatchMessages() call
> in 2/8. I think we need to turn running_ into a state_ variable, with
> three states (Running, Stopping, Stopped) and ASSERT(state_ == Running)
> here.
> 
>>  	proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,
>>  	{%- for param in method|method_param_names -%}
>>  		{{param}}{{- ", " if not loop.last}}
>> @@ -225,6 +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_);
> 
> Here we would ASSERT(state_ != Stopped).
> 
>>  	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
>>  }
>>  
>> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>> index c2ac42fca45b..13dc8fdcab6e 100644
>> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>> @@ -26,12 +26,12 @@
>>  	if (!running_)
>>  		return;
>>  
>> -	running_ = false;
>> -
> 
> 	state_ = Stopping;
> 
>>  	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
>>  
>>  	thread_.exit();
>>  	thread_.wait();
>> +
>> +	running_ = false;
> 
> 	state_ = Stopped;
> 
> This could go on top of this series, as this patch is already an
> improvement on its own.
> 

Yes, I think this is a worthwhile exercise, I'd prefer on top at the minute.



>>  {%- endmacro -%}
>>  
>>  
> 

-- 
Regards
--
Kieran