[igt-dev] [PATCH i-g-t 1/5] lib/igt_kms: remove length parameter from kmstest_force_edid

[igt-dev] [PATCH i-g-t 1/5] lib/igt_kms: remove length parameter from kmstest_force_edid

[Simon Ser]

Given an EDID, computing the size is trivial. Instead of having one size
constant per EDID and hope the callers use the right one (ie. *not* EDID_LENGTH
when there's an extension), we can make functions that take EDIDs compute the
size if they need it.

We have tests in lib/tests/igt_edid.c which assert the number of extensions
present in the EDID anyway.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_kms.c                     |  9 ++++++---
 lib/igt_kms.h                     |  3 +--
 tests/kms_3d.c                    |  4 ++--
 tests/kms_force_connector_basic.c | 12 ++++++------
 tests/kms_hdmi_inject.c           | 11 ++++-------
 5 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index dc8992cb043b..f8185edf6e8b 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -182,6 +182,8 @@ const unsigned char *igt_kms_get_alt_edid(void)
 	return (unsigned char *) &edid;
 }

+#define AUDIO_EDID_LENGTH (2 * EDID_LENGTH)
+
 static void
 generate_audio_edid(unsigned char raw_edid[static AUDIO_EDID_LENGTH],
 		    bool with_vsd, struct cea_sad *sad,
@@ -998,7 +1000,7 @@ bool kmstest_force_connector(int drm_fd, drmModeConnector *connector,
  * If @length is zero, the forced EDID will be removed.
  */
 void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
-			const unsigned char *edid, size_t length)
+			const unsigned char *edid)
 {
 	char *path;
 	int debugfs_fd, ret;
@@ -1011,10 +1013,11 @@ void kmstest_force_edid(int drm_fd, drmModeConnector *connector,

 	igt_require(debugfs_fd != -1);

-	if (length == 0)
+	if (edid == NULL)
 		ret = write(debugfs_fd, "reset", 5);
 	else
-		ret = write(debugfs_fd, edid, length);
+		ret = write(debugfs_fd, edid,
+			    edid_get_size((struct edid *) edid));
 	close(debugfs_fd);

 	/* To allow callers to always use GetConnectorCurrent we need to force a
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index a448a003ae56..f72508640712 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -197,7 +197,7 @@ bool kmstest_force_connector(int fd, drmModeConnector *connector,
 void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
 void kmstest_edid_add_4k(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
 void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
-			const unsigned char *edid, size_t length);
+			const unsigned char *edid);

 bool kmstest_get_connector_default_mode(int drm_fd, drmModeConnector *connector,
 					drmModeModeInfo *mode);
@@ -759,7 +759,6 @@ struct cea_sad;
 struct cea_speaker_alloc;

 #define EDID_LENGTH 128
-#define AUDIO_EDID_LENGTH (2 * EDID_LENGTH)
 const unsigned char *igt_kms_get_base_edid(void);
 const unsigned char *igt_kms_get_alt_edid(void);
 const unsigned char *igt_kms_get_hdmi_audio_edid(void);
diff --git a/tests/kms_3d.c b/tests/kms_3d.c
index df8185abebc4..a170814f6356 100644
--- a/tests/kms_3d.c
+++ b/tests/kms_3d.c
@@ -60,7 +60,7 @@ igt_simple_main
 	kmstest_edid_add_3d(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
 			    &length);

-	kmstest_force_edid(drm_fd, connector, edid, length);
+	kmstest_force_edid(drm_fd, connector, edid);
 	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
 		igt_skip("Could not force connector on\n");

@@ -113,7 +113,7 @@ igt_simple_main
 	}

 	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
-	kmstest_force_edid(drm_fd, connector, NULL, 0);
+	kmstest_force_edid(drm_fd, connector, NULL);

 	drmModeFreeConnector(connector);
 	free(edid);
diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
index 20812d5e3189..f1533e5415c0 100644
--- a/tests/kms_force_connector_basic.c
+++ b/tests/kms_force_connector_basic.c
@@ -48,7 +48,7 @@ static void reset_connectors(void)
 		kmstest_force_connector(drm_fd, connector,
 					FORCE_CONNECTOR_UNSPECIFIED);

-		kmstest_force_edid(drm_fd, connector, NULL, 0);
+		kmstest_force_edid(drm_fd, connector, NULL);

 		drmModeFreeConnector(connector);
 	}
@@ -247,7 +247,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)

 		/* test edid forcing */
 		kmstest_force_edid(drm_fd, vga_connector,
-				   igt_kms_get_base_edid(), EDID_LENGTH);
+				   igt_kms_get_base_edid());
 		temp = drmModeGetConnectorCurrent(drm_fd,
 						  vga_connector->connector_id);

@@ -260,7 +260,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)
 		drmModeFreeConnector(temp);

 		/* remove edid */
-		kmstest_force_edid(drm_fd, vga_connector, NULL, 0);
+		kmstest_force_edid(drm_fd, vga_connector, NULL);
 		kmstest_force_connector(drm_fd, vga_connector,
 					FORCE_CONNECTOR_UNSPECIFIED);
 		temp = drmModeGetConnectorCurrent(drm_fd,
@@ -280,7 +280,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)

 		/* test pruning of stale modes */
 		kmstest_force_edid(drm_fd, vga_connector,
-				   igt_kms_get_alt_edid(), EDID_LENGTH);
+				   igt_kms_get_alt_edid());
 		temp = drmModeGetConnectorCurrent(drm_fd,
 						  vga_connector->connector_id);

@@ -294,7 +294,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)
 		drmModeFreeConnector(temp);

 		kmstest_force_edid(drm_fd, vga_connector,
-				   igt_kms_get_base_edid(), EDID_LENGTH);
+				   igt_kms_get_base_edid());
 		temp = drmModeGetConnectorCurrent(drm_fd,
 						  vga_connector->connector_id);

@@ -307,7 +307,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)

 		drmModeFreeConnector(temp);

-		kmstest_force_edid(drm_fd, vga_connector, NULL, 0);
+		kmstest_force_edid(drm_fd, vga_connector, NULL);
 		kmstest_force_connector(drm_fd, vga_connector,
 					FORCE_CONNECTOR_UNSPECIFIED);
 	}
diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
index 8c0d1333db19..9a968fa9e50e 100644
--- a/tests/kms_hdmi_inject.c
+++ b/tests/kms_hdmi_inject.c
@@ -93,7 +93,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
 	kmstest_edid_add_4k(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
 			    &length);

-	kmstest_force_edid(drm_fd, connector, edid, length);
+	kmstest_force_edid(drm_fd, connector, edid);

 	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
 		igt_skip("Could not force connector on\n");
@@ -134,7 +134,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
 	igt_remove_fb(drm_fd, &fb);

 	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
-	kmstest_force_edid(drm_fd, connector, NULL, 0);
+	kmstest_force_edid(drm_fd, connector, NULL);

 	free(edid);
 }
@@ -143,15 +143,12 @@ static void
 hdmi_inject_audio(int drm_fd, drmModeConnector *connector)
 {
 	const unsigned char *edid;
-	size_t length;
 	int fb_id, cid, ret, crtc_mask = -1;
 	struct igt_fb fb;
 	struct kmstest_connector_config config;

 	edid = igt_kms_get_hdmi_audio_edid();
-	length = AUDIO_EDID_LENGTH;
-
-	kmstest_force_edid(drm_fd, connector, edid, length);
+	kmstest_force_edid(drm_fd, connector, edid);

 	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
 		igt_skip("Could not force connector on\n");
@@ -191,7 +188,7 @@ hdmi_inject_audio(int drm_fd, drmModeConnector *connector)
 	kmstest_dump_mode(&connector->modes[0]);

 	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
-	kmstest_force_edid(drm_fd, connector, NULL, 0);
+	kmstest_force_edid(drm_fd, connector, NULL);
 }

 igt_main
--
2.22.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 2/5] lib/igt_edid: add hdmi_vsd

[Simon Ser]

The HDMI Vendor-Specific Data block, defined as an opaque blob in the EDID
spec, is described in the HDMI spec.

Most of the extension fields are optional, so it doesn't translate well to a C
struct. For now callers need to manually fill the hdmi_vsd.data field.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_edid.c | 40 ++++++++++++++++++++++++++++++++--------
 lib/igt_edid.h | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/lib/igt_edid.c b/lib/igt_edid.c
index cbb7eefff70d..c93d8fa8bca0 100644
--- a/lib/igt_edid.c
+++ b/lib/igt_edid.c
@@ -314,6 +314,14 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
 	sad->bitrate = sample_sizes;
 }
 
+static void cea_vsd_set_hdmi(struct cea_vsd *vsd)
+{
+	/* HDMI's IEEE Registration Identifier */
+	vsd->ieee_oui[0] = 0x03;
+	vsd->ieee_oui[1] = 0x0C;
+	vsd->ieee_oui[2] = 0x00;
+}
+
 /**
  * cea_vsd_get_hdmi_default:
  *
@@ -321,21 +329,22 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
  */
 const struct cea_vsd *cea_vsd_get_hdmi_default(size_t *size)
 {
-	static char raw[sizeof(struct cea_vsd) + 4] = {0};
+	static char raw[CEA_VSD_HDMI_MIN_SIZE + 2] = {0};
 	struct cea_vsd *vsd;
+	struct hdmi_vsd *hdmi;
 
 	*size = sizeof(raw);
 
 	/* Magic incantation. Works better if you orient your screen in the
 	 * direction of the VESA headquarters. */
 	vsd = (struct cea_vsd *) raw;
-	vsd->ieee_oui[0] = 0x03;
-	vsd->ieee_oui[1] = 0x0C;
-	vsd->ieee_oui[2] = 0x00;
-	vsd->data[0] = 0x10;
-	vsd->data[1] = 0x00;
-	vsd->data[2] = 0x38;
-	vsd->data[3] = 0x2D;
+	cea_vsd_set_hdmi(vsd);
+	hdmi = &vsd->data.hdmi;
+	hdmi->src_phy_addr[0] = 0x10;
+	hdmi->src_phy_addr[1] = 0x00;
+	/* 2 VSD extension fields */
+	hdmi->flags1 = 0x38;
+	hdmi->max_tdms_clock = 0x2D;
 
 	return vsd;
 }
@@ -371,6 +380,21 @@ size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
 	return sizeof(struct edid_cea_data_block) + vsd_size;
 }
 
+size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
+					const struct hdmi_vsd *hdmi,
+					size_t hdmi_size)
+{
+	char raw_vsd[CEA_VSD_HDMI_MAX_SIZE] = {0};
+	struct cea_vsd *vsd = (struct cea_vsd *) raw_vsd;
+
+	assert(hdmi_size >= HDMI_VSD_MIN_SIZE && hdmi_size <= HDMI_VSD_MAX_SIZE);
+	cea_vsd_set_hdmi(vsd);
+	memcpy(&vsd->data.hdmi, hdmi, hdmi_size);
+
+	return edid_cea_data_block_set_vsd(block, vsd,
+					   CEA_VSD_HEADER_SIZE + hdmi_size);
+}
+
 size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
 					     const struct cea_speaker_alloc *speakers)
 {
diff --git a/lib/igt_edid.h b/lib/igt_edid.h
index 47581bb778b0..0ac851c0d74a 100644
--- a/lib/igt_edid.h
+++ b/lib/igt_edid.h
@@ -191,12 +191,50 @@ struct cea_sad {
 	uint8_t bitrate;
 } __attribute__((packed));
 
-/* Vendor Specific Data */
-struct cea_vsd {
-	uint8_t ieee_oui[3];
-	char data[];
+enum hdmi_vsd_flags1 {
+	HDMI_VSD_DVI_DUAL = 1 << 0,
+	HDMI_VSD_DC_Y444 = 1 << 3, /* supports YCbCr 4:4:4 */
+	HDMI_VSD_DC_30BIT = 1 << 4, /* 30 bits per pixel */
+	HDMI_VSD_DC_36BIT = 1 << 5, /* 36 bits per pixel */
+	HDMI_VSD_DC_48BIT = 1 << 6, /* 48 bits per pixel */
+	HDMI_VSD_SUPPORTS_AI = 1 << 7, /* supports ACP, ISRC1 or ISRC2 packets */
+};
+
+enum hdmi_vsd_flags2 {
+	HDMI_VSD_CNC_GRAPHICS = 1 << 0,
+	HDMI_VSD_CNC_PHOTO = 1 << 1,
+	HDMI_VSD_CNC_CINEMA = 1 << 2,
+	HDMI_VSD_CNC_GAME = 1 << 3,
+	HDMI_VSD_VIDEO_PRESENT = 1 << 5,
+	HDMI_VSD_INTERLACED_LATENCY_PRESENT = 1 << 6,
+	HDMI_VSD_LATENCY_PRESENT = 1 << 7,
 };
 
+/* HDMI Vendor-Specific Data Block (defined in the HDMI spec) */
+struct hdmi_vsd {
+	uint8_t src_phy_addr[2]; /* source physical address */
+
+	/* Extension fields */
+	uint8_t flags1; /* enum hdmi_vsd_flags1 */
+	uint8_t max_tdms_clock; /* multiply by 5MHz */
+	uint8_t flags2; /* enum hdmi_vsd_flags2 */
+	char data[]; /* latency, misc, VIC, 3D */
+} __attribute__((packed));
+
+#define HDMI_VSD_MIN_SIZE 2 /* just the source physical address */
+#define HDMI_VSD_MAX_SIZE 28
+#define CEA_VSD_HEADER_SIZE 3 /* IEEE OUI */
+#define CEA_VSD_HDMI_MIN_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MIN_SIZE)
+#define CEA_VSD_HDMI_MAX_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MAX_SIZE)
+
+/* Vendor-Specific Data */
+struct cea_vsd {
+	uint8_t ieee_oui[3]; /* 24-bit IEEE Registration Identifier, LSB */
+	union {
+		struct hdmi_vsd hdmi;
+	} data;
+} __attribute__((packed));
+
 enum cea_speaker_alloc_item {
 	CEA_SPEAKER_FRONT_LEFT_RIGHT = 1 << 0,
 	CEA_SPEAKER_LFE = 1 << 1,
@@ -315,6 +353,9 @@ size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
 				   const struct cea_sad *sads, size_t sads_len);
 size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
 				   const struct cea_vsd *vsd, size_t vsd_size);
+size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
+					const struct hdmi_vsd *hdmi,
+					size_t hdmi_size);
 size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
 					     const struct cea_speaker_alloc *speakers);
 void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
-- 
2.22.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 3/5] lib/igt_edid: add support for native DTDs in CEA extension blocks

[Simon Ser]

Native Detailed Timing Descriptors follow the Data Block Collection in the CEA
extension.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_edid.c | 6 ++++--
 lib/igt_edid.h | 2 +-
 lib/igt_kms.c  | 3 +--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/igt_edid.c b/lib/igt_edid.c
index c93d8fa8bca0..93202c138161 100644
--- a/lib/igt_edid.c
+++ b/lib/igt_edid.c
@@ -408,15 +408,17 @@ size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
 }
 
 void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
-		      uint8_t flags)
+		      size_t native_dtds_len, uint8_t flags)
 {
 	struct edid_cea *cea = &ext->data.cea;
 
 	ext->tag = EDID_EXT_CEA;
 
+	assert(native_dtds_len <= 0x0F);
+	assert((flags & 0x0F) == 0);
 	cea->revision = 3;
 	cea->dtd_start = 4 + data_blocks_size;
-	cea->misc = flags; /* just flags, no DTD */
+	cea->misc = flags | native_dtds_len;
 }
 
 void edid_ext_update_cea_checksum(struct edid_ext *ext)
diff --git a/lib/igt_edid.h b/lib/igt_edid.h
index 0ac851c0d74a..702c14c54b5e 100644
--- a/lib/igt_edid.h
+++ b/lib/igt_edid.h
@@ -359,6 +359,6 @@ size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
 size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
 					     const struct cea_speaker_alloc *speakers);
 void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
-		      uint8_t flags);
+		      size_t native_dtds_len, uint8_t flags);
 
 #endif
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index f8185edf6e8b..45c79a380daf 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -226,8 +226,7 @@ generate_audio_edid(unsigned char raw_edid[static AUDIO_EDID_LENGTH],
 
 	assert(cea_data_size <= sizeof(edid_cea->data));
 
-	edid_ext_set_cea(edid_ext, cea_data_size,
-			 EDID_CEA_BASIC_AUDIO);
+	edid_ext_set_cea(edid_ext, cea_data_size, 0, EDID_CEA_BASIC_AUDIO);
 
 	edid_update_checksum(edid);
 	edid_ext_update_cea_checksum(edid_ext);
-- 
2.22.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 4/5] lib/igt_edid: add support for Short Video Descriptors

[Simon Ser]

Short Video Descriptors (SVDs, not to be confused with Vendor-Specific Data
blocks) describe CEA video formats supported by the monitor.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_edid.c | 8 ++++++++
 lib/igt_edid.h | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/lib/igt_edid.c b/lib/igt_edid.c
index 93202c138161..9a365dc40352 100644
--- a/lib/igt_edid.c
+++ b/lib/igt_edid.c
@@ -369,6 +369,14 @@ size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
 	return sizeof(struct edid_cea_data_block) + sads_size;
 }
 
+size_t edid_cea_data_block_set_svd(struct edid_cea_data_block *block,
+				   const uint8_t *svds, size_t svds_len)
+{
+	edid_cea_data_block_init(block, EDID_CEA_DATA_VIDEO, svds_len);
+	memcpy(block->data.svds, svds, svds_len);
+	return sizeof(struct edid_cea_data_block) + svds_len;
+}
+
 size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
 				   const struct cea_vsd *vsd, size_t vsd_size)
 {
diff --git a/lib/igt_edid.h b/lib/igt_edid.h
index 702c14c54b5e..55bdf8a1037b 100644
--- a/lib/igt_edid.h
+++ b/lib/igt_edid.h
@@ -191,6 +191,9 @@ struct cea_sad {
 	uint8_t bitrate;
 } __attribute__((packed));
 
+/* Indicates that a Short Video Descriptor is native */
+#define CEA_SVD_NATIVE (1 << 7)
+
 enum hdmi_vsd_flags1 {
 	HDMI_VSD_DVI_DUAL = 1 << 0,
 	HDMI_VSD_DC_Y444 = 1 << 3, /* supports YCbCr 4:4:4 */
@@ -261,6 +264,7 @@ struct edid_cea_data_block {
 	uint8_t type_len; /* type is from enum edid_cea_data_type */
 	union {
 		struct cea_sad sads[0];
+		uint8_t svds[0]; /* Short Video Descriptors */
 		struct cea_vsd vsds[0];
 		struct cea_speaker_alloc speakers[0];
 	} data;
@@ -351,6 +355,8 @@ void edid_ext_update_cea_checksum(struct edid_ext *ext);
 const struct cea_vsd *cea_vsd_get_hdmi_default(size_t *size);
 size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
 				   const struct cea_sad *sads, size_t sads_len);
+size_t edid_cea_data_block_set_svd(struct edid_cea_data_block *block,
+				   const uint8_t *svds, size_t svds_len);
 size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
 				   const struct cea_vsd *vsd, size_t vsd_size);
 size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
-- 
2.22.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 5/5] lib/igt_kms: use igt_edid to generate a 4K EDID

[Simon Ser]

The new EDID has been byte-by-byte checked to be exactly the same as before.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_kms.c               | 107 ++++++++++++++++++++----------------
 lib/igt_kms.h               |   2 +-
 lib/tests/igt_edid.c        |   1 +
 lib/tests/igt_hdmi_inject.c |   1 -
 tests/kms_hdmi_inject.c     |   9 +--
 5 files changed, 64 insertions(+), 56 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 45c79a380daf..444605e4b110 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -284,6 +284,66 @@ const unsigned char *igt_kms_get_dp_audio_edid(void)
 	return raw_edid;
 }
 
+static const uint8_t edid_4k_svds[] = {
+	32 | CEA_SVD_NATIVE, /* 1080p @ 24Hz (native) */
+	5,                   /* 1080i @ 60Hz */
+	20,                  /* 1080i @ 50Hz */
+	4,                   /* 720p @ 60Hz */
+	19,                  /* 720p @ 50Hz */
+};
+
+const unsigned char *igt_kms_get_4k_edid(void)
+{
+	static unsigned char raw_edid[256];
+	struct edid *edid;
+	struct edid_ext *edid_ext;
+	struct edid_cea *edid_cea;
+	char *cea_data;
+	struct edid_cea_data_block *block;
+	char raw_hdmi[HDMI_VSD_MIN_SIZE + 6] = {0};
+	struct hdmi_vsd *hdmi;
+	size_t cea_data_size = 0;
+	size_t svds_len;
+
+	/* Create a new EDID from the base IGT EDID, and add an
+	 * extension that advertises 4K support. */
+	edid = (struct edid *) raw_edid;
+	memcpy(edid, igt_kms_get_base_edid(), sizeof(struct edid));
+	edid->extensions_len = 1;
+	edid_ext = &edid->extensions[0];
+	edid_cea = &edid_ext->data.cea;
+	cea_data = edid_cea->data;
+
+	/* Short Video Descriptor */
+	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
+	svds_len = sizeof(edid_4k_svds) / sizeof(edid_4k_svds[0]);
+	cea_data_size += edid_cea_data_block_set_svd(block, edid_4k_svds,
+						     svds_len);
+
+	/* Vendor Specific Data block */
+	hdmi = (struct hdmi_vsd *) raw_hdmi;
+	hdmi->src_phy_addr[0] = 0x10;
+	hdmi->src_phy_addr[1] = 0x00;
+	hdmi->flags1 = 0;
+	hdmi->max_tdms_clock = 0;
+	hdmi->flags2 = HDMI_VSD_VIDEO_PRESENT;
+	hdmi->data[0] = 0x00; /* HDMI video flags */
+	hdmi->data[1] = 1 << 5; /* 1 VIC entry, 0 3D entries */
+	hdmi->data[2] = 0x01; /* 2160p, specified as short descriptor */
+
+	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
+	cea_data_size += edid_cea_data_block_set_hdmi_vsd(block, hdmi,
+							  sizeof(raw_hdmi));
+
+	assert(cea_data_size <= sizeof(edid_cea->data));
+
+	edid_ext_set_cea(edid_ext, cea_data_size, 0, 0);
+
+	edid_update_checksum(edid);
+	edid_ext_update_cea_checksum(edid_ext);
+	return raw_edid;
+}
+
 const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	[IGT_PLANE_SRC_X] = "SRC_X",
 	[IGT_PLANE_SRC_Y] = "SRC_Y",
@@ -1348,8 +1408,6 @@ struct edid_block {
     unsigned char *data;
 };
 
-#define DTD_SUPPORTS_AUDIO 1<<6
-
 static struct edid_block
 init_cea_block(const unsigned char *edid, size_t length,
 	       unsigned char *new_edid_ptr[], size_t *new_length,
@@ -1437,51 +1495,6 @@ void kmstest_edid_add_3d(const unsigned char *edid, size_t length,
 	update_edid_csum(new_edid.data, length);
 }
 
-/**
- * kmstest_edid_add_4k:
- * @edid: an existing valid edid block
- * @length: length of @edid
- * @new_edid_ptr: pointer to where the new edid will be placed
- * @new_length: pointer to the size of the new edid
- *
- * Makes a copy of an existing edid block and adds an extension indicating
- * a HDMI 4K mode in vsdb.
- */
-void kmstest_edid_add_4k(const unsigned char *edid, size_t length,
-			 unsigned char *new_edid_ptr[], size_t *new_length)
-{
-	char vsdb_block_len = 12;
-	struct edid_block new_edid = init_cea_block(edid, length, new_edid_ptr,
-						    new_length, vsdb_block_len,
-						    0);
-	int pos = new_edid.pos;
-
-	/* vsdb block ( id | length ) */
-	new_edid.data[pos++] = 3 << 5 | (vsdb_block_len - 1);
-	/* registration id */
-	new_edid.data[pos++] = 0x3;
-	new_edid.data[pos++] = 0xc;
-	new_edid.data[pos++] = 0x0;
-	/* source physical address */
-	new_edid.data[pos++] = 0x10;
-	new_edid.data[pos++] = 0x00;
-	/* Supports_AI ... etc */
-	new_edid.data[pos++] = 0x00;
-	/* Max TMDS Clock */
-	new_edid.data[pos++] = 0x00;
-	/* Latency present, HDMI Video Present */
-	new_edid.data[pos++] = 0x20;
-	/* HDMI Video */
-	new_edid.data[pos++] = 0x00; /* 3D present */
-
-	/* HDMI MODE LEN -- how many entries */
-	new_edid.data[pos++] = 0x20;
-	/* 2160p, specified as short descriptor */
-	new_edid.data[pos++] = 0x01;
-
-	update_edid_csum(new_edid.data, length);
-}
-
 /**
  * kmstest_unset_all_crtcs:
  * @drm_fd: the DRM fd
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index f72508640712..203719878d86 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -195,7 +195,6 @@ enum intel_broadcast_rgb_mode {
 bool kmstest_force_connector(int fd, drmModeConnector *connector,
 			     enum kmstest_force_connector_state state);
 void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
-void kmstest_edid_add_4k(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
 void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
 			const unsigned char *edid);
 
@@ -763,6 +762,7 @@ const unsigned char *igt_kms_get_base_edid(void);
 const unsigned char *igt_kms_get_alt_edid(void);
 const unsigned char *igt_kms_get_hdmi_audio_edid(void);
 const unsigned char *igt_kms_get_dp_audio_edid(void);
+const unsigned char *igt_kms_get_4k_edid(void);
 
 struct udev_monitor *igt_watch_hotplug(void);
 bool igt_hotplug_detected(struct udev_monitor *mon,
diff --git a/lib/tests/igt_edid.c b/lib/tests/igt_edid.c
index a847df272525..1a78b38a945b 100644
--- a/lib/tests/igt_edid.c
+++ b/lib/tests/igt_edid.c
@@ -76,6 +76,7 @@ igt_simple_main
 		{ "base", igt_kms_get_base_edid, 0 },
 		{ "alt", igt_kms_get_alt_edid, 0 },
 		{ "hdmi_audio", igt_kms_get_hdmi_audio_edid, 1 },
+		{ "4k", igt_kms_get_4k_edid, 1 },
 		{0},
 	}, *f;
 	const unsigned char *edid;
diff --git a/lib/tests/igt_hdmi_inject.c b/lib/tests/igt_hdmi_inject.c
index 2534b1a23acb..c70c3195cb1d 100644
--- a/lib/tests/igt_hdmi_inject.c
+++ b/lib/tests/igt_hdmi_inject.c
@@ -72,7 +72,6 @@ igt_simple_main
 		hdmi_inject_func inject;
 	} funcs[] = {
 		{ "3D", kmstest_edid_add_3d },
-		{ "4k", kmstest_edid_add_4k },
 		{ NULL, NULL },
 	}, *f;
 
diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
index 9a968fa9e50e..60198f529817 100644
--- a/tests/kms_hdmi_inject.c
+++ b/tests/kms_hdmi_inject.c
@@ -76,8 +76,7 @@ get_connector(int drm_fd, drmModeRes *res)
 static void
 hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
 {
-	unsigned char *edid;
-	size_t length;
+	const unsigned char *edid;
 	struct kmstest_connector_config config;
 	int ret, cid, i, crtc_mask = -1;
 	int fb_id;
@@ -90,9 +89,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
 	/* 4K requires at least HSW */
 	igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
 
-	kmstest_edid_add_4k(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
-			    &length);
-
+	edid = igt_kms_get_4k_edid();
 	kmstest_force_edid(drm_fd, connector, edid);
 
 	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
@@ -135,8 +132,6 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
 
 	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
 	kmstest_force_edid(drm_fd, connector, NULL);
-
-	free(edid);
 }
 
 static void
