| Message ID | 20250510141220.54872-8-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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. > */ > IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, > - uint32_t maxVersion) > + uint32_t maxVersion, const char *ipaName) > { > for (const auto &module : modules_) { > - if (module->match(pipe, minVersion, maxVersion)) > + if (module->match(pipe, minVersion, maxVersion, ipaName)) > return module.get(); > } > > @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, > * \param[in] pipe The pipeline handler that wants a matching IPA proxy > * \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 > * > * \return A newly created IPA proxy, or nullptr if no matching IPA module is > * found or if the IPA proxy fails to initialize > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > index e6ea61e4..b7004b1c 100644 > --- a/src/libcamera/ipa_module.cpp > +++ b/src/libcamera/ipa_module.cpp > @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface() > * \param[in] pipe Pipeline handler to match with > * \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 > * > * This function checks if this IPA module matches the \a pipe pipeline handler, > - * and the input version range. > + * and the input version range. If \a ipaName is non-null then the IPA module > + * name is matched against \a ipaName instead of matching \a pipe. > * > * \return True if the pipeline handler matches the IPA module, or false otherwise > */ > -bool IPAModule::match(PipelineHandler *pipe, > - uint32_t minVersion, uint32_t maxVersion) const > +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion, > + uint32_t maxVersion, const char *ipaName) const > { > - return info_.pipelineVersion >= minVersion && > - info_.pipelineVersion <= maxVersion && > - !strcmp(info_.pipelineName, pipe->name()); > + if (info_.pipelineVersion < minVersion || > + info_.pipelineVersion > maxVersion) > + return false; > + > + if (ipaName) > + return !strcmp(info_.name, ipaName); > + else > + return !strcmp(info_.pipelineName, pipe->name()); The code looks fine so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > std::string IPAModule::logPrefix() const > -- > 2.49.0 >
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@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 ? Regards, Hans >> */ >> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, >> - uint32_t maxVersion) >> + uint32_t maxVersion, const char *ipaName) >> { >> for (const auto &module : modules_) { >> - if (module->match(pipe, minVersion, maxVersion)) >> + if (module->match(pipe, minVersion, maxVersion, ipaName)) >> return module.get(); >> } >> >> @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, >> * \param[in] pipe The pipeline handler that wants a matching IPA proxy >> * \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 >> * >> * \return A newly created IPA proxy, or nullptr if no matching IPA module is >> * found or if the IPA proxy fails to initialize >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp >> index e6ea61e4..b7004b1c 100644 >> --- a/src/libcamera/ipa_module.cpp >> +++ b/src/libcamera/ipa_module.cpp >> @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface() >> * \param[in] pipe Pipeline handler to match with >> * \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 >> * >> * This function checks if this IPA module matches the \a pipe pipeline handler, >> - * and the input version range. >> + * and the input version range. If \a ipaName is non-null then the IPA module >> + * name is matched against \a ipaName instead of matching \a pipe. >> * >> * \return True if the pipeline handler matches the IPA module, or false otherwise >> */ >> -bool IPAModule::match(PipelineHandler *pipe, >> - uint32_t minVersion, uint32_t maxVersion) const >> +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion, >> + uint32_t maxVersion, const char *ipaName) const >> { >> - return info_.pipelineVersion >= minVersion && >> - info_.pipelineVersion <= maxVersion && >> - !strcmp(info_.pipelineName, pipe->name()); >> + if (info_.pipelineVersion < minVersion || >> + info_.pipelineVersion > maxVersion) >> + return false; >> + >> + if (ipaName) >> + return !strcmp(info_.name, ipaName); >> + else >> + return !strcmp(info_.pipelineName, pipe->name()); > > The code looks fine so: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> } >> >> std::string IPAModule::logPrefix() const >> -- >> 2.49.0 >> >
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@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
Hi Hans, Hans de Goede <hdegoede@redhat.com> writes: > Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for > more then 1 pipeline handler. s/then/than/ > 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, s/then/than/ > 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. > > Signed-off-by: Hans de Goede <hdegoede@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) s/NULL/nullptr/ > { > 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); Wouldn't it be nicer to introduce IPAModule *module(const char *ipaName, uint32_t minVersion, uint32_t maxVersion); and the same for IPAModule::match? It's more lines of code but less confusion. > 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; s/NULL/nullptr/ > 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 > */ > IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, > - uint32_t maxVersion) > + uint32_t maxVersion, const char *ipaName) > { > for (const auto &module : modules_) { > - if (module->match(pipe, minVersion, maxVersion)) > + if (module->match(pipe, minVersion, maxVersion, ipaName)) > return module.get(); > } > > @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, > * \param[in] pipe The pipeline handler that wants a matching IPA proxy > * \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 > * > * \return A newly created IPA proxy, or nullptr if no matching IPA module is > * found or if the IPA proxy fails to initialize > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > index e6ea61e4..b7004b1c 100644 > --- a/src/libcamera/ipa_module.cpp > +++ b/src/libcamera/ipa_module.cpp > @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface() > * \param[in] pipe Pipeline handler to match with > * \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 > * > * This function checks if this IPA module matches the \a pipe pipeline handler, > - * and the input version range. > + * and the input version range. If \a ipaName is non-null then the IPA module > + * name is matched against \a ipaName instead of matching \a pipe. > * > * \return True if the pipeline handler matches the IPA module, or false otherwise > */ > -bool IPAModule::match(PipelineHandler *pipe, > - uint32_t minVersion, uint32_t maxVersion) const > +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion, > + uint32_t maxVersion, const char *ipaName) const > { > - return info_.pipelineVersion >= minVersion && > - info_.pipelineVersion <= maxVersion && > - !strcmp(info_.pipelineName, pipe->name()); > + if (info_.pipelineVersion < minVersion || > + info_.pipelineVersion > maxVersion) > + return false; > + > + if (ipaName) > + return !strcmp(info_.name, ipaName); > + else > + return !strcmp(info_.pipelineName, pipe->name()); > } > > std::string IPAModule::logPrefix() const
Hi 2025. 05. 11. 12:42 keltezéssel, Kieran Bingham írta: > 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" Maybe we can go a step further and explicitly require the IPA name from pipeline handlers? And remove `IPAModuleInfo::pipelineName`? Regards, Barnabás Pőcze > >> Signed-off-by: Hans de Goede <hdegoede@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. > >> */ >> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, >> - uint32_t maxVersion) >> + uint32_t maxVersion, const char *ipaName) >> { >> for (const auto &module : modules_) { >> - if (module->match(pipe, minVersion, maxVersion)) >> + if (module->match(pipe, minVersion, maxVersion, ipaName)) >> return module.get(); >> } >> >> @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, >> * \param[in] pipe The pipeline handler that wants a matching IPA proxy >> * \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 >> * >> * \return A newly created IPA proxy, or nullptr if no matching IPA module is >> * found or if the IPA proxy fails to initialize >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp >> index e6ea61e4..b7004b1c 100644 >> --- a/src/libcamera/ipa_module.cpp >> +++ b/src/libcamera/ipa_module.cpp >> @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface() >> * \param[in] pipe Pipeline handler to match with >> * \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 >> * >> * This function checks if this IPA module matches the \a pipe pipeline handler, >> - * and the input version range. >> + * and the input version range. If \a ipaName is non-null then the IPA module >> + * name is matched against \a ipaName instead of matching \a pipe. >> * >> * \return True if the pipeline handler matches the IPA module, or false otherwise >> */ >> -bool IPAModule::match(PipelineHandler *pipe, >> - uint32_t minVersion, uint32_t maxVersion) const >> +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion, >> + uint32_t maxVersion, const char *ipaName) const >> { >> - return info_.pipelineVersion >= minVersion && >> - info_.pipelineVersion <= maxVersion && >> - !strcmp(info_.pipelineName, pipe->name()); >> + if (info_.pipelineVersion < minVersion || >> + info_.pipelineVersion > maxVersion) >> + return false; >> + >> + if (ipaName) >> + return !strcmp(info_.name, ipaName); >> + else >> + return !strcmp(info_.pipelineName, pipe->name()); > > The code looks fine so: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> } >> >> std::string IPAModule::logPrefix() const >> -- >> 2.49.0 >>
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 */ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, - uint32_t maxVersion) + uint32_t maxVersion, const char *ipaName) { for (const auto &module : modules_) { - if (module->match(pipe, minVersion, maxVersion)) + if (module->match(pipe, minVersion, maxVersion, ipaName)) return module.get(); } @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, * \param[in] pipe The pipeline handler that wants a matching IPA proxy * \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 * * \return A newly created IPA proxy, or nullptr if no matching IPA module is * found or if the IPA proxy fails to initialize diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp index e6ea61e4..b7004b1c 100644 --- a/src/libcamera/ipa_module.cpp +++ b/src/libcamera/ipa_module.cpp @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface() * \param[in] pipe Pipeline handler to match with * \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 * * This function checks if this IPA module matches the \a pipe pipeline handler, - * and the input version range. + * and the input version range. If \a ipaName is non-null then the IPA module + * name is matched against \a ipaName instead of matching \a pipe. * * \return True if the pipeline handler matches the IPA module, or false otherwise */ -bool IPAModule::match(PipelineHandler *pipe, - uint32_t minVersion, uint32_t maxVersion) const +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion, + uint32_t maxVersion, const char *ipaName) const { - return info_.pipelineVersion >= minVersion && - info_.pipelineVersion <= maxVersion && - !strcmp(info_.pipelineName, pipe->name()); + if (info_.pipelineVersion < minVersion || + info_.pipelineVersion > maxVersion) + return false; + + if (ipaName) + return !strcmp(info_.name, ipaName); + else + return !strcmp(info_.pipelineName, pipe->name()); } std::string IPAModule::logPrefix() const
Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for more then 1 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. Signed-off-by: Hans de Goede <hdegoede@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(-)