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 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,
> >  	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.
> 
> 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