* [PATCH] drm/i915: Remove unsafe i915.enable_rc6
@ 2017-03-25 14:23 Chris Wilson
2017-03-27 6:58 ` Daniel Vetter
2017-03-30 16:06 ` Kelvin Gardiner
0 siblings, 2 replies; 9+ messages in thread
From: Chris Wilson @ 2017-03-25 14:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula
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(-)
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
2017-03-25 14:23 [PATCH] drm/i915: Remove unsafe i915.enable_rc6 Chris Wilson
@ 2017-03-27 6:58 ` Daniel Vetter
2017-03-30 16:06 ` Kelvin Gardiner
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-03-27 6:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, intel-gfx
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
2017-03-25 14:23 [PATCH] drm/i915: Remove unsafe i915.enable_rc6 Chris Wilson
2017-03-27 6:58 ` Daniel Vetter
@ 2017-03-30 16:06 ` Kelvin Gardiner
2017-03-30 16:10 ` Chris Wilson
1 sibling, 1 reply; 9+ messages in thread
From: Kelvin Gardiner @ 2017-03-30 16:06 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Jani Nikula
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);
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
2017-03-30 16:06 ` Kelvin Gardiner
@ 2017-03-30 16:10 ` Chris Wilson
2017-03-30 16:35 ` Chris Wilson
0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2017-03-30 16:10 UTC (permalink / raw)
To: Kelvin Gardiner; +Cc: Jani Nikula, intel-gfx
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
2017-03-30 16:10 ` Chris Wilson
@ 2017-03-30 16:35 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2017-03-30 16:35 UTC (permalink / raw)
To: Kelvin Gardiner, intel-gfx, Jani Nikula
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
2017-10-11 9:12 Chris Wilson
2017-10-11 11:35 ` Daniel Vetter
2017-10-12 9:37 ` Joonas Lahtinen
@ 2017-10-12 9:42 ` Imre Deak
2 siblings, 0 replies; 9+ messages in thread
From: Imre Deak @ 2017-10-12 9:42 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, Rodrigo Vivi
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@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Acked-by: Daniel Vetter <daniel.vetter@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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
2017-10-11 9:12 Chris Wilson
2017-10-11 11:35 ` Daniel Vetter
@ 2017-10-12 9:37 ` Joonas Lahtinen
2017-10-12 9:42 ` Imre Deak
2 siblings, 0 replies; 9+ messages in thread
From: Joonas Lahtinen @ 2017-10-12 9:37 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Jani Nikula, Daniel Vetter, Rodrigo Vivi
On Wed, 2017-10-11 at 10:12 +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@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
<SNIP>
> +++ 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 */
This would feel more home above an explicit .has_rc6 = 0
Maybe;
/* Ironlake ... */
#define ILK_D_PLATFORM \
GEN5_FEATURES, \
.platform = INTEL_IRONLAKE, \
.has_rc6 = 0
Or just generalize to GEN5_FEATURES and lift the comment on top of it.
> +
> static const struct intel_device_info intel_ironlake_d_info __initconst = {
> GEN5_FEATURES,
> .platform = INTEL_IRONLAKE,
<SNIP>
> @@ -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 */
Comment feels bit alone now, just drop it.
> - 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) |
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
2017-10-11 9:12 Chris Wilson
@ 2017-10-11 11:35 ` Daniel Vetter
2017-10-12 9:37 ` Joonas Lahtinen
2017-10-12 9:42 ` Imre Deak
2 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-10-11 11:35 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, Rodrigo Vivi
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@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Still acked.
-Daniel
> ---
>
> 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);
>
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/i915: Remove unsafe i915.enable_rc6
@ 2017-10-11 9:12 Chris Wilson
2017-10-11 11:35 ` Daniel Vetter
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Chris Wilson @ 2017-10-11 9:12 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter, Rodrigo Vivi
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@chris-wilson.co.uk>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Acked-by: Daniel Vetter <daniel.vetter@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);
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-12 9:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-25 14:23 [PATCH] drm/i915: Remove unsafe i915.enable_rc6 Chris Wilson
2017-03-27 6:58 ` Daniel Vetter
2017-03-30 16:06 ` Kelvin Gardiner
2017-03-30 16:10 ` Chris Wilson
2017-03-30 16:35 ` Chris Wilson
2017-10-11 9:12 Chris Wilson
2017-10-11 11:35 ` Daniel Vetter
2017-10-12 9:37 ` Joonas Lahtinen
2017-10-12 9:42 ` Imre Deak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.