[PATCH 1/2] drm/i915: Correct the i915_frequency_info debugfs output

[PATCH 1/2] drm/i915: Correct the i915_frequency_info debugfs output

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

There are certain registers, which captures the time elapsed in the
in current Up/Down EI, for how long GT has been Idle/Busy/Avg in the
current Up/Down EI and also in the previous Up/Down EI.
These register values are reported by the i915_frequency_info debugfs
interface. The Driver prints the 'us' suffix after the values, albeit
they are actually in raw form & not in microsecond units.
This patch removes the 'us' suffix so that its clear to User that values
are indeed in raw form.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 931dc60..10d095a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1291,20 +1291,20 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 		seq_printf(m, "RPDECLIMIT: 0x%08x\n", rpdeclimit);
 		seq_printf(m, "RPNSWREQ: %dMHz\n", reqf);
 		seq_printf(m, "CAGF: %dMHz\n", cagf);
-		seq_printf(m, "RP CUR UP EI: %dus\n", rpupei &
+		seq_printf(m, "RP CUR UP EI: %d\n", rpupei &
 			   GEN6_CURICONT_MASK);
-		seq_printf(m, "RP CUR UP: %dus\n", rpcurup &
+		seq_printf(m, "RP CUR UP: %d\n", rpcurup &
 			   GEN6_CURBSYTAVG_MASK);
-		seq_printf(m, "RP PREV UP: %dus\n", rpprevup &
+		seq_printf(m, "RP PREV UP: %d\n", rpprevup &
 			   GEN6_CURBSYTAVG_MASK);
 		seq_printf(m, "Up threshold: %d%%\n",
 			   dev_priv->rps.up_threshold);
 
-		seq_printf(m, "RP CUR DOWN EI: %dus\n", rpdownei &
+		seq_printf(m, "RP CUR DOWN EI: %d\n", rpdownei &
 			   GEN6_CURIAVG_MASK);
-		seq_printf(m, "RP CUR DOWN: %dus\n", rpcurdown &
+		seq_printf(m, "RP CUR DOWN: %d\n", rpcurdown &
 			   GEN6_CURBSYTAVG_MASK);
-		seq_printf(m, "RP PREV DOWN: %dus\n", rpprevdown &
+		seq_printf(m, "RP PREV DOWN: %d\n", rpprevdown &
 			   GEN6_CURBSYTAVG_MASK);
 		seq_printf(m, "Down threshold: %d%%\n",
 			   dev_priv->rps.down_threshold);
-- 
1.9.2

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

Re: [PATCH 1/2] drm/i915: Correct the i915_frequency_info debugfs output

[Chris Wilson]

On Thu, Apr 21, 2016 at 12:33:40PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> There are certain registers, which captures the time elapsed in the
> in current Up/Down EI, for how long GT has been Idle/Busy/Avg in the
> current Up/Down EI and also in the previous Up/Down EI.
> These register values are reported by the i915_frequency_info debugfs
> interface. The Driver prints the 'us' suffix after the values, albeit
> they are actually in raw form & not in microsecond units.
> This patch removes the 'us' suffix so that its clear to User that values
> are indeed in raw form.

Or whilst you are here presenting them as both the raw value and as
microseconds?
-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

Re: [PATCH 1/2] drm/i915: Correct the i915_frequency_info debugfs output

[Goel, Akash]

On 4/21/2016 12:24 PM, Chris Wilson wrote:
> On Thu, Apr 21, 2016 at 12:33:40PM +0530, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@intel.com>
>>
>> There are certain registers, which captures the time elapsed in the
>> in current Up/Down EI, for how long GT has been Idle/Busy/Avg in the
>> current Up/Down EI and also in the previous Up/Down EI.
>> These register values are reported by the i915_frequency_info debugfs
>> interface. The Driver prints the 'us' suffix after the values, albeit
>> they are actually in raw form & not in microsecond units.
>> This patch removes the 'us' suffix so that its clear to User that values
>> are indeed in raw form.
>
> Or whilst you are here presenting them as both the raw value and as
> microseconds?

Ok, so need to present them in microseconds unit also (after platform 
specific conversion) apart from raw form.

Best Regards
Akash

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

[PATCH 1/3] drm/i915: Macros to convert PM time interval values to microseconds

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Added a new GT_PM_INTERVAL_TO_US macro to perform the platform
specific conversion of PM time interval values to microseconds unit.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9464ba3..eacd9ae 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2939,6 +2939,15 @@ enum skl_disp_power_wells {
 				INTERVAL_1_33_US(us)) : \
 				INTERVAL_1_28_US(us))
 
+#define INTERVAL_1_28_TO_US(interval)  (((interval) << 7) / 100)
+#define INTERVAL_1_33_TO_US(interval)  (((interval) << 2) / 3)
+#define INTERVAL_0_833_TO_US(interval) (((interval) * 5)  / 6)
+#define GT_PM_INTERVAL_TO_US(dev_priv, interval) (IS_GEN9(dev_priv) ? \
+                           (IS_BROXTON(dev_priv) ? \
+                           INTERVAL_0_833_TO_US(interval) : \
+                           INTERVAL_1_33_TO_US(interval)) : \
+                           INTERVAL_1_28_TO_US(interval))
+
 /*
  * Logical Context regs
  */
-- 
1.9.2

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

[PATCH v2 2/3] drm/i915: Correct the i915_frequency_info debugfs output

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

There are certain registers, which captures the time elapsed in the
in current Up/Down EI, for how long GT has been Idle/Busy/Avg in the
current Up/Down EI and also in the previous Up/Down EI.
These register values are reported by the i915_frequency_info debugfs
interface. The Driver prints the 'us' suffix after the values, albeit
they are actually in raw form & not in microsecond units.
This patch removes the 'us' suffix so that its clear to User that values
are indeed in raw form.

v2: Present the values in microseconds unit also, after platform
    specific conversion (Chris)

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 931dc60..c09f63a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1246,12 +1246,12 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 		rpdeclimit = I915_READ(GEN6_RP_DOWN_THRESHOLD);
 
 		rpstat = I915_READ(GEN6_RPSTAT1);
-		rpupei = I915_READ(GEN6_RP_CUR_UP_EI);
-		rpcurup = I915_READ(GEN6_RP_CUR_UP);
-		rpprevup = I915_READ(GEN6_RP_PREV_UP);
-		rpdownei = I915_READ(GEN6_RP_CUR_DOWN_EI);
-		rpcurdown = I915_READ(GEN6_RP_CUR_DOWN);
-		rpprevdown = I915_READ(GEN6_RP_PREV_DOWN);
+		rpupei = I915_READ(GEN6_RP_CUR_UP_EI) & GEN6_CURICONT_MASK;
+		rpcurup = I915_READ(GEN6_RP_CUR_UP) & GEN6_CURBSYTAVG_MASK;
+		rpprevup = I915_READ(GEN6_RP_PREV_UP) & GEN6_CURBSYTAVG_MASK;
+		rpdownei = I915_READ(GEN6_RP_CUR_DOWN_EI) & GEN6_CURIAVG_MASK;
+		rpcurdown = I915_READ(GEN6_RP_CUR_DOWN) & GEN6_CURBSYTAVG_MASK;
+		rpprevdown = I915_READ(GEN6_RP_PREV_DOWN) & GEN6_CURBSYTAVG_MASK;
 		if (IS_GEN9(dev))
 			cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT;
 		else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -1291,21 +1291,21 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 		seq_printf(m, "RPDECLIMIT: 0x%08x\n", rpdeclimit);
 		seq_printf(m, "RPNSWREQ: %dMHz\n", reqf);
 		seq_printf(m, "CAGF: %dMHz\n", cagf);
-		seq_printf(m, "RP CUR UP EI: %dus\n", rpupei &
-			   GEN6_CURICONT_MASK);
-		seq_printf(m, "RP CUR UP: %dus\n", rpcurup &
-			   GEN6_CURBSYTAVG_MASK);
-		seq_printf(m, "RP PREV UP: %dus\n", rpprevup &
-			   GEN6_CURBSYTAVG_MASK);
+		seq_printf(m, "RP CUR UP EI: %d(%dus)\n",
+			   rpupei, GT_PM_INTERVAL_TO_US(dev_priv, rpupei));
+		seq_printf(m, "RP CUR UP: %d(%dus)\n",
+			   rpcurup, GT_PM_INTERVAL_TO_US(dev_priv, rpcurup));
+		seq_printf(m, "RP PREV UP: %d(%dus)\n",
+			   rpprevup, GT_PM_INTERVAL_TO_US(dev_priv, rpprevup));
 		seq_printf(m, "Up threshold: %d%%\n",
 			   dev_priv->rps.up_threshold);
 
-		seq_printf(m, "RP CUR DOWN EI: %dus\n", rpdownei &
-			   GEN6_CURIAVG_MASK);
-		seq_printf(m, "RP CUR DOWN: %dus\n", rpcurdown &
-			   GEN6_CURBSYTAVG_MASK);
-		seq_printf(m, "RP PREV DOWN: %dus\n", rpprevdown &
-			   GEN6_CURBSYTAVG_MASK);
+		seq_printf(m, "RP CUR DOWN EI: %d(%dus)\n",
+			   rpdownei, GT_PM_INTERVAL_TO_US(dev_priv, rpdownei));
+		seq_printf(m, "RP CUR DOWN: %d(%dus)\n",
+			   rpcurdown, GT_PM_INTERVAL_TO_US(dev_priv, rpcurdown));
+		seq_printf(m, "RP PREV DOWN: %d(%dus)\n",
+			   rpprevdown, GT_PM_INTERVAL_TO_US(dev_priv, rpprevdown));
 		seq_printf(m, "Down threshold: %d%%\n",
 			   dev_priv->rps.down_threshold);
 
-- 
1.9.2

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

Re: [PATCH v2 2/3] drm/i915: Correct the i915_frequency_info debugfs output

[Chris Wilson]

On Fri, Apr 22, 2016 at 05:22:25PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> There are certain registers, which captures the time elapsed in the
> in current Up/Down EI, for how long GT has been Idle/Busy/Avg in the
> current Up/Down EI and also in the previous Up/Down EI.
> These register values are reported by the i915_frequency_info debugfs
> interface. The Driver prints the 'us' suffix after the values, albeit
> they are actually in raw form & not in microsecond units.
> This patch removes the 'us' suffix so that its clear to User that values
> are indeed in raw form.
> 
> v2: Present the values in microseconds unit also, after platform
>     specific conversion (Chris)
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> @@ -1291,21 +1291,21 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>  		seq_printf(m, "RPDECLIMIT: 0x%08x\n", rpdeclimit);
>  		seq_printf(m, "RPNSWREQ: %dMHz\n", reqf);
>  		seq_printf(m, "CAGF: %dMHz\n", cagf);
> -		seq_printf(m, "RP CUR UP EI: %dus\n", rpupei &
> -			   GEN6_CURICONT_MASK);
> -		seq_printf(m, "RP CUR UP: %dus\n", rpcurup &
> -			   GEN6_CURBSYTAVG_MASK);
> -		seq_printf(m, "RP PREV UP: %dus\n", rpprevup &
> -			   GEN6_CURBSYTAVG_MASK);
> +		seq_printf(m, "RP CUR UP EI: %d(%dus)\n",
> +			   rpupei, GT_PM_INTERVAL_TO_US(dev_priv, rpupei));

A space between raw and time would have been nice.

With that extra character,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 v2 3/3] drm/i915/bxt: Explicitly clear the Turbo control register

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

As a part of WaGsvDisableTurbo, Driver makes an early exit from the
Gen9 Turbo enabling function, so doesn't program the Turbo Control register.
But BIOS could leave the Hw Turbo as enabled, so need to explicitly clear
out the Control register just to avoid inconsitency with debugfs
interface, which will show Turbo as enabled only and that is not expected
after adding the WaGsvDisableTurbo. Apart from this there is no problem
even if the Turbo is left enabled in the Control register, as the Up/Down
interrupts would remain masked.

v2: Add explicit clearing of Turbo Control register to *_disable_rps()
    also for the similar consistency (Chris)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7c2186..695a464 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4587,7 +4587,7 @@ void intel_set_rps(struct drm_device *dev, u8 val)
 		gen6_set_rps(dev, val);
 }
 
-static void gen9_disable_rps(struct drm_device *dev)
+static void gen9_disable_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -4595,12 +4595,20 @@ static void gen9_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN9_PG_ENABLE, 0);
 }
 
+static void gen9_disable_rps(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(GEN6_RP_CONTROL, 0);
+}
+
 static void gen6_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
+	I915_WRITE(GEN6_RP_CONTROL, 0);
 }
 
 static void cherryview_disable_rps(struct drm_device *dev)
@@ -4804,6 +4812,16 @@ static void gen9_enable_rps(struct drm_device *dev)
 
 	/* WaGsvDisableTurbo: Workaround to disable turbo on BXT A* */
 	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
+		/*
+		 * BIOS could leave the Hw Turbo enabled, so need to explicitly
+		 * clear out the Control register just to avoid inconsitency
+		 * with debugfs interface, which will show  Turbo as enabled
+		 * only and that is not expected by the User after adding the
+		 * WaGsvDisableTurbo. Apart from this there is no problem even
+		 * if the Turbo is left enabled in the Control register, as the
+		 * Up/Down interrupts would remain masked.
+		 */
+		gen9_disable_rps(dev);
 		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 		return;
 	}
@@ -6268,9 +6286,10 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 		intel_suspend_gt_powersave(dev);
 
 		mutex_lock(&dev_priv->rps.hw_lock);
-		if (INTEL_INFO(dev)->gen >= 9)
+		if (INTEL_INFO(dev)->gen >= 9) {
+			gen9_disable_rc6(dev);
 			gen9_disable_rps(dev);
-		else if (IS_CHERRYVIEW(dev))
+		} else if (IS_CHERRYVIEW(dev))
 			cherryview_disable_rps(dev);
 		else if (IS_VALLEYVIEW(dev))
 			valleyview_disable_rps(dev);
-- 
1.9.2

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

Re: [PATCH 1/3] drm/i915: Macros to convert PM time interval values to microseconds

[Chris Wilson]

On Fri, Apr 22, 2016 at 05:22:24PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Added a new GT_PM_INTERVAL_TO_US macro to perform the platform
> specific conversion of PM time interval values to microseconds unit.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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

Re: [PATCH 1/3] drm/i915: Macros to convert PM time interval values to microseconds

[Daniel Vetter]

On Fri, Apr 22, 2016 at 05:22:24PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Added a new GT_PM_INTERVAL_TO_US macro to perform the platform
> specific conversion of PM time interval values to microseconds unit.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>

Note that if you submit a new series in-reply to a semirandom existing
one, CI/Patchwork will not pick it up correctly. in-reply-to is just for
resending individual patches in a series, as a reply to the corresponding
patch in the original one. If you add/remove/change the numbers of
patches, it needs to be a new one.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9464ba3..eacd9ae 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2939,6 +2939,15 @@ enum skl_disp_power_wells {
>  				INTERVAL_1_33_US(us)) : \
>  				INTERVAL_1_28_US(us))
>  
> +#define INTERVAL_1_28_TO_US(interval)  (((interval) << 7) / 100)
> +#define INTERVAL_1_33_TO_US(interval)  (((interval) << 2) / 3)
> +#define INTERVAL_0_833_TO_US(interval) (((interval) * 5)  / 6)
> +#define GT_PM_INTERVAL_TO_US(dev_priv, interval) (IS_GEN9(dev_priv) ? \
> +                           (IS_BROXTON(dev_priv) ? \
> +                           INTERVAL_0_833_TO_US(interval) : \
> +                           INTERVAL_1_33_TO_US(interval)) : \
> +                           INTERVAL_1_28_TO_US(interval))
> +
>  /*
>   * Logical Context regs
>   */
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 2/2] drm/i915/bxt: Explicitly clear the Turbo control register

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

As a part of WaGsvDisableTurbo, Driver makes an early exit from the
Gen9 Turbo enabling function, so doesn't program the Turbo Control register.
But BIOS could leave the Hw Turbo as enabled, so need to explicitly clear
out the Control register just to avoid inconsitency with debugfs
interface, which will show Turbo as enabled only and that is not expected
after adding the WaGsvDisableTurbo. Apart from this there is no problem
even if the Turbo is left enabled in the Control register, as the Up/Down
interrupts would remain masked.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7c2186..41d5586 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4804,6 +4804,16 @@ static void gen9_enable_rps(struct drm_device *dev)
 
 	/* WaGsvDisableTurbo: Workaround to disable turbo on BXT A* */
 	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
+		/*
+		 * BIOS could leave the Hw Turbo enabled, so need to explicitly
+		 * clear out the Control register just to avoid inconsitency
+		 * with debugfs interface, which will show  Turbo as enabled
+		 * only and that is not expected by the User after adding the
+		 * WaGsvDisableTurbo. Apart from this there is no problem even
+		 * if the Turbo is left enabled in the Control register, as the
+		 * Up/Down interrupts would remain masked.
+		 */
+		I915_WRITE(GEN6_RP_CONTROL, 0);
 		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 		return;
 	}
-- 
1.9.2

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

Re: [PATCH 2/2] drm/i915/bxt: Explicitly clear the Turbo control register

[Chris Wilson]

On Thu, Apr 21, 2016 at 12:33:41PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> As a part of WaGsvDisableTurbo, Driver makes an early exit from the
> Gen9 Turbo enabling function, so doesn't program the Turbo Control register.
> But BIOS could leave the Hw Turbo as enabled, so need to explicitly clear
> out the Control register just to avoid inconsitency with debugfs
> interface, which will show Turbo as enabled only and that is not expected
> after adding the WaGsvDisableTurbo. Apart from this there is no problem
> even if the Turbo is left enabled in the Control register, as the Up/Down
> interrupts would remain masked.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b7c2186..41d5586 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4804,6 +4804,16 @@ static void gen9_enable_rps(struct drm_device *dev)
>  
>  	/* WaGsvDisableTurbo: Workaround to disable turbo on BXT A* */
>  	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> +		/*
> +		 * BIOS could leave the Hw Turbo enabled, so need to explicitly
> +		 * clear out the Control register just to avoid inconsitency
> +		 * with debugfs interface, which will show  Turbo as enabled
> +		 * only and that is not expected by the User after adding the
> +		 * WaGsvDisableTurbo. Apart from this there is no problem even
> +		 * if the Turbo is left enabled in the Control register, as the
> +		 * Up/Down interrupts would remain masked.
> +		 */
> +		I915_WRITE(GEN6_RP_CONTROL, 0);

First question. Do we not want to sanitize bios state for everybody
during init? That is the usual procedure.

None of the *_disable_rps() actually clear GEN6_RP_CONTROL. They should
according to the rationale above.  And then this should be a call to
gen9_disable_rps().
-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

Re: [PATCH 2/2] drm/i915/bxt: Explicitly clear the Turbo control register

[Goel, Akash]

On 4/21/2016 1:07 PM, Chris Wilson wrote:
> On Thu, Apr 21, 2016 at 12:33:41PM +0530, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@intel.com>
>>
>> As a part of WaGsvDisableTurbo, Driver makes an early exit from the
>> Gen9 Turbo enabling function, so doesn't program the Turbo Control register.
>> But BIOS could leave the Hw Turbo as enabled, so need to explicitly clear
>> out the Control register just to avoid inconsitency with debugfs
>> interface, which will show Turbo as enabled only and that is not expected
>> after adding the WaGsvDisableTurbo. Apart from this there is no problem
>> even if the Turbo is left enabled in the Control register, as the Up/Down
>> interrupts would remain masked.
>>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index b7c2186..41d5586 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4804,6 +4804,16 @@ static void gen9_enable_rps(struct drm_device *dev)
>>
>>   	/* WaGsvDisableTurbo: Workaround to disable turbo on BXT A* */
>>   	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
>> +		/*
>> +		 * BIOS could leave the Hw Turbo enabled, so need to explicitly
>> +		 * clear out the Control register just to avoid inconsitency
>> +		 * with debugfs interface, which will show  Turbo as enabled
>> +		 * only and that is not expected by the User after adding the
>> +		 * WaGsvDisableTurbo. Apart from this there is no problem even
>> +		 * if the Turbo is left enabled in the Control register, as the
>> +		 * Up/Down interrupts would remain masked.
>> +		 */
>> +		I915_WRITE(GEN6_RP_CONTROL, 0);
>
> First question. Do we not want to sanitize bios state for everybody
> during init? That is the usual procedure.
>
Sorry I am not sure. There is a sanitize for certain module parameters.
Recently for RC6 on BXT, sanitize was done depending upon the BIOS state.

> None of the *_disable_rps() actually clear GEN6_RP_CONTROL. They should
> according to the rationale above.  And then this should be a call to
> gen9_disable_rps().

Ok so will add the clearing of GEN6_RP_CONTROL in all *_disable_rps() 
and then call gen9_disable_rps().

Best regards
Akash

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Correct the i915_frequency_info debugfs output

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Correct the i915_frequency_info debugfs output
URL   : https://patchwork.freedesktop.org/series/6044/
State : success

== Summary ==

Series 6044v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6044/revisions/1/mbox/


bdw-ultra        total:194  pass:170  dwarn:0   dfail:0   fail:1   skip:23 
bsw-nuc-2        total:193  pass:153  dwarn:0   dfail:0   fail:0   skip:40 
byt-nuc          total:193  pass:155  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:194  pass:170  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:194  pass:175  dwarn:0   dfail:0   fail:0   skip:19 
ilk-hp8440p      total:194  pass:137  dwarn:0   dfail:0   fail:0   skip:57 
ivb-t430s        total:194  pass:166  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:194  pass:168  dwarn:0   dfail:0   fail:1   skip:25 
skl-nuci5        total:194  pass:183  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:143  pass:117  dwarn:0   dfail:0   fail:0   skip:25 
snb-x220t        total:193  pass:154  dwarn:0   dfail:0   fail:1   skip:38 
bdw-nuci7 failed to collect. IGT log at Patchwork_1974/bdw-nuci7/igt.log

Results at /archive/results/CI_IGT_test/Patchwork_1974/

9dabb0053b63bc32ab6ad5d13209d1e43395313f drm-intel-nightly: 2016y-04m-21d-09h-27m-12s UTC integration manifest
17ef1c9 drm/i915/bxt: Explicitly clear the Turbo control register
0f530cc drm/i915: Correct the i915_frequency_info debugfs output

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

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Correct the i915_frequency_info debugfs output

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Correct the i915_frequency_info debugfs output
URL   : https://patchwork.freedesktop.org/series/6044/
State : warning

== Summary ==

Series 6044v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6044/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (ivb-t430s)
        Subgroup force-load-detect:
                pass       -> SKIP       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                pass       -> SKIP       (bdw-nuci7)

bdw-nuci7        total:193  pass:180  dwarn:0   dfail:0   fail:0   skip:13 
bdw-ultra        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:192  pass:153  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:192  pass:154  dwarn:0   dfail:0   fail:0   skip:38 
ilk-hp8440p      total:193  pass:136  dwarn:0   dfail:0   fail:0   skip:57 
ivb-t430s        total:193  pass:163  dwarn:0   dfail:0   fail:0   skip:30 
skl-i7k-2        total:193  pass:168  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:193  pass:182  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:193  pass:155  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:193  pass:155  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_2017/

340c485ad98d0ec0369a3b18d4a09938f3f5537d drm-intel-nightly: 2016y-04m-22d-17h-32m-25s UTC integration manifest
d575dcc drm/i915/bxt: Explicitly clear the Turbo control register
1dc39b2 drm/i915: Correct the i915_frequency_info debugfs output

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

[PATCH 1/3] drm/i915: Macros to convert PM time interval values to microseconds

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Added a new GT_PM_INTERVAL_TO_US macro to perform the platform
specific conversion of PM time interval values to microseconds unit.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9464ba3..eacd9ae 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2939,6 +2939,15 @@ enum skl_disp_power_wells {
 				INTERVAL_1_33_US(us)) : \
 				INTERVAL_1_28_US(us))
 
+#define INTERVAL_1_28_TO_US(interval)  (((interval) << 7) / 100)
+#define INTERVAL_1_33_TO_US(interval)  (((interval) << 2) / 3)
+#define INTERVAL_0_833_TO_US(interval) (((interval) * 5)  / 6)
+#define GT_PM_INTERVAL_TO_US(dev_priv, interval) (IS_GEN9(dev_priv) ? \
+                           (IS_BROXTON(dev_priv) ? \
+                           INTERVAL_0_833_TO_US(interval) : \
+                           INTERVAL_1_33_TO_US(interval)) : \
+                           INTERVAL_1_28_TO_US(interval))
+
 /*
  * Logical Context regs
  */
-- 
1.9.2

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

Re: [PATCH 1/3] drm/i915: Macros to convert PM time interval values to microseconds

[Daniel Vetter]

On Sat, Apr 23, 2016 at 12:05:44AM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Added a new GT_PM_INTERVAL_TO_US macro to perform the platform
> specific conversion of PM time interval values to microseconds unit.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9464ba3..eacd9ae 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2939,6 +2939,15 @@ enum skl_disp_power_wells {
>  				INTERVAL_1_33_US(us)) : \
>  				INTERVAL_1_28_US(us))
>  
> +#define INTERVAL_1_28_TO_US(interval)  (((interval) << 7) / 100)
> +#define INTERVAL_1_33_TO_US(interval)  (((interval) << 2) / 3)
> +#define INTERVAL_0_833_TO_US(interval) (((interval) * 5)  / 6)
> +#define GT_PM_INTERVAL_TO_US(dev_priv, interval) (IS_GEN9(dev_priv) ? \
> +                           (IS_BROXTON(dev_priv) ? \
> +                           INTERVAL_0_833_TO_US(interval) : \
> +                           INTERVAL_1_33_TO_US(interval)) : \
> +                           INTERVAL_1_28_TO_US(interval))

Bikeshed, but I think once we have platform logic in there a real inline
functino starts to look better.

Feel free to ignore since already pushed.
-Daniel

> +
>  /*
>   * Logical Context regs
>   */
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/3] drm/i915: Macros to convert PM time interval values to microseconds

[Chris Wilson]

On Tue, Apr 26, 2016 at 04:06:05PM +0200, Daniel Vetter wrote:
> On Sat, Apr 23, 2016 at 12:05:44AM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > Added a new GT_PM_INTERVAL_TO_US macro to perform the platform
> > specific conversion of PM time interval values to microseconds unit.
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 9464ba3..eacd9ae 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2939,6 +2939,15 @@ enum skl_disp_power_wells {
> >  				INTERVAL_1_33_US(us)) : \
> >  				INTERVAL_1_28_US(us))
> >  
> > +#define INTERVAL_1_28_TO_US(interval)  (((interval) << 7) / 100)
> > +#define INTERVAL_1_33_TO_US(interval)  (((interval) << 2) / 3)
> > +#define INTERVAL_0_833_TO_US(interval) (((interval) * 5)  / 6)
> > +#define GT_PM_INTERVAL_TO_US(dev_priv, interval) (IS_GEN9(dev_priv) ? \
> > +                           (IS_BROXTON(dev_priv) ? \
> > +                           INTERVAL_0_833_TO_US(interval) : \
> > +                           INTERVAL_1_33_TO_US(interval)) : \
> > +                           INTERVAL_1_28_TO_US(interval))
> 
> Bikeshed, but I think once we have platform logic in there a real inline
> functino starts to look better.

Or when we have many, a feature flag. Could be worse, each could be
expanding to devid.
> 
> Feel free to ignore since already pushed.

Yeah, since this was cut'n'paste for debugfs, I wasn't that concerned
about elegance just that I could compare the macros and check that it
was the inverse.
-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