[Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] gem_wsim: Check sleep times

[Tvrtko Ursulin]

On 14/08/2018 16:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-08-14 16:21:08)
>>
>> On 14/08/2018 16:09, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-08-14 16:05:13)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Notice in more places if we are running behind.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> ---
>>>>    benchmarks/gem_wsim.c | 52 ++++++++++++++++++++++++++++++++++++++-----
>>>>    1 file changed, 46 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>>>> index 80f180829241..ead91b0f009b 100644
>>>> --- a/benchmarks/gem_wsim.c
>>>> +++ b/benchmarks/gem_wsim.c
>>>> @@ -1718,6 +1718,21 @@ static bool sync_deps(struct workload *wrk, struct w_step *w)
>>>>           return synced;
>>>>    }
>>>>    
>>>> +static unsigned int measured_usleep(unsigned int usec)
>>>> +{
>>>> +       struct timespec ts = { };
>>>> +       unsigned int slept;
>>>> +
>>>> +       slept = igt_nsec_elapsed(&ts);
>>>> +       igt_assert(slept == 0);
>>>> +       do {
>>>> +               usleep(usec - slept);
>>>> +               slept = igt_nsec_elapsed(&ts) / 1000;
>>>> +       } while (slept < usec);
>>>> +
>>>> +       return igt_nsec_elapsed(&ts);
>>>> +}
>>>> +
>>>>    static void *run_workload(void *data)
>>>>    {
>>>>           struct workload *wrk = (struct workload *)data;
>>>> @@ -1739,7 +1754,7 @@ static void *run_workload(void *data)
>>>>                count++) {
>>>>                   unsigned int cur_seqno = wrk->sync_seqno;
>>>>    
>>>> -               clock_gettime(CLOCK_MONOTONIC, &wrk->repeat_start);
>>>> +               igt_gettime(&wrk->repeat_start);
>>>
>>> Were you already linking against libigt?
>>> This code is not set up to hit an igt_assert()...
>>
>> This code meaning gem_wsim? There's a lot of asserts in it already, and
>> when they trigger traceback looks fine. What do you think is missing?
> 
> We have different values of fine ;) Looks nothing but a mess for me.
> I don't think igt itself has much value outside of being the test
> runner, we've baked too much knowledge of it being a test harness into
> the code (and in many cases work to reduce those assumptions). I'd
> rather libigtcore be distinct and not bring in the interlinked
> igt_subtest+igt_assert.

To not use it is to reimplement existing handy helpers - not ideal.

To split the test bits from useful helpers sounds like quite a task.

To wean gem_wsim off most igt_ stuff needs decision to be made on the 
former - what is core api, and what is test related.

To me problems outweigh the benefit. :( If gem_wsim was under tools, 
like intel_gpu_top, then yeah, it would be justifiable. Since it is in 
benchmarks I don't think we should bother.

Regards,

Tvrtko