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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 16, 2019 at 12:43:46PM +0000, Lisovskiy, Stanislav wrote:
> 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?

We need to adjust all the randomized testcases to follow this pattern. Not
new testcases - this really is a testcase bug, since the testcase makes
assumption about hw limitations that aren't holding up in all cases.

I'm also not sure we need even more randomized testcases, we seem to have
quite a few already. They're quite expensive in CI machine time, so adding
more (instead of improving the ones we have) feels like the wrong
direction.

And yes reworking the existing tests to follow the above pattern will be a
lot of work. That's why I'm not happy with pushing the quick workaround
and then forgetting about the real problem.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev