[{"id":15913,"web_url":"https://patchwork.libcamera.org/comment/15913/","msgid":"","date":"2021-03-26T02:00:34","subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:\n> Asynchronous tasks can only be submitted while the IPA is running.\n\n, as they are queued to the IPA thread for execution and will thus never\nrun if the IPA is stopped.\n\n> Further more, the shutdown sequence can not be tracked with a simple\n\ns/Further more/Furthermore/ ?\n\n> running flag. We can also be in the state 'Stopping' where we have not\n> yet completed all events, but we must not commence anything new.\n> \n> Refactor the running_ boolean into a stateful enum to track this.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++----\n> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++-\n> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++--\n\nOh, you've now overcome the fear of touching the mojo templates. Be\ncareful, you'll soon be considered a reference person in this area :-D\n\n> 3 files changed, 16 insertions(+), 7 deletions(-)\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index e3b541db4e36..dd0f4d3f64b6 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -45,7 +45,7 @@ namespace {{ns}} {\n> {%- endif %}\n> \n> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> -\t: IPAProxy(ipam), running_(false),\n> +\t: IPAProxy(ipam), state_(Stopped),\n> \t isolate_(isolate), seq_(0)\n> {\n> \tLOG(IPAProxy, Debug)\n> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> \n> \treturn {{ \"_ret\" if method|method_return_value != \"void\" }};\n> {%- elif method.mojom_name == \"start\" %}\n> -\trunning_ = true;\n> +\tstate_ = Running;\n> \tthread_.start();\n> \n> \t{{ \"return \" if method|method_return_value != \"void\" -}}\n> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> \t{%- endfor -%}\n> );\n> {% elif method|is_async %}\n> -\tASSERT(running_);\n> +\tASSERT(state_ == Running);\n> \tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> \t{%- for param in method|method_param_names -%}\n> \t\t{{param}}{{- \", \" if not loop.last}}\n> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> {% for method in interface_event.methods %}\n> {{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> {\n> -\tASSERT(running_);\n> +\tASSERT(state_ != Stopped);\n> \t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> }\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index efb09a180b90..e33c26492d91 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -104,7 +104,13 @@ private:\n> \t\t{{interface_name}} *ipa_;\n> \t};\n> \n> -\tbool running_;\n> +\tenum {{proxy_name}}States {\n> +\t\tStopped,\n> +\t\tStopping,\n> +\t\tRunning,\n\nMaybe ProxyStopped, ProxyStopping, ProxyRunning to protect about\nnamespace clashes ?\n\n> +\t};\n\nBlank line ?\n\n> +\tenum {{proxy_name}}States state_;\n\nYou can drop enum here.\n\n> +\n\nBut I'd drop this one :-)\n\nReviewed-by: Laurent Pinchart \n\n> \tThread thread_;\n> \tThreadProxy proxy_;\n> \tstd::unique_ptr<{{interface_name}}> ipa_;\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index 8addc2fad0a8..09394a4fec83 100644\n> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -23,9 +23,12 @@\n> # \\brief Generate function body for IPA stop() function for thread\n> #}\n> {%- macro stop_thread_body() -%}\n> -\tif (!running_)\n> +\tASSERT(state_ != Stopping);\n> +\tif (state_ != Running)\n> \t\treturn;\n> \n> +\tstate_ = Stopping;\n> +\n> \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> \n> \tthread_.exit();\n> @@ -33,7 +36,7 @@\n> \n> \tThread::current()->dispatchMessages(Message::Type::InvokeMessage);\n> \n> -\trunning_ = false;\n> +\tstate_ = Stopped;\n> {%- endmacro -%}\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A365BDC66\n\tfor ;\n\tFri, 26 Mar 2021 02:01:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF6CE68D66;\n\tFri, 26 Mar 2021 03:01:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D111A602E3\n\tfor ;\n\tFri, 26 Mar 2021 03:01:17 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C696B6F;\n\tFri, 26 Mar 2021 03:01:17 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Wmeu5iex\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616724077;\n\tbh=W2YcZVCvaBqjgW2Xht5dOPmhm5S3JrfH7tCKnriFRTI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wmeu5iexuV0nwTkN4phUzVfezd2Gbh3KJ9LQWmwdbcTPHj7J8Zb8sNV5BA/ltXaUW\n\tBfAWvFS+KxzXtGP/424pjVvPFi7ogjmuP0tIfP3eHiaVhHpXjdAwNf4FE5O2tiWJeh\n\tq5jAH31IQh9DrklXHHiTiUWL4ZvPbRMx4UVBxzt0=","Date":"Fri, 26 Mar 2021 04:00:34 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15922,"web_url":"https://patchwork.libcamera.org/comment/15922/","msgid":"<20210326025010.GC2040@pyrite.rasen.tech>","date":"2021-03-26T02:50:10","subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:\n> Asynchronous tasks can only be submitted while the IPA is running.\n> \n> Further more, the shutdown sequence can not be tracked with a simple\n> running flag. We can also be in the state 'Stopping' where we have not\n> yet completed all events, but we must not commence anything new.\n> \n> Refactor the running_ boolean into a stateful enum to track this.\n\nOoh, good idea!\n\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++----\n> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++-\n> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++--\n> 3 files changed, 16 insertions(+), 7 deletions(-)\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index e3b541db4e36..dd0f4d3f64b6 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -45,7 +45,7 @@ namespace {{ns}} {\n> {%- endif %}\n> \n> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> -\t: IPAProxy(ipam), running_(false),\n> +\t: IPAProxy(ipam), state_(Stopped),\n> \t isolate_(isolate), seq_(0)\n> {\n> \tLOG(IPAProxy, Debug)\n> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> \n> \treturn {{ \"_ret\" if method|method_return_value != \"void\" }};\n> {%- elif method.mojom_name == \"start\" %}\n> -\trunning_ = true;\n> +\tstate_ = Running;\n> \tthread_.start();\n> \n> \t{{ \"return \" if method|method_return_value != \"void\" -}}\n> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> \t{%- endfor -%}\n> );\n> {% elif method|is_async %}\n> -\tASSERT(running_);\n> +\tASSERT(state_ == Running);\n> \tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> \t{%- for param in method|method_param_names -%}\n> \t\t{{param}}{{- \", \" if not loop.last}}\n> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> {% for method in interface_event.methods %}\n> {{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> {\n> -\tASSERT(running_);\n> +\tASSERT(state_ != Stopped);\n> \t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> }\n\nAs mentioned on irc, I think we need to copy these over to the IPC\nversions too, but that can be done on top.\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index efb09a180b90..e33c26492d91 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -104,7 +104,13 @@ private:\n> \t\t{{interface_name}} *ipa_;\n> \t};\n> \n> -\tbool running_;\n> +\tenum {{proxy_name}}States {\n> +\t\tStopped,\n> +\t\tStopping,\n> +\t\tRunning,\n> +\t};\n> +\tenum {{proxy_name}}States state_;\n> +\n\nSince these are the same for all IPAProxies, should we put them in\nipa_proxy.h in the base IPAProxy class instead?\n\n\nWith or without the suggested change,\n\nReviewed-by: Paul Elder \n\n> \tThread thread_;\n> \tThreadProxy proxy_;\n> \tstd::unique_ptr<{{interface_name}}> ipa_;\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index 8addc2fad0a8..09394a4fec83 100644\n> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -23,9 +23,12 @@\n> # \\brief Generate function body for IPA stop() function for thread\n> #}\n> {%- macro stop_thread_body() -%}\n> -\tif (!running_)\n> +\tASSERT(state_ != Stopping);\n> +\tif (state_ != Running)\n> \t\treturn;\n> \n> +\tstate_ = Stopping;\n> +\n> \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> \n> \tthread_.exit();\n> @@ -33,7 +36,7 @@\n> \n> \tThread::current()->dispatchMessages(Message::Type::InvokeMessage);\n> \n> -\trunning_ = false;\n> +\tstate_ = Stopped;\n> {%- endmacro -%}\n> \n> \n> -- \n> 2.25.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EEF62C32EA\n\tfor ;\n\tFri, 26 Mar 2021 02:50:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 545096084F;\n\tFri, 26 Mar 2021 03:50:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E45E5602E3\n\tfor ;\n\tFri, 26 Mar 2021 03:50:17 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EF36C443;\n\tFri, 26 Mar 2021 03:50:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"r1UKeb/t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616727017;\n\tbh=yqgRjtJYVMIfmFc8gaeYaoBJFg7CNcFO+bymb4vYsmw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r1UKeb/tQ6Ill7PBwzZl0+1z2vpryF/atA0n/+GddNmYF1ioFVI5KsHPY+3cj42Rt\n\tZA6COo19QASaeDEmd1t0QT6FH1VeeUCY/cjTwCiOOpLA4G6eoEH6LaNhaim9xp3+kP\n\ttqaDX04ZhAzzDTETzZZC2xYf7uFTQPTB6DPnC+ac=","Date":"Fri, 26 Mar 2021 11:50:10 +0900","From":"paul.elder@ideasonboard.com","To":"Kieran Bingham ","Message-ID":"<20210326025010.GC2040@pyrite.rasen.tech>","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15925,"web_url":"https://patchwork.libcamera.org/comment/15925/","msgid":"","date":"2021-03-26T02:52:18","subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Kieran,\n> \n> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:\n> > Asynchronous tasks can only be submitted while the IPA is running.\n> > \n> > Further more, the shutdown sequence can not be tracked with a simple\n> > running flag. We can also be in the state 'Stopping' where we have not\n> > yet completed all events, but we must not commence anything new.\n> > \n> > Refactor the running_ boolean into a stateful enum to track this.\n> \n> Ooh, good idea!\n> \n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++----\n> > .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++-\n> > .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++--\n> > 3 files changed, 16 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > index e3b541db4e36..dd0f4d3f64b6 100644\n> > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > @@ -45,7 +45,7 @@ namespace {{ns}} {\n> > {%- endif %}\n> > \n> > {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> > -\t: IPAProxy(ipam), running_(false),\n> > +\t: IPAProxy(ipam), state_(Stopped),\n> > \t isolate_(isolate), seq_(0)\n> > {\n> > \tLOG(IPAProxy, Debug)\n> > @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> > \n> > \treturn {{ \"_ret\" if method|method_return_value != \"void\" }};\n> > {%- elif method.mojom_name == \"start\" %}\n> > -\trunning_ = true;\n> > +\tstate_ = Running;\n> > \tthread_.start();\n> > \n> > \t{{ \"return \" if method|method_return_value != \"void\" -}}\n> > @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> > \t{%- endfor -%}\n> > );\n> > {% elif method|is_async %}\n> > -\tASSERT(running_);\n> > +\tASSERT(state_ == Running);\n> > \tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> > \t{%- for param in method|method_param_names -%}\n> > \t\t{{param}}{{- \", \" if not loop.last}}\n> > @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> > {% for method in interface_event.methods %}\n> > {{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> > {\n> > -\tASSERT(running_);\n> > +\tASSERT(state_ != Stopped);\n> > \t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> > }\n> \n> As mentioned on irc, I think we need to copy these over to the IPC\n> versions too, but that can be done on top.\n> > \n> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > index efb09a180b90..e33c26492d91 100644\n> > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > @@ -104,7 +104,13 @@ private:\n> > \t\t{{interface_name}} *ipa_;\n> > \t};\n> > \n> > -\tbool running_;\n> > +\tenum {{proxy_name}}States {\n> > +\t\tStopped,\n> > +\t\tStopping,\n> > +\t\tRunning,\n> > +\t};\n> > +\tenum {{proxy_name}}States state_;\n> > +\n> \n> Since these are the same for all IPAProxies, should we put them in\n> ipa_proxy.h in the base IPAProxy class instead?\n\nI like this.\n\n> With or without the suggested change,\n> \n> Reviewed-by: Paul Elder \n> \n> > \tThread thread_;\n> > \tThreadProxy proxy_;\n> > \tstd::unique_ptr<{{interface_name}}> ipa_;\n> > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > index 8addc2fad0a8..09394a4fec83 100644\n> > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > @@ -23,9 +23,12 @@\n> > # \\brief Generate function body for IPA stop() function for thread\n> > #}\n> > {%- macro stop_thread_body() -%}\n> > -\tif (!running_)\n> > +\tASSERT(state_ != Stopping);\n> > +\tif (state_ != Running)\n> > \t\treturn;\n> > \n> > +\tstate_ = Stopping;\n> > +\n> > \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> > \n> > \tthread_.exit();\n> > @@ -33,7 +36,7 @@\n> > \n> > \tThread::current()->dispatchMessages(Message::Type::InvokeMessage);\n> > \n> > -\trunning_ = false;\n> > +\tstate_ = Stopped;\n> > {%- endmacro -%}\n> > \n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B3A41C32EA\n\tfor ;\n\tFri, 26 Mar 2021 02:53:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6DA4E6084F;\n\tFri, 26 Mar 2021 03:53:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30499602E3\n\tfor ;\n\tFri, 26 Mar 2021 03:53:02 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B754F443;\n\tFri, 26 Mar 2021 03:53:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QJ1V2Au1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616727181;\n\tbh=+YAfIjuC4UaZ+R6I7CgqdK/3opCqzhHXXzV77LfZWIM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QJ1V2Au1PyrgX6qP3wmnn9IDvbepRWnGM1uhoa6VceCyjUNVIPyQhF+YsIkuIYRsl\n\tVmjG+3vLOZ16cngQd4dkekBT+Fjbc8C6vYgAGE6fb4g3ofIadB29c3l2Pz5irwUrgC\n\tv4ZLgMIxmUxRvjljIh8ujkJFs6BB0dg0qTSDxmCA=","Date":"Fri, 26 Mar 2021 04:52:18 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>\n\t<20210326025010.GC2040@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210326025010.GC2040@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15943,"web_url":"https://patchwork.libcamera.org/comment/15943/","msgid":"<01398c71-f703-a6c8-1dce-84213b9a73f0@ideasonboard.com>","date":"2021-03-26T12:27:46","subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent / Paul,\n\nOn 26/03/2021 02:52, Laurent Pinchart wrote:\n> On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote:\n>> Hi Kieran,\n>>\n>> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:\n>>> Asynchronous tasks can only be submitted while the IPA is running.\n>>>\n>>> Further more, the shutdown sequence can not be tracked with a simple\n>>> running flag. We can also be in the state 'Stopping' where we have not\n>>> yet completed all events, but we must not commence anything new.\n>>>\n>>> Refactor the running_ boolean into a stateful enum to track this.\n>>\n>> Ooh, good idea!>>\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++----\n>>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++-\n>>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++--\n>>> 3 files changed, 16 insertions(+), 7 deletions(-)\n>>>\n>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>> index e3b541db4e36..dd0f4d3f64b6 100644\n>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>> @@ -45,7 +45,7 @@ namespace {{ns}} {\n>>> {%- endif %}\n>>> \n>>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n>>> -\t: IPAProxy(ipam), running_(false),\n>>> +\t: IPAProxy(ipam), state_(Stopped),\n>>> \t isolate_(isolate), seq_(0)\n>>> {\n>>> \tLOG(IPAProxy, Debug)\n>>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>> \n>>> \treturn {{ \"_ret\" if method|method_return_value != \"void\" }};\n>>> {%- elif method.mojom_name == \"start\" %}\n>>> -\trunning_ = true;\n>>> +\tstate_ = Running;\n>>> \tthread_.start();\n>>> \n>>> \t{{ \"return \" if method|method_return_value != \"void\" -}}\n>>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>> \t{%- endfor -%}\n>>> );\n>>> {% elif method|is_async %}\n>>> -\tASSERT(running_);\n>>> +\tASSERT(state_ == Running);\n>>> \tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n>>> \t{%- for param in method|method_param_names -%}\n>>> \t\t{{param}}{{- \", \" if not loop.last}}\n>>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>> {% for method in interface_event.methods %}\n>>> {{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>> {\n>>> -\tASSERT(running_);\n>>> +\tASSERT(state_ != Stopped);\n>>> \t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>> }\n>>\n>> As mentioned on irc, I think we need to copy these over to the IPC\n>> versions too, but that can be done on top.\n\nI'm not sure I understand this here ...\n\n>>> \n>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>> index efb09a180b90..e33c26492d91 100644\n>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>> @@ -104,7 +104,13 @@ private:\n>>> \t\t{{interface_name}} *ipa_;\n>>> \t};\n>>> \n>>> -\tbool running_;\n>>> +\tenum {{proxy_name}}States {\n>>> +\t\tStopped,\n>>> +\t\tStopping,\n>>> +\t\tRunning,\n>>> +\t};\n>>> +\tenum {{proxy_name}}States state_;\n>>> +\n>>\n>> Since these are the same for all IPAProxies, should we put them in\n>> ipa_proxy.h in the base IPAProxy class instead?\n> \n> I like this.\n\nI started with that and then moved it here for some reason, that I can\nno longer recall.\n\nBut keeping it common is better I agree, so I'll move it back.\n\n\n> \n>> With or without the suggested change,\n>>\n>> Reviewed-by: Paul Elder \n>>\n>>> \tThread thread_;\n>>> \tThreadProxy proxy_;\n>>> \tstd::unique_ptr<{{interface_name}}> ipa_;\n>>> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n>>> index 8addc2fad0a8..09394a4fec83 100644\n>>> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n>>> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n>>> @@ -23,9 +23,12 @@\n>>> # \\brief Generate function body for IPA stop() function for thread\n>>> #}\n>>> {%- macro stop_thread_body() -%}\n>>> -\tif (!running_)\n>>> +\tASSERT(state_ != Stopping);\n>>> +\tif (state_ != Running)\n>>> \t\treturn;\n>>> \n>>> +\tstate_ = Stopping;\n>>> +\n>>> \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n>>> \n>>> \tthread_.exit();\n>>> @@ -33,7 +36,7 @@\n>>> \n>>> \tThread::current()->dispatchMessages(Message::Type::InvokeMessage);\n>>> \n>>> -\trunning_ = false;\n>>> +\tstate_ = Stopped;\n>>> {%- endmacro -%}\n>>> \n>>> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 97CA2C32EA\n\tfor ;\n\tFri, 26 Mar 2021 12:27:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAAB068D47;\n\tFri, 26 Mar 2021 13:27:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61548602D7\n\tfor ;\n\tFri, 26 Mar 2021 13:27:51 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A8929443;\n\tFri, 26 Mar 2021 13:27:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lqlhMAKW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616761670;\n\tbh=aXkvAzaf8E15o/hYlNu6pZiGONpO/mxzAWgub/BYJx8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=lqlhMAKW2TgyjnkEq1oWlJSeCVD8MkrNXpgL99KcLBWPeW6qP1ivnr+vboxINLWsK\n\t9m3oget65/98YnYO/zlWuVRl2+6rKDenpLBgisp1Plb1epoIaqEhJVRQaCsqfbr5pb\n\tMimF7bMj+BCwoLvtuowoQmdLc1t0rPLT55Eo34fU=","To":"Laurent Pinchart ,\n\tpaul.elder@ideasonboard.com","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>\n\t<20210326025010.GC2040@pyrite.rasen.tech>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<01398c71-f703-a6c8-1dce-84213b9a73f0@ideasonboard.com>","Date":"Fri, 26 Mar 2021 12:27:46 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15944,"web_url":"https://patchwork.libcamera.org/comment/15944/","msgid":"","date":"2021-03-26T12:34:37","subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Mar 26, 2021 at 12:27:46PM +0000, Kieran Bingham wrote:\n> On 26/03/2021 02:52, Laurent Pinchart wrote:\n> > On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote:\n> >> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:\n> >>> Asynchronous tasks can only be submitted while the IPA is running.\n> >>>\n> >>> Further more, the shutdown sequence can not be tracked with a simple\n> >>> running flag. We can also be in the state 'Stopping' where we have not\n> >>> yet completed all events, but we must not commence anything new.\n> >>>\n> >>> Refactor the running_ boolean into a stateful enum to track this.\n> >>\n> >> Ooh, good idea!>>\n> >>>\n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++----\n> >>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++-\n> >>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++--\n> >>> 3 files changed, 16 insertions(+), 7 deletions(-)\n> >>>\n> >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>> index e3b541db4e36..dd0f4d3f64b6 100644\n> >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>> @@ -45,7 +45,7 @@ namespace {{ns}} {\n> >>> {%- endif %}\n> >>> \n> >>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> >>> -\t: IPAProxy(ipam), running_(false),\n> >>> +\t: IPAProxy(ipam), state_(Stopped),\n> >>> \t isolate_(isolate), seq_(0)\n> >>> {\n> >>> \tLOG(IPAProxy, Debug)\n> >>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>> \n> >>> \treturn {{ \"_ret\" if method|method_return_value != \"void\" }};\n> >>> {%- elif method.mojom_name == \"start\" %}\n> >>> -\trunning_ = true;\n> >>> +\tstate_ = Running;\n> >>> \tthread_.start();\n> >>> \n> >>> \t{{ \"return \" if method|method_return_value != \"void\" -}}\n> >>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>> \t{%- endfor -%}\n> >>> );\n> >>> {% elif method|is_async %}\n> >>> -\tASSERT(running_);\n> >>> +\tASSERT(state_ == Running);\n> >>> \tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> >>> \t{%- for param in method|method_param_names -%}\n> >>> \t\t{{param}}{{- \", \" if not loop.last}}\n> >>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>> {% for method in interface_event.methods %}\n> >>> {{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >>> {\n> >>> -\tASSERT(running_);\n> >>> +\tASSERT(state_ != Stopped);\n> >>> \t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >>> }\n> >>\n> >> As mentioned on irc, I think we need to copy these over to the IPC\n> >> versions too, but that can be done on top.\n> \n> I'm not sure I understand this here ...\n\nPaul meant that asserts in the IPC case would be useful.\n\n> >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>> index efb09a180b90..e33c26492d91 100644\n> >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>> @@ -104,7 +104,13 @@ private:\n> >>> \t\t{{interface_name}} *ipa_;\n> >>> \t};\n> >>> \n> >>> -\tbool running_;\n> >>> +\tenum {{proxy_name}}States {\n> >>> +\t\tStopped,\n> >>> +\t\tStopping,\n> >>> +\t\tRunning,\n> >>> +\t};\n> >>> +\tenum {{proxy_name}}States state_;\n> >>> +\n> >>\n> >> Since these are the same for all IPAProxies, should we put them in\n> >> ipa_proxy.h in the base IPAProxy class instead?\n> > \n> > I like this.\n> \n> I started with that and then moved it here for some reason, that I can\n> no longer recall.\n> \n> But keeping it common is better I agree, so I'll move it back.\n> \n> >> With or without the suggested change,\n> >>\n> >> Reviewed-by: Paul Elder \n> >>\n> >>> \tThread thread_;\n> >>> \tThreadProxy proxy_;\n> >>> \tstd::unique_ptr<{{interface_name}}> ipa_;\n> >>> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> >>> index 8addc2fad0a8..09394a4fec83 100644\n> >>> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> >>> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> >>> @@ -23,9 +23,12 @@\n> >>> # \\brief Generate function body for IPA stop() function for thread\n> >>> #}\n> >>> {%- macro stop_thread_body() -%}\n> >>> -\tif (!running_)\n> >>> +\tASSERT(state_ != Stopping);\n> >>> +\tif (state_ != Running)\n> >>> \t\treturn;\n> >>> \n> >>> +\tstate_ = Stopping;\n> >>> +\n> >>> \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> >>> \n> >>> \tthread_.exit();\n> >>> @@ -33,7 +36,7 @@\n> >>> \n> >>> \tThread::current()->dispatchMessages(Message::Type::InvokeMessage);\n> >>> \n> >>> -\trunning_ = false;\n> >>> +\tstate_ = Stopped;\n> >>> {%- endmacro -%}\n> >>> \n> >>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EF3D7C32EB\n\tfor ;\n\tFri, 26 Mar 2021 12:35:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 44AE368D47;\n\tFri, 26 Mar 2021 13:35:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5AB0602D7\n\tfor ;\n\tFri, 26 Mar 2021 13:35:21 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4DE5E443;\n\tFri, 26 Mar 2021 13:35:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c3G9MwcQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616762121;\n\tbh=MYvkDTSu2/Nrz8chqtOnOpOHyA7XdujxEkgWXSAJkic=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c3G9MwcQcldZhZAJplywvaYMKPC4NvrXICC3rU2j5AD/yJ+MIvDFl4k0WFDYBvq+g\n\tjSu9lIxbh6CXS01GBPt2HOdlcBo5VWYXdFKnv4UbvmPMSHjAqrIFCMVU2qJUe61nkp\n\tamsKqAJAGfomu4Ji/gWC/Ot9JJyeaUezqUiTQpL0=","Date":"Fri, 26 Mar 2021 14:34:37 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>\n\t<20210326025010.GC2040@pyrite.rasen.tech>\n\t\n\t<01398c71-f703-a6c8-1dce-84213b9a73f0@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<01398c71-f703-a6c8-1dce-84213b9a73f0@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16015,"web_url":"https://patchwork.libcamera.org/comment/16015/","msgid":"","date":"2021-03-29T11:31:49","subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 26/03/2021 12:34, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Fri, Mar 26, 2021 at 12:27:46PM +0000, Kieran Bingham wrote:\n>> On 26/03/2021 02:52, Laurent Pinchart wrote:\n>>> On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote:\n>>>> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:\n>>>>> Asynchronous tasks can only be submitted while the IPA is running.\n>>>>>\n>>>>> Further more, the shutdown sequence can not be tracked with a simple\n>>>>> running flag. We can also be in the state 'Stopping' where we have not\n>>>>> yet completed all events, but we must not commence anything new.\n>>>>>\n>>>>> Refactor the running_ boolean into a stateful enum to track this.\n>>>>\n>>>> Ooh, good idea!>>\n>>>>>\n>>>>> Signed-off-by: Kieran Bingham \n>>>>> ---\n>>>>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++----\n>>>>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++-\n>>>>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++--\n>>>>> 3 files changed, 16 insertions(+), 7 deletions(-)\n>>>>>\n>>>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>> index e3b541db4e36..dd0f4d3f64b6 100644\n>>>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>> @@ -45,7 +45,7 @@ namespace {{ns}} {\n>>>>> {%- endif %}\n>>>>> \n>>>>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n>>>>> -\t: IPAProxy(ipam), running_(false),\n>>>>> +\t: IPAProxy(ipam), state_(Stopped),\n>>>>> \t isolate_(isolate), seq_(0)\n>>>>> {\n>>>>> \tLOG(IPAProxy, Debug)\n>>>>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>> \n>>>>> \treturn {{ \"_ret\" if method|method_return_value != \"void\" }};\n>>>>> {%- elif method.mojom_name == \"start\" %}\n>>>>> -\trunning_ = true;\n>>>>> +\tstate_ = Running;\n>>>>> \tthread_.start();\n>>>>> \n>>>>> \t{{ \"return \" if method|method_return_value != \"void\" -}}\n>>>>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>> \t{%- endfor -%}\n>>>>> );\n>>>>> {% elif method|is_async %}\n>>>>> -\tASSERT(running_);\n>>>>> +\tASSERT(state_ == Running);\n>>>>> \tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n>>>>> \t{%- for param in method|method_param_names -%}\n>>>>> \t\t{{param}}{{- \", \" if not loop.last}}\n>>>>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>> {% for method in interface_event.methods %}\n>>>>> {{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>>>> {\n>>>>> -\tASSERT(running_);\n>>>>> +\tASSERT(state_ != Stopped);\n>>>>> \t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>>>> }\n>>>>\n>>>> As mentioned on irc, I think we need to copy these over to the IPC\n>>>> versions too, but that can be done on top.\n>>\n>> I'm not sure I understand this here ...\n> \n> Paul meant that asserts in the IPC case would be useful.\n\nI can *read* the sentence ... but not interpret the location that needs\nupdating ;-)\n\nCan you provide additional context please?\n\nOtherwise I'll leave this out, and let someone tackle it on top.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 611C7C32EF\n\tfor ;\n\tMon, 29 Mar 2021 11:31:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59E3668786;\n\tMon, 29 Mar 2021 13:31:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A17F6877D\n\tfor ;\n\tMon, 29 Mar 2021 13:31:53 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 78B1B292;\n\tMon, 29 Mar 2021 13:31:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G+9HP4ry\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617017512;\n\tbh=O+7eVKXRXhrFs9j/KPMnN20PHy++8WlxquJHlnQrtv8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=G+9HP4ry8Ht+cUDeyy8Cp8CoKOirLygwtRhEpfeLYihWHlz5j+u7RyH+QhZyYoW3G\n\thnVA/XIf93VztWhmfo5a+cqy/ngM/9UAyWrZLhEMvLb7smUfoBOmpyKPtiAd6VrHW4\n\tWRTHIrt3bAF5D5x7iw+rgdIkS6sfbt1MbRIy46KQ=","To":"Laurent Pinchart ","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>\n\t<20210326025010.GC2040@pyrite.rasen.tech>\n\t\n\t<01398c71-f703-a6c8-1dce-84213b9a73f0@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 29 Mar 2021 12:31:49 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16024,"web_url":"https://patchwork.libcamera.org/comment/16024/","msgid":"","date":"2021-03-29T17:26:49","subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Mar 29, 2021 at 12:31:49PM +0100, Kieran Bingham wrote:\n> On 26/03/2021 12:34, Laurent Pinchart wrote:\n> > On Fri, Mar 26, 2021 at 12:27:46PM +0000, Kieran Bingham wrote:\n> >> On 26/03/2021 02:52, Laurent Pinchart wrote:\n> >>> On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote:\n> >>>> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote:\n> >>>>> Asynchronous tasks can only be submitted while the IPA is running.\n> >>>>>\n> >>>>> Further more, the shutdown sequence can not be tracked with a simple\n> >>>>> running flag. We can also be in the state 'Stopping' where we have not\n> >>>>> yet completed all events, but we must not commence anything new.\n> >>>>>\n> >>>>> Refactor the running_ boolean into a stateful enum to track this.\n> >>>>\n> >>>> Ooh, good idea!>>\n> >>>>>\n> >>>>> Signed-off-by: Kieran Bingham \n> >>>>> ---\n> >>>>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++----\n> >>>>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++-\n> >>>>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++--\n> >>>>> 3 files changed, 16 insertions(+), 7 deletions(-)\n> >>>>>\n> >>>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>>> index e3b541db4e36..dd0f4d3f64b6 100644\n> >>>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>>> @@ -45,7 +45,7 @@ namespace {{ns}} {\n> >>>>> {%- endif %}\n> >>>>> \n> >>>>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> >>>>> -\t: IPAProxy(ipam), running_(false),\n> >>>>> +\t: IPAProxy(ipam), state_(Stopped),\n> >>>>> \t isolate_(isolate), seq_(0)\n> >>>>> {\n> >>>>> \tLOG(IPAProxy, Debug)\n> >>>>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>>> \n> >>>>> \treturn {{ \"_ret\" if method|method_return_value != \"void\" }};\n> >>>>> {%- elif method.mojom_name == \"start\" %}\n> >>>>> -\trunning_ = true;\n> >>>>> +\tstate_ = Running;\n> >>>>> \tthread_.start();\n> >>>>> \n> >>>>> \t{{ \"return \" if method|method_return_value != \"void\" -}}\n> >>>>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>>> \t{%- endfor -%}\n> >>>>> );\n> >>>>> {% elif method|is_async %}\n> >>>>> -\tASSERT(running_);\n> >>>>> +\tASSERT(state_ == Running);\n> >>>>> \tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> >>>>> \t{%- for param in method|method_param_names -%}\n> >>>>> \t\t{{param}}{{- \", \" if not loop.last}}\n> >>>>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>>> {% for method in interface_event.methods %}\n> >>>>> {{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >>>>> {\n> >>>>> -\tASSERT(running_);\n> >>>>> +\tASSERT(state_ != Stopped);\n> >>>>> \t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >>>>> }\n> >>>>\n> >>>> As mentioned on irc, I think we need to copy these over to the IPC\n> >>>> versions too, but that can be done on top.\n> >>\n> >> I'm not sure I understand this here ...\n> > \n> > Paul meant that asserts in the IPC case would be useful.\n> \n> I can *read* the sentence ... but not interpret the location that needs\n> updating ;-)\n\nSorry :-)\n\n> Can you provide additional context please?\n> \n> Otherwise I'll leave this out, and let someone tackle it on top.\n\nIt can go on top, no issue. It should actually go on top, as I expect we\nmay need a different state machine for the IPC case, given that we need\nto keep processing messages while waiting for the reply to sync calls,\nand that will be a larger development.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8AC85C32F0\n\tfor ;\n\tMon, 29 Mar 2021 17:27:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6C8268784;\n\tMon, 29 Mar 2021 19:27:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AC756877D\n\tfor ;\n\tMon, 29 Mar 2021 19:27:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95735503;\n\tMon, 29 Mar 2021 19:27:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j/4jdoB8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617038853;\n\tbh=XusSpszxABzyj3CnnKXa3ICF1+q4udMGwXlltM0Ozgk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j/4jdoB8v9YFlhC4le8WiT6ZDWlgMU1a4Ti1n0czdjYWIvWzbBgtXzDTfc9b/SKyO\n\tvxO18ngzx6DD9pxnfVF/srsHJKeHIc1Zo41ahEZU4aQhIxIlJcdu9VzivxJzt73Dvt\n\t7YZ/A3cRp8lE6BO1JnO8+O2OG98Z3s3N3hyzC0kE=","Date":"Mon, 29 Mar 2021 20:26:49 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210325134231.1400051-1-kieran.bingham@ideasonboard.com>\n\t<20210325134231.1400051-4-kieran.bingham@ideasonboard.com>\n\t<20210326025010.GC2040@pyrite.rasen.tech>\n\t\n\t<01398c71-f703-a6c8-1dce-84213b9a73f0@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 01/11] utils: ipc: proxy: Track IPA\n\twith a state machine","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]