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

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Thu, Aug 19, 2021 at 04:22:49PM +0100, Kieran Bingham wrote:
> 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

Instead of describing the variable type, which is already conveyed by
the type, I think it would be better to describe its purpose:

 * \param[in] configInfo The IPA configuration data, received from the pipeline
 * handler

> > + * \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.

We normally format doxygen blocks as

/**
 * \brief ...
 * \param ...
 *
 * Documentation body
 *
 * \return ...
 */

Same for the functions below.

> > + */
> > +int Algorithm::configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)

You really don't like line wraps, do you ? :-)

> > +{
> > +	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

Let's be more precise.

 * \brief Fill the \a params buffer with ISP processing parameters for a frame

> > + *
> > + * While streaming, the IPA will configure the IPU3 IMGU through it's

It's usually spelled "ImgU", not "IMGU".

s/it's/its/

> > + * parameter structure.

How about making this describe when this function is called ?

 * This function is called while streaming for every frame before it is
 * processed by the ImgU, to prepare the ImgU processing parameters for that
 * frame.

By the way, libcamera doesn't define a "streaming" concept, so at some
point we'll either have to define "streaming" and apply it consistently
through the documentation, or replace it with something else.

> > + *
> > + * Algorithms shall fill in the parameter structure fields appropriately to
> > + * configure algorithm blocks that they are responsible for.

Maybe "the ImgU processing blocks" instead of "algorithms block" ? I was
about to write "hardware blocks", but it's sometimes firmware too.

> > + * The structure should be updated to set the enable fields and use flags
> > + * of any algorithms they are responsible for.

Same comment about paragraph reflow as earlier in the series.

To avoid repeating "they are responsible for", how about the following ?

 * Algorithms shall fill in the parameter structure fields appropriately to
 * configure the ImgU processing blocks that they are responsible for. This
 * includes setting fields and flags that enable those processing blocks.

> > + */
> > +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.

s/.$//

> > + *
> > + * While streaming, the IPA will receive the generated statistics from the
> > + * IPU3 IMGU of processed frames.

Similarly as above,

 * This function is called while streaming for every frame processed by the
 * ImgU, to process statistics generated from that frame by the ImgU.

> > + *
> > + * Algorithms shall use this data to run calculations and update their
> > + * state accordingly.

You could merge the above two paragraphs.

> > + *
> > + * 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.

You can reflow to 80 columns.

> > + *
> > + * \todo Historical data may be required as part of the processing.

\todo covers a single paragraph only (that's how Doxygen processes them,
it's not a coding rule from libcamera), so the blank line below needs to
be removed otherwise the \todo item will only be the above line, and the
next paragraph will be part of the documentation of the function.

> > + *
> > + * 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 */
> > +

Extra blank line.

I've seen at least another instance of this when applying the previous
version of the series with 'git am'. Could you please check all patches
?

> > 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"

Missing blank line.

> >  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);

You could drop the blank lines between those three functions, up to you.

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

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

-- 
Regards,

Laurent Pinchart