[PATCH 0/4] drm/i915: Read czclk from cck on vlv/chv

[PATCH 0/4] drm/i915: Read czclk from cck on vlv/chv

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This series replaces the czclk readout using a fuse on chv with reading
it directly from the horse's mouth ie. cck. It also adds czclk readout
for vlv, which means it also fixes the PFI credit programming on that
platform.

As a bonus we can simplify the gpu residency calculations quite a bit.

The whole series us available here:
git://github.com/vsyrjala/linux.git czclk_cck

Vandana Kannan (1):
  drm/i915: Renaming CCK related reg definitions

Ville Syrjälä (3):
  drm/i915: Read czclk from CCK on vlv/chv
  drm/i915: Simplify vlv/chv rc6 residency calculation
  drm/i915: Use czclk_freq in vlv c0 residency calculations

 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/i915_irq.c      |  8 +++-
 drivers/gpu/drm/i915/i915_reg.h      | 11 +++--
 drivers/gpu/drm/i915/i915_sysfs.c    | 31 ++-----------
 drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_pm.c      | 25 ++--------
 6 files changed, 74 insertions(+), 91 deletions(-)

-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Renaming CCK related reg definitions

[ville.syrjala]

From: Vandana Kannan <vandana.kannan@intel.com>

Rename the DISPLAY_TRUNK_* and DISPLAY_FREQUENCY_* bits to CCK_... instead
of DISPLAY_... to make it clear they apply to all CCK clock control registers.
Suggested by Ville.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      | 10 +++++-----
 drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 085ffa2..f22fb80 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -729,11 +729,11 @@ enum skl_disp_power_wells {
 #define  DSI_PLL_M1_DIV_SHIFT			0
 #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
 #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
-#define  DISPLAY_TRUNK_FORCE_ON			(1 << 17)
-#define  DISPLAY_TRUNK_FORCE_OFF		(1 << 16)
-#define  DISPLAY_FREQUENCY_STATUS		(0x1f << 8)
-#define  DISPLAY_FREQUENCY_STATUS_SHIFT		8
-#define  DISPLAY_FREQUENCY_VALUES		(0x1f << 0)
+#define  CCK_TRUNK_FORCE_ON			(1 << 17)
+#define  CCK_TRUNK_FORCE_OFF			(1 << 16)
+#define  CCK_FREQUENCY_STATUS			(0x1f << 8)
+#define  CCK_FREQUENCY_STATUS_SHIFT		8
+#define  CCK_FREQUENCY_VALUES			(0x1f << 0)
 
 /**
  * DOC: DPIO
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1847257..8567b46 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5800,12 +5800,12 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 
 		/* adjust cdclk divider */
 		val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
-		val &= ~DISPLAY_FREQUENCY_VALUES;
+		val &= ~CCK_FREQUENCY_VALUES;
 		val |= divider;
 		vlv_cck_write(dev_priv, CCK_DISPLAY_CLOCK_CONTROL, val);
 
 		if (wait_for((vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL) &
-			      DISPLAY_FREQUENCY_STATUS) == (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
+			      CCK_FREQUENCY_STATUS) == (divider << CCK_FREQUENCY_STATUS_SHIFT),
 			     50))
 			DRM_ERROR("timed out waiting for CDclk change\n");
 	}
@@ -6726,10 +6726,10 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
 	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
 	mutex_unlock(&dev_priv->sb_lock);
 
-	divider = val & DISPLAY_FREQUENCY_VALUES;
+	divider = val & CCK_FREQUENCY_VALUES;
 
-	WARN((val & DISPLAY_FREQUENCY_STATUS) !=
-	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
+	WARN((val & CCK_FREQUENCY_STATUS) !=
+	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
 	     "cdclk change in progress\n");
 
 	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Read czclk from CCK on vlv/chv

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

As with the cdclk, read out czclk from CCK as well. This gives us the
real current value and avoids having to decode fuses and whatnot.

Also store it in kHz under dev_priv like we do for cdlck since it's not
just an rps related clock, and having it in kHz is more
standard/convenient for some things.

Imre also pointed out that we currently fail to read czclk on VLV, which
means the PFI credit programming isn't working as expected.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/i915_reg.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_pm.c      | 25 ++--------
 4 files changed, 60 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2b5d587..77c9312 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1146,7 +1146,6 @@ struct intel_gen6_power_mgmt {
 	u8 efficient_freq;	/* AKA RPe. Pre-determined balanced frequency */
 	u8 rp1_freq;		/* "less than" RP0 power/freqency */
 	u8 rp0_freq;		/* Non-overclocked max frequency. */
-	u32 cz_freq;
 
 	u8 up_threshold; /* Current %busy required to uplock */
 	u8 down_threshold; /* Current %busy required to downclock */
@@ -1810,6 +1809,7 @@ struct drm_i915_private {
 	unsigned int cdclk_freq, max_cdclk_freq;
 	unsigned int max_dotclk_freq;
 	unsigned int hpll_freq;
+	unsigned int czclk_freq;
 
 	/**
 	 * wq - Driver workqueue for GEM.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f22fb80..83218032 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -728,6 +728,7 @@ enum skl_disp_power_wells {
 #define  DSI_PLL_N1_DIV_MASK			(3 << 16)
 #define  DSI_PLL_M1_DIV_SHIFT			0
 #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
+#define CCK_CZ_CLOCK_CONTROL			0x62
 #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
 #define  CCK_TRUNK_FORCE_ON			(1 << 17)
 #define  CCK_TRUNK_FORCE_OFF			(1 << 16)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8567b46..d668897 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -132,6 +132,42 @@ struct intel_limit {
 	intel_p2_t	    p2;
 };
 
+/* returns HPLL frequency in kHz */
+static int valleyview_get_vco(struct drm_i915_private *dev_priv)
+{
+	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
+
+	/* Obtain SKU information */
+	mutex_lock(&dev_priv->sb_lock);
+	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
+		CCK_FUSE_HPLL_FREQ_MASK;
+	mutex_unlock(&dev_priv->sb_lock);
+
+	return vco_freq[hpll_freq] * 1000;
+}
+
+static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
+				  const char *name, u32 reg)
+{
+	u32 val;
+	int divider;
+
+	if (dev_priv->hpll_freq == 0)
+		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
+
+	mutex_lock(&dev_priv->sb_lock);
+	val = vlv_cck_read(dev_priv, reg);
+	mutex_unlock(&dev_priv->sb_lock);
+
+	divider = val & CCK_FREQUENCY_VALUES;
+
+	WARN((val & CCK_FREQUENCY_STATUS) !=
+	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
+	     "%s change in progress\n", name);
+
+	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
+}
+
 int
 intel_pch_rawclk(struct drm_device *dev)
 {
@@ -175,6 +211,17 @@ int intel_hrawclk(struct drm_device *dev)
 	}
 }
 
+static void intel_update_czclk(struct drm_i915_private *dev_priv)
+{
+	if (!IS_VALLEYVIEW(dev_priv))
+		return;
+
+	dev_priv->czclk_freq = vlv_get_cck_clock_hpll(dev_priv, "czclk",
+						      CCK_CZ_CLOCK_CONTROL);
+
+	DRM_DEBUG_DRIVER("CZ clock rate: %d kHz\n", dev_priv->czclk_freq);
+}
+
 static inline u32 /* units of 100MHz */
 intel_fdi_link_freq(struct drm_device *dev)
 {
@@ -5749,20 +5796,6 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
 		DRM_ERROR("DBuf power enable timeout\n");
 }
 
-/* returns HPLL frequency in kHz */
-static int valleyview_get_vco(struct drm_i915_private *dev_priv)
-{
-	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
-
-	/* Obtain SKU information */
-	mutex_lock(&dev_priv->sb_lock);
-	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
-		CCK_FUSE_HPLL_FREQ_MASK;
-	mutex_unlock(&dev_priv->sb_lock);
-
-	return vco_freq[hpll_freq] * 1000;
-}
-
 /* Adjust CDclk dividers to allow high res or save power if possible */
 static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 {
@@ -5983,7 +6016,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->cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
+	if (dev_priv->cdclk_freq >= dev_priv->czclk_freq) {
 		/* CHV suggested value is 31 or 63 */
 		if (IS_CHERRYVIEW(dev_priv))
 			credits = PFI_CREDIT_63;
@@ -6715,24 +6748,8 @@ static int haswell_get_display_clock_speed(struct drm_device *dev)
 
 static int valleyview_get_display_clock_speed(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 val;
-	int divider;
-
-	if (dev_priv->hpll_freq == 0)
-		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
-
-	mutex_lock(&dev_priv->sb_lock);
-	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
-	mutex_unlock(&dev_priv->sb_lock);
-
-	divider = val & CCK_FREQUENCY_VALUES;
-
-	WARN((val & CCK_FREQUENCY_STATUS) !=
-	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
-	     "cdclk change in progress\n");
-
-	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
+	return vlv_get_cck_clock_hpll(to_i915(dev), "cdclk",
+				      CCK_DISPLAY_CLOCK_CONTROL);
 }
 
 static int ilk_get_display_clock_speed(struct drm_device *dev)
@@ -13323,8 +13340,6 @@ 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))
@@ -14836,6 +14851,9 @@ void intel_modeset_init(struct drm_device *dev)
 		}
 	}
 
+	intel_update_czclk(dev_priv);
+	intel_update_cdclk(dev);
+
 	intel_shared_dpll_init(dev);
 
 	/* Just disable it once at startup */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ab5ac5e..896a95c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5482,25 +5482,10 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
 	mutex_unlock(&dev_priv->sb_lock);
 
 	switch ((val >> 2) & 0x7) {
-	case 0:
-	case 1:
-		dev_priv->rps.cz_freq = 200;
-		dev_priv->mem_freq = 1600;
-		break;
-	case 2:
-		dev_priv->rps.cz_freq = 267;
-		dev_priv->mem_freq = 1600;
-		break;
 	case 3:
-		dev_priv->rps.cz_freq = 333;
 		dev_priv->mem_freq = 2000;
 		break;
-	case 4:
-		dev_priv->rps.cz_freq = 320;
-		dev_priv->mem_freq = 1600;
-		break;
-	case 5:
-		dev_priv->rps.cz_freq = 400;
+	default:
 		dev_priv->mem_freq = 1600;
 		break;
 	}
@@ -7327,7 +7312,7 @@ static int vlv_gpu_freq_div(unsigned int czclk_freq)
 
 static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
-	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
+	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
 
 	div = vlv_gpu_freq_div(czclk_freq);
 	if (div < 0)
@@ -7338,7 +7323,7 @@ static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 
 static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
 {
-	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
+	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
 
 	mul = vlv_gpu_freq_div(czclk_freq);
 	if (mul < 0)
@@ -7349,7 +7334,7 @@ static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
 
 static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
-	int div, czclk_freq = dev_priv->rps.cz_freq;
+	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
 
 	div = vlv_gpu_freq_div(czclk_freq) / 2;
 	if (div < 0)
@@ -7360,7 +7345,7 @@ static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
 
 static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
 {
-	int mul, czclk_freq = dev_priv->rps.cz_freq;
+	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
 
 	mul = vlv_gpu_freq_div(czclk_freq) / 2;
 	if (mul < 0)
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Simplify vlv/chv rc6 residency calculation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have the czclk freqeuency in dev_priv now, so let's just use it
when converting the rc6 counters to milliseconds. This elimiantes
a bunch of hairy code that essentially tries to extract the czclk
frequency using yet another method.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 55bd04c..74086eb 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -39,7 +39,7 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u64 raw_time; /* 32b value may overflow during fixed point math */
-	u64 units = 128ULL, div = 100000ULL, bias = 100ULL;
+	u64 units = 128ULL, div = 100000ULL;
 	u32 ret;
 
 	if (!intel_enable_rc6(dev))
@@ -49,41 +49,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
 
 	/* On VLV and CHV, residency time is in CZ units rather than 1.28us */
 	if (IS_VALLEYVIEW(dev)) {
-		u32 clk_reg, czcount_30ns;
-
-		if (IS_CHERRYVIEW(dev))
-			clk_reg = CHV_CLK_CTL1;
-		else
-			clk_reg = VLV_CLK_CTL2;
-
-		czcount_30ns = I915_READ(clk_reg) >> CLK_CTL2_CZCOUNT_30NS_SHIFT;
-
-		if (!czcount_30ns) {
-			WARN(!czcount_30ns, "bogus CZ count value");
-			ret = 0;
-			goto out;
-		}
-
-		if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) {
-			/* Special case for 320Mhz */
-			div = 10000000ULL;
-			units = 3125ULL;
-		} else {
-			czcount_30ns += 1;
-			div = 1000000ULL;
-			units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns);
-		}
+		units = 1;
+		div = dev_priv->czclk_freq;
 
 		if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
 			units <<= 8;
-
-		div = div * bias;
 	}
 
 	raw_time = I915_READ(reg) * units;
 	ret = DIV_ROUND_UP_ULL(raw_time, div);
 
-out:
 	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Use czclk_freq in vlv c0 residency calculations

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the use of mem_freq/4 with czclk_freq in the vlv c0 residency
calculations.

Also deal with VLV_COUNT_RANGE_HIGH which affects all RCx residency
counters. We have just enough bits to do this without intermediate
divisions.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 07c87e0..d78ef64 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -998,12 +998,16 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
 			 int threshold)
 {
 	u64 time, c0;
+	unsigned int mul = 100;
 
 	if (old->cz_clock == 0)
 		return false;
 
+	if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
+		mul <<= 8;
+
 	time = now->cz_clock - old->cz_clock;
-	time *= threshold * dev_priv->mem_freq;
+	time *= threshold * dev_priv->czclk_freq;
 
 	/* Workload can be split between render + media, e.g. SwapBuffers
 	 * being blitted in X after being rendered in mesa. To account for
@@ -1011,7 +1015,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
 	 */
 	c0 = now->render_c0 - old->render_c0;
 	c0 += now->media_c0 - old->media_c0;
-	c0 *= 100 * VLV_CZ_CLOCK_TO_MILLI_SEC * 4 / 1000;
+	c0 *= mul * VLV_CZ_CLOCK_TO_MILLI_SEC;
 
 	return c0 >= time;
 }
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Read czclk from CCK on vlv/chv

[Imre Deak]

On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> As with the cdclk, read out czclk from CCK as well. This gives us the
> real current value and avoids having to decode fuses and whatnot.
> 
> Also store it in kHz under dev_priv like we do for cdlck since it's not
> just an rps related clock, and having it in kHz is more
> standard/convenient for some things.
> 
> Imre also pointed out that we currently fail to read czclk on VLV, which
> means the PFI credit programming isn't working as expected.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yep, much better to have a generic way to read out all the CCK clocks,
removes a lot of special casing. It's also true that these clocks can
change, though let's note that czclk is fixed. Looks ok to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

Below some nitpicks about naming.

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_pm.c      | 25 ++--------
>  4 files changed, 60 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2b5d587..77c9312 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1146,7 +1146,6 @@ struct intel_gen6_power_mgmt {
>  	u8 efficient_freq;	/* AKA RPe. Pre-determined balanced frequency */
>  	u8 rp1_freq;		/* "less than" RP0 power/freqency */
>  	u8 rp0_freq;		/* Non-overclocked max frequency. */
> -	u32 cz_freq;
>  
>  	u8 up_threshold; /* Current %busy required to uplock */
>  	u8 down_threshold; /* Current %busy required to downclock */
> @@ -1810,6 +1809,7 @@ struct drm_i915_private {
>  	unsigned int cdclk_freq, max_cdclk_freq;
>  	unsigned int max_dotclk_freq;
>  	unsigned int hpll_freq;
> +	unsigned int czclk_freq;
>  
>  	/**
>  	 * wq - Driver workqueue for GEM.
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f22fb80..83218032 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -728,6 +728,7 @@ enum skl_disp_power_wells {
>  #define  DSI_PLL_N1_DIV_MASK			(3 << 16)
>  #define  DSI_PLL_M1_DIV_SHIFT			0
>  #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
> +#define CCK_CZ_CLOCK_CONTROL			0x62
>  #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
>  #define  CCK_TRUNK_FORCE_ON			(1 << 17)
>  #define  CCK_TRUNK_FORCE_OFF			(1 << 16)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8567b46..d668897 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -132,6 +132,42 @@ struct intel_limit {
>  	intel_p2_t	    p2;
>  };
>  
> +/* returns HPLL frequency in kHz */
> +static int valleyview_get_vco(struct drm_i915_private *dev_priv)

This wasn't added in this patch, but I would call it get_cck_ref or
similar. VCO is too generic and HPLL isn't accurate, since the 800MHz
clock is a bypass clock directly from iCLK.

> +{
> +	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
> +
> +	/* Obtain SKU information */
> +	mutex_lock(&dev_priv->sb_lock);
> +	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
> +		CCK_FUSE_HPLL_FREQ_MASK;
> +	mutex_unlock(&dev_priv->sb_lock);
> +
> +	return vco_freq[hpll_freq] * 1000;
> +}
> +
> +static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> +				  const char *name, u32 reg)

I would have called it simply get_cck_clock for the above reason.

> +{
> +	u32 val;
> +	int divider;
> +
> +	if (dev_priv->hpll_freq == 0)
> +		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> +
> +	mutex_lock(&dev_priv->sb_lock);
> +	val = vlv_cck_read(dev_priv, reg);
> +	mutex_unlock(&dev_priv->sb_lock);
> +
> +	divider = val & CCK_FREQUENCY_VALUES;
> +
> +	WARN((val & CCK_FREQUENCY_STATUS) !=
> +	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
> +	     "%s change in progress\n", name);
> +
> +	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> +}
> +
>  int
>  intel_pch_rawclk(struct drm_device *dev)
>  {
> @@ -175,6 +211,17 @@ int intel_hrawclk(struct drm_device *dev)
>  	}
>  }
>  
> +static void intel_update_czclk(struct drm_i915_private *dev_priv)
> +{
> +	if (!IS_VALLEYVIEW(dev_priv))
> +		return;
> +
> +	dev_priv->czclk_freq = vlv_get_cck_clock_hpll(dev_priv, "czclk",
> +						      CCK_CZ_CLOCK_CONTROL);
> +
> +	DRM_DEBUG_DRIVER("CZ clock rate: %d kHz\n", dev_priv->czclk_freq);
> +}
> +
>  static inline u32 /* units of 100MHz */
>  intel_fdi_link_freq(struct drm_device *dev)
>  {
> @@ -5749,20 +5796,6 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  		DRM_ERROR("DBuf power enable timeout\n");
>  }
>  
> -/* returns HPLL frequency in kHz */
> -static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> -{
> -	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
> -
> -	/* Obtain SKU information */
> -	mutex_lock(&dev_priv->sb_lock);
> -	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
> -		CCK_FUSE_HPLL_FREQ_MASK;
> -	mutex_unlock(&dev_priv->sb_lock);
> -
> -	return vco_freq[hpll_freq] * 1000;
> -}
> -
>  /* Adjust CDclk dividers to allow high res or save power if possible */
>  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  {
> @@ -5983,7 +6016,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->cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
> +	if (dev_priv->cdclk_freq >= dev_priv->czclk_freq) {
>  		/* CHV suggested value is 31 or 63 */
>  		if (IS_CHERRYVIEW(dev_priv))
>  			credits = PFI_CREDIT_63;
> @@ -6715,24 +6748,8 @@ static int haswell_get_display_clock_speed(struct drm_device *dev)
>  
>  static int valleyview_get_display_clock_speed(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 val;
> -	int divider;
> -
> -	if (dev_priv->hpll_freq == 0)
> -		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> -
> -	mutex_lock(&dev_priv->sb_lock);
> -	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
> -	mutex_unlock(&dev_priv->sb_lock);
> -
> -	divider = val & CCK_FREQUENCY_VALUES;
> -
> -	WARN((val & CCK_FREQUENCY_STATUS) !=
> -	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
> -	     "cdclk change in progress\n");
> -
> -	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> +	return vlv_get_cck_clock_hpll(to_i915(dev), "cdclk",
> +				      CCK_DISPLAY_CLOCK_CONTROL);
>  }
>  
>  static int ilk_get_display_clock_speed(struct drm_device *dev)
> @@ -13323,8 +13340,6 @@ 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))
> @@ -14836,6 +14851,9 @@ void intel_modeset_init(struct drm_device *dev)
>  		}
>  	}
>  
> +	intel_update_czclk(dev_priv);
> +	intel_update_cdclk(dev);
> +
>  	intel_shared_dpll_init(dev);
>  
>  	/* Just disable it once at startup */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ab5ac5e..896a95c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5482,25 +5482,10 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
>  	mutex_unlock(&dev_priv->sb_lock);
>  
>  	switch ((val >> 2) & 0x7) {
> -	case 0:
> -	case 1:
> -		dev_priv->rps.cz_freq = 200;
> -		dev_priv->mem_freq = 1600;
> -		break;
> -	case 2:
> -		dev_priv->rps.cz_freq = 267;
> -		dev_priv->mem_freq = 1600;
> -		break;
>  	case 3:
> -		dev_priv->rps.cz_freq = 333;
>  		dev_priv->mem_freq = 2000;
>  		break;
> -	case 4:
> -		dev_priv->rps.cz_freq = 320;
> -		dev_priv->mem_freq = 1600;
> -		break;
> -	case 5:
> -		dev_priv->rps.cz_freq = 400;
> +	default:
>  		dev_priv->mem_freq = 1600;
>  		break;
>  	}
> @@ -7327,7 +7312,7 @@ static int vlv_gpu_freq_div(unsigned int czclk_freq)
>  
>  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
> -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
> +	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
>  
>  	div = vlv_gpu_freq_div(czclk_freq);
>  	if (div < 0)
> @@ -7338,7 +7323,7 @@ static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  
>  static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  {
> -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
> +	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
>  
>  	mul = vlv_gpu_freq_div(czclk_freq);
>  	if (mul < 0)
> @@ -7349,7 +7334,7 @@ static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  
>  static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
> -	int div, czclk_freq = dev_priv->rps.cz_freq;
> +	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
>  
>  	div = vlv_gpu_freq_div(czclk_freq) / 2;
>  	if (div < 0)
> @@ -7360,7 +7345,7 @@ static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  
>  static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  {
> -	int mul, czclk_freq = dev_priv->rps.cz_freq;
> +	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
>  
>  	mul = vlv_gpu_freq_div(czclk_freq) / 2;
>  	if (mul < 0)


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Simplify vlv/chv rc6 residency calculation

[Imre Deak]

On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We have the czclk freqeuency in dev_priv now, so let's just use it
                   ^frequency
> when converting the rc6 counters to milliseconds. This elimiantes
                                                        ^eliminates
> a bunch of hairy code that essentially tries to extract the czclk
> frequency using yet another method.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 31 +++----------------------------
>  1 file changed, 3 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 55bd04c..74086eb 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -39,7 +39,7 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u64 raw_time; /* 32b value may overflow during fixed point math */
> -	u64 units = 128ULL, div = 100000ULL, bias = 100ULL;
> +	u64 units = 128ULL, div = 100000ULL;
>  	u32 ret;
>  
>  	if (!intel_enable_rc6(dev))
> @@ -49,41 +49,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>  
>  	/* On VLV and CHV, residency time is in CZ units rather than 1.28us */
>  	if (IS_VALLEYVIEW(dev)) {
> -		u32 clk_reg, czcount_30ns;
> -
> -		if (IS_CHERRYVIEW(dev))
> -			clk_reg = CHV_CLK_CTL1;
> -		else
> -			clk_reg = VLV_CLK_CTL2;
> -
> -		czcount_30ns = I915_READ(clk_reg) >> CLK_CTL2_CZCOUNT_30NS_SHIFT;
> -
> -		if (!czcount_30ns) {
> -			WARN(!czcount_30ns, "bogus CZ count value");
> -			ret = 0;
> -			goto out;
> -		}
> -
> -		if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) {
> -			/* Special case for 320Mhz */
> -			div = 10000000ULL;
> -			units = 3125ULL;
> -		} else {
> -			czcount_30ns += 1;
> -			div = 1000000ULL;
> -			units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns);
> -		}
> +		units = 1;
> +		div = dev_priv->czclk_freq;
>  
>  		if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
>  			units <<= 8;
> -
> -		div = div * bias;
>  	}
>  
>  	raw_time = I915_READ(reg) * units;
>  	ret = DIV_ROUND_UP_ULL(raw_time, div);
>  
> -out:
>  	intel_runtime_pm_put(dev_priv);
>  	return ret;
>  }


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Read czclk from CCK on vlv/chv

[Ville Syrjälä]

On Mon, Sep 28, 2015 at 10:31:49PM +0300, Imre Deak wrote:
> On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > As with the cdclk, read out czclk from CCK as well. This gives us the
> > real current value and avoids having to decode fuses and whatnot.
> > 
> > Also store it in kHz under dev_priv like we do for cdlck since it's not
> > just an rps related clock, and having it in kHz is more
> > standard/convenient for some things.
> > 
> > Imre also pointed out that we currently fail to read czclk on VLV, which
> > means the PFI credit programming isn't working as expected.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Yep, much better to have a generic way to read out all the CCK clocks,
> removes a lot of special casing. It's also true that these clocks can
> change, though let's note that czclk is fixed. Looks ok to me:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> Below some nitpicks about naming.
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> >  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++--------------
> >  drivers/gpu/drm/i915/intel_pm.c      | 25 ++--------
> >  4 files changed, 60 insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2b5d587..77c9312 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1146,7 +1146,6 @@ struct intel_gen6_power_mgmt {
> >  	u8 efficient_freq;	/* AKA RPe. Pre-determined balanced frequency */
> >  	u8 rp1_freq;		/* "less than" RP0 power/freqency */
> >  	u8 rp0_freq;		/* Non-overclocked max frequency. */
> > -	u32 cz_freq;
> >  
> >  	u8 up_threshold; /* Current %busy required to uplock */
> >  	u8 down_threshold; /* Current %busy required to downclock */
> > @@ -1810,6 +1809,7 @@ struct drm_i915_private {
> >  	unsigned int cdclk_freq, max_cdclk_freq;
> >  	unsigned int max_dotclk_freq;
> >  	unsigned int hpll_freq;
> > +	unsigned int czclk_freq;
> >  
> >  	/**
> >  	 * wq - Driver workqueue for GEM.
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f22fb80..83218032 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -728,6 +728,7 @@ enum skl_disp_power_wells {
> >  #define  DSI_PLL_N1_DIV_MASK			(3 << 16)
> >  #define  DSI_PLL_M1_DIV_SHIFT			0
> >  #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
> > +#define CCK_CZ_CLOCK_CONTROL			0x62
> >  #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
> >  #define  CCK_TRUNK_FORCE_ON			(1 << 17)
> >  #define  CCK_TRUNK_FORCE_OFF			(1 << 16)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8567b46..d668897 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -132,6 +132,42 @@ struct intel_limit {
> >  	intel_p2_t	    p2;
> >  };
> >  
> > +/* returns HPLL frequency in kHz */
> > +static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> 
> This wasn't added in this patch, but I would call it get_cck_ref or
> similar. VCO is too generic and HPLL isn't accurate, since the 800MHz
> clock is a bypass clock directly from iCLK.

800Mhz on VLV and 1600MHz on CHV I believe. I'm not sure the bypass is
ever used though. CHV clock HAS has a note that the bypass option had
a lot of jitter, so that that may be the reason for not using it. VLV
north clock HAS doesn't have such a note, but it does go on to say that
the plan was to use HPLL (except maybe with SKU200). Also there's
another note in the VLV docs saying HPLL bypass+GPLL is a debug only
configuration, and we always use GPLL so that would imply we always use
HPLL too.

Looking at the fuses on a few BYT and BSW machines, they both use the
HPLL @ 1600MHz. The BYT is SKU266 and BSW is SKU320, so in theory I
think both could use the bypass clock but neither does.

But yeah, the current function name isn't all that good. I think
vlv_get_hpll_freq() or something would be OK, since even the bypass
clock is still called "HPLL bypass clock". And the way it's drawn in
the docs is often as a bigger HPLL block which contains both the HPLL
proper, the bypass path, and the mux to select between the two.

> 
> > +{
> > +	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
> > +
> > +	/* Obtain SKU information */
> > +	mutex_lock(&dev_priv->sb_lock);
> > +	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
> > +		CCK_FUSE_HPLL_FREQ_MASK;
> > +	mutex_unlock(&dev_priv->sb_lock);
> > +
> > +	return vco_freq[hpll_freq] * 1000;
> > +}
> > +
> > +static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> > +				  const char *name, u32 reg)
> 
> I would have called it simply get_cck_clock for the above reason.

I have some more patches sitting in branch that split this
into vlv_get_cck_clock() and vlv_get_cck_clock_hpll(), like so:

vlv_get_cck_clock(reg, ref)
{
	// do the math
}

vlv_get_cck_clock_hpll(reg)
{
	if (!hpll_freq)
		hpll_freq = valleyview_get_vco()
	return vlv_get_cck_clock(reg, hpll_freq);
}

Simply because in the normal case the HPLL output is the clock
that gets divider down. But there is at least one special case
(GPLL ref clock) where that isn't the case.

> 
> > +{
> > +	u32 val;
> > +	int divider;
> > +
> > +	if (dev_priv->hpll_freq == 0)
> > +		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> > +
> > +	mutex_lock(&dev_priv->sb_lock);
> > +	val = vlv_cck_read(dev_priv, reg);
> +	mutex_unlock(&dev_priv->sb_lock);
> > +
> > +	divider = val & CCK_FREQUENCY_VALUES;
> > +
> > +	WARN((val & CCK_FREQUENCY_STATUS) !=
> > +	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
> > +	     "%s change in progress\n", name);
> > +
> > +	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> > +}
> > +
> >  int
> >  intel_pch_rawclk(struct drm_device *dev)
> >  {
> > @@ -175,6 +211,17 @@ int intel_hrawclk(struct drm_device *dev)
> >  	}
> >  }
> >  
> > +static void intel_update_czclk(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!IS_VALLEYVIEW(dev_priv))
> > +		return;
> > +
> > +	dev_priv->czclk_freq = vlv_get_cck_clock_hpll(dev_priv, "czclk",
> > +						      CCK_CZ_CLOCK_CONTROL);
> > +
> > +	DRM_DEBUG_DRIVER("CZ clock rate: %d kHz\n", dev_priv->czclk_freq);
> > +}
> > +
> >  static inline u32 /* units of 100MHz */
> >  intel_fdi_link_freq(struct drm_device *dev)
> >  {
> > @@ -5749,20 +5796,6 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
> >  		DRM_ERROR("DBuf power enable timeout\n");
> >  }
> >  
> > -/* returns HPLL frequency in kHz */
> > -static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> > -{
> > -	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
> > -
> > -	/* Obtain SKU information */
> > -	mutex_lock(&dev_priv->sb_lock);
> > -	hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
> > -		CCK_FUSE_HPLL_FREQ_MASK;
> > -	mutex_unlock(&dev_priv->sb_lock);
> > -
> > -	return vco_freq[hpll_freq] * 1000;
> > -}
> > -
> >  /* Adjust CDclk dividers to allow high res or save power if possible */
> >  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >  {
> > @@ -5983,7 +6016,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->cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
> > +	if (dev_priv->cdclk_freq >= dev_priv->czclk_freq) {
> >  		/* CHV suggested value is 31 or 63 */
> >  		if (IS_CHERRYVIEW(dev_priv))
> >  			credits = PFI_CREDIT_63;
> > @@ -6715,24 +6748,8 @@ static int haswell_get_display_clock_speed(struct drm_device *dev)
> >  
> >  static int valleyview_get_display_clock_speed(struct drm_device *dev)
> >  {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	u32 val;
> > -	int divider;
> > -
> > -	if (dev_priv->hpll_freq == 0)
> > -		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> > -
> > -	mutex_lock(&dev_priv->sb_lock);
> > -	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
> > -	mutex_unlock(&dev_priv->sb_lock);
> > -
> > -	divider = val & CCK_FREQUENCY_VALUES;
> > -
> > -	WARN((val & CCK_FREQUENCY_STATUS) !=
> > -	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
> > -	     "cdclk change in progress\n");
> > -
> > -	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> > +	return vlv_get_cck_clock_hpll(to_i915(dev), "cdclk",
> > +				      CCK_DISPLAY_CLOCK_CONTROL);
> >  }
> >  
> >  static int ilk_get_display_clock_speed(struct drm_device *dev)
> > @@ -13323,8 +13340,6 @@ 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))
> > @@ -14836,6 +14851,9 @@ void intel_modeset_init(struct drm_device *dev)
> >  		}
> >  	}
> >  
> > +	intel_update_czclk(dev_priv);
> > +	intel_update_cdclk(dev);
> > +
> >  	intel_shared_dpll_init(dev);
> >  
> >  	/* Just disable it once at startup */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ab5ac5e..896a95c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5482,25 +5482,10 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
> >  	mutex_unlock(&dev_priv->sb_lock);
> >  
> >  	switch ((val >> 2) & 0x7) {
> > -	case 0:
> > -	case 1:
> > -		dev_priv->rps.cz_freq = 200;
> > -		dev_priv->mem_freq = 1600;
> > -		break;
> > -	case 2:
> > -		dev_priv->rps.cz_freq = 267;
> > -		dev_priv->mem_freq = 1600;
> > -		break;
> >  	case 3:
> > -		dev_priv->rps.cz_freq = 333;
> >  		dev_priv->mem_freq = 2000;
> >  		break;
> > -	case 4:
> > -		dev_priv->rps.cz_freq = 320;
> > -		dev_priv->mem_freq = 1600;
> > -		break;
> > -	case 5:
> > -		dev_priv->rps.cz_freq = 400;
> > +	default:
> >  		dev_priv->mem_freq = 1600;
> >  		break;
> >  	}
> > @@ -7327,7 +7312,7 @@ static int vlv_gpu_freq_div(unsigned int czclk_freq)
> >  
> >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
> > +	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >  
> >  	div = vlv_gpu_freq_div(czclk_freq);
> >  	if (div < 0)
> > @@ -7338,7 +7323,7 @@ static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  
> >  static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
> > +	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >  
> >  	mul = vlv_gpu_freq_div(czclk_freq);
> >  	if (mul < 0)
> > @@ -7349,7 +7334,7 @@ static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
> >  
> >  static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int div, czclk_freq = dev_priv->rps.cz_freq;
> > +	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >  
> >  	div = vlv_gpu_freq_div(czclk_freq) / 2;
> >  	if (div < 0)
> > @@ -7360,7 +7345,7 @@ static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  
> >  static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int mul, czclk_freq = dev_priv->rps.cz_freq;
> > +	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >  
> >  	mul = vlv_gpu_freq_div(czclk_freq) / 2;
> >  	if (mul < 0)
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Simplify vlv/chv rc6 residency calculation

[Ville Syrjälä]

On Mon, Sep 28, 2015 at 10:37:52PM +0300, Imre Deak wrote:
> On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We have the czclk freqeuency in dev_priv now, so let's just use it
>                    ^frequency
> > when converting the rc6 counters to milliseconds. This elimiantes
>                                                         ^eliminates

Someone needs to integrate a proper spell checker into checkpatch :)

> > a bunch of hairy code that essentially tries to extract the czclk
> > frequency using yet another method.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/i915_sysfs.c | 31 +++----------------------------
> >  1 file changed, 3 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 55bd04c..74086eb 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -39,7 +39,7 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	u64 raw_time; /* 32b value may overflow during fixed point math */
> > -	u64 units = 128ULL, div = 100000ULL, bias = 100ULL;
> > +	u64 units = 128ULL, div = 100000ULL;
> >  	u32 ret;
> >  
> >  	if (!intel_enable_rc6(dev))
> > @@ -49,41 +49,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
> >  
> >  	/* On VLV and CHV, residency time is in CZ units rather than 1.28us */
> >  	if (IS_VALLEYVIEW(dev)) {
> > -		u32 clk_reg, czcount_30ns;
> > -
> > -		if (IS_CHERRYVIEW(dev))
> > -			clk_reg = CHV_CLK_CTL1;
> > -		else
> > -			clk_reg = VLV_CLK_CTL2;
> > -
> > -		czcount_30ns = I915_READ(clk_reg) >> CLK_CTL2_CZCOUNT_30NS_SHIFT;
> > -
> > -		if (!czcount_30ns) {
> > -			WARN(!czcount_30ns, "bogus CZ count value");
> > -			ret = 0;
> > -			goto out;
> > -		}
> > -
> > -		if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) {
> > -			/* Special case for 320Mhz */
> > -			div = 10000000ULL;
> > -			units = 3125ULL;
> > -		} else {
> > -			czcount_30ns += 1;
> > -			div = 1000000ULL;
> > -			units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns);
> > -		}
> > +		units = 1;
> > +		div = dev_priv->czclk_freq;
> >  
> >  		if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
> >  			units <<= 8;
> > -
> > -		div = div * bias;
> >  	}
> >  
> >  	raw_time = I915_READ(reg) * units;
> >  	ret = DIV_ROUND_UP_ULL(raw_time, div);
> >  
> > -out:
> >  	intel_runtime_pm_put(dev_priv);
> >  	return ret;
> >  }
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/4] drm/i915: Simplify vlv/chv rc6 residency calculation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have the czclk frequency in dev_priv now, so let's just use it
when converting the rc6 counters to milliseconds. This eliminates
a bunch of hairy code that essentially tries to extract the czclk
frequency using yet another method.

v2: Fix typos in commit message (Imre)

Reviewed-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 55bd04c..74086eb 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -39,7 +39,7 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u64 raw_time; /* 32b value may overflow during fixed point math */
-	u64 units = 128ULL, div = 100000ULL, bias = 100ULL;
+	u64 units = 128ULL, div = 100000ULL;
 	u32 ret;
 
 	if (!intel_enable_rc6(dev))
@@ -49,41 +49,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
 
 	/* On VLV and CHV, residency time is in CZ units rather than 1.28us */
 	if (IS_VALLEYVIEW(dev)) {
-		u32 clk_reg, czcount_30ns;
-
-		if (IS_CHERRYVIEW(dev))
-			clk_reg = CHV_CLK_CTL1;
-		else
-			clk_reg = VLV_CLK_CTL2;
-
-		czcount_30ns = I915_READ(clk_reg) >> CLK_CTL2_CZCOUNT_30NS_SHIFT;
-
-		if (!czcount_30ns) {
-			WARN(!czcount_30ns, "bogus CZ count value");
-			ret = 0;
-			goto out;
-		}
-
-		if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) {
-			/* Special case for 320Mhz */
-			div = 10000000ULL;
-			units = 3125ULL;
-		} else {
-			czcount_30ns += 1;
-			div = 1000000ULL;
-			units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns);
-		}
+		units = 1;
+		div = dev_priv->czclk_freq;
 
 		if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
 			units <<= 8;
-
-		div = div * bias;
 	}
 
 	raw_time = I915_READ(reg) * units;
 	ret = DIV_ROUND_UP_ULL(raw_time, div);
 
-out:
 	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Use czclk_freq in vlv c0 residency calculations

[Imre Deak]

On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the use of mem_freq/4 with czclk_freq in the vlv c0 residency
> calculations.
> 
> Also deal with VLV_COUNT_RANGE_HIGH which affects all RCx residency
> counters. We have just enough bits to do this without intermediate
> divisions.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 07c87e0..d78ef64 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -998,12 +998,16 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
>  			 int threshold)
>  {
>  	u64 time, c0;
> +	unsigned int mul = 100;
>  
>  	if (old->cz_clock == 0)
>  		return false;
>  
> +	if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
> +		mul <<= 8;

Could've been a separate patch.

> +
>  	time = now->cz_clock - old->cz_clock;
> -	time *= threshold * dev_priv->mem_freq;
> +	time *= threshold * dev_priv->czclk_freq;

Not introduced in this patch, but the above doesn't look correct to me.
Time is cycles _divided_ by frequency, so imo the above should be either
a division, or better we should calculate c0 (10ns) cycles here.

>  
>  	/* Workload can be split between render + media, e.g. SwapBuffers
>  	 * being blitted in X after being rendered in mesa. To account for
> @@ -1011,7 +1015,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
>  	 */
>  	c0 = now->render_c0 - old->render_c0;
>  	c0 += now->media_c0 - old->media_c0;
> -	c0 *= 100 * VLV_CZ_CLOCK_TO_MILLI_SEC * 4 / 1000;
> +	c0 *= mul * VLV_CZ_CLOCK_TO_MILLI_SEC;

Based on the above this would need to be fixed too.

The above can be done as a follow-up if needed; this patch does what it
says, so:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  
>  	return c0 >= time;
>  }


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Use czclk_freq in vlv c0 residency calculations

[Imre Deak]

On Mon, 2015-09-28 at 23:47 +0300, Imre Deak wrote:
> On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Replace the use of mem_freq/4 with czclk_freq in the vlv c0 residency
> > calculations.
> > 
> > Also deal with VLV_COUNT_RANGE_HIGH which affects all RCx residency
> > counters. We have just enough bits to do this without intermediate
> > divisions.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 07c87e0..d78ef64 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -998,12 +998,16 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> >  			 int threshold)
> >  {
> >  	u64 time, c0;
> > +	unsigned int mul = 100;
> >  
> >  	if (old->cz_clock == 0)
> >  		return false;
> >  
> > +	if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
> > +		mul <<= 8;
> 
> Could've been a separate patch.
> 
> > +
> >  	time = now->cz_clock - old->cz_clock;
> > -	time *= threshold * dev_priv->mem_freq;
> > +	time *= threshold * dev_priv->czclk_freq;
> 
> Not introduced in this patch, but the above doesn't look correct to me.
> Time is cycles _divided_ by frequency, so imo the above should be either
> a division, or better we should calculate c0 (10ns) cycles here.
> 
> >  
> >  	/* Workload can be split between render + media, e.g. SwapBuffers
> >  	 * being blitted in X after being rendered in mesa. To account for
> > @@ -1011,7 +1015,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> >  	 */
> >  	c0 = now->render_c0 - old->render_c0;
> >  	c0 += now->media_c0 - old->media_c0;
> > -	c0 *= 100 * VLV_CZ_CLOCK_TO_MILLI_SEC * 4 / 1000;
> > +	c0 *= mul * VLV_CZ_CLOCK_TO_MILLI_SEC;
> 
> Based on the above this would need to be fixed too.

Nvm the above, I realized now how it works:) I was confused seeing that
we scale by czclk freq and the 10ns freq the "opposite" time value.
Sorry for the noise.

> The above can be done as a follow-up if needed; this patch does what it
> says, so:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  
> >  	return c0 >= time;
> >  }
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Use czclk_freq in vlv c0 residency calculations

[Daniel Vetter]

Pulled in entire series, thanks.
-Daniel

On Tue, Sep 29, 2015 at 12:46:20AM +0300, Imre Deak wrote:
> On Mon, 2015-09-28 at 23:47 +0300, Imre Deak wrote:
> > On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Replace the use of mem_freq/4 with czclk_freq in the vlv c0 residency
> > > calculations.
> > > 
> > > Also deal with VLV_COUNT_RANGE_HIGH which affects all RCx residency
> > > counters. We have just enough bits to do this without intermediate
> > > divisions.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 07c87e0..d78ef64 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -998,12 +998,16 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> > >  			 int threshold)
> > >  {
> > >  	u64 time, c0;
> > > +	unsigned int mul = 100;
> > >  
> > >  	if (old->cz_clock == 0)
> > >  		return false;
> > >  
> > > +	if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
> > > +		mul <<= 8;
> > 
> > Could've been a separate patch.
> > 
> > > +
> > >  	time = now->cz_clock - old->cz_clock;
> > > -	time *= threshold * dev_priv->mem_freq;
> > > +	time *= threshold * dev_priv->czclk_freq;
> > 
> > Not introduced in this patch, but the above doesn't look correct to me.
> > Time is cycles _divided_ by frequency, so imo the above should be either
> > a division, or better we should calculate c0 (10ns) cycles here.
> > 
> > >  
> > >  	/* Workload can be split between render + media, e.g. SwapBuffers
> > >  	 * being blitted in X after being rendered in mesa. To account for
> > > @@ -1011,7 +1015,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> > >  	 */
> > >  	c0 = now->render_c0 - old->render_c0;
> > >  	c0 += now->media_c0 - old->media_c0;
> > > -	c0 *= 100 * VLV_CZ_CLOCK_TO_MILLI_SEC * 4 / 1000;
> > > +	c0 *= mul * VLV_CZ_CLOCK_TO_MILLI_SEC;
> > 
> > Based on the above this would need to be fixed too.
> 
> Nvm the above, I realized now how it works:) I was confused seeing that
> we scale by czclk freq and the 10ns freq the "opposite" time value.
> Sorry for the noise.
> 
> > The above can be done as a follow-up if needed; this patch does what it
> > says, so:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > >  
> > >  	return c0 >= time;
> > >  }
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Use czclk_freq in vlv c0 residency calculations

[Ville Syrjälä]

On Mon, Sep 28, 2015 at 11:47:15PM +0300, Imre Deak wrote:
> On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Replace the use of mem_freq/4 with czclk_freq in the vlv c0 residency
> > calculations.
> > 
> > Also deal with VLV_COUNT_RANGE_HIGH which affects all RCx residency
> > counters. We have just enough bits to do this without intermediate
> > divisions.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 07c87e0..d78ef64 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -998,12 +998,16 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> >  			 int threshold)
> >  {
> >  	u64 time, c0;
> > +	unsigned int mul = 100;
> >  
> >  	if (old->cz_clock == 0)
> >  		return false;
> >  
> > +	if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
> > +		mul <<= 8;
> 
> Could've been a separate patch.
> 
> > +
> >  	time = now->cz_clock - old->cz_clock;
> > -	time *= threshold * dev_priv->mem_freq;
> > +	time *= threshold * dev_priv->czclk_freq;
> 
> Not introduced in this patch, but the above doesn't look correct to me.
> Time is cycles _divided_ by frequency, so imo the above should be either
> a division, or better we should calculate c0 (10ns) cycles here.

I think it's correct. It's just moved the division over the to other
side. So what we want to check is:

threshold * (czts - czts_old)     mul * (c0 - c0_old)
----------------------------- <= --------------------
     cz_to_milli_sec                  czclk_freq

Or actually maybe better think it as 

            (czts - czts_old) * czclk_freq
threshold * ------------------------------  <= mul * (c0 - c0_old)
	           cz_to_milli_sec

The fact that the "cz" timestamp is not in cz clock units forces us to
do this silly conversion. I have no idea why Punit wants to give out the
timestamp in some normalized units. If it would instead give us the raw
cz clock timestamp we could just do
"threshold * (czts - czts_old) <= mul * (c0 - c0_old)"

So yeah, another case of the hardware (well, Punit firmware in this case
I suppose) being "helpful" :(

I think I even tried looking for a raw cz timestamp register so that we
could avoid this mess, but I couldn't find one.
 
> >  
> >  	/* Workload can be split between render + media, e.g. SwapBuffers
> >  	 * being blitted in X after being rendered in mesa. To account for
> > @@ -1011,7 +1015,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> >  	 */
> >  	c0 = now->render_c0 - old->render_c0;
> >  	c0 += now->media_c0 - old->media_c0;
> > -	c0 *= 100 * VLV_CZ_CLOCK_TO_MILLI_SEC * 4 / 1000;
> > +	c0 *= mul * VLV_CZ_CLOCK_TO_MILLI_SEC;
> 
> Based on the above this would need to be fixed too.
> 
> The above can be done as a follow-up if needed; this patch does what it
> says, so:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  
> >  	return c0 >= time;
> >  }
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx