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 <[email protected]> > > --- > > 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 > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
