[libcamera-devel] [PATCH 2/3] libcamera: ipa_manager: Split common code out of createIPA()
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Mon Jul 19 05:19:10 CEST 2021
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 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).
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 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
More information about the libcamera-devel
mailing list