linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] edid-decode: DisplayID additions
@ 2019-12-05  7:34 joevt
  2019-12-05  7:34 ` [PATCH 2/5] edid-decode: Change horizontal frequency to kHz joevt
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: joevt @ 2019-12-05  7:34 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

- Decode "Display Parameters Block". Example in lg-ultrafine-5k*.
- Decode "CTA Timings Block". Similar to "Type 1 VESA DMT Timings Block".
- Decode "GP ASCII String Block". Example in dell-up2715k-mdp-switchresx.

- Added DisplayID 2.0 tags:
    - Decode "Display Interface Features Data Block". Example in acer-xv273k* but it appears to be missing the "Minimum pixel rate at which YCbCr 4:2:0 encoding is supported" byte.
    - Decode "ContainerID Data Block". Example in lg-ultrafine-5k*
    - Unknown DisplayID blocks are dumped as hex.
    - Add DisplayID 2.0 spec to man page.

- Show DisplayID tag hex byte to make it possible to distinguish between DisplayID 1.3 and 2.0 spec blocks of the same name.
- Show DisplayID product type.
- Renamed Type* blocks to distinguish between different types of Video Timing Modes and Supported Timing Modes.
- Check padding after DisplayID checksum.
- Add more checks for DisplayID length and revision.
- Move Tiled Topology decode to new function.

Signed-off-by: Joe van Tunen <joevt@shaw.ca>
---
 edid-decode.1             |   4 +-
 edid-decode.h             |   2 +
 parse-base-block.cpp      |   2 +-
 parse-cta-block.cpp       |   2 +-
 parse-displayid-block.cpp | 446 ++++++++++++++++++++++++++++++++------
 5 files changed, 386 insertions(+), 70 deletions(-)

diff --git a/edid-decode.1 b/edid-decode.1
index 3debc47..48c53c6 100644
--- a/edid-decode.1
+++ b/edid-decode.1
@@ -55,10 +55,12 @@ SPWG Notebook Panel Specification, Version 3.5
 EPI Embedded Panel Interface, Revision 1.0
 .RE
 .TP
-The following EDID standard is partially supported by edid-decode:
+The following EDID standards are partially supported by edid-decode:
 .RS
 .TP
 DisplayID 1.3: VESA Display Identification Data (DisplayID) Standard, Version 1.3
+.TP
+DisplayID 2.0: VESA DisplayID Standard, Version 2.0
 .RE
 
 .SH OPTIONS
diff --git a/edid-decode.h b/edid-decode.h
index 577d5ff..83ded83 100644
--- a/edid-decode.h
+++ b/edid-decode.h
@@ -146,5 +146,7 @@ std::string block_name(unsigned char block);
 void print_timings(edid_state &state, const char *prefix, const struct timings *t, const char *suffix);
 
 const struct timings *find_dmt_id(unsigned char dmt_id);
+const struct timings *vic_to_mode(unsigned char vic);
+char *extract_string(const unsigned char *x, unsigned len);
 
 #endif
diff --git a/parse-base-block.cpp b/parse-base-block.cpp
index 896952b..2d384e8 100644
--- a/parse-base-block.cpp
+++ b/parse-base-block.cpp
@@ -773,7 +773,7 @@ void edid_state::detailed_cvt_descriptor(const unsigned char *x, bool first)
 }
 
 /* extract a string from a detailed subblock, checking for termination */
-static char *extract_string(const unsigned char *x, unsigned len)
+char *extract_string(const unsigned char *x, unsigned len)
 {
 	static char s[EDID_PAGE_SIZE];
 	int seen_newline = 0;
diff --git a/parse-cta-block.cpp b/parse-cta-block.cpp
index 4487edb..dea87c1 100644
--- a/parse-cta-block.cpp
+++ b/parse-cta-block.cpp
@@ -191,7 +191,7 @@ static const struct timings edid_cta_modes2[] = {
 
 static const unsigned char edid_hdmi_mode_map[] = { 95, 94, 93, 98 };
 
-static const struct timings *vic_to_mode(unsigned char vic)
+const struct timings *vic_to_mode(unsigned char vic)
 {
 	if (vic > 0 && vic <= ARRAY_SIZE(edid_cta_modes1))
 		return edid_cta_modes1 + vic - 1;
diff --git a/parse-displayid-block.cpp b/parse-displayid-block.cpp
index cdd3a9f..403ae7b 100644
--- a/parse-displayid-block.cpp
+++ b/parse-displayid-block.cpp
@@ -9,6 +9,78 @@
 
 #include "edid-decode.h"
 
+
+// misc functions
+
+static void check_displayid_datablock_revision(const unsigned char *x)
+{
+	unsigned char revisionflags=x[1];
+	if (revisionflags) {
+		warn("Unexpected revision and flags (0x%02x != 0)\n", revisionflags);
+	}
+}
+
+static bool check_displayid_datablock_length(const unsigned char *x, unsigned expectedlenmin = 0, unsigned expectedlenmax = 128 - 2 - 5 - 3, unsigned payloaddumpstart = 0)
+{
+	unsigned char len=x[2];
+	if ( expectedlenmin == expectedlenmax && len != expectedlenmax ) {
+		fail("DisplayID payload length is different than expected (%d != %d)\n", len, expectedlenmax);
+	} else if (len > expectedlenmax) {
+		fail("DisplayID payload length is greater than expected (%d > %d)\n", len, expectedlenmax);
+	} else if (len < expectedlenmin) {
+		fail("DisplayID payload length is less than expected (%d < %d)\n", len, expectedlenmin);
+	} else {
+		return true;
+	}
+
+	if (len > payloaddumpstart) {
+		hex_block("    ", x + 3 + payloaddumpstart, len - payloaddumpstart);
+	}
+	return false;
+}
+
+// tag 0x01
+
+static const char *feature_support_flags[] = {
+	"De-interlacing",
+	"Support ACP, ISRC1, or ISRC2packets",
+	"Fixed pixel format",
+	"Fixed timing",
+	"Power management (DPM)",
+	"Audio input override",
+	"Separate audio inputs provided",
+	"Audio support on video interface"
+};
+
+static void print_flag_lines(const char *indent, const char *label, unsigned char flag_byte, const char **flags) {
+	if (flag_byte) {
+		printf("%s\n", label);
+		for (int i = 0; i < 8; i++) {
+			if (flag_byte & (1<<i)) {
+				printf("%s%s\n", indent, flags[i]);
+			}
+		}
+	}
+}
+
+static void parse_displayid_parameters(const unsigned char *x) {
+	check_displayid_datablock_revision(x);
+	if (check_displayid_datablock_length(x, 12, 12)) {
+		printf("    Image size: %.1f mm x %.1f mm\n", ((x[4]<<8) + x[3]) / 10.0, ((x[6]<<8) + x[5]) / 10.0);
+		printf("    Pixels: %d x %d\n", (x[8]<<8) + x[7], (x[10]<<8) + x[9]);
+		print_flag_lines("      ", "    Feature support flags:", x[11], feature_support_flags);
+
+		if (x[12] != 0xff) {
+			printf("    Gamma: %.2f\n", ((x[12] + 100.0) / 100.0));
+		}
+		printf("    Aspect ratio: %.2f\n", ((x[13] + 100.0) / 100.0));
+		printf("    Dynamic bpc native: %d\n", (x[14] & 0xf) + 1);
+		printf("    Dynamic bpc overall: %d\n", ((x[14] >> 4) & 0xf) + 1);
+	}
+}
+
+// tag 0x03
+
 static void parse_displayid_detailed_timing(const unsigned char *x)
 {
 	struct timings t = {};
@@ -112,11 +184,226 @@ static void parse_displayid_detailed_timing(const unsigned char *x)
 	      );
 }
 
+// tag 0x0b
+
+static void parse_displayid_gp_string(const unsigned char *x)
+{
+	check_displayid_datablock_revision(x);
+	if (check_displayid_datablock_length(x)) {
+		printf("    %s\n", extract_string(x + 3, x[2]));
+	}
+}
+
+// tag 0x12
+
+static void parse_displayid_tiled_display_topology(const unsigned char *x)
+{
+	check_displayid_datablock_revision(x);
+	if (check_displayid_datablock_length(x, 22, 22)) {
+		unsigned capabilities = x[3];
+		unsigned num_v_tile = (x[4] & 0xf) | (x[6] & 0x30);
+		unsigned num_h_tile = (x[4] >> 4) | ((x[6] >> 2) & 0x30);
+		unsigned tile_v_location = (x[5] & 0xf) | ((x[6] & 0x3) << 4);
+		unsigned tile_h_location = (x[5] >> 4) | (((x[6] >> 2) & 0x3) << 4);
+		unsigned tile_width = x[7] | (x[8] << 8);
+		unsigned tile_height = x[9] | (x[10] << 8);
+		unsigned pix_mult = x[11];
+
+		printf("    Capabilities: 0x%08x\n", capabilities);
+		printf("    Num horizontal tiles: %u Num vertical tiles: %u\n", num_h_tile + 1, num_v_tile + 1);
+		printf("    Tile location: %u, %u\n", tile_h_location, tile_v_location);
+		printf("    Tile resolution: %ux%u\n", tile_width + 1, tile_height + 1);
+		if (capabilities & 0x40) {
+			if (pix_mult) {
+				printf("    Top bevel size: %u pixels\n",
+					   pix_mult * x[12] / 10);
+				printf("    Bottom bevel size: %u pixels\n",
+					   pix_mult * x[13] / 10);
+				printf("    Right bevel size: %u pixels\n",
+					   pix_mult * x[14] / 10);
+				printf("    Left bevel size: %u pixels\n",
+					   pix_mult * x[15] / 10);
+			} else {
+				fail("No bevel information, but the pixel multiplier is non-zero\n");
+			}
+			printf("    Tile resolution: %ux%u\n", tile_width + 1, tile_height + 1);
+		} else if (pix_mult) {
+			fail("No bevel information, but the pixel multiplier is non-zero\n");
+		}
+	}
+}
+
+// tag 0x26
+
+static const char *bpc444[] = {"6", "8", "10", "12", "14", "16", NULL, NULL};
+static const char *bpc4xx[] = {"8", "10", "12", "14", "16", NULL, NULL, NULL};
+static const char *audiorates[] = {"32", "44.1", "48", NULL, NULL, NULL, NULL, NULL};
+
+static const char *colorspace_eotf_combinations[] = {
+	"sRGB",
+	"BT.601",
+	"BT.709/BT.1886",
+	"Adobe RGB",
+	"DCI-P3",
+	"BT.2020",
+	"BT.2020/SMPTE ST 2084"
+};
+
+static const char *colorspace_eotf_reserved[] = {NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL};
+
+static const char *colorspaces[] = {
+	"Undefined",
+	"sRGB",
+	"BT.601",
+	"BT.709",
+	"Adobe RGB",
+	"DCI-P3",
+	"BT.2020",
+	"Custom"
+};
+
+static const char *eotfs[] = {
+	"Undefined",
+	"sRGB",
+	"BT.601",
+	"BT.1886",
+	"Adobe RGB",
+	"DCI-P3",
+	"BT.2020",
+	"Gamma function",
+	"SMPTE ST 2084",
+	"Hybrid Log",
+	"Custom"
+};
+
+static void print_flags(const char *label, unsigned char flag_byte, const char **flags, bool reverse = false)
+{
+	if (flag_byte) {
+		printf("%s: ", label);
+		int countflags = 0;
+		for (int i = 0; i < 8; i++) {
+			if (flag_byte & (1<<(reverse?7-i:i))) {
+				if (countflags)
+					printf(", ");
+				if (flags[i])
+					printf("%s", flags[i]);
+				else
+					printf("Undefined(%d)", i);
+				countflags++;
+			}
+		}
+		printf("\n");
+	}
+}
+
+static void parse_displayid_interface_features(const unsigned char *x)
+{
+	check_displayid_datablock_revision(x);
+	if (!check_displayid_datablock_length(x, 9)) {
+		return;
+	}
+	int len=x[2];
+	if (len > 0) print_flags("    Supported bpc for RGB encoding", x[3], bpc444);
+	if (len > 1) print_flags("    Supported bpc for YCbCr 4:4:4 encoding", x[4], bpc444);
+	if (len > 2) print_flags("    Supported bpc for YCbCr 4:2:2 encoding", x[5], bpc4xx);
+	if (len > 3) print_flags("    Supported bpc for YCbCr 4:2:0 encoding", x[6], bpc4xx);
+	if (len > 4 && x[7])
+	                  printf("    Minimum pixel rate at which YCbCr 4:2:0 encoding is supported: %.3f MHz\n", 74.25 * x[7]);
+	if (len > 5) print_flags("    Supported audio capability and features (kHz)", x[8], audiorates, true);
+	if (len > 6) print_flags("    Supported color space and EOTF standard combination 1", x[9], colorspace_eotf_combinations);
+	if (len > 7) print_flags("    Supported color space and EOTF standard combination 2", x[10], colorspace_eotf_reserved);
+	int i = 0;
+	if (len > 8 && x[11]) {
+		printf("    Supported color space and EOTF additional combinations:");
+		for (i = 0; i < x[11]; i++) {
+			if (i > 6) {
+				printf("\n    Number of additional color space and EOTF combinations (%d) is greater than allowed (7).", x[11]);
+				break;
+			} else if (i + 10 > len) {
+				printf("\n    Number of additional color space and EOTF combinations (%d) is too many to fit in block (%d).", x[11], len - 9);
+				break;
+			}
+
+			const char *colorspace = "Out of range";
+			const char *eotf = "Out of range";
+			unsigned colorspace_index = (x[12 + i] >> 4) & 0xf;
+			unsigned eotf_index = x[12 + i] & 0xf;
+			if (colorspace_index < sizeof(colorspaces) / sizeof(colorspaces[0])) {
+				colorspace = colorspaces[colorspace_index];
+			}
+			if (eotf_index < sizeof(eotfs) / sizeof(eotfs[0])) {
+				eotf = eotfs[eotf_index];
+			}
+
+			if (i > 0)
+				printf(", ");
+			if (!strcmp(colorspace, eotf)) {
+				printf("%s", colorspace);
+			} else {
+				printf("%s/%s", colorspace, eotf);
+			}
+		} // for
+		printf("\n");
+	} // x[11]
+	check_displayid_datablock_length(x, 9 + i, 9 + i, 9 + i);
+}
+
+// tag 0x29
+
+static void parse_displayid_ContainerID(const unsigned char *x)
+{
+	check_displayid_datablock_revision(x);
+	if (check_displayid_datablock_length(x, 16, 16)) {
+		x += 3;
+		printf("    %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n",x[0],x[1],x[2],x[3],x[4],x[5],x[6],x[7],x[8],x[9],x[10],x[11],x[12],x[13],x[14],x[15]);
+	}
+}
+
+// DisplayID main
+
+static std::string product_type(unsigned version, unsigned char x, bool heading)
+{
+	std::string headingstr;
+	if (version < 0x20) {
+		headingstr = "Display Product Type";
+		if (heading) return headingstr;
+		switch (x) {
+		case 0: return "Extension Section";
+		case 1: return "Test Structure; test equipment only";
+		case 2: return "Display panel or other transducer, LCD or PDP module, etc.";
+		case 3: return "Standalone display device";
+		case 4: return "Television receiver";
+		case 5: return "Repeater/translator";
+		case 6: return "DIRECT DRIVE monitor";
+		default: break;
+		}
+	}
+	else
+	{
+		headingstr = "Display Product Primary Use Case";
+		if (heading) return headingstr;
+		switch (x) {
+		case 0: return "Same primary use case as the base section";
+		case 1: return "Test Structure; test equipment only";
+		case 2: return "None of the listed primary use cases; generic display";
+		case 3: return "Television (TV) display";
+		case 4: return "Desktop productivity display";
+		case 5: return "Desktop gaming display";
+		case 6: return "Presentation display";
+		case 7: return "Head-mounted Virtual Reality (VR) display";
+		case 8: return "Head-mounted Augmented Reality (AR) display";
+		default: break;
+		}
+	}
+	fail("Unknown %s 0x%02x\n", headingstr.c_str(), x);
+	return std::string("Unknown " + headingstr + " (") + utohex(x) + ")";
+}
+
 void edid_state::parse_displayid_block(const unsigned char *x)
 {
-	const unsigned char *orig = x;
 	unsigned version = x[1];
-	int length = x[2];
+	unsigned length = x[2];
+	unsigned prod_type = x[3]; // future check: based on type, check for required data blocks
 	unsigned ext_count = x[4];
 	unsigned i;
 
@@ -124,94 +411,114 @@ void edid_state::parse_displayid_block(const unsigned char *x)
 	       block.c_str(), version >> 4, version & 0xf,
 	       length, ext_count);
 
+	if (ext_count > 0) {
+		warn("Non-0 DisplayID extension count %d\n", ext_count);
+	}
+	
+	printf("%s: %s\n", product_type(version, prod_type, true).c_str(), product_type(version, prod_type, false).c_str());
+	
+	if (length > 121) {
+		fail("DisplayID length %d is greater than 121\n", length);
+		length = 121;
+	}
+
 	unsigned offset = 5;
 	while (length > 0) {
 		unsigned tag = x[offset];
+		switch (tag) {
+		// DisplayID 1.3:
+		case 0x00: data_block = "Product Identification Data Block (" + utohex(tag) + ")"; break; // not implemented
+		case 0x01: data_block = "Display Parameters Data Block (" + utohex(tag) + ")"; break;
+		case 0x02: data_block = "Color Characteristics Data Block"; break; // not implemented
+		case 0x03: data_block = "Video Timing Modes Type 1 - Detailed Timings Data Block"; break;
+		case 0x04: data_block = "Video Timing Modes Type 2 - Detailed Timings Data Block"; break; // not implemented
+		case 0x05: data_block = "Video Timing Modes Type 3 - Short Timings Data Block"; break; // not implemented
+		case 0x06: data_block = "Video Timing Modes Type 4 - DMT Timings Data Block"; break; // not implemented
+		case 0x07: data_block = "Supported Timing Modes Type 1 - VESA DMT Timings Data Block"; break;
+		case 0x08: data_block = "Supported Timing Modes Type 2 - CTA Timings Data Block"; break;
+		case 0x09: data_block = "Video Timing Range Data Block"; break; // not implemented
+		case 0x0a: data_block = "Product Serial Number Data Block"; break; // not implemented
+		case 0x0b: data_block = "GP ASCII String Data Block"; break;
+		case 0x0c: data_block = "Display Device Data Data Block"; break; // not implemented
+		case 0x0d: data_block = "Interface Power Sequencing Data Block"; break; // not implemented
+		case 0x0e: data_block = "Transfer Characteristics Data Block"; break; // not implemented
+		case 0x0f: data_block = "Display Interface Data Block"; break; // not implemented
+		case 0x10: data_block = "Stereo Display Interface Data Block (" + utohex(tag) + ")"; break; // not implemented
+		case 0x11: data_block = "Video Timing Modes Type 5 - Short Timings Data Block"; break; // not implemented
+		case 0x12: data_block = "Tiled Display Topology Data Block (" + utohex(tag) + ")"; break;
+		case 0x13: data_block = "Video Timing Modes Type 6 - Detailed Timings Data Block"; break; // not implemented
+		// 14h .. 7Eh RESERVED for Additional VESA-defined Data Blocks
+		// DisplayID 2.0
+		case 0x20: data_block = "Product Identification Data Block (" + utohex(tag) + ")"; break; // not implemented
+		case 0x21: data_block = "Display Parameters Data Block (" + utohex(tag) + ")"; break; // not implemented
+		case 0x22: data_block = "Video Timing Modes Type 7 - Detailed Timings Data Block"; break; // not implemented
+		case 0x23: data_block = "Video Timing Modes Type 8 - Enumerated Timing Codes Data Block"; break; // not implemented
+		case 0x24: data_block = "Video Timing Modes Type 9 - Formula-based Timings Data Block"; break; // not implemented
+		case 0x25: data_block = "Dynamic Video Timing Range Limits Data Block"; break; // not implemented
+		case 0x26: data_block = "Display Interface Features Data Block"; break;
+		case 0x27: data_block = "Stereo Display Interface Data Block (" + utohex(tag) + ")"; break; // not implemented
+		case 0x28: data_block = "Tiled Display Topology Data Block (" + utohex(tag) + ")"; break; // not implemented
+		case 0x29: data_block = "ContainerID Data Block"; break;
+		// 2Ah .. 7Dh RESERVED for Additional VESA-defined Data Blocks
+		case 0x7e: // DisplayID 2.0
+		case 0x7f: data_block = "Vendor-specific Data Block (" + utohex(tag) + ")"; break; // DisplayID 1.3 // not implemented
+		// 7Fh .. 80h RESERVED
+		case 0x81: data_block = "CTA DisplayID Data Block (" + utohex(tag) + ")"; break; // not implemented
+		// 82h .. FFh RESERVED
+		default:   data_block = "Unknown DisplayID Data Block (" + utohex(tag) + ")"; break;
+		}
+
+		if (length < 3) {
+			// report a problem when the remaining bytes are not 0.
+			if (tag || x[offset + 1]) {
+				fail("Not enough bytes remain (%d) for a DisplayID data block or the DisplayID filler is non-0\n", length);
+			}
+			break;
+		}
+
 		unsigned len = x[offset + 2];
+		
+		if (length < len + 3) {
+			fail("The length of this DisplayID data block (%d) exceeds the number of bytes remaining (%d)\n", len + 3, length);
+			break;
+		}
 
 		if (!tag && !len) {
-			while (length && !x[offset]) {
-				length--;
-				offset++;
-			}
-			if (length)
+			// A Product Identification Data Block with no payload bytes is not valid - assume this is the end.
+			if (!memchk(x + offset, length)) {
 				fail("Non-0 filler bytes in the DisplayID block\n");
+			}
 			break;
 		}
-		switch (tag) {
-		case 0x00: data_block = "Product ID Data Block"; break;
-		case 0x01: data_block = "Display Parameters Data Block"; break;
-		case 0x02: data_block = "Color Characteristics Data Block"; break;
-		case 0x03: data_block = "Type 1 Detailed Timings Data Block"; break;
-		case 0x04: data_block = "Type 2 Detailed Timings Data Block"; break;
-		case 0x05: data_block = "Type 3 Short Timings Data Block"; break;
-		case 0x06: data_block = "Type 4 DMT Timings Data Block"; break;
-		case 0x07: data_block = "Type 1 VESA DMT Timings Data Block"; break;
-		case 0x08: data_block = "CTA Timings Data Block"; break;
-		case 0x09: data_block = "Video Timing Range Data Block"; break;
-		case 0x0a: data_block = "Product Serial Number Data Block"; break;
-		case 0x0b: data_block = "GP ASCII String Data Block"; break;
-		case 0x0c: data_block = "Display Device Data Data Block"; break;
-		case 0x0d: data_block = "Interface Power Sequencing Data Block"; break;
-		case 0x0e: data_block = "Transfer Characteristics Data Block"; break;
-		case 0x0f: data_block = "Display Interface Data Block"; break;
-		case 0x10: data_block = "Stereo Display Interface Data Block"; break;
-		case 0x12: data_block = "Tiled Display Topology Data Block"; break;
-		default: data_block = "Unknown DisplayID Data Block (" + utohex(tag) + ")"; break;
-		}
 
 		printf("  %s\n", data_block.c_str());
 
 		switch (tag) {
+		case 0x01: parse_displayid_parameters(x + offset); break;
 		case 0x03:
 			for (i = 0; i < len / 20; i++) {
 				parse_displayid_detailed_timing(&x[offset + 3 + (i * 20)]);
 			}
 			break;
-
 		case 0x07:
 			for (i = 0; i < min(len, 10) * 8; i++)
-				if (x[offset + 3 + i / 8] & (1 << (i % 8)))
+				if (x[offset + 3 + i / 8] & (1 << (i % 8))) {
 					print_timings("    ", find_dmt_id(i + 1), "DMT");
-			break;
-
-		case 0x12: {
-			unsigned capabilities = x[offset + 3];
-			unsigned num_v_tile = (x[offset + 4] & 0xf) | (x[offset + 6] & 0x30);
-			unsigned num_h_tile = (x[offset + 4] >> 4) | ((x[offset + 6] >> 2) & 0x30);
-			unsigned tile_v_location = (x[offset + 5] & 0xf) | ((x[offset + 6] & 0x3) << 4);
-			unsigned tile_h_location = (x[offset + 5] >> 4) | (((x[offset + 6] >> 2) & 0x3) << 4);
-			unsigned tile_width = x[offset + 7] | (x[offset + 8] << 8);
-			unsigned tile_height = x[offset + 9] | (x[offset + 10] << 8);
-			unsigned pix_mult = x[offset + 11];
-
-			printf("    Capabilities: 0x%08x\n", capabilities);
-			printf("    Num horizontal tiles: %u Num vertical tiles: %u\n", num_h_tile + 1, num_v_tile + 1);
-			printf("    Tile location: %u, %u\n", tile_h_location, tile_v_location);
-			printf("    Tile resolution: %ux%u\n", tile_width + 1, tile_height + 1);
-			if (capabilities & 0x40) {
-				if (pix_mult) {
-					printf("    Top bevel size: %u pixels\n",
-					       pix_mult * x[offset + 12] / 10);
-					printf("    Bottom bevel size: %u pixels\n",
-					       pix_mult * x[offset + 13] / 10);
-					printf("    Right bevel size: %u pixels\n",
-					       pix_mult * x[offset + 14] / 10);
-					printf("    Left bevel size: %u pixels\n",
-					       pix_mult * x[offset + 15] / 10);
-				} else {
-					fail("No bevel information, but the pixel multiplier is non-zero\n");
 				}
-				printf("    Tile resolution: %ux%u\n", tile_width + 1, tile_height + 1);
-			} else if (pix_mult) {
-				fail("No bevel information, but the pixel multiplier is non-zero\n");
-			}
 			break;
-		}
-
-		default:
-			hex_block("    ", x + offset + 3, len);
+		case 0x08:
+			for (i = 0; i < min(len, 8) * 8; i++)
+				if (x[offset + 3 + i / 8] & (1 << (i % 8))) {
+					char suffix[16];
+					sprintf(suffix, "VIC %3u", i + 1);
+					print_timings("    ", vic_to_mode(i + 1), suffix);
+				}
 			break;
+		case 0x0b: parse_displayid_gp_string(x + offset); break;
+		case 0x12: parse_displayid_tiled_display_topology(x + offset); break;
+		case 0x26: parse_displayid_interface_features(x + offset); break;
+		case 0x29: parse_displayid_ContainerID(x + offset); break;
+		default: hex_block("    ", x + offset + 3, len); break;
 		}
 		length -= len + 3;
 		offset += len + 3;
@@ -223,5 +530,10 @@ void edid_state::parse_displayid_block(const unsigned char *x)
 	 * (excluding DisplayID-in-EDID magic byte)
 	 */
 	data_block.clear();
-	do_checksum("  ", orig + 1, orig[2] + 5);
+	do_checksum("  ", x + 1, x[2] + 5);
+	
+	if (!memchk(x + 1 + x[2] + 5, 0x7f - (1 + x[2] + 5))) {
+		data_block = "Padding";
+		fail("DisplayID padding contains non-zero bytes\n");
+	}
 }
-- 
2.21.0 (Apple Git-122.2)


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/5] edid-decode: Change horizontal frequency to kHz
  2019-12-05  7:34 [PATCH 1/5] edid-decode: DisplayID additions joevt
@ 2019-12-05  7:34 ` joevt
  2019-12-05  7:34 ` [PATCH 3/5] edid-decode: more back/front porch switching joevt
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: joevt @ 2019-12-05  7:34 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

Change Monitor Ranges error message to use kHz for horizontal frequency.

Signed-off-by: Joe van Tunen <joevt@shaw.ca>
---
 edid-decode.cpp | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/edid-decode.cpp b/edid-decode.cpp
index eac47c5..0c6c237 100644
--- a/edid-decode.cpp
+++ b/edid-decode.cpp
@@ -735,26 +735,22 @@ int edid_state::parse_edid()
 		 * EDID 1.4 states (in an Errata) that explicitly defined
 		 * timings supersede the monitor range definition.
 		 */
+		char buf[512];
+		snprintf(buf, sizeof(buf),
+			"One or more of the timings is out of range of the Monitor Ranges:\n"
+			"    Vertical Freq: %u - %u Hz (Monitor: %u - %u Hz)\n"
+			"    Horizontal Freq: %.3f - %.3f kHz (Monitor: %.3f - %.3f kHz)\n"
+			"    Maximum Clock: %.3f MHz (Monitor: %.3f MHz)\n",
+			min_vert_freq_hz, max_vert_freq_hz,
+			min_display_vert_freq_hz, max_display_vert_freq_hz,
+			min_hor_freq_hz / 1000.0, max_hor_freq_hz / 1000.0,
+			min_display_hor_freq_hz / 1000.0, max_display_hor_freq_hz / 1000.0,
+			max_pixclk_khz / 1000.0, max_display_pixclk_khz / 1000.0);
+		
 		if (edid_minor < 4) {
-			fail("One or more of the timings is out of range of the Monitor Ranges:\n"
-			     "    Vertical Freq: %u - %u Hz (Monitor: %u - %u Hz)\n"
-			     "    Horizontal Freq: %u - %u Hz (Monitor: %u - %u Hz)\n"
-			     "    Maximum Clock: %.3f MHz (Monitor: %.3f MHz)\n",
-			     min_vert_freq_hz, max_vert_freq_hz,
-			     min_display_vert_freq_hz, max_display_vert_freq_hz,
-			     min_hor_freq_hz, max_hor_freq_hz,
-			     min_display_hor_freq_hz, max_display_hor_freq_hz,
-			     max_pixclk_khz / 1000.0, max_display_pixclk_khz / 1000.0);
+			fail("%s", buf);
 		} else {
-			warn("One or more of the timings is out of range of the Monitor Ranges:\n"
-			     "    Vertical Freq: %u - %u Hz (Monitor: %u - %u Hz)\n"
-			     "    Horizontal Freq: %u - %u Hz (Monitor: %u - %u Hz)\n"
-			     "    Maximum Clock: %.3f MHz (Monitor: %.3f MHz)\n",
-			     min_vert_freq_hz, max_vert_freq_hz,
-			     min_display_vert_freq_hz, max_display_vert_freq_hz,
-			     min_hor_freq_hz, max_hor_freq_hz,
-			     min_display_hor_freq_hz, max_display_hor_freq_hz,
-			     max_pixclk_khz / 1000.0, max_display_pixclk_khz / 1000.0);
+			warn("%s", buf);
 		}
 	}
 
-- 
2.21.0 (Apple Git-122.2)


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/5] edid-decode: more back/front porch switching
  2019-12-05  7:34 [PATCH 1/5] edid-decode: DisplayID additions joevt
  2019-12-05  7:34 ` [PATCH 2/5] edid-decode: Change horizontal frequency to kHz joevt
@ 2019-12-05  7:34 ` joevt
  2019-12-05  7:34 ` [PATCH 4/5] edid-decode: CTA extension block cleanup joevt
  2019-12-05  7:34 ` [PATCH 5/5] edid-decode: add missing space joevt
  3 siblings, 0 replies; 8+ messages in thread
From: joevt @ 2019-12-05  7:34 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

Fix more instances where back porch is actually front porch (problem started when "so = sync offset" was mistaken for "bp = back porch" instead of "fp = front porch").

Signed-off-by: Joe van Tunen <joevt@shaw.ca>
---
 parse-base-block.cpp      | 12 ++++++------
 parse-displayid-block.cpp |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/parse-base-block.cpp b/parse-base-block.cpp
index 2d384e8..0b6e3d0 100644
--- a/parse-base-block.cpp
+++ b/parse-base-block.cpp
@@ -1302,13 +1302,13 @@ void edid_state::detailed_timings(const char *prefix, const unsigned char *x)
 
 	bool ok = true;
 
-	if (!t.w || !hbl || !t.hbp || !t.hsync || !t.h || !vbl || !t.vbp || !t.vsync) {
+	if (!t.w || !hbl || !t.hfp || !t.hsync || !t.h || !vbl || !t.vfp || !t.vsync) {
 		fail("0 values in the detailed timings:\n"
 		     "    Horizontal Active/Blanking %u/%u\n"
-		     "    Horizontal Backporch/Sync Width %u/%u\n"
+		     "    Horizontal Frontporch/Sync Width %u/%u\n"
 		     "    Vertical Active/Blanking %u/%u\n"
-		     "    Vertical Backporch/Sync Width %u/%u\n",
-		     t.w, hbl, t.hbp, t.hsync, t.h, vbl, t.vbp, t.vsync);
+		     "    Vertical Frontporch/Sync Width %u/%u\n",
+		     t.w, hbl, t.hfp, t.hsync, t.h, vbl, t.vfp, t.vsync);
 		ok = false;
 	}
 
@@ -1324,10 +1324,10 @@ void edid_state::detailed_timings(const char *prefix, const unsigned char *x)
 	       t.pixclk_khz / 1000.0,
 	       t.hor_mm, t.vert_mm,
 	       prefix,
-	       t.w, t.w + t.hbp, t.w + t.hbp + t.hsync, t.w + hbl, t.hfp, t.hsync, t.hbp,
+	       t.w, t.w + t.hfp, t.w + t.hfp + t.hsync, t.w + hbl, t.hfp, t.hsync, t.hbp,
 	       t.hborder ? (std::string(" hborder ") + std::to_string(t.hborder)).c_str() : "",
 	       prefix,
-	       t.h, t.h + t.vbp, t.h + t.vbp + t.vsync, t.h + vbl, t.vfp, t.vsync, t.vbp,
+	       t.h, t.h + t.vfp, t.h + t.vfp + t.vsync, t.h + vbl, t.vfp, t.vsync, t.vbp,
 	       t.vborder ? (std::string(" vborder ") + std::to_string(t.vborder)).c_str() : "",
 	       prefix,
 	       s_sync.c_str(), s_flags.c_str(),
diff --git a/parse-displayid-block.cpp b/parse-displayid-block.cpp
index 403ae7b..3ac3d92 100644
--- a/parse-displayid-block.cpp
+++ b/parse-displayid-block.cpp
@@ -176,8 +176,8 @@ static void parse_displayid_detailed_timing(const unsigned char *x)
 	       "                   %chsync %cvsync\n"
 	       "                   VertFreq: %.3f Hz, HorFreq: %.3f kHz\n",
 	       (double)t.pixclk_khz/1000.0, s.c_str(),
-	       t.w, t.w + t.hbp, t.w + t.hbp + t.hsync, t.w + hbl, t.hfp, t.hsync, t.hbp,
-	       t.h, t.h + t.vbp, t.h + t.vbp + t.vsync, t.h + vbl, t.vfp, t.vsync, t.vbp,
+	       t.w, t.w + t.hfp, t.w + t.hfp + t.hsync, t.w + hbl, t.hfp, t.hsync, t.hbp,
+	       t.h, t.h + t.vfp, t.h + t.vfp + t.vsync, t.h + vbl, t.vfp, t.vsync, t.vbp,
 	       t.pos_pol_hsync ? '+' : '-', t.pos_pol_vsync ? '+' : '-',
 	       (t.pixclk_khz * 1000.0) / ((t.w + hbl) * (t.h + vbl)),
 	       (double)(t.pixclk_khz) / (t.w + hbl)
-- 
2.21.0 (Apple Git-122.2)


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/5] edid-decode: CTA extension block cleanup
  2019-12-05  7:34 [PATCH 1/5] edid-decode: DisplayID additions joevt
  2019-12-05  7:34 ` [PATCH 2/5] edid-decode: Change horizontal frequency to kHz joevt
  2019-12-05  7:34 ` [PATCH 3/5] edid-decode: more back/front porch switching joevt
@ 2019-12-05  7:34 ` joevt
  2019-12-05  9:38   ` Hans Verkuil
  2019-12-05  7:34 ` [PATCH 5/5] edid-decode: add missing space joevt
  3 siblings, 1 reply; 8+ messages in thread
