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

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

[Sowmya Kaparthi]

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@intel.com>
Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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);
 
 	__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);
 }
-- 
2.25.1

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

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

[Lionel Landwerlin]

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@intel.com>
> Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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);


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


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

[igt-dev] ✗ Fi.CI.BUILD: failure for Fixing the latency of hrtimer (rev4)

[Patchwork]

== Series Details ==

Series: Fixing the latency of hrtimer (rev4)
URL   : https://patchwork.freedesktop.org/series/81132/
State : failure

== Summary ==

Applying: Fixing the latency of hrtimer
Using index info to reconstruct a base tree...
M	tests/i915/perf.c
Falling back to patching base and 3-way merge...
Auto-merging tests/i915/perf.c
CONFLICT (content): Merge conflict in tests/i915/perf.c
Patch failed at 0001 Fixing the latency of hrtimer
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


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

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

[Umesh Nerlige Ramappa]

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@intel.com>
>>Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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

>
>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@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);
>>  }
>
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [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@intel.com>
>>> Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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@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);
>>>  }
>>
>>

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

Re: [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@intel.com>
>>>>Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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@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);
>>>> }
>>>
>>>
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Sowmya Kaparthi]

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@intel.com>
Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@intel.com>
---
 tests/i915/perf.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tests/i915/perf.c b/tests/i915/perf.c
index a894fd38..20d0cc82 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,13 @@ 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);
 
 	__perf_close(stream_fd);
 }
@@ -2152,6 +2159,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 +2294,13 @@ 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);
 }
-- 
2.25.1

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

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

[Lionel Landwerlin]

Sure,

The proportion of time in the test spent in the kernel depends on the 
number of wakeups.
Wakeups are driven by the hrtimer, so if you decrease the hrtimer, 
you'll have more wakeups and spend more time in the kernel.

The current expectation was computed for 5ms wakeups and it was 
expecting <= 1% of the time spent in the kernel.
With 2ms wakeups for instance you should be a bit over twice that : <= 2.5%.

-Lionel

On 27/08/2020 15:32, Kaparthi, SowmyaX wrote:
> As per your suggestions. I have updated the code and tested. It works fine with this change but  I did not understood what you are suggesting here. Can you please clearly make me understand that where I went wrong.
>
> -----Original Message-----
> From: Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>
> Sent: 27 August 2020 17:42
> To: Kaparthi, SowmyaX <sowmyax.kaparthi@intel.com>; igt-dev@lists.freedesktop.org; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>
> Subject: Re: [i-g-t] Fixing the latency of hrtimer
>
> On 27/08/2020 15:02, 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@intel.com>
>> Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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..952da84c 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);
> That's not quite right.
>
>
> test_duration_ns / 100 is the expectation for a 5ms kernel_hrtimer.
>
> You can't just multiply that by kernel_hrtimer, it has a be proportional.
>
>
> Thanks,
>
>
> -Lionel
>
>
>> +else
>> +max_expected_kernel_ns = test_duration_ns / 100;
>> +
>> +igt_assert(kernel_ns <= max_expected_kernel_ns);
>>
>>    __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);
>> +else
>> +max_expected_kernel_ns = test_duration_ns / 100;
>> +
>> +igt_assert(kernel_ns <= max_expected_kernel_ns);
>>
>>    __perf_close(stream_fd);
>>    }
>

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

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

[Lionel Landwerlin]

On 27/08/2020 15:02, 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@intel.com>
> Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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..952da84c 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);

That's not quite right.


test_duration_ns / 100 is the expectation for a 5ms kernel_hrtimer.

You can't just multiply that by kernel_hrtimer, it has a be proportional.


Thanks,


-Lionel


> +	else
> +		max_expected_kernel_ns = test_duration_ns / 100;
> +
> +	igt_assert(kernel_ns <= max_expected_kernel_ns);
>   
>   	__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);
> +	else
> +		max_expected_kernel_ns = test_duration_ns / 100;
> +
> +	igt_assert(kernel_ns <= max_expected_kernel_ns);
>   
>   	__perf_close(stream_fd);
>   }


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

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

[Sowmya Kaparthi]

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@intel.com>
Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@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..952da84c 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);
+	else
+		max_expected_kernel_ns = test_duration_ns / 100;
+
+	igt_assert(kernel_ns <= max_expected_kernel_ns);
 
 	__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);
+	else
+		max_expected_kernel_ns = test_duration_ns / 100;
+
+	igt_assert(kernel_ns <= max_expected_kernel_ns);
 
 	__perf_close(stream_fd);
 }
-- 
2.25.1

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

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

[Lionel Landwerlin]

Looking at the check :

igt_assert(kernel_ns <= (test_duration_ns / 100ull));

It seems to be some sanity check that we're 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.

5000000 (default hrtimer) -> 1/100th
2000000 -> 1/250th

I would update the check to this :

/* We expect the kernel time to not exceed 1/100th of the total test 
duration at the default hrtimer of 200Hz/every5ms. Scale that by the 
provided hrtimer if applicable. */
uint64_t max_expected_kernel_ns;

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

Cheers,

-Lionel

On 27/08/2020 11:06, Sowmya Kaparthi wrote:
> The blocking/polling parameterized tests were introduced to test
> different hrtimer configurations.These tests check how many times the
> process wakes up to read the reports with different hrtimer values (=
> duration of test / hrtimer value). A user is more likely to choose a
> larger hrtimer value than the default 5ms to avoid wake up too frequently.
>
> Cc: Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>
> Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@intel.com>
> ---
>   tests/i915/perf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
> index a894fd38..5fd1193f 100644
> --- a/tests/i915/perf.c
> +++ b/tests/i915/perf.c
> @@ -4995,7 +4995,7 @@ igt_main
>   			      40 * 1000 * 1000 /* default 40ms hrtimer */);
>   		test_blocking(500 * 1000 /* 500us oa period */,
>   			      true /* set_kernel_hrtimer */,
> -			      2 * 1000 * 1000 /* default 2ms hrtimer */);
> +			      10 * 1000 * 1000 /* default 10ms hrtimer */);
>   	}
>   
>   	igt_describe("Test polled read with default hrtimer frequency");
> @@ -5014,7 +5014,7 @@ igt_main
>   			     40 * 1000 * 1000 /* default 40ms hrtimer */);
>   		test_polling(500 * 1000 /* 500us oa period */,
>   			     true /* set_kernel_hrtimer */,
> -			     2 * 1000 * 1000 /* default 2ms hrtimer */);
> +			     10 * 1000 * 1000 /* default 10ms hrtimer */);
>   	}
>   
>   	igt_describe("Test polled read with buffer size smaller than available data");


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

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

[Sowmya Kaparthi]

The blocking/polling parameterized tests were introduced to test
different hrtimer configurations.These tests check how many times the
process wakes up to read the reports with different hrtimer values (=
duration of test / hrtimer value). A user is more likely to choose a
larger hrtimer value than the default 5ms to avoid wake up too frequently.

Cc: Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>
Signed-off-by: Sowmya Kaparthi <sowmyax.kaparthi@intel.com>
---
 tests/i915/perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/i915/perf.c b/tests/i915/perf.c
index a894fd38..5fd1193f 100644
--- a/tests/i915/perf.c
+++ b/tests/i915/perf.c
@@ -4995,7 +4995,7 @@ igt_main
 			      40 * 1000 * 1000 /* default 40ms hrtimer */);
 		test_blocking(500 * 1000 /* 500us oa period */,
 			      true /* set_kernel_hrtimer */,
-			      2 * 1000 * 1000 /* default 2ms hrtimer */);
+			      10 * 1000 * 1000 /* default 10ms hrtimer */);
 	}
 
 	igt_describe("Test polled read with default hrtimer frequency");
@@ -5014,7 +5014,7 @@ igt_main
 			     40 * 1000 * 1000 /* default 40ms hrtimer */);
 		test_polling(500 * 1000 /* 500us oa period */,
 			     true /* set_kernel_hrtimer */,
-			     2 * 1000 * 1000 /* default 2ms hrtimer */);
+			     10 * 1000 * 1000 /* default 10ms hrtimer */);
 	}
 
 	igt_describe("Test polled read with buffer size smaller than available data");
-- 
2.25.1

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