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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

This was a typo on my part, it should be 0x80ff00ff. Sorry about that. I
haven't read the rest of the email yet. It popped into my head while I
was out.

> 
> > 
> > > +	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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux