[{"id":16661,"web_url":"https://patchwork.libcamera.org/comment/16661/","msgid":"<93da7052-61e1-74b5-58d2-0dcd7c8dd4aa@ideasonboard.com>","date":"2021-04-27T20:17:55","subject":"Re: [libcamera-devel] [PATCH v4 0/4] Raspberry Pi: handle sensors\n\tmore flexibly","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 27/04/2021 14:08, David Plowman wrote:\n> Hi\n> \n> Here's a version 4 of this patch set. The only difference over v3 is a\n> new penultimate patch that adds new V4L2 controls for informing the\n> sensor of the colour gains.\n> \n> I've actually added a control for each of the 4 Bayer colour channels\n> even though I use only two of them. It feels like one of those things\n> where some sensor may eventually want the ones that I might have been\n> tempted to miss out, at which point why not just include them? Though\n> I still wonder if having both GREENR and GREENB is overkill, and just\n> GREEN would have sufficed. Any thoughts on that?\n\nI think adding them all in now makes more sense than adding some in now,\nand then finding the others are needed but the numbering is no longer\nconsecutive because a control has been added in the meanwhile.\n\nWould there ever be a (non-bayer?) sensor that would use only R,G,B? In\nwhich case, I would wonder which GREEN should be used... but I suspect\nthat's not really an issue that will occur if sensors that need this\ndata are always going to be some form of bayer.\n\n\n> Once we're happy with the changes here I can apply them in the\n> Raspberry Pi Linux distribution, and then we can look to upstream them\n> too.\n\nI would suggest doing this the other way around.\n\nThe V4L2 patch should already be posted to the V4L2 mailing list, to get\nearly review comments.\n\n\"Upstream first\" is always better ;-)\n\n--\nKieran\n\n\n> Thanks in advance\n> \n> David\n> \n> David Plowman (4):\n> ipa: raspberrypi: Make CamHelper exposure methods virtual\n> ipa: raspberrypi: Add CamHelper::ColourGainCode method\n> include: linux: Add V4L2_CID_NOTIFY_GAIN_XXX controls\n> ipa: raspberrypi: Update sensor's V4L2_CID_NOTIFY_GAIN_RED/BLUE\n> controls when present\n> \n> include/libcamera/ipa/raspberrypi.mojom | 1 +\n> include/linux/v4l2-controls.h | 4 ++++\n> src/ipa/raspberrypi/cam_helper.cpp | 18 ++++++++++++++++++\n> src/ipa/raspberrypi/cam_helper.hpp | 8 +++++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 13 +++++++++++++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++\n> 6 files changed, 51 insertions(+), 3 deletions(-)\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 A86D8BDE3F\n\tfor ;\n\tTue, 27 Apr 2021 20:18:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD8BC68866;\n\tTue, 27 Apr 2021 22:17:59 +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 06301602C2\n\tfor ;\n\tTue, 27 Apr 2021 22:17:59 +0200 (CEST)","from [192.168.0.20]\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 6F784E9;\n\tTue, 27 Apr 2021 22:17:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jX1pzlg1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619554678;\n\tbh=NLZEF11Zwsgn+AvlDIcmISXC15T5Eu1Tbids1KYYZ0g=;\n\th=Reply-To:To:References:From:Subject:Date:In-Reply-To:From;\n\tb=jX1pzlg1FY7F95p2Tuy6hn+xU4p1AKC5T+O7OGN+TmNnMmbvZR95NbRVNVM0/fTIV\n\tFftihDB2ryk9GWu3aq+W+UEWcmKpb47NYirxILpkqqiXxF4+9QGfLJFBJBT7sF5jwy\n\tI795hMohpdY3DDxg2ErEuZpjbREDlr52v/ePO2sY=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210427130844.11357-1-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<93da7052-61e1-74b5-58d2-0dcd7c8dd4aa@ideasonboard.com>","Date":"Tue, 27 Apr 2021 21:17:55 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210427130844.11357-1-david.plowman@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 0/4] Raspberry Pi: handle sensors\n\tmore flexibly","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16669,"web_url":"https://patchwork.libcamera.org/comment/16669/","msgid":"","date":"2021-04-28T11:35:49","subject":"Re: [libcamera-devel] [PATCH v4 0/4] Raspberry Pi: handle sensors\n\tmore flexibly","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Apr 27, 2021 at 09:17:55PM +0100, Kieran Bingham wrote:\n> Hi David,\n> \n> On 27/04/2021 14:08, David Plowman wrote:\n> > Hi\n> > \n> > Here's a version 4 of this patch set. The only difference over v3 is a\n> > new penultimate patch that adds new V4L2 controls for informing the\n> > sensor of the colour gains.\n> > \n> > I've actually added a control for each of the 4 Bayer colour channels\n> > even though I use only two of them. It feels like one of those things\n> > where some sensor may eventually want the ones that I might have been\n> > tempted to miss out, at which point why not just include them? Though\n> > I still wonder if having both GREENR and GREENB is overkill, and just\n> > GREEN would have sufficed. Any thoughts on that?\n> \n> I think adding them all in now makes more sense than adding some in now,\n> and then finding the others are needed but the numbering is no longer\n> consecutive because a control has been added in the meanwhile.\n> \n> Would there ever be a (non-bayer?) sensor that would use only R,G,B? In\n> which case, I would wonder which GREEN should be used... but I suspect\n> that's not really an issue that will occur if sensors that need this\n> data are always going to be some form of bayer.\n> \n> > Once we're happy with the changes here I can apply them in the\n> > Raspberry Pi Linux distribution, and then we can look to upstream them\n> > too.\n> \n> I would suggest doing this the other way around.\n> \n> The V4L2 patch should already be posted to the V4L2 mailing list, to get\n> early review comments.\n> \n> \"Upstream first\" is always better ;-)\n\n+1. We can merge things in libcamera without waiting for upstream in\nsome cases, but the code has to be on its way to upstream. In this\nparticular case, it means that the patches have to be posted to the\nlinux-media mailign list, with at least a general agreement on the\ndirection. Documentation for the new controls is thus needed.\n\nSmall details and upstreaming delays due to the schedule of the upstream\nkernel merge windows are not blockers, we can add the controls to the\nlibcamera copy of the kernel headers once they're approved in principle\nby upstream.\n\n> > David Plowman (4):\n> > ipa: raspberrypi: Make CamHelper exposure methods virtual\n> > ipa: raspberrypi: Add CamHelper::ColourGainCode method\n> > include: linux: Add V4L2_CID_NOTIFY_GAIN_XXX controls\n> > ipa: raspberrypi: Update sensor's V4L2_CID_NOTIFY_GAIN_RED/BLUE\n> > controls when present\n> > \n> > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > include/linux/v4l2-controls.h | 4 ++++\n> > src/ipa/raspberrypi/cam_helper.cpp | 18 ++++++++++++++++++\n> > src/ipa/raspberrypi/cam_helper.hpp | 8 +++++---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 13 +++++++++++++\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++\n> > 6 files changed, 51 insertions(+), 3 deletions(-)","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 12E95BDE41\n\tfor ;\n\tWed, 28 Apr 2021 11:35:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C08DA688DC;\n\tWed, 28 Apr 2021 13:35:55 +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 27415688D9\n\tfor ;\n\tWed, 28 Apr 2021 13:35:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AB53B9F0;\n\tWed, 28 Apr 2021 13:35:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"X+fLFn4B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619609754;\n\tbh=HNjwPyyuuUD7vS97M3qVoXFj73y9Z86PDHs9X8iXmP8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X+fLFn4BSWgdPAL1yfUfpmHmWT/FBUdDy1p4OnRmQ7Qwqata1ZWARgHPlBATkBHFo\n\tFqDfGZ5HMJV4fc+0b0vvDs+jhlDYowH2k/6shmJ3S7AbnpwQPrcTIDBD6i6M7bvHa9\n\t/5j3cxunPqF6hCjtXqG7W4iIDJ4axtVoJ2i81lMs=","Date":"Wed, 28 Apr 2021 14:35:49 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210427130844.11357-1-david.plowman@raspberrypi.com>\n\t<93da7052-61e1-74b5-58d2-0dcd7c8dd4aa@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<93da7052-61e1-74b5-58d2-0dcd7c8dd4aa@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 0/4] Raspberry Pi: handle sensors\n\tmore flexibly","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16772,"web_url":"https://patchwork.libcamera.org/comment/16772/","msgid":"","date":"2021-05-05T14:16:19","subject":"Re: [libcamera-devel] [PATCH v4 0/4] Raspberry Pi: handle sensors\n\tmore flexibly","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi\n\nThanks for the feedback everyone.\n\nI think it probably makes sense to drop this entire set on the\nfollowing grounds:\n\n* We're not doing the virtual exposure method thing, at least, not\nunless we find ourselves boxed into a corner with no other way out!\nBut watch out for another looming metadiscussion on what to do about\nper-mode sensitivities instead... :)\n\n* The V4L2_CID_NOTIFY_GAIN stuff wants to wait until there's been some\nprogress on the linux media mailing list. Hopefully I'll get round to\nthat soon now.\n\n* I don't see any benefit adding the ColourGainCode method until we're using it.\n\nBest regards\n\nDavid\n\nOn Wed, 28 Apr 2021 at 12:35, Laurent Pinchart\n wrote:\n>\n> On Tue, Apr 27, 2021 at 09:17:55PM +0100, Kieran Bingham wrote:\n> > Hi David,\n> >\n> > On 27/04/2021 14:08, David Plowman wrote:\n> > > Hi\n> > >\n> > > Here's a version 4 of this patch set. The only difference over v3 is a\n> > > new penultimate patch that adds new V4L2 controls for informing the\n> > > sensor of the colour gains.\n> > >\n> > > I've actually added a control for each of the 4 Bayer colour channels\n> > > even though I use only two of them. It feels like one of those things\n> > > where some sensor may eventually want the ones that I might have been\n> > > tempted to miss out, at which point why not just include them? Though\n> > > I still wonder if having both GREENR and GREENB is overkill, and just\n> > > GREEN would have sufficed. Any thoughts on that?\n> >\n> > I think adding them all in now makes more sense than adding some in now,\n> > and then finding the others are needed but the numbering is no longer\n> > consecutive because a control has been added in the meanwhile.\n> >\n> > Would there ever be a (non-bayer?) sensor that would use only R,G,B? In\n> > which case, I would wonder which GREEN should be used... but I suspect\n> > that's not really an issue that will occur if sensors that need this\n> > data are always going to be some form of bayer.\n> >\n> > > Once we're happy with the changes here I can apply them in the\n> > > Raspberry Pi Linux distribution, and then we can look to upstream them\n> > > too.\n> >\n> > I would suggest doing this the other way around.\n> >\n> > The V4L2 patch should already be posted to the V4L2 mailing list, to get\n> > early review comments.\n> >\n> > \"Upstream first\" is always better ;-)\n>\n> +1. We can merge things in libcamera without waiting for upstream in\n> some cases, but the code has to be on its way to upstream. In this\n> particular case, it means that the patches have to be posted to the\n> linux-media mailign list, with at least a general agreement on the\n> direction. Documentation for the new controls is thus needed.\n>\n> Small details and upstreaming delays due to the schedule of the upstream\n> kernel merge windows are not blockers, we can add the controls to the\n> libcamera copy of the kernel headers once they're approved in principle\n> by upstream.\n>\n> > > David Plowman (4):\n> > > ipa: raspberrypi: Make CamHelper exposure methods virtual\n> > > ipa: raspberrypi: Add CamHelper::ColourGainCode method\n> > > include: linux: Add V4L2_CID_NOTIFY_GAIN_XXX controls\n> > > ipa: raspberrypi: Update sensor's V4L2_CID_NOTIFY_GAIN_RED/BLUE\n> > > controls when present\n> > >\n> > > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > > include/linux/v4l2-controls.h | 4 ++++\n> > > src/ipa/raspberrypi/cam_helper.cpp | 18 ++++++++++++++++++\n> > > src/ipa/raspberrypi/cam_helper.hpp | 8 +++++---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 13 +++++++++++++\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++\n> > > 6 files changed, 51 insertions(+), 3 deletions(-)\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 2B2EEBDE79\n\tfor ;\n\tWed, 5 May 2021 14:16:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BC6668909;\n\tWed, 5 May 2021 16:16:33 +0200 (CEST)","from mail-wr1-x429.google.com (mail-wr1-x429.google.com\n\t[IPv6:2a00:1450:4864:20::429])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD51C68901\n\tfor ;\n\tWed, 5 May 2021 16:16:31 +0200 (CEST)","by mail-wr1-x429.google.com with SMTP id s8so2023167wrw.10\n\tfor ;\n\tWed, 05 May 2021 07:16:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ZFGPRUtN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=SGgzuup0F33ZRnSPiQYuhpJMZowIUymaRKA0BcFeQUE=;\n\tb=ZFGPRUtN8SC7T9HP0affsIxjMji6JsACJWM9bmiTi87l87A85jKWQY9R+mIzbGfdpB\n\tlTJud6ZwO2zuE/ki44oMSpl/Tx/0eKCjes5iQRFUK1dFlA8s6vseGS669iB4YVWQsWZq\n\tScjU9VDW0ZX481GPvnExRBukRp0kYzGEaw5N3CnC0qnsxUxyjM333sRQc805hpjqm8q6\n\tTAnU63tVGNrbjPhzEkRZXnun7Z3hf4MPcqRVrKRwMX2TVLThAQPvPXJUlV4SVxyqyW4C\n\tOvmGi++jnVtR/A0THUQkmwhRMlqxIAWoer6Waga60fPEdauPoCkcaKgLaHOLQAmGLX6S\n\tlyDA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=SGgzuup0F33ZRnSPiQYuhpJMZowIUymaRKA0BcFeQUE=;\n\tb=hoqk4EHlpGaCHS95BUZHwqRKxQmrQheBjkucIiOp7zyUdRLbzMEyhKJXKoEkQtWfo8\n\tedLC1CLjbAZEBjrSlvCeLyoH1Km4DvmGHRIKm0CM9wPVc/csjHXIs0ps2ZEJSbc6GlC5\n\t/tLVeZHKMxmE3822OsmnBr5IHw3sF+jtkHQlAJqJr/qQ0CllW6PFbcKZDHmMOgClVnN9\n\tqHFmTSKLMQxqdb17M29e+zDylKHXa3cMUGd3eCaMhyMzCf+OHrJLn8nfmAooMIrCCB1L\n\tQ1j5UZYWlAfVLR9dJdg/65nMxk/0eRr1LWydsM+XCWCkH/5hbeJm4vEkLTPfWon7l9lS\n\tiycg==","X-Gm-Message-State":"AOAM532HdOwr4TnO0CheaCPHfWI8CGc+eLU4MSOb/0ItC9XSysWHnbrB\n\t04CNO+8zRBtu16HMRL73hAVf1hX69xEl0c/HmFQjEl/nuuU=","X-Google-Smtp-Source":"ABdhPJxGKUh8Bv0dAyuLdDWcOErXpgiGF5c+QUbVtKnkPDcHWRcVJ0B2mxfwPwb3NwieHnOAaxL5wwEb3v4NJGhQktI=","X-Received":"by 2002:a5d:408f:: with SMTP id\n\to15mr37315274wrp.89.1620224191475; \n\tWed, 05 May 2021 07:16:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210427130844.11357-1-david.plowman@raspberrypi.com>\n\t<93da7052-61e1-74b5-58d2-0dcd7c8dd4aa@ideasonboard.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 5 May 2021 15:16:19 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 0/4] Raspberry Pi: handle sensors\n\tmore flexibly","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]