| Message ID | 20241103152205.29219-8-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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. > > Currently createIPA() / IPAManager::module() assume that there is a 1:1 > relationship between pipeline handlers and IPAs and IPA matching is done > based on pipe->name(). > > Add an optional ipaName argument to createIPA() which when set overrides > pipe->name(), allowing pipeline handlers to request a different IPA. This deserves a bit more explanation. Looking at the followup patch, the purpose seems to be to reuse a given IPA completely in a different pipeline. Now the question would be, does the IPA belong to the original pipeline or to the new one, why should it be named after one or the other? And what if some pipelines would like to reuse only parts of software ISP and use different algorithms from its own IPA (which is not unlikely to happen in foreseeable future)? Is it about whole IPA sharing or more about sharing algorithm implementations? Does it matter whether software ISP runs on CPU or GPU and can the new pipeline get disturbed by contingent changes in the shared IPA? I understand the solution in this patch is the easiest one for the purpose of atomisp. I'm not sure whether the maintainers will like it; if they do I have nothing against it. But I think it's worth to think a bit about other possible ways of sharing pieces of software ISP among pipelines. Maybe the conclusion would be it'd be nice and possible but too complicated at the moment -- then fine but let's know how and why. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > include/libcamera/internal/ipa_manager.h | 11 ++++++++--- > include/libcamera/internal/ipa_module.h | 2 +- > src/libcamera/ipa_manager.cpp | 7 ++++--- > src/libcamera/ipa_module.cpp | 6 +++--- > 4 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h > index 16dede0c..1bbb0011 100644 > --- a/include/libcamera/internal/ipa_manager.h > +++ b/include/libcamera/internal/ipa_manager.h > @@ -33,11 +33,16 @@ 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) nullptr > { > CameraManager *cm = pipe->cameraManager(); > IPAManager *self = cm->_d()->ipaManager(); > - IPAModule *m = self->module(pipe, minVersion, maxVersion); > + > + if (!ipaName) > + ipaName = pipe->name(); > + > + IPAModule *m = self->module(ipaName, minVersion, maxVersion); > if (!m) > return nullptr; > > @@ -62,7 +67,7 @@ private: > std::vector<std::string> &files); > unsigned int addDir(const char *libDir, unsigned int maxDepth = 0); > > - IPAModule *module(PipelineHandler *pipe, uint32_t minVersion, > + IPAModule *module(const char *pipelineName, uint32_t minVersion, > uint32_t maxVersion); > > bool isSignatureValid(IPAModule *ipa) const; > diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h > index 7c49d3f3..f732b0b0 100644 > --- a/include/libcamera/internal/ipa_module.h > +++ b/include/libcamera/internal/ipa_module.h > @@ -36,7 +36,7 @@ public: > > IPAInterface *createInterface(); > > - bool match(PipelineHandler *pipe, > + bool match(const char *pipelineName, > uint32_t minVersion, uint32_t maxVersion) const; > > protected: > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index cfc24d38..a5427ef3 100644 > --- a/src/libcamera/ipa_manager.cpp > +++ b/src/libcamera/ipa_manager.cpp > @@ -243,15 +243,15 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth) > > /** > * \brief Retrieve an IPA module that matches a given pipeline handler > - * \param[in] pipe The pipeline handler > + * \param[in] pipelineName The pipeline handler name > * \param[in] minVersion Minimum acceptable version of IPA module > * \param[in] maxVersion Maximum acceptable version of IPA module > */ > -IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, > +IPAModule *IPAManager::module(const char *pipelineName, uint32_t minVersion, > uint32_t maxVersion) > { > for (IPAModule *module : modules_) { > - if (module->match(pipe, minVersion, maxVersion)) > + if (module->match(pipelineName, minVersion, maxVersion)) > return module; > } > > @@ -264,6 +264,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 overrides pipe->name() for IPA module matching if set > * > * \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 9ca74be6..7392e356 100644 > --- a/src/libcamera/ipa_module.cpp > +++ b/src/libcamera/ipa_module.cpp > @@ -463,7 +463,7 @@ IPAInterface *IPAModule::createInterface() > > /** > * \brief Verify if the IPA module matches a given pipeline handler > - * \param[in] pipe Pipeline handler to match with > + * \param[in] pipelineName Pipeline handler name to match with > * \param[in] minVersion Minimum acceptable version of IPA module > * \param[in] maxVersion Maximum acceptable version of IPA module > * > @@ -472,12 +472,12 @@ IPAInterface *IPAModule::createInterface() > * > * \return True if the pipeline handler matches the IPA module, or false otherwise > */ > -bool IPAModule::match(PipelineHandler *pipe, > +bool IPAModule::match(const char *pipelineName, > uint32_t minVersion, uint32_t maxVersion) const > { > return info_.pipelineVersion >= minVersion && > info_.pipelineVersion <= maxVersion && > - !strcmp(info_.pipelineName, pipe->name()); > + !strcmp(info_.pipelineName, pipelineName); > } > > std::string IPAModule::logPrefix() const
diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h index 16dede0c..1bbb0011 100644 --- a/include/libcamera/internal/ipa_manager.h +++ b/include/libcamera/internal/ipa_manager.h @@ -33,11 +33,16 @@ 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); + + if (!ipaName) + ipaName = pipe->name(); + + IPAModule *m = self->module(ipaName, minVersion, maxVersion); if (!m) return nullptr; @@ -62,7 +67,7 @@ private: std::vector<std::string> &files); unsigned int addDir(const char *libDir, unsigned int maxDepth = 0); - IPAModule *module(PipelineHandler *pipe, uint32_t minVersion, + IPAModule *module(const char *pipelineName, uint32_t minVersion, uint32_t maxVersion); bool isSignatureValid(IPAModule *ipa) const; diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h index 7c49d3f3..f732b0b0 100644 --- a/include/libcamera/internal/ipa_module.h +++ b/include/libcamera/internal/ipa_module.h @@ -36,7 +36,7 @@ public: IPAInterface *createInterface(); - bool match(PipelineHandler *pipe, + bool match(const char *pipelineName, uint32_t minVersion, uint32_t maxVersion) const; protected: diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index cfc24d38..a5427ef3 100644 --- a/src/libcamera/ipa_manager.cpp +++ b/src/libcamera/ipa_manager.cpp @@ -243,15 +243,15 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth) /** * \brief Retrieve an IPA module that matches a given pipeline handler - * \param[in] pipe The pipeline handler + * \param[in] pipelineName The pipeline handler name * \param[in] minVersion Minimum acceptable version of IPA module * \param[in] maxVersion Maximum acceptable version of IPA module */ -IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion, +IPAModule *IPAManager::module(const char *pipelineName, uint32_t minVersion, uint32_t maxVersion) { for (IPAModule *module : modules_) { - if (module->match(pipe, minVersion, maxVersion)) + if (module->match(pipelineName, minVersion, maxVersion)) return module; } @@ -264,6 +264,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 overrides pipe->name() for IPA module matching if set * * \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 9ca74be6..7392e356 100644 --- a/src/libcamera/ipa_module.cpp +++ b/src/libcamera/ipa_module.cpp @@ -463,7 +463,7 @@ IPAInterface *IPAModule::createInterface() /** * \brief Verify if the IPA module matches a given pipeline handler - * \param[in] pipe Pipeline handler to match with + * \param[in] pipelineName Pipeline handler name to match with * \param[in] minVersion Minimum acceptable version of IPA module * \param[in] maxVersion Maximum acceptable version of IPA module * @@ -472,12 +472,12 @@ IPAInterface *IPAModule::createInterface() * * \return True if the pipeline handler matches the IPA module, or false otherwise */ -bool IPAModule::match(PipelineHandler *pipe, +bool IPAModule::match(const char *pipelineName, uint32_t minVersion, uint32_t maxVersion) const { return info_.pipelineVersion >= minVersion && info_.pipelineVersion <= maxVersion && - !strcmp(info_.pipelineName, pipe->name()); + !strcmp(info_.pipelineName, pipelineName); } 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 pipe->name(). Add an optional ipaName argument to createIPA() which when set overrides pipe->name(), allowing pipeline handlers to request a different IPA. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- include/libcamera/internal/ipa_manager.h | 11 ++++++++--- include/libcamera/internal/ipa_module.h | 2 +- src/libcamera/ipa_manager.cpp | 7 ++++--- src/libcamera/ipa_module.cpp | 6 +++--- 4 files changed, 16 insertions(+), 10 deletions(-)