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

Laurent Pinchart laurent.pinchart at ideasonboard.com
Fri Aug 20 13:15:10 CEST 2021 

Hi Jean-Michel,

On Fri, Aug 20, 2021 at 08:13:04AM +0200, Jean-Michel Hautbois wrote:
> On 20/08/2021 02:50, Laurent Pinchart wrote:
> > 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.
> > 
> 
> OK, should I change it maybe then :-). The camera is, when this is
> called, in running state, so I can still mention that with:
> 
>  * This function is called for every frame when the camera is running
>  * before it is processed by the ImgU to prepare the ImgU processing
>  * parameters for that frame."
> 
> What do you think ?

Sounds good.

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

More information about the libcamera-devel mailing list