[Intel-gfx] [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read
Daniel Vetter
daniel at ffwll.ch
Tue Aug 15 07:58:20 UTC 2017
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 at intel.com wrote:
> > > From: Clint Taylor <clinton.a.taylor at 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 at intel.com>
> > > Cc: Marta Lofstedt <marta.lofstedt at intel.com>
> > > Signed-off-by: Clint Taylor <clinton.a.taylor at 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list