[PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching by IPA name instead of by pipeline
Hans de Goede
hdegoede at redhat.com
Sun May 11 15:49:13 CEST 2025
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 ?
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 at ideasonboard.com>
>
>> }
>>
>> std::string IPAModule::logPrefix() const
>> --
>> 2.49.0
>>
>
More information about the libcamera-devel
mailing list