[PATCH igt] igt/perf_pmu: Bump batch_duration for legacy sampling inaccuracy

[PATCH igt] igt/perf_pmu: Bump batch_duration for legacy sampling inaccuracy

[Chris Wilson]

Since the legacy ringbuffer uses a sampling technique, it is limited to
an accuracy based on a 200Hz timer, or 5ms. We assert that measurements
are within 5%, so with a 100ms duration that gives us no room for the
systemmatic error in our sampling. Bump the duration to 500ms to give us
plenty of safety margin, if it then fails, it should not be due to the
sampling.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/perf_pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
index 61da224e..50ca7895 100644
--- a/tests/perf_pmu.c
+++ b/tests/perf_pmu.c
@@ -44,7 +44,7 @@
 IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
 
 const double tolerance = 0.05f;
-const unsigned long batch_duration_ns = 100e6;
+const unsigned long batch_duration_ns = 500e6;
 
 static int open_pmu(uint64_t config)
 {
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt] igt/perf_pmu: Bump batch_duration for legacy sampling inaccuracy

[Tvrtko Ursulin]

On 23/11/2017 00:08, Chris Wilson wrote:
> Since the legacy ringbuffer uses a sampling technique, it is limited to
> an accuracy based on a 200Hz timer, or 5ms. We assert that measurements
> are within 5%, so with a 100ms duration that gives us no room for the
> systemmatic error in our sampling. Bump the duration to 500ms to give us
> plenty of safety margin, if it then fails, it should not be due to the
> sampling.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   tests/perf_pmu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> index 61da224e..50ca7895 100644
> --- a/tests/perf_pmu.c
> +++ b/tests/perf_pmu.c
> @@ -44,7 +44,7 @@
>   IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
>   
>   const double tolerance = 0.05f;
> -const unsigned long batch_duration_ns = 100e6;
> +const unsigned long batch_duration_ns = 500e6;
>   
>   static int open_pmu(uint64_t config)
>   {
> 

Hm, it is definitely too short in sampling mode as you describe in the 
commit.

I am only a bit unhappy that 5x increase makes the total test run much 
longer. Embedding knowledge in the test on what counters are sampling 
and what not would be too bad?

Or perhaps a compromise on those by extending the batch duration a 
little bit less, and increasing the tolerance a bit?

That would mean adding variables like sampling_batch_duration_ns and 
sampling_tolerance and busyness based tests would also pick based on gen.

If you would be happy with that I'll implement it.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt] igt/perf_pmu: Bump batch_duration for legacy sampling inaccuracy

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-23 07:14:13)
> 
> On 23/11/2017 00:08, Chris Wilson wrote:
> > Since the legacy ringbuffer uses a sampling technique, it is limited to
> > an accuracy based on a 200Hz timer, or 5ms. We assert that measurements
> > are within 5%, so with a 100ms duration that gives us no room for the
> > systemmatic error in our sampling. Bump the duration to 500ms to give us
> > plenty of safety margin, if it then fails, it should not be due to the
> > sampling.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   tests/perf_pmu.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > index 61da224e..50ca7895 100644
> > --- a/tests/perf_pmu.c
> > +++ b/tests/perf_pmu.c
> > @@ -44,7 +44,7 @@
> >   IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
> >   
> >   const double tolerance = 0.05f;
> > -const unsigned long batch_duration_ns = 100e6;
> > +const unsigned long batch_duration_ns = 500e6;
> >   
> >   static int open_pmu(uint64_t config)
> >   {
> > 
> 
> Hm, it is definitely too short in sampling mode as you describe in the 
> commit.
> 
> I am only a bit unhappy that 5x increase makes the total test run much 
> longer. Embedding knowledge in the test on what counters are sampling 
> and what not would be too bad?
> 
> Or perhaps a compromise on those by extending the batch duration a 
> little bit less, and increasing the tolerance a bit?

My rough estimate with the current tolerance we need a minimum of 300ms
batch to hide the sampling inaccuracy (liberal use of Nyquist plus error
accumulation). 500ms then to give enough slack to be sure it's not a
systematic error from sampling.

Increasing tolerance is a bit harder to sell, I think. You do want some
notion of accuracy and 5% is a "happy" value.

> That would mean adding variables like sampling_batch_duration_ns and 
> sampling_tolerance and busyness based tests would also pick based on gen.
> 
> If you would be happy with that I'll implement it.

You want something more complicated go for it. Personally, even with .5s
batch duration total runtime wasn't an issue for me. (It's the pauses on
frequency, interrupts  and rc6 that start to get me worried!)

Total runtime with .5s is just under 40s.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt] igt/perf_pmu: Bump batch_duration for legacy sampling inaccuracy

[Tvrtko Ursulin]

On 23/11/2017 07:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-11-23 07:14:13)
>>
>> On 23/11/2017 00:08, Chris Wilson wrote:
>>> Since the legacy ringbuffer uses a sampling technique, it is limited to
>>> an accuracy based on a 200Hz timer, or 5ms. We assert that measurements
>>> are within 5%, so with a 100ms duration that gives us no room for the
>>> systemmatic error in our sampling. Bump the duration to 500ms to give us
>>> plenty of safety margin, if it then fails, it should not be due to the
>>> sampling.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    tests/perf_pmu.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
>>> index 61da224e..50ca7895 100644
>>> --- a/tests/perf_pmu.c
>>> +++ b/tests/perf_pmu.c
>>> @@ -44,7 +44,7 @@
>>>    IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
>>>    
>>>    const double tolerance = 0.05f;
>>> -const unsigned long batch_duration_ns = 100e6;
>>> +const unsigned long batch_duration_ns = 500e6;
>>>    
>>>    static int open_pmu(uint64_t config)
>>>    {
>>>
>>
>> Hm, it is definitely too short in sampling mode as you describe in the
>> commit.
>>
>> I am only a bit unhappy that 5x increase makes the total test run much
>> longer. Embedding knowledge in the test on what counters are sampling
>> and what not would be too bad?
>>
>> Or perhaps a compromise on those by extending the batch duration a
>> little bit less, and increasing the tolerance a bit?
> 
> My rough estimate with the current tolerance we need a minimum of 300ms
> batch to hide the sampling inaccuracy (liberal use of Nyquist plus error
> accumulation). 500ms then to give enough slack to be sure it's not a
> systematic error from sampling.
> 
> Increasing tolerance is a bit harder to sell, I think. You do want some
> notion of accuracy and 5% is a "happy" value.
> 
>> That would mean adding variables like sampling_batch_duration_ns and
>> sampling_tolerance and busyness based tests would also pick based on gen.
>>
>> If you would be happy with that I'll implement it.
> 
> You want something more complicated go for it. Personally, even with .5s
> batch duration total runtime wasn't an issue for me. (It's the pauses on
> frequency, interrupts  and rc6 that start to get me worried!)
> 
> Total runtime with .5s is just under 40s.

You are right, it has a much smaller effect than I assumed.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for igt/perf_pmu: Bump batch_duration for legacy sampling inaccuracy

[Patchwork]

== Series Details ==

Series: igt/perf_pmu: Bump batch_duration for legacy sampling inaccuracy
URL   : https://patchwork.freedesktop.org/series/34263/
State : failure

== Summary ==

Series 34263 revision 1 was fully merged or fully failed: no git log

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx