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

[Ben Widawsky <ben@bwidawsk.net>]

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.

Aside from that, I have quite a few things below which as long as you
address, I'll happily add an r-b.

> ---
>  drivers/gpu/drm/i915/i915_drv.h |    1 +
>  drivers/gpu/drm/i915/i915_irq.c |    5 +-
>  drivers/gpu/drm/i915/intel_pm.c |  150 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 151 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 592e944..34414d1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1820,6 +1820,7 @@ extern void intel_disable_fbc(struct drm_device *dev);
>  extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
>  extern void intel_init_pch_refclk(struct drm_device *dev);
>  extern void gen6_set_rps(struct drm_device *dev, u8 val);
> +extern void valleyview_set_rps(struct drm_device *dev, u8 val);
>  extern void intel_detect_pch(struct drm_device *dev);
>  extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
>  extern int intel_enable_rc6(const struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2139714..65120e1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -395,7 +395,10 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  	 */
>  	if (!(new_delay > dev_priv->rps.max_delay ||
>  	      new_delay < dev_priv->rps.min_delay)) {
> -		gen6_set_rps(dev_priv->dev, new_delay);
> +		if (IS_VALLEYVIEW(dev_priv->dev))
> +			valleyview_set_rps(dev_priv->dev, new_delay);
> +		else
> +			gen6_set_rps(dev_priv->dev, new_delay);
>  	}
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 70eab45..d0b8d58 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2477,6 +2477,47 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	trace_intel_gpu_freq_change(val * 50);
>  }
>  
> +void valleyview_set_rps(struct drm_device *dev, u8 val)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned long timeout = jiffies + msecs_to_jiffies(100);
> +	u32 limits = gen6_rps_limits(dev_priv, &val);
> +	u32 pval;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +	WARN_ON(val > dev_priv->rps.max_delay);
> +	WARN_ON(val < dev_priv->rps.min_delay);
> +
> +	if (val == dev_priv->rps.cur_delay)
> +		return;
> +
> +	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);
> +
> +	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.

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.
> +
> +	/* 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.
> +}
> +
> +
>  static void gen6_disable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2714,6 +2755,102 @@ static void gen6_update_ring_freq(struct drm_device *dev)
>  	}
>  }
>  
> +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?
> +
> +	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?
> +	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
> +
> +	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.

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

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

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

> +	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 8 ? "yes" : "no");
> +	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);

The docs match on this one \o/

> +
> +	DRM_DEBUG_DRIVER("current GPU freq: %x\n", (val >> 8) & 0xff);
> +	dev_priv->rps.cur_delay = (val >> 8) & 0xff;

Both docs match here too.

> +
> +	val = 0xd500;
> +	DRM_DEBUG_DRIVER("setting GPU freq to %d\n", (val >> 8) & 0xff);
> +
> +	valleyview_set_rps(dev_priv->dev, (val >> 8) & 0xff);
> +
> +	/* requires MSI enabled */
> +	I915_WRITE(GEN6_PMIER, GEN6_PM_DEFERRED_EVENTS);
> +	spin_lock_irq(&dev_priv->rps.lock);
> +	WARN_ON(dev_priv->rps.pm_iir != 0);
> +	I915_WRITE(GEN6_PMIMR, 0);
> +	spin_unlock_irq(&dev_priv->rps.lock);
> +	/* enable all PM interrupts */
> +	I915_WRITE(GEN6_PMINTRMSK, 0);
> +
> +	gen6_gt_force_wake_put(dev_priv);
> +}
> +
>  void ironlake_teardown_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3440,7 +3577,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  	if (IS_IRONLAKE_M(dev)) {
>  		ironlake_disable_drps(dev);
>  		ironlake_disable_rc6(dev);
> -	} else if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev)) {
> +	} else if (INTEL_INFO(dev)->gen >= 6) {
>  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>  		mutex_lock(&dev_priv->rps.hw_lock);
>  		gen6_disable_rps(dev);
> @@ -3456,8 +3593,13 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  	struct drm_device *dev = dev_priv->dev;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> -	gen6_enable_rps(dev);
> -	gen6_update_ring_freq(dev);
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		valleyview_enable_rps(dev);
> +	} else {
> +		gen6_enable_rps(dev);
> +		gen6_update_ring_freq(dev);
> +	}
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -3469,7 +3611,7 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>  		ironlake_enable_drps(dev);
>  		ironlake_enable_rc6(dev);
>  		intel_init_emon(dev);
> -	} else if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> +	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
>  		/*
>  		 * PCU communication is slow and this doesn't need to be
>  		 * done at any specific time, so do this out of our fast path
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center