Re: [PATCH 05/19] drm/i915/perf: Enable commands per clock reporting in OA

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>]

On Tue, Sep 13, 2022 at 05:19:24PM -0700, Dixit, Ashutosh wrote:
> On Tue, 23 Aug 2022 13:41:41 -0700, Umesh Nerlige Ramappa wrote:
> >
>
> Hi Umesh,
>
> > XEHPSDV and DG2 provide a way to configure bytes per clock vs commands
> > per clock reporting. Enable command per clock setting on enabling OA.

should be: Enable bytes per clock setting
> What is the reason for selecting commands per clock vs bytes per clock?
> Also probably mention Bspec: 51762 in the commit message too.

It's a default configuration used to interpret the A36/A37 counters here 
- Bspec: 52201

> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index efa7eda83edd..6fc4f0d8fc5a 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2745,10 +2745,12 @@ static int
> >  gen12_enable_metric_set(struct i915_perf_stream *stream,
> > 			struct i915_active *active)
> >  {
> > +	struct drm_i915_private *i915 = stream->perf->i915;
> > 	struct intel_uncore *uncore = stream->uncore;
> > 	struct i915_oa_config *oa_config = stream->oa_config;
> > 	bool periodic = stream->periodic;
> > 	u32 period_exponent = stream->period_exponent;
> > +	u32 sqcnt1;
> > 	int ret;
> >
> > 	intel_uncore_write(uncore, GEN12_OAG_OA_DEBUG,
> > @@ -2767,6 +2769,16 @@ gen12_enable_metric_set(struct i915_perf_stream *stream,
> > 			    (period_exponent << GEN12_OAG_OAGLBCTXCTRL_TIMER_PERIOD_SHIFT))
> > 			    : 0);
> >
> > +	/*
> > +	 * Initialize Super Queue Internal Cnt Register
> > +	 * Set PMON Enable in order to collect valid metrics.
> > +	 * Enable commands per clock reporting in OA for XEHPSDV onward.
> > +	 */
> > +	sqcnt1 = GEN12_SQCNT1_PMON_ENABLE |
> > +		 (HAS_OA_BPC_REPORTING(i915) ? GEN12_SQCNT1_OABPC : 0);
>
> Also from Bspec 0:Unitsof4cmd and 1:Unitsof128B so looks like bit 29 should
> be set to 0 for commands per clock setting? Or I am wrong?

I know bit 29 has to be set for DG2. I think the commit message is 
wrong. Nice catch, thanks

> > +
> > +	intel_uncore_rmw(uncore, GEN12_SQCNT1, 0, sqcnt1);
> > +
> > 	/*
> > 	 * Update all contexts prior writing the mux configurations as we need
> > 	 * to make sure all slices/subslices are ON before writing to NOA
> > @@ -2816,6 +2828,8 @@ static void gen11_disable_metric_set(struct i915_perf_stream *stream)
> >  static void gen12_disable_metric_set(struct i915_perf_stream *stream)
> >  {
> > 	struct intel_uncore *uncore = stream->uncore;
> > +	struct drm_i915_private *i915 = stream->perf->i915;
> > +	u32 sqcnt1;
> >
> > 	/* Reset all contexts' slices/subslices configurations. */
> > 	gen12_configure_all_contexts(stream, NULL, NULL);
> > @@ -2826,6 +2840,12 @@ static void gen12_disable_metric_set(struct i915_perf_stream *stream)
> >
> > 	/* Make sure we disable noa to save power. */
> > 	intel_uncore_rmw(uncore, RPM_CONFIG1, GEN10_GT_NOA_ENABLE, 0);
> > +
> > +	sqcnt1 = GEN12_SQCNT1_PMON_ENABLE |
> > +		 (HAS_OA_BPC_REPORTING(i915) ? GEN12_SQCNT1_OABPC : 0);
> > +
> > +	/* Reset PMON Enable to save power. */
> > +	intel_uncore_rmw(uncore, GEN12_SQCNT1, sqcnt1, 0);
> >  }
> >
> >  static void gen7_oa_enable(struct i915_perf_stream *stream)
> > diff --git a/drivers/gpu/drm/i915/i915_perf_oa_regs.h b/drivers/gpu/drm/i915/i915_perf_oa_regs.h
> > index 0ef3562ff4aa..381d94101610 100644
> > --- a/drivers/gpu/drm/i915/i915_perf_oa_regs.h
> > +++ b/drivers/gpu/drm/i915/i915_perf_oa_regs.h
> > @@ -134,4 +134,8 @@
> >  #define GDT_CHICKEN_BITS    _MMIO(0x9840)
> >  #define   GT_NOA_ENABLE	    0x00000080
> >
> > +#define GEN12_SQCNT1				_MMIO(0x8718)
> > +#define   GEN12_SQCNT1_PMON_ENABLE		REG_BIT(30)
> > +#define   GEN12_SQCNT1_OABPC			REG_BIT(29)
> > +
> >  #endif /* __INTEL_PERF_OA_REGS__ */