| Message ID | 1384189570-2955-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 11, 2013 at 03:06:10PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We recently fixed a bug where it was impossible to do I2C transactions > on eDP panels when they were disabled. Now it should be possible to do > these transactions when the panel is disabled, but there's a race > condition that triggers dmesg errors if we try do do the I2C > transactions and set a mode on the panel at the same time. This > program should reproduce this bug and check dmesg for errors. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Like I've said in the previous mail I think the generic dmesg error checking should be somewhere generic (and probably in piglit). Otherwise the test looks good. And the naming also matches the new convention ;-) -Daniel > --- > tests/.gitignore | 1 + > tests/Makefile.am | 2 + > tests/kms_edp_vdd_race.c | 226 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 229 insertions(+) > create mode 100644 tests/kms_edp_vdd_race.c > > diff --git a/tests/.gitignore b/tests/.gitignore > index 09ea074..ddb61f9 100644 > --- a/tests/.gitignore > +++ b/tests/.gitignore > @@ -102,6 +102,7 @@ igt_no_exit_list_only > igt_no_subtest > kms_addfb > kms_cursor_crc > +kms_edp_vdd_race > kms_flip > kms_pipe_crc_basic > kms_render > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 0426ec0..955d2a3 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -52,6 +52,7 @@ TESTS_progs_M = \ > gem_write_read_ring_switch \ > kms_addfb \ > kms_cursor_crc \ > + kms_edp_vdd_race \ > kms_flip \ > kms_pipe_crc_basic \ > kms_render \ > @@ -236,6 +237,7 @@ gem_fence_thrash_LDADD = $(LDADD) -lpthread > gem_flink_race_LDADD = $(LDADD) -lpthread > gem_threaded_access_tiled_LDADD = $(LDADD) -lpthread > prime_self_import_LDADD = $(LDADD) -lpthread > +kms_edp_vdd_race_LDADD = $(LDADD) -lpthread > > gem_wait_render_timeout_LDADD = $(LDADD) -lrt > kms_flip_LDADD = $(LDADD) -lrt > diff --git a/tests/kms_edp_vdd_race.c b/tests/kms_edp_vdd_race.c > new file mode 100644 > index 0000000..a6bff65 > --- /dev/null > +++ b/tests/kms_edp_vdd_race.c > @@ -0,0 +1,226 @@ > +/* > + * Copyright © 2013 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: Paulo Zanoni <paulo.r.zanoni@intel.com> > + * > + */ > + > +#include <dirent.h> > +#include <fcntl.h> > +#include <linux/i2c.h> > +#include <linux/i2c-dev.h> > +#include <pthread.h> > +#include <string.h> > +#include <sys/ioctl.h> > +#include <sys/stat.h> > +#include <sys/types.h> > + > +#include "drmtest.h" > + > +int drm_fd, kmsg_fd; > +drmModeResPtr res; > +drmModeConnectorPtr edp_connector; > +bool stop; > + > +static void disable_all_screens(void) > +{ > + int i, rc; > + > + for (i = 0; i < res->count_crtcs; i++) { > + rc = drmModeSetCrtc(drm_fd, res->crtcs[i], -1, 0, 0, > + NULL, 0, NULL); > + igt_assert(rc == 0); > + } > +} > + > +static void find_edp_connector(void) > +{ > + int i; > + drmModeConnectorPtr c; > + > + edp_connector = NULL; > + for (i = 0; i < res->count_connectors; i++) { > + c = drmModeGetConnector(drm_fd, res->connectors[i]); > + > + if (c->connector_type == DRM_MODE_CONNECTOR_eDP) { > + igt_require(c->connection == DRM_MODE_CONNECTED); > + edp_connector = c; > + break; > + } > + > + drmModeFreeConnector(c); > + } > + igt_require(edp_connector); > +} > + > +static void read_edid_ioctl(int fd) > +{ > + unsigned char edid[128] = {}; > + struct i2c_msg msgs[] = { > + { /* Start at 0. */ > + .addr = 0x50, > + .flags = 0, > + .len = 1, > + .buf = edid, > + }, { /* Now read the EDID. */ > + .addr = 0x50, > + .flags = I2C_M_RD, > + .len = 128, > + .buf = edid, > + } > + }; > + struct i2c_rdwr_ioctl_data msgset = { > + .msgs = msgs, > + .nmsgs = 2, > + }; > + > + ioctl(fd, I2C_RDWR, &msgset); > +} > + > +/* TODO: We're currently just trying to read all the I2C files. We should try to > + * find which one is really the eDP I2C file and just read from it. */ > +static void read_i2c_edid(void) > +{ > + int fd; > + DIR *dir; > + > + struct dirent *dirent; > + char full_name[32]; > + > + dir = opendir("/dev/"); > + igt_assert(dir); > + > + while ((dirent = readdir(dir))) { > + if (strncmp(dirent->d_name, "i2c-", 4) == 0) { > + snprintf(full_name, 32, "/dev/%s", dirent->d_name); > + fd = open(full_name, O_RDWR); > + igt_assert(fd != -1); > + read_edid_ioctl(fd); > + close(fd); > + } > + } > + > + closedir(dir); > +} > + > +static void *i2c_thread_func(void *unused) > +{ > + while (!stop) > + read_i2c_edid(); > + > + pthread_exit(NULL); > +} > + > +static uint32_t create_fb(int width, int height) > +{ > + struct kmstest_fb fb; > + cairo_t *cr; > + uint32_t buffer_id; > + > + buffer_id = kmstest_create_fb(drm_fd, width, height, 32, 24, false, > + &fb); > + cr = kmstest_get_cairo_ctx(drm_fd, &fb); > + kmstest_paint_test_pattern(cr, width, height); > + return buffer_id; > +} > + > +static void enable_edp_screen(void) > +{ > + int rc; > + uint32_t buffer_id = 0, crtc_id, connector_id; > + drmModeModeInfoPtr mode; > + > + crtc_id = res->crtcs[0]; > + connector_id = edp_connector->connector_id; > + mode = &edp_connector->modes[0]; > + > + buffer_id = create_fb(mode->hdisplay, mode->vdisplay); > + > + rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id, > + 1, mode); > + igt_assert(rc == 0); > +} > + > +static void *modeset_thread_func(void *unused) > +{ > + while (!stop) { > + disable_all_screens(); > + sleep(1); > + enable_edp_screen(); > + sleep(1); > + } > + > + pthread_exit(NULL); > +} > + > +/* This test exercises a race condition that happens when we're trying to read > + * the I2C data from an eDP panel at the same time we're trying to set a mode on > + * the same panel. If we have the bug, we print error messages on dmesg, which > + * we should catch with the kmsg functions. */ > +static void i2c_modeset_vdd_race(void) > +{ > + pthread_t i2c_thread, modeset_thread; > + void *status; > + > + kmsg_error_reset(kmsg_fd); > + > + stop = false; > + pthread_create(&i2c_thread, NULL, i2c_thread_func, NULL); > + pthread_create(&modeset_thread, NULL, modeset_thread_func, NULL); > + > + /* This effectively sleeps for 100 seconds, but kills the program in > + * case there's error on dmesg. */ > + kmsg_error_detect(kmsg_fd, 100 * 1000, ""); > + > + stop = true; > + pthread_join(i2c_thread, &status); > + pthread_join(modeset_thread, &status); > + > + /* Make sure we check everything after the threads have actually > + * stopped. */ > + kmsg_error_detect(kmsg_fd, 1 * 1000, ""); > +} > + > +igt_main > +{ > + igt_fixture { > + drm_fd = drm_open_any(); > + igt_require(drm_fd >= 0); > + > + res = drmModeGetResources(drm_fd); > + igt_assert(res); > + > + kmsg_fd = kmsg_error_setup(); > + > + find_edp_connector(); > + } > + > + igt_subtest("i2c-modeset-vdd-race") > + i2c_modeset_vdd_race(); > + > + igt_fixture { > + kmsg_error_teardown(kmsg_fd); > + drmModeFreeConnector(edp_connector); > + drmModeFreeResources(res); > + close(drm_fd); > + } > +} > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2013/11/11 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Nov 11, 2013 at 03:06:10PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> We recently fixed a bug where it was impossible to do I2C transactions >> on eDP panels when they were disabled. Now it should be possible to do >> these transactions when the panel is disabled, but there's a race >> condition that triggers dmesg errors if we try do do the I2C >> transactions and set a mode on the panel at the same time. This >> program should reproduce this bug and check dmesg for errors. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Like I've said in the previous mail I think the generic dmesg error > checking should be somewhere generic (and probably in piglit). Otherwise > the test looks good. And the naming also matches the new convention ;-) Then this test will always give a SUCCESS. Not really what I wanted :( > -Daniel > >> --- >> tests/.gitignore | 1 + >> tests/Makefile.am | 2 + >> tests/kms_edp_vdd_race.c | 226 +++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 229 insertions(+) >> create mode 100644 tests/kms_edp_vdd_race.c >> >> diff --git a/tests/.gitignore b/tests/.gitignore >> index 09ea074..ddb61f9 100644 >> --- a/tests/.gitignore >> +++ b/tests/.gitignore >> @@ -102,6 +102,7 @@ igt_no_exit_list_only >> igt_no_subtest >> kms_addfb >> kms_cursor_crc >> +kms_edp_vdd_race >> kms_flip >> kms_pipe_crc_basic >> kms_render >> diff --git a/tests/Makefile.am b/tests/Makefile.am >> index 0426ec0..955d2a3 100644 >> --- a/tests/Makefile.am >> +++ b/tests/Makefile.am >> @@ -52,6 +52,7 @@ TESTS_progs_M = \ >> gem_write_read_ring_switch \ >> kms_addfb \ >> kms_cursor_crc \ >> + kms_edp_vdd_race \ >> kms_flip \ >> kms_pipe_crc_basic \ >> kms_render \ >> @@ -236,6 +237,7 @@ gem_fence_thrash_LDADD = $(LDADD) -lpthread >> gem_flink_race_LDADD = $(LDADD) -lpthread >> gem_threaded_access_tiled_LDADD = $(LDADD) -lpthread >> prime_self_import_LDADD = $(LDADD) -lpthread >> +kms_edp_vdd_race_LDADD = $(LDADD) -lpthread >> >> gem_wait_render_timeout_LDADD = $(LDADD) -lrt >> kms_flip_LDADD = $(LDADD) -lrt >> diff --git a/tests/kms_edp_vdd_race.c b/tests/kms_edp_vdd_race.c >> new file mode 100644 >> index 0000000..a6bff65 >> --- /dev/null >> +++ b/tests/kms_edp_vdd_race.c >> @@ -0,0 +1,226 @@ >> +/* >> + * Copyright © 2013 Intel Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the next >> + * paragraph) shall be included in all copies or substantial portions of the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS >> + * IN THE SOFTWARE. >> + * >> + * Authors: Paulo Zanoni <paulo.r.zanoni@intel.com> >> + * >> + */ >> + >> +#include <dirent.h> >> +#include <fcntl.h> >> +#include <linux/i2c.h> >> +#include <linux/i2c-dev.h> >> +#include <pthread.h> >> +#include <string.h> >> +#include <sys/ioctl.h> >> +#include <sys/stat.h> >> +#include <sys/types.h> >> + >> +#include "drmtest.h" >> + >> +int drm_fd, kmsg_fd; >> +drmModeResPtr res; >> +drmModeConnectorPtr edp_connector; >> +bool stop; >> + >> +static void disable_all_screens(void) >> +{ >> + int i, rc; >> + >> + for (i = 0; i < res->count_crtcs; i++) { >> + rc = drmModeSetCrtc(drm_fd, res->crtcs[i], -1, 0, 0, >> + NULL, 0, NULL); >> + igt_assert(rc == 0); >> + } >> +} >> + >> +static void find_edp_connector(void) >> +{ >> + int i; >> + drmModeConnectorPtr c; >> + >> + edp_connector = NULL; >> + for (i = 0; i < res->count_connectors; i++) { >> + c = drmModeGetConnector(drm_fd, res->connectors[i]); >> + >> + if (c->connector_type == DRM_MODE_CONNECTOR_eDP) { >> + igt_require(c->connection == DRM_MODE_CONNECTED); >> + edp_connector = c; >> + break; >> + } >> + >> + drmModeFreeConnector(c); >> + } >> + igt_require(edp_connector); >> +} >> + >> +static void read_edid_ioctl(int fd) >> +{ >> + unsigned char edid[128] = {}; >> + struct i2c_msg msgs[] = { >> + { /* Start at 0. */ >> + .addr = 0x50, >> + .flags = 0, >> + .len = 1, >> + .buf = edid, >> + }, { /* Now read the EDID. */ >> + .addr = 0x50, >> + .flags = I2C_M_RD, >> + .len = 128, >> + .buf = edid, >> + } >> + }; >> + struct i2c_rdwr_ioctl_data msgset = { >> + .msgs = msgs, >> + .nmsgs = 2, >> + }; >> + >> + ioctl(fd, I2C_RDWR, &msgset); >> +} >> + >> +/* TODO: We're currently just trying to read all the I2C files. We should try to >> + * find which one is really the eDP I2C file and just read from it. */ >> +static void read_i2c_edid(void) >> +{ >> + int fd; >> + DIR *dir; >> + >> + struct dirent *dirent; >> + char full_name[32]; >> + >> + dir = opendir("/dev/"); >> + igt_assert(dir); >> + >> + while ((dirent = readdir(dir))) { >> + if (strncmp(dirent->d_name, "i2c-", 4) == 0) { >> + snprintf(full_name, 32, "/dev/%s", dirent->d_name); >> + fd = open(full_name, O_RDWR); >> + igt_assert(fd != -1); >> + read_edid_ioctl(fd); >> + close(fd); >> + } >> + } >> + >> + closedir(dir); >> +} >> + >> +static void *i2c_thread_func(void *unused) >> +{ >> + while (!stop) >> + read_i2c_edid(); >> + >> + pthread_exit(NULL); >> +} >> + >> +static uint32_t create_fb(int width, int height) >> +{ >> + struct kmstest_fb fb; >> + cairo_t *cr; >> + uint32_t buffer_id; >> + >> + buffer_id = kmstest_create_fb(drm_fd, width, height, 32, 24, false, >> + &fb); >> + cr = kmstest_get_cairo_ctx(drm_fd, &fb); >> + kmstest_paint_test_pattern(cr, width, height); >> + return buffer_id; >> +} >> + >> +static void enable_edp_screen(void) >> +{ >> + int rc; >> + uint32_t buffer_id = 0, crtc_id, connector_id; >> + drmModeModeInfoPtr mode; >> + >> + crtc_id = res->crtcs[0]; >> + connector_id = edp_connector->connector_id; >> + mode = &edp_connector->modes[0]; >> + >> + buffer_id = create_fb(mode->hdisplay, mode->vdisplay); >> + >> + rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id, >> + 1, mode); >> + igt_assert(rc == 0); >> +} >> + >> +static void *modeset_thread_func(void *unused) >> +{ >> + while (!stop) { >> + disable_all_screens(); >> + sleep(1); >> + enable_edp_screen(); >> + sleep(1); >> + } >> + >> + pthread_exit(NULL); >> +} >> + >> +/* This test exercises a race condition that happens when we're trying to read >> + * the I2C data from an eDP panel at the same time we're trying to set a mode on >> + * the same panel. If we have the bug, we print error messages on dmesg, which >> + * we should catch with the kmsg functions. */ >> +static void i2c_modeset_vdd_race(void) >> +{ >> + pthread_t i2c_thread, modeset_thread; >> + void *status; >> + >> + kmsg_error_reset(kmsg_fd); >> + >> + stop = false; >> + pthread_create(&i2c_thread, NULL, i2c_thread_func, NULL); >> + pthread_create(&modeset_thread, NULL, modeset_thread_func, NULL); >> + >> + /* This effectively sleeps for 100 seconds, but kills the program in >> + * case there's error on dmesg. */ >> + kmsg_error_detect(kmsg_fd, 100 * 1000, ""); >> + >> + stop = true; >> + pthread_join(i2c_thread, &status); >> + pthread_join(modeset_thread, &status); >> + >> + /* Make sure we check everything after the threads have actually >> + * stopped. */ >> + kmsg_error_detect(kmsg_fd, 1 * 1000, ""); >> +} >> + >> +igt_main >> +{ >> + igt_fixture { >> + drm_fd = drm_open_any(); >> + igt_require(drm_fd >= 0); >> + >> + res = drmModeGetResources(drm_fd); >> + igt_assert(res); >> + >> + kmsg_fd = kmsg_error_setup(); >> + >> + find_edp_connector(); >> + } >> + >> + igt_subtest("i2c-modeset-vdd-race") >> + i2c_modeset_vdd_race(); >> + >> + igt_fixture { >> + kmsg_error_teardown(kmsg_fd); >> + drmModeFreeConnector(edp_connector); >> + drmModeFreeResources(res); >> + close(drm_fd); >> + } >> +} >> -- >> 1.8.3.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Nov 11, 2013 at 04:25:36PM -0200, Paulo Zanoni wrote: > 2013/11/11 Daniel Vetter <daniel@ffwll.ch>: > > On Mon, Nov 11, 2013 at 03:06:10PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> We recently fixed a bug where it was impossible to do I2C transactions > >> on eDP panels when they were disabled. Now it should be possible to do > >> these transactions when the panel is disabled, but there's a race > >> condition that triggers dmesg errors if we try do do the I2C > >> transactions and set a mode on the panel at the same time. This > >> program should reproduce this bug and check dmesg for errors. > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Like I've said in the previous mail I think the generic dmesg error > > checking should be somewhere generic (and probably in piglit). Otherwise > > the test looks good. And the naming also matches the new convention ;-) > > Then this test will always give a SUCCESS. Not really what I wanted :( It's not the only one. We have tests that only annoy the in-kernel debug features like lockdep, object use-after-free and other stuff. Or all the WARN backtraces from testdisplay. And very often they all "succeed". Checking dmesg in individual tests really doesn't make much sense imo and needs to be somewhere where it's done for _all_ testcases. QA already has that in their own testrunner infrastructure, unfortunately that's not shared with developers so we get to invent a new wheel. -Daniel
2013/11/11 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Nov 11, 2013 at 04:25:36PM -0200, Paulo Zanoni wrote: >> 2013/11/11 Daniel Vetter <daniel@ffwll.ch>: >> > On Mon, Nov 11, 2013 at 03:06:10PM -0200, Paulo Zanoni wrote: >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> >> >> We recently fixed a bug where it was impossible to do I2C transactions >> >> on eDP panels when they were disabled. Now it should be possible to do >> >> these transactions when the panel is disabled, but there's a race >> >> condition that triggers dmesg errors if we try do do the I2C >> >> transactions and set a mode on the panel at the same time. This >> >> program should reproduce this bug and check dmesg for errors. >> >> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> > Like I've said in the previous mail I think the generic dmesg error >> > checking should be somewhere generic (and probably in piglit). Otherwise >> > the test looks good. And the naming also matches the new convention ;-) >> >> Then this test will always give a SUCCESS. Not really what I wanted :( > > It's not the only one. We have tests that only annoy the in-kernel debug > features like lockdep, object use-after-free and other stuff. Or all the > WARN backtraces from testdisplay. And very often they all "succeed". And that's the problem I'm trying to solve. We have a solution, it's useful not just for me - you just gave examples of where it would be useful too -, yet, IMHO, you still didn't give a good technical reason on why you're rejecting it. > > Checking dmesg in individual tests really doesn't make much sense imo Well, IMHO it makes a lot of sense. It's even helping me write code, as I already explained. > and > needs to be somewhere where it's done for _all_ testcases. My code is not preventing that. In fact, I think it's helping us get to that point. > QA already has > that in their own testrunner infrastructure, unfortunately that's not > shared with developers so we get to invent a new wheel. I just proposed these new wheels... > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Nov 11, 2013 at 04:54:32PM -0200, Paulo Zanoni wrote: > 2013/11/11 Daniel Vetter <daniel@ffwll.ch>: > > On Mon, Nov 11, 2013 at 04:25:36PM -0200, Paulo Zanoni wrote: > >> 2013/11/11 Daniel Vetter <daniel@ffwll.ch>: > >> > On Mon, Nov 11, 2013 at 03:06:10PM -0200, Paulo Zanoni wrote: > >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> >> > >> >> We recently fixed a bug where it was impossible to do I2C transactions > >> >> on eDP panels when they were disabled. Now it should be possible to do > >> >> these transactions when the panel is disabled, but there's a race > >> >> condition that triggers dmesg errors if we try do do the I2C > >> >> transactions and set a mode on the panel at the same time. This > >> >> program should reproduce this bug and check dmesg for errors. > >> >> > >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > > >> > Like I've said in the previous mail I think the generic dmesg error > >> > checking should be somewhere generic (and probably in piglit). Otherwise > >> > the test looks good. And the naming also matches the new convention ;-) > >> > >> Then this test will always give a SUCCESS. Not really what I wanted :( > > > > It's not the only one. We have tests that only annoy the in-kernel debug > > features like lockdep, object use-after-free and other stuff. Or all the > > WARN backtraces from testdisplay. And very often they all "succeed". > > And that's the problem I'm trying to solve. We have a solution, it's > useful not just for me - you just gave examples of where it would be > useful too -, yet, IMHO, you still didn't give a good technical reason > on why you're rejecting it. > > > > > Checking dmesg in individual tests really doesn't make much sense imo > > Well, IMHO it makes a lot of sense. It's even helping me write code, > as I already explained. > > > > and > > needs to be somewhere where it's done for _all_ testcases. > > My code is not preventing that. In fact, I think it's helping us get > to that point. > > > > QA already has > > that in their own testrunner infrastructure, unfortunately that's not > > shared with developers so we get to invent a new wheel. > > I just proposed these new wheels... Ok, lazy me finally got around to just doing it. I've sent 2 patches to the piglit mailing list which enable dmesg checking for igt runs by default in less than 5 lines of code. For all tests, at the subtest granularity. Imo this simplicity a technical reason to do it in piglit ;-) That leaves us with your use-case of very fine-grained checking of dmesg errors within a testcase. tbh I'm not really sold on this being that useful, but I'd be ok with merging the helper code if you convinced it's a great idea. One thing though which could be improved is the cleanup - imo it's much simpler to just have an atexit handler for such helpers. Cheers, Daniel
diff --git a/tests/.gitignore b/tests/.gitignore index 09ea074..ddb61f9 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -102,6 +102,7 @@ igt_no_exit_list_only igt_no_subtest kms_addfb kms_cursor_crc +kms_edp_vdd_race kms_flip kms_pipe_crc_basic kms_render diff --git a/tests/Makefile.am b/tests/Makefile.am index 0426ec0..955d2a3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -52,6 +52,7 @@ TESTS_progs_M = \ gem_write_read_ring_switch \ kms_addfb \ kms_cursor_crc \ + kms_edp_vdd_race \ kms_flip \ kms_pipe_crc_basic \ kms_render \ @@ -236,6 +237,7 @@ gem_fence_thrash_LDADD = $(LDADD) -lpthread gem_flink_race_LDADD = $(LDADD) -lpthread gem_threaded_access_tiled_LDADD = $(LDADD) -lpthread prime_self_import_LDADD = $(LDADD) -lpthread +kms_edp_vdd_race_LDADD = $(LDADD) -lpthread gem_wait_render_timeout_LDADD = $(LDADD) -lrt kms_flip_LDADD = $(LDADD) -lrt diff --git a/tests/kms_edp_vdd_race.c b/tests/kms_edp_vdd_race.c new file mode 100644 index 0000000..a6bff65 --- /dev/null +++ b/tests/kms_edp_vdd_race.c @@ -0,0 +1,226 @@ +/* + * Copyright © 2013 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: Paulo Zanoni <paulo.r.zanoni@intel.com> + * + */ + +#include <dirent.h> +#include <fcntl.h> +#include <linux/i2c.h> +#include <linux/i2c-dev.h> +#include <pthread.h> +#include <string.h> +#include <sys/ioctl.h> +#include <sys/stat.h> +#include <sys/types.h> + +#include "drmtest.h" + +int drm_fd, kmsg_fd; +drmModeResPtr res; +drmModeConnectorPtr edp_connector; +bool stop; + +static void disable_all_screens(void) +{ + int i, rc; + + for (i = 0; i < res->count_crtcs; i++) { + rc = drmModeSetCrtc(drm_fd, res->crtcs[i], -1, 0, 0, + NULL, 0, NULL); + igt_assert(rc == 0); + } +} + +static void find_edp_connector(void) +{ + int i; + drmModeConnectorPtr c; + + edp_connector = NULL; + for (i = 0; i < res->count_connectors; i++) { + c = drmModeGetConnector(drm_fd, res->connectors[i]); + + if (c->connector_type == DRM_MODE_CONNECTOR_eDP) { + igt_require(c->connection == DRM_MODE_CONNECTED); + edp_connector = c; + break; + } + + drmModeFreeConnector(c); + } + igt_require(edp_connector); +} + +static void read_edid_ioctl(int fd) +{ + unsigned char edid[128] = {}; + struct i2c_msg msgs[] = { + { /* Start at 0. */ + .addr = 0x50, + .flags = 0, + .len = 1, + .buf = edid, + }, { /* Now read the EDID. */ + .addr = 0x50, + .flags = I2C_M_RD, + .len = 128, + .buf = edid, + } + }; + struct i2c_rdwr_ioctl_data msgset = { + .msgs = msgs, + .nmsgs = 2, + }; + + ioctl(fd, I2C_RDWR, &msgset); +} + +/* TODO: We're currently just trying to read all the I2C files. We should try to + * find which one is really the eDP I2C file and just read from it. */ +static void read_i2c_edid(void) +{ + int fd; + DIR *dir; + + struct dirent *dirent; + char full_name[32]; + + dir = opendir("/dev/"); + igt_assert(dir); + + while ((dirent = readdir(dir))) { + if (strncmp(dirent->d_name, "i2c-", 4) == 0) { + snprintf(full_name, 32, "/dev/%s", dirent->d_name); + fd = open(full_name, O_RDWR); + igt_assert(fd != -1); + read_edid_ioctl(fd); + close(fd); + } + } + + closedir(dir); +} + +static void *i2c_thread_func(void *unused) +{ + while (!stop) + read_i2c_edid(); + + pthread_exit(NULL); +} + +static uint32_t create_fb(int width, int height) +{ + struct kmstest_fb fb; + cairo_t *cr; + uint32_t buffer_id; + + buffer_id = kmstest_create_fb(drm_fd, width, height, 32, 24, false, + &fb); + cr = kmstest_get_cairo_ctx(drm_fd, &fb); + kmstest_paint_test_pattern(cr, width, height); + return buffer_id; +} + +static void enable_edp_screen(void) +{ + int rc; + uint32_t buffer_id = 0, crtc_id, connector_id; + drmModeModeInfoPtr mode; + + crtc_id = res->crtcs[0]; + connector_id = edp_connector->connector_id; + mode = &edp_connector->modes[0]; + + buffer_id = create_fb(mode->hdisplay, mode->vdisplay); + + rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id, + 1, mode); + igt_assert(rc == 0); +} + +static void *modeset_thread_func(void *unused) +{ + while (!stop) { + disable_all_screens(); + sleep(1); + enable_edp_screen(); + sleep(1); + } + + pthread_exit(NULL); +} + +/* This test exercises a race condition that happens when we're trying to read + * the I2C data from an eDP panel at the same time we're trying to set a mode on + * the same panel. If we have the bug, we print error messages on dmesg, which + * we should catch with the kmsg functions. */ +static void i2c_modeset_vdd_race(void) +{ + pthread_t i2c_thread, modeset_thread; + void *status; + + kmsg_error_reset(kmsg_fd); + + stop = false; + pthread_create(&i2c_thread, NULL, i2c_thread_func, NULL); + pthread_create(&modeset_thread, NULL, modeset_thread_func, NULL); + + /* This effectively sleeps for 100 seconds, but kills the program in + * case there's error on dmesg. */ + kmsg_error_detect(kmsg_fd, 100 * 1000, ""); + + stop = true; + pthread_join(i2c_thread, &status); + pthread_join(modeset_thread, &status); + + /* Make sure we check everything after the threads have actually + * stopped. */ + kmsg_error_detect(kmsg_fd, 1 * 1000, ""); +} + +igt_main +{ + igt_fixture { + drm_fd = drm_open_any(); + igt_require(drm_fd >= 0); + + res = drmModeGetResources(drm_fd); + igt_assert(res); + + kmsg_fd = kmsg_error_setup(); + + find_edp_connector(); + } + + igt_subtest("i2c-modeset-vdd-race") + i2c_modeset_vdd_race(); + + igt_fixture { + kmsg_error_teardown(kmsg_fd); + drmModeFreeConnector(edp_connector); + drmModeFreeResources(res); + close(drm_fd); + } +}