[igt-dev] [PATCH i-g-t v2] tests/kms_psr2_sf: Add new testcases for moving plane without updates
Hogander, Jouni
jouni.hogander at intel.com
Fri Feb 25 10:06:54 UTC 2022
Hello Jose,
Sent new version please check my responses inline and review new
version.
On Thu, 2022-02-24 at 14:23 +0000, Souza, Jose wrote:
> On Thu, 2022-02-24 at 13:02 +0000, Hogander, Jouni wrote:
> > On Wed, 2022-02-23 at 17:55 +0000, Souza, Jose wrote:
> > > On Wed, 2022-02-23 at 15:42 +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
> > > >
> > > > 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 | 129
> > > > ++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 128 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tests/i915/kms_psr2_sf.c
> > > > b/tests/i915/kms_psr2_sf.c
> > > > index 36f8a786..ab8fb560 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,
> > > > +PLANE_MOVE_NO_UPDATE,
> > > > OVERLAY_PRIM_UPDATE
> > > > };
> > > > plane_move_no_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",
> > > > +[PLANE_MOVE_NO_UPDATE] = "plane-move-no-update",
> > > > [PLANE_MOVE] = "plane-move",
> > > > [OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
> > > > };
> > > > @@ -335,6 +340,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);
> > > > @@ -406,6 +413,42 @@ static void
> > > > plane_move_expected_output(enum
> > > > plane_move_postion pos)
> > > > manual(expected);
> > > > }
> > > >
> > > > +static void plane_move_no_update_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] = {};
> > > > @@ -424,6 +467,9 @@ static void expected_output(data_t *data)
> > > > case PLANE_MOVE:
> > > > plane_move_expected_output(data->pos);
> > > > break;
> > > > +case PLANE_MOVE_NO_UPDATE:
> > > > +plane_move_no_update_expected_output(data->pos);
> > > > +break;
> > > > case PLANE_UPDATE:
> > > > plane_update_expected_output(data->test_plane_id,
> > > > data->damage_area_count,
> > > > @@ -487,6 +533,59 @@ static void damaged_plane_move(data_t
> > > > *data)
> > > > expected_output(data);
> > > > }
> > > >
> > > > +static void plane_move_no_update(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;
> > >
> > > Code style errors.
> >
> > Yeah, will fix these. Checkpatch.pl didn't noticed these. Sorry for
> > sending again with style errors.
> >
> > > Also this negative coordinates are way off screen, would be
> > > better to
> > > at least part of the plane to be visible.
> >
> > Currently half of the plane is off screen and another half is
> > visible.
> > Is that ok?
>
> okay.
>
> > > The damaged_plane_move() don't have support for this negative
> > > coordinates, please add a function to return the x, y coordinates
> > > and
> > > call this function
> > > for both tests updating the for each of igt_subtest_f("%s-sf-dmg-
> > > area", op_str(data.op)).
> > > This change can go to a this first change can go to a separated
> > > patch
> > > then you implement this new subtest.
> > > With the above I believe you will not need
> > > plane_move_no_update_expected_output().
> > >
> > >
> > >
> > > > +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) {
> > >
> > > cur_x and y is only used in this function, why add to data?
> >
> > I wanted to keep them over iterations here:
> >
> > for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> > data->pos = i;
> > plane_move_no_update(data);
> > }
> >
>
> Huum, makes sense now.
> But I guess would be better to initialize the start coordinate for
> each data->pos.
> Like if running POS_TOP_LEFT, would be better to set initial cur to
> POS_BOTTOM_RIGHT values.
Moved setting starting position here and added comment. Hopefully now
more clear.
>
> > > > +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));
> > >
> > > You are moving the plane but there is no guarantee that PSR is
> > > active
> > > while you are doing it.
> >
> > Not sure what do you mean by that? Test is not run if PSR2 is not
> > supported. There is psr2 enable in the testcase. If the code works
> > as
> > we expect it is supposed to activate psr2 also on plane move. Even
> > thought full_update path is taken on negative coordinates we still
> > have
> > psr2 activated. It is just doing full frame update, no?
>
> enabled != active.
> Call 'igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));'
> before or even better between each commit moving the plane.
Moved as suggested.
>
>
> > > > +
> > > > +expected_output(data);
> > > > +}
> > > > +
> > > > static void damaged_plane_update(data_t *data)
> > > > {
> > > > igt_plane_t *test_plane = data->test_plane;
> > > > @@ -526,6 +625,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 +646,12 @@ static void run(data_t *data)
> > > > case PLANE_MOVE:
> > > > damaged_plane_move(data);
> > > > break;
> > > > +case PLANE_MOVE_NO_UPDATE:
> > > > +for (i = POS_TOP_LEFT; i <= POS_TOP_RIGHT_NEGATIVE; i++) {
> > > > +data->pos = i;
> > > > +plane_move_no_update(data);
> > > > +}
> > > > +break;
> > > > default:
> > > > igt_assert(false);
> > > > }
> > > > @@ -654,6 +761,16 @@ igt_main
> > > > cleanup(&data);
> > > > }
> > > >
> > > > +data.op = PLANE_MOVE_NO_UPDATE;
> > > > +/* Verify cursor plane move */
> > > > +igt_describe("Test that selective fetch works on moving cursor
> > > > plane (no update)");
> > >
> > > NO_UPDATE don't make much sense to me, the biggest difference
> > > between
> > > this and regular PLANE_MOVE is that you are moving the plane a
> > > few
> > > pixels at
> > > each commit until target position.
> > > Would be better call it INTERACTIVE_PLANE_MOVE or something like
> > > that.
> >
> > Ok, let's use that one.
> >
> > > > +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 +785,16 @@ igt_main
> > > > }
> > > > }
> > > >
> > > > +data.op = PLANE_MOVE_NO_UPDATE;
> > > > +/* 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
> > > > "
> >
> > BR,
> >
> > Jouni Högander
More information about the igt-dev
mailing list