Fix performance regressions with TLB and add GuC support

From: Chris Wilson <[email protected]>

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, thus reducing the performance regression impact due to it.

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

Cc [email protected]

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 Thomas Hellström <[email protected]>

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] 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        | 17 ++++++++++-------
  drivers/gpu/drm/i915/gt/intel_gt_pm.h     |  3 +++
  3 files changed, 19 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 +10 -7

index 68c2b0d8f187..c4d43da84d8e 100644
@@ -12,6 +12,7 @@
 
  #include "i915_drv.h"
  #include "intel_context.h"
+#include "intel_engine_pm.h"
  #include "intel_engine_regs.h"
  #include "intel_ggtt_gmch.h"
  #include "intel_gt.h"

@@ -924,6 +925,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;

@@ -947,26 +949,31 @@ 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);
 
  	spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
 
+	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;
  	}
 
  	spin_unlock_irq(&uncore->lock);
 
-	for_each_engine(engine, gt, id) {
+	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

@@ -974,12 +981,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;
-
  		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);

Quoted 7 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

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, thus reducing the performance regression impact due to it.

Quoted 5 lines

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

Cc [email protected]

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

Patch itself looks fine but I don't think we closed on the issue of stable/fixes on this patch?

My position here is that, if the functional issue is only with GuC invalidations, then the tags shouldn't be there (and the huge CC list).

Regards,

Tvrtko

Quoted 133 lines

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

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc Thomas Hellström <[email protected]>

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] 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        | 17 ++++++++++-------
  drivers/gpu/drm/i915/gt/intel_gt_pm.h     |  3 +++
  3 files changed, 19 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 +10 -7

index 68c2b0d8f187..c4d43da84d8e 100644
@@ -12,6 +12,7 @@
 
  #include "i915_drv.h"
  #include "intel_context.h"
+#include "intel_engine_pm.h"
  #include "intel_engine_regs.h"
  #include "intel_ggtt_gmch.h"
  #include "intel_gt.h"

@@ -924,6 +925,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;

@@ -947,26 +949,31 @@ 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);
 
  	spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
 
+	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;
  	}
 
  	spin_unlock_irq(&uncore->lock);
 
-	for_each_engine(engine, gt, id) {
+	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

@@ -974,12 +981,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;
-
  		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);  

Quoted 8 lines · On Mon, 18 Jul 2022 14:16:10 +0100 Tvrtko Ursulin…

Quoted 7 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

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, thus reducing the performance regression impact due to it.

Quoted 8 lines

Quoted 5 lines

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

Cc [email protected]

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

Patch itself looks fine but I don't think we closed on the issue of stable/fixes on this patch?

No, because TLB cache invalidation takes time and causes time outs, which in turn affects applications and produce Kernel warnings.

There's even open bugs due to TLB timeouts, like this one:

	[424.370996] i915 0000:00:02.0: [drm] *ERROR* rcs0 TLB invalidation did not complete in 4ms!

See:

	https://gitlab.freedesktop.org/drm/intel/-/issues/6424

So, while this is a performance regression, it ends causing a functional regression.

The first part of this series (patches 1-7) are meant to reduce the risk of such timeouts by doing TLB invalidation in batch and only when really needed (userspace-exposed TLBs for GTs that are powered-on and non-edged).

As they're fixing such regressions, it makes sense c/c stable and having a fixes tag.

Quoted 140 lines

My position here is that, if the functional issue is only with GuC invalidations, then the tags shouldn't be there (and the huge CC list).

Regards,

Tvrtko

Quoted 133 lines

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

Cc Fei Yang <[email protected]>

Cc Andi Shyti <[email protected]>

Cc Thomas Hellström <[email protected]>

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] 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        | 17 ++++++++++-------
   drivers/gpu/drm/i915/gt/intel_gt_pm.h     |  3 +++
   3 files changed, 19 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 +10 -7

index 68c2b0d8f187..c4d43da84d8e 100644
@@ -12,6 +12,7 @@
 
   #include "i915_drv.h"
   #include "intel_context.h"
+#include "intel_engine_pm.h"
   #include "intel_engine_regs.h"
   #include "intel_ggtt_gmch.h"
   #include "intel_gt.h"

@@ -924,6 +925,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;

@@ -947,26 +949,31 @@ 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);
 
   	spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
 
+	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;
   	}
 
   	spin_unlock_irq(&uncore->lock);
 
-	for_each_engine(engine, gt, id) {
+	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

@@ -974,12 +981,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;
-
   		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);

Sent: 18 July 2022 15:54

Quoted 11 lines · On Mon, 18 Jul 2022 14:16:10 +0100 Tvrtko Ursulin…

Quoted 10 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

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, thus reducing the performance regression impact due to it.

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

Quoted 5 lines

Quoted 2 lines

Cc [email protected]

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

Patch itself looks fine but I don't think we closed on the issue of stable/fixes on this patch?

No, because TLB cache invalidation takes time and causes time outs, which in turn affects applications and produce Kernel warnings.

From: Mauro Carvalho Chehab

Quoted 16 lines

Sent: 18 July 2022 15:54

Quoted 11 lines · On Mon, 18 Jul 2022 14:16:10 +0100 Tvrtko Ursulin…

Quoted 10 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

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, thus reducing the performance regression impact due to it.

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

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Cc [email protected]

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

Patch itself looks fine but I don't think we closed on the issue of stable/fixes on this patch?

No, because TLB cache invalidation takes time and causes time outs, which in turn affects applications and produce Kernel warnings.

It's not only the TLB flushes that cause grief.

There is a loop that forces a write-back of all the frame buffer pages. With a large display and some cpu (like my Ivy bridge one) that takes long enough with pre-emption disabled that wakeup of RT processes (and any pinned to the cpu) takes far longer than one might have wished for.

Since some X servers request a flush every few seconds this makes the system unusable for some workloads.

Sent: 19 July 2022 08:25

Quoted 8 lines

It's not only the TLB flushes that cause grief.

There is a loop that forces a write-back of all the frame buffer pages. With a large display and some cpu (like my Ivy bridge one) that takes long enough with pre-emption disabled that wakeup of RT processes (and any pinned to the cpu) takes far longer than one might have wished for.

Quoted 2 lines

Since some X servers request a flush every few seconds this makes the system unusable for some workloads.

Ok TLB invalidations as discussed in this patch does not apply to Ivybridge. But what is the write back loop you mention which is causing you grief? What size frame buffers are we talking about here? If they don't fit in the mappable area recently we merged a patch* which improves things in that situation but not sure you are hitting exactly that.

From: Chris Wilson <[email protected]>

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, thus reducing the performance regression impact due to it.

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

Cc [email protected]

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 Thomas Hellström <[email protected]>

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] 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        | 17 ++++++++++-------
 drivers/gpu/drm/i915/gt/intel_gt_pm.h     |  3 +++
 3 files changed, 19 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 +10 -7

index 68c2b0d8f187..c4d43da84d8e 100644
@@ -12,6 +12,7 @@
 
 #include "i915_drv.h"
 #include "intel_context.h"
+#include "intel_engine_pm.h"
 #include "intel_engine_regs.h"
 #include "intel_ggtt_gmch.h"
 #include "intel_gt.h"

@@ -924,6 +925,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;

@@ -947,26 +949,31 @@ 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);
 
 	spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
 
+	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;
 	}
 
 	spin_unlock_irq(&uncore->lock);
 
-	for_each_engine(engine, gt, id) {
+	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

@@ -974,12 +981,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;
-
 		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);

Signature · 1 line

2.36.1

Add a kernel-doc markup to document this new macro.

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] at: https://lore.kernel.org/all/[email protected]/

  drivers/gpu/drm/i915/gt/intel_gt_pm.h | 7 +++++++
  1 file changed, 7 insertions(+)

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

index a334787a4939..4d4caf612fdc 100644
@@ -55,6 +55,13 @@ 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)

From: Chris Wilson <[email protected]>

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

