[{"id":18235,"web_url":"https://patchwork.libcamera.org/comment/18235/","msgid":"","date":"2021-07-20T00:01:55","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n> Extend the CameraSensorHelper factory with support for an\n> IMX258 sensor as found in the Nautilus Chromebook.\n> \n> The values are read by hacking the IMX258 kernel driver.\n> The values for analog gain constants are obtained by reading the\n> register indexes, corresponding to the analog gain constants, as\n> mentioned in MIPI CCS v1.1 specification.\n\nI'd expand this to explain why.\n\nThe imx258 kernel driver hints that the IMX258 sensor may be compatible\nwith the MIPI CCS specification, as the register set matches. The gain\nparameter values have been retrieved from the sensor by reading the\nanalog gain read-only registers as defined by CCS.\n\nThis being said, given that the maximum gain value doesn't match the\ndriver, I think we need to run additional tests to figure out if this is\nactually correct. Setting the gain to 0, 256, 384 and 448 should result\nin gain values of x1, x2, x4 and x8 according to the coefficients, and\nit should be possible to validate that by capturing images with a fixed\nexposure time and calculate the average luminance for the different gain\nvalues. The exposure time should be picked to have a luminance value of\nabout 10% when the gain is x1, as lower valus would result in measuring\nnoise rather than actual data, and higher values would saturate at\nhigher gains.\n\nIf the formula holds with those values, you could try setting the gain\nto values higher than 448, up to the maximum value of 0x1fff accepted by\nthe driver, to see what happens.\n\nIf 448 is indeed the maximum value, then we should fix the driver\naccordingly.\n\n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 709835a8..c43368df 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -295,6 +295,16 @@ public:\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> \n> +class CameraSensorHelperImx258 : public CameraSensorHelper\n> +{\n> +public:\n> + CameraSensorHelperImx258()\n> + {\n> + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> + }\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> +\n> class CameraSensorHelperOv5670 : public CameraSensorHelper\n> {\n> public:","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 7BC63C0109\n\tfor ;\n\tTue, 20 Jul 2021 00:02:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC4C76853F;\n\tTue, 20 Jul 2021 02:02:01 +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 0780F60277\n\tfor ;\n\tTue, 20 Jul 2021 02:02:01 +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 749D6443;\n\tTue, 20 Jul 2021 02:02:00 +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=\"nwmrUbSB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626739320;\n\tbh=UcKSnKAcJZJTib+4JuOFthYuY5PbXsfb4e0n+SgxB0M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nwmrUbSBaPKLxEF2jjxPXAawBtL6BDuP1Oet67GKaMrhAWdvAXqA3/WwBxTts17of\n\tQPrMZyfLGEPRiAHthHPWDcrIDzLm+SuULtgOW/rVBOulcC3l8t1iaFqBVuvJ5JRua9\n\tR6DfBGt4O/igLoZG9Wc/BUiZoHLdAv8OE9BRw1io=","Date":"Tue, 20 Jul 2021 03:01:55 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210719101437.326523-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18238,"web_url":"https://patchwork.libcamera.org/comment/18238/","msgid":"","date":"2021-07-20T14:25:11","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Umang and Laurent\n\nOn Tue, 20 Jul 2021 at 01:02, Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n> > Extend the CameraSensorHelper factory with support for an\n> > IMX258 sensor as found in the Nautilus Chromebook.\n> >\n> > The values are read by hacking the IMX258 kernel driver.\n> > The values for analog gain constants are obtained by reading the\n> > register indexes, corresponding to the analog gain constants, as\n> > mentioned in MIPI CCS v1.1 specification.\n>\n> I'd expand this to explain why.\n>\n> The imx258 kernel driver hints that the IMX258 sensor may be compatible\n> with the MIPI CCS specification, as the register set matches. The gain\n> parameter values have been retrieved from the sensor by reading the\n> analog gain read-only registers as defined by CCS.\n>\n> This being said, given that the maximum gain value doesn't match the\n> driver, I think we need to run additional tests to figure out if this is\n> actually correct. Setting the gain to 0, 256, 384 and 448 should result\n> in gain values of x1, x2, x4 and x8 according to the coefficients, and\n> it should be possible to validate that by capturing images with a fixed\n> exposure time and calculate the average luminance for the different gain\n> values. The exposure time should be picked to have a luminance value of\n> about 10% when the gain is x1, as lower valus would result in measuring\n> noise rather than actual data, and higher values would saturate at\n> higher gains.\n>\n> If the formula holds with those values, you could try setting the gain\n> to values higher than 448, up to the maximum value of 0x1fff accepted by\n> the driver, to see what happens.\n>\n> If 448 is indeed the maximum value, then we should fix the driver\n> accordingly.\n\nDo these comments mean you're working without a datasheet?\nWe do have a datasheet for IMX258 (sorry, can't share it), and David\nhas IMX258 largely working on the Pi with some extra patches due to\nonly having 2 data lanes available or missing controls [1].\n\nThe gain formula is stated in the datasheet as:\nAnalog gain = 512 / (512 – X)\n(m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).\n\nRange 0 to 480.\n0 = x1\n256 = x2\n384 = x4\n448 = x8\n480 = x16.\n\nTested empirically, an exposure of 200 lines with analogue gain 480\n(x16) is giving the same intensity image as exposure of 400 lines with\nanalogue gain 448 (x8).\n\n Dave\n\n[1] Slightly hacky patches and need cleaning up, but look on our\nshared kernel tree and branch 6by9/imx258.\n\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n> > 1 file changed, 10 insertions(+)\n> >\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index 709835a8..c43368df 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -295,6 +295,16 @@ public:\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> >\n> > +class CameraSensorHelperImx258 : public CameraSensorHelper\n> > +{\n> > +public:\n> > + CameraSensorHelperImx258()\n> > + {\n> > + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> > + }\n> > +};\n> > +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> > +\n> > class CameraSensorHelperOv5670 : public CameraSensorHelper\n> > {\n> > public:\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 8FDAAC322B\n\tfor ;\n\tTue, 20 Jul 2021 14:25:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAB296852A;\n\tTue, 20 Jul 2021 16:25:28 +0200 (CEST)","from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com\n\t[IPv6:2a00:1450:4864:20::42c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9221868521\n\tfor ;\n\tTue, 20 Jul 2021 16:25:27 +0200 (CEST)","by mail-wr1-x42c.google.com with SMTP id m2so26255027wrq.2\n\tfor ;\n\tTue, 20 Jul 2021 07:25:27 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"CG0TWUGP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=0eCyZnTI6UaOM6oZ5afRAVUV4zGq+SHw6kOxQQqoL5w=;\n\tb=CG0TWUGPI+mKM31DL4shUTQPbyjCQM/tZqNJsRtQcZiZynjz8RnBf26sUA4jdV7hYO\n\tNdSLZXPyKSvE3U0uecfVaJL/ByWeGupsOy6RcZAeFkv2e8QNPfPsrUX4szQVHYp0/3al\n\taR/7Tau3KnpyWjFeHgEewjxbURXjzrBfEVdxhI0gwOi9u80eauUxxine88jmq9MKtJ/C\n\tQ1wS3Dc9+R1eK0gySuhKwH+tuMUXR0WQj6pAzG32hkUZRO734DMOwCOLIukaIZouh0aG\n\te/3bIjH4mT9/8B2HaV05G6pjyr70WwtDQyftuo9WGn17RjISL6QvZ7/osCSMga0KA8qR\n\tP5WQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=0eCyZnTI6UaOM6oZ5afRAVUV4zGq+SHw6kOxQQqoL5w=;\n\tb=gqkTa569zAkUj614e5Xi/8gMxSj2HDTd+M+Wla+US576h54/Go7GwG3MbrHdTh+yVp\n\tpIFTcDJ8GiNvuLCEzRbRcigdQp+i6co5s0mx3z7sODQkHZcE9M9WwWOc88c9jhXoKS1+\n\tuVqe7r46rBEKZ6lnCeZyrNOTHOaQhmGBIrM+I7ueWBuZqMkk5gloks3bvwiERkaAHugV\n\tc8F99VgEeO+mkmFTa60ZX+zg0mLpjekNL/a+t8Gkey44yzMKmmAcOVD7WzLLy3b1E7YE\n\tBXa/Lv3750yTwqSZ4KYgBS1/nS1ADvWM5DIEshrtFBlVnnsyWwh2sMJnL87SZ/PZ2sJH\n\t5ejA==","X-Gm-Message-State":"AOAM530K+6VU7ccvrHD89j1rZvhMFyYokYjJaEOSKARHjslQi4p+swq+\n\tF9LSh9w3m12QOVeELEmW1b0oOlp2/LbQaJuR5iAY+Q7VWaUFVg==","X-Google-Smtp-Source":"ABdhPJzk08IfxlYoCZPSemy+OHQrX5DIuEorHOehFNhv66Q+ZHWXYNddPKWCkna59e+6LBi0HGSuMWkear7hIty5t7M=","X-Received":"by 2002:a5d:51c2:: with SMTP id\n\tn2mr35513338wrv.273.1626791127091; \n\tTue, 20 Jul 2021 07:25:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Tue, 20 Jul 2021 15:25:11 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18239,"web_url":"https://patchwork.libcamera.org/comment/18239/","msgid":"","date":"2021-07-20T15:05:22","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dave,\n\nOn Tue, Jul 20, 2021 at 03:25:11PM +0100, Dave Stevenson wrote:\n> On Tue, 20 Jul 2021 at 01:02, Laurent Pinchart wrote:\n> > On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n> > > Extend the CameraSensorHelper factory with support for an\n> > > IMX258 sensor as found in the Nautilus Chromebook.\n> > >\n> > > The values are read by hacking the IMX258 kernel driver.\n> > > The values for analog gain constants are obtained by reading the\n> > > register indexes, corresponding to the analog gain constants, as\n> > > mentioned in MIPI CCS v1.1 specification.\n> >\n> > I'd expand this to explain why.\n> >\n> > The imx258 kernel driver hints that the IMX258 sensor may be compatible\n> > with the MIPI CCS specification, as the register set matches. The gain\n> > parameter values have been retrieved from the sensor by reading the\n> > analog gain read-only registers as defined by CCS.\n> >\n> > This being said, given that the maximum gain value doesn't match the\n> > driver, I think we need to run additional tests to figure out if this is\n> > actually correct. Setting the gain to 0, 256, 384 and 448 should result\n> > in gain values of x1, x2, x4 and x8 according to the coefficients, and\n> > it should be possible to validate that by capturing images with a fixed\n> > exposure time and calculate the average luminance for the different gain\n> > values. The exposure time should be picked to have a luminance value of\n> > about 10% when the gain is x1, as lower valus would result in measuring\n> > noise rather than actual data, and higher values would saturate at\n> > higher gains.\n> >\n> > If the formula holds with those values, you could try setting the gain\n> > to values higher than 448, up to the maximum value of 0x1fff accepted by\n> > the driver, to see what happens.\n> >\n> > If 448 is indeed the maximum value, then we should fix the driver\n> > accordingly.\n> \n> Do these comments mean you're working without a datasheet?\n\nNot completely, but the documentation we have doesn't specify the analog\ngain model.\n\n> We do have a datasheet for IMX258 (sorry, can't share it), and David\n> has IMX258 largely working on the Pi with some extra patches due to\n> only having 2 data lanes available or missing controls [1].\n> \n> The gain formula is stated in the datasheet as:\n> Analog gain = 512 / (512 – X)\n> (m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).\n> \n> Range 0 to 480.\n\nThat's interesting, as reading the analog_gain_code_max register returns\n448. The register is documented as the \"maximum recommended analog Gain\ncode\", so maybe the sensor supports up to x16, but doesn't recommend\ngoing over x8 ?\n\n> 0 = x1\n> 256 = x2\n> 384 = x4\n> 448 = x8\n> 480 = x16.\n\nThanks for the confirmation.\n\n> Tested empirically, an exposure of 200 lines with analogue gain 480\n> (x16) is giving the same intensity image as exposure of 400 lines with\n> analogue gain 448 (x8).\n\nThat's exactly what I wanted to test :-) It also confirms the x16 limit.\n\n> [1] Slightly hacky patches and need cleaning up, but look on our\n> shared kernel tree and branch 6by9/imx258.\n\nNext time I have to work on a sensor without full documentation, I'll\ncheck there first ;-)\n\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n> > > 1 file changed, 10 insertions(+)\n> > >\n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > index 709835a8..c43368df 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > @@ -295,6 +295,16 @@ public:\n> > > };\n> > > REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> > >\n> > > +class CameraSensorHelperImx258 : public CameraSensorHelper\n> > > +{\n> > > +public:\n> > > + CameraSensorHelperImx258()\n> > > + {\n> > > + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> > > + }\n> > > +};\n> > > +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> > > +\n> > > class CameraSensorHelperOv5670 : public CameraSensorHelper\n> > > {\n> > > public:","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 B48B0C0109\n\tfor ;\n\tTue, 20 Jul 2021 15:05:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 347076853F;\n\tTue, 20 Jul 2021 17:05:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2350C68521\n\tfor ;\n\tTue, 20 Jul 2021 17:05:28 +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 9C66251D;\n\tTue, 20 Jul 2021 17:05:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eDdE0IUy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626793527;\n\tbh=Y0Rh3vAqmxhdr3T8rBgOOtRI9/dVbraKP2+UnEg0D7I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eDdE0IUy+YH45YkGVIsBHwiArMFUoHmj/j4m1DX5FJCAGB4msfy0ng3rJs6A4rcDU\n\trTUJb5lX3rLa7e16ueowAcuVDRYbiS8GqqIPq5JwT3wCOcs6iOb2gs5Ddby/8C1f5I\n\teI1SEW3S/c/DneMhXIgbPVRx8ksTS3UgWLRKTrUs=","Date":"Tue, 20 Jul 2021 18:05:22 +0300","From":"Laurent Pinchart ","To":"Dave Stevenson ","Message-ID":"","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18242,"web_url":"https://patchwork.libcamera.org/comment/18242/","msgid":"","date":"2021-07-20T19:40:55","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"On Tue, 20 Jul 2021 at 16:05, Laurent Pinchart\n wrote:\n>\n> Hi Dave,\n>\n> On Tue, Jul 20, 2021 at 03:25:11PM +0100, Dave Stevenson wrote:\n> > On Tue, 20 Jul 2021 at 01:02, Laurent Pinchart wrote:\n> > > On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n> > > > Extend the CameraSensorHelper factory with support for an\n> > > > IMX258 sensor as found in the Nautilus Chromebook.\n> > > >\n> > > > The values are read by hacking the IMX258 kernel driver.\n> > > > The values for analog gain constants are obtained by reading the\n> > > > register indexes, corresponding to the analog gain constants, as\n> > > > mentioned in MIPI CCS v1.1 specification.\n> > >\n> > > I'd expand this to explain why.\n> > >\n> > > The imx258 kernel driver hints that the IMX258 sensor may be compatible\n> > > with the MIPI CCS specification, as the register set matches. The gain\n> > > parameter values have been retrieved from the sensor by reading the\n> > > analog gain read-only registers as defined by CCS.\n> > >\n> > > This being said, given that the maximum gain value doesn't match the\n> > > driver, I think we need to run additional tests to figure out if this is\n> > > actually correct. Setting the gain to 0, 256, 384 and 448 should result\n> > > in gain values of x1, x2, x4 and x8 according to the coefficients, and\n> > > it should be possible to validate that by capturing images with a fixed\n> > > exposure time and calculate the average luminance for the different gain\n> > > values. The exposure time should be picked to have a luminance value of\n> > > about 10% when the gain is x1, as lower valus would result in measuring\n> > > noise rather than actual data, and higher values would saturate at\n> > > higher gains.\n> > >\n> > > If the formula holds with those values, you could try setting the gain\n> > > to values higher than 448, up to the maximum value of 0x1fff accepted by\n> > > the driver, to see what happens.\n> > >\n> > > If 448 is indeed the maximum value, then we should fix the driver\n> > > accordingly.\n> >\n> > Do these comments mean you're working without a datasheet?\n>\n> Not completely, but the documentation we have doesn't specify the analog\n> gain model.\n>\n> > We do have a datasheet for IMX258 (sorry, can't share it), and David\n> > has IMX258 largely working on the Pi with some extra patches due to\n> > only having 2 data lanes available or missing controls [1].\n> >\n> > The gain formula is stated in the datasheet as:\n> > Analog gain = 512 / (512 – X)\n> > (m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).\n> >\n> > Range 0 to 480.\n>\n> That's interesting, as reading the analog_gain_code_max register returns\n> 448. The register is documented as the \"maximum recommended analog Gain\n> code\", so maybe the sensor supports up to x16, but doesn't recommend\n> going over x8 ?\n\nThe datasheet I have doesn't mention the CCS spec, nor anything about\nregister 0x0086 which appears to be the CCS register for analogue gain\ncode max.\nNo mention of issues with exceeding x8. All references are to up to\n24dB of analogue gain available, and 24dB of digital\n\n> > 0 = x1\n> > 256 = x2\n> > 384 = x4\n> > 448 = x8\n> > 480 = x16.\n>\n> Thanks for the confirmation.\n>\n> > Tested empirically, an exposure of 200 lines with analogue gain 480\n> > (x16) is giving the same intensity image as exposure of 400 lines with\n> > analogue gain 448 (x8).\n>\n> That's exactly what I wanted to test :-) It also confirms the x16 limit.\n>\n> > [1] Slightly hacky patches and need cleaning up, but look on our\n> > shared kernel tree and branch 6by9/imx258.\n>\n> Next time I have to work on a sensor without full documentation, I'll\n> check there first ;-)\n\nChance I'm afraid in this case, but feel free to ask.\nWe've had a number of third parties create modules and ask for support\nin creating the relevant kernel drivers and libcamera integration.\nIMX258 happened to be one of those that David was sent recently, and I\ndid the register munging to handle only having 2 lanes, and running\nfrom a 24MHz ext clock (the mainline driver only handles 19.2MHz).\n\nI haven't resolved an issue with running the full res at\n1267Mbit/s/lane yet which is the main reason I haven't merged it to\nour vendor tree or looked at sending the patches to mainline.\nI also need to tweak my patch set over exactly which lines are read\nout. The current driver configures readout of lines 0 to 3118 (3119\nlines total) with an H & V FLIP. Do we correct that to being lines 1\nto 3118 and retain the Bayer order when flipped, or drop it to 0 to\n3117 which will change the Bayer order on existing platforms? I've\ncurrently done the latter, but it probably ought to be the former. The\narray is 3120 pixels high, so both are valid, but G_SELECTION (not\ncurrently supported on mainline) also ought to reflect it.\n\n Dave\n\n> > > > Signed-off-by: Umang Jain \n> > > > ---\n> > > > src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n> > > > 1 file changed, 10 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > index 709835a8..c43368df 100644\n> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > @@ -295,6 +295,16 @@ public:\n> > > > };\n> > > > REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> > > >\n> > > > +class CameraSensorHelperImx258 : public CameraSensorHelper\n> > > > +{\n> > > > +public:\n> > > > + CameraSensorHelperImx258()\n> > > > + {\n> > > > + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> > > > + }\n> > > > +};\n> > > > +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> > > > +\n> > > > class CameraSensorHelperOv5670 : public CameraSensorHelper\n> > > > {\n> > > > public:\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 4EC56C322B\n\tfor ;\n\tTue, 20 Jul 2021 19:41:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A631B68536;\n\tTue, 20 Jul 2021 21:41:13 +0200 (CEST)","from mail-wr1-x435.google.com (mail-wr1-x435.google.com\n\t[IPv6:2a00:1450:4864:20::435])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE06168521\n\tfor ;\n\tTue, 20 Jul 2021 21:41:11 +0200 (CEST)","by mail-wr1-x435.google.com with SMTP id d12so27182400wre.13\n\tfor ;\n\tTue, 20 Jul 2021 12:41:11 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"UElpGzsU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=WfOUFhMSMSgA1q9kiolGPM5gsG8XMuHB3+nRkz1/Yko=;\n\tb=UElpGzsUmFsTjY+i78gNkPo1+MI0EVO8HOiJ/1nvRQlpLC50RUev20LXhmDeyHkg27\n\tLmTybpn7Iw8TWI/l43CMMa6qE/65C9wtuqQdmnHN+Y8TWxUnOAnV9AcCUlZt1r++8PbP\n\tVhIu20JYHBqDre96VKEZRFzbS6cqRdwKWmMuK0ltyY/K+VPPRiNjPS2BquIFs5CN1h9W\n\tNJaIu7Y/zfBupKlUjISDYBQlygKbc819wRXzpaR6C193Zg1HERnYxnrwHI7NHB9x7qqo\n\t5LUDgZI4RjvnQQ/YpoOLxTyiWbLvkJzDuQSxAIOUdNO2MuED8wFUIWjdOyEBg+/bH8QE\n\t0vnw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=WfOUFhMSMSgA1q9kiolGPM5gsG8XMuHB3+nRkz1/Yko=;\n\tb=SVdqXVH7mhok1SbiehxagA2gLtL0n3W8tsbvXIvfYOZctHpNUCu6bVMnWdFVAMjzrE\n\tuR1DbtDwDDjKO5nom1t5N2tujFiXeHCKbQsEaLvtcLJlXKN7r3uakfqKked+v9E/z+bb\n\teokRLJD9PXrWXB+rkH4b4Ay4L3HSZwP4jtA74HQBey2MBxysvUpcAdqUWnr5609OqrJx\n\tg4vMQ1Dl+IGLNUs5dqe72V+HsX6APvi4MfVifMTFs5IBs+nBqic9Ta/CI662epFgM+Bk\n\tRqSLv4igoVDPKrRRWh18RILgw7iSmwda9lVkp3DxUPNBjYOt+kw3OwUfT0eEeb/Mzw7w\n\tjnZQ==","X-Gm-Message-State":"AOAM531vbJ4pcr0YjZLhTkclKVWlNzbFaOjO3fDySEDPPD8SnzDNR5SZ\n\to6MPNClpe1wmmps2nAO3BSvx9rDCyVIrdOV4iMWLZw==","X-Google-Smtp-Source":"ABdhPJxkEgXmsdtjVcITHiGGgKXQ1LUwiiBONtNoboHz/qICwpMVJ/MaDwpsPrQFyHNfabBHU8Ir62uO9rcmFw7018c=","X-Received":"by 2002:a5d:4b42:: with SMTP id w2mr37618641wrs.47.1626810071173;\n\tTue, 20 Jul 2021 12:41:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Tue, 20 Jul 2021 20:40:55 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18243,"web_url":"https://patchwork.libcamera.org/comment/18243/","msgid":"","date":"2021-07-21T00:02:53","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dave,\n\nOn Tue, Jul 20, 2021 at 08:40:55PM +0100, Dave Stevenson wrote:\n> On Tue, 20 Jul 2021 at 16:05, Laurent Pinchart wrote:\n> > On Tue, Jul 20, 2021 at 03:25:11PM +0100, Dave Stevenson wrote:\n> > > On Tue, 20 Jul 2021 at 01:02, Laurent Pinchart wrote:\n> > > > On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n> > > > > Extend the CameraSensorHelper factory with support for an\n> > > > > IMX258 sensor as found in the Nautilus Chromebook.\n> > > > >\n> > > > > The values are read by hacking the IMX258 kernel driver.\n> > > > > The values for analog gain constants are obtained by reading the\n> > > > > register indexes, corresponding to the analog gain constants, as\n> > > > > mentioned in MIPI CCS v1.1 specification.\n> > > >\n> > > > I'd expand this to explain why.\n> > > >\n> > > > The imx258 kernel driver hints that the IMX258 sensor may be compatible\n> > > > with the MIPI CCS specification, as the register set matches. The gain\n> > > > parameter values have been retrieved from the sensor by reading the\n> > > > analog gain read-only registers as defined by CCS.\n> > > >\n> > > > This being said, given that the maximum gain value doesn't match the\n> > > > driver, I think we need to run additional tests to figure out if this is\n> > > > actually correct. Setting the gain to 0, 256, 384 and 448 should result\n> > > > in gain values of x1, x2, x4 and x8 according to the coefficients, and\n> > > > it should be possible to validate that by capturing images with a fixed\n> > > > exposure time and calculate the average luminance for the different gain\n> > > > values. The exposure time should be picked to have a luminance value of\n> > > > about 10% when the gain is x1, as lower valus would result in measuring\n> > > > noise rather than actual data, and higher values would saturate at\n> > > > higher gains.\n> > > >\n> > > > If the formula holds with those values, you could try setting the gain\n> > > > to values higher than 448, up to the maximum value of 0x1fff accepted by\n> > > > the driver, to see what happens.\n> > > >\n> > > > If 448 is indeed the maximum value, then we should fix the driver\n> > > > accordingly.\n> > >\n> > > Do these comments mean you're working without a datasheet?\n> >\n> > Not completely, but the documentation we have doesn't specify the analog\n> > gain model.\n> >\n> > > We do have a datasheet for IMX258 (sorry, can't share it), and David\n> > > has IMX258 largely working on the Pi with some extra patches due to\n> > > only having 2 data lanes available or missing controls [1].\n> > >\n> > > The gain formula is stated in the datasheet as:\n> > > Analog gain = 512 / (512 – X)\n> > > (m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).\n> > >\n> > > Range 0 to 480.\n> >\n> > That's interesting, as reading the analog_gain_code_max register returns\n> > 448. The register is documented as the \"maximum recommended analog Gain\n> > code\", so maybe the sensor supports up to x16, but doesn't recommend\n> > going over x8 ?\n> \n> The datasheet I have doesn't mention the CCS spec, nor anything about\n> register 0x0086 which appears to be the CCS register for analogue gain\n> code max.\n\nI'm not surprised. CCS is relatively recent, although the specification\nit's based on, SMIA++, is ancient. There are lots of sensors that are\n\"nearly\" CCS-compatible (or SMIA++-compatible), sometimes because the\nsensor vendor took a previous sensor design that was compatible with the\nspec and modified it to create a new product without caring about\ncompatibility with the specification, or sometimes because they thought\nfull compatibility was hindering their creativity but decided to follow\nmost of the spec instead of reinventing the wheel. I don't know in which\nof those category (if any) the IMX258 fits. What I know is that it is\ncompatible with the specification to some extent given the register set.\nThe 448 limit in the analog_gain_code_max register may very well be a\nleft-over from the sensor that the IMX258 evolved from.\n\n> No mention of issues with exceeding x8. All references are to up to\n> 24dB of analogue gain available, and 24dB of digital\n\nOK. Let's fix the driver to limit the analog gain to 480 instead of\n0x1fff then.\n\n> > > 0 = x1\n> > > 256 = x2\n> > > 384 = x4\n> > > 448 = x8\n> > > 480 = x16.\n> >\n> > Thanks for the confirmation.\n> >\n> > > Tested empirically, an exposure of 200 lines with analogue gain 480\n> > > (x16) is giving the same intensity image as exposure of 400 lines with\n> > > analogue gain 448 (x8).\n> >\n> > That's exactly what I wanted to test :-) It also confirms the x16 limit.\n> >\n> > > [1] Slightly hacky patches and need cleaning up, but look on our\n> > > shared kernel tree and branch 6by9/imx258.\n> >\n> > Next time I have to work on a sensor without full documentation, I'll\n> > check there first ;-)\n> \n> Chance I'm afraid in this case, but feel free to ask.\n\nOr possibly the fact that if a sensor is of particular interest to\nsomeone, there's a good chance there could be other people interested in\nit too.\n\n> We've had a number of third parties create modules and ask for support\n> in creating the relevant kernel drivers and libcamera integration.\n> IMX258 happened to be one of those that David was sent recently, and I\n> did the register munging to handle only having 2 lanes, and running\n> from a 24MHz ext clock (the mainline driver only handles 19.2MHz).\n> \n> I haven't resolved an issue with running the full res at\n> 1267Mbit/s/lane yet which is the main reason I haven't merged it to\n> our vendor tree or looked at sending the patches to mainline.\n> I also need to tweak my patch set over exactly which lines are read\n> out. The current driver configures readout of lines 0 to 3118 (3119\n> lines total) with an H & V FLIP. Do we correct that to being lines 1\n> to 3118 and retain the Bayer order when flipped, or drop it to 0 to\n> 3117 which will change the Bayer order on existing platforms? I've\n> currently done the latter, but it probably ought to be the former. The\n> array is 3120 pixels high, so both are valid, but G_SELECTION (not\n> currently supported on mainline) also ought to reflect it.\n\nI've looked at the patches in your branch, and it looks pretty good\noverall. I'm tempted to take it a step further and computer register\nvalues instead of hardcoding them in tables, at least for the non\nmanufacturer-specific registers (those are at 0x3000 and above). I'm not\nasking you to do so, I may give it a try if time permits, or find a\nvolunteer to appoint. This work can be done using the CCS spec without\nthe IMX258 documentation, so there's no breach of NDA.\n\n> > > > > Signed-off-by: Umang Jain \n> > > > > ---\n> > > > > src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n> > > > > 1 file changed, 10 insertions(+)\n> > > > >\n> > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > > index 709835a8..c43368df 100644\n> > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > > @@ -295,6 +295,16 @@ public:\n> > > > > };\n> > > > > REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> > > > >\n> > > > > +class CameraSensorHelperImx258 : public CameraSensorHelper\n> > > > > +{\n> > > > > +public:\n> > > > > + CameraSensorHelperImx258()\n> > > > > + {\n> > > > > + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> > > > > + }\n> > > > > +};\n> > > > > +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> > > > > +\n> > > > > class CameraSensorHelperOv5670 : public CameraSensorHelper\n> > > > > {\n> > > > > public:","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 06EBAC0109\n\tfor ;\n\tWed, 21 Jul 2021 00:03:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A6A46853F;\n\tWed, 21 Jul 2021 02:03: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 496CB60276\n\tfor ;\n\tWed, 21 Jul 2021 02:02:59 +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 B3A7C501;\n\tWed, 21 Jul 2021 02:02:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nkMRk9Fm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626825778;\n\tbh=WRt05ulagnbslErrBNIx/yY8eBREghlfJssDz1Jxnz8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nkMRk9FmSwhH21596qEdhpzgAx463B6eyd3i+6CZdfdkOD/sVgy/IIhpGtEaR3Yvd\n\ttaBFQy4IPVIa2YGltqFESn6ahy5Qljba4fvvLcfIE4g48IfTAjIFoS9r4gTmz5r7Yc\n\tm4RdG4tQewf5NIZ0KWpyX8kAuKXkYUiYsj4rrx0o=","Date":"Wed, 21 Jul 2021 03:02:53 +0300","From":"Laurent Pinchart ","To":"Dave Stevenson ","Message-ID":"","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18244,"web_url":"https://patchwork.libcamera.org/comment/18244/","msgid":"","date":"2021-07-21T05:28:07","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent, Dave\n\nOn 7/20/21 8:35 PM, Laurent Pinchart wrote:\n> Hi Dave,\n>\n> On Tue, Jul 20, 2021 at 03:25:11PM +0100, Dave Stevenson wrote:\n>> On Tue, 20 Jul 2021 at 01:02, Laurent Pinchart wrote:\n>>> On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n>>>> Extend the CameraSensorHelper factory with support for an\n>>>> IMX258 sensor as found in the Nautilus Chromebook.\n>>>>\n>>>> The values are read by hacking the IMX258 kernel driver.\n>>>> The values for analog gain constants are obtained by reading the\n>>>> register indexes, corresponding to the analog gain constants, as\n>>>> mentioned in MIPI CCS v1.1 specification.\n>>> I'd expand this to explain why.\n>>>\n>>> The imx258 kernel driver hints that the IMX258 sensor may be compatible\n>>> with the MIPI CCS specification, as the register set matches. The gain\n>>> parameter values have been retrieved from the sensor by reading the\n>>> analog gain read-only registers as defined by CCS.\n>>>\n>>> This being said, given that the maximum gain value doesn't match the\n>>> driver, I think we need to run additional tests to figure out if this is\n>>> actually correct. Setting the gain to 0, 256, 384 and 448 should result\n>>> in gain values of x1, x2, x4 and x8 according to the coefficients, and\n>>> it should be possible to validate that by capturing images with a fixed\n>>> exposure time and calculate the average luminance for the different gain\n>>> values. The exposure time should be picked to have a luminance value of\n>>> about 10% when the gain is x1, as lower valus would result in measuring\n>>> noise rather than actual data, and higher values would saturate at\n>>> higher gains.\n>>>\n>>> If the formula holds with those values, you could try setting the gain\n>>> to values higher than 448, up to the maximum value of 0x1fff accepted by\n>>> the driver, to see what happens.\n>>>\n>>> If 448 is indeed the maximum value, then we should fix the driver\n>>> accordingly.\n>> Do these comments mean you're working without a datasheet?\n> Not completely, but the documentation we have doesn't specify the analog\n> gain model.\n>\n>> We do have a datasheet for IMX258 (sorry, can't share it), and David\n>> has IMX258 largely working on the Pi with some extra patches due to\n>> only having 2 data lanes available or missing controls [1].\n>>\n>> The gain formula is stated in the datasheet as:\n>> Analog gain = 512 / (512 – X)\n>> (m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).\n>>\n>> Range 0 to 480.\n> That's interesting, as reading the analog_gain_code_max register returns\n> 448. The register is documented as the \"maximum recommended analog Gain\n> code\", so maybe the sensor supports up to x16, but doesn't recommend\n> going over x8 ?\nI also tested x32 and x64 and notice that it still keeps on picking up \nthe gain. See below.\n>\n>> 0 = x1\n>> 256 = x2\n>> 384 = x4\n>> 448 = x8\n>> 480 = x16.\n> Thanks for the confirmation.\n\nI have a question, if we are considering x16 as the \"extended\" limit, as \nper our testing, I went further with:\n496 = x32\n504 = x64\n\ncoefficients, Does it make sense? I tested the x32 and x64 with exposure \nof 200 lines, the image is quite bright (over-exposed) but also can \nappears a better if I lower the exposure to 100 lines.\n\nLaurent, with x64 specially, the purplish-tint that I have \ncurrently(with master), goes away. Not sure if it means anything. I also \nunderstand, that \"bright\" and \"better\" are just relative terms here.\nhttps://pasteboard.co/Kc9o3jk.jpg\n\n>> Tested empirically, an exposure of 200 lines with analogue gain 480\n>> (x16) is giving the same intensity image as exposure of 400 lines with\n>> analogue gain 448 (x8).\n> That's exactly what I wanted to test :-) It also confirms the x16 limit.\n\nI tested x1, x2, x4, x8 and even x16 coefficients and I get similar \nresults. No issue till 480 = x16.\n\nPicking value above 512 (for e.g. 544) will drop the gain and brings \nback to x0, x1 levels. So I assume, x16 is a decent limit, that should \nbe reported by driver?\n\n>\n>> [1] Slightly hacky patches and need cleaning up, but look on our\n>> shared kernel tree and branch 6by9/imx258.\n> Next time I have to work on a sensor without full documentation, I'll\n> check there first ;-)\n>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n>>>> 1 file changed, 10 insertions(+)\n>>>>\n>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>> index 709835a8..c43368df 100644\n>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>> @@ -295,6 +295,16 @@ public:\n>>>> };\n>>>> REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>>>>\n>>>> +class CameraSensorHelperImx258 : public CameraSensorHelper\n>>>> +{\n>>>> +public:\n>>>> + CameraSensorHelperImx258()\n>>>> + {\n>>>> + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n>>>> + }\n>>>> +};\n>>>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n>>>> +\n>>>> class CameraSensorHelperOv5670 : public CameraSensorHelper\n>>>> {\n>>>> public:","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 8AAE0C322B\n\tfor ;\n\tWed, 21 Jul 2021 05:28:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7C8468544;\n\tWed, 21 Jul 2021 07:28:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A820A68536\n\tfor ;\n\tWed, 21 Jul 2021 07:28:12 +0200 (CEST)","from [192.168.0.107] (unknown [103.238.109.21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C0AC443;\n\tWed, 21 Jul 2021 07:28:11 +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=\"kFKTDgFC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626845292;\n\tbh=9zzmb//22NxcB4E8Mj0FDIBKcdcux+WrTzTRiNf85d4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=kFKTDgFCjGetiBDusDAD9TQZUUfY80OHN57jcvXwlXCSsN/iau+iIq96BtNZe0sWb\n\tfa6ROg7qZuetgulUQCZ2iIjbKWX4D3pQUogrJkB9oefsnAdNmix4AZtwsz3cU7pm1f\n\tgy2Z2utf/cNb5KfE8wd0N0qaia7/kjU8GAX8iiJw=","To":"Laurent Pinchart ,\n\tDave Stevenson ","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"","Date":"Wed, 21 Jul 2021 10:58:07 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18256,"web_url":"https://patchwork.libcamera.org/comment/18256/","msgid":"","date":"2021-07-22T11:58:36","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Umang\n\nOn Wed, 21 Jul 2021 at 06:28, Umang Jain wrote:\n>\n> Hi Laurent, Dave\n>\n> On 7/20/21 8:35 PM, Laurent Pinchart wrote:\n> > Hi Dave,\n> >\n> > On Tue, Jul 20, 2021 at 03:25:11PM +0100, Dave Stevenson wrote:\n> >> On Tue, 20 Jul 2021 at 01:02, Laurent Pinchart wrote:\n> >>> On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n> >>>> Extend the CameraSensorHelper factory with support for an\n> >>>> IMX258 sensor as found in the Nautilus Chromebook.\n> >>>>\n> >>>> The values are read by hacking the IMX258 kernel driver.\n> >>>> The values for analog gain constants are obtained by reading the\n> >>>> register indexes, corresponding to the analog gain constants, as\n> >>>> mentioned in MIPI CCS v1.1 specification.\n> >>> I'd expand this to explain why.\n> >>>\n> >>> The imx258 kernel driver hints that the IMX258 sensor may be compatible\n> >>> with the MIPI CCS specification, as the register set matches. The gain\n> >>> parameter values have been retrieved from the sensor by reading the\n> >>> analog gain read-only registers as defined by CCS.\n> >>>\n> >>> This being said, given that the maximum gain value doesn't match the\n> >>> driver, I think we need to run additional tests to figure out if this is\n> >>> actually correct. Setting the gain to 0, 256, 384 and 448 should result\n> >>> in gain values of x1, x2, x4 and x8 according to the coefficients, and\n> >>> it should be possible to validate that by capturing images with a fixed\n> >>> exposure time and calculate the average luminance for the different gain\n> >>> values. The exposure time should be picked to have a luminance value of\n> >>> about 10% when the gain is x1, as lower valus would result in measuring\n> >>> noise rather than actual data, and higher values would saturate at\n> >>> higher gains.\n> >>>\n> >>> If the formula holds with those values, you could try setting the gain\n> >>> to values higher than 448, up to the maximum value of 0x1fff accepted by\n> >>> the driver, to see what happens.\n> >>>\n> >>> If 448 is indeed the maximum value, then we should fix the driver\n> >>> accordingly.\n> >> Do these comments mean you're working without a datasheet?\n> > Not completely, but the documentation we have doesn't specify the analog\n> > gain model.\n> >\n> >> We do have a datasheet for IMX258 (sorry, can't share it), and David\n> >> has IMX258 largely working on the Pi with some extra patches due to\n> >> only having 2 data lanes available or missing controls [1].\n> >>\n> >> The gain formula is stated in the datasheet as:\n> >> Analog gain = 512 / (512 – X)\n> >> (m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).\n> >>\n> >> Range 0 to 480.\n> > That's interesting, as reading the analog_gain_code_max register returns\n> > 448. The register is documented as the \"maximum recommended analog Gain\n> > code\", so maybe the sensor supports up to x16, but doesn't recommend\n> > going over x8 ?\n> I also tested x32 and x64 and notice that it still keeps on picking up\n> the gain. See below.\n> >\n> >> 0 = x1\n> >> 256 = x2\n> >> 384 = x4\n> >> 448 = x8\n> >> 480 = x16.\n> > Thanks for the confirmation.\n>\n> I have a question, if we are considering x16 as the \"extended\" limit, as\n> per our testing, I went further with:\n> 496 = x32\n> 504 = x64\n>\n> coefficients, Does it make sense? I tested the x32 and x64 with exposure\n> of 200 lines, the image is quite bright (over-exposed) but also can\n> appears a better if I lower the exposure to 100 lines.\n\nWe're straying into undocumented territory!\n\nMy quick tests of having the preview running and swapping between\nv4l2-ctl --set-ctrl=analogue_gain=480 --set-ctrl=exposure=80\nv4l2-ctl --set-ctrl=analogue_gain=496 --set-ctrl=exposure=40\nv4l2-ctl --set-ctrl=analogue_gain=504 --set-ctrl=exposure=20\nI do see a slight difference in brightness between the steps. 496 is\nslightly brighter than 480, and 504 is brighter again. The noise is\ngetting significant too, but that's not unexpected.\n\n> Laurent, with x64 specially, the purplish-tint that I have\n> currently(with master), goes away. Not sure if it means anything. I also\n> understand, that \"bright\" and \"better\" are just relative terms here.\n> https://pasteboard.co/Kc9o3jk.jpg\n\nFor me the images go weird on highlights when the analogue gain exceeds 499.\nSee images at https://photos.app.goo.gl/6BNHZnZVkWYgo6sp6 (phone\ncamera images of preview running. Analogue gain settings in the image\ndescriptions).\n\nI've checked the raws, and those highlights are going to black there,\nso it's not our ISP going crazy.\n\n> >> Tested empirically, an exposure of 200 lines with analogue gain 480\n> >> (x16) is giving the same intensity image as exposure of 400 lines with\n> >> analogue gain 448 (x8).\n> > That's exactly what I wanted to test :-) It also confirms the x16 limit.\n>\n> I tested x1, x2, x4, x8 and even x16 coefficients and I get similar\n> results. No issue till 480 = x16.\n>\n> Picking value above 512 (for e.g. 544) will drop the gain and brings\n> back to x0, x1 levels. So I assume, x16 is a decent limit, that should\n> be reported by driver?\n\nThe datasheet does list it as a 9 bit value (1-bit in 0x0204, 8-bits\nin 0x0205), so values above 512 will be losing their top bits.\n\nI'll certainly agree that the driver shouldn't be advertising a\nmaximum of 0x1fff.\n480 (x16) would be the sensible max value based on the datasheet,\nalthough there's possibly a case to extend it to 496 (x32) with some\nmore testing.\n\n Dave\n\n> >\n> >> [1] Slightly hacky patches and need cleaning up, but look on our\n> >> shared kernel tree and branch 6by9/imx258.\n> > Next time I have to work on a sensor without full documentation, I'll\n> > check there first ;-)\n> >\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>> src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n> >>>> 1 file changed, 10 insertions(+)\n> >>>>\n> >>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> >>>> index 709835a8..c43368df 100644\n> >>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> >>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> >>>> @@ -295,6 +295,16 @@ public:\n> >>>> };\n> >>>> REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> >>>>\n> >>>> +class CameraSensorHelperImx258 : public CameraSensorHelper\n> >>>> +{\n> >>>> +public:\n> >>>> + CameraSensorHelperImx258()\n> >>>> + {\n> >>>> + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> >>>> + }\n> >>>> +};\n> >>>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> >>>> +\n> >>>> class CameraSensorHelperOv5670 : public CameraSensorHelper\n> >>>> {\n> >>>> public:","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 B3728C322B\n\tfor ;\n\tThu, 22 Jul 2021 11:58:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65F8C68543;\n\tThu, 22 Jul 2021 13:58:54 +0200 (CEST)","from mail-wm1-x333.google.com (mail-wm1-x333.google.com\n\t[IPv6:2a00:1450:4864:20::333])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B0C26027A\n\tfor ;\n\tThu, 22 Jul 2021 13:58:53 +0200 (CEST)","by mail-wm1-x333.google.com with SMTP id\n\tu8-20020a7bcb080000b02901e44e9caa2aso2800909wmj.4\n\tfor ;\n\tThu, 22 Jul 2021 04:58:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Wt52OJ0c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=bMh9Km+CDVhbljhaN8sxIIVtNtoedGJlj+/ES2qayqs=;\n\tb=Wt52OJ0cb3IkHyyLptRxD7Yp6SOgv1j8dFTfh1CPoq8idcq+Eg5gjclpnelBU9I4bk\n\tiafKlzhAHvfkQDKbtH3jdq8abKJJ6yKbD6a5WgNoysbq+5Vx7YjwhE1eK0JuOWH5Banf\n\tV7izwlORJ/6jfEbevcVsasja5Ag0qH6Oob95TFNjXsor/463zAauIwhqxdgpkSk7b1zt\n\tmKa4EzPCb/GhjgU74aFu26sDWWFPSaATPjGUeBRximx2dhX6Mvn5F9NjjFkwcQi5i8rx\n\t84Hs1bl0ZA80uG4bd7KOqeF5boO6c3O8mrZihpLfUCLRxjFfzT5ZGa39Uhrv9wqNjm4l\n\tRIEg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=bMh9Km+CDVhbljhaN8sxIIVtNtoedGJlj+/ES2qayqs=;\n\tb=N+XeHeD3NPCke6n+svdjqmAwNvTQza1GHLNIW5/T3imZF7LXpDfZ1T9DlAl7FTVaVo\n\tQlN6+e4YGi2Qm1OTUfSK9Mq+D0r+UHghYAfjiBt4m3QLx5dSFRoPXWW0oVXfGRUd3gVI\n\t3Ze8zoVHnjQZUdLEb5EoGaJKAss0rlAeXfJWRhHJZ/cMq+tO+DX1KSG8K9tF/zeYeYcv\n\ttGTtDAaYGKcRL9sZogWuz6z/OyxPXNTtnOUpGoPFKK153WqnX3UtAGsM8HuTQ/bEwP8O\n\twkUsyrC3kkTqtm3IWvFga5f51nFGu/vcVAjyi7nDnN6peSTxRAeI3XJCdalC2xZ7NiF4\n\t0nJw==","X-Gm-Message-State":"AOAM530ztjMWKeaTsiawQJH6xtSXVH60M2ZDqsC9EhGa5pQK60JTYUZx\n\tNkN2xxoAbg8fX8WgOADiuAMwJK30GclqsEUeOxQXOA==","X-Google-Smtp-Source":"ABdhPJxI4TCzLDd17VWjMWGhvHWTuu6lOEaJcBpY88H9pWvoiYC2zfwSDK9o6Mp7s5de/OsnnhM8tYpiZhKdboqgGMk=","X-Received":"by 2002:a05:600c:4fcf:: with SMTP id\n\to15mr8747437wmq.116.1626955133142; \n\tThu, 22 Jul 2021 04:58:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Thu, 22 Jul 2021 12:58:36 +0100","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18263,"web_url":"https://patchwork.libcamera.org/comment/18263/","msgid":"","date":"2021-07-22T13:19:04","subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Dave,\n\nOn 7/22/21 5:28 PM, Dave Stevenson wrote:\n> Hi Umang\n>\n> On Wed, 21 Jul 2021 at 06:28, Umang Jain wrote:\n>> Hi Laurent, Dave\n>>\n>> On 7/20/21 8:35 PM, Laurent Pinchart wrote:\n>>> Hi Dave,\n>>>\n>>> On Tue, Jul 20, 2021 at 03:25:11PM +0100, Dave Stevenson wrote:\n>>>> On Tue, 20 Jul 2021 at 01:02, Laurent Pinchart wrote:\n>>>>> On Mon, Jul 19, 2021 at 03:44:36PM +0530, Umang Jain wrote:\n>>>>>> Extend the CameraSensorHelper factory with support for an\n>>>>>> IMX258 sensor as found in the Nautilus Chromebook.\n>>>>>>\n>>>>>> The values are read by hacking the IMX258 kernel driver.\n>>>>>> The values for analog gain constants are obtained by reading the\n>>>>>> register indexes, corresponding to the analog gain constants, as\n>>>>>> mentioned in MIPI CCS v1.1 specification.\n>>>>> I'd expand this to explain why.\n>>>>>\n>>>>> The imx258 kernel driver hints that the IMX258 sensor may be compatible\n>>>>> with the MIPI CCS specification, as the register set matches. The gain\n>>>>> parameter values have been retrieved from the sensor by reading the\n>>>>> analog gain read-only registers as defined by CCS.\n>>>>>\n>>>>> This being said, given that the maximum gain value doesn't match the\n>>>>> driver, I think we need to run additional tests to figure out if this is\n>>>>> actually correct. Setting the gain to 0, 256, 384 and 448 should result\n>>>>> in gain values of x1, x2, x4 and x8 according to the coefficients, and\n>>>>> it should be possible to validate that by capturing images with a fixed\n>>>>> exposure time and calculate the average luminance for the different gain\n>>>>> values. The exposure time should be picked to have a luminance value of\n>>>>> about 10% when the gain is x1, as lower valus would result in measuring\n>>>>> noise rather than actual data, and higher values would saturate at\n>>>>> higher gains.\n>>>>>\n>>>>> If the formula holds with those values, you could try setting the gain\n>>>>> to values higher than 448, up to the maximum value of 0x1fff accepted by\n>>>>> the driver, to see what happens.\n>>>>>\n>>>>> If 448 is indeed the maximum value, then we should fix the driver\n>>>>> accordingly.\n>>>> Do these comments mean you're working without a datasheet?\n>>> Not completely, but the documentation we have doesn't specify the analog\n>>> gain model.\n>>>\n>>>> We do have a datasheet for IMX258 (sorry, can't share it), and David\n>>>> has IMX258 largely working on the Pi with some extra patches due to\n>>>> only having 2 data lanes available or missing controls [1].\n>>>>\n>>>> The gain formula is stated in the datasheet as:\n>>>> Analog gain = 512 / (512 – X)\n>>>> (m0 = 0, m1 = -1, c0 = 512, c1 = 512, as you've read back).\n>>>>\n>>>> Range 0 to 480.\n>>> That's interesting, as reading the analog_gain_code_max register returns\n>>> 448. The register is documented as the \"maximum recommended analog Gain\n>>> code\", so maybe the sensor supports up to x16, but doesn't recommend\n>>> going over x8 ?\n>> I also tested x32 and x64 and notice that it still keeps on picking up\n>> the gain. See below.\n>>>> 0 = x1\n>>>> 256 = x2\n>>>> 384 = x4\n>>>> 448 = x8\n>>>> 480 = x16.\n>>> Thanks for the confirmation.\n>> I have a question, if we are considering x16 as the \"extended\" limit, as\n>> per our testing, I went further with:\n>> 496 = x32\n>> 504 = x64\n>>\n>> coefficients, Does it make sense? I tested the x32 and x64 with exposure\n>> of 200 lines, the image is quite bright (over-exposed) but also can\n>> appears a better if I lower the exposure to 100 lines.\n> We're straying into undocumented territory!\nIndeed, we're.\n>\n> My quick tests of having the preview running and swapping between\n> v4l2-ctl --set-ctrl=analogue_gain=480 --set-ctrl=exposure=80\n> v4l2-ctl --set-ctrl=analogue_gain=496 --set-ctrl=exposure=40\n> v4l2-ctl --set-ctrl=analogue_gain=504 --set-ctrl=exposure=20\n> I do see a slight difference in brightness between the steps. 496 is\n> slightly brighter than 480, and 504 is brighter again. The noise is\n> getting significant too, but that's not unexpected.\n>\n>> Laurent, with x64 specially, the purplish-tint that I have\n>> currently(with master), goes away. Not sure if it means anything. I also\n>> understand, that \"bright\" and \"better\" are just relative terms here.\n>> https://pasteboard.co/Kc9o3jk.jpg\n> For me the images go weird on highlights when the analogue gain exceeds 499.\n> See images at https://photos.app.goo.gl/6BNHZnZVkWYgo6sp6 (phone\n> camera images of preview running. Analogue gain settings in the image\n> descriptions).\nAh yes, analogue gain at 500 seems to make quite notorious highlights \ncompared to other samples.\n>\n> I've checked the raws, and those highlights are going to black there,\n> so it's not our ISP going crazy.\n>\n>>>> Tested empirically, an exposure of 200 lines with analogue gain 480\n>>>> (x16) is giving the same intensity image as exposure of 400 lines with\n>>>> analogue gain 448 (x8).\n>>> That's exactly what I wanted to test :-) It also confirms the x16 limit.\n>> I tested x1, x2, x4, x8 and even x16 coefficients and I get similar\n>> results. No issue till 480 = x16.\n>>\n>> Picking value above 512 (for e.g. 544) will drop the gain and brings\n>> back to x0, x1 levels. So I assume, x16 is a decent limit, that should\n>> be reported by driver?\n> The datasheet does list it as a 9 bit value (1-bit in 0x0204, 8-bits\n> in 0x0205), so values above 512 will be losing their top bits.\nI see and expecting this. Thanks for confirming. :-)\n>\n> I'll certainly agree that the driver shouldn't be advertising a\n> maximum of 0x1fff.\n> 480 (x16) would be the sensible max value based on the datasheet,\n\nIndeed, we think we'll push for 480 (x16) upstream since it's based on \ndatasheet. The register reads 448 (x8) but nevertheless, both these data \npoints should be able to justify that the kernel driver is currently \nadvertising an arbitrary max analog gain value.\n\nThanks for your inputs. You've been quite helpful. :-)\n\n> although there's possibly a case to extend it to 496 (x32) with some\n> more testing.\n>\n> Dave\n>\n>>>> [1] Slightly hacky patches and need cleaning up, but look on our\n>>>> shared kernel tree and branch 6by9/imx258.\n>>> Next time I have to work on a sensor without full documentation, I'll\n>>> check there first ;-)\n>>>\n>>>>>> Signed-off-by: Umang Jain \n>>>>>> ---\n>>>>>> src/ipa/libipa/camera_sensor_helper.cpp | 10 ++++++++++\n>>>>>> 1 file changed, 10 insertions(+)\n>>>>>>\n>>>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>>>> index 709835a8..c43368df 100644\n>>>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>>>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>>>> @@ -295,6 +295,16 @@ public:\n>>>>>> };\n>>>>>> REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>>>>>>\n>>>>>> +class CameraSensorHelperImx258 : public CameraSensorHelper\n>>>>>> +{\n>>>>>> +public:\n>>>>>> + CameraSensorHelperImx258()\n>>>>>> + {\n>>>>>> + analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n>>>>>> + }\n>>>>>> +};\n>>>>>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n>>>>>> +\n>>>>>> class CameraSensorHelperOv5670 : public CameraSensorHelper\n>>>>>> {\n>>>>>> public:","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 3CBCDC0109\n\tfor ;\n\tThu, 22 Jul 2021 13:19:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94EA968546;\n\tThu, 22 Jul 2021 15:19:21 +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 5A98B6027A\n\tfor ;\n\tThu, 22 Jul 2021 15:19:19 +0200 (CEST)","from [192.168.0.107] (unknown [103.238.109.21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F77C465;\n\tThu, 22 Jul 2021 15:19:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ra67S824\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626959958;\n\tbh=eBtK655m5Fh/yteY8rpJWmnqPySFYi7BXHDAOvGzWSY=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Ra67S824dDCvsp/7HGPEQrBAUpgnl/x8vn8YeKjUelImXcrrs/t7d/blXq5FFqU4y\n\tk47OjjhzIvqnsZTbm47dqww4DWWdKXWhXaSTAthcE4szwa8/SM4t1FlItwrwqQnobs\n\tQKdskhG0AKP1y9oeCwNDZbkfixvZ5PkVTuV7W+eE=","To":"Dave Stevenson ","References":"<20210719101437.326523-1-umang.jain@ideasonboard.com>\n\t<20210719101437.326523-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"","Date":"Thu, 22 Jul 2021 18:49:04 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/1] libipa: Add CameraSensorHelper\n\tfor IMX258","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]