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

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Mar 19, 2013 at 03:35:55PM -0700, Jesse Barnes wrote:
> On Tue, 19 Mar 2013 15:27:36 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > On Fri, Mar 08, 2013 at 10:45:58AM -0800, Jesse Barnes wrote:
> > > From: Ben Widawsky <ben@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@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.

I don't see how it could unless there are extra data/address/command
registers for simultaneous access. Anyway, I don't think you need to
change anything, just something we need to notify the other people
writing code about. Also something we need to keep in mind if things
mysteriously blow up at some point.

> 
> > > +
> > > +	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 agree, but I wasn't referring tot his part of the hunk, and you
snipped the part I was referring to, so adding it back:

+       valleyview_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
+
+       do {
+               valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval);
+               if (time_after(jiffies, timeout)) {
+                       DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
+                       break;
+               }
+               udelay(10);
+       } while (pval & 1);

This is what seems unnecessary to me.

> 
> > 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.
> 

It won't effect my adding the r-b, just that I noticed it because it
would have made review a bit easier ;-)

> > > +
> > > +	/* 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.

I was just confused because you set GEN6_RC1e_THRESHOLD below. Is that
one valid?

> 
> > > +	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.
> 

Sent a separate email already. The value I suggested should be
0x80ff00ff I believe.

> > 
> > > +	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.

Just double check it on silicon and it's fine with me.

> 
> > 
> > > +
> > > +	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.

Given you have, or will address all my concerns. Feel free to add
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

with the fixed patch.

> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Ben Widawsky, Intel Open Source Technology Center