[{"id":24481,"web_url":"https://patchwork.libcamera.org/comment/24481/","msgid":"<20220809152513.i5f53xui5mi6lojh@uno.localdomain>","date":"2022-08-09T15:25:13","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel\n\nOn Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> 4 files changed, 34 insertions(+)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index eaf3955e..caa1121a 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> interface IPARkISP1EventInterface {\n> \tparamsBufferReady(uint32 frame);\n> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> +\tsetLensControls(libcamera.ControlList sensorControls);\n> \tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 2bdb6a81..d6d46b57 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> };\n>\n> struct IPAFrameContext {\n> +\tstruct {\n> +\t\tuint32_t lensPosition;\n> +\t\tbool applyLensCtrls;\n> +\t} af;\n> +\n\nSo, I feel like this patch is better delayed after we have completed\nthe rework of the IPU3 and RkISP1 IPA modules and move the algorithms\nto retain their state internally instead of using the IPAFrameContext.\n\nThere's a series almost landed that renames IPAFrameContext to\nIPAActiveState and there will be a few more to align the two IPA\nmodules.\n\nWould you mind taking this in the series that introduces the AF\nalgorithm while I try to collect 1 and 2 asap ?\n\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 9e4c48a2..39e1ab2f 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> \tcontext_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> \tcontext_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n>\n> +\t/* Lens position is unknown at the startup, so initilize the variable\n> +\t * that holds the current position to something out of the range. */\n> +\tcontext_.frameContext.af.lensPosition = std::numeric_limits::max();\n> +\n> \tcontext_.frameContext.frameCount = 0;\n>\n> \tfor (auto const &algo : algorithms()) {\n> @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> +\t\tcontext_.frameContext.af.applyLensCtrls = false;\n> +\t\tControlList lensCtrls(*lensCtrls_);\n> +\t\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> +\t\t\t static_cast(context_.frameContext.af.lensPosition));\n> +\t\tsetLensControls.emit(lensCtrls);\n> +\t}\n> }\n>\n> void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 5f10c26b..de0d37da 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -108,6 +108,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> @@ -324,6 +325,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> @@ -378,6 +380,20 @@ 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> +\n> +\tconst ControlValue &focusValue = lensControls.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.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1DDB5BE173\n\tfor ;\n\tTue, 9 Aug 2022 15:25:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97F6A63326;\n\tTue, 9 Aug 2022 17:25:18 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A0F061FAA\n\tfor ;\n\tTue, 9 Aug 2022 17:25:16 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id C07622000E;\n\tTue, 9 Aug 2022 15:25:15 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660058718;\n\tbh=Q/mZ39aSvdgV/3CN3dphVxFVE+BoELSTF24vholCqO4=;\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=PLp2RRFF0V8Q8ZNwqAivuf3YDp84zHxGorf2PYLLf5+JF1KK+uKGqZNpvDj0dou2f\n\tnnKEzDRLWOWWKXIVmvggHK6n96/mD+L6FA4CFloYnpjSSBsrHW4eL4llVkVUncXXah\n\tUStFiC5/TuomVMYOwiI16Gwq4ldkYh0pKuG87ppV1w3eJRHQAfBt4FsbYB/UbLMDOA\n\tyg5C7W+Lkyto7D+9/jxsAqqzJzn58LJ6XxdKmRrH8e4aquKvIcSHzIrP1NYVDKigpg\n\tactAtn3WrTj55QWXFBtz9aNCwAPLOn00mvrz3X0FREjzCaOyZdXwpJB8sp+XQfOJhp\n\tiU7PLkmlnjPgg==","Date":"Tue, 9 Aug 2022 17:25:13 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220809152513.i5f53xui5mi6lojh@uno.localdomain>","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220809144704.61682-4-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24485,"web_url":"https://patchwork.libcamera.org/comment/24485/","msgid":"<166006025325.15821.17726379324497038019@Monstersaurus>","date":"2022-08-09T15:50:53","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> Hi Daniel\n> \n> On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > 4 files changed, 34 insertions(+)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index eaf3955e..caa1121a 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > interface IPARkISP1EventInterface {\n> > paramsBufferReady(uint32 frame);\n> > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > + setLensControls(libcamera.ControlList sensorControls);\n> > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > };\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 2bdb6a81..d6d46b57 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > };\n> >\n> > struct IPAFrameContext {\n> > + struct {\n> > + uint32_t lensPosition;\n> > + bool applyLensCtrls;\n> > + } af;\n> > +\n> \n> So, I feel like this patch is better delayed after we have completed\n> the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> to retain their state internally instead of using the IPAFrameContext.\n> \n> There's a series almost landed that renames IPAFrameContext to\n> IPAActiveState and there will be a few more to align the two IPA\n> modules.\n> Would you mind taking this in the series that introduces the AF\n> algorithm while I try to collect 1 and 2 asap ?\n\nIt does seem like a lot of this will change indeed. So we really should\ntry to clean up the interfaces to get it a bit more stable.\n\n\n> \n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 9e4c48a2..39e1ab2f 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> >\n> > + /* Lens position is unknown at the startup, so initilize the variable\n> > + * that holds the current position to something out of the range. */\n> > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > +\n> > context_.frameContext.frameCount = 0;\n> >\n> > for (auto const &algo : algorithms()) {\n> > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > + context_.frameContext.af.applyLensCtrls = false;\n> > + ControlList lensCtrls(*lensCtrls_);\n> > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > + static_cast(context_.frameContext.af.lensPosition));\n> > + setLensControls.emit(lensCtrls);\n> > + }\n> > }\n> >\n> > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 5f10c26b..de0d37da 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -108,6 +108,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> > @@ -324,6 +325,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> > @@ -378,6 +380,20 @@ 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\nI think the construction of sensor_->focusLens should make sure that the\ncorrect control is set, so that if we have a valid CameraLens\n*focusLens, we know it's valid to call focusLens->setFocusPosition().\n\nSo the above lensControls.contains() should be redundant.\n\n\n> > +\n> > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > +\n> > + focusLens->setFocusPosition(focusValue.get());\n\nBut I think Jacopo also has plans to change this so that a libcamera\ncontrol is passed in anyway, meaning the conversion from libcamera focus\nposition to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\nCameraLens class anyway.\n\n\n> > +}\n> > +\n> > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > {\n> > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DE81EBE173\n\tfor ;\n\tTue, 9 Aug 2022 15:50:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40B2263326;\n\tTue, 9 Aug 2022 17:50:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B77F61FAA\n\tfor ;\n\tTue, 9 Aug 2022 17:50:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C5553481;\n\tTue, 9 Aug 2022 17:50:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660060258;\n\tbh=5y3Kbz9qIiwP2T+2GE+85s0107QDrbWGJBtyv5QQoIM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HNZ9p9U4eetw0xOJvECjepTs7eTeOxP+i2bYRrgYMsYSKd83JeiBgMNifyXpGK3DZ\n\tzl7P1L2IpGDSTrRDdHHy3pL5fv70LOi4KLiyf/iSwnFd2yJBJaKBjiGrK/9uW2Y7Vg\n\tfA5+BOldod3qjxUTVxqvOGLUF6Nn0P2wq6AB3PyxE5xlOcxPpMDD92jkiaoYngVsXy\n\tQhvXKvNX0WzZsBaTkXjKT2KCDznka3Orn1TcvZzG3grgVsdWEI/yTfLDaQS8Jr6q9K\n\tlm/ZiJRRWESbGy6m7K5ix5D0d3WgKbvIH83lza3ytxTli49eP/yG/1GX/ftq9CEdLt\n\t4vcKDzauhdDhQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660060255;\n\tbh=5y3Kbz9qIiwP2T+2GE+85s0107QDrbWGJBtyv5QQoIM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=IBn7XQ+3LB3zBZ9U4yBvUEp0Q/QZJf51kEYD7MhyaGVJs4+UwHuknf87m0pbKZYhs\n\tnOrgzYGsMkKzRrR/C5ZNOstZPOhVe+fsyEnKy/qG5lcgpfOv+xFSLAltoBfv624bol\n\t1oonKbhelhcWoGWLoUovkG/6wRGDBUFuGDF/DCAw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"IBn7XQ+3\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220809152513.i5f53xui5mi6lojh@uno.localdomain>","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>","To":"Daniel Semkowicz , Jacopo Mondi , \n\tJacopo Mondi via libcamera-devel ","Date":"Tue, 09 Aug 2022 16:50:53 +0100","Message-ID":"<166006025325.15821.17726379324497038019@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24486,"web_url":"https://patchwork.libcamera.org/comment/24486/","msgid":"<20220809160137.rdj2ukhltiegv2zf@uno.localdomain>","date":"2022-08-09T16:01:37","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > Hi Daniel\n> >\n> > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > > 4 files changed, 34 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > index eaf3955e..caa1121a 100644\n> > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > > interface IPARkISP1EventInterface {\n> > > paramsBufferReady(uint32 frame);\n> > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > + setLensControls(libcamera.ControlList sensorControls);\n> > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > };\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 2bdb6a81..d6d46b57 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > > };\n> > >\n> > > struct IPAFrameContext {\n> > > + struct {\n> > > + uint32_t lensPosition;\n> > > + bool applyLensCtrls;\n> > > + } af;\n> > > +\n> >\n> > So, I feel like this patch is better delayed after we have completed\n> > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> > to retain their state internally instead of using the IPAFrameContext.\n> >\n> > There's a series almost landed that renames IPAFrameContext to\n> > IPAActiveState and there will be a few more to align the two IPA\n> > modules.\n> > Would you mind taking this in the series that introduces the AF\n> > algorithm while I try to collect 1 and 2 asap ?\n>\n> It does seem like a lot of this will change indeed. So we really should\n> try to clean up the interfaces to get it a bit more stable.\n>\n>\n> >\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 9e4c48a2..39e1ab2f 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > >\n> > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > + * that holds the current position to something out of the range. */\n> > > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > > +\n> > > context_.frameContext.frameCount = 0;\n> > >\n> > > for (auto const &algo : algorithms()) {\n> > > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > + context_.frameContext.af.applyLensCtrls = false;\n> > > + ControlList lensCtrls(*lensCtrls_);\n> > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > + static_cast(context_.frameContext.af.lensPosition));\n> > > + setLensControls.emit(lensCtrls);\n> > > + }\n> > > }\n> > >\n> > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 5f10c26b..de0d37da 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -108,6 +108,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> > > @@ -324,6 +325,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> > > @@ -378,6 +380,20 @@ 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> I think the construction of sensor_->focusLens should make sure that the\n> correct control is set, so that if we have a valid CameraLens\n> *focusLens, we know it's valid to call focusLens->setFocusPosition().\n>\n> So the above lensControls.contains() should be redundant.\n\nI've initially been fooled as well and was about to make the same\ncomment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part\nof the list of controls to apply to the lens, not if the control is\nexposed by the subdevice\n\n>\n>\n> > > +\n> > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > +\n> > > + focusLens->setFocusPosition(focusValue.get());\n>\n> But I think Jacopo also has plans to change this so that a libcamera\n> control is passed in anyway, meaning the conversion from libcamera focus\n> position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\n> CameraLens class anyway.\n>\n\nThat's the end goal. Focus lens position still has to be assigned a\ndevice-independent values range that we can re-scale in the device-specific\ncontrol range though...\n\n>\n> > > +}\n> > > +\n> > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > {\n> > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > --\n> > > 2.34.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 297AFC3272\n\tfor ;\n\tTue, 9 Aug 2022 16:01:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B8686332B;\n\tTue, 9 Aug 2022 18:01:41 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C41E661FAA\n\tfor ;\n\tTue, 9 Aug 2022 18:01:40 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id BFA381BF208;\n\tTue, 9 Aug 2022 16:01:39 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660060901;\n\tbh=UQBcrs7O473rRaT6EUKw5WHYCi2XSylP1tBB4roZMIk=;\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=xDVB5EcbvY6dHcNZ9Z/Ra0Avup7qXNxtHt/N28QAVt6LGEnbT9o359V2T0u8InooD\n\tRMvMhD+7IobF0kThwps7/uU5Ls6GiAQirMOxb2h74/dHxjFJ1mw0WllK9mcwQxRnRk\n\tfr2f0Wap8jIthPWcs9Zfay/YPeYMMOU+NH2sHazlPPIR7dqOiSAZviMRRCLLvmXo2J\n\tkYpTCqjOo7C45qWUIPP+43qlH85Zi8mm79nvMU9118WA+uW+YZ9JBQhL0+OcuDs0/5\n\t0XFrgqWTe6l/hcFI5Bar6XiZZgDyXOxWhRimzXgmbaRmik77QKOIafpQK6/7aPsDXP\n\tkCOQoTJUqzLrg==","Date":"Tue, 9 Aug 2022 18:01:37 +0200","To":"Kieran Bingham ","Message-ID":"<20220809160137.rdj2ukhltiegv2zf@uno.localdomain>","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166006025325.15821.17726379324497038019@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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 via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24488,"web_url":"https://patchwork.libcamera.org/comment/24488/","msgid":"<166007429579.2190824.16170040357436874716@Monstersaurus>","date":"2022-08-09T19:44:55","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-08-09 17:01:37)\n> Hi Kieran\n> \n> On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > > Hi Daniel\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> > I think the construction of sensor_->focusLens should make sure that the\n> > correct control is set, so that if we have a valid CameraLens\n> > *focusLens, we know it's valid to call focusLens->setFocusPosition().\n> >\n> > So the above lensControls.contains() should be redundant.\n> \n> I've initially been fooled as well and was about to make the same\n> comment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part\n> of the list of controls to apply to the lens, not if the control is\n> exposed by the subdevice\n\nAha - oops - yes indeed. I see that now. Ok ;-)\n\n--\nKieran","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 2A6A1BE173\n\tfor ;\n\tTue, 9 Aug 2022 19:45:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93BAB63326;\n\tTue, 9 Aug 2022 21:45:00 +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 E75B661FAA\n\tfor ;\n\tTue, 9 Aug 2022 21:44:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B3EC481;\n\tTue, 9 Aug 2022 21:44:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660074300;\n\tbh=3X1nPRmsQcRC3YL4rW56SRJQSUr6mrc9eOyuziMkvLI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zy/JyEhhnXbwkSOM1bWmjQNu60nM3Et0qP2AzqWf3BEIq4YIZUJydaVgOTgo30BXY\n\tcwgaJCqJNCo12fhzeA7qH4U3a66PKvktWdNsITjtFDwyQMESO2DAR+MG65+8LfcjV+\n\t34z4MP5NBLJqt8NMHwYWcDw9rGt8tWQEcGqiIcdsyg2hMKOICQIcObaxAN0la7UH62\n\tqg7327ppPvo8R2UoHWxs+6Q36iJ1n6O8avsjMFoa7ikG0RopfZTkmfZYAnFYtSADUk\n\terbcLqIh4uEnsL998bjqpI69aTofBk1i7oDBLDTUGHVE9qCgycQ3Lg+nvMPm+Kvh1o\n\t5S6lmgQnp6LoQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660074298;\n\tbh=3X1nPRmsQcRC3YL4rW56SRJQSUr6mrc9eOyuziMkvLI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=o04sr9mtjpFmm/5ZX/l+pJ08XHjOlTiMU772+lmHJm6TOw2j0/OLFxGGE8jr6mLNe\n\ta22tHKWFmxZD07t19mZGSJBACaxPlfGNrNu/0+Tw+PI/eCXkMDIVyl5VCqOROl5X9s\n\tqHuiCaHsndbqeDPLqm2UFcgesj2zknlhVfkVNm/Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"o04sr9mt\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220809160137.rdj2ukhltiegv2zf@uno.localdomain>","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>\n\t<20220809160137.rdj2ukhltiegv2zf@uno.localdomain>","To":"Jacopo Mondi ","Date":"Tue, 09 Aug 2022 20:44:55 +0100","Message-ID":"<166007429579.2190824.16170040357436874716@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24509,"web_url":"https://patchwork.libcamera.org/comment/24509/","msgid":"","date":"2022-08-10T07:12:05","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"On Tue, Aug 9, 2022 at 6:01 PM Jacopo Mondi wrote:\n>\n> Hi Kieran\n>\n> On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > > Hi Daniel\n> > >\n> > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > > > 4 files changed, 34 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > index eaf3955e..caa1121a 100644\n> > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > > > interface IPARkISP1EventInterface {\n> > > > paramsBufferReady(uint32 frame);\n> > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > > + setLensControls(libcamera.ControlList sensorControls);\n> > > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > > };\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 2bdb6a81..d6d46b57 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > > > };\n> > > >\n> > > > struct IPAFrameContext {\n> > > > + struct {\n> > > > + uint32_t lensPosition;\n> > > > + bool applyLensCtrls;\n> > > > + } af;\n> > > > +\n> > >\n> > > So, I feel like this patch is better delayed after we have completed\n> > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> > > to retain their state internally instead of using the IPAFrameContext.\n> > >\n> > > There's a series almost landed that renames IPAFrameContext to\n> > > IPAActiveState and there will be a few more to align the two IPA\n> > > modules.\n> > > Would you mind taking this in the series that introduces the AF\n> > > algorithm while I try to collect 1 and 2 asap ?\n> >\n> > It does seem like a lot of this will change indeed. So we really should\n> > try to clean up the interfaces to get it a bit more stable.\n\nSure, I will add this patch to the AF series.\n\nAre these the changes you are talking about?\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=3376\n\nDo you think I can already base the AF changes on this series or there\nare additional major changes expected?\n\nBest regards\nDaniel\n\n> >\n> >\n> > >\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 9e4c48a2..39e1ab2f 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > > >\n> > > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > > + * that holds the current position to something out of the range. */\n> > > > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > > > +\n> > > > context_.frameContext.frameCount = 0;\n> > > >\n> > > > for (auto const &algo : algorithms()) {\n> > > > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > > + context_.frameContext.af.applyLensCtrls = false;\n> > > > + ControlList lensCtrls(*lensCtrls_);\n> > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > > + static_cast(context_.frameContext.af.lensPosition));\n> > > > + setLensControls.emit(lensCtrls);\n> > > > + }\n> > > > }\n> > > >\n> > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 5f10c26b..de0d37da 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -108,6 +108,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> > > > @@ -324,6 +325,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> > > > @@ -378,6 +380,20 @@ 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> > I think the construction of sensor_->focusLens should make sure that the\n> > correct control is set, so that if we have a valid CameraLens\n> > *focusLens, we know it's valid to call focusLens->setFocusPosition().\n> >\n> > So the above lensControls.contains() should be redundant.\n>\n> I've initially been fooled as well and was about to make the same\n> comment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part\n> of the list of controls to apply to the lens, not if the control is\n> exposed by the subdevice\n>\n> >\n> >\n> > > > +\n> > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > +\n> > > > + focusLens->setFocusPosition(focusValue.get());\n> >\n> > But I think Jacopo also has plans to change this so that a libcamera\n> > control is passed in anyway, meaning the conversion from libcamera focus\n> > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\n> > CameraLens class anyway.\n> >\n>\n> That's the end goal. Focus lens position still has to be assigned a\n> device-independent values range that we can re-scale in the device-specific\n> control range though...\n>\n> >\n> > > > +}\n> > > > +\n> > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > > {\n> > > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > > --\n> > > > 2.34.1\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EA686C3272\n\tfor ;\n\tWed, 10 Aug 2022 07:12:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6522863328;\n\tWed, 10 Aug 2022 09:12:19 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4AD6861FAB\n\tfor ;\n\tWed, 10 Aug 2022 09:12:17 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id bq11so20033859lfb.5\n\tfor ;\n\tWed, 10 Aug 2022 00:12:17 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660115539;\n\tbh=d8bAMN5nFYvtXLGADu9NmP4qyiTEYfeKgz5yvjr7h44=;\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=EjM2Xk2o2ZMch+OdUW293yvcldpVd9FeB/oixtrujff2rABOmDIvJaolBkZCQ1Npd\n\trTDhK5L2XQBHqMHHPO6YwYuz103OBwICWYRMERYP00IxtnioC0FRdBerhfbsU8qWdn\n\tTuopWYZVaCSAZ1YoSnhkypmofi/qM0j9Y0ZwMrCFWeggdPwMJbdiXviHjhhc/2QnqV\n\tk7hJhC64wQmGMj0TidZpn+zwfEqvXOThyal27cxuHMaWqHWlKjCGejX+opHAY/EWdY\n\tPjCWflYwcaCgcnDy3eP/BWNpZQRqS0fZ1KUiIOCkWaRXhL9hQeiEyNTNH7skVm5KTr\n\tR1Jr0Wo7t+MJA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc;\n\tbh=g+Jq8gBSSeIGxNnTW1UeCU8LFMwvwhKNK44mYskhA28=;\n\tb=T7MxWdJjCFARvjBFOKl1+UGMcwmR1V8/0gxhrTm4lG9EV7QEmOSAqQss0DSE9qMRjP\n\tDU91JR4N4iaf+V3mAI2FJOvg34jx/73WKy8CdagR37VFMYfbwlH7Ymm6ATwXQhbwzVT1\n\tAMb8aXSun74s+zV8g8SRx6dyElTLnOYStVwuE2PKsSEnID4UT1rSFI36KNMHICH+hyzU\n\tdlEE8sHk0Z9lsXV9Hsf8m4YmIEpTYdFNbEOGwP5yK4mnwv1YS5Scy4gZ9fh0kB0Znfqe\n\thMUmUJcm/dfELlvzuAbQdyfZcT7e9GdbDAz0QUzTcFmG2J5v2Fe2e5YbGAawGzU7/2z/\n\tTXLQ=="],"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=\"T7MxWdJj\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc;\n\tbh=g+Jq8gBSSeIGxNnTW1UeCU8LFMwvwhKNK44mYskhA28=;\n\tb=ZLUM6jlxlSEXUHZxL2zh06oW538ocFVqluqGT29Rm2XEnL99KbFZqGCJ9f4OW7Owpe\n\tChBA9dhrGtXLyaHYZprBLd2v0kA+e3tBvQCZF7yx7hh7yPcgPuCDCiFwZin2meoaQ20j\n\tpiG6ZcMp4wW9i/R4qzwaPFQJ1aeipzOSkUe7Q5/auG4yfcu14vl6wNJOF7M8oXMI0k+2\n\t16UIxp/v2XoaHOA4+YzruBRaq5fH4HJhUVtGefyf0/KllvSC8mILZfqmz03+JJTiH+gZ\n\t4qcgj1S/JnwRLbc/2mKxK0/blXwvENi+Nu8dvT/DNBqkYGcBQ11KufS3yteatcIP4y17\n\tErjg==","X-Gm-Message-State":"ACgBeo2ysJdhyPU88pZMXiecUJIbAW9S2seD5jePEdOG1TZPuNUkZNoh\n\tBf9aEHpoWgBfIk7CAatyTHi3DIVZ54OkKRhdASCD/A==","X-Google-Smtp-Source":"AA6agR4m+iKdd0/d4rooQSIJ5jXm5HT3Pvc/kKUFld7BGAXA6erb7sPtx4b/5EwKI7Mfwph7yPmVMMkpiAk4IId9gk8=","X-Received":"by 2002:a05:6512:2210:b0:48c:eba9:5493 with SMTP id\n\th16-20020a056512221000b0048ceba95493mr4290697lfu.540.1660115536458;\n\tWed, 10 Aug 2022 00:12:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>\n\t<20220809160137.rdj2ukhltiegv2zf@uno.localdomain>","In-Reply-To":"<20220809160137.rdj2ukhltiegv2zf@uno.localdomain>","Date":"Wed, 10 Aug 2022 09:12:05 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24510,"web_url":"https://patchwork.libcamera.org/comment/24510/","msgid":"<20220810072513.jmtylvdmp76ixrw7@uno.localdomain>","date":"2022-08-10T07:25:13","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel\n\nOn Wed, Aug 10, 2022 at 09:12:05AM +0200, Daniel Semkowicz wrote:\n> On Tue, Aug 9, 2022 at 6:01 PM Jacopo Mondi wrote:\n> >\n> > Hi Kieran\n> >\n> > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > > > Hi Daniel\n> > > >\n> > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > > > > 4 files changed, 34 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > index eaf3955e..caa1121a 100644\n> > > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > > > > interface IPARkISP1EventInterface {\n> > > > > paramsBufferReady(uint32 frame);\n> > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > > > + setLensControls(libcamera.ControlList sensorControls);\n> > > > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > > > };\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index 2bdb6a81..d6d46b57 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > > > > };\n> > > > >\n> > > > > struct IPAFrameContext {\n> > > > > + struct {\n> > > > > + uint32_t lensPosition;\n> > > > > + bool applyLensCtrls;\n> > > > > + } af;\n> > > > > +\n> > > >\n> > > > So, I feel like this patch is better delayed after we have completed\n> > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> > > > to retain their state internally instead of using the IPAFrameContext.\n> > > >\n> > > > There's a series almost landed that renames IPAFrameContext to\n> > > > IPAActiveState and there will be a few more to align the two IPA\n> > > > modules.\n> > > > Would you mind taking this in the series that introduces the AF\n> > > > algorithm while I try to collect 1 and 2 asap ?\n> > >\n> > > It does seem like a lot of this will change indeed. So we really should\n> > > try to clean up the interfaces to get it a bit more stable.\n>\n> Sure, I will add this patch to the AF series.\n>\n> Are these the changes you are talking about?\n> https://patchwork.libcamera.org/project/libcamera/list/?series=3376\n\nCorrect!\n\n>\n> Do you think I can already base the AF changes on this series or there\n> are additional major changes expected?\n>\n\nThere will be an additional series to further unify the IPU3 and RkISP\nIPA interfaces towards the pipeline handler. As an example, you can\nsee how different is the IPA::configure() prototype between the two.\n\nSo yes, it will be another rebase, hopefully not too painful.\n\n> Best regards\n> Daniel\n>\n> > >\n> > >\n> > > >\n> > > > > struct {\n> > > > > uint32_t exposure;\n> > > > > double gain;\n> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > index 9e4c48a2..39e1ab2f 100644\n> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > > > >\n> > > > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > > > + * that holds the current position to something out of the range. */\n> > > > > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > > > > +\n> > > > > context_.frameContext.frameCount = 0;\n> > > > >\n> > > > > for (auto const &algo : algorithms()) {\n> > > > > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > > > + context_.frameContext.af.applyLensCtrls = false;\n> > > > > + ControlList lensCtrls(*lensCtrls_);\n> > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > > > + static_cast(context_.frameContext.af.lensPosition));\n> > > > > + setLensControls.emit(lensCtrls);\n> > > > > + }\n> > > > > }\n> > > > >\n> > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index 5f10c26b..de0d37da 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -108,6 +108,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> > > > > @@ -324,6 +325,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> > > > > @@ -378,6 +380,20 @@ 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> > > I think the construction of sensor_->focusLens should make sure that the\n> > > correct control is set, so that if we have a valid CameraLens\n> > > *focusLens, we know it's valid to call focusLens->setFocusPosition().\n> > >\n> > > So the above lensControls.contains() should be redundant.\n> >\n> > I've initially been fooled as well and was about to make the same\n> > comment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part\n> > of the list of controls to apply to the lens, not if the control is\n> > exposed by the subdevice\n> >\n> > >\n> > >\n> > > > > +\n> > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > > +\n> > > > > + focusLens->setFocusPosition(focusValue.get());\n> > >\n> > > But I think Jacopo also has plans to change this so that a libcamera\n> > > control is passed in anyway, meaning the conversion from libcamera focus\n> > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\n> > > CameraLens class anyway.\n> > >\n> >\n> > That's the end goal. Focus lens position still has to be assigned a\n> > device-independent values range that we can re-scale in the device-specific\n> > control range though...\n> >\n> > >\n> > > > > +}\n> > > > > +\n> > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > > > {\n> > > > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > > > --\n> > > > > 2.34.1\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 65F73BE173\n\tfor ;\n\tWed, 10 Aug 2022 07:25:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE00C6332B;\n\tWed, 10 Aug 2022 09:25:17 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49AFF61FAB\n\tfor ;\n\tWed, 10 Aug 2022 09:25:16 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 6EA7D20005;\n\tWed, 10 Aug 2022 07:25:15 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660116317;\n\tbh=3G4JUUJ/P15xNWRHii3Aa7/YAuHHEaE67gdmcXVgV9w=;\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=lW759bWXXNKoebcaTykZEICzCp9Mkt3c09uzGWEVe/lKzVZx4ADl5facFYo6LOpsg\n\tbmntRSiPSeGX6QFwIkV7cE2Kbyd9b5tDgSxCxr5IOU2wtEnaVXsc4ERH6XIDV6sxlh\n\tSA6ALS7zUf8zUUVwS+mJEBI9qI1aj4kPXKx3IlN+xKVfXcCAz3YVY2GCBXXDpsDAd+\n\tQYmH8tU2d2C5FMsdvq/dQIu4DmhzBmfToK7HUg6d2/E44OIHlrAvXNfnI8aNe5vLg8\n\tgXWjMfVxE7t2xs0sjcpW/PDqDsW96leM4qYsT87em+M/HDC8lxlef9hefVYnn7bcHo\n\tdq0wKTUu1dg0w==","Date":"Wed, 10 Aug 2022 09:25:13 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220810072513.jmtylvdmp76ixrw7@uno.localdomain>","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>\n\t<20220809160137.rdj2ukhltiegv2zf@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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 via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24518,"web_url":"https://patchwork.libcamera.org/comment/24518/","msgid":"<20220810113420.ikw4ratahjxselzq@uno.localdomain>","date":"2022-08-10T11:34:20","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel,\n + Kieran and Laurent\n\nOn Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > Hi Daniel\n> >\n> > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > > 4 files changed, 34 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > index eaf3955e..caa1121a 100644\n> > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > > interface IPARkISP1EventInterface {\n> > > paramsBufferReady(uint32 frame);\n> > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > + setLensControls(libcamera.ControlList sensorControls);\n> > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > };\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 2bdb6a81..d6d46b57 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > > };\n> > >\n> > > struct IPAFrameContext {\n> > > + struct {\n> > > + uint32_t lensPosition;\n> > > + bool applyLensCtrls;\n> > > + } af;\n> > > +\n> >\n> > So, I feel like this patch is better delayed after we have completed\n> > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> > to retain their state internally instead of using the IPAFrameContext.\n> >\n> > There's a series almost landed that renames IPAFrameContext to\n> > IPAActiveState and there will be a few more to align the two IPA\n> > modules.\n> > Would you mind taking this in the series that introduces the AF\n> > algorithm while I try to collect 1 and 2 asap ?\n>\n> It does seem like a lot of this will change indeed. So we really should\n> try to clean up the interfaces to get it a bit more stable.\n>\n>\n> >\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 9e4c48a2..39e1ab2f 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > >\n> > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > + * that holds the current position to something out of the range. */\n> > > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > > +\n> > > context_.frameContext.frameCount = 0;\n> > >\n> > > for (auto const &algo : algorithms()) {\n> > > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > + context_.frameContext.af.applyLensCtrls = false;\n> > > + ControlList lensCtrls(*lensCtrls_);\n> > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > + static_cast(context_.frameContext.af.lensPosition));\n> > > + setLensControls.emit(lensCtrls);\n> > > + }\n\nIPU3 here does\n\n\tControlList ctrls(sensorCtrls_);\n\tctrls.set(V4L2_CID_EXPOSURE, exposure);\n\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n\n\tControlList lensCtrls(lensCtrls_);\n\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n\t\t static_cast(context_.activeState.af.focus));\n\n\tsetSensorControls.emit(frame, ctrls, lensCtrls);\n\nWhile Daniel has implemented\n\n\tControlList ctrls(sensorCtrls_);\n\tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n\tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n\n\tsetSensorControls.emit(frame, ctrls);\n\tif (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n\t\tcontext_.frameContext.af.applyLensCtrls = false;\n\t\tControlList lensCtrls(*lensCtrls_);\n\t\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n\t\t\t\tstatic_cast(context_.frameContext.af.lensPosition));\n\t\tsetLensControls.emit(lensCtrls);\n\t}\n\nYesterday we briefly discussed about the possibility of sending\nboth sensor and lens controls as part of a single control list, but\nwhat's the preference in the meantime ? One signal with 2 lists or 2\nsignals ?\n\nI would prefer a single signal, mostly because it's handling is\nsynchronous and we can avoid one context switch ?\n\n> > > }\n> > >\n> > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 5f10c26b..de0d37da 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -108,6 +108,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> > > @@ -324,6 +325,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> > > @@ -378,6 +380,20 @@ 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> I think the construction of sensor_->focusLens should make sure that the\n> correct control is set, so that if we have a valid CameraLens\n> *focusLens, we know it's valid to call focusLens->setFocusPosition().\n>\n> So the above lensControls.contains() should be redundant.\n>\n>\n> > > +\n> > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > +\n> > > + focusLens->setFocusPosition(focusValue.get());\n>\n> But I think Jacopo also has plans to change this so that a libcamera\n> control is passed in anyway, meaning the conversion from libcamera focus\n> position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\n> CameraLens class anyway.\n>\n>\n> > > +}\n> > > +\n> > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > {\n> > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > --\n> > > 2.34.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F2E30C3272\n\tfor ;\n\tWed, 10 Aug 2022 11:34:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 284736332B;\n\tWed, 10 Aug 2022 13:34:25 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::223])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6E9B600EA\n\tfor ;\n\tWed, 10 Aug 2022 13:34:23 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 9BA3E60004;\n\tWed, 10 Aug 2022 11:34:22 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660131265;\n\tbh=8hiY+Tfd4mMndsRGtNjvd56XkyRO9XsyIXe8HkP7QNQ=;\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=IGjSSj0ZzrwBRhECZl71hlvSyk+jNmHwxkwsKoQ4vPz3mEByQs09qru64EiN+A7Q5\n\t0Sj9+JIaphmCivJuZ6I7hC0UKs0R6thIATBVc7GNEqA2OzSRUHhPuVAUfZiwZKgtHs\n\tIvj/PqIB/t5G5UTd3xx1SS58fy/PENPX8g/BhBepw44SQ7qidTb5sZwb7zR4EW0c+k\n\tHMWTXZr+rqChIeB3zPxk/SmSEdLRWBwlua2lNIiVGBm9cMS63AE+gixLEt641U6OAP\n\tGj5jaZIBq+0gWwwlkWsZLbhwkl7RQNbZAdwOd7NdkN9edtf1gk6zR8FXeVlqdvemZP\n\tdLlw4SDbFT+yA==","Date":"Wed, 10 Aug 2022 13:34:20 +0200","To":"Kieran Bingham ","Message-ID":"<20220810113420.ikw4ratahjxselzq@uno.localdomain>","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166006025325.15821.17726379324497038019@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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 via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24539,"web_url":"https://patchwork.libcamera.org/comment/24539/","msgid":"","date":"2022-08-11T07:10:48","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi!\n\nOn Wed, Aug 10, 2022 at 1:34 PM Jacopo Mondi wrote:\n>\n> Hi Daniel,\n> + Kieran and Laurent\n>\n> On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > > Hi Daniel\n> > >\n> > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > > > 4 files changed, 34 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > index eaf3955e..caa1121a 100644\n> > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > > > interface IPARkISP1EventInterface {\n> > > > paramsBufferReady(uint32 frame);\n> > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > > + setLensControls(libcamera.ControlList sensorControls);\n> > > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > > };\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 2bdb6a81..d6d46b57 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > > > };\n> > > >\n> > > > struct IPAFrameContext {\n> > > > + struct {\n> > > > + uint32_t lensPosition;\n> > > > + bool applyLensCtrls;\n> > > > + } af;\n> > > > +\n> > >\n> > > So, I feel like this patch is better delayed after we have completed\n> > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> > > to retain their state internally instead of using the IPAFrameContext.\n> > >\n> > > There's a series almost landed that renames IPAFrameContext to\n> > > IPAActiveState and there will be a few more to align the two IPA\n> > > modules.\n> > > Would you mind taking this in the series that introduces the AF\n> > > algorithm while I try to collect 1 and 2 asap ?\n> >\n> > It does seem like a lot of this will change indeed. So we really should\n> > try to clean up the interfaces to get it a bit more stable.\n> >\n> >\n> > >\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 9e4c48a2..39e1ab2f 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > > >\n> > > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > > + * that holds the current position to something out of the range. */\n> > > > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > > > +\n> > > > context_.frameContext.frameCount = 0;\n> > > >\n> > > > for (auto const &algo : algorithms()) {\n> > > > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > > + context_.frameContext.af.applyLensCtrls = false;\n> > > > + ControlList lensCtrls(*lensCtrls_);\n> > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > > + static_cast(context_.frameContext.af.lensPosition));\n> > > > + setLensControls.emit(lensCtrls);\n> > > > + }\n>\n> IPU3 here does\n>\n> ControlList ctrls(sensorCtrls_);\n> ctrls.set(V4L2_CID_EXPOSURE, exposure);\n> ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n>\n> ControlList lensCtrls(lensCtrls_);\n> lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> static_cast(context_.activeState.af.focus));\n>\n> setSensorControls.emit(frame, ctrls, lensCtrls);\n>\n> While Daniel has implemented\n>\n> ControlList ctrls(sensorCtrls_);\n> ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n>\n> setSensorControls.emit(frame, ctrls);\n> if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> context_.frameContext.af.applyLensCtrls = false;\n> ControlList lensCtrls(*lensCtrls_);\n> lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> static_cast(context_.frameContext.af.lensPosition));\n> setLensControls.emit(lensCtrls);\n> }\n>\n> Yesterday we briefly discussed about the possibility of sending\n> both sensor and lens controls as part of a single control list, but\n> what's the preference in the meantime ? One signal with 2 lists or 2\n> signals ?\n>\n> I would prefer a single signal, mostly because it's handling is\n> synchronous and we can avoid one context switch ?\n\nSingle control list sounds as the best solution in my opinion.\nEspecially that lens have only one control item. If there are plans to\nuse a single control list, then in the meantime, I would go with a\nsingle signal and two lists. It would be closer to the final idea.\n\nBest regards\nDaniel\n\n>\n> > > > }\n> > > >\n> > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 5f10c26b..de0d37da 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -108,6 +108,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> > > > @@ -324,6 +325,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> > > > @@ -378,6 +380,20 @@ 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> > I think the construction of sensor_->focusLens should make sure that the\n> > correct control is set, so that if we have a valid CameraLens\n> > *focusLens, we know it's valid to call focusLens->setFocusPosition().\n> >\n> > So the above lensControls.contains() should be redundant.\n> >\n> >\n> > > > +\n> > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > +\n> > > > + focusLens->setFocusPosition(focusValue.get());\n> >\n> > But I think Jacopo also has plans to change this so that a libcamera\n> > control is passed in anyway, meaning the conversion from libcamera focus\n> > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\n> > CameraLens class anyway.\n> >\n> >\n> > > > +}\n> > > > +\n> > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > > {\n> > > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > > --\n> > > > 2.34.1\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BBE1BBE173\n\tfor ;\n\tThu, 11 Aug 2022 07:11:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2583F6332B;\n\tThu, 11 Aug 2022 09:11:03 +0200 (CEST)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8E3263327\n\tfor ;\n\tThu, 11 Aug 2022 09:11:00 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id o2so17056459lfb.1\n\tfor ;\n\tThu, 11 Aug 2022 00:11:00 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660201863;\n\tbh=SllA3mgqvD0nEFj+4YPxrAVyBY3XYLI2zDMZg7DjON8=;\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=eIgcITPLQLx2SDQ+20YSL672QmwOMGCrpUNU3NvJIPPAOGW3i1HndjBatIbrUqq56\n\tJjCEqZnVgfWqpkdHq1wP4qFDSijEwcToDWMJfT/Q1nGZ12pjwxQPPN/3q36LAwYsKI\n\teK0Z0Q97GKDsw0no1H1+yh51brgzGGha78z8O+jXTfgsGFllOVyJTBXWb4hDqT7oi2\n\tQ0z3Ab9T3uxuIvRzE14YQTqexjEYT0IRJPDe2JuxYvXgjHdP1zFGgFcrkLwNr1r9MG\n\tqmd0ygeFpOzxYhGtoXlssY10AUKUEU8G/42Wn0HHLagGX48GXskT1VfX3GNarpcyNg\n\tKf+MkYbsjMW0Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc;\n\tbh=4zOUT7Cl7oCC9l2Ma6QWD0UNKsjTtHB05H0qW0cjBLU=;\n\tb=S87vxlxmLKnqeI1cJMzi7lRpTf5P/BMQAeSkmyVmFfjifqBV5bralpux/nSvHCxuQc\n\t8qUP+x/AQrGHadVOw6YQZUur9q7q/yIvGifhXU3dvJH30/T85ELFNdsJD9ntHDppXUqC\n\t7PRa88bhQ3dT5HasNoBq0mmdLGBkAttLIIi2bsvOHMye0J+66xfGAH36rXOj7pGn+oOM\n\ttr7mDVOeS98rX4rwxoJUQ+NhTtnwqnOZ2deRZahYhJuJjYMtRuwMHCG9vRIjrm5dzegZ\n\t+cihQ/fHe343iJCd/rJXgdULHwEDUxiTv9BvvBEtshbgMvzYgHqHuYzGlTZI7CvlwLLJ\n\txa4Q=="],"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=\"S87vxlxm\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc;\n\tbh=4zOUT7Cl7oCC9l2Ma6QWD0UNKsjTtHB05H0qW0cjBLU=;\n\tb=5FZ+Gxf/VeNY0C22N4OeBG64aPev5G0s+4fxZJZCmYT5hgbRdeX+Bb//abpCjKD0Ec\n\tcUgoTqwVac8vNlHdQTYvr5gaOvEFw2e697UQt9Z19swqY7NgXgYcSEfdtJcf23YmaxFv\n\tIgAV2HEcGVQko2iwnOCO6GfEK9FTEGnoWitNb4Ut0Z5uDWfRj9DZhVUTIgpgBsJXKj3Z\n\tcjy4k4hoS1ETZCfYhGRX20zOgbGZJcXYnuY9aGQRc+NqBl1tOTzMBSM3VXM7HHd7QTMW\n\teBPzH+zqh5PbRFE25yU79Nc6GmirGtbVQ1IvoBO22lHd+HXFYyn0+WEHt8ZCqhejVxc6\n\tASag==","X-Gm-Message-State":"ACgBeo1iQ+uxQYTHv4e0DdCkeaJ2mljZMavPUFMhWWOAr/4SP7Tnpill\n\te2aIcWcMow2yxOrlm67QfYMlNeZJHGCk3dxJonnl6g==","X-Google-Smtp-Source":"AA6agR7psV313XBq+R4OR07xP0526bdM4jJqCvETUx+tMS7Wctcp/ZfTmXdyaCXwIwv9SHHwR403COaMt2Bm8mi3w8A=","X-Received":"by 2002:a05:6512:1154:b0:48b:3020:b29 with SMTP id\n\tm20-20020a056512115400b0048b30200b29mr10319310lfg.338.1660201859855;\n\tThu, 11 Aug 2022 00:10:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>\n\t<20220810113420.ikw4ratahjxselzq@uno.localdomain>","In-Reply-To":"<20220810113420.ikw4ratahjxselzq@uno.localdomain>","Date":"Thu, 11 Aug 2022 09:10:48 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24544,"web_url":"https://patchwork.libcamera.org/comment/24544/","msgid":"<20220811082236.4lirxia24znbvnee@uno.localdomain>","date":"2022-08-11T08:22:36","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel\n\nOn Thu, Aug 11, 2022 at 09:10:48AM +0200, Daniel Semkowicz wrote:\n> Hi!\n>\n> On Wed, Aug 10, 2022 at 1:34 PM Jacopo Mondi wrote:\n> >\n> > Hi Daniel,\n> > + Kieran and Laurent\n> >\n> > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > > > Hi Daniel\n> > > >\n> > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > > > > 4 files changed, 34 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > index eaf3955e..caa1121a 100644\n> > > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > > > > interface IPARkISP1EventInterface {\n> > > > > paramsBufferReady(uint32 frame);\n> > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > > > + setLensControls(libcamera.ControlList sensorControls);\n> > > > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > > > };\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index 2bdb6a81..d6d46b57 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > > > > };\n> > > > >\n> > > > > struct IPAFrameContext {\n> > > > > + struct {\n> > > > > + uint32_t lensPosition;\n> > > > > + bool applyLensCtrls;\n> > > > > + } af;\n> > > > > +\n> > > >\n> > > > So, I feel like this patch is better delayed after we have completed\n> > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> > > > to retain their state internally instead of using the IPAFrameContext.\n> > > >\n> > > > There's a series almost landed that renames IPAFrameContext to\n> > > > IPAActiveState and there will be a few more to align the two IPA\n> > > > modules.\n> > > > Would you mind taking this in the series that introduces the AF\n> > > > algorithm while I try to collect 1 and 2 asap ?\n> > >\n> > > It does seem like a lot of this will change indeed. So we really should\n> > > try to clean up the interfaces to get it a bit more stable.\n> > >\n> > >\n> > > >\n> > > > > struct {\n> > > > > uint32_t exposure;\n> > > > > double gain;\n> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > index 9e4c48a2..39e1ab2f 100644\n> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > > > >\n> > > > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > > > + * that holds the current position to something out of the range. */\n> > > > > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > > > > +\n> > > > > context_.frameContext.frameCount = 0;\n> > > > >\n> > > > > for (auto const &algo : algorithms()) {\n> > > > > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > > > + context_.frameContext.af.applyLensCtrls = false;\n> > > > > + ControlList lensCtrls(*lensCtrls_);\n> > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > > > + static_cast(context_.frameContext.af.lensPosition));\n> > > > > + setLensControls.emit(lensCtrls);\n> > > > > + }\n> >\n> > IPU3 here does\n> >\n> > ControlList ctrls(sensorCtrls_);\n> > ctrls.set(V4L2_CID_EXPOSURE, exposure);\n> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> >\n> > ControlList lensCtrls(lensCtrls_);\n> > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > static_cast(context_.activeState.af.focus));\n> >\n> > setSensorControls.emit(frame, ctrls, lensCtrls);\n> >\n> > While Daniel has implemented\n> >\n> > ControlList ctrls(sensorCtrls_);\n> > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n> >\n> > setSensorControls.emit(frame, ctrls);\n> > if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > context_.frameContext.af.applyLensCtrls = false;\n> > ControlList lensCtrls(*lensCtrls_);\n> > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > static_cast(context_.frameContext.af.lensPosition));\n> > setLensControls.emit(lensCtrls);\n> > }\n> >\n> > Yesterday we briefly discussed about the possibility of sending\n> > both sensor and lens controls as part of a single control list, but\n> > what's the preference in the meantime ? One signal with 2 lists or 2\n> > signals ?\n> >\n> > I would prefer a single signal, mostly because it's handling is\n> > synchronous and we can avoid one context switch ?\n>\n> Single control list sounds as the best solution in my opinion.\n> Especially that lens have only one control item. If there are plans to\n> use a single control list, then in the meantime, I would go with a\n\nYes, that's the idea. When we'll be done aligning the two IPA\nimplementations we're going to move the IPA to use libcamera::controls\nand have the libcamera::control->v4l2::control translation in the\nCameraSensor/CameraLens class.\n\nI have sent a few weeks ago a first version [1], which need to be rebased\non top of [2]. So yeah, things are moving a little too much in this\narea, that's why I suggested you to post-pone the AF algorithm series\nif that's not a problem with you.\n\n[1] https://patchwork.libcamera.org/project/libcamera/list/?series=3238\n[2] https://patchwork.libcamera.org/project/libcamera/list/?series=3376\n\n> single signal and two lists. It would be closer to the final idea.\n>\n> Best regards\n> Daniel\n>\n> >\n> > > > > }\n> > > > >\n> > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index 5f10c26b..de0d37da 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -108,6 +108,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> > > > > @@ -324,6 +325,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> > > > > @@ -378,6 +380,20 @@ 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> > > I think the construction of sensor_->focusLens should make sure that the\n> > > correct control is set, so that if we have a valid CameraLens\n> > > *focusLens, we know it's valid to call focusLens->setFocusPosition().\n> > >\n> > > So the above lensControls.contains() should be redundant.\n> > >\n> > >\n> > > > > +\n> > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > > +\n> > > > > + focusLens->setFocusPosition(focusValue.get());\n> > >\n> > > But I think Jacopo also has plans to change this so that a libcamera\n> > > control is passed in anyway, meaning the conversion from libcamera focus\n> > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\n> > > CameraLens class anyway.\n> > >\n> > >\n> > > > > +}\n> > > > > +\n> > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > > > {\n> > > > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > > > --\n> > > > > 2.34.1\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C3C7DBE173\n\tfor ;\n\tThu, 11 Aug 2022 08:22:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1394C6332B;\n\tThu, 11 Aug 2022 10:22:41 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E85E63327\n\tfor ;\n\tThu, 11 Aug 2022 10:22:39 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4D9BD240006;\n\tThu, 11 Aug 2022 08:22:38 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660206161;\n\tbh=TRwKin6XEdFLgNJNdTht6L+70Jp4cHO608bmqote+So=;\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=fihvZrim5j79fDALtYtqSlvTWo8P0hZYjeHl8xDJmDkeaHQjfG8C75CQwQIrjEWS7\n\ttE1DLXd8wTfivz3sLQwfiRPyP/jtzY2tleaPyE7sN4mSMSJlfBdFzNWrM553YCv922\n\tmuyxp101I6i7dY5tPhE6PnJjD1kRut92GpZi87YTaXs1RCHxoHf19SFukJ02yQtTm6\n\tPoycQ++YFscEkJZFndLiG873dD27cD+OokxGGXBalIQkt447WENf7CCWxv0apLS0hT\n\txTcggViLN4gkYuW35TeUCKEOm+eSClEmIn+1Z/YB4HrMTGRw1o/DF25Ij9aur6sAWC\n\tjnMTYja0KLYZQ==","Date":"Thu, 11 Aug 2022 10:22:36 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220811082236.4lirxia24znbvnee@uno.localdomain>","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>\n\t<20220810113420.ikw4ratahjxselzq@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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 via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24547,"web_url":"https://patchwork.libcamera.org/comment/24547/","msgid":"","date":"2022-08-11T09:21:17","subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control from IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"On Thu, Aug 11, 2022 at 10:22 AM Jacopo Mondi wrote:\n>\n> Hi Daniel\n>\n> On Thu, Aug 11, 2022 at 09:10:48AM +0200, Daniel Semkowicz wrote:\n> > Hi!\n> >\n> > On Wed, Aug 10, 2022 at 1:34 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Daniel,\n> > > + Kieran and Laurent\n> > >\n> > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:\n> > > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)\n> > > > > Hi Daniel\n> > > > >\n> > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel 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/ipa_context.h | 5 +++++\n> > > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++\n> > > > > > 4 files changed, 34 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > index eaf3955e..caa1121a 100644\n> > > > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {\n> > > > > > interface IPARkISP1EventInterface {\n> > > > > > paramsBufferReady(uint32 frame);\n> > > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > > > > + setLensControls(libcamera.ControlList sensorControls);\n> > > > > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > > > > };\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > index 2bdb6a81..d6d46b57 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {\n> > > > > > };\n> > > > > >\n> > > > > > struct IPAFrameContext {\n> > > > > > + struct {\n> > > > > > + uint32_t lensPosition;\n> > > > > > + bool applyLensCtrls;\n> > > > > > + } af;\n> > > > > > +\n> > > > >\n> > > > > So, I feel like this patch is better delayed after we have completed\n> > > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms\n> > > > > to retain their state internally instead of using the IPAFrameContext.\n> > > > >\n> > > > > There's a series almost landed that renames IPAFrameContext to\n> > > > > IPAActiveState and there will be a few more to align the two IPA\n> > > > > modules.\n> > > > > Would you mind taking this in the series that introduces the AF\n> > > > > algorithm while I try to collect 1 and 2 asap ?\n> > > >\n> > > > It does seem like a lot of this will change indeed. So we really should\n> > > > try to clean up the interfaces to get it a bit more stable.\n> > > >\n> > > >\n> > > > >\n> > > > > > struct {\n> > > > > > uint32_t exposure;\n> > > > > > double gain;\n> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > index 9e4c48a2..39e1ab2f 100644\n> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > > > > >\n> > > > > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > > > > + * that holds the current position to something out of the range. */\n> > > > > > + context_.frameContext.af.lensPosition = std::numeric_limits::max();\n> > > > > > +\n> > > > > > context_.frameContext.frameCount = 0;\n> > > > > >\n> > > > > > for (auto const &algo : algorithms()) {\n> > > > > > @@ -348,6 +352,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 (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > > > > + context_.frameContext.af.applyLensCtrls = false;\n> > > > > > + ControlList lensCtrls(*lensCtrls_);\n> > > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > > > > + static_cast(context_.frameContext.af.lensPosition));\n> > > > > > + setLensControls.emit(lensCtrls);\n> > > > > > + }\n> > >\n> > > IPU3 here does\n> > >\n> > > ControlList ctrls(sensorCtrls_);\n> > > ctrls.set(V4L2_CID_EXPOSURE, exposure);\n> > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> > >\n> > > ControlList lensCtrls(lensCtrls_);\n> > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > static_cast(context_.activeState.af.focus));\n> > >\n> > > setSensorControls.emit(frame, ctrls, lensCtrls);\n> > >\n> > > While Daniel has implemented\n> > >\n> > > ControlList ctrls(sensorCtrls_);\n> > > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n> > >\n> > > setSensorControls.emit(frame, ctrls);\n> > > if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {\n> > > context_.frameContext.af.applyLensCtrls = false;\n> > > ControlList lensCtrls(*lensCtrls_);\n> > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > static_cast(context_.frameContext.af.lensPosition));\n> > > setLensControls.emit(lensCtrls);\n> > > }\n> > >\n> > > Yesterday we briefly discussed about the possibility of sending\n> > > both sensor and lens controls as part of a single control list, but\n> > > what's the preference in the meantime ? One signal with 2 lists or 2\n> > > signals ?\n> > >\n> > > I would prefer a single signal, mostly because it's handling is\n> > > synchronous and we can avoid one context switch ?\n> >\n> > Single control list sounds as the best solution in my opinion.\n> > Especially that lens have only one control item. If there are plans to\n> > use a single control list, then in the meantime, I would go with a\n>\n> Yes, that's the idea. When we'll be done aligning the two IPA\n> implementations we're going to move the IPA to use libcamera::controls\n> and have the libcamera::control->v4l2::control translation in the\n> CameraSensor/CameraLens class.\n>\n> I have sent a few weeks ago a first version [1], which need to be rebased\n> on top of [2]. So yeah, things are moving a little too much in this\n> area, that's why I suggested you to post-pone the AF algorithm series\n> if that's not a problem with you.\n>\n> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=3238\n> [2] https://patchwork.libcamera.org/project/libcamera/list/?series=3376\n\nHmm, it makes sense to postpone it until the IPA will be stable as these\nare conflicting changes.\nI already made some changes to the AF after the discussions, so I will\nsubmit my current version, but I will wait for the above series to be\nmerged and then rebase everything.\n\nBest regards\nDaniel\n\n>\n> > single signal and two lists. It would be closer to the final idea.\n> >\n> > Best regards\n> > Daniel\n> >\n> > >\n> > > > > > }\n> > > > > >\n> > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > index 5f10c26b..de0d37da 100644\n> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > @@ -108,6 +108,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> > > > > > @@ -324,6 +325,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> > > > > > @@ -378,6 +380,20 @@ 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> > > > I think the construction of sensor_->focusLens should make sure that the\n> > > > correct control is set, so that if we have a valid CameraLens\n> > > > *focusLens, we know it's valid to call focusLens->setFocusPosition().\n> > > >\n> > > > So the above lensControls.contains() should be redundant.\n> > > >\n> > > >\n> > > > > > +\n> > > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > > > +\n> > > > > > + focusLens->setFocusPosition(focusValue.get());\n> > > >\n> > > > But I think Jacopo also has plans to change this so that a libcamera\n> > > > control is passed in anyway, meaning the conversion from libcamera focus\n> > > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the\n> > > > CameraLens class anyway.\n> > > >\n> > > >\n> > > > > > +}\n> > > > > > +\n> > > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > > > > {\n> > > > > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > > > > --\n> > > > > > 2.34.1\n> > > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CCA58C3272\n\tfor ;\n\tThu, 11 Aug 2022 09:21:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D02F6332B;\n\tThu, 11 Aug 2022 11:21:31 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 69FA963327\n\tfor ;\n\tThu, 11 Aug 2022 11:21:29 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id v10so15204014ljh.9\n\tfor ;\n\tThu, 11 Aug 2022 02:21:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660209691;\n\tbh=V/t9oOi7fL031s0lLvGFS/C2ycI3EZMAn7FuSWqyfd0=;\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=CHlhjvXRkdjb4olltfgP7+Oj6CZ4bib+4MW+OQY/1bJEEp8a7uSKjQW/J9vKVLvGR\n\t8BG1HvyBE6l89jJ02SdCzdLgivULFQcht0rMxbaIIeybPpSzsqQSgx0ssXsWBs9whk\n\tdfzTjzH3kzZ39iIhMncwzL6lkqigVRCpIvo5Ah/DOshnBSQtLxK3qyl3fR7ErFIJhn\n\tyoFDiqCwO4F1UzUGEYvxMWmi7NxOl4xxvbCl+i8+00j3FBFLffkViQagauZ9wCjaaP\n\tKb9lmqhJQkCTtNkGJykkwNOLDZ8JLZW0USz43Kd+WGoF2N3Cq/r3/d3zre0QyNVr8N\n\tfKqElUJP7bRpg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc;\n\tbh=GSBbNjLkXf1vK7DjYD5G6e4f3punaY/dgWYLKGZ9CpY=;\n\tb=LDTeaH2HnE0zqLKmrdGKkHNAy0B1izeYSWyY/voMIb4f6BkikhdtnfOMaVKxaQn6m+\n\tTrei44eR7jaL+no0vSLfQ28YOcTDTQGAA8GDNQziebxLn80tYr1maiCt9z/ecUVhiAhn\n\tISh7nWpgnu7Pym4ZXUD1a7WTsVRtVYjuTU9tTyzZW95P1rhaWqoBgv6gNvnAT2wV4aPe\n\thap4Bbv9XVPhHsYfpwMPfp5CgKMJjUR18RKcEa7kUGHGoWN8JQl9gQmTuh43aKFWHfOR\n\th+qfPZRU7K73Wj3ZEQpIsZk1clSyyf69p4oGo1ozxe3suwagnn6XLWYZYbirsQUtHW0p\n\tS3hw=="],"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=\"LDTeaH2H\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc;\n\tbh=GSBbNjLkXf1vK7DjYD5G6e4f3punaY/dgWYLKGZ9CpY=;\n\tb=gbD0yBPAgAFzrtVN6uHMHnbY6J8SUdL5qlBvj5DwFUXgbbFOUYSwPWLsmS5GxNNDHr\n\tM8jzDUeVNKppKVERjY/b1QjdtR326xKIiJER7rZC9jHekov5bjjfbx12dY64mcTw9asm\n\t2diwCLSfLN+MVNxTGumcyZGB1B1OVo4qRfNXRyAZAO3C7auIGbG+xTd2Txao0qBw8mR8\n\trBuTguMvAw/i+lYlBpJzOQNCtSosVZSmEmff5tYaU3bYo82uYA+555wplLK8X2DrfZ0p\n\tJ2ubnEb6hg1qkWGP38zwAzNGnVHpuZWM6Czjihz0LwWEJaPtTQmImOkLqIf+2I81YkUe\n\t/Omg==","X-Gm-Message-State":"ACgBeo1FZIPwJf0cdb0K9r5++iohM3VBh7SdsFdp/nlrSRyg4X+EmKkM\n\tZ9q6PATeBK/M1/jbiYTx0hs4J2dbPcPnzOoU49hhCckAGfZx+aCk","X-Google-Smtp-Source":"AA6agR4LTFp6eoQWT2zr4pTShHd0P32bMatDy74pN6dYM51Aiwb53am+go6eFXS1EPmDSncO/C+2bDIoxU68x/u13xI=","X-Received":"by 2002:a2e:a811:0:b0:25e:3220:d785 with SMTP id\n\tl17-20020a2ea811000000b0025e3220d785mr10038759ljq.438.1660209688665;\n\tThu, 11 Aug 2022 02:21:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20220809144704.61682-1-dse@thaumatec.com>\n\t<20220809144704.61682-4-dse@thaumatec.com>\n\t<20220809152513.i5f53xui5mi6lojh@uno.localdomain>\n\t<166006025325.15821.17726379324497038019@Monstersaurus>\n\t<20220810113420.ikw4ratahjxselzq@uno.localdomain>\n\t\n\t<20220811082236.4lirxia24znbvnee@uno.localdomain>","In-Reply-To":"<20220811082236.4lirxia24znbvnee@uno.localdomain>","Date":"Thu, 11 Aug 2022 11:21:17 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens\n\tposition control 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]