[igt-dev] [PATCH i-g-t v5 0/2] kms_atomic_transition improvements

[igt-dev] [PATCH i-g-t v5 0/2] kms_atomic_transition improvements

[Stanislav Lisovskiy]

Fixed run_transition_test issue, which might happen when no changes
are done and wait_transition is called and made setup_parms function
tolerate if kernel can't afford having all sprite planes enabled
depending on current mode. Remove redundant condition nesting and code.

Stanislav Lisovskiy (2):
  igt/tests/kms_atomic_transition: Skip transition, if no changes done
  igt/tests/kms_atomic_transition: Tolerate if can't have all planes

 tests/kms_atomic_transition.c | 146 ++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 58 deletions(-)

-- 
2.17.1

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

[igt-dev] [PATCH i-g-t v5 1/2] igt/tests/kms_atomic_transition: Skip transition, if no changes done

[Stanislav Lisovskiy]

While fixing used amount of planes, discovered that if
wm_setup_plane is called with 0 planes(might happen during
main testing cycle, as parms[i].mask can be 0 due to randomization)
then subsequent wait_transition fails in assertion on fd_completed.
So added return value to wm_setup_plane, which would allow to
determine, if we need to skip this step.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 tests/kms_atomic_transition.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 18f73317..638fe17e 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -118,11 +118,12 @@ static void configure_fencing(igt_plane_t *plane)
 	igt_assert_eq(ret, 0);
 }
 
-static void
+static int
 wm_setup_plane(igt_display_t *display, enum pipe pipe,
 	       uint32_t mask, struct plane_parms *parms, bool fencing)
 {
 	igt_plane_t *plane;
+	int planes_set_up = 0;
 
 	/*
 	* Make sure these buffers are suited for display use
@@ -133,8 +134,10 @@ wm_setup_plane(igt_display_t *display, enum pipe pipe,
 		int i = plane->index;
 
 		if (!mask || !(parms[i].mask & mask)) {
-			if (plane->values[IGT_PLANE_FB_ID])
+			if (plane->values[IGT_PLANE_FB_ID]) {
 				igt_plane_set_fb(plane, NULL);
+				planes_set_up++;
+			}
 			continue;
 		}
 
@@ -144,7 +147,10 @@ wm_setup_plane(igt_display_t *display, enum pipe pipe,
 		igt_plane_set_fb(plane, parms[i].fb);
 		igt_fb_set_size(parms[i].fb, plane, parms[i].width, parms[i].height);
 		igt_plane_set_size(plane, parms[i].width, parms[i].height);
+
+		planes_set_up++;
 	}
+	return planes_set_up;
 }
 
 static void ev_page_flip(int fd, unsigned seq, unsigned tv_sec, unsigned tv_usec, void *user_data)
@@ -544,7 +550,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 		igt_output_set_pipe(output, pipe);
 
-		wm_setup_plane(display, pipe, i, parms, fencing);
+		if (!wm_setup_plane(display, pipe, i, parms, fencing))
+			continue;
 
 		atomic_commit(display, pipe, flags, (void *)(unsigned long)i, fencing);
 		wait_for_transition(display, pipe, nonblocking, fencing);
@@ -552,7 +559,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 		if (type == TRANSITION_MODESET_DISABLE) {
 			igt_output_set_pipe(output, PIPE_NONE);
 
-			wm_setup_plane(display, pipe, 0, parms, fencing);
+			if (!wm_setup_plane(display, pipe, 0, parms, fencing))
+				continue;
 
 			atomic_commit(display, pipe, flags, (void *) 0UL, fencing);
 			wait_for_transition(display, pipe, nonblocking, fencing);
@@ -568,7 +576,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 				    n_enable_planes < pipe_obj->n_planes)
 					continue;
 
-				wm_setup_plane(display, pipe, j, parms, fencing);
+				if (!wm_setup_plane(display, pipe, j, parms, fencing))
+					continue;
 
 				if (type >= TRANSITION_MODESET)
 					igt_output_override_mode(output, &override_mode);
@@ -576,7 +585,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 				atomic_commit(display, pipe, flags, (void *)(unsigned long) j, fencing);
 				wait_for_transition(display, pipe, nonblocking, fencing);
 
-				wm_setup_plane(display, pipe, i, parms, fencing);
+				if (!wm_setup_plane(display, pipe, i, parms, fencing))
+					continue;
+
 				if (type >= TRANSITION_MODESET)
 					igt_output_override_mode(output, NULL);
 
-- 
2.17.1

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

Re: [igt-dev] [PATCH i-g-t v5 1/2] igt/tests/kms_atomic_transition: Skip transition, if no changes done

[Souza, Jose]

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

On Thu, 2019-04-04 at 15:13 +0300, Stanislav Lisovskiy wrote:
> While fixing used amount of planes, discovered that if
> wm_setup_plane is called with 0 planes(might happen during
> main testing cycle, as parms[i].mask can be 0 due to randomization)

As already said and you agreed this 'as parms[i].mask can be 0 due to
randomization' statement is not true, even if this patch is right the
commit message needs to be true.

For me this patch is only hidden the real bug.

> then subsequent wait_transition fails in assertion on fd_completed.
> So added return value to wm_setup_plane, which would allow to
> determine, if we need to skip this step.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  tests/kms_atomic_transition.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/kms_atomic_transition.c
> b/tests/kms_atomic_transition.c
> index 18f73317..638fe17e 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -118,11 +118,12 @@ static void configure_fencing(igt_plane_t
> *plane)
>  	igt_assert_eq(ret, 0);
>  }
>  
> -static void
> +static int
>  wm_setup_plane(igt_display_t *display, enum pipe pipe,
>  	       uint32_t mask, struct plane_parms *parms, bool fencing)
>  {
>  	igt_plane_t *plane;
> +	int planes_set_up = 0;
>  
>  	/*
>  	* Make sure these buffers are suited for display use
> @@ -133,8 +134,10 @@ wm_setup_plane(igt_display_t *display, enum pipe
> pipe,
>  		int i = plane->index;
>  
>  		if (!mask || !(parms[i].mask & mask)) {
> -			if (plane->values[IGT_PLANE_FB_ID])
> +			if (plane->values[IGT_PLANE_FB_ID]) {
>  				igt_plane_set_fb(plane, NULL);
> +				planes_set_up++;
> +			}
>  			continue;
>  		}
>  
> @@ -144,7 +147,10 @@ wm_setup_plane(igt_display_t *display, enum pipe
> pipe,
>  		igt_plane_set_fb(plane, parms[i].fb);
>  		igt_fb_set_size(parms[i].fb, plane, parms[i].width,
> parms[i].height);
>  		igt_plane_set_size(plane, parms[i].width,
> parms[i].height);
> +
> +		planes_set_up++;
>  	}
> +	return planes_set_up;
>  }
>  
>  static void ev_page_flip(int fd, unsigned seq, unsigned tv_sec,
> unsigned tv_usec, void *user_data)
> @@ -544,7 +550,8 @@ run_transition_test(igt_display_t *display, enum
> pipe pipe, igt_output_t *output
>  
>  		igt_output_set_pipe(output, pipe);
>  
> -		wm_setup_plane(display, pipe, i, parms, fencing);
> +		if (!wm_setup_plane(display, pipe, i, parms, fencing))
> +			continue;
>  
>  		atomic_commit(display, pipe, flags, (void *)(unsigned
> long)i, fencing);
>  		wait_for_transition(display, pipe, nonblocking,
> fencing);
> @@ -552,7 +559,8 @@ run_transition_test(igt_display_t *display, enum
> pipe pipe, igt_output_t *output
>  		if (type == TRANSITION_MODESET_DISABLE) {
>  			igt_output_set_pipe(output, PIPE_NONE);
>  
> -			wm_setup_plane(display, pipe, 0, parms,
> fencing);
> +			if (!wm_setup_plane(display, pipe, 0, parms,
> fencing))
> +				continue;
>  
>  			atomic_commit(display, pipe, flags, (void *)
> 0UL, fencing);
>  			wait_for_transition(display, pipe, nonblocking,
> fencing);
> @@ -568,7 +576,8 @@ run_transition_test(igt_display_t *display, enum
> pipe pipe, igt_output_t *output
>  				    n_enable_planes < pipe_obj-
> >n_planes)
>  					continue;
>  
> -				wm_setup_plane(display, pipe, j, parms,
> fencing);
> +				if (!wm_setup_plane(display, pipe, j,
> parms, fencing))
> +					continue;
>  
>  				if (type >= TRANSITION_MODESET)
>  					igt_output_override_mode(output
> , &override_mode);
> @@ -576,7 +585,9 @@ run_transition_test(igt_display_t *display, enum
> pipe pipe, igt_output_t *output
>  				atomic_commit(display, pipe, flags,
> (void *)(unsigned long) j, fencing);
>  				wait_for_transition(display, pipe,
> nonblocking, fencing);
>  
> -				wm_setup_plane(display, pipe, i, parms,
> fencing);
> +				if (!wm_setup_plane(display, pipe, i,
> parms, fencing))
> +					continue;
> +
>  				if (type >= TRANSITION_MODESET)
>  					igt_output_override_mode(output
> , NULL);
>  

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

Re: [igt-dev] [PATCH i-g-t v5 1/2] igt/tests/kms_atomic_transition: Skip transition, if no changes done

[Lisovskiy, Stanislav]

On Thu, 2019-04-04 at 15:55 -0700, Souza, Jose wrote:
> On Thu, 2019-04-04 at 15:13 +0300, Stanislav Lisovskiy wrote:
> > While fixing used amount of planes, discovered that if
> > wm_setup_plane is called with 0 planes(might happen during
> > main testing cycle, as parms[i].mask can be 0 due to randomization)
> 
> As already said and you agreed this 'as parms[i].mask can be 0 due to
> randomization' statement is not true, even if this patch is right the
> commit message needs to be true.
> 

I will fix the commit message, you are right parms[i].mask can't be
true. But parms[i].mask & mask parameter can be easily. The function
user should be able to check if no changes are actually done. Otherwise
run_transition will timeout on fd_completed call.

> For me this patch is only hidden the real bug.

Sorry, what do you mean by "real bug"? 

> 
> > then subsequent wait_transition fails in assertion on fd_completed.
> > So added return value to wm_setup_plane, which would allow to
> > determine, if we need to skip this step.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  tests/kms_atomic_transition.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 18f73317..638fe17e 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -118,11 +118,12 @@ static void configure_fencing(igt_plane_t
> > *plane)
> >  	igt_assert_eq(ret, 0);
> >  }
> >  
> > -static void
> > +static int
> >  wm_setup_plane(igt_display_t *display, enum pipe pipe,
> >  	       uint32_t mask, struct plane_parms *parms, bool fencing)
> >  {
> >  	igt_plane_t *plane;
> > +	int planes_set_up = 0;
> >  
> >  	/*
> >  	* Make sure these buffers are suited for display use
> > @@ -133,8 +134,10 @@ wm_setup_plane(igt_display_t *display, enum
> > pipe
> > pipe,
> >  		int i = plane->index;
> >  
> >  		if (!mask || !(parms[i].mask & mask)) {
> > -			if (plane->values[IGT_PLANE_FB_ID])
> > +			if (plane->values[IGT_PLANE_FB_ID]) {
> >  				igt_plane_set_fb(plane, NULL);
> > +				planes_set_up++;
> > +			}
> >  			continue;
> >  		}
> >  
> > @@ -144,7 +147,10 @@ wm_setup_plane(igt_display_t *display, enum
> > pipe
> > pipe,
> >  		igt_plane_set_fb(plane, parms[i].fb);
> >  		igt_fb_set_size(parms[i].fb, plane, parms[i].width,
> > parms[i].height);
> >  		igt_plane_set_size(plane, parms[i].width,
> > parms[i].height);
> > +
> > +		planes_set_up++;
> >  	}
> > +	return planes_set_up;
> >  }
> >  
> >  static void ev_page_flip(int fd, unsigned seq, unsigned tv_sec,
> > unsigned tv_usec, void *user_data)
> > @@ -544,7 +550,8 @@ run_transition_test(igt_display_t *display,
> > enum
> > pipe pipe, igt_output_t *output
> >  
> >  		igt_output_set_pipe(output, pipe);
> >  
> > -		wm_setup_plane(display, pipe, i, parms, fencing);
> > +		if (!wm_setup_plane(display, pipe, i, parms, fencing))
> > +			continue;
> >  
> >  		atomic_commit(display, pipe, flags, (void *)(unsigned
> > long)i, fencing);
> >  		wait_for_transition(display, pipe, nonblocking,
> > fencing);
> > @@ -552,7 +559,8 @@ run_transition_test(igt_display_t *display,
> > enum
> > pipe pipe, igt_output_t *output
> >  		if (type == TRANSITION_MODESET_DISABLE) {
> >  			igt_output_set_pipe(output, PIPE_NONE);
> >  
> > -			wm_setup_plane(display, pipe, 0, parms,
> > fencing);
> > +			if (!wm_setup_plane(display, pipe, 0, parms,
> > fencing))
> > +				continue;
> >  
> >  			atomic_commit(display, pipe, flags, (void *)
> > 0UL, fencing);
> >  			wait_for_transition(display, pipe, nonblocking,
> > fencing);
> > @@ -568,7 +576,8 @@ run_transition_test(igt_display_t *display,
> > enum
> > pipe pipe, igt_output_t *output
> >  				    n_enable_planes < pipe_obj-
> > > n_planes)
> > 
> >  					continue;
> >  
> > -				wm_setup_plane(display, pipe, j, parms,
> > fencing);
> > +				if (!wm_setup_plane(display, pipe, j,
> > parms, fencing))
> > +					continue;
> >  
> >  				if (type >= TRANSITION_MODESET)
> >  					igt_output_override_mode(output
> > , &override_mode);
> > @@ -576,7 +585,9 @@ run_transition_test(igt_display_t *display,
> > enum
> > pipe pipe, igt_output_t *output
> >  				atomic_commit(display, pipe, flags,
> > (void *)(unsigned long) j, fencing);
> >  				wait_for_transition(display, pipe,
> > nonblocking, fencing);
> >  
> > -				wm_setup_plane(display, pipe, i, parms,
> > fencing);
> > +				if (!wm_setup_plane(display, pipe, i,
> > parms, fencing))
> > +					continue;
> > +
> >  				if (type >= TRANSITION_MODESET)
> >  					igt_output_override_mode(output
> > , NULL);
> >  
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v5 1/2] igt/tests/kms_atomic_transition: Skip transition, if no changes done

[Souza, Jose]

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

On Fri, 2019-04-05 at 07:50 +0100, Lisovskiy, Stanislav wrote:
> On Thu, 2019-04-04 at 15:55 -0700, Souza, Jose wrote:
> > On Thu, 2019-04-04 at 15:13 +0300, Stanislav Lisovskiy wrote:
> > > While fixing used amount of planes, discovered that if
> > > wm_setup_plane is called with 0 planes(might happen during
> > > main testing cycle, as parms[i].mask can be 0 due to
> > > randomization)
> > 
> > As already said and you agreed this 'as parms[i].mask can be 0 due
> > to
> > randomization' statement is not true, even if this patch is right
> > the
> > commit message needs to be true.
> > 
> 
> I will fix the commit message, you are right parms[i].mask can't be
> true. But parms[i].mask & mask parameter can be easily. The function
> user should be able to check if no changes are actually done.
> Otherwise
> run_transition will timeout on fd_completed call.
> 
> > For me this patch is only hidden the real bug.
> 
> Sorry, what do you mean by "real bug"? 
> 

Take a look here: https://patchwork.freedesktop.org/series/59086/

It don't need anything like this patch and fixed the ICL BW issue,
something is wrong in the next patch and this patch was only hiding it.


> > > then subsequent wait_transition fails in assertion on
> > > fd_completed.
> > > So added return value to wm_setup_plane, which would allow to
> > > determine, if we need to skip this step.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com
> > > >
> > > ---
> > >  tests/kms_atomic_transition.c | 23 +++++++++++++++++------
> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tests/kms_atomic_transition.c
> > > b/tests/kms_atomic_transition.c
> > > index 18f73317..638fe17e 100644
> > > --- a/tests/kms_atomic_transition.c
> > > +++ b/tests/kms_atomic_transition.c
> > > @@ -118,11 +118,12 @@ static void configure_fencing(igt_plane_t
> > > *plane)
> > >  	igt_assert_eq(ret, 0);
> > >  }
> > >  
> > > -static void
> > > +static int
> > >  wm_setup_plane(igt_display_t *display, enum pipe pipe,
> > >  	       uint32_t mask, struct plane_parms *parms, bool fencing)
> > >  {
> > >  	igt_plane_t *plane;
> > > +	int planes_set_up = 0;
> > >  
> > >  	/*
> > >  	* Make sure these buffers are suited for display use
> > > @@ -133,8 +134,10 @@ wm_setup_plane(igt_display_t *display, enum
> > > pipe
> > > pipe,
> > >  		int i = plane->index;
> > >  
> > >  		if (!mask || !(parms[i].mask & mask)) {
> > > -			if (plane->values[IGT_PLANE_FB_ID])
> > > +			if (plane->values[IGT_PLANE_FB_ID]) {
> > >  				igt_plane_set_fb(plane, NULL);
> > > +				planes_set_up++;
> > > +			}
> > >  			continue;
> > >  		}
> > >  
> > > @@ -144,7 +147,10 @@ wm_setup_plane(igt_display_t *display, enum
> > > pipe
> > > pipe,
> > >  		igt_plane_set_fb(plane, parms[i].fb);
> > >  		igt_fb_set_size(parms[i].fb, plane, parms[i].width,
> > > parms[i].height);
> > >  		igt_plane_set_size(plane, parms[i].width,
> > > parms[i].height);
> > > +
> > > +		planes_set_up++;
> > >  	}
> > > +	return planes_set_up;
> > >  }
> > >  
> > >  static void ev_page_flip(int fd, unsigned seq, unsigned tv_sec,
> > > unsigned tv_usec, void *user_data)
> > > @@ -544,7 +550,8 @@ run_transition_test(igt_display_t *display,
> > > enum
> > > pipe pipe, igt_output_t *output
> > >  
> > >  		igt_output_set_pipe(output, pipe);
> > >  
> > > -		wm_setup_plane(display, pipe, i, parms, fencing);
> > > +		if (!wm_setup_plane(display, pipe, i, parms, fencing))
> > > +			continue;
> > >  
> > >  		atomic_commit(display, pipe, flags, (void *)(unsigned
> > > long)i, fencing);
> > >  		wait_for_transition(display, pipe, nonblocking,
> > > fencing);
> > > @@ -552,7 +559,8 @@ run_transition_test(igt_display_t *display,
> > > enum
> > > pipe pipe, igt_output_t *output
> > >  		if (type == TRANSITION_MODESET_DISABLE) {
> > >  			igt_output_set_pipe(output, PIPE_NONE);
> > >  
> > > -			wm_setup_plane(display, pipe, 0, parms,
> > > fencing);
> > > +			if (!wm_setup_plane(display, pipe, 0, parms,
> > > fencing))
> > > +				continue;
> > >  
> > >  			atomic_commit(display, pipe, flags, (void *)
> > > 0UL, fencing);
> > >  			wait_for_transition(display, pipe, nonblocking,
> > > fencing);
> > > @@ -568,7 +576,8 @@ run_transition_test(igt_display_t *display,
> > > enum
> > > pipe pipe, igt_output_t *output
> > >  				    n_enable_planes < pipe_obj-
> > > > n_planes)
> > > 
> > >  					continue;
> > >  
> > > -				wm_setup_plane(display, pipe, j, parms,
> > > fencing);
> > > +				if (!wm_setup_plane(display, pipe, j,
> > > parms, fencing))
> > > +					continue;
> > >  
> > >  				if (type >= TRANSITION_MODESET)
> > >  					igt_output_override_mode(output
> > > , &override_mode);
> > > @@ -576,7 +585,9 @@ run_transition_test(igt_display_t *display,
> > > enum
> > > pipe pipe, igt_output_t *output
> > >  				atomic_commit(display, pipe, flags,
> > > (void *)(unsigned long) j, fencing);
> > >  				wait_for_transition(display, pipe,
> > > nonblocking, fencing);
> > >  
> > > -				wm_setup_plane(display, pipe, i, parms,
> > > fencing);
> > > +				if (!wm_setup_plane(display, pipe, i,
> > > parms, fencing))
> > > +					continue;
> > > +
> > >  				if (type >= TRANSITION_MODESET)
> > >  					igt_output_override_mode(output
> > > , NULL);
> > >  

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

Re: [igt-dev] [PATCH i-g-t v5 1/2] igt/tests/kms_atomic_transition: Skip transition, if no changes done

[Lisovskiy, Stanislav]

On Fri, 2019-04-05 at 16:45 -0700, Souza, Jose wrote:
> On Fri, 2019-04-05 at 07:50 +0100, Lisovskiy, Stanislav wrote:
> > On Thu, 2019-04-04 at 15:55 -0700, Souza, Jose wrote:
> > > On Thu, 2019-04-04 at 15:13 +0300, Stanislav Lisovskiy wrote:
> > > > While fixing used amount of planes, discovered that if
> > > > wm_setup_plane is called with 0 planes(might happen during
> > > > main testing cycle, as parms[i].mask can be 0 due to
> > > > randomization)
> > > 
> > > As already said and you agreed this 'as parms[i].mask can be 0
> > > due
> > > to
> > > randomization' statement is not true, even if this patch is right
> > > the
> > > commit message needs to be true.
> > > 
> > 
> > I will fix the commit message, you are right parms[i].mask can't be
> > true. But parms[i].mask & mask parameter can be easily. The
> > function
> > user should be able to check if no changes are actually done.
> > Otherwise
> > run_transition will timeout on fd_completed call.
> > 
> > > For me this patch is only hidden the real bug.
> > 
> > Sorry, what do you mean by "real bug"? 
> > 
> 
> Take a look here: https://patchwork.freedesktop.org/series/59086/
> 
> It don't need anything like this patch and fixed the ICL BW issue,
> something is wrong in the next patch and this patch was only hiding
> it.

Ok, I looked and this patch does almost exactly the same thing and has
even more lines. If you think that my patch has some issue, it would be
nice to at least point out what exactly, considering that I've
explained what was the problem and gave appropriate reasoning regarding
wm_setup_plane. Otherwise you or somebody else might just repeat the
same mistake.
I really see no point in this kind of review, when you say that 
"this is bad" and then without any explanation send almost exactly 
similar solution.
What is wrong with wm_setup_plane having return value? Does it break
anything?
You doubt, this might bring problems? Just call it with 0 mask and
then atomic_commit, wait_transition and you will see. 

Instead you just say "this is wrong", sending patch with more lines,
more conditions and which needs to reviewed completely from scratch
again. Great.

> 
> 
> > > > then subsequent wait_transition fails in assertion on
> > > > fd_completed.
> > > > So added return value to wm_setup_plane, which would allow to
> > > > determine, if we need to skip this step.
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <
> > > > stanislav.lisovskiy@intel.com
> > > > > 
> > > > 
> > > > ---
> > > >  tests/kms_atomic_transition.c | 23 +++++++++++++++++------
> > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_atomic_transition.c
> > > > b/tests/kms_atomic_transition.c
> > > > index 18f73317..638fe17e 100644
> > > > --- a/tests/kms_atomic_transition.c
> > > > +++ b/tests/kms_atomic_transition.c
> > > > @@ -118,11 +118,12 @@ static void configure_fencing(igt_plane_t
> > > > *plane)
> > > >  	igt_assert_eq(ret, 0);
> > > >  }
> > > >  
> > > > -static void
> > > > +static int
> > > >  wm_setup_plane(igt_display_t *display, enum pipe pipe,
> > > >  	       uint32_t mask, struct plane_parms *parms, bool
> > > > fencing)
> > > >  {
> > > >  	igt_plane_t *plane;
> > > > +	int planes_set_up = 0;
> > > >  
> > > >  	/*
> > > >  	* Make sure these buffers are suited for display use
> > > > @@ -133,8 +134,10 @@ wm_setup_plane(igt_display_t *display,
> > > > enum
> > > > pipe
> > > > pipe,
> > > >  		int i = plane->index;
> > > >  
> > > >  		if (!mask || !(parms[i].mask & mask)) {
> > > > -			if (plane->values[IGT_PLANE_FB_ID])
> > > > +			if (plane->values[IGT_PLANE_FB_ID]) {
> > > >  				igt_plane_set_fb(plane, NULL);
> > > > +				planes_set_up++;
> > > > +			}
> > > >  			continue;
> > > >  		}
> > > >  
> > > > @@ -144,7 +147,10 @@ wm_setup_plane(igt_display_t *display,
> > > > enum
> > > > pipe
> > > > pipe,
> > > >  		igt_plane_set_fb(plane, parms[i].fb);
> > > >  		igt_fb_set_size(parms[i].fb, plane,
> > > > parms[i].width,
> > > > parms[i].height);
> > > >  		igt_plane_set_size(plane, parms[i].width,
> > > > parms[i].height);
> > > > +
> > > > +		planes_set_up++;
> > > >  	}
> > > > +	return planes_set_up;
> > > >  }
> > > >  
> > > >  static void ev_page_flip(int fd, unsigned seq, unsigned
> > > > tv_sec,
> > > > unsigned tv_usec, void *user_data)
> > > > @@ -544,7 +550,8 @@ run_transition_test(igt_display_t *display,
> > > > enum
> > > > pipe pipe, igt_output_t *output
> > > >  
> > > >  		igt_output_set_pipe(output, pipe);
> > > >  
> > > > -		wm_setup_plane(display, pipe, i, parms,
> > > > fencing);
> > > > +		if (!wm_setup_plane(display, pipe, i, parms,
> > > > fencing))
> > > > +			continue;
> > > >  
> > > >  		atomic_commit(display, pipe, flags, (void
> > > > *)(unsigned
> > > > long)i, fencing);
> > > >  		wait_for_transition(display, pipe, nonblocking,
> > > > fencing);
> > > > @@ -552,7 +559,8 @@ run_transition_test(igt_display_t *display,
> > > > enum
> > > > pipe pipe, igt_output_t *output
> > > >  		if (type == TRANSITION_MODESET_DISABLE) {
> > > >  			igt_output_set_pipe(output, PIPE_NONE);
> > > >  
> > > > -			wm_setup_plane(display, pipe, 0, parms,
> > > > fencing);
> > > > +			if (!wm_setup_plane(display, pipe, 0,
> > > > parms,
> > > > fencing))
> > > > +				continue;
> > > >  
> > > >  			atomic_commit(display, pipe, flags,
> > > > (void *)
> > > > 0UL, fencing);
> > > >  			wait_for_transition(display, pipe,
> > > > nonblocking,
> > > > fencing);
> > > > @@ -568,7 +576,8 @@ run_transition_test(igt_display_t *display,
> > > > enum
> > > > pipe pipe, igt_output_t *output
> > > >  				    n_enable_planes < pipe_obj-
> > > > > n_planes)
> > > > 
> > > >  					continue;
> > > >  
> > > > -				wm_setup_plane(display, pipe,
> > > > j, parms,
> > > > fencing);
> > > > +				if (!wm_setup_plane(display,
> > > > pipe, j,
> > > > parms, fencing))
> > > > +					continue;
> > > >  
> > > >  				if (type >= TRANSITION_MODESET)
> > > >  					igt_output_override_mod
> > > > e(output
> > > > , &override_mode);
> > > > @@ -576,7 +585,9 @@ run_transition_test(igt_display_t *display,
> > > > enum
> > > > pipe pipe, igt_output_t *output
> > > >  				atomic_commit(display, pipe,
> > > > flags,
> > > > (void *)(unsigned long) j, fencing);
> > > >  				wait_for_transition(display,
> > > > pipe,
> > > > nonblocking, fencing);
> > > >  
> > > > -				wm_setup_plane(display, pipe,
> > > > i, parms,
> > > > fencing);
> > > > +				if (!wm_setup_plane(display,
> > > > pipe, i,
> > > > parms, fencing))
> > > > +					continue;
> > > > +
> > > >  				if (type >= TRANSITION_MODESET)
> > > >  					igt_output_override_mod
> > > > e(output
> > > > , NULL);
> > > >  
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

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

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..e6d55ca0 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_warn("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_warn("Removed alpha\n");
+			}
+			else {
+				igt_assert_f(n_planes > 1, "No planes left to proceed with!");
+				n_planes--;
+				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 +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);
 
-- 
2.17.1

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

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

[Souza, Jose]

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

On Thu, 2019-04-04 at 15:13 +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.
> 
> v5: Randomize which of sprite planes to remove if hitting
>     resource limits.
> 
> 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..e6d55ca0 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)) {

This will not guarantee that x(planes_to_remove) planes will be
removed.

> +				parms[i].fb = NULL;
> +				parms[i].mask = 0;
> +				planes_to_remove--;
> +				igt_warn("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_warn("Removed alpha\n");
> +			}
> +			else {
> +				igt_assert_f(n_planes > 1, "No planes
> left to proceed with!");
> +				n_planes--;
> +				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 +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);
>  

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

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

[Lisovskiy, Stanislav]

On Thu, 2019-04-04 at 17:59 -0700, Souza, Jose wrote:
> On Thu, 2019-04-04 at 15:13 +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.
> > 
> > v5: Randomize which of sprite planes to remove if hitting
> >     resource limits.
> > 
> > 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..e6d55ca0 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)) {
> 
> This will not guarantee that x(planes_to_remove) planes will be
> removed.

There are two cases:

1) If planes to remove is less than planes left, it means that we
   still have some freedom for randomization which planes has to be   
removed. Lets say we need to remove 3, but we have for 5, then we can
   randomize which 3 out of 5 we can remove. So in that case we use
   remove variable as a random factor to decide whether plane has to be
   removed or not.
   
2) If planes to remove is equal(or more) than planes left, it means
   that we need to remove anyway. Lets say we need to remove 3 and we
   have 3. Obviously we need to remove all of those.

If you still see that there is a flaw in this algorithm, can you please
explain?

> 
> > +				parms[i].fb = NULL;
> > +				parms[i].mask = 0;
> > +				planes_to_remove--;
> > +				igt_warn("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_warn("Removed alpha\n");
> > +			}
> > +			else {
> > +				igt_assert_f(n_planes > 1, "No planes
> > left to proceed with!");
> > +				n_planes--;
> > +				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 +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

[igt-dev] ✓ Fi.CI.BAT: success for kms_atomic_transition improvements (rev9)

[Patchwork]

== Series Details ==

Series: kms_atomic_transition improvements (rev9)
URL   : https://patchwork.freedesktop.org/series/58728/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5870 -> IGTPW_2787
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58728/revisions/9/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2787 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-icl-u2:          PASS -> DMESG-WARN [fdo#109638]
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries:
    - fi-ilk-650:         DMESG-WARN [fdo#106387] -> PASS
    - {fi-kbl-7567u}:     DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS

  * igt@gem_exec_suspend@basic-s3:
    - {fi-kbl-7567u}:     DMESG-WARN [fdo#103558] / [fdo#105079] / [fdo#105602] -> PASS

  * igt@i915_module_load@reload-with-fault-injection:
    - {fi-kbl-7567u}:     DMESG-WARN [fdo#105602] / [fdo#108529] -> PASS +1

  * igt@i915_pm_rpm@module-reload:
    - {fi-kbl-7567u}:     DMESG-WARN [fdo#108529] -> PASS

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         INCOMPLETE [fdo#103927] / [fdo#109720] -> PASS

  * igt@kms_busy@basic-flip-a:
    - {fi-kbl-7567u}:     SKIP [fdo#109271] / [fdo#109278] -> PASS +2

  * igt@kms_chamelium@hdmi-edid-read:
    - {fi-kbl-7567u}:     FAIL [fdo#109569] -> PASS +1

  * igt@kms_chamelium@hdmi-hpd-fast:
    - {fi-kbl-7567u}:     FAIL [fdo#109800] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#107362] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - {fi-kbl-7567u}:     DMESG-FAIL [fdo#105079] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - {fi-kbl-7567u}:     SKIP [fdo#109271] -> PASS +4

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109569]: https://bugs.freedesktop.org/show_bug.cgi?id=109569
  [fdo#109638]: https://bugs.freedesktop.org/show_bug.cgi?id=109638
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [fdo#109800]: https://bugs.freedesktop.org/show_bug.cgi?id=109800


Participating hosts (50 -> 42)
------------------------------

  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-hsw-4770 fi-pnv-d510 fi-icl-y 


Build changes
-------------

    * IGT: IGT_4928 -> IGTPW_2787

  CI_DRM_5870: 122dc43df57c9a03132314ddc78a6d60c714fa32 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2787: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2787/
  IGT_4928: 014a6fa238322b497116b359cb92df1ce7fa8847 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2787/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.IGT: failure for kms_atomic_transition improvements (rev9)

[Patchwork]

== Series Details ==

Series: kms_atomic_transition improvements (rev9)
URL   : https://patchwork.freedesktop.org/series/58728/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5870_full -> IGTPW_2787_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2787_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2787_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58728/revisions/9/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2787_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_atomic_transition@plane-all-transition:
    - shard-snb:          PASS -> WARN +8

  * igt@kms_atomic_transition@plane-use-after-nonblocking-unbind:
    - shard-hsw:          PASS -> WARN +8

  
#### Warnings ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-hsw:          FAIL [fdo#108686] -> TIMEOUT

  
Known issues
------------

  Here are the changes found in IGTPW_2787_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_atomic_transition@4x-modeset-transitions:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-hsw:          PASS -> DMESG-WARN [fdo#110222]
    - shard-kbl:          PASS -> DMESG-WARN [fdo#110222]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
    - shard-snb:          PASS -> DMESG-WARN [fdo#110222] +1

  * igt@kms_content_protection@atomic:
    - shard-apl:          NOTRUN -> FAIL [fdo#108739] / [fdo#110321]

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-apl:          PASS -> FAIL [fdo#103232]
    - shard-kbl:          PASS -> FAIL [fdo#103232]

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
    - shard-hsw:          PASS -> FAIL [fdo#105767]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-indfb-fliptrack:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +7

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-cur-indfb-draw-blt:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271]

  * igt@kms_plane_scaling@pipe-b-scaler-with-rotation:
    - shard-glk:          PASS -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_rotation_crc@multiplane-rotation:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_vblank@pipe-a-ts-continuation-modeset-rpm:
    - shard-apl:          PASS -> FAIL [fdo#104894] +1

  * igt@kms_vblank@pipe-b-wait-busy-hang:
    - shard-hsw:          PASS -> INCOMPLETE [fdo#103540]

  
#### Possible fixes ####

  * igt@kms_cursor_edge_walk@pipe-a-64x64-left-edge:
    - shard-snb:          SKIP [fdo#109271] / [fdo#109278] -> PASS

  * igt@kms_flip@modeset-vs-vblank-race:
    - shard-kbl:          FAIL [fdo#103060] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-rgb101010-draw-blt:
    - shard-snb:          SKIP [fdo#109271] -> PASS +3

  * igt@kms_plane_scaling@pipe-c-scaler-with-pixel-format:
    - shard-glk:          SKIP [fdo#109271] / [fdo#109278] -> PASS

  * igt@kms_vblank@pipe-c-ts-continuation-modeset:
    - shard-kbl:          FAIL [fdo#104894] -> PASS
    - shard-apl:          FAIL [fdo#104894] -> PASS

  
#### Warnings ####

  * igt@kms_lease@atomic_implicit_crtc:
    - shard-snb:          SKIP [fdo#109271] -> FAIL [fdo#110279]

  
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#108739]: https://bugs.freedesktop.org/show_bug.cgi?id=108739
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
  [fdo#110279]: https://bugs.freedesktop.org/show_bug.cgi?id=110279
  [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (9 -> 5)
------------------------------

  Missing    (4): pig-skl-6260u shard-skl pig-glk-j5005 shard-iclb 


Build changes
-------------

    * IGT: IGT_4928 -> IGTPW_2787
    * Piglit: piglit_4509 -> None

  CI_DRM_5870: 122dc43df57c9a03132314ddc78a6d60c714fa32 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2787: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2787/
  IGT_4928: 014a6fa238322b497116b359cb92df1ce7fa8847 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2787/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v5 0/2] kms_atomic_transition improvements

[Stanislav Lisovskiy]

Fixed run_transition_test issue, which might happen when no changes
are done and wait_transition is called and made setup_parms function
tolerate if kernel can't afford having all sprite planes enabled
depending on current mode. Remove redundant condition nesting and code.

Stanislav Lisovskiy (2):
  igt/tests/kms_atomic_transition: Skip transition, if no changes done
  igt/tests/kms_atomic_transition: Tolerate if can't have all planes

 tests/kms_atomic_transition.c | 146 ++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 58 deletions(-)

-- 
2.17.1

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