Fix TLB invalidate issues with Broadwell

From: Chris Wilson <[email protected]>

As an extension of the current skip TLB invalidations, check if the device is powered down prior to any engine activity,

as, on such cases, all the TLBs were already invalidated, so an explicit TLB invalidation is not needed.

This becomes more significant with GuC, as it can only do so when the connection to the GuC is awake.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH 0/6] at: https://lore.kernel.org/all/[email protected]/

  drivers/gpu/drm/i915/gem/i915_gem_pages.c | 10 +++++----
  drivers/gpu/drm/i915/gt/intel_gt.c        | 26 +++++++++++++++++------
  drivers/gpu/drm/i915/gt/intel_gt_pm.h     |  3 +++
  3 files changed, 28 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/gem/i915_gem_pages.c +6 -4

index 97c820eee115..6835279943df 100644
@@ -6,14 +6,15 @@
 
  #include <drm/drm_cache.h>
 
+#include "gt/intel_gt.h"
+#include "gt/intel_gt_pm.h"
+
  #include "i915_drv.h"
  #include "i915_gem_object.h"
  #include "i915_scatterlist.h"
  #include "i915_gem_lmem.h"
  #include "i915_gem_mman.h"
 
-#include "gt/intel_gt.h"
-
  void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
  				 struct sg_table *pages,
  				 unsigned int sg_page_sizes)

@@ -217,10 +218,11 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 
  	if (test_and_clear_bit(I915_BO_WAS_BOUND_BIT, &obj->flags)) {
  		struct drm_i915_private *i915 = to_i915(obj->base.dev);
+		struct intel_gt *gt = to_gt(i915);
  		intel_wakeref_t wakeref;
 
-		with_intel_runtime_pm_if_active(&i915->runtime_pm, wakeref)
-			intel_gt_invalidate_tlbs(to_gt(i915));
+		with_intel_gt_pm_if_awake(gt, wakeref)
+			intel_gt_invalidate_tlbs(gt);
  	}
 
  	return pages;

drivers/gpu/drm/i915/gt/intel_gt.c +19 -7

index f33290358c51..d5ed6a6ac67c 100644
@@ -11,6 +11,7 @@
 
  #include "i915_drv.h"
  #include "intel_context.h"
+#include "intel_engine_pm.h"
  #include "intel_engine_regs.h"
  #include "intel_gt.h"
  #include "intel_gt_buffer_pool.h"

@@ -1216,6 +1217,7 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  	struct drm_i915_private *i915 = gt->i915;
  	struct intel_uncore *uncore = gt->uncore;
  	struct intel_engine_cs *engine;
+	intel_engine_mask_t awake, tmp;
  	enum intel_engine_id id;
  	const i915_reg_t *regs;
  	unsigned int num = 0;

@@ -1239,12 +1241,27 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
 
  	GEM_TRACE("\n");
 
-	assert_rpm_wakelock_held(&i915->runtime_pm);
-
  	mutex_lock(&gt->tlb_invalidate_lock);
  	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
+	awake = 0;
  	for_each_engine(engine, gt, id) {
+		struct reg_and_bit rb;
+
+		if (!intel_engine_pm_is_awake(engine))
+			continue;
+
+		rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
+		if (!i915_mmio_reg_offset(rb.reg))
+			continue;
+
+		intel_uncore_write_fw(uncore, rb.reg, rb.bit);
+		awake |= engine->mask;
+	}
+
+	for_each_engine_masked(engine, gt, awake, tmp) {
+		struct reg_and_bit rb;
+
  		/*
  		 * HW architecture suggest typical invalidation time at 40us,
  		 * with pessimistic cases up to 100us and a recommendation to

@@ -1252,13 +1269,8 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  		 */
  		const unsigned int timeout_us = 100;
  		const unsigned int timeout_ms = 4;
-		struct reg_and_bit rb;
 
  		rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
-		if (!i915_mmio_reg_offset(rb.reg))
-			continue;
-
-		intel_uncore_write_fw(uncore, rb.reg, rb.bit);
  		if (__intel_wait_for_register_fw(uncore,
  						 rb.reg, rb.bit, 0,
  						 timeout_us, timeout_ms,

drivers/gpu/drm/i915/gt/intel_gt_pm.h +3 -0

index bc898df7a48c..a334787a4939 100644
@@ -55,6 +55,9 @@ static inline void intel_gt_pm_might_put(struct intel_gt *gt)
  	for (tmp = 1, intel_gt_pm_get(gt); tmp; \
  	     intel_gt_pm_put(gt), tmp = 0)
 
+#define with_intel_gt_pm_if_awake(gt, wf) \
+	for (wf = intel_gt_pm_get_if_awake(gt); wf; intel_gt_pm_put_async(gt), wf = 0)
+
  static inline int intel_gt_pm_wait_for_idle(struct intel_gt *gt)
  {
  	return intel_wakeref_wait_for_idle(&gt->wakeref);

From: Chris Wilson <[email protected]>

As an extension of the current skip TLB invalidations, check if the device is powered down prior to any engine activity,

as, on such cases, all the TLBs were already invalidated, so an explicit TLB invalidation is not needed.

This becomes more significant with GuC, as it can only do so when the connection to the GuC is awake.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

From: Chris Wilson <[email protected]>

On gen12 HW, ensure that the TLB of the OA unit is also invalidated as just invalidating the TLB of an engine is not enough.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH 0/6] at: https://lore.kernel.org/all/[email protected]/

drivers/gpu/drm/i915/gt/intel_gt.c | 10 ++++++++++

1 file changed, 10 insertions(+)

drivers/gpu/drm/i915/gt/intel_gt.c +1 -0

index d5ed6a6ac67c..61b7ec5118f9 100644
@@ -10,6 +10,7 @@
 #include "pxp/intel_pxp.h"
 
 #include "i915_drv.h"
+#include "i915_perf_oa_regs.h"
 #include "intel_context.h"
 #include "intel_engine_pm.h"
 #include "intel_engine_regs.h"

@@ -1259,6 +1260,15 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
 		awake |= engine->mask;
 	}

drivers/gpu/drm/i915/gt/intel_gt.c +0 -0

 	for_each_engine_masked(engine, gt, awake, tmp) {
 		struct reg_and_bit rb;
 

Signature · 1 line

2.36.1

From: Chris Wilson <[email protected]>

On gen12 HW, ensure that the TLB of the OA unit is also invalidated as just invalidating the TLB of an engine is not enough.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

From: Chris Wilson <[email protected]>

Skip all further TLB invalidations once the device is wedged and had been reset, as, on such cases, it can no longer process instructions on the GPU and the user no longer has access to the TLB's in each engine.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH 0/6] at: https://lore.kernel.org/all/[email protected]/

  drivers/gpu/drm/i915/gt/intel_gt.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/gt/intel_gt.c +3 -0

index 61b7ec5118f9..fb4fd5273ca4 100644
@@ -1226,6 +1226,9 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  	if (I915_SELFTEST_ONLY(gt->awake == -ENODEV))
  		return;
 
+	if (intel_gt_is_wedged(gt))
+		return;
+
  	if (GRAPHICS_VER(i915) == 12) {
  		regs = gen12_regs;
  		num = ARRAY_SIZE(gen12_regs);

From: Chris Wilson <[email protected]>

Skip all further TLB invalidations once the device is wedged and had been reset, as, on such cases, it can no longer process instructions on the GPU and the user no longer has access to the TLB's in each engine.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH 0/6] at: https://lore.kernel.org/all/[email protected]/

 drivers/gpu/drm/i915/gt/intel_gt.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/gt/intel_gt.c +0 -0

index 61b7ec5118f9..fb4fd5273ca4 100644

From: Chris Wilson <[email protected]>

Don't flush TLBs when the buffer is only used in the GGTT under full control of the kernel, as there's no risk of of concurrent access and stale access from prefetch.

We only need to invalidate the TLB if they are accessible by the user.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH 0/6] at: https://lore.kernel.org/all/[email protected]/

  drivers/gpu/drm/i915/i915_vma.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_vma.c +2 -1

index 0bffb70b3c5f..7989986161e8 100644
@@ -537,7 +537,8 @@ int i915_vma_bind(struct i915_vma *vma,
  				   bind_flags);
  	}
 
-	set_bit(I915_BO_WAS_BOUND_BIT, &vma->obj->flags);
+	if (bind_flags & I915_VMA_LOCAL_BIND)
+		set_bit(I915_BO_WAS_BOUND_BIT, &vma->obj->flags);
 
  	atomic_or(bind_flags, &vma->flags);
  	return 0;

From: Chris Wilson <[email protected]>

Don't flush TLBs when the buffer is only used in the GGTT under full control of the kernel, as there's no risk of of concurrent access and stale access from prefetch.

We only need to invalidate the TLB if they are accessible by the user.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Signed-off-by Chris Wilson <[email protected]>

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

From: Chris Wilson <[email protected]>

Don't allow two engines to be reset in parallel, as they would both try to select a reset bit (and send requests to common registers) and wait on that register, at the same time. Serialize control of the reset requests/acks using the uncore->lock, which will also ensure that no other GT state changes at the same time as the actual reset.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mika Kuoppala <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Mika Kuoppala <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH 0/6] at: https://lore.kernel.org/all/[email protected]/

  drivers/gpu/drm/i915/gt/intel_reset.c | 37 ++++++++++++++++++++-------
  1 file changed, 28 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/gt/intel_reset.c +28 -9

index a5338c3fde7a..c68d36fb5bbd 100644
@@ -300,9 +300,9 @@ static int gen6_hw_domain_reset(struct intel_gt *gt, u32 hw_domain_mask)
  	return err;
  }
 
-static int gen6_reset_engines(struct intel_gt *gt,
-			      intel_engine_mask_t engine_mask,
-			      unsigned int retry)
+static int __gen6_reset_engines(struct intel_gt *gt,
+				intel_engine_mask_t engine_mask,
+				unsigned int retry)
  {
  	struct intel_engine_cs *engine;
  	u32 hw_mask;

@@ -321,6 +321,20 @@ static int gen6_reset_engines(struct intel_gt *gt,
  	return gen6_hw_domain_reset(gt, hw_mask);
  }
 
+static int gen6_reset_engines(struct intel_gt *gt,
+			      intel_engine_mask_t engine_mask,
+			      unsigned int retry)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&gt->uncore->lock, flags);
+	ret = __gen6_reset_engines(gt, engine_mask, retry);
+	spin_unlock_irqrestore(&gt->uncore->lock, flags);
+
+	return ret;
+}
+
  static struct intel_engine_cs *find_sfc_paired_vecs_engine(struct intel_engine_cs *engine)
  {
  	int vecs_id;

@@ -487,9 +501,9 @@ static void gen11_unlock_sfc(struct intel_engine_cs *engine)
  	rmw_clear_fw(uncore, sfc_lock.lock_reg, sfc_lock.lock_bit);
  }
 
-static int gen11_reset_engines(struct intel_gt *gt,
-			       intel_engine_mask_t engine_mask,
-			       unsigned int retry)
+static int __gen11_reset_engines(struct intel_gt *gt,
+				 intel_engine_mask_t engine_mask,
+				 unsigned int retry)
  {
  	struct intel_engine_cs *engine;
  	intel_engine_mask_t tmp;

@@ -583,8 +597,11 @@ static int gen8_reset_engines(struct intel_gt *gt,
  	struct intel_engine_cs *engine;
  	const bool reset_non_ready = retry >= 1;
  	intel_engine_mask_t tmp;
+	unsigned long flags;
  	int ret;
 
+	spin_lock_irqsave(&gt->uncore->lock, flags);
+
  	for_each_engine_masked(engine, gt, engine_mask, tmp) {
  		ret = gen8_engine_reset_prepare(engine);
  		if (ret && !reset_non_ready)

@@ -612,17 +629,19 @@ static int gen8_reset_engines(struct intel_gt *gt,
  	 * This is best effort, so ignore any error from the initial reset.
  	 */
  	if (IS_DG2(gt->i915) && engine_mask == ALL_ENGINES)
-		gen11_reset_engines(gt, gt->info.engine_mask, 0);
+		__gen11_reset_engines(gt, gt->info.engine_mask, 0);
 
  	if (GRAPHICS_VER(gt->i915) >= 11)
-		ret = gen11_reset_engines(gt, engine_mask, retry);
+		ret = __gen11_reset_engines(gt, engine_mask, retry);
  	else
-		ret = gen6_reset_engines(gt, engine_mask, retry);
+		ret = __gen6_reset_engines(gt, engine_mask, retry);
 
  skip_reset:
  	for_each_engine_masked(engine, gt, engine_mask, tmp)
  		gen8_engine_reset_cancel(engine);
 
+	spin_unlock_irqrestore(&gt->uncore->lock, flags);
+
  	return ret;
  }
 

From: Chris Wilson <[email protected]>

Don't allow two engines to be reset in parallel, as they would both try to select a reset bit (and send requests to common registers) and wait on that register, at the same time. Serialize control of the reset requests/acks using the uncore->lock, which will also ensure that no other GT state changes at the same time as the actual reset.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mika Kuoppala <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Mika Kuoppala <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Hi Mauro,

Quoted 10 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

Don't allow two engines to be reset in parallel, as they would both try to select a reset bit (and send requests to common registers) and wait on that register, at the same time. Serialize control of the reset requests/acks using the uncore->lock, which will also ensure that no other GT state changes at the same time as the actual reset.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Quoted 7 lines

Reported-by Mika Kuoppala <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Mika Kuoppala <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Andi Shyti <[email protected]>

Quoted 21 lines · Andi Shyti

Hi Mauro,

Quoted 17 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

Don't allow two engines to be reset in parallel, as they would both try to select a reset bit (and send requests to common registers) and wait on that register, at the same time. Serialize control of the reset requests/acks using the uncore->lock, which will also ensure that no other GT state changes at the same time as the actual reset.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mika Kuoppala <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Mika Kuoppala <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Quoted 1 line

Reviewed-by Andi Shyti <[email protected]>

Notice I had a bunch of questions and asks in this series so please do not merge until those are addressed.

In this particular patch (and some others) for instance Fixes: tag, at least against that sha, shouldn't be there.

Hi Tvrtko,

Quoted 27 lines · On Fri, 24 Jun 2022 09:34:21 +0100 Tvrtko Ursulin…

Quoted 21 lines · Andi Shyti

Hi Mauro,

Quoted 10 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

Don't allow two engines to be reset in parallel, as they would both try to select a reset bit (and send requests to common registers) and wait on that register, at the same time. Serialize control of the reset requests/acks using the uncore->lock, which will also ensure that no other GT state changes at the same time as the actual reset.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Quoted 7 lines

Reported-by Mika Kuoppala <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Mika Kuoppala <[email protected]>

Cc Andi Shyti <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Quoted 1 line

Reviewed-by Andi Shyti <[email protected]>

Notice I had a bunch of questions and asks in this series so please do not merge until those are addressed.

Quoted 2 lines

In this particular patch (and some others) for instance Fixes: tag, at least against that sha, shouldn't be there.

Hmm... I sent an answer to your points, but I can't see it at:

	https://lore.kernel.org/all/[email protected]/

Maybe it got lost somewhere, I dunno.

Yeah, indeed the fixes tag on patch 5/6 should be removed as this is not directly related to changeset 7938d61591d3. Yet, this one is required for patch 6 to work.

The other patches on this series, though, are modifying the code introduced by changeset 7938d61591d3.

Patch 2 is clearly a workaround needed for TLB cache invalidation to work on some GPUs. So, while not related to Broadwell, they're also fixing some TLB cache issues. So, IMO, it should keep the fixes.

I tried to port just the two serialize patches to drm-tip, in order to solve the issues on Broadwell, but it didn't work, as the logic inside the spinlock could be calling schedule() with a spinlock hold:

	Jun 14 17:38:48 silver kernel: [   23.227813] BUG: sleeping function called from invalid context at drivers/gpu/drm/i915/intel_uncore.c:2496
	Jun 14 17:38:48 silver kernel: [   23.227816] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 37, name: kworker/u8:1
	Jun 14 17:38:48 silver kernel: [   23.227818] preempt_count: 1, expected: 0
	Jun 14 17:38:48 silver kernel: [   23.227819] RCU nest depth: 0, expected: 0
	Jun 14 17:38:48 silver kernel: [   23.227820] 5 locks held by kworker/u8:1/37:
	Jun 14 17:38:48 silver kernel: [   23.227822]  #0: ffff88811159b538 ((wq_completion)i915){+.+.}-{0:0}, at: process_one_work+0x1e0/0x580
	Jun 14 17:38:48 silver kernel: [   23.227831]  #1: ffffc90000183e60 ((work_completion)(&(&i915->mm.free_work)->work)){+.+.}-{0:0}, at: process_one_work+0x1e0/0x580
	Jun 14 17:38:48 silver kernel: [   23.227837]  #2: ffff88811b34c5e8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: __i915_gem_free_objects+0xba/0x210 [i915]
	Jun 14 17:38:48 silver kernel: [   23.228283]  #3: ffff88810a66c2d8 (&gt->tlb_invalidate_lock){+.+.}-{3:3}, at: intel_gt_invalidate_tlbs+0xe7/0x4d0 [i915]
	Jun 14 17:38:48 silver kernel: [   23.228663]  #4: ffff88810a668f28 (&uncore->lock){-.-.}-{2:2}, at: intel_gt_invalidate_tlbs+0x115/0x4d0 [i915]

I didn't investigate the root cause, but it seems related to PM, so patches 1 and 3 seem to be required for the serialization logic to actually work.

So, I would keep the Fixes: tag mentioning changeset 7938d61591d3 on patches: 1, 2, 3 and 6.

Yet, IMO the entire series should be merged on -stable.

If that's OK for you and there's no additional issues to be addressed, I'll submit a v2 of this series removing the Fixes tag from patches 4 and 5.

Regards,
Mauro

.. which for me means a different patch 1, followed by patch 6 (moved to be patch 2) would be ideal stable material.

Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Could you please double check if what I am suggesting here is feasible to implement and if it is just send those minimal patches out alone?

Quoted 9 lines · On Tue, 28 Jun 2022 16:49:23 +0100 Tvrtko Ursulin…

.. which for me means a different patch 1, followed by patch 6 (moved to be patch 2) would be ideal stable material.

Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Quoted 2 lines

Could you please double check if what I am suggesting here is feasible to implement and if it is just send those minimal patches out alone?

Tested and porting just those 3 patches are enough to fix the Broadwell bug.

So, I submitted a v2 of this series with just those. They all need to be backported to stable.

I still think that other TLB patches are needed/desired upstream, but I'll submit them on a separate series. Let's fix the regression first ;-)

Quoted 12 lines

Quoted 9 lines · On Tue, 28 Jun 2022 16:49:23 +0100 Tvrtko Ursulin…

.. which for me means a different patch 1, followed by patch 6 (moved to be patch 2) would be ideal stable material.

Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Quoted 8 lines

Quoted 2 lines

Could you please double check if what I am suggesting here is feasible to implement and if it is just send those minimal patches out alone?

Tested and porting just those 3 patches are enough to fix the Broadwell bug.

So, I submitted a v2 of this series with just those. They all need to be backported to stable.

I would really like to give even a smaller fix a try. Something like, although not even compile tested:

commit 4d5e94aef164772f4d85b3b4c1a46eac9a2bd680

Author: Chris Wilson <[email protected]>
Date:   Wed Jun 29 16:25:24 2022 +0100

drm/i915/gt: Serialize TLB invalidates with GT resets

Avoid trying to invalidate the TLB in the middle of performing an engine reset, as this may result in the reset timing out. Currently, the TLB invalidate is only serialised by its own mutex, forgoing the uncore lock, but we can take the uncore->lock as well to serialise the mmio access, thereby serialising with the GDRST.

Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with i915 selftest/hangcheck.

Cc [email protected]

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mauro Carvalho Chehab <[email protected]>

Tested-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Acked-by Thomas Hellström <[email protected]>

Reviewed-by Andi Shyti <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

drivers/gpu/drm/i915/gt/intel_gt.c +16 -0

index 8da3314bb6bf..aaadd0b02043 100644
@@ -952,7 +952,23 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
         mutex_lock(&gt->tlb_invalidate_lock);
         intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
+       spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
+
+       for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
+               rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
+               if (!i915_mmio_reg_offset(rb.reg))
+                       continue;
+
+               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
+       }
+
+       spin_unlock_irq(&uncore->lock);
+
         for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
                 /*
                  * HW architecture suggest typical invalidation time at 40us,
                  * with pessimistic cases up to 100us and a recommendation to

@@ -960,13 +976,11 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
                  */
                 const unsigned int timeout_us = 100;
                 const unsigned int timeout_ms = 4;

If this works it would be least painful to backport. The other improvements can then be devoid of the fixes tag.

Quoted 2 lines

I still think that other TLB patches are needed/desired upstream, but I'll submit them on a separate series. Let's fix the regression first ;-)

Yep, that's exactly right.

Regards,

Tvrtko

Em Wed, 29 Jun 2022 17:02:59 +0100
Tvrtko Ursulin <[email protected]> escreveu:

Quoted 89 lines · Mauro Carvalho Chehab

Quoted 12 lines

Quoted 9 lines · On Tue, 28 Jun 2022 16:49:23 +0100 Tvrtko Ursulin…

.. which for me means a different patch 1, followed by patch 6 (moved to be patch 2) would be ideal stable material.

Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Quoted 8 lines

Quoted 2 lines

Could you please double check if what I am suggesting here is feasible to implement and if it is just send those minimal patches out alone?

Tested and porting just those 3 patches are enough to fix the Broadwell bug.

So, I submitted a v2 of this series with just those. They all need to be backported to stable.

I would really like to give even a smaller fix a try. Something like, although not even compile tested:

commit 4d5e94aef164772f4d85b3b4c1a46eac9a2bd680

Author: Chris Wilson <[email protected]>
Date:   Wed Jun 29 16:25:24 2022 +0100

drm/i915/gt: Serialize TLB invalidates with GT resets

Avoid trying to invalidate the TLB in the middle of performing an engine reset, as this may result in the reset timing out. Currently, the TLB invalidate is only serialised by its own mutex, forgoing the uncore lock, but we can take the uncore->lock as well to serialise the mmio access, thereby serialising with the GDRST.

Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with i915 selftest/hangcheck.

Cc [email protected]

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mauro Carvalho Chehab <[email protected]>

Tested-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Acked-by Thomas Hellström <[email protected]>

Reviewed-by Andi Shyti <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

drivers/gpu/drm/i915/gt/intel_gt.c +16 -1

index 8da3314bb6bf..aaadd0b02043 100644
@@ -952,7 +952,23 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
          mutex_lock(&gt->tlb_invalidate_lock);
          intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
+       spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
+
+       for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
+               rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
+               if (!i915_mmio_reg_offset(rb.reg))
+                       continue;
+
+               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
+       }
+
+       spin_unlock_irq(&uncore->lock);
+
          for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
                  /*
                   * HW architecture suggest typical invalidation time at 40us,
                   * with pessimistic cases up to 100us and a recommendation to

@@ -960,13 +976,11 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
                   */
                  const unsigned int timeout_us = 100;
                  const unsigned int timeout_ms = 4;
-               struct reg_and_bit rb;
 
                  rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
                  if (!i915_mmio_reg_offset(rb.reg))
                          continue;
 

drivers/gpu/drm/i915/gt/intel_gt.c void intel_gt_invalidate_tlbs(struct intel_gt *gt) · line 986

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_gt.c +0 -1

-               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
                  if (__intel_wait_for_register_fw(uncore,
                                                   rb.reg, rb.bit, 0,
                                                   timeout_us, timeout_ms,

This won't work, as it is not serializing TLB cache invalidation with i915 resets. Besides that, this is more or less merging patches 1 and 3,

placing patches with different rationales altogether. Upstream rule is to have one logical change per patch.

Quoted 1 line

If this works it would be least painful to backport. The other improvements can then be devoid of the fixes tag.

From backport PoV, it wouldn't make any difference applying one patch
or two. See, intel_gt_invalidate_tlbs() function doesn't exist before changeset 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store"), so, it shouldn't have merge conflicts while backporting it, maybe except if some functions it calls (or parameters) have changed. On such case, the backport fix should be trivial, and the end result of backporting one folded patch or two would be the same.

If any conflict happens, I can help doing the backports.

Quoted 8 lines

Quoted 2 lines

I still think that other TLB patches are needed/desired upstream, but I'll submit them on a separate series. Let's fix the regression first ;-)

Yep, that's exactly right.

Regards,

Tvrtko

Quoted 92 lines

Em Wed, 29 Jun 2022 17:02:59 +0100
Tvrtko Ursulin <[email protected]> escreveu:

Quoted 89 lines · Mauro Carvalho Chehab

Quoted 12 lines

Quoted 9 lines · On Tue, 28 Jun 2022 16:49:23 +0100 Tvrtko Ursulin…

.. which for me means a different patch 1, followed by patch 6 (moved to be patch 2) would be ideal stable material.

Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Quoted 8 lines

Quoted 2 lines

Could you please double check if what I am suggesting here is feasible to implement and if it is just send those minimal patches out alone?

Tested and porting just those 3 patches are enough to fix the Broadwell bug.

So, I submitted a v2 of this series with just those. They all need to be backported to stable.

I would really like to give even a smaller fix a try. Something like, although not even compile tested:

commit 4d5e94aef164772f4d85b3b4c1a46eac9a2bd680

Author: Chris Wilson <[email protected]>
Date:   Wed Jun 29 16:25:24 2022 +0100

drm/i915/gt: Serialize TLB invalidates with GT resets

Avoid trying to invalidate the TLB in the middle of performing an engine reset, as this may result in the reset timing out. Currently, the TLB invalidate is only serialised by its own mutex, forgoing the uncore lock, but we can take the uncore->lock as well to serialise the mmio access, thereby serialising with the GDRST.

Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with i915 selftest/hangcheck.

Cc [email protected]

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mauro Carvalho Chehab <[email protected]>

Tested-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Acked-by Thomas Hellström <[email protected]>

Reviewed-by Andi Shyti <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

drivers/gpu/drm/i915/gt/intel_gt.c +16 -1

index 8da3314bb6bf..aaadd0b02043 100644
@@ -952,7 +952,23 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
          mutex_lock(&gt->tlb_invalidate_lock);
          intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
+       spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
+
+       for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
+               rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
+               if (!i915_mmio_reg_offset(rb.reg))
+                       continue;
+
+               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
+       }
+
+       spin_unlock_irq(&uncore->lock);
+
          for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
                  /*
                   * HW architecture suggest typical invalidation time at 40us,
                   * with pessimistic cases up to 100us and a recommendation to

@@ -960,13 +976,11 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
                   */
                  const unsigned int timeout_us = 100;
                  const unsigned int timeout_ms = 4;
-               struct reg_and_bit rb;
 
                  rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
                  if (!i915_mmio_reg_offset(rb.reg))
                          continue;
 

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_gt.c void intel_gt_invalidate_tlbs(struct intel_gt *gt) · line 986

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_gt.c +0 -1

-               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
                  if (__intel_wait_for_register_fw(uncore,
                                                   rb.reg, rb.bit, 0,
                                                   timeout_us, timeout_ms,

This won't work, as it is not serializing TLB cache invalidation with i915 resets. Besides that, this is more or less merging patches 1 and 3,

Could you explain why you think it is not doing exactly that? In both versions end result is TLB flush requests are under the uncore lock and waits are outside it.

Quoted 2 lines

placing patches with different rationales altogether. Upstream rule is to have one logical change per patch.

I don't think it applies in this case. It is simply splitting into two loops so lock can be held across all mmio writes. I think of it this way
- what is the rationale for sending only the first patch to stable? What does it _fix_ on it's own?

Quoted 9 lines

If this works it would be least painful to backport. The other improvements can then be devoid of the fixes tag.

From backport PoV, it wouldn't make any difference applying one patch
or two. See, intel_gt_invalidate_tlbs() function doesn't exist before changeset 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store"), so, it shouldn't have merge conflicts while backporting it, maybe except if some functions it calls (or parameters) have changed. On such case, the backport fix should be trivial, and the end result of backporting one folded patch or two would be the same.

Yes a lot of things changed. Not least engine and GT pm code. Note that TLB flushing was backported all the way to 4.4 so any hunk you don't strictly need can and will bite you. I have attached a tarball of patches for you to explore. :)
Regards,

Em Thu, 30 Jun 2022 09:12:41 +0100
Tvrtko Ursulin <[email protected]> escreveu:

Quoted 102 lines · Mauro Carvalho Chehab

Quoted 100 lines

Em Wed, 29 Jun 2022 17:02:59 +0100
Tvrtko Ursulin <[email protected]> escreveu:

Quoted 84 lines · Mauro Carvalho Chehab

Quoted 12 lines

Quoted 9 lines · On Tue, 28 Jun 2022 16:49:23 +0100 Tvrtko Ursulin…

.. which for me means a different patch 1, followed by patch 6 (moved to be patch 2) would be ideal stable material.

Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Quoted 8 lines

Quoted 2 lines

Could you please double check if what I am suggesting here is feasible to implement and if it is just send those minimal patches out alone?

Tested and porting just those 3 patches are enough to fix the Broadwell bug.

So, I submitted a v2 of this series with just those. They all need to be backported to stable.

I would really like to give even a smaller fix a try. Something like, although not even compile tested:

commit 4d5e94aef164772f4d85b3b4c1a46eac9a2bd680

Author: Chris Wilson <[email protected]>
Date:   Wed Jun 29 16:25:24 2022 +0100

drm/i915/gt: Serialize TLB invalidates with GT resets

Avoid trying to invalidate the TLB in the middle of performing an engine reset, as this may result in the reset timing out. Currently, the TLB invalidate is only serialised by its own mutex, forgoing the uncore lock, but we can take the uncore->lock as well to serialise the mmio access, thereby serialising with the GDRST.

Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with i915 selftest/hangcheck.

Cc [email protected]

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mauro Carvalho Chehab <[email protected]>

Tested-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Acked-by Thomas Hellström <[email protected]>

Reviewed-by Andi Shyti <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

drivers/gpu/drm/i915/gt/intel_gt.c +16 -0

index 8da3314bb6bf..aaadd0b02043 100644
@@ -952,7 +952,23 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
           mutex_lock(&gt->tlb_invalidate_lock);
           intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
+       spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
+
+       for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
+               rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
+               if (!i915_mmio_reg_offset(rb.reg))
+                       continue;
+
+               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
+       }
+
+       spin_unlock_irq(&uncore->lock);
+
           for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
                   /*
                    * HW architecture suggest typical invalidation time at 40us,
                    * with pessimistic cases up to 100us and a recommendation to

@@ -960,13 +976,11 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
                    */
                   const unsigned int timeout_us = 100;
                   const unsigned int timeout_ms = 4;

drivers/gpu/drm/i915/gt/intel_gt.c void intel_gt_invalidate_tlbs(struct intel_gt *gt) · line 986

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_gt.c +0 -2

-               struct reg_and_bit rb;
 
                   rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
                   if (!i915_mmio_reg_offset(rb.reg))
                           continue;
 
-               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
                   if (__intel_wait_for_register_fw(uncore,
                                                    rb.reg, rb.bit, 0,
                                                    timeout_us, timeout_ms,

This won't work, as it is not serializing TLB cache invalidation with i915 resets. Besides that, this is more or less merging patches 1 and 3,

Quoted 3 lines

Could you explain why you think it is not doing exactly that? In both versions end result is TLB flush requests are under the uncore lock and waits are outside it.

Sure, but patch 2/3 (see v2) serializes i915 reset with TLB cache changes. This is needed in order to fix the regression.

Quoted 7 lines

Quoted 2 lines

placing patches with different rationales altogether. Upstream rule is to have one logical change per patch.

I don't think it applies in this case. It is simply splitting into two loops so lock can be held across all mmio writes. I think of it this way
- what is the rationale for sending only the first patch to stable? What does it _fix_ on it's own?

There's no -stable rule enforcing that only one patch would be allowed, nor saying that patches should be fold, doing multiple changes on as single patch just due to "Fixes" tag.

So, while several -stable fixes can be done on a single patch, there are fixes that will require multiple patches. That's nothing wrong with that.

The only rule is that backports should follow what's merged upstream. So, if, in order to fix a regression, multiple patches are needed upstream, in principle, all of those can be backported if they fit at -stable rules.

As an example, once we backported a patch series on media that had ~20 patches, addressing security issues at the media compat32 logic (media ioctls usually pass structs and some with pointers). As the issue was discovered several years after compat32 got introduced, those 22 patches (some containing compat32 redesigns) had to be backported to all maintained LTS.

-

In this specific case, fixing the regression requires 3 logical changes:

1) Split the loop;
2) Add serialize logic to i915 reset;
3) use the same i915 reset spinlock to serialize TLB cache invalidation.

