[{"id":15430,"web_url":"https://patchwork.libcamera.org/comment/15430/","msgid":"","date":"2021-03-02T20:02:53","subject":"Re: [libcamera-devel] [PATCH v2 3/3] ipa: rkisp1: update the uapi\n\tto the latest kernel version","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nThank you for the patch.\n\nOn Thu, Feb 25, 2021 at 06:12:05PM +0100, Dafna Hirschfeld wrote:\n> In kernel 5.11 the rkisp1 uapi had changed to support\n> different hardware versions. This patch updates the\n> uapi header and adds support for hardware version 10 only.\n> In the future, support for other version will be added.\n\nWe split changes to the Linux headers in separate patches, with clear\nreferences to the upstream version. git log in include/linux/ should\nshow you how that's usually handled. We also update all headers in one\ngo, to ensure they're consistent with a single kernel version.\n\nI need v5.12-rc1 for a different reason, I'll post the patch, feel free\nto rebase this series on top. I'll also switch from\nRKISP1_CIF_ISP_AE_MEAN_MAX to RKISP1_CIF_ISP_AE_MEAN_MAX_V10 in a\nseparate patch (otherwise the header change will break the rkisp1).\n\n> Signed-off-by: Dafna Hirschfeld \n> Reviewed-by: Paul Elder \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> include/linux/rkisp1-config.h | 88 +++++++++++++++++++++---\n> src/ipa/rkisp1/rkisp1.cpp | 16 +++--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +-\n> 4 files changed, 92 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index 95fa0d93..b097fc8f 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -29,7 +29,7 @@ interface IPARkISP1Interface {\n> \tstart() => (int32 ret);\n> \tstop();\n> \n> -\tconfigure(CameraSensorInfo sensorInfo,\n> +\tconfigure(uint32 hwRevision, CameraSensorInfo sensorInfo,\n> \t\t map streamConfig,\n> \t\t map entityControls) => (int32 ret);\n> \n> diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h\n> index 9c24867d..3bccccbc 100644\n> --- a/include/linux/rkisp1-config.h\n> +++ b/include/linux/rkisp1-config.h\n> @@ -1,4 +1,4 @@\n> -/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */\n> +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */\n> /*\n> * Rockchip ISP1 userspace API\n> * Copyright (C) 2017 Rockchip Electronics Co., Ltd.\n> @@ -53,8 +53,14 @@\n> #define RKISP1_CIF_ISP_CTK_COEFF_MAX 0x100\n> #define RKISP1_CIF_ISP_CTK_OFFSET_MAX 0x800\n> \n> -#define RKISP1_CIF_ISP_AE_MEAN_MAX 25\n> -#define RKISP1_CIF_ISP_HIST_BIN_N_MAX 16\n> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V10\t\t25\n> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V12\t\t81\n> +#define RKISP1_CIF_ISP_AE_MEAN_MAX\t\tRKISP1_CIF_ISP_AE_MEAN_MAX_V12\n> +\n> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10\t16\n> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12\t32\n> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX\t\tRKISP1_CIF_ISP_HIST_BIN_N_MAX_V12\n> +\n> #define RKISP1_CIF_ISP_AFM_MAX_WINDOWS 3\n> #define RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE 17\n> \n> @@ -90,7 +96,9 @@\n> * Gamma out\n> */\n> /* Maximum number of color samples supported */\n> -#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES 17\n> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10 17\n> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12 34\n> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12\n> \n> /*\n> * Lens shade correction\n> @@ -106,8 +114,9 @@\n> /*\n> * Histogram calculation\n> */\n> -/* Last 3 values unused. */\n> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28\n> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 25\n> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81\n> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12\n> \n> /*\n> * Defect Pixel Cluster Correction\n> @@ -128,6 +137,21 @@\n> #define RKISP1_CIF_ISP_STAT_AFM (1U << 2)\n> #define RKISP1_CIF_ISP_STAT_HIST (1U << 3)\n> \n> +/**\n> + * enum rkisp1_cif_isp_version - ISP variants\n> + *\n> + * @RKISP1_V10: used at least in rk3288 and rk3399\n> + * @RKISP1_V11: declared in the original vendor code, but not used\n> + * @RKISP1_V12: used at least in rk3326 and px30\n> + * @RKISP1_V13: used at least in rk1808\n> + */\n> +enum rkisp1_cif_isp_version {\n> +\tRKISP1_V10 = 10,\n> +\tRKISP1_V11,\n> +\tRKISP1_V12,\n> +\tRKISP1_V13,\n> +};\n> +\n> enum rkisp1_cif_isp_histogram_mode {\n> \tRKISP1_CIF_ISP_HISTOGRAM_MODE_DISABLE,\n> \tRKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED,\n> @@ -514,6 +538,15 @@ enum rkisp1_cif_isp_goc_mode {\n> *\n> * @mode: goc mode (from enum rkisp1_cif_isp_goc_mode)\n> * @gamma_y: gamma out curve y-axis for all color components\n> + *\n> + * The number of entries of @gamma_y depends on the hardware revision\n> + * as is reported by the hw_revision field of the struct media_device_info\n> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> + *\n> + * Versions <= V11 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10\n> + * entries, versions >= V12 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12\n> + * entries. RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES is equal to the maximum\n> + * of the two.\n> */\n> struct rkisp1_cif_isp_goc_config {\n> \t__u32 mode;\n> @@ -528,6 +561,15 @@ struct rkisp1_cif_isp_goc_config {\n> *\t\t\t skipped\n> * @meas_window: coordinates of the measure window\n> * @hist_weight: weighting factor for sub-windows\n> + *\n> + * The number of entries of @hist_weight depends on the hardware revision\n> + * as is reported by the hw_revision field of the struct media_device_info\n> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> + *\n> + * Versions <= V11 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10\n> + * entries, versions >= V12 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12\n> + * entries. RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE is equal to the maximum\n> + * of the two.\n> */\n> struct rkisp1_cif_isp_hst_config {\n> \t__u32 mode;\n> @@ -815,7 +857,15 @@ struct rkisp1_cif_isp_bls_meas_val {\n> * @exp_mean: Mean luminance value of block xx\n> * @bls_val: BLS measured values\n> *\n> - * Image is divided into 5x5 blocks.\n> + * The number of entries of @exp_mean depends on the hardware revision\n> + * as is reported by the hw_revision field of the struct media_device_info\n> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> + *\n> + * Versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,\n> + * versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.\n> + * RKISP1_CIF_ISP_AE_MEAN_MAX is equal to the maximum of the two.\n> + *\n> + * Image is divided into 5x5 blocks on V10 and 9x9 blocks on V12.\n> */\n> struct rkisp1_cif_isp_ae_stat {\n> \t__u8 exp_mean[RKISP1_CIF_ISP_AE_MEAN_MAX];\n> @@ -848,13 +898,29 @@ struct rkisp1_cif_isp_af_stat {\n> /**\n> * struct rkisp1_cif_isp_hist_stat - statistics histogram data\n> *\n> - * @hist_bins: measured bin counters\n> + * @hist_bins: measured bin counters. Each bin is a 20 bits unsigned fixed point\n> + *\t type. Bits 0-4 are the fractional part and bits 5-19 are the\n> + *\t integer part.\n> + *\n> + * The window of the measurements area is divided to 5x5 sub-windows for\n> + * V10/V11 and to 9x9 sub-windows for V12. The histogram is then computed for\n> + * each sub-window independently and the final result is a weighted average of\n> + * the histogram measurements on all sub-windows. The window of the\n> + * measurements area and the weight of each sub-window are configurable using\n> + * struct @rkisp1_cif_isp_hst_config.\n> + *\n> + * The histogram contains 16 bins in V10/V11 and 32 bins in V12/V13.\n> + *\n> + * The number of entries of @hist_bins depends on the hardware revision\n> + * as is reported by the hw_revision field of the struct media_device_info\n> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> *\n> - * Measurement window divided into 25 sub-windows, set\n> - * with ISP_HIST_XXX\n> + * Versions <= V11 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10 entries,\n> + * versions >= V12 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12 entries.\n> + * RKISP1_CIF_ISP_HIST_BIN_N_MAX is equal to the maximum of the two.\n> */\n> struct rkisp1_cif_isp_hist_stat {\n> -\t__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];\n> +\t__u32 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];\n> };\n> \n> /**\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 70e8d8f7..39ac8a2b 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -38,7 +38,7 @@ public:\n> \tint start() override { return 0; }\n> \tvoid stop() override {}\n> \n> -\tint configure(const CameraSensorInfo &info,\n> +\tint configure(uint32_t hwRevision, const CameraSensorInfo &info,\n> \t\t const std::map &streamConfig,\n> \t\t const std::map &entityControls) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> @@ -75,10 +75,17 @@ private:\n> * assemble one. Make sure the reported sensor information are relevant\n> * before accessing them.\n> */\n> -int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> +int IPARkISP1::configure(uint32_t hwRevision, [[maybe_unused]] const CameraSensorInfo &info,\n> \t\t\t [[maybe_unused]] const std::map &streamConfig,\n> \t\t\t const std::map &entityControls)\n> {\n> +\t/* \\todo add support for other revisions */\n> +\tif (hwRevision != RKISP1_V10) {\n> +\t\tLOG(IPARkISP1, Error) << \"Hardware version \" << hwRevision << \" is currently\"\n> +\t\t\t\t << \" not supported\";\n\nCould you please wrap this at 80 columns ?\n\n\t\tLOG(IPARkISP1, Error)\n\t\t\t<< \"Hardware version \" << hwRevision\n\t\t\t<< \" is currently not supported\";\n\n> +\t\treturn -ENODEV;\n> +\t}\n\nIt would be nicer to pass the hardware revision to the init() function,\nas it doesn't depend on a particular configuration. I've posted a patch\nthat allow adding more input parameters to init(), you can rebase on top\nof that (a v2 will follow shortly for additional output parameters, but\nyou can ignore that for now).\n\nWould it make sense to add the same check in the match() function of the\npipeline handler ?\n\n> +\n> \tif (entityControls.empty())\n> \t\treturn -EINVAL;\n> \n> @@ -107,7 +114,8 @@ int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> \tgain_ = minGain_;\n> \n> \tLOG(IPARkISP1, Info)\n> -\t\t<< \"Exposure: \" << minExposure_ << \"-\" << maxExposure_\n> +\t\t<< \"Hw Revision: \" << hwRevision\n> +\t\t<< \" Exposure: \" << minExposure_ << \"-\" << maxExposure_\n> \t\t<< \" Gain: \" << minGain_ << \"-\" << maxGain_;\n\nUnelated change ?\n\n> \n> \tsetControls(0);\n> @@ -217,7 +225,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> \n> \t\tunsigned int value = 0;\n> \t\tunsigned int num = 0;\n> -\t\tfor (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX; i++) {\n> +\t\tfor (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {\n> \t\t\tif (ae->exp_mean[i] <= 15)\n> \t\t\t\tcontinue;\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 34814f62..19dc9e1e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -653,7 +653,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tstd::map entityControls;\n> \tentityControls.emplace(0, data->sensor_->controls());\n> \n> -\tret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);\n> +\tret = data->ipa_->configure(media_->hwRevision(), sensorInfo,\n> +\t\t\t\t streamConfig, entityControls);\n> \tif (ret) {\n> \t\tLOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n> \t\treturn ret;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7C386BD1F1\n\tfor ;\n\tTue, 2 Mar 2021 20:03:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A47668A91;\n\tTue, 2 Mar 2021 21:03:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7BC660106\n\tfor ;\n\tTue, 2 Mar 2021 21:03:22 +0100 (CET)","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 4558945D;\n\tTue, 2 Mar 2021 21:03:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aHr2tr5A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614715402;\n\tbh=f5CW9i7Nf0sdWT0PZ1UISAYPF09mAGgxYfKlmluf0z0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aHr2tr5Ac0bBHJ+HlsEEf7vni7r0BUv6l4dCORkjlxpCkhNeVpyhesIxeyl2Y3vsS\n\tAc3pTsKcg3IqY1oVN+CqIqa0Qn43E7F0Gaj+c8uwQZxwwkPLO2Pc3jBULUCYsCAkFs\n\tv10fsLxrPYVmRylOoEDR3Wjgrz2LnIOOJtXXQ1CA=","Date":"Tue, 2 Mar 2021 22:02:53 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210225171205.23341-1-dafna.hirschfeld@collabora.com>\n\t<20210225171205.23341-4-dafna.hirschfeld@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210225171205.23341-4-dafna.hirschfeld@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] ipa: rkisp1: update the uapi\n\tto the latest kernel version","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15449,"web_url":"https://patchwork.libcamera.org/comment/15449/","msgid":"<3d3b2039-ebba-cde2-2208-4048f201cf9e@collabora.com>","date":"2021-03-03T14:49:13","subject":"Re: [libcamera-devel] [PATCH v2 3/3] ipa: rkisp1: update the uapi\n\tto the latest kernel version","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"Hi,\n\nOn 02.03.21 21:02, Laurent Pinchart wrote:\n> Hi Dafna,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 25, 2021 at 06:12:05PM +0100, Dafna Hirschfeld wrote:\n>> In kernel 5.11 the rkisp1 uapi had changed to support\n>> different hardware versions. This patch updates the\n>> uapi header and adds support for hardware version 10 only.\n>> In the future, support for other version will be added.\n> \n> We split changes to the Linux headers in separate patches, with clear\n> references to the upstream version. git log in include/linux/ should\n> show you how that's usually handled. We also update all headers in one\n> go, to ensure they're consistent with a single kernel version.\n> \n> I need v5.12-rc1 for a different reason, I'll post the patch, feel free\n> to rebase this series on top. I'll also switch from\n> RKISP1_CIF_ISP_AE_MEAN_MAX to RKISP1_CIF_ISP_AE_MEAN_MAX_V10 in a\n> separate patch (otherwise the header change will break the rkisp1).\n> \n>> Signed-off-by: Dafna Hirschfeld \n>> Reviewed-by: Paul Elder \n>> ---\n>> include/libcamera/ipa/rkisp1.mojom | 2 +-\n>> include/linux/rkisp1-config.h | 88 +++++++++++++++++++++---\n>> src/ipa/rkisp1/rkisp1.cpp | 16 +++--\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +-\n>> 4 files changed, 92 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n>> index 95fa0d93..b097fc8f 100644\n>> --- a/include/libcamera/ipa/rkisp1.mojom\n>> +++ b/include/libcamera/ipa/rkisp1.mojom\n>> @@ -29,7 +29,7 @@ interface IPARkISP1Interface {\n>> \tstart() => (int32 ret);\n>> \tstop();\n>> \n>> -\tconfigure(CameraSensorInfo sensorInfo,\n>> +\tconfigure(uint32 hwRevision, CameraSensorInfo sensorInfo,\n>> \t\t map streamConfig,\n>> \t\t map entityControls) => (int32 ret);\n>> \n>> diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h\n>> index 9c24867d..3bccccbc 100644\n>> --- a/include/linux/rkisp1-config.h\n>> +++ b/include/linux/rkisp1-config.h\n>> @@ -1,4 +1,4 @@\n>> -/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */\n>> +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */\n>> /*\n>> * Rockchip ISP1 userspace API\n>> * Copyright (C) 2017 Rockchip Electronics Co., Ltd.\n>> @@ -53,8 +53,14 @@\n>> #define RKISP1_CIF_ISP_CTK_COEFF_MAX 0x100\n>> #define RKISP1_CIF_ISP_CTK_OFFSET_MAX 0x800\n>> \n>> -#define RKISP1_CIF_ISP_AE_MEAN_MAX 25\n>> -#define RKISP1_CIF_ISP_HIST_BIN_N_MAX 16\n>> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V10\t\t25\n>> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V12\t\t81\n>> +#define RKISP1_CIF_ISP_AE_MEAN_MAX\t\tRKISP1_CIF_ISP_AE_MEAN_MAX_V12\n>> +\n>> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10\t16\n>> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12\t32\n>> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX\t\tRKISP1_CIF_ISP_HIST_BIN_N_MAX_V12\n>> +\n>> #define RKISP1_CIF_ISP_AFM_MAX_WINDOWS 3\n>> #define RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE 17\n>> \n>> @@ -90,7 +96,9 @@\n>> * Gamma out\n>> */\n>> /* Maximum number of color samples supported */\n>> -#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES 17\n>> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10 17\n>> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12 34\n>> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12\n>> \n>> /*\n>> * Lens shade correction\n>> @@ -106,8 +114,9 @@\n>> /*\n>> * Histogram calculation\n>> */\n>> -/* Last 3 values unused. */\n>> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28\n>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 25\n>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81\n>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12\n>> \n>> /*\n>> * Defect Pixel Cluster Correction\n>> @@ -128,6 +137,21 @@\n>> #define RKISP1_CIF_ISP_STAT_AFM (1U << 2)\n>> #define RKISP1_CIF_ISP_STAT_HIST (1U << 3)\n>> \n>> +/**\n>> + * enum rkisp1_cif_isp_version - ISP variants\n>> + *\n>> + * @RKISP1_V10: used at least in rk3288 and rk3399\n>> + * @RKISP1_V11: declared in the original vendor code, but not used\n>> + * @RKISP1_V12: used at least in rk3326 and px30\n>> + * @RKISP1_V13: used at least in rk1808\n>> + */\n>> +enum rkisp1_cif_isp_version {\n>> +\tRKISP1_V10 = 10,\n>> +\tRKISP1_V11,\n>> +\tRKISP1_V12,\n>> +\tRKISP1_V13,\n>> +};\n>> +\n>> enum rkisp1_cif_isp_histogram_mode {\n>> \tRKISP1_CIF_ISP_HISTOGRAM_MODE_DISABLE,\n>> \tRKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED,\n>> @@ -514,6 +538,15 @@ enum rkisp1_cif_isp_goc_mode {\n>> *\n>> * @mode: goc mode (from enum rkisp1_cif_isp_goc_mode)\n>> * @gamma_y: gamma out curve y-axis for all color components\n>> + *\n>> + * The number of entries of @gamma_y depends on the hardware revision\n>> + * as is reported by the hw_revision field of the struct media_device_info\n>> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n>> + *\n>> + * Versions <= V11 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10\n>> + * entries, versions >= V12 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12\n>> + * entries. RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES is equal to the maximum\n>> + * of the two.\n>> */\n>> struct rkisp1_cif_isp_goc_config {\n>> \t__u32 mode;\n>> @@ -528,6 +561,15 @@ struct rkisp1_cif_isp_goc_config {\n>> *\t\t\t skipped\n>> * @meas_window: coordinates of the measure window\n>> * @hist_weight: weighting factor for sub-windows\n>> + *\n>> + * The number of entries of @hist_weight depends on the hardware revision\n>> + * as is reported by the hw_revision field of the struct media_device_info\n>> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n>> + *\n>> + * Versions <= V11 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10\n>> + * entries, versions >= V12 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12\n>> + * entries. RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE is equal to the maximum\n>> + * of the two.\n>> */\n>> struct rkisp1_cif_isp_hst_config {\n>> \t__u32 mode;\n>> @@ -815,7 +857,15 @@ struct rkisp1_cif_isp_bls_meas_val {\n>> * @exp_mean: Mean luminance value of block xx\n>> * @bls_val: BLS measured values\n>> *\n>> - * Image is divided into 5x5 blocks.\n>> + * The number of entries of @exp_mean depends on the hardware revision\n>> + * as is reported by the hw_revision field of the struct media_device_info\n>> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n>> + *\n>> + * Versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,\n>> + * versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.\n>> + * RKISP1_CIF_ISP_AE_MEAN_MAX is equal to the maximum of the two.\n>> + *\n>> + * Image is divided into 5x5 blocks on V10 and 9x9 blocks on V12.\n>> */\n>> struct rkisp1_cif_isp_ae_stat {\n>> \t__u8 exp_mean[RKISP1_CIF_ISP_AE_MEAN_MAX];\n>> @@ -848,13 +898,29 @@ struct rkisp1_cif_isp_af_stat {\n>> /**\n>> * struct rkisp1_cif_isp_hist_stat - statistics histogram data\n>> *\n>> - * @hist_bins: measured bin counters\n>> + * @hist_bins: measured bin counters. Each bin is a 20 bits unsigned fixed point\n>> + *\t type. Bits 0-4 are the fractional part and bits 5-19 are the\n>> + *\t integer part.\n>> + *\n>> + * The window of the measurements area is divided to 5x5 sub-windows for\n>> + * V10/V11 and to 9x9 sub-windows for V12. The histogram is then computed for\n>> + * each sub-window independently and the final result is a weighted average of\n>> + * the histogram measurements on all sub-windows. The window of the\n>> + * measurements area and the weight of each sub-window are configurable using\n>> + * struct @rkisp1_cif_isp_hst_config.\n>> + *\n>> + * The histogram contains 16 bins in V10/V11 and 32 bins in V12/V13.\n>> + *\n>> + * The number of entries of @hist_bins depends on the hardware revision\n>> + * as is reported by the hw_revision field of the struct media_device_info\n>> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n>> *\n>> - * Measurement window divided into 25 sub-windows, set\n>> - * with ISP_HIST_XXX\n>> + * Versions <= V11 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10 entries,\n>> + * versions >= V12 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12 entries.\n>> + * RKISP1_CIF_ISP_HIST_BIN_N_MAX is equal to the maximum of the two.\n>> */\n>> struct rkisp1_cif_isp_hist_stat {\n>> -\t__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];\n>> +\t__u32 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];\n>> };\n>> \n>> /**\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 70e8d8f7..39ac8a2b 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -38,7 +38,7 @@ public:\n>> \tint start() override { return 0; }\n>> \tvoid stop() override {}\n>> \n>> -\tint configure(const CameraSensorInfo &info,\n>> +\tint configure(uint32_t hwRevision, const CameraSensorInfo &info,\n>> \t\t const std::map &streamConfig,\n>> \t\t const std::map &entityControls) override;\n>> \tvoid mapBuffers(const std::vector &buffers) override;\n>> @@ -75,10 +75,17 @@ private:\n>> * assemble one. Make sure the reported sensor information are relevant\n>> * before accessing them.\n>> */\n>> -int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n>> +int IPARkISP1::configure(uint32_t hwRevision, [[maybe_unused]] const CameraSensorInfo &info,\n>> \t\t\t [[maybe_unused]] const std::map &streamConfig,\n>> \t\t\t const std::map &entityControls)\n>> {\n>> +\t/* \\todo add support for other revisions */\n>> +\tif (hwRevision != RKISP1_V10) {\n>> +\t\tLOG(IPARkISP1, Error) << \"Hardware version \" << hwRevision << \" is currently\"\n>> +\t\t\t\t << \" not supported\";\n> \n> Could you please wrap this at 80 columns ?\n> \n> \t\tLOG(IPARkISP1, Error)\n> \t\t\t<< \"Hardware version \" << hwRevision\n> \t\t\t<< \" is currently not supported\";\n> \n>> +\t\treturn -ENODEV;\n>> +\t}\n> \n> It would be nicer to pass the hardware revision to the init() function,\n> as it doesn't depend on a particular configuration. I've posted a patch\n> that allow adding more input parameters to init(), you can rebase on top\n> of that (a v2 will follow shortly for additional output parameters, but\n> you can ignore that for now).\n> \n> Would it make sense to add the same check in the match() function of the\n> pipeline handler ?\n\nThe hardware revision is only relevant in the IPA code therefore I thought\nit should be handled only in the IPA code. What do you think?\n\n> \n>> +\n>> \tif (entityControls.empty())\n>> \t\treturn -EINVAL;\n>> \n>> @@ -107,7 +114,8 @@ int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n>> \tgain_ = minGain_;\n>> \n>> \tLOG(IPARkISP1, Info)\n>> -\t\t<< \"Exposure: \" << minExposure_ << \"-\" << maxExposure_\n>> +\t\t<< \"Hw Revision: \" << hwRevision\n>> +\t\t<< \" Exposure: \" << minExposure_ << \"-\" << maxExposure_\n>> \t\t<< \" Gain: \" << minGain_ << \"-\" << maxGain_;\n> \n> Unelated change ?\n\nI added a print of the hwRevision so I also needed to add a space before 'Exposure:'\nso this is actually related.\n\nThanks,\nDafna\n\n> \n>> \n>> \tsetControls(0);\n>> @@ -217,7 +225,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n>> \n>> \t\tunsigned int value = 0;\n>> \t\tunsigned int num = 0;\n>> -\t\tfor (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX; i++) {\n>> +\t\tfor (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {\n>> \t\t\tif (ae->exp_mean[i] <= 15)\n>> \t\t\t\tcontinue;\n>> \n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index 34814f62..19dc9e1e 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -653,7 +653,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>> \tstd::map entityControls;\n>> \tentityControls.emplace(0, data->sensor_->controls());\n>> \n>> -\tret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);\n>> +\tret = data->ipa_->configure(media_->hwRevision(), sensorInfo,\n>> +\t\t\t\t streamConfig, entityControls);\n>> \tif (ret) {\n>> \t\tLOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n>> \t\treturn ret;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EA19FBD1F1\n\tfor ;\n\tWed, 3 Mar 2021 14:49:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89E6C68A91;\n\tWed, 3 Mar 2021 15:49:17 +0100 (CET)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 36BB668A7E\n\tfor ;\n\tWed, 3 Mar 2021 15:49:16 +0100 (CET)","from [IPv6:2003:c7:cf38:3800:7d25:342b:1528:aa90]\n\t(p200300c7cf3838007d25342b1528aa90.dip0.t-ipconnect.de\n\t[IPv6:2003:c7:cf38:3800:7d25:342b:1528:aa90])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: dafna)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id C4A011F45A34;\n\tWed, 3 Mar 2021 14:49:15 +0000 (GMT)"],"To":"Laurent Pinchart ","References":"<20210225171205.23341-1-dafna.hirschfeld@collabora.com>\n\t<20210225171205.23341-4-dafna.hirschfeld@collabora.com>\n\t","From":"Dafna Hirschfeld ","Message-ID":"<3d3b2039-ebba-cde2-2208-4048f201cf9e@collabora.com>","Date":"Wed, 3 Mar 2021 15:49:13 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] ipa: rkisp1: update the uapi\n\tto the latest kernel version","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15451,"web_url":"https://patchwork.libcamera.org/comment/15451/","msgid":"","date":"2021-03-03T15:13:21","subject":"Re: [libcamera-devel] [PATCH v2 3/3] ipa: rkisp1: update the uapi\n\tto the latest kernel version","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nOn Wed, Mar 03, 2021 at 03:49:13PM +0100, Dafna Hirschfeld wrote:\n> On 02.03.21 21:02, Laurent Pinchart wrote:\n> > On Thu, Feb 25, 2021 at 06:12:05PM +0100, Dafna Hirschfeld wrote:\n> >> In kernel 5.11 the rkisp1 uapi had changed to support\n> >> different hardware versions. This patch updates the\n> >> uapi header and adds support for hardware version 10 only.\n> >> In the future, support for other version will be added.\n> > \n> > We split changes to the Linux headers in separate patches, with clear\n> > references to the upstream version. git log in include/linux/ should\n> > show you how that's usually handled. We also update all headers in one\n> > go, to ensure they're consistent with a single kernel version.\n> > \n> > I need v5.12-rc1 for a different reason, I'll post the patch, feel free\n> > to rebase this series on top. I'll also switch from\n> > RKISP1_CIF_ISP_AE_MEAN_MAX to RKISP1_CIF_ISP_AE_MEAN_MAX_V10 in a\n> > separate patch (otherwise the header change will break the rkisp1).\n> > \n> >> Signed-off-by: Dafna Hirschfeld \n> >> Reviewed-by: Paul Elder \n> >> ---\n> >> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> >> include/linux/rkisp1-config.h | 88 +++++++++++++++++++++---\n> >> src/ipa/rkisp1/rkisp1.cpp | 16 +++--\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +-\n> >> 4 files changed, 92 insertions(+), 17 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> >> index 95fa0d93..b097fc8f 100644\n> >> --- a/include/libcamera/ipa/rkisp1.mojom\n> >> +++ b/include/libcamera/ipa/rkisp1.mojom\n> >> @@ -29,7 +29,7 @@ interface IPARkISP1Interface {\n> >> \tstart() => (int32 ret);\n> >> \tstop();\n> >> \n> >> -\tconfigure(CameraSensorInfo sensorInfo,\n> >> +\tconfigure(uint32 hwRevision, CameraSensorInfo sensorInfo,\n> >> \t\t map streamConfig,\n> >> \t\t map entityControls) => (int32 ret);\n> >> \n> >> diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h\n> >> index 9c24867d..3bccccbc 100644\n> >> --- a/include/linux/rkisp1-config.h\n> >> +++ b/include/linux/rkisp1-config.h\n> >> @@ -1,4 +1,4 @@\n> >> -/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */\n> >> +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */\n> >> /*\n> >> * Rockchip ISP1 userspace API\n> >> * Copyright (C) 2017 Rockchip Electronics Co., Ltd.\n> >> @@ -53,8 +53,14 @@\n> >> #define RKISP1_CIF_ISP_CTK_COEFF_MAX 0x100\n> >> #define RKISP1_CIF_ISP_CTK_OFFSET_MAX 0x800\n> >> \n> >> -#define RKISP1_CIF_ISP_AE_MEAN_MAX 25\n> >> -#define RKISP1_CIF_ISP_HIST_BIN_N_MAX 16\n> >> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V10\t\t25\n> >> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V12\t\t81\n> >> +#define RKISP1_CIF_ISP_AE_MEAN_MAX\t\tRKISP1_CIF_ISP_AE_MEAN_MAX_V12\n> >> +\n> >> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10\t16\n> >> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12\t32\n> >> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX\t\tRKISP1_CIF_ISP_HIST_BIN_N_MAX_V12\n> >> +\n> >> #define RKISP1_CIF_ISP_AFM_MAX_WINDOWS 3\n> >> #define RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE 17\n> >> \n> >> @@ -90,7 +96,9 @@\n> >> * Gamma out\n> >> */\n> >> /* Maximum number of color samples supported */\n> >> -#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES 17\n> >> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10 17\n> >> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12 34\n> >> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12\n> >> \n> >> /*\n> >> * Lens shade correction\n> >> @@ -106,8 +114,9 @@\n> >> /*\n> >> * Histogram calculation\n> >> */\n> >> -/* Last 3 values unused. */\n> >> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28\n> >> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 25\n> >> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81\n> >> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12\n> >> \n> >> /*\n> >> * Defect Pixel Cluster Correction\n> >> @@ -128,6 +137,21 @@\n> >> #define RKISP1_CIF_ISP_STAT_AFM (1U << 2)\n> >> #define RKISP1_CIF_ISP_STAT_HIST (1U << 3)\n> >> \n> >> +/**\n> >> + * enum rkisp1_cif_isp_version - ISP variants\n> >> + *\n> >> + * @RKISP1_V10: used at least in rk3288 and rk3399\n> >> + * @RKISP1_V11: declared in the original vendor code, but not used\n> >> + * @RKISP1_V12: used at least in rk3326 and px30\n> >> + * @RKISP1_V13: used at least in rk1808\n> >> + */\n> >> +enum rkisp1_cif_isp_version {\n> >> +\tRKISP1_V10 = 10,\n> >> +\tRKISP1_V11,\n> >> +\tRKISP1_V12,\n> >> +\tRKISP1_V13,\n> >> +};\n> >> +\n> >> enum rkisp1_cif_isp_histogram_mode {\n> >> \tRKISP1_CIF_ISP_HISTOGRAM_MODE_DISABLE,\n> >> \tRKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED,\n> >> @@ -514,6 +538,15 @@ enum rkisp1_cif_isp_goc_mode {\n> >> *\n> >> * @mode: goc mode (from enum rkisp1_cif_isp_goc_mode)\n> >> * @gamma_y: gamma out curve y-axis for all color components\n> >> + *\n> >> + * The number of entries of @gamma_y depends on the hardware revision\n> >> + * as is reported by the hw_revision field of the struct media_device_info\n> >> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> >> + *\n> >> + * Versions <= V11 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10\n> >> + * entries, versions >= V12 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12\n> >> + * entries. RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES is equal to the maximum\n> >> + * of the two.\n> >> */\n> >> struct rkisp1_cif_isp_goc_config {\n> >> \t__u32 mode;\n> >> @@ -528,6 +561,15 @@ struct rkisp1_cif_isp_goc_config {\n> >> *\t\t\t skipped\n> >> * @meas_window: coordinates of the measure window\n> >> * @hist_weight: weighting factor for sub-windows\n> >> + *\n> >> + * The number of entries of @hist_weight depends on the hardware revision\n> >> + * as is reported by the hw_revision field of the struct media_device_info\n> >> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> >> + *\n> >> + * Versions <= V11 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10\n> >> + * entries, versions >= V12 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12\n> >> + * entries. RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE is equal to the maximum\n> >> + * of the two.\n> >> */\n> >> struct rkisp1_cif_isp_hst_config {\n> >> \t__u32 mode;\n> >> @@ -815,7 +857,15 @@ struct rkisp1_cif_isp_bls_meas_val {\n> >> * @exp_mean: Mean luminance value of block xx\n> >> * @bls_val: BLS measured values\n> >> *\n> >> - * Image is divided into 5x5 blocks.\n> >> + * The number of entries of @exp_mean depends on the hardware revision\n> >> + * as is reported by the hw_revision field of the struct media_device_info\n> >> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> >> + *\n> >> + * Versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,\n> >> + * versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.\n> >> + * RKISP1_CIF_ISP_AE_MEAN_MAX is equal to the maximum of the two.\n> >> + *\n> >> + * Image is divided into 5x5 blocks on V10 and 9x9 blocks on V12.\n> >> */\n> >> struct rkisp1_cif_isp_ae_stat {\n> >> \t__u8 exp_mean[RKISP1_CIF_ISP_AE_MEAN_MAX];\n> >> @@ -848,13 +898,29 @@ struct rkisp1_cif_isp_af_stat {\n> >> /**\n> >> * struct rkisp1_cif_isp_hist_stat - statistics histogram data\n> >> *\n> >> - * @hist_bins: measured bin counters\n> >> + * @hist_bins: measured bin counters. Each bin is a 20 bits unsigned fixed point\n> >> + *\t type. Bits 0-4 are the fractional part and bits 5-19 are the\n> >> + *\t integer part.\n> >> + *\n> >> + * The window of the measurements area is divided to 5x5 sub-windows for\n> >> + * V10/V11 and to 9x9 sub-windows for V12. The histogram is then computed for\n> >> + * each sub-window independently and the final result is a weighted average of\n> >> + * the histogram measurements on all sub-windows. The window of the\n> >> + * measurements area and the weight of each sub-window are configurable using\n> >> + * struct @rkisp1_cif_isp_hst_config.\n> >> + *\n> >> + * The histogram contains 16 bins in V10/V11 and 32 bins in V12/V13.\n> >> + *\n> >> + * The number of entries of @hist_bins depends on the hardware revision\n> >> + * as is reported by the hw_revision field of the struct media_device_info\n> >> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.\n> >> *\n> >> - * Measurement window divided into 25 sub-windows, set\n> >> - * with ISP_HIST_XXX\n> >> + * Versions <= V11 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10 entries,\n> >> + * versions >= V12 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12 entries.\n> >> + * RKISP1_CIF_ISP_HIST_BIN_N_MAX is equal to the maximum of the two.\n> >> */\n> >> struct rkisp1_cif_isp_hist_stat {\n> >> -\t__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];\n> >> +\t__u32 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];\n> >> };\n> >> \n> >> /**\n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index 70e8d8f7..39ac8a2b 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -38,7 +38,7 @@ public:\n> >> \tint start() override { return 0; }\n> >> \tvoid stop() override {}\n> >> \n> >> -\tint configure(const CameraSensorInfo &info,\n> >> +\tint configure(uint32_t hwRevision, const CameraSensorInfo &info,\n> >> \t\t const std::map &streamConfig,\n> >> \t\t const std::map &entityControls) override;\n> >> \tvoid mapBuffers(const std::vector &buffers) override;\n> >> @@ -75,10 +75,17 @@ private:\n> >> * assemble one. Make sure the reported sensor information are relevant\n> >> * before accessing them.\n> >> */\n> >> -int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> >> +int IPARkISP1::configure(uint32_t hwRevision, [[maybe_unused]] const CameraSensorInfo &info,\n> >> \t\t\t [[maybe_unused]] const std::map &streamConfig,\n> >> \t\t\t const std::map &entityControls)\n> >> {\n> >> +\t/* \\todo add support for other revisions */\n> >> +\tif (hwRevision != RKISP1_V10) {\n> >> +\t\tLOG(IPARkISP1, Error) << \"Hardware version \" << hwRevision << \" is currently\"\n> >> +\t\t\t\t << \" not supported\";\n> > \n> > Could you please wrap this at 80 columns ?\n> > \n> > \t\tLOG(IPARkISP1, Error)\n> > \t\t\t<< \"Hardware version \" << hwRevision\n> > \t\t\t<< \" is currently not supported\";\n> > \n> >> +\t\treturn -ENODEV;\n> >> +\t}\n> > \n> > It would be nicer to pass the hardware revision to the init() function,\n> > as it doesn't depend on a particular configuration. I've posted a patch\n> > that allow adding more input parameters to init(), you can rebase on top\n> > of that (a v2 will follow shortly for additional output parameters, but\n> > you can ignore that for now).\n> > \n> > Would it make sense to add the same check in the match() function of the\n> > pipeline handler ?\n> \n> The hardware revision is only relevant in the IPA code therefore I thought\n> it should be handled only in the IPA code. What do you think?\n\nWouldn't it better, when the driver is too old to be supported by the\npipeline handler, to fail early instead of creating a camera and failing\nonly at configure() time ?\n\n> >> +\n> >> \tif (entityControls.empty())\n> >> \t\treturn -EINVAL;\n> >> \n> >> @@ -107,7 +114,8 @@ int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> >> \tgain_ = minGain_;\n> >> \n> >> \tLOG(IPARkISP1, Info)\n> >> -\t\t<< \"Exposure: \" << minExposure_ << \"-\" << maxExposure_\n> >> +\t\t<< \"Hw Revision: \" << hwRevision\n> >> +\t\t<< \" Exposure: \" << minExposure_ << \"-\" << maxExposure_\n> >> \t\t<< \" Gain: \" << minGain_ << \"-\" << maxGain_;\n> > \n> > Unelated change ?\n> \n> I added a print of the hwRevision so I also needed to add a space before 'Exposure:'\n> so this is actually related.\n\nOops yes sorry I had misread this.\n\n> >> \n> >> \tsetControls(0);\n> >> @@ -217,7 +225,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> >> \n> >> \t\tunsigned int value = 0;\n> >> \t\tunsigned int num = 0;\n> >> -\t\tfor (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX; i++) {\n> >> +\t\tfor (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {\n> >> \t\t\tif (ae->exp_mean[i] <= 15)\n> >> \t\t\t\tcontinue;\n> >> \n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index 34814f62..19dc9e1e 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -653,7 +653,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >> \tstd::map entityControls;\n> >> \tentityControls.emplace(0, data->sensor_->controls());\n> >> \n> >> -\tret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);\n> >> +\tret = data->ipa_->configure(media_->hwRevision(), sensorInfo,\n> >> +\t\t\t\t streamConfig, entityControls);\n> >> \tif (ret) {\n> >> \t\tLOG(RkISP1, Error) << \"failed configuring IPA (\" << ret << \")\";\n> >> \t\treturn ret;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0474DBD1F1\n\tfor ;\n\tWed, 3 Mar 2021 15:13:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79B3968A98;\n\tWed, 3 Mar 2021 16:13:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82F8968A7E\n\tfor ;\n\tWed, 3 Mar 2021 16:13:50 +0100 (CET)","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 CE9068CA;\n\tWed, 3 Mar 2021 16:13:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LMnnS+av\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614784430;\n\tbh=udSIYI9ngRp2ZEAsccONcilmuhka54CcxubF6LS2Wos=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LMnnS+avoXePw5paG53b5fVWrSrYeBX8AdlnBHOsjuHbrhXscQQqqf1dFSs8GXxOj\n\tJ39WmxM+nw4q7pbgV0asZGUUHDxuoJV44SyJbhojiZ5wNaUXLsVKps0jvJjMAvBcpf\n\tWMNfrAp/OJzxUwUkffWDC2hPVf1oT6cLSV6xM7gU=","Date":"Wed, 3 Mar 2021 17:13:21 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210225171205.23341-1-dafna.hirschfeld@collabora.com>\n\t<20210225171205.23341-4-dafna.hirschfeld@collabora.com>\n\t\n\t<3d3b2039-ebba-cde2-2208-4048f201cf9e@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<3d3b2039-ebba-cde2-2208-4048f201cf9e@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] ipa: rkisp1: update the uapi\n\tto the latest kernel version","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]