[PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

[PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Chris Wilson]

Ironlake does support being able to saving and reloading context specific
registers between contexts, providing isolation of the basic GPU state
(as programmable by userspace). This allows userspace to assume that the
GPU retains their state from one batch to the next, minimising the
amount of state it needs to reload.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index fede62daf3e1..88ef00faf576 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -175,6 +175,8 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
 			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
 					PAGE_SIZE);
 		case 5:
+			cxt_size = I915_READ(CXT_SIZE);
+			return round_up(cxt_size * 64, PAGE_SIZE);
 		case 4:
 		case 3:
 		case 2:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e2085820b586..e649b564b165 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1403,11 +1403,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
 		/* These flags are for resource streamer on HSW+ */
 		flags |= HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN;
 	else
+		/* We need to save the extended state for powersaving modes */
 		flags |= MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN;
 
 	len = 4;
 	if (IS_GEN7(i915))
 		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
+	if (IS_GEN5(i915))
+		len += 2;
 
 	cs = intel_ring_begin(rq, len);
 	if (IS_ERR(cs))
@@ -1430,6 +1433,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
 						GEN6_PSMI_SLEEP_MSG_DISABLE);
 			}
 		}
+	} else if (IS_GEN5(i915)) {
+		*cs++ = MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN;
 	}
 
 	*cs++ = MI_NOOP;
@@ -1464,6 +1469,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
 			*cs++ = MI_NOOP;
 		}
 		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	} else if (IS_GEN5(i915)) {
+		*cs++ = MI_SUSPEND_FLUSH;
 	}
 
 	intel_ring_advance(rq, cs);
-- 
2.15.0

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

[PATCH 2/3] drm/i915: Enable render context support for gen4 (Broadwater to Cantiga)

[Chris Wilson]

Broadwater and the rest of gen4  do support being able to saving and
reloading context specific registers between contexts, providing isolation
of the basic GPU state (as programmable by userspace). This allows
userspace to assume that the GPU retains their state from one batch to the
next, minimising the amount of state it needs to reload.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 88ef00faf576..2c9b67e21d48 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -175,9 +175,9 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
 			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
 					PAGE_SIZE);
 		case 5:
+		case 4:
 			cxt_size = I915_READ(CXT_SIZE);
 			return round_up(cxt_size * 64, PAGE_SIZE);
-		case 4:
 		case 3:
 		case 2:
 		/* For the special day when i810 gets merged. */
-- 
2.15.0

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

Re: [PATCH 2/3] drm/i915: Enable render context support for gen4 (Broadwater to Cantiga)

[Ville Syrjälä]

On Thu, Nov 23, 2017 at 04:27:49PM +0000, Chris Wilson wrote:
> Broadwater and the rest of gen4  do support being able to saving and
> reloading context specific registers between contexts, providing isolation
> of the basic GPU state (as programmable by userspace). This allows
> userspace to assume that the GPU retains their state from one batch to the
> next, minimising the amount of state it needs to reload.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 88ef00faf576..2c9b67e21d48 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -175,9 +175,9 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
>  			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
>  					PAGE_SIZE);
>  		case 5:
> +		case 4:
>  			cxt_size = I915_READ(CXT_SIZE);
>  			return round_up(cxt_size * 64, PAGE_SIZE);

On 965 the CXT_SIZE does seem to match the full context image size.
At least my 965 matches the docs perfectly. So just an off by one error
here for 965.

ctg/elk seem to require the same treatment as ilk. Except the extra
we need this time is 9 cachelines. That still can't make the numbers I
see match up with the spec though. I have: CXT_SIZE=0x29,
CTX_SIZE_NOEXT=0x5. The NOEXT value seems to be 2 cachelines too
small. And the full size seems to be 5 cachelines too small. So I wonder
if the full size doesn't account for the media PRT part either? On ILK
we seemed to miss one cacheline there, on ctg/elk 3 cachelines. Or was
it actually two cachelines on ILK, not sure anymore.

Hmm. I wonder if it's the 'Media PRT' part (+ one or two preceding
cachlines) we're missing from this on both platforms. If that's the case
the I guess 0x9+3 and 0xb+2 might be the correct number of extra
cachlines we need on ctg/elk and ilk respectively. Or maybe just add
a comment that we may be missing a few cachelines from the total, but
it doesn't matter?

> -		case 4:
>  		case 3:
>  		case 2:
>  		/* For the special day when i810 gets merged. */
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] 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>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.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  |   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     | 134 +++++++++++-------------------------
 drivers/gpu/drm/i915/intel_uncore.c |   3 -
 10 files changed, 56 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0793a27e2b95..a6af1e710ae1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2505,7 +2505,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 d575a56fc100..6132145eb128 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3214,6 +3214,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) /* HW was never validated */
 
 #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 3328147b4863..7bc538687871 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -46,13 +46,6 @@ i915_param_named_unsafe(panel_ignore_lid, int, 0600,
 	"Override lid status (0=autodetect, 1=autodetect disabled [default], "
 	"-1=force lid closed, -2=force lid open)");
 
-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 8321bd86cba5..c48c88bb95e8 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -34,7 +34,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..edf7221c553a 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -216,6 +216,8 @@ 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 c290cb600eea..c74a20b80182 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -54,7 +54,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 635a96fcd788..1356ea0888a8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1906,15 +1906,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 823d0c2e9ad2..d08e760252d4 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -231,8 +231,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 03d67d8ab647..80f905eafcb5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6417,26 +6417,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;
@@ -6499,42 +6479,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 supported, 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)
@@ -6627,7 +6591,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);
@@ -6668,9 +6632,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) */
@@ -6680,7 +6641,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.
@@ -6689,8 +6652,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);
 }
@@ -6699,7 +6662,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);
@@ -6721,13 +6683,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);
 }
@@ -6776,9 +6736,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);
@@ -6813,22 +6772,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) |
@@ -7271,7 +7220,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);
@@ -7312,10 +7261,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);
@@ -7367,7 +7315,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);
 
@@ -7400,12 +7348,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);
 }
@@ -7932,7 +7876,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);
 	}
@@ -7985,7 +7929,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);
 }
 
@@ -9444,7 +9388,7 @@ u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv,
 	u64 time_hw;
 	u32 mul, div;
 
-	if (!intel_rc6_enabled())
+	if (!HAS_RC6(dev_priv))
 		return 0;
 
 	/* On VLV and CHV, residency time is in CZ units rather than 1.28us */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b4621271e7a2..89547b614aa6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -565,9 +565,6 @@ void intel_uncore_runtime_resume(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

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Enable render context support for Ironlake (gen5)
URL   : https://patchwork.freedesktop.org/series/34315/
State : success

== Summary ==

Series 34315v1 series starting with [1/3] drm/i915: Enable render context support for Ironlake (gen5)
https://patchwork.freedesktop.org/api/1.0/series/34315/revisions/1/mbox/

Test gem_ctx_basic:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
Test gem_ctx_create:
        Subgroup basic:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
        Subgroup basic-files:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
Test gem_ctx_exec:
        Subgroup basic:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
Test gem_ctx_param:
        Subgroup basic:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
        Subgroup basic-default:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
Test gem_ctx_switch:
        Subgroup basic-default:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
        Subgroup basic-default-heavy:
                skip       -> PASS       (fi-bwr-2160)
                skip       -> PASS       (fi-elk-e7500)
                skip       -> PASS       (fi-ilk-650)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (fi-cfl-s2)

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:450s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:383s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:545s
fi-bwr-2160      total:289  pass:191  dwarn:0   dfail:0   fail:0   skip:98  time:440s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:507s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:507s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:502s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:488s
fi-cfl-s2        total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:604s
fi-elk-e7500     total:289  pass:237  dwarn:0   dfail:0   fail:0   skip:52  time:503s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:268s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:433s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:444s
fi-ilk-650       total:289  pass:236  dwarn:0   dfail:0   fail:0   skip:53  time:462s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:477s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:581s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:539s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:565s
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:497s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:463s
fi-snb-2520m     total:246  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:429s
Blacklisted hosts:
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:549s
fi-glk-dsi       total:66   pass:57   dwarn:1   dfail:0   fail:0   skip:7  
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:481s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:529s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:480s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:535s

b407e5f38397c0c22b5056a1664753287993b152 drm-tip: 2017y-11m-23d-16h-14m-59s UTC integration manifest
6b56867b3763 drm/i915: Remove unsafe i915.enable_rc6
dee8ad69fd0b drm/i915: Enable render context support for gen4 (Broadwater to Cantiga)
7a7a9019e1e8 drm/i915: Enable render context support for Ironlake (gen5)

== Logs ==

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

Re: [PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Ville Syrjälä]

On Thu, Nov 23, 2017 at 04:27:48PM +0000, Chris Wilson wrote:
> Ironlake does support being able to saving and reloading context specific
> registers between contexts, providing isolation of the basic GPU state
> (as programmable by userspace). This allows userspace to assume that the
> GPU retains their state from one batch to the next, minimising the
> amount of state it needs to reload.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 2 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index fede62daf3e1..88ef00faf576 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -175,6 +175,8 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
>  			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
>  					PAGE_SIZE);
>  		case 5:
> +			cxt_size = I915_READ(CXT_SIZE);
> +			return round_up(cxt_size * 64, PAGE_SIZE);

I don't think this is correct. It misses the non-pipelined 3D state,
and the ring stuff at the start which IIRC at least SNB still
saved even though it's not used in ring buffer mode. So I think
this needs a 0xb added to the CXT_SIZE value.

But even that doesn't really match the docs. The context image layout
is shown to be 0x3b cachelines long, but 0xb+0x2d only gets us to
0x38. So it looks like CXT_SIZE is off by two for some reason. But
it does't really matter I suppose since we round it up to
a page anyway.

I don't think I ever looked at the context dump to see how big it
really is on ILK. IIRC I did that on SNB and IVB, and maybe HSW.

>  		case 4:
>  		case 3:
>  		case 2:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e2085820b586..e649b564b165 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1403,11 +1403,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  		/* These flags are for resource streamer on HSW+ */
>  		flags |= HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN;
>  	else
> +		/* We need to save the extended state for powersaving modes */
>  		flags |= MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN;
>  
>  	len = 4;
>  	if (IS_GEN7(i915))
>  		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
> +	if (IS_GEN5(i915))
> +		len += 2;
>  
>  	cs = intel_ring_begin(rq, len);
>  	if (IS_ERR(cs))
> @@ -1430,6 +1433,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  						GEN6_PSMI_SLEEP_MSG_DISABLE);
>  			}
>  		}
> +	} else if (IS_GEN5(i915)) {
> +		*cs++ = MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN;

Hmm. These are documented for steppings A/B. I wonder what is the
first production stepping for ILK. Latest stepping mentioned in the
w/a db is C2.

Oh, actually w/a db has WaIlkEnableDisableSuspendFlush listed a
"forever". Not sure which source to believe here.

>  	}
>  
>  	*cs++ = MI_NOOP;
> @@ -1464,6 +1469,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  			*cs++ = MI_NOOP;
>  		}
>  		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
> +	} else if (IS_GEN5(i915)) {
> +		*cs++ = MI_SUSPEND_FLUSH;
>  	}
>  
>  	intel_ring_advance(rq, cs);
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Ville Syrjälä]

On Thu, Nov 23, 2017 at 07:46:23PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 23, 2017 at 04:27:48PM +0000, Chris Wilson wrote:
> > Ironlake does support being able to saving and reloading context specific
> > registers between contexts, providing isolation of the basic GPU state
> > (as programmable by userspace). This allows userspace to assume that the
> > GPU retains their state from one batch to the next, minimising the
> > amount of state it needs to reload.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c  | 2 ++
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index fede62daf3e1..88ef00faf576 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -175,6 +175,8 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
> >  			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
> >  					PAGE_SIZE);
> >  		case 5:
> > +			cxt_size = I915_READ(CXT_SIZE);
> > +			return round_up(cxt_size * 64, PAGE_SIZE);
> 
> I don't think this is correct. It misses the non-pipelined 3D state,
> and the ring stuff at the start which IIRC at least SNB still
> saved even though it's not used in ring buffer mode. So I think
> this needs a 0xb added to the CXT_SIZE value.
> 
> But even that doesn't really match the docs. The context image layout
> is shown to be 0x3b cachelines long, but 0xb+0x2d only gets us to
> 0x38. So it looks like CXT_SIZE is off by two for some reason.

Oh. Actually CXT_SIZE is documented to be U5-1 on pre-SNB, so I
guess it's only off by one at most.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Chris Wilson]

Quoting Ville Syrjälä (2017-11-23 17:50:50)
> On Thu, Nov 23, 2017 at 07:46:23PM +0200, Ville Syrjälä wrote:
> > On Thu, Nov 23, 2017 at 04:27:48PM +0000, Chris Wilson wrote:
> > > Ironlake does support being able to saving and reloading context specific
> > > registers between contexts, providing isolation of the basic GPU state
> > > (as programmable by userspace). This allows userspace to assume that the
> > > GPU retains their state from one batch to the next, minimising the
> > > amount of state it needs to reload.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_engine_cs.c  | 2 ++
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
> > >  2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > index fede62daf3e1..88ef00faf576 100644
> > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > @@ -175,6 +175,8 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
> > >                     return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
> > >                                     PAGE_SIZE);
> > >             case 5:
> > > +                   cxt_size = I915_READ(CXT_SIZE);
> > > +                   return round_up(cxt_size * 64, PAGE_SIZE);
> > 
> > I don't think this is correct. It misses the non-pipelined 3D state,
> > and the ring stuff at the start which IIRC at least SNB still
> > saved even though it's not used in ring buffer mode. So I think
> > this needs a 0xb added to the CXT_SIZE value.
> > 
> > But even that doesn't really match the docs. The context image layout
> > is shown to be 0x3b cachelines long, but 0xb+0x2d only gets us to
> > 0x38. So it looks like CXT_SIZE is off by two for some reason.
> 
> Oh. Actually CXT_SIZE is documented to be U5-1 on pre-SNB, so I
> guess it's only off by one at most.

U5-1, sounds typical of the hw engineers -- anything up to 2k which
corresponds with their 2k alignment requirement and not cross the page
boundary (which iirc was mentioned for gen4).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Chris Wilson]

Quoting Ville Syrjälä (2017-11-23 17:46:23)
> On Thu, Nov 23, 2017 at 04:27:48PM +0000, Chris Wilson wrote:
> > Ironlake does support being able to saving and reloading context specific
> > registers between contexts, providing isolation of the basic GPU state
> > (as programmable by userspace). This allows userspace to assume that the
> > GPU retains their state from one batch to the next, minimising the
> > amount of state it needs to reload.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c  | 2 ++
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index fede62daf3e1..88ef00faf576 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -175,6 +175,8 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
> >                       return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
> >                                       PAGE_SIZE);
> >               case 5:
> > +                     cxt_size = I915_READ(CXT_SIZE);
> > +                     return round_up(cxt_size * 64, PAGE_SIZE);
> 
> I don't think this is correct. It misses the non-pipelined 3D state,
> and the ring stuff at the start which IIRC at least SNB still
> saved even though it's not used in ring buffer mode. So I think
> this needs a 0xb added to the CXT_SIZE value.

/me scratches his head
This was described as the full size of the context; including the
extended state required for the power context.
In the past we know that it is less that one page, which is our minimum
allocation.
 
> But even that doesn't really match the docs. The context image layout
> is shown to be 0x3b cachelines long, but 0xb+0x2d only gets us to
> 0x38. So it looks like CXT_SIZE is off by two for some reason. But
> it does't really matter I suppose since we round it up to
> a page anyway.

Right. I started with just using 1 page and ignoring the register all
together.

> I don't think I ever looked at the context dump to see how big it
> really is on ILK. IIRC I did that on SNB and IVB, and maybe HSW.
> 
> >               case 4:
> >               case 3:
> >               case 2:
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index e2085820b586..e649b564b165 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1403,11 +1403,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
> >               /* These flags are for resource streamer on HSW+ */
> >               flags |= HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN;
> >       else
> > +             /* We need to save the extended state for powersaving modes */
> >               flags |= MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN;
> >  
> >       len = 4;
> >       if (IS_GEN7(i915))
> >               len += 2 + (num_rings ? 4*num_rings + 6 : 0);
> > +     if (IS_GEN5(i915))
> > +             len += 2;
> >  
> >       cs = intel_ring_begin(rq, len);
> >       if (IS_ERR(cs))
> > @@ -1430,6 +1433,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
> >                                               GEN6_PSMI_SLEEP_MSG_DISABLE);
> >                       }
> >               }
> > +     } else if (IS_GEN5(i915)) {
> > +             *cs++ = MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN;
> 
> Hmm. These are documented for steppings A/B. I wonder what is the
> first production stepping for ILK. Latest stepping mentioned in the
> w/a db is C2.

I was going by Ben's notes that we still wanted these for undetermined
reasons based around the powerctx programming example, iirc. Certainly,
the bspec I found only had them for a/b, but it should be a no-op for us
in any case as we don't use sync flush.
 
> Oh, actually w/a db has WaIlkEnableDisableSuspendFlush listed a
> "forever". Not sure which source to believe here.

I think it's safe and cheap enough to include them. But probably worth a
note that it's more paranoia than anything.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/3] drm/i915: Enable render context support for Ironlake (gen5)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Enable render context support for Ironlake (gen5)
URL   : https://patchwork.freedesktop.org/series/34315/
State : failure

== Summary ==

Test pm_rc6_residency:
        Subgroup rc6p-accuracy:
                skip       -> PASS       (shard-snb)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                pass       -> FAIL       (shard-snb) fdo#101623
Test kms_cursor_legacy:
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> SKIP       (shard-hsw) fdo#102670
Test kms_chv_cursor_fail:
        Subgroup pipe-b-128x128-bottom-edge:
                pass       -> SKIP       (shard-hsw)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (shard-hsw)
Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2667 pass:1533 dwarn:1   dfail:0   fail:10  skip:1123 time:9498s
shard-snb        total:2667 pass:1312 dwarn:1   dfail:0   fail:14  skip:1340 time:8069s
Blacklisted hosts:
shard-apl        total:2458 pass:1550 dwarn:2   dfail:0   fail:26  skip:878 time:12487s
shard-kbl        total:2667 pass:1800 dwarn:1   dfail:0   fail:25  skip:841 time:10898s

== Logs ==

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

[PATCH v2] drm/i915: Enable render context support for Ironlake (gen5)

[Chris Wilson]

Ironlake does support being able to saving and reloading context specific
registers between contexts, providing isolation of the basic GPU state
(as programmable by userspace). This allows userspace to assume that the
GPU retains their state from one batch to the next, minimising the
amount of state it needs to reload.

v2: Fix off-by-one in reading CXT_SIZE, and add a comment that the
CXT_SIZE and context-layout do not match in bspec, but the difference is
irrelevant as we overallocate the full page anyway (Ville).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 16 ++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 13 +++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index fede62daf3e1..5b99125a179b 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -175,6 +175,22 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
 			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
 					PAGE_SIZE);
 		case 5:
+			/*
+			 * There is a discrepancy here between the size reported
+			 * by the register and the size of the context layout
+			 * in the docs. Both are described as authorative!
+			 *
+			 * The discrepancy is on the order of a few cachelines,
+			 * but the total is under one page (4k), which is our
+			 * minimum allocation anyway so it should all come
+			 * out in the wash.
+			 */
+			cxt_size = I915_READ(CXT_SIZE) + 1;
+			DRM_DEBUG_DRIVER("gen%d CXT_SIZE = %d bytes [0x%08x]\n",
+					 INTEL_GEN(dev_priv),
+					 cxt_size * 64,
+					 cxt_size - 1);
+			return round_up(cxt_size * 64, PAGE_SIZE);
 		case 4:
 		case 3:
 		case 2:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e2085820b586..2074749b27a5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1403,11 +1403,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
 		/* These flags are for resource streamer on HSW+ */
 		flags |= HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN;
 	else
+		/* We need to save the extended state for powersaving modes */
 		flags |= MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN;
 
 	len = 4;
 	if (IS_GEN7(i915))
 		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
+	if (IS_GEN5(i915))
+		len += 2;
 
 	cs = intel_ring_begin(rq, len);
 	if (IS_ERR(cs))
@@ -1430,6 +1433,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
 						GEN6_PSMI_SLEEP_MSG_DISABLE);
 			}
 		}
+	} else if (IS_GEN5(i915)) {
+		/*
+		 * This w/a is only listed for pre-production ilk a/b steppings,
+		 * but is also mentioned for programming the powerctx. To be
+		 * safe, just apply the workaround; we do not use SyncFlush so
+		 * this should never take effect and so be a no-op!
+		 */
+		*cs++ = MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN;
 	}
 
 	*cs++ = MI_NOOP;
@@ -1464,6 +1475,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
 			*cs++ = MI_NOOP;
 		}
 		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	} else if (IS_GEN5(i915)) {
+		*cs++ = MI_SUSPEND_FLUSH;
 	}
 
 	intel_ring_advance(rq, cs);
-- 
2.15.0

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

Re: [PATCH v2] drm/i915: Enable render context support for Ironlake (gen5)

[Ville Syrjälä]

On Thu, Nov 23, 2017 at 08:47:45PM +0000, Chris Wilson wrote:
> Ironlake does support being able to saving and reloading context specific
> registers between contexts, providing isolation of the basic GPU state
> (as programmable by userspace). This allows userspace to assume that the
> GPU retains their state from one batch to the next, minimising the
> amount of state it needs to reload.
> 
> v2: Fix off-by-one in reading CXT_SIZE, and add a comment that the
> CXT_SIZE and context-layout do not match in bspec, but the difference is
> irrelevant as we overallocate the full page anyway (Ville).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 13 +++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index fede62daf3e1..5b99125a179b 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -175,6 +175,22 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
>  			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
>  					PAGE_SIZE);
>  		case 5:
> +			/*
> +			 * There is a discrepancy here between the size reported
> +			 * by the register and the size of the context layout
> +			 * in the docs. Both are described as authorative!
> +			 *
> +			 * The discrepancy is on the order of a few cachelines,
> +			 * but the total is under one page (4k), which is our
> +			 * minimum allocation anyway so it should all come
> +			 * out in the wash.
> +			 */
> +			cxt_size = I915_READ(CXT_SIZE) + 1;
> +			DRM_DEBUG_DRIVER("gen%d CXT_SIZE = %d bytes [0x%08x]\n",
> +					 INTEL_GEN(dev_priv),
> +					 cxt_size * 64,
> +					 cxt_size - 1);
> +			return round_up(cxt_size * 64, PAGE_SIZE);
>  		case 4:
>  		case 3:
>  		case 2:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e2085820b586..2074749b27a5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1403,11 +1403,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  		/* These flags are for resource streamer on HSW+ */
>  		flags |= HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN;
>  	else
> +		/* We need to save the extended state for powersaving modes */
>  		flags |= MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN;
>  
>  	len = 4;
>  	if (IS_GEN7(i915))
>  		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
> +	if (IS_GEN5(i915))
> +		len += 2;
>  
>  	cs = intel_ring_begin(rq, len);
>  	if (IS_ERR(cs))
> @@ -1430,6 +1433,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  						GEN6_PSMI_SLEEP_MSG_DISABLE);
>  			}
>  		}
> +	} else if (IS_GEN5(i915)) {
> +		/*
> +		 * This w/a is only listed for pre-production ilk a/b steppings,
> +		 * but is also mentioned for programming the powerctx. To be
> +		 * safe, just apply the workaround; we do not use SyncFlush so
> +		 * this should never take effect and so be a no-op!
> +		 */
> +		*cs++ = MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN;

Maybe also toss in the name from the w/a db?

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	}
>  
>  	*cs++ = MI_NOOP;
> @@ -1464,6 +1475,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  			*cs++ = MI_NOOP;
>  		}
>  		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
> +	} else if (IS_GEN5(i915)) {
> +		*cs++ = MI_SUSPEND_FLUSH;
>  	}
>  
>  	intel_ring_advance(rq, cs);
> -- 
> 2.15.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Enable render context support for Ironlake (gen5) (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Enable render context support for Ironlake (gen5) (rev2)
URL   : https://patchwork.freedesktop.org/series/34315/
State : failure

== Summary ==

Applying: drm/i915: Enable render context support for Ironlake (gen5)
Applying: drm/i915: Enable render context support for gen4 (Broadwater to Cantiga)
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_engine_cs.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_engine_cs.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_engine_cs.c
Patch failed at 0002 drm/i915: Enable render context support for gen4 (Broadwater to Cantiga)

Current HEAD:
commit b407e5f38397c0c22b5056a1664753287993b152
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 23 16:15:35 2017 +0000

    drm-tip: 2017y-11m-23d-16h-14m-59s UTC integration manifest

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