Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

Lisovskiy, Stanislav
2019-04-04T11:45:07Z

On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy wrote:
> > With some upcoming changes i915 might not allow
> > all sprite planes enabled, depending on available
> > bandwidth limitation. Thus the test need to decrement
> > amount of planes and try again, instead of panicking.
> >
> > v2: Removed excessive nested conditions, making code a bit
> > more readable(hopefully).
> >
> > v3: Stopped using global n_planes variable as it might cause
> > resources being unreleased.
> > Using now parms[i].mask as a way to determine if plane
> > has to be included into commit.
> >
> > v4: Removed unneeded n_overlays initialization.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> > tests/kms_atomic_transition.c | 107 +++++++++++++++++-------------
> > ----
> > 1 file changed, 55 insertions(+), 52 deletions(-)
> >
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 638fe17e..a04220f3 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> > unsigned sprite_width, sprite_height, prev_w, prev_h;
> > bool max_sprite_width, max_sprite_height, alpha = true;
> > uint32_t n_planes = display->pipes[pipe].n_planes;
> > - uint32_t n_overlays = 0, overlays[n_planes];
> > + uint32_t n_overlays, overlays[n_planes];
> > igt_plane_t *plane;
> > - uint32_t iter_mask = 3;
> > + uint32_t iter_mask;
> > + int retries = n_planes - 1;
> > + int ret = 0;
> >
> > do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > &cursor_width));
> > if (cursor_width >= mode->hdisplay)
> > @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> > if (cursor_height >= mode->vdisplay)
> > cursor_height = mode->vdisplay;
> >
> > +retry:
> > + n_overlays = 0;
> > + iter_mask = 3;
> > +
> > for_each_plane_on_pipe(display, pipe, plane) {
> > int i = plane->index;
> >
> > @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> > parms[i].height = cursor_height;
> > parms[i].mask = 1 << 1;
> > } else {
> > - parms[i].fb = sprite_fb;
> > - parms[i].mask = 1 << 2;
> > -
> > - iter_mask |= 1 << 2;
> > -
> > - overlays[n_overlays++] = i;
> > + if (n_overlays < (n_planes - 2)) {
> > + parms[i].fb = sprite_fb;
> > + parms[i].mask = 1 << 2;
> > + iter_mask |= 1 << 2;
> > + overlays[n_overlays++] = i;
> > + }
> > + else {
> > + parms[i].fb = NULL;
> > + parms[i].mask = 0;
> > + }
> > }
> > }
> >
> > @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> > * Pre gen9 not all sizes are supported, find the biggest
> > possible
> > * size that can be enabled on all sprite planes.
> > */
> > -retry:
> > prev_w = sprite_width = cursor_width;
> > prev_h = sprite_height = cursor_height;
> >
> > max_sprite_width = (sprite_width == mode->hdisplay);
> > max_sprite_height = (sprite_height == mode->vdisplay);
> >
> > - while (1) {
> > - int ret;
> > -
> > + while (!max_sprite_width && !max_sprite_height) {
> > set_sprite_wh(display, pipe, parms, sprite_fb,
> > alpha, sprite_width, sprite_height);
> >
> > @@ -295,54 +302,49 @@ retry:
> > ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > igt_assert(!is_atomic_check_failure_errno(ret));
> >
> > - if (is_atomic_check_plane_size_errno(ret)) {
> > - if (cursor_width == sprite_width &&
> > - cursor_height == sprite_height) {
> > - igt_assert_f(alpha,
> > - "Cannot configure the
> > test with all sprite planes enabled\n");
> > -
> > - /* retry once with XRGB format. */
> > - alpha = false;
> > - goto retry;
> > - }
> > -
> > - sprite_width = prev_w;
> > - sprite_height = prev_h;
> > -
> > - if (max_sprite_width && max_sprite_height) {
> > - set_sprite_wh(display, pipe, parms,
> > sprite_fb,
> > - alpha, sprite_width,
> > sprite_height);
> > - break;
> > - }
> > -
> > - if (!max_sprite_width)
> > - max_sprite_width = true;
> > - else
> > - max_sprite_height = true;
> > - } else {
> > + if (!is_atomic_check_plane_size_errno(ret)) {
> > prev_w = sprite_width;
> > prev_h = sprite_height;
> > - }
> > -
> > - if (!max_sprite_width) {
> > - sprite_width *= 2;
> > -
> > +
> > + sprite_width *= max_sprite_width ? 1 : 2;
> > if (sprite_width >= mode->hdisplay) {
> > max_sprite_width = true;
> > -
> > sprite_width = mode->hdisplay;
> > }
> > - } else if (!max_sprite_height) {
> > - sprite_height *= 2;
> >
> > + sprite_height *= max_sprite_height ? 1 : 2;
> > if (sprite_height >= mode->vdisplay) {
> > max_sprite_height = true;
> > -
> > sprite_height = mode->vdisplay;
> > }
> > - } else
> > - /* Max sized sprites for all! */
> > - break;
> > + continue;
> > + }
> > +
> > + if (cursor_width == sprite_width &&
> > + cursor_height == sprite_height) {
> > + igt_assert_f(retries > 0,
> > + "Cannot configure the test with
> > all sprite planes enabled\n");
> > + --retries;
> > + /* retry once with XRGB format. */
> > + if (alpha) {
> > + alpha = false;
> > + igt_warn("Removed alpha\n");
> > + }
> > + else {
> > + igt_assert_f(n_planes > 1, "No planes
> > left to proceed with!");
> > + n_planes--;
>
> Same comment as with kms_concurrent and all the other plane tests:
> Just
> dropping the last plane reduces our test coverage, we need to
> randomize
> which plane we drop.

I think that is a good idea, I'm now testing a patch which randomizes
which one of available planes is going to be removed.

For the latter change I would leave for further patches as currently
it doesn't matter anyway, but we need the issues related to new bw
requirements to be fixed.
>
> The other bit: We need to recompute how many planes we can enable for
> every transition. Right now that's not the case with Ville's current
> patch
> series, but that's also just an interim solution. Actual bw
> consumption
> very much depends upon where all the planes are place, how big they
> are
> and all that stuff. So we need to recompute/recheck that every
> transition
> works, for each specific transition. Because the exact transition
> might
> also matter.
>
> Or maybe I'm not entirely understanding how this test works here.
> -Daniel
>
> > + igt_warn("Reduced available planes to
> > %d\n", n_planes);
> > + }
> > + goto retry;
> > + }
> > +
> > + sprite_width = prev_w;
> > + sprite_height = prev_h;
> > +
> > + if (!max_sprite_width)
> > + max_sprite_width = true;
> > + else
> > + max_sprite_height = true;
> > }
> >
> > igt_info("Running test on pipe %s with resolution %dx%d and
> > sprite size %dx%d alpha %i\n",
> > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display,
> > enum pipe pipe, igt_output_t *output
> >
> > if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> > igt_output_set_pipe(output, PIPE_NONE);
> > -
> > igt_display_commit2(display, COMMIT_ATOMIC);
> >
> > igt_output_set_pipe(output, pipe);
> > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t *display,
> > enum pipe pipe, igt_output_t *output
> > }
> >
> > /* force planes to be part of commit */
> > - for_each_plane_on_pipe(display, pipe, plane)
> > - igt_plane_set_position(plane, 0, 0);
> > + for_each_plane_on_pipe(display, pipe, plane) {
> > + if (parms[plane->index].mask)
> > + igt_plane_set_position(plane, 0, 0);
> > + }
> >
> > igt_display_commit2(display, COMMIT_ATOMIC);
> >
> > --
> > 2.17.1