[PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching by IPA name instead of by pipeline

[Kieran Bingham]

Quoting Hans de Goede (2025-05-11 14:49:13)
> Hi Kieran,
> 
> Thank you for the review.
> 
> On 11-May-25 12:42 PM, Kieran Bingham wrote:
> > Quoting Hans de Goede (2025-05-10 15:12:19)
> >> Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for
> >> more then 1 pipeline handler.
> > 
> > I can see this being useful in other pipelines too - as I think we can
> > anticipate multiple pipelines that can share a common IPA (certainly the
> > SoftIPA/GPU-IPA) but also potentially other derivatives of the RKISP1
> > pipeline handler.
> > 
> >> Currently createIPA() / IPAManager::module() assume that there is a 1:1
> >> relationship between pipeline handlers and IPAs and IPA matching is done
> >> based on matching the pipe to ipaModuleInfo.pipelineName[].
> >>
> >> One way to allow using a single IPA with multiple pipelines would be to
> >> allow the IPA to declare itself compatible with more then one pipeline,
> >> turning ipaModuleInfo.pipelineName[] into e.g. a vector. But the way
> >> ipaModuleInfo is loaded as an ELF symbol requires it to be a simple flat
> >> C-struct.
> >>
> >> Instead add an optional ipaName argument to createIPA() which when set
> >> switches things over to matching ipaModuleInfo.name[] allowing pipelines
> >> to request a specific shared IPA module this way.
> > 
> > I think that's reasonable. The Pipeline handler knows more about the
> > system than the IPA ... so it's more reasonable for the PH to say "I can
> > use this" rather than the IPA to say "I support X PipelineHandlers"
> > 
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> ---
> >>  include/libcamera/internal/ipa_manager.h |  7 ++++---
> >>  include/libcamera/internal/ipa_module.h  |  4 ++--
> >>  src/libcamera/ipa_manager.cpp            |  6 ++++--
> >>  src/libcamera/ipa_module.cpp             | 19 +++++++++++++------
> >>  4 files changed, 23 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> >> index a0d448cf..af784c9c 100644
> >> --- a/include/libcamera/internal/ipa_manager.h
> >> +++ b/include/libcamera/internal/ipa_manager.h
> >> @@ -34,11 +34,12 @@ public:
> >>         template<typename T>
> >>         static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> >>                                             uint32_t minVersion,
> >> -                                           uint32_t maxVersion)
> >> +                                           uint32_t maxVersion,
> >> +                                           const char *ipaName = NULL)
> >>         {
> >>                 CameraManager *cm = pipe->cameraManager();
> >>                 IPAManager *self = cm->_d()->ipaManager();
> >> -               IPAModule *m = self->module(pipe, minVersion, maxVersion);
> >> +               IPAModule *m = self->module(pipe, minVersion, maxVersion, ipaName);
> >>                 if (!m)
> >>                         return nullptr;
> >>  
> >> @@ -64,7 +65,7 @@ private:
> >>         unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);
> >>  
> >>         IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,
> >> -                         uint32_t maxVersion);
> >> +                         uint32_t maxVersion, const char *ipaName);
> >>  
> >>         bool isSignatureValid(IPAModule *ipa) const;
> >>  
> >> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h
> >> index 15f19492..e7b00fdb 100644
> >> --- a/include/libcamera/internal/ipa_module.h
> >> +++ b/include/libcamera/internal/ipa_module.h
> >> @@ -36,8 +36,8 @@ public:
> >>  
> >>         IPAInterface *createInterface();
> >>  
> >> -       bool match(PipelineHandler *pipe,
> >> -                  uint32_t minVersion, uint32_t maxVersion) const;
> >> +       bool match(PipelineHandler *pipe, uint32_t minVersion,
> >> +                  uint32_t maxVersion, const char *ipaName = NULL) const;
> >>  
> >>  protected:
> >>         std::string logPrefix() const override;
> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> >> index 830750dc..2ef8b98e 100644
> >> --- a/src/libcamera/ipa_manager.cpp
> >> +++ b/src/libcamera/ipa_manager.cpp
> >> @@ -240,12 +240,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
> >>   * \param[in] pipe The pipeline handler
> >>   * \param[in] minVersion Minimum acceptable version of IPA module
> >>   * \param[in] maxVersion Maximum acceptable version of IPA module
> >> + * \param[in] ipaName If set match IPA module by this name instead of by pipe
> > 
> > I wondered about saying "Defaults to the pipeline handler name" ... but
> > either way.
> 
> That is not true/correct though, after this patch there are 2 different
> matching methods using 2 different fields of ipaModuleInfo:
> 
> 1. ipaName==null, match ipaModuleInfo.pipelineName[] against pipe->name()
> 2. ipaName!=null, match ipaModuleInfo.name[] against ipaName
> 
> So claiming that ipaName defaults to pipe->name() when not set is
> incorrect, when ipaName is not set, ipaName and ipaModuleInfo.name[]
> are both not used at all.
> 
> Maybe the help text for IPAModule::match() needs to be updated a bit
> more in this patch to make this more explicit ?

Ohh - I hadn't realised that extra bit there - so maybe the text here
could reference that - but I'm fine with what you have already too.


--
Kieran