Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Mar 25, 2017 at 02:23:44PM +0000, Chris Wilson wrote:
> It has been many years since the last confirmed sighting (and fix) of an
> RC6 related bug (usually a system hang). Remove the parameter to stop
> users from setting dangerous values.
> 
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx

I guess _unsafe is not strong enough, and seems reasonable.

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

> ---
>  drivers/gpu/drm/i915/i915_drv.c     |   2 +-
>  drivers/gpu/drm/i915/i915_drv.h     |   1 +
>  drivers/gpu/drm/i915/i915_params.c  |   9 ---
>  drivers/gpu/drm/i915/i915_params.h  |   1 -
>  drivers/gpu/drm/i915/i915_pci.c     |   3 +-
>  drivers/gpu/drm/i915/intel_drv.h    |   5 --
>  drivers/gpu/drm/i915/intel_pm.c     | 126 ++++++++----------------------------
>  drivers/gpu/drm/i915/intel_uc.c     |   2 +-
>  drivers/gpu/drm/i915/intel_uncore.c |   2 -
>  9 files changed, 33 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6020ef8bd3b2..1223c677cbf9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2367,7 +2367,7 @@ static int intel_runtime_suspend(struct device *kdev)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	int ret;
>  
> -	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6())))
> +	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && HAS_RC6(dev_priv))))
>  		return -ENODEV;
>  
>  	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15a0c31a713d..7ac115de62e2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2961,6 +2961,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define HAS_PSR(dev_priv)		 ((dev_priv)->info.has_psr)
>  #define HAS_RC6(dev_priv)		 ((dev_priv)->info.has_rc6)
>  #define HAS_RC6p(dev_priv)		 ((dev_priv)->info.has_rc6p)
> +#define HAS_RC6pp(dev_priv)		 (false)
>  
>  #define HAS_CSR(dev_priv)	((dev_priv)->info.has_csr)
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8241b673940d..3137572752e3 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -32,7 +32,6 @@ struct i915_params i915 __read_mostly = {
>  	.lvds_channel_mode = 0,
>  	.panel_use_ssc = -1,
>  	.vbt_sdvo_panel_type = -1,
> -	.enable_rc6 = -1,
>  	.enable_dc = -1,
>  	.enable_fbc = -1,
>  	.enable_hangcheck = true,
> @@ -81,14 +80,6 @@ MODULE_PARM_DESC(semaphores,
>  	"Use semaphores for inter-ring sync "
>  	"(default: -1 (use per-chip defaults))");
>  
> -module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, 0400);
> -MODULE_PARM_DESC(enable_rc6,
> -	"Enable power-saving render C-state 6. "
> -	"Different stages can be selected via bitmask values "
> -	"(0 = disable; 1 = enable rc6; 2 = enable deep rc6; 4 = enable deepest rc6). "
> -	"For example, 3 would enable rc6 and deep rc6, and 7 would enable everything. "
> -	"default: -1 (use per-chip default)");
> -
>  module_param_named_unsafe(enable_dc, i915.enable_dc, int, 0400);
>  MODULE_PARM_DESC(enable_dc,
>  	"Enable power-saving display C-states. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index b7bf1578f3eb..b6686a5450fa 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -34,7 +34,6 @@
>  	func(int, lvds_channel_mode); \
>  	func(int, panel_use_ssc); \
>  	func(int, vbt_sdvo_panel_type); \
> -	func(int, enable_rc6); \
>  	func(int, enable_dc); \
>  	func(int, enable_fbc); \
>  	func(int, enable_ppgtt); \
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index f87b0c4e564d..e0846c6c33ae 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -200,6 +200,8 @@ static const struct intel_device_info intel_gm45_info = {
>  	GEN_DEFAULT_PIPEOFFSETS, \
>  	CURSOR_OFFSETS
>  
> +/* Ironlake does support rc6, but we do not implement the power contexts */
> +
>  static const struct intel_device_info intel_ironlake_d_info = {
>  	GEN5_FEATURES,
>  	.platform = INTEL_IRONLAKE,
> @@ -218,7 +220,6 @@ static const struct intel_device_info intel_ironlake_m_info = {
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
>  	.has_llc = 1, \
>  	.has_rc6 = 1, \
> -	.has_rc6p = 1, \
>  	.has_gmbus_irq = 1, \
>  	.has_hw_contexts = 1, \
>  	.has_aliasing_ppgtt = 1, \
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0b3c813336d5..05e5f3b6f2c8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1831,11 +1831,6 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
>  				 const struct skl_ddb_entry *ddb,
>  				 int ignore);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
> -int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> -static inline int intel_enable_rc6(void)
> -{
> -	return i915.enable_rc6;
> -}
>  
>  /* intel_sdvo.c */
>  bool intel_sdvo_init(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ab6fff8369ac..9c92b908530f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5460,26 +5460,6 @@ static void valleyview_disable_rps(struct drm_i915_private *dev_priv)
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
>  
> -static void intel_print_rc6_info(struct drm_i915_private *dev_priv, u32 mode)
> -{
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		if (mode & (GEN7_RC_CTL_TO_MODE | GEN6_RC_CTL_EI_MODE(1)))
> -			mode = GEN6_RC_CTL_RC6_ENABLE;
> -		else
> -			mode = 0;
> -	}
> -	if (HAS_RC6p(dev_priv))
> -		DRM_DEBUG_DRIVER("Enabling RC6 states: "
> -				 "RC6 %s RC6p %s RC6pp %s\n",
> -				 onoff(mode & GEN6_RC_CTL_RC6_ENABLE),
> -				 onoff(mode & GEN6_RC_CTL_RC6p_ENABLE),
> -				 onoff(mode & GEN6_RC_CTL_RC6pp_ENABLE));
> -
> -	else
> -		DRM_DEBUG_DRIVER("Enabling RC6 states: RC6 %s\n",
> -				 onoff(mode & GEN6_RC_CTL_RC6_ENABLE));
> -}
> -
>  static bool bxt_check_bios_rc6_setup(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> @@ -5542,42 +5522,16 @@ static bool bxt_check_bios_rc6_setup(struct drm_i915_private *dev_priv)
>  	return enable_rc6;
>  }
>  
> -int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6)
> +static bool sanitize_rc6(struct drm_i915_private *i915)
>  {
> -	/* No RC6 before Ironlake and code is gone for ilk. */
> -	if (INTEL_INFO(dev_priv)->gen < 6)
> -		return 0;
> -
> -	if (!enable_rc6)
> -		return 0;
> +	struct intel_device_info *info = mkwrite_device_info(i915);
>  
> -	if (IS_GEN9_LP(dev_priv) && !bxt_check_bios_rc6_setup(dev_priv)) {
> +	if (IS_GEN9_LP(i915) && !bxt_check_bios_rc6_setup(i915)) {
>  		DRM_INFO("RC6 disabled by BIOS\n");
> -		return 0;
> -	}
> -
> -	/* Respect the kernel parameter if it is set */
> -	if (enable_rc6 >= 0) {
> -		int mask;
> -
> -		if (HAS_RC6p(dev_priv))
> -			mask = INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE |
> -			       INTEL_RC6pp_ENABLE;
> -		else
> -			mask = INTEL_RC6_ENABLE;
> -
> -		if ((enable_rc6 & mask) != enable_rc6)
> -			DRM_DEBUG_DRIVER("Adjusting RC6 mask to %d "
> -					 "(requested %d, valid %d)\n",
> -					 enable_rc6 & mask, enable_rc6, mask);
> -
> -		return enable_rc6 & mask;
> +		info->has_rc6 = 0;
>  	}
>  
> -	if (IS_IVYBRIDGE(dev_priv))
> -		return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
> -
> -	return INTEL_RC6_ENABLE;
> +	return info->has_rc6;
>  }
>  
>  static void gen6_init_rps_frequencies(struct drm_i915_private *dev_priv)
> @@ -5666,7 +5620,6 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> -	uint32_t rc6_mask = 0;
>  
>  	/* 1a: Software RC state - RC0 */
>  	I915_WRITE(GEN6_RC_STATE, 0);
> @@ -5700,22 +5653,19 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN9_RENDER_PG_IDLE_HYSTERESIS, 25);
>  
>  	/* 3a: Enable RC6 */
> -	if (intel_enable_rc6() & INTEL_RC6_ENABLE)
> -		rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
> -	DRM_INFO("RC6 %s\n", onoff(rc6_mask & GEN6_RC_CTL_RC6_ENABLE));
>  	I915_WRITE(GEN6_RC6_THRESHOLD, 37500); /* 37.5/125ms per EI */
>  	I915_WRITE(GEN6_RC_CONTROL,
> -		   GEN6_RC_CTL_HW_ENABLE | GEN6_RC_CTL_EI_MODE(1) | rc6_mask);
> +		   GEN6_RC_CTL_HW_ENABLE |
> +		   GEN6_RC_CTL_EI_MODE(1) |
> +		   GEN6_RC_CTL_RC6_ENABLE);
>  
>  	/*
>  	 * 3b: Enable Coarse Power Gating only when RC6 is enabled.
>  	 * WaRsDisableCoarsePowerGating:skl,bxt - Render/Media PG need to be disabled with RC6.
>  	 */
> -	if (NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
> -		I915_WRITE(GEN9_PG_ENABLE, 0);
> -	else
> -		I915_WRITE(GEN9_PG_ENABLE, (rc6_mask & GEN6_RC_CTL_RC6_ENABLE) ?
> -				(GEN9_RENDER_PG_ENABLE | GEN9_MEDIA_PG_ENABLE) : 0);
> +	I915_WRITE(GEN9_PG_ENABLE,
> +		   NEEDS_WaRsDisableCoarsePowerGating(dev_priv) ?
> +		   (GEN9_RENDER_PG_ENABLE | GEN9_MEDIA_PG_ENABLE) : 0);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
> @@ -5724,7 +5674,6 @@ static void gen8_enable_rps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> -	uint32_t rc6_mask = 0;
>  
>  	/* 1a: Software RC state - RC0 */
>  	I915_WRITE(GEN6_RC_STATE, 0);
> @@ -5749,17 +5698,14 @@ static void gen8_enable_rps(struct drm_i915_private *dev_priv)
>  		I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */
>  
>  	/* 3: Enable RC6 */
> -	if (intel_enable_rc6() & INTEL_RC6_ENABLE)
> -		rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
> -	intel_print_rc6_info(dev_priv, rc6_mask);
>  	if (IS_BROADWELL(dev_priv))
>  		I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
>  				GEN7_RC_CTL_TO_MODE |
> -				rc6_mask);
> +				GEN6_RC_CTL_RC6_ENABLE);
>  	else
>  		I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
>  				GEN6_RC_CTL_EI_MODE(1) |
> -				rc6_mask);
> +				GEN6_RC_CTL_RC6_ENABLE);
>  
>  	/* 4 Program defaults and thresholds for RPS*/
>  	I915_WRITE(GEN6_RPNSWREQ,
> @@ -5801,9 +5747,8 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> -	u32 rc6vids, rc6_mask = 0;
> +	u32 rc6vids, rc6_mask;
>  	u32 gtfifodbg;
> -	int rc6_mode;
>  	int ret;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> @@ -5846,22 +5791,12 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN6_RC6p_THRESHOLD, 150000);
>  	I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
>  
> -	/* Check if we are enabling RC6 */
> -	rc6_mode = intel_enable_rc6();
> -	if (rc6_mode & INTEL_RC6_ENABLE)
> -		rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
> -
>  	/* We don't use those on Haswell */
> -	if (!IS_HASWELL(dev_priv)) {
> -		if (rc6_mode & INTEL_RC6p_ENABLE)
> -			rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
> -
> -		if (rc6_mode & INTEL_RC6pp_ENABLE)
> -			rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
> -	}
> -
> -	intel_print_rc6_info(dev_priv, rc6_mask);
> -
> +	rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
> +	if (HAS_RC6p(dev_priv))
> +		rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
> +	if (HAS_RC6pp(dev_priv))
> +		rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
>  	I915_WRITE(GEN6_RC_CONTROL,
>  		   rc6_mask |
>  		   GEN6_RC_CTL_EI_MODE(1) |
> @@ -6290,7 +6225,7 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> -	u32 gtfifodbg, val, rc6_mode = 0, pcbr;
> +	u32 gtfifodbg, val, rc6_mode, pcbr;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> @@ -6333,10 +6268,9 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
>  	pcbr = I915_READ(VLV_PCBR);
>  
>  	/* 3: Enable RC6 */
> -	if ((intel_enable_rc6() & INTEL_RC6_ENABLE) &&
> -	    (pcbr >> VLV_PCBR_ADDR_SHIFT))
> +	rc6_mode = 0;
> +	if (pcbr >> VLV_PCBR_ADDR_SHIFT)
>  		rc6_mode = GEN7_RC_CTL_TO_MODE;
> -
>  	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
>  
>  	/* 4 Program defaults and thresholds for RPS*/
> @@ -6379,7 +6313,7 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> -	u32 gtfifodbg, val, rc6_mode = 0;
> +	u32 gtfifodbg, val;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> @@ -6431,12 +6365,8 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
>  				      VLV_MEDIA_RC6_COUNT_EN |
>  				      VLV_RENDER_RC6_COUNT_EN));
>  
> -	if (intel_enable_rc6() & INTEL_RC6_ENABLE)
> -		rc6_mode = GEN7_RC_CTL_TO_MODE | VLV_RC_CTL_CTX_RST_PARALLEL;
> -
> -	intel_print_rc6_info(dev_priv, rc6_mode);
> -
> -	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
> +	I915_WRITE(GEN6_RC_CONTROL,
> +		   GEN7_RC_CTL_TO_MODE | VLV_RC_CTL_CTX_RST_PARALLEL);
>  
>  	/* Setting Fixed Bias */
>  	val = VLV_OVERRIDE_EN |
> @@ -6936,7 +6866,7 @@ void intel_init_gt_powersave(struct drm_i915_private *dev_priv)
>  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
>  	 * requirement.
>  	 */
> -	if (!i915.enable_rc6) {
> +	if (!sanitize_rc6(dev_priv)) {
>  		DRM_INFO("RC6 disabled, disabling runtime PM support\n");
>  		intel_runtime_pm_get(dev_priv);
>  	}
> @@ -6993,7 +6923,7 @@ void intel_cleanup_gt_powersave(struct drm_i915_private *dev_priv)
>  	if (IS_VALLEYVIEW(dev_priv))
>  		valleyview_cleanup_gt_powersave(dev_priv);
>  
> -	if (!i915.enable_rc6)
> +	if (!HAS_RC6(dev_priv))
>  		intel_runtime_pm_put(dev_priv);
>  }
>  
> @@ -8433,7 +8363,7 @@ u64 intel_rc6_residency_us(struct drm_i915_private *dev_priv,
>  {
>  	u64 time_hw, units, div;
>  
> -	if (!intel_enable_rc6())
> +	if (!HAS_RC6(dev_priv))
>  		return 0;
>  
>  	intel_runtime_pm_get(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index e01622757b84..9d08f7fd5497 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -296,7 +296,7 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
>  
>  	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
>  	/* WaRsDisableCoarsePowerGating:skl,bxt */
> -	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
> +	if (NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
>  		action[1] = 0;
>  	else
>  		/* bit 0 and 1 are for Render and Media domain separately */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b036a2d5c2d4..06483227b594 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -432,8 +432,6 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uncore_sanitize(struct drm_i915_private *dev_priv)
>  {
> -	i915.enable_rc6 = sanitize_rc6_option(dev_priv, i915.enable_rc6);
> -
>  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
>  	intel_sanitize_gt_powersave(dev_priv);
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux