Re: [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 11943 bytes --]

On Mon, 2019-04-08 at 10:47 +0100, Lisovskiy, Stanislav wrote:
> On Fri, 2019-04-05 at 16:31 -0700, José Roberto de Souza wrote:
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  tests/kms_atomic_transition.c | 217 ++++++++++++++++++++----------
> > ----
> >  1 file changed, 130 insertions(+), 87 deletions(-)
> > 
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 18f73317..9988f303 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -40,9 +40,18 @@
> >  #define DRM_CAP_CURSOR_HEIGHT 0x9
> >  #endif
> >  
> 
> Also take a look here: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2807/
> 
> This code has just broken all kms_atomic_transition test
> cases for some platforms. 
> You told that my patch has flaws, didn't explain which and then
> sent a patch which has more code, conditions, cycles and introducing
> problems... So why is it better?

I did not properly tested this it was more like a proof of concept,
that is why the "HAX" in the commit message.

It was more to show that we don't that first patch from your series at
all, we should not just skip the whole test iteration because because
wm_setup_plane() == 0.

Fell free to take some ideas from this patch or not.

Anyways I found the problem and now it is passing: 
https://patchwork.freedesktop.org/series/59191/

> 
> > +enum group_type {
> > +	GROUP_TYPE_NONE = 0,
> > +	GROUP_TYPE_PRIMARY = 1 << 0,
> > +	GROUP_TYPE_CURSOR = 1 << 1,
> > +	GROUP_TYPE_OVERLAY = 1 << 2,
> > +	GROUP_TYPE_OVERLAY2 = 1 << 3
> > +};
> > +
> >  struct plane_parms {
> >  	struct igt_fb *fb;
> > -	uint32_t width, height, mask;
> > +	uint32_t width, height;
> > +	enum group_type mask;
> >  };
> >  
> >  /* globals for fence support */
> > @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t
> > *display,
> > enum pipe pipe,
> >  			unsigned *iter_max)
> >  {
> >  	uint64_t cursor_width, cursor_height;
> > -	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 sprite_width, sprite_height, prev_w, prev_h;
> >  	igt_plane_t *plane;
> > -	uint32_t iter_mask = 3;
> > +	uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
> > +	bool alpha = true;
> > +	int ret;
> >  
> >  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > &cursor_width));
> >  	if (cursor_width >= mode->hdisplay)
> > @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> >  			parms[i].fb = primary_fb;
> >  			parms[i].width = mode->hdisplay;
> >  			parms[i].height = mode->vdisplay;
> > -			parms[i].mask = 1 << 0;
> > +			parms[i].mask = GROUP_TYPE_PRIMARY;
> > +			iter_mask |= GROUP_TYPE_PRIMARY;
> >  		} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> >  			parms[i].fb = argb_fb;
> >  			parms[i].width = cursor_width;
> >  			parms[i].height = cursor_height;
> > -			parms[i].mask = 1 << 1;
> > +			parms[i].mask = GROUP_TYPE_CURSOR;
> > +			iter_mask |= GROUP_TYPE_CURSOR;
> >  		} else {
> >  			parms[i].fb = sprite_fb;
> > -			parms[i].mask = 1 << 2;
> > -
> > -			iter_mask |= 1 << 2;
> > -
> > -			overlays[n_overlays++] = i;
> > +			parms[i].mask = GROUP_TYPE_OVERLAY;
> > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > +			n_overlays++;
> > +			if (alpha)
> > +				alpha = igt_plane_has_format_mod(plane,
> > +								 DRM_FO
> > RMAT_ARGB8888,
> > +								 LOCAL_
> > DRM_FORMAT_MOD_NONE);
> >  		}
> >  	}
> >  
> > -	if (n_overlays >= 2) {
> > -		uint32_t i;
> > +	prev_w = sprite_width = cursor_width;
> > +	prev_h = sprite_height = cursor_height;
> > +
> > +	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > +		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > argb_fb);
> > +
> > +	igt_create_fb(display->drm_fd, sprite_width, sprite_height,
> > +		      alpha ? DRM_FORMAT_ARGB8888 :
> > DRM_FORMAT_XRGB8888,
> > +		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > +
> > +	max_overlays = n_overlays;
> > +retry_bw:
> > +	/* Limit the number of planes */
> > +	while (max_overlays < n_overlays) {
> > +		int i = hars_petruska_f54_1_random_unsafe_max(display-
> > > pipes[pipe].n_planes);
> >  
> >  		/*
> > -		 * Create 2 groups for overlays, make sure 1 plane is
> > put
> > -		 * in each then spread the rest out.
> > +		 * Always keep primary and cursor and discard already
> > +		 * removed planes
> >  		 */
> > -		iter_mask |= 1 << 3;
> > -		parms[overlays[n_overlays - 1]].mask = 1 << 3;
> > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > +			continue;
> >  
> > -		for (i = 1; i < n_overlays - 1; i++) {
> > -			int val =
> > hars_petruska_f54_1_random_unsafe_max(2);
> > +		parms[i].mask = GROUP_TYPE_NONE;
> > +		n_overlays--;
> > +	}
> >  
> > -			parms[overlays[i]].mask = 1 << (2 + val);
> > -		}
> > +	/*
> > +	 * Check if there is enough bandwidth for the current number of
> > planes.
> > +	 * As the plane size and position is not taken into account we
> > can do
> > +	 * this earlier.
> > +	 */
> > +	set_sprite_wh(display, pipe, parms, sprite_fb, alpha,
> > sprite_width,
> > +		      sprite_height);
> > +	wm_setup_plane(display, pipe, iter_mask, parms, false);
> > +
> > +	/* It should be able to handle at least primary and cursor */
> > +	if (!max_overlays) {
> > +		iter_mask &= ~GROUP_TYPE_OVERLAY;
> > +		*iter_max = iter_mask + 1;
> > +		return;
> >  	}
> >  
> > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > argb_fb);
> > +	ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY |
> > +					    DRM_MODE_ATOMIC_ALLOW_MODES
> > ET,
> > +					    NULL);
> > +	/*
> > +	 * Could mean other errors but this is also the error returned
> > when
> > +	 * there is not enough bandwidth for all the planes
> > +	 */
> > +	if (ret == -EINVAL) {
> > +		max_overlays--;
> > +		goto retry_bw;
> > +	}
> >  
> > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > sprite_fb);
> > +	igt_assert_f(!ret, "Error %i not expected by try_commit()\n",
> > ret);
> >  
> > -	*iter_max = iter_mask + 1;
> > -	if (!n_overlays)
> > -		return;
> > +	/* So it have enough bandwidth for n_overlays planes */
> >  
> >  	/*
> > -	 * Pre gen9 not all sizes are supported, find the biggest
> > possible
> > -	 * size that can be enabled on all sprite planes.
> > +	 * Create 2 groups for overlays, make sure 1 plane is put in
> > each then
> > +	 * spread the rest out.
> >  	 */
> > -retry:
> > -	prev_w = sprite_width = cursor_width;
> > -	prev_h = sprite_height = cursor_height;
> > +	iter_mask &= ~GROUP_TYPE_OVERLAY;
> > +	for_each_plane_on_pipe(display, pipe, plane) {
> > +		int i = plane->index;
> >  
> > -	max_sprite_width = (sprite_width == mode->hdisplay);
> > -	max_sprite_height = (sprite_height == mode->vdisplay);
> > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > +			continue;
> >  
> > -	while (1) {
> > -		int ret;
> > +		/* First overlay plane will be overlay group 1 */
> > +		if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
> > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > +			continue;
> > +		}
> >  
> > -		set_sprite_wh(display, pipe, parms, sprite_fb,
> > -			      alpha, sprite_width, sprite_height);
> > +		/* Second overlay plane will be overlay group 1 */
> > +		if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
> > +			iter_mask |= GROUP_TYPE_OVERLAY2;
> > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > +			continue;
> > +		}
> >  
> > -		wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > parms, false);
> > -		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));
> > +		/* Sort the group of the rest of overlay planes */
> > +		if (hars_petruska_f54_1_random_unsafe_max(2))
> > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > +	}
> >  
> > -		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;
> > -			}
> > +	*iter_max = iter_mask + 1;
> >  
> > +	/*
> > +	 * Pre gen9 not all sizes are supported, find the biggest
> > possible
> > +	 * size that can be enabled on all sprite planes.
> > +	 */
> > +	while (1) {
> > +		/* Size limit reached */
> > +		if (is_atomic_check_plane_size_errno(ret)) {
> >  			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 {
> > -			prev_w = sprite_width;
> > -			prev_h = sprite_height;
> > +			break;
> >  		}
> >  
> > -		if (!max_sprite_width) {
> > -			sprite_width *= 2;
> > +		/* Commit is valid and it reached max size, use this
> > size */
> > +		if (sprite_width == mode->hdisplay ||
> > +		    sprite_height == mode->vdisplay)
> > +			break;
> >  
> > -			if (sprite_width >= mode->hdisplay) {
> > -				max_sprite_width = true;
> > +		prev_w = sprite_width;
> > +		prev_h = sprite_height;
> >  
> > -				sprite_width = mode->hdisplay;
> > -			}
> > -		} else if (!max_sprite_height) {
> > -			sprite_height *= 2;
> > +		sprite_width *= 2;
> > +		if (sprite_width >= mode->hdisplay)
> > +			sprite_width = mode->hdisplay;
> >  
> > -			if (sprite_height >= mode->vdisplay) {
> > -				max_sprite_height = true;
> > +		sprite_height *= 2;
> > +		if (sprite_height >= mode->vdisplay)
> > +			sprite_height = mode->vdisplay;
> >  
> > -				sprite_height = mode->vdisplay;
> > -			}
> > -		} else
> > -			/* Max sized sprites for all! */
> > -			break;
> > +		set_sprite_wh(display, pipe, parms, sprite_fb,
> > sprite_width,
> > +			      sprite_height, alpha);
> > +		wm_setup_plane(display, pipe, iter_mask, parms, false);
> > +		ret = igt_display_try_commit_atomic(display,
> > +						    DRM_MODE_ATOMIC_TES
> > T_ONLY |
> > +						    DRM_MODE_ATOMIC_ALL
> > OW_MODESET,
> > +						    NULL);
> >  	}
> >  
> > -	igt_info("Running test on pipe %s with resolution %dx%d and
> > sprite size %dx%d alpha %i\n",
> > +	igt_info("Running test on pipe %s with resolution %dx%d, sprite
> > size %dx%d, alpha %i and %i overlay planes\n",
> >  		 kmstest_pipe_name(pipe), mode->hdisplay, mode-
> > > vdisplay,
> > -		 sprite_width, sprite_height, alpha);
> > +		 sprite_width, sprite_height, alpha, n_overlays);
> >  }
> >  
> >  static void prepare_fencing(igt_display_t *display, enum pipe
> > pipe)
> > @@ -519,8 +560,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 !=
> > GROUP_TYPE_NONE)
> > +				igt_plane_set_position(plane, 0, 0);
> > +		}
> >  
> >  		igt_display_commit2(display, COMMIT_ATOMIC);
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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