[{"id":22126,"web_url":"https://patchwork.libcamera.org/comment/22126/","msgid":"","date":"2022-02-07T11:35:25","subject":"Re: [libcamera-devel] [RFC v6] ipa: ipu3: af: Auto focus for dw9719\n\tSurface Go2 VCM","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kate,\n\nThanks for the patch !\n\nOn 07/02/2022 08:16, Kate Hsuan wrote:\n> Since VCM for surface Go 2 (dw9719) had been successfully\n> driven, this Af module can be used to control the VCM and\n> determine the focus value based on the IPU3 AF state.\n> \n> Based on the values from the IPU3 AF buffer, the variance\n> of each focus step is determined and a greedy approach is\n> used to find the maximum variance of the AF state and an\n> appropriate focus value.\n> \n> The grid configuration is implemented as a context. Also,\n> the grid parameter- AF_MIN_BLOCK_WIDTH is set to 4 (default\n> is 3) since if the default value is used, x_start\n> (x_start > 640) will be at an incorrect location of the\n> image (rightmost of the sensor).\n> \n> Signed-off-by: Kate Hsuan \n\nIt works quite well, on top of master and latest's series from Daniel [1].\n\nI am having an issue, I suppose you have the same, with AGC. It is \nblinking on my SGo2 ov8865 device... ?\nI took two captures, with the same conditions, on master [2] and with \nyour patch (with a local patch to apply the TCC configuration, which \nexplains the really better colors) [3].\n\nThe feeling I have is it is quite fast to focus, which is good !\n\n[1]: https://patchwork.libcamera.org/project/libcamera/list/?series=2906\n[2]: https://snipboard.io/oOexGl.jpg\n[3]: https://snipboard.io/kazvGC.jpg\n\n> ---\n> src/ipa/ipu3/algorithms/af.cpp | 375 ++++++++++++++++++++++++++++\n> src/ipa/ipu3/algorithms/af.h | 78 ++++++\n> src/ipa/ipu3/algorithms/meson.build | 3 +-\n> src/ipa/ipu3/ipa_context.cpp | 24 ++\n> src/ipa/ipu3/ipa_context.h | 10 +\n> src/ipa/ipu3/ipu3.cpp | 36 +++\n> 6 files changed, 525 insertions(+), 1 deletion(-)\n> create mode 100644 src/ipa/ipu3/algorithms/af.cpp\n> create mode 100644 src/ipa/ipu3/algorithms/af.h\n> \n> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> new file mode 100644\n> index 00000000..f67ab8a9\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/af.cpp\n> @@ -0,0 +1,375 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + *\n> + * af.cpp - IPU3 auto focus algorithm\n> + */\n> +\n> +#include \"af.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libipa/histogram.h\"\n> +\n> +/**\n> + * \\file af.h\n> + */\n> +\n> +/**\n> + * \\var AF_MIN_GRID_WIDTH\n> + * \\brief the minimum width of AF grid.\n> + * The minimum grid horizontal dimensions, in number of grid blocks(cells).\n> +*/\n\nMy bad, I said to make it constexpr, but did not ask for naming changes...\ns/AF_MIN_GRID_WIDTH/kAfMinGridWidth/\n\nSame for all those defines ;-).\n\n> +\n> +/**\n> + * \\var AF_MIN_GRID_HEIGHT\n> + * \\brief the minimum height of AF grid.\n> + * The minimum grid vertical dimensions, in number of grid blocks(cells).\n> +*/\n> +\n> +/**\n> + * \\var AF_MAX_GRID_WIDTH\n> + * \\brief the maximum width of AF grid.\n> + * The maximum grid horizontal dimensions, in number of grid blocks(cells).\n> +*/\n> +\n> +/**\n> + * \\var AF_MAX_GRID_HEIGHT\n> + * \\brief the maximum height of AF grid.\n> + * The maximum grid vertical dimensions, in number of grid blocks(cells).\n> +*/\n> +\n> +/**\n> + * \\var AF_MIN_BLOCK_WIDTH\n> + * \\brief the minimum block size of the width.\n> + */\n> +\n> +/**\n> + * \\var AF_MIN_BLOCK_HEIGHT\n> + * \\brief the minimum block size of the height.\n> + */\n> +\n> +/**\n> + * \\def AF_MAX_BLOCK_WIDTH\n> + * \\brief the maximum block size of the width.\n> + */\n> +\n> +/**\n> + * \\var AF_MAX_BLOCK_HEIGHT\n> + * \\brief the maximum block size of the height.\n> + */\n> +\n> +/**\n> + * \\var AF_DEFAULT_HEIGHT_PER_SLICE\n> + * \\brief The default number of blocks in vertical axis per slice.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +using namespace std::literals::chrono_literals;\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +/**\n> + * \\class Af\n> + * \\brief An auto-focus algorithm based on IPU3 statistics\n> + *\n> + * This algorithm is used to determine the position of the lens and get a\n> + * focused image. The IPU3 AF processing block computes the statistics,\n> + * composed by high pass and low pass filtered value and stores in a AF buffer.\n> + * Typically, for a focused image, it has relative high contrast than a\ns/has relative high contrast than/has a relatively higher contrast than/\n> + * blurred image, i.e. an out of focus image. Therefore, if an image with the\n> + * highest contrast can be found from the AF scan, the lens' position is the\n> + * best step of the focus.\n> + *\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(IPU3Af)\n> +\n> +/**\n> + * Maximum focus steps of the VCM control\n> + * \\todo should be obtained from the VCM driver\n> + */\n\nIs something missing to implement it and grab it from context ?\n\n> +static constexpr uint32_t maxFocusSteps = 1023;\n> +\n> +/* minimum focus step for searching appropriate focus */\n> +static constexpr uint32_t coarseSearchStep = 30;\n> +static constexpr uint32_t fineSearchStep = 1;\n\nPlease document each variable ;-).\nAnd fix the namings:\ns/coarseSearchStep/kCoarseSearchStep/\ns/fineSearchStep/kFineSearchStep/\n\nSame below.\n\n> +\n> +/* max ratio of variance change, 0.0 < maxChange < 1.0 */\n> +static constexpr double maxChange = 0.5;\n> +\n> +/* the numbers of frame to be ignored, before performing focus scan. */\ns/the numbers of frame/the number of frames/\n> +static constexpr uint32_t ignoreFrame = 10;\n> +\n> +/* fine scan range 0 < findRange < 1 */\n> +static constexpr double findRange = 0.05;\n> +\n> +/* settings for IPU3 AF filter */\n> +static struct ipu3_uapi_af_filter_config afFilterConfigDefault = {\n> +\t.y1_coeff_0 = { 0, 1, 3, 7 },\n> +\t.y1_coeff_1 = { 11, 13, 1, 2 },\n> +\t.y1_coeff_2 = { 8, 19, 34, 242 },\n> +\t.y1_sign_vec = 0x7fdffbfe,\n> +\t.y2_coeff_0 = { 0, 1, 6, 6 },\n> +\t.y2_coeff_1 = { 13, 25, 3, 0 },\n> +\t.y2_coeff_2 = { 25, 3, 177, 254 },\n> +\t.y2_sign_vec = 0x4e53ca72,\n> +\t.y_calc = { 8, 8, 8, 8 },\n> +\t.nf = { 0, 9, 0, 9, 0 },\n> +};\n> +\n> +Af::Af()\n> +\t: focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),\n> +\t coarseComplete_(false), fineComplete_(false)\n\nWhat would you think of:\ns/coarseComplete_/coarseCompleted_/\ns/fineComplete_/fineCompleted_/\n\n> +{\n> +\tmaxStep_ = maxFocusSteps;\n\nOnce the maximum will be grabbed from the configuration context, this \nvariable will not be needed anymore.\n\n> +}\n> +\n> +Af::~Af()\n> +{\n> +}\n\nIt could be a ~Af() = default; in af.h ?\n\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Af::prepare(IPAContext &context, ipu3_uapi_params *params)\n> +{\n> +\tconst struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid;\n> +\tparams->acc_param.af.grid_cfg = grid;\n> +\tparams->acc_param.af.filter_config = afFilterConfigDefault;\n> +\n> +\t/* enable AF processing block */\n> +\tparams->use.acc_af = 1;\n> +}\n> +\n> +/**\n> + * \\brief Configure the Af given a configInfo\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] configInfo The IPA configuration data\n> + *\n> + * \\return 0\n> + */\n> +int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)\n> +{\n> +\t/* determined focus value i.e. current focus value */\n> +\tcontext.frameContext.af.focus = 0;\n> +\t/* maximum variance of the AF statistics */\n> +\tcontext.frameContext.af.maxVariance = 0;\n> +\t/* the stable AF value flag. if it is true, the AF should be in a stable state. */\n> +\tcontext.frameContext.af.stable = false;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief AF coarse scan\n> + *\n> + * Find a near focused image using a coarse step. The step is determined by coarseSearchStep.\n> + *\n> + * \\param[in] context The shared IPA context\n> + *\n> + */\n> +void Af::afCoarseScan(IPAContext &context)\n> +{\n> +\tif (coarseComplete_ == true)\n> +\t\treturn;\n> +\n> +\tif (afScan(context, coarseSearchStep)) {\n> +\t\tcoarseComplete_ = true;\n> +\t\tcontext.frameContext.af.maxVariance = 0;\n> +\t\tfocus_ = context.frameContext.af.focus - (context.frameContext.af.focus * findRange);\n> +\t\tcontext.frameContext.af.focus = focus_;\n> +\t\tpreviousVariance_ = 0;\n> +\t\tmaxStep_ = std::clamp(static_cast(focus_ + (focus_ * findRange)), 0U, maxFocusSteps);\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief AF fine scan\n> + *\n> + * Find an optimum lens position with moving 1 step for each search.\n> + *\n> + * \\param[in] context The shared IPA context\n> + *\n> + */\n> +void Af::afFineScan(IPAContext &context)\n> +{\n> +\tif (coarseComplete_ != true)\n> +\t\treturn;\n> +\n> +\tif (afScan(context, fineSearchStep)) {\n> +\t\tcontext.frameContext.af.stable = true;\n> +\t\tfineComplete_ = true;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief AF reset\n> + *\n> + * Reset all the parameter to start over the AF process.\n> + *\n> + * \\param[in] context The shared IPA context\n> + *\n> + */\n> +void Af::afReset(IPAContext &context)\n> +{\n> +\tcontext.frameContext.af.maxVariance = 0;\n> +\tcontext.frameContext.af.focus = 0;\n> +\tfocus_ = 0;\n> +\tcontext.frameContext.af.stable = false;\n> +\tignoreCounter_ = ignoreFrame;\n> +\tpreviousVariance_ = 0.0;\n> +\tcoarseComplete_ = false;\n> +\tfineComplete_ = false;\n> +\tmaxStep_ = maxFocusSteps;\n> +}\n> +\n> +/**\n> + * \\brief AF scan\n> + * \\param[in] context The shared IPA context\n> + *\n> + * This fuction compares the previous and current variance. It always pick the largest variance to\n> + * replace the previous one. If it finds the decending variance values, it returns immediately.\n> + *\n> + * \\return True, if it finds a AF value.\n> + */\n> +bool Af::afScan(IPAContext &context, int min_step)\n> +{\n> +\t/* find the maximum variance during the AF scan through\n> +\t always picking the maximum variance */\n\nMultiple lines comments need to be in the form:\n/*\n * Find the maximum variance during the AF scan through always picking\n * the maximum variance.\n */\n\n> +\tif (currentVariance_ > context.frameContext.af.maxVariance) {\n> +\t\tcontext.frameContext.af.maxVariance = currentVariance_;\n> +\t\tgoodFocus_ = focus_;\n> +\t}\n> +\n> +\tif (focus_ > maxStep_) {\n> +\t\t/* if reach the max step, move lens to the position and set \"focus stable\". */\n> +\t\tcontext.frameContext.af.focus = goodFocus_;\n> +\t\treturn true;\n> +\t} else {\n> +\t\t/* check negative direction of the variance development. */\n\nCould you maybe elaborate a bit ?\nSomething lik (please reword ;-)):\n/*\n * Find the maximum of the variance by estimating its\n * derivative. If the direction changes, it means we have\n * passed a maximum one step before.\n */\n\n> +\t\tif ((currentVariance_ - context.frameContext.af.maxVariance) > -(context.frameContext.af.maxVariance * 0.15)) {\n> +\t\t\tfocus_ += min_step;\n> +\t\t\tcontext.frameContext.af.focus = focus_;\n> +\t\t} else {\n> +\t\t\tcontext.frameContext.af.focus = goodFocus_;\n> +\t\t\tpreviousVariance_ = currentVariance_;\n> +\t\t\treturn true;\n> +\t\t}\n> +\t}\n> +\tLOG(IPU3Af, Debug) << \"Variance previous: \"\n> +\t\t\t << previousVariance_\n> +\t\t\t << \" current: \"\n> +\t\t\t << currentVariance_\n> +\t\t\t << \" Diff: \"\n> +\t\t\t << (currentVariance_ - context.frameContext.af.maxVariance);\n> +\tpreviousVariance_ = currentVariance_;\n> +\tLOG(IPU3Af, Debug) << \"Focus searching max variance is: \"\n> +\t\t\t << context.frameContext.af.maxVariance\n> +\t\t\t << \" Focus step is \"\n> +\t\t\t << goodFocus_\n> +\t\t\t << \" Current scan is \"\n> +\t\t\t << focus_;\n\nDo we need those logs at each step ?\n\n> +\treturn false;\n> +}\n> +\n> +/**\n> + * \\brief Determine the max contrast image and lens position. y_table is the\n> + * statistic data from IPU3 and is composed of low pass and high pass filtered\n> + * value. High pass filtered value also represents the sharpness of the image.\n> + * Based on this, if the image with highest variance of the high pass filtered\n> + * value (contrast) during the AF scan, the position of the lens should be the\n> + * best focus.\n> + * \\param[in] context The shared IPA context.\n> + * \\param[in] stats The statistic buffer of 3A from the IPU3.\n> + */\n> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> +{\n> +\tuint32_t total = 0;\n> +\tdouble mean;\n> +\tuint64_t var_sum = 0;\n> +\ty_table_item_t y_item[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE / sizeof(y_table_item_t)];\n> +\tuint32_t z = 0;\n> +\n> +\tmemcpy(y_item, stats->af_raw_buffer.y_table, IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);\n> +\n> +\t/* Evaluate the AF buffer length */\n> +\tafRawBufferLen_ = context.configuration.af.afGrid.width *\n> +\t\t\t context.configuration.af.afGrid.height;\n\nYou could memcpy only afRawBufferLen_ elements ?\nBTW, this is a local used variable... I would expect it to be set in \nconfigure() if you want it as a class member ?\n\nAny reason why you copy the elements ?\n\n> +\n> +\t/**\n\n/* only (not a doxygen comment).\n\n> +\t * Calculate the mean and the variance AF statistics, since IPU3 only determine the AF value\n> +\t * for a given grid.\n\nsplit line.\n\n> +\t * For coarse: y1 are used.\n> +\t * For fine: y2 results are used.\n> +\t */\n> +\tif (coarseComplete_) {\n> +\t\tfor (z = 0; z < afRawBufferLen_; z++) {\n> +\t\t\ttotal = total + y_item[z].y2_avg;\n> +\t\t}\n> +\t\tmean = total / afRawBufferLen_;\n> +\n> +\t\tfor (z = 0; z < afRawBufferLen_; z++) {\n> +\t\t\tvar_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));\n\nsplit line.\n\n> +\t\t}\n> +\t} else {\n> +\t\tfor (z = 0; z < afRawBufferLen_; z++) {\n> +\t\t\ttotal = total + y_item[z].y1_avg;\n> +\t\t}\n> +\t\tmean = total / afRawBufferLen_;\n> +\n> +\t\tfor (z = 0; z < afRawBufferLen_; z++) {\n> +\t\t\tvar_sum = var_sum + ((y_item[z].y1_avg - mean) * (y_item[z].y1_avg - mean));\n\nsplit line.\n\n> +\t\t}\n> +\t}\n> +\n> +\t/* Determine the average variance of the frame. */\n> +\tcurrentVariance_ = static_cast(var_sum) / static_cast(afRawBufferLen_);\n> +\tLOG(IPU3Af, Debug) << \"variance: \" << currentVariance_;\n> +\n> +\tif (context.frameContext.af.stable == true) {\n> +\t\tconst uint32_t diff_var = std::abs(currentVariance_ - context.frameContext.af.maxVariance); > +\t\tconst double var_ratio = diff_var / \ncontext.frameContext.af.maxVariance;\n\nsplit lines.\n\n> +\t\tLOG(IPU3Af, Debug) << \"Rate of variance change: \"\n> +\t\t\t\t << var_ratio\n> +\t\t\t\t << \" current focus step: \"\n> +\t\t\t\t << context.frameContext.af.focus;\n> +\t\t/**\n> +\t\t * If the change ratio of contrast is more than maxChange (out of focus),\n> +\t\t * trigger AF again.\n\n/* only (not a doxygen comment) and the first line is too long (cut at 80).\n\n> +\t\t */\n> +\t\tif (var_ratio > maxChange) {\n> +\t\t\tif (ignoreCounter_ == 0) {\n> +\t\t\t\tafReset(context);\n> +\t\t\t} else\n> +\t\t\t\tignoreCounter_--;\n> +\t\t} else\n> +\t\t\tignoreCounter_ = ignoreFrame;\n> +\t} else {\n> +\t\tif (ignoreCounter_ != 0)\n> +\t\t\tignoreCounter_--;\n> +\t\telse {\n> +\t\t\tafCoarseScan(context);\n> +\t\t\tafFineScan(context);\n> +\t\t}\n> +\t}\n\nI think this could be simplified a bit ?\nWe always need to know if we need to ignore the frame or not. If so, we \ncan return early. If not, we want to know if we are stable or not.\n\n> +}\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h\n> new file mode 100644\n> index 00000000..981bf3a2\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/af.h\n> @@ -0,0 +1,78 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + *\n> + * af.h - IPU3 Af algorithm\n> + */\n> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AF_H__\n> +#define __LIBCAMERA_IPU3_ALGORITHMS_AF_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"algorithm.h\"\n> +\n> +/* static variables from repo of chromium */\n> +static constexpr uint8_t AF_MIN_GRID_WIDTH = 16;\n> +static constexpr uint8_t AF_MIN_GRID_HEIGHT = 16;\n> +static constexpr uint8_t AF_MAX_GRID_WIDTH = 32;\n> +static constexpr uint8_t AF_MAX_GRID_HEIGHT = 24;\n> +static constexpr uint16_t AF_MIN_BLOCK_WIDTH = 4;\n> +static constexpr uint16_t AF_MIN_BLOCK_HEIGHT = 3;\n> +static constexpr uint16_t AF_MAX_BLOCK_WIDTH = 6;\n> +static constexpr uint16_t AF_MAX_BLOCK_HEIGHT = 6;\n> +static constexpr uint16_t AF_DEFAULT_HEIGHT_PER_SLICE = 2;\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +class Af : public Algorithm\n> +{\n> +\t/* The format of y_table. From ipu3-ipa repo */\n> +\ttypedef struct __attribute__((packed)) y_table_item {\n> +\t\tuint16_t y1_avg;\n> +\t\tuint16_t y2_avg;\n> +\t} y_table_item_t;\n> +public:\n> +\tAf();\n> +\t~Af();\n> +\n> +\tvoid prepare(IPAContext &context, ipu3_uapi_params *params) override;\n> +\tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> +\n> +private:\n> +\tvoid afCoarseScan(IPAContext &context);\n> +\tvoid afFineScan(IPAContext &context);\n> +\tbool afScan(IPAContext &context, int min_step);\n> +\tvoid afReset(IPAContext &context);\n> +\n> +\t/* Used for focus scan. */\n> +\tuint32_t focus_;\n> +\t/* Focus good */\n> +\tuint32_t goodFocus_;\n> +\t/* Recent AF statistic variance. */\n> +\tdouble currentVariance_;\n> +\t/* The frames to be ignore before starting measuring. */\n> +\tuint32_t ignoreCounter_;\n> +\t/* previous variance. it is used to determine the gradient */\n> +\tdouble previousVariance_;\n> +\t/* Max scan steps of each pass of AF scaning */\n> +\tuint32_t maxStep_;\n> +\t/* coarse scan stable. Complete low pass search (coarse) scan) */\n> +\tbool coarseComplete_;\n> +\t/* fine scan stable. Complete high pass scan (fine scan) */\n> +\tbool fineComplete_;\n> +\t/* Raw buffer length */\n> +\tuint32_t afRawBufferLen_;\n> +};\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AF_H__ */\n> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> index 4db6ae1d..e1099169 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -1,8 +1,9 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> ipu3_ipa_algorithms = files([\n> + 'af.cpp',\n> 'agc.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> - 'tone_mapping.cpp',\n> + 'tone_mapping.cpp'\n> ])\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 86794ac1..ecb2b90a 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -67,6 +67,30 @@ namespace libcamera::ipa::ipu3 {\n> *\n> * \\var IPASessionConfiguration::grid.stride\n> * \\brief Number of cells on one line including the ImgU padding\n> + *\n> + */\n> +\n> +/**\n> + * \\var IPASessionConfiguration::af\n> + * \\brief AF grid configuration of the IPA\n> + *\n> + * \\var IPASessionConfiguration::af.afGrid\n> + *\n> + */\n> +\n> +/**\n> + * \\var IPAFrameContext::af\n> + * \\brief Context for the Automatic Focus algorithm\n> + *\n> + * \\struct IPAFrameContext::af\n> + * \\var IPAFrameContext::af.focus\n> + * \\brief Current position of the lens\n> + *\n> + * \\var IPAFrameContext::af.maxVariance\n> + * \\brief The maximum variance of the current image.\n> + *\n> + * \\var IPAFrameContext::af.stable\n> + * \\brief is the image focused?\n> */\n> \n> /**\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index c6dc0814..60ad3194 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -25,6 +25,10 @@ struct IPASessionConfiguration {\n> \t\tuint32_t stride;\n> \t} grid;\n> \n> +\tstruct {\n> +\t\tipu3_uapi_grid_config afGrid;\n> +\t} af;\n> +\n> \tstruct {\n> \t\tutils::Duration minShutterSpeed;\n> \t\tutils::Duration maxShutterSpeed;\n> @@ -34,6 +38,12 @@ struct IPASessionConfiguration {\n> };\n> \n> struct IPAFrameContext {\n> +\tstruct {\n> +\t\tuint32_t focus;\n> +\t\tdouble maxVariance;\n> +\t\tbool stable;\n> +\t} af;\n> +\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 3d307708..2d23f8f8 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -30,6 +30,7 @@\n> \n> #include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> +#include \"algorithms/af.h\"\n> #include \"algorithms/agc.h\"\n> #include \"algorithms/algorithm.h\"\n> #include \"algorithms/awb.h\"\n> @@ -157,6 +158,7 @@ private:\n> \n> \tvoid setControls(unsigned int frame);\n> \tvoid calculateBdsGrid(const Size &bdsOutputSize);\n> +\tvoid initAfGrid(const Size &bdsOutputSize);\n> \n> \tstd::map buffers_;\n> \n> @@ -294,6 +296,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> \t}\n> \n> \t/* Construct our Algorithms */\n> +\talgorithms_.push_back(std::make_unique());\n> \talgorithms_.push_back(std::make_unique());\n> \talgorithms_.push_back(std::make_unique());\n> \talgorithms_.push_back(std::make_unique());\n> @@ -395,6 +398,37 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \t\t\t << (int)bdsGrid.height << \" << \" << (int)bdsGrid.block_height_log2 << \")\";\n> }\n> \n> +/**\n> + * \\brief Configure the IPU3 AF grid\n> + *\n> + * This function gives the default values for the AF grid configuration.\n> + * All the parameters are set to the minimum acceptable values.\n> + *\n> + * \\param bdsOutputSize The BDS output size\n> + */\n> +void IPAIPU3::initAfGrid(const Size &bdsOutputSize)\n> +{\n> +\tstruct ipu3_uapi_grid_config &grid = context_.configuration.af.afGrid;\n> +\tgrid.width = AF_MIN_GRID_WIDTH;\n> +\tgrid.height = AF_MIN_GRID_HEIGHT;\n> +\tgrid.block_width_log2 = AF_MIN_BLOCK_WIDTH;\n> +\tgrid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;\n> +\tgrid.height_per_slice = AF_DEFAULT_HEIGHT_PER_SLICE;\n> +\tgrid.block_width_log2 = AF_MIN_BLOCK_WIDTH;\n> +\tgrid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;\n> +\n> +\t/* x_start and y start are default to BDS center */\n> +\tgrid.x_start = (bdsOutputSize.width / 2) -\n> +\t\t (((grid.width << grid.block_width_log2) / 2));\n> +\tgrid.y_start = (bdsOutputSize.height / 2) -\n> +\t\t (((grid.height << grid.block_height_log2) / 2));\n> +\n> +\t/* x_start and y_start should be even */\n> +\tgrid.x_start = (grid.x_start / 2) * 2;\n> +\tgrid.y_start = (grid.y_start / 2) * 2;\n> +\tgrid.y_start = grid.y_start | IPU3_UAPI_GRID_Y_START_EN;\n> +}\n\nWhy are you not doing it in the IPU3Af::configure() call ?\nIt would let you remove the [[maybe_unused]] for the IPAConfigInfo \nvariable, as you would use it to get the bdsOutputSize ?\n\n> +\n> /**\n> * \\brief Configure the IPU3 IPA\n> * \\param[in] configInfo The IPA configuration data, received from the pipeline\n> @@ -461,6 +495,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \n> \tlineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\n> \n> +\tinitAfGrid(configInfo.bdsOutputSize);\n> +\n\nAnd this would also be removed ?\n\n> \t/* Update the camera controls using the new sensor settings. */\n> \tupdateControls(sensorInfo_, ctrls_, ipaControls);\n> \n\nI think you could now remove the \"RFC\" tag.\nYou can add my\nTested-by: Jean-Michel Hautbois ","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7BA74BF415\n\tfor ;\n\tMon, 7 Feb 2022 11:35:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC1E861070;\n\tMon, 7 Feb 2022 12:35:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B0A560E58\n\tfor ;\n\tMon, 7 Feb 2022 12:35:29 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:f807:d5e7:1307:1df2] (unknown\n\t[IPv6:2a01:e0a:169:7140:f807:d5e7:1307:1df2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFC00A04;\n\tMon, 7 Feb 2022 12:35:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oWtz/Uq0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644233728;\n\tbh=NqdsNYulMGtpRKiGkOo/Nxy4s94cD2Qxz6PKdc2hWxM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=oWtz/Uq050gmQfC9HElZmI71vVHDWrnLakF9CLsFkhKpC5YZi1th8a4KnTLjEkTk2\n\ttlTWYFot09NGWhSyIvJc6SiZt9URQeKcUqDMtBiWGxmgxaACneQGNIYX7y4TEFJO4B\n\tMzprA+GqFE8uDwx1szonFmziQ56UXO04dZAhAL6Q=","Message-ID":"","Date":"Mon, 7 Feb 2022 12:35:25 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.5.0","Content-Language":"en-US","To":"Kate Hsuan ,\n\tlibcamera devel ","References":"<20220207071606.18564-1-hpa@redhat.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<20220207071606.18564-1-hpa@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v6] ipa: ipu3: af: Auto focus for dw9719\n\tSurface Go2 VCM","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22135,"web_url":"https://patchwork.libcamera.org/comment/22135/","msgid":"","date":"2022-02-08T05:48:48","subject":"Re: [libcamera-devel] [RFC v6] ipa: ipu3: af: Auto focus for dw9719\n\tSurface Go2 VCM","submitter":{"id":105,"url":"https://patchwork.libcamera.org/api/people/105/","name":"Kate Hsuan","email":"hpa@redhat.com"},"content":"coub Hi Jean-Michel,\n\nThank you for your testing and review.\n\nOn Mon, Feb 7, 2022 at 7:35 PM Jean-Michel Hautbois\n wrote:\n>\n> Hi Kate,\n>\n> Thanks for the patch !\n>\n> On 07/02/2022 08:16, Kate Hsuan wrote:\n> > Since VCM for surface Go 2 (dw9719) had been successfully\n> > driven, this Af module can be used to control the VCM and\n> > determine the focus value based on the IPU3 AF state.\n> >\n> > Based on the values from the IPU3 AF buffer, the variance\n> > of each focus step is determined and a greedy approach is\n> > used to find the maximum variance of the AF state and an\n> > appropriate focus value.\n> >\n> > The grid configuration is implemented as a context. Also,\n> > the grid parameter- AF_MIN_BLOCK_WIDTH is set to 4 (default\n> > is 3) since if the default value is used, x_start\n> > (x_start > 640) will be at an incorrect location of the\n> > image (rightmost of the sensor).\n> >\n> > Signed-off-by: Kate Hsuan \n>\n> It works quite well, on top of master and latest's series from Daniel [1].\n\nI also tried it too, Thanks, Daniel and folks!\n\n>\n> I am having an issue, I suppose you have the same, with AGC. It is\n> blinking on my SGo2 ov8865 device... ?\n\nMe too, I maintained my environment to not too bright and not too dark\nto prevent from blinking.\nDoes it seem an issue with the sensor's driver or AGC? I'm not very sure.\n\n> I took two captures, with the same conditions, on master [2] and with\n> your patch (with a local patch to apply the TCC configuration, which\n> explains the really better colors) [3].\n>\n> The feeling I have is it is quite fast to focus, which is good !\n>\n> [1]: https://patchwork.libcamera.org/project/libcamera/list/?series=2906\n> [2]: https://snipboard.io/oOexGl.jpg\n> [3]: https://snipboard.io/kazvGC.jpg\n>\n\nNice piano :). We still could improve it. In some low contrast\nbackgrounds, it was difficult to determine a focus value.\n\n> > ---\n> > src/ipa/ipu3/algorithms/af.cpp | 375 ++++++++++++++++++++++++++++\n> > src/ipa/ipu3/algorithms/af.h | 78 ++++++\n> > src/ipa/ipu3/algorithms/meson.build | 3 +-\n> > src/ipa/ipu3/ipa_context.cpp | 24 ++\n> > src/ipa/ipu3/ipa_context.h | 10 +\n> > src/ipa/ipu3/ipu3.cpp | 36 +++\n> > 6 files changed, 525 insertions(+), 1 deletion(-)\n> > create mode 100644 src/ipa/ipu3/algorithms/af.cpp\n> > create mode 100644 src/ipa/ipu3/algorithms/af.h\n> >\n> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> > new file mode 100644\n> > index 00000000..f67ab8a9\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > @@ -0,0 +1,375 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.cpp - IPU3 auto focus algorithm\n> > + */\n> > +\n> > +#include \"af.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libipa/histogram.h\"\n> > +\n> > +/**\n> > + * \\file af.h\n> > + */\n> > +\n> > +/**\n> > + * \\var AF_MIN_GRID_WIDTH\n> > + * \\brief the minimum width of AF grid.\n> > + * The minimum grid horizontal dimensions, in number of grid blocks(cells).\n> > +*/\n>\n> My bad, I said to make it constexpr, but did not ask for naming changes...\n> s/AF_MIN_GRID_WIDTH/kAfMinGridWidth/\n>\n> Same for all those defines ;-).\nok! no problem.\n\n>\n> > +\n> > +/**\n> > + * \\var AF_MIN_GRID_HEIGHT\n> > + * \\brief the minimum height of AF grid.\n> > + * The minimum grid vertical dimensions, in number of grid blocks(cells).\n> > +*/\n> > +\n> > +/**\n> > + * \\var AF_MAX_GRID_WIDTH\n> > + * \\brief the maximum width of AF grid.\n> > + * The maximum grid horizontal dimensions, in number of grid blocks(cells).\n> > +*/\n> > +\n> > +/**\n> > + * \\var AF_MAX_GRID_HEIGHT\n> > + * \\brief the maximum height of AF grid.\n> > + * The maximum grid vertical dimensions, in number of grid blocks(cells).\n> > +*/\n> > +\n> > +/**\n> > + * \\var AF_MIN_BLOCK_WIDTH\n> > + * \\brief the minimum block size of the width.\n> > + */\n> > +\n> > +/**\n> > + * \\var AF_MIN_BLOCK_HEIGHT\n> > + * \\brief the minimum block size of the height.\n> > + */\n> > +\n> > +/**\n> > + * \\def AF_MAX_BLOCK_WIDTH\n> > + * \\brief the maximum block size of the width.\n> > + */\n> > +\n> > +/**\n> > + * \\var AF_MAX_BLOCK_HEIGHT\n> > + * \\brief the maximum block size of the height.\n> > + */\n> > +\n> > +/**\n> > + * \\var AF_DEFAULT_HEIGHT_PER_SLICE\n> > + * \\brief The default number of blocks in vertical axis per slice.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +/**\n> > + * \\class Af\n> > + * \\brief An auto-focus algorithm based on IPU3 statistics\n> > + *\n> > + * This algorithm is used to determine the position of the lens and get a\n> > + * focused image. The IPU3 AF processing block computes the statistics,\n> > + * composed by high pass and low pass filtered value and stores in a AF buffer.\n> > + * Typically, for a focused image, it has relative high contrast than a\n> s/has relative high contrast than/has a relatively higher contrast than/\n> > + * blurred image, i.e. an out of focus image. Therefore, if an image with the\n> > + * highest contrast can be found from the AF scan, the lens' position is the\n> > + * best step of the focus.\n> > + *\n\nops, I would revise this part. my fault.\n\n\n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(IPU3Af)\n> > +\n> > +/**\n> > + * Maximum focus steps of the VCM control\n> > + * \\todo should be obtained from the VCM driver\n> > + */\n>\n> Is something missing to implement it and grab it from context ?\n\nThe maximum steps of dw9719 VCM is 1023. The max steps should be\nobtained from the VCM driver.\nSo, a kind of method should be implemented to fetch the VCM capability\nfrom the driver.\n\n>\n> > +static constexpr uint32_t maxFocusSteps = 1023;\n> > +\n> > +/* minimum focus step for searching appropriate focus */\n> > +static constexpr uint32_t coarseSearchStep = 30;\n> > +static constexpr uint32_t fineSearchStep = 1;\n>\n> Please document each variable ;-).\n> And fix the namings:\n> s/coarseSearchStep/kCoarseSearchStep/\n> s/fineSearchStep/kFineSearchStep/\n>\n> Same below.\n>\n> > +\n> > +/* max ratio of variance change, 0.0 < maxChange < 1.0 */\n> > +static constexpr double maxChange = 0.5;\n> > +\n> > +/* the numbers of frame to be ignored, before performing focus scan. */\n> s/the numbers of frame/the number of frames/\n> > +static constexpr uint32_t ignoreFrame = 10;\n> > +\n> > +/* fine scan range 0 < findRange < 1 */\n> > +static constexpr double findRange = 0.05;\n> > +\n> > +/* settings for IPU3 AF filter */\n> > +static struct ipu3_uapi_af_filter_config afFilterConfigDefault = {\n> > + .y1_coeff_0 = { 0, 1, 3, 7 },\n> > + .y1_coeff_1 = { 11, 13, 1, 2 },\n> > + .y1_coeff_2 = { 8, 19, 34, 242 },\n> > + .y1_sign_vec = 0x7fdffbfe,\n> > + .y2_coeff_0 = { 0, 1, 6, 6 },\n> > + .y2_coeff_1 = { 13, 25, 3, 0 },\n> > + .y2_coeff_2 = { 25, 3, 177, 254 },\n> > + .y2_sign_vec = 0x4e53ca72,\n> > + .y_calc = { 8, 8, 8, 8 },\n> > + .nf = { 0, 9, 0, 9, 0 },\n> > +};\n> > +\n> > +Af::Af()\n> > + : focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),\n> > + coarseComplete_(false), fineComplete_(false)\n>\n> What would you think of:\n> s/coarseComplete_/coarseCompleted_/\n> s/fineComplete_/fineCompleted_/\n>\n> > +{\n> > + maxStep_ = maxFocusSteps;\n>\n> Once the maximum will be grabbed from the configuration context, this\n> variable will not be needed anymore.\n>\n> > +}\n> > +\n> > +Af::~Af()\n> > +{\n> > +}\n>\n> It could be a ~Af() = default; in af.h ?\n>\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > +void Af::prepare(IPAContext &context, ipu3_uapi_params *params)\n> > +{\n> > + const struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid;\n> > + params->acc_param.af.grid_cfg = grid;\n> > + params->acc_param.af.filter_config = afFilterConfigDefault;\n> > +\n> > + /* enable AF processing block */\n> > + params->use.acc_af = 1;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Configure the Af given a configInfo\n> > + * \\param[in] context The shared IPA context\n> > + * \\param[in] configInfo The IPA configuration data\n> > + *\n> > + * \\return 0\n> > + */\n> > +int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)\n> > +{\n> > + /* determined focus value i.e. current focus value */\n> > + context.frameContext.af.focus = 0;\n> > + /* maximum variance of the AF statistics */\n> > + context.frameContext.af.maxVariance = 0;\n> > + /* the stable AF value flag. if it is true, the AF should be in a stable state. */\n> > + context.frameContext.af.stable = false;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief AF coarse scan\n> > + *\n> > + * Find a near focused image using a coarse step. The step is determined by coarseSearchStep.\n> > + *\n> > + * \\param[in] context The shared IPA context\n> > + *\n> > + */\n> > +void Af::afCoarseScan(IPAContext &context)\n> > +{\n> > + if (coarseComplete_ == true)\n> > + return;\n> > +\n> > + if (afScan(context, coarseSearchStep)) {\n> > + coarseComplete_ = true;\n> > + context.frameContext.af.maxVariance = 0;\n> > + focus_ = context.frameContext.af.focus - (context.frameContext.af.focus * findRange);\n> > + context.frameContext.af.focus = focus_;\n> > + previousVariance_ = 0;\n> > + maxStep_ = std::clamp(static_cast(focus_ + (focus_ * findRange)), 0U, maxFocusSteps);\n> > + }\n> > +}\n> > +\n> > +/**\n> > + * \\brief AF fine scan\n> > + *\n> > + * Find an optimum lens position with moving 1 step for each search.\n> > + *\n> > + * \\param[in] context The shared IPA context\n> > + *\n> > + */\n> > +void Af::afFineScan(IPAContext &context)\n> > +{\n> > + if (coarseComplete_ != true)\n> > + return;\n> > +\n> > + if (afScan(context, fineSearchStep)) {\n> > + context.frameContext.af.stable = true;\n> > + fineComplete_ = true;\n> > + }\n> > +}\n> > +\n> > +/**\n> > + * \\brief AF reset\n> > + *\n> > + * Reset all the parameter to start over the AF process.\n> > + *\n> > + * \\param[in] context The shared IPA context\n> > + *\n> > + */\n> > +void Af::afReset(IPAContext &context)\n> > +{\n> > + context.frameContext.af.maxVariance = 0;\n> > + context.frameContext.af.focus = 0;\n> > + focus_ = 0;\n> > + context.frameContext.af.stable = false;\n> > + ignoreCounter_ = ignoreFrame;\n> > + previousVariance_ = 0.0;\n> > + coarseComplete_ = false;\n> > + fineComplete_ = false;\n> > + maxStep_ = maxFocusSteps;\n> > +}\n> > +\n> > +/**\n> > + * \\brief AF scan\n> > + * \\param[in] context The shared IPA context\n> > + *\n> > + * This fuction compares the previous and current variance. It always pick the largest variance to\n> > + * replace the previous one. If it finds the decending variance values, it returns immediately.\n> > + *\n> > + * \\return True, if it finds a AF value.\n> > + */\n> > +bool Af::afScan(IPAContext &context, int min_step)\n> > +{\n> > + /* find the maximum variance during the AF scan through\n> > + always picking the maximum variance */\n>\n> Multiple lines comments need to be in the form:\n> /*\n> * Find the maximum variance during the AF scan through always picking\n> * the maximum variance.\n> */\n>\nOk thanks.\n\n> > + if (currentVariance_ > context.frameContext.af.maxVariance) {\n> > + context.frameContext.af.maxVariance = currentVariance_;\n> > + goodFocus_ = focus_;\n> > + }\n> > +\n> > + if (focus_ > maxStep_) {\n> > + /* if reach the max step, move lens to the position and set \"focus stable\". */\n> > + context.frameContext.af.focus = goodFocus_;\n> > + return true;\n> > + } else {\n> > + /* check negative direction of the variance development. */\n>\n> Could you maybe elaborate a bit ?\n> Something lik (please reword ;-)):\n> /*\n> * Find the maximum of the variance by estimating its\n> * derivative. If the direction changes, it means we have\n> * passed a maximum one step before.\n> */\n>\n\nThank for revising this. I would improve my comments :).\n\n> > + if ((currentVariance_ - context.frameContext.af.maxVariance) > -(context.frameContext.af.maxVariance * 0.15)) {\n> > + focus_ += min_step;\n> > + context.frameContext.af.focus = focus_;\n> > + } else {\n> > + context.frameContext.af.focus = goodFocus_;\n> > + previousVariance_ = currentVariance_;\n> > + return true;\n> > + }\n> > + }\n> > + LOG(IPU3Af, Debug) << \"Variance previous: \"\n> > + << previousVariance_\n> > + << \" current: \"\n> > + << currentVariance_\n> > + << \" Diff: \"\n> > + << (currentVariance_ - context.frameContext.af.maxVariance);\n> > + previousVariance_ = currentVariance_;\n> > + LOG(IPU3Af, Debug) << \"Focus searching max variance is: \"\n> > + << context.frameContext.af.maxVariance\n> > + << \" Focus step is \"\n> > + << goodFocus_\n> > + << \" Current scan is \"\n> > + << focus_;\n>\n> Do we need those logs at each step ?\n\nIt reveals the variance, current step, and focused step. It could be removed.\n\n>\n> > +}\n> > +\n> > +/**\n> > + * \\brief Determine the max contrast image and lens position. y_table is the\n> > + * statistic data from IPU3 and is composed of low pass and high pass filtered\n> > + * value. High pass filtered value also represents the sharpness of the image.\n> > + * Based on this, if the image with highest variance of the high pass filtered\n> > + * value (contrast) during the AF scan, the position of the lens should be the\n> > + * best focus.\n> > + * \\param[in] context The shared IPA context.\n> > + * \\param[in] stats The statistic buffer of 3A from the IPU3.\n> > + */\n> > +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> > +{\n> > + uint32_t total = 0;\n> > + double mean;\n> > + uint64_t var_sum = 0;\n> > + y_table_item_t y_item[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE / sizeof(y_table_item_t)];\n> > + uint32_t z = 0;\n> > +\n> > + memcpy(y_item, stats->af_raw_buffer.y_table, IPU3_UAPI_AF_Y_TABLE_MAX_SIZE);\n> > +\n> > + /* Evaluate the AF buffer length */\n> > + afRawBufferLen_ = context.configuration.af.afGrid.width *\n> > + context.configuration.af.afGrid.height;\n>\n> You could memcpy only afRawBufferLen_ elements ?\n> BTW, this is a local used variable... I would expect it to be set in\n> configure() if you want it as a class member ?\nSure, I could make afRawBufferLen_ to be a class member and be set in\nconfigure().\n\n>\n> Any reason why you copy the elements ?\n\nDatacasting uses static_cast or reinterpret_cast. I could not make\nthem possible :(\n\n>\n> > +\n> > + /**\n>\n> /* only (not a doxygen comment).\n>\n> > + * Calculate the mean and the variance AF statistics, since IPU3 only determine the AF value\n> > + * for a given grid.\n>\n> split line.\n>\n> > + * For coarse: y1 are used.\n> > + * For fine: y2 results are used.\n> > + */\n> > + if (coarseComplete_) {\n> > + for (z = 0; z < afRawBufferLen_; z++) {\n> > + total = total + y_item[z].y2_avg;\n> > + }\n> > + mean = total / afRawBufferLen_;\n> > +\n> > + for (z = 0; z < afRawBufferLen_; z++) {\n> > + var_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));\n>\n> split line.\n>\n> > + }\n> > + } else {\n> > + for (z = 0; z < afRawBufferLen_; z++) {\n> > + total = total + y_item[z].y1_avg;\n> > + }\n> > + mean = total / afRawBufferLen_;\n> > +\n> > + for (z = 0; z < afRawBufferLen_; z++) {\n> > + var_sum = var_sum + ((y_item[z].y1_avg - mean) * (y_item[z].y1_avg - mean));\n>\n> split line.\n>\n> > + }\n> > + }\n> > +\n> > + /* Determine the average variance of the frame. */\n> > + currentVariance_ = static_cast(var_sum) / static_cast(afRawBufferLen_);\n> > + LOG(IPU3Af, Debug) << \"variance: \" << currentVariance_;\n> > +\n> > + if (context.frameContext.af.stable == true) {\n> > + const uint32_t diff_var = std::abs(currentVariance_ - context.frameContext.af.maxVariance); > + const double var_ratio = diff_var /\n> context.frameContext.af.maxVariance;\n>\n> split lines.\n>\n> > + LOG(IPU3Af, Debug) << \"Rate of variance change: \"\n> > + << var_ratio\n> > + << \" current focus step: \"\n> > + << context.frameContext.af.focus;\n> > + /**\n> > + * If the change ratio of contrast is more than maxChange (out of focus),\n> > + * trigger AF again.\n>\n> /* only (not a doxygen comment) and the first line is too long (cut at 80).\n\nmy fault, I'll correct this.\n\n>\n> > + */\n> > + if (var_ratio > maxChange) {\n> > + if (ignoreCounter_ == 0) {\n> > + afReset(context);\n> > + } else\n> > + ignoreCounter_--;\n> > + } else\n> > + ignoreCounter_ = ignoreFrame;\n> > + } else {\n> > + if (ignoreCounter_ != 0)\n> > + ignoreCounter_--;\n> > + else {\n> > + afCoarseScan(context);\n> > + afFineScan(context);\n> > + }\n> > + }\n>\n> I think this could be simplified a bit ?\n> We always need to know if we need to ignore the frame or not. If so, we\n> can return early. If not, we want to know if we are stable or not.\n\nI found that if the lens is reset suddenly, the AF statistics will\nbounce until the AGC become stable.\nSo we may tweak ignoreCounter_ to find an acceptable range to wait for\nAF statistics stable.\nI'll perform some experiments about this and try to simplify this.\n\n\n>\n> > +}\n> > +\n> > +} /* namespace ipa::ipu3::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h\n> > new file mode 100644\n> > index 00000000..981bf3a2\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.h\n> > @@ -0,0 +1,78 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.h - IPU3 Af algorithm\n> > + */\n> > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AF_H__\n> > +#define __LIBCAMERA_IPU3_ALGORITHMS_AF_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +/* static variables from repo of chromium */\n> > +static constexpr uint8_t AF_MIN_GRID_WIDTH = 16;\n> > +static constexpr uint8_t AF_MIN_GRID_HEIGHT = 16;\n> > +static constexpr uint8_t AF_MAX_GRID_WIDTH = 32;\n> > +static constexpr uint8_t AF_MAX_GRID_HEIGHT = 24;\n> > +static constexpr uint16_t AF_MIN_BLOCK_WIDTH = 4;\n> > +static constexpr uint16_t AF_MIN_BLOCK_HEIGHT = 3;\n> > +static constexpr uint16_t AF_MAX_BLOCK_WIDTH = 6;\n> > +static constexpr uint16_t AF_MAX_BLOCK_HEIGHT = 6;\n> > +static constexpr uint16_t AF_DEFAULT_HEIGHT_PER_SLICE = 2;\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +class Af : public Algorithm\n> > +{\n> > + /* The format of y_table. From ipu3-ipa repo */\n> > + typedef struct __attribute__((packed)) y_table_item {\n> > + uint16_t y1_avg;\n> > + uint16_t y2_avg;\n> > + } y_table_item_t;\n> > +public:\n> > + Af();\n> > + ~Af();\n> > +\n> > + void prepare(IPAContext &context, ipu3_uapi_params *params) override;\n> > + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> > + void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> > +\n> > +private:\n> > + void afCoarseScan(IPAContext &context);\n> > + void afFineScan(IPAContext &context);\n> > + bool afScan(IPAContext &context, int min_step);\n> > + void afReset(IPAContext &context);\n> > +\n> > + /* Used for focus scan. */\n> > + uint32_t focus_;\n> > + /* Focus good */\n> > + uint32_t goodFocus_;\n> > + /* Recent AF statistic variance. */\n> > + double currentVariance_;\n> > + /* The frames to be ignore before starting measuring. */\n> > + uint32_t ignoreCounter_;\n> > + /* previous variance. it is used to determine the gradient */\n> > + double previousVariance_;\n> > + /* Max scan steps of each pass of AF scaning */\n> > + uint32_t maxStep_;\n> > + /* coarse scan stable. Complete low pass search (coarse) scan) */\n> > + bool coarseComplete_;\n> > + /* fine scan stable. Complete high pass scan (fine scan) */\n> > + bool fineComplete_;\n> > + /* Raw buffer length */\n> > + uint32_t afRawBufferLen_;\n> > +};\n> > +\n> > +} /* namespace ipa::ipu3::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AF_H__ */\n> > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> > index 4db6ae1d..e1099169 100644\n> > --- a/src/ipa/ipu3/algorithms/meson.build\n> > +++ b/src/ipa/ipu3/algorithms/meson.build\n> > @@ -1,8 +1,9 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > ipu3_ipa_algorithms = files([\n> > + 'af.cpp',\n> > 'agc.cpp',\n> > 'awb.cpp',\n> > 'blc.cpp',\n> > - 'tone_mapping.cpp',\n> > + 'tone_mapping.cpp'\n> > ])\n> > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > index 86794ac1..ecb2b90a 100644\n> > --- a/src/ipa/ipu3/ipa_context.cpp\n> > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > @@ -67,6 +67,30 @@ namespace libcamera::ipa::ipu3 {\n> > *\n> > * \\var IPASessionConfiguration::grid.stride\n> > * \\brief Number of cells on one line including the ImgU padding\n> > + *\n> > + */\n> > +\n> > +/**\n> > + * \\var IPASessionConfiguration::af\n> > + * \\brief AF grid configuration of the IPA\n> > + *\n> > + * \\var IPASessionConfiguration::af.afGrid\n> > + *\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAFrameContext::af\n> > + * \\brief Context for the Automatic Focus algorithm\n> > + *\n> > + * \\struct IPAFrameContext::af\n> > + * \\var IPAFrameContext::af.focus\n> > + * \\brief Current position of the lens\n> > + *\n> > + * \\var IPAFrameContext::af.maxVariance\n> > + * \\brief The maximum variance of the current image.\n> > + *\n> > + * \\var IPAFrameContext::af.stable\n> > + * \\brief is the image focused?\n> > */\n> >\n> > /**\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index c6dc0814..60ad3194 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -25,6 +25,10 @@ struct IPASessionConfiguration {\n> > uint32_t stride;\n> > } grid;\n> >\n> > + struct {\n> > + ipu3_uapi_grid_config afGrid;\n> > + } af;\n> > +\n> > struct {\n> > utils::Duration minShutterSpeed;\n> > utils::Duration maxShutterSpeed;\n> > @@ -34,6 +38,12 @@ struct IPASessionConfiguration {\n> > };\n> >\n> > struct IPAFrameContext {\n> > + struct {\n> > + uint32_t focus;\n> > + double maxVariance;\n> > + bool stable;\n> > + } af;\n> > +\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 3d307708..2d23f8f8 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -30,6 +30,7 @@\n> >\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > +#include \"algorithms/af.h\"\n> > #include \"algorithms/agc.h\"\n> > #include \"algorithms/algorithm.h\"\n> > #include \"algorithms/awb.h\"\n> > @@ -157,6 +158,7 @@ private:\n> >\n> > void setControls(unsigned int frame);\n> > void calculateBdsGrid(const Size &bdsOutputSize);\n> > + void initAfGrid(const Size &bdsOutputSize);\n> >\n> > std::map buffers_;\n> >\n> > @@ -294,6 +296,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> > }\n> >\n> > /* Construct our Algorithms */\n> > + algorithms_.push_back(std::make_unique());\n> > algorithms_.push_back(std::make_unique());\n> > algorithms_.push_back(std::make_unique());\n> > algorithms_.push_back(std::make_unique());\n> > @@ -395,6 +398,37 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> > << (int)bdsGrid.height << \" << \" << (int)bdsGrid.block_height_log2 << \")\";\n> > }\n> >\n> > +/**\n> > + * \\brief Configure the IPU3 AF grid\n> > + *\n> > + * This function gives the default values for the AF grid configuration.\n> > + * All the parameters are set to the minimum acceptable values.\n> > + *\n> > + * \\param bdsOutputSize The BDS output size\n> > + */\n> > +void IPAIPU3::initAfGrid(const Size &bdsOutputSize)\n> > +{\n> > + struct ipu3_uapi_grid_config &grid = context_.configuration.af.afGrid;\n> > + grid.width = AF_MIN_GRID_WIDTH;\n> > + grid.height = AF_MIN_GRID_HEIGHT;\n> > + grid.block_width_log2 = AF_MIN_BLOCK_WIDTH;\n> > + grid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;\n> > + grid.height_per_slice = AF_DEFAULT_HEIGHT_PER_SLICE;\n> > + grid.block_width_log2 = AF_MIN_BLOCK_WIDTH;\n> > + grid.block_height_log2 = AF_MIN_BLOCK_HEIGHT;\n> > +\n> > + /* x_start and y start are default to BDS center */\n> > + grid.x_start = (bdsOutputSize.width / 2) -\n> > + (((grid.width << grid.block_width_log2) / 2));\n> > + grid.y_start = (bdsOutputSize.height / 2) -\n> > + (((grid.height << grid.block_height_log2) / 2));\n> > +\n> > + /* x_start and y_start should be even */\n> > + grid.x_start = (grid.x_start / 2) * 2;\n> > + grid.y_start = (grid.y_start / 2) * 2;\n> > + grid.y_start = grid.y_start | IPU3_UAPI_GRID_Y_START_EN;\n> > +}\n>\n> Why are you not doing it in the IPU3Af::configure() call ?\n> It would let you remove the [[maybe_unused]] for the IPAConfigInfo\n> variable, as you would use it to get the bdsOutputSize ?\n\nThe bdsOutputSize could be found in IPAConfigInfo. If you feel good\nabout this, I'll move them to IPU3Af::configure().\n\n>\n> > +\n> > /**\n> > * \\brief Configure the IPU3 IPA\n> > * \\param[in] configInfo The IPA configuration data, received from the pipeline\n> > @@ -461,6 +495,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >\n> > lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\n> >\n> > + initAfGrid(configInfo.bdsOutputSize);\n> > +\n>\n> And this would also be removed ?\n\nOnce they move to configure(), it can be removed.\n\n>\n> > /* Update the camera controls using the new sensor settings. */\n> > updateControls(sensorInfo_, ctrls_, ipaControls);\n> >\n>\n> I think you could now remove the \"RFC\" tag.\n> You can add my\n> Tested-by: Jean-Michel Hautbois \n>\n\nUnderstand, thanks a lot :)\n\n\n\n--\nBR,\nKate","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B3614BF415\n\tfor ;\n\tTue, 8 Feb 2022 05:49:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1FA4610A9;\n\tTue, 8 Feb 2022 06:49:11 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28C9461097\n\tfor ;\n\tTue, 8 Feb 2022 06:49:09 +0100 (CET)","from mail-lj1-f200.google.com (mail-lj1-f200.google.com\n\t[209.85.208.200]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-204-65Mpf768MY6sjA9q0uPVXg-1; Tue, 08 Feb 2022 00:49:03 -0500","by mail-lj1-f200.google.com with SMTP id\n\ty19-20020a2e9793000000b0023f158d6cc0so5562977lji.10\n\tfor ;\n\tMon, 07 Feb 2022 21:49:03 -0800 (PST)"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"bUr6W/3i\"; dkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=hpa@redhat.com"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1644299347;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=+yauXnklSjCMX9/8xJ7l6kqgdSwMNC+ofi7qTs97xQM=;\n\tb=bUr6W/3i9rCrhmtv4GpeSrd9JYSyti/MBlhVCc4VJZcnOymYV7i+If62eDuncYLPIRAFgy\n\tHB3xX4jPiQ3X6EctoidqaEXHEl2Hj1foqqBHvrCExE313+58Sh6yt3Ms6smRxQoIy0yCTO\n\tB7NKcVg/WRhiZs5b2N63tixgHvs4/ss=","X-MC-Unique":"65Mpf768MY6sjA9q0uPVXg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=+yauXnklSjCMX9/8xJ7l6kqgdSwMNC+ofi7qTs97xQM=;\n\tb=okZdAf5ekmpZvof1kpcolwD5QOIYmtl6TD2kbNtylFMFiCRuUqR1yj9Ug8qxAWCIjw\n\twoK2lAS/gYyCfH6tL09MrbCDOUwaoHk4Mdw/ORFlsXyibLo2UujBUqdcUSUe9o662tH4\n\tMjUlDXt10JY5e2JhO91GiB2+5nlfRItQ2qne75Inf9NIwra0CnuAZ4z88mj8NP8lKWeW\n\tC+BVIcSpFl3Wh0fEROsfNqGNZFGJDiMP+aXJh94Z6smERRrp1awJXZgw9+4SCFZGT06c\n\ttJuAWprXoEOIMj31k/5CPkCvIwXKiwuz4QDJRdS0OxoREs+NH59rInItM2+hl7ngSMF/\n\taAOw==","X-Gm-Message-State":"AOAM531a2CKKoQK2VC3ld95hKl7qg9oHTbm9Nr18YqeRbIi5taC8wu0T\n\tg2hBcbdNjY844M3L2cdJqlwbM4rPAxzksPWmNcTqzazECuTuxtrmRCWfoz9mcBqQc1LUwDNKEu3\n\th9KM3AUYpboRB8J3G1qrVl4yFT+V9DTIU5q5ZXkbEQ+x+wjVMWw==","X-Received":["by 2002:a05:651c:b13:: with SMTP id\n\tb19mr1836222ljr.162.1644299341422; \n\tMon, 07 Feb 2022 21:49:01 -0800 (PST)","by 2002:a05:651c:b13:: with SMTP id\n\tb19mr1836196ljr.162.1644299340684; \n\tMon, 07 Feb 2022 21:49:00 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJympqi95PNjuuuIaPjOCY5NYByTM7Zh0EXEpzxlYh/WGIWqQpF4CsLxbojnuoE9RyifOSQc9i1k2j65yk1O3FM=","MIME-Version":"1.0","References":"<20220207071606.18564-1-hpa@redhat.com>\n\t","In-Reply-To":"","From":"Kate Hsuan ","Date":"Tue, 8 Feb 2022 13:48:48 +0800","Message-ID":"","To":"Jean-Michel Hautbois ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC v6] ipa: ipu3: af: Auto focus for dw9719\n\tSurface Go2 VCM","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]