-- 
2.22.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/5] lib/igt_kms: remove length parameter from kmstest_force_edid

[Ville Syrjälä]

On Tue, Jul 02, 2019 at 03:50:34PM +0300, Simon Ser wrote:
> Given an EDID, computing the size is trivial. Instead of having one size
> constant per EDID and hope the callers use the right one (ie. *not* EDID_LENGTH
> when there's an extension), we can make functions that take EDIDs compute the
> size if they need it.
> 
> We have tests in lib/tests/igt_edid.c which assert the number of extensions
> present in the EDID anyway.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  lib/igt_kms.c                     |  9 ++++++---
>  lib/igt_kms.h                     |  3 +--
>  tests/kms_3d.c                    |  4 ++--
>  tests/kms_force_connector_basic.c | 12 ++++++------
>  tests/kms_hdmi_inject.c           | 11 ++++-------
>  5 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index dc8992cb043b..f8185edf6e8b 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -182,6 +182,8 @@ const unsigned char *igt_kms_get_alt_edid(void)
>  	return (unsigned char *) &edid;
>  }
> 
> +#define AUDIO_EDID_LENGTH (2 * EDID_LENGTH)
> +
>  static void
>  generate_audio_edid(unsigned char raw_edid[static AUDIO_EDID_LENGTH],
>  		    bool with_vsd, struct cea_sad *sad,
> @@ -998,7 +1000,7 @@ bool kmstest_force_connector(int drm_fd, drmModeConnector *connector,
>   * If @length is zero, the forced EDID will be removed.
        ^^^^^^^^
Please fix the docs too.

With that
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>   */
>  void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
> -			const unsigned char *edid, size_t length)
> +			const unsigned char *edid)
>  {
>  	char *path;
>  	int debugfs_fd, ret;
> @@ -1011,10 +1013,11 @@ void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
> 
>  	igt_require(debugfs_fd != -1);
> 
> -	if (length == 0)
> +	if (edid == NULL)
>  		ret = write(debugfs_fd, "reset", 5);
>  	else
> -		ret = write(debugfs_fd, edid, length);
> +		ret = write(debugfs_fd, edid,
> +			    edid_get_size((struct edid *) edid));
>  	close(debugfs_fd);
> 
>  	/* To allow callers to always use GetConnectorCurrent we need to force a
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index a448a003ae56..f72508640712 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -197,7 +197,7 @@ bool kmstest_force_connector(int fd, drmModeConnector *connector,
>  void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
>  void kmstest_edid_add_4k(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
>  void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
> -			const unsigned char *edid, size_t length);
> +			const unsigned char *edid);
> 
>  bool kmstest_get_connector_default_mode(int drm_fd, drmModeConnector *connector,
>  					drmModeModeInfo *mode);
> @@ -759,7 +759,6 @@ struct cea_sad;
>  struct cea_speaker_alloc;
> 
>  #define EDID_LENGTH 128
> -#define AUDIO_EDID_LENGTH (2 * EDID_LENGTH)
>  const unsigned char *igt_kms_get_base_edid(void);
>  const unsigned char *igt_kms_get_alt_edid(void);
>  const unsigned char *igt_kms_get_hdmi_audio_edid(void);
> diff --git a/tests/kms_3d.c b/tests/kms_3d.c
> index df8185abebc4..a170814f6356 100644
> --- a/tests/kms_3d.c
> +++ b/tests/kms_3d.c
> @@ -60,7 +60,7 @@ igt_simple_main
>  	kmstest_edid_add_3d(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
>  			    &length);
> 
> -	kmstest_force_edid(drm_fd, connector, edid, length);
> +	kmstest_force_edid(drm_fd, connector, edid);
>  	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
>  		igt_skip("Could not force connector on\n");
> 
> @@ -113,7 +113,7 @@ igt_simple_main
>  	}
> 
>  	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
> -	kmstest_force_edid(drm_fd, connector, NULL, 0);
> +	kmstest_force_edid(drm_fd, connector, NULL);
> 
>  	drmModeFreeConnector(connector);
>  	free(edid);
> diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
> index 20812d5e3189..f1533e5415c0 100644
> --- a/tests/kms_force_connector_basic.c
> +++ b/tests/kms_force_connector_basic.c
> @@ -48,7 +48,7 @@ static void reset_connectors(void)
>  		kmstest_force_connector(drm_fd, connector,
>  					FORCE_CONNECTOR_UNSPECIFIED);
> 
> -		kmstest_force_edid(drm_fd, connector, NULL, 0);
> +		kmstest_force_edid(drm_fd, connector, NULL);
> 
>  		drmModeFreeConnector(connector);
>  	}
> @@ -247,7 +247,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)
> 
>  		/* test edid forcing */
>  		kmstest_force_edid(drm_fd, vga_connector,
> -				   igt_kms_get_base_edid(), EDID_LENGTH);
> +				   igt_kms_get_base_edid());
>  		temp = drmModeGetConnectorCurrent(drm_fd,
>  						  vga_connector->connector_id);
> 
> @@ -260,7 +260,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)
>  		drmModeFreeConnector(temp);
> 
>  		/* remove edid */
> -		kmstest_force_edid(drm_fd, vga_connector, NULL, 0);
> +		kmstest_force_edid(drm_fd, vga_connector, NULL);
>  		kmstest_force_connector(drm_fd, vga_connector,
>  					FORCE_CONNECTOR_UNSPECIFIED);
>  		temp = drmModeGetConnectorCurrent(drm_fd,
> @@ -280,7 +280,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)
> 
>  		/* test pruning of stale modes */
>  		kmstest_force_edid(drm_fd, vga_connector,
> -				   igt_kms_get_alt_edid(), EDID_LENGTH);
> +				   igt_kms_get_alt_edid());
>  		temp = drmModeGetConnectorCurrent(drm_fd,
>  						  vga_connector->connector_id);
> 
> @@ -294,7 +294,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)
>  		drmModeFreeConnector(temp);
> 
>  		kmstest_force_edid(drm_fd, vga_connector,
> -				   igt_kms_get_base_edid(), EDID_LENGTH);
> +				   igt_kms_get_base_edid());
>  		temp = drmModeGetConnectorCurrent(drm_fd,
>  						  vga_connector->connector_id);
> 
> @@ -307,7 +307,7 @@ igt_main_args("", long_opts, help_str, opt_handler, NULL)
> 
>  		drmModeFreeConnector(temp);
> 
> -		kmstest_force_edid(drm_fd, vga_connector, NULL, 0);
> +		kmstest_force_edid(drm_fd, vga_connector, NULL);
>  		kmstest_force_connector(drm_fd, vga_connector,
>  					FORCE_CONNECTOR_UNSPECIFIED);
>  	}
> diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
> index 8c0d1333db19..9a968fa9e50e 100644
> --- a/tests/kms_hdmi_inject.c
> +++ b/tests/kms_hdmi_inject.c
> @@ -93,7 +93,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
>  	kmstest_edid_add_4k(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
>  			    &length);
> 
> -	kmstest_force_edid(drm_fd, connector, edid, length);
> +	kmstest_force_edid(drm_fd, connector, edid);
> 
>  	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
>  		igt_skip("Could not force connector on\n");
> @@ -134,7 +134,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
>  	igt_remove_fb(drm_fd, &fb);
> 
>  	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
> -	kmstest_force_edid(drm_fd, connector, NULL, 0);
> +	kmstest_force_edid(drm_fd, connector, NULL);
> 
>  	free(edid);
>  }
> @@ -143,15 +143,12 @@ static void
>  hdmi_inject_audio(int drm_fd, drmModeConnector *connector)
>  {
>  	const unsigned char *edid;
> -	size_t length;
>  	int fb_id, cid, ret, crtc_mask = -1;
>  	struct igt_fb fb;
>  	struct kmstest_connector_config config;
> 
>  	edid = igt_kms_get_hdmi_audio_edid();
> -	length = AUDIO_EDID_LENGTH;
> -
> -	kmstest_force_edid(drm_fd, connector, edid, length);
> +	kmstest_force_edid(drm_fd, connector, edid);
> 
>  	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
>  		igt_skip("Could not force connector on\n");
> @@ -191,7 +188,7 @@ hdmi_inject_audio(int drm_fd, drmModeConnector *connector)
>  	kmstest_dump_mode(&connector->modes[0]);
> 
>  	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
> -	kmstest_force_edid(drm_fd, connector, NULL, 0);
> +	kmstest_force_edid(drm_fd, connector, NULL);
>  }
> 
>  igt_main
> --
> 2.22.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/5] lib/igt_edid: add hdmi_vsd

[Ville Syrjälä]

On Tue, Jul 02, 2019 at 03:50:35PM +0300, Simon Ser wrote:
> The HDMI Vendor-Specific Data block, defined as an opaque blob in the EDID
> spec, is described in the HDMI spec.

You may want to clarify that this is the HDMI 1.4 thing, not the
HDMI 2.0 "HDMI Forum" thing.

> 
> Most of the extension fields are optional, so it doesn't translate well to a C
> struct. For now callers need to manually fill the hdmi_vsd.data field.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  lib/igt_edid.c | 40 ++++++++++++++++++++++++++++++++--------
>  lib/igt_edid.h | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 77 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> index cbb7eefff70d..c93d8fa8bca0 100644
> --- a/lib/igt_edid.c
> +++ b/lib/igt_edid.c
> @@ -314,6 +314,14 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
>  	sad->bitrate = sample_sizes;
>  }
>  
> +static void cea_vsd_set_hdmi(struct cea_vsd *vsd)

vsdb is the acronym used by the specs. I would suggest a sed job
over the whole thing to align with that. The name of the function
is a bit vague. ..._set_ieee_oui_hdmi() ?

