[libcamera-devel,2/3] libcamera: ipa_manager: Split common code out of createIPA()
diff mbox series

Message ID 20210711231547.19664-3-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: ipa_manager: Fix, cleanup, and allow forcing isolation
Related show

Commit Message

Laurent Pinchart July 11, 2021, 11:15 p.m. UTC 
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@ideasonboard.com>
---
 include/libcamera/internal/ipa_manager.h | 13 ++++---------
 src/libcamera/ipa_manager.cpp            | 17 +++++++++++++++++
 2 files changed, 21 insertions(+), 9 deletions(-)

Comments

Kieran Bingham July 12, 2021, 7:50 a.m. UTC | #1 
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@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?



> + * \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
>
Kieran Bingham July 12, 2021, 7:56 a.m. UTC | #2 
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@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@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
>>
Laurent Pinchart July 12, 2021, 9:59 p.m. UTC | #3 
Hi Kieran,

On Mon, Jul 12, 2021 at 08:50:14AM +0100, Kieran Bingham wrote:
> 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@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

s/and/an/

> 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?

I think so, but if I wrote "the first" here, I'd have to define an
order, and there's no defined ordered :-) That's why I went for "an IPA
module". Note that this is a private function, so the documentation
won't be compiled.

I think this should be addressed when we'll work on IPA module selection
among multiple options.

> > + * \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
Paul Elder July 19, 2021, 3:19 a.m. UTC | #4 
Hi Kieran and Laurent,

On Tue, Jul 13, 2021 at 12:59:09AM +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Jul 12, 2021 at 08:50:14AM +0100, Kieran Bingham wrote:
> > 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@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
> 
> s/and/an/
> 
> > This actually finds the 'first' matching IPA module (for however the
> > list is sorted).

Yeah, that's the behavior that we had in the first place.

> > 
> > We can already have multiple potentially matching IPA modules available
> > on a system. Should this be more explicit on what is matched?
> 
> I think so, but if I wrote "the first" here, I'd have to define an
> order, and there's no defined ordered :-) That's why I went for "an IPA
> module". Note that this is a private function, so the documentation
> won't be compiled.
> 
> I think this should be addressed when we'll work on IPA module selection
> among multiple options.

iirc, this was set as a todo and we just took the "first" (of whatever
order) IPA "for now".

And now that we finally have multiple IPAs it has to be addressed :S


Anyway, for this patch,

Reviewed-by: Paul Elder <paul.elder@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

Patch
diff mbox series 

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
+ * \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