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

[Jean-Michel Hautbois]

Hi Laurent,

On 20/08/2021 02:50, Laurent Pinchart wrote:
> 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.
> 

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 ?

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