[libcamera-devel,RFC,v6] ipa: ipu3: af: Auto focus for dw9719 Surface Go2 VCM

15333 diff mbox series

Since VCM for surface Go 2 (dw9719) had been successfully
driven, this Af module can be used to control the VCM and
determine the focus value based on the IPU3 AF state.

Based on the values from the IPU3 AF buffer, the variance
of each focus step is determined and a greedy approach is
used to find the maximum variance of the AF state and an
appropriate focus value.

The grid configuration is implemented as a context. Also,
the grid parameter- AF_MIN_BLOCK_WIDTH is set to 4 (default
is 3) since if the default value is used, x_start
(x_start > 640) will be at an incorrect location of the
image (rightmost of the sensor).

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 src/ipa/ipu3/algorithms/af.cpp      | 375 ++++++++++++++++++++++++++++
 src/ipa/ipu3/algorithms/af.h        |  78 ++++++
 src/ipa/ipu3/algorithms/meson.build |   3 +-
 src/ipa/ipu3/ipa_context.cpp        |  24 ++
 src/ipa/ipu3/ipa_context.h          |  10 +
 src/ipa/ipu3/ipu3.cpp               |  36 +++
 6 files changed, 525 insertions(+), 1 deletion(-)
 create mode 100644 src/ipa/ipu3/algorithms/af.cpp
 create mode 100644 src/ipa/ipu3/algorithms/af.h

Hi Kate,

Thanks for the patch !

On 07/02/2022 08:16, Kate Hsuan wrote:
> Since VCM for surface Go 2 (dw9719) had been successfully
> driven, this Af module can be used to control the VCM and
> determine the focus value based on the IPU3 AF state.
> 
> Based on the values from the IPU3 AF buffer, the variance
> of each focus step is determined and a greedy approach is
> used to find the maximum variance of the AF state and an
> appropriate focus value.
> 
> The grid configuration is implemented as a context. Also,
> the grid parameter- AF_MIN_BLOCK_WIDTH is set to 4 (default
> is 3) since if the default value is used, x_start
> (x_start > 640) will be at an incorrect location of the
> image (rightmost of the sensor).
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

It works quite well, on top of master and latest's series from Daniel [1].

I am having an issue, I suppose you have the same, with AGC. It is 
blinking on my SGo2 ov8865 device... ?
I took two captures, with the same conditions, on master [2] and with 
your patch (with a local patch to apply the TCC configuration, which 
explains the really better colors) [3].

The feeling I have is it is quite fast to focus, which is good !

[1]: https://patchwork.libcamera.org/project/libcamera/list/?series=2906
[2]: https://snipboard.io/oOexGl.jpg
[3]: https://snipboard.io/kazvGC.jpg

> ---
>   src/ipa/ipu3/algorithms/af.cpp      | 375 ++++++++++++++++++++++++++++
>   src/ipa/ipu3/algorithms/af.h        |  78 ++++++
>   src/ipa/ipu3/algorithms/meson.build |   3 +-
>   src/ipa/ipu3/ipa_context.cpp        |  24 ++
>   src/ipa/ipu3/ipa_context.h          |  10 +
>   src/ipa/ipu3/ipu3.cpp               |  36 +++
>   6 files changed, 525 insertions(+), 1 deletion(-)
>   create mode 100644 src/ipa/ipu3/algorithms/af.cpp
>   create mode 100644 src/ipa/ipu3/algorithms/af.h
> 
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> new file mode 100644
> index 00000000..f67ab8a9
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -0,0 +1,375 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Red Hat
> + *
> + * af.cpp - IPU3 auto focus algorithm
> + */
> +
> +#include "af.h"
> +
> +#include <algorithm>
> +#include <chrono>
> +#include <cmath>
> +#include <fcntl.h>
> +#include <numeric>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <linux/videodev2.h>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/ipa/core_ipa_interface.h>
> +
> +#include "libipa/histogram.h"
> +
> +/**
> + * \file af.h
> + */
> +
> +/**
> + * \var AF_MIN_GRID_WIDTH
> + * \brief the minimum width of AF grid.
> + * The minimum grid horizontal dimensions, in number of grid blocks(cells).
> +*/

My bad, I said to make it constexpr, but did not ask for naming changes...
s/AF_MIN_GRID_WIDTH/kAfMinGridWidth/

Same for all those defines ;-).

> +
> +/**
> + * \var AF_MIN_GRID_HEIGHT
> + * \brief the minimum height of AF grid.
> + * The minimum grid vertical dimensions, in number of grid blocks(cells).
> +*/
> +
> +/**
> + * \var AF_MAX_GRID_WIDTH
> + * \brief the maximum width of AF grid.
> + * The maximum grid horizontal dimensions, in number of grid blocks(cells).
> +*/
> +
> +/**
> + * \var AF_MAX_GRID_HEIGHT
> + * \brief the maximum height of AF grid.
> + * The maximum grid vertical dimensions, in number of grid blocks(cells).
> +*/
> +
> +/**
> + * \var AF_MIN_BLOCK_WIDTH
> + * \brief the minimum block size of the width.
> + */
> +
> +/**
> + * \var AF_MIN_BLOCK_HEIGHT
> + * \brief the minimum block size of the height.
> + */
> +
> +/**
> + * \def AF_MAX_BLOCK_WIDTH
> + * \brief the maximum block size of the width.
> + */
> +
> +/**
> + * \var AF_MAX_BLOCK_HEIGHT
> + * \brief the maximum block size of the height.
> + */
> +
> +/**
> + * \var AF_DEFAULT_HEIGHT_PER_SLICE
> + * \brief The default number of blocks in vertical axis per slice.
> + */
> +
> +namespace libcamera {
> +
> +using namespace std::literals::chrono_literals;
> +
> +namespace ipa::ipu3::algorithms {
> +
> +/**
> + * \class Af
> + * \brief An auto-focus algorithm based on IPU3 statistics
> + *
> + * This algorithm is used to determine the position of the lens and get a
> + * focused image. The IPU3 AF processing block computes the statistics,
> + * composed by high pass and low pass filtered value and stores in a AF buffer.
> + * Typically, for a focused image, it has relative high contrast than a
s/has relative high contrast than/has a relatively higher contrast than/
> + * blurred image, i.e. an out of focus image. Therefore, if an image with the
> + * highest contrast can be found from the AF scan, the lens' position is the
> + * best step of the focus.
> + *
> + */
> +
> +LOG_DEFINE_CATEGORY(IPU3Af)
> +
> +/**
> + * Maximum focus steps of the VCM control
> + * \todo should be obtained from the VCM driver
> + */

Is something missing to implement it and grab it from context ?

> +static constexpr uint32_t maxFocusSteps = 1023;
> +
> +/* minimum focus step for searching appropriate focus */
> +static constexpr uint32_t coarseSearchStep = 30;
> +static constexpr uint32_t fineSearchStep = 1;

Please document each variable ;-).
And fix the namings:
s/coarseSearchStep/kCoarseSearchStep/
s/fineSearchStep/kFineSearchStep/

Same below.

> +
> +/* max ratio of variance change, 0.0 < maxChange < 1.0 */
> +static constexpr double maxChange = 0.5;
> +
> +/* the numbers of frame to be ignored, before performing focus scan. */
s/the numbers of frame/the number of frames/
> +static constexpr uint32_t ignoreFrame = 10;
> +
> +/* fine scan range 0 < findRange < 1 */
> +static constexpr double findRange = 0.05;
> +
> +/* settings for IPU3 AF filter */
> +static struct ipu3_uapi_af_filter_config afFilterConfigDefault = {
> +	.y1_coeff_0 = { 0, 1, 3, 7 },
> +	.y1_coeff_1 = { 11, 13, 1, 2 },
> +	.y1_coeff_2 = { 8, 19, 34, 242 },
> +	.y1_sign_vec = 0x7fdffbfe,
> +	.y2_coeff_0 = { 0, 1, 6, 6 },
> +	.y2_coeff_1 = { 13, 25, 3, 0 },
> +	.y2_coeff_2 = { 25, 3, 177, 254 },
> +	.y2_sign_vec = 0x4e53ca72,
> +	.y_calc = { 8, 8, 8, 8 },
> +	.nf = { 0, 9, 0, 9, 0 },
> +};
> +
> +Af::Af()
> +	: focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),
> +	  coarseComplete_(false), fineComplete_(false)

What would you think of:
s/coarseComplete_/coarseCompleted_/
s/fineComplete_/fineCompleted_/

> +{
> +	maxStep_ = maxFocusSteps;

Once the maximum will be grabbed from the configuration context, this 
variable will not be needed anymore.

> +}
> +
> +Af::~Af()
> +{
> +}

It could be a ~Af() = default; in af.h ?

> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
> +void Af::prepare(IPAContext &context, ipu3_uapi_params *params)
> +{
> +	const struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid;
> +	params->acc_param.af.grid_cfg = grid;
> +	params->acc_param.af.filter_config = afFilterConfigDefault;
> +
> +	/* enable AF processing block */
> +	params->use.acc_af = 1;
> +}
> +
> +/**
> + * \brief Configure the Af given a configInfo
> + * \param[in] context The shared IPA context
> + * \param[in] configInfo The IPA configuration data
> + *
> + * \return 0
> + */
> +int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)
> +{
> +	/* determined focus value i.e. current focus value */
> +	context.frameContext.af.focus = 0;
> +	/* maximum variance of the AF statistics */
> +	context.frameContext.af.maxVariance = 0;
> +	/* the stable AF value flag. if it is true, the AF should be in a stable state. */
> +	context.frameContext.af.stable = false;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief AF coarse scan
> + *
> + * Find a near focused image using a coarse step. The step is determined by coarseSearchStep.
> + *
> + * \param[in] context The shared IPA context
> + *
> + */
> +void Af::afCoarseScan(IPAContext &context)
> +{
> +	if (coarseComplete_ == true)
> +		return;
> +
> +	if (afScan(context, coarseSearchStep)) {
> +		coarseComplete_ = true;
> +		context.frameContext.af.maxVariance = 0;
> +		focus_ = context.frameContext.af.focus - (context.frameContext.af.focus * findRange);
> +		context.frameContext.af.focus = focus_;
> +		previousVariance_ = 0;
> +		maxStep_ = std::clamp(static_cast<uint32_t>(focus_ + (focus_ * findRange)), 0U, maxFocusSteps);
> +	}
> +}
> +
> +/**
> + * \brief AF fine scan
> + *
> + * Find an optimum lens position with moving 1 step for each search.
> + *
> + * \param[in] context The shared IPA context
> + *
> + */
> +void Af::afFineScan(IPAContext &context)
> +{
> +	if (coarseComplete_ != true)
> +		return;
> +
> +	if (afScan(context, fineSearchStep)) {
> +		context.frameContext.af.stable = true;
> +		fineComplete_ = true;
> +	}
> +}
> +
> +/**
> + * \brief AF reset
> + *
> + * Reset all the parameter to start over the AF process.
> + *
> + * \param[in] context The shared IPA context
> + *
> + */
> +void Af::afReset(IPAContext &context)
> +{
> +	context.frameContext.af.maxVariance = 0;
> +	context.frameContext.af.focus = 0;
> +	focus_ = 0;
> +	context.frameContext.af.stable = false;
> +	ignoreCounter_ = ignoreFrame;
> +	previousVariance_ = 0.0;
> +	coarseComplete_ = false;
> +	fineComplete_ = false;
> +	maxStep_ = maxFocusSteps;
> +}
> +
> +/**
> + * \brief AF scan
> + * \param[in] context The shared IPA context
> + *
> + * This fuction compares the previous and current variance. It always pick the largest variance to
> + * replace the previous one. If it finds the decending variance values, it returns immediately.
> + *
> + * \return True, if it finds a AF value.
> + */
> +bool Af::afScan(IPAContext &context, int min_step)
> +{
> +	/* find the maximum variance during the AF scan through
> +	   always picking the maximum variance */

Multiple lines comments need to be in the form:
/*
  * Find the maximum variance during the AF scan through always picking
  * the maximum variance.
  */

> +	if (currentVariance_ > context.frameContext.af.maxVariance) {
> +		context.frameContext.af.maxVariance = currentVariance_;
> +		goodFocus_ = focus_;
> +	}
> +
> +	if (focus_ > maxStep_) {
> +		/* if reach the max step, move lens to the position and set "focus stable". */
> +		context.frameContext.af.focus = goodFocus_;
> +		return true;
> +	} else {
> +		/* check negative direction of the variance development. */

Could you maybe elaborate a bit ?
Something lik (please reword ;-)):
/*
  * Find the maximum of the variance by estimating its
  * derivative. If the direction changes, it means we have
  * passed a maximum one step before.
  */

> +		if ((currentVariance_ - context.frameContext.af.maxVariance) > -(context.frameContext.af.maxVariance * 0.15)) {
> +			focus_ += min_step;
> +			context.frameContext.af.focus = focus_;
> +		} else {
> +			context.frameContext.af.focus = goodFocus_;
> +			previousVariance_ = currentVariance_;
> +			return true;
> +		}
> +	}
> +	LOG(IPU3Af, Debug) << "Variance previous: "
> +			   << previousVariance_
> +			   << " current: "
> +			   << currentVariance_
> +			   << " Diff: "
> +			   << (currentVariance_ - context.frameContext.af.maxVariance);
> +	previousVariance_ = currentVariance_;
> +	LOG(IPU3Af, Debug) << "Focus searching max variance is: "
> +			   << context.frameContext.af.maxVariance
> +			   << " Focus step is "
> +			   << goodFocus_
> +			   << " Current scan is "
> +			   << focus_;

Do we need those logs at each step ?

> +	return false;
> +}
> +
> +/**
> + * \brief Determine the max contrast image and lens position. y_table is the
> + * statistic data from IPU3 and is composed of low pass and high pass filtered
> + * value. High pass filtered value also represents the sharpness of the image.
> + * Based on this, if the image with highest variance of the high pass filtered
> + * value (contrast) during the AF scan, the position of the lens should be the
> + * best focus.
> + * \param[in] context The shared IPA context.
> + * \param[in] stats The statistic buffer of 3A from the IPU3.
> + */
> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> +{
> +	uint32_t total = 0;
> +	double mean;
> +	uint64_t var_sum = 0;
> +	y_table_item_t y_item[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE / sizeof(y_table_item_t)];
> +	uint32_t z = 0;
> +
> +	memcpy(y_item, stats->af_raw_buffer.y_table, IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);
> +
> +	/* Evaluate the AF buffer length */
> +	afRawBufferLen_ = context.configuration.af.afGrid.width *
> +			  context.configuration.af.afGrid.height;

You could memcpy only afRawBufferLen_ elements ?
BTW, this is a local used variable... I would expect it to be set in 
configure() if you want it as a class member ?

Any reason why you copy the elements ?

> +
> +	/**

/* only (not a doxygen comment).

> +	 * Calculate the mean and the variance AF statistics, since IPU3 only determine the AF value
> +	 * for a given grid.

split line.

> +	 * For coarse: y1 are used.
> +	 * For fine: y2 results are used.
> +	 */
> +	if (coarseComplete_) {
> +		for (z = 0; z < afRawBufferLen_; z++) {
> +			total = total + y_item[z].y2_avg;
> +		}
> +		mean = total / afRawBufferLen_;
> +
> +		for (z = 0; z < afRawBufferLen_; z++) {
> +			var_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));

split line.

> +		}
> +	} else {
> +		for (z = 0; z < afRawBufferLen_; z++) {
> +			total = total + y_item[z].y1_avg;
> +		}
> +		mean = total / afRawBufferLen_;
> +
> +		for (z = 0; z < afRawBufferLen_; z++) {
> +			var_sum = var_sum + ((y_item[z].y1_avg - mean) * (y_item[z].y1_avg - mean));

split line.

> +		}
> +	}
> +
> +	/* Determine the average variance of the frame. */
> +	currentVariance_ = static_cast<double>(var_sum) / static_cast<double>(afRawBufferLen_);
> +	LOG(IPU3Af, Debug) << "variance: " << currentVariance_;
> +
> +	if (context.frameContext.af.stable == true) {
> +		const uint32_t diff_var = std::abs(currentVariance_ - context.frameContext.af.maxVariance); > +		const double var_ratio = diff_var / 
context.frameContext.af.maxVariance;

split lines.

> +		LOG(IPU3Af, Debug) << "Rate of variance change: "
> +				   << var_ratio
> +				   << " current focus step: "
> +				   << context.frameContext.af.focus;
> +		/**
> +		 * If the change ratio of contrast is more than maxChange (out of focus),
> +		 * trigger AF again.

/* only (not a doxygen comment) and the first line is too long (cut at 80).

> +		 */
> +		if (var_ratio > maxChange) {
> +			if (ignoreCounter_ == 0) {
> +				afReset(context);
> +			} else
> +				ignoreCounter_--;
> +		} else
> +			ignoreCounter_ = ignoreFrame;
> +	} else {
> +		if (ignoreCounter_ != 0)
> +			ignoreCounter_--;
> +		else {
> +			afCoarseScan(context);
> +			afFineScan(context);
> +		}
> +	}

I think this could be simplified a bit ?
We always need to know if we need to ignore the frame or not. If so, we 
can return early. If not, we want to know if we are stable or not.

> +}
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> new file mode 100644
> index 00000000..981bf3a2
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/af.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Red Hat
> + *
> + * af.h - IPU3 Af algorithm
> + */
> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
> +#define __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +#include <libcamera/base/utils.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "algorithm.h"
> +
> +/* static variables from repo of chromium */
> +static constexpr uint8_t AF_MIN_GRID_WIDTH = 16;
> +static constexpr uint8_t AF_MIN_GRID_HEIGHT = 16;
> +static constexpr uint8_t AF_MAX_GRID_WIDTH = 32;
> +static constexpr uint8_t AF_MAX_GRID_HEIGHT = 24;
> +static constexpr uint16_t AF_MIN_BLOCK_WIDTH = 4;
> +static constexpr uint16_t AF_MIN_BLOCK_HEIGHT = 3;
> +static constexpr uint16_t AF_MAX_BLOCK_WIDTH = 6;
> +static constexpr uint16_t AF_MAX_BLOCK_HEIGHT = 6;
> +static constexpr uint16_t AF_DEFAULT_HEIGHT_PER_SLICE = 2;
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +class Af : public Algorithm
> +{
> +	/* The format of y_table. From ipu3-ipa repo */
> +	typedef struct __attribute__((packed)) y_table_item {
> +		uint16_t y1_avg;
> +		uint16_t y2_avg;
> +	} y_table_item_t;
> +public:
> +	Af();
> +	~Af();
> +
> +	void prepare(IPAContext &context, ipu3_uapi_params *params) override;
> +	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> +
> +private:
> +	void afCoarseScan(IPAContext &context);
> +	void afFineScan(IPAContext &context);
> +	bool afScan(IPAContext &context, int min_step);
> +	void afReset(IPAContext &context);
> +
> +	/* Used for focus scan. */
> +	uint32_t focus_;
> +	/* Focus good */
> +	uint32_t goodFocus_;
> +	/* Recent AF statistic variance. */
> +	double currentVariance_;
> +	/* The frames to be ignore before starting measuring. */
> +	uint32_t ignoreCounter_;
> +	/* previous variance. it is used to determine the gradient */
> +	double previousVariance_;
> +	/* Max scan steps of each pass of AF scaning */
> +	uint32_t maxStep_;
> +	/* coarse scan stable. Complete low pass search (coarse) scan) */
> +	bool coarseComplete_;
> +	/* fine scan stable. Complete high pass scan (fine scan) */
> +	bool fineComplete_;
> +	/* Raw buffer length */
> +	uint32_t afRawBufferLen_;
> +};
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AF_H__ */
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> index 4db6ae1d..e1099169 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -1,8 +1,9 @@
>   # SPDX-License-Identifier: CC0-1.0
>   
>   ipu3_ipa_algorithms = files([
> +    'af.cpp',
>       'agc.cpp',
>       'awb.cpp',
>       'blc.cpp',
> -    'tone_mapping.cpp',
> +    'tone_mapping.cpp'
>   ])
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 86794ac1..ecb2b90a 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -67,6 +67,30 @@ namespace libcamera::ipa::ipu3 {
>    *
>    * \var IPASessionConfiguration::grid.stride
>    * \brief Number of cells on one line including the ImgU padding
> + *
> + */
> +
> +/**
> + * \var IPASessionConfiguration::af
> + * \brief AF grid configuration of the IPA
> + *
> + * \var IPASessionConfiguration::af.afGrid
> + *
> + */
> +
> +/**
> + * \var IPAFrameContext::af
> + * \brief Context for the Automatic Focus algorithm
> + *
> + * \struct  IPAFrameContext::af
> + * \var IPAFrameContext::af.focus
> + * \brief Current position of the lens
> + *
> + * \var IPAFrameContext::af.maxVariance
> + * \brief The maximum variance of the current image.
> + *
> + * \var IPAFrameContext::af.stable
> + * \brief is the image focused?
>    */
>   
>   /**
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index c6dc0814..60ad3194 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -25,6 +25,10 @@ struct IPASessionConfiguration {
>   		uint32_t stride;
>   	} grid;
>   
> +	struct {
> +		ipu3_uapi_grid_config afGrid;
> +	} af;
> +
>   	struct {
>   		utils::Duration minShutterSpeed;
>   		utils::Duration maxShutterSpeed;
> @@ -34,6 +38,12 @@ struct IPASessionConfiguration {
>   };
>   
>   struct IPAFrameContext {
> +	struct {
> +		uint32_t focus;
> +		double maxVariance;
> +		bool stable;
> +	} af;
> +
>   	struct {
>   		uint32_t exposure;
>   		double gain;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 3d307708..2d23f8f8 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -30,6 +30,7 @@
>   
>   #include "libcamera/internal/mapped_framebuffer.h"
>   
> +#include "algorithms/af.h"
>   #include "algorithms/agc.h"
>   #include "algorithms/algorithm.h"
>   #include "algorithms/awb.h"
> @@ -157,6 +158,7 @@ private:
>   
>   	void setControls(unsigned int frame);
>   	void calculateBdsGrid(const Size &bdsOutputSize);
> +	void initAfGrid(const Size &bdsOutputSize);
>   
>   	std::map<unsigned int, MappedFrameBuffer> buffers_;
>   
> @@ -294,6 +296,7 @@ int IPAIPU3::init(const IPASettings &settings,
>   	}
>   
>   	/* Construct our Algorithms */
> +	algorithms_.push_back(std::make_unique<algorithms::Af>());
>   	algorithms_.push_back(std::make_unique<algorithms::Agc>());
>   	algorithms_.push_back(std::make_unique<algorithms::Awb>());
>   	algorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());
> @@ -395,6 +398,37 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>   			    << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
>   }
>   
> +/**
> + * \brief Configure the IPU3 AF grid
> + *
> + * This function gives the default values for the AF grid configuration.
> + * All the parameters are set to the minimum acceptable values.
> + *
> + * \param bdsOutputSize The BDS output size
> + */
> +void IPAIPU3::initAfGrid(const Size &bdsOutputSize)
> +{
> +	struct ipu3_uapi_grid_config &grid = context_.configuration.af.afGrid;
> +	grid.width = AF_MIN_GRID_WIDTH;
> +	grid.height = AF_MIN_GRID_HEIGHT;
> +	grid.block_width_log2 = AF_MIN_BLOCK_WIDTH;
> +	grid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;
> +	grid.height_per_slice = AF_DEFAULT_HEIGHT_PER_SLICE;
> +	grid.block_width_log2 = AF_MIN_BLOCK_WIDTH;
> +	grid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;
> +
> +	/* x_start and y start are default to BDS center */
> +	grid.x_start = (bdsOutputSize.width / 2) -
> +		       (((grid.width << grid.block_width_log2) / 2));
> +	grid.y_start = (bdsOutputSize.height / 2) -
> +		       (((grid.height << grid.block_height_log2) / 2));
> +
> +	/* x_start and y_start should be even */
> +	grid.x_start = (grid.x_start / 2) * 2;
> +	grid.y_start = (grid.y_start / 2) * 2;
> +	grid.y_start = grid.y_start | IPU3_UAPI_GRID_Y_START_EN;
> +}

Why are you not doing it in the IPU3Af::configure() call ?
It would let you remove the [[maybe_unused]] for the IPAConfigInfo 
variable, as you would use it to get the bdsOutputSize ?

> +
>   /**
>    * \brief Configure the IPU3 IPA
>    * \param[in] configInfo The IPA configuration data, received from the pipeline
> @@ -461,6 +495,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   
>   	lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
>   
> +	initAfGrid(configInfo.bdsOutputSize);
> +

And this would also be removed ?

>   	/* Update the camera controls using the new sensor settings. */
>   	updateControls(sensorInfo_, ctrls_, ipaControls);
>   

I think you could now remove the "RFC" tag.
You can add my
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

coub Hi Jean-Michel,

Thank you for your testing and review.

On Mon, Feb 7, 2022 at 7:35 PM Jean-Michel Hautbois
<jeanmichel.hautbois@ideasonboard.com> wrote:
>
> Hi Kate,
>
> Thanks for the patch !
>
> On 07/02/2022 08:16, Kate Hsuan wrote:
> > Since VCM for surface Go 2 (dw9719) had been successfully
> > driven, this Af module can be used to control the VCM and
> > determine the focus value based on the IPU3 AF state.
> >
> > Based on the values from the IPU3 AF buffer, the variance
> > of each focus step is determined and a greedy approach is
> > used to find the maximum variance of the AF state and an
> > appropriate focus value.
> >
> > The grid configuration is implemented as a context. Also,
> > the grid parameter- AF_MIN_BLOCK_WIDTH is set to 4 (default
> > is 3) since if the default value is used, x_start
> > (x_start > 640) will be at an incorrect location of the
> > image (rightmost of the sensor).
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
>
> It works quite well, on top of master and latest's series from Daniel [1].

I also tried it too, Thanks, Daniel and folks!

>
> I am having an issue, I suppose you have the same, with AGC. It is
> blinking on my SGo2 ov8865 device... ?

Me too, I maintained my environment to not too bright and not too dark
to prevent from blinking.
Does it seem an issue with the sensor's driver or AGC? I'm not very sure.

> I took two captures, with the same conditions, on master [2] and with
> your patch (with a local patch to apply the TCC configuration, which
> explains the really better colors) [3].
>
> The feeling I have is it is quite fast to focus, which is good !
>
> [1]: https://patchwork.libcamera.org/project/libcamera/list/?series=2906
> [2]: https://snipboard.io/oOexGl.jpg
> [3]: https://snipboard.io/kazvGC.jpg
>

Nice piano :). We still could improve it. In some low contrast
backgrounds, it was difficult to determine a focus value.

> > ---
> >   src/ipa/ipu3/algorithms/af.cpp      | 375 ++++++++++++++++++++++++++++
> >   src/ipa/ipu3/algorithms/af.h        |  78 ++++++
> >   src/ipa/ipu3/algorithms/meson.build |   3 +-
> >   src/ipa/ipu3/ipa_context.cpp        |  24 ++
> >   src/ipa/ipu3/ipa_context.h          |  10 +
> >   src/ipa/ipu3/ipu3.cpp               |  36 +++
> >   6 files changed, 525 insertions(+), 1 deletion(-)
> >   create mode 100644 src/ipa/ipu3/algorithms/af.cpp
> >   create mode 100644 src/ipa/ipu3/algorithms/af.h
> >
> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> > new file mode 100644
> > index 00000000..f67ab8a9
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > @@ -0,0 +1,375 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Red Hat
> > + *
> > + * af.cpp - IPU3 auto focus algorithm
> > + */
> > +
> > +#include "af.h"
> > +
> > +#include <algorithm>
> > +#include <chrono>
> > +#include <cmath>
> > +#include <fcntl.h>
> > +#include <numeric>
> > +#include <sys/ioctl.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +
> > +#include <linux/videodev2.h>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/ipa/core_ipa_interface.h>
> > +
> > +#include "libipa/histogram.h"
> > +
> > +/**
> > + * \file af.h
> > + */
> > +
> > +/**
> > + * \var AF_MIN_GRID_WIDTH
> > + * \brief the minimum width of AF grid.
> > + * The minimum grid horizontal dimensions, in number of grid blocks(cells).
> > +*/
>
> My bad, I said to make it constexpr, but did not ask for naming changes...
> s/AF_MIN_GRID_WIDTH/kAfMinGridWidth/
>
> Same for all those defines ;-).
ok! no problem.

>
> > +
> > +/**
> > + * \var AF_MIN_GRID_HEIGHT
> > + * \brief the minimum height of AF grid.
> > + * The minimum grid vertical dimensions, in number of grid blocks(cells).
> > +*/
> > +
> > +/**
> > + * \var AF_MAX_GRID_WIDTH
> > + * \brief the maximum width of AF grid.
> > + * The maximum grid horizontal dimensions, in number of grid blocks(cells).
> > +*/
> > +
> > +/**
> > + * \var AF_MAX_GRID_HEIGHT
> > + * \brief the maximum height of AF grid.
> > + * The maximum grid vertical dimensions, in number of grid blocks(cells).
> > +*/
> > +
> > +/**
> > + * \var AF_MIN_BLOCK_WIDTH
> > + * \brief the minimum block size of the width.
> > + */
> > +
> > +/**
> > + * \var AF_MIN_BLOCK_HEIGHT
> > + * \brief the minimum block size of the height.
> > + */
> > +
> > +/**
> > + * \def AF_MAX_BLOCK_WIDTH
> > + * \brief the maximum block size of the width.
> > + */
> > +
> > +/**
> > + * \var AF_MAX_BLOCK_HEIGHT
> > + * \brief the maximum block size of the height.
> > + */
> > +
> > +/**
> > + * \var AF_DEFAULT_HEIGHT_PER_SLICE
> > + * \brief The default number of blocks in vertical axis per slice.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +using namespace std::literals::chrono_literals;
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +/**
> > + * \class Af
> > + * \brief An auto-focus algorithm based on IPU3 statistics
> > + *
> > + * This algorithm is used to determine the position of the lens and get a
> > + * focused image. The IPU3 AF processing block computes the statistics,
> > + * composed by high pass and low pass filtered value and stores in a AF buffer.
> > + * Typically, for a focused image, it has relative high contrast than a
> s/has relative high contrast than/has a relatively higher contrast than/
> > + * blurred image, i.e. an out of focus image. Therefore, if an image with the
> > + * highest contrast can be found from the AF scan, the lens' position is the
> > + * best step of the focus.
> > + *

ops, I would revise this part. my fault.


> > + */
> > +
> > +LOG_DEFINE_CATEGORY(IPU3Af)
> > +
> > +/**
> > + * Maximum focus steps of the VCM control
> > + * \todo should be obtained from the VCM driver
> > + */
>
> Is something missing to implement it and grab it from context ?

The maximum steps of dw9719 VCM is 1023. The max steps should be
obtained from the VCM driver.
So, a kind of method should be implemented to fetch the VCM capability
from the driver.

>
> > +static constexpr uint32_t maxFocusSteps = 1023;
> > +
> > +/* minimum focus step for searching appropriate focus */
> > +static constexpr uint32_t coarseSearchStep = 30;
> > +static constexpr uint32_t fineSearchStep = 1;
>
> Please document each variable ;-).
> And fix the namings:
> s/coarseSearchStep/kCoarseSearchStep/
> s/fineSearchStep/kFineSearchStep/
>
> Same below.
>
> > +
> > +/* max ratio of variance change, 0.0 < maxChange < 1.0 */
> > +static constexpr double maxChange = 0.5;
> > +
> > +/* the numbers of frame to be ignored, before performing focus scan. */
> s/the numbers of frame/the number of frames/
> > +static constexpr uint32_t ignoreFrame = 10;
> > +
> > +/* fine scan range 0 < findRange < 1 */
> > +static constexpr double findRange = 0.05;
> > +
> > +/* settings for IPU3 AF filter */
> > +static struct ipu3_uapi_af_filter_config afFilterConfigDefault = {
> > +     .y1_coeff_0 = { 0, 1, 3, 7 },
> > +     .y1_coeff_1 = { 11, 13, 1, 2 },
> > +     .y1_coeff_2 = { 8, 19, 34, 242 },
> > +     .y1_sign_vec = 0x7fdffbfe,
> > +     .y2_coeff_0 = { 0, 1, 6, 6 },
> > +     .y2_coeff_1 = { 13, 25, 3, 0 },
> > +     .y2_coeff_2 = { 25, 3, 177, 254 },
> > +     .y2_sign_vec = 0x4e53ca72,
> > +     .y_calc = { 8, 8, 8, 8 },
> > +     .nf = { 0, 9, 0, 9, 0 },
> > +};
> > +
> > +Af::Af()
> > +     : focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),
> > +       coarseComplete_(false), fineComplete_(false)
>
> What would you think of:
> s/coarseComplete_/coarseCompleted_/
> s/fineComplete_/fineCompleted_/
>
> > +{
> > +     maxStep_ = maxFocusSteps;
>
> Once the maximum will be grabbed from the configuration context, this
> variable will not be needed anymore.
>
> > +}
> > +
> > +Af::~Af()
> > +{
> > +}
>
> It could be a ~Af() = default; in af.h ?
>
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::prepare
> > + */
> > +void Af::prepare(IPAContext &context, ipu3_uapi_params *params)
> > +{
> > +     const struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid;
> > +     params->acc_param.af.grid_cfg = grid;
> > +     params->acc_param.af.filter_config = afFilterConfigDefault;
> > +
> > +     /* enable AF processing block */
> > +     params->use.acc_af = 1;
> > +}
> > +
> > +/**
> > + * \brief Configure the Af given a configInfo
> > + * \param[in] context The shared IPA context
> > + * \param[in] configInfo The IPA configuration data
> > + *
> > + * \return 0
> > + */
> > +int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)
> > +{
> > +     /* determined focus value i.e. current focus value */
> > +     context.frameContext.af.focus = 0;
> > +     /* maximum variance of the AF statistics */
> > +     context.frameContext.af.maxVariance = 0;
> > +     /* the stable AF value flag. if it is true, the AF should be in a stable state. */
> > +     context.frameContext.af.stable = false;
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * \brief AF coarse scan
> > + *
> > + * Find a near focused image using a coarse step. The step is determined by coarseSearchStep.
> > + *
> > + * \param[in] context The shared IPA context
> > + *
> > + */
> > +void Af::afCoarseScan(IPAContext &context)
> > +{
> > +     if (coarseComplete_ == true)
> > +             return;
> > +
> > +     if (afScan(context, coarseSearchStep)) {
> > +             coarseComplete_ = true;
> > +             context.frameContext.af.maxVariance = 0;
> > +             focus_ = context.frameContext.af.focus - (context.frameContext.af.focus * findRange);
> > +             context.frameContext.af.focus = focus_;
> > +             previousVariance_ = 0;
> > +             maxStep_ = std::clamp(static_cast<uint32_t>(focus_ + (focus_ * findRange)), 0U, maxFocusSteps);
> > +     }
> > +}
> > +
> > +/**
> > + * \brief AF fine scan
> > + *
> > + * Find an optimum lens position with moving 1 step for each search.
> > + *
> > + * \param[in] context The shared IPA context
> > + *
> > + */
> > +void Af::afFineScan(IPAContext &context)
> > +{
> > +     if (coarseComplete_ != true)
> > +             return;
> > +
> > +     if (afScan(context, fineSearchStep)) {
> > +             context.frameContext.af.stable = true;
> > +             fineComplete_ = true;
> > +     }
> > +}
> > +
> > +/**
> > + * \brief AF reset
> > + *
> > + * Reset all the parameter to start over the AF process.
> > + *
> > + * \param[in] context The shared IPA context
> > + *
> > + */
> > +void Af::afReset(IPAContext &context)
> > +{
> > +     context.frameContext.af.maxVariance = 0;
> > +     context.frameContext.af.focus = 0;
> > +     focus_ = 0;
> > +     context.frameContext.af.stable = false;
> > +     ignoreCounter_ = ignoreFrame;
> > +     previousVariance_ = 0.0;
> > +     coarseComplete_ = false;
> > +     fineComplete_ = false;
> > +     maxStep_ = maxFocusSteps;
> > +}
> > +
> > +/**
> > + * \brief AF scan
> > + * \param[in] context The shared IPA context
> > + *
> > + * This fuction compares the previous and current variance. It always pick the largest variance to
> > + * replace the previous one. If it finds the decending variance values, it returns immediately.
> > + *
> > + * \return True, if it finds a AF value.
> > + */
> > +bool Af::afScan(IPAContext &context, int min_step)
> > +{
> > +     /* find the maximum variance during the AF scan through
> > +        always picking the maximum variance */
>
> Multiple lines comments need to be in the form:
> /*
>   * Find the maximum variance during the AF scan through always picking
>   * the maximum variance.
>   */
>
Ok thanks.

> > +     if (currentVariance_ > context.frameContext.af.maxVariance) {
> > +             context.frameContext.af.maxVariance = currentVariance_;
> > +             goodFocus_ = focus_;
> > +     }
> > +
> > +     if (focus_ > maxStep_) {
> > +             /* if reach the max step, move lens to the position and set "focus stable". */
> > +             context.frameContext.af.focus = goodFocus_;
> > +             return true;
> > +     } else {
> > +             /* check negative direction of the variance development. */
>
> Could you maybe elaborate a bit ?
> Something lik (please reword ;-)):
> /*
>   * Find the maximum of the variance by estimating its
>   * derivative. If the direction changes, it means we have
>   * passed a maximum one step before.
>   */
>

Thank for revising this. I would improve my comments :).

> > +             if ((currentVariance_ - context.frameContext.af.maxVariance) > -(context.frameContext.af.maxVariance * 0.15)) {
> > +                     focus_ += min_step;
> > +                     context.frameContext.af.focus = focus_;
> > +             } else {
> > +                     context.frameContext.af.focus = goodFocus_;
> > +                     previousVariance_ = currentVariance_;
> > +                     return true;
> > +             }
> > +     }
> > +     LOG(IPU3Af, Debug) << "Variance previous: "
> > +                        << previousVariance_
> > +                        << " current: "
> > +                        << currentVariance_
> > +                        << " Diff: "
> > +                        << (currentVariance_ - context.frameContext.af.maxVariance);
> > +     previousVariance_ = currentVariance_;
> > +     LOG(IPU3Af, Debug) << "Focus searching max variance is: "
> > +                        << context.frameContext.af.maxVariance
> > +                        << " Focus step is "
> > +                        << goodFocus_
> > +                        << " Current scan is "
> > +                        << focus_;
>
> Do we need those logs at each step ?

It reveals the variance, current step, and focused step. It could be removed.

>
> > +}
> > +
> > +/**
> > + * \brief Determine the max contrast image and lens position. y_table is the
> > + * statistic data from IPU3 and is composed of low pass and high pass filtered
> > + * value. High pass filtered value also represents the sharpness of the image.
> > + * Based on this, if the image with highest variance of the high pass filtered
> > + * value (contrast) during the AF scan, the position of the lens should be the
> > + * best focus.
> > + * \param[in] context The shared IPA context.
> > + * \param[in] stats The statistic buffer of 3A from the IPU3.
> > + */
> > +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> > +{
> > +     uint32_t total = 0;
> > +     double mean;
> > +     uint64_t var_sum = 0;
> > +     y_table_item_t y_item[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE / sizeof(y_table_item_t)];
> > +     uint32_t z = 0;
> > +
> > +     memcpy(y_item, stats->af_raw_buffer.y_table, IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);
> > +
> > +     /* Evaluate the AF buffer length */
> > +     afRawBufferLen_ = context.configuration.af.afGrid.width *
> > +                       context.configuration.af.afGrid.height;
>
> You could memcpy only afRawBufferLen_ elements ?
> BTW, this is a local used variable... I would expect it to be set in
> configure() if you want it as a class member ?
Sure, I could make afRawBufferLen_ to be a class member and be set in
configure().

>
> Any reason why you copy the elements ?

Datacasting uses static_cast or reinterpret_cast. I could not make
them possible :(

>
> > +
> > +     /**
>
> /* only (not a doxygen comment).
>
> > +      * Calculate the mean and the variance AF statistics, since IPU3 only determine the AF value
> > +      * for a given grid.
>
> split line.
>
> > +      * For coarse: y1 are used.
> > +      * For fine: y2 results are used.
> > +      */
> > +     if (coarseComplete_) {
> > +             for (z = 0; z < afRawBufferLen_; z++) {
> > +                     total = total + y_item[z].y2_avg;
> > +             }
> > +             mean = total / afRawBufferLen_;
> > +
> > +             for (z = 0; z < afRawBufferLen_; z++) {
> > +                     var_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));
>
> split line.
>
> > +             }
> > +     } else {
> > +             for (z = 0; z < afRawBufferLen_; z++) {
> > +                     total = total + y_item[z].y1_avg;
> > +             }
> > +             mean = total / afRawBufferLen_;
> > +
> > +             for (z = 0; z < afRawBufferLen_; z++) {
> > +                     var_sum = var_sum + ((y_item[z].y1_avg - mean) * (y_item[z].y1_avg - mean));
>
> split line.
>
> > +             }
> > +     }
> > +
> > +     /* Determine the average variance of the frame. */
> > +     currentVariance_ = static_cast<double>(var_sum) / static_cast<double>(afRawBufferLen_);
> > +     LOG(IPU3Af, Debug) << "variance: " << currentVariance_;
> > +
> > +     if (context.frameContext.af.stable == true) {
> > +             const uint32_t diff_var = std::abs(currentVariance_ - context.frameContext.af.maxVariance); > +         const double var_ratio = diff_var /
> context.frameContext.af.maxVariance;
>
> split lines.
>
> > +             LOG(IPU3Af, Debug) << "Rate of variance change: "
> > +                                << var_ratio
> > +                                << " current focus step: "
> > +                                << context.frameContext.af.focus;
> > +             /**
> > +              * If the change ratio of contrast is more than maxChange (out of focus),
> > +              * trigger AF again.
>
> /* only (not a doxygen comment) and the first line is too long (cut at 80).

my fault, I'll correct this.

>
> > +              */
> > +             if (var_ratio > maxChange) {
> > +                     if (ignoreCounter_ == 0) {
> > +                             afReset(context);
> > +                     } else
> > +                             ignoreCounter_--;
> > +             } else
> > +                     ignoreCounter_ = ignoreFrame;
> > +     } else {
> > +             if (ignoreCounter_ != 0)
> > +                     ignoreCounter_--;
> > +             else {
> > +                     afCoarseScan(context);
> > +                     afFineScan(context);
> > +             }
> > +     }
>
> I think this could be simplified a bit ?
> We always need to know if we need to ignore the frame or not. If so, we
> can return early. If not, we want to know if we are stable or not.

I found that if the lens is reset suddenly, the AF statistics will
bounce until the AGC become stable.
So we may tweak ignoreCounter_ to find an acceptable range to wait for
AF statistics stable.
I'll perform some experiments about this and try to simplify this.


>
> > +}
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> > new file mode 100644
> > index 00000000..981bf3a2
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/af.h
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Red Hat
> > + *
> > + * af.h - IPU3 Af algorithm
> > + */
> > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
> > +#define __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
> > +
> > +#include <linux/intel-ipu3.h>
> > +
> > +#include <libcamera/base/utils.h>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +#include "algorithm.h"
> > +
> > +/* static variables from repo of chromium */
> > +static constexpr uint8_t AF_MIN_GRID_WIDTH = 16;
> > +static constexpr uint8_t AF_MIN_GRID_HEIGHT = 16;
> > +static constexpr uint8_t AF_MAX_GRID_WIDTH = 32;
> > +static constexpr uint8_t AF_MAX_GRID_HEIGHT = 24;
> > +static constexpr uint16_t AF_MIN_BLOCK_WIDTH = 4;
> > +static constexpr uint16_t AF_MIN_BLOCK_HEIGHT = 3;
> > +static constexpr uint16_t AF_MAX_BLOCK_WIDTH = 6;
> > +static constexpr uint16_t AF_MAX_BLOCK_HEIGHT = 6;
> > +static constexpr uint16_t AF_DEFAULT_HEIGHT_PER_SLICE = 2;
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +class Af : public Algorithm
> > +{
> > +     /* The format of y_table. From ipu3-ipa repo */
> > +     typedef struct __attribute__((packed)) y_table_item {
> > +             uint16_t y1_avg;
> > +             uint16_t y2_avg;
> > +     } y_table_item_t;
> > +public:
> > +     Af();
> > +     ~Af();
> > +
> > +     void prepare(IPAContext &context, ipu3_uapi_params *params) override;
> > +     int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > +     void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> > +
> > +private:
> > +     void afCoarseScan(IPAContext &context);
> > +     void afFineScan(IPAContext &context);
> > +     bool afScan(IPAContext &context, int min_step);
> > +     void afReset(IPAContext &context);
> > +
> > +     /* Used for focus scan. */
> > +     uint32_t focus_;
> > +     /* Focus good */
> > +     uint32_t goodFocus_;
> > +     /* Recent AF statistic variance. */
> > +     double currentVariance_;
> > +     /* The frames to be ignore before starting measuring. */
> > +     uint32_t ignoreCounter_;
> > +     /* previous variance. it is used to determine the gradient */
> > +     double previousVariance_;
> > +     /* Max scan steps of each pass of AF scaning */
> > +     uint32_t maxStep_;
> > +     /* coarse scan stable. Complete low pass search (coarse) scan) */
> > +     bool coarseComplete_;
> > +     /* fine scan stable. Complete high pass scan (fine scan) */
> > +     bool fineComplete_;
> > +     /* Raw buffer length */
> > +     uint32_t afRawBufferLen_;
> > +};
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AF_H__ */
> > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> > index 4db6ae1d..e1099169 100644
> > --- a/src/ipa/ipu3/algorithms/meson.build
> > +++ b/src/ipa/ipu3/algorithms/meson.build
> > @@ -1,8 +1,9 @@
> >   # SPDX-License-Identifier: CC0-1.0
> >
> >   ipu3_ipa_algorithms = files([
> > +    'af.cpp',
> >       'agc.cpp',
> >       'awb.cpp',
> >       'blc.cpp',
> > -    'tone_mapping.cpp',
> > +    'tone_mapping.cpp'
> >   ])
> > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > index 86794ac1..ecb2b90a 100644
> > --- a/src/ipa/ipu3/ipa_context.cpp
> > +++ b/src/ipa/ipu3/ipa_context.cpp
> > @@ -67,6 +67,30 @@ namespace libcamera::ipa::ipu3 {
> >    *
> >    * \var IPASessionConfiguration::grid.stride
> >    * \brief Number of cells on one line including the ImgU padding
> > + *
> > + */
> > +
> > +/**
> > + * \var IPASessionConfiguration::af
> > + * \brief AF grid configuration of the IPA
> > + *
> > + * \var IPASessionConfiguration::af.afGrid
> > + *
> > + */
> > +
> > +/**
> > + * \var IPAFrameContext::af
> > + * \brief Context for the Automatic Focus algorithm
> > + *
> > + * \struct  IPAFrameContext::af
> > + * \var IPAFrameContext::af.focus
> > + * \brief Current position of the lens
> > + *
> > + * \var IPAFrameContext::af.maxVariance
> > + * \brief The maximum variance of the current image.
> > + *
> > + * \var IPAFrameContext::af.stable
> > + * \brief is the image focused?
> >    */
> >
> >   /**
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index c6dc0814..60ad3194 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -25,6 +25,10 @@ struct IPASessionConfiguration {
> >               uint32_t stride;
> >       } grid;
> >
> > +     struct {
> > +             ipu3_uapi_grid_config afGrid;
> > +     } af;
> > +
> >       struct {
> >               utils::Duration minShutterSpeed;
> >               utils::Duration maxShutterSpeed;
> > @@ -34,6 +38,12 @@ struct IPASessionConfiguration {
> >   };
> >
> >   struct IPAFrameContext {
> > +     struct {
> > +             uint32_t focus;
> > +             double maxVariance;
> > +             bool stable;
> > +     } af;
> > +
> >       struct {
> >               uint32_t exposure;
> >               double gain;
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 3d307708..2d23f8f8 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -30,6 +30,7 @@
> >
> >   #include "libcamera/internal/mapped_framebuffer.h"
> >
> > +#include "algorithms/af.h"
> >   #include "algorithms/agc.h"
> >   #include "algorithms/algorithm.h"
> >   #include "algorithms/awb.h"
> > @@ -157,6 +158,7 @@ private:
> >
> >       void setControls(unsigned int frame);
> >       void calculateBdsGrid(const Size &bdsOutputSize);
> > +     void initAfGrid(const Size &bdsOutputSize);
> >
> >       std::map<unsigned int, MappedFrameBuffer> buffers_;
> >
> > @@ -294,6 +296,7 @@ int IPAIPU3::init(const IPASettings &settings,
> >       }
> >
> >       /* Construct our Algorithms */
> > +     algorithms_.push_back(std::make_unique<algorithms::Af>());
> >       algorithms_.push_back(std::make_unique<algorithms::Agc>());
> >       algorithms_.push_back(std::make_unique<algorithms::Awb>());
> >       algorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());
> > @@ -395,6 +398,37 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> >                           << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
> >   }
> >
> > +/**
> > + * \brief Configure the IPU3 AF grid
> > + *
> > + * This function gives the default values for the AF grid configuration.
> > + * All the parameters are set to the minimum acceptable values.
> > + *
> > + * \param bdsOutputSize The BDS output size
> > + */
> > +void IPAIPU3::initAfGrid(const Size &bdsOutputSize)
> > +{
> > +     struct ipu3_uapi_grid_config &grid = context_.configuration.af.afGrid;
> > +     grid.width = AF_MIN_GRID_WIDTH;
> > +     grid.height = AF_MIN_GRID_HEIGHT;
> > +     grid.block_width_log2 = AF_MIN_BLOCK_WIDTH;
> > +     grid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;
> > +     grid.height_per_slice = AF_DEFAULT_HEIGHT_PER_SLICE;
> > +     grid.block_width_log2 = AF_MIN_BLOCK_WIDTH;
> > +     grid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;
> > +
> > +     /* x_start and y start are default to BDS center */
> > +     grid.x_start = (bdsOutputSize.width / 2) -
> > +                    (((grid.width << grid.block_width_log2) / 2));
> > +     grid.y_start = (bdsOutputSize.height / 2) -
> > +                    (((grid.height << grid.block_height_log2) / 2));
> > +
> > +     /* x_start and y_start should be even */
> > +     grid.x_start = (grid.x_start / 2) * 2;
> > +     grid.y_start = (grid.y_start / 2) * 2;
> > +     grid.y_start = grid.y_start | IPU3_UAPI_GRID_Y_START_EN;
> > +}
>
> Why are you not doing it in the IPU3Af::configure() call ?
> It would let you remove the [[maybe_unused]] for the IPAConfigInfo
> variable, as you would use it to get the bdsOutputSize ?

The bdsOutputSize could be found in IPAConfigInfo. If you feel good
about this, I'll move them to IPU3Af::configure().

>
> > +
> >   /**
> >    * \brief Configure the IPU3 IPA
> >    * \param[in] configInfo The IPA configuration data, received from the pipeline
> > @@ -461,6 +495,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >
> >       lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
> >
> > +     initAfGrid(configInfo.bdsOutputSize);
> > +
>
> And this would also be removed ?

Once they move to configure(), it can be removed.

>
> >       /* Update the camera controls using the new sensor settings. */
> >       updateControls(sensorInfo_, ctrls_, ipaControls);
> >
>
> I think you could now remove the "RFC" tag.
> You can add my
> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>

Understand, thanks a lot :)



--
BR,
Kate