> +{
> +	/* HDMI's IEEE Registration Identifier */
> +	vsd->ieee_oui[0] = 0x03;
> +	vsd->ieee_oui[1] = 0x0C;
> +	vsd->ieee_oui[2] = 0x00;
> +}
> +
>  /**
>   * cea_vsd_get_hdmi_default:
>   *
> @@ -321,21 +329,22 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
>   */
>  const struct cea_vsd *cea_vsd_get_hdmi_default(size_t *size)
>  {
> -	static char raw[sizeof(struct cea_vsd) + 4] = {0};
> +	static char raw[CEA_VSD_HDMI_MIN_SIZE + 2] = {0};

A comment explaining the 2 might be helful.

Otherwise
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	struct cea_vsd *vsd;
> +	struct hdmi_vsd *hdmi;
>  
>  	*size = sizeof(raw);
>  
>  	/* Magic incantation. Works better if you orient your screen in the
>  	 * direction of the VESA headquarters. */
>  	vsd = (struct cea_vsd *) raw;
> -	vsd->ieee_oui[0] = 0x03;
> -	vsd->ieee_oui[1] = 0x0C;
> -	vsd->ieee_oui[2] = 0x00;
> -	vsd->data[0] = 0x10;
> -	vsd->data[1] = 0x00;
> -	vsd->data[2] = 0x38;
> -	vsd->data[3] = 0x2D;
> +	cea_vsd_set_hdmi(vsd);
> +	hdmi = &vsd->data.hdmi;
> +	hdmi->src_phy_addr[0] = 0x10;
> +	hdmi->src_phy_addr[1] = 0x00;
> +	/* 2 VSD extension fields */
> +	hdmi->flags1 = 0x38;
> +	hdmi->max_tdms_clock = 0x2D;
>  
>  	return vsd;
>  }
> @@ -371,6 +380,21 @@ size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
>  	return sizeof(struct edid_cea_data_block) + vsd_size;
>  }
>  
> +size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
> +					const struct hdmi_vsd *hdmi,
> +					size_t hdmi_size)
> +{
> +	char raw_vsd[CEA_VSD_HDMI_MAX_SIZE] = {0};
> +	struct cea_vsd *vsd = (struct cea_vsd *) raw_vsd;
> +
> +	assert(hdmi_size >= HDMI_VSD_MIN_SIZE && hdmi_size <= HDMI_VSD_MAX_SIZE);
> +	cea_vsd_set_hdmi(vsd);
> +	memcpy(&vsd->data.hdmi, hdmi, hdmi_size);
> +
> +	return edid_cea_data_block_set_vsd(block, vsd,
> +					   CEA_VSD_HEADER_SIZE + hdmi_size);
> +}
> +
>  size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
>  					     const struct cea_speaker_alloc *speakers)
>  {
> diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> index 47581bb778b0..0ac851c0d74a 100644
> --- a/lib/igt_edid.h
> +++ b/lib/igt_edid.h
> @@ -191,12 +191,50 @@ struct cea_sad {
>  	uint8_t bitrate;
>  } __attribute__((packed));
>  
> -/* Vendor Specific Data */
> -struct cea_vsd {
> -	uint8_t ieee_oui[3];
> -	char data[];
> +enum hdmi_vsd_flags1 {
> +	HDMI_VSD_DVI_DUAL = 1 << 0,
> +	HDMI_VSD_DC_Y444 = 1 << 3, /* supports YCbCr 4:4:4 */
> +	HDMI_VSD_DC_30BIT = 1 << 4, /* 30 bits per pixel */
> +	HDMI_VSD_DC_36BIT = 1 << 5, /* 36 bits per pixel */
> +	HDMI_VSD_DC_48BIT = 1 << 6, /* 48 bits per pixel */
> +	HDMI_VSD_SUPPORTS_AI = 1 << 7, /* supports ACP, ISRC1 or ISRC2 packets */
> +};
> +
> +enum hdmi_vsd_flags2 {
> +	HDMI_VSD_CNC_GRAPHICS = 1 << 0,
> +	HDMI_VSD_CNC_PHOTO = 1 << 1,
> +	HDMI_VSD_CNC_CINEMA = 1 << 2,
> +	HDMI_VSD_CNC_GAME = 1 << 3,
> +	HDMI_VSD_VIDEO_PRESENT = 1 << 5,
> +	HDMI_VSD_INTERLACED_LATENCY_PRESENT = 1 << 6,
> +	HDMI_VSD_LATENCY_PRESENT = 1 << 7,
>  };
>  
> +/* HDMI Vendor-Specific Data Block (defined in the HDMI spec) */
> +struct hdmi_vsd {
> +	uint8_t src_phy_addr[2]; /* source physical address */
> +
> +	/* Extension fields */
> +	uint8_t flags1; /* enum hdmi_vsd_flags1 */
> +	uint8_t max_tdms_clock; /* multiply by 5MHz */
> +	uint8_t flags2; /* enum hdmi_vsd_flags2 */
> +	char data[]; /* latency, misc, VIC, 3D */
> +} __attribute__((packed));
> +
> +#define HDMI_VSD_MIN_SIZE 2 /* just the source physical address */
> +#define HDMI_VSD_MAX_SIZE 28
> +#define CEA_VSD_HEADER_SIZE 3 /* IEEE OUI */
> +#define CEA_VSD_HDMI_MIN_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MIN_SIZE)
> +#define CEA_VSD_HDMI_MAX_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MAX_SIZE)
> +
> +/* Vendor-Specific Data */
> +struct cea_vsd {
> +	uint8_t ieee_oui[3]; /* 24-bit IEEE Registration Identifier, LSB */
> +	union {
> +		struct hdmi_vsd hdmi;
> +	} data;
> +} __attribute__((packed));
> +
>  enum cea_speaker_alloc_item {
>  	CEA_SPEAKER_FRONT_LEFT_RIGHT = 1 << 0,
>  	CEA_SPEAKER_LFE = 1 << 1,
> @@ -315,6 +353,9 @@ size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
>  				   const struct cea_sad *sads, size_t sads_len);
>  size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
>  				   const struct cea_vsd *vsd, size_t vsd_size);
> +size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
> +					const struct hdmi_vsd *hdmi,
> +					size_t hdmi_size);
>  size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
>  					     const struct cea_speaker_alloc *speakers);
>  void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
> -- 
> 2.22.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ GitLab.Pipeline: warning for series starting with [i-g-t,1/5] lib/igt_kms: remove length parameter from kmstest_force_edid

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/5] lib/igt_kms: remove length parameter from kmstest_force_edid
URL   : https://patchwork.freedesktop.org/series/63072/
State : warning

== Summary ==

Pipeline status: FAILED.

See https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/46122 for more details.

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/46122
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/5] lib/igt_kms: remove length parameter from kmstest_force_edid

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/5] lib/igt_kms: remove length parameter from kmstest_force_edid
URL   : https://patchwork.freedesktop.org/series/63072/
State : success

== Summary ==

CI Bug Log - changes from IGT_5079 -> IGTPW_3222
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/63072/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_3222 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_basic@create-close:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/fi-icl-u3/igt@gem_basic@create-close.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/fi-icl-u3/igt@gem_basic@create-close.html

  * igt@i915_selftest@live_blt:
    - fi-skl-iommu:       [PASS][3] -> [INCOMPLETE][4] ([fdo#108602])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/fi-skl-iommu/igt@i915_selftest@live_blt.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/fi-skl-iommu/igt@i915_selftest@live_blt.html

  
#### Possible fixes ####

  * igt@core_auth@basic-auth:
    - fi-icl-u3:          [DMESG-WARN][5] ([fdo#107724]) -> [PASS][6] +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/fi-icl-u3/igt@core_auth@basic-auth.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/fi-icl-u3/igt@core_auth@basic-auth.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][7] ([fdo#109485]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (53 -> 46)
------------------------------

  Additional (1): fi-pnv-d510 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_5079 -> IGTPW_3222

  CI_DRM_6395: f80b22d5965c56053080a5ce2234d52bdec871c1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3222: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/
  IGT_5079: 873df2fa9e8f5fd02d4532b30ef2579f4fe4f27f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/5] lib/igt_edid: add support for native DTDs in CEA extension blocks

[Ville Syrjälä]

On Tue, Jul 02, 2019 at 03:50:36PM +0300, Simon Ser wrote:
> Native Detailed Timing Descriptors follow the Data Block Collection in the CEA
> extension.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  lib/igt_edid.c | 6 ++++--
>  lib/igt_edid.h | 2 +-
>  lib/igt_kms.c  | 3 +--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> index c93d8fa8bca0..93202c138161 100644
> --- a/lib/igt_edid.c
> +++ b/lib/igt_edid.c
> @@ -408,15 +408,17 @@ size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
>  }
>  
>  void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
> -		      uint8_t flags)
> +		      size_t native_dtds_len, uint8_t flags)

size_t is a an odd choice for something that is not a size.
I would have also called it num_native_dtds or something.

>  {
>  	struct edid_cea *cea = &ext->data.cea;
>  
>  	ext->tag = EDID_EXT_CEA;
>  
> +	assert(native_dtds_len <= 0x0F);

The CEA extension can contain up to six DTDs and the base block
can have up to four. So we could make this assert even tighter:

assert(num_native_dtds <= (127 - 4 - data_block_size) / 18 + 4);

or something. But since that wouldn't check against the actual
number of dtds present maybe a bit pointless to do that.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	assert((flags & 0x0F) == 0);
>  	cea->revision = 3;
>  	cea->dtd_start = 4 + data_blocks_size;
> -	cea->misc = flags; /* just flags, no DTD */
> +	cea->misc = flags | native_dtds_len;
>  }
>  
>  void edid_ext_update_cea_checksum(struct edid_ext *ext)
> diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> index 0ac851c0d74a..702c14c54b5e 100644
> --- a/lib/igt_edid.h
> +++ b/lib/igt_edid.h
> @@ -359,6 +359,6 @@ size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
>  size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
>  					     const struct cea_speaker_alloc *speakers);
>  void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
> -		      uint8_t flags);
> +		      size_t native_dtds_len, uint8_t flags);
>  
>  #endif
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index f8185edf6e8b..45c79a380daf 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -226,8 +226,7 @@ generate_audio_edid(unsigned char raw_edid[static AUDIO_EDID_LENGTH],
>  
>  	assert(cea_data_size <= sizeof(edid_cea->data));
>  
> -	edid_ext_set_cea(edid_ext, cea_data_size,
> -			 EDID_CEA_BASIC_AUDIO);
> +	edid_ext_set_cea(edid_ext, cea_data_size, 0, EDID_CEA_BASIC_AUDIO);
>  
>  	edid_update_checksum(edid);
>  	edid_ext_update_cea_checksum(edid_ext);
> -- 
> 2.22.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/5] lib/igt_edid: add support for Short Video Descriptors

[Ville Syrjälä]

On Tue, Jul 02, 2019 at 03:50:37PM +0300, Simon Ser wrote:
> Short Video Descriptors (SVDs, not to be confused with Vendor-Specific Data
> blocks) describe CEA video formats supported by the monitor.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  lib/igt_edid.c | 8 ++++++++
>  lib/igt_edid.h | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> index 93202c138161..9a365dc40352 100644
> --- a/lib/igt_edid.c
> +++ b/lib/igt_edid.c
> @@ -369,6 +369,14 @@ size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
>  	return sizeof(struct edid_cea_data_block) + sads_size;
>  }
>  
> +size_t edid_cea_data_block_set_svd(struct edid_cea_data_block *block,
> +				   const uint8_t *svds, size_t svds_len)
> +{
> +	edid_cea_data_block_init(block, EDID_CEA_DATA_VIDEO, svds_len);
> +	memcpy(block->data.svds, svds, svds_len);
> +	return sizeof(struct edid_cea_data_block) + svds_len;
> +}
> +
>  size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
>  				   const struct cea_vsd *vsd, size_t vsd_size)
>  {
> diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> index 702c14c54b5e..55bdf8a1037b 100644
> --- a/lib/igt_edid.h
> +++ b/lib/igt_edid.h
> @@ -191,6 +191,9 @@ struct cea_sad {
>  	uint8_t bitrate;
>  } __attribute__((packed));
>  
> +/* Indicates that a Short Video Descriptor is native */
> +#define CEA_SVD_NATIVE (1 << 7)
> +
>  enum hdmi_vsd_flags1 {
>  	HDMI_VSD_DVI_DUAL = 1 << 0,
>  	HDMI_VSD_DC_Y444 = 1 << 3, /* supports YCbCr 4:4:4 */
> @@ -261,6 +264,7 @@ struct edid_cea_data_block {
>  	uint8_t type_len; /* type is from enum edid_cea_data_type */
>  	union {
>  		struct cea_sad sads[0];
> +		uint8_t svds[0]; /* Short Video Descriptors */
>  		struct cea_vsd vsds[0];
>  		struct cea_speaker_alloc speakers[0];
>  	} data;
> @@ -351,6 +355,8 @@ void edid_ext_update_cea_checksum(struct edid_ext *ext);
>  const struct cea_vsd *cea_vsd_get_hdmi_default(size_t *size);
>  size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
>  				   const struct cea_sad *sads, size_t sads_len);
> +size_t edid_cea_data_block_set_svd(struct edid_cea_data_block *block,
> +				   const uint8_t *svds, size_t svds_len);
>  size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
>  				   const struct cea_vsd *vsd, size_t vsd_size);
>  size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
> -- 
> 2.22.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 5/5] lib/igt_kms: use igt_edid to generate a 4K EDID

[Ville Syrjälä]

On Tue, Jul 02, 2019 at 03:50:38PM +0300, Simon Ser wrote:
> The new EDID has been byte-by-byte checked to be exactly the same as before.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  lib/igt_kms.c               | 107 ++++++++++++++++++++----------------
>  lib/igt_kms.h               |   2 +-
>  lib/tests/igt_edid.c        |   1 +
>  lib/tests/igt_hdmi_inject.c |   1 -
>  tests/kms_hdmi_inject.c     |   9 +--
>  5 files changed, 64 insertions(+), 56 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 45c79a380daf..444605e4b110 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -284,6 +284,66 @@ const unsigned char *igt_kms_get_dp_audio_edid(void)
>  	return raw_edid;
>  }
>  
> +static const uint8_t edid_4k_svds[] = {
> +	32 | CEA_SVD_NATIVE, /* 1080p @ 24Hz (native) */
> +	5,                   /* 1080i @ 60Hz */
> +	20,                  /* 1080i @ 50Hz */
> +	4,                   /* 720p @ 60Hz */
> +	19,                  /* 720p @ 50Hz */
> +};
> +
> +const unsigned char *igt_kms_get_4k_edid(void)
> +{
> +	static unsigned char raw_edid[256];
> +	struct edid *edid;
> +	struct edid_ext *edid_ext;
> +	struct edid_cea *edid_cea;
> +	char *cea_data;
> +	struct edid_cea_data_block *block;
> +	char raw_hdmi[HDMI_VSD_MIN_SIZE + 6] = {0};
> +	struct hdmi_vsd *hdmi;
> +	size_t cea_data_size = 0;
> +	size_t svds_len;
> +
> +	/* Create a new EDID from the base IGT EDID, and add an
> +	 * extension that advertises 4K support. */
> +	edid = (struct edid *) raw_edid;
> +	memcpy(edid, igt_kms_get_base_edid(), sizeof(struct edid));

That only contains the base block. Are we leaving stack garbage in the
extension block?

> +	edid->extensions_len = 1;
> +	edid_ext = &edid->extensions[0];
> +	edid_cea = &edid_ext->data.cea;
> +	cea_data = edid_cea->data;
> +
> +	/* Short Video Descriptor */
> +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> +	svds_len = sizeof(edid_4k_svds) / sizeof(edid_4k_svds[0]);

I think we have an ARRAY_SIZE(). Although since we know these are bytes
maybe just a sizeof() would be good enough.

> +	cea_data_size += edid_cea_data_block_set_svd(block, edid_4k_svds,
> +						     svds_len);
> +
> +	/* Vendor Specific Data block */
> +	hdmi = (struct hdmi_vsd *) raw_hdmi;
> +	hdmi->src_phy_addr[0] = 0x10;
> +	hdmi->src_phy_addr[1] = 0x00;
> +	hdmi->flags1 = 0;
> +	hdmi->max_tdms_clock = 0;
> +	hdmi->flags2 = HDMI_VSD_VIDEO_PRESENT;
> +	hdmi->data[0] = 0x00; /* HDMI video flags */
> +	hdmi->data[1] = 1 << 5; /* 1 VIC entry, 0 3D entries */
> +	hdmi->data[2] = 0x01; /* 2160p, specified as short descriptor */
> +
> +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> +	cea_data_size += edid_cea_data_block_set_hdmi_vsd(block, hdmi,
> +							  sizeof(raw_hdmi));
> +
> +	assert(cea_data_size <= sizeof(edid_cea->data));
> +
> +	edid_ext_set_cea(edid_ext, cea_data_size, 0, 0);
> +
> +	edid_update_checksum(edid);
> +	edid_ext_update_cea_checksum(edid_ext);
> +	return raw_edid;
> +}
> +
>  const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>  	[IGT_PLANE_SRC_X] = "SRC_X",
>  	[IGT_PLANE_SRC_Y] = "SRC_Y",
> @@ -1348,8 +1408,6 @@ struct edid_block {
>      unsigned char *data;
>  };
>  
> -#define DTD_SUPPORTS_AUDIO 1<<6
> -
>  static struct edid_block
>  init_cea_block(const unsigned char *edid, size_t length,
>  	       unsigned char *new_edid_ptr[], size_t *new_length,
> @@ -1437,51 +1495,6 @@ void kmstest_edid_add_3d(const unsigned char *edid, size_t length,
>  	update_edid_csum(new_edid.data, length);
>  }
>  
> -/**
> - * kmstest_edid_add_4k:
> - * @edid: an existing valid edid block
> - * @length: length of @edid
> - * @new_edid_ptr: pointer to where the new edid will be placed
> - * @new_length: pointer to the size of the new edid
> - *
> - * Makes a copy of an existing edid block and adds an extension indicating
> - * a HDMI 4K mode in vsdb.
> - */
> -void kmstest_edid_add_4k(const unsigned char *edid, size_t length,
> -			 unsigned char *new_edid_ptr[], size_t *new_length)
> -{
> -	char vsdb_block_len = 12;
> -	struct edid_block new_edid = init_cea_block(edid, length, new_edid_ptr,
> -						    new_length, vsdb_block_len,
> -						    0);
> -	int pos = new_edid.pos;
> -
> -	/* vsdb block ( id | length ) */
> -	new_edid.data[pos++] = 3 << 5 | (vsdb_block_len - 1);
> -	/* registration id */
> -	new_edid.data[pos++] = 0x3;
> -	new_edid.data[pos++] = 0xc;
> -	new_edid.data[pos++] = 0x0;
> -	/* source physical address */
> -	new_edid.data[pos++] = 0x10;
> -	new_edid.data[pos++] = 0x00;
> -	/* Supports_AI ... etc */
> -	new_edid.data[pos++] = 0x00;
> -	/* Max TMDS Clock */
> -	new_edid.data[pos++] = 0x00;
> -	/* Latency present, HDMI Video Present */
> -	new_edid.data[pos++] = 0x20;
> -	/* HDMI Video */
> -	new_edid.data[pos++] = 0x00; /* 3D present */
> -
> -	/* HDMI MODE LEN -- how many entries */
> -	new_edid.data[pos++] = 0x20;
> -	/* 2160p, specified as short descriptor */
> -	new_edid.data[pos++] = 0x01;
> -
> -	update_edid_csum(new_edid.data, length);
> -}
> -
>  /**
>   * kmstest_unset_all_crtcs:
>   * @drm_fd: the DRM fd
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index f72508640712..203719878d86 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -195,7 +195,6 @@ enum intel_broadcast_rgb_mode {
>  bool kmstest_force_connector(int fd, drmModeConnector *connector,
>  			     enum kmstest_force_connector_state state);
>  void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
> -void kmstest_edid_add_4k(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
>  void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
>  			const unsigned char *edid);
>  
> @@ -763,6 +762,7 @@ const unsigned char *igt_kms_get_base_edid(void);
>  const unsigned char *igt_kms_get_alt_edid(void);
>  const unsigned char *igt_kms_get_hdmi_audio_edid(void);
>  const unsigned char *igt_kms_get_dp_audio_edid(void);
> +const unsigned char *igt_kms_get_4k_edid(void);
>  
>  struct udev_monitor *igt_watch_hotplug(void);
>  bool igt_hotplug_detected(struct udev_monitor *mon,
> diff --git a/lib/tests/igt_edid.c b/lib/tests/igt_edid.c
> index a847df272525..1a78b38a945b 100644
> --- a/lib/tests/igt_edid.c
> +++ b/lib/tests/igt_edid.c
> @@ -76,6 +76,7 @@ igt_simple_main
>  		{ "base", igt_kms_get_base_edid, 0 },
>  		{ "alt", igt_kms_get_alt_edid, 0 },
>  		{ "hdmi_audio", igt_kms_get_hdmi_audio_edid, 1 },
> +		{ "4k", igt_kms_get_4k_edid, 1 },
>  		{0},
>  	}, *f;
>  	const unsigned char *edid;
> diff --git a/lib/tests/igt_hdmi_inject.c b/lib/tests/igt_hdmi_inject.c
> index 2534b1a23acb..c70c3195cb1d 100644
> --- a/lib/tests/igt_hdmi_inject.c
> +++ b/lib/tests/igt_hdmi_inject.c
> @@ -72,7 +72,6 @@ igt_simple_main
>  		hdmi_inject_func inject;
>  	} funcs[] = {
>  		{ "3D", kmstest_edid_add_3d },
> -		{ "4k", kmstest_edid_add_4k },
>  		{ NULL, NULL },
>  	}, *f;
>  
> diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
> index 9a968fa9e50e..60198f529817 100644
> --- a/tests/kms_hdmi_inject.c
> +++ b/tests/kms_hdmi_inject.c
> @@ -76,8 +76,7 @@ get_connector(int drm_fd, drmModeRes *res)
>  static void
>  hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
>  {
> -	unsigned char *edid;
> -	size_t length;
> +	const unsigned char *edid;
>  	struct kmstest_connector_config config;
>  	int ret, cid, i, crtc_mask = -1;
>  	int fb_id;
> @@ -90,9 +89,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
>  	/* 4K requires at least HSW */
>  	igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
>  
> -	kmstest_edid_add_4k(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
> -			    &length);
> -
> +	edid = igt_kms_get_4k_edid();
>  	kmstest_force_edid(drm_fd, connector, edid);
>  
>  	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
> @@ -135,8 +132,6 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
>  
>  	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
>  	kmstest_force_edid(drm_fd, connector, NULL);
> -
> -	free(edid);
>  }
>  
>  static void
> -- 
> 2.22.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 5/5] lib/igt_kms: use igt_edid to generate a 4K EDID

[Ser, Simon]

Thanks for your reviews, these are good points!

Replies inline.

On Tue, 2019-07-02 at 18:51 +0300, Ville Syrjälä wrote:
> On Tue, Jul 02, 2019 at 03:50:38PM +0300, Simon Ser wrote:
> > The new EDID has been byte-by-byte checked to be exactly the same as before.
> > 
> > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > ---
> >  lib/igt_kms.c               | 107 ++++++++++++++++++++----------------
> >  lib/igt_kms.h               |   2 +-
> >  lib/tests/igt_edid.c        |   1 +
> >  lib/tests/igt_hdmi_inject.c |   1 -
> >  tests/kms_hdmi_inject.c     |   9 +--
> >  5 files changed, 64 insertions(+), 56 deletions(-)
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 45c79a380daf..444605e4b110 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -284,6 +284,66 @@ const unsigned char *igt_kms_get_dp_audio_edid(void)
> >  	return raw_edid;
> >  }
> >  
> > +static const uint8_t edid_4k_svds[] = {
> > +	32 | CEA_SVD_NATIVE, /* 1080p @ 24Hz (native) */
> > +	5,                   /* 1080i @ 60Hz */
> > +	20,                  /* 1080i @ 50Hz */
> > +	4,                   /* 720p @ 60Hz */
> > +	19,                  /* 720p @ 50Hz */
> > +};
> > +
> > +const unsigned char *igt_kms_get_4k_edid(void)
> > +{
> > +	static unsigned char raw_edid[256];
> > +	struct edid *edid;
> > +	struct edid_ext *edid_ext;
> > +	struct edid_cea *edid_cea;
> > +	char *cea_data;
> > +	struct edid_cea_data_block *block;
> > +	char raw_hdmi[HDMI_VSD_MIN_SIZE + 6] = {0};
> > +	struct hdmi_vsd *hdmi;
> > +	size_t cea_data_size = 0;
> > +	size_t svds_len;
> > +
> > +	/* Create a new EDID from the base IGT EDID, and add an
> > +	 * extension that advertises 4K support. */
> > +	edid = (struct edid *) raw_edid;
> > +	memcpy(edid, igt_kms_get_base_edid(), sizeof(struct edid));
> 
> That only contains the base block. Are we leaving stack garbage in the
> extension block?

raw_edid is zero-initialized. edid is just a pointer to raw_edid.

> > +	edid->extensions_len = 1;
> > +	edid_ext = &edid->extensions[0];
> > +	edid_cea = &edid_ext->data.cea;
> > +	cea_data = edid_cea->data;
> > +
> > +	/* Short Video Descriptor */
> > +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> > +	svds_len = sizeof(edid_4k_svds) / sizeof(edid_4k_svds[0]);
> 
> I think we have an ARRAY_SIZE(). Although since we know these are bytes
> maybe just a sizeof() would be good enough.

Fair

> > +	cea_data_size += edid_cea_data_block_set_svd(block, edid_4k_svds,
> > +						     svds_len);
> > +
> > +	/* Vendor Specific Data block */
> > +	hdmi = (struct hdmi_vsd *) raw_hdmi;
> > +	hdmi->src_phy_addr[0] = 0x10;
> > +	hdmi->src_phy_addr[1] = 0x00;
> > +	hdmi->flags1 = 0;
> > +	hdmi->max_tdms_clock = 0;
> > +	hdmi->flags2 = HDMI_VSD_VIDEO_PRESENT;
> > +	hdmi->data[0] = 0x00; /* HDMI video flags */
> > +	hdmi->data[1] = 1 << 5; /* 1 VIC entry, 0 3D entries */
> > +	hdmi->data[2] = 0x01; /* 2160p, specified as short descriptor */
> > +
> > +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> > +	cea_data_size += edid_cea_data_block_set_hdmi_vsd(block, hdmi,
> > +							  sizeof(raw_hdmi));
> > +
> > +	assert(cea_data_size <= sizeof(edid_cea->data));
> > +
> > +	edid_ext_set_cea(edid_ext, cea_data_size, 0, 0);
> > +
> > +	edid_update_checksum(edid);
> > +	edid_ext_update_cea_checksum(edid_ext);
> > +	return raw_edid;
> > +}
> > +
> >  const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> >  	[IGT_PLANE_SRC_X] = "SRC_X",
> >  	[IGT_PLANE_SRC_Y] = "SRC_Y",
> > @@ -1348,8 +1408,6 @@ struct edid_block {
> >      unsigned char *data;
> >  };
> >  
> > -#define DTD_SUPPORTS_AUDIO 1<<6
> > -
> >  static struct edid_block
> >  init_cea_block(const unsigned char *edid, size_t length,
> >  	       unsigned char *new_edid_ptr[], size_t *new_length,
> > @@ -1437,51 +1495,6 @@ void kmstest_edid_add_3d(const unsigned char *edid, size_t length,
> >  	update_edid_csum(new_edid.data, length);
> >  }
> >  
> > -/**
> > - * kmstest_edid_add_4k:
> > - * @edid: an existing valid edid block
> > - * @length: length of @edid
> > - * @new_edid_ptr: pointer to where the new edid will be placed
> > - * @new_length: pointer to the size of the new edid
> > - *
> > - * Makes a copy of an existing edid block and adds an extension indicating
> > - * a HDMI 4K mode in vsdb.
> > - */
> > -void kmstest_edid_add_4k(const unsigned char *edid, size_t length,
> > -			 unsigned char *new_edid_ptr[], size_t *new_length)
> > -{
> > -	char vsdb_block_len = 12;
> > -	struct edid_block new_edid = init_cea_block(edid, length, new_edid_ptr,
> > -						    new_length, vsdb_block_len,
> > -						    0);
> > -	int pos = new_edid.pos;
> > -
> > -	/* vsdb block ( id | length ) */
> > -	new_edid.data[pos++] = 3 << 5 | (vsdb_block_len - 1);
> > -	/* registration id */
> > -	new_edid.data[pos++] = 0x3;
> > -	new_edid.data[pos++] = 0xc;
> > -	new_edid.data[pos++] = 0x0;
> > -	/* source physical address */
> > -	new_edid.data[pos++] = 0x10;
> > -	new_edid.data[pos++] = 0x00;
> > -	/* Supports_AI ... etc */
> > -	new_edid.data[pos++] = 0x00;
> > -	/* Max TMDS Clock */
> > -	new_edid.data[pos++] = 0x00;
> > -	/* Latency present, HDMI Video Present */
> > -	new_edid.data[pos++] = 0x20;
> > -	/* HDMI Video */
> > -	new_edid.data[pos++] = 0x00; /* 3D present */
> > -
> > -	/* HDMI MODE LEN -- how many entries */
> > -	new_edid.data[pos++] = 0x20;
> > -	/* 2160p, specified as short descriptor */
> > -	new_edid.data[pos++] = 0x01;
> > -
> > -	update_edid_csum(new_edid.data, length);
> > -}
> > -
> >  /**
> >   * kmstest_unset_all_crtcs:
> >   * @drm_fd: the DRM fd
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index f72508640712..203719878d86 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -195,7 +195,6 @@ enum intel_broadcast_rgb_mode {
> >  bool kmstest_force_connector(int fd, drmModeConnector *connector,
> >  			     enum kmstest_force_connector_state state);
> >  void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
> > -void kmstest_edid_add_4k(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
> >  void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
> >  			const unsigned char *edid);
> >  
> > @@ -763,6 +762,7 @@ const unsigned char *igt_kms_get_base_edid(void);
> >  const unsigned char *igt_kms_get_alt_edid(void);
> >  const unsigned char *igt_kms_get_hdmi_audio_edid(void);
> >  const unsigned char *igt_kms_get_dp_audio_edid(void);
> > +const unsigned char *igt_kms_get_4k_edid(void);
> >  
> >  struct udev_monitor *igt_watch_hotplug(void);
> >  bool igt_hotplug_detected(struct udev_monitor *mon,
> > diff --git a/lib/tests/igt_edid.c b/lib/tests/igt_edid.c
> > index a847df272525..1a78b38a945b 100644
> > --- a/lib/tests/igt_edid.c
> > +++ b/lib/tests/igt_edid.c
> > @@ -76,6 +76,7 @@ igt_simple_main
> >  		{ "base", igt_kms_get_base_edid, 0 },
> >  		{ "alt", igt_kms_get_alt_edid, 0 },
> >  		{ "hdmi_audio", igt_kms_get_hdmi_audio_edid, 1 },
> > +		{ "4k", igt_kms_get_4k_edid, 1 },
> >  		{0},
> >  	}, *f;
> >  	const unsigned char *edid;
> > diff --git a/lib/tests/igt_hdmi_inject.c b/lib/tests/igt_hdmi_inject.c
> > index 2534b1a23acb..c70c3195cb1d 100644
> > --- a/lib/tests/igt_hdmi_inject.c
> > +++ b/lib/tests/igt_hdmi_inject.c
> > @@ -72,7 +72,6 @@ igt_simple_main
> >  		hdmi_inject_func inject;
> >  	} funcs[] = {
> >  		{ "3D", kmstest_edid_add_3d },
> > -		{ "4k", kmstest_edid_add_4k },
> >  		{ NULL, NULL },
> >  	}, *f;
> >  
> > diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
> > index 9a968fa9e50e..60198f529817 100644
> > --- a/tests/kms_hdmi_inject.c
> > +++ b/tests/kms_hdmi_inject.c
> > @@ -76,8 +76,7 @@ get_connector(int drm_fd, drmModeRes *res)
> >  static void
> >  hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> >  {
> > -	unsigned char *edid;
> > -	size_t length;
> > +	const unsigned char *edid;
> >  	struct kmstest_connector_config config;
> >  	int ret, cid, i, crtc_mask = -1;
> >  	int fb_id;
> > @@ -90,9 +89,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> >  	/* 4K requires at least HSW */
> >  	igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
> >  
> > -	kmstest_edid_add_4k(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
> > -			    &length);
> > -
> > +	edid = igt_kms_get_4k_edid();
> >  	kmstest_force_edid(drm_fd, connector, edid);
> >  
> >  	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
> > @@ -135,8 +132,6 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> >  
> >  	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
> >  	kmstest_force_edid(drm_fd, connector, NULL);
> > -
> > -	free(edid);
> >  }
> >  
> >  static void
> > -- 
> > 2.22.0
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/5] lib/igt_kms: remove length parameter from kmstest_force_edid

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/5] lib/igt_kms: remove length parameter from kmstest_force_edid
URL   : https://patchwork.freedesktop.org/series/63072/
State : success

== Summary ==

CI Bug Log - changes from IGT_5079_full -> IGTPW_3222_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/63072/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_3222_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ppgtt@blt-vs-render-ctxn:
    - shard-iclb:         [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#109801])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb6/igt@gem_ppgtt@blt-vs-render-ctxn.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb1/igt@gem_ppgtt@blt-vs-render-ctxn.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108566]) +7 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-apl3/igt@i915_suspend@fence-restore-tiled2untiled.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-apl8/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x128-onscreen:
    - shard-apl:          [PASS][5] -> [FAIL][6] ([fdo#103232])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-apl2/igt@kms_cursor_crc@pipe-a-cursor-128x128-onscreen.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-apl6/igt@kms_cursor_crc@pipe-a-cursor-128x128-onscreen.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-mmap-gtt:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([fdo#103167])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-glk7/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-mmap-gtt.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-glk1/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-mmap-gtt.html

  * igt@kms_frontbuffer_tracking@fbc-indfb-scaledprimary:
    - shard-apl:          [PASS][9] -> [FAIL][10] ([fdo#103167])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-apl6/igt@kms_frontbuffer_tracking@fbc-indfb-scaledprimary.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-apl6/igt@kms_frontbuffer_tracking@fbc-indfb-scaledprimary.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite:
    - shard-iclb:         [PASS][11] -> [FAIL][12] ([fdo#103167]) +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [PASS][13] -> [SKIP][14] ([fdo#109441]) +2 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb3/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-rpm:
    - shard-iclb:         [PASS][15] -> [INCOMPLETE][16] ([fdo#107713]) +1 similar issue
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb3/igt@kms_vblank@pipe-b-ts-continuation-dpms-rpm.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb1/igt@kms_vblank@pipe-b-ts-continuation-dpms-rpm.html

  
#### Possible fixes ####

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][17] ([fdo#110854]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb8/igt@gem_exec_balancer@smoke.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb1/igt@gem_exec_balancer@smoke.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [DMESG-WARN][19] ([fdo#108686]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-glk4/igt@gem_tiled_swapping@non-threaded.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-glk9/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_pm_rpm@i2c:
    - shard-hsw:          [FAIL][21] ([fdo#104097]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-hsw6/igt@i915_pm_rpm@i2c.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-hsw1/igt@i915_pm_rpm@i2c.html

  * igt@i915_suspend@fence-restore-untiled:
    - shard-apl:          [DMESG-WARN][23] ([fdo#108566]) -> [PASS][24] +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-apl4/igt@i915_suspend@fence-restore-untiled.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-apl1/igt@i915_suspend@fence-restore-untiled.html

  * igt@kms_color@pipe-b-degamma:
    - shard-kbl:          [FAIL][25] ([fdo#104782]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-kbl2/igt@kms_color@pipe-b-degamma.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-kbl2/igt@kms_color@pipe-b-degamma.html
    - shard-apl:          [FAIL][27] ([fdo#104782]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-apl3/igt@kms_color@pipe-b-degamma.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-apl7/igt@kms_color@pipe-b-degamma.html
    - shard-glk:          [FAIL][29] ([fdo#104782]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-glk9/igt@kms_color@pipe-b-degamma.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-glk5/igt@kms_color@pipe-b-degamma.html

  * igt@kms_flip@basic-plain-flip:
    - shard-hsw:          [INCOMPLETE][31] ([fdo#103540]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-hsw2/igt@kms_flip@basic-plain-flip.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-hsw8/igt@kms_flip@basic-plain-flip.html

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         [FAIL][33] ([fdo#103167]) -> [PASS][34] +6 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-stridechange.html

  * igt@kms_psr@psr2_sprite_blt:
    - shard-iclb:         [SKIP][35] ([fdo#109441]) -> [PASS][36] +3 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb5/igt@kms_psr@psr2_sprite_blt.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb2/igt@kms_psr@psr2_sprite_blt.html

  
#### Warnings ####

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [DMESG-WARN][37] ([fdo#107724]) -> [SKIP][38] ([fdo#109349])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5079/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/shard-iclb1/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#104097]: https://bugs.freedesktop.org/show_bug.cgi?id=104097
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109801]: https://bugs.freedesktop.org/show_bug.cgi?id=109801
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


Build changes
-------------

  * IGT: IGT_5079 -> IGTPW_3222

  CI_DRM_6395: f80b22d5965c56053080a5ce2234d52bdec871c1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3222: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/
  IGT_5079: 873df2fa9e8f5fd02d4532b30ef2579f4fe4f27f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3222/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/5] lib/igt_edid: add hdmi_vsd

[Ser, Simon]

On Tue, 2019-07-02 at 17:05 +0300, Ville Syrjälä wrote:
> On Tue, Jul 02, 2019 at 03:50:35PM +0300, Simon Ser wrote:
> > The HDMI Vendor-Specific Data block, defined as an opaque blob in the EDID
> > spec, is described in the HDMI spec.
> 
> You may want to clarify that this is the HDMI 1.4 thing, not the
> HDMI 2.0 "HDMI Forum" thing.

Indeed

> > Most of the extension fields are optional, so it doesn't translate well to a C
> > struct. For now callers need to manually fill the hdmi_vsd.data field.
> > 
> > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > ---
> >  lib/igt_edid.c | 40 ++++++++++++++++++++++++++++++++--------
> >  lib/igt_edid.h | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 77 insertions(+), 12 deletions(-)
> > 
> > diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> > index cbb7eefff70d..c93d8fa8bca0 100644
> > --- a/lib/igt_edid.c
> > +++ b/lib/igt_edid.c
> > @@ -314,6 +314,14 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
> >  	sad->bitrate = sample_sizes;
> >  }
> >  
> > +static void cea_vsd_set_hdmi(struct cea_vsd *vsd)
> 
> vsdb is the acronym used by the specs. I would suggest a sed job
> over the whole thing to align with that. The name of the function
> is a bit vague. ..._set_ieee_oui_hdmi() ?

That's a good point. I just defined hdmi_ieee_oui as a constant
instead, which I thought would be nice, but in the end also brings
`extern` and memcpy.

> > +{
> > +	/* HDMI's IEEE Registration Identifier */
> > +	vsd->ieee_oui[0] = 0x03;
> > +	vsd->ieee_oui[1] = 0x0C;
> > +	vsd->ieee_oui[2] = 0x00;
> > +}
> > +
> >  /**
> >   * cea_vsd_get_hdmi_default:
> >   *
> > @@ -321,21 +329,22 @@ void cea_sad_init_pcm(struct cea_sad *sad, int channels,
> >   */
> >  const struct cea_vsd *cea_vsd_get_hdmi_default(size_t *size)
> >  {
> > -	static char raw[sizeof(struct cea_vsd) + 4] = {0};
> > +	static char raw[CEA_VSD_HDMI_MIN_SIZE + 2] = {0};
> 
> A comment explaining the 2 might be helful.

Ack

> Otherwise
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> >  	struct cea_vsd *vsd;
> > +	struct hdmi_vsd *hdmi;
> >  
> >  	*size = sizeof(raw);
> >  
> >  	/* Magic incantation. Works better if you orient your screen in the
> >  	 * direction of the VESA headquarters. */
> >  	vsd = (struct cea_vsd *) raw;
> > -	vsd->ieee_oui[0] = 0x03;
> > -	vsd->ieee_oui[1] = 0x0C;
> > -	vsd->ieee_oui[2] = 0x00;
> > -	vsd->data[0] = 0x10;
> > -	vsd->data[1] = 0x00;
> > -	vsd->data[2] = 0x38;
> > -	vsd->data[3] = 0x2D;
> > +	cea_vsd_set_hdmi(vsd);
> > +	hdmi = &vsd->data.hdmi;
> > +	hdmi->src_phy_addr[0] = 0x10;
> > +	hdmi->src_phy_addr[1] = 0x00;
> > +	/* 2 VSD extension fields */
> > +	hdmi->flags1 = 0x38;
> > +	hdmi->max_tdms_clock = 0x2D;
> >  
> >  	return vsd;
> >  }
> > @@ -371,6 +380,21 @@ size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
> >  	return sizeof(struct edid_cea_data_block) + vsd_size;
> >  }
> >  
> > +size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
> > +					const struct hdmi_vsd *hdmi,
> > +					size_t hdmi_size)
> > +{
> > +	char raw_vsd[CEA_VSD_HDMI_MAX_SIZE] = {0};
> > +	struct cea_vsd *vsd = (struct cea_vsd *) raw_vsd;
> > +
> > +	assert(hdmi_size >= HDMI_VSD_MIN_SIZE && hdmi_size <= HDMI_VSD_MAX_SIZE);
> > +	cea_vsd_set_hdmi(vsd);
> > +	memcpy(&vsd->data.hdmi, hdmi, hdmi_size);
> > +
> > +	return edid_cea_data_block_set_vsd(block, vsd,
> > +					   CEA_VSD_HEADER_SIZE + hdmi_size);
> > +}
> > +
> >  size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
> >  					     const struct cea_speaker_alloc *speakers)
> >  {
> > diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> > index 47581bb778b0..0ac851c0d74a 100644
> > --- a/lib/igt_edid.h
> > +++ b/lib/igt_edid.h
> > @@ -191,12 +191,50 @@ struct cea_sad {
> >  	uint8_t bitrate;
> >  } __attribute__((packed));
> >  
> > -/* Vendor Specific Data */
> > -struct cea_vsd {
> > -	uint8_t ieee_oui[3];
> > -	char data[];
> > +enum hdmi_vsd_flags1 {
> > +	HDMI_VSD_DVI_DUAL = 1 << 0,
> > +	HDMI_VSD_DC_Y444 = 1 << 3, /* supports YCbCr 4:4:4 */
> > +	HDMI_VSD_DC_30BIT = 1 << 4, /* 30 bits per pixel */
> > +	HDMI_VSD_DC_36BIT = 1 << 5, /* 36 bits per pixel */
> > +	HDMI_VSD_DC_48BIT = 1 << 6, /* 48 bits per pixel */
> > +	HDMI_VSD_SUPPORTS_AI = 1 << 7, /* supports ACP, ISRC1 or ISRC2 packets */
> > +};
> > +
> > +enum hdmi_vsd_flags2 {
> > +	HDMI_VSD_CNC_GRAPHICS = 1 << 0,
> > +	HDMI_VSD_CNC_PHOTO = 1 << 1,
> > +	HDMI_VSD_CNC_CINEMA = 1 << 2,
> > +	HDMI_VSD_CNC_GAME = 1 << 3,
> > +	HDMI_VSD_VIDEO_PRESENT = 1 << 5,
> > +	HDMI_VSD_INTERLACED_LATENCY_PRESENT = 1 << 6,
> > +	HDMI_VSD_LATENCY_PRESENT = 1 << 7,
> >  };
> >  
> > +/* HDMI Vendor-Specific Data Block (defined in the HDMI spec) */
> > +struct hdmi_vsd {
> > +	uint8_t src_phy_addr[2]; /* source physical address */
> > +
> > +	/* Extension fields */
> > +	uint8_t flags1; /* enum hdmi_vsd_flags1 */
> > +	uint8_t max_tdms_clock; /* multiply by 5MHz */
> > +	uint8_t flags2; /* enum hdmi_vsd_flags2 */
> > +	char data[]; /* latency, misc, VIC, 3D */
> > +} __attribute__((packed));
> > +
> > +#define HDMI_VSD_MIN_SIZE 2 /* just the source physical address */
> > +#define HDMI_VSD_MAX_SIZE 28
> > +#define CEA_VSD_HEADER_SIZE 3 /* IEEE OUI */
> > +#define CEA_VSD_HDMI_MIN_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MIN_SIZE)
> > +#define CEA_VSD_HDMI_MAX_SIZE (CEA_VSD_HEADER_SIZE + HDMI_VSD_MAX_SIZE)
> > +
> > +/* Vendor-Specific Data */
> > +struct cea_vsd {
> > +	uint8_t ieee_oui[3]; /* 24-bit IEEE Registration Identifier, LSB */
> > +	union {
> > +		struct hdmi_vsd hdmi;
> > +	} data;
> > +} __attribute__((packed));
> > +
> >  enum cea_speaker_alloc_item {
> >  	CEA_SPEAKER_FRONT_LEFT_RIGHT = 1 << 0,
> >  	CEA_SPEAKER_LFE = 1 << 1,
> > @@ -315,6 +353,9 @@ size_t edid_cea_data_block_set_sad(struct edid_cea_data_block *block,
> >  				   const struct cea_sad *sads, size_t sads_len);
> >  size_t edid_cea_data_block_set_vsd(struct edid_cea_data_block *block,
> >  				   const struct cea_vsd *vsd, size_t vsd_size);
> > +size_t edid_cea_data_block_set_hdmi_vsd(struct edid_cea_data_block *block,
> > +					const struct hdmi_vsd *hdmi,
> > +					size_t hdmi_size);
> >  size_t edid_cea_data_block_set_speaker_alloc(struct edid_cea_data_block *block,
> >  					     const struct cea_speaker_alloc *speakers);
> >  void edid_ext_set_cea(struct edid_ext *ext, size_t data_blocks_size,
> > -- 
> > 2.22.0
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 5/5] lib/igt_kms: use igt_edid to generate a 4K EDID

[Ville Syrjälä]

On Wed, Jul 03, 2019 at 08:24:02AM +0000, Ser, Simon wrote:
> Thanks for your reviews, these are good points!
> 
> Replies inline.
> 
> On Tue, 2019-07-02 at 18:51 +0300, Ville Syrjälä wrote:
> > On Tue, Jul 02, 2019 at 03:50:38PM +0300, Simon Ser wrote:
> > > The new EDID has been byte-by-byte checked to be exactly the same as before.
> > > 
> > > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > > ---
> > >  lib/igt_kms.c               | 107 ++++++++++++++++++++----------------
> > >  lib/igt_kms.h               |   2 +-
> > >  lib/tests/igt_edid.c        |   1 +
> > >  lib/tests/igt_hdmi_inject.c |   1 -
> > >  tests/kms_hdmi_inject.c     |   9 +--
> > >  5 files changed, 64 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index 45c79a380daf..444605e4b110 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -284,6 +284,66 @@ const unsigned char *igt_kms_get_dp_audio_edid(void)
> > >  	return raw_edid;
> > >  }
> > >  
> > > +static const uint8_t edid_4k_svds[] = {
> > > +	32 | CEA_SVD_NATIVE, /* 1080p @ 24Hz (native) */
> > > +	5,                   /* 1080i @ 60Hz */
> > > +	20,                  /* 1080i @ 50Hz */
> > > +	4,                   /* 720p @ 60Hz */
> > > +	19,                  /* 720p @ 50Hz */
> > > +};
> > > +
> > > +const unsigned char *igt_kms_get_4k_edid(void)
> > > +{
> > > +	static unsigned char raw_edid[256];
> > > +	struct edid *edid;
> > > +	struct edid_ext *edid_ext;
> > > +	struct edid_cea *edid_cea;
> > > +	char *cea_data;
> > > +	struct edid_cea_data_block *block;
> > > +	char raw_hdmi[HDMI_VSD_MIN_SIZE + 6] = {0};
> > > +	struct hdmi_vsd *hdmi;
> > > +	size_t cea_data_size = 0;
> > > +	size_t svds_len;
> > > +
> > > +	/* Create a new EDID from the base IGT EDID, and add an
> > > +	 * extension that advertises 4K support. */
> > > +	edid = (struct edid *) raw_edid;
> > > +	memcpy(edid, igt_kms_get_base_edid(), sizeof(struct edid));
> > 
> > That only contains the base block. Are we leaving stack garbage in the
> > extension block?
> 
> raw_edid is zero-initialized. edid is just a pointer to raw_edid.

Ah, I failed to notice the static.

> 
> > > +	edid->extensions_len = 1;
> > > +	edid_ext = &edid->extensions[0];
> > > +	edid_cea = &edid_ext->data.cea;
> > > +	cea_data = edid_cea->data;
> > > +
> > > +	/* Short Video Descriptor */
> > > +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> > > +	svds_len = sizeof(edid_4k_svds) / sizeof(edid_4k_svds[0]);
> > 
> > I think we have an ARRAY_SIZE(). Although since we know these are bytes
> > maybe just a sizeof() would be good enough.
> 
> Fair
> 
> > > +	cea_data_size += edid_cea_data_block_set_svd(block, edid_4k_svds,
> > > +						     svds_len);
> > > +
> > > +	/* Vendor Specific Data block */
> > > +	hdmi = (struct hdmi_vsd *) raw_hdmi;
> > > +	hdmi->src_phy_addr[0] = 0x10;
> > > +	hdmi->src_phy_addr[1] = 0x00;
> > > +	hdmi->flags1 = 0;
> > > +	hdmi->max_tdms_clock = 0;
> > > +	hdmi->flags2 = HDMI_VSD_VIDEO_PRESENT;
> > > +	hdmi->data[0] = 0x00; /* HDMI video flags */
> > > +	hdmi->data[1] = 1 << 5; /* 1 VIC entry, 0 3D entries */
> > > +	hdmi->data[2] = 0x01; /* 2160p, specified as short descriptor */
> > > +
> > > +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> > > +	cea_data_size += edid_cea_data_block_set_hdmi_vsd(block, hdmi,
> > > +							  sizeof(raw_hdmi));
> > > +
> > > +	assert(cea_data_size <= sizeof(edid_cea->data));
> > > +
> > > +	edid_ext_set_cea(edid_ext, cea_data_size, 0, 0);
> > > +
> > > +	edid_update_checksum(edid);
> > > +	edid_ext_update_cea_checksum(edid_ext);
> > > +	return raw_edid;
> > > +}
> > > +
> > >  const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> > >  	[IGT_PLANE_SRC_X] = "SRC_X",
> > >  	[IGT_PLANE_SRC_Y] = "SRC_Y",
> > > @@ -1348,8 +1408,6 @@ struct edid_block {
> > >      unsigned char *data;
> > >  };
> > >  
> > > -#define DTD_SUPPORTS_AUDIO 1<<6
> > > -
> > >  static struct edid_block
> > >  init_cea_block(const unsigned char *edid, size_t length,
> > >  	       unsigned char *new_edid_ptr[], size_t *new_length,
> > > @@ -1437,51 +1495,6 @@ void kmstest_edid_add_3d(const unsigned char *edid, size_t length,
> > >  	update_edid_csum(new_edid.data, length);
> > >  }
> > >  
> > > -/**
> > > - * kmstest_edid_add_4k:
> > > - * @edid: an existing valid edid block
> > > - * @length: length of @edid
> > > - * @new_edid_ptr: pointer to where the new edid will be placed
> > > - * @new_length: pointer to the size of the new edid
> > > - *
> > > - * Makes a copy of an existing edid block and adds an extension indicating
> > > - * a HDMI 4K mode in vsdb.
> > > - */
> > > -void kmstest_edid_add_4k(const unsigned char *edid, size_t length,
> > > -			 unsigned char *new_edid_ptr[], size_t *new_length)
> > > -{
> > > -	char vsdb_block_len = 12;
> > > -	struct edid_block new_edid = init_cea_block(edid, length, new_edid_ptr,
> > > -						    new_length, vsdb_block_len,
> > > -						    0);
> > > -	int pos = new_edid.pos;
> > > -
> > > -	/* vsdb block ( id | length ) */
> > > -	new_edid.data[pos++] = 3 << 5 | (vsdb_block_len - 1);
> > > -	/* registration id */
> > > -	new_edid.data[pos++] = 0x3;
> > > -	new_edid.data[pos++] = 0xc;
> > > -	new_edid.data[pos++] = 0x0;
> > > -	/* source physical address */
> > > -	new_edid.data[pos++] = 0x10;
> > > -	new_edid.data[pos++] = 0x00;
> > > -	/* Supports_AI ... etc */
> > > -	new_edid.data[pos++] = 0x00;
> > > -	/* Max TMDS Clock */
> > > -	new_edid.data[pos++] = 0x00;
> > > -	/* Latency present, HDMI Video Present */
> > > -	new_edid.data[pos++] = 0x20;
> > > -	/* HDMI Video */
> > > -	new_edid.data[pos++] = 0x00; /* 3D present */
> > > -
> > > -	/* HDMI MODE LEN -- how many entries */
> > > -	new_edid.data[pos++] = 0x20;
> > > -	/* 2160p, specified as short descriptor */
> > > -	new_edid.data[pos++] = 0x01;
> > > -
> > > -	update_edid_csum(new_edid.data, length);
> > > -}
> > > -
> > >  /**
> > >   * kmstest_unset_all_crtcs:
> > >   * @drm_fd: the DRM fd
> > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > > index f72508640712..203719878d86 100644
> > > --- a/lib/igt_kms.h
> > > +++ b/lib/igt_kms.h
> > > @@ -195,7 +195,6 @@ enum intel_broadcast_rgb_mode {
> > >  bool kmstest_force_connector(int fd, drmModeConnector *connector,
> > >  			     enum kmstest_force_connector_state state);
> > >  void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
> > > -void kmstest_edid_add_4k(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
> > >  void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
> > >  			const unsigned char *edid);
> > >  
> > > @@ -763,6 +762,7 @@ const unsigned char *igt_kms_get_base_edid(void);
> > >  const unsigned char *igt_kms_get_alt_edid(void);
> > >  const unsigned char *igt_kms_get_hdmi_audio_edid(void);
> > >  const unsigned char *igt_kms_get_dp_audio_edid(void);
> > > +const unsigned char *igt_kms_get_4k_edid(void);
> > >  
> > >  struct udev_monitor *igt_watch_hotplug(void);
> > >  bool igt_hotplug_detected(struct udev_monitor *mon,
> > > diff --git a/lib/tests/igt_edid.c b/lib/tests/igt_edid.c
> > > index a847df272525..1a78b38a945b 100644
> > > --- a/lib/tests/igt_edid.c
> > > +++ b/lib/tests/igt_edid.c
> > > @@ -76,6 +76,7 @@ igt_simple_main
> > >  		{ "base", igt_kms_get_base_edid, 0 },
> > >  		{ "alt", igt_kms_get_alt_edid, 0 },
> > >  		{ "hdmi_audio", igt_kms_get_hdmi_audio_edid, 1 },
> > > +		{ "4k", igt_kms_get_4k_edid, 1 },
> > >  		{0},
> > >  	}, *f;
> > >  	const unsigned char *edid;
> > > diff --git a/lib/tests/igt_hdmi_inject.c b/lib/tests/igt_hdmi_inject.c
> > > index 2534b1a23acb..c70c3195cb1d 100644
> > > --- a/lib/tests/igt_hdmi_inject.c
> > > +++ b/lib/tests/igt_hdmi_inject.c
> > > @@ -72,7 +72,6 @@ igt_simple_main
> > >  		hdmi_inject_func inject;
> > >  	} funcs[] = {
> > >  		{ "3D", kmstest_edid_add_3d },
> > > -		{ "4k", kmstest_edid_add_4k },
> > >  		{ NULL, NULL },
> > >  	}, *f;
> > >  
> > > diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
> > > index 9a968fa9e50e..60198f529817 100644
> > > --- a/tests/kms_hdmi_inject.c
> > > +++ b/tests/kms_hdmi_inject.c
> > > @@ -76,8 +76,7 @@ get_connector(int drm_fd, drmModeRes *res)
> > >  static void
> > >  hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> > >  {
> > > -	unsigned char *edid;
> > > -	size_t length;
> > > +	const unsigned char *edid;
> > >  	struct kmstest_connector_config config;
> > >  	int ret, cid, i, crtc_mask = -1;
> > >  	int fb_id;
> > > @@ -90,9 +89,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> > >  	/* 4K requires at least HSW */
> > >  	igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
> > >  
> > > -	kmstest_edid_add_4k(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
> > > -			    &length);
> > > -
> > > +	edid = igt_kms_get_4k_edid();
> > >  	kmstest_force_edid(drm_fd, connector, edid);
> > >  
> > >  	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
> > > @@ -135,8 +132,6 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> > >  
> > >  	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
> > >  	kmstest_force_edid(drm_fd, connector, NULL);
> > > -
> > > -	free(edid);
> > >  }
> > >  
> > >  static void
> > > -- 
> > > 2.22.0
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 5/5] lib/igt_kms: use igt_edid to generate a 4K EDID

[Ser, Simon]

On Wed, 2019-07-03 at 15:58 +0300, Ville Syrjälä wrote:
> On Wed, Jul 03, 2019 at 08:24:02AM +0000, Ser, Simon wrote:
> > Thanks for your reviews, these are good points!
> > 
> > Replies inline.
> > 
> > On Tue, 2019-07-02 at 18:51 +0300, Ville Syrjälä wrote:
> > > On Tue, Jul 02, 2019 at 03:50:38PM +0300, Simon Ser wrote:
> > > > The new EDID has been byte-by-byte checked to be exactly the same as before.
> > > > 
> > > > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > > > ---
> > > >  lib/igt_kms.c               | 107 ++++++++++++++++++++----------------
> > > >  lib/igt_kms.h               |   2 +-
> > > >  lib/tests/igt_edid.c        |   1 +
> > > >  lib/tests/igt_hdmi_inject.c |   1 -
> > > >  tests/kms_hdmi_inject.c     |   9 +--
> > > >  5 files changed, 64 insertions(+), 56 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > > index 45c79a380daf..444605e4b110 100644
> > > > --- a/lib/igt_kms.c
> > > > +++ b/lib/igt_kms.c
> > > > @@ -284,6 +284,66 @@ const unsigned char *igt_kms_get_dp_audio_edid(void)
> > > >  	return raw_edid;
> > > >  }
> > > >  
> > > > +static const uint8_t edid_4k_svds[] = {
> > > > +	32 | CEA_SVD_NATIVE, /* 1080p @ 24Hz (native) */
> > > > +	5,                   /* 1080i @ 60Hz */
> > > > +	20,                  /* 1080i @ 50Hz */
> > > > +	4,                   /* 720p @ 60Hz */
> > > > +	19,                  /* 720p @ 50Hz */
> > > > +};
> > > > +
> > > > +const unsigned char *igt_kms_get_4k_edid(void)
> > > > +{
> > > > +	static unsigned char raw_edid[256];
> > > > +	struct edid *edid;
> > > > +	struct edid_ext *edid_ext;
> > > > +	struct edid_cea *edid_cea;
> > > > +	char *cea_data;
> > > > +	struct edid_cea_data_block *block;
> > > > +	char raw_hdmi[HDMI_VSD_MIN_SIZE + 6] = {0};
> > > > +	struct hdmi_vsd *hdmi;
> > > > +	size_t cea_data_size = 0;
> > > > +	size_t svds_len;
> > > > +
> > > > +	/* Create a new EDID from the base IGT EDID, and add an
> > > > +	 * extension that advertises 4K support. */
> > > > +	edid = (struct edid *) raw_edid;
> > > > +	memcpy(edid, igt_kms_get_base_edid(), sizeof(struct edid));
> > > 
> > > That only contains the base block. Are we leaving stack garbage in the
> > > extension block?
> > 
> > raw_edid is zero-initialized. edid is just a pointer to raw_edid.
> 
> Ah, I failed to notice the static.

Hmm, I guess I should add {0} just for good measure.

> > > > +	edid->extensions_len = 1;
> > > > +	edid_ext = &edid->extensions[0];
> > > > +	edid_cea = &edid_ext->data.cea;
> > > > +	cea_data = edid_cea->data;
> > > > +
> > > > +	/* Short Video Descriptor */
> > > > +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> > > > +	svds_len = sizeof(edid_4k_svds) / sizeof(edid_4k_svds[0]);
> > > 
> > > I think we have an ARRAY_SIZE(). Although since we know these are bytes
> > > maybe just a sizeof() would be good enough.
> > 
> > Fair
> > 
> > > > +	cea_data_size += edid_cea_data_block_set_svd(block, edid_4k_svds,
> > > > +						     svds_len);
> > > > +
> > > > +	/* Vendor Specific Data block */
> > > > +	hdmi = (struct hdmi_vsd *) raw_hdmi;
> > > > +	hdmi->src_phy_addr[0] = 0x10;
> > > > +	hdmi->src_phy_addr[1] = 0x00;
> > > > +	hdmi->flags1 = 0;
> > > > +	hdmi->max_tdms_clock = 0;
> > > > +	hdmi->flags2 = HDMI_VSD_VIDEO_PRESENT;
> > > > +	hdmi->data[0] = 0x00; /* HDMI video flags */
> > > > +	hdmi->data[1] = 1 << 5; /* 1 VIC entry, 0 3D entries */
> > > > +	hdmi->data[2] = 0x01; /* 2160p, specified as short descriptor */
> > > > +
> > > > +	block = (struct edid_cea_data_block *) &cea_data[cea_data_size];
> > > > +	cea_data_size += edid_cea_data_block_set_hdmi_vsd(block, hdmi,
> > > > +							  sizeof(raw_hdmi));
> > > > +
> > > > +	assert(cea_data_size <= sizeof(edid_cea->data));
> > > > +
> > > > +	edid_ext_set_cea(edid_ext, cea_data_size, 0, 0);
> > > > +
> > > > +	edid_update_checksum(edid);
> > > > +	edid_ext_update_cea_checksum(edid_ext);
> > > > +	return raw_edid;
> > > > +}
> > > > +
> > > >  const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> > > >  	[IGT_PLANE_SRC_X] = "SRC_X",
> > > >  	[IGT_PLANE_SRC_Y] = "SRC_Y",
> > > > @@ -1348,8 +1408,6 @@ struct edid_block {
> > > >      unsigned char *data;
> > > >  };
> > > >  
> > > > -#define DTD_SUPPORTS_AUDIO 1<<6
> > > > -
> > > >  static struct edid_block
> > > >  init_cea_block(const unsigned char *edid, size_t length,
> > > >  	       unsigned char *new_edid_ptr[], size_t *new_length,
> > > > @@ -1437,51 +1495,6 @@ void kmstest_edid_add_3d(const unsigned char *edid, size_t length,
> > > >  	update_edid_csum(new_edid.data, length);
> > > >  }
> > > >  
> > > > -/**
> > > > - * kmstest_edid_add_4k:
> > > > - * @edid: an existing valid edid block
> > > > - * @length: length of @edid
> > > > - * @new_edid_ptr: pointer to where the new edid will be placed
> > > > - * @new_length: pointer to the size of the new edid
> > > > - *
> > > > - * Makes a copy of an existing edid block and adds an extension indicating
> > > > - * a HDMI 4K mode in vsdb.
> > > > - */
> > > > -void kmstest_edid_add_4k(const unsigned char *edid, size_t length,
> > > > -			 unsigned char *new_edid_ptr[], size_t *new_length)
> > > > -{
> > > > -	char vsdb_block_len = 12;
> > > > -	struct edid_block new_edid = init_cea_block(edid, length, new_edid_ptr,
> > > > -						    new_length, vsdb_block_len,
> > > > -						    0);
> > > > -	int pos = new_edid.pos;
> > > > -
> > > > -	/* vsdb block ( id | length ) */
> > > > -	new_edid.data[pos++] = 3 << 5 | (vsdb_block_len - 1);
> > > > -	/* registration id */
> > > > -	new_edid.data[pos++] = 0x3;
> > > > -	new_edid.data[pos++] = 0xc;
> > > > -	new_edid.data[pos++] = 0x0;
> > > > -	/* source physical address */
> > > > -	new_edid.data[pos++] = 0x10;
> > > > -	new_edid.data[pos++] = 0x00;
> > > > -	/* Supports_AI ... etc */
> > > > -	new_edid.data[pos++] = 0x00;
> > > > -	/* Max TMDS Clock */
> > > > -	new_edid.data[pos++] = 0x00;
> > > > -	/* Latency present, HDMI Video Present */
> > > > -	new_edid.data[pos++] = 0x20;
> > > > -	/* HDMI Video */
> > > > -	new_edid.data[pos++] = 0x00; /* 3D present */
> > > > -
> > > > -	/* HDMI MODE LEN -- how many entries */
> > > > -	new_edid.data[pos++] = 0x20;
> > > > -	/* 2160p, specified as short descriptor */
> > > > -	new_edid.data[pos++] = 0x01;
> > > > -
> > > > -	update_edid_csum(new_edid.data, length);
> > > > -}
> > > > -
> > > >  /**
> > > >   * kmstest_unset_all_crtcs:
> > > >   * @drm_fd: the DRM fd
> > > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > > > index f72508640712..203719878d86 100644
> > > > --- a/lib/igt_kms.h
> > > > +++ b/lib/igt_kms.h
> > > > @@ -195,7 +195,6 @@ enum intel_broadcast_rgb_mode {
> > > >  bool kmstest_force_connector(int fd, drmModeConnector *connector,
> > > >  			     enum kmstest_force_connector_state state);
> > > >  void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
> > > > -void kmstest_edid_add_4k(const unsigned char *edid, size_t length, unsigned char *new_edid_ptr[], size_t *new_length);
> > > >  void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
> > > >  			const unsigned char *edid);
> > > >  
> > > > @@ -763,6 +762,7 @@ const unsigned char *igt_kms_get_base_edid(void);
> > > >  const unsigned char *igt_kms_get_alt_edid(void);
> > > >  const unsigned char *igt_kms_get_hdmi_audio_edid(void);
> > > >  const unsigned char *igt_kms_get_dp_audio_edid(void);
> > > > +const unsigned char *igt_kms_get_4k_edid(void);
> > > >  
> > > >  struct udev_monitor *igt_watch_hotplug(void);
> > > >  bool igt_hotplug_detected(struct udev_monitor *mon,
> > > > diff --git a/lib/tests/igt_edid.c b/lib/tests/igt_edid.c
> > > > index a847df272525..1a78b38a945b 100644
> > > > --- a/lib/tests/igt_edid.c
> > > > +++ b/lib/tests/igt_edid.c
> > > > @@ -76,6 +76,7 @@ igt_simple_main
> > > >  		{ "base", igt_kms_get_base_edid, 0 },
> > > >  		{ "alt", igt_kms_get_alt_edid, 0 },
> > > >  		{ "hdmi_audio", igt_kms_get_hdmi_audio_edid, 1 },
> > > > +		{ "4k", igt_kms_get_4k_edid, 1 },
> > > >  		{0},
> > > >  	}, *f;
> > > >  	const unsigned char *edid;
> > > > diff --git a/lib/tests/igt_hdmi_inject.c b/lib/tests/igt_hdmi_inject.c
> > > > index 2534b1a23acb..c70c3195cb1d 100644
> > > > --- a/lib/tests/igt_hdmi_inject.c
> > > > +++ b/lib/tests/igt_hdmi_inject.c
> > > > @@ -72,7 +72,6 @@ igt_simple_main
> > > >  		hdmi_inject_func inject;
> > > >  	} funcs[] = {
> > > >  		{ "3D", kmstest_edid_add_3d },
> > > > -		{ "4k", kmstest_edid_add_4k },
> > > >  		{ NULL, NULL },
> > > >  	}, *f;
> > > >  
> > > > diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
> > > > index 9a968fa9e50e..60198f529817 100644
> > > > --- a/tests/kms_hdmi_inject.c
> > > > +++ b/tests/kms_hdmi_inject.c
> > > > @@ -76,8 +76,7 @@ get_connector(int drm_fd, drmModeRes *res)
> > > >  static void
> > > >  hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> > > >  {
> > > > -	unsigned char *edid;
> > > > -	size_t length;
> > > > +	const unsigned char *edid;
> > > >  	struct kmstest_connector_config config;
> > > >  	int ret, cid, i, crtc_mask = -1;
> > > >  	int fb_id;
> > > > @@ -90,9 +89,7 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> > > >  	/* 4K requires at least HSW */
> > > >  	igt_require(IS_HASWELL(devid) || intel_gen(devid) >= 8);
> > > >  
> > > > -	kmstest_edid_add_4k(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
> > > > -			    &length);
> > > > -
> > > > +	edid = igt_kms_get_4k_edid();
> > > >  	kmstest_force_edid(drm_fd, connector, edid);
> > > >  
> > > >  	if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
> > > > @@ -135,8 +132,6 @@ hdmi_inject_4k(int drm_fd, drmModeConnector *connector)
> > > >  
> > > >  	kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_UNSPECIFIED);
> > > >  	kmstest_force_edid(drm_fd, connector, NULL);
> > > > -
> > > > -	free(edid);
> > > >  }
> > > >  
> > > >  static void
> > > > -- 
> > > > 2.22.0
> > > > 
> > > > _______________________________________________
> > > > igt-dev mailing list
> > > > igt-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev