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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux