[PATCH 3/3] pipeline: rkisp1: Implement gamma control
Stefan Klug
stefan.klug at ideasonboard.com
Tue May 21 16:49:35 CEST 2024
Hi Dan,
thanks for the review.
On Mon, May 20, 2024 at 11:53:15AM +0100, Dan Scally wrote:
> Hi Stefan - thanks for the patch
>
> On 16/05/2024 13:41, Stefan Klug wrote:
> > Add support for the gamma control on the rkisp1. This was tested on
> > an imx8mp.
> >
> > While at it, reorder the controls inside rkisp1.cpp alphabetically.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> > src/ipa/rkisp1/algorithms/goc.cpp | 74 ++++++++++++++++++++++++++-----
> > src/ipa/rkisp1/algorithms/goc.h | 11 ++++-
> > src/ipa/rkisp1/ipa_context.h | 4 ++
> > src/ipa/rkisp1/rkisp1.cpp | 3 +-
> > 4 files changed, 80 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
> > index 1598d730..2c7cb8a8 100644
> > --- a/src/ipa/rkisp1/algorithms/goc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/goc.cpp
> > @@ -11,6 +11,8 @@
> > #include <libcamera/base/log.h>
> > #include <libcamera/base/utils.h>
> > +#include <libcamera/control_ids.h>
> > +
> > #include "libcamera/internal/yaml_parser.h"
> > #include "linux/rkisp1-config.h"
> > @@ -53,6 +55,41 @@ int Gamma::init([[maybe_unused]] IPAContext &context,
> > return 0;
> > }
> > +/**
> > + * \brief Configure the Gamma given a configInfo
> > + * \param[in] context The shared IPA context
> > + * \param[in] configInfo The IPA configuration data
> > + *
> > + * \return 0
> > + */
> > +int Gamma::configure(IPAContext &context,
> > + [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> > +{
> > + context.activeState.gamma = 2.2;
> > + return 0;
> > +}
> I think ::configure() is often called in stream start paths, so this would
> reset the gamma when the camera is stopped and started - is that what we
> want to happen?
I think that's also what other algos do (agc/awb). I believe we said,
that state should not survive a restart of a camera. But I might be
mistaken.
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> > + */
> > +void Gamma::queueRequest([[maybe_unused]] IPAContext &context,
> > + [[maybe_unused]] const uint32_t frame,
> > + IPAFrameContext &frameContext,
> > + const ControlList &controls)
> > +{
> > + const auto &gamma = controls.get(controls::Gamma);
> > + if (gamma) {
> > + /* \todo This is not correct as it updates the current state with a
> > + * future value. But it does no harm at the moment an allows us to
> > + * track the last active gamma
> > + */
> > + context.activeState.gamma = *gamma;
> > + LOG(RkISP1Gamma, Debug) << "Set gamma to " << *gamma;
> > + }
> > +
> > + frameContext.gamma = context.activeState.gamma;
> > +}
> > +
> > /**
> > * \copydoc libcamera::ipa::Algorithm::prepare
> > */
> > @@ -67,19 +104,36 @@ void Gamma::prepare([[maybe_unused]] IPAContext &context,
> > 512, 512, 512, 512, 512, 0 };
> > auto gamma_y = params->others.goc_config.gamma_y;
> > - if (frame > 0)
> > - return;
> > + if (frame == 0 || std::fabs(frameContext.gamma - gamma_) > 0.001) {
> > + gamma_ = frameContext.gamma;
> > +
> > + int x = 0;
> > + for (int i = 0; i < RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10; i++) {
> > + gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;
> > + x += segments[i];
> > + }
> > - int x = 0;
> > - for (int i = 0; i < RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10; i++) {
> > - gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;
> > - x += segments[i];
> > + params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;
> > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > +
> > + /* It is unclear why these bits need to be set more than once.
> > + * Setting them only on frame 0 didn't apply gamma.
> > + */
> > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > + params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;
> > }
> > +}
> > - params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;
> > - params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > - params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;
> > - params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::process
> > + */
> > +void Gamma::process([[maybe_unused]] IPAContext &context,
> > + [[maybe_unused]] const uint32_t frame,
> > + IPAFrameContext &frameContext,
> > + [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > + ControlList &metadata)
> > +{
> > + metadata.set(controls::Gamma, frameContext.gamma);
> > }
> > REGISTER_IPA_ALGORITHM(Gamma, "Gamma")
> > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h
> > index 3c83655b..c6414040 100644
> > --- a/src/ipa/rkisp1/algorithms/goc.h
> > +++ b/src/ipa/rkisp1/algorithms/goc.h
> > @@ -20,12 +20,21 @@ public:
> > ~Gamma() = default;
> > int init(IPAContext &context, const YamlObject &tuningData) override;
> > + int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > + void queueRequest(IPAContext &context,
> > + const uint32_t frame,
> > + IPAFrameContext &frameContext,
> > + const ControlList &controls) override;
> > void prepare(IPAContext &context, const uint32_t frame,
> > IPAFrameContext &frameContext,
> > rkisp1_params_cfg *params) override;
> > + void process(IPAContext &context, const uint32_t frame,
> > + IPAFrameContext &frameContext,
> > + const rkisp1_stat_buffer *stats,
> > + ControlList &metadata) override;
> > private:
> > - float gamma_ = 2.2;
> > + float gamma_;
> > };
> > } /* namespace ipa::rkisp1::algorithms */
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index bd02a7a2..5252e222 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -104,6 +104,8 @@ struct IPAActiveState {
> > uint8_t denoise;
> > uint8_t sharpness;
> > } filter;
> > +
> > + double gamma;
> > };
> > struct IPAFrameContext : public FrameContext {
> > @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {
> > uint32_t exposure;
> > double gain;
> > } sensor;
> > +
> > + double gamma;
> > };
> > struct IPAContext {
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 6687c91e..7364fd73 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -108,9 +108,10 @@ const IPAHwSettings ipaHwSettingsV12{
> > const ControlInfoMap::Map rkisp1Controls{
> > { &controls::AeEnable, ControlInfo(false, true) },
> > { &controls::AwbEnable, ControlInfo(false, true) },
> > - { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
> > + { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) },
> > + { &controls::Gamma, ControlInfo(0.1f, 10.0f, 2.2f) },
> > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) },
> > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>
>
> I think that this should be registered by the algorithm during ::init() and
> merged into context.ctrlMap - otherwise in a case where the tuning file
> chooses not to instantiate the Gamma algorithm we're going to advertise a
> control that we don't actually support.
Yes that's definitely better. I'll fix it in the next version.
Cheers,
Stefan
More information about the libcamera-devel
mailing list