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

[PATCH v8 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)

v5: Caching reserved stolen base and size in the driver private data.
    Reorganized RC6 setup check. Moved from gen9_enable_rc6 to
    intel_uncore_sanitize. (Imre)

v6: Rebasing on the patch submitted by Imre that moves gem_init_stolen
    earlier in the load.

v7: Removed PWRCTX_MAXCNT_VCSUNIT1 check as it applies to SKL. (Imre)

v8: Fixed formatting and checkpatch issues. Fixed functional issue where
    RC6 ctx size check was missing. (Imre)

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.h    |  2 ++
 drivers/gpu/drm/i915/i915_gem_stolen.c |  3 ++
 drivers/gpu/drm/i915/i915_reg.h        | 11 +++++++
 drivers/gpu/drm/i915/intel_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_pm.c        | 53 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_uncore.c    |  2 ++
 6 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index f520c90..66a6da2 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -342,6 +342,8 @@ struct i915_gtt {
 
 	size_t stolen_size;		/* Total size of stolen memory */
 	size_t stolen_usable_size;	/* Total size minus BIOS reserved */
+	size_t stolen_reserved_base;
+	size_t stolen_reserved_size;
 	u64 mappable_end;		/* End offset that we can CPU map */
 	struct io_mapping *mappable;	/* Mapping to our CPU mappable region */
 	phys_addr_t mappable_base;	/* PA of our GMADR */
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index c384dc9..ba1a00d 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -458,6 +458,9 @@ int i915_gem_init_stolen(struct drm_device *dev)
 		return 0;
 	}
 
+	dev_priv->gtt.stolen_reserved_base = reserved_base;
+	dev_priv->gtt.stolen_reserved_size = reserved_size;
+
 	/* It is possible for the reserved area to end before the end of stolen
 	 * memory, so just consider the start. */
 	reserved_total = stolen_top - reserved_base;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c0bd691..71b1fe9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6784,6 +6784,16 @@ enum skl_disp_power_wells {
 
 #define  VLV_PMWGICZ				_MMIO(0x1300a4)
 
+#define  RC6_LOCATION				_MMIO(0xD40)
+#define	   RC6_CTX_IN_DRAM			(1 << 0)
+#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)
@@ -6922,6 +6932,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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 93ba14a..1251a7a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1566,6 +1566,7 @@ void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
+int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6);
 
 /* intel_sdvo.c */
 bool intel_sdvo_init(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a47b8f2..78db72c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4558,12 +4558,62 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
 			      onoff(mode & GEN6_RC_CTL_RC6_ENABLE));
 }
 
-static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
+static bool bxt_check_bios_rc6_setup(const struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool enable_rc6 = true;
+	unsigned long rc6_ctx_base;
+
+	if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
+		DRM_DEBUG_KMS("RC6 Base location not set properly.\n");
+		enable_rc6 = false;
+	}
+
+	/*
+	 * The exact context size is not known for BXT, so assume a page size
+	 * for this check.
+	 */
+	rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK;
+	if (!((rc6_ctx_base >= dev_priv->gtt.stolen_reserved_base) &&
+	      (rc6_ctx_base + PAGE_SIZE <= dev_priv->gtt.stolen_reserved_base +
+					dev_priv->gtt.stolen_reserved_size))) {
+		DRM_DEBUG_KMS("RC6 Base address not as expected.\n");
+		enable_rc6 = false;
+	}
+
+	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_DEBUG_KMS("Engine Idle wait time not set properly.\n");
+		enable_rc6 = false;
+	}
+
+	if (!(I915_READ(GEN6_RC_CONTROL) & (GEN6_RC_CTL_RC6_ENABLE |
+					    GEN6_RC_CTL_HW_ENABLE)) &&
+	    ((I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) ||
+	     !(I915_READ(GEN6_RC_STATE) & RC6_STATE))) {
+		DRM_DEBUG_KMS("HW/SW RC6 is not enabled by BIOS.\n");
+		enable_rc6 = false;
+	}
+
+	return enable_rc6;
+}
+
+int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
 {
 	/* No RC6 before Ironlake and code is gone for ilk. */
 	if (INTEL_INFO(dev)->gen < 6)
 		return 0;
 
+	if (!enable_rc6)
+		return 0;
+
+	if (IS_BROXTON(dev) && !bxt_check_bios_rc6_setup(dev)) {
+		DRM_INFO("RC6 disabled by BIOS\n");
+		return 0;
+	}
+
 	/* Respect the kernel parameter if it is set */
 	if (enable_rc6 >= 0) {
 		int mask;
@@ -6053,7 +6103,6 @@ void intel_init_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
 	/*
 	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
 	 * requirement.
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index bfa79e5..436d8f2 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -400,6 +400,8 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
 
 void intel_uncore_sanitize(struct drm_device *dev)
 {
+	i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
+
 	/* 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 v8 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Imre Deak]

On la, 2016-02-06 at 00:13 +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)
> 
> v5: Caching reserved stolen base and size in the driver private data.
>     Reorganized RC6 setup check. Moved from gen9_enable_rc6 to
>     intel_uncore_sanitize. (Imre)
> 
> v6: Rebasing on the patch submitted by Imre that moves
> gem_init_stolen
>     earlier in the load.
> 
> v7: Removed PWRCTX_MAXCNT_VCSUNIT1 check as it applies to SKL. (Imre)
> 
> v8: Fixed formatting and checkpatch issues. Fixed functional issue
> where
>     RC6 ctx size check was missing. (Imre)
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

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

Thanks for the patch, I pushed it to -dinq.

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h    |  2 ++
>  drivers/gpu/drm/i915/i915_gem_stolen.c |  3 ++
>  drivers/gpu/drm/i915/i915_reg.h        | 11 +++++++
>  drivers/gpu/drm/i915/intel_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_pm.c        | 53
> ++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_uncore.c    |  2 ++
>  6 files changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index f520c90..66a6da2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -342,6 +342,8 @@ struct i915_gtt {
>  
>  	size_t stolen_size;		/* Total size of stolen
> memory */
>  	size_t stolen_usable_size;	/* Total size minus BIOS
> reserved */
> +	size_t stolen_reserved_base;
> +	size_t stolen_reserved_size;
>  	u64 mappable_end;		/* End offset that we can
> CPU map */
>  	struct io_mapping *mappable;	/* Mapping to our CPU
> mappable region */
>  	phys_addr_t mappable_base;	/* PA of our GMADR */
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index c384dc9..ba1a00d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -458,6 +458,9 @@ int i915_gem_init_stolen(struct drm_device *dev)
>  		return 0;
>  	}
>  
> +	dev_priv->gtt.stolen_reserved_base = reserved_base;
> +	dev_priv->gtt.stolen_reserved_size = reserved_size;
> +
>  	/* It is possible for the reserved area to end before the
> end of stolen
>  	 * memory, so just consider the start. */
>  	reserved_total = stolen_top - reserved_base;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index c0bd691..71b1fe9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6784,6 +6784,16 @@ enum skl_disp_power_wells {
>  
>  #define  VLV_PMWGICZ				_MMIO(0x1300a4)
>  
> +#define  RC6_LOCATION				_MMIO(0xD40)
> +#define	   RC6_CTX_IN_DRAM			(1 << 0)
> +#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)
> @@ -6922,6 +6932,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_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 93ba14a..1251a7a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1566,6 +1566,7 @@ void skl_wm_get_hw_state(struct drm_device
> *dev);
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> *pipe_config);
> +int sanitize_rc6_option(const struct drm_device *dev, int
> enable_rc6);
>  
>  /* intel_sdvo.c */
>  bool intel_sdvo_init(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index a47b8f2..78db72c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4558,12 +4558,62 @@ static void intel_print_rc6_info(struct
> drm_device *dev, u32 mode)
>  			      onoff(mode & GEN6_RC_CTL_RC6_ENABLE));
>  }
>  
> -static int sanitize_rc6_option(const struct drm_device *dev, int
> enable_rc6)
> +static bool bxt_check_bios_rc6_setup(const struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool enable_rc6 = true;
> +	unsigned long rc6_ctx_base;
> +
> +	if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
> +		DRM_DEBUG_KMS("RC6 Base location not set
> properly.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	/*
> +	 * The exact context size is not known for BXT, so assume a
> page size
> +	 * for this check.
> +	 */
> +	rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK;
> +	if (!((rc6_ctx_base >= dev_priv->gtt.stolen_reserved_base)
> &&
> +	      (rc6_ctx_base + PAGE_SIZE <= dev_priv-
> >gtt.stolen_reserved_base +
> +					dev_priv-
> >gtt.stolen_reserved_size))) {
> +		DRM_DEBUG_KMS("RC6 Base address not as
> expected.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	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_DEBUG_KMS("Engine Idle wait time not set
> properly.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	if (!(I915_READ(GEN6_RC_CONTROL) & (GEN6_RC_CTL_RC6_ENABLE |
> +					    GEN6_RC_CTL_HW_ENABLE))
> &&
> +	    ((I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) ||
> +	     !(I915_READ(GEN6_RC_STATE) & RC6_STATE))) {
> +		DRM_DEBUG_KMS("HW/SW RC6 is not enabled by
> BIOS.\n");
> +		enable_rc6 = false;
> +	}
> +
> +	return enable_rc6;
> +}
> +
> +int sanitize_rc6_option(const struct drm_device *dev, int
> enable_rc6)
>  {
>  	/* No RC6 before Ironlake and code is gone for ilk. */
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return 0;
>  
> +	if (!enable_rc6)
> +		return 0;
> +
> +	if (IS_BROXTON(dev) && !bxt_check_bios_rc6_setup(dev)) {
> +		DRM_INFO("RC6 disabled by BIOS\n");
> +		return 0;
> +	}
> +
>  	/* Respect the kernel parameter if it is set */
>  	if (enable_rc6 >= 0) {
>  		int mask;
> @@ -6053,7 +6103,6 @@ void intel_init_gt_powersave(struct drm_device
> *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
>  	/*
>  	 * RPM depends on RC6 to save restore the GT HW context, so
> make RC6 a
>  	 * requirement.
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index bfa79e5..436d8f2 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -400,6 +400,8 @@ void intel_uncore_early_sanitize(struct
> drm_device *dev, bool restore_forcewake)
>  
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
> +	i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
> +
>  	/* 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

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

[Jani Nikula]

On Fri, 05 Feb 2016, Imre Deak <imre.deak@intel.com> wrote:
> On la, 2016-02-06 at 00:13 +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)
>> 
>> v5: Caching reserved stolen base and size in the driver private data.
>>     Reorganized RC6 setup check. Moved from gen9_enable_rc6 to
>>     intel_uncore_sanitize. (Imre)
>> 
>> v6: Rebasing on the patch submitted by Imre that moves
>> gem_init_stolen
>>     earlier in the load.
>> 
>> v7: Removed PWRCTX_MAXCNT_VCSUNIT1 check as it applies to SKL. (Imre)
>> 
>> v8: Fixed formatting and checkpatch issues. Fixed functional issue
>> where
>>     RC6 ctx size check was missing. (Imre)
>> 
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
> Thanks for the patch, I pushed it to -dinq.

The rule is, we should wait for the CI results before pushing.

BR,
Jani.



>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.h    |  2 ++
>>  drivers/gpu/drm/i915/i915_gem_stolen.c |  3 ++
>>  drivers/gpu/drm/i915/i915_reg.h        | 11 +++++++
>>  drivers/gpu/drm/i915/intel_drv.h       |  1 +
>>  drivers/gpu/drm/i915/intel_pm.c        | 53
>> ++++++++++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_uncore.c    |  2 ++
>>  6 files changed, 70 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index f520c90..66a6da2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -342,6 +342,8 @@ struct i915_gtt {
>>  
>>  	size_t stolen_size;		/* Total size of stolen
>> memory */
>>  	size_t stolen_usable_size;	/* Total size minus BIOS
>> reserved */
>> +	size_t stolen_reserved_base;
>> +	size_t stolen_reserved_size;
>>  	u64 mappable_end;		/* End offset that we can
>> CPU map */
>>  	struct io_mapping *mappable;	/* Mapping to our CPU
>> mappable region */
>>  	phys_addr_t mappable_base;	/* PA of our GMADR */
>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> index c384dc9..ba1a00d 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -458,6 +458,9 @@ int i915_gem_init_stolen(struct drm_device *dev)
>>  		return 0;
>>  	}
>>  
>> +	dev_priv->gtt.stolen_reserved_base = reserved_base;
>> +	dev_priv->gtt.stolen_reserved_size = reserved_size;
>> +
>>  	/* It is possible for the reserved area to end before the
>> end of stolen
>>  	 * memory, so just consider the start. */
>>  	reserved_total = stolen_top - reserved_base;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index c0bd691..71b1fe9 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6784,6 +6784,16 @@ enum skl_disp_power_wells {
>>  
>>  #define  VLV_PMWGICZ				_MMIO(0x1300a4)
>>  
>> +#define  RC6_LOCATION				_MMIO(0xD40)
>> +#define	   RC6_CTX_IN_DRAM			(1 << 0)
>> +#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)
>> @@ -6922,6 +6932,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_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 93ba14a..1251a7a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1566,6 +1566,7 @@ void skl_wm_get_hw_state(struct drm_device
>> *dev);
>>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>>  			  struct skl_ddb_allocation *ddb /* out */);
>>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
>> *pipe_config);
>> +int sanitize_rc6_option(const struct drm_device *dev, int
>> enable_rc6);
>>  
>>  /* intel_sdvo.c */
>>  bool intel_sdvo_init(struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index a47b8f2..78db72c 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4558,12 +4558,62 @@ static void intel_print_rc6_info(struct
>> drm_device *dev, u32 mode)
>>  			      onoff(mode & GEN6_RC_CTL_RC6_ENABLE));
>>  }
>>  
>> -static int sanitize_rc6_option(const struct drm_device *dev, int
>> enable_rc6)
>> +static bool bxt_check_bios_rc6_setup(const struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	bool enable_rc6 = true;
>> +	unsigned long rc6_ctx_base;
>> +
>> +	if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
>> +		DRM_DEBUG_KMS("RC6 Base location not set
>> properly.\n");
>> +		enable_rc6 = false;
>> +	}
>> +
>> +	/*
>> +	 * The exact context size is not known for BXT, so assume a
>> page size
>> +	 * for this check.
>> +	 */
>> +	rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK;
>> +	if (!((rc6_ctx_base >= dev_priv->gtt.stolen_reserved_base)
>> &&
>> +	      (rc6_ctx_base + PAGE_SIZE <= dev_priv-
>> >gtt.stolen_reserved_base +
>> +					dev_priv-
>> >gtt.stolen_reserved_size))) {
>> +		DRM_DEBUG_KMS("RC6 Base address not as
>> expected.\n");
>> +		enable_rc6 = false;
>> +	}
>> +
>> +	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_DEBUG_KMS("Engine Idle wait time not set
>> properly.\n");
>> +		enable_rc6 = false;
>> +	}
>> +
>> +	if (!(I915_READ(GEN6_RC_CONTROL) & (GEN6_RC_CTL_RC6_ENABLE |
>> +					    GEN6_RC_CTL_HW_ENABLE))
>> &&
>> +	    ((I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE) ||
>> +	     !(I915_READ(GEN6_RC_STATE) & RC6_STATE))) {
>> +		DRM_DEBUG_KMS("HW/SW RC6 is not enabled by
>> BIOS.\n");
>> +		enable_rc6 = false;
>> +	}
>> +
>> +	return enable_rc6;
>> +}
>> +
>> +int sanitize_rc6_option(const struct drm_device *dev, int
>> enable_rc6)
>>  {
>>  	/* No RC6 before Ironlake and code is gone for ilk. */
>>  	if (INTEL_INFO(dev)->gen < 6)
>>  		return 0;
>>  
>> +	if (!enable_rc6)
>> +		return 0;
>> +
>> +	if (IS_BROXTON(dev) && !bxt_check_bios_rc6_setup(dev)) {
>> +		DRM_INFO("RC6 disabled by BIOS\n");
>> +		return 0;
>> +	}
>> +
>>  	/* Respect the kernel parameter if it is set */
>>  	if (enable_rc6 >= 0) {
>>  		int mask;
>> @@ -6053,7 +6103,6 @@ void intel_init_gt_powersave(struct drm_device
>> *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> -	i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
>>  	/*
>>  	 * RPM depends on RC6 to save restore the GT HW context, so
>> make RC6 a
>>  	 * requirement.
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index bfa79e5..436d8f2 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -400,6 +400,8 @@ void intel_uncore_early_sanitize(struct
>> drm_device *dev, bool restore_forcewake)
>>  
>>  void intel_uncore_sanitize(struct drm_device *dev)
>>  {
>> +	i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
>> +
>>  	/* 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ma, 2016-02-08 at 11:19 +0200, Jani Nikula wrote:
> On Fri, 05 Feb 2016, Imre Deak <imre.deak@intel.com> wrote:
> > On la, 2016-02-06 at 00:13 +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)
> > > 
> > > v5: Caching reserved stolen base and size in the driver private
> > > data.
> > >     Reorganized RC6 setup check. Moved from gen9_enable_rc6 to
> > >     intel_uncore_sanitize. (Imre)
> > > 
> > > v6: Rebasing on the patch submitted by Imre that moves
> > > gem_init_stolen
> > >     earlier in the load.
> > > 
> > > v7: Removed PWRCTX_MAXCNT_VCSUNIT1 check as it applies to SKL.
> > > (Imre)
> > > 
> > > v8: Fixed formatting and checkpatch issues. Fixed functional
> > > issue
> > > where
> > >     RC6 ctx size check was missing. (Imre)
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > 
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > Thanks for the patch, I pushed it to -dinq.
> 
> The rule is, we should wait for the CI results before pushing.

Yes, I forgot to wait for the result for this version of the patch,
thanks for pointing it out. As a side-note the CI result still didn't
show up, what to do in that case? Resend the patch after a day or so?

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

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

[Jani Nikula]

On Mon, 08 Feb 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > Thanks for the patch, I pushed it to -dinq.
>> 
>> The rule is, we should wait for the CI results before pushing.
>
> Yes, I forgot to wait for the result for this version of the patch,
> thanks for pointing it out. As a side-note the CI result still didn't
> show up, what to do in that case? Resend the patch after a day or so?

Complain to Damien and/or Tomi.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Mon, Feb 08, 2016 at 05:08:03PM +0200, Jani Nikula wrote:
> On Mon, 08 Feb 2016, Imre Deak <imre.deak@intel.com> wrote:
> >> > Thanks for the patch, I pushed it to -dinq.
> >> 
> >> The rule is, we should wait for the CI results before pushing.
> >
> > Yes, I forgot to wait for the result for this version of the patch,
> > thanks for pointing it out. As a side-note the CI result still didn't
> > show up, what to do in that case? Resend the patch after a day or so?
> 
> Complain to Damien and/or Tomi.

CI is/was down. Please don't push patches when that happens, but instead
wait for things to stabilize again. Yes that slows things down, but otoh
everytime CI is down we manage to sneak in an overlapping regression. And
recovering from overlapping regressions makes everything even more painful
than it already is.

Yes, some kind of CI status dashboard is on the wishlist.
-Daniel
-- 
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 v8 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Tomi Sarvela]

On Monday 15 February 2016 18:07:47 Daniel Vetter wrote:
> On Mon, Feb 08, 2016 at 05:08:03PM +0200, Jani Nikula wrote:
> > On Mon, 08 Feb 2016, Imre Deak <imre.deak@intel.com> wrote:
> > >> > Thanks for the patch, I pushed it to -dinq.
> > >> 
> > >> The rule is, we should wait for the CI results before pushing.
> > > 
> > > Yes, I forgot to wait for the result for this version of the patch,
> > > thanks for pointing it out. As a side-note the CI result still didn't
> > > show up, what to do in that case? Resend the patch after a day or so?
> > 
> > Complain to Damien and/or Tomi.
> 
> CI is/was down. Please don't push patches when that happens, but instead
> wait for things to stabilize again. Yes that slows things down, but otoh
> everytime CI is down we manage to sneak in an overlapping regression. And
> recovering from overlapping regressions makes everything even more painful
> than it already is.
> 
> Yes, some kind of CI status dashboard is on the wishlist.

Well, CI wasn't down, it's just that there wasn't stable drm-intel-nightly to 
apply patches on. There could be something CI could do when tree is burning, 
and trying patches on *some* old version might not be the best choice, 
especially when patches might try to fix the breakage. Ideas welcome.

I'm halting the Patchwork processing when in above situation. This means that 
the non-tested Patchwork patchsets will be tested when the tree is in 
approximately good condition. Resending doesn't change anything.

My opinion is that as Patchwork is heavily dependent on drm-intel-nightly, 
this is probably how it should go, but automatically, without my intervention.

What kind of status dashboard would be good? As mentioned, CI itself was all 
good and well, and status board would reflect that.

Tomi



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

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

[Daniel Vetter]

On Tue, Feb 16, 2016 at 09:52:49AM +0200, Tomi Sarvela wrote:
> On Monday 15 February 2016 18:07:47 Daniel Vetter wrote:
> > On Mon, Feb 08, 2016 at 05:08:03PM +0200, Jani Nikula wrote:
> > > On Mon, 08 Feb 2016, Imre Deak <imre.deak@intel.com> wrote:
> > > >> > Thanks for the patch, I pushed it to -dinq.
> > > >> 
> > > >> The rule is, we should wait for the CI results before pushing.
> > > > 
> > > > Yes, I forgot to wait for the result for this version of the patch,
> > > > thanks for pointing it out. As a side-note the CI result still didn't
> > > > show up, what to do in that case? Resend the patch after a day or so?
> > > 
> > > Complain to Damien and/or Tomi.
> > 
> > CI is/was down. Please don't push patches when that happens, but instead
> > wait for things to stabilize again. Yes that slows things down, but otoh
> > everytime CI is down we manage to sneak in an overlapping regression. And
> > recovering from overlapping regressions makes everything even more painful
> > than it already is.
> > 
> > Yes, some kind of CI status dashboard is on the wishlist.
> 
> Well, CI wasn't down, it's just that there wasn't stable drm-intel-nightly to 
> apply patches on. There could be something CI could do when tree is burning, 
> and trying patches on *some* old version might not be the best choice, 
> especially when patches might try to fix the breakage. Ideas welcome.
> 
> I'm halting the Patchwork processing when in above situation. This means that 
> the non-tested Patchwork patchsets will be tested when the tree is in 
> approximately good condition. Resending doesn't change anything.
> 
> My opinion is that as Patchwork is heavily dependent on drm-intel-nightly, 
> this is probably how it should go, but automatically, without my intervention.
> 
> What kind of status dashboard would be good? As mentioned, CI itself was all 
> good and well, and status board would reflect that.

Well status board should also include status of patchwork jobs, and if
they're down why exactly ("drm-intel-nightly on fire"). So yeah I don't
expect that the status board will show "CI is down" at all ever (or at
least I hope), but communicating why patches aren't being tested would be
good. Later on we could also put up some kind of queue, so that people
know how long it'll take when there's a backlog for CI to get around to
their patch.
-Daniel
-- 
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 v8 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Tomi Sarvela]

On Tuesday 16 February 2016 16:28:43 Daniel Vetter wrote:
> On Tue, Feb 16, 2016 at 09:52:49AM +0200, Tomi Sarvela wrote:
> > What kind of status dashboard would be good? As mentioned, CI itself was
> > all good and well, and status board would reflect that.
> 
> Well status board should also include status of patchwork jobs, and if
> they're down why exactly ("drm-intel-nightly on fire"). So yeah I don't
> expect that the status board will show "CI is down" at all ever (or at
> least I hope), but communicating why patches aren't being tested would be
> good. Later on we could also put up some kind of queue, so that people
> know how long it'll take when there's a backlog for CI to get around to
> their patch.

The problem is that the patches are not taken from one queue: there is (in 
priority order) CI_DIF, CI_DINF, CI_DRM, CI_Patchwork and soon-to-be 
CI_Private. Patchwork and Private work by advancing from old patchseries to 
new, others just take newest and roll with it (no matter how many new 
commits). In short, we're doing one thing at a time, statelessly.

So, what can be done:

Patchwork/Private queue can be scanned for the next <n> patches, but the order 
changes if there's new revision, and the estimated time changes if there's 
drm-intel-nightly pushed (which is higher in priority). Can't know either in 
advance. Might be still useful to get information that next two things to be 
tested is "probably not you".

Jenkins status can be queried, but it's usually "Yes, I'm working". More 
specifically it can return easily CI_DRM or CI_Patchwork job it's waiting to 
finish, but that isn't terribly useful.

Jenkins jobs can be queried easily: interesting might be status of 
CI_Patchwork_check (disabled if there's hot in CI_DRM side).

Better estimates would all depend on getting rid of the prio-queues. That 
might be possible, but current order has worked well so far.

Tomi


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