[Intel-gfx] [PATCH] drm/i915: Remove unsafe i915.enable_rc6
Daniel Vetter
daniel at ffwll.ch
Mon Mar 27 06:58:08 UTC 2017
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 at chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Imre Deak <imre.deak at intel.com
I guess _unsafe is not strong enough, and seems reasonable.
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list