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

1877 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
but lacks the support to inform 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 | 165 +++++++++++++++++++++++++++++++++++++++++
 src/ipa/meson.build    |   1 +
 2 files changed, 166 insertions(+)
 create mode 100644 src/ipa/ipa_rkisp1.cpp

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@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')
>

Hi Kieran,

Thanks for your feedback.

On 2019-08-29 16:16:06 +0100, Kieran Bingham wrote:
> 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@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 ...

The IPA will be isolated so we can't store references. We could have a 
local ControlList, but why have all that extra code in a hot path?

> 
> 
> > +};
> > +
> > +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?

Yes, this is a step to reduce the information passed between pipeline 
and IPA to visualize what is really needed and make it easier to 
serialize.

> 
> > +{
> > +	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