[igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Tue Apr 9 07:27:44 UTC 2019
On Mon, 2019-04-08 at 18:51 -0700, Souza, Jose wrote:
> On Mon, 2019-04-08 at 10:47 +0100, Lisovskiy, Stanislav wrote:
> > On Fri, 2019-04-05 at 16:31 -0700, José Roberto de Souza wrote:
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > ---
> > > tests/kms_atomic_transition.c | 217 ++++++++++++++++++++------
> > > ----
> > > ----
> > > 1 file changed, 130 insertions(+), 87 deletions(-)
> > >
> > > diff --git a/tests/kms_atomic_transition.c
> > > b/tests/kms_atomic_transition.c
> > > index 18f73317..9988f303 100644
> > > --- a/tests/kms_atomic_transition.c
> > > +++ b/tests/kms_atomic_transition.c
> > > @@ -40,9 +40,18 @@
> > > #define DRM_CAP_CURSOR_HEIGHT 0x9
> > > #endif
> > >
> >
> > Also take a look here:
> > https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2807/
> >
> > This code has just broken all kms_atomic_transition test
> > cases for some platforms.
> > You told that my patch has flaws, didn't explain which and then
> > sent a patch which has more code, conditions, cycles and
> > introducing
> > problems... So why is it better?
>
> I did not properly tested this it was more like a proof of concept,
> that is why the "HAX" in the commit message.
Ok,
>
> It was more to show that we don't that first patch from your series
> at
> all, we should not just skip the whole test iteration because because
> wm_setup_plane() == 0.
>
> Fell free to take some ideas from this patch or not.
>
> Anyways I found the problem and now it is passing:
> https://patchwork.freedesktop.org/series/59191/
Well if you, or anyone is in doubt, just copy-paste this code to
run_transition test in kms_atomic_transition.c:
parms[0].mask = 0; /* GROUP_TYPE_NONE in your patch */
wm_setup_plane(display, pipe, 0, parms, false);
atomic_commit(display, pipe, flags, (void *)(unsigned long)0, false);
wait_for_transition(display, pipe, false, false);
wm_setup_plane(display, pipe, 1, parms, false);
atomic_commit(display, pipe, flags, (void *)(unsigned long)1, false);
wait_for_transition(display, pipe, false, false);
Here we could have i = 0, j = 1 in run_transition.
Reproduces the below issue with 100% reproduction rate:
(kms_atomic_transition:3227) CRITICAL: Test assertion failure function
wait_for_transition, file ../tests/kms_atomic_transition.c:431:
(kms_atomic_transition:3227) CRITICAL: Failed assertion:
fd_completed(display->drm_fd)
Stack trace:
#0 ../lib/igt_core.c:1474 __igt_fail_assert()
#1 ../tests/kms_atomic_transition.c:433 wait_for_transition()
#2 ../tests/kms_atomic_transition.c:540 run_transition_test()
#3 ../tests/kms_atomic_transition.c:966 __real_main933()
#4 ../tests/kms_atomic_transition.c:933 main()
#5 ../csu/libc-start.c:344 __libc_start_main()
#6 [_start+0x2a]
Subtest plane-all-transition-nonblocking failed.
Or this one(i=1, j=3):
parms[0].mask = 1<<2;
parms[1].mask = 0; /* (GROUP_TYPE_NONE in your patch) */
wm_setup_plane(display, pipe, 1, parms, false);
atomic_commit(display, pipe, flags, (void *)(unsigned long)1, false);
wait_for_transition(display, pipe, false, false);
wm_setup_plane(display, pipe, 3, parms, false);
atomic_commit(display, pipe, flags, (void *)(unsigned long)3, false);
wait_for_transition(display, pipe, false, false);
Again gives:
(kms_atomic_transition:3227) CRITICAL: Test assertion failure function
wait_for_transition, file ../tests/kms_atomic_transition.c:431:
(kms_atomic_transition:3227) CRITICAL: Failed assertion:
fd_completed(display->drm_fd)
Stack trace:
#0 ../lib/igt_core.c:1474 __igt_fail_assert()
#1 ../tests/kms_atomic_transition.c:433 wait_for_transition()
#2 ../tests/kms_atomic_transition.c:540 run_transition_test()
#3 ../tests/kms_atomic_transition.c:966 __real_main933()
#4 ../tests/kms_atomic_transition.c:933 main()
#5 ../csu/libc-start.c:344 __libc_start_main()
#6 [_start+0x2a]
Subtest plane-all-transition-nonblocking failed.
Note, this might not be visible in tests or visible from time to time,
depending which planes got removed, for which i were parms[i].mask set,
i.e just a matter of luck or until someone changes the transition
algorithm.
So should we really wait until shit hits the fan?
>
> >
> > > +enum group_type {
> > > + GROUP_TYPE_NONE = 0,
> > > + GROUP_TYPE_PRIMARY = 1 << 0,
> > > + GROUP_TYPE_CURSOR = 1 << 1,
> > > + GROUP_TYPE_OVERLAY = 1 << 2,
> > > + GROUP_TYPE_OVERLAY2 = 1 << 3
> > > +};
> > > +
> > > struct plane_parms {
> > > struct igt_fb *fb;
> > > - uint32_t width, height, mask;
> > > + uint32_t width, height;
> > > + enum group_type mask;
> > > };
> > >
> > > /* globals for fence support */
> > > @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t
> > > *display,
> > > enum pipe pipe,
> > > unsigned *iter_max)
> > > {
> > > uint64_t cursor_width, cursor_height;
> > > - 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 sprite_width, sprite_height, prev_w, prev_h;
> > > igt_plane_t *plane;
> > > - uint32_t iter_mask = 3;
> > > + uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
> > > + bool alpha = true;
> > > + int ret;
> > >
> > > do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > > &cursor_width));
> > > if (cursor_width >= mode->hdisplay)
> > > @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t
> > > *display, enum pipe pipe,
> > > parms[i].fb = primary_fb;
> > > parms[i].width = mode->hdisplay;
> > > parms[i].height = mode->vdisplay;
> > > - parms[i].mask = 1 << 0;
> > > + parms[i].mask = GROUP_TYPE_PRIMARY;
> > > + iter_mask |= GROUP_TYPE_PRIMARY;
> > > } else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > > parms[i].fb = argb_fb;
> > > parms[i].width = cursor_width;
> > > parms[i].height = cursor_height;
> > > - parms[i].mask = 1 << 1;
> > > + parms[i].mask = GROUP_TYPE_CURSOR;
> > > + iter_mask |= GROUP_TYPE_CURSOR;
> > > } else {
> > > parms[i].fb = sprite_fb;
> > > - parms[i].mask = 1 << 2;
> > > -
> > > - iter_mask |= 1 << 2;
> > > -
> > > - overlays[n_overlays++] = i;
> > > + parms[i].mask = GROUP_TYPE_OVERLAY;
> > > + iter_mask |= GROUP_TYPE_OVERLAY;
> > > + n_overlays++;
> > > + if (alpha)
> > > + alpha = igt_plane_has_format_mod(plane,
> > > + DRM_FO
> > > RMAT_ARGB8888,
> > > + LOCAL_
> > > DRM_FORMAT_MOD_NONE);
> > > }
> > > }
> > >
> > > - if (n_overlays >= 2) {
> > > - uint32_t i;
> > > + prev_w = sprite_width = cursor_width;
> > > + prev_h = sprite_height = cursor_height;
> > > +
> > > + igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > > + DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > argb_fb);
> > > +
> > > + igt_create_fb(display->drm_fd, sprite_width, sprite_height,
> > > + alpha ? DRM_FORMAT_ARGB8888 :
> > > DRM_FORMAT_XRGB8888,
> > > + LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > > +
> > > + max_overlays = n_overlays;
> > > +retry_bw:
> > > + /* Limit the number of planes */
> > > + while (max_overlays < n_overlays) {
> > > + int i = hars_petruska_f54_1_random_unsafe_max(display-
> > > > pipes[pipe].n_planes);
> > >
> > >
> > > /*
> > > - * Create 2 groups for overlays, make sure 1 plane is
> > > put
> > > - * in each then spread the rest out.
> > > + * Always keep primary and cursor and discard already
> > > + * removed planes
> > > */
> > > - iter_mask |= 1 << 3;
> > > - parms[overlays[n_overlays - 1]].mask = 1 << 3;
> > > + if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > > + continue;
> > >
> > > - for (i = 1; i < n_overlays - 1; i++) {
> > > - int val =
> > > hars_petruska_f54_1_random_unsafe_max(2);
> > > + parms[i].mask = GROUP_TYPE_NONE;
> > > + n_overlays--;
> > > + }
> > >
> > > - parms[overlays[i]].mask = 1 << (2 + val);
> > > - }
> > > + /*
> > > + * Check if there is enough bandwidth for the current number of
> > > planes.
> > > + * As the plane size and position is not taken into account we
> > > can do
> > > + * this earlier.
> > > + */
> > > + set_sprite_wh(display, pipe, parms, sprite_fb, alpha,
> > > sprite_width,
> > > + sprite_height);
> > > + wm_setup_plane(display, pipe, iter_mask, parms, false);
> > > +
> > > + /* It should be able to handle at least primary and cursor */
> > > + if (!max_overlays) {
> > > + iter_mask &= ~GROUP_TYPE_OVERLAY;
> > > + *iter_max = iter_mask + 1;
> > > + return;
> > > }
> > >
> > > - igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > > - DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > argb_fb);
> > > + ret = igt_display_try_commit_atomic(display,
> > > DRM_MODE_ATOMIC_TEST_ONLY |
> > > + DRM_MODE_ATOMIC_ALLOW_MODES
> > > ET,
> > > + NULL);
> > > + /*
> > > + * Could mean other errors but this is also the error returned
> > > when
> > > + * there is not enough bandwidth for all the planes
> > > + */
> > > + if (ret == -EINVAL) {
> > > + max_overlays--;
> > > + goto retry_bw;
> > > + }
> > >
> > > - igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > > - DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > sprite_fb);
> > > + igt_assert_f(!ret, "Error %i not expected by try_commit()\n",
> > > ret);
> > >
> > > - *iter_max = iter_mask + 1;
> > > - if (!n_overlays)
> > > - return;
> > > + /* So it have enough bandwidth for n_overlays planes */
> > >
> > > /*
> > > - * Pre gen9 not all sizes are supported, find the biggest
> > > possible
> > > - * size that can be enabled on all sprite planes.
> > > + * Create 2 groups for overlays, make sure 1 plane is put in
> > > each then
> > > + * spread the rest out.
> > > */
> > > -retry:
> > > - prev_w = sprite_width = cursor_width;
> > > - prev_h = sprite_height = cursor_height;
> > > + iter_mask &= ~GROUP_TYPE_OVERLAY;
> > > + for_each_plane_on_pipe(display, pipe, plane) {
> > > + int i = plane->index;
> > >
> > > - max_sprite_width = (sprite_width == mode->hdisplay);
> > > - max_sprite_height = (sprite_height == mode->vdisplay);
> > > + if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > > + continue;
> > >
> > > - while (1) {
> > > - int ret;
> > > + /* First overlay plane will be overlay group 1 */
> > > + if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
> > > + iter_mask |= GROUP_TYPE_OVERLAY;
> > > + continue;
> > > + }
> > >
> > > - set_sprite_wh(display, pipe, parms, sprite_fb,
> > > - alpha, sprite_width, sprite_height);
> > > + /* Second overlay plane will be overlay group 1 */
> > > + if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
> > > + iter_mask |= GROUP_TYPE_OVERLAY2;
> > > + parms[i].mask = GROUP_TYPE_OVERLAY2;
> > > + continue;
> > > + }
> > >
> > > - wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > > parms, false);
> > > - 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));
> > > + /* Sort the group of the rest of overlay planes */
> > > + if (hars_petruska_f54_1_random_unsafe_max(2))
> > > + parms[i].mask = GROUP_TYPE_OVERLAY2;
> > > + }
> > >
> > > - 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;
> > > - }
> > > + *iter_max = iter_mask + 1;
> > >
> > > + /*
> > > + * Pre gen9 not all sizes are supported, find the biggest
> > > possible
> > > + * size that can be enabled on all sprite planes.
> > > + */
> > > + while (1) {
> > > + /* Size limit reached */
> > > + if (is_atomic_check_plane_size_errno(ret)) {
> > > 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 {
> > > - prev_w = sprite_width;
> > > - prev_h = sprite_height;
> > > + break;
> > > }
> > >
> > > - if (!max_sprite_width) {
> > > - sprite_width *= 2;
> > > + /* Commit is valid and it reached max size, use this
> > > size */
> > > + if (sprite_width == mode->hdisplay ||
> > > + sprite_height == mode->vdisplay)
> > > + break;
> > >
> > > - if (sprite_width >= mode->hdisplay) {
> > > - max_sprite_width = true;
> > > + prev_w = sprite_width;
> > > + prev_h = sprite_height;
> > >
> > > - sprite_width = mode->hdisplay;
> > > - }
> > > - } else if (!max_sprite_height) {
> > > - sprite_height *= 2;
> > > + sprite_width *= 2;
> > > + if (sprite_width >= mode->hdisplay)
> > > + sprite_width = mode->hdisplay;
> > >
> > > - if (sprite_height >= mode->vdisplay) {
> > > - max_sprite_height = true;
> > > + sprite_height *= 2;
> > > + if (sprite_height >= mode->vdisplay)
> > > + sprite_height = mode->vdisplay;
> > >
> > > - sprite_height = mode->vdisplay;
> > > - }
> > > - } else
> > > - /* Max sized sprites for all! */
> > > - break;
> > > + set_sprite_wh(display, pipe, parms, sprite_fb,
> > > sprite_width,
> > > + sprite_height, alpha);
> > > + wm_setup_plane(display, pipe, iter_mask, parms, false);
> > > + ret = igt_display_try_commit_atomic(display,
> > > + DRM_MODE_ATOMIC_TES
> > > T_ONLY |
> > > + DRM_MODE_ATOMIC_ALL
> > > OW_MODESET,
> > > + NULL);
> > > }
> > >
> > > - igt_info("Running test on pipe %s with resolution %dx%d and
> > > sprite size %dx%d alpha %i\n",
> > > + igt_info("Running test on pipe %s with resolution %dx%d, sprite
> > > size %dx%d, alpha %i and %i overlay planes\n",
> > > kmstest_pipe_name(pipe), mode->hdisplay, mode-
> > > > vdisplay,
> > >
> > > - sprite_width, sprite_height, alpha);
> > > + sprite_width, sprite_height, alpha, n_overlays);
> > > }
> > >
> > > static void prepare_fencing(igt_display_t *display, enum pipe
> > > pipe)
> > > @@ -519,8 +560,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 !=
> > > GROUP_TYPE_NONE)
> > > + igt_plane_set_position(plane, 0, 0);
> > > + }
> > >
> > > igt_display_commit2(display, COMMIT_ATOMIC);
> > >
More information about the igt-dev
mailing list