[PATCH v2 4/4] pipeline: rkisp1: Implement gamma control
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 23 14:18:44 CEST 2024
Quoting Stefan Klug (2024-05-23 09:41:20)
> 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.
That does make sense, as we need to set the gamma handling in the
i.MX8MP regardless of whether we add the control indeed.
I guess it depends on what others think about the Gamma control...
> > > > 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.
The thing is at the time the request is queued, we also need to be
working on what the future state of the hardware will be, *not* what the
current state of the hardware is!
It's definitely a tricky balance as the queueRequest() time frame can be
many frames ahead of what is actually being processed. But that's where
the FrameContext should then balance out what context is relevant for
/that/ frame time, vs what the most current state is.
I think for a control like gamma where there is no 'automatic
processing' based on statistics we're fine here - but indeed if we have
auto algorithsm there are 3 event points to potentially track:
- queueRequest (sets the future)
- prepare (controls the immediate future)
- processStatistics (processes the past, helps to determine the future)
I do think it's up to each algorithm to decide how it uses the
ActiveState and it's own private state to meet those requirements, but
maybe we should already do better at what the current algorithms use and
how.
--
Kieran
>
> > >
> > >
> > > > + */
> > > > + 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