| Message ID | 1449823450-24165-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 --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); }
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(+)