[{"id":18019,"web_url":"https://patchwork.libcamera.org/comment/18019/","msgid":"","date":"2021-07-08T08:13:11","subject":"Re: [libcamera-devel] [PATCH] ipa: libipa: Fix ov5670 analogue gain\n\tparameters","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Jul 05, 2021 at 06:01:20PM +0200, Jean-Michel Hautbois wrote:\n> Based on datasheet, the gain multiplier is 128 and not 256 (the\n> fraction bits are the 7 lower bits). Fix it.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Change-Id: I87e4b078f65c68da005a3ad4f1216256b8487df7\n\nYou know about this already :-)\n\n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 23335ed6..cd0a19bb 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -294,7 +294,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv5670()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n\nInterestingly, the OV5670 supports two different gain models at the\nhardware level, with a register bit to select which model to use. Bit 2\nin register 0x3503 selects the \"real gain format\" (linear) when set to\n0, and the \"sensor gain format\" (exponential) when set to 1. I really\nwonder why.\n\nI've had a look at the driver, and quite interestingly, it selects the\nlinear gain model in all cases except for the 2560x1440 mode, where the\nexponential gain model is used. Register 0x3503 is set to 0x04 towards\nthe beginning of the mode-specific register arrays for all modes, and\nthen set to 0x00 towards the end, except for 2560x1440 where the 0x00\nentry is missing.\n\nhttps://lore.kernel.org/linux-media/YOay2tIJxpBzYKzW@pendragon.ideasonboard.com/\n\nI believe that's a driver bug, and this patch looks good to me.\n\nReviewed-by: Laurent Pinchart \n\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)","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 05A99BD794\n\tfor ;\n\tThu, 8 Jul 2021 08:14:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40BFE6851A;\n\tThu, 8 Jul 2021 10:14: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 6EA1F68509\n\tfor ;\n\tThu, 8 Jul 2021 10:13:56 +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 CBB9FE7;\n\tThu, 8 Jul 2021 10:13:55 +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=\"M2+bE9BT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625732036;\n\tbh=xNMDGLMkXXnva8z7djW0daXQE+XVy6U1pZZtP+ZPXpc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M2+bE9BTGj/OJP2xl5LA2/Ww+0enG6QcLm1P4wQH9475N883i/rMrjwrKZKORnCLX\n\t3atjFzGhXIoilIKHIw24JEVrw3plIJ3zLvmE2ZCsDbVnvT6DdI0VsvyZuLVo0IhkJI\n\t26nndLogasOkEZV2d40eDRt/jog6W+UyDknAEsN4=","Date":"Thu, 8 Jul 2021 11:13:11 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210705160120.86313-1-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210705160120.86313-1-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: libipa: Fix ov5670 analogue gain\n\tparameters","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":18020,"web_url":"https://patchwork.libcamera.org/comment/18020/","msgid":"<0a692633-e031-0226-2267-e9030068b446@ideasonboard.com>","date":"2021-07-08T09:15:41","subject":"Re: [libcamera-devel] [PATCH] ipa: libipa: Fix ov5670 analogue gain\n\tparameters","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 08/07/2021 10:13, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Mon, Jul 05, 2021 at 06:01:20PM +0200, Jean-Michel Hautbois wrote:\n>> Based on datasheet, the gain multiplier is 128 and not 256 (the\n>> fraction bits are the 7 lower bits). Fix it.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> Change-Id: I87e4b078f65c68da005a3ad4f1216256b8487df7\n> \n> You know about this already :-)\n\nIndeed :-).\n\n>> ---\n>> src/ipa/libipa/camera_sensor_helper.cpp | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>> index 23335ed6..cd0a19bb 100644\n>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -294,7 +294,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n>> public:\n>> \tCameraSensorHelperOv5670()\n>> \t{\n>> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> \n> Interestingly, the OV5670 supports two different gain models at the\n> hardware level, with a register bit to select which model to use. Bit 2\n> in register 0x3503 selects the \"real gain format\" (linear) when set to\n> 0, and the \"sensor gain format\" (exponential) when set to 1. I really\n> wonder why.\n> \n> I've had a look at the driver, and quite interestingly, it selects the\n> linear gain model in all cases except for the 2560x1440 mode, where the\n> exponential gain model is used. Register 0x3503 is set to 0x04 towards\n> the beginning of the mode-specific register arrays for all modes, and\n> then set to 0x00 towards the end, except for 2560x1440 where the 0x00\n> entry is missing.\n> \n> https://lore.kernel.org/linux-media/YOay2tIJxpBzYKzW@pendragon.ideasonboard.com/\n\nNice catch !\nI wonder if other OV* drivers may have the same issue...\n\n> I believe that's a driver bug, and this patch looks good to me.\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> \t}\n>> };\n>> REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\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 34ACABD794\n\tfor ;\n\tThu, 8 Jul 2021 09:15:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E53666851A;\n\tThu, 8 Jul 2021 11:15: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 EF2F268509\n\tfor ;\n\tThu, 8 Jul 2021 11:15:43 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:dc81:cff2:f1bb:a4ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B6A10E7;\n\tThu, 8 Jul 2021 11:15:43 +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=\"fMAQwY2V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625735743;\n\tbh=i+iZXRa70qeLcvNHiAbuZwi3IlPJAJnU+MGftNXqDRY=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=fMAQwY2VXLXzZu4mdICyn7IxNmmck0hXpi/K8AZ40IAi8/A83QMFvmndl7CZVA9hS\n\tQcfArKOJxXdf19a7eisNN6F6Ix5cZm1nHNuBvqn3pff9Oz3VKE4h4uBYfW0gZ+wcCn\n\tQqdM0ZEiTNSY/lXx5LfiXSqAFTNh83y33zVAkcac=","To":"Laurent Pinchart ","References":"<20210705160120.86313-1-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<0a692633-e031-0226-2267-e9030068b446@ideasonboard.com>","Date":"Thu, 8 Jul 2021 11:15:41 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] ipa: libipa: Fix ov5670 analogue gain\n\tparameters","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\" "}}]