[PATCH v2 0/6] MCR fixes and more

[PATCH v2 0/6] MCR fixes and more

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A few bugs in programming the MCR register sneaked in past code review.

First of all fls() usage is wrong and suffers from off-by-one problem.

Secondly the assert in WaProgramMgsrForL3BankSpecificMmioReads is also wrong
due inverted logic.

With MCR programming fixed we can stop ignoring the engine workarounds
verification of GEN8_L3SQCREG4. But not registers in the 0xb100-0xb3ff range
which cannot be read reliably by the command streamers.

The logic is also improved to not only assert when static MCR configuration
would not work given specific subslice and L3 bank configuration, but to find a
valid static configuration if possible.

Finally there was a missing perfomance based workaround which loosely belongs
to this overall story of ICL, subslices, L3 banks and workarounds.

Tvrtko Ursulin (6):
  drm/i915: Fix GEN8_MCR_SELECTOR programming
  drm/i915: Trust programmed MCR in read_subslice_reg
  drm/i915: Fix and improve MCR selection logic
  drm/i915: Skip CS verification of L3 bank registers
  drm/i915/icl: Verify engine workarounds in GEN8_L3SQCREG4
  drm/i915/icl: Add Wa_1409178092

 drivers/gpu/drm/i915/gt/intel_engine_cs.c   |  56 ++-----
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 161 ++++++++++++--------
 drivers/gpu/drm/i915/i915_drv.h             |   2 -
 drivers/gpu/drm/i915/i915_reg.h             |   3 +
 4 files changed, 109 insertions(+), 113 deletions(-)

-- 
2.20.1

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

[PATCH 1/6] drm/i915: Fix GEN8_MCR_SELECTOR programming

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

fls returns bit positions starting from one for the lsb and the MCR
register expects zero based (sub)slice addressing.

Incorrent MCR programming can have the effect of directing MMIO reads of
registers in the 0xb100-0xb3ff range to invalid subslice returning zeroes
instead of actual content.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads")
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c0bc9cb7f228..6f93caf7a5a1 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -962,9 +962,14 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
 u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv)
 {
 	const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu;
+	unsigned int slice = fls(sseu->slice_mask) - 1;
+	unsigned int subslice;
 	u32 mcr_s_ss_select;
-	u32 slice = fls(sseu->slice_mask);
-	u32 subslice = fls(sseu->subslice_mask[slice]);
+
+	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
+	subslice = fls(sseu->subslice_mask[slice]);
+	GEM_BUG_ON(!subslice);
+	subslice--;
 
 	if (IS_GEN(dev_priv, 10))
 		mcr_s_ss_select = GEN8_MCR_SLICE(slice) |
-- 
2.20.1

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

[PATCH 2/6] drm/i915: Trust programmed MCR in read_subslice_reg

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Instead of re-calculating the MCR selector in read_subslice_reg do the
rwm on its existing value and restore it when done.

This consolidates MCR programming to one place for cnl+, and avoids
re-calculating its default value on older platforms during hangcheck.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 37 ++++++++---------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 6f93caf7a5a1..cc4d1826173d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -989,27 +989,17 @@ read_subslice_reg(struct intel_engine_cs *engine, int slice, int subslice,
 {
 	struct drm_i915_private *i915 = engine->i915;
 	struct intel_uncore *uncore = engine->uncore;
-	u32 mcr_slice_subslice_mask;
-	u32 mcr_slice_subslice_select;
-	u32 default_mcr_s_ss_select;
-	u32 mcr;
-	u32 ret;
+	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
 	enum forcewake_domains fw_domains;
 
 	if (INTEL_GEN(i915) >= 11) {
-		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
-					  GEN11_MCR_SUBSLICE_MASK;
-		mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) |
-					    GEN11_MCR_SUBSLICE(subslice);
+		mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
+		mcr_ss = GEN11_MCR_SLICE(slice) | GEN11_MCR_SUBSLICE(subslice);
 	} else {
-		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
-					  GEN8_MCR_SUBSLICE_MASK;
-		mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) |
-					    GEN8_MCR_SUBSLICE(subslice);
+		mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
+		mcr_ss = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
 	}
 
-	default_mcr_s_ss_select = intel_calculate_mcr_s_ss_select(i915);
-
 	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
 						    FW_REG_READ);
 	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
@@ -1019,26 +1009,23 @@ read_subslice_reg(struct intel_engine_cs *engine, int slice, int subslice,
 	spin_lock_irq(&uncore->lock);
 	intel_uncore_forcewake_get__locked(uncore, fw_domains);
 
-	mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
-
-	WARN_ON_ONCE((mcr & mcr_slice_subslice_mask) !=
-		     default_mcr_s_ss_select);
+	old_mcr = mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
 
-	mcr &= ~mcr_slice_subslice_mask;
-	mcr |= mcr_slice_subslice_select;
+	mcr &= ~mcr_mask;
+	mcr |= mcr_ss;
 	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
 
-	ret = intel_uncore_read_fw(uncore, reg);
+	val = intel_uncore_read_fw(uncore, reg);
 
-	mcr &= ~mcr_slice_subslice_mask;
-	mcr |= default_mcr_s_ss_select;
+	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 ret;
+	return val;
 }
 
 /* NB: please notice the memset */
-- 
2.20.1

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

[PATCH 3/6] drm/i915: Fix and improve MCR selection logic

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A couple issues were present in this code:

1.
fls() usage was incorrect causing off by one in subslice mask lookup,
which in other words means subslice mask of all zeroes is always used
(subslice mask of a slice which is not present, or even out of bounds
array access), rendering the checks in wa_init_mcr either futile or
random.

2.
Condition in WARN_ON was not correct. It is doing a bitwise and operation
between a positive (present subslices) and negative mask (disabled L3
banks).

This means that with corrected fls() usage the assert would always
incorrectly fail.

We could fix this by inverting the fuse bits in the check, but instead do
one better and improve the code so it not only asserts, but finds the
first common index between the two masks and only warns if no such index
can be found.

v2:
 * Simplify check for logic and redability.
 * Improve commentary explaining what is really happening ie. what the
   assert is really trying to check and why.

v3:
 * Find first common index instead of just asserting.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: fe864b76c2ab ("drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads")
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 24 ------
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 90 +++++++++++----------
 drivers/gpu/drm/i915/i915_drv.h             |  2 -
 3 files changed, 49 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index cc4d1826173d..65cbf1d9118d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -959,30 +959,6 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
 	}
 }
 
-u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv)
-{
-	const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu;
-	unsigned int slice = fls(sseu->slice_mask) - 1;
-	unsigned int subslice;
-	u32 mcr_s_ss_select;
-
-	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
-	subslice = fls(sseu->subslice_mask[slice]);
-	GEM_BUG_ON(!subslice);
-	subslice--;
-
-	if (IS_GEN(dev_priv, 10))
-		mcr_s_ss_select = GEN8_MCR_SLICE(slice) |
-				  GEN8_MCR_SUBSLICE(subslice);
-	else if (INTEL_GEN(dev_priv) >= 11)
-		mcr_s_ss_select = GEN11_MCR_SLICE(slice) |
-				  GEN11_MCR_SUBSLICE(subslice);
-	else
-		mcr_s_ss_select = 0;
-
-	return mcr_s_ss_select;
-}
-
 static u32
 read_subslice_reg(struct intel_engine_cs *engine, int slice, int subslice,
 		  i915_reg_t reg)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 3b1fc7c8faa8..c2325b7ecf8d 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -762,7 +762,10 @@ static void
 wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
 {
 	const struct sseu_dev_info *sseu = &RUNTIME_INFO(i915)->sseu;
-	u32 mcr_slice_subslice_mask;
+	unsigned int slice, subslice;
+	u32 l3_en, mcr, mcr_mask;
+
+	GEM_BUG_ON(INTEL_GEN(i915) < 10);
 
 	/*
 	 * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
@@ -770,42 +773,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
 	 * 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).
-	 * This might be incompatible with
-	 * WaProgramMgsrForCorrectSliceSpecificMmioReads.
-	 * Fortunately, this should not happen in production hardware, so
-	 * we only assert that this is the case (instead of implementing
-	 * something more complex that requires checking the range of every
-	 * MMIO read).
-	 */
-	if (INTEL_GEN(i915) >= 10 &&
-	    is_power_of_2(sseu->slice_mask)) {
-		/*
-		 * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
-		 * enabled subslice, no need to redirect MCR packet
-		 */
-		u32 slice = fls(sseu->slice_mask);
-		u32 fuse3 =
-			intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3);
-		u8 ss_mask = sseu->subslice_mask[slice];
-
-		u8 enabled_mask = (ss_mask | ss_mask >>
-				   GEN10_L3BANK_PAIR_COUNT) & GEN10_L3BANK_MASK;
-		u8 disabled_mask = fuse3 & GEN10_L3BANK_MASK;
-
-		/*
-		 * Production silicon should have matched L3Bank and
-		 * subslice enabled
-		 */
-		WARN_ON((enabled_mask & disabled_mask) != enabled_mask);
-	}
-
-	if (INTEL_GEN(i915) >= 11)
-		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
-					  GEN11_MCR_SUBSLICE_MASK;
-	else
-		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
-					  GEN8_MCR_SUBSLICE_MASK;
-	/*
+	 *
 	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl
 	 * Before any MMIO read into slice/subslice specific registers, MCR
 	 * packet control register needs to be programmed to point to any
@@ -815,11 +783,51 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
 	 * 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.
 	 */
-	wa_write_masked_or(wal,
-			   GEN8_MCR_SELECTOR,
-			   mcr_slice_subslice_mask,
-			   intel_calculate_mcr_s_ss_select(i915));
+
+	if (INTEL_GEN(i915) >= 10 && is_power_of_2(sseu->slice_mask)) {
+		u32 l3_fuse =
+			intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3) &
+			GEN10_L3BANK_MASK;
+
+		DRM_DEBUG_DRIVER("L3 fuse = %x\n", l3_fuse);
+		l3_en = ~(l3_fuse << GEN10_L3BANK_PAIR_COUNT | l3_fuse);
+	} else {
+		l3_en = ~0;
+	}
+
+	slice = fls(sseu->slice_mask) - 1;
+	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
+	subslice = fls(l3_en & sseu->subslice_mask[slice]);
+	if (!subslice) {
+		DRM_WARN("No common index found between subslice mask %x and L3 bank mask %x!\n",
+			 sseu->subslice_mask[slice], l3_en);
+		subslice = fls(l3_en);
+		WARN_ON(!subslice);
+	}
+	subslice--;
+
+	if (INTEL_GEN(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;
+	}
+
+	DRM_DEBUG_DRIVER("MCR slice/subslice = %x\n", mcr);
+
+	wa_write_masked_or(wal, GEN8_MCR_SELECTOR, mcr_mask, mcr);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 78c1ed6e17b2..0e44cc4b2ca1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2389,8 +2389,6 @@ void i915_driver_remove(struct drm_device *dev);
 void intel_engine_init_hangcheck(struct intel_engine_cs *engine);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 
-u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv);
-
 static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
 {
 	return dev_priv->gvt;
-- 
2.20.1

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

[PATCH 4/6] drm/i915: Skip CS verification of L3 bank registers

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Access to 0xb100 - 0xb3ff mmio range is controlled by the MCR selector
which only affects CPU MMIO. Therefore these registers cannot be realiably
read with MI_SRM from the command streamer so skip their verification.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 38 ++++++++++++++++++---
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index c2325b7ecf8d..619d42a2b81b 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1436,26 +1436,50 @@ create_scratch(struct i915_address_space *vm, int count)
 	return ERR_PTR(err);
 }
 
+static bool mcr_range(struct drm_i915_private *i915, u32 offset)
+{
+	/*
+	 * Registers in this range are affected by the MCR selector
+	 * which only controls CPU initiated MMIO. Routing does not
+	 * work for CS access so we cannot verify them on this path.
+	 */
+	if (INTEL_GEN(i915) >= 8 && (offset >= 0xb100 && offset <= 0xb3ff))
+		return true;
+
+	return false;
+}
+
 static int
 wa_list_srm(struct i915_request *rq,
 	    const struct i915_wa_list *wal,
 	    struct i915_vma *vma)
 {
+	struct drm_i915_private *i915 = rq->i915;
+	unsigned int i, count = 0;
 	const struct i915_wa *wa;
-	unsigned int i;
 	u32 srm, *cs;
 
 	srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
-	if (INTEL_GEN(rq->i915) >= 8)
+	if (INTEL_GEN(i915) >= 8)
 		srm++;
 
-	cs = intel_ring_begin(rq, 4 * wal->count);
+	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
+		if (!mcr_range(i915, i915_mmio_reg_offset(wa->reg)))
+			count++;
+	}
+
+	cs = intel_ring_begin(rq, 4 * count);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
 	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
+		u32 offset = i915_mmio_reg_offset(wa->reg);
+
+		if (mcr_range(i915, offset))
+			continue;
+
 		*cs++ = srm;
-		*cs++ = i915_mmio_reg_offset(wa->reg);
+		*cs++ = offset;
 		*cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
 		*cs++ = 0;
 	}
