[libcamera-devel] [PATCH] ipa: ipu3: af: Auto focus for dw9719 Surface Go2 VCM

[Umang Jain]

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 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>());