[libcamera-devel] [PATCH v2 05/10] libcamera: ipa_module: match IPA module with pipeline handler

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Jun 03, 2019 at 07:16:32PM -0400, Paul Elder wrote:
> Add a method to IPAModule to check if it matches with a given pipeline
> handler.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> New patch, but this IPAModule::match method used to be IPAManager::match
> 
>  src/libcamera/include/ipa_module.h |  4 ++++
>  src/libcamera/ipa_module.cpp       | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h
> index 557435c..48ff2b6 100644
> --- a/src/libcamera/include/ipa_module.h
> +++ b/src/libcamera/include/ipa_module.h
> @@ -13,6 +13,8 @@
>  #include <libcamera/ipa/ipa_interface.h>
>  #include <libcamera/ipa/ipa_module_info.h>
>  
> +#include "pipeline_handler.h"

You can forward-declare class PipelineHandler instead of including this
file.

> +
>  namespace libcamera {
>  
>  class IPAModule
> @@ -29,6 +31,8 @@ public:
>  
>  	std::unique_ptr<IPAInterface> createInstance();
>  
> +	bool match(PipelineHandler *pipe) const;
> +
>  private:
>  	struct IPAModuleInfo info_;
>  
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 91d97ae..c1b04fe 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -18,6 +18,7 @@
>  #include <unistd.h>
>  
>  #include "log.h"
> +#include "pipeline_handler.h"
>  
>  /**
>   * \file ipa_module.h
> @@ -417,4 +418,22 @@ std::unique_ptr<IPAInterface> IPAModule::createInstance()
>  	return std::unique_ptr<IPAInterface>(ipaCreate_());
>  }
>  
> +/**
> + * \brief Match this IPAModule with a PipelineHandler

"Verify if the IPA module matches a given pipeline handler" ?

> + * \param[in] pipe Pipeline handler to match with
> + *
> + * Checks if this IPA module matches the \a pipe pipeline handler.

"This method checks" or "Check". I'd go for the former.

> + * Matching is based on the fields of the IPA module info, and the
> + * corresponding attributes of the pipeline handler, such as pipeline version
> + * and name.
> + *
> + * \return true if the pipeline handler matches the IPA module, false otherwise

s/true/True/
s/false/ or false/

> + */
> +bool IPAModule::match(PipelineHandler *pipe) const
> +{
> +	return info_.moduleAPIVersion == IPA_MODULE_API_VERSION &&

I would move the first check to IPAModule::loadIPAModuleInfo(), as an
invalid module API version means we can't interpret the contents of the
structure, so we can use the module at all.

> +	       info_.pipelineVersion == pipe->version() &&

I was about to write that we should match the major exactly and the
minor with a minimum required version, but given that we're still
discussing how to handle IPA versioning, would it make sense to drop the
comments that explain how major/minor versions work in the other
patches, and just require an exact match for now ? It will make this
series a bit simpler, and avoid blocking it until we sort out the
versioning scheme. What do you think ? If you agree, with the small
issues above fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	       !strcmp(info_.pipelineName, pipe->name());
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart