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