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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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. :)

Regards,

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