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

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
> 

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