[igt-dev] [PATCH i-g-t v3] tests/kms_psr2_sf: Add new testcases for moving plane without updates
Souza, Jose
jose.souza at intel.com
Wed Mar 9 19:06:06 UTC 2022
On Fri, 2022-02-25 at 12:05 +0200, Jouni Högander wrote:
> Add new testcases which are moving cursor and overlay planes
> without updating plane contents or setting any damage areas.
>
> Cursor testcase is reproducing following bug:
>
> https://gitlab.freedesktop.org/drm/intel/-/issues/5077
>
> Current *-plane-move-sf-dmg-area is not suitable for this
> purpose. It's blinking display on cleanup/prepare and also updating
> contents of cursor plane. This makes it harder to observe possible
> flickering. Patch is keeping original *-plane-move-sf-dmg-area as it
> is and just adding a new testcase.
>
> New testcase is moving the plane first inside pipe area. Then it's
> moving it to x < 0 and last to y < 0. This is done step-by-step
> because issue mentioned above is not triggered if the plane is just
> moved to x < 0 or y < 0.
>
> v2:
> - Keep PLANE_MOVE enum as it was
> - Remove changes to functions that are not really modified
> - Fix several copy paste errors
> v3:
> - Use INTERACTIVE_PLANE_MOVE instead of PLANE_MOVE_NO_UPDATE
> - Fix coding style errors
> - Move psr_wait_entry before moving the plane
> - Cur_x/cur_y usage made more clear
>
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> ---
> tests/i915/kms_psr2_sf.c | 132 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 131 insertions(+), 1 deletion(-)
>
> diff --git a/tests/i915/kms_psr2_sf.c b/tests/i915/kms_psr2_sf.c
> index 36f8a786..f9fa5620 100644
> --- a/tests/i915/kms_psr2_sf.c
> +++ b/tests/i915/kms_psr2_sf.c
> @@ -44,6 +44,7 @@ enum operations {
> PLANE_UPDATE,
> PLANE_UPDATE_CONTINUOUS,
> PLANE_MOVE,
> + INTERACTIVE_PLANE_MOVE,
Sorry, I have suggested this name but I did not saw that we already have something like it so would be nice to rename it and the functions to:
PLANE_MOVE_CONTINUOUS
> 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,6 +85,7 @@ static const char *op_str(enum operations op)
> static const char * const name[] = {
> [PLANE_UPDATE] = "plane-update",
> [PLANE_UPDATE_CONTINUOUS] = "plane-update-continuous",
> + [INTERACTIVE_PLANE_MOVE] = "interactive-plane-move",
> [PLANE_MOVE] = "plane-move",
> [OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
> };
> @@ -406,6 +411,42 @@ static void plane_move_expected_output(enum plane_move_postion pos)
> manual(expected);
> }
>
> +static void interactive_plane_move_expected_output(enum plane_move_postion pos)
> +{
> + char expected[73] = {};
For stack allocation go ahead and shoot it up and round, this avoid overflow when someone changes some string that will not fit in 73 bytes.
expected[128]...64, 128, 256...
> +
> + 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] = {};
> @@ -424,6 +465,9 @@ static void expected_output(data_t *data)
> case PLANE_MOVE:
> plane_move_expected_output(data->pos);
> break;
> + case INTERACTIVE_PLANE_MOVE:
> + interactive_plane_move_expected_output(data->pos);
> + break;
> case PLANE_UPDATE:
> plane_update_expected_output(data->test_plane_id,
> data->damage_area_count,
> @@ -487,6 +531,59 @@ static void damaged_plane_move(data_t *data)
> expected_output(data);
> }
>
> +static void interactive_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);
> + }
> +
> + igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> +
> + 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);
> + }
> +
> + expected_output(data);
> +}
> +
> static void damaged_plane_update(data_t *data)
> {
> igt_plane_t *test_plane = data->test_plane;
> @@ -526,6 +623,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;
> @@ -545,6 +644,17 @@ static void run(data_t *data)
> case PLANE_MOVE:
> damaged_plane_move(data);
> break;
> + case INTERACTIVE_PLANE_MOVE:
> + /*
> + * Start from top left corner and keep plane position
> + * over iterations.
> + */
> + data->cur_x = data->cur_y = 0;
> + for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> + data->pos = i;
> + interactive_plane_move(data);
> + }
> + break;
> default:
> igt_assert(false);
> }
> @@ -654,6 +764,16 @@ igt_main
> cleanup(&data);
> }
>
> + data.op = INTERACTIVE_PLANE_MOVE;
> + /* Verify cursor plane move */
This comment should mention the continuous/interactive move but I guess the igt_describe that you filled is enough, it is duplicated information here
and in other subtests, I vote to just drop this new comments.
The rest LGTM, with the changes above you can include:
Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
> + igt_describe("Test that selective fetch works on moving cursor plane (no update)");
> + 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;
> /* Verify overlay plane move selective fetch */
> @@ -668,6 +788,16 @@ igt_main
> }
> }
>
> + data.op = INTERACTIVE_PLANE_MOVE;
> + /* Verify overlay plane move */
> + igt_describe("Test that selective fetch works on moving overlay plane (no update)");
> + 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 "
More information about the igt-dev
mailing list