[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 12:27:25 UTC 2019
On Tue, Apr 16, 2019 at 09:07:52AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-04-16 at 10:52 +0200, Daniel Vetter wrote:
> > 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.
>
> Well, thats is what most of those fixes are about - we just change test
> case to query first, if that config is going to work, before actually
> committing it. If it doesn't work we try to reduce planes number or
> their size. So what do you exactly mean or don't like here?
>
> >
> > 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).
>
> Well, I can't literally "merge" anything. What I'm doing is changing
> patches to match new bw paradigm. Have you seen bw patches?
> Due those changes we now have -EINVAL returned, whenever we don't have
> resources(kms_concurrent, kms_atomic_transition, kms_plane_multiple..).
> So as you have said we need to use try_commit to understand
> first, if configuration is going to work or not. I'm doing that.
>
> As I understand also BW is not something, we have any other solutions
> for currently.
>
> If you are not happy with it, I understand that - but as you
> understand, I'm definitely not the first guy whom you should refer to
> here.
Maybe there's a misunderstanding. What this patch does here is figure out
how many planes can be used at most using try-commit. But ones it does
that, it also assumes that any combination of these planes (no matter
which dest rectangles we end up picking for them) will work out.
This again encodes an assumption about how i915.ko works, like the
previous assumption that we can use all planes.
What I'd like to see, and how atomic is specified, is that _any_ change in
the configuration (dst/src, pixel format, fb stride, size, tiling, really
anything) could flip the configuration from "works" to "doesn't work".
Worse, the actual transition (i.e. the previous state) also matters. Hence
the usual way an atomic commit should work is:
while {useful_config_left} {
build_config();
ret = try_commit();
if (ret == -EINVAL || ret == -ERANGE) {
reduce configs
} else {
break;
}
}
render_stuff();
commit();
Iow _every_ commit needs to first figure out what's possible. And this is
what we need to do for the more randomized coverage tests that try to make
sure corner cases work. More feature specific (and hence limited) tests
can make more assumption.
Hopefully that explains better what I mean here.
This is relevant for BW calculation since on most hw it matters whether
planes are close together (as measured in horizontal scan lines) or not.
I'm assuming that intel hw is similar here, and Ville already said he'll
try to figure out what's possible and where the real limits are. E.g. if
you use all planes as horizontal bars, we should be able to use all 7. But
we can't use all 7 if you split the screen into vertical bars.
-Daniel
>
> > -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