[v4,1/4] libcamera: ipa_manager: Create IPA by name
diff mbox series

Message ID 20260408115606.12417-2-johannes.goede@oss.qualcomm.com
State Superseded
Headers show
Series
  • ipa: Allow IPA creation by name
Related show

Commit Message

Hans de Goede April 8, 2026, 11:56 a.m. UTC
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 than 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, move the IPA creation procedure to be name-based, introducing
an overload to IPAManager::createIPA(pipe, name, minVer, maxVer)  that
allows to specify the name of the IPA module to match. Pipeline handlers
that wants to use their name as matching criteria can continue doing so
using the already existing createIPA(pipe, minVer, maxVer) overload.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 include/libcamera/internal/ipa_manager.h | 13 +++++++--
 include/libcamera/internal/ipa_module.h  |  4 +--
 src/libcamera/ipa_manager.cpp            | 34 +++++++++++++++++++-----
 src/libcamera/ipa_module.cpp             | 12 ++++-----
 4 files changed, 47 insertions(+), 16 deletions(-)

Comments

Barnabás Pőcze April 13, 2026, 10:21 a.m. UTC | #1
Hi

2026. 04. 08. 13:56 keltezéssel, Hans de Goede írta:
> 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 than 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, move the IPA creation procedure to be name-based, introducing
> an overload to IPAManager::createIPA(pipe, name, minVer, maxVer)  that
> allows to specify the name of the IPA module to match. Pipeline handlers
> that wants to use their name as matching criteria can continue doing so
> using the already existing createIPA(pipe, minVer, maxVer) overload.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> ---
>   include/libcamera/internal/ipa_manager.h | 13 +++++++--
>   include/libcamera/internal/ipa_module.h  |  4 +--
>   src/libcamera/ipa_manager.cpp            | 34 +++++++++++++++++++-----
>   src/libcamera/ipa_module.cpp             | 12 ++++-----
>   4 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index f8ce78016..4c01e76f1 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -34,12 +34,13 @@ public:
>   
>   	template<typename T>
>   	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> +					    const char *name,
>   					    uint32_t minVersion,
>   					    uint32_t maxVersion)
>   	{

I agree with this change, but I wish that the name could be derived from `T`.
And that is quite easy to do, just adding a

   static constexpr const char *name() { return "..."; }

and then using `T::name()` here works mostly.

But unfortunately the raspberry pi ipa modules are not compatible at the moment
with this approach. I have tried to come up with something, but so for has failed.

Maybe the `name` argument could be kept like this:

   const char *name = T::name()

and then the rpi pipeline handler could be changed to use the specific names. Not
a fan of this in any case.


Regards,
Barnabás Pőcze


>   		CameraManager *cm = pipe->cameraManager();
>   		IPAManager *self = cm->_d()->ipaManager();
> -		IPAModule *m = self->module(pipe, minVersion, maxVersion);
> +		IPAModule *m = self->module(name, minVersion, maxVersion);
>   		if (!m)
>   			return nullptr;
>   
> @@ -60,6 +61,14 @@ public:
>   		return proxy;
>   	}
>   
> +	template<typename T>
> +	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> +					    uint32_t minVersion,
> +					    uint32_t maxVersion)
> +	{
> +		return createIPA<T>(pipe, pipe->name(), minVersion, maxVersion);
> +	}
> +
>   #if HAVE_IPA_PUBKEY
>   	static const PubKey &pubKey()
>   	{
> @@ -72,7 +81,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 *name, 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 15f19492c..a0a53764e 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(const char *name, uint32_t minVersion,
> +		   uint32_t maxVersion) const;
>   
>   protected:
>   	std::string logPrefix() const override;
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 35171d097..f62a4ee5f 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -247,15 +247,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] name The IPA module string identifier
>    * \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 *name, uint32_t minVersion,
>   			      uint32_t maxVersion)
>   {
>   	for (const auto &module : modules_) {
> -		if (module->match(pipe, minVersion, maxVersion))
> +		if (module->match(name, minVersion, maxVersion))
>   			return module.get();
>   	}
>   
> @@ -263,12 +263,34 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>   }
>   
>   /**
> - * \fn IPAManager::createIPA()
> - * \brief Create an IPA proxy that matches a given pipeline handler
> - * \param[in] pipe The pipeline handler that wants a matching IPA proxy
> + * \fn IPAManager::createIPA(PipelineHandler *pipe, const char *ipaName, uint32_t minVersion, uint32_t maxVersion)
> + * \brief Create an IPA proxy that matches the requested name and version
> + * \param[in] pipe The pipeline handler that wants to create the IPA module
> + * \param[in] ipaName The IPA module name
>    * \param[in] minVersion Minimum acceptable version of IPA module
>    * \param[in] maxVersion Maximum acceptable version of IPA module
>    *
> + * Create an IPA module using \a name as the matching identifier. This overload
> + * allows pipeline handlers to create an IPA module by specifying its name
> + * instead of relying on the fact that the IPA module matches the pipeline
> + * handler's one.
> + *
> + * \return A newly created IPA proxy, or nullptr if no matching IPA module is
> + * found or if the IPA proxy fails to initialize
> + */
> +
> +/**
> + * \fn IPAManager::createIPA(PipelineHandler *pipe, uint32_t minVersion, uint32_t maxVersion)
> + * \brief Create an IPA proxy that matches the pipeline handler name and the
> + * requested version
> + * \param[in] pipe The pipeline handler that wants to create the IPA module
> + * \param[in] minVersion Minimum acceptable version of IPA module
> + * \param[in] maxVersion Maximum acceptable version of IPA module
> + *
> + * Create an IPA module using the pipeline handler name as the matching
> + * identifier. This overload allows pipeline handler to create an IPA module
> + * whose name matches the pipeline handler one.
> + *
>    * \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 e6ea61e44..0bd6f1462 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -463,21 +463,21 @@ IPAInterface *IPAModule::createInterface()
>   
>   /**
>    * \brief Verify if the IPA module matches a given pipeline handler
> - * \param[in] pipe Pipeline handler to match with
> + * \param[in] name The IPA module name
>    * \param[in] minVersion Minimum acceptable version of IPA module
>    * \param[in] maxVersion Maximum acceptable version of IPA module
>    *
> - * This function checks if this IPA module matches the \a pipe pipeline handler,
> + * This function checks if this IPA module matches the requested \a name
>    * and the input version range.
>    *
> - * \return True if the pipeline handler matches the IPA module, or false otherwise
> + * \return True if the IPA module matches, or false otherwise
>    */
> -bool IPAModule::match(PipelineHandler *pipe,
> -		      uint32_t minVersion, uint32_t maxVersion) const
> +bool IPAModule::match(const char *name, uint32_t minVersion,
> +		      uint32_t maxVersion) const
>   {
>   	return info_.pipelineVersion >= minVersion &&
>   	       info_.pipelineVersion <= maxVersion &&
> -	       !strcmp(info_.pipelineName, pipe->name());
> +	       !strcmp(info_.name, name);
>   }
>   
>   std::string IPAModule::logPrefix() const
Jacopo Mondi April 13, 2026, 1:32 p.m. UTC | #2
Hi Barnabás

On Mon, Apr 13, 2026 at 12:21:59PM +0200, Barnabás Pőcze wrote:
> Hi
>
> 2026. 04. 08. 13:56 keltezéssel, Hans de Goede írta:
> > 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 than 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, move the IPA creation procedure to be name-based, introducing
> > an overload to IPAManager::createIPA(pipe, name, minVer, maxVer)  that
> > allows to specify the name of the IPA module to match. Pipeline handlers
> > that wants to use their name as matching criteria can continue doing so
> > using the already existing createIPA(pipe, minVer, maxVer) overload.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> > ---
> >   include/libcamera/internal/ipa_manager.h | 13 +++++++--
> >   include/libcamera/internal/ipa_module.h  |  4 +--
> >   src/libcamera/ipa_manager.cpp            | 34 +++++++++++++++++++-----
> >   src/libcamera/ipa_module.cpp             | 12 ++++-----
> >   4 files changed, 47 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> > index f8ce78016..4c01e76f1 100644
> > --- a/include/libcamera/internal/ipa_manager.h
> > +++ b/include/libcamera/internal/ipa_manager.h
> > @@ -34,12 +34,13 @@ public:
> >   	template<typename T>
> >   	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> > +					    const char *name,
> >   					    uint32_t minVersion,
> >   					    uint32_t maxVersion)
> >   	{
>
> I agree with this change, but I wish that the name could be derived from `T`.
> And that is quite easy to do, just adding a
>
>   static constexpr const char *name() { return "..."; }
>
> and then using `T::name()` here works mostly.
>
> But unfortunately the raspberry pi ipa modules are not compatible at the moment
> with this approach. I have tried to come up with something, but so for has failed.

What is the issue ? That the proxy is the same for v4 and v5 ?


>
> Maybe the `name` argument could be kept like this:
>
>   const char *name = T::name()
>
> and then the rpi pipeline handler could be changed to use the specific names. Not
> a fan of this in any case.
>
>
> Regards,
> Barnabás Pőcze
>
>
> >   		CameraManager *cm = pipe->cameraManager();
> >   		IPAManager *self = cm->_d()->ipaManager();
> > -		IPAModule *m = self->module(pipe, minVersion, maxVersion);
> > +		IPAModule *m = self->module(name, minVersion, maxVersion);
> >   		if (!m)
> >   			return nullptr;
> > @@ -60,6 +61,14 @@ public:
> >   		return proxy;
> >   	}
> > +	template<typename T>
> > +	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> > +					    uint32_t minVersion,
> > +					    uint32_t maxVersion)
> > +	{
> > +		return createIPA<T>(pipe, pipe->name(), minVersion, maxVersion);
> > +	}
> > +
> >   #if HAVE_IPA_PUBKEY
> >   	static const PubKey &pubKey()
> >   	{
> > @@ -72,7 +81,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 *name, 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 15f19492c..a0a53764e 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(const char *name, uint32_t minVersion,
> > +		   uint32_t maxVersion) const;
> >   protected:
> >   	std::string logPrefix() const override;
> > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> > index 35171d097..f62a4ee5f 100644
> > --- a/src/libcamera/ipa_manager.cpp
> > +++ b/src/libcamera/ipa_manager.cpp
> > @@ -247,15 +247,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] name The IPA module string identifier
> >    * \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 *name, uint32_t minVersion,
> >   			      uint32_t maxVersion)
> >   {
> >   	for (const auto &module : modules_) {
> > -		if (module->match(pipe, minVersion, maxVersion))
> > +		if (module->match(name, minVersion, maxVersion))
> >   			return module.get();
> >   	}
> > @@ -263,12 +263,34 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
> >   }
> >   /**
> > - * \fn IPAManager::createIPA()
> > - * \brief Create an IPA proxy that matches a given pipeline handler
> > - * \param[in] pipe The pipeline handler that wants a matching IPA proxy
> > + * \fn IPAManager::createIPA(PipelineHandler *pipe, const char *ipaName, uint32_t minVersion, uint32_t maxVersion)
> > + * \brief Create an IPA proxy that matches the requested name and version
> > + * \param[in] pipe The pipeline handler that wants to create the IPA module
> > + * \param[in] ipaName The IPA module name
> >    * \param[in] minVersion Minimum acceptable version of IPA module
> >    * \param[in] maxVersion Maximum acceptable version of IPA module
> >    *
> > + * Create an IPA module using \a name as the matching identifier. This overload
> > + * allows pipeline handlers to create an IPA module by specifying its name
> > + * instead of relying on the fact that the IPA module matches the pipeline
> > + * handler's one.
> > + *
> > + * \return A newly created IPA proxy, or nullptr if no matching IPA module is
> > + * found or if the IPA proxy fails to initialize
> > + */
> > +
> > +/**
> > + * \fn IPAManager::createIPA(PipelineHandler *pipe, uint32_t minVersion, uint32_t maxVersion)
> > + * \brief Create an IPA proxy that matches the pipeline handler name and the
> > + * requested version
> > + * \param[in] pipe The pipeline handler that wants to create the IPA module
> > + * \param[in] minVersion Minimum acceptable version of IPA module
> > + * \param[in] maxVersion Maximum acceptable version of IPA module
> > + *
> > + * Create an IPA module using the pipeline handler name as the matching
> > + * identifier. This overload allows pipeline handler to create an IPA module
> > + * whose name matches the pipeline handler one.
> > + *
> >    * \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 e6ea61e44..0bd6f1462 100644
> > --- a/src/libcamera/ipa_module.cpp
> > +++ b/src/libcamera/ipa_module.cpp
> > @@ -463,21 +463,21 @@ IPAInterface *IPAModule::createInterface()
> >   /**
> >    * \brief Verify if the IPA module matches a given pipeline handler
> > - * \param[in] pipe Pipeline handler to match with
> > + * \param[in] name The IPA module name
> >    * \param[in] minVersion Minimum acceptable version of IPA module
> >    * \param[in] maxVersion Maximum acceptable version of IPA module
> >    *
> > - * This function checks if this IPA module matches the \a pipe pipeline handler,
> > + * This function checks if this IPA module matches the requested \a name
> >    * and the input version range.
> >    *
> > - * \return True if the pipeline handler matches the IPA module, or false otherwise
> > + * \return True if the IPA module matches, or false otherwise
> >    */
> > -bool IPAModule::match(PipelineHandler *pipe,
> > -		      uint32_t minVersion, uint32_t maxVersion) const
> > +bool IPAModule::match(const char *name, uint32_t minVersion,
> > +		      uint32_t maxVersion) const
> >   {
> >   	return info_.pipelineVersion >= minVersion &&
> >   	       info_.pipelineVersion <= maxVersion &&
> > -	       !strcmp(info_.pipelineName, pipe->name());
> > +	       !strcmp(info_.name, name);
> >   }
> >   std::string IPAModule::logPrefix() const
>
Barnabás Pőcze April 13, 2026, 1:55 p.m. UTC | #3
2026. 04. 13. 15:32 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Apr 13, 2026 at 12:21:59PM +0200, Barnabás Pőcze wrote:
>> Hi
>>
>> 2026. 04. 08. 13:56 keltezéssel, Hans de Goede írta:
>>> 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 than 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, move the IPA creation procedure to be name-based, introducing
>>> an overload to IPAManager::createIPA(pipe, name, minVer, maxVer)  that
>>> allows to specify the name of the IPA module to match. Pipeline handlers
>>> that wants to use their name as matching criteria can continue doing so
>>> using the already existing createIPA(pipe, minVer, maxVer) overload.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
>>> ---
>>>    include/libcamera/internal/ipa_manager.h | 13 +++++++--
>>>    include/libcamera/internal/ipa_module.h  |  4 +--
>>>    src/libcamera/ipa_manager.cpp            | 34 +++++++++++++++++++-----
>>>    src/libcamera/ipa_module.cpp             | 12 ++++-----
>>>    4 files changed, 47 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>>> index f8ce78016..4c01e76f1 100644
>>> --- a/include/libcamera/internal/ipa_manager.h
>>> +++ b/include/libcamera/internal/ipa_manager.h
>>> @@ -34,12 +34,13 @@ public:
>>>    	template<typename T>
>>>    	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
>>> +					    const char *name,
>>>    					    uint32_t minVersion,
>>>    					    uint32_t maxVersion)
>>>    	{
>>
>> I agree with this change, but I wish that the name could be derived from `T`.
>> And that is quite easy to do, just adding a
>>
>>    static constexpr const char *name() { return "..."; }
>>
>> and then using `T::name()` here works mostly.
>>
>> But unfortunately the raspberry pi ipa modules are not compatible at the moment
>> with this approach. I have tried to come up with something, but so for has failed.
> 
> What is the issue ? That the proxy is the same for v4 and v5 ?

The proxy type is the same, but different names need to be used.


> 
> 
>>
>> Maybe the `name` argument could be kept like this:
>>
>>    const char *name = T::name()
>>
>> and then the rpi pipeline handler could be changed to use the specific names. Not
>> a fan of this in any case.
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
>>>    		CameraManager *cm = pipe->cameraManager();
>>>    		IPAManager *self = cm->_d()->ipaManager();
>>> -		IPAModule *m = self->module(pipe, minVersion, maxVersion);
>>> +		IPAModule *m = self->module(name, minVersion, maxVersion);
>>>    		if (!m)
>>>    			return nullptr;
>>> @@ -60,6 +61,14 @@ public:
>>>    		return proxy;
>>>    	}
>>> +	template<typename T>
>>> +	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
>>> +					    uint32_t minVersion,
>>> +					    uint32_t maxVersion)
>>> +	{
>>> +		return createIPA<T>(pipe, pipe->name(), minVersion, maxVersion);
>>> +	}
>>> +
>>>    #if HAVE_IPA_PUBKEY
>>>    	static const PubKey &pubKey()
>>>    	{
>>> @@ -72,7 +81,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 *name, 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 15f19492c..a0a53764e 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(const char *name, uint32_t minVersion,
>>> +		   uint32_t maxVersion) const;
>>>    protected:
>>>    	std::string logPrefix() const override;
>>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>>> index 35171d097..f62a4ee5f 100644
>>> --- a/src/libcamera/ipa_manager.cpp
>>> +++ b/src/libcamera/ipa_manager.cpp
>>> @@ -247,15 +247,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] name The IPA module string identifier
>>>     * \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 *name, uint32_t minVersion,
>>>    			      uint32_t maxVersion)
>>>    {
>>>    	for (const auto &module : modules_) {
>>> -		if (module->match(pipe, minVersion, maxVersion))
>>> +		if (module->match(name, minVersion, maxVersion))
>>>    			return module.get();
>>>    	}
>>> @@ -263,12 +263,34 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>>>    }
>>>    /**
>>> - * \fn IPAManager::createIPA()
>>> - * \brief Create an IPA proxy that matches a given pipeline handler
>>> - * \param[in] pipe The pipeline handler that wants a matching IPA proxy
>>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, const char *ipaName, uint32_t minVersion, uint32_t maxVersion)
>>> + * \brief Create an IPA proxy that matches the requested name and version
>>> + * \param[in] pipe The pipeline handler that wants to create the IPA module
>>> + * \param[in] ipaName The IPA module name
>>>     * \param[in] minVersion Minimum acceptable version of IPA module
>>>     * \param[in] maxVersion Maximum acceptable version of IPA module
>>>     *
>>> + * Create an IPA module using \a name as the matching identifier. This overload
>>> + * allows pipeline handlers to create an IPA module by specifying its name
>>> + * instead of relying on the fact that the IPA module matches the pipeline
>>> + * handler's one.
>>> + *
>>> + * \return A newly created IPA proxy, or nullptr if no matching IPA module is
>>> + * found or if the IPA proxy fails to initialize
>>> + */
>>> +
>>> +/**
>>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, uint32_t minVersion, uint32_t maxVersion)
>>> + * \brief Create an IPA proxy that matches the pipeline handler name and the
>>> + * requested version
>>> + * \param[in] pipe The pipeline handler that wants to create the IPA module
>>> + * \param[in] minVersion Minimum acceptable version of IPA module
>>> + * \param[in] maxVersion Maximum acceptable version of IPA module
>>> + *
>>> + * Create an IPA module using the pipeline handler name as the matching
>>> + * identifier. This overload allows pipeline handler to create an IPA module
>>> + * whose name matches the pipeline handler one.
>>> + *
>>>     * \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 e6ea61e44..0bd6f1462 100644
>>> --- a/src/libcamera/ipa_module.cpp
>>> +++ b/src/libcamera/ipa_module.cpp
>>> @@ -463,21 +463,21 @@ IPAInterface *IPAModule::createInterface()
>>>    /**
>>>     * \brief Verify if the IPA module matches a given pipeline handler
>>> - * \param[in] pipe Pipeline handler to match with
>>> + * \param[in] name The IPA module name
>>>     * \param[in] minVersion Minimum acceptable version of IPA module
>>>     * \param[in] maxVersion Maximum acceptable version of IPA module
>>>     *
>>> - * This function checks if this IPA module matches the \a pipe pipeline handler,
>>> + * This function checks if this IPA module matches the requested \a name
>>>     * and the input version range.
>>>     *
>>> - * \return True if the pipeline handler matches the IPA module, or false otherwise
>>> + * \return True if the IPA module matches, or false otherwise
>>>     */
>>> -bool IPAModule::match(PipelineHandler *pipe,
>>> -		      uint32_t minVersion, uint32_t maxVersion) const
>>> +bool IPAModule::match(const char *name, uint32_t minVersion,
>>> +		      uint32_t maxVersion) const
>>>    {
>>>    	return info_.pipelineVersion >= minVersion &&
>>>    	       info_.pipelineVersion <= maxVersion &&
>>> -	       !strcmp(info_.pipelineName, pipe->name());
>>> +	       !strcmp(info_.name, name);
>>>    }
>>>    std::string IPAModule::logPrefix() const
>>
Laurent Pinchart May 13, 2026, 11:56 p.m. UTC | #4
CC'ing Naush and David.

On Mon, Apr 13, 2026 at 03:55:58PM +0200, Barnabás Pőcze wrote:
> 2026. 04. 13. 15:32 keltezéssel, Jacopo Mondi írta:
> > On Mon, Apr 13, 2026 at 12:21:59PM +0200, Barnabás Pőcze wrote:
> >> 2026. 04. 08. 13:56 keltezéssel, Hans de Goede írta:
> >>> 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 than 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, move the IPA creation procedure to be name-based, introducing
> >>> an overload to IPAManager::createIPA(pipe, name, minVer, maxVer)  that
> >>> allows to specify the name of the IPA module to match. Pipeline handlers
> >>> that wants to use their name as matching criteria can continue doing so
> >>> using the already existing createIPA(pipe, minVer, maxVer) overload.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> >>> ---
> >>>    include/libcamera/internal/ipa_manager.h | 13 +++++++--
> >>>    include/libcamera/internal/ipa_module.h  |  4 +--
> >>>    src/libcamera/ipa_manager.cpp            | 34 +++++++++++++++++++-----
> >>>    src/libcamera/ipa_module.cpp             | 12 ++++-----
> >>>    4 files changed, 47 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> >>> index f8ce78016..4c01e76f1 100644
> >>> --- a/include/libcamera/internal/ipa_manager.h
> >>> +++ b/include/libcamera/internal/ipa_manager.h
> >>> @@ -34,12 +34,13 @@ public:
> >>>    	template<typename T>
> >>>    	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> >>> +					    const char *name,
> >>>    					    uint32_t minVersion,
> >>>    					    uint32_t maxVersion)
> >>>    	{
> >>
> >> I agree with this change, but I wish that the name could be derived from `T`.
> >> And that is quite easy to do, just adding a
> >>
> >>    static constexpr const char *name() { return "..."; }
> >>
> >> and then using `T::name()` here works mostly.
> >>
> >> But unfortunately the raspberry pi ipa modules are not compatible at the moment
> >> with this approach. I have tried to come up with something, but so for has failed.
> > 
> > What is the issue ? That the proxy is the same for v4 and v5 ?
> 
> The proxy type is the same, but different names need to be used.

The more I think about it, the more uncomfortable I get with specifying
a name here.

The API contract between the pipeline handler is the IPA interface.
That's all we should have to specify in the pipeline handler. Adding a
name gives the pipeline handler the ability to select any IPA module,
without any check whatsoever to ensure compatibility of the interface.
That sounds like a very bad idea.

Raspberry Pi is the only platform where we have two IPA modules using
the same IPA interface without being interchangeable. Trying to use the
Pi4 IPA module on a Pi5 will not work, and vice versa. I would really
like to fix that, and I see two options:

- Merging the two IPA modules into a single one, compatible with both
  platforms. The IPA interface functions in the IPA module would
  essentially dispatch to Pi4 or Pi5 implementations, based on a
  platform version identifier (passed to the init() function I suppose).

- Splitting the IPA interface into two different interfaces. The
  interfaces can still share the same API, the important part is to have
  two different interface names in two .mojom files. All the data
  structures can be defined in a shared .mojom file imported by both
  interfaces (and maybe there's a way to also share a common interface
  definition instead of duplicating it).

The drawback of the first option is that the IPA module will contain
dead code (Pi4 code won't run on Pi5 and vice versa), while the drawback
of the second option is that we'll duplicate proxy and worker code (as
the code is generated, it's just a matter of binary size, there's no
additional maintenance burden there).

Naush, David, any opinion ?


Note that we would still support multiple intercheangeable IPA modules
using the same interface (and therefore compatible with the same
pipeline handler).

> >> Maybe the `name` argument could be kept like this:
> >>
> >>    const char *name = T::name()
> >>
> >> and then the rpi pipeline handler could be changed to use the specific names. Not
> >> a fan of this in any case.
> >>
> >>>    		CameraManager *cm = pipe->cameraManager();
> >>>    		IPAManager *self = cm->_d()->ipaManager();
> >>> -		IPAModule *m = self->module(pipe, minVersion, maxVersion);
> >>> +		IPAModule *m = self->module(name, minVersion, maxVersion);
> >>>    		if (!m)
> >>>    			return nullptr;
> >>> @@ -60,6 +61,14 @@ public:
> >>>    		return proxy;
> >>>    	}
> >>> +	template<typename T>
> >>> +	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> >>> +					    uint32_t minVersion,
> >>> +					    uint32_t maxVersion)
> >>> +	{
> >>> +		return createIPA<T>(pipe, pipe->name(), minVersion, maxVersion);
> >>> +	}
> >>> +
> >>>    #if HAVE_IPA_PUBKEY
> >>>    	static const PubKey &pubKey()
> >>>    	{
> >>> @@ -72,7 +81,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 *name, 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 15f19492c..a0a53764e 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(const char *name, uint32_t minVersion,
> >>> +		   uint32_t maxVersion) const;
> >>>    protected:
> >>>    	std::string logPrefix() const override;
> >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> >>> index 35171d097..f62a4ee5f 100644
> >>> --- a/src/libcamera/ipa_manager.cpp
> >>> +++ b/src/libcamera/ipa_manager.cpp
> >>> @@ -247,15 +247,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] name The IPA module string identifier
> >>>     * \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 *name, uint32_t minVersion,
> >>>    			      uint32_t maxVersion)
> >>>    {
> >>>    	for (const auto &module : modules_) {
> >>> -		if (module->match(pipe, minVersion, maxVersion))
> >>> +		if (module->match(name, minVersion, maxVersion))
> >>>    			return module.get();
> >>>    	}
> >>> @@ -263,12 +263,34 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
> >>>    }
> >>>    /**
> >>> - * \fn IPAManager::createIPA()
> >>> - * \brief Create an IPA proxy that matches a given pipeline handler
> >>> - * \param[in] pipe The pipeline handler that wants a matching IPA proxy
> >>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, const char *ipaName, uint32_t minVersion, uint32_t maxVersion)
> >>> + * \brief Create an IPA proxy that matches the requested name and version
> >>> + * \param[in] pipe The pipeline handler that wants to create the IPA module
> >>> + * \param[in] ipaName The IPA module name
> >>>     * \param[in] minVersion Minimum acceptable version of IPA module
> >>>     * \param[in] maxVersion Maximum acceptable version of IPA module
> >>>     *
> >>> + * Create an IPA module using \a name as the matching identifier. This overload
> >>> + * allows pipeline handlers to create an IPA module by specifying its name
> >>> + * instead of relying on the fact that the IPA module matches the pipeline
> >>> + * handler's one.
> >>> + *
> >>> + * \return A newly created IPA proxy, or nullptr if no matching IPA module is
> >>> + * found or if the IPA proxy fails to initialize
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, uint32_t minVersion, uint32_t maxVersion)
> >>> + * \brief Create an IPA proxy that matches the pipeline handler name and the
> >>> + * requested version
> >>> + * \param[in] pipe The pipeline handler that wants to create the IPA module
> >>> + * \param[in] minVersion Minimum acceptable version of IPA module
> >>> + * \param[in] maxVersion Maximum acceptable version of IPA module
> >>> + *
> >>> + * Create an IPA module using the pipeline handler name as the matching
> >>> + * identifier. This overload allows pipeline handler to create an IPA module
> >>> + * whose name matches the pipeline handler one.
> >>> + *
> >>>     * \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 e6ea61e44..0bd6f1462 100644
> >>> --- a/src/libcamera/ipa_module.cpp
> >>> +++ b/src/libcamera/ipa_module.cpp
> >>> @@ -463,21 +463,21 @@ IPAInterface *IPAModule::createInterface()
> >>>    /**
> >>>     * \brief Verify if the IPA module matches a given pipeline handler
> >>> - * \param[in] pipe Pipeline handler to match with
> >>> + * \param[in] name The IPA module name
> >>>     * \param[in] minVersion Minimum acceptable version of IPA module
> >>>     * \param[in] maxVersion Maximum acceptable version of IPA module
> >>>     *
> >>> - * This function checks if this IPA module matches the \a pipe pipeline handler,
> >>> + * This function checks if this IPA module matches the requested \a name
> >>>     * and the input version range.
> >>>     *
> >>> - * \return True if the pipeline handler matches the IPA module, or false otherwise
> >>> + * \return True if the IPA module matches, or false otherwise
> >>>     */
> >>> -bool IPAModule::match(PipelineHandler *pipe,
> >>> -		      uint32_t minVersion, uint32_t maxVersion) const
> >>> +bool IPAModule::match(const char *name, uint32_t minVersion,
> >>> +		      uint32_t maxVersion) const
> >>>    {
> >>>    	return info_.pipelineVersion >= minVersion &&
> >>>    	       info_.pipelineVersion <= maxVersion &&
> >>> -	       !strcmp(info_.pipelineName, pipe->name());
> >>> +	       !strcmp(info_.name, name);
> >>>    }
> >>>    std::string IPAModule::logPrefix() const
Naushir Patuck May 14, 2026, 7:57 a.m. UTC | #5
Hi Laurent,

On Thu, 14 May 2026 at 00:56, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> CC'ing Naush and David.
>
> On Mon, Apr 13, 2026 at 03:55:58PM +0200, Barnabás Pőcze wrote:
> > 2026. 04. 13. 15:32 keltezéssel, Jacopo Mondi írta:
> > > On Mon, Apr 13, 2026 at 12:21:59PM +0200, Barnabás Pőcze wrote:
> > >> 2026. 04. 08. 13:56 keltezéssel, Hans de Goede írta:
> > >>> 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 than 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, move the IPA creation procedure to be name-based,
> introducing
> > >>> an overload to IPAManager::createIPA(pipe, name, minVer, maxVer)
> that
> > >>> allows to specify the name of the IPA module to match. Pipeline
> handlers
> > >>> that wants to use their name as matching criteria can continue doing
> so
> > >>> using the already existing createIPA(pipe, minVer, maxVer) overload.
> > >>>
> > >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >>> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> > >>> ---
> > >>>    include/libcamera/internal/ipa_manager.h | 13 +++++++--
> > >>>    include/libcamera/internal/ipa_module.h  |  4 +--
> > >>>    src/libcamera/ipa_manager.cpp            | 34
> +++++++++++++++++++-----
> > >>>    src/libcamera/ipa_module.cpp             | 12 ++++-----
> > >>>    4 files changed, 47 insertions(+), 16 deletions(-)
> > >>>
> > >>> diff --git a/include/libcamera/internal/ipa_manager.h
> b/include/libcamera/internal/ipa_manager.h
> > >>> index f8ce78016..4c01e76f1 100644
> > >>> --- a/include/libcamera/internal/ipa_manager.h
> > >>> +++ b/include/libcamera/internal/ipa_manager.h
> > >>> @@ -34,12 +34,13 @@ public:
> > >>>           template<typename T>
> > >>>           static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> > >>> +                                     const char *name,
> > >>>                                               uint32_t minVersion,
> > >>>                                               uint32_t maxVersion)
> > >>>           {
> > >>
> > >> I agree with this change, but I wish that the name could be derived
> from `T`.
> > >> And that is quite easy to do, just adding a
> > >>
> > >>    static constexpr const char *name() { return "..."; }
> > >>
> > >> and then using `T::name()` here works mostly.
> > >>
> > >> But unfortunately the raspberry pi ipa modules are not compatible at
> the moment
> > >> with this approach. I have tried to come up with something, but so
> for has failed.
> > >
> > > What is the issue ? That the proxy is the same for v4 and v5 ?
> >
> > The proxy type is the same, but different names need to be used.
>
> The more I think about it, the more uncomfortable I get with specifying
> a name here.
>
> The API contract between the pipeline handler is the IPA interface.
> That's all we should have to specify in the pipeline handler. Adding a
> name gives the pipeline handler the ability to select any IPA module,
> without any check whatsoever to ensure compatibility of the interface.
> That sounds like a very bad idea.
>
> Raspberry Pi is the only platform where we have two IPA modules using
> the same IPA interface without being interchangeable. Trying to use the
> Pi4 IPA module on a Pi5 will not work, and vice versa. I would really
> like to fix that, and I see two options:
>
> - Merging the two IPA modules into a single one, compatible with both
>   platforms. The IPA interface functions in the IPA module would
>   essentially dispatch to Pi4 or Pi5 implementations, based on a
>   platform version identifier (passed to the init() function I suppose).
>
> - Splitting the IPA interface into two different interfaces. The
>   interfaces can still share the same API, the important part is to have
>   two different interface names in two .mojom files. All the data
>   structures can be defined in a shared .mojom file imported by both
>   interfaces (and maybe there's a way to also share a common interface
>   definition instead of duplicating it).
>
> The drawback of the first option is that the IPA module will contain
> dead code (Pi4 code won't run on Pi5 and vice versa), while the drawback
> of the second option is that we'll duplicate proxy and worker code (as
> the code is generated, it's just a matter of binary size, there's no
> additional maintenance burden there).
>
> Naush, David, any opinion ?
>

I would opt for the second option, i.e. splitting the IPA interface in this
case.  There will be quite a lot of duplicate code, but our IPAs are pretty
stable at this point so maintaining fixes/changes across both should be
manageable.

Would you like me to prototype this?  I'll likely only have a chance to do
this after the Nice F2F though.

Naush



>
>
> Note that we would still support multiple intercheangeable IPA modules
> using the same interface (and therefore compatible with the same
> pipeline handler).
>
> > >> Maybe the `name` argument could be kept like this:
> > >>
> > >>    const char *name = T::name()
> > >>
> > >> and then the rpi pipeline handler could be changed to use the
> specific names. Not
> > >> a fan of this in any case.
> > >>
> > >>>                   CameraManager *cm = pipe->cameraManager();
> > >>>                   IPAManager *self = cm->_d()->ipaManager();
> > >>> -         IPAModule *m = self->module(pipe, minVersion, maxVersion);
> > >>> +         IPAModule *m = self->module(name, minVersion, maxVersion);
> > >>>                   if (!m)
> > >>>                           return nullptr;
> > >>> @@ -60,6 +61,14 @@ public:
> > >>>                   return proxy;
> > >>>           }
> > >>> + template<typename T>
> > >>> + static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> > >>> +                                     uint32_t minVersion,
> > >>> +                                     uint32_t maxVersion)
> > >>> + {
> > >>> +         return createIPA<T>(pipe, pipe->name(), minVersion,
> maxVersion);
> > >>> + }
> > >>> +
> > >>>    #if HAVE_IPA_PUBKEY
> > >>>           static const PubKey &pubKey()
> > >>>           {
> > >>> @@ -72,7 +81,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 *name, 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 15f19492c..a0a53764e 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(const char *name, uint32_t minVersion,
> > >>> +            uint32_t maxVersion) const;
> > >>>    protected:
> > >>>           std::string logPrefix() const override;
> > >>> diff --git a/src/libcamera/ipa_manager.cpp
> b/src/libcamera/ipa_manager.cpp
> > >>> index 35171d097..f62a4ee5f 100644
> > >>> --- a/src/libcamera/ipa_manager.cpp
> > >>> +++ b/src/libcamera/ipa_manager.cpp
> > >>> @@ -247,15 +247,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] name The IPA module string identifier
> > >>>     * \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 *name, uint32_t minVersion,
> > >>>                                 uint32_t maxVersion)
> > >>>    {
> > >>>           for (const auto &module : modules_) {
> > >>> -         if (module->match(pipe, minVersion, maxVersion))
> > >>> +         if (module->match(name, minVersion, maxVersion))
> > >>>                           return module.get();
> > >>>           }
> > >>> @@ -263,12 +263,34 @@ IPAModule *IPAManager::module(PipelineHandler
> *pipe, uint32_t minVersion,
> > >>>    }
> > >>>    /**
> > >>> - * \fn IPAManager::createIPA()
> > >>> - * \brief Create an IPA proxy that matches a given pipeline handler
> > >>> - * \param[in] pipe The pipeline handler that wants a matching IPA
> proxy
> > >>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, const char
> *ipaName, uint32_t minVersion, uint32_t maxVersion)
> > >>> + * \brief Create an IPA proxy that matches the requested name and
> version
> > >>> + * \param[in] pipe The pipeline handler that wants to create the
> IPA module
> > >>> + * \param[in] ipaName The IPA module name
> > >>>     * \param[in] minVersion Minimum acceptable version of IPA module
> > >>>     * \param[in] maxVersion Maximum acceptable version of IPA module
> > >>>     *
> > >>> + * Create an IPA module using \a name as the matching identifier.
> This overload
> > >>> + * allows pipeline handlers to create an IPA module by specifying
> its name
> > >>> + * instead of relying on the fact that the IPA module matches the
> pipeline
> > >>> + * handler's one.
> > >>> + *
> > >>> + * \return A newly created IPA proxy, or nullptr if no matching IPA
> module is
> > >>> + * found or if the IPA proxy fails to initialize
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, uint32_t
> minVersion, uint32_t maxVersion)
> > >>> + * \brief Create an IPA proxy that matches the pipeline handler
> name and the
> > >>> + * requested version
> > >>> + * \param[in] pipe The pipeline handler that wants to create the
> IPA module
> > >>> + * \param[in] minVersion Minimum acceptable version of IPA module
> > >>> + * \param[in] maxVersion Maximum acceptable version of IPA module
> > >>> + *
> > >>> + * Create an IPA module using the pipeline handler name as the
> matching
> > >>> + * identifier. This overload allows pipeline handler to create an
> IPA module
> > >>> + * whose name matches the pipeline handler one.
> > >>> + *
> > >>>     * \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 e6ea61e44..0bd6f1462 100644
> > >>> --- a/src/libcamera/ipa_module.cpp
> > >>> +++ b/src/libcamera/ipa_module.cpp
> > >>> @@ -463,21 +463,21 @@ IPAInterface *IPAModule::createInterface()
> > >>>    /**
> > >>>     * \brief Verify if the IPA module matches a given pipeline
> handler
> > >>> - * \param[in] pipe Pipeline handler to match with
> > >>> + * \param[in] name The IPA module name
> > >>>     * \param[in] minVersion Minimum acceptable version of IPA module
> > >>>     * \param[in] maxVersion Maximum acceptable version of IPA module
> > >>>     *
> > >>> - * This function checks if this IPA module matches the \a pipe
> pipeline handler,
> > >>> + * This function checks if this IPA module matches the requested \a
> name
> > >>>     * and the input version range.
> > >>>     *
> > >>> - * \return True if the pipeline handler matches the IPA module, or
> false otherwise
> > >>> + * \return True if the IPA module matches, or false otherwise
> > >>>     */
> > >>> -bool IPAModule::match(PipelineHandler *pipe,
> > >>> -               uint32_t minVersion, uint32_t maxVersion) const
> > >>> +bool IPAModule::match(const char *name, uint32_t minVersion,
> > >>> +               uint32_t maxVersion) const
> > >>>    {
> > >>>           return info_.pipelineVersion >= minVersion &&
> > >>>                  info_.pipelineVersion <= maxVersion &&
> > >>> -        !strcmp(info_.pipelineName, pipe->name());
> > >>> +        !strcmp(info_.name, name);
> > >>>    }
> > >>>    std::string IPAModule::logPrefix() const
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart May 14, 2026, 8:32 a.m. UTC | #6
On Thu, May 14, 2026 at 08:57:12AM +0100, Naushir Patuck wrote:
> On Thu, 14 May 2026 at 00:56, Laurent Pinchart wrote:
> > CC'ing Naush and David.
> >
> > On Mon, Apr 13, 2026 at 03:55:58PM +0200, Barnabás Pőcze wrote:
> > > 2026. 04. 13. 15:32 keltezéssel, Jacopo Mondi írta:
> > > > On Mon, Apr 13, 2026 at 12:21:59PM +0200, Barnabás Pőcze wrote:
> > > >> 2026. 04. 08. 13:56 keltezéssel, Hans de Goede írta:
> > > >>> 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 than 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, move the IPA creation procedure to be name-based, introducing
> > > >>> an overload to IPAManager::createIPA(pipe, name, minVer, maxVer) that
> > > >>> allows to specify the name of the IPA module to match. Pipeline handlers
> > > >>> that wants to use their name as matching criteria can continue doing so
> > > >>> using the already existing createIPA(pipe, minVer, maxVer) overload.
> > > >>>
> > > >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > >>> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > >>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> > > >>> ---
> > > >>>    include/libcamera/internal/ipa_manager.h | 13 +++++++--
> > > >>>    include/libcamera/internal/ipa_module.h  |  4 +--
> > > >>>    src/libcamera/ipa_manager.cpp            | 34 +++++++++++++++++++-----
> > > >>>    src/libcamera/ipa_module.cpp             | 12 ++++-----
> > > >>>    4 files changed, 47 insertions(+), 16 deletions(-)
> > > >>>
> > > >>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> > > >>> index f8ce78016..4c01e76f1 100644
> > > >>> --- a/include/libcamera/internal/ipa_manager.h
> > > >>> +++ b/include/libcamera/internal/ipa_manager.h
> > > >>> @@ -34,12 +34,13 @@ public:
> > > >>>           template<typename T>
> > > >>>           static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> > > >>> +                                     const char *name,
> > > >>>                                               uint32_t minVersion,
> > > >>>                                               uint32_t maxVersion)
> > > >>>           {
> > > >>
> > > >> I agree with this change, but I wish that the name could be derived from `T`.
> > > >> And that is quite easy to do, just adding a
> > > >>
> > > >>    static constexpr const char *name() { return "..."; }
> > > >>
> > > >> and then using `T::name()` here works mostly.
> > > >>
> > > >> But unfortunately the raspberry pi ipa modules are not compatible at the moment
> > > >> with this approach. I have tried to come up with something, but so for has failed.
> > > >
> > > > What is the issue ? That the proxy is the same for v4 and v5 ?
> > >
> > > The proxy type is the same, but different names need to be used.
> >
> > The more I think about it, the more uncomfortable I get with specifying
> > a name here.
> >
> > The API contract between the pipeline handler is the IPA interface.
> > That's all we should have to specify in the pipeline handler. Adding a
> > name gives the pipeline handler the ability to select any IPA module,
> > without any check whatsoever to ensure compatibility of the interface.
> > That sounds like a very bad idea.
> >
> > Raspberry Pi is the only platform where we have two IPA modules using
> > the same IPA interface without being interchangeable. Trying to use the
> > Pi4 IPA module on a Pi5 will not work, and vice versa. I would really
> > like to fix that, and I see two options:
> >
> > - Merging the two IPA modules into a single one, compatible with both
> >   platforms. The IPA interface functions in the IPA module would
> >   essentially dispatch to Pi4 or Pi5 implementations, based on a
> >   platform version identifier (passed to the init() function I suppose).
> >
> > - Splitting the IPA interface into two different interfaces. The
> >   interfaces can still share the same API, the important part is to have
> >   two different interface names in two .mojom files. All the data
> >   structures can be defined in a shared .mojom file imported by both
> >   interfaces (and maybe there's a way to also share a common interface
> >   definition instead of duplicating it).
> >
> > The drawback of the first option is that the IPA module will contain
> > dead code (Pi4 code won't run on Pi5 and vice versa), while the drawback
> > of the second option is that we'll duplicate proxy and worker code (as
> > the code is generated, it's just a matter of binary size, there's no
> > additional maintenance burden there).
> >
> > Naush, David, any opinion ?
> >
> 
> I would opt for the second option, i.e. splitting the IPA interface in this
> case.  There will be quite a lot of duplicate code, but our IPAs are pretty
> stable at this point so maintaining fixes/changes across both should be
> manageable.
> 
> Would you like me to prototype this?  I'll likely only have a chance to do
> this after the Nice F2F though.

I would appreciate that, thank you. I gave it a quick try last night,
and the IPABase class was in the way as it inherits from
IPARPiInterface. We would need to move the inheritance to individual IPA
modules, but you can still use an IPABase helper that contains the
shared code, as long as it doesn't inherit from the interface class.
That likely means that some of the platform*() functions could be removed.

> > Note that we would still support multiple intercheangeable IPA modules
> > using the same interface (and therefore compatible with the same
> > pipeline handler).
> >
> > > >> Maybe the `name` argument could be kept like this:
> > > >>
> > > >>    const char *name = T::name()
> > > >>
> > > >> and then the rpi pipeline handler could be changed to use the specific names. Not
> > > >> a fan of this in any case.
> > > >>
> > > >>>                   CameraManager *cm = pipe->cameraManager();
> > > >>>                   IPAManager *self = cm->_d()->ipaManager();
> > > >>> -         IPAModule *m = self->module(pipe, minVersion, maxVersion);
> > > >>> +         IPAModule *m = self->module(name, minVersion, maxVersion);
> > > >>>                   if (!m)
> > > >>>                           return nullptr;
> > > >>> @@ -60,6 +61,14 @@ public:
> > > >>>                   return proxy;
> > > >>>           }
> > > >>> + template<typename T>
> > > >>> + static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
> > > >>> +                                     uint32_t minVersion,
> > > >>> +                                     uint32_t maxVersion)
> > > >>> + {
> > > >>> +         return createIPA<T>(pipe, pipe->name(), minVersion, maxVersion);
> > > >>> + }
> > > >>> +
> > > >>>    #if HAVE_IPA_PUBKEY
> > > >>>           static const PubKey &pubKey()
> > > >>>           {
> > > >>> @@ -72,7 +81,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 *name, 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 15f19492c..a0a53764e 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(const char *name, uint32_t minVersion,
> > > >>> +            uint32_t maxVersion) const;
> > > >>>    protected:
> > > >>>           std::string logPrefix() const override;
> > > >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> > > >>> index 35171d097..f62a4ee5f 100644
> > > >>> --- a/src/libcamera/ipa_manager.cpp
> > > >>> +++ b/src/libcamera/ipa_manager.cpp
> > > >>> @@ -247,15 +247,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] name The IPA module string identifier
> > > >>>     * \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 *name, uint32_t minVersion,
> > > >>>                                 uint32_t maxVersion)
> > > >>>    {
> > > >>>           for (const auto &module : modules_) {
> > > >>> -         if (module->match(pipe, minVersion, maxVersion))
> > > >>> +         if (module->match(name, minVersion, maxVersion))
> > > >>>                           return module.get();
> > > >>>           }
> > > >>> @@ -263,12 +263,34 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
> > > >>>    }
> > > >>>    /**
> > > >>> - * \fn IPAManager::createIPA()
> > > >>> - * \brief Create an IPA proxy that matches a given pipeline handler
> > > >>> - * \param[in] pipe The pipeline handler that wants a matching IPA proxy
> > > >>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, const char *ipaName, uint32_t minVersion, uint32_t maxVersion)
> > > >>> + * \brief Create an IPA proxy that matches the requested name and version
> > > >>> + * \param[in] pipe The pipeline handler that wants to create the IPA module
> > > >>> + * \param[in] ipaName The IPA module name
> > > >>>     * \param[in] minVersion Minimum acceptable version of IPA module
> > > >>>     * \param[in] maxVersion Maximum acceptable version of IPA module
> > > >>>     *
> > > >>> + * Create an IPA module using \a name as the matching identifier. This overload
> > > >>> + * allows pipeline handlers to create an IPA module by specifying its name
> > > >>> + * instead of relying on the fact that the IPA module matches the pipeline
> > > >>> + * handler's one.
> > > >>> + *
> > > >>> + * \return A newly created IPA proxy, or nullptr if no matching IPA module is
> > > >>> + * found or if the IPA proxy fails to initialize
> > > >>> + */
> > > >>> +
> > > >>> +/**
> > > >>> + * \fn IPAManager::createIPA(PipelineHandler *pipe, uint32_t minVersion, uint32_t maxVersion)
> > > >>> + * \brief Create an IPA proxy that matches the pipeline handler name and the
> > > >>> + * requested version
> > > >>> + * \param[in] pipe The pipeline handler that wants to create the IPA module
> > > >>> + * \param[in] minVersion Minimum acceptable version of IPA module
> > > >>> + * \param[in] maxVersion Maximum acceptable version of IPA module
> > > >>> + *
> > > >>> + * Create an IPA module using the pipeline handler name as the matching
> > > >>> + * identifier. This overload allows pipeline handler to create an IPA module
> > > >>> + * whose name matches the pipeline handler one.
> > > >>> + *
> > > >>>     * \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 e6ea61e44..0bd6f1462 100644
> > > >>> --- a/src/libcamera/ipa_module.cpp
> > > >>> +++ b/src/libcamera/ipa_module.cpp
> > > >>> @@ -463,21 +463,21 @@ IPAInterface *IPAModule::createInterface()
> > > >>>    /**
> > > >>>     * \brief Verify if the IPA module matches a given pipeline handler
> > > >>> - * \param[in] pipe Pipeline handler to match with
> > > >>> + * \param[in] name The IPA module name
> > > >>>     * \param[in] minVersion Minimum acceptable version of IPA module
> > > >>>     * \param[in] maxVersion Maximum acceptable version of IPA module
> > > >>>     *
> > > >>> - * This function checks if this IPA module matches the \a pipe pipeline handler,
> > > >>> + * This function checks if this IPA module matches the requested \a name
> > > >>>     * and the input version range.
> > > >>>     *
> > > >>> - * \return True if the pipeline handler matches the IPA module, or false otherwise
> > > >>> + * \return True if the IPA module matches, or false otherwise
> > > >>>     */
> > > >>> -bool IPAModule::match(PipelineHandler *pipe,
> > > >>> -               uint32_t minVersion, uint32_t maxVersion) const
> > > >>> +bool IPAModule::match(const char *name, uint32_t minVersion,
> > > >>> +               uint32_t maxVersion) const
> > > >>>    {
> > > >>>           return info_.pipelineVersion >= minVersion &&
> > > >>>                  info_.pipelineVersion <= maxVersion &&
> > > >>> -        !strcmp(info_.pipelineName, pipe->name());
> > > >>> +        !strcmp(info_.name, name);
> > > >>>    }
> > > >>>    std::string IPAModule::logPrefix() const
Hans de Goede May 15, 2026, 7:37 a.m. UTC | #7
Hi Laurent,

On 14-May-26 01:56, Laurent Pinchart wrote:
> CC'ing Naush and David.
> 
> On Mon, Apr 13, 2026 at 03:55:58PM +0200, Barnabás Pőcze wrote:
>> 2026. 04. 13. 15:32 keltezéssel, Jacopo Mondi írta:
>>> On Mon, Apr 13, 2026 at 12:21:59PM +0200, Barnabás Pőcze wrote:
>>>> 2026. 04. 08. 13:56 keltezéssel, Hans de Goede írta:
>>>>> 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 than 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, move the IPA creation procedure to be name-based, introducing
>>>>> an overload to IPAManager::createIPA(pipe, name, minVer, maxVer)  that
>>>>> allows to specify the name of the IPA module to match. Pipeline handlers
>>>>> that wants to use their name as matching criteria can continue doing so
>>>>> using the already existing createIPA(pipe, minVer, maxVer) overload.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
>>>>> ---
>>>>>    include/libcamera/internal/ipa_manager.h | 13 +++++++--
>>>>>    include/libcamera/internal/ipa_module.h  |  4 +--
>>>>>    src/libcamera/ipa_manager.cpp            | 34 +++++++++++++++++++-----
>>>>>    src/libcamera/ipa_module.cpp             | 12 ++++-----
>>>>>    4 files changed, 47 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>>>>> index f8ce78016..4c01e76f1 100644
>>>>> --- a/include/libcamera/internal/ipa_manager.h
>>>>> +++ b/include/libcamera/internal/ipa_manager.h
>>>>> @@ -34,12 +34,13 @@ public:
>>>>>    	template<typename T>
>>>>>    	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
>>>>> +					    const char *name,
>>>>>    					    uint32_t minVersion,
>>>>>    					    uint32_t maxVersion)
>>>>>    	{
>>>>
>>>> I agree with this change, but I wish that the name could be derived from `T`.
>>>> And that is quite easy to do, just adding a
>>>>
>>>>    static constexpr const char *name() { return "..."; }
>>>>
>>>> and then using `T::name()` here works mostly.
>>>>
>>>> But unfortunately the raspberry pi ipa modules are not compatible at the moment
>>>> with this approach. I have tried to come up with something, but so for has failed.
>>>
>>> What is the issue ? That the proxy is the same for v4 and v5 ?
>>
>> The proxy type is the same, but different names need to be used.
> 
> The more I think about it, the more uncomfortable I get with specifying
> a name here.
> 
> The API contract between the pipeline handler is the IPA interface.
> That's all we should have to specify in the pipeline handler. Adding a
> name gives the pipeline handler the ability to select any IPA module,
> without any check whatsoever to ensure compatibility of the interface.
> That sounds like a very bad idea.

Yes we all agreed in the softISP meeting last Wednesday that eventually
we want to move to using the IPA interface type as the thing on which
we should match when loading an IPA.

I'm a bit confused here though now, we also agreed to move forward
with v5 of this patch-set for now, with a follow up patch to add
a TODO comment to move to using the IPA interface type in the future.

Is this a nack for moving forward with v5 as previously agreed on ?

Or did you just want to get the discussion on the TODO item started?

Regards,

Hans

Patch
diff mbox series

diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index f8ce78016..4c01e76f1 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -34,12 +34,13 @@  public:
 
 	template<typename T>
 	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
+					    const char *name,
 					    uint32_t minVersion,
 					    uint32_t maxVersion)
 	{
 		CameraManager *cm = pipe->cameraManager();
 		IPAManager *self = cm->_d()->ipaManager();
-		IPAModule *m = self->module(pipe, minVersion, maxVersion);
+		IPAModule *m = self->module(name, minVersion, maxVersion);
 		if (!m)
 			return nullptr;
 
@@ -60,6 +61,14 @@  public:
 		return proxy;
 	}
 
+	template<typename T>
+	static std::unique_ptr<T> createIPA(PipelineHandler *pipe,
+					    uint32_t minVersion,
+					    uint32_t maxVersion)
+	{
+		return createIPA<T>(pipe, pipe->name(), minVersion, maxVersion);
+	}
+
 #if HAVE_IPA_PUBKEY
 	static const PubKey &pubKey()
 	{
@@ -72,7 +81,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 *name, 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 15f19492c..a0a53764e 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(const char *name, uint32_t minVersion,
+		   uint32_t maxVersion) const;
 
 protected:
 	std::string logPrefix() const override;
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index 35171d097..f62a4ee5f 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -247,15 +247,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] name The IPA module string identifier
  * \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 *name, uint32_t minVersion,
 			      uint32_t maxVersion)
 {
 	for (const auto &module : modules_) {
-		if (module->match(pipe, minVersion, maxVersion))
+		if (module->match(name, minVersion, maxVersion))
 			return module.get();
 	}
 
@@ -263,12 +263,34 @@  IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
 }
 
 /**
- * \fn IPAManager::createIPA()
- * \brief Create an IPA proxy that matches a given pipeline handler
- * \param[in] pipe The pipeline handler that wants a matching IPA proxy
+ * \fn IPAManager::createIPA(PipelineHandler *pipe, const char *ipaName, uint32_t minVersion, uint32_t maxVersion)
+ * \brief Create an IPA proxy that matches the requested name and version
+ * \param[in] pipe The pipeline handler that wants to create the IPA module
+ * \param[in] ipaName The IPA module name
  * \param[in] minVersion Minimum acceptable version of IPA module
  * \param[in] maxVersion Maximum acceptable version of IPA module
  *
+ * Create an IPA module using \a name as the matching identifier. This overload
+ * allows pipeline handlers to create an IPA module by specifying its name
+ * instead of relying on the fact that the IPA module matches the pipeline
+ * handler's one.
+ *
+ * \return A newly created IPA proxy, or nullptr if no matching IPA module is
+ * found or if the IPA proxy fails to initialize
+ */
+
+/**
+ * \fn IPAManager::createIPA(PipelineHandler *pipe, uint32_t minVersion, uint32_t maxVersion)
+ * \brief Create an IPA proxy that matches the pipeline handler name and the
+ * requested version
+ * \param[in] pipe The pipeline handler that wants to create the IPA module
+ * \param[in] minVersion Minimum acceptable version of IPA module
+ * \param[in] maxVersion Maximum acceptable version of IPA module
+ *
+ * Create an IPA module using the pipeline handler name as the matching
+ * identifier. This overload allows pipeline handler to create an IPA module
+ * whose name matches the pipeline handler one.
+ *
  * \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 e6ea61e44..0bd6f1462 100644
--- a/src/libcamera/ipa_module.cpp
+++ b/src/libcamera/ipa_module.cpp
@@ -463,21 +463,21 @@  IPAInterface *IPAModule::createInterface()
 
 /**
  * \brief Verify if the IPA module matches a given pipeline handler
- * \param[in] pipe Pipeline handler to match with
+ * \param[in] name The IPA module name
  * \param[in] minVersion Minimum acceptable version of IPA module
  * \param[in] maxVersion Maximum acceptable version of IPA module
  *
- * This function checks if this IPA module matches the \a pipe pipeline handler,
+ * This function checks if this IPA module matches the requested \a name
  * and the input version range.
  *
- * \return True if the pipeline handler matches the IPA module, or false otherwise
+ * \return True if the IPA module matches, or false otherwise
  */
-bool IPAModule::match(PipelineHandler *pipe,
-		      uint32_t minVersion, uint32_t maxVersion) const
+bool IPAModule::match(const char *name, uint32_t minVersion,
+		      uint32_t maxVersion) const
 {
 	return info_.pipelineVersion >= minVersion &&
 	       info_.pipelineVersion <= maxVersion &&
-	       !strcmp(info_.pipelineName, pipe->name());
+	       !strcmp(info_.name, name);
 }
 
 std::string IPAModule::logPrefix() const