| Message ID | 20170325142344.15869-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com I guess _unsafe is not strong enough, and seems reasonable. Reviewed-by: Daniel Vetter <daniel.vetter@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 25/03/17 07:23, 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. This type of option can be useful for early debug and testing. Maybe it can be moved to a Kconfig option. > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com > --- > 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); > } >
On Thu, Mar 30, 2017 at 09:06:17AM -0700, Kelvin Gardiner wrote: > > > On 25/03/17 07:23, 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. > > This type of option can be useful for early debug and testing. Maybe > it can be moved to a Kconfig option. That's a reasonable idea to allow overriding any and all (ideally) of intel_device_info. But usually a test branch will enable a feature it is using (as the first patch when testing, and then as the last patch when upstreaming). -Chris
On Thu, Mar 30, 2017 at 05:10:55PM +0100, Chris Wilson wrote: > On Thu, Mar 30, 2017 at 09:06:17AM -0700, Kelvin Gardiner wrote: > > > > > > On 25/03/17 07:23, 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. > > > > This type of option can be useful for early debug and testing. Maybe > > it can be moved to a Kconfig option. > > That's a reasonable idea to allow overriding any and all (ideally) of > intel_device_info. But usually a test branch will enable a feature it is > using (as the first patch when testing, and then as the last patch when > upstreaming). Hmm, come to think of it, I used to have a poc to override this via a user provided DT, which makes some testing easier, but still has the same issue in allowing user override just obfuscated. Only a matter of time before Gentoo forums start carrying complete DT overrides. -Chris
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); }
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@chris-wilson.co.uk> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Imre Deak <imre.deak@intel.com --- 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(-)