Re: [igt-dev] [PATCH i-g-t] i915: Exercise preemption timeout controls in sysfs

[Petri Latvala <petri.latvala@intel.com>]

On Fri, Oct 18, 2019 at 01:39:37PM +0100, Tvrtko Ursulin wrote:
> 
> On 18/10/2019 13:35, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-10-18 13:23:53)
> > > 
> > > On 17/10/2019 15:30, Chris Wilson wrote:
> > > > Dynamic subtests!
> > > 
> > > Ouch! :)
> > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > > +static void test_timeout(int i915, int engine)
> > > > +{
> > > > +     int delays[] = { 1, 50, 100, 500 };
> > > > +     unsigned int saved, delay;
> > > > +
> > > > +     igt_assert(igt_sysfs_scanf(engine, "preempt_timeout_ms", "%u", &saved) == 1);
> > > > +     igt_debug("Initial preempt_timeout_ms:%u\n", saved);
> > > > +
> > > > +     gem_quiescent_gpu(i915);
> > > > +     igt_require(enable_hangcheck(i915, false));
> > > > +
> > > > +     for (int i = 0; i < ARRAY_SIZE(delays); i++) {
> > > > +             uint64_t elapsed;
> > > > +
> > > > +             elapsed = __test_timeout(i915, engine, delays[i]);
> > > > +             igt_info("preempt_timeout_ms:%d, elapsed=%.3fms\n",
> > > > +                      delays[i], elapsed * 1e-6);
> > > 
> > > No checking that measured time relates to configured timeout?
> > 
> > Have now. Just needed some soaking to decide on thresholds. I've 50ms
> > but that may change as CI tends to have more scheduling intolerance than
> > local machines.
> > 
> > > > +     }
> > > > +
> > > > +     igt_assert(enable_hangcheck(i915, true));
> > > > +     gem_quiescent_gpu(i915);
> > > > +
> > > > +     igt_sysfs_printf(engine, "preempt_timeout_ms", "%u", saved);
> > > > +     igt_sysfs_scanf(engine, "preempt_timeout_ms", "%u", &delay);
> > > > +     igt_assert_eq(delay, saved);
> > > > +}
> > > > +
> > > > +igt_main
> > > > +{
> > > > +     int i915, sys = -1;
> > > > +     struct dirent *de;
> > > > +     int engines;
> > > > +     DIR *dir;
> > > > +
> > > > +     igt_fixture {
> > > > +             i915 = drm_open_driver(DRIVER_INTEL);
> > > > +             igt_require_gem(i915);
> > > > +
> > > > +             sys = igt_sysfs_open(i915);
> > > > +             igt_require(sys != -1);
> > > 
> > > igt_assert_fd?
> > 
> > Do we guarantee that the sysadmin has mounted sysfs? We don't automount
> > it unlike debugfs.
> > 
> > > > +             igt_subtest_group {
> > > > +                     igt_fixture {
> > > > +                             igt_require(fstatat(engine,
> > > > +                                                 "preempt_timeout_ms",
> > > > +                                                 &st, 0) == 0);
> > > > +                     }
> > > > +
> > > > +                     igt_subtest_f("%s-idempotent", name)
> > > > +                             test_idempotent(i915, engine);
> > > > +                     igt_subtest_f("%s-invalid", name)
> > > > +                             test_invalid(i915, engine);
> > > > +                     igt_subtest_f("%s-timeout", name)
> > > > +                             test_timeout(i915, engine);
> > > > +             }
> > > > +
> > > > +             free(name);
> > > > +             close(engine);
> > > > +     }
> > > 
> > > You probably should use __for_each_static_engine and then open sysfs
> > > nodes based on that. Gets around the dynamic subtests no-no at least.
> > 
> > Defeatist!
> 
> Well I have challenged this status quo a few times and now I am embracing
> it, or should I say disagreeing and committing, so bonus points all round.
> :)


Perhaps next week I'll get around to reshaping the dynamic subtests
series. Watch this space!

(Meanwhile, I hope it goes without saying, dynamic subtests are indeed
a no-no)


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev