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

2035 diff mbox series

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
and informing the camera of the status of the AE control loop.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/ipa/ipa_rkisp1.cpp | 228 +++++++++++++++++++++++++++++++++++++++++
 src/ipa/meson.build    |  13 +++
 2 files changed, 241 insertions(+)
 create mode 100644 src/ipa/ipa_rkisp1.cpp

Hi Niklas,

 I would change the subject to point out you're adding the IPA module
and not extending it with one more control

On Fri, Sep 27, 2019 at 04:44:16AM +0200, 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
> and informing the camera of the status of the AE control loop.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/ipa/ipa_rkisp1.cpp | 228 +++++++++++++++++++++++++++++++++++++++++
>  src/ipa/meson.build    |  13 +++
>  2 files changed, 241 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..f90465516c6aff87
> --- /dev/null
> +++ b/src/ipa/ipa_rkisp1.cpp
> @@ -0,0 +1,228 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_rkisp1.cpp - RkISP1 Image Processing Algorithms

The RkISP name is very unfortunate. I miss-spell it to RKISP all the
times. Nothing related to this patch though.

> + */
> +
> +#include <algorithm>
> +#include <math.h>
> +#include <queue>
> +#include <string.h>
> +
> +#include <linux/rkisp1-config.h>
> +
> +#include <ipa/ipa_interface.h>
> +#include <ipa/ipa_module_info.h>
> +#include <libcamera/buffer.h>
> +#include <libcamera/request.h>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +#define BUFFER_PARAM 1
> +#define BUFFER_STAT 2

I would prefix them with RKISP1

> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPARkISP1)
> +
> +class IPARkISP1 : public IPAInterface
> +{
> +public:
> +	int init() override { return 0; }
> +
> +	void initSensor(const V4L2ControlInfoMap &controls) override;
> +	void initBuffers(unsigned int type,
> +			 const std::vector<BufferMemory> &buffers) override;
> +	void signalBuffer(unsigned int type, unsigned int id) override;
> +	void queueRequest(unsigned int frame, const ControlList &controls) override;
> +
> +private:
> +	void setControls(unsigned int frame);
> +	void updateStatistics(unsigned int frame, BufferMemory &statistics);
> +
> +	std::map<unsigned int, std::map<unsigned int, BufferMemory>> bufferInfo_;
> +	std::map<unsigned int, std::map<unsigned int, unsigned int>> bufferFrame_;
> +	std::map<unsigned int, std::queue<unsigned int>> bufferFree_;
> +
> +	/* Camera sensor controls. */
> +	bool autoExposure_;
> +	uint64_t exposure_;
> +	uint64_t minExposure_;
> +	uint64_t maxExposure_;
> +	uint64_t gain_;
> +	uint64_t minGain_;
> +	uint64_t maxGain_;
> +};
> +
> +void IPARkISP1::initSensor(const V4L2ControlInfoMap &controls)
> +{
> +	const auto itExp = controls.find(V4L2_CID_EXPOSURE);
> +	if (itExp == controls.end()) {
> +		LOG(IPARkISP1, Error) << "Can't find exposure control";
> +		return;
> +	}
> +
> +	const auto itGain = controls.find(V4L2_CID_ANALOGUE_GAIN);
> +	if (itGain == controls.end()) {
> +		LOG(IPARkISP1, Error) << "Can't find gain control";
> +		return;
> +	}
> +
> +	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_;

We need a default for v4l2 controls otherwise you would initialize all
to min. Or at least the current value reported by the kernel.

I now wonder if the -supported- V4L2Controls and Controls should not
be passed at init() and here we should receive a V4L2ControlList which
contains default, gets processed by the IPA (if it wants to) and sent
back with setControls()

> +
> +	LOG(IPARkISP1, Info)
> +		<< "Exposure: " << minExposure_ << "-" << maxExposure_
> +		<< " Gain: " << minGain_ << "-" << maxGain_;
> +
> +	setControls(0);
> +}
> +
> +void IPARkISP1::initBuffers(unsigned int type,
> +			    const std::vector<BufferMemory> &buffers)
> +{
> +	bufferInfo_[type].clear();
> +	for (unsigned int i = 0; i < buffers.size(); i++) {
> +		bufferInfo_[type][i] = buffers[i];
> +		bufferFree_[type].push(i);
> +	}

As I said, I fear this will get soon unmaintainable. There a lot of
bookeeping and the types used to do so are two/three level maps. I know,
I had the same comment for IPU3 and I was like "what?? I'm maintaining it,
it's super easy and works". It soon fel into pieces as soon as the use case
changed slightly.

To avoid going down the same path I would consider (not for this first
version, but something to work on after) to serialize Pools and add
there methods to pull a buffer (remove it from the list of usable
ones) and push it back to the pool (make it available again). Could we
consider this during the Buffer rework effert we've been planning for
some time?

> +}
> +
> +void IPARkISP1::signalBuffer(unsigned int type, unsigned int id)
> +{
> +	if (type == BUFFER_STAT) {
> +		unsigned int frame = bufferFrame_[type][id];
> +		BufferMemory &mem = bufferInfo_[type][id];
> +		updateStatistics(frame, mem);
> +	}
> +
> +	bufferFree_[type].push(id);
> +}
> +
> +void IPARkISP1::queueRequest(unsigned int frame, const ControlList &controls)
> +{
> +	/* Find buffers. */
> +	if (bufferFree_[BUFFER_PARAM].empty()) {
> +		LOG(IPARkISP1, Error) << "Param buffer underrun";
> +		return;
> +	}
> +
> +	if (bufferFree_[BUFFER_STAT].empty()) {
> +		LOG(IPARkISP1, Error) << "Statistics buffer underrun";
> +		return;
> +	}
> +
> +	unsigned int paramid = bufferFree_[BUFFER_PARAM].front();
> +	bufferFree_[BUFFER_PARAM].pop();
> +	unsigned int statid = bufferFree_[BUFFER_STAT].front();
> +	bufferFree_[BUFFER_STAT].pop();
> +
> +	bufferFrame_[BUFFER_PARAM][paramid] = frame;
> +	bufferFrame_[BUFFER_STAT][statid] = frame;
> +
> +	/* Prepare parameters buffer. */
> +	BufferMemory &mem = bufferInfo_[BUFFER_PARAM][paramid];
> +	rkisp1_isp_params_cfg *params =
> +		static_cast<rkisp1_isp_params_cfg *>(mem.planes()[0].mem());
> +
> +	memset(params, 0, sizeof(*params));
> +
> +	/* Auto Exposure on/off. */
> +	if (controls.contains(AeEnable)) {
> +		autoExposure_ = controls[AeEnable].getBool();
> +		if (autoExposure_)
> +			params->module_ens = CIFISP_MODULE_AEC;
> +
> +		params->module_en_update = CIFISP_MODULE_AEC;
> +	}
> +
> +	/* Queue buffers to pipeline. */
> +	queueBuffer.emit(frame, BUFFER_PARAM, paramid);
> +	queueBuffer.emit(frame, BUFFER_STAT, statid);

Here I am missing a piece. Not easy to comment as the 'issue' is in
the pipeline handler queueRequest implementation and in the timeline
and here, but: shouldn't we have dependecy tracking somehow ? The
queueRequest in the pipeline handler schedules queueing of the buffer
to the video capture node after this two signals have been emitted and
the two corresponding actions scheduled on the timeline. What
guarantees the buffer that is used to capture images is queued -after-
the associated ISP parameters ?

> +}
> +
> +void IPARkISP1::setControls(unsigned int frame)
> +{
> +	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_);

