[Intel-gfx] [PATCH 0/3] Explicity steer l3bank multicast reads when necessary

[Intel-gfx] [PATCH 0/3] Explicity steer l3bank multicast reads when necessary

[Matt Roper]

We've recently learned that when steering reads of multicast registers
that use 'subslice' replication, it's not only important to steer to a
subslice that isn't fused off, but also to steer to the lowest-numbered
subslice.  This is because when Render Power Gating is enabled, grabbing
forcewake will only cause the hardware to power up a single subslice
(referred to as the "minconfig") until/unless a real workload is being
run on the EUs.  If we try to read back a value from a register instance
other than the minconfig subslice, the read operation will either return
0 or random garbage.

Unfortunately this extra requirement to steer to the minconfig means
that the steering target we use for subslice-replicated registers may
not select a valid instance for l3bank-replicated registers.  In cases
where the two types of multicast registers do not have compatible
steering targets, we'll initialize the steering control register to the
proper subslice target at driver load, and then explicitly re-steer
individual reads of l3bank registers as they occur at runtime.

This series sets up an infrastructure to handle explicit resteering of
multiple multicast register types, and then applies it to l3bank
registers.  Our next upcoming platform (which we'll probably start
upstreaming soon) will bring several more types of multicast registers,
each with their own steering criteria, so the infrastructure here is
partially in preparation for those extra multicast types that will be
arriving soon.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>

Daniele Ceraolo Spurio (1):
  drm/i915: extract steered reg access to common function

Matt Roper (2):
  drm/i915: Add GT support for multiple types of multicast steering
  drm/i915: Add support for explicit L3BANK steering

 drivers/gpu/drm/i915/gt/intel_engine_cs.c     |  41 +------
 drivers/gpu/drm/i915/gt/intel_gt.c            | 102 ++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt.h            |   8 ++
 drivers/gpu/drm/i915/gt/intel_gt_types.h      |  26 ++++
 drivers/gpu/drm/i915/gt/intel_workarounds.c   | 112 +++++++-----------
 .../gpu/drm/i915/gt/selftest_workarounds.c    |   2 +-
 drivers/gpu/drm/i915/intel_uncore.c           |  55 +++++++++
 drivers/gpu/drm/i915/intel_uncore.h           |   6 +
 8 files changed, 240 insertions(+), 112 deletions(-)

-- 
2.25.4

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

[Intel-gfx] [PATCH 1/3] drm/i915: extract steered reg access to common function

[Matt Roper]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

New steering cases will be added in the follow-up patches, so prepare a
common helper to avoid code duplication.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 41 +----------------
 drivers/gpu/drm/i915/intel_uncore.c       | 55 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.h       |  6 +++
 3 files changed, 63 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 9ceddfbb1687..8b913c6961c3 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1105,45 +1105,8 @@ static u32
 read_subslice_reg(const struct intel_engine_cs *engine,
 		  int slice, int subslice, i915_reg_t reg)
 {
-	struct drm_i915_private *i915 = engine->i915;
-	struct intel_uncore *uncore = engine->uncore;
-	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
-	enum forcewake_domains fw_domains;
-
-	if (GRAPHICS_VER(i915) >= 11) {
-		mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
-		mcr_ss = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
-	} else {
-		mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
-		mcr_ss = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
-	}
-
-	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
-						    FW_REG_READ);
-	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
-						     GEN8_MCR_SELECTOR,
-						     FW_REG_READ | FW_REG_WRITE);
-
-	spin_lock_irq(&uncore->lock);
-	intel_uncore_forcewake_get__locked(uncore, fw_domains);
-
-	old_mcr = mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
-
-	mcr &= ~mcr_mask;
-	mcr |= mcr_ss;
-	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
-
-	val = intel_uncore_read_fw(uncore, reg);
-
-	mcr &= ~mcr_mask;
-	mcr |= old_mcr & mcr_mask;
-
-	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
-
-	intel_uncore_forcewake_put__locked(uncore, fw_domains);
-	spin_unlock_irq(&uncore->lock);
-
-	return val;
+	return intel_uncore_read_with_mcr_steering(engine->uncore, reg,
+						   slice, subslice);
 }
 
 /* NB: please notice the memset */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 1bed8f666048..d067524f9162 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2277,6 +2277,61 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
 	return fw_domains;
 }
 
+u32 intel_uncore_read_with_mcr_steering_fw(struct intel_uncore *uncore,
+					   i915_reg_t reg,
+					   int slice, int subslice)
+{
+	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
+
+	lockdep_assert_held(&uncore->lock);
+
+	if (GRAPHICS_VER(uncore->i915) >= 11) {
+		mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
+		mcr_ss = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
+	} else {
+		mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
+		mcr_ss = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
+	}
+
+	old_mcr = mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
+
+	mcr &= ~mcr_mask;
+	mcr |= mcr_ss;
+	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
+
+	val = intel_uncore_read_fw(uncore, reg);
+
+	mcr &= ~mcr_mask;
+	mcr |= old_mcr & mcr_mask;
+
+	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
+
+	return val;
+}
+
+u32 intel_uncore_read_with_mcr_steering(struct intel_uncore *uncore,
+					i915_reg_t reg, int slice, int subslice)
+{
+	enum forcewake_domains fw_domains;
+	u32 val;
+
+	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
+						    FW_REG_READ);
+	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
+						     GEN8_MCR_SELECTOR,
+						     FW_REG_READ | FW_REG_WRITE);
+
+	spin_lock_irq(&uncore->lock);
+	intel_uncore_forcewake_get__locked(uncore, fw_domains);
+
+	val = intel_uncore_read_with_mcr_steering_fw(uncore, reg, slice, subslice);
+
+	intel_uncore_forcewake_put__locked(uncore, fw_domains);
+	spin_unlock_irq(&uncore->lock);
+
+	return val;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_uncore.c"
 #include "selftests/intel_uncore.c"
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 59f0da8f1fbb..a18bdb57af7b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -182,6 +182,12 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
 	return uncore->flags & UNCORE_HAS_FIFO;
 }
 
+u32 intel_uncore_read_with_mcr_steering_fw(struct intel_uncore *uncore,
+					   i915_reg_t reg,
+					   int slice, int subslice);
+u32 intel_uncore_read_with_mcr_steering(struct intel_uncore *uncore,
+					i915_reg_t reg,	int slice, int subslice);
+
 void
 intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug);
 void intel_uncore_init_early(struct intel_uncore *uncore,
-- 
2.25.4

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

[Intel-gfx] [PATCH 2/3] drm/i915: Add GT support for multiple types of multicast steering

[Matt Roper]

Although most of our multicast registers are replicated per-subslice, we
also have a small number of multicast registers that are replicated
per-l3 bank instead.  For both types of multicast registers we need to
make sure we steer reads of these registers to a valid instance.
Ideally we'd like to find a specific instance ID that would steer reads
of either type of multicast register to a valid instance (i.e., not
fused off and not powered down), but sometimes the combination of
part-specific fusing and the additional restrictions imposed by Render
Power Gating make it impossible to find any overlap between the set of
valid subslices and valid l3 banks.  This problem will become even more
noticeable on our upcoming platforms since they will be adding
additional types of multicast registers with new types of replication
and rules for finding valid instances for reads.

To handle this we'll continue to pick a suitable subslice instance at
driver startup and program this as the default (sliceid,subsliceid)
setting in the steering control register (0xFDC).  In cases where we
need to read another type of multicast GT register, but the default
subslice steering would not correspond to a valid instance, we'll
explicitly re-steer the single read to a valid value, perform the read,
and then reset the steering to it's "subslice" default.

This patch adds the general functionality to prepare for this explicit
steering of other multicast register types.  We'll plug L3 bank steering
into this in the next patch, and then add additional types of multicast
registers when the support for our next upcoming platform arrives.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c            | 84 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt.h            |  8 ++
 drivers/gpu/drm/i915/gt/intel_gt_types.h      | 22 +++++
 drivers/gpu/drm/i915/gt/intel_workarounds.c   | 28 ++++---
 .../gpu/drm/i915/gt/selftest_workarounds.c    |  2 +-
 5 files changed, 131 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 2161bf01ef8b..f2bea1c20d56 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -697,6 +697,90 @@ void intel_gt_driver_late_release(struct intel_gt *gt)
 	intel_engines_free(gt);
 }
 
+/**
+ * intel_gt_reg_needs_read_steering - determine whether a register read
+ *     requires explicit steering
+ * @gt: GT structure
+ * @reg: the register to check steering requirements for
+ * @type: type of multicast steering to check
+ *
+ * Determines whether @reg needs explicit steering of a specific type for
+ * reads.
+ *
+ * Returns false if @reg does not belong to a register range of the given
+ * steering type, or if the default (subslice-based) steering IDs are suitable
+ * for @type steering too.
+ */
+static bool intel_gt_reg_needs_read_steering(struct intel_gt *gt,
+					     i915_reg_t reg,
+					     enum intel_steering_type type)
+{
+	const u32 offset = i915_mmio_reg_offset(reg);
+	const struct intel_mmio_range *entry;
+
+	if (likely(!intel_gt_needs_read_steering(gt, type)))
+		return false;
+
+	for (entry = gt->steering_table[type]; entry->start < 0xFFFFFF; entry++) {
+		if (offset >= entry->start && offset <= entry->end)
+			return true;
+	}
+
+	return false;
+}
+
+/**
+ * intel_gt_get_valid_steering - determines valid IDs for a class of MCR steering
+ * @gt: GT structure
+ * @type: multicast register type
+ * @sliceid: Slice ID returned
+ * @subsliceid: Subslice ID returned
+ *
+ * Determines sliceid and subsliceid values that will steer reads
+ * of a specific multicast register class to a valid value.
+ */
+static void intel_gt_get_valid_steering(struct intel_gt *gt,
+					enum intel_steering_type type,
+					u8 *sliceid, u8 *subsliceid)
+{
+	switch (type) {
+	default:
+		MISSING_CASE(type);
+		*sliceid = 0;
+		*subsliceid = 0;
+	}
+}
+
+/**
+ * intel_gt_read_register_fw - reads a GT register with support for multicast
+ * @gt: GT structure
+ * @reg: register to read
+ *
+ * This function will read a GT register.  If the register is a multicast
+ * register, the read will be steered to a valid instance (i.e., one that
+ * isn't fused off or powered down by power gating).
+ *
+ * Returns the value from a valid instance of @reg.
+ */
+u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg)
+{
+	int type;
+	u8 sliceid, subsliceid;
+
+	for (type = 0; type < NUM_STEERING_TYPES; type++) {
+		if (intel_gt_reg_needs_read_steering(gt, reg, type)) {
+			intel_gt_get_valid_steering(gt, type, &sliceid,
+						    &subsliceid);
+			return intel_uncore_read_with_mcr_steering_fw(gt->uncore,
+								      reg,
+								      sliceid,
+								      subsliceid);
+		}
+	}
+
+	return intel_uncore_read_fw(gt->uncore, reg);
+}
+
 void intel_gt_info_print(const struct intel_gt_info *info,
 			 struct drm_printer *p)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
index 7ec395cace69..e7aabe0cc5bf 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -75,6 +75,14 @@ static inline bool intel_gt_is_wedged(const struct intel_gt *gt)
 	return unlikely(test_bit(I915_WEDGED, &gt->reset.flags));
 }
 
+static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
+						enum intel_steering_type type)
+{
+	return gt->steering_table[type];
+}
+
+u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
+
 void intel_gt_info_print(const struct intel_gt_info *info,
 			 struct drm_printer *p);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index fecfacf551d5..47957837c8c0 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -31,6 +31,26 @@ struct i915_ggtt;
 struct intel_engine_cs;
 struct intel_uncore;
 
+struct intel_mmio_range {
+       u32 start;
+       u32 end;
+};
+
+/*
+ * The hardware has multiple kinds of multicast register ranges that need
+ * special register steering (and future platforms are expected to add
+ * additional types).
+ *
+ * During driver startup, we initialize the steering control register to
+ * direct reads to a slice/subslice that are valid for the 'subslice' class
+ * of multicast registers.  If another type of steering does not have any
+ * overlap in valid steering targets with 'subslice' style registers, we will
+ * need to explicitly re-steer reads of registers of the other type.
+ */
+enum intel_steering_type {
+	NUM_STEERING_TYPES
+};
+
 enum intel_submission_method {
 	INTEL_SUBMISSION_RING,
 	INTEL_SUBMISSION_ELSP,
@@ -145,6 +165,8 @@ struct intel_gt {
 
 	struct i915_vma *scratch;
 
+	const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
+
 	struct intel_gt_info {
 		intel_engine_mask_t engine_mask;
 		u8 num_engines;
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index b62d1e31a645..689045d3752b 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1247,8 +1247,9 @@ wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
 }
 
 static void
-wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
+wa_list_apply(struct intel_gt *gt, const struct i915_wa_list *wal)
 {
+	struct intel_uncore *uncore = gt->uncore;
 	enum forcewake_domains fw;
 	unsigned long flags;
 	struct i915_wa *wa;
@@ -1263,13 +1264,16 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
 	intel_uncore_forcewake_get__locked(uncore, fw);
 
 	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
-		if (wa->clr)
-			intel_uncore_rmw_fw(uncore, wa->reg, wa->clr, wa->set);
-		else
-			intel_uncore_write_fw(uncore, wa->reg, wa->set);
+		u32 val, old = 0;
+
+		/* open-coded rmw due to steering */
+		old = wa->clr ? intel_gt_read_register_fw(gt, wa->reg) : 0;
+		val = (old & ~wa->clr) | wa->set;
+		if (val != old || !wa->clr)
+			intel_uncore_write_fw(uncore, wa->reg, val);
+
 		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
-			wa_verify(wa,
-				  intel_uncore_read_fw(uncore, wa->reg),
+			wa_verify(wa, intel_gt_read_register_fw(gt, wa->reg),
 				  wal->name, "application");
 	}
 
@@ -1279,10 +1283,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
 
 void intel_gt_apply_workarounds(struct intel_gt *gt)
 {
-	wa_list_apply(gt->uncore, &gt->i915->gt_wa_list);
+	wa_list_apply(gt, &gt->i915->gt_wa_list);
 }
 
-static bool wa_list_verify(struct intel_uncore *uncore,
+static bool wa_list_verify(struct intel_gt *gt,
 			   const struct i915_wa_list *wal,
 			   const char *from)
 {
@@ -1292,7 +1296,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
 
 	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
 		ok &= wa_verify(wa,
-				intel_uncore_read(uncore, wa->reg),
+				intel_gt_read_register_fw(gt, wa->reg),
 				wal->name, from);
 
 	return ok;
@@ -1300,7 +1304,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
 
 bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
 {
-	return wa_list_verify(gt->uncore, &gt->i915->gt_wa_list, from);
+	return wa_list_verify(gt, &gt->i915->gt_wa_list, from);
 }
 
 __maybe_unused
@@ -2081,7 +2085,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
 
 void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
 {
-	wa_list_apply(engine->uncore, &engine->wa_list);
+	wa_list_apply(engine->gt, &engine->wa_list);
 }
 
 struct mcr_range {
diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index c30754daf4b1..7ebc4edb8ecf 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -1147,7 +1147,7 @@ verify_wa_lists(struct intel_gt *gt, struct wa_lists *lists,
 	enum intel_engine_id id;
 	bool ok = true;
 
-	ok &= wa_list_verify(gt->uncore, &lists->gt_wa_list, str);
+	ok &= wa_list_verify(gt, &lists->gt_wa_list, str);
 
 	for_each_engine(engine, gt, id) {
 		struct intel_context *ce;
-- 
2.25.4

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

[Intel-gfx] [PATCH 3/3] drm/i915: Add support for explicit L3BANK steering

[Matt Roper]

Because Render Power Gating restricts us to just a single subslice as a
valid steering target for reads of multicast registers in a SUBSLICE
range, the default steering we setup at init may not lead to a suitable
target for L3BANK multicast register.  In cases where it does not, use
explicit runtime steering whenever an L3BANK multicast register is read.

While we're at it, let's simplify the function a little bit and drop its
support for gen10/CNL since no such platforms ever materialized for real
use.  Multicast register steering is already an area that causes enough
confusion; no need to complicate it with what's effectively dead code.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c          | 18 +++++
 drivers/gpu/drm/i915/gt/intel_gt_types.h    |  4 +
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 84 ++++++---------------
 3 files changed, 46 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index f2bea1c20d56..2c9cc34b0cbd 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -83,6 +83,11 @@ void intel_gt_init_hw_early(struct intel_gt *gt, struct i915_ggtt *ggtt)
 	gt->ggtt = ggtt;
 }
 
+static const struct intel_mmio_range icl_l3bank_steering_table[] = {
+       { 0x00B100, 0x00B3FF },
+       { 0xFFFFFF, 0xFFFFFF }, /* terminating entry */
+};
+
 int intel_gt_init_mmio(struct intel_gt *gt)
 {
 	intel_gt_init_clock_frequency(gt);
@@ -90,6 +95,13 @@ int intel_gt_init_mmio(struct intel_gt *gt)
 	intel_uc_init_mmio(&gt->uc);
 	intel_sseu_info_init(gt);
 
+	if (GRAPHICS_VER(gt->i915) >= 11) {
+		gt->steering_table[L3BANK] = icl_l3bank_steering_table;
+		gt->info.l3bank_mask =
+			intel_uncore_read(&gt->i915->uncore, GEN10_MIRROR_FUSE3) &
+			GEN10_L3BANK_MASK;
+	}
+
 	return intel_engines_init_mmio(gt);
 }
 
@@ -744,6 +756,12 @@ static void intel_gt_get_valid_steering(struct intel_gt *gt,
 					u8 *sliceid, u8 *subsliceid)
 {
 	switch (type) {
+	case L3BANK:
+		GEM_DEBUG_WARN_ON(!gt->info.l3bank_mask); /* should be impossible! */
+
+		*sliceid = __ffs(gt->info.l3bank_mask);
+		*subsliceid = 0;        /* unused */
+		break;
 	default:
 		MISSING_CASE(type);
 		*sliceid = 0;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index 47957837c8c0..5ecad25de6ed 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -48,6 +48,8 @@ struct intel_mmio_range {
  * need to explicitly re-steer reads of registers of the other type.
  */
 enum intel_steering_type {
+	L3BANK,
+
 	NUM_STEERING_TYPES
 };
 
@@ -174,6 +176,8 @@ struct intel_gt {
 		/* Media engine access to SFC per instance */
 		u8 vdbox_sfc_access;
 
+		u32 l3bank_mask;
+
 		/* Slice/subslice/EU info */
 		struct sseu_dev_info sseu;
 	} info;
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 689045d3752b..a0be3c09a7f9 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -944,71 +944,37 @@ cfl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
 }
 
 static void
-wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
+icl_wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
 {
 	const struct sseu_dev_info *sseu = &i915->gt.info.sseu;
 	unsigned int slice, subslice;
-	u32 l3_en, mcr, mcr_mask;
+	u32 mcr, mcr_mask;
 
-	GEM_BUG_ON(GRAPHICS_VER(i915) < 10);
+	GEM_BUG_ON(GRAPHICS_VER(i915) < 11);
+	GEM_BUG_ON(hweight8(sseu->slice_mask) > 1);
+	slice = 0;
 
 	/*
-	 * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
-	 * L3Banks could be fused off in single slice scenario. If that is
-	 * the case, we might need to program MCR select to a valid L3Bank
-	 * by default, to make sure we correctly read certain registers
-	 * later on (in the range 0xB100 - 0xB3FF).
-	 *
-	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl
-	 * Before any MMIO read into slice/subslice specific registers, MCR
-	 * packet control register needs to be programmed to point to any
-	 * enabled s/ss pair. Otherwise, incorrect values will be returned.
-	 * This means each subsequent MMIO read will be forwarded to an
-	 * specific s/ss combination, but this is OK since these registers
-	 * are consistent across s/ss in almost all cases. In the rare
-	 * occasions, such as INSTDONE, where this value is dependent
-	 * on s/ss combo, the read should be done with read_subslice_reg.
-	 *
-	 * Since GEN8_MCR_SELECTOR contains dual-purpose bits which select both
-	 * to which subslice, or to which L3 bank, the respective mmio reads
-	 * will go, we have to find a common index which works for both
-	 * accesses.
-	 *
-	 * Case where we cannot find a common index fortunately should not
-	 * happen in production hardware, so we only emit a warning instead of
-	 * implementing something more complex that requires checking the range
-	 * of every MMIO read.
+	 * Although a platform may have subslices, we need to always steer
+	 * reads to the lowest instance that isn't fused off.  When Render
+	 * Power Gating is enabled, grabbing forcewake will only power up a
+	 * single subslice (the "minconfig") if there isn't a real workload
+	 * that needs to be run; this means that if we steer register reads to
+	 * one of the higher subslices, we run the risk of reading back 0's or
+	 * random garbage.
 	 */
+	subslice = __ffs(intel_sseu_get_subslices(sseu, slice));
 
-	if (GRAPHICS_VER(i915) >= 10 && is_power_of_2(sseu->slice_mask)) {
-		u32 l3_fuse =
-			intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3) &
-			GEN10_L3BANK_MASK;
-
-		drm_dbg(&i915->drm, "L3 fuse = %x\n", l3_fuse);
-		l3_en = ~(l3_fuse << GEN10_L3BANK_PAIR_COUNT | l3_fuse);
-	} else {
-		l3_en = ~0;
-	}
+	/*
+	 * If the subslice we picked above also steers us to a valid L3 bank,
+	 * then we can just rely on the default steering and won't need to
+	 * worry about explicitly re-steering L3BANK reads later.
+	 */
+	if (i915->gt.info.l3bank_mask & BIT(subslice))
+		i915->gt.steering_table[L3BANK] = NULL;
 
-	slice = fls(sseu->slice_mask) - 1;
-	subslice = fls(l3_en & intel_sseu_get_subslices(sseu, slice));
-	if (!subslice) {
-		drm_warn(&i915->drm,
-			 "No common index found between subslice mask %x and L3 bank mask %x!\n",
-			 intel_sseu_get_subslices(sseu, slice), l3_en);
-		subslice = fls(l3_en);
-		drm_WARN_ON(&i915->drm, !subslice);
-	}
-	subslice--;
-
-	if (GRAPHICS_VER(i915) >= 11) {
-		mcr = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
-		mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
-	} else {
-		mcr = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
-		mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
-	}
+	mcr = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
+	mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
 
 	drm_dbg(&i915->drm, "MCR slice/subslice = %x\n", mcr);
 
@@ -1018,8 +984,6 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
 static void
 cnl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
 {
-	wa_init_mcr(i915, wal);
-
 	/* WaInPlaceDecompressionHang:cnl */
 	wa_write_or(wal,
 		    GEN9_GAMT_ECO_REG_RW_IA,
@@ -1029,7 +993,7 @@ cnl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
 static void
 icl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
 {
-	wa_init_mcr(i915, wal);
+	icl_wa_init_mcr(i915, wal);
 
 	/* WaInPlaceDecompressionHang:icl */
 	wa_write_or(wal,
@@ -1111,7 +1075,7 @@ static void
 gen12_gt_workarounds_init(struct drm_i915_private *i915,
 			  struct i915_wa_list *wal)
 {
-	wa_init_mcr(i915, wal);
+	icl_wa_init_mcr(i915, wal);
 
 	/* Wa_14011060649:tgl,rkl,dg1,adls */
 	wa_14011060649(i915, wal);
-- 
2.25.4

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Explicity steer l3bank multicast reads when necessary

[Patchwork]

== Series Details ==

Series: Explicity steer l3bank multicast reads when necessary
URL   : https://patchwork.freedesktop.org/series/91485/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
16715f8678e5 drm/i915: extract steered reg access to common function
-:89: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#89: FILE: drivers/gpu/drm/i915/intel_uncore.c:2296:
+	old_mcr = mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);

total: 0 errors, 0 warnings, 1 checks, 120 lines checked
3707583bbcf3 drm/i915: Add GT support for multiple types of multicast steering
-:159: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#159: FILE: drivers/gpu/drm/i915/gt/intel_gt_types.h:35:
+       u32 start;$

-:160: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#160: FILE: drivers/gpu/drm/i915/gt/intel_gt_types.h:36:
+       u32 end;$

total: 0 errors, 2 warnings, 0 checks, 214 lines checked
ee481231a00b drm/i915: Add support for explicit L3BANK steering
-:28: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#28: FILE: drivers/gpu/drm/i915/gt/intel_gt.c:87:
+       { 0x00B100, 0x00B3FF },$

-:29: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#29: FILE: drivers/gpu/drm/i915/gt/intel_gt.c:88:
+       { 0xFFFFFF, 0xFFFFFF }, /* terminating entry */$

total: 0 errors, 2 warnings, 0 checks, 169 lines checked


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

[Intel-gfx] ✗ Fi.CI.BAT: failure for Explicity steer l3bank multicast reads when necessary

[Patchwork]

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

== Series Details ==

Series: Explicity steer l3bank multicast reads when necessary
URL   : https://patchwork.freedesktop.org/series/91485/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_10222 -> Patchwork_20365
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_20365 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_20365, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_20365:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@workarounds:
    - fi-ivb-3770:        [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-ivb-3770/igt@i915_selftest@live@workarounds.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-ivb-3770/igt@i915_selftest@live@workarounds.html
    - fi-skl-6700k2:      [PASS][3] -> [DMESG-FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-skl-6700k2/igt@i915_selftest@live@workarounds.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-skl-6700k2/igt@i915_selftest@live@workarounds.html
    - fi-cml-s:           [PASS][5] -> [DMESG-FAIL][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-cml-s/igt@i915_selftest@live@workarounds.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-cml-s/igt@i915_selftest@live@workarounds.html
    - fi-kbl-x1275:       [PASS][7] -> [DMESG-FAIL][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-x1275/igt@i915_selftest@live@workarounds.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-x1275/igt@i915_selftest@live@workarounds.html
    - fi-cfl-guc:         [PASS][9] -> [DMESG-FAIL][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-cfl-guc/igt@i915_selftest@live@workarounds.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-cfl-guc/igt@i915_selftest@live@workarounds.html
    - fi-kbl-7567u:       [PASS][11] -> [DMESG-FAIL][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-7567u/igt@i915_selftest@live@workarounds.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-7567u/igt@i915_selftest@live@workarounds.html
    - fi-skl-6600u:       [PASS][13] -> [DMESG-FAIL][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-skl-6600u/igt@i915_selftest@live@workarounds.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-skl-6600u/igt@i915_selftest@live@workarounds.html
    - fi-glk-dsi:         [PASS][15] -> [DMESG-FAIL][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-glk-dsi/igt@i915_selftest@live@workarounds.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-glk-dsi/igt@i915_selftest@live@workarounds.html
    - fi-kbl-8809g:       [PASS][17] -> [DMESG-FAIL][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-8809g/igt@i915_selftest@live@workarounds.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-8809g/igt@i915_selftest@live@workarounds.html
    - fi-cfl-8700k:       [PASS][19] -> [DMESG-FAIL][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-cfl-8700k/igt@i915_selftest@live@workarounds.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-cfl-8700k/igt@i915_selftest@live@workarounds.html
    - fi-kbl-r:           [PASS][21] -> [DMESG-FAIL][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-r/igt@i915_selftest@live@workarounds.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-r/igt@i915_selftest@live@workarounds.html
    - fi-cfl-8109u:       [PASS][23] -> [DMESG-FAIL][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-cfl-8109u/igt@i915_selftest@live@workarounds.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-cfl-8109u/igt@i915_selftest@live@workarounds.html
    - fi-kbl-7500u:       [PASS][25] -> [DMESG-FAIL][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-7500u/igt@i915_selftest@live@workarounds.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-7500u/igt@i915_selftest@live@workarounds.html
    - fi-kbl-guc:         [PASS][27] -> [DMESG-FAIL][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-guc/igt@i915_selftest@live@workarounds.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-guc/igt@i915_selftest@live@workarounds.html
    - fi-hsw-4770:        [PASS][29] -> [DMESG-FAIL][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-hsw-4770/igt@i915_selftest@live@workarounds.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-hsw-4770/igt@i915_selftest@live@workarounds.html
    - fi-bxt-dsi:         [PASS][31] -> [DMESG-FAIL][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-bxt-dsi/igt@i915_selftest@live@workarounds.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-bxt-dsi/igt@i915_selftest@live@workarounds.html
    - fi-cml-u2:          [PASS][33] -> [DMESG-FAIL][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-cml-u2/igt@i915_selftest@live@workarounds.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-cml-u2/igt@i915_selftest@live@workarounds.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@gt_engines:
    - {fi-ehl-2}:         [DMESG-FAIL][35] ([i915#1222]) -> [FAIL][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-ehl-2/igt@i915_selftest@live@gt_engines.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-ehl-2/igt@i915_selftest@live@gt_engines.html
    - {fi-jsl-1}:         [DMESG-FAIL][37] ([i915#1222]) -> [FAIL][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-jsl-1/igt@i915_selftest@live@gt_engines.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-jsl-1/igt@i915_selftest@live@gt_engines.html

  * igt@i915_selftest@live@workarounds:
    - {fi-hsw-gt1}:       [PASS][39] -> [DMESG-FAIL][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-hsw-gt1/igt@i915_selftest@live@workarounds.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-hsw-gt1/igt@i915_selftest@live@workarounds.html
    - {fi-tgl-dsi}:       [PASS][41] -> [DMESG-FAIL][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-tgl-dsi/igt@i915_selftest@live@workarounds.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-tgl-dsi/igt@i915_selftest@live@workarounds.html
    - {fi-rkl-11500t}:    NOTRUN -> [DMESG-FAIL][43]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-rkl-11500t/igt@i915_selftest@live@workarounds.html
    - {fi-tgl-1115g4}:    [PASS][44] -> [DMESG-FAIL][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-tgl-1115g4/igt@i915_selftest@live@workarounds.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-tgl-1115g4/igt@i915_selftest@live@workarounds.html
    - {fi-jsl-1}:         [DMESG-WARN][46] ([i915#1222]) -> [DMESG-FAIL][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-jsl-1/igt@i915_selftest@live@workarounds.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-jsl-1/igt@i915_selftest@live@workarounds.html
    - {fi-ehl-2}:         [DMESG-WARN][48] ([i915#1222]) -> [DMESG-FAIL][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-ehl-2/igt@i915_selftest@live@workarounds.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-ehl-2/igt@i915_selftest@live@workarounds.html

  
Known issues
------------

  Here are the changes found in Patchwork_20365 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-kbl-soraka:      [PASS][50] -> [INCOMPLETE][51] ([i915#155])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-soraka/igt@gem_exec_suspend@basic-s0.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-soraka/igt@gem_exec_suspend@basic-s0.html

  * igt@runner@aborted:
    - fi-icl-u2:          NOTRUN -> [FAIL][52] ([i915#1569] / [i915#3363] / [k.org#202973])
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-icl-u2/igt@runner@aborted.html
    - fi-icl-y:           NOTRUN -> [FAIL][53] ([i915#1569])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-icl-y/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload:
    - {fi-ehl-2}:         [DMESG-WARN][54] ([i915#1222]) -> [PASS][55] +36 similar issues
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-ehl-2/igt@i915_module_load@reload.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-ehl-2/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-guc:         [SKIP][56] ([fdo#109271]) -> [PASS][57]
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@gt_lrc:
    - {fi-jsl-1}:         [DMESG-WARN][58] ([i915#1222]) -> [PASS][59] +36 similar issues
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10222/fi-jsl-1/igt@i915_selftest@live@gt_lrc.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/fi-jsl-1/igt@i915_selftest@live@gt_lrc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1222]: https://gitlab.freedesktop.org/drm/intel/issues/1222
  [i915#155]: https://gitlab.freedesktop.org/drm/intel/issues/155
  [i915#1569]: https://gitlab.freedesktop.org/drm/intel/issues/1569
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3012]: https://gitlab.freedesktop.org/drm/intel/issues/3012
  [i915#3276]: https://gitlab.freedesktop.org/drm/intel/issues/3276
  [i915#3277]: https://gitlab.freedesktop.org/drm/intel/issues/3277
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3283]: https://gitlab.freedesktop.org/drm/intel/issues/3283
  [i915#3363]: https://gitlab.freedesktop.org/drm/intel/issues/3363
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3542]: https://gitlab.freedesktop.org/drm/intel/issues/3542
  [i915#3544]: https://gitlab.freedesktop.org/drm/intel/issues/3544
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [k.org#202973]: https://bugzilla.kernel.org/show_bug.cgi?id=202973


Participating hosts (42 -> 39)
------------------------------

  Additional (1): fi-rkl-11500t 
  Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 


Build changes
-------------

  * Linux: CI_DRM_10222 -> Patchwork_20365

  CI-20190529: 20190529
  CI_DRM_10222: 9b5675dc51137543709a5ec444b0d7076e43198e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6105: 598a154680374e7875ae9ffc98425abc57398b2f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_20365: ee481231a00bce45f4a7d3a17061102e071db7f2 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ee481231a00b drm/i915: Add support for explicit L3BANK steering
3707583bbcf3 drm/i915: Add GT support for multiple types of multicast steering
16715f8678e5 drm/i915: extract steered reg access to common function

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20365/index.html

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

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

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

Re: [Intel-gfx] [PATCH 1/3] drm/i915: extract steered reg access to common function

[Rodrigo Vivi]

On Mon, Jun 14, 2021 at 08:34:31PM -0700, Matt Roper wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> New steering cases will be added in the follow-up patches, so prepare a
> common helper to avoid code duplication.
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 41 +----------------
>  drivers/gpu/drm/i915/intel_uncore.c       | 55 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.h       |  6 +++
>  3 files changed, 63 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 9ceddfbb1687..8b913c6961c3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1105,45 +1105,8 @@ static u32
>  read_subslice_reg(const struct intel_engine_cs *engine,
>  		  int slice, int subslice, i915_reg_t reg)
>  {
> -	struct drm_i915_private *i915 = engine->i915;
> -	struct intel_uncore *uncore = engine->uncore;
> -	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
> -	enum forcewake_domains fw_domains;
> -
> -	if (GRAPHICS_VER(i915) >= 11) {
> -		mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
> -		mcr_ss = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
> -	} else {
> -		mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
> -		mcr_ss = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
> -	}
> -
> -	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
> -						    FW_REG_READ);
> -	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
> -						     GEN8_MCR_SELECTOR,
> -						     FW_REG_READ | FW_REG_WRITE);
> -
> -	spin_lock_irq(&uncore->lock);
> -	intel_uncore_forcewake_get__locked(uncore, fw_domains);
> -
> -	old_mcr = mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
> -
> -	mcr &= ~mcr_mask;
> -	mcr |= mcr_ss;
> -	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
> -
> -	val = intel_uncore_read_fw(uncore, reg);
> -
> -	mcr &= ~mcr_mask;
> -	mcr |= old_mcr & mcr_mask;
> -
> -	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
> -
> -	intel_uncore_forcewake_put__locked(uncore, fw_domains);
> -	spin_unlock_irq(&uncore->lock);
> -
> -	return val;
> +	return intel_uncore_read_with_mcr_steering(engine->uncore, reg,
> +						   slice, subslice);
>  }
>  
>  /* NB: please notice the memset */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 1bed8f666048..d067524f9162 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2277,6 +2277,61 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
>  	return fw_domains;
>  }
>  
> +u32 intel_uncore_read_with_mcr_steering_fw(struct intel_uncore *uncore,
> +					   i915_reg_t reg,
> +					   int slice, int subslice)
> +{
> +	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
> +
> +	lockdep_assert_held(&uncore->lock);
> +
> +	if (GRAPHICS_VER(uncore->i915) >= 11) {
> +		mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
> +		mcr_ss = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
> +	} else {
> +		mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
> +		mcr_ss = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
> +	}
> +
> +	old_mcr = mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
> +
> +	mcr &= ~mcr_mask;
> +	mcr |= mcr_ss;
> +	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
> +
> +	val = intel_uncore_read_fw(uncore, reg);
> +
> +	mcr &= ~mcr_mask;
> +	mcr |= old_mcr & mcr_mask;
> +
> +	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
> +
> +	return val;
> +}
> +
> +u32 intel_uncore_read_with_mcr_steering(struct intel_uncore *uncore,
> +					i915_reg_t reg, int slice, int subslice)
> +{
> +	enum forcewake_domains fw_domains;
> +	u32 val;
> +
> +	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
> +						    FW_REG_READ);
> +	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
> +						     GEN8_MCR_SELECTOR,
> +						     FW_REG_READ | FW_REG_WRITE);
> +
> +	spin_lock_irq(&uncore->lock);
> +	intel_uncore_forcewake_get__locked(uncore, fw_domains);
> +
> +	val = intel_uncore_read_with_mcr_steering_fw(uncore, reg, slice, subslice);
> +
> +	intel_uncore_forcewake_put__locked(uncore, fw_domains);
> +	spin_unlock_irq(&uncore->lock);
> +
> +	return val;
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/mock_uncore.c"
>  #include "selftests/intel_uncore.c"
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 59f0da8f1fbb..a18bdb57af7b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -182,6 +182,12 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>  	return uncore->flags & UNCORE_HAS_FIFO;
>  }
>  
> +u32 intel_uncore_read_with_mcr_steering_fw(struct intel_uncore *uncore,
> +					   i915_reg_t reg,
> +					   int slice, int subslice);
> +u32 intel_uncore_read_with_mcr_steering(struct intel_uncore *uncore,
> +					i915_reg_t reg,	int slice, int subslice);
> +
>  void
>  intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug);
>  void intel_uncore_init_early(struct intel_uncore *uncore,
> -- 
> 2.25.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Add GT support for multiple types of multicast steering

[Rodrigo Vivi]

On Mon, Jun 14, 2021 at 08:34:32PM -0700, Matt Roper wrote:
> Although most of our multicast registers are replicated per-subslice, we
> also have a small number of multicast registers that are replicated
> per-l3 bank instead.  For both types of multicast registers we need to
> make sure we steer reads of these registers to a valid instance.
> Ideally we'd like to find a specific instance ID that would steer reads
> of either type of multicast register to a valid instance (i.e., not
> fused off and not powered down), but sometimes the combination of
> part-specific fusing and the additional restrictions imposed by Render
> Power Gating make it impossible to find any overlap between the set of
> valid subslices and valid l3 banks.  This problem will become even more
> noticeable on our upcoming platforms since they will be adding
> additional types of multicast registers with new types of replication
> and rules for finding valid instances for reads.
> 
> To handle this we'll continue to pick a suitable subslice instance at
> driver startup and program this as the default (sliceid,subsliceid)
> setting in the steering control register (0xFDC).  In cases where we
> need to read another type of multicast GT register, but the default
> subslice steering would not correspond to a valid instance, we'll
> explicitly re-steer the single read to a valid value, perform the read,
> and then reset the steering to it's "subslice" default.
> 
> This patch adds the general functionality to prepare for this explicit
> steering of other multicast register types.  We'll plug L3 bank steering
> into this in the next patch, and then add additional types of multicast
> registers when the support for our next upcoming platform arrives.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c            | 84 +++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_gt.h            |  8 ++
>  drivers/gpu/drm/i915/gt/intel_gt_types.h      | 22 +++++
>  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 28 ++++---
>  .../gpu/drm/i915/gt/selftest_workarounds.c    |  2 +-
>  5 files changed, 131 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 2161bf01ef8b..f2bea1c20d56 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -697,6 +697,90 @@ void intel_gt_driver_late_release(struct intel_gt *gt)
>  	intel_engines_free(gt);
>  }
>  
> +/**
> + * intel_gt_reg_needs_read_steering - determine whether a register read
> + *     requires explicit steering
> + * @gt: GT structure
> + * @reg: the register to check steering requirements for
> + * @type: type of multicast steering to check
> + *
> + * Determines whether @reg needs explicit steering of a specific type for
> + * reads.
> + *
> + * Returns false if @reg does not belong to a register range of the given
> + * steering type, or if the default (subslice-based) steering IDs are suitable
> + * for @type steering too.
> + */
> +static bool intel_gt_reg_needs_read_steering(struct intel_gt *gt,
> +					     i915_reg_t reg,
> +					     enum intel_steering_type type)
> +{
> +	const u32 offset = i915_mmio_reg_offset(reg);
> +	const struct intel_mmio_range *entry;
> +
> +	if (likely(!intel_gt_needs_read_steering(gt, type)))
> +		return false;
> +
> +	for (entry = gt->steering_table[type]; entry->start < 0xFFFFFF; entry++) {

I'm not comfortable with this stop condition...
we should know the right amount of entries that we have.
Even if that means having another intermediate struct.

> +		if (offset >= entry->start && offset <= entry->end)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * intel_gt_get_valid_steering - determines valid IDs for a class of MCR steering
> + * @gt: GT structure
> + * @type: multicast register type
> + * @sliceid: Slice ID returned
> + * @subsliceid: Subslice ID returned
> + *
> + * Determines sliceid and subsliceid values that will steer reads
> + * of a specific multicast register class to a valid value.
> + */
> +static void intel_gt_get_valid_steering(struct intel_gt *gt,
> +					enum intel_steering_type type,
> +					u8 *sliceid, u8 *subsliceid)
> +{
> +	switch (type) {
> +	default:
> +		MISSING_CASE(type);

I understand that we are preparing the infra for the upcoming cases,
but adding a missing_case warn by default doesn't look the right way...
did CI not complain?!

> +		*sliceid = 0;
> +		*subsliceid = 0;
> +	}
> +}
> +
> +/**
> + * intel_gt_read_register_fw - reads a GT register with support for multicast
> + * @gt: GT structure
> + * @reg: register to read
> + *
> + * This function will read a GT register.  If the register is a multicast
> + * register, the read will be steered to a valid instance (i.e., one that
> + * isn't fused off or powered down by power gating).
> + *
> + * Returns the value from a valid instance of @reg.
> + */
> +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg)
> +{
> +	int type;
> +	u8 sliceid, subsliceid;
> +
> +	for (type = 0; type < NUM_STEERING_TYPES; type++) {
> +		if (intel_gt_reg_needs_read_steering(gt, reg, type)) {
> +			intel_gt_get_valid_steering(gt, type, &sliceid,
> +						    &subsliceid);
> +			return intel_uncore_read_with_mcr_steering_fw(gt->uncore,
> +								      reg,
> +								      sliceid,
> +								      subsliceid);
> +		}
> +	}
> +
> +	return intel_uncore_read_fw(gt->uncore, reg);
> +}
> +
>  void intel_gt_info_print(const struct intel_gt_info *info,
>  			 struct drm_printer *p)
>  {
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
> index 7ec395cace69..e7aabe0cc5bf 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -75,6 +75,14 @@ static inline bool intel_gt_is_wedged(const struct intel_gt *gt)
>  	return unlikely(test_bit(I915_WEDGED, &gt->reset.flags));
>  }
>  
> +static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
> +						enum intel_steering_type type)
> +{
> +	return gt->steering_table[type];

then if we know the right amount we also don't need this function right?!

> +}
> +
> +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
> +
>  void intel_gt_info_print(const struct intel_gt_info *info,
>  			 struct drm_printer *p);
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index fecfacf551d5..47957837c8c0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -31,6 +31,26 @@ struct i915_ggtt;
>  struct intel_engine_cs;
>  struct intel_uncore;
>  
> +struct intel_mmio_range {
> +       u32 start;
> +       u32 end;
> +};
> +
> +/*
> + * The hardware has multiple kinds of multicast register ranges that need
> + * special register steering (and future platforms are expected to add
> + * additional types).
> + *
> + * During driver startup, we initialize the steering control register to
> + * direct reads to a slice/subslice that are valid for the 'subslice' class
> + * of multicast registers.  If another type of steering does not have any
> + * overlap in valid steering targets with 'subslice' style registers, we will
> + * need to explicitly re-steer reads of registers of the other type.
> + */
> +enum intel_steering_type {
> +	NUM_STEERING_TYPES
> +};
> +
>  enum intel_submission_method {
>  	INTEL_SUBMISSION_RING,
>  	INTEL_SUBMISSION_ELSP,
> @@ -145,6 +165,8 @@ struct intel_gt {
>  
>  	struct i915_vma *scratch;
>  
> +	const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
> +
>  	struct intel_gt_info {
>  		intel_engine_mask_t engine_mask;
>  		u8 num_engines;
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index b62d1e31a645..689045d3752b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1247,8 +1247,9 @@ wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
>  }
>  
>  static void
> -wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> +wa_list_apply(struct intel_gt *gt, const struct i915_wa_list *wal)
>  {
> +	struct intel_uncore *uncore = gt->uncore;
>  	enum forcewake_domains fw;
>  	unsigned long flags;
>  	struct i915_wa *wa;
> @@ -1263,13 +1264,16 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
>  	intel_uncore_forcewake_get__locked(uncore, fw);
>  
>  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> -		if (wa->clr)
> -			intel_uncore_rmw_fw(uncore, wa->reg, wa->clr, wa->set);
> -		else
> -			intel_uncore_write_fw(uncore, wa->reg, wa->set);
> +		u32 val, old = 0;
> +
> +		/* open-coded rmw due to steering */
> +		old = wa->clr ? intel_gt_read_register_fw(gt, wa->reg) : 0;
> +		val = (old & ~wa->clr) | wa->set;
> +		if (val != old || !wa->clr)
> +			intel_uncore_write_fw(uncore, wa->reg, val);
> +
>  		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> -			wa_verify(wa,
> -				  intel_uncore_read_fw(uncore, wa->reg),
> +			wa_verify(wa, intel_gt_read_register_fw(gt, wa->reg),
>  				  wal->name, "application");
>  	}
>  
> @@ -1279,10 +1283,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
>  
>  void intel_gt_apply_workarounds(struct intel_gt *gt)
>  {
> -	wa_list_apply(gt->uncore, &gt->i915->gt_wa_list);
> +	wa_list_apply(gt, &gt->i915->gt_wa_list);
>  }
>  
> -static bool wa_list_verify(struct intel_uncore *uncore,
> +static bool wa_list_verify(struct intel_gt *gt,
>  			   const struct i915_wa_list *wal,
>  			   const char *from)
>  {
> @@ -1292,7 +1296,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
>  
>  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
>  		ok &= wa_verify(wa,
> -				intel_uncore_read(uncore, wa->reg),
> +				intel_gt_read_register_fw(gt, wa->reg),
>  				wal->name, from);
>  
>  	return ok;
> @@ -1300,7 +1304,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
>  
>  bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
>  {
> -	return wa_list_verify(gt->uncore, &gt->i915->gt_wa_list, from);
> +	return wa_list_verify(gt, &gt->i915->gt_wa_list, from);
>  }
>  
>  __maybe_unused
> @@ -2081,7 +2085,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
>  
>  void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
>  {
> -	wa_list_apply(engine->uncore, &engine->wa_list);
> +	wa_list_apply(engine->gt, &engine->wa_list);
>  }
>  
>  struct mcr_range {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index c30754daf4b1..7ebc4edb8ecf 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -1147,7 +1147,7 @@ verify_wa_lists(struct intel_gt *gt, struct wa_lists *lists,
>  	enum intel_engine_id id;
>  	bool ok = true;
>  
> -	ok &= wa_list_verify(gt->uncore, &lists->gt_wa_list, str);
> +	ok &= wa_list_verify(gt, &lists->gt_wa_list, str);
>  
>  	for_each_engine(engine, gt, id) {
>  		struct intel_context *ce;
> -- 
> 2.25.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Add GT support for multiple types of multicast steering

[Rodrigo Vivi]

On Tue, Jun 15, 2021 at 05:08:20AM -0400, Rodrigo Vivi wrote:
> On Mon, Jun 14, 2021 at 08:34:32PM -0700, Matt Roper wrote:
> > Although most of our multicast registers are replicated per-subslice, we
> > also have a small number of multicast registers that are replicated
> > per-l3 bank instead.  For both types of multicast registers we need to
> > make sure we steer reads of these registers to a valid instance.
> > Ideally we'd like to find a specific instance ID that would steer reads
> > of either type of multicast register to a valid instance (i.e., not
> > fused off and not powered down), but sometimes the combination of
> > part-specific fusing and the additional restrictions imposed by Render
> > Power Gating make it impossible to find any overlap between the set of
> > valid subslices and valid l3 banks.  This problem will become even more
> > noticeable on our upcoming platforms since they will be adding
> > additional types of multicast registers with new types of replication
> > and rules for finding valid instances for reads.
> > 
> > To handle this we'll continue to pick a suitable subslice instance at
> > driver startup and program this as the default (sliceid,subsliceid)
> > setting in the steering control register (0xFDC).  In cases where we
> > need to read another type of multicast GT register, but the default
> > subslice steering would not correspond to a valid instance, we'll
> > explicitly re-steer the single read to a valid value, perform the read,
> > and then reset the steering to it's "subslice" default.
> > 
> > This patch adds the general functionality to prepare for this explicit
> > steering of other multicast register types.  We'll plug L3 bank steering
> > into this in the next patch, and then add additional types of multicast
> > registers when the support for our next upcoming platform arrives.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_gt.c            | 84 +++++++++++++++++++
> >  drivers/gpu/drm/i915/gt/intel_gt.h            |  8 ++
> >  drivers/gpu/drm/i915/gt/intel_gt_types.h      | 22 +++++
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 28 ++++---
> >  .../gpu/drm/i915/gt/selftest_workarounds.c    |  2 +-
> >  5 files changed, 131 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 2161bf01ef8b..f2bea1c20d56 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -697,6 +697,90 @@ void intel_gt_driver_late_release(struct intel_gt *gt)
> >  	intel_engines_free(gt);
> >  }
> >  
> > +/**
> > + * intel_gt_reg_needs_read_steering - determine whether a register read
> > + *     requires explicit steering
> > + * @gt: GT structure
> > + * @reg: the register to check steering requirements for
> > + * @type: type of multicast steering to check
> > + *
> > + * Determines whether @reg needs explicit steering of a specific type for
> > + * reads.
> > + *
> > + * Returns false if @reg does not belong to a register range of the given
> > + * steering type, or if the default (subslice-based) steering IDs are suitable
> > + * for @type steering too.
> > + */
> > +static bool intel_gt_reg_needs_read_steering(struct intel_gt *gt,
> > +					     i915_reg_t reg,
> > +					     enum intel_steering_type type)
> > +{
> > +	const u32 offset = i915_mmio_reg_offset(reg);
> > +	const struct intel_mmio_range *entry;
> > +
> > +	if (likely(!intel_gt_needs_read_steering(gt, type)))
> > +		return false;
> > +
> > +	for (entry = gt->steering_table[type]; entry->start < 0xFFFFFF; entry++) {
> 
> I'm not comfortable with this stop condition...
> we should know the right amount of entries that we have.
> Even if that means having another intermediate struct.
> 
> > +		if (offset >= entry->start && offset <= entry->end)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +/**
> > + * intel_gt_get_valid_steering - determines valid IDs for a class of MCR steering
> > + * @gt: GT structure
> > + * @type: multicast register type
> > + * @sliceid: Slice ID returned
> > + * @subsliceid: Subslice ID returned
> > + *
> > + * Determines sliceid and subsliceid values that will steer reads
> > + * of a specific multicast register class to a valid value.
> > + */
> > +static void intel_gt_get_valid_steering(struct intel_gt *gt,
> > +					enum intel_steering_type type,
> > +					u8 *sliceid, u8 *subsliceid)
> > +{
> > +	switch (type) {
> > +	default:
> > +		MISSING_CASE(type);
> 
> I understand that we are preparing the infra for the upcoming cases,
> but adding a missing_case warn by default doesn't look the right way...
> did CI not complain?!
oh, ofcourse not... I saw the next patch now.. please ignore this
comment...

but the others about the for stop condition remains..

> 
> > +		*sliceid = 0;
> > +		*subsliceid = 0;
> > +	}
> > +}
> > +
> > +/**
> > + * intel_gt_read_register_fw - reads a GT register with support for multicast
> > + * @gt: GT structure
> > + * @reg: register to read
> > + *
> > + * This function will read a GT register.  If the register is a multicast
> > + * register, the read will be steered to a valid instance (i.e., one that
> > + * isn't fused off or powered down by power gating).
> > + *
> > + * Returns the value from a valid instance of @reg.
> > + */
> > +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg)
> > +{
> > +	int type;
> > +	u8 sliceid, subsliceid;
> > +
> > +	for (type = 0; type < NUM_STEERING_TYPES; type++) {
> > +		if (intel_gt_reg_needs_read_steering(gt, reg, type)) {
> > +			intel_gt_get_valid_steering(gt, type, &sliceid,
> > +						    &subsliceid);
> > +			return intel_uncore_read_with_mcr_steering_fw(gt->uncore,
> > +								      reg,
> > +								      sliceid,
> > +								      subsliceid);
> > +		}
> > +	}
> > +
> > +	return intel_uncore_read_fw(gt->uncore, reg);
> > +}
> > +
> >  void intel_gt_info_print(const struct intel_gt_info *info,
> >  			 struct drm_printer *p)
> >  {
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
> > index 7ec395cace69..e7aabe0cc5bf 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> > @@ -75,6 +75,14 @@ static inline bool intel_gt_is_wedged(const struct intel_gt *gt)
> >  	return unlikely(test_bit(I915_WEDGED, &gt->reset.flags));
> >  }
> >  
> > +static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
> > +						enum intel_steering_type type)
> > +{
> > +	return gt->steering_table[type];
> 
> then if we know the right amount we also don't need this function right?!
> 
> > +}
> > +
> > +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
> > +
> >  void intel_gt_info_print(const struct intel_gt_info *info,
> >  			 struct drm_printer *p);
> >  
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index fecfacf551d5..47957837c8c0 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -31,6 +31,26 @@ struct i915_ggtt;
> >  struct intel_engine_cs;
> >  struct intel_uncore;
> >  
> > +struct intel_mmio_range {
> > +       u32 start;
> > +       u32 end;
> > +};
> > +
> > +/*
> > + * The hardware has multiple kinds of multicast register ranges that need
> > + * special register steering (and future platforms are expected to add
> > + * additional types).
> > + *
> > + * During driver startup, we initialize the steering control register to
> > + * direct reads to a slice/subslice that are valid for the 'subslice' class
> > + * of multicast registers.  If another type of steering does not have any
> > + * overlap in valid steering targets with 'subslice' style registers, we will
> > + * need to explicitly re-steer reads of registers of the other type.
> > + */
> > +enum intel_steering_type {
> > +	NUM_STEERING_TYPES
> > +};
> > +
> >  enum intel_submission_method {
> >  	INTEL_SUBMISSION_RING,
> >  	INTEL_SUBMISSION_ELSP,
> > @@ -145,6 +165,8 @@ struct intel_gt {
> >  
> >  	struct i915_vma *scratch;
> >  
> > +	const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
> > +
> >  	struct intel_gt_info {
> >  		intel_engine_mask_t engine_mask;
> >  		u8 num_engines;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index b62d1e31a645..689045d3752b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -1247,8 +1247,9 @@ wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> >  }
> >  
> >  static void
> > -wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > +wa_list_apply(struct intel_gt *gt, const struct i915_wa_list *wal)
> >  {
> > +	struct intel_uncore *uncore = gt->uncore;
> >  	enum forcewake_domains fw;
> >  	unsigned long flags;
> >  	struct i915_wa *wa;
> > @@ -1263,13 +1264,16 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> >  	intel_uncore_forcewake_get__locked(uncore, fw);
> >  
> >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> > -		if (wa->clr)
> > -			intel_uncore_rmw_fw(uncore, wa->reg, wa->clr, wa->set);
> > -		else
> > -			intel_uncore_write_fw(uncore, wa->reg, wa->set);
> > +		u32 val, old = 0;
> > +
> > +		/* open-coded rmw due to steering */
> > +		old = wa->clr ? intel_gt_read_register_fw(gt, wa->reg) : 0;
> > +		val = (old & ~wa->clr) | wa->set;
> > +		if (val != old || !wa->clr)
> > +			intel_uncore_write_fw(uncore, wa->reg, val);
> > +
> >  		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> > -			wa_verify(wa,
> > -				  intel_uncore_read_fw(uncore, wa->reg),
> > +			wa_verify(wa, intel_gt_read_register_fw(gt, wa->reg),
> >  				  wal->name, "application");
> >  	}
> >  
> > @@ -1279,10 +1283,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> >  
> >  void intel_gt_apply_workarounds(struct intel_gt *gt)
> >  {
> > -	wa_list_apply(gt->uncore, &gt->i915->gt_wa_list);
> > +	wa_list_apply(gt, &gt->i915->gt_wa_list);
> >  }
> >  
> > -static bool wa_list_verify(struct intel_uncore *uncore,
> > +static bool wa_list_verify(struct intel_gt *gt,
> >  			   const struct i915_wa_list *wal,
> >  			   const char *from)
> >  {
> > @@ -1292,7 +1296,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> >  
> >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> >  		ok &= wa_verify(wa,
> > -				intel_uncore_read(uncore, wa->reg),
> > +				intel_gt_read_register_fw(gt, wa->reg),
> >  				wal->name, from);
> >  
> >  	return ok;
> > @@ -1300,7 +1304,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> >  
> >  bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
> >  {
> > -	return wa_list_verify(gt->uncore, &gt->i915->gt_wa_list, from);
> > +	return wa_list_verify(gt, &gt->i915->gt_wa_list, from);
> >  }
> >  
> >  __maybe_unused
> > @@ -2081,7 +2085,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> >  
> >  void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> >  {
> > -	wa_list_apply(engine->uncore, &engine->wa_list);
> > +	wa_list_apply(engine->gt, &engine->wa_list);
> >  }
> >  
> >  struct mcr_range {
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > index c30754daf4b1..7ebc4edb8ecf 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > @@ -1147,7 +1147,7 @@ verify_wa_lists(struct intel_gt *gt, struct wa_lists *lists,
> >  	enum intel_engine_id id;
> >  	bool ok = true;
> >  
> > -	ok &= wa_list_verify(gt->uncore, &lists->gt_wa_list, str);
> > +	ok &= wa_list_verify(gt, &lists->gt_wa_list, str);
> >  
> >  	for_each_engine(engine, gt, id) {
> >  		struct intel_context *ce;
> > -- 
> > 2.25.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Add support for explicit L3BANK steering

[Tvrtko Ursulin]

On 15/06/2021 04:34, Matt Roper wrote:
> Because Render Power Gating restricts us to just a single subslice as a
> valid steering target for reads of multicast registers in a SUBSLICE
> range, the default steering we setup at init may not lead to a suitable
> target for L3BANK multicast register.  In cases where it does not, use
> explicit runtime steering whenever an L3BANK multicast register is read.
> 
> While we're at it, let's simplify the function a little bit and drop its
> support for gen10/CNL since no such platforms ever materialized for real
> use.  Multicast register steering is already an area that causes enough
> confusion; no need to complicate it with what's effectively dead code.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt.c          | 18 +++++
>   drivers/gpu/drm/i915/gt/intel_gt_types.h    |  4 +
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 84 ++++++---------------
>   3 files changed, 46 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index f2bea1c20d56..2c9cc34b0cbd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -83,6 +83,11 @@ void intel_gt_init_hw_early(struct intel_gt *gt, struct i915_ggtt *ggtt)
>   	gt->ggtt = ggtt;
>   }
>   
> +static const struct intel_mmio_range icl_l3bank_steering_table[] = {
> +       { 0x00B100, 0x00B3FF },
> +       { 0xFFFFFF, 0xFFFFFF }, /* terminating entry */
> +};
> +
>   int intel_gt_init_mmio(struct intel_gt *gt)
>   {
>   	intel_gt_init_clock_frequency(gt);
> @@ -90,6 +95,13 @@ int intel_gt_init_mmio(struct intel_gt *gt)
>   	intel_uc_init_mmio(&gt->uc);
>   	intel_sseu_info_init(gt);
>   
> +	if (GRAPHICS_VER(gt->i915) >= 11) {
> +		gt->steering_table[L3BANK] = icl_l3bank_steering_table;
> +		gt->info.l3bank_mask =
> +			intel_uncore_read(&gt->i915->uncore, GEN10_MIRROR_FUSE3) &

gt->uncore, unless there's a special reason not to.

Regards,

Tvrtko

> +			GEN10_L3BANK_MASK;
> +	}
> +
>   	return intel_engines_init_mmio(gt);
>   }
>   
> @@ -744,6 +756,12 @@ static void intel_gt_get_valid_steering(struct intel_gt *gt,
>   					u8 *sliceid, u8 *subsliceid)
>   {
>   	switch (type) {
> +	case L3BANK:
> +		GEM_DEBUG_WARN_ON(!gt->info.l3bank_mask); /* should be impossible! */
> +
> +		*sliceid = __ffs(gt->info.l3bank_mask);
> +		*subsliceid = 0;        /* unused */
> +		break;
>   	default:
>   		MISSING_CASE(type);
>   		*sliceid = 0;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index 47957837c8c0..5ecad25de6ed 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -48,6 +48,8 @@ struct intel_mmio_range {
>    * need to explicitly re-steer reads of registers of the other type.
>    */
>   enum intel_steering_type {
> +	L3BANK,
> +
>   	NUM_STEERING_TYPES
>   };
>   
> @@ -174,6 +176,8 @@ struct intel_gt {
>   		/* Media engine access to SFC per instance */
>   		u8 vdbox_sfc_access;
>   
> +		u32 l3bank_mask;
> +
>   		/* Slice/subslice/EU info */
>   		struct sseu_dev_info sseu;
>   	} info;
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 689045d3752b..a0be3c09a7f9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -944,71 +944,37 @@ cfl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>   }
>   
>   static void
> -wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
> +icl_wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
>   {
>   	const struct sseu_dev_info *sseu = &i915->gt.info.sseu;
>   	unsigned int slice, subslice;
> -	u32 l3_en, mcr, mcr_mask;
> +	u32 mcr, mcr_mask;
>   
> -	GEM_BUG_ON(GRAPHICS_VER(i915) < 10);
> +	GEM_BUG_ON(GRAPHICS_VER(i915) < 11);
> +	GEM_BUG_ON(hweight8(sseu->slice_mask) > 1);
> +	slice = 0;
>   
>   	/*
> -	 * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
> -	 * L3Banks could be fused off in single slice scenario. If that is
> -	 * the case, we might need to program MCR select to a valid L3Bank
> -	 * by default, to make sure we correctly read certain registers
> -	 * later on (in the range 0xB100 - 0xB3FF).
> -	 *
> -	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl
> -	 * Before any MMIO read into slice/subslice specific registers, MCR
> -	 * packet control register needs to be programmed to point to any
> -	 * enabled s/ss pair. Otherwise, incorrect values will be returned.
> -	 * This means each subsequent MMIO read will be forwarded to an
> -	 * specific s/ss combination, but this is OK since these registers
> -	 * are consistent across s/ss in almost all cases. In the rare
> -	 * occasions, such as INSTDONE, where this value is dependent
> -	 * on s/ss combo, the read should be done with read_subslice_reg.
> -	 *
> -	 * Since GEN8_MCR_SELECTOR contains dual-purpose bits which select both
> -	 * to which subslice, or to which L3 bank, the respective mmio reads
> -	 * will go, we have to find a common index which works for both
> -	 * accesses.
> -	 *
> -	 * Case where we cannot find a common index fortunately should not
> -	 * happen in production hardware, so we only emit a warning instead of
> -	 * implementing something more complex that requires checking the range
> -	 * of every MMIO read.
> +	 * Although a platform may have subslices, we need to always steer
> +	 * reads to the lowest instance that isn't fused off.  When Render
> +	 * Power Gating is enabled, grabbing forcewake will only power up a
> +	 * single subslice (the "minconfig") if there isn't a real workload
> +	 * that needs to be run; this means that if we steer register reads to
> +	 * one of the higher subslices, we run the risk of reading back 0's or
> +	 * random garbage.
>   	 */
> +	subslice = __ffs(intel_sseu_get_subslices(sseu, slice));
>   
> -	if (GRAPHICS_VER(i915) >= 10 && is_power_of_2(sseu->slice_mask)) {
> -		u32 l3_fuse =
> -			intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3) &
> -			GEN10_L3BANK_MASK;
> -
> -		drm_dbg(&i915->drm, "L3 fuse = %x\n", l3_fuse);
> -		l3_en = ~(l3_fuse << GEN10_L3BANK_PAIR_COUNT | l3_fuse);
> -	} else {
> -		l3_en = ~0;
> -	}
> +	/*
> +	 * If the subslice we picked above also steers us to a valid L3 bank,
> +	 * then we can just rely on the default steering and won't need to
> +	 * worry about explicitly re-steering L3BANK reads later.
> +	 */
> +	if (i915->gt.info.l3bank_mask & BIT(subslice))
> +		i915->gt.steering_table[L3BANK] = NULL;
>   
> -	slice = fls(sseu->slice_mask) - 1;
> -	subslice = fls(l3_en & intel_sseu_get_subslices(sseu, slice));
> -	if (!subslice) {
> -		drm_warn(&i915->drm,
> -			 "No common index found between subslice mask %x and L3 bank mask %x!\n",
> -			 intel_sseu_get_subslices(sseu, slice), l3_en);
> -		subslice = fls(l3_en);
> -		drm_WARN_ON(&i915->drm, !subslice);
> -	}
> -	subslice--;
> -
> -	if (GRAPHICS_VER(i915) >= 11) {
> -		mcr = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
> -		mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
> -	} else {
> -		mcr = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
> -		mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
> -	}
> +	mcr = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
> +	mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
>   
>   	drm_dbg(&i915->drm, "MCR slice/subslice = %x\n", mcr);
>   
> @@ -1018,8 +984,6 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
>   static void
>   cnl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>   {
> -	wa_init_mcr(i915, wal);
> -
>   	/* WaInPlaceDecompressionHang:cnl */
>   	wa_write_or(wal,
>   		    GEN9_GAMT_ECO_REG_RW_IA,
> @@ -1029,7 +993,7 @@ cnl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>   static void
>   icl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>   {
> -	wa_init_mcr(i915, wal);
> +	icl_wa_init_mcr(i915, wal);
>   
>   	/* WaInPlaceDecompressionHang:icl */
>   	wa_write_or(wal,
> @@ -1111,7 +1075,7 @@ static void
>   gen12_gt_workarounds_init(struct drm_i915_private *i915,
>   			  struct i915_wa_list *wal)
>   {
> -	wa_init_mcr(i915, wal);
> +	icl_wa_init_mcr(i915, wal);
>   
>   	/* Wa_14011060649:tgl,rkl,dg1,adls */
>   	wa_14011060649(i915, wal);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Add GT support for multiple types of multicast steering

[Matt Roper]

On Tue, Jun 15, 2021 at 05:11:04AM -0400, Rodrigo Vivi wrote:
> On Tue, Jun 15, 2021 at 05:08:20AM -0400, Rodrigo Vivi wrote:
> > On Mon, Jun 14, 2021 at 08:34:32PM -0700, Matt Roper wrote:
> > > Although most of our multicast registers are replicated per-subslice, we
> > > also have a small number of multicast registers that are replicated
> > > per-l3 bank instead.  For both types of multicast registers we need to
> > > make sure we steer reads of these registers to a valid instance.
> > > Ideally we'd like to find a specific instance ID that would steer reads
> > > of either type of multicast register to a valid instance (i.e., not
> > > fused off and not powered down), but sometimes the combination of
> > > part-specific fusing and the additional restrictions imposed by Render
> > > Power Gating make it impossible to find any overlap between the set of
> > > valid subslices and valid l3 banks.  This problem will become even more
> > > noticeable on our upcoming platforms since they will be adding
> > > additional types of multicast registers with new types of replication
> > > and rules for finding valid instances for reads.
> > > 
> > > To handle this we'll continue to pick a suitable subslice instance at
> > > driver startup and program this as the default (sliceid,subsliceid)
> > > setting in the steering control register (0xFDC).  In cases where we
> > > need to read another type of multicast GT register, but the default
> > > subslice steering would not correspond to a valid instance, we'll
> > > explicitly re-steer the single read to a valid value, perform the read,
> > > and then reset the steering to it's "subslice" default.
> > > 
> > > This patch adds the general functionality to prepare for this explicit
> > > steering of other multicast register types.  We'll plug L3 bank steering
> > > into this in the next patch, and then add additional types of multicast
> > > registers when the support for our next upcoming platform arrives.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_gt.c            | 84 +++++++++++++++++++
> > >  drivers/gpu/drm/i915/gt/intel_gt.h            |  8 ++
> > >  drivers/gpu/drm/i915/gt/intel_gt_types.h      | 22 +++++
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 28 ++++---
> > >  .../gpu/drm/i915/gt/selftest_workarounds.c    |  2 +-
> > >  5 files changed, 131 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > index 2161bf01ef8b..f2bea1c20d56 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > @@ -697,6 +697,90 @@ void intel_gt_driver_late_release(struct intel_gt *gt)
> > >  	intel_engines_free(gt);
> > >  }
> > >  
> > > +/**
> > > + * intel_gt_reg_needs_read_steering - determine whether a register read
> > > + *     requires explicit steering
> > > + * @gt: GT structure
> > > + * @reg: the register to check steering requirements for
> > > + * @type: type of multicast steering to check
> > > + *
> > > + * Determines whether @reg needs explicit steering of a specific type for
> > > + * reads.
> > > + *
> > > + * Returns false if @reg does not belong to a register range of the given
> > > + * steering type, or if the default (subslice-based) steering IDs are suitable
> > > + * for @type steering too.
> > > + */
> > > +static bool intel_gt_reg_needs_read_steering(struct intel_gt *gt,
> > > +					     i915_reg_t reg,
> > > +					     enum intel_steering_type type)
> > > +{
> > > +	const u32 offset = i915_mmio_reg_offset(reg);
> > > +	const struct intel_mmio_range *entry;
> > > +
> > > +	if (likely(!intel_gt_needs_read_steering(gt, type)))
> > > +		return false;
> > > +
> > > +	for (entry = gt->steering_table[type]; entry->start < 0xFFFFFF; entry++) {
> > 
> > I'm not comfortable with this stop condition...

Is your worry that we'll someday have registers going more than 16MB
into the MMIO BAR?  Or just that we use a terminator entry in general?
We have lots of other places in the driver where we use this pattern
already (cdclk tables, dbuf tables, etc.).  We're soon going to need to
deal with lots of different MCR range tables (which will vary according
to platform too), so we don't want to be trying to hardcode table sizes
here.

If your concern is just that our MMIO space may eventually become big
enough that 0xFFFFFF becomes a valid register, I could switch the
condition to looking for 'entry->end == 0' and just make the terminator
row a '{}' since an upper bound of 0 will always be invalid.


...
> > > +static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
> > > +						enum intel_steering_type type)
> > > +{
> > > +	return gt->steering_table[type];
> > 
> > then if we know the right amount we also don't need this function right?!

No, this function is critical.  We still attempt to find an implicit
steering value that will work for all steering types.  If we succeed and
explicit steering isn't needed, then we set the table pointer to NULL to
disable the explicit steering for that type (see patch 3 for where this
happens for l3bank).  But we don't know until runtime if/when that will
be possible since it all depends on the fuse values for the specific
system that you're running on.


Matt

> > 
> > > +}
> > > +
> > > +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
> > > +
> > >  void intel_gt_info_print(const struct intel_gt_info *info,
> > >  			 struct drm_printer *p);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > index fecfacf551d5..47957837c8c0 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > @@ -31,6 +31,26 @@ struct i915_ggtt;
> > >  struct intel_engine_cs;
> > >  struct intel_uncore;
> > >  
> > > +struct intel_mmio_range {
> > > +       u32 start;
> > > +       u32 end;
> > > +};
> > > +
> > > +/*
> > > + * The hardware has multiple kinds of multicast register ranges that need
> > > + * special register steering (and future platforms are expected to add
> > > + * additional types).
> > > + *
> > > + * During driver startup, we initialize the steering control register to
> > > + * direct reads to a slice/subslice that are valid for the 'subslice' class
> > > + * of multicast registers.  If another type of steering does not have any
> > > + * overlap in valid steering targets with 'subslice' style registers, we will
> > > + * need to explicitly re-steer reads of registers of the other type.
> > > + */
> > > +enum intel_steering_type {
> > > +	NUM_STEERING_TYPES
> > > +};
> > > +
> > >  enum intel_submission_method {
> > >  	INTEL_SUBMISSION_RING,
> > >  	INTEL_SUBMISSION_ELSP,
> > > @@ -145,6 +165,8 @@ struct intel_gt {
> > >  
> > >  	struct i915_vma *scratch;
> > >  
> > > +	const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
> > > +
> > >  	struct intel_gt_info {
> > >  		intel_engine_mask_t engine_mask;
> > >  		u8 num_engines;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index b62d1e31a645..689045d3752b 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -1247,8 +1247,9 @@ wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> > >  }
> > >  
> > >  static void
> > > -wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > > +wa_list_apply(struct intel_gt *gt, const struct i915_wa_list *wal)
> > >  {
> > > +	struct intel_uncore *uncore = gt->uncore;
> > >  	enum forcewake_domains fw;
> > >  	unsigned long flags;
> > >  	struct i915_wa *wa;
> > > @@ -1263,13 +1264,16 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > >  	intel_uncore_forcewake_get__locked(uncore, fw);
> > >  
> > >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> > > -		if (wa->clr)
> > > -			intel_uncore_rmw_fw(uncore, wa->reg, wa->clr, wa->set);
> > > -		else
> > > -			intel_uncore_write_fw(uncore, wa->reg, wa->set);
> > > +		u32 val, old = 0;
> > > +
> > > +		/* open-coded rmw due to steering */
> > > +		old = wa->clr ? intel_gt_read_register_fw(gt, wa->reg) : 0;
> > > +		val = (old & ~wa->clr) | wa->set;
> > > +		if (val != old || !wa->clr)
> > > +			intel_uncore_write_fw(uncore, wa->reg, val);
> > > +
> > >  		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> > > -			wa_verify(wa,
> > > -				  intel_uncore_read_fw(uncore, wa->reg),
> > > +			wa_verify(wa, intel_gt_read_register_fw(gt, wa->reg),
> > >  				  wal->name, "application");
> > >  	}
> > >  
> > > @@ -1279,10 +1283,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > >  
> > >  void intel_gt_apply_workarounds(struct intel_gt *gt)
> > >  {
> > > -	wa_list_apply(gt->uncore, &gt->i915->gt_wa_list);
> > > +	wa_list_apply(gt, &gt->i915->gt_wa_list);
> > >  }
> > >  
> > > -static bool wa_list_verify(struct intel_uncore *uncore,
> > > +static bool wa_list_verify(struct intel_gt *gt,
> > >  			   const struct i915_wa_list *wal,
> > >  			   const char *from)
> > >  {
> > > @@ -1292,7 +1296,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> > >  
> > >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> > >  		ok &= wa_verify(wa,
> > > -				intel_uncore_read(uncore, wa->reg),
> > > +				intel_gt_read_register_fw(gt, wa->reg),
> > >  				wal->name, from);
> > >  
> > >  	return ok;
> > > @@ -1300,7 +1304,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> > >  
> > >  bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
> > >  {
> > > -	return wa_list_verify(gt->uncore, &gt->i915->gt_wa_list, from);
> > > +	return wa_list_verify(gt, &gt->i915->gt_wa_list, from);
> > >  }
> > >  
> > >  __maybe_unused
> > > @@ -2081,7 +2085,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> > >  
> > >  void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> > >  {
> > > -	wa_list_apply(engine->uncore, &engine->wa_list);
> > > +	wa_list_apply(engine->gt, &engine->wa_list);
> > >  }
> > >  
> > >  struct mcr_range {
> > > diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > index c30754daf4b1..7ebc4edb8ecf 100644
> > > --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > @@ -1147,7 +1147,7 @@ verify_wa_lists(struct intel_gt *gt, struct wa_lists *lists,
> > >  	enum intel_engine_id id;
> > >  	bool ok = true;
> > >  
> > > -	ok &= wa_list_verify(gt->uncore, &lists->gt_wa_list, str);
> > > +	ok &= wa_list_verify(gt, &lists->gt_wa_list, str);
> > >  
> > >  	for_each_engine(engine, gt, id) {
> > >  		struct intel_context *ce;
> > > -- 
> > > 2.25.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Add GT support for multiple types of multicast steering

[Rodrigo Vivi]

On Tue, Jun 15, 2021 at 08:30:23AM -0700, Matt Roper wrote:
> On Tue, Jun 15, 2021 at 05:11:04AM -0400, Rodrigo Vivi wrote:
> > On Tue, Jun 15, 2021 at 05:08:20AM -0400, Rodrigo Vivi wrote:
> > > On Mon, Jun 14, 2021 at 08:34:32PM -0700, Matt Roper wrote:
> > > > Although most of our multicast registers are replicated per-subslice, we
> > > > also have a small number of multicast registers that are replicated
> > > > per-l3 bank instead.  For both types of multicast registers we need to
> > > > make sure we steer reads of these registers to a valid instance.
> > > > Ideally we'd like to find a specific instance ID that would steer reads
> > > > of either type of multicast register to a valid instance (i.e., not
> > > > fused off and not powered down), but sometimes the combination of
> > > > part-specific fusing and the additional restrictions imposed by Render
> > > > Power Gating make it impossible to find any overlap between the set of
> > > > valid subslices and valid l3 banks.  This problem will become even more
> > > > noticeable on our upcoming platforms since they will be adding
> > > > additional types of multicast registers with new types of replication
> > > > and rules for finding valid instances for reads.
> > > > 
> > > > To handle this we'll continue to pick a suitable subslice instance at
> > > > driver startup and program this as the default (sliceid,subsliceid)
> > > > setting in the steering control register (0xFDC).  In cases where we
> > > > need to read another type of multicast GT register, but the default
> > > > subslice steering would not correspond to a valid instance, we'll
> > > > explicitly re-steer the single read to a valid value, perform the read,
> > > > and then reset the steering to it's "subslice" default.
> > > > 
> > > > This patch adds the general functionality to prepare for this explicit
> > > > steering of other multicast register types.  We'll plug L3 bank steering
> > > > into this in the next patch, and then add additional types of multicast
> > > > registers when the support for our next upcoming platform arrives.
> > > > 
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gt/intel_gt.c            | 84 +++++++++++++++++++
> > > >  drivers/gpu/drm/i915/gt/intel_gt.h            |  8 ++
> > > >  drivers/gpu/drm/i915/gt/intel_gt_types.h      | 22 +++++
> > > >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 28 ++++---
> > > >  .../gpu/drm/i915/gt/selftest_workarounds.c    |  2 +-
> > > >  5 files changed, 131 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > index 2161bf01ef8b..f2bea1c20d56 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > @@ -697,6 +697,90 @@ void intel_gt_driver_late_release(struct intel_gt *gt)
> > > >  	intel_engines_free(gt);
> > > >  }
> > > >  
> > > > +/**
> > > > + * intel_gt_reg_needs_read_steering - determine whether a register read
> > > > + *     requires explicit steering
> > > > + * @gt: GT structure
> > > > + * @reg: the register to check steering requirements for
> > > > + * @type: type of multicast steering to check
> > > > + *
> > > > + * Determines whether @reg needs explicit steering of a specific type for
> > > > + * reads.
> > > > + *
> > > > + * Returns false if @reg does not belong to a register range of the given
> > > > + * steering type, or if the default (subslice-based) steering IDs are suitable
> > > > + * for @type steering too.
> > > > + */
> > > > +static bool intel_gt_reg_needs_read_steering(struct intel_gt *gt,
> > > > +					     i915_reg_t reg,
> > > > +					     enum intel_steering_type type)
> > > > +{
> > > > +	const u32 offset = i915_mmio_reg_offset(reg);
> > > > +	const struct intel_mmio_range *entry;
> > > > +
> > > > +	if (likely(!intel_gt_needs_read_steering(gt, type)))
> > > > +		return false;
> > > > +
> > > > +	for (entry = gt->steering_table[type]; entry->start < 0xFFFFFF; entry++) {
> > > 
> > > I'm not comfortable with this stop condition...
> 
> Is your worry that we'll someday have registers going more than 16MB
> into the MMIO BAR?

no no, if this is the case we have bigger problems ;)

> Or just that we use a terminator entry in general?
> We have lots of other places in the driver where we use this pattern
> already (cdclk tables, dbuf tables, etc.).  We're soon going to need to
> deal with lots of different MCR range tables (which will vary according
> to platform too), so we don't want to be trying to hardcode table sizes
> here.
> 
> If your concern is just that our MMIO space may eventually become big
> enough that 0xFFFFFF becomes a valid register, I could switch the
> condition to looking for 'entry->end == 0' and just make the terminator
> row a '{}' since an upper bound of 0 will always be invalid.

okay, what about go like cdclk terminator then and use only {},
then iterate with

for (entry = gt->steering_table[type]; entry->start; entry++) {

But well, looking at patch 3 it is indeed minor thing...

> 
> 
> ...
> > > > +static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
> > > > +						enum intel_steering_type type)
> > > > +{
> > > > +	return gt->steering_table[type];
> > > 
> > > then if we know the right amount we also don't need this function right?!
> 
> No, this function is critical.  We still attempt to find an implicit
> steering value that will work for all steering types.  If we succeed and
> explicit steering isn't needed, then we set the table pointer to NULL to
> disable the explicit steering for that type (see patch 3 for where this
> happens for l3bank).  But we don't know until runtime if/when that will
> be possible since it all depends on the fuse values for the specific
> system that you're running on.

hmm... got it...

So, my only remaining concern with this patch is if it is bisectable...
if the checks without the terminators won't blow up any auto bisect
case...

> 
> 
> Matt
> 
> > > 
> > > > +}
> > > > +
> > > > +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
> > > > +
> > > >  void intel_gt_info_print(const struct intel_gt_info *info,
> > > >  			 struct drm_printer *p);
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > index fecfacf551d5..47957837c8c0 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > @@ -31,6 +31,26 @@ struct i915_ggtt;
> > > >  struct intel_engine_cs;
> > > >  struct intel_uncore;
> > > >  
> > > > +struct intel_mmio_range {
> > > > +       u32 start;
> > > > +       u32 end;
> > > > +};
> > > > +
> > > > +/*
> > > > + * The hardware has multiple kinds of multicast register ranges that need
> > > > + * special register steering (and future platforms are expected to add
> > > > + * additional types).
> > > > + *
> > > > + * During driver startup, we initialize the steering control register to
> > > > + * direct reads to a slice/subslice that are valid for the 'subslice' class
> > > > + * of multicast registers.  If another type of steering does not have any
> > > > + * overlap in valid steering targets with 'subslice' style registers, we will
> > > > + * need to explicitly re-steer reads of registers of the other type.
> > > > + */
> > > > +enum intel_steering_type {
> > > > +	NUM_STEERING_TYPES
> > > > +};
> > > > +
> > > >  enum intel_submission_method {
> > > >  	INTEL_SUBMISSION_RING,
> > > >  	INTEL_SUBMISSION_ELSP,
> > > > @@ -145,6 +165,8 @@ struct intel_gt {
> > > >  
> > > >  	struct i915_vma *scratch;
> > > >  
> > > > +	const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
> > > > +
> > > >  	struct intel_gt_info {
> > > >  		intel_engine_mask_t engine_mask;
> > > >  		u8 num_engines;
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > index b62d1e31a645..689045d3752b 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > @@ -1247,8 +1247,9 @@ wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> > > >  }
> > > >  
> > > >  static void
> > > > -wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > > > +wa_list_apply(struct intel_gt *gt, const struct i915_wa_list *wal)
> > > >  {
> > > > +	struct intel_uncore *uncore = gt->uncore;
> > > >  	enum forcewake_domains fw;
> > > >  	unsigned long flags;
> > > >  	struct i915_wa *wa;
> > > > @@ -1263,13 +1264,16 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > > >  	intel_uncore_forcewake_get__locked(uncore, fw);
> > > >  
> > > >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> > > > -		if (wa->clr)
> > > > -			intel_uncore_rmw_fw(uncore, wa->reg, wa->clr, wa->set);
> > > > -		else
> > > > -			intel_uncore_write_fw(uncore, wa->reg, wa->set);
> > > > +		u32 val, old = 0;
> > > > +
> > > > +		/* open-coded rmw due to steering */
> > > > +		old = wa->clr ? intel_gt_read_register_fw(gt, wa->reg) : 0;
> > > > +		val = (old & ~wa->clr) | wa->set;
> > > > +		if (val != old || !wa->clr)
> > > > +			intel_uncore_write_fw(uncore, wa->reg, val);
> > > > +
> > > >  		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> > > > -			wa_verify(wa,
> > > > -				  intel_uncore_read_fw(uncore, wa->reg),
> > > > +			wa_verify(wa, intel_gt_read_register_fw(gt, wa->reg),
> > > >  				  wal->name, "application");
> > > >  	}
> > > >  
> > > > @@ -1279,10 +1283,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > > >  
> > > >  void intel_gt_apply_workarounds(struct intel_gt *gt)
> > > >  {
> > > > -	wa_list_apply(gt->uncore, &gt->i915->gt_wa_list);
> > > > +	wa_list_apply(gt, &gt->i915->gt_wa_list);
> > > >  }
> > > >  
> > > > -static bool wa_list_verify(struct intel_uncore *uncore,
> > > > +static bool wa_list_verify(struct intel_gt *gt,
> > > >  			   const struct i915_wa_list *wal,
> > > >  			   const char *from)
> > > >  {
> > > > @@ -1292,7 +1296,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> > > >  
> > > >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> > > >  		ok &= wa_verify(wa,
> > > > -				intel_uncore_read(uncore, wa->reg),
> > > > +				intel_gt_read_register_fw(gt, wa->reg),
> > > >  				wal->name, from);
> > > >  
> > > >  	return ok;
> > > > @@ -1300,7 +1304,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> > > >  
> > > >  bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
> > > >  {
> > > > -	return wa_list_verify(gt->uncore, &gt->i915->gt_wa_list, from);
> > > > +	return wa_list_verify(gt, &gt->i915->gt_wa_list, from);
> > > >  }
> > > >  
> > > >  __maybe_unused
> > > > @@ -2081,7 +2085,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> > > >  
> > > >  void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> > > >  {
> > > > -	wa_list_apply(engine->uncore, &engine->wa_list);
> > > > +	wa_list_apply(engine->gt, &engine->wa_list);
> > > >  }
> > > >  
> > > >  struct mcr_range {
> > > > diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > > index c30754daf4b1..7ebc4edb8ecf 100644
> > > > --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > > +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > > @@ -1147,7 +1147,7 @@ verify_wa_lists(struct intel_gt *gt, struct wa_lists *lists,
> > > >  	enum intel_engine_id id;
> > > >  	bool ok = true;
> > > >  
> > > > -	ok &= wa_list_verify(gt->uncore, &lists->gt_wa_list, str);
> > > > +	ok &= wa_list_verify(gt, &lists->gt_wa_list, str);
> > > >  
> > > >  	for_each_engine(engine, gt, id) {
> > > >  		struct intel_context *ce;
> > > > -- 
> > > > 2.25.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Add GT support for multiple types of multicast steering

[Matt Roper]

On Tue, Jun 15, 2021 at 03:48:32PM -0400, Rodrigo Vivi wrote:
> On Tue, Jun 15, 2021 at 08:30:23AM -0700, Matt Roper wrote:
> > On Tue, Jun 15, 2021 at 05:11:04AM -0400, Rodrigo Vivi wrote:
> > > On Tue, Jun 15, 2021 at 05:08:20AM -0400, Rodrigo Vivi wrote:
> > > > On Mon, Jun 14, 2021 at 08:34:32PM -0700, Matt Roper wrote:
> > > > > Although most of our multicast registers are replicated per-subslice, we
> > > > > also have a small number of multicast registers that are replicated
> > > > > per-l3 bank instead.  For both types of multicast registers we need to
> > > > > make sure we steer reads of these registers to a valid instance.
> > > > > Ideally we'd like to find a specific instance ID that would steer reads
> > > > > of either type of multicast register to a valid instance (i.e., not
> > > > > fused off and not powered down), but sometimes the combination of
> > > > > part-specific fusing and the additional restrictions imposed by Render
> > > > > Power Gating make it impossible to find any overlap between the set of
> > > > > valid subslices and valid l3 banks.  This problem will become even more
> > > > > noticeable on our upcoming platforms since they will be adding
> > > > > additional types of multicast registers with new types of replication
> > > > > and rules for finding valid instances for reads.
> > > > > 
> > > > > To handle this we'll continue to pick a suitable subslice instance at
> > > > > driver startup and program this as the default (sliceid,subsliceid)
> > > > > setting in the steering control register (0xFDC).  In cases where we
> > > > > need to read another type of multicast GT register, but the default
> > > > > subslice steering would not correspond to a valid instance, we'll
> > > > > explicitly re-steer the single read to a valid value, perform the read,
> > > > > and then reset the steering to it's "subslice" default.
> > > > > 
> > > > > This patch adds the general functionality to prepare for this explicit
> > > > > steering of other multicast register types.  We'll plug L3 bank steering
> > > > > into this in the next patch, and then add additional types of multicast
> > > > > registers when the support for our next upcoming platform arrives.
> > > > > 
> > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/gt/intel_gt.c            | 84 +++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/gt/intel_gt.h            |  8 ++
> > > > >  drivers/gpu/drm/i915/gt/intel_gt_types.h      | 22 +++++
> > > > >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 28 ++++---
> > > > >  .../gpu/drm/i915/gt/selftest_workarounds.c    |  2 +-
> > > > >  5 files changed, 131 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > > index 2161bf01ef8b..f2bea1c20d56 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > > @@ -697,6 +697,90 @@ void intel_gt_driver_late_release(struct intel_gt *gt)
> > > > >  	intel_engines_free(gt);
> > > > >  }
> > > > >  
> > > > > +/**
> > > > > + * intel_gt_reg_needs_read_steering - determine whether a register read
> > > > > + *     requires explicit steering
> > > > > + * @gt: GT structure
> > > > > + * @reg: the register to check steering requirements for
> > > > > + * @type: type of multicast steering to check
> > > > > + *
> > > > > + * Determines whether @reg needs explicit steering of a specific type for
> > > > > + * reads.
> > > > > + *
> > > > > + * Returns false if @reg does not belong to a register range of the given
> > > > > + * steering type, or if the default (subslice-based) steering IDs are suitable
> > > > > + * for @type steering too.
> > > > > + */
> > > > > +static bool intel_gt_reg_needs_read_steering(struct intel_gt *gt,
> > > > > +					     i915_reg_t reg,
> > > > > +					     enum intel_steering_type type)
> > > > > +{
> > > > > +	const u32 offset = i915_mmio_reg_offset(reg);
> > > > > +	const struct intel_mmio_range *entry;
> > > > > +
> > > > > +	if (likely(!intel_gt_needs_read_steering(gt, type)))
> > > > > +		return false;
> > > > > +
> > > > > +	for (entry = gt->steering_table[type]; entry->start < 0xFFFFFF; entry++) {
> > > > 
> > > > I'm not comfortable with this stop condition...
> > 
> > Is your worry that we'll someday have registers going more than 16MB
> > into the MMIO BAR?
> 
> no no, if this is the case we have bigger problems ;)
> 
> > Or just that we use a terminator entry in general?
> > We have lots of other places in the driver where we use this pattern
> > already (cdclk tables, dbuf tables, etc.).  We're soon going to need to
> > deal with lots of different MCR range tables (which will vary according
> > to platform too), so we don't want to be trying to hardcode table sizes
> > here.
> > 
> > If your concern is just that our MMIO space may eventually become big
> > enough that 0xFFFFFF becomes a valid register, I could switch the
> > condition to looking for 'entry->end == 0' and just make the terminator
> > row a '{}' since an upper bound of 0 will always be invalid.
> 
> okay, what about go like cdclk terminator then and use only {},
> then iterate with
> 
> for (entry = gt->steering_table[type]; entry->start; entry++) {

Yeah.  I'll make this change.  Although I'll make the test for
entry->end instead of entry->start since it's more obvious that that
can't happen for a real register range.

> 
> But well, looking at patch 3 it is indeed minor thing...
> 
> > 
> > 
> > ...
> > > > > +static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
> > > > > +						enum intel_steering_type type)
> > > > > +{
> > > > > +	return gt->steering_table[type];
> > > > 
> > > > then if we know the right amount we also don't need this function right?!
> > 
> > No, this function is critical.  We still attempt to find an implicit
> > steering value that will work for all steering types.  If we succeed and
> > explicit steering isn't needed, then we set the table pointer to NULL to
> > disable the explicit steering for that type (see patch 3 for where this
> > happens for l3bank).  But we don't know until runtime if/when that will
> > be possible since it all depends on the fuse values for the specific
> > system that you're running on.
> 
> hmm... got it...
> 
> So, my only remaining concern with this patch is if it is bisectable...
> if the checks without the terminators won't blow up any auto bisect
> case...

I think it should be bisectable.  After patch 2, we have zero explicit
steering types so the

        for (type = 0; type < NUM_STEERING_TYPES; type++)

loop in intel_gt_read_register_fw() becomes a noop and the function just
falls through to the traditional intel_uncore_read_fw() without doing
anything steering-related.  We do temporarily have a zero-sized array in
the structure member which is a bit odd, but gcc doesn't mind that as
long as we never access elements of it (which we won't because all
accesses are inside the body of the loop above).


Matt

> 
> > 
> > 
> > Matt
> > 
> > > > 
> > > > > +}
> > > > > +
> > > > > +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
> > > > > +
> > > > >  void intel_gt_info_print(const struct intel_gt_info *info,
> > > > >  			 struct drm_printer *p);
> > > > >  
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > index fecfacf551d5..47957837c8c0 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > @@ -31,6 +31,26 @@ struct i915_ggtt;
> > > > >  struct intel_engine_cs;
> > > > >  struct intel_uncore;
> > > > >  
> > > > > +struct intel_mmio_range {
> > > > > +       u32 start;
> > > > > +       u32 end;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * The hardware has multiple kinds of multicast register ranges that need
> > > > > + * special register steering (and future platforms are expected to add
> > > > > + * additional types).
> > > > > + *
> > > > > + * During driver startup, we initialize the steering control register to
> > > > > + * direct reads to a slice/subslice that are valid for the 'subslice' class
> > > > > + * of multicast registers.  If another type of steering does not have any
> > > > > + * overlap in valid steering targets with 'subslice' style registers, we will
> > > > > + * need to explicitly re-steer reads of registers of the other type.
> > > > > + */
> > > > > +enum intel_steering_type {
> > > > > +	NUM_STEERING_TYPES
> > > > > +};
> > > > > +
> > > > >  enum intel_submission_method {
> > > > >  	INTEL_SUBMISSION_RING,
> > > > >  	INTEL_SUBMISSION_ELSP,
> > > > > @@ -145,6 +165,8 @@ struct intel_gt {
> > > > >  
> > > > >  	struct i915_vma *scratch;
> > > > >  
> > > > > +	const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
> > > > > +
> > > > >  	struct intel_gt_info {
> > > > >  		intel_engine_mask_t engine_mask;
> > > > >  		u8 num_engines;
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > index b62d1e31a645..689045d3752b 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > @@ -1247,8 +1247,9 @@ wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> > > > >  }
> > > > >  
> > > > >  static void
> > > > > -wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > > > > +wa_list_apply(struct intel_gt *gt, const struct i915_wa_list *wal)
> > > > >  {
> > > > > +	struct intel_uncore *uncore = gt->uncore;
> > > > >  	enum forcewake_domains fw;
> > > > >  	unsigned long flags;
> > > > >  	struct i915_wa *wa;
> > > > > @@ -1263,13 +1264,16 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > > > >  	intel_uncore_forcewake_get__locked(uncore, fw);
> > > > >  
> > > > >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> > > > > -		if (wa->clr)
> > > > > -			intel_uncore_rmw_fw(uncore, wa->reg, wa->clr, wa->set);
> > > > > -		else
> > > > > -			intel_uncore_write_fw(uncore, wa->reg, wa->set);
> > > > > +		u32 val, old = 0;
> > > > > +
> > > > > +		/* open-coded rmw due to steering */
> > > > > +		old = wa->clr ? intel_gt_read_register_fw(gt, wa->reg) : 0;
> > > > > +		val = (old & ~wa->clr) | wa->set;
> > > > > +		if (val != old || !wa->clr)
> > > > > +			intel_uncore_write_fw(uncore, wa->reg, val);
> > > > > +
> > > > >  		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> > > > > -			wa_verify(wa,
> > > > > -				  intel_uncore_read_fw(uncore, wa->reg),
> > > > > +			wa_verify(wa, intel_gt_read_register_fw(gt, wa->reg),
> > > > >  				  wal->name, "application");
> > > > >  	}
> > > > >  
> > > > > @@ -1279,10 +1283,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> > > > >  
> > > > >  void intel_gt_apply_workarounds(struct intel_gt *gt)
> > > > >  {
> > > > > -	wa_list_apply(gt->uncore, &gt->i915->gt_wa_list);
> > > > > +	wa_list_apply(gt, &gt->i915->gt_wa_list);
> > > > >  }
> > > > >  
> > > > > -static bool wa_list_verify(struct intel_uncore *uncore,
> > > > > +static bool wa_list_verify(struct intel_gt *gt,
> > > > >  			   const struct i915_wa_list *wal,
> > > > >  			   const char *from)
> > > > >  {
> > > > > @@ -1292,7 +1296,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> > > > >  
> > > > >  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> > > > >  		ok &= wa_verify(wa,
> > > > > -				intel_uncore_read(uncore, wa->reg),
> > > > > +				intel_gt_read_register_fw(gt, wa->reg),
> > > > >  				wal->name, from);
> > > > >  
> > > > >  	return ok;
> > > > > @@ -1300,7 +1304,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
> > > > >  
> > > > >  bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
> > > > >  {
> > > > > -	return wa_list_verify(gt->uncore, &gt->i915->gt_wa_list, from);
> > > > > +	return wa_list_verify(gt, &gt->i915->gt_wa_list, from);
> > > > >  }
> > > > >  
> > > > >  __maybe_unused
> > > > > @@ -2081,7 +2085,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> > > > >  
> > > > >  void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> > > > >  {
> > > > > -	wa_list_apply(engine->uncore, &engine->wa_list);
> > > > > +	wa_list_apply(engine->gt, &engine->wa_list);
> > > > >  }
> > > > >  
> > > > >  struct mcr_range {
> > > > > diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > > > index c30754daf4b1..7ebc4edb8ecf 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> > > > > @@ -1147,7 +1147,7 @@ verify_wa_lists(struct intel_gt *gt, struct wa_lists *lists,
> > > > >  	enum intel_engine_id id;
> > > > >  	bool ok = true;
> > > > >  
> > > > > -	ok &= wa_list_verify(gt->uncore, &lists->gt_wa_list, str);
> > > > > +	ok &= wa_list_verify(gt, &lists->gt_wa_list, str);
> > > > >  
> > > > >  	for_each_engine(engine, gt, id) {
> > > > >  		struct intel_context *ce;
> > > > > -- 
> > > > > 2.25.4
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx