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

15355 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 statistics.

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>
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
Changes in v7:
1. Improved comments and fixed typo.
2. Simplified AF algorithm. "ignore frame" had been moved into a
function and remove a unnecessary "if" expression.
3. afRawBufferLen_ keeped to be a local variable not a class member.
4. Initial configurations are moved to configure().
---
 src/ipa/ipu3/algorithms/af.cpp      | 426 ++++++++++++++++++++++++++++
 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               |   2 +
 6 files changed, 542 insertions(+), 1 deletion(-)
 create mode 100644 src/ipa/ipu3/algorithms/af.cpp
 create mode 100644 src/ipa/ipu3/algorithms/af.h

Hi Kate,

A few minor style comments in here, but seeing this is at v7 those could
possibly be fixed while applying anyway.

But I'll leave the algorithm review itself to JM.

Quoting Kate Hsuan (2022-02-10 08:57:59)
> 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 statistics.
> 
> 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>
> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

I can't see which values are to be passed to the pipeline handler / lens
yet, but I think that's expected as that plumbing isn't merged. But I
think it's good to progress this, and it could even be merged before the
plumbing.

Overall though, I like this, as I can see how it's acting to sweep
through and when it makes a decision to go back if it passes a peak.

> ---
> Changes in v7:
> 1. Improved comments and fixed typo.
> 2. Simplified AF algorithm. "ignore frame" had been moved into a
> function and remove a unnecessary "if" expression.
> 3. afRawBufferLen_ keeped to be a local variable not a class member.
> 4. Initial configurations are moved to configure().
> ---
>  src/ipa/ipu3/algorithms/af.cpp      | 426 ++++++++++++++++++++++++++++
>  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               |   2 +
>  6 files changed, 542 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..b26aa018
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -0,0 +1,426 @@
> +/* 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 kAfMinGridWidth
> + * \brief the minimum width of AF grid.
> + * The minimum grid horizontal dimensions, in number of grid blocks(cells).
> +*/
> +
> +/**
> + * \var kAfMinGridHeight
> + * \brief the minimum height of AF grid.
> + * The minimum grid vertical dimensions, in number of grid blocks(cells).
> +*/
> +
> +/**
> + * \var kAfMaxGridWidth
> + * \brief the maximum width of AF grid.
> + * The maximum grid horizontal dimensions, in number of grid blocks(cells).
> +*/
> +
> +/**
> + * \var kAfMaxGridHeight
> + * \brief the maximum height of AF grid.
> + * The maximum grid vertical dimensions, in number of grid blocks(cells).
> +*/
> +
> +/**
> + * \var kAfMinGridBlockWidth
> + * \brief the minimum block size of the width.
> + */
> +
> +/**
> + * \var kAfMinGridBlockHeight
> + * \brief the minimum block size of the height.
> + */
> +
> +/**
> + * \def kAfMaxGridBlockWidth
> + * \brief the maximum block size of the width.
> + */
> +
> +/**
> + * \var kAfMaxGridBlockHeight
> + * \brief the maximum block size of the height.
> + */
> +
> +/**
> + * \var kAfDefaultHeightPerSlice
> + * \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 to make a
> + * focused image. The IPU3 AF processing block computes the statistics that
> + * are composed by two types of filtered value and stores in a AF buffer.
> + * Typically, for a clear image, it has a relatively higher contrast than a
> + * blurred one. Therefore, if an image with the highest contrast can be
> + * found through the scan, the position of the len indicates to a clearest
> + * image.
> + *

No need for the blank line (well, it has a star but that can be removed)

> + */
> +
> +LOG_DEFINE_CATEGORY(IPU3Af)
> +
> +/**
> + * Maximum focus steps of the VCM control
> + * \todo should be obtained from the VCM driver
> + */
> +static constexpr uint32_t kMaxFocusSteps = 1023;
> +
> +/* minimum focus step for searching appropriate focus */
> +static constexpr uint32_t kCoarseSearchStep = 30;
> +static constexpr uint32_t kFineSearchStep = 1;
> +
> +/* max ratio of variance change, 0.0 < kMaxChange < 1.0 */
> +static constexpr double kMaxChange = 0.5;
> +
> +/* the numbers of frame to be ignored, before performing focus scan. */
> +static constexpr uint32_t kIgnoreFrame = 10;
> +
> +/* fine scan range 0 < kFineRange < 1 */
> +static constexpr double kFineRange = 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 },
> +};

Are these existing values from the kernel, or is there anything we can
do to reference what they represent or any existing documentation for
how to configure them?

> +
> +Af::Af()
> +       : focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),
> +         coarseCompleted_(false), fineCompleted_(false)
> +{
> +}
> +
> +/**
> + * \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, const IPAConfigInfo &configInfo)
> +{
> +       struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid;
> +       grid.width = kAfMinGridWidth;
> +       grid.height = kAfMinGridHeight;
> +       grid.block_width_log2 = kAfMinGridBlockWidth;
> +       grid.block_height_log2 = kAfMinGridBlockHeight;
> +       grid.height_per_slice = kAfDefaultHeightPerSlice;
> +
> +       /* x_start and y start are default to BDS center */
> +       grid.x_start = (configInfo.bdsOutputSize.width / 2) -
> +                      (((grid.width << grid.block_width_log2) / 2));
> +       grid.y_start = (configInfo.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;
> +
> +       /* inital max focus step */
> +       maxStep_ = kMaxFocusSteps;
> +
> +       /* 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
> + *

We can remove this extra line too. Looks like that's throughout.

> + */
> +void Af::afCoarseScan(IPAContext &context)
> +{
> +       if (coarseCompleted_ == true)
> +               return;
> +
> +       if (afScan(context, kCoarseSearchStep)) {
> +               coarseCompleted_ = true;
> +               context.frameContext.af.maxVariance = 0;
> +               focus_ = context.frameContext.af.focus - (context.frameContext.af.focus * kFineRange);
> +               context.frameContext.af.focus = focus_;
> +               previousVariance_ = 0;
> +               maxStep_ = std::clamp(static_cast<uint32_t>(focus_ + (focus_ * kFineRange)), 0U, kMaxFocusSteps);
> +       }
> +}
> +
> +/**
> + * \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 (coarseCompleted_ != true)
> +               return;
> +
> +       if (afScan(context, kFineSearchStep)) {
> +               context.frameContext.af.stable = true;
> +               fineCompleted_ = true;
> +       }
> +}
> +
> +/**
> + * \brief AF reset
> + *
> + * Reset all the parameters 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_ = kIgnoreFrame;
> +       previousVariance_ = 0.0;
> +       coarseCompleted_ = false;
> +       fineCompleted_ = false;
> +       maxStep_ = kMaxFocusSteps;
> +}
> +
> +/**
> + * \brief AF variance comparison.
> + *
> + * This fuction compares the previous and current variance. It always picks

s/fuction/function/ but I don't think we need to say 'this function'
when describing a function, but it doesn't hurt.

> + * the largest variance to replace the previous one. The image with a larger
> + * variance also indicates it is a clearer image than previous one. If it
> + * finds the negative sign of derivative, it returns immediately.
> + *
> + * \param[in] context The shared IPA context
> + * \return True, if it finds a AF value.
> + */
> +bool Af::afScan(IPAContext &context, int min_step)
> +{
> +       if (focus_ > maxStep_) {
> +               /* if reach the max step, move lens to the position. */
> +               context.frameContext.af.focus = goodFocus_;

I'd be tempted to call goodFocus_ bestFocus_ as it's the 'best' focus
point we've seen? But that's not a requirement. 'good' can also indicate
it was the known 'good' focus that we have stored.


> +               return true;
> +       } else {
> +               /*
> +                * 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.1)) {
> +                       /*
> +                        * positive and zero derivative:
> +                        * The variance is still increasing. The focus could be
> +                        * increased for the next comparison. Also, the max variance
> +                        * and previous focus value are updated.
> +                        */
> +                       goodFocus_ = focus_;
> +                       focus_ += min_step;
> +                       context.frameContext.af.focus = focus_;
> +                       context.frameContext.af.maxVariance = currentVariance_;
> +               } else {
> +                       /*
> +                        * negative derivative:
> +                        * The variance starts to decrease which means the maximum
> +                        * variance is found. Set focus step to previous good one
> +                        * then returnimmediately.

s/returnimmediately./return immediately./

> +                        */
> +                       context.frameContext.af.focus = goodFocus_;
> +                       return true;
> +               }
> +       }
> +
> +       previousVariance_ = currentVariance_;
> +       LOG(IPU3Af, Debug) << " Previous step is "
> +                          << goodFocus_
> +                          << " Current step is "
> +                          << focus_;
> +       return false;
> +}
> +
> +/**
> + * \brief Determine the frame to be ignored.
> + *
> + * \return Return true the frame is ignored.
> + * \return Return false the frame should be processed.
> + */
> +

This blank line can be removed so the comment hugs the function.

> +bool Af::afNeedIgnoreFrame()
> +{
> +       if (ignoreCounter_ == 0)
> +               return false;
> +       else
> +               ignoreCounter_--;
> +       return true;
> +}
> +
> +/**
> + * @brief Reset frame ignore counter.

s/@brief/\brief/ to match doxygen style.

> + *

And no extra line.

> + */
> +void Af::afIgnoreFrameReset()
> +{
> +       ignoreCounter_ = kIgnoreFrame;
> +}
> +
> +/**
> + * \brief Determine the max contrast image and lens position.
> + *
> + * y_table is the AF statistic of IPU3 and is composed of two kinds of filtered
> + * values. Based on this, the variance of a image could be used to determine

Do we know what the values represent? Could we say perhaps:

"""
y_table is the AF statistic of IPU3 and is composed of two kinds of
filtered values representing the contrast levels of the image.
"""

However, don't take that as verbatim, I'm guessing based on context ;-)


> + * the clearness of the given image. Ideally, a clear image also has a
> + * raletively higher contrast. So, the VCM is moved step by step and variance

s/raletively/relatively/

> + * of each frame are calculated to find a maximum variance which corresponds
> + * with a specific focus step. If it is found, that is the best focus step of
> + * current scene.
> + *
> + * \param[in] context The shared IPA context.
> + * \param[in] stats The statistic buffer of 3A of 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;
> +       uint32_t afRawBufferLen_;
> +
> +       /* evaluate the AF buffer length */
> +       afRawBufferLen_ = context.configuration.af.afGrid.width *
> +                         context.configuration.af.afGrid.height;
> +
> +       memcpy(y_item, stats->af_raw_buffer.y_table, afRawBufferLen_ * sizeof(y_table_item_t));
> +
> +       /*
> +        * calculate the mean and the variance of AF statistics for a given grid.
> +        *
> +        * For coarse: y1 are used.
> +        * For fine: y2 results are used.
> +        */
> +       if (coarseCompleted_) {
> +               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));
> +               }
> +       } 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));
> +               }
> +       }
> +
> +       /* determine the average variance of the AF statistic. */
> +       currentVariance_ = static_cast<double>(var_sum) / static_cast<double>(afRawBufferLen_);
> +
> +       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;
> +               LOG(IPU3Af, Debug) << "Variance change rate: "
> +                                  << var_ratio
> +                                  << " Current VCM step: "
> +                                  << context.frameContext.af.focus;
> +
> +               /*
> +                * If the change rate is greater than kMaxChange (out of focus),
> +                * trigger AF again.
> +                */
> +               if (var_ratio > kMaxChange) {
> +                       if (!afNeedIgnoreFrame())
> +                               afReset(context);
> +               } else
> +                       afIgnoreFrameReset();
> +       } else {
> +               if (!afNeedIgnoreFrame()) {
> +                       afCoarseScan(context);
> +                       afFineScan(context);
> +               }
> +       }
> +}
> +
> +} /* 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..bc5d2064
> --- /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__

We can use #pragma once here now. (The whole code base was converted).

> +
> +#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 kAfMinGridWidth = 16;
> +static constexpr uint8_t kAfMinGridHeight = 16;
> +static constexpr uint8_t kAfMaxGridWidth = 32;
> +static constexpr uint8_t kAfMaxGridHeight = 24;
> +static constexpr uint16_t kAfMinGridBlockWidth = 4;
> +static constexpr uint16_t kAfMinGridBlockHeight = 3;
> +static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> +static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> +static constexpr uint16_t kAfDefaultHeightPerSlice = 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() = default;
> +
> +       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);
> +       bool afNeedIgnoreFrame();
> +       void afIgnoreFrameReset();
> +
> +       /* Used for focus scan. */

If we're describing it, what is it? Is it the current focus position
when scanning?

> +       uint32_t focus_;
> +       /* Focus good */

Focus Good doesn't really describe a variable called good Focus. What
gets put in it? Is it the last known good focus value?

> +       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 coarseCompleted_;
> +       /* fine scan stable. Complete high pass scan (fine scan) */
> +       bool fineCompleted_;
> +};
> +
> +} /* 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'

We should keep the ',' at the end of tone_mapping.cpp so that we don't have to
modify extra lines when appending to the list.

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

No need for extra blank line

> + */
> +
> +/**
> + * \var IPASessionConfiguration::af
> + * \brief AF grid configuration of the IPA
> + *
> + * \var IPASessionConfiguration::af.afGrid
> + *

No need for extra blank line

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

Is this the focus point/value of the current frame, or the desired value to
set on the next frame?

> +               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 e44a31bb..417e0562 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"
> @@ -295,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>());
> -- 
> 2.33.1
>

Hi Kate,

Thanks for the patch !

On 10/02/2022 11:09, Kieran Bingham wrote:
> Hi Kate,
> 
> A few minor style comments in here, but seeing this is at v7 those could
> possibly be fixed while applying anyway.
> 
> But I'll leave the algorithm review itself to JM.

Sure. Thanks (?) ;-).

> 
> Quoting Kate Hsuan (2022-02-10 08:57:59)
>> 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 statistics.
>>
>> 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>
>> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> I can't see which values are to be passed to the pipeline handler / lens
> yet, but I think that's expected as that plumbing isn't merged. But I
> think it's good to progress this, and it could even be merged before the
> plumbing.

It could indeed be interesting to at least refer to the ancillary links 
patch series from Dan [1] and [2]. Those are needed for this algorithm 
to have an effect :-).

[1]: https://lore.kernel.org/all/20220130235821.48076-1-djrscally@gmail.com/
[2]: https://patchwork.libcamera.org/project/libcamera/list/?series=2906

> 
> Overall though, I like this, as I can see how it's acting to sweep
> through and when it makes a decision to go back if it passes a peak.
> 
>> ---
>> Changes in v7:
>> 1. Improved comments and fixed typo.
>> 2. Simplified AF algorithm. "ignore frame" had been moved into a
>> function and remove a unnecessary "if" expression.
>> 3. afRawBufferLen_ keeped to be a local variable not a class member.
>> 4. Initial configurations are moved to configure().
>> ---
>>   src/ipa/ipu3/algorithms/af.cpp      | 426 ++++++++++++++++++++++++++++
>>   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               |   2 +
>>   6 files changed, 542 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..b26aa018
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/af.cpp
>> @@ -0,0 +1,426 @@
>> +/* 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 kAfMinGridWidth
>> + * \brief the minimum width of AF grid.
>> + * The minimum grid horizontal dimensions, in number of grid blocks(cells).
>> +*/
>> +
>> +/**
>> + * \var kAfMinGridHeight
>> + * \brief the minimum height of AF grid.
>> + * The minimum grid vertical dimensions, in number of grid blocks(cells).
>> +*/
>> +
>> +/**
>> + * \var kAfMaxGridWidth
>> + * \brief the maximum width of AF grid.
>> + * The maximum grid horizontal dimensions, in number of grid blocks(cells).
>> +*/
>> +
>> +/**
>> + * \var kAfMaxGridHeight
>> + * \brief the maximum height of AF grid.
>> + * The maximum grid vertical dimensions, in number of grid blocks(cells).
>> +*/
>> +
>> +/**
>> + * \var kAfMinGridBlockWidth
>> + * \brief the minimum block size of the width.
>> + */
>> +
>> +/**
>> + * \var kAfMinGridBlockHeight
>> + * \brief the minimum block size of the height.
>> + */
>> +
>> +/**
>> + * \def kAfMaxGridBlockWidth
>> + * \brief the maximum block size of the width.
>> + */
>> +
>> +/**
>> + * \var kAfMaxGridBlockHeight
>> + * \brief the maximum block size of the height.
>> + */
>> +
>> +/**
>> + * \var kAfDefaultHeightPerSlice
>> + * \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 to make a
>> + * focused image. The IPU3 AF processing block computes the statistics that
>> + * are composed by two types of filtered value and stores in a AF buffer.
>> + * Typically, for a clear image, it has a relatively higher contrast than a
>> + * blurred one. Therefore, if an image with the highest contrast can be
>> + * found through the scan, the position of the len indicates to a clearest
>> + * image.
>> + *
> 
> No need for the blank line (well, it has a star but that can be removed)
> 
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Af)
>> +
>> +/**
>> + * Maximum focus steps of the VCM control
>> + * \todo should be obtained from the VCM driver
>> + */
>> +static constexpr uint32_t kMaxFocusSteps = 1023;
>> +
>> +/* minimum focus step for searching appropriate focus */
>> +static constexpr uint32_t kCoarseSearchStep = 30;
>> +static constexpr uint32_t kFineSearchStep = 1;
>> +
>> +/* max ratio of variance change, 0.0 < kMaxChange < 1.0 */
>> +static constexpr double kMaxChange = 0.5;
>> +
>> +/* the numbers of frame to be ignored, before performing focus scan. */
>> +static constexpr uint32_t kIgnoreFrame = 10;
>> +
>> +/* fine scan range 0 < kFineRange < 1 */
>> +static constexpr double kFineRange = 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 },
>> +};
> 
> Are these existing values from the kernel, or is there anything we can
> do to reference what they represent or any existing documentation for
> how to configure them?
> 

