[libcamera-devel] [PATCH v4 13/37] libcamera: IPAModule: Replace ipa_context with IPAInterface

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Nov 06, 2020 at 07:36:43PM +0900, Paul Elder wrote:
> With the new IPC infrastructure, we no longer need the C interface as
> provided by struct ipa_context. Make ipaCreate_() and createInterface()
> return IPAInterface.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> No change in v4
> 
> No change in v3
> 
> Changes in v2:
> - add documentation for IPAModule::createInterface()
> ---
>  include/libcamera/internal/ipa_module.h |  4 ++--
>  src/libcamera/ipa_module.cpp            | 10 ++++------
>  src/libcamera/meson.build               |  1 -
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h
> index c2df2476..19fc5827 100644
> --- a/include/libcamera/internal/ipa_module.h
> +++ b/include/libcamera/internal/ipa_module.h
> @@ -33,7 +33,7 @@ public:
>  
>  	bool load();
>  
> -	struct ipa_context *createContext();
> +	IPAInterface *createInterface();
>  
>  	bool match(PipelineHandler *pipe,
>  		   uint32_t minVersion, uint32_t maxVersion) const;
> @@ -52,7 +52,7 @@ private:
>  	bool loaded_;
>  
>  	void *dlHandle_;
> -	typedef struct ipa_context *(*IPAIntfFactory)();
> +	typedef IPAInterface *(*IPAIntfFactory)(void);
>  	IPAIntfFactory ipaCreate_;
>  };
>  
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index de512a7f..d5ac9820 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -439,20 +439,18 @@ bool IPAModule::load()
>  }
>  
>  /**
> - * \brief Instantiate an IPA context
> + * \brief Instantiate an IPA interface
>   *
>   * After loading the IPA module with load(), this method creates an instance of
> - * the IPA module context. Ownership of the context is passed to the caller, and
> - * the context shall be destroyed by calling the \ref ipa_context_ops::destroy
> - * "ipa_context::ops::destroy()" function.
> + * the IPA module interface.
>   *
>   * Calling this function on a module that has not yet been loaded, or an
>   * invalid module (as returned by load() and isValid(), respectively) is
>   * an error.
>   *
> - * \return The IPA context on success, or nullptr on error
> + * \return The IPA interface on success, or nullptr on error
>   */
> -struct ipa_context *IPAModule::createContext()
> +IPAInterface *IPAModule::createInterface()

There are also two occurrences of createContext() in the documentation
of IPAModule::load(), as well as occurrences of ipa_context. Could you
address that in this patch ?

>  {
>  	if (!valid_ || !loaded_)
>  		return nullptr;
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 9c889a4f..51925bd6 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -22,7 +22,6 @@ libcamera_sources = files([
>      'formats.cpp',
>      'framebuffer_allocator.cpp',
>      'geometry.cpp',
> -    'ipa_context_wrapper.cpp',

Doesn't this belong to patch 14/37 ? Alternatively you can squash 13/37
and 14/37 together.

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

>      'ipa_controls.cpp',
>      'ipa_data_serializer.cpp',
>      'ipa_ipc.cpp',

-- 
Regards,

Laurent Pinchart