[Intel-gfx] [PATCH 6/7] drm/i915/skl: Updated the 'i915_frequency_info' debugs function

[Akash Goel]

On Tue, 2015-02-17 at 15:38 +0000, Damien Lespiau wrote:
> On Fri, Feb 06, 2015 at 08:26:37PM +0530, akash.goel at intel.com wrote:
> > From: Akash Goel <akash.goel at intel.com>
> > 
> > Added support for SKL in the 'i915_frequency_info' debugfs function
> > 
> > Signed-off-by: Akash Goel <akash.goel at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 9af17fb..32c62a2 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1089,7 +1089,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  		seq_printf(m, "Current P-state: %d\n",
> >  			   (rgvstat & MEMSTAT_PSTATE_MASK) >> MEMSTAT_PSTATE_SHIFT);
> >  	} else if (IS_GEN6(dev) || (IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) ||
> > -		   IS_BROADWELL(dev)) {
> > +		   IS_BROADWELL(dev) || IS_GEN9(dev)) {
> 
> Can we be optimistic by default (and hope next platform will be no extra
> work by having a INTEL_INFO(dev)->gen >= 9)?
Sorry I am not sure..
If it turns out, that there is no extra work required for future
platforms, then at that time we can do >= 9 change. 

> 
> >  		u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
> >  		u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
> >  		u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> > @@ -1109,8 +1109,12 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  
> >  		reqf = I915_READ(GEN6_RPNSWREQ);
> >  		reqf &= ~GEN6_TURBO_DISABLE;
> > +		if (!IS_GEN9(dev))
> > +			reqf &= ~GEN6_TURBO_DISABLE;
> 
> It seems like you to remove one masking of bit 31? (can we have >= 9 as
> well?).  
> 
> Maybe a simpler way to go about it would be:

Thanks for spotting this, will modify it as per your suggestion.
> 
> 		if (INTEL_INFO(dev)->gen >= 9)
> 			reqf >>= 23;
> 		else {
> 			reqf &= ~GEN6_TURBO_DISABLE;
> 			if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> 				reqf >>= 24;
> 			else
> 				reqf >>= 25;
> 		}
> 
> >  		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  			reqf >>= 24;
> > +		else if IS_GEN9(dev)
> > +			reqf >>= 23;
> >  		else
> >  			reqf >>= 25;
> >  		reqf = intel_gpu_freq(dev_priv, reqf);
> > @@ -1128,6 +1132,8 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  		rpprevdown = I915_READ(GEN6_RP_PREV_DOWN);
> >  		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  			cagf = (rpstat & HSW_CAGF_MASK) >> HSW_CAGF_SHIFT;
> > +		else if (IS_GEN9(dev))
> > +			cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT;
> >  		else
> >  			cagf = (rpstat & GEN6_CAGF_MASK) >> GEN6_CAGF_SHIFT;
> 
> cagf (as well as reqf) is(are) used in a printf saying they are Mhz. That looks
> wrong.

There is a conversion done below, before the printf, through
"cagf = intel_gpu_freq(dev_priv, cagf)"
So should be alright then ?
But not able to find the precise info, that frequency specified in
0xA01C register, is in which units for GEN9.

For the reqf, there is a conversion missing from 16.667 MHZ units, for
GEN9. Will add that.

> 
> >  		cagf = intel_gpu_freq(dev_priv, cagf);
> > @@ -1152,7 +1158,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  			   pm_ier, pm_imr, pm_isr, pm_iir, pm_mask);
> >  		seq_printf(m, "GT_PERF_STATUS: 0x%08x\n", gt_perf_status);
> >  		seq_printf(m, "Render p-state ratio: %d\n",
> > -			   (gt_perf_status & 0xff00) >> 8);
> > +			   (gt_perf_status & (IS_GEN9(dev) ? 0x1ff00 : 0xff00)) >> 8);
> 
> Eeek, that's a weird name to say freq. Here the 16.66 unit strikes back, can we
> have at least a comment?
Sorry didn't get this point. Just printing the raw value of P State
ratio (Un-Slice) in GT_PERF_STATUS register.
> 
> >  		seq_printf(m, "Render p-state VID: %d\n",
> >  			   gt_perf_status & 0xff);
> >  		seq_printf(m, "Render p-state limit: %d\n",
> > -- 
> > 1.9.2
> > 

Best Regards
Akash