[PATCH i-g-t] tests/kms: increase max threshold time for edid read

[PATCH i-g-t] tests/kms: increase max threshold time for edid read

[clinton.a.taylor]

From: Clint Taylor <clinton.a.taylor@intel.com>

Current 50ms max threshold timing for an EDID read is very close to the
actual time for a 2 block HDMI EDID read of 48ms. Any delay like a clock
stretch by the EDID eeprom will cause this test to fail. A 4 block HDMI
EDID read takes approximately 88ms under nominal conditions, making the max
threshold 95ms will allow this test to pass regardless of monitor attached.

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 tests/kms_sysfs_edid_timing.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
index 1201388..b45e080 100644
--- a/tests/kms_sysfs_edid_timing.c
+++ b/tests/kms_sysfs_edid_timing.c
@@ -27,14 +27,14 @@
 #include <sys/stat.h>
 
 #define THRESHOLD_PER_CONNECTOR	10
-#define THRESHOLD_TOTAL		50
+#define THRESHOLD_TOTAL		95
 #define CHECK_TIMES		15
 
 IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
 		     "the possible connectors. Without the edid -ENXIO patch "
 		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
-		     "we sometimes take a *really* long time. "
-		     "So let's just check for some reasonable timing here");
+		     "we sometimes take a *really* long time. So let's just "
+		     "check an approximate HDMI 4 block edid read timing here");
 
 
 igt_simple_main
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/kms: increase max threshold time for edid read

[Daniel Vetter]

On Fri, Aug 04, 2017 at 11:23:18AM -0700, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Current 50ms max threshold timing for an EDID read is very close to the
> actual time for a 2 block HDMI EDID read of 48ms. Any delay like a clock
> stretch by the EDID eeprom will cause this test to fail. A 4 block HDMI
> EDID read takes approximately 88ms under nominal conditions, making the max
> threshold 95ms will allow this test to pass regardless of monitor attached.
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>

Per internal mail, this needs to be runtime adjusted to fit the EDID we're
reading. Maybe 30ms per edid block.

Thanks, Daniel

> ---
>  tests/kms_sysfs_edid_timing.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> index 1201388..b45e080 100644
> --- a/tests/kms_sysfs_edid_timing.c
> +++ b/tests/kms_sysfs_edid_timing.c
> @@ -27,14 +27,14 @@
>  #include <sys/stat.h>
>  
>  #define THRESHOLD_PER_CONNECTOR	10
> -#define THRESHOLD_TOTAL		50
> +#define THRESHOLD_TOTAL		95
>  #define CHECK_TIMES		15
>  
>  IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
>  		     "the possible connectors. Without the edid -ENXIO patch "
>  		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> -		     "we sometimes take a *really* long time. "
> -		     "So let's just check for some reasonable timing here");
> +		     "we sometimes take a *really* long time. So let's just "
> +		     "check an approximate HDMI 4 block edid read timing here");
>  
>  
>  igt_simple_main
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/kms: increase max threshold time for edid read

[Lofstedt, Marta]

Thanks Clinton!

If you add:
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047

You have my Rb.
/Marta

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of clinton.a.taylor@intel.com
> Sent: Friday, August 4, 2017 9:23 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH i-g-t] tests/kms: increase max threshold time for
> edid read
> 
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Current 50ms max threshold timing for an EDID read is very close to the
> actual time for a 2 block HDMI EDID read of 48ms. Any delay like a clock
> stretch by the EDID eeprom will cause this test to fail. A 4 block HDMI EDID
> read takes approximately 88ms under nominal conditions, making the max
> threshold 95ms will allow this test to pass regardless of monitor attached.
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  tests/kms_sysfs_edid_timing.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> index 1201388..b45e080 100644
> --- a/tests/kms_sysfs_edid_timing.c
> +++ b/tests/kms_sysfs_edid_timing.c
> @@ -27,14 +27,14 @@
>  #include <sys/stat.h>
> 
>  #define THRESHOLD_PER_CONNECTOR	10
> -#define THRESHOLD_TOTAL		50
> +#define THRESHOLD_TOTAL		95
>  #define CHECK_TIMES		15
> 
>  IGT_TEST_DESCRIPTION("This check the time we take to read the content of
> all "
>  		     "the possible connectors. Without the edid -
> ENXIO patch "
> 
> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> -		     "we sometimes take a *really* long time. "
> -		     "So let's just check for some reasonable
> timing here");
> +		     "we sometimes take a *really* long time. So
> let's just "
> +		     "check an approximate HDMI 4 block edid
> read timing here");
> 
> 
>  igt_simple_main
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/kms: increase max threshold time for edid read

[Clint Taylor]

On 08/08/2017 12:51 AM, Lofstedt, Marta wrote:
> Thanks Clinton!
>
> If you add:
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>
> You have my Rb.
> /Marta

Thanks Marta,
     However I'm going to extend the functionality to read the EDID 
block size like Daniel suggested. During testing today of the new edid 
size read SKL passed and IVB failed. This may be a result of an actual 
HDMI port vs LSPCON. More testing is required before I have a actual 
working test.

-Clint
>
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of clinton.a.taylor@intel.com
>> Sent: Friday, August 4, 2017 9:23 PM
>> To: intel-gfx@lists.freedesktop.org
>> Subject: [Intel-gfx] [PATCH i-g-t] tests/kms: increase max threshold time for
>> edid read
>>
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> Current 50ms max threshold timing for an EDID read is very close to the
>> actual time for a 2 block HDMI EDID read of 48ms. Any delay like a clock
>> stretch by the EDID eeprom will cause this test to fail. A 4 block HDMI EDID
>> read takes approximately 88ms under nominal conditions, making the max
>> threshold 95ms will allow this test to pass regardless of monitor attached.
>>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> ---
>>   tests/kms_sysfs_edid_timing.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
>> index 1201388..b45e080 100644
>> --- a/tests/kms_sysfs_edid_timing.c
>> +++ b/tests/kms_sysfs_edid_timing.c
>> @@ -27,14 +27,14 @@
>>   #include <sys/stat.h>
>>
>>   #define THRESHOLD_PER_CONNECTOR	10
>> -#define THRESHOLD_TOTAL		50
>> +#define THRESHOLD_TOTAL		95
>>   #define CHECK_TIMES		15
>>
>>   IGT_TEST_DESCRIPTION("This check the time we take to read the content of
>> all "
>>   		     "the possible connectors. Without the edid -
>> ENXIO patch "
>>
>> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083),"
>> -		     "we sometimes take a *really* long time. "
>> -		     "So let's just check for some reasonable
>> timing here");
>> +		     "we sometimes take a *really* long time. So
>> let's just "
>> +		     "check an approximate HDMI 4 block edid
>> read timing here");
>>
>>
>>   igt_simple_main
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 i-g-t] tests/kms: increase max threshold time for edid read

[clinton.a.taylor]

From: Clint Taylor <clinton.a.taylor@intel.com>

Current 50ms max threshold timing for an EDID read is very close to the
actual time for a 2 block HDMI EDID read. Adjust the timings base on
connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
is connected to device under test the -l option should be passed to update
the threshold timing to allow the LSPcon to read the EDID at the HDMI
timing. The -l option should be used when LSPcon is on the motherboard or
if a USB_C->HDMI converter is present

V2: Adjust timings based on connector type, EDID size, and Add an option to
specify if an LSPcon is present.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 tests/kms_sysfs_edid_timing.c | 76 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 14 deletions(-)

diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
index 1201388..441dfee 100644
--- a/tests/kms_sysfs_edid_timing.c
+++ b/tests/kms_sysfs_edid_timing.c
@@ -26,21 +26,46 @@
 #include <fcntl.h>
 #include <sys/stat.h>
 
-#define THRESHOLD_PER_CONNECTOR	10
-#define THRESHOLD_TOTAL		50
-#define CHECK_TIMES		15
+#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
+#define THRESHOLD_PER_EDID_BLOCK		5
+#define HDMI_THRESHOLD_MULTIPLIER		10
+#define CHECK_TIMES				10
 
 IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
 		     "the possible connectors. Without the edid -ENXIO patch "
 		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
-		     "we sometimes take a *really* long time. "
-		     "So let's just check for some reasonable timing here");
+		     "we sometimes take a *really* long time. So let's check "
+		     "an approximate time per edid block based on connector "
+		     "type. The -l option adjusts DP timing to reflect HDMI read "
+		     "timings from LSPcon.");
+
+/* The -l option has been added to correctly handle timings when an LSPcon is
+ * present. This could be on the platform itself or in a USB_C->HDMI converter.
+ * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
+ * bus speed to the 100 Kbit HDMI DDC bus speed
+ */
+bool lspcon_present;
 
+static int opt_handler(int opt, int opt_index, void *data)
+{
+	if (opt == 'l') {
+		lspcon_present = true;
+		igt_info("set LSPcon present on DP ports\n");
+	}
 
-igt_simple_main
+	return 0;
+}
+
+int main(int argc, char **argv)
 {
 	DIR *dirp;
 	struct dirent *de;
+	lspcon_present = false;
+
+	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
+				      opt_handler, NULL);
+
+	igt_skip_on_simulation();
 
 	dirp = opendir("/sys/class/drm");
 	igt_assert(dirp != NULL);
@@ -49,17 +74,36 @@ igt_simple_main
 		struct igt_mean mean = {};
 		struct stat st;
 		char path[PATH_MAX];
-		int i;
+		char edid_path[PATH_MAX];
+		char edid[512]; /* enough for 4 block edid */
+		unsigned long edid_size = 0;
+		int i, fd_edid;
+		unsigned int threshold = 0;
 
 		if (*de->d_name == '.')
 			continue;;
 
 		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
 				de->d_name);
+		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
+				de->d_name);
 
 		if (stat(path, &st))
 			continue;
 
+		fd_edid = open(edid_path, O_RDONLY);
+		if (fd_edid == -1) {
+			igt_warn("Read Error EDID\n");
+			continue;
+		}
+
+		edid_size = read(fd_edid, edid, 512);
+		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
+		if (lspcon_present || (edid_size > 128 &&
+		    !strncmp(de->d_name, "card0-HDMI", 10))) {
+			threshold *= HDMI_THRESHOLD_MULTIPLIER;
+		}
+
 		igt_mean_init(&mean);
 		for (i = 0; i < CHECK_TIMES; i++) {
 			struct timespec ts = {};
@@ -76,22 +120,26 @@ igt_simple_main
 		}
 
 		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
-			  "mean.avg %.2fns, %.2fus, %.2fms\n",
+			  "mean.avg %.2fns, %.2fus, %.2fms, "
+			  "edid_size %lu, threshold %d\n",
 			  de->d_name,
 			  mean.max, mean.max / 1e3, mean.max / 1e6,
-			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
+			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
+			  edid_size, threshold);
 
-		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
+		if (edid_size == 0 &&
+		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
 			igt_warn("%s: probe time exceed 10ms, "
 				 "max=%.2fms, avg=%.2fms\n", de->d_name,
 				 mean.max / 1e6, mean.mean / 1e6);
 		}
-		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
-			     "%s: average probe time exceeded 50ms, "
-			     "max=%.2fms, avg=%.2fms\n", de->d_name,
+		if (edid_size > 0)
+			igt_assert_f(mean.mean < (threshold * 1e6),
+			     "%s: average probe time exceeded %dms, "
+			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
 			     mean.max / 1e6, mean.mean / 1e6);
 
 	}
 	closedir(dirp);
-
+	igt_exit();
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for tests/kms: increase max threshold time for edid read (rev2)

[Patchwork]

== Series Details ==

Series: tests/kms: increase max threshold time for edid read (rev2)
URL   : https://patchwork.freedesktop.org/series/28399/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
c129026622accef6f54c0cfb0dc55e930cfa60b5 igt: add syncobj_basic.

with latest DRM-Tip kernel build CI_DRM_2942
2d0288b5b28c drm-tip: 2017y-08m-09d-18h-09m-54s UTC integration manifest

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_ringfill:
        Subgroup basic-default-hang:
                dmesg-warn -> INCOMPLETE (fi-pnv-d510) fdo#101600
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:437s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:418s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:362s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:492s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:496s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:522s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:512s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:589s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:434s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:406s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:428s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:514s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:460s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:563s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:582s
fi-pnv-d510      total:147  pass:113  dwarn:0   dfail:0   fail:0   skip:33 
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:454s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:649s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:458s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:425s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:468s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:551s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:404s

== Logs ==

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

[PATCH v3 i-g-t] tests/kms: increase max threshold time for edid read

[clinton.a.taylor]

From: Clint Taylor <clinton.a.taylor@intel.com>

Current 50ms max threshold timing for an EDID read is very close to the
actual time for a 2 block HDMI EDID read. Adjust the timings base on
connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
is connected to device under test the -l option should be passed to update
the threshold timing to allow the LSPcon to read the EDID at the HDMI
timing. The -l option should be used when LSPcon is on the motherboard or
if a USB_C->HDMI converter is present

V2: Adjust timings based on connector type, EDID size, and Add an option to
specify if an LSPcon is present.
V3: added bugzilla to commit message

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 tests/kms_sysfs_edid_timing.c | 76 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 14 deletions(-)

diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
index 1201388..441dfee 100644
--- a/tests/kms_sysfs_edid_timing.c
+++ b/tests/kms_sysfs_edid_timing.c
@@ -26,21 +26,46 @@
 #include <fcntl.h>
 #include <sys/stat.h>
 
-#define THRESHOLD_PER_CONNECTOR	10
-#define THRESHOLD_TOTAL		50
-#define CHECK_TIMES		15
+#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
+#define THRESHOLD_PER_EDID_BLOCK		5
+#define HDMI_THRESHOLD_MULTIPLIER		10
+#define CHECK_TIMES				10
 
 IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
 		     "the possible connectors. Without the edid -ENXIO patch "
 		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
-		     "we sometimes take a *really* long time. "
-		     "So let's just check for some reasonable timing here");
+		     "we sometimes take a *really* long time. So let's check "
+		     "an approximate time per edid block based on connector "
+		     "type. The -l option adjusts DP timing to reflect HDMI read "
+		     "timings from LSPcon.");
+
+/* The -l option has been added to correctly handle timings when an LSPcon is
+ * present. This could be on the platform itself or in a USB_C->HDMI converter.
+ * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
+ * bus speed to the 100 Kbit HDMI DDC bus speed
+ */
+bool lspcon_present;
 
+static int opt_handler(int opt, int opt_index, void *data)
+{
+	if (opt == 'l') {
+		lspcon_present = true;
+		igt_info("set LSPcon present on DP ports\n");
+	}
 
-igt_simple_main
+	return 0;
+}
+
+int main(int argc, char **argv)
 {
 	DIR *dirp;
 	struct dirent *de;
+	lspcon_present = false;
+
+	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
+				      opt_handler, NULL);
+
+	igt_skip_on_simulation();
 
 	dirp = opendir("/sys/class/drm");
 	igt_assert(dirp != NULL);
@@ -49,17 +74,36 @@ igt_simple_main
 		struct igt_mean mean = {};
 		struct stat st;
 		char path[PATH_MAX];
-		int i;
+		char edid_path[PATH_MAX];
+		char edid[512]; /* enough for 4 block edid */
+		unsigned long edid_size = 0;
+		int i, fd_edid;
+		unsigned int threshold = 0;
 
 		if (*de->d_name == '.')
 			continue;;
 
 		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
 				de->d_name);
+		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
+				de->d_name);
 
 		if (stat(path, &st))
 			continue;
 
+		fd_edid = open(edid_path, O_RDONLY);
+		if (fd_edid == -1) {
+			igt_warn("Read Error EDID\n");
+			continue;
+		}
+
+		edid_size = read(fd_edid, edid, 512);
+		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
+		if (lspcon_present || (edid_size > 128 &&
+		    !strncmp(de->d_name, "card0-HDMI", 10))) {
+			threshold *= HDMI_THRESHOLD_MULTIPLIER;
+		}
+
 		igt_mean_init(&mean);
 		for (i = 0; i < CHECK_TIMES; i++) {
 			struct timespec ts = {};
@@ -76,22 +120,26 @@ igt_simple_main
 		}
 
 		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
-			  "mean.avg %.2fns, %.2fus, %.2fms\n",
+			  "mean.avg %.2fns, %.2fus, %.2fms, "
+			  "edid_size %lu, threshold %d\n",
 			  de->d_name,
 			  mean.max, mean.max / 1e3, mean.max / 1e6,
-			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
+			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
+			  edid_size, threshold);
 
-		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
+		if (edid_size == 0 &&
+		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
 			igt_warn("%s: probe time exceed 10ms, "
 				 "max=%.2fms, avg=%.2fms\n", de->d_name,
 				 mean.max / 1e6, mean.mean / 1e6);
 		}
-		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
-			     "%s: average probe time exceeded 50ms, "
-			     "max=%.2fms, avg=%.2fms\n", de->d_name,
+		if (edid_size > 0)
+			igt_assert_f(mean.mean < (threshold * 1e6),
+			     "%s: average probe time exceeded %dms, "
+			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
 			     mean.max / 1e6, mean.mean / 1e6);
 
 	}
 	closedir(dirp);
-
+	igt_exit();
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for tests/kms: increase max threshold time for edid read (rev3)

[Patchwork]

== Series Details ==

Series: tests/kms: increase max threshold time for edid read (rev3)
URL   : https://patchwork.freedesktop.org/series/28399/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
c129026622accef6f54c0cfb0dc55e930cfa60b5 igt: add syncobj_basic.

with latest DRM-Tip kernel build CI_DRM_2942
2d0288b5b28c drm-tip: 2017y-08m-09d-18h-09m-54s UTC integration manifest

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:445s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:418s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:358s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:499s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:482s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:523s
fi-byt-n2820     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:516s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:585s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:433s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:418s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:506s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:461s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:567s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:577s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:523s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:447s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:647s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:463s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:424s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:463s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:544s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:411s

== Logs ==

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

Re: [PATCH i-g-t] tests/kms: increase max threshold time for edid read

[Chris Wilson]

Quoting Daniel Vetter (2017-08-07 17:20:27)
> On Fri, Aug 04, 2017 at 11:23:18AM -0700, clinton.a.taylor@intel.com wrote:
> > From: Clint Taylor <clinton.a.taylor@intel.com>
> > 
> > Current 50ms max threshold timing for an EDID read is very close to the
> > actual time for a 2 block HDMI EDID read of 48ms. Any delay like a clock
> > stretch by the EDID eeprom will cause this test to fail. A 4 block HDMI
> > EDID read takes approximately 88ms under nominal conditions, making the max
> > threshold 95ms will allow this test to pass regardless of monitor attached.
> > 
> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Per internal mail, this needs to be runtime adjusted to fit the EDID we're
> reading. Maybe 30ms per edid block.

Those are scary numbers. So please also a kms_flip flip-vs-edid
with timestamp check. Given the number of random probes we do, we must
make sure that we can do those in parallel to driving the display
fluidly. Similarly, some crc testing against edid reading would be in
order to make sure that there are no display glitches during the
transfer.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 i-g-t] tests/kms: increase max threshold time for edid read

[Lofstedt, Marta]

Sorry Clinton I fail the test again on my BDW NUCi5 with this V3.

Check: https://bugs.freedesktop.org/show_bug.cgi?id=100047
For details.

/Marta 

> -----Original Message-----
> From: Taylor, Clinton A
> Sent: Thursday, August 10, 2017 1:52 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Taylor, Clinton A <clinton.a.taylor@intel.com>; Vetter, Daniel
> <daniel.vetter@intel.com>; Lofstedt, Marta <marta.lofstedt@intel.com>
> Subject: [PATCH v3 i-g-t] tests/kms: increase max threshold time for edid
> read
> 
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Current 50ms max threshold timing for an EDID read is very close to the
> actual time for a 2 block HDMI EDID read. Adjust the timings base on
> connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon is
> connected to device under test the -l option should be passed to update the
> threshold timing to allow the LSPcon to read the EDID at the HDMI timing.
> The -l option should be used when LSPcon is on the motherboard or if a
> USB_C->HDMI converter is present
> 
> V2: Adjust timings based on connector type, EDID size, and Add an option to
> specify if an LSPcon is present.
> V3: added bugzilla to commit message
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  tests/kms_sysfs_edid_timing.c | 76
> +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 62 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> index 1201388..441dfee 100644
> --- a/tests/kms_sysfs_edid_timing.c
> +++ b/tests/kms_sysfs_edid_timing.c
> @@ -26,21 +26,46 @@
>  #include <fcntl.h>
>  #include <sys/stat.h>
> 
> -#define THRESHOLD_PER_CONNECTOR	10
> -#define THRESHOLD_TOTAL		50
> -#define CHECK_TIMES		15
> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> +#define THRESHOLD_PER_EDID_BLOCK		5
> +#define HDMI_THRESHOLD_MULTIPLIER		10
> +#define CHECK_TIMES				10
> 
>  IGT_TEST_DESCRIPTION("This check the time we take to read the content of
> all "
>  		     "the possible connectors. Without the edid -
> ENXIO patch "
> 
> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> -		     "we sometimes take a *really* long time. "
> -		     "So let's just check for some reasonable
> timing here");
> +		     "we sometimes take a *really* long time. So
> let's check "
> +		     "an approximate time per edid block based on
> connector "
> +		     "type. The -l option adjusts DP timing to
> reflect HDMI read "
> +		     "timings from LSPcon.");
> +
> +/* The -l option has been added to correctly handle timings when an
> +LSPcon is
> + * present. This could be on the platform itself or in a USB_C->HDMI
> converter.
> + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
> + * bus speed to the 100 Kbit HDMI DDC bus speed  */ bool
> +lspcon_present;
> 
> +static int opt_handler(int opt, int opt_index, void *data) {
> +	if (opt == 'l') {
> +		lspcon_present = true;
> +		igt_info("set LSPcon present on DP ports\n");
> +	}
> 
> -igt_simple_main
> +	return 0;
> +}
> +
> +int main(int argc, char **argv)
>  {
>  	DIR *dirp;
>  	struct dirent *de;
> +	lspcon_present = false;
> +
> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> +				      opt_handler,
> NULL);
> +
> +	igt_skip_on_simulation();
> 
>  	dirp = opendir("/sys/class/drm");
>  	igt_assert(dirp != NULL);
> @@ -49,17 +74,36 @@ igt_simple_main
>  		struct igt_mean mean = {};
>  		struct stat st;
>  		char path[PATH_MAX];
> -		int i;
> +		char edid_path[PATH_MAX];
> +		char edid[512]; /* enough for 4 block edid */
> +		unsigned long edid_size = 0;
> +		int i, fd_edid;
> +		unsigned int threshold = 0;
> 
>  		if (*de->d_name == '.')
>  			continue;;
> 
>  		snprintf(path, sizeof(path),
> "/sys/class/drm/%s/status",
>  				de->d_name);
> +		snprintf(edid_path, sizeof(edid_path),
> "/sys/class/drm/%s/edid",
> +				de->d_name);
> 
>  		if (stat(path, &st))
>  			continue;
> 
> +		fd_edid = open(edid_path, O_RDONLY);
> +		if (fd_edid == -1) {
> +			igt_warn("Read Error EDID\n");
> +			continue;
> +		}
> +
> +		edid_size = read(fd_edid, edid, 512);
> +		threshold = THRESHOLD_PER_EDID_BLOCK *
> (edid_size / 128);
> +		if (lspcon_present || (edid_size > 128 &&
> +		    !strncmp(de->d_name, "card0-HDMI", 10))) {
> +			threshold *=
> HDMI_THRESHOLD_MULTIPLIER;
> +		}
> +
>  		igt_mean_init(&mean);
>  		for (i = 0; i < CHECK_TIMES; i++) {
>  			struct timespec ts = {};
> @@ -76,22 +120,26 @@ igt_simple_main
>  		}
> 
>  		igt_debug("%s: mean.max %.2fns, %.2fus,
> %.2fms, "
> -			  "mean.avg %.2fns, %.2fus,
> %.2fms\n",
> +			  "mean.avg %.2fns, %.2fus,
> %.2fms, "
> +			  "edid_size %lu, threshold %d\n",
>  			  de->d_name,
>  			  mean.max, mean.max / 1e3,
> mean.max / 1e6,
> -			  mean.mean, mean.mean / 1e3,
> mean.mean / 1e6);
> +			  mean.mean, mean.mean / 1e3,
> mean.mean / 1e6,
> +			  edid_size, threshold);
> 
> -		if (mean.max > (THRESHOLD_PER_CONNECTOR
> * 1e6)) {
> +		if (edid_size == 0 &&
> +		   (mean.max >
> (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
>  			igt_warn("%s: probe time exceed
> 10ms, "
>  				 "max=%.2fms,
> avg=%.2fms\n", de->d_name,
>  				 mean.max / 1e6,
> mean.mean / 1e6);
>  		}
> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL
> * 1e6),
> -			     "%s: average probe time
> exceeded 50ms, "
> -			     "max=%.2fms, avg=%.2fms\n",
> de->d_name,
> +		if (edid_size > 0)
> +			igt_assert_f(mean.mean <
> (threshold * 1e6),
> +			     "%s: average probe time
> exceeded %dms, "
> +			     "max=%.2fms, avg=%.2fms\n",
> de->d_name, threshold,
>  			     mean.max / 1e6, mean.mean /
> 1e6);
> 
>  	}
>  	closedir(dirp);
> -
> +	igt_exit();
>  }
> --
> 1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 i-g-t] tests/kms: increase max threshold time for edid read

[Lofstedt, Marta]

With this tweak the test pass:
--- a/tests/kms_sysfs_edid_timing.c
+++ b/tests/kms_sysfs_edid_timing.c
@@ -99,7 +99,7 @@ int main(int argc, char **argv)
 
                edid_size = read(fd_edid, edid, 512);
                threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
-               if (lspcon_present || (edid_size > 128 &&
+               if (lspcon_present || (edid_size >= 128 &&
                    !strncmp(de->d_name, "card0-HDMI", 10))) {
                        threshold *= HDMI_THRESHOLD_MULTIPLIER;
                }

> -----Original Message-----
> From: Lofstedt, Marta
> Sent: Thursday, August 10, 2017 9:41 AM
> To: Taylor, Clinton A <clinton.a.taylor@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Vetter, Daniel <daniel.vetter@intel.com>
> Subject: RE: [PATCH v3 i-g-t] tests/kms: increase max threshold time for edid
> read
> 
> Sorry Clinton I fail the test again on my BDW NUCi5 with this V3.
> 
> Check: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> For details.
> 
> /Marta
> 
> > -----Original Message-----
> > From: Taylor, Clinton A
> > Sent: Thursday, August 10, 2017 1:52 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Taylor, Clinton A <clinton.a.taylor@intel.com>; Vetter, Daniel
> > <daniel.vetter@intel.com>; Lofstedt, Marta <marta.lofstedt@intel.com>
> > Subject: [PATCH v3 i-g-t] tests/kms: increase max threshold time for
> > edid read
> >
> > From: Clint Taylor <clinton.a.taylor@intel.com>
> >
> > Current 50ms max threshold timing for an EDID read is very close to
> > the actual time for a 2 block HDMI EDID read. Adjust the timings base
> > on connector type as DP reads are at 1 MBit and HDMI at 100K bit. If
> > an LSPcon is connected to device under test the -l option should be
> > passed to update the threshold timing to allow the LSPcon to read the EDID
> at the HDMI timing.
> > The -l option should be used when LSPcon is on the motherboard or if a
> > USB_C->HDMI converter is present
> >
> > V2: Adjust timings based on connector type, EDID size, and Add an
> > option to specify if an LSPcon is present.
> > V3: added bugzilla to commit message
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> >
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > ---
> >  tests/kms_sysfs_edid_timing.c | 76
> > +++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 62 insertions(+), 14 deletions(-)
> >
> > diff --git a/tests/kms_sysfs_edid_timing.c
> > b/tests/kms_sysfs_edid_timing.c index 1201388..441dfee 100644
> > --- a/tests/kms_sysfs_edid_timing.c
> > +++ b/tests/kms_sysfs_edid_timing.c
> > @@ -26,21 +26,46 @@
> >  #include <fcntl.h>
> >  #include <sys/stat.h>
> >
> > -#define THRESHOLD_PER_CONNECTOR	10
> > -#define THRESHOLD_TOTAL		50
> > -#define CHECK_TIMES		15
> > +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> > +#define THRESHOLD_PER_EDID_BLOCK		5
> > +#define HDMI_THRESHOLD_MULTIPLIER		10
> > +#define CHECK_TIMES				10
> >
> >  IGT_TEST_DESCRIPTION("This check the time we take to read the content
> > of all "
> >  		     "the possible connectors. Without the edid -
> ENXIO patch "
> >
> > "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> > -		     "we sometimes take a *really* long time. "
> > -		     "So let's just check for some reasonable
> > timing here");
> > +		     "we sometimes take a *really* long time. So
> > let's check "
> > +		     "an approximate time per edid block based on
> > connector "
> > +		     "type. The -l option adjusts DP timing to
> > reflect HDMI read "
> > +		     "timings from LSPcon.");
> > +
> > +/* The -l option has been added to correctly handle timings when an
> > +LSPcon is
> > + * present. This could be on the platform itself or in a USB_C->HDMI
> > converter.
> > + * With LSPCon EDID read timing will need to change from the 1 Mbit
> > +AUX
> > + * bus speed to the 100 Kbit HDMI DDC bus speed  */ bool
> > +lspcon_present;
> >
> > +static int opt_handler(int opt, int opt_index, void *data) {
> > +	if (opt == 'l') {
> > +		lspcon_present = true;
> > +		igt_info("set LSPcon present on DP ports\n");
> > +	}
> >
> > -igt_simple_main
> > +	return 0;
> > +}
> > +
> > +int main(int argc, char **argv)
> >  {
> >  	DIR *dirp;
> >  	struct dirent *de;
> > +	lspcon_present = false;
> > +
> > +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> > +				      opt_handler,
> > NULL);
> > +
> > +	igt_skip_on_simulation();
> >
> >  	dirp = opendir("/sys/class/drm");
> >  	igt_assert(dirp != NULL);
> > @@ -49,17 +74,36 @@ igt_simple_main
> >  		struct igt_mean mean = {};
> >  		struct stat st;
> >  		char path[PATH_MAX];
> > -		int i;
> > +		char edid_path[PATH_MAX];
> > +		char edid[512]; /* enough for 4 block edid */
> > +		unsigned long edid_size = 0;
> > +		int i, fd_edid;
> > +		unsigned int threshold = 0;
> >
> >  		if (*de->d_name == '.')
> >  			continue;;
> >
> >  		snprintf(path, sizeof(path),
> > "/sys/class/drm/%s/status",
> >  				de->d_name);
> > +		snprintf(edid_path, sizeof(edid_path),
> > "/sys/class/drm/%s/edid",
> > +				de->d_name);
> >
> >  		if (stat(path, &st))
> >  			continue;
> >
> > +		fd_edid = open(edid_path, O_RDONLY);
> > +		if (fd_edid == -1) {
> > +			igt_warn("Read Error EDID\n");
> > +			continue;
> > +		}
> > +
> > +		edid_size = read(fd_edid, edid, 512);
> > +		threshold = THRESHOLD_PER_EDID_BLOCK *
> > (edid_size / 128);
> > +		if (lspcon_present || (edid_size > 128 &&
> > +		    !strncmp(de->d_name, "card0-HDMI", 10))) {
> > +			threshold *=
> > HDMI_THRESHOLD_MULTIPLIER;
> > +		}
> > +
> >  		igt_mean_init(&mean);
> >  		for (i = 0; i < CHECK_TIMES; i++) {
> >  			struct timespec ts = {};
> > @@ -76,22 +120,26 @@ igt_simple_main
> >  		}
> >
> >  		igt_debug("%s: mean.max %.2fns, %.2fus,
> %.2fms, "
> > -			  "mean.avg %.2fns, %.2fus,
> > %.2fms\n",
> > +			  "mean.avg %.2fns, %.2fus,
> > %.2fms, "
> > +			  "edid_size %lu, threshold %d\n",
> >  			  de->d_name,
> >  			  mean.max, mean.max / 1e3,
> > mean.max / 1e6,
> > -			  mean.mean, mean.mean / 1e3,
> > mean.mean / 1e6);
> > +			  mean.mean, mean.mean / 1e3,
> > mean.mean / 1e6,
> > +			  edid_size, threshold);
> >
> > -		if (mean.max > (THRESHOLD_PER_CONNECTOR
> > * 1e6)) {
> > +		if (edid_size == 0 &&
> > +		   (mean.max >
> > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
> >  			igt_warn("%s: probe time exceed
> > 10ms, "
> >  				 "max=%.2fms,
> > avg=%.2fms\n", de->d_name,
> >  				 mean.max / 1e6,
> > mean.mean / 1e6);
> >  		}
> > -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL
> > * 1e6),
> > -			     "%s: average probe time
> > exceeded 50ms, "
> > -			     "max=%.2fms, avg=%.2fms\n",
> > de->d_name,
> > +		if (edid_size > 0)
> > +			igt_assert_f(mean.mean <
> > (threshold * 1e6),
> > +			     "%s: average probe time
> > exceeded %dms, "
> > +			     "max=%.2fms, avg=%.2fms\n",
> > de->d_name, threshold,
> >  			     mean.max / 1e6, mean.mean /
> > 1e6);
> >
> >  	}
> >  	closedir(dirp);
> > -
> > +	igt_exit();
> >  }
> > --
> > 1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 i-g-t] tests/kms: increase max threshold time for edid read

[Clint Taylor]

On 08/10/2017 12:00 AM, Lofstedt, Marta wrote:
> With this tweak the test pass:
> --- a/tests/kms_sysfs_edid_timing.c
> +++ b/tests/kms_sysfs_edid_timing.c
> @@ -99,7 +99,7 @@ int main(int argc, char **argv)
>   
>                  edid_size = read(fd_edid, edid, 512);
>                  threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
> -               if (lspcon_present || (edid_size > 128 &&
> +               if (lspcon_present || (edid_size >= 128 &&
Good catch. DVI monitors only have 128 byte EDIDs and connected to an 
HDMI port would not fall into the HDMI_THRESHOLD multiplier. There is no 
reason to check edid_size here.

I might need a special DVI multiplier as the 128 byte read is taking an 
average of 49.77ms when the threshold is being computed currently as 
50ms. As the testing progresses with this code we might need to make 
further tweaks to the timings. I will submit a v4 shortly.

-Clint


>                      !strncmp(de->d_name, "card0-HDMI", 10))) {
>                          threshold *= HDMI_THRESHOLD_MULTIPLIER;
>                  }
>
>> -----Original Message-----
>> From: Lofstedt, Marta
>> Sent: Thursday, August 10, 2017 9:41 AM
>> To: Taylor, Clinton A <clinton.a.taylor@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Cc: Vetter, Daniel <daniel.vetter@intel.com>
>> Subject: RE: [PATCH v3 i-g-t] tests/kms: increase max threshold time for edid
>> read
>>
>> Sorry Clinton I fail the test again on my BDW NUCi5 with this V3.
>>
>> Check: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>> For details.
>>
>> /Marta
>>
>>> -----Original Message-----
>>> From: Taylor, Clinton A
>>> Sent: Thursday, August 10, 2017 1:52 AM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: Taylor, Clinton A <clinton.a.taylor@intel.com>; Vetter, Daniel
>>> <daniel.vetter@intel.com>; Lofstedt, Marta <marta.lofstedt@intel.com>
>>> Subject: [PATCH v3 i-g-t] tests/kms: increase max threshold time for
>>> edid read
>>>
>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>>
>>> Current 50ms max threshold timing for an EDID read is very close to
>>> the actual time for a 2 block HDMI EDID read. Adjust the timings base
>>> on connector type as DP reads are at 1 MBit and HDMI at 100K bit. If
>>> an LSPcon is connected to device under test the -l option should be
>>> passed to update the threshold timing to allow the LSPcon to read the EDID
>> at the HDMI timing.
>>> The -l option should be used when LSPcon is on the motherboard or if a
>>> USB_C->HDMI converter is present
>>>
>>> V2: Adjust timings based on connector type, EDID size, and Add an
>>> option to specify if an LSPcon is present.
>>> V3: added bugzilla to commit message
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>>>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>> ---
>>>   tests/kms_sysfs_edid_timing.c | 76
>>> +++++++++++++++++++++++++++++++++++--------
>>>   1 file changed, 62 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/tests/kms_sysfs_edid_timing.c
>>> b/tests/kms_sysfs_edid_timing.c index 1201388..441dfee 100644
>>> --- a/tests/kms_sysfs_edid_timing.c
>>> +++ b/tests/kms_sysfs_edid_timing.c
>>> @@ -26,21 +26,46 @@
>>>   #include <fcntl.h>
>>>   #include <sys/stat.h>
>>>
>>> -#define THRESHOLD_PER_CONNECTOR	10
>>> -#define THRESHOLD_TOTAL		50
>>> -#define CHECK_TIMES		15
>>> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
>>> +#define THRESHOLD_PER_EDID_BLOCK		5
>>> +#define HDMI_THRESHOLD_MULTIPLIER		10
>>> +#define CHECK_TIMES				10
>>>
>>>   IGT_TEST_DESCRIPTION("This check the time we take to read the content
>>> of all "
>>>   		     "the possible connectors. Without the edid -
>> ENXIO patch "
>>> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083),"
>>> -		     "we sometimes take a *really* long time. "
>>> -		     "So let's just check for some reasonable
>>> timing here");
>>> +		     "we sometimes take a *really* long time. So
>>> let's check "
>>> +		     "an approximate time per edid block based on
>>> connector "
>>> +		     "type. The -l option adjusts DP timing to
>>> reflect HDMI read "
>>> +		     "timings from LSPcon.");
>>> +
>>> +/* The -l option has been added to correctly handle timings when an
>>> +LSPcon is
>>> + * present. This could be on the platform itself or in a USB_C->HDMI
>>> converter.
>>> + * With LSPCon EDID read timing will need to change from the 1 Mbit
>>> +AUX
>>> + * bus speed to the 100 Kbit HDMI DDC bus speed  */ bool
>>> +lspcon_present;
>>>
>>> +static int opt_handler(int opt, int opt_index, void *data) {
>>> +	if (opt == 'l') {
>>> +		lspcon_present = true;
>>> +		igt_info("set LSPcon present on DP ports\n");
>>> +	}
>>>
>>> -igt_simple_main
>>> +	return 0;
>>> +}
>>> +
>>> +int main(int argc, char **argv)
>>>   {
>>>   	DIR *dirp;
>>>   	struct dirent *de;
>>> +	lspcon_present = false;
>>> +
>>> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
>>> +				      opt_handler,
>>> NULL);
>>> +
>>> +	igt_skip_on_simulation();
>>>
>>>   	dirp = opendir("/sys/class/drm");
>>>   	igt_assert(dirp != NULL);
>>> @@ -49,17 +74,36 @@ igt_simple_main
>>>   		struct igt_mean mean = {};
>>>   		struct stat st;
>>>   		char path[PATH_MAX];
>>> -		int i;
>>> +		char edid_path[PATH_MAX];
>>> +		char edid[512]; /* enough for 4 block edid */
>>> +		unsigned long edid_size = 0;
>>> +		int i, fd_edid;
>>> +		unsigned int threshold = 0;
>>>
>>>   		if (*de->d_name == '.')
>>>   			continue;;
>>>
>>>   		snprintf(path, sizeof(path),
>>> "/sys/class/drm/%s/status",
>>>   				de->d_name);
>>> +		snprintf(edid_path, sizeof(edid_path),
>>> "/sys/class/drm/%s/edid",
>>> +				de->d_name);
>>>
>>>   		if (stat(path, &st))
>>>   			continue;
>>>
>>> +		fd_edid = open(edid_path, O_RDONLY);
>>> +		if (fd_edid == -1) {
>>> +			igt_warn("Read Error EDID\n");
>>> +			continue;
>>> +		}
>>> +
>>> +		edid_size = read(fd_edid, edid, 512);
>>> +		threshold = THRESHOLD_PER_EDID_BLOCK *
>>> (edid_size / 128);
>>> +		if (lspcon_present || (edid_size > 128 &&
>>> +		    !strncmp(de->d_name, "card0-HDMI", 10))) {
>>> +			threshold *=
>>> HDMI_THRESHOLD_MULTIPLIER;
>>> +		}
>>> +
>>>   		igt_mean_init(&mean);
>>>   		for (i = 0; i < CHECK_TIMES; i++) {
>>>   			struct timespec ts = {};
>>> @@ -76,22 +120,26 @@ igt_simple_main
>>>   		}
>>>
>>>   		igt_debug("%s: mean.max %.2fns, %.2fus,
>> %.2fms, "
>>> -			  "mean.avg %.2fns, %.2fus,
>>> %.2fms\n",
>>> +			  "mean.avg %.2fns, %.2fus,
>>> %.2fms, "
>>> +			  "edid_size %lu, threshold %d\n",
>>>   			  de->d_name,
>>>   			  mean.max, mean.max / 1e3,
>>> mean.max / 1e6,
>>> -			  mean.mean, mean.mean / 1e3,
>>> mean.mean / 1e6);
>>> +			  mean.mean, mean.mean / 1e3,
>>> mean.mean / 1e6,
>>> +			  edid_size, threshold);
>>>
>>> -		if (mean.max > (THRESHOLD_PER_CONNECTOR
>>> * 1e6)) {
>>> +		if (edid_size == 0 &&
>>> +		   (mean.max >
>>> (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
>>>   			igt_warn("%s: probe time exceed
>>> 10ms, "
>>>   				 "max=%.2fms,
>>> avg=%.2fms\n", de->d_name,
>>>   				 mean.max / 1e6,
>>> mean.mean / 1e6);
>>>   		}
>>> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL
>>> * 1e6),
>>> -			     "%s: average probe time
>>> exceeded 50ms, "
>>> -			     "max=%.2fms, avg=%.2fms\n",
>>> de->d_name,
>>> +		if (edid_size > 0)
>>> +			igt_assert_f(mean.mean <
>>> (threshold * 1e6),
>>> +			     "%s: average probe time
>>> exceeded %dms, "
>>> +			     "max=%.2fms, avg=%.2fms\n",
>>> de->d_name, threshold,
>>>   			     mean.max / 1e6, mean.mean /
>>> 1e6);
>>>
>>>   	}
>>>   	closedir(dirp);
>>> -
>>> +	igt_exit();
>>>   }
>>> --
>>> 1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[clinton.a.taylor]

From: Clint Taylor <clinton.a.taylor@intel.com>

Current 50ms max threshold timing for an EDID read is very close to the
actual time for a 2 block HDMI EDID read. Adjust the timings base on
connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
is connected to device under test the -l option should be passed to update
the threshold timing to allow the LSPcon to read the EDID at the HDMI
timing. The -l option should be used when LSPcon is on the motherboard or
if a USB_C->HDMI converter is present

V2: Adjust timings based on connector type, EDID size, and Add an option to
specify if an LSPcon is present.
V3: Add bugzilla to commit message
V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 tests/kms_sysfs_edid_timing.c | 74 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 14 deletions(-)

diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
index 1201388..0382610 100644
--- a/tests/kms_sysfs_edid_timing.c
+++ b/tests/kms_sysfs_edid_timing.c
@@ -26,21 +26,46 @@
 #include <fcntl.h>
 #include <sys/stat.h>
 
-#define THRESHOLD_PER_CONNECTOR	10
-#define THRESHOLD_TOTAL		50
-#define CHECK_TIMES		15
+#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
+#define THRESHOLD_PER_EDID_BLOCK		5
+#define HDMI_THRESHOLD_MULTIPLIER		10
+#define CHECK_TIMES				10
 
 IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
 		     "the possible connectors. Without the edid -ENXIO patch "
 		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
-		     "we sometimes take a *really* long time. "
-		     "So let's just check for some reasonable timing here");
+		     "we sometimes take a *really* long time. So let's check "
+		     "an approximate time per edid block based on connector "
+		     "type. The -l option adjusts DP timing to reflect HDMI read "
+		     "timings from LSPcon.");
+
+/* The -l option has been added to correctly handle timings when an LSPcon is
+ * present. This could be on the platform itself or in a USB_C->HDMI converter.
+ * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
+ * bus speed to the 100 Kbit HDMI DDC bus speed
+ */
+bool lspcon_present;
 
+static int opt_handler(int opt, int opt_index, void *data)
+{
+	if (opt == 'l') {
+		lspcon_present = true;
+		igt_info("set LSPcon present on DP ports\n");
+	}
 
-igt_simple_main
+	return 0;
+}
+
+int main(int argc, char **argv)
 {
 	DIR *dirp;
 	struct dirent *de;
+	lspcon_present = false;
+
+	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
+				      opt_handler, NULL);
+
+	igt_skip_on_simulation();
 
 	dirp = opendir("/sys/class/drm");
 	igt_assert(dirp != NULL);
@@ -49,17 +74,34 @@ igt_simple_main
 		struct igt_mean mean = {};
 		struct stat st;
 		char path[PATH_MAX];
-		int i;
+		char edid_path[PATH_MAX];
+		char edid[512]; /* enough for 4 block edid */
+		unsigned long edid_size = 0;
+		int i, fd_edid;
+		unsigned int threshold = 0;
 
 		if (*de->d_name == '.')
 			continue;;
 
 		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
 				de->d_name);
+		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
+				de->d_name);
 
 		if (stat(path, &st))
 			continue;
 
+		fd_edid = open(edid_path, O_RDONLY);
+		if (fd_edid == -1) {
+			igt_warn("Read Error EDID\n");
+			continue;
+		}
+
+		edid_size = read(fd_edid, edid, 512);
+		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
+		if (lspcon_present || !strncmp(de->d_name, "card0-HDMI", 10))
+			threshold *= HDMI_THRESHOLD_MULTIPLIER;
+
 		igt_mean_init(&mean);
 		for (i = 0; i < CHECK_TIMES; i++) {
 			struct timespec ts = {};
@@ -76,22 +118,26 @@ igt_simple_main
 		}
 
 		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
-			  "mean.avg %.2fns, %.2fus, %.2fms\n",
+			  "mean.avg %.2fns, %.2fus, %.2fms, "
+			  "edid_size %lu, threshold %d\n",
 			  de->d_name,
 			  mean.max, mean.max / 1e3, mean.max / 1e6,
-			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
+			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
+			  edid_size, threshold);
 
-		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
+		if (edid_size == 0 &&
+		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
 			igt_warn("%s: probe time exceed 10ms, "
 				 "max=%.2fms, avg=%.2fms\n", de->d_name,
 				 mean.max / 1e6, mean.mean / 1e6);
 		}
-		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
-			     "%s: average probe time exceeded 50ms, "
-			     "max=%.2fms, avg=%.2fms\n", de->d_name,
+		if (edid_size > 0)
+			igt_assert_f(mean.mean < (threshold * 1e6),
+			     "%s: average probe time exceeded %dms, "
+			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
 			     mean.max / 1e6, mean.mean / 1e6);
 
 	}
 	closedir(dirp);
-
+	igt_exit();
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Lofstedt, Marta]

> -----Original Message-----
> From: Taylor, Clinton A
> Sent: Thursday, August 10, 2017 8:50 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Taylor, Clinton A <clinton.a.taylor@intel.com>; Vetter, Daniel
> <daniel.vetter@intel.com>; Lofstedt, Marta <marta.lofstedt@intel.com>
> Subject: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid
> read
> 
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Current 50ms max threshold timing for an EDID read is very close to the
> actual time for a 2 block HDMI EDID read. Adjust the timings base on
> connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon is
> connected to device under test the -l option should be passed to update the
> threshold timing to allow the LSPcon to read the EDID at the HDMI timing.
> The -l option should be used when LSPcon is on the motherboard or if a
> USB_C->HDMI converter is present
> 
> V2: Adjust timings based on connector type, EDID size, and Add an option to
> specify if an LSPcon is present.
> V3: Add bugzilla to commit message
> V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  tests/kms_sysfs_edid_timing.c | 74
> +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 60 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> index 1201388..0382610 100644
> --- a/tests/kms_sysfs_edid_timing.c
> +++ b/tests/kms_sysfs_edid_timing.c
> @@ -26,21 +26,46 @@
>  #include <fcntl.h>
>  #include <sys/stat.h>
> 
> -#define THRESHOLD_PER_CONNECTOR	10
> -#define THRESHOLD_TOTAL		50
> -#define CHECK_TIMES		15
> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> +#define THRESHOLD_PER_EDID_BLOCK		5
> +#define HDMI_THRESHOLD_MULTIPLIER		10
> +#define CHECK_TIMES				10
> 
>  IGT_TEST_DESCRIPTION("This check the time we take to read the content of
> all "
>  		     "the possible connectors. Without the edid -
> ENXIO patch "
> 
> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> -		     "we sometimes take a *really* long time. "
> -		     "So let's just check for some reasonable
> timing here");
> +		     "we sometimes take a *really* long time. So
> let's check "
> +		     "an approximate time per edid block based on
> connector "
> +		     "type. The -l option adjusts DP timing to
> reflect HDMI read "
> +		     "timings from LSPcon.");
> +
> +/* The -l option has been added to correctly handle timings when an
> +LSPcon is
> + * present. This could be on the platform itself or in a USB_C->HDMI
> converter.
> + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
> + * bus speed to the 100 Kbit HDMI DDC bus speed  */ bool
> +lspcon_present;
> 
> +static int opt_handler(int opt, int opt_index, void *data) {
> +	if (opt == 'l') {
> +		lspcon_present = true;
> +		igt_info("set LSPcon present on DP ports\n");
> +	}
> 
> -igt_simple_main
> +	return 0;
> +}
> +
> +int main(int argc, char **argv)
>  {
>  	DIR *dirp;
>  	struct dirent *de;
> +	lspcon_present = false;
> +
> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> +				      opt_handler,
> NULL);

We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI. 
Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
Your detailed work could however be used in a benchmark, where the result would be the actually timing, I am sure there is a performance team who would like that.

/Marta

> +
> +	igt_skip_on_simulation();
> 
>  	dirp = opendir("/sys/class/drm");
>  	igt_assert(dirp != NULL);
> @@ -49,17 +74,34 @@ igt_simple_main
>  		struct igt_mean mean = {};
>  		struct stat st;
>  		char path[PATH_MAX];
> -		int i;
> +		char edid_path[PATH_MAX];
> +		char edid[512]; /* enough for 4 block edid */
> +		unsigned long edid_size = 0;
> +		int i, fd_edid;
> +		unsigned int threshold = 0;
> 
>  		if (*de->d_name == '.')
>  			continue;;
> 
>  		snprintf(path, sizeof(path),
> "/sys/class/drm/%s/status",
>  				de->d_name);
> +		snprintf(edid_path, sizeof(edid_path),
> "/sys/class/drm/%s/edid",
> +				de->d_name);
> 
>  		if (stat(path, &st))
>  			continue;
> 
> +		fd_edid = open(edid_path, O_RDONLY);
> +		if (fd_edid == -1) {
> +			igt_warn("Read Error EDID\n");
> +			continue;
> +		}
> +
> +		edid_size = read(fd_edid, edid, 512);
> +		threshold = THRESHOLD_PER_EDID_BLOCK *
> (edid_size / 128);
> +		if (lspcon_present || !strncmp(de->d_name,
> "card0-HDMI", 10))
> +			threshold *=
> HDMI_THRESHOLD_MULTIPLIER;
> +
>  		igt_mean_init(&mean);
>  		for (i = 0; i < CHECK_TIMES; i++) {
>  			struct timespec ts = {};
> @@ -76,22 +118,26 @@ igt_simple_main
>  		}
> 
>  		igt_debug("%s: mean.max %.2fns, %.2fus,
> %.2fms, "
> -			  "mean.avg %.2fns, %.2fus,
> %.2fms\n",
> +			  "mean.avg %.2fns, %.2fus,
> %.2fms, "
> +			  "edid_size %lu, threshold %d\n",
>  			  de->d_name,
>  			  mean.max, mean.max / 1e3,
> mean.max / 1e6,
> -			  mean.mean, mean.mean / 1e3,
> mean.mean / 1e6);
> +			  mean.mean, mean.mean / 1e3,
> mean.mean / 1e6,
> +			  edid_size, threshold);
> 
> -		if (mean.max > (THRESHOLD_PER_CONNECTOR
> * 1e6)) {
> +		if (edid_size == 0 &&
> +		   (mean.max >
> (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
>  			igt_warn("%s: probe time exceed
> 10ms, "
>  				 "max=%.2fms,
> avg=%.2fms\n", de->d_name,
>  				 mean.max / 1e6,
> mean.mean / 1e6);
>  		}
> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL
> * 1e6),
> -			     "%s: average probe time
> exceeded 50ms, "
> -			     "max=%.2fms, avg=%.2fms\n",
> de->d_name,
> +		if (edid_size > 0)
> +			igt_assert_f(mean.mean <
> (threshold * 1e6),
> +			     "%s: average probe time
> exceeded %dms, "
> +			     "max=%.2fms, avg=%.2fms\n",
> de->d_name, threshold,
>  			     mean.max / 1e6, mean.mean /
> 1e6);
> 
>  	}
>  	closedir(dirp);
> -
> +	igt_exit();
>  }
> --
> 1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for tests/kms: increase max threshold time for edid read (rev5)

[Patchwork]

== Series Details ==

Series: tests/kms: increase max threshold time for edid read (rev5)
URL   : https://patchwork.freedesktop.org/series/28399/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
1385b31d9371fae02af2fd8adb0d9ea86a5bb0f2 tests/igt_command_line: Ignore subtest list for kms_ccs

with latest DRM-Tip kernel build CI_DRM_2948
fbb8288699ef drm-tip: 2017y-08m-11d-09h-03m-47s UTC integration manifest

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:452s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:429s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:361s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:541s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:520s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:517s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:509s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:600s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:444s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:423s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:421s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:512s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:481s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:579s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:594s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:530s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:460s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:474s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:481s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:476s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:549s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:406s

== Logs ==

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

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Clint Taylor]

On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
>
>> -----Original Message-----
>> From: Taylor, Clinton A
>> Sent: Thursday, August 10, 2017 8:50 PM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Taylor, Clinton A <clinton.a.taylor@intel.com>; Vetter, Daniel
>> <daniel.vetter@intel.com>; Lofstedt, Marta <marta.lofstedt@intel.com>
>> Subject: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid
>> read
>>
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> Current 50ms max threshold timing for an EDID read is very close to the
>> actual time for a 2 block HDMI EDID read. Adjust the timings base on
>> connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon is
>> connected to device under test the -l option should be passed to update the
>> threshold timing to allow the LSPcon to read the EDID at the HDMI timing.
>> The -l option should be used when LSPcon is on the motherboard or if a
>> USB_C->HDMI converter is present
>>
>> V2: Adjust timings based on connector type, EDID size, and Add an option to
>> specify if an LSPcon is present.
>> V3: Add bugzilla to commit message
>> V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> ---
>>   tests/kms_sysfs_edid_timing.c | 74
>> +++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 60 insertions(+), 14 deletions(-)
>>
>> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
>> index 1201388..0382610 100644
>> --- a/tests/kms_sysfs_edid_timing.c
>> +++ b/tests/kms_sysfs_edid_timing.c
>> @@ -26,21 +26,46 @@
>>   #include <fcntl.h>
>>   #include <sys/stat.h>
>>
>> -#define THRESHOLD_PER_CONNECTOR	10
>> -#define THRESHOLD_TOTAL		50
>> -#define CHECK_TIMES		15
>> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
>> +#define THRESHOLD_PER_EDID_BLOCK		5
>> +#define HDMI_THRESHOLD_MULTIPLIER		10
>> +#define CHECK_TIMES				10
>>
>>   IGT_TEST_DESCRIPTION("This check the time we take to read the content of
>> all "
>>   		     "the possible connectors. Without the edid -
>> ENXIO patch "
>>
>> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083),"
>> -		     "we sometimes take a *really* long time. "
>> -		     "So let's just check for some reasonable
>> timing here");
>> +		     "we sometimes take a *really* long time. So
>> let's check "
>> +		     "an approximate time per edid block based on
>> connector "
>> +		     "type. The -l option adjusts DP timing to
>> reflect HDMI read "
>> +		     "timings from LSPcon.");
>> +
>> +/* The -l option has been added to correctly handle timings when an
>> +LSPcon is
>> + * present. This could be on the platform itself or in a USB_C->HDMI
>> converter.
>> + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
>> + * bus speed to the 100 Kbit HDMI DDC bus speed  */ bool
>> +lspcon_present;
>>
>> +static int opt_handler(int opt, int opt_index, void *data) {
>> +	if (opt == 'l') {
>> +		lspcon_present = true;
>> +		igt_info("set LSPcon present on DP ports\n");
>> +	}
>>
>> -igt_simple_main
>> +	return 0;
>> +}
>> +
>> +int main(int argc, char **argv)
>>   {
>>   	DIR *dirp;
>>   	struct dirent *de;
>> +	lspcon_present = false;
>> +
>> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
>> +				      opt_handler,
>> NULL);
> We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI.
> Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
The test would need to know if an LSPcon is connected on a port by port 
basis. This is easy if the LSPcon driver is loaded but in the case of 
USB_C->HDMI is gets a little more complicated (not impossible) to figure 
out. Even if we know exactly what device is connected failures can still 
occur if the TCON/Monitor clock stretches the EDID read.

> The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
Unfortunately with the timing differences (3ms to 96ms) based on the 
monitor type connected and EDID size there is no way for a one size fits 
all sanity check to be reliable. If the original point of this test was 
to figure out if probe caused more than 1 EDID read, maybe we should 
actually count EDID reads and not infer it by measuring time.

-Clint

> Your detailed work could however be used in a benchmark, where the result would be the actually timing, I am sure there is a performance team who would like that.
>
> /Marta
>
>> +
>> +	igt_skip_on_simulation();
>>
>>   	dirp = opendir("/sys/class/drm");
>>   	igt_assert(dirp != NULL);
>> @@ -49,17 +74,34 @@ igt_simple_main
>>   		struct igt_mean mean = {};
>>   		struct stat st;
>>   		char path[PATH_MAX];
>> -		int i;
>> +		char edid_path[PATH_MAX];
>> +		char edid[512]; /* enough for 4 block edid */
>> +		unsigned long edid_size = 0;
>> +		int i, fd_edid;
>> +		unsigned int threshold = 0;
>>
>>   		if (*de->d_name == '.')
>>   			continue;;
>>
>>   		snprintf(path, sizeof(path),
>> "/sys/class/drm/%s/status",
>>   				de->d_name);
>> +		snprintf(edid_path, sizeof(edid_path),
>> "/sys/class/drm/%s/edid",
>> +				de->d_name);
>>
>>   		if (stat(path, &st))
>>   			continue;
>>
>> +		fd_edid = open(edid_path, O_RDONLY);
>> +		if (fd_edid == -1) {
>> +			igt_warn("Read Error EDID\n");
>> +			continue;
>> +		}
>> +
>> +		edid_size = read(fd_edid, edid, 512);
>> +		threshold = THRESHOLD_PER_EDID_BLOCK *
>> (edid_size / 128);
>> +		if (lspcon_present || !strncmp(de->d_name,
>> "card0-HDMI", 10))
>> +			threshold *=
>> HDMI_THRESHOLD_MULTIPLIER;
>> +
>>   		igt_mean_init(&mean);
>>   		for (i = 0; i < CHECK_TIMES; i++) {
>>   			struct timespec ts = {};
>> @@ -76,22 +118,26 @@ igt_simple_main
>>   		}
>>
>>   		igt_debug("%s: mean.max %.2fns, %.2fus,
>> %.2fms, "
>> -			  "mean.avg %.2fns, %.2fus,
>> %.2fms\n",
>> +			  "mean.avg %.2fns, %.2fus,
>> %.2fms, "
>> +			  "edid_size %lu, threshold %d\n",
>>   			  de->d_name,
>>   			  mean.max, mean.max / 1e3,
>> mean.max / 1e6,
>> -			  mean.mean, mean.mean / 1e3,
>> mean.mean / 1e6);
>> +			  mean.mean, mean.mean / 1e3,
>> mean.mean / 1e6,
>> +			  edid_size, threshold);
>>
>> -		if (mean.max > (THRESHOLD_PER_CONNECTOR
>> * 1e6)) {
>> +		if (edid_size == 0 &&
>> +		   (mean.max >
>> (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
>>   			igt_warn("%s: probe time exceed
>> 10ms, "
>>   				 "max=%.2fms,
>> avg=%.2fms\n", de->d_name,
>>   				 mean.max / 1e6,
>> mean.mean / 1e6);
>>   		}
>> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL
>> * 1e6),
>> -			     "%s: average probe time
>> exceeded 50ms, "
>> -			     "max=%.2fms, avg=%.2fms\n",
>> de->d_name,
>> +		if (edid_size > 0)
>> +			igt_assert_f(mean.mean <
>> (threshold * 1e6),
>> +			     "%s: average probe time
>> exceeded %dms, "
>> +			     "max=%.2fms, avg=%.2fms\n",
>> de->d_name, threshold,
>>   			     mean.max / 1e6, mean.mean /
>> 1e6);
>>
>>   	}
>>   	closedir(dirp);
>> -
>> +	igt_exit();
>>   }
>> --
>> 1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Lofstedt, Marta]

> -----Original Message-----
> From: Taylor, Clinton A
> Sent: Friday, August 11, 2017 7:36 PM
> To: Lofstedt, Marta <marta.lofstedt@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Vetter, Daniel <daniel.vetter@intel.com>
> Subject: Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid
> read
> 
> 
> 
> On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
> >
> >> -----Original Message-----
> >> From: Taylor, Clinton A
> >> Sent: Thursday, August 10, 2017 8:50 PM
> >> To: intel-gfx@lists.freedesktop.org
> >> Cc: Taylor, Clinton A <clinton.a.taylor@intel.com>; Vetter, Daniel
> >> <daniel.vetter@intel.com>; Lofstedt, Marta <marta.lofstedt@intel.com>
> >> Subject: [PATCH v4 i-g-t] tests/kms: increase max threshold time for
> >> edid read
> >>
> >> From: Clint Taylor <clinton.a.taylor@intel.com>
> >>
> >> Current 50ms max threshold timing for an EDID read is very close to
> >> the actual time for a 2 block HDMI EDID read. Adjust the timings base
> >> on connector type as DP reads are at 1 MBit and HDMI at 100K bit. If
> >> an LSPcon is connected to device under test the -l option should be
> >> passed to update the threshold timing to allow the LSPcon to read the
> EDID at the HDMI timing.
> >> The -l option should be used when LSPcon is on the motherboard or if
> >> a USB_C->HDMI converter is present
> >>
> >> V2: Adjust timings based on connector type, EDID size, and Add an
> >> option to specify if an LSPcon is present.
> >> V3: Add bugzilla to commit message
> >> V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> >>
> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> >> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> >> ---
> >>   tests/kms_sysfs_edid_timing.c | 74
> >> +++++++++++++++++++++++++++++++++++--------
> >>   1 file changed, 60 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/tests/kms_sysfs_edid_timing.c
> >> b/tests/kms_sysfs_edid_timing.c index 1201388..0382610 100644
> >> --- a/tests/kms_sysfs_edid_timing.c
> >> +++ b/tests/kms_sysfs_edid_timing.c
> >> @@ -26,21 +26,46 @@
> >>   #include <fcntl.h>
> >>   #include <sys/stat.h>
> >>
> >> -#define THRESHOLD_PER_CONNECTOR	10
> >> -#define THRESHOLD_TOTAL		50
> >> -#define CHECK_TIMES		15
> >> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> >> +#define THRESHOLD_PER_EDID_BLOCK		5
> >> +#define HDMI_THRESHOLD_MULTIPLIER		10
> >> +#define CHECK_TIMES				10
> >>
> >>   IGT_TEST_DESCRIPTION("This check the time we take to read the
> >> content of all "
> >>   		     "the possible connectors. Without the edid -
> ENXIO patch "
> >>
> >> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083),"
> >> -		     "we sometimes take a *really* long time. "
> >> -		     "So let's just check for some reasonable
> >> timing here");
> >> +		     "we sometimes take a *really* long time. So
> >> let's check "
> >> +		     "an approximate time per edid block based on
> >> connector "
> >> +		     "type. The -l option adjusts DP timing to
> >> reflect HDMI read "
> >> +		     "timings from LSPcon.");
> >> +
> >> +/* The -l option has been added to correctly handle timings when an
> >> +LSPcon is
> >> + * present. This could be on the platform itself or in a USB_C->HDMI
> >> converter.
> >> + * With LSPCon EDID read timing will need to change from the 1 Mbit
> >> +AUX
> >> + * bus speed to the 100 Kbit HDMI DDC bus speed  */ bool
> >> +lspcon_present;
> >>
> >> +static int opt_handler(int opt, int opt_index, void *data) {
> >> +	if (opt == 'l') {
> >> +		lspcon_present = true;
> >> +		igt_info("set LSPcon present on DP ports\n");
> >> +	}
> >>
> >> -igt_simple_main
> >> +	return 0;
> >> +}
> >> +
> >> +int main(int argc, char **argv)
> >>   {
> >>   	DIR *dirp;
> >>   	struct dirent *de;
> >> +	lspcon_present = false;
> >> +
> >> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> >> +				      opt_handler,
> >> NULL);
> > We can't have this lspcon as an argument to the test, it will not work with
> automated testing using piglit as we do for CI.
> > Theoretically we could add a "has_lspcon" to debugfs, but I believe that
> this test has started to be over complicated.
> The test would need to know if an LSPcon is connected on a port by port
> basis. This is easy if the LSPcon driver is loaded but in the case of USB_C-
> >HDMI is gets a little more complicated (not impossible) to figure out. Even if
> we know exactly what device is connected failures can still occur if the
> TCON/Monitor clock stretches the EDID read.
> 
> > The intention of the test is to do a sanity check so that we don't drift off
> here, so I actually prefer the V1.
> Unfortunately with the timing differences (3ms to 96ms) based on the
> monitor type connected and EDID size there is no way for a one size fits all
> sanity check to be reliable. If the original point of this test was to figure out if
> probe caused more than 1 EDID read, maybe we should actually count EDID
> reads and not infer it by measuring time.
> 
That sound good to me, but let's see what the list thinks.
/Marta

> -Clint
> 
> > Your detailed work could however be used in a benchmark, where the
> result would be the actually timing, I am sure there is a performance team
> who would like that.
> >
> > /Marta
> >
> >> +
> >> +	igt_skip_on_simulation();
> >>
> >>   	dirp = opendir("/sys/class/drm");
> >>   	igt_assert(dirp != NULL);
> >> @@ -49,17 +74,34 @@ igt_simple_main
> >>   		struct igt_mean mean = {};
> >>   		struct stat st;
> >>   		char path[PATH_MAX];
> >> -		int i;
> >> +		char edid_path[PATH_MAX];
> >> +		char edid[512]; /* enough for 4 block edid */
> >> +		unsigned long edid_size = 0;
> >> +		int i, fd_edid;
> >> +		unsigned int threshold = 0;
> >>
> >>   		if (*de->d_name == '.')
> >>   			continue;;
> >>
> >>   		snprintf(path, sizeof(path),
> >> "/sys/class/drm/%s/status",
> >>   				de->d_name);
> >> +		snprintf(edid_path, sizeof(edid_path),
> >> "/sys/class/drm/%s/edid",
> >> +				de->d_name);
> >>
> >>   		if (stat(path, &st))
> >>   			continue;
> >>
> >> +		fd_edid = open(edid_path, O_RDONLY);
> >> +		if (fd_edid == -1) {
> >> +			igt_warn("Read Error EDID\n");
> >> +			continue;
> >> +		}
> >> +
> >> +		edid_size = read(fd_edid, edid, 512);
> >> +		threshold = THRESHOLD_PER_EDID_BLOCK *
> >> (edid_size / 128);
> >> +		if (lspcon_present || !strncmp(de->d_name,
> >> "card0-HDMI", 10))
> >> +			threshold *=
> >> HDMI_THRESHOLD_MULTIPLIER;
> >> +
> >>   		igt_mean_init(&mean);
> >>   		for (i = 0; i < CHECK_TIMES; i++) {
> >>   			struct timespec ts = {};
> >> @@ -76,22 +118,26 @@ igt_simple_main
> >>   		}
> >>
> >>   		igt_debug("%s: mean.max %.2fns, %.2fus,
> %.2fms, "
> >> -			  "mean.avg %.2fns, %.2fus,
> >> %.2fms\n",
> >> +			  "mean.avg %.2fns, %.2fus,
> >> %.2fms, "
> >> +			  "edid_size %lu, threshold %d\n",
> >>   			  de->d_name,
> >>   			  mean.max, mean.max / 1e3,
> >> mean.max / 1e6,
> >> -			  mean.mean, mean.mean / 1e3,
> >> mean.mean / 1e6);
> >> +			  mean.mean, mean.mean / 1e3,
> >> mean.mean / 1e6,
> >> +			  edid_size, threshold);
> >>
> >> -		if (mean.max > (THRESHOLD_PER_CONNECTOR
> >> * 1e6)) {
> >> +		if (edid_size == 0 &&
> >> +		   (mean.max >
> >> (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
> >>   			igt_warn("%s: probe time exceed
> >> 10ms, "
> >>   				 "max=%.2fms,
> >> avg=%.2fms\n", de->d_name,
> >>   				 mean.max / 1e6,
> >> mean.mean / 1e6);
> >>   		}
> >> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL
> >> * 1e6),
> >> -			     "%s: average probe time
> >> exceeded 50ms, "
> >> -			     "max=%.2fms, avg=%.2fms\n",
> >> de->d_name,
> >> +		if (edid_size > 0)
> >> +			igt_assert_f(mean.mean <
> >> (threshold * 1e6),
> >> +			     "%s: average probe time
> >> exceeded %dms, "
> >> +			     "max=%.2fms, avg=%.2fms\n",
> >> de->d_name, threshold,
> >>   			     mean.max / 1e6, mean.mean /
> 1e6);
> >>
> >>   	}
> >>   	closedir(dirp);
> >> -
> >> +	igt_exit();
> >>   }
> >> --
> >> 1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Jani Nikula]

On Fri, 11 Aug 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
>> We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI.
>> Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
> The test would need to know if an LSPcon is connected on a port by port 
> basis. This is easy if the LSPcon driver is loaded but in the case of 
> USB_C->HDMI is gets a little more complicated (not impossible) to figure 
> out. Even if we know exactly what device is connected failures can still 
> occur if the TCON/Monitor clock stretches the EDID read.
>
>> The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
> Unfortunately with the timing differences (3ms to 96ms) based on the 
> monitor type connected and EDID size there is no way for a one size fits 
> all sanity check to be reliable. If the original point of this test was 
> to figure out if probe caused more than 1 EDID read, maybe we should 
> actually count EDID reads and not infer it by measuring time.

I'll take a step back here and try to look at the big picture.

I think the point of the test originally was to catch regressions in the
EDID code that would slow down EDID reading. That's a valid thing to
test per se, but unfortunately it's not easy to do that accurately. The
basic alternatives are, from most accurate to least accurate:

1) Start off with reference timing, and make relative comparisons
against that. This is not unlike performance testing. The problem is, to
not compare apples and oranges, you'll need to take into account
platform, connector type, adapters, cables, hubs, EDID size, and the
display. Change one, and you can't make the comparison anymore. You end
up tracking configurations and timings, a lot.

2) Try to make a reasonable estimate of the absolute maximum based on
some subset of the configuration (such as connector type and
adapters). If you stay below, regardless of the timing changes, you
assume it's fine, and otherwise you consider it a regression. So you try
to keep the limits tight to catch regressions, but not flag normal
behaviour too much. You end up accumulating heuristics for the
configuration and timing, because, let's face it, there is a lot of
variance in what's acceptable. (This is v2+ of the patch at hand.)

3) Try to make a reasonable estimate of the absolute maximum independent
of the configuration. You'll never catch regressions in the fast
configurations, because your limits are based on the worst case. You'll
only catch the really pathological problems. Note that the current code
uses mean, so it evens out connector type specific problems; we might
also not catch pathological problems in, say, DP if the HDMI is
fast. (This is the original and v1 of the patch.)

I think 1) is more trouble than it's worth. 3) is simplistic but
easy. The question is, do we get enough benefit from 2) to offset the
complications?


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Daniel Vetter]

On Mon, Aug 14, 2017 at 11:43:34AM +0300, Jani Nikula wrote:
> On Fri, 11 Aug 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> > On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
> >> We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI.
> >> Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
> > The test would need to know if an LSPcon is connected on a port by port 
> > basis. This is easy if the LSPcon driver is loaded but in the case of 
> > USB_C->HDMI is gets a little more complicated (not impossible) to figure 
> > out. Even if we know exactly what device is connected failures can still 
> > occur if the TCON/Monitor clock stretches the EDID read.
> >
> >> The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
> > Unfortunately with the timing differences (3ms to 96ms) based on the 
> > monitor type connected and EDID size there is no way for a one size fits 
> > all sanity check to be reliable. If the original point of this test was 
> > to figure out if probe caused more than 1 EDID read, maybe we should 
> > actually count EDID reads and not infer it by measuring time.
> 
> I'll take a step back here and try to look at the big picture.
> 
> I think the point of the test originally was to catch regressions in the
> EDID code that would slow down EDID reading. That's a valid thing to
> test per se, but unfortunately it's not easy to do that accurately. The
> basic alternatives are, from most accurate to least accurate:
> 
> 1) Start off with reference timing, and make relative comparisons
> against that. This is not unlike performance testing. The problem is, to
> not compare apples and oranges, you'll need to take into account
> platform, connector type, adapters, cables, hubs, EDID size, and the
> display. Change one, and you can't make the comparison anymore. You end
> up tracking configurations and timings, a lot.
> 
> 2) Try to make a reasonable estimate of the absolute maximum based on
> some subset of the configuration (such as connector type and
> adapters). If you stay below, regardless of the timing changes, you
> assume it's fine, and otherwise you consider it a regression. So you try
> to keep the limits tight to catch regressions, but not flag normal
> behaviour too much. You end up accumulating heuristics for the
> configuration and timing, because, let's face it, there is a lot of
> variance in what's acceptable. (This is v2+ of the patch at hand.)
> 
> 3) Try to make a reasonable estimate of the absolute maximum independent
> of the configuration. You'll never catch regressions in the fast
> configurations, because your limits are based on the worst case. You'll
> only catch the really pathological problems. Note that the current code
> uses mean, so it evens out connector type specific problems; we might
> also not catch pathological problems in, say, DP if the HDMI is
> fast. (This is the original and v1 of the patch.)
> 
> I think 1) is more trouble than it's worth. 3) is simplistic but
> easy. The question is, do we get enough benefit from 2) to offset the
> complications?

Why is 2) hard? EDID read takes 22ms at wire-speed, per block. Add a bit
of fudge, gives us 10ms + 25ms*num_edid_blocks. Imo everything else is a
bug in our kernel driver. I have no idea why lspcon matters or not,
because it really shouldn't.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Daniel Vetter]

On Thu, Aug 10, 2017 at 10:50:19AM -0700, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Current 50ms max threshold timing for an EDID read is very close to the
> actual time for a 2 block HDMI EDID read. Adjust the timings base on
> connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
> is connected to device under test the -l option should be passed to update
> the threshold timing to allow the LSPcon to read the EDID at the HDMI
> timing. The -l option should be used when LSPcon is on the motherboard or
> if a USB_C->HDMI converter is present
> 
> V2: Adjust timings based on connector type, EDID size, and Add an option to
> specify if an LSPcon is present.
> V3: Add bugzilla to commit message
> V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  tests/kms_sysfs_edid_timing.c | 74 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 60 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> index 1201388..0382610 100644
> --- a/tests/kms_sysfs_edid_timing.c
> +++ b/tests/kms_sysfs_edid_timing.c
> @@ -26,21 +26,46 @@
>  #include <fcntl.h>
>  #include <sys/stat.h>
>  
> -#define THRESHOLD_PER_CONNECTOR	10
> -#define THRESHOLD_TOTAL		50
> -#define CHECK_TIMES		15
> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> +#define THRESHOLD_PER_EDID_BLOCK		5
> +#define HDMI_THRESHOLD_MULTIPLIER		10
> +#define CHECK_TIMES				10
>  
>  IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
>  		     "the possible connectors. Without the edid -ENXIO patch "
>  		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> -		     "we sometimes take a *really* long time. "
> -		     "So let's just check for some reasonable timing here");
> +		     "we sometimes take a *really* long time. So let's check "
> +		     "an approximate time per edid block based on connector "
> +		     "type. The -l option adjusts DP timing to reflect HDMI read "
> +		     "timings from LSPcon.");
> +
> +/* The -l option has been added to correctly handle timings when an LSPcon is
> + * present. This could be on the platform itself or in a USB_C->HDMI converter.
> + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
> + * bus speed to the 100 Kbit HDMI DDC bus speed

This is blantantly wrong. EDID reads are done at 100kbit, even over dp
aux. There's some panels which have forgoe the i2c bus that
i2c-over-dp-aux drivers and givey ou the EDID a bit faster, but edids read
at 100kbit.

That means 25ms per edid block and no special cases anywhere. I think you
can still keep the 10ms for basic probe (but really shouldn't be needed).

Note your limits are off by 2x, since you use 50ms per 128b edid block. We
can totally read a CEA edid with 2 blocks in 50ms.
-Daniel

> + */
> +bool lspcon_present;
>  
> +static int opt_handler(int opt, int opt_index, void *data)
> +{
> +	if (opt == 'l') {
> +		lspcon_present = true;
> +		igt_info("set LSPcon present on DP ports\n");
> +	}
>  
> -igt_simple_main
> +	return 0;
> +}
> +
> +int main(int argc, char **argv)
>  {
>  	DIR *dirp;
>  	struct dirent *de;
> +	lspcon_present = false;
> +
> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> +				      opt_handler, NULL);
> +
> +	igt_skip_on_simulation();
>  
>  	dirp = opendir("/sys/class/drm");
>  	igt_assert(dirp != NULL);
> @@ -49,17 +74,34 @@ igt_simple_main
>  		struct igt_mean mean = {};
>  		struct stat st;
>  		char path[PATH_MAX];
> -		int i;
> +		char edid_path[PATH_MAX];
> +		char edid[512]; /* enough for 4 block edid */
> +		unsigned long edid_size = 0;
> +		int i, fd_edid;
> +		unsigned int threshold = 0;
>  
>  		if (*de->d_name == '.')
>  			continue;;
>  
>  		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
>  				de->d_name);
> +		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
> +				de->d_name);
>  
>  		if (stat(path, &st))
>  			continue;
>  
> +		fd_edid = open(edid_path, O_RDONLY);
> +		if (fd_edid == -1) {
> +			igt_warn("Read Error EDID\n");
> +			continue;
> +		}
> +
> +		edid_size = read(fd_edid, edid, 512);
> +		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
> +		if (lspcon_present || !strncmp(de->d_name, "card0-HDMI", 10))
> +			threshold *= HDMI_THRESHOLD_MULTIPLIER;
> +
>  		igt_mean_init(&mean);
>  		for (i = 0; i < CHECK_TIMES; i++) {
>  			struct timespec ts = {};
> @@ -76,22 +118,26 @@ igt_simple_main
>  		}
>  
>  		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
> -			  "mean.avg %.2fns, %.2fus, %.2fms\n",
> +			  "mean.avg %.2fns, %.2fus, %.2fms, "
> +			  "edid_size %lu, threshold %d\n",
>  			  de->d_name,
>  			  mean.max, mean.max / 1e3, mean.max / 1e6,
> -			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
> +			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
> +			  edid_size, threshold);
>  
> -		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
> +		if (edid_size == 0 &&
> +		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
>  			igt_warn("%s: probe time exceed 10ms, "
>  				 "max=%.2fms, avg=%.2fms\n", de->d_name,
>  				 mean.max / 1e6, mean.mean / 1e6);
>  		}
> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
> -			     "%s: average probe time exceeded 50ms, "
> -			     "max=%.2fms, avg=%.2fms\n", de->d_name,
> +		if (edid_size > 0)
> +			igt_assert_f(mean.mean < (threshold * 1e6),
> +			     "%s: average probe time exceeded %dms, "
> +			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
>  			     mean.max / 1e6, mean.mean / 1e6);
>  
>  	}
>  	closedir(dirp);
> -
> +	igt_exit();
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Jani Nikula]

On Mon, 14 Aug 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Aug 14, 2017 at 11:43:34AM +0300, Jani Nikula wrote:
>> On Fri, 11 Aug 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>> > On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
>> >> We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI.
>> >> Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
>> > The test would need to know if an LSPcon is connected on a port by port 
>> > basis. This is easy if the LSPcon driver is loaded but in the case of 
>> > USB_C->HDMI is gets a little more complicated (not impossible) to figure 
>> > out. Even if we know exactly what device is connected failures can still 
>> > occur if the TCON/Monitor clock stretches the EDID read.
>> >
>> >> The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
>> > Unfortunately with the timing differences (3ms to 96ms) based on the 
>> > monitor type connected and EDID size there is no way for a one size fits 
>> > all sanity check to be reliable. If the original point of this test was 
>> > to figure out if probe caused more than 1 EDID read, maybe we should 
>> > actually count EDID reads and not infer it by measuring time.
>> 
>> I'll take a step back here and try to look at the big picture.
>> 
>> I think the point of the test originally was to catch regressions in the
>> EDID code that would slow down EDID reading. That's a valid thing to
>> test per se, but unfortunately it's not easy to do that accurately. The
>> basic alternatives are, from most accurate to least accurate:
>> 
>> 1) Start off with reference timing, and make relative comparisons
>> against that. This is not unlike performance testing. The problem is, to
>> not compare apples and oranges, you'll need to take into account
>> platform, connector type, adapters, cables, hubs, EDID size, and the
>> display. Change one, and you can't make the comparison anymore. You end
>> up tracking configurations and timings, a lot.
>> 
>> 2) Try to make a reasonable estimate of the absolute maximum based on
>> some subset of the configuration (such as connector type and
>> adapters). If you stay below, regardless of the timing changes, you
>> assume it's fine, and otherwise you consider it a regression. So you try
>> to keep the limits tight to catch regressions, but not flag normal
>> behaviour too much. You end up accumulating heuristics for the
>> configuration and timing, because, let's face it, there is a lot of
>> variance in what's acceptable. (This is v2+ of the patch at hand.)
>> 
>> 3) Try to make a reasonable estimate of the absolute maximum independent
>> of the configuration. You'll never catch regressions in the fast
>> configurations, because your limits are based on the worst case. You'll
>> only catch the really pathological problems. Note that the current code
>> uses mean, so it evens out connector type specific problems; we might
>> also not catch pathological problems in, say, DP if the HDMI is
>> fast. (This is the original and v1 of the patch.)
>> 
>> I think 1) is more trouble than it's worth. 3) is simplistic but
>> easy. The question is, do we get enough benefit from 2) to offset the
>> complications?
>
> Why is 2) hard? EDID read takes 22ms at wire-speed, per block. Add a bit
> of fudge, gives us 10ms + 25ms*num_edid_blocks. Imo everything else is a
> bug in our kernel driver. I have no idea why lspcon matters or not,
> because it really shouldn't.

2) is hard partly for the same reason 1) is hard. All of the things I
mention impact the overall speed. You only mention wire speed, but
e.g. DP i2c-over-aux can reply with a bunch of defers before anything
happens. The EDID reads are chuncked to a number of i2c-over-aux
transactions, *each* of which may defer. For a number of valid reasons.
See the commentary in drm_dp_i2c_do_msg(); we've basically punted on
accurate timeouts and retry counts, and set them high enough.

I don't think even the driver is capable of taking all of the variables
into account, let alone igt.

Bottom line, you can't set generic igt limits based on your wire speed
assumptions.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Daniel Vetter]

On Mon, Aug 14, 2017 at 5:30 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 14 Aug 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Aug 14, 2017 at 11:43:34AM +0300, Jani Nikula wrote:
>>> On Fri, 11 Aug 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>>> > On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
>>> >> We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI.
>>> >> Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
>>> > The test would need to know if an LSPcon is connected on a port by port
>>> > basis. This is easy if the LSPcon driver is loaded but in the case of
>>> > USB_C->HDMI is gets a little more complicated (not impossible) to figure
>>> > out. Even if we know exactly what device is connected failures can still
>>> > occur if the TCON/Monitor clock stretches the EDID read.
>>> >
>>> >> The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
>>> > Unfortunately with the timing differences (3ms to 96ms) based on the
>>> > monitor type connected and EDID size there is no way for a one size fits
>>> > all sanity check to be reliable. If the original point of this test was
>>> > to figure out if probe caused more than 1 EDID read, maybe we should
>>> > actually count EDID reads and not infer it by measuring time.
>>>
>>> I'll take a step back here and try to look at the big picture.
>>>
>>> I think the point of the test originally was to catch regressions in the
>>> EDID code that would slow down EDID reading. That's a valid thing to
>>> test per se, but unfortunately it's not easy to do that accurately. The
>>> basic alternatives are, from most accurate to least accurate:
>>>
>>> 1) Start off with reference timing, and make relative comparisons
>>> against that. This is not unlike performance testing. The problem is, to
>>> not compare apples and oranges, you'll need to take into account
>>> platform, connector type, adapters, cables, hubs, EDID size, and the
>>> display. Change one, and you can't make the comparison anymore. You end
>>> up tracking configurations and timings, a lot.
>>>
>>> 2) Try to make a reasonable estimate of the absolute maximum based on
>>> some subset of the configuration (such as connector type and
>>> adapters). If you stay below, regardless of the timing changes, you
>>> assume it's fine, and otherwise you consider it a regression. So you try
>>> to keep the limits tight to catch regressions, but not flag normal
>>> behaviour too much. You end up accumulating heuristics for the
>>> configuration and timing, because, let's face it, there is a lot of
>>> variance in what's acceptable. (This is v2+ of the patch at hand.)
>>>
>>> 3) Try to make a reasonable estimate of the absolute maximum independent
>>> of the configuration. You'll never catch regressions in the fast
>>> configurations, because your limits are based on the worst case. You'll
>>> only catch the really pathological problems. Note that the current code
>>> uses mean, so it evens out connector type specific problems; we might
>>> also not catch pathological problems in, say, DP if the HDMI is
>>> fast. (This is the original and v1 of the patch.)
>>>
>>> I think 1) is more trouble than it's worth. 3) is simplistic but
>>> easy. The question is, do we get enough benefit from 2) to offset the
>>> complications?
>>
>> Why is 2) hard? EDID read takes 22ms at wire-speed, per block. Add a bit
>> of fudge, gives us 10ms + 25ms*num_edid_blocks. Imo everything else is a
>> bug in our kernel driver. I have no idea why lspcon matters or not,
>> because it really shouldn't.
>
> 2) is hard partly for the same reason 1) is hard. All of the things I
> mention impact the overall speed. You only mention wire speed, but
> e.g. DP i2c-over-aux can reply with a bunch of defers before anything
> happens. The EDID reads are chuncked to a number of i2c-over-aux
> transactions, *each* of which may defer. For a number of valid reasons.
> See the commentary in drm_dp_i2c_do_msg(); we've basically punted on
> accurate timeouts and retry counts, and set them high enough.

i2c over dp aux can defer becuase dp aux runs at 2mbit, and i2c at
100kbit. The defers are to handle the mismatch. If you look actual
transfer rates, then the i2c transactions still completes at wire
speed of the i2c bus.

And yes we've had bugs in this area where i2c over dp_aux was suddenly
2x slower, so making allowances for bugs here isn't a good idea.

> I don't think even the driver is capable of taking all of the variables
> into account, let alone igt.

So the _one_ variable we haven't taken into account, which is the one
that spurred Clint here into this endeavor, is EDID size. There's
simply no way to transfer an 3block edid in 50ms. Afaik that's the
only bug. I've seen piles of displays, QA has checked for the 50ms
limit for ages, as long as your edid is only 256 bytes it all works.
Minor kernel bugs.

There is provisions in the i2c spec for clock stretching, but I've
never seen a screen that actually needs that.

> Bottom line, you can't set generic igt limits based on your wire speed
> assumptions.

Which bug breaks this assumption? All the stuff about lspcon is
because Clint assumed the wrong wire speed.

All I'm proposing is that we apply the one-liner that takes screens
with 3/4 block edids into account, because that's a new thing. Afaik
there's nothing else.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Jani Nikula]

On Mon, 14 Aug 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Aug 14, 2017 at 5:30 PM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>> On Mon, 14 Aug 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Mon, Aug 14, 2017 at 11:43:34AM +0300, Jani Nikula wrote:
>>>> On Fri, 11 Aug 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>>>> > On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
>>>> >> We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI.
>>>> >> Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
>>>> > The test would need to know if an LSPcon is connected on a port by port
>>>> > basis. This is easy if the LSPcon driver is loaded but in the case of
>>>> > USB_C->HDMI is gets a little more complicated (not impossible) to figure
>>>> > out. Even if we know exactly what device is connected failures can still
>>>> > occur if the TCON/Monitor clock stretches the EDID read.
>>>> >
>>>> >> The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
>>>> > Unfortunately with the timing differences (3ms to 96ms) based on the
>>>> > monitor type connected and EDID size there is no way for a one size fits
>>>> > all sanity check to be reliable. If the original point of this test was
>>>> > to figure out if probe caused more than 1 EDID read, maybe we should
>>>> > actually count EDID reads and not infer it by measuring time.
>>>>
>>>> I'll take a step back here and try to look at the big picture.
>>>>
>>>> I think the point of the test originally was to catch regressions in the
>>>> EDID code that would slow down EDID reading. That's a valid thing to
>>>> test per se, but unfortunately it's not easy to do that accurately. The
>>>> basic alternatives are, from most accurate to least accurate:
>>>>
>>>> 1) Start off with reference timing, and make relative comparisons
>>>> against that. This is not unlike performance testing. The problem is, to
>>>> not compare apples and oranges, you'll need to take into account
>>>> platform, connector type, adapters, cables, hubs, EDID size, and the
>>>> display. Change one, and you can't make the comparison anymore. You end
>>>> up tracking configurations and timings, a lot.
>>>>
>>>> 2) Try to make a reasonable estimate of the absolute maximum based on
>>>> some subset of the configuration (such as connector type and
>>>> adapters). If you stay below, regardless of the timing changes, you
>>>> assume it's fine, and otherwise you consider it a regression. So you try
>>>> to keep the limits tight to catch regressions, but not flag normal
>>>> behaviour too much. You end up accumulating heuristics for the
>>>> configuration and timing, because, let's face it, there is a lot of
>>>> variance in what's acceptable. (This is v2+ of the patch at hand.)
>>>>
>>>> 3) Try to make a reasonable estimate of the absolute maximum independent
>>>> of the configuration. You'll never catch regressions in the fast
>>>> configurations, because your limits are based on the worst case. You'll
>>>> only catch the really pathological problems. Note that the current code
>>>> uses mean, so it evens out connector type specific problems; we might
>>>> also not catch pathological problems in, say, DP if the HDMI is
>>>> fast. (This is the original and v1 of the patch.)
>>>>
>>>> I think 1) is more trouble than it's worth. 3) is simplistic but
>>>> easy. The question is, do we get enough benefit from 2) to offset the
>>>> complications?
>>>
>>> Why is 2) hard? EDID read takes 22ms at wire-speed, per block. Add a bit
>>> of fudge, gives us 10ms + 25ms*num_edid_blocks. Imo everything else is a
>>> bug in our kernel driver. I have no idea why lspcon matters or not,
>>> because it really shouldn't.
>>
>> 2) is hard partly for the same reason 1) is hard. All of the things I
>> mention impact the overall speed. You only mention wire speed, but
>> e.g. DP i2c-over-aux can reply with a bunch of defers before anything
>> happens. The EDID reads are chuncked to a number of i2c-over-aux
>> transactions, *each* of which may defer. For a number of valid reasons.
>> See the commentary in drm_dp_i2c_do_msg(); we've basically punted on
>> accurate timeouts and retry counts, and set them high enough.
>
> i2c over dp aux can defer becuase dp aux runs at 2mbit, and i2c at
> 100kbit. The defers are to handle the mismatch. If you look actual
> transfer rates, then the i2c transactions still completes at wire
> speed of the i2c bus.
>
> And yes we've had bugs in this area where i2c over dp_aux was suddenly
> 2x slower, so making allowances for bugs here isn't a good idea.
>
>> I don't think even the driver is capable of taking all of the variables
>> into account, let alone igt.
>
> So the _one_ variable we haven't taken into account, which is the one
> that spurred Clint here into this endeavor, is EDID size. There's
> simply no way to transfer an 3block edid in 50ms. Afaik that's the
> only bug. I've seen piles of displays, QA has checked for the 50ms
> limit for ages, as long as your edid is only 256 bytes it all works.
> Minor kernel bugs.

Agreed on EDID size being the thing with the largest impact that we're
not taking into account. And it's the low hanging fruit, too.

We can label the rest under hypothetical, at least until it's not... but
that doesn't conflict with EDID size needing to be fixed first. It's not
productive to argue on the rest.

BR,
Jani.


>
> There is provisions in the i2c spec for clock stretching, but I've
> never seen a screen that actually needs that.
>
>> Bottom line, you can't set generic igt limits based on your wire speed
>> assumptions.
>
> Which bug breaks this assumption? All the stuff about lspcon is
> because Clint assumed the wrong wire speed.
>
> All I'm proposing is that we apply the one-liner that takes screens
> with 3/4 block edids into account, because that's a new thing. Afaik
> there's nothing else.
> -Daniel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Clint Taylor]

On 08/14/2017 07:40 AM, Daniel Vetter wrote:
> On Thu, Aug 10, 2017 at 10:50:19AM -0700, clinton.a.taylor@intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> Current 50ms max threshold timing for an EDID read is very close to the
>> actual time for a 2 block HDMI EDID read. Adjust the timings base on
>> connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
>> is connected to device under test the -l option should be passed to update
>> the threshold timing to allow the LSPcon to read the EDID at the HDMI
>> timing. The -l option should be used when LSPcon is on the motherboard or
>> if a USB_C->HDMI converter is present
>>
>> V2: Adjust timings based on connector type, EDID size, and Add an option to
>> specify if an LSPcon is present.
>> V3: Add bugzilla to commit message
>> V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> ---
>>   tests/kms_sysfs_edid_timing.c | 74 +++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 60 insertions(+), 14 deletions(-)
>>
>> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
>> index 1201388..0382610 100644
>> --- a/tests/kms_sysfs_edid_timing.c
>> +++ b/tests/kms_sysfs_edid_timing.c
>> @@ -26,21 +26,46 @@
>>   #include <fcntl.h>
>>   #include <sys/stat.h>
>>   
>> -#define THRESHOLD_PER_CONNECTOR	10
>> -#define THRESHOLD_TOTAL		50
>> -#define CHECK_TIMES		15
>> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
>> +#define THRESHOLD_PER_EDID_BLOCK		5
>> +#define HDMI_THRESHOLD_MULTIPLIER		10
>> +#define CHECK_TIMES				10
>>   
>>   IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
>>   		     "the possible connectors. Without the edid -ENXIO patch "
>>   		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
>> -		     "we sometimes take a *really* long time. "
>> -		     "So let's just check for some reasonable timing here");
>> +		     "we sometimes take a *really* long time. So let's check "
>> +		     "an approximate time per edid block based on connector "
>> +		     "type. The -l option adjusts DP timing to reflect HDMI read "
>> +		     "timings from LSPcon.");
>> +
>> +/* The -l option has been added to correctly handle timings when an LSPcon is
>> + * present. This could be on the platform itself or in a USB_C->HDMI converter.
>> + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
>> + * bus speed to the 100 Kbit HDMI DDC bus speed
> This is blantantly wrong. EDID reads are done at 100kbit, even over dp
> aux. There's some panels which have forgoe the i2c bus that
> i2c-over-dp-aux drivers and givey ou the EDID a bit faster, but edids read
> at 100kbit.

Actually most modern devices deliver DP EDID much faster than 100kbit. I 
have several 4K DP monitors that deliver 256 bytes EDID in < 8ms. eDP 
panels deliver 128 bytes in <4 ms.
I also have seen devices that takes almost 50ms to deliver a 128 byte 
EDID block.

 From kms_sysfs_edid_timing to a Asus MG24UQ monitor:
card0-DP-3: mean.max 7758610.00ns, 7758.61us, 7.76ms, mean.avg 
7758610.00ns, 7758.61us, 7.76ms, edid_size 256, threshold 10

showing < 8ms to read 2 blocks of EDID.

 From kms_sysfs_edid_timing connected to a QD980B DP analyzer:
card0-DP-3: mean.max 74251273.00ns, 74251.27us, 74.25ms, mean.avg 
74251273.00ns, 74251.27us, 74.25ms, edid_size 256, threshold 10

showing > 74ms to read 2 blocks of EDID.

These times were confirmed with a Unigraf DPA400 to confirm both timing 
and that 256 bytes of data were transferred.

>
> That means 25ms per edid block and no special cases anywhere. I think you
> can still keep the 10ms for basic probe (but really shouldn't be needed).
>
> Note your limits are off by 2x, since you use 50ms per 128b edid block. We
> can totally read a CEA edid with 2 blocks in 50ms.
A single value like 25ms has been proven not to be enough time depending 
on the downstream device and connector type.
> -Daniel
>
>> + */
>> +bool lspcon_present;
>>   
>> +static int opt_handler(int opt, int opt_index, void *data)
>> +{
>> +	if (opt == 'l') {
>> +		lspcon_present = true;
>> +		igt_info("set LSPcon present on DP ports\n");
>> +	}
>>   
>> -igt_simple_main
>> +	return 0;
>> +}
>> +
>> +int main(int argc, char **argv)
>>   {
>>   	DIR *dirp;
>>   	struct dirent *de;
>> +	lspcon_present = false;
>> +
>> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
>> +				      opt_handler, NULL);
>> +
>> +	igt_skip_on_simulation();
>>   
>>   	dirp = opendir("/sys/class/drm");
>>   	igt_assert(dirp != NULL);
>> @@ -49,17 +74,34 @@ igt_simple_main
>>   		struct igt_mean mean = {};
>>   		struct stat st;
>>   		char path[PATH_MAX];
>> -		int i;
>> +		char edid_path[PATH_MAX];
>> +		char edid[512]; /* enough for 4 block edid */
>> +		unsigned long edid_size = 0;
>> +		int i, fd_edid;
>> +		unsigned int threshold = 0;
>>   
>>   		if (*de->d_name == '.')
>>   			continue;;
>>   
>>   		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
>>   				de->d_name);
>> +		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
>> +				de->d_name);
>>   
>>   		if (stat(path, &st))
>>   			continue;
>>   
>> +		fd_edid = open(edid_path, O_RDONLY);
>> +		if (fd_edid == -1) {
>> +			igt_warn("Read Error EDID\n");
>> +			continue;
>> +		}
>> +
>> +		edid_size = read(fd_edid, edid, 512);
>> +		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
>> +		if (lspcon_present || !strncmp(de->d_name, "card0-HDMI", 10))
>> +			threshold *= HDMI_THRESHOLD_MULTIPLIER;
>> +
>>   		igt_mean_init(&mean);
>>   		for (i = 0; i < CHECK_TIMES; i++) {
>>   			struct timespec ts = {};
>> @@ -76,22 +118,26 @@ igt_simple_main
>>   		}
>>   
>>   		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
>> -			  "mean.avg %.2fns, %.2fus, %.2fms\n",
>> +			  "mean.avg %.2fns, %.2fus, %.2fms, "
>> +			  "edid_size %lu, threshold %d\n",
>>   			  de->d_name,
>>   			  mean.max, mean.max / 1e3, mean.max / 1e6,
>> -			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
>> +			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
>> +			  edid_size, threshold);
>>   
>> -		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
>> +		if (edid_size == 0 &&
>> +		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
>>   			igt_warn("%s: probe time exceed 10ms, "
>>   				 "max=%.2fms, avg=%.2fms\n", de->d_name,
>>   				 mean.max / 1e6, mean.mean / 1e6);
>>   		}
>> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
>> -			     "%s: average probe time exceeded 50ms, "
>> -			     "max=%.2fms, avg=%.2fms\n", de->d_name,
>> +		if (edid_size > 0)
>> +			igt_assert_f(mean.mean < (threshold * 1e6),
>> +			     "%s: average probe time exceeded %dms, "
>> +			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
>>   			     mean.max / 1e6, mean.mean / 1e6);
>>   
>>   	}
>>   	closedir(dirp);
>> -
>> +	igt_exit();
>>   }
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Daniel Vetter]

On Mon, Aug 14, 2017 at 10:21:51AM -0700, Clint Taylor wrote:
> 
> 
> On 08/14/2017 07:40 AM, Daniel Vetter wrote:
> > On Thu, Aug 10, 2017 at 10:50:19AM -0700, clinton.a.taylor@intel.com wrote:
> > > From: Clint Taylor <clinton.a.taylor@intel.com>
> > > 
> > > Current 50ms max threshold timing for an EDID read is very close to the
> > > actual time for a 2 block HDMI EDID read. Adjust the timings base on
> > > connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
> > > is connected to device under test the -l option should be passed to update
> > > the threshold timing to allow the LSPcon to read the EDID at the HDMI
> > > timing. The -l option should be used when LSPcon is on the motherboard or
> > > if a USB_C->HDMI converter is present
> > > 
> > > V2: Adjust timings based on connector type, EDID size, and Add an option to
> > > specify if an LSPcon is present.
> > > V3: Add bugzilla to commit message
> > > V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> > > 
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > > ---
> > >   tests/kms_sysfs_edid_timing.c | 74 +++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 60 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> > > index 1201388..0382610 100644
> > > --- a/tests/kms_sysfs_edid_timing.c
> > > +++ b/tests/kms_sysfs_edid_timing.c
> > > @@ -26,21 +26,46 @@
> > >   #include <fcntl.h>
> > >   #include <sys/stat.h>
> > > -#define THRESHOLD_PER_CONNECTOR	10
> > > -#define THRESHOLD_TOTAL		50
> > > -#define CHECK_TIMES		15
> > > +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> > > +#define THRESHOLD_PER_EDID_BLOCK		5
> > > +#define HDMI_THRESHOLD_MULTIPLIER		10
> > > +#define CHECK_TIMES				10
> > >   IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
> > >   		     "the possible connectors. Without the edid -ENXIO patch "
> > >   		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> > > -		     "we sometimes take a *really* long time. "
> > > -		     "So let's just check for some reasonable timing here");
> > > +		     "we sometimes take a *really* long time. So let's check "
> > > +		     "an approximate time per edid block based on connector "
> > > +		     "type. The -l option adjusts DP timing to reflect HDMI read "
> > > +		     "timings from LSPcon.");
> > > +
> > > +/* The -l option has been added to correctly handle timings when an LSPcon is
> > > + * present. This could be on the platform itself or in a USB_C->HDMI converter.
> > > + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
> > > + * bus speed to the 100 Kbit HDMI DDC bus speed
> > This is blantantly wrong. EDID reads are done at 100kbit, even over dp
> > aux. There's some panels which have forgoe the i2c bus that
> > i2c-over-dp-aux drivers and givey ou the EDID a bit faster, but edids read
> > at 100kbit.
> 
> Actually most modern devices deliver DP EDID much faster than 100kbit. I
> have several 4K DP monitors that deliver 256 bytes EDID in < 8ms. eDP panels
> deliver 128 bytes in <4 ms.
> I also have seen devices that takes almost 50ms to deliver a 128 byte EDID
> block.
> 
> From kms_sysfs_edid_timing to a Asus MG24UQ monitor:
> card0-DP-3: mean.max 7758610.00ns, 7758.61us, 7.76ms, mean.avg 7758610.00ns,
> 7758.61us, 7.76ms, edid_size 256, threshold 10
> 
> showing < 8ms to read 2 blocks of EDID.

Ok, I think I've been proven wrong here. I guess this happens with fancy
new DP screens that drop the internal i2c bus and just emulate it all.

> From kms_sysfs_edid_timing connected to a QD980B DP analyzer:
> card0-DP-3: mean.max 74251273.00ns, 74251.27us, 74.25ms, mean.avg
> 74251273.00ns, 74251.27us, 74.25ms, edid_size 256, threshold 10
> 
> showing > 74ms to read 2 blocks of EDID.

Hm, DP analyzer ... Does this also happen with real hw? If it's just DP
analyzer I think we still should just shrug it off.

> These times were confirmed with a Unigraf DPA400 to confirm both timing and
> that 256 bytes of data were transferred.
> 
> > 
> > That means 25ms per edid block and no special cases anywhere. I think you
> > can still keep the 10ms for basic probe (but really shouldn't be needed).
> > 
> > Note your limits are off by 2x, since you use 50ms per 128b edid block. We
> > can totally read a CEA edid with 2 blocks in 50ms.
> A single value like 25ms has been proven not to be enough time depending on
> the downstream device and connector type.

Besides the DP analyzer, what else requires more than 25ms per EDID block?
-Daniel

> > -Daniel
> > 
> > > + */
> > > +bool lspcon_present;
> > > +static int opt_handler(int opt, int opt_index, void *data)
> > > +{
> > > +	if (opt == 'l') {
> > > +		lspcon_present = true;
> > > +		igt_info("set LSPcon present on DP ports\n");
> > > +	}
> > > -igt_simple_main
> > > +	return 0;
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > >   {
> > >   	DIR *dirp;
> > >   	struct dirent *de;
> > > +	lspcon_present = false;
> > > +
> > > +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> > > +				      opt_handler, NULL);
> > > +
> > > +	igt_skip_on_simulation();
> > >   	dirp = opendir("/sys/class/drm");
> > >   	igt_assert(dirp != NULL);
> > > @@ -49,17 +74,34 @@ igt_simple_main
> > >   		struct igt_mean mean = {};
> > >   		struct stat st;
> > >   		char path[PATH_MAX];
> > > -		int i;
> > > +		char edid_path[PATH_MAX];
> > > +		char edid[512]; /* enough for 4 block edid */
> > > +		unsigned long edid_size = 0;
> > > +		int i, fd_edid;
> > > +		unsigned int threshold = 0;
> > >   		if (*de->d_name == '.')
> > >   			continue;;
> > >   		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
> > >   				de->d_name);
> > > +		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
> > > +				de->d_name);
> > >   		if (stat(path, &st))
> > >   			continue;
> > > +		fd_edid = open(edid_path, O_RDONLY);
> > > +		if (fd_edid == -1) {
> > > +			igt_warn("Read Error EDID\n");
> > > +			continue;
> > > +		}
> > > +
> > > +		edid_size = read(fd_edid, edid, 512);
> > > +		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
> > > +		if (lspcon_present || !strncmp(de->d_name, "card0-HDMI", 10))
> > > +			threshold *= HDMI_THRESHOLD_MULTIPLIER;
> > > +
> > >   		igt_mean_init(&mean);
> > >   		for (i = 0; i < CHECK_TIMES; i++) {
> > >   			struct timespec ts = {};
> > > @@ -76,22 +118,26 @@ igt_simple_main
> > >   		}
> > >   		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
> > > -			  "mean.avg %.2fns, %.2fus, %.2fms\n",
> > > +			  "mean.avg %.2fns, %.2fus, %.2fms, "
> > > +			  "edid_size %lu, threshold %d\n",
> > >   			  de->d_name,
> > >   			  mean.max, mean.max / 1e3, mean.max / 1e6,
> > > -			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
> > > +			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
> > > +			  edid_size, threshold);
> > > -		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
> > > +		if (edid_size == 0 &&
> > > +		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
> > >   			igt_warn("%s: probe time exceed 10ms, "
> > >   				 "max=%.2fms, avg=%.2fms\n", de->d_name,
> > >   				 mean.max / 1e6, mean.mean / 1e6);
> > >   		}
> > > -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
> > > -			     "%s: average probe time exceeded 50ms, "
> > > -			     "max=%.2fms, avg=%.2fms\n", de->d_name,
> > > +		if (edid_size > 0)
> > > +			igt_assert_f(mean.mean < (threshold * 1e6),
> > > +			     "%s: average probe time exceeded %dms, "
> > > +			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
> > >   			     mean.max / 1e6, mean.mean / 1e6);
> > >   	}
> > >   	closedir(dirp);
> > > -
> > > +	igt_exit();
> > >   }
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Clint Taylor]

On 08/15/2017 12:58 AM, Daniel Vetter wrote:
> On Mon, Aug 14, 2017 at 10:21:51AM -0700, Clint Taylor wrote:
>>
>> On 08/14/2017 07:40 AM, Daniel Vetter wrote:
>>> On Thu, Aug 10, 2017 at 10:50:19AM -0700, clinton.a.taylor@intel.com wrote:
>>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>>>
>>>> Current 50ms max threshold timing for an EDID read is very close to the
>>>> actual time for a 2 block HDMI EDID read. Adjust the timings base on
>>>> connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
>>>> is connected to device under test the -l option should be passed to update
>>>> the threshold timing to allow the LSPcon to read the EDID at the HDMI
>>>> timing. The -l option should be used when LSPcon is on the motherboard or
>>>> if a USB_C->HDMI converter is present
>>>>
>>>> V2: Adjust timings based on connector type, EDID size, and Add an option to
>>>> specify if an LSPcon is present.
>>>> V3: Add bugzilla to commit message
>>>> V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>>>>
>>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
>>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>>> ---
>>>>    tests/kms_sysfs_edid_timing.c | 74 +++++++++++++++++++++++++++++++++++--------
>>>>    1 file changed, 60 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
>>>> index 1201388..0382610 100644
>>>> --- a/tests/kms_sysfs_edid_timing.c
>>>> +++ b/tests/kms_sysfs_edid_timing.c
>>>> @@ -26,21 +26,46 @@
>>>>    #include <fcntl.h>
>>>>    #include <sys/stat.h>
>>>> -#define THRESHOLD_PER_CONNECTOR	10
>>>> -#define THRESHOLD_TOTAL		50
>>>> -#define CHECK_TIMES		15
>>>> +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
>>>> +#define THRESHOLD_PER_EDID_BLOCK		5
>>>> +#define HDMI_THRESHOLD_MULTIPLIER		10
>>>> +#define CHECK_TIMES				10
>>>>    IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
>>>>    		     "the possible connectors. Without the edid -ENXIO patch "
>>>>    		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
>>>> -		     "we sometimes take a *really* long time. "
>>>> -		     "So let's just check for some reasonable timing here");
>>>> +		     "we sometimes take a *really* long time. So let's check "
>>>> +		     "an approximate time per edid block based on connector "
>>>> +		     "type. The -l option adjusts DP timing to reflect HDMI read "
>>>> +		     "timings from LSPcon.");
>>>> +
>>>> +/* The -l option has been added to correctly handle timings when an LSPcon is
>>>> + * present. This could be on the platform itself or in a USB_C->HDMI converter.
>>>> + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
>>>> + * bus speed to the 100 Kbit HDMI DDC bus speed
>>> This is blantantly wrong. EDID reads are done at 100kbit, even over dp
>>> aux. There's some panels which have forgoe the i2c bus that
>>> i2c-over-dp-aux drivers and givey ou the EDID a bit faster, but edids read
>>> at 100kbit.
>> Actually most modern devices deliver DP EDID much faster than 100kbit. I
>> have several 4K DP monitors that deliver 256 bytes EDID in < 8ms. eDP panels
>> deliver 128 bytes in <4 ms.
>> I also have seen devices that takes almost 50ms to deliver a 128 byte EDID
>> block.
>>
>>  From kms_sysfs_edid_timing to a Asus MG24UQ monitor:
>> card0-DP-3: mean.max 7758610.00ns, 7758.61us, 7.76ms, mean.avg 7758610.00ns,
>> 7758.61us, 7.76ms, edid_size 256, threshold 10
>>
>> showing < 8ms to read 2 blocks of EDID.
> Ok, I think I've been proven wrong here. I guess this happens with fancy
> new DP screens that drop the internal i2c bus and just emulate it all.
>
>>  From kms_sysfs_edid_timing connected to a QD980B DP analyzer:
>> card0-DP-3: mean.max 74251273.00ns, 74251.27us, 74.25ms, mean.avg
>> 74251273.00ns, 74251.27us, 74.25ms, edid_size 256, threshold 10
>>
>> showing > 74ms to read 2 blocks of EDID.
> Hm, DP analyzer ... Does this also happen with real hw? If it's just DP
> analyzer I think we still should just shrug it off.

Validation has a DVI monitor connected to a BDW NUC in CI that takes 
49.77ms to read a 128 Byte EDID. This is the reason V4 was submitted 
with the threshold time doubled to 50ms.

-Clint

>
>> These times were confirmed with a Unigraf DPA400 to confirm both timing and
>> that 256 bytes of data were transferred.
>>
>>> That means 25ms per edid block and no special cases anywhere. I think you
>>> can still keep the 10ms for basic probe (but really shouldn't be needed).
>>>
>>> Note your limits are off by 2x, since you use 50ms per 128b edid block. We
>>> can totally read a CEA edid with 2 blocks in 50ms.
>> A single value like 25ms has been proven not to be enough time depending on
>> the downstream device and connector type.
> Besides the DP analyzer, what else requires more than 25ms per EDID block?
> -Daniel
>
>>> -Daniel
>>>
>>>> + */
>>>> +bool lspcon_present;
>>>> +static int opt_handler(int opt, int opt_index, void *data)
>>>> +{
>>>> +	if (opt == 'l') {
>>>> +		lspcon_present = true;
>>>> +		igt_info("set LSPcon present on DP ports\n");
>>>> +	}
>>>> -igt_simple_main
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int main(int argc, char **argv)
>>>>    {
>>>>    	DIR *dirp;
>>>>    	struct dirent *de;
>>>> +	lspcon_present = false;
>>>> +
>>>> +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
>>>> +				      opt_handler, NULL);
>>>> +
>>>> +	igt_skip_on_simulation();
>>>>    	dirp = opendir("/sys/class/drm");
>>>>    	igt_assert(dirp != NULL);
>>>> @@ -49,17 +74,34 @@ igt_simple_main
>>>>    		struct igt_mean mean = {};
>>>>    		struct stat st;
>>>>    		char path[PATH_MAX];
>>>> -		int i;
>>>> +		char edid_path[PATH_MAX];
>>>> +		char edid[512]; /* enough for 4 block edid */
>>>> +		unsigned long edid_size = 0;
>>>> +		int i, fd_edid;
>>>> +		unsigned int threshold = 0;
>>>>    		if (*de->d_name == '.')
>>>>    			continue;;
>>>>    		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
>>>>    				de->d_name);
>>>> +		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
>>>> +				de->d_name);
>>>>    		if (stat(path, &st))
>>>>    			continue;
>>>> +		fd_edid = open(edid_path, O_RDONLY);
>>>> +		if (fd_edid == -1) {
>>>> +			igt_warn("Read Error EDID\n");
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		edid_size = read(fd_edid, edid, 512);
>>>> +		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
>>>> +		if (lspcon_present || !strncmp(de->d_name, "card0-HDMI", 10))
>>>> +			threshold *= HDMI_THRESHOLD_MULTIPLIER;
>>>> +
>>>>    		igt_mean_init(&mean);
>>>>    		for (i = 0; i < CHECK_TIMES; i++) {
>>>>    			struct timespec ts = {};
>>>> @@ -76,22 +118,26 @@ igt_simple_main
>>>>    		}
>>>>    		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
>>>> -			  "mean.avg %.2fns, %.2fus, %.2fms\n",
>>>> +			  "mean.avg %.2fns, %.2fus, %.2fms, "
>>>> +			  "edid_size %lu, threshold %d\n",
>>>>    			  de->d_name,
>>>>    			  mean.max, mean.max / 1e3, mean.max / 1e6,
>>>> -			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
>>>> +			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
>>>> +			  edid_size, threshold);
>>>> -		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
>>>> +		if (edid_size == 0 &&
>>>> +		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
>>>>    			igt_warn("%s: probe time exceed 10ms, "
>>>>    				 "max=%.2fms, avg=%.2fms\n", de->d_name,
>>>>    				 mean.max / 1e6, mean.mean / 1e6);
>>>>    		}
>>>> -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
>>>> -			     "%s: average probe time exceeded 50ms, "
>>>> -			     "max=%.2fms, avg=%.2fms\n", de->d_name,
>>>> +		if (edid_size > 0)
>>>> +			igt_assert_f(mean.mean < (threshold * 1e6),
>>>> +			     "%s: average probe time exceeded %dms, "
>>>> +			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
>>>>    			     mean.max / 1e6, mean.mean / 1e6);
>>>>    	}
>>>>    	closedir(dirp);
>>>> -
>>>> +	igt_exit();
>>>>    }
>>>> -- 
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Daniel Vetter]

On Tue, Aug 15, 2017 at 11:04:21AM -0700, Clint Taylor wrote:
> 
> 
> On 08/15/2017 12:58 AM, Daniel Vetter wrote:
> > On Mon, Aug 14, 2017 at 10:21:51AM -0700, Clint Taylor wrote:
> > > 
> > > On 08/14/2017 07:40 AM, Daniel Vetter wrote:
> > > > On Thu, Aug 10, 2017 at 10:50:19AM -0700, clinton.a.taylor@intel.com wrote:
> > > > > From: Clint Taylor <clinton.a.taylor@intel.com>
> > > > > 
> > > > > Current 50ms max threshold timing for an EDID read is very close to the
> > > > > actual time for a 2 block HDMI EDID read. Adjust the timings base on
> > > > > connector type as DP reads are at 1 MBit and HDMI at 100K bit. If an LSPcon
> > > > > is connected to device under test the -l option should be passed to update
> > > > > the threshold timing to allow the LSPcon to read the EDID at the HDMI
> > > > > timing. The -l option should be used when LSPcon is on the motherboard or
> > > > > if a USB_C->HDMI converter is present
> > > > > 
> > > > > V2: Adjust timings based on connector type, EDID size, and Add an option to
> > > > > specify if an LSPcon is present.
> > > > > V3: Add bugzilla to commit message
> > > > > V4: remove edid_size check from HDMI multiplier. Fixes DVI on HDMI.
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> > > > > 
> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > > > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > > > > ---
> > > > >    tests/kms_sysfs_edid_timing.c | 74 +++++++++++++++++++++++++++++++++++--------
> > > > >    1 file changed, 60 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> > > > > index 1201388..0382610 100644
> > > > > --- a/tests/kms_sysfs_edid_timing.c
> > > > > +++ b/tests/kms_sysfs_edid_timing.c
> > > > > @@ -26,21 +26,46 @@
> > > > >    #include <fcntl.h>
> > > > >    #include <sys/stat.h>
> > > > > -#define THRESHOLD_PER_CONNECTOR	10
> > > > > -#define THRESHOLD_TOTAL		50
> > > > > -#define CHECK_TIMES		15
> > > > > +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> > > > > +#define THRESHOLD_PER_EDID_BLOCK		5
> > > > > +#define HDMI_THRESHOLD_MULTIPLIER		10
> > > > > +#define CHECK_TIMES				10
> > > > >    IGT_TEST_DESCRIPTION("This check the time we take to read the content of all "
> > > > >    		     "the possible connectors. Without the edid -ENXIO patch "
> > > > >    		     "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> > > > > -		     "we sometimes take a *really* long time. "
> > > > > -		     "So let's just check for some reasonable timing here");
> > > > > +		     "we sometimes take a *really* long time. So let's check "
> > > > > +		     "an approximate time per edid block based on connector "
> > > > > +		     "type. The -l option adjusts DP timing to reflect HDMI read "
> > > > > +		     "timings from LSPcon.");
> > > > > +
> > > > > +/* The -l option has been added to correctly handle timings when an LSPcon is
> > > > > + * present. This could be on the platform itself or in a USB_C->HDMI converter.
> > > > > + * With LSPCon EDID read timing will need to change from the 1 Mbit AUX
> > > > > + * bus speed to the 100 Kbit HDMI DDC bus speed
> > > > This is blantantly wrong. EDID reads are done at 100kbit, even over dp
> > > > aux. There's some panels which have forgoe the i2c bus that
> > > > i2c-over-dp-aux drivers and givey ou the EDID a bit faster, but edids read
> > > > at 100kbit.
> > > Actually most modern devices deliver DP EDID much faster than 100kbit. I
> > > have several 4K DP monitors that deliver 256 bytes EDID in < 8ms. eDP panels
> > > deliver 128 bytes in <4 ms.
> > > I also have seen devices that takes almost 50ms to deliver a 128 byte EDID
> > > block.
> > > 
> > >  From kms_sysfs_edid_timing to a Asus MG24UQ monitor:
> > > card0-DP-3: mean.max 7758610.00ns, 7758.61us, 7.76ms, mean.avg 7758610.00ns,
> > > 7758.61us, 7.76ms, edid_size 256, threshold 10
> > > 
> > > showing < 8ms to read 2 blocks of EDID.
> > Ok, I think I've been proven wrong here. I guess this happens with fancy
> > new DP screens that drop the internal i2c bus and just emulate it all.
> > 
> > >  From kms_sysfs_edid_timing connected to a QD980B DP analyzer:
> > > card0-DP-3: mean.max 74251273.00ns, 74251.27us, 74.25ms, mean.avg
> > > 74251273.00ns, 74251.27us, 74.25ms, edid_size 256, threshold 10
> > > 
> > > showing > 74ms to read 2 blocks of EDID.
> > Hm, DP analyzer ... Does this also happen with real hw? If it's just DP
> > analyzer I think we still should just shrug it off.
> 
> Validation has a DVI monitor connected to a BDW NUC in CI that takes 49.77ms
> to read a 128 Byte EDID. This is the reason V4 was submitted with the
> threshold time doubled to 50ms.

Hm ... this sucks, because iirc the bugs we've had didn't do more than a
2x increase in time for reading edids.

We do have some other igts which are essentially benchmarks, but CI isn't
quite set up yet to track these. Probably best if you start a discussion
with the CI team how we could handle this, but for now I now agree with
Jani that adjusting the time limit doesn't seem to leave us with a
testcase that's useful.
-Daniel
> 
> -Clint
> 
> > 
> > > These times were confirmed with a Unigraf DPA400 to confirm both timing and
> > > that 256 bytes of data were transferred.
> > > 
> > > > That means 25ms per edid block and no special cases anywhere. I think you
> > > > can still keep the 10ms for basic probe (but really shouldn't be needed).
> > > > 
> > > > Note your limits are off by 2x, since you use 50ms per 128b edid block. We
> > > > can totally read a CEA edid with 2 blocks in 50ms.
> > > A single value like 25ms has been proven not to be enough time depending on
> > > the downstream device and connector type.
> > Besides the DP analyzer, what else requires more than 25ms per EDID block?
> > -Daniel
> > 
> > > > -Daniel
> > > > 
> > > > > + */
> > > > > +bool lspcon_present;
> > > > > +static int opt_handler(int opt, int opt_index, void *data)
> > > > > +{
> > > > > +	if (opt == 'l') {
> > > > > +		lspcon_present = true;
> > > > > +		igt_info("set LSPcon present on DP ports\n");
> > > > > +	}
> > > > > -igt_simple_main
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int main(int argc, char **argv)
> > > > >    {
> > > > >    	DIR *dirp;
> > > > >    	struct dirent *de;
> > > > > +	lspcon_present = false;
> > > > > +
> > > > > +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> > > > > +				      opt_handler, NULL);
> > > > > +
> > > > > +	igt_skip_on_simulation();
> > > > >    	dirp = opendir("/sys/class/drm");
> > > > >    	igt_assert(dirp != NULL);
> > > > > @@ -49,17 +74,34 @@ igt_simple_main
> > > > >    		struct igt_mean mean = {};
> > > > >    		struct stat st;
> > > > >    		char path[PATH_MAX];
> > > > > -		int i;
> > > > > +		char edid_path[PATH_MAX];
> > > > > +		char edid[512]; /* enough for 4 block edid */
> > > > > +		unsigned long edid_size = 0;
> > > > > +		int i, fd_edid;
> > > > > +		unsigned int threshold = 0;
> > > > >    		if (*de->d_name == '.')
> > > > >    			continue;;
> > > > >    		snprintf(path, sizeof(path), "/sys/class/drm/%s/status",
> > > > >    				de->d_name);
> > > > > +		snprintf(edid_path, sizeof(edid_path), "/sys/class/drm/%s/edid",
> > > > > +				de->d_name);
> > > > >    		if (stat(path, &st))
> > > > >    			continue;
> > > > > +		fd_edid = open(edid_path, O_RDONLY);
> > > > > +		if (fd_edid == -1) {
> > > > > +			igt_warn("Read Error EDID\n");
> > > > > +			continue;
> > > > > +		}
> > > > > +
> > > > > +		edid_size = read(fd_edid, edid, 512);
> > > > > +		threshold = THRESHOLD_PER_EDID_BLOCK * (edid_size / 128);
> > > > > +		if (lspcon_present || !strncmp(de->d_name, "card0-HDMI", 10))
> > > > > +			threshold *= HDMI_THRESHOLD_MULTIPLIER;
> > > > > +
> > > > >    		igt_mean_init(&mean);
> > > > >    		for (i = 0; i < CHECK_TIMES; i++) {
> > > > >    			struct timespec ts = {};
> > > > > @@ -76,22 +118,26 @@ igt_simple_main
> > > > >    		}
> > > > >    		igt_debug("%s: mean.max %.2fns, %.2fus, %.2fms, "
> > > > > -			  "mean.avg %.2fns, %.2fus, %.2fms\n",
> > > > > +			  "mean.avg %.2fns, %.2fus, %.2fms, "
> > > > > +			  "edid_size %lu, threshold %d\n",
> > > > >    			  de->d_name,
> > > > >    			  mean.max, mean.max / 1e3, mean.max / 1e6,
> > > > > -			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
> > > > > +			  mean.mean, mean.mean / 1e3, mean.mean / 1e6,
> > > > > +			  edid_size, threshold);
> > > > > -		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
> > > > > +		if (edid_size == 0 &&
> > > > > +		   (mean.max > (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
> > > > >    			igt_warn("%s: probe time exceed 10ms, "
> > > > >    				 "max=%.2fms, avg=%.2fms\n", de->d_name,
> > > > >    				 mean.max / 1e6, mean.mean / 1e6);
> > > > >    		}
> > > > > -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL * 1e6),
> > > > > -			     "%s: average probe time exceeded 50ms, "
> > > > > -			     "max=%.2fms, avg=%.2fms\n", de->d_name,
> > > > > +		if (edid_size > 0)
> > > > > +			igt_assert_f(mean.mean < (threshold * 1e6),
> > > > > +			     "%s: average probe time exceeded %dms, "
> > > > > +			     "max=%.2fms, avg=%.2fms\n", de->d_name, threshold,
> > > > >    			     mean.max / 1e6, mean.mean / 1e6);
> > > > >    	}
> > > > >    	closedir(dirp);
> > > > > -
> > > > > +	igt_exit();
> > > > >    }
> > > > > -- 
> > > > > 1.9.1
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read

[Lofstedt, Marta]

This is still an issue, can we please come to some consensus.
My position is to take the V1.

/Marta

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Friday, August 18, 2017 10:59 AM
> To: Taylor, Clinton A <clinton.a.taylor@intel.com>
> Cc: Vetter, Daniel <daniel.vetter@intel.com>; intel-
> gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v4 i-g-t] tests/kms: increase max threshold
> time for edid read
> 
> On Tue, Aug 15, 2017 at 11:04:21AM -0700, Clint Taylor wrote:
> >
> >
> > On 08/15/2017 12:58 AM, Daniel Vetter wrote:
> > > On Mon, Aug 14, 2017 at 10:21:51AM -0700, Clint Taylor wrote:
> > > >
> > > > On 08/14/2017 07:40 AM, Daniel Vetter wrote:
> > > > > On Thu, Aug 10, 2017 at 10:50:19AM -0700, clinton.a.taylor@intel.com
> wrote:
> > > > > > From: Clint Taylor <clinton.a.taylor@intel.com>
> > > > > >
> > > > > > Current 50ms max threshold timing for an EDID read is very
> > > > > > close to the actual time for a 2 block HDMI EDID read. Adjust
> > > > > > the timings base on connector type as DP reads are at 1 MBit
> > > > > > and HDMI at 100K bit. If an LSPcon is connected to device
> > > > > > under test the -l option should be passed to update the
> > > > > > threshold timing to allow the LSPcon to read the EDID at the
> > > > > > HDMI timing. The -l option should be used when LSPcon is on
> > > > > > the motherboard or if a USB_C->HDMI converter is present
> > > > > >
> > > > > > V2: Adjust timings based on connector type, EDID size, and Add
> > > > > > an option to specify if an LSPcon is present.
> > > > > > V3: Add bugzilla to commit message
> > > > > > V4: remove edid_size check from HDMI multiplier. Fixes DVI on
> HDMI.
> > > > > >
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100047
> > > > > >
> > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > > > > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > > > > > ---
> > > > > >    tests/kms_sysfs_edid_timing.c | 74
> +++++++++++++++++++++++++++++++++++--------
> > > > > >    1 file changed, 60 insertions(+), 14 deletions(-)
> > > > > >
> > > > > > diff --git a/tests/kms_sysfs_edid_timing.c
> > > > > > b/tests/kms_sysfs_edid_timing.c index 1201388..0382610 100644
> > > > > > --- a/tests/kms_sysfs_edid_timing.c
> > > > > > +++ b/tests/kms_sysfs_edid_timing.c
> > > > > > @@ -26,21 +26,46 @@
> > > > > >    #include <fcntl.h>
> > > > > >    #include <sys/stat.h>
> > > > > > -#define THRESHOLD_PER_CONNECTOR	10
> > > > > > -#define THRESHOLD_TOTAL		50
> > > > > > -#define CHECK_TIMES		15
> > > > > > +#define THRESHOLD_FOR_EMPTY_CONNECTOR	10
> > > > > > +#define THRESHOLD_PER_EDID_BLOCK		5
> > > > > > +#define HDMI_THRESHOLD_MULTIPLIER		10
> > > > > > +#define CHECK_TIMES
> 	10
> > > > > >    IGT_TEST_DESCRIPTION("This check the time we take to read the
> content of all "
> > > > > >    		     "the possible connectors. Without the edid -
> ENXIO patch "
> > > > > >
> "(http://permalink.gmane.org/gmane.comp.video.dri.devel/62083), "
> > > > > > -		     "we sometimes take a *really* long time. "
> > > > > > -		     "So let's just check for some reasonable
> timing here");
> > > > > > +		     "we sometimes take a *really* long time. So
> let's check "
> > > > > > +		     "an approximate time per edid block based on
> connector "
> > > > > > +		     "type. The -l option adjusts DP timing to
> reflect HDMI read "
> > > > > > +		     "timings from LSPcon.");
> > > > > > +
> > > > > > +/* The -l option has been added to correctly handle timings
> > > > > > +when an LSPcon is
> > > > > > + * present. This could be on the platform itself or in a USB_C-
> >HDMI converter.
> > > > > > + * With LSPCon EDID read timing will need to change from the
> > > > > > +1 Mbit AUX
> > > > > > + * bus speed to the 100 Kbit HDMI DDC bus speed
> > > > > This is blantantly wrong. EDID reads are done at 100kbit, even
> > > > > over dp aux. There's some panels which have forgoe the i2c bus
> > > > > that i2c-over-dp-aux drivers and givey ou the EDID a bit faster,
> > > > > but edids read at 100kbit.
> > > > Actually most modern devices deliver DP EDID much faster than
> > > > 100kbit. I have several 4K DP monitors that deliver 256 bytes EDID
> > > > in < 8ms. eDP panels deliver 128 bytes in <4 ms.
> > > > I also have seen devices that takes almost 50ms to deliver a 128
> > > > byte EDID block.
> > > >
> > > >  From kms_sysfs_edid_timing to a Asus MG24UQ monitor:
> > > > card0-DP-3: mean.max 7758610.00ns, 7758.61us, 7.76ms, mean.avg
> > > > 7758610.00ns, 7758.61us, 7.76ms, edid_size 256, threshold 10
> > > >
> > > > showing < 8ms to read 2 blocks of EDID.
> > > Ok, I think I've been proven wrong here. I guess this happens with
> > > fancy new DP screens that drop the internal i2c bus and just emulate it all.
> > >
> > > >  From kms_sysfs_edid_timing connected to a QD980B DP analyzer:
> > > > card0-DP-3: mean.max 74251273.00ns, 74251.27us, 74.25ms, mean.avg
> > > > 74251273.00ns, 74251.27us, 74.25ms, edid_size 256, threshold 10
> > > >
> > > > showing > 74ms to read 2 blocks of EDID.
> > > Hm, DP analyzer ... Does this also happen with real hw? If it's just
> > > DP analyzer I think we still should just shrug it off.
> >
> > Validation has a DVI monitor connected to a BDW NUC in CI that takes
> > 49.77ms to read a 128 Byte EDID. This is the reason V4 was submitted
> > with the threshold time doubled to 50ms.
> 
> Hm ... this sucks, because iirc the bugs we've had didn't do more than a 2x
> increase in time for reading edids.
> 
> We do have some other igts which are essentially benchmarks, but CI isn't
> quite set up yet to track these. Probably best if you start a discussion with
> the CI team how we could handle this, but for now I now agree with Jani that
> adjusting the time limit doesn't seem to leave us with a testcase that's
> useful.
> -Daniel
> >
> > -Clint
> >
> > >
> > > > These times were confirmed with a Unigraf DPA400 to confirm both
> > > > timing and that 256 bytes of data were transferred.
> > > >
> > > > > That means 25ms per edid block and no special cases anywhere. I
> > > > > think you can still keep the 10ms for basic probe (but really shouldn't
> be needed).
> > > > >
> > > > > Note your limits are off by 2x, since you use 50ms per 128b edid
> > > > > block. We can totally read a CEA edid with 2 blocks in 50ms.
> > > > A single value like 25ms has been proven not to be enough time
> > > > depending on the downstream device and connector type.
> > > Besides the DP analyzer, what else requires more than 25ms per EDID
> block?
> > > -Daniel
> > >
> > > > > -Daniel
> > > > >
> > > > > > + */
> > > > > > +bool lspcon_present;
> > > > > > +static int opt_handler(int opt, int opt_index, void *data) {
> > > > > > +	if (opt == 'l') {
> > > > > > +		lspcon_present = true;
> > > > > > +		igt_info("set LSPcon present on DP ports\n");
> > > > > > +	}
> > > > > > -igt_simple_main
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +int main(int argc, char **argv)
> > > > > >    {
> > > > > >    	DIR *dirp;
> > > > > >    	struct dirent *de;
> > > > > > +	lspcon_present = false;
> > > > > > +
> > > > > > +	igt_simple_init_parse_opts(&argc, argv, "l", NULL, NULL,
> > > > > > +				      opt_handler,
> NULL);
> > > > > > +
> > > > > > +	igt_skip_on_simulation();
> > > > > >    	dirp = opendir("/sys/class/drm");
> > > > > >    	igt_assert(dirp != NULL);
> > > > > > @@ -49,17 +74,34 @@ igt_simple_main
> > > > > >    		struct igt_mean mean = {};
> > > > > >    		struct stat st;
> > > > > >    		char path[PATH_MAX];
> > > > > > -		int i;
> > > > > > +		char edid_path[PATH_MAX];
> > > > > > +		char edid[512]; /* enough for 4 block edid */
> > > > > > +		unsigned long edid_size = 0;
> > > > > > +		int i, fd_edid;
> > > > > > +		unsigned int threshold = 0;
> > > > > >    		if (*de->d_name == '.')
> > > > > >    			continue;;
> > > > > >    		snprintf(path, sizeof(path),
> "/sys/class/drm/%s/status",
> > > > > >    				de->d_name);
> > > > > > +		snprintf(edid_path, sizeof(edid_path),
> "/sys/class/drm/%s/edid",
> > > > > > +				de->d_name);
> > > > > >    		if (stat(path, &st))
> > > > > >    			continue;
> > > > > > +		fd_edid = open(edid_path, O_RDONLY);
> > > > > > +		if (fd_edid == -1) {
> > > > > > +			igt_warn("Read Error EDID\n");
> > > > > > +			continue;
> > > > > > +		}
> > > > > > +
> > > > > > +		edid_size = read(fd_edid, edid, 512);
> > > > > > +		threshold = THRESHOLD_PER_EDID_BLOCK *
> (edid_size / 128);
> > > > > > +		if (lspcon_present || !strncmp(de->d_name,
> "card0-HDMI", 10))
> > > > > > +			threshold *=
> HDMI_THRESHOLD_MULTIPLIER;
> > > > > > +
> > > > > >    		igt_mean_init(&mean);
> > > > > >    		for (i = 0; i < CHECK_TIMES; i++) {
> > > > > >    			struct timespec ts = {}; @@ -76,22
> +118,26 @@
> > > > > > igt_simple_main
> > > > > >    		}
> > > > > >    		igt_debug("%s: mean.max %.2fns, %.2fus,
> %.2fms, "
> > > > > > -			  "mean.avg %.2fns, %.2fus,
> %.2fms\n",
> > > > > > +			  "mean.avg %.2fns, %.2fus,
> %.2fms, "
> > > > > > +			  "edid_size %lu, threshold %d\n",
> > > > > >    			  de->d_name,
> > > > > >    			  mean.max, mean.max / 1e3,
> mean.max / 1e6,
> > > > > > -			  mean.mean, mean.mean / 1e3,
> mean.mean / 1e6);
> > > > > > +			  mean.mean, mean.mean / 1e3,
> mean.mean / 1e6,
> > > > > > +			  edid_size, threshold);
> > > > > > -		if (mean.max > (THRESHOLD_PER_CONNECTOR
> * 1e6)) {
> > > > > > +		if (edid_size == 0 &&
> > > > > > +		   (mean.max >
> (THRESHOLD_FOR_EMPTY_CONNECTOR * 1e6))) {
> > > > > >    			igt_warn("%s: probe time exceed
> 10ms, "
> > > > > >    				 "max=%.2fms,
> avg=%.2fms\n", de->d_name,
> > > > > >    				 mean.max / 1e6,
> mean.mean / 1e6);
> > > > > >    		}
> > > > > > -		igt_assert_f(mean.mean < (THRESHOLD_TOTAL
> * 1e6),
> > > > > > -			     "%s: average probe time
> exceeded 50ms, "
> > > > > > -			     "max=%.2fms, avg=%.2fms\n",
> de->d_name,
> > > > > > +		if (edid_size > 0)
> > > > > > +			igt_assert_f(mean.mean <
> (threshold * 1e6),
> > > > > > +			     "%s: average probe time
> exceeded %dms, "
> > > > > > +			     "max=%.2fms, avg=%.2fms\n",
> de->d_name, threshold,
> > > > > >    			     mean.max / 1e6, mean.mean /
> 1e6);
> > > > > >    	}
> > > > > >    	closedir(dirp);
> > > > > > -
> > > > > > +	igt_exit();
> > > > > >    }
> > > > > > --
> > > > > > 1.9.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx