[igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane

[igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane

[Jouni Högander]

Add new testcases which are moving cursor and overlay planes
without setting any damage areas.

Cursor testcase is reproducing following bug:

https://gitlab.freedesktop.org/drm/intel/-/issues/5077

Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 tests/i915/kms_psr2_sf.c | 144 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 136 insertions(+), 8 deletions(-)

diff --git a/tests/i915/kms_psr2_sf.c b/tests/i915/kms_psr2_sf.c
index 36f8a786..b9990f87 100644
--- a/tests/i915/kms_psr2_sf.c
+++ b/tests/i915/kms_psr2_sf.c
@@ -43,6 +43,7 @@ IGT_TEST_DESCRIPTION("Tests to varify PSR2 selective fetch by sending multiple"
 enum operations {
 	PLANE_UPDATE,
 	PLANE_UPDATE_CONTINUOUS,
+	PLANE_MOVE_DAMAGED,
 	PLANE_MOVE,
 	OVERLAY_PRIM_UPDATE
 };
@@ -51,7 +52,9 @@ enum plane_move_postion {
 	POS_TOP_LEFT,
 	POS_TOP_RIGHT,
 	POS_BOTTOM_LEFT,
-	POS_BOTTOM_RIGHT
+	POS_BOTTOM_RIGHT,
+	POS_BOTTOM_LEFT_NEGATIVE,
+	POS_TOP_RIGHT_NEGATIVE,
 };
 
 typedef struct {
@@ -74,6 +77,7 @@ typedef struct {
 	igt_plane_t *test_plane;
 	cairo_t *cr;
 	uint32_t screen_changes;
+	int cur_x, cur_y;
 } data_t;
 
 static const char *op_str(enum operations op)
@@ -81,7 +85,8 @@ static const char *op_str(enum operations op)
 	static const char * const name[] = {
 		[PLANE_UPDATE] = "plane-update",
 		[PLANE_UPDATE_CONTINUOUS] = "plane-update-continuous",
-		[PLANE_MOVE] = "plane-move",
+		[PLANE_MOVE] = "plane-move-without-damage",
+		[PLANE_MOVE_DAMAGED] = "plane-move",
 		[OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
 	};
 
@@ -187,7 +192,7 @@ static void plane_update_setup_squares(data_t *data, igt_fb_t *fb, uint32_t h,
 	}
 }
 
-static void plane_move_setup_square(data_t *data, igt_fb_t *fb, uint32_t h,
+static void plane_move_damaged_setup_square(data_t *data, igt_fb_t *fb, uint32_t h,
 				       uint32_t v)
 {
 	int x = 0, y = 0;
@@ -260,8 +265,8 @@ static void prepare(data_t *data)
 
 		data->fb_continuous = &data->fb_overlay;
 
-		if (data->op == PLANE_MOVE) {
-			plane_move_setup_square(data, &data->fb_test,
+		if (data->op == PLANE_MOVE_DAMAGED) {
+			plane_move_damaged_setup_square(data, &data->fb_test,
 					   data->mode->hdisplay/2,
 					   data->mode->vdisplay/2);
 
@@ -273,6 +278,7 @@ static void prepare(data_t *data)
 
 		igt_plane_set_fb(sprite, &data->fb_overlay);
 		data->test_plane = sprite;
+
 		break;
 
 	case DRM_PLANE_TYPE_PRIMARY:
@@ -335,6 +341,8 @@ static void prepare(data_t *data)
 		igt_assert(false);
 	}
 
+	data->cur_x = data->cur_y = 0;
+
 	igt_plane_set_fb(primary, &data->fb_primary);
 
 	igt_display_commit2(&data->display, COMMIT_ATOMIC);
@@ -378,7 +386,7 @@ static void plane_update_expected_output(int plane_type, int box_count,
 	manual(expected);
 }
 
-static void plane_move_expected_output(enum plane_move_postion pos)
+static void plane_move_damaged_expected_output(enum plane_move_postion pos)
 {
 	char expected[64] = {};
 
@@ -406,6 +414,42 @@ static void plane_move_expected_output(enum plane_move_postion pos)
 	manual(expected);
 }
 
+static void plane_move_expected_output(enum plane_move_postion pos)
+{
+	char expected[73] = {};
+
+	switch (pos) {
+	case POS_TOP_LEFT:
+		sprintf(expected,
+			"screen Green with Blue box on top left corner");
+		break;
+	case POS_TOP_RIGHT:
+		sprintf(expected,
+			"screen Green with Blue box on top right corner");
+		break;
+	case POS_BOTTOM_LEFT:
+		sprintf(expected,
+			"screen Green with Blue box on bottom left corner");
+		break;
+	case POS_BOTTOM_RIGHT:
+		sprintf(expected,
+			"screen Green with Blue box on bottom right corner");
+		break;
+	case POS_BOTTOM_LEFT_NEGATIVE:
+		sprintf(expected,
+			"screen Green with Blue box on bottom left corner (partly exceeding area)");
+		break;
+	case POS_TOP_RIGHT_NEGATIVE:
+		sprintf(expected,
+			"screen Green with Blue box on top right corner (partly exceeding area)");
+		break;
+	default:
+		igt_assert(false);
+	}
+
+	manual(expected);
+}
+
 static void overlay_prim_update_expected_output(int box_count)
 {
 	char expected[64] = {};
@@ -421,6 +465,9 @@ static void overlay_prim_update_expected_output(int box_count)
 static void expected_output(data_t *data)
 {
 	switch (data->op) {
+	case PLANE_MOVE_DAMAGED:
+		plane_move_damaged_expected_output(data->pos);
+		break;
 	case PLANE_MOVE:
 		plane_move_expected_output(data->pos);
 		break;
@@ -487,6 +534,59 @@ static void damaged_plane_move(data_t *data)
 	expected_output(data);
 }
 
+static void plane_move(data_t *data)
+{
+	int target_x, target_y;
+
+	switch (data->pos) {
+	case POS_TOP_LEFT:
+		target_x = 0;
+		target_y = 0;
+		break;
+	case POS_TOP_RIGHT:
+		target_x = data->mode->hdisplay - data->fb_test.width;
+		target_y = 0;
+		break;
+	case POS_TOP_RIGHT_NEGATIVE:
+		target_x = data->mode->hdisplay - data->fb_test.width;
+		target_y = -data->fb_test.width / 2;
+		break;
+	case POS_BOTTOM_LEFT:
+		target_x = 0;
+		target_y = data->mode->vdisplay - data->fb_test.height;
+		break;
+	case POS_BOTTOM_LEFT_NEGATIVE:
+	  target_x = -data->fb_test.width / 2;
+	  target_y = data->mode->vdisplay - data->fb_test.height;
+		break;
+	case POS_BOTTOM_RIGHT:
+	  target_x = data->mode->hdisplay - data->fb_test.width;
+	  target_y = data->mode->vdisplay - data->fb_test.height;
+		break;
+	default:
+		igt_assert(false);
+	}
+
+	while (data->cur_x != target_x || data->cur_y != target_y) {
+		if (data->cur_x < target_x)
+			data->cur_x += min(target_x - data->cur_x, 20);
+		else if (data->cur_x > target_x)
+			data->cur_x -= min(data->cur_x - target_x, 20);
+
+		if (data->cur_y < target_y)
+			data->cur_y += min(target_y - data->cur_y, 20);
+		else if (data->cur_y > target_y)
+			data->cur_y -= min(data->cur_y - target_y, 20);
+
+		igt_plane_set_position(data->test_plane, data->cur_x, data->cur_y);
+		igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	}
+
+	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
+
+	expected_output(data);
+}
+
 static void damaged_plane_update(data_t *data)
 {
 	igt_plane_t *test_plane = data->test_plane;
@@ -526,6 +626,8 @@ static void damaged_plane_update(data_t *data)
 
 static void run(data_t *data)
 {
+	int i;
+
 	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
 
 	data->screen_changes = 0;
@@ -542,9 +644,15 @@ static void run(data_t *data)
 			damaged_plane_update(data);
 		}
 		break;
-	case PLANE_MOVE:
+	case PLANE_MOVE_DAMAGED:
 		damaged_plane_move(data);
 		break;
+	case PLANE_MOVE:
+		for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
+			data->pos = i;
+			plane_move(data);
+		}
+		break;
 	default:
 		igt_assert(false);
 	}
@@ -654,10 +762,20 @@ igt_main
 		cleanup(&data);
 	}
 
-	/* Only for overlay plane */
 	data.op = PLANE_MOVE;
 	/* Verify overlay plane move selective fetch */
 	igt_describe("Test that selective fetch works on moving overlay plane");
+	igt_subtest_f("cursor-%s-sf", op_str(data.op)) {
+		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
+		prepare(&data);
+		run(&data);
+		cleanup(&data);
+	}
+
+	/* Only for overlay plane */
+	data.op = PLANE_MOVE_DAMAGED;
+	/* Verify overlay plane move selective fetch */
+	igt_describe("Test that selective fetch works on moving overlay plane");
 	igt_subtest_f("%s-sf-dmg-area", op_str(data.op)) {
 		for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
 			data.pos = i;
@@ -668,6 +786,16 @@ igt_main
 		}
 	}
 
+	data.op = PLANE_MOVE;
+	/* Verify overlay plane move selective fetch */
+	igt_describe("Test that selective fetch works on moving overlay plane");
+	igt_subtest_f("overlay-%s-sf", op_str(data.op)) {
+		data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
+		prepare(&data);
+		run(&data);
+		cleanup(&data);
+	}
+
 	/* Verify primary plane selective fetch with overplay plane blended */
 	data.op = OVERLAY_PRIM_UPDATE;
 	igt_describe("Test that selective fetch works on primary plane "
-- 
2.32.0

Re: [igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane

[Petri Latvala]

On Tue, Feb 22, 2022 at 12:38:41PM +0200, Jouni Högander wrote:
> Add new testcases which are moving cursor and overlay planes
> without setting any damage areas.
> 
> Cursor testcase is reproducing following bug:
> 
> https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  tests/i915/kms_psr2_sf.c | 144 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 136 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/i915/kms_psr2_sf.c b/tests/i915/kms_psr2_sf.c
> index 36f8a786..b9990f87 100644
> --- a/tests/i915/kms_psr2_sf.c
> +++ b/tests/i915/kms_psr2_sf.c
> @@ -43,6 +43,7 @@ IGT_TEST_DESCRIPTION("Tests to varify PSR2 selective fetch by sending multiple"
>  enum operations {
>  	PLANE_UPDATE,
>  	PLANE_UPDATE_CONTINUOUS,
> +	PLANE_MOVE_DAMAGED,
>  	PLANE_MOVE,
>  	OVERLAY_PRIM_UPDATE
>  };
> @@ -51,7 +52,9 @@ enum plane_move_postion {
>  	POS_TOP_LEFT,
>  	POS_TOP_RIGHT,
>  	POS_BOTTOM_LEFT,
> -	POS_BOTTOM_RIGHT
> +	POS_BOTTOM_RIGHT,
> +	POS_BOTTOM_LEFT_NEGATIVE,
> +	POS_TOP_RIGHT_NEGATIVE,
>  };
>  
>  typedef struct {
> @@ -74,6 +77,7 @@ typedef struct {
>  	igt_plane_t *test_plane;
>  	cairo_t *cr;
>  	uint32_t screen_changes;
> +	int cur_x, cur_y;
>  } data_t;
>  
>  static const char *op_str(enum operations op)
> @@ -81,7 +85,8 @@ static const char *op_str(enum operations op)
>  	static const char * const name[] = {
>  		[PLANE_UPDATE] = "plane-update",
>  		[PLANE_UPDATE_CONTINUOUS] = "plane-update-continuous",
> -		[PLANE_MOVE] = "plane-move",
> +		[PLANE_MOVE] = "plane-move-without-damage",
> +		[PLANE_MOVE_DAMAGED] = "plane-move",
>  		[OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
>  	};
>  
> @@ -187,7 +192,7 @@ static void plane_update_setup_squares(data_t *data, igt_fb_t *fb, uint32_t h,
>  	}
>  }
>  
> -static void plane_move_setup_square(data_t *data, igt_fb_t *fb, uint32_t h,
> +static void plane_move_damaged_setup_square(data_t *data, igt_fb_t *fb, uint32_t h,
>  				       uint32_t v)
>  {
>  	int x = 0, y = 0;
> @@ -260,8 +265,8 @@ static void prepare(data_t *data)
>  
>  		data->fb_continuous = &data->fb_overlay;
>  
> -		if (data->op == PLANE_MOVE) {
> -			plane_move_setup_square(data, &data->fb_test,
> +		if (data->op == PLANE_MOVE_DAMAGED) {
> +			plane_move_damaged_setup_square(data, &data->fb_test,
>  					   data->mode->hdisplay/2,
>  					   data->mode->vdisplay/2);
>  
> @@ -273,6 +278,7 @@ static void prepare(data_t *data)
>  
>  		igt_plane_set_fb(sprite, &data->fb_overlay);
>  		data->test_plane = sprite;
> +
>  		break;
>  
>  	case DRM_PLANE_TYPE_PRIMARY:
> @@ -335,6 +341,8 @@ static void prepare(data_t *data)
>  		igt_assert(false);
>  	}
>  
> +	data->cur_x = data->cur_y = 0;
> +
>  	igt_plane_set_fb(primary, &data->fb_primary);
>  
>  	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> @@ -378,7 +386,7 @@ static void plane_update_expected_output(int plane_type, int box_count,
>  	manual(expected);
>  }
>  
> -static void plane_move_expected_output(enum plane_move_postion pos)
> +static void plane_move_damaged_expected_output(enum plane_move_postion pos)
>  {
>  	char expected[64] = {};
>  
> @@ -406,6 +414,42 @@ static void plane_move_expected_output(enum plane_move_postion pos)
>  	manual(expected);
>  }
>  
> +static void plane_move_expected_output(enum plane_move_postion pos)
> +{
> +	char expected[73] = {};
> +
> +	switch (pos) {
> +	case POS_TOP_LEFT:
> +		sprintf(expected,
> +			"screen Green with Blue box on top left corner");
> +		break;
> +	case POS_TOP_RIGHT:
> +		sprintf(expected,
> +			"screen Green with Blue box on top right corner");
> +		break;
> +	case POS_BOTTOM_LEFT:
> +		sprintf(expected,
> +			"screen Green with Blue box on bottom left corner");
> +		break;
> +	case POS_BOTTOM_RIGHT:
> +		sprintf(expected,
> +			"screen Green with Blue box on bottom right corner");
> +		break;
> +	case POS_BOTTOM_LEFT_NEGATIVE:
> +		sprintf(expected,
> +			"screen Green with Blue box on bottom left corner (partly exceeding area)");
> +		break;
> +	case POS_TOP_RIGHT_NEGATIVE:
> +		sprintf(expected,
> +			"screen Green with Blue box on top right corner (partly exceeding area)");
> +		break;
> +	default:
> +		igt_assert(false);
> +	}
> +
> +	manual(expected);
> +}
> +
>  static void overlay_prim_update_expected_output(int box_count)
>  {
>  	char expected[64] = {};
> @@ -421,6 +465,9 @@ static void overlay_prim_update_expected_output(int box_count)
>  static void expected_output(data_t *data)
>  {
>  	switch (data->op) {
> +	case PLANE_MOVE_DAMAGED:
> +		plane_move_damaged_expected_output(data->pos);
> +		break;
>  	case PLANE_MOVE:
>  		plane_move_expected_output(data->pos);
>  		break;
> @@ -487,6 +534,59 @@ static void damaged_plane_move(data_t *data)
>  	expected_output(data);
>  }
>  
> +static void plane_move(data_t *data)
> +{
> +	int target_x, target_y;
> +
> +	switch (data->pos) {
> +	case POS_TOP_LEFT:
> +		target_x = 0;
> +		target_y = 0;
> +		break;
> +	case POS_TOP_RIGHT:
> +		target_x = data->mode->hdisplay - data->fb_test.width;
> +		target_y = 0;
> +		break;
> +	case POS_TOP_RIGHT_NEGATIVE:
> +		target_x = data->mode->hdisplay - data->fb_test.width;
> +		target_y = -data->fb_test.width / 2;
> +		break;
> +	case POS_BOTTOM_LEFT:
> +		target_x = 0;
> +		target_y = data->mode->vdisplay - data->fb_test.height;
> +		break;
> +	case POS_BOTTOM_LEFT_NEGATIVE:
> +	  target_x = -data->fb_test.width / 2;
> +	  target_y = data->mode->vdisplay - data->fb_test.height;
> +		break;
> +	case POS_BOTTOM_RIGHT:
> +	  target_x = data->mode->hdisplay - data->fb_test.width;
> +	  target_y = data->mode->vdisplay - data->fb_test.height;
> +		break;
> +	default:
> +		igt_assert(false);
> +	}
> +
> +	while (data->cur_x != target_x || data->cur_y != target_y) {
> +		if (data->cur_x < target_x)
> +			data->cur_x += min(target_x - data->cur_x, 20);
> +		else if (data->cur_x > target_x)
> +			data->cur_x -= min(data->cur_x - target_x, 20);
> +
> +		if (data->cur_y < target_y)
> +			data->cur_y += min(target_y - data->cur_y, 20);
> +		else if (data->cur_y > target_y)
> +			data->cur_y -= min(data->cur_y - target_y, 20);
> +
> +		igt_plane_set_position(data->test_plane, data->cur_x, data->cur_y);
> +		igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +	}
> +
> +	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> +
> +	expected_output(data);
> +}
> +
>  static void damaged_plane_update(data_t *data)
>  {
>  	igt_plane_t *test_plane = data->test_plane;
> @@ -526,6 +626,8 @@ static void damaged_plane_update(data_t *data)
>  
>  static void run(data_t *data)
>  {
> +	int i;
> +
>  	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
>  
>  	data->screen_changes = 0;
> @@ -542,9 +644,15 @@ static void run(data_t *data)
>  			damaged_plane_update(data);
>  		}
>  		break;
> -	case PLANE_MOVE:
> +	case PLANE_MOVE_DAMAGED:
>  		damaged_plane_move(data);
>  		break;
> +	case PLANE_MOVE:
> +		for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> +			data->pos = i;
> +			plane_move(data);
> +		}
> +		break;
>  	default:
>  		igt_assert(false);
>  	}
> @@ -654,10 +762,20 @@ igt_main
>  		cleanup(&data);
>  	}
>  
> -	/* Only for overlay plane */
>  	data.op = PLANE_MOVE;
>  	/* Verify overlay plane move selective fetch */
>  	igt_describe("Test that selective fetch works on moving overlay plane");
> +	igt_subtest_f("cursor-%s-sf", op_str(data.op)) {
> +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> +		prepare(&data);
> +		run(&data);
> +		cleanup(&data);
> +	}
> +
> +	/* Only for overlay plane */
> +	data.op = PLANE_MOVE_DAMAGED;
> +	/* Verify overlay plane move selective fetch */
> +	igt_describe("Test that selective fetch works on moving overlay plane");
>  	igt_subtest_f("%s-sf-dmg-area", op_str(data.op)) {
>  		for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
>  			data.pos = i;
> @@ -668,6 +786,16 @@ igt_main
>  		}
>  	}
>  
> +	data.op = PLANE_MOVE;
> +	/* Verify overlay plane move selective fetch */
> +	igt_describe("Test that selective fetch works on moving overlay plane");
> +	igt_subtest_f("overlay-%s-sf", op_str(data.op)) {
> +		data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> +		prepare(&data);
> +		run(&data);
> +		cleanup(&data);
> +	}

Same text on three igt_describes and subtest names sound like some of
them are copypaste errors.


-- 
Petri Latvala



> +
>  	/* Verify primary plane selective fetch with overplay plane blended */
>  	data.op = OVERLAY_PRIM_UPDATE;
>  	igt_describe("Test that selective fetch works on primary plane "
> -- 
> 2.32.0
> 

[igt-dev] ✗ Fi.CI.BAT: failure for tests/kms_psr2_sf: Add new testcases for moving plane

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8062 bytes --]

== Series Details ==

Series: tests/kms_psr2_sf: Add new testcases for moving plane
URL   : https://patchwork.freedesktop.org/series/100559/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11264 -> IGTPW_6672
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_6672 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_6672, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/index.html

Participating hosts (44 -> 41)
------------------------------

  Additional (2): bat-dg2-8 fi-bdw-5557u 
  Missing    (5): fi-kbl-soraka shard-tglu fi-bsw-cyan shard-rkl shard-dg1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_6672:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - fi-rkl-guc:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/fi-rkl-guc/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-rkl-guc/igt@i915_selftest@live@hangcheck.html

  
Known issues
------------

  Here are the changes found in IGTPW_6672 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-bdw-5557u:       NOTRUN -> [INCOMPLETE][3] ([i915#146])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@gem_huc_copy@huc-copy:
    - fi-skl-6600u:       NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-skl-6600u/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@verify-random:
    - fi-skl-6600u:       NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#4613]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-skl-6600u/igt@gem_lmem_swapping@verify-random.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-n3050:       [PASS][6] -> [INCOMPLETE][7] ([i915#2940])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-bsw-n3050/igt@i915_selftest@live@execlists.html

  * igt@kms_chamelium@vga-edid-read:
    - fi-skl-6600u:       NOTRUN -> [SKIP][8] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-skl-6600u/igt@kms_chamelium@vga-edid-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-skl-6600u:       NOTRUN -> [SKIP][9] ([fdo#109271]) +21 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-skl-6600u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-skl-6600u:       NOTRUN -> [SKIP][10] ([fdo#109271] / [i915#533])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-skl-6600u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  * igt@runner@aborted:
    - fi-bsw-n3050:       NOTRUN -> [FAIL][11] ([fdo#109271] / [i915#1436] / [i915#2722] / [i915#3428] / [i915#4312])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-bsw-n3050/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-skl-6600u:       [INCOMPLETE][12] ([i915#4547]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/fi-skl-6600u/igt@gem_exec_suspend@basic-s3@smem.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-skl-6600u/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@gt_pm:
    - fi-tgl-1115g4:      [DMESG-FAIL][14] ([i915#3987]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/fi-tgl-1115g4/igt@i915_selftest@live@gt_pm.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-tgl-1115g4/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@gt_timelines:
    - {fi-tgl-dsi}:       [INCOMPLETE][16] -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/fi-tgl-dsi/igt@i915_selftest@live@gt_timelines.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-tgl-dsi/igt@i915_selftest@live@gt_timelines.html

  * igt@i915_selftest@live@hangcheck:
    - bat-dg1-6:          [DMESG-FAIL][18] ([i915#4494] / [i915#4957]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/bat-dg1-6/igt@i915_selftest@live@hangcheck.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/bat-dg1-6/igt@i915_selftest@live@hangcheck.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-cml-u2:          [DMESG-WARN][20] ([i915#4269]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/fi-cml-u2/igt@kms_frontbuffer_tracking@basic.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-cml-u2/igt@kms_frontbuffer_tracking@basic.html

  
#### Warnings ####

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [INCOMPLETE][22] ([i915#3303]) -> [INCOMPLETE][23] ([i915#4785])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11264/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#1759]: https://gitlab.freedesktop.org/drm/intel/issues/1759
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2373]: https://gitlab.freedesktop.org/drm/intel/issues/2373
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2722]: https://gitlab.freedesktop.org/drm/intel/issues/2722
  [i915#2940]: https://gitlab.freedesktop.org/drm/intel/issues/2940
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#3428]: https://gitlab.freedesktop.org/drm/intel/issues/3428
  [i915#3987]: https://gitlab.freedesktop.org/drm/intel/issues/3987
  [i915#4269]: https://gitlab.freedesktop.org/drm/intel/issues/4269
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4494]: https://gitlab.freedesktop.org/drm/intel/issues/4494
  [i915#4547]: https://gitlab.freedesktop.org/drm/intel/issues/4547
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#4957]: https://gitlab.freedesktop.org/drm/intel/issues/4957
  [i915#5127]: https://gitlab.freedesktop.org/drm/intel/issues/5127
  [i915#5137]: https://gitlab.freedesktop.org/drm/intel/issues/5137
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_6349 -> IGTPW_6672

  CI-20190529: 20190529
  CI_DRM_11264: 3b562232559bd940e63f977619548e5cc70da985 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_6672: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/index.html
  IGT_6349: 0513032006f385f34fcd094c810b93f31cbed09d @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git



== Testlist changes ==

+igt@kms_psr2_sf@cursor-plane-move-without-damage-sf
+igt@kms_psr2_sf@overlay-plane-move-without-damage-sf

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6672/index.html

[-- Attachment #2: Type: text/html, Size: 8975 bytes --]

Re: [igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane

[Hogander, Jouni]

On Tue, 2022-02-22 at 13:16 +0200, Petri Latvala wrote:
> On Tue, Feb 22, 2022 at 12:38:41PM +0200, Jouni Högander wrote:
> > Add new testcases which are moving cursor and overlay planes
> > without setting any damage areas.
> > 
> > Cursor testcase is reproducing following bug:
> > 
> > https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> > 
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  tests/i915/kms_psr2_sf.c | 144
> > ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 136 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/i915/kms_psr2_sf.c b/tests/i915/kms_psr2_sf.c
> > index 36f8a786..b9990f87 100644
> > --- a/tests/i915/kms_psr2_sf.c
> > +++ b/tests/i915/kms_psr2_sf.c
> > @@ -43,6 +43,7 @@ IGT_TEST_DESCRIPTION("Tests to varify PSR2
> > selective fetch by sending multiple"
> >  enum operations {
> >  	PLANE_UPDATE,
> >  	PLANE_UPDATE_CONTINUOUS,
> > +	PLANE_MOVE_DAMAGED,
> >  	PLANE_MOVE,
> >  	OVERLAY_PRIM_UPDATE
> >  };
> > @@ -51,7 +52,9 @@ enum plane_move_postion {
> >  	POS_TOP_LEFT,
> >  	POS_TOP_RIGHT,
> >  	POS_BOTTOM_LEFT,
> > -	POS_BOTTOM_RIGHT
> > +	POS_BOTTOM_RIGHT,
> > +	POS_BOTTOM_LEFT_NEGATIVE,
> > +	POS_TOP_RIGHT_NEGATIVE,
> >  };
> >  
> >  typedef struct {
> > @@ -74,6 +77,7 @@ typedef struct {
> >  	igt_plane_t *test_plane;
> >  	cairo_t *cr;
> >  	uint32_t screen_changes;
> > +	int cur_x, cur_y;
> >  } data_t;
> >  
> >  static const char *op_str(enum operations op)
> > @@ -81,7 +85,8 @@ static const char *op_str(enum operations op)
> >  	static const char * const name[] = {
> >  		[PLANE_UPDATE] = "plane-update",
> >  		[PLANE_UPDATE_CONTINUOUS] = "plane-update-continuous",
> > -		[PLANE_MOVE] = "plane-move",
> > +		[PLANE_MOVE] = "plane-move-without-damage",
> > +		[PLANE_MOVE_DAMAGED] = "plane-move",
> >  		[OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
> >  	};
> >  
> > @@ -187,7 +192,7 @@ static void plane_update_setup_squares(data_t
> > *data, igt_fb_t *fb, uint32_t h,
> >  	}
> >  }
> >  
> > -static void plane_move_setup_square(data_t *data, igt_fb_t *fb,
> > uint32_t h,
> > +static void plane_move_damaged_setup_square(data_t *data, igt_fb_t
> > *fb, uint32_t h,
> >  				       uint32_t v)
> >  {
> >  	int x = 0, y = 0;
> > @@ -260,8 +265,8 @@ static void prepare(data_t *data)
> >  
> >  		data->fb_continuous = &data->fb_overlay;
> >  
> > -		if (data->op == PLANE_MOVE) {
> > -			plane_move_setup_square(data, &data->fb_test,
> > +		if (data->op == PLANE_MOVE_DAMAGED) {
> > +			plane_move_damaged_setup_square(data, &data-
> > >fb_test,
> >  					   data->mode->hdisplay/2,
> >  					   data->mode->vdisplay/2);
> >  
> > @@ -273,6 +278,7 @@ static void prepare(data_t *data)
> >  
> >  		igt_plane_set_fb(sprite, &data->fb_overlay);
> >  		data->test_plane = sprite;
> > +
> >  		break;
> >  
> >  	case DRM_PLANE_TYPE_PRIMARY:
> > @@ -335,6 +341,8 @@ static void prepare(data_t *data)
> >  		igt_assert(false);
> >  	}
> >  
> > +	data->cur_x = data->cur_y = 0;
> > +
> >  	igt_plane_set_fb(primary, &data->fb_primary);
> >  
> >  	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > @@ -378,7 +386,7 @@ static void plane_update_expected_output(int
> > plane_type, int box_count,
> >  	manual(expected);
> >  }
> >  
> > -static void plane_move_expected_output(enum plane_move_postion
> > pos)
> > +static void plane_move_damaged_expected_output(enum
> > plane_move_postion pos)
> >  {
> >  	char expected[64] = {};
> >  
> > @@ -406,6 +414,42 @@ static void plane_move_expected_output(enum
> > plane_move_postion pos)
> >  	manual(expected);
> >  }
> >  
> > +static void plane_move_expected_output(enum plane_move_postion
> > pos)
> > +{
> > +	char expected[73] = {};
> > +
> > +	switch (pos) {
> > +	case POS_TOP_LEFT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on top left
> > corner");
> > +		break;
> > +	case POS_TOP_RIGHT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on top right
> > corner");
> > +		break;
> > +	case POS_BOTTOM_LEFT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on bottom left
> > corner");
> > +		break;
> > +	case POS_BOTTOM_RIGHT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on bottom right
> > corner");
> > +		break;
> > +	case POS_BOTTOM_LEFT_NEGATIVE:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on bottom left
> > corner (partly exceeding area)");
> > +		break;
> > +	case POS_TOP_RIGHT_NEGATIVE:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on top right corner
> > (partly exceeding area)");
> > +		break;
> > +	default:
> > +		igt_assert(false);
> > +	}
> > +
> > +	manual(expected);
> > +}
> > +
> >  static void overlay_prim_update_expected_output(int box_count)
> >  {
> >  	char expected[64] = {};
> > @@ -421,6 +465,9 @@ static void
> > overlay_prim_update_expected_output(int box_count)
> >  static void expected_output(data_t *data)
> >  {
> >  	switch (data->op) {
> > +	case PLANE_MOVE_DAMAGED:
> > +		plane_move_damaged_expected_output(data->pos);
> > +		break;
> >  	case PLANE_MOVE:
> >  		plane_move_expected_output(data->pos);
> >  		break;
> > @@ -487,6 +534,59 @@ static void damaged_plane_move(data_t *data)
> >  	expected_output(data);
> >  }
> >  
> > +static void plane_move(data_t *data)
> > +{
> > +	int target_x, target_y;
> > +
> > +	switch (data->pos) {
> > +	case POS_TOP_LEFT:
> > +		target_x = 0;
> > +		target_y = 0;
> > +		break;
> > +	case POS_TOP_RIGHT:
> > +		target_x = data->mode->hdisplay - data->fb_test.width;
> > +		target_y = 0;
> > +		break;
> > +	case POS_TOP_RIGHT_NEGATIVE:
> > +		target_x = data->mode->hdisplay - data->fb_test.width;
> > +		target_y = -data->fb_test.width / 2;
> > +		break;
> > +	case POS_BOTTOM_LEFT:
> > +		target_x = 0;
> > +		target_y = data->mode->vdisplay - data->fb_test.height;
> > +		break;
> > +	case POS_BOTTOM_LEFT_NEGATIVE:
> > +	  target_x = -data->fb_test.width / 2;
> > +	  target_y = data->mode->vdisplay - data->fb_test.height;
> > +		break;
> > +	case POS_BOTTOM_RIGHT:
> > +	  target_x = data->mode->hdisplay - data->fb_test.width;
> > +	  target_y = data->mode->vdisplay - data->fb_test.height;
> > +		break;
> > +	default:
> > +		igt_assert(false);
> > +	}
> > +
> > +	while (data->cur_x != target_x || data->cur_y != target_y) {
> > +		if (data->cur_x < target_x)
> > +			data->cur_x += min(target_x - data->cur_x, 20);
> > +		else if (data->cur_x > target_x)
> > +			data->cur_x -= min(data->cur_x - target_x, 20);
> > +
> > +		if (data->cur_y < target_y)
> > +			data->cur_y += min(target_y - data->cur_y, 20);
> > +		else if (data->cur_y > target_y)
> > +			data->cur_y -= min(data->cur_y - target_y, 20);
> > +
> > +		igt_plane_set_position(data->test_plane, data->cur_x,
> > data->cur_y);
> > +		igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +	}
> > +
> > +	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > +
> > +	expected_output(data);
> > +}
> > +
> >  static void damaged_plane_update(data_t *data)
> >  {
> >  	igt_plane_t *test_plane = data->test_plane;
> > @@ -526,6 +626,8 @@ static void damaged_plane_update(data_t *data)
> >  
> >  static void run(data_t *data)
> >  {
> > +	int i;
> > +
> >  	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> >  
> >  	data->screen_changes = 0;
> > @@ -542,9 +644,15 @@ static void run(data_t *data)
> >  			damaged_plane_update(data);
> >  		}
> >  		break;
> > -	case PLANE_MOVE:
> > +	case PLANE_MOVE_DAMAGED:
> >  		damaged_plane_move(data);
> >  		break;
> > +	case PLANE_MOVE:
> > +		for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE;
> > i++) {
> > +			data->pos = i;
> > +			plane_move(data);
> > +		}
> > +		break;
> >  	default:
> >  		igt_assert(false);
> >  	}
> > @@ -654,10 +762,20 @@ igt_main
> >  		cleanup(&data);
> >  	}
> >  
> > -	/* Only for overlay plane */
> >  	data.op = PLANE_MOVE;
> >  	/* Verify overlay plane move selective fetch */
> >  	igt_describe("Test that selective fetch works on moving overlay
> > plane");
> > +	igt_subtest_f("cursor-%s-sf", op_str(data.op)) {
> > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > +		prepare(&data);
> > +		run(&data);
> > +		cleanup(&data);
> > +	}
> > +
> > +	/* Only for overlay plane */
> > +	data.op = PLANE_MOVE_DAMAGED;
> > +	/* Verify overlay plane move selective fetch */
> > +	igt_describe("Test that selective fetch works on moving overlay
> > plane");
> >  	igt_subtest_f("%s-sf-dmg-area", op_str(data.op)) {
> >  		for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
> >  			data.pos = i;
> > @@ -668,6 +786,16 @@ igt_main
> >  		}
> >  	}
> >  
> > +	data.op = PLANE_MOVE;
> > +	/* Verify overlay plane move selective fetch */
> > +	igt_describe("Test that selective fetch works on moving overlay
> > plane");
> > +	igt_subtest_f("overlay-%s-sf", op_str(data.op)) {
> > +		data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > +		prepare(&data);
> > +		run(&data);
> > +		cleanup(&data);
> > +	}
> 
> Same text on three igt_describes and subtest names sound like some of
> them are copypaste errors.
> 
> 

Thank you for pointing this out. I will fix it before sending next
version.

BR,

Jouni Högander

Re: [igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane

[Souza, Jose]

On Tue, 2022-02-22 at 12:38 +0200, Jouni Högander wrote:
> Add new testcases which are moving cursor and overlay planes
> without setting any damage areas.
> 
> Cursor testcase is reproducing following bug:
> 
> https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  tests/i915/kms_psr2_sf.c | 144 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 136 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/i915/kms_psr2_sf.c b/tests/i915/kms_psr2_sf.c
> index 36f8a786..b9990f87 100644
> --- a/tests/i915/kms_psr2_sf.c
> +++ b/tests/i915/kms_psr2_sf.c
> @@ -43,6 +43,7 @@ IGT_TEST_DESCRIPTION("Tests to varify PSR2 selective fetch by sending multiple"
>  enum operations {
>  	PLANE_UPDATE,
>  	PLANE_UPDATE_CONTINUOUS,
> +	PLANE_MOVE_DAMAGED,

Missing more explanation in the commit message, please split each change in a single patch.
I see here a patch for this new PLANE_MOVE_DAMAGED and another adding the negative positions...

Anyhow, I don't think we need PLANE_MOVE_DAMAGED.
One of the differences between it and PLANE_MOVE is set_clip(&data->plane_move_clip) that is wrong, the damaged property is based on the plane
coordinates.


>  	PLANE_MOVE,
>  	OVERLAY_PRIM_UPDATE
>  };
> @@ -51,7 +52,9 @@ enum plane_move_postion {
>  	POS_TOP_LEFT,
>  	POS_TOP_RIGHT,
>  	POS_BOTTOM_LEFT,
> -	POS_BOTTOM_RIGHT
> +	POS_BOTTOM_RIGHT,
> +	POS_BOTTOM_LEFT_NEGATIVE,
> +	POS_TOP_RIGHT_NEGATIVE,
>  };
>  
>  typedef struct {
> @@ -74,6 +77,7 @@ typedef struct {
>  	igt_plane_t *test_plane;
>  	cairo_t *cr;
>  	uint32_t screen_changes;
> +	int cur_x, cur_y;
>  } data_t;
>  
>  static const char *op_str(enum operations op)
> @@ -81,7 +85,8 @@ static const char *op_str(enum operations op)
>  	static const char * const name[] = {
>  		[PLANE_UPDATE] = "plane-update",
>  		[PLANE_UPDATE_CONTINUOUS] = "plane-update-continuous",
> -		[PLANE_MOVE] = "plane-move",
> +		[PLANE_MOVE] = "plane-move-without-damage",
> +		[PLANE_MOVE_DAMAGED] = "plane-move",
>  		[OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
>  	};
>  
> @@ -187,7 +192,7 @@ static void plane_update_setup_squares(data_t *data, igt_fb_t *fb, uint32_t h,
>  	}
>  }
>  
> -static void plane_move_setup_square(data_t *data, igt_fb_t *fb, uint32_t h,
> +static void plane_move_damaged_setup_square(data_t *data, igt_fb_t *fb, uint32_t h,
>  				       uint32_t v)
>  {
>  	int x = 0, y = 0;
> @@ -260,8 +265,8 @@ static void prepare(data_t *data)
>  
>  		data->fb_continuous = &data->fb_overlay;
>  
> -		if (data->op == PLANE_MOVE) {
> -			plane_move_setup_square(data, &data->fb_test,
> +		if (data->op == PLANE_MOVE_DAMAGED) {
> +			plane_move_damaged_setup_square(data, &data->fb_test,
>  					   data->mode->hdisplay/2,
>  					   data->mode->vdisplay/2);
>  
> @@ -273,6 +278,7 @@ static void prepare(data_t *data)
>  
>  		igt_plane_set_fb(sprite, &data->fb_overlay);
>  		data->test_plane = sprite;
> +
>  		break;
>  
>  	case DRM_PLANE_TYPE_PRIMARY:
> @@ -335,6 +341,8 @@ static void prepare(data_t *data)
>  		igt_assert(false);
>  	}
>  
> +	data->cur_x = data->cur_y = 0;
> +
>  	igt_plane_set_fb(primary, &data->fb_primary);
>  
>  	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> @@ -378,7 +386,7 @@ static void plane_update_expected_output(int plane_type, int box_count,
>  	manual(expected);
>  }
>  
> -static void plane_move_expected_output(enum plane_move_postion pos)
> +static void plane_move_damaged_expected_output(enum plane_move_postion pos)
>  {
>  	char expected[64] = {};
>  
> @@ -406,6 +414,42 @@ static void plane_move_expected_output(enum plane_move_postion pos)
>  	manual(expected);
>  }
>  
> +static void plane_move_expected_output(enum plane_move_postion pos)
> +{
> +	char expected[73] = {};
> +
> +	switch (pos) {
> +	case POS_TOP_LEFT:
> +		sprintf(expected,
> +			"screen Green with Blue box on top left corner");
> +		break;
> +	case POS_TOP_RIGHT:
> +		sprintf(expected,
> +			"screen Green with Blue box on top right corner");
> +		break;
> +	case POS_BOTTOM_LEFT:
> +		sprintf(expected,
> +			"screen Green with Blue box on bottom left corner");
> +		break;
> +	case POS_BOTTOM_RIGHT:
> +		sprintf(expected,
> +			"screen Green with Blue box on bottom right corner");
> +		break;
> +	case POS_BOTTOM_LEFT_NEGATIVE:
> +		sprintf(expected,
> +			"screen Green with Blue box on bottom left corner (partly exceeding area)");
> +		break;
> +	case POS_TOP_RIGHT_NEGATIVE:
> +		sprintf(expected,
> +			"screen Green with Blue box on top right corner (partly exceeding area)");
> +		break;
> +	default:
> +		igt_assert(false);
> +	}
> +
> +	manual(expected);
> +}

Another unexpected changed, you changed the visual output of a current subtest, why?

> +
>  static void overlay_prim_update_expected_output(int box_count)
>  {
>  	char expected[64] = {};
> @@ -421,6 +465,9 @@ static void overlay_prim_update_expected_output(int box_count)
>  static void expected_output(data_t *data)
>  {
>  	switch (data->op) {
> +	case PLANE_MOVE_DAMAGED:
> +		plane_move_damaged_expected_output(data->pos);
> +		break;
>  	case PLANE_MOVE:
>  		plane_move_expected_output(data->pos);
>  		break;
> @@ -487,6 +534,59 @@ static void damaged_plane_move(data_t *data)
>  	expected_output(data);
>  }
>  
> +static void plane_move(data_t *data)
> +{
> +	int target_x, target_y;
> +
> +	switch (data->pos) {
> +	case POS_TOP_LEFT:
> +		target_x = 0;
> +		target_y = 0;
> +		break;
> +	case POS_TOP_RIGHT:
> +		target_x = data->mode->hdisplay - data->fb_test.width;
> +		target_y = 0;
> +		break;
> +	case POS_TOP_RIGHT_NEGATIVE:
> +		target_x = data->mode->hdisplay - data->fb_test.width;
> +		target_y = -data->fb_test.width / 2;
> +		break;
> +	case POS_BOTTOM_LEFT:
> +		target_x = 0;
> +		target_y = data->mode->vdisplay - data->fb_test.height;
> +		break;
> +	case POS_BOTTOM_LEFT_NEGATIVE:
> +	  target_x = -data->fb_test.width / 2;
> +	  target_y = data->mode->vdisplay - data->fb_test.height;
> +		break;
> +	case POS_BOTTOM_RIGHT:
> +	  target_x = data->mode->hdisplay - data->fb_test.width;
> +	  target_y = data->mode->vdisplay - data->fb_test.height;
> +		break;
> +	default:
> +		igt_assert(false);
> +	}
> +
> +	while (data->cur_x != target_x || data->cur_y != target_y) {
> +		if (data->cur_x < target_x)
> +			data->cur_x += min(target_x - data->cur_x, 20);
> +		else if (data->cur_x > target_x)
> +			data->cur_x -= min(data->cur_x - target_x, 20);
> +
> +		if (data->cur_y < target_y)
> +			data->cur_y += min(target_y - data->cur_y, 20);
> +		else if (data->cur_y > target_y)
> +			data->cur_y -= min(data->cur_y - target_y, 20);
> +
> +		igt_plane_set_position(data->test_plane, data->cur_x, data->cur_y);
> +		igt_display_commit2(&data->display, COMMIT_ATOMIC);

what is this? why do atomic commits in loop until reach target?
another change that if necessary should go to another patch as this is changing a current subtest.

> +	}
> +
> +	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> +
> +	expected_output(data);
> +}
> +
>  static void damaged_plane_update(data_t *data)
>  {
>  	igt_plane_t *test_plane = data->test_plane;
> @@ -526,6 +626,8 @@ static void damaged_plane_update(data_t *data)
>  
>  static void run(data_t *data)
>  {
> +	int i;
> +
>  	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
>  
>  	data->screen_changes = 0;
> @@ -542,9 +644,15 @@ static void run(data_t *data)
>  			damaged_plane_update(data);
>  		}
>  		break;
> -	case PLANE_MOVE:
> +	case PLANE_MOVE_DAMAGED:
>  		damaged_plane_move(data);
>  		break;
> +	case PLANE_MOVE:
> +		for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> +			data->pos = i;
> +			plane_move(data);

Another unexpected change.

> +		}
> +		break;
>  	default:
>  		igt_assert(false);
>  	}
> @@ -654,10 +762,20 @@ igt_main
>  		cleanup(&data);
>  	}
>  
> -	/* Only for overlay plane */
>  	data.op = PLANE_MOVE;
>  	/* Verify overlay plane move selective fetch */
>  	igt_describe("Test that selective fetch works on moving overlay plane");

overlay != cursor

> +	igt_subtest_f("cursor-%s-sf", op_str(data.op)) {
> +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> +		prepare(&data);
> +		run(&data);
> +		cleanup(&data);
> +	}
> +
> +	/* Only for overlay plane */
> +	data.op = PLANE_MOVE_DAMAGED;
> +	/* Verify overlay plane move selective fetch */
> +	igt_describe("Test that selective fetch works on moving overlay plane");
>  	igt_subtest_f("%s-sf-dmg-area", op_str(data.op)) {
>  		for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
>  			data.pos = i;
> @@ -668,6 +786,16 @@ igt_main
>  		}
>  	}
>  
> +	data.op = PLANE_MOVE;
> +	/* Verify overlay plane move selective fetch */
> +	igt_describe("Test that selective fetch works on moving overlay plane");
> +	igt_subtest_f("overlay-%s-sf", op_str(data.op)) {
> +		data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> +		prepare(&data);
> +		run(&data);
> +		cleanup(&data);
> +	}
> +
>  	/* Verify primary plane selective fetch with overplay plane blended */
>  	data.op = OVERLAY_PRIM_UPDATE;
>  	igt_describe("Test that selective fetch works on primary plane "

There is a few code style errors.

Other thing that you need to pay attention is the patch diff, saw some new line being added to a function that is not being changed.

Re: [igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane

[Hogander, Jouni]

On Tue, 2022-02-22 at 16:38 +0000, Souza, Jose wrote:
> On Tue, 2022-02-22 at 12:38 +0200, Jouni Högander wrote:
> > Add new testcases which are moving cursor and overlay planes
> > without setting any damage areas.
> > 
> > Cursor testcase is reproducing following bug:
> > 
> > https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> > 
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  tests/i915/kms_psr2_sf.c | 144
> > ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 136 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/i915/kms_psr2_sf.c b/tests/i915/kms_psr2_sf.c
> > index 36f8a786..b9990f87 100644
> > --- a/tests/i915/kms_psr2_sf.c
> > +++ b/tests/i915/kms_psr2_sf.c
> > @@ -43,6 +43,7 @@ IGT_TEST_DESCRIPTION("Tests to varify PSR2
> > selective fetch by sending multiple"
> >  enum operations {
> >  PLANE_UPDATE,
> >  PLANE_UPDATE_CONTINUOUS,
> > +PLANE_MOVE_DAMAGED,
> 
> Missing more explanation in the commit message, please split each
> change in a single patch.
> I see here a patch for this new PLANE_MOVE_DAMAGED and another adding
> the negative positions...
> 
> Anyhow, I don't think we need PLANE_MOVE_DAMAGED.
> One of the differences between it and PLANE_MOVE is set_clip(&data-
> >plane_move_clip) that is wrong, the damaged property is based on the
> plane
> coordinates.

Obviously more explanation is needed in commit message. I will improve
it.

I wanted to keep original testcase as it is. Add a new testcase which
is targeted to trigger the screen flicker reported here:

https://gitlab.freedesktop.org/drm/intel/-/issues/5077

Current testcase is blinking screen and switching moving plane color
between blue and white when plane moves. This is because one plane move
is done on each iteration here: 

for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
			data.pos = i;
			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
			prepare(&data);
			run(&data);
			cleanup(&data);
}

Additinally blinking/switching color on each iteration due to
prepare/cleanup. Doing this makes it harder to see possible glitches.
The testcase I added is doing:

prepare(&data);
for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
run(@data);
}
cleanup(&data);

This is moving cursor (or overlay plane) smoothly around screen and
possible glitches are easy to observe.

Damaged property I removed completely from this new testcase. It's not
used by psr selective fetch area calculation at all when plane moves.

I named original testcase enum as PLANE_MOVE_DAMAGED and use PLANE_MOVE
for the new one.

> 
> >  PLANE_MOVE,
> >  OVERLAY_PRIM_UPDATE
> >  };
> > @@ -51,7 +52,9 @@ enum plane_move_postion {
> >  POS_TOP_LEFT,
> >  POS_TOP_RIGHT,
> >  POS_BOTTOM_LEFT,
> > -POS_BOTTOM_RIGHT
> > +POS_BOTTOM_RIGHT,
> > +POS_BOTTOM_LEFT_NEGATIVE,
> > +POS_TOP_RIGHT_NEGATIVE,
> >  };
> > 
> >  typedef struct {
> > @@ -74,6 +77,7 @@ typedef struct {
> >  igt_plane_t *test_plane;
> >  cairo_t *cr;
> >  uint32_t screen_changes;
> > +int cur_x, cur_y;
> >  } data_t;
> > 
> >  static const char *op_str(enum operations op)
> > @@ -81,7 +85,8 @@ static const char *op_str(enum operations op)
> >  static const char * const name[] = {
> >  [PLANE_UPDATE] = "plane-update",
> >  [PLANE_UPDATE_CONTINUOUS] = "plane-update-continuous",
> > -[PLANE_MOVE] = "plane-move",
> > +[PLANE_MOVE] = "plane-move-without-damage",
> > +[PLANE_MOVE_DAMAGED] = "plane-move",
> >  [OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
> >  };
> > 
> > @@ -187,7 +192,7 @@ static void plane_update_setup_squares(data_t
> > *data, igt_fb_t *fb, uint32_t h,
> >  }
> >  }
> > 
> > -static void plane_move_setup_square(data_t *data, igt_fb_t *fb,
> > uint32_t h,
> > +static void plane_move_damaged_setup_square(data_t *data, igt_fb_t
> > *fb, uint32_t h,
> >         uint32_t v)
> >  {
> >  int x = 0, y = 0;
> > @@ -260,8 +265,8 @@ static void prepare(data_t *data)
> > 
> >  data->fb_continuous = &data->fb_overlay;
> > 
> > -if (data->op == PLANE_MOVE) {
> > -plane_move_setup_square(data, &data->fb_test,
> > +if (data->op == PLANE_MOVE_DAMAGED) {
> > +plane_move_damaged_setup_square(data, &data->fb_test,
> >     data->mode->hdisplay/2,
> >     data->mode->vdisplay/2);
> > 
> > @@ -273,6 +278,7 @@ static void prepare(data_t *data)
> > 
> >  igt_plane_set_fb(sprite, &data->fb_overlay);
> >  data->test_plane = sprite;
> > +
> >  break;
> > 
> >  case DRM_PLANE_TYPE_PRIMARY:
> > @@ -335,6 +341,8 @@ static void prepare(data_t *data)
> >  igt_assert(false);
> >  }
> > 
> > +data->cur_x = data->cur_y = 0;
> > +
> >  igt_plane_set_fb(primary, &data->fb_primary);
> > 
> >  igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > @@ -378,7 +386,7 @@ static void plane_update_expected_output(int
> > plane_type, int box_count,
> >  manual(expected);
> >  }
> > 
> > -static void plane_move_expected_output(enum plane_move_postion
> > pos)
> > +static void plane_move_damaged_expected_output(enum
> > plane_move_postion pos)
> >  {
> >  char expected[64] = {};
> > 
> > @@ -406,6 +414,42 @@ static void plane_move_expected_output(enum
> > plane_move_postion pos)
> >  manual(expected);
> >  }
> > 
> > +static void plane_move_expected_output(enum plane_move_postion
> > pos)
> > +{
> > +char expected[73] = {};
> > +
> > +switch (pos) {
> > +case POS_TOP_LEFT:
> > +sprintf(expected,
> > +"screen Green with Blue box on top left corner");
> > +break;
> > +case POS_TOP_RIGHT:
> > +sprintf(expected,
> > +"screen Green with Blue box on top right corner");
> > +break;
> > +case POS_BOTTOM_LEFT:
> > +sprintf(expected,
> > +"screen Green with Blue box on bottom left corner");
> > +break;
> > +case POS_BOTTOM_RIGHT:
> > +sprintf(expected,
> > +"screen Green with Blue box on bottom right corner");
> > +break;
> > +case POS_BOTTOM_LEFT_NEGATIVE:
> > +sprintf(expected,
> > +"screen Green with Blue box on bottom left corner (partly
> > exceeding area)");
> > +break;
> > +case POS_TOP_RIGHT_NEGATIVE:
> > +sprintf(expected,
> > +"screen Green with Blue box on top right corner (partly exceeding
> > area)");
> > +break;
> > +default:
> > +igt_assert(false);
> > +}
> > +
> > +manual(expected);
> > +}
> 
> Another unexpected changed, you changed the visual output of a
> current subtest, why?

No. Old one is now plane_move_damaged_expected_output and new is
plane_move_expected_output.

> 
> > +
> >  static void overlay_prim_update_expected_output(int box_count)
> >  {
> >  char expected[64] = {};
> > @@ -421,6 +465,9 @@ static void
> > overlay_prim_update_expected_output(int box_count)
> >  static void expected_output(data_t *data)
> >  {
> >  switch (data->op) {
> > +case PLANE_MOVE_DAMAGED:
> > +plane_move_damaged_expected_output(data->pos);
> > +break;
> >  case PLANE_MOVE:
> >  plane_move_expected_output(data->pos);
> >  break;
> > @@ -487,6 +534,59 @@ static void damaged_plane_move(data_t *data)
> >  expected_output(data);
> >  }
> > 
> > +static void plane_move(data_t *data)
> > +{
> > +int target_x, target_y;
> > +
> > +switch (data->pos) {
> > +case POS_TOP_LEFT:
> > +target_x = 0;
> > +target_y = 0;
> > +break;
> > +case POS_TOP_RIGHT:
> > +target_x = data->mode->hdisplay - data->fb_test.width;
> > +target_y = 0;
> > +break;
> > +case POS_TOP_RIGHT_NEGATIVE:
> > +target_x = data->mode->hdisplay - data->fb_test.width;
> > +target_y = -data->fb_test.width / 2;
> > +break;plane_move_damaged_expected_output
> > +case POS_BOTTOM_LEFT:
> > +target_x = 0;
> > +target_y = data->mode->vdisplay - data->fb_test.height;
> > +break;
> > +case POS_BOTTOM_LEFT_NEGATIVE:
> > +  target_x = -data->fb_test.width / 2;
> > +  target_y = data->mode->vdisplay - data->fb_test.height;
> > +break;
> > +case POS_BOTTOM_RIGHT:
> > +  target_x = data->mode->hdisplay - data->fb_test.width;
> > +  target_y = data->mode->vdisplay - data->fb_test.height;
> > +break;
> > +default:
> > +igt_assert(false);
> > +}
> > +
> > +while (data->cur_x != target_x || data->cur_y != target_y) {
> > +if (data->cur_x < target_x)
> > +data->cur_x += min(target_x - data->cur_x, 20);
> > +else if (data->cur_x > target_x)
> > +data->cur_x -= min(data->cur_x - target_x, 20);
> > +
> > +if (data->cur_y < target_y)
> > +data->cur_y += min(target_y - data->cur_y, 20);
> > +else if (data->cur_y > target_y)
> > +data->cur_y -= min(data->cur_y - target_y, 20);
> > +
> > +igt_plane_set_position(data->test_plane, data->cur_x, data-
> > >cur_y);
> > +igt_display_commit2(&data->display, COMMIT_ATOMIC);
> 
> what is this? why do atomic commits in loop until reach target?
> another change that if necessary should go to another patch as this
> is changing a current subtest.

I'm moving plane here step by step to target position. This is the
"run" function for the testcase I described above. I need to do it like
this to reproduce the glitch this new testcase is targeted to reveal.
If I just move it directly to target position (i.e. x < or y < 0) the
glitch doesn't reproduce. See also my explanation above.

> 
> > +}
> > +
> > +igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > +
> > +expected_output(data);
> > +}
> > +
> >  static void damaged_plane_update(data_t *data)
> >  {
> >  igt_plane_t *test_plane = data->test_plane;
> > @@ -526,6 +626,8 @@ static void damaged_plane_update(data_t *data)
> > 
> >  static void run(data_t *data)
> >  {
> > +int i;
> > +
> >  igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > 
> >  data->screen_changes = 0;
> > @@ -542,9 +644,15 @@ static void run(data_t *data)
> >  damaged_plane_update(data);
> >  }
> >  break;
> > -case PLANE_MOVE:
> > +case PLANE_MOVE_DAMAGED:
> >  damaged_plane_move(data);
> >  break;
> > +case PLANE_MOVE:
> > +for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> > +data->pos = i;
> > +plane_move(data);
> 
> Another unexpected change.

I will try to split the patch as you suggested. PLANE_MOVE_DAMAGED here
is the original testcase. New is PLANE_MOVE. I choose to change enum
naming because original testcase is also switching moving plane content
and using damage property for that (PLANE_MOVE_DAMAGED). New testcase
is just moving the plane (PLANE_MOVE).

> 
> > +}
> > +break;
> >  default:
> >  igt_assert(false);
> >  }
> > @@ -654,10 +762,20 @@ igt_main
> >  cleanup(&data);
> >  }
> > 
> > -/* Only for overlay plane */
> >  data.op = PLANE_MOVE;
> >  /* Verify overlay plane move selective fetch */
> >  igt_describe("Test that selective fetch works on moving overlay
> > plane");
> 
> overlay != cursor

Oops, one more copy paste error. I will fix it.

> 
> > +igt_subtest_f("cursor-%s-sf", op_str(data.op)) {
> > +data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > +prepare(&data);
> > +run(&data);
> > +cleanup(&data);
> > +}
> > +
> > +/* Only for overlay plane */
> > +data.op = PLANE_MOVE_DAMAGED;
> > +/* Verify overlay plane move selective fetch */
> > +igt_describe("Test that selective fetch works on moving overlay
> > plane");
> >  igt_subtest_f("%s-sf-dmg-area", op_str(data.op)) {
> >  for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
> >  data.pos = i;
> > @@ -668,6 +786,16 @@ igt_main
> >  }
> >  }
> > 
> > +data.op = PLANE_MOVE;
> > +/* Verify overlay plane move selective fetch */
> > +igt_describe("Test that selective fetch works on moving overlay
> > plane");
> > +igt_subtest_f("overlay-%s-sf", op_str(data.op)) {
> > +data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > +prepare(&data);
> > +run(&data);
> > +cleanup(&data);
> > +}
> > +
> >  /* Verify primary plane selective fetch with overplay plane
> > blended */
> >  data.op = OVERLAY_PRIM_UPDATE;
> >  igt_describe("Test that selective fetch works on primary plane "
> 
> There is a few code style errors.
> 

Ok, I will fix these as well.

> Other thing that you need to pay attention is the patch diff, saw
> some new line being added to a function that is not being changed.
> 

Yes, I will drop those.

BR,

Jouni Högander

Re: [igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane

[Hogander, Jouni]

On Wed, 2022-02-23 at 07:39 +0000, Hogander, Jouni wrote:
> On Tue, 2022-02-22 at 16:38 +0000, Souza, Jose wrote:
> > On Tue, 2022-02-22 at 12:38 +0200, Jouni Högander wrote:
> > > Add new testcases which are moving cursor and overlay planes
> > > without setting any damage areas.
> > > 
> > > Cursor testcase is reproducing following bug:
> > > 
> > > https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> > > 
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  tests/i915/kms_psr2_sf.c | 144
> > > ++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 136 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tests/i915/kms_psr2_sf.c b/tests/i915/kms_psr2_sf.c
> > > index 36f8a786..b9990f87 100644
> > > --- a/tests/i915/kms_psr2_sf.c
> > > +++ b/tests/i915/kms_psr2_sf.c
> > > @@ -43,6 +43,7 @@ IGT_TEST_DESCRIPTION("Tests to varify PSR2
> > > selective fetch by sending multiple"
> > >  enum operations {
> > >  PLANE_UPDATE,
> > >  PLANE_UPDATE_CONTINUOUS,
> > > +PLANE_MOVE_DAMAGED,
> > 
> > Missing more explanation in the commit message, please split each
> > change in a single patch.
> > I see here a patch for this new PLANE_MOVE_DAMAGED and another
> > adding
> > the negative positions...
> > 
> > Anyhow, I don't think we need PLANE_MOVE_DAMAGED.
> > One of the differences between it and PLANE_MOVE is set_clip(&data-
> > > plane_move_clip) that is wrong, the damaged property is based on
> > > the
> > plane
> > coordinates.
> 
> Obviously more explanation is needed in commit message. I will
> improve
> it.
> 
> I wanted to keep original testcase as it is. Add a new testcase which
> is targeted to trigger the screen flicker reported here:
> 
> https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> 
> Current testcase is blinking screen and switching moving plane color
> between blue and white when plane moves. This is because one plane
> move
> is done on each iteration here: 
> 
> for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
> 			data.pos = i;
> 			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> 			prepare(&data);
> 			run(&data);
> 			cleanup(&data);
> }
> 
> Additinally blinking/switching color on each iteration due to
> prepare/cleanup. Doing this makes it harder to see possible glitches.
> The testcase I added is doing:
> 
> prepare(&data);
> for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> run(@data);
> }
> cleanup(&data);
> 
> This is moving cursor (or overlay plane) smoothly around screen and
> possible glitches are easy to observe.
> 
> Damaged property I removed completely from this new testcase. It's
> not
> used by psr selective fetch area calculation at all when plane moves.
> 
> I named original testcase enum as PLANE_MOVE_DAMAGED and use
> PLANE_MOVE
> for the new one.
> 
> > >  PLANE_MOVE,
> > >  OVERLAY_PRIM_UPDATE
> > >  };
> > > @@ -51,7 +52,9 @@ enum plane_move_postion {
> > >  POS_TOP_LEFT,
> > >  POS_TOP_RIGHT,
> > >  POS_BOTTOM_LEFT,
> > > -POS_BOTTOM_RIGHT
> > > +POS_BOTTOM_RIGHT,
> > > +POS_BOTTOM_LEFT_NEGATIVE,
> > > +POS_TOP_RIGHT_NEGATIVE,
> > >  };
> > > 
> > >  typedef struct {
> > > @@ -74,6 +77,7 @@ typedef struct {
> > >  igt_plane_t *test_plane;
> > >  cairo_t *cr;
> > >  uint32_t screen_changes;
> > > +int cur_x, cur_y;
> > >  } data_t;
> > > 
> > >  static const char *op_str(enum operations op)
> > > @@ -81,7 +85,8 @@ static const char *op_str(enum operations op)
> > >  static const char * const name[] = {
> > >  [PLANE_UPDATE] = "plane-update",
> > >  [PLANE_UPDATE_CONTINUOUS] = "plane-update-continuous",
> > > -[PLANE_MOVE] = "plane-move",
> > > +[PLANE_MOVE] = "plane-move-without-damage",
> > > +[PLANE_MOVE_DAMAGED] = "plane-move",
> > >  [OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
> > >  };
> > > 
> > > @@ -187,7 +192,7 @@ static void plane_update_setup_squares(data_t
> > > *data, igt_fb_t *fb, uint32_t h,
> > >  }
> > >  }
> > > 
> > > -static void plane_move_setup_square(data_t *data, igt_fb_t *fb,
> > > uint32_t h,
> > > +static void plane_move_damaged_setup_square(data_t *data,
> > > igt_fb_t
> > > *fb, uint32_t h,
> > >         uint32_t v)
> > >  {
> > >  int x = 0, y = 0;
> > > @@ -260,8 +265,8 @@ static void prepare(data_t *data)
> > > 
> > >  data->fb_continuous = &data->fb_overlay;
> > > 
> > > -if (data->op == PLANE_MOVE) {
> > > -plane_move_setup_square(data, &data->fb_test,
> > > +if (data->op == PLANE_MOVE_DAMAGED) {
> > > +plane_move_damaged_setup_square(data, &data->fb_test,
> > >     data->mode->hdisplay/2,
> > >     data->mode->vdisplay/2);
> > > 
> > > @@ -273,6 +278,7 @@ static void prepare(data_t *data)
> > > 
> > >  igt_plane_set_fb(sprite, &data->fb_overlay);
> > >  data->test_plane = sprite;
> > > +
> > >  break;
> > > 
> > >  case DRM_PLANE_TYPE_PRIMARY:
> > > @@ -335,6 +341,8 @@ static void prepare(data_t *data)
> > >  igt_assert(false);
> > >  }
> > > 
> > > +data->cur_x = data->cur_y = 0;
> > > +
> > >  igt_plane_set_fb(primary, &data->fb_primary);
> > > 
> > >  igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > > @@ -378,7 +386,7 @@ static void plane_update_expected_output(int
> > > plane_type, int box_count,
> > >  manual(expected);
> > >  }
> > > 
> > > -static void plane_move_expected_output(enum plane_move_postion
> > > pos)
> > > +static void plane_move_damaged_expected_output(enum
> > > plane_move_postion pos)
> > >  {
> > >  char expected[64] = {};
> > > 
> > > @@ -406,6 +414,42 @@ static void plane_move_expected_output(enum
> > > plane_move_postion pos)
> > >  manual(expected);
> > >  }
> > > 
> > > +static void plane_move_expected_output(enum plane_move_postion
> > > pos)
> > > +{
> > > +char expected[73] = {};
> > > +
> > > +switch (pos) {
> > > +case POS_TOP_LEFT:
> > > +sprintf(expected,
> > > +"screen Green with Blue box on top left corner");
> > > +break;
> > > +case POS_TOP_RIGHT:
> > > +sprintf(expected,
> > > +"screen Green with Blue box on top right corner");
> > > +break;
> > > +case POS_BOTTOM_LEFT:
> > > +sprintf(expected,
> > > +"screen Green with Blue box on bottom left corner");
> > > +break;
> > > +case POS_BOTTOM_RIGHT:
> > > +sprintf(expected,
> > > +"screen Green with Blue box on bottom right corner");
> > > +break;
> > > +case POS_BOTTOM_LEFT_NEGATIVE:
> > > +sprintf(expected,
> > > +"screen Green with Blue box on bottom left corner (partly
> > > exceeding area)");
> > > +break;
> > > +case POS_TOP_RIGHT_NEGATIVE:
> > > +sprintf(expected,
> > > +"screen Green with Blue box on top right corner (partly
> > > exceeding
> > > area)");
> > > +break;
> > > +default:
> > > +igt_assert(false);
> > > +}
> > > +
> > > +manual(expected);
> > > +}
> > 
> > Another unexpected changed, you changed the visual output of a
> > current subtest, why?
> 
> No. Old one is now plane_move_damaged_expected_output and new is
> plane_move_expected_output.
> 
> > > +
> > >  static void overlay_prim_update_expected_output(int box_count)
> > >  {
> > >  char expected[64] = {};
> > > @@ -421,6 +465,9 @@ static void
> > > overlay_prim_update_expected_output(int box_count)
> > >  static void expected_output(data_t *data)
> > >  {
> > >  switch (data->op) {
> > > +case PLANE_MOVE_DAMAGED:
> > > +plane_move_damaged_expected_output(data->pos);
> > > +break;
> > >  case PLANE_MOVE:
> > >  plane_move_expected_output(data->pos);
> > >  break;
> > > @@ -487,6 +534,59 @@ static void damaged_plane_move(data_t *data)
> > >  expected_output(data);
> > >  }
> > > 
> > > +static void plane_move(data_t *data)
> > > +{
> > > +int target_x, target_y;
> > > +
> > > +switch (data->pos) {
> > > +case POS_TOP_LEFT:
> > > +target_x = 0;
> > > +target_y = 0;
> > > +break;
> > > +case POS_TOP_RIGHT:
> > > +target_x = data->mode->hdisplay - data->fb_test.width;
> > > +target_y = 0;
> > > +break;
> > > +case POS_TOP_RIGHT_NEGATIVE:
> > > +target_x = data->mode->hdisplay - data->fb_test.width;
> > > +target_y = -data->fb_test.width / 2;
> > > +break;plane_move_damaged_expected_output
> > > +case POS_BOTTOM_LEFT:
> > > +target_x = 0;
> > > +target_y = data->mode->vdisplay - data->fb_test.height;
> > > +break;
> > > +case POS_BOTTOM_LEFT_NEGATIVE:
> > > +  target_x = -data->fb_test.width / 2;
> > > +  target_y = data->mode->vdisplay - data->fb_test.height;
> > > +break;
> > > +case POS_BOTTOM_RIGHT:
> > > +  target_x = data->mode->hdisplay - data->fb_test.width;
> > > +  target_y = data->mode->vdisplay - data->fb_test.height;
> > > +break;
> > > +default:
> > > +igt_assert(false);
> > > +}
> > > +
> > > +while (data->cur_x != target_x || data->cur_y != target_y) {
> > > +if (data->cur_x < target_x)
> > > +data->cur_x += min(target_x - data->cur_x, 20);
> > > +else if (data->cur_x > target_x)
> > > +data->cur_x -= min(data->cur_x - target_x, 20);
> > > +
> > > +if (data->cur_y < target_y)
> > > +data->cur_y += min(target_y - data->cur_y, 20);
> > > +else if (data->cur_y > target_y)
> > > +data->cur_y -= min(data->cur_y - target_y, 20);
> > > +
> > > +igt_plane_set_position(data->test_plane, data->cur_x, data-
> > > > cur_y);
> > > +igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > 
> > what is this? why do atomic commits in loop until reach target?
> > another change that if necessary should go to another patch as this
> > is changing a current subtest.
> 
> I'm moving plane here step by step to target position. This is the
> "run" function for the testcase I described above. I need to do it
> like
> this to reproduce the glitch this new testcase is targeted to reveal.
> If I just move it directly to target position (i.e. x < or y < 0) the
> glitch doesn't reproduce. See also my explanation above.
> 
> > > +}
> > > +
> > > +igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > > +
> > > +expected_output(data);
> > > +}
> > > +
> > >  static void damaged_plane_update(data_t *data)
> > >  {
> > >  igt_plane_t *test_plane = data->test_plane;
> > > @@ -526,6 +626,8 @@ static void damaged_plane_update(data_t
> > > *data)
> > > 
> > >  static void run(data_t *data)
> > >  {
> > > +int i;
> > > +
> > >  igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > > 
> > >  data->screen_changes = 0;
> > > @@ -542,9 +644,15 @@ static void run(data_t *data)
> > >  damaged_plane_update(data);
> > >  }
> > >  break;
> > > -case PLANE_MOVE:
> > > +case PLANE_MOVE_DAMAGED:
> > >  damaged_plane_move(data);
> > >  break;
> > > +case PLANE_MOVE:
> > > +for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> > > +data->pos = i;
> > > +plane_move(data);
> > 
> > Another unexpected change.
> 
> I will try to split the patch as you suggested. PLANE_MOVE_DAMAGED
> here
> is the original testcase. New is PLANE_MOVE. I choose to change enum
> naming because original testcase is also switching moving plane
> content
> and using damage property for that (PLANE_MOVE_DAMAGED). New testcase
> is just moving the plane (PLANE_MOVE).

I still didn't split the patch. To me it looks much cleaner now as I
choose to keep PLANE_MOVE as it is and added new enum
PLANE_MOVE_NO_UPDATE. Please check version 2 and let's split if you
still think it's necessary.

> 
> > > +}
> > > +break;
> > >  default:
> > >  igt_assert(false);
> > >  }
> > > @@ -654,10 +762,20 @@ igt_main
> > >  cleanup(&data);
> > >  }
> > > 
> > > -/* Only for overlay plane */
> > >  data.op = PLANE_MOVE;
> > >  /* Verify overlay plane move selective fetch */
> > >  igt_describe("Test that selective fetch works on moving overlay
> > > plane");
> > 
> > overlay != cursor
> 
> Oops, one more copy paste error. I will fix it.
> 
> > > +igt_subtest_f("cursor-%s-sf", op_str(data.op)) {
> > > +data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > +prepare(&data);
> > > +run(&data);
> > > +cleanup(&data);
> > > +}
> > > +
> > > +/* Only for overlay plane */
> > > +data.op = PLANE_MOVE_DAMAGED;
> > > +/* Verify overlay plane move selective fetch */
> > > +igt_describe("Test that selective fetch works on moving overlay
> > > plane");
> > >  igt_subtest_f("%s-sf-dmg-area", op_str(data.op)) {
> > >  for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
> > >  data.pos = i;
> > > @@ -668,6 +786,16 @@ igt_main
> > >  }
> > >  }
> > > 
> > > +data.op = PLANE_MOVE;
> > > +/* Verify overlay plane move selective fetch */
> > > +igt_describe("Test that selective fetch works on moving overlay
> > > plane");
> > > +igt_subtest_f("overlay-%s-sf", op_str(data.op)) {
> > > +data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > > +prepare(&data);
> > > +run(&data);
> > > +cleanup(&data);
> > > +}
> > > +
> > >  /* Verify primary plane selective fetch with overplay plane
> > > blended */
> > >  data.op = OVERLAY_PRIM_UPDATE;
> > >  igt_describe("Test that selective fetch works on primary plane "
> > 
> > There is a few code style errors.
> > 
> 
> Ok, I will fix these as well.
> 
> > Other thing that you need to pay attention is the patch diff, saw
> > some new line being added to a function that is not being changed.
> > 
> 
> Yes, I will drop those.
> 
> BR,
> 
> Jouni Högander