[libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA with a state machine
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 26 03:52:18 CET 2021
On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder at ideasonboard.com wrote:
> Hi Kieran,
>
> 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.
> >
> > 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,
> > + };
> > + enum {{proxy_name}}States state_;
> > +
>
> Since these are the same for all IPAProxies, should we put them in
> ipa_proxy.h in the base IPAProxy class instead?
I like this.
> With or without the suggested change,
>
> Reviewed-by: Paul Elder <paul.elder 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
More information about the libcamera-devel
mailing list