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

[Kieran Bingham]

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.
--
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