[PATCH v2 4/4] pipeline: rkisp1: Implement gamma control
Stefan Klug
stefan.klug at ideasonboard.com
Thu May 23 10:41:20 CEST 2024
Hi Jacopo, hi Kieran,
thanks for the review.
On Thu, May 23, 2024 at 09:54:57AM +0200, Jacopo Mondi wrote:
> Hi Stefan
>
> On Wed, May 22, 2024 at 11:40:18PM GMT, Kieran Bingham wrote:
> > 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.
I split them in two, because I didn't know if we would come to an
agreement on the control easy enough. The split allows to merge the base
algo and work on the user-interface later on.
> >
> >
> > >
> > > 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;
>
> Should 2.2 be defined as a constant value (and iirc you have it defined in 1/4)
>
Ack
>
> > > + 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
>
> /*
> * Multiline
> * comments
> */
>
> > > + * 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?
This fits to the question below, if gamma_ is a private context store. I
actually used gamma_ to represent the current state of the hardware.
With pfc in mind and the way we use context.activeState, it represents a
*future* state of the hardware (Think of queuing 100 requests in
advance). I don't know if this was the intended usage. But we need to
track the current state somewhere to know if we have to reconfigure the
hardware.
> >
> >
> > > + */
> > > + 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?
>
> Seconded
As noted above, it was used to see if the hardware state differs from
the state requested for that frame. Mabe I'm missing something
conceptionally here...
>
> >
> > 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];
> > > + }
>
> This explains to me why we're re-programming the curve (question I had
> in 1/4)
>
> > >
> > > - 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;
>
> And this also explains the question I had about using the logarithimc
> curve instead of the equidistant segments one
>
> > > + 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 thought the module_en* registers only specify which modules are
enabled and that I could reconfigure them without having to touch the
en* bits again.
> >
> > I would think the comment can be dropped here.
I'm fine with that.
Cheers,
Stefan
> >
> > > + */
> > > + 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_;
>
> Ah ok. As Kieran suggest I presume you can drop gamma_ and make it a
> static constexpr flost kGammaSRGB = 2.2; (or any other name you like)
>
> > > };
> > >
> > > } /* 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