[libcamera-devel] [PATCH] ipa: ipu3: af: Auto focus for dw9719 Surface Go2 VCM
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Thu Nov 18 13:42:55 CET 2021
Hi Umang,
On 18/11/2021 13:31, Umang Jain wrote:
> Hi Kate,
>
> Thank you for you work on this.
>
> While I understand this is an MVP (as posted by you in one of the other
> autofocus threads) here are some of my high-level queries and
> understanding. The interface to handle autofocus (and focus controls) in
> general is missing from libcamera, so I understand the current
> limitations as well, to move this forward.
>
> On 11/15/21 11:14 AM, Kate Hsuan wrote:
>> Since VCM for surface Go 2 (dw9719) had been successfully
>> driven, this Af module can be used to control the VCM and
>> determine the focus value based on the IPU3 AF state.
>>
>> The variance of each focus step is determined and a greedy
>> approah is used to find the maximum variance of the AF
>> state and a appropriate focus value.
>
>
> Can you please suggest a high level reading of finding the focus on the
> image? I don't understand the implementation / algorithm for the
> focus-sweep implemented in process()?
>
>>
>> Signed-off-by: Kate Hsuan <hpa at redhat.com>
>> ---
>> src/ipa/ipu3/algorithms/af.cpp | 165 ++++++++++++++++++++++++++++
>> src/ipa/ipu3/algorithms/af.h | 48 ++++++++
>> src/ipa/ipu3/algorithms/meson.build | 3 +-
>> src/ipa/ipu3/ipa_context.h | 6 +
>> src/ipa/ipu3/ipu3.cpp | 2 +
>> 5 files changed, 223 insertions(+), 1 deletion(-)
>> create mode 100644 src/ipa/ipu3/algorithms/af.cpp
>> create mode 100644 src/ipa/ipu3/algorithms/af.h
>>
>> diff --git a/src/ipa/ipu3/algorithms/af.cpp
>> b/src/ipa/ipu3/algorithms/af.cpp
>> new file mode 100644
>> index 00000000..c276b539
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/af.cpp
>
>
> I won't get into style issues here (as don't seem relevant for now), but
> for your information we do have a style checker at:
>
> $libcamera/utils/checkstyle.py <commit-id-of-the-patch>
>
>> @@ -0,0 +1,165 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Red Hat
>> + *
>> + * af.cpp - IPU3 Af control
>
>
> would be better to expand on the acronym "IPU3 Auto Focus control" maybe
>
>> + */
>> +
>> +#include "af.h"
>> +
>> +#include <algorithm>
>> +#include <chrono>
>> +#include <cmath>
>> +#include <numeric>
>> +
>> +#include <linux/videodev2.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +#include <sys/ioctl.h>
>> +#include <unistd.h>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +#include "libipa/histogram.h"
>> +
>> +/**
>> + * \file af.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +using namespace std::literals::chrono_literals;
>> +
>> +namespace ipa::ipu3::algorithms {
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Af)
>> +
>> +Af::Af(): focus_(0), maxVariance_(0.0), currentVar_(0.0)
>> +{
>> + /* For surface Go 2 back camera VCM */
>> + vcmFd_ = open("/dev/v4l-subdev8", O_RDWR);
>
>
> So I have nautilus (Samsung Chromebook v2) which as a UVC camera and a
> IPU3 one (with IMX258 sensor). The VCM there is dw9807.
>
> I tried this patch on nautilus by mapping the v4l-subdevX to dw9807 and
> it did work. I can literally hear the VCM move and tries to focus on the
> scene. However, there are a few niggles I have noticed:
>
> The autofocus seems to lock up quite frequently leaving an un-focused
> streaming. However if I put up a object close to lens it does tries to
> focus it again (then locks up again but works sometimes). I am
> deliberately leaving the details out as it's too broad to say anything
> specific from nautilus' side.
I will let Kate answer in depth obviously. From what I know, the AF
needs a grid configuration in IPU3, and the default grid is set to
[16*8,16*8] pixels, starting at x=10 and y=2:
https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ipu3/ipu3-tables.c#L9587
It means that if your BDS output size is 1280x720 for instance, you will
focus on the 10% left part and 17% top part of your scene. Depending on
the scene, you might very well be out-of-focus :-).
That's why I suggested adding a configure() call to set a grid based on
the BDS grid, to make it focus in the center as a default.
>
> I am interesting in getting better understanding of the values reported,
> so once I have a overall reading on the topic, I can start debugging the
> process().
>
>> +}
>> +
>> +Af::~Af()
>> +{
>> + if(vcmFd_ != -1)
>> + close(vcmFd_);
>> +}
>> +
>> +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>
>
> I think [[maybe_unused]] on configInfo as a replacement for the below line
>
>> +{
>> + const IPAConfigInfo tmp __attribute__((unused)) = configInfo;
>> + context.frameContext.af.focus = 0;
>> + context.frameContext.af.maxVar = 0;
>> + context.frameContext.af.stable = false;
>> +
>> + maxVariance_ = 0;
>> + ignoreFrame_ = 100;
>> +
>> + return 0;
>> +}
>> +
>> +void Af::vcmSet(int value)
>> +{
>> + int ret;
>> + struct v4l2_control ctrl;
>> + if(vcmFd_ == -1)
>> + return;
>
>
> Maybe we should return -EINVAL?
>
>> + memset(&ctrl, 0, sizeof(struct v4l2_control));
>> + ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;
>> + ctrl.value = value;
>> + ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);
>
>
> and return ret instead? To check if we really succeeded on setting the
> control
>
>> +
>> +}
>> +
>> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats )
>> +{
>> + typedef struct y_table_item {
>> + uint16_t y1_avg;
>> + uint16_t y2_avg;
>> + }y_table_item_t;
>> +
>> + uint32_t total = 0;
>> + double mean;
>> + uint64_t var_sum = 0;
>> + y_table_item_t *y_item;
>> +
>> + y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;
>> + int z = 0;
>> +
>> + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4; z++)
>> + {
>> + total = total + y_item[z].y2_avg;
>> + if(y_item[z].y2_avg == 0)
>> + break;
>> + }
>> + mean = total / z;
>> +
>> + for(z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE)/4 &&
>> y_item[z].y2_avg != 0; z++)
>> + {
>> + var_sum = var_sum + ((y_item[z].y2_avg - mean) *
>> (y_item[z].y2_avg - mean));
>> + if(y_item[z].y2_avg == 0)
>> + break;
>> + }
>> + currentVar_ = static_cast<double>(var_sum) /static_cast<double>(z);
>> + LOG(IPU3Af, Debug) << "variance: " << currentVar_;
>> +
>> + if( context.frameContext.af.stable == true )
>> + {
>> + const uint32_t diff_var = std::abs(currentVar_ -
>> context.frameContext.af.maxVar);
>> + const double var_ratio = diff_var /
>> context.frameContext.af.maxVar;
>> + LOG(IPU3Af, Debug) << "Change ratio: "
>> + << var_ratio
>> + << " current focus: "
>> + << context.frameContext.af.focus;
>> + if(var_ratio > 0.8)
>
>
> hmm, I think we should opt for "variance_" instead "var_" as var in
> general has variable meaning, nevermind, details...
>
>> + {
>> + if(ignoreFrame_ == 0)
>> + {
>> + context.frameContext.af.maxVar = 0;
>> + context.frameContext.af.focus = 0;
>> + focus_ = 0;
>> + context.frameContext.af.stable = false;
>> + ignoreFrame_ = 60;
>> + }
>> + else
>> + ignoreFrame_--;
>> + }else
>> + ignoreFrame_ = 60;
>> + }else
>> + {
>> + if(ignoreFrame_ != 0)
>> + ignoreFrame_--;
>> + else{
>> + if(currentVar_ > context.frameContext.af.maxVar)
>> + {
>> + context.frameContext.af.maxVar = currentVar_;
>> + context.frameContext.af.focus = focus_;
>> + }
>> +
>> + if(focus_ > 1023)
>> + {
>> + context.frameContext.af.stable = true;
>> + vcmSet(context.frameContext.af.focus);
>> + LOG(IPU3Af, Debug) << "Finall Focus "
>> + << context.frameContext.af.focus;
>> + }else
>> + {
>> + focus_ += 5;
>> + vcmSet(focus_);
>> + }
>> + LOG(IPU3Af, Debug) << "Focus searching max var is: "
>> + << context.frameContext.af.maxVar
>> + << " Focus step is "
>> + << context.frameContext.af.focus;
>> + }
>> + }
>> +}
>> +
>> +
>> +} /* namespace ipa::ipu3::algorithms */
>> +
>> +} /* namespace libcamera */
>> \ No newline at end of file
>> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
>> new file mode 100644
>> index 00000000..64c704b1
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/af.h
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Red Hat
>> + *
>> + * af.h - IPU3 Af control
>> + */
>> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
>> +#define __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
>> +
>> +#include <linux/intel-ipu3.h>
>> +
>> +#include <libcamera/base/utils.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::ipu3::algorithms {
>> +
>> +class Af : public Algorithm
>> +{
>> +public:
>> + Af();
>> + ~Af();
>> +
>> + int configure(IPAContext &context, const IPAConfigInfo
>> &configInfo) override;
>> + void process(IPAContext &context, const ipu3_uapi_stats_3a
>> *stats) override;
>> +
>> +private:
>> + void filterVariance(double new_var);
>> +
>> + void vcmSet(int value);
>> +
>> + int vcmFd_;
>> + int focus_;
>> + unsigned int ignoreFrame_;
>
>
> What's this stand for? Is it ignoring a number of frames?
>
>> + double maxVariance_;
>> + double currentVar_;
>> +
>> +};
>> +
>> +} /* namespace ipa::ipu3::algorithms */
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AF_H__ */
>> diff --git a/src/ipa/ipu3/algorithms/meson.build
>> b/src/ipa/ipu3/algorithms/meson.build
>> index 3ec42f72..d32c61d2 100644
>> --- a/src/ipa/ipu3/algorithms/meson.build
>> +++ b/src/ipa/ipu3/algorithms/meson.build
>> @@ -5,5 +5,6 @@ ipu3_ipa_algorithms = files([
>> 'algorithm.cpp',
>> 'awb.cpp',
>> 'blc.cpp',
>> - 'tone_mapping.cpp',
>> + 'af.cpp',
>> + 'tone_mapping.cpp'
>
>
> alphabetical order please, I think checkstyle.py will report this as well
>
>> ])
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 1e46c61a..5d92f63c 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -47,6 +47,12 @@ struct IPAFrameContext {
>> } gains;
>> } awb;
>> + struct {
>> + uint32_t focus;
>> + double maxVar;
>> + bool stable;
>> + } af;
>> +
>> struct {
>> double gamma;
>> struct ipu3_uapi_gamma_corr_lut gammaCorrection;
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 5c51607d..980815ee 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -31,6 +31,7 @@
>> #include "libcamera/internal/mapped_framebuffer.h"
>> #include "algorithms/agc.h"
>> +#include "algorithms/af.h"
>
>
> Ditto.
>
>> #include "algorithms/algorithm.h"
>> #include "algorithms/awb.h"
>> #include "algorithms/blc.h"
>> @@ -298,6 +299,7 @@ int IPAIPU3::init(const IPASettings &settings,
>> }
>> /* Construct our Algorithms */
>> + algorithms_.push_back(std::make_unique<algorithms::Af>());
>> algorithms_.push_back(std::make_unique<algorithms::Agc>());
>> algorithms_.push_back(std::make_unique<algorithms::Awb>());
>>
>> algorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());
>>
More information about the libcamera-devel
mailing list