[igt-dev] [PATCH] tests/i915_pm_rpm: improved strictness and verbosity of i2c subtest

* [igt-dev] [PATCH] tests/i915_pm_rpm: improved strictness and verbosity of i2c subtest
@ 2019-05-07  8:51 Oleg Vasilev
  2019-05-07  9:57 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Oleg Vasilev @ 2019-05-07  8:51 UTC (permalink / raw)
  To: igt-dev

Test checked that the number of valid edids from drm is equal to the
same number from i2c.

Now for each edid the whole blob is compared. In case of mismatch the
whole edid is printed.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104097
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
---
 tests/i915/i915_pm_rpm.c | 176 ++++++++++++++++++++++++---------------
 1 file changed, 108 insertions(+), 68 deletions(-)

diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
index a2c9d0ed..f1530391 100644
--- a/tests/i915/i915_pm_rpm.c
+++ b/tests/i915/i915_pm_rpm.c
@@ -571,33 +571,56 @@ static void assert_drm_infos_equal(struct compare_data *d1,
 		assert_drm_crtcs_equal(d1->crtcs[i], d2->crtcs[i]);
 }
 
-/* We could check the checksum too, but just the header is probably enough. */
-static bool edid_is_valid(const unsigned char *edid)
+static bool find_i2c_path(char *connector_name, char *i2c_path)
 {
-	char edid_header[] = {
-		0x0, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x0,
-	};
+	struct dirent *dirent;
+	DIR *dir;
+	int sysfs_card_fd = igt_sysfs_open(drm_fd);
+	int connector_fd = -1;
+	bool found_i2c_file = false;
 
-	return (memcmp(edid, edid_header, sizeof(edid_header)) == 0);
-}
+	dir = fdopendir(sysfs_card_fd);
+	igt_assert(dir);
 
-static int count_drm_valid_edids(struct mode_set_data *data)
-{
-	int ret = 0;
+	while ((dirent = readdir(dir))) {
+		/* Skip "cardx-" prefix */
+		char *dirname = dirent->d_name;
+		while(dirname[0] != '\0' && dirname[0] != '-')
+			++dirname;
 
-	if (!data->res)
-		return 0;
+		if(*dirname == '-')
+			++dirname;
 
-	for (int i = 0; i < data->res->count_connectors; i++)
-		if (data->edids[i] && edid_is_valid(data->edids[i]->data))
-			ret++;
-	return ret;
+		if (strcmp(dirname, connector_name) == 0) {
+			connector_fd = openat(sysfs_card_fd, dirent->d_name, O_RDONLY);
+			break;
+		}
+	}
+	closedir(dir);
+
+	if(connector_fd < 0)
+		return false;
+
+	dir = fdopendir(connector_fd);
+	igt_assert(dir);
+
+	while ((dirent = readdir(dir))) {
+		if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
+			sprintf(i2c_path, "/dev/%s", dirent->d_name);
+			found_i2c_file = true;
+		}
+	}
+	closedir(dir);
+	return found_i2c_file;
 }
 
-static bool i2c_edid_is_valid(int fd)
+
+static bool i2c_read_edid(char *connector_name, unsigned char *edid)
 {
-	int rc;
-	unsigned char edid[128] = {};
+	char i2c_path[PATH_MAX];
+	bool result = find_i2c_path(connector_name, i2c_path);
+	int rc, fd;
+
 	struct i2c_msg msgs[] = {
 		{ /* Start at 0. */
 			.addr = 0x50,
@@ -616,69 +639,86 @@ static bool i2c_edid_is_valid(int fd)
 		.nmsgs = 2,
 	};
 
+
+	if (!result)
+		return false;
+
+	fd = open(i2c_path, O_RDWR);
+	igt_assert_neq(fd, -1);
+
 	rc = ioctl(fd, I2C_RDWR, &msgset);
-	return (rc >= 0) ? edid_is_valid(edid) : false;
+
+	close(fd);
+	return rc >= 0;
 }
 
-static int count_i2c_valid_edids(void)
+static void format_hex_string(unsigned char edid[static EDID_LENGTH],
+			      char buf[static EDID_LENGTH * 5 + 1])
 {
-	int fd, ret = 0;
-	DIR *dir;
+	for (size_t i = 0; i < EDID_LENGTH; ++i)
+		sprintf(buf+i*5, "0x%02x ", edid[i]);
+}
 
-	struct dirent *dirent;
-	char full_name[PATH_MAX];
+static void test_i2c(struct mode_set_data *data)
+{
+	bool failed = false;
+	igt_display_t display;
+	igt_display_require(&display, drm_fd);
 
-	dir = opendir("/dev/");
-	igt_assert(dir);
+	for (int i = 0; i < data->res->count_connectors; i++) {
+		unsigned char *drm_edid = data->edids[i] ? data->edids[i]->data : NULL;
+		unsigned char i2c_edid[EDID_LENGTH] = {};
 
-	while ((dirent = readdir(dir))) {
-		if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
-			sprintf(full_name, "/dev/%s", dirent->d_name);
-			fd = open(full_name, O_RDWR);
-			igt_assert_neq(fd, -1);
-			if (i2c_edid_is_valid(fd))
-				ret++;
-			close(fd);
-		}
-	}
+		igt_output_t *output = igt_output_from_connector(&display,
+								 data->connectors[i]);
+		char *connector_name = (char *) igt_output_name(output);
 
-	closedir(dir);
+		bool got_i2c_edid = i2c_read_edid(connector_name, i2c_edid);
+		bool got_drm_edid = drm_edid != NULL;
+		bool is_vga = data->connectors[i]->connector_type == DRM_MODE_CONNECTOR_VGA;
 
-	return ret;
-}
+		bool edids_equal;
 
-static int count_vga_outputs(struct mode_set_data *data)
-{
-	int count = 0;
+		/* We fail to detect some VGA monitors using our i2c method. If you look
+		 * at the dmesg of these cases, you'll see the Kernel complaining about
+		 * the EDID reading mostly FFs and then disabling bit-banging. Since we
+		 * don't want to reimplement everything the Kernel does, let's just
+		 * accept the fact that some VGA outputs won't be properly detected. */
+		if(is_vga)
+			continue;
 
-	if (!data->res)
-		return 0;
+		if(!got_i2c_edid && !got_drm_edid)
+			continue;
+
+		if(got_i2c_edid && got_drm_edid)
+			edids_equal = (0 == memcmp(drm_edid, i2c_edid, EDID_LENGTH));
+		else
+			edids_equal = false;
 
-	for (int i = 0; i < data->res->count_connectors; i++)
-		if (data->connectors[i]->connector_type ==
-		    DRM_MODE_CONNECTOR_VGA)
-			count++;
 
-	return count;
-}
+		if (!edids_equal) {
+			char buf[5 * EDID_LENGTH + 1];
+			igt_critical("For connector %s EDID mismatch!\n",
+				     connector_name);
 
-static void test_i2c(struct mode_set_data *data)
-{
-	int i2c_edids = count_i2c_valid_edids();
-	int drm_edids = count_drm_valid_edids(data);
-	int vga_outputs = count_vga_outputs(data);
-	int diff;
-
-	igt_debug("i2c edids:%d drm edids:%d vga outputs:%d\n",
-		  i2c_edids, drm_edids, vga_outputs);
-
-	/* We fail to detect some VGA monitors using our i2c method. If you look
-	 * at the dmesg of these cases, you'll see the Kernel complaining about
-	 * the EDID reading mostly FFs and then disabling bit-banging. Since we
-	 * don't want to reimplement everything the Kernel does, let's just
-	 * accept the fact that some VGA outputs won't be properly detected. */
-	diff = drm_edids - i2c_edids;
-	igt_assert(diff <= vga_outputs && diff >= 0);
+			if(got_i2c_edid)
+				format_hex_string(i2c_edid, buf);
+			else
+				sprintf(buf, "NULL");
+
+			igt_critical("i2c: %s\n", buf);
+
+			if(got_drm_edid)
+				format_hex_string(drm_edid, buf);
+			else
+				sprintf(buf, "NULL");
+
+			igt_critical("drm: %s\n", buf);
+
+			failed = true;
+		}
+	}
+	igt_fail_on_f(failed, "Mismatch on EDID we got from i2c and DRM!\n");
 }
 
 static void setup_pc8(void)
-- 
2.21.0

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

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