[i-g-t] i915/perf_pmu: Measure how many batches can fit into the ring
diff mbox series

Message ID 20191204132051.3740419-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [i-g-t] i915/perf_pmu: Measure how many batches can fit into the ring
Related show

Commit Message

Chris Wilson Dec. 4, 2019, 1:20 p.m. UTC
Do not blindly assume 30 spin batches will always fit into the ring, but
use our measurement tool instead.

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

Comments

Summers, Stuart Dec. 4, 2019, 7:13 p.m. UTC | #1
On Wed, 2019-12-04 at 13:20 +0000, Chris Wilson wrote:
> Do not blindly assume 30 spin batches will always fit into the ring,
> but
> use our measurement tool instead.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  tests/perf_pmu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> index de4c231dd..8e50ac9a0 100644
> --- a/tests/perf_pmu.c
> +++ b/tests/perf_pmu.c
> @@ -42,6 +42,7 @@
>  #include "igt_perf.h"
>  #include "igt_sysfs.h"
>  #include "igt_pm.h"
> +#include "i915/gem_ring.h"
>  #include "sw_sync.h"
>  
>  IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
> @@ -1276,8 +1277,9 @@ static void cpu_hotplug(int gem_fd)
>  static void
>  test_interrupts(int gem_fd)
>  {
> +	const int target =
> +		gem_measure_ring_inflight(gem_fd, I915_EXEC_DEFAULT,
> 0);

In case we ever want to change this engine, should we make
I915_EXEC_DEFAULT a macro within this test?

Looks a lot better. My only question here is can we make
gem_measure_ring_inflight a generic routine instead of something i915-
specific, since we're using this in one of the cross-arch tests?

Thanks,
Stuart

>  	const unsigned int test_duration_ms = 1000;
> -	const int target = 30;
>  	igt_spin_t *spin[target];
>  	struct pollfd pfd;
>  	uint64_t idle, busy;
Chris Wilson Dec. 4, 2019, 7:21 p.m. UTC | #2
Quoting Summers, Stuart (2019-12-04 19:13:16)
> On Wed, 2019-12-04 at 13:20 +0000, Chris Wilson wrote:
> > Do not blindly assume 30 spin batches will always fit into the ring,
> > but
> > use our measurement tool instead.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  tests/perf_pmu.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > index de4c231dd..8e50ac9a0 100644
> > --- a/tests/perf_pmu.c
> > +++ b/tests/perf_pmu.c
> > @@ -42,6 +42,7 @@
> >  #include "igt_perf.h"
> >  #include "igt_sysfs.h"
> >  #include "igt_pm.h"
> > +#include "i915/gem_ring.h"
> >  #include "sw_sync.h"
> >  
> >  IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
> > @@ -1276,8 +1277,9 @@ static void cpu_hotplug(int gem_fd)
> >  static void
> >  test_interrupts(int gem_fd)
> >  {
> > +     const int target =
> > +             gem_measure_ring_inflight(gem_fd, I915_EXEC_DEFAULT,
> > 0);
> 
> In case we ever want to change this engine, should we make
> I915_EXEC_DEFAULT a macro within this test?

Not really, EXEC_DEFAULT itself is the placeholder for first engine on
the system... I really should land my ffs() before it makes a
difference.
> 
> Looks a lot better. My only question here is can we make
> gem_measure_ring_inflight a generic routine instead of something i915-
> specific, since we're using this in one of the cross-arch tests?

This is not a generic test. Simply has not been moved yet.
-Chris
Summers, Stuart Dec. 4, 2019, 7:38 p.m. UTC | #3
On Wed, 2019-12-04 at 19:21 +0000, Chris Wilson wrote:
> Quoting Summers, Stuart (2019-12-04 19:13:16)
> > On Wed, 2019-12-04 at 13:20 +0000, Chris Wilson wrote:
> > > Do not blindly assume 30 spin batches will always fit into the
> > > ring,
> > > but
> > > use our measurement tool instead.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > ---
> > >  tests/perf_pmu.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > > index de4c231dd..8e50ac9a0 100644
> > > --- a/tests/perf_pmu.c
> > > +++ b/tests/perf_pmu.c
> > > @@ -42,6 +42,7 @@
> > >  #include "igt_perf.h"
> > >  #include "igt_sysfs.h"
> > >  #include "igt_pm.h"
> > > +#include "i915/gem_ring.h"
> > >  #include "sw_sync.h"
> > >  
> > >  IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
> > > @@ -1276,8 +1277,9 @@ static void cpu_hotplug(int gem_fd)
> > >  static void
> > >  test_interrupts(int gem_fd)
> > >  {
> > > +     const int target =
> > > +             gem_measure_ring_inflight(gem_fd,
> > > I915_EXEC_DEFAULT,
> > > 0);
> > 
> > In case we ever want to change this engine, should we make
> > I915_EXEC_DEFAULT a macro within this test?
> 
> Not really, EXEC_DEFAULT itself is the placeholder for first engine
> on
> the system... I really should land my ffs() before it makes a
> difference.
> > 
> > Looks a lot better. My only question here is can we make
> > gem_measure_ring_inflight a generic routine instead of something
> > i915-
> > specific, since we're using this in one of the cross-arch tests?
> 
> This is not a generic test. Simply has not been moved yet.

Makes sense then.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

> -Chris

Patch
diff mbox series

diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
index de4c231dd..8e50ac9a0 100644
--- a/tests/perf_pmu.c
+++ b/tests/perf_pmu.c
@@ -42,6 +42,7 @@ 
 #include "igt_perf.h"
 #include "igt_sysfs.h"
 #include "igt_pm.h"
+#include "i915/gem_ring.h"
 #include "sw_sync.h"
 
 IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
@@ -1276,8 +1277,9 @@  static void cpu_hotplug(int gem_fd)
 static void
 test_interrupts(int gem_fd)
 {
+	const int target =
+		gem_measure_ring_inflight(gem_fd, I915_EXEC_DEFAULT, 0);
 	const unsigned int test_duration_ms = 1000;
-	const int target = 30;
 	igt_spin_t *spin[target];
 	struct pollfd pfd;
 	uint64_t idle, busy;