| Message ID | 20230504144801.2590668-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > Ensure that when we iterate the libcamera libdir we only select shared > objects which are expected to be IPA modules, with an ipa_ prefix. > > Any shared object not matching this filter is ignored and not processed > by the IPA Manager. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I wonder what this protects against, as there shouldn't be other files in this directory, especially if we move IPA modules to an ipa/ subdirectory. I don't mind much, as this isn't a hot path, so I have no objection if you want to merge this, but is it useful ? > --- > src/libcamera/ipa_manager.cpp | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index 030ef43fb994..c0c2f027e902 100644 > --- a/src/libcamera/ipa_manager.cpp > +++ b/src/libcamera/ipa_manager.cpp > @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth, > if (strcmp(&ent->d_name[offset], ".so")) > continue; > > + /* Ignore any modules which are not IPAs. */ > + if (strncmp(ent->d_name, "ipa_", 4) != 0) > + continue; > + > files.push_back(std::string(libDir) + "/" + ent->d_name); > } >
On 04/05/2023 16:10, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: >> Ensure that when we iterate the libcamera libdir we only select shared >> objects which are expected to be IPA modules, with an ipa_ prefix. >> >> Any shared object not matching this filter is ignored and not processed >> by the IPA Manager. >> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > I wonder what this protects against, as there shouldn't be other files > in this directory, especially if we move IPA modules to an ipa/ > subdirectory. I don't mind much, as this isn't a hot path, so I have no > objection if you want to merge this, but is it useful ? Only that it prevents us trying to load any files that might be there otherwise. [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 fake.so: IPA module is not an ELF file [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info We already filter on .so objects, and we do other checks so it's not crucial, and it's not going to prevent any attempt to get 'non-libcamera' code parsed as anyone dropping a file here could equally call it 'ipa_fake.so' ... so no not really. I'll drop this and collect 2/2 -- Kieran > >> --- >> src/libcamera/ipa_manager.cpp | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp >> index 030ef43fb994..c0c2f027e902 100644 >> --- a/src/libcamera/ipa_manager.cpp >> +++ b/src/libcamera/ipa_manager.cpp >> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth, >> if (strcmp(&ent->d_name[offset], ".so")) >> continue; >> >> + /* Ignore any modules which are not IPAs. */ >> + if (strncmp(ent->d_name, "ipa_", 4) != 0) >> + continue; >> + >> files.push_back(std::string(libDir) + "/" + ent->d_name); >> } >> >
Quoting Kieran Bingham (2023-05-04 20:03:56) > On 04/05/2023 16:10, Laurent Pinchart wrote: > > Hi Kieran, > > > > Thank you for the patch. > > > > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > >> Ensure that when we iterate the libcamera libdir we only select shared > >> objects which are expected to be IPA modules, with an ipa_ prefix. > >> > >> Any shared object not matching this filter is ignored and not processed > >> by the IPA Manager. > >> > >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > I wonder what this protects against, as there shouldn't be other files > > in this directory, especially if we move IPA modules to an ipa/ > > subdirectory. I don't mind much, as this isn't a hot path, so I have no > > objection if you want to merge this, but is it useful ? > > Only that it prevents us trying to load any files that might be there > otherwise. > > > > [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 > fake.so: IPA module is not an ELF file > [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol > ipaModuleInfo not found > [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 > v4l2-compat.so: IPA module has no valid info > > > We already filter on .so objects, and we do other checks so it's not > crucial, and it's not going to prevent any attempt to get > 'non-libcamera' code parsed as anyone dropping a file here could equally > call it 'ipa_fake.so' ... so no not really. > > I'll drop this and collect 2/2 I've started seeing various logs that report similar to: ``` $ wireplumber [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 ``` I believe this is because some distributions are probably overriding the installation path of the v4l2-compat.so. Anyway, that makes me think that we /should/ ignore files that are not prefixed by ipa_ ... -- Kieran > > -- > Kieran > > > > > > >> --- > >> src/libcamera/ipa_manager.cpp | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > >> index 030ef43fb994..c0c2f027e902 100644 > >> --- a/src/libcamera/ipa_manager.cpp > >> +++ b/src/libcamera/ipa_manager.cpp > >> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth, > >> if (strcmp(&ent->d_name[offset], ".so")) > >> continue; > >> > >> + /* Ignore any modules which are not IPAs. */ > >> + if (strncmp(ent->d_name, "ipa_", 4) != 0) > >> + continue; > >> + > >> files.push_back(std::string(libDir) + "/" + ent->d_name); > >> } > >> > >
On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote: > Quoting Kieran Bingham (2023-05-04 20:03:56) > > On 04/05/2023 16:10, Laurent Pinchart wrote: > > > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > > >> Ensure that when we iterate the libcamera libdir we only select shared > > >> objects which are expected to be IPA modules, with an ipa_ prefix. > > >> > > >> Any shared object not matching this filter is ignored and not processed > > >> by the IPA Manager. > > >> > > >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > I wonder what this protects against, as there shouldn't be other files > > > in this directory, especially if we move IPA modules to an ipa/ > > > subdirectory. I don't mind much, as this isn't a hot path, so I have no > > > objection if you want to merge this, but is it useful ? > > > > Only that it prevents us trying to load any files that might be there > > otherwise. > > > > > > [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 > > fake.so: IPA module is not an ELF file > > [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol > > ipaModuleInfo not found > > [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 > > v4l2-compat.so: IPA module has no valid info > > > > > > We already filter on .so objects, and we do other checks so it's not > > crucial, and it's not going to prevent any attempt to get > > 'non-libcamera' code parsed as anyone dropping a file here could equally > > call it 'ipa_fake.so' ... so no not really. > > > > I'll drop this and collect 2/2 > > I've started seeing various logs that report similar to: > ``` > $ wireplumber > [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found > [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info > [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 > ``` > > I believe this is because some distributions are probably overriding the > installation path of the v4l2-compat.so. > > Anyway, that makes me think that we /should/ ignore files that are not > prefixed by ipa_ ... Or move IPA modules to an ipa/ subdirectory ? > > >> --- > > >> src/libcamera/ipa_manager.cpp | 4 ++++ > > >> 1 file changed, 4 insertions(+) > > >> > > >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > >> index 030ef43fb994..c0c2f027e902 100644 > > >> --- a/src/libcamera/ipa_manager.cpp > > >> +++ b/src/libcamera/ipa_manager.cpp > > >> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth, > > >> if (strcmp(&ent->d_name[offset], ".so")) > > >> continue; > > >> > > >> + /* Ignore any modules which are not IPAs. */ > > >> + if (strncmp(ent->d_name, "ipa_", 4) != 0) > > >> + continue; > > >> + > > >> files.push_back(std::string(libDir) + "/" + ent->d_name); > > >> } > > >>
Quoting Laurent Pinchart (2023-08-28 20:13:13) > On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote: > > Quoting Kieran Bingham (2023-05-04 20:03:56) > > > On 04/05/2023 16:10, Laurent Pinchart wrote: > > > > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > > > >> Ensure that when we iterate the libcamera libdir we only select shared > > > >> objects which are expected to be IPA modules, with an ipa_ prefix. > > > >> > > > >> Any shared object not matching this filter is ignored and not processed > > > >> by the IPA Manager. > > > >> > > > >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > I wonder what this protects against, as there shouldn't be other files > > > > in this directory, especially if we move IPA modules to an ipa/ > > > > subdirectory. I don't mind much, as this isn't a hot path, so I have no > > > > objection if you want to merge this, but is it useful ? > > > > > > Only that it prevents us trying to load any files that might be there > > > otherwise. > > > > > > > > > [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 > > > fake.so: IPA module is not an ELF file > > > [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol > > > ipaModuleInfo not found > > > [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 > > > v4l2-compat.so: IPA module has no valid info > > > > > > > > > We already filter on .so objects, and we do other checks so it's not > > > crucial, and it's not going to prevent any attempt to get > > > 'non-libcamera' code parsed as anyone dropping a file here could equally > > > call it 'ipa_fake.so' ... so no not really. > > > > > > I'll drop this and collect 2/2 > > > > I've started seeing various logs that report similar to: > > ``` > > $ wireplumber > > [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found > > [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info > > [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 > > ``` > > > > I believe this is because some distributions are probably overriding the > > installation path of the v4l2-compat.so. > > > > Anyway, that makes me think that we /should/ ignore files that are not > > prefixed by ipa_ ... > > Or move IPA modules to an ipa/ subdirectory ? And I've just seen this in some logs from a raspberry pi issue: ``` (AGS_311_venv) @AGS-RPI-0004:~$ libcamera-hello [0:01:23.242753930] [811] ERROR IPAModule ipa_module.cpp:171 Symbol ipaModuleInfo not found [0:01:23.242802797] [811] ERROR IPAModule ipa_module.cpp:291 v4l2-compat.so: IPA module has no valid info [0:01:23.242827435] [811] INFO Camera camera_manager.cpp:327 libcamera v0.4.0+51-ca36c77f ``` So I think it's time to move the IPA modules to a subdirectory. I like that better than arbitrary filtering of the name. -- Kieran > > > > >> --- > > > >> src/libcamera/ipa_manager.cpp | 4 ++++ > > > >> 1 file changed, 4 insertions(+) > > > >> > > > >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > > >> index 030ef43fb994..c0c2f027e902 100644 > > > >> --- a/src/libcamera/ipa_manager.cpp > > > >> +++ b/src/libcamera/ipa_manager.cpp > > > >> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth, > > > >> if (strcmp(&ent->d_name[offset], ".so")) > > > >> continue; > > > >> > > > >> + /* Ignore any modules which are not IPAs. */ > > > >> + if (strncmp(ent->d_name, "ipa_", 4) != 0) > > > >> + continue; > > > >> + > > > >> files.push_back(std::string(libDir) + "/" + ent->d_name); > > > >> } > > > >> > > -- > Regards, > > Laurent Pinchart
On Wed, Feb 12, 2025 at 09:42:07AM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart (2023-08-28 20:13:13) > > On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote: > > > Quoting Kieran Bingham (2023-05-04 20:03:56) > > > > On 04/05/2023 16:10, Laurent Pinchart wrote: > > > > > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > > > > >> Ensure that when we iterate the libcamera libdir we only select shared > > > > >> objects which are expected to be IPA modules, with an ipa_ prefix. > > > > >> > > > > >> Any shared object not matching this filter is ignored and not processed > > > > >> by the IPA Manager. > > > > >> > > > > >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > I wonder what this protects against, as there shouldn't be other files > > > > > in this directory, especially if we move IPA modules to an ipa/ > > > > > subdirectory. I don't mind much, as this isn't a hot path, so I have no > > > > > objection if you want to merge this, but is it useful ? > > > > > > > > Only that it prevents us trying to load any files that might be there > > > > otherwise. > > > > > > > > > > > > [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 > > > > fake.so: IPA module is not an ELF file > > > > [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol > > > > ipaModuleInfo not found > > > > [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 > > > > v4l2-compat.so: IPA module has no valid info > > > > > > > > > > > > We already filter on .so objects, and we do other checks so it's not > > > > crucial, and it's not going to prevent any attempt to get > > > > 'non-libcamera' code parsed as anyone dropping a file here could equally > > > > call it 'ipa_fake.so' ... so no not really. > > > > > > > > I'll drop this and collect 2/2 > > > > > > I've started seeing various logs that report similar to: > > > ``` > > > $ wireplumber > > > [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found > > > [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info > > > [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 > > > ``` > > > > > > I believe this is because some distributions are probably overriding the > > > installation path of the v4l2-compat.so. > > > > > > Anyway, that makes me think that we /should/ ignore files that are not > > > prefixed by ipa_ ... > > > > Or move IPA modules to an ipa/ subdirectory ? > > And I've just seen this in some logs from a raspberry pi issue: > > ``` > (AGS_311_venv) @AGS-RPI-0004:~$ libcamera-hello > [0:01:23.242753930] [811] ERROR IPAModule ipa_module.cpp:171 Symbol ipaModuleInfo not found > [0:01:23.242802797] [811] ERROR IPAModule ipa_module.cpp:291 v4l2-compat.so: IPA module has > no valid info > [0:01:23.242827435] [811] INFO Camera camera_manager.cpp:327 libcamera v0.4.0+51-ca36c77f > ``` > > So I think it's time to move the IPA modules to a subdirectory. I like > that better than arbitrary filtering of the name. It's really a distribution issue, by default IPA modules go to (on an Arm64 system) /usr/local/lib64 (and only IPA modules are installed there), and v4l2-compat.so go to /usr/local/libexec/libcamera/. We can create a subdirectory for IPA modules, but will distributions then decide to install v4l2-compat.so there ? :-) > > > > >> --- > > > > >> src/libcamera/ipa_manager.cpp | 4 ++++ > > > > >> 1 file changed, 4 insertions(+) > > > > >> > > > > >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > > > >> index 030ef43fb994..c0c2f027e902 100644 > > > > >> --- a/src/libcamera/ipa_manager.cpp > > > > >> +++ b/src/libcamera/ipa_manager.cpp > > > > >> @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth, > > > > >> if (strcmp(&ent->d_name[offset], ".so")) > > > > >> continue; > > > > >> > > > > >> + /* Ignore any modules which are not IPAs. */ > > > > >> + if (strncmp(ent->d_name, "ipa_", 4) != 0) > > > > >> + continue; > > > > >> + > > > > >> files.push_back(std::string(libDir) + "/" + ent->d_name); > > > > >> } > > > > >>
Hi Dylan, Javier, I've got some questions for you both at the bottom of this mail regarding libcamera packaging. Quoting Laurent Pinchart (2025-02-13 17:01:26) > On Wed, Feb 12, 2025 at 09:42:07AM +0000, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2023-08-28 20:13:13) > > > On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote: > > > > Quoting Kieran Bingham (2023-05-04 20:03:56) > > > > > On 04/05/2023 16:10, Laurent Pinchart wrote: > > > > > > On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > > > > > >> Ensure that when we iterate the libcamera libdir we only select shared > > > > > >> objects which are expected to be IPA modules, with an ipa_ prefix. > > > > > >> > > > > > >> Any shared object not matching this filter is ignored and not processed > > > > > >> by the IPA Manager. > > > > > >> > > > > > >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > > > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > I wonder what this protects against, as there shouldn't be other files > > > > > > in this directory, especially if we move IPA modules to an ipa/ > > > > > > subdirectory. I don't mind much, as this isn't a hot path, so I have no > > > > > > objection if you want to merge this, but is it useful ? > > > > > > > > > > Only that it prevents us trying to load any files that might be there > > > > > otherwise. > > > > > > > > > > > > > > > [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 > > > > > fake.so: IPA module is not an ELF file > > > > > [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol > > > > > ipaModuleInfo not found > > > > > [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 > > > > > v4l2-compat.so: IPA module has no valid info > > > > > > > > > > > > > > > We already filter on .so objects, and we do other checks so it's not > > > > > crucial, and it's not going to prevent any attempt to get > > > > > 'non-libcamera' code parsed as anyone dropping a file here could equally > > > > > call it 'ipa_fake.so' ... so no not really. > > > > > > > > > > I'll drop this and collect 2/2 > > > > > > > > I've started seeing various logs that report similar to: > > > > ``` > > > > $ wireplumber > > > > [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found > > > > [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info > > > > [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 > > > > ``` > > > > > > > > I believe this is because some distributions are probably overriding the > > > > installation path of the v4l2-compat.so. > > > > > > > > Anyway, that makes me think that we /should/ ignore files that are not > > > > prefixed by ipa_ ... > > > > > > Or move IPA modules to an ipa/ subdirectory ? > > > > And I've just seen this in some logs from a raspberry pi issue: > > > > ``` > > (AGS_311_venv) @AGS-RPI-0004:~$ libcamera-hello > > [0:01:23.242753930] [811] ERROR IPAModule ipa_module.cpp:171 Symbol ipaModuleInfo not found > > [0:01:23.242802797] [811] ERROR IPAModule ipa_module.cpp:291 v4l2-compat.so: IPA module has > > no valid info > > [0:01:23.242827435] [811] INFO Camera camera_manager.cpp:327 libcamera v0.4.0+51-ca36c77f > > ``` > > > > So I think it's time to move the IPA modules to a subdirectory. I like > > that better than arbitrary filtering of the name. > > It's really a distribution issue, by default IPA modules go to (on an > Arm64 system) /usr/local/lib64 (and only IPA modules are installed > there), and v4l2-compat.so go to /usr/local/libexec/libcamera/. We can > create a subdirectory for IPA modules, but will distributions then > decide to install v4l2-compat.so there ? :-) A distribution issue that's impacting multiple distributions ... So the question is /why/ ? https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-ipa.install?ref_type=heads shows: usr/lib/*/libcamera/ipa_*.so usr/lib/*/libcamera/ipa_*.so.sign usr/lib/*/libcamera/*_ipa_proxy usr/share/libcamera so it's capturing the ipa files into a package based on where they get installed. https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-v4l2.install?ref_type=heads shows usr/lib/*/libcamera/v4l2-compat.so for the V4L2 side. And finally: https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/rules?ref_type=heads Shows: override_dh_auto_configure: dh_auto_configure -- \ --libexecdir=lib/${DEB_HOST_MULTIARCH} \ Which is overridding the libexecdir for multiarch. Trying to search for debian guidance on this I hit: https://linux.debian.devel.mentors.narkive.com/hkilz1si/using-usr-libexec-directory which is a mentor on the debian lists giving the following guidance: > Move the files to use "usr/lib" instead. Nothing on debian uses libexec. > > Usually there is a configure option one can pass, e.g. --libexecdir=/usr/lib and https://lists.debian.org/debian-devel/2005/05/msg00404.html > > Should we change some of these to /usr/libexec? > > well, it would be against the FHS, I think. > > The BSDs use libexec but I don't really see a good reason why it exists. > And this one: https://lists.debian.org/debian-devel/2005/05/msg00565.html > The BSDs use libexec but I don't really see a good reason why it exists. > > The only reason we don't have it is because of petty bickering and > politics between the FHS folks (several years ago). There were few > technical objections to it on the FHS list, but it was dropped for > non-technical reasons. Given that the FHS is supposed to codify > existing practice, it should be in there on that count alone. Every > libexec-using package in Debian has been reconfigured not to use it; > upstreams do use it, and I'd like to use it myself. > > I'd personally be very glad to have it, and would support using it in > Debian. But so far I haven't found any documentation to 'specify' this. Dylan, Do you know /why/ the v4l2-compat.so is moved out of libexec and into lib/libcamera ? (Noting that it causes a spurious error to be reported to users who have libcamera-v4l2 installed). I know this same issue is occuring on Alpine and Arch systems too. So it seems like "all the major distributions" are doing roughly the same thing here. Javier, Do you know if this is happening on Fedora or if there is any requirements regarding /usr/libexec ? -- Kieran
Hi Kieran, On 2/13/25 18:36, Kieran Bingham wrote: > Hi Dylan, Javier, > > I've got some questions for you both at the bottom of this mail > regarding libcamera packaging. > > Quoting Laurent Pinchart (2025-02-13 17:01:26) >> On Wed, Feb 12, 2025 at 09:42:07AM +0000, Kieran Bingham wrote: >>> Quoting Laurent Pinchart (2023-08-28 20:13:13) >>>> On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote: >>>>> Quoting Kieran Bingham (2023-05-04 20:03:56) >>>>>> On 04/05/2023 16:10, Laurent Pinchart wrote: >>>>>>> On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: >>>>>>>> Ensure that when we iterate the libcamera libdir we only select shared >>>>>>>> objects which are expected to be IPA modules, with an ipa_ prefix. >>>>>>>> >>>>>>>> Any shared object not matching this filter is ignored and not processed >>>>>>>> by the IPA Manager. >>>>>>>> >>>>>>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >>>>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>>>> >>>>>>> I wonder what this protects against, as there shouldn't be other files >>>>>>> in this directory, especially if we move IPA modules to an ipa/ >>>>>>> subdirectory. I don't mind much, as this isn't a hot path, so I have no >>>>>>> objection if you want to merge this, but is it useful ? >>>>>> >>>>>> Only that it prevents us trying to load any files that might be there >>>>>> otherwise. >>>>>> >>>>>> >>>>>> [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 >>>>>> fake.so: IPA module is not an ELF file >>>>>> [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol >>>>>> ipaModuleInfo not found >>>>>> [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 >>>>>> v4l2-compat.so: IPA module has no valid info >>>>>> >>>>>> >>>>>> We already filter on .so objects, and we do other checks so it's not >>>>>> crucial, and it's not going to prevent any attempt to get >>>>>> 'non-libcamera' code parsed as anyone dropping a file here could equally >>>>>> call it 'ipa_fake.so' ... so no not really. >>>>>> >>>>>> I'll drop this and collect 2/2 >>>>> >>>>> I've started seeing various logs that report similar to: >>>>> ``` >>>>> $ wireplumber >>>>> [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found >>>>> [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info >>>>> [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 >>>>> ``` >>>>> >>>>> I believe this is because some distributions are probably overriding the >>>>> installation path of the v4l2-compat.so. >>>>> >>>>> Anyway, that makes me think that we /should/ ignore files that are not >>>>> prefixed by ipa_ ... >>>> >>>> Or move IPA modules to an ipa/ subdirectory ? >>> >>> And I've just seen this in some logs from a raspberry pi issue: >>> >>> ``` >>> (AGS_311_venv) @AGS-RPI-0004:~$ libcamera-hello >>> [0:01:23.242753930] [811] ERROR IPAModule ipa_module.cpp:171 Symbol ipaModuleInfo not found >>> [0:01:23.242802797] [811] ERROR IPAModule ipa_module.cpp:291 v4l2-compat.so: IPA module has >>> no valid info >>> [0:01:23.242827435] [811] INFO Camera camera_manager.cpp:327 libcamera v0.4.0+51-ca36c77f >>> ``` >>> >>> So I think it's time to move the IPA modules to a subdirectory. I like >>> that better than arbitrary filtering of the name. >> >> It's really a distribution issue, by default IPA modules go to (on an >> Arm64 system) /usr/local/lib64 (and only IPA modules are installed >> there), and v4l2-compat.so go to /usr/local/libexec/libcamera/. We can >> create a subdirectory for IPA modules, but will distributions then >> decide to install v4l2-compat.so there ? :-) > > A distribution issue that's impacting multiple distributions ... > > So the question is /why/ ? > > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-ipa.install?ref_type=heads > shows: > > > usr/lib/*/libcamera/ipa_*.so > usr/lib/*/libcamera/ipa_*.so.sign > usr/lib/*/libcamera/*_ipa_proxy > usr/share/libcamera > > so it's capturing the ipa files into a package based on where they get > installed. > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-v4l2.install?ref_type=heads > > shows > > usr/lib/*/libcamera/v4l2-compat.so > > for the V4L2 side. > > > And finally: > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/rules?ref_type=heads > > Shows: > > override_dh_auto_configure: > dh_auto_configure -- \ > --libexecdir=lib/${DEB_HOST_MULTIARCH} \ > > > Which is overridding the libexecdir for multiarch. > > > Trying to search for debian guidance on this I hit: > > https://linux.debian.devel.mentors.narkive.com/hkilz1si/using-usr-libexec-directory > which is a mentor on the debian lists giving the following guidance: > >> Move the files to use "usr/lib" instead. Nothing on debian uses libexec. >> >> Usually there is a configure option one can pass, e.g. --libexecdir=/usr/lib > > and > https://lists.debian.org/debian-devel/2005/05/msg00404.html >>> Should we change some of these to /usr/libexec? >> >> well, it would be against the FHS, I think. >> >> The BSDs use libexec but I don't really see a good reason why it exists. >> > > And this one: > https://lists.debian.org/debian-devel/2005/05/msg00565.html >> The BSDs use libexec but I don't really see a good reason why it exists. >> >> The only reason we don't have it is because of petty bickering and >> politics between the FHS folks (several years ago). There were few >> technical objections to it on the FHS list, but it was dropped for >> non-technical reasons. Given that the FHS is supposed to codify >> existing practice, it should be in there on that count alone. Every >> libexec-using package in Debian has been reconfigured not to use it; >> upstreams do use it, and I'd like to use it myself. >> >> I'd personally be very glad to have it, and would support using it in >> Debian. > > > But so far I haven't found any documentation to 'specify' this. > > Dylan, Do you know /why/ the v4l2-compat.so is moved out of libexec and > into lib/libcamera ? (Noting that it causes a spurious error to be > reported to users who have libcamera-v4l2 installed). I think the answer is just "bad sequence of events" and there is no real reason. Let's do some git archeology to understand what happened: [1] In 2020, libexecdir was overridden to make the package multiarch compatible. [2] ~ 10 days later, v4l2 compat was enabled, so with libexecdir already overridden. [3] In 2022, I moved v4l2 into its own binary package, but unrelated with our current issue. [4] In 2023, v4l2 was moved upstream from libexec to libexec/libcamera. [5] This was reflected in the debian package but with the override of libexecdir, v4l2 has landed into usr/lib/*/libcamera/ which is the same directory than ipa modules. I don't see a good reason other than trying to make v4l2 and *_ipa_proxy multiarch co-installable. Now the question is, does this make sense? I don't know. I can drop the libexecdir override, both v4l2 and *_ipa_proxy will go back in usr/libexec/libcamera/ and both binary packages libcamera-v4l2 and libcamera-ipa will no longer be multiarch co-installable, but I am not sure there was a real use case anyway. [1] https://salsa.debian.org/multimedia-team/libcamera/-/commit/04fda66ee27bfacd07a39acb86bbaba376bcc050 [2] https://salsa.debian.org/multimedia-team/libcamera/-/commit/89ab628d6d9605547423c7696f4dbc8b10ce7708 [3] https://salsa.debian.org/multimedia-team/libcamera/-/commit/7bfebf853bb18004ed26c822a0b8a1cb3fb1f0d7 [4] https://gitlab.freedesktop.org/camera/libcamera/-/commit/1c512d406536d72a393c38c3f6a75fe0fdb9ecb2 [5] https://salsa.debian.org/multimedia-team/libcamera/-/commit/a991999c4c935d150c04a88f401cf2dcf5314c71 > I know this same issue is occuring on Alpine and Arch systems too. So it > seems like "all the major distributions" are doing roughly the same > thing here. Maybe for the same reason, or maybe they just copied our mistakes. > Javier, Do you know if this is happening on Fedora or if there is any > requirements regarding /usr/libexec ? > > -- > Kieran Best regards, Dylan
Hi Dylan, Quoting Dylan Aissi (2025-02-13 22:17:45) > Hi Kieran, > > On 2/13/25 18:36, Kieran Bingham wrote: > > Hi Dylan, Javier, > > > > I've got some questions for you both at the bottom of this mail > > regarding libcamera packaging. > > > > Quoting Laurent Pinchart (2025-02-13 17:01:26) > >> On Wed, Feb 12, 2025 at 09:42:07AM +0000, Kieran Bingham wrote: > >>> Quoting Laurent Pinchart (2023-08-28 20:13:13) > >>>> On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote: > >>>>> Quoting Kieran Bingham (2023-05-04 20:03:56) > >>>>>> On 04/05/2023 16:10, Laurent Pinchart wrote: > >>>>>>> On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > >>>>>>>> Ensure that when we iterate the libcamera libdir we only select shared > >>>>>>>> objects which are expected to be IPA modules, with an ipa_ prefix. > >>>>>>>> > >>>>>>>> Any shared object not matching this filter is ignored and not processed > >>>>>>>> by the IPA Manager. > >>>>>>>> > >>>>>>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > >>>>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>>>> > >>>>>>> I wonder what this protects against, as there shouldn't be other files > >>>>>>> in this directory, especially if we move IPA modules to an ipa/ > >>>>>>> subdirectory. I don't mind much, as this isn't a hot path, so I have no > >>>>>>> objection if you want to merge this, but is it useful ? > >>>>>> > >>>>>> Only that it prevents us trying to load any files that might be there > >>>>>> otherwise. > >>>>>> > >>>>>> > >>>>>> [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 > >>>>>> fake.so: IPA module is not an ELF file > >>>>>> [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol > >>>>>> ipaModuleInfo not found > >>>>>> [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 > >>>>>> v4l2-compat.so: IPA module has no valid info > >>>>>> > >>>>>> > >>>>>> We already filter on .so objects, and we do other checks so it's not > >>>>>> crucial, and it's not going to prevent any attempt to get > >>>>>> 'non-libcamera' code parsed as anyone dropping a file here could equally > >>>>>> call it 'ipa_fake.so' ... so no not really. > >>>>>> > >>>>>> I'll drop this and collect 2/2 > >>>>> > >>>>> I've started seeing various logs that report similar to: > >>>>> ``` > >>>>> $ wireplumber > >>>>> [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found > >>>>> [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info > >>>>> [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 > >>>>> ``` > >>>>> > >>>>> I believe this is because some distributions are probably overriding the > >>>>> installation path of the v4l2-compat.so. > >>>>> > >>>>> Anyway, that makes me think that we /should/ ignore files that are not > >>>>> prefixed by ipa_ ... > >>>> > >>>> Or move IPA modules to an ipa/ subdirectory ? > >>> > >>> And I've just seen this in some logs from a raspberry pi issue: > >>> > >>> ``` > >>> (AGS_311_venv) @AGS-RPI-0004:~$ libcamera-hello > >>> [0:01:23.242753930] [811] ERROR IPAModule ipa_module.cpp:171 Symbol ipaModuleInfo not found > >>> [0:01:23.242802797] [811] ERROR IPAModule ipa_module.cpp:291 v4l2-compat.so: IPA module has > >>> no valid info > >>> [0:01:23.242827435] [811] INFO Camera camera_manager.cpp:327 libcamera v0.4.0+51-ca36c77f > >>> ``` > >>> > >>> So I think it's time to move the IPA modules to a subdirectory. I like > >>> that better than arbitrary filtering of the name. > >> > >> It's really a distribution issue, by default IPA modules go to (on an > >> Arm64 system) /usr/local/lib64 (and only IPA modules are installed > >> there), and v4l2-compat.so go to /usr/local/libexec/libcamera/. We can > >> create a subdirectory for IPA modules, but will distributions then > >> decide to install v4l2-compat.so there ? :-) > > > > A distribution issue that's impacting multiple distributions ... > > > > So the question is /why/ ? > > > > > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-ipa.install?ref_type=heads > > shows: > > > > > > usr/lib/*/libcamera/ipa_*.so > > usr/lib/*/libcamera/ipa_*.so.sign > > usr/lib/*/libcamera/*_ipa_proxy > > usr/share/libcamera > > > > so it's capturing the ipa files into a package based on where they get > > installed. > > > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-v4l2.install?ref_type=heads > > > > shows > > > > usr/lib/*/libcamera/v4l2-compat.so > > > > for the V4L2 side. > > > > > > And finally: > > > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/rules?ref_type=heads > > > > Shows: > > > > override_dh_auto_configure: > > dh_auto_configure -- \ > > --libexecdir=lib/${DEB_HOST_MULTIARCH} \ > > > > > > Which is overridding the libexecdir for multiarch. > > > > > > Trying to search for debian guidance on this I hit: > > > > https://linux.debian.devel.mentors.narkive.com/hkilz1si/using-usr-libexec-directory > > which is a mentor on the debian lists giving the following guidance: > > > >> Move the files to use "usr/lib" instead. Nothing on debian uses libexec. > >> > >> Usually there is a configure option one can pass, e.g. --libexecdir=/usr/lib > > > > and > > https://lists.debian.org/debian-devel/2005/05/msg00404.html > >>> Should we change some of these to /usr/libexec? > >> > >> well, it would be against the FHS, I think. > >> > >> The BSDs use libexec but I don't really see a good reason why it exists. > >> > > > > And this one: > > https://lists.debian.org/debian-devel/2005/05/msg00565.html > >> The BSDs use libexec but I don't really see a good reason why it exists. > >> > >> The only reason we don't have it is because of petty bickering and > >> politics between the FHS folks (several years ago). There were few > >> technical objections to it on the FHS list, but it was dropped for > >> non-technical reasons. Given that the FHS is supposed to codify > >> existing practice, it should be in there on that count alone. Every > >> libexec-using package in Debian has been reconfigured not to use it; > >> upstreams do use it, and I'd like to use it myself. > >> > >> I'd personally be very glad to have it, and would support using it in > >> Debian. > > > > > > But so far I haven't found any documentation to 'specify' this. > > > > Dylan, Do you know /why/ the v4l2-compat.so is moved out of libexec and > > into lib/libcamera ? (Noting that it causes a spurious error to be > > reported to users who have libcamera-v4l2 installed). > > I think the answer is just "bad sequence of events" and there is no real > reason. > Let's do some git archeology to understand what happened: > [1] In 2020, libexecdir was overridden to make the package multiarch > compatible. > [2] ~ 10 days later, v4l2 compat was enabled, so with libexecdir already > overridden. > [3] In 2022, I moved v4l2 into its own binary package, but unrelated > with our current issue. > [4] In 2023, v4l2 was moved upstream from libexec to libexec/libcamera. > [5] This was reflected in the debian package but with the override of > libexecdir, v4l2 has landed into usr/lib/*/libcamera/ which is the same > directory than ipa modules. Thank you - that is incredibly helpful! And now it's much clearer what was going on. > I don't see a good reason other than trying to make v4l2 and *_ipa_proxy > multiarch co-installable. > Now the question is, does this make sense? I don't know. I can drop the > libexecdir override, both v4l2 and *_ipa_proxy will go back in > usr/libexec/libcamera/ > and both binary packages libcamera-v4l2 and libcamera-ipa will no longer > be multiarch co-installable, but I am not sure there was a real use case > anyway. The tricky part here will likely be the IPA proxies. I think that's part of 'libcamera' but could be handled as part of libcamera-ipa indeed, and then that component shouldn't be multiarch - the package itself would be architecture specific. Same for the libcamera-v4l2. Having libcamera core multi-arch facilitates cross compiling which is great. libcamera-v4l2 and libcamera-ipa are not required for that. Are there other use-cases for having multi-arch libcamera? > > [1] > https://salsa.debian.org/multimedia-team/libcamera/-/commit/04fda66ee27bfacd07a39acb86bbaba376bcc050 > [2] > https://salsa.debian.org/multimedia-team/libcamera/-/commit/89ab628d6d9605547423c7696f4dbc8b10ce7708 > [3] > https://salsa.debian.org/multimedia-team/libcamera/-/commit/7bfebf853bb18004ed26c822a0b8a1cb3fb1f0d7 > [4] > https://gitlab.freedesktop.org/camera/libcamera/-/commit/1c512d406536d72a393c38c3f6a75fe0fdb9ecb2 > [5] > https://salsa.debian.org/multimedia-team/libcamera/-/commit/a991999c4c935d150c04a88f401cf2dcf5314c71 > > > > I know this same issue is occuring on Alpine and Arch systems too. So it > > seems like "all the major distributions" are doing roughly the same > > thing here. > > Maybe for the same reason, or maybe they just copied our mistakes. Indeed, I'll chase those distributions with the same rationale now that we understand it though. Thank you very much for helping get this clear. Will you make an update to the debian packages? -- Kieran > > Javier, Do you know if this is happening on Fedora or if there is any > > requirements regarding /usr/libexec ? > > > > -- > > Kieran > > Best regards, > Dylan
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: Hello Kieran, [...] >> > ``` >> > >> > So I think it's time to move the IPA modules to a subdirectory. I like >> > that better than arbitrary filtering of the name. >> >> It's really a distribution issue, by default IPA modules go to (on an >> Arm64 system) /usr/local/lib64 (and only IPA modules are installed >> there), and v4l2-compat.so go to /usr/local/libexec/libcamera/. We can >> create a subdirectory for IPA modules, but will distributions then >> decide to install v4l2-compat.so there ? :-) > > A distribution issue that's impacting multiple distributions ... > > So the question is /why/ ? > Maybe I'm misunderstanding what is the question but in Fedora at least most packages don't have multi{lib,arch} support. If the question is only about the IPA modules being moved to a ipa sub directory then I think that it makes sense and shouldn't be an issue. > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-ipa.install?ref_type=heads > shows: > > > usr/lib/*/libcamera/ipa_*.so > usr/lib/*/libcamera/ipa_*.so.sign > usr/lib/*/libcamera/*_ipa_proxy > usr/share/libcamera > > so it's capturing the ipa files into a package based on where they get > installed. > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-v4l2.install?ref_type=heads > > shows > > usr/lib/*/libcamera/v4l2-compat.so > > for the V4L2 side. > In Fedora we have: rpm -ql libcamera-ipa.x86_64 | grep "\.so" /usr/lib64/libcamera/ipa_ipu3.so /usr/lib64/libcamera/ipa_ipu3.so.sign /usr/lib64/libcamera/ipa_soft_simple.so /usr/lib64/libcamera/ipa_soft_simple.so.sign rpm -ql libcamera-v4l2.x86_64 | grep "\.so" /usr/libexec/libcamera/v4l2-compat.so > > And finally: > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/rules?ref_type=heads > > Shows: > > override_dh_auto_configure: > dh_auto_configure -- \ > --libexecdir=lib/${DEB_HOST_MULTIARCH} \ > > > Which is overridding the libexecdir for multiarch. > [...] > > Javier, Do you know if this is happening on Fedora or if there is any > requirements regarding /usr/libexec ? > In Fedora we don't override libexecdir, just use the distro default that is /usr/libexec/. There are no requirements about libexec in this regard, in fact libexec is used mostly for binaries and only have a few libraries. The only requirement that Fedora has in its packaging guidelines is that unversioned shared objects (which the v4l2-compact.so is IIRC) must not be visible to the dynamic linker: https://docs.fedoraproject.org/en-US/packaging-guidelines/Unversioned_shared_objects/ Which I think was the reason why it was moved to libexecdir to start with?
On Fri, Feb 14, 2025 at 10:37:54AM +0000, Kieran Bingham wrote: > Hi Dylan, > > Quoting Dylan Aissi (2025-02-13 22:17:45) > > Hi Kieran, > > > > On 2/13/25 18:36, Kieran Bingham wrote: > > > Hi Dylan, Javier, > > > > > > I've got some questions for you both at the bottom of this mail > > > regarding libcamera packaging. > > > > > > Quoting Laurent Pinchart (2025-02-13 17:01:26) > > >> On Wed, Feb 12, 2025 at 09:42:07AM +0000, Kieran Bingham wrote: > > >>> Quoting Laurent Pinchart (2023-08-28 20:13:13) > > >>>> On Mon, Aug 28, 2023 at 01:28:37PM +0100, Kieran Bingham wrote: > > >>>>> Quoting Kieran Bingham (2023-05-04 20:03:56) > > >>>>>> On 04/05/2023 16:10, Laurent Pinchart wrote: > > >>>>>>> On Thu, May 04, 2023 at 03:48:00PM +0100, Kieran Bingham via libcamera-devel wrote: > > >>>>>>>> Ensure that when we iterate the libcamera libdir we only select shared > > >>>>>>>> objects which are expected to be IPA modules, with an ipa_ prefix. > > >>>>>>>> > > >>>>>>>> Any shared object not matching this filter is ignored and not processed > > >>>>>>>> by the IPA Manager. > > >>>>>>>> > > >>>>>>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > >>>>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>>>>>> > > >>>>>>> I wonder what this protects against, as there shouldn't be other files > > >>>>>>> in this directory, especially if we move IPA modules to an ipa/ > > >>>>>>> subdirectory. I don't mind much, as this isn't a hot path, so I have no > > >>>>>>> objection if you want to merge this, but is it useful ? > > >>>>>> > > >>>>>> Only that it prevents us trying to load any files that might be there > > >>>>>> otherwise. > > >>>>>> > > >>>>>> > > >>>>>> [76:32:31.897778214] [2776180] ERROR IPAModule ipa_module.cpp:286 > > >>>>>> fake.so: IPA module is not an ELF file > > >>>>>> [76:32:31.899112311] [2776180] ERROR IPAModule ipa_module.cpp:172 Symbol > > >>>>>> ipaModuleInfo not found > > >>>>>> [76:32:31.899119975] [2776180] ERROR IPAModule ipa_module.cpp:292 > > >>>>>> v4l2-compat.so: IPA module has no valid info > > >>>>>> > > >>>>>> > > >>>>>> We already filter on .so objects, and we do other checks so it's not > > >>>>>> crucial, and it's not going to prevent any attempt to get > > >>>>>> 'non-libcamera' code parsed as anyone dropping a file here could equally > > >>>>>> call it 'ipa_fake.so' ... so no not really. > > >>>>>> > > >>>>>> I'll drop this and collect 2/2 > > >>>>> > > >>>>> I've started seeing various logs that report similar to: > > >>>>> ``` > > >>>>> $ wireplumber > > >>>>> [12:34:30.015061894] [206341] ERROR IPAModule ipa_module.cpp:172 Symbol ipaModuleInfo not found > > >>>>> [12:34:30.015085339] [206341] ERROR IPAModule ipa_module.cpp:292 v4l2-compat.so: IPA module has no valid info > > >>>>> [12:34:30.015125806] [206341] INFO Camera camera_manager.cpp:284 libcamera v0.1.0 > > >>>>> ``` > > >>>>> > > >>>>> I believe this is because some distributions are probably overriding the > > >>>>> installation path of the v4l2-compat.so. > > >>>>> > > >>>>> Anyway, that makes me think that we /should/ ignore files that are not > > >>>>> prefixed by ipa_ ... > > >>>> > > >>>> Or move IPA modules to an ipa/ subdirectory ? > > >>> > > >>> And I've just seen this in some logs from a raspberry pi issue: > > >>> > > >>> ``` > > >>> (AGS_311_venv) @AGS-RPI-0004:~$ libcamera-hello > > >>> [0:01:23.242753930] [811] ERROR IPAModule ipa_module.cpp:171 Symbol ipaModuleInfo not found > > >>> [0:01:23.242802797] [811] ERROR IPAModule ipa_module.cpp:291 v4l2-compat.so: IPA module has > > >>> no valid info > > >>> [0:01:23.242827435] [811] INFO Camera camera_manager.cpp:327 libcamera v0.4.0+51-ca36c77f > > >>> ``` > > >>> > > >>> So I think it's time to move the IPA modules to a subdirectory. I like > > >>> that better than arbitrary filtering of the name. > > >> > > >> It's really a distribution issue, by default IPA modules go to (on an > > >> Arm64 system) /usr/local/lib64 (and only IPA modules are installed > > >> there), and v4l2-compat.so go to /usr/local/libexec/libcamera/. We can > > >> create a subdirectory for IPA modules, but will distributions then > > >> decide to install v4l2-compat.so there ? :-) > > > > > > A distribution issue that's impacting multiple distributions ... > > > > > > So the question is /why/ ? > > > > > > > > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-ipa.install?ref_type=heads > > > shows: > > > > > > > > > usr/lib/*/libcamera/ipa_*.so > > > usr/lib/*/libcamera/ipa_*.so.sign > > > usr/lib/*/libcamera/*_ipa_proxy > > > usr/share/libcamera > > > > > > so it's capturing the ipa files into a package based on where they get > > > installed. > > > > > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/libcamera-v4l2.install?ref_type=heads > > > > > > shows > > > > > > usr/lib/*/libcamera/v4l2-compat.so > > > > > > for the V4L2 side. > > > > > > > > > And finally: > > > > > > https://salsa.debian.org/multimedia-team/libcamera/-/blob/debian/unstable/debian/rules?ref_type=heads > > > > > > Shows: > > > > > > override_dh_auto_configure: > > > dh_auto_configure -- \ > > > --libexecdir=lib/${DEB_HOST_MULTIARCH} \ > > > > > > > > > Which is overridding the libexecdir for multiarch. > > > > > > > > > Trying to search for debian guidance on this I hit: > > > > > > https://linux.debian.devel.mentors.narkive.com/hkilz1si/using-usr-libexec-directory > > > which is a mentor on the debian lists giving the following guidance: > > > > > >> Move the files to use "usr/lib" instead. Nothing on debian uses libexec. > > >> > > >> Usually there is a configure option one can pass, e.g. --libexecdir=/usr/lib > > > > > > and > > > https://lists.debian.org/debian-devel/2005/05/msg00404.html > > >>> Should we change some of these to /usr/libexec? > > >> > > >> well, it would be against the FHS, I think. > > >> > > >> The BSDs use libexec but I don't really see a good reason why it exists. > > >> > > > > > > And this one: > > > https://lists.debian.org/debian-devel/2005/05/msg00565.html > > >> The BSDs use libexec but I don't really see a good reason why it exists. > > >> > > >> The only reason we don't have it is because of petty bickering and > > >> politics between the FHS folks (several years ago). There were few > > >> technical objections to it on the FHS list, but it was dropped for > > >> non-technical reasons. Given that the FHS is supposed to codify > > >> existing practice, it should be in there on that count alone. Every > > >> libexec-using package in Debian has been reconfigured not to use it; > > >> upstreams do use it, and I'd like to use it myself. > > >> > > >> I'd personally be very glad to have it, and would support using it in > > >> Debian. > > > > > > > > > But so far I haven't found any documentation to 'specify' this. > > > > > > Dylan, Do you know /why/ the v4l2-compat.so is moved out of libexec and > > > into lib/libcamera ? (Noting that it causes a spurious error to be > > > reported to users who have libcamera-v4l2 installed). > > > > I think the answer is just "bad sequence of events" and there is no real > > reason. > > Let's do some git archeology to understand what happened: > > [1] In 2020, libexecdir was overridden to make the package multiarch > > compatible. > > [2] ~ 10 days later, v4l2 compat was enabled, so with libexecdir already > > overridden. > > [3] In 2022, I moved v4l2 into its own binary package, but unrelated > > with our current issue. > > [4] In 2023, v4l2 was moved upstream from libexec to libexec/libcamera. > > [5] This was reflected in the debian package but with the override of > > libexecdir, v4l2 has landed into usr/lib/*/libcamera/ which is the same > > directory than ipa modules. > > Thank you - that is incredibly helpful! And now it's much clearer what > was going on. > > > I don't see a good reason other than trying to make v4l2 and *_ipa_proxy > > multiarch co-installable. > > Now the question is, does this make sense? I don't know. I can drop the > > libexecdir override, both v4l2 and *_ipa_proxy will go back in > > usr/libexec/libcamera/ > > and both binary packages libcamera-v4l2 and libcamera-ipa will no longer > > be multiarch co-installable, but I am not sure there was a real use case > > anyway. > > The tricky part here will likely be the IPA proxies. I think that's part > of 'libcamera' but could be handled as part of libcamera-ipa indeed, and > then that component shouldn't be multiarch - the package itself would be > architecture specific. Same for the libcamera-v4l2. > > Having libcamera core multi-arch facilitates cross compiling which is > great. libcamera-v4l2 and libcamera-ipa are not required for that. > > Are there other use-cases for having multi-arch libcamera? I agree with the reasoning. All components needed at build time should come from multiarch packages. Components needed at runtime only (IPA modules, IPA proxies, and v4l2-compat.so) don't need to be multiarch-compatible as far as I can tell. > > [1] https://salsa.debian.org/multimedia-team/libcamera/-/commit/04fda66ee27bfacd07a39acb86bbaba376bcc050 > > [2] https://salsa.debian.org/multimedia-team/libcamera/-/commit/89ab628d6d9605547423c7696f4dbc8b10ce7708 > > [3] https://salsa.debian.org/multimedia-team/libcamera/-/commit/7bfebf853bb18004ed26c822a0b8a1cb3fb1f0d7 > > [4] https://gitlab.freedesktop.org/camera/libcamera/-/commit/1c512d406536d72a393c38c3f6a75fe0fdb9ecb2 > > [5] https://salsa.debian.org/multimedia-team/libcamera/-/commit/a991999c4c935d150c04a88f401cf2dcf5314c71 > > > > > I know this same issue is occuring on Alpine and Arch systems too. So it > > > seems like "all the major distributions" are doing roughly the same > > > thing here. > > > > Maybe for the same reason, or maybe they just copied our mistakes. > > Indeed, I'll chase those distributions with the same rationale now that > we understand it though. > > Thank you very much for helping get this clear. > > Will you make an update to the debian packages? > > > > Javier, Do you know if this is happening on Fedora or if there is any > > > requirements regarding /usr/libexec ?
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index 030ef43fb994..c0c2f027e902 100644 --- a/src/libcamera/ipa_manager.cpp +++ b/src/libcamera/ipa_manager.cpp @@ -206,6 +206,10 @@ void IPAManager::parseDir(const char *libDir, unsigned int maxDepth, if (strcmp(&ent->d_name[offset], ".so")) continue; + /* Ignore any modules which are not IPAs. */ + if (strncmp(ent->d_name, "ipa_", 4) != 0) + continue; + files.push_back(std::string(libDir) + "/" + ent->d_name); }