[libcamera-devel] [PATCH 2/5] ipa: rkisp1: Add support of Gamma Sensor Linearization control
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 13 01:59:50 CEST 2022
Hello,
On Tue, Jul 12, 2022 at 04:18:01PM +0900, Paul Elder via libcamera-devel wrote:
> On Wed, Jun 22, 2022 at 05:19:15PM +0200, Florian Sylvestre via libcamera-devel wrote:
> > The GammaSensorLinearization algorithm improves the image dynamic using
It's not really about improving the image dynamic, but about
compensating the non-linearities of the camera sensor to obtain a linear
image for further processing.
> > a coefficient table in the YAML tuning file.
> >
> > Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> > ---
> > src/ipa/rkisp1/algorithms/gsl.cpp | 142 ++++++++++++++++++++++++++
> > src/ipa/rkisp1/algorithms/gsl.h | 38 +++++++
> > src/ipa/rkisp1/algorithms/meson.build | 1 +
> > src/ipa/rkisp1/data/ov5640.yaml | 6 ++
> > src/ipa/rkisp1/rkisp1.cpp | 1 +
> > 5 files changed, 188 insertions(+)
> > create mode 100644 src/ipa/rkisp1/algorithms/gsl.cpp
> > create mode 100644 src/ipa/rkisp1/algorithms/gsl.h
> >
> > diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp
> > new file mode 100644
> > index 00000000..7ff3d360
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/gsl.cpp
> > @@ -0,0 +1,142 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021-2022, Ideas On Board
> > + *
> > + * gsl.cpp - RkISP1 Gamma Sensor Linearization control
> > + */
> > +
> > +#include "gsl.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
Please add a blank line here.
> > +#include "linux/rkisp1-config.h"
> > +
> > +/**
> > + * \file gsl.h
> > + */
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +/**
> > + * \class GammaSensorLinearization
> > + * \brief RkISP1 Gamma Sensor Linearization control
> > + *
> > + * This algorithm improves the image dynamic using a coefficient table in the
> > + * Yaml tuning file, the table contains:
s/Yaml/YAML/
s/, the table/. The table/
> > + * - 'x-intervals': These values corresponds to GAMMA_DX_n values (from
> > + * GAMMA_DX_0 to GAMMA_DX_16 fields in RKISP1_CIF_ISP_GAMMA_DX_LO and
>
> s/DX_0/DX_1/ ?
>
> > + * RKISP1_CIF_ISP_GAMMA_DX_HI rkisp1 registers).
> > + * Note: rkisp1 use each value to compute a X interval equal to 2^(value+4)
>
> s/use/uses/
>
> s/a X/an X/
>
> > + * - 'y' values for each component (red, green , blue) on 12 bits resolution.
I'd make it explicit that the curves are specified as piecewise linear
functions.
* This algorithm linearizes the sensor output to compensate the sensor
* non-linearities by applying piecewise linear curves to the red, green and
* blue channels.
*
* The curves are specified in the tuning data and defined using 17 points.
*
* - The X coordinates are expressed using 16 intervals, with the first point at
* X coordinate 0. Each interval is expressed as a 2-bit value DX (from
* GAMMA_DX_1 to GAMMA_DX_16), stored in the RKISP1_CIF_ISP_GAMMA_DX_LO and
* RKISP1_CIF_ISP_GAMMA_DX_HI registers. The real interval is equal to
* \f$2^{dx+4}\f$. X coordinates are shared between the red, green and blue
* curves.
*
* - The Y coordinates are specified as 17 values separately for the
* red, green and blue channels, with a 12-bit resolution. Each value must be
* in the [-2048, 2047] range compared to the previous value.
> > + */
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Gsl)
> > +
> > +#define DEGAMMA_X_INTERVALS 16
static constexpr unsigned int kDegammaXIntervals = 16;
> > +
> > +GammaSensorLinearization::GammaSensorLinearization()
> > + : tuningParameters_(false)
>
> I would call this tuned_ or something along this lines, as this variable
> signifies if the tuning parameters have been loaded correctly or not,
> and does not actually contain the tuning parameters as the varaible name
> suggests.
Or maybe just "valid_" or "loaded_" ?
> Also, what happens if no tuning file is provided? Does the algorithm
> just... not work (as in it doesn't do anything)? Is that better than
> running on some default tuning values? (I feel like I saw discussion on
> this topic elsewhere but I'm just asking it again :S)
Algorithms are instantiated based on the tuning file, so if the tuning
file doesn't list a gsl algorithm, this class won't be instantiaed. See
Module::createAlgorithm() in src/ipa/libipa/module.h.
We have an uncalibrated.yaml file that could list a gsl algorithm with
default values, but that will just be a linear curve. Not enabling the
algorithm will disable the corresponding hardware block, which I think
is better.
> > +{
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,
> > + const YamlObject &tuningData)
> > +{
> > + std::vector<uint16_t> xIntervals_ = tuningData["x-intervals"].getList<uint16_t>();
This is not a class member variable, it should be named without the
trailing _.
> > + if (xIntervals_.size() != DEGAMMA_X_INTERVALS) {
> > + LOG(RkISP1Gsl, Error)
> > + << "Issue while parsing 'x'"
> > + << "in tuning file (list size:"
> > + << xIntervals_.size() << "/"
> > + << DEGAMMA_X_INTERVALS << ")";
>
> I would prefer slightly more verbosity, like printing which/where the
> tuning file is,
I wouldn't add the file name here as we don't have access to the
information, and it would be a bit painful to pass the file name around
everywhere. We could however print the file name in IPARkISP1::init() in
case of errors.
> and saying explicitly "expected". Also I think you're
> missing spaces in the string concatenation.
Maybe as follows ?
LOG(RkISP1Gsl, Error)
<< "Invalid 'x' coordinates: expected "
<< kDegammaXIntervals << " elements, got "
<< xIntervals.size();
> Same for the other error messages below.
>
> > + return -EINVAL;
> > + }
> > +
> > + /* Compute gammaDx_ intervals from xIntervals_ values */
> > + gammaDx_[0] = 0;
> > + gammaDx_[1] = 0;
> > + for (int i = 0; i < DEGAMMA_X_INTERVALS; ++i) {
unsigned int i
> > + gammaDx_[i / 8] |= (xIntervals_[i] & 0x07) << ((i % 8) * 4);
> > + }
>
> I don't think you need the braces.
Indeed.
> Neat math trick :)
>
> > +
> > + const YamlObject &yObject = tuningData["y"];
> > +
> > + if (!yObject.isDictionary()) {
> > + LOG(RkISP1Gsl, Error)
> > + << "Issue while parsing 'y' in tuning file: "
> > + << "entry must be a dictionary";
> > + return -EINVAL;
> > + }
> > +
> > + curveYr_ = yObject["red"].getList<uint16_t>();
> > + if (curveYr_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {
> > + LOG(RkISP1Gsl, Error)
> > + << "Issue while parsing 'y:red'"
> > + << "in tuning file (list size: "
> > + << curveYr_.size() << "/"
> > + << RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << ")";
> > + return -EINVAL;
> > + }
> > +
> > + curveYg_ = yObject["green"].getList<uint16_t>();
> > + if (curveYg_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {
> > + LOG(RkISP1Gsl, Error)
> > + << "Issue while parsing 'y:green'"
> > + << "in tuning file (list size: "
> > + << curveYg_.size() << "/"
> > + << RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << ")";
> > + return -EINVAL;
> > + }
> > +
> > + curveYb_ = yObject["blue"].getList<uint16_t>();
> > + if (curveYb_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {
> > + LOG(RkISP1Gsl, Error)
> > + << "Issue while parsing 'y:blue'"
> > + << "in tuning file (list size: "
> > + << curveYb_.size() << "/"
> > + << RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << ")";
> > + return -EINVAL;
> > + }
> > +
> > + tuningParameters_ = true;
> > + return 0;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::prepare
> > + */
> > +void GammaSensorLinearization::prepare(IPAContext &context,
> > + rkisp1_params_cfg *params)
> > +{
> > + if (context.frameContext.frameCount > 0)
> > + return;
> > +
> > + if (!tuningParameters_)
> > + return;
> > +
> > + params->others.sdg_config.xa_pnts.gamma_dx0 = gammaDx_[0];
> > + params->others.sdg_config.xa_pnts.gamma_dx1 = gammaDx_[1];
> > +
> > + std::copy(curveYr_.begin(), curveYr_.end(),
> > + params->others.sdg_config.curve_r.gamma_y);
> > + std::copy(curveYg_.begin(), curveYg_.end(),
> > + params->others.sdg_config.curve_g.gamma_y);
> > + std::copy(curveYb_.begin(), curveYb_.end(),
> > + params->others.sdg_config.curve_b.gamma_y);
> > +
> > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_SDG;
> > + params->module_ens |= RKISP1_CIF_ISP_MODULE_SDG;
> > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_SDG;
> > +}
> > +
> > +REGISTER_IPA_ALGORITHM(GammaSensorLinearization)
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/gsl.h b/src/ipa/rkisp1/algorithms/gsl.h
> > new file mode 100644
> > index 00000000..df16a89b
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/gsl.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021-2022, Ideas On Board
> > + *
> > + * gsl.h - RkISP1 Gamma Sensor Linearization control
> > + */
> > +
> > +#pragma once
> > +
> > +#include <linux/rkisp1-config.h>
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +struct IPACameraSensorInfo;
I think you can drop this, it's not used in this header.
linux/rkisp1-config.h can also be dropped, its only usage is for the
parameters of the functions defined by the Algorithm base class, so
there's a guarantee the right headers will be included by including
algorithm.h.
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +class GammaSensorLinearization : public Algorithm
> > +{
> > +public:
> > + GammaSensorLinearization();
> > + ~GammaSensorLinearization() = default;
> > +
> > + int init(IPAContext &context, const YamlObject &tuningData) override;
> > + void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> > +
> > +private:
> > + bool tuningParameters_;
> > + uint32_t gammaDx_[2];
> > + std::vector<uint16_t> curveYr_;
> > + std::vector<uint16_t> curveYg_;
> > + std::vector<uint16_t> curveYb_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > index 7ec53d89..0597c353 100644
> > --- a/src/ipa/rkisp1/algorithms/meson.build
> > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > @@ -4,4 +4,5 @@ rkisp1_ipa_algorithms = files([
> > 'agc.cpp',
> > 'awb.cpp',
> > 'blc.cpp',
> > + 'gsl.cpp',
> > ])
> > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml
> > index 232d8ae8..6cb84ed9 100644
> > --- a/src/ipa/rkisp1/data/ov5640.yaml
> > +++ b/src/ipa/rkisp1/data/ov5640.yaml
> > @@ -10,4 +10,10 @@ algorithms:
> > Gr: 256
> > Gb: 256
> > B: 256
> > + - GammaSensorLinearization:
> > + x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]
> > + y:
> > + red: [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]
> > + green: [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]
> > + blue: [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]
The last value should be 4095, 4096 doesn't fit in 12 bits.
> > ...
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index d052954e..622a0d61 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -31,6 +31,7 @@
> > #include "algorithms/algorithm.h"
> > #include "algorithms/awb.h"
> > #include "algorithms/blc.h"
> > +#include "algorithms/gsl.h"
> > #include "libipa/camera_sensor_helper.h"
> >
> > #include "ipa_context.h"
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list