you could
        ctrls.add(V4L2_CID..., value_)

> +
> +	updateSensor.emit(frame, ctrls);
> +}
> +
> +void IPARkISP1::updateStatistics(unsigned int frame, BufferMemory &statistics)
> +{
> +	const rkisp1_stat_buffer *stats =
> +		static_cast<rkisp1_stat_buffer *>(statistics.planes()[0].mem());
> +	const cifisp_stat *params = &stats->params;
> +	IPAMetaData metaData = {};
> +
> +	if (stats->meas_type & CIFISP_STAT_AUTOEXP) {
> +		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;
> +
> +		if (frame % 3 == 0) {
> +			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(frame + 1);
> +		}
> +
> +		metaData.aeState = fabs(factor - 1.0f) < 0.05f ?
> +			AeState::Converged : AeState::Searching;
> +	} else {
> +		metaData.aeState = AeState::Inactive;
> +	}
> +
> +	metaDataReady.emit(frame, metaData);
> +}
> +
> +/*
> + * 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 10448c2ffc76af4b..eb5b852eec282735 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -3,6 +3,10 @@ ipa_dummy_sources = [
>      ['ipa_dummy_isolate', 'Proprietary'],
>  ]
>
> +ipa_sources = [
> +    ['ipa_rkisp1', 'ipa_rkisp1.cpp', 'LGPL-2.1-or-later'],
> +]
> +
>  ipa_install_dir = join_paths(get_option('libdir'), 'libcamera')
>
>  foreach t : ipa_dummy_sources
> @@ -14,5 +18,14 @@ foreach t : ipa_dummy_sources
>                          cpp_args : '-DLICENSE="' + t[1] + '"')
>  endforeach
>
> +foreach t : ipa_sources
> +    ipa = shared_module(t[0], t[1],
> +                        name_prefix : '',
> +                        include_directories : includes,
> +                        install : true,
> +                        install_dir : ipa_install_dir,
> +                        cpp_args : '-DLICENSE="' + t[2] + '"')
> +endforeach
> +
>  config_h.set('IPA_MODULE_DIR',
>               '"' + join_paths(get_option('prefix'), ipa_install_dir) + '"')
> --
> 2.23.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel