[libcamera-devel] [PATCH v1 1/6] ipa: rkisp1: Introduce sensor degamma
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 3 10:26:17 CET 2021
Hi Jean-Michel,
On Fri, Dec 03, 2021 at 07:27:17AM +0100, Jean-Michel Hautbois wrote:
> On 03/12/2021 03:24, Laurent Pinchart wrote:
> > On Thu, Dec 02, 2021 at 07:04:05PM +0100, Jean-Michel Hautbois wrote:
> >> The RkISP1 has a sensor degamma control. Introduce the algorithm, but
> >> only implement the prepare function as it is a static table to be set.
> >> The table used is the one found in the imx219 tuning data in RPi as this
> >> is the only sensor I have right now and it looks like a decent default
> >> table.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >> src/ipa/rkisp1/algorithms/meson.build | 1 +
> >> src/ipa/rkisp1/algorithms/sdg.cpp | 49 +++++++++++++++++++++++++++
> >> src/ipa/rkisp1/algorithms/sdg.h | 30 ++++++++++++++++
> >> src/ipa/rkisp1/rkisp1.cpp | 2 ++
> >> 4 files changed, 82 insertions(+)
> >> create mode 100644 src/ipa/rkisp1/algorithms/sdg.cpp
> >> create mode 100644 src/ipa/rkisp1/algorithms/sdg.h
> >>
> >> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> >> index a19c1a4fa..0678518d8 100644
> >> --- a/src/ipa/rkisp1/algorithms/meson.build
> >> +++ b/src/ipa/rkisp1/algorithms/meson.build
> >> @@ -2,4 +2,5 @@
> >>
> >> rkisp1_ipa_algorithms = files([
> >> 'agc.cpp',
> >> + 'sdg.cpp',
> >> ])
> >> diff --git a/src/ipa/rkisp1/algorithms/sdg.cpp b/src/ipa/rkisp1/algorithms/sdg.cpp
> >> new file mode 100644
> >> index 000000000..3f6b3e205
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/sdg.cpp
> >> @@ -0,0 +1,49 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Ideas On Board
> >> + *
> >> + * sdg.cpp - Sensor degamma control algorithm
> >> + */
> >> +
> >> +#include "sdg.h"
> >> +
> >> +/**
> >> + * \file sdg.h
> >> + */
> >> +
> >> +namespace libcamera {
> >> +
> >> +namespace ipa::rkisp1::algorithms {
> >> +
> >> +/**
> >> + * \class SensorDeGamma
> >> + * \brief A sensor degamma algorithm
> >> + */
> >> +
> >> +static const uint16_t imx219DeGammaCurve[] = { 0, 583, 957, 1299, 1609, 1877,
> >> + 2123, 2350, 2540, 2859, 3101, 3293,
> >> + 3429, 3666, 3823, 3963, 4095 };
> >
> > Where does this come from ?
>
> As stated in the commit message, this is coming from the imx219 tuning file.
> I could have been more precise, it is the rpi.contrast.gamma_curve table
> adapted to the RkISP1.
That's what I thought looking at imx219.json, but I wasn't sure. The two
curves don't seem to match when plotting this one linearly. You may have
missed that the horizontal step in rpi.contrast.gamma_curve isn't
constant.
This brings me to the next question then: the above is a contrast curve
to be applied at the end of the pipeline to enhance contrast, while the
SDG is a curved meant to linearize the sensor output. Those are two
entirely different things.
> >> +
> >> +/**
> >> + * \copydoc libcamera::ipa::Algorithm::prepare
> >> + */
> >> +void SensorDeGamma::prepare([[maybe_unused]] IPAContext &context,
> >> + rkisp1_params_cfg *params)
> >> +{
> >> + for (uint32_t i = 0; i < RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE; i++) {
> >
> > That's dangerous.
> >
> > namespace {
> >
> > const std::array<uint16_t, RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE> imx219DeGammaCurve = {
> > 0, 583, 957, 1299, 1609, 1877, 2123, 2350, 2540,
> > 2859, 3101, 3293, 3429, 3666, 3823, 3963, 4095
> > };
> >
> > } /* namespace */
> >
> > ...
> >
> > for (const auto &[i, value] : utils::enumerate(imx219DeGammaCurve)) {
> > params->others.sdg_config.curve_r.gamma_y[i] = value;
> > params->others.sdg_config.curve_b.gamma_y[i] = value;
> > params->others.sdg_config.curve_g.gamma_y[i] = value;
> > }
> >
> > (utils::enumerate is really just because we can, the important part is
> > to avoid a potential array overflow, you can use
> > std::size(imx219DeGammaCurve) instead).
>
> I don't always have the C++ reflex indeed :-), thanks.
>
> >> + params->others.sdg_config.curve_r.gamma_y[i] = imx219DeGammaCurve[i];
> >> + params->others.sdg_config.curve_b.gamma_y[i] = imx219DeGammaCurve[i];
> >> + params->others.sdg_config.curve_g.gamma_y[i] = imx219DeGammaCurve[i];
> >> + }
> >> +
> >> + params->others.sdg_config.xa_pnts.gamma_dx0 = 0x44444444;
> >> + params->others.sdg_config.xa_pnts.gamma_dx1 = 0x44444444;
> >
> > What is this ?
> >
> >> +
> >> + params->module_en_update |= RKISP1_CIF_ISP_MODULE_SDG;
> >
> > As the array doesn't change, can you update the contents for the first
> > frame only ?
>
> Only if the params is no more forced to 0 in IPARkISP1 then :-). I can
> change this.
I mean that if you skip setting module_cfg_update on all frames after
the first, the driver will not update the SDG configuration. You can
read the driver code to see how it works.
> >> + params->module_ens |= RKISP1_CIF_ISP_MODULE_SDG;
> >> + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_SDG;
> >> +}
> >> +
> >> +} /* namespace ipa::rkisp1::algorithms */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/ipa/rkisp1/algorithms/sdg.h b/src/ipa/rkisp1/algorithms/sdg.h
> >> new file mode 100644
> >> index 000000000..24c41627a
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/sdg.h
> >> @@ -0,0 +1,30 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Ideas On Board
> >> + *
> >> + * sdg.h - Sensor degamma control algorithm
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <linux/rkisp1-config.h>
> >> +
> >> +#include "algorithm.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +struct IPACameraSensorInfo;
> >> +
> >> +namespace ipa::rkisp1::algorithms {
> >> +
> >> +class SensorDeGamma : public Algorithm
> >> +{
> >> +public:
> >> + SensorDeGamma() = default;
> >> + ~SensorDeGamma() = default;
> >> +
> >> + void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> >> +};
> >> +
> >> +} /* namespace ipa::rkisp1::algorithms */
> >> +} /* namespace libcamera */
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index 2d79f15fe..07f1f1c87 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -27,6 +27,7 @@
> >>
> >> #include "algorithms/agc.h"
> >> #include "algorithms/algorithm.h"
> >> +#include "algorithms/sdg.h"
> >> #include "libipa/camera_sensor_helper.h"
> >>
> >> #include "ipa_context.h"
> >> @@ -126,6 +127,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> >>
> >> /* Construct our Algorithms */
> >> algorithms_.push_back(std::make_unique<algorithms::Agc>());
> >> + algorithms_.push_back(std::make_unique<algorithms::SensorDeGamma>());
> >>
> >> return 0;
> >> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list