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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@intel.com>
> ---
>  tests/kms_atomic_transition.c | 123 ++++++++++++++++++++--------------
>  1 file changed, 71 insertions(+), 52 deletions(-)
>
> diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
> index 638fe17e..a37f1d46 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -212,9 +212,12 @@ 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;
> +	uint32_t planes_to_remove;
>  
>  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH, &cursor_width));
>  	if (cursor_width >= mode->hdisplay)
> @@ -224,6 +227,11 @@ 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;
> +	planes_to_remove = display->pipes[pipe].n_planes - n_planes;
> +
>  	for_each_plane_on_pipe(display, pipe, plane) {
>  		int i = plane->index;
>  
> @@ -238,12 +246,30 @@ 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;
> +			/* Randomize if we remove that sprite plane or not */
> +			uint32_t remove = hars_petruska_f54_1_random_unsafe_max(2);
> +			/*
> +			 * Figure out how much sprite planes left:
> +			 * planes left = overall amount - 1 primary - 1 cursor - sprite planes used
> +			 */
> +			int planes_left = display->pipes[pipe].n_planes - 2 - n_overlays;
> +			/*
> +			 * Remove the plane if remove is set and we have sprite planes to remove.
> +			 * If we have left same or less amount of planes than we need
> +			 * to remove then no randomization, just remove.
> +			 */
> +			if ((remove && planes_to_remove > 0) || (planes_to_remove >= planes_left)) {
> +				parms[i].fb = NULL;
> +				parms[i].mask = 0;
> +				planes_to_remove--;
> +				igt_info("Removed plane %d\n", i);
> +			}
> +			else {
> +				parms[i].fb = sprite_fb;
> +				parms[i].mask = 1 << 2;
> +				iter_mask |= 1 << 2;
> +				overlays[n_overlays++] = i;
> +			}
>  		}
>  	}
>  
> @@ -278,16 +304,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 +318,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_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.

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.

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.

~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);
>  


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev