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

[Niklas Söderlund]

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 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 ...

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

-- 
Regards,
Niklas Söderlund