| Message ID | 20171213093519.639-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-12-13 at 10:35 +0100, Maarten Lankhorst wrote: > From: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > Compiler complained on crc_lowres and crc_hires2 being uninitialized > and indeed, display_commit_mode() seems to have intention of > retruning > the value through the parameter that is only a single pointer. > > This couses only the local copy of the pointer, the one inside > display_commit_mode(), to be overwritten. > > Let's fix that! > > Also add missing hires crc comparison (M. Kahola). > > v2: make display_commit_mode return just the last CRC > v3: Don't do memory allocations, it's hard. (Maarten) > v4: Use igt_pipe_crc_collect_crc() instead, cleans up crc handling a > lot. > > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> With typo s/couses/causes/ fixed this is Reviewed-by: Mika Kahola <mika.kahola@intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > tests/kms_plane_lowres.c | 36 ++++++++++++------------------------ > 1 file changed, 12 insertions(+), 24 deletions(-) > > diff --git a/tests/kms_plane_lowres.c b/tests/kms_plane_lowres.c > index 85d3145de4b6..c224a1bf7026 100644 > --- a/tests/kms_plane_lowres.c > +++ b/tests/kms_plane_lowres.c > @@ -125,17 +125,12 @@ test_fini(data_t *data, igt_output_t *output, > enum pipe pipe) > data->fb = NULL; > } > > -static int > +static void > display_commit_mode(igt_display_t *display, igt_pipe_crc_t > *pipe_crc, > - enum pipe pipe, int flags, igt_crc_t *crc) > + enum pipe pipe, int flags, igt_crc_t *out_crc) > { > char buf[256]; > - struct drm_event *e = (void *)buf; > - unsigned int vblank_start, vblank_stop; > - int n, ret; > - > - vblank_start = kmstest_get_vblank(display->drm_fd, pipe, > - DRM_VBLANK_NEXTONMISS); > + int ret; > > ret = igt_display_try_commit_atomic(display, flags, NULL); > igt_skip_on(ret != 0); > @@ -144,14 +139,7 @@ display_commit_mode(igt_display_t *display, > igt_pipe_crc_t *pipe_crc, > ret = read(display->drm_fd, buf, sizeof(buf)); > igt_assert(ret >= 0); > > - vblank_stop = kmstest_get_vblank(display->drm_fd, pipe, 0); > - igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE); > - igt_reset_timeout(); > - > - n = igt_pipe_crc_get_crcs(pipe_crc, vblank_stop - > vblank_start, &crc); > - igt_assert_eq(n, vblank_stop - vblank_start); > - > - return n; > + igt_pipe_crc_collect_crc(pipe_crc, out_crc); > } > > static void > @@ -218,11 +206,11 @@ static void > test_plane_position_with_output(data_t *data, enum pipe pipe, > igt_output_t *output, uint64_t > modifier) > { > - igt_crc_t *crc_hires1, *crc_hires2; > - igt_crc_t *crc_lowres; > + igt_crc_t crc_hires1, crc_hires2; > + igt_crc_t crc_lowres; > drmModeModeInfo mode_lowres; > drmModeModeInfo *mode1, *mode2, *mode3; > - int ret, n; > + int ret; > int flags = DRM_MODE_PAGE_FLIP_EVENT | > DRM_MODE_ATOMIC_ALLOW_MODESET; > igt_pipe_crc_t *pipe_crc; > > @@ -237,10 +225,8 @@ test_plane_position_with_output(data_t *data, > enum pipe pipe, > igt_skip_on(ret != 0); > > pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe, > INTEL_PIPE_CRC_SOURCE_AUTO); > - igt_pipe_crc_start(pipe_crc); > > - n = igt_pipe_crc_get_crcs(pipe_crc, 1, &crc_hires1); > - igt_assert_eq(1, n); > + igt_pipe_crc_collect_crc(pipe_crc, &crc_hires1); > > igt_assert_plane_visible(data->drm_fd, pipe, true); > > @@ -252,7 +238,7 @@ test_plane_position_with_output(data_t *data, > enum pipe pipe, > > check_mode(&mode_lowres, mode2); > > - display_commit_mode(&data->display, pipe_crc, pipe, flags, > crc_lowres); > + display_commit_mode(&data->display, pipe_crc, pipe, flags, > &crc_lowres); > > igt_assert_plane_visible(data->drm_fd, pipe, false); > > @@ -264,10 +250,12 @@ test_plane_position_with_output(data_t *data, > enum pipe pipe, > > check_mode(mode1, mode3); > > - display_commit_mode(&data->display, pipe_crc, pipe, flags, > crc_hires2); > + display_commit_mode(&data->display, pipe_crc, pipe, flags, > &crc_hires2); > > igt_assert_plane_visible(data->drm_fd, pipe, true); > > + igt_assert_crc_equal(&crc_hires1, &crc_hires2); > + > igt_pipe_crc_stop(pipe_crc); > igt_pipe_crc_free(pipe_crc); >
On Wed, Dec 13, 2017 at 11:08:39AM +0000, Patchwork wrote: > == Series Details == > > Series: test/kms_plane_lowres: Fix display_commit_mode() so it returns the crc (rev4) > URL : https://patchwork.freedesktop.org/series/34749/ > State : warning > > == Summary == > > Test pm_rc6_residency: > Subgroup rc6-accuracy: > pass -> SKIP (shard-snb) > Test gem_tiled_swapping: > Subgroup non-threaded: > incomplete -> PASS (shard-snb) fdo#104009 > Test gem_pwrite: > Subgroup huge-gtt-backwards: > incomplete -> PASS (shard-hsw) fdo#104218 > Test gem_softpin: > Subgroup noreloc-s4: > fail -> SKIP (shard-snb) fdo#103375 > Test kms_frontbuffer_tracking: > Subgroup fbc-1p-offscren-pri-shrfb-draw-render: > pass -> FAIL (shard-snb) fdo#101623 +1 Okay, finally it's all green on kms_plane_lowres and the tests does what it was intended to do. Thanks for improving on the original patch, the feedback and the reviews. This is now merged with the typos corrected.
diff --git a/tests/kms_plane_lowres.c b/tests/kms_plane_lowres.c index 85d3145de4b6..c224a1bf7026 100644 --- a/tests/kms_plane_lowres.c +++ b/tests/kms_plane_lowres.c @@ -125,17 +125,12 @@ test_fini(data_t *data, igt_output_t *output, enum pipe pipe) data->fb = NULL; } -static int +static void display_commit_mode(igt_display_t *display, igt_pipe_crc_t *pipe_crc, - enum pipe pipe, int flags, igt_crc_t *crc) + enum pipe pipe, int flags, igt_crc_t *out_crc) { char buf[256]; - struct drm_event *e = (void *)buf; - unsigned int vblank_start, vblank_stop; - int n, ret; - - vblank_start = kmstest_get_vblank(display->drm_fd, pipe, - DRM_VBLANK_NEXTONMISS); + int ret; ret = igt_display_try_commit_atomic(display, flags, NULL); igt_skip_on(ret != 0); @@ -144,14 +139,7 @@ display_commit_mode(igt_display_t *display, igt_pipe_crc_t *pipe_crc, ret = read(display->drm_fd, buf, sizeof(buf)); igt_assert(ret >= 0); - vblank_stop = kmstest_get_vblank(display->drm_fd, pipe, 0); - igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE); - igt_reset_timeout(); - - n = igt_pipe_crc_get_crcs(pipe_crc, vblank_stop - vblank_start, &crc); - igt_assert_eq(n, vblank_stop - vblank_start); - - return n; + igt_pipe_crc_collect_crc(pipe_crc, out_crc); } static void @@ -218,11 +206,11 @@ static void test_plane_position_with_output(data_t *data, enum pipe pipe, igt_output_t *output, uint64_t modifier) { - igt_crc_t *crc_hires1, *crc_hires2; - igt_crc_t *crc_lowres; + igt_crc_t crc_hires1, crc_hires2; + igt_crc_t crc_lowres; drmModeModeInfo mode_lowres; drmModeModeInfo *mode1, *mode2, *mode3; - int ret, n; + int ret; int flags = DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_ALLOW_MODESET; igt_pipe_crc_t *pipe_crc; @@ -237,10 +225,8 @@ test_plane_position_with_output(data_t *data, enum pipe pipe, igt_skip_on(ret != 0); pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); - igt_pipe_crc_start(pipe_crc); - n = igt_pipe_crc_get_crcs(pipe_crc, 1, &crc_hires1); - igt_assert_eq(1, n); + igt_pipe_crc_collect_crc(pipe_crc, &crc_hires1); igt_assert_plane_visible(data->drm_fd, pipe, true); @@ -252,7 +238,7 @@ test_plane_position_with_output(data_t *data, enum pipe pipe, check_mode(&mode_lowres, mode2); - display_commit_mode(&data->display, pipe_crc, pipe, flags, crc_lowres); + display_commit_mode(&data->display, pipe_crc, pipe, flags, &crc_lowres); igt_assert_plane_visible(data->drm_fd, pipe, false); @@ -264,10 +250,12 @@ test_plane_position_with_output(data_t *data, enum pipe pipe, check_mode(mode1, mode3); - display_commit_mode(&data->display, pipe_crc, pipe, flags, crc_hires2); + display_commit_mode(&data->display, pipe_crc, pipe, flags, &crc_hires2); igt_assert_plane_visible(data->drm_fd, pipe, true); + igt_assert_crc_equal(&crc_hires1, &crc_hires2); + igt_pipe_crc_stop(pipe_crc); igt_pipe_crc_free(pipe_crc);