On 08/09/2015 13:53, Daniluk, Lukasz wrote:
-----Original Message-----
From: Arun Siluvery [mailto:arun.siluvery@xxxxxxxxxxxxxxx]
Sent: Wednesday, September 2, 2015 17:08
To: Daniluk, Lukasz; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH v2] drm/i915/bdw: Check for slice, subslice and
EU count for BDW
On 02/09/2015 16:47, Łukasz Daniluk wrote:
Added checks for available slices, subslices and EUs for Broadwell.
This information is filled in intel_device_info and is available to
user with GET_PARAM.
Added checks for enabled slices, subslices and EU for Broadwell. This
information is based on available counts but takes power gated slices
into account. It can be read in debugfs.
Introduce new register defines that contain information on slices on
Broadwell.
v2:
- Introduce GT_SLICE_INFO register
- Change Broadwell sseu_device_status function to use GT_SLICE_INFO
register instead of RPCS register
- Undo removal of dev_priv variables in Cherryview and Gen9
sseu_device_satus functions
Cc: Jeff Mcgee <jeff.mcgee@xxxxxxxxx>
Signed-off-by: Łukasz Daniluk <lukasz.daniluk@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++-
drivers/gpu/drm/i915/i915_dma.c | 89
+++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_reg.h | 22 ++++++++-
3 files changed, 137 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 23a69307..e8a62ef 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4932,13 +4932,38 @@ static void gen9_sseu_device_status(struct
drm_device *dev,
}
}
+static void broadwell_sseu_device_status(struct drm_device *dev,
+ struct sseu_dev_status *stat)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ int s;
+ u32 slice_info = I915_READ(GEN8_GT_SLICE_INFO);
I don't see this register (0x138064 as defined below) in bdw spec.
Yes, this register isn't listed in spec. I am unsure when the full spec for this register
will be published.
+
+ stat->slice_total =
+ hweight32(slice_info & GEN8_LSLICESTAT_MASK);
why not in a single line, seems to fit under 80 chars.
I was using longer define name and didn't recheck if this line fits under 80. Thanks for catch.
+
+ if (stat->slice_total)
+ {
+ stat->subslice_per_slice = INTEL_INFO(dev)->subslice_per_slice;
+ stat->eu_per_subslice = INTEL_INFO(dev)->eu_per_subslice;
+ stat->subslice_total = stat->slice_total * stat-
subslice_per_slice;
+ stat->eu_total = stat->eu_per_subslice * stat->subslice_total;
+
Please reorder such that all subslice values are populated first followed by EUs
to follow an order.
Okay. My idea was to order them based on where value comes from (is it computed here or
simply read with INTEL_INFO).
+ /* subtract fused off EU(s) from enabled slice(s) */
+ for (s = 0; s < stat->slice_total; s++) {
+ u8 subslice_7eu = INTEL_INFO(dev)->subslice_7eu[s];
+ stat->eu_total -= hweight8(subslice_7eu);
+ }
+ }
+}
+
static int i915_sseu_status(struct seq_file *m, void *unused)
{
struct drm_info_node *node = (struct drm_info_node *) m->private;
struct drm_device *dev = node->minor->dev;
struct sseu_dev_status stat;
- if ((INTEL_INFO(dev)->gen < 8) || IS_BROADWELL(dev))
+ if (INTEL_INFO(dev)->gen < 8)
return -ENODEV;
seq_puts(m, "SSEU Device Info\n");
@@ -4963,6 +4988,8 @@ static int i915_sseu_status(struct seq_file *m, void
*unused)
memset(&stat, 0, sizeof(stat));
if (IS_CHERRYVIEW(dev)) {
cherryview_sseu_device_status(dev, &stat);
+ } else if (IS_BROADWELL(dev)) {
+ broadwell_sseu_device_status(dev, &stat);
} else if (INTEL_INFO(dev)->gen >= 9) {
gen9_sseu_device_status(dev, &stat);
}
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c index ab37d11..2d52b1e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -705,6 +705,93 @@ static void gen9_sseu_info_init(struct drm_device
*dev)
info->has_eu_pg = (info->eu_per_subslice > 2);
}
+static void broadwell_sseu_info_init(struct drm_device *dev) {
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_device_info *info;
+ const int s_max = 3, ss_max = 3, eu_max = 8;
I only see max of 2 slices for bdw in spec.
Register values assume that there can be 3 slices (slice 0,1 and 2) and that’s why
I used 3 here as max slices count.
+ int s, ss;
+ u32 fuse2, eu_disable[s_max], s_enable, ss_disable;
+
+ fuse2 = I915_READ(GEN8_FUSE2);
+ s_enable = (fuse2 & GEN8_F2_S_ENA_MASK) >>
+ GEN8_F2_S_ENA_SHIFT;
+ ss_disable = (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) |
+ ((I915_READ(GEN8_EU_DISABLE1) &
+ GEN8_EU_DIS1_S1_MASK) <<
+ (32 - GEN8_EU_DIS0_S1_SHIFT));
+ eu_disable[2] = (I915_READ(GEN8_EU_DISABLE1) >>
+ GEN8_EU_DIS1_S2_SHIFT) |
+ ((I915_READ(GEN8_EU_DISABLE2) &
+ GEN8_EU_DIS2_S2_MASK) <<
+ (32 - GEN8_EU_DIS1_S2_SHIFT));
+
GEN9_EU_DISABLE(slice) is already available, since the register addresses are
same maybe it can be reused after renaming it to GEN8 instead of creating new
definitions? ofcourse shift/mask are different.
I wouldn't use renaming of register from other generation. Since contents of this register
are read in different way I treated these as different registers and that’s why I choose
to create new define without any references to GEN9 register. However, if the renaming
(or even using existing define) is preferred approach I will modify the patch.
Normally we create a new define for newer Gen if the existing one cannot
be reused, in this case it is the opposite, I am not sure what is best
way to handle this, better to check with Daniel.
regards
Arun
Indentation is off, in general please use () to force indentation.
first two statements can fit in single line.
regards
Arun
Thank you for questions and style suggestions. I will reformat the lines before submitting
updated patch.
---
Łukasz Daniluk
+
+ info = (struct intel_device_info *)&dev_priv->info;
+ info->slice_total = hweight32(s_enable);
+
+ /*
+ * The subslice disable field is global, i.e. it applies
+ * to each of the enabled slices.
+ */
+ info->subslice_per_slice = ss_max - hweight32(ss_disable);
+ info->subslice_total = info->slice_total *
+ info->subslice_per_slice;
+
+ /*
+ * Iterate through enabled slices and subslices to
+ * count the total enabled EU.
+ */
+ for (s = 0; s < s_max; s++) {
+ if (!(s_enable & (0x1 << s)))
+ /* skip disabled slice */
+ continue;
+
+ for (ss = 0; ss < ss_max; ss++) {
+ u32 n_disabled;
+
+ if (ss_disable & (0x1 << ss))
+ /* skip disabled subslice */
+ continue;
+
+ n_disabled = hweight8(eu_disable[s] >>
+ (ss * eu_max));
+
+ /*
+ * Record which subslice(s) has(have) 7 EUs. we
+ * can tune the hash used to spread work among
+ * subslices if they are unbalanced.
+ */
+ if (eu_max - n_disabled == 7)
+ info->subslice_7eu[s] |= 1 << ss;
+
+ info->eu_total += eu_max - n_disabled;
+ }
+ }
+
+ /*
+ * 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
+ * be fused off for die recovery.
+ */
+ info->eu_per_subslice = info->subslice_total ?
+ DIV_ROUND_UP(info->eu_total, info->subslice_total) : 0;
+
+ /*
+ * BDW supports slice power gating on devices with more than
+ * one slice.
+ */
+ info->has_slice_pg = (info->slice_total > 1);
+ info->has_subslice_pg = 0;
+ info->has_eu_pg = 0;
+}
+
/*
* Determine various intel_device_info fields at runtime.
*
@@ -775,6 +862,8 @@ static void intel_device_info_runtime_init(struct
drm_device *dev)
/* Initialize slice/subslice/EU info */
if (IS_CHERRYVIEW(dev))
cherryview_sseu_info_init(dev);
+ else if (IS_BROADWELL(dev))
+ broadwell_sseu_info_init(dev);
else if (INTEL_INFO(dev)->gen >= 9)
gen9_sseu_info_init(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h index be87e3b..37f658f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1841,11 +1841,26 @@ enum skl_disp_power_wells {
#define CHV_FGT_EU_DIS_SS1_R1_MASK (0xf <<
CHV_FGT_EU_DIS_SS1_R1_SHIFT)
#define GEN8_FUSE2 0x9120
+#define GEN8_F2_SS_DIS_SHIFT 21
+#define GEN8_F2_SS_DIS_MASK (0x7 <<
GEN8_F2_SS_DIS_SHIFT)
#define GEN8_F2_S_ENA_SHIFT 25
#define GEN8_F2_S_ENA_MASK (0x7 <<
GEN8_F2_S_ENA_SHIFT)
-#define GEN9_F2_SS_DIS_SHIFT 20
-#define GEN9_F2_SS_DIS_MASK (0xf <<
GEN9_F2_SS_DIS_SHIFT)
+#define GEN8_EU_DISABLE0 0x9134
+#define GEN8_EU_DIS0_S0_MASK 0xffffff
+#define GEN8_EU_DIS0_S1_SHIFT 24
+#define GEN8_EU_DIS0_S1_MASK (0xff <<
GEN8_EU_DIS0_S1_SHIFT)
+
+#define GEN8_EU_DISABLE1 0x9138
+#define GEN8_EU_DIS1_S1_MASK 0xffff
+#define GEN8_EU_DIS1_S2_SHIFT 16
+#define GEN8_EU_DIS1_S2_MASK (0xffff <<
GEN8_EU_DIS1_S2_SHIFT)
+
+#define GEN8_EU_DISABLE2 0x913c
+#define GEN8_EU_DIS2_S2_MASK 0xff
+
+#define GEN9_F2_SS_DIS_SHIFT 20
+#define GEN9_F2_SS_DIS_MASK (0xf <<
GEN9_F2_SS_DIS_SHIFT)
#define GEN9_EU_DISABLE(slice) (0x9134 + (slice)*0x4)
@@ -6822,6 +6837,9 @@ enum skl_disp_power_wells {
#define GEN6_RC6 3
#define GEN6_RC7 4
+#define GEN8_GT_SLICE_INFO 0x138064
+#define GEN8_LSLICESTAT_MASK 0x7
+
#define CHV_POWER_SS0_SIG1 0xa720
#define CHV_POWER_SS1_SIG1 0xa728
#define CHV_SS_PG_ENABLE (1<<1)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
