[Intel-gfx] [PATCH] drm/i915: Remove unsafe i915.enable_rc6
Imre Deak
imre.deak at intel.com
Thu Oct 12 09:42:53 UTC 2017
On Wed, Oct 11, 2017 at 10:12:05AM +0100, 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, as they often set it during triage
> and end up disabling the entire runtime pm instead (the option is not a
> fine scalpel!).
>
> Furthermore, it allows users to set known dangerous values which were
> intended for testing and not for production use. For testing, we can
> always patch in the required setting without having to expose ourselves
> to random abuse.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>
> Floating again to see if the consensus is in favour of removing this
> modparam...
> -Chris
>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_params.c | 7 --
> drivers/gpu/drm/i915/i915_params.h | 1 -
> drivers/gpu/drm/i915/i915_pci.c | 2 +
> drivers/gpu/drm/i915/i915_sysfs.c | 13 +++-
> drivers/gpu/drm/i915/intel_drv.h | 5 --
> drivers/gpu/drm/i915/intel_guc.c | 3 +-
> drivers/gpu/drm/i915/intel_pm.c | 129 +++++++++---------------------------
> drivers/gpu/drm/i915/intel_uncore.c | 3 -
> 10 files changed, 47 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f1e651703764..732d15d65a5a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2502,7 +2502,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->gt_pm.rc6.enabled && intel_rc6_enabled())))
> + if (WARN_ON_ONCE(!(dev_priv->gt_pm.rc6.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 6bbc4b83aa0a..57c2903040cf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3181,6 +3181,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 b4faeb6aa2bd..6da48a77d70c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -50,13 +50,6 @@ i915_param_named_unsafe(semaphores, int, 0400,
> "Use semaphores for inter-ring sync "
> "(default: -1 (use per-chip defaults))");
>
> -i915_param_named_unsafe(enable_rc6, int, 0400,
> - "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)");
> -
> i915_param_named_unsafe(enable_dc, int, 0400,
> "Enable power-saving display C-states. "
> "(-1=auto [default]; 0=disable; 1=up to DC5; 2=up to DC6)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index c7292268ed43..374d3a7cb687 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -35,7 +35,6 @@
> param(int, lvds_channel_mode, 0) \
> param(int, panel_use_ssc, -1) \
> param(int, vbt_sdvo_panel_type, -1) \
> - param(int, enable_rc6, -1) \
> param(int, enable_dc, -1) \
> param(int, enable_fbc, -1) \
> param(int, enable_ppgtt, -1) \
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index bf467f30c99b..1644ab6efe8c 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -215,6 +215,8 @@ static const struct intel_device_info intel_gm45_info __initconst = {
> GEN_DEFAULT_PAGE_SIZES, \
> CURSOR_OFFSETS
>
> +/* Ironlake does support rc6, but we do not implement [power] contexts */
> +
> static const struct intel_device_info intel_ironlake_d_info __initconst = {
> GEN5_FEATURES,
> .platform = INTEL_IRONLAKE,
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 791759f632e1..1c95c2167d10 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -49,7 +49,18 @@ static u32 calc_residency(struct drm_i915_private *dev_priv,
> static ssize_t
> show_rc6_mask(struct device *kdev, struct device_attribute *attr, char *buf)
> {
> - return snprintf(buf, PAGE_SIZE, "%x\n", intel_rc6_enabled());
> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> + unsigned int mask;
> +
> + mask = 0;
> + if (HAS_RC6(dev_priv))
> + mask |= BIT(0);
> + if (HAS_RC6p(dev_priv))
> + mask |= BIT(1);
> + if (HAS_RC6pp(dev_priv))
> + mask |= BIT(2);
> +
> + return snprintf(buf, PAGE_SIZE, "%x\n", mask);
> }
>
> static ssize_t
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cdda0a84babe..7b358d7371f8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1898,15 +1898,10 @@ bool skl_ddb_allocation_overlaps(struct drm_i915_private *dev_priv,
> 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);
> int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
> struct intel_crtc_state *cstate);
> void intel_init_ipc(struct drm_i915_private *dev_priv);
> void intel_enable_ipc(struct drm_i915_private *dev_priv);
> -static inline int intel_rc6_enabled(void)
> -{
> - return i915_modparams.enable_rc6;
> -}
>
> /* intel_sdvo.c */
> bool intel_sdvo_init(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 9e18c4fb9909..c52c71b12762 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -137,8 +137,7 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
>
> action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
> /* WaRsDisableCoarsePowerGating:skl,bxt */
> - if (!intel_rc6_enabled() ||
> - NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
> + if (!HAS_RC6(dev_priv) || 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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2fcff9788b6f..399c7aa90b77 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6382,26 +6382,6 @@ static void valleyview_disable_rps(struct drm_i915_private *dev_priv)
> I915_WRITE(GEN6_RP_CONTROL, 0);
> }
>
> -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;
> @@ -6464,42 +6444,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)
> @@ -6591,7 +6545,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);
> @@ -6625,22 +6578,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_rc6_enabled() & 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);
The original did the opposite.
>
> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> }
> @@ -6649,7 +6599,6 @@ static void gen8_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);
> @@ -6671,13 +6620,11 @@ static void gen8_enable_rc6(struct drm_i915_private *dev_priv)
> I915_WRITE(GEN6_RC6_THRESHOLD, 625); /* 800us/1.28 for TO */
>
> /* 3: Enable RC6 */
> - if (intel_rc6_enabled() & INTEL_RC6_ENABLE)
> - rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
> - intel_print_rc6_info(dev_priv, rc6_mask);
>
> - I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> - GEN7_RC_CTL_TO_MODE |
> - rc6_mask);
> + I915_WRITE(GEN6_RC_CONTROL,
> + GEN6_RC_CTL_HW_ENABLE |
> + GEN7_RC_CTL_TO_MODE |
> + GEN6_RC_CTL_RC6_ENABLE);
>
> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> }
> @@ -6726,9 +6673,8 @@ static void gen6_enable_rc6(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;
>
> I915_WRITE(GEN6_RC_STATE, 0);
> @@ -6763,22 +6709,12 @@ static void gen6_enable_rc6(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_rc6_enabled();
> - 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) |
> @@ -7221,7 +7157,7 @@ static void cherryview_enable_rc6(struct drm_i915_private *dev_priv)
> {
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> - u32 gtfifodbg, rc6_mode = 0, pcbr;
> + u32 gtfifodbg, rc6_mode, pcbr;
>
> gtfifodbg = I915_READ(GTFIFODBG) & ~(GT_FIFO_SBDEDICATE_FREE_ENTRY_CHV |
> GT_FIFO_FREE_ENTRIES_CHV);
> @@ -7262,10 +7198,9 @@ static void cherryview_enable_rc6(struct drm_i915_private *dev_priv)
> pcbr = I915_READ(VLV_PCBR);
>
> /* 3: Enable RC6 */
> - if ((intel_rc6_enabled() & 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);
>
> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> @@ -7317,7 +7252,7 @@ static void valleyview_enable_rc6(struct drm_i915_private *dev_priv)
> {
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> - u32 gtfifodbg, rc6_mode = 0;
> + u32 gtfifodbg;
>
> valleyview_check_pctx(dev_priv);
>
> @@ -7350,12 +7285,8 @@ static void valleyview_enable_rc6(struct drm_i915_private *dev_priv)
> VLV_MEDIA_RC6_COUNT_EN |
> VLV_RENDER_RC6_COUNT_EN));
>
> - if (intel_rc6_enabled() & 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);
>
> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> }
> @@ -7882,7 +7813,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_modparams.enable_rc6) {
> + if (!sanitize_rc6(dev_priv)) {
> DRM_INFO("RC6 disabled, disabling runtime PM support\n");
> intel_runtime_pm_get(dev_priv);
> }
> @@ -7939,7 +7870,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_modparams.enable_rc6)
> + if (!HAS_RC6(dev_priv))
> intel_runtime_pm_put(dev_priv);
> }
>
> @@ -9487,7 +9418,7 @@ u64 intel_rc6_residency_us(struct drm_i915_private *dev_priv,
> {
> u64 time_hw, units, div;
>
> - if (!intel_rc6_enabled())
> + if (!HAS_RC6(dev_priv))
> return 0;
>
> intel_runtime_pm_get(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 983617b5b338..82b87c03df37 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -436,9 +436,6 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv)
>
> void intel_uncore_sanitize(struct drm_i915_private *dev_priv)
> {
> - i915_modparams.enable_rc6 =
> - sanitize_rc6_option(dev_priv, i915_modparams.enable_rc6);
> -
> /* BIOS often leaves RC6 enabled, but disable it for hw init */
> intel_sanitize_gt_powersave(dev_priv);
> }
> --
> 2.15.0.rc0
>
More information about the Intel-gfx
mailing list