[libcamera-devel] [PATCH v6 8/9] libcamera: ipa: rkisp1: Add basic control of auto exposure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Oct 11 14:37:05 CEST 2019
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 at 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 = ¶ms->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 at 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list