| Message ID | 1435671669-12572-2-git-send-email-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: > @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) > set_mode_for_params(&prim_mode_params); > > sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); > - igt_assert(sink_crc.fd >= 0); > + igt_assert_lte(0, sink_crc.fd); This one is wrong, and similar transformations. > > rc = read(sink_crc.fd, crc.data, SINK_CRC_SIZE); > errno_ = errno; > @@ -1184,7 +1184,7 @@ static bool fbc_supported_on_chipset(void) > static void setup_fbc(void) > { > fbc.fd = igt_debugfs_open("i915_fbc_status", O_RDONLY); > - igt_assert(fbc.fd >= 0); > + igt_assert_lte(0, fbc.fd); > > if (!fbc_supported_on_chipset()) { > igt_info("Can't test FBC: not supported on this chipset\n"); > @@ -1220,7 +1220,7 @@ static void setup_psr(void) > } > > psr.fd = igt_debugfs_open("i915_edp_psr_status", O_RDONLY); > - igt_assert(psr.fd >= 0); > + igt_assert_lte(0, psr.fd); > > if (!psr_sink_has_support()) { > igt_info("Can't test PSR: not supported by sink.\n"); > @@ -1426,13 +1426,13 @@ static void set_cursor_for_test(const struct test_mode *t, > fill_fb_region(¶ms->cursor, 0xFF0000FF); > > rc = drmModeMoveCursor(drm.fd, params->crtc_id, 0, 0); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); As a general comment, not on your patch, this assert doesn't provide anywhere near the right information. rc here is either -1 or 0, it's the errno that's interesting (fortunately also printed by the assert), but it is igt_assert_eq(drmModeModeCursor(drm.fd, params->crtc_id, 0, 0), 0); that provides the most useful immediate debugging aid. -Chris
2015-06-30 10:41 GMT-03:00 Michel Thierry <michel.thierry@intel.com>: > Also include script output. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> I've asked a few times already whether igt.cocci is a requirement or not, and nobody ever answered me. Can we please have an official answer/discussion before we merge a patch running igt.cocci on the whole codebase? See my previous emails for further arguments. > --- > lib/igt.cocci | 42 +++++++++++++++++++++++++++++++ > tests/gem_cs_tlb.c | 2 +- > tests/kms_draw_crc.c | 6 ++--- > tests/kms_frontbuffer_tracking.c | 54 ++++++++++++++++++++-------------------- > tests/kms_panel_fitting.c | 2 +- > tests/kms_pipe_b_c_ivb.c | 30 +++++++++++----------- > tests/kms_plane_scaling.c | 2 +- > tests/kms_rotation_crc.c | 4 +-- > tests/pm_backlight.c | 20 +++++++-------- > 9 files changed, 102 insertions(+), 60 deletions(-) > > diff --git a/lib/igt.cocci b/lib/igt.cocci > index 3aee72f..f6b33fb 100644 > --- a/lib/igt.cocci > +++ b/lib/igt.cocci > @@ -174,6 +174,48 @@ int E3, E4; > + igt_assert_lt(E3, E4); > ) > > + > +// Use comparison macros instead of raw igt_assert when possible (double variables) > +@@ > +double E1, E2; > +@@ > +( > +- igt_assert(E1 == E2); > ++ igt_assert_eq_double(E1, E2); > +) > + > +// Use comparison macros instead of raw igt_assert when possible (64-bit variables) > +@@ > +typedef uint64_t; > +uint64_t E1, E2; > +long long E3, E4; > +@@ > +( > +- igt_assert(E1 == E2); > ++ igt_assert_eq_u64(E1, E2); > +| > +- igt_assert(E1 != E2); > ++ igt_assert_neq64(E1, E2); > +| > +- igt_assert(E1 <= E2); > ++ igt_assert_lte64(E1, E2); > +| > +- igt_assert(E1 < E2); > ++ igt_assert_lt64(E1, E2); > +| > +- igt_assert(E3 == E4); > ++ igt_assert_eq64(E3, E4); > +| > +- igt_assert(E3 != E4); > ++ igt_assert_neq64(E3, E4); > +| > +- igt_assert(E3 <= E4); > ++ igt_assert_lte64(E3, E4); > +| > +- igt_assert(E3 < E4); > ++ igt_assert_lt64(E3, E4); > +) > + > // avoid unused-result warnings when compiling with _FORTIFY_SOURCE defined > @@ > identifier func =~ "^(read|write)$"; > diff --git a/tests/gem_cs_tlb.c b/tests/gem_cs_tlb.c > index 62f446c..e4f6d28 100644 > --- a/tests/gem_cs_tlb.c > +++ b/tests/gem_cs_tlb.c > @@ -135,7 +135,7 @@ static void run_on_ring(int fd, unsigned ring_id, const char *ring_name) > > if (split > 0) { > /* Check that we've managed to collide in the tlb. */ > - igt_assert(gtt_offset == gtt_offset_new); > + igt_assert_eq_u64(gtt_offset, gtt_offset_new); > > /* We hang onto the storage of the old batch by keeping > * the cpu mmap around. */ > diff --git a/tests/kms_draw_crc.c b/tests/kms_draw_crc.c > index 9fcf997..88cee1e 100644 > --- a/tests/kms_draw_crc.c > +++ b/tests/kms_draw_crc.c > @@ -100,7 +100,7 @@ static void get_method_crc(enum igt_draw_method method, uint64_t tiling, > > rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0, > &ms.connector_id, 1, ms.mode); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > igt_pipe_crc_collect_crc(pipe_crc, crc); > > @@ -141,7 +141,7 @@ static void get_fill_crc(uint64_t tiling, igt_crc_t *crc) > > rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0, > &ms.connector_id, 1, ms.mode); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > igt_pipe_crc_collect_crc(pipe_crc, crc); > > @@ -167,7 +167,7 @@ static void fill_fb_subtest(void) > > rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0, > &ms.connector_id, 1, ms.mode); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > igt_pipe_crc_collect_crc(pipe_crc, &base_crc); > > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > index 3785a5a..70cb5bc 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -821,7 +821,7 @@ static struct rect pat4_get_rect(struct fb_region *fb, int r) > { > struct rect rect; > > - igt_assert(r == 0); > + igt_assert_eq(r, 0); > > rect.x = 0; > rect.y = 0; > @@ -871,16 +871,16 @@ static void unset_all_crtcs(void) > for (i = 0; i < drm.res->count_crtcs; i++) { > rc = drmModeSetCrtc(drm.fd, drm.res->crtcs[i], -1, 0, 0, NULL, > 0, NULL); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > rc = drmModeSetCursor(drm.fd, drm.res->crtcs[i], 0, 0, 0); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > } > > for (i = 0; i < drm.planes->count_planes; i++) { > rc = drmModeSetPlane(drm.fd, drm.planes->planes[i], 0, 0, 0, 0, > 0, 0, 0, 0, 0, 0, 0); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > } > } > > @@ -914,7 +914,7 @@ static void start_busy_thread(struct igt_fb *fb) > busy_thread.height = fb->height; > > rc = pthread_create(&busy_thread.thread, NULL, busy_thread_func, NULL); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > } > > static void stop_busy_thread(void) > @@ -967,7 +967,7 @@ static void init_blue_crc(void) > rc = drmModeSetCrtc(drm.fd, prim_mode_params.crtc_id, > blue.fb_id, 0, 0, &prim_mode_params.connector_id, 1, > prim_mode_params.mode); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > collect_crcs(&blue_crc); > > print_crc("Blue CRC: ", &blue_crc); > @@ -1010,7 +1010,7 @@ static void init_crcs(struct draw_pattern_info *pattern) > tmp_fbs[r].fb_id, 0, 0, > &prim_mode_params.connector_id, 1, > prim_mode_params.mode); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > collect_crcs(&pattern->crcs[r]); > } > > @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) > set_mode_for_params(&prim_mode_params); > > sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); > - igt_assert(sink_crc.fd >= 0); > + igt_assert_lte(0, sink_crc.fd); > > rc = read(sink_crc.fd, crc.data, SINK_CRC_SIZE); > errno_ = errno; > @@ -1184,7 +1184,7 @@ static bool fbc_supported_on_chipset(void) > static void setup_fbc(void) > { > fbc.fd = igt_debugfs_open("i915_fbc_status", O_RDONLY); > - igt_assert(fbc.fd >= 0); > + igt_assert_lte(0, fbc.fd); > > if (!fbc_supported_on_chipset()) { > igt_info("Can't test FBC: not supported on this chipset\n"); > @@ -1220,7 +1220,7 @@ static void setup_psr(void) > } > > psr.fd = igt_debugfs_open("i915_edp_psr_status", O_RDONLY); > - igt_assert(psr.fd >= 0); > + igt_assert_lte(0, psr.fd); > > if (!psr_sink_has_support()) { > igt_info("Can't test PSR: not supported by sink.\n"); > @@ -1426,13 +1426,13 @@ static void set_cursor_for_test(const struct test_mode *t, > fill_fb_region(¶ms->cursor, 0xFF0000FF); > > rc = drmModeMoveCursor(drm.fd, params->crtc_id, 0, 0); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > rc = drmModeSetCursor(drm.fd, params->crtc_id, > params->cursor.fb->gem_handle, > params->cursor.w, > params->cursor.h); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > do_assertions(ASSERT_NO_ACTION_CHANGE); > } > @@ -1449,7 +1449,7 @@ static void set_sprite_for_test(const struct test_mode *t, > params->sprite.w, params->sprite.h, > 0, 0, params->sprite.w << 16, > params->sprite.h << 16); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > do_assertions(ASSERT_NO_ACTION_CHANGE); > } > @@ -1830,7 +1830,7 @@ static void flip_subtest(const struct test_mode *t) > > rc = drmModePageFlip(drm.fd, params->crtc_id, > params->fb.fb->fb_id, 0, NULL); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > do_assertions(assertions); > } > @@ -1875,7 +1875,7 @@ static void move_subtest(const struct test_mode *t) > case PLANE_CUR: > rc = drmModeMoveCursor(drm.fd, params->crtc_id, rect.x, > rect.y); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > break; > case PLANE_SPR: > rc = drmModeSetPlane(drm.fd, params->sprite_id, > @@ -1884,7 +1884,7 @@ static void move_subtest(const struct test_mode *t) > rect.x, rect.y, rect.w, > rect.h, 0, 0, rect.w << 16, > rect.h << 16); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > break; > default: > igt_assert(false); > @@ -1936,13 +1936,13 @@ static void onoff_subtest(const struct test_mode *t) > case PLANE_CUR: > rc = drmModeSetCursor(drm.fd, params->crtc_id, > 0, 0, 0); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > break; > case PLANE_SPR: > rc = drmModeSetPlane(drm.fd, params->sprite_id, > 0, 0, 0, 0, 0, 0, 0, 0, 0, > 0, 0); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > break; > default: > igt_assert(false); > @@ -1956,7 +1956,7 @@ static void onoff_subtest(const struct test_mode *t) > params->cursor.fb->gem_handle, > params->cursor.w, > params->cursor.h); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > break; > case PLANE_SPR: > rc = drmModeSetPlane(drm.fd, params->sprite_id, > @@ -1967,7 +1967,7 @@ static void onoff_subtest(const struct test_mode *t) > 0, > params->sprite.w << 16, > params->sprite.h << 16); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > break; > default: > igt_assert(false); > @@ -2014,7 +2014,7 @@ static void fullscreen_plane_subtest(const struct test_mode *t) > fullscreen_fb.height, 0, 0, > fullscreen_fb.width << 16, > fullscreen_fb.height << 16); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > update_wanted_crc(t, &pattern->crcs[0]); > > switch (t->screen) { > @@ -2032,7 +2032,7 @@ static void fullscreen_plane_subtest(const struct test_mode *t) > > rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 0, > 0, 0, 0); > - igt_assert(rc == 0); > + igt_assert_eq(rc, 0); > > if (t->screen == SCREEN_PRIM) > assertions = ASSERT_LAST_ACTION_CHANGED; > @@ -2114,23 +2114,23 @@ static int opt_handler(int option, int option_index, void *data) > opt.step++; > break; > case 'n': > - igt_assert(opt.only_feature == FEATURE_COUNT); > + igt_assert_eq(opt.only_feature, FEATURE_COUNT); > opt.only_feature = FEATURE_NONE; > break; > case 'f': > - igt_assert(opt.only_feature == FEATURE_COUNT); > + igt_assert_eq(opt.only_feature, FEATURE_COUNT); > opt.only_feature = FEATURE_FBC; > break; > case 'p': > - igt_assert(opt.only_feature == FEATURE_COUNT); > + igt_assert_eq(opt.only_feature, FEATURE_COUNT); > opt.only_feature = FEATURE_PSR; > break; > case '1': > - igt_assert(opt.only_pipes == PIPE_COUNT); > + igt_assert_eq(opt.only_pipes, PIPE_COUNT); > opt.only_pipes = PIPE_SINGLE; > break; > case '2': > - igt_assert(opt.only_pipes == PIPE_COUNT); > + igt_assert_eq(opt.only_pipes, PIPE_COUNT); > opt.only_pipes = PIPE_DUAL; > break; > default: > diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c > index 2d079b4..4e789dc 100644 > --- a/tests/kms_panel_fitting.c > +++ b/tests/kms_panel_fitting.c > @@ -127,7 +127,7 @@ static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe, > &output->id, > 1, > mode); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > } else { > igt_display_commit2(display, s); > } > diff --git a/tests/kms_pipe_b_c_ivb.c b/tests/kms_pipe_b_c_ivb.c > index b6e234c..9a0a336 100644 > --- a/tests/kms_pipe_b_c_ivb.c > +++ b/tests/kms_pipe_b_c_ivb.c > @@ -96,7 +96,7 @@ set_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output) > mode->hdisplay, mode->vdisplay, > DRM_FORMAT_XRGB8888, I915_TILING_NONE, > 1.0, 1.0, 1.0, &fb); > - igt_assert(fb_id >= 0); > + igt_assert_lte(0, fb_id); > > igt_plane_set_fb(primary, &fb); > return igt_display_try_commit2(&data->display, COMMIT_LEGACY); > @@ -152,12 +152,12 @@ test_dpms(data_t *data) > kmstest_pipe_name(PIPE_C), igt_output_name(output2)); > > ret = set_big_mode_on_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > kmstest_set_connector_dpms(data->drm_fd, output1->config.connector, DRM_MODE_DPMS_OFF); > > ret = set_big_mode_on_pipe(data, PIPE_C, output2); > - igt_assert(ret != 0); > + igt_assert_neq(ret, 0); > } > > static void > @@ -174,13 +174,13 @@ test_lane_reduction(data_t *data) > kmstest_pipe_name(PIPE_C), igt_output_name(output2)); > > ret = set_big_mode_on_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = set_normal_mode_on_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = set_normal_mode_on_pipe(data, PIPE_C, output2); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > } > > static void > @@ -197,16 +197,16 @@ test_disable_pipe_B(data_t *data) > kmstest_pipe_name(PIPE_C), igt_output_name(output2)); > > ret = set_big_mode_on_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = disable_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = set_normal_mode_on_pipe(data, PIPE_C, output2); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = set_normal_mode_on_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > } > > static void > @@ -223,13 +223,13 @@ test_from_C_to_B_with_3_lanes(data_t *data) > kmstest_pipe_name(PIPE_C), igt_output_name(output2)); > > ret = set_normal_mode_on_pipe(data, PIPE_C, output2); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = disable_pipe(data, PIPE_C, output2); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = set_big_mode_on_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > } > > static void > @@ -246,10 +246,10 @@ test_fail_enable_pipe_C_while_B_has_3_lanes(data_t *data) > kmstest_pipe_name(PIPE_C), igt_output_name(output2)); > > ret = set_big_mode_on_pipe(data, PIPE_B, output1); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > > ret = set_normal_mode_on_pipe(data, PIPE_C, output2); > - igt_assert(ret != 0); > + igt_assert_neq(ret, 0); > } > > static data_t data; > diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c > index 00db5cb..77c57ab 100644 > --- a/tests/kms_plane_scaling.c > +++ b/tests/kms_plane_scaling.c > @@ -129,7 +129,7 @@ static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe, > &output->id, > 1, > mode); > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > } else { > igt_display_commit2(display, s); > } > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c > index 3fd77c4..c61c3f8 100644 > --- a/tests/kms_rotation_crc.c > +++ b/tests/kms_rotation_crc.c > @@ -240,9 +240,9 @@ static void test_plane_rotation(data_t *data, enum igt_plane plane_type) > igt_plane_set_rotation(plane, data->rotation); > ret = igt_display_try_commit2(display, commit); > if (data->override_fmt || data->override_tiling) { > - igt_assert(ret == -EINVAL); > + igt_assert_eq(ret, -EINVAL); > } else { > - igt_assert(ret == 0); > + igt_assert_eq(ret, 0); > igt_pipe_crc_collect_crc(data->pipe_crc, > &crc_output); > igt_assert_crc_equal(&data->ref_crc, > diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c > index d02336d..136e127 100644 > --- a/tests/pm_backlight.c > +++ b/tests/pm_backlight.c > @@ -95,12 +95,12 @@ static void test_and_verify(int val) > { > int result; > > - igt_assert(backlight_write(val, "brightness") == 0); > - igt_assert(backlight_read(&result, "brightness") == 0); > + igt_assert_eq(backlight_write(val, "brightness"), 0); > + igt_assert_eq(backlight_read(&result, "brightness"), 0); > /* Check that the exact value sticks */ > - igt_assert(result == val); > + igt_assert_eq(result, val); > > - igt_assert(backlight_read(&result, "actual_brightness") == 0); > + igt_assert_eq(backlight_read(&result, "actual_brightness"), 0); > /* Some rounding may happen depending on hw. Just check that it's close enough. */ > igt_assert(result <= val + val * TOLERANCE / 100 && result >= val - val * TOLERANCE / 100); > } > @@ -118,15 +118,15 @@ static void test_bad_brightness(int max) > /* First write some sane value */ > backlight_write(max / 2, "brightness"); > /* Writing invalid values should fail and not change the value */ > - igt_assert(backlight_write(-1, "brightness") < 0); > + igt_assert_lt(backlight_write(-1, "brightness"), 0); > backlight_read(&val, "brightness"); > - igt_assert(val == max / 2); > - igt_assert(backlight_write(max + 1, "brightness") < 0); > + igt_assert_eq(val, max / 2); > + igt_assert_lt(backlight_write(max + 1, "brightness"), 0); > backlight_read(&val, "brightness"); > - igt_assert(val == max / 2); > - igt_assert(backlight_write(INT_MAX, "brightness") < 0); > + igt_assert_eq(val, max / 2); > + igt_assert_lt(backlight_write(INT_MAX, "brightness"), 0); > backlight_read(&val, "brightness"); > - igt_assert(val == max / 2); > + igt_assert_eq(val, max / 2); > } > > static void test_fade(int max) > -- > 2.4.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 6/30/2015 2:54 PM, Chris Wilson wrote: > On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: >> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) >> set_mode_for_params(&prim_mode_params); >> >> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); >> - igt_assert(sink_crc.fd >= 0); >> + igt_assert_lte(0, sink_crc.fd); > > This one is wrong, and similar transformations. I also saw it wrong at the beginning... But, I think it's correct because coccinelle changed the operands order (the macro is checking for less-than or equals to). - igt_assert(E3 <= E4); + igt_assert_lte(E3, E4); >> >> rc = read(sink_crc.fd, crc.data, SINK_CRC_SIZE); >> errno_ = errno; >> @@ -1184,7 +1184,7 @@ static bool fbc_supported_on_chipset(void) >> static void setup_fbc(void) >> { >> fbc.fd = igt_debugfs_open("i915_fbc_status", O_RDONLY); >> - igt_assert(fbc.fd >= 0); >> + igt_assert_lte(0, fbc.fd); >> >> if (!fbc_supported_on_chipset()) { >> igt_info("Can't test FBC: not supported on this chipset\n"); >> @@ -1220,7 +1220,7 @@ static void setup_psr(void) >> } >> >> psr.fd = igt_debugfs_open("i915_edp_psr_status", O_RDONLY); >> - igt_assert(psr.fd >= 0); >> + igt_assert_lte(0, psr.fd); >> >> if (!psr_sink_has_support()) { >> igt_info("Can't test PSR: not supported by sink.\n"); >> @@ -1426,13 +1426,13 @@ static void set_cursor_for_test(const struct test_mode *t, >> fill_fb_region(¶ms->cursor, 0xFF0000FF); >> >> rc = drmModeMoveCursor(drm.fd, params->crtc_id, 0, 0); >> - igt_assert(rc == 0); >> + igt_assert_eq(rc, 0); > > As a general comment, not on your patch, this assert doesn't provide > anywhere near the right information. rc here is either -1 or 0, it's the > errno that's interesting (fortunately also printed by the assert), but > it is igt_assert_eq(drmModeModeCursor(drm.fd, params->crtc_id, 0, 0), 0); > that provides the most useful immediate debugging aid. > -Chris >
2015-06-30 10:54 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: >> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) >> set_mode_for_params(&prim_mode_params); >> >> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); >> - igt_assert(sink_crc.fd >= 0); >> + igt_assert_lte(0, sink_crc.fd); >> This one is wrong, and similar transformations. Maybe I'm not intelligent enough, but I _really_ think these inequality comparison macros are very hard to read, and the value they add does not compensate the readability problem they bring, especially since, as you pointed, in a lot of cases, the errno is what's important. I'd love to _not_ have that on IGT. The fact that you and Michel are discussing whether the macro is correct or not kinda proves my point on readability. I don't really want to check which one of you is correct because it's going to take some time reading the macro definition, and I've done it before and didn't like it. Reading the plain original assertion is always easy and instantaneous. Also, most of the assertions on IGT are "just in case" assertions that should probably never happen. I'm in favor of the idea that we should only "instrument" the important assertions that are likely to fail, while all the others should just be readable. >> >> rc = read(sink_crc.fd, crc.data, SINK_CRC_SIZE); >> errno_ = errno; >> @@ -1184,7 +1184,7 @@ static bool fbc_supported_on_chipset(void) >> static void setup_fbc(void) >> { >> fbc.fd = igt_debugfs_open("i915_fbc_status", O_RDONLY); >> - igt_assert(fbc.fd >= 0); >> + igt_assert_lte(0, fbc.fd); >> >> if (!fbc_supported_on_chipset()) { >> igt_info("Can't test FBC: not supported on this chipset\n"); >> @@ -1220,7 +1220,7 @@ static void setup_psr(void) >> } >> >> psr.fd = igt_debugfs_open("i915_edp_psr_status", O_RDONLY); >> - igt_assert(psr.fd >= 0); >> + igt_assert_lte(0, psr.fd); >> >> if (!psr_sink_has_support()) { >> igt_info("Can't test PSR: not supported by sink.\n"); >> @@ -1426,13 +1426,13 @@ static void set_cursor_for_test(const struct test_mode *t, >> fill_fb_region(¶ms->cursor, 0xFF0000FF); >> >> rc = drmModeMoveCursor(drm.fd, params->crtc_id, 0, 0); >> - igt_assert(rc == 0); >> + igt_assert_eq(rc, 0); > > As a general comment, not on your patch, this assert doesn't provide > anywhere near the right information. rc here is either -1 or 0, it's the > errno that's interesting (fortunately also printed by the assert), but > it is igt_assert_eq(drmModeModeCursor(drm.fd, params->crtc_id, 0, 0), 0); > that provides the most useful immediate debugging aid. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 30, 2015 at 03:08:48PM +0100, Michel Thierry wrote: > On 6/30/2015 2:54 PM, Chris Wilson wrote: > >On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: > >>@@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) > >> set_mode_for_params(&prim_mode_params); > >> > >> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); > >>- igt_assert(sink_crc.fd >= 0); > >>+ igt_assert_lte(0, sink_crc.fd); > > > >This one is wrong, and similar transformations. > > I also saw it wrong at the beginning... > But, I think it's correct because coccinelle changed the operands > order (the macro is checking for less-than or equals to). Apparently my logic stinks. I was thinking that '<' was the logical opposite of '>=' and so that's what I then expected to see. In this case, just igt_assert_fd(sink_crtc.fd) would be more useful. -Chris
On Tue, Jun 30, 2015 at 11:14:54AM -0300, Paulo Zanoni wrote: > 2015-06-30 10:54 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: > >> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) > >> set_mode_for_params(&prim_mode_params); > >> > >> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); > >> - igt_assert(sink_crc.fd >= 0); > >> + igt_assert_lte(0, sink_crc.fd); > >> This one is wrong, and similar transformations. > > Maybe I'm not intelligent enough, but I _really_ think these > inequality comparison macros are very hard to read, and the value they > add does not compensate the readability problem they bring, especially > since, as you pointed, in a lot of cases, the errno is what's > important. I'd love to _not_ have that on IGT. The fact that you and > Michel are discussing whether the macro is correct or not kinda proves > my point on readability. I don't really want to check which one of you > is correct because it's going to take some time reading the macro > definition, and I've done it before and didn't like it. Reading the > plain original assertion is always easy and instantaneous. > > Also, most of the assertions on IGT are "just in case" assertions that > should probably never happen. I'm in favor of the idea that we should > only "instrument" the important assertions that are likely to fail, > while all the others should just be readable. Imo igt_assert_cmpint was definitely useful for all the "did the right value land" testcase. Many of those run in a loop and it's really useful to see what the expected vs. real value is imo. It has gotten a bit out of hand though, and some of the igt.cocci transforms that have been added where plain wrong. But ignoring those hiccups I still think this is somewhat useful. -Daniel
On 01/07/15 14:02, Daniel Vetter wrote: > On Tue, Jun 30, 2015 at 11:14:54AM -0300, Paulo Zanoni wrote: >> 2015-06-30 10:54 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >>> On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: >>>> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) >>>> set_mode_for_params(&prim_mode_params); >>>> >>>> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); >>>> - igt_assert(sink_crc.fd >= 0); >>>> + igt_assert_lte(0, sink_crc.fd); >>>> This one is wrong, and similar transformations. >> >> Maybe I'm not intelligent enough, but I _really_ think these >> inequality comparison macros are very hard to read, and the value they >> add does not compensate the readability problem they bring, especially >> since, as you pointed, in a lot of cases, the errno is what's >> important. I'd love to _not_ have that on IGT. The fact that you and >> Michel are discussing whether the macro is correct or not kinda proves >> my point on readability. I don't really want to check which one of you >> is correct because it's going to take some time reading the macro >> definition, and I've done it before and didn't like it. Reading the >> plain original assertion is always easy and instantaneous. >> >> Also, most of the assertions on IGT are "just in case" assertions that >> should probably never happen. I'm in favor of the idea that we should >> only "instrument" the important assertions that are likely to fail, >> while all the others should just be readable. > > Imo igt_assert_cmpint was definitely useful for all the "did the right > value land" testcase. Many of those run in a loop and it's really useful > to see what the expected vs. real value is imo. It has gotten a bit out of > hand though, and some of the igt.cocci transforms that have been added > where plain wrong. > > But ignoring those hiccups I still think this is somewhat useful. > -Daniel At another company where we were trying to do pretty much this, we defined the assert-comparison macro to take the comparison operator as one of the arguments, thus not destroying readability quite as much: thus assert(a >= b); was transformed to insist(a, >=, b); So the order of operands and the specific comparator remain clearly visible, rather than being interchanged or logically inverted, but the macro can still report both the expected and actual values, and the text of the expressions used for each of them and the comparator. .Dave.
2015-07-03 6:23 GMT-03:00 Dave Gordon <david.s.gordon@intel.com>: > On 01/07/15 14:02, Daniel Vetter wrote: >> >> On Tue, Jun 30, 2015 at 11:14:54AM -0300, Paulo Zanoni wrote: >>> >>> 2015-06-30 10:54 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >>>> >>>> On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: >>>>> >>>>> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) >>>>> set_mode_for_params(&prim_mode_params); >>>>> >>>>> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); >>>>> - igt_assert(sink_crc.fd >= 0); >>>>> + igt_assert_lte(0, sink_crc.fd); >>>>> This one is wrong, and similar transformations. >>> >>> >>> Maybe I'm not intelligent enough, but I _really_ think these >>> inequality comparison macros are very hard to read, and the value they >>> add does not compensate the readability problem they bring, especially >>> since, as you pointed, in a lot of cases, the errno is what's >>> important. I'd love to _not_ have that on IGT. The fact that you and >>> Michel are discussing whether the macro is correct or not kinda proves >>> my point on readability. I don't really want to check which one of you >>> is correct because it's going to take some time reading the macro >>> definition, and I've done it before and didn't like it. Reading the >>> plain original assertion is always easy and instantaneous. >>> >>> Also, most of the assertions on IGT are "just in case" assertions that >>> should probably never happen. I'm in favor of the idea that we should >>> only "instrument" the important assertions that are likely to fail, >>> while all the others should just be readable. >> >> >> Imo igt_assert_cmpint was definitely useful for all the "did the right >> value land" testcase. Many of those run in a loop and it's really useful >> to see what the expected vs. real value is imo. It has gotten a bit out of >> hand though, and some of the igt.cocci transforms that have been added >> where plain wrong. >> >> But ignoring those hiccups I still think this is somewhat useful. >> -Daniel > > > At another company where we were trying to do pretty much this, we defined > the assert-comparison macro to take the comparison operator as one of the > arguments, thus not destroying readability quite as much: > > thus assert(a >= b); was transformed to > > insist(a, >=, b); > > So the order of operands and the specific comparator remain clearly visible, > rather than being interchanged or logically inverted, but the macro can > still report both the expected and actual values, and the text of the > expressions used for each of them and the comparator. I like the idea, so I decided to look at how to implement that. I discovered that igt_assert_cmpint() used to be exactly what you described. Later we added the ncmp argument and started favoring the usage of _lte everywhere... > > .Dave.
On Fri, Jul 03, 2015 at 01:52:04PM -0300, Paulo Zanoni wrote: > 2015-07-03 6:23 GMT-03:00 Dave Gordon <david.s.gordon@intel.com>: > > On 01/07/15 14:02, Daniel Vetter wrote: > >> > >> On Tue, Jun 30, 2015 at 11:14:54AM -0300, Paulo Zanoni wrote: > >>> > >>> 2015-06-30 10:54 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > >>>> > >>>> On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: > >>>>> > >>>>> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) > >>>>> set_mode_for_params(&prim_mode_params); > >>>>> > >>>>> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); > >>>>> - igt_assert(sink_crc.fd >= 0); > >>>>> + igt_assert_lte(0, sink_crc.fd); > >>>>> This one is wrong, and similar transformations. > >>> > >>> > >>> Maybe I'm not intelligent enough, but I _really_ think these > >>> inequality comparison macros are very hard to read, and the value they > >>> add does not compensate the readability problem they bring, especially > >>> since, as you pointed, in a lot of cases, the errno is what's > >>> important. I'd love to _not_ have that on IGT. The fact that you and > >>> Michel are discussing whether the macro is correct or not kinda proves > >>> my point on readability. I don't really want to check which one of you > >>> is correct because it's going to take some time reading the macro > >>> definition, and I've done it before and didn't like it. Reading the > >>> plain original assertion is always easy and instantaneous. > >>> > >>> Also, most of the assertions on IGT are "just in case" assertions that > >>> should probably never happen. I'm in favor of the idea that we should > >>> only "instrument" the important assertions that are likely to fail, > >>> while all the others should just be readable. > >> > >> > >> Imo igt_assert_cmpint was definitely useful for all the "did the right > >> value land" testcase. Many of those run in a loop and it's really useful > >> to see what the expected vs. real value is imo. It has gotten a bit out of > >> hand though, and some of the igt.cocci transforms that have been added > >> where plain wrong. > >> > >> But ignoring those hiccups I still think this is somewhat useful. > >> -Daniel > > > > > > At another company where we were trying to do pretty much this, we defined > > the assert-comparison macro to take the comparison operator as one of the > > arguments, thus not destroying readability quite as much: > > > > thus assert(a >= b); was transformed to > > > > insist(a, >=, b); > > > > So the order of operands and the specific comparator remain clearly visible, > > rather than being interchanged or logically inverted, but the macro can > > still report both the expected and actual values, and the text of the > > expressions used for each of them and the comparator. > > I like the idea, so I decided to look at how to implement that. I > discovered that igt_assert_cmpint() used to be exactly what you > described. Later we added the ncmp argument and started favoring the > usage of _lte everywhere... Well I was the one who added igt_assert_cmpint, but I didn't add all the _lte/gte/eq variants. I don't have a personal opinion about this so I'm totally open to going back to only igt_assert_cmpint everywhere (with cocci this is easy). But we'd need someone to push the discussion and get acks from everyone who seems to care (you have mine already). Matt Roper and Thomas Wood are probably the ones to poke. -Daniel
On 6 July 2015 at 08:48, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jul 03, 2015 at 01:52:04PM -0300, Paulo Zanoni wrote: >> 2015-07-03 6:23 GMT-03:00 Dave Gordon <david.s.gordon@intel.com>: >> > On 01/07/15 14:02, Daniel Vetter wrote: >> >> >> >> On Tue, Jun 30, 2015 at 11:14:54AM -0300, Paulo Zanoni wrote: >> >>> >> >>> 2015-06-30 10:54 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >> >>>> >> >>>> On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote: >> >>>>> >> >>>>> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) >> >>>>> set_mode_for_params(&prim_mode_params); >> >>>>> >> >>>>> sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); >> >>>>> - igt_assert(sink_crc.fd >= 0); >> >>>>> + igt_assert_lte(0, sink_crc.fd); >> >>>>> This one is wrong, and similar transformations. >> >>> >> >>> >> >>> Maybe I'm not intelligent enough, but I _really_ think these >> >>> inequality comparison macros are very hard to read, and the value they >> >>> add does not compensate the readability problem they bring, especially >> >>> since, as you pointed, in a lot of cases, the errno is what's >> >>> important. I'd love to _not_ have that on IGT. The fact that you and >> >>> Michel are discussing whether the macro is correct or not kinda proves >> >>> my point on readability. I don't really want to check which one of you >> >>> is correct because it's going to take some time reading the macro >> >>> definition, and I've done it before and didn't like it. Reading the >> >>> plain original assertion is always easy and instantaneous. >> >>> >> >>> Also, most of the assertions on IGT are "just in case" assertions that >> >>> should probably never happen. I'm in favor of the idea that we should >> >>> only "instrument" the important assertions that are likely to fail, >> >>> while all the others should just be readable. >> >> >> >> >> >> Imo igt_assert_cmpint was definitely useful for all the "did the right >> >> value land" testcase. Many of those run in a loop and it's really useful >> >> to see what the expected vs. real value is imo. It has gotten a bit out of >> >> hand though, and some of the igt.cocci transforms that have been added >> >> where plain wrong. >> >> >> >> But ignoring those hiccups I still think this is somewhat useful. >> >> -Daniel >> > >> > >> > At another company where we were trying to do pretty much this, we defined >> > the assert-comparison macro to take the comparison operator as one of the >> > arguments, thus not destroying readability quite as much: >> > >> > thus assert(a >= b); was transformed to >> > >> > insist(a, >=, b); >> > >> > So the order of operands and the specific comparator remain clearly visible, >> > rather than being interchanged or logically inverted, but the macro can >> > still report both the expected and actual values, and the text of the >> > expressions used for each of them and the comparator. >> >> I like the idea, so I decided to look at how to implement that. I >> discovered that igt_assert_cmpint() used to be exactly what you >> described. Later we added the ncmp argument and started favoring the >> usage of _lte everywhere... > > Well I was the one who added igt_assert_cmpint, but I didn't add all the > _lte/gte/eq variants. I don't have a personal opinion about this so I'm > totally open to going back to only igt_assert_cmpint everywhere (with > cocci this is easy). But we'd need someone to push the discussion and get > acks from everyone who seems to care (you have mine already). Matt Roper > and Thomas Wood are probably the ones to poke. Chris Wilson added the ncmp parameter to igt_assert_cmpint to make the output messages more readable, which lead to the various _lte/_eq/etc. variants. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/lib/igt.cocci b/lib/igt.cocci index 3aee72f..f6b33fb 100644 --- a/lib/igt.cocci +++ b/lib/igt.cocci @@ -174,6 +174,48 @@ int E3, E4; + igt_assert_lt(E3, E4); ) + +// Use comparison macros instead of raw igt_assert when possible (double variables) +@@ +double E1, E2; +@@ +( +- igt_assert(E1 == E2); ++ igt_assert_eq_double(E1, E2); +) + +// Use comparison macros instead of raw igt_assert when possible (64-bit variables) +@@ +typedef uint64_t; +uint64_t E1, E2; +long long E3, E4; +@@ +( +- igt_assert(E1 == E2); ++ igt_assert_eq_u64(E1, E2); +| +- igt_assert(E1 != E2); ++ igt_assert_neq64(E1, E2); +| +- igt_assert(E1 <= E2); ++ igt_assert_lte64(E1, E2); +| +- igt_assert(E1 < E2); ++ igt_assert_lt64(E1, E2); +| +- igt_assert(E3 == E4); ++ igt_assert_eq64(E3, E4); +| +- igt_assert(E3 != E4); ++ igt_assert_neq64(E3, E4); +| +- igt_assert(E3 <= E4); ++ igt_assert_lte64(E3, E4); +| +- igt_assert(E3 < E4); ++ igt_assert_lt64(E3, E4); +) + // avoid unused-result warnings when compiling with _FORTIFY_SOURCE defined @@ identifier func =~ "^(read|write)$"; diff --git a/tests/gem_cs_tlb.c b/tests/gem_cs_tlb.c index 62f446c..e4f6d28 100644 --- a/tests/gem_cs_tlb.c +++ b/tests/gem_cs_tlb.c @@ -135,7 +135,7 @@ static void run_on_ring(int fd, unsigned ring_id, const char *ring_name) if (split > 0) { /* Check that we've managed to collide in the tlb. */ - igt_assert(gtt_offset == gtt_offset_new); + igt_assert_eq_u64(gtt_offset, gtt_offset_new); /* We hang onto the storage of the old batch by keeping * the cpu mmap around. */ diff --git a/tests/kms_draw_crc.c b/tests/kms_draw_crc.c index 9fcf997..88cee1e 100644 --- a/tests/kms_draw_crc.c +++ b/tests/kms_draw_crc.c @@ -100,7 +100,7 @@ static void get_method_crc(enum igt_draw_method method, uint64_t tiling, rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0, &ms.connector_id, 1, ms.mode); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); igt_pipe_crc_collect_crc(pipe_crc, crc); @@ -141,7 +141,7 @@ static void get_fill_crc(uint64_t tiling, igt_crc_t *crc) rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0, &ms.connector_id, 1, ms.mode); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); igt_pipe_crc_collect_crc(pipe_crc, crc); @@ -167,7 +167,7 @@ static void fill_fb_subtest(void) rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0, &ms.connector_id, 1, ms.mode); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); igt_pipe_crc_collect_crc(pipe_crc, &base_crc); diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c index 3785a5a..70cb5bc 100644 --- a/tests/kms_frontbuffer_tracking.c +++ b/tests/kms_frontbuffer_tracking.c @@ -821,7 +821,7 @@ static struct rect pat4_get_rect(struct fb_region *fb, int r) { struct rect rect; - igt_assert(r == 0); + igt_assert_eq(r, 0); rect.x = 0; rect.y = 0; @@ -871,16 +871,16 @@ static void unset_all_crtcs(void) for (i = 0; i < drm.res->count_crtcs; i++) { rc = drmModeSetCrtc(drm.fd, drm.res->crtcs[i], -1, 0, 0, NULL, 0, NULL); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); rc = drmModeSetCursor(drm.fd, drm.res->crtcs[i], 0, 0, 0); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); } for (i = 0; i < drm.planes->count_planes; i++) { rc = drmModeSetPlane(drm.fd, drm.planes->planes[i], 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); } } @@ -914,7 +914,7 @@ static void start_busy_thread(struct igt_fb *fb) busy_thread.height = fb->height; rc = pthread_create(&busy_thread.thread, NULL, busy_thread_func, NULL); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); } static void stop_busy_thread(void) @@ -967,7 +967,7 @@ static void init_blue_crc(void) rc = drmModeSetCrtc(drm.fd, prim_mode_params.crtc_id, blue.fb_id, 0, 0, &prim_mode_params.connector_id, 1, prim_mode_params.mode); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); collect_crcs(&blue_crc); print_crc("Blue CRC: ", &blue_crc); @@ -1010,7 +1010,7 @@ static void init_crcs(struct draw_pattern_info *pattern) tmp_fbs[r].fb_id, 0, 0, &prim_mode_params.connector_id, 1, prim_mode_params.mode); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); collect_crcs(&pattern->crcs[r]); } @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void) set_mode_for_params(&prim_mode_params); sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY); - igt_assert(sink_crc.fd >= 0); + igt_assert_lte(0, sink_crc.fd); rc = read(sink_crc.fd, crc.data, SINK_CRC_SIZE); errno_ = errno; @@ -1184,7 +1184,7 @@ static bool fbc_supported_on_chipset(void) static void setup_fbc(void) { fbc.fd = igt_debugfs_open("i915_fbc_status", O_RDONLY); - igt_assert(fbc.fd >= 0); + igt_assert_lte(0, fbc.fd); if (!fbc_supported_on_chipset()) { igt_info("Can't test FBC: not supported on this chipset\n"); @@ -1220,7 +1220,7 @@ static void setup_psr(void) } psr.fd = igt_debugfs_open("i915_edp_psr_status", O_RDONLY); - igt_assert(psr.fd >= 0); + igt_assert_lte(0, psr.fd); if (!psr_sink_has_support()) { igt_info("Can't test PSR: not supported by sink.\n"); @@ -1426,13 +1426,13 @@ static void set_cursor_for_test(const struct test_mode *t, fill_fb_region(¶ms->cursor, 0xFF0000FF); rc = drmModeMoveCursor(drm.fd, params->crtc_id, 0, 0); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); rc = drmModeSetCursor(drm.fd, params->crtc_id, params->cursor.fb->gem_handle, params->cursor.w, params->cursor.h); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); do_assertions(ASSERT_NO_ACTION_CHANGE); } @@ -1449,7 +1449,7 @@ static void set_sprite_for_test(const struct test_mode *t, params->sprite.w, params->sprite.h, 0, 0, params->sprite.w << 16, params->sprite.h << 16); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); do_assertions(ASSERT_NO_ACTION_CHANGE); } @@ -1830,7 +1830,7 @@ static void flip_subtest(const struct test_mode *t) rc = drmModePageFlip(drm.fd, params->crtc_id, params->fb.fb->fb_id, 0, NULL); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); do_assertions(assertions); } @@ -1875,7 +1875,7 @@ static void move_subtest(const struct test_mode *t) case PLANE_CUR: rc = drmModeMoveCursor(drm.fd, params->crtc_id, rect.x, rect.y); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); break; case PLANE_SPR: rc = drmModeSetPlane(drm.fd, params->sprite_id, @@ -1884,7 +1884,7 @@ static void move_subtest(const struct test_mode *t) rect.x, rect.y, rect.w, rect.h, 0, 0, rect.w << 16, rect.h << 16); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); break; default: igt_assert(false); @@ -1936,13 +1936,13 @@ static void onoff_subtest(const struct test_mode *t) case PLANE_CUR: rc = drmModeSetCursor(drm.fd, params->crtc_id, 0, 0, 0); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); break; case PLANE_SPR: rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); break; default: igt_assert(false); @@ -1956,7 +1956,7 @@ static void onoff_subtest(const struct test_mode *t) params->cursor.fb->gem_handle, params->cursor.w, params->cursor.h); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); break; case PLANE_SPR: rc = drmModeSetPlane(drm.fd, params->sprite_id, @@ -1967,7 +1967,7 @@ static void onoff_subtest(const struct test_mode *t) 0, params->sprite.w << 16, params->sprite.h << 16); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); break; default: igt_assert(false); @@ -2014,7 +2014,7 @@ static void fullscreen_plane_subtest(const struct test_mode *t) fullscreen_fb.height, 0, 0, fullscreen_fb.width << 16, fullscreen_fb.height << 16); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); update_wanted_crc(t, &pattern->crcs[0]); switch (t->screen) { @@ -2032,7 +2032,7 @@ static void fullscreen_plane_subtest(const struct test_mode *t) rc = drmModeSetPlane(drm.fd, params->sprite_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); - igt_assert(rc == 0); + igt_assert_eq(rc, 0); if (t->screen == SCREEN_PRIM) assertions = ASSERT_LAST_ACTION_CHANGED; @@ -2114,23 +2114,23 @@ static int opt_handler(int option, int option_index, void *data) opt.step++; break; case 'n': - igt_assert(opt.only_feature == FEATURE_COUNT); + igt_assert_eq(opt.only_feature, FEATURE_COUNT); opt.only_feature = FEATURE_NONE; break; case 'f': - igt_assert(opt.only_feature == FEATURE_COUNT); + igt_assert_eq(opt.only_feature, FEATURE_COUNT); opt.only_feature = FEATURE_FBC; break; case 'p': - igt_assert(opt.only_feature == FEATURE_COUNT); + igt_assert_eq(opt.only_feature, FEATURE_COUNT); opt.only_feature = FEATURE_PSR; break; case '1': - igt_assert(opt.only_pipes == PIPE_COUNT); + igt_assert_eq(opt.only_pipes, PIPE_COUNT); opt.only_pipes = PIPE_SINGLE; break; case '2': - igt_assert(opt.only_pipes == PIPE_COUNT); + igt_assert_eq(opt.only_pipes, PIPE_COUNT); opt.only_pipes = PIPE_DUAL; break; default: diff --git a/tests/kms_panel_fitting.c b/tests/kms_panel_fitting.c index 2d079b4..4e789dc 100644 --- a/tests/kms_panel_fitting.c +++ b/tests/kms_panel_fitting.c @@ -127,7 +127,7 @@ static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe, &output->id, 1, mode); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); } else { igt_display_commit2(display, s); } diff --git a/tests/kms_pipe_b_c_ivb.c b/tests/kms_pipe_b_c_ivb.c index b6e234c..9a0a336 100644 --- a/tests/kms_pipe_b_c_ivb.c +++ b/tests/kms_pipe_b_c_ivb.c @@ -96,7 +96,7 @@ set_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output) mode->hdisplay, mode->vdisplay, DRM_FORMAT_XRGB8888, I915_TILING_NONE, 1.0, 1.0, 1.0, &fb); - igt_assert(fb_id >= 0); + igt_assert_lte(0, fb_id); igt_plane_set_fb(primary, &fb); return igt_display_try_commit2(&data->display, COMMIT_LEGACY); @@ -152,12 +152,12 @@ test_dpms(data_t *data) kmstest_pipe_name(PIPE_C), igt_output_name(output2)); ret = set_big_mode_on_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); kmstest_set_connector_dpms(data->drm_fd, output1->config.connector, DRM_MODE_DPMS_OFF); ret = set_big_mode_on_pipe(data, PIPE_C, output2); - igt_assert(ret != 0); + igt_assert_neq(ret, 0); } static void @@ -174,13 +174,13 @@ test_lane_reduction(data_t *data) kmstest_pipe_name(PIPE_C), igt_output_name(output2)); ret = set_big_mode_on_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = set_normal_mode_on_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = set_normal_mode_on_pipe(data, PIPE_C, output2); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); } static void @@ -197,16 +197,16 @@ test_disable_pipe_B(data_t *data) kmstest_pipe_name(PIPE_C), igt_output_name(output2)); ret = set_big_mode_on_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = disable_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = set_normal_mode_on_pipe(data, PIPE_C, output2); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = set_normal_mode_on_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); } static void @@ -223,13 +223,13 @@ test_from_C_to_B_with_3_lanes(data_t *data) kmstest_pipe_name(PIPE_C), igt_output_name(output2)); ret = set_normal_mode_on_pipe(data, PIPE_C, output2); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = disable_pipe(data, PIPE_C, output2); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = set_big_mode_on_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); } static void @@ -246,10 +246,10 @@ test_fail_enable_pipe_C_while_B_has_3_lanes(data_t *data) kmstest_pipe_name(PIPE_C), igt_output_name(output2)); ret = set_big_mode_on_pipe(data, PIPE_B, output1); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); ret = set_normal_mode_on_pipe(data, PIPE_C, output2); - igt_assert(ret != 0); + igt_assert_neq(ret, 0); } static data_t data; diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c index 00db5cb..77c57ab 100644 --- a/tests/kms_plane_scaling.c +++ b/tests/kms_plane_scaling.c @@ -129,7 +129,7 @@ static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe, &output->id, 1, mode); - igt_assert(ret == 0); + igt_assert_eq(ret, 0); } else { igt_display_commit2(display, s); } diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c index 3fd77c4..c61c3f8 100644 --- a/tests/kms_rotation_crc.c +++ b/tests/kms_rotation_crc.c @@ -240,9 +240,9 @@ static void test_plane_rotation(data_t *data, enum igt_plane plane_type) igt_plane_set_rotation(plane, data->rotation); ret = igt_display_try_commit2(display, commit); if (data->override_fmt || data->override_tiling) { - igt_assert(ret == -EINVAL); + igt_assert_eq(ret, -EINVAL); } else { - igt_assert(ret == 0); + igt_assert_eq(ret, 0); igt_pipe_crc_collect_crc(data->pipe_crc, &crc_output); igt_assert_crc_equal(&data->ref_crc, diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c index d02336d..136e127 100644 --- a/tests/pm_backlight.c +++ b/tests/pm_backlight.c @@ -95,12 +95,12 @@ static void test_and_verify(int val) { int result; - igt_assert(backlight_write(val, "brightness") == 0); - igt_assert(backlight_read(&result, "brightness") == 0); + igt_assert_eq(backlight_write(val, "brightness"), 0); + igt_assert_eq(backlight_read(&result, "brightness"), 0); /* Check that the exact value sticks */ - igt_assert(result == val); + igt_assert_eq(result, val); - igt_assert(backlight_read(&result, "actual_brightness") == 0); + igt_assert_eq(backlight_read(&result, "actual_brightness"), 0); /* Some rounding may happen depending on hw. Just check that it's close enough. */ igt_assert(result <= val + val * TOLERANCE / 100 && result >= val - val * TOLERANCE / 100); } @@ -118,15 +118,15 @@ static void test_bad_brightness(int max) /* First write some sane value */ backlight_write(max / 2, "brightness"); /* Writing invalid values should fail and not change the value */ - igt_assert(backlight_write(-1, "brightness") < 0); + igt_assert_lt(backlight_write(-1, "brightness"), 0); backlight_read(&val, "brightness"); - igt_assert(val == max / 2); - igt_assert(backlight_write(max + 1, "brightness") < 0); + igt_assert_eq(val, max / 2); + igt_assert_lt(backlight_write(max + 1, "brightness"), 0); backlight_read(&val, "brightness"); - igt_assert(val == max / 2); - igt_assert(backlight_write(INT_MAX, "brightness") < 0); + igt_assert_eq(val, max / 2); + igt_assert_lt(backlight_write(INT_MAX, "brightness"), 0); backlight_read(&val, "brightness"); - igt_assert(val == max / 2); + igt_assert_eq(val, max / 2); } static void test_fade(int max)
Also include script output. Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Michel Thierry <michel.thierry@intel.com> --- lib/igt.cocci | 42 +++++++++++++++++++++++++++++++ tests/gem_cs_tlb.c | 2 +- tests/kms_draw_crc.c | 6 ++--- tests/kms_frontbuffer_tracking.c | 54 ++++++++++++++++++++-------------------- tests/kms_panel_fitting.c | 2 +- tests/kms_pipe_b_c_ivb.c | 30 +++++++++++----------- tests/kms_plane_scaling.c | 2 +- tests/kms_rotation_crc.c | 4 +-- tests/pm_backlight.c | 20 +++++++-------- 9 files changed, 102 insertions(+), 60 deletions(-)