[Intel-gfx] [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid read
Clint Taylor
clinton.a.taylor at intel.com
Fri Aug 11 16:36:12 UTC 2017
On 08/11/2017 12:49 AM, Lofstedt, Marta wrote:
>
>> -----Original Message-----
>> From: Taylor, Clinton A
>> Sent: Thursday, August 10, 2017 8:50 PM
>> To: intel-gfx at lists.freedesktop.org
>> Cc: Taylor, Clinton A <clinton.a.taylor at intel.com>; Vetter, Daniel
>> <daniel.vetter at intel.com>; Lofstedt, Marta <marta.lofstedt at intel.com>
>> Subject: [PATCH v4 i-g-t] tests/kms: increase max threshold time for edid
>> read
>>
>> 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 */ 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);
> We can't have this lspcon as an argument to the test, it will not work with automated testing using piglit as we do for CI.
> Theoretically we could add a "has_lspcon" to debugfs, but I believe that this test has started to be over complicated.
The test would need to know if an LSPcon is connected on a port by port
basis. This is easy if the LSPcon driver is loaded but in the case of
USB_C->HDMI is gets a little more complicated (not impossible) to figure
out. Even if we know exactly what device is connected failures can still
occur if the TCON/Monitor clock stretches the EDID read.
> The intention of the test is to do a sanity check so that we don't drift off here, so I actually prefer the V1.
Unfortunately with the timing differences (3ms to 96ms) based on the
monitor type connected and EDID size there is no way for a one size fits
all sanity check to be reliable. If the original point of this test was
to figure out if probe caused more than 1 EDID read, maybe we should
actually count EDID reads and not infer it by measuring time.
-Clint
> Your detailed work could however be used in a benchmark, where the result would be the actually timing, I am sure there is a performance team who would like that.
>
> /Marta
>
>> +
>> + 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
More information about the Intel-gfx
mailing list