[igt-dev] [PATCH] tests/i915_pm_rpm: improved strictness and verbosity of i2c subtest
Imre Deak
imre.deak at intel.com
Tue May 7 13:28:30 UTC 2019
On Tue, May 07, 2019 at 03:25:21PM +0300, Ville Syrjälä wrote:
> On Tue, May 07, 2019 at 02:58:10PM +0300, Imre Deak wrote:
> > On Tue, May 07, 2019 at 11:51:54AM +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.
> >
> > Hm, if I'm parsing the code correctly we have a symlink to the
> > connector's I2C device only for DP and SDVO connectors, but not for
> > HDMI or VGA.
>
> For DP the aux/i2c is a child of the connector. For everything else I
> think it's a child of the pci device. SDVO looks like the non-DP case
> to me, ie. i2c dev is a child of the pci dev. Which probably makes
> sense in case the SDVO encoder has multiple connectors.
>
> >
> > Jani, Ville do you know what's the reason for that? I think having a
> > symlink would be useful.
>
> Yes, symlink would be nice.
Ok, so Oleg, I think first you'd need to add the I2C symlinks to all
connectors in the driver. I don't think there is any other good way to
determine the mapping.
>
> >
> > Is there another way to identify which I2C device a connector uses?
> >
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104097
> > > Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > Signed-off-by: Oleg Vasilev <oleg.vasilev at intel.com>
> > > ---
> > > tests/i915/i915_pm_rpm.c | 176 ++++++++++++++++++++++++---------------
> > > 1 file changed, 108 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> > > index a2c9d0ed..f1530391 100644
> > > --- a/tests/i915/i915_pm_rpm.c
> > > +++ b/tests/i915/i915_pm_rpm.c
> > > @@ -571,33 +571,56 @@ 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)
> > > {
> > > - 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;
> > >
> > > - 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,
> > > @@ -616,69 +639,86 @@ static bool i2c_edid_is_valid(int fd)
> > > .nmsgs = 2,
> > > };
> > >
> > > +
> > > + 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;
> > > +
> > > + 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])
> > > {
> > > - int fd, ret = 0;
> > > - DIR *dir;
> > > + for (size_t i = 0; i < EDID_LENGTH; ++i)
> > > + 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 failed = 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("For connector %s EDID mismatch!\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);
> > > +
> > > + failed = true;
> > > + }
> > > + }
> > > + igt_fail_on_f(failed, "Mismatch on EDID we got from i2c and DRM!\n");
> > > }
> > >
> > > static void setup_pc8(void)
> > > --
> > > 2.21.0
> > >
>
> --
> Ville Syrjälä
> Intel
More information about the igt-dev
mailing list