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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart