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

15289 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.

The variance of each focus step is determined and a greedy
approah is used to find the maximum variance of the AF
state and a 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      | 367 ++++++++++++++++++++++++++++
 src/ipa/ipu3/algorithms/af.h        |  79 ++++++
 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               |  32 +++
 6 files changed, 514 insertions(+), 1 deletion(-)
 create mode 100644 src/ipa/ipu3/algorithms/af.cpp
 create mode 100644 src/ipa/ipu3/algorithms/af.h

Hi Jean-Michel,

== The second part  of the mail :)

On Fri, Jan 21, 2022 at 5:52 PM Jean-Michel Hautbois
<jeanmichel.hautbois@ideasonboard.com> wrote:
>
> Sorry, this is the second part of my review, bad key stroke :-(
>
> On 21/01/2022 10:26, Jean-Michel Hautbois wrote:
> > Hi Kate,
> >
> > Thanks for the patch !
> >
> > On 20/01/2022 12:41, 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.
> >>
> >> The variance of each focus step is determined and a greedy
> >> approah is used to find the maximum variance of the AF
> >> state and a appropriate focus value.
> > s/approah/approach
> >
> >>
> >> 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).
> >
> > We should investigate this, as it should not be a blocker ?
> >
> >>
> >> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> >> ---
> >>   src/ipa/ipu3/algorithms/af.cpp      | 367 ++++++++++++++++++++++++++++
> >>   src/ipa/ipu3/algorithms/af.h        |  79 ++++++
> >>   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               |  32 +++
> >>   6 files changed, 514 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..26c98868
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/algorithms/af.cpp
> >> @@ -0,0 +1,367 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Red Hat
> >> + *
> >> + * af.cpp - IPU3 auto focus control
> >> + */
> >> +
> >> +#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
> >> + */
> >> +
> >> +/**
> >> + * \def AF_MIN_GRID_WIDTH
> >> + * \brief the minimum width of AF grid.
> >> + * The minimum grid horizontal dimensions, in number of grid
> >> blocks(cells).
> >> +*/
> >
> > This is true for this one and all the defines, please replace those by
> > static constexpr instead ?
> >
> >> +
> >> +/**
> >> + * \def AF_MIN_GRID_HEIGHT
> >> + * \brief the minimum height of AF grid.
> >> + * The minimum grid horizontal dimensions, in number of grid
> >> blocks(cells).
> >> +*/
> >> +
> >> +/**
> >> + * \def AF_MIN_BLOCK_WIDTH
> >> + * \brief the minimum block size of the width.
> >> + */
> >> +
> >> +/**
> >> + * \def AF_MAX_GRID_HEIGHT
> >> + * \brief the minimum height of AF grid.
> >> + * The minimum grid horizontal dimensions, in number of grid
> >> blocks(cells).
> >> +*/
> >> +
> >> +/**
> >> + * \def AF_MAX_BLOCK_WIDTH
> >> + * \brief the minimum block size of the width.
> >> + */
> >> +
> >> +/**
> >> + * \def AF_MAX_BLOCK_WIDTH
> >> + * \brief the maximum block size of the width.
> >> + */
> >> +
> >> +/**
> >> + * \def AF_MIN_BLOCK_HEIGHT
> >> + * \brief the minimum block size of the height.
> >> + */
> >> +
> >> +/**
> >> + * \def AF_MAX_BLOCK_HEIGHT
> >> + * \brief the maximum block size of the height.
> >> + */
> >> +
> >> +/**
> >> + * \def 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 A IPU3 auto-focus accelerator based auto focus algorthim
> >
> > 'An auto-focus algorithm based on IPU3 statistics' maybe ?
> > This proposal should be taken with a grain of salt, as I am not a native
> > speaker ;-).
> >
> >> + *
> >> + * This algorithm is used to determine the position of the lens and
> >> get a
> >> + * focused image. The IPU3 AF accelerator computes the statistics,
> >> composed
> > s/accelerator/processing block/
> >
> >> + * 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
> >> + * 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 value of the VCM control
> >> + * \todo should be obtained from the VCM driver
> >> + */
> >> +static constexpr uint32_t MaxFocusSteps_ = 1023;
> >
> > Remove the trailing underscore (same below):
> > s/MaxFocusSteps_/maxFocusSteps/
> >
> >> +
> >> +/* minimum focus step for searching appropriate focus */
> >> +static constexpr uint32_t coarseSearchStep_ = 10;
> >> +static constexpr uint32_t fineSearchStep_ = 1;
> >
> > Correct me if I am wrong:
> > - the coarse step is used to determine a first approximation of the
> > focused image
> > - The fine step is then used to find the optimal lens position
> >
> > The VCM used on SGo2 has 1024 steps, so, you may search for ~100 frames
> > (ie ~3 seconds at 30fps) before going into the fine search step ?
> >
> >> +
> >> +/* max ratio of variance change, 0.0 < MaxChange_ < 1.0 */
> >> +static constexpr double MaxChange_ = 0.2;
> >> +
> >> +/* the numbers of frame to be ignored, before performing focus scan. */
> >> +static constexpr uint32_t ignoreFrame_ = 10;
> >> +
> >> +/* fine scan range 0 < findRange_ < 1 */
> >> +static constexpr double findRange_ = 0.05;
> >> +
> >> +/* settings for Auto Focus from the kernel */
> > Those filters are not the ones defined in the kernel ;-).
> >
> >> +static struct ipu3_uapi_af_filter_config afFilterConfigDefault = {
> >> +    { 0, 1, 3, 7 },
> >> +    { 11, 13, 1, 2 },
> >> +    { 8, 19, 34, 242 },
> >> +    0x7fdffbfe,
> >> +    { 0, 1, 6, 6 },
> >> +    { 13, 25, 3, 0 },
> >> +    { 25, 3, 177, 254 },
> >> +    0x4e53ca72,
> >> +    .y_calc = { 8, 8, 8, 8 },
> >> +    .nf = { 0, 9, 0, 9, 0 },
> >> +};
> >
> > Use the fields names for all the structure:
> > static struct ipu3_uapi_af_filter_config afFilterConfigDefault = {
> >      .y1_coeff_0 = { 0, 1, 3, 7 },
> >      .y1_coeff_1 = { 11, 13, 1, 2 },
> > etc.
> > };
> >
> >> +
> >> +Af::Af()
> >> +    : focus_(0), goodFocus_(0), currentVariance_(0.0),
> >> previousVariance_(0.0),
> >> +      coarseComplete_(false), fineComplete_(false)
> >> +{
> >> +    maxStep_ = MaxFocusSteps_;
> >> +}
> >> +
> >> +Af::~Af()
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \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 acc */
> > s/acc/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)
> >
> > Are you using the utils/hooks/ in your git repo ?
> > If so, I think you should have a misaligned warning here ?
> >
> >> +{
> >> +    /* 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;
> >> +    /* AF buffer length */
> >> +    afRawBufferLen_ = context.configuration.af.afGrid.width *
> >> +              context.configuration.af.afGrid.height;
> >
> > AFAIK, this is only used in the process() function, maybe could it be a
> > local variable ?
> >
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief AF coarse scan
> >> + * \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
> >> + *
> >> + * Finetune the lens position with moving 1 step for each variance
> >> computation.
> >> + *
> >> + * \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
> >> + *
> >> + * \return True, if it finds a AF value.
> >> + */
> >> +bool Af::afScan(IPAContext &context, int min_step)
> >> +{
> >> +    /* find the maximum variance during the AF scan using a greedy
> >> strategy */
>
> I am interested here: why greedy :-) ?
>
> >> +    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 gradient */
>
> You are mixing gradient and variance terms. You are checking the
> direction of the variance derivate, right ?
>
> >> +        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_;
> >> +    return false;
> >> +}
> >> +
> >> +/**
> >> + * \brief Determine the max contrast image and lens position. y_table
> >> is the
> >> + * statictic data from IPU3 and is composed of low pass and high pass
> >> filtered
> s/statictic/statistics
>
> >> + * 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;
> >> +    uint32_t z = 0;
> >> +
> >> +    y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;
>
> static_cast<> ?

OK, I'll try to use static_cast.

>
> >> +
> >> +    /**
> >> +     * Calculate the mean and the varience AF statistics, since IPU3
> >> only determine the AF value
> s/varience/variance
>
> >> +     * for a given grid.
> >> +     * For coarse: low pass results are used.
> >> +     * For fine: high pass 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));
> >> +        }
> >> +    } 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 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;
> >> +        LOG(IPU3Af, Debug) << "Rate of variance change: "
> >> +                   << var_ratio
> >> +                   << " current focus step: "
> >> +                   << context.frameContext.af.focus;
> >> +        /**
> >> +         * If the change ratio of contrast is over Maxchange_ (out of
> >> focus),
> >> +         * trigger AF again.
> >> +         */
> >> +        if (var_ratio > MaxChange_) {
> >> +            if (ignoreCounter_ == 0) {
> >> +                afReset(context);
> >> +            } else
> >> +                ignoreCounter_--;
> >> +        } else
> >> +            ignoreCounter_ = ignoreFrame_;
> >> +    } else {
> >> +        if (ignoreCounter_ != 0)
> >> +            ignoreCounter_--;
> >> +        else {
> >> +            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..5654a964
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/algorithms/af.h
> >> @@ -0,0 +1,79 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Red Hat
> >> + *
> >> + * af.h - IPU3 Af control
> >> + */
> >> +#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"
> >> +
> >> +/* Definitions from repo of chromium */
> >> +#define AF_MIN_GRID_WIDTH 16
> >> +#define AF_MIN_GRID_HEIGHT 16
> >> +#define AF_MAX_GRID_WIDTH 32
> >> +#define AF_MAX_GRID_HEIGHT 24
> >> +#define AF_MIN_BLOCK_WIDTH 4
> >> +#define AF_MIN_BLOCK_HEIGHT 3
> >> +#define AF_MAX_BLOCK_WIDTH 6
> >> +#define AF_MAX_BLOCK_HEIGHT 6
> >> +#define AF_DEFAULT_HEIGHT_PER_SLICE 2
>
> Those would better be static constexpr ?

OK

>
> >> +
> >> +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..e643d38f 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -31,6 +31,10 @@ struct IPASessionConfiguration {
> >>           double minAnalogueGain;
> >>           double maxAnalogueGain;
> >>       } agc;
> >> +
> >> +    struct {
> >> +        ipu3_uapi_grid_config afGrid;
> >> +    } af;
>
> Alphabetical order (af should be before agc).
>
> >>   };
> >>   struct IPAFrameContext {
> >> @@ -49,6 +53,12 @@ struct IPAFrameContext {
> >>           double temperatureK;
> >>       } awb;
>
> Add a blank line.
>
> >> +    struct {
> >> +        uint32_t focus;
> >> +        double maxVariance;
> >> +        bool stable;
> >> +    } af;
>
> Alphabetical order.
>
> >> +
> >>       struct {
> >>           uint32_t exposure;
> >>           double gain;
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index 3d307708..b5149bcd 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>());H
> >>       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,33 @@ 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 bsd output size
> s/bsd/Bayer Down Scaler/
>
> >> + */
> >> +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;
> >> +    /* 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));
> >> +    /* make sure x_start is multiplied by 10 and y_start is
> >> multiplied by 2 */
> >> +    grid.x_start = (grid.x_start / 10) * 10;
> >> +    grid.y_start = (grid.y_start / 2) * 2;
>
> I am not sure to understand why you are doing this ?
> We nned to be sure x_start is at least 10 pixels from the left of the
> frame, and y_start needs to be 2 pixels from the top of the frame. I am
> not sure we need to have them as multiples of 10 (and 2) ?

Here is the chrome OS mentioned.
https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/af_public.h#84

unsigned short x_start; /**< default value 10, X top left corner of
the grid x_start is even */ (should be even)
unsigned short y_start; /**< default value 2, Y top left corner of the
grid y_start is even */ (should be even)

I actually set both x_start and y_start to odd numbers and the AF
could not work properly.
I would double-check that behaviour and try to make sure both of them
are even numbers.

Also, I'll keep x_start and y_start are multiplied by 2 in the next v6 patch.

>
> >> +    grid.y_start = grid.y_start | IPU3_UAPI_GRID_Y_START_EN;
> >> +}
> >> +
> >>   /**
> >>    * \brief Configure the IPU3 IPA
> >>    * \param[in] configInfo The IPA configuration data, received from
> >> the pipeline
> >> @@ -461,6 +491,8 @@ int IPAIPU3::configure(const IPAConfigInfo
> >> &configInfo,
> >>       lineDuration_ = sensorInfo_.lineLength * 1.0s /
> >> sensorInfo_.pixelRate;
> >> +    initAfGrid(configInfo.bdsOutputSize);
> >> +
> >>       /* Update the camera controls using the new sensor settings. */
> >>       updateControls(sensorInfo_, ctrls_, ipaControls);
>