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.harri...@intel.com wrote: > >> From: John Harrison <john.c.harri...@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.harri...@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.di...@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 > >> >