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