[Intel-gfx] [igt-dev] [PATCH i-g-t] tests/sysfs: Update timeslice/preemption for new range limits

[John Harrison]

On 11/1/2022 08:27, Dixit, Ashutosh wrote:
> On Mon, 31 Oct 2022 15:24:40 -0700, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> Guc submission imposes new range limits on certain scheduling
>> parameters. The idempotent sections of the timeslice duration and
>> pre-emption timeout tests was exceeding those limits and so would fail.
>>
>> Reduce the excessively large value (654s) to one which does not
>> overflow (54s). Also add an assert that the write of the new value
>> actually succeeds.
>>
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> ---
>>   tests/i915/sysfs_preempt_timeout.c    | 4 ++--
>>   tests/i915/sysfs_timeslice_duration.c | 4 ++--
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/i915/sysfs_preempt_timeout.c b/tests/i915/sysfs_preempt_timeout.c
>> index 515038281638..5e0a7d96299f 100644
>> --- a/tests/i915/sysfs_preempt_timeout.c
>> +++ b/tests/i915/sysfs_preempt_timeout.c
>> @@ -68,7 +68,7 @@ static void set_preempt_timeout(int engine, unsigned int value)
>>   {
>> 	unsigned int delay;
>>
>> -	igt_sysfs_printf(engine, ATTR, "%u", value);
>> +	igt_assert_lte(0, igt_sysfs_printf(engine, ATTR, "%u", value));
>> 	igt_sysfs_scanf(engine, ATTR, "%u", &delay);
>> 	igt_assert_eq(delay, value);
>>   }
>> @@ -82,7 +82,7 @@ static int wait_for_reset(int fence)
>>
>>   static void test_idempotent(int i915, int engine)
>>   {
>> -	unsigned int delays[] = { 0, 1, 1000, 1234, 654321 };
>> +	unsigned int delays[] = { 0, 1, 1000, 1234, 54321 };
> By way of documenting the difference between GuC and execlists, I think we
> should use gem_using_guc_submission and define two different arrays, one
> for execlists and the other for GuC?
I really don't think it is worth the effort. Is execlist mode ever going 
to genuinely want an pre-emption timeout or execution quantum of 654s? 
And note that this test is not actually testing the behaviour with those 
values. It merely tests that it can set the value. There are other 
behavioural tests and they max out at 0.5s. So I don't see any benefit 
to adding in the extra complexity to verify that a certain range of 
values passes on execlist but fails on GuC.

>
> We could of course go the extra yard and check for errors with excessively
> large values but I'm not sure if that's worth it so am ok if we skip that
> at this point. Or that's a later patch.
The 'invalid' test already puts in 'excessively large' values and checks 
that they are rejected.

John.

>
> Below too.
>
> Thanks.
> --
> Ashutosh
>
>
>> 	unsigned int saved;
>>
>> 	/* Quick test that store/show reports the same values */
>> diff --git a/tests/i915/sysfs_timeslice_duration.c b/tests/i915/sysfs_timeslice_duration.c
>> index 8a2f1c2f2ece..95dc377785a5 100644
>> --- a/tests/i915/sysfs_timeslice_duration.c
>> +++ b/tests/i915/sysfs_timeslice_duration.c
>> @@ -79,7 +79,7 @@ static void set_timeslice(int engine, unsigned int value)
>>   {
>> 	unsigned int delay;
>>
>> -	igt_sysfs_printf(engine, ATTR, "%u", value);
>> +	igt_assert_lte(0, igt_sysfs_printf(engine, ATTR, "%u", value));
>> 	igt_sysfs_scanf(engine, ATTR, "%u", &delay);
>> 	igt_assert_eq(delay, value);
>>   }
>> @@ -93,7 +93,7 @@ static int wait_for_reset(int fence)
>>
>>   static void test_idempotent(int i915, int engine)
>>   {
>> -	const unsigned int delays[] = { 0, 1, 1234, 654321 };
>> +	const unsigned int delays[] = { 0, 1, 1234, 54321 };
>> 	unsigned int saved;
>>
>> 	/* Quick test to verify the kernel reports the same values as we write */
>> --
>> 2.37.3
>>