[2/4] libcamera: Use disconnect signal in ipa proxy worker
diff mbox series

Message ID 20241018075942.1150378-3-chenghaoyang@chromium.org
State New
Headers show
Series
  • IPC disconnect signals
Related show

Commit Message

Cheng-Hao Yang Oct. 18, 2024, 7:57 a.m. UTC 
Previously the worker might not receive the disconnect signal, and the
forked process might stay alive forever.

This CL also helps DMA buf recycling, as the algo process might hold DMA
buf file descriptors.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
---
 .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl     | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Kieran Bingham Oct. 19, 2024, 10:45 p.m. UTC | #1 
Quoting Harvey Yang (2024-10-18 08:57:35)
> Previously the worker might not receive the disconnect signal, and the
> forked process might stay alive forever.
> 
> This CL also helps DMA buf recycling, as the algo process might hold DMA
> buf file descriptors.

Should this just be part of the previous patch? Or is it somehow
distinct?

> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> ---
>  .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl     | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> index 1f990d3f9..68d68c4a5 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> @@ -60,6 +60,10 @@ public:
>  
>         ~{{proxy_worker_name}}() {}
>  
> +       void disconnected() {
> +               exit_ = true;
> +       }
> +
>         void readyRead()
>         {
>                 IPCUnixSocket::Payload _message;
> @@ -131,6 +135,7 @@ public:
>                         return EXIT_FAILURE;
>                 }
>                 socket_.readyRead.connect(this, &{{proxy_worker_name}}::readyRead);
> +               socket_.disconnected.connect(this, &{{proxy_worker_name}}::disconnected);
>  
>                 ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
>                 if (!ipa_) {
> -- 
> 2.47.0.rc1.288.g06298d1525-goog
>
Cheng-Hao Yang Oct. 20, 2024, 4:35 a.m. UTC | #2 
Hi Kieran,

On Sun, Oct 20, 2024 at 6:45 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-10-18 08:57:35)
> > Previously the worker might not receive the disconnect signal, and the
> > forked process might stay alive forever.
> >
> > This CL also helps DMA buf recycling, as the algo process might hold DMA
> > buf file descriptors.
>
> Should this just be part of the previous patch? Or is it somehow
> distinct?

Yes, I'm also fine with merging them. Will do in the next version.

BR,
Harvey

>
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > ---
> >  .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl     | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > index 1f990d3f9..68d68c4a5 100644
> > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > @@ -60,6 +60,10 @@ public:
> >
> >         ~{{proxy_worker_name}}() {}
> >
> > +       void disconnected() {
> > +               exit_ = true;
> > +       }
> > +
> >         void readyRead()
> >         {
> >                 IPCUnixSocket::Payload _message;
> > @@ -131,6 +135,7 @@ public:
> >                         return EXIT_FAILURE;
> >                 }
> >                 socket_.readyRead.connect(this, &{{proxy_worker_name}}::readyRead);
> > +               socket_.disconnected.connect(this, &{{proxy_worker_name}}::disconnected);
> >
> >                 ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
> >                 if (!ipa_) {
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >
Laurent Pinchart Oct. 20, 2024, 4:48 p.m. UTC | #3 
On Sun, Oct 20, 2024 at 12:35:57PM +0800, Cheng-Hao Yang wrote:
> On Sun, Oct 20, 2024 at 6:45 AM Kieran Bingham wrote:
> > Quoting Harvey Yang (2024-10-18 08:57:35)
> > > Previously the worker might not receive the disconnect signal, and the
> > > forked process might stay alive forever.
> > >
> > > This CL also helps DMA buf recycling, as the algo process might hold DMA
> > > buf file descriptors.
> >
> > Should this just be part of the previous patch? Or is it somehow
> > distinct?
> 
> Yes, I'm also fine with merging them. Will do in the next version.

Please detail in the commit message both the expected behaviour on the
libcamera side and on the proxy worker side.

> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > ---
> > >  .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl     | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > index 1f990d3f9..68d68c4a5 100644
> > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > @@ -60,6 +60,10 @@ public:
> > >
> > >         ~{{proxy_worker_name}}() {}
> > >
> > > +       void disconnected() {
> > > +               exit_ = true;
> > > +       }

	void disconnected()
	{
		exit_ = true;
	}

> > > +
> > >         void readyRead()
> > >         {
> > >                 IPCUnixSocket::Payload _message;
> > > @@ -131,6 +135,7 @@ public:
> > >                         return EXIT_FAILURE;
> > >                 }
> > >                 socket_.readyRead.connect(this, &{{proxy_worker_name}}::readyRead);
> > > +               socket_.disconnected.connect(this, &{{proxy_worker_name}}::disconnected);
> > >
> > >                 ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
> > >                 if (!ipa_) {
Cheng-Hao Yang Oct. 23, 2024, 6:51 a.m. UTC | #4 
Hi Laurent,

On Mon, Oct 21, 2024 at 12:49 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Sun, Oct 20, 2024 at 12:35:57PM +0800, Cheng-Hao Yang wrote:
> > On Sun, Oct 20, 2024 at 6:45 AM Kieran Bingham wrote:
> > > Quoting Harvey Yang (2024-10-18 08:57:35)
> > > > Previously the worker might not receive the disconnect signal, and the
> > > > forked process might stay alive forever.
> > > >
> > > > This CL also helps DMA buf recycling, as the algo process might hold DMA
> > > > buf file descriptors.
> > >
> > > Should this just be part of the previous patch? Or is it somehow
> > > distinct?
> >
> > Yes, I'm also fine with merging them. Will do in the next version.
>
> Please detail in the commit message both the expected behaviour on the
> libcamera side and on the proxy worker side.

Updated.

>
> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > ---
> > > >  .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl     | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > > index 1f990d3f9..68d68c4a5 100644
> > > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > > @@ -60,6 +60,10 @@ public:
> > > >
> > > >         ~{{proxy_worker_name}}() {}
> > > >
> > > > +       void disconnected() {
> > > > +               exit_ = true;
> > > > +       }
>
>         void disconnected()
>         {
>                 exit_ = true;
>         }
>

Thanks, will be updated in the next version.

BR,
Harvey

> > > > +
> > > >         void readyRead()
> > > >         {
> > > >                 IPCUnixSocket::Payload _message;
> > > > @@ -131,6 +135,7 @@ public:
> > > >                         return EXIT_FAILURE;
> > > >                 }
> > > >                 socket_.readyRead.connect(this, &{{proxy_worker_name}}::readyRead);
> > > > +               socket_.disconnected.connect(this, &{{proxy_worker_name}}::disconnected);
> > > >
> > > >                 ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
> > > >                 if (!ipa_) {
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
index 1f990d3f9..68d68c4a5 100644
--- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
+++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
@@ -60,6 +60,10 @@  public:
 
 	~{{proxy_worker_name}}() {}
 
+	void disconnected() {
+		exit_ = true;
+	}
+
 	void readyRead()
 	{
 		IPCUnixSocket::Payload _message;
@@ -131,6 +135,7 @@  public:
 			return EXIT_FAILURE;
 		}
 		socket_.readyRead.connect(this, &{{proxy_worker_name}}::readyRead);
+		socket_.disconnected.connect(this, &{{proxy_worker_name}}::disconnected);
 
 		ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
 		if (!ipa_) {