[PATCH 0/3] drm/i915: Some GT freq stuff

[PATCH 0/3] drm/i915: Some GT freq stuff

[ville.syrjala]

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

A small pile of gt freq related stuff I had lying around.

Ville Syrjälä (3):
  drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV
  drm/i915: Set GPU freq to idle_freq initially
  drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max
    freq sysfs read

 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/i915_reg.h      |  1 +
 drivers/gpu/drm/i915/i915_sysfs.c    | 84 ++++++----------------------------
 drivers/gpu/drm/i915/intel_display.c | 19 +++++---
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 drivers/gpu/drm/i915/intel_pm.c      | 88 ++++++++++++++----------------------
 6 files changed, 64 insertions(+), 131 deletions(-)

-- 
2.4.10

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

[PATCH 1/3] drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV

[ville.syrjala]

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

Extract the GPLL reference frequency from CCK and use it in the
GPU freq<->opcode conversions on VLV/CHV. This eliminates all the
assumptions we have about which divider is used for which czclk
frequency.

Note that unlike most clocks from CCK, the GPLL ref clock is a divided
down version of the CZ clock rather than the HPLL clock. CZ clock itself
is a divided down version of the HPLL clock though, so in effect it just
gets divided down twice.

While at it, throw in a few comments explaining the remaining constants
for anyone who later wants to compare this to the spreadsheets.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/i915_reg.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 19 ++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 drivers/gpu/drm/i915/intel_pm.c      | 74 +++++++++++++-----------------------
 5 files changed, 44 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f37ac120a29d..95f77cd0ce17 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1166,6 +1166,7 @@ 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. */
+	u16 gpll_ref_freq;	/* vlv/chv GPLL reference frequency */
 
 	u8 up_threshold; /* Current %busy required to uplock */
 	u8 down_threshold; /* Current %busy required to downclock */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7dfc4007f3fa..aa21bfc06a24 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -785,6 +785,7 @@ enum skl_disp_power_wells {
 #define  DSI_PLL_M1_DIV_SHIFT			0
 #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
 #define CCK_CZ_CLOCK_CONTROL			0x62
+#define CCK_GPLL_CLOCK_CONTROL			0x67
 #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
 #define CCK_DISPLAY_REF_CLOCK_CONTROL		0x6c
 #define  CCK_TRUNK_FORCE_ON			(1 << 17)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 62d36a7b3398..ba50de534f9a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -147,15 +147,12 @@ static int valleyview_get_vco(struct drm_i915_private *dev_priv)
 	return vco_freq[hpll_freq] * 1000;
 }
 
-static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
-				  const char *name, u32 reg)
+int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
+		      const char *name, u32 reg, int ref_freq)
 {
 	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);
@@ -166,7 +163,17 @@ static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
 	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
 	     "%s change in progress\n", name);
 
-	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
+	return DIV_ROUND_CLOSEST(ref_freq << 1, divider + 1);
+}
+
+static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
+				  const char *name, u32 reg)
+{
+	if (dev_priv->hpll_freq == 0)
+		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
+
+	return vlv_get_cck_clock(dev_priv, name, reg,
+				 dev_priv->hpll_freq);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cd0b4eacbddf..c416f0adae48 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1116,6 +1116,8 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv);
 void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
 
 /* intel_display.c */
+int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
+		      const char *name, u32 reg, int ref_freq);
 extern const struct drm_plane_funcs intel_plane_funcs;
 unsigned int intel_rotation_info_size(const struct intel_rotation_info *rot_info);
 bool intel_has_pending_fb_unpin(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f65e84137060..c53b8c4d381c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5364,6 +5364,17 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
 	dev_priv->vlv_pctx = NULL;
 }
 
+static void vlv_init_gpll_ref_freq(struct drm_i915_private *dev_priv)
+{
+	dev_priv->rps.gpll_ref_freq =
+		vlv_get_cck_clock(dev_priv, "GPLL ref",
+				  CCK_GPLL_CLOCK_CONTROL,
+				  dev_priv->czclk_freq);
+
+	DRM_DEBUG_DRIVER("GPLL reference freq: %d kHz\n",
+			 dev_priv->rps.gpll_ref_freq);
+}
+
 static void valleyview_init_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5371,6 +5382,8 @@ static void valleyview_init_gt_powersave(struct drm_device *dev)
 
 	valleyview_setup_pctx(dev);
 
+	vlv_init_gpll_ref_freq(dev_priv);
+
 	mutex_lock(&dev_priv->rps.hw_lock);
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
@@ -5428,6 +5441,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
 
 	cherryview_setup_pctx(dev);
 
+	vlv_init_gpll_ref_freq(dev_priv);
+
 	mutex_lock(&dev_priv->rps.hw_lock);
 
 	mutex_lock(&dev_priv->sb_lock);
@@ -7261,68 +7276,33 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val
 	return 0;
 }
 
-static int vlv_gpu_freq_div(unsigned int czclk_freq)
-{
-	switch (czclk_freq) {
-	case 200:
-		return 10;
-	case 267:
-		return 12;
-	case 320:
-	case 333:
-		return 16;
-	case 400:
-		return 20;
-	default:
-		return -1;
-	}
-}
-
 static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
-	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
-
-	div = vlv_gpu_freq_div(czclk_freq);
-	if (div < 0)
-		return div;
-
-	return DIV_ROUND_CLOSEST(czclk_freq * (val + 6 - 0xbd), div);
+	/*
+	 * N = val - 0xb7
+	 * Slow = Fast = GPLL ref * N
+	 */
+	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * (val - 0xb7), 1000);
 }
 
 static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
 {
-	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
-
-	mul = vlv_gpu_freq_div(czclk_freq);
-	if (mul < 0)
-		return mul;
-
-	return DIV_ROUND_CLOSEST(mul * val, czclk_freq) + 0xbd - 6;
+	return DIV_ROUND_CLOSEST(1000 * val, dev_priv->rps.gpll_ref_freq) + 0xb7;
 }
 
 static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
-	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
-
-	div = vlv_gpu_freq_div(czclk_freq);
-	if (div < 0)
-		return div;
-	div /= 2;
-
-	return DIV_ROUND_CLOSEST(czclk_freq * val, 2 * div) / 2;
+	/*
+	 * N = val / 2
+	 * CU = CU2x / 2 = GPLL ref * N / 2
+	 */
+	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * val, 2 * 2 * 1000);
 }
 
 static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
 {
-	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
-
-	mul = vlv_gpu_freq_div(czclk_freq);
-	if (mul < 0)
-		return mul;
-	mul /= 2;
-
 	/* CHV needs even values */
-	return DIV_ROUND_CLOSEST(val * 2 * mul, czclk_freq) * 2;
+	return DIV_ROUND_CLOSEST(2 * 1000 * val, dev_priv->rps.gpll_ref_freq) * 2;
 }
 
 int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
-- 
2.4.10

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

Re: [PATCH 1/3] drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV

[Imre Deak]

On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Extract the GPLL reference frequency from CCK and use it in the
> GPU freq<->opcode conversions on VLV/CHV. This eliminates all the
> assumptions we have about which divider is used for which czclk
> frequency.
> 
> Note that unlike most clocks from CCK, the GPLL ref clock is a divided
> down version of the CZ clock rather than the HPLL clock. CZ clock itself
> is a divided down version of the HPLL clock though, so in effect it just
> gets divided down twice.
> 
> While at it, throw in a few comments explaining the remaining constants
> for anyone who later wants to compare this to the spreadsheets.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks ok. The relevant info is spread across multiple documents but
based on what I gathered this is ok and the old and new way of
calculation matches, so:
Reviewed-by: Imre Deak <imre.deak@intel.com>

Some notes:
For clarity, I'd rename *_gpu_freq to *_gpu_opcode_to_freq and
*_freq_opcode to *_gpu_freq_to_opcode and mention that the CHV CU/CU2X
clocks are also known as slow/fast clock, so it better matches the VLV
code.

The entries for VLV 320/400MHz CZ clock are missing from the tables
that I saw, which is one justification for this change, since we don't
know if vlv_gpu_freq_div() was correct.

--Imre



> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 19 ++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 74 +++++++++++++-----------------------
>  5 files changed, 44 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f37ac120a29d..95f77cd0ce17 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1166,6 +1166,7 @@ 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. */
> +	u16 gpll_ref_freq;	/* vlv/chv GPLL reference frequency */
>  
>  	u8 up_threshold; /* Current %busy required to uplock */
>  	u8 down_threshold; /* Current %busy required to downclock */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7dfc4007f3fa..aa21bfc06a24 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -785,6 +785,7 @@ enum skl_disp_power_wells {
>  #define  DSI_PLL_M1_DIV_SHIFT			0
>  #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
>  #define CCK_CZ_CLOCK_CONTROL			0x62
> +#define CCK_GPLL_CLOCK_CONTROL			0x67
>  #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
>  #define CCK_DISPLAY_REF_CLOCK_CONTROL		0x6c
>  #define  CCK_TRUNK_FORCE_ON			(1 << 17)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 62d36a7b3398..ba50de534f9a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -147,15 +147,12 @@ static int valleyview_get_vco(struct drm_i915_private *dev_priv)
>  	return vco_freq[hpll_freq] * 1000;
>  }
>  
> -static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> -				  const char *name, u32 reg)
> +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> +		      const char *name, u32 reg, int ref_freq)
>  {
>  	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);
> @@ -166,7 +163,17 @@ static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
>  	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
>  	     "%s change in progress\n", name);
>  
> -	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> +	return DIV_ROUND_CLOSEST(ref_freq << 1, divider + 1);
> +}
> +
> +static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> +				  const char *name, u32 reg)
> +{
> +	if (dev_priv->hpll_freq == 0)
> +		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> +
> +	return vlv_get_cck_clock(dev_priv, name, reg,
> +				 dev_priv->hpll_freq);
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cd0b4eacbddf..c416f0adae48 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1116,6 +1116,8 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv);
>  void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
>  
>  /* intel_display.c */
> +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> +		      const char *name, u32 reg, int ref_freq);
>  extern const struct drm_plane_funcs intel_plane_funcs;
>  unsigned int intel_rotation_info_size(const struct intel_rotation_info *rot_info);
>  bool intel_has_pending_fb_unpin(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f65e84137060..c53b8c4d381c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5364,6 +5364,17 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>  	dev_priv->vlv_pctx = NULL;
>  }
>  
> +static void vlv_init_gpll_ref_freq(struct drm_i915_private *dev_priv)
> +{
> +	dev_priv->rps.gpll_ref_freq =
> +		vlv_get_cck_clock(dev_priv, "GPLL ref",
> +				  CCK_GPLL_CLOCK_CONTROL,
> +				  dev_priv->czclk_freq);
> +
> +	DRM_DEBUG_DRIVER("GPLL reference freq: %d kHz\n",
> +			 dev_priv->rps.gpll_ref_freq);
> +}
> +
>  static void valleyview_init_gt_powersave(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5371,6 +5382,8 @@ static void valleyview_init_gt_powersave(struct drm_device *dev)
>  
>  	valleyview_setup_pctx(dev);
>  
> +	vlv_init_gpll_ref_freq(dev_priv);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> @@ -5428,6 +5441,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
>  
>  	cherryview_setup_pctx(dev);
>  
> +	vlv_init_gpll_ref_freq(dev_priv);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
>  	mutex_lock(&dev_priv->sb_lock);
> @@ -7261,68 +7276,33 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val
>  	return 0;
>  }
>  
> -static int vlv_gpu_freq_div(unsigned int czclk_freq)
> -{
> -	switch (czclk_freq) {
> -	case 200:
> -		return 10;
> -	case 267:
> -		return 12;
> -	case 320:
> -	case 333:
> -		return 16;
> -	case 400:
> -		return 20;
> -	default:
> -		return -1;
> -	}
> -}
> -
>  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
> -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> -
> -	div = vlv_gpu_freq_div(czclk_freq);
> -	if (div < 0)
> -		return div;
> -
> -	return DIV_ROUND_CLOSEST(czclk_freq * (val + 6 - 0xbd), div);
> +	/*
> +	 * N = val - 0xb7
> +	 * Slow = Fast = GPLL ref * N
> +	 */
> +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * (val - 0xb7), 1000);
>  }
>  
>  static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  {
> -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> -
> -	mul = vlv_gpu_freq_div(czclk_freq);
> -	if (mul < 0)
> -		return mul;
> -
> -	return DIV_ROUND_CLOSEST(mul * val, czclk_freq) + 0xbd - 6;
> +	return DIV_ROUND_CLOSEST(1000 * val, dev_priv->rps.gpll_ref_freq) + 0xb7;
>  }
>  
>  static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
> -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> -
> -	div = vlv_gpu_freq_div(czclk_freq);
> -	if (div < 0)
> -		return div;
> -	div /= 2;
> -
> -	return DIV_ROUND_CLOSEST(czclk_freq * val, 2 * div) / 2;
> +	/*
> +	 * N = val / 2
> +	 * CU = CU2x / 2 = GPLL ref * N / 2
> +	 */
> +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * val, 2 * 2 * 1000);
>  }
>  
>  static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  {
> -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> -
> -	mul = vlv_gpu_freq_div(czclk_freq);
> -	if (mul < 0)
> -		return mul;
> -	mul /= 2;
> -
>  	/* CHV needs even values */
> -	return DIV_ROUND_CLOSEST(val * 2 * mul, czclk_freq) * 2;
> +	return DIV_ROUND_CLOSEST(2 * 1000 * val, dev_priv->rps.gpll_ref_freq) * 2;
>  }
>  
>  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV

[Ville Syrjälä]

On Wed, Mar 16, 2016 at 07:17:51PM +0200, Imre Deak wrote:
> On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Extract the GPLL reference frequency from CCK and use it in the
> > GPU freq<->opcode conversions on VLV/CHV. This eliminates all the
> > assumptions we have about which divider is used for which czclk
> > frequency.
> > 
> > Note that unlike most clocks from CCK, the GPLL ref clock is a divided
> > down version of the CZ clock rather than the HPLL clock. CZ clock itself
> > is a divided down version of the HPLL clock though, so in effect it just
> > gets divided down twice.
> > 
> > While at it, throw in a few comments explaining the remaining constants
> > for anyone who later wants to compare this to the spreadsheets.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Looks ok. The relevant info is spread across multiple documents but
> based on what I gathered this is ok and the old and new way of
> calculation matches, so:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> Some notes:
> For clarity, I'd rename *_gpu_freq to *_gpu_opcode_to_freq and
> *_freq_opcode to *_gpu_freq_to_opcode and mention that the CHV CU/CU2X
> clocks are also known as slow/fast clock, so it better matches the VLV
> code.

Well I think the slow/fast names might be more of a leftover from VLV,
ie. someone just forgot to update the names in bunch of the docs for
CHV. But adding a note might be prudent to avoid people having to
wonder about it.

> 
> The entries for VLV 320/400MHz CZ clock are missing from the tables
> that I saw, which is one justification for this change, since we don't
> know if vlv_gpu_freq_div() was correct.

I don't think VLV had 320 or 400 SKUs.

> 
> --Imre
> 
> 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 19 ++++++---
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 +
> >  drivers/gpu/drm/i915/intel_pm.c      | 74 +++++++++++++-----------------------
> >  5 files changed, 44 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f37ac120a29d..95f77cd0ce17 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1166,6 +1166,7 @@ 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. */
> > +	u16 gpll_ref_freq;	/* vlv/chv GPLL reference frequency */
> >  
> >  	u8 up_threshold; /* Current %busy required to uplock */
> >  	u8 down_threshold; /* Current %busy required to downclock */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 7dfc4007f3fa..aa21bfc06a24 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -785,6 +785,7 @@ enum skl_disp_power_wells {
> >  #define  DSI_PLL_M1_DIV_SHIFT			0
> >  #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
> >  #define CCK_CZ_CLOCK_CONTROL			0x62
> > +#define CCK_GPLL_CLOCK_CONTROL			0x67
> >  #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
> >  #define CCK_DISPLAY_REF_CLOCK_CONTROL		0x6c
> >  #define  CCK_TRUNK_FORCE_ON			(1 << 17)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 62d36a7b3398..ba50de534f9a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -147,15 +147,12 @@ static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> >  	return vco_freq[hpll_freq] * 1000;
> >  }
> >  
> > -static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> > -				  const char *name, u32 reg)
> > +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> > +		      const char *name, u32 reg, int ref_freq)
> >  {
> >  	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);
> > @@ -166,7 +163,17 @@ static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> >  	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
> >  	     "%s change in progress\n", name);
> >  
> > -	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> > +	return DIV_ROUND_CLOSEST(ref_freq << 1, divider + 1);
> > +}
> > +
> > +static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> > +				  const char *name, u32 reg)
> > +{
> > +	if (dev_priv->hpll_freq == 0)
> > +		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> > +
> > +	return vlv_get_cck_clock(dev_priv, name, reg,
> > +				 dev_priv->hpll_freq);
> >  }
> >  
> >  static int
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index cd0b4eacbddf..c416f0adae48 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1116,6 +1116,8 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv);
> >  void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
> >  
> >  /* intel_display.c */
> > +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> > +		      const char *name, u32 reg, int ref_freq);
> >  extern const struct drm_plane_funcs intel_plane_funcs;
> >  unsigned int intel_rotation_info_size(const struct intel_rotation_info *rot_info);
> >  bool intel_has_pending_fb_unpin(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index f65e84137060..c53b8c4d381c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5364,6 +5364,17 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
> >  	dev_priv->vlv_pctx = NULL;
> >  }
> >  
> > +static void vlv_init_gpll_ref_freq(struct drm_i915_private *dev_priv)
> > +{
> > +	dev_priv->rps.gpll_ref_freq =
> > +		vlv_get_cck_clock(dev_priv, "GPLL ref",
> > +				  CCK_GPLL_CLOCK_CONTROL,
> > +				  dev_priv->czclk_freq);
> > +
> > +	DRM_DEBUG_DRIVER("GPLL reference freq: %d kHz\n",
> > +			 dev_priv->rps.gpll_ref_freq);
> > +}
> > +
> >  static void valleyview_init_gt_powersave(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -5371,6 +5382,8 @@ static void valleyview_init_gt_powersave(struct drm_device *dev)
> >  
> >  	valleyview_setup_pctx(dev);
> >  
> > +	vlv_init_gpll_ref_freq(dev_priv);
> > +
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  
> >  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > @@ -5428,6 +5441,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
> >  
> >  	cherryview_setup_pctx(dev);
> >  
> > +	vlv_init_gpll_ref_freq(dev_priv);
> > +
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  
> >  	mutex_lock(&dev_priv->sb_lock);
> > @@ -7261,68 +7276,33 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val
> >  	return 0;
> >  }
> >  
> > -static int vlv_gpu_freq_div(unsigned int czclk_freq)
> > -{
> > -	switch (czclk_freq) {
> > -	case 200:
> > -		return 10;
> > -	case 267:
> > -		return 12;
> > -	case 320:
> > -	case 333:
> > -		return 16;
> > -	case 400:
> > -		return 20;
> > -	default:
> > -		return -1;
> > -	}
> > -}
> > -
> >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > -
> > -	div = vlv_gpu_freq_div(czclk_freq);
> > -	if (div < 0)
> > -		return div;
> > -
> > -	return DIV_ROUND_CLOSEST(czclk_freq * (val + 6 - 0xbd), div);
> > +	/*
> > +	 * N = val - 0xb7
> > +	 * Slow = Fast = GPLL ref * N
> > +	 */
> > +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * (val - 0xb7), 1000);
> >  }
> >  
> >  static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > -
> > -	mul = vlv_gpu_freq_div(czclk_freq);
> > -	if (mul < 0)
> > -		return mul;
> > -
> > -	return DIV_ROUND_CLOSEST(mul * val, czclk_freq) + 0xbd - 6;
> > +	return DIV_ROUND_CLOSEST(1000 * val, dev_priv->rps.gpll_ref_freq) + 0xb7;
> >  }
> >  
> >  static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > -
> > -	div = vlv_gpu_freq_div(czclk_freq);
> > -	if (div < 0)
> > -		return div;
> > -	div /= 2;
> > -
> > -	return DIV_ROUND_CLOSEST(czclk_freq * val, 2 * div) / 2;
> > +	/*
> > +	 * N = val / 2
> > +	 * CU = CU2x / 2 = GPLL ref * N / 2
> > +	 */
> > +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * val, 2 * 2 * 1000);
> >  }
> >  
> >  static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
> >  {
> > -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > -
> > -	mul = vlv_gpu_freq_div(czclk_freq);
> > -	if (mul < 0)
> > -		return mul;
> > -	mul /= 2;
> > -
> >  	/* CHV needs even values */
> > -	return DIV_ROUND_CLOSEST(val * 2 * mul, czclk_freq) * 2;
> > +	return DIV_ROUND_CLOSEST(2 * 1000 * val, dev_priv->rps.gpll_ref_freq) * 2;
> >  }
> >  
> >  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)

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

Re: [PATCH 1/3] drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV

[Imre Deak]

On Wed, 2016-03-16 at 19:47 +0200, Ville Syrjälä wrote:
> On Wed, Mar 16, 2016 at 07:17:51PM +0200, Imre Deak wrote:
> > On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com
> > wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Extract the GPLL reference frequency from CCK and use it in the
> > > GPU freq<->opcode conversions on VLV/CHV. This eliminates all the
> > > assumptions we have about which divider is used for which czclk
> > > frequency.
> > > 
> > > Note that unlike most clocks from CCK, the GPLL ref clock is a
> > > divided
> > > down version of the CZ clock rather than the HPLL clock. CZ clock
> > > itself
> > > is a divided down version of the HPLL clock though, so in effect
> > > it just
> > > gets divided down twice.
> > > 
> > > While at it, throw in a few comments explaining the remaining
> > > constants
> > > for anyone who later wants to compare this to the spreadsheets.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Looks ok. The relevant info is spread across multiple documents but
> > based on what I gathered this is ok and the old and new way of
> > calculation matches, so:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > Some notes:
> > For clarity, I'd rename *_gpu_freq to *_gpu_opcode_to_freq and
> > *_freq_opcode to *_gpu_freq_to_opcode and mention that the CHV
> > CU/CU2X
> > clocks are also known as slow/fast clock, so it better matches the
> > VLV
> > code.
> 
> Well I think the slow/fast names might be more of a leftover from
> VLV,
> ie. someone just forgot to update the names in bunch of the docs for
> CHV. But adding a note might be prudent to avoid people having to
> wonder about it.
> 
> > 
> > The entries for VLV 320/400MHz CZ clock are missing from the tables
> > that I saw, which is one justification for this change, since we
> > don't
> > know if vlv_gpu_freq_div() was correct.
> 
> I don't think VLV had 320 or 400 SKUs.

Ah, ok so those exist only on CHV, I missed that.

--Imre

> 
> > 
> > --Imre
> > 
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> > >  drivers/gpu/drm/i915/intel_display.c | 19 ++++++---
> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 +
> > >  drivers/gpu/drm/i915/intel_pm.c      | 74 +++++++++++++---------
> > > --------------
> > >  5 files changed, 44 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index f37ac120a29d..95f77cd0ce17 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1166,6 +1166,7 @@ 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. */
> > > +	u16 gpll_ref_freq;	/* vlv/chv GPLL reference
> > > frequency */
> > >  
> > >  	u8 up_threshold; /* Current %busy required to uplock */
> > >  	u8 down_threshold; /* Current %busy required to
> > > downclock */
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 7dfc4007f3fa..aa21bfc06a24 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -785,6 +785,7 @@ enum skl_disp_power_wells {
> > >  #define  DSI_PLL_M1_DIV_SHIFT			0
> > >  #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
> > >  #define CCK_CZ_CLOCK_CONTROL			0x62
> > > +#define CCK_GPLL_CLOCK_CONTROL			0x67
> > >  #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
> > >  #define CCK_DISPLAY_REF_CLOCK_CONTROL		0x6c
> > >  #define  CCK_TRUNK_FORCE_ON			(1 << 17)
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 62d36a7b3398..ba50de534f9a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -147,15 +147,12 @@ static int valleyview_get_vco(struct
> > > drm_i915_private *dev_priv)
> > >  	return vco_freq[hpll_freq] * 1000;
> > >  }
> > >  
> > > -static int vlv_get_cck_clock_hpll(struct drm_i915_private
> > > *dev_priv,
> > > -				  const char *name, u32 reg)
> > > +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> > > +		      const char *name, u32 reg, int ref_freq)
> > >  {
> > >  	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);
> > > @@ -166,7 +163,17 @@ static int vlv_get_cck_clock_hpll(struct
> > > drm_i915_private *dev_priv,
> > >  	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
> > >  	     "%s change in progress\n", name);
> > >  
> > > -	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1,
> > > divider + 1);
> > > +	return DIV_ROUND_CLOSEST(ref_freq << 1, divider + 1);
> > > +}
> > > +
> > > +static int vlv_get_cck_clock_hpll(struct drm_i915_private
> > > *dev_priv,
> > > +				  const char *name, u32 reg)
> > > +{
> > > +	if (dev_priv->hpll_freq == 0)
> > > +		dev_priv->hpll_freq =
> > > valleyview_get_vco(dev_priv);
> > > +
> > > +	return vlv_get_cck_clock(dev_priv, name, reg,
> > > +				 dev_priv->hpll_freq);
> > >  }
> > >  
> > >  static int
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index cd0b4eacbddf..c416f0adae48 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1116,6 +1116,8 @@ void i915_audio_component_init(struct
> > > drm_i915_private *dev_priv);
> > >  void i915_audio_component_cleanup(struct drm_i915_private
> > > *dev_priv);
> > >  
> > >  /* intel_display.c */
> > > +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> > > +		      const char *name, u32 reg, int ref_freq);
> > >  extern const struct drm_plane_funcs intel_plane_funcs;
> > >  unsigned int intel_rotation_info_size(const struct
> > > intel_rotation_info *rot_info);
> > >  bool intel_has_pending_fb_unpin(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index f65e84137060..c53b8c4d381c 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5364,6 +5364,17 @@ static void valleyview_cleanup_pctx(struct
> > > drm_device *dev)
> > >  	dev_priv->vlv_pctx = NULL;
> > >  }
> > >  
> > > +static void vlv_init_gpll_ref_freq(struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > +	dev_priv->rps.gpll_ref_freq =
> > > +		vlv_get_cck_clock(dev_priv, "GPLL ref",
> > > +				  CCK_GPLL_CLOCK_CONTROL,
> > > +				  dev_priv->czclk_freq);
> > > +
> > > +	DRM_DEBUG_DRIVER("GPLL reference freq: %d kHz\n",
> > > +			 dev_priv->rps.gpll_ref_freq);
> > > +}
> > > +
> > >  static void valleyview_init_gt_powersave(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -5371,6 +5382,8 @@ static void
> > > valleyview_init_gt_powersave(struct drm_device *dev)
> > >  
> > >  	valleyview_setup_pctx(dev);
> > >  
> > > +	vlv_init_gpll_ref_freq(dev_priv);
> > > +
> > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > >  
> > >  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > > @@ -5428,6 +5441,8 @@ static void
> > > cherryview_init_gt_powersave(struct drm_device *dev)
> > >  
> > >  	cherryview_setup_pctx(dev);
> > >  
> > > +	vlv_init_gpll_ref_freq(dev_priv);
> > > +
> > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > >  
> > >  	mutex_lock(&dev_priv->sb_lock);
> > > @@ -7261,68 +7276,33 @@ int sandybridge_pcode_write(struct
> > > drm_i915_private *dev_priv, u32 mbox, u32 val
> > >  	return 0;
> > >  }
> > >  
> > > -static int vlv_gpu_freq_div(unsigned int czclk_freq)
> > > -{
> > > -	switch (czclk_freq) {
> > > -	case 200:
> > > -		return 10;
> > > -	case 267:
> > > -		return 12;
> > > -	case 320:
> > > -	case 333:
> > > -		return 16;
> > > -	case 400:
> > > -		return 20;
> > > -	default:
> > > -		return -1;
> > > -	}
> > > -}
> > > -
> > >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int
> > > val)
> > >  {
> > > -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv-
> > > >czclk_freq, 1000);
> > > -
> > > -	div = vlv_gpu_freq_div(czclk_freq);
> > > -	if (div < 0)
> > > -		return div;
> > > -
> > > -	return DIV_ROUND_CLOSEST(czclk_freq * (val + 6 - 0xbd),
> > > div);
> > > +	/*
> > > +	 * N = val - 0xb7
> > > +	 * Slow = Fast = GPLL ref * N
> > > +	 */
> > > +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq *
> > > (val - 0xb7), 1000);
> > >  }
> > >  
> > >  static int byt_freq_opcode(struct drm_i915_private *dev_priv,
> > > int val)
> > >  {
> > > -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv-
> > > >czclk_freq, 1000);
> > > -
> > > -	mul = vlv_gpu_freq_div(czclk_freq);
> > > -	if (mul < 0)
> > > -		return mul;
> > > -
> > > -	return DIV_ROUND_CLOSEST(mul * val, czclk_freq) + 0xbd -
> > > 6;
> > > +	return DIV_ROUND_CLOSEST(1000 * val, dev_priv-
> > > >rps.gpll_ref_freq) + 0xb7;
> > >  }
> > >  
> > >  static int chv_gpu_freq(struct drm_i915_private *dev_priv, int
> > > val)
> > >  {
> > > -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv-
> > > >czclk_freq, 1000);
> > > -
> > > -	div = vlv_gpu_freq_div(czclk_freq);
> > > -	if (div < 0)
> > > -		return div;
> > > -	div /= 2;
> > > -
> > > -	return DIV_ROUND_CLOSEST(czclk_freq * val, 2 * div) / 2;
> > > +	/*
> > > +	 * N = val / 2
> > > +	 * CU = CU2x / 2 = GPLL ref * N / 2
> > > +	 */
> > > +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq *
> > > val, 2 * 2 * 1000);
> > >  }
> > >  
> > >  static int chv_freq_opcode(struct drm_i915_private *dev_priv,
> > > int val)
> > >  {
> > > -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv-
> > > >czclk_freq, 1000);
> > > -
> > > -	mul = vlv_gpu_freq_div(czclk_freq);
> > > -	if (mul < 0)
> > > -		return mul;
> > > -	mul /= 2;
> > > -
> > >  	/* CHV needs even values */
> > > -	return DIV_ROUND_CLOSEST(val * 2 * mul, czclk_freq) * 2;
> > > +	return DIV_ROUND_CLOSEST(2 * 1000 * val, dev_priv-
> > > >rps.gpll_ref_freq) * 2;
> > >  }
> > >  
> > >  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV

[Ville Syrjälä]

On Wed, Mar 16, 2016 at 07:47:06PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 16, 2016 at 07:17:51PM +0200, Imre Deak wrote:
> > On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Extract the GPLL reference frequency from CCK and use it in the
> > > GPU freq<->opcode conversions on VLV/CHV. This eliminates all the
> > > assumptions we have about which divider is used for which czclk
> > > frequency.
> > > 
> > > Note that unlike most clocks from CCK, the GPLL ref clock is a divided
> > > down version of the CZ clock rather than the HPLL clock. CZ clock itself
> > > is a divided down version of the HPLL clock though, so in effect it just
> > > gets divided down twice.
> > > 
> > > While at it, throw in a few comments explaining the remaining constants
> > > for anyone who later wants to compare this to the spreadsheets.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Looks ok. The relevant info is spread across multiple documents but
> > based on what I gathered this is ok and the old and new way of
> > calculation matches, so:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > Some notes:
> > For clarity, I'd rename *_gpu_freq to *_gpu_opcode_to_freq and
> > *_freq_opcode to *_gpu_freq_to_opcode and mention that the CHV CU/CU2X
> > clocks are also known as slow/fast clock, so it better matches the VLV
> > code.
> 
> Well I think the slow/fast names might be more of a leftover from VLV,
> ie. someone just forgot to update the names in bunch of the docs for
> CHV. But adding a note might be prudent to avoid people having to
> wonder about it.

Changed the comment to
"CU (slow) = CU2x (fast) / 2 = GPLL ref * N / 2"

and pushed the first two patches from the series to dinq.
Thanks for the review.

> 
> > 
> > The entries for VLV 320/400MHz CZ clock are missing from the tables
> > that I saw, which is one justification for this change, since we don't
> > know if vlv_gpu_freq_div() was correct.
> 
> I don't think VLV had 320 or 400 SKUs.
> 
> > 
> > --Imre
> > 
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> > >  drivers/gpu/drm/i915/intel_display.c | 19 ++++++---
> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 +
> > >  drivers/gpu/drm/i915/intel_pm.c      | 74 +++++++++++++-----------------------
> > >  5 files changed, 44 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index f37ac120a29d..95f77cd0ce17 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1166,6 +1166,7 @@ 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. */
> > > +	u16 gpll_ref_freq;	/* vlv/chv GPLL reference frequency */
> > >  
> > >  	u8 up_threshold; /* Current %busy required to uplock */
> > >  	u8 down_threshold; /* Current %busy required to downclock */
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 7dfc4007f3fa..aa21bfc06a24 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -785,6 +785,7 @@ enum skl_disp_power_wells {
> > >  #define  DSI_PLL_M1_DIV_SHIFT			0
> > >  #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
> > >  #define CCK_CZ_CLOCK_CONTROL			0x62
> > > +#define CCK_GPLL_CLOCK_CONTROL			0x67
> > >  #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
> > >  #define CCK_DISPLAY_REF_CLOCK_CONTROL		0x6c
> > >  #define  CCK_TRUNK_FORCE_ON			(1 << 17)
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 62d36a7b3398..ba50de534f9a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -147,15 +147,12 @@ static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> > >  	return vco_freq[hpll_freq] * 1000;
> > >  }
> > >  
> > > -static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> > > -				  const char *name, u32 reg)
> > > +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> > > +		      const char *name, u32 reg, int ref_freq)
> > >  {
> > >  	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);
> > > @@ -166,7 +163,17 @@ static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> > >  	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
> > >  	     "%s change in progress\n", name);
> > >  
> > > -	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> > > +	return DIV_ROUND_CLOSEST(ref_freq << 1, divider + 1);
> > > +}
> > > +
> > > +static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> > > +				  const char *name, u32 reg)
> > > +{
> > > +	if (dev_priv->hpll_freq == 0)
> > > +		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> > > +
> > > +	return vlv_get_cck_clock(dev_priv, name, reg,
> > > +				 dev_priv->hpll_freq);
> > >  }
> > >  
> > >  static int
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index cd0b4eacbddf..c416f0adae48 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1116,6 +1116,8 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv);
> > >  void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
> > >  
> > >  /* intel_display.c */
> > > +int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
> > > +		      const char *name, u32 reg, int ref_freq);
> > >  extern const struct drm_plane_funcs intel_plane_funcs;
> > >  unsigned int intel_rotation_info_size(const struct intel_rotation_info *rot_info);
> > >  bool intel_has_pending_fb_unpin(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index f65e84137060..c53b8c4d381c 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5364,6 +5364,17 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
> > >  	dev_priv->vlv_pctx = NULL;
> > >  }
> > >  
> > > +static void vlv_init_gpll_ref_freq(struct drm_i915_private *dev_priv)
> > > +{
> > > +	dev_priv->rps.gpll_ref_freq =
> > > +		vlv_get_cck_clock(dev_priv, "GPLL ref",
> > > +				  CCK_GPLL_CLOCK_CONTROL,
> > > +				  dev_priv->czclk_freq);
> > > +
> > > +	DRM_DEBUG_DRIVER("GPLL reference freq: %d kHz\n",
> > > +			 dev_priv->rps.gpll_ref_freq);
> > > +}
> > > +
> > >  static void valleyview_init_gt_powersave(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -5371,6 +5382,8 @@ static void valleyview_init_gt_powersave(struct drm_device *dev)
> > >  
> > >  	valleyview_setup_pctx(dev);
> > >  
> > > +	vlv_init_gpll_ref_freq(dev_priv);
> > > +
> > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > >  
> > >  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > > @@ -5428,6 +5441,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
> > >  
> > >  	cherryview_setup_pctx(dev);
> > >  
> > > +	vlv_init_gpll_ref_freq(dev_priv);
> > > +
> > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > >  
> > >  	mutex_lock(&dev_priv->sb_lock);
> > > @@ -7261,68 +7276,33 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val
> > >  	return 0;
> > >  }
> > >  
> > > -static int vlv_gpu_freq_div(unsigned int czclk_freq)
> > > -{
> > > -	switch (czclk_freq) {
> > > -	case 200:
> > > -		return 10;
> > > -	case 267:
> > > -		return 12;
> > > -	case 320:
> > > -	case 333:
> > > -		return 16;
> > > -	case 400:
> > > -		return 20;
> > > -	default:
> > > -		return -1;
> > > -	}
> > > -}
> > > -
> > >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > >  {
> > > -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > > -
> > > -	div = vlv_gpu_freq_div(czclk_freq);
> > > -	if (div < 0)
> > > -		return div;
> > > -
> > > -	return DIV_ROUND_CLOSEST(czclk_freq * (val + 6 - 0xbd), div);
> > > +	/*
> > > +	 * N = val - 0xb7
> > > +	 * Slow = Fast = GPLL ref * N
> > > +	 */
> > > +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * (val - 0xb7), 1000);
> > >  }
> > >  
> > >  static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
> > >  {
> > > -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > > -
> > > -	mul = vlv_gpu_freq_div(czclk_freq);
> > > -	if (mul < 0)
> > > -		return mul;
> > > -
> > > -	return DIV_ROUND_CLOSEST(mul * val, czclk_freq) + 0xbd - 6;
> > > +	return DIV_ROUND_CLOSEST(1000 * val, dev_priv->rps.gpll_ref_freq) + 0xb7;
> > >  }
> > >  
> > >  static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > >  {
> > > -	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > > -
> > > -	div = vlv_gpu_freq_div(czclk_freq);
> > > -	if (div < 0)
> > > -		return div;
> > > -	div /= 2;
> > > -
> > > -	return DIV_ROUND_CLOSEST(czclk_freq * val, 2 * div) / 2;
> > > +	/*
> > > +	 * N = val / 2
> > > +	 * CU = CU2x / 2 = GPLL ref * N / 2
> > > +	 */
> > > +	return DIV_ROUND_CLOSEST(dev_priv->rps.gpll_ref_freq * val, 2 * 2 * 1000);
> > >  }
> > >  
> > >  static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
> > >  {
> > > -	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> > > -
> > > -	mul = vlv_gpu_freq_div(czclk_freq);
> > > -	if (mul < 0)
> > > -		return mul;
> > > -	mul /= 2;
> > > -
> > >  	/* CHV needs even values */
> > > -	return DIV_ROUND_CLOSEST(val * 2 * mul, czclk_freq) * 2;
> > > +	return DIV_ROUND_CLOSEST(2 * 1000 * val, dev_priv->rps.gpll_ref_freq) * 2;
> > >  }
> > >  
> > >  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 2/3] drm/i915: Set GPU freq to idle_freq initially

[ville.syrjala]

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

Currently we set the initial GPU frequency to min_freq_softlimit
on gen9, and to efficient_freq on VLV/CHV. On all the other platforms
we set it to idle_freq. Let's use idle_freq across the board to make
sure we don't waste power. This is especially relevant for VLV since
Vnn won't drop to minimum unless the GPU is at the minimum frequency.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c53b8c4d381c..2591d533a895 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4804,7 +4804,7 @@ static void gen9_enable_rps(struct drm_device *dev)
 	 * Up/Down EI & threshold registers, as well as the RP_CONTROL,
 	 * RP_INTERRUPT_LIMITS & RPNSWREQ registers */
 	dev_priv->rps.power = HIGH_POWER; /* force a reset */
-	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+	gen6_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5594,10 +5594,10 @@ static void cherryview_enable_rps(struct drm_device *dev)
 			 dev_priv->rps.cur_freq);
 
 	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
-			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
-			 dev_priv->rps.efficient_freq);
+			 intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq),
+			 dev_priv->rps.idle_freq);
 
-	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
+	valleyview_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5684,10 +5684,10 @@ static void valleyview_enable_rps(struct drm_device *dev)
 			 dev_priv->rps.cur_freq);
 
 	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
-			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
-			 dev_priv->rps.efficient_freq);
+			 intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq),
+			 dev_priv->rps.idle_freq);
 
-	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
+	valleyview_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
-- 
2.4.10

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

Re: [PATCH 2/3] drm/i915: Set GPU freq to idle_freq initially

[Imre Deak]

On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we set the initial GPU frequency to min_freq_softlimit
> on gen9, and to efficient_freq on VLV/CHV. On all the other platforms
> we set it to idle_freq. Let's use idle_freq across the board to make
> sure we don't waste power. This is especially relevant for VLV since
> Vnn won't drop to minimum unless the GPU is at the minimum frequency.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yes, I think having the same logic on all platforms make sense, so:
Reviewed-by: Imre Deak <imre.deak@intel.com>

I noticed that rps.min_freq is always the same as rps.idle_freq, what's
the reason to have both, could we remove one of them? Adding Chris.

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c53b8c4d381c..2591d533a895 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4804,7 +4804,7 @@ static void gen9_enable_rps(struct drm_device *dev)
>  	 * Up/Down EI & threshold registers, as well as the RP_CONTROL,
>  	 * RP_INTERRUPT_LIMITS & RPNSWREQ registers */
>  	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> -	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +	gen6_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
> @@ -5594,10 +5594,10 @@ static void cherryview_enable_rps(struct drm_device *dev)
>  			 dev_priv->rps.cur_freq);
>  
>  	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
> -			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> -			 dev_priv->rps.efficient_freq);
> +			 intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq),
> +			 dev_priv->rps.idle_freq);
>  
> -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> +	valleyview_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
> @@ -5684,10 +5684,10 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  			 dev_priv->rps.cur_freq);
>  
>  	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
> -			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> -			 dev_priv->rps.efficient_freq);
> +			 intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq),
> +			 dev_priv->rps.idle_freq);
>  
> -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> +	valleyview_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Set GPU freq to idle_freq initially

[Chris Wilson]

On Wed, Mar 16, 2016 at 07:56:58PM +0200, Imre Deak wrote:
> On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we set the initial GPU frequency to min_freq_softlimit
> > on gen9, and to efficient_freq on VLV/CHV. On all the other platforms
> > we set it to idle_freq. Let's use idle_freq across the board to make
> > sure we don't waste power. This is especially relevant for VLV since
> > Vnn won't drop to minimum unless the GPU is at the minimum frequency.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Yes, I think having the same logic on all platforms make sense, so:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> I noticed that rps.min_freq is always the same as rps.idle_freq, what's
> the reason to have both, could we remove one of them? Adding Chris.

They have two different meanings, one is the hardware minimum and the
other is the frequency to use at idle. As indicated by vlv, it was not
always clear if the HW engineers thought it was a good idea to rest at
minimum and so having a separate control knob would be useful.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read

[ville.syrjala]

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

cur_freq, min/max_freq_softlimit, efficient_freq are just single values
stored under dev_priv.rps, so there's no real point in locking, resuming
the devices and flushing the delayed resume work just to print them out.

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

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 2d576b7ff299..9e39d7a18fdb 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -305,55 +305,6 @@ static ssize_t gt_act_freq_mhz_show(struct device *kdev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
 }
 
-static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct drm_minor *minor = dev_to_drm_minor(kdev);
-	struct drm_device *dev = minor->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
-
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
-	intel_runtime_pm_get(dev_priv);
-
-	mutex_lock(&dev_priv->rps.hw_lock);
-	ret = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
-	intel_runtime_pm_put(dev_priv);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
-}
-
-static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
-				     struct device_attribute *attr, char *buf)
-{
-	struct drm_minor *minor = dev_to_drm_minor(kdev);
-	struct drm_device *dev = minor->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	return snprintf(buf, PAGE_SIZE,
-			"%d\n",
-			intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq));
-}
-
-static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_minor *minor = dev_to_drm_minor(kdev);
-	struct drm_device *dev = minor->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
-
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
-	mutex_lock(&dev_priv->rps.hw_lock);
-	ret = intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit);
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
-}
-
 static ssize_t gt_max_freq_mhz_store(struct device *kdev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -406,22 +357,6 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
 	return count;
 }
 
-static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_minor *minor = dev_to_drm_minor(kdev);
-	struct drm_device *dev = minor->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
-
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
-	mutex_lock(&dev_priv->rps.hw_lock);
-	ret = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit);
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
-}
-
 static ssize_t gt_min_freq_mhz_store(struct device *kdev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -472,16 +407,15 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
 }
 
 static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store);
-static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store);
-
-static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show, NULL);
 
 static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf);
+static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
+static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_rp_mhz_show, gt_max_freq_mhz_store);
+static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_rp_mhz_show, gt_min_freq_mhz_store);
 static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
 static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
 static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
+static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
 
 /* For now we have a static number of RP states */
 static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
@@ -491,12 +425,20 @@ static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 val;
 
-	if (attr == &dev_attr_gt_RP0_freq_mhz)
+	if (attr == &dev_attr_gt_cur_freq_mhz)
+		val = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
+	else if (attr == &dev_attr_gt_max_freq_mhz)
+		val = intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit);
+	else if (attr == &dev_attr_gt_min_freq_mhz)
+		val = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit);
+	else if (attr == &dev_attr_gt_RP0_freq_mhz)
 		val = intel_gpu_freq(dev_priv, dev_priv->rps.rp0_freq);
 	else if (attr == &dev_attr_gt_RP1_freq_mhz)
 		val = intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq);
 	else if (attr == &dev_attr_gt_RPn_freq_mhz)
 		val = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq);
+	else if (attr == &dev_attr_vlv_rpe_freq_mhz)
+		val = intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq);
 	else
 		BUG();
 
-- 
2.4.10

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

Re: [PATCH 3/3] drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read

[Imre Deak]

On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> cur_freq, min/max_freq_softlimit, efficient_freq are just single
> values
> stored under dev_priv.rps, so there's no real point in locking,
> resuming
> the devices and flushing the delayed resume work just to print them
> out.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

This makes things clearer, so would it make sense to still keep the
flush wq and leave out patch 4 until somebody benchmarks things? That
would address Tom's concern as well.

> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 84 ++++++-----------------------
> ----------
>  1 file changed, 13 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index 2d576b7ff299..9e39d7a18fdb 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -305,55 +305,6 @@ static ssize_t gt_act_freq_mhz_show(struct
> device *kdev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
>  }
>  
> -static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> -				    struct device_attribute *attr,
> char *buf)
> -{
> -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> -	struct drm_device *dev = minor->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
> -
> -	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> -	intel_runtime_pm_get(dev_priv);
> -
> -	mutex_lock(&dev_priv->rps.hw_lock);
> -	ret = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -
> -	intel_runtime_pm_put(dev_priv);
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> -}
> -
> -static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
> -				     struct device_attribute *attr,
> char *buf)
> -{
> -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> -	struct drm_device *dev = minor->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	return snprintf(buf, PAGE_SIZE,
> -			"%d\n",
> -			intel_gpu_freq(dev_priv, dev_priv-
> >rps.efficient_freq));
> -}
> -
> -static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> -{
> -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> -	struct drm_device *dev = minor->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
> -
> -	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> -	mutex_lock(&dev_priv->rps.hw_lock);
> -	ret = intel_gpu_freq(dev_priv, dev_priv-
> >rps.max_freq_softlimit);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> -}
> -
>  static ssize_t gt_max_freq_mhz_store(struct device *kdev,
>  				     struct device_attribute *attr,
>  				     const char *buf, size_t count)
> @@ -406,22 +357,6 @@ static ssize_t gt_max_freq_mhz_store(struct
> device *kdev,
>  	return count;
>  }
>  
> -static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> -{
> -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> -	struct drm_device *dev = minor->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
> -
> -	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> -	mutex_lock(&dev_priv->rps.hw_lock);
> -	ret = intel_gpu_freq(dev_priv, dev_priv-
> >rps.min_freq_softlimit);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> -}
> -
>  static ssize_t gt_min_freq_mhz_store(struct device *kdev,
>  				     struct device_attribute *attr,
>  				     const char *buf, size_t count)
> @@ -472,16 +407,15 @@ static ssize_t gt_min_freq_mhz_store(struct
> device *kdev,
>  }
>  
>  static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show,
> NULL);
> -static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show,
> NULL);
> -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR,
> gt_max_freq_mhz_show, gt_max_freq_mhz_store);
> -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR,
> gt_min_freq_mhz_show, gt_min_freq_mhz_store);
> -
> -static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show,
> NULL);
>  
>  static ssize_t gt_rp_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf);
> +static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR,
> gt_rp_mhz_show, gt_max_freq_mhz_store);
> +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR,
> gt_rp_mhz_show, gt_min_freq_mhz_store);
>  static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
>  static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
>  static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
>  
>  /* For now we have a static number of RP states */
>  static ssize_t gt_rp_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> @@ -491,12 +425,20 @@ static ssize_t gt_rp_mhz_show(struct device
> *kdev, struct device_attribute *attr
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 val;
>  
> -	if (attr == &dev_attr_gt_RP0_freq_mhz)
> +	if (attr == &dev_attr_gt_cur_freq_mhz)
> +		val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.cur_freq);
> +	else if (attr == &dev_attr_gt_max_freq_mhz)
> +		val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.max_freq_softlimit);
> +	else if (attr == &dev_attr_gt_min_freq_mhz)
> +		val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.min_freq_softlimit);
> +	else if (attr == &dev_attr_gt_RP0_freq_mhz)
>  		val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.rp0_freq);
>  	else if (attr == &dev_attr_gt_RP1_freq_mhz)
>  		val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.rp1_freq);
>  	else if (attr == &dev_attr_gt_RPn_freq_mhz)
>  		val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.min_freq);
> +	else if (attr == &dev_attr_vlv_rpe_freq_mhz)
> +		val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.efficient_freq);
>  	else
>  		BUG();
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read

[Ville Syrjälä]

On Wed, Mar 16, 2016 at 08:07:03PM +0200, Imre Deak wrote:
> On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > cur_freq, min/max_freq_softlimit, efficient_freq are just single
> > values
> > stored under dev_priv.rps, so there's no real point in locking,
> > resuming
> > the devices and flushing the delayed resume work just to print them
> > out.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This makes things clearer, so would it make sense to still keep the
> flush wq and leave out patch 4 until somebody benchmarks things? That
> would address Tom's concern as well.

Yeah, at least that would get rid of the mutex + rpm. And I suppose I
should look at the debugfs side too.

> 
> > ---
> >  drivers/gpu/drm/i915/i915_sysfs.c | 84 ++++++-----------------------
> > ----------
> >  1 file changed, 13 insertions(+), 71 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> > b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 2d576b7ff299..9e39d7a18fdb 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -305,55 +305,6 @@ static ssize_t gt_act_freq_mhz_show(struct
> > device *kdev,
> >  	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> >  }
> >  
> > -static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> > -				    struct device_attribute *attr,
> > char *buf)
> > -{
> > -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> > -	struct drm_device *dev = minor->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int ret;
> > -
> > -	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> > -
> > -	intel_runtime_pm_get(dev_priv);
> > -
> > -	mutex_lock(&dev_priv->rps.hw_lock);
> > -	ret = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
> > -	mutex_unlock(&dev_priv->rps.hw_lock);
> > -
> > -	intel_runtime_pm_put(dev_priv);
> > -
> > -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> > -}
> > -
> > -static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
> > -				     struct device_attribute *attr,
> > char *buf)
> > -{
> > -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> > -	struct drm_device *dev = minor->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > -	return snprintf(buf, PAGE_SIZE,
> > -			"%d\n",
> > -			intel_gpu_freq(dev_priv, dev_priv-
> > >rps.efficient_freq));
> > -}
> > -
> > -static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct
> > device_attribute *attr, char *buf)
> > -{
> > -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> > -	struct drm_device *dev = minor->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int ret;
> > -
> > -	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> > -
> > -	mutex_lock(&dev_priv->rps.hw_lock);
> > -	ret = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.max_freq_softlimit);
> > -	mutex_unlock(&dev_priv->rps.hw_lock);
> > -
> > -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> > -}
> > -
> >  static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> >  				     struct device_attribute *attr,
> >  				     const char *buf, size_t count)
> > @@ -406,22 +357,6 @@ static ssize_t gt_max_freq_mhz_store(struct
> > device *kdev,
> >  	return count;
> >  }
> >  
> > -static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct
> > device_attribute *attr, char *buf)
> > -{
> > -	struct drm_minor *minor = dev_to_drm_minor(kdev);
> > -	struct drm_device *dev = minor->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int ret;
> > -
> > -	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> > -
> > -	mutex_lock(&dev_priv->rps.hw_lock);
> > -	ret = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.min_freq_softlimit);
> > -	mutex_unlock(&dev_priv->rps.hw_lock);
> > -
> > -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> > -}
> > -
> >  static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> >  				     struct device_attribute *attr,
> >  				     const char *buf, size_t count)
> > @@ -472,16 +407,15 @@ static ssize_t gt_min_freq_mhz_store(struct
> > device *kdev,
> >  }
> >  
> >  static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show,
> > NULL);
> > -static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show,
> > NULL);
> > -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR,
> > gt_max_freq_mhz_show, gt_max_freq_mhz_store);
> > -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR,
> > gt_min_freq_mhz_show, gt_min_freq_mhz_store);
> > -
> > -static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show,
> > NULL);
> >  
> >  static ssize_t gt_rp_mhz_show(struct device *kdev, struct
> > device_attribute *attr, char *buf);
> > +static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> > +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR,
> > gt_rp_mhz_show, gt_max_freq_mhz_store);
> > +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR,
> > gt_rp_mhz_show, gt_min_freq_mhz_store);
> >  static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> >  static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> >  static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> > +static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> >  
> >  /* For now we have a static number of RP states */
> >  static ssize_t gt_rp_mhz_show(struct device *kdev, struct
> > device_attribute *attr, char *buf)
> > @@ -491,12 +425,20 @@ static ssize_t gt_rp_mhz_show(struct device
> > *kdev, struct device_attribute *attr
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	u32 val;
> >  
> > -	if (attr == &dev_attr_gt_RP0_freq_mhz)
> > +	if (attr == &dev_attr_gt_cur_freq_mhz)
> > +		val = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.cur_freq);
> > +	else if (attr == &dev_attr_gt_max_freq_mhz)
> > +		val = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.max_freq_softlimit);
> > +	else if (attr == &dev_attr_gt_min_freq_mhz)
> > +		val = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.min_freq_softlimit);
> > +	else if (attr == &dev_attr_gt_RP0_freq_mhz)
> >  		val = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.rp0_freq);
> >  	else if (attr == &dev_attr_gt_RP1_freq_mhz)
> >  		val = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.rp1_freq);
> >  	else if (attr == &dev_attr_gt_RPn_freq_mhz)
> >  		val = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.min_freq);
> > +	else if (attr == &dev_attr_vlv_rpe_freq_mhz)
> > +		val = intel_gpu_freq(dev_priv, dev_priv-
> > >rps.efficient_freq);
> >  	else
> >  		BUG();
> >  

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

✗ Fi.CI.BAT: failure for drm/i915: Some GT freq stuff

[Patchwork]

== Series Details ==

Series: drm/i915: Some GT freq stuff
URL   : https://patchwork.freedesktop.org/series/4129/
State : failure

== Summary ==

Series 4129v1 drm/i915: Some GT freq stuff
http://patchwork.freedesktop.org/api/1.0/series/4129/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (bsw-nuc-2)
                pass       -> DMESG-FAIL (hsw-brixbox)
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (hsw-gt2)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (hsw-gt2)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (hsw-brixbox)
        Subgroup read-crc-pipe-c:
                dmesg-warn -> PASS       (hsw-gt2)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (skl-i5k-2)
                skip       -> PASS       (hsw-brixbox)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (snb-dellxps)
Test pm_rps:
        Subgroup basic-api:
                pass       -> FAIL       (skl-i5k-2)

bdw-ultra        total:183  pass:165  dwarn:0   dfail:0   fail:0   skip:18 
bsw-nuc-2        total:183  pass:149  dwarn:0   dfail:0   fail:0   skip:34 
byt-nuc          total:183  pass:152  dwarn:0   dfail:0   fail:0   skip:31 
hsw-brixbox      total:183  pass:162  dwarn:1   dfail:1   fail:0   skip:19 
hsw-gt2          total:183  pass:168  dwarn:1   dfail:0   fail:0   skip:14 
ilk-hp8440p      total:183  pass:124  dwarn:1   dfail:0   fail:0   skip:58 
ivb-t430s        total:183  pass:162  dwarn:0   dfail:0   fail:0   skip:21 
skl-i5k-2        total:183  pass:162  dwarn:0   dfail:0   fail:1   skip:20 
skl-i7k-2        total:183  pass:163  dwarn:0   dfail:0   fail:0   skip:20 
snb-dellxps      total:183  pass:152  dwarn:2   dfail:0   fail:0   skip:29 

Results at /archive/results/CI_IGT_test/Patchwork_1530/

d369e0096716c6000139162b3b340f684f0a51da drm-intel-nightly: 2016y-03m-04d-17h-18m-08s UTC integration manifest
7e3aa23d98537d012a1af05c5fef19b28ba71404 drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read
d027c2e4920ffe7c0cf514c06327485376ef16ba drm/i915: Set GPU freq to idle_freq initially
83be894eede637336015c43f661341c05f236681 drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: Some GT freq stuff

[Ville Syrjälä]

On Mon, Mar 07, 2016 at 10:34:26AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Some GT freq stuff
> URL   : https://patchwork.freedesktop.org/series/4129/
> State : failure
> 
> == Summary ==
> 
> Series 4129v1 drm/i915: Some GT freq stuff
> http://patchwork.freedesktop.org/api/1.0/series/4129/revisions/1/mbox/
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE

ilk underrun
https://bugs.freedesktop.org/show_bug.cgi?id=93787

>         Subgroup basic-flip-vs-wf_vblank:
>                 fail       -> PASS       (bsw-nuc-2)
>                 pass       -> DMESG-FAIL (hsw-brixbox)


(kms_flip:6763) DEBUG: name = flip
last_ts = 333.745977 usec
last_received_ts = 333.745522 usec
last_seq = 1706
current_ts = 333.929216 usec
current_received_ts = 333.929426 usec
current_seq = 1716
count = 51
seq_step = 1
(kms_flip:6763) CRITICAL: Test assertion failure function check_state,
file kms_flip.c:692:
(kms_flip:6763) CRITICAL: Failed assertion: fabs((((double)
diff.tv_usec) - usec_interflip) / usec_interflip) <= 0.005
(kms_flip:6763) CRITICAL: Last errno: 25, Inappropriate ioctl for device
(kms_flip:6763) CRITICAL: inter-flip ts jitter: 0s, 183239usec
****  END  ****

This sort of looks like the off by one error between the vbl timestamp
and seq number
https://bugs.freedesktop.org/show_bug.cgi?id=94294
except here seq_step==1, so not sure why we ended up getting a
difference of 11 frames (based on ts, 10 according to seq). I've seen
this same thing before at least once before.

>         Subgroup basic-plain-flip:
>                 dmesg-warn -> PASS       (hsw-gt2)
> Test kms_frontbuffer_tracking:
>         Subgroup basic:
>                 pass       -> DMESG-WARN (hsw-gt2)

[  333.891630] Device suspended during HW access
[  333.891668]  [<ffffffffa03027ac>] hsw_write32+0x1dc/0x270 [i915]
[  333.891676]  [<ffffffffa02ab9c8>] _ilk_disable_lp_wm+0x98/0xd0 [i915]
https://bugs.freedesktop.org/show_bug.cgi?id=94349

> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-b-frame-sequence:
>                 pass       -> DMESG-WARN (hsw-brixbox)

same
[  346.472879]  [<ffffffffa03397eb>] hsw_write32+0x21b/0x270 [i915]
[  346.472888]  [<ffffffffa02e29c8>] _ilk_disable_lp_wm+0x98/0xd0 [i915]

>         Subgroup read-crc-pipe-c:
>                 dmesg-warn -> PASS       (hsw-gt2)
>         Subgroup suspend-read-crc-pipe-a:
>                 dmesg-warn -> PASS       (skl-i5k-2)
>                 skip       -> PASS       (hsw-brixbox)
> Test pm_rpm:
>         Subgroup basic-rte:
>                 pass       -> DMESG-WARN (snb-dellxps)

same
[  306.449231]  [<ffffffffa0329dec>] gen6_write32+0x1dc/0x270 [i915]
[  306.449244]  [<ffffffffa02d39c8>] _ilk_disable_lp_wm+0x98/0xd0 [i915]


> Test pm_rps:
>         Subgroup basic-api:
>                 pass       -> FAIL       (skl-i5k-2)

(pm_rps:6439) CRITICAL: Test assertion failure function do_writeval, file pm_rps.c:136:
(pm_rps:6439) CRITICAL: Failed assertion: readval(filp) == val
(pm_rps:6439) CRITICAL: Last errno: 22, Invalid argument
(pm_rps:6439) CRITICAL: error: 350 != 0

Hmm. This looks like the patches might have a problem. Need to
investigate more.

> 
> bdw-ultra        total:183  pass:165  dwarn:0   dfail:0   fail:0   skip:18 
> bsw-nuc-2        total:183  pass:149  dwarn:0   dfail:0   fail:0   skip:34 
> byt-nuc          total:183  pass:152  dwarn:0   dfail:0   fail:0   skip:31 
> hsw-brixbox      total:183  pass:162  dwarn:1   dfail:1   fail:0   skip:19 
> hsw-gt2          total:183  pass:168  dwarn:1   dfail:0   fail:0   skip:14 
> ilk-hp8440p      total:183  pass:124  dwarn:1   dfail:0   fail:0   skip:58 
> ivb-t430s        total:183  pass:162  dwarn:0   dfail:0   fail:0   skip:21 
> skl-i5k-2        total:183  pass:162  dwarn:0   dfail:0   fail:1   skip:20 
> skl-i7k-2        total:183  pass:163  dwarn:0   dfail:0   fail:0   skip:20 
> snb-dellxps      total:183  pass:152  dwarn:2   dfail:0   fail:0   skip:29 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1530/
> 
> d369e0096716c6000139162b3b340f684f0a51da drm-intel-nightly: 2016y-03m-04d-17h-18m-08s UTC integration manifest
> 7e3aa23d98537d012a1af05c5fef19b28ba71404 drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read
> d027c2e4920ffe7c0cf514c06327485376ef16ba drm/i915: Set GPU freq to idle_freq initially
> 83be894eede637336015c43f661341c05f236681 drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: Some GT freq stuff

[Ville Syrjälä]

On Mon, Mar 07, 2016 at 05:07:51PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 07, 2016 at 10:34:26AM -0000, Patchwork wrote:
> > == Series Details ==
> > 
> > Series: drm/i915: Some GT freq stuff
> > URL   : https://patchwork.freedesktop.org/series/4129/
> > State : failure
> > 
> > == Summary ==
> > 
> > Series 4129v1 drm/i915: Some GT freq stuff
> > http://patchwork.freedesktop.org/api/1.0/series/4129/revisions/1/mbox/
> > 
> > Test kms_flip:
> >         Subgroup basic-flip-vs-dpms:
> >                 pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
> 
> ilk underrun
> https://bugs.freedesktop.org/show_bug.cgi?id=93787
> 
> >         Subgroup basic-flip-vs-wf_vblank:
> >                 fail       -> PASS       (bsw-nuc-2)
> >                 pass       -> DMESG-FAIL (hsw-brixbox)
> 
> 
> (kms_flip:6763) DEBUG: name = flip
> last_ts = 333.745977 usec
> last_received_ts = 333.745522 usec
> last_seq = 1706
> current_ts = 333.929216 usec
> current_received_ts = 333.929426 usec
> current_seq = 1716
> count = 51
> seq_step = 1
> (kms_flip:6763) CRITICAL: Test assertion failure function check_state,
> file kms_flip.c:692:
> (kms_flip:6763) CRITICAL: Failed assertion: fabs((((double)
> diff.tv_usec) - usec_interflip) / usec_interflip) <= 0.005
> (kms_flip:6763) CRITICAL: Last errno: 25, Inappropriate ioctl for device
> (kms_flip:6763) CRITICAL: inter-flip ts jitter: 0s, 183239usec
> ****  END  ****
> 
> This sort of looks like the off by one error between the vbl timestamp
> and seq number
> https://bugs.freedesktop.org/show_bug.cgi?id=94294
> except here seq_step==1, so not sure why we ended up getting a
> difference of 11 frames (based on ts, 10 according to seq). I've seen
> this same thing before at least once before.
> 
> >         Subgroup basic-plain-flip:
> >                 dmesg-warn -> PASS       (hsw-gt2)
> > Test kms_frontbuffer_tracking:
> >         Subgroup basic:
> >                 pass       -> DMESG-WARN (hsw-gt2)
> 
> [  333.891630] Device suspended during HW access
> [  333.891668]  [<ffffffffa03027ac>] hsw_write32+0x1dc/0x270 [i915]
> [  333.891676]  [<ffffffffa02ab9c8>] _ilk_disable_lp_wm+0x98/0xd0 [i915]
> https://bugs.freedesktop.org/show_bug.cgi?id=94349
> 
> > Test kms_pipe_crc_basic:
> >         Subgroup nonblocking-crc-pipe-b-frame-sequence:
> >                 pass       -> DMESG-WARN (hsw-brixbox)
> 
> same
> [  346.472879]  [<ffffffffa03397eb>] hsw_write32+0x21b/0x270 [i915]
> [  346.472888]  [<ffffffffa02e29c8>] _ilk_disable_lp_wm+0x98/0xd0 [i915]
> 
> >         Subgroup read-crc-pipe-c:
> >                 dmesg-warn -> PASS       (hsw-gt2)
> >         Subgroup suspend-read-crc-pipe-a:
> >                 dmesg-warn -> PASS       (skl-i5k-2)
> >                 skip       -> PASS       (hsw-brixbox)
> > Test pm_rpm:
> >         Subgroup basic-rte:
> >                 pass       -> DMESG-WARN (snb-dellxps)
> 
> same
> [  306.449231]  [<ffffffffa0329dec>] gen6_write32+0x1dc/0x270 [i915]
> [  306.449244]  [<ffffffffa02d39c8>] _ilk_disable_lp_wm+0x98/0xd0 [i915]
> 
> 
> > Test pm_rps:
> >         Subgroup basic-api:
> >                 pass       -> FAIL       (skl-i5k-2)
> 
> (pm_rps:6439) CRITICAL: Test assertion failure function do_writeval, file pm_rps.c:136:
> (pm_rps:6439) CRITICAL: Failed assertion: readval(filp) == val
> (pm_rps:6439) CRITICAL: Last errno: 22, Invalid argument
> (pm_rps:6439) CRITICAL: error: 350 != 0
> 
> Hmm. This looks like the patches might have a problem. Need to
> investigate more.

OK, so I think with the flush_delayed_work()+locking gone, userspace
managed to read out the sysfs files before the delayed enable rps work
had populated the values. On VLV/CHV that can't happen since we populate
the values already before registering the sysfs files. I think I'll
change the gen6+ code to follow the same pattern.

This also highlighted a bug with the test itself. It failed to notice
that writing a bogus value to the sysfs file returned an error when we
weren't expecting one. I'll need to fix up the assert to check for the
right thing.

> 
> > 
> > bdw-ultra        total:183  pass:165  dwarn:0   dfail:0   fail:0   skip:18 
> > bsw-nuc-2        total:183  pass:149  dwarn:0   dfail:0   fail:0   skip:34 
> > byt-nuc          total:183  pass:152  dwarn:0   dfail:0   fail:0   skip:31 
> > hsw-brixbox      total:183  pass:162  dwarn:1   dfail:1   fail:0   skip:19 
> > hsw-gt2          total:183  pass:168  dwarn:1   dfail:0   fail:0   skip:14 
> > ilk-hp8440p      total:183  pass:124  dwarn:1   dfail:0   fail:0   skip:58 
> > ivb-t430s        total:183  pass:162  dwarn:0   dfail:0   fail:0   skip:21 
> > skl-i5k-2        total:183  pass:162  dwarn:0   dfail:0   fail:1   skip:20 
> > skl-i7k-2        total:183  pass:163  dwarn:0   dfail:0   fail:0   skip:20 
> > snb-dellxps      total:183  pass:152  dwarn:2   dfail:0   fail:0   skip:29 
> > 
> > Results at /archive/results/CI_IGT_test/Patchwork_1530/
> > 
> > d369e0096716c6000139162b3b340f684f0a51da drm-intel-nightly: 2016y-03m-04d-17h-18m-08s UTC integration manifest
> > 7e3aa23d98537d012a1af05c5fef19b28ba71404 drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read
> > d027c2e4920ffe7c0cf514c06327485376ef16ba drm/i915: Set GPU freq to idle_freq initially
> > 83be894eede637336015c43f661341c05f236681 drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 4/3] drm/i915: Read RPS frequencies earlier on non-VLV/CHV

[ville.syrjala]

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

Read out the RPS frequencies already in intel_init_gt_powersave()
on all the platforms. So far we only did that on VLV/CHV, and the
rest of the platforms read them out at rps enable time, which happens
asynchronously from a workqueue. Reading them out earlier prevents
userspace from reading out invalid (zero) values via the relevant
sysfs files before the rps enable work has been executed.

This used to be prevented by the flush_delayed_work() + locking
in the sysfs code, but now that we no longer do that, we run the
risk of letting userspace see the initial zeroed values.

Note that it's still possible to read out cur_freq as 0, since that
only gets initialized from the delayed rps enable. Should that pose
a real problem, I guess we could always add the flush+locking back
for the cur_freq read.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2591d533a895..5fe098b94f06 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4782,8 +4782,6 @@ static void gen9_enable_rps(struct drm_device *dev)
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
-	gen6_init_rps_frequencies(dev);
-
 	/* WaGsvDisableTurbo: Workaround to disable turbo on BXT A* */
 	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
 		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
@@ -4896,9 +4894,6 @@ static void gen8_enable_rps(struct drm_device *dev)
 	/* 2a: Disable RC states. */
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 
-	/* Initialize rps frequencies */
-	gen6_init_rps_frequencies(dev);
-
 	/* 2b: Program RC6 thresholds.*/
 	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
@@ -4988,9 +4983,6 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
-	/* Initialize rps frequencies */
-	gen6_init_rps_frequencies(dev);
-
 	/* disable the counters and set deterministic thresholds */
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 
@@ -6197,6 +6189,8 @@ void intel_init_gt_powersave(struct drm_device *dev)
 		cherryview_init_gt_powersave(dev);
 	else if (IS_VALLEYVIEW(dev))
 		valleyview_init_gt_powersave(dev);
+	else if (INTEL_INFO(dev_priv)->gen >= 6)
+		gen6_init_rps_frequencies(dev);
 }
 
 void intel_cleanup_gt_powersave(struct drm_device *dev)
-- 
2.4.10

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

Re: [PATCH 4/3] drm/i915: Read RPS frequencies earlier on non-VLV/CHV

[O'Rourke, Tom]

On Mon, Mar 07, 2016 at 08:57:09PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Read out the RPS frequencies already in intel_init_gt_powersave()
> on all the platforms. So far we only did that on VLV/CHV, and the
> rest of the platforms read them out at rps enable time, which happens
> asynchronously from a workqueue. Reading them out earlier prevents
> userspace from reading out invalid (zero) values via the relevant
> sysfs files before the rps enable work has been executed.
> 
> This used to be prevented by the flush_delayed_work() + locking
> in the sysfs code, but now that we no longer do that, we run the
> risk of letting userspace see the initial zeroed values.
> 
> Note that it's still possible to read out cur_freq as 0, since that
> only gets initialized from the delayed rps enable. Should that pose
> a real problem, I guess we could always add the flush+locking back
> for the cur_freq read.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

Hello,

The flush_delayed_work() calls were added in:
"commit 5c9669cee534cbb834d51aae115267f5e561b622
Author: Tom O'Rourke <Tom.O'Rourke@intel.com>
Date:   Mon Sep 16 14:56:43 2013 -0700

    drm/i915: Finish enabling rps before use by sysfs or debugfs"

This change was made to address use before initialization
problems in min/max/cur frequency values.  The work queue
being flushed was added in:
"commit 1a01ab3b2dc4394c46c4c3230805748f632f6f74
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Fri Nov 2 11:14:00 2012 -0700

    drm/i915: put ring frequency and turbo setup into a work queue v5

    Communicating via the mailbox registers with the PCU can take quite
    awhile.  And updating the ring frequency or enabling turbo is not
    something that needs to happen synchronously, so take it out of our init
    and resume paths to speed things up (~200ms on my T420)."

This change was made to reduce driver load times.

The two concerns I have are:
1) Similar changes would be needed in debugfs files.
2) This change may increase driver load times due to pcode
mailbox command used in gen6_rps_init_frequencies() that
is now in the init path.

I am not objecting to these changes but we should
be aware of the impact to driver load latency.

Thanks,
Tom

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

Re: [PATCH 4/3] drm/i915: Read RPS frequencies earlier on non-VLV/CHV

[Ville Syrjälä]

On Mon, Mar 07, 2016 at 04:49:14PM -0800, O'Rourke, Tom wrote:
> On Mon, Mar 07, 2016 at 08:57:09PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Read out the RPS frequencies already in intel_init_gt_powersave()
> > on all the platforms. So far we only did that on VLV/CHV, and the
> > rest of the platforms read them out at rps enable time, which happens
> > asynchronously from a workqueue. Reading them out earlier prevents
> > userspace from reading out invalid (zero) values via the relevant
> > sysfs files before the rps enable work has been executed.
> > 
> > This used to be prevented by the flush_delayed_work() + locking
> > in the sysfs code, but now that we no longer do that, we run the
> > risk of letting userspace see the initial zeroed values.
> > 
> > Note that it's still possible to read out cur_freq as 0, since that
> > only gets initialized from the delayed rps enable. Should that pose
> > a real problem, I guess we could always add the flush+locking back
> > for the cur_freq read.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> 
> Hello,
> 
> The flush_delayed_work() calls were added in:
> "commit 5c9669cee534cbb834d51aae115267f5e561b622
> Author: Tom O'Rourke <Tom.O'Rourke@intel.com>
> Date:   Mon Sep 16 14:56:43 2013 -0700
> 
>     drm/i915: Finish enabling rps before use by sysfs or debugfs"
> 
> This change was made to address use before initialization
> problems in min/max/cur frequency values.  The work queue
> being flushed was added in:
> "commit 1a01ab3b2dc4394c46c4c3230805748f632f6f74
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Fri Nov 2 11:14:00 2012 -0700
> 
>     drm/i915: put ring frequency and turbo setup into a work queue v5
> 
>     Communicating via the mailbox registers with the PCU can take quite
>     awhile.  And updating the ring frequency or enabling turbo is not
>     something that needs to happen synchronously, so take it out of our init
>     and resume paths to speed things up (~200ms on my T420)."
> 
> This change was made to reduce driver load times.
> 
> The two concerns I have are:
> 1) Similar changes would be needed in debugfs files.
> 2) This change may increase driver load times due to pcode
> mailbox command used in gen6_rps_init_frequencies() that
> is now in the init path.
> 
> I am not objecting to these changes but we should
> be aware of the impact to driver load latency.

If a single pcode command takes singificant amount of time there must
be something seriously wrong. TBH I never really bought the whole two
stage rps enable thing. Seemed like extra complexity for little gain.
Someone should definitely measure it again though, especially after
commit 9848de082ff4 ("drm/i915: Use usleep_range() in wait_for()")

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