Re: [PATCH i-g-t v1] tests/intel/xe_gt_freq: Change basic throttle API test criteria

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Nov 05, 2024 at 11:09:41AM -0500, Rodrigo Vivi wrote:
> On Tue, Nov 05, 2024 at 12:02:28PM +0530, Raag Jadav wrote:
> > Basic throttle API test is implemented in a way that expects multiple
> > sysfs reads to provide atomic results. Since atomicity is never guaranteed
> > between multiple sysfs reads, this is an incorrect expection from the test.
> > Change the test criteria to check for boolean value from throttle status
> > and throttle reasons sysfs entries.
> > 
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3254
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3147
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2810
> 
> There's clear something wrong with the 'status'....
> 
> look at this tool: https://github.com/ulissesf/qmassa
> look at the recorded example in the readme... status is poping up so much.
> that is not normal. I noticed this here as well. status is flipping to 1
> so many times while we have absolutely no one of the real reasons flipping
> to 1 as well. This is the problem that needs to be solved.

There's nothing "suspicious" that I was able to find in the driver or
the documentation. Open to suggestions.

> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >  tests/intel/xe_gt_freq.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/intel/xe_gt_freq.c b/tests/intel/xe_gt_freq.c
> > index de4d111ea..632415d33 100644
> > --- a/tests/intel/xe_gt_freq.c
> > +++ b/tests/intel/xe_gt_freq.c
> > @@ -121,10 +121,8 @@ static void test_throttle_basic_api(int fd, int gt_id)
> >  	reasons |= get_throttle(fd, gt_id, "reason_vr_tdc");
> >  	reasons |= get_throttle(fd, gt_id, "reason_vr_thermalert");
> >  
> > -	if (status)
> > -		igt_assert(reasons);
> > -	else
> > -		igt_assert(!reasons);
> > +	igt_assert(status == true || status == false);
> > +	igt_assert(reasons == true || reasons == false);
> 
> This would be the same as deleting the test case...

Well, given the current scenario there's nothing much we can test anyway,
other than maybe the file exists and we're reading something from it.

Raag