Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Tue, 2019-04-16 at 14:27 +0200, Daniel Vetter wrote:
> 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@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,
> > > > > > > > 
> 
> 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.

I agree that we need such test, kms_atomic_transition now at least
tries to randomize which plane will be thrown away, once we run
into bw limits.
I'm also writing now a stress test case, which attempts to use allpipes/planes simultaneously with gpu/cpu load. 
However it doesn't have a randomization, but only tries to enable
everything it can simultaneously at least up to the limits when
try_commit fails. 

So do you think we should add this format/tiling randomized
configuration testing to kms_atomic_transition or have some other test
case for that?

> 
> 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
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev