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

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

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