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

[Kieran Bingham]

On 18/08/2021 16:53, 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
... at each frame when the request is queued

> - process called on EventStatReady event at each frame

at each frame completion when the statistics have been generated.


> As AGC already has a process function with different arguments, let's
> temporarily disable the warnings from the compiler in this same commit.
> It will be removed when AGC will be fully modular.

I'd write that paragraph as:

"""
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 | 46 +++++++++++++++++++++++++++
>  src/ipa/ipu3/algorithms/algorithm.h   |  9 ++++++
>  3 files changed, 59 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 325d34f0..d43a0e90 100644
> --- a/src/ipa/ipu3/algorithms/algorithm.cpp
> +++ b/src/ipa/ipu3/algorithms/algorithm.cpp
> @@ -25,6 +25,52 @@ namespace ipa::ipu3 {
>   * to manage algorithms regardless of their specific type.
>   */
>  
> +/**
> + * \param[in] context The IPAContext structure reference

Should be updated the same as prepare below.

> + * \param[in] configInfo The IPAConfigInfo structure reference
> + * \return 0 if sucess, an error code otherwise

s/sucess/successful/

> + *
> + * \brief Sets the IPAconfiguration to the proper values based on the

/IPAconfiguration/IPAConfiguration provided by the IPAContext/

But this isn't just about 'setting values in the IPAConfiguration'...

"""
\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 IPAConfiguration structure of
the IPAContext.
"""

> + * IPAConfigInfo values.
> + *
> + * It is called one time when IPA is configured. It is meant to prepare everything
> + * needed by the algorithms for their calculations.
> + */
> +int Algorithm::configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)
> +{
> +	return 0;
> +}
> +
> +/**
> + * \param[in] context The IPAContext structure reference

Should this be inout? I'm not entirely certain.

I'd probably just list it as "The shared IPA context" or "The global IPA
context" ?



> + * \param[in] params The parameters structure reference to fill

\param[out] params The IPU3 specific parameters.

> + *
> + * \brief Fill the parameter buffer with the updated context values
> + *
> + * While streaming, an EventFillParams event is received from the pipeline handler.
> + * The algorithms should then fill the parameter structure with any updated value
> + * they have calculated before the IPA passes it back to the ISP.

"""
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)
> +{
> +}
> +
> +/**
> + * \param[in] context The IPAContext structure reference

Same as the usage in prepare, however you chose to update that one.

> + * \param[in] stats The statistics structure to use

\param[in] stats The IPU3 statistics and ISP results

> + *
> + * \brief Fill the context buffer using the received statistics buffer

\brief Process ISP statistics, and run algorithm operations.

> + *
> + * While streaming, an EventStatReady event is received from the pipeline handler.
> + * The algorithms can then use the statistics buffer received to update their
> + * internal state. This state can be shared with other algorithms through the
> + * context->frameContext structure.

"""
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)
> +{
> +}
> +
>  } /* namespace ipa::ipu3 */
>  
>  } /* namespace libcamera */
> +
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> index 072f01c4..bd1f923d 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);

How is the params able to be passed as a reference, while the stats is a
pointer ...

Can/should they both be the same?

(Can stats be passed by reference? or is it already a pointer, and if so
- shouldn't params be the same?, or vice-versa)


>  };
>  
>  } /* namespace ipa::ipu3 */
>