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

[Laurent Pinchart]

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 ?

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