[{"id":23850,"web_url":"https://patchwork.libcamera.org/comment/23850/","msgid":"<Ys4bWPdgMpWLb30B@pendragon.ideasonboard.com>","date":"2022-07-13T01:09:44","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Florian,\n\nThank you for the patch.\n\nOn Mon, Jul 04, 2022 at 05:23:15PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> Add queueRequest() function to the Algorithm class. The queueRequest() function\n> provides controls values coming from the application to each algorithm.\n> Each algorithm is responsible for retrieving the controls associated to them.\n> \n> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> ---\n>  src/ipa/ipu3/module.h        |  2 +-\n>  src/ipa/libipa/algorithm.cpp | 13 +++++++++++++\n>  src/ipa/libipa/algorithm.h   |  6 ++++++\n>  src/ipa/libipa/module.cpp    |  5 +++++\n>  src/ipa/libipa/module.h      |  3 ++-\n>  src/ipa/rkisp1/module.h      |  2 +-\n>  6 files changed, 28 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h\n> index d94fc459..5c2179e0 100644\n> --- a/src/ipa/ipu3/module.h\n> +++ b/src/ipa/ipu3/module.h\n> @@ -20,7 +20,7 @@ namespace libcamera {\n>  namespace ipa::ipu3 {\n>  \n>  using Module = ipa::Module<IPAContext, IPAFrameContext, IPAConfigInfo,\n> -\t\t\t   ipu3_uapi_params, ipu3_uapi_stats_3a>;\n> +\t\t\t   ipu3_uapi_params, ipu3_uapi_stats_3a, ControlList>;\n>  \n>  } /* namespace ipa::ipu3 */\n>  \n> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> index 8549fe3f..1fb811ef 100644\n> --- a/src/ipa/libipa/algorithm.cpp\n> +++ b/src/ipa/libipa/algorithm.cpp\n> @@ -81,6 +81,19 @@ namespace ipa {\n>   * includes setting fields and flags that enable those processing blocks.\n>   */\n>  \n> +/**\n> + * \\fn Algorithm::queueRequest()\n> + * \\brief Provide control values to the algorithm\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] frame The frame number to aplly the control values\n\ns/aplly/apply/\n\n> + * \\param[in] controls The list of user controls\n> + *\n> + * This function provides controls values coming from the application to the\n> + * algorithm. A frame number is provided to indicate the concerned frame.\n> + * Each algorithm is responsible for retrieving the controls associated to\n> + * them.\n\nLet's also mention when this is called:\n\n * This function is called for each request queued to the camera. It provides\n * the controls stored in the request to the algorithm. The \\a frame number\n * identifies the corresponding frame.\n *\n * Algorithms shall read the applicable controls and store their value for later\n * use during frame processing.\n\n> + */\n> +\n>  /**\n>   * \\fn Algorithm::process()\n>   * \\brief Process ISP statistics, and run algorithm operations\n> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> index 2a8871d8..aa846625 100644\n> --- a/src/ipa/libipa/algorithm.h\n> +++ b/src/ipa/libipa/algorithm.h\n> @@ -40,6 +40,12 @@ public:\n>  \t{\n>  \t}\n>  \n> +\tvirtual void queueRequest([[maybe_unused]] typename Module::Context &context,\n> +\t\t\t\t  [[maybe_unused]] const uint32_t frame,\n> +\t\t\t\t  [[maybe_unused]] const typename Module::ControlList &controls)\n> +\t{\n> +\t}\n> +\n>  \tvirtual void process([[maybe_unused]] typename Module::Context &context,\n>  \t\t\t     [[maybe_unused]] typename Module::FrameContext *frameContext,\n>  \t\t\t     [[maybe_unused]] const typename Module::Stats *stats)\n> diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp\n> index 77352104..e87a52fc 100644\n> --- a/src/ipa/libipa/module.cpp\n> +++ b/src/ipa/libipa/module.cpp\n> @@ -77,6 +77,11 @@ namespace ipa {\n>   * \\brief The type of the IPA statistics and ISP results\n>   */\n>  \n> +/**\n> + * \\typedef Module::ControlList\n> + * \\brief The type of the ISP runtime controls list\n> + */\n> +\n>  /**\n>   * \\fn Module::algorithms()\n>   * \\brief Retrieve the list of instantiated algorithms\n> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h\n> index 4149a353..81d3bf7f 100644\n> --- a/src/ipa/libipa/module.h\n> +++ b/src/ipa/libipa/module.h\n> @@ -26,7 +26,7 @@ LOG_DECLARE_CATEGORY(IPAModuleAlgo)\n>  namespace ipa {\n>  \n>  template<typename _Context, typename _FrameContext, typename _Config,\n> -\t typename _Params, typename _Stats>\n> +\t typename _Params, typename _Stats, typename _ControlList>\n\nI don't expect usage of a different container than\nlibcamera::ControlList for controls, so this can be hardcoded. No need\nto change the template parameters to the Module class, just use\nControlList directly in queueRequest() instead of Module::ControlList.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  class Module : public Loggable\n>  {\n>  public:\n> @@ -35,6 +35,7 @@ public:\n>  \tusing Config = _Config;\n>  \tusing Params = _Params;\n>  \tusing Stats = _Stats;\n> +\tusing ControlList = _ControlList;\n>  \n>  \tvirtual ~Module() {}\n>  \n> diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h\n> index 89f83208..08381a08 100644\n> --- a/src/ipa/rkisp1/module.h\n> +++ b/src/ipa/rkisp1/module.h\n> @@ -20,7 +20,7 @@ namespace libcamera {\n>  namespace ipa::rkisp1 {\n>  \n>  using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,\n> -\t\t\t   rkisp1_params_cfg, rkisp1_stat_buffer>;\n> +\t\t\t   rkisp1_params_cfg, rkisp1_stat_buffer, ControlList>;\n>  \n>  } /* namespace ipa::rkisp1 */\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 52C1EBE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Jul 2022 01:10:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94FCE6330F;\n\tWed, 13 Jul 2022 03:10:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 065FA6048B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Jul 2022 03:10:14 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 567D5305;\n\tWed, 13 Jul 2022 03:10:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657674615;\n\tbh=zNLOJnK+JG4XRJgWku6M4vEiVIZ3pr04TSsEVS7/LYg=;\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=mvMOGuffTF2dUK2ZCAYX3KCzNTu2Tz4lmGgl75eWGKevKL/9bQ+w3+aIa8lBxectr\n\tcWbEwLHx+ZkFiRgFFkRyLg1jzkxsjj703oJLsQJwTRTtIFpMSc/c4a4aqG93bMvCoT\n\tdFrDBKYR4Y+6ALYqnH0Gop/A2VztXfUKwuc85YzwwT+Pa4+sPbmEnXE1ORt8hfgWIE\n\tLBlBb484sfRCrri/h/keE8+LiyfuqWlJbzHQZmH17s5AaJGIuJP8gs/tgJ8grLUNdc\n\tZA1Dxr3Cb1zXzvFX1+E4wtFxGEsWMBYt2vIIwQClonhJJVehK/AU8rhPoAg17zrgw6\n\tpPaVQ0tKxGXDw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657674613;\n\tbh=zNLOJnK+JG4XRJgWku6M4vEiVIZ3pr04TSsEVS7/LYg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UgM8+qbhb7DPC+5QgqMRpGyuA+wdRyx+o3d9taSPjv694v4mYndqIuJ6rNKrIN1nb\n\tWudEWRU+6yJMQg1egGmzjq4pM0fUjJbvSwDG0+qMHqSwKu6bmlb6VWOwL9ACSlSVyw\n\tRU418oTRPPnvpbibv0PdKjrH2KQPEFdCvEEElpH0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UgM8+qbh\"; dkim-atps=neutral","Date":"Wed, 13 Jul 2022 04:09:44 +0300","To":"Florian Sylvestre <fsylvestre@baylibre.com>","Message-ID":"<Ys4bWPdgMpWLb30B@pendragon.ideasonboard.com>","References":"<20220704152318.221213-1-fsylvestre@baylibre.com>\n\t<20220704152318.221213-2-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220704152318.221213-2-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","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":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@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":23920,"web_url":"https://patchwork.libcamera.org/comment/23920/","msgid":"<165787551955.1177852.8415241702441765352@Monstersaurus>","date":"2022-07-15T08:58:39","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-07-13 02:09:44)\n> Hi Florian,\n> \n> Thank you for the patch.\n> \n> On Mon, Jul 04, 2022 at 05:23:15PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> > Add queueRequest() function to the Algorithm class. The queueRequest() function\n> > provides controls values coming from the application to each algorithm.\n> > Each algorithm is responsible for retrieving the controls associated to them.\n\nFantastic - just what I had in mind, and was envisaging!\n\n> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> > ---\n> >  src/ipa/ipu3/module.h        |  2 +-\n> >  src/ipa/libipa/algorithm.cpp | 13 +++++++++++++\n> >  src/ipa/libipa/algorithm.h   |  6 ++++++\n> >  src/ipa/libipa/module.cpp    |  5 +++++\n> >  src/ipa/libipa/module.h      |  3 ++-\n> >  src/ipa/rkisp1/module.h      |  2 +-\n> >  6 files changed, 28 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h\n> > index d94fc459..5c2179e0 100644\n> > --- a/src/ipa/ipu3/module.h\n> > +++ b/src/ipa/ipu3/module.h\n> > @@ -20,7 +20,7 @@ namespace libcamera {\n> >  namespace ipa::ipu3 {\n> >  \n> >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPAConfigInfo,\n> > -                        ipu3_uapi_params, ipu3_uapi_stats_3a>;\n> > +                        ipu3_uapi_params, ipu3_uapi_stats_3a, ControlList>;\n\nI'm not sure that we need to define ControlList here ?\n\n> >  \n> >  } /* namespace ipa::ipu3 */\n> >  \n> > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > index 8549fe3f..1fb811ef 100644\n> > --- a/src/ipa/libipa/algorithm.cpp\n> > +++ b/src/ipa/libipa/algorithm.cpp\n> > @@ -81,6 +81,19 @@ namespace ipa {\n> >   * includes setting fields and flags that enable those processing blocks.\n> >   */\n> >  \n> > +/**\n> > + * \\fn Algorithm::queueRequest()\n> > + * \\brief Provide control values to the algorithm\n> > + * \\param[in] context The shared IPA context\n> > + * \\param[in] frame The frame number to aplly the control values\n> \n> s/aplly/apply/\n> \n> > + * \\param[in] controls The list of user controls\n> > + *\n> > + * This function provides controls values coming from the application to the\n> > + * algorithm. A frame number is provided to indicate the concerned frame.\n> > + * Each algorithm is responsible for retrieving the controls associated to\n> > + * them.\n> \n> Let's also mention when this is called:\n> \n>  * This function is called for each request queued to the camera. It provides\n>  * the controls stored in the request to the algorithm. The \\a frame number\n>  * identifies the corresponding frame.\n\nTechnically - it identifies the request sequence that the control list\nwas queued in.\n\nWe are working to infer that the request sequence represents the desired\ncorresponding frame number - but that is not fully settled throughout\nyet, so I would probably prefer to see:\n\n* This function is called for each request queued to the camera. It\n* provides the controls stored in the request to the algorithm. The \\a\n* frame number is the Request sequence number and identifies the desired\n* corresponding frame to target for the controls to take effect.\n\n\nBut perhaps that gets 'too' wordy...\n\n>  *\n>  * Algorithms shall read the applicable controls and store their value for later\n>  * use during frame processing.\n> \n> > + */\n> > +\n> >  /**\n> >   * \\fn Algorithm::process()\n> >   * \\brief Process ISP statistics, and run algorithm operations\n> > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> > index 2a8871d8..aa846625 100644\n> > --- a/src/ipa/libipa/algorithm.h\n> > +++ b/src/ipa/libipa/algorithm.h\n> > @@ -40,6 +40,12 @@ public:\n> >       {\n> >       }\n> >  \n> > +     virtual void queueRequest([[maybe_unused]] typename Module::Context &context,\n> > +                               [[maybe_unused]] const uint32_t frame,\n\nTo match the 'current' implementation of process, this should pass the\nframeContext for the corresponding frame number.\n\n                               [[maybe_unused]] typename Module::FrameContext *frameContext,\n\nAny algorithm should store actionable state in that frameContext for\nwhen it actually is processing that frame.\n\n\n> > +                               [[maybe_unused]] const typename Module::ControlList &controls)\n> > +     {\n> > +     }\n> > +\n> >       virtual void process([[maybe_unused]] typename Module::Context &context,\n> >                            [[maybe_unused]] typename Module::FrameContext *frameContext,\n> >                            [[maybe_unused]] const typename Module::Stats *stats)\n> > diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp\n> > index 77352104..e87a52fc 100644\n> > --- a/src/ipa/libipa/module.cpp\n> > +++ b/src/ipa/libipa/module.cpp\n> > @@ -77,6 +77,11 @@ namespace ipa {\n> >   * \\brief The type of the IPA statistics and ISP results\n> >   */\n> >  \n> > +/**\n> > + * \\typedef Module::ControlList\n> > + * \\brief The type of the ISP runtime controls list\n> > + */\n> > +\n> >  /**\n> >   * \\fn Module::algorithms()\n> >   * \\brief Retrieve the list of instantiated algorithms\n> > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h\n> > index 4149a353..81d3bf7f 100644\n> > --- a/src/ipa/libipa/module.h\n> > +++ b/src/ipa/libipa/module.h\n> > @@ -26,7 +26,7 @@ LOG_DECLARE_CATEGORY(IPAModuleAlgo)\n> >  namespace ipa {\n> >  \n> >  template<typename _Context, typename _FrameContext, typename _Config,\n> > -      typename _Params, typename _Stats>\n> > +      typename _Params, typename _Stats, typename _ControlList>\n> \n> I don't expect usage of a different container than\n> libcamera::ControlList for controls, so this can be hardcoded. No need\n> to change the template parameters to the Module class, just use\n> ControlList directly in queueRequest() instead of Module::ControlList.\n\nAh yes - agreed here. It should just be a ControlList.\n\nReally keen to see this move forwards, with the above topics considered:\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> >  class Module : public Loggable\n> >  {\n> >  public:\n> > @@ -35,6 +35,7 @@ public:\n> >       using Config = _Config;\n> >       using Params = _Params;\n> >       using Stats = _Stats;\n> > +     using ControlList = _ControlList;\n> >  \n> >       virtual ~Module() {}\n> >  \n> > diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h\n> > index 89f83208..08381a08 100644\n> > --- a/src/ipa/rkisp1/module.h\n> > +++ b/src/ipa/rkisp1/module.h\n> > @@ -20,7 +20,7 @@ namespace libcamera {\n> >  namespace ipa::rkisp1 {\n> >  \n> >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,\n> > -                        rkisp1_params_cfg, rkisp1_stat_buffer>;\n> > +                        rkisp1_params_cfg, rkisp1_stat_buffer, ControlList>;\n> >  \n> >  } /* namespace ipa::rkisp1 */\n> >  \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 B2051BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Jul 2022 08:58:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2076C63312;\n\tFri, 15 Jul 2022 10:58:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 030BD6330B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Jul 2022 10:58:42 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69556993;\n\tFri, 15 Jul 2022 10:58:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657875524;\n\tbh=N+bILkFtgaFxbfuXml0Sx1p63ztJp5qzWWFxvhfK+6M=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=MOXnLgIsEuOArgFQvdO6Y/kfHYao9lvKn4/sktsdv2v8wRt7VsPkziyU+3vf6IK6m\n\tpITmt7a5orotry1+peTGYrDYuDFFwWQag+3JQpCuEq2CnEEKPbWHBO3DiN8a/6M1oc\n\tHq6VcFnK3SnGkthzIAmJUxtcTxVv5gBautHoyvNMyOedJnzXzvKrVavnMhwQYZUzfH\n\t0t85s4hmIeQcXTFWZn1PHYoVvVQDw0nKyHiA8LACc2m5Mzq83CNXAsTzGKS9QZda4i\n\tapDWAYquetLMJ1O9v5WFeTcwA8ava9AJICe5EefJupgd3czINg0ACHwX2CHY8nMMts\n\tnJ+QsOmRwv6kQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657875522;\n\tbh=N+bILkFtgaFxbfuXml0Sx1p63ztJp5qzWWFxvhfK+6M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JfWlLmJD2aQdeQdIGxBGcZ+4s2REhFpZa63pr/5kcA7cKVnDTb/00t6T/U33k3Jue\n\tc8F0GX/9Jc9o0P3LWnk92t+SSh3BFKhOoej600Ao76Y6EODJ2bVMFp1pZH930oBbyq\n\tQzRFZcUrhtGAriql80wTaLEUZTQ5Fd5BiXd74bes="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JfWlLmJD\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<Ys4bWPdgMpWLb30B@pendragon.ideasonboard.com>","References":"<20220704152318.221213-1-fsylvestre@baylibre.com>\n\t<20220704152318.221213-2-fsylvestre@baylibre.com>\n\t<Ys4bWPdgMpWLb30B@pendragon.ideasonboard.com>","To":"Florian Sylvestre <fsylvestre@baylibre.com>,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tLaurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Date":"Fri, 15 Jul 2022 09:58:39 +0100","Message-ID":"<165787551955.1177852.8415241702441765352@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","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":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@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":23923,"web_url":"https://patchwork.libcamera.org/comment/23923/","msgid":"<YtE5mtcjKVYbD+AN@pendragon.ideasonboard.com>","date":"2022-07-15T09:55:38","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Jul 15, 2022 at 09:58:39AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-07-13 02:09:44)\n> > On Mon, Jul 04, 2022 at 05:23:15PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> > > Add queueRequest() function to the Algorithm class. The queueRequest() function\n> > > provides controls values coming from the application to each algorithm.\n> > > Each algorithm is responsible for retrieving the controls associated to them.\n> \n> Fantastic - just what I had in mind, and was envisaging!\n> \n> > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> > > ---\n> > >  src/ipa/ipu3/module.h        |  2 +-\n> > >  src/ipa/libipa/algorithm.cpp | 13 +++++++++++++\n> > >  src/ipa/libipa/algorithm.h   |  6 ++++++\n> > >  src/ipa/libipa/module.cpp    |  5 +++++\n> > >  src/ipa/libipa/module.h      |  3 ++-\n> > >  src/ipa/rkisp1/module.h      |  2 +-\n> > >  6 files changed, 28 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h\n> > > index d94fc459..5c2179e0 100644\n> > > --- a/src/ipa/ipu3/module.h\n> > > +++ b/src/ipa/ipu3/module.h\n> > > @@ -20,7 +20,7 @@ namespace libcamera {\n> > >  namespace ipa::ipu3 {\n> > >  \n> > >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPAConfigInfo,\n> > > -                        ipu3_uapi_params, ipu3_uapi_stats_3a>;\n> > > +                        ipu3_uapi_params, ipu3_uapi_stats_3a, ControlList>;\n> \n> I'm not sure that we need to define ControlList here ?\n\nNo we don't, as you've noticed I've commented on that below.\n\n> > >  \n> > >  } /* namespace ipa::ipu3 */\n> > >  \n> > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > > index 8549fe3f..1fb811ef 100644\n> > > --- a/src/ipa/libipa/algorithm.cpp\n> > > +++ b/src/ipa/libipa/algorithm.cpp\n> > > @@ -81,6 +81,19 @@ namespace ipa {\n> > >   * includes setting fields and flags that enable those processing blocks.\n> > >   */\n> > >  \n> > > +/**\n> > > + * \\fn Algorithm::queueRequest()\n> > > + * \\brief Provide control values to the algorithm\n> > > + * \\param[in] context The shared IPA context\n> > > + * \\param[in] frame The frame number to aplly the control values\n> > \n> > s/aplly/apply/\n> > \n> > > + * \\param[in] controls The list of user controls\n> > > + *\n> > > + * This function provides controls values coming from the application to the\n> > > + * algorithm. A frame number is provided to indicate the concerned frame.\n> > > + * Each algorithm is responsible for retrieving the controls associated to\n> > > + * them.\n> > \n> > Let's also mention when this is called:\n> > \n> >  * This function is called for each request queued to the camera. It provides\n> >  * the controls stored in the request to the algorithm. The \\a frame number\n> >  * identifies the corresponding frame.\n> \n> Technically - it identifies the request sequence that the control list\n> was queued in.\n> \n> We are working to infer that the request sequence represents the desired\n> corresponding frame number - but that is not fully settled throughout\n> yet, so I would probably prefer to see:\n> \n> * This function is called for each request queued to the camera. It\n> * provides the controls stored in the request to the algorithm. The \\a\n> * frame number is the Request sequence number and identifies the desired\n> * corresponding frame to target for the controls to take effect.\n> \n> But perhaps that gets 'too' wordy...\n\nI'm fine with that. The reason I didn't take an extra step (or a few\nactually) is that we're still hamering down the details of per-frame\ncontrol, so whatever we write here will be reworded anyway :-)\n\n> >  *\n> >  * Algorithms shall read the applicable controls and store their value for later\n> >  * use during frame processing.\n> > \n> > > + */\n> > > +\n> > >  /**\n> > >   * \\fn Algorithm::process()\n> > >   * \\brief Process ISP statistics, and run algorithm operations\n> > > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> > > index 2a8871d8..aa846625 100644\n> > > --- a/src/ipa/libipa/algorithm.h\n> > > +++ b/src/ipa/libipa/algorithm.h\n> > > @@ -40,6 +40,12 @@ public:\n> > >       {\n> > >       }\n> > >  \n> > > +     virtual void queueRequest([[maybe_unused]] typename Module::Context &context,\n> > > +                               [[maybe_unused]] const uint32_t frame,\n> \n> To match the 'current' implementation of process, this should pass the\n> frameContext for the corresponding frame number.\n> \n>                                [[maybe_unused]] typename Module::FrameContext *frameContext,\n> \n> Any algorithm should store actionable state in that frameContext for\n> when it actually is processing that frame.\n\nGood point, let's do that already.\n\n> > > +                               [[maybe_unused]] const typename Module::ControlList &controls)\n> > > +     {\n> > > +     }\n> > > +\n> > >       virtual void process([[maybe_unused]] typename Module::Context &context,\n> > >                            [[maybe_unused]] typename Module::FrameContext *frameContext,\n> > >                            [[maybe_unused]] const typename Module::Stats *stats)\n> > > diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp\n> > > index 77352104..e87a52fc 100644\n> > > --- a/src/ipa/libipa/module.cpp\n> > > +++ b/src/ipa/libipa/module.cpp\n> > > @@ -77,6 +77,11 @@ namespace ipa {\n> > >   * \\brief The type of the IPA statistics and ISP results\n> > >   */\n> > >  \n> > > +/**\n> > > + * \\typedef Module::ControlList\n> > > + * \\brief The type of the ISP runtime controls list\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\fn Module::algorithms()\n> > >   * \\brief Retrieve the list of instantiated algorithms\n> > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h\n> > > index 4149a353..81d3bf7f 100644\n> > > --- a/src/ipa/libipa/module.h\n> > > +++ b/src/ipa/libipa/module.h\n> > > @@ -26,7 +26,7 @@ LOG_DECLARE_CATEGORY(IPAModuleAlgo)\n> > >  namespace ipa {\n> > >  \n> > >  template<typename _Context, typename _FrameContext, typename _Config,\n> > > -      typename _Params, typename _Stats>\n> > > +      typename _Params, typename _Stats, typename _ControlList>\n> > \n> > I don't expect usage of a different container than\n> > libcamera::ControlList for controls, so this can be hardcoded. No need\n> > to change the template parameters to the Module class, just use\n> > ControlList directly in queueRequest() instead of Module::ControlList.\n> \n> Ah yes - agreed here. It should just be a ControlList.\n> \n> Really keen to see this move forwards, with the above topics considered:\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > \n> > >  class Module : public Loggable\n> > >  {\n> > >  public:\n> > > @@ -35,6 +35,7 @@ public:\n> > >       using Config = _Config;\n> > >       using Params = _Params;\n> > >       using Stats = _Stats;\n> > > +     using ControlList = _ControlList;\n> > >  \n> > >       virtual ~Module() {}\n> > >  \n> > > diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h\n> > > index 89f83208..08381a08 100644\n> > > --- a/src/ipa/rkisp1/module.h\n> > > +++ b/src/ipa/rkisp1/module.h\n> > > @@ -20,7 +20,7 @@ namespace libcamera {\n> > >  namespace ipa::rkisp1 {\n> > >  \n> > >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,\n> > > -                        rkisp1_params_cfg, rkisp1_stat_buffer>;\n> > > +                        rkisp1_params_cfg, rkisp1_stat_buffer, ControlList>;\n> > >  \n> > >  } /* namespace ipa::rkisp1 */\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 C30BEBE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Jul 2022 09:56:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0257463312;\n\tFri, 15 Jul 2022 11:56:12 +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 97FC36330B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Jul 2022 11:56:10 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E0CA6993;\n\tFri, 15 Jul 2022 11:56:09 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657878972;\n\tbh=N9d0rfGSm8FvK2MKGMXD2QNPzfXdSGg/3C1Duwgm+4M=;\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=dmajMEChMeJZcdPB6DUOjVnM6uqAR4+BbdilTGRYAxGk8qIfSMhMfhU7iPMcUqtz+\n\tJilvsaOTDrLFK8mxYjdkygwMZ5eNSbmAD9O3Nk52l8y0EjQWGNTdgPbrfbUKKJ/inp\n\tCCtmNsAL2iAr+rw77FPv65fDrtU/o1Z+mTHvG/pn22sCQHGWA29beTDDm5sE1n9OGV\n\tonFFPL6WqEBBRQXlI+LRNgllac5TPaH0+1cnmjJF1tI77Kkco+va8b6jKFPU4p4qst\n\tIT+UhxEqVJNRIuHl4ZxNQdqZLw7G2HWVKUXaTzLwyleZM52KtcBQOqveQc+Pl+ZgVx\n\tedDQyFVVZkLtA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657878970;\n\tbh=N9d0rfGSm8FvK2MKGMXD2QNPzfXdSGg/3C1Duwgm+4M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VMBZvow3pACLLfgtYNFto8Kb1IKquy7l3c06zdIXer3g+pE/qrkofXma4YtODCkw5\n\toGkiZ07JhaSRj+mUn8PmpMXZlJyB8EIFmp5tioodVdLLcqTOibzpvjcyB4JcSLjWla\n\tttRRDKd3iWFYKCgnTpAXU3u/8QwVxVaJ3M6zslhU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VMBZvow3\"; dkim-atps=neutral","Date":"Fri, 15 Jul 2022 12:55:38 +0300","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YtE5mtcjKVYbD+AN@pendragon.ideasonboard.com>","References":"<20220704152318.221213-1-fsylvestre@baylibre.com>\n\t<20220704152318.221213-2-fsylvestre@baylibre.com>\n\t<Ys4bWPdgMpWLb30B@pendragon.ideasonboard.com>\n\t<165787551955.1177852.8415241702441765352@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165787551955.1177852.8415241702441765352@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","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":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":24009,"web_url":"https://patchwork.libcamera.org/comment/24009/","msgid":"<CALzBHU7tpPDozBA=Rx+Ok4rFo+TW=EDz8Ta+RA8iuVd=wi16oQ@mail.gmail.com>","date":"2022-07-20T15:21:11","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","submitter":{"id":123,"url":"https://patchwork.libcamera.org/api/people/123/","name":"Florian Sylvestre","email":"fsylvestre@baylibre.com"},"content":"Hello Kieran,\n\nOn Fri, 15 Jul 2022 at 11:56, Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Kieran,\n>\n> On Fri, Jul 15, 2022 at 09:58:39AM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart via libcamera-devel (2022-07-13 02:09:44)\n> > > On Mon, Jul 04, 2022 at 05:23:15PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> > > > Add queueRequest() function to the Algorithm class. The queueRequest() function\n> > > > provides controls values coming from the application to each algorithm.\n> > > > Each algorithm is responsible for retrieving the controls associated to them.\n> >\n> > Fantastic - just what I had in mind, and was envisaging!\n> >\n> > > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> > > > ---\n> > > >  src/ipa/ipu3/module.h        |  2 +-\n> > > >  src/ipa/libipa/algorithm.cpp | 13 +++++++++++++\n> > > >  src/ipa/libipa/algorithm.h   |  6 ++++++\n> > > >  src/ipa/libipa/module.cpp    |  5 +++++\n> > > >  src/ipa/libipa/module.h      |  3 ++-\n> > > >  src/ipa/rkisp1/module.h      |  2 +-\n> > > >  6 files changed, 28 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h\n> > > > index d94fc459..5c2179e0 100644\n> > > > --- a/src/ipa/ipu3/module.h\n> > > > +++ b/src/ipa/ipu3/module.h\n> > > > @@ -20,7 +20,7 @@ namespace libcamera {\n> > > >  namespace ipa::ipu3 {\n> > > >\n> > > >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPAConfigInfo,\n> > > > -                        ipu3_uapi_params, ipu3_uapi_stats_3a>;\n> > > > +                        ipu3_uapi_params, ipu3_uapi_stats_3a, ControlList>;\n> >\n> > I'm not sure that we need to define ControlList here ?\n>\n> No we don't, as you've noticed I've commented on that below.\n>\n> > > >\n> > > >  } /* namespace ipa::ipu3 */\n> > > >\n> > > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > > > index 8549fe3f..1fb811ef 100644\n> > > > --- a/src/ipa/libipa/algorithm.cpp\n> > > > +++ b/src/ipa/libipa/algorithm.cpp\n> > > > @@ -81,6 +81,19 @@ namespace ipa {\n> > > >   * includes setting fields and flags that enable those processing blocks.\n> > > >   */\n> > > >\n> > > > +/**\n> > > > + * \\fn Algorithm::queueRequest()\n> > > > + * \\brief Provide control values to the algorithm\n> > > > + * \\param[in] context The shared IPA context\n> > > > + * \\param[in] frame The frame number to aplly the control values\n> > >\n> > > s/aplly/apply/\n> > >\n> > > > + * \\param[in] controls The list of user controls\n> > > > + *\n> > > > + * This function provides controls values coming from the application to the\n> > > > + * algorithm. A frame number is provided to indicate the concerned frame.\n> > > > + * Each algorithm is responsible for retrieving the controls associated to\n> > > > + * them.\n> > >\n> > > Let's also mention when this is called:\n> > >\n> > >  * This function is called for each request queued to the camera. It provides\n> > >  * the controls stored in the request to the algorithm. The \\a frame number\n> > >  * identifies the corresponding frame.\n> >\n> > Technically - it identifies the request sequence that the control list\n> > was queued in.\n> >\n> > We are working to infer that the request sequence represents the desired\n> > corresponding frame number - but that is not fully settled throughout\n> > yet, so I would probably prefer to see:\n> >\n> > * This function is called for each request queued to the camera. It\n> > * provides the controls stored in the request to the algorithm. The \\a\n> > * frame number is the Request sequence number and identifies the desired\n> > * corresponding frame to target for the controls to take effect.\n> >\n> > But perhaps that gets 'too' wordy...\n>\n> I'm fine with that. The reason I didn't take an extra step (or a few\n> actually) is that we're still hamering down the details of per-frame\n> control, so whatever we write here will be reworded anyway :-)\n>\n> > >  *\n> > >  * Algorithms shall read the applicable controls and store their value for later\n> > >  * use during frame processing.\n> > >\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\fn Algorithm::process()\n> > > >   * \\brief Process ISP statistics, and run algorithm operations\n> > > > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> > > > index 2a8871d8..aa846625 100644\n> > > > --- a/src/ipa/libipa/algorithm.h\n> > > > +++ b/src/ipa/libipa/algorithm.h\n> > > > @@ -40,6 +40,12 @@ public:\n> > > >       {\n> > > >       }\n> > > >\n> > > > +     virtual void queueRequest([[maybe_unused]] typename Module::Context &context,\n> > > > +                               [[maybe_unused]] const uint32_t frame,\n> >\n> > To match the 'current' implementation of process, this should pass the\n> > frameContext for the corresponding frame number.\nAs it's not currently available (at least on rkisp) I am using the current\nframeContext.\nI think that when the development will be done to manage several frameContext,\neach one associated with the correct frame number, then we can update this\ninterface if needed. For the moment I have no clue what it will look like...\n> >\n> >                                [[maybe_unused]] typename Module::FrameContext *frameContext,\n> >\n> > Any algorithm should store actionable state in that frameContext for\n> > when it actually is processing that frame.\n>\n> Good point, let's do that already.\n>\n> > > > +                               [[maybe_unused]] const typename Module::ControlList &controls)\n> > > > +     {\n> > > > +     }\n> > > > +\n> > > >       virtual void process([[maybe_unused]] typename Module::Context &context,\n> > > >                            [[maybe_unused]] typename Module::FrameContext *frameContext,\n> > > >                            [[maybe_unused]] const typename Module::Stats *stats)\n> > > > diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp\n> > > > index 77352104..e87a52fc 100644\n> > > > --- a/src/ipa/libipa/module.cpp\n> > > > +++ b/src/ipa/libipa/module.cpp\n> > > > @@ -77,6 +77,11 @@ namespace ipa {\n> > > >   * \\brief The type of the IPA statistics and ISP results\n> > > >   */\n> > > >\n> > > > +/**\n> > > > + * \\typedef Module::ControlList\n> > > > + * \\brief The type of the ISP runtime controls list\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\fn Module::algorithms()\n> > > >   * \\brief Retrieve the list of instantiated algorithms\n> > > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h\n> > > > index 4149a353..81d3bf7f 100644\n> > > > --- a/src/ipa/libipa/module.h\n> > > > +++ b/src/ipa/libipa/module.h\n> > > > @@ -26,7 +26,7 @@ LOG_DECLARE_CATEGORY(IPAModuleAlgo)\n> > > >  namespace ipa {\n> > > >\n> > > >  template<typename _Context, typename _FrameContext, typename _Config,\n> > > > -      typename _Params, typename _Stats>\n> > > > +      typename _Params, typename _Stats, typename _ControlList>\n> > >\n> > > I don't expect usage of a different container than\n> > > libcamera::ControlList for controls, so this can be hardcoded. No need\n> > > to change the template parameters to the Module class, just use\n> > > ControlList directly in queueRequest() instead of Module::ControlList.\n> >\n> > Ah yes - agreed here. It should just be a ControlList.\n> >\n> > Really keen to see this move forwards, with the above topics considered:\n> >\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >\n> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > >\n> > > >  class Module : public Loggable\n> > > >  {\n> > > >  public:\n> > > > @@ -35,6 +35,7 @@ public:\n> > > >       using Config = _Config;\n> > > >       using Params = _Params;\n> > > >       using Stats = _Stats;\n> > > > +     using ControlList = _ControlList;\n> > > >\n> > > >       virtual ~Module() {}\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h\n> > > > index 89f83208..08381a08 100644\n> > > > --- a/src/ipa/rkisp1/module.h\n> > > > +++ b/src/ipa/rkisp1/module.h\n> > > > @@ -20,7 +20,7 @@ namespace libcamera {\n> > > >  namespace ipa::rkisp1 {\n> > > >\n> > > >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,\n> > > > -                        rkisp1_params_cfg, rkisp1_stat_buffer>;\n> > > > +                        rkisp1_params_cfg, rkisp1_stat_buffer, ControlList>;\n> > > >\n> > > >  } /* namespace ipa::rkisp1 */\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\nI will send a v2 of this patch series with all comments addressed (I hope) but\nwith keeping the frameContext associated with the current frame (and not the\ntarget one).\nLet's discuss the new version if needed.\n\nRegards,","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 07F99BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 20 Jul 2022 15:21:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5053463312;\n\tWed, 20 Jul 2022 17:21:26 +0200 (CEST)","from mail-pg1-x531.google.com (mail-pg1-x531.google.com\n\t[IPv6:2607:f8b0:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9B0860489\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 20 Jul 2022 17:21:24 +0200 (CEST)","by mail-pg1-x531.google.com with SMTP id 72so16700612pge.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 20 Jul 2022 08:21:24 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658330486;\n\tbh=xMqEc1eY49VMmX0WUwoCXo+7W0ckh0g0x+Ie98/rSyo=;\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=ybfWPixeEgiI8lYNzkupmClwpl9t47rLQL9/+U4s6CWHsctF5GPQbwsmo7KV1kvIc\n\tXDBUflGZmn1GFBlhST8WeNvHCPb6aVVTmqtrwI/y/R3SRqeC0y+5ENvBjYEo26//RM\n\t5aqU0bbCSCSVWsgkqtWEU0tXFiZ14EvKI6HFQG8l12RBk1ED6VFXMAeyD3XgT7QS2A\n\tZbTBfUhFeJDgmRnFQxwoexpKxVsH01xoCLrEds5hDyDtQlaHcXrTDE8gFAITTGpC7v\n\tW3SmV2f7TGRXkG1R4RGVyw1ID1oQF4pBkrv26fmf62e21yQDoGp/B+HBwfh+zVY+wa\n\t9gFyC6p4P1U7A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ex4CQ4mPyRw97VeUugAaSrQk4LzsG35RooaS6aUi+0o=;\n\tb=MSsUMcOErS+FQRDMbuvEGJdNVMYLTX2IP/Xv01C4n0vS3QeCerkfJu8tsHpu+ntZzR\n\t0tItEBugRes0un4IkdcaznMftVeBAFsjaeJXnM/crd3RLn8ESzmJskMWwx7Srvcj8LIp\n\twvc9H4HP+/RxZrBZoXqd+LaSOCXZ3MinUOGkbFwNLO26IrF0Lf3QNbb4j4cN6tYeFoLY\n\tIS8T2XVkrua+dZbufhNlWTGazIbeOJ0kO1jS7/7exu3xByAcJjLpvHJF71A87v1fhgog\n\tyYcIA5uTsJQ1mCs6vciutlPlr4Nt7CwQOoaeBOuIe9VKx4r8235/KDVsbo4lDsOqiTbq\n\t1NBQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"MSsUMcOE\"; \n\tdkim-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=ex4CQ4mPyRw97VeUugAaSrQk4LzsG35RooaS6aUi+0o=;\n\tb=2hTAEChESqwCRrxDJvpfkwdykayFJIfIkzCG58Ix+Vn3+i8woTF/Ra1dwL17DhJOdQ\n\tki3AzjnHmCOv/s5E7yb7TWKECnlGt2ZgiDhhNaOQXXnZpzVDZs2Y0GAWYwPhOaDk9LYT\n\ta/HkM0PXTclm0f8RAe2jMPyPE0wMccXGJLipKfBWwtVofv2emUqrN3XgzIm32fK97WPg\n\tPyhw8xiLvMEkkHuA7HHvBC9UXGzKU2gaVyA6ttBEYRdDjWejRd0ZPcKjfvZF+Bo83AaB\n\tmOXlE5RYaHX2mWDiuMb0fSeNurQg5trSRj9t6m/OoZ6hpDo6eWJNx5D1X5JL9wvYeFuN\n\t1iMA==","X-Gm-Message-State":"AJIora/Xhga1aDUFQ13/cGPELbF3OoKQXwwYEHbXMa8Q+EZz3lJejGxi\n\tZeE0/dvr8mN2/V+lkDfmzcIhGd8Nmg9th1OcALYbHOQnwO79rg==","X-Google-Smtp-Source":"AGRyM1v9ahHJZ8aIofuuH9fUgdnPETf87Zq7GwxdqA5v8LNmCXqVUI5+YrIs+aZ5a2DxnszjeU7trhPFFXfScYRUgck=","X-Received":"by 2002:a63:e051:0:b0:41a:23dd:471b with SMTP id\n\tn17-20020a63e051000000b0041a23dd471bmr14213733pgj.289.1658330482642;\n\tWed, 20 Jul 2022 08:21:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20220704152318.221213-1-fsylvestre@baylibre.com>\n\t<20220704152318.221213-2-fsylvestre@baylibre.com>\n\t<Ys4bWPdgMpWLb30B@pendragon.ideasonboard.com>\n\t<165787551955.1177852.8415241702441765352@Monstersaurus>\n\t<YtE5mtcjKVYbD+AN@pendragon.ideasonboard.com>","In-Reply-To":"<YtE5mtcjKVYbD+AN@pendragon.ideasonboard.com>","Date":"Wed, 20 Jul 2022 17:21:11 +0200","Message-ID":"<CALzBHU7tpPDozBA=Rx+Ok4rFo+TW=EDz8Ta+RA8iuVd=wi16oQ@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add\n\tqueueRequest() to the Algorithm class","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":"Florian Sylvestre via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Florian Sylvestre <fsylvestre@baylibre.com>","Cc":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]