On Tue, Dec 19, 2017 at 12:20:18AM +0000, Srivatsa, Anusha wrote: > > > >-----Original Message----- > >From: daniel.vetter@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > >Vetter > >Sent: Thursday, December 14, 2017 2:14 AM > >To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; Strano, Luis > ><luis.strano@xxxxxxxxx>; Latvala, Petri <petri.latvala@xxxxxxxxx>; Lofstedt, > >Marta <marta.lofstedt@xxxxxxxxx>; Saarinen, Jani <jani.saarinen@xxxxxxxxx> > >Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; Joseph Garvey > ><joseph1.garvey@xxxxxxxxx> > >Subject: Re: [PATCH i-g-t] igt/kms_rotation_crc: Add horizontal flip > >subtest. > > > >On Thu, Nov 23, 2017 at 12:05 AM, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> > >wrote: > >> From: Joseph Garvey <joseph1.garvey@xxxxxxxxx> > >> > >> Test that horizontal flip works with supported rotations. Includes a > >> fix for the unrotated fb which was not being positioned correctly with > >> portrait and landscape rectangles. > >> > >> v2:(from Anusha) > >> - Change 180 degree rotation to follow the rest, use igt_swap(), make > >> flip variable a bool. Format the patch correctly (Ville, Petri > >> Latvala) > >> > >> v3: (From Anusha) > >> - Correct the name of subtests in order to avoid duplication of names > >> (Arek) > >> > >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> > >> Signed-off-by: Joseph Garvey <joseph1.garvey@xxxxxxxxx> > >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> Cc: Petri Latvala <petri.latvala@xxxxxxxxx> > >> Cc: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx> > > > >I didn't see this patch fly by originally, but now Marta pointed out that this skips > >everywhere. We need to rework it. > > > >General principle is that in kms tests we should _not_ have any platform/feature > >checks encoded in the test. Instead, the testcase should check properties to > >figure out whether something should work or not. > > > > > >> --- > >> lib/igt_kms.c | 2 +- > >> lib/igt_kms.h | 5 ++ > >> tests/kms_rotation_crc.c | 198 > >> +++++++++++++++++++++++++++++++++++++---------- > >> 3 files changed, 164 insertions(+), 41 deletions(-) > >> > >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index a572fc6..3034e44 > >> 100644 > >> --- a/lib/igt_kms.c > >> +++ b/lib/igt_kms.c > >> @@ -3050,7 +3050,7 @@ void igt_fb_set_size(struct igt_fb *fb, > >> igt_plane_t *plane, > >> > >> static const char *rotation_name(igt_rotation_t rotation) { > >> - switch (rotation) { > >> + switch (rotation & IGT_ROTATION_MASK) { > >> case IGT_ROTATION_0: > >> return "0°"; > >> case IGT_ROTATION_90: > >> diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 8dc118c..b83a828 > >> 100644 > >> --- a/lib/igt_kms.h > >> +++ b/lib/igt_kms.h > >> @@ -281,8 +281,13 @@ typedef enum { > >> IGT_ROTATION_90 = 1 << 1, > >> IGT_ROTATION_180 = 1 << 2, > >> IGT_ROTATION_270 = 1 << 3, > >> + IGT_REFLECT_X = 1 << 4, > >> + IGT_REFLECT_Y = 1 << 5, > >> } igt_rotation_t; > >> > >> +#define IGT_ROTATION_MASK \ > >> + (IGT_ROTATION_0 | IGT_ROTATION_90 | IGT_ROTATION_180 | > >> +IGT_ROTATION_270) > >> + > >> typedef struct { > >> /*< private >*/ > >> igt_pipe_t *pipe; > >> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c index > >> 5aec8fa..9e13667 100644 > >> --- a/tests/kms_rotation_crc.c > >> +++ b/tests/kms_rotation_crc.c > >> @@ -32,6 +32,7 @@ typedef struct { > >> igt_display_t display; > >> struct igt_fb fb; > >> struct igt_fb fb_reference; > >> + struct igt_fb fb_unrotated; > >> struct igt_fb fb_modeset; > >> struct igt_fb fb_flip; > >> igt_crc_t ref_crc; > >> @@ -43,8 +44,59 @@ typedef struct { > >> uint32_t override_fmt; > >> uint64_t override_tiling; > >> bool flips; > >> + int devid; > >> } data_t; > >> > >> +typedef struct { > >> + float r; > >> + float g; > >> + float b; > >> +} rgb_color_t; > >> + > >> +static void set_color(rgb_color_t *color, float r, float g, float b) > >> +{ > >> + color->r = r; > >> + color->g = g; > >> + color->b = b; > >> +} > >> + > >> +static void rotate_colors(rgb_color_t *tl, rgb_color_t *tr, rgb_color_t *br, > >> + rgb_color_t *bl, igt_rotation_t rotation) { > >> + rgb_color_t bl_tmp, br_tmp, tl_tmp, tr_tmp; > >> + > >> + if (rotation & IGT_REFLECT_X) { > >> + igt_swap(*tl, *tr); > >> + igt_swap(*bl, *br); > >> + } > >> + > >> + if (rotation & IGT_ROTATION_90) { > >> + bl_tmp = *bl; > >> + br_tmp = *br; > >> + tl_tmp = *tl; > >> + tr_tmp = *tr; > >> + *tl = tr_tmp; > >> + *bl = tl_tmp; > >> + *tr = br_tmp; > >> + *br = bl_tmp; > >> + } else if (rotation & IGT_ROTATION_180) { > >> + igt_swap(*tl, *br); > >> + igt_swap(*tr, *bl); > >> + } else if (rotation & IGT_ROTATION_270) { > >> + bl_tmp = *bl; > >> + br_tmp = *br; > >> + tl_tmp = *tl; > >> + tr_tmp = *tr; > >> + *tl = bl_tmp; > >> + *bl = br_tmp; > >> + *tr = tl_tmp; > >> + *br = tr_tmp; > >> + } > >> +} > >> + > >> +#define RGB_COLOR(color) \ > >> + color.r, color.g, color.b > >> + > >> static void > >> paint_squares(data_t *data, igt_rotation_t rotation, > >> struct igt_fb *fb, float o) @@ -52,35 +104,21 @@ > >> paint_squares(data_t *data, igt_rotation_t rotation, > >> cairo_t *cr; > >> unsigned int w = fb->width; > >> unsigned int h = fb->height; > >> + rgb_color_t tl, tr, bl, br; > >> > >> cr = igt_get_cairo_ctx(data->gfx_fd, fb); > >> > >> - if (rotation == IGT_ROTATION_180) { > >> - cairo_translate(cr, w, h); > >> - cairo_rotate(cr, M_PI); > >> - } > >> + set_color(&tl, o, 0.0f, 0.0f); > >> + set_color(&tr, 0.0f, o, 0.0f); > >> + set_color(&br, o, o, o); > >> + set_color(&bl, 0.0f, 0.0f, o); > >> > >> - if (rotation == IGT_ROTATION_90) { > >> - /* Paint 4 squares with width == height in Green, White, > >> - Blue, Red Clockwise order to look like 270 degree rotated*/ > >> - igt_paint_color(cr, 0, 0, w / 2, h / 2, 0.0, o, 0.0); > >> - igt_paint_color(cr, w / 2, 0, w / 2, h / 2, o, o, o); > >> - igt_paint_color(cr, 0, h / 2, w / 2, h / 2, o, 0.0, 0.0); > >> - igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, 0.0, 0.0, o); > >> - } else if (rotation == IGT_ROTATION_270) { > >> - /* Paint 4 squares with width == height in Blue, Red, > >> - Green, White Clockwise order to look like 90 degree rotated*/ > >> - igt_paint_color(cr, 0, 0, w / 2, h / 2, 0.0, 0.0, o); > >> - igt_paint_color(cr, w / 2, 0, w / 2, h / 2, o, 0.0, 0.0); > >> - igt_paint_color(cr, 0, h / 2, w / 2, h / 2, o, o, o); > >> - igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, 0.0, o, 0.0); > >> - } else { > >> - /* Paint with 4 squares of Red, Green, White, Blue Clockwise */ > >> - igt_paint_color(cr, 0, 0, w / 2, h / 2, o, 0.0, 0.0); > >> - igt_paint_color(cr, w / 2, 0, w / 2, h / 2, 0.0, o, 0.0); > >> - igt_paint_color(cr, 0, h / 2, w / 2, h / 2, 0.0, 0.0, o); > >> - igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, o, o, o); > >> - } > >> + rotate_colors(&tl, &tr, &br, &bl, rotation); > >> + > >> + igt_paint_color(cr, 0, 0, w / 2, h / 2, RGB_COLOR(tl)); > >> + igt_paint_color(cr, w / 2, 0, w / 2, h / 2, RGB_COLOR(tr)); > >> + igt_paint_color(cr, 0, h / 2, w / 2, h / 2, RGB_COLOR(bl)); > >> + igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, > >> + RGB_COLOR(br)); > >> > >> cairo_destroy(cr); > >> } > >> @@ -141,6 +179,7 @@ static void remove_fbs(data_t *data) > >> > >> igt_remove_fb(data->gfx_fd, &data->fb); > >> igt_remove_fb(data->gfx_fd, &data->fb_reference); > >> + igt_remove_fb(data->gfx_fd, &data->fb_unrotated); > >> > >> if (data->fb_flip.fb_id) > >> igt_remove_fb(data->gfx_fd, &data->fb_flip); @@ > >> -212,17 +251,12 @@ static void prepare_fbs(data_t *data, igt_output_t > >*output, > >> * For 90/270, we will use create smaller fb so that the rotated > >> * frame can fit in > >> */ > >> - if (data->rotation == IGT_ROTATION_90 || > >> - data->rotation == IGT_ROTATION_270) { > >> + if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270)) { > >> tiling = data->override_tiling ?: > >> LOCAL_I915_FORMAT_MOD_Y_TILED; > >> > >> igt_swap(w, h); > >> } > >> > >> - igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, &data->fb); > >> - > >> - igt_plane_set_rotation(plane, IGT_ROTATION_0); > >> - > >> /* > >> * Create a reference software rotated flip framebuffer. > >> */ > >> @@ -255,8 +289,20 @@ static void prepare_fbs(data_t *data, igt_output_t > >*output, > >> igt_pipe_crc_collect_crc(data->pipe_crc, &data->ref_crc); > >> > >> /* > >> + * Prepare the non-rotated reference fb. > >> + */ > >> + igt_create_fb(data->gfx_fd, ref_w, ref_h, pixel_format, tiling, &data- > >>fb_unrotated); > >> + paint_squares(data, IGT_ROTATION_0, &data->fb_unrotated, 1.0); > >> + igt_plane_set_fb(plane, &data->fb_unrotated); > >> + igt_plane_set_rotation(plane, IGT_ROTATION_0); > >> + if (plane->type != DRM_PLANE_TYPE_CURSOR) > >> + igt_plane_set_position(plane, data->pos_x, data->pos_y); > >> + igt_display_commit2(display, display->is_atomic ? > >> + COMMIT_ATOMIC : COMMIT_UNIVERSAL); > >> + > >> + /* > >> * Prepare the plane with an non-rotated fb let the hw rotate it. > >> */ > >> + igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, > >> + &data->fb); > >> paint_squares(data, IGT_ROTATION_0, &data->fb, 1.0); > >> igt_plane_set_fb(plane, &data->fb); > >> > >> @@ -322,7 +368,7 @@ static void wait_for_pageflip(int fd) > >> igt_assert(drmHandleEvent(fd, &evctx) == 0); } > >> > >> -static void test_plane_rotation(data_t *data, int plane_type) > >> +static void __test_plane_rotation(data_t *data, int plane_type, bool > >> +test_bad_format) > >> { > >> igt_display_t *display = &data->display; > >> igt_output_t *output; > >> @@ -348,6 +394,9 @@ static void test_plane_rotation(data_t *data, int > >plane_type) > >> igt_plane_t *plane; > >> int i; > >> > >> + if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B) > >> + continue; > > > >This check here must be removed. > OK. > >> + > >> igt_output_set_pipe(output, pipe); > >> > >> plane = igt_output_get_plane_type(output, plane_type); > >> @@ -369,14 +418,12 @@ static void test_plane_rotation(data_t *data, int > >plane_type) > >> igt_debug("Testing case %i on pipe %s\n", i, > >kmstest_pipe_name(pipe)); > >> prepare_fbs(data, output, plane, i); > >> > >> - igt_display_commit2(display, commit); > >> - > >> igt_plane_set_rotation(plane, data->rotation); > >> - if (data->rotation == IGT_ROTATION_90 || data->rotation == > >IGT_ROTATION_270) > >> + if (data->rotation & (IGT_ROTATION_90 | > >> + IGT_ROTATION_270)) > >> igt_plane_set_size(plane, > >> data->fb.height, data->fb.width); > >> > >> ret = igt_display_try_commit2(display, commit); > >> - if (data->override_fmt || data->override_tiling) { > >> + if (test_bad_format && (data->override_fmt || > >> + data->override_tiling)) { > >> igt_assert_eq(ret, -EINVAL); > >> continue; > >> } > >> @@ -421,6 +468,16 @@ static void test_plane_rotation(data_t *data, int > >plane_type) > >> igt_require_f(valid_tests, "no valid crtc/connector > >> combinations found\n"); } > >> > >> +static inline void test_bad_plane_rotation(data_t *data, int > >> +plane_type) { > >> + __test_plane_rotation(data, plane_type, true); } > >> + > >> +static inline void test_plane_rotation(data_t *data, int plane_type) > >> +{ > >> + __test_plane_rotation(data, plane_type, false); } > >> + > >> static void test_plane_rotation_ytiled_obj(data_t *data, > >> igt_output_t *output, > >> int plane_type) @@ -613,6 > >> +670,8 @@ static const char *plane_test_str(unsigned plane) static > >> const char *rot_test_str(igt_rotation_t rot) { > >> switch (rot) { > >> + case IGT_ROTATION_0: > >> + return "0"; > >> case IGT_ROTATION_90: > >> return "90"; > >> case IGT_ROTATION_180: > >> @@ -624,6 +683,20 @@ static const char *rot_test_str(igt_rotation_t rot) > >> } > >> } > >> > >> +static const char *tiling_test_str(uint64_t tiling) { > >> + switch (tiling) { > >> + case LOCAL_I915_FORMAT_MOD_X_TILED: > >> + return "x-tiled"; > >> + case LOCAL_I915_FORMAT_MOD_Y_TILED: > >> + return "y-tiled"; > >> + case LOCAL_I915_FORMAT_MOD_Yf_TILED: > >> + return "yf-tiled"; > >> + default: > >> + igt_assert(0); > >> + } > >> +} > >> + > >> static const char *flip_test_str(unsigned flips) { > >> if (flips) > >> @@ -637,7 +710,7 @@ igt_main > >> struct rot_subtest { > >> unsigned plane; > >> igt_rotation_t rot; > >> - unsigned flips; > >> + bool flips; > >> } *subtest, subtests[] = { > >> { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_90, 0 }, > >> { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_180, 0 }, @@ > >> -654,6 +727,35 @@ igt_main > >> { DRM_PLANE_TYPE_CURSOR, IGT_ROTATION_180, 0 }, > >> { 0, 0, 0} > >> }; > >> + > >> + struct reflect_x { > >> + uint64_t tiling; > >> + igt_rotation_t rot; > >> + bool flips; > >> + } *reflect_x, reflect_x_subtests[] = { > >> + { LOCAL_I915_FORMAT_MOD_X_TILED, IGT_ROTATION_0, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_X_TILED, IGT_ROTATION_180, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_0, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_90, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_180, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_270, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_0, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_90, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_180, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_270, 0 }, > >> + { LOCAL_I915_FORMAT_MOD_X_TILED, IGT_ROTATION_0, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_X_TILED, IGT_ROTATION_180, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_0, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_90, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_180, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Y_TILED, IGT_ROTATION_270, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_0, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_90, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_180, 1 }, > >> + { LOCAL_I915_FORMAT_MOD_Yf_TILED, IGT_ROTATION_270, 1 }, > >> + { 0, 0, 0 } > >> + }; > >> + > >> data_t data = {}; > >> int gen = 0; > >> > >> @@ -661,7 +763,8 @@ igt_main > >> > >> igt_fixture { > >> data.gfx_fd = drm_open_driver_master(DRIVER_INTEL); > >> - gen = intel_gen(intel_get_drm_devid(data.gfx_fd)); > >> + data.devid = intel_get_drm_devid(data.gfx_fd); > >> + gen = intel_gen(data.devid); > >> > >> kmstest_set_vt_graphics_mode(); > >> > >> @@ -698,7 +801,7 @@ igt_main > >> data.pos_y = 0; > >> data.rotation = IGT_ROTATION_90; > >> data.override_fmt = DRM_FORMAT_RGB565; > >> - test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY); > >> + test_bad_plane_rotation(&data, > >> + DRM_PLANE_TYPE_PRIMARY); > >> } > >> > >> igt_subtest_f("bad-tiling") { > >> @@ -706,7 +809,7 @@ igt_main > >> data.override_fmt = 0; > >> data.rotation = IGT_ROTATION_90; > >> data.override_tiling = LOCAL_DRM_FORMAT_MOD_NONE; > >> - test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY); > >> + test_bad_plane_rotation(&data, > >> + DRM_PLANE_TYPE_PRIMARY); > >> } > >> > >> igt_subtest_f("primary-rotation-90-Y-tiled") { @@ -729,6 > >> +832,21 @@ igt_main > >> igt_require_f(valid_tests, "no valid crtc/connector combinations > >found\n"); > >> } > >> > >> + for (reflect_x = reflect_x_subtests; reflect_x->tiling; reflect_x++) { > >> + igt_subtest_f("primary-%s-reflect-x-%s%s", > >> + tiling_test_str(reflect_x->tiling), > >> + rot_test_str(reflect_x->rot), > >> + flip_test_str(reflect_x->flips)) { > >> + igt_require(gen >= 10 || > >> + (IS_CHERRYVIEW(data.devid) && reflect_x->rot == > >IGT_ROTATION_0 > >> + && reflect_x->tiling == > >> + LOCAL_I915_FORMAT_MOD_X_TILED)); > > > >This check here also must be removed and instead we need to check whether the > >rotation property supports the combination of rotation/flipping that we want to > >test. > > > >Anusha, can you pls follow up with this? > > > >Cc'ing Luis too to keep track of this. > Sure Daniel, Thanks a lot for your feedback. > Will roll up a new version of this. It is already merged so a patch to fix is needed instead of a new version. > > Anusha > >Thanks, Daniel > > > >> + data.rotation = (IGT_REFLECT_X | reflect_x->rot); > >> + data.override_tiling = reflect_x->tiling; > >> + data.flips = reflect_x->flips; > >> + test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY); > >> + } > >> + } > >> + > >> igt_subtest_f("exhaust-fences") { > >> enum pipe pipe; > >> igt_output_t *output; > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > >-- > >Daniel Vetter > >Software Engineer, Intel Corporation > >+41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
