[{"id":26811,"web_url":"https://patchwork.libcamera.org/comment/26811/","msgid":"<20230403085754.duzzd6kk7ynn2daw@uno.localdomain>","date":"2023-04-03T08:57:54","subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Fri, Mar 31, 2023 at 10:19:22AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Add information about focus lens position limits to the IPA session\n> configuration. These information can then be used by IPA algorithms\n> to know which focus positions are valid.\n>\n> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>\n> ---\n>  src/ipa/rkisp1/ipa_context.cpp | 17 +++++++++++++++++\n>  src/ipa/rkisp1/ipa_context.h   |  9 +++++++++\n>  src/ipa/rkisp1/rkisp1.cpp      | 23 ++++++++++++++++++++++-\n>  3 files changed, 48 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 9bbf3684..aea99299 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -14,6 +14,18 @@\n>\n>  namespace libcamera::ipa::rkisp1 {\n>\n> +/**\n> + * \\struct LensConfiguration\n> + * \\brief Lens-specific parameters\n> + *\n> + * \\var LensConfiguration::minFocusPosition\n> + * \\brief Minimum position supported by the camera focus lens\n> + *\n> + * \\var LensConfiguration::maxFocusPosition\n> + * \\brief Maximum position supported by the camera focus lens\n> + *\n> + */\n> +\n>  /**\n>   * \\struct IPASessionConfiguration\n>   * \\brief Session configuration for the IPA module\n> @@ -89,6 +101,11 @@ namespace libcamera::ipa::rkisp1 {\n>   * \\brief Sensor output resolution\n>   */\n>\n> +/**\n> + * \\var IPASessionConfiguration::lens\n> + * \\brief Contains lens-specific parameters if lens was detected\n> + */\n> +\n>  /**\n>   * \\var IPASessionConfiguration::raw\n>   * \\brief Indicates if the camera is configured to capture raw frames\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index b9b20653..65b3fbfe 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -8,6 +8,8 @@\n>\n>  #pragma once\n>\n> +#include <optional>\n> +\n>  #include <linux/rkisp1-config.h>\n>\n>  #include <libcamera/base/utils.h>\n> @@ -20,6 +22,11 @@ namespace libcamera {\n>\n>  namespace ipa::rkisp1 {\n>\n> +struct LensConfiguration {\n> +\tint32_t minFocusPosition = 0;\n> +\tint32_t maxFocusPosition = 0;\n> +};\n> +\n>  struct IPASessionConfiguration {\n>  \tstruct {\n>  \t\tstruct rkisp1_cif_isp_window measureWindow;\n> @@ -45,6 +52,8 @@ struct IPASessionConfiguration {\n>  \t\tSize size;\n>  \t} sensor;\n>\n> +\tstd::optional<LensConfiguration> lens;\n> +\n>  \tstruct {\n>  \t\trkisp1_cif_isp_version revision;\n>  \t} hw;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index d338d374..292768cf 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -164,9 +164,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n>  \tcontext_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n>  \t\t\t\t\t\t   * 1.0s / sensorInfo.pixelRate;\n>\n> -\tif (!lensControls.empty())\n> +\tif (!lensControls.empty()) {\n>  \t\tlensControls_ = lensControls;\n>\n> +\t\tconst ControlInfo &focusAbsolute =\n> +\t\t\tlensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);\n> +\n> +\t\tLOG(IPARkISP1, Debug)\n> +\t\t\t<< \"Focus lens: \" << focusAbsolute.toString();\n> +\n> +\t\tcontext_.configuration.lens = {\n> +\t\t\t.minFocusPosition = focusAbsolute.min().get<int32_t>(),\n> +\t\t\t.maxFocusPosition = focusAbsolute.max().get<int32_t>()\n> +\t\t};\n\nDo you need to initialize context_.configuration.lens at init() time ?\n\nIt doesn't seem so to me as in updateControls() below you anyway\nre-fetch V4L2_CID_FOCUS_ABSOLUTE from lensControls_.\n\nCan't it be done in configure() to avoid ...\n\n> +\t}\n> +\n>  \t/* Load the tuning data file. */\n>  \tFile file(settings.configurationFile);\n>  \tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> @@ -234,6 +246,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n>  \t\t<< \"Exposure: [\" << minExposure << \", \" << maxExposure\n>  \t\t<< \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n>\n> +\t/*\n> +\t * Save the existing lens configuration and restore it after context\n> +\t * reset. It does not change since init().\n> +\t */\n> +\tconst std::optional<LensConfiguration> lensConfig =\n> +\t\tcontext_.configuration.lens;\n> +\n\n... this ?\n\nThe computation seems very cheap and camera configuration happens once\nper streaming session, so I wonder if the it wouldn't be better to\nmaintain a cleaner coding patch at the expense of this little cost.\n\nThe rest looks good!\n\n\n>  \t/* Clear the IPA context before the streaming session. */\n>  \tcontext_.configuration = {};\n>  \tcontext_.activeState = {};\n> @@ -272,6 +291,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n>  \t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>  \t\t});\n>\n> +\tcontext_.configuration.lens = lensConfig;\n> +\n>  \tfor (auto const &a : algorithms()) {\n>  \t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n>\n> --\n> 2.39.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D0BDDC0F2A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  3 Apr 2023 08:58:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F98D6271C;\n\tMon,  3 Apr 2023 10:58:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B62861EC6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  3 Apr 2023 10:57:58 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC0476CF;\n\tMon,  3 Apr 2023 10:57:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680512280;\n\tbh=AfLb5jQZQSOe4tYm9UBdnmOsVKSxVNf91M+8Y3USJtw=;\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=fLZXceGpeLALnek0X4/hE/2NRGLnjwUcq1ioGpCtXVm1IIQEKPW97m/tEDGX1+A9V\n\t1rVQN3e/frDTrlgR0V4R6kORVYLYQyWoByvbg/FU9SCBTvWl3zmZbz05qMxeljOF2J\n\tslkHYrBSPyKdTBV3sjJ3zS9urkDvDUkOam8CEdth/Hu6p8H6SPu3h4Z8Zv9hwINfHl\n\tlexk4qA7wIvd//3kI1/lMRwJS4ypXuK8RXFcaqcrGKQqsw3fzekoAXO9YaDq0bjpX2\n\tDpYv3T9hnT073vjz/7EysHOD6d97vw0UyS7/OapJQ66ELYrJfv6rAV3PU7LSaKMoBz\n\tY43mnVeVcbZEw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680512277;\n\tbh=AfLb5jQZQSOe4tYm9UBdnmOsVKSxVNf91M+8Y3USJtw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GjlffSaZFStl8wlyI4MkW5QJRAk80fqpORFIoXmd6YRLaomDBM1eO7F6Lhaf9Sx1a\n\tw2QRNdEANlr9mMYal+BgX1luwC5GzqJCy+0Yh8xfz8aAwTXD1QJZdPHi4C5Xq9AEh/\n\tse6n4fKo5VnGqGQwR799nLhDSsysylViAq2vndKI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GjlffSaZ\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 10:57:54 +0200","To":"Daniel Semkowicz <dse@thaumatec.com>","Message-ID":"<20230403085754.duzzd6kk7ynn2daw@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-3-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230331081930.19289-3-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26827,"web_url":"https://patchwork.libcamera.org/comment/26827/","msgid":"<CAHgnY3k81BjgFaYUGpyeRHxAMkmqLZAmdC40jHVjoOu4AkYH4Q@mail.gmail.com>","date":"2023-04-04T06:06:33","subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi Daniel\n>\n> On Fri, Mar 31, 2023 at 10:19:22AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Add information about focus lens position limits to the IPA session\n> > configuration. These information can then be used by IPA algorithms\n> > to know which focus positions are valid.\n> >\n> > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>\n> > ---\n> >  src/ipa/rkisp1/ipa_context.cpp | 17 +++++++++++++++++\n> >  src/ipa/rkisp1/ipa_context.h   |  9 +++++++++\n> >  src/ipa/rkisp1/rkisp1.cpp      | 23 ++++++++++++++++++++++-\n> >  3 files changed, 48 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 9bbf3684..aea99299 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -14,6 +14,18 @@\n> >\n> >  namespace libcamera::ipa::rkisp1 {\n> >\n> > +/**\n> > + * \\struct LensConfiguration\n> > + * \\brief Lens-specific parameters\n> > + *\n> > + * \\var LensConfiguration::minFocusPosition\n> > + * \\brief Minimum position supported by the camera focus lens\n> > + *\n> > + * \\var LensConfiguration::maxFocusPosition\n> > + * \\brief Maximum position supported by the camera focus lens\n> > + *\n> > + */\n> > +\n> >  /**\n> >   * \\struct IPASessionConfiguration\n> >   * \\brief Session configuration for the IPA module\n> > @@ -89,6 +101,11 @@ namespace libcamera::ipa::rkisp1 {\n> >   * \\brief Sensor output resolution\n> >   */\n> >\n> > +/**\n> > + * \\var IPASessionConfiguration::lens\n> > + * \\brief Contains lens-specific parameters if lens was detected\n> > + */\n> > +\n> >  /**\n> >   * \\var IPASessionConfiguration::raw\n> >   * \\brief Indicates if the camera is configured to capture raw frames\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index b9b20653..65b3fbfe 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -8,6 +8,8 @@\n> >\n> >  #pragma once\n> >\n> > +#include <optional>\n> > +\n> >  #include <linux/rkisp1-config.h>\n> >\n> >  #include <libcamera/base/utils.h>\n> > @@ -20,6 +22,11 @@ namespace libcamera {\n> >\n> >  namespace ipa::rkisp1 {\n> >\n> > +struct LensConfiguration {\n> > +     int32_t minFocusPosition = 0;\n> > +     int32_t maxFocusPosition = 0;\n> > +};\n> > +\n> >  struct IPASessionConfiguration {\n> >       struct {\n> >               struct rkisp1_cif_isp_window measureWindow;\n> > @@ -45,6 +52,8 @@ struct IPASessionConfiguration {\n> >               Size size;\n> >       } sensor;\n> >\n> > +     std::optional<LensConfiguration> lens;\n> > +\n> >       struct {\n> >               rkisp1_cif_isp_version revision;\n> >       } hw;\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index d338d374..292768cf 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -164,9 +164,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> >       context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> >                                                  * 1.0s / sensorInfo.pixelRate;\n> >\n> > -     if (!lensControls.empty())\n> > +     if (!lensControls.empty()) {\n> >               lensControls_ = lensControls;\n> >\n> > +             const ControlInfo &focusAbsolute =\n> > +                     lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);\n> > +\n> > +             LOG(IPARkISP1, Debug)\n> > +                     << \"Focus lens: \" << focusAbsolute.toString();\n> > +\n> > +             context_.configuration.lens = {\n> > +                     .minFocusPosition = focusAbsolute.min().get<int32_t>(),\n> > +                     .maxFocusPosition = focusAbsolute.max().get<int32_t>()\n> > +             };\n>\n> Do you need to initialize context_.configuration.lens at init() time ?\n>\n> It doesn't seem so to me as in updateControls() below you anyway\n> re-fetch V4L2_CID_FOCUS_ABSOLUTE from lensControls_.\n>\n> Can't it be done in configure() to avoid ...\n>\n> > +     }\n> > +\n> >       /* Load the tuning data file. */\n> >       File file(settings.configurationFile);\n> >       if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > @@ -234,6 +246,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> >               << \"Exposure: [\" << minExposure << \", \" << maxExposure\n> >               << \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n> >\n> > +     /*\n> > +      * Save the existing lens configuration and restore it after context\n> > +      * reset. It does not change since init().\n> > +      */\n> > +     const std::optional<LensConfiguration> lensConfig =\n> > +             context_.configuration.lens;\n> > +\n>\n> ... this ?\n>\n> The computation seems very cheap and camera configuration happens once\n> per streaming session, so I wonder if the it wouldn't be better to\n> maintain a cleaner coding patch at the expense of this little cost.\n\nAs you already noticed in the next commits, the\ncontext_.configuration.lens is used to validate early in the Af::init()\nif lens is available and fail if it is missing. I would like to have\nthe lens configuration available as early as possible to be able to\ncheck it early and it should not change during runtime.\n\nI can move the context_.configuration.lens initialization to a separate\nfunction and call it twice (in the init() and configure()) to avoid\ncopying and code duplication.\n\nHowever, I do not understand why the context_.configuration is cleared\nin the configure(). Problems with setting the same config multiple\ntimes could be easily avoided, if configuration persist between\nfunction calls. Parameters that change can just overwrite the existing\nvalue, without clearing the whole structure. And right now, only the\nlineDuration is set in the init().\n\n>\n> The rest looks good!\n>\n>\n> >       /* Clear the IPA context before the streaming session. */\n> >       context_.configuration = {};\n> >       context_.activeState = {};\n> > @@ -272,6 +291,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> >                       return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> >               });\n> >\n> > +     context_.configuration.lens = lensConfig;\n> > +\n> >       for (auto const &a : algorithms()) {\n> >               Algorithm *algo = static_cast<Algorithm *>(a.get());\n> >\n> > --\n> > 2.39.2\n> >\n\nBest regards\nDaniel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5EA17C326B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  4 Apr 2023 06:06:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 158EA62720;\n\tTue,  4 Apr 2023 08:06:47 +0200 (CEST)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AC4E62720\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  4 Apr 2023 08:06:45 +0200 (CEST)","by mail-ed1-x529.google.com with SMTP id eg48so125952603edb.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 03 Apr 2023 23:06:45 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680588407;\n\tbh=KRqTOMyyTJIMIgMNIKKbvoBFJ4MCL5gh1OHT0usIzDo=;\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=aWt/9F42YfBVhZ+yFqYzfA6ALMXhe4yo31d2J0H9SYCsSSYPaqyVg/NDJynhDgGjd\n\tlEKjUgw/FuGtuedq5NhNjWKTpXxSy0HS4k2mz1/f5W5Dkz09W00H8u6f+uap7EnZ1S\n\tlsQi/xYq7dpub2DQdB221mqwUn6zwEnMwMalEzHBydwVjIw/xrxK2OilXePjGJZRAw\n\tsykp7aPtg2aSZNjZWHlwSTlordhvWch5X8dkT+RFOfHKdhZCAC6RrkfAVFB16GgF6J\n\tfCrtZbuuPirbwQXQQWxumFNCBlzEpd1Hi0RTvEFTFOPx23wzYW/fTD1U043LZlO18i\n\tjaQgh435qeIRw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1680588404;\n\tx=1683180404; \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=ZCDeCgm/Mg31aBuWmHPb0gnZsjfx0thPv/wmbdCjVS4=;\n\tb=oaCDeKCQMt6YhlRuvmujUy1RVK4jNNuNHKNHeYJ7Nldl7myYpQcfFhTsUs8Kp6QHlj\n\tj6Y+KXuDmPFd/pkZd+jhtmZrK0kOZ9bxcyhVQC/6cWou6xkYdfWTiId14w4REeGNA6wr\n\t6Yr5IrP+P1UK4oKw1gSpWW1UJSkImiwh10hJFC1z5QbR8+eujxGQJhj19B+fji//ay1l\n\tkXXCpdUKQWrnl100TySBw3kNSxvlz8kdAEoeNqbD5k9ZoBPxoBRaCDSU8YnCJkX9oBYy\n\tetViT4oofYhwapbc9rTFyjPbUivz9hDgRrPqzRGGzs+CTsi3Kf63HpX3hzEY091xHIlk\n\tHOxw=="],"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=\"oaCDeKCQ\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680588404; x=1683180404;\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=ZCDeCgm/Mg31aBuWmHPb0gnZsjfx0thPv/wmbdCjVS4=;\n\tb=BWikhWMtjCPP9uwFRDHV+Bn3oHguP3blG5J5Ppc7YblxfAbkxgKX4ytGxML6WMPVTx\n\tAfSLsFCkmFMu7bgmHwPIg6gB6FrKGUGJ38s3miWlklkX6JmBq2f12bFGXRSXSnvlo8hb\n\txyDjL7hc0MaJf1zQEl4EWjXy1Xgwk4NhTHdSeXfbMbxUy98vw+E3lyZm6CO+KmPlX8UO\n\tKiP21XGTCTUIJ8IFhhRVaDMllrg8odpCntaOZQIOMrxm7bxIXwwaAkZm8CAvq9CxjOyS\n\tVbrBlGrjUNBwmmx+ReA+R4wdqPdCPC5UmeVJOe4A+NTqfVipnjxoL2BdabVIpne1WpAC\n\tYkkQ==","X-Gm-Message-State":"AAQBX9fzhAFZTV9e8iQwL66ca66M8I5DFvc4UnplP9BGHfboq5d5uctk\n\tHnSm0xpFA6jv8gl2s0PTjhhjtuCv/BsDrs5//KNelw==","X-Google-Smtp-Source":"AKy350Y6vdG7MRx8vVykemgqfhXvwYUsAx/CZ9oRbsGVFQReiRTCgH2zzxI4VscvRoACLbXJWu2EMWzya9QINNm2ZEU=","X-Received":"by 2002:a50:9f89:0:b0:4fb:c8e3:1adb with SMTP id\n\tc9-20020a509f89000000b004fbc8e31adbmr778379edf.3.1680588404622;\n\tMon, 03 Apr 2023 23:06:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-3-dse@thaumatec.com>\n\t<20230403085754.duzzd6kk7ynn2daw@uno.localdomain>","In-Reply-To":"<20230403085754.duzzd6kk7ynn2daw@uno.localdomain>","Date":"Tue, 4 Apr 2023 08:06:33 +0200","Message-ID":"<CAHgnY3k81BjgFaYUGpyeRHxAMkmqLZAmdC40jHVjoOu4AkYH4Q@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Daniel Semkowicz via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Daniel Semkowicz <dse@thaumatec.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26829,"web_url":"https://patchwork.libcamera.org/comment/26829/","msgid":"<20230404062538.f6klin3wjswa7l2e@uno.localdomain>","date":"2023-04-04T06:25:38","subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Tue, Apr 04, 2023 at 08:06:33AM +0200, Daniel Semkowicz wrote:\n> Hi Jacopo,\n>\n> On Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi\n> <jacopo.mondi@ideasonboard.com> wrote:\n> >\n> > Hi Daniel\n> >\n> > On Fri, Mar 31, 2023 at 10:19:22AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > Add information about focus lens position limits to the IPA session\n> > > configuration. These information can then be used by IPA algorithms\n> > > to know which focus positions are valid.\n> > >\n> > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>\n> > > ---\n> > >  src/ipa/rkisp1/ipa_context.cpp | 17 +++++++++++++++++\n> > >  src/ipa/rkisp1/ipa_context.h   |  9 +++++++++\n> > >  src/ipa/rkisp1/rkisp1.cpp      | 23 ++++++++++++++++++++++-\n> > >  3 files changed, 48 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > index 9bbf3684..aea99299 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > @@ -14,6 +14,18 @@\n> > >\n> > >  namespace libcamera::ipa::rkisp1 {\n> > >\n> > > +/**\n> > > + * \\struct LensConfiguration\n> > > + * \\brief Lens-specific parameters\n> > > + *\n> > > + * \\var LensConfiguration::minFocusPosition\n> > > + * \\brief Minimum position supported by the camera focus lens\n> > > + *\n> > > + * \\var LensConfiguration::maxFocusPosition\n> > > + * \\brief Maximum position supported by the camera focus lens\n> > > + *\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\struct IPASessionConfiguration\n> > >   * \\brief Session configuration for the IPA module\n> > > @@ -89,6 +101,11 @@ namespace libcamera::ipa::rkisp1 {\n> > >   * \\brief Sensor output resolution\n> > >   */\n> > >\n> > > +/**\n> > > + * \\var IPASessionConfiguration::lens\n> > > + * \\brief Contains lens-specific parameters if lens was detected\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\var IPASessionConfiguration::raw\n> > >   * \\brief Indicates if the camera is configured to capture raw frames\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index b9b20653..65b3fbfe 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -8,6 +8,8 @@\n> > >\n> > >  #pragma once\n> > >\n> > > +#include <optional>\n> > > +\n> > >  #include <linux/rkisp1-config.h>\n> > >\n> > >  #include <libcamera/base/utils.h>\n> > > @@ -20,6 +22,11 @@ namespace libcamera {\n> > >\n> > >  namespace ipa::rkisp1 {\n> > >\n> > > +struct LensConfiguration {\n> > > +     int32_t minFocusPosition = 0;\n> > > +     int32_t maxFocusPosition = 0;\n> > > +};\n> > > +\n> > >  struct IPASessionConfiguration {\n> > >       struct {\n> > >               struct rkisp1_cif_isp_window measureWindow;\n> > > @@ -45,6 +52,8 @@ struct IPASessionConfiguration {\n> > >               Size size;\n> > >       } sensor;\n> > >\n> > > +     std::optional<LensConfiguration> lens;\n> > > +\n> > >       struct {\n> > >               rkisp1_cif_isp_version revision;\n> > >       } hw;\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index d338d374..292768cf 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -164,9 +164,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > >       context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> > >                                                  * 1.0s / sensorInfo.pixelRate;\n> > >\n> > > -     if (!lensControls.empty())\n> > > +     if (!lensControls.empty()) {\n> > >               lensControls_ = lensControls;\n> > >\n> > > +             const ControlInfo &focusAbsolute =\n> > > +                     lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);\n> > > +\n> > > +             LOG(IPARkISP1, Debug)\n> > > +                     << \"Focus lens: \" << focusAbsolute.toString();\n> > > +\n> > > +             context_.configuration.lens = {\n> > > +                     .minFocusPosition = focusAbsolute.min().get<int32_t>(),\n> > > +                     .maxFocusPosition = focusAbsolute.max().get<int32_t>()\n> > > +             };\n> >\n> > Do you need to initialize context_.configuration.lens at init() time ?\n> >\n> > It doesn't seem so to me as in updateControls() below you anyway\n> > re-fetch V4L2_CID_FOCUS_ABSOLUTE from lensControls_.\n> >\n> > Can't it be done in configure() to avoid ...\n> >\n> > > +     }\n> > > +\n> > >       /* Load the tuning data file. */\n> > >       File file(settings.configurationFile);\n> > >       if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > @@ -234,6 +246,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > >               << \"Exposure: [\" << minExposure << \", \" << maxExposure\n> > >               << \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n> > >\n> > > +     /*\n> > > +      * Save the existing lens configuration and restore it after context\n> > > +      * reset. It does not change since init().\n> > > +      */\n> > > +     const std::optional<LensConfiguration> lensConfig =\n> > > +             context_.configuration.lens;\n> > > +\n> >\n> > ... this ?\n> >\n> > The computation seems very cheap and camera configuration happens once\n> > per streaming session, so I wonder if the it wouldn't be better to\n> > maintain a cleaner coding patch at the expense of this little cost.\n>\n> As you already noticed in the next commits, the\n> context_.configuration.lens is used to validate early in the Af::init()\n> if lens is available and fail if it is missing. I would like to have\n> the lens configuration available as early as possible to be able to\n> check it early and it should not change during runtime.\n>\n\nThis makes sense, what doesn't sound right to me is that\ncontext_.configuration is meant to be transient between configurations\n\n> I can move the context_.configuration.lens initialization to a separate\n> function and call it twice (in the init() and configure()) to avoid\n> copying and code duplication.\n>\n\nNot worth it imho.\n\nWhat I actually wonder is if we don't need a context_.initdata or\nsomething where to store parameters that are fixed for the whole\nlifetime of the IPA\n\n> However, I do not understand why the context_.configuration is cleared\n> in the configure(). Problems with setting the same config multiple\n> times could be easily avoided, if configuration persist between\n> function calls. Parameters that change can just overwrite the existing\n> value, without clearing the whole structure. And right now, only the\n> lineDuration is set in the init().\n>\n\nI think instead it is desirable to clear context_.configuration as it\nshould only contain data that are valid for a single capture session.\nIt's for extra safty if you like, to make sure the IPA doesn't operate\non stale data.\n\nLet's put it in this way, there's no need to re-design this if you\ndon't want to, you can keep it this way and I can explore on top if a\ncontext_.initdata might help.\n\n> >\n> > The rest looks good!\n> >\n> >\n> > >       /* Clear the IPA context before the streaming session. */\n> > >       context_.configuration = {};\n> > >       context_.activeState = {};\n> > > @@ -272,6 +291,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > >                       return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > >               });\n> > >\n> > > +     context_.configuration.lens = lensConfig;\n> > > +\n> > >       for (auto const &a : algorithms()) {\n> > >               Algorithm *algo = static_cast<Algorithm *>(a.get());\n> > >\n> > > --\n> > > 2.39.2\n> > >\n>\n> Best regards\n> Daniel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5DC63C326B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  4 Apr 2023 06:25:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BD1962724;\n\tTue,  4 Apr 2023 08:25:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0016D603A4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  4 Apr 2023 08:25:41 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 705A77F8;\n\tTue,  4 Apr 2023 08:25:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680589543;\n\tbh=n2k0bDN9SHmPUBEYSbEJlxFTUMgcc3YcGdaf+fGJCjs=;\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=YKzKpH0/4NFZTkwkKplJpbe7EN+VHVlTDwjTa2ik5hvD7EPbO/79pO3abcj0n450y\n\tTCZD5dcUjV2SseFUJCacUZdqlvZWDxmnYfQ9SU9opUgB7uF1ah8MAwYVWU786H4A8r\n\tm9J1+iEUZgeI88leFTYYrlin9ZethzTK54AQE6t+lwL8OCFdpI/tRH12e6Jg2CFftm\n\tnr8eqEE4E7bJntYJAGzCF4BftpvxZBZoJy9s7a7VFsyl2h1kaYhC6eDZ0x9f2HeOHW\n\tQygwvAiqS+EkNvaxm16OsDCTQ/ihjn8x53x7gXq9M5bA7hq7lUtmH+NMA7Mo3IseWs\n\tpWr21MGK7eU6g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680589541;\n\tbh=n2k0bDN9SHmPUBEYSbEJlxFTUMgcc3YcGdaf+fGJCjs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OUoj6hJK3GC+BRR3S4/qQTYpswcOXsqww+IGuoYjEB/NbKTwZVzgNoD+KUIszDPlq\n\tA28bSmJD9jmnRV6N8WoHqcRhqqajIbSlBHx9TeTt4xBNlaQ25hN1FxK7mnjyyFjkc7\n\tWQlGGJShFKxt8Q/UqPX9YGUAlrfrR5kl0NoD13fs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OUoj6hJK\"; dkim-atps=neutral","Date":"Tue, 4 Apr 2023 08:25:38 +0200","To":"Daniel Semkowicz <dse@thaumatec.com>","Message-ID":"<20230404062538.f6klin3wjswa7l2e@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-3-dse@thaumatec.com>\n\t<20230403085754.duzzd6kk7ynn2daw@uno.localdomain>\n\t<CAHgnY3k81BjgFaYUGpyeRHxAMkmqLZAmdC40jHVjoOu4AkYH4Q@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<CAHgnY3k81BjgFaYUGpyeRHxAMkmqLZAmdC40jHVjoOu4AkYH4Q@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26830,"web_url":"https://patchwork.libcamera.org/comment/26830/","msgid":"<CAHgnY3=e9prgE+=F7u9GL47VUoi9EKuC=TQzAn_4Q569G0ykWA@mail.gmail.com>","date":"2023-04-04T06:47:38","subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"On Tue, Apr 4, 2023 at 8:25 AM Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi Daniel\n>\n> On Tue, Apr 04, 2023 at 08:06:33AM +0200, Daniel Semkowicz wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi\n> > <jacopo.mondi@ideasonboard.com> wrote:\n> > >\n> > > Hi Daniel\n> > >\n> > > On Fri, Mar 31, 2023 at 10:19:22AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Add information about focus lens position limits to the IPA session\n> > > > configuration. These information can then be used by IPA algorithms\n> > > > to know which focus positions are valid.\n> > > >\n> > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>\n> > > > ---\n> > > >  src/ipa/rkisp1/ipa_context.cpp | 17 +++++++++++++++++\n> > > >  src/ipa/rkisp1/ipa_context.h   |  9 +++++++++\n> > > >  src/ipa/rkisp1/rkisp1.cpp      | 23 ++++++++++++++++++++++-\n> > > >  3 files changed, 48 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > index 9bbf3684..aea99299 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > @@ -14,6 +14,18 @@\n> > > >\n> > > >  namespace libcamera::ipa::rkisp1 {\n> > > >\n> > > > +/**\n> > > > + * \\struct LensConfiguration\n> > > > + * \\brief Lens-specific parameters\n> > > > + *\n> > > > + * \\var LensConfiguration::minFocusPosition\n> > > > + * \\brief Minimum position supported by the camera focus lens\n> > > > + *\n> > > > + * \\var LensConfiguration::maxFocusPosition\n> > > > + * \\brief Maximum position supported by the camera focus lens\n> > > > + *\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\struct IPASessionConfiguration\n> > > >   * \\brief Session configuration for the IPA module\n> > > > @@ -89,6 +101,11 @@ namespace libcamera::ipa::rkisp1 {\n> > > >   * \\brief Sensor output resolution\n> > > >   */\n> > > >\n> > > > +/**\n> > > > + * \\var IPASessionConfiguration::lens\n> > > > + * \\brief Contains lens-specific parameters if lens was detected\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\var IPASessionConfiguration::raw\n> > > >   * \\brief Indicates if the camera is configured to capture raw frames\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index b9b20653..65b3fbfe 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -8,6 +8,8 @@\n> > > >\n> > > >  #pragma once\n> > > >\n> > > > +#include <optional>\n> > > > +\n> > > >  #include <linux/rkisp1-config.h>\n> > > >\n> > > >  #include <libcamera/base/utils.h>\n> > > > @@ -20,6 +22,11 @@ namespace libcamera {\n> > > >\n> > > >  namespace ipa::rkisp1 {\n> > > >\n> > > > +struct LensConfiguration {\n> > > > +     int32_t minFocusPosition = 0;\n> > > > +     int32_t maxFocusPosition = 0;\n> > > > +};\n> > > > +\n> > > >  struct IPASessionConfiguration {\n> > > >       struct {\n> > > >               struct rkisp1_cif_isp_window measureWindow;\n> > > > @@ -45,6 +52,8 @@ struct IPASessionConfiguration {\n> > > >               Size size;\n> > > >       } sensor;\n> > > >\n> > > > +     std::optional<LensConfiguration> lens;\n> > > > +\n> > > >       struct {\n> > > >               rkisp1_cif_isp_version revision;\n> > > >       } hw;\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index d338d374..292768cf 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -164,9 +164,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > > >       context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> > > >                                                  * 1.0s / sensorInfo.pixelRate;\n> > > >\n> > > > -     if (!lensControls.empty())\n> > > > +     if (!lensControls.empty()) {\n> > > >               lensControls_ = lensControls;\n> > > >\n> > > > +             const ControlInfo &focusAbsolute =\n> > > > +                     lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > +\n> > > > +             LOG(IPARkISP1, Debug)\n> > > > +                     << \"Focus lens: \" << focusAbsolute.toString();\n> > > > +\n> > > > +             context_.configuration.lens = {\n> > > > +                     .minFocusPosition = focusAbsolute.min().get<int32_t>(),\n> > > > +                     .maxFocusPosition = focusAbsolute.max().get<int32_t>()\n> > > > +             };\n> > >\n> > > Do you need to initialize context_.configuration.lens at init() time ?\n> > >\n> > > It doesn't seem so to me as in updateControls() below you anyway\n> > > re-fetch V4L2_CID_FOCUS_ABSOLUTE from lensControls_.\n> > >\n> > > Can't it be done in configure() to avoid ...\n> > >\n> > > > +     }\n> > > > +\n> > > >       /* Load the tuning data file. */\n> > > >       File file(settings.configurationFile);\n> > > >       if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > > @@ -234,6 +246,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > >               << \"Exposure: [\" << minExposure << \", \" << maxExposure\n> > > >               << \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n> > > >\n> > > > +     /*\n> > > > +      * Save the existing lens configuration and restore it after context\n> > > > +      * reset. It does not change since init().\n> > > > +      */\n> > > > +     const std::optional<LensConfiguration> lensConfig =\n> > > > +             context_.configuration.lens;\n> > > > +\n> > >\n> > > ... this ?\n> > >\n> > > The computation seems very cheap and camera configuration happens once\n> > > per streaming session, so I wonder if the it wouldn't be better to\n> > > maintain a cleaner coding patch at the expense of this little cost.\n> >\n> > As you already noticed in the next commits, the\n> > context_.configuration.lens is used to validate early in the Af::init()\n> > if lens is available and fail if it is missing. I would like to have\n> > the lens configuration available as early as possible to be able to\n> > check it early and it should not change during runtime.\n> >\n>\n> This makes sense, what doesn't sound right to me is that\n> context_.configuration is meant to be transient between configurations\n>\n> > I can move the context_.configuration.lens initialization to a separate\n> > function and call it twice (in the init() and configure()) to avoid\n> > copying and code duplication.\n> >\n>\n> Not worth it imho.\n>\n> What I actually wonder is if we don't need a context_.initdata or\n> something where to store parameters that are fixed for the whole\n> lifetime of the IPA\n\nThis sounds like something that would simplify the configuration.\n\n>\n> > However, I do not understand why the context_.configuration is cleared\n> > in the configure(). Problems with setting the same config multiple\n> > times could be easily avoided, if configuration persist between\n> > function calls. Parameters that change can just overwrite the existing\n> > value, without clearing the whole structure. And right now, only the\n> > lineDuration is set in the init().\n> >\n>\n> I think instead it is desirable to clear context_.configuration as it\n> should only contain data that are valid for a single capture session.\n> It's for extra safty if you like, to make sure the IPA doesn't operate\n> on stale data.\n>\n> Let's put it in this way, there's no need to re-design this if you\n> don't want to, you can keep it this way and I can explore on top if a\n> context_.initdata might help.\n\nYes, I would like to avoid intermediate steps in this case, as there\nis only a neglible profit here.\n\n>\n> > >\n> > > The rest looks good!\n> > >\n> > >\n> > > >       /* Clear the IPA context before the streaming session. */\n> > > >       context_.configuration = {};\n> > > >       context_.activeState = {};\n> > > > @@ -272,6 +291,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > >                       return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > > >               });\n> > > >\n> > > > +     context_.configuration.lens = lensConfig;\n> > > > +\n> > > >       for (auto const &a : algorithms()) {\n> > > >               Algorithm *algo = static_cast<Algorithm *>(a.get());\n> > > >\n> > > > --\n> > > > 2.39.2\n> > > >\n> >\n> > Best regards\n> > Daniel\n\nThanks!\nDaniel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 62F74C0F2A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  4 Apr 2023 06:47:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7A3F6271C;\n\tTue,  4 Apr 2023 08:47:51 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92DFE603A4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  4 Apr 2023 08:47:49 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id h8so126407633ede.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 03 Apr 2023 23:47:49 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680590871;\n\tbh=c+TjiE/gmZpFofbHey3ABzYccOzkuOx/2pF+oSBb1+E=;\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=M1/8M7R34HmCqH2yNE2RQOaGand614Vbrb448vXpQuYdajyxAVjcy9ZmgDru6nlHf\n\t+wF5rqEQ7hUrEbWHZmqzyK0dyPxBIRVlgGJ0ckK9MEWMMleTVw8TFGDWYTVcdT5hQx\n\tFIrcB9k+O4GwYJGvkkESh/3s1blMlqfVb6VDgVG2E3iR9++/2tXdBaXxAKFbhBkAj6\n\tpaRttMuNxd0278yNeseaypu0yPePrP19Fae1U26ey9RMSzD6AO8nc9pBT0ud59/k5z\n\tHnyY5QsYeoaRd603hRmjVgEFYuPz/1tlLtNxayqs4VjWBhq8/ak81ND3+kYgNUfZHE\n\tXSbcscvG1cxVg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1680590869;\n\tx=1683182869; \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=laU5UmKWCwVc2fVw/3OOrUfuOJMJAOO1JJo5d+YAOd0=;\n\tb=ipWNvDs5sJLqls+YYmeWW99JgKo/73f+HN+7ftxJ0dB9oZZZeUtkV5eVHoQ3KyG3xp\n\tqTxaUDoOlUpl4VfR20H4PVnqyPMxFlN55r+2gkp1FK6q90uFGUCpSr1LsujQlvRI/rGQ\n\t+SHz41ANyalw9f3NMS6FCEiahe8iqMP/EyyHB2xdM/rXiQEBDm047OxOSJBvX1B6Hg4N\n\tq6sGQ4KPo+dLupCwzsX0CG/Fx2iVcfgYIZYNhdHlRPq1JoRYC8qFy+UP0UVw6FbhzWWN\n\ttHUezo63+SBAST5KkyN/rvWK9tVpdajHjrQeYQBzGgaBS0swuOOaxfjn2WTn69MvwRPP\n\to/5A=="],"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=\"ipWNvDs5\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680590869; x=1683182869;\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=laU5UmKWCwVc2fVw/3OOrUfuOJMJAOO1JJo5d+YAOd0=;\n\tb=oGiWtsSoEdHdpGrKkWv+pb0C+iCCmb12amMUWce466oABTKCqL2vcNg+AF0Nb/opsV\n\tCtRG1M+dG+hnq8b+W+tngh8n6q3qxQUueth4zfxkFjZ7cdxsnLdmi/Rhtm0CtkNW6YmG\n\tcqLyBm/LAb+emgApZWr5rxy84joViKQrXM8TRls6VIdEZmNfN8sR8Qt93isz17fNSPqc\n\tiwovB8gywrSD8NyawIE02NLNiJN+yTy/BJSDItCGfPH3c1Ex4ozwDy7GofN2We0f4rFg\n\tEHuWHB4se7OTYnpo/0Nw0VerZ9ozpeA2v+aTsEuH2znzhSiLdKk+gBqjzU+eqhk2nmM3\n\tofNw==","X-Gm-Message-State":"AAQBX9f8YUcIK4/a+foD2STVF001q3NVZNrDy++Hy4E49bOvbmyCAcsN\n\tBeJzPogmAzb5LPdXpJkGLaHUJbZo1gdD9hYeTHIo0A==","X-Google-Smtp-Source":"AKy350b+fCNIH8k4E3ehADfBZ/Y4T2ljx4pm17gP3yKGS2sF5NZqIv1+DC6y7Cgy+LcwC61uShCURogBdocKkqHaU5I=","X-Received":"by 2002:a17:907:961a:b0:92f:cbfe:1635 with SMTP id\n\tgb26-20020a170907961a00b0092fcbfe1635mr765152ejc.6.1680590868990;\n\tMon, 03 Apr 2023 23:47:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-3-dse@thaumatec.com>\n\t<20230403085754.duzzd6kk7ynn2daw@uno.localdomain>\n\t<CAHgnY3k81BjgFaYUGpyeRHxAMkmqLZAmdC40jHVjoOu4AkYH4Q@mail.gmail.com>\n\t<20230404062538.f6klin3wjswa7l2e@uno.localdomain>","In-Reply-To":"<20230404062538.f6klin3wjswa7l2e@uno.localdomain>","Date":"Tue, 4 Apr 2023 08:47:38 +0200","Message-ID":"<CAHgnY3=e9prgE+=F7u9GL47VUoi9EKuC=TQzAn_4Q569G0ykWA@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits\n\tto the session config","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Daniel Semkowicz via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Daniel Semkowicz <dse@thaumatec.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]