| Message ID | 20171123122614.36496-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 23, 2017 at 01:26:14PM +0100, Maarten Lankhorst wrote: > This was implemented correctly only on the atomic ioctl before, but > it should really be working on all 3 ioctl's involved, so ensure we > always set crtc_id correctly with a testcase. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> We seemt to completely lack these checks for the vblank ioctl and the atomic ioctl too. Can you pls fill these gaps (probably best in kms_flip.c and kms_atomic.c), too? > --- > tests/kms_vblank.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 56 insertions(+), 4 deletions(-) > > diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c > index 47fd10fb9078..004f0e6104ee 100644 > --- a/tests/kms_vblank.c > +++ b/tests/kms_vblank.c > @@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) > igt_display_t *display = &data->display; > igt_output_t *output; > enum pipe p; > - unsigned int valid_tests = 0; > > for_each_pipe_with_valid_output(display, p, output) { > igt_hang_t hang; > @@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) > > /* cleanup what prepare_crtc() has done */ > cleanup_crtc(data, fd, output); > - valid_tests++; > } > +} > + > +static void crtc_id_subtest(data_t *data, int fd) > +{ > + igt_display_t *display = &data->display; > + igt_output_t *output; > + enum pipe p; > + > + for_each_pipe_with_valid_output(display, p, output) { > + struct drm_event_vblank buf; > + const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p); > + unsigned crtc_id, expected_crtc_id; > + uint64_t val; > + union drm_wait_vblank vbl; > + > + crtc_id = display->pipes[p].crtc_id; > + if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0) > + expected_crtc_id = crtc_id; > + else > + expected_crtc_id = 0; > + > + data->pipe = p; > + prepare_crtc(data, fd, output); > + > + memset(&vbl, 0, sizeof(vbl)); > + vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT; > + vbl.request.type |= pipe_id_flag; > + vbl.request.sequence = 1; > + igt_assert_eq(wait_vblank(fd, &vbl), 0); > + > + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); > + igt_assert_eq(buf.crtc_id, expected_crtc_id); > + > + do_or_die(drmModePageFlip(fd, crtc_id, > + data->primary_fb.fb_id, > + DRM_MODE_PAGE_FLIP_EVENT, NULL)); > + > + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); > + igt_assert_eq(buf.crtc_id, expected_crtc_id); > + > + if (display->is_atomic) { > + igt_plane_t *primary = igt_output_get_plane(output, 0); > + > + igt_plane_set_fb(primary, &data->primary_fb); > + igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL); > > - igt_require_f(valid_tests, > - "no valid crtc/connector combinations found\n"); > + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); > + igt_assert_eq(buf.crtc_id, expected_crtc_id); > + } > + > + cleanup_crtc(data, fd, output); > + return; > + } > } > > static void accuracy(data_t *data, int fd, int nchildren) > @@ -307,8 +355,12 @@ igt_main > fd = drm_open_driver(DRIVER_ANY); > kmstest_set_vt_graphics_mode(); > igt_display_init(&data.display, fd); > + igt_display_require_output(&data.display); > } > > + igt_subtest("crtc-id") > + crtc_id_subtest(&data, fd); Either I'm stupid, or this doesn't apply on top of latest igt master. Either way I think taking the expensive modesets out of individual tests would be good, so that when we run entire binaries in CI, we'll benefit from some good speedup. Which is the plan, now that machines are a bit more stable ... -Daniel > + > for (f = funcs; f->name; f++) { > for (m = modes; m->name; m++) { > if (m->flags & ~f->valid) > -- > 2.15.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 24-11-17 om 16:03 schreef Daniel Vetter: > On Thu, Nov 23, 2017 at 01:26:14PM +0100, Maarten Lankhorst wrote: >> This was implemented correctly only on the atomic ioctl before, but >> it should really be working on all 3 ioctl's involved, so ensure we >> always set crtc_id correctly with a testcase. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > We seemt to completely lack these checks for the vblank ioctl and the > atomic ioctl too. Can you pls fill these gaps (probably best in kms_flip.c > and kms_atomic.c), too? Summary is a bit out of date, but I did add them in here. :) >> --- >> tests/kms_vblank.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 56 insertions(+), 4 deletions(-) >> >> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c >> index 47fd10fb9078..004f0e6104ee 100644 >> --- a/tests/kms_vblank.c >> +++ b/tests/kms_vblank.c >> @@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) >> igt_display_t *display = &data->display; >> igt_output_t *output; >> enum pipe p; >> - unsigned int valid_tests = 0; >> >> for_each_pipe_with_valid_output(display, p, output) { >> igt_hang_t hang; >> @@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) >> >> /* cleanup what prepare_crtc() has done */ >> cleanup_crtc(data, fd, output); >> - valid_tests++; >> } >> +} >> + >> +static void crtc_id_subtest(data_t *data, int fd) >> +{ >> + igt_display_t *display = &data->display; >> + igt_output_t *output; >> + enum pipe p; >> + >> + for_each_pipe_with_valid_output(display, p, output) { >> + struct drm_event_vblank buf; >> + const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p); >> + unsigned crtc_id, expected_crtc_id; >> + uint64_t val; >> + union drm_wait_vblank vbl; >> + >> + crtc_id = display->pipes[p].crtc_id; >> + if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0) >> + expected_crtc_id = crtc_id; >> + else >> + expected_crtc_id = 0; >> + >> + data->pipe = p; >> + prepare_crtc(data, fd, output); >> + >> + memset(&vbl, 0, sizeof(vbl)); >> + vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT; >> + vbl.request.type |= pipe_id_flag; >> + vbl.request.sequence = 1; >> + igt_assert_eq(wait_vblank(fd, &vbl), 0); >> + >> + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); >> + igt_assert_eq(buf.crtc_id, expected_crtc_id); >> + >> + do_or_die(drmModePageFlip(fd, crtc_id, >> + data->primary_fb.fb_id, >> + DRM_MODE_PAGE_FLIP_EVENT, NULL)); >> + >> + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); >> + igt_assert_eq(buf.crtc_id, expected_crtc_id); >> + >> + if (display->is_atomic) { >> + igt_plane_t *primary = igt_output_get_plane(output, 0); >> + >> + igt_plane_set_fb(primary, &data->primary_fb); >> + igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL); >> >> - igt_require_f(valid_tests, >> - "no valid crtc/connector combinations found\n"); >> + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); >> + igt_assert_eq(buf.crtc_id, expected_crtc_id); >> + } >> + >> + cleanup_crtc(data, fd, output); >> + return; >> + } >> } >> >> static void accuracy(data_t *data, int fd, int nchildren) >> @@ -307,8 +355,12 @@ igt_main >> fd = drm_open_driver(DRIVER_ANY); >> kmstest_set_vt_graphics_mode(); >> igt_display_init(&data.display, fd); >> + igt_display_require_output(&data.display); >> } >> >> + igt_subtest("crtc-id") >> + crtc_id_subtest(&data, fd); > Either I'm stupid, or this doesn't apply on top of latest igt master. > > Either way I think taking the expensive modesets out of individual tests > would be good, so that when we run entire binaries in CI, we'll benefit > from some good speedup. Which is the plan, now that machines are a bit > more stable ... This is my intention of speeding up IGT as well, tests that don't care can use the inherited pipe/connector. It will speed up testing a lot by preventing even performing a modeset. I also plan to add a function to read the current state through igt_display_read_hw_state(), then find an active pipe/connector, or just the first combination if nothing matches. And after the end of the test keep it alive without disabling the crtc's, only removing framebuffers, which no longer disables the crtc's on i915. :-) ~Maarten
On Fri, Nov 24, 2017 at 04:35:10PM +0100, Maarten Lankhorst wrote: > Op 24-11-17 om 16:03 schreef Daniel Vetter: > > On Thu, Nov 23, 2017 at 01:26:14PM +0100, Maarten Lankhorst wrote: > >> This was implemented correctly only on the atomic ioctl before, but > >> it should really be working on all 3 ioctl's involved, so ensure we > >> always set crtc_id correctly with a testcase. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > We seemt to completely lack these checks for the vblank ioctl and the > > atomic ioctl too. Can you pls fill these gaps (probably best in kms_flip.c > > and kms_atomic.c), too? > Summary is a bit out of date, but I did add them in here. :) Oh was blind. > >> --- > >> tests/kms_vblank.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++---- > >> 1 file changed, 56 insertions(+), 4 deletions(-) > >> > >> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c > >> index 47fd10fb9078..004f0e6104ee 100644 > >> --- a/tests/kms_vblank.c > >> +++ b/tests/kms_vblank.c > >> @@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) > >> igt_display_t *display = &data->display; > >> igt_output_t *output; > >> enum pipe p; > >> - unsigned int valid_tests = 0; > >> > >> for_each_pipe_with_valid_output(display, p, output) { > >> igt_hang_t hang; > >> @@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) > >> > >> /* cleanup what prepare_crtc() has done */ > >> cleanup_crtc(data, fd, output); > >> - valid_tests++; > >> } > >> +} > >> + > >> +static void crtc_id_subtest(data_t *data, int fd) > >> +{ > >> + igt_display_t *display = &data->display; > >> + igt_output_t *output; > >> + enum pipe p; > >> + > >> + for_each_pipe_with_valid_output(display, p, output) { > >> + struct drm_event_vblank buf; > >> + const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p); > >> + unsigned crtc_id, expected_crtc_id; > >> + uint64_t val; > >> + union drm_wait_vblank vbl; > >> + > >> + crtc_id = display->pipes[p].crtc_id; > >> + if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0) > >> + expected_crtc_id = crtc_id; > >> + else > >> + expected_crtc_id = 0; > >> + > >> + data->pipe = p; > >> + prepare_crtc(data, fd, output); > >> + > >> + memset(&vbl, 0, sizeof(vbl)); > >> + vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT; > >> + vbl.request.type |= pipe_id_flag; > >> + vbl.request.sequence = 1; > >> + igt_assert_eq(wait_vblank(fd, &vbl), 0); > >> + > >> + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); > >> + igt_assert_eq(buf.crtc_id, expected_crtc_id); > >> + > >> + do_or_die(drmModePageFlip(fd, crtc_id, > >> + data->primary_fb.fb_id, > >> + DRM_MODE_PAGE_FLIP_EVENT, NULL)); > >> + > >> + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); > >> + igt_assert_eq(buf.crtc_id, expected_crtc_id); > >> + > >> + if (display->is_atomic) { > >> + igt_plane_t *primary = igt_output_get_plane(output, 0); > >> + > >> + igt_plane_set_fb(primary, &data->primary_fb); > >> + igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL); > >> > >> - igt_require_f(valid_tests, > >> - "no valid crtc/connector combinations found\n"); > >> + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); > >> + igt_assert_eq(buf.crtc_id, expected_crtc_id); > >> + } > >> + > >> + cleanup_crtc(data, fd, output); > >> + return; > >> + } > >> } > >> > >> static void accuracy(data_t *data, int fd, int nchildren) > >> @@ -307,8 +355,12 @@ igt_main > >> fd = drm_open_driver(DRIVER_ANY); > >> kmstest_set_vt_graphics_mode(); > >> igt_display_init(&data.display, fd); > >> + igt_display_require_output(&data.display); > >> } > >> > >> + igt_subtest("crtc-id") > >> + crtc_id_subtest(&data, fd); > > Either I'm stupid, or this doesn't apply on top of latest igt master. > > > > Either way I think taking the expensive modesets out of individual tests > > would be good, so that when we run entire binaries in CI, we'll benefit > > from some good speedup. Which is the plan, now that machines are a bit > > more stable ... > This is my intention of speeding up IGT as well, tests that don't care can use > the inherited pipe/connector. It will speed up testing a lot by preventing even > performing a modeset. > > I also plan to add a function to read the current state through igt_display_read_hw_state(), > then find an active pipe/connector, or just the first combination if nothing matches. > And after the end of the test keep it alive without disabling the crtc's, only removing framebuffers, > which no longer disables the crtc's on i915. :-) Hm, for many things we do want to test different CRTC (e.g. this exact test here), and then modesets are fairly unavoidable. But if we batch them up (and create e.g. pipe-* subtests so we can batch them up without modesets) we should get mostly there. Either way, with my blindness rectified, and if you pimp the commit message a bit: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > ~Maarten
diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c index 47fd10fb9078..004f0e6104ee 100644 --- a/tests/kms_vblank.c +++ b/tests/kms_vblank.c @@ -121,7 +121,6 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) igt_display_t *display = &data->display; igt_output_t *output; enum pipe p; - unsigned int valid_tests = 0; for_each_pipe_with_valid_output(display, p, output) { igt_hang_t hang; @@ -170,11 +169,60 @@ static void run_test(data_t *data, int fd, void (*testfunc)(data_t *, int, int)) /* cleanup what prepare_crtc() has done */ cleanup_crtc(data, fd, output); - valid_tests++; } +} + +static void crtc_id_subtest(data_t *data, int fd) +{ + igt_display_t *display = &data->display; + igt_output_t *output; + enum pipe p; + + for_each_pipe_with_valid_output(display, p, output) { + struct drm_event_vblank buf; + const uint32_t pipe_id_flag = kmstest_get_vbl_flag(p); + unsigned crtc_id, expected_crtc_id; + uint64_t val; + union drm_wait_vblank vbl; + + crtc_id = display->pipes[p].crtc_id; + if (drmGetCap(display->drm_fd, DRM_CAP_CRTC_IN_VBLANK_EVENT, &val) == 0) + expected_crtc_id = crtc_id; + else + expected_crtc_id = 0; + + data->pipe = p; + prepare_crtc(data, fd, output); + + memset(&vbl, 0, sizeof(vbl)); + vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT; + vbl.request.type |= pipe_id_flag; + vbl.request.sequence = 1; + igt_assert_eq(wait_vblank(fd, &vbl), 0); + + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); + igt_assert_eq(buf.crtc_id, expected_crtc_id); + + do_or_die(drmModePageFlip(fd, crtc_id, + data->primary_fb.fb_id, + DRM_MODE_PAGE_FLIP_EVENT, NULL)); + + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); + igt_assert_eq(buf.crtc_id, expected_crtc_id); + + if (display->is_atomic) { + igt_plane_t *primary = igt_output_get_plane(output, 0); + + igt_plane_set_fb(primary, &data->primary_fb); + igt_display_commit_atomic(display, DRM_MODE_PAGE_FLIP_EVENT, NULL); - igt_require_f(valid_tests, - "no valid crtc/connector combinations found\n"); + igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf)); + igt_assert_eq(buf.crtc_id, expected_crtc_id); + } + + cleanup_crtc(data, fd, output); + return; + } } static void accuracy(data_t *data, int fd, int nchildren) @@ -307,8 +355,12 @@ igt_main fd = drm_open_driver(DRIVER_ANY); kmstest_set_vt_graphics_mode(); igt_display_init(&data.display, fd); + igt_display_require_output(&data.display); } + igt_subtest("crtc-id") + crtc_id_subtest(&data, fd); + for (f = funcs; f->name; f++) { for (m = modes; m->name; m++) { if (m->flags & ~f->valid)
This was implemented correctly only on the atomic ioctl before, but it should really be working on all 3 ioctl's involved, so ensure we always set crtc_id correctly with a testcase. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- tests/kms_vblank.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 4 deletions(-)