[{"id":30258,"web_url":"https://patchwork.libcamera.org/comment/30258/","msgid":"<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>","date":"2024-07-03T12:20:56","subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","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:53)\n> Move black levels for tuning files that contained a BLC block into\n> the camera sensor helpers.\n> \n> ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the\n> value from the datasheet (scaled to 16bit).\n\nI wonder who chose 66 here. Was it measured by someone perhaps?\n\nGit blame states \"Mikhail Rudenko\" (Added to Cc) Mikhail, any opinion on\nthis change?\n\nAnyway, I don't object to this right now - I think once we release our\nupdates to the IPA and tuning tools, most sensors 'supported' in\nlibcamera will likely benefit from being '(re)tuned' correctly and\n'fully' (and consistently?).\n\nReviewed-by: Kieran Bingham \n\n> ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking\n> at the commit message the 256 most likely stems from the imx219 tuning\n> file and 16@10bit is the same as the 64@12bit from the ov4689. This\n> seems more likely and is therefore used.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 2 ++\n> src/ipa/rkisp1/data/ov4689.yaml | 4 ----\n> src/ipa/rkisp1/data/ov5640.yaml | 4 ----\n> 3 files changed, 2 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 3d0e756927f0..982e35beaa90 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper\n> public:\n> CameraSensorHelperOv4689()\n> {\n> + blackLevel_ = 1024;\n> gainType_ = AnalogueGainLinear;\n> gainConstants_.linear = { 1, 0, 0, 128 };\n> }\n> @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper\n> public:\n> CameraSensorHelperOv5640()\n> {\n> + blackLevel_ = 1024;\n> gainType_ = AnalogueGainLinear;\n> gainConstants_.linear = { 1, 0, 0, 16 };\n> }\n> diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml\n> index 2068684cafcd..609012967e02 100644\n> --- a/src/ipa/rkisp1/data/ov4689.yaml\n> +++ b/src/ipa/rkisp1/data/ov4689.yaml\n> @@ -6,8 +6,4 @@ algorithms:\n> - Agc:\n> - Awb:\n> - BlackLevelCorrection:\n> - R: 66\n> - Gr: 66\n> - Gb: 66\n> - B: 66\n> ...\n> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> index 897b83cb435b..4b21d412e44e 100644\n> --- a/src/ipa/rkisp1/data/ov5640.yaml\n> +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> @@ -6,10 +6,6 @@ algorithms:\n> - Agc:\n> - Awb:\n> - BlackLevelCorrection:\n> - R: 256\n> - Gr: 256\n> - Gb: 256\n> - B: 256\n> - ColorProcessing:\n> - GammaSensorLinearization:\n> x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]\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 2775BBEFBE\n\tfor ;\n\tWed, 3 Jul 2024 12:21:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0395D62E23;\n\tWed, 3 Jul 2024 14:21:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 789EF62C96\n\tfor ;\n\tWed, 3 Jul 2024 14:20:59 +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 1F4C03E6;\n\tWed, 3 Jul 2024 14:20:31 +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=\"r9DU2z4z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720009231;\n\tbh=Gi1EcZGlWfJtTAvEKIlXqrV/KU65NCwn8vOHWrJgu+E=;\n\th=In-Reply-To:References:Subject:From:To:Cc:Date:From;\n\tb=r9DU2z4zf2YRhOqx/fzfvW21TMR5tDn8KEG3srEdG3Ug4BL/JEfMMLcNwAgReISGV\n\twy1j4pYSy2oXgw52cnBeOdF0d6q8hJ5aewGRB/bM+B+lkcP1nYlCWufuE2Wuhv9a8C\n\tKkmrBOxHOrodT+mqcMXF3rsFrwsb3Dom8VuHOaEk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240703104004.184783-7-stefan.klug@ideasonboard.com>","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-7-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","From":"Kieran Bingham ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Stefan Klug ,\n\tMikhail Rudenko ","Date":"Wed, 03 Jul 2024 13:20:56 +0100","Message-ID":"<172000925637.3353312.16323259168271323660@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":30270,"web_url":"https://patchwork.libcamera.org/comment/30270/","msgid":"<20240703124819.GE27927@pendragon.ideasonboard.com>","date":"2024-07-03T12:48:19","subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","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:20:56PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-07-03 11:39:53)\n> > Move black levels for tuning files that contained a BLC block into\n> > the camera sensor helpers.\n> > \n> > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the\n> > value from the datasheet (scaled to 16bit).\n> \n> I wonder who chose 66 here. Was it measured by someone perhaps?\n> \n> Git blame states \"Mikhail Rudenko\" (Added to Cc) Mikhail, any opinion on\n> this change?\n> \n> Anyway, I don't object to this right now - I think once we release our\n> updates to the IPA and tuning tools, most sensors 'supported' in\n> libcamera will likely benefit from being '(re)tuned' correctly and\n> 'fully' (and consistently?).\n> \n> Reviewed-by: Kieran Bingham \n>\n> > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking\n> > at the commit message the 256 most likely stems from the imx219 tuning\n> > file and 16@10bit is the same as the 64@12bit from the ov4689. This\n> > seems more likely and is therefore used.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++\n> > src/ipa/rkisp1/data/ov4689.yaml | 4 ----\n> > src/ipa/rkisp1/data/ov5640.yaml | 4 ----\n> > 3 files changed, 2 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index 3d0e756927f0..982e35beaa90 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper\n> > public:\n> > CameraSensorHelperOv4689()\n> > {\n> > + blackLevel_ = 1024;\n\nI wonder if\n\n\t\tblackLevel_ = 16 << 6;\n\nwould be more readable. I suppose what you want to see at a glance, the\n16-bit value, or the value for the native sensor bit depth. Up to you.\nIf you decide to change it, please update all helpers.\n\nReviewed-by: Laurent Pinchart \n\n> > gainType_ = AnalogueGainLinear;\n> > gainConstants_.linear = { 1, 0, 0, 128 };\n> > }\n> > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper\n> > public:\n> > CameraSensorHelperOv5640()\n> > {\n> > + blackLevel_ = 1024;\n> > gainType_ = AnalogueGainLinear;\n> > gainConstants_.linear = { 1, 0, 0, 16 };\n> > }\n> > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml\n> > index 2068684cafcd..609012967e02 100644\n> > --- a/src/ipa/rkisp1/data/ov4689.yaml\n> > +++ b/src/ipa/rkisp1/data/ov4689.yaml\n> > @@ -6,8 +6,4 @@ algorithms:\n> > - Agc:\n> > - Awb:\n> > - BlackLevelCorrection:\n> > - R: 66\n> > - Gr: 66\n> > - Gb: 66\n> > - B: 66\n> > ...\n> > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> > index 897b83cb435b..4b21d412e44e 100644\n> > --- a/src/ipa/rkisp1/data/ov5640.yaml\n> > +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> > @@ -6,10 +6,6 @@ algorithms:\n> > - Agc:\n> > - Awb:\n> > - BlackLevelCorrection:\n> > - R: 256\n> > - Gr: 256\n> > - Gb: 256\n> > - B: 256\n> > - ColorProcessing:\n> > - GammaSensorLinearization:\n> > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]","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 9AADABEFBE\n\tfor ;\n\tWed, 3 Jul 2024 12:48:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6832C62C99;\n\tWed, 3 Jul 2024 14:48:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B77FD62C99\n\tfor ;\n\tWed, 3 Jul 2024 14:48:40 +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 340676D6;\n\tWed, 3 Jul 2024 14:48:12 +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=\"OifY1pkW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720010892;\n\tbh=0LwfimhXgRNeqafw25cLUWwHvkRFRBQ+oNbXQeK0lHo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OifY1pkWlMDkbVHtoZGsbZhUmuNj2orjTtlri8kAwUnlBASGBhWIRUlVO35rAUnkn\n\tbrfAlS0Y49cUNYLHuXNOcE8Y1AZNhpoReWs+zdH4AxAxFY0+jJ5CAWSrIp02i/Ts3Z\n\tZA/gfhEu3XOrcf+kdQwUThm09/RQrCHxuTEYkDo8=","Date":"Wed, 3 Jul 2024 15:48:19 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMikhail Rudenko ","Subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","Message-ID":"<20240703124819.GE27927@pendragon.ideasonboard.com>","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-7-stefan.klug@ideasonboard.com>\n\t<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172000925637.3353312.16323259168271323660@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":30272,"web_url":"https://patchwork.libcamera.org/comment/30272/","msgid":"","date":"2024-07-03T13:11:04","subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","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 03:48:19PM +0300, Laurent Pinchart wrote:\n> On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-07-03 11:39:53)\n> > > Move black levels for tuning files that contained a BLC block into\n> > > the camera sensor helpers.\n> > > \n> > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the\n> > > value from the datasheet (scaled to 16bit).\n> > \n> > I wonder who chose 66 here. Was it measured by someone perhaps?\n> > \n> > Git blame states \"Mikhail Rudenko\" (Added to Cc) Mikhail, any opinion on\n> > this change?\n> > \n> > Anyway, I don't object to this right now - I think once we release our\n> > updates to the IPA and tuning tools, most sensors 'supported' in\n> > libcamera will likely benefit from being '(re)tuned' correctly and\n> > 'fully' (and consistently?).\n> > \n> > Reviewed-by: Kieran Bingham \n> >\n> > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking\n> > > at the commit message the 256 most likely stems from the imx219 tuning\n> > > file and 16@10bit is the same as the 64@12bit from the ov4689. This\n> > > seems more likely and is therefore used.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++\n> > > src/ipa/rkisp1/data/ov4689.yaml | 4 ----\n> > > src/ipa/rkisp1/data/ov5640.yaml | 4 ----\n> > > 3 files changed, 2 insertions(+), 8 deletions(-)\n> > > \n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > index 3d0e756927f0..982e35beaa90 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper\n> > > public:\n> > > CameraSensorHelperOv4689()\n> > > {\n> > > + blackLevel_ = 1024;\n> \n> I wonder if\n> \n> \t\tblackLevel_ = 16 << 6;\n> \n> would be more readable. I suppose what you want to see at a glance, the\n> 16-bit value, or the value for the native sensor bit depth. Up to you.\n> If you decide to change it, please update all helpers.\n> \n> Reviewed-by: Laurent Pinchart \n\nThat is a difficult one. I'd like to stick to the current version.\nWithout it, I think I would have missed that all Omnivision black levels\nare the same, even though the documentation mentions 64@12 and 16@10.\nWould it help to include comments above with the origin of the\ninformation?\n\nRegards,\nStefan\n\n> \n> > > gainType_ = AnalogueGainLinear;\n> > > gainConstants_.linear = { 1, 0, 0, 128 };\n> > > }\n> > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper\n> > > public:\n> > > CameraSensorHelperOv5640()\n> > > {\n> > > + blackLevel_ = 1024;\n> > > gainType_ = AnalogueGainLinear;\n> > > gainConstants_.linear = { 1, 0, 0, 16 };\n> > > }\n> > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml\n> > > index 2068684cafcd..609012967e02 100644\n> > > --- a/src/ipa/rkisp1/data/ov4689.yaml\n> > > +++ b/src/ipa/rkisp1/data/ov4689.yaml\n> > > @@ -6,8 +6,4 @@ algorithms:\n> > > - Agc:\n> > > - Awb:\n> > > - BlackLevelCorrection:\n> > > - R: 66\n> > > - Gr: 66\n> > > - Gb: 66\n> > > - B: 66\n> > > ...\n> > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> > > index 897b83cb435b..4b21d412e44e 100644\n> > > --- a/src/ipa/rkisp1/data/ov5640.yaml\n> > > +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> > > @@ -6,10 +6,6 @@ algorithms:\n> > > - Agc:\n> > > - Awb:\n> > > - BlackLevelCorrection:\n> > > - R: 256\n> > > - Gr: 256\n> > > - Gb: 256\n> > > - B: 256\n> > > - ColorProcessing:\n> > > - GammaSensorLinearization:\n> > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 E98EABEFBE\n\tfor ;\n\tWed, 3 Jul 2024 13:11:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8C8F62C96;\n\tWed, 3 Jul 2024 15:11:08 +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 A0BCD62C95\n\tfor ;\n\tWed, 3 Jul 2024 15:11:07 +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 82B6E8A9;\n\tWed, 3 Jul 2024 15:10: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=\"QzwLJ8zc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720012239;\n\tbh=fYbq0HfB3NnswfeCBKY0xLnlmodeBHdo1sU+4cH0l/c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QzwLJ8zc87xWjFZuUphA6mFAA0BDbpw7K8seB8Djxp4jTsyAqTbAIM5F37anNbXgS\n\tr9UGKGNKJ8VvDBpoiWOeHt/uy6pYXCuoS8+v/0VJqQ9OzgUcRtDIejcJBpGTK5l61N\n\tOKKQC0yNQ9NW98R/v3iFH65sZu4NDCQXquHA4CyQ=","Date":"Wed, 3 Jul 2024 15:11:04 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org,\n\tMikhail Rudenko ","Subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","Message-ID":"","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-7-stefan.klug@ideasonboard.com>\n\t<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>\n\t<20240703124819.GE27927@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240703124819.GE27927@pendragon.ideasonboard.com>","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":30273,"web_url":"https://patchwork.libcamera.org/comment/30273/","msgid":"<172001274074.3353312.7954717625998615271@ping.linuxembedded.co.uk>","date":"2024-07-03T13:19:00","subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-07-03 13:48:19)\n> On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-07-03 11:39:53)\n> > > Move black levels for tuning files that contained a BLC block into\n> > > the camera sensor helpers.\n> > > \n> > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the\n> > > value from the datasheet (scaled to 16bit).\n> > \n> > I wonder who chose 66 here. Was it measured by someone perhaps?\n> > \n> > Git blame states \"Mikhail Rudenko\" (Added to Cc) Mikhail, any opinion on\n> > this change?\n> > \n> > Anyway, I don't object to this right now - I think once we release our\n> > updates to the IPA and tuning tools, most sensors 'supported' in\n> > libcamera will likely benefit from being '(re)tuned' correctly and\n> > 'fully' (and consistently?).\n> > \n> > Reviewed-by: Kieran Bingham \n> >\n> > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking\n> > > at the commit message the 256 most likely stems from the imx219 tuning\n> > > file and 16@10bit is the same as the 64@12bit from the ov4689. This\n> > > seems more likely and is therefore used.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++\n> > > src/ipa/rkisp1/data/ov4689.yaml | 4 ----\n> > > src/ipa/rkisp1/data/ov5640.yaml | 4 ----\n> > > 3 files changed, 2 insertions(+), 8 deletions(-)\n> > > \n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > index 3d0e756927f0..982e35beaa90 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper\n> > > public:\n> > > CameraSensorHelperOv4689()\n> > > {\n> > > + blackLevel_ = 1024;\n> \n> I wonder if\n> \n> blackLevel_ = 16 << 6;\n\nI like this idea ... except the '<< 6' isn't particularly clear at\nconveying the 16 is in 10 bit.\n\n\nI guess we could add a helper\n\tblackLevel_ = to16Bit(10 /*bit*/, 16)\n\n\n#define to16Bit(depth, val) ((val) << (16 - (depth)))\n\nOr some other magic ... but I definitely recall my confusion when I\nfirst looked at the black levels in a datasheet and saw different values\nat different bit depths, and had to convert again for RPi. The operation\nis simple of course - but it's the reason/intent that may not be clear\nto a new comer - so doing something to make that easier to bring up a\nnew sensor is a good thing IMO.\n\nCould be something that gets tackled later though.\n\n--\nKieran\n\n\n> \n> would be more readable. I suppose what you want to see at a glance, the\n> 16-bit value, or the value for the native sensor bit depth. Up to you.\n> If you decide to change it, please update all helpers.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > > gainType_ = AnalogueGainLinear;\n> > > gainConstants_.linear = { 1, 0, 0, 128 };\n> > > }\n> > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper\n> > > public:\n> > > CameraSensorHelperOv5640()\n> > > {\n> > > + blackLevel_ = 1024;\n> > > gainType_ = AnalogueGainLinear;\n> > > gainConstants_.linear = { 1, 0, 0, 16 };\n> > > }\n> > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml\n> > > index 2068684cafcd..609012967e02 100644\n> > > --- a/src/ipa/rkisp1/data/ov4689.yaml\n> > > +++ b/src/ipa/rkisp1/data/ov4689.yaml\n> > > @@ -6,8 +6,4 @@ algorithms:\n> > > - Agc:\n> > > - Awb:\n> > > - BlackLevelCorrection:\n> > > - R: 66\n> > > - Gr: 66\n> > > - Gb: 66\n> > > - B: 66\n> > > ...\n> > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> > > index 897b83cb435b..4b21d412e44e 100644\n> > > --- a/src/ipa/rkisp1/data/ov5640.yaml\n> > > +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> > > @@ -6,10 +6,6 @@ algorithms:\n> > > - Agc:\n> > > - Awb:\n> > > - BlackLevelCorrection:\n> > > - R: 256\n> > > - Gr: 256\n> > > - Gb: 256\n> > > - B: 256\n> > > - ColorProcessing:\n> > > - GammaSensorLinearization:\n> > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 590C9BD87C\n\tfor ;\n\tWed, 3 Jul 2024 13:19:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FB8562E24;\n\tWed, 3 Jul 2024 15:19:05 +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 D3FBD62C95\n\tfor ;\n\tWed, 3 Jul 2024 15:19:03 +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 6E9483D6;\n\tWed, 3 Jul 2024 15:18:35 +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=\"rzviEVQP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720012715;\n\tbh=um2QkyA5xsKJDv2jLGV3iWxlEVEeI69a+iJpbtar2+c=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rzviEVQPjIYQm++1zxmpj9e5Wqp3WVHk1Dx9cdLgxUklsnxK5tEE76TxlYuOhF6Zq\n\t2ia3viVqJYe1Qj7hnepNjvGxHSqiunVBf5SS7i9W7YLkPr5MrM3h83Ws1Xr+4+WA0z\n\tW0gdGcW+7vO5zoZB3IOxlM3uuMz3d/fvCvjXxoiQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240703124819.GE27927@pendragon.ideasonboard.com>","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-7-stefan.klug@ideasonboard.com>\n\t<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>\n\t<20240703124819.GE27927@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","From":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMikhail Rudenko ","To":"Laurent Pinchart ","Date":"Wed, 03 Jul 2024 14:19:00 +0100","Message-ID":"<172001274074.3353312.7954717625998615271@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":30274,"web_url":"https://patchwork.libcamera.org/comment/30274/","msgid":"<20240703131926.GA9406@pendragon.ideasonboard.com>","date":"2024-07-03T13:19:26","subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","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 03:11:04PM +0200, Stefan Klug wrote:\n> On Wed, Jul 03, 2024 at 03:48:19PM +0300, Laurent Pinchart wrote:\n> > On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote:\n> > > Quoting Stefan Klug (2024-07-03 11:39:53)\n> > > > Move black levels for tuning files that contained a BLC block into\n> > > > the camera sensor helpers.\n> > > > \n> > > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the\n> > > > value from the datasheet (scaled to 16bit).\n> > > \n> > > I wonder who chose 66 here. Was it measured by someone perhaps?\n> > > \n> > > Git blame states \"Mikhail Rudenko\" (Added to Cc) Mikhail, any opinion on\n> > > this change?\n> > > \n> > > Anyway, I don't object to this right now - I think once we release our\n> > > updates to the IPA and tuning tools, most sensors 'supported' in\n> > > libcamera will likely benefit from being '(re)tuned' correctly and\n> > > 'fully' (and consistently?).\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking\n> > > > at the commit message the 256 most likely stems from the imx219 tuning\n> > > > file and 16@10bit is the same as the 64@12bit from the ov4689. This\n> > > > seems more likely and is therefore used.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++\n> > > > src/ipa/rkisp1/data/ov4689.yaml | 4 ----\n> > > > src/ipa/rkisp1/data/ov5640.yaml | 4 ----\n> > > > 3 files changed, 2 insertions(+), 8 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > index 3d0e756927f0..982e35beaa90 100644\n> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper\n> > > > public:\n> > > > CameraSensorHelperOv4689()\n> > > > {\n> > > > + blackLevel_ = 1024;\n> > \n> > I wonder if\n> > \n> > \t\tblackLevel_ = 16 << 6;\n> > \n> > would be more readable. I suppose what you want to see at a glance, the\n> > 16-bit value, or the value for the native sensor bit depth. Up to you.\n> > If you decide to change it, please update all helpers.\n> > \n> > Reviewed-by: Laurent Pinchart \n> \n> That is a difficult one. I'd like to stick to the current version.\n> Without it, I think I would have missed that all Omnivision black levels\n> are the same, even though the documentation mentions 64@12 and 16@10.\n> Would it help to include comments above with the origin of the\n> information?\n\nI wouldn't mind comments. Up to you.\n\n> > > > gainType_ = AnalogueGainLinear;\n> > > > gainConstants_.linear = { 1, 0, 0, 128 };\n> > > > }\n> > > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper\n> > > > public:\n> > > > CameraSensorHelperOv5640()\n> > > > {\n> > > > + blackLevel_ = 1024;\n> > > > gainType_ = AnalogueGainLinear;\n> > > > gainConstants_.linear = { 1, 0, 0, 16 };\n> > > > }\n> > > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml\n> > > > index 2068684cafcd..609012967e02 100644\n> > > > --- a/src/ipa/rkisp1/data/ov4689.yaml\n> > > > +++ b/src/ipa/rkisp1/data/ov4689.yaml\n> > > > @@ -6,8 +6,4 @@ algorithms:\n> > > > - Agc:\n> > > > - Awb:\n> > > > - BlackLevelCorrection:\n> > > > - R: 66\n> > > > - Gr: 66\n> > > > - Gb: 66\n> > > > - B: 66\n> > > > ...\n> > > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> > > > index 897b83cb435b..4b21d412e44e 100644\n> > > > --- a/src/ipa/rkisp1/data/ov5640.yaml\n> > > > +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> > > > @@ -6,10 +6,6 @@ algorithms:\n> > > > - Agc:\n> > > > - Awb:\n> > > > - BlackLevelCorrection:\n> > > > - R: 256\n> > > > - Gr: 256\n> > > > - Gb: 256\n> > > > - B: 256\n> > > > - ColorProcessing:\n> > > > - GammaSensorLinearization:\n> > > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]","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 60AE5BD87C\n\tfor ;\n\tWed, 3 Jul 2024 13:19:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23EB662E24;\n\tWed, 3 Jul 2024 15:19: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 4EF8562C99\n\tfor ;\n\tWed, 3 Jul 2024 15:19:47 +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 356603D6;\n\tWed, 3 Jul 2024 15:19:19 +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=\"ZkSpXo4O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720012759;\n\tbh=AVuTHZOuE9XFMBROLaXt6WhoKZbMjIRSw6UEYYNuj3s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZkSpXo4OZmFb1wBXyrlzZUIUEi8vjQbT+ZU4iR+yrRdRHEpBqVTW81rQwEy/AAtrK\n\tADmYijcji/zEVCIhkNWVBXfUbF3lv0dYupSmXWXI1895vi469LSfbWmhH3z+qIEtWT\n\tpnp4kS2j92mQSZy3ZhlAa8UHqB9d65iXkR7oPaUw=","Date":"Wed, 3 Jul 2024 16:19:26 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMikhail Rudenko ","Subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","Message-ID":"<20240703131926.GA9406@pendragon.ideasonboard.com>","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-7-stefan.klug@ideasonboard.com>\n\t<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>\n\t<20240703124819.GE27927@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":30275,"web_url":"https://patchwork.libcamera.org/comment/30275/","msgid":"<20240703133122.GB7479@pendragon.ideasonboard.com>","date":"2024-07-03T13:31:22","subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","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 02:19:00PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-07-03 13:48:19)\n> > On Wed, Jul 03, 2024 at 01:20:56PM +0100, Kieran Bingham wrote:\n> > > Quoting Stefan Klug (2024-07-03 11:39:53)\n> > > > Move black levels for tuning files that contained a BLC block into\n> > > > the camera sensor helpers.\n> > > > \n> > > > ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the\n> > > > value from the datasheet (scaled to 16bit).\n> > > \n> > > I wonder who chose 66 here. Was it measured by someone perhaps?\n> > > \n> > > Git blame states \"Mikhail Rudenko\" (Added to Cc) Mikhail, any opinion on\n> > > this change?\n> > > \n> > > Anyway, I don't object to this right now - I think once we release our\n> > > updates to the IPA and tuning tools, most sensors 'supported' in\n> > > libcamera will likely benefit from being '(re)tuned' correctly and\n> > > 'fully' (and consistently?).\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking\n> > > > at the commit message the 256 most likely stems from the imx219 tuning\n> > > > file and 16@10bit is the same as the 64@12bit from the ov4689. This\n> > > > seems more likely and is therefore used.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/libipa/camera_sensor_helper.cpp | 2 ++\n> > > > src/ipa/rkisp1/data/ov4689.yaml | 4 ----\n> > > > src/ipa/rkisp1/data/ov5640.yaml | 4 ----\n> > > > 3 files changed, 2 insertions(+), 8 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > index 3d0e756927f0..982e35beaa90 100644\n> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper\n> > > > public:\n> > > > CameraSensorHelperOv4689()\n> > > > {\n> > > > + blackLevel_ = 1024;\n> > \n> > I wonder if\n> > \n> > blackLevel_ = 16 << 6;\n> \n> I like this idea ... except the '<< 6' isn't particularly clear at\n> conveying the 16 is in 10 bit.\n\nPossibly\n\n\t\tblackLevel_ = 16 << (16 - 10);\n\nbut then you start wondering if the left 16 is related to the right 16\n:-) I think we can stop bikeshedding on this and let Stefan decide.\n\n> \n> I guess we could add a helper\n> \tblackLevel_ = to16Bit(10 /*bit*/, 16)\n> \n> #define to16Bit(depth, val) ((val) << (16 - (depth)))\n\nIt's functions or function templates in C++ ;-)\n\n> Or some other magic ... but I definitely recall my confusion when I\n> first looked at the black levels in a datasheet and saw different values\n> at different bit depths, and had to convert again for RPi. The operation\n> is simple of course - but it's the reason/intent that may not be clear\n> to a new comer - so doing something to make that easier to bring up a\n> new sensor is a good thing IMO.\n> \n> Could be something that gets tackled later though.\n> \n> > \n> > would be more readable. I suppose what you want to see at a glance, the\n> > 16-bit value, or the value for the native sensor bit depth. Up to you.\n> > If you decide to change it, please update all helpers.\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > > > gainType_ = AnalogueGainLinear;\n> > > > gainConstants_.linear = { 1, 0, 0, 128 };\n> > > > }\n> > > > @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper\n> > > > public:\n> > > > CameraSensorHelperOv5640()\n> > > > {\n> > > > + blackLevel_ = 1024;\n> > > > gainType_ = AnalogueGainLinear;\n> > > > gainConstants_.linear = { 1, 0, 0, 16 };\n> > > > }\n> > > > diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml\n> > > > index 2068684cafcd..609012967e02 100644\n> > > > --- a/src/ipa/rkisp1/data/ov4689.yaml\n> > > > +++ b/src/ipa/rkisp1/data/ov4689.yaml\n> > > > @@ -6,8 +6,4 @@ algorithms:\n> > > > - Agc:\n> > > > - Awb:\n> > > > - BlackLevelCorrection:\n> > > > - R: 66\n> > > > - Gr: 66\n> > > > - Gb: 66\n> > > > - B: 66\n> > > > ...\n> > > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> > > > index 897b83cb435b..4b21d412e44e 100644\n> > > > --- a/src/ipa/rkisp1/data/ov5640.yaml\n> > > > +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> > > > @@ -6,10 +6,6 @@ algorithms:\n> > > > - Agc:\n> > > > - Awb:\n> > > > - BlackLevelCorrection:\n> > > > - R: 256\n> > > > - Gr: 256\n> > > > - Gb: 256\n> > > > - B: 256\n> > > > - ColorProcessing:\n> > > > - GammaSensorLinearization:\n> > > > x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]","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 300DBBEFBE\n\tfor ;\n\tWed, 3 Jul 2024 13:31:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF18962E22;\n\tWed, 3 Jul 2024 15:31: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 9055362C95\n\tfor ;\n\tWed, 3 Jul 2024 15:31:43 +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 365BF3D6;\n\tWed, 3 Jul 2024 15:31:15 +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=\"cpBC+PG2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720013475;\n\tbh=slCX6mzD6Xi37SjMISYsLYeaLY/LmjWr/ExT6C1EcRk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cpBC+PG2cuhVCN8IZ3oCyozE97y8UPZXw01Svs3iBtVnmeeCAEWqRqIOTWae508NM\n\teANRZ2jXkZ7ISuJL8Qx/lzf2BrR2i+Vad2qBoap8cpzxnrLjyasdkKzgKJ+vGAq8hQ\n\tYvhvpHCp9bnCeHD16B6aESGTeSad4MJSsJ+os5NQ=","Date":"Wed, 3 Jul 2024 16:31:22 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMikhail Rudenko ","Subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","Message-ID":"<20240703133122.GB7479@pendragon.ideasonboard.com>","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-7-stefan.klug@ideasonboard.com>\n\t<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>\n\t<20240703124819.GE27927@pendragon.ideasonboard.com>\n\t<172001274074.3353312.7954717625998615271@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172001274074.3353312.7954717625998615271@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":30284,"web_url":"https://patchwork.libcamera.org/comment/30284/","msgid":"<87h6d6m8gy.fsf@gmail.com>","date":"2024-07-03T21:58:05","subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","submitter":{"id":146,"url":"https://patchwork.libcamera.org/api/people/146/","name":"Mikhail Rudenko","email":"mike.rudenko@gmail.com"},"content":"Hi, Kieran!\n\nOn 2024-07-03 at 13:20 +01, Kieran Bingham wrote:\n\n> Quoting Stefan Klug (2024-07-03 11:39:53)\n>> Move black levels for tuning files that contained a BLC block into\n>> the camera sensor helpers.\n>>\n>> ov4689.yaml had 66@12bit while the datasheet states 64@12bit. Use the\n>> value from the datasheet (scaled to 16bit).\n>\n> I wonder who chose 66 here. Was it measured by someone perhaps?\n>\n> Git blame states \"Mikhail Rudenko\" (Added to Cc) Mikhail, any opinion on\n> this change?\n\nBack then I measured that number, not knowing that it's specified in the\ndatasheet. I think we should stick to the datasheet, so\n\nAcked-by: Mikhail Rudenko \n\n> Anyway, I don't object to this right now - I think once we release our\n> updates to the IPA and tuning tools, most sensors 'supported' in\n> libcamera will likely benefit from being '(re)tuned' correctly and\n> 'fully' (and consistently?).\n>\n> Reviewed-by: Kieran Bingham \n>\n>> ov5640.yaml had 256@12bit while the datasheet states 16@10bit. Looking\n>> at the commit message the 256 most likely stems from the imx219 tuning\n>> file and 16@10bit is the same as the 64@12bit from the ov4689. This\n>> seems more likely and is therefore used.\n>>\n>> Signed-off-by: Stefan Klug \n>> ---\n>> src/ipa/libipa/camera_sensor_helper.cpp | 2 ++\n>> src/ipa/rkisp1/data/ov4689.yaml | 4 ----\n>> src/ipa/rkisp1/data/ov5640.yaml | 4 ----\n>> 3 files changed, 2 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>> index 3d0e756927f0..982e35beaa90 100644\n>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -542,6 +542,7 @@ class CameraSensorHelperOv4689 : public CameraSensorHelper\n>> public:\n>> CameraSensorHelperOv4689()\n>> {\n>> + blackLevel_ = 1024;\n>> gainType_ = AnalogueGainLinear;\n>> gainConstants_.linear = { 1, 0, 0, 128 };\n>> }\n>> @@ -553,6 +554,7 @@ class CameraSensorHelperOv5640 : public CameraSensorHelper\n>> public:\n>> CameraSensorHelperOv5640()\n>> {\n>> + blackLevel_ = 1024;\n>> gainType_ = AnalogueGainLinear;\n>> gainConstants_.linear = { 1, 0, 0, 16 };\n>> }\n>> diff --git a/src/ipa/rkisp1/data/ov4689.yaml b/src/ipa/rkisp1/data/ov4689.yaml\n>> index 2068684cafcd..609012967e02 100644\n>> --- a/src/ipa/rkisp1/data/ov4689.yaml\n>> +++ b/src/ipa/rkisp1/data/ov4689.yaml\n>> @@ -6,8 +6,4 @@ algorithms:\n>> - Agc:\n>> - Awb:\n>> - BlackLevelCorrection:\n>> - R: 66\n>> - Gr: 66\n>> - Gb: 66\n>> - B: 66\n>> ...\n>> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n>> index 897b83cb435b..4b21d412e44e 100644\n>> --- a/src/ipa/rkisp1/data/ov5640.yaml\n>> +++ b/src/ipa/rkisp1/data/ov5640.yaml\n>> @@ -6,10 +6,6 @@ algorithms:\n>> - Agc:\n>> - Awb:\n>> - BlackLevelCorrection:\n>> - R: 256\n>> - Gr: 256\n>> - Gb: 256\n>> - B: 256\n>> - ColorProcessing:\n>> - GammaSensorLinearization:\n>> x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]\n>> --\n>> 2.43.0\n>>\n\n\n--\nBest regards,\nMikhail Rudenko","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 ADCB6BD87C\n\tfor ;\n\tWed, 3 Jul 2024 22:11:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 966AD62E22;\n\tThu, 4 Jul 2024 00:11:48 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6FF3619CC\n\tfor ;\n\tThu, 4 Jul 2024 00:11:45 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id\n\t38308e7fff4ca-2ebeefb9a7fso66045601fa.0\n\tfor ;\n\tWed, 03 Jul 2024 15:11:45 -0700 (PDT)","from razdolb ([77.220.204.220]) by smtp.gmail.com with ESMTPSA id\n\t38308e7fff4ca-2ee87433c2asm2442981fa.139.2024.07.03.15.11.43\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 03 Jul 2024 15:11:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"QJPmqxNj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1720044705; x=1720649505;\n\tdarn=lists.libcamera.org; \n\th=mime-version:message-id:in-reply-to:date:subject:cc:to:from\n\t:user-agent:references:from:to:cc:subject:date:message-id:reply-to;\n\tbh=zavl2DjirPN3bD9A8Fafj72m/NDUJ8a2C4S6KSD9+NY=;\n\tb=QJPmqxNjvF4gJKYkniI22+txrD8A+YYdZDDxt1LJzsluRna33a4uMOYI/kvjvSmxYe\n\tzbZaThvPbvPnRajuQLsMxH6HwEonsNqGdEj17lp9j9Y/Ib7DCQHVCoB4AX6huoVNxIi4\n\tGW169ZjO3wRJfXlXGwACVesi7PiGF+tOarvm0uvbTqY/tsG/KE2IINXNtpOu3kLBJ0S4\n\t6sB7oiWDs+LR2MeOs/+Z019FCLLD8VcgR+5U3o85if/VfswFB/28TI8wqSnIg7XqCsRK\n\tcJQk7M74LPrv02basB1VoS/utKMmYElTnn0td5Um7X9iD46Znxy51UGu9ejNcy6ohyCQ\n\t2lyg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1720044705; x=1720649505;\n\th=mime-version:message-id:in-reply-to:date:subject:cc:to:from\n\t:user-agent:references:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=zavl2DjirPN3bD9A8Fafj72m/NDUJ8a2C4S6KSD9+NY=;\n\tb=FLnHDbDA+GO1or9Ha5KXMIVz3ljPlK6g8gWWJcz1jSL2OIoZEZC5oX667N+qMPgKPL\n\tsL+iLBIGHyKN6K2y/wdm7ly61JsBJ4dNEDp4e+6rXG3CiDFQb3/xANt5I+qptKafCMPy\n\tBcKrwsEMbaf10mqZVuRHfI7gEOR8gRn5lxDL/pYCCeK37D1ehb3qivmcNMM+8qdU6QAj\n\tqM50fG5rz5NuhxRpBybf/J26cNOnQYj1HZsSg/0MfVzWpNw9aL5SoVMEiGeXz43dSVMx\n\tQQrjfeFeEe8f4DIlqekUvMWNqTJS8PCgCuDlvaqJNb/ICBS2unv4Daet/aUoAscFksMf\n\tMaEQ==","X-Gm-Message-State":"AOJu0YzjoRFLO3FcLDtNXnNkZVB+c7eVzaTmjf6KmoP+0hMl+j1x2kxx\n\tx8wn1twkUDUkdJPtD8CDk8nH9j4hkYm820juzliShIrVABGMDEOfuxgFzg==","X-Google-Smtp-Source":"AGHT+IF3/CWjpvtdl/z4Sqomj5sfFVPfiz8ZIL/6tzH9FI37szG/czSazZwne1/M99dCz3bbZ4yfEA==","X-Received":"by 2002:a05:651c:2113:b0:2ec:52f8:17ad with SMTP id\n\t38308e7fff4ca-2ee5e6f7913mr93022481fa.45.1720044704697; \n\tWed, 03 Jul 2024 15:11:44 -0700 (PDT)","References":"<20240703104004.184783-1-stefan.klug@ideasonboard.com>\n\t<20240703104004.184783-7-stefan.klug@ideasonboard.com>\n\t<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>","User-agent":"mu4e 1.10.8; emacs 29.3.50","From":"Mikhail Rudenko ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Stefan Klug\n\t","Subject":"Re: [PATCH v2 6/6] ipa: rkisp1: Move ov4689 and ov5640 black levels\n\tinto sensor helpers","Date":"Thu, 04 Jul 2024 00:58:05 +0300","In-reply-to":"<172000925637.3353312.16323259168271323660@ping.linuxembedded.co.uk>","Message-ID":"<87h6d6m8gy.fsf@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain","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\" "}}]