[igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes
Daniel Vetter
daniel at ffwll.ch
Thu Apr 4 12:56:27 UTC 2019
On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy, Stanislav wrote:
> 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 at 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.
Just doing some bug scrub and we have some EINVAL failures like this all
over the place. So I guess there's some other cases where we reject
configs in the kernel and then the test falls over. We'll need this (it's
how atomic is meant to work really).
Also, the bw band-aid from Ville is a really bad solution, we need
something better - no point in having 7 planes if you can't even use them.
Adding more try_commit in the wrong place feels like a detour.
-Daniel
> >
> > 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
> > >
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the igt-dev
mailing list