@@ -1505,9 +1529,13 @@ static int engine_wa_list_verify(struct intel_context *ce,
 	}
 
 	err = 0;
-	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
+	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
+		if (mcr_range(rq->i915, i915_mmio_reg_offset(wa->reg)))
+			continue;
+
 		if (!wa_verify(wa, results[i], wal->name, from))
 			err = -ENXIO;
+	}
 
 	i915_gem_object_unpin_map(vma->obj);
 
-- 
2.20.1

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

[PATCH 5/6] drm/i915/icl: Verify engine workarounds in GEN8_L3SQCREG4

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Having fixed the incorect MCR programming in an earlier patch, we can now
stop ignoring read back of GEN8_L3SQCREG4 during engine workaround
verification.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 27 +++++----------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 619d42a2b81b..ff532ff5d574 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -177,19 +177,6 @@ wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
 	wa_write_masked_or(wal, reg, val, val);
 }
 
-static void
-ignore_wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask, u32 val)
-{
-	struct i915_wa wa = {
-		.reg  = reg,
-		.mask = mask,
-		.val  = val,
-		/* Bonkers HW, skip verifying */
-	};
-
-	_wa_add(wal, &wa);
-}
-
 #define WA_SET_BIT_MASKED(addr, mask) \
 	wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_ENABLE(mask))
 
@@ -1260,10 +1247,9 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 			     _3D_CHICKEN3_AA_LINE_QUALITY_FIX_ENABLE);
 
 		/* WaPipelineFlushCoherentLines:icl */
-		ignore_wa_write_or(wal,
-				   GEN8_L3SQCREG4,
-				   GEN8_LQSC_FLUSH_COHERENT_LINES,
-				   GEN8_LQSC_FLUSH_COHERENT_LINES);
+		wa_write_or(wal,
+			    GEN8_L3SQCREG4,
+			    GEN8_LQSC_FLUSH_COHERENT_LINES);
 
 		/*
 		 * Wa_1405543622:icl
@@ -1290,10 +1276,9 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 		 * Wa_1405733216:icl
 		 * Formerly known as WaDisableCleanEvicts
 		 */
-		ignore_wa_write_or(wal,
-				   GEN8_L3SQCREG4,
-				   GEN11_LQSC_CLEAN_EVICT_DISABLE,
-				   GEN11_LQSC_CLEAN_EVICT_DISABLE);
+		wa_write_or(wal,
+			    GEN8_L3SQCREG4,
+			    GEN11_LQSC_CLEAN_EVICT_DISABLE);
 
 		/* WaForwardProgressSoftReset:icl */
 		wa_write_or(wal,
-- 
2.20.1

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

[PATCH 6/6] drm/i915/icl: Add Wa_1409178092

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We were missing this workaround which can cause hangs if fine grained
coherency was used.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++
 drivers/gpu/drm/i915/i915_reg.h             | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index ff532ff5d574..704ace01e7f5 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1297,6 +1297,12 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 		wa_write_or(wal,
 			    GEN7_SARCHKMD,
 			    GEN7_DISABLE_SAMPLER_PREFETCH);
+
+		/* Wa_1409178092:icl */
+		wa_write_masked_or(wal,
+				   GEN11_SCRATCH2,
+				   GEN11_COHERENT_PARTIAL_WRITE_MERGE_ENABLE,
+				   0);
 	}
 
 	if (IS_GEN_RANGE(i915, 9, 11)) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fdd9bc01e694..24f2a52a2b42 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7721,6 +7721,9 @@ enum {
 #define GEN7_L3SQCREG4				_MMIO(0xb034)
 #define  L3SQ_URB_READ_CAM_MATCH_DISABLE	(1 << 27)
 
+#define GEN11_SCRATCH2					_MMIO(0xb140)
+#define  GEN11_COHERENT_PARTIAL_WRITE_MERGE_ENABLE	(1 << 19)
+
 #define GEN8_L3SQCREG4				_MMIO(0xb118)
 #define  GEN11_LQSC_CLEAN_EVICT_DISABLE		(1 << 6)
 #define  GEN8_LQSC_RO_PERF_DIS			(1 << 27)
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for MCR fixes and more

[Patchwork]

== Series Details ==

Series: MCR fixes and more
URL   : https://patchwork.freedesktop.org/series/63831/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
53048f39ddd0 drm/i915: Fix GEN8_MCR_SELECTOR programming
9874e34931ae drm/i915: Trust programmed MCR in read_subslice_reg
-:59: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#59: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:1012:
+	old_mcr = mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);

total: 0 errors, 0 warnings, 1 checks, 65 lines checked
05988d2693e3 drm/i915: Fix and improve MCR selection logic
478276140729 drm/i915: Skip CS verification of L3 bank registers
69c8f4a77a62 drm/i915/icl: Verify engine workarounds in GEN8_L3SQCREG4
d3068b3ac79f drm/i915/icl: Add Wa_1409178092

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

✓ Fi.CI.BAT: success for MCR fixes and more

[Patchwork]

== Series Details ==

Series: MCR fixes and more
URL   : https://patchwork.freedesktop.org/series/63831/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6502 -> Patchwork_13676
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@gem_basic@bad-close:
    - fi-icl-u3:          [DMESG-WARN][1] ([fdo#107724]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-u3/igt@gem_basic@bad-close.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-u3/igt@gem_basic@bad-close.html

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u3:          [INCOMPLETE][3] ([fdo#107713] / [fdo#109100]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-u3/igt@gem_ctx_create@basic-files.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-u3/igt@gem_ctx_create@basic-files.html

  * {igt@gem_ctx_switch@legacy-render}:
    - fi-icl-guc:         [INCOMPLETE][5] ([fdo#107713]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html

  * {igt@gem_ctx_switch@rcs0}:
    - fi-icl-u2:          [INCOMPLETE][7] ([fdo#107713]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-u2/igt@gem_ctx_switch@rcs0.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-u2/igt@gem_ctx_switch@rcs0.html

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

  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100


Participating hosts (52 -> 47)
------------------------------

  Additional (1): fi-apl-guc 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_6502 -> Patchwork_13676

  CI_DRM_6502: 606a844d5d932fb07b2377b95c0fe7b08383e32a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5102: 6038ace76016d8892f4d13aef5301f71ca1a6e2d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13676: d3068b3ac79ffaaa0ade8d8ef004e1f9fa2b59c6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d3068b3ac79f drm/i915/icl: Add Wa_1409178092
69c8f4a77a62 drm/i915/icl: Verify engine workarounds in GEN8_L3SQCREG4
478276140729 drm/i915: Skip CS verification of L3 bank registers
05988d2693e3 drm/i915: Fix and improve MCR selection logic
9874e34931ae drm/i915: Trust programmed MCR in read_subslice_reg
53048f39ddd0 drm/i915: Fix GEN8_MCR_SELECTOR programming

== Logs ==

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

Re: [PATCH 2/6] drm/i915: Trust programmed MCR in read_subslice_reg

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 19:06:20)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Instead of re-calculating the MCR selector in read_subslice_reg do the
> rwm on its existing value and restore it when done.

I successfully worked back from implementation to changelog.
> 
> This consolidates MCR programming to one place for cnl+, and avoids
> re-calculating its default value on older platforms during hangcheck.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: Fix and improve MCR selection logic

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 19:06:21)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> A couple issues were present in this code:
> 
> 1.
> fls() usage was incorrect causing off by one in subslice mask lookup,
> which in other words means subslice mask of all zeroes is always used
> (subslice mask of a slice which is not present, or even out of bounds
> array access), rendering the checks in wa_init_mcr either futile or
> random.
> 
> 2.
> Condition in WARN_ON was not correct. It is doing a bitwise and operation
> between a positive (present subslices) and negative mask (disabled L3
> banks).
> 
> This means that with corrected fls() usage the assert would always
> incorrectly fail.
> 
> We could fix this by inverting the fuse bits in the check, but instead do
> one better and improve the code so it not only asserts, but finds the
> first common index between the two masks and only warns if no such index
> can be found.
> 
> v2:
>  * Simplify check for logic and redability.
>  * Improve commentary explaining what is really happening ie. what the
>    assert is really trying to check and why.
> 
> v3:
>  * Find first common index instead of just asserting.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: fe864b76c2ab ("drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads")
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1

It's still magic to me, but I can attest that it does what you say, and
should be no worse than before :)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: Skip CS verification of L3 bank registers

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 19:06:22)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Access to 0xb100 - 0xb3ff mmio range is controlled by the MCR selector
> which only affects CPU MMIO. Therefore these registers cannot be realiably
> read with MI_SRM from the command streamer so skip their verification.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 38 ++++++++++++++++++---
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index c2325b7ecf8d..619d42a2b81b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1436,26 +1436,50 @@ create_scratch(struct i915_address_space *vm, int count)
>         return ERR_PTR(err);
>  }
>  
> +static bool mcr_range(struct drm_i915_private *i915, u32 offset)
> +{
> +       /*
> +        * Registers in this range are affected by the MCR selector
> +        * which only controls CPU initiated MMIO. Routing does not
> +        * work for CS access so we cannot verify them on this path.
> +        */
> +       if (INTEL_GEN(i915) >= 8 && (offset >= 0xb100 && offset <= 0xb3ff))
> +               return true;

Bonus (). I was thinking maybe
	if (INTEL_GEN() < 8)
		return false;
	return offset >= 0xb100 && offset <= 0xb3ff;
to remove the apparent need. But structuring the checks with a
per-gen if-ladder will probably be easier to extend should the need
arise.

> +       return false;
> +}
> +
>  static int
>  wa_list_srm(struct i915_request *rq,
>             const struct i915_wa_list *wal,
>             struct i915_vma *vma)
>  {
> +       struct drm_i915_private *i915 = rq->i915;
> +       unsigned int i, count = 0;
>         const struct i915_wa *wa;
> -       unsigned int i;
>         u32 srm, *cs;
>  
>         srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
> -       if (INTEL_GEN(rq->i915) >= 8)
> +       if (INTEL_GEN(i915) >= 8)
>                 srm++;
>  
> -       cs = intel_ring_begin(rq, 4 * wal->count);
> +       for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +               if (!mcr_range(i915, i915_mmio_reg_offset(wa->reg)))
> +                       count++;
> +       }
> +
> +       cs = intel_ring_begin(rq, 4 * count);
>         if (IS_ERR(cs))
>                 return PTR_ERR(cs);
>  
>         for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +               u32 offset = i915_mmio_reg_offset(wa->reg);
> +
> +               if (mcr_range(i915, offset))
> +                       continue;
> +

I would have just done the reg read and ignored the result, rather than
add another loop to fixup the count.

>                 *cs++ = srm;
> -               *cs++ = i915_mmio_reg_offset(wa->reg);
> +               *cs++ = offset;
>                 *cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
>                 *cs++ = 0;
>         }
> @@ -1505,9 +1529,13 @@ static int engine_wa_list_verify(struct intel_context *ce,
>         }
>  
>         err = 0;
> -       for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> +       for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +               if (mcr_range(rq->i915, i915_mmio_reg_offset(wa->reg)))
> +                       continue;
> +
>                 if (!wa_verify(wa, results[i], wal->name, from))
>                         err = -ENXIO;
> +       }

Looks fine though
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/icl: Add Wa_1409178092

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 19:06:24)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> We were missing this workaround which can cause hangs if fine grained
> coherency was used.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++
>  drivers/gpu/drm/i915/i915_reg.h             | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index ff532ff5d574..704ace01e7f5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1297,6 +1297,12 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>                 wa_write_or(wal,
>                             GEN7_SARCHKMD,
>                             GEN7_DISABLE_SAMPLER_PREFETCH);
> +
> +               /* Wa_1409178092:icl */
> +               wa_write_masked_or(wal,
> +                                  GEN11_SCRATCH2,
> +                                  GEN11_COHERENT_PARTIAL_WRITE_MERGE_ENABLE,
> +                                  0);

It's mentioned that this is only for HAS_LLC(), to avoid coming back to
this in future, I'd mark it up now.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: Trust programmed MCR in read_subslice_reg

[Summers, Stuart]

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

On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Instead of re-calculating the MCR selector in read_subslice_reg do
> the
> rwm on its existing value and restore it when done.
> 
> This consolidates MCR programming to one place for cnl+, and avoids
> re-calculating its default value on older platforms during hangcheck.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 37 ++++++++-------------
> --
>  1 file changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 6f93caf7a5a1..cc4d1826173d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -989,27 +989,17 @@ read_subslice_reg(struct intel_engine_cs
> *engine, int slice, int subslice,
>  {
>  	struct drm_i915_private *i915 = engine->i915;
>  	struct intel_uncore *uncore = engine->uncore;
> -	u32 mcr_slice_subslice_mask;
> -	u32 mcr_slice_subslice_select;
> -	u32 default_mcr_s_ss_select;
> -	u32 mcr;
> -	u32 ret;
> +	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
>  	enum forcewake_domains fw_domains;
>  
>  	if (INTEL_GEN(i915) >= 11) {
> -		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
> -					  GEN11_MCR_SUBSLICE_MASK;
> -		mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) |
> -					    GEN11_MCR_SUBSLICE(subslice
> );
> +		mcr_mask = GEN11_MCR_SLICE_MASK |
> GEN11_MCR_SUBSLICE_MASK;
> +		mcr_ss = GEN11_MCR_SLICE(slice) |
> GEN11_MCR_SUBSLICE(subslice);
>  	} else {
> -		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
> -					  GEN8_MCR_SUBSLICE_MASK;
> -		mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) |
> -					    GEN8_MCR_SUBSLICE(subslice)
> ;
> +		mcr_mask = GEN8_MCR_SLICE_MASK |
> GEN8_MCR_SUBSLICE_MASK;
> +		mcr_ss = GEN8_MCR_SLICE(slice) |
> GEN8_MCR_SUBSLICE(subslice);
>  	}
>  
> -	default_mcr_s_ss_select =
> intel_calculate_mcr_s_ss_select(i915);

I'm only now reading one user of this function after this change. Does
it make sense to move it to gt/intel_workarounds.c as a static function
instead? Not critical here.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

> -
>  	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
>  						    FW_REG_READ);
>  	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
> @@ -1019,26 +1009,23 @@ read_subslice_reg(struct intel_engine_cs
> *engine, int slice, int subslice,
>  	spin_lock_irq(&uncore->lock);
>  	intel_uncore_forcewake_get__locked(uncore, fw_domains);
>  
> -	mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
> -
> -	WARN_ON_ONCE((mcr & mcr_slice_subslice_mask) !=
> -		     default_mcr_s_ss_select);
> +	old_mcr = mcr = intel_uncore_read_fw(uncore,
> GEN8_MCR_SELECTOR);
>  
> -	mcr &= ~mcr_slice_subslice_mask;
> -	mcr |= mcr_slice_subslice_select;
> +	mcr &= ~mcr_mask;
> +	mcr |= mcr_ss;
>  	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
>  
> -	ret = intel_uncore_read_fw(uncore, reg);
> +	val = intel_uncore_read_fw(uncore, reg);
>  
> -	mcr &= ~mcr_slice_subslice_mask;
> -	mcr |= default_mcr_s_ss_select;
> +	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 ret;
> +	return val;
>  }
>  
>  /* NB: please notice the memset */

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

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

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

Re: [PATCH 2/6] drm/i915: Trust programmed MCR in read_subslice_reg

[Summers, Stuart]

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

On Wed, 2019-07-17 at 20:47 +0000, Summers, Stuart wrote:
> On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Instead of re-calculating the MCR selector in read_subslice_reg do
> > the
> > rwm on its existing value and restore it when done.
> > 
> > This consolidates MCR programming to one place for cnl+, and avoids
> > re-calculating its default value on older platforms during
> > hangcheck.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 37 ++++++++-----------
> > --
> > --
> >  1 file changed, 12 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index 6f93caf7a5a1..cc4d1826173d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -989,27 +989,17 @@ read_subslice_reg(struct intel_engine_cs
> > *engine, int slice, int subslice,
> >  {
> >  	struct drm_i915_private *i915 = engine->i915;
> >  	struct intel_uncore *uncore = engine->uncore;
> > -	u32 mcr_slice_subslice_mask;
> > -	u32 mcr_slice_subslice_select;
> > -	u32 default_mcr_s_ss_select;
> > -	u32 mcr;
> > -	u32 ret;
> > +	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
> >  	enum forcewake_domains fw_domains;
> >  
> >  	if (INTEL_GEN(i915) >= 11) {
> > -		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
> > -					  GEN11_MCR_SUBSLICE_MASK;
> > -		mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) |
> > -					    GEN11_MCR_SUBSLICE(subslice
> > );
> > +		mcr_mask = GEN11_MCR_SLICE_MASK |
> > GEN11_MCR_SUBSLICE_MASK;
> > +		mcr_ss = GEN11_MCR_SLICE(slice) |
> > GEN11_MCR_SUBSLICE(subslice);
> >  	} else {
> > -		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
> > -					  GEN8_MCR_SUBSLICE_MASK;
> > -		mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) |
> > -					    GEN8_MCR_SUBSLICE(subslice)
> > ;
> > +		mcr_mask = GEN8_MCR_SLICE_MASK |
> > GEN8_MCR_SUBSLICE_MASK;
> > +		mcr_ss = GEN8_MCR_SLICE(slice) |
> > GEN8_MCR_SUBSLICE(subslice);
> >  	}
> >  
> > -	default_mcr_s_ss_select =
> > intel_calculate_mcr_s_ss_select(i915);
> 
> I'm only now reading one user of this function after this change.
> Does
> it make sense to move it to gt/intel_workarounds.c as a static
> function
> instead? Not critical here.

I see you already did this, what I get for reading these one at a
time... Please ignore the above comment.

Thanks,
Stuart

> 
> Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> 
> > -
> >  	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
> >  						    FW_REG_READ);
> >  	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
> > @@ -1019,26 +1009,23 @@ read_subslice_reg(struct intel_engine_cs
> > *engine, int slice, int subslice,
> >  	spin_lock_irq(&uncore->lock);
> >  	intel_uncore_forcewake_get__locked(uncore, fw_domains);
> >  
> > -	mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
> > -
> > -	WARN_ON_ONCE((mcr & mcr_slice_subslice_mask) !=
> > -		     default_mcr_s_ss_select);
> > +	old_mcr = mcr = intel_uncore_read_fw(uncore,
> > GEN8_MCR_SELECTOR);
> >  
> > -	mcr &= ~mcr_slice_subslice_mask;
> > -	mcr |= mcr_slice_subslice_select;
> > +	mcr &= ~mcr_mask;
> > +	mcr |= mcr_ss;
> >  	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
> >  
> > -	ret = intel_uncore_read_fw(uncore, reg);
> > +	val = intel_uncore_read_fw(uncore, reg);
> >  
> > -	mcr &= ~mcr_slice_subslice_mask;
> > -	mcr |= default_mcr_s_ss_select;
> > +	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 ret;
> > +	return val;
> >  }
> >  
> >  /* NB: please notice the memset */
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

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

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

Re: [PATCH 3/6] drm/i915: Fix and improve MCR selection logic

[Summers, Stuart]

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

On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> A couple issues were present in this code:
> 
> 1.
> fls() usage was incorrect causing off by one in subslice mask lookup,
> which in other words means subslice mask of all zeroes is always used
> (subslice mask of a slice which is not present, or even out of bounds
> array access), rendering the checks in wa_init_mcr either futile or
> random.
> 
> 2.
> Condition in WARN_ON was not correct. It is doing a bitwise and
> operation
> between a positive (present subslices) and negative mask (disabled L3
> banks).
> 
> This means that with corrected fls() usage the assert would always
> incorrectly fail.
> 
> We could fix this by inverting the fuse bits in the check, but
> instead do
> one better and improve the code so it not only asserts, but finds the
> first common index between the two masks and only warns if no such
> index
> can be found.
> 
> v2:
>  * Simplify check for logic and redability.
>  * Improve commentary explaining what is really happening ie. what
> the
>    assert is really trying to check and why.
> 
> v3:
>  * Find first common index instead of just asserting.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: fe864b76c2ab ("drm/i915: Implement
> WaProgramMgsrForL3BankSpecificMmioReads")
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 24 ------
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 90 +++++++++++------
> ----
>  drivers/gpu/drm/i915/i915_drv.h             |  2 -
>  3 files changed, 49 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index cc4d1826173d..65cbf1d9118d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -959,30 +959,6 @@ const char *i915_cache_level_str(struct
> drm_i915_private *i915, int type)
>  	}
>  }
>  
> -u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private
> *dev_priv)
> -{
> -	const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)-
> >sseu;
> -	unsigned int slice = fls(sseu->slice_mask) - 1;
> -	unsigned int subslice;
> -	u32 mcr_s_ss_select;
> -
> -	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
> -	subslice = fls(sseu->subslice_mask[slice]);
> -	GEM_BUG_ON(!subslice);
> -	subslice--;
> -
> -	if (IS_GEN(dev_priv, 10))
> -		mcr_s_ss_select = GEN8_MCR_SLICE(slice) |
> -				  GEN8_MCR_SUBSLICE(subslice);
> -	else if (INTEL_GEN(dev_priv) >= 11)
> -		mcr_s_ss_select = GEN11_MCR_SLICE(slice) |
> -				  GEN11_MCR_SUBSLICE(subslice);
> -	else
> -		mcr_s_ss_select = 0;
> -
> -	return mcr_s_ss_select;
> -}
> -
>  static u32
>  read_subslice_reg(struct intel_engine_cs *engine, int slice, int
> subslice,
>  		  i915_reg_t reg)
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 3b1fc7c8faa8..c2325b7ecf8d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -762,7 +762,10 @@ static void
>  wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
>  {
>  	const struct sseu_dev_info *sseu = &RUNTIME_INFO(i915)->sseu;
> -	u32 mcr_slice_subslice_mask;
> +	unsigned int slice, subslice;
> +	u32 l3_en, mcr, mcr_mask;
> +
> +	GEM_BUG_ON(INTEL_GEN(i915) < 10);
>  
>  	/*
>  	 * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
> @@ -770,42 +773,7 @@ wa_init_mcr(struct drm_i915_private *i915,
> struct i915_wa_list *wal)
>  	 * 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).
> -	 * This might be incompatible with
> -	 * WaProgramMgsrForCorrectSliceSpecificMmioReads.
> -	 * Fortunately, this should not happen in production hardware,
> so
> -	 * we only assert that this is the case (instead of
> implementing
> -	 * something more complex that requires checking the range of
> every
> -	 * MMIO read).
> -	 */
> -	if (INTEL_GEN(i915) >= 10 &&
> -	    is_power_of_2(sseu->slice_mask)) {
> -		/*
> -		 * read FUSE3 for enabled L3 Bank IDs, if L3 Bank
> matches
> -		 * enabled subslice, no need to redirect MCR packet
> -		 */
> -		u32 slice = fls(sseu->slice_mask);
> -		u32 fuse3 =
> -			intel_uncore_read(&i915->uncore,
> GEN10_MIRROR_FUSE3);
> -		u8 ss_mask = sseu->subslice_mask[slice];
> -
> -		u8 enabled_mask = (ss_mask | ss_mask >>
> -				   GEN10_L3BANK_PAIR_COUNT) &
> GEN10_L3BANK_MASK;
> -		u8 disabled_mask = fuse3 & GEN10_L3BANK_MASK;
> -
> -		/*
> -		 * Production silicon should have matched L3Bank and
> -		 * subslice enabled
> -		 */
> -		WARN_ON((enabled_mask & disabled_mask) !=
> enabled_mask);
> -	}
> -
> -	if (INTEL_GEN(i915) >= 11)
> -		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
> -					  GEN11_MCR_SUBSLICE_MASK;
> -	else
> -		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
> -					  GEN8_MCR_SUBSLICE_MASK;
> -	/*
> +	 *
>  	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl
>  	 * Before any MMIO read into slice/subslice specific registers,
> MCR
>  	 * packet control register needs to be programmed to point to
> any
> @@ -815,11 +783,51 @@ wa_init_mcr(struct drm_i915_private *i915,
> struct i915_wa_list *wal)
>  	 * 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.
>  	 */
> -	wa_write_masked_or(wal,
> -			   GEN8_MCR_SELECTOR,
> -			   mcr_slice_subslice_mask,
> -			   intel_calculate_mcr_s_ss_select(i915));
> +
> +	if (INTEL_GEN(i915) >= 10 && is_power_of_2(sseu->slice_mask)) {
> +		u32 l3_fuse =
> +			intel_uncore_read(&i915->uncore,
> GEN10_MIRROR_FUSE3) &
> +			GEN10_L3BANK_MASK;
> +
> +		DRM_DEBUG_DRIVER("L3 fuse = %x\n", l3_fuse);
> +		l3_en = ~(l3_fuse << GEN10_L3BANK_PAIR_COUNT |
> l3_fuse);
> +	} else {
> +		l3_en = ~0;
> +	}
> +
> +	slice = fls(sseu->slice_mask) - 1;
> +	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
> +	subslice = fls(l3_en & sseu->subslice_mask[slice]);
> +	if (!subslice) {
> +		DRM_WARN("No common index found between subslice mask
> %x and L3 bank mask %x!\n",
> +			 sseu->subslice_mask[slice], l3_en);
> +		subslice = fls(l3_en);
> +		WARN_ON(!subslice);
> +	}
> +	subslice--;
> +
> +	if (INTEL_GEN(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;
> +	}
> +
> +	DRM_DEBUG_DRIVER("MCR slice/subslice = %x\n", mcr);
> +
> +	wa_write_masked_or(wal, GEN8_MCR_SELECTOR, mcr_mask, mcr);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 78c1ed6e17b2..0e44cc4b2ca1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2389,8 +2389,6 @@ void i915_driver_remove(struct drm_device
> *dev);
>  void intel_engine_init_hangcheck(struct intel_engine_cs *engine);
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>  
> -u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private
> *dev_priv);
> -
>  static inline bool intel_gvt_active(struct drm_i915_private
> *dev_priv)
>  {
>  	return dev_priv->gvt;

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

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

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

Re: [PATCH 1/6] drm/i915: Fix GEN8_MCR_SELECTOR programming

[Summers, Stuart]

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

On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Not opposed to this exactly, but do we really need this patch if we're
just getting rid of this routine later in the series?

Thanks,
Stuart

> 
> fls returns bit positions starting from one for the lsb and the MCR
> register expects zero based (sub)slice addressing.
> 
> Incorrent MCR programming can have the effect of directing MMIO reads
> of
> registers in the 0xb100-0xb3ff range to invalid subslice returning
> zeroes
> instead of actual content.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement
> WaProgramMgsrForCorrectSliceSpecificMmioReads")
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index c0bc9cb7f228..6f93caf7a5a1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -962,9 +962,14 @@ const char *i915_cache_level_str(struct
> drm_i915_private *i915, int type)
>  u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private
> *dev_priv)
>  {
>  	const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)-
> >sseu;
> +	unsigned int slice = fls(sseu->slice_mask) - 1;
> +	unsigned int subslice;
>  	u32 mcr_s_ss_select;
> -	u32 slice = fls(sseu->slice_mask);
> -	u32 subslice = fls(sseu->subslice_mask[slice]);
> +
> +	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
> +	subslice = fls(sseu->subslice_mask[slice]);
> +	GEM_BUG_ON(!subslice);
> +	subslice--;
>  
>  	if (IS_GEN(dev_priv, 10))
>  		mcr_s_ss_select = GEN8_MCR_SLICE(slice) |

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

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

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

✓ Fi.CI.IGT: success for MCR fixes and more

[Patchwork]

== Series Details ==

Series: MCR fixes and more
URL   : https://patchwork.freedesktop.org/series/63831/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6502_full -> Patchwork_13676_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +4 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-apl1/igt@gem_ctx_isolation@rcs0-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-apl8/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108686])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-apl1/igt@gem_tiled_swapping@non-threaded.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-apl8/igt@gem_tiled_swapping@non-threaded.html

  * igt@gem_workarounds@suspend-resume-context:
    - shard-skl:          [PASS][5] -> [INCOMPLETE][6] ([fdo#104108]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-skl9/igt@gem_workarounds@suspend-resume-context.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-skl3/igt@gem_workarounds@suspend-resume-context.html

  * igt@i915_pm_rpm@system-suspend-execbuf:
    - shard-iclb:         [PASS][7] -> [INCOMPLETE][8] ([fdo#107713] / [fdo#108840])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-iclb2/igt@i915_pm_rpm@system-suspend-execbuf.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-iclb2/igt@i915_pm_rpm@system-suspend-execbuf.html

  * igt@i915_pm_rps@waitboost:
    - shard-skl:          [PASS][9] -> [FAIL][10] ([fdo#102250])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-skl9/igt@i915_pm_rps@waitboost.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-skl8/igt@i915_pm_rps@waitboost.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [PASS][11] -> [DMESG-WARN][12] ([fdo#108566]) +4 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-kbl1/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-kbl3/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][13] -> [FAIL][14] ([fdo#103167]) +4 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html

  * igt@kms_plane_cursor@pipe-c-viewport-size-128:
    - shard-iclb:         [PASS][15] -> [INCOMPLETE][16] ([fdo#107713])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-iclb1/igt@kms_plane_cursor@pipe-c-viewport-size-128.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-iclb7/igt@kms_plane_cursor@pipe-c-viewport-size-128.html

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries_display_off:
    - shard-iclb:         [DMESG-WARN][17] -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-iclb5/igt@debugfs_test@read_all_entries_display_off.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-iclb8/igt@debugfs_test@read_all_entries_display_off.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [DMESG-WARN][19] ([fdo#108686]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-glk5/igt@gem_tiled_swapping@non-threaded.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-glk5/igt@gem_tiled_swapping@non-threaded.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][21] ([fdo#108566]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-kbl3/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-b-cursor-128x42-random:
    - shard-skl:          [FAIL][23] ([fdo#103232]) -> [PASS][24] +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-skl10/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-skl2/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
    - shard-hsw:          [FAIL][25] ([fdo#105767]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-hsw8/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-hsw4/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          [FAIL][27] ([fdo#105363]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-glk7/igt@kms_flip@flip-vs-expired-vblank.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-glk9/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][29] ([fdo#105363]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-skl3/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-skl7/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [DMESG-WARN][31] ([fdo#108566]) -> [PASS][32] +3 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-apl8/igt@kms_flip@flip-vs-suspend-interruptible.html
    - shard-snb:          [INCOMPLETE][33] ([fdo#105411]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-snb1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-snb6/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
    - shard-iclb:         [FAIL][35] ([fdo#103167]) -> [PASS][36] +4 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][37] ([fdo#108145]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-skl3/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [FAIL][39] ([fdo#108145] / [fdo#110403]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-skl10/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         [SKIP][41] ([fdo#109441]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-iclb3/igt@kms_psr@psr2_dpms.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-iclb2/igt@kms_psr@psr2_dpms.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][43] ([fdo#99912]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/shard-apl2/igt@kms_setmode@basic.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/shard-apl4/igt@kms_setmode@basic.html

  
  [fdo#102250]: https://bugs.freedesktop.org/show_bug.cgi?id=102250
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


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

  * Linux: CI_DRM_6502 -> Patchwork_13676

  CI_DRM_6502: 606a844d5d932fb07b2377b95c0fe7b08383e32a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5102: 6038ace76016d8892f4d13aef5301f71ca1a6e2d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13676: d3068b3ac79ffaaa0ade8d8ef004e1f9fa2b59c6 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 1/6] drm/i915: Fix GEN8_MCR_SELECTOR programming

[Tvrtko Ursulin]

On 17/07/2019 22:25, Summers, Stuart wrote:
> On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Not opposed to this exactly, but do we really need this patch if we're
> just getting rid of this routine later in the series?

It just happens this fix alone is enough to fix MCR on all ICL parts I 
have seen (what CI has in BAT and in shards) and being a small one it is 
realistic to backport it to stable kernels. Sounds reasonable or I have 
missed something?

Regards,

Tvrtko

> Thanks,
> Stuart
> 
>>
>> fls returns bit positions starting from one for the lsb and the MCR
>> register expects zero based (sub)slice addressing.
>>
>> Incorrent MCR programming can have the effect of directing MMIO reads
>> of
>> registers in the 0xb100-0xb3ff range to invalid subslice returning
>> zeroes
>> instead of actual content.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement
>> WaProgramMgsrForCorrectSliceSpecificMmioReads")
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index c0bc9cb7f228..6f93caf7a5a1 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -962,9 +962,14 @@ const char *i915_cache_level_str(struct
>> drm_i915_private *i915, int type)
>>   u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private
>> *dev_priv)
>>   {
>>   	const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)-
>>> sseu;
>> +	unsigned int slice = fls(sseu->slice_mask) - 1;
>> +	unsigned int subslice;
>>   	u32 mcr_s_ss_select;
>> -	u32 slice = fls(sseu->slice_mask);
>> -	u32 subslice = fls(sseu->subslice_mask[slice]);
>> +
>> +	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
>> +	subslice = fls(sseu->subslice_mask[slice]);
>> +	GEM_BUG_ON(!subslice);
>> +	subslice--;
>>   
>>   	if (IS_GEN(dev_priv, 10))
>>   		mcr_s_ss_select = GEN8_MCR_SLICE(slice) |
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for MCR fixes and more

[Tvrtko Ursulin]

On 17/07/2019 20:18, Patchwork wrote:
> == Series Details ==
> 
> Series: MCR fixes and more
> URL   : https://patchwork.freedesktop.org/series/63831/
> State : success
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_6502 -> Patchwork_13676
> ====================================================
> 
> Summary
> -------
> 
>    **SUCCESS**
> 
>    No regressions found.
> 
>    External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/
> 
> Known issues
> ------------
> 
>    Here are the changes found in Patchwork_13676 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Possible fixes ####
> 
>    * igt@gem_basic@bad-close:
>      - fi-icl-u3:          [DMESG-WARN][1] ([fdo#107724]) -> [PASS][2]
>     [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-u3/igt@gem_basic@bad-close.html
>     [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-u3/igt@gem_basic@bad-close.html
> 
>    * igt@gem_ctx_create@basic-files:
>      - fi-icl-u3:          [INCOMPLETE][3] ([fdo#107713] / [fdo#109100]) -> [PASS][4]
>     [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-u3/igt@gem_ctx_create@basic-files.html
>     [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-u3/igt@gem_ctx_create@basic-files.html
> 
>    * {igt@gem_ctx_switch@legacy-render}:
>      - fi-icl-guc:         [INCOMPLETE][5] ([fdo#107713]) -> [PASS][6]
>     [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html
>     [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html
> 
>    * {igt@gem_ctx_switch@rcs0}:
>      - fi-icl-u2:          [INCOMPLETE][7] ([fdo#107713]) -> [PASS][8]
>     [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6502/fi-icl-u2/igt@gem_ctx_switch@rcs0.html
>     [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13676/fi-icl-u2/igt@gem_ctx_switch@rcs0.html
> 
>    
>    {name}: This element is suppressed. This means it is ignored when computing
>            the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>    [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
>    [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
>    [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
> 
> 
> Participating hosts (52 -> 47)
> ------------------------------
> 
>    Additional (1): fi-apl-guc
>    Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus
> 
> 
> Build changes
> -------------
> 
>    * Linux: CI_DRM_6502 -> Patchwork_13676
> 
>    CI_DRM_6502: 606a844d5d932fb07b2377b95c0fe7b08383e32a @ git://anongit.freedesktop.org/gfx-ci/linux
>    IGT_5102: 6038ace76016d8892f4d13aef5301f71ca1a6e2d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>    Patchwork_13676: d3068b3ac79ffaaa0ade8d8ef004e1f9fa2b59c6 @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> d3068b3ac79f drm/i915/icl: Add Wa_1409178092
> 69c8f4a77a62 drm/i915/icl: Verify engine workarounds in GEN8_L3SQCREG4
> 478276140729 drm/i915: Skip CS verification of L3 bank registers
> 05988d2693e3 drm/i915: Fix and improve MCR selection logic
> 9874e34931ae drm/i915: Trust programmed MCR in read_subslice_reg
> 53048f39ddd0 drm/i915: Fix GEN8_MCR_SELECTOR programming

Pushed since it fixes some obvious problem, even if it doesn't answer 
all the questions.

Regards,

Tvrtko


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

Re: [PATCH 1/6] drm/i915: Fix GEN8_MCR_SELECTOR programming

[Summers, Stuart]

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

On Thu, 2019-07-18 at 06:58 +0100, Tvrtko Ursulin wrote:
> On 17/07/2019 22:25, Summers, Stuart wrote:
> > On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Not opposed to this exactly, but do we really need this patch if
> > we're
> > just getting rid of this routine later in the series?
> 
> It just happens this fix alone is enough to fix MCR on all ICL parts
> I 
> have seen (what CI has in BAT and in shards) and being a small one it
> is 
> realistic to backport it to stable kernels. Sounds reasonable or I
> have 
> missed something?

Sorry for the delayed response here. Yes that sounds reasonable to me
too, thanks for the explanation.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

Thanks,
Stuart

> 
> Regards,
> 
> Tvrtko
> 
> > Thanks,
> > Stuart
> > 
> > > 
> > > fls returns bit positions starting from one for the lsb and the
> > > MCR
> > > register expects zero based (sub)slice addressing.
> > > 
> > > Incorrent MCR programming can have the effect of directing MMIO
> > > reads
> > > of
> > > registers in the 0xb100-0xb3ff range to invalid subslice
> > > returning
> > > zeroes
> > > instead of actual content.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement
> > > WaProgramMgsrForCorrectSliceSpecificMmioReads")
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > index c0bc9cb7f228..6f93caf7a5a1 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > @@ -962,9 +962,14 @@ const char *i915_cache_level_str(struct
> > > drm_i915_private *i915, int type)
> > >   u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private
> > > *dev_priv)
> > >   {
> > >   	const struct sseu_dev_info *sseu =
> > > &RUNTIME_INFO(dev_priv)-
> > > > sseu;
> > > 
> > > +	unsigned int slice = fls(sseu->slice_mask) - 1;
> > > +	unsigned int subslice;
> > >   	u32 mcr_s_ss_select;
> > > -	u32 slice = fls(sseu->slice_mask);
> > > -	u32 subslice = fls(sseu->subslice_mask[slice]);
> > > +
> > > +	GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
> > > +	subslice = fls(sseu->subslice_mask[slice]);
> > > +	GEM_BUG_ON(!subslice);
> > > +	subslice--;
> > >   
> > >   	if (IS_GEN(dev_priv, 10))
> > >   		mcr_s_ss_select = GEN8_MCR_SLICE(slice) |

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

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

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