[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
Tue Apr 16 08:52:07 UTC 2019
On Fri, Apr 05, 2019 at 07:20:15AM +0000, Lisovskiy, Stanislav wrote:
> On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> > 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
>
> That's a topic for much larger discussion, I have been hearing around
> for almost a month :)
> I have been talking with Ville and Martin a lot about this.
> And yeah to me it is also controversial, that you have planes, but they
> are not always usable, however...
> I completely understand all the pros and cons, you
> are describing here. But that really goes far beyond that IGT patch.
> I'm just dealing with reality what we have currently here and trying to
> make it not to fail miserably at least. Once we will have "something
> better" that you describe, I would be happy to implement a
> correspondent changes.
We _do_ have this "something better", since day 1 of atomic. It's called
try_commit.
And yes "some planes sometimes don't work for reasons only clear to the
kernel driver" has also been part of atomic since day one.
Please don't merge hacks to handle management pressure, let's do the right
thing. I'm happy to handle managers for you who don't get this. Cutting
corners here doesn't help anyone (and yes this should have been caught
when the test was originally written, but well it wasnt).
-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