[{"id":20945,"web_url":"https://patchwork.libcamera.org/comment/20945/","msgid":"","date":"2021-11-15T06:49:38","subject":"Re: [libcamera-devel] [PATCH] 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 15/11/2021 06:44, 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> The variance of each focus step is determined and a greedy\n> approah is used to find the maximum variance of the AF\ns/approah/approach\n\n> state and a appropriate focus value.\n> \n> Signed-off-by: Kate Hsuan \n> ---\n> src/ipa/ipu3/algorithms/af.cpp | 165 ++++++++++++++++++++++++++++\n> src/ipa/ipu3/algorithms/af.h | 48 ++++++++\n> src/ipa/ipu3/algorithms/meson.build | 3 +-\n> src/ipa/ipu3/ipa_context.h | 6 +\n> src/ipa/ipu3/ipu3.cpp | 2 +\n> 5 files changed, 223 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..c276b539\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/af.cpp\n> @@ -0,0 +1,165 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + *\n> + * af.cpp - IPU3 Af control\n> + */\n> +\n> +#include \"af.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libipa/histogram.h\"\n> +\n> +/**\n> + * \\file af.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +using namespace std::literals::chrono_literals;\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(IPU3Af)\n> +\n> +Af::Af(): focus_(0), maxVariance_(0.0), currentVar_(0.0)\n> +{\n> +\t/* For surface Go 2 back camera VCM */\n> +\tvcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n> +}\n\nIPA algorithms should not know anything about V4L2 controls, subdev, \netc. I know some discussion is running about this in \"Fwd: Surface Go \nVCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go \n(version1))​\" on the list, and it is not yet solved afaik.\n\nI would say this should be done through the pipeline handler by the \nCameraSensor class somehow.\nAnd some documentation would then follow to specify it in \nDocumentation/sensor_driver_requirements.rst\n\n> +\n> +Af::~Af()\n> +{\n> +\tif(vcmFd_ != -1)\n> +\t\tclose(vcmFd_);\n> +}\n> +\n> +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> +{\n> +\tconst IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n> +\tcontext.frameContext.af.focus = 0;\n> +\tcontext.frameContext.af.maxVar = 0;\n> +\tcontext.frameContext.af.stable = false;\n> +\n> +\tmaxVariance_ = 0;\n> +\tignoreFrame_ = 100;\n\nThose magic values should be at least commented or set as constexpr.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Af::vcmSet(int value)\n> +{\n> +\tint ret;\n> +\tstruct v4l2_control ctrl;\n> +\tif(vcmFd_ == -1)\n> +\t\treturn;\n> +\tmemset(&ctrl, 0, sizeof(struct v4l2_control));\n> +\tctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n> +\tctrl.value = value;\n> +\tret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);\n\nSame comment, this is not something which should be done in the algorithm.\n\n> +\n> +}\n> +\n> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n> +{\n> +\ttypedef struct y_table_item {\n> +\t\tuint16_t y1_avg;\n> +\t\tuint16_t y2_avg;\n> +\t}y_table_item_t;\n\nNot inside the function. This is the table defined in: \nhttps://www.kernel.org/doc/html/v5.6/media/uapi/v4l/pixfmt-meta-intel-ipu3.html#c.ipu3_uapi_af_raw_buffer\n\nHow do you know it is splitted in y1_avg and y2_avg ? Is it based in CrOs ?\nhttps://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/af_public.h\n\n\n> +\n> +\tuint32_t total = 0;\n> +\tdouble mean;\n> +\tuint64_t var_sum = 0;\n> +\ty_table_item_t *y_item;\n> +\n> +\ty_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n> +\tint z = 0;\n> +\t\n> +\tfor(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n> +\t{\n> +\t\ttotal = total + y_item[z].y2_avg;\n> +\t\tif(y_item[z].y2_avg == 0)\n> +\t\t\tbreak;\n> +\t}\n> +\tmean = total / z;\n\nSome comments to follow the algorithm could help :-). Is the algorithm \ndescribed in some book or something ? Is it a known one ? If so, may you \nreference it ?\n\n> +\n> +\tfor(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 && y_item[z].y2_avg != 0; z++)\n> +\t{\n> +\t\tvar_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));\n> +\t\tif(y_item[z].y2_avg == 0)\n> +\t\t\tbreak;\n> +\t}\n> +\tcurrentVar_ = static_cast(var_sum) /static_cast(z);\n> +\tLOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n> +\n> +\tif( context.frameContext.af.stable == true )\n> +\t{\n> +\t\tconst uint32_t diff_var = std::abs(currentVar_ - context.frameContext.af.maxVar);\n> +\t\tconst double var_ratio = diff_var / context.frameContext.af.maxVar;\n> +\t\tLOG(IPU3Af, Debug) << \"Change ratio: \"\n> +\t\t\t\t << var_ratio\n> +\t\t\t\t << \" current focus: \"\n> +\t\t\t\t << context.frameContext.af.focus;\n> +\t\tif(var_ratio > 0.8)\n> +\t\t{\n> +\t\t\tif(ignoreFrame_ == 0)\n> +\t\t\t{\n> +\t\t\t\tcontext.frameContext.af.maxVar = 0;\n> +\t\t\t\tcontext.frameContext.af.focus = 0;\n> +\t\t\t\tfocus_ = 0;\n> +\t\t\t\tcontext.frameContext.af.stable = false;\n> +\t\t\t\tignoreFrame_ = 60;\n> +\t\t\t}\n> +\t\t\telse\n> +\t\t\t\tignoreFrame_--;\n> +\t\t}else\n> +\t\t\tignoreFrame_ = 60;\n> +\t}else\n> +\t{\n> +\t\tif(ignoreFrame_ != 0)\n> +\t\t\tignoreFrame_--;\n> +\t\telse{\n> +\t\t\tif(currentVar_ > context.frameContext.af.maxVar)\n> +\t\t\t{\n> +\t\t\t\tcontext.frameContext.af.maxVar = currentVar_;\n> +\t\t\t\tcontext.frameContext.af.focus = focus_;\n> +\t\t\t}\n> +\n> +\t\t\tif(focus_ > 1023)\n1023, because... ?\n\n> +\t\t\t{\n> +\t\t\t\tcontext.frameContext.af.stable = true;\n> +\t\t\t\tvcmSet(context.frameContext.af.focus);\n> +\t\t\t\tLOG(IPU3Af, Debug) << \"Finall Focus \"\ns/Finall/Final\n\n> +\t\t\t\t\t\t << context.frameContext.af.focus;\n> +\t\t\t}else\n> +\t\t\t{\n> +\t\t\t\tfocus_ += 5;\n> +\t\t\t\tvcmSet(focus_);\n> +\t\t\t}\n> +\t\t\tLOG(IPU3Af, Debug) << \"Focus searching max var is: \"\n> +\t\t\t\t\t << context.frameContext.af.maxVar\n> +\t\t\t\t\t << \" Focus step is \"\n> +\t\t\t\t\t << context.frameContext.af.focus;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> +\n> +} /* namespace libcamera */\n> \\ No newline at end of file\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..64c704b1\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/af.h\n> @@ -0,0 +1,48 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + *\n> + * af.h - IPU3 Af control\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> +namespace libcamera {\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +class Af : public Algorithm\n> +{\n> +public:\n> +\tAf();\n> +\t~Af();\n> +\n> +\tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n\nIsn't there a need to configure the AF parameters in a prepare() \nfunction ? There is a lot of tables associated, and at the very least, I \nwould have expected a grid:\n\nhttps://www.kernel.org/doc/html/v5.6/media/uapi/v4l/pixfmt-meta-intel-ipu3.html#c.ipu3_uapi_af_config_s\n\n> +\n> +private:\n> +\tvoid filterVariance(double new_var);\n> +\n> +\tvoid vcmSet(int value);\n> +\n> +\tint vcmFd_;\n> +\tint focus_;\n> +\tunsigned int ignoreFrame_;\n> +\tdouble maxVariance_;\n> +\tdouble currentVar_;\n> +\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 3ec42f72..d32c61d2 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n> 'algorithm.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> - 'tone_mapping.cpp',\n> + 'af.cpp',\n\nAlphabetical sorted\n\n> + 'tone_mapping.cpp'\n> ])\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 1e46c61a..5d92f63c 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -47,6 +47,12 @@ struct IPAFrameContext {\n> \t\t} gains;\n> \t} awb;\n> \n> +\tstruct {\n> +\t\tuint32_t focus;\n> +\t\tdouble maxVar;\n> +\t\tbool stable;\n> +\t} af;\n\nThose fields are not documented in ipa_context.cpp, you probably have a \nDoxygen warning ? If not, I suggest you activate the documentation \ngeneration ;-).\n\n> +\n> \tstruct {\n> \t\tdouble gamma;\n> \t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 5c51607d..980815ee 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -31,6 +31,7 @@\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> #include \"algorithms/agc.h\"\n> +#include \"algorithms/af.h\"\n> #include \"algorithms/algorithm.h\"\n> #include \"algorithms/awb.h\"\n> #include \"algorithms/blc.h\"\n> @@ -298,6 +299,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>","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 49BDFBDB1C\n\tfor ;\n\tMon, 15 Nov 2021 06:49:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D01860120;\n\tMon, 15 Nov 2021 07:49:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DA0260120\n\tfor ;\n\tMon, 15 Nov 2021 07:49:41 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:de6c:30de:5224:d992] (unknown\n\t[IPv6:2a01:e0a:169:7140:de6c:30de:5224:d992])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F23159CA;\n\tMon, 15 Nov 2021 07:49:40 +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=\"iv/mRvqd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636958981;\n\tbh=/s4IFYDgDRqXD4laZaNl0saOTEp0Nuy8eXuMYTWeAm4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=iv/mRvqd6aFFPSVdSoS/DbLDWBn4vtykKI2nDmVsq/ZgRoU58K0tVhO4P79OUs1P4\n\tb3JuEa2K9cOCyPkSbnf7ItnOtwHz2SGH8wr89ijgvf0dpW6c2ZDtdYJ4JGgRRsvBTm\n\tRQ9/CnkR4MyzVsjdcK35CMGvWS0ANFTaQDCD6HJU=","Message-ID":"","Date":"Mon, 15 Nov 2021 07:49:38 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Kate Hsuan ,\n\tlibcamera devel ","References":"<20211115054400.17797-1-hpa@redhat.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<20211115054400.17797-1-hpa@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] 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":20947,"web_url":"https://patchwork.libcamera.org/comment/20947/","msgid":"","date":"2021-11-15T08:39:12","subject":"Re: [libcamera-devel] [PATCH] 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":"Hi Jean-Michel,\n\nOn Mon, Nov 15, 2021 at 2:49 PM Jean-Michel Hautbois <\njeanmichel.hautbois@ideasonboard.com> wrote:\n>\n> Hi Kate,\n>\n> Thanks for the patch !\n>\n> On 15/11/2021 06:44, 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> > The variance of each focus step is determined and a greedy\n> > approah is used to find the maximum variance of the AF\n> s/approah/approach\n\nI'll correct my typo. T_T\n\n>\n> > state and a appropriate focus value.\n> >\n> > Signed-off-by: Kate Hsuan \n> > ---\n> > src/ipa/ipu3/algorithms/af.cpp | 165 ++++++++++++++++++++++++++++\n> > src/ipa/ipu3/algorithms/af.h | 48 ++++++++\n> > src/ipa/ipu3/algorithms/meson.build | 3 +-\n> > src/ipa/ipu3/ipa_context.h | 6 +\n> > src/ipa/ipu3/ipu3.cpp | 2 +\n> > 5 files changed, 223 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\nb/src/ipa/ipu3/algorithms/af.cpp\n> > new file mode 100644\n> > index 00000000..c276b539\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > @@ -0,0 +1,165 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.cpp - IPU3 Af control\n> > + */\n> > +\n> > +#include \"af.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libipa/histogram.h\"\n> > +\n> > +/**\n> > + * \\file af.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +LOG_DEFINE_CATEGORY(IPU3Af)\n> > +\n> > +Af::Af(): focus_(0), maxVariance_(0.0), currentVar_(0.0)\n> > +{\n> > + /* For surface Go 2 back camera VCM */\n> > + vcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n> > +}\n>\n> IPA algorithms should not know anything about V4L2 controls, subdev,\n> etc. I know some discussion is running about this in \"Fwd: Surface Go\n> VCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go\n> (version1))\" on the list, and it is not yet solved afaik.\n>\n> I would say this should be done through the pipeline handler by the\n> CameraSensor class somehow.\n> And some documentation would then follow to specify it in\n> Documentation/sensor_driver_requirements.rst\n\nThis is my first version of VCM control and to test the Raw AF data from\nIPU3 so I open the device in the class directly.\nAlso, I have traced the codes and found the control class of v4l2-subdev.\nI'll try to update them in my v2 patch.\n\n>\n> > +\n> > +Af::~Af()\n> > +{\n> > + if(vcmFd_ != -1)\n> > + close(vcmFd_);\n> > +}\n> > +\n> > +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> > +{\n> > + const IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n> > + context.frameContext.af.focus = 0;\n> > + context.frameContext.af.maxVar = 0;\n> > + context.frameContext.af.stable = false;\n> > +\n> > + maxVariance_ = 0;\n> > + ignoreFrame_ = 100;\n>\n> Those magic values should be at least commented or set as constexpr.\n\nI'll add the comments to the code.\n\n>\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +void Af::vcmSet(int value)\n> > +{\n> > + int ret;\n> > + struct v4l2_control ctrl;\n> > + if(vcmFd_ == -1)\n> > + return;\n> > + memset(&ctrl, 0, sizeof(struct v4l2_control));\n> > + ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n> > + ctrl.value = value;\n> > + ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);\n>\n> Same comment, this is not something which should be done in the algorithm.\n\nOK, I'll try to do this in my v2 patch.\n\n>\n> > +\n> > +}\n> > +\n> > +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n> > +{\n> > + typedef struct y_table_item {\n> > + uint16_t y1_avg;\n> > + uint16_t y2_avg;\n> > + }y_table_item_t;\n>\n> Not inside the function. This is the table defined in:\n>\nhttps://www.kernel.org/doc/html/v5.6/media/uapi/v4l/pixfmt-meta-intel-ipu3.html#c.ipu3_uapi_af_raw_buffer\n>\n> How do you know it is splitted in y1_avg and y2_avg ? Is it based in CrOs\n?\n>\nhttps://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/af_public.h\n\nI found the y_table format from ipu3-ipa repository (\nhttps://git.libcamera.org/libcamera/ipu3-ipa.git). Also, combined some\nkeywords from the kernel, such as high and low frequency and convolution,\nthe AF raw buffer may contain the result of low and high pass filter\nconvolution (I guess).\nhttps://lxr.missinglinkelectronics.com/linux/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h#L256\n\n\n>\n>\n> > +\n> > + uint32_t total = 0;\n> > + double mean;\n> > + uint64_t var_sum = 0;\n> > + y_table_item_t *y_item;\n> > +\n> > + y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n> > + int z = 0;\n> > +\n> > + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n> > + {\n> > + total = total + y_item[z].y2_avg;\n> > + if(y_item[z].y2_avg == 0)\n> > + break;\n> > + }\n> > + mean = total / z;\n>\n> Some comments to follow the algorithm could help :-). Is the algorithm\n> described in some book or something ? Is it a known one ? If so, may you\n> reference it ?\n\nOh. I just calculate the variance for each non-zero y2_avg (high pass)\nvalue. For a clear image, the variance should be the largest value from\neach focus step. By increasing the focus step and searching for the maximum\nvariance, an appropriate focus value can be found. It is something like the\nmountain climbing algorithm.\n\n\n>\n> > +\n> > + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 &&\ny_item[z].y2_avg != 0; z++)\n> > + {\n> > + var_sum = var_sum + ((y_item[z].y2_avg - mean) *\n(y_item[z].y2_avg - mean));\n> > + if(y_item[z].y2_avg == 0)\n> > + break;\n> > + }\n> > + currentVar_ = static_cast(var_sum)\n/static_cast(z);\n> > + LOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n> > +\n> > + if( context.frameContext.af.stable == true )\n> > + {\n> > + const uint32_t diff_var = std::abs(currentVar_ -\ncontext.frameContext.af.maxVar);\n> > + const double var_ratio = diff_var /\ncontext.frameContext.af.maxVar;\n> > + LOG(IPU3Af, Debug) << \"Change ratio: \"\n> > + << var_ratio\n> > + << \" current focus: \"\n> > + << context.frameContext.af.focus;\n> > + if(var_ratio > 0.8)\n> > + {\n> > + if(ignoreFrame_ == 0)\n> > + {\n> > + context.frameContext.af.maxVar = 0;\n> > + context.frameContext.af.focus = 0;\n> > + focus_ = 0;\n> > + context.frameContext.af.stable = false;\n> > + ignoreFrame_ = 60;\n> > + }\n> > + else\n> > + ignoreFrame_--;\n> > + }else\n> > + ignoreFrame_ = 60;\n> > + }else\n> > + {\n> > + if(ignoreFrame_ != 0)\n> > + ignoreFrame_--;\n> > + else{\n> > + if(currentVar_ > context.frameContext.af.maxVar)\n> > + {\n> > + context.frameContext.af.maxVar =\ncurrentVar_;\n> > + context.frameContext.af.focus = focus_;\n> > + }\n> > +\n> > + if(focus_ > 1023)\n> 1023, because... ?\n\nIt's the maximum focus step of VCM.\n\n>\n> > + {\n> > + context.frameContext.af.stable = true;\n> > + vcmSet(context.frameContext.af.focus);\n> > + LOG(IPU3Af, Debug) << \"Finall Focus \"\n> s/Finall/Final\n\nSorry for my typo 😅\n\n>\n> > + <<\ncontext.frameContext.af.focus;\n> > + }else\n> > + {\n> > + focus_ += 5;\n> > + vcmSet(focus_);\n> > + }\n> > + LOG(IPU3Af, Debug) << \"Focus searching max var\nis: \"\n> > + <<\ncontext.frameContext.af.maxVar\n> > + << \" Focus step is \"\n> > + <<\ncontext.frameContext.af.focus;\n> > + }\n> > + }\n> > +}\n> > +\n> > +\n> > +} /* namespace ipa::ipu3::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > \\ No newline at end of file\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..64c704b1\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.h\n> > @@ -0,0 +1,48 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.h - IPU3 Af control\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> > +namespace libcamera {\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +class Af : public Algorithm\n> > +{\n> > +public:\n> > + Af();\n> > + ~Af();\n> > +\n> > + int configure(IPAContext &context, const IPAConfigInfo\n&configInfo) override;\n> > + void process(IPAContext &context, const ipu3_uapi_stats_3a\n*stats) override;\n>\n> Isn't there a need to configure the AF parameters in a prepare()\n> function ? There is a lot of tables associated, and at the very least, I\n> would have expected a grid:\n>\n>\nhttps://www.kernel.org/doc/html/v5.6/media/uapi/v4l/pixfmt-meta-intel-ipu3.html#c.ipu3_uapi_af_config_s\n\nIn this version, I try to keep all the configurations in default to prove\nthe algorithm and buffer format works.\nI could add grid configuration in my v2 patch.\n\n>\n> > +\n> > +private:\n> > + void filterVariance(double new_var);\n> > +\n> > + void vcmSet(int value);\n> > +\n> > + int vcmFd_;\n> > + int focus_;\n> > + unsigned int ignoreFrame_;\n> > + double maxVariance_;\n> > + double currentVar_;\n> > +\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\nb/src/ipa/ipu3/algorithms/meson.build\n> > index 3ec42f72..d32c61d2 100644\n> > --- a/src/ipa/ipu3/algorithms/meson.build\n> > +++ b/src/ipa/ipu3/algorithms/meson.build\n> > @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n> > 'algorithm.cpp',\n> > 'awb.cpp',\n> > 'blc.cpp',\n> > - 'tone_mapping.cpp',\n> > + 'af.cpp',\n>\n> Alphabetical sorted\n\nI got it.\n\n>\n> > + 'tone_mapping.cpp'\n> > ])\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index 1e46c61a..5d92f63c 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -47,6 +47,12 @@ struct IPAFrameContext {\n> > } gains;\n> > } awb;\n> >\n> > + struct {\n> > + uint32_t focus;\n> > + double maxVar;\n> > + bool stable;\n> > + } af;\n>\n> Those fields are not documented in ipa_context.cpp, you probably have a\n> Doxygen warning ? If not, I suggest you activate the documentation\n> generation ;-).\n\nI don't have Doxygen warning. I guess I don't activate the documentation\ngeneration. I'll activate it, thank you :)\n\n>\n> > +\n> > struct {\n> > double gamma;\n> > struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 5c51607d..980815ee 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -31,6 +31,7 @@\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > #include \"algorithms/agc.h\"\n> > +#include \"algorithms/af.h\"\n> > #include \"algorithms/algorithm.h\"\n> > #include \"algorithms/awb.h\"\n> > #include \"algorithms/blc.h\"\n> > @@ -298,6 +299,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> >\nalgorithms_.push_back(std::make_unique());\n> >\n>","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 74F4DBDB1C\n\tfor ;\n\tMon, 15 Nov 2021 08:40:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98F1B6036B;\n\tMon, 15 Nov 2021 09:40:54 +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 3437A60232\n\tfor ;\n\tMon, 15 Nov 2021 09:40:52 +0100 (CET)","from mail-lf1-f69.google.com (mail-lf1-f69.google.com\n\t[209.85.167.69]) (Using TLS) by relay.mimecast.com with ESMTP id\n\tus-mta-525-JzNqgCkUMhaFAGEBongUDw-1; Mon, 15 Nov 2021 03:39:25 -0500","by mail-lf1-f69.google.com with SMTP id\n\td2-20020a0565123d0200b0040370d0d2fbso6448423lfv.23\n\tfor ;\n\tMon, 15 Nov 2021 00:39:24 -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=\"XuGPTCGy\"; 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=1636965650;\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=bEp5avptcf/kK5mFOpfYmRLvO9/H8jywu3BIPAPRd8Q=;\n\tb=XuGPTCGyaYa9SFSqnH3Qu5XNVJXNOfT9CeuBGLRrSaiQPIrfz962i/td1mfUzttZ8RxKAo\n\tnGTQ1tej1wIwgXjJ0WY+FkMxO8yfVmP4jsw28e5w68hU9Tak45iuikqOZlZS620We3mWl1\n\t5OudBCb3Oz8ZVTjvorLTGlvThOils5A=","X-MC-Unique":"JzNqgCkUMhaFAGEBongUDw-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=bEp5avptcf/kK5mFOpfYmRLvO9/H8jywu3BIPAPRd8Q=;\n\tb=AQd8TC3k8wAvvwifznB/VuzuRMT0zL0ikHn+oJn1+Vl3dlwAtMDD9ZQLHa7ygGftqg\n\tKkcqrREJrNhMuHg9FX2TZKvwUH9vH7wnmegBrn5BT8GYuXHsv2kiYPKBtvBdaObF7si5\n\t6asG7VxQe6+WeS3uhV/5VNpoy4LT8DIzvFN1rvTejtC+OBuHy6BLzD/hVmlMTYkK6poM\n\txJUMq8ijJBY5Sw+fFbSj06adMZKBEY+YVcIEri2uEaciVUrvikdTAA3wzlaOH5Fza7Fr\n\tdnV2LjaU2VStPA9n4Abv8cIx2jue+OeRLaPpVIE/wFiLUxIJGscD8fveFlMfJWCqWwjc\n\tq9GA==","X-Gm-Message-State":"AOAM532c02AVuNi4IJjEGBEUsrtKx3AA2a/l9hh7+RphsT2Z0awb+l7u\n\t5j/vVR9x7BCM30az/dyGCE3Ilz0w7DEs1FfqGVKFrh5KMP4SrBUnZiBy5VkifWiGW/XdGbJOQ50\n\t84eHh+aBtcmJSzs42ad7WFr+hboGrH3t6E/vHQbV42Ntyw3V0Xw==","X-Received":["by 2002:a2e:a588:: with SMTP id m8mr38044008ljp.23.1636965563451;\n\tMon, 15 Nov 2021 00:39:23 -0800 (PST)","by 2002:a2e:a588:: with SMTP id m8mr38043971ljp.23.1636965563012;\n\tMon, 15 Nov 2021 00:39:23 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJzQXJti2VEk7Og6H8Qo/3ALpp/1tyPYi/auqxEmi2mCna3p4TWow9tEDzuHDlWrdqGOREOBX2BKberfwe0i2Z8=","MIME-Version":"1.0","References":"<20211115054400.17797-1-hpa@redhat.com>\n\t","In-Reply-To":"","From":"Kate Hsuan ","Date":"Mon, 15 Nov 2021 16:39:12 +0800","Message-ID":"","To":"Jean-Michel Hautbois ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"multipart/alternative; boundary=\"000000000000e4a92705d0cfbed7\"","Subject":"Re: [libcamera-devel] [PATCH] 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\" "}},{"id":20948,"web_url":"https://patchwork.libcamera.org/comment/20948/","msgid":"<283eec60-0b9d-2213-6487-542bfed628d4@ideasonboard.com>","date":"2021-11-15T10:11:44","subject":"Re: [libcamera-devel] [PATCH] 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":"Kate,\n\nOn 15/11/2021 09:39, Kate Hsuan wrote:\n> Hi Jean-Michel,\n> \n> On Mon, Nov 15, 2021 at 2:49 PM Jean-Michel Hautbois \n> > wrote:\n> >\n> > Hi Kate,\n> >\n> > Thanks for the patch !\n> >\n> > On 15/11/2021 06:44, 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> > > The variance of each focus step is determined and a greedy\n> > > approah is used to find the maximum variance of the AF\n> > s/approah/approach\n> \n> I'll correct my typo. T_T\n> \n> >\n> > > state and a appropriate focus value.\n> > >\n> > > Signed-off-by: Kate Hsuan >\n> > > ---\n> > >   src/ipa/ipu3/algorithms/af.cpp      | 165 \n> ++++++++++++++++++++++++++++\n> > >   src/ipa/ipu3/algorithms/af.h        |  48 ++++++++\n> > >   src/ipa/ipu3/algorithms/meson.build |   3 +-\n> > >   src/ipa/ipu3/ipa_context.h          |   6 +\n> > >   src/ipa/ipu3/ipu3.cpp               |   2 +\n> > >   5 files changed, 223 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 \n> b/src/ipa/ipu3/algorithms/af.cpp\n> > > new file mode 100644\n> > > index 00000000..c276b539\n> > > --- /dev/null\n> > > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > > @@ -0,0 +1,165 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Red Hat\n> > > + *\n> > > + * af.cpp - IPU3 Af control\n> > > + */\n> > > +\n> > > +#include \"af.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libipa/histogram.h\"\n> > > +\n> > > +/**\n> > > + * \\file af.h\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +using namespace std::literals::chrono_literals;\n> > > +\n> > > +namespace ipa::ipu3::algorithms {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(IPU3Af)\n> > > +\n> > > +Af::Af(): focus_(0),  maxVariance_(0.0), currentVar_(0.0)\n> > > +{\n> > > +     /* For surface Go 2 back camera VCM */\n> > > +     vcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n> > > +}\n> >\n> > IPA algorithms should not know anything about V4L2 controls, subdev,\n> > etc. I know some discussion is running about this in \"Fwd: Surface Go\n> > VCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go\n> > (version1))\" on the list, and it is not yet solved afaik.\n> >\n> > I would say this should be done through the pipeline handler by the\n> > CameraSensor class somehow.\n> > And some documentation would then follow to specify it in\n> > Documentation/sensor_driver_requirements.rst\n> \n> This is my first version of VCM control and to test the Raw AF data from \n> IPU3 so I open the device in the class directly.\n> Also, I have traced the codes and found the control class of \n> v4l2-subdev. I'll try to update them in my v2 patch.\n\nMaybe could you be interested by the thread \"ipa: ipu3: Extend ipu3 ipa \ninterface for lens controls\" in its v3, maybe would you like to review \nit, as it sounds like the same beast here :-).\n\n> \n> >\n> > > +\n> > > +Af::~Af()\n> > > +{\n> > > +     if(vcmFd_ != -1)\n> > > +             close(vcmFd_);\n> > > +}\n> > > +\n> > > +int Af::configure(IPAContext &context, const IPAConfigInfo \n> &configInfo)\n> > > +{\n> > > +     const IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n> > > +     context.frameContext.af.focus = 0;\n> > > +     context.frameContext.af.maxVar = 0;\n> > > +     context.frameContext.af.stable = false;\n> > > +\n> > > +     maxVariance_ = 0;\n> > > +     ignoreFrame_ = 100;\n> >\n> > Those magic values should be at least commented or set as constexpr.\n> \n> I'll add the comments to the code.\n> \n> >\n> > > +\n> > > +     return 0;\n> > > +}\n> > > +\n> > > +void Af::vcmSet(int value)\n> > > +{\n> > > +     int ret;\n> > > +     struct v4l2_control ctrl;\n> > > +     if(vcmFd_ == -1)\n> > > +             return;\n> > > +     memset(&ctrl, 0, sizeof(struct v4l2_control));\n> > > + ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n> > > +     ctrl.value = value;\n> > > +     ret = ioctl(vcmFd_,  VIDIOC_S_CTRL, &ctrl);\n> >\n> > Same comment, this is not something which should be done in the \n> algorithm.\n> \n> OK, I'll try to do this in my v2 patch.\n> \n> >\n> > > +\n> > > +}\n> > > +\n> > > +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a \n> *stats )\n> > > +{\n> > > +     typedef struct y_table_item {\n> > > +             uint16_t y1_avg;\n> > > +             uint16_t y2_avg;\n> > > +     }y_table_item_t;\n> >\n> > Not inside the function. This is the table defined in:\n> > \n> https://www.kernel.org/doc/html/v5.6/media/uapi/v4l/pixfmt-meta-intel-ipu3.html#c.ipu3_uapi_af_raw_buffer \n> \n> >\n> > How do you know it is splitted in y1_avg and y2_avg ? Is it based in \n> CrOs ?\n> > \n> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/af_public.h \n> \n> \n> I found the y_table format from ipu3-ipa repository \n> (https://git.libcamera.org/libcamera/ipu3-ipa.git \n> ). Also, combined some \n> keywords from the kernel, such as high and low frequency and \n> convolution, the AF raw buffer may contain the result of low and high \n> pass filter convolution (I guess).\n> https://lxr.missinglinkelectronics.com/linux/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h#L256 \n> \n> \n> \n> >\n> >\n> > > +\n> > > +     uint32_t total = 0;\n> > > +     double mean;\n> > > +     uint64_t var_sum = 0;\n> > > +     y_table_item_t *y_item;\n> > > +\n> > > +     y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n> > > +     int z = 0;\n> > > +\n> > > +     for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n> > > +     {\n> > > +             total = total + y_item[z].y2_avg;\n> > > +             if(y_item[z].y2_avg == 0)\n> > > +                     break;\n> > > +     }\n> > > +     mean = total / z;\n> >\n> > Some comments to follow the algorithm could help :-). Is the algorithm\n> > described in some book or something ? Is it a known one ? If so, may you\n> > reference it ?\n> \n> Oh. I just calculate the variance for each non-zero y2_avg (high pass) \n> value. For a clear image, the variance should be the largest value from \n> each focus step. By increasing the focus step and searching for the \n> maximum variance, an appropriate focus value can be found. It is \n> something like the mountain climbing algorithm.\n\nI mean, not everybody will be an image processing expert as you are, so \nadding a documentation in the code could help following how AF works in \nthis case (looks like something like a contrast detection algorithm).\n\n> \n> \n> >\n> > > +\n> > > +     for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 && \n> y_item[z].y2_avg != 0; z++)\n> > > +     {\n> > > +             var_sum = var_sum + ((y_item[z].y2_avg - mean) * \n> (y_item[z].y2_avg - mean));\n> > > +             if(y_item[z].y2_avg == 0)\n> > > +                     break;\n> > > +     }\n> > > +     currentVar_ = static_cast(var_sum) \n> /static_cast(z);\n> > > +     LOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n> > > +\n> > > +     if( context.frameContext.af.stable == true )\n> > > +     {\n> > > +             const uint32_t diff_var = std::abs(currentVar_ - \n> context.frameContext.af.maxVar);\n> > > +             const double var_ratio =  diff_var / \n> context.frameContext.af.maxVar;\n> > > +             LOG(IPU3Af, Debug) << \"Change ratio: \"\n> > > +                                << var_ratio\n> > > +                                << \" current focus: \"\n> > > +                                << context.frameContext.af.focus;\n> > > +             if(var_ratio > 0.8)\n> > > +             {\n> > > +                     if(ignoreFrame_ == 0)\n> > > +                     {\n> > > +                             context.frameContext.af.maxVar = 0;\n> > > +                             context.frameContext.af.focus = 0;\n> > > +                             focus_ = 0;\n> > > +                             context.frameContext.af.stable = false;\n> > > +                             ignoreFrame_ = 60;\n> > > +                     }\n> > > +                     else\n> > > +                             ignoreFrame_--;\n> > > +             }else\n> > > +                     ignoreFrame_ = 60;\n> > > +     }else\n> > > +     {\n> > > +             if(ignoreFrame_ != 0)\n> > > +                     ignoreFrame_--;\n> > > +             else{\n> > > +                     if(currentVar_ > context.frameContext.af.maxVar)\n> > > +                     {\n> > > +                             context.frameContext.af.maxVar = \n> currentVar_;\n> > > +                             context.frameContext.af.focus = focus_;\n> > > +                     }\n> > > +\n> > > +                     if(focus_ > 1023)\n> > 1023, because... ?\n> \n> It's the maximum focus step of VCM.\n> \n> >\n> > > +                     {\n> > > +                             context.frameContext.af.stable = true;\n> > > +                             vcmSet(context.frameContext.af.focus);\n> > > +                             LOG(IPU3Af, Debug) << \"Finall Focus \"\n> > s/Finall/Final\n> \n> Sorry for my typo 😅\n> \n> >\n> > > +                                                << \n> context.frameContext.af.focus;\n> > > +                     }else\n> > > +                     {\n> > > +                             focus_ += 5;\n> > > +                             vcmSet(focus_);\n> > > +                     }\n> > > +                     LOG(IPU3Af, Debug)  << \"Focus searching max \n> var is: \"\n> > > +                                         << \n> context.frameContext.af.maxVar\n> > > +                                         << \" Focus step is \"\n> > > +                                         << \n> context.frameContext.af.focus;\n> > > +             }\n> > > +     }\n> > > +}\n> > > +\n> > > +\n> > > +} /* namespace ipa::ipu3::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > \\ No newline at end of file\n> > > diff --git a/src/ipa/ipu3/algorithms/af.h \n> b/src/ipa/ipu3/algorithms/af.h\n> > > new file mode 100644\n> > > index 00000000..64c704b1\n> > > --- /dev/null\n> > > +++ b/src/ipa/ipu3/algorithms/af.h\n> > > @@ -0,0 +1,48 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Red Hat\n> > > + *\n> > > + * af.h - IPU3 Af control\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> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::ipu3::algorithms {\n> > > +\n> > > +class Af : public Algorithm\n> > > +{\n> > > +public:\n> > > +     Af();\n> > > +     ~Af();\n> > > +\n> > > +     int configure(IPAContext &context, const IPAConfigInfo \n> &configInfo) override;\n> > > +     void process(IPAContext &context, const ipu3_uapi_stats_3a \n> *stats) override;\n> >\n> > Isn't there a need to configure the AF parameters in a prepare()\n> > function ? There is a lot of tables associated, and at the very least, I\n> > would have expected a grid:\n> >\n> > \n> https://www.kernel.org/doc/html/v5.6/media/uapi/v4l/pixfmt-meta-intel-ipu3.html#c.ipu3_uapi_af_config_s \n> \n> \n> In this version, I try to keep all the configurations in default to \n> prove the algorithm and buffer format works.\n> I could add grid configuration in my v2 patch.\n> \n> >\n> > > +\n> > > +private:\n> > > +     void filterVariance(double new_var);\n> > > +\n> > > +     void vcmSet(int value);\n> > > +\n> > > +     int vcmFd_;\n> > > +     int focus_;\n> > > +     unsigned int ignoreFrame_;\n> > > +     double maxVariance_;\n> > > +     double currentVar_;\n> > > +\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 \n> b/src/ipa/ipu3/algorithms/meson.build\n> > > index 3ec42f72..d32c61d2 100644\n> > > --- a/src/ipa/ipu3/algorithms/meson.build\n> > > +++ b/src/ipa/ipu3/algorithms/meson.build\n> > > @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n> > >       'algorithm.cpp',\n> > >       'awb.cpp',\n> > >       'blc.cpp',\n> > > -    'tone_mapping.cpp',\n> > > +    'af.cpp',\n> >\n> > Alphabetical sorted\n> \n> I got it.\n> \n> >\n> > > +    'tone_mapping.cpp'\n> > >   ])\n> > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > > index 1e46c61a..5d92f63c 100644\n> > > --- a/src/ipa/ipu3/ipa_context.h\n> > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > @@ -47,6 +47,12 @@ struct IPAFrameContext {\n> > >               } gains;\n> > >       } awb;\n> > >\n> > > +     struct {\n> > > +             uint32_t focus;\n> > > +             double maxVar;\n> > > +             bool stable;\n> > > +     } af;\n> >\n> > Those fields are not documented in ipa_context.cpp, you probably have a\n> > Doxygen warning ? If not, I suggest you activate the documentation\n> > generation ;-).\n> \n> I don't have Doxygen warning. I guess I don't activate the documentation \n> generation. I'll activate it, thank you :)\n> \n> >\n> > > +\n> > >       struct {\n> > >               double gamma;\n> > >               struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 5c51607d..980815ee 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -31,6 +31,7 @@\n> > >   #include \"libcamera/internal/mapped_framebuffer.h\"\n> > >\n> > >   #include \"algorithms/agc.h\"\n> > > +#include \"algorithms/af.h\"\n> > >   #include \"algorithms/algorithm.h\"\n> > >   #include \"algorithms/awb.h\"\n> > >   #include \"algorithms/blc.h\"\n> > > @@ -298,6 +299,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> > > \n> algorithms_.push_back(std::make_unique());\n> > >\n> >\n> \n> \n> -- \n> BR,\n> Kate","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 E2D0EBF415\n\tfor ;\n\tMon, 15 Nov 2021 10:11:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E10A6032C;\n\tMon, 15 Nov 2021 11:11:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9619460232\n\tfor ;\n\tMon, 15 Nov 2021 11:11:47 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:de6c:30de:5224:d992] (unknown\n\t[IPv6:2a01:e0a:169:7140:de6c:30de:5224:d992])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38C569CA;\n\tMon, 15 Nov 2021 11:11:47 +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=\"J+9vcn/F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636971107;\n\tbh=2bX2GD2dtBcHpvD0wNzh8GMSVqYC50YeA3w7V9gaQCM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=J+9vcn/FgA0ehIZSFUAzrYm1udeuTfvKR3JqikZU9YG/Tax45Ahf4a4nvfqb+8GET\n\tSktsbEfP2Ep9uSXtWWTPbsZsAEft5RvSRsgWi6ExQeYO57zMvmbf5HsqVJItekyWFC\n\toDPH2KqfVwvDfolesgzgzn5a1cNyWgtUBv65lRTk=","Message-ID":"<283eec60-0b9d-2213-6487-542bfed628d4@ideasonboard.com>","Date":"Mon, 15 Nov 2021 11:11:44 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Kate Hsuan ","References":"<20211115054400.17797-1-hpa@redhat.com>\n\t\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] 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\" "}},{"id":21004,"web_url":"https://patchwork.libcamera.org/comment/21004/","msgid":"<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","date":"2021-11-18T12:31:35","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: af: Auto focus for dw9719\n\tSurface Go2 VCM","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kate,\n\nThank you for you work on this.\n\nWhile I understand this is an MVP (as posted by you in one of the other \nautofocus threads) here are some of my high-level queries and \nunderstanding. The interface to handle autofocus (and focus controls) in \ngeneral is missing from libcamera, so I understand the current \nlimitations as well, to move this forward.\n\nOn 11/15/21 11:14 AM, 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> The variance of each focus step is determined and a greedy\n> approah is used to find the maximum variance of the AF\n> state and a appropriate focus value.\n\n\nCan you please suggest a high level reading of finding the focus on the \nimage? I don't understand the implementation / algorithm for the \nfocus-sweep implemented in process()?\n\n>\n> Signed-off-by: Kate Hsuan \n> ---\n> src/ipa/ipu3/algorithms/af.cpp | 165 ++++++++++++++++++++++++++++\n> src/ipa/ipu3/algorithms/af.h | 48 ++++++++\n> src/ipa/ipu3/algorithms/meson.build | 3 +-\n> src/ipa/ipu3/ipa_context.h | 6 +\n> src/ipa/ipu3/ipu3.cpp | 2 +\n> 5 files changed, 223 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..c276b539\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/af.cpp\n\n\nI won't get into style issues here (as don't seem relevant for now), but \nfor your information we do have a style checker at:\n\n     $libcamera/utils/checkstyle.py \n\n> @@ -0,0 +1,165 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + *\n> + * af.cpp - IPU3 Af control\n\n\nwould be better to expand on the acronym \"IPU3 Auto Focus control\" maybe\n\n> + */\n> +\n> +#include \"af.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libipa/histogram.h\"\n> +\n> +/**\n> + * \\file af.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +using namespace std::literals::chrono_literals;\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(IPU3Af)\n> +\n> +Af::Af(): focus_(0), maxVariance_(0.0), currentVar_(0.0)\n> +{\n> +\t/* For surface Go 2 back camera VCM */\n> +\tvcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n\n\nSo I have nautilus (Samsung Chromebook v2) which as a UVC camera and a \nIPU3 one (with IMX258 sensor). The VCM there is dw9807.\n\nI tried this patch on nautilus by mapping the v4l-subdevX to dw9807 and \nit did work. I can literally hear the VCM move and tries to focus on the \nscene. However, there are a few niggles I have noticed:\n\nThe autofocus seems to lock up quite frequently leaving an un-focused \nstreaming. However if I put up a object close to lens it does tries to \nfocus it again (then locks up again but works sometimes). I am \ndeliberately leaving the details out as it's too broad to say anything \nspecific from nautilus' side.\n\nI am interesting in getting better understanding of the values reported, \nso once I have a overall reading on the topic, I can start debugging the \nprocess().\n\n> +}\n> +\n> +Af::~Af()\n> +{\n> +\tif(vcmFd_ != -1)\n> +\t\tclose(vcmFd_);\n> +}\n> +\n> +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n\n\nI think [[maybe_unused]] on configInfo as a replacement for the below line\n\n> +{\n> +\tconst IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n> +\tcontext.frameContext.af.focus = 0;\n> +\tcontext.frameContext.af.maxVar = 0;\n> +\tcontext.frameContext.af.stable = false;\n> +\n> +\tmaxVariance_ = 0;\n> +\tignoreFrame_ = 100;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Af::vcmSet(int value)\n> +{\n> +\tint ret;\n> +\tstruct v4l2_control ctrl;\n> +\tif(vcmFd_ == -1)\n> +\t\treturn;\n\n\nMaybe we should return -EINVAL?\n\n> +\tmemset(&ctrl, 0, sizeof(struct v4l2_control));\n> +\tctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n> +\tctrl.value = value;\n> +\tret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);\n\n\nand return ret instead? To check if we really succeeded on setting the \ncontrol\n\n> +\n> +}\n> +\n> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n> +{\n> +\ttypedef struct y_table_item {\n> +\t\tuint16_t y1_avg;\n> +\t\tuint16_t y2_avg;\n> +\t}y_table_item_t;\n> +\n> +\tuint32_t total = 0;\n> +\tdouble mean;\n> +\tuint64_t var_sum = 0;\n> +\ty_table_item_t *y_item;\n> +\n> +\ty_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n> +\tint z = 0;\n> +\t\n> +\tfor(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n> +\t{\n> +\t\ttotal = total + y_item[z].y2_avg;\n> +\t\tif(y_item[z].y2_avg == 0)\n> +\t\t\tbreak;\n> +\t}\n> +\tmean = total / z;\n> +\n> +\tfor(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 && y_item[z].y2_avg != 0; z++)\n> +\t{\n> +\t\tvar_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));\n> +\t\tif(y_item[z].y2_avg == 0)\n> +\t\t\tbreak;\n> +\t}\n> +\tcurrentVar_ = static_cast(var_sum) /static_cast(z);\n> +\tLOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n> +\n> +\tif( context.frameContext.af.stable == true )\n> +\t{\n> +\t\tconst uint32_t diff_var = std::abs(currentVar_ - context.frameContext.af.maxVar);\n> +\t\tconst double var_ratio = diff_var / context.frameContext.af.maxVar;\n> +\t\tLOG(IPU3Af, Debug) << \"Change ratio: \"\n> +\t\t\t\t << var_ratio\n> +\t\t\t\t << \" current focus: \"\n> +\t\t\t\t << context.frameContext.af.focus;\n> +\t\tif(var_ratio > 0.8)\n\n\nhmm,  I think we should opt for \"variance_\" instead \"var_\" as var in \ngeneral has variable meaning, nevermind, details...\n\n> +\t\t{\n> +\t\t\tif(ignoreFrame_ == 0)\n> +\t\t\t{\n> +\t\t\t\tcontext.frameContext.af.maxVar = 0;\n> +\t\t\t\tcontext.frameContext.af.focus = 0;\n> +\t\t\t\tfocus_ = 0;\n> +\t\t\t\tcontext.frameContext.af.stable = false;\n> +\t\t\t\tignoreFrame_ = 60;\n> +\t\t\t}\n> +\t\t\telse\n> +\t\t\t\tignoreFrame_--;\n> +\t\t}else\n> +\t\t\tignoreFrame_ = 60;\n> +\t}else\n> +\t{\n> +\t\tif(ignoreFrame_ != 0)\n> +\t\t\tignoreFrame_--;\n> +\t\telse{\n> +\t\t\tif(currentVar_ > context.frameContext.af.maxVar)\n> +\t\t\t{\n> +\t\t\t\tcontext.frameContext.af.maxVar = currentVar_;\n> +\t\t\t\tcontext.frameContext.af.focus = focus_;\n> +\t\t\t}\n> +\n> +\t\t\tif(focus_ > 1023)\n> +\t\t\t{\n> +\t\t\t\tcontext.frameContext.af.stable = true;\n> +\t\t\t\tvcmSet(context.frameContext.af.focus);\n> +\t\t\t\tLOG(IPU3Af, Debug) << \"Finall Focus \"\n> +\t\t\t\t\t\t << context.frameContext.af.focus;\n> +\t\t\t}else\n> +\t\t\t{\n> +\t\t\t\tfocus_ += 5;\n> +\t\t\t\tvcmSet(focus_);\n> +\t\t\t}\n> +\t\t\tLOG(IPU3Af, Debug) << \"Focus searching max var is: \"\n> +\t\t\t\t\t << context.frameContext.af.maxVar\n> +\t\t\t\t\t << \" Focus step is \"\n> +\t\t\t\t\t << context.frameContext.af.focus;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> +\n> +} /* namespace libcamera */\n> \\ No newline at end of file\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..64c704b1\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/af.h\n> @@ -0,0 +1,48 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + *\n> + * af.h - IPU3 Af control\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> +namespace libcamera {\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +class Af : public Algorithm\n> +{\n> +public:\n> +\tAf();\n> +\t~Af();\n> +\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 filterVariance(double new_var);\n> +\n> +\tvoid vcmSet(int value);\n> +\n> +\tint vcmFd_;\n> +\tint focus_;\n> +\tunsigned int ignoreFrame_;\n\n\nWhat's this stand for? Is it ignoring a number of frames?\n\n> +\tdouble maxVariance_;\n> +\tdouble currentVar_;\n> +\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 3ec42f72..d32c61d2 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n> 'algorithm.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> - 'tone_mapping.cpp',\n> + 'af.cpp',\n> + 'tone_mapping.cpp'\n\n\nalphabetical order please, I think checkstyle.py will report this as well\n\n> ])\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 1e46c61a..5d92f63c 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -47,6 +47,12 @@ struct IPAFrameContext {\n> \t\t} gains;\n> \t} awb;\n> \n> +\tstruct {\n> +\t\tuint32_t focus;\n> +\t\tdouble maxVar;\n> +\t\tbool stable;\n> +\t} af;\n> +\n> \tstruct {\n> \t\tdouble gamma;\n> \t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 5c51607d..980815ee 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -31,6 +31,7 @@\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> #include \"algorithms/agc.h\"\n> +#include \"algorithms/af.h\"\n\n\nDitto.\n\n> #include \"algorithms/algorithm.h\"\n> #include \"algorithms/awb.h\"\n> #include \"algorithms/blc.h\"\n> @@ -298,6 +299,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());","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 64E0ABDB1C\n\tfor ;\n\tThu, 18 Nov 2021 12:31:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBBD560371;\n\tThu, 18 Nov 2021 13:31:42 +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 2962460231\n\tfor ;\n\tThu, 18 Nov 2021 13:31:41 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.87])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 128D3E7;\n\tThu, 18 Nov 2021 13:31:39 +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=\"I8OwWQns\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637238700;\n\tbh=cXS2vlU38p06s08Y2cPilarFJDfaIF3UGXbH1x4liHY=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=I8OwWQnsop7YvKlRKtjhJ+jGQTILwJExgHmUubdJvEp0F7WIJwdNAuGImUpasziR6\n\tMTcVnRGLD++gjIfiGCCs9y1Rs0HZnzZeY3wbXaa/femCqji4N6/ZrghhINaYOlnLsm\n\tnoZvmToXfOfpsB0p0PAVfvqFpBvYqQv14KQJ/feY=","To":"Kate Hsuan ,\n\tlibcamera devel ","References":"<20211115054400.17797-1-hpa@redhat.com>","From":"Umang Jain ","Message-ID":"<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","Date":"Thu, 18 Nov 2021 18:01:35 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211115054400.17797-1-hpa@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] 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":21006,"web_url":"https://patchwork.libcamera.org/comment/21006/","msgid":"<3e4d378c-fe8c-22f8-c620-2817767d0426@ideasonboard.com>","date":"2021-11-18T12:42:55","subject":"Re: [libcamera-devel] [PATCH] 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 Umang,\n\nOn 18/11/2021 13:31, Umang Jain wrote:\n> Hi Kate,\n> \n> Thank you for you work on this.\n> \n> While I understand this is an MVP (as posted by you in one of the other \n> autofocus threads) here are some of my high-level queries and \n> understanding. The interface to handle autofocus (and focus controls) in \n> general is missing from libcamera, so I understand the current \n> limitations as well, to move this forward.\n> \n> On 11/15/21 11:14 AM, 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>> The variance of each focus step is determined and a greedy\n>> approah is used to find the maximum variance of the AF\n>> state and a appropriate focus value.\n> \n> \n> Can you please suggest a high level reading of finding the focus on the \n> image? I don't understand the implementation / algorithm for the \n> focus-sweep implemented in process()?\n> \n>>\n>> Signed-off-by: Kate Hsuan \n>> ---\n>>   src/ipa/ipu3/algorithms/af.cpp      | 165 ++++++++++++++++++++++++++++\n>>   src/ipa/ipu3/algorithms/af.h        |  48 ++++++++\n>>   src/ipa/ipu3/algorithms/meson.build |   3 +-\n>>   src/ipa/ipu3/ipa_context.h          |   6 +\n>>   src/ipa/ipu3/ipu3.cpp               |   2 +\n>>   5 files changed, 223 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 \n>> b/src/ipa/ipu3/algorithms/af.cpp\n>> new file mode 100644\n>> index 00000000..c276b539\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/af.cpp\n> \n> \n> I won't get into style issues here (as don't seem relevant for now), but \n> for your information we do have a style checker at:\n> \n>     $libcamera/utils/checkstyle.py \n> \n>> @@ -0,0 +1,165 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Red Hat\n>> + *\n>> + * af.cpp - IPU3 Af control\n> \n> \n> would be better to expand on the acronym \"IPU3 Auto Focus control\" maybe\n> \n>> + */\n>> +\n>> +#include \"af.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \"libipa/histogram.h\"\n>> +\n>> +/**\n>> + * \\file af.h\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +using namespace std::literals::chrono_literals;\n>> +\n>> +namespace ipa::ipu3::algorithms {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPU3Af)\n>> +\n>> +Af::Af(): focus_(0),  maxVariance_(0.0), currentVar_(0.0)\n>> +{\n>> +    /* For surface Go 2 back camera VCM */\n>> +    vcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n> \n> \n> So I have nautilus (Samsung Chromebook v2) which as a UVC camera and a \n> IPU3 one (with IMX258 sensor). The VCM there is dw9807.\n> \n> I tried this patch on nautilus by mapping the v4l-subdevX to dw9807 and \n> it did work. I can literally hear the VCM move and tries to focus on the \n> scene. However, there are a few niggles I have noticed:\n> \n> The autofocus seems to lock up quite frequently leaving an un-focused \n> streaming. However if I put up a object close to lens it does tries to \n> focus it again (then locks up again but works sometimes). I am \n> deliberately leaving the details out as it's too broad to say anything \n> specific from nautilus' side.\n\nI will let Kate answer in depth obviously. From what I know, the AF \nneeds a grid configuration in IPU3, and the default grid is set to \n[16*8,16*8] pixels, starting at x=10 and y=2: \nhttps://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ipu3/ipu3-tables.c#L9587\n\nIt means that if your BDS output size is 1280x720 for instance, you will \nfocus on the 10% left part and 17% top part of your scene. Depending on \nthe scene, you might very well be out-of-focus :-).\n\nThat's why I suggested adding a configure() call to set a grid based on \nthe BDS grid, to make it focus in the center as a default.\n\n> \n> I am interesting in getting better understanding of the values reported, \n> so once I have a overall reading on the topic, I can start debugging the \n> process().\n> \n>> +}\n>> +\n>> +Af::~Af()\n>> +{\n>> +    if(vcmFd_ != -1)\n>> +        close(vcmFd_);\n>> +}\n>> +\n>> +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> \n> \n> I think [[maybe_unused]] on configInfo as a replacement for the below line\n> \n>> +{\n>> +    const IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n>> +    context.frameContext.af.focus = 0;\n>> +    context.frameContext.af.maxVar = 0;\n>> +    context.frameContext.af.stable = false;\n>> +\n>> +    maxVariance_ = 0;\n>> +    ignoreFrame_ = 100;\n>> +\n>> +    return 0;\n>> +}\n>> +\n>> +void Af::vcmSet(int value)\n>> +{\n>> +    int ret;\n>> +    struct v4l2_control ctrl;\n>> +    if(vcmFd_ == -1)\n>> +        return;\n> \n> \n> Maybe we should return -EINVAL?\n> \n>> +    memset(&ctrl, 0, sizeof(struct v4l2_control));\n>> +    ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n>> +    ctrl.value = value;\n>> +    ret = ioctl(vcmFd_,  VIDIOC_S_CTRL, &ctrl);\n> \n> \n> and return ret instead? To check if we really succeeded on setting the \n> control\n> \n>> +\n>> +}\n>> +\n>> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n>> +{\n>> +    typedef struct y_table_item {\n>> +        uint16_t y1_avg;\n>> +        uint16_t y2_avg;\n>> +    }y_table_item_t;\n>> +\n>> +    uint32_t total = 0;\n>> +    double mean;\n>> +    uint64_t var_sum = 0;\n>> +    y_table_item_t *y_item;\n>> +\n>> +    y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n>> +    int z = 0;\n>> +\n>> +    for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n>> +    {\n>> +        total = total + y_item[z].y2_avg;\n>> +        if(y_item[z].y2_avg == 0)\n>> +            break;\n>> +    }\n>> +    mean = total / z;\n>> +\n>> +    for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 && \n>> y_item[z].y2_avg != 0; z++)\n>> +    {\n>> +        var_sum = var_sum + ((y_item[z].y2_avg - mean) * \n>> (y_item[z].y2_avg - mean));\n>> +        if(y_item[z].y2_avg == 0)\n>> +            break;\n>> +    }\n>> +    currentVar_ = static_cast(var_sum) /static_cast(z);\n>> +    LOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n>> +\n>> +    if( context.frameContext.af.stable == true )\n>> +    {\n>> +        const uint32_t diff_var = std::abs(currentVar_ - \n>> context.frameContext.af.maxVar);\n>> +        const double var_ratio =  diff_var / \n>> context.frameContext.af.maxVar;\n>> +        LOG(IPU3Af, Debug) << \"Change ratio: \"\n>> +                   << var_ratio\n>> +                   << \" current focus: \"\n>> +                   << context.frameContext.af.focus;\n>> +        if(var_ratio > 0.8)\n> \n> \n> hmm,  I think we should opt for \"variance_\" instead \"var_\" as var in \n> general has variable meaning, nevermind, details...\n> \n>> +        {\n>> +            if(ignoreFrame_ == 0)\n>> +            {\n>> +                context.frameContext.af.maxVar = 0;\n>> +                context.frameContext.af.focus = 0;\n>> +                focus_ = 0;\n>> +                context.frameContext.af.stable = false;\n>> +                ignoreFrame_ = 60;\n>> +            }\n>> +            else\n>> +                ignoreFrame_--;\n>> +        }else\n>> +            ignoreFrame_ = 60;\n>> +    }else\n>> +    {\n>> +        if(ignoreFrame_ != 0)\n>> +            ignoreFrame_--;\n>> +        else{\n>> +            if(currentVar_ > context.frameContext.af.maxVar)\n>> +            {\n>> +                context.frameContext.af.maxVar = currentVar_;\n>> +                context.frameContext.af.focus = focus_;\n>> +            }\n>> +\n>> +            if(focus_ > 1023)\n>> +            {\n>> +                context.frameContext.af.stable = true;\n>> +                vcmSet(context.frameContext.af.focus);\n>> +                LOG(IPU3Af, Debug) << \"Finall Focus \"\n>> +                           << context.frameContext.af.focus;\n>> +            }else\n>> +            {\n>> +                focus_ += 5;\n>> +                vcmSet(focus_);\n>> +            }\n>> +            LOG(IPU3Af, Debug)  << \"Focus searching max var is: \"\n>> +                        << context.frameContext.af.maxVar\n>> +                        << \" Focus step is \"\n>> +                        << context.frameContext.af.focus;\n>> +        }\n>> +    }\n>> +}\n>> +\n>> +\n>> +} /* namespace ipa::ipu3::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> \\ No newline at end of file\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..64c704b1\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/af.h\n>> @@ -0,0 +1,48 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Red Hat\n>> + *\n>> + * af.h - IPU3 Af control\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>> +namespace libcamera {\n>> +\n>> +namespace ipa::ipu3::algorithms {\n>> +\n>> +class Af : public Algorithm\n>> +{\n>> +public:\n>> +    Af();\n>> +    ~Af();\n>> +\n>> +    int configure(IPAContext &context, const IPAConfigInfo \n>> &configInfo) override;\n>> +    void process(IPAContext &context, const ipu3_uapi_stats_3a \n>> *stats) override;\n>> +\n>> +private:\n>> +    void filterVariance(double new_var);\n>> +\n>> +    void vcmSet(int value);\n>> +\n>> +    int vcmFd_;\n>> +    int focus_;\n>> +    unsigned int ignoreFrame_;\n> \n> \n> What's this stand for? Is it ignoring a number of frames?\n> \n>> +    double maxVariance_;\n>> +    double currentVar_;\n>> +\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 \n>> b/src/ipa/ipu3/algorithms/meson.build\n>> index 3ec42f72..d32c61d2 100644\n>> --- a/src/ipa/ipu3/algorithms/meson.build\n>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>> @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n>>       'algorithm.cpp',\n>>       'awb.cpp',\n>>       'blc.cpp',\n>> -    'tone_mapping.cpp',\n>> +    'af.cpp',\n>> +    'tone_mapping.cpp'\n> \n> \n> alphabetical order please, I think checkstyle.py will report this as well\n> \n>>   ])\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index 1e46c61a..5d92f63c 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -47,6 +47,12 @@ struct IPAFrameContext {\n>>           } gains;\n>>       } awb;\n>> +    struct {\n>> +        uint32_t focus;\n>> +        double maxVar;\n>> +        bool stable;\n>> +    } af;\n>> +\n>>       struct {\n>>           double gamma;\n>>           struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 5c51607d..980815ee 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -31,6 +31,7 @@\n>>   #include \"libcamera/internal/mapped_framebuffer.h\"\n>>   #include \"algorithms/agc.h\"\n>> +#include \"algorithms/af.h\"\n> \n> \n> Ditto.\n> \n>>   #include \"algorithms/algorithm.h\"\n>>   #include \"algorithms/awb.h\"\n>>   #include \"algorithms/blc.h\"\n>> @@ -298,6 +299,7 @@ int IPAIPU3::init(const IPASettings &settings,\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>> \n>> algorithms_.push_back(std::make_unique()); \n>>","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 2AD67BDB1C\n\tfor ;\n\tThu, 18 Nov 2021 12:43:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D15D360378;\n\tThu, 18 Nov 2021 13:42:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2CB4F60231\n\tfor ;\n\tThu, 18 Nov 2021 13:42:58 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:b12c:8f66:c7d9:e3b7] (unknown\n\t[IPv6:2a01:e0a:169:7140:b12c:8f66:c7d9:e3b7])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D718DE7;\n\tThu, 18 Nov 2021 13:42:57 +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=\"j8VmSkDl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637239377;\n\tbh=G7SnOzTB+NWQBgB2bTbWlFikOqTi78j++1zsnop47Vg=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=j8VmSkDlD3JoeHvNx4HBAuwgEjeWyVugFQTi4Rmah8mxX8k0Ubu+ZOiFbi9Iy0eVK\n\tRnxNMX3A4YdCSGbznHq19/wTNsacOOCjQ2KJfAyloA+ooHCFQOXFlm2fcEiQD8SBK+\n\tL2PHglk8k6bSYvm9uClwZ4YFZVnzLZ8pU5KmhDIs=","Message-ID":"<3e4d378c-fe8c-22f8-c620-2817767d0426@ideasonboard.com>","Date":"Thu, 18 Nov 2021 13:42:55 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Umang Jain , Kate Hsuan ,\n\tlibcamera devel ","References":"<20211115054400.17797-1-hpa@redhat.com>\n\t<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] 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":21007,"web_url":"https://patchwork.libcamera.org/comment/21007/","msgid":"","date":"2021-11-18T12:50:13","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: af: Auto focus for dw9719\n\tSurface Go2 VCM","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 18, 2021 at 06:01:35PM +0530, Umang Jain wrote:\n> Hi Kate,\n> \n> Thank you for you work on this.\n> \n> While I understand this is an MVP (as posted by you in one of the other \n> autofocus threads) here are some of my high-level queries and \n> understanding. The interface to handle autofocus (and focus controls) in \n> general is missing from libcamera, so I understand the current \n> limitations as well, to move this forward.\n> \n> On 11/15/21 11:14 AM, 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> > The variance of each focus step is determined and a greedy\n> > approah is used to find the maximum variance of the AF\n> > state and a appropriate focus value.\n> \n> Can you please suggest a high level reading of finding the focus on the \n> image? I don't understand the implementation / algorithm for the \n> focus-sweep implemented in process()?\n> \n> > Signed-off-by: Kate Hsuan \n> > ---\n> > src/ipa/ipu3/algorithms/af.cpp | 165 ++++++++++++++++++++++++++++\n> > src/ipa/ipu3/algorithms/af.h | 48 ++++++++\n> > src/ipa/ipu3/algorithms/meson.build | 3 +-\n> > src/ipa/ipu3/ipa_context.h | 6 +\n> > src/ipa/ipu3/ipu3.cpp | 2 +\n> > 5 files changed, 223 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..c276b539\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> \n> I won't get into style issues here (as don't seem relevant for now), but \n> for your information we do have a style checker at:\n> \n>     $libcamera/utils/checkstyle.py \n\nWhich can be automated with a git commit hook:\n\ncp utils/hooks/post-commit .git/hooks/\n\n> > @@ -0,0 +1,165 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.cpp - IPU3 Af control\n> \n> would be better to expand on the acronym \"IPU3 Auto Focus control\" maybe\n> \n> > + */\n> > +\n> > +#include \"af.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libipa/histogram.h\"\n> > +\n> > +/**\n> > + * \\file af.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +LOG_DEFINE_CATEGORY(IPU3Af)\n> > +\n> > +Af::Af(): focus_(0), maxVariance_(0.0), currentVar_(0.0)\n> > +{\n> > +\t/* For surface Go 2 back camera VCM */\n> > +\tvcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n> \n> So I have nautilus (Samsung Chromebook v2) which as a UVC camera and a \n> IPU3 one (with IMX258 sensor). The VCM there is dw9807.\n> \n> I tried this patch on nautilus by mapping the v4l-subdevX to dw9807 and \n> it did work. I can literally hear the VCM move and tries to focus on the \n> scene. However, there are a few niggles I have noticed:\n> \n> The autofocus seems to lock up quite frequently leaving an un-focused \n> streaming. However if I put up a object close to lens it does tries to \n> focus it again (then locks up again but works sometimes). I am \n> deliberately leaving the details out as it's too broad to say anything \n> specific from nautilus' side.\n> \n> I am interesting in getting better understanding of the values reported, \n> so once I have a overall reading on the topic, I can start debugging the \n> process().\n> \n> > +}\n> > +\n> > +Af::~Af()\n> > +{\n> > +\tif(vcmFd_ != -1)\n> > +\t\tclose(vcmFd_);\n> > +}\n> > +\n> > +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> \n> I think [[maybe_unused]] on configInfo as a replacement for the below line\n> \n> > +{\n> > +\tconst IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n> > +\tcontext.frameContext.af.focus = 0;\n> > +\tcontext.frameContext.af.maxVar = 0;\n> > +\tcontext.frameContext.af.stable = false;\n> > +\n> > +\tmaxVariance_ = 0;\n> > +\tignoreFrame_ = 100;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void Af::vcmSet(int value)\n> > +{\n> > +\tint ret;\n> > +\tstruct v4l2_control ctrl;\n> > +\tif(vcmFd_ == -1)\n> > +\t\treturn;\n> \n> Maybe we should return -EINVAL?\n> \n> > +\tmemset(&ctrl, 0, sizeof(struct v4l2_control));\n> > +\tctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n> > +\tctrl.value = value;\n> > +\tret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);\n> \n> \n> and return ret instead? To check if we really succeeded on setting the \n> control\n> \n> > +\n> > +}\n> > +\n> > +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n> > +{\n> > +\ttypedef struct y_table_item {\n> > +\t\tuint16_t y1_avg;\n> > +\t\tuint16_t y2_avg;\n> > +\t}y_table_item_t;\n> > +\n> > +\tuint32_t total = 0;\n> > +\tdouble mean;\n> > +\tuint64_t var_sum = 0;\n> > +\ty_table_item_t *y_item;\n> > +\n> > +\ty_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n> > +\tint z = 0;\n> > +\t\n> > +\tfor(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n> > +\t{\n> > +\t\ttotal = total + y_item[z].y2_avg;\n> > +\t\tif(y_item[z].y2_avg == 0)\n> > +\t\t\tbreak;\n> > +\t}\n> > +\tmean = total / z;\n> > +\n> > +\tfor(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 && y_item[z].y2_avg != 0; z++)\n> > +\t{\n> > +\t\tvar_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));\n> > +\t\tif(y_item[z].y2_avg == 0)\n> > +\t\t\tbreak;\n> > +\t}\n> > +\tcurrentVar_ = static_cast(var_sum) /static_cast(z);\n> > +\tLOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n> > +\n> > +\tif( context.frameContext.af.stable == true )\n> > +\t{\n> > +\t\tconst uint32_t diff_var = std::abs(currentVar_ - context.frameContext.af.maxVar);\n> > +\t\tconst double var_ratio = diff_var / context.frameContext.af.maxVar;\n> > +\t\tLOG(IPU3Af, Debug) << \"Change ratio: \"\n> > +\t\t\t\t << var_ratio\n> > +\t\t\t\t << \" current focus: \"\n> > +\t\t\t\t << context.frameContext.af.focus;\n> > +\t\tif(var_ratio > 0.8)\n> \n> \n> hmm,  I think we should opt for \"variance_\" instead \"var_\" as var in \n> general has variable meaning, nevermind, details...\n> \n> > +\t\t{\n> > +\t\t\tif(ignoreFrame_ == 0)\n> > +\t\t\t{\n> > +\t\t\t\tcontext.frameContext.af.maxVar = 0;\n> > +\t\t\t\tcontext.frameContext.af.focus = 0;\n> > +\t\t\t\tfocus_ = 0;\n> > +\t\t\t\tcontext.frameContext.af.stable = false;\n> > +\t\t\t\tignoreFrame_ = 60;\n> > +\t\t\t}\n> > +\t\t\telse\n> > +\t\t\t\tignoreFrame_--;\n> > +\t\t}else\n> > +\t\t\tignoreFrame_ = 60;\n> > +\t}else\n> > +\t{\n> > +\t\tif(ignoreFrame_ != 0)\n> > +\t\t\tignoreFrame_--;\n> > +\t\telse{\n> > +\t\t\tif(currentVar_ > context.frameContext.af.maxVar)\n> > +\t\t\t{\n> > +\t\t\t\tcontext.frameContext.af.maxVar = currentVar_;\n> > +\t\t\t\tcontext.frameContext.af.focus = focus_;\n> > +\t\t\t}\n> > +\n> > +\t\t\tif(focus_ > 1023)\n> > +\t\t\t{\n> > +\t\t\t\tcontext.frameContext.af.stable = true;\n> > +\t\t\t\tvcmSet(context.frameContext.af.focus);\n> > +\t\t\t\tLOG(IPU3Af, Debug) << \"Finall Focus \"\n> > +\t\t\t\t\t\t << context.frameContext.af.focus;\n> > +\t\t\t}else\n> > +\t\t\t{\n> > +\t\t\t\tfocus_ += 5;\n> > +\t\t\t\tvcmSet(focus_);\n> > +\t\t\t}\n> > +\t\t\tLOG(IPU3Af, Debug) << \"Focus searching max var is: \"\n> > +\t\t\t\t\t << context.frameContext.af.maxVar\n> > +\t\t\t\t\t << \" Focus step is \"\n> > +\t\t\t\t\t << context.frameContext.af.focus;\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> > +\n> > +} /* namespace ipa::ipu3::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > \\ No newline at end of file\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..64c704b1\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.h\n> > @@ -0,0 +1,48 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.h - IPU3 Af control\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> > +namespace libcamera {\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +class Af : public Algorithm\n> > +{\n> > +public:\n> > +\tAf();\n> > +\t~Af();\n> > +\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 filterVariance(double new_var);\n> > +\n> > +\tvoid vcmSet(int value);\n> > +\n> > +\tint vcmFd_;\n> > +\tint focus_;\n> > +\tunsigned int ignoreFrame_;\n> \n> \n> What's this stand for? Is it ignoring a number of frames?\n> \n> > +\tdouble maxVariance_;\n> > +\tdouble currentVar_;\n> > +\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 3ec42f72..d32c61d2 100644\n> > --- a/src/ipa/ipu3/algorithms/meson.build\n> > +++ b/src/ipa/ipu3/algorithms/meson.build\n> > @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n> > 'algorithm.cpp',\n> > 'awb.cpp',\n> > 'blc.cpp',\n> > - 'tone_mapping.cpp',\n> > + 'af.cpp',\n> > + 'tone_mapping.cpp'\n> \n> \n> alphabetical order please, I think checkstyle.py will report this as well\n> \n> > ])\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index 1e46c61a..5d92f63c 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -47,6 +47,12 @@ struct IPAFrameContext {\n> > \t\t} gains;\n> > \t} awb;\n> > \n> > +\tstruct {\n> > +\t\tuint32_t focus;\n> > +\t\tdouble maxVar;\n> > +\t\tbool stable;\n> > +\t} af;\n> > +\n> > \tstruct {\n> > \t\tdouble gamma;\n> > \t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 5c51607d..980815ee 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -31,6 +31,7 @@\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > \n> > #include \"algorithms/agc.h\"\n> > +#include \"algorithms/af.h\"\n> \n> \n> Ditto.\n> \n> > #include \"algorithms/algorithm.h\"\n> > #include \"algorithms/awb.h\"\n> > #include \"algorithms/blc.h\"\n> > @@ -298,6 +299,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());","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 2A688BF415\n\tfor ;\n\tThu, 18 Nov 2021 12:50:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAE3D60371;\n\tThu, 18 Nov 2021 13:50:37 +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 C752E60231\n\tfor ;\n\tThu, 18 Nov 2021 13:50:35 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5ED41E7;\n\tThu, 18 Nov 2021 13:50:35 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"m0fhPnwi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637239835;\n\tbh=S/BpebW7CMH2WCBq8tZYlQ6DDVY8TCF3zFr/5P7w+po=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m0fhPnwir/b2QmyNXmB/UQsZEohutoc0XzvI2iY1miaLjVPEzhQk0JUfCM2QIwQPW\n\to2cMJPjYJULAi+qzpQ42HlFHuknQOak/0CAoib2ro9RwXntQ/3tFhBM97dkAIiPJSg\n\tA+/Nq3ZEhv4MOnfTE9nUzZHWLjQkf0Vf1Gy8BYyc=","Date":"Thu, 18 Nov 2021 14:50:13 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211115054400.17797-1-hpa@redhat.com>\n\t<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] 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\" "}},{"id":21008,"web_url":"https://patchwork.libcamera.org/comment/21008/","msgid":"<8653546e-97c4-2287-b1d6-70766c4edf55@ideasonboard.com>","date":"2021-11-18T12:53:19","subject":"Re: [libcamera-devel] [PATCH] 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":"On 18/11/2021 13:42, Jean-Michel Hautbois wrote:\n> Hi Umang,\n> \n> On 18/11/2021 13:31, Umang Jain wrote:\n>> Hi Kate,\n>>\n>> Thank you for you work on this.\n>>\n>> While I understand this is an MVP (as posted by you in one of the \n>> other autofocus threads) here are some of my high-level queries and \n>> understanding. The interface to handle autofocus (and focus controls) \n>> in general is missing from libcamera, so I understand the current \n>> limitations as well, to move this forward.\n>>\n>> On 11/15/21 11:14 AM, 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>>> The variance of each focus step is determined and a greedy\n>>> approah is used to find the maximum variance of the AF\n>>> state and a appropriate focus value.\n>>\n>>\n>> Can you please suggest a high level reading of finding the focus on \n>> the image? I don't understand the implementation / algorithm for the \n>> focus-sweep implemented in process()?\n>>\n>>>\n>>> Signed-off-by: Kate Hsuan \n>>> ---\n>>>   src/ipa/ipu3/algorithms/af.cpp      | 165 ++++++++++++++++++++++++++++\n>>>   src/ipa/ipu3/algorithms/af.h        |  48 ++++++++\n>>>   src/ipa/ipu3/algorithms/meson.build |   3 +-\n>>>   src/ipa/ipu3/ipa_context.h          |   6 +\n>>>   src/ipa/ipu3/ipu3.cpp               |   2 +\n>>>   5 files changed, 223 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 \n>>> b/src/ipa/ipu3/algorithms/af.cpp\n>>> new file mode 100644\n>>> index 00000000..c276b539\n>>> --- /dev/null\n>>> +++ b/src/ipa/ipu3/algorithms/af.cpp\n>>\n>>\n>> I won't get into style issues here (as don't seem relevant for now), \n>> but for your information we do have a style checker at:\n>>\n>>      $libcamera/utils/checkstyle.py \n>>\n>>> @@ -0,0 +1,165 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Red Hat\n>>> + *\n>>> + * af.cpp - IPU3 Af control\n>>\n>>\n>> would be better to expand on the acronym \"IPU3 Auto Focus control\" maybe\n>>\n>>> + */\n>>> +\n>>> +#include \"af.h\"\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +#include \"libipa/histogram.h\"\n>>> +\n>>> +/**\n>>> + * \\file af.h\n>>> + */\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +using namespace std::literals::chrono_literals;\n>>> +\n>>> +namespace ipa::ipu3::algorithms {\n>>> +\n>>> +LOG_DEFINE_CATEGORY(IPU3Af)\n>>> +\n>>> +Af::Af(): focus_(0),  maxVariance_(0.0), currentVar_(0.0)\n>>> +{\n>>> +    /* For surface Go 2 back camera VCM */\n>>> +    vcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n>>\n>>\n>> So I have nautilus (Samsung Chromebook v2) which as a UVC camera and a \n>> IPU3 one (with IMX258 sensor). The VCM there is dw9807.\n>>\n>> I tried this patch on nautilus by mapping the v4l-subdevX to dw9807 \n>> and it did work. I can literally hear the VCM move and tries to focus \n>> on the scene. However, there are a few niggles I have noticed:\n>>\n>> The autofocus seems to lock up quite frequently leaving an un-focused \n>> streaming. However if I put up a object close to lens it does tries to \n>> focus it again (then locks up again but works sometimes). I am \n>> deliberately leaving the details out as it's too broad to say anything \n>> specific from nautilus' side.\n> \n> I will let Kate answer in depth obviously. From what I know, the AF \n> needs a grid configuration in IPU3, and the default grid is set to \n> [16*8,16*8] pixels, starting at x=10 and y=2: \n> https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ipu3/ipu3-tables.c#L9587 \n> \n> \n> It means that if your BDS output size is 1280x720 for instance, you will \n> focus on the 10% left part and 17% top part of your scene. Depending on \n> the scene, you might very well be out-of-focus :-).\n> \n> That's why I suggested adding a configure() call to set a grid based on \n> the BDS grid, to make it focus in the center as a default.\n> \n\nBe aware that AF grid can't always be the same as AWB grid, as the \nlimits are not the same:\nhttps://lore.kernel.org/linux-media/1634525295-1410-1-git-send-email-bingbu.cao@intel.com/\n\nAnd I can see in CrOS that the log2 may not be 7 but 6:\nhttps://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/af_public.h#32\n\n>>\n>> I am interesting in getting better understanding of the values \n>> reported, so once I have a overall reading on the topic, I can start \n>> debugging the process().\n>>\n>>> +}\n>>> +\n>>> +Af::~Af()\n>>> +{\n>>> +    if(vcmFd_ != -1)\n>>> +        close(vcmFd_);\n>>> +}\n>>> +\n>>> +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>>\n>>\n>> I think [[maybe_unused]] on configInfo as a replacement for the below \n>> line\n>>\n>>> +{\n>>> +    const IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n>>> +    context.frameContext.af.focus = 0;\n>>> +    context.frameContext.af.maxVar = 0;\n>>> +    context.frameContext.af.stable = false;\n>>> +\n>>> +    maxVariance_ = 0;\n>>> +    ignoreFrame_ = 100;\n>>> +\n>>> +    return 0;\n>>> +}\n>>> +\n>>> +void Af::vcmSet(int value)\n>>> +{\n>>> +    int ret;\n>>> +    struct v4l2_control ctrl;\n>>> +    if(vcmFd_ == -1)\n>>> +        return;\n>>\n>>\n>> Maybe we should return -EINVAL?\n>>\n>>> +    memset(&ctrl, 0, sizeof(struct v4l2_control));\n>>> +    ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n>>> +    ctrl.value = value;\n>>> +    ret = ioctl(vcmFd_,  VIDIOC_S_CTRL, &ctrl);\n>>\n>>\n>> and return ret instead? To check if we really succeeded on setting the \n>> control\n>>\n>>> +\n>>> +}\n>>> +\n>>> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n>>> +{\n>>> +    typedef struct y_table_item {\n>>> +        uint16_t y1_avg;\n>>> +        uint16_t y2_avg;\n>>> +    }y_table_item_t;\n>>> +\n>>> +    uint32_t total = 0;\n>>> +    double mean;\n>>> +    uint64_t var_sum = 0;\n>>> +    y_table_item_t *y_item;\n>>> +\n>>> +    y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n>>> +    int z = 0;\n>>> +\n>>> +    for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n>>> +    {\n>>> +        total = total + y_item[z].y2_avg;\n>>> +        if(y_item[z].y2_avg == 0)\n>>> +            break;\n>>> +    }\n>>> +    mean = total / z;\n>>> +\n>>> +    for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 && \n>>> y_item[z].y2_avg != 0; z++)\n>>> +    {\n>>> +        var_sum = var_sum + ((y_item[z].y2_avg - mean) * \n>>> (y_item[z].y2_avg - mean));\n>>> +        if(y_item[z].y2_avg == 0)\n>>> +            break;\n>>> +    }\n>>> +    currentVar_ = static_cast(var_sum) /static_cast(z);\n>>> +    LOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n>>> +\n>>> +    if( context.frameContext.af.stable == true )\n>>> +    {\n>>> +        const uint32_t diff_var = std::abs(currentVar_ - \n>>> context.frameContext.af.maxVar);\n>>> +        const double var_ratio =  diff_var / \n>>> context.frameContext.af.maxVar;\n>>> +        LOG(IPU3Af, Debug) << \"Change ratio: \"\n>>> +                   << var_ratio\n>>> +                   << \" current focus: \"\n>>> +                   << context.frameContext.af.focus;\n>>> +        if(var_ratio > 0.8)\n>>\n>>\n>> hmm,  I think we should opt for \"variance_\" instead \"var_\" as var in \n>> general has variable meaning, nevermind, details...\n>>\n>>> +        {\n>>> +            if(ignoreFrame_ == 0)\n>>> +            {\n>>> +                context.frameContext.af.maxVar = 0;\n>>> +                context.frameContext.af.focus = 0;\n>>> +                focus_ = 0;\n>>> +                context.frameContext.af.stable = false;\n>>> +                ignoreFrame_ = 60;\n>>> +            }\n>>> +            else\n>>> +                ignoreFrame_--;\n>>> +        }else\n>>> +            ignoreFrame_ = 60;\n>>> +    }else\n>>> +    {\n>>> +        if(ignoreFrame_ != 0)\n>>> +            ignoreFrame_--;\n>>> +        else{\n>>> +            if(currentVar_ > context.frameContext.af.maxVar)\n>>> +            {\n>>> +                context.frameContext.af.maxVar = currentVar_;\n>>> +                context.frameContext.af.focus = focus_;\n>>> +            }\n>>> +\n>>> +            if(focus_ > 1023)\n>>> +            {\n>>> +                context.frameContext.af.stable = true;\n>>> +                vcmSet(context.frameContext.af.focus);\n>>> +                LOG(IPU3Af, Debug) << \"Finall Focus \"\n>>> +                           << context.frameContext.af.focus;\n>>> +            }else\n>>> +            {\n>>> +                focus_ += 5;\n>>> +                vcmSet(focus_);\n>>> +            }\n>>> +            LOG(IPU3Af, Debug)  << \"Focus searching max var is: \"\n>>> +                        << context.frameContext.af.maxVar\n>>> +                        << \" Focus step is \"\n>>> +                        << context.frameContext.af.focus;\n>>> +        }\n>>> +    }\n>>> +}\n>>> +\n>>> +\n>>> +} /* namespace ipa::ipu3::algorithms */\n>>> +\n>>> +} /* namespace libcamera */\n>>> \\ No newline at end of file\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..64c704b1\n>>> --- /dev/null\n>>> +++ b/src/ipa/ipu3/algorithms/af.h\n>>> @@ -0,0 +1,48 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Red Hat\n>>> + *\n>>> + * af.h - IPU3 Af control\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>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa::ipu3::algorithms {\n>>> +\n>>> +class Af : public Algorithm\n>>> +{\n>>> +public:\n>>> +    Af();\n>>> +    ~Af();\n>>> +\n>>> +    int configure(IPAContext &context, const IPAConfigInfo \n>>> &configInfo) override;\n>>> +    void process(IPAContext &context, const ipu3_uapi_stats_3a \n>>> *stats) override;\n>>> +\n>>> +private:\n>>> +    void filterVariance(double new_var);\n>>> +\n>>> +    void vcmSet(int value);\n>>> +\n>>> +    int vcmFd_;\n>>> +    int focus_;\n>>> +    unsigned int ignoreFrame_;\n>>\n>>\n>> What's this stand for? Is it ignoring a number of frames?\n>>\n>>> +    double maxVariance_;\n>>> +    double currentVar_;\n>>> +\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 \n>>> b/src/ipa/ipu3/algorithms/meson.build\n>>> index 3ec42f72..d32c61d2 100644\n>>> --- a/src/ipa/ipu3/algorithms/meson.build\n>>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>>> @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n>>>       'algorithm.cpp',\n>>>       'awb.cpp',\n>>>       'blc.cpp',\n>>> -    'tone_mapping.cpp',\n>>> +    'af.cpp',\n>>> +    'tone_mapping.cpp'\n>>\n>>\n>> alphabetical order please, I think checkstyle.py will report this as well\n>>\n>>>   ])\n>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>>> index 1e46c61a..5d92f63c 100644\n>>> --- a/src/ipa/ipu3/ipa_context.h\n>>> +++ b/src/ipa/ipu3/ipa_context.h\n>>> @@ -47,6 +47,12 @@ struct IPAFrameContext {\n>>>           } gains;\n>>>       } awb;\n>>> +    struct {\n>>> +        uint32_t focus;\n>>> +        double maxVar;\n>>> +        bool stable;\n>>> +    } af;\n>>> +\n>>>       struct {\n>>>           double gamma;\n>>>           struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index 5c51607d..980815ee 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -31,6 +31,7 @@\n>>>   #include \"libcamera/internal/mapped_framebuffer.h\"\n>>>   #include \"algorithms/agc.h\"\n>>> +#include \"algorithms/af.h\"\n>>\n>>\n>> Ditto.\n>>\n>>>   #include \"algorithms/algorithm.h\"\n>>>   #include \"algorithms/awb.h\"\n>>>   #include \"algorithms/blc.h\"\n>>> @@ -298,6 +299,7 @@ int IPAIPU3::init(const IPASettings &settings,\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>>>","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 9781ABDB1C\n\tfor ;\n\tThu, 18 Nov 2021 12:53:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC4AA60378;\n\tThu, 18 Nov 2021 13:53:24 +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 E36AB60231\n\tfor ;\n\tThu, 18 Nov 2021 13:53:22 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:b12c:8f66:c7d9:e3b7] (unknown\n\t[IPv6:2a01:e0a:169:7140:b12c:8f66:c7d9:e3b7])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D846513;\n\tThu, 18 Nov 2021 13:53:22 +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=\"r4DZ6l9I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637240002;\n\tbh=8wnemGmBAVNJ3Hvl+8MkJfm8c8A8idLaotBuYOGxOLI=;\n\th=Date:Subject:From:To:References:In-Reply-To:From;\n\tb=r4DZ6l9I9vomQQeglOnhaMSZ1iR/tkrls3jHgpXzDeJVJnyfcZNTxhe2A4VsIv6RH\n\ts/DW/BfQoCfhtDaK30FOpFvT2UOg5z1SFXxdyfpP0WUFyIfKqUVU7g0ona1UXP93aj\n\tPrt/tlUetYgSlsp8DEMuIE1HBNDcFp0B4wHsRlm4=","Message-ID":"<8653546e-97c4-2287-b1d6-70766c4edf55@ideasonboard.com>","Date":"Thu, 18 Nov 2021 13:53:19 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","From":"Jean-Michel Hautbois ","To":"Umang Jain , Kate Hsuan ,\n\tlibcamera devel ","References":"<20211115054400.17797-1-hpa@redhat.com>\n\t<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>\n\t<3e4d378c-fe8c-22f8-c620-2817767d0426@ideasonboard.com>","In-Reply-To":"<3e4d378c-fe8c-22f8-c620-2817767d0426@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] 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":21014,"web_url":"https://patchwork.libcamera.org/comment/21014/","msgid":"","date":"2021-11-19T06:06:05","subject":"Re: [libcamera-devel] [PATCH] 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":"Hi Umang,\n\nOn Thu, Nov 18, 2021 at 8:40 PM Umang Jain wrote:\n>\n> Hi Kate,\n\n> Thank you for you work on this.\n>\n> While I understand this is an MVP (as posted by you in one of the other\n> autofocus threads) here are some of my high-level queries and\n> understanding. The interface to handle autofocus (and focus controls) in\n> general is missing from libcamera, so I understand the current\n> limitations as well, to move this forward.\n\nYes, I agree with you. Since VCM was driven and works, I quickly made\nthis POC version of the autofocus then write the missing interface\npiece by piece.\n\n>\n> On 11/15/21 11:14 AM, 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> > The variance of each focus step is determined and a greedy\n> > approah is used to find the maximum variance of the AF\n> > state and a appropriate focus value.\n>\n>\n> Can you please suggest a high level reading of finding the focus on the\n> image? I don't understand the implementation / algorithm for the\n> focus-sweep implemented in process()?\n>\n\nThe idea of autofocus is to find the edge of the image then determine\nthe contrast of the image of every focus step. The blurred image will\nget a lower value of the contrast and the clearer image will get a\nhigher contrast. Based on this, the max contrast image can be found in\na greedy manner by comparing all the contrast values of each image\nfrom 0 to max step of the lens position.\n\nLaplacian of Gaussian and Sodel can be used to find the edge of the\nimage but they require more CPU resources to find the edge and compute\nthe contrast. I traced the IPU3 kernel code and ipu3-ipa repo a little\nbit and found it provides an AF raw buffer and contains the low pass\nand high pass filter statistic values. Typically, the high pass can be\nused to determine the sharpness (contrast) of the image. (I'm not an\nexpert on image processing. if it is wrong please correct me :) ). So,\nthe problem is simplified to calculate the variance of the AF buffer\nand search the image with the maximum variance. The algorithm is\nsimple, move the lens from 0 - 1023 (max step of dw9719), estimate the\nvariance of each step, and find the lens position with max variance.\nFinally, move the lens to the position.\n\n> >\n> > Signed-off-by: Kate Hsuan \n> > ---\n> > src/ipa/ipu3/algorithms/af.cpp | 165 ++++++++++++++++++++++++++++\n> > src/ipa/ipu3/algorithms/af.h | 48 ++++++++\n> > src/ipa/ipu3/algorithms/meson.build | 3 +-\n> > src/ipa/ipu3/ipa_context.h | 6 +\n> > src/ipa/ipu3/ipu3.cpp | 2 +\n> > 5 files changed, 223 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..c276b539\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.cpp\n>\n>\n> I won't get into style issues here (as don't seem relevant for now), but\n> for your information we do have a style checker at:\n>\n> $libcamera/utils/checkstyle.py \n\nThanks for noticing me this :)\n\n>\n> > @@ -0,0 +1,165 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.cpp - IPU3 Af control\n>\n>\n> would be better to expand on the acronym \"IPU3 Auto Focus control\" maybe\n\nOk thanks.\n\n\n>\n> > + */\n> > +\n> > +#include \"af.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libipa/histogram.h\"\n> > +\n> > +/**\n> > + * \\file af.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +LOG_DEFINE_CATEGORY(IPU3Af)\n> > +\n> > +Af::Af(): focus_(0), maxVariance_(0.0), currentVar_(0.0)\n> > +{\n> > + /* For surface Go 2 back camera VCM */\n> > + vcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n>\n>\n> So I have nautilus (Samsung Chromebook v2) which as a UVC camera and a\n> IPU3 one (with IMX258 sensor). The VCM there is dw9807.\n>\n> I tried this patch on nautilus by mapping the v4l-subdevX to dw9807 and\n> it did work. I can literally hear the VCM move and tries to focus on the\n> scene. However, there are a few niggles I have noticed:\n>\n> The autofocus seems to lock up quite frequently leaving an un-focused\n> streaming. However if I put up a object close to lens it does tries to\n\nThe AF area should be at the top left corner of the sensor (default).\nI don't configure the AF grid in this patch.\n\n> focus it again (then locks up again but works sometimes). I am\n> deliberately leaving the details out as it's too broad to say anything\n> specific from nautilus' side.\n\nSince the AE can not work correctly on my Surface Go 2, I manually set\nthe exposure and gain to get a stable image. You could try it.\n\n>\n> I am interesting in getting better understanding of the values reported,\n> so once I have a overall reading on the topic, I can start debugging the\n> process().\n\nThe value of debug messages are all around variance and focus step. :)\n\n>\n> > +}\n> > +\n> > +Af::~Af()\n> > +{\n> > + if(vcmFd_ != -1)\n> > + close(vcmFd_);\n> > +}\n> > +\n> > +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>\n>\n> I think [[maybe_unused]] on configInfo as a replacement for the below line\n\nOK thanks.\n\n>\n> > +{\n> > + const IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n> > + context.frameContext.af.focus = 0;\n> > + context.frameContext.af.maxVar = 0;\n> > + context.frameContext.af.stable = false;\n> > +\n> > + maxVariance_ = 0;\n> > + ignoreFrame_ = 100;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +void Af::vcmSet(int value)\n> > +{\n> > + int ret;\n> > + struct v4l2_control ctrl;\n> > + if(vcmFd_ == -1)\n> > + return;\n>\n>\n> Maybe we should return -EINVAL?\n\nSure.\n\n>\n> > + memset(&ctrl, 0, sizeof(struct v4l2_control));\n> > + ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n> > + ctrl.value = value;\n> > + ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);\n>\n>\n> and return ret instead? To check if we really succeeded on setting the\n> control\n\nHere I may improve the error checking but we don't actually put ioctl\nhere. It may move to someplace of libcamera.\n\n>\n> > +\n> > +}\n> > +\n> > +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n> > +{\n> > + typedef struct y_table_item {\n> > + uint16_t y1_avg;\n> > + uint16_t y2_avg;\n> > + }y_table_item_t;\n> > +\n> > + uint32_t total = 0;\n> > + double mean;\n> > + uint64_t var_sum = 0;\n> > + y_table_item_t *y_item;\n> > +\n> > + y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n> > + int z = 0;\n> > +\n> > + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n> > + {\n> > + total = total + y_item[z].y2_avg;\n> > + if(y_item[z].y2_avg == 0)\n> > + break;\n> > + }\n> > + mean = total / z;\n> > +\n> > + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 && y_item[z].y2_avg != 0; z++)\n> > + {\n> > + var_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));\n> > + if(y_item[z].y2_avg == 0)\n> > + break;\n> > + }\n> > + currentVar_ = static_cast(var_sum) /static_cast(z);\n> > + LOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n> > +\n> > + if( context.frameContext.af.stable == true )\n> > + {\n> > + const uint32_t diff_var = std::abs(currentVar_ - context.frameContext.af.maxVar);\n> > + const double var_ratio = diff_var / context.frameContext.af.maxVar;\n> > + LOG(IPU3Af, Debug) << \"Change ratio: \"\n> > + << var_ratio\n> > + << \" current focus: \"\n> > + << context.frameContext.af.focus;\n> > + if(var_ratio > 0.8)\n>\n>\n> hmm, I think we should opt for \"variance_\" instead \"var_\" as var in\n> general has variable meaning, nevermind, details...\n\nOK, sure. thank you.\n\n>\n> > + {\n> > + if(ignoreFrame_ == 0)\n> > + {\n> > + context.frameContext.af.maxVar = 0;\n> > + context.frameContext.af.focus = 0;\n> > + focus_ = 0;\n> > + context.frameContext.af.stable = false;\n> > + ignoreFrame_ = 60;\n> > + }\n> > + else\n> > + ignoreFrame_--;\n> > + }else\n> > + ignoreFrame_ = 60;\n> > + }else\n> > + {\n> > + if(ignoreFrame_ != 0)\n> > + ignoreFrame_--;\n> > + else{\n> > + if(currentVar_ > context.frameContext.af.maxVar)\n> > + {\n> > + context.frameContext.af.maxVar = currentVar_;\n> > + context.frameContext.af.focus = focus_;\n> > + }\n> > +\n> > + if(focus_ > 1023)\n> > + {\n> > + context.frameContext.af.stable = true;\n> > + vcmSet(context.frameContext.af.focus);\n> > + LOG(IPU3Af, Debug) << \"Finall Focus \"\n> > + << context.frameContext.af.focus;\n> > + }else\n> > + {\n> > + focus_ += 5;\n> > + vcmSet(focus_);\n> > + }\n> > + LOG(IPU3Af, Debug) << \"Focus searching max var is: \"\n> > + << context.frameContext.af.maxVar\n> > + << \" Focus step is \"\n> > + << context.frameContext.af.focus;\n> > + }\n> > + }\n> > +}\n> > +\n> > +\n> > +} /* namespace ipa::ipu3::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > \\ No newline at end of file\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..64c704b1\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/af.h\n> > @@ -0,0 +1,48 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + *\n> > + * af.h - IPU3 Af control\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> > +namespace libcamera {\n> > +\n> > +namespace ipa::ipu3::algorithms {\n> > +\n> > +class Af : public Algorithm\n> > +{\n> > +public:\n> > + Af();\n> > + ~Af();\n> > +\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 filterVariance(double new_var);\n> > +\n> > + void vcmSet(int value);\n> > +\n> > + int vcmFd_;\n> > + int focus_;\n> > + unsigned int ignoreFrame_;\n>\n>\n> What's this stand for? Is it ignoring a number of frames?\n\nIf we suddenly move the lens position, the variance will be out of the\nrange of change and trigger another AF scan. So, after ignoring some\nframes and waiting for the AF raw buffer stable, it starts to\ncalculate variance again.\n\n>\n> > + double maxVariance_;\n> > + double currentVar_;\n> > +\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 3ec42f72..d32c61d2 100644\n> > --- a/src/ipa/ipu3/algorithms/meson.build\n> > +++ b/src/ipa/ipu3/algorithms/meson.build\n> > @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n> > 'algorithm.cpp',\n> > 'awb.cpp',\n> > 'blc.cpp',\n> > - 'tone_mapping.cpp',\n> > + 'af.cpp',\n> > + 'tone_mapping.cpp'\n>\n>\n> alphabetical order please, I think checkstyle.py will report this as well\n\nOk thanks.\n\n>\n> > ])\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index 1e46c61a..5d92f63c 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -47,6 +47,12 @@ struct IPAFrameContext {\n> > } gains;\n> > } awb;\n> >\n> > + struct {\n> > + uint32_t focus;\n> > + double maxVar;\n> > + bool stable;\n> > + } af;\n> > +\n> > struct {\n> > double gamma;\n> > struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 5c51607d..980815ee 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -31,6 +31,7 @@\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > #include \"algorithms/agc.h\"\n> > +#include \"algorithms/af.h\"\n>\n>\n> Ditto.\n>\n> > #include \"algorithms/algorithm.h\"\n> > #include \"algorithms/awb.h\"\n> > #include \"algorithms/blc.h\"\n> > @@ -298,6 +299,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>\n\n\nThank you.","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 397A9BDB1C\n\tfor ;\n\tFri, 19 Nov 2021 06:06:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6607960378;\n\tFri, 19 Nov 2021 07:06:26 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0DD3060121\n\tfor ;\n\tFri, 19 Nov 2021 07:06:24 +0100 (CET)","from mail-lf1-f70.google.com (mail-lf1-f70.google.com\n\t[209.85.167.70]) 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-417-AqE-D6oNNs-Xbd_ZclQuwQ-1; Fri, 19 Nov 2021 01:06:18 -0500","by mail-lf1-f70.google.com with SMTP id\n\tx7-20020a056512130700b003fd1a7424a8so5790374lfu.5\n\tfor ;\n\tThu, 18 Nov 2021 22:06:18 -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=\"ZmQtcKJR\"; 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=1637301983;\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=4tY1bITngV6/MLSUpD0EH3jzsc9aB6qGIaix4dl05W4=;\n\tb=ZmQtcKJRZfG+Xj91FtFBHzI4DsRYABsq7rNAcs7+1Mu8qQi2Gy9fMgQbfGzLN3bYYlZEo3\n\tWiicfQa1v6TdtnLg58h8w0zzg/3MjY/kYMoF+CZEAw2uuVEHmFn7DhD5b6pocX16DuSL5u\n\tza9z18pCspxxNIZtX9gSj2nmrpeRV5k=","X-MC-Unique":"AqE-D6oNNs-Xbd_ZclQuwQ-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=4tY1bITngV6/MLSUpD0EH3jzsc9aB6qGIaix4dl05W4=;\n\tb=L0QjlKGKiYXo6/O7SetljO/oEDrKvF/uBxZxhPTfEO+760MncZT/ZjUO7o5uVdyHBg\n\tn/ykPI56bc9kNUcEoTVdjEpu/wVILSFnH6BfBRoiJVnZGdxgc0XImOcYluw4/23DfA1h\n\tukDuf/m47058UJ99PRBVscjPHOj3kIzvTt+ddL8PAnq9ON2k+EY89guRqcd+ECOG4WQO\n\trCjLunb+pEkxULBbwsHMp582NMBajvK2d+8bJ7ac9C0dD9OqcvzWf2/eBtPkO90zomsy\n\tifpOlD0f/pKQ33FC8IFLDhHevwT3XEOaWmsNlUnOblq/5dFi0O6h75XaADCQm8ToT8CH\n\tY8Fw==","X-Gm-Message-State":"AOAM530+It/QzUtR6euRWDQG0+XOnIolg+hIkyXGXfO/oIb3q95sV5J2\n\tgHU4tX6pIUU1DGclqyw/sfycQNpaCeSMJ0H4FTHET/XK4P8pt9fNWOU8EfCmcsyL0pWmFrs80Wg\n\tXcNru3VMRZsfjUPN7pufwnCjNwDPxye0UBNVC6+1yMhKO7K2o4Q==","X-Received":["by 2002:ac2:4292:: with SMTP id\n\tm18mr30252359lfh.539.1637301976773; \n\tThu, 18 Nov 2021 22:06:16 -0800 (PST)","by 2002:ac2:4292:: with SMTP id\n\tm18mr30252316lfh.539.1637301976275; \n\tThu, 18 Nov 2021 22:06:16 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJyKvgI63nhfH7zbutEovMGg9bwKjQaO6s9O6wT6c066Ufqu83kdFvc3PshpZO6rC6raq64CyPV0nbEqIciFFQw=","MIME-Version":"1.0","References":"<20211115054400.17797-1-hpa@redhat.com>\n\t<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","In-Reply-To":"<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>","From":"Kate Hsuan ","Date":"Fri, 19 Nov 2021 14:06:05 +0800","Message-ID":"","To":"Umang Jain ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] 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\" "}},{"id":21015,"web_url":"https://patchwork.libcamera.org/comment/21015/","msgid":"","date":"2021-11-19T06:16:25","subject":"Re: [libcamera-devel] [PATCH] 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":"Hi Jean-Michel,\n\nOn Thu, Nov 18, 2021 at 8:43 PM Jean-Michel Hautbois\n wrote:\n>\n> Hi Umang,\n>\n> On 18/11/2021 13:31, Umang Jain wrote:\n> > Hi Kate,\n> >\n> > Thank you for you work on this.\n> >\n> > While I understand this is an MVP (as posted by you in one of the other\n> > autofocus threads) here are some of my high-level queries and\n> > understanding. The interface to handle autofocus (and focus controls) in\n> > general is missing from libcamera, so I understand the current\n> > limitations as well, to move this forward.\n> >\n> > On 11/15/21 11:14 AM, 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> >> The variance of each focus step is determined and a greedy\n> >> approah is used to find the maximum variance of the AF\n> >> state and a appropriate focus value.\n> >\n> >\n> > Can you please suggest a high level reading of finding the focus on the\n> > image? I don't understand the implementation / algorithm for the\n> > focus-sweep implemented in process()?\n> >\n> >>\n> >> Signed-off-by: Kate Hsuan \n> >> ---\n> >> src/ipa/ipu3/algorithms/af.cpp | 165 ++++++++++++++++++++++++++++\n> >> src/ipa/ipu3/algorithms/af.h | 48 ++++++++\n> >> src/ipa/ipu3/algorithms/meson.build | 3 +-\n> >> src/ipa/ipu3/ipa_context.h | 6 +\n> >> src/ipa/ipu3/ipu3.cpp | 2 +\n> >> 5 files changed, 223 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\n> >> b/src/ipa/ipu3/algorithms/af.cpp\n> >> new file mode 100644\n> >> index 00000000..c276b539\n> >> --- /dev/null\n> >> +++ b/src/ipa/ipu3/algorithms/af.cpp\n> >\n> >\n> > I won't get into style issues here (as don't seem relevant for now), but\n> > for your information we do have a style checker at:\n> >\n> > $libcamera/utils/checkstyle.py \n> >\n> >> @@ -0,0 +1,165 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Red Hat\n> >> + *\n> >> + * af.cpp - IPU3 Af control\n> >\n> >\n> > would be better to expand on the acronym \"IPU3 Auto Focus control\" maybe\n> >\n> >> + */\n> >> +\n> >> +#include \"af.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +#include \"libipa/histogram.h\"\n> >> +\n> >> +/**\n> >> + * \\file af.h\n> >> + */\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +using namespace std::literals::chrono_literals;\n> >> +\n> >> +namespace ipa::ipu3::algorithms {\n> >> +\n> >> +LOG_DEFINE_CATEGORY(IPU3Af)\n> >> +\n> >> +Af::Af(): focus_(0), maxVariance_(0.0), currentVar_(0.0)\n> >> +{\n> >> + /* For surface Go 2 back camera VCM */\n> >> + vcmFd_ = open(\"/dev/v4l-subdev8\", O_RDWR);\n> >\n> >\n> > So I have nautilus (Samsung Chromebook v2) which as a UVC camera and a\n> > IPU3 one (with IMX258 sensor). The VCM there is dw9807.\n> >\n> > I tried this patch on nautilus by mapping the v4l-subdevX to dw9807 and\n> > it did work. I can literally hear the VCM move and tries to focus on the\n> > scene. However, there are a few niggles I have noticed:\n> >\n> > The autofocus seems to lock up quite frequently leaving an un-focused\n> > streaming. However if I put up a object close to lens it does tries to\n> > focus it again (then locks up again but works sometimes). I am\n> > deliberately leaving the details out as it's too broad to say anything\n> > specific from nautilus' side.\n>\n> I will let Kate answer in depth obviously. From what I know, the AF\n> needs a grid configuration in IPU3, and the default grid is set to\n> [16*8,16*8] pixels, starting at x=10 and y=2:\n> https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ipu3/ipu3-tables.c#L9587\n\nThank you for providing the grid information. I am trying to set this\nin my v2 patch.\n\n>\n> It means that if your BDS output size is 1280x720 for instance, you will\n> focus on the 10% left part and 17% top part of your scene. Depending on\n> the scene, you might very well be out-of-focus :-).\n\nYes, the top left of the sensor.\n\n>\n> That's why I suggested adding a configure() call to set a grid based on\n> the BDS grid, to make it focus in the center as a default.\n>\n> >\n> > I am interesting in getting better understanding of the values reported,\n> > so once I have a overall reading on the topic, I can start debugging the\n> > process().\n> >\n> >> +}\n> >> +\n> >> +Af::~Af()\n> >> +{\n> >> + if(vcmFd_ != -1)\n> >> + close(vcmFd_);\n> >> +}\n> >> +\n> >> +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> >\n> >\n> > I think [[maybe_unused]] on configInfo as a replacement for the below line\n> >\n> >> +{\n> >> + const IPAConfigInfo tmp __attribute__((unused)) = configInfo;\n> >> + context.frameContext.af.focus = 0;\n> >> + context.frameContext.af.maxVar = 0;\n> >> + context.frameContext.af.stable = false;\n> >> +\n> >> + maxVariance_ = 0;\n> >> + ignoreFrame_ = 100;\n> >> +\n> >> + return 0;\n> >> +}\n> >> +\n> >> +void Af::vcmSet(int value)\n> >> +{\n> >> + int ret;\n> >> + struct v4l2_control ctrl;\n> >> + if(vcmFd_ == -1)\n> >> + return;\n> >\n> >\n> > Maybe we should return -EINVAL?\n> >\n> >> + memset(&ctrl, 0, sizeof(struct v4l2_control));\n> >> + ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;\n> >> + ctrl.value = value;\n> >> + ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);\n> >\n> >\n> > and return ret instead? To check if we really succeeded on setting the\n> > control\n> >\n> >> +\n> >> +}\n> >> +\n> >> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )\n> >> +{\n> >> + typedef struct y_table_item {\n> >> + uint16_t y1_avg;\n> >> + uint16_t y2_avg;\n> >> + }y_table_item_t;\n> >> +\n> >> + uint32_t total = 0;\n> >> + double mean;\n> >> + uint64_t var_sum = 0;\n> >> + y_table_item_t *y_item;\n> >> +\n> >> + y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;\n> >> + int z = 0;\n> >> +\n> >> + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)\n> >> + {\n> >> + total = total + y_item[z].y2_avg;\n> >> + if(y_item[z].y2_avg == 0)\n> >> + break;\n> >> + }\n> >> + mean = total / z;\n> >> +\n> >> + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 &&\n> >> y_item[z].y2_avg != 0; z++)\n> >> + {\n> >> + var_sum = var_sum + ((y_item[z].y2_avg - mean) *\n> >> (y_item[z].y2_avg - mean));\n> >> + if(y_item[z].y2_avg == 0)\n> >> + break;\n> >> + }\n> >> + currentVar_ = static_cast(var_sum) /static_cast(z);\n> >> + LOG(IPU3Af, Debug) << \"variance: \" << currentVar_;\n> >> +\n> >> + if( context.frameContext.af.stable == true )\n> >> + {\n> >> + const uint32_t diff_var = std::abs(currentVar_ -\n> >> context.frameContext.af.maxVar);\n> >> + const double var_ratio = diff_var /\n> >> context.frameContext.af.maxVar;\n> >> + LOG(IPU3Af, Debug) << \"Change ratio: \"\n> >> + << var_ratio\n> >> + << \" current focus: \"\n> >> + << context.frameContext.af.focus;\n> >> + if(var_ratio > 0.8)\n> >\n> >\n> > hmm, I think we should opt for \"variance_\" instead \"var_\" as var in\n> > general has variable meaning, nevermind, details...\n> >\n> >> + {\n> >> + if(ignoreFrame_ == 0)\n> >> + {\n> >> + context.frameContext.af.maxVar = 0;\n> >> + context.frameContext.af.focus = 0;\n> >> + focus_ = 0;\n> >> + context.frameContext.af.stable = false;\n> >> + ignoreFrame_ = 60;\n> >> + }\n> >> + else\n> >> + ignoreFrame_--;\n> >> + }else\n> >> + ignoreFrame_ = 60;\n> >> + }else\n> >> + {\n> >> + if(ignoreFrame_ != 0)\n> >> + ignoreFrame_--;\n> >> + else{\n> >> + if(currentVar_ > context.frameContext.af.maxVar)\n> >> + {\n> >> + context.frameContext.af.maxVar = currentVar_;\n> >> + context.frameContext.af.focus = focus_;\n> >> + }\n> >> +\n> >> + if(focus_ > 1023)\n> >> + {\n> >> + context.frameContext.af.stable = true;\n> >> + vcmSet(context.frameContext.af.focus);\n> >> + LOG(IPU3Af, Debug) << \"Finall Focus \"\n> >> + << context.frameContext.af.focus;\n> >> + }else\n> >> + {\n> >> + focus_ += 5;\n> >> + vcmSet(focus_);\n> >> + }\n> >> + LOG(IPU3Af, Debug) << \"Focus searching max var is: \"\n> >> + << context.frameContext.af.maxVar\n> >> + << \" Focus step is \"\n> >> + << context.frameContext.af.focus;\n> >> + }\n> >> + }\n> >> +}\n> >> +\n> >> +\n> >> +} /* namespace ipa::ipu3::algorithms */\n> >> +\n> >> +} /* namespace libcamera */\n> >> \\ No newline at end of file\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..64c704b1\n> >> --- /dev/null\n> >> +++ b/src/ipa/ipu3/algorithms/af.h\n> >> @@ -0,0 +1,48 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Red Hat\n> >> + *\n> >> + * af.h - IPU3 Af control\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> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::ipu3::algorithms {\n> >> +\n> >> +class Af : public Algorithm\n> >> +{\n> >> +public:\n> >> + Af();\n> >> + ~Af();\n> >> +\n> >> + int configure(IPAContext &context, const IPAConfigInfo\n> >> &configInfo) override;\n> >> + void process(IPAContext &context, const ipu3_uapi_stats_3a\n> >> *stats) override;\n> >> +\n> >> +private:\n> >> + void filterVariance(double new_var);\n> >> +\n> >> + void vcmSet(int value);\n> >> +\n> >> + int vcmFd_;\n> >> + int focus_;\n> >> + unsigned int ignoreFrame_;\n> >\n> >\n> > What's this stand for? Is it ignoring a number of frames?\n> >\n> >> + double maxVariance_;\n> >> + double currentVar_;\n> >> +\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\n> >> b/src/ipa/ipu3/algorithms/meson.build\n> >> index 3ec42f72..d32c61d2 100644\n> >> --- a/src/ipa/ipu3/algorithms/meson.build\n> >> +++ b/src/ipa/ipu3/algorithms/meson.build\n> >> @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([\n> >> 'algorithm.cpp',\n> >> 'awb.cpp',\n> >> 'blc.cpp',\n> >> - 'tone_mapping.cpp',\n> >> + 'af.cpp',\n> >> + 'tone_mapping.cpp'\n> >\n> >\n> > alphabetical order please, I think checkstyle.py will report this as well\n> >\n> >> ])\n> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> >> index 1e46c61a..5d92f63c 100644\n> >> --- a/src/ipa/ipu3/ipa_context.h\n> >> +++ b/src/ipa/ipu3/ipa_context.h\n> >> @@ -47,6 +47,12 @@ struct IPAFrameContext {\n> >> } gains;\n> >> } awb;\n> >> + struct {\n> >> + uint32_t focus;\n> >> + double maxVar;\n> >> + bool stable;\n> >> + } af;\n> >> +\n> >> struct {\n> >> double gamma;\n> >> struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 5c51607d..980815ee 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -31,6 +31,7 @@\n> >> #include \"libcamera/internal/mapped_framebuffer.h\"\n> >> #include \"algorithms/agc.h\"\n> >> +#include \"algorithms/af.h\"\n> >\n> >\n> > Ditto.\n> >\n> >> #include \"algorithms/algorithm.h\"\n> >> #include \"algorithms/awb.h\"\n> >> #include \"algorithms/blc.h\"\n> >> @@ -298,6 +299,7 @@ int IPAIPU3::init(const IPASettings &settings,\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> >>\n> >> algorithms_.push_back(std::make_unique());\n> >>\n>","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 B3EE6BF415\n\tfor ;\n\tFri, 19 Nov 2021 06:16:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27CA560371;\n\tFri, 19 Nov 2021 07:16:50 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9735D60121\n\tfor ;\n\tFri, 19 Nov 2021 07:16:48 +0100 (CET)","from mail-lf1-f72.google.com (mail-lf1-f72.google.com\n\t[209.85.167.72]) 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-429-_Foaj-q7Mvu9BwM1QcCVtA-1; Fri, 19 Nov 2021 01:16:39 -0500","by mail-lf1-f72.google.com with SMTP id\n\tw2-20020a0565120b0200b004036bc9597eso5804316lfu.14\n\tfor ;\n\tThu, 18 Nov 2021 22:16:38 -0800 (PST)"],"Authentication-Results":["lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"XFSPaavj\"; 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=1637302607;\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=C9BFHSX5hnauVQ34hrNrNez2P8+rG9mjMh9R4pxLxgc=;\n\tb=XFSPaavjJoq7+CQpNH9KHW2SSYjbkJ73IWv655kplpLCb6QPH/ZUiBmOlArsD0lKbeVd3j\n\t+V2y38HDsDa0tnNCGe/pGK8enW9ogDg1qlRV2u5PjfP9pdfJDP0aiZ5WGbCwB51+WA9r8Y\n\ttNyU6Y+6sohYPubhfaiqPqrZajdE2dc=","X-MC-Unique":"_Foaj-q7Mvu9BwM1QcCVtA-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=C9BFHSX5hnauVQ34hrNrNez2P8+rG9mjMh9R4pxLxgc=;\n\tb=a8u05jDiux0jMuAnLSNUGMyzUU8jwTlHjzIstzL4pW5GzJq4LhpnYkPiyImWuf++6/\n\t2+c5eUlhJpRVbVDAd8O6vI8edZOQ8jacxCnRrOLiaIPyPl6lwr27TS/IwvKZLweBITop\n\t7bPqy90DapwMQMMgY09jBPVbSgDMs+dC419+A2tDVZgemHuyAP3l/DOPJOIr6HqV4d+I\n\t1sXiSQDz2EG7NTxEBlgDnCU7QynE2f1qJxrz8zHgj5/VK3c0Wk7hEZ+L7Zo6Jd6VKXCP\n\tMUK+gNtO5QBupT8FFuOcIPz9LFghH/ohRe+iVfRTqPSGKO43x+nTkkLzudw6lBhV9m5k\n\tmNDw==","X-Gm-Message-State":"AOAM530HyF1hIqPDS0Os184NBdJ3SMYroqV6oNXuVWATogcr+CprXiek\n\tPwMZxT0Na62YzqbK9WQ10fehgmJa+rsUmJ4Xtuc8k/0MPqXx0VuOqiACfF0gvZzV0tVmvO1PGyE\n\t3CBrJ9+uUhMkT8PwXbDhiIXSoet43LuPtC4I6L5JAMjxMIymeVg==","X-Received":["by 2002:a2e:a78e:: with SMTP id\n\tc14mr23659850ljf.162.1637302597211; \n\tThu, 18 Nov 2021 22:16:37 -0800 (PST)","by 2002:a2e:a78e:: with SMTP id\n\tc14mr23659815ljf.162.1637302596797; \n\tThu, 18 Nov 2021 22:16:36 -0800 (PST)"],"X-Google-Smtp-Source":"ABdhPJwCqEmd7P2P9qyxITwEo4dXTaL3hgl11jQKLyrZlTpCbiPOLHxlOKKHR1JBn20fCR4DtnWE1tMmVpQhTKy+n0w=","MIME-Version":"1.0","References":"<20211115054400.17797-1-hpa@redhat.com>\n\t<860bb149-df92-ecca-5434-1c9363a2fc2e@ideasonboard.com>\n\t<3e4d378c-fe8c-22f8-c620-2817767d0426@ideasonboard.com>","In-Reply-To":"<3e4d378c-fe8c-22f8-c620-2817767d0426@ideasonboard.com>","From":"Kate Hsuan ","Date":"Fri, 19 Nov 2021 14:16:25 +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] [PATCH] 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\" "}}]