diff mbox

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

Message ID 1449823450-24165-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sagar.a.kamble@intel.com Dec. 11, 2015, 8:44 a.m. UTC
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(+)

Comments

Imre Deak Dec. 14, 2015, 4:36 p.m. UTC | #1
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);
>  }
diff mbox

Patch

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);
 }