[libcamera-devel] [PATCH 12/13] libcamera: ipa: rkisp1: Add basic control of auto exposure

[Kieran Bingham]

Hi Niklas,

On 28/08/2019 02:17, Niklas Söderlund wrote:
> Add an IPA which controls the exposure time and analog gain for a sensor
> connected to the rkisp1 pipeline. The IPA supports turning AE on and off
> but lacks the support to inform the camera of the status of the AE
> control loop.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/ipa/ipa_rkisp1.cpp | 165 +++++++++++++++++++++++++++++++++++++++++
>  src/ipa/meson.build    |   1 +
>  2 files changed, 166 insertions(+)
>  create mode 100644 src/ipa/ipa_rkisp1.cpp
> 
> diff --git a/src/ipa/ipa_rkisp1.cpp b/src/ipa/ipa_rkisp1.cpp
> new file mode 100644
> index 0000000000000000..950efb244cfe7879
> --- /dev/null
> +++ b/src/ipa/ipa_rkisp1.cpp
> @@ -0,0 +1,165 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_rkisp1.cpp - RkISP1 Image Processing Algorithms
> + */
> +
> +#include <algorithm>
> +#include <string.h>
> +
> +#include <linux/rkisp1-config.h>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
> +#include <libcamera/request.h>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPARkISP1)
> +
> +class IPARkISP1 : public IPAInterface
> +{
> +public:
> +	int initSensor(const V4L2ControlInfoMap &controls) override;
> +	void processRequest(const void *cookie, const ControlList &controls,
> +			    Buffer &parameters) override;
> +	void updateStatistics(const void *cookie, Buffer &statistics) override;
> +
> +private:
> +	void setControls();
> +
> +	uint64_t statFrame_;
> +
> +	/* Camera sensor controls. */
> +	bool autoExposure_;
> +	uint64_t exposure_;
> +	uint64_t minExposure_;
> +	uint64_t maxExposure_;
> +	uint64_t gain_;
> +	uint64_t minGain_;
> +	uint64_t maxGain_;

I don't think you need to store the min/max value of each. You should
instead store a reference to the relevant ControlInfo for the control,
which will be populated with the min/max.

And perhaps all of those would be stored in a single ControlList ...


> +};
> +
> +int IPARkISP1::initSensor(const V4L2ControlInfoMap &controls)
> +{
> +	statFrame_ = 0;
> +
> +	const auto itExp = controls.find(V4L2_CID_EXPOSURE);
> +	if (itExp == controls.end())
> +		return -ENODEV;
> +
> +	const auto itGain = controls.find(V4L2_CID_ANALOGUE_GAIN);
> +	if (itGain == controls.end())
> +		return -ENODEV;
> +
> +	autoExposure_ = true;
> +
> +	minExposure_ = std::max<uint64_t>(itExp->second.min(), 1);
> +	maxExposure_ = itExp->second.max();
> +	exposure_ = minExposure_;
> +
> +	minGain_ = std::max<uint64_t>(itGain->second.min(), 1);
> +	maxGain_ = itGain->second.max();
> +	gain_ = minGain_;
> +
> +	LOG(IPARkISP1, Info)
> +		<< "Exposure: " << minExposure_ << "-" << maxExposure_
> +		<< " Gain: " << minGain_ << "-" << maxGain_;
> +
> +	setControls();
> +
> +	return 0;
> +}
> +
> +void IPARkISP1::setControls()
> +{
> +	V4L2ControlList ctrls;
> +	ctrls.add(V4L2_CID_EXPOSURE);
> +	ctrls.add(V4L2_CID_ANALOGUE_GAIN);
> +	ctrls[V4L2_CID_EXPOSURE]->setValue(exposure_);
> +	ctrls[V4L2_CID_ANALOGUE_GAIN]->setValue(gain_);
> +
> +	updateSensor.emit(ctrls);
> +}
> +
> +void IPARkISP1::processRequest(const void *cookie, const ControlList &controls,
> +			       Buffer &parameters)

Are you preventing the Request being passed in directly here to stop
direct access to the Request object? If so - why not pass it in as a const?

Are you perhaps trying to optimise this for serialisation already?

> +{
> +	rkisp1_isp_params_cfg *params =
> +		static_cast<rkisp1_isp_params_cfg *>(parameters.mem()->planes()[0].mem());
> +
> +	memset(params, 0, sizeof(*params));
> +
> +	/* Auto Exposure on/off*/

s#off*/#off. */#

> +	if (controls.contains(AeEnable)) {
> +		autoExposure_ = controls[AeEnable].getBool();
> +		if (autoExposure_)
> +			params->module_ens = CIFISP_MODULE_AEC;
> +
> +		params->module_en_update = CIFISP_MODULE_AEC;
> +	}
> +
> +	queueRequest.emit(cookie);
> +}
> +
> +void IPARkISP1::updateStatistics(const void *cookie, Buffer &statistics)
> +{
> +	const rkisp1_stat_buffer *stats =
> +		static_cast<rkisp1_stat_buffer *>(statistics.mem()->planes()[0].mem());
> +	const cifisp_stat *params = &stats->params;
> +
> +	if ((stats->meas_type & CIFISP_STAT_AUTOEXP) && (statFrame_ % 2 == 0)) {
> +		const cifisp_ae_stat *ae = &params->ae;
> +
> +		const unsigned int target = 60;
> +
> +		unsigned int value = 0;
> +		unsigned int num = 0;
> +		for (int i = 0; i < CIFISP_AE_MEAN_MAX; i++) {
> +			if (ae->exp_mean[i] > 15) {
> +				value += ae->exp_mean[i];
> +				num++;
> +			}
> +		}
> +		value /= num;
> +
> +		double factor = (double)target / value;
> +		double tmp;
> +
> +		tmp = factor * exposure_ * gain_ / minGain_;
> +		exposure_ = utils::clamp<uint64_t>((uint64_t)tmp, minExposure_, maxExposure_);
> +
> +		tmp = tmp / exposure_ * minGain_;
> +		gain_ = utils::clamp<uint64_t>((uint64_t)tmp, minGain_, maxGain_);
> +
> +		setControls();
> +	}
> +
> +	statFrame_++;
> +}

I think this would update the local ControlList, which would at the end
be submitted, and reused.

I'll ignore all the calculations for now, and assume they do 'something'
useful :D


> +
> +/*
> + * External IPA module interface
> + */
> +
> +extern "C" {
> +const struct IPAModuleInfo ipaModuleInfo = {
> +	IPA_MODULE_API_VERSION,
> +	1,
> +	"PipelineHandlerRkISP1",
> +	"RkISP1 IPA",
> +	"LGPL-2.1-or-later",
> +};
> +
> +IPAInterface *ipaCreate()
> +{
> +	return new IPARkISP1();
> +}
> +};
> +
> +}; /* namespace libcamera */
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> index dca7a9461385b68d..16592a71e03990ce 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -1,6 +1,7 @@
>  ipa_dummy_sources = [
>      ['ipa_dummy', 'ipa_dummy.cpp'],
>      ['ipa_dummy_isolate', 'ipa_dummy_isolate.cpp'],
> +    ['ipa_rkisp1', 'ipa_rkisp1.cpp'],

As mentioned in an earlier patch, I don't think this is an
'ipa_dummy_source' file, but I do think this list can be re-used.

It just needs renaming first.


>  ]
>  
>  ipa_install_dir = join_paths(get_option('libdir'), 'libcamera')
> 

-- 
Regards
--
Kieran