Neither one of those logical changes alone would solve the issue. That's why I originally added the same Fixes: to the entire series: basically, any Kernel that has the TLB patch backported will require those three logical changes to be backported too.

That basically will follow what's there at the Kernel process docs:

"If your patch fixes a bug in a specific commit, e.g. you found an issue using
``git bisect``, please use the 'Fixes:' tag with the first 12 characters of the SHA-1 ID, and the one line summary."

Documentation/process/submitting-patches.rst

See, Fixes was originally introduced to be a hint to help stable and distro maintainers to identify how far they need to backport a patch. That's mainly why I placed fixes to the entire series. Yet, the same will also happen, in practice, if we place:

Cc [email protected] # Up to version 4.4

Greg, Sasha and others -stable/distro maintainers will also have a (much less precise) hint about how far the backport is needed.

Quoted 10 lines

Quoted 9 lines

If this works it would be least painful to backport. The other improvements can then be devoid of the fixes tag.

From backport PoV, it wouldn't make any difference applying one patch
or two. See, intel_gt_invalidate_tlbs() function doesn't exist before changeset 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store"), so, it shouldn't have merge conflicts while backporting it, maybe except if some functions it calls (or parameters) have changed. On such case, the backport fix should be trivial, and the end result of backporting one folded patch or two would be the same.

Quoted 5 lines

Yes a lot of things changed. Not least engine and GT pm code. Note that TLB flushing was backported all the way to 4.4 so any hunk you don't strictly need can and will bite you. I have attached a tarball of patches for you to explore. :)
Regards,

Thanks! That's very helpful to check the amount of work. It makes easy to use interdiff and (k)diff3 to check what changed.

From it, the differences between 5.4 and 5.16 at intel_gt_invalidate_tlbs()
are really trivial.

On 4.14, the function was added on a different file (intel_gem), and there were a few more API differences, as only gen8 code is there, but again, the changes are trivial: mostly macros/functions were renamed and some function parameters changed.

From 4.9 to 4.14 there were also some changes but they also look trivial.

Kernel 4.4 has some other differences - the loop logic is different, and there's a ring initialization function, but, as version 4.4 is not listed anymore as LTS at kernel.org, we probably need to backport only up to 4.9.

All the above should be affecting patch v2 1/3. Patches v2 2/3 and 3/3 just have spin lock/unlock for the gt uncore spinlock. Those will very likely require some work on Kernels 4.x, but folding (or not) the patches won't really help.

Quoted 101 lines · Mauro Carvalho Chehab

Em Thu, 30 Jun 2022 09:12:41 +0100
Tvrtko Ursulin <[email protected]> escreveu:

Quoted 98 lines · Mauro Carvalho Chehab

Quoted 97 lines

Em Wed, 29 Jun 2022 17:02:59 +0100
Tvrtko Ursulin <[email protected]> escreveu:

Quoted 94 lines · Mauro Carvalho Chehab

Quoted 12 lines

Quoted 9 lines · On Tue, 28 Jun 2022 16:49:23 +0100 Tvrtko Ursulin…

.. which for me means a different patch 1, followed by patch 6 (moved to be patch 2) would be ideal stable material.

Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Quoted 8 lines

Quoted 2 lines

Could you please double check if what I am suggesting here is feasible to implement and if it is just send those minimal patches out alone?

Tested and porting just those 3 patches are enough to fix the Broadwell bug.

So, I submitted a v2 of this series with just those. They all need to be backported to stable.

I would really like to give even a smaller fix a try. Something like, although not even compile tested:

commit 4d5e94aef164772f4d85b3b4c1a46eac9a2bd680

Author: Chris Wilson <[email protected]>
Date:   Wed Jun 29 16:25:24 2022 +0100

drm/i915/gt: Serialize TLB invalidates with GT resets

Avoid trying to invalidate the TLB in the middle of performing an engine reset, as this may result in the reset timing out. Currently, the TLB invalidate is only serialised by its own mutex, forgoing the uncore lock, but we can take the uncore->lock as well to serialise the mmio access, thereby serialising with the GDRST.

Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with i915 selftest/hangcheck.

Cc [email protected]

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mauro Carvalho Chehab <[email protected]>

Tested-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Acked-by Thomas Hellström <[email protected]>

Reviewed-by Andi Shyti <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

drivers/gpu/drm/i915/gt/intel_gt.c +16 -2

index 8da3314bb6bf..aaadd0b02043 100644
@@ -952,7 +952,23 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
           mutex_lock(&gt->tlb_invalidate_lock);
           intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
+       spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
+
+       for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
+               rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
+               if (!i915_mmio_reg_offset(rb.reg))
+                       continue;
+
+               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
+       }
+
+       spin_unlock_irq(&uncore->lock);
+
           for_each_engine(engine, gt, id) {
+               struct reg_and_bit rb;
+
                   /*
                    * HW architecture suggest typical invalidation time at 40us,
                    * with pessimistic cases up to 100us and a recommendation to

@@ -960,13 +976,11 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
                    */
                   const unsigned int timeout_us = 100;
                   const unsigned int timeout_ms = 4;
-               struct reg_and_bit rb;
 
                   rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
                   if (!i915_mmio_reg_offset(rb.reg))
                           continue;
 
-               intel_uncore_write_fw(uncore, rb.reg, rb.bit);
                   if (__intel_wait_for_register_fw(uncore,
                                                    rb.reg, rb.bit, 0,
                                                    timeout_us, timeout_ms,

What about intel_engine_pm_is_awake, what will you do with that one?

From: Chris Wilson <[email protected]>

Avoid trying to invalidate the TLB in the middle of performing an engine reset, as this may result in the reset timing out. Currently, the TLB invalidate is only serialised by its own mutex, forgoing the uncore lock, but we can take the uncore->lock as well to serialise the mmio access, thereby serialising with the GDRST.

Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with i915 selftest/hangcheck.

Fixes 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by Mauro Carvalho Chehab <[email protected]>

Tested-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc [email protected]

Acked-by Thomas Hellström <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>