[05/12] drm/i915: Cache current cdclk frequency in dev_priv

Commit Message

Mika Kahola April 16, 2015, 7:40 a.m. UTC

From: Ville Syrjälä <[e-mail]>

Rather that extracting the current cdclk freuqncy every time someone
wants to know it, cache the current value and use that. VLV/CHV already
stored a cached value there so just expand that to cover all platforms.

Signed-off-by: Ville Syrjälä <[e-mail]>

V2: Rebased to the latest

Signed-off-by: Mika Kahola <[e-mail]>

Author:    Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++-------------
 2 files changed, 27 insertions(+), 15 deletions(-)

Comments

Chris Wilson April 16, 2015, 8:02 a.m. UTC | #1

On Thu, Apr 16, 2015 at 10:40:51AM +0300, Mika Kahola wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> Rather that extracting the current cdclk freuqncy every time someone
> wants to know it, cache the current value and use that. VLV/CHV already
> stored a cached value there so just expand that to cover all platforms.
> 
> Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> V2: Rebased to the latest
> 
> Signed-off-by: Mika Kahola <[e-mail]>

Which debugfs file shows this? Do we have one for the similar clock freqs?
-Chris

Mika Kahola April 16, 2015, 10:51 a.m. UTC | #2

On Thu, 2015-04-16 at 09:02 +0100, Chris Wilson wrote:
> On Thu, Apr 16, 2015 at 10:40:51AM +0300, Mika Kahola wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > Rather that extracting the current cdclk freuqncy every time someone
> > wants to know it, cache the current value and use that. VLV/CHV already
> > stored a cached value there so just expand that to cover all platforms.
> > 
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > 
> > V2: Rebased to the latest
> > 
> > Signed-off-by: Mika Kahola <[e-mail]>
> 
> Which debugfs file shows this? Do we have one for the similar clock freqs?
> -Chris
> 

I don't think we have one for cdclk. Then again, should we add this to
debugfs?

-Mika-

Chris Wilson April 16, 2015, 11:05 a.m. UTC | #3

On Thu, Apr 16, 2015 at 01:51:43PM +0300, Mika Kahola wrote:
> On Thu, 2015-04-16 at 09:02 +0100, Chris Wilson wrote:
> > On Thu, Apr 16, 2015 at 10:40:51AM +0300, Mika Kahola wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > Rather that extracting the current cdclk freuqncy every time someone
> > > wants to know it, cache the current value and use that. VLV/CHV already
> > > stored a cached value there so just expand that to cover all platforms.
> > > 
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > 
> > > V2: Rebased to the latest
> > > 
> > > Signed-off-by: Mika Kahola <[e-mail]>
> > 
> > Which debugfs file shows this? Do we have one for the similar clock freqs?
> > -Chris
> > 
> 
> I don't think we have one for cdclk. Then again, should we add this to
> debugfs?

Is it useful to know? Yes, it people have added it to userspace tools.
It it derived, or variable state that we may get wrong, or are other
calculations dependent on it such that we will want to know its value to
check the results? Yes.
-Chris

Ville Syrjälä April 16, 2015, 11:20 a.m. UTC | #4

On Thu, Apr 16, 2015 at 12:05:12PM +0100, Chris Wilson wrote:
> On Thu, Apr 16, 2015 at 01:51:43PM +0300, Mika Kahola wrote:
> > On Thu, 2015-04-16 at 09:02 +0100, Chris Wilson wrote:
> > > On Thu, Apr 16, 2015 at 10:40:51AM +0300, Mika Kahola wrote:
> > > > From: Ville Syrjälä <[e-mail]>
> > > > 
> > > > Rather that extracting the current cdclk freuqncy every time someone
> > > > wants to know it, cache the current value and use that. VLV/CHV already
> > > > stored a cached value there so just expand that to cover all platforms.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > 
> > > > V2: Rebased to the latest
> > > > 
> > > > Signed-off-by: Mika Kahola <[e-mail]>
> > > 
> > > Which debugfs file shows this? Do we have one for the similar clock freqs?
> > > -Chris
> > > 
> > 
> > I don't think we have one for cdclk. Then again, should we add this to
> > debugfs?
> 
> Is it useful to know? Yes, it people have added it to userspace tools.
> It it derived, or variable state that we may get wrong, or are other
> calculations dependent on it such that we will want to know its value to
> check the results? Yes.

Such a file should probably show the pch/hrawclk, czclk, and perhaps
memory clock too (or maybe we have that somewhere already?).

I've had plans to clean up the rawclk stuff too to resemble the cdclk
handling a bit more. The rawclk shouldn't change at runtime though,
so it should be mostly a search and replaec operation. Though we might
want to extract it for real on VLV/CHV from CCK rather than just
assuming the 100MHz (which it will be though). Similar thing could be
done for czclk as well.

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 89231ae..baec8d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1657,7 +1657,7 @@  struct drm_i915_private {
 	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
 
 	unsigned int fsb_freq, mem_freq, is_ddr3;
-	unsigned int vlv_cdclk_freq;
+	unsigned int cdclk_freq;
 	unsigned int hpll_freq;
 
 	/**
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 20aa288..3204e5e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5210,20 +5210,27 @@  static int valleyview_get_vco(struct drm_i915_private *dev_priv)
 	return vco_freq[hpll_freq] * 1000;
 }
 
-static void vlv_update_cdclk(struct drm_device *dev)
+static void intel_update_cdclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	dev_priv->vlv_cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
+	dev_priv->cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
 	DRM_DEBUG_DRIVER("Current CD clock rate: %d kHz\n",
-			 dev_priv->vlv_cdclk_freq);
+			 dev_priv->cdclk_freq);
 
 	/*
 	 * Program the gmbus_freq based on the cdclk frequency.
 	 * BSpec erroneously claims we should aim for 4MHz, but
 	 * in fact 1MHz is the correct frequency.
 	 */
-	I915_WRITE(GMBUSFREQ_VLV, DIV_ROUND_UP(dev_priv->vlv_cdclk_freq, 1000));
+	if (IS_VALLEYVIEW(dev)) {
+		/*
+		 * Program the gmbus_freq based on the cdclk frequency.
+		 * BSpec erroneously claims we should aim for 4MHz, but
+		 * in fact 1MHz is the correct frequency.
+		 */
+		I915_WRITE(GMBUSFREQ_VLV, DIV_ROUND_UP(dev_priv->cdclk_freq, 1000));
+	}
 }
 
 /* Adjust CDclk dividers to allow high res or save power if possible */
@@ -5232,7 +5239,7 @@  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 val, cmd;
 
-	WARN_ON(dev_priv->display.get_display_clock_speed(dev) != dev_priv->vlv_cdclk_freq);
+	WARN_ON(dev_priv->display.get_display_clock_speed(dev) != dev_priv->cdclk_freq);
 
 	if (cdclk >= 320000) /* jump to highest voltage for 400MHz too */
 		cmd = 2;
@@ -5288,7 +5295,7 @@  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 	vlv_bunit_write(dev_priv, BUNIT_REG_BISOC, val);
 	mutex_unlock(&dev_priv->dpio_lock);
 
-	vlv_update_cdclk(dev);
+	intel_update_cdclk(dev);
 }
 
 static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
@@ -5296,7 +5303,7 @@  static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 val, cmd;
 
-	WARN_ON(dev_priv->display.get_display_clock_speed(dev) != dev_priv->vlv_cdclk_freq);
+	WARN_ON(dev_priv->display.get_display_clock_speed(dev) != dev_priv->cdclk_freq);
 
 	switch (cdclk) {
 	case 333333:
@@ -5328,7 +5335,7 @@  static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
-	vlv_update_cdclk(dev);
+	intel_update_cdclk(dev);
 }
 
 static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
@@ -5395,7 +5402,7 @@  static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
 		return max_pixclk;
 
 	if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
-	    dev_priv->vlv_cdclk_freq)
+	    dev_priv->cdclk_freq)
 		return 0;
 
 	/* disable/enable all currently active pipes while we change cdclk */
@@ -5415,7 +5422,7 @@  static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
 	else
 		default_credits = PFI_CREDIT(8);
 
-	if (DIV_ROUND_CLOSEST(dev_priv->vlv_cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
+	if (DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
 		/* CHV suggested value is 31 or 63 */
 		if (IS_CHERRYVIEW(dev_priv))
 			credits = PFI_CREDIT_31;
@@ -5459,7 +5466,7 @@  static void valleyview_modeset_global_resources(struct drm_atomic_state *state)
 
 	req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
 
-	if (req_cdclk != dev_priv->vlv_cdclk_freq) {
+	if (req_cdclk != dev_priv->cdclk_freq) {
 		/*
 		 * FIXME: We can end up here with all power domains off, yet
 		 * with a CDCLK frequency other than the minimum. To account
@@ -5480,6 +5487,8 @@  static void valleyview_modeset_global_resources(struct drm_atomic_state *state)
 
 		intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
 	}
+
+	intel_update_cdclk(dev_priv->dev);
 }
 
 static void valleyview_crtc_enable(struct drm_crtc *crtc)
@@ -8950,6 +8959,8 @@  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	}
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+	intel_update_cdclk(dev_priv->dev);
 }
 
 /*
@@ -12934,6 +12945,8 @@  static void intel_shared_dpll_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	intel_update_cdclk(dev);
+
 	if (HAS_DDI(dev))
 		intel_ddi_pll_init(dev);
 	else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
@@ -14424,10 +14437,9 @@  static void i915_disable_vga(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
-	intel_prepare_ddi(dev);
+	intel_update_cdclk(dev);
 
-	if (IS_VALLEYVIEW(dev))
-		vlv_update_cdclk(dev);
+	intel_prepare_ddi(dev);
 
 	intel_init_clock_gating(dev);