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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 01 Nov 2022 09:22:11 -0700, John Harrison wrote:
>
> On 11/1/2022 08:27, Dixit, Ashutosh wrote:
> > On Mon, 31 Oct 2022 15:24:40 -0700, John.C.Harrison@Intel.com wrote:
> >> From: John Harrison <John.C.Harrison@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@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.

Fair enough:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

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