[{"id":20145,"web_url":"https://patchwork.libcamera.org/comment/20145/","msgid":"<163404763530.2993659.1121836612493028956@Monstersaurus>","date":"2021-10-12T14:07:15","subject":"Re: [libcamera-devel] [PATCH] libcamera: Add Samsung S5K3L6XX\n\tsensor support","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Dorota,\n\nIt's great to see this progress on the Purism phone camera support.\n\nQuoting Dorota Czaplejewicz (2021-10-12 12:39:46)\n> The sensor is called s5k3l6xx in the kernel.\n> \n> The driver is currently out of tree and maintained by Purism.\n\nWe prefer kernel drivers to be either upstream, or at least on their way\nupstream. Ideally, a minimum of having been posted to the linux-media\nmailinglist, however we know that the review process can take time, so\nwe can integrate support to libcamera before it's fully upstream in the\nkernel to support the upstreaming process.\n\nIs this sensor driver something that you are intending to post to the\nlinux-media mailinglist in the near future, or has it been posted\nalready?\n\n> \n> It's the main camera sensor on the Librem 5.\n> \n> Signed-off-by: Dorota Czaplejewicz \n> ---\n> src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> index 48305ac4..0625a0d8 100644\n> --- a/src/libcamera/camera_sensor_properties.cpp\n> +++ b/src/libcamera/camera_sensor_properties.cpp\n> @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeColorBars, 1 },\n> },\n> } },\n> + { \"s5k3l6xx\", {\n> + .unitCellSize = { 1120, 1120 },\n> + .testPatternModes = {\n> + { 0, controls::draft::TestPatternModeOff },\n> + { 1, controls::draft::TestPatternModeSolidColor },\n> + { 2, controls::draft::TestPatternModeColorBars },\n> + { 3, controls::draft::TestPatternModeColorBarsFadeToGray },\n> + { 4, controls::draft::TestPatternModeCustom1 }, /* White */\n> + { 5, controls::draft::TestPatternModePn9 },\n> + { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */\n> + { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */\n\nThat's an interesting way to define the extra custom modes...\nInterstingly, I don't see any users of TestPatternModeCustom1 yet.\n\nWhat is LFSR32 and Address?\n\nIs 'White' just a solid colour of 'white'?\n\nI suspect these modes should be defined within\nsrc/libcamera/control_ids.yaml where possible. An all white could be\nTestPatternModeSolidColorWhite or TestPatternModeSolidWhite for intance.\n\n--\nKieran\n\n\n> + },\n> + } },\n> };\n> \n> const auto it = sensorProps.find(sensor);\n> -- \n> 2.31.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 105C1BDC71\n\tfor ;\n\tTue, 12 Oct 2021 14:07:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8003268F4F;\n\tTue, 12 Oct 2021 16:07:19 +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 211F768F4C\n\tfor ;\n\tTue, 12 Oct 2021 16:07:18 +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 A1B6EF1;\n\tTue, 12 Oct 2021 16:07:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JFpMObjt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634047637;\n\tbh=R2BjQz46eMxfdTFWFtPYnzi0gi71tAUFHx+cmmTNdYM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JFpMObjtrWsWoujfHb02vV/U2RQplK52Ssgyk5mfgz6UGovzepGYXvqtBdHLvZau+\n\t6s0Ko3A/abiLgi3GOteCtsFsVz/8DTOMz8Y8hDtnXIfgsaiK2wyg8Pv0YF4NDiD8ZT\n\tuXUPgxXeMi8eEqVIOu7n4Cx9uVwpfXadBAezlpjs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211012113946.484914-1-dorota.czaplejewicz@puri.sm>","References":"<20211012113946.484914-1-dorota.czaplejewicz@puri.sm>","From":"Kieran Bingham ","To":"Dorota Czaplejewicz ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 12 Oct 2021 15:07:15 +0100","Message-ID":"<163404763530.2993659.1121836612493028956@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Add Samsung S5K3L6XX\n\tsensor support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dorota Czaplejewicz ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20147,"web_url":"https://patchwork.libcamera.org/comment/20147/","msgid":"<20211012171038.0c37350d.dorota.czaplejewicz@puri.sm>","date":"2021-10-12T15:10:38","subject":"Re: [libcamera-devel] [PATCH] libcamera: Add Samsung S5K3L6XX\n\tsensor support","submitter":{"id":96,"url":"https://patchwork.libcamera.org/api/people/96/","name":"Dorota Czaplejewicz","email":"dorota.czaplejewicz@puri.sm"},"content":"Hi Kieran,\n\nOn Tue, 12 Oct 2021 15:07:15 +0100\nKieran Bingham wrote:\n> Quoting Dorota Czaplejewicz (2021-10-12 12:39:46)\n> > The sensor is called s5k3l6xx in the kernel.\n> > \n> > The driver is currently out of tree and maintained by Purism. \n> \n> We prefer kernel drivers to be either upstream, or at least on their way\n> upstream. Ideally, a minimum of having been posted to the linux-media\n> mailinglist, however we know that the review process can take time, so\n> we can integrate support to libcamera before it's fully upstream in the\n> kernel to support the upstreaming process.\n> \n> Is this sensor driver something that you are intending to post to the\n> linux-media mailinglist in the near future, or has it been posted\n> already?\n\nThe driver still has some deficiencies, so I haven't posted it yet. I expect I will within a couple months.\n> \n> > \n> > It's the main camera sensor on the Librem 5.\n> > \n> > Signed-off-by: Dorota Czaplejewicz \n> > ---\n> > src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++\n> > 1 file changed, 13 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > index 48305ac4..0625a0d8 100644\n> > --- a/src/libcamera/camera_sensor_properties.cpp\n> > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > { controls::draft::TestPatternModeColorBars, 1 },\n> > },\n> > } },\n> > + { \"s5k3l6xx\", {\n> > + .unitCellSize = { 1120, 1120 },\n> > + .testPatternModes = {\n> > + { 0, controls::draft::TestPatternModeOff },\n> > + { 1, controls::draft::TestPatternModeSolidColor },\n> > + { 2, controls::draft::TestPatternModeColorBars },\n> > + { 3, controls::draft::TestPatternModeColorBarsFadeToGray },\n> > + { 4, controls::draft::TestPatternModeCustom1 }, /* White */\n> > + { 5, controls::draft::TestPatternModePn9 },\nI just noticed those two are swapped.\n\n> > + { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */\n> > + { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */ \n> \n> That's an interesting way to define the extra custom modes...\n> Interstingly, I don't see any users of TestPatternModeCustom1 yet.\n\nI found it directly in the yaml file, where the description suggests this way of dealing with it.\n> \n> What is LFSR32 and Address?\nNeither is documented in the application note, but LFSR32 appears to be a pseudo-random algorithm: https://gist.github.com/edarc/2946448\n\n> \n> Is 'White' just a solid colour of 'white'?\n> \nYes, the maximum integer value is returned.\n\n> I suspect these modes should be defined within\n> src/libcamera/control_ids.yaml where possible. An all white could be\n> TestPatternModeSolidColorWhite or TestPatternModeSolidWhite for intance.\n> \n> --\n> Kieran\n> \n> \n> > + },\n> > + } },\n> > };\n> > \n> > const auto it = sensorProps.find(sensor);\n> > -- \n> > 2.31.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 5BE49BDC71\n\tfor ;\n\tTue, 12 Oct 2021 15:10:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2D8068F51;\n\tTue, 12 Oct 2021 17:10:55 +0200 (CEST)","from comms.puri.sm (comms.puri.sm [159.203.221.185])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D377268F4C\n\tfor ;\n\tTue, 12 Oct 2021 17:10:53 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n\tby comms.puri.sm (Postfix) with ESMTP id E58FEDFAC1;\n\tTue, 12 Oct 2021 08:10:51 -0700 (PDT)","from comms.puri.sm ([127.0.0.1])\n\tby localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id LqNAV4t3E-2i; Tue, 12 Oct 2021 08:10:51 -0700 (PDT)"],"Date":"Tue, 12 Oct 2021 17:10:38 +0200","From":"Dorota Czaplejewicz ","To":"Kieran Bingham ","Message-ID":"<20211012171038.0c37350d.dorota.czaplejewicz@puri.sm>","In-Reply-To":"<163404763530.2993659.1121836612493028956@Monstersaurus>","References":"<20211012113946.484914-1-dorota.czaplejewicz@puri.sm>\n\t<163404763530.2993659.1121836612493028956@Monstersaurus>","Organization":"Purism","MIME-Version":"1.0","Content-Type":"multipart/signed; boundary=\"Sig_/EiiE5SGVdwBuEpu35f5OisF\";\n\tprotocol=\"application/pgp-signature\"; micalg=pgp-sha256","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Add Samsung S5K3L6XX\n\tsensor support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20186,"web_url":"https://patchwork.libcamera.org/comment/20186/","msgid":"<163420033945.3654017.13500326592590424987@Monstersaurus>","date":"2021-10-14T08:32:19","subject":"Re: [libcamera-devel] [PATCH] libcamera: Add Samsung S5K3L6XX\n\tsensor support","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Dorota,\n\nQuoting Dorota Czaplejewicz (2021-10-12 16:10:38)\n> Hi Kieran,\n> \n> On Tue, 12 Oct 2021 15:07:15 +0100\n> Kieran Bingham wrote:\n> > Quoting Dorota Czaplejewicz (2021-10-12 12:39:46)\n> > > The sensor is called s5k3l6xx in the kernel.\n> > > \n> > > The driver is currently out of tree and maintained by Purism. \n> > \n> > We prefer kernel drivers to be either upstream, or at least on their way\n> > upstream. Ideally, a minimum of having been posted to the linux-media\n> > mailinglist, however we know that the review process can take time, so\n> > we can integrate support to libcamera before it's fully upstream in the\n> > kernel to support the upstreaming process.\n> > \n> > Is this sensor driver something that you are intending to post to the\n> > linux-media mailinglist in the near future, or has it been posted\n> > already?\n> \n> The driver still has some deficiencies, so I haven't posted it yet. I expect I will within a couple months.\n\nOk, it's understandable to still have some issues to work through.\n\nI'm sure you're already aware, but if there's anything you want support\non, you can post to linux-media and tag it as RFC. Highlight any known\ndeficiencies in the coverletter or comment section, and early review\nwill help make sure the driver is going in the right direction overall.\n\nA posted driver will make it easier to accept this patch, as we ideally\nneed something to review it against...\n\n\n> > > It's the main camera sensor on the Librem 5.\n> > > \n> > > Signed-off-by: Dorota Czaplejewicz \n> > > ---\n> > > src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++\n> > > 1 file changed, 13 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp\n> > > index 48305ac4..0625a0d8 100644\n> > > --- a/src/libcamera/camera_sensor_properties.cpp\n> > > +++ b/src/libcamera/camera_sensor_properties.cpp\n> > > @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > > { controls::draft::TestPatternModeColorBars, 1 },\n> > > },\n> > > } },\n> > > + { \"s5k3l6xx\", {\n> > > + .unitCellSize = { 1120, 1120 },\n> > > + .testPatternModes = {\n> > > + { 0, controls::draft::TestPatternModeOff },\n> > > + { 1, controls::draft::TestPatternModeSolidColor },\n> > > + { 2, controls::draft::TestPatternModeColorBars },\n> > > + { 3, controls::draft::TestPatternModeColorBarsFadeToGray },\n> > > + { 4, controls::draft::TestPatternModeCustom1 }, /* White */\n> > > + { 5, controls::draft::TestPatternModePn9 },\n> I just noticed those two are swapped.\n\nFor instance, without a driver posted, or at least referenced, I can't\nreview these control defintions ...\n\n> > > + { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */\n> > > + { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */ \n> > \n> > That's an interesting way to define the extra custom modes...\n> > Interstingly, I don't see any users of TestPatternModeCustom1 yet.\n> \n> I found it directly in the yaml file, where the description suggests this way of dealing with it.\n\nIt does seem reasonable to 'add offsets' to TestPatternModeCustom1\ninitially, but given that the control is called\n\"TestPatternModeCustom1\", I'd expect TestPatternModeCustom2 and\nTestPatternModeCustom3 if they were really required. Otherwise, we\nshould probably call it TestPatternModeCustom and always require a\ncustom offset.\n\nBut I would hope we would really try to avoid 'Custom' controls like\nthis unless they are absolutley necessary. Adding a new test pattern\nmode here is easy. It just needs a new definition in\nsrc/libcamera/control_ids.yaml with a description of the mode.\n\nFor instance, the LFSR32 could be added like the Pn9:\n\n - name: TestPatternModeLfsr32\n value: 5\n description: |\n All pixel data is replaced by a pseudo-random sequence generated\n from an LFSR32 sequence generator.\n\n\n> > \n> > What is LFSR32 and Address?\n> Neither is documented in the application note, but LFSR32 appears to be a pseudo-random algorithm: https://gist.github.com/edarc/2946448\n\nIt might be more difficult to document TestPatternModeAddress, but can\nyou generate a raw output with that mode set yet?\n\nIf so - a quick examination might make it obvious that it's a sequential\nnumber relating to the address of the pixel perhaps - or it might be\nsomething else of course. But a test capture is likely the only way for\nus to find out.\n\n\n\n> > \n> > Is 'White' just a solid colour of 'white'?\n> > \n> Yes, the maximum integer value is returned.\n\nThis sounds like it is certainly worthy of it's own named mode, and\ncould be expected to be a mode used by other sensors. A clear name will\nhelp identifying the mode too.\n\n--\nKieran\n\n\n> > I suspect these modes should be defined within\n> > src/libcamera/control_ids.yaml where possible. An all white could be\n> > TestPatternModeSolidColorWhite or TestPatternModeSolidWhite for intance.\n> > \n> > --\n> > Kieran\n> > \n> > \n> > > + },\n> > > + } },\n> > > };\n> > > \n> > > const auto it = sensorProps.find(sensor);\n> > > -- \n> > > 2.31.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 36E98C323E\n\tfor ;\n\tThu, 14 Oct 2021 08:32:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E39D68F4F;\n\tThu, 14 Oct 2021 10:32:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7DBC368F4A\n\tfor ;\n\tThu, 14 Oct 2021 10:32:22 +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 164861B48;\n\tThu, 14 Oct 2021 10:32:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Akoaa82J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634200342;\n\tbh=TlWgwqeDISLJ6CR5nUniIYCWkok680eee8Sv6QRSX8I=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Akoaa82JrWx1dfwMHUwRCnT4F4Wy1JVCoyA8QltvccCm96jQ7fiBtYq4mU6YIcwwQ\n\tdZGSOWZUVWe4cWIGbdRz5tQ/BFfGrGf/G0OlqRYv/RX2E9S+gVu3ZyhvD8bdwIJwYZ\n\trIKol7GUK5B8FNRX01j3l7vPMQLLUbZ4q6r6lXsg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211012171038.0c37350d.dorota.czaplejewicz@puri.sm>","References":"<20211012113946.484914-1-dorota.czaplejewicz@puri.sm>\n\t<163404763530.2993659.1121836612493028956@Monstersaurus>\n\t<20211012171038.0c37350d.dorota.czaplejewicz@puri.sm>","From":"Kieran Bingham ","To":"Dorota Czaplejewicz ","Date":"Thu, 14 Oct 2021 09:32:19 +0100","Message-ID":"<163420033945.3654017.13500326592590424987@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Add Samsung S5K3L6XX\n\tsensor support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]