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

[Umesh Nerlige Ramappa]

On Fri, Aug 28, 2020 at 10:44:27PM +0300, Lionel Landwerlin wrote:
>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?

TGL values were measured with kernel function level ftrace. The time 
taken by the read remained very close between the 2 cases that the 
polling-parameterized test ran, so failure itself had nothing to do with 
the value of hrtimer, but was rather due to the number of wakeups.

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

Great, Let's drop the checks only for the parameterized tests.

Thanks,
Umesh

>
>
>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);
>>>> }
>>>
>>>
>