[{"id":25626,"web_url":"https://patchwork.libcamera.org/comment/25626/","msgid":"<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>","date":"2022-10-27T16:41:08","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicholas,\n\nplease pardon me for being pedantic, but I would:\nlibcamera: Add support for ov8858 sensor\n\ninstead of:\nipa: libcamera: add metadata for the ov8858 sensor\n\n\"metadata\" is an overloaded already term and usually refers to the\nproperties associated to a captured frame (the term comes from the\nAndroid lingo, but we use it too).\n\nOn Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via libcamera-devel wrote:\n> From: Nicholas Roth \n>\n> Currently, libcamera does not have information for the ov8858 sensor\n> used in the PinePhone Pro, a phone designed to run Linux.\n>\n> This commit adds metadata, especially that sensor gain is reported and\n> set in 1/16 discrete increments.\n>\n> For more information, see \"5.8 manual exposure compensation/ manual\n> gain compensation\" in [0].\n>\n> [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n\nThe sensor doesn't seem to be supported mainline :(\nlibcamera has a policy that requires supported components to be\nupstream or at least on their way to upstream (ie posted to the\nlinux-media mailing list). The policy is there because different\ndownstream driver implementations might behave differently one from\nthe other, making it impossible for libcamera to support the part\nfully. The policy is a strict requirement for ISPs, I guess we're a\nbit more elastic when it comes to sensor. Howver knowing what driver\nyou are using, where it lives, and if there's any plan to upstream it\nwould be great.\n\nLet's start from the first point: where does this driver lives ? What\nkernel are you using ?\n\nKnowing what driver you're using is relevant, in example, as the\nOV5688 sensor computes exposure in 1/16 of line length. This is not\nwhat libcamera expects, and sensor drivers are expected to express the\nV4L2_CID_EXPOSURE control in line units.\n\nFrom Documentation/sensor_driver_requirements.rst\n\nWhile V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\nrequires it to be expressed as a number of image lines. Camera sensor drivers\nthat do not comply with this requirement will need to be adapted or will produce\nincorrect results.\n\n>\n> Signed-off-by: Nicholas Roth \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> 2 files changed, 25 insertions(+)\n>\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 35056bec..f2040cbd 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -476,6 +476,17 @@ public:\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>\n> +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv8858()\n> +\t{\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> +\t}\n> +};\n\nNice this matches the CCS defined linear gain model.\nHowever the sensor allows to select two formats for the analogue gain\nthe \"real gain\" format and \"sensor gain\" format. The selection is made by\nregister 0x3503[2] and it would be nice to validate that the driver\nuses the format that you expect.\n\n> +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n\nAh! that's a weird name for the sensor entity :)\n\n> +\n> class CameraSensorHelperOv8865 : public CameraSensorHelper\n> {\n> public:\n> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> index e5f27f06..d0757c15 100644\n> --- a/src/libcamera/camera_sensor_properties.cpp\n> +++ b/src/libcamera/camera_sensor_properties.cpp\n> @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> \t\t\t\t */\n> \t\t\t},\n> \t\t} },\n> +\t\t{ \"m00_f_ov8858\", {\n> +\t\t\t.unitCellSize = { 1200, 1200 },\n\nI read 1.12 um x 1.12 um\n\n> +\t\t\t.testPatternModes = {\n> +\t\t\t\t{ controls::draft::TestPatternModeOff, 0 },\n> +\t\t\t\t{ controls::draft::TestPatternModeColorBars, 1 },\n> +\t\t\t\t/*\n> +\t\t\t\t * No best corresponding test pattern for:\n> +\t\t\t\t * 1: \"Vertical Color Bar Type 1\",\n> +\t\t\t\t * 2: \"Vertical Color Bar Type 2\",\n> +\t\t\t\t * 3: \"Vertical Color Bar Type 3\",\n> +\t\t\t\t * 4: \"Vertical Color Bar Type 4\"\n> +\t\t\t\t */\n> +\t\t\t},\n> +\t\t} },\n> \t\t{ \"ov8865\", {\n> \t\t\t.unitCellSize = { 1400, 1400 },\n> \t\t\t.testPatternModes = {\n> --\n> 2.34.1\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 BA62FBDB16\n\tfor ;\n\tThu, 27 Oct 2022 16:41:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F114462F9A;\n\tThu, 27 Oct 2022 18:41:15 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8FD1462F89\n\tfor ;\n\tThu, 27 Oct 2022 18:41:14 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4EC8D240004;\n\tThu, 27 Oct 2022 16:41:10 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666888876;\n\tbh=Ce/WppyRst3NHSTGggEshNIFFDmuLk2tzPPQsGjv84k=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Y2NQLoAwVGtcIB7yRZbnCNew5cWYF0Y5KxS4TDQh13NSR4xCprSH2O3oYohlKFtbU\n\tv5J7/WvbFOtVTgPqWgfj5khRGR6+B+QJYydD5BMY5I8tnKSiP/XLiTCGxexyX9RLje\n\tBnA65gZ99KkfmPjq2FV955WJNtor2zvNUpyDAB3SDJZSVtq6FXU9AZaRxA3Sg1jSxo\n\t60sI1pWzVU9hIWu1W4wuDb+7hFx6Ov5nFj8ttWLVxr6aScbVPv/NPVPoN5C/gS2hmw\n\tXudEvJ+37GTIEqpTLn5ghrM3ZrylmvRu/bt9oJ2cmPHj5ooP0Q/DydQvU4XSoAdoQu\n\tUDK0odepOUNNw==","Date":"Thu, 27 Oct 2022 18:41:08 +0200","To":"Nicholas Roth via libcamera-devel ","Message-ID":"<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>","References":"\n\t<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221027055515.321791-5-nicholas@rothemail.net>","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"nicholas@rothemail.net","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25631,"web_url":"https://patchwork.libcamera.org/comment/25631/","msgid":"","date":"2022-10-27T17:25:20","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":137,"url":"https://patchwork.libcamera.org/api/people/137/","name":"Nicholas Roth","email":"nicholas@rothemail.net"},"content":"> please pardon me for being pedantic, but I would:\n> libcamera: Add support for ov8858 sensor\n>\n> instead of:\n> ipa: libcamera: add metadata for the ov8858 sensor\n\nHappy to make the change.\n\n> The sensor doesn't seem to be supported mainline :(...\n> Let's start from the first point: where does this driver lives ? What\n> kernel are you using ?\nI'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n\nIt looks like my package manager is picking up the Manjaro kernel from\nhttps://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,\nwhich sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no 8858-related\nchanges. This ultimately pulls the driver from\nhttps://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n.\n\nLooks like this has been in and out of Torvalds' peripheral view, e.g.:\nhttps://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n\nIf you think that it would be valuable to try and mainline this, I'd be\ninterested to do so once I can get libcamera working on my hardware, though\nI'll likely need some guidance in the process, since I'll be learning a lot.\n\n> Nice this matches the CCS defined linear gain model.\n> However the sensor allows to select two formats for the analogue gain\n> the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> register 0x3503[2] and it would be nice to validate that the driver\n> uses the format that you expect.\nFrankly, I'm not sure what you mean by this, but I'll try to find out from\nthe driver above and verify.\n\n> Knowing what driver you're using is relevant, in example, as the\n> OV5688 sensor computes exposure in 1/16 of line length. This is not\n> what libcamera expects, and sensor drivers are expected to express the\n> V4L2_CID_EXPOSURE control in line units.\n\n> From Documentation/sensor_driver_requirements.rst\n\n> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> requires it to be expressed as a number of image lines. Camera sensor\ndrivers\n> that do not comply with this requirement will need to be adapted or will\nproduce\n> incorrect results.\n\nLet me read the driver and get back to you.\n\n\nOn Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi wrote:\n\n> Hi Nicholas,\n>\n> please pardon me for being pedantic, but I would:\n> libcamera: Add support for ov8858 sensor\n>\n> instead of:\n> ipa: libcamera: add metadata for the ov8858 sensor\n>\n> \"metadata\" is an overloaded already term and usually refers to the\n> properties associated to a captured frame (the term comes from the\n> Android lingo, but we use it too).\n>\n> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> libcamera-devel wrote:\n> > From: Nicholas Roth \n> >\n> > Currently, libcamera does not have information for the ov8858 sensor\n> > used in the PinePhone Pro, a phone designed to run Linux.\n> >\n> > This commit adds metadata, especially that sensor gain is reported and\n> > set in 1/16 discrete increments.\n> >\n> > For more information, see \"5.8 manual exposure compensation/ manual\n> > gain compensation\" in [0].\n> >\n> > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n>\n> The sensor doesn't seem to be supported mainline :(\n> libcamera has a policy that requires supported components to be\n> upstream or at least on their way to upstream (ie posted to the\n> linux-media mailing list). The policy is there because different\n> downstream driver implementations might behave differently one from\n> the other, making it impossible for libcamera to support the part\n> fully. The policy is a strict requirement for ISPs, I guess we're a\n> bit more elastic when it comes to sensor. Howver knowing what driver\n> you are using, where it lives, and if there's any plan to upstream it\n> would be great.\n>\n> Let's start from the first point: where does this driver lives ? What\n> kernel are you using ?\n>\n> Knowing what driver you're using is relevant, in example, as the\n> OV5688 sensor computes exposure in 1/16 of line length. This is not\n> what libcamera expects, and sensor drivers are expected to express the\n> V4L2_CID_EXPOSURE control in line units.\n>\n> From Documentation/sensor_driver_requirements.rst\n>\n> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> requires it to be expressed as a number of image lines. Camera sensor\n> drivers\n> that do not comply with this requirement will need to be adapted or will\n> produce\n> incorrect results.\n>\n> >\n> > Signed-off-by: Nicholas Roth \n> > ---\n> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> > 2 files changed, 25 insertions(+)\n> >\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n> b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index 35056bec..f2040cbd 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -476,6 +476,17 @@ public:\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> >\n> > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> > +{\n> > +public:\n> > + CameraSensorHelperOv8858()\n> > + {\n> > + gainType_ = AnalogueGainLinear;\n> > + gainConstants_.linear = { 1, 0, 0, 16 };\n> > + }\n> > +};\n>\n> Nice this matches the CCS defined linear gain model.\n> However the sensor allows to select two formats for the analogue gain\n> the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> register 0x3503[2] and it would be nice to validate that the driver\n> uses the format that you expect.\n>\n> > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n>\n> Ah! that's a weird name for the sensor entity :)\n>\n> > +\n> > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > {\n> > public:\n> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> b/src/libcamera/camera_sensor_properties.cpp\n> > index e5f27f06..d0757c15 100644\n> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > @@ -146,6 +146,20 @@ const CameraSensorProperties\n> *CameraSensorProperties::get(const std::string &sen\n> > */\n> > },\n> > } },\n> > + { \"m00_f_ov8858\", {\n> > + .unitCellSize = { 1200, 1200 },\n>\n> I read 1.12 um x 1.12 um\n>\n> > + .testPatternModes = {\n> > + { controls::draft::TestPatternModeOff, 0 },\n> > + {\n> controls::draft::TestPatternModeColorBars, 1 },\n> > + /*\n> > + * No best corresponding test pattern for:\n> > + * 1: \"Vertical Color Bar Type 1\",\n> > + * 2: \"Vertical Color Bar Type 2\",\n> > + * 3: \"Vertical Color Bar Type 3\",\n> > + * 4: \"Vertical Color Bar Type 4\"\n> > + */\n> > + },\n> > + } },\n> > { \"ov8865\", {\n> > .unitCellSize = { 1400, 1400 },\n> > .testPatternModes = {\n> > --\n> > 2.34.1\n> >\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 55E75BD16B\n\tfor ;\n\tThu, 27 Oct 2022 17:25:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A986C62FA0;\n\tThu, 27 Oct 2022 19:25:34 +0200 (CEST)","from mail-il1-x130.google.com (mail-il1-x130.google.com\n\t[IPv6:2607:f8b0:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A99A662F89\n\tfor ;\n\tThu, 27 Oct 2022 19:25:32 +0200 (CEST)","by mail-il1-x130.google.com with SMTP id o13so1429614ilc.7\n\tfor ;\n\tThu, 27 Oct 2022 10:25:32 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666891534;\n\tbh=tGIvKXtffRMRjKHG3gQoVKP4RMchermiYsaIwpP0fkE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=JkMawZsJXuPAXRNdvl7KNJUepRl0MFgYFjNIBuR3UaGtPAXzq+vm6CEhNtSbMko0b\n\tzq1Djp/Ajei33hg20s0UlpEwULA6uEJl3dwU8V/Fk9V4jjV5spV9EllR88tzgVmtkP\n\thTbl2bkOFLUsZRzAAckXsV5qRqkQKDlusHEM6fAHpIXKhsYLvTSGvz0eJ5W8Tq5Fc8\n\txDI4DsGuO+v6Apwh8itQRWZ785RpYTegolOG4gv+UMfpEK+WQv+XhG8r5IZPdc/PPh\n\ta781WAYIHOd3SGaz1iL1yPTHBsNEcyKcnng+TcQ/pMZewkjpdSbn+vJjn+qFFQgm/t\n\tpkf8Qu4woCIjg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=rothemail-net.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=3yh/4KSaepJsqennHLka2U61XiP7Doi/recZRGhlKE8=;\n\tb=loSPbt9PkdAIm9NvG09V8ffukK3HgjyDW41W74ugC816EbhibDAh0uVnRq3+f2+Hut\n\t1X6IXK6H22V1Xa5VBfb9uQr6PHY07LH5oTRlSelfidRpJsjc5hixA3lPnVlSppwulvG4\n\t6cpkSPqBUXtHhRbUGnNVEiNAm9gkl9GmKfHfPCBVc8cpDfALxi6DmGnf2skrzT6i/i/g\n\thXuivrlxIsQjs7m+o1a7kdeC2BY4u+GTsMZah9526U4zwTlVwsK8MleTX/qdE9Fyf0mk\n\treKUWNXWwVKJMFW2E1Knzc321WRPfnGqNP9minTWK8bARF+9Ra6nmCVNI+NZzI3UL0wK\n\t7i7A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=rothemail-net.20210112.gappssmtp.com\n\theader.i=@rothemail-net.20210112.gappssmtp.com header.b=\"loSPbt9P\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=3yh/4KSaepJsqennHLka2U61XiP7Doi/recZRGhlKE8=;\n\tb=C54Zny8kMJyGiP5HskjE8ux/bh2jqGHvlL6vwzNduuIcnCkUshxdKww0TZYAuKc8qU\n\tdxaSdPzNBiMCkrMhjS9oN8cUbZjrbED1wbVEo/UAL/To7s9ZFeDDkOwL9ctqlA6m7LFc\n\tdCY2C0i6HinLqzr9LeAqOYT59h7rSBlWmtjc8X4j8xmSJya2i+XQTUPt2DoCil09vXbq\n\tcCg5eAXXupnMBQItNG7oxk7NoutRUxXRXreHUbjma+cfXn/jpJQ92DwmfU8XBa3sZ5xw\n\tIGRZpaDgGlsrhGGJsQyS+iEM1Ay74TzMMksgluk5d08AGl4/zSurSVCNId8OzuIrv4ut\n\tGYIg==","X-Gm-Message-State":"ACrzQf1toX42Bv/6uXOCjSenfMNuKbggWKAZA0brrtanoZf4uTLG6C2l\n\tQn/NO5Cz7wf57pRYhjX0DbowBt66GmLyC+HXcpv8yY1++wLpTnWW","X-Google-Smtp-Source":"AMsMyM4IxsiJbASShFDk+BCjqKnUXUjBhxy7mScpcv4i1gyu9KviknG544zJUuADjYrVS5gp5edV5lN4eUJngYeD75E=","X-Received":"by 2002:a05:6e02:1c42:b0:300:56a5:adf6 with SMTP id\n\td2-20020a056e021c4200b0030056a5adf6mr7612786ilg.53.1666891531317;\n\tThu, 27 Oct 2022 10:25:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>","In-Reply-To":"<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>","Date":"Thu, 27 Oct 2022 12:25:20 -0500","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"0000000000009a8c9f05ec076d11\"","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Nicholas Roth via libcamera-devel ","Reply-To":"Nicholas Roth ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25634,"web_url":"https://patchwork.libcamera.org/comment/25634/","msgid":"","date":"2022-10-27T22:21:37","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":137,"url":"https://patchwork.libcamera.org/api/people/137/","name":"Nicholas Roth","email":"nicholas@rothemail.net"},"content":"> Nice this matches the CCS defined linear gain model.\n> However the sensor allows to select two formats for the analogue gain\n> the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> register 0x3503[2] and it would be nice to validate that the driver\n> uses the format that you expect.\n\nI think I understand this better after reading the driver. In the register\nvalue settings, I see:\n {0x3502, 0x40}, // exposure L\n{0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain\n{0x3505, 0x80}, // gain option\n\nThe 0 low-nibbl indicates we're using \"real gain.\"\n\nAccording to the datasheet:\n0x3503[2]=0, gain[7:0] is real gain format,\nwhere low 4 bits are fraction bits, for\nexample, 0x10 is 1x gain, 0x28 is 2.5x gain\n\nThis tells me that we need to divide reported gain from the driver by 16 to\nget a properly-scaled double-value gain, which these gain constants do.\n\n> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> requires it to be expressed as a number of image lines. Camera sensor\ndrivers\n> that do not comply with this requirement will need to be adapted or will\nproduce\n> incorrect results.\n\nLooks like the driver multiples inputs x16 to convert to 1/16 line length\nbefore writing to the register...\n\n case V4L2_CID_EXPOSURE:\n/* 4 least significant bits of expsoure are fractional part */\nret = ov8858_write_reg(ov8858->client,\nOV8858_REG_EXPOSURE,\nOV8858_REG_VALUE_24BIT,\nctrl->val << 4);\n\n\nLet me know if this looks good, and what needs to happen to merge this!\n\nThanks,\n-Nicholas\n\nOn Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth \nwrote:\n\n> > please pardon me for being pedantic, but I would:\n> > libcamera: Add support for ov8858 sensor\n> >\n> > instead of:\n> > ipa: libcamera: add metadata for the ov8858 sensor\n>\n> Happy to make the change.\n>\n> > The sensor doesn't seem to be supported mainline :(...\n> > Let's start from the first point: where does this driver lives ? What\n> > kernel are you using ?\n> I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n>\n> It looks like my package manager is picking up the Manjaro kernel from\n> https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,\n> which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no\n> 8858-related changes. This ultimately pulls the driver from\n> https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> .\n>\n> Looks like this has been in and out of Torvalds' peripheral view, e.g.:\n> https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n>\n> If you think that it would be valuable to try and mainline this, I'd be\n> interested to do so once I can get libcamera working on my hardware, though\n> I'll likely need some guidance in the process, since I'll be learning a lot.\n>\n> > Nice this matches the CCS defined linear gain model.\n> > However the sensor allows to select two formats for the analogue gain\n> > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > register 0x3503[2] and it would be nice to validate that the driver\n> > uses the format that you expect.\n> Frankly, I'm not sure what you mean by this, but I'll try to find out from\n> the driver above and verify.\n>\n> > Knowing what driver you're using is relevant, in example, as the\n> > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > what libcamera expects, and sensor drivers are expected to express the\n> > V4L2_CID_EXPOSURE control in line units.\n>\n> > From Documentation/sensor_driver_requirements.rst\n>\n> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > requires it to be expressed as a number of image lines. Camera sensor\n> drivers\n> > that do not comply with this requirement will need to be adapted or will\n> produce\n> > incorrect results.\n>\n> Let me read the driver and get back to you.\n>\n>\n> On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi wrote:\n>\n>> Hi Nicholas,\n>>\n>> please pardon me for being pedantic, but I would:\n>> libcamera: Add support for ov8858 sensor\n>>\n>> instead of:\n>> ipa: libcamera: add metadata for the ov8858 sensor\n>>\n>> \"metadata\" is an overloaded already term and usually refers to the\n>> properties associated to a captured frame (the term comes from the\n>> Android lingo, but we use it too).\n>>\n>> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n>> libcamera-devel wrote:\n>> > From: Nicholas Roth \n>> >\n>> > Currently, libcamera does not have information for the ov8858 sensor\n>> > used in the PinePhone Pro, a phone designed to run Linux.\n>> >\n>> > This commit adds metadata, especially that sensor gain is reported and\n>> > set in 1/16 discrete increments.\n>> >\n>> > For more information, see \"5.8 manual exposure compensation/ manual\n>> > gain compensation\" in [0].\n>> >\n>> > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n>>\n>> The sensor doesn't seem to be supported mainline :(\n>> libcamera has a policy that requires supported components to be\n>> upstream or at least on their way to upstream (ie posted to the\n>> linux-media mailing list). The policy is there because different\n>> downstream driver implementations might behave differently one from\n>> the other, making it impossible for libcamera to support the part\n>> fully. The policy is a strict requirement for ISPs, I guess we're a\n>> bit more elastic when it comes to sensor. Howver knowing what driver\n>> you are using, where it lives, and if there's any plan to upstream it\n>> would be great.\n>>\n>> Let's start from the first point: where does this driver lives ? What\n>> kernel are you using ?\n>>\n>> Knowing what driver you're using is relevant, in example, as the\n>> OV5688 sensor computes exposure in 1/16 of line length. This is not\n>> what libcamera expects, and sensor drivers are expected to express the\n>> V4L2_CID_EXPOSURE control in line units.\n>>\n>> From Documentation/sensor_driver_requirements.rst\n>>\n>> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n>> requires it to be expressed as a number of image lines. Camera sensor\n>> drivers\n>> that do not comply with this requirement will need to be adapted or will\n>> produce\n>> incorrect results.\n>>\n>> >\n>> > Signed-off-by: Nicholas Roth \n>> > ---\n>> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n>> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n>> > 2 files changed, 25 insertions(+)\n>> >\n>> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n>> b/src/ipa/libipa/camera_sensor_helper.cpp\n>> > index 35056bec..f2040cbd 100644\n>> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> > @@ -476,6 +476,17 @@ public:\n>> > };\n>> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>> >\n>> > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n>> > +{\n>> > +public:\n>> > + CameraSensorHelperOv8858()\n>> > + {\n>> > + gainType_ = AnalogueGainLinear;\n>> > + gainConstants_.linear = { 1, 0, 0, 16 };\n>> > + }\n>> > +};\n>>\n>> Nice this matches the CCS defined linear gain model.\n>> However the sensor allows to select two formats for the analogue gain\n>> the \"real gain\" format and \"sensor gain\" format. The selection is made by\n>> register 0x3503[2] and it would be nice to validate that the driver\n>> uses the format that you expect.\n>>\n>> > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n>>\n>> Ah! that's a weird name for the sensor entity :)\n>>\n>> > +\n>> > class CameraSensorHelperOv8865 : public CameraSensorHelper\n>> > {\n>> > public:\n>> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n>> b/src/libcamera/camera_sensor_properties.cpp\n>> > index e5f27f06..d0757c15 100644\n>> > --- a/src/libcamera/camera_sensor_properties.cpp\n>> > +++ b/src/libcamera/camera_sensor_properties.cpp\n>> > @@ -146,6 +146,20 @@ const CameraSensorProperties\n>> *CameraSensorProperties::get(const std::string &sen\n>> > */\n>> > },\n>> > } },\n>> > + { \"m00_f_ov8858\", {\n>> > + .unitCellSize = { 1200, 1200 },\n>>\n>> I read 1.12 um x 1.12 um\n>>\n>> > + .testPatternModes = {\n>> > + { controls::draft::TestPatternModeOff, 0\n>> },\n>> > + {\n>> controls::draft::TestPatternModeColorBars, 1 },\n>> > + /*\n>> > + * No best corresponding test pattern for:\n>> > + * 1: \"Vertical Color Bar Type 1\",\n>> > + * 2: \"Vertical Color Bar Type 2\",\n>> > + * 3: \"Vertical Color Bar Type 3\",\n>> > + * 4: \"Vertical Color Bar Type 4\"\n>> > + */\n>> > + },\n>> > + } },\n>> > { \"ov8865\", {\n>> > .unitCellSize = { 1400, 1400 },\n>> > .testPatternModes = {\n>> > --\n>> > 2.34.1\n>> >\n>>\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 ED982BDB16\n\tfor ;\n\tThu, 27 Oct 2022 22:21:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C48062FA3;\n\tFri, 28 Oct 2022 00:21:50 +0200 (CEST)","from mail-io1-xd36.google.com (mail-io1-xd36.google.com\n\t[IPv6:2607:f8b0:4864:20::d36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C5BF61F4B\n\tfor ;\n\tFri, 28 Oct 2022 00:21:49 +0200 (CEST)","by mail-io1-xd36.google.com with SMTP id p141so3075277iod.6\n\tfor ;\n\tThu, 27 Oct 2022 15:21:49 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666909310;\n\tbh=rDIJY93/H3iL+Mkb0BRKqU2PN3gqcvImS4eCSjD+OUM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xbl6APv05WcEXptADjy7jO/l1qFlmUPqQ307R5qpIxXCPpgg2I7Q2fe0kOgUV9RRE\n\tV+VBGfpzZgj4JrsVzjrKmmjgsLssgdKjFnzz+iK26OJ8sbOrf/st5dQGFt8MGUu6OB\n\tLpN2dCzsdFA5Y8ohNZy/5kwOTjmqNCNh7U58yMaIMcWf69lw0rE3a6rz6dqOuUmJMI\n\t+FLJddh6IUcWGeVvQYLT0QD1tK74ATA/uJGTm0r5bfUiJYgySl2lCR6OJ8xrOVSn5n\n\tS6UWENVfNQRgoMO1f9KAgqUQhYVLnglMagLpMpuaMY3XtpZtPc2wsB0msg4v2EfqdG\n\t4aCj0c2C1UAEA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=rothemail-net.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Hm/0cHZqyYNUiCdq9tbDFUJrZ1JHkFD1zqvAcQzEy6Q=;\n\tb=A1F/kF43Mz1q1Nv2oRVawTp/o6GmRJXwGzeQDZ48tBLVPIMM5gvBwMOJ3GdKJ7wXgw\n\tDK4fncWTL5OiiwEXv5tOXtRtAkuNxaQJrnGpKBbtunYWPN4744+C77zYpKSReRw/wDje\n\t7cyBoZ6qEjv1rKwxcmYTfa7SqfFJPvpHx6JZmPhLy7dEXtYQz1R8JNUKsT7a33FWknfq\n\tTGAjl354Muq2YOxc7esecmbXFw/MPn0QsPYS/8jsQbpbdtaf8FL2lSATGT5gaQFReisT\n\tuSG/fBDa/rSmFQZAdynieCAYFVX5tFHt+eBRok1CfXvoNwfpfc4Prg0Gm5YnQ2xAu3Ox\n\tO2UQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=rothemail-net.20210112.gappssmtp.com\n\theader.i=@rothemail-net.20210112.gappssmtp.com header.b=\"A1F/kF43\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Hm/0cHZqyYNUiCdq9tbDFUJrZ1JHkFD1zqvAcQzEy6Q=;\n\tb=YfDFdxUQZ60XWSy8/lTYhtuizEw5RxEHL6L/lDopKD+S6XcGAI+qvYYZm5x7e6ezdU\n\tZpcD8vRZtjaNja4AS1D1jiMHvUUa5bzAXwOFjcgHJS9mg7ZNAfUkG3i/uO1+rn3TgoVT\n\tLPIbbIJ354I68FjfzMs8hLqaiZk/bgqQAFdXhAxYKcvmCcDHJ7DDy9xCPpLce/g2rPkp\n\t3iAS59GXPpKEZuxWhnXOCiszDMfHhM3DGl+7mrCNmV6VQ/6PNK3TxHab8zLFLjdbXBgV\n\tivB2D5X50k6Dnula2nCt1PWzAqYD+XoN2ibD9tZVkF8mrilqJhMcJQkr1J+DuqwQfCyz\n\tmeFQ==","X-Gm-Message-State":"ACrzQf0wr5UrvgOd/aAQBuhY2sK7eG3YU1XN7yU8Slm/VXHU4afRZqVV\n\tM0X7jY/cuWP4722Wjf3p4MBmAe5EKU5HUCkNPVJa32He8jUpxahs","X-Google-Smtp-Source":"AMsMyM75D5Ntg0liFzEF4rB4YNasEGVZLb+bZJxaYvgET7AkjI3VAxl5boIlZoZdfdWJE9vZkjB4TvQZ0ktPAUh/L4g=","X-Received":"by 2002:a6b:c3cd:0:b0:6a8:3ca0:dafa with SMTP id\n\tt196-20020a6bc3cd000000b006a83ca0dafamr29804811iof.193.1666909307981;\n\tThu, 27 Oct 2022 15:21:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t","In-Reply-To":"","Date":"Thu, 27 Oct 2022 17:21:37 -0500","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"0000000000002cebe705ec0b9131\"","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Nicholas Roth via libcamera-devel ","Reply-To":"Nicholas Roth ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25637,"web_url":"https://patchwork.libcamera.org/comment/25637/","msgid":"<20221028075728.qhpio2gu6zp3aqsh@uno.localdomain>","date":"2022-10-28T07:57:28","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicholas\n\nOn Thu, Oct 27, 2022 at 12:25:20PM -0500, Nicholas Roth wrote:\n> > please pardon me for being pedantic, but I would:\n> > libcamera: Add support for ov8858 sensor\n> >\n> > instead of:\n> > ipa: libcamera: add metadata for the ov8858 sensor\n>\n> Happy to make the change.\n>\n> > The sensor doesn't seem to be supported mainline :(...\n> > Let's start from the first point: where does this driver lives ? What\n> > kernel are you using ?\n> I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n>\n> It looks like my package manager is picking up the Manjaro kernel from\n> https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,\n> which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no 8858-related\n> changes. This ultimately pulls the driver from\n> https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> .\n\nRight! So indeed that's downstream stuff, probably from Rockchip's\nBSP.\n\nThe driver is not -that- bad, there's a little vendor's code crud here\nand there but it looks like a decent candidate for upstreaming.\n\nI would be happy to help if you're willing to try. I also own a pinephone\npro, which so far I haven't had enough time to play with\n\n>\n> Looks like this has been in and out of Torvalds' peripheral view, e.g.:\n> https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n\nThe driver mentioned lives in:\n drivers/staging/media/atomisp/i2c/\n\nThe AtomISP support was upstreamed with its own set of sensor drivers,\nwhich were probably not compliant with the mainline V4L2 kAPI and used\nsome vendor defined abstractions. That's why they used to live in staging.\nHowever they might serve as reference is something goes wrong, and\nsometimes they're even copies of regular drivers/media/i2c/ drivers\nadapted to work with the custom abstractions. AtomISP has also been\nremoved from upstream a few years ago. It's being re-upstreamed in\nthese days by Hans de Goede, hopefully without custom sensor drivers.\n\nTL;DR do not consider drivers/staging/media/atomisp/i2c drivers as\ncandidates for upstreaming, even if they might sometime provide\nreferences to hw quirks and configurations.\n\n>\n> If you think that it would be valuable to try and mainline this, I'd be\n> interested to do so once I can get libcamera working on my hardware, though\n> I'll likely need some guidance in the process, since I'll be learning a lot.\n\nHappy to help if you want to give it a go!\n\n>\n> > Nice this matches the CCS defined linear gain model.\n> > However the sensor allows to select two formats for the analogue gain\n> > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > register 0x3503[2] and it would be nice to validate that the driver\n> > uses the format that you expect.\n> Frankly, I'm not sure what you mean by this, but I'll try to find out from\n> the driver above and verify.\n>\n\nSorry for having given a few things for granted.\n\nCCS is the MIPI Alliance specification for a \"Camera Command Set\", an\nattempt to define a standard for image sensors register interfaces.\n\nAs part of the register interface definition, CCS specifies how sensor\ndrivers expose their configuration parameters including the analogue\ngain model.\n\nThe specs are publicly available\nhttps://www.mipi.org/specifications/camera-command-set\n\nand you'll find in section 9.3 how the Analogue Global Gain control is\nimplemented.\n\nWhen it comes to this specific sensor, the datasheet documents\nregister 0x3503[2] as\n\n 0: Input gain as real gain format\n 1: Input gain as sensor gain format\n\nThe \"real gain\" vs \"sensor gain\" modes are described in the\ndocumentation of register 0x3509\n\n 0x3503[2]=0, gain[7:0] is real gain format,\n where low 4 bits are fraction bits, for\n example, 0x10 is 1x gain, 0x28 is 2.5x gain\n\n If 0x3503[2]=1, gain[7:0] is sensor gain\n format, gain[7:4] is coarse gain, 00000: 1x,\n 00001: 2x, 00011: 4x, 00111: 8x, gain[7] is\n 1, gain[3:0] is fine gain. For example, 0x10\n is 1x gain, 0x30 is 2x gain, 0x70 is 4x gain\n\nSo what you want here is to have the sensor operate in \"real gain\"\nmode, where 0x01 == 1/16 increment in gain\n\n> > Knowing what driver you're using is relevant, in example, as the\n> > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > what libcamera expects, and sensor drivers are expected to express the\n> > V4L2_CID_EXPOSURE control in line units.\n>\n> > From Documentation/sensor_driver_requirements.rst\n>\n> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > requires it to be expressed as a number of image lines. Camera sensor\n> drivers\n> > that do not comply with this requirement will need to be adapted or will\n> produce\n> > incorrect results.\n>\n> Let me read the driver and get back to you.\n>\n>\n> On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi wrote:\n>\n> > Hi Nicholas,\n> >\n> > please pardon me for being pedantic, but I would:\n> > libcamera: Add support for ov8858 sensor\n> >\n> > instead of:\n> > ipa: libcamera: add metadata for the ov8858 sensor\n> >\n> > \"metadata\" is an overloaded already term and usually refers to the\n> > properties associated to a captured frame (the term comes from the\n> > Android lingo, but we use it too).\n> >\n> > On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> > libcamera-devel wrote:\n> > > From: Nicholas Roth \n> > >\n> > > Currently, libcamera does not have information for the ov8858 sensor\n> > > used in the PinePhone Pro, a phone designed to run Linux.\n> > >\n> > > This commit adds metadata, especially that sensor gain is reported and\n> > > set in 1/16 discrete increments.\n> > >\n> > > For more information, see \"5.8 manual exposure compensation/ manual\n> > > gain compensation\" in [0].\n> > >\n> > > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n> >\n> > The sensor doesn't seem to be supported mainline :(\n> > libcamera has a policy that requires supported components to be\n> > upstream or at least on their way to upstream (ie posted to the\n> > linux-media mailing list). The policy is there because different\n> > downstream driver implementations might behave differently one from\n> > the other, making it impossible for libcamera to support the part\n> > fully. The policy is a strict requirement for ISPs, I guess we're a\n> > bit more elastic when it comes to sensor. Howver knowing what driver\n> > you are using, where it lives, and if there's any plan to upstream it\n> > would be great.\n> >\n> > Let's start from the first point: where does this driver lives ? What\n> > kernel are you using ?\n> >\n> > Knowing what driver you're using is relevant, in example, as the\n> > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > what libcamera expects, and sensor drivers are expected to express the\n> > V4L2_CID_EXPOSURE control in line units.\n> >\n> > From Documentation/sensor_driver_requirements.rst\n> >\n> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > requires it to be expressed as a number of image lines. Camera sensor\n> > drivers\n> > that do not comply with this requirement will need to be adapted or will\n> > produce\n> > incorrect results.\n> >\n> > >\n> > > Signed-off-by: Nicholas Roth \n> > > ---\n> > > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> > > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> > > 2 files changed, 25 insertions(+)\n> > >\n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n> > b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > index 35056bec..f2040cbd 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > @@ -476,6 +476,17 @@ public:\n> > > };\n> > > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > >\n> > > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> > > +{\n> > > +public:\n> > > + CameraSensorHelperOv8858()\n> > > + {\n> > > + gainType_ = AnalogueGainLinear;\n> > > + gainConstants_.linear = { 1, 0, 0, 16 };\n> > > + }\n> > > +};\n> >\n> > Nice this matches the CCS defined linear gain model.\n> > However the sensor allows to select two formats for the analogue gain\n> > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > register 0x3503[2] and it would be nice to validate that the driver\n> > uses the format that you expect.\n> >\n> > > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n> >\n> > Ah! that's a weird name for the sensor entity :)\n> >\n> > > +\n> > > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > > {\n> > > public:\n> > > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> > b/src/libcamera/camera_sensor_properties.cpp\n> > > index e5f27f06..d0757c15 100644\n> > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > @@ -146,6 +146,20 @@ const CameraSensorProperties\n> > *CameraSensorProperties::get(const std::string &sen\n> > > */\n> > > },\n> > > } },\n> > > + { \"m00_f_ov8858\", {\n> > > + .unitCellSize = { 1200, 1200 },\n> >\n> > I read 1.12 um x 1.12 um\n> >\n> > > + .testPatternModes = {\n> > > + { controls::draft::TestPatternModeOff, 0 },\n> > > + {\n> > controls::draft::TestPatternModeColorBars, 1 },\n> > > + /*\n> > > + * No best corresponding test pattern for:\n> > > + * 1: \"Vertical Color Bar Type 1\",\n> > > + * 2: \"Vertical Color Bar Type 2\",\n> > > + * 3: \"Vertical Color Bar Type 3\",\n> > > + * 4: \"Vertical Color Bar Type 4\"\n> > > + */\n> > > + },\n> > > + } },\n> > > { \"ov8865\", {\n> > > .unitCellSize = { 1400, 1400 },\n> > > .testPatternModes = {\n> > > --\n> > > 2.34.1\n> > >\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 8E836BD16B\n\tfor ;\n\tFri, 28 Oct 2022 07:57:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7105462FB5;\n\tFri, 28 Oct 2022 09:57:33 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B43162F41\n\tfor ;\n\tFri, 28 Oct 2022 09:57:31 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id B7DECFF802;\n\tFri, 28 Oct 2022 07:57:30 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666943853;\n\tbh=UMqo/5zthRn4SWgAJO+D/g/WbhAq0jNNWiWm2H5F0mY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YILjJf9y+h50EScNDUBH+vqmBqVp9y8gmYBz2brKJY1I1ws3U0LcOnqht0u0kocVe\n\ts/tAPKrvzW9CaB8QFtgucjDeElt3r5hKapdIhFy5SSByaXOeUQWG7dOWHPGbaGGzFp\n\tz6AyLGpf6aZQVgtFuLvpmFS8HtX32fjravwujX3qvi6XlIvckpdOv0PO1bOwXrLLce\n\tPai4btme9+Y+k4tVgnu9+NHOQd98Bn91/B+NH1lJxvtdrxBSwZXVpbPgz70IMw3s3F\n\tRfq51imFVQBORqHBR9ciN+VJJP6fj0JX1z6unMEGB5NEYKCuWZg2lz5Rb4ISHFexVN\n\txwMdgccCCw5WQ==","Date":"Fri, 28 Oct 2022 09:57:28 +0200","To":"Nicholas Roth ","Message-ID":"<20221028075728.qhpio2gu6zp3aqsh@uno.localdomain>","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25638,"web_url":"https://patchwork.libcamera.org/comment/25638/","msgid":"<20221028081427.s25wdjmmqh5ikjem@uno.localdomain>","date":"2022-10-28T08:14:27","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicholas,\n + Laurent question below\n\nOn Thu, Oct 27, 2022 at 05:21:37PM -0500, Nicholas Roth wrote:\n> > Nice this matches the CCS defined linear gain model.\n> > However the sensor allows to select two formats for the analogue gain\n> > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > register 0x3503[2] and it would be nice to validate that the driver\n> > uses the format that you expect.\n>\n> I think I understand this better after reading the driver. In the register\n> value settings, I see:\n> {0x3502, 0x40}, // exposure L\n> {0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain\n> {0x3505, 0x80}, // gain option\n>\n> The 0 low-nibbl indicates we're using \"real gain.\"\n>\n> According to the datasheet:\n> 0x3503[2]=0, gain[7:0] is real gain format,\n> where low 4 bits are fraction bits, for\n> example, 0x10 is 1x gain, 0x28 is 2.5x gain\n>\n> This tells me that we need to divide reported gain from the driver by 16 to\n> get a properly-scaled double-value gain, which these gain constants do.\n>\n\nHere you go!\n\nLooking at the driver you're using I indeed see 3503[2]=0 in all modes\n\n> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > requires it to be expressed as a number of image lines. Camera sensor\n> drivers\n> > that do not comply with this requirement will need to be adapted or will\n> produce\n> > incorrect results.\n>\n> Looks like the driver multiples inputs x16 to convert to 1/16 line length\n> before writing to the register...\n>\n> case V4L2_CID_EXPOSURE:\n> /* 4 least significant bits of expsoure are fractional part */\n> ret = ov8858_write_reg(ov8858->client,\n> OV8858_REG_EXPOSURE,\n> OV8858_REG_VALUE_24BIT,\n> ctrl->val << 4);\n>\n\nGreat, it means that towards userspace the exposure is indeed in units\nof 1 line.\n\n>\n> Let me know if this looks good, and what needs to happen to merge this!\n\nI think I had a comment on the pixel cell size.\n\nApart from that, let me check with Laurent about the policy when it\ncomes to supporting sensor without an upstream driver.\n\nI would not be super happy of adding support to \"m00_f_ov8858\" as if the\ndriver will be upstreamed the entity name should at least change.\n\nAs a middle term solution we can add support for \"m00_f_ov8858\" with a\ntodo note to move to a more canonical \"ov8858\" name once the driver is\nupstreamed, or at least submitted upstream.\n\nThanks\n j\n\n>\n> Thanks,\n> -Nicholas\n>\n> On Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth \n> wrote:\n>\n> > > please pardon me for being pedantic, but I would:\n> > > libcamera: Add support for ov8858 sensor\n> > >\n> > > instead of:\n> > > ipa: libcamera: add metadata for the ov8858 sensor\n> >\n> > Happy to make the change.\n> >\n> > > The sensor doesn't seem to be supported mainline :(...\n> > > Let's start from the first point: where does this driver lives ? What\n> > > kernel are you using ?\n> > I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n> >\n> > It looks like my package manager is picking up the Manjaro kernel from\n> > https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,\n> > which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no\n> > 8858-related changes. This ultimately pulls the driver from\n> > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> > .\n> >\n> > Looks like this has been in and out of Torvalds' peripheral view, e.g.:\n> > https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n> >\n> > If you think that it would be valuable to try and mainline this, I'd be\n> > interested to do so once I can get libcamera working on my hardware, though\n> > I'll likely need some guidance in the process, since I'll be learning a lot.\n> >\n> > > Nice this matches the CCS defined linear gain model.\n> > > However the sensor allows to select two formats for the analogue gain\n> > > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > > register 0x3503[2] and it would be nice to validate that the driver\n> > > uses the format that you expect.\n> > Frankly, I'm not sure what you mean by this, but I'll try to find out from\n> > the driver above and verify.\n> >\n> > > Knowing what driver you're using is relevant, in example, as the\n> > > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > > what libcamera expects, and sensor drivers are expected to express the\n> > > V4L2_CID_EXPOSURE control in line units.\n> >\n> > > From Documentation/sensor_driver_requirements.rst\n> >\n> > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > > requires it to be expressed as a number of image lines. Camera sensor\n> > drivers\n> > > that do not comply with this requirement will need to be adapted or will\n> > produce\n> > > incorrect results.\n> >\n> > Let me read the driver and get back to you.\n> >\n> >\n> > On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi wrote:\n> >\n> >> Hi Nicholas,\n> >>\n> >> please pardon me for being pedantic, but I would:\n> >> libcamera: Add support for ov8858 sensor\n> >>\n> >> instead of:\n> >> ipa: libcamera: add metadata for the ov8858 sensor\n> >>\n> >> \"metadata\" is an overloaded already term and usually refers to the\n> >> properties associated to a captured frame (the term comes from the\n> >> Android lingo, but we use it too).\n> >>\n> >> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> >> libcamera-devel wrote:\n> >> > From: Nicholas Roth \n> >> >\n> >> > Currently, libcamera does not have information for the ov8858 sensor\n> >> > used in the PinePhone Pro, a phone designed to run Linux.\n> >> >\n> >> > This commit adds metadata, especially that sensor gain is reported and\n> >> > set in 1/16 discrete increments.\n> >> >\n> >> > For more information, see \"5.8 manual exposure compensation/ manual\n> >> > gain compensation\" in [0].\n> >> >\n> >> > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n> >>\n> >> The sensor doesn't seem to be supported mainline :(\n> >> libcamera has a policy that requires supported components to be\n> >> upstream or at least on their way to upstream (ie posted to the\n> >> linux-media mailing list). The policy is there because different\n> >> downstream driver implementations might behave differently one from\n> >> the other, making it impossible for libcamera to support the part\n> >> fully. The policy is a strict requirement for ISPs, I guess we're a\n> >> bit more elastic when it comes to sensor. Howver knowing what driver\n> >> you are using, where it lives, and if there's any plan to upstream it\n> >> would be great.\n> >>\n> >> Let's start from the first point: where does this driver lives ? What\n> >> kernel are you using ?\n> >>\n> >> Knowing what driver you're using is relevant, in example, as the\n> >> OV5688 sensor computes exposure in 1/16 of line length. This is not\n> >> what libcamera expects, and sensor drivers are expected to express the\n> >> V4L2_CID_EXPOSURE control in line units.\n> >>\n> >> From Documentation/sensor_driver_requirements.rst\n> >>\n> >> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> >> requires it to be expressed as a number of image lines. Camera sensor\n> >> drivers\n> >> that do not comply with this requirement will need to be adapted or will\n> >> produce\n> >> incorrect results.\n> >>\n> >> >\n> >> > Signed-off-by: Nicholas Roth \n> >> > ---\n> >> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> >> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> >> > 2 files changed, 25 insertions(+)\n> >> >\n> >> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n> >> b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> > index 35056bec..f2040cbd 100644\n> >> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> >> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> > @@ -476,6 +476,17 @@ public:\n> >> > };\n> >> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> >> >\n> >> > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> >> > +{\n> >> > +public:\n> >> > + CameraSensorHelperOv8858()\n> >> > + {\n> >> > + gainType_ = AnalogueGainLinear;\n> >> > + gainConstants_.linear = { 1, 0, 0, 16 };\n> >> > + }\n> >> > +};\n> >>\n> >> Nice this matches the CCS defined linear gain model.\n> >> However the sensor allows to select two formats for the analogue gain\n> >> the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> >> register 0x3503[2] and it would be nice to validate that the driver\n> >> uses the format that you expect.\n> >>\n> >> > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n> >>\n> >> Ah! that's a weird name for the sensor entity :)\n> >>\n> >> > +\n> >> > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> >> > {\n> >> > public:\n> >> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> >> b/src/libcamera/camera_sensor_properties.cpp\n> >> > index e5f27f06..d0757c15 100644\n> >> > --- a/src/libcamera/camera_sensor_properties.cpp\n> >> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> >> > @@ -146,6 +146,20 @@ const CameraSensorProperties\n> >> *CameraSensorProperties::get(const std::string &sen\n> >> > */\n> >> > },\n> >> > } },\n> >> > + { \"m00_f_ov8858\", {\n> >> > + .unitCellSize = { 1200, 1200 },\n> >>\n> >> I read 1.12 um x 1.12 um\n> >>\n> >> > + .testPatternModes = {\n> >> > + { controls::draft::TestPatternModeOff, 0\n> >> },\n> >> > + {\n> >> controls::draft::TestPatternModeColorBars, 1 },\n> >> > + /*\n> >> > + * No best corresponding test pattern for:\n> >> > + * 1: \"Vertical Color Bar Type 1\",\n> >> > + * 2: \"Vertical Color Bar Type 2\",\n> >> > + * 3: \"Vertical Color Bar Type 3\",\n> >> > + * 4: \"Vertical Color Bar Type 4\"\n> >> > + */\n> >> > + },\n> >> > + } },\n> >> > { \"ov8865\", {\n> >> > .unitCellSize = { 1400, 1400 },\n> >> > .testPatternModes = {\n> >> > --\n> >> > 2.34.1\n> >> >\n> >>\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 41FBCBDB16\n\tfor ;\n\tFri, 28 Oct 2022 08:14:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A889862F41;\n\tFri, 28 Oct 2022 10:14:31 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A4C7762F41\n\tfor ;\n\tFri, 28 Oct 2022 10:14:29 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id DB6BF40013;\n\tFri, 28 Oct 2022 08:14:28 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666944871;\n\tbh=xYhLS44Lohp9o80thnWeyTVyBS5LzZYX94592RSON78=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lerYCAUmzEtmvg6TPlov3UhITjatVAraceRrFZtvgRdslinH6zTOYfZVYh1ZQD3Mw\n\tEpwGnunJJiX51xgbym2T1inzWxIIDniEz3nBrqbQoQ3DCqxRLGDI0m7Ps24XDz6E3S\n\tKTEihn3AZAPIurlBbcizIG1e3BTNjC5PPkHifyZ8hOtattKZ6qNMTP/go81k51uFby\n\t8gmlboLbQDGdvYM41HR0zcX98jaVqQFqBDI5Z1/LoIXqDwGpt0WwgYSFUEIjQOaWZY\n\t7+NkM/4pxeVSPlf0pdiGcSadeczi2ayIn8shiig4nVsXfPfV5NgHxexcviuBf09CUS\n\tc+2QmCl2uY1VA==","Date":"Fri, 28 Oct 2022 10:14:27 +0200","To":"Nicholas Roth ,\n\tLaurent Pinchart ","Message-ID":"<20221028081427.s25wdjmmqh5ikjem@uno.localdomain>","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25641,"web_url":"https://patchwork.libcamera.org/comment/25641/","msgid":"<166694624777.1769154.10301809954931640270@Monstersaurus>","date":"2022-10-28T08:37:27","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Nicholas Roth via libcamera-devel (2022-10-27 18:25:20)\n> > please pardon me for being pedantic, but I would:\n> > libcamera: Add support for ov8858 sensor\n> >\n> > instead of:\n> > ipa: libcamera: add metadata for the ov8858 sensor\n> \n> Happy to make the change.\n> \n> > The sensor doesn't seem to be supported mainline :(...\n> > Let's start from the first point: where does this driver lives ? What\n> > kernel are you using ?\n> I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n> \n> It looks like my package manager is picking up the Manjaro kernel from\n> https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,\n> which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no 8858-related\n> changes. This ultimately pulls the driver from\n> https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> .\n> \n> Looks like this has been in and out of Torvalds' peripheral view, e.g.:\n> https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n\nThat driver is in fact separate and not what you are using. The AtomISP\nhas a separate architecture, and doesn't use the v4l2-subdevice API.\n\n\n> If you think that it would be valuable to try and mainline this, I'd be\n\nAbsolutely valuable. (In fact, 'almost' a requirement, to have a camera\nsupported in libcamera).\n\n> interested to do so once I can get libcamera working on my hardware, though\n> I'll likely need some guidance in the process, since I'll be learning a lot.\n\nYes, please do - I keep asking for the kernel patches to be posted, but\nno one has done so. As I understand it, megi isn't interested in getting\nthe the patches upstream. So someone needs to take ownership of them\n(ideally with the hardware, so you're an excellent candidate).\n\nHappy to help with the process anywhere along the way.\n\nThe kernel development model is much like libcamera (or rather,\nlibcamera development model is based on the kernel) - so you're already\nthrough training. \n\nPatches need to be based on an upstream kernel, and posted as a series\nto the linux-media mailing list. There are some nuances which are a\npain, but it's best to work through those as they arrise. (like adding\ndifferent lists for different patches for instance covering the device\ntree bindings).\n\n> > Nice this matches the CCS defined linear gain model.\n> > However the sensor allows to select two formats for the analogue gain\n> > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > register 0x3503[2] and it would be nice to validate that the driver\n> > uses the format that you expect.\n> Frankly, I'm not sure what you mean by this, but I'll try to find out from\n> the driver above and verify.\n> \n> > Knowing what driver you're using is relevant, in example, as the\n> > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > what libcamera expects, and sensor drivers are expected to express the\n> > V4L2_CID_EXPOSURE control in line units.\n> \n> > From Documentation/sensor_driver_requirements.rst\n> \n> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > requires it to be expressed as a number of image lines. Camera sensor\n> drivers\n> > that do not comply with this requirement will need to be adapted or will\n> produce\n> > incorrect results.\n> \n> Let me read the driver and get back to you.\n> \n> \n> On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi wrote:\n> \n> > Hi Nicholas,\n> >\n> > please pardon me for being pedantic, but I would:\n> > libcamera: Add support for ov8858 sensor\n> >\n> > instead of:\n> > ipa: libcamera: add metadata for the ov8858 sensor\n> >\n> > \"metadata\" is an overloaded already term and usually refers to the\n> > properties associated to a captured frame (the term comes from the\n> > Android lingo, but we use it too).\n> >\n> > On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> > libcamera-devel wrote:\n> > > From: Nicholas Roth \n> > >\n> > > Currently, libcamera does not have information for the ov8858 sensor\n> > > used in the PinePhone Pro, a phone designed to run Linux.\n> > >\n> > > This commit adds metadata, especially that sensor gain is reported and\n> > > set in 1/16 discrete increments.\n> > >\n> > > For more information, see \"5.8 manual exposure compensation/ manual\n> > > gain compensation\" in [0].\n> > >\n> > > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n> >\n> > The sensor doesn't seem to be supported mainline :(\n> > libcamera has a policy that requires supported components to be\n> > upstream or at least on their way to upstream (ie posted to the\n> > linux-media mailing list). The policy is there because different\n> > downstream driver implementations might behave differently one from\n> > the other, making it impossible for libcamera to support the part\n> > fully. The policy is a strict requirement for ISPs, I guess we're a\n> > bit more elastic when it comes to sensor. Howver knowing what driver\n> > you are using, where it lives, and if there's any plan to upstream it\n> > would be great.\n> >\n> > Let's start from the first point: where does this driver lives ? What\n> > kernel are you using ?\n> >\n> > Knowing what driver you're using is relevant, in example, as the\n> > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > what libcamera expects, and sensor drivers are expected to express the\n> > V4L2_CID_EXPOSURE control in line units.\n> >\n> > From Documentation/sensor_driver_requirements.rst\n> >\n> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > requires it to be expressed as a number of image lines. Camera sensor\n> > drivers\n> > that do not comply with this requirement will need to be adapted or will\n> > produce\n> > incorrect results.\n> >\n> > >\n> > > Signed-off-by: Nicholas Roth \n> > > ---\n> > > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> > > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> > > 2 files changed, 25 insertions(+)\n> > >\n> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n> > b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > index 35056bec..f2040cbd 100644\n> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > @@ -476,6 +476,17 @@ public:\n> > > };\n> > > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > >\n> > > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> > > +{\n> > > +public:\n> > > + CameraSensorHelperOv8858()\n> > > + {\n> > > + gainType_ = AnalogueGainLinear;\n> > > + gainConstants_.linear = { 1, 0, 0, 16 };\n> > > + }\n> > > +};\n> >\n> > Nice this matches the CCS defined linear gain model.\n> > However the sensor allows to select two formats for the analogue gain\n> > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > register 0x3503[2] and it would be nice to validate that the driver\n> > uses the format that you expect.\n> >\n> > > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n> >\n> > Ah! that's a weird name for the sensor entity :)\n> >\n> > > +\n> > > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > > {\n> > > public:\n> > > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> > b/src/libcamera/camera_sensor_properties.cpp\n> > > index e5f27f06..d0757c15 100644\n> > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > @@ -146,6 +146,20 @@ const CameraSensorProperties\n> > *CameraSensorProperties::get(const std::string &sen\n> > > */\n> > > },\n> > > } },\n> > > + { \"m00_f_ov8858\", {\n> > > + .unitCellSize = { 1200, 1200 },\n> >\n> > I read 1.12 um x 1.12 um\n> >\n> > > + .testPatternModes = {\n> > > + { controls::draft::TestPatternModeOff, 0 },\n> > > + {\n> > controls::draft::TestPatternModeColorBars, 1 },\n> > > + /*\n> > > + * No best corresponding test pattern for:\n> > > + * 1: \"Vertical Color Bar Type 1\",\n> > > + * 2: \"Vertical Color Bar Type 2\",\n> > > + * 3: \"Vertical Color Bar Type 3\",\n> > > + * 4: \"Vertical Color Bar Type 4\"\n> > > + */\n> > > + },\n> > > + } },\n> > > { \"ov8865\", {\n> > > .unitCellSize = { 1400, 1400 },\n> > > .testPatternModes = {\n> > > --\n> > > 2.34.1\n> > >\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 A40C3BD16B\n\tfor ;\n\tFri, 28 Oct 2022 08:37:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E62ED62FBB;\n\tFri, 28 Oct 2022 10:37:31 +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 60B2662F41\n\tfor ;\n\tFri, 28 Oct 2022 10:37:30 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD4776BE;\n\tFri, 28 Oct 2022 10:37:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666946252;\n\tbh=w6Uk6n45IvHIJJzbWSBD79IgvAvLF8YSFEgoelM1D00=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=NhKCeNiGR12UwX7N7PXIVjU/tpZ4LVa5P18u+M4ZaWMvuk2bd6x7+Yf3sVc6yn1y7\n\t0PBMNge3fJXdH9yN449/tUIlnda63Zy02OW3x635uXJc3EZt8rxO51YSgw4OAggwD5\n\ttuy9eGNkbeV5orRbqbhAgy3WLomjJPnrTvXfajXh8R0F9UoI6CNjTL+qk28o8DrMKU\n\ts8f3hOx7KMVV0FM9Ry3qr5UWRxDJoLH8kP/6CcLyuLInT7If0v/rAXCJZclpV/TV8n\n\tEkTF4nlW+xTR3YQa2L6owwS0iwB4dy2gbs1hGfX8b3kBcYEp73DeFY1FzD4BXBrv3A\n\tvece52IS12zeA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666946249;\n\tbh=w6Uk6n45IvHIJJzbWSBD79IgvAvLF8YSFEgoelM1D00=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=iazppGgCrXq7zEF7+yyqngdTBUinS50FmIezGSw2GjaYL09ViP7HxyU+Xz4ba7PJ8\n\tDJO7jnRxci9hmRwR1sBjng7BxlqXq+h1d0VWJ9hWE566BTrMjWxpEpdJXmqOFYXBLn\n\t+8tc6vZ4WX2JSIhY6dhrreLNFbM6GKyZ+4eSOQPM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iazppGgC\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t","To":"Jacopo Mondi , Nicholas Roth ,\n\tNicholas Roth via libcamera-devel ","Date":"Fri, 28 Oct 2022 09:37:27 +0100","Message-ID":"<166694624777.1769154.10301809954931640270@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25649,"web_url":"https://patchwork.libcamera.org/comment/25649/","msgid":"<166695273851.1769154.5384280975536306961@Monstersaurus>","date":"2022-10-28T10:25:38","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2022-10-28 09:14:27)\n> Hi Nicholas,\n> + Laurent question below\n> \n> On Thu, Oct 27, 2022 at 05:21:37PM -0500, Nicholas Roth wrote:\n> > > Nice this matches the CCS defined linear gain model.\n> > > However the sensor allows to select two formats for the analogue gain\n> > > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > > register 0x3503[2] and it would be nice to validate that the driver\n> > > uses the format that you expect.\n> >\n> > I think I understand this better after reading the driver. In the register\n> > value settings, I see:\n> > {0x3502, 0x40}, // exposure L\n> > {0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain\n> > {0x3505, 0x80}, // gain option\n> >\n> > The 0 low-nibbl indicates we're using \"real gain.\"\n> >\n> > According to the datasheet:\n> > 0x3503[2]=0, gain[7:0] is real gain format,\n> > where low 4 bits are fraction bits, for\n> > example, 0x10 is 1x gain, 0x28 is 2.5x gain\n> >\n> > This tells me that we need to divide reported gain from the driver by 16 to\n> > get a properly-scaled double-value gain, which these gain constants do.\n> >\n> \n> Here you go!\n> \n> Looking at the driver you're using I indeed see 3503[2]=0 in all modes\n> \n> > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > > requires it to be expressed as a number of image lines. Camera sensor\n> > drivers\n> > > that do not comply with this requirement will need to be adapted or will\n> > produce\n> > > incorrect results.\n> >\n> > Looks like the driver multiples inputs x16 to convert to 1/16 line length\n> > before writing to the register...\n> >\n> > case V4L2_CID_EXPOSURE:\n> > /* 4 least significant bits of expsoure are fractional part */\n> > ret = ov8858_write_reg(ov8858->client,\n> > OV8858_REG_EXPOSURE,\n> > OV8858_REG_VALUE_24BIT,\n> > ctrl->val << 4);\n> >\n> \n> Great, it means that towards userspace the exposure is indeed in units\n> of 1 line.\n> \n> >\n> > Let me know if this looks good, and what needs to happen to merge this!\n> \n> I think I had a comment on the pixel cell size.\n> \n> Apart from that, let me check with Laurent about the policy when it\n> comes to supporting sensor without an upstream driver.\n> \n> I would not be super happy of adding support to \"m00_f_ov8858\" as if the\n> driver will be upstreamed the entity name should at least change.\n\nYes, this is problematic.\n\nLooking at the driver, it comes from: \n https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c#L2936\n\nThis will likely have to be changed to get this merged upstream in the\nkernel.\n\n\n> As a middle term solution we can add support for \"m00_f_ov8858\" with a\n> todo note to move to a more canonical \"ov8858\" name once the driver is\n> upstreamed, or at least submitted upstream.\n\nThis is problematic too - there would be overlap - would we have to\nduplicate the camera sensor properties? Or perhaps support registering a\ncamera sensor helper as mutliple names?\n\n\nThis is the 'why' upstreaming the driver is 'almost' a requirement.\nParts like that should be solved before integrating in libcamera\nideally. Perhaps we can workaround it this time as it's hopefully\nmanageable - but it means that we would then have libcamera 'supporting'\nthe pinephone in one version, but then losing it's support when we\nsuddenly change the names to match the upstream kernel until the\npinephone developers update their kernel.\n\nAll a bit messy ... :S\n\n> \n> Thanks\n> j\n> \n> >\n> > Thanks,\n> > -Nicholas\n> >\n> > On Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth \n> > wrote:\n> >\n> > > > please pardon me for being pedantic, but I would:\n> > > > libcamera: Add support for ov8858 sensor\n> > > >\n> > > > instead of:\n> > > > ipa: libcamera: add metadata for the ov8858 sensor\n> > >\n> > > Happy to make the change.\n> > >\n> > > > The sensor doesn't seem to be supported mainline :(...\n> > > > Let's start from the first point: where does this driver lives ? What\n> > > > kernel are you using ?\n> > > I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n> > >\n> > > It looks like my package manager is picking up the Manjaro kernel from\n> > > https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,\n> > > which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no\n> > > 8858-related changes. This ultimately pulls the driver from\n> > > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> > > .\n> > >\n> > > Looks like this has been in and out of Torvalds' peripheral view, e.g.:\n> > > https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n> > >\n> > > If you think that it would be valuable to try and mainline this, I'd be\n> > > interested to do so once I can get libcamera working on my hardware, though\n> > > I'll likely need some guidance in the process, since I'll be learning a lot.\n> > >\n> > > > Nice this matches the CCS defined linear gain model.\n> > > > However the sensor allows to select two formats for the analogue gain\n> > > > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > > > register 0x3503[2] and it would be nice to validate that the driver\n> > > > uses the format that you expect.\n> > > Frankly, I'm not sure what you mean by this, but I'll try to find out from\n> > > the driver above and verify.\n> > >\n> > > > Knowing what driver you're using is relevant, in example, as the\n> > > > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > > > what libcamera expects, and sensor drivers are expected to express the\n> > > > V4L2_CID_EXPOSURE control in line units.\n> > >\n> > > > From Documentation/sensor_driver_requirements.rst\n> > >\n> > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > > > requires it to be expressed as a number of image lines. Camera sensor\n> > > drivers\n> > > > that do not comply with this requirement will need to be adapted or will\n> > > produce\n> > > > incorrect results.\n> > >\n> > > Let me read the driver and get back to you.\n> > >\n> > >\n> > > On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi wrote:\n> > >\n> > >> Hi Nicholas,\n> > >>\n> > >> please pardon me for being pedantic, but I would:\n> > >> libcamera: Add support for ov8858 sensor\n> > >>\n> > >> instead of:\n> > >> ipa: libcamera: add metadata for the ov8858 sensor\n> > >>\n> > >> \"metadata\" is an overloaded already term and usually refers to the\n> > >> properties associated to a captured frame (the term comes from the\n> > >> Android lingo, but we use it too).\n> > >>\n> > >> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> > >> libcamera-devel wrote:\n> > >> > From: Nicholas Roth \n> > >> >\n> > >> > Currently, libcamera does not have information for the ov8858 sensor\n> > >> > used in the PinePhone Pro, a phone designed to run Linux.\n> > >> >\n> > >> > This commit adds metadata, especially that sensor gain is reported and\n> > >> > set in 1/16 discrete increments.\n> > >> >\n> > >> > For more information, see \"5.8 manual exposure compensation/ manual\n> > >> > gain compensation\" in [0].\n> > >> >\n> > >> > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n> > >>\n> > >> The sensor doesn't seem to be supported mainline :(\n> > >> libcamera has a policy that requires supported components to be\n> > >> upstream or at least on their way to upstream (ie posted to the\n> > >> linux-media mailing list). The policy is there because different\n> > >> downstream driver implementations might behave differently one from\n> > >> the other, making it impossible for libcamera to support the part\n> > >> fully. The policy is a strict requirement for ISPs, I guess we're a\n> > >> bit more elastic when it comes to sensor. Howver knowing what driver\n> > >> you are using, where it lives, and if there's any plan to upstream it\n> > >> would be great.\n> > >>\n> > >> Let's start from the first point: where does this driver lives ? What\n> > >> kernel are you using ?\n> > >>\n> > >> Knowing what driver you're using is relevant, in example, as the\n> > >> OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > >> what libcamera expects, and sensor drivers are expected to express the\n> > >> V4L2_CID_EXPOSURE control in line units.\n> > >>\n> > >> From Documentation/sensor_driver_requirements.rst\n> > >>\n> > >> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > >> requires it to be expressed as a number of image lines. Camera sensor\n> > >> drivers\n> > >> that do not comply with this requirement will need to be adapted or will\n> > >> produce\n> > >> incorrect results.\n> > >>\n> > >> >\n> > >> > Signed-off-by: Nicholas Roth \n> > >> > ---\n> > >> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> > >> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> > >> > 2 files changed, 25 insertions(+)\n> > >> >\n> > >> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n> > >> b/src/ipa/libipa/camera_sensor_helper.cpp\n> > >> > index 35056bec..f2040cbd 100644\n> > >> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > >> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > >> > @@ -476,6 +476,17 @@ public:\n> > >> > };\n> > >> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > >> >\n> > >> > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> > >> > +{\n> > >> > +public:\n> > >> > + CameraSensorHelperOv8858()\n> > >> > + {\n> > >> > + gainType_ = AnalogueGainLinear;\n> > >> > + gainConstants_.linear = { 1, 0, 0, 16 };\n> > >> > + }\n> > >> > +};\n> > >>\n> > >> Nice this matches the CCS defined linear gain model.\n> > >> However the sensor allows to select two formats for the analogue gain\n> > >> the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > >> register 0x3503[2] and it would be nice to validate that the driver\n> > >> uses the format that you expect.\n> > >>\n> > >> > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n> > >>\n> > >> Ah! that's a weird name for the sensor entity :)\n> > >>\n> > >> > +\n> > >> > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > >> > {\n> > >> > public:\n> > >> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> > >> b/src/libcamera/camera_sensor_properties.cpp\n> > >> > index e5f27f06..d0757c15 100644\n> > >> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > >> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > >> > @@ -146,6 +146,20 @@ const CameraSensorProperties\n> > >> *CameraSensorProperties::get(const std::string &sen\n> > >> > */\n> > >> > },\n> > >> > } },\n> > >> > + { \"m00_f_ov8858\", {\n> > >> > + .unitCellSize = { 1200, 1200 },\n> > >>\n> > >> I read 1.12 um x 1.12 um\n> > >>\n> > >> > + .testPatternModes = {\n> > >> > + { controls::draft::TestPatternModeOff, 0\n> > >> },\n> > >> > + {\n> > >> controls::draft::TestPatternModeColorBars, 1 },\n> > >> > + /*\n> > >> > + * No best corresponding test pattern for:\n> > >> > + * 1: \"Vertical Color Bar Type 1\",\n> > >> > + * 2: \"Vertical Color Bar Type 2\",\n> > >> > + * 3: \"Vertical Color Bar Type 3\",\n> > >> > + * 4: \"Vertical Color Bar Type 4\"\n> > >> > + */\n> > >> > + },\n> > >> > + } },\n> > >> > { \"ov8865\", {\n> > >> > .unitCellSize = { 1400, 1400 },\n> > >> > .testPatternModes = {\n> > >> > --\n> > >> > 2.34.1\n> > >> >\n> > >>\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 D2E23BDB16\n\tfor ;\n\tFri, 28 Oct 2022 10:25:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C4D162FCC;\n\tFri, 28 Oct 2022 12:25: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 2B9D561F4A\n\tfor ;\n\tFri, 28 Oct 2022 12:25:41 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C6E4C6BE;\n\tFri, 28 Oct 2022 12:25:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666952742;\n\tbh=pca58ItXuPP2DPEf2tMQ4bFR7hlBPDKMuXQo3FuKtuw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=2GXRHjlLQn+NHgQWLUiZWO4Xri9KIrLeXtnceP6NZf2QyCWhwrggVmNYhhgp1xLVn\n\tNXyYF+6wGzM9pMvnr3Zd1jFspUrqUn6x6jp2NZI1EC9brt//fgGSdGp7zRgVug43G9\n\tLiURnBhNJAnUWDa8v7w3XIG5TgxEZIwdsV4/wTnkHbtpP2sMdgnE+KBU5h3PNllKmQ\n\tGm7GP/0EpJz29YhGhX2xwfYKOZbmsauTNcFHZPmRRp6LlEplKwFff3oOheBmykxrgi\n\trlw2GyB6hJCE2DZgLNIyT9bitiq5VzqJhZ+zP2CqoS/a6pcJ/tVT8ALA7e7Dbsada8\n\tfGNGRdqMkmgwQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666952740;\n\tbh=pca58ItXuPP2DPEf2tMQ4bFR7hlBPDKMuXQo3FuKtuw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=dD4V/WMz8dCH1x+lEec2IJ07KabQdtJV7ikImxsCKRgsTBgETqJm1L1KDnhRd9WKX\n\tnAmJwruVhib3D5P0q86AC/+VOlO/cMjYTsVwO8VyT3V/zxHg9RjxmKmeYHZ3RBEy2R\n\ta2tJLKoTJ2RY3YBVEGuIROk/L2t37fpUWc+ajdsk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dD4V/WMz\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221028081427.s25wdjmmqh5ikjem@uno.localdomain>","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t\n\t\n\t<20221028081427.s25wdjmmqh5ikjem@uno.localdomain>","To":"Jacopo Mondi , Jacopo Mondi via libcamera-devel\n\t, \n\tLaurent Pinchart ,\n\tNicholas Roth ","Date":"Fri, 28 Oct 2022 11:25:38 +0100","Message-ID":"<166695273851.1769154.5384280975536306961@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25679,"web_url":"https://patchwork.libcamera.org/comment/25679/","msgid":"","date":"2022-10-30T16:20:46","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Oct 28, 2022 at 11:25:38AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2022-10-28 09:14:27)\n> > Hi Nicholas,\n> > + Laurent question below\n> > \n> > On Thu, Oct 27, 2022 at 05:21:37PM -0500, Nicholas Roth wrote:\n> > > > Nice this matches the CCS defined linear gain model.\n> > > > However the sensor allows to select two formats for the analogue gain\n> > > > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > > > register 0x3503[2] and it would be nice to validate that the driver\n> > > > uses the format that you expect.\n> > >\n> > > I think I understand this better after reading the driver. In the register\n> > > value settings, I see:\n> > > {0x3502, 0x40}, // exposure L\n> > > {0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain\n> > > {0x3505, 0x80}, // gain option\n> > >\n> > > The 0 low-nibbl indicates we're using \"real gain.\"\n> > >\n> > > According to the datasheet:\n> > > 0x3503[2]=0, gain[7:0] is real gain format,\n> > > where low 4 bits are fraction bits, for\n> > > example, 0x10 is 1x gain, 0x28 is 2.5x gain\n> > >\n> > > This tells me that we need to divide reported gain from the driver by 16 to\n> > > get a properly-scaled double-value gain, which these gain constants do.\n> > \n> > Here you go!\n> > \n> > Looking at the driver you're using I indeed see 3503[2]=0 in all modes\n> > \n> > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > > > requires it to be expressed as a number of image lines. Camera sensor drivers\n> > > > that do not comply with this requirement will need to be adapted or will produce\n> > > > incorrect results.\n> > >\n> > > Looks like the driver multiples inputs x16 to convert to 1/16 line length\n> > > before writing to the register...\n> > >\n> > > case V4L2_CID_EXPOSURE:\n> > > /* 4 least significant bits of expsoure are fractional part */\n> > > ret = ov8858_write_reg(ov8858->client,\n> > > OV8858_REG_EXPOSURE,\n> > > OV8858_REG_VALUE_24BIT,\n> > > ctrl->val << 4);\n> > \n> > Great, it means that towards userspace the exposure is indeed in units\n> > of 1 line.\n> > \n> > > Let me know if this looks good, and what needs to happen to merge this!\n> > \n> > I think I had a comment on the pixel cell size.\n> > \n> > Apart from that, let me check with Laurent about the policy when it\n> > comes to supporting sensor without an upstream driver.\n> > \n> > I would not be super happy of adding support to \"m00_f_ov8858\" as if the\n> > driver will be upstreamed the entity name should at least change.\n> \n> Yes, this is problematic.\n> \n> Looking at the driver, it comes from: \n> https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c#L2936\n> \n> This will likely have to be changed to get this merged upstream in the\n> kernel.\n> \n> > As a middle term solution we can add support for \"m00_f_ov8858\" with a\n> > todo note to move to a more canonical \"ov8858\" name once the driver is\n> > upstreamed, or at least submitted upstream.\n> \n> This is problematic too - there would be overlap - would we have to\n> duplicate the camera sensor properties? Or perhaps support registering a\n> camera sensor helper as mutliple names?\n> \n> \n> This is the 'why' upstreaming the driver is 'almost' a requirement.\n> Parts like that should be solved before integrating in libcamera\n> ideally. Perhaps we can workaround it this time as it's hopefully\n> manageable - but it means that we would then have libcamera 'supporting'\n> the pinephone in one version, but then losing it's support when we\n> suddenly change the names to match the upstream kernel until the\n> pinephone developers update their kernel.\n\nAgreed. The driver should be upstreamed, and while we don't require\ndrivers to be merged upstream before support for them can be added to\nlibcamera, we do require the drivers to be on their way to upstream.\nThis means showing a reasonable effort to get it there, most likely with\na patch posted to the linux-media mailing list.\n\nGive that the entity name will need to change when upstreaming, one way\nforward would be to patch the pinephone kernel driver with the correct\nentity name already, and use it in libcamera right away.\n\n> All a bit messy ... :S\n>\n> > > On Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth wrote:\n> > >\n> > > > > please pardon me for being pedantic, but I would:\n> > > > > libcamera: Add support for ov8858 sensor\n> > > > >\n> > > > > instead of:\n> > > > > ipa: libcamera: add metadata for the ov8858 sensor\n> > > >\n> > > > Happy to make the change.\n> > > >\n> > > > > The sensor doesn't seem to be supported mainline :(...\n> > > > > Let's start from the first point: where does this driver lives ? What\n> > > > > kernel are you using ?\n> > > > I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n> > > >\n> > > > It looks like my package manager is picking up the Manjaro kernel from\n> > > > https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,\n> > > > which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no\n> > > > 8858-related changes. This ultimately pulls the driver from\n> > > > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> > > > .\n> > > >\n> > > > Looks like this has been in and out of Torvalds' peripheral view, e.g.:\n> > > > https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n> > > >\n> > > > If you think that it would be valuable to try and mainline this, I'd be\n> > > > interested to do so once I can get libcamera working on my hardware, though\n> > > > I'll likely need some guidance in the process, since I'll be learning a lot.\n> > > >\n> > > > > Nice this matches the CCS defined linear gain model.\n> > > > > However the sensor allows to select two formats for the analogue gain\n> > > > > the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > > > > register 0x3503[2] and it would be nice to validate that the driver\n> > > > > uses the format that you expect.\n> > > > \n> > > > Frankly, I'm not sure what you mean by this, but I'll try to find out from\n> > > > the driver above and verify.\n> > > >\n> > > > > Knowing what driver you're using is relevant, in example, as the\n> > > > > OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > > > > what libcamera expects, and sensor drivers are expected to express the\n> > > > > V4L2_CID_EXPOSURE control in line units.\n> > > >\n> > > > > From Documentation/sensor_driver_requirements.rst\n> > > >\n> > > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > > > > requires it to be expressed as a number of image lines. Camera sensor drivers\n> > > > > that do not comply with this requirement will need to be adapted or will produce\n> > > > > incorrect results.\n> > > >\n> > > > Let me read the driver and get back to you.\n> > > >\n> > > >\n> > > > On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi wrote:\n> > > >\n> > > >> Hi Nicholas,\n> > > >>\n> > > >> please pardon me for being pedantic, but I would:\n> > > >> libcamera: Add support for ov8858 sensor\n> > > >>\n> > > >> instead of:\n> > > >> ipa: libcamera: add metadata for the ov8858 sensor\n> > > >>\n> > > >> \"metadata\" is an overloaded already term and usually refers to the\n> > > >> properties associated to a captured frame (the term comes from the\n> > > >> Android lingo, but we use it too).\n> > > >>\n> > > >> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> > > >> libcamera-devel wrote:\n> > > >> > From: Nicholas Roth \n> > > >> >\n> > > >> > Currently, libcamera does not have information for the ov8858 sensor\n> > > >> > used in the PinePhone Pro, a phone designed to run Linux.\n> > > >> >\n> > > >> > This commit adds metadata, especially that sensor gain is reported and\n> > > >> > set in 1/16 discrete increments.\n> > > >> >\n> > > >> > For more information, see \"5.8 manual exposure compensation/ manual\n> > > >> > gain compensation\" in [0].\n> > > >> >\n> > > >> > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n> > > >>\n> > > >> The sensor doesn't seem to be supported mainline :(\n> > > >> libcamera has a policy that requires supported components to be\n> > > >> upstream or at least on their way to upstream (ie posted to the\n> > > >> linux-media mailing list). The policy is there because different\n> > > >> downstream driver implementations might behave differently one from\n> > > >> the other, making it impossible for libcamera to support the part\n> > > >> fully. The policy is a strict requirement for ISPs, I guess we're a\n> > > >> bit more elastic when it comes to sensor. Howver knowing what driver\n> > > >> you are using, where it lives, and if there's any plan to upstream it\n> > > >> would be great.\n> > > >>\n> > > >> Let's start from the first point: where does this driver lives ? What\n> > > >> kernel are you using ?\n> > > >>\n> > > >> Knowing what driver you're using is relevant, in example, as the\n> > > >> OV5688 sensor computes exposure in 1/16 of line length. This is not\n> > > >> what libcamera expects, and sensor drivers are expected to express the\n> > > >> V4L2_CID_EXPOSURE control in line units.\n> > > >>\n> > > >> From Documentation/sensor_driver_requirements.rst\n> > > >>\n> > > >> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > > >> requires it to be expressed as a number of image lines. Camera sensor drivers\n> > > >> that do not comply with this requirement will need to be adapted or will produce\n> > > >> incorrect results.\n> > > >>\n> > > >> > Signed-off-by: Nicholas Roth \n> > > >> > ---\n> > > >> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> > > >> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> > > >> > 2 files changed, 25 insertions(+)\n> > > >> >\n> > > >> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > >> > index 35056bec..f2040cbd 100644\n> > > >> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > >> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > >> > @@ -476,6 +476,17 @@ public:\n> > > >> > };\n> > > >> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > > >> >\n> > > >> > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> > > >> > +{\n> > > >> > +public:\n> > > >> > + CameraSensorHelperOv8858()\n> > > >> > + {\n> > > >> > + gainType_ = AnalogueGainLinear;\n> > > >> > + gainConstants_.linear = { 1, 0, 0, 16 };\n> > > >> > + }\n> > > >> > +};\n> > > >>\n> > > >> Nice this matches the CCS defined linear gain model.\n> > > >> However the sensor allows to select two formats for the analogue gain\n> > > >> the \"real gain\" format and \"sensor gain\" format. The selection is made by\n> > > >> register 0x3503[2] and it would be nice to validate that the driver\n> > > >> uses the format that you expect.\n> > > >>\n> > > >> > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\", CameraSensorHelperOv8858)\n> > > >>\n> > > >> Ah! that's a weird name for the sensor entity :)\n> > > >>\n> > > >> > +\n> > > >> > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > > >> > {\n> > > >> > public:\n> > > >> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > > >> > index e5f27f06..d0757c15 100644\n> > > >> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > >> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > >> > @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > > >> > */\n> > > >> > },\n> > > >> > } },\n> > > >> > + { \"m00_f_ov8858\", {\n> > > >> > + .unitCellSize = { 1200, 1200 },\n> > > >>\n> > > >> I read 1.12 um x 1.12 um\n> > > >>\n> > > >> > + .testPatternModes = {\n> > > >> > + { controls::draft::TestPatternModeOff, 0 },\n> > > >> > + { controls::draft::TestPatternModeColorBars, 1 },\n> > > >> > + /*\n> > > >> > + * No best corresponding test pattern for:\n> > > >> > + * 1: \"Vertical Color Bar Type 1\",\n> > > >> > + * 2: \"Vertical Color Bar Type 2\",\n> > > >> > + * 3: \"Vertical Color Bar Type 3\",\n> > > >> > + * 4: \"Vertical Color Bar Type 4\"\n> > > >> > + */\n> > > >> > + },\n> > > >> > + } },\n> > > >> > { \"ov8865\", {\n> > > >> > .unitCellSize = { 1400, 1400 },\n> > > >> > .testPatternModes = {","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 7F3F4BDB16\n\tfor ;\n\tSun, 30 Oct 2022 16:21:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEB6C6301C;\n\tSun, 30 Oct 2022 17:21:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5190061F47\n\tfor ;\n\tSun, 30 Oct 2022 17:21:11 +0100 (CET)","from pendragon.ideasonboard.com (85-76-73-45-nat.elisa-mobile.fi\n\t[85.76.73.45])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A997440;\n\tSun, 30 Oct 2022 17:21:09 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667146872;\n\tbh=5DopcBCB9QWS7P80zMfaEBdWC69jI+xKOv9nOqmvzFY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=0ZEa2/fl2euVbVWXl0IS9lYW+9mOht6dTbt044trJBs9ZlolEx31/42OSrLY/cshl\n\tNJBlPKzHWLjZ8G9NbWjU+Nj0MxhGuSOJghO6nnXsCdQtMbtnUVTCXHSbmoTt8LQi6P\n\tbPcJcWNng8ATUj2mXNXKF9r5f08J4XqWQ4cfCJ9q6u1jwxaOkWkw2czh9HfkiADfM/\n\tTBtAGFLFuQUiimlX0WicSmdcovmNRSbeCdqXH2Ux0tiHly+UVihhDRvpOBt/TEMK02\n\tZ+udaMfSWNZ7RrC9ieQGoL1eIiDzGlt1wlHk9fxE3IYr4aXxsCOj7LUSIBFIGO6zKi\n\treiGusX2guzHA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667146870;\n\tbh=5DopcBCB9QWS7P80zMfaEBdWC69jI+xKOv9nOqmvzFY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PC63O9dzt2/F1Mia0NDX1Nb7GLOYWHw3m8K/AF3K3qbKT12979IttcUqNW6Df0eJR\n\t/8D1wUcv7s2pVfHp7hbVxr2IS8hHgVC3DnjqZ7vzbJZEDcMA5FV2YIJNgjM3gLs9Ha\n\tbwJ57Pv2Br5CNnyHZTptEsUDDudqsagqdMpoh0mo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PC63O9dz\"; dkim-atps=neutral","Date":"Sun, 30 Oct 2022 18:20:46 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t\n\t\n\t<20221028081427.s25wdjmmqh5ikjem@uno.localdomain>\n\t<166695273851.1769154.5384280975536306961@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166695273851.1769154.5384280975536306961@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Nicholas Roth , Jacopo Mondi via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25689,"web_url":"https://patchwork.libcamera.org/comment/25689/","msgid":"","date":"2022-10-30T22:15:42","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":137,"url":"https://patchwork.libcamera.org/api/people/137/","name":"Nicholas Roth","email":"nicholas@rothemail.net"},"content":"> Agreed. The driver should be upstreamed\n\nI'm going to send an initial version to linux-media today for comment. I'll\ninclude a link to their mailing list with the next set of changes.\n\n> Give that the entity name will need to change when upstreaming, one way\n> forward would be to patch the pinephone kernel driver with the correct\n> entity name already, and use it in libcamera right away.\n\nTL;DR: Media node names do not seem to be standardized and various formats\nexist. I suggest adding an `else` case to [6] with an additional regex for\nthe ov8858 driver for maximum compatibility with non-mainline ov8858\nimplementations while we sort out getting this upstreamed.\n--\n\nUpon reading the driver [0] more closely, it looks like \"m00_f_ov8858\" is\njust the *subdevice name *(set to\n\"m{module_index:02d}_{facing}_{OV8858_NAME}\" on :2936), not the driver\nname (set to OV8858_NAME == \"ov8858\" on :3004) or sensor name (set to\nOV8858_NAME == \"ov8858\" on :1778).\n\nReading the kernel docs [3], it looks like this is required to be unique:\n\"Afterwards you need to initialize sd->name with a unique name and set the\nmodule owner. This is done for you if you use the i2c helper functions.\"\n\nThe subdevice name *defaults* to the driver name in [4] when\nv4l2_i2c_subdev_init() invokes v4l2_i2c_subdev_set_name(sd, client, NULL,\nNULL), but this is not guaranteed to be the driver name if I understand\ncorrectly. In fact, this appears to solve for the case where there may be\nmultiple ov8858s installed, which the default implementation would not.\nThough there is some ambiguity in the phrase \"unique name,\" v4l2-subdev.h\n[5] seems to suggest that these must be unique among device instances, not\ndrivers, since `struct v4l2_subdev` corresponds to a devnode and also\nspecifies \"@name: Name of the sub-device. Please notice that the name must\nbe unique.\"\n\nMeanwhile, both Megi's imx258 fork [1] and the mainline imx258 driver [2]\nuse the default implementation, so we do not have this issue using\nlibcamera with an imx258.\n\n***libcamera subdevice name handling (regex guess)***\nCurrently, libcamera uses a regex to guess the model name from the\nsubdevice name in v4l2_subdevice.cpp V4L2Subdevice::model() [6]. Instead,\n\n***Proposed Solution***:\nI suggest adding an `else` case to [6] with an additional regex for the\nov8858 driver for maximum compatibility with non-mainline ov8858\nimplementations while we sort out getting this upstreamed.\n\n0:\nhttps://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/ov8858.c\n1:\nhttps://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/imx258.c\n2: https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c\n3: https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html\n4:\nhttps://github.com/torvalds/linux/blob/master/drivers/media/v4l2-core/v4l2-i2c.c\n5: https://github.com/torvalds/linux/blob/master/include/media/v4l2-subdev.h\n6:\nhttps://github.com/kbingham/libcamera/blob/master/src/libcamera/v4l2_subdevice.cpp\n\nOn Sun, Oct 30, 2022 at 11:21 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hello,\n>\n> On Fri, Oct 28, 2022 at 11:25:38AM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-10-28 09:14:27)\n> > > Hi Nicholas,\n> > > + Laurent question below\n> > >\n> > > On Thu, Oct 27, 2022 at 05:21:37PM -0500, Nicholas Roth wrote:\n> > > > > Nice this matches the CCS defined linear gain model.\n> > > > > However the sensor allows to select two formats for the analogue\n> gain\n> > > > > the \"real gain\" format and \"sensor gain\" format. The selection is\n> made by\n> > > > > register 0x3503[2] and it would be nice to validate that the driver\n> > > > > uses the format that you expect.\n> > > >\n> > > > I think I understand this better after reading the driver. In the\n> register\n> > > > value settings, I see:\n> > > > {0x3502, 0x40}, // exposure L\n> > > > {0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain\n> > > > {0x3505, 0x80}, // gain option\n> > > >\n> > > > The 0 low-nibbl indicates we're using \"real gain.\"\n> > > >\n> > > > According to the datasheet:\n> > > > 0x3503[2]=0, gain[7:0] is real gain format,\n> > > > where low 4 bits are fraction bits, for\n> > > > example, 0x10 is 1x gain, 0x28 is 2.5x gain\n> > > >\n> > > > This tells me that we need to divide reported gain from the driver\n> by 16 to\n> > > > get a properly-scaled double-value gain, which these gain constants\n> do.\n> > >\n> > > Here you go!\n> > >\n> > > Looking at the driver you're using I indeed see 3503[2]=0 in all modes\n> > >\n> > > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control,\n> libcamera\n> > > > > requires it to be expressed as a number of image lines. Camera\n> sensor drivers\n> > > > > that do not comply with this requirement will need to be adapted\n> or will produce\n> > > > > incorrect results.\n> > > >\n> > > > Looks like the driver multiples inputs x16 to convert to 1/16 line\n> length\n> > > > before writing to the register...\n> > > >\n> > > > case V4L2_CID_EXPOSURE:\n> > > > /* 4 least significant bits of expsoure are fractional part */\n> > > > ret = ov8858_write_reg(ov8858->client,\n> > > > OV8858_REG_EXPOSURE,\n> > > > OV8858_REG_VALUE_24BIT,\n> > > > ctrl->val << 4);\n> > >\n> > > Great, it means that towards userspace the exposure is indeed in units\n> > > of 1 line.\n> > >\n> > > > Let me know if this looks good, and what needs to happen to merge\n> this!\n> > >\n> > > I think I had a comment on the pixel cell size.\n> > >\n> > > Apart from that, let me check with Laurent about the policy when it\n> > > comes to supporting sensor without an upstream driver.\n> > >\n> > > I would not be super happy of adding support to \"m00_f_ov8858\" as if\n> the\n> > > driver will be upstreamed the entity name should at least change.\n> >\n> > Yes, this is problematic.\n> >\n> > Looking at the driver, it comes from:\n> >\n> https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c#L2936\n> >\n> > This will likely have to be changed to get this merged upstream in the\n> > kernel.\n> >\n> > > As a middle term solution we can add support for \"m00_f_ov8858\" with a\n> > > todo note to move to a more canonical \"ov8858\" name once the driver is\n> > > upstreamed, or at least submitted upstream.\n> >\n> > This is problematic too - there would be overlap - would we have to\n> > duplicate the camera sensor properties? Or perhaps support registering a\n> > camera sensor helper as mutliple names?\n> >\n> >\n> > This is the 'why' upstreaming the driver is 'almost' a requirement.\n> > Parts like that should be solved before integrating in libcamera\n> > ideally. Perhaps we can workaround it this time as it's hopefully\n> > manageable - but it means that we would then have libcamera 'supporting'\n> > the pinephone in one version, but then losing it's support when we\n> > suddenly change the names to match the upstream kernel until the\n> > pinephone developers update their kernel.\n>\n> Agreed. The driver should be upstreamed, and while we don't require\n> drivers to be merged upstream before support for them can be added to\n> libcamera, we do require the drivers to be on their way to upstream.\n> This means showing a reasonable effort to get it there, most likely with\n> a patch posted to the linux-media mailing list.\n>\n> Give that the entity name will need to change when upstreaming, one way\n> forward would be to patch the pinephone kernel driver with the correct\n> entity name already, and use it in libcamera right away.\n>\n> > All a bit messy ... :S\n> >\n> > > > On Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth wrote:\n> > > >\n> > > > > > please pardon me for being pedantic, but I would:\n> > > > > > libcamera: Add support for ov8858 sensor\n> > > > > >\n> > > > > > instead of:\n> > > > > > ipa: libcamera: add metadata for the ov8858 sensor\n> > > > >\n> > > > > Happy to make the change.\n> > > > >\n> > > > > > The sensor doesn't seem to be supported mainline :(...\n> > > > > > Let's start from the first point: where does this driver lives ?\n> What\n> > > > > > kernel are you using ?\n> > > > > I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n> > > > >\n> > > > > It looks like my package manager is picking up the Manjaro kernel\n> from\n> > > > >\n> https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config\n> ,\n> > > > > which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no\n> > > > > 8858-related changes. This ultimately pulls the driver from\n> > > > >\n> https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> > > > > .\n> > > > >\n> > > > > Looks like this has been in and out of Torvalds' peripheral view,\n> e.g.:\n> > > > > https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n> > > > >\n> > > > > If you think that it would be valuable to try and mainline this,\n> I'd be\n> > > > > interested to do so once I can get libcamera working on my\n> hardware, though\n> > > > > I'll likely need some guidance in the process, since I'll be\n> learning a lot.\n> > > > >\n> > > > > > Nice this matches the CCS defined linear gain model.\n> > > > > > However the sensor allows to select two formats for the analogue\n> gain\n> > > > > > the \"real gain\" format and \"sensor gain\" format. The selection\n> is made by\n> > > > > > register 0x3503[2] and it would be nice to validate that the\n> driver\n> > > > > > uses the format that you expect.\n> > > > >\n> > > > > Frankly, I'm not sure what you mean by this, but I'll try to find\n> out from\n> > > > > the driver above and verify.\n> > > > >\n> > > > > > Knowing what driver you're using is relevant, in example, as the\n> > > > > > OV5688 sensor computes exposure in 1/16 of line length. This is\n> not\n> > > > > > what libcamera expects, and sensor drivers are expected to\n> express the\n> > > > > > V4L2_CID_EXPOSURE control in line units.\n> > > > >\n> > > > > > From Documentation/sensor_driver_requirements.rst\n> > > > >\n> > > > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control,\n> libcamera\n> > > > > > requires it to be expressed as a number of image lines. Camera\n> sensor drivers\n> > > > > > that do not comply with this requirement will need to be adapted\n> or will produce\n> > > > > > incorrect results.\n> > > > >\n> > > > > Let me read the driver and get back to you.\n> > > > >\n> > > > >\n> > > > > On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi \n> wrote:\n> > > > >\n> > > > >> Hi Nicholas,\n> > > > >>\n> > > > >> please pardon me for being pedantic, but I would:\n> > > > >> libcamera: Add support for ov8858 sensor\n> > > > >>\n> > > > >> instead of:\n> > > > >> ipa: libcamera: add metadata for the ov8858 sensor\n> > > > >>\n> > > > >> \"metadata\" is an overloaded already term and usually refers to the\n> > > > >> properties associated to a captured frame (the term comes from the\n> > > > >> Android lingo, but we use it too).\n> > > > >>\n> > > > >> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> > > > >> libcamera-devel wrote:\n> > > > >> > From: Nicholas Roth \n> > > > >> >\n> > > > >> > Currently, libcamera does not have information for the ov8858\n> sensor\n> > > > >> > used in the PinePhone Pro, a phone designed to run Linux.\n> > > > >> >\n> > > > >> > This commit adds metadata, especially that sensor gain is\n> reported and\n> > > > >> > set in 1/16 discrete increments.\n> > > > >> >\n> > > > >> > For more information, see \"5.8 manual exposure compensation/\n> manual\n> > > > >> > gain compensation\" in [0].\n> > > > >> >\n> > > > >> > [0]\n> http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n> > > > >>\n> > > > >> The sensor doesn't seem to be supported mainline :(\n> > > > >> libcamera has a policy that requires supported components to be\n> > > > >> upstream or at least on their way to upstream (ie posted to the\n> > > > >> linux-media mailing list). The policy is there because different\n> > > > >> downstream driver implementations might behave differently one\n> from\n> > > > >> the other, making it impossible for libcamera to support the part\n> > > > >> fully. The policy is a strict requirement for ISPs, I guess we're\n> a\n> > > > >> bit more elastic when it comes to sensor. Howver knowing what\n> driver\n> > > > >> you are using, where it lives, and if there's any plan to\n> upstream it\n> > > > >> would be great.\n> > > > >>\n> > > > >> Let's start from the first point: where does this driver lives ?\n> What\n> > > > >> kernel are you using ?\n> > > > >>\n> > > > >> Knowing what driver you're using is relevant, in example, as the\n> > > > >> OV5688 sensor computes exposure in 1/16 of line length. This is\n> not\n> > > > >> what libcamera expects, and sensor drivers are expected to\n> express the\n> > > > >> V4L2_CID_EXPOSURE control in line units.\n> > > > >>\n> > > > >> From Documentation/sensor_driver_requirements.rst\n> > > > >>\n> > > > >> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control,\n> libcamera\n> > > > >> requires it to be expressed as a number of image lines. Camera\n> sensor drivers\n> > > > >> that do not comply with this requirement will need to be adapted\n> or will produce\n> > > > >> incorrect results.\n> > > > >>\n> > > > >> > Signed-off-by: Nicholas Roth \n> > > > >> > ---\n> > > > >> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> > > > >> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> > > > >> > 2 files changed, 25 insertions(+)\n> > > > >> >\n> > > > >> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n> b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > >> > index 35056bec..f2040cbd 100644\n> > > > >> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > >> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > >> > @@ -476,6 +476,17 @@ public:\n> > > > >> > };\n> > > > >> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\",\n> CameraSensorHelperOv5693)\n> > > > >> >\n> > > > >> > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> > > > >> > +{\n> > > > >> > +public:\n> > > > >> > + CameraSensorHelperOv8858()\n> > > > >> > + {\n> > > > >> > + gainType_ = AnalogueGainLinear;\n> > > > >> > + gainConstants_.linear = { 1, 0, 0, 16 };\n> > > > >> > + }\n> > > > >> > +};\n> > > > >>\n> > > > >> Nice this matches the CCS defined linear gain model.\n> > > > >> However the sensor allows to select two formats for the analogue\n> gain\n> > > > >> the \"real gain\" format and \"sensor gain\" format. The selection is\n> made by\n> > > > >> register 0x3503[2] and it would be nice to validate that the\n> driver\n> > > > >> uses the format that you expect.\n> > > > >>\n> > > > >> > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\",\n> CameraSensorHelperOv8858)\n> > > > >>\n> > > > >> Ah! that's a weird name for the sensor entity :)\n> > > > >>\n> > > > >> > +\n> > > > >> > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > > > >> > {\n> > > > >> > public:\n> > > > >> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> b/src/libcamera/camera_sensor_properties.cpp\n> > > > >> > index e5f27f06..d0757c15 100644\n> > > > >> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > > >> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > > >> > @@ -146,6 +146,20 @@ const CameraSensorProperties\n> *CameraSensorProperties::get(const std::string &sen\n> > > > >> > */\n> > > > >> > },\n> > > > >> > } },\n> > > > >> > + { \"m00_f_ov8858\", {\n> > > > >> > + .unitCellSize = { 1200, 1200 },\n> > > > >>\n> > > > >> I read 1.12 um x 1.12 um\n> > > > >>\n> > > > >> > + .testPatternModes = {\n> > > > >> > + {\n> controls::draft::TestPatternModeOff, 0 },\n> > > > >> > + {\n> controls::draft::TestPatternModeColorBars, 1 },\n> > > > >> > + /*\n> > > > >> > + * No best corresponding test\n> pattern for:\n> > > > >> > + * 1: \"Vertical Color Bar Type 1\",\n> > > > >> > + * 2: \"Vertical Color Bar Type 2\",\n> > > > >> > + * 3: \"Vertical Color Bar Type 3\",\n> > > > >> > + * 4: \"Vertical Color Bar Type 4\"\n> > > > >> > + */\n> > > > >> > + },\n> > > > >> > + } },\n> > > > >> > { \"ov8865\", {\n> > > > >> > .unitCellSize = { 1400, 1400 },\n> > > > >> > .testPatternModes = {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 CB82ABDB16\n\tfor ;\n\tSun, 30 Oct 2022 22:15:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 347F463027;\n\tSun, 30 Oct 2022 23:15:57 +0100 (CET)","from mail-io1-xd36.google.com (mail-io1-xd36.google.com\n\t[IPv6:2607:f8b0:4864:20::d36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C71161F47\n\tfor ;\n\tSun, 30 Oct 2022 23:15:55 +0100 (CET)","by mail-io1-xd36.google.com with SMTP id p141so8561901iod.6\n\tfor ;\n\tSun, 30 Oct 2022 15:15:55 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667168157;\n\tbh=W6pFOMRAoVlY1NBN1A/jVN/iN6BI2AUb152cUniuC4s=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zty9zVHTttNnfJzESmDNQjZzuWywJn7Js/kqqjlQm1nyc/fEu/ILo8WwnzSXddTxd\n\tEXanXuh1GsFnQxTh6YMF0cQthbCnejpu8NTC83MMk0RMzlpgjlEhCGAszAMn6XfIf5\n\tL1O43Lr3MtOwnqFmix47Vt7AMvT6vL0z8ptMoMgWQFbJZZdLwD36ylTHpklPVMxQ5q\n\tQwsVXx5oWB4RzWunJbv4uHYxGoZ3x8272D9QsXubcH+5fJgapVyp1ia8hK8Unh2Jk9\n\tdTO35ER4kSvNcSPq21ySIjKc+eIflem4ib7gwUZ4jXOFMsnZV56xPKwl7SBlJX8la1\n\txt2VJjmlesdGQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=rothemail-net.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=1VMc3XV9ToVPgvg5TIELgS0lNLah5uqra/KfzlD/jUc=;\n\tb=CEkc8J70yC4uAzf0VA9Va4QYDFX8og+Ta4SnN9tMV1t5L92JEofzGP56uKH+qhKNlg\n\tLyLBM5MBCMbjk4xo+aIYtrSSfkFvPjX+LHUz9pT8NO3vYWBQidDN72+eCfrSQvIysofp\n\tvAOPiOUqIK6RA8NI1swXUi5WvAjPd1dfU39+kBVdU55R23kwX25e3XIV0wKn0sl2SYt1\n\teYJtE3Y6+OpF7fvqh3G5yTqC3famiRM/VuAbyT+a/FDq7Anp0bvO72W6N1LRr+IivlJK\n\tVsV5D4TxvN4/zo9HTmUEGgV8w3Xri7ZQ+nfepuQJnWYVVsYSk+sZPGtC76+/3JUmoN01\n\t/2gw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=rothemail-net.20210112.gappssmtp.com\n\theader.i=@rothemail-net.20210112.gappssmtp.com header.b=\"CEkc8J70\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=1VMc3XV9ToVPgvg5TIELgS0lNLah5uqra/KfzlD/jUc=;\n\tb=F8f8B5DTkc76iYL7wEoJLGRV16Sp7UaS2m/BescAl6ltv6ChZ2ZGb8qi+zi29jh/E4\n\tkeE0/vtf9uHsvPM9K+XOXJt2+fFdnXM8h+t1bDCewsuqi+xorQqrS6iztmg92xKsL98w\n\tb3F3lmPFJWIHlxxqXMESWwO+M9Bfx3psS/bvJmfJ5PiX9GBq81SyUVsn40XCO7vdohac\n\txCiuG2DDFzAkAGd0CCfJitmvBOfdZcTDK8P6dDDZRnZbavXF9tEtZ21lfbeDVmFIwMzM\n\tDjYmVQbrpJ6AUdUUZpKTXkklJ26yp5Z38O6xklCTQ8qTp9vzPidW7lHW8j9E2qqNZhig\n\t1lGg==","X-Gm-Message-State":"ACrzQf2lCcL/jTXbbqfLvd9JZaWdLG2rd6gr3MOx4uNxOXY9uh9SxDax\n\t2FJKznp/+D/s+Ao3LMZ0yIX91WWdu90fWMX4jiGPOw==","X-Google-Smtp-Source":"AMsMyM5DKcx3xT/lzO43nTF+qi5GbgQMSlflfbEXdOyKDTVxCa6grAaCKU4uhaArFXja/0zZTIh42GT6uwXyxKxzwko=","X-Received":"by 2002:a02:9715:0:b0:375:13ba:927 with SMTP id\n\tx21-20020a029715000000b0037513ba0927mr5789600jai.127.1667168153500;\n\tSun, 30 Oct 2022 15:15:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t\n\t\n\t<20221028081427.s25wdjmmqh5ikjem@uno.localdomain>\n\t<166695273851.1769154.5384280975536306961@Monstersaurus>\n\t","In-Reply-To":"","Date":"Sun, 30 Oct 2022 17:15:42 -0500","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000092113105ec47d52a\"","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Nicholas Roth via libcamera-devel ","Reply-To":"Nicholas Roth ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25693,"web_url":"https://patchwork.libcamera.org/comment/25693/","msgid":"<20221031091003.pqmzo4pq6smyylij@uno.localdomain>","date":"2022-10-31T09:10:03","subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicholas\n\nOn Sun, Oct 30, 2022 at 05:15:42PM -0500, Nicholas Roth wrote:\n> > Agreed. The driver should be upstreamed\n>\n> I'm going to send an initial version to linux-media today for comment. I'll\n> include a link to their mailing list with the next set of changes.\n>\n> > Give that the entity name will need to change when upstreaming, one way\n> > forward would be to patch the pinephone kernel driver with the correct\n> > entity name already, and use it in libcamera right away.\n>\n> TL;DR: Media node names do not seem to be standardized and various formats\n> exist. I suggest adding an `else` case to [6] with an additional regex for\n> the ov8858 driver for maximum compatibility with non-mainline ov8858\n> implementations while we sort out getting this upstreamed.\n> --\n>\n> Upon reading the driver [0] more closely, it looks like \"m00_f_ov8858\" is\n> just the *subdevice name *(set to\n> \"m{module_index:02d}_{facing}_{OV8858_NAME}\" on :2936), not the driver\n\nmodule_index is obtained parsing a vendor property that doesn't exist\nin mainline and should not be reported in the subdevice name. When you\nwill upstream the driver you'll be asked to remove it.\n\n> name (set to OV8858_NAME == \"ov8858\" on :3004) or sensor name (set to\n> OV8858_NAME == \"ov8858\" on :1778).\n\n1778 is the callback for a custom ioctl that does not exist upstream.\nYou will have to remove that part when upstreaming.\n\nThe driver name is set through [1] and used to identify the driver by\nthe kernel's core. It's not something exposed by the media controller\napi, ie not something we care about when matching on the subdevice\nentity name\n\n[1] https://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/ov8858.c#L3003\n\n>\n> Reading the kernel docs [3], it looks like this is required to be unique:\n> \"Afterwards you need to initialize sd->name with a unique name and set the\n> module owner. This is done for you if you use the i2c helper functions.\"\n>\n> The subdevice name *defaults* to the driver name in [4] when\n> v4l2_i2c_subdev_init() invokes v4l2_i2c_subdev_set_name(sd, client, NULL,\n> NULL), but this is not guaranteed to be the driver name if I understand\n> correctly. In fact, this appears to solve for the case where there may be\n\ndriver name != subdevice name\n\nThe subdevice name (or rather, the media entity name) is exposed by\nthe media controller API (iow it's the one that you see using\nmedia-ctl -p, and it's the one libcamera matches on).\n\nThe driver name is a kernel-wide concept. It is used to construct the\nsubdevice name, but as you have noticed it alone is not sufficient to\nguarantee it is unique in a system, hence...\n\n\n> multiple ov8858s installed, which the default implementation would not.\n\n... read\n\nhttps://github.com/torvalds/linux/blob/master/drivers/media/v4l2-core/v4l2-i2c.c#L31\n\n snprintf(sd->name, sizeof(sd->name), \"%s%s %d-%04x\", devname, postfix,\n i2c_adapter_id(client->adapter), client->addr);\n\nThe subdevice name is constructed by adding to the driver name the\ndevice's i2c bus addresses, which guarantees the resulting subdevice\nname is unique.\n\n> Though there is some ambiguity in the phrase \"unique name,\" v4l2-subdev.h\n> [5] seems to suggest that these must be unique among device instances, not\n> drivers, since `struct v4l2_subdev` corresponds to a devnode and also\n> specifies \"@name: Name of the sub-device. Please notice that the name must\n> be unique.\"\n>\n> Meanwhile, both Megi's imx258 fork [1] and the mainline imx258 driver [2]\n> use the default implementation, so we do not have this issue using\n> libcamera with an imx258.\n>\n> ***libcamera subdevice name handling (regex guess)***\n> Currently, libcamera uses a regex to guess the model name from the\n> subdevice name in v4l2_subdevice.cpp V4L2Subdevice::model() [6]. Instead,\n>\n> ***Proposed Solution***:\n> I suggest adding an `else` case to [6] with an additional regex for the\n> ov8858 driver for maximum compatibility with non-mainline ov8858\n> implementations while we sort out getting this upstreamed.\n>\n\nWhen you will upstream the driver all the vendor stupidities (the\nmodule_index and the device facing information in the name, the custom\nioctls etc) will have to be removed. Same for the the subdevice name:\nyou'll be asked to simply use v4l2_i2c_subdev_init() and let it deal\nwith names for you. You will need -strong- arguments to convince\nupstream you have to use a custom name there.\n\nNow, I don't want to put you in a bad position. Your downstream uses a\ndriver which needs to be changed, and at the same time you're trying to\nupstream one version of the same driver, and libcamera should work\nideally with both until things do not settle.\n\nOfc if you can patch your BSP earlier than anything else, and have\nyour system use the same name as the one we expect to be upstream, the\nproblem is solved and we can use \"ov8858\" directly here.\n\nThe ideal path for this would then be for you to succesfully upstream the\ndriver and have your BSP replace the vendor's version with your\nupstream one, provided it is functionally equal. Mainline kernel code\n+ mainline libcamera code == fully upstream support == victory.\n\nNow, if you get any resistence on changing your BSP (something tells\nme you will, I hope to be wrong), considering your CameraSensorHelper\nis fairly small, I would accept to have one version that matches on\n\"ov8858\" and one that matches on \"m00_f_ov8858\" for an interim period,\nwith a big todo note that it should be remove and with the idea to\nconverge on the upstream name. I don't like this but I hope it might\nmake things easier for you, if plan A of patching the BSP immediately\nfails.\n\n\n> 0:\n> https://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/ov8858.c\n> 1:\n> https://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/imx258.c\n> 2: https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c\n> 3: https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html\n> 4:\n> https://github.com/torvalds/linux/blob/master/drivers/media/v4l2-core/v4l2-i2c.c\n> 5: https://github.com/torvalds/linux/blob/master/include/media/v4l2-subdev.h\n> 6:\n> https://github.com/kbingham/libcamera/blob/master/src/libcamera/v4l2_subdevice.cpp\n>\n> On Sun, Oct 30, 2022 at 11:21 AM Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n> > Hello,\n> >\n> > On Fri, Oct 28, 2022 at 11:25:38AM +0100, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi via libcamera-devel (2022-10-28 09:14:27)\n> > > > Hi Nicholas,\n> > > > + Laurent question below\n> > > >\n> > > > On Thu, Oct 27, 2022 at 05:21:37PM -0500, Nicholas Roth wrote:\n> > > > > > Nice this matches the CCS defined linear gain model.\n> > > > > > However the sensor allows to select two formats for the analogue\n> > gain\n> > > > > > the \"real gain\" format and \"sensor gain\" format. The selection is\n> > made by\n> > > > > > register 0x3503[2] and it would be nice to validate that the driver\n> > > > > > uses the format that you expect.\n> > > > >\n> > > > > I think I understand this better after reading the driver. In the\n> > register\n> > > > > value settings, I see:\n> > > > > {0x3502, 0x40}, // exposure L\n> > > > > {0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain\n> > > > > {0x3505, 0x80}, // gain option\n> > > > >\n> > > > > The 0 low-nibbl indicates we're using \"real gain.\"\n> > > > >\n> > > > > According to the datasheet:\n> > > > > 0x3503[2]=0, gain[7:0] is real gain format,\n> > > > > where low 4 bits are fraction bits, for\n> > > > > example, 0x10 is 1x gain, 0x28 is 2.5x gain\n> > > > >\n> > > > > This tells me that we need to divide reported gain from the driver\n> > by 16 to\n> > > > > get a properly-scaled double-value gain, which these gain constants\n> > do.\n> > > >\n> > > > Here you go!\n> > > >\n> > > > Looking at the driver you're using I indeed see 3503[2]=0 in all modes\n> > > >\n> > > > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control,\n> > libcamera\n> > > > > > requires it to be expressed as a number of image lines. Camera\n> > sensor drivers\n> > > > > > that do not comply with this requirement will need to be adapted\n> > or will produce\n> > > > > > incorrect results.\n> > > > >\n> > > > > Looks like the driver multiples inputs x16 to convert to 1/16 line\n> > length\n> > > > > before writing to the register...\n> > > > >\n> > > > > case V4L2_CID_EXPOSURE:\n> > > > > /* 4 least significant bits of expsoure are fractional part */\n> > > > > ret = ov8858_write_reg(ov8858->client,\n> > > > > OV8858_REG_EXPOSURE,\n> > > > > OV8858_REG_VALUE_24BIT,\n> > > > > ctrl->val << 4);\n> > > >\n> > > > Great, it means that towards userspace the exposure is indeed in units\n> > > > of 1 line.\n> > > >\n> > > > > Let me know if this looks good, and what needs to happen to merge\n> > this!\n> > > >\n> > > > I think I had a comment on the pixel cell size.\n> > > >\n> > > > Apart from that, let me check with Laurent about the policy when it\n> > > > comes to supporting sensor without an upstream driver.\n> > > >\n> > > > I would not be super happy of adding support to \"m00_f_ov8858\" as if\n> > the\n> > > > driver will be upstreamed the entity name should at least change.\n> > >\n> > > Yes, this is problematic.\n> > >\n> > > Looking at the driver, it comes from:\n> > >\n> > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c#L2936\n> > >\n> > > This will likely have to be changed to get this merged upstream in the\n> > > kernel.\n> > >\n> > > > As a middle term solution we can add support for \"m00_f_ov8858\" with a\n> > > > todo note to move to a more canonical \"ov8858\" name once the driver is\n> > > > upstreamed, or at least submitted upstream.\n> > >\n> > > This is problematic too - there would be overlap - would we have to\n> > > duplicate the camera sensor properties? Or perhaps support registering a\n> > > camera sensor helper as mutliple names?\n> > >\n> > >\n> > > This is the 'why' upstreaming the driver is 'almost' a requirement.\n> > > Parts like that should be solved before integrating in libcamera\n> > > ideally. Perhaps we can workaround it this time as it's hopefully\n> > > manageable - but it means that we would then have libcamera 'supporting'\n> > > the pinephone in one version, but then losing it's support when we\n> > > suddenly change the names to match the upstream kernel until the\n> > > pinephone developers update their kernel.\n> >\n> > Agreed. The driver should be upstreamed, and while we don't require\n> > drivers to be merged upstream before support for them can be added to\n> > libcamera, we do require the drivers to be on their way to upstream.\n> > This means showing a reasonable effort to get it there, most likely with\n> > a patch posted to the linux-media mailing list.\n> >\n> > Give that the entity name will need to change when upstreaming, one way\n> > forward would be to patch the pinephone kernel driver with the correct\n> > entity name already, and use it in libcamera right away.\n> >\n> > > All a bit messy ... :S\n> > >\n> > > > > On Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth wrote:\n> > > > >\n> > > > > > > please pardon me for being pedantic, but I would:\n> > > > > > > libcamera: Add support for ov8858 sensor\n> > > > > > >\n> > > > > > > instead of:\n> > > > > > > ipa: libcamera: add metadata for the ov8858 sensor\n> > > > > >\n> > > > > > Happy to make the change.\n> > > > > >\n> > > > > > > The sensor doesn't seem to be supported mainline :(...\n> > > > > > > Let's start from the first point: where does this driver lives ?\n> > What\n> > > > > > > kernel are you using ?\n> > > > > > I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).\n> > > > > >\n> > > > > > It looks like my package manager is picking up the Manjaro kernel\n> > from\n> > > > > >\n> > https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config\n> > ,\n> > > > > > which sets \"CONFIG_VIDEO_OV8858=m\" in /config otherwise has no\n> > > > > > 8858-related changes. This ultimately pulls the driver from\n> > > > > >\n> > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c\n> > > > > > .\n> > > > > >\n> > > > > > Looks like this has been in and out of Torvalds' peripheral view,\n> > e.g.:\n> > > > > > https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html\n> > > > > >\n> > > > > > If you think that it would be valuable to try and mainline this,\n> > I'd be\n> > > > > > interested to do so once I can get libcamera working on my\n> > hardware, though\n> > > > > > I'll likely need some guidance in the process, since I'll be\n> > learning a lot.\n> > > > > >\n> > > > > > > Nice this matches the CCS defined linear gain model.\n> > > > > > > However the sensor allows to select two formats for the analogue\n> > gain\n> > > > > > > the \"real gain\" format and \"sensor gain\" format. The selection\n> > is made by\n> > > > > > > register 0x3503[2] and it would be nice to validate that the\n> > driver\n> > > > > > > uses the format that you expect.\n> > > > > >\n> > > > > > Frankly, I'm not sure what you mean by this, but I'll try to find\n> > out from\n> > > > > > the driver above and verify.\n> > > > > >\n> > > > > > > Knowing what driver you're using is relevant, in example, as the\n> > > > > > > OV5688 sensor computes exposure in 1/16 of line length. This is\n> > not\n> > > > > > > what libcamera expects, and sensor drivers are expected to\n> > express the\n> > > > > > > V4L2_CID_EXPOSURE control in line units.\n> > > > > >\n> > > > > > > From Documentation/sensor_driver_requirements.rst\n> > > > > >\n> > > > > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control,\n> > libcamera\n> > > > > > > requires it to be expressed as a number of image lines. Camera\n> > sensor drivers\n> > > > > > > that do not comply with this requirement will need to be adapted\n> > or will produce\n> > > > > > > incorrect results.\n> > > > > >\n> > > > > > Let me read the driver and get back to you.\n> > > > > >\n> > > > > >\n> > > > > > On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi \n> > wrote:\n> > > > > >\n> > > > > >> Hi Nicholas,\n> > > > > >>\n> > > > > >> please pardon me for being pedantic, but I would:\n> > > > > >> libcamera: Add support for ov8858 sensor\n> > > > > >>\n> > > > > >> instead of:\n> > > > > >> ipa: libcamera: add metadata for the ov8858 sensor\n> > > > > >>\n> > > > > >> \"metadata\" is an overloaded already term and usually refers to the\n> > > > > >> properties associated to a captured frame (the term comes from the\n> > > > > >> Android lingo, but we use it too).\n> > > > > >>\n> > > > > >> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via\n> > > > > >> libcamera-devel wrote:\n> > > > > >> > From: Nicholas Roth \n> > > > > >> >\n> > > > > >> > Currently, libcamera does not have information for the ov8858\n> > sensor\n> > > > > >> > used in the PinePhone Pro, a phone designed to run Linux.\n> > > > > >> >\n> > > > > >> > This commit adds metadata, especially that sensor gain is\n> > reported and\n> > > > > >> > set in 1/16 discrete increments.\n> > > > > >> >\n> > > > > >> > For more information, see \"5.8 manual exposure compensation/\n> > manual\n> > > > > >> > gain compensation\" in [0].\n> > > > > >> >\n> > > > > >> > [0]\n> > http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf\n> > > > > >>\n> > > > > >> The sensor doesn't seem to be supported mainline :(\n> > > > > >> libcamera has a policy that requires supported components to be\n> > > > > >> upstream or at least on their way to upstream (ie posted to the\n> > > > > >> linux-media mailing list). The policy is there because different\n> > > > > >> downstream driver implementations might behave differently one\n> > from\n> > > > > >> the other, making it impossible for libcamera to support the part\n> > > > > >> fully. The policy is a strict requirement for ISPs, I guess we're\n> > a\n> > > > > >> bit more elastic when it comes to sensor. Howver knowing what\n> > driver\n> > > > > >> you are using, where it lives, and if there's any plan to\n> > upstream it\n> > > > > >> would be great.\n> > > > > >>\n> > > > > >> Let's start from the first point: where does this driver lives ?\n> > What\n> > > > > >> kernel are you using ?\n> > > > > >>\n> > > > > >> Knowing what driver you're using is relevant, in example, as the\n> > > > > >> OV5688 sensor computes exposure in 1/16 of line length. This is\n> > not\n> > > > > >> what libcamera expects, and sensor drivers are expected to\n> > express the\n> > > > > >> V4L2_CID_EXPOSURE control in line units.\n> > > > > >>\n> > > > > >> From Documentation/sensor_driver_requirements.rst\n> > > > > >>\n> > > > > >> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control,\n> > libcamera\n> > > > > >> requires it to be expressed as a number of image lines. Camera\n> > sensor drivers\n> > > > > >> that do not comply with this requirement will need to be adapted\n> > or will produce\n> > > > > >> incorrect results.\n> > > > > >>\n> > > > > >> > Signed-off-by: Nicholas Roth \n> > > > > >> > ---\n> > > > > >> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++\n> > > > > >> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++\n> > > > > >> > 2 files changed, 25 insertions(+)\n> > > > > >> >\n> > > > > >> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp\n> > b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > > >> > index 35056bec..f2040cbd 100644\n> > > > > >> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > > >> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > > > > >> > @@ -476,6 +476,17 @@ public:\n> > > > > >> > };\n> > > > > >> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\",\n> > CameraSensorHelperOv5693)\n> > > > > >> >\n> > > > > >> > +class CameraSensorHelperOv8858 : public CameraSensorHelper\n> > > > > >> > +{\n> > > > > >> > +public:\n> > > > > >> > + CameraSensorHelperOv8858()\n> > > > > >> > + {\n> > > > > >> > + gainType_ = AnalogueGainLinear;\n> > > > > >> > + gainConstants_.linear = { 1, 0, 0, 16 };\n> > > > > >> > + }\n> > > > > >> > +};\n> > > > > >>\n> > > > > >> Nice this matches the CCS defined linear gain model.\n> > > > > >> However the sensor allows to select two formats for the analogue\n> > gain\n> > > > > >> the \"real gain\" format and \"sensor gain\" format. The selection is\n> > made by\n> > > > > >> register 0x3503[2] and it would be nice to validate that the\n> > driver\n> > > > > >> uses the format that you expect.\n> > > > > >>\n> > > > > >> > +REGISTER_CAMERA_SENSOR_HELPER(\"m00_f_ov8858\",\n> > CameraSensorHelperOv8858)\n> > > > > >>\n> > > > > >> Ah! that's a weird name for the sensor entity :)\n> > > > > >>\n> > > > > >> > +\n> > > > > >> > class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > > > > >> > {\n> > > > > >> > public:\n> > > > > >> > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> > b/src/libcamera/camera_sensor_properties.cpp\n> > > > > >> > index e5f27f06..d0757c15 100644\n> > > > > >> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > > > >> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > > > >> > @@ -146,6 +146,20 @@ const CameraSensorProperties\n> > *CameraSensorProperties::get(const std::string &sen\n> > > > > >> > */\n> > > > > >> > },\n> > > > > >> > } },\n> > > > > >> > + { \"m00_f_ov8858\", {\n> > > > > >> > + .unitCellSize = { 1200, 1200 },\n> > > > > >>\n> > > > > >> I read 1.12 um x 1.12 um\n> > > > > >>\n> > > > > >> > + .testPatternModes = {\n> > > > > >> > + {\n> > controls::draft::TestPatternModeOff, 0 },\n> > > > > >> > + {\n> > controls::draft::TestPatternModeColorBars, 1 },\n> > > > > >> > + /*\n> > > > > >> > + * No best corresponding test\n> > pattern for:\n> > > > > >> > + * 1: \"Vertical Color Bar Type 1\",\n> > > > > >> > + * 2: \"Vertical Color Bar Type 2\",\n> > > > > >> > + * 3: \"Vertical Color Bar Type 3\",\n> > > > > >> > + * 4: \"Vertical Color Bar Type 4\"\n> > > > > >> > + */\n> > > > > >> > + },\n> > > > > >> > + } },\n> > > > > >> > { \"ov8865\", {\n> > > > > >> > .unitCellSize = { 1400, 1400 },\n> > > > > >> > .testPatternModes = {\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 BE3A8BDB16\n\tfor ;\n\tMon, 31 Oct 2022 09:10:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 085DE63035;\n\tMon, 31 Oct 2022 10:10:08 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB64462FCA\n\tfor ;\n\tMon, 31 Oct 2022 10:10:05 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 97112240008;\n\tMon, 31 Oct 2022 09:10:04 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667207408;\n\tbh=wx5MARRz64bzEQ822Yyvb/UGxUTDD2WJEWYFtaotDmo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nMJ/MY1V19YQgMR2AmwFR8WPBkKfDfOwJWbTkg4qpd4Xuqiuu3Jdo0MOXR0JkasLC\n\tvBrcEhVEYl8Sk8zzqVSiBsAYpdyeiGG3nXAxeelU7ftC3bEtQcZVtMiqNkvTGUVMmw\n\t8AMgcOeTyQVfW5wg2hrvLCiPEw/9y1Rf5SH71nMf2Uf3INBJQptlJpHE+erY3cTZnu\n\tUCJ9fIfHHMxX6x4PGavVgNLbqMCGC0RRFYsL3elf6C/VDZEWu1ZQdgNbs9aXWAv8Sh\n\tCLPi/J2/gdIRQdipsbgSXw21MazcmdG7rqWN4IUPFKTp4gFQoh86hrD62VmKUp0FEi\n\tvVxktAeJt8Mvg==","Date":"Mon, 31 Oct 2022 10:10:03 +0100","To":"Nicholas Roth ","Message-ID":"<20221031091003.pqmzo4pq6smyylij@uno.localdomain>","References":"<20221027055515.321791-1-nicholas@rothemail.net>\n\t<20221027055515.321791-5-nicholas@rothemail.net>\n\t<20221027164108.5326mbcdzh4yoxlb@uno.localdomain>\n\t\n\t\n\t<20221028081427.s25wdjmmqh5ikjem@uno.localdomain>\n\t<166695273851.1769154.5384280975536306961@Monstersaurus>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata\n\tfor the ov8858 sensor","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]