[libcamera-devel] [PATCH 2/3] libcamera: ipa_manager: Split common code out of createIPA()

[Kieran Bingham]

On 12/07/2021 08:50, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 12/07/2021 00:15, Laurent Pinchart wrote:
>> The createIPA() template function starts with code that doesn't depend
>> on the template parameters. Split it to a non-template function to avoid
>> code duplication in the binary.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>>  include/libcamera/internal/ipa_manager.h | 13 ++++---------
>>  src/libcamera/ipa_manager.cpp            | 17 +++++++++++++++++
>>  2 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>> index 42201839901b..0687842e5c06 100644
>> --- a/include/libcamera/internal/ipa_manager.h
>> +++ b/include/libcamera/internal/ipa_manager.h
>> @@ -34,15 +34,7 @@ public:
>>  					    uint32_t minVersion,
>>  					    uint32_t maxVersion)
>>  	{
>> -		IPAModule *m = nullptr;
>> -
>> -		for (IPAModule *module : self_->modules_) {
>> -			if (module->match(pipe, minVersion, maxVersion)) {
>> -				m = module;
>> -				break;
>> -			}
>> -		}
>> -
>> +		IPAModule *m = self_->module(pipe, minVersion, maxVersion);
>>  		if (!m)
>>  			return nullptr;
>>  
>> @@ -62,6 +54,9 @@ private:
>>  		      std::vector<std::string> &files);
>>  	unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);
>>  
>> +	IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,
>> +			  uint32_t maxVersion);
>> +
>>  	bool isSignatureValid(IPAModule *ipa) const;
>>  
>>  	std::vector<IPAModule *> modules_;
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index b4606c6159e5..9533c8fadea6 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -245,6 +245,23 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>>  	return count;
>>  }
>>  
>> +/**
>> + * \brief Retrieve and IPA module that matches a given pipeline handler
> 
> This actually finds the 'first' matching IPA module (for however the
> list is sorted).
> 
> We can already have multiple potentially matching IPA modules available
> on a system. Should this be more explicit on what is matched?

This wasn't an objection, just a comment,

For the patch:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
>> + * \param[in] pipe The pipeline handler
>> + * \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,
>> +			      uint32_t maxVersion)
>> +{
>> +	for (IPAModule *module : modules_) {
>> +		if (module->match(pipe, minVersion, maxVersion))
>> +			return module;
>> +	}
>> +
>> +	return nullptr;
>> +}
>> +
>>  /**
>>   * \fn IPAManager::createIPA()
>>   * \brief Create an IPA proxy that matches a given pipeline handler
>>

-- 
Regards
--
Kieran