Re: [PATCH v8 1/1] drm/i915/bxt: Check BIOS RC6 setup before enabling RC6

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 05 Feb 2016, Imre Deak <imre.deak@intel.com> wrote:
> On la, 2016-02-06 at 00:13 +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.
>> 
>> v7: Removed PWRCTX_MAXCNT_VCSUNIT1 check as it applies to SKL. (Imre)
>> 
>> v8: Fixed formatting and checkpatch issues. Fixed functional issue
>> where
>>     RC6 ctx size check was missing. (Imre)
>> 
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
> Thanks for the patch, I pushed it to -dinq.

The rule is, we should wait for the CI results before pushing.

BR,
Jani.



>
>> ---
>>  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        | 53
>> ++++++++++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_uncore.c    |  2 ++
>>  6 files changed, 70 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 c0bd691..71b1fe9 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6784,6 +6784,16 @@ enum skl_disp_power_wells {
>>  
>>  #define  VLV_PMWGICZ				_MMIO(0x1300a4)
>>  
>> +#define  RC6_LOCATION				_MMIO(0xD40)
>> +#define	   RC6_CTX_IN_DRAM			(1 << 0)
>> +#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)
>> @@ -6922,6 +6932,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 93ba14a..1251a7a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1566,6 +1566,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 a47b8f2..78db72c 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4558,12 +4558,62 @@ 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;
>> +
>> +	if (!(I915_READ(RC6_LOCATION) & RC6_CTX_IN_DRAM)) {
>> +		DRM_DEBUG_KMS("RC6 Base location not set
>> properly.\n");
>> +		enable_rc6 = false;
>> +	}
>> +
>> +	/*
>> +	 * The exact context size is not known for BXT, so assume a
>> page size
>> +	 * for this check.
>> +	 */
>> +	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 + PAGE_SIZE <= 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 (!(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_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 +6103,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);
>>  }
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx