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

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 15, 2021 at 12:38:05PM +0000, Kieran Bingham wrote:
> On 13/03/2021 21:36, Laurent Pinchart wrote:
> > 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.

Fine with me. Could you either include a patch on top in the next
iteration, or add a \todo comment ?

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

-- 
Regards,

Laurent Pinchart