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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

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

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux