[PATCH 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6

[PATCH 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6

[Sagar Arun Kamble]

RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
configuration registers. If those are not setup Driver should not enable RC6.
For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
to know if BIOS has enabled HW/SW RC6.
This will also enable user to control RC6 using BIOS settings alone.

Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9dda3ea..8c595e0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4836,6 +4836,15 @@ static void gen8_enable_rps(struct drm_device *dev)
 	struct intel_engine_cs *ring;
 	uint32_t rc6_mask = 0;
 	int unused;
+	bool hw_rc6_enabled, sw_rc6_enabled;
+
+	/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
+	hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
+				(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
+	sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
+				&& (I915_READ(GEN6_RC_STATE) & 0x40000);
+	if (!(hw_rc6_enabled || sw_rc6_enabled))
+		i915.enable_rc6 = 0;
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
-- 
1.9.1

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

[PATCH v2 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6

[Sagar Arun Kamble]

RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
configuration registers. If those are not setup Driver should not enable RC6.
For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
to know if BIOS has enabled HW/SW RC6.
This will also enable user to control RC6 using BIOS settings alone.

v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is
not enabled in case BIOS disabled RC6.

Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2c7c9fc..1ec52f2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -30,6 +30,7 @@
 #include "intel_drv.h"
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 
 /**
  * RC6 is a special power stage which allows the GPU to enter an very
@@ -4763,6 +4764,20 @@ static void gen9_enable_rc6(struct drm_device *dev)
 	struct intel_engine_cs *ring;
 	uint32_t rc6_mask = 0;
 	int unused;
+	bool hw_rc6_enabled, sw_rc6_enabled;
+	struct device *device = &dev->pdev->dev;
+
+	/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
+	hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
+				(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
+	sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
+				&& (I915_READ(GEN6_RC_STATE) & 0x40000);
+	if (!(hw_rc6_enabled || sw_rc6_enabled)) {
+		i915.enable_rc6 = 0;
+		pm_runtime_forbid(device);
+		DRM_INFO("RC6 disabled by BIOS, disabled runtime PM support\n");
+	}
+
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
-- 
1.9.1

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

Re: [PATCH v2 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6

[Daniel Vetter]

On Fri, Oct 30, 2015 at 05:00:49PM +0530, Sagar Arun Kamble wrote:
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> configuration registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> 
> v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is
> not enabled in case BIOS disabled RC6.
> 
> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2c7c9fc..1ec52f2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -30,6 +30,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  
>  /**
>   * RC6 is a special power stage which allows the GPU to enter an very
> @@ -4763,6 +4764,20 @@ static void gen9_enable_rc6(struct drm_device *dev)
>  	struct intel_engine_cs *ring;
>  	uint32_t rc6_mask = 0;
>  	int unused;
> +	bool hw_rc6_enabled, sw_rc6_enabled;
> +	struct device *device = &dev->pdev->dev;
> +
> +	/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
> +	hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> +				(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
> +	sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
> +				&& (I915_READ(GEN6_RC_STATE) & 0x40000);

Please add a define for the magic value 0x40000. Also, should we check
this on earlier platforms too? In that case a small helper and calling it
everywhere would be great.
-Daniel

> +	if (!(hw_rc6_enabled || sw_rc6_enabled)) {
> +		i915.enable_rc6 = 0;
> +		pm_runtime_forbid(device);
> +		DRM_INFO("RC6 disabled by BIOS, disabled runtime PM support\n");
> +	}
> +
>  
>  	/* 1a: Software RC state - RC0 */
>  	I915_WRITE(GEN6_RC_STATE, 0);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 1/1] drm/i915/gen9: Check BIOS RC6 setup before enabling RC6

[Daniel Vetter]

On Fri, Oct 30, 2015 at 05:00:49PM +0530, Sagar Arun Kamble wrote:
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> configuration registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> 
> v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is
> not enabled in case BIOS disabled RC6.
> 
> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2c7c9fc..1ec52f2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -30,6 +30,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  
>  /**
>   * RC6 is a special power stage which allows the GPU to enter an very
> @@ -4763,6 +4764,20 @@ static void gen9_enable_rc6(struct drm_device *dev)
>  	struct intel_engine_cs *ring;
>  	uint32_t rc6_mask = 0;
>  	int unused;
> +	bool hw_rc6_enabled, sw_rc6_enabled;
> +	struct device *device = &dev->pdev->dev;
> +
> +	/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
> +	hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> +				(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
> +	sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
> +				&& (I915_READ(GEN6_RC_STATE) & 0x40000);
> +	if (!(hw_rc6_enabled || sw_rc6_enabled)) {
> +		i915.enable_rc6 = 0;
> +		pm_runtime_forbid(device);
> +		DRM_INFO("RC6 disabled by BIOS, disabled runtime PM support\n");

One more: You don't disable runtime PM here, only RC6. Please fix this.
-Daniel

> +	}
> +
>  
>  	/* 1a: Software RC state - RC0 */
>  	I915_WRITE(GEN6_RC_STATE, 0);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Sagar Arun Kamble]

RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
setup registers. If those are not setup Driver should not enable RC6.
For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
to know if BIOS has enabled HW/SW RC6.
This will also enable user to control RC6 using BIOS settings alone.
RC6 related instability can be avoided by disabling via BIOS settings
till driver fixes it.

RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration registers. If those are not setup Driver should not enable RC6.
For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values to know if BIOS has enabled HW/SW RC6.
This will also enable user to control RC6 using BIOS settings alone.

v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is not enabled in case BIOS disabled RC6.

v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
Runtime PM enabling happens before gen9_enable_rc6.
Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.

Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8942532..6ed3542 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6823,6 +6823,7 @@ enum skl_disp_power_wells {
 #define GEN6_RPDEUC				0xA084
 #define GEN6_RPDEUCSW				0xA088
 #define GEN6_RC_STATE				0xA094
+#define RC6_STATE				(1<<18)
 #define GEN6_RC1_WAKE_RATE_LIMIT		0xA098
 #define GEN6_RC6_WAKE_RATE_LIMIT		0xA09C
 #define GEN6_RC6pp_WAKE_RATE_LIMIT		0xA0A0
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index f0f97b2..bedb089 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
 	i915_check_and_clear_faults(dev);
 }
 
+static void sanitize_bios_rc6_setup(const struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool hw_rc6_enabled = false, sw_rc6_enabled = false;
+
+	if (IS_BROXTON(dev)) {
+		/* Get forcewake during program sequence. Although the driver
+		 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+
+		/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
+		hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
+					(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
+		sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
+					&& (I915_READ(GEN6_RC_STATE) & RC6_STATE);
+
+		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+		if (!hw_rc6_enabled && !sw_rc6_enabled) {
+			i915.enable_rc6 = 0;
+			DRM_INFO("RC6 disabled by BIOS\n");
+		}
+	}
+}
+
 void intel_uncore_sanitize(struct drm_device *dev)
 {
+	sanitize_bios_rc6_setup(dev);
+
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
 	intel_disable_gt_powersave(dev);
 }
-- 
1.9.1

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

Re: [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Daniel Vetter]

On Wed, Nov 04, 2015 at 03:34:54PM +0530, Sagar Arun Kamble wrote:
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> setup registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> RC6 related instability can be avoided by disabling via BIOS settings
> till driver fixes it.
> 
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> 
> v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is not enabled in case BIOS disabled RC6.
> 
> v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
> Runtime PM enabling happens before gen9_enable_rc6.
> Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.
> 
> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |  1 +
>  drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8942532..6ed3542 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells {
>  #define GEN6_RPDEUC				0xA084
>  #define GEN6_RPDEUCSW				0xA088
>  #define GEN6_RC_STATE				0xA094
> +#define RC6_STATE				(1<<18)
>  #define GEN6_RC1_WAKE_RATE_LIMIT		0xA098
>  #define GEN6_RC6_WAKE_RATE_LIMIT		0xA09C
>  #define GEN6_RC6pp_WAKE_RATE_LIMIT		0xA0A0
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index f0f97b2..bedb089 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
>  	i915_check_and_clear_faults(dev);
>  }
>  
> +static void sanitize_bios_rc6_setup(const struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool hw_rc6_enabled = false, sw_rc6_enabled = false;
> +
> +	if (IS_BROXTON(dev)) {
> +		/* Get forcewake during program sequence. Although the driver
> +		 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> +
> +		/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
> +		hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> +					(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
> +		sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
> +					&& (I915_READ(GEN6_RC_STATE) & RC6_STATE);
> +
> +		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +		if (!hw_rc6_enabled && !sw_rc6_enabled) {
> +			i915.enable_rc6 = 0;
> +			DRM_INFO("RC6 disabled by BIOS\n");
> +		}
> +	}
> +}
> +
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
> +	sanitize_bios_rc6_setup(dev);

Why did you move this out of the enable_rc6 functions? It seems to fit
much better there, instead of here.

Also I think we shouldn't change the module option, instead it's
conceptually cleaner to just not set up rc6 in genX_enable_rc6, i.e.

	if (!check_bios_rc6_setup())
		return;

somewhere at the beginning of gen9_enable_rc6 like you had in the previous
patch.
-Daniel

> +
>  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
>  	intel_disable_gt_powersave(dev);
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Daniel Vetter]

On Tue, Nov 17, 2015 at 05:45:06PM +0100, Daniel Vetter wrote:
> On Wed, Nov 04, 2015 at 03:34:54PM +0530, Sagar Arun Kamble wrote:
> > RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> > setup registers. If those are not setup Driver should not enable RC6.
> > For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> > to know if BIOS has enabled HW/SW RC6.
> > This will also enable user to control RC6 using BIOS settings alone.
> > RC6 related instability can be avoided by disabling via BIOS settings
> > till driver fixes it.
> > 
> > RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6 configuration registers. If those are not setup Driver should not enable RC6.
> > For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values to know if BIOS has enabled HW/SW RC6.
> > This will also enable user to control RC6 using BIOS settings alone.
> > 
> > v2: Had placed logic in gen8 function by mistake. Fixed it. Ensuring RPM is not enabled in case BIOS disabled RC6.
> > 
> > v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
> > Runtime PM enabling happens before gen9_enable_rc6.
> > Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.
> > 
> > Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h     |  1 +
> >  drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8942532..6ed3542 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells {
> >  #define GEN6_RPDEUC				0xA084
> >  #define GEN6_RPDEUCSW				0xA088
> >  #define GEN6_RC_STATE				0xA094
> > +#define RC6_STATE				(1<<18)
> >  #define GEN6_RC1_WAKE_RATE_LIMIT		0xA098
> >  #define GEN6_RC6_WAKE_RATE_LIMIT		0xA09C
> >  #define GEN6_RC6pp_WAKE_RATE_LIMIT		0xA0A0
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index f0f97b2..bedb089 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
> >  	i915_check_and_clear_faults(dev);
> >  }
> >  
> > +static void sanitize_bios_rc6_setup(const struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	bool hw_rc6_enabled = false, sw_rc6_enabled = false;
> > +
> > +	if (IS_BROXTON(dev)) {
> > +		/* Get forcewake during program sequence. Although the driver
> > +		 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
> > +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> > +
> > +		/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
> > +		hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> > +					(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
> > +		sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
> > +					&& (I915_READ(GEN6_RC_STATE) & RC6_STATE);
> > +
> > +		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> > +
> > +		if (!hw_rc6_enabled && !sw_rc6_enabled) {
> > +			i915.enable_rc6 = 0;
> > +			DRM_INFO("RC6 disabled by BIOS\n");
> > +		}
> > +	}
> > +}
> > +
> >  void intel_uncore_sanitize(struct drm_device *dev)
> >  {
> > +	sanitize_bios_rc6_setup(dev);
> 
> Why did you move this out of the enable_rc6 functions? It seems to fit
> much better there, instead of here.
> 
> Also I think we shouldn't change the module option, instead it's
> conceptually cleaner to just not set up rc6 in genX_enable_rc6, i.e.
> 
> 	if (!check_bios_rc6_setup())
> 		return;
> 
> somewhere at the beginning of gen9_enable_rc6 like you had in the previous
> patch.

Well scrap this since it's just a bikeshed, we do adjust the module
options in other places too. Patch looks fine, I'll pull it in if someone
with domain knowledge reviews it.
-Daniel

> -Daniel
> 
> > +
> >  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
> >  	intel_disable_gt_powersave(dev);
> >  }
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Imre Deak]

Hi Sagar,

sorry for the delay, see below for my comments.

On Wed, 2015-11-04 at 15:34 +0530, Sagar Arun Kamble wrote:
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> setup registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> RC6 related instability can be avoided by disabling via BIOS settings
> till driver fixes it.
> 
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of
> RC6 configuration registers. If those are not setup Driver should not
> enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> 
> v2: Had placed logic in gen8 function by mistake. Fixed it.
> Ensuring RPM is not enabled in case BIOS disabled RC6.
> 
> v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
> Runtime PM enabling happens before gen9_enable_rc6.
> Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.
> 
> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |  1 +
>  drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8942532..6ed3542 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells {
>  #define GEN6_RPDEUC				0xA084
>  #define GEN6_RPDEUCSW				0xA088
>  #define GEN6_RC_STATE				0xA094
> +#define RC6_STATE				(1<<18)
>  #define GEN6_RC1_WAKE_RATE_LIMIT		0xA098
>  #define GEN6_RC6_WAKE_RATE_LIMIT		0xA09C
>  #define GEN6_RC6pp_WAKE_RATE_LIMIT		0xA0A0
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index f0f97b2..bedb089 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
>  	i915_check_and_clear_faults(dev);
>  }
>  
> +static void sanitize_bios_rc6_setup(const struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool hw_rc6_enabled = false, sw_rc6_enabled = false;
> +
> +	if (IS_BROXTON(dev)) {
> +		/* Get forcewake during program sequence. Although the driver
> +		 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);

Do we really need FW for reading? We don't program things here and
I915_READ() does take FW for the access itself.

> +
> +		/* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
> +		hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> +					(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
> +		sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
> +					&& (I915_READ(GEN6_RC_STATE) & RC6_STATE);

Could you also add the following checks and bail out if anyone is
invalid?:

RC6LOCATION (0xd40) [0] should be 1.
RC6CTXBASE (0xd48) [31:4] should be non-zero. We could also check if it
falls within the WOPCM as it should.
PWRCTX_MAXCNT_RCSUNIT (0x2054),
 PWRCTX_MAXCNT_VCSUNIT0 (0x12054),
 PWRCTX_MAXCNT_BCSUNIT (0x22054),
 PWRCTX_MAXCNT_VECSUNIT (0x1A054),
 PWRCTX_MAXCNT_VCSUNIT1 (0x1C054) [19:0] should be greater than 1.

> +
> +		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +		if (!hw_rc6_enabled && !sw_rc6_enabled) {
> +			i915.enable_rc6 = 0;
> +			DRM_INFO("RC6 disabled by BIOS\n");
> +		}
> +	}
> +}
> +
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
> +	sanitize_bios_rc6_setup(dev);
> +

I'd prefer keeping this together with the RC6-sanitize code done for
other platforms in intel_init_gt_powersave(). You could also add a
broxton_check_pctx() similar to VLV/CHV that would perform the above
sanity checks any time intel_enable_gt_powersave() is called (in
addition to the check during intel_init_gt_powersave()).

>  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
>  	intel_disable_gt_powersave(dev);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Sagar Arun Kamble]

RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
setup registers. If those are not setup Driver should not enable RC6.
For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
to know if BIOS has enabled HW/SW RC6.
This will also enable user to control RC6 using BIOS settings alone.
RC6 related instability can be avoided by disabling via BIOS settings
till driver fixes it.

v2: Had placed logic in gen8 function by mistake. Fixed it.
Ensuring RPM is not enabled in case BIOS disabled RC6.

v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
Runtime PM enabling happens before gen9_enable_rc6.
Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.

v4: Added elaborate check for BIOS RC6 setup. Prepared check_pctx for bxt. (Imre)

Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  4 +++
 drivers/gpu/drm/i915/i915_gem_stolen.c |  9 +++++
 drivers/gpu/drm/i915/i915_reg.h        | 12 +++++++
 drivers/gpu/drm/i915/intel_pm.c        | 62 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c    | 10 ++++++
 5 files changed, 97 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4c03449..265d08e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1696,6 +1696,8 @@ struct drm_i915_private {
 
 	void __iomem *regs;
 
+	bool bios_hw_rc6_enabled;
+	bool bios_sw_rc6_enabled;
 	struct intel_uncore uncore;
 
 	struct i915_virtual_gpu vgpu;
@@ -3234,6 +3236,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 					       u32 stolen_offset,
 					       u32 gtt_offset,
 					       u32 size);
+void i915_get_stolen_reserved(struct drm_i915_private *dev_priv,
+				    unsigned long *base, unsigned long *size);
 
 /* i915_gem_shrinker.c */
 unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 598ed2f..92ea7c0 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -386,6 +386,15 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
 		*size = stolen_top - *base;
 }
 
+void i915_get_stolen_reserved(struct drm_i915_private *dev_priv,
+				    unsigned long *base, unsigned long *size)
+{
+	if (IS_SKYLAKE(dev_priv))
+		bdw_get_stolen_reserved(dev_priv, base, size);
+	else
+		gen8_get_stolen_reserved(dev_priv, base, size);
+}
+
 int i915_gem_init_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ed0e785..fd4f907 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6765,6 +6765,17 @@ enum skl_disp_power_wells {
 
 #define  VLV_PMWGICZ				_MMIO(0x1300a4)
 
+#define  RC6_LOCATION				_MMIO(0xD40)
+#define	 RC6_CTX_IN_DRAM			1
+#define  RC6_CTX_BASE				_MMIO(0xD48)
+#define  RC6_CTX_BASE_MASK			0xFFFFFFF0
+#define  PWRCTX_MAXCNT_RCSUNIT 			_MMIO(0x2054)
+#define  PWRCTX_MAXCNT_VCSUNIT0 		_MMIO(0x12054)
+#define  PWRCTX_MAXCNT_BCSUNIT 			_MMIO(0x22054)
+#define  PWRCTX_MAXCNT_VECSUNIT 		_MMIO(0x1A054)
+#define  PWRCTX_MAXCNT_VCSUNIT1 		_MMIO(0x1C054)
+#define  IDLE_TIME_MASK				0xFFFFF
+
 #define  FORCEWAKE				_MMIO(0xA18C)
 #define  FORCEWAKE_VLV				_MMIO(0x1300b0)
 #define  FORCEWAKE_ACK_VLV			_MMIO(0x1300b4)
@@ -6903,6 +6914,7 @@ enum skl_disp_power_wells {
 #define GEN6_RPDEUC				_MMIO(0xA084)
 #define GEN6_RPDEUCSW				_MMIO(0xA088)
 #define GEN6_RC_STATE				_MMIO(0xA094)
+#define RC6_STATE				(1<<18)
 #define GEN6_RC1_WAKE_RATE_LIMIT		_MMIO(0xA098)
 #define GEN6_RC6_WAKE_RATE_LIMIT		_MMIO(0xA09C)
 #define GEN6_RC6pp_WAKE_RATE_LIMIT		_MMIO(0xA0A0)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ee05ce8..4d9cbab 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4619,6 +4619,65 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
 	}
 }
 
+static void bxt_check_pctx(const struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool enable_rc6 = true;
+	unsigned long reserved_base = 0, reserved_size, rc6_ctx_base;
+
+	i915_get_stolen_reserved(dev_priv, &reserved_base,
+					&reserved_size);
+
+	if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
+		DRM_ERROR("RC6 Base location not set properly.\n");
+		enable_rc6 = false;
+	}
+
+	rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK;
+	if (!((rc6_ctx_base >= reserved_base) &&
+		(rc6_ctx_base <= (reserved_base + reserved_size)))) {
+		DRM_ERROR("RC6 Base address not as expected.\n");
+		enable_rc6 = false;
+	}
+
+	if (!enable_rc6) {
+		i915.enable_rc6 = 0;
+		DRM_ERROR("RC6 disabled by BIOS\n");
+	}
+}
+
+static void bxt_check_bios_rc6_setup(const struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool enable_rc6 = true;
+
+	bxt_check_pctx(dev);
+
+	if (!(((I915_READ(PWRCTX_MAXCNT_RCSUNIT) & IDLE_TIME_MASK) > 1) &&
+	      ((I915_READ(PWRCTX_MAXCNT_VCSUNIT0) & IDLE_TIME_MASK) > 1) &&
+	      ((I915_READ(PWRCTX_MAXCNT_BCSUNIT) & IDLE_TIME_MASK) > 1) &&
+	      ((I915_READ(PWRCTX_MAXCNT_VECSUNIT) & IDLE_TIME_MASK) > 1))) {
+		DRM_ERROR("Engine Idle wait time not set properly.\n");
+		enable_rc6 = false;
+	}
+
+	if (HAS_BSD2(dev) &&
+		((I915_READ(PWRCTX_MAXCNT_VCSUNIT1) & IDLE_TIME_MASK) > 1)) {
+		DRM_ERROR("VCSUNIT1 Idle wait time not set properly.\n");
+		enable_rc6 = false;
+	}
+
+	if (!dev_priv->bios_hw_rc6_enabled && !dev_priv->bios_sw_rc6_enabled) {
+		DRM_ERROR("HW/SW RC6 is not enabled by BIOS.\n");
+		enable_rc6 = false;
+	}
+
+	if (!enable_rc6) {
+		i915.enable_rc6 = 0;
+		DRM_ERROR("RC6 disabled by BIOS\n");
+	}
+}
+
 /* See the Gen9_GT_PM_Programming_Guide doc for the below */
 static void gen9_enable_rps(struct drm_device *dev)
 {
@@ -4660,6 +4719,9 @@ static void gen9_enable_rc6(struct drm_device *dev)
 	uint32_t rc6_mask = 0;
 	int unused;
 
+	if (IS_BROXTON(dev))
+		bxt_check_bios_rc6_setup(dev);
+
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c2358ba..c76c076 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -366,6 +366,16 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
 
 void intel_uncore_sanitize(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (IS_BROXTON(dev)) {
+		/* Store HW/SW RC6 status set by BIOS before we disable.*/
+		dev_priv->bios_hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
+					(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
+		dev_priv->bios_sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
+					&& (I915_READ(GEN6_RC_STATE) & RC6_STATE);
+	}
+
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
 	intel_disable_gt_powersave(dev);
 }
-- 
1.9.1

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

Re: [PATCH v4 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Imre Deak]

On pe, 2015-12-11 at 14:14 +0530, Sagar Arun Kamble wrote:
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> setup registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> RC6 related instability can be avoided by disabling via BIOS settings
> till driver fixes it.
> 
> v2: Had placed logic in gen8 function by mistake. Fixed it.
> Ensuring RPM is not enabled in case BIOS disabled RC6.
> 
> v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
> Runtime PM enabling happens before gen9_enable_rc6.
> Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.
> 
> v4: Added elaborate check for BIOS RC6 setup. Prepared check_pctx for bxt. (Imre)
> 
> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  4 +++
>  drivers/gpu/drm/i915/i915_gem_stolen.c |  9 +++++
>  drivers/gpu/drm/i915/i915_reg.h        | 12 +++++++
>  drivers/gpu/drm/i915/intel_pm.c        | 62 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.c    | 10 ++++++
>  5 files changed, 97 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4c03449..265d08e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1696,6 +1696,8 @@ struct drm_i915_private {
>  
>  	void __iomem *regs;
>  
> +	bool bios_hw_rc6_enabled;
> +	bool bios_sw_rc6_enabled;
>  	struct intel_uncore uncore;
>  
>  	struct i915_virtual_gpu vgpu;
> @@ -3234,6 +3236,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  					       u32 stolen_offset,
>  					       u32 gtt_offset,
>  					       u32 size);
> +void i915_get_stolen_reserved(struct drm_i915_private *dev_priv,
> +				    unsigned long *base, unsigned long *size);

For this and other similar changes in the patch: wrapped function
arguments need to be aligned to start at the column next to the
function's opening brace.

>  
>  /* i915_gem_shrinker.c */
>  unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 598ed2f..92ea7c0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -386,6 +386,15 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  		*size = stolen_top - *base;
>  }
>  
> +void i915_get_stolen_reserved(struct drm_i915_private *dev_priv,
> +				    unsigned long *base, unsigned long *size)
> +{
> +	if (IS_SKYLAKE(dev_priv))
> +		bdw_get_stolen_reserved(dev_priv, base, size);
> +	else
> +		gen8_get_stolen_reserved(dev_priv, base, size);
> +}
> +

Hm, but then we are missing all the rest of the platforms and leave
things open-coded in i915_gem_init_stolen(). I suggest just making
i915_gem_init_stolen() cache the reserved base and size in dev_priv-
>gtt too, since we will use these to check the HW state both during
driver loading and resume.

>  int i915_gem_init_stolen(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ed0e785..fd4f907 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6765,6 +6765,17 @@ enum skl_disp_power_wells {
>  
>  #define  VLV_PMWGICZ				_MMIO(0x1300a4)
>  
> +#define  RC6_LOCATION				_MMIO(0xD40)
> +#define	 RC6_CTX_IN_DRAM			1
> +#define  RC6_CTX_BASE				_MMIO(0xD48)
> +#define  RC6_CTX_BASE_MASK			0xFFFFFFF0
> +#define  PWRCTX_MAXCNT_RCSUNIT 			_MMIO(0x2054)
> +#define  PWRCTX_MAXCNT_VCSUNIT0 		_MMIO(0x12054)
> +#define  PWRCTX_MAXCNT_BCSUNIT 			_MMIO(0x22054)
> +#define  PWRCTX_MAXCNT_VECSUNIT 		_MMIO(0x1A054)
> +#define  PWRCTX_MAXCNT_VCSUNIT1 		_MMIO(0x1C054)
> +#define  IDLE_TIME_MASK				0xFFFFF

No spaces before tabs, and indent register flag names with one or two
extra spaces starting from the column of the register name.

> +
>  #define  FORCEWAKE				_MMIO(0xA18C)
>  #define  FORCEWAKE_VLV				_MMIO(0x1300b0)
>  #define  FORCEWAKE_ACK_VLV			_MMIO(0x1300b4)
> @@ -6903,6 +6914,7 @@ enum skl_disp_power_wells {
>  #define GEN6_RPDEUC				_MMIO(0xA084)
>  #define GEN6_RPDEUCSW				_MMIO(0xA088)
>  #define GEN6_RC_STATE				_MMIO(0xA094)
> +#define RC6_STATE				(1<<18)

Needs proper indent as above.

>  #define GEN6_RC1_WAKE_RATE_LIMIT		_MMIO(0xA098)
>  #define GEN6_RC6_WAKE_RATE_LIMIT		_MMIO(0xA09C)
>  #define GEN6_RC6pp_WAKE_RATE_LIMIT		_MMIO(0xA0A0)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ee05ce8..4d9cbab 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4619,6 +4619,65 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
>  	}
>  }
>  
> +static void bxt_check_pctx(const struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool enable_rc6 = true;
> +	unsigned long reserved_base = 0, reserved_size, rc6_ctx_base;
> +
> +	i915_get_stolen_reserved(dev_priv, &reserved_base,
> +					&reserved_size);
> +
> +	if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
> +		DRM_ERROR("RC6 Base location not set properly.\n");

It's not necessarily an error, since the user could've disabled it in
BIOS, so let's turn these into DRM_DEBUG_KMS and let the caller print a
DRM_INFO("RC6 disabled by BIOS\n") if any of the conditions are unmet.

> +		enable_rc6 = false;
> +	}
> +
> +	rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK;
> +	if (!((rc6_ctx_base >= reserved_base) &&
> +		(rc6_ctx_base <= (reserved_base + reserved_size)))) {
> +		DRM_ERROR("RC6 Base address not as expected.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	if (!enable_rc6) {
> +		i915.enable_rc6 = 0;
> +		DRM_ERROR("RC6 disabled by BIOS\n");
> +	}
> +}
> +
> +static void bxt_check_bios_rc6_setup(const struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool enable_rc6 = true;
> +
> +	bxt_check_pctx(dev);
> +
> +	if (!(((I915_READ(PWRCTX_MAXCNT_RCSUNIT) & IDLE_TIME_MASK) > 1) &&
> +	      ((I915_READ(PWRCTX_MAXCNT_VCSUNIT0) & IDLE_TIME_MASK) > 1) &&
> +	      ((I915_READ(PWRCTX_MAXCNT_BCSUNIT) & IDLE_TIME_MASK) > 1) &&
> +	      ((I915_READ(PWRCTX_MAXCNT_VECSUNIT) & IDLE_TIME_MASK) > 1))) {
> +		DRM_ERROR("Engine Idle wait time not set properly.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	if (HAS_BSD2(dev) &&
> +		((I915_READ(PWRCTX_MAXCNT_VCSUNIT1) & IDLE_TIME_MASK) > 1)) {
> +		DRM_ERROR("VCSUNIT1 Idle wait time not set properly.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	if (!dev_priv->bios_hw_rc6_enabled && !dev_priv->bios_sw_rc6_enabled) {
> +		DRM_ERROR("HW/SW RC6 is not enabled by BIOS.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	if (!enable_rc6) {
> +		i915.enable_rc6 = 0;
> +		DRM_ERROR("RC6 disabled by BIOS\n");
> +	}
> +}

No need to separate the checks into two functions, we can have all the
above checks in a single bool function returning success if all the
conditions are met. 

> +
>  /* See the Gen9_GT_PM_Programming_Guide doc for the below */
>  static void gen9_enable_rps(struct drm_device *dev)
>  {
> @@ -4660,6 +4719,9 @@ static void gen9_enable_rc6(struct drm_device *dev)
>  	uint32_t rc6_mask = 0;
>  	int unused;
>  
> +	if (IS_BROXTON(dev))
> +		bxt_check_bios_rc6_setup(dev);

Actually we can get rid of the check here, and ...

> +
>  	/* 1a: Software RC state - RC0 */
>  	I915_WRITE(GEN6_RC_STATE, 0);
>  
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c2358ba..c76c076 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -366,6 +366,16 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
>  
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (IS_BROXTON(dev)) {
> +		/* Store HW/SW RC6 status set by BIOS before we disable.*/
> +		dev_priv->bios_hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> +					(GEN6_RC_CTL_RC6_ENABLE | GEN6_RC_CTL_HW_ENABLE);
> +		dev_priv->bios_sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
> +					&& (I915_READ(GEN6_RC_STATE) & RC6_STATE);

do all the checks here once, by adding these checks to the above
combined bxt_check_bios_rc6_setup(), moving
i915.enable_rc6 = sanitize_rc6_option() from intel_init_gt_powersave()
here and calling bxt_check_bios_rc6_setup() from sanitize_rc6_option().
We can then do away with bios_hw_rc6_enabled and bios_sw_rc6_enabled.

--Imre

> +	}
> +
>  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
>  	intel_disable_gt_powersave(dev);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx