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

[Kahola, Mika]

> -----Original Message-----
> From: Luca Coelho <luca at coelho.fi>
> Sent: Wednesday, June 15, 2022 1:50 PM
> To: Latvala, Petri <petri.latvala at intel.com>; Kahola, Mika
> <mika.kahola at intel.com>
> Cc: igt-dev at lists.freedesktop.org
> Subject: Re: [igt-dev] [PATCH i-g-t 2/4] tests/kms_sysfs_edid_timing: fail if
> probing a single connector takes too long
> 
> 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?

Well, I don't have any better knowledge what the "reasonable" time might be. I remember these to be more or less "trial and error" based guesses.

-Mika-

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