[Intel-gfx] [PATCH v6 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6
Kamble, Sagar A
sagar.a.kamble at intel.com
Tue Feb 2 18:06:55 UTC 2016
Thanks for the review Imre.
I Kept that BSD2 check thinking some BXT revision might support it :)
Will remove the check and send the patch.
Thanks
Sagar
On 2/2/2016 9:48 PM, Imre Deak wrote:
> On pe, 2016-01-29 at 23:22 +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.
>>
>> 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.
>>
>> v4: Added elaborate check for BIOS RC6 setup. Prepared check_pctx for bxt. (Imre)
>>
>> v5: Caching reserved stolen base and size in the driver private data.
>> Reorganized RC6 setup check. Moved from gen9_enable_rc6 to intel_uncore_sanitize. (Imre)
>>
>> v6: Rebasing on the patch submitted by Imre that moves gem_init_stolen earlier in the load.
>>
>> Cc: Imre Deak <imre.deak at intel.com>
>> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
>> drivers/gpu/drm/i915/i915_gem_stolen.c | 3 ++
>> drivers/gpu/drm/i915/i915_reg.h | 11 +++++++
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_pm.c | 60 ++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/intel_uncore.c | 2 ++
>> 6 files changed, 77 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index f520c90..66a6da2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -342,6 +342,8 @@ struct i915_gtt {
>>
>> size_t stolen_size; /* Total size of stolen memory */
>> size_t stolen_usable_size; /* Total size minus BIOS reserved */
>> + size_t stolen_reserved_base;
>> + size_t stolen_reserved_size;
>> u64 mappable_end; /* End offset that we can CPU map */
>> struct io_mapping *mappable; /* Mapping to our CPU mappable region */
>> phys_addr_t mappable_base; /* PA of our GMADR */
>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> index c384dc9..ba1a00d 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -458,6 +458,9 @@ int i915_gem_init_stolen(struct drm_device *dev)
>> return 0;
>> }
>>
>> + dev_priv->gtt.stolen_reserved_base = reserved_base;
>> + dev_priv->gtt.stolen_reserved_size = reserved_size;
>> +
>> /* It is possible for the reserved area to end before the end of stolen
>> * memory, so just consider the start. */
>> reserved_total = stolen_top - reserved_base;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 65e32a3..00aac28 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6781,6 +6781,16 @@ enum skl_disp_power_wells {
>>
>> #define VLV_PMWGICZ _MMIO(0x1300a4)
>>
>> +#define RC6_LOCATION _MMIO(0xD40)
>> +#define RC6_CTX_IN_DRAM 1
>> +#define RC6_CTX_BASE _MMIO(0xD48)
>> +#define RC6_CTX_BASE_MASK 0xFFFFFFF0
>> +#define PWRCTX_MAXCNT_RCSUNIT _MMIO(0x2054)
>> +#define PWRCTX_MAXCNT_VCSUNIT0 _MMIO(0x12054)
>> +#define PWRCTX_MAXCNT_BCSUNIT _MMIO(0x22054)
>> +#define PWRCTX_MAXCNT_VECSUNIT _MMIO(0x1A054)
>> +#define PWRCTX_MAXCNT_VCSUNIT1 _MMIO(0x1C054)
>> +#define IDLE_TIME_MASK 0xFFFFF
>> #define FORCEWAKE _MMIO(0xA18C)
>> #define FORCEWAKE_VLV _MMIO(0x1300b0)
>> #define FORCEWAKE_ACK_VLV _MMIO(0x1300b4)
>> @@ -6919,6 +6929,7 @@ enum skl_disp_power_wells {
>> #define GEN6_RPDEUC _MMIO(0xA084)
>> #define GEN6_RPDEUCSW _MMIO(0xA088)
>> #define GEN6_RC_STATE _MMIO(0xA094)
>> +#define RC6_STATE (1<<18)
>> #define GEN6_RC1_WAKE_RATE_LIMIT _MMIO(0xA098)
>> #define GEN6_RC6_WAKE_RATE_LIMIT _MMIO(0xA09C)
>> #define GEN6_RC6pp_WAKE_RATE_LIMIT _MMIO(0xA0A0)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index f620023..aaa2051a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1560,6 +1560,7 @@ void skl_wm_get_hw_state(struct drm_device *dev);
>> void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>> struct skl_ddb_allocation *ddb /* out */);
>> uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>> +int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6);
>>
>> /* intel_sdvo.c */
>> bool intel_sdvo_init(struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 31bc4ea..8929f69 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4558,12 +4558,69 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
>> onoff(mode & GEN6_RC_CTL_RC6_ENABLE));
>> }
>>
>> -static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
>> +static bool bxt_check_bios_rc6_setup(const struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + bool enable_rc6 = true;
>> + unsigned long rc6_ctx_base;
>> + bool bios_hw_rc6_enabled, bios_sw_rc6_enabled;
>> +
>> + bios_hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
>> + (GEN6_RC_CTL_RC6_ENABLE |
>> + GEN6_RC_CTL_HW_ENABLE);
>> + bios_sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) &
>> + GEN6_RC_CTL_HW_ENABLE) &&
>> + (I915_READ(GEN6_RC_STATE) & RC6_STATE);
>> +
>> + if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
>> + DRM_DEBUG_KMS("RC6 Base location not set properly.\n");
>> + enable_rc6 = false;
>> + }
>> +
>> + rc6_ctx_base = I915_READ(RC6_CTX_BASE) & RC6_CTX_BASE_MASK;
>> + if (!((rc6_ctx_base >= dev_priv->gtt.stolen_reserved_base) &&
>> + (rc6_ctx_base <= (dev_priv->gtt.stolen_reserved_base +
>> + dev_priv->gtt.stolen_reserved_size)))) {
>> + DRM_DEBUG_KMS("RC6 Base address not as expected.\n");
>> + enable_rc6 = false;
>> + }
>> +
>> + if (!(((I915_READ(PWRCTX_MAXCNT_RCSUNIT) & IDLE_TIME_MASK) > 1) &&
>> + ((I915_READ(PWRCTX_MAXCNT_VCSUNIT0) & IDLE_TIME_MASK) > 1) &&
>> + ((I915_READ(PWRCTX_MAXCNT_BCSUNIT) & IDLE_TIME_MASK) > 1) &&
>> + ((I915_READ(PWRCTX_MAXCNT_VECSUNIT) & IDLE_TIME_MASK) > 1))) {
>> + DRM_DEBUG_KMS("Engine Idle wait time not set properly.\n");
>> + enable_rc6 = false;
>> + }
>> +
>> + if (HAS_BSD2(dev) &&
>> + ((I915_READ(PWRCTX_MAXCNT_VCSUNIT1) & IDLE_TIME_MASK) > 1)) {
>> + DRM_DEBUG_KMS("VCSUNIT1 Idle wait time not set properly.\n");
>> + enable_rc6 = false;
>> + }
> I missed this from the previous review: this check would make sense on
> SKL, but since the function is BXT specific it shouldn't be here, so
> please remove checking PWRCTX_MAXCNT_VCSUNIT1 for now. With that this
> is:
> Reviewed-by: Imre Deak <imre.deak at intel.com>
>
>> +
>> + if (!bios_hw_rc6_enabled && !bios_sw_rc6_enabled) {
>> + DRM_DEBUG_KMS("HW/SW RC6 is not enabled by BIOS.\n");
>> + enable_rc6 = false;
>> + }
>> +
>> + return enable_rc6;
>> +}
>> +
>> +int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
>> {
>> /* No RC6 before Ironlake and code is gone for ilk. */
>> if (INTEL_INFO(dev)->gen < 6)
>> return 0;
>>
>> + if (!enable_rc6)
>> + return 0;
>> +
>> + if (IS_BROXTON(dev) && !bxt_check_bios_rc6_setup(dev)) {
>> + DRM_INFO("RC6 disabled by BIOS\n");
>> + return 0;
>> + }
>> +
>> /* Respect the kernel parameter if it is set */
>> if (enable_rc6 >= 0) {
>> int mask;
>> @@ -6053,7 +6110,6 @@ void intel_init_gt_powersave(struct drm_device *dev)
>> {
>> struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> - i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
>> /*
>> * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
>> * requirement.
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index bfa79e5..436d8f2 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -400,6 +400,8 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
>>
>> void intel_uncore_sanitize(struct drm_device *dev)
>> {
>> + i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
>> +
>> /* BIOS often leaves RC6 enabled, but disable it for hw init */
>> intel_disable_gt_powersave(dev);
>> }
More information about the Intel-gfx
mailing list