[igt-dev] [PATCH i-g-t] tests/kms_psr2_sf: Add new testcases for moving plane
Hogander, Jouni
jouni.hogander at intel.com
Wed Feb 23 13:44:35 UTC 2022
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 at intel.com>
> > > Signed-off-by: Jouni Högander <jouni.hogander at 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
More information about the igt-dev
mailing list