[PATCH 0/6] drm/i915: expose RCS topology to userspace

[PATCH 0/6] drm/i915: expose RCS topology to userspace

[Lionel Landwerlin]

Hi all,

This series a respin of a few attempts ([1], [2], [3]) to expose RCS
topology to userspace.

The motivation for this is to be able to monitor part of the GPU using
i915 perf and have detailled knowledge about what execution units are
fused off. Let's say you monitor slice 1, knowing how many EUs are
actually usuable on that slice is critical to provide normalized
numbers (like average busyness of the EUs on slice1) on that
particular part of the GPU. Currently numbers returned like
I915_PARAM_EU_TOTAL don't give us that information.

This series introduces a new uAPI to query information from i915. It's
partially inspired by the amdgpu_cs_ioctl, although we don't allow
setting values onto the device, only queries. There is a good chance
this new uAPI will be reused for exposing available engines (Tvrtko
was looking into this).

Thanks,

[1]: https://patchwork.freedesktop.org/patch/185959/
[2]: https://patchwork.freedesktop.org/series/33436/
[3]: https://patchwork.freedesktop.org/series/33950/

Lionel Landwerlin (6):
  drm/i915: store all subslice masks
  drm/i915/debugfs: reuse max slice/subslices already stored in sseu
  drm/i915/debugfs: add rcs topology entry
  drm/i915: add rcs topology to error state
  drm/i915: add query uAPI
  drm/i915: expose rcs topology through query uAPI

 drivers/gpu/drm/i915/Makefile            |   1 +
 drivers/gpu/drm/i915/i915_debugfs.c      |  87 ++++++++++-----
 drivers/gpu/drm/i915/i915_drv.c          |   3 +-
 drivers/gpu/drm/i915/i915_drv.h          |  26 ++++-
 drivers/gpu/drm/i915/i915_gpu_error.c    |  35 ++++++
 drivers/gpu/drm/i915/i915_query.c        | 186 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_device_info.c | 169 ++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
 drivers/gpu/drm/i915/intel_query_info.c  |  88 +++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
 include/uapi/drm/i915_drm.h              |  76 +++++++++++++
 11 files changed, 598 insertions(+), 77 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_query.c
 create mode 100644 drivers/gpu/drm/i915/intel_query_info.c

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

[PATCH 1/6] drm/i915: store all subslice masks

[Lionel Landwerlin]

Up to now, subslice mask was assumed to be uniform across slices. But
starting with Cannonlake, slices can be asymetric (for example slice0
has different number of subslices as slice1+). This change stores all
subslices masks for all slices rather than having a single mask that
applies to all slices.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c      |  24 +++--
 drivers/gpu/drm/i915/i915_drv.c          |   2 +-
 drivers/gpu/drm/i915/i915_drv.h          |  23 ++++-
 drivers/gpu/drm/i915/intel_device_info.c | 169 ++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
 6 files changed, 161 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0ddce72552bf..0c7890b695c5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4293,7 +4293,7 @@ static void cherryview_sseu_device_status(struct drm_i915_private *dev_priv,
 			continue;
 
 		sseu->slice_mask = BIT(0);
-		sseu->subslice_mask |= BIT(ss);
+		sseu->subslices_mask[0] |= BIT(ss);
 		eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) +
 			 ((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) +
 			 ((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) +
@@ -4340,7 +4340,7 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
 			continue;
 
 		sseu->slice_mask |= BIT(s);
-		sseu->subslice_mask = info->sseu.subslice_mask;
+		sseu->subslices_mask[s] = info->sseu.subslices_mask[s];
 
 		for (ss = 0; ss < ss_max; ss++) {
 			unsigned int eu_cnt;
@@ -4395,8 +4395,8 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 		sseu->slice_mask |= BIT(s);
 
 		if (IS_GEN9_BC(dev_priv))
-			sseu->subslice_mask =
-				INTEL_INFO(dev_priv)->sseu.subslice_mask;
+			sseu->subslices_mask[s] =
+				INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
 
 		for (ss = 0; ss < ss_max; ss++) {
 			unsigned int eu_cnt;
@@ -4406,7 +4406,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 					/* skip disabled subslice */
 					continue;
 
-				sseu->subslice_mask |= BIT(ss);
+				sseu->subslices_mask[s] |= BIT(ss);
 			}
 
 			eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
@@ -4428,9 +4428,12 @@ static void broadwell_sseu_device_status(struct drm_i915_private *dev_priv,
 	sseu->slice_mask = slice_info & GEN8_LSLICESTAT_MASK;
 
 	if (sseu->slice_mask) {
-		sseu->subslice_mask = INTEL_INFO(dev_priv)->sseu.subslice_mask;
 		sseu->eu_per_subslice =
 				INTEL_INFO(dev_priv)->sseu.eu_per_subslice;
+		for (s = 0; s < fls(sseu->slice_mask); s++) {
+			sseu->subslices_mask[s] =
+				INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
+		}
 		sseu->eu_total = sseu->eu_per_subslice *
 				 sseu_subslice_total(sseu);
 
@@ -4449,6 +4452,7 @@ static void i915_print_sseu_info(struct seq_file *m, bool is_available_info,
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	const char *type = is_available_info ? "Available" : "Enabled";
+	int s;
 
 	seq_printf(m, "  %s Slice Mask: %04x\n", type,
 		   sseu->slice_mask);
@@ -4456,10 +4460,10 @@ static void i915_print_sseu_info(struct seq_file *m, bool is_available_info,
 		   hweight8(sseu->slice_mask));
 	seq_printf(m, "  %s Subslice Total: %u\n", type,
 		   sseu_subslice_total(sseu));
-	seq_printf(m, "  %s Subslice Mask: %04x\n", type,
-		   sseu->subslice_mask);
-	seq_printf(m, "  %s Subslice Per Slice: %u\n", type,
-		   hweight8(sseu->subslice_mask));
+	for (s = 0; s < fls(sseu->slice_mask); s++) {
+		seq_printf(m, "  %s Slice%i Subslice Mask: %04x\n", type,
+			   s, sseu->subslices_mask[s]);
+	}
 	seq_printf(m, "  %s EU Total: %u\n", type,
 		   sseu->eu_total);
 	seq_printf(m, "  %s EU Per Subslice: %u\n", type,
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 72bea281edb7..8b99e415c345 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -414,7 +414,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 			return -ENODEV;
 		break;
 	case I915_PARAM_SUBSLICE_MASK:
-		value = INTEL_INFO(dev_priv)->sseu.subslice_mask;
+		value = INTEL_INFO(dev_priv)->sseu.subslices_mask[0];
 		if (!value)
 			return -ENODEV;
 		break;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1aba5657f5f0..82fc59078c6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -802,9 +802,12 @@ struct intel_csr {
 	func(supports_tv); \
 	func(has_ipc);
 
+#define GEN_MAX_SLICES		(6) /* CNL upper bound */
+#define GEN_MAX_SUBSLICES	(7)
+
 struct sseu_dev_info {
 	u8 slice_mask;
-	u8 subslice_mask;
+	u8 subslices_mask[GEN_MAX_SLICES];
 	u8 eu_total;
 	u8 eu_per_subslice;
 	u8 min_eu_in_pool;
@@ -813,11 +816,27 @@ struct sseu_dev_info {
 	u8 has_slice_pg:1;
 	u8 has_subslice_pg:1;
 	u8 has_eu_pg:1;
+
+	/* Topology fields */
+	u8 max_slices;
+	u8 max_subslices;
+	u8 max_eus_per_subslice;
+
+	/* We don't have more than 8 eus per subslice at the moment and as we
+	 * store eus enabled using bits, no need to multiply by eus per
+	 * subslice.
+	 */
+	u8 eu_mask[GEN_MAX_SLICES * GEN_MAX_SUBSLICES];
 };
 
 static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
 {
-	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
+	unsigned s, total = 0;
+
+	for (s = 0; s < ARRAY_SIZE(sseu->subslices_mask); s++)
+		total += hweight8(sseu->subslices_mask[s]);
+
+	return total;
 }
 
 /* Keep in gen based order, and chronological order within a gen */
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index f478be3ae0ba..6a3c40439e83 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -82,22 +82,74 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv)
 #undef PRINT_FLAG
 }
 
+static u8 compute_eu_total(const struct sseu_dev_info *sseu)
+{
+	u8 i, total = 0;
+
+	for (i = 0; i < ARRAY_SIZE(sseu->eu_mask); i++)
+		total += hweight8(sseu->eu_mask[i]);
+
+	return total;
+}
+
 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);
+	int s, ss, eu_mask = 0xff;
+	u32 subslice_mask, eu_en;
 
 	sseu->slice_mask = (fuse2 & GEN10_F2_S_ENA_MASK) >>
 			    GEN10_F2_S_ENA_SHIFT;
-	sseu->subslice_mask = (1 << 4) - 1;
-	sseu->subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
-				 GEN10_F2_SS_DIS_SHIFT);
+	sseu->max_slices = 6;
+	sseu->max_subslices = 4;
+	sseu->max_eus_per_subslice = 8;
+
+	subslice_mask = (1 << 4) - 1;
+	subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
+			   GEN10_F2_SS_DIS_SHIFT);
+
+	/* Slice0 can have up to 3 subslices, but there are only 2 in
+	 * slice1/2.
+	 */
+	sseu->subslices_mask[0] = subslice_mask;
+	for (s = 1; s < sseu->max_slices; s++)
+		sseu->subslices_mask[s] = subslice_mask & 0x3;
+
+	/* Slice0 */
+	eu_en = ~I915_READ(GEN8_EU_DISABLE0);
+	for (ss = 0; ss < sseu->max_subslices; ss++)
+		sseu->eu_mask[ss]     = (eu_en >> (8 * ss)) & eu_mask;
+	/* Slice1 */
+	sseu->eu_mask[sseu->max_subslices]         = (eu_en >> 24) & eu_mask;
+	eu_en = ~I915_READ(GEN8_EU_DISABLE1);
+	sseu->eu_mask[sseu->max_subslices + 1]     = eu_en & eu_mask;
+	/* Slice2 */
+	sseu->eu_mask[2 * sseu->max_subslices]     = (eu_en >> 8) & eu_mask;
+	sseu->eu_mask[2 * sseu->max_subslices + 1] = (eu_en >> 16) & eu_mask;
+	/* Slice3 */
+	sseu->eu_mask[3 * sseu->max_subslices]     = (eu_en >> 24) & eu_mask;
+	eu_en = ~I915_READ(GEN8_EU_DISABLE2);
+	sseu->eu_mask[3 * sseu->max_subslices + 1] = eu_en & eu_mask;
+	/* Slice4 */
+	sseu->eu_mask[4 * sseu->max_subslices]     = (eu_en >> 8) & eu_mask;
+	sseu->eu_mask[4 * sseu->max_subslices + 1] = (eu_en >> 16) & eu_mask;
+	/* Slice5 */
+	sseu->eu_mask[5 * sseu->max_subslices]     = (eu_en >> 24) & eu_mask;
+	eu_en = ~I915_READ(GEN10_EU_DISABLE3);
+	sseu->eu_mask[5 * sseu->max_subslices + 1] = eu_en & eu_mask;
+
+	/* Do a second pass where we marked the subslices disabled if all
+	 * their eus are off.
+	 */
+	for (s = 0; s < sseu->max_slices; s++) {
+		for (ss = 0; ss < sseu->max_subslices; ss++) {
+			if (sseu->eu_mask[s * sseu->max_subslices + ss] == 0)
+				sseu->subslices_mask[s] &= ~BIT(ss);
+		}
+	}
 
-	sseu->eu_total = hweight32(~I915_READ(GEN8_EU_DISABLE0));
-	sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE1));
-	sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE2));
-	sseu->eu_total += hweight8(~(I915_READ(GEN10_EU_DISABLE3) &
-				     GEN10_EU_DIS_SS_MASK));
+	sseu->eu_total = compute_eu_total(sseu);
 
 	/*
 	 * CNL is expected to always have a uniform distribution
@@ -118,26 +170,30 @@ static void gen10_sseu_info_init(struct drm_i915_private *dev_priv)
 static void cherryview_sseu_info_init(struct drm_i915_private *dev_priv)
 {
 	struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
-	u32 fuse, eu_dis;
+	u32 fuse;
 
 	fuse = I915_READ(CHV_FUSE_GT);
 
 	sseu->slice_mask = BIT(0);
+	sseu->max_slices = 1;
+	sseu->max_subslices = 2;
+	sseu->max_eus_per_subslice = 8;
 
 	if (!(fuse & CHV_FGT_DISABLE_SS0)) {
-		sseu->subslice_mask |= BIT(0);
-		eu_dis = fuse & (CHV_FGT_EU_DIS_SS0_R0_MASK |
-				 CHV_FGT_EU_DIS_SS0_R1_MASK);
-		sseu->eu_total += 8 - hweight32(eu_dis);
+		sseu->subslices_mask[0] |= BIT(0);
+		sseu->eu_mask[0] = (fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
+		sseu->eu_mask[0] |= ((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;
+		sseu->subslices_mask[0] = 1;
 	}
 
 	if (!(fuse & CHV_FGT_DISABLE_SS1)) {
-		sseu->subslice_mask |= BIT(1);
-		eu_dis = fuse & (CHV_FGT_EU_DIS_SS1_R0_MASK |
-				 CHV_FGT_EU_DIS_SS1_R1_MASK);
-		sseu->eu_total += 8 - hweight32(eu_dis);
+		sseu->subslices_mask[0] |= BIT(1);
+		sseu->eu_mask[1] = (fuse & CHV_FGT_EU_DIS_SS1_R0_MASK) >> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
+		sseu->eu_mask[2] |= ((fuse & CHV_FGT_EU_DIS_SS1_R1_MASK) >> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;
 	}
 
+	sseu->eu_total = compute_eu_total(sseu);
+
 	/*
 	 * CHV expected to always have a uniform distribution of EU
 	 * across subslices.
@@ -159,41 +215,50 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_device_info *info = mkwrite_device_info(dev_priv);
 	struct sseu_dev_info *sseu = &info->sseu;
-	int s_max = 3, ss_max = 4, eu_max = 8;
 	int s, ss;
-	u32 fuse2, eu_disable;
+	u32 fuse2, eu_disable, subslice_mask;
 	u8 eu_mask = 0xff;
 
 	fuse2 = I915_READ(GEN8_FUSE2);
 	sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
 
+	/* BXT has a single slice and at most 3 subslices. */
+	sseu->max_slices = IS_GEN9_LP(dev_priv) ? 1 : 3;
+	sseu->max_subslices = IS_GEN9_LP(dev_priv) ? 3 : 4;
+	sseu->max_eus_per_subslice = 8;
+
 	/*
 	 * The subslice disable field is global, i.e. it applies
 	 * to each of the enabled slices.
 	*/
-	sseu->subslice_mask = (1 << ss_max) - 1;
-	sseu->subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
-				 GEN9_F2_SS_DIS_SHIFT);
+	subslice_mask = (1 << sseu->max_subslices) - 1;
+	subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
+			   GEN9_F2_SS_DIS_SHIFT);
 
 	/*
 	 * Iterate through enabled slices and subslices to
 	 * count the total enabled EU.
 	*/
-	for (s = 0; s < s_max; s++) {
+	for (s = 0; s < sseu->max_slices; s++) {
 		if (!(sseu->slice_mask & BIT(s)))
 			/* skip disabled slice */
 			continue;
 
+		sseu->subslices_mask[s] = subslice_mask;
+
 		eu_disable = I915_READ(GEN9_EU_DISABLE(s));
-		for (ss = 0; ss < ss_max; ss++) {
+		for (ss = 0; ss < sseu->max_subslices; ss++) {
 			int eu_per_ss;
 
-			if (!(sseu->subslice_mask & BIT(ss)))
+			if (!(sseu->subslices_mask[s] & BIT(ss)))
 				/* skip disabled subslice */
 				continue;
 
-			eu_per_ss = eu_max - hweight8((eu_disable >> (ss*8)) &
-						      eu_mask);
+			sseu->eu_mask[ss + s * sseu->max_subslices] =
+				~((eu_disable >> (ss*8)) & eu_mask);
+
+			eu_per_ss = sseu->max_eus_per_subslice -
+				hweight8((eu_disable >> (ss*8)) & eu_mask);
 
 			/*
 			 * Record which subslice(s) has(have) 7 EUs. we
@@ -202,11 +267,11 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
 			 */
 			if (eu_per_ss == 7)
 				sseu->subslice_7eu[s] |= BIT(ss);
-
-			sseu->eu_total += eu_per_ss;
 		}
 	}
 
+	sseu->eu_total = compute_eu_total(sseu);
+
 	/*
 	 * SKL is expected to always have a uniform distribution
 	 * of EU across subslices with the exception that any one
@@ -232,8 +297,8 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
 	sseu->has_eu_pg = sseu->eu_per_subslice > 2;
 
 	if (IS_GEN9_LP(dev_priv)) {
-#define IS_SS_DISABLED(ss)	(!(sseu->subslice_mask & BIT(ss)))
-		info->has_pooled_eu = hweight8(sseu->subslice_mask) == 3;
+#define IS_SS_DISABLED(ss)	(!(sseu->subslices_mask[0] & BIT(ss)))
+		info->has_pooled_eu = hweight8(sseu->subslices_mask[0]) == 3;
 
 		sseu->min_eu_in_pool = 0;
 		if (info->has_pooled_eu) {
@@ -251,19 +316,22 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
 static void broadwell_sseu_info_init(struct drm_i915_private *dev_priv)
 {
 	struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
-	const int s_max = 3, ss_max = 3, eu_max = 8;
 	int s, ss;
-	u32 fuse2, eu_disable[3]; /* s_max */
+	u32 fuse2, subslice_mask, eu_disable[3]; /* s_max */
 
 	fuse2 = I915_READ(GEN8_FUSE2);
 	sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
+	sseu->max_slices = 3;
+	sseu->max_subslices = 3;
+	sseu->max_eus_per_subslice = 8;
+
 	/*
 	 * The subslice disable field is global, i.e. it applies
 	 * to each of the enabled slices.
 	 */
-	sseu->subslice_mask = GENMASK(ss_max - 1, 0);
-	sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
-				 GEN8_F2_SS_DIS_SHIFT);
+	subslice_mask = GENMASK(sseu->max_subslices - 1, 0);
+	subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
+			   GEN8_F2_SS_DIS_SHIFT);
 
 	eu_disable[0] = I915_READ(GEN8_EU_DISABLE0) & GEN8_EU_DIS0_S0_MASK;
 	eu_disable[1] = (I915_READ(GEN8_EU_DISABLE0) >> GEN8_EU_DIS0_S1_SHIFT) |
@@ -277,30 +345,36 @@ static void broadwell_sseu_info_init(struct drm_i915_private *dev_priv)
 	 * Iterate through enabled slices and subslices to
 	 * count the total enabled EU.
 	 */
-	for (s = 0; s < s_max; s++) {
+	for (s = 0; s < sseu->max_slices; s++) {
 		if (!(sseu->slice_mask & BIT(s)))
 			/* skip disabled slice */
 			continue;
 
-		for (ss = 0; ss < ss_max; ss++) {
+		sseu->subslices_mask[s] = subslice_mask;
+
+		for (ss = 0; ss < sseu->max_subslices; ss++) {
 			u32 n_disabled;
 
-			if (!(sseu->subslice_mask & BIT(ss)))
+			if (!(sseu->subslices_mask[ss] & BIT(ss)))
 				/* skip disabled subslice */
 				continue;
 
-			n_disabled = hweight8(eu_disable[s] >> (ss * eu_max));
+			sseu->eu_mask[ss + s * sseu->max_subslices] =
+				~(eu_disable[s] >>
+				  (ss * sseu->max_eus_per_subslice));
+			n_disabled = hweight8(eu_disable[s] >>
+					      (ss * sseu->max_eus_per_subslice));
 
 			/*
 			 * Record which subslices have 7 EUs.
 			 */
-			if (eu_max - n_disabled == 7)
+			if (sseu->max_eus_per_subslice - n_disabled == 7)
 				sseu->subslice_7eu[s] |= 1 << ss;
-
-			sseu->eu_total += eu_max - n_disabled;
 		}
 	}
 
+	sseu->eu_total = compute_eu_total(sseu);
+
 	/*
 	 * BDW is expected to always have a uniform distribution of EU across
 	 * subslices with the exception that any one EU in any one subslice may
@@ -437,6 +511,7 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_device_info *info = mkwrite_device_info(dev_priv);
 	enum pipe pipe;
+	int s;
 
 	if (INTEL_GEN(dev_priv) >= 10) {
 		for_each_pipe(dev_priv, pipe)
@@ -548,9 +623,11 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_DRIVER("slice total: %u\n", hweight8(info->sseu.slice_mask));
 	DRM_DEBUG_DRIVER("subslice total: %u\n",
 			 sseu_subslice_total(&info->sseu));
-	DRM_DEBUG_DRIVER("subslice mask %04x\n", info->sseu.subslice_mask);
-	DRM_DEBUG_DRIVER("subslice per slice: %u\n",
-			 hweight8(info->sseu.subslice_mask));
+	for (s = 0; s < ARRAY_SIZE(info->sseu.subslices_mask); s++) {
+		DRM_DEBUG_DRIVER("subslice mask %04x\n", info->sseu.subslices_mask[s]);
+		DRM_DEBUG_DRIVER("subslice per slice: %u\n",
+				 hweight8(info->sseu.subslices_mask[s]));
+	}
 	DRM_DEBUG_DRIVER("EU total: %u\n", info->sseu.eu_total);
 	DRM_DEBUG_DRIVER("EU per subslice: %u\n", info->sseu.eu_per_subslice);
 	DRM_DEBUG_DRIVER("has slice power gating: %s\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2e38fbfdf08f..6e55b842c21d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2072,7 +2072,7 @@ make_rpcs(struct drm_i915_private *dev_priv)
 
 	if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
 		rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
-		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask) <<
+		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslices_mask[0]) <<
 			GEN8_RPCS_SS_CNT_SHIFT;
 		rpcs |= GEN8_RPCS_ENABLE;
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c5ff203e42d6..a94bc1b5c502 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -90,7 +90,7 @@ hangcheck_action_to_str(const enum intel_engine_hangcheck_action a)
 
 #define instdone_subslice_mask(dev_priv__) \
 	(INTEL_GEN(dev_priv__) == 7 ? \
-	 1 : INTEL_INFO(dev_priv__)->sseu.subslice_mask)
+	 1 : INTEL_INFO(dev_priv__)->sseu.subslices_mask[0])
 
 #define for_each_instdone_slice_subslice(dev_priv__, slice__, subslice__) \
 	for ((slice__) = 0, (subslice__) = 0; \
-- 
2.15.1

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

Re: [PATCH 1/6] drm/i915: store all subslice masks

[Tvrtko Ursulin]

On 18/12/2017 15:35, Lionel Landwerlin wrote:
> Up to now, subslice mask was assumed to be uniform across slices. But
> starting with Cannonlake, slices can be asymetric (for example slice0

asymmetric, thanks auto spell checker. :)

> has different number of subslices as slice1+). This change stores all
> subslices masks for all slices rather than having a single mask that
> applies to all slices.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c      |  24 +++--
>   drivers/gpu/drm/i915/i915_drv.c          |   2 +-
>   drivers/gpu/drm/i915/i915_drv.h          |  23 ++++-
>   drivers/gpu/drm/i915/intel_device_info.c | 169 ++++++++++++++++++++++---------
>   drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
>   drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
>   6 files changed, 161 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0ddce72552bf..0c7890b695c5 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4293,7 +4293,7 @@ static void cherryview_sseu_device_status(struct drm_i915_private *dev_priv,
>   			continue;
>   
>   		sseu->slice_mask = BIT(0);
> -		sseu->subslice_mask |= BIT(ss);
> +		sseu->subslices_mask[0] |= BIT(ss);
>   		eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) +
>   			 ((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) +
>   			 ((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) +
> @@ -4340,7 +4340,7 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
>   			continue;
>   
>   		sseu->slice_mask |= BIT(s);
> -		sseu->subslice_mask = info->sseu.subslice_mask;
> +		sseu->subslices_mask[s] = info->sseu.subslices_mask[s];
>   
>   		for (ss = 0; ss < ss_max; ss++) {
>   			unsigned int eu_cnt;
> @@ -4395,8 +4395,8 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
>   		sseu->slice_mask |= BIT(s);
>   
>   		if (IS_GEN9_BC(dev_priv))
> -			sseu->subslice_mask =
> -				INTEL_INFO(dev_priv)->sseu.subslice_mask;
> +			sseu->subslices_mask[s] =
> +				INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
>   
>   		for (ss = 0; ss < ss_max; ss++) {
>   			unsigned int eu_cnt;
> @@ -4406,7 +4406,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
>   					/* skip disabled subslice */
>   					continue;
>   
> -				sseu->subslice_mask |= BIT(ss);
> +				sseu->subslices_mask[s] |= BIT(ss);
>   			}
>   
>   			eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
> @@ -4428,9 +4428,12 @@ static void broadwell_sseu_device_status(struct drm_i915_private *dev_priv,
>   	sseu->slice_mask = slice_info & GEN8_LSLICESTAT_MASK;
>   
>   	if (sseu->slice_mask) {
> -		sseu->subslice_mask = INTEL_INFO(dev_priv)->sseu.subslice_mask;
>   		sseu->eu_per_subslice =
>   				INTEL_INFO(dev_priv)->sseu.eu_per_subslice;
> +		for (s = 0; s < fls(sseu->slice_mask); s++) {
> +			sseu->subslices_mask[s] =
> +				INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
> +		}
>   		sseu->eu_total = sseu->eu_per_subslice *
>   				 sseu_subslice_total(sseu);
>   
> @@ -4449,6 +4452,7 @@ static void i915_print_sseu_info(struct seq_file *m, bool is_available_info,
>   {
>   	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>   	const char *type = is_available_info ? "Available" : "Enabled";
> +	int s;
>   
>   	seq_printf(m, "  %s Slice Mask: %04x\n", type,
>   		   sseu->slice_mask);
> @@ -4456,10 +4460,10 @@ static void i915_print_sseu_info(struct seq_file *m, bool is_available_info,
>   		   hweight8(sseu->slice_mask));
>   	seq_printf(m, "  %s Subslice Total: %u\n", type,
>   		   sseu_subslice_total(sseu));
> -	seq_printf(m, "  %s Subslice Mask: %04x\n", type,
> -		   sseu->subslice_mask);
> -	seq_printf(m, "  %s Subslice Per Slice: %u\n", type,
> -		   hweight8(sseu->subslice_mask));
> +	for (s = 0; s < fls(sseu->slice_mask); s++) {

Slice mask is always contiguous bits?

> +		seq_printf(m, "  %s Slice%i Subslice Mask: %04x\n", type,
> +			   s, sseu->subslices_mask[s]);

Don't want to keep printing the count? Like maybe " %s Slice%u %u 
sublices, mask=%04x\n" ?

> +	}
>   	seq_printf(m, "  %s EU Total: %u\n", type,
>   		   sseu->eu_total);
>   	seq_printf(m, "  %s EU Per Subslice: %u\n", type,
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 72bea281edb7..8b99e415c345 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -414,7 +414,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
>   			return -ENODEV;
>   		break;
>   	case I915_PARAM_SUBSLICE_MASK:
> -		value = INTEL_INFO(dev_priv)->sseu.subslice_mask;
> +		value = INTEL_INFO(dev_priv)->sseu.subslices_mask[0];
>   		if (!value)
>   			return -ENODEV;
>   		break;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1aba5657f5f0..82fc59078c6a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -802,9 +802,12 @@ struct intel_csr {
>   	func(supports_tv); \
>   	func(has_ipc);
>   
> +#define GEN_MAX_SLICES		(6) /* CNL upper bound */
> +#define GEN_MAX_SUBSLICES	(7)
> +
>   struct sseu_dev_info {
>   	u8 slice_mask;
> -	u8 subslice_mask;
> +	u8 subslices_mask[GEN_MAX_SLICES];

Personally I would probably kept the subslice_mask name and just turned 
it into an array, but that is bike-shed territory.

>   	u8 eu_total;
>   	u8 eu_per_subslice;
>   	u8 min_eu_in_pool;
> @@ -813,11 +816,27 @@ struct sseu_dev_info {
>   	u8 has_slice_pg:1;
>   	u8 has_subslice_pg:1;
>   	u8 has_eu_pg:1;
> +
> +	/* Topology fields */
> +	u8 max_slices;
> +	u8 max_subslices;
> +	u8 max_eus_per_subslice;
> +
> +	/* We don't have more than 8 eus per subslice at the moment and as we
> +	 * store eus enabled using bits, no need to multiply by eus per
> +	 * subslice.
> +	 */
> +	u8 eu_mask[GEN_MAX_SLICES * GEN_MAX_SUBSLICES];
>   };
>   
>   static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>   {
> -	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
> +	unsigned s, total = 0;
> +
> +	for (s = 0; s < ARRAY_SIZE(sseu->subslices_mask); s++)
> +		total += hweight8(sseu->subslices_mask[s]);
> +
> +	return total;

The function is bigger now so maybe it needs stops being inline, but 
either moved to the .c file, or even the total stored in struct 
sseu_dev_info, and then this becomes just a getter?

>   }
>   
>   /* Keep in gen based order, and chronological order within a gen */
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index f478be3ae0ba..6a3c40439e83 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -82,22 +82,74 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv)
>   #undef PRINT_FLAG
>   }
>   
> +static u8 compute_eu_total(const struct sseu_dev_info *sseu)
> +{
> +	u8 i, total = 0;

sseu_subslice_total returns an unsinged int, while this is u8, 
suggesting there can be more sublices and eus. ;) Jokes aside, I'd 
probably make them consistent. The index variable is probably best as 
unsigned int, but the total and return can be u8 if big enough for total 
eus? Either way best it matches the types in the struct they get 
assigned to.

> +
> +	for (i = 0; i < ARRAY_SIZE(sseu->eu_mask); i++)
> +		total += hweight8(sseu->eu_mask[i]);
> +
> +	return total;
> +}
> +
>   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);
> +	int s, ss, eu_mask = 0xff;

const eu_mask ?

> +	u32 subslice_mask, eu_en;
>   
>   	sseu->slice_mask = (fuse2 & GEN10_F2_S_ENA_MASK) >>
>   			    GEN10_F2_S_ENA_SHIFT;
> -	sseu->subslice_mask = (1 << 4) - 1;
> -	sseu->subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
> -				 GEN10_F2_SS_DIS_SHIFT);
> +	sseu->max_slices = 6;
> +	sseu->max_subslices = 4;
> +	sseu->max_eus_per_subslice = 8;
> +
> +	subslice_mask = (1 << 4) - 1;
> +	subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
> +			   GEN10_F2_SS_DIS_SHIFT);
> +
> +	/* Slice0 can have up to 3 subslices, but there are only 2 in
> +	 * slice1/2.
> +	 */
> +	sseu->subslices_mask[0] = subslice_mask;
> +	for (s = 1; s < sseu->max_slices; s++)
> +		sseu->subslices_mask[s] = subslice_mask & 0x3;
> +
> +	/* Slice0 */
> +	eu_en = ~I915_READ(GEN8_EU_DISABLE0);
> +	for (ss = 0; ss < sseu->max_subslices; ss++)
> +		sseu->eu_mask[ss]     = (eu_en >> (8 * ss)) & eu_mask;
> +	/* Slice1 */
> +	sseu->eu_mask[sseu->max_subslices]         = (eu_en >> 24) & eu_mask;
> +	eu_en = ~I915_READ(GEN8_EU_DISABLE1);
> +	sseu->eu_mask[sseu->max_subslices + 1]     = eu_en & eu_mask;

I suggest a helper to index into sse->eu_mask, like 
sseu->eu_mask[_eu_mask_idx(slice, subslice)] or something, so it is more 
readable what is happening here. Or even:

  _eu_mask(sseu, slice, subslice) = mask;

Doable? I am not 100% I picked up exactly on the layout of the eu_mask 
array.. each element is one subslice? Consecutive for slice0, 
subslice0..N, slice1... sliceN ?

> +	/* Slice2 */
> +	sseu->eu_mask[2 * sseu->max_subslices]     = (eu_en >> 8) & eu_mask;
> +	sseu->eu_mask[2 * sseu->max_subslices + 1] = (eu_en >> 16) & eu_mask;
> +	/* Slice3 */
> +	sseu->eu_mask[3 * sseu->max_subslices]     = (eu_en >> 24) & eu_mask;
> +	eu_en = ~I915_READ(GEN8_EU_DISABLE2);
> +	sseu->eu_mask[3 * sseu->max_subslices + 1] = eu_en & eu_mask;
> +	/* Slice4 */
> +	sseu->eu_mask[4 * sseu->max_subslices]     = (eu_en >> 8) & eu_mask;
> +	sseu->eu_mask[4 * sseu->max_subslices + 1] = (eu_en >> 16) & eu_mask;
> +	/* Slice5 */
> +	sseu->eu_mask[5 * sseu->max_subslices]     = (eu_en >> 24) & eu_mask;
> +	eu_en = ~I915_READ(GEN10_EU_DISABLE3);
> +	sseu->eu_mask[5 * sseu->max_subslices + 1] = eu_en & eu_mask;
> +
> +	/* Do a second pass where we marked the subslices disabled if all
> +	 * their eus are off.
> +	 */
> +	for (s = 0; s < sseu->max_slices; s++) {
> +		for (ss = 0; ss < sseu->max_subslices; ss++) {
> +			if (sseu->eu_mask[s * sseu->max_subslices + ss] == 0)
> +				sseu->subslices_mask[s] &= ~BIT(ss);
> +		}
> +	}
>   
> -	sseu->eu_total = hweight32(~I915_READ(GEN8_EU_DISABLE0));
> -	sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE1));
> -	sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE2));
> -	sseu->eu_total += hweight8(~(I915_READ(GEN10_EU_DISABLE3) &
> -				     GEN10_EU_DIS_SS_MASK));
> +	sseu->eu_total = compute_eu_total(sseu);
>   
>   	/*
>   	 * CNL is expected to always have a uniform distribution
> @@ -118,26 +170,30 @@ static void gen10_sseu_info_init(struct drm_i915_private *dev_priv)
>   static void cherryview_sseu_info_init(struct drm_i915_private *dev_priv)
>   {
>   	struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
> -	u32 fuse, eu_dis;
> +	u32 fuse;
>   
>   	fuse = I915_READ(CHV_FUSE_GT);
>   
>   	sseu->slice_mask = BIT(0);
> +	sseu->max_slices = 1;
> +	sseu->max_subslices = 2;
> +	sseu->max_eus_per_subslice = 8;
>   
>   	if (!(fuse & CHV_FGT_DISABLE_SS0)) {
> -		sseu->subslice_mask |= BIT(0);
> -		eu_dis = fuse & (CHV_FGT_EU_DIS_SS0_R0_MASK |
> -				 CHV_FGT_EU_DIS_SS0_R1_MASK);
> -		sseu->eu_total += 8 - hweight32(eu_dis);
> +		sseu->subslices_mask[0] |= BIT(0);
> +		sseu->eu_mask[0] = (fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
> +		sseu->eu_mask[0] |= ((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;

Do you need to invert the bits here?

> +		sseu->subslices_mask[0] = 1;
>   	}
>   
>   	if (!(fuse & CHV_FGT_DISABLE_SS1)) {
> -		sseu->subslice_mask |= BIT(1);
> -		eu_dis = fuse & (CHV_FGT_EU_DIS_SS1_R0_MASK |
> -				 CHV_FGT_EU_DIS_SS1_R1_MASK);
> -		sseu->eu_total += 8 - hweight32(eu_dis);
> +		sseu->subslices_mask[0] |= BIT(1);
> +		sseu->eu_mask[1] = (fuse & CHV_FGT_EU_DIS_SS1_R0_MASK) >> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
> +		sseu->eu_mask[2] |= ((fuse & CHV_FGT_EU_DIS_SS1_R1_MASK) >> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;

Why eu_mask indices 1 and 2 here, while for subslice 0 you merged them 
into index 0?

>   	}
>   
> +	sseu->eu_total = compute_eu_total(sseu);
> +
>   	/*
>   	 * CHV expected to always have a uniform distribution of EU
>   	 * across subslices.
> @@ -159,41 +215,50 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_device_info *info = mkwrite_device_info(dev_priv);
>   	struct sseu_dev_info *sseu = &info->sseu;
> -	int s_max = 3, ss_max = 4, eu_max = 8;
>   	int s, ss;
> -	u32 fuse2, eu_disable;
> +	u32 fuse2, eu_disable, subslice_mask;
>   	u8 eu_mask = 0xff;
>   
>   	fuse2 = I915_READ(GEN8_FUSE2);
>   	sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
>   
> +	/* BXT has a single slice and at most 3 subslices. */
> +	sseu->max_slices = IS_GEN9_LP(dev_priv) ? 1 : 3;
> +	sseu->max_subslices = IS_GEN9_LP(dev_priv) ? 3 : 4;
> +	sseu->max_eus_per_subslice = 8;
> +
>   	/*
>   	 * The subslice disable field is global, i.e. it applies
>   	 * to each of the enabled slices.
>   	*/
> -	sseu->subslice_mask = (1 << ss_max) - 1;
> -	sseu->subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
> -				 GEN9_F2_SS_DIS_SHIFT);
> +	subslice_mask = (1 << sseu->max_subslices) - 1;
> +	subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
> +			   GEN9_F2_SS_DIS_SHIFT);
>   
>   	/*
>   	 * Iterate through enabled slices and subslices to
>   	 * count the total enabled EU.
>   	*/
> -	for (s = 0; s < s_max; s++) {
> +	for (s = 0; s < sseu->max_slices; s++) {
>   		if (!(sseu->slice_mask & BIT(s)))
>   			/* skip disabled slice */
>   			continue;
>   
> +		sseu->subslices_mask[s] = subslice_mask;
> +
>   		eu_disable = I915_READ(GEN9_EU_DISABLE(s));
> -		for (ss = 0; ss < ss_max; ss++) {
> +		for (ss = 0; ss < sseu->max_subslices; ss++) {
>   			int eu_per_ss;
>   
> -			if (!(sseu->subslice_mask & BIT(ss)))
> +			if (!(sseu->subslices_mask[s] & BIT(ss)))
>   				/* skip disabled subslice */
>   				continue;
>   
> -			eu_per_ss = eu_max - hweight8((eu_disable >> (ss*8)) &
> -						      eu_mask);
> +			sseu->eu_mask[ss + s * sseu->max_subslices] =
> +				~((eu_disable >> (ss*8)) & eu_mask);
> +
> +			eu_per_ss = sseu->max_eus_per_subslice -
> +				hweight8((eu_disable >> (ss*8)) & eu_mask);

Store "eu_disable >> (ss*8)) & eu_mask" in a local for readability since 
it is calculated twice?

>   
>   			/*
>   			 * Record which subslice(s) has(have) 7 EUs. we
> @@ -202,11 +267,11 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
>   			 */
>   			if (eu_per_ss == 7)
>   				sseu->subslice_7eu[s] |= BIT(ss);
> -
> -			sseu->eu_total += eu_per_ss;

You could theoretically keep this running total and then at the end 
compare against compute_eu_total under a GEM_BUG_ON, just to check 
everyting is working as expected.

>   		}
>   	}
>   
> +	sseu->eu_total = compute_eu_total(sseu);
> +
>   	/*
>   	 * SKL is expected to always have a uniform distribution
>   	 * of EU across subslices with the exception that any one
> @@ -232,8 +297,8 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
>   	sseu->has_eu_pg = sseu->eu_per_subslice > 2;
>   
>   	if (IS_GEN9_LP(dev_priv)) {
> -#define IS_SS_DISABLED(ss)	(!(sseu->subslice_mask & BIT(ss)))
> -		info->has_pooled_eu = hweight8(sseu->subslice_mask) == 3;
> +#define IS_SS_DISABLED(ss)	(!(sseu->subslices_mask[0] & BIT(ss)))
> +		info->has_pooled_eu = hweight8(sseu->subslices_mask[0]) == 3;
>   
>   		sseu->min_eu_in_pool = 0;
>   		if (info->has_pooled_eu) {
> @@ -251,19 +316,22 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
>   static void broadwell_sseu_info_init(struct drm_i915_private *dev_priv)
>   {
>   	struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
> -	const int s_max = 3, ss_max = 3, eu_max = 8;
>   	int s, ss;
> -	u32 fuse2, eu_disable[3]; /* s_max */
> +	u32 fuse2, subslice_mask, eu_disable[3]; /* s_max */
>   
>   	fuse2 = I915_READ(GEN8_FUSE2);
>   	sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
> +	sseu->max_slices = 3;
> +	sseu->max_subslices = 3;
> +	sseu->max_eus_per_subslice = 8;
> +
>   	/*
>   	 * The subslice disable field is global, i.e. it applies
>   	 * to each of the enabled slices.
>   	 */
> -	sseu->subslice_mask = GENMASK(ss_max - 1, 0);
> -	sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
> -				 GEN8_F2_SS_DIS_SHIFT);
> +	subslice_mask = GENMASK(sseu->max_subslices - 1, 0);
> +	subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
> +			   GEN8_F2_SS_DIS_SHIFT);
>   
>   	eu_disable[0] = I915_READ(GEN8_EU_DISABLE0) & GEN8_EU_DIS0_S0_MASK;
>   	eu_disable[1] = (I915_READ(GEN8_EU_DISABLE0) >> GEN8_EU_DIS0_S1_SHIFT) |
> @@ -277,30 +345,36 @@ static void broadwell_sseu_info_init(struct drm_i915_private *dev_priv)
>   	 * Iterate through enabled slices and subslices to
>   	 * count the total enabled EU.
>   	 */
> -	for (s = 0; s < s_max; s++) {
> +	for (s = 0; s < sseu->max_slices; s++) {
>   		if (!(sseu->slice_mask & BIT(s)))
>   			/* skip disabled slice */
>   			continue;
>   
> -		for (ss = 0; ss < ss_max; ss++) {
> +		sseu->subslices_mask[s] = subslice_mask;
> +
> +		for (ss = 0; ss < sseu->max_subslices; ss++) {
>   			u32 n_disabled;
>   
> -			if (!(sseu->subslice_mask & BIT(ss)))
> +			if (!(sseu->subslices_mask[ss] & BIT(ss)))
>   				/* skip disabled subslice */
>   				continue;
>   
> -			n_disabled = hweight8(eu_disable[s] >> (ss * eu_max));
> +			sseu->eu_mask[ss + s * sseu->max_subslices] =
> +				~(eu_disable[s] >>
> +				  (ss * sseu->max_eus_per_subslice));
> +			n_disabled = hweight8(eu_disable[s] >>
> +					      (ss * sseu->max_eus_per_subslice));

As above could use a local for "eu_disable[s] >> (ss * 
sseu->max_eus_per_subslice)"

>   
>   			/*
>   			 * Record which subslices have 7 EUs.
>   			 */
> -			if (eu_max - n_disabled == 7)
> +			if (sseu->max_eus_per_subslice - n_disabled == 7)
>   				sseu->subslice_7eu[s] |= 1 << ss;
> -
> -			sseu->eu_total += eu_max - n_disabled;
>   		}
>   	}
>   
> +	sseu->eu_total = compute_eu_total(sseu);
> +
>   	/*
>   	 * BDW is expected to always have a uniform distribution of EU across
>   	 * subslices with the exception that any one EU in any one subslice may
> @@ -437,6 +511,7 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_device_info *info = mkwrite_device_info(dev_priv);
>   	enum pipe pipe;
> +	int s;
>   
>   	if (INTEL_GEN(dev_priv) >= 10) {
>   		for_each_pipe(dev_priv, pipe)
> @@ -548,9 +623,11 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
>   	DRM_DEBUG_DRIVER("slice total: %u\n", hweight8(info->sseu.slice_mask));
>   	DRM_DEBUG_DRIVER("subslice total: %u\n",
>   			 sseu_subslice_total(&info->sseu));
> -	DRM_DEBUG_DRIVER("subslice mask %04x\n", info->sseu.subslice_mask);
> -	DRM_DEBUG_DRIVER("subslice per slice: %u\n",
> -			 hweight8(info->sseu.subslice_mask));
> +	for (s = 0; s < ARRAY_SIZE(info->sseu.subslices_mask); s++) {
> +		DRM_DEBUG_DRIVER("subslice mask %04x\n", info->sseu.subslices_mask[s]);
> +		DRM_DEBUG_DRIVER("subslice per slice: %u\n",
> +				 hweight8(info->sseu.subslices_mask[s]));

Put a subslice index into messages?

> +	}
>   	DRM_DEBUG_DRIVER("EU total: %u\n", info->sseu.eu_total);
>   	DRM_DEBUG_DRIVER("EU per subslice: %u\n", info->sseu.eu_per_subslice);
>   	DRM_DEBUG_DRIVER("has slice power gating: %s\n",
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 2e38fbfdf08f..6e55b842c21d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2072,7 +2072,7 @@ make_rpcs(struct drm_i915_private *dev_priv)
>   
>   	if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>   		rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
> -		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask) <<
> +		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslices_mask[0]) <<
>   			GEN8_RPCS_SS_CNT_SHIFT;
>   		rpcs |= GEN8_RPCS_ENABLE;
>   	}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c5ff203e42d6..a94bc1b5c502 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -90,7 +90,7 @@ hangcheck_action_to_str(const enum intel_engine_hangcheck_action a)
>   
>   #define instdone_subslice_mask(dev_priv__) \
>   	(INTEL_GEN(dev_priv__) == 7 ? \
> -	 1 : INTEL_INFO(dev_priv__)->sseu.subslice_mask)
> +	 1 : INTEL_INFO(dev_priv__)->sseu.subslices_mask[0])
>   
>   #define for_each_instdone_slice_subslice(dev_priv__, slice__, subslice__) \
>   	for ((slice__) = 0, (subslice__) = 0; \
> 

Regards,

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

Re: [PATCH 1/6] drm/i915: store all subslice masks

[Lionel Landwerlin]

On 11/01/18 11:12, Tvrtko Ursulin wrote:
>
> On 18/12/2017 15:35, Lionel Landwerlin wrote:
>> Up to now, subslice mask was assumed to be uniform across slices. But
>> starting with Cannonlake, slices can be asymetric (for example slice0
>
> asymmetric, thanks auto spell checker. :)

Done, thanks.

>
>> has different number of subslices as slice1+). This change stores all
>> subslices masks for all slices rather than having a single mask that
>> applies to all slices.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c      |  24 +++--
>>   drivers/gpu/drm/i915/i915_drv.c          |   2 +-
>>   drivers/gpu/drm/i915/i915_drv.h          |  23 ++++-
>>   drivers/gpu/drm/i915/intel_device_info.c | 169 
>> ++++++++++++++++++++++---------
>>   drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
>>   drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
>>   6 files changed, 161 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 0ddce72552bf..0c7890b695c5 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -4293,7 +4293,7 @@ static void 
>> cherryview_sseu_device_status(struct drm_i915_private *dev_priv,
>>               continue;
>>             sseu->slice_mask = BIT(0);
>> -        sseu->subslice_mask |= BIT(ss);
>> +        sseu->subslices_mask[0] |= BIT(ss);
>>           eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) +
>>                ((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) +
>>                ((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) +
>> @@ -4340,7 +4340,7 @@ static void gen10_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>               continue;
>>             sseu->slice_mask |= BIT(s);
>> -        sseu->subslice_mask = info->sseu.subslice_mask;
>> +        sseu->subslices_mask[s] = info->sseu.subslices_mask[s];
>>             for (ss = 0; ss < ss_max; ss++) {
>>               unsigned int eu_cnt;
>> @@ -4395,8 +4395,8 @@ static void gen9_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>           sseu->slice_mask |= BIT(s);
>>             if (IS_GEN9_BC(dev_priv))
>> -            sseu->subslice_mask =
>> -                INTEL_INFO(dev_priv)->sseu.subslice_mask;
>> +            sseu->subslices_mask[s] =
>> + INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
>>             for (ss = 0; ss < ss_max; ss++) {
>>               unsigned int eu_cnt;
>> @@ -4406,7 +4406,7 @@ static void gen9_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>                       /* skip disabled subslice */
>>                       continue;
>>   -                sseu->subslice_mask |= BIT(ss);
>> +                sseu->subslices_mask[s] |= BIT(ss);
>>               }
>>                 eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
>> @@ -4428,9 +4428,12 @@ static void 
>> broadwell_sseu_device_status(struct drm_i915_private *dev_priv,
>>       sseu->slice_mask = slice_info & GEN8_LSLICESTAT_MASK;
>>         if (sseu->slice_mask) {
>> -        sseu->subslice_mask = INTEL_INFO(dev_priv)->sseu.subslice_mask;
>>           sseu->eu_per_subslice =
>>                   INTEL_INFO(dev_priv)->sseu.eu_per_subslice;
>> +        for (s = 0; s < fls(sseu->slice_mask); s++) {
>> +            sseu->subslices_mask[s] =
>> + INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
>> +        }
>>           sseu->eu_total = sseu->eu_per_subslice *
>>                    sseu_subslice_total(sseu);
>>   @@ -4449,6 +4452,7 @@ static void i915_print_sseu_info(struct 
>> seq_file *m, bool is_available_info,
>>   {
>>       struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>       const char *type = is_available_info ? "Available" : "Enabled";
>> +    int s;
>>         seq_printf(m, "  %s Slice Mask: %04x\n", type,
>>              sseu->slice_mask);
>> @@ -4456,10 +4460,10 @@ static void i915_print_sseu_info(struct 
>> seq_file *m, bool is_available_info,
>>              hweight8(sseu->slice_mask));
>>       seq_printf(m, "  %s Subslice Total: %u\n", type,
>>              sseu_subslice_total(sseu));
>> -    seq_printf(m, "  %s Subslice Mask: %04x\n", type,
>> -           sseu->subslice_mask);
>> -    seq_printf(m, "  %s Subslice Per Slice: %u\n", type,
>> -           hweight8(sseu->subslice_mask));
>> +    for (s = 0; s < fls(sseu->slice_mask); s++) {
>
> Slice mask is always contiguous bits?

I have a 2x6 BXT on my desk where the subslice0 appears to be fused-off.
I assumed same could be true for slices.
fls() should make us iterate through all the slices (even the fused off 
ones).

>
>> +        seq_printf(m, "  %s Slice%i Subslice Mask: %04x\n", type,
>> +               s, sseu->subslices_mask[s]);
>
> Don't want to keep printing the count? Like maybe " %s Slice%u %u 
> sublices, mask=%04x\n" ?

Sure, done.

>
>> +    }
>>       seq_printf(m, "  %s EU Total: %u\n", type,
>>              sseu->eu_total);
>>       seq_printf(m, "  %s EU Per Subslice: %u\n", type,
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 72bea281edb7..8b99e415c345 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -414,7 +414,7 @@ static int i915_getparam(struct drm_device *dev, 
>> void *data,
>>               return -ENODEV;
>>           break;
>>       case I915_PARAM_SUBSLICE_MASK:
>> -        value = INTEL_INFO(dev_priv)->sseu.subslice_mask;
>> +        value = INTEL_INFO(dev_priv)->sseu.subslices_mask[0];
>>           if (!value)
>>               return -ENODEV;
>>           break;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 1aba5657f5f0..82fc59078c6a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -802,9 +802,12 @@ struct intel_csr {
>>       func(supports_tv); \
>>       func(has_ipc);
>>   +#define GEN_MAX_SLICES        (6) /* CNL upper bound */
>> +#define GEN_MAX_SUBSLICES    (7)
>> +
>>   struct sseu_dev_info {
>>       u8 slice_mask;
>> -    u8 subslice_mask;
>> +    u8 subslices_mask[GEN_MAX_SLICES];
>
> Personally I would probably kept the subslice_mask name and just 
> turned it into an array, but that is bike-shed territory.

It was easier to spot all the places to modify. Will go back.

>
>>       u8 eu_total;
>>       u8 eu_per_subslice;
>>       u8 min_eu_in_pool;
>> @@ -813,11 +816,27 @@ struct sseu_dev_info {
>>       u8 has_slice_pg:1;
>>       u8 has_subslice_pg:1;
>>       u8 has_eu_pg:1;
>> +
>> +    /* Topology fields */
>> +    u8 max_slices;
>> +    u8 max_subslices;
>> +    u8 max_eus_per_subslice;
>> +
>> +    /* We don't have more than 8 eus per subslice at the moment and 
>> as we
>> +     * store eus enabled using bits, no need to multiply by eus per
>> +     * subslice.
>> +     */
>> +    u8 eu_mask[GEN_MAX_SLICES * GEN_MAX_SUBSLICES];
>>   };
>>     static inline unsigned int sseu_subslice_total(const struct 
>> sseu_dev_info *sseu)
>>   {
>> -    return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
>> +    unsigned s, total = 0;
>> +
>> +    for (s = 0; s < ARRAY_SIZE(sseu->subslices_mask); s++)
>> +        total += hweight8(sseu->subslices_mask[s]);
>> +
>> +    return total;
>
> The function is bigger now so maybe it needs stops being inline, but 
> either moved to the .c file, or even the total stored in struct 
> sseu_dev_info, and then this becomes just a getter?

Let's go with a subslice_total field.

>
>>   }
>>     /* Keep in gen based order, and chronological order within a gen */
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index f478be3ae0ba..6a3c40439e83 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -82,22 +82,74 @@ void intel_device_info_dump(struct 
>> drm_i915_private *dev_priv)
>>   #undef PRINT_FLAG
>>   }
>>   +static u8 compute_eu_total(const struct sseu_dev_info *sseu)
>> +{
>> +    u8 i, total = 0;
>
> sseu_subslice_total returns an unsinged int, while this is u8, 
> suggesting there can be more sublices and eus. ;) Jokes aside, I'd 
> probably make them consistent. The index variable is probably best as 
> unsigned int, but the total and return can be u8 if big enough for 
> total eus? Either way best it matches the types in the struct they get 
> assigned to.

You're right!
u16 should be fine though.

>
>> +
>> +    for (i = 0; i < ARRAY_SIZE(sseu->eu_mask); i++)
>> +        total += hweight8(sseu->eu_mask[i]);
>> +
>> +    return total;
>> +}
>> +
>>   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);
>> +    int s, ss, eu_mask = 0xff;
>
> const eu_mask ?

Done.

>
>> +    u32 subslice_mask, eu_en;
>>         sseu->slice_mask = (fuse2 & GEN10_F2_S_ENA_MASK) >>
>>                   GEN10_F2_S_ENA_SHIFT;
>> -    sseu->subslice_mask = (1 << 4) - 1;
>> -    sseu->subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
>> -                 GEN10_F2_SS_DIS_SHIFT);
>> +    sseu->max_slices = 6;
>> +    sseu->max_subslices = 4;
>> +    sseu->max_eus_per_subslice = 8;
>> +
>> +    subslice_mask = (1 << 4) - 1;
>> +    subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
>> +               GEN10_F2_SS_DIS_SHIFT);
>> +
>> +    /* Slice0 can have up to 3 subslices, but there are only 2 in
>> +     * slice1/2.
>> +     */
>> +    sseu->subslices_mask[0] = subslice_mask;
>> +    for (s = 1; s < sseu->max_slices; s++)
>> +        sseu->subslices_mask[s] = subslice_mask & 0x3;
>> +
>> +    /* Slice0 */
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE0);
>> +    for (ss = 0; ss < sseu->max_subslices; ss++)
>> +        sseu->eu_mask[ss]     = (eu_en >> (8 * ss)) & eu_mask;
>> +    /* Slice1 */
>> +    sseu->eu_mask[sseu->max_subslices]         = (eu_en >> 24) & 
>> eu_mask;
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE1);
>> +    sseu->eu_mask[sseu->max_subslices + 1]     = eu_en & eu_mask;
>
> I suggest a helper to index into sse->eu_mask, like 
> sseu->eu_mask[_eu_mask_idx(slice, subslice)] or something, so it is 
> more readable what is happening here. Or even:
>
>  _eu_mask(sseu, slice, subslice) = mask;
>
> Doable? I am not 100% I picked up exactly on the layout of the eu_mask 
> array.. each element is one subslice? Consecutive for slice0, 
> subslice0..N, slice1... sliceN ?

Unfortunately that's not possible because of the asymmetry thing...

>
>> +    /* Slice2 */
>> +    sseu->eu_mask[2 * sseu->max_subslices]     = (eu_en >> 8) & 
>> eu_mask;
>> +    sseu->eu_mask[2 * sseu->max_subslices + 1] = (eu_en >> 16) & 
>> eu_mask;
>> +    /* Slice3 */
>> +    sseu->eu_mask[3 * sseu->max_subslices]     = (eu_en >> 24) & 
>> eu_mask;
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE2);
>> +    sseu->eu_mask[3 * sseu->max_subslices + 1] = eu_en & eu_mask;
>> +    /* Slice4 */
>> +    sseu->eu_mask[4 * sseu->max_subslices]     = (eu_en >> 8) & 
>> eu_mask;
>> +    sseu->eu_mask[4 * sseu->max_subslices + 1] = (eu_en >> 16) & 
>> eu_mask;
>> +    /* Slice5 */
>> +    sseu->eu_mask[5 * sseu->max_subslices]     = (eu_en >> 24) & 
>> eu_mask;
>> +    eu_en = ~I915_READ(GEN10_EU_DISABLE3);
>> +    sseu->eu_mask[5 * sseu->max_subslices + 1] = eu_en & eu_mask;
>> +
>> +    /* Do a second pass where we marked the subslices disabled if all
>> +     * their eus are off.
>> +     */
>> +    for (s = 0; s < sseu->max_slices; s++) {
>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>> +            if (sseu->eu_mask[s * sseu->max_subslices + ss] == 0)
>> +                sseu->subslices_mask[s] &= ~BIT(ss);
>> +        }
>> +    }
>>   -    sseu->eu_total = hweight32(~I915_READ(GEN8_EU_DISABLE0));
>> -    sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE1));
>> -    sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE2));
>> -    sseu->eu_total += hweight8(~(I915_READ(GEN10_EU_DISABLE3) &
>> -                     GEN10_EU_DIS_SS_MASK));
>> +    sseu->eu_total = compute_eu_total(sseu);
>>         /*
>>        * CNL is expected to always have a uniform distribution
>> @@ -118,26 +170,30 @@ static void gen10_sseu_info_init(struct 
>> drm_i915_private *dev_priv)
>>   static void cherryview_sseu_info_init(struct drm_i915_private 
>> *dev_priv)
>>   {
>>       struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
>> -    u32 fuse, eu_dis;
>> +    u32 fuse;
>>         fuse = I915_READ(CHV_FUSE_GT);
>>         sseu->slice_mask = BIT(0);
>> +    sseu->max_slices = 1;
>> +    sseu->max_subslices = 2;
>> +    sseu->max_eus_per_subslice = 8;
>>         if (!(fuse & CHV_FGT_DISABLE_SS0)) {
>> -        sseu->subslice_mask |= BIT(0);
>> -        eu_dis = fuse & (CHV_FGT_EU_DIS_SS0_R0_MASK |
>> -                 CHV_FGT_EU_DIS_SS0_R1_MASK);
>> -        sseu->eu_total += 8 - hweight32(eu_dis);
>> +        sseu->subslices_mask[0] |= BIT(0);
>> +        sseu->eu_mask[0] = (fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >> 
>> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
>> +        sseu->eu_mask[0] |= ((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >> 
>> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;
>
> Do you need to invert the bits here?

Oh dear... Indeed!

>
>> +        sseu->subslices_mask[0] = 1;
>>       }
>>         if (!(fuse & CHV_FGT_DISABLE_SS1)) {
>> -        sseu->subslice_mask |= BIT(1);
>> -        eu_dis = fuse & (CHV_FGT_EU_DIS_SS1_R0_MASK |
>> -                 CHV_FGT_EU_DIS_SS1_R1_MASK);
>> -        sseu->eu_total += 8 - hweight32(eu_dis);
>> +        sseu->subslices_mask[0] |= BIT(1);
>> +        sseu->eu_mask[1] = (fuse & CHV_FGT_EU_DIS_SS1_R0_MASK) >> 
>> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
>> +        sseu->eu_mask[2] |= ((fuse & CHV_FGT_EU_DIS_SS1_R1_MASK) >> 
>> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;
>
> Why eu_mask indices 1 and 2 here, while for subslice 0 you merged them 
> into index 0?

Again, you're right.
Thanks!

>
>>       }
>>   +    sseu->eu_total = compute_eu_total(sseu);
>> +
>>       /*
>>        * CHV expected to always have a uniform distribution of EU
>>        * across subslices.
>> @@ -159,41 +215,50 @@ static void gen9_sseu_info_init(struct 
>> drm_i915_private *dev_priv)
>>   {
>>       struct intel_device_info *info = mkwrite_device_info(dev_priv);
>>       struct sseu_dev_info *sseu = &info->sseu;
>> -    int s_max = 3, ss_max = 4, eu_max = 8;
>>       int s, ss;
>> -    u32 fuse2, eu_disable;
>> +    u32 fuse2, eu_disable, subslice_mask;
>>       u8 eu_mask = 0xff;
>>         fuse2 = I915_READ(GEN8_FUSE2);
>>       sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> 
>> GEN8_F2_S_ENA_SHIFT;
>>   +    /* BXT has a single slice and at most 3 subslices. */
>> +    sseu->max_slices = IS_GEN9_LP(dev_priv) ? 1 : 3;
>> +    sseu->max_subslices = IS_GEN9_LP(dev_priv) ? 3 : 4;
>> +    sseu->max_eus_per_subslice = 8;
>> +
>>       /*
>>        * The subslice disable field is global, i.e. it applies
>>        * to each of the enabled slices.
>>       */
>> -    sseu->subslice_mask = (1 << ss_max) - 1;
>> -    sseu->subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
>> -                 GEN9_F2_SS_DIS_SHIFT);
>> +    subslice_mask = (1 << sseu->max_subslices) - 1;
>> +    subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
>> +               GEN9_F2_SS_DIS_SHIFT);
>>         /*
>>        * Iterate through enabled slices and subslices to
>>        * count the total enabled EU.
>>       */
>> -    for (s = 0; s < s_max; s++) {
>> +    for (s = 0; s < sseu->max_slices; s++) {
>>           if (!(sseu->slice_mask & BIT(s)))
>>               /* skip disabled slice */
>>               continue;
>>   +        sseu->subslices_mask[s] = subslice_mask;
>> +
>>           eu_disable = I915_READ(GEN9_EU_DISABLE(s));
>> -        for (ss = 0; ss < ss_max; ss++) {
>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>>               int eu_per_ss;
>>   -            if (!(sseu->subslice_mask & BIT(ss)))
>> +            if (!(sseu->subslices_mask[s] & BIT(ss)))
>>                   /* skip disabled subslice */
>>                   continue;
>>   -            eu_per_ss = eu_max - hweight8((eu_disable >> (ss*8)) &
>> -                              eu_mask);
>> +            sseu->eu_mask[ss + s * sseu->max_subslices] =
>> +                ~((eu_disable >> (ss*8)) & eu_mask);
>> +
>> +            eu_per_ss = sseu->max_eus_per_subslice -
>> +                hweight8((eu_disable >> (ss*8)) & eu_mask);
>
> Store "eu_disable >> (ss*8)) & eu_mask" in a local for readability 
> since it is calculated twice?

Done.

>
>>                 /*
>>                * Record which subslice(s) has(have) 7 EUs. we
>> @@ -202,11 +267,11 @@ static void gen9_sseu_info_init(struct 
>> drm_i915_private *dev_priv)
>>                */
>>               if (eu_per_ss == 7)
>>                   sseu->subslice_7eu[s] |= BIT(ss);
>> -
>> -            sseu->eu_total += eu_per_ss;
>
> You could theoretically keep this running total and then at the end 
> compare against compute_eu_total under a GEM_BUG_ON, just to check 
> everyting is working as expected.
>
>>           }
>>       }
>>   +    sseu->eu_total = compute_eu_total(sseu);
>> +
>>       /*
>>        * SKL is expected to always have a uniform distribution
>>        * of EU across subslices with the exception that any one
>> @@ -232,8 +297,8 @@ static void gen9_sseu_info_init(struct 
>> drm_i915_private *dev_priv)
>>       sseu->has_eu_pg = sseu->eu_per_subslice > 2;
>>         if (IS_GEN9_LP(dev_priv)) {
>> -#define IS_SS_DISABLED(ss)    (!(sseu->subslice_mask & BIT(ss)))
>> -        info->has_pooled_eu = hweight8(sseu->subslice_mask) == 3;
>> +#define IS_SS_DISABLED(ss)    (!(sseu->subslices_mask[0] & BIT(ss)))
>> +        info->has_pooled_eu = hweight8(sseu->subslices_mask[0]) == 3;
>>             sseu->min_eu_in_pool = 0;
>>           if (info->has_pooled_eu) {
>> @@ -251,19 +316,22 @@ static void gen9_sseu_info_init(struct 
>> drm_i915_private *dev_priv)
>>   static void broadwell_sseu_info_init(struct drm_i915_private 
>> *dev_priv)
>>   {
>>       struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
>> -    const int s_max = 3, ss_max = 3, eu_max = 8;
>>       int s, ss;
>> -    u32 fuse2, eu_disable[3]; /* s_max */
>> +    u32 fuse2, subslice_mask, eu_disable[3]; /* s_max */
>>         fuse2 = I915_READ(GEN8_FUSE2);
>>       sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> 
>> GEN8_F2_S_ENA_SHIFT;
>> +    sseu->max_slices = 3;
>> +    sseu->max_subslices = 3;
>> +    sseu->max_eus_per_subslice = 8;
>> +
>>       /*
>>        * The subslice disable field is global, i.e. it applies
>>        * to each of the enabled slices.
>>        */
>> -    sseu->subslice_mask = GENMASK(ss_max - 1, 0);
>> -    sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
>> -                 GEN8_F2_SS_DIS_SHIFT);
>> +    subslice_mask = GENMASK(sseu->max_subslices - 1, 0);
>> +    subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
>> +               GEN8_F2_SS_DIS_SHIFT);
>>         eu_disable[0] = I915_READ(GEN8_EU_DISABLE0) & 
>> GEN8_EU_DIS0_S0_MASK;
>>       eu_disable[1] = (I915_READ(GEN8_EU_DISABLE0) >> 
>> GEN8_EU_DIS0_S1_SHIFT) |
>> @@ -277,30 +345,36 @@ static void broadwell_sseu_info_init(struct 
>> drm_i915_private *dev_priv)
>>        * Iterate through enabled slices and subslices to
>>        * count the total enabled EU.
>>        */
>> -    for (s = 0; s < s_max; s++) {
>> +    for (s = 0; s < sseu->max_slices; s++) {
>>           if (!(sseu->slice_mask & BIT(s)))
>>               /* skip disabled slice */
>>               continue;
>>   -        for (ss = 0; ss < ss_max; ss++) {
>> +        sseu->subslices_mask[s] = subslice_mask;
>> +
>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>>               u32 n_disabled;
>>   -            if (!(sseu->subslice_mask & BIT(ss)))
>> +            if (!(sseu->subslices_mask[ss] & BIT(ss)))
>>                   /* skip disabled subslice */
>>                   continue;
>>   -            n_disabled = hweight8(eu_disable[s] >> (ss * eu_max));
>> +            sseu->eu_mask[ss + s * sseu->max_subslices] =
>> +                ~(eu_disable[s] >>
>> +                  (ss * sseu->max_eus_per_subslice));
>> +            n_disabled = hweight8(eu_disable[s] >>
>> +                          (ss * sseu->max_eus_per_subslice));
>
> As above could use a local for "eu_disable[s] >> (ss * 
> sseu->max_eus_per_subslice)"

Done.

>
>>                 /*
>>                * Record which subslices have 7 EUs.
>>                */
>> -            if (eu_max - n_disabled == 7)
>> +            if (sseu->max_eus_per_subslice - n_disabled == 7)
>>                   sseu->subslice_7eu[s] |= 1 << ss;
>> -
>> -            sseu->eu_total += eu_max - n_disabled;
>>           }
>>       }
>>   +    sseu->eu_total = compute_eu_total(sseu);
>> +
>>       /*
>>        * BDW is expected to always have a uniform distribution of EU 
>> across
>>        * subslices with the exception that any one EU in any one 
>> subslice may
>> @@ -437,6 +511,7 @@ void intel_device_info_runtime_init(struct 
>> drm_i915_private *dev_priv)
>>   {
>>       struct intel_device_info *info = mkwrite_device_info(dev_priv);
>>       enum pipe pipe;
>> +    int s;
>>         if (INTEL_GEN(dev_priv) >= 10) {
>>           for_each_pipe(dev_priv, pipe)
>> @@ -548,9 +623,11 @@ void intel_device_info_runtime_init(struct 
>> drm_i915_private *dev_priv)
>>       DRM_DEBUG_DRIVER("slice total: %u\n", 
>> hweight8(info->sseu.slice_mask));
>>       DRM_DEBUG_DRIVER("subslice total: %u\n",
>>                sseu_subslice_total(&info->sseu));
>> -    DRM_DEBUG_DRIVER("subslice mask %04x\n", info->sseu.subslice_mask);
>> -    DRM_DEBUG_DRIVER("subslice per slice: %u\n",
>> -             hweight8(info->sseu.subslice_mask));
>> +    for (s = 0; s < ARRAY_SIZE(info->sseu.subslices_mask); s++) {
>> +        DRM_DEBUG_DRIVER("subslice mask %04x\n", 
>> info->sseu.subslices_mask[s]);
>> +        DRM_DEBUG_DRIVER("subslice per slice: %u\n",
>> +                 hweight8(info->sseu.subslices_mask[s]));
>
> Put a subslice index into messages?

Done.

>
>> +    }
>>       DRM_DEBUG_DRIVER("EU total: %u\n", info->sseu.eu_total);
>>       DRM_DEBUG_DRIVER("EU per subslice: %u\n", 
>> info->sseu.eu_per_subslice);
>>       DRM_DEBUG_DRIVER("has slice power gating: %s\n",
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index 2e38fbfdf08f..6e55b842c21d 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -2072,7 +2072,7 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>         if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>           rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
>> -        rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask) <<
>> +        rpcs |= 
>> hweight8(INTEL_INFO(dev_priv)->sseu.subslices_mask[0]) <<
>>               GEN8_RPCS_SS_CNT_SHIFT;
>>           rpcs |= GEN8_RPCS_ENABLE;
>>       }
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index c5ff203e42d6..a94bc1b5c502 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -90,7 +90,7 @@ hangcheck_action_to_str(const enum 
>> intel_engine_hangcheck_action a)
>>     #define instdone_subslice_mask(dev_priv__) \
>>       (INTEL_GEN(dev_priv__) == 7 ? \
>> -     1 : INTEL_INFO(dev_priv__)->sseu.subslice_mask)
>> +     1 : INTEL_INFO(dev_priv__)->sseu.subslices_mask[0])
>>     #define for_each_instdone_slice_subslice(dev_priv__, slice__, 
>> subslice__) \
>>       for ((slice__) = 0, (subslice__) = 0; \
>>
>
> Regards,
>
> Tvrtko
>

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

Re: [PATCH 1/6] drm/i915: store all subslice masks

[Tvrtko Ursulin]

On 11/01/2018 15:48, Lionel Landwerlin wrote:
> On 11/01/18 11:12, Tvrtko Ursulin wrote:
>>
>> On 18/12/2017 15:35, Lionel Landwerlin wrote:
>>> Up to now, subslice mask was assumed to be uniform across slices. But
>>> starting with Cannonlake, slices can be asymetric (for example slice0
>>
>> asymmetric, thanks auto spell checker. :)
> 
> Done, thanks.
> 
>>
>>> has different number of subslices as slice1+). This change stores all
>>> subslices masks for all slices rather than having a single mask that
>>> applies to all slices.
>>>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c      |  24 +++--
>>>   drivers/gpu/drm/i915/i915_drv.c          |   2 +-
>>>   drivers/gpu/drm/i915/i915_drv.h          |  23 ++++-
>>>   drivers/gpu/drm/i915/intel_device_info.c | 169 
>>> ++++++++++++++++++++++---------
>>>   drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
>>>   drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
>>>   6 files changed, 161 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 0ddce72552bf..0c7890b695c5 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -4293,7 +4293,7 @@ static void 
>>> cherryview_sseu_device_status(struct drm_i915_private *dev_priv,
>>>               continue;
>>>             sseu->slice_mask = BIT(0);
>>> -        sseu->subslice_mask |= BIT(ss);
>>> +        sseu->subslices_mask[0] |= BIT(ss);
>>>           eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) +
>>>                ((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) +
>>>                ((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) +
>>> @@ -4340,7 +4340,7 @@ static void gen10_sseu_device_status(struct 
>>> drm_i915_private *dev_priv,
>>>               continue;
>>>             sseu->slice_mask |= BIT(s);
>>> -        sseu->subslice_mask = info->sseu.subslice_mask;
>>> +        sseu->subslices_mask[s] = info->sseu.subslices_mask[s];
>>>             for (ss = 0; ss < ss_max; ss++) {
>>>               unsigned int eu_cnt;
>>> @@ -4395,8 +4395,8 @@ static void gen9_sseu_device_status(struct 
>>> drm_i915_private *dev_priv,
>>>           sseu->slice_mask |= BIT(s);
>>>             if (IS_GEN9_BC(dev_priv))
>>> -            sseu->subslice_mask =
>>> -                INTEL_INFO(dev_priv)->sseu.subslice_mask;
>>> +            sseu->subslices_mask[s] =
>>> + INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
>>>             for (ss = 0; ss < ss_max; ss++) {
>>>               unsigned int eu_cnt;
>>> @@ -4406,7 +4406,7 @@ static void gen9_sseu_device_status(struct 
>>> drm_i915_private *dev_priv,
>>>                       /* skip disabled subslice */
>>>                       continue;
>>>   -                sseu->subslice_mask |= BIT(ss);
>>> +                sseu->subslices_mask[s] |= BIT(ss);
>>>               }
>>>                 eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
>>> @@ -4428,9 +4428,12 @@ static void 
>>> broadwell_sseu_device_status(struct drm_i915_private *dev_priv,
>>>       sseu->slice_mask = slice_info & GEN8_LSLICESTAT_MASK;
>>>         if (sseu->slice_mask) {
>>> -        sseu->subslice_mask = INTEL_INFO(dev_priv)->sseu.subslice_mask;
>>>           sseu->eu_per_subslice =
>>>                   INTEL_INFO(dev_priv)->sseu.eu_per_subslice;
>>> +        for (s = 0; s < fls(sseu->slice_mask); s++) {
>>> +            sseu->subslices_mask[s] =
>>> + INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
>>> +        }
>>>           sseu->eu_total = sseu->eu_per_subslice *
>>>                    sseu_subslice_total(sseu);
>>>   @@ -4449,6 +4452,7 @@ static void i915_print_sseu_info(struct 
>>> seq_file *m, bool is_available_info,
>>>   {
>>>       struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>>       const char *type = is_available_info ? "Available" : "Enabled";
>>> +    int s;
>>>         seq_printf(m, "  %s Slice Mask: %04x\n", type,
>>>              sseu->slice_mask);
>>> @@ -4456,10 +4460,10 @@ static void i915_print_sseu_info(struct 
>>> seq_file *m, bool is_available_info,
>>>              hweight8(sseu->slice_mask));
>>>       seq_printf(m, "  %s Subslice Total: %u\n", type,
>>>              sseu_subslice_total(sseu));
>>> -    seq_printf(m, "  %s Subslice Mask: %04x\n", type,
>>> -           sseu->subslice_mask);
>>> -    seq_printf(m, "  %s Subslice Per Slice: %u\n", type,
>>> -           hweight8(sseu->subslice_mask));
>>> +    for (s = 0; s < fls(sseu->slice_mask); s++) {
>>
>> Slice mask is always contiguous bits?
> 
> I have a 2x6 BXT on my desk where the subslice0 appears to be fused-off.
> I assumed same could be true for slices.
> fls() should make us iterate through all the slices (even the fused off 
> ones).

Do you then want to skip printing the fused off ones? Just asking, don't 
think it matters hugely.

>>
>>> +        seq_printf(m, "  %s Slice%i Subslice Mask: %04x\n", type,
>>> +               s, sseu->subslices_mask[s]);
>>
>> Don't want to keep printing the count? Like maybe " %s Slice%u %u 
>> sublices, mask=%04x\n" ?
> 
> Sure, done.
> 
>>
>>> +    }
>>>       seq_printf(m, "  %s EU Total: %u\n", type,
>>>              sseu->eu_total);
>>>       seq_printf(m, "  %s EU Per Subslice: %u\n", type,
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index 72bea281edb7..8b99e415c345 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -414,7 +414,7 @@ static int i915_getparam(struct drm_device *dev, 
>>> void *data,
>>>               return -ENODEV;
>>>           break;
>>>       case I915_PARAM_SUBSLICE_MASK:
>>> -        value = INTEL_INFO(dev_priv)->sseu.subslice_mask;
>>> +        value = INTEL_INFO(dev_priv)->sseu.subslices_mask[0];
>>>           if (!value)
>>>               return -ENODEV;
>>>           break;
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 1aba5657f5f0..82fc59078c6a 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -802,9 +802,12 @@ struct intel_csr {
>>>       func(supports_tv); \
>>>       func(has_ipc);
>>>   +#define GEN_MAX_SLICES        (6) /* CNL upper bound */
>>> +#define GEN_MAX_SUBSLICES    (7)
>>> +
>>>   struct sseu_dev_info {
>>>       u8 slice_mask;
>>> -    u8 subslice_mask;
>>> +    u8 subslices_mask[GEN_MAX_SLICES];
>>
>> Personally I would probably kept the subslice_mask name and just 
>> turned it into an array, but that is bike-shed territory.
> 
> It was easier to spot all the places to modify. Will go back.
> 
>>
>>>       u8 eu_total;
>>>       u8 eu_per_subslice;
>>>       u8 min_eu_in_pool;
>>> @@ -813,11 +816,27 @@ struct sseu_dev_info {
>>>       u8 has_slice_pg:1;
>>>       u8 has_subslice_pg:1;
>>>       u8 has_eu_pg:1;
>>> +
>>> +    /* Topology fields */
>>> +    u8 max_slices;
>>> +    u8 max_subslices;
>>> +    u8 max_eus_per_subslice;
>>> +
>>> +    /* We don't have more than 8 eus per subslice at the moment and 
>>> as we
>>> +     * store eus enabled using bits, no need to multiply by eus per
>>> +     * subslice.
>>> +     */
>>> +    u8 eu_mask[GEN_MAX_SLICES * GEN_MAX_SUBSLICES];
>>>   };
>>>     static inline unsigned int sseu_subslice_total(const struct 
>>> sseu_dev_info *sseu)
>>>   {
>>> -    return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
>>> +    unsigned s, total = 0;
>>> +
>>> +    for (s = 0; s < ARRAY_SIZE(sseu->subslices_mask); s++)
>>> +        total += hweight8(sseu->subslices_mask[s]);
>>> +
>>> +    return total;
>>
>> The function is bigger now so maybe it needs stops being inline, but 
>> either moved to the .c file, or even the total stored in struct 
>> sseu_dev_info, and then this becomes just a getter?
> 
> Let's go with a subslice_total field.
> 
>>
>>>   }
>>>     /* Keep in gen based order, and chronological order within a gen */
>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>>> b/drivers/gpu/drm/i915/intel_device_info.c
>>> index f478be3ae0ba..6a3c40439e83 100644
>>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>>> @@ -82,22 +82,74 @@ void intel_device_info_dump(struct 
>>> drm_i915_private *dev_priv)
>>>   #undef PRINT_FLAG
>>>   }
>>>   +static u8 compute_eu_total(const struct sseu_dev_info *sseu)
>>> +{
>>> +    u8 i, total = 0;
>>
>> sseu_subslice_total returns an unsinged int, while this is u8, 
>> suggesting there can be more sublices and eus. ;) Jokes aside, I'd 
>> probably make them consistent. The index variable is probably best as 
>> unsigned int, but the total and return can be u8 if big enough for 
>> total eus? Either way best it matches the types in the struct they get 
>> assigned to.
> 
> You're right!
> u16 should be fine though.
> 
>>
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(sseu->eu_mask); i++)
>>> +        total += hweight8(sseu->eu_mask[i]);
>>> +
>>> +    return total;
>>> +}
>>> +
>>>   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);
>>> +    int s, ss, eu_mask = 0xff;
>>
>> const eu_mask ?
> 
> Done.
> 
>>
>>> +    u32 subslice_mask, eu_en;
>>>         sseu->slice_mask = (fuse2 & GEN10_F2_S_ENA_MASK) >>
>>>                   GEN10_F2_S_ENA_SHIFT;
>>> -    sseu->subslice_mask = (1 << 4) - 1;
>>> -    sseu->subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
>>> -                 GEN10_F2_SS_DIS_SHIFT);
>>> +    sseu->max_slices = 6;
>>> +    sseu->max_subslices = 4;
>>> +    sseu->max_eus_per_subslice = 8;
>>> +
>>> +    subslice_mask = (1 << 4) - 1;
>>> +    subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
>>> +               GEN10_F2_SS_DIS_SHIFT);
>>> +
>>> +    /* Slice0 can have up to 3 subslices, but there are only 2 in
>>> +     * slice1/2.
>>> +     */
>>> +    sseu->subslices_mask[0] = subslice_mask;
>>> +    for (s = 1; s < sseu->max_slices; s++)
>>> +        sseu->subslices_mask[s] = subslice_mask & 0x3;
>>> +
>>> +    /* Slice0 */
>>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE0);
>>> +    for (ss = 0; ss < sseu->max_subslices; ss++)
>>> +        sseu->eu_mask[ss]     = (eu_en >> (8 * ss)) & eu_mask;
>>> +    /* Slice1 */
>>> +    sseu->eu_mask[sseu->max_subslices]         = (eu_en >> 24) & 
>>> eu_mask;
>>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE1);
>>> +    sseu->eu_mask[sseu->max_subslices + 1]     = eu_en & eu_mask;
>>
>> I suggest a helper to index into sse->eu_mask, like 
>> sseu->eu_mask[_eu_mask_idx(slice, subslice)] or something, so it is 
>> more readable what is happening here. Or even:
>>
>>  _eu_mask(sseu, slice, subslice) = mask;
>>
>> Doable? I am not 100% I picked up exactly on the layout of the eu_mask 
>> array.. each element is one subslice? Consecutive for slice0, 
>> subslice0..N, slice1... sliceN ?
> 
> Unfortunately that's not possible because of the asymmetry thing...

Hm so on a given SKU you are saying the eu_mask you are generating is 
also asymmetrical? Meaning different number of total allocated bits 
per-slice? Could we have it symmetrical in the internal representation 
and uAPI for simpler code?

Regards,

Tvrtko

>>
>>> +    /* Slice2 */
>>> +    sseu->eu_mask[2 * sseu->max_subslices]     = (eu_en >> 8) & 
>>> eu_mask;
>>> +    sseu->eu_mask[2 * sseu->max_subslices + 1] = (eu_en >> 16) & 
>>> eu_mask;
>>> +    /* Slice3 */
>>> +    sseu->eu_mask[3 * sseu->max_subslices]     = (eu_en >> 24) & 
>>> eu_mask;
>>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE2);
>>> +    sseu->eu_mask[3 * sseu->max_subslices + 1] = eu_en & eu_mask;
>>> +    /* Slice4 */
>>> +    sseu->eu_mask[4 * sseu->max_subslices]     = (eu_en >> 8) & 
>>> eu_mask;
>>> +    sseu->eu_mask[4 * sseu->max_subslices + 1] = (eu_en >> 16) & 
>>> eu_mask;
>>> +    /* Slice5 */
>>> +    sseu->eu_mask[5 * sseu->max_subslices]     = (eu_en >> 24) & 
>>> eu_mask;
>>> +    eu_en = ~I915_READ(GEN10_EU_DISABLE3);
>>> +    sseu->eu_mask[5 * sseu->max_subslices + 1] = eu_en & eu_mask;
>>> +
>>> +    /* Do a second pass where we marked the subslices disabled if all
>>> +     * their eus are off.
>>> +     */
>>> +    for (s = 0; s < sseu->max_slices; s++) {
>>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>>> +            if (sseu->eu_mask[s * sseu->max_subslices + ss] == 0)
>>> +                sseu->subslices_mask[s] &= ~BIT(ss);
>>> +        }
>>> +    }
>>>   -    sseu->eu_total = hweight32(~I915_READ(GEN8_EU_DISABLE0));
>>> -    sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE1));
>>> -    sseu->eu_total += hweight32(~I915_READ(GEN8_EU_DISABLE2));
>>> -    sseu->eu_total += hweight8(~(I915_READ(GEN10_EU_DISABLE3) &
>>> -                     GEN10_EU_DIS_SS_MASK));
>>> +    sseu->eu_total = compute_eu_total(sseu);
>>>         /*
>>>        * CNL is expected to always have a uniform distribution
>>> @@ -118,26 +170,30 @@ static void gen10_sseu_info_init(struct 
>>> drm_i915_private *dev_priv)
>>>   static void cherryview_sseu_info_init(struct drm_i915_private 
>>> *dev_priv)
>>>   {
>>>       struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
>>> -    u32 fuse, eu_dis;
>>> +    u32 fuse;
>>>         fuse = I915_READ(CHV_FUSE_GT);
>>>         sseu->slice_mask = BIT(0);
>>> +    sseu->max_slices = 1;
>>> +    sseu->max_subslices = 2;
>>> +    sseu->max_eus_per_subslice = 8;
>>>         if (!(fuse & CHV_FGT_DISABLE_SS0)) {
>>> -        sseu->subslice_mask |= BIT(0);
>>> -        eu_dis = fuse & (CHV_FGT_EU_DIS_SS0_R0_MASK |
>>> -                 CHV_FGT_EU_DIS_SS0_R1_MASK);
>>> -        sseu->eu_total += 8 - hweight32(eu_dis);
>>> +        sseu->subslices_mask[0] |= BIT(0);
>>> +        sseu->eu_mask[0] = (fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >> 
>>> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
>>> +        sseu->eu_mask[0] |= ((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >> 
>>> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;
>>
>> Do you need to invert the bits here?
> 
> Oh dear... Indeed!
> 
>>
>>> +        sseu->subslices_mask[0] = 1;
>>>       }
>>>         if (!(fuse & CHV_FGT_DISABLE_SS1)) {
>>> -        sseu->subslice_mask |= BIT(1);
>>> -        eu_dis = fuse & (CHV_FGT_EU_DIS_SS1_R0_MASK |
>>> -                 CHV_FGT_EU_DIS_SS1_R1_MASK);
>>> -        sseu->eu_total += 8 - hweight32(eu_dis);
>>> +        sseu->subslices_mask[0] |= BIT(1);
>>> +        sseu->eu_mask[1] = (fuse & CHV_FGT_EU_DIS_SS1_R0_MASK) >> 
>>> CHV_FGT_EU_DIS_SS0_R0_SHIFT;
>>> +        sseu->eu_mask[2] |= ((fuse & CHV_FGT_EU_DIS_SS1_R1_MASK) >> 
>>> CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4;
>>
>> Why eu_mask indices 1 and 2 here, while for subslice 0 you merged them 
>> into index 0?
> 
> Again, you're right.
> Thanks!
> 
>>
>>>       }
>>>   +    sseu->eu_total = compute_eu_total(sseu);
>>> +
>>>       /*
>>>        * CHV expected to always have a uniform distribution of EU
>>>        * across subslices.
>>> @@ -159,41 +215,50 @@ static void gen9_sseu_info_init(struct 
>>> drm_i915_private *dev_priv)
>>>   {
>>>       struct intel_device_info *info = mkwrite_device_info(dev_priv);
>>>       struct sseu_dev_info *sseu = &info->sseu;
>>> -    int s_max = 3, ss_max = 4, eu_max = 8;
>>>       int s, ss;
>>> -    u32 fuse2, eu_disable;
>>> +    u32 fuse2, eu_disable, subslice_mask;
>>>       u8 eu_mask = 0xff;
>>>         fuse2 = I915_READ(GEN8_FUSE2);
>>>       sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> 
>>> GEN8_F2_S_ENA_SHIFT;
>>>   +    /* BXT has a single slice and at most 3 subslices. */
>>> +    sseu->max_slices = IS_GEN9_LP(dev_priv) ? 1 : 3;
>>> +    sseu->max_subslices = IS_GEN9_LP(dev_priv) ? 3 : 4;
>>> +    sseu->max_eus_per_subslice = 8;
>>> +
>>>       /*
>>>        * The subslice disable field is global, i.e. it applies
>>>        * to each of the enabled slices.
>>>       */
>>> -    sseu->subslice_mask = (1 << ss_max) - 1;
>>> -    sseu->subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
>>> -                 GEN9_F2_SS_DIS_SHIFT);
>>> +    subslice_mask = (1 << sseu->max_subslices) - 1;
>>> +    subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
>>> +               GEN9_F2_SS_DIS_SHIFT);
>>>         /*
>>>        * Iterate through enabled slices and subslices to
>>>        * count the total enabled EU.
>>>       */
>>> -    for (s = 0; s < s_max; s++) {
>>> +    for (s = 0; s < sseu->max_slices; s++) {
>>>           if (!(sseu->slice_mask & BIT(s)))
>>>               /* skip disabled slice */
>>>               continue;
>>>   +        sseu->subslices_mask[s] = subslice_mask;
>>> +
>>>           eu_disable = I915_READ(GEN9_EU_DISABLE(s));
>>> -        for (ss = 0; ss < ss_max; ss++) {
>>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>>>               int eu_per_ss;
>>>   -            if (!(sseu->subslice_mask & BIT(ss)))
>>> +            if (!(sseu->subslices_mask[s] & BIT(ss)))
>>>                   /* skip disabled subslice */
>>>                   continue;
>>>   -            eu_per_ss = eu_max - hweight8((eu_disable >> (ss*8)) &
>>> -                              eu_mask);
>>> +            sseu->eu_mask[ss + s * sseu->max_subslices] =
>>> +                ~((eu_disable >> (ss*8)) & eu_mask);
>>> +
>>> +            eu_per_ss = sseu->max_eus_per_subslice -
>>> +                hweight8((eu_disable >> (ss*8)) & eu_mask);
>>
>> Store "eu_disable >> (ss*8)) & eu_mask" in a local for readability 
>> since it is calculated twice?
> 
> Done.
> 
>>
>>>                 /*
>>>                * Record which subslice(s) has(have) 7 EUs. we
>>> @@ -202,11 +267,11 @@ static void gen9_sseu_info_init(struct 
>>> drm_i915_private *dev_priv)
>>>                */
>>>               if (eu_per_ss == 7)
>>>                   sseu->subslice_7eu[s] |= BIT(ss);
>>> -
>>> -            sseu->eu_total += eu_per_ss;
>>
>> You could theoretically keep this running total and then at the end 
>> compare against compute_eu_total under a GEM_BUG_ON, just to check 
>> everyting is working as expected.
>>
>>>           }
>>>       }
>>>   +    sseu->eu_total = compute_eu_total(sseu);
>>> +
>>>       /*
>>>        * SKL is expected to always have a uniform distribution
>>>        * of EU across subslices with the exception that any one
>>> @@ -232,8 +297,8 @@ static void gen9_sseu_info_init(struct 
>>> drm_i915_private *dev_priv)
>>>       sseu->has_eu_pg = sseu->eu_per_subslice > 2;
>>>         if (IS_GEN9_LP(dev_priv)) {
>>> -#define IS_SS_DISABLED(ss)    (!(sseu->subslice_mask & BIT(ss)))
>>> -        info->has_pooled_eu = hweight8(sseu->subslice_mask) == 3;
>>> +#define IS_SS_DISABLED(ss)    (!(sseu->subslices_mask[0] & BIT(ss)))
>>> +        info->has_pooled_eu = hweight8(sseu->subslices_mask[0]) == 3;
>>>             sseu->min_eu_in_pool = 0;
>>>           if (info->has_pooled_eu) {
>>> @@ -251,19 +316,22 @@ static void gen9_sseu_info_init(struct 
>>> drm_i915_private *dev_priv)
>>>   static void broadwell_sseu_info_init(struct drm_i915_private 
>>> *dev_priv)
>>>   {
>>>       struct sseu_dev_info *sseu = &mkwrite_device_info(dev_priv)->sseu;
>>> -    const int s_max = 3, ss_max = 3, eu_max = 8;
>>>       int s, ss;
>>> -    u32 fuse2, eu_disable[3]; /* s_max */
>>> +    u32 fuse2, subslice_mask, eu_disable[3]; /* s_max */
>>>         fuse2 = I915_READ(GEN8_FUSE2);
>>>       sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> 
>>> GEN8_F2_S_ENA_SHIFT;
>>> +    sseu->max_slices = 3;
>>> +    sseu->max_subslices = 3;
>>> +    sseu->max_eus_per_subslice = 8;
>>> +
>>>       /*
>>>        * The subslice disable field is global, i.e. it applies
>>>        * to each of the enabled slices.
>>>        */
>>> -    sseu->subslice_mask = GENMASK(ss_max - 1, 0);
>>> -    sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
>>> -                 GEN8_F2_SS_DIS_SHIFT);
>>> +    subslice_mask = GENMASK(sseu->max_subslices - 1, 0);
>>> +    subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
>>> +               GEN8_F2_SS_DIS_SHIFT);
>>>         eu_disable[0] = I915_READ(GEN8_EU_DISABLE0) & 
>>> GEN8_EU_DIS0_S0_MASK;
>>>       eu_disable[1] = (I915_READ(GEN8_EU_DISABLE0) >> 
>>> GEN8_EU_DIS0_S1_SHIFT) |
>>> @@ -277,30 +345,36 @@ static void broadwell_sseu_info_init(struct 
>>> drm_i915_private *dev_priv)
>>>        * Iterate through enabled slices and subslices to
>>>        * count the total enabled EU.
>>>        */
>>> -    for (s = 0; s < s_max; s++) {
>>> +    for (s = 0; s < sseu->max_slices; s++) {
>>>           if (!(sseu->slice_mask & BIT(s)))
>>>               /* skip disabled slice */
>>>               continue;
>>>   -        for (ss = 0; ss < ss_max; ss++) {
>>> +        sseu->subslices_mask[s] = subslice_mask;
>>> +
>>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>>>               u32 n_disabled;
>>>   -            if (!(sseu->subslice_mask & BIT(ss)))
>>> +            if (!(sseu->subslices_mask[ss] & BIT(ss)))
>>>                   /* skip disabled subslice */
>>>                   continue;
>>>   -            n_disabled = hweight8(eu_disable[s] >> (ss * eu_max));
>>> +            sseu->eu_mask[ss + s * sseu->max_subslices] =
>>> +                ~(eu_disable[s] >>
>>> +                  (ss * sseu->max_eus_per_subslice));
>>> +            n_disabled = hweight8(eu_disable[s] >>
>>> +                          (ss * sseu->max_eus_per_subslice));
>>
>> As above could use a local for "eu_disable[s] >> (ss * 
>> sseu->max_eus_per_subslice)"
> 
> Done.
> 
>>
>>>                 /*
>>>                * Record which subslices have 7 EUs.
>>>                */
>>> -            if (eu_max - n_disabled == 7)
>>> +            if (sseu->max_eus_per_subslice - n_disabled == 7)
>>>                   sseu->subslice_7eu[s] |= 1 << ss;
>>> -
>>> -            sseu->eu_total += eu_max - n_disabled;
>>>           }
>>>       }
>>>   +    sseu->eu_total = compute_eu_total(sseu);
>>> +
>>>       /*
>>>        * BDW is expected to always have a uniform distribution of EU 
>>> across
>>>        * subslices with the exception that any one EU in any one 
>>> subslice may
>>> @@ -437,6 +511,7 @@ void intel_device_info_runtime_init(struct 
>>> drm_i915_private *dev_priv)
>>>   {
>>>       struct intel_device_info *info = mkwrite_device_info(dev_priv);
>>>       enum pipe pipe;
>>> +    int s;
>>>         if (INTEL_GEN(dev_priv) >= 10) {
>>>           for_each_pipe(dev_priv, pipe)
>>> @@ -548,9 +623,11 @@ void intel_device_info_runtime_init(struct 
>>> drm_i915_private *dev_priv)
>>>       DRM_DEBUG_DRIVER("slice total: %u\n", 
>>> hweight8(info->sseu.slice_mask));
>>>       DRM_DEBUG_DRIVER("subslice total: %u\n",
>>>                sseu_subslice_total(&info->sseu));
>>> -    DRM_DEBUG_DRIVER("subslice mask %04x\n", info->sseu.subslice_mask);
>>> -    DRM_DEBUG_DRIVER("subslice per slice: %u\n",
>>> -             hweight8(info->sseu.subslice_mask));
>>> +    for (s = 0; s < ARRAY_SIZE(info->sseu.subslices_mask); s++) {
>>> +        DRM_DEBUG_DRIVER("subslice mask %04x\n", 
>>> info->sseu.subslices_mask[s]);
>>> +        DRM_DEBUG_DRIVER("subslice per slice: %u\n",
>>> +                 hweight8(info->sseu.subslices_mask[s]));
>>
>> Put a subslice index into messages?
> 
> Done.
> 
>>
>>> +    }
>>>       DRM_DEBUG_DRIVER("EU total: %u\n", info->sseu.eu_total);
>>>       DRM_DEBUG_DRIVER("EU per subslice: %u\n", 
>>> info->sseu.eu_per_subslice);
>>>       DRM_DEBUG_DRIVER("has slice power gating: %s\n",
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 2e38fbfdf08f..6e55b842c21d 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -2072,7 +2072,7 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>>         if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>>           rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
>>> -        rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask) <<
>>> +        rpcs |= 
>>> hweight8(INTEL_INFO(dev_priv)->sseu.subslices_mask[0]) <<
>>>               GEN8_RPCS_SS_CNT_SHIFT;
>>>           rpcs |= GEN8_RPCS_ENABLE;
>>>       }
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> index c5ff203e42d6..a94bc1b5c502 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> @@ -90,7 +90,7 @@ hangcheck_action_to_str(const enum 
>>> intel_engine_hangcheck_action a)
>>>     #define instdone_subslice_mask(dev_priv__) \
>>>       (INTEL_GEN(dev_priv__) == 7 ? \
>>> -     1 : INTEL_INFO(dev_priv__)->sseu.subslice_mask)
>>> +     1 : INTEL_INFO(dev_priv__)->sseu.subslices_mask[0])
>>>     #define for_each_instdone_slice_subslice(dev_priv__, slice__, 
>>> subslice__) \
>>>       for ((slice__) = 0, (subslice__) = 0; \
>>>
>>
>> Regards,
>>
>> Tvrtko
>>
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: store all subslice masks

[Lionel Landwerlin]

[-- Attachment #1.1: Type: text/plain, Size: 1002 bytes --]

On 11/01/18 11:12, Tvrtko Ursulin wrote:
>> */
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE0);
>> +    for (ss = 0; ss < sseu->max_subslices; ss++)
>> +        sseu->eu_mask[ss]     = (eu_en >> (8 * ss)) & eu_mask;
>> +    /* Slice1 */
>> +    sseu->eu_mask[sseu->max_subslices]         = (eu_en >> 24) & 
>> eu_mask;
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE1);
>> +    sseu->eu_mask[sseu->max_subslices + 1]     = eu_en & eu_mask;
>
> I suggest a helper to index into sse->eu_mask, like 
> sseu->eu_mask[_eu_mask_idx(slice, subslice)] or something, so it is 
> more readable what is happening here. Or even:
>
>  _eu_mask(sseu, slice, subslice) = mask;
>
> Doable? I am not 100% I picked up exactly on the layout of the eu_mask 
> array.. each element is one subslice? Consecutive for slice0, 
> subslice0..N, slice1... sliceN ? 
Oops, I misread that part.
I thought you wanted a helper for accessing the registers.
I'll add a helper for what you would like.

[-- Attachment #1.2: Type: text/html, Size: 1708 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 2/6] drm/i915/debugfs: reuse max slice/subslices already stored in sseu

[Lionel Landwerlin]

Now that we have that information in topology fields, let's just reused it.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0c7890b695c5..6ec7543e698f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4308,11 +4308,11 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
 				     struct sseu_dev_info *sseu)
 {
 	const struct intel_device_info *info = INTEL_INFO(dev_priv);
-	int s_max = 6, ss_max = 4;
 	int s, ss;
-	u32 s_reg[s_max], eu_reg[2 * s_max], eu_mask[2];
+	u32 s_reg[info->sseu.max_slices],
+		eu_reg[2 * info->sseu.max_subslices], eu_mask[2];
 
-	for (s = 0; s < s_max; s++) {
+	for (s = 0; s < info->sseu.max_slices; s++) {
 		/*
 		 * FIXME: Valid SS Mask respects the spec and read
 		 * only valid bits for those registers, excluding reserverd
@@ -4334,7 +4334,7 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
 		     GEN9_PGCTL_SSB_EU210_ACK |
 		     GEN9_PGCTL_SSB_EU311_ACK;
 
-	for (s = 0; s < s_max; s++) {
+	for (s = 0; s < info->sseu.max_slices; s++) {
 		if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
 			/* skip disabled slice */
 			continue;
@@ -4342,7 +4342,7 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
 		sseu->slice_mask |= BIT(s);
 		sseu->subslices_mask[s] = info->sseu.subslices_mask[s];
 
-		for (ss = 0; ss < ss_max; ss++) {
+		for (ss = 0; ss < info->sseu.max_subslices; ss++) {
 			unsigned int eu_cnt;
 
 			if (!(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss))))
@@ -4362,17 +4362,11 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
 static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 				    struct sseu_dev_info *sseu)
 {
-	int s_max = 3, ss_max = 4;
+	const struct intel_device_info *info = INTEL_INFO(dev_priv);
 	int s, ss;
-	u32 s_reg[s_max], eu_reg[2*s_max], eu_mask[2];
-
-	/* BXT has a single slice and at most 3 subslices. */
-	if (IS_GEN9_LP(dev_priv)) {
-		s_max = 1;
-		ss_max = 3;
-	}
+	u32 s_reg[info->sseu.max_slices], eu_reg[2*info->sseu.max_subslices], eu_mask[2];
 
-	for (s = 0; s < s_max; s++) {
+	for (s = 0; s < info->sseu.max_slices; s++) {
 		s_reg[s] = I915_READ(GEN9_SLICE_PGCTL_ACK(s));
 		eu_reg[2*s] = I915_READ(GEN9_SS01_EU_PGCTL_ACK(s));
 		eu_reg[2*s + 1] = I915_READ(GEN9_SS23_EU_PGCTL_ACK(s));
@@ -4387,7 +4381,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 		     GEN9_PGCTL_SSB_EU210_ACK |
 		     GEN9_PGCTL_SSB_EU311_ACK;
 
-	for (s = 0; s < s_max; s++) {
+	for (s = 0; s < info->sseu.max_slices; s++) {
 		if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
 			/* skip disabled slice */
 			continue;
@@ -4398,7 +4392,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 			sseu->subslices_mask[s] =
 				INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
 
-		for (ss = 0; ss < ss_max; ss++) {
+		for (ss = 0; ss < info->sseu.max_subslices; ss++) {
 			unsigned int eu_cnt;
 
 			if (IS_GEN9_LP(dev_priv)) {
-- 
2.15.1

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

Re: [PATCH 2/6] drm/i915/debugfs: reuse max slice/subslices already stored in sseu

[Tvrtko Ursulin]

On 18/12/2017 15:35, Lionel Landwerlin wrote:
> Now that we have that information in topology fields, let's just reused it.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 26 ++++++++++----------------
>   1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0c7890b695c5..6ec7543e698f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4308,11 +4308,11 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
>   				     struct sseu_dev_info *sseu)
>   {
>   	const struct intel_device_info *info = INTEL_INFO(dev_priv);
> -	int s_max = 6, ss_max = 4;
>   	int s, ss;
> -	u32 s_reg[s_max], eu_reg[2 * s_max], eu_mask[2];
> +	u32 s_reg[info->sseu.max_slices],
> +		eu_reg[2 * info->sseu.max_subslices], eu_mask[2];

This is a bit unusual style, perhaps split into separate declarations.

>   
> -	for (s = 0; s < s_max; s++) {
> +	for (s = 0; s < info->sseu.max_slices; s++) {
>   		/*
>   		 * FIXME: Valid SS Mask respects the spec and read
>   		 * only valid bits for those registers, excluding reserverd
> @@ -4334,7 +4334,7 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
>   		     GEN9_PGCTL_SSB_EU210_ACK |
>   		     GEN9_PGCTL_SSB_EU311_ACK;
>   
> -	for (s = 0; s < s_max; s++) {
> +	for (s = 0; s < info->sseu.max_slices; s++) {
>   		if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
>   			/* skip disabled slice */
>   			continue;
> @@ -4342,7 +4342,7 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
>   		sseu->slice_mask |= BIT(s);
>   		sseu->subslices_mask[s] = info->sseu.subslices_mask[s];
>   
> -		for (ss = 0; ss < ss_max; ss++) {
> +		for (ss = 0; ss < info->sseu.max_subslices; ss++) {
>   			unsigned int eu_cnt;
>   
>   			if (!(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss))))
> @@ -4362,17 +4362,11 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
>   static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
>   				    struct sseu_dev_info *sseu)
>   {
> -	int s_max = 3, ss_max = 4;
> +	const struct intel_device_info *info = INTEL_INFO(dev_priv);
>   	int s, ss;
> -	u32 s_reg[s_max], eu_reg[2*s_max], eu_mask[2];
> -
> -	/* BXT has a single slice and at most 3 subslices. */
> -	if (IS_GEN9_LP(dev_priv)) {
> -		s_max = 1;
> -		ss_max = 3;
> -	}
> +	u32 s_reg[info->sseu.max_slices], eu_reg[2*info->sseu.max_subslices], eu_mask[2];

Spaces around operators are preferred.

>   
> -	for (s = 0; s < s_max; s++) {
> +	for (s = 0; s < info->sseu.max_slices; s++) {
>   		s_reg[s] = I915_READ(GEN9_SLICE_PGCTL_ACK(s));
>   		eu_reg[2*s] = I915_READ(GEN9_SS01_EU_PGCTL_ACK(s));
>   		eu_reg[2*s + 1] = I915_READ(GEN9_SS23_EU_PGCTL_ACK(s));
> @@ -4387,7 +4381,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
>   		     GEN9_PGCTL_SSB_EU210_ACK |
>   		     GEN9_PGCTL_SSB_EU311_ACK;
>   
> -	for (s = 0; s < s_max; s++) {
> +	for (s = 0; s < info->sseu.max_slices; s++) {
>   		if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
>   			/* skip disabled slice */
>   			continue;
> @@ -4398,7 +4392,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
>   			sseu->subslices_mask[s] =
>   				INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
>   
> -		for (ss = 0; ss < ss_max; ss++) {
> +		for (ss = 0; ss < info->sseu.max_subslices; ss++) {
>   			unsigned int eu_cnt;
>   
>   			if (IS_GEN9_LP(dev_priv)) {
> 

With the formatting tweaks,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 2/6] drm/i915/debugfs: reuse max slice/subslices already stored in sseu

[Lionel Landwerlin]

On 11/01/18 11:21, Tvrtko Ursulin wrote:
>
> On 18/12/2017 15:35, Lionel Landwerlin wrote:
>> Now that we have that information in topology fields, let's just 
>> reused it.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 26 ++++++++++----------------
>>   1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 0c7890b695c5..6ec7543e698f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -4308,11 +4308,11 @@ static void gen10_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>                        struct sseu_dev_info *sseu)
>>   {
>>       const struct intel_device_info *info = INTEL_INFO(dev_priv);
>> -    int s_max = 6, ss_max = 4;
>>       int s, ss;
>> -    u32 s_reg[s_max], eu_reg[2 * s_max], eu_mask[2];
>> +    u32 s_reg[info->sseu.max_slices],
>> +        eu_reg[2 * info->sseu.max_subslices], eu_mask[2];
>
> This is a bit unusual style, perhaps split into separate declarations.

Done.

>
>>   -    for (s = 0; s < s_max; s++) {
>> +    for (s = 0; s < info->sseu.max_slices; s++) {
>>           /*
>>            * FIXME: Valid SS Mask respects the spec and read
>>            * only valid bits for those registers, excluding reserverd
>> @@ -4334,7 +4334,7 @@ static void gen10_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>                GEN9_PGCTL_SSB_EU210_ACK |
>>                GEN9_PGCTL_SSB_EU311_ACK;
>>   -    for (s = 0; s < s_max; s++) {
>> +    for (s = 0; s < info->sseu.max_slices; s++) {
>>           if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
>>               /* skip disabled slice */
>>               continue;
>> @@ -4342,7 +4342,7 @@ static void gen10_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>           sseu->slice_mask |= BIT(s);
>>           sseu->subslices_mask[s] = info->sseu.subslices_mask[s];
>>   -        for (ss = 0; ss < ss_max; ss++) {
>> +        for (ss = 0; ss < info->sseu.max_subslices; ss++) {
>>               unsigned int eu_cnt;
>>                 if (!(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss))))
>> @@ -4362,17 +4362,11 @@ static void gen10_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>   static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
>>                       struct sseu_dev_info *sseu)
>>   {
>> -    int s_max = 3, ss_max = 4;
>> +    const struct intel_device_info *info = INTEL_INFO(dev_priv);
>>       int s, ss;
>> -    u32 s_reg[s_max], eu_reg[2*s_max], eu_mask[2];
>> -
>> -    /* BXT has a single slice and at most 3 subslices. */
>> -    if (IS_GEN9_LP(dev_priv)) {
>> -        s_max = 1;
>> -        ss_max = 3;
>> -    }
>> +    u32 s_reg[info->sseu.max_slices], 
>> eu_reg[2*info->sseu.max_subslices], eu_mask[2];
>
> Spaces around operators are preferred.

Done.

>
>>   -    for (s = 0; s < s_max; s++) {
>> +    for (s = 0; s < info->sseu.max_slices; s++) {
>>           s_reg[s] = I915_READ(GEN9_SLICE_PGCTL_ACK(s));
>>           eu_reg[2*s] = I915_READ(GEN9_SS01_EU_PGCTL_ACK(s));
>>           eu_reg[2*s + 1] = I915_READ(GEN9_SS23_EU_PGCTL_ACK(s));
>> @@ -4387,7 +4381,7 @@ static void gen9_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>                GEN9_PGCTL_SSB_EU210_ACK |
>>                GEN9_PGCTL_SSB_EU311_ACK;
>>   -    for (s = 0; s < s_max; s++) {
>> +    for (s = 0; s < info->sseu.max_slices; s++) {
>>           if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
>>               /* skip disabled slice */
>>               continue;
>> @@ -4398,7 +4392,7 @@ static void gen9_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>               sseu->subslices_mask[s] =
>> INTEL_INFO(dev_priv)->sseu.subslices_mask[s];
>>   -        for (ss = 0; ss < ss_max; ss++) {
>> +        for (ss = 0; ss < info->sseu.max_subslices; ss++) {
>>               unsigned int eu_cnt;
>>                 if (IS_GEN9_LP(dev_priv)) {
>>
>
> With the formatting tweaks,
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko
>

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

[PATCH 3/6] drm/i915/debugfs: add rcs topology entry

[Lionel Landwerlin]

While the end goal is to make this information available to userspace
through a new ioctl, there is no reason we can't display it in a human
readable fashion through debugfs.

slice0 (subslice_mask=0x7):
	subslice0:
		eu_mask: 0xff (8)
	subslice1:
		eu_mask: 0xff (8)
	subslice2:
		eu_mask: 0xff (8)
	subslice3:
		eu_mask: 0x0 (0)
slice1 (subslice_mask=0x7):
	subslice0:
		eu_mask: 0xff (8)
	subslice1:
		eu_mask: 0xff (8)
	subslice2:
		eu_mask: 0xff (8)
	subslice3:
		eu_mask: 0x0 (0)
slice2 (subslice_mask=0x7):
	subslice0:
		eu_mask: 0xff (8)
	subslice1:
		eu_mask: 0xff (8)
	subslice2:
		eu_mask: 0xff (8)
	subslice3:
		eu_mask: 0x0 (0)

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6ec7543e698f..79ca6e9f9ec9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3173,6 +3173,42 @@ static int i915_engine_info(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_rcs_topology(struct seq_file *m, void *unused)
+{
+	struct drm_i915_private *dev_priv = node_to_i915(m->private);
+	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
+	int s, ss;
+	int subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
+	int slice_stride = sseu->max_subslices * subslice_stride;
+
+	if (sseu->max_slices == 0) {
+		seq_printf(m, "Unavailable\n");
+		return 0;
+	}
+
+	for (s = 0; s < sseu->max_slices; s++) {
+		seq_printf(m, "slice%i (subslice_mask=0x%x):\n",
+			   s, sseu->subslices_mask[s]);
+
+		for (ss = 0; ss < slice_stride / subslice_stride; ss++) {
+			int eu, n_subslice_eus = 0;
+
+			seq_printf(m, "\tsubslice%i:\n", ss);
+
+			seq_printf(m, "\t\teu_mask:");
+			for (eu = 0; eu < subslice_stride; eu++) {
+				u8 val = sseu->eu_mask[s * slice_stride +
+						       ss * subslice_stride + eu];
+				seq_printf(m, " 0x%x", val);
+				n_subslice_eus += hweight8(val);
+			}
+			seq_printf(m, " (%i)\n", n_subslice_eus);
+		}
+	}
+
+	return 0;
+}
+
 static int i915_shrinker_info(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *i915 = node_to_i915(m->private);
@@ -4658,6 +4694,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dmc_info", i915_dmc_info, 0},
 	{"i915_display_info", i915_display_info, 0},
 	{"i915_engine_info", i915_engine_info, 0},
+	{"i915_rcs_topology", i915_rcs_topology, 0},
 	{"i915_shrinker_info", i915_shrinker_info, 0},
 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
-- 
2.15.1

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

Re: [PATCH 3/6] drm/i915/debugfs: add rcs topology entry

[Tvrtko Ursulin]

On 18/12/2017 15:35, Lionel Landwerlin wrote:
> While the end goal is to make this information available to userspace
> through a new ioctl, there is no reason we can't display it in a human
> readable fashion through debugfs.
> 
> slice0 (subslice_mask=0x7):

I'd add a subslice count while at it, since the eu lines have counts.

Bike-shedding on whether counts or masks are typically more important?

Slice0: 3 slices (0x7):
	Subslice 0: 8 EUs (0xff)
	Subslice 1: 8 EUs (0xff)
...

?

> 	subslice0:
> 		eu_mask: 0xff (8)
> 	subslice1:
> 		eu_mask: 0xff (8)
> 	subslice2:
> 		eu_mask: 0xff (8)
> 	subslice3:
> 		eu_mask: 0x0 (0)
> slice1 (subslice_mask=0x7):
> 	subslice0:
> 		eu_mask: 0xff (8)
> 	subslice1:
> 		eu_mask: 0xff (8)
> 	subslice2:
> 		eu_mask: 0xff (8)
> 	subslice3:
> 		eu_mask: 0x0 (0)
> slice2 (subslice_mask=0x7):
> 	subslice0:
> 		eu_mask: 0xff (8)
> 	subslice1:
> 		eu_mask: 0xff (8)
> 	subslice2:
> 		eu_mask: 0xff (8)
> 	subslice3:
> 		eu_mask: 0x0 (0)
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6ec7543e698f..79ca6e9f9ec9 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3173,6 +3173,42 @@ static int i915_engine_info(struct seq_file *m, void *unused)
>   	return 0;
>   }
>   
> +static int i915_rcs_topology(struct seq_file *m, void *unused)
> +{
> +	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> +	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
> +	int s, ss;
> +	int subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
> +	int slice_stride = sseu->max_subslices * subslice_stride;

Another case for the before mentioned helper for indexing into eu_mask 
array?

> +
> +	if (sseu->max_slices == 0) {
> +		seq_printf(m, "Unavailable\n");
> +		return 0;
> +	}

Is this possible?

> +
> +	for (s = 0; s < sseu->max_slices; s++) {
> +		seq_printf(m, "slice%i (subslice_mask=0x%x):\n",

%i always confuses me. Googling shows it is equivalent to %d for 
printing? Or is it something different in kernel space? If it is 
equivalent I would go with a more standard one. And I would even change 
to unsigned variables for iterators but I realize some people have a 
different opinion so up to you.

> +			   s, sseu->subslices_mask[s]);
> +
> +		for (ss = 0; ss < slice_stride / subslice_stride; ss++) {

With the indexing helpers hopefully it would be possible to simply 
iterate to hweight8(sseu->sublice_mask[s]) ?

> +			int eu, n_subslice_eus = 0;
> +
> +			seq_printf(m, "\tsubslice%i:\n", ss);
> +
> +			seq_printf(m, "\t\teu_mask:");
> +			for (eu = 0; eu < subslice_stride; eu++) {
> +				u8 val = sseu->eu_mask[s * slice_stride +
> +						       ss * subslice_stride + eu];
> +				seq_printf(m, " 0x%x", val);
> +				n_subslice_eus += hweight8(val);
> +			}
> +			seq_printf(m, " (%i)\n", n_subslice_eus);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static int i915_shrinker_info(struct seq_file *m, void *unused)
>   {
>   	struct drm_i915_private *i915 = node_to_i915(m->private);
> @@ -4658,6 +4694,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>   	{"i915_dmc_info", i915_dmc_info, 0},
>   	{"i915_display_info", i915_display_info, 0},
>   	{"i915_engine_info", i915_engine_info, 0},
> +	{"i915_rcs_topology", i915_rcs_topology, 0},
>   	{"i915_shrinker_info", i915_shrinker_info, 0},
>   	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
>   	{"i915_dp_mst_info", i915_dp_mst_info, 0},
> 

Regards,

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

Re: [PATCH 3/6] drm/i915/debugfs: add rcs topology entry

[Lionel Landwerlin]

On 11/01/18 11:31, Tvrtko Ursulin wrote:
>
> On 18/12/2017 15:35, Lionel Landwerlin wrote:
>> While the end goal is to make this information available to userspace
>> through a new ioctl, there is no reason we can't display it in a human
>> readable fashion through debugfs.
>>
>> slice0 (subslice_mask=0x7):
>
> I'd add a subslice count while at it, since the eu lines have counts.
>
> Bike-shedding on whether counts or masks are typically more important?
>
> Slice0: 3 slices (0x7):
>     Subslice 0: 8 EUs (0xff)
>     Subslice 1: 8 EUs (0xff)
> ...
>
> ?

Yeah, sure.

>
>>     subslice0:
>>         eu_mask: 0xff (8)
>>     subslice1:
>>         eu_mask: 0xff (8)
>>     subslice2:
>>         eu_mask: 0xff (8)
>>     subslice3:
>>         eu_mask: 0x0 (0)
>> slice1 (subslice_mask=0x7):
>>     subslice0:
>>         eu_mask: 0xff (8)
>>     subslice1:
>>         eu_mask: 0xff (8)
>>     subslice2:
>>         eu_mask: 0xff (8)
>>     subslice3:
>>         eu_mask: 0x0 (0)
>> slice2 (subslice_mask=0x7):
>>     subslice0:
>>         eu_mask: 0xff (8)
>>     subslice1:
>>         eu_mask: 0xff (8)
>>     subslice2:
>>         eu_mask: 0xff (8)
>>     subslice3:
>>         eu_mask: 0x0 (0)
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 37 
>> +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 6ec7543e698f..79ca6e9f9ec9 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3173,6 +3173,42 @@ static int i915_engine_info(struct seq_file 
>> *m, void *unused)
>>       return 0;
>>   }
>>   +static int i915_rcs_topology(struct seq_file *m, void *unused)
>> +{
>> +    struct drm_i915_private *dev_priv = node_to_i915(m->private);
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    int s, ss;
>> +    int subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
>> +    int slice_stride = sseu->max_subslices * subslice_stride;
>
> Another case for the before mentioned helper for indexing into eu_mask 
> array?

Done.

>
>> +
>> +    if (sseu->max_slices == 0) {
>> +        seq_printf(m, "Unavailable\n");
>> +        return 0;
>> +    }
>
> Is this possible?

Yeah, there are no registers to read on anything < gen8.
I've decided not to generate data there. Most userspaces already has the 
numbers from a table by pci-id.

>
>> +
>> +    for (s = 0; s < sseu->max_slices; s++) {
>> +        seq_printf(m, "slice%i (subslice_mask=0x%x):\n",
>
> %i always confuses me. Googling shows it is equivalent to %d for 
> printing? Or is it something different in kernel space? If it is 
> equivalent I would go with a more standard one. And I would even 
> change to unsigned variables for iterators but I realize some people 
> have a different opinion so up to you.

Yeah, I'm always using %i, but I must be the only one.
It's been easier to remember int -> %i.

>
>> +               s, sseu->subslices_mask[s]);
>> +
>> +        for (ss = 0; ss < slice_stride / subslice_stride; ss++) {
>
> With the indexing helpers hopefully it would be possible to simply 
> iterate to hweight8(sseu->sublice_mask[s]) ?

Actually I should use sseu->max_subslices.

>
>> +            int eu, n_subslice_eus = 0;
>> +
>> +            seq_printf(m, "\tsubslice%i:\n", ss);
>> +
>> +            seq_printf(m, "\t\teu_mask:");
>> +            for (eu = 0; eu < subslice_stride; eu++) {
>> +                u8 val = sseu->eu_mask[s * slice_stride +
>> +                               ss * subslice_stride + eu];
>> +                seq_printf(m, " 0x%x", val);
>> +                n_subslice_eus += hweight8(val);
>> +            }
>> +            seq_printf(m, " (%i)\n", n_subslice_eus);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int i915_shrinker_info(struct seq_file *m, void *unused)
>>   {
>>       struct drm_i915_private *i915 = node_to_i915(m->private);
>> @@ -4658,6 +4694,7 @@ static const struct drm_info_list 
>> i915_debugfs_list[] = {
>>       {"i915_dmc_info", i915_dmc_info, 0},
>>       {"i915_display_info", i915_display_info, 0},
>>       {"i915_engine_info", i915_engine_info, 0},
>> +    {"i915_rcs_topology", i915_rcs_topology, 0},
>>       {"i915_shrinker_info", i915_shrinker_info, 0},
>>       {"i915_shared_dplls_info", i915_shared_dplls_info, 0},
>>       {"i915_dp_mst_info", i915_dp_mst_info, 0},
>>
>
> Regards,
>
> Tvrtko
>

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

[PATCH 4/6] drm/i915: add rcs topology to error state

[Lionel Landwerlin]

This might be useful information for developers looking at an error
state.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index aba50aa613f1..ba0e4cd082c4 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -621,6 +621,40 @@ static void err_print_uc(struct drm_i915_error_state_buf *m,
 	print_error_obj(m, NULL, "GuC log buffer", error_uc->guc_log);
 }
 
+static void err_print_rcs_topology(struct drm_i915_error_state_buf *m,
+				   const struct sseu_dev_info *sseu)
+{
+	int s, ss;
+	int subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
+	int slice_stride = sseu->max_subslices * subslice_stride;
+
+	/* Unavailable prior to Gen 8. */
+	if (sseu->max_slices == 0)
+		return;
+
+	err_printf(m, "RCS topology:\n");
+
+	for (s = 0; s < sseu->max_slices; s++) {
+		err_printf(m, "  slice%i (subslice_mask=0x%x):\n",
+			   s, sseu->subslices_mask[s]);
+
+		for (ss = 0; ss < slice_stride / subslice_stride; ss++) {
+			int eu, n_subslice_eus = 0;
+
+			err_printf(m, "    subslice%i:\n", ss);
+
+			err_printf(m, "      eu_mask:");
+			for (eu = 0; eu < subslice_stride; eu++) {
+				u8 val = sseu->eu_mask[s * slice_stride +
+						       ss * subslice_stride + eu];
+				err_printf(m, " 0x%x", val);
+				n_subslice_eus += hweight8(val);
+			}
+			err_printf(m, " (%i)\n", n_subslice_eus);
+		}
+	}
+}
+
 int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 			    const struct i915_gpu_state *error)
 {
@@ -657,6 +691,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	err_printf(m, "Suspend count: %u\n", error->suspend_count);
 	err_printf(m, "Platform: %s\n", intel_platform_name(error->device_info.platform));
 	err_print_pciid(m, error->i915);
+	err_print_rcs_topology(m, &INTEL_INFO(dev_priv)->sseu);
 
 	err_printf(m, "IOMMU enabled?: %d\n", error->iommu);
 
-- 
2.15.1

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

Re: [PATCH 4/6] drm/i915: add rcs topology to error state

[Chris Wilson]

Quoting Lionel Landwerlin (2017-12-18 15:35:18)
> This might be useful information for developers looking at an error
> state.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index aba50aa613f1..ba0e4cd082c4 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -621,6 +621,40 @@ static void err_print_uc(struct drm_i915_error_state_buf *m,
>         print_error_obj(m, NULL, "GuC log buffer", error_uc->guc_log);
>  }
>  
> +static void err_print_rcs_topology(struct drm_i915_error_state_buf *m,
> +                                  const struct sseu_dev_info *sseu)
> +{
> +       int s, ss;
> +       int subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
> +       int slice_stride = sseu->max_subslices * subslice_stride;
> +
> +       /* Unavailable prior to Gen 8. */
> +       if (sseu->max_slices == 0)
> +               return;
> +
> +       err_printf(m, "RCS topology:\n");
> +
> +       for (s = 0; s < sseu->max_slices; s++) {
> +               err_printf(m, "  slice%i (subslice_mask=0x%x):\n",
> +                          s, sseu->subslices_mask[s]);
> +
> +               for (ss = 0; ss < slice_stride / subslice_stride; ss++) {
> +                       int eu, n_subslice_eus = 0;
> +
> +                       err_printf(m, "    subslice%i:\n", ss);
> +
> +                       err_printf(m, "      eu_mask:");
> +                       for (eu = 0; eu < subslice_stride; eu++) {
> +                               u8 val = sseu->eu_mask[s * slice_stride +
> +                                                      ss * subslice_stride + eu];
> +                               err_printf(m, " 0x%x", val);
> +                               n_subslice_eus += hweight8(val);
> +                       }
> +                       err_printf(m, " (%i)\n", n_subslice_eus);
> +               }
> +       }
> +}
> +
>  int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>                             const struct i915_gpu_state *error)
>  {
> @@ -657,6 +691,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>         err_printf(m, "Suspend count: %u\n", error->suspend_count);
>         err_printf(m, "Platform: %s\n", intel_platform_name(error->device_info.platform));
>         err_print_pciid(m, error->i915);
> +       err_print_rcs_topology(m, &INTEL_INFO(dev_priv)->sseu);

I don't think it's going to be that crucial (that it needs to be at the
start of the error-state, in your face). More likely down in the platform
capabilities after the user state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] drm/i915: add query uAPI

[Lionel Landwerlin]

There are a number of information that are readable from hardware
registers and that we would like to make accessible to userspace. One
particular example is the topology of the execution units (how are
execution units grouped in subslices and slices and also which ones
have been fused off for die recovery).

At the moment the GET_PARAM ioctl covers some basic needs, but
generally is only able to return a single value for each defined
parameter. This is a bit problematic with topology descriptions which
are array/maps of available units.

This change introduces a new ioctl that can deal with requests to fill
structures of potentially variable lengths. The user is expected fill
a query with length fields set at 0 on the first call, the kernel then
sets the length fields to the their expected values. A second call to
the kernel with length fields at their expected values will trigger a
copy of the data to the pointed memory locations.

The scope of this uAPI is only to provide information to userspace,
not to allow configuration of the device.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/Makefile     |  1 +
 drivers/gpu/drm/i915/i915_drv.c   |  1 +
 drivers/gpu/drm/i915/i915_drv.h   |  3 +++
 drivers/gpu/drm/i915/i915_query.c | 52 +++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h       | 31 +++++++++++++++++++++++
 5 files changed, 88 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/i915_query.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 091aef281963..9627e7e309dc 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -69,6 +69,7 @@ i915-y += i915_cmd_parser.o \
 	  i915_gem_timeline.o \
 	  i915_gem_userptr.o \
 	  i915_gemfs.o \
+	  i915_query.o \
 	  i915_trace_points.o \
 	  i915_vma.o \
 	  intel_breadcrumbs.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8b99e415c345..a90ed9f2b759 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2814,6 +2814,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 82fc59078c6a..3415a3d2399c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4084,6 +4084,9 @@ extern void i915_perf_fini(struct drm_i915_private *dev_priv);
 extern void i915_perf_register(struct drm_i915_private *dev_priv);
 extern void i915_perf_unregister(struct drm_i915_private *dev_priv);
 
+/* i915_query.c */
+int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
+
 /* i915_suspend.c */
 extern int i915_save_state(struct drm_i915_private *dev_priv);
 extern int i915_restore_state(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
new file mode 100644
index 000000000000..227a28978190
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "i915_drv.h"
+#include <uapi/drm/i915_drm.h>
+
+int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
+{
+	struct drm_i915_query *args = data;
+	u32 i;
+
+	for (i = 0; i < args->num_items; i++) {
+		struct drm_i915_query_item item;
+		u64 item_user_ptr = args->items_ptr + sizeof(item) * i;
+
+		if (copy_from_user(&item, u64_to_user_ptr(item_user_ptr),
+				   sizeof(item)))
+			return -EFAULT;
+
+		switch (item.query_id) {
+		default:
+			return -EINVAL;
+		}
+
+		if (copy_to_user(u64_to_user_ptr(item_user_ptr), &item,
+				 sizeof(item)))
+			return -EFAULT;
+	}
+
+	return 0;
+}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 536ee4febd74..87dd8b15548c 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -318,6 +318,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_PERF_OPEN		0x36
 #define DRM_I915_PERF_ADD_CONFIG	0x37
 #define DRM_I915_PERF_REMOVE_CONFIG	0x38
+#define DRM_I915_QUERY			0x39
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -375,6 +376,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_PERF_OPEN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
 #define DRM_IOCTL_I915_PERF_ADD_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
 #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)
+#define DRM_IOCTL_I915_QUERY			DRM_IOW(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1613,6 +1615,35 @@ struct drm_i915_perf_oa_config {
 	__u64 flex_regs_ptr;
 };
 
+
+struct drm_i915_query_item {
+	__u32 query_id;
+
+	/*
+	 * When left to 0 by userspace, this is filled with the size the data
+	 * to be written at the query_data pointer.
+	 */
+	__u32 length;
+
+	/*
+	 * Data will be written at the location pointed by data_ptr when the
+	 * value of length matches the length of the data to be written by the
+	 * kernel.
+	 */
+	__u64 data_ptr;
+};
+
+struct drm_i915_query {
+	__u32 num_items;
+	__u32 _pad;
+
+	/*
+	 * This points an array of struct drm_i915_query_item of length
+	 * num_items.
+	 */
+	__u64 items_ptr;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.15.1

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

Re: [PATCH 5/6] drm/i915: add query uAPI

[Tvrtko Ursulin]

On 18/12/2017 15:35, Lionel Landwerlin wrote:
> There are a number of information that are readable from hardware
> registers and that we would like to make accessible to userspace. One
> particular example is the topology of the execution units (how are
> execution units grouped in subslices and slices and also which ones
> have been fused off for die recovery).
> 
> At the moment the GET_PARAM ioctl covers some basic needs, but
> generally is only able to return a single value for each defined
> parameter. This is a bit problematic with topology descriptions which
> are array/maps of available units.
> 
> This change introduces a new ioctl that can deal with requests to fill
> structures of potentially variable lengths. The user is expected fill
> a query with length fields set at 0 on the first call, the kernel then
> sets the length fields to the their expected values. A second call to
> the kernel with length fields at their expected values will trigger a
> copy of the data to the pointed memory locations.
> 
> The scope of this uAPI is only to provide information to userspace,
> not to allow configuration of the device.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile     |  1 +
>   drivers/gpu/drm/i915/i915_drv.c   |  1 +
>   drivers/gpu/drm/i915/i915_drv.h   |  3 +++
>   drivers/gpu/drm/i915/i915_query.c | 52 +++++++++++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h       | 31 +++++++++++++++++++++++
>   5 files changed, 88 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/i915_query.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 091aef281963..9627e7e309dc 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -69,6 +69,7 @@ i915-y += i915_cmd_parser.o \
>   	  i915_gem_timeline.o \
>   	  i915_gem_userptr.o \
>   	  i915_gemfs.o \
> +	  i915_query.o \
>   	  i915_trace_points.o \
>   	  i915_vma.o \
>   	  intel_breadcrumbs.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8b99e415c345..a90ed9f2b759 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2814,6 +2814,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>   	DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>   };
>   
>   static struct drm_driver driver = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 82fc59078c6a..3415a3d2399c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4084,6 +4084,9 @@ extern void i915_perf_fini(struct drm_i915_private *dev_priv);
>   extern void i915_perf_register(struct drm_i915_private *dev_priv);
>   extern void i915_perf_unregister(struct drm_i915_private *dev_priv);
>   
> +/* i915_query.c */
> +int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
> +
>   /* i915_suspend.c */
>   extern int i915_save_state(struct drm_i915_private *dev_priv);
>   extern int i915_restore_state(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> new file mode 100644
> index 000000000000..227a28978190
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "i915_drv.h"
> +#include <uapi/drm/i915_drm.h>
> +
> +int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> +{
> +	struct drm_i915_query *args = data;

An alternative I think could be:

struct drm_i915_query_item __user *user_item_ptr = 
u64_to_user_ptr(args->items_ptr);

> +	u32 i;
> +
> +	for (i = 0; i < args->num_items; i++) {

i++, user_item_ptr++

> +		struct drm_i915_query_item item;
> +		u64 item_user_ptr = args->items_ptr + sizeof(item) * i;
So you can drop this line.

> +
> +		if (copy_from_user(&item, u64_to_user_ptr(item_user_ptr),
> +				   sizeof(item)))
> +			return -EFAULT;

And drop u64_to_user_ptr from here...

> +
> +		switch (item.query_id) {
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		if (copy_to_user(u64_to_user_ptr(item_user_ptr), &item,

... and here.

> +				 sizeof(item)))
> +			return -EFAULT;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 536ee4febd74..87dd8b15548c 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -318,6 +318,7 @@ typedef struct _drm_i915_sarea {
>   #define DRM_I915_PERF_OPEN		0x36
>   #define DRM_I915_PERF_ADD_CONFIG	0x37
>   #define DRM_I915_PERF_REMOVE_CONFIG	0x38
> +#define DRM_I915_QUERY			0x39
>   
>   #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
>   #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -375,6 +376,7 @@ typedef struct _drm_i915_sarea {
>   #define DRM_IOCTL_I915_PERF_OPEN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
>   #define DRM_IOCTL_I915_PERF_ADD_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
>   #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)
> +#define DRM_IOCTL_I915_QUERY			DRM_IOW(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query)

DRM_IOWR I think.

>   
>   /* Allow drivers to submit batchbuffers directly to hardware, relying
>    * on the security mechanisms provided by hardware.
> @@ -1613,6 +1615,35 @@ struct drm_i915_perf_oa_config {
>   	__u64 flex_regs_ptr;
>   };
>   
> +
> +struct drm_i915_query_item {
> +	__u32 query_id;

Maybe make it 64-bit, just in case we get a need to reserve blocks of 
bits so we have some more space then.

> +
> +	/*
> +	 * When left to 0 by userspace, this is filled with the size the data

s/left to 0/set to zero/?
s/the data/of the data/

> +	 * to be written at the query_data pointer.

s/query_data/data_ptr/ ? Or rename the field to query_data - I don't 
have a case for one versus the other.

> +	 */
> +	__u32 length;
> +
> +	/*
> +	 * Data will be written at the location pointed by data_ptr when the
> +	 * value of length matches the length of the data to be written by the
> +	 * kernel.
> +	 */
> +	__u64 data_ptr; > +};
> +
> +struct drm_i915_query {
> +	__u32 num_items;
> +	__u32 _pad;
> +
> +	/*
> +	 * This points an array of struct drm_i915_query_item of length

"This point to an array of num_items drm_i915_query_item structures" ?

> +	 * num_items.
> +	 */
> +	__u64 items_ptr;
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> 

But in general this works for me.

Can't remember who last time raised a complaint about dispatcher, within 
a dispatcher, within.., but we can't have it all. It's either nested 
dispatchers, or multiple single purpose ioctls, or sysfs. Executive 
decision time. :)

Again, this approach looks quite easy to extend, and with low 
boilerplate code requirements so it looks good to me.

Regards,

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

Re: [PATCH 5/6] drm/i915: add query uAPI

[Lionel Landwerlin]

Applied all of your comments here.

On 11/01/18 12:19, Tvrtko Ursulin wrote:
>
> On 18/12/2017 15:35, Lionel Landwerlin wrote:
>> There are a number of information that are readable from hardware
>> registers and that we would like to make accessible to userspace. One
>> particular example is the topology of the execution units (how are
>> execution units grouped in subslices and slices and also which ones
>> have been fused off for die recovery).
>>
>> At the moment the GET_PARAM ioctl covers some basic needs, but
>> generally is only able to return a single value for each defined
>> parameter. This is a bit problematic with topology descriptions which
>> are array/maps of available units.
>>
>> This change introduces a new ioctl that can deal with requests to fill
>> structures of potentially variable lengths. The user is expected fill
>> a query with length fields set at 0 on the first call, the kernel then
>> sets the length fields to the their expected values. A second call to
>> the kernel with length fields at their expected values will trigger a
>> copy of the data to the pointed memory locations.
>>
>> The scope of this uAPI is only to provide information to userspace,
>> not to allow configuration of the device.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile     |  1 +
>>   drivers/gpu/drm/i915/i915_drv.c   |  1 +
>>   drivers/gpu/drm/i915/i915_drv.h   |  3 +++
>>   drivers/gpu/drm/i915/i915_query.c | 52 
>> +++++++++++++++++++++++++++++++++++++++
>>   include/uapi/drm/i915_drm.h       | 31 +++++++++++++++++++++++
>>   5 files changed, 88 insertions(+)
>>   create mode 100644 drivers/gpu/drm/i915/i915_query.c
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile 
>> b/drivers/gpu/drm/i915/Makefile
>> index 091aef281963..9627e7e309dc 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -69,6 +69,7 @@ i915-y += i915_cmd_parser.o \
>>         i915_gem_timeline.o \
>>         i915_gem_userptr.o \
>>         i915_gemfs.o \
>> +      i915_query.o \
>>         i915_trace_points.o \
>>         i915_vma.o \
>>         intel_breadcrumbs.o \
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 8b99e415c345..a90ed9f2b759 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -2814,6 +2814,7 @@ static const struct drm_ioctl_desc 
>> i915_ioctls[] = {
>>       DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, 
>> DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, 
>> i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, 
>> i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>> +    DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, 
>> DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>   };
>>     static struct drm_driver driver = {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 82fc59078c6a..3415a3d2399c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -4084,6 +4084,9 @@ extern void i915_perf_fini(struct 
>> drm_i915_private *dev_priv);
>>   extern void i915_perf_register(struct drm_i915_private *dev_priv);
>>   extern void i915_perf_unregister(struct drm_i915_private *dev_priv);
>>   +/* i915_query.c */
>> +int i915_query_ioctl(struct drm_device *dev, void *data, struct 
>> drm_file *file);
>> +
>>   /* i915_suspend.c */
>>   extern int i915_save_state(struct drm_i915_private *dev_priv);
>>   extern int i915_restore_state(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> new file mode 100644
>> index 000000000000..227a28978190
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "i915_drv.h"
>> +#include <uapi/drm/i915_drm.h>
>> +
>> +int i915_query_ioctl(struct drm_device *dev, void *data, struct 
>> drm_file *file)
>> +{
>> +    struct drm_i915_query *args = data;
>
> An alternative I think could be:
>
> struct drm_i915_query_item __user *user_item_ptr = 
> u64_to_user_ptr(args->items_ptr);
>
>> +    u32 i;
>> +
>> +    for (i = 0; i < args->num_items; i++) {
>
> i++, user_item_ptr++
>
>> +        struct drm_i915_query_item item;
>> +        u64 item_user_ptr = args->items_ptr + sizeof(item) * i;
> So you can drop this line.
>
>> +
>> +        if (copy_from_user(&item, u64_to_user_ptr(item_user_ptr),
>> +                   sizeof(item)))
>> +            return -EFAULT;
>
> And drop u64_to_user_ptr from here...
>
>> +
>> +        switch (item.query_id) {
>> +        default:
>> +            return -EINVAL;
>> +        }
>> +
>> +        if (copy_to_user(u64_to_user_ptr(item_user_ptr), &item,
>
> ... and here.
>
>> +                 sizeof(item)))
>> +            return -EFAULT;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 536ee4febd74..87dd8b15548c 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -318,6 +318,7 @@ typedef struct _drm_i915_sarea {
>>   #define DRM_I915_PERF_OPEN        0x36
>>   #define DRM_I915_PERF_ADD_CONFIG    0x37
>>   #define DRM_I915_PERF_REMOVE_CONFIG    0x38
>> +#define DRM_I915_QUERY            0x39
>>     #define DRM_IOCTL_I915_INIT        DRM_IOW( DRM_COMMAND_BASE + 
>> DRM_I915_INIT, drm_i915_init_t)
>>   #define DRM_IOCTL_I915_FLUSH        DRM_IO ( DRM_COMMAND_BASE + 
>> DRM_I915_FLUSH)
>> @@ -375,6 +376,7 @@ typedef struct _drm_i915_sarea {
>>   #define DRM_IOCTL_I915_PERF_OPEN    DRM_IOW(DRM_COMMAND_BASE + 
>> DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
>>   #define DRM_IOCTL_I915_PERF_ADD_CONFIG DRM_IOW(DRM_COMMAND_BASE + 
>> DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
>>   #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG DRM_IOW(DRM_COMMAND_BASE 
>> + DRM_I915_PERF_REMOVE_CONFIG, __u64)
>> +#define DRM_IOCTL_I915_QUERY DRM_IOW(DRM_COMMAND_BASE + 
>> DRM_I915_QUERY, struct drm_i915_query)
>
> DRM_IOWR I think.
>
>>     /* Allow drivers to submit batchbuffers directly to hardware, 
>> relying
>>    * on the security mechanisms provided by hardware.
>> @@ -1613,6 +1615,35 @@ struct drm_i915_perf_oa_config {
>>       __u64 flex_regs_ptr;
>>   };
>>   +
>> +struct drm_i915_query_item {
>> +    __u32 query_id;
>
> Maybe make it 64-bit, just in case we get a need to reserve blocks of 
> bits so we have some more space then.
>
>> +
>> +    /*
>> +     * When left to 0 by userspace, this is filled with the size the 
>> data
>
> s/left to 0/set to zero/?
> s/the data/of the data/
>
>> +     * to be written at the query_data pointer.
>
> s/query_data/data_ptr/ ? Or rename the field to query_data - I don't 
> have a case for one versus the other.
>
>> +     */
>> +    __u32 length;
>> +
>> +    /*
>> +     * Data will be written at the location pointed by data_ptr when 
>> the
>> +     * value of length matches the length of the data to be written 
>> by the
>> +     * kernel.
>> +     */
>> +    __u64 data_ptr; > +};
>> +
>> +struct drm_i915_query {
>> +    __u32 num_items;
>> +    __u32 _pad;
>> +
>> +    /*
>> +     * This points an array of struct drm_i915_query_item of length
>
> "This point to an array of num_items drm_i915_query_item structures" ?
>
>> +     * num_items.
>> +     */
>> +    __u64 items_ptr;
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>>
>
> But in general this works for me.
>
> Can't remember who last time raised a complaint about dispatcher, 
> within a dispatcher, within.., but we can't have it all. It's either 
> nested dispatchers, or multiple single purpose ioctls, or sysfs. 
> Executive decision time. :)
>
> Again, this approach looks quite easy to extend, and with low 
> boilerplate code requirements so it looks good to me.
>
> Regards,
>
> Tvrtko
>

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

[PATCH 6/6] drm/i915: expose rcs topology through query uAPI

[Lionel Landwerlin]

With the introduction of asymetric slices in CNL, we cannot rely on
the previous SUBSLICE_MASK getparam to tell userspace what subslices
are available. Here we introduce a more detailed way of querying the
Gen's GPU topology that doesn't aggregate numbers.

This is essential for monitoring parts of the GPU with the OA unit,
because counters need to be normalized to the number of
EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do
not gives us sufficient information.

As a bonus we can draw representations of the GPU :

        https://imgur.com/a/vuqpa

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_query.c       | 134 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_query_info.c |  88 +++++++++++++++++++++
 include/uapi/drm/i915_drm.h             |  45 +++++++++++
 3 files changed, 267 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/intel_query_info.c

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 227a28978190..7c0eb09d3aac 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -25,24 +25,158 @@
 #include "i915_drv.h"
 #include <uapi/drm/i915_drm.h>
 
+static int query_slices_mask(struct drm_i915_private *dev_priv,
+			     struct drm_i915_query_item *query_item)
+{
+	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
+	struct drm_i915_query_slices_mask slices_info;
+	u32 data_length, length;
+
+	if (sseu->max_slices == 0)
+		return -ENODEV;
+
+	memset(&slices_info, 0, sizeof(slices_info));
+
+	slices_info.n_slices = sseu->max_slices;
+
+	data_length = sizeof(u8);
+	length = sizeof(struct drm_i915_query_slices_mask) + data_length;
+
+	/*
+	 * If we ever change the internal slice mask data type, we'll need to
+	 * update this function.
+	 */
+	BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
+
+	if (query_item->length == 0) {
+		query_item->length = length;
+		return 0;
+	}
+
+	if (query_item->length != length)
+		return -EINVAL;
+
+	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &slices_info,
+			 sizeof(slices_info)))
+		return -EFAULT;
+
+	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
+					 offsetof(struct drm_i915_query_slices_mask, data)),
+			 &sseu->slice_mask, data_length))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int query_subslices_mask(struct drm_i915_private *dev_priv,
+				struct drm_i915_query_item *query_item)
+{
+	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
+	struct drm_i915_query_subslices_mask subslices_info;
+	u32 data_length, length;
+
+	if (sseu->max_slices == 0)
+		return -ENODEV;
+
+	memset(&subslices_info, 0, sizeof(subslices_info));
+
+	subslices_info.n_slices = sseu->max_slices;
+	subslices_info.slice_stride = ALIGN(sseu->max_subslices, 8) / 8;
+
+	data_length = subslices_info.n_slices * subslices_info.slice_stride;
+	length = sizeof(struct drm_i915_query_subslices_mask) + data_length;
+
+	if (query_item->length == 0) {
+		query_item->length = length;
+		return 0;
+	}
+
+	if (query_item->length != length)
+		return -EINVAL;
+
+	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &subslices_info,
+			 sizeof(subslices_info)))
+		return -EFAULT;
+
+	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
+					 offsetof(struct drm_i915_query_subslices_mask, data)),
+			 sseu->subslices_mask, data_length))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int query_eus_mask(struct drm_i915_private *dev_priv,
+			  struct drm_i915_query_item *query_item)
+{
+	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
+	struct drm_i915_query_eus_mask eus_info;
+	u32 data_length, length;
+
+	if (sseu->max_slices == 0)
+		return -ENODEV;
+
+	memset(&eus_info, 0, sizeof(eus_info));
+
+	eus_info.subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
+	eus_info.slice_stride = sseu->max_subslices * eus_info.subslice_stride;
+	eus_info.n_slices = sseu->max_slices;
+
+	data_length = eus_info.n_slices * eus_info.slice_stride;
+	length = sizeof(struct drm_i915_query_eus_mask) + data_length;
+
+	if (query_item->length == 0) {
+		query_item->length = length;
+		return 0;
+	}
+
+	if (query_item->length != length)
+		return -EINVAL;
+
+	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &eus_info,
+			 sizeof(eus_info)))
+		return -EFAULT;
+
+	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
+					 offsetof(struct drm_i915_query_eus_mask, data)),
+			 sseu->eu_mask, data_length))
+		return -EFAULT;
+
+	return 0;
+}
+
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_query *args = data;
 	u32 i;
 
 	for (i = 0; i < args->num_items; i++) {
 		struct drm_i915_query_item item;
 		u64 item_user_ptr = args->items_ptr + sizeof(item) * i;
+		int ret;
 
 		if (copy_from_user(&item, u64_to_user_ptr(item_user_ptr),
 				   sizeof(item)))
 			return -EFAULT;
 
 		switch (item.query_id) {
+		case DRM_I915_QUERY_ID_SLICES_MASK:
+			ret = query_slices_mask(dev_priv, &item);
+			break;
+		case DRM_I915_QUERY_ID_SUBSLICES_MASK:
+			ret = query_subslices_mask(dev_priv, &item);
+			break;
+		case DRM_I915_QUERY_ID_EUS_MASK:
+			ret = query_eus_mask(dev_priv, &item);
+			break;
 		default:
 			return -EINVAL;
 		}
 
+		if (ret)
+			return ret;
+
 		if (copy_to_user(u64_to_user_ptr(item_user_ptr), &item,
 				 sizeof(item)))
 			return -EFAULT;
diff --git a/drivers/gpu/drm/i915/intel_query_info.c b/drivers/gpu/drm/i915/intel_query_info.c
new file mode 100644
index 000000000000..79b03be9f51a
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_query_info.c
@@ -0,0 +1,88 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "i915_drv.h"
+#include <uapi/drm/i915_drm.h>
+
+static int query_info_rcs_topology(struct drm_i915_private *dev_priv,
+				   struct drm_i915_query_info *args)
+{
+	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
+	struct drm_i915_rcs_topology_info __user *user_topology =
+		u64_to_user_ptr(args->info_ptr);
+	struct drm_i915_rcs_topology_info topology;
+	u32 data_size, total_size;
+	const u8 *data = NULL;
+	int ret;
+
+	/* Not supported on gen < 8. */
+	if (sseu->max_slices == 0)
+		return -ENODEV;
+
+	switch (args->query_params[0]) {
+	case I915_RCS_TOPOLOGY_SLICE:
+		topology.params[0] = sseu->max_slices;
+		data_size = sizeof(sseu->slice_mask);
+		data = &sseu->slice_mask;
+		break;
+
+	case I915_RCS_TOPOLOGY_SUBSLICE:
+		topology.params[0] = sseu->max_slices;
+		topology.params[1] = ALIGN(sseu->max_subslices, 8) / 8;
+		data_size = sseu->max_slices * topology.params[1];
+		data = sseu->subslices_mask;
+		break;
+
+	case I915_RCS_TOPOLOGY_EU:
+		topology.params[2] = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
+		topology.params[1] = sseu->max_subslices * topology.params[2];
+		topology.params[0] = sseu->max_slices;
+		data_size = sseu->max_slices * topology.params[1];
+		data = sseu->eu_mask;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	total_size = sizeof(topology) + data_size;
+
+	if (args->info_ptr_len == 0) {
+		args->info_ptr_len = total_size;
+		return 0;
+	}
+
+	if (args->info_ptr_len < total_size)
+		return -EINVAL;
+
+	ret = copy_to_user(user_topology, &topology, sizeof(topology));
+	if (ret)
+		return -EFAULT;
+
+	ret = copy_to_user(user_topology + 1, data, data_size);
+	if (ret)
+		return -EFAULT;
+
+	return 0;
+}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 87dd8b15548c..cdfbfcb123d0 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1618,6 +1618,9 @@ struct drm_i915_perf_oa_config {
 
 struct drm_i915_query_item {
 	__u32 query_id;
+#define DRM_I915_QUERY_ID_SLICES_MASK    0x01
+#define DRM_I915_QUERY_ID_SUBSLICES_MASK 0x02
+#define DRM_I915_QUERY_ID_EUS_MASK       0x03
 
 	/*
 	 * When left to 0 by userspace, this is filled with the size the data
@@ -1644,6 +1647,48 @@ struct drm_i915_query {
 	__u64 items_ptr;
 };
 
+/* Data written by the kernel with query DRM_I915_QUERY_ID_SLICES_MASK :
+ *
+ * data: each bit indicates whether a slice is available (1) or fused off
+ * (0). Formula to tell if slice X is available :
+ *
+ *         (data[X / 8] >> (X % 8)) & 1
+ */
+struct drm_i915_query_slices_mask {
+	__u32 n_slices;
+
+	__u8 data[];
+};
+
+/* Data written by the kernel with query DRM_I915_QUERY_ID_SUBSLICES_MASK :
+ *
+ * data: each bit indicates whether a subslice is available (1) or fused off
+ * (0). Formula to tell if slice X subslice Y is available :
+ *
+ *         (data[(X * slice_stride) + Y / 8] >> (Y % 8)) & 1
+ */
+struct drm_i915_query_subslices_mask {
+	__u32 n_slices;
+	__u32 slice_stride;
+
+	__u8 data[];
+};
+
+/* Data written by the kernel with query DRM_I915_QUERY_ID_EUS_MASK :
+ *
+ * data: Each bit indicates whether a subslice is available (1) or fused off
+ * (0). Formula to tell if slice X subslice Y eu Z is available :
+ *
+ *         (data[X * slice_stride + Y * subslice_stride + Z / 8] >> (Z % 8)) & 1
+ */
+struct drm_i915_query_eus_mask {
+	__u32 n_slices;
+	__u32 slice_stride;
+	__u32 subslice_stride;
+
+	__u8 data[];
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.15.1

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

Re: [PATCH 6/6] drm/i915: expose rcs topology through query uAPI

[Tvrtko Ursulin]

On 18/12/2017 15:35, Lionel Landwerlin wrote:
> With the introduction of asymetric slices in CNL, we cannot rely on

asymmetric

> the previous SUBSLICE_MASK getparam to tell userspace what subslices
> are available. Here we introduce a more detailed way of querying the
> Gen's GPU topology that doesn't aggregate numbers.
> 
> This is essential for monitoring parts of the GPU with the OA unit,
> because counters need to be normalized to the number of
> EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do
> not gives us sufficient information.
> 
> As a bonus we can draw representations of the GPU :
> 
>          https://imgur.com/a/vuqpa
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_query.c       | 134 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_query_info.c |  88 +++++++++++++++++++++
>   include/uapi/drm/i915_drm.h             |  45 +++++++++++
>   3 files changed, 267 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/intel_query_info.c
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 227a28978190..7c0eb09d3aac 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -25,24 +25,158 @@
>   #include "i915_drv.h"
>   #include <uapi/drm/i915_drm.h>
>   
> +static int query_slices_mask(struct drm_i915_private *dev_priv,
> +			     struct drm_i915_query_item *query_item)
> +{
> +	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
> +	struct drm_i915_query_slices_mask slices_info;
> +	u32 data_length, length;
> +
> +	if (sseu->max_slices == 0)
> +		return -ENODEV;

Do you need to handle this explicitly or just return all zeros? I guess 
I don't know which GPUs have zero slices.

> +
> +	memset(&slices_info, 0, sizeof(slices_info));

Is this needed since a) you will write to the only field just below, and 
b) it won't clear the tail data array anyway?

> +
> +	slices_info.n_slices = sseu->max_slices;
Move to after the query_item->length checks.

> +
> +	data_length = sizeof(u8);

Size is dependant on number of slices? DIV_ROUND_UP(n_slices, 
BITS_PER_BYTE) ?

> +	length = sizeof(struct drm_i915_query_slices_mask) + data_length;

sizeof(slices_info)

> +
> +	/*
> +	 * If we ever change the internal slice mask data type, we'll need to
> +	 * update this function.
> +	 */
> +	BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));

Hm a bit weak since it is not only about size but also data 
representation. But I don't have any better ideas.

> +
> +	if (query_item->length == 0) {
> +		query_item->length = length;
> +		return 0;
> +	}
> +
> +	if (query_item->length != length)
> +		return -EINVAL;
> +
> +	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &slices_info,
> +			 sizeof(slices_info)))
> +		return -EFAULT;
> +
> +	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
> +					 offsetof(struct drm_i915_query_slices_mask, data)),
> +			 &sseu->slice_mask, data_length))
> +		return -EFAULT;
> +
> +	return 0;
> +}

Some of the above comments also apply to the other two query functions 
below.

> +static int query_subslices_mask(struct drm_i915_private *dev_priv,
> +				struct drm_i915_query_item *query_item)
> +{
> +	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
> +	struct drm_i915_query_subslices_mask subslices_info;
> +	u32 data_length, length;
> +
> +	if (sseu->max_slices == 0)
> +		return -ENODEV;
> +
> +	memset(&subslices_info, 0, sizeof(subslices_info));
> +
> +	subslices_info.n_slices = sseu->max_slices;
> +	subslices_info.slice_stride = ALIGN(sseu->max_subslices, 8) / 8;
> +
> +	data_length = subslices_info.n_slices * subslices_info.slice_stride;
> +	length = sizeof(struct drm_i915_query_subslices_mask) + data_length;
> +
> +	if (query_item->length == 0) {
> +		query_item->length = length;
> +		return 0;
> +	}
> +
> +	if (query_item->length != length)
> +		return -EINVAL;
> +
> +	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &subslices_info,
> +			 sizeof(subslices_info)))
> +		return -EFAULT;
> +
> +	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
> +					 offsetof(struct drm_i915_query_subslices_mask, data)),
> +			 sseu->subslices_mask, data_length))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int query_eus_mask(struct drm_i915_private *dev_priv,
> +			  struct drm_i915_query_item *query_item)
> +{
> +	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
> +	struct drm_i915_query_eus_mask eus_info;
> +	u32 data_length, length;
> +
> +	if (sseu->max_slices == 0)
> +		return -ENODEV;
> +
> +	memset(&eus_info, 0, sizeof(eus_info));
> +
> +	eus_info.subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
> +	eus_info.slice_stride = sseu->max_subslices * eus_info.subslice_stride;
> +	eus_info.n_slices = sseu->max_slices;
> +
> +	data_length = eus_info.n_slices * eus_info.slice_stride;
> +	length = sizeof(struct drm_i915_query_eus_mask) + data_length;
> +
> +	if (query_item->length == 0) {
> +		query_item->length = length;
> +		return 0;
> +	}
> +
> +	if (query_item->length != length)
> +		return -EINVAL;
> +
> +	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &eus_info,
> +			 sizeof(eus_info)))
> +		return -EFAULT;
> +
> +	if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
> +					 offsetof(struct drm_i915_query_eus_mask, data)),
> +			 sseu->eu_mask, data_length))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
>   int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   {
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>   	struct drm_i915_query *args = data;
>   	u32 i;
>   
>   	for (i = 0; i < args->num_items; i++) {
>   		struct drm_i915_query_item item;
>   		u64 item_user_ptr = args->items_ptr + sizeof(item) * i;
> +		int ret;
>   
>   		if (copy_from_user(&item, u64_to_user_ptr(item_user_ptr),
>   				   sizeof(item)))
>   			return -EFAULT;
>   
>   		switch (item.query_id) {
> +		case DRM_I915_QUERY_ID_SLICES_MASK:
> +			ret = query_slices_mask(dev_priv, &item);
> +			break;
> +		case DRM_I915_QUERY_ID_SUBSLICES_MASK:
> +			ret = query_subslices_mask(dev_priv, &item);
> +			break;
> +		case DRM_I915_QUERY_ID_EUS_MASK:
> +			ret = query_eus_mask(dev_priv, &item);
> +			break;
>   		default:
>   			return -EINVAL;
>   		}
>   
> +		if (ret)
> +			return ret;
> +
>   		if (copy_to_user(u64_to_user_ptr(item_user_ptr), &item,
>   				 sizeof(item)))
>   			return -EFAULT;
> diff --git a/drivers/gpu/drm/i915/intel_query_info.c b/drivers/gpu/drm/i915/intel_query_info.c
> new file mode 100644
> index 000000000000..79b03be9f51a
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_query_info.c
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "i915_drv.h"
> +#include <uapi/drm/i915_drm.h>
> +
> +static int query_info_rcs_topology(struct drm_i915_private *dev_priv,
> +				   struct drm_i915_query_info *args)

Unused artefact of a previous version?

> +{
> +	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
> +	struct drm_i915_rcs_topology_info __user *user_topology =
> +		u64_to_user_ptr(args->info_ptr);
> +	struct drm_i915_rcs_topology_info topology;
> +	u32 data_size, total_size;
> +	const u8 *data = NULL;
> +	int ret;
> +
> +	/* Not supported on gen < 8. */
> +	if (sseu->max_slices == 0)
> +		return -ENODEV;
> +
> +	switch (args->query_params[0]) {
> +	case I915_RCS_TOPOLOGY_SLICE:
> +		topology.params[0] = sseu->max_slices;
> +		data_size = sizeof(sseu->slice_mask);
> +		data = &sseu->slice_mask;
> +		break;
> +
> +	case I915_RCS_TOPOLOGY_SUBSLICE:
> +		topology.params[0] = sseu->max_slices;
> +		topology.params[1] = ALIGN(sseu->max_subslices, 8) / 8;
> +		data_size = sseu->max_slices * topology.params[1];
> +		data = sseu->subslices_mask;
> +		break;
> +
> +	case I915_RCS_TOPOLOGY_EU:
> +		topology.params[2] = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
> +		topology.params[1] = sseu->max_subslices * topology.params[2];
> +		topology.params[0] = sseu->max_slices;
> +		data_size = sseu->max_slices * topology.params[1];
> +		data = sseu->eu_mask;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	total_size = sizeof(topology) + data_size;
> +
> +	if (args->info_ptr_len == 0) {
> +		args->info_ptr_len = total_size;
> +		return 0;
> +	}
> +
> +	if (args->info_ptr_len < total_size)
> +		return -EINVAL;
> +
> +	ret = copy_to_user(user_topology, &topology, sizeof(topology));
> +	if (ret)
> +		return -EFAULT;
> +
> +	ret = copy_to_user(user_topology + 1, data, data_size);
> +	if (ret)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 87dd8b15548c..cdfbfcb123d0 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1618,6 +1618,9 @@ struct drm_i915_perf_oa_config {
>   
>   struct drm_i915_query_item {
>   	__u32 query_id;
> +#define DRM_I915_QUERY_ID_SLICES_MASK    0x01
> +#define DRM_I915_QUERY_ID_SUBSLICES_MASK 0x02
> +#define DRM_I915_QUERY_ID_EUS_MASK       0x03
>   
>   	/*
>   	 * When left to 0 by userspace, this is filled with the size the data
> @@ -1644,6 +1647,48 @@ struct drm_i915_query {
>   	__u64 items_ptr;
>   };
>   
> +/* Data written by the kernel with query DRM_I915_QUERY_ID_SLICES_MASK :
> + *
> + * data: each bit indicates whether a slice is available (1) or fused off
> + * (0). Formula to tell if slice X is available :
> + *
> + *         (data[X / 8] >> (X % 8)) & 1

Add a helper taking struct drm_i915_query_slices_mask for userspace to 
do this. Macro would be good I think.

> + */
> +struct drm_i915_query_slices_mask {

Rename all uapi struct to _info instead of _mask suffix?

> +	__u32 n_slices;
> +
> +	__u8 data[];

Is a zero size array a GCC extension or something? I somehow seem to 
remember someone was complaining about this.

> +};
> +
> +/* Data written by the kernel with query DRM_I915_QUERY_ID_SUBSLICES_MASK :
> + *
> + * data: each bit indicates whether a subslice is available (1) or fused off
> + * (0). Formula to tell if slice X subslice Y is available :
> + *
> + *         (data[(X * slice_stride) + Y / 8] >> (Y % 8)) & 1
> + */
> +struct drm_i915_query_subslices_mask {
> +	__u32 n_slices;
> +	__u32 slice_stride;

I think it would be better to return subslice_max and also add a query 
helper like above.

> +
> +	__u8 data[];
> +};
> +
> +/* Data written by the kernel with query DRM_I915_QUERY_ID_EUS_MASK :
> + *
> + * data: Each bit indicates whether a subslice is available (1) or fused off
> + * (0). Formula to tell if slice X subslice Y eu Z is available :
> + *
> + *         (data[X * slice_stride + Y * subslice_stride + Z / 8] >> (Z % 8)) & 1
> + */
> +struct drm_i915_query_eus_mask {
> +	__u32 n_slices;
> +	__u32 slice_stride;
> +	__u32 subslice_stride;

Same as above, I think max_slices, max_sublices and a query helper would 
be much better.

> +
> +	__u8 data[];
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> 

Regards,

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

Re: [PATCH 6/6] drm/i915: expose rcs topology through query uAPI

[Lionel Landwerlin]

On 11/01/18 12:45, Tvrtko Ursulin wrote:
>
> On 18/12/2017 15:35, Lionel Landwerlin wrote:
>> With the introduction of asymetric slices in CNL, we cannot rely on
>
> asymmetric

Check ;)

>
>> the previous SUBSLICE_MASK getparam to tell userspace what subslices
>> are available. Here we introduce a more detailed way of querying the
>> Gen's GPU topology that doesn't aggregate numbers.
>>
>> This is essential for monitoring parts of the GPU with the OA unit,
>> because counters need to be normalized to the number of
>> EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do
>> not gives us sufficient information.
>>
>> As a bonus we can draw representations of the GPU :
>>
>>          https://imgur.com/a/vuqpa
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_query.c       | 134 
>> ++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_query_info.c |  88 +++++++++++++++++++++
>>   include/uapi/drm/i915_drm.h             |  45 +++++++++++
>>   3 files changed, 267 insertions(+)
>>   create mode 100644 drivers/gpu/drm/i915/intel_query_info.c
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index 227a28978190..7c0eb09d3aac 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -25,24 +25,158 @@
>>   #include "i915_drv.h"
>>   #include <uapi/drm/i915_drm.h>
>>   +static int query_slices_mask(struct drm_i915_private *dev_priv,
>> +                 struct drm_i915_query_item *query_item)
>> +{
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    struct drm_i915_query_slices_mask slices_info;
>> +    u32 data_length, length;
>> +
>> +    if (sseu->max_slices == 0)
>> +        return -ENODEV;
>
> Do you need to handle this explicitly or just return all zeros? I 
> guess I don't know which GPUs have zero slices.

Again, this is < gen8. I rather not care about those and return ENODEV.

>
>> +
>> +    memset(&slices_info, 0, sizeof(slices_info));
>
> Is this needed since a) you will write to the only field just below, 
> and b) it won't clear the tail data array anyway?

I don't know.
It's just being careful, this struct lives on the stack and I don't want 
to leak any data to userspace.

>
>> +
>> +    slices_info.n_slices = sseu->max_slices;
> Move to after the query_item->length checks.

Done.

>
>> +
>> +    data_length = sizeof(u8);
>
> Size is dependant on number of slices? DIV_ROUND_UP(n_slices, 
> BITS_PER_BYTE) ?

Done.

>
>> +    length = sizeof(struct drm_i915_query_slices_mask) + data_length;
>
> sizeof(slices_info)

Done.

>
>> +
>> +    /*
>> +     * If we ever change the internal slice mask data type, we'll 
>> need to
>> +     * update this function.
>> +     */
>> +    BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
>
> Hm a bit weak since it is not only about size but also data 
> representation. But I don't have any better ideas.
>
>> +
>> +    if (query_item->length == 0) {
>> +        query_item->length = length;
>> +        return 0;
>> +    }
>> +
>> +    if (query_item->length != length)
>> +        return -EINVAL;
>> +
>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), 
>> &slices_info,
>> +             sizeof(slices_info)))
>> +        return -EFAULT;
>> +
>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>> +                     offsetof(struct drm_i915_query_slices_mask, 
>> data)),
>> +             &sseu->slice_mask, data_length))
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>
> Some of the above comments also apply to the other two query functions 
> below.
>
>> +static int query_subslices_mask(struct drm_i915_private *dev_priv,
>> +                struct drm_i915_query_item *query_item)
>> +{
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    struct drm_i915_query_subslices_mask subslices_info;
>> +    u32 data_length, length;
>> +
>> +    if (sseu->max_slices == 0)
>> +        return -ENODEV;
>> +
>> +    memset(&subslices_info, 0, sizeof(subslices_info));
>> +
>> +    subslices_info.n_slices = sseu->max_slices;
>> +    subslices_info.slice_stride = ALIGN(sseu->max_subslices, 8) / 8;
>> +
>> +    data_length = subslices_info.n_slices * 
>> subslices_info.slice_stride;
>> +    length = sizeof(struct drm_i915_query_subslices_mask) + 
>> data_length;
>> +
>> +    if (query_item->length == 0) {
>> +        query_item->length = length;
>> +        return 0;
>> +    }
>> +
>> +    if (query_item->length != length)
>> +        return -EINVAL;
>> +
>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), 
>> &subslices_info,
>> +             sizeof(subslices_info)))
>> +        return -EFAULT;
>> +
>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>> +                     offsetof(struct drm_i915_query_subslices_mask, 
>> data)),
>> +             sseu->subslices_mask, data_length))
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>> +
>> +static int query_eus_mask(struct drm_i915_private *dev_priv,
>> +              struct drm_i915_query_item *query_item)
>> +{
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    struct drm_i915_query_eus_mask eus_info;
>> +    u32 data_length, length;
>> +
>> +    if (sseu->max_slices == 0)
>> +        return -ENODEV;
>> +
>> +    memset(&eus_info, 0, sizeof(eus_info));
>> +
>> +    eus_info.subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) 
>> / 8;
>> +    eus_info.slice_stride = sseu->max_subslices * 
>> eus_info.subslice_stride;
>> +    eus_info.n_slices = sseu->max_slices;
>> +
>> +    data_length = eus_info.n_slices * eus_info.slice_stride;
>> +    length = sizeof(struct drm_i915_query_eus_mask) + data_length;
>> +
>> +    if (query_item->length == 0) {
>> +        query_item->length = length;
>> +        return 0;
>> +    }
>> +
>> +    if (query_item->length != length)
>> +        return -EINVAL;
>> +
>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &eus_info,
>> +             sizeof(eus_info)))
>> +        return -EFAULT;
>> +
>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>> +                     offsetof(struct drm_i915_query_eus_mask, data)),
>> +             sseu->eu_mask, data_length))
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>> +
>>   int i915_query_ioctl(struct drm_device *dev, void *data, struct 
>> drm_file *file)
>>   {
>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>>       struct drm_i915_query *args = data;
>>       u32 i;
>>         for (i = 0; i < args->num_items; i++) {
>>           struct drm_i915_query_item item;
>>           u64 item_user_ptr = args->items_ptr + sizeof(item) * i;
>> +        int ret;
>>             if (copy_from_user(&item, u64_to_user_ptr(item_user_ptr),
>>                      sizeof(item)))
>>               return -EFAULT;
>>             switch (item.query_id) {
>> +        case DRM_I915_QUERY_ID_SLICES_MASK:
>> +            ret = query_slices_mask(dev_priv, &item);
>> +            break;
>> +        case DRM_I915_QUERY_ID_SUBSLICES_MASK:
>> +            ret = query_subslices_mask(dev_priv, &item);
>> +            break;
>> +        case DRM_I915_QUERY_ID_EUS_MASK:
>> +            ret = query_eus_mask(dev_priv, &item);
>> +            break;
>>           default:
>>               return -EINVAL;
>>           }
>>   +        if (ret)
>> +            return ret;
>> +
>>           if (copy_to_user(u64_to_user_ptr(item_user_ptr), &item,
>>                    sizeof(item)))
>>               return -EFAULT;
>> diff --git a/drivers/gpu/drm/i915/intel_query_info.c 
>> b/drivers/gpu/drm/i915/intel_query_info.c
>> new file mode 100644
>> index 000000000000..79b03be9f51a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_query_info.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "i915_drv.h"
>> +#include <uapi/drm/i915_drm.h>
>> +
>> +static int query_info_rcs_topology(struct drm_i915_private *dev_priv,
>> +                   struct drm_i915_query_info *args)
>
> Unused artefact of a previous version?

Dammit...

>
>> +{
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    struct drm_i915_rcs_topology_info __user *user_topology =
>> +        u64_to_user_ptr(args->info_ptr);
>> +    struct drm_i915_rcs_topology_info topology;
>> +    u32 data_size, total_size;
>> +    const u8 *data = NULL;
>> +    int ret;
>> +
>> +    /* Not supported on gen < 8. */
>> +    if (sseu->max_slices == 0)
>> +        return -ENODEV;
>> +
>> +    switch (args->query_params[0]) {
>> +    case I915_RCS_TOPOLOGY_SLICE:
>> +        topology.params[0] = sseu->max_slices;
>> +        data_size = sizeof(sseu->slice_mask);
>> +        data = &sseu->slice_mask;
>> +        break;
>> +
>> +    case I915_RCS_TOPOLOGY_SUBSLICE:
>> +        topology.params[0] = sseu->max_slices;
>> +        topology.params[1] = ALIGN(sseu->max_subslices, 8) / 8;
>> +        data_size = sseu->max_slices * topology.params[1];
>> +        data = sseu->subslices_mask;
>> +        break;
>> +
>> +    case I915_RCS_TOPOLOGY_EU:
>> +        topology.params[2] = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
>> +        topology.params[1] = sseu->max_subslices * topology.params[2];
>> +        topology.params[0] = sseu->max_slices;
>> +        data_size = sseu->max_slices * topology.params[1];
>> +        data = sseu->eu_mask;
>> +        break;
>> +
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    total_size = sizeof(topology) + data_size;
>> +
>> +    if (args->info_ptr_len == 0) {
>> +        args->info_ptr_len = total_size;
>> +        return 0;
>> +    }
>> +
>> +    if (args->info_ptr_len < total_size)
>> +        return -EINVAL;
>> +
>> +    ret = copy_to_user(user_topology, &topology, sizeof(topology));
>> +    if (ret)
>> +        return -EFAULT;
>> +
>> +    ret = copy_to_user(user_topology + 1, data, data_size);
>> +    if (ret)
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 87dd8b15548c..cdfbfcb123d0 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -1618,6 +1618,9 @@ struct drm_i915_perf_oa_config {
>>     struct drm_i915_query_item {
>>       __u32 query_id;
>> +#define DRM_I915_QUERY_ID_SLICES_MASK    0x01
>> +#define DRM_I915_QUERY_ID_SUBSLICES_MASK 0x02
>> +#define DRM_I915_QUERY_ID_EUS_MASK       0x03
>>         /*
>>        * When left to 0 by userspace, this is filled with the size 
>> the data
>> @@ -1644,6 +1647,48 @@ struct drm_i915_query {
>>       __u64 items_ptr;
>>   };
>>   +/* Data written by the kernel with query 
>> DRM_I915_QUERY_ID_SLICES_MASK :
>> + *
>> + * data: each bit indicates whether a slice is available (1) or 
>> fused off
>> + * (0). Formula to tell if slice X is available :
>> + *
>> + *         (data[X / 8] >> (X % 8)) & 1
>
> Add a helper taking struct drm_i915_query_slices_mask for userspace to 
> do this. Macro would be good I think.

Done.

>
>> + */
>> +struct drm_i915_query_slices_mask {
>
> Rename all uapi struct to _info instead of _mask suffix?

Sure.

>
>> +    __u32 n_slices;
>> +
>> +    __u8 data[];
>
> Is a zero size array a GCC extension or something? I somehow seem to 
> remember someone was complaining about this.

[0] is a GNU C extension
[] is a flexible array in C99

>
>> +};
>> +
>> +/* Data written by the kernel with query 
>> DRM_I915_QUERY_ID_SUBSLICES_MASK :
>> + *
>> + * data: each bit indicates whether a subslice is available (1) or 
>> fused off
>> + * (0). Formula to tell if slice X subslice Y is available :
>> + *
>> + *         (data[(X * slice_stride) + Y / 8] >> (Y % 8)) & 1
>> + */
>> +struct drm_i915_query_subslices_mask {
>> +    __u32 n_slices;
>> +    __u32 slice_stride;
>
> I think it would be better to return subslice_max and also add a query 
> helper like above.

Okay.

>
>> +
>> +    __u8 data[];
>> +};
>> +
>> +/* Data written by the kernel with query DRM_I915_QUERY_ID_EUS_MASK :
>> + *
>> + * data: Each bit indicates whether a subslice is available (1) or 
>> fused off
>> + * (0). Formula to tell if slice X subslice Y eu Z is available :
>> + *
>> + *         (data[X * slice_stride + Y * subslice_stride + Z / 8] >> 
>> (Z % 8)) & 1
>> + */
>> +struct drm_i915_query_eus_mask {
>> +    __u32 n_slices;
>> +    __u32 slice_stride;
>> +    __u32 subslice_stride;
>
> Same as above, I think max_slices, max_sublices and a query helper 
> would be much better.

Okay, it will require max_eus_per_subslice though.

>
>> +
>> +    __u8 data[];
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>>
>
> Regards,
>
> Tvrtko
>

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

Re: [PATCH 6/6] drm/i915: expose rcs topology through query uAPI

[Tvrtko Ursulin]

On 11/01/2018 18:38, Lionel Landwerlin wrote:
> On 11/01/18 12:45, Tvrtko Ursulin wrote:
>>

[snip]

>>
>>> +    __u32 n_slices;
>>> +
>>> +    __u8 data[];
>>
>> Is a zero size array a GCC extension or something? I somehow seem to 
>> remember someone was complaining about this.
> 
> [0] is a GNU C extension
> [] is a flexible array in C99

Right.. hm.. I was pretty sure both were not acceptable, but, when I 
look into include/uapi I see that there are both actually in at least 
some use. So I don't know.. fine I guess, but don't be surprised if 
someone complains.

Regards,

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

✗ Fi.CI.BAT: warning for drm/i915: expose RCS topology to userspace

[Patchwork]

== Series Details ==

Series: drm/i915: expose RCS topology to userspace
URL   : https://patchwork.freedesktop.org/series/35519/
State : warning

== Summary ==

Series 35519v1 drm/i915: expose RCS topology to userspace
https://patchwork.freedesktop.org/api/1.0/series/35519/revisions/1/mbox/

Test kms_busy:
        Subgroup basic-flip-a:
                pass       -> DMESG-WARN (fi-elk-e7500) fdo#103989
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (fi-hsw-4770)
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                pass       -> SKIP       (fi-snb-2600)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (fi-bdw-5557u) fdo#104162
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104162 https://bugs.freedesktop.org/show_bug.cgi?id=104162
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:444s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:498s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:276s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:497s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:499s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:479s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:467s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:264s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:528s
fi-hsw-4770      total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:401s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:414s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:387s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:476s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:429s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:473s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:515s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:466s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:520s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:586s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:442s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:533s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:558s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:503s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:509s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:455s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:547s
fi-snb-2600      total:288  pass:247  dwarn:0   dfail:0   fail:0   skip:41  time:410s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:587s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:618s
fi-glk-dsi       total:288  pass:178  dwarn:1   dfail:4   fail:0   skip:105 time:343s

913d6a0d4d78fba4cb62a1ac14d539d959fe422a drm-tip: 2017y-12m-18d-13h-25m-28s UTC integration manifest
647a50323213 drm/i915: expose rcs topology through query uAPI
2b45997096c4 drm/i915: add query uAPI
c81319176c97 drm/i915: add rcs topology to error state
17481e1146b6 drm/i915/debugfs: add rcs topology entry
fe927a0631fb drm/i915/debugfs: reuse max slice/subslices already stored in sseu
da15e9f295cf drm/i915: store all subslice masks

== Logs ==

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

Re: [PATCH 0/6] drm/i915: expose RCS topology to userspace

[Lionel Landwerlin]

Hey Tvrtko,

I've had only one comment from Chris on this series (addressed locally).
Since atm you might be the only other person in need of this API, do you 
have any feedback on it?

Thanks,

-
Lionel

On 18/12/17 15:35, Lionel Landwerlin wrote:
> Hi all,
>
> This series a respin of a few attempts ([1], [2], [3]) to expose RCS
> topology to userspace.
>
> The motivation for this is to be able to monitor part of the GPU using
> i915 perf and have detailled knowledge about what execution units are
> fused off. Let's say you monitor slice 1, knowing how many EUs are
> actually usuable on that slice is critical to provide normalized
> numbers (like average busyness of the EUs on slice1) on that
> particular part of the GPU. Currently numbers returned like
> I915_PARAM_EU_TOTAL don't give us that information.
>
> This series introduces a new uAPI to query information from i915. It's
> partially inspired by the amdgpu_cs_ioctl, although we don't allow
> setting values onto the device, only queries. There is a good chance
> this new uAPI will be reused for exposing available engines (Tvrtko
> was looking into this).
>
> Thanks,
>
> [1]: https://patchwork.freedesktop.org/patch/185959/
> [2]: https://patchwork.freedesktop.org/series/33436/
> [3]: https://patchwork.freedesktop.org/series/33950/
>
> Lionel Landwerlin (6):
>    drm/i915: store all subslice masks
>    drm/i915/debugfs: reuse max slice/subslices already stored in sseu
>    drm/i915/debugfs: add rcs topology entry
>    drm/i915: add rcs topology to error state
>    drm/i915: add query uAPI
>    drm/i915: expose rcs topology through query uAPI
>
>   drivers/gpu/drm/i915/Makefile            |   1 +
>   drivers/gpu/drm/i915/i915_debugfs.c      |  87 ++++++++++-----
>   drivers/gpu/drm/i915/i915_drv.c          |   3 +-
>   drivers/gpu/drm/i915/i915_drv.h          |  26 ++++-
>   drivers/gpu/drm/i915/i915_gpu_error.c    |  35 ++++++
>   drivers/gpu/drm/i915/i915_query.c        | 186 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_device_info.c | 169 ++++++++++++++++++++--------
>   drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
>   drivers/gpu/drm/i915/intel_query_info.c  |  88 +++++++++++++++
>   drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
>   include/uapi/drm/i915_drm.h              |  76 +++++++++++++
>   11 files changed, 598 insertions(+), 77 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/i915_query.c
>   create mode 100644 drivers/gpu/drm/i915/intel_query_info.c
>
> --
> 2.15.1
>

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