[Intel-gfx] [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read
Daniel Vetter
daniel at ffwll.ch
Fri Aug 18 07:59:04 UTC 2017
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 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.
>
> 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 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