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 Tue, 2019-04-16 at 10:52 +0200, Daniel Vetter wrote:
> On Fri, Apr 05, 2019 at 07:20:15AM +0000, Lisovskiy, Stanislav wrote:
> > On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> > > On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy, Stanislav
> > > wrote:
> > > > 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.
> > > 
> > > Just doing some bug scrub and we have some EINVAL failures like
> > > this
> > > all
> > > over the place. So I guess there's some other cases where we
> > > reject
> > > configs in the kernel and then the test falls over. We'll need
> > > this
> > > (it's
> > > how atomic is meant to work really).
> > > 
> > > Also, the bw band-aid from Ville is a really bad solution, we
> > > need
> > > something better - no point in having 7 planes if you can't even
> > > use
> > > them.
> > > Adding more try_commit in the wrong place feels like a detour.
> > > -Daniel
> > 
> > That's a topic for much larger discussion, I have been hearing
> > around
> > for almost a month :) 
> > I have been talking with Ville and Martin a lot about this.
> > And yeah to me it is also controversial, that you have planes, but
> > they
> > are not always usable, however...
> > I completely understand all the pros and cons, you 
> > are describing here. But that really goes far beyond that IGT
> > patch.
> > I'm just dealing with reality what we have currently here and
> > trying to
> > make it not to fail miserably at least. Once we will have
> > "something
> > better" that you describe, I would be happy to implement a
> > correspondent changes. 
> 
> We _do_ have this "something better", since day 1 of atomic. It's
> called
> try_commit.
> 
> And yes "some planes sometimes don't work for reasons only clear to
> the
> kernel driver" has also been part of atomic since day one.

Well, thats is what most of those fixes are about - we just change test
case to query first, if that config is going to work, before actually
committing it. If it doesn't work we try to reduce planes number or
their size. So what do you exactly mean or don't like here? 

> 
> Please don't merge hacks to handle management pressure, let's do the
> right
> thing. I'm happy to handle managers for you who don't get this.
> Cutting
> corners here doesn't help anyone (and yes this should have been
> caught
> when the test was originally written, but well it wasnt).

Well, I can't literally "merge" anything. What I'm doing is changing 
patches to match new bw paradigm. Have you seen bw patches?
Due those changes we now have -EINVAL returned, whenever we don't have
resources(kms_concurrent, kms_atomic_transition, kms_plane_multiple..).
So as you have said we need to use try_commit to understand
first, if configuration is going to work or not. I'm doing that.

As I understand also BW is not something, we have any other solutions
for currently.

If you are not happy with it, I understand that - but as you
understand, I'm definitely not the first guy whom you should refer to
here.

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