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

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

On Tue, 2019-04-02 at 13:42 -0700, Souza, Jose wrote:
> On Mon, 2019-04-01 at 14:23 +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.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  tests/kms_atomic_transition.c | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 638fe17e..e4dee7e2 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -214,7 +214,8 @@ static void setup_parms(igt_display_t *display,
> > enum pipe pipe,
> >  	uint32_t n_planes = display->pipes[pipe].n_planes;
> >  	uint32_t n_overlays = 0, overlays[n_planes];
> 

Hi,

First of all, thanks for fast review :)

> Nitpick: you don't need to initialize n_overlays here as you do right
> after the retry label.

True, I've actually removed it, but during next refactoring phase added
back.
> 
> >  	igt_plane_t *plane;
> > -	uint32_t iter_mask = 3;
> > +	uint32_t iter_mask;
> > +	int retries = n_planes - 1;
> >  
> >  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > &cursor_width));
> >  	if (cursor_width >= mode->hdisplay)
> > @@ -224,6 +225,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;
> >  
> > @@ -278,7 +283,6 @@ 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;
> >  
> > @@ -298,12 +302,22 @@ retry:
> >  		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;
> > +				if (--retries >= 0) {
> 
> The 7 first loops it is going to this, on the 8 it will try for the
> first time to reduce the number of planes.
> Also this way we will only test without alpha in platforms with a
> high
> number of planes.

In fact as I see this code above is from older revision of a patch,
I've refactored it to remove excessive nesting(asked by Daniel), so now
there is a newer version. 

Please see https://patchwork.freedesktop.org/series/58728/

However currently loop looks like this: 

1) We enter a loop, try to commit, if we fail then we check the size
of a plane, if it is bigger than cursor, we assign to a previous
size and mark correspondent dimension(width or height) as it has
reached max sprite size. If both has reached, we exit this loop
2) If plane is still of a cursor plane size, try first without alpha
3) If alpha is set to false already, decrement amount of planes, if it
is bigger than 1 or assert if not.

> 
> > +					/* retry once with XRGB format.
> > */
> > +					if (alpha) {
> > +						alpha = false;
> > +					}
> > +					else if (display-
> > > pipes[pipe].n_planes > 0) {
> > 
> > +						display-
> > > pipes[pipe].n_planes--;
> 
> When you do this wm_setup_plane() will iterate in one less plane, and
> not removing framebuffer from that plane, this is probably why you
> needed the first patch.

I think you are right here, decrementing display.pipe[pipe].n_planes is
probably a bad idea at all, otherwise I will have to manually remove FB
from those, while it has to been cleaned up in wm_setup_plane. Probably
need just to add one check to for_each_plane_on_pipe loops to stop
iterating, once max allowed planes is reached.

> 
> > +						igt_warn("Reduced
> > available planes to %d\n",
> > +							    display-
> > > pipes[pipe].n_planes);
> > 
> > +					}
> > +					n_planes = display-
> > > pipes[pipe].n_planes;
> > 
> > +					igt_assert_f(n_planes > 0, "No
> > planes left to proceed with!");
> > +					goto retry;
> > +				}
> > +				igt_assert_f(retries > 0,
> > +				      "Cannot configure the test with
> > all sprite planes enabled\n");
> >  			}
> >  
> >  			sprite_width = prev_w;
> 
> 
> My suggestion:
> 
> Right after "max_sprite_height = (sprite_height == mode->vdisplay);"
> test if all planes support alpha by using:
> igt_plane_has_format_mod(plane, plane_format, plane_modifier).
> 
> Then write a new loop testing the maximum number of planes supported
> and remember to remove the framebuffer from planes.

I guess if I will not decrement display.pipe[pipe].n_planes, but rather
add additional check to for_each_plane_on_pipe loops, wm_setup_plane
should do this job. So I will not have to take care of it separately. 

> 
> And finally run this final loop that will find the max size for
> sprite
> planes.
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev