[{"id":22501,"web_url":"https://patchwork.libcamera.org/comment/22501/","msgid":"<efcafaa2-0719-4add-a079-539ae4e1d138@ideasonboard.com>","date":"2022-03-29T05:55:24","subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nThanks for the patch !\n\nOn 25/03/2022 10:25, Kieran Bingham wrote:\n> A selection of constants are imported from ChromiumOS.\n> \n> Move these out of the header, and simplify their documentation.  Further\n> more, add a direct reference to the location they were obtained from.\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nReviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n\n> ---\n>   src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------\n>   src/ipa/ipu3/algorithms/af.h   | 11 -----\n>   2 files changed, 22 insertions(+), 62 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> index 0170a3728892..634d0f2e3176 100644\n> --- a/src/ipa/ipu3/algorithms/af.cpp\n> +++ b/src/ipa/ipu3/algorithms/af.cpp\n> @@ -29,59 +29,30 @@\n>    * \\file af.h\n>    */\n>   \n> -/**\n> - * \\var kAfMinGridWidth\n> - * \\brief the minimum width of AF grid.\n> - * The minimum grid horizontal dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMinGridHeight\n> - * \\brief the minimum height of AF grid.\n> - * The minimum grid vertical dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMaxGridWidth\n> - * \\brief the maximum width of AF grid.\n> - * The maximum grid horizontal dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMaxGridHeight\n> - * \\brief The maximum height of AF grid.\n> - * The maximum grid vertical dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMinGridBlockWidth\n> - * \\brief The minimum block size of the width.\n> - * The minimum value of Log2 of the width of the grid cell.\n> - */\n> -\n> -/**\n> - * \\var kAfMinGridBlockHeight\n> - * \\brief The minimum block size of the height.\n> - * The minimum value of Log2 of the height of the grid cell.\n> - */\n> -\n> -/**\n> - * \\def kAfMaxGridBlockWidth\n> - * \\brief The maximum block size of the width.\n> - * The maximum value of Log2 of the width of the grid cell.\n> - */\n> -\n> -/**\n> - * \\var kAfMaxGridBlockHeight\n> - * \\brief The maximum block size of the height.\n> - * The maximum value of Log2 of the height of the grid cell.\n> +/*\n> + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:\n> + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h\n> + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h\n>    */\n>   \n> -/**\n> - * \\var kAfDefaultHeightPerSlice\n> - * \\brief The default number of blocks in vertical axis per slice.\n> - * The number of blocks in vertical axis per slice.\n> - */\n> +/** The minimum horizontal grid dimension. */\n> +static constexpr uint8_t kAfMinGridWidth = 16;\n> +/** The minimum vertical grid dimension. */\n> +static constexpr uint8_t kAfMinGridHeight = 16;\n> +/** The maximum horizontal grid dimension. */\n> +static constexpr uint8_t kAfMaxGridWidth = 32;\n> +/** The maximum vertical grid dimension. */\n> +static constexpr uint8_t kAfMaxGridHeight = 24;\n> +/** The minimum value of Log2 of the width of the grid cell. */\n> +static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> +/** The minimum value of Log2 of the height of the grid cell. */\n> +static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> +/** The maximum value of Log2 of the width of the grid cell. */\n> +static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> +/** The maximum value of Log2 of the height of the grid cell. */\n> +static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> +/** The number of blocks in vertical axis per slice. */\n> +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n>   \n>   namespace libcamera {\n>   \n> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h\n> index 13c7e0e877fd..906f2843dd49 100644\n> --- a/src/ipa/ipu3/algorithms/af.h\n> +++ b/src/ipa/ipu3/algorithms/af.h\n> @@ -15,17 +15,6 @@\n>   \n>   #include \"algorithm.h\"\n>   \n> -/* Static variables from repo of chromium */\n> -static constexpr uint8_t kAfMinGridWidth = 16;\n> -static constexpr uint8_t kAfMinGridHeight = 16;\n> -static constexpr uint8_t kAfMaxGridWidth = 32;\n> -static constexpr uint8_t kAfMaxGridHeight = 24;\n> -static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> -static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> -static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> -static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n> -\n>   namespace libcamera {\n>   \n>   namespace ipa::ipu3::algorithms {","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 AA77CC0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 29 Mar 2022 05:55:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FE61604BC;\n\tTue, 29 Mar 2022 07:55:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E2ACA604BC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Mar 2022 07:55:27 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:46c7:900e:3d98:76e7] (unknown\n\t[IPv6:2a01:e0a:169:7140:46c7:900e:3d98:76e7])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9596E888;\n\tTue, 29 Mar 2022 07:55:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648533329;\n\tbh=trIx0sYMCvBHcc2a8OTIRHvpwgVRzRFIG6nBh4F0p+g=;\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:\n\tFrom;\n\tb=z0DLm/JdUsmh9oz3o9ycOxdP0uQPhmHrTv4AQuvfZHkxg0i7gmfhrkTvNj6UieYuI\n\tlK3Aa5eU45bOd5o2tmbcqW6fuEd9kU8CieDVFCcTgXzqCHAYAI5iZiFMg7Oa3k8Qxq\n\tBBZZ/2hR5X9RJtRo8JSKF2Vo7Y6OQa18tLVU5BKnb9IrWcqpewwePkkmQb11hxPPZT\n\t/f4qtr1nRqdtSuHliVxUYDe5CkeTUKC/jZ1OVb7iAo7LRo6fNhnVpGnxMssa8Wl6G1\n\ttvNzG73lskkN3/mR171YVZcn930OKfG/C7fm2l/GBtsYKrv6DZY/duAJz2C5qrzF2W\n\t4cF5juVlrehqQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648533327;\n\tbh=trIx0sYMCvBHcc2a8OTIRHvpwgVRzRFIG6nBh4F0p+g=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=LK7ReTgc1bQnLx4qWHSj5V3df7LEBPx+WVt9NcJ4lG3idqmbwPNp3Fd2oHWWZEHIR\n\t7MCRM4WLln+j/T9Ws6+SCkHNoujGYzrZlavKzYMGcFnl2wZ8tD3+J1T15BkyjogJh1\n\t0Rf8Kqxn7W+HsgjzZpBlQuKlDt19FJLreMi9iI/k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LK7ReTgc\"; dkim-atps=neutral","Message-ID":"<efcafaa2-0719-4add-a079-539ae4e1d138@ideasonboard.com>","Date":"Tue, 29 Mar 2022 07:55:24 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-US","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera devel <libcamera-devel@lists.libcamera.org>,\n\tKate Hsuan <hpa@redhat.com>","References":"<20220325092555.1799897-1-kieran.bingham@ideasonboard.com>\n\t<20220325092555.1799897-2-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20220325092555.1799897-2-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","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":"Jean-Michel Hautbois via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22509,"web_url":"https://patchwork.libcamera.org/comment/22509/","msgid":"<164854907877.15275.16698732505572505419@Monstersaurus>","date":"2022-03-29T10:17:58","subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kieran\n\nQuoting Kieran Bingham (2022-03-25 09:25:51)\n> A selection of constants are imported from ChromiumOS.\n> \n> Move these out of the header, and simplify their documentation.  Further\n> more, add a direct reference to the location they were obtained from.\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------\n>  src/ipa/ipu3/algorithms/af.h   | 11 -----\n>  2 files changed, 22 insertions(+), 62 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> index 0170a3728892..634d0f2e3176 100644\n> --- a/src/ipa/ipu3/algorithms/af.cpp\n> +++ b/src/ipa/ipu3/algorithms/af.cpp\n> @@ -29,59 +29,30 @@\n>   * \\file af.h\n>   */\n>  \n> -/**\n> - * \\var kAfMinGridWidth\n> - * \\brief the minimum width of AF grid.\n> - * The minimum grid horizontal dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMinGridHeight\n> - * \\brief the minimum height of AF grid.\n> - * The minimum grid vertical dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMaxGridWidth\n> - * \\brief the maximum width of AF grid.\n> - * The maximum grid horizontal dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMaxGridHeight\n> - * \\brief The maximum height of AF grid.\n> - * The maximum grid vertical dimensions.\n> - */\n> -\n> -/**\n> - * \\var kAfMinGridBlockWidth\n> - * \\brief The minimum block size of the width.\n> - * The minimum value of Log2 of the width of the grid cell.\n> - */\n> -\n> -/**\n> - * \\var kAfMinGridBlockHeight\n> - * \\brief The minimum block size of the height.\n> - * The minimum value of Log2 of the height of the grid cell.\n> - */\n> -\n> -/**\n> - * \\def kAfMaxGridBlockWidth\n> - * \\brief The maximum block size of the width.\n> - * The maximum value of Log2 of the width of the grid cell.\n> - */\n> -\n> -/**\n> - * \\var kAfMaxGridBlockHeight\n> - * \\brief The maximum block size of the height.\n> - * The maximum value of Log2 of the height of the grid cell.\n> +/*\n> + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:\n> + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h\n> + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h\n>   */\n>  \n> -/**\n> - * \\var kAfDefaultHeightPerSlice\n> - * \\brief The default number of blocks in vertical axis per slice.\n> - * The number of blocks in vertical axis per slice.\n> - */\n> +/** The minimum horizontal grid dimension. */\n> +static constexpr uint8_t kAfMinGridWidth = 16;\n> +/** The minimum vertical grid dimension. */\n> +static constexpr uint8_t kAfMinGridHeight = 16;\n> +/** The maximum horizontal grid dimension. */\n> +static constexpr uint8_t kAfMaxGridWidth = 32;\n> +/** The maximum vertical grid dimension. */\n> +static constexpr uint8_t kAfMaxGridHeight = 24;\n> +/** The minimum value of Log2 of the width of the grid cell. */\n> +static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> +/** The minimum value of Log2 of the height of the grid cell. */\n> +static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> +/** The maximum value of Log2 of the width of the grid cell. */\n> +static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> +/** The maximum value of Log2 of the height of the grid cell. */\n> +static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n\nIt turns out some of these are unused and that breaks Clang-12\ncompilation by default:\n\nclang++-12 -Isrc/ipa/ipu3/ipa_ipu3.so.p -Isrc/ipa/ipu3 -I../../../src/libcamera/src/ipa/ipu3 -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wextra-semi -Wthread-safety -Wshadow -include config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -MF src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o.d -o src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -c ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp\n../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:43:26: error: unused variable 'kAfMaxGridWidth' [-Werror,-Wunused-const-variable]\nstatic constexpr uint8_t kAfMaxGridWidth = 32;\n                         ^\n../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:45:26: error: unused variable 'kAfMaxGridHeight' [-Werror,-Wunused-const-variable]\nstatic constexpr uint8_t kAfMaxGridHeight = 24;\n                         ^\n../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:51:27: error: unused variable 'kAfMaxGridBlockWidth' [-Werror,-Wunused-const-variable]\nstatic constexpr uint16_t kAfMaxGridBlockWidth = 6;\n                          ^\n../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:53:27: error: unused variable 'kAfMaxGridBlockHeight' [-Werror,-Wunused-const-variable]\nstatic constexpr uint16_t kAfMaxGridBlockHeight = 6;\n                          ^\n4 errors generated.\n\n\nKate/JM - \n\nDo you think we should keep these either commented out or in some form\nof documentation?\n\nOr should we add an assert that makes use of them so that they can be\nkept?\n\nOr should we just drop the unused constants altogether?\n\n--\nKieran\n\n\n> +/** The number of blocks in vertical axis per slice. */\n> +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n>  \n>  namespace libcamera {\n>  \n> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h\n> index 13c7e0e877fd..906f2843dd49 100644\n> --- a/src/ipa/ipu3/algorithms/af.h\n> +++ b/src/ipa/ipu3/algorithms/af.h\n> @@ -15,17 +15,6 @@\n>  \n>  #include \"algorithm.h\"\n>  \n> -/* Static variables from repo of chromium */\n> -static constexpr uint8_t kAfMinGridWidth = 16;\n> -static constexpr uint8_t kAfMinGridHeight = 16;\n> -static constexpr uint8_t kAfMaxGridWidth = 32;\n> -static constexpr uint8_t kAfMaxGridHeight = 24;\n> -static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> -static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> -static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> -static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n> -\n>  namespace libcamera {\n>  \n>  namespace ipa::ipu3::algorithms {\n> -- \n> 2.32.0\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 31C35C3256\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 29 Mar 2022 10:18:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF190601F8;\n\tTue, 29 Mar 2022 12:18:03 +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 B62A160135\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Mar 2022 12:18:01 +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 70378299;\n\tTue, 29 Mar 2022 12:18:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648549083;\n\tbh=DMgtS2rBYaRKXkVYOjEJNNppyyc6u/lgf07Lh0aAlh8=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=3YoXO9gsuXnsBgJhE3GFrq2VE0QwQdXbGP4Ef9LJhi5/l+ekufbbsRKnhBYhk/WxK\n\tv90TtUKsv/MTPWi2FFIVsxcoali3JRjTumXa40mHz5Uacl3rzlco+racS1wt+bErk6\n\tGjB1BXGL46ciBU+Vyd6guEsZtUiUDlFVhsa64fDwu7m1v3qKkyEHdV7uLdhl1Opq7G\n\tfgHGtKHHqLclOm9dZoRq4OcnkVHm5kBkXrF6DvIwwtzVmkALP04cT0mDVycNEMnePv\n\t0pZl7QXFYs8WdxvHfkH2lgZlZDsDdjYA/QY3qIZ4MWc98OysrJuuBp4XeAPScgv1Q/\n\tFM5p+fVGiMkkQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648549081;\n\tbh=DMgtS2rBYaRKXkVYOjEJNNppyyc6u/lgf07Lh0aAlh8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hbUNnRoiBko6DPDKRP9KhMuwlHraiy/eFuaPNZra6yhCzMk3mHsEBAAOpBIgGHi64\n\thEGCrCfK0eg2ATBpAJN1GkNwy+4i7zNfzQc8btOnPZD9j0qWTm02n2b6qOVeEn1p3A\n\tHob/BBlRsla0l2OECqIjmb8rQr1zRgzXtN3b52lc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hbUNnRoi\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220325092555.1799897-2-kieran.bingham@ideasonboard.com>","References":"<20220325092555.1799897-1-kieran.bingham@ideasonboard.com>\n\t<20220325092555.1799897-2-kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tKate Hsuan <hpa@redhat.com>,\n\tlibcamera devel <libcamera-devel@lists.libcamera.org>","Date":"Tue, 29 Mar 2022 11:17:58 +0100","Message-ID":"<164854907877.15275.16698732505572505419@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22510,"web_url":"https://patchwork.libcamera.org/comment/22510/","msgid":"<YkLfYyTYsYBbzcJe@pendragon.ideasonboard.com>","date":"2022-03-29T10:28:51","subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Mar 29, 2022 at 11:17:58AM +0100, Kieran Bingham wrote:\n> Hi Kieran\n> \n> Quoting Kieran Bingham (2022-03-25 09:25:51)\n> > A selection of constants are imported from ChromiumOS.\n> > \n> > Move these out of the header, and simplify their documentation.  Further\n> > more, add a direct reference to the location they were obtained from.\n> > \n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > ---\n> >  src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------\n> >  src/ipa/ipu3/algorithms/af.h   | 11 -----\n> >  2 files changed, 22 insertions(+), 62 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> > index 0170a3728892..634d0f2e3176 100644\n> > --- a/src/ipa/ipu3/algorithms/af.cpp\n> > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > @@ -29,59 +29,30 @@\n> >   * \\file af.h\n> >   */\n> >  \n> > -/**\n> > - * \\var kAfMinGridWidth\n> > - * \\brief the minimum width of AF grid.\n> > - * The minimum grid horizontal dimensions.\n> > - */\n> > -\n> > -/**\n> > - * \\var kAfMinGridHeight\n> > - * \\brief the minimum height of AF grid.\n> > - * The minimum grid vertical dimensions.\n> > - */\n> > -\n> > -/**\n> > - * \\var kAfMaxGridWidth\n> > - * \\brief the maximum width of AF grid.\n> > - * The maximum grid horizontal dimensions.\n> > - */\n> > -\n> > -/**\n> > - * \\var kAfMaxGridHeight\n> > - * \\brief The maximum height of AF grid.\n> > - * The maximum grid vertical dimensions.\n> > - */\n> > -\n> > -/**\n> > - * \\var kAfMinGridBlockWidth\n> > - * \\brief The minimum block size of the width.\n> > - * The minimum value of Log2 of the width of the grid cell.\n> > - */\n> > -\n> > -/**\n> > - * \\var kAfMinGridBlockHeight\n> > - * \\brief The minimum block size of the height.\n> > - * The minimum value of Log2 of the height of the grid cell.\n> > - */\n> > -\n> > -/**\n> > - * \\def kAfMaxGridBlockWidth\n> > - * \\brief The maximum block size of the width.\n> > - * The maximum value of Log2 of the width of the grid cell.\n> > - */\n> > -\n> > -/**\n> > - * \\var kAfMaxGridBlockHeight\n> > - * \\brief The maximum block size of the height.\n> > - * The maximum value of Log2 of the height of the grid cell.\n> > +/*\n> > + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:\n> > + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h\n> > + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h\n> >   */\n> >  \n> > -/**\n> > - * \\var kAfDefaultHeightPerSlice\n> > - * \\brief The default number of blocks in vertical axis per slice.\n> > - * The number of blocks in vertical axis per slice.\n> > - */\n> > +/** The minimum horizontal grid dimension. */\n> > +static constexpr uint8_t kAfMinGridWidth = 16;\n> > +/** The minimum vertical grid dimension. */\n> > +static constexpr uint8_t kAfMinGridHeight = 16;\n> > +/** The maximum horizontal grid dimension. */\n> > +static constexpr uint8_t kAfMaxGridWidth = 32;\n> > +/** The maximum vertical grid dimension. */\n> > +static constexpr uint8_t kAfMaxGridHeight = 24;\n> > +/** The minimum value of Log2 of the width of the grid cell. */\n> > +static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> > +/** The minimum value of Log2 of the height of the grid cell. */\n> > +static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> > +/** The maximum value of Log2 of the width of the grid cell. */\n> > +static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> > +/** The maximum value of Log2 of the height of the grid cell. */\n> > +static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> \n> It turns out some of these are unused and that breaks Clang-12\n> compilation by default:\n> \n> clang++-12 -Isrc/ipa/ipu3/ipa_ipu3.so.p -Isrc/ipa/ipu3 -I../../../src/libcamera/src/ipa/ipu3 -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wextra-semi -Wthread-safety -Wshadow -include config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -MF src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o.d -o src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -c ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp\n> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:43:26: error: unused variable 'kAfMaxGridWidth' [-Werror,-Wunused-const-variable]\n> static constexpr uint8_t kAfMaxGridWidth = 32;\n>                          ^\n> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:45:26: error: unused variable 'kAfMaxGridHeight' [-Werror,-Wunused-const-variable]\n> static constexpr uint8_t kAfMaxGridHeight = 24;\n>                          ^\n> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:51:27: error: unused variable 'kAfMaxGridBlockWidth' [-Werror,-Wunused-const-variable]\n> static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n>                           ^\n> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:53:27: error: unused variable 'kAfMaxGridBlockHeight' [-Werror,-Wunused-const-variable]\n> static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n>                           ^\n> 4 errors generated.\n> \n> \n> Kate/JM - \n> \n> Do you think we should keep these either commented out or in some form\n> of documentation?\n> \n> Or should we add an assert that makes use of them so that they can be\n> kept?\n> \n> Or should we just drop the unused constants altogether?\n\nf you want to keep them, I'd use them in Af::configure() to clamp the\ngrid and block size values. It's quite pointless as the values are set\nto the minimum, so clamping to [min, max] will not do anything useful,\nbut it can prepare for dynamic calculation of the grid parameters.\n\n> > +/** The number of blocks in vertical axis per slice. */\n> > +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n> >  \n> >  namespace libcamera {\n> >  \n> > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h\n> > index 13c7e0e877fd..906f2843dd49 100644\n> > --- a/src/ipa/ipu3/algorithms/af.h\n> > +++ b/src/ipa/ipu3/algorithms/af.h\n> > @@ -15,17 +15,6 @@\n> >  \n> >  #include \"algorithm.h\"\n> >  \n> > -/* Static variables from repo of chromium */\n> > -static constexpr uint8_t kAfMinGridWidth = 16;\n> > -static constexpr uint8_t kAfMinGridHeight = 16;\n> > -static constexpr uint8_t kAfMaxGridWidth = 32;\n> > -static constexpr uint8_t kAfMaxGridHeight = 24;\n> > -static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> > -static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> > -static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> > -static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> > -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n> > -\n> >  namespace libcamera {\n> >  \n> >  namespace ipa::ipu3::algorithms {","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 C5A96C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 29 Mar 2022 10:28:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CA9C65631;\n\tTue, 29 Mar 2022 12:28:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04C0960135\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Mar 2022 12:28:55 +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 4E900299;\n\tTue, 29 Mar 2022 12:28:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648549736;\n\tbh=E0I77YFVkMEuil91o7GJV1qV/KxddKaYEw5UbypZS6c=;\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=KejzFt6XnYC1tf/ep9a6/vgQOiUdPNfufpxbXO73bRWld2m6zlS7iUD8HrXrq9xZG\n\tOAVPx25y9LbcWa3T2lCiY1w/jEKgrwzefz/DLZR2D/Tal3HU0z31cpCjEJ1BnzUUvp\n\tlcM5fETbOWjTj5NtDA2Xy9mMa0IY1HezRsjd8nJIFgXxmobJ/LZ1hRJCSnzqtOT96b\n\tq5s9UseGcW9gNG4iQbrbaIo3Xro554zq866npYJhbbeDTXHaQyaLoXCPJZiVmodGp1\n\tghD5YaBp2AcmJx4RAbEE99lmgFHJOQOBsxGvshv6t7bQtllDk4pdCgoR/GhXZpzBWf\n\tx47OAVlOVlhsg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648549734;\n\tbh=E0I77YFVkMEuil91o7GJV1qV/KxddKaYEw5UbypZS6c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TFKPClrnf4OqspfGKcSVoPgeqkE0jJJLfv5JmkPKj2ufxsJmqLdsOqOYn6CjpweEV\n\tka6Ft7S1pzzVWrs1hRICdES2i8wdr6LIm3Zrhgcjctfp7FDWhMBus2Aun2/iw20GUN\n\t6k1vBncpV0p2G2e1RaNvESljv7VfraNg8s6gSFvw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TFKPClrn\"; dkim-atps=neutral","Date":"Tue, 29 Mar 2022 13:28:51 +0300","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YkLfYyTYsYBbzcJe@pendragon.ideasonboard.com>","References":"<20220325092555.1799897-1-kieran.bingham@ideasonboard.com>\n\t<20220325092555.1799897-2-kieran.bingham@ideasonboard.com>\n\t<164854907877.15275.16698732505572505419@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164854907877.15275.16698732505572505419@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","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 <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22511,"web_url":"https://patchwork.libcamera.org/comment/22511/","msgid":"<CAEth8oFt1JeQvpHqg8f7Rz+wniaT-Nda2y0SVSQFeuhht3_Xmw@mail.gmail.com>","date":"2022-03-29T11:23:45","subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","submitter":{"id":105,"url":"https://patchwork.libcamera.org/api/people/105/","name":"Kate Hsuan","email":"hpa@redhat.com"},"content":"Hi Laurent and Kieran,\n\nOn Tue, Mar 29, 2022 at 6:28 PM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> On Tue, Mar 29, 2022 at 11:17:58AM +0100, Kieran Bingham wrote:\n> > Hi Kieran\n> >\n> > Quoting Kieran Bingham (2022-03-25 09:25:51)\n> > > A selection of constants are imported from ChromiumOS.\n> > >\n> > > Move these out of the header, and simplify their documentation.  Further\n> > > more, add a direct reference to the location they were obtained from.\n> > >\n> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > ---\n> > >  src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------\n> > >  src/ipa/ipu3/algorithms/af.h   | 11 -----\n> > >  2 files changed, 22 insertions(+), 62 deletions(-)\n> > >\n> > > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> > > index 0170a3728892..634d0f2e3176 100644\n> > > --- a/src/ipa/ipu3/algorithms/af.cpp\n> > > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > > @@ -29,59 +29,30 @@\n> > >   * \\file af.h\n> > >   */\n> > >\n> > > -/**\n> > > - * \\var kAfMinGridWidth\n> > > - * \\brief the minimum width of AF grid.\n> > > - * The minimum grid horizontal dimensions.\n> > > - */\n> > > -\n> > > -/**\n> > > - * \\var kAfMinGridHeight\n> > > - * \\brief the minimum height of AF grid.\n> > > - * The minimum grid vertical dimensions.\n> > > - */\n> > > -\n> > > -/**\n> > > - * \\var kAfMaxGridWidth\n> > > - * \\brief the maximum width of AF grid.\n> > > - * The maximum grid horizontal dimensions.\n> > > - */\n> > > -\n> > > -/**\n> > > - * \\var kAfMaxGridHeight\n> > > - * \\brief The maximum height of AF grid.\n> > > - * The maximum grid vertical dimensions.\n> > > - */\n> > > -\n> > > -/**\n> > > - * \\var kAfMinGridBlockWidth\n> > > - * \\brief The minimum block size of the width.\n> > > - * The minimum value of Log2 of the width of the grid cell.\n> > > - */\n> > > -\n> > > -/**\n> > > - * \\var kAfMinGridBlockHeight\n> > > - * \\brief The minimum block size of the height.\n> > > - * The minimum value of Log2 of the height of the grid cell.\n> > > - */\n> > > -\n> > > -/**\n> > > - * \\def kAfMaxGridBlockWidth\n> > > - * \\brief The maximum block size of the width.\n> > > - * The maximum value of Log2 of the width of the grid cell.\n> > > - */\n> > > -\n> > > -/**\n> > > - * \\var kAfMaxGridBlockHeight\n> > > - * \\brief The maximum block size of the height.\n> > > - * The maximum value of Log2 of the height of the grid cell.\n> > > +/*\n> > > + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:\n> > > + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h\n> > > + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h\n> > >   */\n> > >\n> > > -/**\n> > > - * \\var kAfDefaultHeightPerSlice\n> > > - * \\brief The default number of blocks in vertical axis per slice.\n> > > - * The number of blocks in vertical axis per slice.\n> > > - */\n> > > +/** The minimum horizontal grid dimension. */\n> > > +static constexpr uint8_t kAfMinGridWidth = 16;\n> > > +/** The minimum vertical grid dimension. */\n> > > +static constexpr uint8_t kAfMinGridHeight = 16;\n> > > +/** The maximum horizontal grid dimension. */\n> > > +static constexpr uint8_t kAfMaxGridWidth = 32;\n> > > +/** The maximum vertical grid dimension. */\n> > > +static constexpr uint8_t kAfMaxGridHeight = 24;\n> > > +/** The minimum value of Log2 of the width of the grid cell. */\n> > > +static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> > > +/** The minimum value of Log2 of the height of the grid cell. */\n> > > +static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> > > +/** The maximum value of Log2 of the width of the grid cell. */\n> > > +static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> > > +/** The maximum value of Log2 of the height of the grid cell. */\n> > > +static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> >\n> > It turns out some of these are unused and that breaks Clang-12\n> > compilation by default:\n> >\n> > clang++-12 -Isrc/ipa/ipu3/ipa_ipu3.so.p -Isrc/ipa/ipu3 -I../../../src/libcamera/src/ipa/ipu3 -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wextra-semi -Wthread-safety -Wshadow -include config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -MF src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o.d -o src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -c ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp\n> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:43:26: error: unused variable 'kAfMaxGridWidth' [-Werror,-Wunused-const-variable]\n> > static constexpr uint8_t kAfMaxGridWidth = 32;\n> >                          ^\n> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:45:26: error: unused variable 'kAfMaxGridHeight' [-Werror,-Wunused-const-variable]\n> > static constexpr uint8_t kAfMaxGridHeight = 24;\n> >                          ^\n> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:51:27: error: unused variable 'kAfMaxGridBlockWidth' [-Werror,-Wunused-const-variable]\n> > static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> >                           ^\n> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:53:27: error: unused variable 'kAfMaxGridBlockHeight' [-Werror,-Wunused-const-variable]\n> > static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> >                           ^\n> > 4 errors generated.\n> >\n> >\n> > Kate/JM -\n> >\n> > Do you think we should keep these either commented out or in some form\n> > of documentation?\n> >\n> > Or should we add an assert that makes use of them so that they can be\n> > kept?\n> >\n> > Or should we just drop the unused constants altogether?\n>\n> f you want to keep them, I'd use them in Af::configure() to clamp the\n> grid and block size values. It's quite pointless as the values are set\n> to the minimum, so clamping to [min, max] will not do anything useful,\n> but it can prepare for dynamic calculation of the grid parameters.\n\nI would prefer to keep them for the dynamic grid configuration since\nsome of the camera can dynamically configure the size and coordinate\nof AF grid.\nAgree with Laurent, clamp can be used to perform additional checks to\navoid some incorrect configurations.\n\nThank you.\n\n>\n> > > +/** The number of blocks in vertical axis per slice. */\n> > > +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n> > >\n> > >  namespace libcamera {\n> > >\n> > > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h\n> > > index 13c7e0e877fd..906f2843dd49 100644\n> > > --- a/src/ipa/ipu3/algorithms/af.h\n> > > +++ b/src/ipa/ipu3/algorithms/af.h\n> > > @@ -15,17 +15,6 @@\n> > >\n> > >  #include \"algorithm.h\"\n> > >\n> > > -/* Static variables from repo of chromium */\n> > > -static constexpr uint8_t kAfMinGridWidth = 16;\n> > > -static constexpr uint8_t kAfMinGridHeight = 16;\n> > > -static constexpr uint8_t kAfMaxGridWidth = 32;\n> > > -static constexpr uint8_t kAfMaxGridHeight = 24;\n> > > -static constexpr uint16_t kAfMinGridBlockWidth = 4;\n> > > -static constexpr uint16_t kAfMinGridBlockHeight = 3;\n> > > -static constexpr uint16_t kAfMaxGridBlockWidth = 6;\n> > > -static constexpr uint16_t kAfMaxGridBlockHeight = 6;\n> > > -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;\n> > > -\n> > >  namespace libcamera {\n> > >\n> > >  namespace ipa::ipu3::algorithms {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 E513FC3256\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 29 Mar 2022 11:24:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 471C5604C5;\n\tTue, 29 Mar 2022 13:24:04 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B907660135\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Mar 2022 13:24:02 +0200 (CEST)","from mail-lf1-f70.google.com (mail-lf1-f70.google.com\n\t[209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-282-uhONru7ZM3ioxvngAST1dw-1; Tue, 29 Mar 2022 07:23:59 -0400","by mail-lf1-f70.google.com with SMTP id\n\td41-20020a0565123d2900b0044a10c21f39so1672898lfv.22\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 29 Mar 2022 04:23:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648553044;\n\tbh=r7LX7017jFDPIg62tMXiaQRrRsAcUO1RQqPHJxgw8hE=;\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=lYPkxrJQCmzmCKFwgNuvw3hx2tfOIwHX3ANzMEpSHGhMS5gLjjsxpEDyYAYnq0S9a\n\tQJBBmOW3FIDIEc4nHLKfNRbBAgp3jkG9z2j3V1B3epAKdxLVuQQIBciuUaD5gTvCIb\n\tMdfmyg8wSQlTdaoI3MY1GIwD4qne9RKAyxoN7Vabjdd8l/mnNoJroa1bG4dkqJzae8\n\tz3LzhXtOqobpZz4PUmTz0/bfz64CtClQJcTamTA/jsjfIFwK9CXpzpccQK6mZABIpu\n\tZZgxHMc/ogrvTIBhZyv3UNumBmlAtuUYE8HumPQbDmI+bipznuL8DdED6c6zsPNJ7r\n\tsEojSjyOGeI6Q==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1648553041;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=M/Nw8Zgt7tlJNdwui36LIwH2qYRaSLDUej5xY19aCAs=;\n\tb=IVCyMYFKVPxvWXC0hhZBN6p1IkpjeAQ5/nj1bZk02Ru/4Z/O5ZHDZUPo9oBFmQuyFEkj7a\n\tJhn4+0yrMcKrP06bPkR49XgpzQxmxIAwDNFcSFxewrHynAYI9kaEumlwdsrkkysx2rEHjt\n\tjLIv9q/wZZ0JIktwJkbWYgmj9Y3mE4I="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"IVCyMYFK\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=hpa@redhat.com"],"X-MC-Unique":"uhONru7ZM3ioxvngAST1dw-1","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:content-transfer-encoding;\n\tbh=M/Nw8Zgt7tlJNdwui36LIwH2qYRaSLDUej5xY19aCAs=;\n\tb=SvN7nUils10cWEmEBUQkDdN/Ib6p8ra3HGpuKq4j7ZQGZ3tdibwX/mVJ7PbPxDV6r7\n\th7BdisVt7q7oa40Dw8rqJdyNl879YMvryfeTneyVgH3PAD4SV6mm5Ds22wRb2mDd93wo\n\tBWH0T3Zb4nq5raEyg5kEHcMWELHwkrpHXHC6UhRTnI9Wl/chNlo71rd27e75d1PgdqgD\n\tIXqUCosZzhlGBL/o6aYJPB7iAMkywUXnBShfI1YRMCYHQ2n2lYCw9/CVu2dIi1HERXdu\n\t8YdJ5iTALBeZr6hnGQ1VJbS0//qPPSDDnpO8W2JA8fl48+mfH+UqnAHz4i84L3K/a9vU\n\ts5sQ==","X-Gm-Message-State":"AOAM531CtEo48E/0JLHO2ZNWlre1Po897ohl2T05d9UYE0h4X+OZcYRJ\n\tw0kk3rdI39AwznV4vX+UmnZ/HgF6l64WHRiFbCQrsAdXfBlmdjSv84nvic3zT2U3z+kDpCHXHq/\n\t18TcFamTpJtCTlyGbTvnOjjqXwG4o3E5Gr4lC41bUJWe/PCsj/Q==","X-Received":["by 2002:a05:6512:1316:b0:44a:26d1:c72b with SMTP id\n\tx22-20020a056512131600b0044a26d1c72bmr2211335lfu.384.1648553037264; \n\tTue, 29 Mar 2022 04:23:57 -0700 (PDT)","by 2002:a05:6512:1316:b0:44a:26d1:c72b with SMTP id\n\tx22-20020a056512131600b0044a26d1c72bmr2211296lfu.384.1648553036810;\n\tTue, 29 Mar 2022 04:23:56 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJyO0iaOi7ikUnQin+xsw0jnY6wgoEwiBmnFtPQauc65kIRqFqEkpBn+80MFRayVntTQSlH2B7pby4ks/znAs5Q=","MIME-Version":"1.0","References":"<20220325092555.1799897-1-kieran.bingham@ideasonboard.com>\n\t<20220325092555.1799897-2-kieran.bingham@ideasonboard.com>\n\t<164854907877.15275.16698732505572505419@Monstersaurus>\n\t<YkLfYyTYsYBbzcJe@pendragon.ideasonboard.com>","In-Reply-To":"<YkLfYyTYsYBbzcJe@pendragon.ideasonboard.com>","Date":"Tue, 29 Mar 2022 19:23:45 +0800","Message-ID":"<CAEth8oFt1JeQvpHqg8f7Rz+wniaT-Nda2y0SVSQFeuhht3_Xmw@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants\n\tto implementation","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":"Kate Hsuan via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Kate Hsuan <hpa@redhat.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]