[{"id":29599,"web_url":"https://patchwork.libcamera.org/comment/29599/","msgid":"<171641761895.3019175.14878933535706187617@ping.linuxembedded.co.uk>","date":"2024-05-22T22:40:18","subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-05-22 15:54:38)\n> Add support for the gamma control on the rkisp1. This was tested on\n> an imx8mp.\n\nI think patch 1/4 and 4/4 could maybe be merged together, but reviewing\nindependently for now.\n\n\n> \n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> ---\n>  src/ipa/rkisp1/algorithms/goc.cpp | 77 +++++++++++++++++++++++++++----\n>  src/ipa/rkisp1/algorithms/goc.h   | 11 ++++-\n>  src/ipa/rkisp1/ipa_context.h      |  4 ++\n>  3 files changed, 81 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> index 6f313820..0b312e7a 100644\n> --- a/src/ipa/rkisp1/algorithms/goc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> @@ -11,6 +11,8 @@\n>  #include <libcamera/base/log.h>\n>  #include <libcamera/base/utils.h>\n>  \n> +#include <libcamera/control_ids.h>\n> +\n>  #include \"libcamera/internal/yaml_parser.h\"\n>  \n>  #include \"linux/rkisp1-config.h\"\n> @@ -41,6 +43,7 @@ namespace ipa::rkisp1::algorithms {\n>  LOG_DEFINE_CATEGORY(RkISP1Gamma)\n>  \n>  Gamma::Gamma()\n> +       : gamma_(0)\n>  {\n>  }\n>  \n> @@ -50,6 +53,8 @@ Gamma::Gamma()\n>  int Gamma::init([[maybe_unused]] IPAContext &context,\n>                 [[maybe_unused]] const YamlObject &tuningData)\n>  {\n> +       context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> +\n>         if (context.hw->numGammaOutSamples !=\n>             RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n>                 LOG(RkISP1Gamma, Error)\n> @@ -60,6 +65,41 @@ int Gamma::init([[maybe_unused]] IPAContext &context,\n>         return 0;\n>  }\n>  \n> +/**\n> + * \\brief Configure the Gamma given a configInfo\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] configInfo The IPA configuration data\n> + *\n> + * \\return 0\n> + */\n> +int Gamma::configure(IPAContext &context,\n> +                    [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> +{\n> +       context.activeState.gamma = 2.2;\n> +       return 0;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Gamma::queueRequest([[maybe_unused]] IPAContext &context,\n> +                        [[maybe_unused]] const uint32_t frame,\n> +                        IPAFrameContext &frameContext,\n> +                        const ControlList &controls)\n> +{\n> +       const auto &gamma = controls.get(controls::Gamma);\n> +       if (gamma) {\n> +               /* \\todo This is not correct as it updates the current state with a\n> +                * future value. But it does no harm at the moment an allows us to\n> +                * track the last active gamma\n\nI'm not sure if this is a todo. I kind of think it's correct?\n\n\n> +                */\n> +               context.activeState.gamma = *gamma;\n> +               LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> +       }\n> +\n> +       frameContext.gamma = context.activeState.gamma;\n> +}\n> +\n>  /**\n>   * \\copydoc libcamera::ipa::Algorithm::prepare\n>   */\n> @@ -78,19 +118,36 @@ void Gamma::prepare([[maybe_unused]] IPAContext &context,\n>         ASSERT(context.hw->numGammaOutSamples ==\n>                RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n>  \n> -       if (frame > 0)\n> -               return;\n> +       if (frame == 0 || std::fabs(frameContext.gamma - gamma_) > 0.001) {\n\nCan you invert the logic here so we still return early if there's no\nwork todo - and lower the indent on the code that follows?\n\n> +               gamma_ = frameContext.gamma;\n\nIs gamma_ actually used as a private context store outside of this now?\nShould it be a local alias if it's just to shortent frameContext.gamma,\nor can that be used directly?\n\nThat definitely makes me think merging 1/4 and 4/4 would make this a\ncleaner patch. I'm not sure the split gives us much here, except meaning\nI have to have both patches open side by side to review.\n\n\n> +\n> +               int x = 0;\n> +               for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> +                       gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> +                       x += segments[i];\n> +               }\n>  \n> -       int x = 0;\n> -       for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> -               gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> -               x += segments[i];\n> +               params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> +               params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> +\n> +               /* It is unclear why these bits need to be set more than once.\n> +                * Setting them only on frame 0 didn't apply gamma.\n\nI think that's probably expected behaviour. these flags tell the kernel\nthat there is an update to the configuration structures, and for it to\nread the update and apply it.\n\nI would think the comment can be dropped here.\n\n> +                */\n> +               params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> +               params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n>         }\n> +}\n>  \n> -       params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> -       params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> -       params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> -       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::process\n> + */\n> +void Gamma::process([[maybe_unused]] IPAContext &context,\n> +                   [[maybe_unused]] const uint32_t frame,\n> +                   IPAFrameContext &frameContext,\n> +                   [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> +                   ControlList &metadata)\n> +{\n> +       metadata.set(controls::Gamma, frameContext.gamma);\n>  }\n>  \n>  REGISTER_IPA_ALGORITHM(Gamma, \"Gamma\")\n> diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> index fe7caba3..f2142b55 100644\n> --- a/src/ipa/rkisp1/algorithms/goc.h\n> +++ b/src/ipa/rkisp1/algorithms/goc.h\n> @@ -20,12 +20,21 @@ public:\n>         ~Gamma() = default;\n>  \n>         int init(IPAContext &context, const YamlObject &tuningData) override;\n> +       int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> +       void queueRequest(IPAContext &context,\n> +                         const uint32_t frame,\n> +                         IPAFrameContext &frameContext,\n> +                         const ControlList &controls) override;\n>         void prepare(IPAContext &context, const uint32_t frame,\n>                      IPAFrameContext &frameContext,\n>                      rkisp1_params_cfg *params) override;\n> +       void process(IPAContext &context, const uint32_t frame,\n> +                    IPAFrameContext &frameContext,\n> +                    const rkisp1_stat_buffer *stats,\n> +                    ControlList &metadata) override;\n>  \n>  private:\n> -       float gamma_ = 2.2;\n> +       float gamma_;\n>  };\n>  \n>  } /* namespace ipa::rkisp1::algorithms */\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index bd02a7a2..5252e222 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -104,6 +104,8 @@ struct IPAActiveState {\n>                 uint8_t denoise;\n>                 uint8_t sharpness;\n>         } filter;\n> +\n> +       double gamma;\n>  };\n>  \n>  struct IPAFrameContext : public FrameContext {\n> @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {\n>                 uint32_t exposure;\n>                 double gain;\n>         } sensor;\n> +\n> +       double gamma;\n>  };\n>  \n>  struct IPAContext {\n> -- \n> 2.40.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 847CEBDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 22 May 2024 22:40:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C4A86349D;\n\tThu, 23 May 2024 00:40:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37AD361A55\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 23 May 2024 00:40:22 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 460F1475;\n\tThu, 23 May 2024 00:40:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BEKxyk8Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716417609;\n\tbh=vhRuDRwiYmYJAhFQvk6rYg9YyuPOzeqi++A5jKTRLIo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=BEKxyk8YupkPprMGT15bpNY2Is8wvjAywJoRvfy8ZE25gqlKl4DGXkiIGi4xCbfkY\n\tZ/UfzAR0wU88UtW5DnrvwZL/zl0vAAmGiy98zbF4R0tvPFv1IlJ1cRUWpQH0pd8wND\n\tMVjgtpVDzRTLYvi2icnwgbmgF5XSKnWD32wv5Ptg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240522145438.436688-5-stefan.klug@ideasonboard.com>","References":"<20240522145438.436688-1-stefan.klug@ideasonboard.com>\n\t<20240522145438.436688-5-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 22 May 2024 23:40:18 +0100","Message-ID":"<171641761895.3019175.14878933535706187617@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29605,"web_url":"https://patchwork.libcamera.org/comment/29605/","msgid":"<hcqylywlnhr5ab2dz7sdfgdxva7tw2mwcdc66udovizxb637da@tdtatlwhirk7>","date":"2024-05-23T07:54:57","subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Wed, May 22, 2024 at 11:40:18PM GMT, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-05-22 15:54:38)\n> > Add support for the gamma control on the rkisp1. This was tested on\n> > an imx8mp.\n>\n> I think patch 1/4 and 4/4 could maybe be merged together, but reviewing\n> independently for now.\n>\n>\n> >\n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > ---\n> >  src/ipa/rkisp1/algorithms/goc.cpp | 77 +++++++++++++++++++++++++++----\n> >  src/ipa/rkisp1/algorithms/goc.h   | 11 ++++-\n> >  src/ipa/rkisp1/ipa_context.h      |  4 ++\n> >  3 files changed, 81 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> > index 6f313820..0b312e7a 100644\n> > --- a/src/ipa/rkisp1/algorithms/goc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> > @@ -11,6 +11,8 @@\n> >  #include <libcamera/base/log.h>\n> >  #include <libcamera/base/utils.h>\n> >\n> > +#include <libcamera/control_ids.h>\n> > +\n> >  #include \"libcamera/internal/yaml_parser.h\"\n> >\n> >  #include \"linux/rkisp1-config.h\"\n> > @@ -41,6 +43,7 @@ namespace ipa::rkisp1::algorithms {\n> >  LOG_DEFINE_CATEGORY(RkISP1Gamma)\n> >\n> >  Gamma::Gamma()\n> > +       : gamma_(0)\n> >  {\n> >  }\n> >\n> > @@ -50,6 +53,8 @@ Gamma::Gamma()\n> >  int Gamma::init([[maybe_unused]] IPAContext &context,\n> >                 [[maybe_unused]] const YamlObject &tuningData)\n> >  {\n> > +       context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> > +\n> >         if (context.hw->numGammaOutSamples !=\n> >             RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n> >                 LOG(RkISP1Gamma, Error)\n> > @@ -60,6 +65,41 @@ int Gamma::init([[maybe_unused]] IPAContext &context,\n> >         return 0;\n> >  }\n> >\n> > +/**\n> > + * \\brief Configure the Gamma given a configInfo\n> > + * \\param[in] context The shared IPA context\n> > + * \\param[in] configInfo The IPA configuration data\n> > + *\n> > + * \\return 0\n> > + */\n> > +int Gamma::configure(IPAContext &context,\n> > +                    [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > +{\n> > +       context.activeState.gamma = 2.2;\n\nShould 2.2 be defined as a constant value (and iirc you have it defined in 1/4)\n\n\n> > +       return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > + */\n> > +void Gamma::queueRequest([[maybe_unused]] IPAContext &context,\n> > +                        [[maybe_unused]] const uint32_t frame,\n> > +                        IPAFrameContext &frameContext,\n> > +                        const ControlList &controls)\n> > +{\n> > +       const auto &gamma = controls.get(controls::Gamma);\n> > +       if (gamma) {\n> > +               /* \\todo This is not correct as it updates the current state with a\n\n                   /*\n                    * Multiline\n                    * comments\n                    */\n\n> > +                * future value. But it does no harm at the moment an allows us to\n> > +                * track the last active gamma\n>\n> I'm not sure if this is a todo. I kind of think it's correct?\n>\n>\n> > +                */\n> > +               context.activeState.gamma = *gamma;\n> > +               LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> > +       }\n> > +\n> > +       frameContext.gamma = context.activeState.gamma;\n> > +}\n> > +\n> >  /**\n> >   * \\copydoc libcamera::ipa::Algorithm::prepare\n> >   */\n> > @@ -78,19 +118,36 @@ void Gamma::prepare([[maybe_unused]] IPAContext &context,\n> >         ASSERT(context.hw->numGammaOutSamples ==\n> >                RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n> >\n> > -       if (frame > 0)\n> > -               return;\n> > +       if (frame == 0 || std::fabs(frameContext.gamma - gamma_) > 0.001) {\n>\n> Can you invert the logic here so we still return early if there's no\n> work todo - and lower the indent on the code that follows?\n>\n> > +               gamma_ = frameContext.gamma;\n>\n> Is gamma_ actually used as a private context store outside of this now?\n> Should it be a local alias if it's just to shortent frameContext.gamma,\n> or can that be used directly?\n\nSeconded\n\n>\n> That definitely makes me think merging 1/4 and 4/4 would make this a\n> cleaner patch. I'm not sure the split gives us much here, except meaning\n> I have to have both patches open side by side to review.\n>\n>\n> > +\n> > +               int x = 0;\n> > +               for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> > +                       gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > +                       x += segments[i];\n> > +               }\n\nThis explains to me why we're re-programming the curve (question I had\nin 1/4)\n\n> >\n> > -       int x = 0;\n> > -       for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> > -               gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > -               x += segments[i];\n> > +               params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n\nAnd this also explains the question I had about using the logarithimc\ncurve instead of the equidistant segments one\n\n> > +               params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > +\n> > +               /* It is unclear why these bits need to be set more than once.\n> > +                * Setting them only on frame 0 didn't apply gamma.\n>\n> I think that's probably expected behaviour. these flags tell the kernel\n> that there is an update to the configuration structures, and for it to\n> read the update and apply it.\n>\n> I would think the comment can be dropped here.\n>\n> > +                */\n> > +               params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > +               params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> >         }\n> > +}\n> >\n> > -       params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> > -       params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > -       params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> > -       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::process\n> > + */\n> > +void Gamma::process([[maybe_unused]] IPAContext &context,\n> > +                   [[maybe_unused]] const uint32_t frame,\n> > +                   IPAFrameContext &frameContext,\n> > +                   [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > +                   ControlList &metadata)\n> > +{\n> > +       metadata.set(controls::Gamma, frameContext.gamma);\n> >  }\n> >\n> >  REGISTER_IPA_ALGORITHM(Gamma, \"Gamma\")\n> > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> > index fe7caba3..f2142b55 100644\n> > --- a/src/ipa/rkisp1/algorithms/goc.h\n> > +++ b/src/ipa/rkisp1/algorithms/goc.h\n> > @@ -20,12 +20,21 @@ public:\n> >         ~Gamma() = default;\n> >\n> >         int init(IPAContext &context, const YamlObject &tuningData) override;\n> > +       int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > +       void queueRequest(IPAContext &context,\n> > +                         const uint32_t frame,\n> > +                         IPAFrameContext &frameContext,\n> > +                         const ControlList &controls) override;\n> >         void prepare(IPAContext &context, const uint32_t frame,\n> >                      IPAFrameContext &frameContext,\n> >                      rkisp1_params_cfg *params) override;\n> > +       void process(IPAContext &context, const uint32_t frame,\n> > +                    IPAFrameContext &frameContext,\n> > +                    const rkisp1_stat_buffer *stats,\n> > +                    ControlList &metadata) override;\n> >\n> >  private:\n> > -       float gamma_ = 2.2;\n> > +       float gamma_;\n\nAh ok. As Kieran suggest I presume you can drop gamma_ and make it a\nstatic constexpr flost kGammaSRGB = 2.2; (or any other name you like)\n\n> >  };\n> >\n> >  } /* namespace ipa::rkisp1::algorithms */\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index bd02a7a2..5252e222 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -104,6 +104,8 @@ struct IPAActiveState {\n> >                 uint8_t denoise;\n> >                 uint8_t sharpness;\n> >         } filter;\n> > +\n> > +       double gamma;\n> >  };\n> >\n> >  struct IPAFrameContext : public FrameContext {\n> > @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {\n> >                 uint32_t exposure;\n> >                 double gain;\n> >         } sensor;\n> > +\n> > +       double gamma;\n> >  };\n> >\n> >  struct IPAContext {\n> > --\n> > 2.40.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4B3B5BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 23 May 2024 07:55:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B38E6349A;\n\tThu, 23 May 2024 09:55:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 174EB63495\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 23 May 2024 09:55:01 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 91F3FD01;\n\tThu, 23 May 2024 09:54:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IE5WWA04\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716450887;\n\tbh=vT+amop73XNrsMYlCpPNCAMQxAuddoFEcu/bE41ErSU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IE5WWA04VoAX++yXb+bbRBxM5d+CeYxC5ddVmqMbEHuceRQJBOtZtH2GvZuL1nQGY\n\tRHtuT6CrgKEtQQ96YAh+X7+F2203QZSjUTk4OvIuZCCywM5nOzoZNS5ODIwDb2/Si8\n\tO0lmEbnDi0LuISgqTgn98fAhrReM1Ye+Rw4oTxYA=","Date":"Thu, 23 May 2024 09:54:57 +0200","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","Message-ID":"<hcqylywlnhr5ab2dz7sdfgdxva7tw2mwcdc66udovizxb637da@tdtatlwhirk7>","References":"<20240522145438.436688-1-stefan.klug@ideasonboard.com>\n\t<20240522145438.436688-5-stefan.klug@ideasonboard.com>\n\t<171641761895.3019175.14878933535706187617@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171641761895.3019175.14878933535706187617@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29609,"web_url":"https://patchwork.libcamera.org/comment/29609/","msgid":"<20240523084120.solzbgvan6khiyqi@jasper>","date":"2024-05-23T08:41:20","subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo, hi Kieran,\n\nthanks for the review.\n\nOn Thu, May 23, 2024 at 09:54:57AM +0200, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Wed, May 22, 2024 at 11:40:18PM GMT, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-05-22 15:54:38)\n> > > Add support for the gamma control on the rkisp1. This was tested on\n> > > an imx8mp.\n> >\n> > I think patch 1/4 and 4/4 could maybe be merged together, but reviewing\n> > independently for now.\n\nI split them in two, because I didn't know if we would come to an\nagreement on the control easy enough. The split allows to merge the base\nalgo and work on the user-interface later on.\n\n> >\n> >\n> > >\n> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > ---\n> > >  src/ipa/rkisp1/algorithms/goc.cpp | 77 +++++++++++++++++++++++++++----\n> > >  src/ipa/rkisp1/algorithms/goc.h   | 11 ++++-\n> > >  src/ipa/rkisp1/ipa_context.h      |  4 ++\n> > >  3 files changed, 81 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > index 6f313820..0b312e7a 100644\n> > > --- a/src/ipa/rkisp1/algorithms/goc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > @@ -11,6 +11,8 @@\n> > >  #include <libcamera/base/log.h>\n> > >  #include <libcamera/base/utils.h>\n> > >\n> > > +#include <libcamera/control_ids.h>\n> > > +\n> > >  #include \"libcamera/internal/yaml_parser.h\"\n> > >\n> > >  #include \"linux/rkisp1-config.h\"\n> > > @@ -41,6 +43,7 @@ namespace ipa::rkisp1::algorithms {\n> > >  LOG_DEFINE_CATEGORY(RkISP1Gamma)\n> > >\n> > >  Gamma::Gamma()\n> > > +       : gamma_(0)\n> > >  {\n> > >  }\n> > >\n> > > @@ -50,6 +53,8 @@ Gamma::Gamma()\n> > >  int Gamma::init([[maybe_unused]] IPAContext &context,\n> > >                 [[maybe_unused]] const YamlObject &tuningData)\n> > >  {\n> > > +       context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> > > +\n> > >         if (context.hw->numGammaOutSamples !=\n> > >             RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n> > >                 LOG(RkISP1Gamma, Error)\n> > > @@ -60,6 +65,41 @@ int Gamma::init([[maybe_unused]] IPAContext &context,\n> > >         return 0;\n> > >  }\n> > >\n> > > +/**\n> > > + * \\brief Configure the Gamma given a configInfo\n> > > + * \\param[in] context The shared IPA context\n> > > + * \\param[in] configInfo The IPA configuration data\n> > > + *\n> > > + * \\return 0\n> > > + */\n> > > +int Gamma::configure(IPAContext &context,\n> > > +                    [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > > +{\n> > > +       context.activeState.gamma = 2.2;\n> \n> Should 2.2 be defined as a constant value (and iirc you have it defined in 1/4)\n> \n\nAck\n\n> \n> > > +       return 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > + */\n> > > +void Gamma::queueRequest([[maybe_unused]] IPAContext &context,\n> > > +                        [[maybe_unused]] const uint32_t frame,\n> > > +                        IPAFrameContext &frameContext,\n> > > +                        const ControlList &controls)\n> > > +{\n> > > +       const auto &gamma = controls.get(controls::Gamma);\n> > > +       if (gamma) {\n> > > +               /* \\todo This is not correct as it updates the current state with a\n> \n>                    /*\n>                     * Multiline\n>                     * comments\n>                     */\n> \n> > > +                * future value. But it does no harm at the moment an allows us to\n> > > +                * track the last active gamma\n> >\n> > I'm not sure if this is a todo. I kind of think it's correct?\n\nThis fits to the question below, if gamma_ is a private context store. I\nactually used gamma_ to represent the current state of the hardware.\nWith pfc in mind and the way we use context.activeState, it represents a\n*future* state of the hardware (Think of queuing 100 requests in\nadvance). I don't know if this was the intended usage. But we need to\ntrack the current state somewhere to know if we have to reconfigure the\nhardware.\n\n> >\n> >\n> > > +                */\n> > > +               context.activeState.gamma = *gamma;\n> > > +               LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> > > +       }\n> > > +\n> > > +       frameContext.gamma = context.activeState.gamma;\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\copydoc libcamera::ipa::Algorithm::prepare\n> > >   */\n> > > @@ -78,19 +118,36 @@ void Gamma::prepare([[maybe_unused]] IPAContext &context,\n> > >         ASSERT(context.hw->numGammaOutSamples ==\n> > >                RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n> > >\n> > > -       if (frame > 0)\n> > > -               return;\n> > > +       if (frame == 0 || std::fabs(frameContext.gamma - gamma_) > 0.001) {\n> >\n> > Can you invert the logic here so we still return early if there's no\n> > work todo - and lower the indent on the code that follows?\n> >\n> > > +               gamma_ = frameContext.gamma;\n> >\n> > Is gamma_ actually used as a private context store outside of this now?\n> > Should it be a local alias if it's just to shortent frameContext.gamma,\n> > or can that be used directly?\n> \n> Seconded\n\nAs noted above, it was used to see if the hardware state differs from\nthe state requested for that frame. Mabe I'm missing something\nconceptionally here...\n\n> \n> >\n> > That definitely makes me think merging 1/4 and 4/4 would make this a\n> > cleaner patch. I'm not sure the split gives us much here, except meaning\n> > I have to have both patches open side by side to review.\n> >\n> >\n> > > +\n> > > +               int x = 0;\n> > > +               for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> > > +                       gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > > +                       x += segments[i];\n> > > +               }\n> \n> This explains to me why we're re-programming the curve (question I had\n> in 1/4)\n> \n> > >\n> > > -       int x = 0;\n> > > -       for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> > > -               gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > > -               x += segments[i];\n> > > +               params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> \n> And this also explains the question I had about using the logarithimc\n> curve instead of the equidistant segments one\n> \n> > > +               params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > +\n> > > +               /* It is unclear why these bits need to be set more than once.\n> > > +                * Setting them only on frame 0 didn't apply gamma.\n> >\n> > I think that's probably expected behaviour. these flags tell the kernel\n> > that there is an update to the configuration structures, and for it to\n> > read the update and apply it.\n\nI thought the module_en* registers only specify which modules are\nenabled and that I could reconfigure them without having to touch the\nen* bits again.\n\n> >\n> > I would think the comment can be dropped here.\n\nI'm fine with that.\n\nCheers,\nStefan\n\n> >\n> > > +                */\n> > > +               params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > +               params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> > >         }\n> > > +}\n> > >\n> > > -       params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> > > -       params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > -       params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > -       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > + */\n> > > +void Gamma::process([[maybe_unused]] IPAContext &context,\n> > > +                   [[maybe_unused]] const uint32_t frame,\n> > > +                   IPAFrameContext &frameContext,\n> > > +                   [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > +                   ControlList &metadata)\n> > > +{\n> > > +       metadata.set(controls::Gamma, frameContext.gamma);\n> > >  }\n> > >\n> > >  REGISTER_IPA_ALGORITHM(Gamma, \"Gamma\")\n> > > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> > > index fe7caba3..f2142b55 100644\n> > > --- a/src/ipa/rkisp1/algorithms/goc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/goc.h\n> > > @@ -20,12 +20,21 @@ public:\n> > >         ~Gamma() = default;\n> > >\n> > >         int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > +       int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > > +       void queueRequest(IPAContext &context,\n> > > +                         const uint32_t frame,\n> > > +                         IPAFrameContext &frameContext,\n> > > +                         const ControlList &controls) override;\n> > >         void prepare(IPAContext &context, const uint32_t frame,\n> > >                      IPAFrameContext &frameContext,\n> > >                      rkisp1_params_cfg *params) override;\n> > > +       void process(IPAContext &context, const uint32_t frame,\n> > > +                    IPAFrameContext &frameContext,\n> > > +                    const rkisp1_stat_buffer *stats,\n> > > +                    ControlList &metadata) override;\n> > >\n> > >  private:\n> > > -       float gamma_ = 2.2;\n> > > +       float gamma_;\n> \n> Ah ok. As Kieran suggest I presume you can drop gamma_ and make it a\n> static constexpr flost kGammaSRGB = 2.2; (or any other name you like)\n> \n> > >  };\n> > >\n> > >  } /* namespace ipa::rkisp1::algorithms */\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index bd02a7a2..5252e222 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -104,6 +104,8 @@ struct IPAActiveState {\n> > >                 uint8_t denoise;\n> > >                 uint8_t sharpness;\n> > >         } filter;\n> > > +\n> > > +       double gamma;\n> > >  };\n> > >\n> > >  struct IPAFrameContext : public FrameContext {\n> > > @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {\n> > >                 uint32_t exposure;\n> > >                 double gain;\n> > >         } sensor;\n> > > +\n> > > +       double gamma;\n> > >  };\n> > >\n> > >  struct IPAContext {\n> > > --\n> > > 2.40.1\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50116BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 23 May 2024 08:41:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F0F26349A;\n\tThu, 23 May 2024 10:41:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B812563495\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 23 May 2024 10:41:23 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:6e2:db60:9aa4:f6ee])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4CA5521D;\n\tThu, 23 May 2024 10:41:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oN6aYSMn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716453670;\n\tbh=5uPJya0jemSsL2Vvhge4qtu2APETszJ8gEEQWLuUzfE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oN6aYSMnmp28fEJP2fimSwQZ7Ez3sXsfQ6eGcZNnA+PiNIGppvLM0eAh3AIvoSWA/\n\tubkxv0j9GiJsdH04DHZC8NZ+V5auYWna137Ukm6/6dDyhRDirEfC5MLZrehf1QFnci\n\tpJzRZ7+MZ/n8SiaBe2HoMySttwaGSy3e8BxBLRhU=","Date":"Thu, 23 May 2024 10:41:20 +0200","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","Message-ID":"<20240523084120.solzbgvan6khiyqi@jasper>","References":"<20240522145438.436688-1-stefan.klug@ideasonboard.com>\n\t<20240522145438.436688-5-stefan.klug@ideasonboard.com>\n\t<171641761895.3019175.14878933535706187617@ping.linuxembedded.co.uk>\n\t<hcqylywlnhr5ab2dz7sdfgdxva7tw2mwcdc66udovizxb637da@tdtatlwhirk7>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<hcqylywlnhr5ab2dz7sdfgdxva7tw2mwcdc66udovizxb637da@tdtatlwhirk7>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":29611,"web_url":"https://patchwork.libcamera.org/comment/29611/","msgid":"<171646672433.1456831.4949360964670822962@ping.linuxembedded.co.uk>","date":"2024-05-23T12:18:44","subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-05-23 09:41:20)\n> Hi Jacopo, hi Kieran,\n> \n> thanks for the review.\n> \n> On Thu, May 23, 2024 at 09:54:57AM +0200, Jacopo Mondi wrote:\n> > Hi Stefan\n> > \n> > On Wed, May 22, 2024 at 11:40:18PM GMT, Kieran Bingham wrote:\n> > > Quoting Stefan Klug (2024-05-22 15:54:38)\n> > > > Add support for the gamma control on the rkisp1. This was tested on\n> > > > an imx8mp.\n> > >\n> > > I think patch 1/4 and 4/4 could maybe be merged together, but reviewing\n> > > independently for now.\n> \n> I split them in two, because I didn't know if we would come to an\n> agreement on the control easy enough. The split allows to merge the base\n> algo and work on the user-interface later on.\n\nThat does make sense, as we need to set the gamma handling in the\ni.MX8MP regardless of whether we add the control indeed.\n\nI guess it depends on what others think about the Gamma control...\n\n\n> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > > ---\n> > > >  src/ipa/rkisp1/algorithms/goc.cpp | 77 +++++++++++++++++++++++++++----\n> > > >  src/ipa/rkisp1/algorithms/goc.h   | 11 ++++-\n> > > >  src/ipa/rkisp1/ipa_context.h      |  4 ++\n> > > >  3 files changed, 81 insertions(+), 11 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > > index 6f313820..0b312e7a 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/goc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> > > > @@ -11,6 +11,8 @@\n> > > >  #include <libcamera/base/log.h>\n> > > >  #include <libcamera/base/utils.h>\n> > > >\n> > > > +#include <libcamera/control_ids.h>\n> > > > +\n> > > >  #include \"libcamera/internal/yaml_parser.h\"\n> > > >\n> > > >  #include \"linux/rkisp1-config.h\"\n> > > > @@ -41,6 +43,7 @@ namespace ipa::rkisp1::algorithms {\n> > > >  LOG_DEFINE_CATEGORY(RkISP1Gamma)\n> > > >\n> > > >  Gamma::Gamma()\n> > > > +       : gamma_(0)\n> > > >  {\n> > > >  }\n> > > >\n> > > > @@ -50,6 +53,8 @@ Gamma::Gamma()\n> > > >  int Gamma::init([[maybe_unused]] IPAContext &context,\n> > > >                 [[maybe_unused]] const YamlObject &tuningData)\n> > > >  {\n> > > > +       context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);\n> > > > +\n> > > >         if (context.hw->numGammaOutSamples !=\n> > > >             RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {\n> > > >                 LOG(RkISP1Gamma, Error)\n> > > > @@ -60,6 +65,41 @@ int Gamma::init([[maybe_unused]] IPAContext &context,\n> > > >         return 0;\n> > > >  }\n> > > >\n> > > > +/**\n> > > > + * \\brief Configure the Gamma given a configInfo\n> > > > + * \\param[in] context The shared IPA context\n> > > > + * \\param[in] configInfo The IPA configuration data\n> > > > + *\n> > > > + * \\return 0\n> > > > + */\n> > > > +int Gamma::configure(IPAContext &context,\n> > > > +                    [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > > > +{\n> > > > +       context.activeState.gamma = 2.2;\n> > \n> > Should 2.2 be defined as a constant value (and iirc you have it defined in 1/4)\n> > \n> \n> Ack\n> \n> > \n> > > > +       return 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > > > + */\n> > > > +void Gamma::queueRequest([[maybe_unused]] IPAContext &context,\n> > > > +                        [[maybe_unused]] const uint32_t frame,\n> > > > +                        IPAFrameContext &frameContext,\n> > > > +                        const ControlList &controls)\n> > > > +{\n> > > > +       const auto &gamma = controls.get(controls::Gamma);\n> > > > +       if (gamma) {\n> > > > +               /* \\todo This is not correct as it updates the current state with a\n> > \n> >                    /*\n> >                     * Multiline\n> >                     * comments\n> >                     */\n> > \n> > > > +                * future value. But it does no harm at the moment an allows us to\n> > > > +                * track the last active gamma\n> > >\n> > > I'm not sure if this is a todo. I kind of think it's correct?\n> \n> This fits to the question below, if gamma_ is a private context store. I\n> actually used gamma_ to represent the current state of the hardware.\n> With pfc in mind and the way we use context.activeState, it represents a\n> *future* state of the hardware (Think of queuing 100 requests in\n> advance). I don't know if this was the intended usage. But we need to\n> track the current state somewhere to know if we have to reconfigure the\n> hardware.\n\nThe thing is at the time the request is queued, we also need to be\nworking on what the future state of the hardware will be, *not* what the\ncurrent state of the hardware is!\n\nIt's definitely a tricky balance as the queueRequest() time frame can be\nmany frames ahead of what is actually being processed. But that's where\nthe FrameContext should then balance out what context is relevant for\n/that/ frame time, vs what the most current state is.\n\nI think for a control like gamma where there is no 'automatic\nprocessing' based on statistics we're fine here - but indeed if we have\nauto algorithsm there are 3 event points to potentially track:\n\n - queueRequest (sets the future)\n - prepare (controls the immediate future)\n - processStatistics (processes the past, helps to determine the future)\n\nI do think it's up to each algorithm to decide how it uses the\nActiveState and it's own private state to meet those requirements, but\nmaybe we should already do better at what the current algorithms use and\nhow.\n\n--\nKieran\n\n\n> \n> > >\n> > >\n> > > > +                */\n> > > > +               context.activeState.gamma = *gamma;\n> > > > +               LOG(RkISP1Gamma, Debug) << \"Set gamma to \" << *gamma;\n> > > > +       }\n> > > > +\n> > > > +       frameContext.gamma = context.activeState.gamma;\n> > > > +}\n> > > > +\n> > > >  /**\n> > > >   * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > >   */\n> > > > @@ -78,19 +118,36 @@ void Gamma::prepare([[maybe_unused]] IPAContext &context,\n> > > >         ASSERT(context.hw->numGammaOutSamples ==\n> > > >                RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n> > > >\n> > > > -       if (frame > 0)\n> > > > -               return;\n> > > > +       if (frame == 0 || std::fabs(frameContext.gamma - gamma_) > 0.001) {\n> > >\n> > > Can you invert the logic here so we still return early if there's no\n> > > work todo - and lower the indent on the code that follows?\n> > >\n> > > > +               gamma_ = frameContext.gamma;\n> > >\n> > > Is gamma_ actually used as a private context store outside of this now?\n> > > Should it be a local alias if it's just to shortent frameContext.gamma,\n> > > or can that be used directly?\n> > \n> > Seconded\n> \n> As noted above, it was used to see if the hardware state differs from\n> the state requested for that frame. Mabe I'm missing something\n> conceptionally here...\n> \n> > \n> > >\n> > > That definitely makes me think merging 1/4 and 4/4 would make this a\n> > > cleaner patch. I'm not sure the split gives us much here, except meaning\n> > > I have to have both patches open side by side to review.\n> > >\n> > >\n> > > > +\n> > > > +               int x = 0;\n> > > > +               for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> > > > +                       gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > > > +                       x += segments[i];\n> > > > +               }\n> > \n> > This explains to me why we're re-programming the curve (question I had\n> > in 1/4)\n> > \n> > > >\n> > > > -       int x = 0;\n> > > > -       for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {\n> > > > -               gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;\n> > > > -               x += segments[i];\n> > > > +               params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> > \n> > And this also explains the question I had about using the logarithimc\n> > curve instead of the equidistant segments one\n> > \n> > > > +               params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > > +\n> > > > +               /* It is unclear why these bits need to be set more than once.\n> > > > +                * Setting them only on frame 0 didn't apply gamma.\n> > >\n> > > I think that's probably expected behaviour. these flags tell the kernel\n> > > that there is an update to the configuration structures, and for it to\n> > > read the update and apply it.\n> \n> I thought the module_en* registers only specify which modules are\n> enabled and that I could reconfigure them without having to touch the\n> en* bits again.\n> \n> > >\n> > > I would think the comment can be dropped here.\n> \n> I'm fine with that.\n> \n> Cheers,\n> Stefan\n> \n> > >\n> > > > +                */\n> > > > +               params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > > +               params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > >         }\n> > > > +}\n> > > >\n> > > > -       params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> > > > -       params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > > -       params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > > -       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > > + */\n> > > > +void Gamma::process([[maybe_unused]] IPAContext &context,\n> > > > +                   [[maybe_unused]] const uint32_t frame,\n> > > > +                   IPAFrameContext &frameContext,\n> > > > +                   [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > +                   ControlList &metadata)\n> > > > +{\n> > > > +       metadata.set(controls::Gamma, frameContext.gamma);\n> > > >  }\n> > > >\n> > > >  REGISTER_IPA_ALGORITHM(Gamma, \"Gamma\")\n> > > > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> > > > index fe7caba3..f2142b55 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/goc.h\n> > > > +++ b/src/ipa/rkisp1/algorithms/goc.h\n> > > > @@ -20,12 +20,21 @@ public:\n> > > >         ~Gamma() = default;\n> > > >\n> > > >         int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > +       int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > > > +       void queueRequest(IPAContext &context,\n> > > > +                         const uint32_t frame,\n> > > > +                         IPAFrameContext &frameContext,\n> > > > +                         const ControlList &controls) override;\n> > > >         void prepare(IPAContext &context, const uint32_t frame,\n> > > >                      IPAFrameContext &frameContext,\n> > > >                      rkisp1_params_cfg *params) override;\n> > > > +       void process(IPAContext &context, const uint32_t frame,\n> > > > +                    IPAFrameContext &frameContext,\n> > > > +                    const rkisp1_stat_buffer *stats,\n> > > > +                    ControlList &metadata) override;\n> > > >\n> > > >  private:\n> > > > -       float gamma_ = 2.2;\n> > > > +       float gamma_;\n> > \n> > Ah ok. As Kieran suggest I presume you can drop gamma_ and make it a\n> > static constexpr flost kGammaSRGB = 2.2; (or any other name you like)\n> > \n> > > >  };\n> > > >\n> > > >  } /* namespace ipa::rkisp1::algorithms */\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index bd02a7a2..5252e222 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -104,6 +104,8 @@ struct IPAActiveState {\n> > > >                 uint8_t denoise;\n> > > >                 uint8_t sharpness;\n> > > >         } filter;\n> > > > +\n> > > > +       double gamma;\n> > > >  };\n> > > >\n> > > >  struct IPAFrameContext : public FrameContext {\n> > > > @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {\n> > > >                 uint32_t exposure;\n> > > >                 double gain;\n> > > >         } sensor;\n> > > > +\n> > > > +       double gamma;\n> > > >  };\n> > > >\n> > > >  struct IPAContext {\n> > > > --\n> > > > 2.40.1\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9AD15BDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 23 May 2024 12:18:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A193063497;\n\tThu, 23 May 2024 14:18:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8C3661A49\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 23 May 2024 14:18:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 227BFD49;\n\tThu, 23 May 2024 14:18:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"elwtFMgX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716466714;\n\tbh=p7JzQzYfT1cU/IeuxIxPR+dHDwZU2kqCX4kJ8IX2HUI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=elwtFMgXez/5rayp7rpfI2Y13pdyZxovKvU/3iWZuTyX+rVC1ueP6TUPdzqp8fEJc\n\tSs3o6nvklhrFOfu3ntpMVQyZkeAXFsFwGElX+VNwuP7+fnM/H2s5sgFxE49A2dz76m\n\tgBwe/kq1OtecsqnZXJmGOSRWsbCRSDscc+lHBduo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240523084120.solzbgvan6khiyqi@jasper>","References":"<20240522145438.436688-1-stefan.klug@ideasonboard.com>\n\t<20240522145438.436688-5-stefan.klug@ideasonboard.com>\n\t<171641761895.3019175.14878933535706187617@ping.linuxembedded.co.uk>\n\t<hcqylywlnhr5ab2dz7sdfgdxva7tw2mwcdc66udovizxb637da@tdtatlwhirk7>\n\t<20240523084120.solzbgvan6khiyqi@jasper>","Subject":"Re: [PATCH v2 4/4] pipeline: rkisp1: Implement gamma control","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tStefan Klug <stefan.klug@ideasonboard.com>","Date":"Thu, 23 May 2024 13:18:44 +0100","Message-ID":"<171646672433.1456831.4949360964670822962@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]