[Intel-gfx] [PATCH 2/2] drm/i915/bxt: Fix sanity check for BIOS RC6 setup

[Imre Deak]

On pe, 2016-07-01 at 12:19 +0530, Kamble, Sagar A wrote:

> Have seen BIOS having option "RC6" disabled and "GTPM" enabled for cases 
> where there are RC6 specific issues.

It's possible although I haven't seen any based on the specs I have and
the tests I ran. In any case the checks I added should catch any such
missing setup and if there is something on top of that we need to add
those (along with an update to the specification). 

> GTPM option entails setup for other features as well I guess.

Yes, it affects RPS setup too, but my point was that disabling it is
what leaves RC6 unconfigured. I guess this doesn't really matter in the
end, the main thing is that we check all the RC6 specific registers.

> In such cases - Can we output some DRM_INFO log saying BIOS has disabled 
> RC6 although setup is available.

Yes, can add that, but since it's something we'd need for debugging I'd
use DRM_DEBUG.

> Do we need to also check for other unit level clock gating register 
> setup done by BIOS like: GEN7_MISCCPCTL, GEN6_UCGCTL1 to
> GEN6_UCGCTL4, 
> GEN8_UCGCTL6 etc.

These are subject to change with later HW steppings. In any case their
default value is the more conservative scenario with clock gating
disabled, which should still allow RC6 functionality. They can be also
enabled/disabled separately from the GTPM option in BIOS setup (via
sub-options to GTPM), something we haven't checked so far anyway.

Are you ok if I add a debug print for these too?

--Imre

> 
> Thanks
> Sagar
> 
> 
> On 6/29/2016 9:43 PM, Imre Deak wrote:
> > BXT BIOS has two options related to GPU power management:
> > "RC6(Render
> > Standby)" and "GT PM Support". The assumption so far was that
> > disabling
> > either of these options would leave RC6 uninitialized. According to
> > my
> > tests this isn't so: for a proper RC6 setup we only need the "GT PM
> > Support" option to be enabled while the "RC6" option only controls
> > whether RC6 is left enabled or not by BIOS. OTOH we were missing a
> > few
> > checks to ensure a proper RC6 setup. Add these now and don't fail
> > the
> > sanity check if RC6 is disabled. This fixes a problem where RC6
> > remains
> > disabled after reloading the driver, since we explicitly disable
> > RC6
> > during unloading.
> > 
> > CC: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h |  5 +++++
> >   drivers/gpu/drm/i915/intel_pm.c | 19 ++++++++++++++-----
> >   2 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index c6bfbf8..92b4046 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7085,12 +7085,17 @@ enum {
> >   #define GEN6_RC6pp_THRESHOLD			_MMIO(0xA0C0)
> >   #define GEN6_PMINTRMSK				_MMIO(0xA16
> > 8)
> >   #define   GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
> > +#define GEN8_MISC_CTRL0				_MMIO(0xA18
> > 0)
> >   #define VLV_PWRDWNUPCTL				_MMIO(0xA2
> > 94)
> >   #define GEN9_MEDIA_PG_IDLE_HYSTERESIS		_MMIO(0xA0C4
> > )
> >   #define GEN9_RENDER_PG_IDLE_HYSTERESIS		_MMIO(0xA0C
> > 8)
> >   #define GEN9_PG_ENABLE				_MMIO(0xA21
> > 0)
> >   #define GEN9_RENDER_PG_ENABLE			(1<<0)
> >   #define GEN9_MEDIA_PG_ENABLE			(1<<1)
> > +#define GEN8_PUSHBUS_CONTROL			_MMIO(0xA248)
> > +#define GEN8_PUSHBUS_ENABLE			_MMIO(0xA250)
> > +#define GEN8_PUSHBUS_SHIFT			_MMIO(0xA25C)
> > +
> >   
> >   #define VLV_CHICKEN_3				_MMIO(VLV_DI
> > SPLAY_BASE + 0x7040C)
> >   #define  PIXEL_OVERLAP_CNT_MASK			(3 << 30)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 5dce264..fe76991 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5015,11 +5015,20 @@ static bool bxt_check_bios_rc6_setup(struct
> > drm_i915_private *dev_priv)
> >   		enable_rc6 = false;
> >   	}
> >   
> > -	if (!(I915_READ(GEN6_RC_CONTROL) & (GEN6_RC_CTL_RC6_ENABLE
> > |
> > -					    GEN6_RC_CTL_HW_ENABLE)
> > ) &&
> > -	    ((I915_READ(GEN6_RC_CONTROL) & GEN6_RC_CTL_HW_ENABLE)
> > ||
> > -	     !(I915_READ(GEN6_RC_STATE) & RC6_STATE))) {
> > -		DRM_DEBUG_DRIVER("HW/SW RC6 is not enabled by
> > BIOS.\n");
> > +	if (!I915_READ(GEN8_PUSHBUS_CONTROL) ||
> > +	    !I915_READ(GEN8_PUSHBUS_ENABLE) ||
> > +	    !I915_READ(GEN8_PUSHBUS_SHIFT)) {
> > +		DRM_DEBUG_DRIVER("Pushbus not setup properly.\n");
> > +		enable_rc6 = false;
> > +	}
> > +
> > +	if (!I915_READ(GEN6_GFXPAUSE)) {
> > +		DRM_DEBUG_DRIVER("GFX pause not setup
> > properly.\n");
> > +		enable_rc6 = false;
> > +	}
> > +
> > +	if (!I915_READ(GEN8_MISC_CTRL0)) {
> > +		DRM_DEBUG_DRIVER("GPM control not setup
> > properly.\n");
> >   		enable_rc6 = false;
> >   	}
> >   
>