From patchwork Wed Jul 27 03:47:25 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 16827 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 83729C3275 for ; Wed, 27 Jul 2022 03:47:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id B73B863312; Wed, 27 Jul 2022 05:47:28 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1658893648; bh=3mMRvSbXSn4p/keuUtriwiabR9XEkbThCXNYET3MqMM=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=PgOSYfY6Fd8sJwWy0i6UpbkO7Sdc9nnXx3KEen93Ck6k1PBSHL+oMdGIrOgLVBuFa z1Kl5ZcrGzIxfT6JqDeiNYx23OgL/Ppo+gKuc8f4aqEXDebez465v2Bj6bXUuTpvsN 5rbaFB5dAgm1WrMJ/KCVdin6a750OeEM8WPuJi5MsBi5AqbnNCgDvkmff9Ga0dfrC4 UyOUN8V18oGhV74DDnnZCA+bHNW6piAgZhrluGY0NAY5lpz5PdEiPvudAwDUhuwvHU tpQZCHdxEx5YbfqNzGsDmEMwnVoiVrmYlA1boNN2m9IqAmT+mD8qo1xHuMdMqcYDPh eoABWU7Va1TWg== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C982603E8 for ; Wed, 27 Jul 2022 05:47:27 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="wLv48Bx5"; dkim-atps=neutral Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id BD09756D; Wed, 27 Jul 2022 05:47:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1658893647; bh=3mMRvSbXSn4p/keuUtriwiabR9XEkbThCXNYET3MqMM=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=wLv48Bx5W/Lm/tOZXxP+aPV8nEwp8aUjVTXTRSLu3eeN0ndXxRsxLi53WGwge1rTS vLGAdQJausccuu8+PxCsyuPr9qzxKIKeUy10k4q/eEmic6NqiXeGA2cW3LtoEXQytp tNvoxw/IdcNca0xR18Fo4RbEegiHJnpuWaA06nNU= To: libcamera-devel@lists.libcamera.org Date: Wed, 27 Jul 2022 06:47:25 +0300 Message-Id: <20220727034725.1248-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220726143635.518227-1-fsylvestre@baylibre.com> References: <20220726143635.518227-1-fsylvestre@baylibre.com> MIME-Version: 1.0 Subject: [libcamera-devel] [RFC PATCH] ipa: rkisp1: dpcc: Generalize YAML parsing X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Laurent Pinchart via libcamera-devel From: Laurent Pinchart Reply-To: Laurent Pinchart Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The DefectPixelClusterCorrection::init() function contains a large manually written piece of tuning data parsing code, with duplication of similar but slightly different sections. This is error-prone as copy and paste errors easily creep in and can be hard to spot. As an attempt to address this issue, replace that code with two generic functions tbat operate over a data table which describes the structure of the tuning data and where it fits in the ISP configuration structure (which maps directly to registers). At the same time, restructure the tuning data to group parameter per method, to organize the data in a set-method-parameter structure. The line-threshold and line-mad-factor parameters are grouped in a "lc" (line check) method, and the rnd-threshold and rnd-offset parameters in a "rnd" (rank neighbour difference) method. The other parameters (pg-factor, rg-factor and ro-limits) are directly specified in a "pg", "rg" or "ro" method respectively without a parameter name for simplicity. Signed-off-by: Laurent Pinchart --- This patch applies on top of "[PATCH v3 0/5] Add GSL, LSC and DPCC tuning support for rkisp1" and could be squashed with patch 5/5 in that series, of be left separate. A few questions (hence the RFC): - Should methods that have a single parameter (e.g. "pg") have an explicit parameter name in YAML, to match methods that have multiple parameters ? This would result in pg: green: factor: 10 red-blue: factor: 10 instead of pg: green: 10 red-blue: 10 - Should the method names be spelled out fully in YAML, instead of being abbreviated to 2 or 3 letters ? - Is this actually worth it ? --- src/ipa/rkisp1/algorithms/dpcc.cpp | 319 ++++++++++++++++------------- src/ipa/rkisp1/data/ov5640.yaml | 96 ++++----- 2 files changed, 222 insertions(+), 193 deletions(-) diff --git a/src/ipa/rkisp1/algorithms/dpcc.cpp b/src/ipa/rkisp1/algorithms/dpcc.cpp index dab9e78546fd..6e0d4d4750e1 100644 --- a/src/ipa/rkisp1/algorithms/dpcc.cpp +++ b/src/ipa/rkisp1/algorithms/dpcc.cpp @@ -36,6 +36,158 @@ namespace ipa::rkisp1::algorithms { LOG_DEFINE_CATEGORY(RkISP1Dpcc) +namespace { + +using MethodParamExtract = uint32_t &(*)(struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set); + +struct MethodParam { + std::string name; + unsigned int indexShift; + unsigned int redBlueShift; + MethodParamExtract param; +}; + +struct Method { + std::string name; + uint32_t enableGreen; + uint32_t enableRedBlue; + std::vector params; +}; + +static const Method methods[] = { + { + "pg", /* Peak Gradient */ + RKISP1_CIF_ISP_DPCC_METHODS_SET_PG_GREEN_ENABLE, + RKISP1_CIF_ISP_DPCC_METHODS_SET_PG_RED_BLUE_ENABLE, + { + { + "", 0, 8, + []([[maybe_unused]] struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set) -> uint32_t & { + return set.pg_fac; + }, + }, + }, + }, { + "lc", /* Line Check */ + RKISP1_CIF_ISP_DPCC_METHODS_SET_LC_GREEN_ENABLE, + RKISP1_CIF_ISP_DPCC_METHODS_SET_LC_RED_BLUE_ENABLE, + { + { + "threshold", 0, 8, + []([[maybe_unused]] struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set) -> uint32_t & { + return set.line_thresh; + }, + }, { + "mad-factor", 0, 8, + []([[maybe_unused]] struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set) -> uint32_t & { + return set.line_mad_fac; + }, + }, + }, + }, { + "ro", /* Rank Order */ + RKISP1_CIF_ISP_DPCC_METHODS_SET_RO_GREEN_ENABLE, + RKISP1_CIF_ISP_DPCC_METHODS_SET_RO_RED_BLUE_ENABLE, + { + { + "", 4, 2, + [](struct rkisp1_cif_isp_dpcc_config &config, + [[maybe_unused]] struct rkisp1_cif_isp_dpcc_methods_config &set) -> uint32_t & { + return config.ro_limits; + }, + }, + }, + }, { + "rnd", /* Rank Neighbour Difference */ + RKISP1_CIF_ISP_DPCC_METHODS_SET_RND_GREEN_ENABLE, + RKISP1_CIF_ISP_DPCC_METHODS_SET_RND_RED_BLUE_ENABLE, + { + { + "threshold", 0, 8, + []([[maybe_unused]] struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set) -> uint32_t & { + return set.rnd_thresh; + }, + }, { + "offset", 4, 2, + [](struct rkisp1_cif_isp_dpcc_config &config, + [[maybe_unused]] struct rkisp1_cif_isp_dpcc_methods_config &set) -> uint32_t & { + return config.rnd_offs; + }, + }, + }, + }, { + "rg", /* Rank Gradient */ + RKISP1_CIF_ISP_DPCC_METHODS_SET_RG_GREEN_ENABLE, + RKISP1_CIF_ISP_DPCC_METHODS_SET_RG_RED_BLUE_ENABLE, + { + { + "", 0, 8, + []([[maybe_unused]] struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set) -> uint32_t & { + return set.rg_fac; + }, + }, + }, + }, +}; + +bool parseMethodParam(const YamlObject &data, unsigned int setIndex, + bool redBlue, const MethodParam ¶m, + struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set) +{ + uint32_t value; + + if (!param.name.empty()) { + if (!data.contains(param.name)) + return false; + + value = data[param.name].get(0); + } else { + value = data.get(0); + } + + uint32_t &field = param.param(config, set); + unsigned int shift = param.indexShift * setIndex + + (redBlue ? param.redBlueShift : 0); + field |= value << shift; + + return true; +} + +bool parseMethod(const YamlObject &yamlMethod, const Method &method, + unsigned int setIndex, bool redBlue, + struct rkisp1_cif_isp_dpcc_config &config, + struct rkisp1_cif_isp_dpcc_methods_config &set) +{ + const char *color = redBlue ? "red-blue" : "green"; + + if (!yamlMethod.contains(color)) + return true; + + const YamlObject &yamlParams = yamlMethod[color]; + + for (const MethodParam ¶m : method.params) { + if (!parseMethodParam(yamlParams, setIndex, redBlue, param, config, set)) { + LOG(RkISP1Dpcc, Error) + << "Failed to parse " << color << " values for " + << method.name << "." << param.name + << " in set " << setIndex; + return false; + } + } + + set.method |= method.enableGreen; + return true; +} + +} /* namespace */ + DefectPixelClusterCorrection::DefectPixelClusterCorrection() : initialized_(false), config_({}) { @@ -55,171 +207,46 @@ int DefectPixelClusterCorrection::init([[maybe_unused]] IPAContext &context, ? RKISP1_CIF_ISP_DPCC_SET_USE_STAGE1_USE_FIX_SET : 0; /* Get all defined sets to apply (up to 3). */ - const YamlObject &setsObject = tuningData["sets"]; - if (!setsObject.isList()) { + const YamlObject &yamlSets = tuningData["sets"]; + if (!yamlSets.isList()) { LOG(RkISP1Dpcc, Error) << "'sets' parameter not found in tuning file"; return -EINVAL; } - if (setsObject.size() > RKISP1_CIF_ISP_DPCC_METHODS_MAX) { + if (yamlSets.size() > RKISP1_CIF_ISP_DPCC_METHODS_MAX) { LOG(RkISP1Dpcc, Error) - << "'sets' size in tuning file (" << setsObject.size() + << "'sets' size in tuning file (" << yamlSets.size() << ") exceeds the maximum hardware capacity (3)"; return -EINVAL; } - for (std::size_t i = 0; i < setsObject.size(); ++i) { - struct rkisp1_cif_isp_dpcc_methods_config &method = config_.methods[i]; - const YamlObject &set = setsObject[i]; - uint16_t value; + for (std::size_t i = 0; i < yamlSets.size(); ++i) { + struct rkisp1_cif_isp_dpcc_methods_config &set = config_.methods[i]; + const YamlObject &yamlSet = yamlSets[i]; /* Enable set if described in YAML tuning file. */ config_.set_use |= 1 << i; - /* PG Method */ - const YamlObject &pgObject = set["pg-factor"]; + /* Parse the methods contained in the set. */ + for (const Method &method : methods) { + if (!yamlSet.contains(method.name)) + continue; - if (pgObject.contains("green")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_PG_GREEN_ENABLE; + const YamlObject &yamlMethod = yamlSet[method.name]; - value = pgObject["green"].get(0); - method.pg_fac |= RKISP1_CIF_ISP_DPCC_PG_FAC_G(value); - } - - if (pgObject.contains("red-blue")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_PG_RED_BLUE_ENABLE; - - value = pgObject["red-blue"].get(0); - method.pg_fac |= RKISP1_CIF_ISP_DPCC_PG_FAC_RB(value); - } - - /* RO Method */ - const YamlObject &roObject = set["ro-limits"]; - - if (roObject.contains("green")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RO_GREEN_ENABLE; - - value = roObject["green"].get(0); - config_.ro_limits |= - RKISP1_CIF_ISP_DPCC_RO_LIMITS_n_G(i, value); - } - - if (roObject.contains("red-blue")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RO_RED_BLUE_ENABLE; - - value = roObject["red-blue"].get(0); - config_.ro_limits |= - RKISP1_CIF_ISP_DPCC_RO_LIMITS_n_RB(i, value); - } - - /* RG Method */ - const YamlObject &rgObject = set["rg-factor"]; - method.rg_fac = 0; - - if (rgObject.contains("green")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RG_GREEN_ENABLE; - - value = rgObject["green"].get(0); - method.rg_fac |= RKISP1_CIF_ISP_DPCC_RG_FAC_G(value); - } - - if (rgObject.contains("red-blue")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RG_RED_BLUE_ENABLE; - - value = rgObject["red-blue"].get(0); - method.rg_fac |= RKISP1_CIF_ISP_DPCC_RG_FAC_RB(value); - } - - /* RND Method */ - const YamlObject &rndOffsetsObject = set["rnd-offsets"]; - - if (rndOffsetsObject.contains("green")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RND_GREEN_ENABLE; - - value = rndOffsetsObject["green"].get(0); - config_.rnd_offs |= - RKISP1_CIF_ISP_DPCC_RND_OFFS_n_G(i, value); - } - - if (rndOffsetsObject.contains("red-blue")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RND_RED_BLUE_ENABLE; - - value = rndOffsetsObject["red-blue"].get(0); - config_.rnd_offs |= - RKISP1_CIF_ISP_DPCC_RND_OFFS_n_RB(i, value); - } - - const YamlObject &rndThresholdObject = set["rnd-threshold"]; - method.rnd_thresh = 0; - - if (rndThresholdObject.contains("green")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RND_GREEN_ENABLE; + /* Parse the green and red-blue params. */ + if (!parseMethod(yamlMethod, method, i, false, config_, set)) + return -EINVAL; - value = rndThresholdObject["green"].get(0); - method.rnd_thresh |= - RKISP1_CIF_ISP_DPCC_RND_THRESH_G(value); + if (!parseMethod(yamlMethod, method, i, true, config_, set)) + return -EINVAL; } - if (rndThresholdObject.contains("red-blue")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_RND_RED_BLUE_ENABLE; - - value = rndThresholdObject["red-blue"].get(0); - method.rnd_thresh |= - RKISP1_CIF_ISP_DPCC_RND_THRESH_RB(value); - } - - /* LC Method */ - const YamlObject &lcThresholdObject = set["line-threshold"]; - method.line_thresh = 0; - - if (lcThresholdObject.contains("green")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_LC_GREEN_ENABLE; - - value = lcThresholdObject["green"].get(0); - method.line_thresh |= - RKISP1_CIF_ISP_DPCC_LINE_THRESH_G(value); - } - - if (lcThresholdObject.contains("red-blue")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_LC_RED_BLUE_ENABLE; - - value = lcThresholdObject["red-blue"].get(0); - method.line_thresh |= - RKISP1_CIF_ISP_DPCC_LINE_THRESH_RB(value); - } - - const YamlObject &lcTMadFactorObject = set["line-mad-factor"]; - method.line_mad_fac = 0; - - if (lcTMadFactorObject.contains("green")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_LC_GREEN_ENABLE; - - value = lcTMadFactorObject["green"].get(0); - method.line_mad_fac |= - RKISP1_CIF_ISP_DPCC_LINE_MAD_FAC_G(value); - } - - if (lcTMadFactorObject.contains("red-blue")) { - method.method |= - RKISP1_CIF_ISP_DPCC_METHODS_SET_LC_RED_BLUE_ENABLE; - - value = lcTMadFactorObject["red-blue"].get(0); - method.line_mad_fac |= - RKISP1_CIF_ISP_DPCC_LINE_MAD_FAC_RB(value); + if (!set.method) { + LOG(RkISP1Dpcc, Error) + << "No valid method specified in set " << i; + return -EINVAL; } } diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml index 93d7d1e709d3..459312252a39 100644 --- a/src/ipa/rkisp1/data/ov5640.yaml +++ b/src/ipa/rkisp1/data/ov5640.yaml @@ -99,62 +99,64 @@ algorithms: - DefectPixelClusterCorrection: fixed-set: false sets: - # PG, LC, RO, RND, RG - - line-threshold: + - pg: green: 8 red-blue: 8 - line-mad-factor: - green: 4 - red-blue: 4 - pg-factor: - green: 8 - red-blue: 8 - rnd-threshold: - green: 10 - red-blue: 10 - rg-factor: - green: 32 - red-blue: 32 - ro-limits: + lc: + green: + threshold: 8 + mad-factor: 4 + red-blue: + threshold: 8 + mad-factor: 4 + ro: green: 1 red-blue: 1 - rnd-offsets: - green: 2 - red-blue: 2 - # PG, LC, RO - - line-threshold: - green: 24 - red-blue: 32 - line-mad-factor: - green: 16 - red-blue: 24 - pg-factor: - green: 6 - red-blue: 8 - ro-limits: - green: 2 - red-blue: 2 - # PG, LC, RO, RND, RG - - line-threshold: + rnd: + green: + threshold: 10 + offset: 2 + red-blue: + threshold: 10 + offset: 2 + rg: green: 32 red-blue: 32 - line-mad-factor: - green: 4 - red-blue: 4 - pg-factor: + - lc: + green: + threshold: 24 + mad-factor: 16 + red-blue: + threshold: 32 + mad-factor: 24 + pg: + green: 6 + red-blue: 8 + ro: + green: 2 + red-blue: 2 + - pg: green: 10 red-blue: 10 - rnd-threshold: - green: 6 - red-blue: 8 - rg-factor: - green: 4 - red-blue: 4 - ro-limits: + lc: + green: + threshold: 32 + mad-factor: 4 + red-blue: + threshold: 32 + mad-factor: 4 + ro: green: 1 red-blue: 2 - rnd-offsets: - green: 2 - red-blue: 2 + rnd: + green: + threshold: 6 + offset: 2 + red-blue: + threshold: 8 + offset: 2 + rg: + green: 4 + red-blue: 4 - Filter: ...