Re: igt/kms_rotation_crc: Add horizontal flip subtest.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 20, 2017 at 08:50:37PM -0700, Joseph Garvey wrote:
> 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.
> 
> Signed-off-by: Joseph Garvey <joseph1.garvey@intel.com>
> ---
>  lib/igt_kms.c            |   2 +-
>  lib/igt_kms.h            |   4 +
>  tests/kms_rotation_crc.c | 197 ++++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 166 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 7bcafc0..ec6ffd2 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -3054,7 +3054,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 3d1061f..61393d1 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -281,8 +281,12 @@ typedef enum {
>  	IGT_ROTATION_90  = 1 << 1,
>  	IGT_ROTATION_180 = 1 << 2,
>  	IGT_ROTATION_270 = 1 << 3,
> +	IGT_REFLECT_X    = 1 << 4,

Maybe add REFLECT_Y as well?

>  } 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 21e264a..0e2df96 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,62 @@ 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) {
> +		bl_tmp = *bl;
> +		br_tmp = *br;
> +		tl_tmp = *tl;
> +		tr_tmp = *tr;
> +		*tl = tr_tmp;
> +		*bl = br_tmp;
> +		*tr = tl_tmp;
> +		*br = bl_tmp;

These could use igt_swap()

> +	}
> +
> +	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_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 +107,26 @@ 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) {
> +	set_color(&tl, o, 0, 0);
> +	set_color(&tr, 0, o, 0);
> +	set_color(&br, o, o, o);
> +	set_color(&bl, 0, 0, o);

Maybe write the float constants as actual float constants? Would make
it much more clear that it actually takes floats instead of integers.

> +
> +	rotate_colors(&tl, &tr, &br, &bl, rotation);
> +
> +	if (rotation & IGT_ROTATION_180) {
>  		cairo_translate(cr, w, h);
>  		cairo_rotate(cr, M_PI);
>  	}

This 180 degree special case has always bothered me. Could you remove this
and just do things one way for all angles?

>  
> -	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);
> -	}
> +	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 +187,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 +259,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 +297,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 +376,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;
> @@ -345,6 +399,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;

We should be able to just ask the plane which rotations it supports.

> +
>  		igt_output_set_pipe(output, pipe);
>  
>  		plane = igt_output_get_plane_type(output, plane_type);
> @@ -366,14 +423,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;
>  			}
> @@ -410,6 +465,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)
> @@ -612,6 +677,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:
> @@ -623,6 +690,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)
> @@ -653,6 +734,35 @@ igt_main
>  		{ DRM_PLANE_TYPE_CURSOR, IGT_ROTATION_180, 0 },
>  		{ 0, 0, 0}
>  	};
> +
> +	struct hflip {
> +		uint64_t tiling;
> +		igt_rotation_t rot;
> +		unsigned flips;

bool?

> +	} *hflip, hflip_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;
>  
> @@ -660,7 +770,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();
>  
> @@ -697,7 +808,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") {
> @@ -705,7 +816,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") {
> @@ -728,6 +839,20 @@ igt_main
>  		igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
>  	}
>  
> +	for (hflip = hflip_subtests; hflip->tiling; hflip++) {
> +		igt_subtest_f("primary-h-flip-%s-%s",

I'd call it "reflect-x" or something like that since that's what the
flag is called.

> +			      tiling_test_str(hflip->tiling),
> +			      rot_test_str(hflip->rot)) {
> +			igt_require(gen >= 10 ||
> +				    (IS_CHERRYVIEW(data.devid) && hflip->rot == IGT_ROTATION_0 &&
> +				    hflip->tiling == LOCAL_I915_FORMAT_MOD_X_TILED));
> +			data.rotation = (IGT_REFLECT_X | hflip->rot);
> +			data.override_tiling = hflip->tiling;
> +			data.flips = hflip->flips;
> +			test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> +		}
> +	}
> +
>  	igt_subtest_f("exhaust-fences") {
>  		enum pipe pipe;
>  		igt_output_t *output;
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx