[libcamera-devel,v6,8/9] libcamera: ipa: rkisp1: Add basic control of auto exposure

2157 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>
---
 include/ipa/rkisp1.h       |  18 +++
 src/ipa/meson.build        |   2 +
 src/ipa/rkisp1/meson.build |   6 +
 src/ipa/rkisp1/rkisp1.cpp  | 264 +++++++++++++++++++++++++++++++++++++
 4 files changed, 290 insertions(+)
 create mode 100644 include/ipa/rkisp1.h
 create mode 100644 src/ipa/rkisp1/meson.build
 create mode 100644 src/ipa/rkisp1/rkisp1.cpp

Hi Niklas,

Thank you for the patch.

On Fri, Oct 11, 2019 at 05:22:15AM +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>
> ---
>  include/ipa/rkisp1.h       |  18 +++
>  src/ipa/meson.build        |   2 +
>  src/ipa/rkisp1/meson.build |   6 +
>  src/ipa/rkisp1/rkisp1.cpp  | 264 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 290 insertions(+)
>  create mode 100644 include/ipa/rkisp1.h
>  create mode 100644 src/ipa/rkisp1/meson.build
>  create mode 100644 src/ipa/rkisp1/rkisp1.cpp
> 
> diff --git a/include/ipa/rkisp1.h b/include/ipa/rkisp1.h
> new file mode 100644
> index 0000000000000000..4fe0482b8de51419
> --- /dev/null
> +++ b/include/ipa/rkisp1.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * rkisp1.h - Image Processing Algorithm interface for RkISP1
> + */
> +#ifndef __LIBCAMERA_IPA_INTERFACE_RKISP1_H__
> +#define __LIBCAMERA_IPA_INTERFACE_RKISP1_H__
> +
> +enum RkISP1Operations {
> +	RKISP1_IPA_ACTION_V4L2_SET = 1,
> +	RKISP1_IPA_ACTION_PARAM_FILLED = 2,
> +	RKISP1_IPA_ACTION_METADATA = 3,
> +	RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER = 4,
> +	RKISP1_IPA_EVENT_QUEUE_REQUEST = 5,
> +};
> +
> +#endif /* __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ */
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> index ac16e1da6126bb2a..4f2a457712016d53 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -22,3 +22,5 @@ endforeach
>  
>  config_h.set('IPA_MODULE_DIR',
>               '"' + join_paths(get_option('prefix'), ipa_install_dir) + '"')
> +
> +subdir('rkisp1')
> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> new file mode 100644
> index 0000000000000000..c2b1d228555f0636
> --- /dev/null
> +++ b/src/ipa/rkisp1/meson.build
> @@ -0,0 +1,6 @@
> +rkisp1_ipa = shared_module('ipa_rkisp1',
> +                           'rkisp1.cpp',
> +                           name_prefix : '',
> +                           include_directories : ipa_includes,
> +                           install : true,
> +                           install_dir : ipa_install_dir)

I think you need to link this to libcamera.so.

> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> new file mode 100644
> index 0000000000000000..6510ddf14353c554
> --- /dev/null
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -0,0 +1,264 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * rkisp1.cpp - RkISP1 Image Processing Algorithms
> + */
> +
> +#include <algorithm>
> +#include <cstdint>
> +#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 <ipa/rkisp1.h>
> +#include <libcamera/buffer.h>
> +#include <libcamera/control_ids.h>
> +#include <libcamera/request.h>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPARkISP1)
> +
> +class IPARkISP1 : public IPAInterface
> +{
> +public:
> +	int init() override { return 0; }
> +
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override;
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> +	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> +	void processEvent(const IPAOperationData &event) override;
> +
> +private:
> +	void queueRequest(unsigned int frame, unsigned int paramid,
> +			  rkisp1_isp_params_cfg *params,
> +			  const ControlList &controls);
> +	void updateStatistics(unsigned int frame,
> +			      const rkisp1_stat_buffer *stats);
> +
> +	void setControls(unsigned int frame);
> +	void queueBuffer(unsigned int frame, unsigned int id);
> +	void metadataReady(unsigned int frame, unsigned int aeState);
> +
> +	std::map<unsigned int, BufferMemory> bufferInfo_;
> +
> +	/* Camera sensor controls. */
> +	bool autoExposure_;
> +	uint32_t exposure_;
> +	uint32_t minExposure_;
> +	uint32_t maxExposure_;
> +	uint32_t gain_;
> +	uint32_t minGain_;
> +	uint32_t maxGain_;
> +};
> +
> +void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +			  const std::map<unsigned int, V4L2ControlInfoMap> &entityControls)
> +{
> +	if (entityControls.empty())
> +		return;
> +
> +	const V4L2ControlInfoMap &ctrls = entityControls.at(0);
> +
> +	const auto itExp = ctrls.find(V4L2_CID_EXPOSURE);
> +	if (itExp == ctrls.end()) {
> +		LOG(IPARkISP1, Error) << "Can't find exposure control";
> +		return;
> +	}
> +
> +	const auto itGain = ctrls.find(V4L2_CID_ANALOGUE_GAIN);
> +	if (itGain == ctrls.end()) {
> +		LOG(IPARkISP1, Error) << "Can't find gain control";
> +		return;
> +	}
> +
> +	autoExposure_ = true;
> +
> +	minExposure_ = std::max<uint64_t>(itExp->second.range().min().get<int32_t>(), 1);

s/uint64_t/uint32_t/

> +	maxExposure_ = itExp->second.range().max().get<int32_t>();
> +	exposure_ = minExposure_;
> +
> +	minGain_ = std::max<uint64_t>(itGain->second.range().min().get<int32_t>(), 1);

Here too.

> +	maxGain_ = itGain->second.range().max().get<int32_t>();
> +	gain_ = minGain_;
> +
> +	LOG(IPARkISP1, Info)
> +		<< "Exposure: " << minExposure_ << "-" << maxExposure_
> +		<< " Gain: " << minGain_ << "-" << maxGain_;
> +
> +	setControls(0);
> +}
> +
> +void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> +{
> +	for (IPABuffer buffer : buffers) {
> +		bufferInfo_[buffer.id] = buffer.memory;
> +		bufferInfo_[buffer.id].planes()[0].mem();
> +	}
> +}
> +
> +void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> +{
> +	for (unsigned int id : ids)
> +		bufferInfo_.erase(id);
> +}
> +
> +void IPARkISP1::processEvent(const IPAOperationData &event)
> +{
> +	switch (event.operation) {
> +	case RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER: {
> +		unsigned int frame = event.data[0];

Would it make sense to move the frame number out of IPAOperationData to
an explicit parameter of the processEvent method, as we did for the
queueFrameAction signal ? If you're unsure this could be done later, on
top of this series.

> +		unsigned int id = event.data[1];

Maybe s/id/buffer/ (or bufferId) to be more explicit ?

> +
> +		const rkisp1_stat_buffer *stats =
> +			static_cast<rkisp1_stat_buffer *>(bufferInfo_[id].planes()[0].mem());
> +
> +		updateStatistics(frame, stats);
> +		break;
> +	}
> +	case RKISP1_IPA_EVENT_QUEUE_REQUEST: {
> +		unsigned int frame = event.data[0];
> +		unsigned int id = event.data[1];

Same here.

> +
> +		rkisp1_isp_params_cfg *params =
> +			static_cast<rkisp1_isp_params_cfg *>(bufferInfo_[id].planes()[0].mem());
> +
> +		queueRequest(frame, event.data[1], params, event.controls[0]);
> +		break;
> +	}
> +	default:
> +		LOG(IPARkISP1, Error) << "Unkown event " << event.operation;
> +		break;
> +	}
> +}
> +
> +void IPARkISP1::queueRequest(unsigned int frame, unsigned int paramid,
> +			     rkisp1_isp_params_cfg *params,
> +			     const ControlList &controls)
> +{
> +	/* Prepare parameters buffer. */
> +	memset(params, 0, sizeof(*params));
> +
> +	/* Auto Exposure on/off. */
> +	if (controls.contains(controls::AeEnable)) {
> +		autoExposure_ = controls.get(controls::AeEnable);
> +		if (autoExposure_)
> +			params->module_ens = CIFISP_MODULE_AEC;
> +
> +		params->module_en_update = CIFISP_MODULE_AEC;
> +	}
> +
> +	/* Queue buffers to pipeline. */
> +	queueBuffer(frame, paramid);
> +}
> +
> +void IPARkISP1::updateStatistics(unsigned int frame,
> +				 const rkisp1_stat_buffer *stats)
> +{
> +	const cifisp_stat *params = &stats->params;
> +	unsigned int aeState = 0;
> +
> +	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)
> +				continue;
> +
> +			value += ae->exp_mean[i];
> +			num++;
> +		}
> +		value /= num;
> +
> +		double factor = (double)target / value;
> +
> +		if (frame % 3 == 0) {
> +			double tmp;

s/tmp/exposure/ ?

> +
> +			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);
> +		}
> +
> +		aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> +	}
> +
> +	metadataReady(frame, aeState);
> +}
> +
> +void IPARkISP1::setControls(unsigned int frame)
> +{
> +	IPAOperationData op;
> +	op.operation = RKISP1_IPA_ACTION_V4L2_SET;
> +
> +	V4L2ControlList ctrls;
> +	ctrls.add(V4L2_CID_EXPOSURE, exposure_);
> +	ctrls.add(V4L2_CID_ANALOGUE_GAIN, gain_);
> +	op.v4l2controls.push_back(ctrls);
> +
> +	queueFrameAction.emit(frame, op);
> +}
> +
> +void IPARkISP1::queueBuffer(unsigned int frame, unsigned int id)

The id argument is not used, you can remove it.

The method name should also be updated, as it doesn't queue a buffer
anymore. As it's called in a single place, maybe you could inline it ?
Don't forgot to also update the comment in queueRequest() accordingly.

> +{
> +	IPAOperationData op;
> +	op.operation = RKISP1_IPA_ACTION_PARAM_FILLED;
> +
> +	queueFrameAction.emit(frame, op);
> +}
> +
> +void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> +{
> +	ControlList ctrls(nullptr);
> +
> +	IPAOperationData op;
> +	op.operation = RKISP1_IPA_ACTION_METADATA;
> +
> +	if (aeState)
> +		ctrls.set(controls::AeLocked, aeState == 2);

I would either move those two lines just after defining the ctrls
variable, or move the ctrls variable definition just before them.

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	op.controls.push_back(ctrls);
> +
> +	queueFrameAction.emit(frame, op);
> +}
> +
> +/*
> + * 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 */