From: joevt @ 2019-12-05  7:34 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

Reduced tag parsing code.
Added OUI min block length checks.

Signed-off-by: Joe van Tunen <joevt@shaw.ca>
---
 edid-decode.h       |   1 +
 parse-cta-block.cpp | 336 ++++++++++++++++++--------------------------
 2 files changed, 138 insertions(+), 199 deletions(-)

diff --git a/edid-decode.h b/edid-decode.h
index 83ded83..758bdcf 100644
--- a/edid-decode.h
+++ b/edid-decode.h
@@ -111,6 +111,7 @@ struct edid_state {
 	void cta_y420cmdb(const unsigned char *x, unsigned length);
 	void cta_vfpdb(const unsigned char *x, unsigned length);
 	void cta_hdmi_block(const unsigned char *x, unsigned length);
+	void cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum);
 	void cta_block(const unsigned char *x);
 	void preparse_cta_block(const unsigned char *x);
 	void parse_cta_block(const unsigned char *x);
diff --git a/parse-cta-block.cpp b/parse-cta-block.cpp
index dea87c1..e52822a 100644
--- a/parse-cta-block.cpp
+++ b/parse-cta-block.cpp
@@ -375,6 +375,11 @@ void edid_state::cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420)
 
 		if (vic == 1 && !for_ycbcr420)
 			has_cta861_vic_1 = 1;
+
+		// vics and has_vic are basically the same (if has_vic was not bool), except vics
+		// is built after preparse (during parse) which allows errors for duplicates to be
+		// output in parse order. has_vic is built during preparse and is used when vics
+		// from other blocks need to be checked.
 		if (++vics[vic][for_ycbcr420] == 2)
 			fail("Duplicate %sVIC %u\n", for_ycbcr420 ? "YCbCr 4:2:0 " : "", vic);
 		if (for_ycbcr420 && has_vic[0][vic])
@@ -485,7 +490,6 @@ void edid_state::cta_hdmi_block(const unsigned char *x, unsigned length)
 {
 	unsigned len_vic, len_3d;
 
-	printf(" (HDMI)\n");
 	printf("    Source physical address %u.%u.%u.%u\n", x[3] >> 4, x[3] & 0x0f,
 	       x[4] >> 4, x[4] & 0x0f);
 
@@ -1228,10 +1232,20 @@ static void cta_hdmi_audio_block(const unsigned char *x, unsigned length)
 		x += 4;
 	}
 }
+const unsigned kOUI_Unknown   = 1<<12;
+const unsigned kOUI_HDMI      = 2<<12;
+const unsigned kOUI_HDMIForum = 3<<12;
+const unsigned kOUI_HDR10     = 4<<12;
 
-static const char *oui_name(unsigned oui)
+static const char *oui_name(unsigned oui, unsigned *ouinum = NULL)
 {
+	unsigned ouinumscratch;
+	if (!ouinum) ouinum = &ouinumscratch;
+	*ouinum = kOUI_Unknown;
 	switch (oui) {
+	case 0x000c03: *ouinum = kOUI_HDMI      ; return "HDMI";
+	case 0xc45dd8: *ouinum = kOUI_HDMIForum ; return "HDMI Forum";
+	case 0x90848b: *ouinum = kOUI_HDR10     ; return "HDR10+";
 	case 0x00001a: return "AMD";
 	case 0x00044b: return "NVIDIA";
 	case 0x000c6e: return "ASUS";
@@ -1244,212 +1258,136 @@ static const char *oui_name(unsigned oui)
 	}
 }
 
+void edid_state::cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum)
+{
+	char buf[10];
+	unsigned oui;
+
+	if (length < 3) {
+		oui = 0xffffffff;
+		sprintf(buf, "?");
+	} else {
+		oui = (x[2] << 16) + (x[1] << 8) + x[0];
+		sprintf(buf, "0x%06x", oui);
+	}
+	
+	const char *ouiname = oui_name(oui, ouinum);
+	std::string name = std::string(block_name) + ", OUI " + buf;
+	if (ouiname) name += std::string(" (") + ouiname + ")";
+	data_block = name;
+	
+	if (oui == 0xffffffff)
+		fail("CTA data block is not long enough to contain an OUI\n");
+}
+
+#define data_block_o(n) cta_oui(n, x + 1 + extended, length - extended, &ouinum)
+
 void edid_state::cta_block(const unsigned char *x)
 {
-	static int last_block_was_hdmi_vsdb;
-	static int have_hf_vsdb, have_hf_scdb;
-	static int first_block = 1;
+	static unsigned previous_cta_tag = 0;
+	static bool have_hf_vsdb = false;
+	static bool have_hf_scdb = false;
+	static unsigned cta_block_number = 0;
+
 	unsigned length = x[0] & 0x1f;
-	const char *name;
-	unsigned oui;
+	unsigned ouinum = 0;
+	unsigned tag=(x[0] & 0xe0) >> 5;
+	unsigned extended = tag == 0x07 ? 1 : 0;
+	if (extended) tag = 0x700 + x[1];
+
+	switch (tag) {
+	case 0x001: data_block = "Audio Data Block"; break;
+	case 0x002: data_block = "Video Data Block"; break;
+	case 0x003: data_block_o("Vendor-Specific Data Block"); break;
+	case 0x004: data_block = "Speaker Allocation Data Block"; break;
+	case 0x005: data_block = "VESA DTC Data Block"; break; // not implemented
+
+	case 0x700: data_block = "Video Capability Data Block"; break;
+	case 0x701: data_block_o("Vendor-Specific Video Data Block"); break;
+	case 0x702: data_block = "VESA Video Display Device Data Block"; break; // not implemented
+	case 0x703: data_block = "VESA Video Timing Block Extension"; break; // not implemented
+	case 0x704: data_block = "Reserved for HDMI Video Data Block"; break; // reserved
+	case 0x705: data_block = "Colorimetry Data Block"; break;
+	case 0x706: data_block = "HDR Static Metadata Data Block"; break;
+	case 0x707: data_block = "HDR Dynamic Metadata Data Block"; break;
+
+	case 0x70d: data_block = "Video Format Preference Data Block"; break;
+	case 0x70e: data_block = "YCbCr 4:2:0 Video Data Block"; break;
+	case 0x70f: data_block = "YCbCr 4:2:0 Capability Map Data Block"; break;
+	case 0x710: data_block = "Reserved for CTA Miscellaneous Audio Fields"; break; // reserved
+	case 0x711: data_block_o("Vendor-Specific Audio Data Block"); break; // no vendors implemented
+	case 0x712: data_block = "HDMI Audio Data Block"; break;
+	case 0x713: data_block = "Room Configuration Data Block"; break;
+	case 0x714: data_block = "Speaker Location Data Block"; break;
+
+	case 0x720: data_block = "InfoFrame Data Block"; break;
+
+	case 0x778: data_block = "HDMI Forum EDID Extension Override Data Block"; break;
+	case 0x779: data_block = "HDMI Forum Sink Capability Data Block"; break;
+	default:
+		     if (tag < 0x700) data_block = "Unknown CTA Data Block";
+		else if (tag < 0x70d) data_block = "Unknown CTA Video-Related Data Block";
+		else if (tag < 0x720) data_block = "Unknown CTA Audio-Related Data Block";
+		else if (tag < 0x778) data_block = "Unknown CTA Data Block";
+		else if (tag < 0x780) data_block = "Unknown CTA HDMI-Related Data Block";
+		else                  data_block = "Unknown CTA Data Block";
+		data_block += std::string(" (") + (extended ? "extended " : "") + "tag " + utohex(tag & 0xff) + ")";
+	}
 
-	switch ((x[0] & 0xe0) >> 5) {
-	case 0x01:
-		data_block = "Audio Data Block";
-		printf("  %s\n", data_block.c_str());
-		cta_audio_block(x + 1, length);
+	printf("  %s\n", data_block.c_str());
+	
+	tag |= ouinum;
+	switch (tag) {
+	case 0x001: cta_audio_block(x + 1, length); break;
+	case 0x002: cta_svd(x + 1, length, 0); break;
+	case 0x003|kOUI_HDMI:
+		cta_hdmi_block(x + 1, length);
+		if (edid_minor != 3)
+			fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
 		break;
-	case 0x02:
-		data_block = "Video Data Block";
-		printf("  %s\n", data_block.c_str());
-		cta_svd(x + 1, length, 0);
+	case 0x003|kOUI_HDMIForum:
+		if (previous_cta_tag != (0x003|kOUI_HDMI))
+			fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
+		if (have_hf_scdb || have_hf_vsdb)
+			fail("Duplicate HDMI Forum VSDB/SCDB\n");
+		cta_hf_scdb(x + 4, length - 3);
+		have_hf_vsdb = true;
 		break;
-	case 0x03:
-		oui = (x[3] << 16) + (x[2] << 8) + x[1];
-		printf("  Vendor-Specific Data Block, OUI 0x%06x", oui);
-		name = oui_name(oui);
-		if (oui == 0x000c03) {
-			data_block = "Vendor-Specific Data Block (HDMI)";
-			cta_hdmi_block(x + 1, length);
-			last_block_was_hdmi_vsdb = 1;
-			first_block = 0;
-			if (edid_minor != 3)
-				fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
-			return;
-		}
-		if (oui == 0xc45dd8) {
-			data_block = "Vendor-Specific Data Block (HDMI Forum)";
-			if (!last_block_was_hdmi_vsdb)
-				fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
-			if (have_hf_scdb || have_hf_vsdb)
-				fail("Duplicate HDMI Forum VSDB/SCDB\n");
-			printf(" (HDMI Forum)\n");
-			cta_hf_scdb(x + 4, length - 3);
-			have_hf_vsdb = 1;
-		} else if (name) {
-			data_block = std::string("Vendor-Specific Data Block (") + name + ")";
-			printf(" (%s)\n", name);
-			hex_block("    ", x + 4, length - 3);
-		} else {
-			printf("\n");
-			hex_block("    ", x + 4, length - 3);
-			data_block.clear();
-			warn("Unknown Vendor-Specific Data Block, OUI 0x%06x\n", oui);
-		}
-		break;
-	case 0x04:
-		data_block = "Speaker Allocation Data Block";
-		printf("  %s\n", data_block.c_str());
-		cta_sadb(x + 1, length);
-		break;
-	case 0x05:
-		printf("  VESA DTC Data Block\n");
-		hex_block("  ", x + 1, length);
-		break;
-	case 0x07:
-		printf("  Extended tag: ");
-		switch (x[1]) {
-		case 0x00:
-			data_block = "Video Capability Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_vcdb(x + 2, length - 1);
-			break;
-		case 0x01:
-			oui = (x[4] << 16) + (x[3] << 8) + x[2];
-			printf("Vendor-Specific Video Data Block, OUI 0x%06x", oui);
-			name = oui_name(oui);
-			if (oui == 0x90848b) {
-				data_block = "Vendor-Specific Video Data Block (HDR10+)";
-				printf(" (HDR10+)\n");
-				cta_hdr10plus(x + 5, length - 4);
-			} else if (name) {
-				data_block = std::string("Vendor-Specific Data Block (") + name + ")";
-				printf(" (%s)\n", name);
-				hex_block("    ", x + 5, length - 4);
-			} else {
-				printf("\n");
-				hex_block("    ", x + 5, length - 4);
-				data_block.clear();
-				warn("Unknown Extended Vendor-Specific Data Block, OUI 0x%06x\n", oui);
-			}
-			break;
-		case 0x02:
-			printf("VESA Video Display Device Data Block\n");
-			hex_block("  ", x + 2, length - 1);
-			break;
-		case 0x03:
-			printf("VESA Video Timing Block Extension\n");
-			hex_block("  ", x + 2, length - 1);
-			break;
-		case 0x04:
-			printf("Reserved for HDMI Video Data Block\n");
-			hex_block("  ", x + 2, length - 1);
-			break;
-		case 0x05:
-			data_block = "Colorimetry Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_colorimetry_block(x + 2, length - 1);
-			break;
-		case 0x06:
-			data_block = "HDR Static Metadata Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_hdr_static_metadata_block(x + 2, length - 1);
-			break;
-		case 0x07:
-			data_block = "HDR Dynamic Metadata Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_hdr_dyn_metadata_block(x + 2, length - 1);
-			break;
-		case 0x0d:
-			data_block = "Video Format Preference Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_vfpdb(x + 2, length - 1);
-			break;
-		case 0x0e:
-			data_block = "YCbCr 4:2:0 Video Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_svd(x + 2, length - 1, 1);
-			break;
-		case 0x0f:
-			data_block = "YCbCr 4:2:0 Capability Map Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_y420cmdb(x + 2, length - 1);
-			break;
-		case 0x10:
-			printf("Reserved for CTA Miscellaneous Audio Fields\n");
-			hex_block("  ", x + 2, length - 1);
-			break;
-		case 0x11:
-			printf("Vendor-Specific Audio Data Block\n");
-			hex_block("  ", x + 2, length - 1);
-			break;
-		case 0x12:
-			data_block = "HDMI Audio Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_hdmi_audio_block(x + 2, length - 1);
-			break;
-		case 0x13:
-			data_block = "Room Configuration Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_rcdb(x + 2, length - 1);
-			break;
-		case 0x14:
-			data_block = "Speaker Location Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_sldb(x + 2, length - 1);
-			break;
-		case 0x20:
-			printf("InfoFrame Data Block\n");
-			cta_ifdb(x + 2, length - 1);
-			break;
-		case 0x78:
-			data_block = "HDMI Forum EDID Extension Override Data Block";
-			printf("%s\n", data_block.c_str());
-			cta_hf_eeodb(x + 2, length - 1);
-			// This must be the first CTA block
-			if (!first_block)
-				fail("Block starts at a wrong offset\n");
-			break;
-		case 0x79:
-			data_block = "HDMI Forum Sink Capability Data Block";
-			printf("%s\n", data_block.c_str());
-			if (!last_block_was_hdmi_vsdb)
-				fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
-			if (have_hf_scdb || have_hf_vsdb)
-				fail("Duplicate HDMI Forum VSDB/SCDB\n");
-			if (x[2] || x[3])
-				printf("  Non-zero SCDB reserved fields!\n");
-			cta_hf_scdb(x + 4, length - 3);
-			have_hf_scdb = 1;
-			break;
-		default:
-			if (x[1] <= 12)
-				printf("Unknown CTA Video-Related");
-			else if (x[1] <= 31)
-				printf("Unknown CTA Audio-Related");
-			else if (x[1] >= 120 && x[1] <= 127)
-				printf("Unknown CTA HDMI-Related");
-			else
-				printf("Unknown CTA");
-			printf(" Data Block (extended tag 0x%02x, length %u)\n", x[1], length - 1);
-			hex_block("    ", x + 2, length - 1);
-			data_block.clear();
-			warn("Unknown Extended CTA Data Block 0x%02x\n", x[1]);
-			break;
-		}
+	case 0x004: cta_sadb(x + 1, length); break;
+	case 0x700: cta_vcdb(x + 2, length - 1); break;
+	case 0x701|kOUI_HDR10: cta_hdr10plus(x + 5, length - 4); break;
+	case 0x705: cta_colorimetry_block(x + 2, length - 1); break;
+	case 0x706: cta_hdr_static_metadata_block(x + 2, length - 1); break;
+	case 0x707: cta_hdr_dyn_metadata_block(x + 2, length - 1); break;
+	case 0x70d: cta_vfpdb(x + 2, length - 1); break;
+	case 0x70e: cta_svd(x + 2, length - 1, 1); break;
+	case 0x70f: cta_y420cmdb(x + 2, length - 1); break;
+	case 0x712: cta_hdmi_audio_block(x + 2, length - 1); break;
+	case 0x713: cta_rcdb(x + 2, length - 1); break;
+	case 0x714: cta_sldb(x + 2, length - 1); break;
+	case 0x720: cta_ifdb(x + 2, length - 1); break;
+	case 0x778: 
+		cta_hf_eeodb(x + 2, length - 1);
+		// This must be the first CTA block
+		if (cta_block_number != 0)
+			fail("Block starts at a wrong offset\n");
 		break;
-	default: {
-		unsigned tag = (*x & 0xe0) >> 5;
-		unsigned length = *x & 0x1f;
-		printf("  Unknown CTA tag 0x%02x, length %u\n", tag, length);
-		hex_block("    ", x + 1, length);
-		data_block.clear();
-		warn("Unknown CTA Data Block %u\n", tag);
+	case 0x779:
+		if (previous_cta_tag != (0x003|kOUI_HDMI))
+			fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
+		if (have_hf_scdb || have_hf_vsdb)
+			fail("Duplicate HDMI Forum VSDB/SCDB\n");
+		if (x[2] || x[3])
+			printf("    Non-zero SCDB reserved fields!\n");
+		cta_hf_scdb(x + 4, length - 3);
+		have_hf_scdb = true;
 		break;
+	default:
+		warn("Unknown %s\n", data_block.c_str());
+		hex_block("    ", x + 1 + extended + (ouinum ? 3 : 0), length - (extended + (ouinum ? 3 : 0)));
 	}
-	}
-	first_block = 0;
-	last_block_was_hdmi_vsdb = 0;
+	cta_block_number++;
+	previous_cta_tag = tag;
 }
 
 void edid_state::preparse_cta_block(const unsigned char *x)
-- 
2.21.0 (Apple Git-122.2)


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/5] edid-decode: add missing space
  2019-12-05  7:34 [PATCH 1/5] edid-decode: DisplayID additions joevt
                   ` (2 preceding siblings ...)
  2019-12-05  7:34 ` [PATCH 4/5] edid-decode: CTA extension block cleanup joevt
@ 2019-12-05  7:34 ` joevt
  3 siblings, 0 replies; 8+ messages in thread
From: joevt @ 2019-12-05  7:34 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

Space character was missing from error message.

Signed-off-by: Joe van Tunen <joevt@shaw.ca>
---
 parse-cta-block.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-cta-block.cpp b/parse-cta-block.cpp
index e52822a..5c1a679 100644
--- a/parse-cta-block.cpp
+++ b/parse-cta-block.cpp
@@ -1480,7 +1480,7 @@ void edid_state::parse_cta_block(const unsigned char *x)
 		fail("Both the serial number and the serial string are set\n");
 	if (!has_cta861_vic_1 && !has_640x480p60_est_timing)
 		fail("Required 640x480p60 timings are missing in the established timings"
-		     "and the SVD list (VIC 1)\n");
+		     " and the SVD list (VIC 1)\n");
 	if ((supported_hdmi_vic_vsb_codes & supported_hdmi_vic_codes) !=
 	    supported_hdmi_vic_codes)
 		fail("HDMI VIC Codes must have their CTA-861 VIC equivalents in the VSB\n");
-- 
2.21.0 (Apple Git-122.2)


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/5] edid-decode: CTA extension block cleanup
  2019-12-05  7:34 ` [PATCH 4/5] edid-decode: CTA extension block cleanup joevt
@ 2019-12-05  9:38   ` Hans Verkuil
  2019-12-05 13:52     ` Joe van Tunen
  0 siblings, 1 reply; 8+ messages in thread
From: Hans Verkuil @ 2019-12-05  9:38 UTC (permalink / raw)
  To: joevt; +Cc: linux-media

Hi Joe,

I've committed the other patches, but I'm not sure about this one. I'll look at
it again when I have more time. I might make my own implementation based on
ideas of this patch.

BTW, I made a real mess with the back/front porch timings. Thanks for fixing that!

Regards,

	Hans

On 12/5/19 8:34 AM, joevt wrote:
> Reduced tag parsing code.
> Added OUI min block length checks.
> 
> Signed-off-by: Joe van Tunen <joevt@shaw.ca>
> ---
>  edid-decode.h       |   1 +
>  parse-cta-block.cpp | 336 ++++++++++++++++++--------------------------
>  2 files changed, 138 insertions(+), 199 deletions(-)
> 
> diff --git a/edid-decode.h b/edid-decode.h
> index 83ded83..758bdcf 100644
> --- a/edid-decode.h
> +++ b/edid-decode.h
> @@ -111,6 +111,7 @@ struct edid_state {
>  	void cta_y420cmdb(const unsigned char *x, unsigned length);
>  	void cta_vfpdb(const unsigned char *x, unsigned length);
>  	void cta_hdmi_block(const unsigned char *x, unsigned length);
> +	void cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum);
>  	void cta_block(const unsigned char *x);
>  	void preparse_cta_block(const unsigned char *x);
>  	void parse_cta_block(const unsigned char *x);
> diff --git a/parse-cta-block.cpp b/parse-cta-block.cpp
> index dea87c1..e52822a 100644
> --- a/parse-cta-block.cpp
> +++ b/parse-cta-block.cpp
> @@ -375,6 +375,11 @@ void edid_state::cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420)
>  
>  		if (vic == 1 && !for_ycbcr420)
>  			has_cta861_vic_1 = 1;
> +
> +		// vics and has_vic are basically the same (if has_vic was not bool), except vics
> +		// is built after preparse (during parse) which allows errors for duplicates to be
> +		// output in parse order. has_vic is built during preparse and is used when vics
> +		// from other blocks need to be checked.
>  		if (++vics[vic][for_ycbcr420] == 2)
>  			fail("Duplicate %sVIC %u\n", for_ycbcr420 ? "YCbCr 4:2:0 " : "", vic);
>  		if (for_ycbcr420 && has_vic[0][vic])
> @@ -485,7 +490,6 @@ void edid_state::cta_hdmi_block(const unsigned char *x, unsigned length)
>  {
>  	unsigned len_vic, len_3d;
>  
> -	printf(" (HDMI)\n");
>  	printf("    Source physical address %u.%u.%u.%u\n", x[3] >> 4, x[3] & 0x0f,
>  	       x[4] >> 4, x[4] & 0x0f);
>  
> @@ -1228,10 +1232,20 @@ static void cta_hdmi_audio_block(const unsigned char *x, unsigned length)
>  		x += 4;
>  	}
>  }
> +const unsigned kOUI_Unknown   = 1<<12;
> +const unsigned kOUI_HDMI      = 2<<12;
> +const unsigned kOUI_HDMIForum = 3<<12;
> +const unsigned kOUI_HDR10     = 4<<12;
>  
> -static const char *oui_name(unsigned oui)
> +static const char *oui_name(unsigned oui, unsigned *ouinum = NULL)
>  {
> +	unsigned ouinumscratch;
> +	if (!ouinum) ouinum = &ouinumscratch;
> +	*ouinum = kOUI_Unknown;
>  	switch (oui) {
> +	case 0x000c03: *ouinum = kOUI_HDMI      ; return "HDMI";
> +	case 0xc45dd8: *ouinum = kOUI_HDMIForum ; return "HDMI Forum";
> +	case 0x90848b: *ouinum = kOUI_HDR10     ; return "HDR10+";
>  	case 0x00001a: return "AMD";
>  	case 0x00044b: return "NVIDIA";
>  	case 0x000c6e: return "ASUS";
> @@ -1244,212 +1258,136 @@ static const char *oui_name(unsigned oui)
>  	}
>  }
>  
> +void edid_state::cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum)
> +{
> +	char buf[10];
> +	unsigned oui;
> +
> +	if (length < 3) {
> +		oui = 0xffffffff;
> +		sprintf(buf, "?");
> +	} else {
> +		oui = (x[2] << 16) + (x[1] << 8) + x[0];
> +		sprintf(buf, "0x%06x", oui);
> +	}
> +	
> +	const char *ouiname = oui_name(oui, ouinum);
> +	std::string name = std::string(block_name) + ", OUI " + buf;
> +	if (ouiname) name += std::string(" (") + ouiname + ")";
> +	data_block = name;
> +	
> +	if (oui == 0xffffffff)
> +		fail("CTA data block is not long enough to contain an OUI\n");
> +}
> +
> +#define data_block_o(n) cta_oui(n, x + 1 + extended, length - extended, &ouinum)
> +
>  void edid_state::cta_block(const unsigned char *x)
>  {
> -	static int last_block_was_hdmi_vsdb;
> -	static int have_hf_vsdb, have_hf_scdb;
> -	static int first_block = 1;
> +	static unsigned previous_cta_tag = 0;
> +	static bool have_hf_vsdb = false;
> +	static bool have_hf_scdb = false;
> +	static unsigned cta_block_number = 0;
> +
>  	unsigned length = x[0] & 0x1f;
> -	const char *name;
> -	unsigned oui;
> +	unsigned ouinum = 0;
> +	unsigned tag=(x[0] & 0xe0) >> 5;
> +	unsigned extended = tag == 0x07 ? 1 : 0;
> +	if (extended) tag = 0x700 + x[1];
> +
> +	switch (tag) {
> +	case 0x001: data_block = "Audio Data Block"; break;
> +	case 0x002: data_block = "Video Data Block"; break;
> +	case 0x003: data_block_o("Vendor-Specific Data Block"); break;
> +	case 0x004: data_block = "Speaker Allocation Data Block"; break;
> +	case 0x005: data_block = "VESA DTC Data Block"; break; // not implemented
> +
> +	case 0x700: data_block = "Video Capability Data Block"; break;
> +	case 0x701: data_block_o("Vendor-Specific Video Data Block"); break;
> +	case 0x702: data_block = "VESA Video Display Device Data Block"; break; // not implemented
> +	case 0x703: data_block = "VESA Video Timing Block Extension"; break; // not implemented
> +	case 0x704: data_block = "Reserved for HDMI Video Data Block"; break; // reserved
> +	case 0x705: data_block = "Colorimetry Data Block"; break;
> +	case 0x706: data_block = "HDR Static Metadata Data Block"; break;
> +	case 0x707: data_block = "HDR Dynamic Metadata Data Block"; break;
> +
> +	case 0x70d: data_block = "Video Format Preference Data Block"; break;
> +	case 0x70e: data_block = "YCbCr 4:2:0 Video Data Block"; break;
> +	case 0x70f: data_block = "YCbCr 4:2:0 Capability Map Data Block"; break;
> +	case 0x710: data_block = "Reserved for CTA Miscellaneous Audio Fields"; break; // reserved
> +	case 0x711: data_block_o("Vendor-Specific Audio Data Block"); break; // no vendors implemented
> +	case 0x712: data_block = "HDMI Audio Data Block"; break;
> +	case 0x713: data_block = "Room Configuration Data Block"; break;
> +	case 0x714: data_block = "Speaker Location Data Block"; break;
> +
> +	case 0x720: data_block = "InfoFrame Data Block"; break;
> +
> +	case 0x778: data_block = "HDMI Forum EDID Extension Override Data Block"; break;
> +	case 0x779: data_block = "HDMI Forum Sink Capability Data Block"; break;
> +	default:
> +		     if (tag < 0x700) data_block = "Unknown CTA Data Block";
> +		else if (tag < 0x70d) data_block = "Unknown CTA Video-Related Data Block";
> +		else if (tag < 0x720) data_block = "Unknown CTA Audio-Related Data Block";
> +		else if (tag < 0x778) data_block = "Unknown CTA Data Block";
> +		else if (tag < 0x780) data_block = "Unknown CTA HDMI-Related Data Block";
> +		else                  data_block = "Unknown CTA Data Block";
> +		data_block += std::string(" (") + (extended ? "extended " : "") + "tag " + utohex(tag & 0xff) + ")";
> +	}
>  
> -	switch ((x[0] & 0xe0) >> 5) {
> -	case 0x01:
> -		data_block = "Audio Data Block";
> -		printf("  %s\n", data_block.c_str());
> -		cta_audio_block(x + 1, length);
> +	printf("  %s\n", data_block.c_str());
> +	
> +	tag |= ouinum;
> +	switch (tag) {
> +	case 0x001: cta_audio_block(x + 1, length); break;
> +	case 0x002: cta_svd(x + 1, length, 0); break;
> +	case 0x003|kOUI_HDMI:
> +		cta_hdmi_block(x + 1, length);
> +		if (edid_minor != 3)
> +			fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
>  		break;
> -	case 0x02:
> -		data_block = "Video Data Block";
> -		printf("  %s\n", data_block.c_str());
> -		cta_svd(x + 1, length, 0);
> +	case 0x003|kOUI_HDMIForum:
> +		if (previous_cta_tag != (0x003|kOUI_HDMI))
> +			fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
> +		if (have_hf_scdb || have_hf_vsdb)
> +			fail("Duplicate HDMI Forum VSDB/SCDB\n");
> +		cta_hf_scdb(x + 4, length - 3);
> +		have_hf_vsdb = true;
>  		break;
> -	case 0x03:
> -		oui = (x[3] << 16) + (x[2] << 8) + x[1];
> -		printf("  Vendor-Specific Data Block, OUI 0x%06x", oui);
> -		name = oui_name(oui);
> -		if (oui == 0x000c03) {
> -			data_block = "Vendor-Specific Data Block (HDMI)";
> -			cta_hdmi_block(x + 1, length);
> -			last_block_was_hdmi_vsdb = 1;
> -			first_block = 0;
> -			if (edid_minor != 3)
> -				fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
> -			return;
> -		}
> -		if (oui == 0xc45dd8) {
> -			data_block = "Vendor-Specific Data Block (HDMI Forum)";
> -			if (!last_block_was_hdmi_vsdb)
> -				fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
> -			if (have_hf_scdb || have_hf_vsdb)
> -				fail("Duplicate HDMI Forum VSDB/SCDB\n");
> -			printf(" (HDMI Forum)\n");
> -			cta_hf_scdb(x + 4, length - 3);
> -			have_hf_vsdb = 1;
> -		} else if (name) {
> -			data_block = std::string("Vendor-Specific Data Block (") + name + ")";
> -			printf(" (%s)\n", name);
> -			hex_block("    ", x + 4, length - 3);
> -		} else {
> -			printf("\n");
> -			hex_block("    ", x + 4, length - 3);
> -			data_block.clear();
> -			warn("Unknown Vendor-Specific Data Block, OUI 0x%06x\n", oui);
> -		}
> -		break;
> -	case 0x04:
> -		data_block = "Speaker Allocation Data Block";
> -		printf("  %s\n", data_block.c_str());
> -		cta_sadb(x + 1, length);
> -		break;
> -	case 0x05:
> -		printf("  VESA DTC Data Block\n");
> -		hex_block("  ", x + 1, length);
> -		break;
> -	case 0x07:
> -		printf("  Extended tag: ");
> -		switch (x[1]) {
> -		case 0x00:
> -			data_block = "Video Capability Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_vcdb(x + 2, length - 1);
> -			break;
> -		case 0x01:
> -			oui = (x[4] << 16) + (x[3] << 8) + x[2];
> -			printf("Vendor-Specific Video Data Block, OUI 0x%06x", oui);
> -			name = oui_name(oui);
> -			if (oui == 0x90848b) {
> -				data_block = "Vendor-Specific Video Data Block (HDR10+)";
> -				printf(" (HDR10+)\n");
> -				cta_hdr10plus(x + 5, length - 4);
> -			} else if (name) {
> -				data_block = std::string("Vendor-Specific Data Block (") + name + ")";
> -				printf(" (%s)\n", name);
> -				hex_block("    ", x + 5, length - 4);
> -			} else {
> -				printf("\n");
> -				hex_block("    ", x + 5, length - 4);
> -				data_block.clear();
> -				warn("Unknown Extended Vendor-Specific Data Block, OUI 0x%06x\n", oui);
> -			}
> -			break;
> -		case 0x02:
> -			printf("VESA Video Display Device Data Block\n");
> -			hex_block("  ", x + 2, length - 1);
> -			break;
> -		case 0x03:
> -			printf("VESA Video Timing Block Extension\n");
> -			hex_block("  ", x + 2, length - 1);
> -			break;
> -		case 0x04:
> -			printf("Reserved for HDMI Video Data Block\n");
> -			hex_block("  ", x + 2, length - 1);
> -			break;
> -		case 0x05:
> -			data_block = "Colorimetry Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_colorimetry_block(x + 2, length - 1);
> -			break;
> -		case 0x06:
> -			data_block = "HDR Static Metadata Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_hdr_static_metadata_block(x + 2, length - 1);
> -			break;
> -		case 0x07:
> -			data_block = "HDR Dynamic Metadata Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_hdr_dyn_metadata_block(x + 2, length - 1);
> -			break;
> -		case 0x0d:
> -			data_block = "Video Format Preference Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_vfpdb(x + 2, length - 1);
> -			break;
> -		case 0x0e:
> -			data_block = "YCbCr 4:2:0 Video Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_svd(x + 2, length - 1, 1);
> -			break;
> -		case 0x0f:
> -			data_block = "YCbCr 4:2:0 Capability Map Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_y420cmdb(x + 2, length - 1);
> -			break;
> -		case 0x10:
> -			printf("Reserved for CTA Miscellaneous Audio Fields\n");
> -			hex_block("  ", x + 2, length - 1);
> -			break;
> -		case 0x11:
> -			printf("Vendor-Specific Audio Data Block\n");
> -			hex_block("  ", x + 2, length - 1);
> -			break;
> -		case 0x12:
> -			data_block = "HDMI Audio Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_hdmi_audio_block(x + 2, length - 1);
> -			break;
> -		case 0x13:
> -			data_block = "Room Configuration Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_rcdb(x + 2, length - 1);
> -			break;
> -		case 0x14:
> -			data_block = "Speaker Location Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_sldb(x + 2, length - 1);
> -			break;
> -		case 0x20:
> -			printf("InfoFrame Data Block\n");
> -			cta_ifdb(x + 2, length - 1);
> -			break;
> -		case 0x78:
> -			data_block = "HDMI Forum EDID Extension Override Data Block";
> -			printf("%s\n", data_block.c_str());
> -			cta_hf_eeodb(x + 2, length - 1);
> -			// This must be the first CTA block
> -			if (!first_block)
> -				fail("Block starts at a wrong offset\n");
> -			break;
> -		case 0x79:
> -			data_block = "HDMI Forum Sink Capability Data Block";
> -			printf("%s\n", data_block.c_str());
> -			if (!last_block_was_hdmi_vsdb)
> -				fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
> -			if (have_hf_scdb || have_hf_vsdb)
> -				fail("Duplicate HDMI Forum VSDB/SCDB\n");
> -			if (x[2] || x[3])
> -				printf("  Non-zero SCDB reserved fields!\n");
> -			cta_hf_scdb(x + 4, length - 3);
> -			have_hf_scdb = 1;
> -			break;
> -		default:
> -			if (x[1] <= 12)
> -				printf("Unknown CTA Video-Related");
> -			else if (x[1] <= 31)
> -				printf("Unknown CTA Audio-Related");
> -			else if (x[1] >= 120 && x[1] <= 127)
> -				printf("Unknown CTA HDMI-Related");
> -			else
> -				printf("Unknown CTA");
> -			printf(" Data Block (extended tag 0x%02x, length %u)\n", x[1], length - 1);
> -			hex_block("    ", x + 2, length - 1);
> -			data_block.clear();
> -			warn("Unknown Extended CTA Data Block 0x%02x\n", x[1]);
> -			break;
> -		}
> +	case 0x004: cta_sadb(x + 1, length); break;
> +	case 0x700: cta_vcdb(x + 2, length - 1); break;
> +	case 0x701|kOUI_HDR10: cta_hdr10plus(x + 5, length - 4); break;
> +	case 0x705: cta_colorimetry_block(x + 2, length - 1); break;
> +	case 0x706: cta_hdr_static_metadata_block(x + 2, length - 1); break;
> +	case 0x707: cta_hdr_dyn_metadata_block(x + 2, length - 1); break;
> +	case 0x70d: cta_vfpdb(x + 2, length - 1); break;
> +	case 0x70e: cta_svd(x + 2, length - 1, 1); break;
> +	case 0x70f: cta_y420cmdb(x + 2, length - 1); break;
> +	case 0x712: cta_hdmi_audio_block(x + 2, length - 1); break;
> +	case 0x713: cta_rcdb(x + 2, length - 1); break;
> +	case 0x714: cta_sldb(x + 2, length - 1); break;
> +	case 0x720: cta_ifdb(x + 2, length - 1); break;
> +	case 0x778: 
> +		cta_hf_eeodb(x + 2, length - 1);
> +		// This must be the first CTA block
> +		if (cta_block_number != 0)
> +			fail("Block starts at a wrong offset\n");
>  		break;
> -	default: {
> -		unsigned tag = (*x & 0xe0) >> 5;
> -		unsigned length = *x & 0x1f;
> -		printf("  Unknown CTA tag 0x%02x, length %u\n", tag, length);
> -		hex_block("    ", x + 1, length);
> -		data_block.clear();
> -		warn("Unknown CTA Data Block %u\n", tag);
> +	case 0x779:
> +		if (previous_cta_tag != (0x003|kOUI_HDMI))
> +			fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
> +		if (have_hf_scdb || have_hf_vsdb)
> +			fail("Duplicate HDMI Forum VSDB/SCDB\n");
> +		if (x[2] || x[3])
> +			printf("    Non-zero SCDB reserved fields!\n");
> +		cta_hf_scdb(x + 4, length - 3);
> +		have_hf_scdb = true;
>  		break;
> +	default:
> +		warn("Unknown %s\n", data_block.c_str());
> +		hex_block("    ", x + 1 + extended + (ouinum ? 3 : 0), length - (extended + (ouinum ? 3 : 0)));
>  	}
> -	}
> -	first_block = 0;
> -	last_block_was_hdmi_vsdb = 0;
> +	cta_block_number++;
> +	previous_cta_tag = tag;
>  }
>  
>  void edid_state::preparse_cta_block(const unsigned char *x)
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/5] edid-decode: CTA extension block cleanup
  2019-12-05  9:38   ` Hans Verkuil
@ 2019-12-05 13:52     ` Joe van Tunen
  2020-01-15 11:52       ` Hans Verkuil
  0 siblings, 1 reply; 8+ messages in thread
From: Joe van Tunen @ 2019-12-05 13:52 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

I did some evil things with the switch statement but I think it at least looks nice (for some definitions of nice).

The main idea is to be more like DisplayID. First, there's a switch to get the data_block name. The data_block name is required for error handling so it is obtained first. Instead of a nested switch, it combines the tag and extended tag bytes. The switch obtains an oui for vendor specific data blocks. There's three tags that parse oui (I added oui parsing for audio but none of those are decoded) and now they all use the same oui function.

Second, there's a separate switch for the decode part. This switch combines tag, extended tag, and oui. The duplicate code was unduplicated. There's only one default for outputting hex of unparsed data blocks (instead of 12) - whether they have a normal tag, extended tag, a tag with an oui, or an extended tag with an oui.


On 2019-12-05, 1:38 AM, "Hans Verkuil" <hverkuil@xs4all.nl> wrote:

    Hi Joe,
    
    I've committed the other patches, but I'm not sure about this one. I'll look at
    it again when I have more time. I might make my own implementation based on
    ideas of this patch.
    
    BTW, I made a real mess with the back/front porch timings. Thanks for fixing that!
    
    Regards,
    
    	Hans
    
    On 12/5/19 8:34 AM, joevt wrote:
    > Reduced tag parsing code.
    > Added OUI min block length checks.
    > 
    > Signed-off-by: Joe van Tunen <joevt@shaw.ca>
    > ---
    >  edid-decode.h       |   1 +
    >  parse-cta-block.cpp | 336 ++++++++++++++++++--------------------------
    >  2 files changed, 138 insertions(+), 199 deletions(-)
    > 
    > diff --git a/edid-decode.h b/edid-decode.h
    > index 83ded83..758bdcf 100644
    > --- a/edid-decode.h
    > +++ b/edid-decode.h
    > @@ -111,6 +111,7 @@ struct edid_state {
    >  	void cta_y420cmdb(const unsigned char *x, unsigned length);
    >  	void cta_vfpdb(const unsigned char *x, unsigned length);
    >  	void cta_hdmi_block(const unsigned char *x, unsigned length);
    > +	void cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum);
    >  	void cta_block(const unsigned char *x);
    >  	void preparse_cta_block(const unsigned char *x);
    >  	void parse_cta_block(const unsigned char *x);
    > diff --git a/parse-cta-block.cpp b/parse-cta-block.cpp
    > index dea87c1..e52822a 100644
    > --- a/parse-cta-block.cpp
    > +++ b/parse-cta-block.cpp
    > @@ -375,6 +375,11 @@ void edid_state::cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420)
    >  
    >  		if (vic == 1 && !for_ycbcr420)
    >  			has_cta861_vic_1 = 1;
    > +
    > +		// vics and has_vic are basically the same (if has_vic was not bool), except vics
    > +		// is built after preparse (during parse) which allows errors for duplicates to be
    > +		// output in parse order. has_vic is built during preparse and is used when vics
    > +		// from other blocks need to be checked.
    >  		if (++vics[vic][for_ycbcr420] == 2)
    >  			fail("Duplicate %sVIC %u\n", for_ycbcr420 ? "YCbCr 4:2:0 " : "", vic);
    >  		if (for_ycbcr420 && has_vic[0][vic])
    > @@ -485,7 +490,6 @@ void edid_state::cta_hdmi_block(const unsigned char *x, unsigned length)
    >  {
    >  	unsigned len_vic, len_3d;
    >  
    > -	printf(" (HDMI)\n");
    >  	printf("    Source physical address %u.%u.%u.%u\n", x[3] >> 4, x[3] & 0x0f,
    >  	       x[4] >> 4, x[4] & 0x0f);
    >  
    > @@ -1228,10 +1232,20 @@ static void cta_hdmi_audio_block(const unsigned char *x, unsigned length)
    >  		x += 4;
    >  	}
    >  }
    > +const unsigned kOUI_Unknown   = 1<<12;
    > +const unsigned kOUI_HDMI      = 2<<12;
    > +const unsigned kOUI_HDMIForum = 3<<12;
    > +const unsigned kOUI_HDR10     = 4<<12;
    >  
    > -static const char *oui_name(unsigned oui)
    > +static const char *oui_name(unsigned oui, unsigned *ouinum = NULL)
    >  {
    > +	unsigned ouinumscratch;
    > +	if (!ouinum) ouinum = &ouinumscratch;
    > +	*ouinum = kOUI_Unknown;
    >  	switch (oui) {
    > +	case 0x000c03: *ouinum = kOUI_HDMI      ; return "HDMI";
    > +	case 0xc45dd8: *ouinum = kOUI_HDMIForum ; return "HDMI Forum";
    > +	case 0x90848b: *ouinum = kOUI_HDR10     ; return "HDR10+";
    >  	case 0x00001a: return "AMD";
    >  	case 0x00044b: return "NVIDIA";
    >  	case 0x000c6e: return "ASUS";
    > @@ -1244,212 +1258,136 @@ static const char *oui_name(unsigned oui)
    >  	}
    >  }
    >  
    > +void edid_state::cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum)
    > +{
    > +	char buf[10];
    > +	unsigned oui;
    > +
    > +	if (length < 3) {
    > +		oui = 0xffffffff;
    > +		sprintf(buf, "?");
    > +	} else {
    > +		oui = (x[2] << 16) + (x[1] << 8) + x[0];
    > +		sprintf(buf, "0x%06x", oui);
    > +	}
    > +
    > +	const char *ouiname = oui_name(oui, ouinum);
    > +	std::string name = std::string(block_name) + ", OUI " + buf;
    > +	if (ouiname) name += std::string(" (") + ouiname + ")";
    > +	data_block = name;
    > +
    > +	if (oui == 0xffffffff)
    > +		fail("CTA data block is not long enough to contain an OUI\n");
    > +}
    > +
    > +#define data_block_o(n) cta_oui(n, x + 1 + extended, length - extended, &ouinum)
    > +
    >  void edid_state::cta_block(const unsigned char *x)
    >  {
    > -	static int last_block_was_hdmi_vsdb;
    > -	static int have_hf_vsdb, have_hf_scdb;
    > -	static int first_block = 1;
    > +	static unsigned previous_cta_tag = 0;
    > +	static bool have_hf_vsdb = false;
    > +	static bool have_hf_scdb = false;
    > +	static unsigned cta_block_number = 0;
    > +
    >  	unsigned length = x[0] & 0x1f;
    > -	const char *name;
    > -	unsigned oui;
    > +	unsigned ouinum = 0;
    > +	unsigned tag=(x[0] & 0xe0) >> 5;
    > +	unsigned extended = tag == 0x07 ? 1 : 0;
    > +	if (extended) tag = 0x700 + x[1];
    > +
    > +	switch (tag) {
    > +	case 0x001: data_block = "Audio Data Block"; break;
    > +	case 0x002: data_block = "Video Data Block"; break;
    > +	case 0x003: data_block_o("Vendor-Specific Data Block"); break;
    > +	case 0x004: data_block = "Speaker Allocation Data Block"; break;
    > +	case 0x005: data_block = "VESA DTC Data Block"; break; // not implemented
    > +
    > +	case 0x700: data_block = "Video Capability Data Block"; break;
    > +	case 0x701: data_block_o("Vendor-Specific Video Data Block"); break;
    > +	case 0x702: data_block = "VESA Video Display Device Data Block"; break; // not implemented
    > +	case 0x703: data_block = "VESA Video Timing Block Extension"; break; // not implemented
    > +	case 0x704: data_block = "Reserved for HDMI Video Data Block"; break; // reserved
    > +	case 0x705: data_block = "Colorimetry Data Block"; break;
    > +	case 0x706: data_block = "HDR Static Metadata Data Block"; break;
    > +	case 0x707: data_block = "HDR Dynamic Metadata Data Block"; break;
    > +
    > +	case 0x70d: data_block = "Video Format Preference Data Block"; break;
    > +	case 0x70e: data_block = "YCbCr 4:2:0 Video Data Block"; break;
    > +	case 0x70f: data_block = "YCbCr 4:2:0 Capability Map Data Block"; break;
    > +	case 0x710: data_block = "Reserved for CTA Miscellaneous Audio Fields"; break; // reserved
    > +	case 0x711: data_block_o("Vendor-Specific Audio Data Block"); break; // no vendors implemented
    > +	case 0x712: data_block = "HDMI Audio Data Block"; break;
    > +	case 0x713: data_block = "Room Configuration Data Block"; break;
    > +	case 0x714: data_block = "Speaker Location Data Block"; break;
    > +
    > +	case 0x720: data_block = "InfoFrame Data Block"; break;
    > +
    > +	case 0x778: data_block = "HDMI Forum EDID Extension Override Data Block"; break;
    > +	case 0x779: data_block = "HDMI Forum Sink Capability Data Block"; break;
    > +	default:
    > +		     if (tag < 0x700) data_block = "Unknown CTA Data Block";
    > +		else if (tag < 0x70d) data_block = "Unknown CTA Video-Related Data Block";
    > +		else if (tag < 0x720) data_block = "Unknown CTA Audio-Related Data Block";
    > +		else if (tag < 0x778) data_block = "Unknown CTA Data Block";
    > +		else if (tag < 0x780) data_block = "Unknown CTA HDMI-Related Data Block";
    > +		else                  data_block = "Unknown CTA Data Block";
    > +		data_block += std::string(" (") + (extended ? "extended " : "") + "tag " + utohex(tag & 0xff) + ")";
    > +	}
    >  
    > -	switch ((x[0] & 0xe0) >> 5) {
    > -	case 0x01:
    > -		data_block = "Audio Data Block";
    > -		printf("  %s\n", data_block.c_str());
    > -		cta_audio_block(x + 1, length);
    > +	printf("  %s\n", data_block.c_str());
    > +
    > +	tag |= ouinum;
    > +	switch (tag) {
    > +	case 0x001: cta_audio_block(x + 1, length); break;
    > +	case 0x002: cta_svd(x + 1, length, 0); break;
    > +	case 0x003|kOUI_HDMI:
    > +		cta_hdmi_block(x + 1, length);
    > +		if (edid_minor != 3)
    > +			fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
    >  		break;
    > -	case 0x02:
    > -		data_block = "Video Data Block";
    > -		printf("  %s\n", data_block.c_str());
    > -		cta_svd(x + 1, length, 0);
    > +	case 0x003|kOUI_HDMIForum:
    > +		if (previous_cta_tag != (0x003|kOUI_HDMI))
    > +			fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
    > +		if (have_hf_scdb || have_hf_vsdb)
    > +			fail("Duplicate HDMI Forum VSDB/SCDB\n");
    > +		cta_hf_scdb(x + 4, length - 3);
    > +		have_hf_vsdb = true;
    >  		break;
    > -	case 0x03:
    > -		oui = (x[3] << 16) + (x[2] << 8) + x[1];
    > -		printf("  Vendor-Specific Data Block, OUI 0x%06x", oui);
    > -		name = oui_name(oui);
    > -		if (oui == 0x000c03) {
    > -			data_block = "Vendor-Specific Data Block (HDMI)";
    > -			cta_hdmi_block(x + 1, length);
    > -			last_block_was_hdmi_vsdb = 1;
    > -			first_block = 0;
    > -			if (edid_minor != 3)
    > -				fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
    > -			return;
    > -		}
    > -		if (oui == 0xc45dd8) {
    > -			data_block = "Vendor-Specific Data Block (HDMI Forum)";
    > -			if (!last_block_was_hdmi_vsdb)
    > -				fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
    > -			if (have_hf_scdb || have_hf_vsdb)
    > -				fail("Duplicate HDMI Forum VSDB/SCDB\n");
    > -			printf(" (HDMI Forum)\n");
    > -			cta_hf_scdb(x + 4, length - 3);
    > -			have_hf_vsdb = 1;
    > -		} else if (name) {
    > -			data_block = std::string("Vendor-Specific Data Block (") + name + ")";
    > -			printf(" (%s)\n", name);
    > -			hex_block("    ", x + 4, length - 3);
    > -		} else {
    > -			printf("\n");
    > -			hex_block("    ", x + 4, length - 3);
    > -			data_block.clear();
    > -			warn("Unknown Vendor-Specific Data Block, OUI 0x%06x\n", oui);
    > -		}
    > -		break;
    > -	case 0x04:
    > -		data_block = "Speaker Allocation Data Block";
    > -		printf("  %s\n", data_block.c_str());
    > -		cta_sadb(x + 1, length);
    > -		break;
    > -	case 0x05:
    > -		printf("  VESA DTC Data Block\n");
    > -		hex_block("  ", x + 1, length);
    > -		break;
    > -	case 0x07:
    > -		printf("  Extended tag: ");
    > -		switch (x[1]) {
    > -		case 0x00:
    > -			data_block = "Video Capability Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_vcdb(x + 2, length - 1);
    > -			break;
    > -		case 0x01:
    > -			oui = (x[4] << 16) + (x[3] << 8) + x[2];
    > -			printf("Vendor-Specific Video Data Block, OUI 0x%06x", oui);
    > -			name = oui_name(oui);
    > -			if (oui == 0x90848b) {
    > -				data_block = "Vendor-Specific Video Data Block (HDR10+)";
    > -				printf(" (HDR10+)\n");
    > -				cta_hdr10plus(x + 5, length - 4);
    > -			} else if (name) {
    > -				data_block = std::string("Vendor-Specific Data Block (") + name + ")";
    > -				printf(" (%s)\n", name);
    > -				hex_block("    ", x + 5, length - 4);
    > -			} else {
    > -				printf("\n");
    > -				hex_block("    ", x + 5, length - 4);
    > -				data_block.clear();
    > -				warn("Unknown Extended Vendor-Specific Data Block, OUI 0x%06x\n", oui);
    > -			}
    > -			break;
    > -		case 0x02:
    > -			printf("VESA Video Display Device Data Block\n");
    > -			hex_block("  ", x + 2, length - 1);
    > -			break;
    > -		case 0x03:
    > -			printf("VESA Video Timing Block Extension\n");
    > -			hex_block("  ", x + 2, length - 1);
    > -			break;
    > -		case 0x04:
    > -			printf("Reserved for HDMI Video Data Block\n");
    > -			hex_block("  ", x + 2, length - 1);
    > -			break;
    > -		case 0x05:
    > -			data_block = "Colorimetry Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_colorimetry_block(x + 2, length - 1);
    > -			break;
    > -		case 0x06:
    > -			data_block = "HDR Static Metadata Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_hdr_static_metadata_block(x + 2, length - 1);
    > -			break;
    > -		case 0x07:
    > -			data_block = "HDR Dynamic Metadata Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_hdr_dyn_metadata_block(x + 2, length - 1);
    > -			break;
    > -		case 0x0d:
    > -			data_block = "Video Format Preference Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_vfpdb(x + 2, length - 1);
    > -			break;
    > -		case 0x0e:
    > -			data_block = "YCbCr 4:2:0 Video Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_svd(x + 2, length - 1, 1);
    > -			break;
    > -		case 0x0f:
    > -			data_block = "YCbCr 4:2:0 Capability Map Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_y420cmdb(x + 2, length - 1);
    > -			break;
    > -		case 0x10:
    > -			printf("Reserved for CTA Miscellaneous Audio Fields\n");
    > -			hex_block("  ", x + 2, length - 1);
    > -			break;
    > -		case 0x11:
    > -			printf("Vendor-Specific Audio Data Block\n");
    > -			hex_block("  ", x + 2, length - 1);
    > -			break;
    > -		case 0x12:
    > -			data_block = "HDMI Audio Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_hdmi_audio_block(x + 2, length - 1);
    > -			break;
    > -		case 0x13:
    > -			data_block = "Room Configuration Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_rcdb(x + 2, length - 1);
    > -			break;
    > -		case 0x14:
    > -			data_block = "Speaker Location Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_sldb(x + 2, length - 1);
    > -			break;
    > -		case 0x20:
    > -			printf("InfoFrame Data Block\n");
    > -			cta_ifdb(x + 2, length - 1);
    > -			break;
    > -		case 0x78:
    > -			data_block = "HDMI Forum EDID Extension Override Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			cta_hf_eeodb(x + 2, length - 1);
    > -			// This must be the first CTA block
    > -			if (!first_block)
    > -				fail("Block starts at a wrong offset\n");
    > -			break;
    > -		case 0x79:
    > -			data_block = "HDMI Forum Sink Capability Data Block";
    > -			printf("%s\n", data_block.c_str());
    > -			if (!last_block_was_hdmi_vsdb)
    > -				fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
    > -			if (have_hf_scdb || have_hf_vsdb)
    > -				fail("Duplicate HDMI Forum VSDB/SCDB\n");
    > -			if (x[2] || x[3])
    > -				printf("  Non-zero SCDB reserved fields!\n");
    > -			cta_hf_scdb(x + 4, length - 3);
    > -			have_hf_scdb = 1;
    > -			break;
    > -		default:
    > -			if (x[1] <= 12)
    > -				printf("Unknown CTA Video-Related");
    > -			else if (x[1] <= 31)
    > -				printf("Unknown CTA Audio-Related");
    > -			else if (x[1] >= 120 && x[1] <= 127)
    > -				printf("Unknown CTA HDMI-Related");
    > -			else
    > -				printf("Unknown CTA");
    > -			printf(" Data Block (extended tag 0x%02x, length %u)\n", x[1], length - 1);
    > -			hex_block("    ", x + 2, length - 1);
    > -			data_block.clear();
    > -			warn("Unknown Extended CTA Data Block 0x%02x\n", x[1]);
    > -			break;
    > -		}
    > +	case 0x004: cta_sadb(x + 1, length); break;
    > +	case 0x700: cta_vcdb(x + 2, length - 1); break;
    > +	case 0x701|kOUI_HDR10: cta_hdr10plus(x + 5, length - 4); break;
    > +	case 0x705: cta_colorimetry_block(x + 2, length - 1); break;
    > +	case 0x706: cta_hdr_static_metadata_block(x + 2, length - 1); break;
    > +	case 0x707: cta_hdr_dyn_metadata_block(x + 2, length - 1); break;
    > +	case 0x70d: cta_vfpdb(x + 2, length - 1); break;
    > +	case 0x70e: cta_svd(x + 2, length - 1, 1); break;
    > +	case 0x70f: cta_y420cmdb(x + 2, length - 1); break;
    > +	case 0x712: cta_hdmi_audio_block(x + 2, length - 1); break;
    > +	case 0x713: cta_rcdb(x + 2, length - 1); break;
    > +	case 0x714: cta_sldb(x + 2, length - 1); break;
    > +	case 0x720: cta_ifdb(x + 2, length - 1); break;
    > +	case 0x778: 
    > +		cta_hf_eeodb(x + 2, length - 1);
    > +		// This must be the first CTA block
    > +		if (cta_block_number != 0)
    > +			fail("Block starts at a wrong offset\n");
    >  		break;
    > -	default: {
    > -		unsigned tag = (*x & 0xe0) >> 5;
    > -		unsigned length = *x & 0x1f;
    > -		printf("  Unknown CTA tag 0x%02x, length %u\n", tag, length);
    > -		hex_block("    ", x + 1, length);
    > -		data_block.clear();
    > -		warn("Unknown CTA Data Block %u\n", tag);
    > +	case 0x779:
    > +		if (previous_cta_tag != (0x003|kOUI_HDMI))
    > +			fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
    > +		if (have_hf_scdb || have_hf_vsdb)
    > +			fail("Duplicate HDMI Forum VSDB/SCDB\n");
    > +		if (x[2] || x[3])
    > +			printf("    Non-zero SCDB reserved fields!\n");
    > +		cta_hf_scdb(x + 4, length - 3);
    > +		have_hf_scdb = true;
    >  		break;
    > +	default:
    > +		warn("Unknown %s\n", data_block.c_str());
    > +		hex_block("    ", x + 1 + extended + (ouinum ? 3 : 0), length - (extended + (ouinum ? 3 : 0)));
    >  	}
    > -	}
    > -	first_block = 0;
    > -	last_block_was_hdmi_vsdb = 0;
    > +	cta_block_number++;
    > +	previous_cta_tag = tag;
    >  }
    >  
    >  void edid_state::preparse_cta_block(const unsigned char *x)
    > 
    
    



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/5] edid-decode: CTA extension block cleanup
  2019-12-05 13:52     ` Joe van Tunen
@ 2020-01-15 11:52       ` Hans Verkuil
  0 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2020-01-15 11:52 UTC (permalink / raw)
  To: Joe van Tunen; +Cc: linux-media

Hi Joe,

I marked this patch as 'Obsoleted' since I applied a new patch that is
inspired by this one.

I never liked that this patch combined tag/extended tag/oui in one switch,
but I did like that there was only one default case for the hex dumps and
that it removed some duplicate code.

In any case, thank you for this patch, even though I didn't merge it.

Regards,

	Hans

On 12/5/19 2:52 PM, Joe van Tunen wrote:
> I did some evil things with the switch statement but I think it at least looks nice (for some definitions of nice).
> 
> The main idea is to be more like DisplayID. First, there's a switch to get the data_block name. The data_block name is required for error handling so it is obtained first. Instead of a nested switch, it combines the tag and extended tag bytes. The switch obtains an oui for vendor specific data blocks. There's three tags that parse oui (I added oui parsing for audio but none of those are decoded) and now they all use the same oui function.
> 
> Second, there's a separate switch for the decode part. This switch combines tag, extended tag, and oui. The duplicate code was unduplicated. There's only one default for outputting hex of unparsed data blocks (instead of 12) - whether they have a normal tag, extended tag, a tag with an oui, or an extended tag with an oui.
> 
> 
> On 2019-12-05, 1:38 AM, "Hans Verkuil" <hverkuil@xs4all.nl> wrote:
> 
>     Hi Joe,
>     
>     I've committed the other patches, but I'm not sure about this one. I'll look at
>     it again when I have more time. I might make my own implementation based on
>     ideas of this patch.
>     
>     BTW, I made a real mess with the back/front porch timings. Thanks for fixing that!
>     
>     Regards,
>     
>     	Hans
>     
>     On 12/5/19 8:34 AM, joevt wrote:
>     > Reduced tag parsing code.
>     > Added OUI min block length checks.
>     > 
>     > Signed-off-by: Joe van Tunen <joevt@shaw.ca>
>     > ---
>     >  edid-decode.h       |   1 +
>     >  parse-cta-block.cpp | 336 ++++++++++++++++++--------------------------
>     >  2 files changed, 138 insertions(+), 199 deletions(-)
>     > 
>     > diff --git a/edid-decode.h b/edid-decode.h
>     > index 83ded83..758bdcf 100644
>     > --- a/edid-decode.h
>     > +++ b/edid-decode.h
>     > @@ -111,6 +111,7 @@ struct edid_state {
>     >  	void cta_y420cmdb(const unsigned char *x, unsigned length);
>     >  	void cta_vfpdb(const unsigned char *x, unsigned length);
>     >  	void cta_hdmi_block(const unsigned char *x, unsigned length);
>     > +	void cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum);
>     >  	void cta_block(const unsigned char *x);
>     >  	void preparse_cta_block(const unsigned char *x);
>     >  	void parse_cta_block(const unsigned char *x);
>     > diff --git a/parse-cta-block.cpp b/parse-cta-block.cpp
>     > index dea87c1..e52822a 100644
>     > --- a/parse-cta-block.cpp
>     > +++ b/parse-cta-block.cpp
>     > @@ -375,6 +375,11 @@ void edid_state::cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420)
>     >  
>     >  		if (vic == 1 && !for_ycbcr420)
>     >  			has_cta861_vic_1 = 1;
>     > +
>     > +		// vics and has_vic are basically the same (if has_vic was not bool), except vics
>     > +		// is built after preparse (during parse) which allows errors for duplicates to be
>     > +		// output in parse order. has_vic is built during preparse and is used when vics
>     > +		// from other blocks need to be checked.
>     >  		if (++vics[vic][for_ycbcr420] == 2)
>     >  			fail("Duplicate %sVIC %u\n", for_ycbcr420 ? "YCbCr 4:2:0 " : "", vic);
>     >  		if (for_ycbcr420 && has_vic[0][vic])
>     > @@ -485,7 +490,6 @@ void edid_state::cta_hdmi_block(const unsigned char *x, unsigned length)
>     >  {
>     >  	unsigned len_vic, len_3d;
>     >  
>     > -	printf(" (HDMI)\n");
>     >  	printf("    Source physical address %u.%u.%u.%u\n", x[3] >> 4, x[3] & 0x0f,
>     >  	       x[4] >> 4, x[4] & 0x0f);
>     >  
>     > @@ -1228,10 +1232,20 @@ static void cta_hdmi_audio_block(const unsigned char *x, unsigned length)
>     >  		x += 4;
>     >  	}
>     >  }
>     > +const unsigned kOUI_Unknown   = 1<<12;
>     > +const unsigned kOUI_HDMI      = 2<<12;
>     > +const unsigned kOUI_HDMIForum = 3<<12;
>     > +const unsigned kOUI_HDR10     = 4<<12;
>     >  
>     > -static const char *oui_name(unsigned oui)
>     > +static const char *oui_name(unsigned oui, unsigned *ouinum = NULL)
>     >  {
>     > +	unsigned ouinumscratch;
>     > +	if (!ouinum) ouinum = &ouinumscratch;
>     > +	*ouinum = kOUI_Unknown;
>     >  	switch (oui) {
>     > +	case 0x000c03: *ouinum = kOUI_HDMI      ; return "HDMI";
>     > +	case 0xc45dd8: *ouinum = kOUI_HDMIForum ; return "HDMI Forum";
>     > +	case 0x90848b: *ouinum = kOUI_HDR10     ; return "HDR10+";
>     >  	case 0x00001a: return "AMD";
>     >  	case 0x00044b: return "NVIDIA";
>     >  	case 0x000c6e: return "ASUS";
>     > @@ -1244,212 +1258,136 @@ static const char *oui_name(unsigned oui)
>     >  	}
>     >  }
>     >  
>     > +void edid_state::cta_oui(const char *block_name, const unsigned char *x, unsigned length, unsigned *ouinum)
>     > +{
>     > +	char buf[10];
>     > +	unsigned oui;
>     > +
>     > +	if (length < 3) {
>     > +		oui = 0xffffffff;
>     > +		sprintf(buf, "?");
>     > +	} else {
>     > +		oui = (x[2] << 16) + (x[1] << 8) + x[0];
>     > +		sprintf(buf, "0x%06x", oui);
>     > +	}
>     > +	
>     > +	const char *ouiname = oui_name(oui, ouinum);
>     > +	std::string name = std::string(block_name) + ", OUI " + buf;
>     > +	if (ouiname) name += std::string(" (") + ouiname + ")";
>     > +	data_block = name;
>     > +	
>     > +	if (oui == 0xffffffff)
>     > +		fail("CTA data block is not long enough to contain an OUI\n");
>     > +}
>     > +
>     > +#define data_block_o(n) cta_oui(n, x + 1 + extended, length - extended, &ouinum)
>     > +
>     >  void edid_state::cta_block(const unsigned char *x)
>     >  {
>     > -	static int last_block_was_hdmi_vsdb;
>     > -	static int have_hf_vsdb, have_hf_scdb;
>     > -	static int first_block = 1;
>     > +	static unsigned previous_cta_tag = 0;
>     > +	static bool have_hf_vsdb = false;
>     > +	static bool have_hf_scdb = false;
>     > +	static unsigned cta_block_number = 0;
>     > +
>     >  	unsigned length = x[0] & 0x1f;
>     > -	const char *name;
>     > -	unsigned oui;
>     > +	unsigned ouinum = 0;
>     > +	unsigned tag=(x[0] & 0xe0) >> 5;
>     > +	unsigned extended = tag == 0x07 ? 1 : 0;
>     > +	if (extended) tag = 0x700 + x[1];
>     > +
>     > +	switch (tag) {
>     > +	case 0x001: data_block = "Audio Data Block"; break;
>     > +	case 0x002: data_block = "Video Data Block"; break;
>     > +	case 0x003: data_block_o("Vendor-Specific Data Block"); break;
>     > +	case 0x004: data_block = "Speaker Allocation Data Block"; break;
>     > +	case 0x005: data_block = "VESA DTC Data Block"; break; // not implemented
>     > +
>     > +	case 0x700: data_block = "Video Capability Data Block"; break;
>     > +	case 0x701: data_block_o("Vendor-Specific Video Data Block"); break;
>     > +	case 0x702: data_block = "VESA Video Display Device Data Block"; break; // not implemented
>     > +	case 0x703: data_block = "VESA Video Timing Block Extension"; break; // not implemented
>     > +	case 0x704: data_block = "Reserved for HDMI Video Data Block"; break; // reserved
>     > +	case 0x705: data_block = "Colorimetry Data Block"; break;
>     > +	case 0x706: data_block = "HDR Static Metadata Data Block"; break;
>     > +	case 0x707: data_block = "HDR Dynamic Metadata Data Block"; break;
>     > +
>     > +	case 0x70d: data_block = "Video Format Preference Data Block"; break;
>     > +	case 0x70e: data_block = "YCbCr 4:2:0 Video Data Block"; break;
>     > +	case 0x70f: data_block = "YCbCr 4:2:0 Capability Map Data Block"; break;
>     > +	case 0x710: data_block = "Reserved for CTA Miscellaneous Audio Fields"; break; // reserved
>     > +	case 0x711: data_block_o("Vendor-Specific Audio Data Block"); break; // no vendors implemented
>     > +	case 0x712: data_block = "HDMI Audio Data Block"; break;
>     > +	case 0x713: data_block = "Room Configuration Data Block"; break;
>     > +	case 0x714: data_block = "Speaker Location Data Block"; break;
>     > +
>     > +	case 0x720: data_block = "InfoFrame Data Block"; break;
>     > +
>     > +	case 0x778: data_block = "HDMI Forum EDID Extension Override Data Block"; break;
>     > +	case 0x779: data_block = "HDMI Forum Sink Capability Data Block"; break;
>     > +	default:
>     > +		     if (tag < 0x700) data_block = "Unknown CTA Data Block";
>     > +		else if (tag < 0x70d) data_block = "Unknown CTA Video-Related Data Block";
>     > +		else if (tag < 0x720) data_block = "Unknown CTA Audio-Related Data Block";
>     > +		else if (tag < 0x778) data_block = "Unknown CTA Data Block";
>     > +		else if (tag < 0x780) data_block = "Unknown CTA HDMI-Related Data Block";
>     > +		else                  data_block = "Unknown CTA Data Block";
>     > +		data_block += std::string(" (") + (extended ? "extended " : "") + "tag " + utohex(tag & 0xff) + ")";
>     > +	}
>     >  
>     > -	switch ((x[0] & 0xe0) >> 5) {
>     > -	case 0x01:
>     > -		data_block = "Audio Data Block";
>     > -		printf("  %s\n", data_block.c_str());
>     > -		cta_audio_block(x + 1, length);
>     > +	printf("  %s\n", data_block.c_str());
>     > +	
>     > +	tag |= ouinum;
>     > +	switch (tag) {
>     > +	case 0x001: cta_audio_block(x + 1, length); break;
>     > +	case 0x002: cta_svd(x + 1, length, 0); break;
>     > +	case 0x003|kOUI_HDMI:
>     > +		cta_hdmi_block(x + 1, length);
>     > +		if (edid_minor != 3)
>     > +			fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
>     >  		break;
>     > -	case 0x02:
>     > -		data_block = "Video Data Block";
>     > -		printf("  %s\n", data_block.c_str());
>     > -		cta_svd(x + 1, length, 0);
>     > +	case 0x003|kOUI_HDMIForum:
>     > +		if (previous_cta_tag != (0x003|kOUI_HDMI))
>     > +			fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
>     > +		if (have_hf_scdb || have_hf_vsdb)
>     > +			fail("Duplicate HDMI Forum VSDB/SCDB\n");
>     > +		cta_hf_scdb(x + 4, length - 3);
>     > +		have_hf_vsdb = true;
>     >  		break;
>     > -	case 0x03:
>     > -		oui = (x[3] << 16) + (x[2] << 8) + x[1];
>     > -		printf("  Vendor-Specific Data Block, OUI 0x%06x", oui);
>     > -		name = oui_name(oui);
>     > -		if (oui == 0x000c03) {
>     > -			data_block = "Vendor-Specific Data Block (HDMI)";
>     > -			cta_hdmi_block(x + 1, length);
>     > -			last_block_was_hdmi_vsdb = 1;
>     > -			first_block = 0;
>     > -			if (edid_minor != 3)
>     > -				fail("The HDMI Specification uses EDID 1.3, not 1.%u\n", edid_minor);
>     > -			return;
>     > -		}
>     > -		if (oui == 0xc45dd8) {
>     > -			data_block = "Vendor-Specific Data Block (HDMI Forum)";
>     > -			if (!last_block_was_hdmi_vsdb)
>     > -				fail("HDMI Forum VSDB did not immediately follow the HDMI VSDB\n");
>     > -			if (have_hf_scdb || have_hf_vsdb)
>     > -				fail("Duplicate HDMI Forum VSDB/SCDB\n");
>     > -			printf(" (HDMI Forum)\n");
>     > -			cta_hf_scdb(x + 4, length - 3);
>     > -			have_hf_vsdb = 1;
>     > -		} else if (name) {
>     > -			data_block = std::string("Vendor-Specific Data Block (") + name + ")";
>     > -			printf(" (%s)\n", name);
>     > -			hex_block("    ", x + 4, length - 3);
>     > -		} else {
>     > -			printf("\n");
>     > -			hex_block("    ", x + 4, length - 3);
>     > -			data_block.clear();
>     > -			warn("Unknown Vendor-Specific Data Block, OUI 0x%06x\n", oui);
>     > -		}
>     > -		break;
>     > -	case 0x04:
>     > -		data_block = "Speaker Allocation Data Block";
>     > -		printf("  %s\n", data_block.c_str());
>     > -		cta_sadb(x + 1, length);
>     > -		break;
>     > -	case 0x05:
>     > -		printf("  VESA DTC Data Block\n");
>     > -		hex_block("  ", x + 1, length);
>     > -		break;
>     > -	case 0x07:
>     > -		printf("  Extended tag: ");
>     > -		switch (x[1]) {
>     > -		case 0x00:
>     > -			data_block = "Video Capability Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_vcdb(x + 2, length - 1);
>     > -			break;
>     > -		case 0x01:
>     > -			oui = (x[4] << 16) + (x[3] << 8) + x[2];
>     > -			printf("Vendor-Specific Video Data Block, OUI 0x%06x", oui);
>     > -			name = oui_name(oui);
>     > -			if (oui == 0x90848b) {
>     > -				data_block = "Vendor-Specific Video Data Block (HDR10+)";
>     > -				printf(" (HDR10+)\n");
>     > -				cta_hdr10plus(x + 5, length - 4);
>     > -			} else if (name) {
>     > -				data_block = std::string("Vendor-Specific Data Block (") + name + ")";
>     > -				printf(" (%s)\n", name);
>     > -				hex_block("    ", x + 5, length - 4);
>     > -			} else {
>     > -				printf("\n");
>     > -				hex_block("    ", x + 5, length - 4);
>     > -				data_block.clear();
>     > -				warn("Unknown Extended Vendor-Specific Data Block, OUI 0x%06x\n", oui);
>     > -			}
>     > -			break;
>     > -		case 0x02:
>     > -			printf("VESA Video Display Device Data Block\n");
>     > -			hex_block("  ", x + 2, length - 1);
>     > -			break;
>     > -		case 0x03:
>     > -			printf("VESA Video Timing Block Extension\n");
>     > -			hex_block("  ", x + 2, length - 1);
>     > -			break;
>     > -		case 0x04:
>     > -			printf("Reserved for HDMI Video Data Block\n");
>     > -			hex_block("  ", x + 2, length - 1);
>     > -			break;
>     > -		case 0x05:
>     > -			data_block = "Colorimetry Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_colorimetry_block(x + 2, length - 1);
>     > -			break;
>     > -		case 0x06:
>     > -			data_block = "HDR Static Metadata Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_hdr_static_metadata_block(x + 2, length - 1);
>     > -			break;
>     > -		case 0x07:
>     > -			data_block = "HDR Dynamic Metadata Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_hdr_dyn_metadata_block(x + 2, length - 1);
>     > -			break;
>     > -		case 0x0d:
>     > -			data_block = "Video Format Preference Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_vfpdb(x + 2, length - 1);
>     > -			break;
>     > -		case 0x0e:
>     > -			data_block = "YCbCr 4:2:0 Video Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_svd(x + 2, length - 1, 1);
>     > -			break;
>     > -		case 0x0f:
>     > -			data_block = "YCbCr 4:2:0 Capability Map Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_y420cmdb(x + 2, length - 1);
>     > -			break;
>     > -		case 0x10:
>     > -			printf("Reserved for CTA Miscellaneous Audio Fields\n");
>     > -			hex_block("  ", x + 2, length - 1);
>     > -			break;
>     > -		case 0x11:
>     > -			printf("Vendor-Specific Audio Data Block\n");
>     > -			hex_block("  ", x + 2, length - 1);
>     > -			break;
>     > -		case 0x12:
>     > -			data_block = "HDMI Audio Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_hdmi_audio_block(x + 2, length - 1);
>     > -			break;
>     > -		case 0x13:
>     > -			data_block = "Room Configuration Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_rcdb(x + 2, length - 1);
>     > -			break;
>     > -		case 0x14:
>     > -			data_block = "Speaker Location Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_sldb(x + 2, length - 1);
>     > -			break;
>     > -		case 0x20:
>     > -			printf("InfoFrame Data Block\n");
>     > -			cta_ifdb(x + 2, length - 1);
>     > -			break;
>     > -		case 0x78:
>     > -			data_block = "HDMI Forum EDID Extension Override Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			cta_hf_eeodb(x + 2, length - 1);
>     > -			// This must be the first CTA block
>     > -			if (!first_block)
>     > -				fail("Block starts at a wrong offset\n");
>     > -			break;
>     > -		case 0x79:
>     > -			data_block = "HDMI Forum Sink Capability Data Block";
>     > -			printf("%s\n", data_block.c_str());
>     > -			if (!last_block_was_hdmi_vsdb)
>     > -				fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
>     > -			if (have_hf_scdb || have_hf_vsdb)
>     > -				fail("Duplicate HDMI Forum VSDB/SCDB\n");
>     > -			if (x[2] || x[3])
>     > -				printf("  Non-zero SCDB reserved fields!\n");
>     > -			cta_hf_scdb(x + 4, length - 3);
>     > -			have_hf_scdb = 1;
>     > -			break;
>     > -		default:
>     > -			if (x[1] <= 12)
>     > -				printf("Unknown CTA Video-Related");
>     > -			else if (x[1] <= 31)
>     > -				printf("Unknown CTA Audio-Related");
>     > -			else if (x[1] >= 120 && x[1] <= 127)
>     > -				printf("Unknown CTA HDMI-Related");
>     > -			else
>     > -				printf("Unknown CTA");
>     > -			printf(" Data Block (extended tag 0x%02x, length %u)\n", x[1], length - 1);
>     > -			hex_block("    ", x + 2, length - 1);
>     > -			data_block.clear();
>     > -			warn("Unknown Extended CTA Data Block 0x%02x\n", x[1]);
>     > -			break;
>     > -		}
>     > +	case 0x004: cta_sadb(x + 1, length); break;
>     > +	case 0x700: cta_vcdb(x + 2, length - 1); break;
>     > +	case 0x701|kOUI_HDR10: cta_hdr10plus(x + 5, length - 4); break;
>     > +	case 0x705: cta_colorimetry_block(x + 2, length - 1); break;
>     > +	case 0x706: cta_hdr_static_metadata_block(x + 2, length - 1); break;
>     > +	case 0x707: cta_hdr_dyn_metadata_block(x + 2, length - 1); break;
>     > +	case 0x70d: cta_vfpdb(x + 2, length - 1); break;
>     > +	case 0x70e: cta_svd(x + 2, length - 1, 1); break;
>     > +	case 0x70f: cta_y420cmdb(x + 2, length - 1); break;
>     > +	case 0x712: cta_hdmi_audio_block(x + 2, length - 1); break;
>     > +	case 0x713: cta_rcdb(x + 2, length - 1); break;
>     > +	case 0x714: cta_sldb(x + 2, length - 1); break;
>     > +	case 0x720: cta_ifdb(x + 2, length - 1); break;
>     > +	case 0x778: 
>     > +		cta_hf_eeodb(x + 2, length - 1);
>     > +		// This must be the first CTA block
>     > +		if (cta_block_number != 0)
>     > +			fail("Block starts at a wrong offset\n");
>     >  		break;
>     > -	default: {
>     > -		unsigned tag = (*x & 0xe0) >> 5;
>     > -		unsigned length = *x & 0x1f;
>     > -		printf("  Unknown CTA tag 0x%02x, length %u\n", tag, length);
>     > -		hex_block("    ", x + 1, length);
>     > -		data_block.clear();
>     > -		warn("Unknown CTA Data Block %u\n", tag);
>     > +	case 0x779:
>     > +		if (previous_cta_tag != (0x003|kOUI_HDMI))
>     > +			fail("HDMI Forum SCDB did not immediately follow the HDMI VSDB\n");
>     > +		if (have_hf_scdb || have_hf_vsdb)
>     > +			fail("Duplicate HDMI Forum VSDB/SCDB\n");
>     > +		if (x[2] || x[3])
>     > +			printf("    Non-zero SCDB reserved fields!\n");
>     > +		cta_hf_scdb(x + 4, length - 3);
>     > +		have_hf_scdb = true;
>     >  		break;
>     > +	default:
>     > +		warn("Unknown %s\n", data_block.c_str());
>     > +		hex_block("    ", x + 1 + extended + (ouinum ? 3 : 0), length - (extended + (ouinum ? 3 : 0)));
>     >  	}
>     > -	}
>     > -	first_block = 0;
>     > -	last_block_was_hdmi_vsdb = 0;
>     > +	cta_block_number++;
>     > +	previous_cta_tag = tag;
>     >  }
>     >  
>     >  void edid_state::preparse_cta_block(const unsigned char *x)
>     > 
>     
>     
> 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-01-15 11:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05  7:34 [PATCH 1/5] edid-decode: DisplayID additions joevt
2019-12-05  7:34 ` [PATCH 2/5] edid-decode: Change horizontal frequency to kHz joevt
2019-12-05  7:34 ` [PATCH 3/5] edid-decode: more back/front porch switching joevt
2019-12-05  7:34 ` [PATCH 4/5] edid-decode: CTA extension block cleanup joevt
2019-12-05  9:38   ` Hans Verkuil
2019-12-05 13:52     ` Joe van Tunen
2020-01-15 11:52       ` Hans Verkuil
2019-12-05  7:34 ` [PATCH 5/5] edid-decode: add missing space joevt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).