[libcamera-devel] [PATCH v4 3/9] ipa: ipu3: Add the functions to the Algorithm class

[Kieran Bingham]

On 19/08/2021 15:57, Jean-Michel Hautbois wrote:
> Introduce three functions in the Algorithm class to manage algorithms:
> - configure which is called when IPA is configured only
> - prepare called on EventFillParams event at each frame when the request
> is queued
> - process called on EventStatReady event at each frame completion when
> the statistics have been generated.
> 
> The existing AGC implementation already has a function named process(),
> though it has different arguments. Adding the new virtual process()
> interface causes a compiler warning due to the AGC implementation
> overloading a virtual function, even though the overload can be resolved
> correctly.
> 
> Temporarily disable the warning in this commit to maintain bisection
> until the AGC is converted to the new interface.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  meson.build                           |  4 ++
>  src/ipa/ipu3/algorithms/algorithm.cpp | 72 +++++++++++++++++++++++++++
>  src/ipa/ipu3/algorithms/algorithm.h   |  9 ++++
>  3 files changed, 85 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index a49c484f..4a10e2b6 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -108,6 +108,10 @@ if cc.has_argument('-Wno-c99-designator')
>      ]
>  endif
>  
> +# Do not warn when a function declaration hides virtual functions from
> +# a base class
> +cpp_arguments += '-Wno-overloaded-virtual'
> +
>  c_arguments += common_arguments
>  cpp_arguments += common_arguments
>  
> diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/ipu3/algorithms/algorithm.cpp
> index dd46846a..ec3c152e 100644
> --- a/src/ipa/ipu3/algorithms/algorithm.cpp
> +++ b/src/ipa/ipu3/algorithms/algorithm.cpp
> @@ -25,6 +25,78 @@ namespace ipa::ipu3 {
>   * to manage algorithms regardless of their specific type.
>   */
>  
> +/**
> + * \param[in] context The shared IPA context
> + * \param[in] configInfo The IPAConfigInfo structure reference
> + * \return 0 if successful, an error code otherwise
> + *
> + * \brief Configure the Algorithm given an IPAConfigInfo
> + * Algorithms may implement a configure operation to pre-calculate
> + * parameters prior to commencing streaming.
> + *
> + * Configuration state may be stored in the IPASessionConfiguration structure of
> + * the IPAContext.
> + */
> +int Algorithm::configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)
> +{
> +	return 0;
> +}
> +
> +/**
> + * \param[in] context The shared IPA context
> + * \param[in] params The IPU3 specific parameters.

params is an [out] not an in.


> + *
> + * \brief Fill the parameter buffer with the updated context values
> + *
> + * While streaming, the IPA will configure the IPU3 IMGU through it's
> + * parameter structure.
> + *
> + * Algorithms shall fill in the parameter structure fields appropriately to
> + * configure algorithm blocks that they are responsible for.
> + * The structure should be updated to set the enable fields and use flags
> + * of any algorithms they are responsible for.
> + */
> +void Algorithm::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params)

I think params should be a pointer here, not a reference.


> +{
> +}
> +
> +/**
> + * \param[in] context The shared IPA context
> + * \param[in] stats The IPU3 statistics and ISP results
> + *
> + * \brief Process ISP statistics, and run algorithm operations.
> + *
> + * While streaming, the IPA will receive the generated statistics from the
> + * IPU3 IMGU of processed frames.
> + *
> + * Algorithms shall use this data to run calculations and update their
> + * state accordingly.
> + *
> + * Processing shall not take an undue amount of time, and any extended or
> + * computationally expensive calculations or operations must be handled
> + * asynchronously in a separate thread.
> + *
> + * Algorithms can store state in their respective IPAFrameContext
> + * structures, and reference state from the IPAFrameContext of other
> + * algorithms.
> + *
> + * \todo Historical data may be required as part of the processing.
> + *
> + * Either the previous frame, or the IPAFrameContext state of the frame
> + * that generated the statistics for this operation may be required for
> + * some advanced algorithms to prevent oscillations or support control
> + * loops correctly. Only a single IPAFrameContext is available currently,
> + * and so any data stored may represent the results of the previously
> + * completed operations.
> + *
> + * Care shall be taken to ensure the ordering of access to the information
> + * such that the algorithms use up to date state as required.
> + */
> +void Algorithm::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a &stats)

And stats should be a pointer here, not a reference too.

With those,

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +{
> +}
> +
>  } /* namespace ipa::ipu3 */
>  
>  } /* namespace libcamera */
> +
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> index 072f01c4..89431005 100644
> --- a/src/ipa/ipu3/algorithms/algorithm.h
> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> @@ -7,6 +7,9 @@
>  #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
>  #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
>  
> +#include <libcamera/ipa/ipu3_ipa_interface.h>
> +
> +#include "ipa_context.h"
>  namespace libcamera {
>  
>  namespace ipa::ipu3 {
> @@ -15,6 +18,12 @@ class Algorithm
>  {
>  public:
>  	virtual ~Algorithm() {}
> +
> +	virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo);
> +
> +	virtual void prepare(IPAContext &context, ipu3_uapi_params &params);
> +
> +	virtual void process(IPAContext &context, const ipu3_uapi_stats_3a &stats);
>  };
>  
>  } /* namespace ipa::ipu3 */
>