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

[Lisovskiy, Stanislav]

On Tue, 2019-04-09 at 13:23 +0200, Maarten Lankhorst wrote:
> Op 08-04-2019 om 12:18 schreef Stanislav Lisovskiy:
> > 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.
> > 
> > v5: Randomize which of sprite planes to remove if hitting
> >     resource limits.
> > 
> > v6: Replace igt_warn with igt_info, to make IGT tests happier.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > ---
> >  tests/kms_atomic_transition.c | 123 ++++++++++++++++++++--------
> > ------
> >  1 file changed, 71 insertions(+), 52 deletions(-)
> > 


> > +
> > +		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_info("Removed alpha\n");
> > +			}
> > +			else {
> > +				igt_assert_f(n_planes > 1, "No planes
> > left to proceed with!");
> > +				n_planes--;
> > +				igt_info("Reduced available planes to
> > %d\n", n_planes);
> > +			}
> > +			goto retry;
> 
> You've removed the alpha fallback? It should be removed, now that
> igt_plane_has_format_mod exists;
> even so, I would do it in a separate preparation patch, otherwise
> earlier platforms may start failing.

No I haven't. I made it so, that it first tries to remove alpha and 
only if this was not enough(alpha == false), start to play with planes.
I agree igt_plane_has_format_mod could be helpful to save at least
one
iteration here.

> 
> We could remove 1 plane at a time in this block here, and then do
> goto retry; to the place where it was before
> the patch. I think this would remove the need for a lot of double
> work in the early part of setup_parms, and
> makes the test slightly more deterministic. We would for example
> remove plane 5 first, and then not enable it
> again until the next subtest. And then disable more and more on top
> of that, instead of retrying completely.

I agree, this could be great idea. So that I actually don't even
go back but just disable one random plain and keep going. 

> 
> You don't even need to loop more than once, when encountering an
> enabled overlay plane, set
> if (rand() % (++planes_encountered_so_far)) plane_to_remove = plane;
> and disable the selected plane at the end.
> 
> This way plane 1 has a 100% chance of being removed, if we encounter
> plane 2 it's 50/50 for both, plane 3 has 1/3
> chance of being removed, 2/3 for one of the previous, etc.

Also true! Will do that.

-Stanislav

> 
> ~Maarten
> 
> > +		}
> > +
> > +		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 +481,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 +542,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);
> >  
> 
>