[libcamera-devel,v3,0/9] IPU3: Introduce modularity for algorithms
mbox series

Message ID 20210818155403.268694-1-jeanmichel.hautbois@ideasonboard.com
Headers show
Series
  • IPU3: Introduce modularity for algorithms
Related show

Message

Jean-Michel Hautbois Aug. 18, 2021, 3:53 p.m. UTC 
This patch series splits the algorithms in IPU3 into a modular form.
When the Metadata class was discussed a proposal from Kieran was to have
a fully-defined structure which would be used to pass the context
between algorithms and the IPAIPU3 class, as well as between the
algorithms themselves.

The algorithms are statically emplaced in a list for the moment, and a
registration system may be used later.
In order to demonstrate how to add a new algorithm, patch 5/9
moves the contrast algorithm from AWB into its own algorithm.

Patch 6/9 and 7/9 change the AWB and AGC to use the new interface.
And the two latest patches are indeed the usage of the new modular
interface applied to AWB and AGC.

Jean-Michel Hautbois (9):
  ipa: move libipa::Algorithm to ipa/ipu3/algorithms
  ipa: ipu3: Introduce a Context structure
  ipa: ipu3: Add the functions to the Algorithm class
  ipa: ipu3: Introduce modular algorithm
  ipa: ipu3: Introduce a modular contrast algorithm
  ipa: ipu3: convert AWB to the new algorithm interface
  ipa: ipu3: convert AGC to the new algorithm interface
  ipa: ipu3: Move IPU3 awb into algorithms
  ipa: ipu3: Move IPU3 agc into algorithms

 .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} |  39 ++--
 src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} |  32 ++--
 src/ipa/ipu3/algorithms/algorithm.cpp         |  76 ++++++++
 src/ipa/ipu3/algorithms/algorithm.h           |  33 ++++
 .../ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} | 168 +++++++-----------
 src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} |  26 +--
 src/ipa/ipu3/algorithms/contrast.cpp          |  53 ++++++
 src/ipa/ipu3/algorithms/contrast.h            |  32 ++++
 src/ipa/ipu3/algorithms/meson.build           |   8 +
 src/ipa/ipu3/ipa_context.h                    |  54 ++++++
 src/ipa/ipu3/ipu3.cpp                         | 124 +++++++++----
 src/ipa/ipu3/meson.build                      |   6 +-
 src/ipa/libipa/algorithm.cpp                  |  39 ----
 src/ipa/libipa/algorithm.h                    |  24 ---
 src/ipa/libipa/libipa.cpp                     |  22 +++
 src/ipa/libipa/meson.build                    |   5 +-
 16 files changed, 484 insertions(+), 257 deletions(-)
 rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (88%)
 rename src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} (51%)
 create mode 100644 src/ipa/ipu3/algorithms/algorithm.cpp
 create mode 100644 src/ipa/ipu3/algorithms/algorithm.h
 rename src/ipa/ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} (71%)
 rename src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} (77%)
 create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
 create mode 100644 src/ipa/ipu3/algorithms/contrast.h
 create mode 100644 src/ipa/ipu3/algorithms/meson.build
 create mode 100644 src/ipa/ipu3/ipa_context.h
 delete mode 100644 src/ipa/libipa/algorithm.cpp
 delete mode 100644 src/ipa/libipa/algorithm.h
 create mode 100644 src/ipa/libipa/libipa.cpp

Comments

Kieran Bingham Aug. 18, 2021, 9:19 p.m. UTC | #1 
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@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 */
>
Laurent Pinchart Aug. 18, 2021, 10 p.m. UTC | #2 
Hi Jean-Michel,

Thank you for the patch.

On Wed, Aug 18, 2021 at 05:54:01PM +0200, Jean-Michel Hautbois wrote:
> In preparation for using the AGC through the new algorithm interfaces,
> convert the existing code to use the new function types.
> 
> Now that the process call is rewritten, re-enable the compiler flag to
> warn when a function declaration hides virtual functions from a base class
> (-Woverloaded-virtual).
> 
> We never use converged_ so remove its declaration.
> The controls may not need to be updated at each call, but it should be
> decided on the context side and not by a specific call by using a lock
> status in the Agc structure for instance.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  meson.build               |  4 ----
>  src/ipa/ipu3/ipu3.cpp     |  8 +++-----
>  src/ipa/ipu3/ipu3_agc.cpp | 19 +++++++------------
>  src/ipa/ipu3/ipu3_agc.h   |  9 ++-------
>  4 files changed, 12 insertions(+), 28 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 4a10e2b6..a49c484f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -108,10 +108,6 @@ 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/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2b4a4c8c..423cc957 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -341,7 +341,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	awbAlgo_ = std::make_unique<IPU3Awb>();
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
> -	agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
> +	agcAlgo_->configure(context_, configInfo);
>  
>  	return 0;
>  }
> @@ -418,8 +418,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  	for (auto const &algo : algorithms_)
>  		algo->prepare(context_, params_);
>  
> -	if (agcAlgo_->updateControls())
> -		awbAlgo_->prepare(context_, params_);
> +	awbAlgo_->prepare(context_, params_);
>  
>  	*params = params_;
>  
> @@ -447,8 +446,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  
>  	awbAlgo_->process(context_, stats);
>  
> -	if (agcAlgo_->updateControls())
> -		setControls(frame);
> +	setControls(frame);
>  
>  	/* \todo Use VBlank value calculated from each frame exposure. */
>  	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> index c6782ff4..1c1f5fb5 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -51,20 +51,20 @@ static constexpr double kEvGainTarget = 0.5;
>  static constexpr uint8_t kCellSize = 8;
>  
>  IPU3Agc::IPU3Agc()
> -	: frameCount_(0), lastFrame_(0), converged_(false),
> -	  updateControls_(false), iqMean_(0.0),
> -	  lineDuration_(0s), maxExposureTime_(0s),
> -	  prevExposure_(0s), prevExposureNoDg_(0s),
> +	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> +	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
>  	  currentExposure_(0s), currentExposureNoDg_(0s)
>  {
>  }
>  
> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)
> +int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>  {
> -	aeGrid_ = bdsGrid;
> +	aeGrid_ = context.configuration.grid.bdsGrid;

Do we need to keep a copy of the grid, can't we git it from the context
when needed ? It doesn't have to be addressed in this patch, could be
done on top if easier.

>  
> -	lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
> +	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s / configInfo.sensorInfo.pixelRate;

Line wrap again :-)

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

>  	maxExposureTime_ = kMaxExposure * lineDuration_;
> +
> +	return 0;
>  }
>  
>  void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> @@ -144,8 +144,6 @@ void IPU3Agc::filterExposure()
>  
>  void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  {
> -	updateControls_ = false;
> -
>  	/* Algorithm initialization should wait for first valid frames */
>  	/* \todo - have a number of frames given by DelayedControls ?
>  	 * - implement a function for IIR */
> @@ -155,7 +153,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  	/* Are we correctly exposed ? */
>  	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
>  		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> -		converged_ = true;
>  	} else {
>  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
> @@ -178,12 +175,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
>  			newExposure = currentExposure_ / exposure;
>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> -			updateControls_ = true;
>  		} else if (currentShutter >= maxExposureTime_) {
>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
>  			newExposure = currentExposure_ / gain;
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
> -			updateControls_ = true;
>  		}
>  		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
>  	}
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index f8290abd..0e922664 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -29,10 +29,8 @@ public:
>  	IPU3Agc();
>  	~IPU3Agc() = default;
>  
> -	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
> -	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats);
> -	bool converged() { return converged_; }
> -	bool updateControls() { return updateControls_; }
> +	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>  
>  private:
>  	void processBrightness(const ipu3_uapi_stats_3a *stats);
> @@ -44,9 +42,6 @@ private:
>  	uint64_t frameCount_;
>  	uint64_t lastFrame_;
>  
> -	bool converged_;
> -	bool updateControls_;
> -
>  	double iqMean_;
>  
>  	Duration lineDuration_;