[PATCH v2 4/4] pipeline: rkisp1: Implement gamma control
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 23 00:40:18 CEST 2024
Quoting Stefan Klug (2024-05-22 15:54:38)
> Add support for the gamma control on the rkisp1. This was tested on
> an imx8mp.
I think patch 1/4 and 4/4 could maybe be merged together, but reviewing
independently for now.
>
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
> src/ipa/rkisp1/algorithms/goc.cpp | 77 +++++++++++++++++++++++++++----
> src/ipa/rkisp1/algorithms/goc.h | 11 ++++-
> src/ipa/rkisp1/ipa_context.h | 4 ++
> 3 files changed, 81 insertions(+), 11 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
> index 6f313820..0b312e7a 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"
> @@ -41,6 +43,7 @@ namespace ipa::rkisp1::algorithms {
> LOG_DEFINE_CATEGORY(RkISP1Gamma)
>
> Gamma::Gamma()
> + : gamma_(0)
> {
> }
>
> @@ -50,6 +53,8 @@ Gamma::Gamma()
> int Gamma::init([[maybe_unused]] IPAContext &context,
> [[maybe_unused]] const YamlObject &tuningData)
> {
> + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);
> +
> if (context.hw->numGammaOutSamples !=
> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {
> LOG(RkISP1Gamma, Error)
> @@ -60,6 +65,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;
> +}
> +
> +/**
> + * \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
I'm not sure if this is a todo. I kind of think it's correct?
> + */
> + context.activeState.gamma = *gamma;
> + LOG(RkISP1Gamma, Debug) << "Set gamma to " << *gamma;
> + }
> +
> + frameContext.gamma = context.activeState.gamma;
> +}
> +
> /**
> * \copydoc libcamera::ipa::Algorithm::prepare
> */
> @@ -78,19 +118,36 @@ void Gamma::prepare([[maybe_unused]] IPAContext &context,
> ASSERT(context.hw->numGammaOutSamples ==
> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);
>
> - if (frame > 0)
> - return;
> + if (frame == 0 || std::fabs(frameContext.gamma - gamma_) > 0.001) {
Can you invert the logic here so we still return early if there's no
work todo - and lower the indent on the code that follows?
> + gamma_ = frameContext.gamma;
Is gamma_ actually used as a private context store outside of this now?
Should it be a local alias if it's just to shortent frameContext.gamma,
or can that be used directly?
That definitely makes me think merging 1/4 and 4/4 would make this a
cleaner patch. I'm not sure the split gives us much here, except meaning
I have to have both patches open side by side to review.
> +
> + int x = 0;
> + for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {
> + gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;
> + x += segments[i];
> + }
>
> - int x = 0;
> - for (unsigned i = 0; i < context.hw->numGammaOutSamples; 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.
I think that's probably expected behaviour. these flags tell the kernel
that there is an update to the configuration structures, and for it to
read the update and apply it.
I would think the comment can be dropped here.
> + */
> + 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 fe7caba3..f2142b55 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 {
> --
> 2.40.1
>
More information about the libcamera-devel
mailing list