[{"id":26810,"web_url":"https://patchwork.libcamera.org/comment/26810/","msgid":"<20230403085053.hbrdun5rll4d7pew@uno.localdomain>","date":"2023-04-03T08:50:53","subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel,\n\nOn Fri, Mar 31, 2023 at 10:19:21AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Extend the IPA init() function by additional lensControls input\n> parameter. Check in pipeline handler if camera lens exists, and expose\n> its controls to the IPA using the new parameter.\n>\n> Signed-off-by: Daniel Semkowicz \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> include/libcamera/ipa/rkisp1.mojom | 3 ++-\n> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> 3 files changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index 1009e970..d4ff1230 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {\n> \tinit(libcamera.IPASettings settings,\n> \t uint32 hwRevision,\n> \t libcamera.IPACameraSensorInfo sensorInfo,\n> -\t libcamera.ControlInfoMap sensorControls)\n> +\t libcamera.ControlInfoMap sensorControls,\n> +\t libcamera.ControlInfoMap lensControls)\n> \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \tstart() => (int32 ret);\n> \tstop();\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 6544c925..d338d374 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -7,6 +7,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -52,6 +53,7 @@ public:\n> \tint init(const IPASettings &settings, unsigned int hwRevision,\n> \t\t const IPACameraSensorInfo &sensorInfo,\n> \t\t const ControlInfoMap &sensorControls,\n> +\t\t const ControlInfoMap &lensControls,\n> \t\t ControlInfoMap *ipaControls) override;\n> \tint start() override;\n> \tvoid stop() override;\n> @@ -80,6 +82,7 @@ private:\n> \tstd::map mappedBuffers_;\n>\n> \tControlInfoMap sensorControls_;\n> +\tstd::optional lensControls_;\n>\n> \t/* revision-specific data */\n> \trkisp1_cif_isp_version hwRevision_;\n> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const\n> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \t\t const IPACameraSensorInfo &sensorInfo,\n> \t\t const ControlInfoMap &sensorControls,\n> +\t\t const ControlInfoMap &lensControls,\n> \t\t ControlInfoMap *ipaControls)\n> {\n> \t/* \\todo Add support for other revisions */\n> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \tcontext_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> \t\t\t\t\t\t * 1.0s / sensorInfo.pixelRate;\n>\n> +\tif (!lensControls.empty())\n> +\t\tlensControls_ = lensControls;\n> +\n> \t/* Load the tuning data file. */\n> \tFile file(settings.configurationFile);\n> \tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 8a30fe06..f966254a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -32,6 +32,7 @@\n> #include \n>\n> #include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/camera_lens.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/delayed_controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t\treturn ret;\n> \t}\n>\n> +\tControlInfoMap lensControls;\n> +\tCameraLens *lens = sensor_->focusLens();\n> +\tif (lens)\n> +\t\tlensControls = lens->controls();\n> +\n> \tret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> -\t\t\t sensorInfo, sensor_->controls(), &controlInfo_);\n> +\t\t\t sensorInfo, sensor_->controls(), lensControls,\n> +\t\t\t &controlInfo_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> --\n> 2.39.2\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 C0514C326B\n\tfor ;\n\tMon, 3 Apr 2023 08:50:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0513862724;\n\tMon, 3 Apr 2023 10:50: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 E408F61EC6\n\tfor ;\n\tMon, 3 Apr 2023 10:50:57 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5AA486CF;\n\tMon, 3 Apr 2023 10:50:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680511859;\n\tbh=BQ2MnQflnkAAdPpPRRBEBxowsEbLngiJ6xpDbBz3xJ8=;\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=nv3c16GR9U2CSHV6PgPrisvMjXafKRG88VO364F4j25GVwD8geNAK/9Q3ss9ZwyAo\n\tkrRel074pmUztr5BjT5M6QiCyD0NKheWStDPTw/6GrUFwP3XDmChX+A0bZvhpCCpCK\n\tkaXMZN23zxbXf4Gu5+tlAA/3rEQsmwmAydsQbf2wUKnOECbIdN7HwTEGGj0HIGVJYV\n\tP8dtbiMPU0Qzdf7vswn96mnh1reZ0EtjenK/Tgr0P9A2ZCRxH7jii1Fgou7V5TPvQI\n\tREN4Nz1bgvFSITywOni8tggdktdzqDpbg9j2Z1g4t6HFDsYNs2m/ipSNOz5dL3FWAe\n\ty0ZE4flOVCHOQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680511857;\n\tbh=BQ2MnQflnkAAdPpPRRBEBxowsEbLngiJ6xpDbBz3xJ8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XxRHATaXaonjh5BGKl1lqT6QeFHbBgf5zt4tbwSCsIUl5d9NkaAUV9UBpdw53988y\n\t1a/mz3P3HeBrYcYI3WxYmj4Lue9lwaettLiGH/3CVHIun03g5BFl/n5FSkVHP1mTpe\n\tntERwEE7BpBmPUzWZVOjfC6zKXHFMaZfhPULIxNM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XxRHATaX\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 10:50:53 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230403085053.hbrdun5rll4d7pew@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-2-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230331081930.19289-2-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27080,"web_url":"https://patchwork.libcamera.org/comment/27080/","msgid":"","date":"2023-05-11T12:12:58","subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Laurent,\n\nOn Tue, Apr 25, 2023 at 3:29 PM Laurent Pinchart\n wrote:\n>\n> Hi Daniel,\n>\n> Thank you for the patch.\n>\n> On Fri, Mar 31, 2023 at 10:19:21AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Extend the IPA init() function by additional lensControls input\n> > parameter. Check in pipeline handler if camera lens exists, and expose\n> > its controls to the IPA using the new parameter.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 3 ++-\n> > src/ipa/rkisp1/rkisp1.cpp | 7 +++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> > 3 files changed, 17 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index 1009e970..d4ff1230 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -17,7 +17,8 @@ interface IPARkISP1Interface {\n> > init(libcamera.IPASettings settings,\n> > uint32 hwRevision,\n> > libcamera.IPACameraSensorInfo sensorInfo,\n> > - libcamera.ControlInfoMap sensorControls)\n> > + libcamera.ControlInfoMap sensorControls,\n> > + libcamera.ControlInfoMap lensControls)\n> > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > start() => (int32 ret);\n> > stop();\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 6544c925..d338d374 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -52,6 +53,7 @@ public:\n> > int init(const IPASettings &settings, unsigned int hwRevision,\n> > const IPACameraSensorInfo &sensorInfo,\n> > const ControlInfoMap &sensorControls,\n> > + const ControlInfoMap &lensControls,\n> > ControlInfoMap *ipaControls) override;\n> > int start() override;\n> > void stop() override;\n> > @@ -80,6 +82,7 @@ private:\n> > std::map mappedBuffers_;\n> >\n> > ControlInfoMap sensorControls_;\n> > + std::optional lensControls_;\n> >\n> > /* revision-specific data */\n> > rkisp1_cif_isp_version hwRevision_;\n> > @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const\n> > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > const IPACameraSensorInfo &sensorInfo,\n> > const ControlInfoMap &sensorControls,\n> > + const ControlInfoMap &lensControls,\n> > ControlInfoMap *ipaControls)\n> > {\n> > /* \\todo Add support for other revisions */\n> > @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> > * 1.0s / sensorInfo.pixelRate;\n> >\n> > + if (!lensControls.empty())\n> > + lensControls_ = lensControls;\n>\n> Is there a reason to use std::optional<> for lensControls_, instead of a\n> plain ControlInfoMap that you would unconditionally initialize here to\n>\n> lensControls_ = lensControls;\n>\n> ? When checking if the lens controls exist in further patches in the\n> series, you could test\n>\n> if (!lensControls_.empty())\n>\n> instead of\n>\n> if (lensControls_)\n>\n> I'm fine with std::optional<> if there's a reason for it, so, either\n> way,\n\nYou are right that I could just test \"lensControls_.empty()\".\nThe only reason I use the std::optional here, is that I wanted to make it clear\nin the IPA code, that lens existence is optional. Unfortunately, there is still\none check for \"lensControls.empty()\" in the IPA IPARkISP1::init() function.\nThe best would be to have std::optional already passed as the init() argument,\nbut I did not find an easy way to do this in mojom file... This way, We could\nset the optional lens controls parameter only if sensor_->focusLens() is\navailable. Right now, I base a bit on the underlying behavior of CameraLens that\nCameraLens::controls() always returns at least one element if lens is available.\n\nBest regards\nDaniel\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +\n> > /* Load the tuning data file. */\n> > File file(settings.configurationFile);\n> > if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 8a30fe06..f966254a 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -32,6 +32,7 @@\n> > #include \n> >\n> > #include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/camera_lens.h\"\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/delayed_controls.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > return ret;\n> > }\n> >\n> > + ControlInfoMap lensControls;\n> > + CameraLens *lens = sensor_->focusLens();\n> > + if (lens)\n> > + lensControls = lens->controls();\n> > +\n> > ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > - sensorInfo, sensor_->controls(), &controlInfo_);\n> > + sensorInfo, sensor_->controls(), lensControls,\n> > + &controlInfo_);\n> > if (ret < 0) {\n> > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > return ret;\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 7FDA1BD16B\n\tfor ;\n\tThu, 11 May 2023 12:13:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DCDF0633B4;\n\tThu, 11 May 2023 14:13:11 +0200 (CEST)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3750360548\n\tfor ;\n\tThu, 11 May 2023 14:13:10 +0200 (CEST)","by mail-ed1-x533.google.com with SMTP id\n\t4fb4d7f45d1cf-50bc3088b7aso16100251a12.3\n\tfor ;\n\tThu, 11 May 2023 05:13:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683807191;\n\tbh=qokf4XAoKIuUErmtLvk/DYun79eN3pBwr6DYAm1uSto=;\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=e5SnIqzVCLC2Hgybt0FwB1hXGjz7Jf+UxZFAPE3PgWTXPxJ4CJfLc8EnlRgSzPxYm\n\tf16R3O3HQro2ngNJQfnpjvnn5GULC9Xp9Nbw7jbJYYQ6cB08xKeaiWSbdF5sizo2oT\n\tFJTN1SiJMbeOpmECssItuhVOzOD8UZ0+u9mbY/fj/rr+o9kSMy3V00eZ76VKVVkmTW\n\tHtdawTZ5iL1DZTgc+GgdpJmhmugWqT2pv/OqdQLhl7YaWIeUXMHzj8tAOP4IGWEuSg\n\t+BXf8U/B+6xVeJADU44aURgmgy0r2n1DFmfZjLsIjLQ3WjHB6ydrEOAZmbZozpCGS+\n\tCi9fiDRRGj0Wg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1683807190;\n\tx=1686399190; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=X0gTkiTQTa3przmrNunwoLgoKB8ZwL8aqsZoaZ/lWWo=;\n\tb=KGLGZmkq29vtThFf3iMLDhKDpmyLM1Js34/oUF55mBTE+PPHPOHXRVzIINnhxDBvnb\n\tn6DqAIn6WbMQHk18+iDRSkeAUxGv5r3dJsL3bZ815NS5ZqhP3zgayd/TvzwheLRz/GVZ\n\tDKbtp2xiK2/3uKuLvsus66uM5QviTU68pU0vAKP9qCQgPpG160MPrzW2G2ANorTrba+4\n\t4fUcgMR3HDDT7plnCd1BHxzwz96HOS30dtKNlkVrbZwiCjZI5fjuYD2+epym+qQZxI3/\n\tQUmoyH9INlaEg7zHXCipPB3mR2c5kg4zwVFjOQFloruBpdjf6q0EhReZA8KUNk5NMtr/\n\t9fEg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20221208.gappssmtp.com\n\theader.i=@thaumatec-com.20221208.gappssmtp.com header.b=\"KGLGZmkq\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1683807190; x=1686399190;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=X0gTkiTQTa3przmrNunwoLgoKB8ZwL8aqsZoaZ/lWWo=;\n\tb=FFZG0qH5vEFpsL80YvrqSjUasMNn99DqcrAQ7PAhJdaWZQnUd0pDP6eHujhU5kUk0P\n\tG+DGbSW3WV6NzEyWDTe3qs9upmxlIno8B9uT31wPf4O2EoH0vNMcLKss4JdeHMo+0wBz\n\t1Oi+A/OSRI/z/A5ltcCkGD8TBCwjcRiITcaheZK/1WiPbcCUS1+Qh32zq2GUTTwUsXwp\n\tU3OtfBZtUsVDR6k1gWA7KReEuX1xX9HN00sMFmc78uaooRblW8nXCweWWh93uPlEkEuu\n\tEyBlaq8232eis8Srr9CRzS6GRAtgumtlfPJ47W6gP/ODf2CdOjSNGmJ6eu3/6pcOUugu\n\t4clg==","X-Gm-Message-State":"AC+VfDwlN5yTuW8Vq9VRwaTYgovlD2sGclJJD4+fBA2x7rJqKYcyXFpj\n\tPhPEtLHrwDASqutUYLJWxSUo+CGmB3aRqaz+Km1rlA==","X-Google-Smtp-Source":"ACHHUZ4cBAZuMkgp6Mtph5S8FgtIBGuPvKXLTYHutYq2HiNxaNV2Lf+Fj3tMEoGDQZ5V34xFiynVu7UQJGkCFJzNMQw=","X-Received":"by 2002:aa7:df97:0:b0:50b:d1b6:c372 with SMTP id\n\tb23-20020aa7df97000000b0050bd1b6c372mr16647608edy.26.1683807189598;\n\tThu, 11 May 2023 05:13:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-2-dse@thaumatec.com>\n\t<20230425132910.GA1667@pendragon.ideasonboard.com>","In-Reply-To":"<20230425132910.GA1667@pendragon.ideasonboard.com>","Date":"Thu, 11 May 2023 14:12:58 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","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":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27081,"web_url":"https://patchwork.libcamera.org/comment/27081/","msgid":"","date":"2023-05-11T12:14:07","subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Michael,\n\nOn Wed, Apr 26, 2023 at 10:18 AM Michael Riesch\n wrote:\n>\n> Hi Daniel,\n>\n> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote:\n> > Extend the IPA init() function by additional lensControls input\n> > parameter. Check in pipeline handler if camera lens exists, and expose\n> > its controls to the IPA using the new parameter.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 3 ++-\n> > src/ipa/rkisp1/rkisp1.cpp | 7 +++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> > 3 files changed, 17 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index 1009e970..d4ff1230 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -17,7 +17,8 @@ interface IPARkISP1Interface {\n> > init(libcamera.IPASettings settings,\n> > uint32 hwRevision,\n> > libcamera.IPACameraSensorInfo sensorInfo,\n> > - libcamera.ControlInfoMap sensorControls)\n> > + libcamera.ControlInfoMap sensorControls,\n> > + libcamera.ControlInfoMap lensControls)\n>\n> One could imagine that in future other subdevices are supported. For\n> example, I expect that soon we'll have a \"flashControls\" parameter.\n> Would it make sense to gather all those controls in a single parameter\n> \"hwControls\" (name to be bikeshedded)? (In #libcamera the notion of\n> internal controls has been introduced recently. Maybe simply\n> \"internalControls\"?)\n>\n> BTW I also could imagine that a \"lensInfo\" parameter is necessary for\n> complex lens controllers (e.g., does the lens have a zoom function, how\n> many zoom lenses does it have, model of the lens controller, ...). But\n> for simple VCM controllers this may not be necessary, so feel free to\n> ignore this at this point.\n\nIt would probably make sense, but I would like not to do everything at once,\nbut introduce the changes gradually. I would suggest to extended it with the\nintroduction of next optional subdevice.\n\n>\n> > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > start() => (int32 ret);\n> > stop();\n>\n> Shouldn't processStatsBuffer have an additional \"lensControls\"\n> parameter? Of course, the comment above would apply here as well and we\n> could get away with simply adjusting the name \"sensorControls\" ->\n> \"internalControls\".\n\nRight now, We do not report any current values of lens controls to the IPA,\nso such parameter would be left unused.\n\nBest regards\nDaniel\n\n>\n> Best regards,\n> Michael\n>\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 6544c925..d338d374 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -52,6 +53,7 @@ public:\n> > int init(const IPASettings &settings, unsigned int hwRevision,\n> > const IPACameraSensorInfo &sensorInfo,\n> > const ControlInfoMap &sensorControls,\n> > + const ControlInfoMap &lensControls,\n> > ControlInfoMap *ipaControls) override;\n> > int start() override;\n> > void stop() override;\n> > @@ -80,6 +82,7 @@ private:\n> > std::map mappedBuffers_;\n> >\n> > ControlInfoMap sensorControls_;\n> > + std::optional lensControls_;\n> >\n> > /* revision-specific data */\n> > rkisp1_cif_isp_version hwRevision_;\n> > @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const\n> > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > const IPACameraSensorInfo &sensorInfo,\n> > const ControlInfoMap &sensorControls,\n> > + const ControlInfoMap &lensControls,\n> > ControlInfoMap *ipaControls)\n> > {\n> > /* \\todo Add support for other revisions */\n> > @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> > * 1.0s / sensorInfo.pixelRate;\n> >\n> > + if (!lensControls.empty())\n> > + lensControls_ = lensControls;\n> > +\n> > /* Load the tuning data file. */\n> > File file(settings.configurationFile);\n> > if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 8a30fe06..f966254a 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -32,6 +32,7 @@\n> > #include \n> >\n> > #include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/camera_lens.h\"\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/delayed_controls.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > return ret;\n> > }\n> >\n> > + ControlInfoMap lensControls;\n> > + CameraLens *lens = sensor_->focusLens();\n> > + if (lens)\n> > + lensControls = lens->controls();\n> > +\n> > ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > - sensorInfo, sensor_->controls(), &controlInfo_);\n> > + sensorInfo, sensor_->controls(), lensControls,\n> > + &controlInfo_);\n> > if (ret < 0) {\n> > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > return ret;","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 C7B66BD16B\n\tfor ;\n\tThu, 11 May 2023 12:14:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8211D633B4;\n\tThu, 11 May 2023 14:14:19 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5D7160548\n\tfor ;\n\tThu, 11 May 2023 14:14:18 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id\n\t4fb4d7f45d1cf-50bdd7b229cso15506557a12.0\n\tfor ;\n\tThu, 11 May 2023 05:14:18 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683807259;\n\tbh=otqisdEubWrgiFf9ykfnJYPIBnBrFq7fDdtp6lyxEbw=;\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=FJiEasimJx2hHAls0sU08LQmazYCIabTPiolJ6JfWEnOxZjiNuSRZgor0ckS+e5Th\n\tsnB6Cs2otg9v4OTxQlKQm/WtMntebiochFPaDMP1xK623+y9sjE90PgnQnAdqynOuf\n\t7v27sJ3qQKGoNZE7UV5/YPBRHiPf7M0scgqMSKojMgE4lu7t2Z+qfl+G7wcPc7YMo9\n\tQ7AR13PaC3vrc8wbwjOk6HS9NDVWSroFIbJXV+/yA7e23es0BF+nrUl4KrWkarpdCv\n\tZt4ZEiKIj1y6fufTCnFjMTGsTtd/J/kgy3gosKpvLw6wrtMt4s+RtG0l7G+n6btivv\n\tuaBNI7+0IPjPA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1683807258;\n\tx=1686399258; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=iy3F/GT/onflCJ1WSgRJgAEAEH8aAoUGo3lVwEPd5Lw=;\n\tb=NcqywBTSigR6wyevEqmwiwcsRdQdjVyPIJPZxAKyjvnrcB+UYwWju1NPzP6pQcfvxE\n\tGviJdq8jy16ahSFdVk2PpFUJNrsJ9Ba1izrHY2B68PdH99TUqDWN0c5TLDcqo+TsBK2I\n\tuA6kB9g5lDI+yZDcQsszq1PkVbKcO8kzs0j4HWvoB3UwBsNoDaIu3eNcFxUmjzxGdTKY\n\tdZYkURtnF36qZ8etMeTcui1Kf282b69cxWJGRtGTN/l3Dc+2eClGy6K1S/Iymu8FhMiA\n\tn53Rm9h4p5d5Sc8kRVQrFseCb7ixZbz/crkxLRE/oUzrz+uly0iLZGqMPcnCqCjGHdG/\n\tulpg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20221208.gappssmtp.com\n\theader.i=@thaumatec-com.20221208.gappssmtp.com header.b=\"NcqywBTS\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1683807258; x=1686399258;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=iy3F/GT/onflCJ1WSgRJgAEAEH8aAoUGo3lVwEPd5Lw=;\n\tb=EW6vYbxwBcx48GS+W4yPWk8oVc0JQaCTvRS9pO2fsQgNUmIQanJ/26DbzgT0j+BzDo\n\tfVh5GzQ3TrRZgPDPG0f2TRCEBFQaSPi4b4cHWOUOjHbepMJ9WSEENCmOE8L04kPHAHp+\n\t/cnGhaU61JvhyMH8OkMKVjdBWYUZ1kx3Bm4IeJ1VxceossLFyUuNiVUVQUmBd9P//TtP\n\tAR1cSyrVD+5BnjGKyiLM6S53HGqugyt3U7hNJSJCQYfGGIVqFyRJPu93bcNXjj6cn9ZW\n\tZaisZT3NSqK8AUG7Gpr3c4cmrssm6pDzh31Oydsv/zECiLvfed8MhKHT5dFwQTPM0J92\n\tcTPQ==","X-Gm-Message-State":"AC+VfDzAbMCHalyAau5YpmRbRAiGt/e2pk1K/7cdL/2u1lANgWbr89VJ\n\txL+T/SIYw4KB0mp/BS0OCZf8Fzjaw7yrjIwUi1pHJSMg+uY5OzHiUOAE9w==","X-Google-Smtp-Source":"ACHHUZ5HDf99Rcw2h9E5aDJWBRomaBF1T4EYqCEvtTYEDGrS6RS2g3OnipxlrpxsNFDNxZwLiBB2iy0zgvBAqtdMZlM=","X-Received":"by 2002:a05:6402:104f:b0:50b:d863:30ea with SMTP id\n\te15-20020a056402104f00b0050bd86330eamr16514017edu.0.1683807258317;\n\tThu, 11 May 2023 05:14:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-2-dse@thaumatec.com>\n\t<6d47db97-7076-6bbb-4d25-e43718e50890@wolfvision.net>","In-Reply-To":"<6d47db97-7076-6bbb-4d25-e43718e50890@wolfvision.net>","Date":"Thu, 11 May 2023 14:14:07 +0200","Message-ID":"","To":"Michael Riesch ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","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":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27083,"web_url":"https://patchwork.libcamera.org/comment/27083/","msgid":"<1a76605c-d2ec-03ef-9948-14ee6fe0135e@wolfvision.net>","date":"2023-05-12T10:08:41","subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","submitter":{"id":135,"url":"https://patchwork.libcamera.org/api/people/135/","name":"Michael Riesch","email":"michael.riesch@wolfvision.net"},"content":"Hi Daniel,\n\nOn 5/11/23 14:14, Daniel Semkowicz wrote:\n> Hi Michael,\n> \n> On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch\n> wrote:\n>>\n>> Hi Daniel,\n>>\n>> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote:\n>>> Extend the IPA init() function by additional lensControls input\n>>> parameter. Check in pipeline handler if camera lens exists, and expose\n>>> its controls to the IPA using the new parameter.\n>>>\n>>> Signed-off-by: Daniel Semkowicz \n>>> ---\n>>> include/libcamera/ipa/rkisp1.mojom | 3 ++-\n>>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n>>> 3 files changed, 17 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n>>> index 1009e970..d4ff1230 100644\n>>> --- a/include/libcamera/ipa/rkisp1.mojom\n>>> +++ b/include/libcamera/ipa/rkisp1.mojom\n>>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {\n>>> init(libcamera.IPASettings settings,\n>>> uint32 hwRevision,\n>>> libcamera.IPACameraSensorInfo sensorInfo,\n>>> - libcamera.ControlInfoMap sensorControls)\n>>> + libcamera.ControlInfoMap sensorControls,\n>>> + libcamera.ControlInfoMap lensControls)\n>>\n>> One could imagine that in future other subdevices are supported. For\n>> example, I expect that soon we'll have a \"flashControls\" parameter.\n>> Would it make sense to gather all those controls in a single parameter\n>> \"hwControls\" (name to be bikeshedded)? (In #libcamera the notion of\n>> internal controls has been introduced recently. Maybe simply\n>> \"internalControls\"?)\n>>\n>> BTW I also could imagine that a \"lensInfo\" parameter is necessary for\n>> complex lens controllers (e.g., does the lens have a zoom function, how\n>> many zoom lenses does it have, model of the lens controller, ...). But\n>> for simple VCM controllers this may not be necessary, so feel free to\n>> ignore this at this point.\n> \n> It would probably make sense, but I would like not to do everything at once,\n> but introduce the changes gradually. I would suggest to extended it with the\n> introduction of next optional subdevice.\n\nGoing one step at a time makes sense, of course. As I already said, the\n\"lensInfo\" parameter can be introduced once it is actually required and\nused.\n\nOn the other hand, it probably makes sense to discuss the\n\"sensorControls\", \"lensControls\", ... -> \"internalControls\" issue right\nnow. Of course I can wait until your series is applied and then propose\na change, but this would be a bit back-and-forth. If we agree that there\nshould be a single parameter (instead of one parameter per subdev), then\nthis change should be incorporated in this series already IMHO.\n\nOf course, there may be perfectly fine reasons to have one parameter per\nsubdev. I can't be the judge of that, but it would be great to see a\nverdict on that case.\n\n>>> => (int32 ret, libcamera.ControlInfoMap ipaControls);\n>>> start() => (int32 ret);\n>>> stop();\n>>\n>> Shouldn't processStatsBuffer have an additional \"lensControls\"\n>> parameter? Of course, the comment above would apply here as well and we\n>> could get away with simply adjusting the name \"sensorControls\" ->\n>> \"internalControls\".\n> \n> Right now, We do not report any current values of lens controls to the IPA,\n> so such parameter would be left unused.\n\nRight, this should be proposed/introduced when it is actually required/used.\n\nBest regards,\nMichael\n\n> \n> Best regards\n> Daniel\n> \n>>\n>> Best regards,\n>> Michael\n>>\n>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>> index 6544c925..d338d374 100644\n>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>> @@ -7,6 +7,7 @@\n>>>\n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -52,6 +53,7 @@ public:\n>>> int init(const IPASettings &settings, unsigned int hwRevision,\n>>> const IPACameraSensorInfo &sensorInfo,\n>>> const ControlInfoMap &sensorControls,\n>>> + const ControlInfoMap &lensControls,\n>>> ControlInfoMap *ipaControls) override;\n>>> int start() override;\n>>> void stop() override;\n>>> @@ -80,6 +82,7 @@ private:\n>>> std::map mappedBuffers_;\n>>>\n>>> ControlInfoMap sensorControls_;\n>>> + std::optional lensControls_;\n>>>\n>>> /* revision-specific data */\n>>> rkisp1_cif_isp_version hwRevision_;\n>>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const\n>>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n>>> const IPACameraSensorInfo &sensorInfo,\n>>> const ControlInfoMap &sensorControls,\n>>> + const ControlInfoMap &lensControls,\n>>> ControlInfoMap *ipaControls)\n>>> {\n>>> /* \\todo Add support for other revisions */\n>>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n>>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n>>> * 1.0s / sensorInfo.pixelRate;\n>>>\n>>> + if (!lensControls.empty())\n>>> + lensControls_ = lensControls;\n>>> +\n>>> /* Load the tuning data file. */\n>>> File file(settings.configurationFile);\n>>> if (!file.open(File::OpenModeFlag::ReadOnly)) {\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index 8a30fe06..f966254a 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -32,6 +32,7 @@\n>>> #include \n>>>\n>>> #include \"libcamera/internal/camera.h\"\n>>> +#include \"libcamera/internal/camera_lens.h\"\n>>> #include \"libcamera/internal/camera_sensor.h\"\n>>> #include \"libcamera/internal/delayed_controls.h\"\n>>> #include \"libcamera/internal/device_enumerator.h\"\n>>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n>>> return ret;\n>>> }\n>>>\n>>> + ControlInfoMap lensControls;\n>>> + CameraLens *lens = sensor_->focusLens();\n>>> + if (lens)\n>>> + lensControls = lens->controls();\n>>> +\n>>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n>>> - sensorInfo, sensor_->controls(), &controlInfo_);\n>>> + sensorInfo, sensor_->controls(), lensControls,\n>>> + &controlInfo_);\n>>> if (ret < 0) {\n>>> LOG(RkISP1, Error) << \"IPA initialization failure\";\n>>> return ret;","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 6E35ABE08B\n\tfor ;\n\tFri, 12 May 2023 10:08:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1884633B4;\n\tFri, 12 May 2023 12:08:47 +0200 (CEST)","from EUR05-VI1-obe.outbound.protection.outlook.com\n\t(mail-vi1eur05on20610.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f400:7d00::610])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99A97627DC\n\tfor ;\n\tFri, 12 May 2023 12:08:45 +0200 (CEST)","from DU0PR08MB9155.eurprd08.prod.outlook.com (2603:10a6:10:416::5)\n\tby DB4PR08MB9285.eurprd08.prod.outlook.com (2603:10a6:10:3f2::21)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6387.19;\n\tFri, 12 May 2023 10:08:43 +0000","from DU0PR08MB9155.eurprd08.prod.outlook.com\n\t([fe80::77dd:14e4:a772:d85f]) by\n\tDU0PR08MB9155.eurprd08.prod.outlook.com\n\t([fe80::77dd:14e4:a772:d85f%6]) with mapi id 15.20.6387.020;\n\tFri, 12 May 2023 10:08:43 +0000"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683886127;\n\tbh=XTNjgZCdND6ukBshIABLmiMSDdQYIIz2pYWGx4KAHtk=;\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=j2P11excM1YhzADcn9Mfl7xC/0yp6GYP5cPD4SG56NGRCkX9Dxr030ChECv3KQVKR\n\tusClz+5jngCUH70pJWr0qrCcykKwWB++bxyQF0/9Wzlmr3ESS7G4Ki2ssxrveEnqDW\n\tejJC6CbrDJII79FB+uenu/OU9JMCmXb8a6q4P9ulU18Ihj7fwPL7k+R+8Boeh2eaHp\n\tiUq+tr/EJdEachoIVE86clme17gr1wdDqh7rOp/fHkZznoY39atAAF6GKm5poWIp6O\n\tHnsNv6oreZqPmckc/29GphR3RWHfrEpNv5Hg+TQmW7EivEOBMYXPPWqCzxY+8q6ctU\n\t1dk0JxEDW0OTQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=wolfvision.net;\n\ts=selector2;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=CUG2gFp+H0yrZMOQ8sV6dGqKcNhGjGp6dHggx5G87x8=;\n\tb=BdRVJpFNtePRmTT6Se5K6TKDYeZSIpQ3PoCmcfTLBO6V5SwSnLYKZBYmMmf8m61tecB1DiLtMr6z8rK2fedKmqIVVi1If4aYtOCb//wwpn7GVaMe2vx5vWHasSUMscBt7/uJH5kYSfYpE/QT8HuU7pKdNfmwMNuNWyGArpFbYhk="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=wolfvision.net\n\theader.i=@wolfvision.net\n\theader.b=\"BdRVJpFN\"; dkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=wolfvision.net;"],"ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=ivY+iWmNXUId5z+HwJRmdrbJKom8QD+oUQRlVCSSpVCkqadtOnCvx4gbdwUnPL1pkeOYCBezJSJ3g4NySSnvzt0eR64143M0WfVsDzUoBQUoTllw9eCx95I0gQFnGSKsY6TW7rOqzIA+Xf/3uzDwgC3itF6090zMDoAIHG6fTEyDEmsQyLz6Y/w8Mq6COhxgndNVMbw2jKkrkwbcOEdMbjicRbpvqxpzrsy9b+DLzcstOg9doccGGjGkUXrqDlzMwqx/KvAU4xHHPEzeq8zQZcmOTTtH0Q0OgRlk8W3jB0nDp9h2im2zov6QuMlepXpIKhU9SIp46DLSUKrNH6kZZw==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=CUG2gFp+H0yrZMOQ8sV6dGqKcNhGjGp6dHggx5G87x8=;\n\tb=eWBpQUuNHz3LNOn+yLCb/PmWMe6zCBkwUzWjmQJVkM3F5oGfpeHyhVekldMDv9A2ROIB0eDRMa8pgm7G2Ks26n/BHO3xh5BSl8KBeUUyE4ULfiHJX80R/9dGcDhFVJ5KtncoBvR5FYvo+nhVkcDWOAU/b8ApwCCfN1GVA2renfoaiWGPipwg7obiwpi0WNnyUthhwe4c4Fp32oZhraqKgoW5CCoCpv8a7GOL+m/AWX1RpakjwW03/cOGYKSgdAyJ6eqk7SZW6z8Demky6MFv9T9/BM6dsCm+5TC40LRo4UF9TufTt6xfeCR2NIrarLFijcgjstWX1bQXu99f41qS6g==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=wolfvision.net; dmarc=pass action=none\n\theader.from=wolfvision.net; dkim=pass header.d=wolfvision.net;\n\tarc=none","Message-ID":"<1a76605c-d2ec-03ef-9948-14ee6fe0135e@wolfvision.net>","Date":"Fri, 12 May 2023 12:08:41 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.10.0","Content-Language":"en-US","To":"Daniel Semkowicz ","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-2-dse@thaumatec.com>\n\t<6d47db97-7076-6bbb-4d25-e43718e50890@wolfvision.net>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","X-ClientProxiedBy":"ZR2P278CA0049.CHEP278.PROD.OUTLOOK.COM\n\t(2603:10a6:910:53::15) To DU0PR08MB9155.eurprd08.prod.outlook.com\n\t(2603:10a6:10:416::5)","MIME-Version":"1.0","X-MS-PublicTrafficType":"Email","X-MS-TrafficTypeDiagnostic":"DU0PR08MB9155:EE_|DB4PR08MB9285:EE_","X-MS-Office365-Filtering-Correlation-Id":"7c710e2f-a70c-4a5f-a7d5-08db52d0ddb8","X-MS-Exchange-SenderADCheck":"1","X-MS-Exchange-AntiSpam-Relay":"0","X-Microsoft-Antispam":"BCL:0;","X-Microsoft-Antispam-Message-Info":"kFY1YrRwqVRxa1mfV+FFJaoYd7B4p/HNBHtkzN3fLguhXeqV5kFT+Cj1QHL6/y5gPFtZTL30LTXCenaRU17XJoiuCXcBOzuetcNFIpi8qmpPt+vXJDiTuwGhcv1mnUus9+jHaYNrko8iMxkKO0DT63fh0JQvP33wFDajSTdBGEteZ/lrQ0cxW6TXG5eoK76sd68vMLAaDgy7PHPCbvxTyR8ilVn+EOHoZdo5pHWNIsomqK9QwUpfcbWJrHz7+QGbYRySkAQDu44lEVpIRmD7jtZBJiBA71jupuK+Liwv84tMm+DczPsM4OxU/TNiybeWJQtwLYvElsmmGL08an5/l7zXX6uDGGEzBeEdPe4raJmJlG5kqd6mI2NerYlYVwSgSFR/RWNLJ7XcqloEFFDiRIUnZfWNuhcWSwzaeHPvdWvfaQ+ndRjsfeuCOFVo7Sk+YO+oZtLUXC6lyilyCKk1NxbwjwL+5yWVra6dV15KdWqlbEK9Gk12xS5L+7SSRYdbMMT6UsvJNUwdOPjfrPIjhgfg+xb4axjdSPEOK9SYNJdQVaSknTC20dJyArBxD+iCA0BkxuLOtTeCnn7kvMeV+dbCtAp28CXI206W1Q+R7DncUeuSktTuFiCuhFpyjbaxiD+0eEKqMcdJKmrz3o/V7w==","X-Forefront-Antispam-Report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:DU0PR08MB9155.eurprd08.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230028)(4636009)(396003)(136003)(39840400004)(376002)(346002)(366004)(451199021)(2616005)(2906002)(83380400001)(86362001)(186003)(4326008)(6916009)(36756003)(66946007)(66476007)(66556008)(6506007)(6512007)(53546011)(31696002)(31686004)(5660300002)(38100700002)(41300700001)(6486002)(44832011)(478600001)(316002)(8936002)(8676002)(45980500001)(43740500002);\n\tDIR:OUT; SFP:1101; ","X-MS-Exchange-AntiSpam-MessageData-ChunkCount":"1","X-MS-Exchange-AntiSpam-MessageData-0":"=?utf-8?q?joi8eYc34RvusOJlnuHhySv56?=\n\t=?utf-8?q?2kFiGofxCPgpBUKR2EIZZK7AKirh+LAKtbZPrh0B2g0Frq7s4brKA8lR?=\n\t=?utf-8?q?ClKvM0u9YwVnab7vcsoKA688Gw/EzNZn3l8L+Jud6ON0azylkNKY0f9Q?=\n\t=?utf-8?q?S+zknAd1PFJDPjN9LmDQsCWmu8Uw/WtLW8kB8U6a4EOE6g2llZd6mjM4?=\n\t=?utf-8?q?UGWPkxWx6MRFoKXI60eUBwjzu5/lSqNvKUIUiAmHs7Nlw8lNgLnapmRd?=\n\t=?utf-8?q?fnOvoGfyrm6BLfxGHZRnpEn297WfFD4ymcTOk+eI7YgqkJU96M/VPeoO?=\n\t=?utf-8?q?byIHOiY7sFq02uH2xJkwkHhpT+Rj/tffD3VhhsciXuVBt8wqD7eNEgTm?=\n\t=?utf-8?q?mpgUItUzz+Nhqe+Dx4vc6wpTKzO/V6lgaoQcsNqn6vru7H68H+iBmb/V?=\n\t=?utf-8?q?qyXFI/1rxXQVtVRxokGqMPsRXYzwRGuyFQM/qo/pHZdlwgvpVtyxY+sU?=\n\t=?utf-8?q?Esgtt4Nvpc1V8/vuoi8LK6q5xU+jTIl7DEDNkfXp2F3Ux3Ywwo2yiN3v?=\n\t=?utf-8?q?BoRPjQbbO1oqFa9vivs80nr1femS6J62mBvszveHcsERreClOKR6tCG1?=\n\t=?utf-8?q?oaX1JWuH2Pb1Ogi0vMA4YK0pzsdFC1t0TSLt2wv48LRQOY6VqQua228G?=\n\t=?utf-8?q?Wk1gIaUX76QSzfi7JrhVsG5aWluB5+KiHgLPFkVPppj1Qo2GVw1AJxLC?=\n\t=?utf-8?q?Y6ybMSw19GBsOM6mfOZW9ckP8wAbDM9jsv1Dni3e7RMb3+e/dOB6Ugn7?=\n\t=?utf-8?q?EXI+LDWencjB5mHui4xzewnVcyubmYnNbNpA7nUaMBBdS81MWxP2MNW8?=\n\t=?utf-8?q?ifv2a7fUKiKyd9Nd8fGXHDZU19k+tU/sAos2pxk8QGaieJWZUAoqAaYz?=\n\t=?utf-8?q?5pgoS5q9aeVOWr3rypJdncrj1RzpDW/Hm4I26t9lXegnSbaTn0QAJX/o?=\n\t=?utf-8?q?c6Tmk6RvktxejC4htJU+wAmDUuVGFvDNlx5KCY22w4ztHO5it/7fb3kI?=\n\t=?utf-8?q?FOPvYaL6x0T25aD4cqFaFSJdxCVl/C2n6ebu0cLPo9qu1WNLZceEtbUB?=\n\t=?utf-8?q?4fW8FWUKd13Y6niclvjPVCd3/dxxfQ4FaJqj3iyX7n4CzpE0JRiO46Pk?=\n\t=?utf-8?q?SC3nlztQUWczwlHB89VgMBRRLxE/pkwerhfVVKVjunSW6LQtWE5J7ZKM?=\n\t=?utf-8?q?0osuC2x3qN6M/vcr87VsA5dVVY24961v3T2KyLDVhLd5w4D9vnN8WSAf?=\n\t=?utf-8?q?QgXu5Ft/V7IQH6R/envHIZR1vCQPlNtrMZaIjJ+PJAGjLh5Efr3vOc90?=\n\t=?utf-8?q?uB1K7r8LKJcgBF97N9AYjYyfGK0UPvF4QNE7BPt0tWEIZGr5wiqN8QRf?=\n\t=?utf-8?q?DZjYR7xoqL3ggBvglETXFXk6E9E6q8ukd72jQ4sLCwfny81AmGSaSAZv?=\n\t=?utf-8?q?7CXTFQxXUCks+89179ImlufrFx3O5ZUeCWC0mU7OJAEdQAlfCMOoIPuQ?=\n\t=?utf-8?q?bE6Nkwz2TywbhrbaWW2n1X5z5irvc2jhpneq0N6lV8uyvvheINy4crnl?=\n\t=?utf-8?q?V9s4jTWe8G8MbiMY64Y/N0qun78V585vnYBMa8wXXuqY/EBejFnb1ny8?=\n\t=?utf-8?q?q/ZRUuG62Q79hKHTEInr+MCOhAuzc9pP9zfEOxdfe5hu52GK89OQAT/r?=\n\t=?utf-8?q?ct6yFaXwq2zKz9vzCgtrKGQdI/k20MhJaG4nSyhfWR1Sl1kZUijHZ1Cx?=\n\t=?utf-8?q?69qFF78HtEeBF/loKuTsugeu3oAu1bnJXo4mQ=3D=3D?=","X-OriginatorOrg":"wolfvision.net","X-MS-Exchange-CrossTenant-Network-Message-Id":"7c710e2f-a70c-4a5f-a7d5-08db52d0ddb8","X-MS-Exchange-CrossTenant-AuthSource":"DU0PR08MB9155.eurprd08.prod.outlook.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-OriginalArrivalTime":"12 May 2023 10:08:43.0801\n\t(UTC)","X-MS-Exchange-CrossTenant-FromEntityHeader":"Hosted","X-MS-Exchange-CrossTenant-Id":"e94ec9da-9183-471e-83b3-51baa8eb804f","X-MS-Exchange-CrossTenant-MailboxType":"HOSTED","X-MS-Exchange-CrossTenant-UserPrincipalName":"vYBwPUVhFMjKLpqfbp88E1BcFRdMt2ZvqGO/Hs8FflD8dbcatW8N90R10Ng+7Rj/01HNyonc2xbabsO59IZGqgAifuv7PxPREULqlGrL2Qk=","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"DB4PR08MB9285","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","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":"Michael Riesch via libcamera-devel\n\t","Reply-To":"Michael Riesch ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27155,"web_url":"https://patchwork.libcamera.org/comment/27155/","msgid":"","date":"2023-05-29T10:09:41","subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hello Michael,\n\nOn Fri, May 12, 2023 at 12:08 PM Michael Riesch\n wrote:\n>\n> Hi Daniel,\n>\n> On 5/11/23 14:14, Daniel Semkowicz wrote:\n> > Hi Michael,\n> >\n> > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch\n> > wrote:\n> >>\n> >> Hi Daniel,\n> >>\n> >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote:\n> >>> Extend the IPA init() function by additional lensControls input\n> >>> parameter. Check in pipeline handler if camera lens exists, and expose\n> >>> its controls to the IPA using the new parameter.\n> >>>\n> >>> Signed-off-by: Daniel Semkowicz \n> >>> ---\n> >>> include/libcamera/ipa/rkisp1.mojom | 3 ++-\n> >>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++\n> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> >>> 3 files changed, 17 insertions(+), 2 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> >>> index 1009e970..d4ff1230 100644\n> >>> --- a/include/libcamera/ipa/rkisp1.mojom\n> >>> +++ b/include/libcamera/ipa/rkisp1.mojom\n> >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {\n> >>> init(libcamera.IPASettings settings,\n> >>> uint32 hwRevision,\n> >>> libcamera.IPACameraSensorInfo sensorInfo,\n> >>> - libcamera.ControlInfoMap sensorControls)\n> >>> + libcamera.ControlInfoMap sensorControls,\n> >>> + libcamera.ControlInfoMap lensControls)\n> >>\n> >> One could imagine that in future other subdevices are supported. For\n> >> example, I expect that soon we'll have a \"flashControls\" parameter.\n> >> Would it make sense to gather all those controls in a single parameter\n> >> \"hwControls\" (name to be bikeshedded)? (In #libcamera the notion of\n> >> internal controls has been introduced recently. Maybe simply\n> >> \"internalControls\"?)\n> >>\n> >> BTW I also could imagine that a \"lensInfo\" parameter is necessary for\n> >> complex lens controllers (e.g., does the lens have a zoom function, how\n> >> many zoom lenses does it have, model of the lens controller, ...). But\n> >> for simple VCM controllers this may not be necessary, so feel free to\n> >> ignore this at this point.\n> >\n> > It would probably make sense, but I would like not to do everything at once,\n> > but introduce the changes gradually. I would suggest to extended it with the\n> > introduction of next optional subdevice.\n>\n> Going one step at a time makes sense, of course. As I already said, the\n> \"lensInfo\" parameter can be introduced once it is actually required and\n> used.\n>\n> On the other hand, it probably makes sense to discuss the\n> \"sensorControls\", \"lensControls\", ... -> \"internalControls\" issue right\n> now. Of course I can wait until your series is applied and then propose\n> a change, but this would be a bit back-and-forth. If we agree that there\n> should be a single parameter (instead of one parameter per subdev), then\n> this change should be incorporated in this series already IMHO.\n>\n> Of course, there may be perfectly fine reasons to have one parameter per\n> subdev. I can't be the judge of that, but it would be great to see a\n> verdict on that case.\n\nI agree We need to find a good way to handle multiple subdevices,\nbut there are already a lot of changes in this series. That is why I am\nreluctant to add even more. I would suggest starting a separate thread\nto find a solution to this problem. What others think about it?\n\nIs it safe to gather all controls in a single parameter?\nCan we guarantee the same control will not show up in two different\nsubdevices? Is it possible there will be two subdevices of the same\ntype, e.g. 2x flashlight?\n\n>\n> >>> => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> >>> start() => (int32 ret);\n> >>> stop();\n> >>\n> >> Shouldn't processStatsBuffer have an additional \"lensControls\"\n> >> parameter? Of course, the comment above would apply here as well and we\n> >> could get away with simply adjusting the name \"sensorControls\" ->\n> >> \"internalControls\".\n> >\n> > Right now, We do not report any current values of lens controls to the IPA,\n> > so such parameter would be left unused.\n>\n> Right, this should be proposed/introduced when it is actually required/used.\n>\n> Best regards,\n> Michael\n>\n> >\n> > Best regards\n> > Daniel\n> >\n> >>\n> >> Best regards,\n> >> Michael\n> >>\n> >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >>> index 6544c925..d338d374 100644\n> >>> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >>> @@ -7,6 +7,7 @@\n> >>>\n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -52,6 +53,7 @@ public:\n> >>> int init(const IPASettings &settings, unsigned int hwRevision,\n> >>> const IPACameraSensorInfo &sensorInfo,\n> >>> const ControlInfoMap &sensorControls,\n> >>> + const ControlInfoMap &lensControls,\n> >>> ControlInfoMap *ipaControls) override;\n> >>> int start() override;\n> >>> void stop() override;\n> >>> @@ -80,6 +82,7 @@ private:\n> >>> std::map mappedBuffers_;\n> >>>\n> >>> ControlInfoMap sensorControls_;\n> >>> + std::optional lensControls_;\n> >>>\n> >>> /* revision-specific data */\n> >>> rkisp1_cif_isp_version hwRevision_;\n> >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const\n> >>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> >>> const IPACameraSensorInfo &sensorInfo,\n> >>> const ControlInfoMap &sensorControls,\n> >>> + const ControlInfoMap &lensControls,\n> >>> ControlInfoMap *ipaControls)\n> >>> {\n> >>> /* \\todo Add support for other revisions */\n> >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> >>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> >>> * 1.0s / sensorInfo.pixelRate;\n> >>>\n> >>> + if (!lensControls.empty())\n> >>> + lensControls_ = lensControls;\n> >>> +\n> >>> /* Load the tuning data file. */\n> >>> File file(settings.configurationFile);\n> >>> if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> index 8a30fe06..f966254a 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> @@ -32,6 +32,7 @@\n> >>> #include \n> >>>\n> >>> #include \"libcamera/internal/camera.h\"\n> >>> +#include \"libcamera/internal/camera_lens.h\"\n> >>> #include \"libcamera/internal/camera_sensor.h\"\n> >>> #include \"libcamera/internal/delayed_controls.h\"\n> >>> #include \"libcamera/internal/device_enumerator.h\"\n> >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> >>> return ret;\n> >>> }\n> >>>\n> >>> + ControlInfoMap lensControls;\n> >>> + CameraLens *lens = sensor_->focusLens();\n> >>> + if (lens)\n> >>> + lensControls = lens->controls();\n> >>> +\n> >>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> >>> - sensorInfo, sensor_->controls(), &controlInfo_);\n> >>> + sensorInfo, sensor_->controls(), lensControls,\n> >>> + &controlInfo_);\n> >>> if (ret < 0) {\n> >>> LOG(RkISP1, Error) << \"IPA initialization failure\";\n> >>> return ret;\n\nBest regards\nDaniel","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 DC4ACC31E9\n\tfor ;\n\tMon, 29 May 2023 10:09:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9477E626F8;\n\tMon, 29 May 2023 12:09:54 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFCC1626F5\n\tfor ;\n\tMon, 29 May 2023 12:09:52 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id\n\t4fb4d7f45d1cf-51478f6106cso4962309a12.1\n\tfor ;\n\tMon, 29 May 2023 03:09:52 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685354994;\n\tbh=GAbKf6BSuVjGcVxo4YnT0gFlTvtzQud2YP0LFV0Z5eQ=;\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=NtenkhaHhPt1RxptvOjCJu9Es4ZYm5JssSeb+gLePgVKTACVVsgVnJHgaRrse3xep\n\tqyq9e77Gnq/athzPUlBubI8h26KRhAS3HzhXn+Me8EUdCZ1D88r1roGOgg5byX8Nxs\n\tJJ1NYwdNV7s1lpwUShNsAmKumAKKpfIs5sWid0MXar+Au4KF6GUk5P/3d3PQZKbAAO\n\typPBrKXuTiPWPZnhbYefLmSf+6syedAZ3WyuBViZG2bOWFOA843ZzWvBz0uGH74Msa\n\tJRP4YyrULaoHXWe8F8oJrIyOGr727XyYnawdVIS7la+9EGP7ftPCH+17vu96rneHeB\n\tMTj7uJXx35ghA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1685354992;\n\tx=1687946992; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=KymuRDkokFuGW2AC/sC8rFbQaz6Y7AxwQJAuWkwDD7k=;\n\tb=JTKmtwzBmGaBU2v4L4LXplaqZ/fUChf5fqi9rRxZCS3HeFOI0FFFiMSjaq2NGSYkTL\n\t0WltuIJfZFG9u7wWWpRAYrqx3FSVW6ePKccoOFumEXMJ0xfjiTm3y28i9pg8UKiBkdsf\n\tWlmGnzTb9iPIszBBGZvKLocUsk++ybkR3YmXkEf+P7fhu73AxtPYa8pZVcoiwwMWmnKL\n\tjGYooGaeqxI8PI6ssENNOmQIWiorM6B7mS47Y/WLqD5+uNSWt+9KHLOPmrTBROb8ucuz\n\tfo1M4oug8EQfoxP9EY0WRUjbcoWqaah5MdqaHWIUxUYt09uAQagz7hXBn8y/MqLeVDsr\n\tlzmw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20221208.gappssmtp.com\n\theader.i=@thaumatec-com.20221208.gappssmtp.com header.b=\"JTKmtwzB\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685354992; x=1687946992;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=KymuRDkokFuGW2AC/sC8rFbQaz6Y7AxwQJAuWkwDD7k=;\n\tb=B+E/nfL2bbGeeLlc911LXenrnc04b2tz1tsQ5Wn3+jhAr5asNbJMi39fwyL1KOYmnv\n\t9bjOoo+AXfodKgJMD+2+UV668jl3PPaCoE6H1pegfEVXj6iWr0nMUIrzp3xyoyXKsU+A\n\t25MaZRgFAUlwOun3eMbNjo3ScjcfhOTzp58QsqOsDLi6Ip5HJ1wgr1oD/vrsFKvok57x\n\th5ySsxDvXVm2eUor8eQA7XyECvLDsJyF1P91Am0L+O1lvh4XoMqSIA/IVisns69OL/u1\n\tvI4TSQV1GpocJ75Vmtidg0T20IfFQ+h1NKUQg8iSrnGeW5/Obq8vlWsd5OkNvpQrXCdu\n\t8xjQ==","X-Gm-Message-State":"AC+VfDzTi5Nhiop5znAWlv+5Iy9YXpLF0cC/P0mR8F4SsH0YPjWFNgms\n\ttvlrdW3WPLX6Vce0mi3Aic9ZJAmrtD7UAZDHPag1bg==","X-Google-Smtp-Source":"ACHHUZ7ZmJSMi049FWyFhiyWXEz/rGsTf8+Sq1VDnR5GghuWjQE8QFaP9opd1rAy1kXXnvkxKZW5e0rdv7LmwE1CpCg=","X-Received":"by 2002:aa7:d494:0:b0:514:9e61:18ab with SMTP id\n\tb20-20020aa7d494000000b005149e6118abmr1917380edr.14.1685354992444;\n\tMon, 29 May 2023 03:09:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-2-dse@thaumatec.com>\n\t<6d47db97-7076-6bbb-4d25-e43718e50890@wolfvision.net>\n\t\n\t<1a76605c-d2ec-03ef-9948-14ee6fe0135e@wolfvision.net>","In-Reply-To":"<1a76605c-d2ec-03ef-9948-14ee6fe0135e@wolfvision.net>","Date":"Mon, 29 May 2023 12:09:41 +0200","Message-ID":"","To":"Michael Riesch ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","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":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27202,"web_url":"https://patchwork.libcamera.org/comment/27202/","msgid":"<20230531183945.GJ24749@pendragon.ideasonboard.com>","date":"2023-05-31T18:39:45","subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, May 29, 2023 at 12:09:41PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> On Fri, May 12, 2023 at 12:08 PM Michael Riesch wrote:\n> > On 5/11/23 14:14, Daniel Semkowicz wrote:\n> > > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch wrote:\n> > >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote:\n> > >>> Extend the IPA init() function by additional lensControls input\n> > >>> parameter. Check in pipeline handler if camera lens exists, and expose\n> > >>> its controls to the IPA using the new parameter.\n> > >>>\n> > >>> Signed-off-by: Daniel Semkowicz \n> > >>> ---\n> > >>> include/libcamera/ipa/rkisp1.mojom | 3 ++-\n> > >>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++\n> > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> > >>> 3 files changed, 17 insertions(+), 2 deletions(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > >>> index 1009e970..d4ff1230 100644\n> > >>> --- a/include/libcamera/ipa/rkisp1.mojom\n> > >>> +++ b/include/libcamera/ipa/rkisp1.mojom\n> > >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {\n> > >>> init(libcamera.IPASettings settings,\n> > >>> uint32 hwRevision,\n> > >>> libcamera.IPACameraSensorInfo sensorInfo,\n> > >>> - libcamera.ControlInfoMap sensorControls)\n> > >>> + libcamera.ControlInfoMap sensorControls,\n> > >>> + libcamera.ControlInfoMap lensControls)\n> > >>\n> > >> One could imagine that in future other subdevices are supported. For\n> > >> example, I expect that soon we'll have a \"flashControls\" parameter.\n> > >> Would it make sense to gather all those controls in a single parameter\n> > >> \"hwControls\" (name to be bikeshedded)? (In #libcamera the notion of\n> > >> internal controls has been introduced recently. Maybe simply\n> > >> \"internalControls\"?)\n> > >>\n> > >> BTW I also could imagine that a \"lensInfo\" parameter is necessary for\n> > >> complex lens controllers (e.g., does the lens have a zoom function, how\n> > >> many zoom lenses does it have, model of the lens controller, ...). But\n> > >> for simple VCM controllers this may not be necessary, so feel free to\n> > >> ignore this at this point.\n> > >\n> > > It would probably make sense, but I would like not to do everything at once,\n> > > but introduce the changes gradually. I would suggest to extended it with the\n> > > introduction of next optional subdevice.\n> >\n> > Going one step at a time makes sense, of course. As I already said, the\n> > \"lensInfo\" parameter can be introduced once it is actually required and\n> > used.\n> >\n> > On the other hand, it probably makes sense to discuss the\n> > \"sensorControls\", \"lensControls\", ... -> \"internalControls\" issue right\n> > now. Of course I can wait until your series is applied and then propose\n> > a change, but this would be a bit back-and-forth. If we agree that there\n> > should be a single parameter (instead of one parameter per subdev), then\n> > this change should be incorporated in this series already IMHO.\n> >\n> > Of course, there may be perfectly fine reasons to have one parameter per\n> > subdev. I can't be the judge of that, but it would be great to see a\n> > verdict on that case.\n> \n> I agree We need to find a good way to handle multiple subdevices,\n> but there are already a lot of changes in this series. That is why I am\n> reluctant to add even more. I would suggest starting a separate thread\n> to find a solution to this problem. What others think about it?\n> \n> Is it safe to gather all controls in a single parameter?\n> Can we guarantee the same control will not show up in two different\n> subdevices? Is it possible there will be two subdevices of the same\n> type, e.g. 2x flashlight?\n\nIt's more about how we build abstraction layers inside libcamera than\nabout the subdevices. We have CameraSensor and CameraLens classes, and\nthose should expose an abstract interface towards the pipeline handler,\nregardless of how the kernel exposes the corresponding devices. Those\ntwo classes should move from using V4L2 controls to internal libcamera\ncontrols, and avoid exposing anything V4L2-specific. The interface\nbetween the IPA module and pipeline handler should use the same internal\ncontrols. The translation to V4L2 controls, and the mapping to\nparticular subdevs, will be internal to the helper classes.\n\nI don't really envision a need for multiple flashes per camera. I also\ndon't think we'll have to deal with multiple independent focus lenses,\nbut we will have optical zoom in addition to focus.\n\n> > >>> => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > >>> start() => (int32 ret);\n> > >>> stop();\n> > >>\n> > >> Shouldn't processStatsBuffer have an additional \"lensControls\"\n> > >> parameter? Of course, the comment above would apply here as well and we\n> > >> could get away with simply adjusting the name \"sensorControls\" ->\n> > >> \"internalControls\".\n> > >\n> > > Right now, We do not report any current values of lens controls to the IPA,\n> > > so such parameter would be left unused.\n> >\n> > Right, this should be proposed/introduced when it is actually required/used.\n> >\n> > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > >>> index 6544c925..d338d374 100644\n> > >>> --- a/src/ipa/rkisp1/rkisp1.cpp\n> > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > >>> @@ -7,6 +7,7 @@\n> > >>>\n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> @@ -52,6 +53,7 @@ public:\n> > >>> int init(const IPASettings &settings, unsigned int hwRevision,\n> > >>> const IPACameraSensorInfo &sensorInfo,\n> > >>> const ControlInfoMap &sensorControls,\n> > >>> + const ControlInfoMap &lensControls,\n> > >>> ControlInfoMap *ipaControls) override;\n> > >>> int start() override;\n> > >>> void stop() override;\n> > >>> @@ -80,6 +82,7 @@ private:\n> > >>> std::map mappedBuffers_;\n> > >>>\n> > >>> ControlInfoMap sensorControls_;\n> > >>> + std::optional lensControls_;\n> > >>>\n> > >>> /* revision-specific data */\n> > >>> rkisp1_cif_isp_version hwRevision_;\n> > >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const\n> > >>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > >>> const IPACameraSensorInfo &sensorInfo,\n> > >>> const ControlInfoMap &sensorControls,\n> > >>> + const ControlInfoMap &lensControls,\n> > >>> ControlInfoMap *ipaControls)\n> > >>> {\n> > >>> /* \\todo Add support for other revisions */\n> > >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > >>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> > >>> * 1.0s / sensorInfo.pixelRate;\n> > >>>\n> > >>> + if (!lensControls.empty())\n> > >>> + lensControls_ = lensControls;\n> > >>> +\n> > >>> /* Load the tuning data file. */\n> > >>> File file(settings.configurationFile);\n> > >>> if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> index 8a30fe06..f966254a 100644\n> > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> @@ -32,6 +32,7 @@\n> > >>> #include \n> > >>>\n> > >>> #include \"libcamera/internal/camera.h\"\n> > >>> +#include \"libcamera/internal/camera_lens.h\"\n> > >>> #include \"libcamera/internal/camera_sensor.h\"\n> > >>> #include \"libcamera/internal/delayed_controls.h\"\n> > >>> #include \"libcamera/internal/device_enumerator.h\"\n> > >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > >>> return ret;\n> > >>> }\n> > >>>\n> > >>> + ControlInfoMap lensControls;\n> > >>> + CameraLens *lens = sensor_->focusLens();\n> > >>> + if (lens)\n> > >>> + lensControls = lens->controls();\n> > >>> +\n> > >>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > >>> - sensorInfo, sensor_->controls(), &controlInfo_);\n> > >>> + sensorInfo, sensor_->controls(), lensControls,\n> > >>> + &controlInfo_);\n> > >>> if (ret < 0) {\n> > >>> LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > >>> return ret;","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 18CF7C31E9\n\tfor ;\n\tWed, 31 May 2023 18:39:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 832BF62722;\n\tWed, 31 May 2023 20:39:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDF59626F8\n\tfor ;\n\tWed, 31 May 2023 20:39:46 +0200 (CEST)","from pendragon.ideasonboard.com (om126205251136.34.openmobile.ne.jp\n\t[126.205.251.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B777904;\n\tWed, 31 May 2023 20:39:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685558388;\n\tbh=qD7nqgpJT1ratuksJv2b/IXOGqSruEPiecU05+eNr/g=;\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=BHF7HB6+s3R6CP4U1wot9apDNc2zO5YZpk6h0qZxnJOmJTlL+Vzpcun5/f1sUnIu5\n\tLDroMKIhVDGyDdoqpVAV/zB3oZSX8SndFJRl4deEbi2h4tg3NO23hG7wKn7hkl4DNM\n\tqZwjs+GIf1lUSAgas+GRaL/j3sqmTW76ykevv/aAq5TGHw3lw7tpg0v2zALN0Xv9wz\n\tZiDynWV/SdEVSKoivUHXwryPisw9G1OFcvxgih5EcxLVPmhbk42ys+A6yAxO7j8C+Q\n\tQ8DOdeU638FdJnw0GpuWG0j5D0K/En6GmhepTcCfIAguIwsXlHrC3zhcCt56iLqseZ\n\tS+jtCtegfwucA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685558365;\n\tbh=qD7nqgpJT1ratuksJv2b/IXOGqSruEPiecU05+eNr/g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MVL7dMBCCwgV2jCxRsHeQA+HUymsFVViPN/vk3FWMj+Nwq7qrVclnNLNhJM6AtC9J\n\t0GPnr1Taabzyqur62zpl8b3TKhn1dVEybxuLRE2G7g6Uyey+ZwpOVaaqbUeCO8Jrhh\n\tUi4UhGCQf63RfDr7QX9KT0Co2iqXZVd5wa4Ke1QQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MVL7dMBC\"; dkim-atps=neutral","Date":"Wed, 31 May 2023 21:39:45 +0300","To":"Daniel Semkowicz ","Message-ID":"<20230531183945.GJ24749@pendragon.ideasonboard.com>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-2-dse@thaumatec.com>\n\t<6d47db97-7076-6bbb-4d25-e43718e50890@wolfvision.net>\n\t\n\t<1a76605c-d2ec-03ef-9948-14ee6fe0135e@wolfvision.net>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27207,"web_url":"https://patchwork.libcamera.org/comment/27207/","msgid":"","date":"2023-06-01T07:33:01","subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hello,\n\n\nOn Wed, May 31, 2023 at 8:39 PM Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> On Mon, May 29, 2023 at 12:09:41PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > On Fri, May 12, 2023 at 12:08 PM Michael Riesch wrote:\n> > > On 5/11/23 14:14, Daniel Semkowicz wrote:\n> > > > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch wrote:\n> > > >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote:\n> > > >>> Extend the IPA init() function by additional lensControls input\n> > > >>> parameter. Check in pipeline handler if camera lens exists, and expose\n> > > >>> its controls to the IPA using the new parameter.\n> > > >>>\n> > > >>> Signed-off-by: Daniel Semkowicz \n> > > >>> ---\n> > > >>> include/libcamera/ipa/rkisp1.mojom | 3 ++-\n> > > >>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++\n> > > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-\n> > > >>> 3 files changed, 17 insertions(+), 2 deletions(-)\n> > > >>>\n> > > >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > >>> index 1009e970..d4ff1230 100644\n> > > >>> --- a/include/libcamera/ipa/rkisp1.mojom\n> > > >>> +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {\n> > > >>> init(libcamera.IPASettings settings,\n> > > >>> uint32 hwRevision,\n> > > >>> libcamera.IPACameraSensorInfo sensorInfo,\n> > > >>> - libcamera.ControlInfoMap sensorControls)\n> > > >>> + libcamera.ControlInfoMap sensorControls,\n> > > >>> + libcamera.ControlInfoMap lensControls)\n> > > >>\n> > > >> One could imagine that in future other subdevices are supported. For\n> > > >> example, I expect that soon we'll have a \"flashControls\" parameter.\n> > > >> Would it make sense to gather all those controls in a single parameter\n> > > >> \"hwControls\" (name to be bikeshedded)? (In #libcamera the notion of\n> > > >> internal controls has been introduced recently. Maybe simply\n> > > >> \"internalControls\"?)\n> > > >>\n> > > >> BTW I also could imagine that a \"lensInfo\" parameter is necessary for\n> > > >> complex lens controllers (e.g., does the lens have a zoom function, how\n> > > >> many zoom lenses does it have, model of the lens controller, ...). But\n> > > >> for simple VCM controllers this may not be necessary, so feel free to\n> > > >> ignore this at this point.\n> > > >\n> > > > It would probably make sense, but I would like not to do everything at once,\n> > > > but introduce the changes gradually. I would suggest to extended it with the\n> > > > introduction of next optional subdevice.\n> > >\n> > > Going one step at a time makes sense, of course. As I already said, the\n> > > \"lensInfo\" parameter can be introduced once it is actually required and\n> > > used.\n> > >\n> > > On the other hand, it probably makes sense to discuss the\n> > > \"sensorControls\", \"lensControls\", ... -> \"internalControls\" issue right\n> > > now. Of course I can wait until your series is applied and then propose\n> > > a change, but this would be a bit back-and-forth. If we agree that there\n> > > should be a single parameter (instead of one parameter per subdev), then\n> > > this change should be incorporated in this series already IMHO.\n> > >\n> > > Of course, there may be perfectly fine reasons to have one parameter per\n> > > subdev. I can't be the judge of that, but it would be great to see a\n> > > verdict on that case.\n> >\n> > I agree We need to find a good way to handle multiple subdevices,\n> > but there are already a lot of changes in this series. That is why I am\n> > reluctant to add even more. I would suggest starting a separate thread\n> > to find a solution to this problem. What others think about it?\n> >\n> > Is it safe to gather all controls in a single parameter?\n> > Can we guarantee the same control will not show up in two different\n> > subdevices? Is it possible there will be two subdevices of the same\n> > type, e.g. 2x flashlight?\n>\n> It's more about how we build abstraction layers inside libcamera than\n> about the subdevices. We have CameraSensor and CameraLens classes, and\n> those should expose an abstract interface towards the pipeline handler,\n> regardless of how the kernel exposes the corresponding devices. Those\n> two classes should move from using V4L2 controls to internal libcamera\n> controls, and avoid exposing anything V4L2-specific. The interface\n> between the IPA module and pipeline handler should use the same internal\n> controls. The translation to V4L2 controls, and the mapping to\n> particular subdevs, will be internal to the helper classes.\n>\n> I don't really envision a need for multiple flashes per camera. I also\n> don't think we'll have to deal with multiple independent focus lenses,\n> but we will have optical zoom in addition to focus.\n\nThis clarifies a lot to me. Right now, CameraLens just exposes\nthe V4L2 controls and I assumed this is the target approach, hence my\ndoubts.\n\nNow, I need the decision how to extend the init() parameters, because it\nimpacts how I will handle it in the IPA. I am thinking about few options,\nmost of them already proposed in this thread:\n\n1. Leave the current implementation and solve this problem later:\n\n int init(..., const ControlInfoMap &sensorControls, const\nControlInfoMap &lensControls, ...)\n\n2. Merge all controls into single parameter:\n\n int init(..., const ControlInfoMap &internalControls, ...)\n\n3. Leave the sensorControls intact, add auxiliaryDevicesControls\nparameter and put there all controls other than sensor:\n\n int init(..., const ControlInfoMap &sensorControls, const\nControlInfoMap &auxiliaryDevicesControls, ...)\n\n4. Create a separate map for each auxiliary device. This will avoid\nproblems if there are multiple devices of the same type:\n\n int init(..., const ControlInfoMap &sensorControls, const\nSpan &auxiliaryDevicesControls, ...)\n\nWhat do you think?\n\n>\n> > > >>> => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > >>> start() => (int32 ret);\n> > > >>> stop();\n> > > >>\n> > > >> Shouldn't processStatsBuffer have an additional \"lensControls\"\n> > > >> parameter? Of course, the comment above would apply here as well and we\n> > > >> could get away with simply adjusting the name \"sensorControls\" ->\n> > > >> \"internalControls\".\n> > > >\n> > > > Right now, We do not report any current values of lens controls to the IPA,\n> > > > so such parameter would be left unused.\n> > >\n> > > Right, this should be proposed/introduced when it is actually required/used.\n> > >\n> > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > >>> index 6544c925..d338d374 100644\n> > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > >>> @@ -7,6 +7,7 @@\n> > > >>>\n> > > >>> #include \n> > > >>> #include \n> > > >>> +#include \n> > > >>> #include \n> > > >>> #include \n> > > >>> #include \n> > > >>> @@ -52,6 +53,7 @@ public:\n> > > >>> int init(const IPASettings &settings, unsigned int hwRevision,\n> > > >>> const IPACameraSensorInfo &sensorInfo,\n> > > >>> const ControlInfoMap &sensorControls,\n> > > >>> + const ControlInfoMap &lensControls,\n> > > >>> ControlInfoMap *ipaControls) override;\n> > > >>> int start() override;\n> > > >>> void stop() override;\n> > > >>> @@ -80,6 +82,7 @@ private:\n> > > >>> std::map mappedBuffers_;\n> > > >>>\n> > > >>> ControlInfoMap sensorControls_;\n> > > >>> + std::optional lensControls_;\n> > > >>>\n> > > >>> /* revision-specific data */\n> > > >>> rkisp1_cif_isp_version hwRevision_;\n> > > >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const\n> > > >>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > > >>> const IPACameraSensorInfo &sensorInfo,\n> > > >>> const ControlInfoMap &sensorControls,\n> > > >>> + const ControlInfoMap &lensControls,\n> > > >>> ControlInfoMap *ipaControls)\n> > > >>> {\n> > > >>> /* \\todo Add support for other revisions */\n> > > >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > > >>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> > > >>> * 1.0s / sensorInfo.pixelRate;\n> > > >>>\n> > > >>> + if (!lensControls.empty())\n> > > >>> + lensControls_ = lensControls;\n> > > >>> +\n> > > >>> /* Load the tuning data file. */\n> > > >>> File file(settings.configurationFile);\n> > > >>> if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >>> index 8a30fe06..f966254a 100644\n> > > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >>> @@ -32,6 +32,7 @@\n> > > >>> #include \n> > > >>>\n> > > >>> #include \"libcamera/internal/camera.h\"\n> > > >>> +#include \"libcamera/internal/camera_lens.h\"\n> > > >>> #include \"libcamera/internal/camera_sensor.h\"\n> > > >>> #include \"libcamera/internal/delayed_controls.h\"\n> > > >>> #include \"libcamera/internal/device_enumerator.h\"\n> > > >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > >>> return ret;\n> > > >>> }\n> > > >>>\n> > > >>> + ControlInfoMap lensControls;\n> > > >>> + CameraLens *lens = sensor_->focusLens();\n> > > >>> + if (lens)\n> > > >>> + lensControls = lens->controls();\n> > > >>> +\n> > > >>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > >>> - sensorInfo, sensor_->controls(), &controlInfo_);\n> > > >>> + sensorInfo, sensor_->controls(), lensControls,\n> > > >>> + &controlInfo_);\n> > > >>> if (ret < 0) {\n> > > >>> LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > >>> return ret;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nBest regards\nDaniel","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 B39B9C3200\n\tfor ;\n\tThu, 1 Jun 2023 07:33:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15B4A626FE;\n\tThu, 1 Jun 2023 09:33:17 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27D99626FA\n\tfor ;\n\tThu, 1 Jun 2023 09:33:15 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id\n\t2adb3069b0e04-4f004cc54f4so484609e87.3\n\tfor ;\n\tThu, 01 Jun 2023 00:33:15 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685604797;\n\tbh=CaxQOHexGm0UZbGmAHXt7sq7f7/pejeXgnl0CWiu10M=;\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=4aJh8dQCcPYcWSivtP+Yn4A7mefRvuSEjP67rCdHI7RwXUNB4s1tjb6camBmZw5/1\n\tR9lEUCZcz06emn+S3cFWdsYZPTYW1Oj7Z6cT7wJVLAcxDL8ksZNHggwp+1rZUsrDm3\n\tjol0sH3WulEdN+BbPdwPNwQYoHysFrx3XaIHG0KFMPlEnP5WmsAsf9GqfxpO5AFhgd\n\t/E4Ryk/wDYJj7h/LvR6uHDSovpndFafkqUi6EUIOuDoDbHaCnlW7bDq3GzV5RGJN1m\n\t8WC7ml6U7S7zUmBXNQmjRQ8SlJwIxvStIQP8ppJM22rcMWxyAJwkRoQBaW+bVFjEZz\n\t6G+07wwpvuXkQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1685604794;\n\tx=1688196794; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=SkfYrTL7oINLon175Wa+R2MiueA8VL9OBVJjOQQ8NCY=;\n\tb=YPO2M2s6q6fKfENaD74Muoeg2lOJz0ldgP/kAzbV9/eT8alHtZMR2vF9b027cqOeTR\n\tvL/QgmfdeDFVghwwYAajgm+/A/OERBoTVVCGAJKySlsJIoq0+wbw/QKBqkVsKH213Qut\n\tglLoRnrfiZdZvjJ+ivesuN3trtUOCNkP9xXZDIp6kAVjKfb3fbl6Wz/Q8wlJk+pAY07Z\n\t2FJQiCvwZj0YD+F4FFjFfsF9Wg3z0SIpcZfTAf4BzOPOHDRXfMbXjn/qCltv3IOCFygs\n\toS/+zhXLoDMWv/72WS8SMI0EXWky7KgZYaK3keckX+J6+qYwZTkoI+WxjW6du4m0iE7+\n\t0qxg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20221208.gappssmtp.com\n\theader.i=@thaumatec-com.20221208.gappssmtp.com header.b=\"YPO2M2s6\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685604794; x=1688196794;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=SkfYrTL7oINLon175Wa+R2MiueA8VL9OBVJjOQQ8NCY=;\n\tb=gDV1efWbtrr3AUnXvvLIoSZ+iRs2wOSngifN9fkGWWz2lHEi1zWiqIIAiAaGn0w6jl\n\t9nAxhd4FYwBJqP8oOKX2TLTjylMm63BXJuvPLzSPTGEhIIJo1Kkw/3EjkLGZGa0M4sAg\n\tupsmqWV3NWdhmPdlcP+DvNbwYsZlcOHxVcIBeWLrqngWkyplmxI72SD4hvR29gAIDaSb\n\tZxrCeWSMFuKUJxNirqJr6GYaMlARGZ8aS8nZFv3//dn5E7DDKRGY6hCaXNXg5MbIMuTE\n\tFEJel/VQOd5QArxFZBhcVvYwJBFufhpwl2Ny8j3zxIVnapMgeJjC6NxXJnu0vm2kfs+l\n\tGPAQ==","X-Gm-Message-State":"AC+VfDysAvjw8mvQyXiJ5H+MRRixO3WP3iAu3t8m5pMbQ6YVTNEs91le\n\tYqJqEqy5urIqxpVqq4XQtJVM98GxItA8wwctGKN7Kw==","X-Google-Smtp-Source":"ACHHUZ5ehMIDRistqSf4RSMyolFlGLkwFHVZ+3dA0gFWG1W2iZ9UQUiWVf2hNBWJ0kS/DHej8BdxeG/3O/p6Wiap8OA=","X-Received":"by 2002:a19:7616:0:b0:4eb:c85:bdc2 with SMTP id\n\tc22-20020a197616000000b004eb0c85bdc2mr770870lff.2.1685604793839;\n\tThu, 01 Jun 2023 00:33:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-2-dse@thaumatec.com>\n\t<6d47db97-7076-6bbb-4d25-e43718e50890@wolfvision.net>\n\t\n\t<1a76605c-d2ec-03ef-9948-14ee6fe0135e@wolfvision.net>\n\t\n\t<20230531183945.GJ24749@pendragon.ideasonboard.com>","In-Reply-To":"<20230531183945.GJ24749@pendragon.ideasonboard.com>","Date":"Thu, 1 Jun 2023 09:33:01 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to\n\tPH and expose it to the IPA","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":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]