[igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
Souza, Jose
jose.souza at intel.com
Tue Apr 9 01:41:08 UTC 2019
On Mon, 2019-04-08 at 09:38 +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
> >
> > +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;
>
> You were arguing that parms[i].mask can't be 0. Now what if you call
> now wm_setup_plane with it as a parameter? :)
There is no problem in have mask = 0, the problem is say that is mask =
0 in the commit message but that don't happen.
>
> > + 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;
> > }
>
> You could just take care about this organizing cycle above properly.
> Moreover you could bail out, checking that much earlier, thus
> reducing
> the amount of code executed.
The calls to set_sprite_wh() and wm_setup_plane() in !max_overlays
conditions takes care of free the framebuffers.
>
> >
> > - 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;
> > + }
>
> There was a macro for that, in fact I have agreed with Daniel, so
> that we'll have a macro here to see if there is a "correct" return
> value or if we need to fail. What if ret is ENOSPACE? Or something
> else? We had a bug about this. Now you are going to make it fail
> somewhere else again.
Yeah it should return a error for anything != 0 after check for
-EINVAL.
>
> Also
>
> >
> > - 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;
> > + }
> >
>
> Why it is so complex? Previously we were setting it only once.
> Now you have two more conditions, checking and setting it second
> time,
> introducing more entropy.
Here it is only randomizing the groups once, after the number of planes
that fits in BW is founds.
>
> Moreover this is very unreadable and hard to check which errors
> it can bring.
>
> > - 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);
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20190409/476e0f27/attachment-0001.sig>
More information about the igt-dev
mailing list