[{"id":30255,"web_url":"https://patchwork.libcamera.org/comment/30255/","msgid":"<172000869940.3353312.9221678746525519@ping.linuxembedded.co.uk>","date":"2024-07-03T12:11:39","subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-07-03 11:39:50)\n> As the camera sensor helper now has the ability to provide the black\n> level, use them. Black levels can still be overwritten by the tuning\n> file. But the direction is to remove them from the tuning files and move\n> them into the sensor helpers.\n> \n> Additionally interpret all values based on 16bits. The conversion to the\n> scale required by the hardware is done in process(). It ensures all the\n> values inside libcamera are the same scale and is in preparation for the\n> i.MX8MP where black levels are based on a 20bit scale. Note that this\n> breaks existing tuning files. The tuning files distributed with\n> libcamera will be fixed in a later patch.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/blc.cpp | 54 ++++++++++++++++++++++++++-----\n> 1 file changed, 46 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> index d2e743541c99..87025e4f8c72 100644\n> --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> @@ -46,10 +46,47 @@ BlackLevelCorrection::BlackLevelCorrection()\n> int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> const YamlObject &tuningData)\n> {\n> - blackLevelRed_ = tuningData[\"R\"].get(256);\n> - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> + std::optional levelRed = tuningData[\"R\"].get();\n> + std::optional levelGreenR = tuningData[\"Gr\"].get();\n> + std::optional levelGreenB = tuningData[\"Gb\"].get();\n> + std::optional levelBlue = tuningData[\"B\"].get();\n\nIs this why we're using int16_t instead of uint16_t ... can we\ndistinguise int vs uint in the yaml parsers?\n\n> + bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> +\n> + auto blackLevel = context.camHelper->blackLevel();\n> + if (!blackLevel) {\n> + /*\n> + * Not all camera sensor helpers have been updated with black\n> + * levels. Print a warning and fall back to the levels from the\n> + * tuning data to preserve backward compatibility. This should\n> + * be removed once all helpers provide the data.\n> + */\n> + LOG(RkISP1Blc, Warning)\n> + << \"No black levels provided by camera sensor helper\"\n> + << \", please fix\";\n> +\n> + blackLevelRed_ = levelRed.value_or(4096);\n> + blackLevelGreenR_ = levelGreenR.value_or(4096);\n> + blackLevelGreenB_ = levelGreenB.value_or(4096);\n> + blackLevelBlue_ = levelBlue.value_or(4096);\n> + } else if (tuningHasLevels) {\n> + /*\n> + * If black levels are provided in the tuning file, use them to\n> + * avoid breaking existing camera tuning. This is deprecated and\n> + * will be removed.\n> + */\n> + LOG(RkISP1Blc, Warning)\n> + << \"Deprecated: black levels overwritten by tuning file\";\n> +\n> + blackLevelRed_ = *levelRed;\n> + blackLevelGreenR_ = *levelGreenR;\n> + blackLevelGreenB_ = *levelGreenB;\n> + blackLevelBlue_ = *levelBlue;\n> + } else {\n> + blackLevelRed_ = *blackLevel;\n> + blackLevelGreenR_ = *blackLevel;\n> + blackLevelGreenB_ = *blackLevel;\n> + blackLevelBlue_ = *blackLevel;\n> + }\n> \n> tuningParameters_ = true;\n> \n> @@ -77,10 +114,11 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,\n> return;\n> \n> params->others.bls_config.enable_auto = 0;\n> - params->others.bls_config.fixed_val.r = blackLevelRed_;\n> - params->others.bls_config.fixed_val.gr = blackLevelGreenR_;\n> - params->others.bls_config.fixed_val.gb = blackLevelGreenB_;\n> - params->others.bls_config.fixed_val.b = blackLevelBlue_;\n> + /* The rkisp1 uses 12bit based black levels. Scale down accordingly. */\n> + params->others.bls_config.fixed_val.r = blackLevelRed_ << 4;\n> + params->others.bls_config.fixed_val.gr = blackLevelGreenR_ << 4;\n> + params->others.bls_config.fixed_val.gb = blackLevelGreenB_ << 4;\n> + params->others.bls_config.fixed_val.b = blackLevelBlue_ << 4;\n\nErm, Should those be >> 4 or did I miss something obvious?\n\n> \n> params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;\n> params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;\n> -- \n> 2.43.0\n>","headers":{"Return-Path":"","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 7BF32BEFBE\n\tfor ;\n\tWed, 3 Jul 2024 12:11:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 530C362E22;\n\tWed, 3 Jul 2024 14:11:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A596262C95\n\tfor ;\n\tWed, 3 Jul 2024 14:11:42 +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 57A1E3E6;\n\tWed, 3 Jul 2024 14:11:14 +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=\"LX9YHyTw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720008674;\n\tbh=OaHCRhs0GmD0vrrFagEMEfgEUO6OiPYDWnpk+UPnUE4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=LX9YHyTwEZoFx1LEphAnvcR9S/MkjgLO7NxaT5uAiij95LVyuZ0dD7R25ou+Gw4rC\n\tMQEE+PkULZYdEyAm4Q84TKQ41hwHc9svWihPqVwgMhsXTw9KedvxG49owMkOCp7bHu\n\tqkK97tjDOXpZvR/NC49V5VFDjmncUf39aCm9Pll4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240703104004.184783-4-stefan.klug@ideasonboard.com>","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-4-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 03 Jul 2024 13:11:39 +0100","Message-ID":"<172000869940.3353312.9221678746525519@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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30261,"web_url":"https://patchwork.libcamera.org/comment/30261/","msgid":"","date":"2024-07-03T12:28:17","subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Wed, Jul 03, 2024 at 01:11:39PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-07-03 11:39:50)\n> > As the camera sensor helper now has the ability to provide the black\n> > level, use them. Black levels can still be overwritten by the tuning\n> > file. But the direction is to remove them from the tuning files and move\n> > them into the sensor helpers.\n> > \n> > Additionally interpret all values based on 16bits. The conversion to the\n> > scale required by the hardware is done in process(). It ensures all the\n> > values inside libcamera are the same scale and is in preparation for the\n> > i.MX8MP where black levels are based on a 20bit scale. Note that this\n> > breaks existing tuning files. The tuning files distributed with\n> > libcamera will be fixed in a later patch.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/blc.cpp | 54 ++++++++++++++++++++++++++-----\n> > 1 file changed, 46 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > index d2e743541c99..87025e4f8c72 100644\n> > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > @@ -46,10 +46,47 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > const YamlObject &tuningData)\n> > {\n> > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > + std::optional levelRed = tuningData[\"R\"].get();\n> > + std::optional levelGreenR = tuningData[\"Gr\"].get();\n> > + std::optional levelGreenB = tuningData[\"Gb\"].get();\n> > + std::optional levelBlue = tuningData[\"B\"].get();\n> \n> Is this why we're using int16_t instead of uint16_t ... can we\n> distinguise int vs uint in the yaml parsers?\n\nYes, that is actually the reason. I was undecided here. The metadata is\nint32_t, the tuning files used to read int16_t, the rkisp1 allows\nnegative black levels for whatever reason... So I thought int16_t is the\nbitwidth we always mention as reference and the maximum value beeing 50%\nof the uint16_t range should be more than sufficient for the black\nlevel.\n\n> \n> > + bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> > +\n> > + auto blackLevel = context.camHelper->blackLevel();\n> > + if (!blackLevel) {\n> > + /*\n> > + * Not all camera sensor helpers have been updated with black\n> > + * levels. Print a warning and fall back to the levels from the\n> > + * tuning data to preserve backward compatibility. This should\n> > + * be removed once all helpers provide the data.\n> > + */\n> > + LOG(RkISP1Blc, Warning)\n> > + << \"No black levels provided by camera sensor helper\"\n> > + << \", please fix\";\n> > +\n> > + blackLevelRed_ = levelRed.value_or(4096);\n> > + blackLevelGreenR_ = levelGreenR.value_or(4096);\n> > + blackLevelGreenB_ = levelGreenB.value_or(4096);\n> > + blackLevelBlue_ = levelBlue.value_or(4096);\n> > + } else if (tuningHasLevels) {\n> > + /*\n> > + * If black levels are provided in the tuning file, use them to\n> > + * avoid breaking existing camera tuning. This is deprecated and\n> > + * will be removed.\n> > + */\n> > + LOG(RkISP1Blc, Warning)\n> > + << \"Deprecated: black levels overwritten by tuning file\";\n> > +\n> > + blackLevelRed_ = *levelRed;\n> > + blackLevelGreenR_ = *levelGreenR;\n> > + blackLevelGreenB_ = *levelGreenB;\n> > + blackLevelBlue_ = *levelBlue;\n> > + } else {\n> > + blackLevelRed_ = *blackLevel;\n> > + blackLevelGreenR_ = *blackLevel;\n> > + blackLevelGreenB_ = *blackLevel;\n> > + blackLevelBlue_ = *blackLevel;\n> > + }\n> > \n> > tuningParameters_ = true;\n> > \n> > @@ -77,10 +114,11 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,\n> > return;\n> > \n> > params->others.bls_config.enable_auto = 0;\n> > - params->others.bls_config.fixed_val.r = blackLevelRed_;\n> > - params->others.bls_config.fixed_val.gr = blackLevelGreenR_;\n> > - params->others.bls_config.fixed_val.gb = blackLevelGreenB_;\n> > - params->others.bls_config.fixed_val.b = blackLevelBlue_;\n> > + /* The rkisp1 uses 12bit based black levels. Scale down accordingly. */\n> > + params->others.bls_config.fixed_val.r = blackLevelRed_ << 4;\n> > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_ << 4;\n> > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_ << 4;\n> > + params->others.bls_config.fixed_val.b = blackLevelBlue_ << 4;\n> \n> Erm, Should those be >> 4 or did I miss something obvious?\n\nOh damn you are right. I'm just in the final real test, so that would\nhave been caught. But yes, that is wrong.\n\nThanks for catching it.\n\nBest regards,\nStefan\n\n> \n> > \n> > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;\n> > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;\n> > -- \n> > 2.43.0\n> >","headers":{"Return-Path":"","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 F2EA8BEFBE\n\tfor ;\n\tWed, 3 Jul 2024 12:28:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE1D662E26;\n\tWed, 3 Jul 2024 14:28: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 F1FCD62C95\n\tfor ;\n\tWed, 3 Jul 2024 14:28:19 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:9263:c199:9587:576])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3CB93E6;\n\tWed, 3 Jul 2024 14:27:51 +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=\"m9a0ehOf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720009672;\n\tbh=UnIZpasLiwp3hHX37aMZwK0iizbQHJ7SFZ6xih6TuUc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m9a0ehOf4QXXobL6JuY9DoRz+1ZtxnrtZPEjCc+nn5f0lhODllNT+jvYR8D7Jc9YV\n\t9LGWOmvaFm4ZJn6qdWy8fZd/9/vkg2rHjc53PZIECXOWA8mEnfVRrr1dsosb8U4+ad\n\tqSwk43B2N3Mia8h81rAyWMfyVzh9YoddcAk6USDc=","Date":"Wed, 3 Jul 2024 14:28:17 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-4-stefan.klug@ideasonboard.com>\n\t<172000869940.3353312.9221678746525519@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172000869940.3353312.9221678746525519@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30264,"web_url":"https://patchwork.libcamera.org/comment/30264/","msgid":"<172000992444.3353312.16629149208861970019@ping.linuxembedded.co.uk>","date":"2024-07-03T12:32:04","subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-07-03 13:28:17)\n> On Wed, Jul 03, 2024 at 01:11:39PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-07-03 11:39:50)\n> > > As the camera sensor helper now has the ability to provide the black\n> > > level, use them. Black levels can still be overwritten by the tuning\n> > > file. But the direction is to remove them from the tuning files and move\n> > > them into the sensor helpers.\n> > > \n> > > Additionally interpret all values based on 16bits. The conversion to the\n> > > scale required by the hardware is done in process(). It ensures all the\n> > > values inside libcamera are the same scale and is in preparation for the\n> > > i.MX8MP where black levels are based on a 20bit scale. Note that this\n> > > breaks existing tuning files. The tuning files distributed with\n> > > libcamera will be fixed in a later patch.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/algorithms/blc.cpp | 54 ++++++++++++++++++++++++++-----\n> > > 1 file changed, 46 insertions(+), 8 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > index d2e743541c99..87025e4f8c72 100644\n> > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > @@ -46,10 +46,47 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > const YamlObject &tuningData)\n> > > {\n> > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > + std::optional levelRed = tuningData[\"R\"].get();\n> > > + std::optional levelGreenR = tuningData[\"Gr\"].get();\n> > > + std::optional levelGreenB = tuningData[\"Gb\"].get();\n> > > + std::optional levelBlue = tuningData[\"B\"].get();\n> > \n> > Is this why we're using int16_t instead of uint16_t ... can we\n> > distinguise int vs uint in the yaml parsers?\n> \n> Yes, that is actually the reason. I was undecided here. The metadata is\n> int32_t, the tuning files used to read int16_t, the rkisp1 allows\n> negative black levels for whatever reason... So I thought int16_t is the\n> bitwidth we always mention as reference and the maximum value beeing 50%\n> of the uint16_t range should be more than sufficient for the black\n> level.\n> \n> > \n> > > + bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> > > +\n> > > + auto blackLevel = context.camHelper->blackLevel();\n> > > + if (!blackLevel) {\n> > > + /*\n> > > + * Not all camera sensor helpers have been updated with black\n> > > + * levels. Print a warning and fall back to the levels from the\n> > > + * tuning data to preserve backward compatibility. This should\n> > > + * be removed once all helpers provide the data.\n> > > + */\n> > > + LOG(RkISP1Blc, Warning)\n> > > + << \"No black levels provided by camera sensor helper\"\n> > > + << \", please fix\";\n> > > +\n> > > + blackLevelRed_ = levelRed.value_or(4096);\n> > > + blackLevelGreenR_ = levelGreenR.value_or(4096);\n> > > + blackLevelGreenB_ = levelGreenB.value_or(4096);\n> > > + blackLevelBlue_ = levelBlue.value_or(4096);\n> > > + } else if (tuningHasLevels) {\n> > > + /*\n> > > + * If black levels are provided in the tuning file, use them to\n> > > + * avoid breaking existing camera tuning. This is deprecated and\n> > > + * will be removed.\n> > > + */\n> > > + LOG(RkISP1Blc, Warning)\n> > > + << \"Deprecated: black levels overwritten by tuning file\";\n> > > +\n> > > + blackLevelRed_ = *levelRed;\n> > > + blackLevelGreenR_ = *levelGreenR;\n> > > + blackLevelGreenB_ = *levelGreenB;\n> > > + blackLevelBlue_ = *levelBlue;\n> > > + } else {\n> > > + blackLevelRed_ = *blackLevel;\n> > > + blackLevelGreenR_ = *blackLevel;\n> > > + blackLevelGreenB_ = *blackLevel;\n> > > + blackLevelBlue_ = *blackLevel;\n> > > + }\n> > > \n> > > tuningParameters_ = true;\n> > > \n> > > @@ -77,10 +114,11 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,\n> > > return;\n> > > \n> > > params->others.bls_config.enable_auto = 0;\n> > > - params->others.bls_config.fixed_val.r = blackLevelRed_;\n> > > - params->others.bls_config.fixed_val.gr = blackLevelGreenR_;\n> > > - params->others.bls_config.fixed_val.gb = blackLevelGreenB_;\n> > > - params->others.bls_config.fixed_val.b = blackLevelBlue_;\n> > > + /* The rkisp1 uses 12bit based black levels. Scale down accordingly. */\n> > > + params->others.bls_config.fixed_val.r = blackLevelRed_ << 4;\n> > > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_ << 4;\n> > > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_ << 4;\n> > > + params->others.bls_config.fixed_val.b = blackLevelBlue_ << 4;\n> > \n> > Erm, Should those be >> 4 or did I miss something obvious?\n> \n> Oh damn you are right. I'm just in the final real test, so that would\n> have been caught. But yes, that is wrong.\n> \n> Thanks for catching it.\n\nNo worries - with that corrected:\n\nReviewed-by: Kieran Bingham \n\n> \n> Best regards,\n> Stefan\n> \n> > \n> > > \n> > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;\n> > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;\n> > > -- \n> > > 2.43.0\n> > >","headers":{"Return-Path":"","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 2AAC6BD87C\n\tfor ;\n\tWed, 3 Jul 2024 12:32:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C44B763331;\n\tWed, 3 Jul 2024 14:32:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 396EA62E22\n\tfor ;\n\tWed, 3 Jul 2024 14:32:07 +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 2EBD83E6;\n\tWed, 3 Jul 2024 14:31:39 +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=\"sGIoxNv3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720009899;\n\tbh=fRdcphYnRmGE7g1xCgYPe0k2pzcDM6NGwrhlpDDDqTM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sGIoxNv3DKLkGIwa047BBt6x+D9T6srCGjFzGjSahG1QFQC6ApfAiCeQD19g5g2kD\n\tkvLQjKsOM1EbCb1/OW76956dMve0aEFUIOHU++t5AO681ozEevqHbKeFi/RY/tBliv\n\tah0axUYqB4oGMDmOVFCU3o03x57U71Y+4iI+nJ50=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-4-stefan.klug@ideasonboard.com>\n\t<172000869940.3353312.9221678746525519@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Stefan Klug ","Date":"Wed, 03 Jul 2024 13:32:04 +0100","Message-ID":"<172000992444.3353312.16629149208861970019@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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30267,"web_url":"https://patchwork.libcamera.org/comment/30267/","msgid":"<20240703124214.GB27927@pendragon.ideasonboard.com>","date":"2024-07-03T12:42:14","subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jul 03, 2024 at 01:32:04PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-07-03 13:28:17)\n> > On Wed, Jul 03, 2024 at 01:11:39PM +0100, Kieran Bingham wrote:\n> > > Quoting Stefan Klug (2024-07-03 11:39:50)\n> > > > As the camera sensor helper now has the ability to provide the black\n> > > > level, use them. Black levels can still be overwritten by the tuning\n\ns/use them/use it/ or s/helper now has/helpers now have/\n\n> > > > file. But the direction is to remove them from the tuning files and move\n\ns/file. But the/file. The/ or s/file. But/file, but/\n\n> > > > them into the sensor helpers.\n> > > > \n> > > > Additionally interpret all values based on 16bits. The conversion to the\n> > > > scale required by the hardware is done in process(). It ensures all the\n> > > > values inside libcamera are the same scale and is in preparation for the\n> > > > i.MX8MP where black levels are based on a 20bit scale. Note that this\n> > > > breaks existing tuning files. The tuning files distributed with\n> > > > libcamera will be fixed in a later patch.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/blc.cpp | 54 ++++++++++++++++++++++++++-----\n> > > > 1 file changed, 46 insertions(+), 8 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > index d2e743541c99..87025e4f8c72 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp\n> > > > @@ -46,10 +46,47 @@ BlackLevelCorrection::BlackLevelCorrection()\n> > > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,\n> > > > const YamlObject &tuningData)\n> > > > {\n> > > > - blackLevelRed_ = tuningData[\"R\"].get(256);\n> > > > - blackLevelGreenR_ = tuningData[\"Gr\"].get(256);\n> > > > - blackLevelGreenB_ = tuningData[\"Gb\"].get(256);\n> > > > - blackLevelBlue_ = tuningData[\"B\"].get(256);\n> > > > + std::optional levelRed = tuningData[\"R\"].get();\n> > > > + std::optional levelGreenR = tuningData[\"Gr\"].get();\n> > > > + std::optional levelGreenB = tuningData[\"Gb\"].get();\n> > > > + std::optional levelBlue = tuningData[\"B\"].get();\n> > > \n> > > Is this why we're using int16_t instead of uint16_t ... can we\n> > > distinguise int vs uint in the yaml parsers?\n> > \n> > Yes, that is actually the reason. I was undecided here. The metadata is\n> > int32_t, the tuning files used to read int16_t, the rkisp1 allows\n> > negative black levels for whatever reason... So I thought int16_t is the\n> > bitwidth we always mention as reference and the maximum value beeing 50%\n> > of the uint16_t range should be more than sufficient for the black\n> > level.\n\nI think it's sufficient indeed :-) We could use uint16_t if desired\nthough.\n\n> > > > + bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;\n> > > > +\n> > > > + auto blackLevel = context.camHelper->blackLevel();\n> > > > + if (!blackLevel) {\n> > > > + /*\n> > > > + * Not all camera sensor helpers have been updated with black\n> > > > + * levels. Print a warning and fall back to the levels from the\n> > > > + * tuning data to preserve backward compatibility. This should\n> > > > + * be removed once all helpers provide the data.\n> > > > + */\n> > > > + LOG(RkISP1Blc, Warning)\n> > > > + << \"No black levels provided by camera sensor helper\"\n> > > > + << \", please fix\";\n> > > > +\n> > > > + blackLevelRed_ = levelRed.value_or(4096);\n> > > > + blackLevelGreenR_ = levelGreenR.value_or(4096);\n> > > > + blackLevelGreenB_ = levelGreenB.value_or(4096);\n> > > > + blackLevelBlue_ = levelBlue.value_or(4096);\n> > > > + } else if (tuningHasLevels) {\n> > > > + /*\n> > > > + * If black levels are provided in the tuning file, use them to\n> > > > + * avoid breaking existing camera tuning. This is deprecated and\n> > > > + * will be removed.\n> > > > + */\n> > > > + LOG(RkISP1Blc, Warning)\n> > > > + << \"Deprecated: black levels overwritten by tuning file\";\n> > > > +\n> > > > + blackLevelRed_ = *levelRed;\n> > > > + blackLevelGreenR_ = *levelGreenR;\n> > > > + blackLevelGreenB_ = *levelGreenB;\n> > > > + blackLevelBlue_ = *levelBlue;\n> > > > + } else {\n> > > > + blackLevelRed_ = *blackLevel;\n> > > > + blackLevelGreenR_ = *blackLevel;\n> > > > + blackLevelGreenB_ = *blackLevel;\n> > > > + blackLevelBlue_ = *blackLevel;\n> > > > + }\n> > > > \n> > > > tuningParameters_ = true;\n> > > > \n> > > > @@ -77,10 +114,11 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,\n> > > > return;\n> > > > \n> > > > params->others.bls_config.enable_auto = 0;\n> > > > - params->others.bls_config.fixed_val.r = blackLevelRed_;\n> > > > - params->others.bls_config.fixed_val.gr = blackLevelGreenR_;\n> > > > - params->others.bls_config.fixed_val.gb = blackLevelGreenB_;\n> > > > - params->others.bls_config.fixed_val.b = blackLevelBlue_;\n> > > > + /* The rkisp1 uses 12bit based black levels. Scale down accordingly. */\n> > > > + params->others.bls_config.fixed_val.r = blackLevelRed_ << 4;\n> > > > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_ << 4;\n> > > > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_ << 4;\n> > > > + params->others.bls_config.fixed_val.b = blackLevelBlue_ << 4;\n> > > \n> > > Erm, Should those be >> 4 or did I miss something obvious?\n> > \n> > Oh damn you are right. I'm just in the final real test, so that would\n> > have been caught. But yes, that is wrong.\n> > \n> > Thanks for catching it.\n> \n> No worries - with that corrected:\n> \n> Reviewed-by: Kieran Bingham \n\nReviewed-by: Laurent Pinchart \n\n> > > > \n> > > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;\n> > > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;","headers":{"Return-Path":"","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 94C7CBD87C\n\tfor ;\n\tWed, 3 Jul 2024 12:42:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8ECB262E24;\n\tWed, 3 Jul 2024 14:42:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A1AA62C99\n\tfor ;\n\tWed, 3 Jul 2024 14:42:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 34E0C3E6;\n\tWed, 3 Jul 2024 14:42:07 +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=\"BuU3h5Q7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720010527;\n\tbh=aGLuZWqgQ3apqtRWzPm3EQWzOR9yeknXAogBcVuNzX0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BuU3h5Q7x++qOL9bGk+WhtmV1T14/mvjUpYOKlCiSyDdQwAxMkrgd1qUd7/EaH5Vv\n\tCukD6Vor/HptSDGoEG5VtrPg/JJu4Hv1aCnDsRwWpUVJ7+/OOI8uKC87cZZoqwbFAJ\n\ttJp/cXyalWL0mMBtzVw+IriVW32Uou/ZJqbjaTX0=","Date":"Wed, 3 Jul 2024 15:42:14 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera\n\tsensor helper","Message-ID":"<20240703124214.GB27927@pendragon.ideasonboard.com>","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-4-stefan.klug@ideasonboard.com>\n\t<172000869940.3353312.9221678746525519@ping.linuxembedded.co.uk>\n\t\n\t<172000992444.3353312.16629149208861970019@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172000992444.3353312.16629149208861970019@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]