Re: [PATCH] drm/i915/cnl: Add support slice/subslice/eu configs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 08/31/2017 04:59 PM, Rodrigo Vivi wrote:
From: Ben Widawsky <ben@xxxxxxxxxxxx>

Cannonlake Slice and Subslice information has changed.
This Patch provided by Ben adds the proper sseu
initialization.

v2: This v2 done by Rodrigo includes:
     - Fix on Total slices count by avoiding [1][2] and [2][2].
     - Inclusion of EU Per Subslice.
     - Commit message.

Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.h          |  5 +++-
  drivers/gpu/drm/i915/i915_reg.h          | 20 +++++++++++++
  drivers/gpu/drm/i915/intel_device_info.c | 50 +++++++++++++++++++++++++++++++-
  3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0383e879a315..2fdd59e85189 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -786,7 +786,10 @@ struct sseu_dev_info {
  	u8 slice_mask;
  	u8 subslice_mask;
  	u8 eu_total;
-	u8 eu_per_subslice;
+	union {
+		u8 per_subslice_eu_disable_mask[3][3];
+		u8 eu_per_subslice;
+	};
  	u8 min_eu_in_pool;
  	/* For each slice, which subslice(s) has(have) 7 EUs (bitfield)? */
  	u8 subslice_7eu[3];
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5651081ff789..7f71007baa94 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2730,6 +2730,11 @@ enum i915_power_well_id {
  #define   GEN9_F2_SS_DIS_SHIFT		20
  #define   GEN9_F2_SS_DIS_MASK		(0xf << GEN9_F2_SS_DIS_SHIFT)
+#define GEN10_F2_S_ENA_SHIFT 22
+#define   GEN10_F2_S_ENA_MASK		(0x7 << GEN10_F2_S_ENA_SHIFT)
+#define   GEN10_F2_SS_DIS_SHIFT		18
+#define   GEN10_F2_SS_DIS_MASK		(0x7 << GEN10_F2_SS_DIS_SHIFT)
+

Technically, the BSpec allows 6 bits for slice_enable (of which only 4 are used) and 4 bits for subslice_disable, instead of 3 and 3, right? Even if some variants are not productized, isn't it better to cover our back and read the fuses completely?

  #define GEN8_EU_DISABLE0		_MMIO(0x9134)
  #define   GEN8_EU_DIS0_S0_MASK		0xffffff
  #define   GEN8_EU_DIS0_S1_SHIFT		24
@@ -2745,6 +2750,21 @@ enum i915_power_well_id {
#define GEN9_EU_DISABLE(slice) _MMIO(0x9134 + (slice)*0x4) +#define GEN10_EU_DIS0_S0_SHIFT 0
+#define GEN10_EU_DIS0_S0_MASK		(0xff << GEN10_EU_DIS0_S0_SHIFT)
+#define GEN10_EU_DIS0_S1_SHIFT		8
+#define GEN10_EU_DIS0_S1_MASK		(0xff << GEN10_EU_DIS0_S1_SHIFT)
+#define GEN10_EU_DIS0_S2_SHIFT		16
+#define GEN10_EU_DIS0_S2_MASK		(0xff << GEN10_EU_DIS0_S2_SHIFT)
+#define GEN10_EU_DIS1_S0_SHIFT		24
+#define GEN10_EU_DIS1_S0_MASK		(0xff << GEN10_EU_DIS1_S0_SHIFT)
+#define GEN10_EU_DIS1_S1_SHIFT		0
+#define GEN10_EU_DIS1_S1_MASK		(0xff << GEN10_EU_DIS1_S1_SHIFT)
+#define GEN10_EU_DIS2_S0_SHIFT		8
+#define GEN10_EU_DIS2_S0_MASK		(0xff << GEN10_EU_DIS2_S0_SHIFT)
+#define GEN10_EU_DIS2_S1_SHIFT		16
+#define GEN10_EU_DIS2_S1_MASK		(0xff << GEN10_EU_DIS2_S1_SHIFT)
+

I suggest a naming like:

GEN10_EU_DIS_Sm_SSn_SHIFT

where n is the slice number and m the subslice number

Also, related to my previous comment, there should be a:

#define GEN10_EU_DIS3_S0_SHIFT        24
#define GEN10_EU_DIS3_S0_MASK        (0xff << GEN10_EU_DIS3_S0_SHIFT)

  #define GEN6_BSD_SLEEP_PSMI_CONTROL	_MMIO(0x12050)
  #define   GEN6_BSD_SLEEP_MSG_DISABLE	(1 << 0)
  #define   GEN6_BSD_SLEEP_FLUSH_DISABLE	(1 << 2)
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 5f91ddc78c7a..da13becf97f1 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -82,6 +82,52 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv)
  #undef PRINT_FLAG
  }
+static void gen10_sseu_info_init(struct drm_i915_private *dev_priv)
+{
+	struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
+	const u32 fuse2 = I915_READ(GEN8_FUSE2);
+	u32 temp, i, j;
+
+	sseu->slice_mask = (fuse2 & GEN10_F2_S_ENA_MASK) >> GEN10_F2_S_ENA_SHIFT;
+	sseu->subslice_mask = (1 << 3) - 1;
+	sseu->subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
+				 GEN10_F2_SS_DIS_SHIFT);

See my comments above about slice_enable and subslice_disable masks.

+	temp = I915_READ(GEN8_EU_DISABLE0);
+	sseu->per_subslice_eu_disable_mask[0][0] =
+		(temp & GEN10_EU_DIS0_S0_MASK) >> GEN10_EU_DIS0_S0_SHIFT;
+	sseu->per_subslice_eu_disable_mask[0][1] =
+		(temp & GEN10_EU_DIS0_S1_MASK) >> GEN10_EU_DIS0_S1_SHIFT;
+	sseu->per_subslice_eu_disable_mask[0][2] =
+		(temp & GEN10_EU_DIS0_S2_MASK) >> GEN10_EU_DIS0_S2_SHIFT;
+	sseu->per_subslice_eu_disable_mask[1][0] =
+		(temp & GEN10_EU_DIS1_S0_MASK) >> GEN10_EU_DIS1_S0_SHIFT;
+
+	temp = I915_READ(GEN8_EU_DISABLE1);
+	sseu->per_subslice_eu_disable_mask[1][1] =
+		(temp & GEN10_EU_DIS1_S1_MASK) >> GEN10_EU_DIS1_S1_SHIFT;
+	sseu->per_subslice_eu_disable_mask[2][0] =
+		(temp & GEN10_EU_DIS2_S0_MASK) >> GEN10_EU_DIS2_S0_SHIFT;
+	sseu->per_subslice_eu_disable_mask[2][1] =
+		(temp & GEN10_EU_DIS2_S1_MASK) >> GEN10_EU_DIS2_S1_SHIFT;
+
+	for (i = 0; i < 3; i++)
+		for (j = 0; j < (i == 0 ? 3 : 2); j++)
+			sseu->eu_total +=
+				hweight8(~sseu->per_subslice_eu_disable_mask[i][j]);

Same thing as before.
Also: don't we want to read all of this back in debugfs (i915_sseu_status)? (although maybe there is a separate patch for this that I have missed)

+	/* Let's assume uniform distribution */
+	sseu->eu_per_subslice = sseu_subslice_total(sseu) ?
+				DIV_ROUND_UP(sseu->eu_total,
+					     sseu_subslice_total(sseu)) : 0;

Can we add a comment similar to the one for Gen9?:

    /*
     * CNL is expected to always have a uniform distribution
     * of EU across subslices with the exception that any one
     * EU in any one subslice may be fused off for die
     * recovery.
     */

+	/* No restrictions on Power Gating */
+	sseu->has_slice_pg = 1;
+	sseu->has_subslice_pg = 1;
+	sseu->has_eu_pg = 1;
+}
+
+
  static void cherryview_sseu_info_init(struct drm_i915_private *dev_priv)
  {
  	struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
@@ -409,8 +455,10 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
  		cherryview_sseu_info_init(dev_priv);
  	else if (IS_BROADWELL(dev_priv))
  		broadwell_sseu_info_init(dev_priv);
-	else if (INTEL_INFO(dev_priv)->gen >= 9)
+	else if (INTEL_INFO(dev_priv)->gen == 9)
  		gen9_sseu_info_init(dev_priv);
+	else if (INTEL_INFO(dev_priv)->gen >= 10)
+		gen10_sseu_info_init(dev_priv);

Maybe leverage this change to move to INTEL_GEN(dev_priv)?

info->has_snoop = !info->has_llc;

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux