[igt-dev] [i-g-t] Fixing the latency of hrtimer

[Lionel Landwerlin]

On 28/08/2020 22:34, Umesh Nerlige Ramappa wrote:
> On Thu, Aug 27, 2020 at 06:22:40PM +0300, Lionel Landwerlin wrote:
>> On 27/08/2020 18:09, Sowmya Kaparthi wrote:
>>> This test undergoes sanity check that we are not spending more than
>>> 1/100th of the time awake in the kernel. The right thing would be to
>>> adjust that value by the number of expected wakeups. We expect the
>>> kernel time not to exceed 1/100th of the total test duration at the
>>> default hrtimer of 200Hz/every5ms. Scale that by the provided 
>>> hrtimer if
>>> applicable.
>>>
>>> Cc: Landwerlin, Lionel G <lionel.g.landwerlin at intel.com>
>>> Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi at intel.com>
>>> ---
>>>  tests/i915/perf.c | 16 ++++++++++++++--
>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
>>> index a894fd38..0caa9fe0 100644
>>> --- a/tests/i915/perf.c
>>> +++ b/tests/i915/perf.c
>>> @@ -1993,6 +1993,7 @@ test_blocking(uint64_t requested_oa_period, 
>>> bool set_kernel_hrtimer, uint64_t ke
>>>      int64_t start, end;
>>>      int n = 0;
>>> +    uint64_t max_expected_kernel_ns;
>>>      stream_fd = __perf_open(drm_fd, &param, true /* prevent_pm */);
>>> @@ -2098,7 +2099,12 @@ test_blocking(uint64_t requested_oa_period, 
>>> bool set_kernel_hrtimer, uint64_t ke
>>>       */
>>>      igt_assert(n > (min_iterations + n_extra_iterations));
>>> -    igt_assert(kernel_ns <= (test_duration_ns / 100ull));
>>> +    if (set_kernel_hrtimer)
>>> +        max_expected_kernel_ns = kernel_hrtimer * (test_duration_ns 
>>> / 100)
>>> +                     /(5 * 1000 * 1000);
>>> +    else
>>> +        max_expected_kernel_ns = test_duration_ns / 100;
>>> +    igt_assert(kernel_ns <= max_expected_kernel_ns);
>>
>
> Sorry, a little late to comment on this, but here it is anyways:
>
> I agree that we should ideally scale the check so that different 
> values of hrtimer can be accounted for. One of the values that should 
> factor into this calculation is the average time taken by a read on a 
> particular platform. For example, we saw an average 30 us on TGL and 
> this was counting up to around 150 ms in kernel_ns, so test failed. 
> That said, this is time taken by the cpu and I am not sure we can 
> guarantee this time. If the cpu frequency is scaled, we have different 
> values.  Also different platforms would have different results. Unless 
> we can enforce a cpu frequency from IGT and calibrate the kernel_ns 
> for each platform, this check is going to be buggy.
>
> For the *-parameterized tests alone, I would recommend not checking 
> kernel_ns at all because the intent of the test is to make sure we are 
> woken up a predetermined number of times for different values of hrtimer.
>
> Thoughts?
>
> Thanks,
> Umesh


Agreed, those checks are pretty old and I'm struggling to find a good 
way to find whether the kernel is doing the right thing.

Tracepoints maybe?


If you want to drop the checks that would be fine.


Thanks,


-Lionel


>
>>
>> Thanks, looks good. Just a small coding style issue.
>>
>> Multiline blocks in if/for/while() needs to be surrounded by {}.
>>
>>
>> With that fixed:
>>
>>
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>
>>
>> Cheers,
>>
>>
>> -Lionel
>>
>>
>>>      __perf_close(stream_fd);
>>>  }
>>> @@ -2152,6 +2158,7 @@ test_polling(uint64_t requested_oa_period, 
>>> bool set_kernel_hrtimer, uint64_t ker
>>>      int min_iterations = (test_duration_ns / (oa_period + 
>>> (kernel_hrtimer + kernel_hrtimer / 5)));
>>>      int64_t start, end;
>>>      int n = 0;
>>> +    uint64_t max_expected_kernel_ns;
>>>      stream_fd = __perf_open(drm_fd, &param, true /* prevent_pm */);
>>> @@ -2286,7 +2293,12 @@ test_polling(uint64_t requested_oa_period, 
>>> bool set_kernel_hrtimer, uint64_t ker
>>>       */
>>>      igt_assert(n > (min_iterations + n_extra_iterations));
>>> -    igt_assert(kernel_ns <= (test_duration_ns / 100ull));
>>> +    if (set_kernel_hrtimer)
>>> +        max_expected_kernel_ns = kernel_hrtimer * (test_duration_ns 
>>> / 100)
>>> +                     / (5 * 1000 * 1000);
>>> +    else
>>> +        max_expected_kernel_ns = test_duration_ns / 100;
>>> +    igt_assert(kernel_ns <= max_expected_kernel_ns);
>>>      __perf_close(stream_fd);
>>>  }
>>
>>