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@xxxxxxxxxxxxxxxxxxxxx] On Behalf > Of Daniel Vetter > Sent: Friday, August 18, 2017 10:59 AM > To: Taylor, Clinton A <clinton.a.taylor@xxxxxxxxx> > Cc: Vetter, Daniel <daniel.vetter@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxx > wrote: > > > > > > 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 > > > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
