[Intel-gfx] [PATCH 15/20] drm/i915: turbo & RC6 support for VLV v2

[Jesse Barnes]

On Tue, 19 Mar 2013 15:27:36 -0700
Ben Widawsky <ben at bwidawsk.net> wrote:

> On Fri, Mar 08, 2013 at 10:45:58AM -0800, Jesse Barnes wrote:
> > From: Ben Widawsky <ben at bwidawsk.net>
> > 
> > Uses slightly different interfaces than other platforms.
> > 
> > v2: track actual set freq, not requested (Rohit)
> >     fix debug prints in init code (Jesse)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> 
> One important question is how is our punit code going to interact with
> the other potential users? It seems a bunch of power management drivers
> would also want to touch this uC.

Pretty sure the PUnit has to deal with concurrent access... if not
we'll have to add common routines for all drivers to use and do proper
locking.

> > +
> > +	valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval);
> > +	if ((pval >> 8) != val)
> > +		DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n",
> > +			  val, pval >> 8);
> I'm debating whether this is a useful thing to do here... You either get
> the frequency or you don't. Really it would seem more useful to me to
> check things are sane when you first enter the function (ie. did the
> last request do what you want). But I don't care what you end up with.

It's not really a matter of sanity, it's more about what state the
platform is in.  If the Punit decides things are getting too hot for
example, it may clamp your freq down.  That's totally ok and normal
though, and may change in future calls.

> I suppose if you wanted to make things a bit cleaner you could extract
> just the frequency setting part, since most of this function is
> identical to gen6 set rps.

I was afraid more changes would creep in over time, but yeah that's a
possibility.  I have a couple more changes here before I consider that.

> > +
> > +	/* Make sure we continue to get interrupts
> > +	 * until we hit the minimum or maximum frequencies.
> > +	 */
> > +	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
> > +
> > +	dev_priv->rps.cur_delay = pval >> 8;
> > +
> > +	trace_intel_gpu_freq_change(val * 50);
> Based on our IRC discussion, I believe the value in this trace is wrong.

Ah yeah, correct.  I can just trace val here maybe, or I"ll have to put
this off until I post the real frequencies.

> > +static void valleyview_enable_rps(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> > +	u32 gtfifodbg, val;
> > +	int i;
> > +
> > +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> 
> Should we check FB_GFX_TURBO_EN_FUSE here?

I guess it wouldn't hurt.

> > +
> > +	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
> > +		DRM_ERROR("GT fifo had a previous error %x\n", gtfifodbg);
> > +		I915_WRITE(GTFIFODBG, gtfifodbg);
> > +	}
> > +
> > +	gen6_gt_force_wake_get(dev_priv);
> > +
> > +	I915_WRITE(GEN6_RC_SLEEP, 0);
> You've dropped GEN6_RC1_WAKE_RATE_LIMIT. Intentional?

Uh I should have commented that.  I *think* it was intentional since
RC1 doesn't exist on VLV, but I'll have to check.

> > +	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 0x00280000);
> > +	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
> > +	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 0x19);
> 
> Don't use 0x19, use 25 since we use 25 in the gen6 path

Ok.

> > +
> > +	for_each_ring(ring, dev_priv, i)
> > +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> > +
> > +	I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
> > +	I915_WRITE(GEN6_RC6_THRESHOLD, 0xc350);
> > +
> > +	I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
> > +	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
> > +	I915_WRITE(GEN6_RP_UP_EI, 66000);
> > +	I915_WRITE(GEN6_RP_DOWN_EI, 350000);
> > +
> > +	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> 
> I can't find most of the above values, but, I'll assume they're correct.
> I'd also suggest converging on either all decimal, or all hex, just to
> make things less confusing.

A comment about units probably wouldn't hurt either.  Ok.

> > +
> > +	I915_WRITE(GEN6_RP_CONTROL,
> > +		   GEN6_RP_MEDIA_TURBO |
> > +		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
> > +		   GEN6_RP_MEDIA_IS_GFX |
> > +		   GEN6_RP_ENABLE |
> > +		   GEN6_RP_UP_BUSY_AVG |
> > +		   GEN6_RP_DOWN_IDLE_CONT);
> > +
> 
> > +	/* allows RC6 residency counter to work */
> > +	I915_WRITE(0x138104, 0xffff00ff);
> This is writing read only registers. Should be:
> I915_WRITE(0x138104, 0x800000ff);
> 
> Also, just for though, we may want to use the upper bits instead of the
> lower ones...

I think the upper bits are a mask, and all the low 8 bits are
writeable, so maybe 0x00ff00ff?  I'll have to test that.

> 
> > +	I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE);
> > +
> 
> So the following stuff doesn't seem to jive in the docs I'm reading. Can
> you double check your docs, and the silicon. I tried to punt this to IRC
> but you didn't respond. These IOSF registers are all 32 bits, so I'm not
> sure how this is supposed to work.
> 
> > +	valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS1, &val);
> > +	DRM_DEBUG_DRIVER("max GPU freq: %d\n", val);
> > +	dev_priv->rps.max_delay = val;
> val >> 16 && 0xff?
> 
> 
> > +
> > +	valleyview_punit_read(dev_priv, PUNIT_REG_GPU_LFM, &val);
> > +	DRM_DEBUG_DRIVER("min GPU freq: %d\n", val);
> > +	dev_priv->rps.min_delay = val;
> val & 0xff?
> > +
> > +	valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS2, &val);
> > +	DRM_DEBUG_DRIVER("max GPLL freq: %d\n", val);
> 
> One doc suggests this is actually offset 0x87, bits 7:0

Hm I'll check these out; the different docs talk about different bit
fields, but I think they're offsets into the fuse bus array.

> 
> > +
> > +	valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val);
> > +	DRM_DEBUG_DRIVER("DDR speed: ");
> > +	if (drm_debug & DRM_UT_DRIVER) {
> > +		if (((val >> 6) & 3) == 0) {
> > +			dev_priv->mem_freq = 800;
> > +			printk("800 MHz\n");
> > +		} else if (((val >> 6) & 3) == 1) {
> > +			printk("1066 MHz\n");
> > +			dev_priv->mem_freq = 1066;
> > +		} else if (((val >> 6) & 3) == 2) {
> > +			printk("1333 MHz\n");
> > +			dev_priv->mem_freq = 1333;
> > +		} else if (((val >> 6) & 3) == 3)
> > +			printk("invalid\n");
> > +	}
> 
> printk?
> could be one line, but would print 1332:
> dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3))

Yeah because the DRM_DEBUG above doesn't have a newline.  I like your
calculation better.

Thanks for checking these out; I know wading through these docs is no
fun.

-- 
Jesse Barnes, Intel Open Source Technology Center