Re: [igt-dev] [PATCH v2] tests/i915_pm_rpm: improved strictness and verbosity of i2c subtest

Ville Syrjälä
2019-06-14T15:30:55Z

On Thu, Jun 13, 2019 at 04:06:23PM +0300, Oleg Vasilev wrote:
> Test checked that the number of valid edids from drm is equal to the
> same number from i2c.
>
> Now for each edid the whole blob is compared. In case of mismatch the
> whole edid is printed.
>
> v2:
> - Improved messages in order to set up CI filter
> - Log and clear errno in case of i2c failure
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104097
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Martin Peres <martin.peres@linux.intel.com>
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> ---
> tests/i915/i915_pm_rpm.c | 180 ++++++++++++++++++++++++---------------
> 1 file changed, 112 insertions(+), 68 deletions(-)
>
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index f9f67ed3..87140e30 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -572,33 +572,55 @@ static void assert_drm_infos_equal(struct compare_data *d1,
> assert_drm_crtcs_equal(d1->crtcs[i], d2->crtcs[i]);
> }
>
> -/* We could check the checksum too, but just the header is probably enough. */
> -static bool edid_is_valid(const unsigned char *edid)
> +static bool find_i2c_path(char *connector_name, char *i2c_path)

const char *connector_name

> {
> - char edid_header[] = {
> - 0x0, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x0,
> - };
> + struct dirent *dirent;
> + DIR *dir;
> + int sysfs_card_fd = igt_sysfs_open(drm_fd);
> + int connector_fd = -1;
> + bool found_i2c_file = false;
>
> - return (memcmp(edid, edid_header, sizeof(edid_header)) == 0);
> -}
> + dir = fdopendir(sysfs_card_fd);
> + igt_assert(dir);
>
> -static int count_drm_valid_edids(struct mode_set_data *data)
> -{
> - int ret = 0;
> + while ((dirent = readdir(dir))) {
> + /* Skip "cardx-" prefix */
> + char *dirname = dirent->d_name;
> + while(dirname[0] != '\0' && dirname[0] != '-')
> + ++dirname;
> - if (!data->res)
> - return 0;
> + if(*dirname == '-')
> + ++dirname;

dirname = dirent->d_name;
strsep(&dirname, "-");
dirname = dirname ?: dirent->d_name;

or

dirname = strchr(dirent->d_name, '-');
dirname = dirname ? dirname + 1 : dirent->d_name;

?

>
> - for (int i = 0; i < data->res->count_connectors; i++)
> - if (data->edids[i] && edid_is_valid(data->edids[i]->data))
> - ret++;
> - return ret;
> + if (strcmp(dirname, connector_name) == 0) {
> + connector_fd = openat(sysfs_card_fd, dirent->d_name, O_RDONLY);
> + break;
> + }
> + }
> + closedir(dir);
> +
> + if(connector_fd < 0)
> + return false;
> +
> + dir = fdopendir(connector_fd);
> + igt_assert(dir);
> +
> + while ((dirent = readdir(dir))) {
> + if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
> + sprintf(i2c_path, "/dev/%s", dirent->d_name);
> + found_i2c_file = true;
> + }
> + }
> + closedir(dir);
> + return found_i2c_file;
> }
>
> -static bool i2c_edid_is_valid(int fd)
> +
> +static bool i2c_read_edid(char *connector_name, unsigned char *edid)
> {
> - int rc;
> - unsigned char edid[128] = {};
> + char i2c_path[PATH_MAX];
> + bool result = find_i2c_path(connector_name, i2c_path);
> + int rc, fd;
> struct i2c_msg msgs[] = {
> { /* Start at 0. */
> .addr = 0x50,
> @@ -617,69 +639,91 @@ static bool i2c_edid_is_valid(int fd)
> .nmsgs = 2,
> };
>

I would put the
result = find_i2c_path(connector_name, i2c_path);

here because it's a nasty function with side effects. So IMO
shouldn't be hidden in the declaration block.

> + if (!result)
> + return false;
> +
> + fd = open(i2c_path, O_RDWR);
> + igt_assert_neq(fd, -1);
> +
> rc = ioctl(fd, I2C_RDWR, &msgset);
> - return (rc >= 0) ? edid_is_valid(edid) : false;
> + if(rc==-1){
> + igt_debug("I2C access failed with errno %d, %s\n",
> + errno, strerror(errno));
> + errno = 0;
> + }
> +
> + close(fd);
> + return rc >= 0;
> }
>
> -static int count_i2c_valid_edids(void)
> +static void format_hex_string(unsigned char edid[static EDID_LENGTH],
> + char buf[static EDID_LENGTH * 5 + 1])

What's the deal with those 'static's?

'edid' can be const.

> {
> - int fd, ret = 0;
> - DIR *dir;
> + for (size_t i = 0; i < EDID_LENGTH; ++i)

I'm not a fan of 'i' which is not int. Most people would expect
it to be signed int so I would recommend against using 'i'
when you have some other type. In this case 'int' should
do just fine I think.

> + sprintf(buf+i*5, "0x%02x ", edid[i]);
> +}
>
> - struct dirent *dirent;
> - char full_name[PATH_MAX];
> +static void test_i2c(struct mode_set_data *data)
> +{
> + bool edid_mistmach_i2c_vs_drm = false;
> + igt_display_t display;
> + igt_display_require(&display, drm_fd);
>
> - dir = opendir("/dev/");
> - igt_assert(dir);
> + for (int i = 0; i < data->res->count_connectors; i++) {
> + unsigned char *drm_edid = data->edids[i] ? data->edids[i]->data : NULL;
> + unsigned char i2c_edid[EDID_LENGTH] = {};
>
> - while ((dirent = readdir(dir))) {
> - if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
> - sprintf(full_name, "/dev/%s", dirent->d_name);
> - fd = open(full_name, O_RDWR);
> - igt_assert_neq(fd, -1);
> - if (i2c_edid_is_valid(fd))
> - ret++;
> - close(fd);
> - }
> - }
> + igt_output_t *output = igt_output_from_connector(&display,
> + data->connectors[i]);
> + char *connector_name = (char *) igt_output_name(output);
>
> - closedir(dir);
> + bool got_i2c_edid = i2c_read_edid(connector_name, i2c_edid);
> + bool got_drm_edid = drm_edid != NULL;
> + bool is_vga = data->connectors[i]->connector_type == DRM_MODE_CONNECTOR_VGA;
>
> - return ret;
> -}
> + bool edids_equal;
>
> -static int count_vga_outputs(struct mode_set_data *data)
> -{
> - int count = 0;
> + /* We fail to detect some VGA monitors using our i2c method. If you look
> + * at the dmesg of these cases, you'll see the Kernel complaining about
> + * the EDID reading mostly FFs and then disabling bit-banging. Since we
> + * don't want to reimplement everything the Kernel does, let's just
> + * accept the fact that some VGA outputs won't be properly detected. */
> + if(is_vga)
> + continue;
>
> - if (!data->res)
> - return 0;
> + if(!got_i2c_edid && !got_drm_edid)
> + continue;
> +
> + if(got_i2c_edid && got_drm_edid)
> + edids_equal = (0 == memcmp(drm_edid, i2c_edid, EDID_LENGTH));
> + else
> + edids_equal = false;
>
> - for (int i = 0; i < data->res->count_connectors; i++)
> - if (data->connectors[i]->connector_type ==
> - DRM_MODE_CONNECTOR_VGA)
> - count++;
>
> - return count;
> -}
> + if (!edids_equal) {
> + char buf[5 * EDID_LENGTH + 1];
> + igt_critical("Detected EDID mismatch on connector %s\n",
> + connector_name);
>
> -static void test_i2c(struct mode_set_data *data)
> -{
> - int i2c_edids = count_i2c_valid_edids();
> - int drm_edids = count_drm_valid_edids(data);
> - int vga_outputs = count_vga_outputs(data);
> - int diff;
> -
> - igt_debug("i2c edids:%d drm edids:%d vga outputs:%d\n",
> - i2c_edids, drm_edids, vga_outputs);
> -
> - /* We fail to detect some VGA monitors using our i2c method. If you look
> - * at the dmesg of these cases, you'll see the Kernel complaining about
> - * the EDID reading mostly FFs and then disabling bit-banging. Since we
> - * don't want to reimplement everything the Kernel does, let's just
> - * accept the fact that some VGA outputs won't be properly detected. */
> - diff = drm_edids - i2c_edids;
> - igt_assert(diff <= vga_outputs && diff >= 0);
> + if(got_i2c_edid)
> + format_hex_string(i2c_edid, buf);
> + else
> + sprintf(buf, "NULL");
> +
> + igt_critical("i2c: %s\n", buf);
> +
> + if(got_drm_edid)
> + format_hex_string(drm_edid, buf);
> + else
> + sprintf(buf, "NULL");
> +
> + igt_critical("drm: %s\n", buf);
> +
> + edid_mistmach_i2c_vs_drm = true;
> + }
> + }
> + igt_fail_on_f(edid_mistmach_i2c_vs_drm,
> + "There is an EDID mismatch between i2c and DRM!\n");

The kernel does modify the reported EDID for some quirks IIRC. I don't
really like that behaviour, and it would be an issue for this test. But
I guess we don't have any of those on any monitors we test.

Apart from the minor nits looks good to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> }
>
> static void setup_pc8(void)
> --
> 2.21.0

--
Ville Syrjälä
Intel