[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
Mon Apr 8 09:47:36 UTC 2019
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?
> +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