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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:
> Asynchronous tasks can only be submitted while the IPA is running.

, as they are queued to the IPA thread for execution and will thus never
run if the IPA is stopped.

> Further more, the shutdown sequence can not be tracked with a simple

s/Further more/Furthermore/ ?

> 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.
> 
> 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 +++++--

Oh, you've now overcome the fear of touching the mojo templates. Be
careful, you'll soon be considered a reference person in this area :-D

>  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}});
>  }
>  
> 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,

Maybe ProxyStopped, ProxyStopping, ProxyRunning to protect about
namespace clashes ?

> +	};

Blank line ?

> +	enum {{proxy_name}}States state_;

You can drop enum here.

> +

But I'd drop this one :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart 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,

Laurent Pinchart