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

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

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