[{"id":26401,"web_url":"https://patchwork.libcamera.org/comment/26401/","msgid":"<20230206094559.s44nxnphzckfvgaz@uno.localdomain>","date":"2023-02-06T09:45:59","subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Thu, Jan 19, 2023 at 09:41:06AM +0100, 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\nAs a minor nit: I would move this to the end of the series, after\nhaving plumbed in the algorithm implementation... Just a nit though\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 bf6e9141..c3ed87aa 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n> interface IPARkISP1EventInterface {\n> \tparamsBufferReady(uint32 frame);\n> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> +\tsetLensControls(libcamera.ControlList lensControls);\n> \tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index b9b20653..1fac6af9 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -53,6 +53,11 @@ struct IPASessionConfiguration {\n> };\n>\n> struct IPAActiveState {\n> +\tstruct {\n> +\t\tuint32_t lensPosition;\n> +\t\tbool applyLensCtrls;\n\nOh gosh lens are -complicated- /o\\\n\nFor regular sensor controls we defer the decision to apply or not a\ncontrol to DelayedControls on the pipeline handler side, which takes\ncare of evaluating if a control has to be updated or not and how many\nframe it takes to take effect.\n\nLenses (well, VCM to be fair) are a bit more complex than that, in the\nsense that moving the lens might take a number of frames that depends\non the movement range. Some VCM datasheet I've seen provide a model to\nestimate the delay depending on the movement range and the VCM\ncharacteristics. The risk is that updating the lens position while the\nlens hasn't reached its final position might trigger some resonation\neffect, especially if the algorithm runs in \"continuous auto-focus\"\nmode and tries to update the lens position too often.\n\nI've cc-ed Matthias Fend which has recently sent a series for \"more\ncomplex optics\"\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=3735\n(which partially overlaps with the work you've done here) as he\ncertainly knows more than me about VCM and optics to know what his\nopinion is\n\n\n> +\t} af;\n> +\n> \tstruct {\n> \t\tstruct {\n> \t\t\tuint32_t exposure;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 9e861fc0..297161b2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> \t\t});\n>\n> +\t/* Lens position is unknown at the startup, so initilize the variable\n ^ initialize\n> +\t * that holds the current position to something out of the range. */\n\nMultiline comments as\n /*\n * first line\n * next line\n */\n\n> +\tcontext_.activeState.af.lensPosition = std::numeric_limits::max();\n> +\n> \tfor (auto const &a : algorithms()) {\n> \t\tAlgorithm *algo = static_cast(a.get());\n>\n> @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n>\n> \tsetSensorControls.emit(frame, ctrls);\n> +\n> +\tif (lensControls_ && context_.activeState.af.applyLensCtrls) {\n> +\t\tcontext_.activeState.af.applyLensCtrls = false;\n> +\t\tControlList lensCtrls(*lensControls_);\n> +\t\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> +\t\t\t static_cast(context_.activeState.af.lensPosition));\n> +\t\tsetLensControls.emit(lensCtrls);\n> +\t}\n> }\n>\n> } /* namespace ipa::rkisp1 */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 0559d261..b2fedc5f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -113,6 +113,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> @@ -337,6 +338,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> @@ -400,6 +402,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.39.0\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 990A3BEFBE\n\tfor ;\n\tMon, 6 Feb 2023 09:46:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DEE7F61EF3;\n\tMon, 6 Feb 2023 10:46:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86E3F61EF3\n\tfor ;\n\tMon, 6 Feb 2023 10:46:03 +0100 (CET)","from ideasonboard.com (host-79-55-56-167.retail.telecomitalia.it\n\t[79.55.56.167])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D1AF24DA;\n\tMon, 6 Feb 2023 10:46:02 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675676764;\n\tbh=oBdzGXO8YKSZcRr4sbvuz1eHJbxiWKo3g1ZhfeuVM5Y=;\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=BPYssGVYrpgZwf0U6U31i1Dh3EjUmsRSzia3zq2PUYExIp5GpWCIJgPYbjzJiTCNv\n\tgadCw7N+W/T7fQ3tNlNpfXJjqj24avwaUuZ5SXWwYfj+DUcEPA9Fm6jmIgDg1nIErY\n\tETyxA1Zcsk7krO9Tl5beMbiDFg7OEDxQBtkObg0OlhkYHHGKbwNrY9/MxlzbXeE2/m\n\t3gtIXnW/rT4SsyKN5Pue7wjHMQ8mc6azo22a1PGxgy5eFYcYqsAs4ffLEJMFRzhDjD\n\tUlWltgY0WYdL3ybKW+p7oX7ehys2wrga3qsPaf8ihfc++uQhSiG17UcIq00X25m7rL\n\tXLvVsZpgneS6g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675676763;\n\tbh=oBdzGXO8YKSZcRr4sbvuz1eHJbxiWKo3g1ZhfeuVM5Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bsOzvjBm9+9rm2iBCPLn27lGcyLuu2PWembnyK4rzvXHcYYDkXBH2Jx9ZPhV90sgR\n\tkK5dnj9wDf64Ailcr4+NiNQku5tvZwNR9wIqmvgTTRX9eWItg/S+gKhu1N0g3SrWQR\n\tDBizbtxyIKyWA6LWsFm8CugChFfY6+GcVjfo3FrI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bsOzvjBm\"; dkim-atps=neutral","Date":"Mon, 6 Feb 2023 10:45:59 +0100","To":"Daniel Semkowicz , matthias.fend@emfend.at","Message-ID":"<20230206094559.s44nxnphzckfvgaz@uno.localdomain>","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-3-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230119084112.20564-3-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26404,"web_url":"https://patchwork.libcamera.org/comment/26404/","msgid":"","date":"2023-02-06T11:23:49","subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Jacopo and Daniel\n\nOn Mon, 6 Feb 2023 at 09:46, Jacopo Mondi via libcamera-devel\n wrote:\n>\n> Hi Daniel\n>\n> On Thu, Jan 19, 2023 at 09:41:06AM +0100, 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>\n> As a minor nit: I would move this to the end of the series, after\n> having plumbed in the algorithm implementation... Just a nit though\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 bf6e9141..c3ed87aa 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n> > interface IPARkISP1EventInterface {\n> > paramsBufferReady(uint32 frame);\n> > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > + setLensControls(libcamera.ControlList lensControls);\n> > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > };\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index b9b20653..1fac6af9 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -53,6 +53,11 @@ struct IPASessionConfiguration {\n> > };\n> >\n> > struct IPAActiveState {\n> > + struct {\n> > + uint32_t lensPosition;\n> > + bool applyLensCtrls;\n>\n> Oh gosh lens are -complicated- /o\\\n>\n> For regular sensor controls we defer the decision to apply or not a\n> control to DelayedControls on the pipeline handler side, which takes\n> care of evaluating if a control has to be updated or not and how many\n> frame it takes to take effect.\n>\n> Lenses (well, VCM to be fair) are a bit more complex than that, in the\n> sense that moving the lens might take a number of frames that depends\n> on the movement range. Some VCM datasheet I've seen provide a model to\n> estimate the delay depending on the movement range and the VCM\n> characteristics. The risk is that updating the lens position while the\n> lens hasn't reached its final position might trigger some resonation\n> effect, especially if the algorithm runs in \"continuous auto-focus\"\n> mode and tries to update the lens position too often.\n\nPlease don't limit thinking to VCMs.\n\nFrom my days of doing CCTV control systems I have a Computar 7.5-120mm\nmotorized zoom and focus lens with C-mount [1]. It has motors to drive\nthe optics, and potentiometers to read back the position. Add a couple\nof L298N motor driver chips and an ADC and I can control it fine,\nhowever significant movement takes several seconds.\nLikewise there are stepper motor controlled lenses such as [2]. You\nneed a reset sequence at power on (largely driving into an endstop),\nbut after that you have calibrated movement.\n\nI have MCU code to drive both of those via I2C, but V4L2 really needs\na control for reporting the current focus (and zoom) position, with\nthe existing controls taking the requested position. The IPA can then\nhold off if it knows the lens hasn't finished moving yet.\n\nJust food for thought.\n\n Dave\n\n[1] https://computar.com/product/680/H16Z7516PDC (at full zoom it\nshould be able to view things a couple of miles away with IMX477 or\nsimilar)\n[2] https://www.amazon.co.uk/dp/B09V53CMSK\n\n> I've cc-ed Matthias Fend which has recently sent a series for \"more\n> complex optics\"\n> https://patchwork.libcamera.org/project/libcamera/list/?series=3735\n> (which partially overlaps with the work you've done here) as he\n> certainly knows more than me about VCM and optics to know what his\n> opinion is\n>\n>\n> > + } af;\n> > +\n> > struct {\n> > struct {\n> > uint32_t exposure;\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 9e861fc0..297161b2 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > });\n> >\n> > + /* Lens position is unknown at the startup, so initilize the variable\n> ^ initialize\n> > + * that holds the current position to something out of the range. */\n>\n> Multiline comments as\n> /*\n> * first line\n> * next line\n> */\n>\n> > + context_.activeState.af.lensPosition = std::numeric_limits::max();\n> > +\n> > for (auto const &a : algorithms()) {\n> > Algorithm *algo = static_cast(a.get());\n> >\n> > @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)\n> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n> >\n> > setSensorControls.emit(frame, ctrls);\n> > +\n> > + if (lensControls_ && context_.activeState.af.applyLensCtrls) {\n> > + context_.activeState.af.applyLensCtrls = false;\n> > + ControlList lensCtrls(*lensControls_);\n> > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > + static_cast(context_.activeState.af.lensPosition));\n> > + setLensControls.emit(lensCtrls);\n> > + }\n> > }\n> >\n> > } /* namespace ipa::rkisp1 */\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 0559d261..b2fedc5f 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -113,6 +113,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> > @@ -337,6 +338,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> > @@ -400,6 +402,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> > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > +\n> > + focusLens->setFocusPosition(focusValue.get());\n> > +}\n> > +\n> > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > {\n> > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > --\n> > 2.39.0\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 E7140BDB1C\n\tfor ;\n\tMon, 6 Feb 2023 11:24:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52F64603B8;\n\tMon, 6 Feb 2023 12:24:08 +0100 (CET)","from mail-vs1-xe33.google.com (mail-vs1-xe33.google.com\n\t[IPv6:2607:f8b0:4864:20::e33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F028E603B8\n\tfor ;\n\tMon, 6 Feb 2023 12:24:06 +0100 (CET)","by mail-vs1-xe33.google.com with SMTP id s24so12255009vsi.12\n\tfor ;\n\tMon, 06 Feb 2023 03:24:06 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675682648;\n\tbh=UKDNcsZ50AttpOYdrulVoLFll1Ml7ohM2re0vjqjwPE=;\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=kd+UrGn0TacUjxqOrpRe53Fj9rXkYKjlHAXC13WRn7R0nyBywhxnqK0Pkvkqzpqmh\n\tYE8tlEV33aY7ywoOuoU0p0qhc8bc9U3T+I2xgZsyJxQT/VrJ3Pv+b3Pwv0AtfYk1TH\n\tPFQFpa0IG//B9HIWhoeDScXR4UhD5bvSXNHCtlLjP0zR4T0RlDPs80hOBkl00FaXo9\n\t4iK64J0/Zi+ramPljHRgrQ73UjTdFBz3wAeBqfUO7ICAoQxLMPbFVBYjYmMZArPL9X\n\t+SLDVcSQ8e+uK1UokEF7b9G2MSjBib+rQDjVdOl7srXbKoFz8mfDeZX59fgj4QzsOC\n\tgYvQRU+vT0erA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=IjXWQe6UTQVWrvfn3Dw8wn+vIXBamknwNWFDCriWKxA=;\n\tb=GXWMGlPwPN5q9HSbUML0SKQ1naJfHvfD1S5dWeXh5yC9FWx8Ntgltx3a6WMzBwFqnI\n\t+X4qbQWoLrIumi3RdPo5jVyV/c209pA5QqSEBow/OTVU29lAnziod20Bj/92Ts1N1i6E\n\tjUzH54uWi5UjPueBYHxjec2V4ipGQ+wyHI/Z4dZy+oak0FMllF7N5yy0u7HNYkW02DCF\n\t9svllXjaTF+9lA3Q6rsqX1kL4qoVkIPu87tQyHbMbEOCWrlWF+cmitoH+iQ7s06RK+ff\n\t2PNKw1wbf2B5Ul1697kfZJZEV5/kbCl+nOQt0BKKg8EZpCMEWkBHFNnvFF9ElLwkyDVz\n\tI02g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"GXWMGlPw\"; dkim-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:subject:date:message-id\n\t:reply-to;\n\tbh=IjXWQe6UTQVWrvfn3Dw8wn+vIXBamknwNWFDCriWKxA=;\n\tb=T52Ctq1UYs/AVKo1YvrelKwGjk/5HEJuJhfz9EDhtPUvLv1LBDCdPqZMcjeOC9/nl6\n\t4AJEqRTu7h8Xs5CkeR4ckIcpLsk+iRfUrnnhR17sHTi+UYAm68Vyxbq70ZJKO60yiM+O\n\tC56EUKWw1R8kEMbhSKnOkRCsbe/ojxjUMFTWVJk/74tXBuzSciZaidEIAJ7KQCaTkbZi\n\t+lePvDBvMMc17f6aVA6GZquUfcI8xfdTroPwCV5yrWOynSCv+uPI0WVyh7ePVwJmUekN\n\tan4iqk1m6qQY0wl2pPep7nRR/wIBCCLz1iHQAocysyU5bvhwpGBjmP/zEHIEcM8XZtvr\n\t4kAQ==","X-Gm-Message-State":"AO0yUKXVfNOGHGxTBysk1m7DCg/MMWY0pnNGZBHmXfVZcEbE5awg8u/Q\n\tQ2SLQUc6o5xyTrlByfWmKAQlrZ75GNKYVhLDgRNRpvthnnjEqmPR","X-Google-Smtp-Source":"AK7set89n2MgpLW5Sgjihd8mJrprAeljXSNVpWI3mDCP82jDEz+w4FQVeMPTjto31ZoxYcCDYQbYyUr98HrF0yX4ZNU=","X-Received":"by 2002:a67:ef41:0:b0:3f2:edef:718f with SMTP id\n\tk1-20020a67ef41000000b003f2edef718fmr3097722vsr.28.1675682645608;\n\tMon, 06 Feb 2023 03:24:05 -0800 (PST)","MIME-Version":"1.0","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-3-dse@thaumatec.com>\n\t<20230206094559.s44nxnphzckfvgaz@uno.localdomain>","In-Reply-To":"<20230206094559.s44nxnphzckfvgaz@uno.localdomain>","Date":"Mon, 6 Feb 2023 11:23:49 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26406,"web_url":"https://patchwork.libcamera.org/comment/26406/","msgid":"<20230206120942.nsz6dbgjs6kgxk5b@uno.localdomain>","date":"2023-02-06T12:09:42","subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dave,\n thanks for chiming in and sorry I've not cc-ed you, to me you're the\nRPi kernel guru and I never know if it's fine to rope you in in\nlibcamera stuff. Happy you're reading the ml and reply so promptly.\n\n\nOn Mon, Feb 06, 2023 at 11:23:49AM +0000, Dave Stevenson wrote:\n> Hi Jacopo and Daniel\n>\n> On Mon, 6 Feb 2023 at 09:46, Jacopo Mondi via libcamera-devel\n> wrote:\n> >\n> > Hi Daniel\n> >\n> > On Thu, Jan 19, 2023 at 09:41:06AM +0100, 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> >\n> > As a minor nit: I would move this to the end of the series, after\n> > having plumbed in the algorithm implementation... Just a nit though\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 bf6e9141..c3ed87aa 100644\n> > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n> > > interface IPARkISP1EventInterface {\n> > > paramsBufferReady(uint32 frame);\n> > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > + setLensControls(libcamera.ControlList lensControls);\n> > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > };\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index b9b20653..1fac6af9 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -53,6 +53,11 @@ struct IPASessionConfiguration {\n> > > };\n> > >\n> > > struct IPAActiveState {\n> > > + struct {\n> > > + uint32_t lensPosition;\n> > > + bool applyLensCtrls;\n> >\n> > Oh gosh lens are -complicated- /o\\\n> >\n> > For regular sensor controls we defer the decision to apply or not a\n> > control to DelayedControls on the pipeline handler side, which takes\n> > care of evaluating if a control has to be updated or not and how many\n> > frame it takes to take effect.\n> >\n> > Lenses (well, VCM to be fair) are a bit more complex than that, in the\n> > sense that moving the lens might take a number of frames that depends\n> > on the movement range. Some VCM datasheet I've seen provide a model to\n> > estimate the delay depending on the movement range and the VCM\n> > characteristics. The risk is that updating the lens position while the\n> > lens hasn't reached its final position might trigger some resonation\n> > effect, especially if the algorithm runs in \"continuous auto-focus\"\n> > mode and tries to update the lens position too often.\n>\n> Please don't limit thinking to VCMs.\n>\n> From my days of doing CCTV control systems I have a Computar 7.5-120mm\n> motorized zoom and focus lens with C-mount [1]. It has motors to drive\n> the optics, and potentiometers to read back the position. Add a couple\n> of L298N motor driver chips and an ADC and I can control it fine,\n> however significant movement takes several seconds.\n> Likewise there are stepper motor controlled lenses such as [2]. You\n> need a reset sequence at power on (largely driving into an endstop),\n> but after that you have calibrated movement.\n>\n> I have MCU code to drive both of those via I2C, but V4L2 really needs\n> a control for reporting the current focus (and zoom) position, with\n> the existing controls taking the requested position. The IPA can then\n> hold off if it knows the lens hasn't finished moving yet.\n\nJust to be clear:\n\n- \"Existing control\" being V4L2_CID_FOCUS_ABSOLUTE and\n V4L2_CID_FOCUS_RELATIVE which allows to set the lens position\n\n- And \"need a control\" would be something that allows reporting if the\n lens is still moving, it has reached the desired point etc ?\n\n\n>\n> Just food for thought.\n\nTrust me, I would have preferred a stricter diet when it comes to\nthoughts about lens and optics :)\n\n>\n> Dave\n>\n> [1] https://computar.com/product/680/H16Z7516PDC (at full zoom it\n> should be able to view things a couple of miles away with IMX477 or\n> similar)\n> [2] https://www.amazon.co.uk/dp/B09V53CMSK\n>\n> > I've cc-ed Matthias Fend which has recently sent a series for \"more\n> > complex optics\"\n> > https://patchwork.libcamera.org/project/libcamera/list/?series=3735\n> > (which partially overlaps with the work you've done here) as he\n> > certainly knows more than me about VCM and optics to know what his\n> > opinion is\n> >\n> >\n> > > + } af;\n> > > +\n> > > struct {\n> > > struct {\n> > > uint32_t exposure;\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 9e861fc0..297161b2 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > > });\n> > >\n> > > + /* Lens position is unknown at the startup, so initilize the variable\n> > ^ initialize\n> > > + * that holds the current position to something out of the range. */\n> >\n> > Multiline comments as\n> > /*\n> > * first line\n> > * next line\n> > */\n> >\n> > > + context_.activeState.af.lensPosition = std::numeric_limits::max();\n> > > +\n> > > for (auto const &a : algorithms()) {\n> > > Algorithm *algo = static_cast(a.get());\n> > >\n> > > @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n> > >\n> > > setSensorControls.emit(frame, ctrls);\n> > > +\n> > > + if (lensControls_ && context_.activeState.af.applyLensCtrls) {\n> > > + context_.activeState.af.applyLensCtrls = false;\n> > > + ControlList lensCtrls(*lensControls_);\n> > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > + static_cast(context_.activeState.af.lensPosition));\n> > > + setLensControls.emit(lensCtrls);\n> > > + }\n> > > }\n> > >\n> > > } /* namespace ipa::rkisp1 */\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 0559d261..b2fedc5f 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -113,6 +113,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> > > @@ -337,6 +338,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> > > @@ -400,6 +402,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> > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > +\n> > > + focusLens->setFocusPosition(focusValue.get());\n> > > +}\n> > > +\n> > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > {\n> > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > --\n> > > 2.39.0\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 2FAB1BDB1C\n\tfor ;\n\tMon, 6 Feb 2023 12:09:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C50B625F1;\n\tMon, 6 Feb 2023 13:09:47 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7F73603B8\n\tfor ;\n\tMon, 6 Feb 2023 13:09:46 +0100 (CET)","from ideasonboard.com (host-79-55-56-167.retail.telecomitalia.it\n\t[79.55.56.167])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D311D4DA;\n\tMon, 6 Feb 2023 13:09:45 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675685387;\n\tbh=otPCViMeyPrjPn1kABuUEXnpUpxbbkZ2KZi33Gqdthw=;\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=WR0xIwmtFaf8ZtWrDvC0nem0cWU/K7BEpkSnxmgiAkQXYAM8MpzsIrgCkGo/MXLUM\n\tnFOTgBPj/qtzUS1Q+Z5KoV0nle0udBJcGqMfkoS9niY6Apu8cdBNfg4uq9C/oPXjnX\n\t7/KX+55ROmL+n/YX7vCEufIc89Hbbfa3zZEcz7AKaWEHi+/k1n8hxvkam84MrUCEfL\n\tlegpYUEL22c/XiM1v8IcGSQ0/PiG52ZicX+4/lG2AYyFBfa1+859SBQoHQ8pk+W1lp\n\tRrgexPlyUnFZwsS8cVxkfn+zi4C+MopnSK+EkqUYJnJrlrW4+pVqpRDUDblDp2i7aU\n\tJDdwlv4b++hjQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675685386;\n\tbh=otPCViMeyPrjPn1kABuUEXnpUpxbbkZ2KZi33Gqdthw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bCO8hzcEHnNZ//BI/NhKT1+tLXZstpB/4cDx19lFtXyGap0hSAVPffrwdpJgQ/8QE\n\tDY2fTm0E16DoJmxrIW4JUT2iN465UJLy+QfKKF/t8u1+GEpiIJfyJscN+hhWmCjzGV\n\tiCMkphELeP8BX0cBgATFMCRdLVXfGxIoJYA/CwAk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bCO8hzcE\"; dkim-atps=neutral","Date":"Mon, 6 Feb 2023 13:09:42 +0100","To":"Dave Stevenson ","Message-ID":"<20230206120942.nsz6dbgjs6kgxk5b@uno.localdomain>","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-3-dse@thaumatec.com>\n\t<20230206094559.s44nxnphzckfvgaz@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 v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26408,"web_url":"https://patchwork.libcamera.org/comment/26408/","msgid":"","date":"2023-02-06T15:26:17","subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Mon, 6 Feb 2023 at 12:09, Jacopo Mondi wrote:\n>\n> Hi Dave,\n> thanks for chiming in and sorry I've not cc-ed you, to me you're the\n> RPi kernel guru and I never know if it's fine to rope you in in\n> libcamera stuff. Happy you're reading the ml and reply so promptly.\n\nI'm currently on other stuff, but am keeping an eye out on the mailing\nlists for interesting stuff.\nNaush & David will rope me in when needed.\n\n> On Mon, Feb 06, 2023 at 11:23:49AM +0000, Dave Stevenson wrote:\n> > Hi Jacopo and Daniel\n> >\n> > On Mon, 6 Feb 2023 at 09:46, Jacopo Mondi via libcamera-devel\n> > wrote:\n> > >\n> > > Hi Daniel\n> > >\n> > > On Thu, Jan 19, 2023 at 09:41:06AM +0100, 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> > >\n> > > As a minor nit: I would move this to the end of the series, after\n> > > having plumbed in the algorithm implementation... Just a nit though\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 bf6e9141..c3ed87aa 100644\n> > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n> > > > interface IPARkISP1EventInterface {\n> > > > paramsBufferReady(uint32 frame);\n> > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > > + setLensControls(libcamera.ControlList lensControls);\n> > > > metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > > };\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index b9b20653..1fac6af9 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -53,6 +53,11 @@ struct IPASessionConfiguration {\n> > > > };\n> > > >\n> > > > struct IPAActiveState {\n> > > > + struct {\n> > > > + uint32_t lensPosition;\n> > > > + bool applyLensCtrls;\n> > >\n> > > Oh gosh lens are -complicated- /o\\\n> > >\n> > > For regular sensor controls we defer the decision to apply or not a\n> > > control to DelayedControls on the pipeline handler side, which takes\n> > > care of evaluating if a control has to be updated or not and how many\n> > > frame it takes to take effect.\n> > >\n> > > Lenses (well, VCM to be fair) are a bit more complex than that, in the\n> > > sense that moving the lens might take a number of frames that depends\n> > > on the movement range. Some VCM datasheet I've seen provide a model to\n> > > estimate the delay depending on the movement range and the VCM\n> > > characteristics. The risk is that updating the lens position while the\n> > > lens hasn't reached its final position might trigger some resonation\n> > > effect, especially if the algorithm runs in \"continuous auto-focus\"\n> > > mode and tries to update the lens position too often.\n> >\n> > Please don't limit thinking to VCMs.\n> >\n> > From my days of doing CCTV control systems I have a Computar 7.5-120mm\n> > motorized zoom and focus lens with C-mount [1]. It has motors to drive\n> > the optics, and potentiometers to read back the position. Add a couple\n> > of L298N motor driver chips and an ADC and I can control it fine,\n> > however significant movement takes several seconds.\n> > Likewise there are stepper motor controlled lenses such as [2]. You\n> > need a reset sequence at power on (largely driving into an endstop),\n> > but after that you have calibrated movement.\n> >\n> > I have MCU code to drive both of those via I2C, but V4L2 really needs\n> > a control for reporting the current focus (and zoom) position, with\n> > the existing controls taking the requested position. The IPA can then\n> > hold off if it knows the lens hasn't finished moving yet.\n>\n> Just to be clear:\n>\n> - \"Existing control\" being V4L2_CID_FOCUS_ABSOLUTE and\n> V4L2_CID_FOCUS_RELATIVE which allows to set the lens position\n\nYes.\nV4L2_CID_FOCUS_RELATIVE is near useless in my book, but exists.\nV4L2_CID_FOCUS_ABSOLUTE as the existing control sets the (desired)\nfocus position.\nLikewise V4L2_CID_ZOOM_ABSOLUTE for desired zoom position\n(V4L2_CID_ZOOM_RELATIVE is also fairly useless, but also unused).\n\n> - And \"need a control\" would be something that allows reporting if the\n> lens is still moving, it has reached the desired point etc ?\n\n\"Still moving\" can be subjective if worrying about noise in ADC\nvalues, PID control loops, ringing, overshoots, etc, hence my\nsuggestion of current position and allowing userspace to make a call\nas to if it is good enough.\n\nIf you're reading back the position via an ADC or by counting stepper\npulses, then you know where you are.\nFor VCMs there often isn't an easy read back of position, but if you\nhave movement timing information in the datasheet then that can be\nreplicated in the kernel driver. Simplistic would be to report the old\nposition until sufficient time has passed for the new position to be\nvalid.\n\nAdding something like V4L2_CID_FOCUS_CURRENT_ABSOLUTE and\nV4L2_CID_ZOOM_CURRENT_ABSOLUTE would allow reporting back whether the\nlens has got to the requested position. Both would need to be volatile\nto ensure the driver got the chance to give the current position.\n\n> >\n> > Just food for thought.\n>\n> Trust me, I would have preferred a stricter diet when it comes to\n> thoughts about lens and optics :)\n\nIt is a minefield, but partly as it's just been ignored for so long :-(\n\nHmm, atomisp ov5693 appears to be the only implementation of\nV4L2_CID_FOCUS_RELATIVE, and that only maps on V4L2_CID_FOCUS_ABSOLUTE\nusing \"current + val\".\nIn looking that up I notice that they have defined their own\nV4L2_CID_VCM_SLEW and V4L2_CID_VCM_TIMING controls, and it looks like\nthey're defining a g_volatile_ctrl for V4L2_CID_FOCUS_ABSOLUTE which\nreturns the dead-reckoned position based on time since the position\nrequest was made.\nSo perhaps returning a different value via V4L2_CID_FOCUS_ABSOLUTE is\nacceptable then? It feels a little horrid to me, and I know never to\ntake atomisp behaviour as acceptable.\n\n Dave\n\n> >\n> > Dave\n> >\n> > [1] https://computar.com/product/680/H16Z7516PDC (at full zoom it\n> > should be able to view things a couple of miles away with IMX477 or\n> > similar)\n> > [2] https://www.amazon.co.uk/dp/B09V53CMSK\n> >\n> > > I've cc-ed Matthias Fend which has recently sent a series for \"more\n> > > complex optics\"\n> > > https://patchwork.libcamera.org/project/libcamera/list/?series=3735\n> > > (which partially overlaps with the work you've done here) as he\n> > > certainly knows more than me about VCM and optics to know what his\n> > > opinion is\n> > >\n> > >\n> > > > + } af;\n> > > > +\n> > > > struct {\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 9e861fc0..297161b2 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > > > });\n> > > >\n> > > > + /* Lens position is unknown at the startup, so initilize the variable\n> > > ^ initialize\n> > > > + * that holds the current position to something out of the range. */\n> > >\n> > > Multiline comments as\n> > > /*\n> > > * first line\n> > > * next line\n> > > */\n> > >\n> > > > + context_.activeState.af.lensPosition = std::numeric_limits::max();\n> > > > +\n> > > > for (auto const &a : algorithms()) {\n> > > > Algorithm *algo = static_cast(a.get());\n> > > >\n> > > > @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n> > > >\n> > > > setSensorControls.emit(frame, ctrls);\n> > > > +\n> > > > + if (lensControls_ && context_.activeState.af.applyLensCtrls) {\n> > > > + context_.activeState.af.applyLensCtrls = false;\n> > > > + ControlList lensCtrls(*lensControls_);\n> > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > > + static_cast(context_.activeState.af.lensPosition));\n> > > > + setLensControls.emit(lensCtrls);\n> > > > + }\n> > > > }\n> > > >\n> > > > } /* namespace ipa::rkisp1 */\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 0559d261..b2fedc5f 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -113,6 +113,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> > > > @@ -337,6 +338,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> > > > @@ -400,6 +402,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> > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > +\n> > > > + focusLens->setFocusPosition(focusValue.get());\n> > > > +}\n> > > > +\n> > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > > {\n> > > > RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > > --\n> > > > 2.39.0\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 81178BEFBE\n\tfor ;\n\tMon, 6 Feb 2023 15:26:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA1CD625F1;\n\tMon, 6 Feb 2023 16:26:35 +0100 (CET)","from mail-vs1-xe2c.google.com (mail-vs1-xe2c.google.com\n\t[IPv6:2607:f8b0:4864:20::e2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6EC25603B8\n\tfor ;\n\tMon, 6 Feb 2023 16:26:34 +0100 (CET)","by mail-vs1-xe2c.google.com with SMTP id cz15so3387109vsb.6\n\tfor ;\n\tMon, 06 Feb 2023 07:26:34 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675697195;\n\tbh=DJT5oOfe62BQYUbdAOCsqaYrzDxfWIR5jpux91pX8oU=;\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=naOOkOPITTmuF0o+fctSBE0nH5bTSzUBF5Oz3T0VTpeSkWn4xVTDTwNhvEStJFDw6\n\tHY7JiZ0Y4jFcj/losuGdBu0cC+E3duKHUVPma3XRhIOJu4qHTEzt136P263Wf6+JAp\n\t8+veXij4+gHBxvwYBYQSzKqy99beuliGbNQBqs/nlrenkKJBEIWRi/5kLzJm0G9imd\n\tZXim643H0yrdRhQdY4v9fFUETCWf6Zpq6yIBNTD498ed2XbxCljNBVyDWgFjO2LbuU\n\t0ou8OuAvQ2p6bsJ0QOEvnQD8hyOVWgeLtg0+JBjZep5iAvn/36RUZiEPyL6fWJc2qg\n\tGId67wKrVNQPA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=a7zfM5DO3Wuj6VOfM1qNiO0ZPYSl5AE2/aqglcprrMc=;\n\tb=gPDp7TO1G1iP4GEpGTZTkM0VFZGKvS9qwjTs6M/4bXL2tKCY19tWiRh2e8W4C+mvM7\n\tdAUzZxkVQrbJijBJvS2lhDxqsFepdyAoa+DVVx34Nz5XQFbswoOVG2eFn6QwtpmupO79\n\tSwr42nTvPbbLKQMsGSLyMBJA+pHYJW11uRjYJZOax0a6nX7wnTYsfMWnRWhJdOluh12P\n\tu6gTpRviOjwF6NTPLy+nRdJR5I1TYy1cEeV9p90bP8d6Ull4iyEtFRkzLGxSp9O5oAZt\n\tdkymzGfPy4H+fDlqM/VJylzSEed8ZTFvVbPACcJy3ntYICuc1sjFGFnqEY0Bp5GstpdW\n\txR/w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"gPDp7TO1\"; dkim-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:subject:date:message-id\n\t:reply-to;\n\tbh=a7zfM5DO3Wuj6VOfM1qNiO0ZPYSl5AE2/aqglcprrMc=;\n\tb=0p0A0uX/sxFWffy3xtHi0u57nqm5VExVkeJVmQrjGxM7KhroIZx/DtuoazyUrGnf3A\n\tKIRiSb8Lr679HipSTaCsU3MpKVzokq5fGG83MBJiH1+R2JzhEz42+i6Xz16qshXr17S3\n\tbT1KN4expiVD75yISZsztWq7urRDy3Mya3O+4ngVYabPFlszYyyBDkj0kCAQEg6J1iol\n\t9AXHrGkXpLMxD1Fnd9FsjZAQLGEv4ppFssdrC1cV+ZIgPdnrBvVVc8aQGYJh/niHmWTK\n\tty6f5+30YZ940LpNk9y2K1i7FrHNiL9PzGWx5FNwmooTgV2no9DMQlhoXjiIz5FeRl/w\n\tlzYQ==","X-Gm-Message-State":"AO0yUKXCfuIuA8vDQRF+I6IOxZ8SEsOG00Ihz0AwJvii2UJlILPql6Fy\n\tTKuy75xC0w9OxjzXuRcqeSQduAJ5xPCQo8EBXQLVdQ==","X-Google-Smtp-Source":"AK7set/k052MUhc/kQi0Yieg+wJ8kC0SQ12luWzw/4dwQ6b1xndXa0SCypL+6yNAkSQcUGZHmrUMnYw9+UVGXsbRQOQ=","X-Received":"by 2002:a67:f10a:0:b0:3f2:edef:718f with SMTP id\n\tn10-20020a67f10a000000b003f2edef718fmr19131vsk.28.1675697193065;\n\tMon, 06 Feb 2023 07:26:33 -0800 (PST)","MIME-Version":"1.0","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-3-dse@thaumatec.com>\n\t<20230206094559.s44nxnphzckfvgaz@uno.localdomain>\n\t\n\t<20230206120942.nsz6dbgjs6kgxk5b@uno.localdomain>","In-Reply-To":"<20230206120942.nsz6dbgjs6kgxk5b@uno.localdomain>","Date":"Mon, 6 Feb 2023 15:26:17 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26415,"web_url":"https://patchwork.libcamera.org/comment/26415/","msgid":"<6abcd073-b8a8-ae9e-9a04-e35c05a0d46e@emfend.at>","date":"2023-02-07T08:20:29","subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":134,"url":"https://patchwork.libcamera.org/api/people/134/","name":"Matthias Fend","email":"matthias.fend@emfend.at"},"content":"Hi Daniel, hi Jacopo,\n\nAm 06.02.2023 um 10:45 schrieb Jacopo Mondi:\n> Hi Daniel\n> \n> On Thu, Jan 19, 2023 at 09:41:06AM +0100, 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> \n> As a minor nit: I would move this to the end of the series, after\n> having plumbed in the algorithm implementation... Just a nit though\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 bf6e9141..c3ed87aa 100644\n>> --- a/include/libcamera/ipa/rkisp1.mojom\n>> +++ b/include/libcamera/ipa/rkisp1.mojom\n>> @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n>> interface IPARkISP1EventInterface {\n>> \tparamsBufferReady(uint32 frame);\n>> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n>> +\tsetLensControls(libcamera.ControlList lensControls);\n>> \tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n>> };\n>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n>> index b9b20653..1fac6af9 100644\n>> --- a/src/ipa/rkisp1/ipa_context.h\n>> +++ b/src/ipa/rkisp1/ipa_context.h\n>> @@ -53,6 +53,11 @@ struct IPASessionConfiguration {\n>> };\n>>\n>> struct IPAActiveState {\n>> +\tstruct {\n>> +\t\tuint32_t lensPosition;\n>> +\t\tbool applyLensCtrls;\n> \n> Oh gosh lens are -complicated- /o\\\n> \n> For regular sensor controls we defer the decision to apply or not a\n> control to DelayedControls on the pipeline handler side, which takes\n> care of evaluating if a control has to be updated or not and how many\n> frame it takes to take effect.\n> \n> Lenses (well, VCM to be fair) are a bit more complex than that, in the\n> sense that moving the lens might take a number of frames that depends\n> on the movement range. Some VCM datasheet I've seen provide a model to\n> estimate the delay depending on the movement range and the VCM\n> characteristics. The risk is that updating the lens position while the\n> lens hasn't reached its final position might trigger some resonation\n> effect, especially if the algorithm runs in \"continuous auto-focus\"\n> mode and tries to update the lens position too often.\n> \n> I've cc-ed Matthias Fend which has recently sent a series for \"more\n> complex optics\"\n> https://patchwork.libcamera.org/project/libcamera/list/?series=3735\n> (which partially overlaps with the work you've done here) as he\n> certainly knows more than me about VCM and optics to know what his\n> opinion is\n\nFirst, thank you Daniel, for your work.\n\nThe only overlap is the patch in which I basically add the lens \ncontroller to the pipeline as is done in other pipelines.\nThis patch is there to better show what changes with the conversion.\n\nAssuming that the calculated or desired position of the lens is always \nsomehow valid, I would say for simplicity that it is OK to just always \nupdate the control. If the position of the lens does not change, the \ncontrol is filtered and not passed to the driver. From there, you might \nconsider optimizing applyLensCtrls away.\n\nIn my idea, the interface between IPA and pipeline consists of actual \nv4l2 controls for optics. The optic helper classes would go into the IPA \nor the IPA library. This would mean that an autofocus algorithm can \ndirectly use the optics to read the current position e.g. \noptics.getCurrentFocusPosition() or also set the new position e.g. \noptics.setFocusPosition(). I know that this is a bigger change, but I \ndidn't know how else to fulfill all my requirements.\n\nSo how and where you want to integrate and handle the optics in the \nfuture has a significant impact on changes like this.\nOf course, this can be changed later, but maybe it would make sense to \nthink about how this should look in the end right now.\n\nRegardless, I'm very happy to see that something is moving forward here \nregarding optic support.\n\nThanks\n ~Matthias\n\n> \n> \n>> +\t} af;\n>> +\n>> \tstruct {\n>> \t\tstruct {\n>> \t\t\tuint32_t exposure;\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 9e861fc0..297161b2 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n>> \t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>> \t\t});\n>>\n>> +\t/* Lens position is unknown at the startup, so initilize the variable\n> ^ initialize\n>> +\t * that holds the current position to something out of the range. */\n> \n> Multiline comments as\n> /*\n> * first line\n> * next line\n> */\n> \n>> +\tcontext_.activeState.af.lensPosition = std::numeric_limits::max();\n>> +\n>> \tfor (auto const &a : algorithms()) {\n>> \t\tAlgorithm *algo = static_cast(a.get());\n>>\n>> @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)\n>> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n>>\n>> \tsetSensorControls.emit(frame, ctrls);\n>> +\n>> +\tif (lensControls_ && context_.activeState.af.applyLensCtrls) {\n>> +\t\tcontext_.activeState.af.applyLensCtrls = false;\n>> +\t\tControlList lensCtrls(*lensControls_);\n>> +\t\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n>> +\t\t\t static_cast(context_.activeState.af.lensPosition));\n>> +\t\tsetLensControls.emit(lensCtrls);\n>> +\t}\n>> }\n>>\n>> } /* namespace ipa::rkisp1 */\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index 0559d261..b2fedc5f 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -113,6 +113,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>> @@ -337,6 +338,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>> @@ -400,6 +402,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.39.0\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 565FBBDB1C\n\tfor ;\n\tTue, 7 Feb 2023 08:20:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C78F4625DF;\n\tTue, 7 Feb 2023 09:20:35 +0100 (CET)","from lx20.hoststar.hosting (lx20.hoststar.hosting [168.119.41.54])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94775603BD\n\tfor ;\n\tTue, 7 Feb 2023 09:20:33 +0100 (CET)","from 194-208-208-245.tele.net ([194.208.208.245]:51969\n\thelo=[192.168.0.218])\n\tby lx20.hoststar.hosting with esmtpsa (TLS1.3) tls\n\tTLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (Exim 4.93)\n\t(envelope-from )\n\tid 1pPJDK-0068Mx-LK; Tue, 07 Feb 2023 09:20:31 +0100"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675758035;\n\tbh=izQ7qcsvpWF6TsAs4iqDyPLzGoFDJ2BgaOA8sLxhlVQ=;\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=N56avKKSv/xTnHZKhRbt64DXds5dj0Fp+35G2axTCs09iYScu0TxcIxCPe0N+GI21\n\tQGKHWj00nT0v/9BwEgoLlRbvnB/hTaKUoxz35jAc5ADjJAWF2c9LCpSwcKh1JxKkV1\n\tScr7glyYaYAQhHuMRGWlqe2wgOukQqdZfD01cbrA+V9IwnSYLgRWMeUk4+5nLkM5OU\n\tMcuzoAlHIVqXZGtmS4e2xGIXWzPEntfIKwRBy2abP1jCHKn9Oz4AjAdeiDT27hnGZQ\n\tPoFkLaw9uGTQZYaCYAikkGhcspZd8x3pW/QQRH2d5vgTjsQkrF4uOfWeAWvxVPEuL0\n\t3quyyHJ3S2OZQ==","v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=emfend.at;\n\ts=mail;\n\th=Content-Transfer-Encoding:Content-Type:In-Reply-To:From:References\n\t:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=h5mvBIVGA4W3b+ImwBk0NfGnyDhWvP+E4H9KR1mlHV8=;\n\tb=pCp80poMzwhlKv/uSOJDVGSfDE\n\tNXkbghb2ten79IHpFvvvTd+ND7dTfai+gV7X95LFSxnq1zeOy+0CheueQPoSakPNRbbP+1ZfeDiXK\n\t0M88HS5Qgg4z6Rhtu1FGOBUh3NloOXzI9mf3vx3UPCfKeFYRmmx/K0R1D7YeysZiw+EI=;"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=emfend.at header.i=@emfend.at\n\theader.b=\"pCp80poM\"; dkim-atps=neutral","Message-ID":"<6abcd073-b8a8-ae9e-9a04-e35c05a0d46e@emfend.at>","Date":"Tue, 7 Feb 2023 09:20:29 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64;\n\trv:102.0) Gecko/20100101 Thunderbird/102.6.1","Content-Language":"de-DE","To":"Jacopo Mondi ,\n\tDaniel Semkowicz ","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-3-dse@thaumatec.com>\n\t<20230206094559.s44nxnphzckfvgaz@uno.localdomain>","In-Reply-To":"<20230206094559.s44nxnphzckfvgaz@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-Spam-Score":"","X-Spam-Bar":"","X-Spam-Report":"","Subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Matthias Fend via libcamera-devel ","Reply-To":"Matthias Fend ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26416,"web_url":"https://patchwork.libcamera.org/comment/26416/","msgid":"","date":"2023-02-07T08:20:34","subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":134,"url":"https://patchwork.libcamera.org/api/people/134/","name":"Matthias Fend","email":"matthias.fend@emfend.at"},"content":"Hi Dave, hi Jacopo,\n\nAm 06.02.2023 um 16:26 schrieb Dave Stevenson:\n> Hi Jacopo\n> \n> On Mon, 6 Feb 2023 at 12:09, Jacopo Mondi wrote:\n>>\n>> Hi Dave,\n>> thanks for chiming in and sorry I've not cc-ed you, to me you're the\n>> RPi kernel guru and I never know if it's fine to rope you in in\n>> libcamera stuff. Happy you're reading the ml and reply so promptly.\n> \n> I'm currently on other stuff, but am keeping an eye out on the mailing\n> lists for interesting stuff.\n> Naush & David will rope me in when needed.\n> \n>> On Mon, Feb 06, 2023 at 11:23:49AM +0000, Dave Stevenson wrote:\n>>> Hi Jacopo and Daniel\n>>>\n>>> On Mon, 6 Feb 2023 at 09:46, Jacopo Mondi via libcamera-devel\n>>> wrote:\n>>>>\n>>>> Hi Daniel\n>>>>\n>>>> On Thu, Jan 19, 2023 at 09:41:06AM +0100, 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>>>>\n>>>> As a minor nit: I would move this to the end of the series, after\n>>>> having plumbed in the algorithm implementation... Just a nit though\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 bf6e9141..c3ed87aa 100644\n>>>>> --- a/include/libcamera/ipa/rkisp1.mojom\n>>>>> +++ b/include/libcamera/ipa/rkisp1.mojom\n>>>>> @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n>>>>> interface IPARkISP1EventInterface {\n>>>>> paramsBufferReady(uint32 frame);\n>>>>> setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n>>>>> + setLensControls(libcamera.ControlList lensControls);\n>>>>> metadataReady(uint32 frame, libcamera.ControlList metadata);\n>>>>> };\n>>>>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n>>>>> index b9b20653..1fac6af9 100644\n>>>>> --- a/src/ipa/rkisp1/ipa_context.h\n>>>>> +++ b/src/ipa/rkisp1/ipa_context.h\n>>>>> @@ -53,6 +53,11 @@ struct IPASessionConfiguration {\n>>>>> };\n>>>>>\n>>>>> struct IPAActiveState {\n>>>>> + struct {\n>>>>> + uint32_t lensPosition;\n>>>>> + bool applyLensCtrls;\n>>>>\n>>>> Oh gosh lens are -complicated- /o\\\n>>>>\n>>>> For regular sensor controls we defer the decision to apply or not a\n>>>> control to DelayedControls on the pipeline handler side, which takes\n>>>> care of evaluating if a control has to be updated or not and how many\n>>>> frame it takes to take effect.\n>>>>\n>>>> Lenses (well, VCM to be fair) are a bit more complex than that, in the\n>>>> sense that moving the lens might take a number of frames that depends\n>>>> on the movement range. Some VCM datasheet I've seen provide a model to\n>>>> estimate the delay depending on the movement range and the VCM\n>>>> characteristics. The risk is that updating the lens position while the\n>>>> lens hasn't reached its final position might trigger some resonation\n>>>> effect, especially if the algorithm runs in \"continuous auto-focus\"\n>>>> mode and tries to update the lens position too often.\n>>>\n>>> Please don't limit thinking to VCMs.\n>>>\n>>> From my days of doing CCTV control systems I have a Computar 7.5-120mm\n>>> motorized zoom and focus lens with C-mount [1]. It has motors to drive\n>>> the optics, and potentiometers to read back the position. Add a couple\n>>> of L298N motor driver chips and an ADC and I can control it fine,\n>>> however significant movement takes several seconds.\n>>> Likewise there are stepper motor controlled lenses such as [2]. You\n>>> need a reset sequence at power on (largely driving into an endstop),\n>>> but after that you have calibrated movement.\n>>>\n>>> I have MCU code to drive both of those via I2C, but V4L2 really needs\n>>> a control for reporting the current focus (and zoom) position, with\n>>> the existing controls taking the requested position. The IPA can then\n>>> hold off if it knows the lens hasn't finished moving yet.\n>>\n>> Just to be clear:\n>>\n>> - \"Existing control\" being V4L2_CID_FOCUS_ABSOLUTE and\n>> V4L2_CID_FOCUS_RELATIVE which allows to set the lens position\n\nThanks for bringing this up. I'm glad to hear that there are more people \nout there who are dealing with some more elaborate optics ;)\n\n> \n> Yes.\n> V4L2_CID_FOCUS_RELATIVE is near useless in my book, but exists.\n> V4L2_CID_FOCUS_ABSOLUTE as the existing control sets the (desired)\n> focus position.\n> Likewise V4L2_CID_ZOOM_ABSOLUTE for desired zoom position\n> (V4L2_CID_ZOOM_RELATIVE is also fairly useless, but also unused).\n> \n>> - And \"need a control\" would be something that allows reporting if the\n>> lens is still moving, it has reached the desired point etc ?\n> \n> \"Still moving\" can be subjective if worrying about noise in ADC\n> values, PID control loops, ringing, overshoots, etc, hence my\n> suggestion of current position and allowing userspace to make a call\n> as to if it is good enough.\n\nI think that even both information can be helpful here.\n\nThe current position is basically very important and probably already \ncovers most cases. For example, you can instruct the MCU that moves the \noptics to move the whole focus area from one end to the other (which can \ntake a few seconds) and observe the peak value of the AF statistics. If \nyou also have the current position of the focus lens, you know pretty \nwell where the perfect position will be.\n\nWhether the lens group is still moving can be helpful in cases where the \ndesired position cannot be reached (anymore). This can be the case, for \nexample, if the originally desired focus position may no longer be \napproached in order to avoid a collision with other lens groups. There \nare optics where the movement ranges of the lens groups overlap.\n\nI also see a need for additional enhancements like a possibility to move \nto absolute position with a certain speed, to move single lens groups \nfor calibration, to search the mechanical limits of the optics, etc.\n\n> \n> If you're reading back the position via an ADC or by counting stepper\n> pulses, then you know where you are.\n> For VCMs there often isn't an easy read back of position, but if you\n> have movement timing information in the datasheet then that can be\n> replicated in the kernel driver. Simplistic would be to report the old\n> position until sufficient time has passed for the new position to be\n> valid.\n> \n> Adding something like V4L2_CID_FOCUS_CURRENT_ABSOLUTE and\n> V4L2_CID_ZOOM_CURRENT_ABSOLUTE would allow reporting back whether the\n> lens has got to the requested position. Both would need to be volatile\n> to ensure the driver got the chance to give the current position.\n\nIn my proposal patch series I tried to illustrate a way to transport \nsuch volatile controls from the driver to the algorithm.\n\n> \n>>>\n>>> Just food for thought.\n>>\n>> Trust me, I would have preferred a stricter diet when it comes to\n>> thoughts about lens and optics :)\n> \n> It is a minefield, but partly as it's just been ignored for so long :-(\n> \n> Hmm, atomisp ov5693 appears to be the only implementation of\n> V4L2_CID_FOCUS_RELATIVE, and that only maps on V4L2_CID_FOCUS_ABSOLUTE\n> using \"current + val\".\n> In looking that up I notice that they have defined their own\n> V4L2_CID_VCM_SLEW and V4L2_CID_VCM_TIMING controls, and it looks like\n> they're defining a g_volatile_ctrl for V4L2_CID_FOCUS_ABSOLUTE which\n> returns the dead-reckoned position based on time since the position\n> request was made.\n> So perhaps returning a different value via V4L2_CID_FOCUS_ABSOLUTE is\n> acceptable then? It feels a little horrid to me, and I know never to\n> take atomisp behaviour as acceptable.\n\nI don't like the idea of modeling the mechanical behavior in the driver.\nI would find it better if the driver only returns things where it can \nmake a meaningful contribution. This would mean in this case that the \ndriver only returns the control for the current position if it has a \ncorresponding measured value.\nIf you want to estimate the actual position for a VCM via an \napproximation, then I would see this in libcamera.\nThe driver usually only maps the control chip for the VCM. But for the \nestimation of the position other facts like the mounted optics and the \norientation in space can be relevant. These are things libcamera can \nknow about, but not the driver. Libcamera could then implement an optics \nhelper that provides such an estimation for all VCM based optics.\n\n~Matthias\n\n> \n> Dave\n> \n>>>\n>>> Dave\n>>>\n>>> [1] https://computar.com/product/680/H16Z7516PDC (at full zoom it\n>>> should be able to view things a couple of miles away with IMX477 or\n>>> similar)\n>>> [2] https://www.amazon.co.uk/dp/B09V53CMSK\n>>>\n>>>> I've cc-ed Matthias Fend which has recently sent a series for \"more\n>>>> complex optics\"\n>>>> https://patchwork.libcamera.org/project/libcamera/list/?series=3735\n>>>> (which partially overlaps with the work you've done here) as he\n>>>> certainly knows more than me about VCM and optics to know what his\n>>>> opinion is\n>>>>\n>>>>\n>>>>> + } af;\n>>>>> +\n>>>>> struct {\n>>>>> struct {\n>>>>> uint32_t exposure;\n>>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>>>> index 9e861fc0..297161b2 100644\n>>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>>>> @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n>>>>> return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>>>>> });\n>>>>>\n>>>>> + /* Lens position is unknown at the startup, so initilize the variable\n>>>> ^ initialize\n>>>>> + * that holds the current position to something out of the range. */\n>>>>\n>>>> Multiline comments as\n>>>> /*\n>>>> * first line\n>>>> * next line\n>>>> */\n>>>>\n>>>>> + context_.activeState.af.lensPosition = std::numeric_limits::max();\n>>>>> +\n>>>>> for (auto const &a : algorithms()) {\n>>>>> Algorithm *algo = static_cast(a.get());\n>>>>>\n>>>>> @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)\n>>>>> ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n>>>>>\n>>>>> setSensorControls.emit(frame, ctrls);\n>>>>> +\n>>>>> + if (lensControls_ && context_.activeState.af.applyLensCtrls) {\n>>>>> + context_.activeState.af.applyLensCtrls = false;\n>>>>> + ControlList lensCtrls(*lensControls_);\n>>>>> + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n>>>>> + static_cast(context_.activeState.af.lensPosition));\n>>>>> + setLensControls.emit(lensCtrls);\n>>>>> + }\n>>>>> }\n>>>>>\n>>>>> } /* namespace ipa::rkisp1 */\n>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>> index 0559d261..b2fedc5f 100644\n>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>> @@ -113,6 +113,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>>>>> @@ -337,6 +338,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>>>>> @@ -400,6 +402,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>>>>> + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>>>>> +\n>>>>> + focusLens->setFocusPosition(focusValue.get());\n>>>>> +}\n>>>>> +\n>>>>> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n>>>>> {\n>>>>> RkISP1FrameInfo *info = frameInfo_.find(frame);\n>>>>> --\n>>>>> 2.39.0\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 9B425BDB1C\n\tfor ;\n\tTue, 7 Feb 2023 08:20:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B9A2625F6;\n\tTue, 7 Feb 2023 09:20:39 +0100 (CET)","from lx20.hoststar.hosting (lx20.hoststar.hosting [168.119.41.54])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C2C1603BD\n\tfor ;\n\tTue, 7 Feb 2023 09:20:38 +0100 (CET)","from 194-208-208-245.tele.net ([194.208.208.245]:51970\n\thelo=[192.168.0.218])\n\tby lx20.hoststar.hosting with esmtpsa (TLS1.3) tls\n\tTLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (Exim 4.93)\n\t(envelope-from )\n\tid 1pPJDP-0068Ol-F4; Tue, 07 Feb 2023 09:20:36 +0100"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675758039;\n\tbh=pVDD0L3zNnHeDM/ug3XocYggz6l6Tgl0ZwvT4atuA/0=;\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=3c20Bbd93mVEmBamV2RCZjseQxBa6jXNYWXrprIRDU/C1LnKG+K7AB9LswbM03QtP\n\tOvDMC12HgnS4lYVREfw4ForkfoIuPZ/gb+MrI8db3N8WdjYZiPSwQwPR8IC8BOS7yw\n\tuc6RcQirbpz/YNlsk4MmFFWjl0c1iFxjHB9R4t/n9omNUQ1Z0ouDIv5sXENCS5DNja\n\teh/osjuBjVY4IHqh6JoshdofRrfjCaHrznJD3xIZa0rlNgyjcRX0Dg0MnS8FwNNikM\n\tyuODW8nEMw85E5aIq67i74oXnP/mWV60BbcBHXkSRXnyAcuSVo1Qhlq+WN3V2PknNW\n\t1d3wP73OWwL/g==","v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=emfend.at;\n\ts=mail;\n\th=Content-Transfer-Encoding:Content-Type:In-Reply-To:From:References\n\t:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=773EzJ0IsvTkUb3c64cpIyP+aGcaNZFffeCEnFrJ0Ws=;\n\tb=dvHVY3hmVsoCtLJLcJ5uZst6lX\n\tm/zn0ifLbDA9dKDM2vC/hcH7tuh3pI/MPzskkV3piymhPYb6PXtEGcDZCS0etqlxmw5KkFsoDDwbl\n\t02Zzy3VRvsXmXsBkLh62UDzwqV5uMHECMY+07DTXdMlTOPqO94UVByBpmG19DEJ1TqAM=;"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=emfend.at header.i=@emfend.at\n\theader.b=\"dvHVY3hm\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 7 Feb 2023 09:20:34 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64;\n\trv:102.0) Gecko/20100101 Thunderbird/102.6.1","Content-Language":"de-DE","To":"Dave Stevenson ,\n\tJacopo Mondi ","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-3-dse@thaumatec.com>\n\t<20230206094559.s44nxnphzckfvgaz@uno.localdomain>\n\t\n\t<20230206120942.nsz6dbgjs6kgxk5b@uno.localdomain>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-Spam-Score":"","X-Spam-Bar":"","X-Spam-Report":"","Subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Matthias Fend via libcamera-devel ","Reply-To":"Matthias Fend ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26603,"web_url":"https://patchwork.libcamera.org/comment/26603/","msgid":"","date":"2023-03-09T09:53:14","subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Dave, hi Matthias, hi Jacopo,\n\nOn Tue, Feb 7, 2023 at 9:20 AM Matthias Fend wrote:\n>\n> Hi Dave, hi Jacopo,\n>\n> Am 06.02.2023 um 16:26 schrieb Dave Stevenson:\n> > Hi Jacopo\n> >\n> > On Mon, 6 Feb 2023 at 12:09, Jacopo Mondi wrote:\n> >>\n> >> Hi Dave,\n> >> thanks for chiming in and sorry I've not cc-ed you, to me you're the\n> >> RPi kernel guru and I never know if it's fine to rope you in in\n> >> libcamera stuff. Happy you're reading the ml and reply so promptly.\n> >\n> > I'm currently on other stuff, but am keeping an eye out on the mailing\n> > lists for interesting stuff.\n> > Naush & David will rope me in when needed.\n> >\n> >> On Mon, Feb 06, 2023 at 11:23:49AM +0000, Dave Stevenson wrote:\n> >>> Hi Jacopo and Daniel\n> >>>\n> >>> On Mon, 6 Feb 2023 at 09:46, Jacopo Mondi via libcamera-devel\n> >>> wrote:\n> >>>>\n> >>>> Hi Daniel\n> >>>>\n> >>>> On Thu, Jan 19, 2023 at 09:41:06AM +0100, 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> >>>>\n> >>>> As a minor nit: I would move this to the end of the series, after\n> >>>> having plumbed in the algorithm implementation... Just a nit though\n\nSure, I will reorder the changes.\n\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 bf6e9141..c3ed87aa 100644\n> >>>>> --- a/include/libcamera/ipa/rkisp1.mojom\n> >>>>> +++ b/include/libcamera/ipa/rkisp1.mojom\n> >>>>> @@ -39,5 +39,6 @@ interface IPARkISP1Interface {\n> >>>>> interface IPARkISP1EventInterface {\n> >>>>> paramsBufferReady(uint32 frame);\n> >>>>> setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> >>>>> + setLensControls(libcamera.ControlList lensControls);\n> >>>>> metadataReady(uint32 frame, libcamera.ControlList metadata);\n> >>>>> };\n> >>>>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> >>>>> index b9b20653..1fac6af9 100644\n> >>>>> --- a/src/ipa/rkisp1/ipa_context.h\n> >>>>> +++ b/src/ipa/rkisp1/ipa_context.h\n> >>>>> @@ -53,6 +53,11 @@ struct IPASessionConfiguration {\n> >>>>> };\n> >>>>>\n> >>>>> struct IPAActiveState {\n> >>>>> + struct {\n> >>>>> + uint32_t lensPosition;\n> >>>>> + bool applyLensCtrls;\n> >>>>\n> >>>> Oh gosh lens are -complicated- /o\\\n> >>>>\n> >>>> For regular sensor controls we defer the decision to apply or not a\n> >>>> control to DelayedControls on the pipeline handler side, which takes\n> >>>> care of evaluating if a control has to be updated or not and how many\n> >>>> frame it takes to take effect.\n> >>>>\n> >>>> Lenses (well, VCM to be fair) are a bit more complex than that, in the\n> >>>> sense that moving the lens might take a number of frames that depends\n> >>>> on the movement range. Some VCM datasheet I've seen provide a model to\n> >>>> estimate the delay depending on the movement range and the VCM\n> >>>> characteristics. The risk is that updating the lens position while the\n> >>>> lens hasn't reached its final position might trigger some resonation\n> >>>> effect, especially if the algorithm runs in \"continuous auto-focus\"\n> >>>> mode and tries to update the lens position too often.\n> >>>\n> >>> Please don't limit thinking to VCMs.\n> >>>\n> >>> From my days of doing CCTV control systems I have a Computar 7.5-120mm\n> >>> motorized zoom and focus lens with C-mount [1]. It has motors to drive\n> >>> the optics, and potentiometers to read back the position. Add a couple\n> >>> of L298N motor driver chips and an ADC and I can control it fine,\n> >>> however significant movement takes several seconds.\n> >>> Likewise there are stepper motor controlled lenses such as [2]. You\n> >>> need a reset sequence at power on (largely driving into an endstop),\n> >>> but after that you have calibrated movement.\n> >>>\n> >>> I have MCU code to drive both of those via I2C, but V4L2 really needs\n> >>> a control for reporting the current focus (and zoom) position, with\n> >>> the existing controls taking the requested position. The IPA can then\n> >>> hold off if it knows the lens hasn't finished moving yet.\n> >>\n> >> Just to be clear:\n> >>\n> >> - \"Existing control\" being V4L2_CID_FOCUS_ABSOLUTE and\n> >> V4L2_CID_FOCUS_RELATIVE which allows to set the lens position\n>\n> Thanks for bringing this up. I'm glad to hear that there are more people\n> out there who are dealing with some more elaborate optics ;)\n>\n> >\n> > Yes.\n> > V4L2_CID_FOCUS_RELATIVE is near useless in my book, but exists.\n> > V4L2_CID_FOCUS_ABSOLUTE as the existing control sets the (desired)\n> > focus position.\n> > Likewise V4L2_CID_ZOOM_ABSOLUTE for desired zoom position\n> > (V4L2_CID_ZOOM_RELATIVE is also fairly useless, but also unused).\n> >\n> >> - And \"need a control\" would be something that allows reporting if the\n> >> lens is still moving, it has reached the desired point etc ?\n> >\n> > \"Still moving\" can be subjective if worrying about noise in ADC\n> > values, PID control loops, ringing, overshoots, etc, hence my\n> > suggestion of current position and allowing userspace to make a call\n> > as to if it is good enough.\n>\n> I think that even both information can be helpful here.\n>\n> The current position is basically very important and probably already\n> covers most cases. For example, you can instruct the MCU that moves the\n> optics to move the whole focus area from one end to the other (which can\n> take a few seconds) and observe the peak value of the AF statistics. If\n> you also have the current position of the focus lens, you know pretty\n> well where the perfect position will be.\n>\n> Whether the lens group is still moving can be helpful in cases where the\n> desired position cannot be reached (anymore). This can be the case, for\n> example, if the originally desired focus position may no longer be\n> approached in order to avoid a collision with other lens groups. There\n> are optics where the movement ranges of the lens groups overlap.\n>\n> I also see a need for additional enhancements like a possibility to move\n> to absolute position with a certain speed, to move single lens groups\n> for calibration, to search the mechanical limits of the optics, etc.\n>\n> >\n> > If you're reading back the position via an ADC or by counting stepper\n> > pulses, then you know where you are.\n> > For VCMs there often isn't an easy read back of position, but if you\n> > have movement timing information in the datasheet then that can be\n> > replicated in the kernel driver. Simplistic would be to report the old\n> > position until sufficient time has passed for the new position to be\n> > valid.\n> >\n> > Adding something like V4L2_CID_FOCUS_CURRENT_ABSOLUTE and\n> > V4L2_CID_ZOOM_CURRENT_ABSOLUTE would allow reporting back whether the\n> > lens has got to the requested position. Both would need to be volatile\n> > to ensure the driver got the chance to give the current position.\n>\n> In my proposal patch series I tried to illustrate a way to transport\n> such volatile controls from the driver to the algorithm.\n\nThank you all for the broad presentation of the optics world! Now I see\nthat the number of cases to be covered is a lot more than I thought...\n\n>\n> >\n> >>>\n> >>> Just food for thought.\n> >>\n> >> Trust me, I would have preferred a stricter diet when it comes to\n> >> thoughts about lens and optics :)\n> >\n> > It is a minefield, but partly as it's just been ignored for so long :-(\n> >\n> > Hmm, atomisp ov5693 appears to be the only implementation of\n> > V4L2_CID_FOCUS_RELATIVE, and that only maps on V4L2_CID_FOCUS_ABSOLUTE\n> > using \"current + val\".\n> > In looking that up I notice that they have defined their own\n> > V4L2_CID_VCM_SLEW and V4L2_CID_VCM_TIMING controls, and it looks like\n> > they're defining a g_volatile_ctrl for V4L2_CID_FOCUS_ABSOLUTE which\n> > returns the dead-reckoned position based on time since the position\n> > request was made.\n> > So perhaps returning a different value via V4L2_CID_FOCUS_ABSOLUTE is\n> > acceptable then? It feels a little horrid to me, and I know never to\n> > take atomisp behaviour as acceptable.\n>\n> I don't like the idea of modeling the mechanical behavior in the driver.\n> I would find it better if the driver only returns things where it can\n> make a meaningful contribution. This would mean in this case that the\n> driver only returns the control for the current position if it has a\n> corresponding measured value.\n> If you want to estimate the actual position for a VCM via an\n> approximation, then I would see this in libcamera.\n> The driver usually only maps the control chip for the VCM. But for the\n> estimation of the position other facts like the mounted optics and the\n> orientation in space can be relevant. These are things libcamera can\n> know about, but not the driver. Libcamera could then implement an optics\n> helper that provides such an estimation for all VCM based optics.\n\nI agree on that. I would also see the complex modelling\nin the libcamera and leave the drivers as simple hardware interface.\n\n>\n> ~Matthias\n>\n> >\n> > Dave\n> >\n> >>>\n> >>> Dave\n> >>>\n> >>> [1] https://computar.com/product/680/H16Z7516PDC (at full zoom it\n> >>> should be able to view things a couple of miles away with IMX477 or\n> >>> similar)\n> >>> [2] https://www.amazon.co.uk/dp/B09V53CMSK\n> >>>\n> >>>> I've cc-ed Matthias Fend which has recently sent a series for \"more\n> >>>> complex optics\"\n> >>>> https://patchwork.libcamera.org/project/libcamera/list/?series=3735\n> >>>> (which partially overlaps with the work you've done here) as he\n> >>>> certainly knows more than me about VCM and optics to know what his\n> >>>> opinion is\n> >>>>\n> >>>>\n> >>>>> + } af;\n> >>>>> +\n> >>>>> struct {\n> >>>>> struct {\n> >>>>> uint32_t exposure;\n> >>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >>>>> index 9e861fc0..297161b2 100644\n> >>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >>>>> @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> >>>>> return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> >>>>> });\n> >>>>>\n> >>>>> + /* Lens position is unknown at the startup, so initilize the variable\n> >>>> ^ initialize\n> >>>>> + * that holds the current position to something out of the range. */\n> >>>>\n> >>>> Multiline comments as\n> >>>> /*\n> >>>> * first line\n> >>>> * next line\n> >>>> */\n> >>>>\n> >>>>> + context_.activeState.af.lensPosition = std::numeric_limits::max();\n> >>>>> +\n> >>>>> for (auto const &a : algorithms()) {\n> >>>>> Algorithm *algo = static_cast(a.get());\n> >>>>>\n> >>>>> @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)\n> >>>>> ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain));\n> >>>>>\n> >>>>> setSensorControls.emit(frame, ctrls);\n> >>>>> +\n> >>>>> + if (lensControls_ && context_.activeState.af.applyLensCtrls) {\n> >>>>> + context_.activeState.af.applyLensCtrls = false;\n> >>>>> + ControlList lensCtrls(*lensControls_);\n> >>>>> + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> >>>>> + static_cast(context_.activeState.af.lensPosition));\n> >>>>> + setLensControls.emit(lensCtrls);\n> >>>>> + }\n> >>>>> }\n> >>>>>\n> >>>>> } /* namespace ipa::rkisp1 */\n> >>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>>> index 0559d261..b2fedc5f 100644\n> >>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>>> @@ -113,6 +113,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> >>>>> @@ -337,6 +338,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> >>>>> @@ -400,6 +402,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> >>>>> + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> >>>>> +\n> >>>>> + focusLens->setFocusPosition(focusValue.get());\n> >>>>> +}\n> >>>>> +\n> >>>>> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> >>>>> {\n> >>>>> RkISP1FrameInfo *info = frameInfo_.find(frame);\n> >>>>> --\n> >>>>> 2.39.0\n> >>>>>\n\nIt looks the thread was split into two mails, so I will copy\nthe related fragments below to have everything in one place.\n\n> Assuming that the calculated or desired position of the lens is always\n> somehow valid, I would say for simplicity that it is OK to just always\n> update the control. If the position of the lens does not change, the\n> control is filtered and not passed to the driver. From there, you might\n> consider optimizing applyLensCtrls away.\n>\n\nI wanted to avoid unnecessary calls with the lens position on each\nframe and it is easier to decide directly in the algorithm if We need\nto update the value. Additionally, there is a fixed wait time in Af\nalgorithm for lens movement, because currently there is no information\nif lens movement has finished. I agree this is something to optimize,\nbut I think it would be easier to do when We introduce 'current focus'\nor/and 'still moving' information.\n\n> In my idea, the interface between IPA and pipeline consists of actual\n> v4l2 controls for optics. The optic helper classes would go into the IPA\n> or the IPA library. This would mean that an autofocus algorithm can\n> directly use the optics to read the current position e.g.\n> optics.getCurrentFocusPosition() or also set the new position e.g.\n> optics.setFocusPosition(). I know that this is a bigger change, but I\n> didn't know how else to fulfill all my requirements.\n>\n> So how and where you want to integrate and handle the optics in the\n> future has a significant impact on changes like this.\n> Of course, this can be changed later, but maybe it would make sense to\n> think about how this should look in the end right now.\n\nI have some concerns about the idea of algorithms directly controlling\nthe optics. Shouldn't we guarantee as much as possible the time\nsynchronisation of data per frame? If the lens position is read in\nthe AF algorithm, there will be some time period between the point when\nframe statistics were captured and the point where the position was\nread. The AF algo doesn't know how many other algorithms were executed\nbefore it and how much time passed during execution. Of course this is\nstill not the real time system, but at least the time difference should\nbe minimized.\n\nWhat I would propose is similliar to how the sensor controls\nare currently managed in pipeline and IPA:\n1) Pipeline: read the current values for optics (position and state)\n on signal that statistics for frame are ready\n2) IPA: Add 'lens' field to the IPAFrameContext struct and fill it with\n the values read in the pipeline\n3) AF algorithm can then use the information from the IPAFrameContext\n\nCameraLens can be extended with methods:\n- setFocusPosition(position)\n- focusPosition()\n- setFocusSpeed()\n- focusSpeed()\n- setZoom(zoom)\n- zoom()\n...\n\nAnd underneath it can use the specific device implementation, for example\nestimate the position or read the real values.\n\nMy main idea is to expose only the top level interface to the IPA\nAnd control hardware specific things in the pipeline handler,\nCameraLens.\nProbably I am not aware of more complex problems you are facing and\nmaybe this will not be the best solution to you, but should cover most\nof the cases.\n\nBest regards\nDaniel\n\n>\n> Regardless, I'm very happy to see that something is moving forward here\n> regarding optic support.\n>\n> Thanks\n> ~Matthias","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 D2287BE080\n\tfor ;\n\tThu, 9 Mar 2023 09:53:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B439626CF;\n\tThu, 9 Mar 2023 10:53:28 +0100 (CET)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE53862664\n\tfor ;\n\tThu, 9 Mar 2023 10:53:25 +0100 (CET)","by mail-ed1-x531.google.com with SMTP id ay14so4555540edb.11\n\tfor ;\n\tThu, 09 Mar 2023 01:53:25 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678355608;\n\tbh=cAAO0eiWCKhy9QLTIIQEMotoKc+IV5Iny2sJe6JCEwA=;\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=hLlL9kE0FyZSrBV9+veJwgndcus8kUSjNUwNeHlEctYanztZfG9Xf7uG3MoEydtJV\n\t1GWhPtJ1L+i/7+vtJU5LOwLGa17nPTNVBHaIWcByVPer8X235WY/z1/QH4rirxB8Te\n\tVD9x9/FrA9IBnIDCQcPubhI7+qVxfS7RD9rih5uAXNnPWhhpkKAcDaSS3AbndHFFEG\n\tIjmMcBPnhLm56JaTrlL1/1Su2Q+hPuuhnkljbvmBAGqoJLTmjlNYgkjYowX5F4pFqE\n\t+v64ALtcKAu+v+1kH2zrep1AhOhkO9j42FQCirRyzfP8RaWlvHawpJuK03wBLcu6gA\n\tiDf5mSCWrz0Tg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1678355605;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=tPjKRVgEMAYCrhxwTOsQp/y63dLeJ4dCYX7b9QA7KC0=;\n\tb=2mqKR7H+7N3fg40tSmPe4svHExnfVGj86AVQYXObSsQaCJoqQe3U8DoYOA7tONOGT1\n\tXyE96IBujNuVWVp2UiECQxaSX3RrlVDh87Fi0Oyz+L1tDeF/IPgVvVjQoOEBTy425nBE\n\tg0yG0FZIajE0wUE0nCN4TJbK/ghnxTpN0SiN3DNEsnDv5/1qY462C2a17VZ+iV6yoH2i\n\tQGMkENKC2q8SQ/9Ln+CraMglpCGn2JubP4UcAr9qPW7Zg2AZ/XfYwxGi+D41BVMw7nFc\n\trOLyH9nx+pgXC905KnDjY/tgkQQcJIvATfWbMVKC87tEl+PyUz+u1z8b8Po2egMPpjXg\n\tThfQ=="],"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=\"2mqKR7H+\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678355605;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=tPjKRVgEMAYCrhxwTOsQp/y63dLeJ4dCYX7b9QA7KC0=;\n\tb=O67FFBoTBG9Cho1iNNuf87RyulhHdQjotdz/Pzo0rwexg2NftjDRwjlQUHy1JKKj9W\n\twacfUcSGn7Z2N+4kNhCoAClEl+2BkmOv9/Ww8W6sXCqk+1G2TiWdzM27MS3KP1aqZDD3\n\t0LT9ZmQImD1YPOpQRzugEs4m9BbIXOIO0McfbmX6EEK4dymKecdYft2Fbwn1VUy0hq+R\n\tnNuLRNd/Ll/v6TvcUBuwXYSLabvt2mN+1UAaMldA1gJODy9DbquX8gt1bNGR2Ns9Njjv\n\t7XD064P90kIToW8RJs6gMTwOVqUH5ZP3gBPkofTOtWUzrTdTNtxNKSN2H46sQ79+9/Ab\n\t2uXQ==","X-Gm-Message-State":"AO0yUKU5p6J9foeHYjw7jDJZu8/nzM7fbKESWAmLG880TxaE2AHAwbde\n\tMsJ3hGnqe5BAIuGWqhiXdU6esvC7qMIbn68XhdQ63A==","X-Google-Smtp-Source":"AK7set9DMvg9IJNyPkUdjphMml85OAwQFJ6gd4+9M+vuWo8pp3TD6g7Do19eGO8Ub9FMQa/DB8qIR/Q62h5I1ISAXDw=","X-Received":"by 2002:a17:906:398a:b0:8d1:9162:514a with SMTP id\n\th10-20020a170906398a00b008d19162514amr10940155eje.8.1678355605139;\n\tThu, 09 Mar 2023 01:53:25 -0800 (PST)","MIME-Version":"1.0","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-3-dse@thaumatec.com>\n\t<20230206094559.s44nxnphzckfvgaz@uno.localdomain>\n\t\n\t<20230206120942.nsz6dbgjs6kgxk5b@uno.localdomain>\n\t\n\t","In-Reply-To":"","Date":"Thu, 9 Mar 2023 10:53:14 +0100","Message-ID":"","To":"Matthias Fend ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens\n\tposition from IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]