[Intel-gfx] [PATCH v3 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Daniel Vetter]

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 at 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 at lists.freedesktop.org
> > 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