[{"id":22288,"web_url":"https://patchwork.libcamera.org/comment/22288/","msgid":"<164742585788.11309.4188991275024246751@Monstersaurus>","date":"2022-03-16T10:17:37","subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Control the\n\tlens position","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-16 09:39:51)\n> Now that the ancillary links are configured, we can use the CameraLens\n> class and control the VCM through the IPA.\n> For now, force a default value for the lens position, until the AF\n> algorithm is introduced.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 38 +++++++++++++++++++++++++++--\n> 1 file changed, 36 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index fd8fecb0..0a0b5a3a 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -108,6 +108,7 @@ private:\n> void setMode(const IPACameraSensorInfo &sensorInfo);\n> bool validateSensorControls();\n> bool validateIspControls();\n> + bool validateLensControls();\n> void queueRequest(const ControlList &controls);\n> void returnEmbeddedBuffer(unsigned int bufferId);\n> void prepareISP(const ipa::RPi::ISPConfig &data);\n> @@ -132,6 +133,7 @@ private:\n> \n> ControlInfoMap sensorCtrls_;\n> ControlInfoMap ispCtrls_;\n> + ControlInfoMap lensCtrls_;\n> ControlList libcameraMetadata_;\n> \n> /* Camera sensor params. */\n> @@ -342,13 +344,14 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> const ipa::RPi::IPAConfig &ipaConfig,\n> ControlList *controls)\n> {\n> - if (entityControls.size() != 2) {\n> - LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> + if (entityControls.size() != 3) {\n\nOk - so that explains the '2' in the previous patch.\n\nFeels a bit arbitrary though ... I wonder if it needs to be a map to\nmake it more explicit to which control list is which...\n\n> + LOG(IPARPI, Error) << \"No ISP, lens or sensor controls found.\";\n\nAgain, what happens on a device with no lens ...\n\n\n> return -1;\n> }\n> \n> sensorCtrls_ = entityControls.at(0);\n> ispCtrls_ = entityControls.at(1);\n> + lensCtrls_ = entityControls.at(2);\n> \n> if (!validateSensorControls()) {\n> LOG(IPARPI, Error) << \"Sensor control validation failed.\";\n> @@ -360,6 +363,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> return -1;\n> }\n> \n> + if (!validateLensControls()) {\n\nWhat happens here if there is no lens ...\n\n\n> + LOG(IPARPI, Error) << \"Lens control validation failed.\";\n> + return -1;\n> + }\n> +\n> maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n> \n> /* Setup a metadata ControlList to output metadata. */\n> @@ -578,6 +586,23 @@ bool IPARPi::validateIspControls()\n> return true;\n> }\n> \n> +bool IPARPi::validateLensControls()\n> +{\n> + static const uint32_t ctrls[] = {\n> + V4L2_CID_FOCUS_ABSOLUTE,\n> + };\n> +\n> + for (auto c : ctrls) {\n> + if (lensCtrls_.find(c) == lensCtrls_.end()) {\n> + LOG(IPARPI, Error) << \"Unable to find lens control \"\n> + << utils::hex(c);\n> + return false;\n> + }\n> + }\n> +\n> + return true;\n> +}\n> +\n> /*\n> * Converting between enums (used in the libcamera API) and the names that\n> * we use to identify different modes. Unfortunately, the conversion tables\n> @@ -1066,6 +1091,15 @@ void IPARPi::processStats(unsigned int bufferId)\n> \n> setDelayedControls.emit(ctrls);\n> }\n> +\n> + /*\n> + * Set the focus position\n> + * \\todo Use an AF algorithm and replace the arbitrary value\n> + */\n> + ControlList lensCtrls(lensCtrls_);\n> + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast(123));\n> + setLensControls.emit(lensCtrls);\n> +\n\nno need for extra blank line here.\n\nBut I guess this is just a 'sample' for the moment anyway.\n\nIt really makes me think changing the IPA API to use a libcamera control\nlist would be better too - so that the plumbing between is cleaner. But\nthat's a whole chunk of rework...\n\n\n> }\n> \n> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> -- \n> 2.32.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 965E5BF415\n\tfor ;\n\tWed, 16 Mar 2022 10:17:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01845604E7;\n\tWed, 16 Mar 2022 11:17:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B93E601F5\n\tfor ;\n\tWed, 16 Mar 2022 11:17:40 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E5DA0A67;\n\tWed, 16 Mar 2022 11:17:39 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647425862;\n\tbh=hGHq+r11VFECo8spdLmf/VCYPKGDhGCqG4ya2FUiRTQ=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=h7LiiwTy7gnqgqjX5Taf+FWdXzjqNoCQ0DTB2Tm8xKpzv1/9A0kEw6PuOWul/qDfn\n\tTgZr9QqLfIHEz0lVYYp3QmROjhlKX+7vLseVOeN3mny7oKXCHZn6ifnkk9PWmG/ft0\n\t3kd8Ph3Qvfi308WrueIUnPaiXWEpzDmruJwg38qRh5fMcOoTCr76Z/D825z2VAJ309\n\tSWdXGPJV3fS630V/saimaqTxvD/B8jz149Ori1H8qHSCKnYGVpmYlrHQW/52S2155t\n\tJ8Wcc2/JT8153B/XT2JlxWIilRO4vcQTzdomLqgqo3TScSgKapDqcbExGiunByqugN\n\tkI6PxpoJPh8OQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647425860;\n\tbh=hGHq+r11VFECo8spdLmf/VCYPKGDhGCqG4ya2FUiRTQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=K4jalkXwuioXCeBuhl5Yb2fLpngZFboYjHqnQcKqKF1kwNHkabQLEOsbtGZQaHtF2\n\tQajHGHVv18jBQE0aNfzgJeOCVvolNesyLow8Fe7WWFp6Ozc6dFCiOGyWgI6g6/MDb0\n\tDkI6eciZaf2euzgqqd7kR1fLtV1b0XgbpF73QJFY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K4jalkXw\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220316093951.33779-2-jeanmichel.hautbois@ideasonboard.com>","References":"<20220316093951.33779-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20220316093951.33779-2-jeanmichel.hautbois@ideasonboard.com>","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 16 Mar 2022 10:17:37 +0000","Message-ID":"<164742585788.11309.4188991275024246751@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Control the\n\tlens position","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22307,"web_url":"https://patchwork.libcamera.org/comment/22307/","msgid":"","date":"2022-03-17T10:22:42","subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Control the\n\tlens position","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Jean-Michel,\n\nThank you for your work.\n\nOn Wed, 16 Mar 2022 at 10:17, Kieran Bingham via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-16 09:39:51)\n> > Now that the ancillary links are configured, we can use the CameraLens\n> > class and control the VCM through the IPA.\n> > For now, force a default value for the lens position, until the AF\n> > algorithm is introduced.\n> >\n> > Signed-off-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 38 +++++++++++++++++++++++++++--\n> > 1 file changed, 36 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index fd8fecb0..0a0b5a3a 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -108,6 +108,7 @@ private:\n> > void setMode(const IPACameraSensorInfo &sensorInfo);\n> > bool validateSensorControls();\n> > bool validateIspControls();\n> > + bool validateLensControls();\n> > void queueRequest(const ControlList &controls);\n> > void returnEmbeddedBuffer(unsigned int bufferId);\n> > void prepareISP(const ipa::RPi::ISPConfig &data);\n> > @@ -132,6 +133,7 @@ private:\n> >\n> > ControlInfoMap sensorCtrls_;\n> > ControlInfoMap ispCtrls_;\n> > + ControlInfoMap lensCtrls_;\n> > ControlList libcameraMetadata_;\n> >\n> > /* Camera sensor params. */\n> > @@ -342,13 +344,14 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > const ipa::RPi::IPAConfig &ipaConfig,\n> > ControlList *controls)\n> > {\n> > - if (entityControls.size() != 2) {\n> > - LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > + if (entityControls.size() != 3) {\n>\n> Ok - so that explains the '2' in the previous patch.\n>\n> Feels a bit arbitrary though ... I wonder if it needs to be a map to\n> make it more explicit to which control list is which...\n>\n\nThe entityControls() are vestiges from the days before the mojom IPA\ninterface, and this was the\nonly mechanism to pass controls across. I'm all for changing this to a map\nor something more sensible\nnow that we have the flexibility.\n\nApart from that, my only real comment for this patch would be, similar to\nwhat Kieran has pointed out,\nwe need to guard against instances where there is no lens driver.\n\nRegards,\nNaush\n\n\n>\n> > + LOG(IPARPI, Error) << \"No ISP, lens or sensor controls\n> found.\";\n>\n> Again, what happens on a device with no lens ...\n>\n>\n> > return -1;\n> > }\n> >\n> > sensorCtrls_ = entityControls.at(0);\n> > ispCtrls_ = entityControls.at(1);\n> > + lensCtrls_ = entityControls.at(2);\n> >\n> > if (!validateSensorControls()) {\n> > LOG(IPARPI, Error) << \"Sensor control validation\n> failed.\";\n> > @@ -360,6 +363,11 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > return -1;\n> > }\n> >\n> > + if (!validateLensControls()) {\n>\n> What happens here if there is no lens ...\n>\n>\n> > + LOG(IPARPI, Error) << \"Lens control validation failed.\";\n> > + return -1;\n> > + }\n> > +\n> > maxSensorGainCode_ =\n> sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n> >\n> > /* Setup a metadata ControlList to output metadata. */\n> > @@ -578,6 +586,23 @@ bool IPARPi::validateIspControls()\n> > return true;\n> > }\n> >\n> > +bool IPARPi::validateLensControls()\n> > +{\n> > + static const uint32_t ctrls[] = {\n> > + V4L2_CID_FOCUS_ABSOLUTE,\n> > + };\n> > +\n> > + for (auto c : ctrls) {\n> > + if (lensCtrls_.find(c) == lensCtrls_.end()) {\n> > + LOG(IPARPI, Error) << \"Unable to find lens\n> control \"\n> > + << utils::hex(c);\n> > + return false;\n> > + }\n> > + }\n> > +\n> > + return true;\n> > +}\n> > +\n> > /*\n> > * Converting between enums (used in the libcamera API) and the names\n> that\n> > * we use to identify different modes. Unfortunately, the conversion\n> tables\n> > @@ -1066,6 +1091,15 @@ void IPARPi::processStats(unsigned int bufferId)\n> >\n> > setDelayedControls.emit(ctrls);\n> > }\n> > +\n> > + /*\n> > + * Set the focus position\n> > + * \\todo Use an AF algorithm and replace the arbitrary value\n> > + */\n> > + ControlList lensCtrls(lensCtrls_);\n> > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> static_cast(123));\n> > + setLensControls.emit(lensCtrls);\n> > +\n>\n> no need for extra blank line here.\n>\n> But I guess this is just a 'sample' for the moment anyway.\n>\n> It really makes me think changing the IPA API to use a libcamera control\n> list would be better too - so that the plumbing between is cleaner. But\n> that's a whole chunk of rework...\n>\n>\n> > }\n> >\n> > void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls)\n> > --\n> > 2.32.0\n> >\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 9DAEEBDE17\n\tfor ;\n\tThu, 17 Mar 2022 10:23:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB591604E6;\n\tThu, 17 Mar 2022 11:22:59 +0100 (CET)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDE72604DB\n\tfor ;\n\tThu, 17 Mar 2022 11:22:58 +0100 (CET)","by mail-lf1-x12c.google.com with SMTP id b28so8204416lfc.4\n\tfor ;\n\tThu, 17 Mar 2022 03:22:58 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647512579;\n\tbh=8G54kPRqz76+auUYiqLm+Sm/UbESrtlGAcgjv2IIbQ0=;\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=I7eY5Hlru62kvcBoXC6vERKveHkzQ+IhJlPvwfXL853vCtGJUgUxdcigUHRAT7Xek\n\t0qWaOYi44u5N/KvHqtMYXsCmwEZ+sTCM+3f0jMzScqAjiDyAiZuCy0E00RLznzIrra\n\tX+Bl/T/m/4oXUtAJMX6gkgCHRJuZkVtnGq8ConQeEOqz6vfgPOHUV+AokxHp5Tc/cc\n\tpzJsGRj/yk9tVt4dktf3p3gjxcDTML/8IevtuxtBwXyW6kNKzQ6uKqI8KDIP2ehaoL\n\tX9Nu+wKCnf9clNhuli9ttZjRgxqxmXhrhN30Qx9SKTcFjhiFX54GeyGWEURnTXVFga\n\t4/3CMtfhhmS/Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=bLpJKh0/ha1TLfFHmG3fEZTOaJVP8xBAs0HVWPwp+Do=;\n\tb=ZxaQU/ht9viQkvGox0AIGMF495UbHNNIse18dX1vXQZ5+wzTzUlmySmVg4QGeBQPmk\n\tXsqm8m+RumqatxPiBoi11P7enKwy9hpd27t9Eu5NfnBFyxgycQeXeBN/jIBuwq3vdqc0\n\tlRG0xtBAFhEcetxR0QdqvKvwPyQmvQR8XisaQawxTL7R8LpOpotST75wrCspG5S1YJP6\n\tffnN6uX96lpK8D691/9cUEL4z1v5zpCs2ar9WqIUWE3fnv7VvOBD8cbPONFJaUuu31t4\n\tXnweSpyRSQrkMPxYeKu5mhRld7Er2MpW1oxFNUlwEgI4sv0rcmWATt10jZXyi7As7pQk\n\tHcKA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ZxaQU/ht\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=bLpJKh0/ha1TLfFHmG3fEZTOaJVP8xBAs0HVWPwp+Do=;\n\tb=lKZMVyQ7G94wHKGRbGgkHvG6NqIo1yU4+AF6PyApBsgaqeYjlXpeQKu8i7QrykGpH4\n\t5SgY388Cu6KAXZGIkuSneSIk6zhrBxd73hgOTBttoTB0lUfyRvdcKO4NpaMnqOdT5Q1h\n\tw8bUvhTkUxeefT3YGb93bHbx/LamMoLJQ9HMdZqWzUJkPnQr9sF9tRqk4yt0YAhEcrm3\n\texIyqJA30d88mLalwgMKoR7LETo0ozROgekqeNjKaMzYqJ66TSFGcm6W5Y/3wKTd82Y1\n\tnRqlzspoeknLIMrOQnluzNFt0up8LkhxMQPSZbf9j2hSxbdJGhnW3aO4AcoFVAqRuzBG\n\tpqCQ==","X-Gm-Message-State":"AOAM532TsAmDjgw2JA49++zMf+njdFewbXvoahW1XnXMoejyX6o7pkU7\n\tBnn49tzjrE5peTvWf2l7fC08FliTWB4EHTmpdhrXhoVFF7Y=","X-Google-Smtp-Source":"ABdhPJw3fSHmu1ZJVuMXCbea2TtdJFt+9Oi3wp3bV2XagL28ThwKydoy6b2eA7zS/SSrMaSiY9jXqVy5K2+4XxjODvA=","X-Received":"by 2002:a05:6512:12c8:b0:448:7c3f:60f8 with SMTP id\n\tp8-20020a05651212c800b004487c3f60f8mr2546236lfg.79.1647512578040;\n\tThu, 17 Mar 2022 03:22:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20220316093951.33779-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20220316093951.33779-2-jeanmichel.hautbois@ideasonboard.com>\n\t<164742585788.11309.4188991275024246751@Monstersaurus>","In-Reply-To":"<164742585788.11309.4188991275024246751@Monstersaurus>","Date":"Thu, 17 Mar 2022 10:22:42 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000fa3dec05da67691c\"","Subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Control the\n\tlens position","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22310,"web_url":"https://patchwork.libcamera.org/comment/22310/","msgid":"","date":"2022-03-17T11:16:21","subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Control the\n\tlens position","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Jean-Michel\n\nThanks for starting this work!\n\nOn Thu, 17 Mar 2022 at 10:23, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Hi Jean-Michel,\n>\n> Thank you for your work.\n>\n> On Wed, 16 Mar 2022 at 10:17, Kieran Bingham via libcamera-devel wrote:\n>>\n>> Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-16 09:39:51)\n>> > Now that the ancillary links are configured, we can use the CameraLens\n>> > class and control the VCM through the IPA.\n>> > For now, force a default value for the lens position, until the AF\n>> > algorithm is introduced.\n>> >\n>> > Signed-off-by: Jean-Michel Hautbois \n>> > ---\n>> > src/ipa/raspberrypi/raspberrypi.cpp | 38 +++++++++++++++++++++++++++--\n>> > 1 file changed, 36 insertions(+), 2 deletions(-)\n>> >\n>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > index fd8fecb0..0a0b5a3a 100644\n>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > @@ -108,6 +108,7 @@ private:\n>> > void setMode(const IPACameraSensorInfo &sensorInfo);\n>> > bool validateSensorControls();\n>> > bool validateIspControls();\n>> > + bool validateLensControls();\n>> > void queueRequest(const ControlList &controls);\n>> > void returnEmbeddedBuffer(unsigned int bufferId);\n>> > void prepareISP(const ipa::RPi::ISPConfig &data);\n>> > @@ -132,6 +133,7 @@ private:\n>> >\n>> > ControlInfoMap sensorCtrls_;\n>> > ControlInfoMap ispCtrls_;\n>> > + ControlInfoMap lensCtrls_;\n>> > ControlList libcameraMetadata_;\n>> >\n>> > /* Camera sensor params. */\n>> > @@ -342,13 +344,14 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>> > const ipa::RPi::IPAConfig &ipaConfig,\n>> > ControlList *controls)\n>> > {\n>> > - if (entityControls.size() != 2) {\n>> > - LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n>> > + if (entityControls.size() != 3) {\n>>\n>> Ok - so that explains the '2' in the previous patch.\n>>\n>> Feels a bit arbitrary though ... I wonder if it needs to be a map to\n>> make it more explicit to which control list is which...\n>\n>\n> The entityControls() are vestiges from the days before the mojom IPA interface, and this was the\n> only mechanism to pass controls across. I'm all for changing this to a map or something more sensible\n> now that we have the flexibility.\n>\n> Apart from that, my only real comment for this patch would be, similar to what Kieran has pointed out,\n> we need to guard against instances where there is no lens driver.\n>\n> Regards,\n> Naush\n>\n>>\n>>\n>> > + LOG(IPARPI, Error) << \"No ISP, lens or sensor controls found.\";\n>>\n>> Again, what happens on a device with no lens ...\n>>\n>>\n>> > return -1;\n>> > }\n>> >\n>> > sensorCtrls_ = entityControls.at(0);\n>> > ispCtrls_ = entityControls.at(1);\n>> > + lensCtrls_ = entityControls.at(2);\n>> >\n>> > if (!validateSensorControls()) {\n>> > LOG(IPARPI, Error) << \"Sensor control validation failed.\";\n>> > @@ -360,6 +363,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>> > return -1;\n>> > }\n>> >\n>> > + if (!validateLensControls()) {\n>>\n>> What happens here if there is no lens ...\n>>\n>>\n>> > + LOG(IPARPI, Error) << \"Lens control validation failed.\";\n>> > + return -1;\n>> > + }\n>> > +\n>> > maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get();\n>> >\n>> > /* Setup a metadata ControlList to output metadata. */\n>> > @@ -578,6 +586,23 @@ bool IPARPi::validateIspControls()\n>> > return true;\n>> > }\n>> >\n>> > +bool IPARPi::validateLensControls()\n>> > +{\n>> > + static const uint32_t ctrls[] = {\n>> > + V4L2_CID_FOCUS_ABSOLUTE,\n>> > + };\n>> > +\n>> > + for (auto c : ctrls) {\n>> > + if (lensCtrls_.find(c) == lensCtrls_.end()) {\n>> > + LOG(IPARPI, Error) << \"Unable to find lens control \"\n>> > + << utils::hex(c);\n>> > + return false;\n>> > + }\n>> > + }\n>> > +\n>> > + return true;\n>> > +}\n>> > +\n>> > /*\n>> > * Converting between enums (used in the libcamera API) and the names that\n>> > * we use to identify different modes. Unfortunately, the conversion tables\n>> > @@ -1066,6 +1091,15 @@ void IPARPi::processStats(unsigned int bufferId)\n>> >\n>> > setDelayedControls.emit(ctrls);\n>> > }\n>> > +\n>> > + /*\n>> > + * Set the focus position\n>> > + * \\todo Use an AF algorithm and replace the arbitrary value\n>> > + */\n>> > + ControlList lensCtrls(lensCtrls_);\n>> > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast(123));\n\nProbably it's just too early to nit-pick too much, but I wonder if the\ncontrol might have a default value that we could use? It would be just\nslightly less arbitrary!\n\nHaving said that, there's the whole question of where we will get\nsuitable ranges and default values for the lens position... at some\npoint we're going to have to figure that out.\n\nBest regards\nDavid\n\n>> > + setLensControls.emit(lensCtrls);\n>> > +\n>>\n>> no need for extra blank line here.\n>>\n>> But I guess this is just a 'sample' for the moment anyway.\n>>\n>> It really makes me think changing the IPA API to use a libcamera control\n>> list would be better too - so that the plumbing between is cleaner. But\n>> that's a whole chunk of rework...\n>>\n>>\n>> > }\n>> >\n>> > void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>> > --\n>> > 2.32.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 8A091BF415\n\tfor ;\n\tThu, 17 Mar 2022 11:16:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CEF58604E9;\n\tThu, 17 Mar 2022 12:16:33 +0100 (CET)","from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com\n\t[IPv6:2a00:1450:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 34BCA601F8\n\tfor ;\n\tThu, 17 Mar 2022 12:16:33 +0100 (CET)","by mail-wr1-x42d.google.com with SMTP id x15so6829387wru.13\n\tfor ;\n\tThu, 17 Mar 2022 04:16:33 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647515793;\n\tbh=vpdwB5HbEqzrxwV2KtOMxzkV97qN7BGErRdAJKY4So4=;\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=NWf7bLrh1BhIQAsFVEAkdc9bwPpdVYTnZsz/OnlnDx4XT8PdRdbCxuClhENKqfnvq\n\tlIBDt+muZ7jqKdyUaXctb+VuHneiHjiYmNplU8xh6HX4ls3CNmT/BzMninTXlLNUJP\n\tARrojD7MKvcWwA0uu2WVubxIv+buKpKiHrT6QzBxKt60tTqpIyMBeR0Kyy7/ORUNPX\n\tnVfNubDAd/oV+V3ek6RzrBXe8wzj04sZUli86p2kc8b1s1LY2/ivV5Nvb/Z6hMvLFU\n\tU9RtfkiDylUW2g8q439QtcB/MJEaueBKbo21nq4qF9Fc9JZgeKn4FlOyrYaBfnViWu\n\tsutUj3bCxhOvg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=8uk3lYKmWYd+6TDKnHqJ2LfHueHBwV9IS37Ba98GfaY=;\n\tb=gvfpbex3tfA5yGAS5TEnDLjx+Y/TW3+EcOhlOV4aAIKw4aTsLYbPHnZdYipDp9HGTJ\n\tMjutF43Sag48rMZinhozzYyOWrT+vuVUsibyFSKRMaqsnVULQYrmUTKPoVC3+p/8FRrw\n\t2yit7vGLdJsXcMYslyvpDrjMyA147rKr3eBsbUbutbjXCeT0Fv0uhv9hhj/sERjSsLMz\n\tsoAFzovaa4SU8ijRaE4FlX4nvNluaS2SJVVKynGkyvvM0hwbxuRE+UmgbCI6MO9ae/LK\n\tvpD6aotDu/p9lueQ7r2enqx7eYnlhH2AM1+zKdXsT4qu8mJPvUdW3KMWbT9Zur3MR1Jk\n\tR8WA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"gvfpbex3\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=8uk3lYKmWYd+6TDKnHqJ2LfHueHBwV9IS37Ba98GfaY=;\n\tb=ALojxOpIxedritycfyx60oI1zudsMljYaxvwhy2BLGD4BLISbqmnlFdaYztFNaRc+L\n\tWbKzroTA9xXgHShRcNbYFBF7H9EpnHkiIIr8QJiTh5v6s7m4k/Asm/W/nnBJxwxFpcs0\n\tc2WmzQfyW8DzOkSsatPapK5BqTgcTxs8jZG73M8Yxb79VkZ6U+sKrOWkO5+0wp6mNuT7\n\tgnsjOJsKgBbAB6odE920EgLa9ZT3rs7b2wBKdjI+9wutxH/NE9pXog9kcI63lsQ7R0CL\n\tinHbIEAnxkI8PSNENnF96SrJOjld0aDKoW3wRY57mVMSEe+M1tYjzUSIyFDHewEfTP0o\n\tChQw==","X-Gm-Message-State":"AOAM531wcSf3SW79ZfwKQ5qaiRF71dFBiy9SasYBWesecFDyeFfU5y2k\n\tBQZuTUMgy1vQlNrlgyj1LYYv4UzFDEegf6pihTml+MyUhIw=","X-Google-Smtp-Source":"ABdhPJzI3IwPtAwlE+xMZRokfvY9onHd2E0um6uh+ATeymYpC8uOb1dwrQqYi1oHxqUNR56qsssEgK98CRvkCEvdzgk=","X-Received":"by 2002:a5d:404c:0:b0:203:ea4e:3c07 with SMTP id\n\tw12-20020a5d404c000000b00203ea4e3c07mr2087023wrp.597.1647515792738;\n\tThu, 17 Mar 2022 04:16:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20220316093951.33779-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20220316093951.33779-2-jeanmichel.hautbois@ideasonboard.com>\n\t<164742585788.11309.4188991275024246751@Monstersaurus>\n\t","In-Reply-To":"","Date":"Thu, 17 Mar 2022 11:16:21 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Control the\n\tlens position","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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]