[igt-dev] [PATCH i-g-t v2] tests/kms_setmode: degrade assert to debug

[Petri Latvala]

On Tue, Sep 13, 2022 at 11:08:55AM -0400, Hamza Mahfooz wrote:
> Unfortunately, there are too many sources of jitter that can cause the
> vblank interval approximation obtained from DRM_IOCTL_WAIT_VBLANK to be
> off by more than a scanline for this assert to be useful. So, to allow
> this test to pass consistently degrade the assert to a debug.
> 
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz at amd.com>
> ---
> v2: fix logic
> ---
>  tests/kms_setmode.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c
> index bfa10891..58abfe14 100644
> --- a/tests/kms_setmode.c
> +++ b/tests/kms_setmode.c
> @@ -527,12 +527,12 @@ static void check_timings(int crtc_idx, const drmModeModeInfo *kmode)
>  	 * See:
>  	 * https://en.wikipedia.org/wiki/Standard_deviation#Rules_for_normally_distributed_data
>  	 */
> -	igt_assert_f(fabs(mean - expected) < max(line_time(kmode), 1.718 * stddev),
> -		     "vblank interval differs from modeline! expected %.1fus, measured %1.fus +- %.3fus, difference %.1fus (%.1f sigma, %.1f scanlines)\n",
> -		     expected, mean, stddev,
> -		     fabs(mean - expected),
> -		     fabs(mean - expected) / stddev,
> -		     fabs(mean - expected) / line_time(kmode));
> +	if (fabs(mean - expected) > max(line_time(kmode), 1.718 * stddev))
> +		igt_info("vblank interval differs from modeline! expected %.1fus, measured %1.fus +- %.3fus, difference %.1fus (%.1f sigma, %.1f scanlines)\n",
> +			 expected, mean, stddev,
> +			 fabs(mean - expected),
> +			 fabs(mean - expected) / stddev,
> +			 fabs(mean - expected) / line_time(kmode));
>  }


History has taught that having such messages in the logs are generally
useless. Without a mechanism to tell that the output has changed, it's
going to get missed and when you realize something is completely off,
figuring out when it changed is a ghost hunt.

In short, the action "to allow this test to pass" is effectively
deleting the test.

Having said that, deleting the test might be the right move. I said
people will miss changes if it's just a message, but it has been an
assertion failure for i915 CI for some time... But a consistent
one. The difference on SNB looks to be 2.5 scanlines, sometimes 2.1,
but most often 2.5.

Maybe Ville is able to comment? What's up with DRM_IOCTL_WAIT_VBLANK
timings, is this assert obsolete with today's kernel? Or does the test
need some kind of tuning?

-- 
Petri Latvala