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