Cc [email protected]

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]>

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

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] 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 +10 -0

index c4d43da84d8e..1d84418e8676 100644
@@ -11,6 +11,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"

@@ -969,6 +970,15 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  		awake |= engine->mask;
  	}
 
+	/* Wa_2207587034:tgl,dg1,rkl,adl-s,adl-p */
+	if (awake &&
+	    (IS_TIGERLAKE(i915) ||
+	     IS_DG1(i915) ||
+	     IS_ROCKETLAKE(i915) ||
+	     IS_ALDERLAKE_S(i915) ||
+	     IS_ALDERLAKE_P(i915)))
+		intel_uncore_write_fw(uncore, GEN12_OA_TLB_INV_CR, 1);
+
  	spin_unlock_irq(&uncore->lock);
 
  	for_each_engine_masked(engine, gt, awake, tmp) {

From: Chris Wilson <[email protected]>

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

Cc [email protected]

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]>

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

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

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 concurrent access and stale access from prefetch.

We only need to invalidate the TLB if they are accessible by the user. That helps to reduce the performance regression introduced by TLB invalidate logic.

Cc [email protected]

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]>

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

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] 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 ef3b04c7e153..646f419b2035 100644
@@ -538,7 +538,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;

Quoted 6 lines · Mauro Carvalho Chehab

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 concurrent access and stale access from prefetch.

Quoted 6 lines

We only need to invalidate the TLB if they are accessible by the user. That helps to reduce the performance regression introduced by TLB invalidate logic.

Cc [email protected]

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

Do we really need or want stable and fixes on this one?

What do we think the performance improvement is, given there's very little in GGTT, which is not mapped via PPGTT as well?

I think it is safe, but part of me would ideally not even want to think about whether it is safe, if the performance improvement is non-existent. Which I can't imagine how there would be?

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 concurrent access and stale access from prefetch.

We only need to invalidate the TLB if they are accessible by the user. That helps to reduce the performance regression introduced by TLB invalidate logic.

Cc [email protected]

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]>

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.

That helps to reduce the performance regression introduced by TLB invalidate logic.

Cc [email protected]

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]>

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

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] 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 1d84418e8676..5c55a90672f4 100644
@@ -934,6 +934,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);

Quoted 9 lines · Mauro Carvalho Chehab

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.

That helps to reduce the performance regression introduced by TLB invalidate logic.

Quoted 2 lines

Cc [email protected]

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

Is the claim of a performance regression this solved based on a wedged GPU which does not work any more to the extend where mmio tlb invalidation requests keep timing out? If so please clarify in the commit text and then it looks good to me. Even if it is IMO a very borderline situation to declare something a fix.

Quoted 10 lines · On Mon, 18 Jul 2022 14:45:22 +0100 Tvrtko Ursulin…

Quoted 9 lines · Mauro Carvalho Chehab

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.

That helps to reduce the performance regression introduced by TLB invalidate logic.

Quoted 8 lines

Quoted 2 lines

Cc [email protected]

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

Is the claim of a performance regression this solved based on a wedged GPU which does not work any more to the extend where mmio tlb invalidation requests keep timing out? If so please clarify in the commit text and then it looks good to me. Even if it is IMO a very borderline situation to declare something a fix.

Indeed this helps on a borderline situation: if GT is wedged, TLB invalidation will timeout, so it makes sense to keep the patch with a comment like:

drm/i915/gt: Skip TLB invalidations once wedged

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.

So, an attempt to do a TLB cache invalidation will produce a timeout.

That helps to reduce the performance regression introduced by TLB invalidate logic.

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.

That helps to reduce the performance regression introduced by TLB invalidate logic.

Cc [email protected]

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]>

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

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

From: Chris Wilson <[email protected]>

Invalidate TLB in patch, in order to reduce performance regressions.

Currently, every caller performs a full barrier around a TLB invalidation, ignoring all other invalidations that may have already removed their PTEs from the cache. As this is a synchronous operation and can be quite slow, we cause multiple threads to contend on the TLB invalidate mutex blocking userspace.

We only need to invalidate the TLB once after replacing our PTE to ensure that there is no possible continued access to the physical address before releasing our pages. By tracking a seqno for each full TLB invalidate we can quickly determine if one has been performed since rewriting the PTE, and only if necessary trigger one for ourselves.

That helps to reduce the performance regression introduced by TLB invalidate logic.

[mchehab: rebased to not require moving the code to a separate file]

Cc [email protected]

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

Suggested-by Tvrtko Ursulin <[email protected]>

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

Cc Fei Yang <[email protected]>

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] at: https://lore.kernel.org/all/[email protected]/

  .../gpu/drm/i915/gem/i915_gem_object_types.h  |  3 +-
  drivers/gpu/drm/i915/gem/i915_gem_pages.c     | 21 +++++---
  drivers/gpu/drm/i915/gt/intel_gt.c            | 53 ++++++++++++++-----
  drivers/gpu/drm/i915/gt/intel_gt.h            | 12 ++++-
  drivers/gpu/drm/i915/gt/intel_gt_types.h      | 18 ++++++-
  drivers/gpu/drm/i915/gt/intel_ppgtt.c         |  8 ++-
  drivers/gpu/drm/i915/i915_vma.c               | 34 +++++++++---
  drivers/gpu/drm/i915/i915_vma.h               |  1 +
  drivers/gpu/drm/i915/i915_vma_resource.c      |  5 +-
  drivers/gpu/drm/i915/i915_vma_resource.h      |  6 ++-
  10 files changed, 125 insertions(+), 36 deletions(-)

drivers/gpu/drm/i915/gem/i915_gem_object_types.h +2 -1

index 5cf36a130061..9f6b14ec189a 100644
@@ -335,7 +335,6 @@ struct drm_i915_gem_object {
  #define I915_BO_READONLY          BIT(7)
  #define I915_TILING_QUIRK_BIT     8 /* unknown swizzling; do not release! */
  #define I915_BO_PROTECTED         BIT(9)
-#define I915_BO_WAS_BOUND_BIT     10
  	/**
  	 * @mem_flags - Mutable placement-related flags
  	 *

@@ -616,6 +615,8 @@ struct drm_i915_gem_object {
  		 * pages were last acquired.
  		 */
  		bool dirty:1;
+
+		u32 tlb;
  	} mm;
 
  	struct {

drivers/gpu/drm/i915/gem/i915_gem_pages.c +13 -8

index 6835279943df..8357dbdcab5c 100644
@@ -191,6 +191,18 @@ static void unmap_object(struct drm_i915_gem_object *obj, void *ptr)
  		vunmap(ptr);
  }
 
+static void flush_tlb_invalidate(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct intel_gt *gt = to_gt(i915);
+
+	if (!obj->mm.tlb)
+		return;
+
+	intel_gt_invalidate_tlb(gt, obj->mm.tlb);
+	obj->mm.tlb = 0;
+}
+
  struct sg_table *
  __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
  {

@@ -216,14 +228,7 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
  	__i915_gem_object_reset_page_iter(obj);
  	obj->mm.page_sizes.phys = obj->mm.page_sizes.sg = 0;
 
-	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_gt_pm_if_awake(gt, wakeref)
-			intel_gt_invalidate_tlbs(gt);
-	}
+	flush_tlb_invalidate(obj);
 
  	return pages;
  }

drivers/gpu/drm/i915/gt/intel_gt.c +40 -13

index 5c55a90672f4..f435e06125aa 100644
@@ -38,8 +38,6 @@ static void __intel_gt_init_early(struct intel_gt *gt)
  {
  	spin_lock_init(&gt->irq_lock);
 
-	mutex_init(&gt->tlb_invalidate_lock);
-
  	INIT_LIST_HEAD(&gt->closed_vma);
  	spin_lock_init(&gt->closed_lock);
 

@@ -50,6 +48,8 @@ static void __intel_gt_init_early(struct intel_gt *gt)
  	intel_gt_init_reset(gt);
  	intel_gt_init_requests(gt);
  	intel_gt_init_timelines(gt);
+	mutex_init(&gt->tlb.invalidate_lock);
+	seqcount_mutex_init(&gt->tlb.seqno, &gt->tlb.invalidate_lock);
  	intel_gt_pm_init_early(gt);
 
  	intel_uc_init_early(&gt->uc);

@@ -770,6 +770,7 @@ void intel_gt_driver_late_release_all(struct drm_i915_private *i915)
  		intel_gt_fini_requests(gt);
  		intel_gt_fini_reset(gt);
  		intel_gt_fini_timelines(gt);
+		mutex_destroy(&gt->tlb.invalidate_lock);
  		intel_engines_free(gt);
  	}
  }

@@ -908,7 +909,7 @@ get_reg_and_bit(const struct intel_engine_cs *engine, const bool gen8,
  	return rb;
  }
 
-void intel_gt_invalidate_tlbs(struct intel_gt *gt)
+static void mmio_invalidate_full(struct intel_gt *gt)
  {
  	static const i915_reg_t gen8_regs[] = {
  		[RENDER_CLASS]			= GEN8_RTCR,

@@ -931,12 +932,6 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  	const i915_reg_t *regs;
  	unsigned int num = 0;
 
-	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);

@@ -951,9 +946,6 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  			  "Platform does not implement TLB invalidation!"))
  		return;
 
-	GEM_TRACE("\n");
-
-	mutex_lock(&gt->tlb_invalidate_lock);
  	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
  	spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */

@@ -973,6 +965,8 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  		awake |= engine->mask;
  	}
 
+	GT_TRACE(gt, "invalidated engines %08x\n", awake);
+
  	/* Wa_2207587034:tgl,dg1,rkl,adl-s,adl-p */
  	if (awake &&
  	    (IS_TIGERLAKE(i915) ||

@@ -1012,5 +1006,38 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  	 * transitions.
  	 */
  	intel_uncore_forcewake_put_delayed(uncore, FORCEWAKE_ALL);
-	mutex_unlock(&gt->tlb_invalidate_lock);
+}
+
+static bool tlb_seqno_passed(const struct intel_gt *gt, u32 seqno)
+{
+	u32 cur = intel_gt_tlb_seqno(gt);
+
+	/* Only skip if a *full* TLB invalidate barrier has passed */
+	return (s32)(cur - ALIGN(seqno, 2)) > 0;
+}
+
+void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno)
+{
+	intel_wakeref_t wakeref;
+
+	if (I915_SELFTEST_ONLY(gt->awake == -ENODEV))
+		return;
+
+	if (intel_gt_is_wedged(gt))
+		return;
+
+	if (tlb_seqno_passed(gt, seqno))
+		return;
+
+	with_intel_gt_pm_if_awake(gt, wakeref) {
+		mutex_lock(&gt->tlb.invalidate_lock);
+		if (tlb_seqno_passed(gt, seqno))
+			goto unlock;
+
+		mmio_invalidate_full(gt);
+
+		write_seqcount_invalidate(&gt->tlb.seqno);
+unlock:
+		mutex_unlock(&gt->tlb.invalidate_lock);
+	}
  }

drivers/gpu/drm/i915/gt/intel_gt.h +11 -1

index 82d6f248d876..40b06adf509a 100644
@@ -101,6 +101,16 @@ void intel_gt_info_print(const struct intel_gt_info *info,
 
  void intel_gt_watchdog_work(struct work_struct *work);
 
-void intel_gt_invalidate_tlbs(struct intel_gt *gt);
+static inline u32 intel_gt_tlb_seqno(const struct intel_gt *gt)
+{
+	return seqprop_sequence(&gt->tlb.seqno);
+}
+
+static inline u32 intel_gt_next_invalidate_tlb_full(const struct intel_gt *gt)
+{
+	return intel_gt_tlb_seqno(gt) | 1;
+}
+
+void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno);
 
  #endif /* __INTEL_GT_H__ */

drivers/gpu/drm/i915/gt/intel_gt_types.h +17 -1

index df708802889d..3804a583382b 100644
@@ -11,6 +11,7 @@
  #include <linux/llist.h>
  #include <linux/mutex.h>
  #include <linux/notifier.h>
+#include <linux/seqlock.h>
  #include <linux/spinlock.h>
  #include <linux/types.h>
  #include <linux/workqueue.h>

@@ -83,7 +84,22 @@ struct intel_gt {
  	struct intel_uc uc;
  	struct intel_gsc gsc;
 
-	struct mutex tlb_invalidate_lock;
+	struct {
+		/* Serialize global tlb invalidations */
+		struct mutex invalidate_lock;
+
+		/*
+		 * Batch TLB invalidations
+		 *
+		 * After unbinding the PTE, we need to ensure the TLB
+		 * are invalidated prior to releasing the physical pages.
+		 * But we only need one such invalidation for all unbinds,
+		 * so we track how many TLB invalidations have been
+		 * performed since unbind the PTE and only emit an extra
+		 * invalidate if no full barrier has been passed.
+		 */
+		seqcount_mutex_t seqno;
+	} tlb;
 
  	struct i915_wa_list wa_list;
 

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

index d8b94d638559..2da6c82a8bd2 100644
@@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
  void ppgtt_unbind_vma(struct i915_address_space *vm,

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

  }
 
  static unsigned long pd_count(u64 size, int shift)

drivers/gpu/drm/i915/i915_vma.c +26 -8

index 646f419b2035..84a9ccbc5fc5 100644
@@ -538,9 +538,6 @@ int i915_vma_bind(struct i915_vma *vma,
  				   bind_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;
  }

@@ -1311,6 +1308,19 @@ I915_SELFTEST_EXPORT int i915_vma_get_pages(struct i915_vma *vma)
  	return err;
  }
 
+void vma_invalidate_tlb(struct i915_address_space *vm, u32 tlb)
+{
+	/*
+	 * Before we release the pages that were bound by this vma, we
+	 * must invalidate all the TLBs that may still have a reference
+	 * back to our physical address. It only needs to be done once,
+	 * so after updating the PTE to point away from the pages, record
+	 * the most recent TLB invalidation seqno, and if we have not yet
+	 * flushed the TLBs upon release, perform a full invalidation.
+	 */
+	WRITE_ONCE(tlb, intel_gt_next_invalidate_tlb_full(vm->gt));
+}
+
  static void __vma_put_pages(struct i915_vma *vma, unsigned int count)
  {
  	/* We allocate under vma_get_pages, so beware the shrinker */

@@ -1942,7 +1952,12 @@ struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async)
  		vma->vm->skip_pte_rewrite;
  	trace_i915_vma_unbind(vma);
 
-	unbind_fence = i915_vma_resource_unbind(vma_res);
+	if (async)
+		unbind_fence = i915_vma_resource_unbind(vma_res,
+							&vma->obj->mm.tlb);
+	else
+		unbind_fence = i915_vma_resource_unbind(vma_res, NULL);
+
  	vma->resource = NULL;
 
  	atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_GGTT_WRITE),

@@ -1950,10 +1965,13 @@ struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async)
 
  	i915_vma_detach(vma);
 
-	if (!async && unbind_fence) {
-		dma_fence_wait(unbind_fence, false);
-		dma_fence_put(unbind_fence);
-		unbind_fence = NULL;
+	if (!async) {
+		if (unbind_fence) {
+			dma_fence_wait(unbind_fence, false);
+			dma_fence_put(unbind_fence);
+			unbind_fence = NULL;
+		}
+		vma_invalidate_tlb(vma->vm, vma->obj->mm.tlb);
  	}
 
  	/*

drivers/gpu/drm/i915/i915_vma.h +1 -0

index 88ca0bd9c900..5048eed536da 100644
@@ -213,6 +213,7 @@ bool i915_vma_misplaced(const struct i915_vma *vma,
  			u64 size, u64 alignment, u64 flags);
  void __i915_vma_set_map_and_fenceable(struct i915_vma *vma);
  void i915_vma_revoke_mmap(struct i915_vma *vma);
+void vma_invalidate_tlb(struct i915_address_space *vm, u32 tlb);
  struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async);
  int __i915_vma_unbind(struct i915_vma *vma);
  int __must_check i915_vma_unbind(struct i915_vma *vma);

drivers/gpu/drm/i915/i915_vma_resource.c +4 -1

index 27c55027387a..5a67995ea5fe 100644
@@ -223,10 +223,13 @@ i915_vma_resource_fence_notify(struct i915_sw_fence *fence,
   * Return: A refcounted pointer to a dma-fence that signals when unbinding is
   * complete.
   */
-struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res)
+struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res,
+					   u32 *tlb)
  {
  	struct i915_address_space *vm = vma_res->vm;
 
+	vma_res->tlb = tlb;
+
  	/* Reference for the sw fence */
  	i915_vma_resource_get(vma_res);
 

drivers/gpu/drm/i915/i915_vma_resource.h +5 -1

index 5d8427caa2ba..06923d1816e7 100644
@@ -67,6 +67,7 @@ struct i915_page_sizes {
   * taken when the unbind is scheduled.
   * @skip_pte_rewrite: During ggtt suspend and vm takedown pte rewriting
   * needs to be skipped for unbind.
+ * @tlb: pointer for obj->mm.tlb, if async unbind. Otherwise, NULL
   *
   * The lifetime of a struct i915_vma_resource is from a binding request to
   * the actual possible asynchronous unbind has completed.

@@ -119,6 +120,8 @@ struct i915_vma_resource {
  	bool immediate_unbind:1;
  	bool needs_wakeref:1;
  	bool skip_pte_rewrite:1;
+
+	u32 *tlb;
  };
 
  bool i915_vma_resource_hold(struct i915_vma_resource *vma_res,

@@ -131,7 +134,8 @@ struct i915_vma_resource *i915_vma_resource_alloc(void);
 
  void i915_vma_resource_free(struct i915_vma_resource *vma_res);
 
-struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res);
+struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res,
+					   u32 *tlb);
 
  void __i915_vma_resource_init(struct i915_vma_resource *vma_res);
 

Quoted 3 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

Invalidate TLB in patch, in order to reduce performance regressions.

"in batches"?

Quoted 11 lines

drivers/gpu/drm/i915/gt/intel_ppgtt.c +1 -2

index d8b94d638559..2da6c82a8bd2 100644
@@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
  void ppgtt_unbind_vma(struct i915_address_space *vm,
  		      struct i915_vma_resource *vma_res)
  {
-	if (vma_res->allocated)
-		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (!vma_res->allocated)

drivers/gpu/drm/i915/gt/intel_ppgtt.c void ppgtt_bind_vma(struct i915_address_space *vm, · line 214

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_ppgtt.c +5 -0

+		return;
+
+	vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (vma_res->tlb)
+		vma_invalidate_tlb(vm, *vma_res->tlb);

The patch is about more than batching? If there is a security hole in this area (unbind) with the current code?

Quoted 7 lines · On Mon, 18 Jul 2022 14:52:05 +0100 Tvrtko Ursulin…

Quoted 3 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

Invalidate TLB in patch, in order to reduce performance regressions.

"in batches"?

Yeah. Will fix it.

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/i915/gt/intel_ppgtt.c +1 -2

index d8b94d638559..2da6c82a8bd2 100644
@@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
   void ppgtt_unbind_vma(struct i915_address_space *vm,
   		      struct i915_vma_resource *vma_res)
   {
-	if (vma_res->allocated)
-		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (!vma_res->allocated)

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_ppgtt.c void ppgtt_bind_vma(struct i915_address_space *vm, · line 214

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_ppgtt.c +5 -0

+		return;
+
+	vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (vma_res->tlb)
+		vma_invalidate_tlb(vm, *vma_res->tlb);

The patch is about more than batching? If there is a security hole in this area (unbind) with the current code?

No, I don't think there's a security hole. The rationale for this is not due to it.

Since commit 2f6b90da9192 ("drm/i915: Use vma resources for async unbinding"), VMA unbind can happen either sync or async.

So, the logic needs to do TLB invalidate on two places. After this patch, the code at __i915_vma_evict is:

	struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async)
	{
...
		if (async)
			unbind_fence = i915_vma_resource_unbind(vma_res,
								&vma->obj->mm.tlb);
		else
			unbind_fence = i915_vma_resource_unbind(vma_res, NULL);

		vma->resource = NULL;

		atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_GGTT_WRITE),
			   &vma->flags);

		i915_vma_detach(vma);

		if (!async) {
			if (unbind_fence) {
				dma_fence_wait(unbind_fence, false);
				dma_fence_put(unbind_fence);
				unbind_fence = NULL;
			}
			vma_invalidate_tlb(vma->vm, vma->obj->mm.tlb);
		}

...

So, basically, if !async, __i915_vma_evict() will do TLB cache invalidation.

However, when async is used, the actual page release will happen later, at this function:

	void ppgtt_unbind_vma(struct i915_address_space *vm,
			      struct i915_vma_resource *vma_res)
	{
		if (!vma_res->allocated)
			return;

		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
		if (vma_res->tlb)
			vma_invalidate_tlb(vm, *vma_res->tlb);
	}

From: Chris Wilson <[email protected]>

Invalidate TLB in patch, in order to reduce performance regressions.

Currently, every caller performs a full barrier around a TLB invalidation, ignoring all other invalidations that may have already removed their PTEs from the cache. As this is a synchronous operation and can be quite slow, we cause multiple threads to contend on the TLB invalidate mutex blocking userspace.

We only need to invalidate the TLB once after replacing our PTE to ensure that there is no possible continued access to the physical address before releasing our pages. By tracking a seqno for each full TLB invalidate we can quickly determine if one has been performed since rewriting the PTE, and only if necessary trigger one for ourselves.

That helps to reduce the performance regression introduced by TLB invalidate logic.

[mchehab: rebased to not require moving the code to a separate file]

Cc [email protected]

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

Suggested-by Tvrtko Ursulin <[email protected]>

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

Cc Fei Yang <[email protected]>

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] at: https://lore.kernel.org/all/[email protected]/

  .../gpu/drm/i915/gem/i915_gem_object_types.h  |  3 +-
  drivers/gpu/drm/i915/gem/i915_gem_pages.c     | 21 +++++---
  drivers/gpu/drm/i915/gt/intel_gt.c            | 53 ++++++++++++++-----
  drivers/gpu/drm/i915/gt/intel_gt.h            | 12 ++++-
  drivers/gpu/drm/i915/gt/intel_gt_types.h      | 18 ++++++-
  drivers/gpu/drm/i915/gt/intel_ppgtt.c         |  8 ++-
  drivers/gpu/drm/i915/i915_vma.c               | 34 +++++++++---
  drivers/gpu/drm/i915/i915_vma.h               |  1 +
  drivers/gpu/drm/i915/i915_vma_resource.c      |  5 +-
  drivers/gpu/drm/i915/i915_vma_resource.h      |  6 ++-
  10 files changed, 125 insertions(+), 36 deletions(-)

drivers/gpu/drm/i915/gem/i915_gem_object_types.h +2 -1

index 5cf36a130061..9f6b14ec189a 100644
@@ -335,7 +335,6 @@ struct drm_i915_gem_object {
  #define I915_BO_READONLY          BIT(7)
  #define I915_TILING_QUIRK_BIT     8 /* unknown swizzling; do not release! */
  #define I915_BO_PROTECTED         BIT(9)
-#define I915_BO_WAS_BOUND_BIT     10
  	/**
  	 * @mem_flags - Mutable placement-related flags
  	 *

@@ -616,6 +615,8 @@ struct drm_i915_gem_object {
  		 * pages were last acquired.
  		 */
  		bool dirty:1;
+
+		u32 tlb;
  	} mm;
 
  	struct {

drivers/gpu/drm/i915/gem/i915_gem_pages.c +13 -8

index 6835279943df..8357dbdcab5c 100644
@@ -191,6 +191,18 @@ static void unmap_object(struct drm_i915_gem_object *obj, void *ptr)
  		vunmap(ptr);
  }
 
+static void flush_tlb_invalidate(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct intel_gt *gt = to_gt(i915);
+
+	if (!obj->mm.tlb)
+		return;
+
+	intel_gt_invalidate_tlb(gt, obj->mm.tlb);
+	obj->mm.tlb = 0;
+}
+
  struct sg_table *
  __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
  {

@@ -216,14 +228,7 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
  	__i915_gem_object_reset_page_iter(obj);
  	obj->mm.page_sizes.phys = obj->mm.page_sizes.sg = 0;
 
-	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_gt_pm_if_awake(gt, wakeref)
-			intel_gt_invalidate_tlbs(gt);
-	}
+	flush_tlb_invalidate(obj);
 
  	return pages;
  }

drivers/gpu/drm/i915/gt/intel_gt.c +40 -13

index 5c55a90672f4..f435e06125aa 100644
@@ -38,8 +38,6 @@ static void __intel_gt_init_early(struct intel_gt *gt)
  {
  	spin_lock_init(&gt->irq_lock);
 
-	mutex_init(&gt->tlb_invalidate_lock);
-
  	INIT_LIST_HEAD(&gt->closed_vma);
  	spin_lock_init(&gt->closed_lock);
 

@@ -50,6 +48,8 @@ static void __intel_gt_init_early(struct intel_gt *gt)
  	intel_gt_init_reset(gt);
  	intel_gt_init_requests(gt);
  	intel_gt_init_timelines(gt);
+	mutex_init(&gt->tlb.invalidate_lock);
+	seqcount_mutex_init(&gt->tlb.seqno, &gt->tlb.invalidate_lock);
  	intel_gt_pm_init_early(gt);
 
  	intel_uc_init_early(&gt->uc);

@@ -770,6 +770,7 @@ void intel_gt_driver_late_release_all(struct drm_i915_private *i915)
  		intel_gt_fini_requests(gt);
  		intel_gt_fini_reset(gt);
  		intel_gt_fini_timelines(gt);
+		mutex_destroy(&gt->tlb.invalidate_lock);
  		intel_engines_free(gt);
  	}
  }

@@ -908,7 +909,7 @@ get_reg_and_bit(const struct intel_engine_cs *engine, const bool gen8,
  	return rb;
  }
 
-void intel_gt_invalidate_tlbs(struct intel_gt *gt)
+static void mmio_invalidate_full(struct intel_gt *gt)
  {
  	static const i915_reg_t gen8_regs[] = {
  		[RENDER_CLASS]			= GEN8_RTCR,

@@ -931,12 +932,6 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  	const i915_reg_t *regs;
  	unsigned int num = 0;
 
-	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);

@@ -951,9 +946,6 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  			  "Platform does not implement TLB invalidation!"))
  		return;
 
-	GEM_TRACE("\n");
-
-	mutex_lock(&gt->tlb_invalidate_lock);
  	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
  	spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */

@@ -973,6 +965,8 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  		awake |= engine->mask;
  	}
 
+	GT_TRACE(gt, "invalidated engines %08x\n", awake);
+
  	/* Wa_2207587034:tgl,dg1,rkl,adl-s,adl-p */
  	if (awake &&
  	    (IS_TIGERLAKE(i915) ||

@@ -1012,5 +1006,38 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
  	 * transitions.
  	 */
  	intel_uncore_forcewake_put_delayed(uncore, FORCEWAKE_ALL);
-	mutex_unlock(&gt->tlb_invalidate_lock);
+}
+
+static bool tlb_seqno_passed(const struct intel_gt *gt, u32 seqno)
+{
+	u32 cur = intel_gt_tlb_seqno(gt);
+
+	/* Only skip if a *full* TLB invalidate barrier has passed */
+	return (s32)(cur - ALIGN(seqno, 2)) > 0;
+}
+
+void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno)
+{
+	intel_wakeref_t wakeref;
+
+	if (I915_SELFTEST_ONLY(gt->awake == -ENODEV))
+		return;
+
+	if (intel_gt_is_wedged(gt))
+		return;
+
+	if (tlb_seqno_passed(gt, seqno))
+		return;
+
+	with_intel_gt_pm_if_awake(gt, wakeref) {
+		mutex_lock(&gt->tlb.invalidate_lock);
+		if (tlb_seqno_passed(gt, seqno))
+			goto unlock;
+
+		mmio_invalidate_full(gt);
+
+		write_seqcount_invalidate(&gt->tlb.seqno);
+unlock:
+		mutex_unlock(&gt->tlb.invalidate_lock);
+	}
  }

drivers/gpu/drm/i915/gt/intel_gt.h +11 -1

index 82d6f248d876..40b06adf509a 100644
@@ -101,6 +101,16 @@ void intel_gt_info_print(const struct intel_gt_info *info,
 
  void intel_gt_watchdog_work(struct work_struct *work);
 
-void intel_gt_invalidate_tlbs(struct intel_gt *gt);
+static inline u32 intel_gt_tlb_seqno(const struct intel_gt *gt)
+{
+	return seqprop_sequence(&gt->tlb.seqno);
+}
+
+static inline u32 intel_gt_next_invalidate_tlb_full(const struct intel_gt *gt)
+{
+	return intel_gt_tlb_seqno(gt) | 1;
+}
+
+void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno);
 
  #endif /* __INTEL_GT_H__ */

drivers/gpu/drm/i915/gt/intel_gt_types.h +17 -1

index df708802889d..3804a583382b 100644
@@ -11,6 +11,7 @@
  #include <linux/llist.h>
  #include <linux/mutex.h>
  #include <linux/notifier.h>
+#include <linux/seqlock.h>
  #include <linux/spinlock.h>
  #include <linux/types.h>
  #include <linux/workqueue.h>

@@ -83,7 +84,22 @@ struct intel_gt {
  	struct intel_uc uc;
  	struct intel_gsc gsc;
 
-	struct mutex tlb_invalidate_lock;
+	struct {
+		/* Serialize global tlb invalidations */
+		struct mutex invalidate_lock;
+
+		/*
+		 * Batch TLB invalidations
+		 *
+		 * After unbinding the PTE, we need to ensure the TLB
+		 * are invalidated prior to releasing the physical pages.
+		 * But we only need one such invalidation for all unbinds,
+		 * so we track how many TLB invalidations have been
+		 * performed since unbind the PTE and only emit an extra
+		 * invalidate if no full barrier has been passed.
+		 */
+		seqcount_mutex_t seqno;
+	} tlb;
 
  	struct i915_wa_list wa_list;
 

drivers/gpu/drm/i915/gt/intel_ppgtt.c +6 -2

index d8b94d638559..2da6c82a8bd2 100644
@@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
  void ppgtt_unbind_vma(struct i915_address_space *vm,
  		      struct i915_vma_resource *vma_res)
  {
-	if (vma_res->allocated)
-		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (!vma_res->allocated)
+		return;
+
+	vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (vma_res->tlb)
+		vma_invalidate_tlb(vm, *vma_res->tlb);
  }
 
  static unsigned long pd_count(u64 size, int shift)

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

index 646f419b2035..84a9ccbc5fc5 100644
@@ -538,9 +538,6 @@ int i915_vma_bind(struct i915_vma *vma,
  				   bind_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;
  }

@@ -1311,6 +1308,19 @@ I915_SELFTEST_EXPORT int i915_vma_get_pages(struct i915_vma *vma)
  	return err;
  }
 
+void vma_invalidate_tlb(struct i915_address_space *vm, u32 tlb)

drivers/gpu/drm/i915/i915_vma.c +15 -5

+}
+
  static void __vma_put_pages(struct i915_vma *vma, unsigned int count)
  {
  	/* We allocate under vma_get_pages, so beware the shrinker */

@@ -1942,7 +1952,12 @@ struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async)
  		vma->vm->skip_pte_rewrite;
  	trace_i915_vma_unbind(vma);
 
-	unbind_fence = i915_vma_resource_unbind(vma_res);
+	if (async)
+		unbind_fence = i915_vma_resource_unbind(vma_res,
+							&vma->obj->mm.tlb);
+	else
+		unbind_fence = i915_vma_resource_unbind(vma_res, NULL);
+
  	vma->resource = NULL;
 
  	atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_GGTT_WRITE),

@@ -1950,10 +1965,13 @@ struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async)
 
  	i915_vma_detach(vma);
 
-	if (!async && unbind_fence) {
-		dma_fence_wait(unbind_fence, false);
-		dma_fence_put(unbind_fence);
-		unbind_fence = NULL;
+	if (!async) {
+		if (unbind_fence) {
+			dma_fence_wait(unbind_fence, false);
+			dma_fence_put(unbind_fence);
+			unbind_fence = NULL;
+		}
+		vma_invalidate_tlb(vma->vm, vma->obj->mm.tlb);
  	}
 
  	/*

drivers/gpu/drm/i915/i915_vma.h +1 -0

index 88ca0bd9c900..5048eed536da 100644
@@ -213,6 +213,7 @@ bool i915_vma_misplaced(const struct i915_vma *vma,
  			u64 size, u64 alignment, u64 flags);
  void __i915_vma_set_map_and_fenceable(struct i915_vma *vma);
  void i915_vma_revoke_mmap(struct i915_vma *vma);
+void vma_invalidate_tlb(struct i915_address_space *vm, u32 tlb);
  struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async);
  int __i915_vma_unbind(struct i915_vma *vma);
  int __must_check i915_vma_unbind(struct i915_vma *vma);

drivers/gpu/drm/i915/i915_vma_resource.c +4 -1

index 27c55027387a..5a67995ea5fe 100644
@@ -223,10 +223,13 @@ i915_vma_resource_fence_notify(struct i915_sw_fence *fence,
   * Return: A refcounted pointer to a dma-fence that signals when unbinding is
   * complete.
   */
-struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res)
+struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res,
+					   u32 *tlb)
  {
  	struct i915_address_space *vm = vma_res->vm;
 
+	vma_res->tlb = tlb;
+
  	/* Reference for the sw fence */
  	i915_vma_resource_get(vma_res);
 

drivers/gpu/drm/i915/i915_vma_resource.h +5 -1

index 5d8427caa2ba..06923d1816e7 100644
@@ -67,6 +67,7 @@ struct i915_page_sizes {
   * taken when the unbind is scheduled.
   * @skip_pte_rewrite: During ggtt suspend and vm takedown pte rewriting
   * needs to be skipped for unbind.
+ * @tlb: pointer for obj->mm.tlb, if async unbind. Otherwise, NULL
   *
   * The lifetime of a struct i915_vma_resource is from a binding request to
   * the actual possible asynchronous unbind has completed.

@@ -119,6 +120,8 @@ struct i915_vma_resource {
  	bool immediate_unbind:1;
  	bool needs_wakeref:1;
  	bool skip_pte_rewrite:1;
+
+	u32 *tlb;
  };
 
  bool i915_vma_resource_hold(struct i915_vma_resource *vma_res,

@@ -131,7 +134,8 @@ struct i915_vma_resource *i915_vma_resource_alloc(void);
 
  void i915_vma_resource_free(struct i915_vma_resource *vma_res);
 
-struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res);
+struct dma_fence *i915_vma_resource_unbind(struct i915_vma_resource *vma_res,
+					   u32 *tlb);
 
  void __i915_vma_resource_init(struct i915_vma_resource *vma_res);
 

Quoted 12 lines · Mauro Carvalho Chehab

Quoted 7 lines · On Mon, 18 Jul 2022 14:52:05 +0100 Tvrtko Ursulin…

Quoted 3 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

Invalidate TLB in patch, in order to reduce performance regressions.

"in batches"?

Yeah. Will fix it.

drivers/gpu/drm/i915/i915_vma.c I915_SELFTEST_EXPORT int i915_vma_get_pages(struct i915_vma *vma) · line 1321

Quoted 11 lines

drivers/gpu/drm/i915/i915_vma.c +11 -0

+void vma_invalidate_tlb(struct i915_address_space *vm, u32 tlb)
+{
+	/*
+	 * Before we release the pages that were bound by this vma, we
+	 * must invalidate all the TLBs that may still have a reference
+	 * back to our physical address. It only needs to be done once,
+	 * so after updating the PTE to point away from the pages, record
+	 * the most recent TLB invalidation seqno, and if we have not yet
+	 * flushed the TLBs upon release, perform a full invalidation.
+	 */
+	WRITE_ONCE(tlb, intel_gt_next_invalidate_tlb_full(vm->gt));  

Shouldn't tlb be a pointer for this to make sense?

Quoted 21 lines

Quoted 20 lines

Quoted 7 lines

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

index d8b94d638559..2da6c82a8bd2 100644
@@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
   void ppgtt_unbind_vma(struct i915_address_space *vm,

drivers/gpu/drm/i915/gt/intel_ppgtt.c void ppgtt_bind_vma(struct i915_address_space *vm, · line 214

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_ppgtt.c +6 -2

   		      struct i915_vma_resource *vma_res)
   {
-	if (vma_res->allocated)
-		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (!vma_res->allocated)
+		return;
+
+	vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (vma_res->tlb)
+		vma_invalidate_tlb(vm, *vma_res->tlb);  

The patch is about more than batching? If there is a security hole in this area (unbind) with the current code?

Quoted 2 lines

No, I don't think there's a security hole. The rationale for this is not due to it.

In this case obvious question is why are these changes in the patch which declares itself to be about batching invalidations? Because...

I am explaining why it looks to me that the patch is doing two things. Implementing batching _and_ adding invalidation points at VMA unbind sites, while so far we had it at backing store release only. Maybe I am wrong and perhaps I am too slow to pick up on the explanation here.

So if the patch is doing two things please split it up.

I am further confused by the invalidation call site in evict and in unbind - why there can't be one logical site since the logical sequence is evict -> unbind.

Quoted 14 lines · On Wed, 20 Jul 2022 11:49:59 +0100 Tvrtko Ursulin…

Quoted 12 lines · Mauro Carvalho Chehab

Quoted 7 lines · On Mon, 18 Jul 2022 14:52:05 +0100 Tvrtko Ursulin…

Quoted 3 lines · Mauro Carvalho Chehab

From: Chris Wilson <[email protected]>

Invalidate TLB in patch, in order to reduce performance regressions.

"in batches"?

Yeah. Will fix it.

Quoted 13 lines

drivers/gpu/drm/i915/i915_vma.c I915_SELFTEST_EXPORT int i915_vma_get_pages(struct i915_vma *vma) · line 1321

Quoted 11 lines

drivers/gpu/drm/i915/i915_vma.c +11 -0

+void vma_invalidate_tlb(struct i915_address_space *vm, u32 tlb)
+{
+	/*
+	 * Before we release the pages that were bound by this vma, we
+	 * must invalidate all the TLBs that may still have a reference
+	 * back to our physical address. It only needs to be done once,
+	 * so after updating the PTE to point away from the pages, record
+	 * the most recent TLB invalidation seqno, and if we have not yet
+	 * flushed the TLBs upon release, perform a full invalidation.
+	 */
+	WRITE_ONCE(tlb, intel_gt_next_invalidate_tlb_full(vm->gt));

Shouldn't tlb be a pointer for this to make sense?

Oh, my mistake! Will fix at the next version.

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 7 lines

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

index d8b94d638559..2da6c82a8bd2 100644
@@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm,
    void ppgtt_unbind_vma(struct i915_address_space *vm,

drivers/gpu/drm/i915/gt/intel_ppgtt.c void ppgtt_bind_vma(struct i915_address_space *vm, · line 214

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_ppgtt.c +6 -2

    		      struct i915_vma_resource *vma_res)
    {
-	if (vma_res->allocated)
-		vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (!vma_res->allocated)
+		return;
+
+	vm->clear_range(vm, vma_res->start, vma_res->vma_size);
+	if (vma_res->tlb)
+		vma_invalidate_tlb(vm, *vma_res->tlb);

The patch is about more than batching? If there is a security hole in this area (unbind) with the current code?

Quoted 5 lines

Quoted 2 lines

No, I don't think there's a security hole. The rationale for this is not due to it.

In this case obvious question is why are these changes in the patch which declares itself to be about batching invalidations? Because...

Because vma_invalidate_tlb() basically stores a TLB seqno, but the actual invalidation is deferred to when the pages are unset, at __i915_gem_object_unset_pages().

So, what happens is:

- on VMA sync mode, the need to invalidate TLB is marked at __vma_put_pages(), before VMA unbind;
- on async, this is deferred to happen at ppgtt_unbind_vma(), where it marks the need to invalidate TLBs.

On both cases, __i915_gem_object_unset_pages() is called later, when the driver is ready to unmap the page.

Quoted 11 lines

I am explaining why it looks to me that the patch is doing two things. Implementing batching _and_ adding invalidation points at VMA unbind sites, while so far we had it at backing store release only. Maybe I am wrong and perhaps I am too slow to pick up on the explanation here.

So if the patch is doing two things please split it up.

I am further confused by the invalidation call site in evict and in unbind - why there can't be one logical site since the logical sequence is evict -> unbind.

The invalidation happens only on one place: __i915_gem_object_unset_pages().

Despite its name, vma_invalidate_tlb() just marks the need of doing TLB invalidation.

Regards,
Mauro

Quoted 6 lines

Because vma_invalidate_tlb() basically stores a TLB seqno, but the actual invalidation is deferred to when the pages are unset, at __i915_gem_object_unset_pages().

So, what happens is:

Quoted 7 lines

- on VMA sync mode, the need to invalidate TLB is marked at __vma_put_pages(), before VMA unbind;
- on async, this is deferred to happen at ppgtt_unbind_vma(), where it marks the need to invalidate TLBs.

On both cases, __i915_gem_object_unset_pages() is called later, when the driver is ready to unmap the page.

Sorry still not clear to me why is the patch moving marking of the need to invalidate (regardless if it a bit like today, or a seqno like in this patch) from bind to unbind?

What if the seqno was stored in i915_vma_bind, where the bit is set today, and all the hunks which touch the unbind and evict would disappear from the patch. What wouldn't work in that case, if anything?

Quoted 14 lines · On Wed, 27 Jul 2022 13:56:50 +0100 Tvrtko Ursulin…

Quoted 6 lines

Because vma_invalidate_tlb() basically stores a TLB seqno, but the actual invalidation is deferred to when the pages are unset, at __i915_gem_object_unset_pages().

So, what happens is:

Quoted 7 lines

- on VMA sync mode, the need to invalidate TLB is marked at __vma_put_pages(), before VMA unbind;
- on async, this is deferred to happen at ppgtt_unbind_vma(), where it marks the need to invalidate TLBs.

On both cases, __i915_gem_object_unset_pages() is called later, when the driver is ready to unmap the page.

Quoted 7 lines

Sorry still not clear to me why is the patch moving marking of the need to invalidate (regardless if it a bit like today, or a seqno like in this patch) from bind to unbind?

What if the seqno was stored in i915_vma_bind, where the bit is set today, and all the hunks which touch the unbind and evict would disappear from the patch. What wouldn't work in that case, if anything?

Ah, now I see your point.

I can't see any sense on having a sequence number at VMA bind, as the unbind order can be different. The need of doing a full TLB invalidation or not depends on the unbind order.

The way the current algorithm works is that drm_i915_gem_object can be created on any order, and, at unbind/evict, they receive a seqno.

The seqno is incremented at intel_gt_invalidate_tlb():

    void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno)
    {
	with_intel_gt_pm_if_awake(gt, wakeref) {
		mutex_lock(&gt->tlb.invalidate_lock);
		if (tlb_seqno_passed(gt, seqno))
				goto unlock;

		mmio_invalidate_full(gt);

		write_seqcount_invalidate(&gt->tlb.seqno);	// increment seqno

So, let's say 3 objects were created, on this order:

obj1
obj2
obj3

They would be unbind/evict on a different order. On that time, the mm.tlb will be stamped with a seqno, using the number from the last TLB flush, plus 1.

As different threads can be used to handle TLB flushes, let's imagine two threads (just for the sake of having an example). On such case, what we would have is:

seqno Thread 0 Thread 1

seqno=2 unbind/evict event

		obj3.mm.tlb = seqno | 1
seqno=2		unbind/evict event
		obj1.mm.tlb = seqno | 1
						__i915_gem_object_unset_pages() 
						called for obj3, TLB flush happened,
						invalidating both obj1 and obj2.
						seqno += 2					
seqno=4		unbind/evict event
		obj1.mm.tlb = seqno | 1

						__i915_gem_object_unset_pages()
						called for obj1, don't flush.
...
						__i915_gem_object_unset_pages() called for obj2, TLB flush happened
						seqno += 2

seqno=6

So, basically the seqno is used to track when the object data stopped being updated, because of an unbind/evict event, being later used by intel_gt_invalidate_tlb() when called from __i915_gem_object_unset_pages(), in order to check if a previous invalidation call was enough to invalidate the object, or if a new call is needed.

Now, if seqno is stored at bind, data can still leak, as the assumption made by intel_gt_invalidate_tlb() that the data stopped being used at seqno is not true anymore.

Still, I agree that this logic is complex and should be better documented. So, if you're now OK with this patch, I'll add the above explanation inside a kernel-doc comment.

Quoted 14 lines · On Wed, 27 Jul 2022 13:56:50 +0100 Tvrtko Ursulin…

Quoted 6 lines

Because vma_invalidate_tlb() basically stores a TLB seqno, but the actual invalidation is deferred to when the pages are unset, at __i915_gem_object_unset_pages().

So, what happens is:

Quoted 7 lines

- on VMA sync mode, the need to invalidate TLB is marked at

    __vma_put_pages(), before VMA unbind;

- on async, this is deferred to happen at ppgtt_unbind_vma(), where it marks the need to invalidate TLBs.

On both cases, __i915_gem_object_unset_pages() is called later, when the driver is ready to unmap the page.

Quoted 7 lines

Sorry still not clear to me why is the patch moving marking of the need to invalidate (regardless if it a bit like today, or a seqno like in this patch) from bind to unbind?

What if the seqno was stored in i915_vma_bind, where the bit is set today, and all the hunks which touch the unbind and evict would disappear from the patch. What wouldn't work in that case, if anything?

Ah, now I see your point.

I can't see any sense on having a sequence number at VMA bind, as the unbind order can be different. The need of doing a full TLB invalidation or not depends on the unbind order.

The way the current algorithm works is that drm_i915_gem_object can be created on any order, and, at unbind/evict, they receive a seqno.

The seqno is incremented at intel_gt_invalidate_tlb():

     void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno)
     {
	with_intel_gt_pm_if_awake(gt, wakeref) {
		mutex_lock(&gt->tlb.invalidate_lock);
		if (tlb_seqno_passed(gt, seqno))
				goto unlock;

		mmio_invalidate_full(gt);

		write_seqcount_invalidate(&gt->tlb.seqno);	// increment seqno

So, let's say 3 objects were created, on this order:

obj1
obj2
obj3

They would be unbind/evict on a different order. On that time, the mm.tlb will be stamped with a seqno, using the number from the last TLB flush, plus 1.

As different threads can be used to handle TLB flushes, let's imagine two threads (just for the sake of having an example). On such case, what we would have is:

seqno Thread 0 Thread 1

seqno=2 unbind/evict event

		obj3.mm.tlb = seqno | 1
seqno=2		unbind/evict event
		obj1.mm.tlb = seqno | 1
						__i915_gem_object_unset_pages()
						called for obj3, TLB flush happened,
						invalidating both obj1 and obj2.
						seqno += 2					
seqno=4		unbind/evict event
		obj1.mm.tlb = seqno | 1
						__i915_gem_object_unset_pages()
						called for obj1, don't flush.
...
						__i915_gem_object_unset_pages() called for obj2, TLB flush happened
						seqno += 2

seqno=6

So, basically the seqno is used to track when the object data stopped being updated, because of an unbind/evict event, being later used by intel_gt_invalidate_tlb() when called from __i915_gem_object_unset_pages(), in order to check if a previous invalidation call was enough to invalidate the object, or if a new call is needed.

Now, if seqno is stored at bind, data can still leak, as the assumption made by intel_gt_invalidate_tlb() that the data stopped being used at seqno is not true anymore.

Still, I agree that this logic is complex and should be better documented. So, if you're now OK with this patch, I'll add the above explanation inside a kernel-doc comment.

Regards,
Mauro

From: Chris Wilson <[email protected]>

Prepare for supporting more TLB invalidation scenarios by moving the current MMIO invalidation to its own file.

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

Cc Fei Yang <[email protected]>

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

From: Prathap Kumar Valsan <[email protected]>

Add routines to interface with GuC firmware for TLB invalidation.

Signed-off-by Prathap Kumar Valsan <[email protected]>

Cc Bruce Chang <[email protected]>

Cc Michal Wajdeczko <[email protected]>

Cc Matthew Brost <[email protected]>

Cc Chris Wilson <[email protected]>

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

---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v2 00/21] at: https://lore.kernel.org/all/[email protected]/

 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  | 35 +++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc.c        | 90 ++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        | 13 +++
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     | 24 ++++-
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  6 ++
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++++++++++++++++++-
 6 files changed, 253 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +0 -0

index 4ef9990ed7f8..2e39d8df4c82 100644

drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +14 -0

+/* Flush PPC or SMRO caches along with TLB invalidation request */
+#define INTEL_GUC_TLB_INVAL_FLUSH_CACHE (1 << 31)
+
+enum intel_guc_tlb_invalidation_type {
+	INTEL_GUC_TLB_INVAL_GUC = 0x3,
+};
+
+/*
+ * 0: Heavy mode of Invalidation:
+ * The pipeline of the engine(s) for which the invalidation is targeted to is
+ * blocked, and all the in-flight transactions are guaranteed to be Globally
+ * Observed before completing the TLB invalidation
+ * 1: Lite mode of Invalidation:
+ * TLBs of the targeted engine(s) are immediately invalidated.

drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +5 -0

+enum intel_guc_tlb_inval_mode {
+	INTEL_GUC_TLB_INVAL_MODE_HEAVY = 0x0,
+	INTEL_GUC_TLB_INVAL_MODE_LITE = 0x1,
+};
+
 #endif /* _ABI_GUC_ACTIONS_ABI_H */

drivers/gpu/drm/i915/gt/uc/intel_guc.c +0 -0

index 2706a8c65090..5c59f9b144a3 100644

drivers/gpu/drm/i915/gt/uc/intel_guc.c +20 -0

+{
+	struct intel_guc_tlb_wait _wq, *wq = &_wq;
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	int err = 0;
+	u32 seqno;
+
+	init_waitqueue_head(&_wq.wq);
+
+	if (xa_alloc_cyclic_irq(&guc->tlb_lookup, &seqno, wq,
+				xa_limit_32b, &guc->next_seqno,
+				GFP_ATOMIC | __GFP_NOWARN) < 0) {
+		/* Under severe memory pressure? Serialise TLB allocations */
+		xa_lock_irq(&guc->tlb_lookup);
+		wq = xa_load(&guc->tlb_lookup, guc->serial_slot);
+		wait_event_lock_irq(wq->wq,
+				    !READ_ONCE(wq->status),
+				    guc->tlb_lookup.xa_lock);
+		/*
+		 * Update wq->status under lock to ensure only one waiter can
+		 * issue the tlb invalidation command using the serial slot at a

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

+		err = -ETIME;
+	}
+out:
+	remove_wait_queue(&wq->wq, &wait);
+	if (seqno != guc->serial_slot)
+		xa_erase_irq(&guc->tlb_lookup, seqno);
+
+	return err;
+}
+
+/*
+ * Guc TLB Invalidation: Invalidate the TLB's of GuC itself.
+ */
+int intel_guc_invalidate_tlb_guc(struct intel_guc *guc,
+				 enum intel_guc_tlb_inval_mode mode)
+{

drivers/gpu/drm/i915/gt/uc/intel_guc.c +6 -0

+		return 0;
+	}
+
+	return guc_send_invalidate_tlb(guc, action, ARRAY_SIZE(action));
+}
+
 /**
  * intel_guc_load_status - dump information about GuC load status
  * @guc: the GuC

drivers/gpu/drm/i915/gt/uc/intel_guc.h +0 -0

index d0d99f178f2d..f82a121b0838 100644

drivers/gpu/drm/i915/gt/uc/intel_guc.h +10 -0

+
 	struct {
 		void (*reset)(struct intel_guc *guc);
 		void (*enable)(struct intel_guc *guc);

@@ -248,6 +252,11 @@ struct intel_guc {
 #endif
 };
 
+struct intel_guc_tlb_wait {
+	struct wait_queue_head wq;
+	u8 status;
+} __aligned(4);
+
 static inline struct intel_guc *log_to_guc(struct intel_guc_log *log)
 {
 	return container_of(log, struct intel_guc, log);

@@ -363,6 +372,9 @@ int intel_guc_allocate_and_map_vma(struct intel_guc *guc, u32 size,
 int intel_guc_self_cfg32(struct intel_guc *guc, u16 key, u32 value);
 int intel_guc_self_cfg64(struct intel_guc *guc, u16 key, u64 value);
 
+int intel_guc_invalidate_tlb_guc(struct intel_guc *guc,
+				 enum intel_guc_tlb_inval_mode mode);
+
 static inline bool intel_guc_is_supported(struct intel_guc *guc)
 {
 	return intel_uc_fw_is_supported(&guc->fw);

@@ -440,6 +452,7 @@ int intel_guc_engine_failure_process_msg(struct intel_guc *guc,
 					 const u32 *msg, u32 len);
 int intel_guc_error_capture_process_msg(struct intel_guc *guc,
 					const u32 *msg, u32 len);
+void intel_guc_tlb_invalidation_done(struct intel_guc *guc, u32 seqno);
 
 struct intel_engine_cs *
 intel_guc_lookup_engine(struct intel_guc *guc, u8 guc_class, u8 instance);

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +4 -4

index f01325cd1b62..c1ce542b7855 100644
@@ -1023,7 +1023,7 @@ static int ct_process_request(struct intel_guc_ct *ct, struct ct_incoming_msg *r
 	return 0;
 }
 
-static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
+static bool ct_process_incoming_requests(struct intel_guc_ct *ct, struct list_head *incoming)
 {
 	unsigned long flags;
 	struct ct_incoming_msg *request;

@@ -1031,11 +1031,11 @@ static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
 	int err;
 
 	spin_lock_irqsave(&ct->requests.lock, flags);
-	request = list_first_entry_or_null(&ct->requests.incoming,
+	request = list_first_entry_or_null(incoming,
 					   struct ct_incoming_msg, link);
 	if (request)
 		list_del(&request->link);
-	done = !!list_empty(&ct->requests.incoming);
+	done = !!list_empty(incoming);
 	spin_unlock_irqrestore(&ct->requests.lock, flags);
 
 	if (!request)

@@ -1058,7 +1058,7 @@ static void ct_incoming_request_worker_func(struct work_struct *w)
 	bool done;
 
 	do {
-		done = ct_process_incoming_requests(ct);
+		done = ct_process_incoming_requests(ct, &ct->requests.incoming);
 	} while (!done);
 }