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@xxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > +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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
