[{"id":23825,"web_url":"https://patchwork.libcamera.org/comment/23825/","msgid":"<20220712071801.GC2364006@pyrite.rasen.tech>","date":"2022-07-12T07:18:01","subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: rkisp1: Add support of Gamma\n\tSensor Linearization control","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Florian,\n\nOn Wed, Jun 22, 2022 at 05:19:15PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> The GammaSensorLinearization algorithm improves the image dynamic using\n> a coefficient table in the YAML tuning file.\n> \n> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> ---\n>  src/ipa/rkisp1/algorithms/gsl.cpp     | 142 ++++++++++++++++++++++++++\n>  src/ipa/rkisp1/algorithms/gsl.h       |  38 +++++++\n>  src/ipa/rkisp1/algorithms/meson.build |   1 +\n>  src/ipa/rkisp1/data/ov5640.yaml       |   6 ++\n>  src/ipa/rkisp1/rkisp1.cpp             |   1 +\n>  5 files changed, 188 insertions(+)\n>  create mode 100644 src/ipa/rkisp1/algorithms/gsl.cpp\n>  create mode 100644 src/ipa/rkisp1/algorithms/gsl.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp\n> new file mode 100644\n> index 00000000..7ff3d360\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/gsl.cpp\n> @@ -0,0 +1,142 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * gsl.cpp - RkISP1 Gamma Sensor Linearization control\n> + */\n> +\n> +#include \"gsl.h\"\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +#include \"linux/rkisp1-config.h\"\n> +\n> +/**\n> + * \\file gsl.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class GammaSensorLinearization\n> + * \\brief RkISP1 Gamma Sensor Linearization control\n> + *\n> + * This algorithm improves the image dynamic using a coefficient table in the\n> + * Yaml tuning file, the table contains:\n> + *  - 'x-intervals': These values corresponds to GAMMA_DX_n values (from\n> + *    GAMMA_DX_0 to GAMMA_DX_16 fields in RKISP1_CIF_ISP_GAMMA_DX_LO and\n\ns/DX_0/DX_1/ ?\n\n> + *    RKISP1_CIF_ISP_GAMMA_DX_HI rkisp1 registers).\n> + *    Note: rkisp1 use each value to compute a X interval equal to 2^(value+4)\n\ns/use/uses/\n\ns/a X/an X/\n\n> + *  - 'y' values for each component (red, green , blue) on 12 bits resolution.\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Gsl)\n> +\n> +#define DEGAMMA_X_INTERVALS 16\n> +\n> +GammaSensorLinearization::GammaSensorLinearization()\n> +\t: tuningParameters_(false)\n\nI would call this tuned_ or something along this lines, as this variable\nsignifies if the tuning parameters have been loaded correctly or not,\nand does not actually contain the tuning parameters as the varaible name\nsuggests.\n\nAlso, what happens if no tuning file is provided? Does the algorithm\njust... not work (as in it doesn't do anything)? Is that better than\nrunning on some default tuning values? (I feel like I saw discussion on\nthis topic elsewhere but I'm just asking it again :S)\n\n> +{\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> +\t\t\t\t   const YamlObject &tuningData)\n> +{\n> +\tstd::vector<uint16_t> xIntervals_ = tuningData[\"x-intervals\"].getList<uint16_t>();\n> +\tif (xIntervals_.size() != DEGAMMA_X_INTERVALS) {\n> +\t\tLOG(RkISP1Gsl, Error)\n> +\t\t\t<< \"Issue while parsing 'x'\"\n> +\t\t\t<< \"in tuning file (list size:\"\n> +\t\t\t<< xIntervals_.size() << \"/\"\n> +\t\t\t<< DEGAMMA_X_INTERVALS << \")\";\n\nI would prefer slightly more verbosity, like printing which/where the\ntuning file is, and saying explicitly \"expected\". Also I think you're\nmissing spaces in the string concatenation.\n\nSame for the other error messages below.\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* Compute gammaDx_ intervals from xIntervals_ values */\n> +\tgammaDx_[0] = 0;\n> +\tgammaDx_[1] = 0;\n> +\tfor (int i = 0; i < DEGAMMA_X_INTERVALS; ++i) {\n> +\t\tgammaDx_[i / 8] |= (xIntervals_[i] & 0x07) << ((i % 8) * 4);\n> +\t}\n\nI don't think you need the braces.\n\nNeat math trick :)\n\n\nPaul\n\n> +\n> +\tconst YamlObject &yObject = tuningData[\"y\"];\n> +\n> +\tif (!yObject.isDictionary()) {\n> +\t\tLOG(RkISP1Gsl, Error)\n> +\t\t\t<< \"Issue while parsing 'y' in tuning file: \"\n> +\t\t\t<< \"entry must be a dictionary\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcurveYr_ = yObject[\"red\"].getList<uint16_t>();\n> +\tif (curveYr_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> +\t\tLOG(RkISP1Gsl, Error)\n> +\t\t\t<< \"Issue while parsing 'y:red'\"\n> +\t\t\t<< \"in tuning file (list size: \"\n> +\t\t\t<< curveYr_.size() << \"/\"\n> +\t\t\t<< RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << \")\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcurveYg_ = yObject[\"green\"].getList<uint16_t>();\n> +\tif (curveYg_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> +\t\tLOG(RkISP1Gsl, Error)\n> +\t\t\t<< \"Issue while parsing 'y:green'\"\n> +\t\t\t<< \"in tuning file (list size: \"\n> +\t\t\t<< curveYg_.size() << \"/\"\n> +\t\t\t<< RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << \")\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcurveYb_ = yObject[\"blue\"].getList<uint16_t>();\n> +\tif (curveYb_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> +\t\tLOG(RkISP1Gsl, Error)\n> +\t\t\t<< \"Issue while parsing 'y:blue'\"\n> +\t\t\t<< \"in tuning file (list size: \"\n> +\t\t\t<< curveYb_.size() << \"/\"\n> +\t\t\t<< RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << \")\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\ttuningParameters_ = true;\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void GammaSensorLinearization::prepare(IPAContext &context,\n> +\t\t\t\t       rkisp1_params_cfg *params)\n> +{\n> +\tif (context.frameContext.frameCount > 0)\n> +\t\treturn;\n> +\n> +\tif (!tuningParameters_)\n> +\t\treturn;\n> +\n> +\tparams->others.sdg_config.xa_pnts.gamma_dx0 = gammaDx_[0];\n> +\tparams->others.sdg_config.xa_pnts.gamma_dx1 = gammaDx_[1];\n> +\n> +\tstd::copy(curveYr_.begin(), curveYr_.end(),\n> +\t\t  params->others.sdg_config.curve_r.gamma_y);\n> +\tstd::copy(curveYg_.begin(), curveYg_.end(),\n> +\t\t  params->others.sdg_config.curve_g.gamma_y);\n> +\tstd::copy(curveYb_.begin(), curveYb_.end(),\n> +\t\t  params->others.sdg_config.curve_b.gamma_y);\n> +\n> +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_SDG;\n> +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_SDG;\n> +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_SDG;\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(GammaSensorLinearization)\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/gsl.h b/src/ipa/rkisp1/algorithms/gsl.h\n> new file mode 100644\n> index 00000000..df16a89b\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/gsl.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * gsl.h - RkISP1 Gamma Sensor Linearization control\n> + */\n> +\n> +#pragma once\n> +\n> +#include <linux/rkisp1-config.h>\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +struct IPACameraSensorInfo;\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class GammaSensorLinearization : public Algorithm\n> +{\n> +public:\n> +\tGammaSensorLinearization();\n> +\t~GammaSensorLinearization() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> +\tvoid prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n> +\n> +private:\n> +\tbool tuningParameters_;\n> +\tuint32_t gammaDx_[2];\n> +\tstd::vector<uint16_t> curveYr_;\n> +\tstd::vector<uint16_t> curveYg_;\n> +\tstd::vector<uint16_t> curveYb_;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 7ec53d89..0597c353 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -4,4 +4,5 @@ rkisp1_ipa_algorithms = files([\n>      'agc.cpp',\n>      'awb.cpp',\n>      'blc.cpp',\n> +    'gsl.cpp',\n>  ])\n> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> index 232d8ae8..6cb84ed9 100644\n> --- a/src/ipa/rkisp1/data/ov5640.yaml\n> +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> @@ -10,4 +10,10 @@ algorithms:\n>        Gr: 256\n>        Gb: 256\n>        B:  256\n> +  - GammaSensorLinearization:\n> +      x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]\n> +      y:\n> +        red:   [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]\n> +        green: [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]\n> +        blue:  [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]\n>  ...\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index d052954e..622a0d61 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -31,6 +31,7 @@\n>  #include \"algorithms/algorithm.h\"\n>  #include \"algorithms/awb.h\"\n>  #include \"algorithms/blc.h\"\n> +#include \"algorithms/gsl.h\"\n>  #include \"libipa/camera_sensor_helper.h\"\n>  \n>  #include \"ipa_context.h\"\n> -- \n> 2.34.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 EDA56BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 12 Jul 2022 07:18:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64DF963312;\n\tTue, 12 Jul 2022 09:18:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC27D6330E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Jul 2022 09:18:08 +0200 (CEST)","from pyrite.rasen.tech (softbank036240121080.bbtec.net\n\t[36.240.121.80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57AE130B;\n\tTue, 12 Jul 2022 09:18:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657610290;\n\tbh=uJlzPk3QbdG0SG44acXec848PRr3LZjHozBOm+1r2ws=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=le6b36LByu5RolAwFHn0IFPoMUKYqh/Xx5qJNj9L+MWISsobCgfqLLqW6CiBR6+oE\n\tlLnhjs9DgUqOdPCJbbkJb4pYmoSVi9kSrcIUsWQcghUoHFukHdSq7X8420Tgt1av34\n\tHBlDOXN3B/0exjXoAeInPC6XrRSt3wB3Mgtt5Zh8uyB6Qu62Qgodrp1dCCuW8Q0/Z/\n\tlNwZTDRM+FVWftO/mNr3Ljr3PtOiRTslh4n+2n3JGFiPepKFWkq4ofhCUW9pyEiQAs\n\tklyQHb3Px0sWg9k0Sm7MSKOfJQmCBqxzARnhu1hy4JKZc40HZ0q3gQy5ohvxkPM6Rz\n\twPrLDJoXFqc2Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657610288;\n\tbh=uJlzPk3QbdG0SG44acXec848PRr3LZjHozBOm+1r2ws=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gGBVjSnBPmCx8g4uaLQgJjPonl9LeACji7tpOCJIoIVrPyypzUq2HelzEJNEH9Toz\n\tgUQNQqUKQL48oaX1WUzpbmU5F/IwbqMnjad8O2tADel7FB/nT18NMpk9z23+Ax7HwK\n\tnv9DYRl3R5YMfwLwscPdVgxOwkDFbx7AONVa0Nt4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gGBVjSnB\"; dkim-atps=neutral","Date":"Tue, 12 Jul 2022 16:18:01 +0900","To":"Florian Sylvestre <fsylvestre@baylibre.com>","Message-ID":"<20220712071801.GC2364006@pyrite.rasen.tech>","References":"<20220622151918.451635-1-fsylvestre@baylibre.com>\n\t<20220622151918.451635-3-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220622151918.451635-3-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: rkisp1: Add support of Gamma\n\tSensor Linearization control","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>","From":"Paul Elder via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":23846,"web_url":"https://patchwork.libcamera.org/comment/23846/","msgid":"<Ys4K9viEF1NdwMiZ@pendragon.ideasonboard.com>","date":"2022-07-12T23:59:50","subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: rkisp1: Add support of Gamma\n\tSensor Linearization control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Jul 12, 2022 at 04:18:01PM +0900, Paul Elder via libcamera-devel wrote:\n> On Wed, Jun 22, 2022 at 05:19:15PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> > The GammaSensorLinearization algorithm improves the image dynamic using\n\nIt's not really about improving the image dynamic, but about\ncompensating the non-linearities of the camera sensor to obtain a linear\nimage for further processing.\n\n> > a coefficient table in the YAML tuning file.\n> > \n> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> > ---\n> >  src/ipa/rkisp1/algorithms/gsl.cpp     | 142 ++++++++++++++++++++++++++\n> >  src/ipa/rkisp1/algorithms/gsl.h       |  38 +++++++\n> >  src/ipa/rkisp1/algorithms/meson.build |   1 +\n> >  src/ipa/rkisp1/data/ov5640.yaml       |   6 ++\n> >  src/ipa/rkisp1/rkisp1.cpp             |   1 +\n> >  5 files changed, 188 insertions(+)\n> >  create mode 100644 src/ipa/rkisp1/algorithms/gsl.cpp\n> >  create mode 100644 src/ipa/rkisp1/algorithms/gsl.h\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp\n> > new file mode 100644\n> > index 00000000..7ff3d360\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/gsl.cpp\n> > @@ -0,0 +1,142 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021-2022, Ideas On Board\n> > + *\n> > + * gsl.cpp - RkISP1 Gamma Sensor Linearization control\n> > + */\n> > +\n> > +#include \"gsl.h\"\n> > +\n> > +#include <libcamera/base/log.h>\n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n\nPlease add a blank line here.\n\n> > +#include \"linux/rkisp1-config.h\"\n> > +\n> > +/**\n> > + * \\file gsl.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +/**\n> > + * \\class GammaSensorLinearization\n> > + * \\brief RkISP1 Gamma Sensor Linearization control\n> > + *\n> > + * This algorithm improves the image dynamic using a coefficient table in the\n> > + * Yaml tuning file, the table contains:\n\ns/Yaml/YAML/\ns/, the table/. The table/\n\n> > + *  - 'x-intervals': These values corresponds to GAMMA_DX_n values (from\n> > + *    GAMMA_DX_0 to GAMMA_DX_16 fields in RKISP1_CIF_ISP_GAMMA_DX_LO and\n> \n> s/DX_0/DX_1/ ?\n> \n> > + *    RKISP1_CIF_ISP_GAMMA_DX_HI rkisp1 registers).\n> > + *    Note: rkisp1 use each value to compute a X interval equal to 2^(value+4)\n> \n> s/use/uses/\n> \n> s/a X/an X/\n> \n> > + *  - 'y' values for each component (red, green , blue) on 12 bits resolution.\n\nI'd make it explicit that the curves are specified as piecewise linear\nfunctions.\n\n * This algorithm linearizes the sensor output to compensate the sensor\n * non-linearities by applying piecewise linear curves to the red, green and\n * blue channels.\n *\n * The curves are specified in the tuning data and defined using 17 points.\n *\n * - The X coordinates are expressed using 16 intervals, with the first point at\n *   X coordinate 0. Each interval is expressed as a 2-bit value DX (from\n *   GAMMA_DX_1 to GAMMA_DX_16), stored in the RKISP1_CIF_ISP_GAMMA_DX_LO and\n *   RKISP1_CIF_ISP_GAMMA_DX_HI registers. The real interval is equal to\n *   \\f$2^{dx+4}\\f$. X coordinates are shared between the red, green and blue\n *   curves.\n * \n * - The Y coordinates are specified as 17 values separately for the\n *   red, green and blue channels, with a 12-bit resolution. Each value must be\n *   in the [-2048, 2047] range compared to the previous value.\n\n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1Gsl)\n> > +\n> > +#define DEGAMMA_X_INTERVALS 16\n\nstatic constexpr unsigned int kDegammaXIntervals = 16;\n\n> > +\n> > +GammaSensorLinearization::GammaSensorLinearization()\n> > +\t: tuningParameters_(false)\n> \n> I would call this tuned_ or something along this lines, as this variable\n> signifies if the tuning parameters have been loaded correctly or not,\n> and does not actually contain the tuning parameters as the varaible name\n> suggests.\n\nOr maybe just \"valid_\" or \"loaded_\" ?\n\n> Also, what happens if no tuning file is provided? Does the algorithm\n> just... not work (as in it doesn't do anything)? Is that better than\n> running on some default tuning values? (I feel like I saw discussion on\n> this topic elsewhere but I'm just asking it again :S)\n\nAlgorithms are instantiated based on the tuning file, so if the tuning\nfile doesn't list a gsl algorithm, this class won't be instantiaed. See\nModule::createAlgorithm() in src/ipa/libipa/module.h.\n\nWe have an uncalibrated.yaml file that could list a gsl algorithm with\ndefault values, but that will just be a linear curve. Not enabling the\nalgorithm will disable the corresponding hardware block, which I think\nis better.\n\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > +\t\t\t\t   const YamlObject &tuningData)\n> > +{\n> > +\tstd::vector<uint16_t> xIntervals_ = tuningData[\"x-intervals\"].getList<uint16_t>();\n\nThis is not a class member variable, it should be named without the\ntrailing _.\n\n> > +\tif (xIntervals_.size() != DEGAMMA_X_INTERVALS) {\n> > +\t\tLOG(RkISP1Gsl, Error)\n> > +\t\t\t<< \"Issue while parsing 'x'\"\n> > +\t\t\t<< \"in tuning file (list size:\"\n> > +\t\t\t<< xIntervals_.size() << \"/\"\n> > +\t\t\t<< DEGAMMA_X_INTERVALS << \")\";\n> \n> I would prefer slightly more verbosity, like printing which/where the\n> tuning file is,\n\nI wouldn't add the file name here as we don't have access to the\ninformation, and it would be a bit painful to pass the file name around\neverywhere. We could however print the file name in IPARkISP1::init() in\ncase of errors.\n\n> and saying explicitly \"expected\". Also I think you're\n> missing spaces in the string concatenation.\n\nMaybe as follows ?\n\n\t\tLOG(RkISP1Gsl, Error)\n\t\t\t<< \"Invalid 'x' coordinates: expected \"\n\t\t\t<< kDegammaXIntervals << \" elements, got \"\n\t\t\t<< xIntervals.size();\n\n\n> Same for the other error messages below.\n> \n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/* Compute gammaDx_ intervals from xIntervals_ values */\n> > +\tgammaDx_[0] = 0;\n> > +\tgammaDx_[1] = 0;\n> > +\tfor (int i = 0; i < DEGAMMA_X_INTERVALS; ++i) {\n\nunsigned int i\n\n> > +\t\tgammaDx_[i / 8] |= (xIntervals_[i] & 0x07) << ((i % 8) * 4);\n> > +\t}\n> \n> I don't think you need the braces.\n\nIndeed.\n\n> Neat math trick :)\n> \n> > +\n> > +\tconst YamlObject &yObject = tuningData[\"y\"];\n> > +\n> > +\tif (!yObject.isDictionary()) {\n> > +\t\tLOG(RkISP1Gsl, Error)\n> > +\t\t\t<< \"Issue while parsing 'y' in tuning file: \"\n> > +\t\t\t<< \"entry must be a dictionary\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcurveYr_ = yObject[\"red\"].getList<uint16_t>();\n> > +\tif (curveYr_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > +\t\tLOG(RkISP1Gsl, Error)\n> > +\t\t\t<< \"Issue while parsing 'y:red'\"\n> > +\t\t\t<< \"in tuning file (list size: \"\n> > +\t\t\t<< curveYr_.size() << \"/\"\n> > +\t\t\t<< RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << \")\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcurveYg_ = yObject[\"green\"].getList<uint16_t>();\n> > +\tif (curveYg_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > +\t\tLOG(RkISP1Gsl, Error)\n> > +\t\t\t<< \"Issue while parsing 'y:green'\"\n> > +\t\t\t<< \"in tuning file (list size: \"\n> > +\t\t\t<< curveYg_.size() << \"/\"\n> > +\t\t\t<< RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << \")\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcurveYb_ = yObject[\"blue\"].getList<uint16_t>();\n> > +\tif (curveYb_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > +\t\tLOG(RkISP1Gsl, Error)\n> > +\t\t\t<< \"Issue while parsing 'y:blue'\"\n> > +\t\t\t<< \"in tuning file (list size: \"\n> > +\t\t\t<< curveYb_.size() << \"/\"\n> > +\t\t\t<< RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE << \")\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\ttuningParameters_ = true;\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > +void GammaSensorLinearization::prepare(IPAContext &context,\n> > +\t\t\t\t       rkisp1_params_cfg *params)\n> > +{\n> > +\tif (context.frameContext.frameCount > 0)\n> > +\t\treturn;\n> > +\n> > +\tif (!tuningParameters_)\n> > +\t\treturn;\n> > +\n> > +\tparams->others.sdg_config.xa_pnts.gamma_dx0 = gammaDx_[0];\n> > +\tparams->others.sdg_config.xa_pnts.gamma_dx1 = gammaDx_[1];\n> > +\n> > +\tstd::copy(curveYr_.begin(), curveYr_.end(),\n> > +\t\t  params->others.sdg_config.curve_r.gamma_y);\n> > +\tstd::copy(curveYg_.begin(), curveYg_.end(),\n> > +\t\t  params->others.sdg_config.curve_g.gamma_y);\n> > +\tstd::copy(curveYb_.begin(), curveYb_.end(),\n> > +\t\t  params->others.sdg_config.curve_b.gamma_y);\n> > +\n> > +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_SDG;\n> > +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_SDG;\n> > +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_SDG;\n> > +}\n> > +\n> > +REGISTER_IPA_ALGORITHM(GammaSensorLinearization)\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/gsl.h b/src/ipa/rkisp1/algorithms/gsl.h\n> > new file mode 100644\n> > index 00000000..df16a89b\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/gsl.h\n> > @@ -0,0 +1,38 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021-2022, Ideas On Board\n> > + *\n> > + * gsl.h - RkISP1 Gamma Sensor Linearization control\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include <linux/rkisp1-config.h>\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +struct IPACameraSensorInfo;\n\nI think you can drop this, it's not used in this header.\nlinux/rkisp1-config.h can also be dropped, its only usage is for the\nparameters of the functions defined by the Algorithm base class, so\nthere's a guarantee the right headers will be included by including\nalgorithm.h.\n\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +class GammaSensorLinearization : public Algorithm\n> > +{\n> > +public:\n> > +\tGammaSensorLinearization();\n> > +\t~GammaSensorLinearization() = default;\n> > +\n> > +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> > +\tvoid prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n> > +\n> > +private:\n> > +\tbool tuningParameters_;\n> > +\tuint32_t gammaDx_[2];\n> > +\tstd::vector<uint16_t> curveYr_;\n> > +\tstd::vector<uint16_t> curveYg_;\n> > +\tstd::vector<uint16_t> curveYb_;\n> > +};\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > index 7ec53d89..0597c353 100644\n> > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > @@ -4,4 +4,5 @@ rkisp1_ipa_algorithms = files([\n> >      'agc.cpp',\n> >      'awb.cpp',\n> >      'blc.cpp',\n> > +    'gsl.cpp',\n> >  ])\n> > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> > index 232d8ae8..6cb84ed9 100644\n> > --- a/src/ipa/rkisp1/data/ov5640.yaml\n> > +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> > @@ -10,4 +10,10 @@ algorithms:\n> >        Gr: 256\n> >        Gb: 256\n> >        B:  256\n> > +  - GammaSensorLinearization:\n> > +      x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]\n> > +      y:\n> > +        red:   [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]\n> > +        green: [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]\n> > +        blue:  [ 0, 256, 512, 768, 1024, 1280, 1536, 1792, 2048, 2304, 2560, 2816, 3072, 3328, 3584, 3840, 4096 ]\n\nThe last value should be 4095, 4096 doesn't fit in 12 bits.\n\n> >  ...\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index d052954e..622a0d61 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -31,6 +31,7 @@\n> >  #include \"algorithms/algorithm.h\"\n> >  #include \"algorithms/awb.h\"\n> >  #include \"algorithms/blc.h\"\n> > +#include \"algorithms/gsl.h\"\n> >  #include \"libipa/camera_sensor_helper.h\"\n> >  \n> >  #include \"ipa_context.h\"","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 66F8DBE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Jul 2022 00:00:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD65A6330B;\n\tWed, 13 Jul 2022 02:00:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4CA5A6048B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Jul 2022 02:00:20 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6E83305;\n\tWed, 13 Jul 2022 02:00:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657670421;\n\tbh=4Sd9/iswzPqc35Ui7hYLEt/u4FQnsacW2ASfYbQIM8k=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=g5isZK6eIsXPzIkR5v23fCEHjfkzYZBfBAuGDs8CTJSU7NAsyGM1y5yise/s0HD/t\n\tFDcmwgXHmo97MdJjh1NgSWk+gjw41SK+rBPpQN30rSQkDr1//YENzh0kEu1Lb7YD5l\n\ttuKAeQbD/7Fk20v8Ro2dXQzJ3Tex3rug+PGqFIDXOpi+Z8NtfPhaU28s/3fAwwQf8Z\n\tWWn43J2BUaULQUT6RbW8ZfGUuaig2EOW7dJczii6hd5dOPywbb+WY4hSfv5De2vHSk\n\tYXZnVWz3VAQiyDo5lZGLfKpzVDJEzFrBH5Vb6YUpe/oHLPaLJ7Py2ND1Iej8ABVytE\n\tltRNQy0i3oinw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657670419;\n\tbh=4Sd9/iswzPqc35Ui7hYLEt/u4FQnsacW2ASfYbQIM8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HoSThZe8ZkipoznGa8vqMkX5fw+fASC5HPb1MQlHJvMhvNaP6cYhW1N7tKTM0MkfI\n\tsesd8XPXqzrMamgSyac8Ktm818ulWdnO80kJ8hduqNIB2YBHWL6I3y50XwhcAUyN6v\n\t2h2oYs4T2ghsbL6BaR1U7rQFsC3FH1VnWe0WVxKo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HoSThZe8\"; dkim-atps=neutral","Date":"Wed, 13 Jul 2022 02:59:50 +0300","To":"paul.elder@ideasonboard.com","Message-ID":"<Ys4K9viEF1NdwMiZ@pendragon.ideasonboard.com>","References":"<20220622151918.451635-1-fsylvestre@baylibre.com>\n\t<20220622151918.451635-3-fsylvestre@baylibre.com>\n\t<20220712071801.GC2364006@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220712071801.GC2364006@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: rkisp1: Add support of Gamma\n\tSensor Linearization control","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>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]