Re: [PATCH 2/2] drm/i915/vlv: use correct units for rc6 residency

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 26 Sep 2013 23:25:46 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, Sep 26, 2013 at 12:33:21PM -0700, Jesse Barnes wrote:
> > We need to use the clock control reg to figure out how many CZ clks are in
> > 30ns and use that as the basis for our RC6 residency calculations.
> 
> Hmm, that was confusing. Took a couple of reads to be sure that the
> specs said that the units were always CZ clock cycles.
> 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=69692
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h   |  3 +++
> >  drivers/gpu/drm/i915/i915_sysfs.c | 22 ++++++++++++++++++++--
> >  2 files changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index cf995bb..6f8d0cf 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1797,6 +1797,9 @@
> >   */
> >  #define HSW_CXT_TOTAL_SIZE		(17 * PAGE_SIZE)
> >  
> > +#define VLV_CLK_CTL2			0x101104
> > +#define   CLK_CTL2_CZCOUNT_30NS_SHIFT	28
> > +
> >  /*
> >   * Overlay regs
> >   */
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 44f4c1a..9c60515 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -37,12 +37,30 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	u64 raw_time; /* 32b value may overflow during fixed point math */
> > +	u64 units = 128ULL, div = 100 000ULL;
> 
> The ULL suffix here are superfluous and I notice that you didn't use the
> suffix for the later constants. Be consistent.
> 
> Normal units = 128 / (100 * 1000), i.e. each unit is 1.28/1000ms

I can drop the ULL, sure.

> 
> >  
> >  	if (!intel_enable_rc6(dev))
> >  		return 0;
> >  
> > -	raw_time = I915_READ(reg) * 128ULL;
> > -	return DIV_ROUND_UP_ULL(raw_time, 100000);
> > +	/* On VLV, residency time is in CZ units rather than 1.28us */
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		u32 clkctl2;
> > +
> > +		clkctl2 = I915_READ(VLV_CLK_CTL2) >>
> > +			CLK_CTL2_CZCOUNT_30NS_SHIFT;
> > +		if (!clkctl2) {
> > +			WARN(!clkctl2, "bogus CZ count value");
> > +			return 0;
> > +		}
> > +		units = DIV_ROUND_UP_ULL(3000ULL, (u64)clkctl2);
> 
> For your divisor, this should 30*1000 not 3*1000.

30ns * 100 for fixed point precision, just as above.

> 
> > +		if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
> > +			units <<= 8;
> > +
> > +		div = 100 000 000;

Then here we divide out the ns to ms (1000000) and also the 100 for
fixed point.

Or do I still have it wrong?

-- 
Jesse Barnes, Intel Open Source Technology Center