[libcamera-devel] [PATCH] ipa: ipu3: af: Auto focus for dw9719 Surface Go2 VCM
Kate Hsuan
hpa at redhat.com
Fri Nov 19 07:16:25 CET 2021
Hi Jean-Michel,
On Thu, Nov 18, 2021 at 8:43 PM Jean-Michel Hautbois
<jeanmichel.hautbois at ideasonboard.com> 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
Thank you for providing the grid information. I am trying to set this
in my v2 patch.
>
> 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 :-).
Yes, the top left of the sensor.
>
> 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>());
> >>
>
--
BR,
Kate
More information about the libcamera-devel
mailing list