[libcamera-devel] [RFC PATCH v2 2/5] libcamera: ipa_module: add aquired attribute

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, May 23, 2019 at 12:42:07PM -0400, Paul Elder wrote:
> The IPAManager will be designed like an enumerator, and IPA modules
> cannot be used by multiple pipelines at once. Add an aquired attribute
> to IPAModule, and corresponding getter and setters.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - renamed acquired() to isAcquired(), to match isValid()
> - added documentation
> 
>  src/libcamera/include/ipa_module.h |  5 +++
>  src/libcamera/ipa_module.cpp       | 62 ++++++++++++++++++++++++++----
>  2 files changed, 60 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h
> index a4c6dbd..58faeca 100644
> --- a/src/libcamera/include/ipa_module.h
> +++ b/src/libcamera/include/ipa_module.h
> @@ -22,11 +22,16 @@ public:
>  
>  	const struct IPAModuleInfo &info() const;
>  
> +	bool isAcquired() const;
> +	bool acquire();
> +	void release();
> +
>  private:
>  	struct IPAModuleInfo info_;
>  
>  	std::string libPath_;
>  	bool valid_;
> +	bool acquired_;
>  
>  	int loadIPAModuleInfo();
>  };
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 86cbe71..4922e3e 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -176,14 +176,17 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>   * This structure contains the information of an IPA module. It is loaded,
>   * read, and validated before anything else is loaded from the shared object.
>   *

Ah, here is the documentation :-) Please move it to patch 1/5.

> - * \var IPAModuleInfo::name
> - * \brief The name of the IPA module
> + * \var IPAModuleInfo::ipaAPIVersion
> + * \brief The IPA API version that the IPA module was made with

You should also describe how the version is constructed. For this field,
I believe an plain integer is fine, and we will bump it every time the
IPAModuleInfo structure layout gets modified. The IPA module shall
report here the version that it was built for, and the ipa_module_info.h
header shall contain a macro with the current version number (starting
at 1).

>   *
> - * \var IPAModuleInfo::version
> - * \brief The version of the IPA module
> + * \var IPAModuleInfo::pipelineVersion
> + * \brief The pipeline version that the IPA module is for

For this field, however, I think we need a major.minor type of version.
I'm however not sure if the IPA module should report the version it has
been built for, or the minimal version it needs. A match would occur if
the major is identical, and the minor at least the requested one. In the
other direction the module would report an exact version, and the
pipeline would request a minimum version. What do you think would be
best ?

>   *
> - * \todo abi compatability version
> - * \todo pipeline compatability matcher
> + * \var IPAModuleInfo::pipelineName
> + * \brief The name of the pipeline that the IPA module is for
> + *

Please describe this in more details, at least stating where the
pipeline name comes from (and it should be pipeline handler, not
pipeline).

> + * \var IPAModuleInfo::name
> + * \brief The name of the IPA module
>   */
>  
>  /**
> @@ -212,7 +215,7 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>   * IPAModule instance to verify the validity of the IPAModule.
>   */
>  IPAModule::IPAModule(const std::string &libPath)
> -	: libPath_(libPath), valid_(false)
> +	: libPath_(libPath), valid_(false), acquired_(false)
>  {
>  	if (loadIPAModuleInfo() < 0)
>  		return;
> @@ -289,4 +292,49 @@ const struct IPAModuleInfo &IPAModule::info() const
>  	return info_;
>  }
>  
> +/**
> + * \brief Check if IPA module is in use
> + *
> + * \return true if the IPA module has been claimed for exclusive use, or
> + * false if it is available
> + * \sa acquire(), release()
> + */
> +bool IPAModule::isAcquired() const
> +{
> +	return acquired_;
> +}
> +
> +/**
> + * \brief Claim an IPA module for exclusive use
> + *
> + * Each IPA module is meant to be used by only one pipeline handler.
> + * IPA modules will be acquired through the IPAManager, which will
> + * use this method to claim an IPA module before returning it, and will
> + * skip over already claimed IPA modules.
> + *
> + * When the IPA module is not needed anymore, the release() method should
> + * be called.
> + *
> + * \return true if the IPA module was successfully claimed, or false if
> + * was already claimed
> + * \sa isAcquired(), release()
> + */
> +bool IPAModule::acquire()
> +{
> +	if (acquired_)
> +		return false;
> +
> +	acquired_ = true;
> +	return true;
> +}
> +
> +/**
> + * \brief Release an IPA module previously claimed for exclusive use
> + * \sa acquire(), isAcquired()
> + */
> +void IPAModule::release()
> +{
> +	acquired_ = false;
> +}
> +

I don't think this will work. If we have two instances of a pipeline
handler because the system contains two instances of the same device,
they should both work with the same IPA module. What will then happen is
that the factory method exported by the IPA module will be called twice
to create two instances of a yet-to-be-defined IPA object, one for each
pipeline handler. I believe you can thus drop those three functions.

>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart