[Intel-gfx] [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read
Lofstedt, Marta
marta.lofstedt at intel.com
Mon Oct 9 07:05:23 UTC 2017
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 at lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Friday, August 18, 2017 10:59 AM
> To: Taylor, Clinton A <clinton.a.taylor at intel.com>
> Cc: Vetter, Daniel <daniel.vetter at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list