[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:53:19 CET 2021
On 18/11/2021 13:42, Jean-Michel Hautbois wrote:
> 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.
>
Be aware that AF grid can't always be the same as AWB grid, as the
limits are not the same:
https://lore.kernel.org/linux-media/1634525295-1410-1-git-send-email-bingbu.cao@intel.com/
And I can see in CrOS that the log2 may not be 7 but 6:
https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/af_public.h#32
>>
>> 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