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

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

[Chris Wilson]

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

✓ Fi.CI.BAT: success for drm/i915: Remove unsafe i915.enable_rc6 (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Remove unsafe i915.enable_rc6 (rev2)
URL   : https://patchwork.freedesktop.org/series/21884/
State : success

== Summary ==

Series 21884v2 drm/i915: Remove unsafe i915.enable_rc6
https://patchwork.freedesktop.org/api/1.0/series/21884/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                dmesg-fail -> PASS       (fi-cfl-s) fdo#102996
        Subgroup nonblocking-crc-pipe-b:
                incomplete -> PASS       (fi-cfl-s) fdo#103022

fdo#102996 https://bugs.freedesktop.org/show_bug.cgi?id=102996
fdo#103022 https://bugs.freedesktop.org/show_bug.cgi?id=103022

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:458s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:474s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:389s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:569s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:283s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:531s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:516s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:538s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:516s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:558s
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:616s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:429s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:598s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:436s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:457s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:470s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:501s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:582s
fi-kbl-7567u     total:289  pass:265  dwarn:4   dfail:0   fail:0   skip:20  time:483s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:589s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:658s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:659s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:531s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:513s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:472s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:577s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:433s

2dcc40d169e600cfa63887260901c74f943e9bf5 drm-tip: 2017y-10m-11d-08h-54m-27s UTC integration manifest
630263d9d2b3 drm/i915: Remove unsafe i915.enable_rc6

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5988/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

✓ Fi.CI.IGT: success for drm/i915: Remove unsafe i915.enable_rc6 (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Remove unsafe i915.enable_rc6 (rev2)
URL   : https://patchwork.freedesktop.org/series/21884/
State : success

== Summary ==

shard-hsw        total:2501 pass:1411 dwarn:6   dfail:0   fail:7   skip:1077 time:9322s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5988/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

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

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

[Imre Deak]

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

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

[Chris Wilson]

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.

v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
lack of ilk support better.
v3: Clear intel_info->rc6p if we don't support rc6 itself.

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>
---
 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     |   3 +
 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     | 134 +++++++++++-------------------------
 drivers/gpu/drm/i915/intel_uncore.c |   3 -
 10 files changed, 57 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3db5851756f0..ca3d5f39338e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2503,7 +2503,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 366ba74b0ad2..75100c6e35c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3193,6 +3193,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 6458c309c039..f1c6756440a9 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -216,6 +216,9 @@ static const struct intel_device_info intel_gm45_info __initconst = {
 static const struct intel_device_info intel_ironlake_d_info __initconst = {
 	GEN5_FEATURES,
 	.platform = INTEL_IRONLAKE,
+	/* ilk does support rc6, but we do not implement [power] contexts */
+	.has_rc6 = 0,
+
 };
 
 static const struct intel_device_info intel_ironlake_m_info __initconst = {
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 3b4eafd39f55..5dc759eecd3c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1908,15 +1908,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 10037c0fdf95..00a86a44e8fb 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -230,8 +230,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 742d5455b201..3e45df64eec4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6383,26 +6383,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;
@@ -6465,42 +6445,26 @@ 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;
+		info->has_rc6 = 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;
-	}
-
-	if (IS_IVYBRIDGE(dev_priv))
-		return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
+	/*
+	 * We assume that we do not have any deep rc6 levels if we don't have
+	 * have the previous rc6 level supporteditself, i.e. we use HAS_RC6()
+	 * as the initial coarse check for rc6 in general, moving on to
+	 * progressively finer/deeper levels.
+	 */
+	if (WARN(!info->has_rc6 && info->has_rc6p,
+		 "deep rc6p enabled without rc6; disabling!\n"))
+		info->has_rc6p = 0;
 
-	return INTEL_RC6_ENABLE;
+	return info->has_rc6;
 }
 
 static void gen6_init_rps_frequencies(struct drm_i915_private *dev_priv)
@@ -6592,7 +6556,7 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
-	u32 rc6_mode, rc6_mask = 0;
+	u32 rc6_mode;
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
@@ -6633,9 +6597,6 @@ 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 */
 
 	/* WaRsUseTimeoutMode:cnl (pre-prod) */
@@ -6645,7 +6606,9 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
 		rc6_mode = GEN6_RC_CTL_EI_MODE(1);
 
 	I915_WRITE(GEN6_RC_CONTROL,
-		   GEN6_RC_CTL_HW_ENABLE | rc6_mode | rc6_mask);
+		   GEN6_RC_CTL_HW_ENABLE |
+		   GEN6_RC_CTL_RC6_ENABLE |
+		   rc6_mode);
 
 	/*
 	 * 3b: Enable Coarse Power Gating only when RC6 is enabled.
@@ -6654,8 +6617,8 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
 	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,
+			   GEN9_RENDER_PG_ENABLE | GEN9_MEDIA_PG_ENABLE);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -6664,7 +6627,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);
@@ -6686,13 +6648,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);
 }
@@ -6741,9 +6701,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);
@@ -6778,22 +6737,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) |
@@ -7236,7 +7185,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);
@@ -7277,10 +7226,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);
@@ -7332,7 +7280,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);
 
@@ -7365,12 +7313,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);
 }
@@ -7897,7 +7841,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);
 	}
@@ -7954,7 +7898,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);
 }
 
@@ -9505,7 +9449,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 20e3c65c0999..51797d40b4fd 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.rc2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Remove unsafe i915.enable_rc6 (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Remove unsafe i915.enable_rc6 (rev3)
URL   : https://patchwork.freedesktop.org/series/21884/
State : failure

== Summary ==

Series 21884v3 drm/i915: Remove unsafe i915.enable_rc6
https://patchwork.freedesktop.org/api/1.0/series/21884/revisions/3/mbox/

Test gem_exec_reloc:
        Subgroup basic-gtt-active:
                pass       -> FAIL       (fi-gdg-551) fdo#102582 +2
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-cnl-y)

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:447s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:445s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:374s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:531s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:264s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:499s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:506s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:497s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:483s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:565s
fi-cnl-y         total:284  pass:221  dwarn:0   dfail:0   fail:0   skip:24 
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:415s
fi-gdg-551       total:289  pass:175  dwarn:1   dfail:0   fail:4   skip:109 time:249s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:580s
fi-glk-dsi       total:289  pass:258  dwarn:0   dfail:0   fail:1   skip:30  time:488s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:440s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:494s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:574s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:582s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:544s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:593s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:648s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:507s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:459s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:571s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:417s

2ea0b3d47030274c97624258e09fc7d1ffd0e0f2 drm-tip: 2017y-10m-25d-18h-42m-20s UTC integration manifest
e1feeba5a496 drm/i915: Remove unsafe i915.enable_rc6

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6200/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

On Thu, 2017-10-26 at 11:32 +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.
> 
> v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> lack of ilk support better.
> v3: Clear intel_info->rc6p if we don't support rc6 itself.
> 
> 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_drv.h
> @@ -3193,6 +3193,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)

Slap a comment at the end.

> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -216,6 +216,9 @@ static const struct intel_device_info intel_gm45_info __initconst = {
>  static const struct intel_device_info intel_ironlake_d_info __initconst = {
>  	GEN5_FEATURES,
>  	.platform = INTEL_IRONLAKE,
> +	/* ilk does support rc6, but we do not implement [power] contexts */
> +	.has_rc6 = 0,
> +

Extra newline.

<SNIP>

> -	if (IS_IVYBRIDGE(dev_priv))
> -		return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
> +	/*
> +	 * We assume that we do not have any deep rc6 levels if we don't have
> +	 * have the previous rc6 level supporteditself, i.e. we use HAS_RC6()

"supported itself"

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

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

[Daniele Ceraolo Spurio]

On 26/10/17 03:32, 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.
> 
> v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> lack of ilk support better.
> v3: Clear intel_info->rc6p if we don't support rc6 itself.
> 
> 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>
> ---

I think that for execution/debug on early silicon we might still want 
the ability to turn features like RC6 off. Maybe we can add a debug 
kconfig to force info->has_rc6 = 0? Not a blocker to this patch but 
worth considering IMO.

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Weinehall]

On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 26/10/17 03:32, 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.
> > 
> > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> > lack of ilk support better.
> > v3: Clear intel_info->rc6p if we don't support rc6 itself.
> > 
> > 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>
> > ---
> 
> I think that for execution/debug on early silicon we might still want the
> ability to turn features like RC6 off. Maybe we can add a debug kconfig to
> force info->has_rc6 = 0? Not a blocker to this patch but worth considering
> IMO.

Most of the BIOSes I've seen on our RVPs have had an option to disable
RC6.


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
> On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
> >
> >
> > On 26/10/17 03:32, 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.
> > >
> > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> > > lack of ilk support better.
> > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
> > >
> > > 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>
> > > ---
> >
> > I think that for execution/debug on early silicon we might still want the
> > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
> > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
> > IMO.
>
> Most of the BIOSes I've seen on our RVPs have had an option to disable
> RC6.

BIOS option don't block our code to run and set some MMIOs.
Not sure how the GPU will behave on such cases.

I like the idea of removing some and keeping the parameters clean.
But there are few ones like RC6 and disable_power_wells that are very
useful on platform enabling and also when assisting others to debug issues.

For instance right now that we fixed RC6 on CNL someone told that
he believe seeing more hangs, so I immediately asked to boot with
i915.enable_rc6=0 to double check. It is easier and straighforward
to direct them to the unsafe param than to ask them to compile the code
with different options or to use some BIOS options that we are not sure.

Also on bug triage some options like this are helpful.

Also BIOS and compile are saved flags. So if you need to do a quick test
you have to save it, and then unsave later. Parameters are very convinient
for 1 boot only check.

Maybe we could clean and remove the different options and levels and
let it be just a boolean.

Thanks,
Rodrigo.

>
>
> Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
> On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
> > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
> > > 
> > > 
> > > On 26/10/17 03:32, 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.
> > > > 
> > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> > > > lack of ilk support better.
> > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
> > > > 
> > > > 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>
> > > > ---
> > > 
> > > I think that for execution/debug on early silicon we might still want the
> > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
> > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
> > > IMO.
> > 
> > Most of the BIOSes I've seen on our RVPs have had an option to disable
> > RC6.
> 
> BIOS option don't block our code to run and set some MMIOs.
> Not sure how the GPU will behave on such cases.
> 
> I like the idea of removing some and keeping the parameters clean.
> But there are few ones like RC6 and disable_power_wells that are very
> useful on platform enabling and also when assisting others to debug issues.
> 
> For instance right now that we fixed RC6 on CNL someone told that
> he believe seeing more hangs, so I immediately asked to boot with
> i915.enable_rc6=0 to double check. It is easier and straighforward
> to direct them to the unsafe param than to ask them to compile the code
> with different options or to use some BIOS options that we are not sure.
> 
> Also on bug triage some options like this are helpful.
> 
> Also BIOS and compile are saved flags. So if you need to do a quick test
> you have to save it, and then unsave later. Parameters are very convinient
> for 1 boot only check.

It's convenient for sure, but the unsafe module parameters seems to be
finding their way into way too many HOWTOs, and from there to some
"productized" use-cases. Chris states that setting .enable_rc6=0 to
solving an issue on publicly shipping products has been some years ago,
so I don't see a need for carrying this.

We shouldn't allow the convenience of not having to change one line and
recompile kernel during development to affect the end-users who are
Googling how to get the best performance out of their hardware (I could
mention some distro here).

This seems the like the best option as I don't think introducing kernel
parameters that only exists on debug builds would be too convenient
either. It'd maybe just add more confusion.

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

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

[Ben Widawsky]

On 17-11-01 14:07:28, Joonas Lahtinen wrote:
>On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
>> On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
>> > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
>> > >
>> > >
>> > > On 26/10/17 03:32, 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.
>> > > >
>> > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
>> > > > lack of ilk support better.
>> > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
>> > > >
>> > > > 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>
>> > > > ---
>> > >
>> > > I think that for execution/debug on early silicon we might still want the
>> > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
>> > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
>> > > IMO.
>> >
>> > Most of the BIOSes I've seen on our RVPs have had an option to disable
>> > RC6.
>>
>> BIOS option don't block our code to run and set some MMIOs.
>> Not sure how the GPU will behave on such cases.
>>
>> I like the idea of removing some and keeping the parameters clean.
>> But there are few ones like RC6 and disable_power_wells that are very
>> useful on platform enabling and also when assisting others to debug issues.
>>
>> For instance right now that we fixed RC6 on CNL someone told that
>> he believe seeing more hangs, so I immediately asked to boot with
>> i915.enable_rc6=0 to double check. It is easier and straighforward
>> to direct them to the unsafe param than to ask them to compile the code
>> with different options or to use some BIOS options that we are not sure.
>>
>> Also on bug triage some options like this are helpful.
>>
>> Also BIOS and compile are saved flags. So if you need to do a quick test
>> you have to save it, and then unsave later. Parameters are very convinient
>> for 1 boot only check.
>
>It's convenient for sure, but the unsafe module parameters seems to be
>finding their way into way too many HOWTOs, and from there to some
>"productized" use-cases. Chris states that setting .enable_rc6=0 to
>solving an issue on publicly shipping products has been some years ago,
>so I don't see a need for carrying this.
>
>We shouldn't allow the convenience of not having to change one line and
>recompile kernel during development to affect the end-users who are
>Googling how to get the best performance out of their hardware (I could
>mention some distro here).
>
>This seems the like the best option as I don't think introducing kernel
>parameters that only exists on debug builds would be too convenient
>either. It'd maybe just add more confusion.
>
>Regards, Joonas

I believe the ability to disable RC6 is valuable not just for debugging
purposes. Folks with very latency sensitive workloads are often willing to
forego power savings. The real problem I see is that we don't test without rc6
in our setup, which indeed makes it unsafe. As such, I see the other option here
going back to the ability to toggle rc6 after load (either module parameter, or
make it sysfs), and actually run some subset of our workloads with RC6. I
suspect people will poop on that suggestion, but I figured I'd mention.



-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

+ Kimmo and Paul

On Wed, 2017-11-01 at 07:43 -0700, Ben Widawsky wrote:
> On 17-11-01 14:07:28, Joonas Lahtinen wrote:
> > On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
> > > On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
> > > > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
> > > > > 
> > > > > 
> > > > > On 26/10/17 03:32, 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.
> > > > > > 
> > > > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> > > > > > lack of ilk support better.
> > > > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
> > > > > > 
> > > > > > 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>
> > > > > > ---
> > > > > 
> > > > > I think that for execution/debug on early silicon we might still want the
> > > > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
> > > > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
> > > > > IMO.
> > > > 
> > > > Most of the BIOSes I've seen on our RVPs have had an option to disable
> > > > RC6.
> > > 
> > > BIOS option don't block our code to run and set some MMIOs.
> > > Not sure how the GPU will behave on such cases.
> > > 
> > > I like the idea of removing some and keeping the parameters clean.
> > > But there are few ones like RC6 and disable_power_wells that are very
> > > useful on platform enabling and also when assisting others to debug issues.
> > > 
> > > For instance right now that we fixed RC6 on CNL someone told that
> > > he believe seeing more hangs, so I immediately asked to boot with
> > > i915.enable_rc6=0 to double check. It is easier and straighforward
> > > to direct them to the unsafe param than to ask them to compile the code
> > > with different options or to use some BIOS options that we are not sure.
> > > 
> > > Also on bug triage some options like this are helpful.
> > > 
> > > Also BIOS and compile are saved flags. So if you need to do a quick test
> > > you have to save it, and then unsave later. Parameters are very convinient
> > > for 1 boot only check.
> > 
> > It's convenient for sure, but the unsafe module parameters seems to be
> > finding their way into way too many HOWTOs, and from there to some
> > "productized" use-cases. Chris states that setting .enable_rc6=0 to
> > solving an issue on publicly shipping products has been some years ago,
> > so I don't see a need for carrying this.
> > 
> > We shouldn't allow the convenience of not having to change one line and
> > recompile kernel during development to affect the end-users who are
> > Googling how to get the best performance out of their hardware (I could
> > mention some distro here).
> > 
> > This seems the like the best option as I don't think introducing kernel
> > parameters that only exists on debug builds would be too convenient
> > either. It'd maybe just add more confusion.
> > 
> > Regards, Joonas
> 
> I believe the ability to disable RC6 is valuable not just for debugging
> purposes. Folks with very latency sensitive workloads are often willing to
> forego power savings. The real problem I see is that we don't test without rc6
> in our setup, which indeed makes it unsafe. As such, I see the other option here
> going back to the ability to toggle rc6 after load (either module parameter, or
> make it sysfs), and actually run some subset of our workloads with RC6. I
> suspect people will poop on that suggestion, but I figured I'd mention.

I agree there, but by my understanding there's really no ask to support
the feature in upstream. And the original motive from Chris to drop the
feature is that it's unsafe as it currently is.

So unless we've got the resources to bring it back from the unsafe
zone, I think we should drop it like this patch proposes.

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

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

[Ben Widawsky]

On 17-11-01 18:09:47, Joonas Lahtinen wrote:
>+ Kimmo and Paul
>
>On Wed, 2017-11-01 at 07:43 -0700, Ben Widawsky wrote:
>> On 17-11-01 14:07:28, Joonas Lahtinen wrote:
>> > On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
>> > > On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
>> > > > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
>> > > > >
>> > > > >
>> > > > > On 26/10/17 03:32, 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.
>> > > > > >
>> > > > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
>> > > > > > lack of ilk support better.
>> > > > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
>> > > > > >
>> > > > > > 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>
>> > > > > > ---
>> > > > >
>> > > > > I think that for execution/debug on early silicon we might still want the
>> > > > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
>> > > > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
>> > > > > IMO.
>> > > >
>> > > > Most of the BIOSes I've seen on our RVPs have had an option to disable
>> > > > RC6.
>> > >
>> > > BIOS option don't block our code to run and set some MMIOs.
>> > > Not sure how the GPU will behave on such cases.
>> > >
>> > > I like the idea of removing some and keeping the parameters clean.
>> > > But there are few ones like RC6 and disable_power_wells that are very
>> > > useful on platform enabling and also when assisting others to debug issues.
>> > >
>> > > For instance right now that we fixed RC6 on CNL someone told that
>> > > he believe seeing more hangs, so I immediately asked to boot with
>> > > i915.enable_rc6=0 to double check. It is easier and straighforward
>> > > to direct them to the unsafe param than to ask them to compile the code
>> > > with different options or to use some BIOS options that we are not sure.
>> > >
>> > > Also on bug triage some options like this are helpful.
>> > >
>> > > Also BIOS and compile are saved flags. So if you need to do a quick test
>> > > you have to save it, and then unsave later. Parameters are very convinient
>> > > for 1 boot only check.
>> >
>> > It's convenient for sure, but the unsafe module parameters seems to be
>> > finding their way into way too many HOWTOs, and from there to some
>> > "productized" use-cases. Chris states that setting .enable_rc6=0 to
>> > solving an issue on publicly shipping products has been some years ago,
>> > so I don't see a need for carrying this.
>> >
>> > We shouldn't allow the convenience of not having to change one line and
>> > recompile kernel during development to affect the end-users who are
>> > Googling how to get the best performance out of their hardware (I could
>> > mention some distro here).
>> >
>> > This seems the like the best option as I don't think introducing kernel
>> > parameters that only exists on debug builds would be too convenient
>> > either. It'd maybe just add more confusion.
>> >
>> > Regards, Joonas
>>
>> I believe the ability to disable RC6 is valuable not just for debugging
>> purposes. Folks with very latency sensitive workloads are often willing to
>> forego power savings. The real problem I see is that we don't test without rc6
>> in our setup, which indeed makes it unsafe. As such, I see the other option here
>> going back to the ability to toggle rc6 after load (either module parameter, or
>> make it sysfs), and actually run some subset of our workloads with RC6. I
>> suspect people will poop on that suggestion, but I figured I'd mention.
>
>I agree there, but by my understanding there's really no ask to support
>the feature in upstream. And the original motive from Chris to drop the
>feature is that it's unsafe as it currently is.
>
>So unless we've got the resources to bring it back from the unsafe
>zone, I think we should drop it like this patch proposes.
>
>Regards, Joonas

Yep, I agree. One other option would be to move i915_forcewake_user to sysfs and
let them use that.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Wed, Nov 01, 2017 at 04:21:08PM +0000, Ben Widawsky wrote:
> On 17-11-01 18:09:47, Joonas Lahtinen wrote:
> > + Kimmo and Paul
> > 
> > On Wed, 2017-11-01 at 07:43 -0700, Ben Widawsky wrote:
> > > On 17-11-01 14:07:28, Joonas Lahtinen wrote:
> > > > On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
> > > > > On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
> > > > > > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 26/10/17 03:32, 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.
> > > > > > > >
> > > > > > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> > > > > > > > lack of ilk support better.
> > > > > > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
> > > > > > > >
> > > > > > > > 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>
> > > > > > > > ---
> > > > > > >
> > > > > > > I think that for execution/debug on early silicon we might still want the
> > > > > > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
> > > > > > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
> > > > > > > IMO.
> > > > > >
> > > > > > Most of the BIOSes I've seen on our RVPs have had an option to disable
> > > > > > RC6.
> > > > >
> > > > > BIOS option don't block our code to run and set some MMIOs.
> > > > > Not sure how the GPU will behave on such cases.
> > > > >
> > > > > I like the idea of removing some and keeping the parameters clean.
> > > > > But there are few ones like RC6 and disable_power_wells that are very
> > > > > useful on platform enabling and also when assisting others to debug issues.
> > > > >
> > > > > For instance right now that we fixed RC6 on CNL someone told that
> > > > > he believe seeing more hangs, so I immediately asked to boot with
> > > > > i915.enable_rc6=0 to double check. It is easier and straighforward
> > > > > to direct them to the unsafe param than to ask them to compile the code
> > > > > with different options or to use some BIOS options that we are not sure.
> > > > >
> > > > > Also on bug triage some options like this are helpful.
> > > > >
> > > > > Also BIOS and compile are saved flags. So if you need to do a quick test
> > > > > you have to save it, and then unsave later. Parameters are very convinient
> > > > > for 1 boot only check.
> > > >
> > > > It's convenient for sure, but the unsafe module parameters seems to be
> > > > finding their way into way too many HOWTOs, and from there to some
> > > > "productized" use-cases. Chris states that setting .enable_rc6=0 to
> > > > solving an issue on publicly shipping products has been some years ago,
> > > > so I don't see a need for carrying this.
> > > >
> > > > We shouldn't allow the convenience of not having to change one line and
> > > > recompile kernel during development to affect the end-users who are
> > > > Googling how to get the best performance out of their hardware (I could
> > > > mention some distro here).
> > > >
> > > > This seems the like the best option as I don't think introducing kernel
> > > > parameters that only exists on debug builds would be too convenient
> > > > either. It'd maybe just add more confusion.
> > > >
> > > > Regards, Joonas
> > > 
> > > I believe the ability to disable RC6 is valuable not just for debugging
> > > purposes. Folks with very latency sensitive workloads are often willing to
> > > forego power savings. The real problem I see is that we don't test without rc6
> > > in our setup, which indeed makes it unsafe. As such, I see the other option here
> > > going back to the ability to toggle rc6 after load (either module parameter, or
> > > make it sysfs), and actually run some subset of our workloads with RC6. I
> > > suspect people will poop on that suggestion, but I figured I'd mention.
> > 
> > I agree there, but by my understanding there's really no ask to support
> > the feature in upstream. And the original motive from Chris to drop the
> > feature is that it's unsafe as it currently is.
> > 
> > So unless we've got the resources to bring it back from the unsafe
> > zone, I think we should drop it like this patch proposes.
> > 
> > Regards, Joonas
> 
> Yep, I agree. One other option would be to move i915_forcewake_user to sysfs and
> let them use that.

Well, I won't try to block that. I just put my 2 cents that I believe it is a very
useful parameter.

It wasn't that long ago the last time that we needed the flag to allow
end users to have a functional machine: https://plus.google.com/+JonMasters/posts/BqWLEjenLKv.

or to debug a related issue:
https://bugzilla.redhat.com/show_bug.cgi?id=1440988
https://bugzilla.kernel.org/show_bug.cgi?id=116431

Although date on few seems over than 1 year. We need to consider that
that was our latest new gpu... gen9.

If products are recommending the use of enable_rc6=0 I can see they
adding the patch to disable that. Effect is the same and our convenience is gone.

But again, just my view here. Not a nack ;)

Thanks,
Rodrigo.

> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Wed, 01 Nov 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Wed, Nov 01, 2017 at 04:21:08PM +0000, Ben Widawsky wrote:
>> On 17-11-01 18:09:47, Joonas Lahtinen wrote:
>> > + Kimmo and Paul
>> > 
>> > On Wed, 2017-11-01 at 07:43 -0700, Ben Widawsky wrote:
>> > > On 17-11-01 14:07:28, Joonas Lahtinen wrote:
>> > > > On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
>> > > > > On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
>> > > > > > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
>> > > > > > >
>> > > > > > >
>> > > > > > > On 26/10/17 03:32, 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.
>> > > > > > > >
>> > > > > > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
>> > > > > > > > lack of ilk support better.
>> > > > > > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
>> > > > > > > >
>> > > > > > > > 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>
>> > > > > > > > ---
>> > > > > > >
>> > > > > > > I think that for execution/debug on early silicon we might still want the
>> > > > > > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
>> > > > > > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
>> > > > > > > IMO.
>> > > > > >
>> > > > > > Most of the BIOSes I've seen on our RVPs have had an option to disable
>> > > > > > RC6.
>> > > > >
>> > > > > BIOS option don't block our code to run and set some MMIOs.
>> > > > > Not sure how the GPU will behave on such cases.
>> > > > >
>> > > > > I like the idea of removing some and keeping the parameters clean.
>> > > > > But there are few ones like RC6 and disable_power_wells that are very
>> > > > > useful on platform enabling and also when assisting others to debug issues.
>> > > > >
>> > > > > For instance right now that we fixed RC6 on CNL someone told that
>> > > > > he believe seeing more hangs, so I immediately asked to boot with
>> > > > > i915.enable_rc6=0 to double check. It is easier and straighforward
>> > > > > to direct them to the unsafe param than to ask them to compile the code
>> > > > > with different options or to use some BIOS options that we are not sure.
>> > > > >
>> > > > > Also on bug triage some options like this are helpful.
>> > > > >
>> > > > > Also BIOS and compile are saved flags. So if you need to do a quick test
>> > > > > you have to save it, and then unsave later. Parameters are very convinient
>> > > > > for 1 boot only check.
>> > > >
>> > > > It's convenient for sure, but the unsafe module parameters seems to be
>> > > > finding their way into way too many HOWTOs, and from there to some
>> > > > "productized" use-cases. Chris states that setting .enable_rc6=0 to
>> > > > solving an issue on publicly shipping products has been some years ago,
>> > > > so I don't see a need for carrying this.
>> > > >
>> > > > We shouldn't allow the convenience of not having to change one line and
>> > > > recompile kernel during development to affect the end-users who are
>> > > > Googling how to get the best performance out of their hardware (I could
>> > > > mention some distro here).
>> > > >
>> > > > This seems the like the best option as I don't think introducing kernel
>> > > > parameters that only exists on debug builds would be too convenient
>> > > > either. It'd maybe just add more confusion.
>> > > >
>> > > > Regards, Joonas
>> > > 
>> > > I believe the ability to disable RC6 is valuable not just for debugging
>> > > purposes. Folks with very latency sensitive workloads are often willing to
>> > > forego power savings. The real problem I see is that we don't test without rc6
>> > > in our setup, which indeed makes it unsafe. As such, I see the other option here
>> > > going back to the ability to toggle rc6 after load (either module parameter, or
>> > > make it sysfs), and actually run some subset of our workloads with RC6. I
>> > > suspect people will poop on that suggestion, but I figured I'd mention.
>> > 
>> > I agree there, but by my understanding there's really no ask to support
>> > the feature in upstream. And the original motive from Chris to drop the
>> > feature is that it's unsafe as it currently is.
>> > 
>> > So unless we've got the resources to bring it back from the unsafe
>> > zone, I think we should drop it like this patch proposes.
>> > 
>> > Regards, Joonas
>> 
>> Yep, I agree. One other option would be to move i915_forcewake_user to sysfs and
>> let them use that.
>
> Well, I won't try to block that. I just put my 2 cents that I believe it is a very
> useful parameter.
>
> It wasn't that long ago the last time that we needed the flag to allow
> end users to have a functional machine: https://plus.google.com/+JonMasters/posts/BqWLEjenLKv.
>
> or to debug a related issue:
> https://bugzilla.redhat.com/show_bug.cgi?id=1440988
> https://bugzilla.kernel.org/show_bug.cgi?id=116431
>
> Although date on few seems over than 1 year. We need to consider that
> that was our latest new gpu... gen9.
>
> If products are recommending the use of enable_rc6=0 I can see they
> adding the patch to disable that. Effect is the same and our convenience is gone.
>
> But again, just my view here. Not a nack ;)

I suppose the compromise would be to make it a boolean module parameter
to only allow disabling rc6 on platforms where it's enabled by default,
but not letting you enable rc6 where it's disabled by default. I.e. only
support i915.enable_rc6=0 to be passed by the user.

BR,
Jani.


>
> Thanks,
> Rodrigo.
>
>> 
>> -- 
>> Ben Widawsky, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Thu, Nov 02, 2017 at 08:06:29AM +0000, Jani Nikula wrote:
> On Wed, 01 Nov 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Wed, Nov 01, 2017 at 04:21:08PM +0000, Ben Widawsky wrote:
> >> On 17-11-01 18:09:47, Joonas Lahtinen wrote:
> >> > + Kimmo and Paul
> >> > 
> >> > On Wed, 2017-11-01 at 07:43 -0700, Ben Widawsky wrote:
> >> > > On 17-11-01 14:07:28, Joonas Lahtinen wrote:
> >> > > > On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
> >> > > > > On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
> >> > > > > > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > On 26/10/17 03:32, 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.
> >> > > > > > > >
> >> > > > > > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> >> > > > > > > > lack of ilk support better.
> >> > > > > > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
> >> > > > > > > >
> >> > > > > > > > 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>
> >> > > > > > > > ---
> >> > > > > > >
> >> > > > > > > I think that for execution/debug on early silicon we might still want the
> >> > > > > > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
> >> > > > > > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
> >> > > > > > > IMO.
> >> > > > > >
> >> > > > > > Most of the BIOSes I've seen on our RVPs have had an option to disable
> >> > > > > > RC6.
> >> > > > >
> >> > > > > BIOS option don't block our code to run and set some MMIOs.
> >> > > > > Not sure how the GPU will behave on such cases.
> >> > > > >
> >> > > > > I like the idea of removing some and keeping the parameters clean.
> >> > > > > But there are few ones like RC6 and disable_power_wells that are very
> >> > > > > useful on platform enabling and also when assisting others to debug issues.
> >> > > > >
> >> > > > > For instance right now that we fixed RC6 on CNL someone told that
> >> > > > > he believe seeing more hangs, so I immediately asked to boot with
> >> > > > > i915.enable_rc6=0 to double check. It is easier and straighforward
> >> > > > > to direct them to the unsafe param than to ask them to compile the code
> >> > > > > with different options or to use some BIOS options that we are not sure.
> >> > > > >
> >> > > > > Also on bug triage some options like this are helpful.
> >> > > > >
> >> > > > > Also BIOS and compile are saved flags. So if you need to do a quick test
> >> > > > > you have to save it, and then unsave later. Parameters are very convinient
> >> > > > > for 1 boot only check.
> >> > > >
> >> > > > It's convenient for sure, but the unsafe module parameters seems to be
> >> > > > finding their way into way too many HOWTOs, and from there to some
> >> > > > "productized" use-cases. Chris states that setting .enable_rc6=0 to
> >> > > > solving an issue on publicly shipping products has been some years ago,
> >> > > > so I don't see a need for carrying this.
> >> > > >
> >> > > > We shouldn't allow the convenience of not having to change one line and
> >> > > > recompile kernel during development to affect the end-users who are
> >> > > > Googling how to get the best performance out of their hardware (I could
> >> > > > mention some distro here).
> >> > > >
> >> > > > This seems the like the best option as I don't think introducing kernel
> >> > > > parameters that only exists on debug builds would be too convenient
> >> > > > either. It'd maybe just add more confusion.
> >> > > >
> >> > > > Regards, Joonas
> >> > > 
> >> > > I believe the ability to disable RC6 is valuable not just for debugging
> >> > > purposes. Folks with very latency sensitive workloads are often willing to
> >> > > forego power savings. The real problem I see is that we don't test without rc6
> >> > > in our setup, which indeed makes it unsafe. As such, I see the other option here
> >> > > going back to the ability to toggle rc6 after load (either module parameter, or
> >> > > make it sysfs), and actually run some subset of our workloads with RC6. I
> >> > > suspect people will poop on that suggestion, but I figured I'd mention.
> >> > 
> >> > I agree there, but by my understanding there's really no ask to support
> >> > the feature in upstream. And the original motive from Chris to drop the
> >> > feature is that it's unsafe as it currently is.
> >> > 
> >> > So unless we've got the resources to bring it back from the unsafe
> >> > zone, I think we should drop it like this patch proposes.
> >> > 
> >> > Regards, Joonas
> >> 
> >> Yep, I agree. One other option would be to move i915_forcewake_user to sysfs and
> >> let them use that.
> >
> > Well, I won't try to block that. I just put my 2 cents that I believe it is a very
> > useful parameter.
> >
> > It wasn't that long ago the last time that we needed the flag to allow
> > end users to have a functional machine: https://plus.google.com/+JonMasters/posts/BqWLEjenLKv.
> >
> > or to debug a related issue:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1440988
> > https://bugzilla.kernel.org/show_bug.cgi?id=116431
> >
> > Although date on few seems over than 1 year. We need to consider that
> > that was our latest new gpu... gen9.
> >
> > If products are recommending the use of enable_rc6=0 I can see they
> > adding the patch to disable that. Effect is the same and our convenience is gone.
> >
> > But again, just my view here. Not a nack ;)
> 
> I suppose the compromise would be to make it a boolean module parameter
> to only allow disabling rc6 on platforms where it's enabled by default,
> but not letting you enable rc6 where it's disabled by default. I.e. only
> support i915.enable_rc6=0 to be passed by the user.

+1. I like this approach.

> 
> BR,
> Jani.
> 
> 
> >
> > Thanks,
> > Rodrigo.
> >
> >> 
> >> -- 
> >> Ben Widawsky, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

On Thu, 2017-11-02 at 07:47 -0700, Rodrigo Vivi wrote:
> On Thu, Nov 02, 2017 at 08:06:29AM +0000, Jani Nikula wrote:
> > On Wed, 01 Nov 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > On Wed, Nov 01, 2017 at 04:21:08PM +0000, Ben Widawsky wrote:
> > > > On 17-11-01 18:09:47, Joonas Lahtinen wrote:
> > > > > + Kimmo and Paul
> > > > > 
> > > > > On Wed, 2017-11-01 at 07:43 -0700, Ben Widawsky wrote:
> > > > > > On 17-11-01 14:07:28, Joonas Lahtinen wrote:
> > > > > > > On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
> > > > > > > > On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
> > > > > > > > > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > On 26/10/17 03:32, 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.
> > > > > > > > > > > 
> > > > > > > > > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
> > > > > > > > > > > lack of ilk support better.
> > > > > > > > > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
> > > > > > > > > > > 
> > > > > > > > > > > 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>
> > > > > > > > > > > ---
> > > > > > > > > > 
> > > > > > > > > > I think that for execution/debug on early silicon we might still want the
> > > > > > > > > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
> > > > > > > > > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
> > > > > > > > > > IMO.
> > > > > > > > > 
> > > > > > > > > Most of the BIOSes I've seen on our RVPs have had an option to disable
> > > > > > > > > RC6.
> > > > > > > > 
> > > > > > > > BIOS option don't block our code to run and set some MMIOs.
> > > > > > > > Not sure how the GPU will behave on such cases.
> > > > > > > > 
> > > > > > > > I like the idea of removing some and keeping the parameters clean.
> > > > > > > > But there are few ones like RC6 and disable_power_wells that are very
> > > > > > > > useful on platform enabling and also when assisting others to debug issues.
> > > > > > > > 
> > > > > > > > For instance right now that we fixed RC6 on CNL someone told that
> > > > > > > > he believe seeing more hangs, so I immediately asked to boot with
> > > > > > > > i915.enable_rc6=0 to double check. It is easier and straighforward
> > > > > > > > to direct them to the unsafe param than to ask them to compile the code
> > > > > > > > with different options or to use some BIOS options that we are not sure.
> > > > > > > > 
> > > > > > > > Also on bug triage some options like this are helpful.
> > > > > > > > 
> > > > > > > > Also BIOS and compile are saved flags. So if you need to do a quick test
> > > > > > > > you have to save it, and then unsave later. Parameters are very convinient
> > > > > > > > for 1 boot only check.
> > > > > > > 
> > > > > > > It's convenient for sure, but the unsafe module parameters seems to be
> > > > > > > finding their way into way too many HOWTOs, and from there to some
> > > > > > > "productized" use-cases. Chris states that setting .enable_rc6=0 to
> > > > > > > solving an issue on publicly shipping products has been some years ago,
> > > > > > > so I don't see a need for carrying this.
> > > > > > > 
> > > > > > > We shouldn't allow the convenience of not having to change one line and
> > > > > > > recompile kernel during development to affect the end-users who are
> > > > > > > Googling how to get the best performance out of their hardware (I could
> > > > > > > mention some distro here).
> > > > > > > 
> > > > > > > This seems the like the best option as I don't think introducing kernel
> > > > > > > parameters that only exists on debug builds would be too convenient
> > > > > > > either. It'd maybe just add more confusion.
> > > > > > > 
> > > > > > > Regards, Joonas
> > > > > > 
> > > > > > I believe the ability to disable RC6 is valuable not just for debugging
> > > > > > purposes. Folks with very latency sensitive workloads are often willing to
> > > > > > forego power savings. The real problem I see is that we don't test without rc6
> > > > > > in our setup, which indeed makes it unsafe. As such, I see the other option here
> > > > > > going back to the ability to toggle rc6 after load (either module parameter, or
> > > > > > make it sysfs), and actually run some subset of our workloads with RC6. I
> > > > > > suspect people will poop on that suggestion, but I figured I'd mention.
> > > > > 
> > > > > I agree there, but by my understanding there's really no ask to support
> > > > > the feature in upstream. And the original motive from Chris to drop the
> > > > > feature is that it's unsafe as it currently is.
> > > > > 
> > > > > So unless we've got the resources to bring it back from the unsafe
> > > > > zone, I think we should drop it like this patch proposes.
> > > > > 
> > > > > Regards, Joonas
> > > > 
> > > > Yep, I agree. One other option would be to move i915_forcewake_user to sysfs and
> > > > let them use that.
> > > 
> > > Well, I won't try to block that. I just put my 2 cents that I believe it is a very
> > > useful parameter.
> > > 
> > > It wasn't that long ago the last time that we needed the flag to allow
> > > end users to have a functional machine: https://plus.google.com/+JonMasters/posts/BqWLEjenLKv.
> > > 
> > > or to debug a related issue:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1440988
> > > https://bugzilla.kernel.org/show_bug.cgi?id=116431
> > > 
> > > Although date on few seems over than 1 year. We need to consider that
> > > that was our latest new gpu... gen9.
> > > 
> > > If products are recommending the use of enable_rc6=0 I can see they
> > > adding the patch to disable that. Effect is the same and our convenience is gone.
> > > 
> > > But again, just my view here. Not a nack ;)
> > 
> > I suppose the compromise would be to make it a boolean module parameter
> > to only allow disabling rc6 on platforms where it's enabled by default,
> > but not letting you enable rc6 where it's disabled by default. I.e. only
> > support i915.enable_rc6=0 to be passed by the user.
> 
> +1. I like this approach.

Umm, it still doesn't resolve the issue that it's not being tested.

I try to be super clear; until we have resources to support that
specific code path, I'd much prefer not to have an easy kernel
parameter to set it.

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

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

[Jani Nikula]

On Thu, 02 Nov 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> On Thu, 2017-11-02 at 07:47 -0700, Rodrigo Vivi wrote:
>> On Thu, Nov 02, 2017 at 08:06:29AM +0000, Jani Nikula wrote:
>> > On Wed, 01 Nov 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> > > On Wed, Nov 01, 2017 at 04:21:08PM +0000, Ben Widawsky wrote:
>> > > > On 17-11-01 18:09:47, Joonas Lahtinen wrote:
>> > > > > + Kimmo and Paul
>> > > > > 
>> > > > > On Wed, 2017-11-01 at 07:43 -0700, Ben Widawsky wrote:
>> > > > > > On 17-11-01 14:07:28, Joonas Lahtinen wrote:
>> > > > > > > On Mon, 2017-10-30 at 10:48 -0700, Rodrigo Vivi wrote:
>> > > > > > > > On Mon, Oct 30, 2017 at 01:00:51PM +0000, David Weinehall wrote:
>> > > > > > > > > On Fri, Oct 27, 2017 at 01:57:09PM -0700, Daniele Ceraolo Spurio wrote:
>> > > > > > > > > > 
>> > > > > > > > > > 
>> > > > > > > > > > On 26/10/17 03:32, 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.
>> > > > > > > > > > > 
>> > > > > > > > > > > v2: Fixup NEEDS_WaRsDisableCoarsePowerGating fumble, and document the
>> > > > > > > > > > > lack of ilk support better.
>> > > > > > > > > > > v3: Clear intel_info->rc6p if we don't support rc6 itself.
>> > > > > > > > > > > 
>> > > > > > > > > > > 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>
>> > > > > > > > > > > ---
>> > > > > > > > > > 
>> > > > > > > > > > I think that for execution/debug on early silicon we might still want the
>> > > > > > > > > > ability to turn features like RC6 off. Maybe we can add a debug kconfig to
>> > > > > > > > > > force info->has_rc6 = 0? Not a blocker to this patch but worth considering
>> > > > > > > > > > IMO.
>> > > > > > > > > 
>> > > > > > > > > Most of the BIOSes I've seen on our RVPs have had an option to disable
>> > > > > > > > > RC6.
>> > > > > > > > 
>> > > > > > > > BIOS option don't block our code to run and set some MMIOs.
>> > > > > > > > Not sure how the GPU will behave on such cases.
>> > > > > > > > 
>> > > > > > > > I like the idea of removing some and keeping the parameters clean.
>> > > > > > > > But there are few ones like RC6 and disable_power_wells that are very
>> > > > > > > > useful on platform enabling and also when assisting others to debug issues.
>> > > > > > > > 
>> > > > > > > > For instance right now that we fixed RC6 on CNL someone told that
>> > > > > > > > he believe seeing more hangs, so I immediately asked to boot with
>> > > > > > > > i915.enable_rc6=0 to double check. It is easier and straighforward
>> > > > > > > > to direct them to the unsafe param than to ask them to compile the code
>> > > > > > > > with different options or to use some BIOS options that we are not sure.
>> > > > > > > > 
>> > > > > > > > Also on bug triage some options like this are helpful.
>> > > > > > > > 
>> > > > > > > > Also BIOS and compile are saved flags. So if you need to do a quick test
>> > > > > > > > you have to save it, and then unsave later. Parameters are very convinient
>> > > > > > > > for 1 boot only check.
>> > > > > > > 
>> > > > > > > It's convenient for sure, but the unsafe module parameters seems to be
>> > > > > > > finding their way into way too many HOWTOs, and from there to some
>> > > > > > > "productized" use-cases. Chris states that setting .enable_rc6=0 to
>> > > > > > > solving an issue on publicly shipping products has been some years ago,
>> > > > > > > so I don't see a need for carrying this.
>> > > > > > > 
>> > > > > > > We shouldn't allow the convenience of not having to change one line and
>> > > > > > > recompile kernel during development to affect the end-users who are
>> > > > > > > Googling how to get the best performance out of their hardware (I could
>> > > > > > > mention some distro here).
>> > > > > > > 
>> > > > > > > This seems the like the best option as I don't think introducing kernel
>> > > > > > > parameters that only exists on debug builds would be too convenient
>> > > > > > > either. It'd maybe just add more confusion.
>> > > > > > > 
>> > > > > > > Regards, Joonas
>> > > > > > 
>> > > > > > I believe the ability to disable RC6 is valuable not just for debugging
>> > > > > > purposes. Folks with very latency sensitive workloads are often willing to
>> > > > > > forego power savings. The real problem I see is that we don't test without rc6
>> > > > > > in our setup, which indeed makes it unsafe. As such, I see the other option here
>> > > > > > going back to the ability to toggle rc6 after load (either module parameter, or
>> > > > > > make it sysfs), and actually run some subset of our workloads with RC6. I
>> > > > > > suspect people will poop on that suggestion, but I figured I'd mention.
>> > > > > 
>> > > > > I agree there, but by my understanding there's really no ask to support
>> > > > > the feature in upstream. And the original motive from Chris to drop the
>> > > > > feature is that it's unsafe as it currently is.
>> > > > > 
>> > > > > So unless we've got the resources to bring it back from the unsafe
>> > > > > zone, I think we should drop it like this patch proposes.
>> > > > > 
>> > > > > Regards, Joonas
>> > > > 
>> > > > Yep, I agree. One other option would be to move i915_forcewake_user to sysfs and
>> > > > let them use that.
>> > > 
>> > > Well, I won't try to block that. I just put my 2 cents that I believe it is a very
>> > > useful parameter.
>> > > 
>> > > It wasn't that long ago the last time that we needed the flag to allow
>> > > end users to have a functional machine: https://plus.google.com/+JonMasters/posts/BqWLEjenLKv.
>> > > 
>> > > or to debug a related issue:
>> > > https://bugzilla.redhat.com/show_bug.cgi?id=1440988
>> > > https://bugzilla.kernel.org/show_bug.cgi?id=116431
>> > > 
>> > > Although date on few seems over than 1 year. We need to consider that
>> > > that was our latest new gpu... gen9.
>> > > 
>> > > If products are recommending the use of enable_rc6=0 I can see they
>> > > adding the patch to disable that. Effect is the same and our convenience is gone.
>> > > 
>> > > But again, just my view here. Not a nack ;)
>> > 
>> > I suppose the compromise would be to make it a boolean module parameter
>> > to only allow disabling rc6 on platforms where it's enabled by default,
>> > but not letting you enable rc6 where it's disabled by default. I.e. only
>> > support i915.enable_rc6=0 to be passed by the user.
>> 
>> +1. I like this approach.
>
> Umm, it still doesn't resolve the issue that it's not being tested.
>
> I try to be super clear; until we have resources to support that
> specific code path, I'd much prefer not to have an easy kernel
> parameter to set it.

It resolves the worst part of the issue: people enabling rc6 where it's
known not to work.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

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

[Chris Wilson]

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

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

[Kelvin Gardiner]

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

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

[Daniel Vetter]

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

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

[Chris Wilson]

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