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

Message ID 20210312054727.852622-2-kieran.bingham@ideasonboard.com
State Changes Requested
Delegated to: Kieran Bingham
Headers show
Series
  • IPU3 Stability
Related show

Commit Message

Kieran Bingham March 12, 2021, 5:47 a.m. UTC 
From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@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(-)

Comments

Niklas Söderlund March 12, 2021, 11:12 p.m. UTC | #1 
Hi Kieran and Laurent,

On 2021-03-12 05:47:20 +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

I think this makes sens.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  .../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_);
>  	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_);
>  	{{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;
> -
>  	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
>  
>  	thread_.exit();
>  	thread_.wait();
> +
> +	running_ = false;
>  {%- endmacro -%}
>  
>  
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi March 13, 2021, 9:26 a.m. UTC | #2 
Hi Kieran

On Fri, Mar 12, 2021 at 05:47:20AM +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  .../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_);
>  	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_);
>  	{{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;
> -
>  	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
>
>  	thread_.exit();
>  	thread_.wait();
> +
> +	running_ = false;
>  {%- endmacro -%}
>
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart March 13, 2021, 9:36 p.m. UTC | #3 
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@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@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@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.

>  {%- endmacro -%}
>  
>
Kieran Bingham March 15, 2021, 12:38 p.m. UTC | #4 
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@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@ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@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 -%}
>>  
>>  
>
Laurent Pinchart March 15, 2021, 11:17 p.m. UTC | #5 
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@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@ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@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 -%}
> >>  
> >>

Patch
diff mbox series 

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_);
 	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_);
 	{{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;
-
 	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
 
 	thread_.exit();
 	thread_.wait();
+
+	running_ = false;
 {%- endmacro -%}