[igt-dev] [PATCH i-g-t 2/4] tests/kms_sysfs_edid_timing: fail if probing a single connector takes too long

[Luca Coelho]

On Thu, 2022-06-09 at 12:55 +0300, Petri Latvala wrote:
> On Thu, Jun 09, 2022 at 11:49:47AM +0300, Luca Coelho wrote:
> > On Thu, 2022-06-09 at 10:32 +0300, Petri Latvala wrote:
> > > On Tue, Jun 07, 2022 at 10:42:38AM +0300, Luca Coelho wrote:
> > > > From: Luca Coelho <luciano.coelho at intel.com>
> > > > 
> > > > There's no point in just warning if probing a single connector takes
> > > > too long.  If a test finds any issues, it should just fail.
> > > > 
> > > > Convert the warning into an assertion.
> > > > 
> > > > Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
> > > > ---
> > > >  tests/kms_sysfs_edid_timing.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_sysfs_edid_timing.c b/tests/kms_sysfs_edid_timing.c
> > > > index 028a5323c0b2..ea0e169e6ed9 100644
> > > > --- a/tests/kms_sysfs_edid_timing.c
> > > > +++ b/tests/kms_sysfs_edid_timing.c
> > > > @@ -81,11 +81,11 @@ igt_simple_main
> > > >  			  mean.max, mean.max / 1e3, mean.max / 1e6,
> > > >  			  mean.mean, mean.mean / 1e3, mean.mean / 1e6);
> > > >  
> > > > -		if (mean.max > (THRESHOLD_PER_CONNECTOR * 1e6)) {
> > > > -			igt_warn("%s: probe time exceed %dms, max=%.2fms, avg=%.2fms\n",
> > > > -				 de->d_name, THRESHOLD_PER_CONNECTOR,
> > > > -				 mean.max / 1e6, mean.mean / 1e6);
> > > > -		}
> > > > +		igt_assert_f(mean.max < THRESHOLD_PER_CONNECTOR * 1e6,
> > > > +			     "%s: probe time exceed %dms, max=%.2fms, avg=%.2fms\n",
> > > > +			     de->d_name, THRESHOLD_PER_CONNECTOR,
> > > > +			     mean.max / 1e6, mean.mean / 1e6);
> > > > +
> > > 
> > > 
> > > 120ms on KBL:
> > > https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7242/shard-kbl4/igt@kms_sysfs_edid_timing.html
> > > 
> > > 
> > > 80-ish ms on APL:
> > > https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7242/shard-apl4/igt@kms_sysfs_edid_timing.html
> > 
> > Thanks for checking these values IRL.
> > 
> > 
> > > As for whether that makes this IGT patch an acceptable change, depends
> > > on whether those times are considered anomalies that someone plans to
> > > fix somehow...
> > 
> > I don't really know what is considered a reasonable time for this.  We
> > used to have 10 ms which was not enough for almost any platform,
> > AFAICT.  So the tests were already failing (with warnings).
> > 
> > I think it doesn't make sense to differentiate between warning and
> > assertion failures if both cause the tests to fail.  It is just
> > confusing.  Is a warning a failure or not? Can it be ignored? IMHO a
> > test results should be black and white: fails or doesn't fail.
> > 
> > In any case, I agree that we should find reasonable values that will
> > work with all platforms.  120 ms sounds really high to me, though.  But
> > I don't really know if this can be fixed.
> 
> 
> Fair assessment. Mika, do you remember the history of this test enough
> to judge? Or have any insight on selecting a "reasonable" time.

Mika, any input?

In any case, let's set the threshold to 130 ms for now, so we can at
least be sure it will pass in all the platforms already tested?

It sounds like a huge amount of time to me, but I guess there can be a
lot of external influence as well, such as the time the display takes
to respond or other external issues?

For now, I'll just update my patches to use 130 ms and other higher
thresholds to match what's happening in real life.

--
Cheers,
Luca.