[{"id":26261,"web_url":"https://patchwork.libcamera.org/comment/26261/","msgid":"<167413338494.42371.14345994623895785637@Monstersaurus>","date":"2023-01-19T13:03:04","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> From: Nick Hollinghurst \n> \n> The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,\n> IR block/pass) by modifying the media entity name string. So add duplicate\n> entries for each variant.\n\nThe duplication here is a bit of a pain. Are there any sensor properties\nwe might register that would change in each of these variants?\n\nOr should we make the CameraSensorProperties class more intelligent so\nit just does the lookup on the sensor only. Of course ideally we should\nbe able to identify the sensor and the variant separately from the\nkernel - which might not be possible right now - in which case, given\nhow little information is actually duplicated here I'd be fine with this\nfor now.\n\n\nReviewed-by: Kieran Bingham \n\n> \n> Signed-off-by: Nick Hollinghurst \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> 1 file changed, 16 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> index c3c2caced906..3afd500ea3be 100644\n> --- a/src/libcamera/camera_sensor_properties.cpp\n> +++ b/src/libcamera/camera_sensor_properties.cpp\n> @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> */\n> },\n> } },\n> + { \"imx708\", {\n> + .unitCellSize = { 1400, 1400 },\n> + .testPatternModes = {},\n> + } },\n> + { \"imx708_noir\", {\n> + .unitCellSize = { 1400, 1400 },\n> + .testPatternModes = {},\n> + } },\n> + { \"imx708_wide\", {\n> + .unitCellSize = { 1400, 1400 },\n> + .testPatternModes = {},\n> + } },\n> + { \"imx708_wide_noir\", {\n> + .unitCellSize = { 1400, 1400 },\n> + .testPatternModes = {},\n> + } },\n> { \"ov2740\", {\n> .unitCellSize = { 1400, 1400 },\n> .testPatternModes = {\n> -- \n> 2.25.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 DC9D7C3240\n\tfor ;\n\tThu, 19 Jan 2023 13:03:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 663D7625E4;\n\tThu, 19 Jan 2023 14:03:08 +0100 (CET)","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 DA42061EFE\n\tfor ;\n\tThu, 19 Jan 2023 14:03:07 +0100 (CET)","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 713D17EC;\n\tThu, 19 Jan 2023 14:03:07 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674133388;\n\tbh=2FNSDzE2kAtMDdU9ul3qjaH/QLf4dQ+Cb+Ei/zB5eys=;\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=lS8Ke5mmacLunfSg2jMsutzRVIWstddQUIcpjjHRghReYCb4s3eq/J/1t/HE/9rq7\n\tQap9cgc0KqGKXgGR5XDx673E1c16IigABewAiUtBH1PuyJRUVLOICEpl6CZO80O61P\n\tblJPzFBzM3+a8qi3drN+R1sjIb7adYTUMfTcUjBLSUAyADslebmnaw3FFOoAgBdr8s\n\tAdagamKLOQpwSNprD2k0V+coTcp6J1tyRLWKxYbxvEvZ2d2Eba050jJlfpyhxjLsdT\n\t1f4qUTwZjjYNHEjOJPx19lafQkGsGsd5gRRE7XOsfgsRGWXyDqrxctvXHp+C8vmL+M\n\tzbzkYBDeGKCPQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674133387;\n\tbh=2FNSDzE2kAtMDdU9ul3qjaH/QLf4dQ+Cb+Ei/zB5eys=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=TmMgMmxM78tfYIMu155dunq7B44lf3R9XnWMIpEwGgJkdkaY+2bJzMXrSqH8XmLuW\n\t6zd0vH9dLZjkiaPNBANQXfrRoajOhS0mKzQA/yTB2GQ4tAVRRvGbFDE9zb9Fwl6zzE\n\tpttbRIKeXgCblEJInJJq42Y3BkSW25SkIfcCuyQM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TmMgMmxM\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230119104544.9456-14-naush@raspberrypi.com>","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 19 Jan 2023 13:03:04 +0000","Message-ID":"<167413338494.42371.14345994623895785637@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Nick Hollinghurst ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26306,"web_url":"https://patchwork.libcamera.org/comment/26306/","msgid":"","date":"2023-01-22T18:58:16","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> > From: Nick Hollinghurst \n> > \n> > The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,\n> > IR block/pass) by modifying the media entity name string. So add duplicate\n> > entries for each variant.\n> \n> The duplication here is a bit of a pain. Are there any sensor properties\n> we might register that would change in each of these variants?\n> \n> Or should we make the CameraSensorProperties class more intelligent so\n> it just does the lookup on the sensor only. Of course ideally we should\n> be able to identify the sensor and the variant separately from the\n> kernel - which might not be possible right now - in which case, given\n> how little information is actually duplicated here I'd be fine with this\n> for now.\n\nI'm concerned by this too. The kernel shouldn't report different entity\nnames for different lenses. There should thus be a single entry in the\ncamera sensors properties table, and lens information should be reported\nseparately.\n\n> Reviewed-by: Kieran Bingham \n> \n> > Signed-off-by: Nick Hollinghurst \n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> > 1 file changed, 16 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > index c3c2caced906..3afd500ea3be 100644\n> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > */\n> > },\n> > } },\n> > + { \"imx708\", {\n> > + .unitCellSize = { 1400, 1400 },\n> > + .testPatternModes = {},\n> > + } },\n> > + { \"imx708_noir\", {\n> > + .unitCellSize = { 1400, 1400 },\n> > + .testPatternModes = {},\n> > + } },\n> > + { \"imx708_wide\", {\n> > + .unitCellSize = { 1400, 1400 },\n> > + .testPatternModes = {},\n> > + } },\n> > + { \"imx708_wide_noir\", {\n> > + .unitCellSize = { 1400, 1400 },\n> > + .testPatternModes = {},\n> > + } },\n> > { \"ov2740\", {\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 48A09BDB1D\n\tfor ;\n\tSun, 22 Jan 2023 18:58:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63FE4625E4;\n\tSun, 22 Jan 2023 19:58:20 +0100 (CET)","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 0AD53625DD\n\tfor ;\n\tSun, 22 Jan 2023 19:58:19 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5595B308;\n\tSun, 22 Jan 2023 19:58:18 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674413900;\n\tbh=fAamJVeI5li11whY7JURAQgLBD/6TVyRHmcOS+4vbqw=;\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=MdGXV2LBxOW8U9c4UT+1SLA45sK5ucczdWur+PpBoqpnhmdnBL1udPD0byeOW6vla\n\tG422mT8AiFLqlbIVszFRsda7A2dIiANZogbi83UlR+45Gcg13E6Fw/7TzZCLuQHUIj\n\tcTEJ9SK2R8qBfI4DbryF1pkKjySfZMQSo5RbBD607pMo3bvD9mR7ML5xBUKtxwfstW\n\tzCnKOM3Acj2piWmAx726tGJk79UHxg6qo4asGkxCSBzQ2Ocj+zAujT7aKn87k2k+TZ\n\t7/CiQlaAO7ZlQsdg4VOKl5qgMkFt4tLwiBERI1Soi32dT1/OZK2S8awSXCej7UvEwL\n\tqnvicrU90GHgQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674413898;\n\tbh=fAamJVeI5li11whY7JURAQgLBD/6TVyRHmcOS+4vbqw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mizRmQFVHs4Jv9WQ4Y1lAQ2B79bBeXrMbJd4uII4uNgccdUnoc+Qd5YfENlQQSB8k\n\twLfAlmLRsfBBN4ftH9ZAU5fkYEXaWvGZ/4QGnSYiykcpxKtUv6H8Lj+ssLDhIG5+Cb\n\tdiE78dNaria1mo5jdPRExWAVsVdlRXpcqyorARCw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mizRmQFV\"; dkim-atps=neutral","Date":"Sun, 22 Jan 2023 20:58:16 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<167413338494.42371.14345994623895785637@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26321,"web_url":"https://patchwork.libcamera.org/comment/26321/","msgid":"","date":"2023-01-23T09:07:53","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\nOn Sun, 22 Jan 2023 at 18:58, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via\n> libcamera-devel wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> > > From: Nick Hollinghurst \n> > >\n> > > The IMX708 sensor driver advertises its module variants (narrow/wide\n> angle lens,\n> > > IR block/pass) by modifying the media entity name string. So add\n> duplicate\n> > > entries for each variant.\n> >\n> > The duplication here is a bit of a pain. Are there any sensor properties\n> > we might register that would change in each of these variants?\n> >\n> > Or should we make the CameraSensorProperties class more intelligent so\n> > it just does the lookup on the sensor only. Of course ideally we should\n> > be able to identify the sensor and the variant separately from the\n> > kernel - which might not be possible right now - in which case, given\n> > how little information is actually duplicated here I'd be fine with this\n> > for now.\n>\n> I'm concerned by this too. The kernel shouldn't report different entity\n> names for different lenses. There should thus be a single entry in the\n> camera sensors properties table, and lens information should be reported\n> separately.\n>\n\nI agree this is not the tidiest thing to do. Right now, we have no other\nway of\ngetting the variant details from the kernel driver into userland. I believe\n@Dave Stevenson raised this topic at the last Embedded Linux Conference,\nbut no\nconsensus was reached. Once we have a mechanism in the kernel to report\nlens\nproperties, we will remove this workaround. Would a \\todo suffice for now?\n\nRegards,\nNaush\n\n\n> > Reviewed-by: Kieran Bingham \n> >\n> > > Signed-off-by: Nick Hollinghurst \n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> > > 1 file changed, 16 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> b/src/libcamera/camera_sensor_properties.cpp\n> > > index c3c2caced906..3afd500ea3be 100644\n> > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > @@ -123,6 +123,22 @@ const CameraSensorProperties\n> *CameraSensorProperties::get(const std::string &sen\n> > > */\n> > > },\n> > > } },\n> > > + { \"imx708\", {\n> > > + .unitCellSize = { 1400, 1400 },\n> > > + .testPatternModes = {},\n> > > + } },\n> > > + { \"imx708_noir\", {\n> > > + .unitCellSize = { 1400, 1400 },\n> > > + .testPatternModes = {},\n> > > + } },\n> > > + { \"imx708_wide\", {\n> > > + .unitCellSize = { 1400, 1400 },\n> > > + .testPatternModes = {},\n> > > + } },\n> > > + { \"imx708_wide_noir\", {\n> > > + .unitCellSize = { 1400, 1400 },\n> > > + .testPatternModes = {},\n> > > + } },\n> > > { \"ov2740\", {\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 F0AE1BEFBE\n\tfor ;\n\tMon, 23 Jan 2023 09:08:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50C5D625DF;\n\tMon, 23 Jan 2023 10:08:11 +0100 (CET)","from mail-yw1-x1134.google.com (mail-yw1-x1134.google.com\n\t[IPv6:2607:f8b0:4864:20::1134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB79B603C0\n\tfor ;\n\tMon, 23 Jan 2023 10:08:09 +0100 (CET)","by mail-yw1-x1134.google.com with SMTP id\n\t00721157ae682-4b718cab0e4so162564247b3.9\n\tfor ;\n\tMon, 23 Jan 2023 01:08:09 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674464891;\n\tbh=qpXYXf1N+qwk1VWn8xOjkjBX5HrhnM+53wBK8P/8TpA=;\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=gPlTXvJNxA0S+NEH6OmngRwm7lS7b8xyX6lC4gkLJEQNxi8J0pKjgXAT+ttaWoIfR\n\tlgNg8YZ6h3FE7wLASsWFturWhD1FL/E92IpRrk+VUJ37CaIXWeYbanbHpr71TCsvRF\n\tp2bAFGs/EoYQHUS9uqRPrQ5X0I9ApflgIeOsXwAZ5yRr3XLppH0RByV09A+s2DpB2b\n\tXb18i/6sfbt615CPvMGPDVpQys64gwhxzBmI4J4JOhwz2PYvO3AXAPP9tcTX+EPBpV\n\tfzV6KSKK3/0rkthY/TlWR0jMK/par2cUb0kjXQiExcg/iY00no2vcR9CfPv+Aq961i\n\tkSShQ6v9tUXjw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=HlYrZvd8J+thYrQ7foMA224qNai8I1Yxae92TfdyqM4=;\n\tb=oiVkU9HJfd/fEhCTKHcc+TBb3JB2ZoDrjeB0HG+8O8lmJEtFhdr0g0OirWzZcPRzft\n\t8tn4a8QdzxTiCrSlxO8U6nvr8qSv7mjnY0SpVnnlmfFc8mkZ0FEfa68XH3Mb6YfFPG24\n\tAcUStuijdAGDVmqvlQrWq8LNekqjtj2pwCzRzE98kmBDgFc9zw52NgCbRs6X4lsBh04n\n\tlzwt2AycgLHWi863Ii5NeOX+eHykWSjXpgqLbYmb2xSjWZbaWdujGQGgjwhGMHQyyK31\n\taUhlMI7peKHfprqOO7BfBR6Y+aheLkuhfcWIeocXJYogMitCvdrCXZLDYraGh8YUPj5S\n\tTiKw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"oiVkU9HJ\"; dkim-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=HlYrZvd8J+thYrQ7foMA224qNai8I1Yxae92TfdyqM4=;\n\tb=EIuFrnStctaza9DTcjU4Exc+53po+9CmER1N4Hber73X2Hf4olLXLszpUy/bACIt45\n\t+oPub70yAJuD9iR4gwy/vx54oJOXeOkIxy/tGQ2GerJCBdKBcjCQWXWNL5bB5TyndDp4\n\tPGFbR9zUzD0oy7ORLTS5ZkCnTHE54DlcwZCEK9O9QQhYbS5DaIchLhelXzKPm1sxUhWJ\n\tltmB6zn0f3d0agJ15etKw/Lh7fR1GGZHDb8bV1Icy/RhsrFtL8BrSl+GVPbuQ2l66qbF\n\tO4ya5jO/b8pEZpdkOAbfda4xH1aVaPgHkZ0fsR+x0r2emzYNs9GId4B+CnJBf/beslC1\n\tMtsg==","X-Gm-Message-State":"AFqh2kqqJsaJnTLfCNKMvoC7tAk9tbJ8TfIFSlYTNPhOm1c0GtycfVKJ\n\ta24cvtpbMZjvJFlutn8vHv6O7CFH6gpoFf+4nU6gnQ==","X-Google-Smtp-Source":"AMrXdXuQX+07idE7WW8PcIszYzQzXr7uXEjLPANG3PNH3cRHo6NZOUf04LXijb3v1LqkPXK8XTnL5IP8Pm8LByXiOCU=","X-Received":"by 2002:a05:690c:852:b0:4d7:eb11:6bf7 with SMTP id\n\tbz18-20020a05690c085200b004d7eb116bf7mr2766621ywb.235.1674464888576;\n\tMon, 23 Jan 2023 01:08:08 -0800 (PST)","MIME-Version":"1.0","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>\n\t","In-Reply-To":"","Date":"Mon, 23 Jan 2023 09:07:53 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000df7a0d05f2eabc59\"","Subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26326,"web_url":"https://patchwork.libcamera.org/comment/26326/","msgid":"","date":"2023-01-23T10:01:16","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:\n> On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:\n> > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> > > > From: Nick Hollinghurst \n> > > >\n> > > > The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,\n> > > > IR block/pass) by modifying the media entity name string. So add duplicate\n> > > > entries for each variant.\n> > >\n> > > The duplication here is a bit of a pain. Are there any sensor properties\n> > > we might register that would change in each of these variants?\n> > >\n> > > Or should we make the CameraSensorProperties class more intelligent so\n> > > it just does the lookup on the sensor only. Of course ideally we should\n> > > be able to identify the sensor and the variant separately from the\n> > > kernel - which might not be possible right now - in which case, given\n> > > how little information is actually duplicated here I'd be fine with this\n> > > for now.\n> >\n> > I'm concerned by this too. The kernel shouldn't report different entity\n> > names for different lenses. There should thus be a single entry in the\n> > camera sensors properties table, and lens information should be reported\n> > separately.\n> \n> I agree this is not the tidiest thing to do. Right now, we have no other way of\n> getting the variant details from the kernel driver into userland. I believe\n> @Dave Stevenson raised this topic at the last Embedded Linux Conference, but no\n> consensus was reached. Once we have a mechanism in the kernel to report lens\n> properties, we will remove this workaround. Would a \\todo suffice for now?\n\nIf adding a \\todo was enough to make a mechanism for this appear\nupstream by magic, I'd say it's fine. Unfortunately, that doesn't match\nmy experience :-)\n\nOne middleground option would be to merge the first \"variant\" (the\n\"imx708\") when the driver gets posted to the linux-media mailing list,\nwhile the rest gets developed, but in any case it will require someone\nmaking a proposal upstream.\n\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > Signed-off-by: Nick Hollinghurst \n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> > > > 1 file changed, 16 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > > > index c3c2caced906..3afd500ea3be 100644\n> > > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > > @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > > > */\n> > > > },\n> > > > } },\n> > > > + { \"imx708\", {\n> > > > + .unitCellSize = { 1400, 1400 },\n> > > > + .testPatternModes = {},\n> > > > + } },\n> > > > + { \"imx708_noir\", {\n> > > > + .unitCellSize = { 1400, 1400 },\n> > > > + .testPatternModes = {},\n> > > > + } },\n> > > > + { \"imx708_wide\", {\n> > > > + .unitCellSize = { 1400, 1400 },\n> > > > + .testPatternModes = {},\n> > > > + } },\n> > > > + { \"imx708_wide_noir\", {\n> > > > + .unitCellSize = { 1400, 1400 },\n> > > > + .testPatternModes = {},\n> > > > + } },\n> > > > { \"ov2740\", {\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 14511BDB1D\n\tfor ;\n\tMon, 23 Jan 2023 10:01:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EF53625E4;\n\tMon, 23 Jan 2023 11:01:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59F42603C0\n\tfor ;\n\tMon, 23 Jan 2023 11:01:21 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5948410B;\n\tMon, 23 Jan 2023 11:01:20 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674468082;\n\tbh=eZjW5dvKCYgsIFXreDLkhBDjXj5e9Fjt86PTNhwuDI8=;\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=HZLJSnJzMBu2pGSUyzH9OTYJ3Va7HUDhqmGTsPi9OLRTvhATjV1evTczwCdyg2vDt\n\tmEKPonwc3pTKgnKQp4Km8zVp4Hr2EpU5fVT3G9Ee9BfTJdn2HBbJxsiSgLEqUUyhj8\n\twyf/18dHkbvQhPkfscQkRLvALe5vGiFMwHv1y0bKEERnYC5SWHcTwuHbLdckMTrSlD\n\tgk7BsRM4fH8WpfAatFoakLn/gunhhI5g3zCZk4nb35I7okQyaJYkV1eVmThyWBXi+y\n\tH+LFjiSE88dE46ba5XdCqDeG6yiHHsaIybLZ3UnfVmDQhGZLSKXZ/u/Gv9rTFtxH/h\n\tCGgU/4dOdNiGw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674468081;\n\tbh=eZjW5dvKCYgsIFXreDLkhBDjXj5e9Fjt86PTNhwuDI8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tOgvUrsmXAyc7HVrPaNMwBW3myHgl5gnJdtjOktjmtdrMSlFMkMWLyWcEpLRTjnhX\n\tSVQd7+P27vuOCaMm9vHtyMwM2q9ftgj3x+VebNIhxITcIOl5A1m7i7oKjmKYtRJZ8Z\n\tk+HA4K1nQ/iK3Zz17SbtID55dlEcslB2+Dul03CU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tOgvUrsm\"; dkim-atps=neutral","Date":"Mon, 23 Jan 2023 12:01:16 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@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 v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26327,"web_url":"https://patchwork.libcamera.org/comment/26327/","msgid":"","date":"2023-01-23T10:33:35","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\nOn Mon, 23 Jan 2023 at 10:01, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:\n> > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:\n> > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via\n> libcamera-devel wrote:\n> > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> > > > > From: Nick Hollinghurst \n> > > > >\n> > > > > The IMX708 sensor driver advertises its module variants\n> (narrow/wide angle lens,\n> > > > > IR block/pass) by modifying the media entity name string. So add\n> duplicate\n> > > > > entries for each variant.\n> > > >\n> > > > The duplication here is a bit of a pain. Are there any sensor\n> properties\n> > > > we might register that would change in each of these variants?\n> > > >\n> > > > Or should we make the CameraSensorProperties class more intelligent\n> so\n> > > > it just does the lookup on the sensor only. Of course ideally we\n> should\n> > > > be able to identify the sensor and the variant separately from the\n> > > > kernel - which might not be possible right now - in which case, given\n> > > > how little information is actually duplicated here I'd be fine with\n> this\n> > > > for now.\n> > >\n> > > I'm concerned by this too. The kernel shouldn't report different entity\n> > > names for different lenses. There should thus be a single entry in the\n> > > camera sensors properties table, and lens information should be\n> reported\n> > > separately.\n> >\n> > I agree this is not the tidiest thing to do. Right now, we have no\n> other way of\n> > getting the variant details from the kernel driver into userland. I\n> believe\n> > @Dave Stevenson raised this topic at the last Embedded Linux Conference,\n> but no\n> > consensus was reached. Once we have a mechanism in the kernel to report\n> lens\n> > properties, we will remove this workaround. Would a \\todo suffice for\n> now?\n>\n> If adding a \\todo was enough to make a mechanism for this appear\n> upstream by magic, I'd say it's fine. Unfortunately, that doesn't match\n> my experience :-)\n>\n\nIf only! :)\n\n\n>\n> One middleground option would be to merge the first \"variant\" (the\n> \"imx708\") when the driver gets posted to the linux-media mailing list,\n> while the rest gets developed, but in any case it will require someone\n> making a proposal upstream.\n>\n\nWe will post a variant-less driver to the mailing list and chat with Dave\nabout\na proposal for module variant reporting.\n\nThe reason we added the other variants in the CameraSensorProperties was to\nquiet the warning log message when a device is not listed. Would it be ok to\nkeep all variants listed like this with a \\todo to clean-up? If not, we\nwill have to\nmake this change in our downstream tree which we are trying hard to\ndeprecate :(\n\nNaush\n\n\n>\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > > Signed-off-by: Nick Hollinghurst <\n> nick.hollinghurst@raspberrypi.com>\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Reviewed-by: Naushir Patuck \n> > > > > Reviewed-by: David Plowman \n> > > > > ---\n> > > > > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> > > > > 1 file changed, 16 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> b/src/libcamera/camera_sensor_properties.cpp\n> > > > > index c3c2caced906..3afd500ea3be 100644\n> > > > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties\n> *CameraSensorProperties::get(const std::string &sen\n> > > > > */\n> > > > > },\n> > > > > } },\n> > > > > + { \"imx708\", {\n> > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > + .testPatternModes = {},\n> > > > > + } },\n> > > > > + { \"imx708_noir\", {\n> > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > + .testPatternModes = {},\n> > > > > + } },\n> > > > > + { \"imx708_wide\", {\n> > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > + .testPatternModes = {},\n> > > > > + } },\n> > > > > + { \"imx708_wide_noir\", {\n> > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > + .testPatternModes = {},\n> > > > > + } },\n> > > > > { \"ov2740\", {\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 2E539BEFBE\n\tfor ;\n\tMon, 23 Jan 2023 10:33:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89B73625E4;\n\tMon, 23 Jan 2023 11:33:53 +0100 (CET)","from mail-yw1-x1133.google.com (mail-yw1-x1133.google.com\n\t[IPv6:2607:f8b0:4864:20::1133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3FA8603C0\n\tfor ;\n\tMon, 23 Jan 2023 11:33:51 +0100 (CET)","by mail-yw1-x1133.google.com with SMTP id\n\t00721157ae682-4ff07dae50dso120026267b3.2\n\tfor ;\n\tMon, 23 Jan 2023 02:33:51 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674470033;\n\tbh=bkUFjxMtrRQ2BJc/iT1ffS5BP7PR7cp0J2iHkY2c+7I=;\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=m812ViTG+pRuOG3nmcuHgTMpR2pSojpNjN+a2gCy547iRdwsIEMdvPb6pXXylVE1c\n\togsY3FDMLKyPyWKgZHwGm69nQaUa+3SA1McKSz/QrFkt1D/JaRJgApJI5lHSFxocNW\n\tX0LB0kiaHuGr3Nf8MSNBJybD9xTyXPmXNYqXop0VbgoBLn1X4ea7QlrqvY8nx1krHR\n\tD+pVtxT54zFJ3I/tSWdJ3rbc4bvDACfT5XO4hi7/nYYe4GBLOrqrkBiGL6uLxy5VgY\n\tciaFtPDaqT0YG81cAIgFVOyDjsrY1j1ACGZ+7qOriRmiXw8EkyaQnF3Voek5jOnfP4\n\tsyTfkGiSgdVkg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=xXjVyeCcyS8DtMkiHzgMfW3c5+TGA6dXNRq6Q9jevH4=;\n\tb=EnjSEcV5ZDCXkYPPl+GamAgyYm+3wjn17AISsPCPQowVyqEDBHV9oYLMNiweEUpNdh\n\thy4X19IdzXH7tFCWqQTnsxUoGAQu/h9HsSyS1OWkveqw4V7iD2DMJ5sZsDQ2upXmAvhM\n\tFp9DWIlHTosscC2+CyqcLH4Hc7NRYFagIGKu7eQZQSI6qad//Al5K2szGHnXdCW++enI\n\tc8y0OROZss6GHKfPEBGWL8Ien93Wo//KsW4bQWV8Qli3doq3CZUOIwRSN4bjUYCoTl29\n\t0wC5UYL+RTIBRaaSKkh9vlSzLq+3cyYa5iS0ToJLo8zBp2TE3RVwLcol21b6e9c7YcPK\n\to6JQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EnjSEcV5\"; dkim-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=xXjVyeCcyS8DtMkiHzgMfW3c5+TGA6dXNRq6Q9jevH4=;\n\tb=58H/r4um3j31kr5PnXdd2hxivCdtwxUUcUrx+0c5Ebm2zvZVIre6br6w2HP4pl7b8T\n\taBDdCHYAEIsfovc7xH1NZoSdzWgJfGQ0Fr0cdo9G0Chqm4TNoMApq6SpWMkyZdFhOSra\n\tjAw8/spGv/POzOfhUeL57GauHRjShAVaB7l6sLsZTJK++CrAR/p/a4/Wxc1EgGx9TZtI\n\t0H6/4bYVFa5CwPxahCAp+jiFg6AjHgw1e/lf4+xgFMYPoLxg40Y9GV+9z2v7C3RzNai5\n\ti3cGZzNVVWwHwZZBjTMHMwbzTdGHnjcBU79/u8ARyFiQ6I6PVC8x86SZgaGKqtONbOzX\n\t3CZw==","X-Gm-Message-State":"AFqh2kp/LhaPinvLEpfOIWmm9W5fap10Y6g67lA7cNdn0FZeh1Z9jX/S\n\tca0yi28Sa6zh6BzULAB068yLCHeVQw+ryaz5hYfwUlpufOB2k1hXrVI=","X-Google-Smtp-Source":"AMrXdXtuCWO7CbzbsQUFuJGGzk4LpI4PdyEdYy6ekraKBs34qXAhsDocIbbfjpNYvqTZJUeXxZxHHWiVZL5RO+jNK9E=","X-Received":"by 2002:a05:690c:852:b0:4d7:eb11:6bf7 with SMTP id\n\tbz18-20020a05690c085200b004d7eb116bf7mr2788603ywb.235.1674470030856;\n\tMon, 23 Jan 2023 02:33:50 -0800 (PST)","MIME-Version":"1.0","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>\n\t\n\t\n\t","In-Reply-To":"","Date":"Mon, 23 Jan 2023 10:33:35 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000006066ef05f2ebefc3\"","Subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26330,"web_url":"https://patchwork.libcamera.org/comment/26330/","msgid":"","date":"2023-01-23T11:47:25","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Laurent and Naush\n\nOn Mon, 23 Jan 2023 at 10:33, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Hi Laurent,\n>\n>\n> On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart wrote:\n>>\n>> Hi Naush,\n>>\n>> On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:\n>> > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:\n>> > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via libcamera-devel wrote:\n>> > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n>> > > > > From: Nick Hollinghurst \n>> > > > >\n>> > > > > The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,\n>> > > > > IR block/pass) by modifying the media entity name string. So add duplicate\n>> > > > > entries for each variant.\n>> > > >\n>> > > > The duplication here is a bit of a pain. Are there any sensor properties\n>> > > > we might register that would change in each of these variants?\n>> > > >\n>> > > > Or should we make the CameraSensorProperties class more intelligent so\n>> > > > it just does the lookup on the sensor only. Of course ideally we should\n>> > > > be able to identify the sensor and the variant separately from the\n>> > > > kernel - which might not be possible right now - in which case, given\n>> > > > how little information is actually duplicated here I'd be fine with this\n>> > > > for now.\n>> > >\n>> > > I'm concerned by this too. The kernel shouldn't report different entity\n>> > > names for different lenses. There should thus be a single entry in the\n>> > > camera sensors properties table, and lens information should be reported\n>> > > separately.\n>> >\n>> > I agree this is not the tidiest thing to do. Right now, we have no other way of\n>> > getting the variant details from the kernel driver into userland. I believe\n>> > @Dave Stevenson raised this topic at the last Embedded Linux Conference, but no\n>> > consensus was reached. Once we have a mechanism in the kernel to report lens\n>> > properties, we will remove this workaround. Would a \\todo suffice for now?\n>>\n>> If adding a \\todo was enough to make a mechanism for this appear\n>> upstream by magic, I'd say it's fine. Unfortunately, that doesn't match\n>> my experience :-)\n>\n>\n> If only! :)\n\nI'll acknowledge that time was getting short at the media summit.\n\nThe initial proposal was made as I knew these modules were coming. I'd\nsuggested the addition of an optics descriptor in the media entitiy,\nnot unlike the connector on the end of bridges in DRM. That would\nprovide the basic properties of the optics, with the option of\ncalibration too.\nThe general response seemed to be along the lines \"oh no it's far too\ncomplicated to model, and needs to be done as a module description\".\nSeeing as a module description almost flies in the face of the current\nkernel frameworks describing the generic sensor and VCM drivers, I\nhave no concept of the framework being anticipated and how it fits\ninto the current world. Add in the complication of ACPI as well as DT,\nand all bets are off.\n\nYourself (Laurent) and Sakari are the two main gatekeepers of what is\nmerged to mainline, so it's down to what satisfies you. As you both\nshot down the initial proposal with no greater context, it leaves 3rd\nparties totally at sea as to the use cases that were considered as not\ncovered, or any indication of what would be acceptable. That's not a\ngreat motivator to invest time developing a proposal.\n\nThere needs to be an initial discussion including both of you to\nformulate a basic direction on the subject, otherwise it's wasted\neffort. Until then you'll get vendors doing their own thing, and their\nuse of libcamera will inherently fragment.\n\n Dave\n\n>>\n>>\n>> One middleground option would be to merge the first \"variant\" (the\n>> \"imx708\") when the driver gets posted to the linux-media mailing list,\n>> while the rest gets developed, but in any case it will require someone\n>> making a proposal upstream.\n>\n>\n> We will post a variant-less driver to the mailing list and chat with Dave about\n> a proposal for module variant reporting.\n>\n> The reason we added the other variants in the CameraSensorProperties was to\n> quiet the warning log message when a device is not listed. Would it be ok to\n> keep all variants listed like this with a \\todo to clean-up? If not, we will have to\n> make this change in our downstream tree which we are trying hard to deprecate :(\n>\n> Naush\n>\n>>\n>>\n>> > > > Reviewed-by: Kieran Bingham \n>> > > >\n>> > > > > Signed-off-by: Nick Hollinghurst \n>> > > > > Signed-off-by: Naushir Patuck \n>> > > > > Reviewed-by: Naushir Patuck \n>> > > > > Reviewed-by: David Plowman \n>> > > > > ---\n>> > > > > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n>> > > > > 1 file changed, 16 insertions(+)\n>> > > > >\n>> > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n>> > > > > index c3c2caced906..3afd500ea3be 100644\n>> > > > > --- a/src/libcamera/camera_sensor_properties.cpp\n>> > > > > +++ b/src/libcamera/camera_sensor_properties.cpp\n>> > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> > > > > */\n>> > > > > },\n>> > > > > } },\n>> > > > > + { \"imx708\", {\n>> > > > > + .unitCellSize = { 1400, 1400 },\n>> > > > > + .testPatternModes = {},\n>> > > > > + } },\n>> > > > > + { \"imx708_noir\", {\n>> > > > > + .unitCellSize = { 1400, 1400 },\n>> > > > > + .testPatternModes = {},\n>> > > > > + } },\n>> > > > > + { \"imx708_wide\", {\n>> > > > > + .unitCellSize = { 1400, 1400 },\n>> > > > > + .testPatternModes = {},\n>> > > > > + } },\n>> > > > > + { \"imx708_wide_noir\", {\n>> > > > > + .unitCellSize = { 1400, 1400 },\n>> > > > > + .testPatternModes = {},\n>> > > > > + } },\n>> > > > > { \"ov2740\", {\n>> > > > > .unitCellSize = { 1400, 1400 },\n>> > > > > .testPatternModes = {\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D0B86BDB1D\n\tfor ;\n\tMon, 23 Jan 2023 11:47:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A549625E4;\n\tMon, 23 Jan 2023 12:47:45 +0100 (CET)","from mail-vs1-xe2e.google.com (mail-vs1-xe2e.google.com\n\t[IPv6:2607:f8b0:4864:20::e2e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85918603C7\n\tfor ;\n\tMon, 23 Jan 2023 12:47:43 +0100 (CET)","by mail-vs1-xe2e.google.com with SMTP id n190so12513796vsc.11\n\tfor ;\n\tMon, 23 Jan 2023 03:47:43 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674474465;\n\tbh=gvMGF2nDJL9HVrtm2Meluh32YvZe5s/B5s7BTbIAy3Y=;\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=L0+zzCH/+qal9w+jqmomaBmcXrtlEEz2X1bXK7bTCLhF0pM43G6ietZgvA9rPQUIU\n\tlBL+YLOA7yWkx7dzp2/oJKVBtEIlAMAE55pXl+mP0hjTQ5MWUdQvxthn1vTAU+pZoI\n\tvSRXOKHLbNW/xBOWhH5duVZ1PziBolHLaIDvpHUTSR9YB9ZGB3si6nTgzaUUvftHGn\n\tBj6vMiI3V7zWQF3TbKT/GtyLK+TvSoaaOd2teO9KJhXwaglRTu86UJ8I3q1WcUR5sx\n\tRUETSHlrKAtvwbQ9DX458j7yE6FEkEEo52AZ4TuTtIY09vt+tqJdeYj+5xr3Qjv9xB\n\tcLkD9RlDGeN7Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=fMcyZd+1q1Y3GT9EgX5QXGxZDgqpoPYTHj0yNQeMsWk=;\n\tb=P3xIoxWcJSFu7Ljw0ZllSIkKpGZS40TWvddu+M5S1Oxr8A6z7LozMKxAcckosvE7W+\n\thFHTeZMHsyshryRngcSEax2VIT274/FiMB7FU8jo6jtUycyxZTZ6ls60VzFTG7QqhE6p\n\t1ecyhOgSv3qOFdDnGkNirUdVIZCXVdiMkrwBZwLGPrKpuDGjbvem72wBpGw/4HDLK/TO\n\t2bX1Sx1w1axyiMuJ+zcLOlETtjOEVQVA1IfeqVmusRABNgoSKkF5aTETvET1WFPu2VLy\n\tL9hkAXNyxOL1YB11Ci1I883v4qI40oHkobZMjzYzYHraFWmZyh7JLVcuxRfKYSzo/fVq\n\t9xAA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"P3xIoxWc\"; dkim-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=fMcyZd+1q1Y3GT9EgX5QXGxZDgqpoPYTHj0yNQeMsWk=;\n\tb=XcDa0ZlEwA5xe8mUhV+wac7G8lsCbEb5uSD1E+rkULEi5abQIEL6hoVhyn8CnUuG08\n\t5zVRajkH54C0F7o6ixzQLYvqVAsBV4EiZC9zjHAjerVt2MwX7rt6YHgdXdZeClXnAFA5\n\tftsbekMAQlDz4km5f+SoKENbP0OKHOpkP/tqx+mzLtYB2/S+vwh5+vivhd9yPuoceZJ6\n\tkAA/yvUkesG+k0LrohxCb68AZkyjVyju7wXpAZwRAaW2row1yunMEWCmaydnzu7wAxgg\n\tdS3/Y5ojJLKfEXrEEmjg0x7loNeA4UVlhSQDTXtip95LTVjmcX3GRTllIbxBzqxCHNsx\n\tVNfA==","X-Gm-Message-State":"AFqh2ko07311422p/gr0tpFOnZsaPMqEsJVzCbbQHTi7+NDik3bjfgva\n\tdebEK0QHvM4LTWdKS7QLaYPWcUCd+rHu8xMFy3iirA==","X-Google-Smtp-Source":"AMrXdXutqlXsCIZNHsWXSonW8BUCHeG4URjDnkWLYdP0Ua6APNumaTvJgI3L59O3DWgLzPq66j97Lf8zZi3D4jRIEMI=","X-Received":"by 2002:a67:ea8f:0:b0:3d0:ee83:9599 with SMTP id\n\tf15-20020a67ea8f000000b003d0ee839599mr3454215vso.45.1674474462148;\n\tMon, 23 Jan 2023 03:47:42 -0800 (PST)","MIME-Version":"1.0","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Mon, 23 Jan 2023 11:47:25 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26331,"web_url":"https://patchwork.libcamera.org/comment/26331/","msgid":"","date":"2023-01-23T12:12:16","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jan 23, 2023 at 10:33:35AM +0000, Naushir Patuck wrote:\n> On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart wrote:\n> > On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:\n> > > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:\n> > > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> > > > > > From: Nick Hollinghurst \n> > > > > >\n> > > > > > The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,\n> > > > > > IR block/pass) by modifying the media entity name string. So add duplicate\n> > > > > > entries for each variant.\n> > > > >\n> > > > > The duplication here is a bit of a pain. Are there any sensor properties\n> > > > > we might register that would change in each of these variants?\n> > > > >\n> > > > > Or should we make the CameraSensorProperties class more intelligent so\n> > > > > it just does the lookup on the sensor only. Of course ideally we should\n> > > > > be able to identify the sensor and the variant separately from the\n> > > > > kernel - which might not be possible right now - in which case, given\n> > > > > how little information is actually duplicated here I'd be fine with this\n> > > > > for now.\n> > > >\n> > > > I'm concerned by this too. The kernel shouldn't report different entity\n> > > > names for different lenses. There should thus be a single entry in the\n> > > > camera sensors properties table, and lens information should be reported\n> > > > separately.\n> > >\n> > > I agree this is not the tidiest thing to do. Right now, we have no other way of\n> > > getting the variant details from the kernel driver into userland. I believe\n> > > @Dave Stevenson raised this topic at the last Embedded Linux Conference, but no\n> > > consensus was reached. Once we have a mechanism in the kernel to report lens\n> > > properties, we will remove this workaround. Would a \\todo suffice for now?\n> >\n> > If adding a \\todo was enough to make a mechanism for this appear\n> > upstream by magic, I'd say it's fine. Unfortunately, that doesn't match\n> > my experience :-)\n> \n> If only! :)\n> \n> > One middleground option would be to merge the first \"variant\" (the\n> > \"imx708\") when the driver gets posted to the linux-media mailing list,\n> > while the rest gets developed, but in any case it will require someone\n> > making a proposal upstream.\n> \n> We will post a variant-less driver to the mailing list and chat with Dave about\n> a proposal for module variant reporting.\n\nYou could also include variant support in the driver posted to\nlinux-media, so the proposal could be discussed during reviews. Up to\nyou.\n\n> The reason we added the other variants in the CameraSensorProperties was to\n> quiet the warning log message when a device is not listed. Would it be ok to\n> keep all variants listed like this with a \\todo to clean-up? If not, we will have to\n> make this change in our downstream tree which we are trying hard to deprecate :(\n\nWhat concerns me is when that would be cleaned up. Unless there's a real\neffort to design and upstream a mechanism for this, it won't happen, so\nI want to see someone taking the lead and bringing it to completion.\nMerging support for a device in libcamera before the kernel driver is\nready upstream is already a middleground.\n\n> > > > > Reviewed-by: Kieran Bingham \n> > > > >\n> > > > > > Signed-off-by: Nick Hollinghurst \n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > Reviewed-by: Naushir Patuck \n> > > > > > Reviewed-by: David Plowman \n> > > > > > ---\n> > > > > > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> > > > > > 1 file changed, 16 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > > > > > index c3c2caced906..3afd500ea3be 100644\n> > > > > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > > > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > > > > > */\n> > > > > > },\n> > > > > > } },\n> > > > > > + { \"imx708\", {\n> > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > + .testPatternModes = {},\n> > > > > > + } },\n> > > > > > + { \"imx708_noir\", {\n> > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > + .testPatternModes = {},\n> > > > > > + } },\n> > > > > > + { \"imx708_wide\", {\n> > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > + .testPatternModes = {},\n> > > > > > + } },\n> > > > > > + { \"imx708_wide_noir\", {\n> > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > + .testPatternModes = {},\n> > > > > > + } },\n> > > > > > { \"ov2740\", {\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 2BD26BEFBE\n\tfor ;\n\tMon, 23 Jan 2023 12:12:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5A85603C7;\n\tMon, 23 Jan 2023 13:12:21 +0100 (CET)","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 85726603C7\n\tfor ;\n\tMon, 23 Jan 2023 13:12:20 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C11EA2B3;\n\tMon, 23 Jan 2023 13:12:19 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674475941;\n\tbh=iNq/rk2XP5W2U1EwKw8og+1wQBxlVfoVpnTiJpc/06U=;\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=xi1dZI7KmshyA6GXXSv5t0WJNxbW3Wp9sqKF5c+STC0k0PdbkSsbrg8wYLfstm13G\n\tWB1VeF6Z7kmIqs1Zu01rhDKTbZZ2ZlYVTrUyu+Rykmm5WgK0mGE29XTISUVN5K/ZRk\n\t5mNwZvRjG2/s/65GQSqNbCJxEg5y8m6yVNykAEcvwOZobkUPQcedYn6yZpcTp8ek4W\n\t/Sb/JRMw/0yDM9TLuZxdggYc54p7MDThIOVtjfpROZIorsLat1tFS+ZKd2emjry2DV\n\tj2Br8a89aI6nR+MPcUyYU5VtAFd54VHJXPRf5e76zGMTomnszvAiLVsX1/cRYpBj1D\n\tMdppQSTQwVd3g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674475940;\n\tbh=iNq/rk2XP5W2U1EwKw8og+1wQBxlVfoVpnTiJpc/06U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jZXQxB7PnWNmQ9NYGTxAXHPMBhn8UfD0K7Win7trI71zFbclCGjyT8Z0Pb59SME4e\n\tZABbnGuMHCsEqk8XnZPHXcSAGiTi7A62igcTSSN7syT3dEGAP64QexRICytd57vzoi\n\t2n2HNPI7IJgGe307HnnHoXEbVMoGKltQzbIvopFQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jZXQxB7P\"; dkim-atps=neutral","Date":"Mon, 23 Jan 2023 14:12:16 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>\n\t\n\t\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 v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26332,"web_url":"https://patchwork.libcamera.org/comment/26332/","msgid":"","date":"2023-01-23T12:21:03","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dave,\n\n(CC'ing Sakari)\n\nOn Mon, Jan 23, 2023 at 11:47:25AM +0000, Dave Stevenson wrote:\n> On Mon, 23 Jan 2023 at 10:33, Naushir Patuck via libcamera-devel wrote:\n> > On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart wrote:\n> >> On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:\n> >> > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:\n> >> > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via libcamera-devel wrote:\n> >> > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> >> > > > > From: Nick Hollinghurst \n> >> > > > >\n> >> > > > > The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,\n> >> > > > > IR block/pass) by modifying the media entity name string. So add duplicate\n> >> > > > > entries for each variant.\n> >> > > >\n> >> > > > The duplication here is a bit of a pain. Are there any sensor properties\n> >> > > > we might register that would change in each of these variants?\n> >> > > >\n> >> > > > Or should we make the CameraSensorProperties class more intelligent so\n> >> > > > it just does the lookup on the sensor only. Of course ideally we should\n> >> > > > be able to identify the sensor and the variant separately from the\n> >> > > > kernel - which might not be possible right now - in which case, given\n> >> > > > how little information is actually duplicated here I'd be fine with this\n> >> > > > for now.\n> >> > >\n> >> > > I'm concerned by this too. The kernel shouldn't report different entity\n> >> > > names for different lenses. There should thus be a single entry in the\n> >> > > camera sensors properties table, and lens information should be reported\n> >> > > separately.\n> >> >\n> >> > I agree this is not the tidiest thing to do. Right now, we have no other way of\n> >> > getting the variant details from the kernel driver into userland. I believe\n> >> > @Dave Stevenson raised this topic at the last Embedded Linux Conference, but no\n> >> > consensus was reached. Once we have a mechanism in the kernel to report lens\n> >> > properties, we will remove this workaround. Would a \\todo suffice for now?\n> >>\n> >> If adding a \\todo was enough to make a mechanism for this appear\n> >> upstream by magic, I'd say it's fine. Unfortunately, that doesn't match\n> >> my experience :-)\n> >\n> >\n> > If only! :)\n> \n> I'll acknowledge that time was getting short at the media summit.\n> \n> The initial proposal was made as I knew these modules were coming. I'd\n> suggested the addition of an optics descriptor in the media entitiy,\n> not unlike the connector on the end of bridges in DRM. That would\n> provide the basic properties of the optics, with the option of\n> calibration too.\n> The general response seemed to be along the lines \"oh no it's far too\n> complicated to model, and needs to be done as a module description\".\n> Seeing as a module description almost flies in the face of the current\n> kernel frameworks describing the generic sensor and VCM drivers, I\n> have no concept of the framework being anticipated and how it fits\n> into the current world. Add in the complication of ACPI as well as DT,\n> and all bets are off.\n\nSakari, do you have any insight on how this is handled in ACPI (if at\nall) ?\n\n> Yourself (Laurent) and Sakari are the two main gatekeepers of what is\n> merged to mainline, so it's down to what satisfies you. As you both\n> shot down the initial proposal with no greater context, it leaves 3rd\n> parties totally at sea as to the use cases that were considered as not\n> covered, or any indication of what would be acceptable. That's not a\n> great motivator to invest time developing a proposal.\n> \n> There needs to be an initial discussion including both of you to\n> formulate a basic direction on the subject, otherwise it's wasted\n> effort. Until then you'll get vendors doing their own thing, and their\n> use of libcamera will inherently fragment.\n\nThen let's have that discussion.\n\nI'm afraid that support for the whole camera ecosystem can't rest on the\nshoulders of two individuals only, with a whole industry working in a\nfire-and-forget mode, and Sakari and I left to fix everything. It can't\nscale, and it just wouldn't be fair.\n\n> >> One middleground option would be to merge the first \"variant\" (the\n> >> \"imx708\") when the driver gets posted to the linux-media mailing list,\n> >> while the rest gets developed, but in any case it will require someone\n> >> making a proposal upstream.\n> >\n> > We will post a variant-less driver to the mailing list and chat with Dave about\n> > a proposal for module variant reporting.\n> >\n> > The reason we added the other variants in the CameraSensorProperties was to\n> > quiet the warning log message when a device is not listed. Would it be ok to\n> > keep all variants listed like this with a \\todo to clean-up? If not, we will have to\n> > make this change in our downstream tree which we are trying hard to deprecate :(\n> >\n> >> > > > Reviewed-by: Kieran Bingham \n> >> > > >\n> >> > > > > Signed-off-by: Nick Hollinghurst \n> >> > > > > Signed-off-by: Naushir Patuck \n> >> > > > > Reviewed-by: Naushir Patuck \n> >> > > > > Reviewed-by: David Plowman \n> >> > > > > ---\n> >> > > > > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> >> > > > > 1 file changed, 16 insertions(+)\n> >> > > > >\n> >> > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> >> > > > > index c3c2caced906..3afd500ea3be 100644\n> >> > > > > --- a/src/libcamera/camera_sensor_properties.cpp\n> >> > > > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> >> > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> > > > > */\n> >> > > > > },\n> >> > > > > } },\n> >> > > > > + { \"imx708\", {\n> >> > > > > + .unitCellSize = { 1400, 1400 },\n> >> > > > > + .testPatternModes = {},\n> >> > > > > + } },\n> >> > > > > + { \"imx708_noir\", {\n> >> > > > > + .unitCellSize = { 1400, 1400 },\n> >> > > > > + .testPatternModes = {},\n> >> > > > > + } },\n> >> > > > > + { \"imx708_wide\", {\n> >> > > > > + .unitCellSize = { 1400, 1400 },\n> >> > > > > + .testPatternModes = {},\n> >> > > > > + } },\n> >> > > > > + { \"imx708_wide_noir\", {\n> >> > > > > + .unitCellSize = { 1400, 1400 },\n> >> > > > > + .testPatternModes = {},\n> >> > > > > + } },\n> >> > > > > { \"ov2740\", {\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 55946BDB1D\n\tfor ;\n\tMon, 23 Jan 2023 12:21:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5F90625D8;\n\tMon, 23 Jan 2023 13:21:09 +0100 (CET)","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 AB403603C7\n\tfor ;\n\tMon, 23 Jan 2023 13:21:07 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B64F2B3;\n\tMon, 23 Jan 2023 13:21:06 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674476469;\n\tbh=LUCSBDQSUEtidnINTN+hcMCNe16eEBaR+QX7IACQ3Cw=;\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=rFUZkwbgD2+3bOFTfcBZLtQgvQIwrXZ1V5/mF9IKFpanRniJtyPh8RgAeC7syeJaz\n\tqbcyRBYV1AQAay6zcMnIOBMJ0e+BCzQSUm8JkJrXJrCJtHMInBndb41oxXn4nporiS\n\tmYCu/wJtLrrFvWYwdbv5J3QtQ2OpdKF8RXFl1etLelpNM9IS8KjJRLb43Gc9AiX/wA\n\tqoxLeyMynBX0esNcY8oViwX7sLgLDIPzKU/oWcrC/+Iw13pLEGUjW/LsF8zIUgj6CA\n\tts/S6sxcfKtNTmRysxcFrG+v9ReccG1bGxuGQLsbZrXb+EFnzEF3ZVf68C+kDYzjX1\n\txlXtXCdl9xQWw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674476467;\n\tbh=LUCSBDQSUEtidnINTN+hcMCNe16eEBaR+QX7IACQ3Cw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X/eG7ed7eyZsskFCUW4S2bFbM3MU44caR7xjNPxdQXTWEsEpexO3wYtVb4lS6E2oP\n\tbp6KRghW0w/tK/a7oHpA7PA5xgLFjYkBIshFVmxUodRfhG+EeUqV6JKLdEoT4PKNuX\n\t9e7yvDipt+d/GJ4zSWW29EQXPMOFtHs5dXu4zkvc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"X/eG7ed7\"; dkim-atps=neutral","Date":"Mon, 23 Jan 2023 14:21:03 +0200","To":"Dave Stevenson ","Message-ID":"","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>\n\t\n\t\n\t\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 v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26334,"web_url":"https://patchwork.libcamera.org/comment/26334/","msgid":"","date":"2023-01-23T12:36:13","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Mon, 23 Jan 2023 at 12:12, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Mon, Jan 23, 2023 at 10:33:35AM +0000, Naushir Patuck wrote:\n> > On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart wrote:\n> > > On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:\n> > > > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:\n> > > > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via\n> libcamera-devel wrote:\n> > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> > > > > > > From: Nick Hollinghurst \n> > > > > > >\n> > > > > > > The IMX708 sensor driver advertises its module variants\n> (narrow/wide angle lens,\n> > > > > > > IR block/pass) by modifying the media entity name string. So\n> add duplicate\n> > > > > > > entries for each variant.\n> > > > > >\n> > > > > > The duplication here is a bit of a pain. Are there any sensor\n> properties\n> > > > > > we might register that would change in each of these variants?\n> > > > > >\n> > > > > > Or should we make the CameraSensorProperties class more\n> intelligent so\n> > > > > > it just does the lookup on the sensor only. Of course ideally we\n> should\n> > > > > > be able to identify the sensor and the variant separately from\n> the\n> > > > > > kernel - which might not be possible right now - in which case,\n> given\n> > > > > > how little information is actually duplicated here I'd be fine\n> with this\n> > > > > > for now.\n> > > > >\n> > > > > I'm concerned by this too. The kernel shouldn't report different\n> entity\n> > > > > names for different lenses. There should thus be a single entry in\n> the\n> > > > > camera sensors properties table, and lens information should be\n> reported\n> > > > > separately.\n> > > >\n> > > > I agree this is not the tidiest thing to do. Right now, we have no\n> other way of\n> > > > getting the variant details from the kernel driver into userland. I\n> believe\n> > > > @Dave Stevenson raised this topic at the last Embedded Linux\n> Conference, but no\n> > > > consensus was reached. Once we have a mechanism in the kernel to\n> report lens\n> > > > properties, we will remove this workaround. Would a \\todo suffice\n> for now?\n> > >\n> > > If adding a \\todo was enough to make a mechanism for this appear\n> > > upstream by magic, I'd say it's fine. Unfortunately, that doesn't match\n> > > my experience :-)\n> >\n> > If only! :)\n> >\n> > > One middleground option would be to merge the first \"variant\" (the\n> > > \"imx708\") when the driver gets posted to the linux-media mailing list,\n> > > while the rest gets developed, but in any case it will require someone\n> > > making a proposal upstream.\n> >\n> > We will post a variant-less driver to the mailing list and chat with\n> Dave about\n> > a proposal for module variant reporting.\n>\n> You could also include variant support in the driver posted to\n> linux-media, so the proposal could be discussed during reviews. Up to\n> you.\n>\n\nI think we will remove this to start with. I fear it will only muddy the\nwaters\nfor the bigger discussion. We can always refer back to the downstream\ndriver\nto show examples of why it is required.\n\n\n>\n> > The reason we added the other variants in the CameraSensorProperties was\n> to\n> > quiet the warning log message when a device is not listed. Would it be\n> ok to\n> > keep all variants listed like this with a \\todo to clean-up? If not, we\n> will have to\n> > make this change in our downstream tree which we are trying hard to\n> deprecate :(\n>\n> What concerns me is when that would be cleaned up. Unless there's a real\n> effort to design and upstream a mechanism for this, it won't happen, so\n> I want to see someone taking the lead and bringing it to completion.\n> Merging support for a device in libcamera before the kernel driver is\n> ready upstream is already a middleground.\n>\n\nGot it, I'll remove the duplicate entries from the patch.\n\nRegards,\nNaush\n\n\n>\n> > > > > > Reviewed-by: Kieran Bingham \n> > > > > >\n> > > > > > > Signed-off-by: Nick Hollinghurst <\n> nick.hollinghurst@raspberrypi.com>\n> > > > > > > Signed-off-by: Naushir Patuck \n> > > > > > > Reviewed-by: Naushir Patuck \n> > > > > > > Reviewed-by: David Plowman \n> > > > > > > ---\n> > > > > > > src/libcamera/camera_sensor_properties.cpp | 16\n> ++++++++++++++++\n> > > > > > > 1 file changed, 16 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp\n> b/src/libcamera/camera_sensor_properties.cpp\n> > > > > > > index c3c2caced906..3afd500ea3be 100644\n> > > > > > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > > > > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties\n> *CameraSensorProperties::get(const std::string &sen\n> > > > > > > */\n> > > > > > > },\n> > > > > > > } },\n> > > > > > > + { \"imx708\", {\n> > > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > > + .testPatternModes = {},\n> > > > > > > + } },\n> > > > > > > + { \"imx708_noir\", {\n> > > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > > + .testPatternModes = {},\n> > > > > > > + } },\n> > > > > > > + { \"imx708_wide\", {\n> > > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > > + .testPatternModes = {},\n> > > > > > > + } },\n> > > > > > > + { \"imx708_wide_noir\", {\n> > > > > > > + .unitCellSize = { 1400, 1400 },\n> > > > > > > + .testPatternModes = {},\n> > > > > > > + } },\n> > > > > > > { \"ov2740\", {\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 7E422BEFBE\n\tfor ;\n\tMon, 23 Jan 2023 12:36:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFA56625E4;\n\tMon, 23 Jan 2023 13:36:31 +0100 (CET)","from mail-yb1-xb2c.google.com (mail-yb1-xb2c.google.com\n\t[IPv6:2607:f8b0:4864:20::b2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 068F3603C7\n\tfor ;\n\tMon, 23 Jan 2023 13:36:29 +0100 (CET)","by mail-yb1-xb2c.google.com with SMTP id 203so14517329yby.10\n\tfor ;\n\tMon, 23 Jan 2023 04:36:29 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674477391;\n\tbh=IBcf3Vyt80+d/yhZ+hoR+S7aQ0p0VtjhbklWpvb1Was=;\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=GBLDagO6b91sN5Ugx+cXQZV2zn5k2xPjJQqyxJ1/W640xbxKadJOoGrCHESGU/lLj\n\tH+27YikwoWatUgo0L4RzwXvq4TYcC7qq1vukAnbp8wwol1jq21qS3VfyMM5Cwr4o6g\n\tMX7ETtyY/yXMgbasR2VReLLHJLJS1S6D+BfXz7bU+2s+90nlJWKpj8f1SiJsKUskRq\n\t4BDOmmFTfMcMfFTAEHfyEIAjw7e6zC/9BgHS65odLOk14xREkfjaiMMlKYRXTs//Pk\n\tp33Cc7lBez+h8Qr587eM6gHLLHfVW212N3KTvcJWECOPndnxX317TNcZYuRgTo8xCO\n\tAD+DSvoxO95OA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=nKUJ3Ou+8DWq3QMc5fe+g89ppv9ilyFJMWBMWEkVQq4=;\n\tb=t+l0/RhaZ+iebo8KTYK6IPrOpaZ1LFhBxaCrDsOKC85rBm/ypNXqpe/njIZZor4P7T\n\tlPUNMcyYfFI6L7jZpz7cvji25EeDuGTML8MqFrKbqTHt8CGKIY+eEq4qZj1/17GXN2AD\n\tx4TUQFDOb0ZqLBozdUsy5qspWVyLlVpOH/92SnSAr8nJCW7geu6TVSkfs3cMb352mGJO\n\tXFxRTFviwcdajiJ+LkAwNp88x4z4ub96V1w+xfzPs4eB0vtb9hivaj/e7iCdKAEcKr9b\n\tRg7aXIerN/pD6R9qyoBC9v/dIb9nwvJW5DKlJW8TPavH42ya6sBdw27Qy9yro0RTHKuk\n\tnwGQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"t+l0/Rha\"; dkim-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=nKUJ3Ou+8DWq3QMc5fe+g89ppv9ilyFJMWBMWEkVQq4=;\n\tb=XlIt664sAkHyyr4gWcJ/1TEB9d/wLkiNk4ZhuJzFIuzOLYzWxvmArUSg7wppOO3VYs\n\tf9qxz8gxD61tmBhBsMrCFv/JhAVnWf0R0e6mMCZY912ddQwIKClBQdhDUj4I/LWUd/wY\n\tbpEPi7HhSYrNzIddCA2ZqLqVNaQ5pcOW0+02eOmBuNfG+dPBHXsA6q033sCQv5aKbH5e\n\tOqwRQ/CXj8qlna1w73iDqSjBfEGZAs2nCKOKLKf6bsT8KtMRV1VRwDIREQp1f1/KCr8A\n\touu0Xy4Fn6n+1+EZS6TXf72G8pvoVpfiLf1SiT6R5ATgKcXwVWAlCHW+1WgS7Mgg8a1l\n\tnYfw==","X-Gm-Message-State":"AFqh2kpba1cUm5nzUrVsSXp0p2AF/VQdAyFt9v7bAET67l0Gxe6QY9RW\n\tFbsR/G2ARoaTTthONTGc7ZVWaf9/AOSJ5J5bPmQFWkT4EVDZ+VaHYac=","X-Google-Smtp-Source":"AMrXdXtR06nvK7b09b7UjWdb5snA1Xmqn0pqxkQXMCRNB4sJqseHpHzCDDMDwJ9HB2LVhuwAkAzNnjXciTofI07qiqI=","X-Received":"by 2002:a25:bf8e:0:b0:7d7:ec44:7cdc with SMTP id\n\tl14-20020a25bf8e000000b007d7ec447cdcmr2549040ybk.598.1674477388898;\n\tMon, 23 Jan 2023 04:36:28 -0800 (PST)","MIME-Version":"1.0","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Mon, 23 Jan 2023 12:36:13 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000f33ffe05f2eda503\"","Subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26336,"web_url":"https://patchwork.libcamera.org/comment/26336/","msgid":"","date":"2023-01-23T14:09:02","subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","submitter":{"id":32,"url":"https://patchwork.libcamera.org/api/people/32/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Laurent, Dave,\n\nOn Mon, Jan 23, 2023 at 02:21:03PM +0200, Laurent Pinchart wrote:\n> Hi Dave,\n> \n> (CC'ing Sakari)\n> \n> On Mon, Jan 23, 2023 at 11:47:25AM +0000, Dave Stevenson wrote:\n> > On Mon, 23 Jan 2023 at 10:33, Naushir Patuck via libcamera-devel wrote:\n> > > On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart wrote:\n> > >> On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:\n> > >> > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:\n> > >> > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > >> > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)\n> > >> > > > > From: Nick Hollinghurst \n> > >> > > > >\n> > >> > > > > The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,\n> > >> > > > > IR block/pass) by modifying the media entity name string. So add duplicate\n> > >> > > > > entries for each variant.\n> > >> > > >\n> > >> > > > The duplication here is a bit of a pain. Are there any sensor properties\n> > >> > > > we might register that would change in each of these variants?\n> > >> > > >\n> > >> > > > Or should we make the CameraSensorProperties class more intelligent so\n> > >> > > > it just does the lookup on the sensor only. Of course ideally we should\n> > >> > > > be able to identify the sensor and the variant separately from the\n> > >> > > > kernel - which might not be possible right now - in which case, given\n> > >> > > > how little information is actually duplicated here I'd be fine with this\n> > >> > > > for now.\n> > >> > >\n> > >> > > I'm concerned by this too. The kernel shouldn't report different entity\n> > >> > > names for different lenses. There should thus be a single entry in the\n> > >> > > camera sensors properties table, and lens information should be reported\n> > >> > > separately.\n> > >> >\n> > >> > I agree this is not the tidiest thing to do. Right now, we have no other way of\n> > >> > getting the variant details from the kernel driver into userland. I believe\n> > >> > @Dave Stevenson raised this topic at the last Embedded Linux Conference, but no\n> > >> > consensus was reached. Once we have a mechanism in the kernel to report lens\n> > >> > properties, we will remove this workaround. Would a \\todo suffice for now?\n> > >>\n> > >> If adding a \\todo was enough to make a mechanism for this appear\n> > >> upstream by magic, I'd say it's fine. Unfortunately, that doesn't match\n> > >> my experience :-)\n> > >\n> > >\n> > > If only! :)\n> > \n> > I'll acknowledge that time was getting short at the media summit.\n> > \n> > The initial proposal was made as I knew these modules were coming. I'd\n> > suggested the addition of an optics descriptor in the media entitiy,\n> > not unlike the connector on the end of bridges in DRM. That would\n> > provide the basic properties of the optics, with the option of\n> > calibration too.\n> > The general response seemed to be along the lines \"oh no it's far too\n> > complicated to model, and needs to be done as a module description\".\n> > Seeing as a module description almost flies in the face of the current\n> > kernel frameworks describing the generic sensor and VCM drivers, I\n> > have no concept of the framework being anticipated and how it fits\n> > into the current world. Add in the complication of ACPI as well as DT,\n> > and all bets are off.\n> \n> Sakari, do you have any insight on how this is handled in ACPI (if at\n> all) ?\n\nNot at all I'm afraid.\n\nWith more generic user space, I expect there to be more and more needs to\nconvey information from the system firmware to the user space. I would use\ncontrols, especially for passing individual parameters that we're mostly\ndealing with.\n\nThe difficulty comes from the fact that these parameters are not often\nknown so the user space can't really rely on them being there. Still the\nonly way to address this is to start passing this information to the user\nspace.\n\nIn DT (nor ACPI) we haven't had even a concept of the module as it's been\nviewed as a \"box\" which you just throw things into. Perhaps it's time to\nstart changing that. I'd begin with DT.\n\nThe camera sensor is central to the model as you always have one, so we\ncould continue to rely on that I think. There are cases where a single\nmodule contains multiple sensors, perhaps we could have the module as a\nparent node in that case.\n\nThis also overlaps with power on / off sequences that are module specific,\nbut there are very large numbers of different modules...\n\n> \n> > Yourself (Laurent) and Sakari are the two main gatekeepers of what is\n> > merged to mainline, so it's down to what satisfies you. As you both\n> > shot down the initial proposal with no greater context, it leaves 3rd\n> > parties totally at sea as to the use cases that were considered as not\n> > covered, or any indication of what would be acceptable. That's not a\n> > great motivator to invest time developing a proposal.\n> > \n> > There needs to be an initial discussion including both of you to\n> > formulate a basic direction on the subject, otherwise it's wasted\n> > effort. Until then you'll get vendors doing their own thing, and their\n> > use of libcamera will inherently fragment.\n> \n> Then let's have that discussion.\n> \n> I'm afraid that support for the whole camera ecosystem can't rest on the\n> shoulders of two individuals only, with a whole industry working in a\n> fire-and-forget mode, and Sakari and I left to fix everything. It can't\n> scale, and it just wouldn't be fair.\n> \n> > >> One middleground option would be to merge the first \"variant\" (the\n> > >> \"imx708\") when the driver gets posted to the linux-media mailing list,\n> > >> while the rest gets developed, but in any case it will require someone\n> > >> making a proposal upstream.\n> > >\n> > > We will post a variant-less driver to the mailing list and chat with Dave about\n> > > a proposal for module variant reporting.\n> > >\n> > > The reason we added the other variants in the CameraSensorProperties was to\n> > > quiet the warning log message when a device is not listed. Would it be ok to\n> > > keep all variants listed like this with a \\todo to clean-up? If not, we will have to\n> > > make this change in our downstream tree which we are trying hard to deprecate :(\n> > >\n> > >> > > > Reviewed-by: Kieran Bingham \n> > >> > > >\n> > >> > > > > Signed-off-by: Nick Hollinghurst \n> > >> > > > > Signed-off-by: Naushir Patuck \n> > >> > > > > Reviewed-by: Naushir Patuck \n> > >> > > > > Reviewed-by: David Plowman \n> > >> > > > > ---\n> > >> > > > > src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++\n> > >> > > > > 1 file changed, 16 insertions(+)\n> > >> > > > >\n> > >> > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > >> > > > > index c3c2caced906..3afd500ea3be 100644\n> > >> > > > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > >> > > > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > >> > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> > > > > */\n> > >> > > > > },\n> > >> > > > > } },\n> > >> > > > > + { \"imx708\", {\n> > >> > > > > + .unitCellSize = { 1400, 1400 },\n> > >> > > > > + .testPatternModes = {},\n> > >> > > > > + } },\n> > >> > > > > + { \"imx708_noir\", {\n> > >> > > > > + .unitCellSize = { 1400, 1400 },\n> > >> > > > > + .testPatternModes = {},\n> > >> > > > > + } },\n> > >> > > > > + { \"imx708_wide\", {\n> > >> > > > > + .unitCellSize = { 1400, 1400 },\n> > >> > > > > + .testPatternModes = {},\n> > >> > > > > + } },\n> > >> > > > > + { \"imx708_wide_noir\", {\n> > >> > > > > + .unitCellSize = { 1400, 1400 },\n> > >> > > > > + .testPatternModes = {},\n> > >> > > > > + } },\n> > >> > > > > { \"ov2740\", {\n> > >> > > > > .unitCellSize = { 1400, 1400 },\n> > >> > > > > .testPatternModes = {\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 339C2BDB1D\n\tfor ;\n\tMon, 23 Jan 2023 14:09:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93EAE625E4;\n\tMon, 23 Jan 2023 15:09:35 +0100 (CET)","from meesny.iki.fi (meesny.iki.fi [IPv6:2001:67c:2b0:1c1::201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C849603C7\n\tfor ;\n\tMon, 23 Jan 2023 15:09:34 +0100 (CET)","from hillosipuli.retiisi.eu (82-181-192-243.bb.dnainternet.fi\n\t[82.181.192.243])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (2048 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: sailus)\n\tby meesny.iki.fi (Postfix) with ESMTPSA id D5D39200C5;\n\tMon, 23 Jan 2023 16:09:32 +0200 (EET)","from valkosipuli.retiisi.eu (valkosipuli.localdomain [192.168.4.2])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256) (No client certificate requested)\n\tby hillosipuli.retiisi.eu (Postfix) with ESMTPS id 31DF0634C91;\n\tMon, 23 Jan 2023 16:09:02 +0200 (EET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674482975;\n\tbh=AzhXNM7i68iu8T5brrWkBNxSJN+RpMWxb6DNsejINrE=;\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=r8E4pmZ2rv2nZhS70bt5WlJZHHlr4y4NgfAmcPEOIuhfkCNyoQxGb7Z8hiz203ZhJ\n\tUSzDQdG3cXC2ZCRGrqbcboFPYFa2i4XU7baUGvkR1Q4WY9VvWULpR7wbC/lAAN6sMx\n\tjvZtL5pSL0PD+J52Vn+aT9s5p3srOUeoZ0QsH9Vzq0TkTVtCM45LaWoGaFFdE/bKYM\n\t69vrVPGx0Jd8f2eDo/h3vchHjYpOGox7pmdkFl2lSBBJErsOcGqU33T2GvvFcT6Utj\n\tSl9TOL1znN8FFrRy4kvLU4CR+gMXAawOk9V2zkJ9yLpn/Flv9sHY1AG74adFnVaW5B\n\tqIYqouQ7wxXOw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny;\n\tt=1674482973;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=hVgQSEhfodDaamjGPLN4SH76zQgnOFepWQNJsujuT/k=;\n\tb=rQqKTl37hG3FeWM9bDU9Hnwa7xx76xS9OLVcLAbftkBIGdsydrRUODeyOf2+3782KdV5GV\n\tE1BXkJrhr6qRL9DkNh2gBDW9zSGDKA5U8SxDD+hFN/H8W9hetlCGJ4LL0TPiAUyGGBGw/L\n\tS2kiuEer9RW3ArdN4++kJCjINpR5uoo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=iki.fi header.i=@iki.fi\n\theader.b=\"rQqKTl37\"; dkim-atps=neutral","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi;\n\ts=meesny; t=1674482973;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=hVgQSEhfodDaamjGPLN4SH76zQgnOFepWQNJsujuT/k=;\n\tb=FISU8PXvW8l7YPYwtGOhbP0IxCFZ6Dyi7sq1P7qT/dlAopmsebO7AL/IkXG4Mz17P0pcuM\n\tEUyco0gLZVRzWykyNifLznEdbLy53ui4Jhu/c9Ne6Yek2IAF/pD7k4WLkeNSkx1mccu+3K\n\tz6kp14wvjyjwsThelI0JpkFmG5qNlH0=","ARC-Authentication-Results":"i=1; ORIGINATING;\n\tauth=pass smtp.auth=sailus smtp.mailfrom=sakari.ailus@iki.fi","ARC-Seal":"i=1; s=meesny; d=iki.fi; t=1674482973; a=rsa-sha256; cv=none;\n\tb=T6h1h0GGEXHGWdhZEKJn4nAcCB85QY4cq88rNH4NCiy6NWRbqSISaqmBukablb02TcWMER\n\tdtip9ebDgjLZR7KcCbqGDWXW9haGJsk8Bxg2lZKhB2ckf6b6W+zKQUvJzXPy/STMScQnS3\n\td+eTryh/lsPYM2bO+k+mUjlYKpEEc4o=","Date":"Mon, 23 Jan 2023 16:09:02 +0200","To":"Laurent Pinchart ","Message-ID":"","References":"<20230119104544.9456-1-naush@raspberrypi.com>\n\t<20230119104544.9456-14-naush@raspberrypi.com>\n\t<167413338494.42371.14345994623895785637@Monstersaurus>\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor:\n\tAdd Sony IMX708 sensor properties","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":"Sakari Ailus via libcamera-devel ","Reply-To":"Sakari Ailus ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]