Re: [igt-dev] [PATCH i-g-t v2] igt: tests: kms_vblank: Stop using magic value

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Apr 02, 2019 at 06:59:55PM +0300, Stanislav Lisovskiy wrote:
> Lot of tests fail, when assertion checking
> how many vblanks should have passed during
> suspend/resume is compared to magic value 150.
> 
> At the same time even for failed cases, sometimes
> it is clearly visible that there were no issue -
> simply suspend took longer on that machine.
> If suspend took around 10 s and we get roughly
> 60 vblanks per second the value then should be
> around 600 and not 150.
> 
> This change removes 150 magic value and starts
> to use calculation of what it is expected to
> be instead of being hardcoded.
> 
> v2: Add possible error delta interval, where expected
>     vblanks must lie, i.e:
>      [estimated_vblanks - err, estimated_vblanks + err]
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104894
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  tests/kms_vblank.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> index dafadb58..a5608d27 100644
> --- a/tests/kms_vblank.c
> +++ b/tests/kms_vblank.c
> @@ -332,14 +332,21 @@ static int get_vblank(int fd, enum pipe pipe, unsigned flags)
>  	return vbl.reply.sequence;
>  }
>  
> +#define VBLANK_ERR 2

Hmm. Not sure 2 isn't a bit too strict. Although I suppose our suspend
tests should be reasonably fast so not a lot of time to accumulate more
error.

> +
>  static void vblank_ts_cont(data_t *data, int fd, int nchildren)
>  {
>  	igt_display_t *display = &data->display;
>  	igt_output_t *output = data->output;
>  	int seq1, seq2;
>  	union drm_wait_vblank vbl;
> +	struct timespec start, end;
> +	int estimated_vblanks = 0;
> +	int vrefresh = igt_output_get_mode(output)->vrefresh;
> +	double time_elapsed;
>  
>  	seq1 = get_vblank(fd, data->pipe, 0);
> +	clock_gettime(CLOCK_MONOTONIC, &start);
>  
>  	if (data->flags & DPMS) {
>  		igt_output_set_prop_value(output, IGT_CONNECTOR_DPMS, DRM_MODE_DPMS_OFF);
> @@ -377,11 +384,19 @@ static void vblank_ts_cont(data_t *data, int fd, int nchildren)
>  	}
>  
>  	seq2 = get_vblank(fd, data->pipe, 0);
> +	clock_gettime(CLOCK_MONOTONIC, &end);
> +
> +	time_elapsed = igt_time_elapsed(&start, &end);
> +	estimated_vblanks = (int)(time_elapsed * vrefresh) + 1;
>  
>  	igt_debug("testing ts continuity: Current frame %u, old frame %u\n", seq2, seq1);
>  
> -	igt_assert_f(seq2 - seq1 >= 0, "unexpected vblank seq %u, should be >= %u\n", seq2, seq1);
> -	igt_assert_f(seq2 - seq1 <= 150, "unexpected vblank seq %u, should be < %u\n", seq2, seq1 + 150);
> +	igt_assert_f(seq2 - seq1 >= 0, "elapsed %f(%d vblanks) unexpected vblank seq %u, should be >= %u\n", time_elapsed,
> +			estimated_vblanks, seq2, seq1);

This first assert would appear to be redundant now.

> +	igt_assert_f(seq2 - seq1 <= (estimated_vblanks + VBLANK_ERR), "elapsed %f(%d vblanks) unexpected vblank seq %u, should be < %u\n", time_elapsed,
> +			estimated_vblanks, seq2, seq1 + estimated_vblanks);
> +	igt_assert_f(seq2 - seq1 >= (estimated_vblanks - VBLANK_ERR), "elapsed %f(%d vblanks) unexpected vblank seq %u, should be < %u\n", time_elapsed,

nit: pointless parens

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +			estimated_vblanks, seq2, seq1 + estimated_vblanks);
>  }
>  
>  static void run_subtests_for_pipe(data_t *data)
> -- 
> 2.17.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev