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

Kamble, Sagar A
2016-07-01T10:56:42Z

On 7/1/2016 2:45 PM, Imre Deak wrote:
> 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.
Fine. With this:
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
>> 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?
We can skip given that these are not so much impacting RC6 as you said.

>
> --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@intel.com>
>>> Signed-off-by: Imre Deak <imre.deak@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;
>>> }
>>>