[{"id":26701,"web_url":"https://patchwork.libcamera.org/comment/26701/","msgid":"<20230321145317.apfqfwnclo2q4vh7@uno.localdomain>","date":"2023-03-21T14:53:17","subject":"Re: [libcamera-devel] [PATCH v4 09/10] rkisp1: Control camera lens\n\tposition from 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 Tue, Mar 14, 2023 at 03:48:33PM +0100, Daniel Semkowicz wrote:\n> Allow control of lens position from the IPA, by setting corresponding\n> af fields in the IPAFrameContext structure. Controls are then passed to\n> the pipeline handler, which sets the lens position in CameraLens.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 8 ++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++++++++++++\n> 3 files changed, 26 insertions(+)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index bf6e9141..c3ed87aa 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n> interface IPARkISP1EventInterface {\n> \tparamsBufferReady(uint32 frame);\n> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> +\tsetLensControls(libcamera.ControlList lensControls);\n> \tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 4b30844f..4aef2e30 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -469,6 +469,14 @@ void IPARkISP1::setControls(unsigned int frame)\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n>\n> \tsetSensorControls.emit(frame, ctrls);\n> +\n> +\tif (lensControls_ && context_.activeState.af.applyLensCtrls) {\n> +\t\tcontext_.activeState.af.applyLensCtrls = false;\n> +\t\tControlList lensCtrls(*lensControls_);\n> +\t\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> +\t\t\t context_.activeState.af.lensPosition);\n> +\t\tsetLensControls.emit(lensCtrls);\n> +\t}\n> }\n>\n> } /* namespace ipa::rkisp1 */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 83fb6287..1dc3f957 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -114,6 +114,7 @@ private:\n> \tvoid paramFilled(unsigned int frame);\n> \tvoid setSensorControls(unsigned int frame,\n> \t\t\t const ControlList &sensorControls);\n> +\tvoid setLensControls(const ControlList &lensControls);\n>\n> \tvoid metadataReady(unsigned int frame, const ControlList &metadata);\n> };\n> @@ -340,6 +341,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t\treturn -ENOENT;\n>\n> \tipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);\n> +\tipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);\n> \tipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);\n> \tipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);\n>\n> @@ -403,6 +405,21 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> \tdelayedCtrls_->push(sensorControls);\n> }\n>\n> +void RkISP1CameraData::setLensControls(const ControlList &lensControls)\n> +{\n> +\tCameraLens *focusLens = sensor_->focusLens();\n> +\tif (!focusLens)\n> +\t\treturn;\n> +\n> +\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> +\t\treturn;\n\nAs you correctly pointed out in the reply to my review of 2/10,\nAm I wrong thinking that if any of the two above condition is false\nwe don't get a valid CameraLens * from the CameraSensor, hence we\nnever initialize ipaConfig.lensControls, effecively disabling lens\nsupport in the IPA ?\n\n\tCameraLens *lens = data->sensor_->focusLens();\n\tif (lens)\n\t\tipaConfig.lensControls = lens->controls();\n\n> +\n> +\tconst ControlValue &focusValue =\n> +\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> +\n> +\tfocusLens->setFocusPosition(focusValue.get());\n> +}\n> +\n> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> {\n> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\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 63C8BC0F1B\n\tfor ;\n\tTue, 21 Mar 2023 14:53:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2068626E5;\n\tTue, 21 Mar 2023 15:53:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FDA1626DA\n\tfor ;\n\tTue, 21 Mar 2023 15:53:22 +0100 (CET)","from ideasonboard.com (host-87-18-61-243.retail.telecomitalia.it\n\t[87.18.61.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55FD510B;\n\tTue, 21 Mar 2023 15:53:21 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679410403;\n\tbh=yeodG2eYXy62kzceNUqjjrwIFBNKFv+eO7Acpd/mAoU=;\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=K/G8PH9p305jRk/ujZ9f8pvgcJwaKo1mgfQsa+f4Ftkxgs+fNzpLeTyzn8wn+p1oa\n\tEmaSi2M3zoCY+6856CPzPUC08OumEAV/+ihxFOmP2Fq59y2VZCVbk45ht6OmUE6Co7\n\tIMbm2Qmd2u+GjY1JbxoijnAWE3s7gGrs8/71wSpfefq9pC87Zjz4dVYkEkGItclH1a\n\t8Z0KXPiynRQHiuDWRCDNPHWfQkMQQAtUVJh6yfjlT06DpBPj8v49zZgfeb8TlJ3Zcd\n\tEtz5u33/ymB8n0DaZecQs7oU/vtRI3Tpu4euDkwHchDk1CP6Kdl9jdl+/6AvXMC4mB\n\tIzt8XGGrJwolA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679410401;\n\tbh=yeodG2eYXy62kzceNUqjjrwIFBNKFv+eO7Acpd/mAoU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=se7Y+AWa3mPJNnuJTxAVOf49bu284vyB8c+3OO08a3d5/zXxWnNPLyQ6oRAOC4rD6\n\tajhM9k4pTO6evuiEZb7/3y/XQxtd8S7QiliKlp655HmyEwtgK+Ib6zZHakhZSDH1qQ\n\tSC9hyAKz+C4F9LeWslKdbhGVwvq+RStJRI09klyA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"se7Y+AWa\"; dkim-atps=neutral","Date":"Tue, 21 Mar 2023 15:53:17 +0100","To":"Daniel Semkowicz ","Message-ID":"<20230321145317.apfqfwnclo2q4vh7@uno.localdomain>","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-10-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230314144834.85193-10-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v4 09/10] rkisp1: Control camera lens\n\tposition from 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":"jacopo.mondi@ideasonboard.com, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26706,"web_url":"https://patchwork.libcamera.org/comment/26706/","msgid":"","date":"2023-03-22T11:11:59","subject":"Re: [libcamera-devel] [PATCH v4 09/10] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"On Tue, Mar 21, 2023 at 3:53 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n>\n> On Tue, Mar 14, 2023 at 03:48:33PM +0100, Daniel Semkowicz wrote:\n> > Allow control of lens position from the IPA, by setting corresponding\n> > af fields in the IPAFrameContext structure. Controls are then passed to\n> > the pipeline handler, which sets the lens position in CameraLens.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 1 +\n> > src/ipa/rkisp1/rkisp1.cpp | 8 ++++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++++++++++++\n> > 3 files changed, 26 insertions(+)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index bf6e9141..c3ed87aa 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n> > interface IPARkISP1EventInterface {\n> > paramsBufferReady(uint32 frame);\n> > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > + setLensControls(libcamera.ControlList lensControls);\n> > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > };\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 4b30844f..4aef2e30 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -469,6 +469,14 @@ void IPARkISP1::setControls(unsigned int frame)\n> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n> >\n> > setSensorControls.emit(frame, ctrls);\n> > +\n> > + if (lensControls_ && context_.activeState.af.applyLensCtrls) {\n> > + context_.activeState.af.applyLensCtrls = false;\n> > + ControlList lensCtrls(*lensControls_);\n> > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > + context_.activeState.af.lensPosition);\n> > + setLensControls.emit(lensCtrls);\n> > + }\n> > }\n> >\n> > } /* namespace ipa::rkisp1 */\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 83fb6287..1dc3f957 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -114,6 +114,7 @@ private:\n> > void paramFilled(unsigned int frame);\n> > void setSensorControls(unsigned int frame,\n> > const ControlList &sensorControls);\n> > + void setLensControls(const ControlList &lensControls);\n> >\n> > void metadataReady(unsigned int frame, const ControlList &metadata);\n> > };\n> > @@ -340,6 +341,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > return -ENOENT;\n> >\n> > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);\n> > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);\n> > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);\n> > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);\n> >\n> > @@ -403,6 +405,21 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > delayedCtrls_->push(sensorControls);\n> > }\n> >\n> > +void RkISP1CameraData::setLensControls(const ControlList &lensControls)\n> > +{\n> > + CameraLens *focusLens = sensor_->focusLens();\n> > + if (!focusLens)\n> > + return;\n> > +\n> > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> > + return;\n>\n> As you correctly pointed out in the reply to my review of 2/10,\n> Am I wrong thinking that if any of the two above condition is false\n> we don't get a valid CameraLens * from the CameraSensor, hence we\n> never initialize ipaConfig.lensControls, effecively disabling lens\n> support in the IPA ?\n\nYes, you are right. I think I should even connect the setLensControls()\nslot only if the lens were successfully initialized. Then, We can\nsafely assume, this function will not be called from the IPA even by\naccident. In this case I can remove the:\n\n if (!focusLens)\n return;\n\ncode block.\n\nThe following block:\n\n if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n return;\n\nwill need to stay, as there is no guarantee that V4L2_CID_FOCUS_ABSOLUTE\nwas included in the lensControls by the IPA calling\nthe setLensControls().\n\n>\n> CameraLens *lens = data->sensor_->focusLens();\n> if (lens)\n> ipaConfig.lensControls = lens->controls();\n>\n> > +\n> > + const ControlValue &focusValue =\n> > + lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > +\n> > + focusLens->setFocusPosition(focusValue.get());\n> > +}\n> > +\n> > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > {\n> > RkISP1FrameInfo *info = frameInfo_.find(frame);\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 7901FC0F1B\n\tfor ;\n\tWed, 22 Mar 2023 11:12:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C823D626E5;\n\tWed, 22 Mar 2023 12:12:12 +0100 (CET)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0093561ECE\n\tfor ;\n\tWed, 22 Mar 2023 12:12:10 +0100 (CET)","by mail-ed1-x535.google.com with SMTP id y4so71349640edo.2\n\tfor ;\n\tWed, 22 Mar 2023 04:12:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679483532;\n\tbh=obzCC/uvcsgOiSlTIZxa4d47q7BN/O4jK+2RMzYZ3w8=;\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=2RNZZNH/GaKq7v44wY8rb1aRAv64XS5gwMKbZj1fwdN6lMxH3wh162fhmOPry2Iej\n\twO+/UDttYsFfdx8r4VvAsIedQSfui4VOOfLtVBNqPiWSdIzKfqmHtJs4WBqlHiQuVY\n\tFDBmGoDZ+I877cxLvSjx5K1gvAI0rF8wuqEIJqh6YGHVCmW+HDD3NgQmANaiYExXac\n\t1wglEB26g7+MffA/8ev85gD+5J4m3mGnIszkFhHNUBgvn5qZ+JXjqDyH7AZsJ51ge7\n\tbf1pciPjGyoHy2gqwLC2G0jT4K9H3jiO0ccYjoPMRHT0G2bnJv86V5Rj20FUVpkpoW\n\tGWct+vw4VwTEw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1679483530;\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=p++lv1VCLLjWrJ9tZwBKs1JnDwQmS4YFtEkwWpbfhKE=;\n\tb=ml+/f2q3B4PeEEHVzz2Z5jZy8SxzTE3etkGo/6arZYElhEWKXGy6ONQDEdxlWMUD1M\n\t+aUu54i2ZwhnDsTpz8812fQHzKTSVFwCBukcFtlYZBI4P54EGgUtvFcXshHFXZ7fhe8+\n\tfUGKrQvMKRy6MiXY/oE/Bw4VYRKLhDJaG6maLJaT+woA8L/8iUIXKNfSWhCeSOV2Kkml\n\tZYqrlODpLTzVs8wAnRirRJprmJcxxbYSjog9oCJHkYE6BSP+SF1LiHSWZbM1uKhSBTJs\n\tm2nLlNgq8pS2hAznwLn1eRGISSyuHcwJ0I2WkoCZxXNQoWNaT9cwJvDyAUPQwdPH8G66\n\tzqzg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20210112.gappssmtp.com\n\theader.i=@thaumatec-com.20210112.gappssmtp.com header.b=\"ml+/f2q3\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679483530;\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=p++lv1VCLLjWrJ9tZwBKs1JnDwQmS4YFtEkwWpbfhKE=;\n\tb=sFRiDvlXn1OP8CGw6bhhnCCVUhSw7Rfv79zlvLDXkAKSLpQ4oy1l7t2fa04EjFXTS4\n\tX7axh7lStFLIFs3jbl4TqrdPNGVzUPWSDGSXa/WYW5Xe2CFgLGQSVySMiChR6PDSrCRQ\n\tHlLNv402Friu/5aQ4gg39HEfIUWC0tTOra50lR6bA6pXIt5PuLExyRfY41yePJrrz6Dh\n\t754FmWawejILPUlnkg7J7RAqAiLdvhvFxrXlvFgKQSFuO+azdXrteG7MJ4LWdScQiqeh\n\tos0S1BIkK231/SI8GVTpRhPXwu4w7KXjEQXLGzbwLJRj7eopKTh64Z7eDs4yWEvXEn6X\n\t/YiA==","X-Gm-Message-State":"AO0yUKXZOpQL1ribdTEJV4i0OQrBeuCH8axrZ+YMp/6McQcNLgsaSkOb\n\twTiCVw1a/8hG5WQSx5UWiu9HIO4OX3kgYUj6Y+GF4scQYHDq39oQmxE=","X-Google-Smtp-Source":"AK7set97cKbol8ww+JqbED2cSUXqHYUa1ByjAWO1nXp0erEsxDdOPcuORHZ1L8xM9Vazx9Fbx/NsGOnzwVFPsBa1dQg=","X-Received":"by 2002:a17:907:2c66:b0:931:faf0:3db1 with SMTP id\n\tib6-20020a1709072c6600b00931faf03db1mr1038646ejc.4.1679483530454;\n\tWed, 22 Mar 2023 04:12:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-10-dse@thaumatec.com>\n\t<20230321145317.apfqfwnclo2q4vh7@uno.localdomain>","In-Reply-To":"<20230321145317.apfqfwnclo2q4vh7@uno.localdomain>","Date":"Wed, 22 Mar 2023 12:11:59 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v4 09/10] rkisp1: Control camera lens\n\tposition from 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\" "}}]