Those are the values used by the intel closed source library, not the 
default kernel ones (which are identical for both y1 and y2). I don't 
know if Kate has a complete description of those ?

>> +
>> +Af::Af()
>> +       : focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),
>> +         coarseCompleted_(false), fineCompleted_(false)
>> +{
>> +}
>> +
>> +/**
>> + * \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, const IPAConfigInfo &configInfo)
>> +{
>> +       struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid;
>> +       grid.width = kAfMinGridWidth;
>> +       grid.height = kAfMinGridHeight;
>> +       grid.block_width_log2 = kAfMinGridBlockWidth;
>> +       grid.block_height_log2 = kAfMinGridBlockHeight;
>> +       grid.height_per_slice = kAfDefaultHeightPerSlice;
>> +
>> +       /* x_start and y start are default to BDS center */
>> +       grid.x_start = (configInfo.bdsOutputSize.width / 2) -
>> +                      (((grid.width << grid.block_width_log2) / 2));
>> +       grid.y_start = (configInfo.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;
>> +
>> +       /* inital max focus step */
>> +       maxStep_ = kMaxFocusSteps;
>> +
>> +       /* 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
>> + *
> 
> We can remove this extra line too. Looks like that's throughout.
> 
>> + */
>> +void Af::afCoarseScan(IPAContext &context)
>> +{
>> +       if (coarseCompleted_ == true)
>> +               return;
>> +
>> +       if (afScan(context, kCoarseSearchStep)) {
>> +               coarseCompleted_ = true;
>> +               context.frameContext.af.maxVariance = 0;
>> +               focus_ = context.frameContext.af.focus - (context.frameContext.af.focus * kFineRange);
>> +               context.frameContext.af.focus = focus_;
>> +               previousVariance_ = 0;
>> +               maxStep_ = std::clamp(static_cast<uint32_t>(focus_ + (focus_ * kFineRange)), 0U, kMaxFocusSteps);
>> +       }
>> +}
>> +
>> +/**
>> + * \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 (coarseCompleted_ != true)
>> +               return;
>> +
>> +       if (afScan(context, kFineSearchStep)) {
>> +               context.frameContext.af.stable = true;
>> +               fineCompleted_ = true;
>> +       }
>> +}
>> +
>> +/**
>> + * \brief AF reset
>> + *
>> + * Reset all the parameters 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_ = kIgnoreFrame;
>> +       previousVariance_ = 0.0;
>> +       coarseCompleted_ = false;
>> +       fineCompleted_ = false;
>> +       maxStep_ = kMaxFocusSteps;
>> +}
>> +
>> +/**
>> + * \brief AF variance comparison.
>> + *
>> + * This fuction compares the previous and current variance. It always picks
> 
> s/fuction/function/ but I don't think we need to say 'this function'
> when describing a function, but it doesn't hurt.
> 
>> + * the largest variance to replace the previous one. The image with a larger
>> + * variance also indicates it is a clearer image than previous one. If it
>> + * finds the negative sign of derivative, it returns immediately.
>> + *
>> + * \param[in] context The shared IPA context
>> + * \return True, if it finds a AF value.
>> + */
>> +bool Af::afScan(IPAContext &context, int min_step)
>> +{
>> +       if (focus_ > maxStep_) {
>> +               /* if reach the max step, move lens to the position. */
>> +               context.frameContext.af.focus = goodFocus_;
> 
> I'd be tempted to call goodFocus_ bestFocus_ as it's the 'best' focus
> point we've seen? But that's not a requirement. 'good' can also indicate
> it was the known 'good' focus that we have stored.
> 
> 
>> +               return true;
>> +       } else {
>> +               /*
>> +                * 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.1)) {
>> +                       /*
>> +                        * positive and zero derivative:
>> +                        * The variance is still increasing. The focus could be
>> +                        * increased for the next comparison. Also, the max variance
>> +                        * and previous focus value are updated.
>> +                        */
>> +                       goodFocus_ = focus_;
>> +                       focus_ += min_step;
>> +                       context.frameContext.af.focus = focus_;
>> +                       context.frameContext.af.maxVariance = currentVariance_;
>> +               } else {
>> +                       /*
>> +                        * negative derivative:
>> +                        * The variance starts to decrease which means the maximum
>> +                        * variance is found. Set focus step to previous good one
>> +                        * then returnimmediately.
> 
> s/returnimmediately./return immediately./
> 
>> +                        */
>> +                       context.frameContext.af.focus = goodFocus_;
>> +                       return true;
>> +               }
>> +       }
>> +
>> +       previousVariance_ = currentVariance_;
>> +       LOG(IPU3Af, Debug) << " Previous step is "
>> +                          << goodFocus_
>> +                          << " Current step is "
>> +                          << focus_;
>> +       return false;
>> +}
>> +
>> +/**
>> + * \brief Determine the frame to be ignored.
>> + *
>> + * \return Return true the frame is ignored.
>> + * \return Return false the frame should be processed.
>> + */
>> +
> 
> This blank line can be removed so the comment hugs the function.
> 
>> +bool Af::afNeedIgnoreFrame()
>> +{
>> +       if (ignoreCounter_ == 0)
>> +               return false;
>> +       else
>> +               ignoreCounter_--;
>> +       return true;
>> +}
>> +
>> +/**
>> + * @brief Reset frame ignore counter.
> 
> s/@brief/\brief/ to match doxygen style.
> 
>> + *
> 
> And no extra line.
> 
>> + */
>> +void Af::afIgnoreFrameReset()
>> +{
>> +       ignoreCounter_ = kIgnoreFrame;
>> +}
>> +
>> +/**
>> + * \brief Determine the max contrast image and lens position.
>> + *
>> + * y_table is the AF statistic of IPU3 and is composed of two kinds of filtered
>> + * values. Based on this, the variance of a image could be used to determine
> 
> Do we know what the values represent? Could we say perhaps:
> 
> """
> y_table is the AF statistic of IPU3 and is composed of two kinds of
> filtered values representing the contrast levels of the image.
> """
> 
> However, don't take that as verbatim, I'm guessing based on context ;-)
> 
> 
>> + * the clearness of the given image. Ideally, a clear image also has a
>> + * raletively higher contrast. So, the VCM is moved step by step and variance
> 
> s/raletively/relatively/
> 
>> + * of each frame are calculated to find a maximum variance which corresponds
>> + * with a specific focus step. If it is found, that is the best focus step of
>> + * current scene.
>> + *
>> + * \param[in] context The shared IPA context.
>> + * \param[in] stats The statistic buffer of 3A of 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;
>> +       uint32_t afRawBufferLen_;
>> +
>> +       /* evaluate the AF buffer length */
>> +       afRawBufferLen_ = context.configuration.af.afGrid.width *
>> +                         context.configuration.af.afGrid.height;
>> +
>> +       memcpy(y_item, stats->af_raw_buffer.y_table, afRawBufferLen_ * sizeof(y_table_item_t));
>> +
>> +       /*
>> +        * calculate the mean and the variance of AF statistics for a given grid.
>> +        *
>> +        * For coarse: y1 are used.
>> +        * For fine: y2 results are used.
>> +        */
>> +       if (coarseCompleted_) {
>> +               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));

var_sum += ((y_item[z].y2_avg - mean) *
	    (y_item[z].y2_avg - mean));

same below.

I would have loved seeing a function like estimateVariance(y_item) which 
would contain all the calculation and return the variance as a double ;-).

>> +               }
>> +       } 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));
>> +               }
>> +       }
>> +
>> +       /* determine the average variance of the AF statistic. */
>> +       currentVariance_ = static_cast<double>(var_sum) / static_cast<double>(afRawBufferLen_);
>> +
>> +       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;
>> +               LOG(IPU3Af, Debug) << "Variance change rate: "
>> +                                  << var_ratio
>> +                                  << " Current VCM step: "
>> +                                  << context.frameContext.af.focus;
>> +
>> +               /*
>> +                * If the change rate is greater than kMaxChange (out of focus),
>> +                * trigger AF again.
>> +                */
>> +               if (var_ratio > kMaxChange) {
>> +                       if (!afNeedIgnoreFrame())
>> +                               afReset(context);
>> +               } else
>> +                       afIgnoreFrameReset();
>> +       } else {
>> +               if (!afNeedIgnoreFrame()) {
>> +                       afCoarseScan(context);
>> +                       afFineScan(context);
>> +               }
>> +       }

This is a matter of taste. As if we are stable we are not just returning 
early, I would have inverted the conditional, to have:

if (!context.frameContext.af.stable) {
	scan();
} else {
	double varRatio = estimateRatio();
	/*
	 * If the change rate is greater than kMaxRatio, we are out
	 * of focus and need to trigger AF again.
	 */
	if (varRatio > kMaxRatio)
		afReset(context);
	else
		ignoreCounter_ = kIgnoreFrame;
}

You could change afReset and have:
void Af::afReset(IPAContext &context)
{
	if (afNeedIgnoreFrame())
		return;

	context.frameContext.af.maxVariance = 0;
	context.frameContext.af.focus = 0;
	focus_ = 0;
	context.frameContext.af.stable = false;
	ignoreCounter_ = kIgnoreFrame;
	previousVariance_ = 0.0;
	coarseCompleted_ = false;
	fineCompleted_ = false;
	maxStep_ = kMaxFocusSteps;
}

Not sure if it is better, but as you call afReset only in the case 
!afNeedIgnoreFrame() it could simplify the process() reading.

>> +}
>> +
>> +} /* 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..bc5d2064
>> --- /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__
> 
> We can use #pragma once here now. (The whole code base was converted).
> 
>> +
>> +#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 kAfMinGridWidth = 16;
>> +static constexpr uint8_t kAfMinGridHeight = 16;
>> +static constexpr uint8_t kAfMaxGridWidth = 32;
>> +static constexpr uint8_t kAfMaxGridHeight = 24;
>> +static constexpr uint16_t kAfMinGridBlockWidth = 4;
>> +static constexpr uint16_t kAfMinGridBlockHeight = 3;
>> +static constexpr uint16_t kAfMaxGridBlockWidth = 6;
>> +static constexpr uint16_t kAfMaxGridBlockHeight = 6;
>> +static constexpr uint16_t kAfDefaultHeightPerSlice = 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() = default;
>> +
>> +       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);
>> +       bool afNeedIgnoreFrame();
>> +       void afIgnoreFrameReset();
>> +
>> +       /* Used for focus scan. */
> 
> If we're describing it, what is it? Is it the current focus position
> when scanning?
> 
>> +       uint32_t focus_;
>> +       /* Focus good */
> 
> Focus Good doesn't really describe a variable called good Focus. What
> gets put in it? Is it the last known good focus value?
> 
>> +       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 coarseCompleted_;
>> +       /* fine scan stable. Complete high pass scan (fine scan) */
>> +       bool fineCompleted_;
>> +};
>> +
>> +} /* 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'
> 
> We should keep the ',' at the end of tone_mapping.cpp so that we don't have to
> modify extra lines when appending to the list.
> 
>>   ])
>> 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
>> + *
> 
> No need for extra blank line
> 
>> + */
>> +
>> +/**
>> + * \var IPASessionConfiguration::af
>> + * \brief AF grid configuration of the IPA
>> + *
>> + * \var IPASessionConfiguration::af.afGrid
>> + *
> 
> No need for extra blank line
> 
>> + */
>> +
>> +/**
>> + * \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;
> 
> Is this the focus point/value of the current frame, or the desired value to
> set on the next frame?
> 
>> +               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 e44a31bb..417e0562 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"
>> @@ -295,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>());
>> -- 
>> 2.33.1
>>

)Hi Jean-Michel and Kieran,

Thanks for your feedback.

On Thu, Feb 10, 2022 at 11:43 PM Jean-Michel Hautbois
<jeanmichel.hautbois@ideasonboard.com> wrote:
>
> Hi Kate,
>
> Thanks for the patch !
>
> On 10/02/2022 11:09, Kieran Bingham wrote:
> > Hi Kate,
> >
> > A few minor style comments in here, but seeing this is at v7 those could
> > possibly be fixed while applying anyway.
> >
> > But I'll leave the algorithm review itself to JM.
>
> Sure. Thanks (?) ;-).

Once again, thank you, folks :)

>
> >
> > Quoting Kate Hsuan (2022-02-10 08:57:59)
> >> 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 statistics.
> >>
> >> 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>
> >> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >
> > I can't see which values are to be passed to the pipeline handler / lens
> > yet, but I think that's expected as that plumbing isn't merged. But I
> > think it's good to progress this, and it could even be merged before the
> > plumbing.
>
> It could indeed be interesting to at least refer to the ancillary links
> patch series from Dan [1] and [2]. Those are needed for this algorithm
> to have an effect :-).
>
> [1]: https://lore.kernel.org/all/20220130235821.48076-1-djrscally@gmail.com/
> [2]: https://patchwork.libcamera.org/project/libcamera/list/?series=2906
>

Thanks to the folks' works :)

> >
> > Overall though, I like this, as I can see how it's acting to sweep
> > through and when it makes a decision to go back if it passes a peak.
> >
> >> ---
> >> Changes in v7:
> >> 1. Improved comments and fixed typo.
> >> 2. Simplified AF algorithm. "ignore frame" had been moved into a
> >> function and remove a unnecessary "if" expression.
> >> 3. afRawBufferLen_ keeped to be a local variable not a class member.
> >> 4. Initial configurations are moved to configure().
> >> ---
> >>   src/ipa/ipu3/algorithms/af.cpp      | 426 ++++++++++++++++++++++++++++
> >>   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               |   2 +
> >>   6 files changed, 542 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..b26aa018
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/algorithms/af.cpp
> >> @@ -0,0 +1,426 @@
> >> +/* 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 kAfMinGridWidth
> >> + * \brief the minimum width of AF grid.
> >> + * The minimum grid horizontal dimensions, in number of grid blocks(cells).
> >> +*/
> >> +
> >> +/**
> >> + * \var kAfMinGridHeight
> >> + * \brief the minimum height of AF grid.
> >> + * The minimum grid vertical dimensions, in number of grid blocks(cells).
> >> +*/
> >> +
> >> +/**
> >> + * \var kAfMaxGridWidth
> >> + * \brief the maximum width of AF grid.
> >> + * The maximum grid horizontal dimensions, in number of grid blocks(cells).
> >> +*/
> >> +
> >> +/**
> >> + * \var kAfMaxGridHeight
> >> + * \brief the maximum height of AF grid.
> >> + * The maximum grid vertical dimensions, in number of grid blocks(cells).
> >> +*/
> >> +
> >> +/**
> >> + * \var kAfMinGridBlockWidth
> >> + * \brief the minimum block size of the width.
> >> + */
> >> +
> >> +/**
> >> + * \var kAfMinGridBlockHeight
> >> + * \brief the minimum block size of the height.
> >> + */
> >> +
> >> +/**
> >> + * \def kAfMaxGridBlockWidth
> >> + * \brief the maximum block size of the width.
> >> + */
> >> +
> >> +/**
> >> + * \var kAfMaxGridBlockHeight
> >> + * \brief the maximum block size of the height.
> >> + */
> >> +
> >> +/**
> >> + * \var kAfDefaultHeightPerSlice
> >> + * \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 to make a
> >> + * focused image. The IPU3 AF processing block computes the statistics that
> >> + * are composed by two types of filtered value and stores in a AF buffer.
> >> + * Typically, for a clear image, it has a relatively higher contrast than a
> >> + * blurred one. Therefore, if an image with the highest contrast can be
> >> + * found through the scan, the position of the len indicates to a clearest
> >> + * image.
> >> + *
> >
> > No need for the blank line (well, it has a star but that can be removed)


> >
> >> + */
> >> +
> >> +LOG_DEFINE_CATEGORY(IPU3Af)
> >> +
> >> +/**
> >> + * Maximum focus steps of the VCM control
> >> + * \todo should be obtained from the VCM driver
> >> + */
> >> +static constexpr uint32_t kMaxFocusSteps = 1023;
> >> +
> >> +/* minimum focus step for searching appropriate focus */
> >> +static constexpr uint32_t kCoarseSearchStep = 30;
> >> +static constexpr uint32_t kFineSearchStep = 1;
> >> +
> >> +/* max ratio of variance change, 0.0 < kMaxChange < 1.0 */
> >> +static constexpr double kMaxChange = 0.5;
> >> +
> >> +/* the numbers of frame to be ignored, before performing focus scan. */
> >> +static constexpr uint32_t kIgnoreFrame = 10;
> >> +
> >> +/* fine scan range 0 < kFineRange < 1 */
> >> +static constexpr double kFineRange = 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 },
> >> +};
> >
> > Are these existing values from the kernel, or is there anything we can
> > do to reference what they represent or any existing documentation for
> > how to configure them?
> >
>
> Those are the values used by the intel closed source library, not the
> default kernel ones (which are identical for both y1 and y2). I don't
> know if Kate has a complete description of those ?
>

Actually, I can't. I have to thank JM to provide this solution and
secret numbers :).

> >> +
> >> +Af::Af()
> >> +       : focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),
> >> +         coarseCompleted_(false), fineCompleted_(false)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \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, const IPAConfigInfo &configInfo)
> >> +{
> >> +       struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid;
> >> +       grid.width = kAfMinGridWidth;
> >> +       grid.height = kAfMinGridHeight;
> >> +       grid.block_width_log2 = kAfMinGridBlockWidth;
> >> +       grid.block_height_log2 = kAfMinGridBlockHeight;
> >> +       grid.height_per_slice = kAfDefaultHeightPerSlice;
> >> +
> >> +       /* x_start and y start are default to BDS center */
> >> +       grid.x_start = (configInfo.bdsOutputSize.width / 2) -
> >> +                      (((grid.width << grid.block_width_log2) / 2));
> >> +       grid.y_start = (configInfo.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;
> >> +
> >> +       /* inital max focus step */
> >> +       maxStep_ = kMaxFocusSteps;
> >> +
> >> +       /* 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
> >> + *
> >
> > We can remove this extra line too. Looks like that's throughout.
> >
> >> + */
> >> +void Af::afCoarseScan(IPAContext &context)
> >> +{
> >> +       if (coarseCompleted_ == true)
> >> +               return;
> >> +
> >> +       if (afScan(context, kCoarseSearchStep)) {
> >> +               coarseCompleted_ = true;
> >> +               context.frameContext.af.maxVariance = 0;
> >> +               focus_ = context.frameContext.af.focus - (context.frameContext.af.focus * kFineRange);
> >> +               context.frameContext.af.focus = focus_;
> >> +               previousVariance_ = 0;
> >> +               maxStep_ = std::clamp(static_cast<uint32_t>(focus_ + (focus_ * kFineRange)), 0U, kMaxFocusSteps);
> >> +       }
> >> +}
> >> +
> >> +/**
> >> + * \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 (coarseCompleted_ != true)
> >> +               return;
> >> +
> >> +       if (afScan(context, kFineSearchStep)) {
> >> +               context.frameContext.af.stable = true;
> >> +               fineCompleted_ = true;
> >> +       }
> >> +}
> >> +
> >> +/**
> >> + * \brief AF reset
> >> + *
> >> + * Reset all the parameters 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_ = kIgnoreFrame;
> >> +       previousVariance_ = 0.0;
> >> +       coarseCompleted_ = false;
> >> +       fineCompleted_ = false;
> >> +       maxStep_ = kMaxFocusSteps;
> >> +}
> >> +
> >> +/**
> >> + * \brief AF variance comparison.
> >> + *
> >> + * This fuction compares the previous and current variance. It always picks
> >
> > s/fuction/function/ but I don't think we need to say 'this function'
> > when describing a function, but it doesn't hurt.

OK

> >
> >> + * the largest variance to replace the previous one. The image with a larger
> >> + * variance also indicates it is a clearer image than previous one. If it
> >> + * finds the negative sign of derivative, it returns immediately.
> >> + *
> >> + * \param[in] context The shared IPA context
> >> + * \return True, if it finds a AF value.
> >> + */
> >> +bool Af::afScan(IPAContext &context, int min_step)
> >> +{
> >> +       if (focus_ > maxStep_) {
> >> +               /* if reach the max step, move lens to the position. */
> >> +               context.frameContext.af.focus = goodFocus_;
> >
> > I'd be tempted to call goodFocus_ bestFocus_ as it's the 'best' focus
> > point we've seen? But that's not a requirement. 'good' can also indicate
> > it was the known 'good' focus that we have stored.

It holds a local maximum (local best) value and finally, it will
become the best focus value (global best) of the entire AF process.
I think it can be called "bestFocus_". I used the term "good" to
prevent misunderstanding.

> >
> >
> >> +               return true;
> >> +       } else {
> >> +               /*
> >> +                * 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.1)) {
> >> +                       /*
> >> +                        * positive and zero derivative:
> >> +                        * The variance is still increasing. The focus could be
> >> +                        * increased for the next comparison. Also, the max variance
> >> +                        * and previous focus value are updated.
> >> +                        */
> >> +                       goodFocus_ = focus_;
> >> +                       focus_ += min_step;
> >> +                       context.frameContext.af.focus = focus_;
> >> +                       context.frameContext.af.maxVariance = currentVariance_;
> >> +               } else {
> >> +                       /*
> >> +                        * negative derivative:
> >> +                        * The variance starts to decrease which means the maximum
> >> +                        * variance is found. Set focus step to previous good one
> >> +                        * then returnimmediately.
> >
> > s/returnimmediately./return immediately./
> >
> >> +                        */
> >> +                       context.frameContext.af.focus = goodFocus_;
> >> +                       return true;
> >> +               }
> >> +       }
> >> +
> >> +       previousVariance_ = currentVariance_;
> >> +       LOG(IPU3Af, Debug) << " Previous step is "
> >> +                          << goodFocus_
> >> +                          << " Current step is "
> >> +                          << focus_;
> >> +       return false;
> >> +}
> >> +
> >> +/**
> >> + * \brief Determine the frame to be ignored.
> >> + *
> >> + * \return Return true the frame is ignored.
> >> + * \return Return false the frame should be processed.
> >> + */
> >> +
> >
> > This blank line can be removed so the comment hugs the function.
> >
> >> +bool Af::afNeedIgnoreFrame()
> >> +{
> >> +       if (ignoreCounter_ == 0)
> >> +               return false;
> >> +       else
> >> +               ignoreCounter_--;
> >> +       return true;
> >> +}
> >> +
> >> +/**
> >> + * @brief Reset frame ignore counter.
> >
> > s/@brief/\brief/ to match doxygen style.
> >
> >> + *
> >
> > And no extra line.
> >
> >> + */
> >> +void Af::afIgnoreFrameReset()
> >> +{
> >> +       ignoreCounter_ = kIgnoreFrame;
> >> +}
> >> +
> >> +/**
> >> + * \brief Determine the max contrast image and lens position.
> >> + *
> >> + * y_table is the AF statistic of IPU3 and is composed of two kinds of filtered
> >> + * values. Based on this, the variance of a image could be used to determine
> >
> > Do we know what the values represent? Could we say perhaps:

This is my understanding. y_table holds y1 and y2 filtered results
which are controlled by the filter configuration. (you can see from
the afFilterConfigDefault)
So, those y1 and y2 can be kinds of the result of edge detection of IPU3.

> >
> > """
> > y_table is the AF statistic of IPU3 and is composed of two kinds of
> > filtered values representing the contrast levels of the image.
> > """
> >
> > However, don't take that as verbatim, I'm guessing based on context ;-)

ok, I'll revise this.
> >
> >
> >> + * the clearness of the given image. Ideally, a clear image also has a
> >> + * raletively higher contrast. So, the VCM is moved step by step and variance
> >
> > s/raletively/relatively/
> >
> >> + * of each frame are calculated to find a maximum variance which corresponds
> >> + * with a specific focus step. If it is found, that is the best focus step of
> >> + * current scene.
> >> + *
> >> + * \param[in] context The shared IPA context.
> >> + * \param[in] stats The statistic buffer of 3A of 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;
> >> +       uint32_t afRawBufferLen_;
> >> +
> >> +       /* evaluate the AF buffer length */
> >> +       afRawBufferLen_ = context.configuration.af.afGrid.width *
> >> +                         context.configuration.af.afGrid.height;
> >> +
> >> +       memcpy(y_item, stats->af_raw_buffer.y_table, afRawBufferLen_ * sizeof(y_table_item_t));
> >> +
> >> +       /*
> >> +        * calculate the mean and the variance of AF statistics for a given grid.
> >> +        *
> >> +        * For coarse: y1 are used.
> >> +        * For fine: y2 results are used.
> >> +        */
> >> +       if (coarseCompleted_) {
> >> +               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));
>
> var_sum += ((y_item[z].y2_avg - mean) *
>             (y_item[z].y2_avg - mean));
>
> same below.
>
> I would have loved seeing a function like estimateVariance(y_item) which
> would contain all the calculation and return the variance as a double ;-).

OK

>
> >> +               }
> >> +       } 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));
> >> +               }
> >> +       }
> >> +
> >> +       /* determine the average variance of the AF statistic. */
> >> +       currentVariance_ = static_cast<double>(var_sum) / static_cast<double>(afRawBufferLen_);
> >> +
> >> +       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;
> >> +               LOG(IPU3Af, Debug) << "Variance change rate: "
> >> +                                  << var_ratio
> >> +                                  << " Current VCM step: "
> >> +                                  << context.frameContext.af.focus;
> >> +
> >> +               /*
> >> +                * If the change rate is greater than kMaxChange (out of focus),
> >> +                * trigger AF again.
> >> +                */
> >> +               if (var_ratio > kMaxChange) {
> >> +                       if (!afNeedIgnoreFrame())
> >> +                               afReset(context);
> >> +               } else
> >> +                       afIgnoreFrameReset();
> >> +       } else {
> >> +               if (!afNeedIgnoreFrame()) {
> >> +                       afCoarseScan(context);
> >> +                       afFineScan(context);
> >> +               }
> >> +       }
>
> This is a matter of taste. As if we are stable we are not just returning
> early, I would have inverted the conditional, to have:
>
> if (!context.frameContext.af.stable) {
>         scan();
> } else {
>         double varRatio = estimateRatio();
>         /*
>          * If the change rate is greater than kMaxRatio, we are out
>          * of focus and need to trigger AF again.
>          */
>         if (varRatio > kMaxRatio)
>                 afReset(context);
>         else
>                 ignoreCounter_ = kIgnoreFrame;
> }
>
> You could change afReset and have:
> void Af::afReset(IPAContext &context)
> {
>         if (afNeedIgnoreFrame())
>                 return;
>
>         context.frameContext.af.maxVariance = 0;
>         context.frameContext.af.focus = 0;
>         focus_ = 0;
>         context.frameContext.af.stable = false;
>         ignoreCounter_ = kIgnoreFrame;
>         previousVariance_ = 0.0;
>         coarseCompleted_ = false;
>         fineCompleted_ = false;
>         maxStep_ = kMaxFocusSteps;
> }
>
> Not sure if it is better, but as you call afReset only in the case
> !afNeedIgnoreFrame() it could simplify the process() reading.

Got it. I'll move all the detail to a function and only put the
algorithm itself in the process().

>
> >> +}
> >> +
> >> +} /* 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..bc5d2064
> >> --- /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__
> >
> > We can use #pragma once here now. (The whole code base was converted).
> >
> >> +
> >> +#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 kAfMinGridWidth = 16;
> >> +static constexpr uint8_t kAfMinGridHeight = 16;
> >> +static constexpr uint8_t kAfMaxGridWidth = 32;
> >> +static constexpr uint8_t kAfMaxGridHeight = 24;
> >> +static constexpr uint16_t kAfMinGridBlockWidth = 4;
> >> +static constexpr uint16_t kAfMinGridBlockHeight = 3;
> >> +static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> >> +static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> >> +static constexpr uint16_t kAfDefaultHeightPerSlice = 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() = default;
> >> +
> >> +       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);
> >> +       bool afNeedIgnoreFrame();
> >> +       void afIgnoreFrameReset();
> >> +
> >> +       /* Used for focus scan. */
> >
> > If we're describing it, what is it? Is it the current focus position
> > when scanning?
> >
> >> +       uint32_t focus_;
> >> +       /* Focus good */
> >
> > Focus Good doesn't really describe a variable called good Focus. What
> > gets put in it? Is it the last known good focus value?

I'll revise this part.

> >
> >> +       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 coarseCompleted_;
> >> +       /* fine scan stable. Complete high pass scan (fine scan) */
> >> +       bool fineCompleted_;
> >> +};
> >> +
> >> +} /* 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'
> >
> > We should keep the ',' at the end of tone_mapping.cpp so that we don't have to
> > modify extra lines when appending to the list.

OK

> >
> >>   ])
> >> 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
> >> + *
> >
> > No need for extra blank line
> >
> >> + */
> >> +
> >> +/**
> >> + * \var IPASessionConfiguration::af
> >> + * \brief AF grid configuration of the IPA
> >> + *
> >> + * \var IPASessionConfiguration::af.afGrid
> >> + *
> >
> > No need for extra blank line
> >
> >> + */
> >> +
> >> +/**
> >> + * \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;
> >
> > Is this the focus point/value of the current frame, or the desired value to
> > set on the next frame?
> >
> >> +               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 e44a31bb..417e0562 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"
> >> @@ -295,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>());
> >> --
> >> 2.33.1
> >>
>