[v4] tests/kms: increase max threshold time for edid read

Submitted by Clinton Taylor on Aug. 10, 2017, 5:50 p.m.

Details

Message ID 1502387419-10094-1-git-send-email-clinton.a.taylor@intel.com
State New
Headers show
Series "tests/kms: increase max threshold time for edid read" ( rev: 5 ) in IGT (deprecated)

Browsing this patch as part of:
"tests/kms: increase max threshold time for edid read" rev 5 in IGT (deprecated)
<< prev patch [1/1] next patch >>

Commit Message

Clinton Taylor Aug. 10, 2017, 5:50 p.m.
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(-)

Patch hide | download patch | download mbox

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();
 }

Comments

> -----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
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
> -----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
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.
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
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
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.
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
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
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
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
>
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
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
>
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