[RFC] ipa: rkisp1: dpcc: Generalize YAML parsing

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 <laurent.pinchart@ideasonboard.com>
---
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 ?

2 files changed, 219 insertions, 190 deletions

diff --git a/src/ipa/rkisp1/algorithms/dpcc.cpp b/src/ipa/rkisp1/algorithms/dpcc.cpp
index 80a1b734..3d6b87e6 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<MethodParam> 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 &param,
+		      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<uint16_t>(0);
+	} else {
+		value = data.get<uint16_t>(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 &param : 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()
 	: 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"];
-
-		if (pgObject.contains("green")) {
-			method.method |=
-				RKISP1_CIF_ISP_DPCC_METHODS_SET_PG_GREEN_ENABLE;
-
-			value = pgObject["green"].get<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(0);
-			config_.rnd_offs |=
-				RKISP1_CIF_ISP_DPCC_RND_OFFS_n_RB(i, value);
-		}
+		/* Parse the methods contained in the set. */
+		for (const Method &method : methods) {
+			if (!yamlSet.contains(method.name))
+				continue;
 
-		const YamlObject &rndThresholdObject = set["rnd-threshold"];
-		method.rnd_thresh = 0;
+			const YamlObject &yamlMethod = yamlSet[method.name];
 
-		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<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(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<uint16_t>(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 897b83cb..d80c1655 100644
--- a/src/ipa/rkisp1/data/ov5640.yaml
+++ b/src/ipa/rkisp1/data/ov5640.yaml
@@ -178,63 +178,65 @@ 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
+          rnd:
+            green:
+              threshold: 10
+              offset: 2
+            red-blue:
+              threshold: 10
+              offset: 2
+          rg:
+            green: 32
             red-blue: 32
-          line-mad-factor:
-            green: 16
-            red-blue: 24
-          pg-factor:
+        - lc:
+            green:
+              threshold: 24
+              mad-factor: 16
+            red-blue:
+              threshold: 32
+              mad-factor: 24
+          pg:
             green: 6
             red-blue: 8
-          ro-limits:
+          ro:
             green: 2
             red-blue: 2
-        # PG, LC, RO, RND, RG
-        - line-threshold:
-            green: 32
-            red-blue: 32
-          line-mad-factor:
-            green: 4
-            red-blue: 4
-          pg-factor:
+        - 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
   - Dpf:
       DomainFilter:
         g: [ 16, 16, 16, 16, 16, 16 ]