[libcamera-devel] [PATCH v2 1/2] libcamera: ipa_manager: Only parse IPA modules

[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 at redhat.com>
> > > > >> Signed-off-by: Kieran Bingham <kieran.bingham at 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);
> > > > >>      }
> > > > >>   

-- 
Regards,

Laurent Pinchart