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

[Riana Tauro <riana.tauro@intel.com>]

Hi Raag

On 11/5/2024 9:39 PM, 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.
> 
>> 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);
Why not fail the test only if both status and reasons are set to true.
That would indicate there is throttling and also be a valid failure.


Thanks
Riana
> 
> This would be the same as deleting the test case...
> 
>>   }
>>   
>>   /**
>> -- 
>> 2.34.1
>>