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

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

[Lionel Landwerlin]

Hi all,

Here is another iteration on this series. Most changes are pretty
cosmetic (slightly different helper in patch1, factored code in patch
3 & 4, etc...).

Thanks for your feedback!

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      |  63 +++++----
 drivers/gpu/drm/i915/i915_drv.c          |   3 +-
 drivers/gpu/drm/i915/i915_drv.h          |   3 +
 drivers/gpu/drm/i915/i915_gpu_error.c    |   9 ++
 drivers/gpu/drm/i915/i915_query.c        | 185 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_device_info.c | 225 ++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_device_info.h |  49 ++++++-
 drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
 include/uapi/drm/i915_drm.h              |  84 ++++++++++++
 11 files changed, 547 insertions(+), 79 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_query.c

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

[PATCH v4 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 asymmetric (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.

v2: Rework how we store total numbers in sseu_dev_info (Tvrtko)
    Fix CHV eu masks, was reading disabled as enabled (Tvrtko)
    Readability changes (Tvrtko)
    Add EU index helper (Tvrtko)

v3: Turn ALIGN(v, 8) / 8 into DIV_ROUND_UP(v, BITS_PER_BYTE) (Tvrtko)
    Reuse sseu_eu_idx() for setting eu_mask on CHV (Tvrtko)
    Reformat debug prints for subslices (Tvrtko)

v4: Change eu_mask helper into sseu_set_eus() (Tvrtko)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c      |  25 ++--
 drivers/gpu/drm/i915/i915_drv.c          |   2 +-
 drivers/gpu/drm/i915/intel_device_info.c | 201 +++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_device_info.h |  47 +++++++-
 drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
 6 files changed, 216 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index cc659b4b2a45..684551114965 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4289,7 +4289,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->subslice_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) +
@@ -4336,7 +4336,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->subslice_mask[s] = info->sseu.subslice_mask[s];
 
 		for (ss = 0; ss < ss_max; ss++) {
 			unsigned int eu_cnt;
@@ -4391,8 +4391,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->subslice_mask[s] =
+				INTEL_INFO(dev_priv)->sseu.subslice_mask[s];
 
 		for (ss = 0; ss < ss_max; ss++) {
 			unsigned int eu_cnt;
@@ -4402,7 +4402,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 					/* skip disabled subslice */
 					continue;
 
-				sseu->subslice_mask |= BIT(ss);
+				sseu->subslice_mask[s] |= BIT(ss);
 			}
 
 			eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
@@ -4424,9 +4424,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->subslice_mask[s] =
+				INTEL_INFO(dev_priv)->sseu.subslice_mask[s];
+		}
 		sseu->eu_total = sseu->eu_per_subslice *
 				 sseu_subslice_total(sseu);
 
@@ -4445,6 +4448,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);
@@ -4452,10 +4456,11 @@ 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 %u subslices, mask=%04x\n", type,
+			   s, hweight8(sseu->subslice_mask[s]),
+			   sseu->subslice_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 6c8da9d20c33..969835d3cbcd 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.subslice_mask[0];
 		if (!value)
 			return -ENODEV;
 		break;
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index d28592e43512..ed14994527fc 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -80,12 +80,16 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
 
 static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p)
 {
+	int s;
+
 	drm_printf(p, "slice mask: %04x\n", sseu->slice_mask);
 	drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask));
 	drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu));
-	drm_printf(p, "subslice mask %04x\n", sseu->subslice_mask);
-	drm_printf(p, "subslice per slice: %u\n",
-		   hweight8(sseu->subslice_mask));
+	for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) {
+		drm_printf(p, "slice%d %u subslices mask=%04x\n",
+			   s, hweight8(sseu->subslice_mask[s]),
+			   sseu->subslice_mask[s]);
+	}
 	drm_printf(p, "EU total: %u\n", sseu->eu_total);
 	drm_printf(p, "EU per subslice: %u\n", sseu->eu_per_subslice);
 	drm_printf(p, "has slice power gating: %s\n",
@@ -119,22 +123,87 @@ void intel_device_info_dump(const struct intel_device_info *info,
 	intel_device_info_dump_flags(info, p);
 }
 
+static u16 compute_eu_total(const struct sseu_dev_info *sseu)
+{
+	u16 i, total = 0;
+
+	for (i = 0; i < ARRAY_SIZE(sseu->eu_mask); i++)
+		total += hweight8(sseu->eu_mask[i]);
+
+	return total;
+}
+
+static u16 compute_subslice_total(const struct sseu_dev_info *sseu)
+{
+	u16 i, total = 0;
+
+	for (i = 0; i < ARRAY_SIZE(sseu->subslice_mask); i++)
+		total += hweight8(sseu->subslice_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;
+	const int 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;
 
-	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));
+	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->subslice_mask[0] = subslice_mask;
+	for (s = 1; s < sseu->max_slices; s++)
+		sseu->subslice_mask[s] = subslice_mask & 0x3;
+
+	/* Slice0 */
+	eu_en = ~I915_READ(GEN8_EU_DISABLE0);
+	for (ss = 0; ss < sseu->max_subslices; ss++)
+		sseu_set_eus(sseu, 0, ss, (eu_en >> (8 * ss)) & eu_mask);
+	/* Slice1 */
+	sseu_set_eus(sseu, 1, 0, (eu_en >> 24) & eu_mask);
+	eu_en = ~I915_READ(GEN8_EU_DISABLE1);
+	sseu_set_eus(sseu, 1, 1, eu_en & eu_mask);
+	/* Slice2 */
+	sseu_set_eus(sseu, 2, 0, (eu_en >> 8) & eu_mask);
+	sseu_set_eus(sseu, 2, 1, (eu_en >> 16) & eu_mask);
+	/* Slice3 */
+	sseu_set_eus(sseu, 3, 0, (eu_en >> 24) & eu_mask);
+	eu_en = ~I915_READ(GEN8_EU_DISABLE2);
+	sseu_set_eus(sseu, 3, 1, eu_en & eu_mask);
+	/* Slice4 */
+	sseu_set_eus(sseu, 4, 0, (eu_en >> 8) & eu_mask);
+	sseu_set_eus(sseu, 4, 1, (eu_en >> 16) & eu_mask);
+	/* Slice5 */
+	sseu_set_eus(sseu, 5, 0, (eu_en >> 24) & eu_mask);
+	eu_en = ~I915_READ(GEN10_EU_DISABLE3);
+	sseu_set_eus(sseu, 5, 1, eu_en & eu_mask);
+
+	/* Do a second pass where we mark 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_get_eus(sseu, s, ss) == 0)
+				sseu->subslice_mask[s] &= ~BIT(ss);
+		}
+	}
+
+	sseu->subslice_total = compute_subslice_total(sseu);
+	sseu->eu_total = compute_eu_total(sseu);
 
 	/*
 	 * CNL is expected to always have a uniform distribution
@@ -155,26 +224,40 @@ 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);
+		u8 disabled_mask =
+			((fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >>
+			 CHV_FGT_EU_DIS_SS0_R0_SHIFT) |
+			(((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >>
+			  CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4);
+
+		sseu->subslice_mask[0] |= BIT(0);
+		sseu_set_eus(sseu, 0, 0, ~disabled_mask);
 	}
 
 	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);
+		u8 disabled_mask =
+			((fuse & CHV_FGT_EU_DIS_SS1_R0_MASK) >>
+			 CHV_FGT_EU_DIS_SS1_R0_SHIFT) |
+			(((fuse & CHV_FGT_EU_DIS_SS1_R1_MASK) >>
+			  CHV_FGT_EU_DIS_SS1_R1_SHIFT) << 4);
+
+		sseu->subslice_mask[0] |= BIT(1);
+		sseu_set_eus(sseu, 0, 1, ~disabled_mask);
 	}
 
+	sseu->subslice_total = compute_subslice_total(sseu);
+	sseu->eu_total = compute_eu_total(sseu);
+
 	/*
 	 * CHV expected to always have a uniform distribution of EU
 	 * across subslices.
@@ -196,41 +279,52 @@ 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;
-	u8 eu_mask = 0xff;
+	u32 fuse2, eu_disable, subslice_mask;
+	const 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->subslice_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;
+			u8 eu_disabled_mask;
 
-			if (!(sseu->subslice_mask & BIT(ss)))
+			if (!(sseu->subslice_mask[s] & BIT(ss)))
 				/* skip disabled subslice */
 				continue;
 
-			eu_per_ss = eu_max - hweight8((eu_disable >> (ss*8)) &
-						      eu_mask);
+			eu_disabled_mask = (eu_disable >> (ss*8)) & eu_mask;
+
+			sseu_set_eus(sseu, s, ss, ~eu_disabled_mask);
+
+			eu_per_ss = sseu->max_eus_per_subslice -
+				hweight8(eu_disabled_mask);
 
 			/*
 			 * Record which subslice(s) has(have) 7 EUs. we
@@ -239,11 +333,12 @@ 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->subslice_total = compute_subslice_total(sseu);
+	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
@@ -269,8 +364,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->subslice_mask[0] & BIT(ss)))
+		info->has_pooled_eu = hweight8(sseu->subslice_mask[0]) == 3;
 
 		sseu->min_eu_in_pool = 0;
 		if (info->has_pooled_eu) {
@@ -288,19 +383,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) |
@@ -314,30 +412,39 @@ 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->subslice_mask[s] = subslice_mask;
+
+		for (ss = 0; ss < sseu->max_subslices; ss++) {
+			u8 eu_disabled_mask;
 			u32 n_disabled;
 
-			if (!(sseu->subslice_mask & BIT(ss)))
+			if (!(sseu->subslice_mask[ss] & BIT(ss)))
 				/* skip disabled subslice */
 				continue;
 
-			n_disabled = hweight8(eu_disable[s] >> (ss * eu_max));
+			eu_disabled_mask =
+				eu_disable[s] >> (ss * sseu->max_eus_per_subslice);
+
+			sseu_set_eus(sseu, s, ss, ~eu_disabled_mask);
+
+			n_disabled = hweight8(eu_disabled_mask);
 
 			/*
 			 * 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->subslice_total = compute_subslice_total(sseu);
+	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
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 49cb27bd04c1..36e0df87862d 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -110,10 +110,14 @@ enum intel_platform {
 	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 eu_total;
+	u8 subslice_mask[GEN_MAX_SUBSLICES];
+	u16 subslice_total;
+	u16 eu_total;
 	u8 eu_per_subslice;
 	u8 min_eu_in_pool;
 	/* For each slice, which subslice(s) has(have) 7 EUs (bitfield)? */
@@ -121,6 +125,17 @@ 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];
 };
 
 struct intel_device_info {
@@ -167,7 +182,33 @@ struct intel_device_info {
 
 static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
 {
-	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
+	return sseu->subslice_total;
+}
+
+static inline int sseu_eu_idx(const struct sseu_dev_info *sseu,
+			      int slice, int subslice)
+{
+	int subslice_stride = DIV_ROUND_UP(sseu->max_eus_per_subslice,
+					   BITS_PER_BYTE);
+	int slice_stride = sseu->max_subslices * subslice_stride;
+
+	return slice * slice_stride + subslice * subslice_stride;
+}
+
+/*
+ * The following functions prototypes should be updated with a larger type
+ * than u8 if we ever have more than 8 EUs per subslice.
+ */
+static inline u8 sseu_get_eus(const struct sseu_dev_info *sseu,
+			      int slice, int subslice)
+{
+	return sseu->eu_mask[sseu_eu_idx(sseu, slice, subslice)];
+}
+
+static inline void sseu_set_eus(struct sseu_dev_info *sseu,
+				int slice, int subslice, u8 eu_mask)
+{
+	sseu->eu_mask[sseu_eu_idx(sseu, slice, subslice)] = eu_mask;
 }
 
 const char *intel_platform_name(enum intel_platform platform);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ff25f209d0a5..ac7896031b8d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2098,7 +2098,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.subslice_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..23ae9a957fab 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.subslice_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 v4 1/6] drm/i915: store all subslice masks

[Tvrtko Ursulin]

On 15/01/2018 14:41, Lionel Landwerlin wrote:
> Up to now, subslice mask was assumed to be uniform across slices. But
> starting with Cannonlake, slices can be asymmetric (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.
> 
> v2: Rework how we store total numbers in sseu_dev_info (Tvrtko)
>      Fix CHV eu masks, was reading disabled as enabled (Tvrtko)
>      Readability changes (Tvrtko)
>      Add EU index helper (Tvrtko)
> 
> v3: Turn ALIGN(v, 8) / 8 into DIV_ROUND_UP(v, BITS_PER_BYTE) (Tvrtko)
>      Reuse sseu_eu_idx() for setting eu_mask on CHV (Tvrtko)
>      Reformat debug prints for subslices (Tvrtko)
> 
> v4: Change eu_mask helper into sseu_set_eus() (Tvrtko)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c      |  25 ++--
>   drivers/gpu/drm/i915/i915_drv.c          |   2 +-
>   drivers/gpu/drm/i915/intel_device_info.c | 201 +++++++++++++++++++++++--------
>   drivers/gpu/drm/i915/intel_device_info.h |  47 +++++++-
>   drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
>   drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
>   6 files changed, 216 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index cc659b4b2a45..684551114965 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4289,7 +4289,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->subslice_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) +
> @@ -4336,7 +4336,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->subslice_mask[s] = info->sseu.subslice_mask[s];
>   
>   		for (ss = 0; ss < ss_max; ss++) {
>   			unsigned int eu_cnt;
> @@ -4391,8 +4391,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->subslice_mask[s] =
> +				INTEL_INFO(dev_priv)->sseu.subslice_mask[s];
>   
>   		for (ss = 0; ss < ss_max; ss++) {
>   			unsigned int eu_cnt;
> @@ -4402,7 +4402,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
>   					/* skip disabled subslice */
>   					continue;
>   
> -				sseu->subslice_mask |= BIT(ss);
> +				sseu->subslice_mask[s] |= BIT(ss);
>   			}
>   
>   			eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
> @@ -4424,9 +4424,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->subslice_mask[s] =
> +				INTEL_INFO(dev_priv)->sseu.subslice_mask[s];
> +		}
>   		sseu->eu_total = sseu->eu_per_subslice *
>   				 sseu_subslice_total(sseu);
>   
> @@ -4445,6 +4448,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);
> @@ -4452,10 +4456,11 @@ 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 %u subslices, mask=%04x\n", type,
> +			   s, hweight8(sseu->subslice_mask[s]),
> +			   sseu->subslice_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 6c8da9d20c33..969835d3cbcd 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.subslice_mask[0];
>   		if (!value)
>   			return -ENODEV;
>   		break;
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index d28592e43512..ed14994527fc 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -80,12 +80,16 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
>   
>   static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p)
>   {
> +	int s;
> +
>   	drm_printf(p, "slice mask: %04x\n", sseu->slice_mask);
>   	drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask));
>   	drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu));
> -	drm_printf(p, "subslice mask %04x\n", sseu->subslice_mask);
> -	drm_printf(p, "subslice per slice: %u\n",
> -		   hweight8(sseu->subslice_mask));
> +	for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) {
> +		drm_printf(p, "slice%d %u subslices mask=%04x\n",
> +			   s, hweight8(sseu->subslice_mask[s]),
> +			   sseu->subslice_mask[s]);
> +	}
>   	drm_printf(p, "EU total: %u\n", sseu->eu_total);
>   	drm_printf(p, "EU per subslice: %u\n", sseu->eu_per_subslice);
>   	drm_printf(p, "has slice power gating: %s\n",
> @@ -119,22 +123,87 @@ void intel_device_info_dump(const struct intel_device_info *info,
>   	intel_device_info_dump_flags(info, p);
>   }
>   
> +static u16 compute_eu_total(const struct sseu_dev_info *sseu)
> +{
> +	u16 i, total = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(sseu->eu_mask); i++)
> +		total += hweight8(sseu->eu_mask[i]);
> +
> +	return total;
> +}
> +
> +static u16 compute_subslice_total(const struct sseu_dev_info *sseu)
> +{
> +	u16 i, total = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(sseu->subslice_mask); i++)
> +		total += hweight8(sseu->subslice_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;
> +	const int 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;
>   
> -	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));
> +	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->subslice_mask[0] = subslice_mask;
> +	for (s = 1; s < sseu->max_slices; s++)
> +		sseu->subslice_mask[s] = subslice_mask & 0x3;
> +
> +	/* Slice0 */
> +	eu_en = ~I915_READ(GEN8_EU_DISABLE0);
> +	for (ss = 0; ss < sseu->max_subslices; ss++)
> +		sseu_set_eus(sseu, 0, ss, (eu_en >> (8 * ss)) & eu_mask);
> +	/* Slice1 */
> +	sseu_set_eus(sseu, 1, 0, (eu_en >> 24) & eu_mask);
> +	eu_en = ~I915_READ(GEN8_EU_DISABLE1);
> +	sseu_set_eus(sseu, 1, 1, eu_en & eu_mask);
> +	/* Slice2 */
> +	sseu_set_eus(sseu, 2, 0, (eu_en >> 8) & eu_mask);
> +	sseu_set_eus(sseu, 2, 1, (eu_en >> 16) & eu_mask);
> +	/* Slice3 */
> +	sseu_set_eus(sseu, 3, 0, (eu_en >> 24) & eu_mask);
> +	eu_en = ~I915_READ(GEN8_EU_DISABLE2);
> +	sseu_set_eus(sseu, 3, 1, eu_en & eu_mask);
> +	/* Slice4 */
> +	sseu_set_eus(sseu, 4, 0, (eu_en >> 8) & eu_mask);
> +	sseu_set_eus(sseu, 4, 1, (eu_en >> 16) & eu_mask);
> +	/* Slice5 */
> +	sseu_set_eus(sseu, 5, 0, (eu_en >> 24) & eu_mask);
> +	eu_en = ~I915_READ(GEN10_EU_DISABLE3);
> +	sseu_set_eus(sseu, 5, 1, eu_en & eu_mask);
> +
> +	/* Do a second pass where we mark 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_get_eus(sseu, s, ss) == 0)
> +				sseu->subslice_mask[s] &= ~BIT(ss);
> +		}
> +	}
> +
> +	sseu->subslice_total = compute_subslice_total(sseu);
> +	sseu->eu_total = compute_eu_total(sseu);
>   
>   	/*
>   	 * CNL is expected to always have a uniform distribution
> @@ -155,26 +224,40 @@ 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);
> +		u8 disabled_mask =
> +			((fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >>
> +			 CHV_FGT_EU_DIS_SS0_R0_SHIFT) |
> +			(((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >>
> +			  CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4);
> +
> +		sseu->subslice_mask[0] |= BIT(0);
> +		sseu_set_eus(sseu, 0, 0, ~disabled_mask);
>   	}
>   
>   	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);
> +		u8 disabled_mask =
> +			((fuse & CHV_FGT_EU_DIS_SS1_R0_MASK) >>
> +			 CHV_FGT_EU_DIS_SS1_R0_SHIFT) |
> +			(((fuse & CHV_FGT_EU_DIS_SS1_R1_MASK) >>
> +			  CHV_FGT_EU_DIS_SS1_R1_SHIFT) << 4);
> +
> +		sseu->subslice_mask[0] |= BIT(1);
> +		sseu_set_eus(sseu, 0, 1, ~disabled_mask);
>   	}
>   
> +	sseu->subslice_total = compute_subslice_total(sseu);
> +	sseu->eu_total = compute_eu_total(sseu);
> +
>   	/*
>   	 * CHV expected to always have a uniform distribution of EU
>   	 * across subslices.
> @@ -196,41 +279,52 @@ 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;
> -	u8 eu_mask = 0xff;
> +	u32 fuse2, eu_disable, subslice_mask;
> +	const 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->subslice_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;
> +			u8 eu_disabled_mask;
>   
> -			if (!(sseu->subslice_mask & BIT(ss)))
> +			if (!(sseu->subslice_mask[s] & BIT(ss)))
>   				/* skip disabled subslice */
>   				continue;
>   
> -			eu_per_ss = eu_max - hweight8((eu_disable >> (ss*8)) &
> -						      eu_mask);
> +			eu_disabled_mask = (eu_disable >> (ss*8)) & eu_mask;
> +
> +			sseu_set_eus(sseu, s, ss, ~eu_disabled_mask);
> +
> +			eu_per_ss = sseu->max_eus_per_subslice -
> +				hweight8(eu_disabled_mask);
>   
>   			/*
>   			 * Record which subslice(s) has(have) 7 EUs. we
> @@ -239,11 +333,12 @@ 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->subslice_total = compute_subslice_total(sseu);
> +	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
> @@ -269,8 +364,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->subslice_mask[0] & BIT(ss)))
> +		info->has_pooled_eu = hweight8(sseu->subslice_mask[0]) == 3;
>   
>   		sseu->min_eu_in_pool = 0;
>   		if (info->has_pooled_eu) {
> @@ -288,19 +383,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) |
> @@ -314,30 +412,39 @@ 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->subslice_mask[s] = subslice_mask;
> +
> +		for (ss = 0; ss < sseu->max_subslices; ss++) {
> +			u8 eu_disabled_mask;
>   			u32 n_disabled;
>   
> -			if (!(sseu->subslice_mask & BIT(ss)))
> +			if (!(sseu->subslice_mask[ss] & BIT(ss)))
>   				/* skip disabled subslice */
>   				continue;
>   
> -			n_disabled = hweight8(eu_disable[s] >> (ss * eu_max));
> +			eu_disabled_mask =
> +				eu_disable[s] >> (ss * sseu->max_eus_per_subslice);
> +
> +			sseu_set_eus(sseu, s, ss, ~eu_disabled_mask);
> +
> +			n_disabled = hweight8(eu_disabled_mask);
>   
>   			/*
>   			 * 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->subslice_total = compute_subslice_total(sseu);
> +	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
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 49cb27bd04c1..36e0df87862d 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -110,10 +110,14 @@ enum intel_platform {
>   	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 eu_total;
> +	u8 subslice_mask[GEN_MAX_SUBSLICES];
> +	u16 subslice_total;
> +	u16 eu_total;
>   	u8 eu_per_subslice;
>   	u8 min_eu_in_pool;
>   	/* For each slice, which subslice(s) has(have) 7 EUs (bitfield)? */
> @@ -121,6 +125,17 @@ 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];
>   };
>   
>   struct intel_device_info {
> @@ -167,7 +182,33 @@ struct intel_device_info {
>   
>   static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>   {
> -	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
> +	return sseu->subslice_total;
> +}
> +
> +static inline int sseu_eu_idx(const struct sseu_dev_info *sseu,
> +			      int slice, int subslice)
> +{
> +	int subslice_stride = DIV_ROUND_UP(sseu->max_eus_per_subslice,
> +					   BITS_PER_BYTE);
> +	int slice_stride = sseu->max_subslices * subslice_stride;
> +
> +	return slice * slice_stride + subslice * subslice_stride;
> +}
> +
> +/*
> + * The following functions prototypes should be updated with a larger type
> + * than u8 if we ever have more than 8 EUs per subslice.
> + */
> +static inline u8 sseu_get_eus(const struct sseu_dev_info *sseu,
> +			      int slice, int subslice)
> +{
> +	return sseu->eu_mask[sseu_eu_idx(sseu, slice, subslice)];
> +}
> +
> +static inline void sseu_set_eus(struct sseu_dev_info *sseu,
> +				int slice, int subslice, u8 eu_mask)
> +{
> +	sseu->eu_mask[sseu_eu_idx(sseu, slice, subslice)] = eu_mask;
>   }
>   
>   const char *intel_platform_name(enum intel_platform platform);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ff25f209d0a5..ac7896031b8d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2098,7 +2098,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.subslice_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..23ae9a957fab 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.subslice_mask[0])
>   
>   #define for_each_instdone_slice_subslice(dev_priv__, slice__, subslice__) \
>   	for ((slice__) = 0, (subslice__) = 0; \
> 

I am happy with this version and I did not spot any mistakes.

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

Do you know if we have any test coverage which would be able to tell us 
if the reported numbers before and after match?

Regards,

Tvrtko

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

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

[Lionel Landwerlin]

On 15/01/18 17:37, Tvrtko Ursulin wrote:
>
> On 15/01/2018 14:41, Lionel Landwerlin wrote:
>> Up to now, subslice mask was assumed to be uniform across slices. But
>> starting with Cannonlake, slices can be asymmetric (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.
>>
>> v2: Rework how we store total numbers in sseu_dev_info (Tvrtko)
>>      Fix CHV eu masks, was reading disabled as enabled (Tvrtko)
>>      Readability changes (Tvrtko)
>>      Add EU index helper (Tvrtko)
>>
>> v3: Turn ALIGN(v, 8) / 8 into DIV_ROUND_UP(v, BITS_PER_BYTE) (Tvrtko)
>>      Reuse sseu_eu_idx() for setting eu_mask on CHV (Tvrtko)
>>      Reformat debug prints for subslices (Tvrtko)
>>
>> v4: Change eu_mask helper into sseu_set_eus() (Tvrtko)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c      |  25 ++--
>>   drivers/gpu/drm/i915/i915_drv.c          |   2 +-
>>   drivers/gpu/drm/i915/intel_device_info.c | 201 
>> +++++++++++++++++++++++--------
>>   drivers/gpu/drm/i915/intel_device_info.h |  47 +++++++-
>>   drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
>>   drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +-
>>   6 files changed, 216 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index cc659b4b2a45..684551114965 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -4289,7 +4289,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->subslice_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) +
>> @@ -4336,7 +4336,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->subslice_mask[s] = info->sseu.subslice_mask[s];
>>             for (ss = 0; ss < ss_max; ss++) {
>>               unsigned int eu_cnt;
>> @@ -4391,8 +4391,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->subslice_mask[s] =
>> +                INTEL_INFO(dev_priv)->sseu.subslice_mask[s];
>>             for (ss = 0; ss < ss_max; ss++) {
>>               unsigned int eu_cnt;
>> @@ -4402,7 +4402,7 @@ static void gen9_sseu_device_status(struct 
>> drm_i915_private *dev_priv,
>>                       /* skip disabled subslice */
>>                       continue;
>>   -                sseu->subslice_mask |= BIT(ss);
>> +                sseu->subslice_mask[s] |= BIT(ss);
>>               }
>>                 eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
>> @@ -4424,9 +4424,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->subslice_mask[s] =
>> +                INTEL_INFO(dev_priv)->sseu.subslice_mask[s];
>> +        }
>>           sseu->eu_total = sseu->eu_per_subslice *
>>                    sseu_subslice_total(sseu);
>>   @@ -4445,6 +4448,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);
>> @@ -4452,10 +4456,11 @@ 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 %u subslices, mask=%04x\n", type,
>> +               s, hweight8(sseu->subslice_mask[s]),
>> +               sseu->subslice_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 6c8da9d20c33..969835d3cbcd 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.subslice_mask[0];
>>           if (!value)
>>               return -ENODEV;
>>           break;
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index d28592e43512..ed14994527fc 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -80,12 +80,16 @@ void intel_device_info_dump_flags(const struct 
>> intel_device_info *info,
>>     static void sseu_dump(const struct sseu_dev_info *sseu, struct 
>> drm_printer *p)
>>   {
>> +    int s;
>> +
>>       drm_printf(p, "slice mask: %04x\n", sseu->slice_mask);
>>       drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask));
>>       drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu));
>> -    drm_printf(p, "subslice mask %04x\n", sseu->subslice_mask);
>> -    drm_printf(p, "subslice per slice: %u\n",
>> -           hweight8(sseu->subslice_mask));
>> +    for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) {
>> +        drm_printf(p, "slice%d %u subslices mask=%04x\n",
>> +               s, hweight8(sseu->subslice_mask[s]),
>> +               sseu->subslice_mask[s]);
>> +    }
>>       drm_printf(p, "EU total: %u\n", sseu->eu_total);
>>       drm_printf(p, "EU per subslice: %u\n", sseu->eu_per_subslice);
>>       drm_printf(p, "has slice power gating: %s\n",
>> @@ -119,22 +123,87 @@ void intel_device_info_dump(const struct 
>> intel_device_info *info,
>>       intel_device_info_dump_flags(info, p);
>>   }
>>   +static u16 compute_eu_total(const struct sseu_dev_info *sseu)
>> +{
>> +    u16 i, total = 0;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(sseu->eu_mask); i++)
>> +        total += hweight8(sseu->eu_mask[i]);
>> +
>> +    return total;
>> +}
>> +
>> +static u16 compute_subslice_total(const struct sseu_dev_info *sseu)
>> +{
>> +    u16 i, total = 0;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(sseu->subslice_mask); i++)
>> +        total += hweight8(sseu->subslice_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;
>> +    const int 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;
>>   -    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));
>> +    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->subslice_mask[0] = subslice_mask;
>> +    for (s = 1; s < sseu->max_slices; s++)
>> +        sseu->subslice_mask[s] = subslice_mask & 0x3;
>> +
>> +    /* Slice0 */
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE0);
>> +    for (ss = 0; ss < sseu->max_subslices; ss++)
>> +        sseu_set_eus(sseu, 0, ss, (eu_en >> (8 * ss)) & eu_mask);
>> +    /* Slice1 */
>> +    sseu_set_eus(sseu, 1, 0, (eu_en >> 24) & eu_mask);
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE1);
>> +    sseu_set_eus(sseu, 1, 1, eu_en & eu_mask);
>> +    /* Slice2 */
>> +    sseu_set_eus(sseu, 2, 0, (eu_en >> 8) & eu_mask);
>> +    sseu_set_eus(sseu, 2, 1, (eu_en >> 16) & eu_mask);
>> +    /* Slice3 */
>> +    sseu_set_eus(sseu, 3, 0, (eu_en >> 24) & eu_mask);
>> +    eu_en = ~I915_READ(GEN8_EU_DISABLE2);
>> +    sseu_set_eus(sseu, 3, 1, eu_en & eu_mask);
>> +    /* Slice4 */
>> +    sseu_set_eus(sseu, 4, 0, (eu_en >> 8) & eu_mask);
>> +    sseu_set_eus(sseu, 4, 1, (eu_en >> 16) & eu_mask);
>> +    /* Slice5 */
>> +    sseu_set_eus(sseu, 5, 0, (eu_en >> 24) & eu_mask);
>> +    eu_en = ~I915_READ(GEN10_EU_DISABLE3);
>> +    sseu_set_eus(sseu, 5, 1, eu_en & eu_mask);
>> +
>> +    /* Do a second pass where we mark 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_get_eus(sseu, s, ss) == 0)
>> +                sseu->subslice_mask[s] &= ~BIT(ss);
>> +        }
>> +    }
>> +
>> +    sseu->subslice_total = compute_subslice_total(sseu);
>> +    sseu->eu_total = compute_eu_total(sseu);
>>         /*
>>        * CNL is expected to always have a uniform distribution
>> @@ -155,26 +224,40 @@ 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);
>> +        u8 disabled_mask =
>> +            ((fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >>
>> +             CHV_FGT_EU_DIS_SS0_R0_SHIFT) |
>> +            (((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >>
>> +              CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4);
>> +
>> +        sseu->subslice_mask[0] |= BIT(0);
>> +        sseu_set_eus(sseu, 0, 0, ~disabled_mask);
>>       }
>>         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);
>> +        u8 disabled_mask =
>> +            ((fuse & CHV_FGT_EU_DIS_SS1_R0_MASK) >>
>> +             CHV_FGT_EU_DIS_SS1_R0_SHIFT) |
>> +            (((fuse & CHV_FGT_EU_DIS_SS1_R1_MASK) >>
>> +              CHV_FGT_EU_DIS_SS1_R1_SHIFT) << 4);
>> +
>> +        sseu->subslice_mask[0] |= BIT(1);
>> +        sseu_set_eus(sseu, 0, 1, ~disabled_mask);
>>       }
>>   +    sseu->subslice_total = compute_subslice_total(sseu);
>> +    sseu->eu_total = compute_eu_total(sseu);
>> +
>>       /*
>>        * CHV expected to always have a uniform distribution of EU
>>        * across subslices.
>> @@ -196,41 +279,52 @@ 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;
>> -    u8 eu_mask = 0xff;
>> +    u32 fuse2, eu_disable, subslice_mask;
>> +    const 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->subslice_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;
>> +            u8 eu_disabled_mask;
>>   -            if (!(sseu->subslice_mask & BIT(ss)))
>> +            if (!(sseu->subslice_mask[s] & BIT(ss)))
>>                   /* skip disabled subslice */
>>                   continue;
>>   -            eu_per_ss = eu_max - hweight8((eu_disable >> (ss*8)) &
>> -                              eu_mask);
>> +            eu_disabled_mask = (eu_disable >> (ss*8)) & eu_mask;
>> +
>> +            sseu_set_eus(sseu, s, ss, ~eu_disabled_mask);
>> +
>> +            eu_per_ss = sseu->max_eus_per_subslice -
>> +                hweight8(eu_disabled_mask);
>>                 /*
>>                * Record which subslice(s) has(have) 7 EUs. we
>> @@ -239,11 +333,12 @@ 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->subslice_total = compute_subslice_total(sseu);
>> +    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
>> @@ -269,8 +364,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->subslice_mask[0] & BIT(ss)))
>> +        info->has_pooled_eu = hweight8(sseu->subslice_mask[0]) == 3;
>>             sseu->min_eu_in_pool = 0;
>>           if (info->has_pooled_eu) {
>> @@ -288,19 +383,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) |
>> @@ -314,30 +412,39 @@ 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->subslice_mask[s] = subslice_mask;
>> +
>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>> +            u8 eu_disabled_mask;
>>               u32 n_disabled;
>>   -            if (!(sseu->subslice_mask & BIT(ss)))
>> +            if (!(sseu->subslice_mask[ss] & BIT(ss)))
>>                   /* skip disabled subslice */
>>                   continue;
>>   -            n_disabled = hweight8(eu_disable[s] >> (ss * eu_max));
>> +            eu_disabled_mask =
>> +                eu_disable[s] >> (ss * sseu->max_eus_per_subslice);
>> +
>> +            sseu_set_eus(sseu, s, ss, ~eu_disabled_mask);
>> +
>> +            n_disabled = hweight8(eu_disabled_mask);
>>                 /*
>>                * 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->subslice_total = compute_subslice_total(sseu);
>> +    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
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h 
>> b/drivers/gpu/drm/i915/intel_device_info.h
>> index 49cb27bd04c1..36e0df87862d 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -110,10 +110,14 @@ enum intel_platform {
>>       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 eu_total;
>> +    u8 subslice_mask[GEN_MAX_SUBSLICES];
>> +    u16 subslice_total;
>> +    u16 eu_total;
>>       u8 eu_per_subslice;
>>       u8 min_eu_in_pool;
>>       /* For each slice, which subslice(s) has(have) 7 EUs 
>> (bitfield)? */
>> @@ -121,6 +125,17 @@ 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];
>>   };
>>     struct intel_device_info {
>> @@ -167,7 +182,33 @@ struct intel_device_info {
>>     static inline unsigned int sseu_subslice_total(const struct 
>> sseu_dev_info *sseu)
>>   {
>> -    return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
>> +    return sseu->subslice_total;
>> +}
>> +
>> +static inline int sseu_eu_idx(const struct sseu_dev_info *sseu,
>> +                  int slice, int subslice)
>> +{
>> +    int subslice_stride = DIV_ROUND_UP(sseu->max_eus_per_subslice,
>> +                       BITS_PER_BYTE);
>> +    int slice_stride = sseu->max_subslices * subslice_stride;
>> +
>> +    return slice * slice_stride + subslice * subslice_stride;
>> +}
>> +
>> +/*
>> + * The following functions prototypes should be updated with a 
>> larger type
>> + * than u8 if we ever have more than 8 EUs per subslice.
>> + */
>> +static inline u8 sseu_get_eus(const struct sseu_dev_info *sseu,
>> +                  int slice, int subslice)
>> +{
>> +    return sseu->eu_mask[sseu_eu_idx(sseu, slice, subslice)];
>> +}
>> +
>> +static inline void sseu_set_eus(struct sseu_dev_info *sseu,
>> +                int slice, int subslice, u8 eu_mask)
>> +{
>> +    sseu->eu_mask[sseu_eu_idx(sseu, slice, subslice)] = eu_mask;
>>   }
>>     const char *intel_platform_name(enum intel_platform platform);
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index ff25f209d0a5..ac7896031b8d 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -2098,7 +2098,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.subslice_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..23ae9a957fab 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.subslice_mask[0])
>>     #define for_each_instdone_slice_subslice(dev_priv__, slice__, 
>> subslice__) \
>>       for ((slice__) = 0, (subslice__) = 0; \
>>
>
> I am happy with this version and I did not spot any mistakes.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Do you know if we have any test coverage which would be able to tell 
> us if the reported numbers before and after match?
>

I have some IGT tests (which I should resend) that verify that the old 
GET_PARAM values match the new uapi.
But because this refactors code, the old behavior is lost :(

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

[PATCH v4 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.

v2: Style tweaks (Tvrtko)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 684551114965..e41a19b7d7bb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4304,11 +4304,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];
+	u32 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
@@ -4330,7 +4330,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;
@@ -4338,7 +4338,7 @@ static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
 		sseu->slice_mask |= BIT(s);
 		sseu->subslice_mask[s] = info->sseu.subslice_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))))
@@ -4358,17 +4358,12 @@ 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];
+	u32 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));
@@ -4383,7 +4378,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;
@@ -4394,7 +4389,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 			sseu->subslice_mask[s] =
 				INTEL_INFO(dev_priv)->sseu.subslice_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

[PATCH v4 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: 3 subslice(s) (0x7):
	subslice0: 8 EUs (0xff)
	subslice1: 8 EUs (0xff)
	subslice2: 8 EUs (0xff)
	subslice3: 0 EUs (0x0)
slice1: 3 subslice(s) (0x7):
	subslice0: 8 EUs (0xff)
	subslice1: 8 EUs (0xff)
	subslice2: 8 EUs (0xff)
	subslice3: 0 EUs (0x0)
slice2: 3 subslice(s) (0x7):
	subslice0: 8 EUs (0xff)
	subslice1: 8 EUs (0xff)
	subslice2: 8 EUs (0xff)
	subslice3: 0 EUs (0x0)

v2: Reformat debugfs printing (Tvrtko)
    Use the new EU mask helper (Tvrtko)

v3: Move printing code to intel_device_info.c to be shared with error
    state (Michal)

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      | 11 +++++++++++
 drivers/gpu/drm/i915/intel_device_info.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_device_info.h |  2 ++
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e41a19b7d7bb..c3a44a54a0ba 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3166,6 +3166,16 @@ 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);
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	intel_device_info_dump_topology(&INTEL_INFO(dev_priv)->sseu, &p);
+
+	return 0;
+}
+
 static int i915_shrinker_info(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *i915 = node_to_i915(m->private);
@@ -4696,6 +4706,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},
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index ed14994527fc..dcb5127a174c 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -123,6 +123,30 @@ void intel_device_info_dump(const struct intel_device_info *info,
 	intel_device_info_dump_flags(info, p);
 }
 
+void intel_device_info_dump_topology(const struct sseu_dev_info *sseu,
+				     struct drm_printer *p)
+{
+	int s, ss;
+
+	if (sseu->max_slices == 0) {
+		drm_printf(p, "Unavailable\n");
+		return;
+	}
+
+	for (s = 0; s < sseu->max_slices; s++) {
+		drm_printf(p, "slice%i: %u subslice(s) (0x%hhx):\n",
+			   s, hweight8(sseu->subslice_mask[s]),
+			   sseu->subslice_mask[s]);
+
+		for (ss = 0; ss < sseu->max_subslices; ss++) {
+			drm_printf(p, "\tsubslice%i: %u EUs (0x%hhx)",
+				   ss, hweight8(sseu_get_eus(sseu, s, ss)),
+				   sseu_get_eus(sseu, s, ss));
+		}
+	}
+}
+
+
 static u16 compute_eu_total(const struct sseu_dev_info *sseu)
 {
 	u16 i, total = 0;
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 36e0df87862d..701888162944 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -220,5 +220,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
 				  struct drm_printer *p);
 void intel_device_info_dump_runtime(const struct intel_device_info *info,
 				    struct drm_printer *p);
+void intel_device_info_dump_topology(const struct sseu_dev_info *sseu,
+				     struct drm_printer *p);
 
 #endif
-- 
2.15.1

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

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

[Tvrtko Ursulin]

On 15/01/2018 14:41, 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: 3 subslice(s) (0x7):
> 	subslice0: 8 EUs (0xff)
> 	subslice1: 8 EUs (0xff)
> 	subslice2: 8 EUs (0xff)
> 	subslice3: 0 EUs (0x0)
> slice1: 3 subslice(s) (0x7):
> 	subslice0: 8 EUs (0xff)
> 	subslice1: 8 EUs (0xff)
> 	subslice2: 8 EUs (0xff)
> 	subslice3: 0 EUs (0x0)
> slice2: 3 subslice(s) (0x7):
> 	subslice0: 8 EUs (0xff)
> 	subslice1: 8 EUs (0xff)
> 	subslice2: 8 EUs (0xff)
> 	subslice3: 0 EUs (0x0)
> 
> v2: Reformat debugfs printing (Tvrtko)
>      Use the new EU mask helper (Tvrtko)
> 
> v3: Move printing code to intel_device_info.c to be shared with error
>      state (Michal)
> 
> 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      | 11 +++++++++++
>   drivers/gpu/drm/i915/intel_device_info.c | 24 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_device_info.h |  2 ++
>   3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e41a19b7d7bb..c3a44a54a0ba 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3166,6 +3166,16 @@ 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);
> +	struct drm_printer p = drm_seq_file_printer(m);
> +
> +	intel_device_info_dump_topology(&INTEL_INFO(dev_priv)->sseu, &p);
> +
> +	return 0;
> +}
> +
>   static int i915_shrinker_info(struct seq_file *m, void *unused)
>   {
>   	struct drm_i915_private *i915 = node_to_i915(m->private);
> @@ -4696,6 +4706,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},
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index ed14994527fc..dcb5127a174c 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -123,6 +123,30 @@ void intel_device_info_dump(const struct intel_device_info *info,
>   	intel_device_info_dump_flags(info, p);
>   }
>   
> +void intel_device_info_dump_topology(const struct sseu_dev_info *sseu,
> +				     struct drm_printer *p)
> +{
> +	int s, ss;
> +
> +	if (sseu->max_slices == 0) {
> +		drm_printf(p, "Unavailable\n");
> +		return;
> +	}
> +
> +	for (s = 0; s < sseu->max_slices; s++) {
> +		drm_printf(p, "slice%i: %u subslice(s) (0x%hhx):\n",
> +			   s, hweight8(sseu->subslice_mask[s]),
> +			   sseu->subslice_mask[s]);
> +
> +		for (ss = 0; ss < sseu->max_subslices; ss++) {
> +			drm_printf(p, "\tsubslice%i: %u EUs (0x%hhx)",
> +				   ss, hweight8(sseu_get_eus(sseu, s, ss)),
> +				   sseu_get_eus(sseu, s, ss));

If gcc cannot reduce to one call to sseu_get_eus consider caching in a 
local here.

> +		}
> +	}
> +}
> +
> +
>   static u16 compute_eu_total(const struct sseu_dev_info *sseu)
>   {
>   	u16 i, total = 0;
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 36e0df87862d..701888162944 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -220,5 +220,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
>   				  struct drm_printer *p);
>   void intel_device_info_dump_runtime(const struct intel_device_info *info,
>   				    struct drm_printer *p);
> +void intel_device_info_dump_topology(const struct sseu_dev_info *sseu,
> +				     struct drm_printer *p);
>   
>   #endif
> 

I wished you used %d instead of %i but OK.

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 v4 3/6] drm/i915/debugfs: add rcs topology entry

[Lionel Landwerlin]

On 15/01/18 17:42, Tvrtko Ursulin wrote:
>
> On 15/01/2018 14:41, 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: 3 subslice(s) (0x7):
>>     subslice0: 8 EUs (0xff)
>>     subslice1: 8 EUs (0xff)
>>     subslice2: 8 EUs (0xff)
>>     subslice3: 0 EUs (0x0)
>> slice1: 3 subslice(s) (0x7):
>>     subslice0: 8 EUs (0xff)
>>     subslice1: 8 EUs (0xff)
>>     subslice2: 8 EUs (0xff)
>>     subslice3: 0 EUs (0x0)
>> slice2: 3 subslice(s) (0x7):
>>     subslice0: 8 EUs (0xff)
>>     subslice1: 8 EUs (0xff)
>>     subslice2: 8 EUs (0xff)
>>     subslice3: 0 EUs (0x0)
>>
>> v2: Reformat debugfs printing (Tvrtko)
>>      Use the new EU mask helper (Tvrtko)
>>
>> v3: Move printing code to intel_device_info.c to be shared with error
>>      state (Michal)
>>
>> 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      | 11 +++++++++++
>>   drivers/gpu/drm/i915/intel_device_info.c | 24 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_device_info.h |  2 ++
>>   3 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index e41a19b7d7bb..c3a44a54a0ba 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3166,6 +3166,16 @@ 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);
>> +    struct drm_printer p = drm_seq_file_printer(m);
>> +
>> + intel_device_info_dump_topology(&INTEL_INFO(dev_priv)->sseu, &p);
>> +
>> +    return 0;
>> +}
>> +
>>   static int i915_shrinker_info(struct seq_file *m, void *unused)
>>   {
>>       struct drm_i915_private *i915 = node_to_i915(m->private);
>> @@ -4696,6 +4706,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},
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index ed14994527fc..dcb5127a174c 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -123,6 +123,30 @@ void intel_device_info_dump(const struct 
>> intel_device_info *info,
>>       intel_device_info_dump_flags(info, p);
>>   }
>>   +void intel_device_info_dump_topology(const struct sseu_dev_info 
>> *sseu,
>> +                     struct drm_printer *p)
>> +{
>> +    int s, ss;
>> +
>> +    if (sseu->max_slices == 0) {
>> +        drm_printf(p, "Unavailable\n");
>> +        return;
>> +    }
>> +
>> +    for (s = 0; s < sseu->max_slices; s++) {
>> +        drm_printf(p, "slice%i: %u subslice(s) (0x%hhx):\n",
>> +               s, hweight8(sseu->subslice_mask[s]),
>> +               sseu->subslice_mask[s]);
>> +
>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>> +            drm_printf(p, "\tsubslice%i: %u EUs (0x%hhx)",
>> +                   ss, hweight8(sseu_get_eus(sseu, s, ss)),
>> +                   sseu_get_eus(sseu, s, ss));
>
> If gcc cannot reduce to one call to sseu_get_eus consider caching in a 
> local here.
>
>> +        }
>> +    }
>> +}
>> +
>> +
>>   static u16 compute_eu_total(const struct sseu_dev_info *sseu)
>>   {
>>       u16 i, total = 0;
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h 
>> b/drivers/gpu/drm/i915/intel_device_info.h
>> index 36e0df87862d..701888162944 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -220,5 +220,7 @@ void intel_device_info_dump_flags(const struct 
>> intel_device_info *info,
>>                     struct drm_printer *p);
>>   void intel_device_info_dump_runtime(const struct intel_device_info 
>> *info,
>>                       struct drm_printer *p);
>> +void intel_device_info_dump_topology(const struct sseu_dev_info *sseu,
>> +                     struct drm_printer *p);
>>     #endif
>>
>
> I wished you used %d instead of %i but OK.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko
>
>
Sure, will apply you nits. Thanks.

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

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

[Lionel Landwerlin]

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

v2: Place topology towards the end of the error state (Chris)

v3: Reuse common printing code (Michal)

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

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 944059322daa..aaa34a2bebe1 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -605,6 +605,14 @@ 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)
+{
+	struct drm_printer p = i915_error_printer(m);
+
+	intel_device_info_dump_topology(sseu, &p);
+}
+
 int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 			    const struct i915_gpu_state *error)
 {
@@ -787,6 +795,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		intel_display_print_error_state(m, error->display);
 
 	err_print_capabilities(m, &error->device_info);
+	err_print_rcs_topology(m, &INTEL_INFO(dev_priv)->sseu);
 	err_print_params(m, &error->params);
 	err_print_uc(m, &error->uc);
 
-- 
2.15.1

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

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

[Tvrtko Ursulin]

On 15/01/2018 14:41, Lionel Landwerlin wrote:
> This might be useful information for developers looking at an error
> state.
> 
> v2: Place topology towards the end of the error state (Chris)
> 
> v3: Reuse common printing code (Michal)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 944059322daa..aaa34a2bebe1 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -605,6 +605,14 @@ 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)
> +{
> +	struct drm_printer p = i915_error_printer(m);
> +
> +	intel_device_info_dump_topology(sseu, &p);
> +}
> +
>   int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>   			    const struct i915_gpu_state *error)
>   {
> @@ -787,6 +795,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>   		intel_display_print_error_state(m, error->display);
>   
>   	err_print_capabilities(m, &error->device_info);
> +	err_print_rcs_topology(m, &INTEL_INFO(dev_priv)->sseu);
>   	err_print_params(m, &error->params);
>   	err_print_uc(m, &error->uc);
>   
> 

I trust you tested it works - code is trivial so:

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 v4 4/6] drm/i915: add rcs topology to error state

[Lionel Landwerlin]

On 15/01/18 17:43, Tvrtko Ursulin wrote:
>
> On 15/01/2018 14:41, Lionel Landwerlin wrote:
>> This might be useful information for developers looking at an error
>> state.
>>
>> v2: Place topology towards the end of the error state (Chris)
>>
>> v3: Reuse common printing code (Michal)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
>> b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 944059322daa..aaa34a2bebe1 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -605,6 +605,14 @@ 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)
>> +{
>> +    struct drm_printer p = i915_error_printer(m);
>> +
>> +    intel_device_info_dump_topology(sseu, &p);
>> +}
>> +
>>   int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>>                   const struct i915_gpu_state *error)
>>   {
>> @@ -787,6 +795,7 @@ int i915_error_state_to_str(struct 
>> drm_i915_error_state_buf *m,
>>           intel_display_print_error_state(m, error->display);
>>         err_print_capabilities(m, &error->device_info);
>> +    err_print_rcs_topology(m, &INTEL_INFO(dev_priv)->sseu);
>>       err_print_params(m, &error->params);
>>       err_print_uc(m, &error->uc);
>>
>
> I trust you tested it works - code is trivial so:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko
>
>
Tested, works fine.


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

[PATCH v4 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.

v2: Simplify dispatcher code iteration (Tvrtko)
    Tweak uapi drm_i915_query_item structure (Tvrtko)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@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 | 51 +++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h       | 31 ++++++++++++++++++++++++
 5 files changed, 87 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 3bddd8a06806..b0415a3e2d59 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 969835d3cbcd..d92e1b7236fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2824,6 +2824,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 c42015b05b47..b2615a88936e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3622,6 +3622,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..5694cfea4553
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -0,0 +1,51 @@
+/*
+ * 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;
+	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++, user_item_ptr++) {
+		struct drm_i915_query_item item;
+
+		if (copy_from_user(&item, user_item_ptr, sizeof(item)))
+			return -EFAULT;
+
+		switch (item.query_id) {
+		default:
+			return -EINVAL;
+		}
+
+		if (copy_to_user(user_item_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..39e93f10f2cd 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_IOWR(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 {
+	__u64 query_id;
+
+	/*
+	 * When set to zero by userspace, this is filled with the size of the
+	 * data to be written at the data_ptr pointer.
+	 */
+	__u32 length;
+	__u32 pad;
+
+	/*
+	 * 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 point to an array of num_items drm_i915_query_item structures.
+	 */
+	__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

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

[Lionel Landwerlin]

With the introduction of asymmetric 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

v2: Rename uapi struct s/_mask/_info/ (Tvrtko)
    Report max_slice/subslice/eus_per_subslice rather than strides (Tvrtko)
    Add uapi macros to read data from *_info structs (Tvrtko)

v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom shifts (Tvrtko)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_query.c | 134 ++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h       |  53 +++++++++++++++
 2 files changed, 187 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 5694cfea4553..465ec18a472f 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -25,8 +25,129 @@
 #include "i915_drv.h"
 #include <uapi/drm/i915_drm.h>
 
+static int query_slices_info(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_info slices_info;
+	u32 data_length, length;
+
+	if (sseu->max_slices == 0)
+		return -ENODEV;
+
+	data_length = sizeof(sseu->slice_mask);
+	length = sizeof(slices_info) + 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;
+
+	memset(&slices_info, 0, sizeof(slices_info));
+	slices_info.max_slices = sseu->max_slices;
+
+	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_info, data)),
+			 &sseu->slice_mask, data_length))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int query_subslices_info(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_info subslices_info;
+	u32 data_length, length;
+
+	if (sseu->max_slices == 0)
+		return -ENODEV;
+
+	memset(&subslices_info, 0, sizeof(subslices_info));
+	subslices_info.max_slices = sseu->max_slices;
+	subslices_info.max_subslices = sseu->max_subslices;
+
+	data_length = subslices_info.max_slices *
+		DIV_ROUND_UP(subslices_info.max_subslices,
+			     sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE);
+	length = sizeof(subslices_info) + 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_info, data)),
+			 sseu->subslice_mask, data_length))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int query_eus_info(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_info eus_info;
+	u32 data_length, length;
+
+	if (sseu->max_slices == 0)
+		return -ENODEV;
+
+	memset(&eus_info, 0, sizeof(eus_info));
+	eus_info.max_slices = sseu->max_slices;
+	eus_info.max_subslices = sseu->max_subslices;
+	eus_info.max_eus_per_subslice = sseu->max_eus_per_subslice;
+
+	data_length = eus_info.max_slices * eus_info.max_subslices *
+		DIV_ROUND_UP(eus_info.max_eus_per_subslice, BITS_PER_BYTE);
+	length = sizeof(eus_info) + 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_info, 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;
 	struct drm_i915_query_item __user *user_item_ptr =
 		u64_to_user_ptr(args->items_ptr);
@@ -34,15 +155,28 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 
 	for (i = 0; i < args->num_items; i++, user_item_ptr++) {
 		struct drm_i915_query_item item;
+		int ret;
 
 		if (copy_from_user(&item, user_item_ptr, sizeof(item)))
 			return -EFAULT;
 
 		switch (item.query_id) {
+		case DRM_I915_QUERY_ID_SLICES_INFO:
+			ret = query_slices_info(dev_priv, &item);
+			break;
+		case DRM_I915_QUERY_ID_SUBSLICES_INFO:
+			ret = query_subslices_info(dev_priv, &item);
+			break;
+		case DRM_I915_QUERY_ID_EUS_INFO:
+			ret = query_eus_info(dev_priv, &item);
+			break;
 		default:
 			return -EINVAL;
 		}
 
+		if (ret)
+			return ret;
+
 		if (copy_to_user(user_item_ptr, &item, sizeof(item)))
 			return -EFAULT;
 	}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 39e93f10f2cd..d25f21e3c107 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 {
 	__u64 query_id;
+#define DRM_I915_QUERY_ID_SLICES_INFO    0x01
+#define DRM_I915_QUERY_ID_SUBSLICES_INFO 0x02
+#define DRM_I915_QUERY_ID_EUS_INFO       0x03
 
 	/*
 	 * When set to zero by userspace, this is filled with the size of the
@@ -1644,6 +1647,56 @@ struct drm_i915_query {
 	__u64 items_ptr;
 };
 
+#define DRM_I915_BIT(bit) (1 << (bit))
+
+/* Data written by the kernel with query DRM_I915_QUERY_ID_SLICES_INFO :
+ *
+ * data: each bit indicates whether a slice is available (1) or fused off (0).
+ *       Use DRM_I915_QUERY_SLICE_AVAILABLE() to query a given slice's
+ *       availability.
+ */
+struct drm_i915_query_slices_info {
+	__u32 max_slices;
+
+#define DRM_I915_QUERY_SLICE_AVAILABLE(info, slice) \
+	!!((info)->data[(slice) / 8] & DRM_I915_BIT((slice) % 8))
+	__u8 data[];
+};
+
+/* Data written by the kernel with query DRM_I915_QUERY_ID_SUBSLICES_INFO :
+ *
+ * data: each bit indicates whether a subslice is available (1) or fused off
+ *       (0). Use DRM_I915_QUERY_SUBSLICE_AVAILABLE() to query a given
+ *       subslice's availability.
+ */
+struct drm_i915_query_subslices_info {
+	__u32 max_slices;
+	__u32 max_subslices;
+
+#define DRM_I915_QUERY_SUBSLICE_AVAILABLE(info, slice, subslice) \
+	!!((info)->data[(slice) * ALIGN((info)->max_subslices, 8) / 8 + \
+			(subslice) / 8] & DRM_I915_BIT((subslice) % 8))
+	__u8 data[];
+};
+
+/* Data written by the kernel with query DRM_I915_QUERY_ID_EUS_INFO :
+ *
+ * data: Each bit indicates whether a subslice is available (1) or fused off
+ *       (0). Use DRM_I915_QUERY_EU_AVAILABLE() to query a given EU's
+ *       availability.
+ */
+struct drm_i915_query_eus_info {
+	__u32 max_slices;
+	__u32 max_subslices;
+	__u32 max_eus_per_subslice;
+
+#define DRM_I915_QUERY_EU_AVAILABLE(info, slice, subslice, eu) \
+	!!((info)->data[(slice) * ALIGN((info)->max_eus_per_subslice, 8) / 8 * (info)->max_subslices + \
+			(subslice) * ALIGN((info)->max_eus_per_subslice, 8) / 8 + \
+			(eu) / 8] & DRM_I915_BIT((eu) % 8))
+	__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 v4 6/6] drm/i915: expose rcs topology through query uAPI

[Tvrtko Ursulin]

On 15/01/2018 14:41, Lionel Landwerlin wrote:
> With the introduction of asymmetric 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
> 
> v2: Rename uapi struct s/_mask/_info/ (Tvrtko)
>      Report max_slice/subslice/eus_per_subslice rather than strides (Tvrtko)
>      Add uapi macros to read data from *_info structs (Tvrtko)
> 
> v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom shifts (Tvrtko)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_query.c | 134 ++++++++++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h       |  53 +++++++++++++++
>   2 files changed, 187 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5694cfea4553..465ec18a472f 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -25,8 +25,129 @@
>   #include "i915_drv.h"
>   #include <uapi/drm/i915_drm.h>
>   
> +static int query_slices_info(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_info slices_info;
> +	u32 data_length, length;
> +
> +	if (sseu->max_slices == 0)
> +		return -ENODEV;
> +
> +	data_length = sizeof(sseu->slice_mask);
> +	length = sizeof(slices_info) + 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;
> +
> +	memset(&slices_info, 0, sizeof(slices_info));
> +	slices_info.max_slices = sseu->max_slices;
> +
> +	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_info, data)),
> +			 &sseu->slice_mask, data_length))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int query_subslices_info(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_info subslices_info;
> +	u32 data_length, length;
> +
> +	if (sseu->max_slices == 0)
> +		return -ENODEV;
> +
> +	memset(&subslices_info, 0, sizeof(subslices_info));
> +	subslices_info.max_slices = sseu->max_slices;
> +	subslices_info.max_subslices = sseu->max_subslices;
> +
> +	data_length = subslices_info.max_slices *
> +		DIV_ROUND_UP(subslices_info.max_subslices,
> +			     sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE);
> +	length = sizeof(subslices_info) + 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_info, data)),
> +			 sseu->subslice_mask, data_length))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int query_eus_info(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_info eus_info;
> +	u32 data_length, length;
> +
> +	if (sseu->max_slices == 0)
> +		return -ENODEV;
> +
> +	memset(&eus_info, 0, sizeof(eus_info));
> +	eus_info.max_slices = sseu->max_slices;
> +	eus_info.max_subslices = sseu->max_subslices;
> +	eus_info.max_eus_per_subslice = sseu->max_eus_per_subslice;
> +
> +	data_length = eus_info.max_slices * eus_info.max_subslices *
> +		DIV_ROUND_UP(eus_info.max_eus_per_subslice, BITS_PER_BYTE);
> +	length = sizeof(eus_info) + 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_info, data)),
> +			 sseu->eu_mask, data_length))
> +		return -EFAULT;

I think in all three queries, once you have the length, you could 
extract the four if statement above into a common helper? Arguments are 
query_item, length, data_length, ptr_query_item, ptr_data, and 
offset_data, unless I am missing something. Up to you if you would find 
that a worthy compaction.

> +
> +	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;
>   	struct drm_i915_query_item __user *user_item_ptr =
>   		u64_to_user_ptr(args->items_ptr);
> @@ -34,15 +155,28 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   
>   	for (i = 0; i < args->num_items; i++, user_item_ptr++) {
>   		struct drm_i915_query_item item;
> +		int ret;
>   
>   		if (copy_from_user(&item, user_item_ptr, sizeof(item)))
>   			return -EFAULT;
>   
>   		switch (item.query_id) {
> +		case DRM_I915_QUERY_ID_SLICES_INFO:
> +			ret = query_slices_info(dev_priv, &item);
> +			break;
> +		case DRM_I915_QUERY_ID_SUBSLICES_INFO:
> +			ret = query_subslices_info(dev_priv, &item);
> +			break;
> +		case DRM_I915_QUERY_ID_EUS_INFO:
> +			ret = query_eus_info(dev_priv, &item);
> +			break;
>   		default:
>   			return -EINVAL;
>   		}
>   
> +		if (ret)
> +			return ret;
> +
>   		if (copy_to_user(user_item_ptr, &item, sizeof(item)))
>   			return -EFAULT;
>   	}
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 39e93f10f2cd..d25f21e3c107 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 {
>   	__u64 query_id;
> +#define DRM_I915_QUERY_ID_SLICES_INFO    0x01
> +#define DRM_I915_QUERY_ID_SUBSLICES_INFO 0x02
> +#define DRM_I915_QUERY_ID_EUS_INFO       0x03

Would DRM_I915_QUERY_ID_"singular"_INFO read better? In struct names as 
well if so?

>   
>   	/*
>   	 * When set to zero by userspace, this is filled with the size of the
> @@ -1644,6 +1647,56 @@ struct drm_i915_query {
>   	__u64 items_ptr;
>   };

Okay.. up to here it's all pretty much fine.

>   
> +#define DRM_I915_BIT(bit) (1 << (bit))
> +
> +/* Data written by the kernel with query DRM_I915_QUERY_ID_SLICES_INFO :
> + *
> + * data: each bit indicates whether a slice is available (1) or fused off (0).
> + *       Use DRM_I915_QUERY_SLICE_AVAILABLE() to query a given slice's
> + *       availability.
> + */
> +struct drm_i915_query_slices_info {
> +	__u32 max_slices;
> +
> +#define DRM_I915_QUERY_SLICE_AVAILABLE(info, slice) \
> +	!!((info)->data[(slice) / 8] & DRM_I915_BIT((slice) % 8))
> +	__u8 data[];
> +};
> +
> +/* Data written by the kernel with query DRM_I915_QUERY_ID_SUBSLICES_INFO :
> + *
> + * data: each bit indicates whether a subslice is available (1) or fused off
> + *       (0). Use DRM_I915_QUERY_SUBSLICE_AVAILABLE() to query a given
> + *       subslice's availability.
> + */
> +struct drm_i915_query_subslices_info {
> +	__u32 max_slices;
> +	__u32 max_subslices;
> +
> +#define DRM_I915_QUERY_SUBSLICE_AVAILABLE(info, slice, subslice) \
> +	!!((info)->data[(slice) * ALIGN((info)->max_subslices, 8) / 8 + \
> +			(subslice) / 8] & DRM_I915_BIT((subslice) % 8))
> +	__u8 data[];
> +};
> +
> +/* Data written by the kernel with query DRM_I915_QUERY_ID_EUS_INFO :
> + *
> + * data: Each bit indicates whether a subslice is available (1) or fused off
> + *       (0). Use DRM_I915_QUERY_EU_AVAILABLE() to query a given EU's
> + *       availability.
> + */
> +struct drm_i915_query_eus_info {
> +	__u32 max_slices;
> +	__u32 max_subslices;
> +	__u32 max_eus_per_subslice;
> +
> +#define DRM_I915_QUERY_EU_AVAILABLE(info, slice, subslice, eu) \
> +	!!((info)->data[(slice) * ALIGN((info)->max_eus_per_subslice, 8) / 8 * (info)->max_subslices + \
> +			(subslice) * ALIGN((info)->max_eus_per_subslice, 8) / 8 + \
> +			(eu) / 8] & DRM_I915_BIT((eu) % 8))
> +	__u8 data[];
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> 

This last bit however I'm a bit unsure of.

I know I suggested the accessor macros but now they look a bit 
unsightly. I don't have ideas on how to improve them though.

And the unspecified length array is also potentially questionable.

I'll take the liberty of Cc-ing Chris and Joonas to start with to see if 
they have comments on the above.

Regards,

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

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

[Lionel Landwerlin]

On 15/01/18 17:54, Tvrtko Ursulin wrote:
>
> On 15/01/2018 14:41, Lionel Landwerlin wrote:
>> With the introduction of asymmetric 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
>>
>> v2: Rename uapi struct s/_mask/_info/ (Tvrtko)
>>      Report max_slice/subslice/eus_per_subslice rather than strides 
>> (Tvrtko)
>>      Add uapi macros to read data from *_info structs (Tvrtko)
>>
>> v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom 
>> shifts (Tvrtko)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_query.c | 134 
>> ++++++++++++++++++++++++++++++++++++++
>>   include/uapi/drm/i915_drm.h       |  53 +++++++++++++++
>>   2 files changed, 187 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index 5694cfea4553..465ec18a472f 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -25,8 +25,129 @@
>>   #include "i915_drv.h"
>>   #include <uapi/drm/i915_drm.h>
>>   +static int query_slices_info(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_info slices_info;
>> +    u32 data_length, length;
>> +
>> +    if (sseu->max_slices == 0)
>> +        return -ENODEV;
>> +
>> +    data_length = sizeof(sseu->slice_mask);
>> +    length = sizeof(slices_info) + 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;
>> +
>> +    memset(&slices_info, 0, sizeof(slices_info));
>> +    slices_info.max_slices = sseu->max_slices;
>> +
>> +    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_info, 
>> data)),
>> +             &sseu->slice_mask, data_length))
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>> +
>> +static int query_subslices_info(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_info subslices_info;
>> +    u32 data_length, length;
>> +
>> +    if (sseu->max_slices == 0)
>> +        return -ENODEV;
>> +
>> +    memset(&subslices_info, 0, sizeof(subslices_info));
>> +    subslices_info.max_slices = sseu->max_slices;
>> +    subslices_info.max_subslices = sseu->max_subslices;
>> +
>> +    data_length = subslices_info.max_slices *
>> +        DIV_ROUND_UP(subslices_info.max_subslices,
>> +                 sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE);
>> +    length = sizeof(subslices_info) + 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_info, 
>> data)),
>> +             sseu->subslice_mask, data_length))
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>> +
>> +static int query_eus_info(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_info eus_info;
>> +    u32 data_length, length;
>> +
>> +    if (sseu->max_slices == 0)
>> +        return -ENODEV;
>> +
>> +    memset(&eus_info, 0, sizeof(eus_info));
>> +    eus_info.max_slices = sseu->max_slices;
>> +    eus_info.max_subslices = sseu->max_subslices;
>> +    eus_info.max_eus_per_subslice = sseu->max_eus_per_subslice;
>> +
>> +    data_length = eus_info.max_slices * eus_info.max_subslices *
>> +        DIV_ROUND_UP(eus_info.max_eus_per_subslice, BITS_PER_BYTE);
>> +    length = sizeof(eus_info) + 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_info, data)),
>> +             sseu->eu_mask, data_length))
>> +        return -EFAULT;
>
> I think in all three queries, once you have the length, you could 
> extract the four if statement above into a common helper? Arguments 
> are query_item, length, data_length, ptr_query_item, ptr_data, and 
> offset_data, unless I am missing something. Up to you if you would 
> find that a worthy compaction.

I just tried that but the result doesn't really look good.
You need to maintain the control flow via an indirect return value...

>
>> +
>> +    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;
>>       struct drm_i915_query_item __user *user_item_ptr =
>>           u64_to_user_ptr(args->items_ptr);
>> @@ -34,15 +155,28 @@ int i915_query_ioctl(struct drm_device *dev, 
>> void *data, struct drm_file *file)
>>         for (i = 0; i < args->num_items; i++, user_item_ptr++) {
>>           struct drm_i915_query_item item;
>> +        int ret;
>>             if (copy_from_user(&item, user_item_ptr, sizeof(item)))
>>               return -EFAULT;
>>             switch (item.query_id) {
>> +        case DRM_I915_QUERY_ID_SLICES_INFO:
>> +            ret = query_slices_info(dev_priv, &item);
>> +            break;
>> +        case DRM_I915_QUERY_ID_SUBSLICES_INFO:
>> +            ret = query_subslices_info(dev_priv, &item);
>> +            break;
>> +        case DRM_I915_QUERY_ID_EUS_INFO:
>> +            ret = query_eus_info(dev_priv, &item);
>> +            break;
>>           default:
>>               return -EINVAL;
>>           }
>>   +        if (ret)
>> +            return ret;
>> +
>>           if (copy_to_user(user_item_ptr, &item, sizeof(item)))
>>               return -EFAULT;
>>       }
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 39e93f10f2cd..d25f21e3c107 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 {
>>       __u64 query_id;
>> +#define DRM_I915_QUERY_ID_SLICES_INFO    0x01
>> +#define DRM_I915_QUERY_ID_SUBSLICES_INFO 0x02
>> +#define DRM_I915_QUERY_ID_EUS_INFO       0x03
>
> Would DRM_I915_QUERY_ID_"singular"_INFO read better? In struct names 
> as well if so?

Hmm... Sorry, I don't understand your comment here.
What would be "singular" ?

>
>>         /*
>>        * When set to zero by userspace, this is filled with the size 
>> of the
>> @@ -1644,6 +1647,56 @@ struct drm_i915_query {
>>       __u64 items_ptr;
>>   };
>
> Okay.. up to here it's all pretty much fine.
>
>>   +#define DRM_I915_BIT(bit) (1 << (bit))
>> +
>> +/* Data written by the kernel with query 
>> DRM_I915_QUERY_ID_SLICES_INFO :
>> + *
>> + * data: each bit indicates whether a slice is available (1) or 
>> fused off (0).
>> + *       Use DRM_I915_QUERY_SLICE_AVAILABLE() to query a given slice's
>> + *       availability.
>> + */
>> +struct drm_i915_query_slices_info {
>> +    __u32 max_slices;
>> +
>> +#define DRM_I915_QUERY_SLICE_AVAILABLE(info, slice) \
>> +    !!((info)->data[(slice) / 8] & DRM_I915_BIT((slice) % 8))
>> +    __u8 data[];
>> +};
>> +
>> +/* Data written by the kernel with query 
>> DRM_I915_QUERY_ID_SUBSLICES_INFO :
>> + *
>> + * data: each bit indicates whether a subslice is available (1) or 
>> fused off
>> + *       (0). Use DRM_I915_QUERY_SUBSLICE_AVAILABLE() to query a given
>> + *       subslice's availability.
>> + */
>> +struct drm_i915_query_subslices_info {
>> +    __u32 max_slices;
>> +    __u32 max_subslices;
>> +
>> +#define DRM_I915_QUERY_SUBSLICE_AVAILABLE(info, slice, subslice) \
>> +    !!((info)->data[(slice) * ALIGN((info)->max_subslices, 8) / 8 + \
>> +            (subslice) / 8] & DRM_I915_BIT((subslice) % 8))
>> +    __u8 data[];
>> +};
>> +
>> +/* Data written by the kernel with query DRM_I915_QUERY_ID_EUS_INFO :
>> + *
>> + * data: Each bit indicates whether a subslice is available (1) or 
>> fused off
>> + *       (0). Use DRM_I915_QUERY_EU_AVAILABLE() to query a given EU's
>> + *       availability.
>> + */
>> +struct drm_i915_query_eus_info {
>> +    __u32 max_slices;
>> +    __u32 max_subslices;
>> +    __u32 max_eus_per_subslice;
>> +
>> +#define DRM_I915_QUERY_EU_AVAILABLE(info, slice, subslice, eu) \
>> +    !!((info)->data[(slice) * ALIGN((info)->max_eus_per_subslice, 8) 
>> / 8 * (info)->max_subslices + \
>> +            (subslice) * ALIGN((info)->max_eus_per_subslice, 8) / 8 + \
>> +            (eu) / 8] & DRM_I915_BIT((eu) % 8))
>> +    __u8 data[];
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>>
>
> This last bit however I'm a bit unsure of.
>
> I know I suggested the accessor macros but now they look a bit 
> unsightly. I don't have ideas on how to improve them though.
>
> And the unspecified length array is also potentially questionable.
>
> I'll take the liberty of Cc-ing Chris and Joonas to start with to see 
> if they have comments on the above.
>
> Regards,
>
> Tvrtko
>
Thanks again for your review.


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

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

[Tvrtko Ursulin]

On 15/01/2018 18:23, Lionel Landwerlin wrote:
> On 15/01/18 17:54, Tvrtko Ursulin wrote:
>>
>> On 15/01/2018 14:41, Lionel Landwerlin wrote:
>>> With the introduction of asymmetric 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
>>>
>>> v2: Rename uapi struct s/_mask/_info/ (Tvrtko)
>>>      Report max_slice/subslice/eus_per_subslice rather than strides 
>>> (Tvrtko)
>>>      Add uapi macros to read data from *_info structs (Tvrtko)
>>>
>>> v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom 
>>> shifts (Tvrtko)
>>>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_query.c | 134 
>>> ++++++++++++++++++++++++++++++++++++++
>>>   include/uapi/drm/i915_drm.h       |  53 +++++++++++++++
>>>   2 files changed, 187 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>>> b/drivers/gpu/drm/i915/i915_query.c
>>> index 5694cfea4553..465ec18a472f 100644
>>> --- a/drivers/gpu/drm/i915/i915_query.c
>>> +++ b/drivers/gpu/drm/i915/i915_query.c
>>> @@ -25,8 +25,129 @@
>>>   #include "i915_drv.h"
>>>   #include <uapi/drm/i915_drm.h>
>>>   +static int query_slices_info(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_info slices_info;
>>> +    u32 data_length, length;
>>> +
>>> +    if (sseu->max_slices == 0)
>>> +        return -ENODEV;
>>> +
>>> +    data_length = sizeof(sseu->slice_mask);
>>> +    length = sizeof(slices_info) + 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;
>>> +
>>> +    memset(&slices_info, 0, sizeof(slices_info));
>>> +    slices_info.max_slices = sseu->max_slices;
>>> +
>>> +    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_info, 
>>> data)),
>>> +             &sseu->slice_mask, data_length))
>>> +        return -EFAULT;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int query_subslices_info(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_info subslices_info;
>>> +    u32 data_length, length;
>>> +
>>> +    if (sseu->max_slices == 0)
>>> +        return -ENODEV;
>>> +
>>> +    memset(&subslices_info, 0, sizeof(subslices_info));
>>> +    subslices_info.max_slices = sseu->max_slices;
>>> +    subslices_info.max_subslices = sseu->max_subslices;
>>> +
>>> +    data_length = subslices_info.max_slices *
>>> +        DIV_ROUND_UP(subslices_info.max_subslices,
>>> +                 sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE);
>>> +    length = sizeof(subslices_info) + 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_info, 
>>> data)),
>>> +             sseu->subslice_mask, data_length))
>>> +        return -EFAULT;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int query_eus_info(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_info eus_info;
>>> +    u32 data_length, length;
>>> +
>>> +    if (sseu->max_slices == 0)
>>> +        return -ENODEV;
>>> +
>>> +    memset(&eus_info, 0, sizeof(eus_info));
>>> +    eus_info.max_slices = sseu->max_slices;
>>> +    eus_info.max_subslices = sseu->max_subslices;
>>> +    eus_info.max_eus_per_subslice = sseu->max_eus_per_subslice;
>>> +
>>> +    data_length = eus_info.max_slices * eus_info.max_subslices *
>>> +        DIV_ROUND_UP(eus_info.max_eus_per_subslice, BITS_PER_BYTE);
>>> +    length = sizeof(eus_info) + 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_info, data)),
>>> +             sseu->eu_mask, data_length))
>>> +        return -EFAULT;
>>
>> I think in all three queries, once you have the length, you could 
>> extract the four if statement above into a common helper? Arguments 
>> are query_item, length, data_length, ptr_query_item, ptr_data, and 
>> offset_data, unless I am missing something. Up to you if you would 
>> find that a worthy compaction.
> 
> I just tried that but the result doesn't really look good.
> You need to maintain the control flow via an indirect return value...

I was thinking something like this:

int insert_query_helper_name(query_item, query_item_sz, length, 
data_length, query_item_ptr, query_data_ptr)
{
     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),
			query_item_ptr, query_item_sz))
         return -EFAULT;

     if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
                      query_item_sz,
              query_data_ptr, data_length))
         return -EFAULT;

     return 0;
}

Then in the handlers just end with:

return insert_query_helper_hame(...);

?

>>
>>> +
>>> +    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;
>>>       struct drm_i915_query_item __user *user_item_ptr =
>>>           u64_to_user_ptr(args->items_ptr);
>>> @@ -34,15 +155,28 @@ int i915_query_ioctl(struct drm_device *dev, 
>>> void *data, struct drm_file *file)
>>>         for (i = 0; i < args->num_items; i++, user_item_ptr++) {
>>>           struct drm_i915_query_item item;
>>> +        int ret;
>>>             if (copy_from_user(&item, user_item_ptr, sizeof(item)))
>>>               return -EFAULT;
>>>             switch (item.query_id) {
>>> +        case DRM_I915_QUERY_ID_SLICES_INFO:
>>> +            ret = query_slices_info(dev_priv, &item);
>>> +            break;
>>> +        case DRM_I915_QUERY_ID_SUBSLICES_INFO:
>>> +            ret = query_subslices_info(dev_priv, &item);
>>> +            break;
>>> +        case DRM_I915_QUERY_ID_EUS_INFO:
>>> +            ret = query_eus_info(dev_priv, &item);
>>> +            break;
>>>           default:
>>>               return -EINVAL;
>>>           }
>>>   +        if (ret)
>>> +            return ret;
>>> +
>>>           if (copy_to_user(user_item_ptr, &item, sizeof(item)))
>>>               return -EFAULT;
>>>       }
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index 39e93f10f2cd..d25f21e3c107 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 {
>>>       __u64 query_id;
>>> +#define DRM_I915_QUERY_ID_SLICES_INFO    0x01
>>> +#define DRM_I915_QUERY_ID_SUBSLICES_INFO 0x02
>>> +#define DRM_I915_QUERY_ID_EUS_INFO       0x03
>>
>> Would DRM_I915_QUERY_ID_"singular"_INFO read better? In struct names 
>> as well if so?
> 
> Hmm... Sorry, I don't understand your comment here.
> What would be "singular" ?

Opposite of plural, like:

_QUERY_ID_SLICE_INFO, _QUERY_ID_SUBSLICE_INFO, _QUERY_ID_EU_INFO.

Actually now I'm thinking if _ID_ part could also be removed for no loss?

Regards,

Tvrtko

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

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

[Lionel Landwerlin]

On 16/01/18 08:42, Tvrtko Ursulin wrote:
>
> On 15/01/2018 18:23, Lionel Landwerlin wrote:
>> On 15/01/18 17:54, Tvrtko Ursulin wrote:
>>>
>>> On 15/01/2018 14:41, Lionel Landwerlin wrote:
>>>> With the introduction of asymmetric 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
>>>>
>>>> v2: Rename uapi struct s/_mask/_info/ (Tvrtko)
>>>>      Report max_slice/subslice/eus_per_subslice rather than strides 
>>>> (Tvrtko)
>>>>      Add uapi macros to read data from *_info structs (Tvrtko)
>>>>
>>>> v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom 
>>>> shifts (Tvrtko)
>>>>
>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_query.c | 134 
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>   include/uapi/drm/i915_drm.h       |  53 +++++++++++++++
>>>>   2 files changed, 187 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>>>> b/drivers/gpu/drm/i915/i915_query.c
>>>> index 5694cfea4553..465ec18a472f 100644
>>>> --- a/drivers/gpu/drm/i915/i915_query.c
>>>> +++ b/drivers/gpu/drm/i915/i915_query.c
>>>> @@ -25,8 +25,129 @@
>>>>   #include "i915_drv.h"
>>>>   #include <uapi/drm/i915_drm.h>
>>>>   +static int query_slices_info(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_info slices_info;
>>>> +    u32 data_length, length;
>>>> +
>>>> +    if (sseu->max_slices == 0)
>>>> +        return -ENODEV;
>>>> +
>>>> +    data_length = sizeof(sseu->slice_mask);
>>>> +    length = sizeof(slices_info) + 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;
>>>> +
>>>> +    memset(&slices_info, 0, sizeof(slices_info));
>>>> +    slices_info.max_slices = sseu->max_slices;
>>>> +
>>>> +    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_info, 
>>>> data)),
>>>> +             &sseu->slice_mask, data_length))
>>>> +        return -EFAULT;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int query_subslices_info(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_info subslices_info;
>>>> +    u32 data_length, length;
>>>> +
>>>> +    if (sseu->max_slices == 0)
>>>> +        return -ENODEV;
>>>> +
>>>> +    memset(&subslices_info, 0, sizeof(subslices_info));
>>>> +    subslices_info.max_slices = sseu->max_slices;
>>>> +    subslices_info.max_subslices = sseu->max_subslices;
>>>> +
>>>> +    data_length = subslices_info.max_slices *
>>>> +        DIV_ROUND_UP(subslices_info.max_subslices,
>>>> +                 sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE);
>>>> +    length = sizeof(subslices_info) + 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_info, data)),
>>>> +             sseu->subslice_mask, data_length))
>>>> +        return -EFAULT;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int query_eus_info(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_info eus_info;
>>>> +    u32 data_length, length;
>>>> +
>>>> +    if (sseu->max_slices == 0)
>>>> +        return -ENODEV;
>>>> +
>>>> +    memset(&eus_info, 0, sizeof(eus_info));
>>>> +    eus_info.max_slices = sseu->max_slices;
>>>> +    eus_info.max_subslices = sseu->max_subslices;
>>>> +    eus_info.max_eus_per_subslice = sseu->max_eus_per_subslice;
>>>> +
>>>> +    data_length = eus_info.max_slices * eus_info.max_subslices *
>>>> +        DIV_ROUND_UP(eus_info.max_eus_per_subslice, BITS_PER_BYTE);
>>>> +    length = sizeof(eus_info) + 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_info, data)),
>>>> +             sseu->eu_mask, data_length))
>>>> +        return -EFAULT;
>>>
>>> I think in all three queries, once you have the length, you could 
>>> extract the four if statement above into a common helper? Arguments 
>>> are query_item, length, data_length, ptr_query_item, ptr_data, and 
>>> offset_data, unless I am missing something. Up to you if you would 
>>> find that a worthy compaction.
>>
>> I just tried that but the result doesn't really look good.
>> You need to maintain the control flow via an indirect return value...
>
> I was thinking something like this:
>
> int insert_query_helper_name(query_item, query_item_sz, length, 
> data_length, query_item_ptr, query_data_ptr)
> {
>     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),
>             query_item_ptr, query_item_sz))
>         return -EFAULT;
>
>     if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>                      query_item_sz,
>              query_data_ptr, data_length))
>         return -EFAULT;
>
>     return 0;
> }
>
> Then in the handlers just end with:
>
> return insert_query_helper_hame(...);

Ah thanks, I stopped at copying the query item but didn't do the data...
That looks better indeed.

>
> ?
>
>>>
>>>> +
>>>> +    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;
>>>>       struct drm_i915_query_item __user *user_item_ptr =
>>>>           u64_to_user_ptr(args->items_ptr);
>>>> @@ -34,15 +155,28 @@ int i915_query_ioctl(struct drm_device *dev, 
>>>> void *data, struct drm_file *file)
>>>>         for (i = 0; i < args->num_items; i++, user_item_ptr++) {
>>>>           struct drm_i915_query_item item;
>>>> +        int ret;
>>>>             if (copy_from_user(&item, user_item_ptr, sizeof(item)))
>>>>               return -EFAULT;
>>>>             switch (item.query_id) {
>>>> +        case DRM_I915_QUERY_ID_SLICES_INFO:
>>>> +            ret = query_slices_info(dev_priv, &item);
>>>> +            break;
>>>> +        case DRM_I915_QUERY_ID_SUBSLICES_INFO:
>>>> +            ret = query_subslices_info(dev_priv, &item);
>>>> +            break;
>>>> +        case DRM_I915_QUERY_ID_EUS_INFO:
>>>> +            ret = query_eus_info(dev_priv, &item);
>>>> +            break;
>>>>           default:
>>>>               return -EINVAL;
>>>>           }
>>>>   +        if (ret)
>>>> +            return ret;
>>>> +
>>>>           if (copy_to_user(user_item_ptr, &item, sizeof(item)))
>>>>               return -EFAULT;
>>>>       }
>>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>>> index 39e93f10f2cd..d25f21e3c107 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 {
>>>>       __u64 query_id;
>>>> +#define DRM_I915_QUERY_ID_SLICES_INFO    0x01
>>>> +#define DRM_I915_QUERY_ID_SUBSLICES_INFO 0x02
>>>> +#define DRM_I915_QUERY_ID_EUS_INFO       0x03
>>>
>>> Would DRM_I915_QUERY_ID_"singular"_INFO read better? In struct names 
>>> as well if so?
>>
>> Hmm... Sorry, I don't understand your comment here.
>> What would be "singular" ?
>
> Opposite of plural, like:
>
> _QUERY_ID_SLICE_INFO, _QUERY_ID_SUBSLICE_INFO, _QUERY_ID_EU_INFO.
>
> Actually now I'm thinking if _ID_ part could also be removed for no loss?

Yeah, sure.

>
> Regards,
>
> Tvrtko
>
>

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

✓ Fi.CI.BAT: success 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/36486/
State : success

== Summary ==

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

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:423s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:499s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:482s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:490s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:476s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:458s
fi-elk-e7500     total:224  pass:168  dwarn:10  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:280s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:513s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:392s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:402s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:463s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:415s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:461s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:501s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:505s
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:429s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:509s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:533s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:485s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:490s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:526s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:401s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:478s

37eb04c6fe0cb220563d14430cd9cdec7be0138e drm-tip: 2018y-01m-15d-14h-18m-36s UTC integration manifest
255292e3e095 drm/i915: expose rcs topology through query uAPI
795c9cdb8a88 drm/i915: add query uAPI
7e507f7d804f drm/i915: add rcs topology to error state
19cb6218770f drm/i915/debugfs: add rcs topology entry
888ac1f9167f drm/i915/debugfs: reuse max slice/subslices already stored in sseu
a55bb34ad567 drm/i915: store all subslice masks

== Logs ==

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

✗ Fi.CI.IGT: failure 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/36486/
State : failure

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                pass       -> FAIL       (shard-snb) fdo#101623
Test kms_vblank:
        Subgroup accuracy-idle:
                pass       -> FAIL       (shard-hsw) fdo#102583
Test kms_cursor_crc:
        Subgroup cursor-256x256-suspend:
                pass       -> DMESG-WARN (shard-snb) fdo#103375
Test gem_pwrite:
        Subgroup small-gtt-fbr:
                pass       -> INCOMPLETE (shard-hsw)
Test kms_flip:
        Subgroup plain-flip-ts-check-interruptible:
                pass       -> FAIL       (shard-snb) fdo#100368

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368

shard-hsw        total:2666 pass:1509 dwarn:1   dfail:0   fail:11  skip:1144 time:8690s
shard-snb        total:2723 pass:1314 dwarn:2   dfail:0   fail:12  skip:1395 time:7970s

== Logs ==

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