Sometimes opt for wbinvd over clflush

When the original drm code was written there were no centralized functions for doing a coordinated wbinvd across all CPUs. Now (since 2010) there are, so use them instead of rolling a new one.

Cc Intel GFX <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

Quoted 6 lines · Ben Widawsky

When the original drm code was written there were no centralized functions for doing a coordinated wbinvd across all CPUs. Now (since 2010) there are, so use them instead of rolling a new one.

Cc Intel GFX <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

Reviewed-by Chris Wilson <[email protected]>

From: Ben Widawsky <[email protected]>

When the original drm code was written there were no centralized functions for doing a coordinated wbinvd across all CPUs. Now (since 2010) there are, so use them instead of rolling a new one.

v2: On x86 UP systems the wbinvd_on_all_cpus() is defined as a static inline in smp.h. We must therefore include this file so we don't get compiler errors. This error was found by 0-DAY kernel test infrastructure. We only need this for x86.

Cc Intel GFX <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

Reviewed-by Chris Wilson <[email protected]> (v1)

---
 drivers/gpu/drm/drm_cache.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

drivers/gpu/drm/drm_cache.c +4 -9

index a6b6906..9a62d7a 100644
@@ -32,6 +32,7 @@
 #include <drm/drmP.h>
 
 #if defined(CONFIG_X86)
+#include <asm/smp.h>
 
 /*
  * clflushopt is an unordered instruction which needs fencing with mfence or

@@ -64,12 +65,6 @@ static void drm_cache_flush_clflush(struct page *pages[],
 		drm_clflush_page(*pages++);
 	mb();
 }
-
-static void
-drm_clflush_ipi_handler(void *null)
-{
-	wbinvd();
-}
 #endif
 
 void

@@ -82,7 +77,7 @@ drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 		return;
 	}
 
-	if (on_each_cpu(drm_clflush_ipi_handler, NULL, 1) != 0)
+	if (wbinvd_on_all_cpus())
 		printk(KERN_ERR "Timed out waiting for cache flush.\n");
 
 #elif defined(__powerpc__)

@@ -121,7 +116,7 @@ drm_clflush_sg(struct sg_table *st)
 		return;
 	}
 
-	if (on_each_cpu(drm_clflush_ipi_handler, NULL, 1) != 0)
+	if (wbinvd_on_all_cpus())
 		printk(KERN_ERR "Timed out waiting for cache flush.\n");
 #else
 	printk(KERN_ERR "Architecture has no drm_cache.c support\n");

@@ -144,7 +139,7 @@ drm_clflush_virt_range(void *addr, unsigned long length)
 		return;
 	}
 
-	if (on_each_cpu(drm_clflush_ipi_handler, NULL, 1) != 0)
+	if (wbinvd_on_all_cpus())
 		printk(KERN_ERR "Timed out waiting for cache flush.\n");
 #else
 	printk(KERN_ERR "Architecture has no drm_cache.c support\n");

Signature · 6 lines

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Any GEM driver which has very large objects and a slow CPU is subject to very long waits simply for clflushing incoherent objects. Generally, each individual object is not a problem, but if you have very large objects, or very many objects, the flushing begins to show up in profiles. Because on x86 we know the cache size, we can easily determine when an object will use all the cache, and forego iterating over each cacheline.

We need to be careful when using wbinvd. wbinvd() is itself potentially slow because it requires synchronizing the flush across all CPUs so they have a coherent view of memory. This can result in either stalling work being done on other CPUs, or this call itself stalling while waiting for a CPU to accept the interrupt. Also, wbinvd() also has the downside of invalidating all cachelines, so we don't want to use it unless we're sure we already own most of the cachelines.

The current algorithm is very naive. I think it can be tweaked more, and it would be good if someone else gave it some thought. I am pretty confident in i915, we can even skip the IPI in the execbuf path with minimal code change (or perhaps just some verifying of the existing code). It would be nice to hear what other developers who depend on this code think.

Cc Intel GFX <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/drm_cache.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_cache.c +16 -2

index d7797e8..6009c2d 100644
@@ -64,6 +64,20 @@ static void drm_cache_flush_clflush(struct page *pages[],
                drm_clflush_page(*pages++);
        mb();
 }
+
+static bool
+drm_cache_should_clflush(unsigned long num_pages)
+{
+       const int cache_size = boot_cpu_data.x86_cache_size;
+
+       /* For now the algorithm simply checks if the number of pages to be
+        * flushed is greater than the entire system cache. One could make the
+        * function more aware of the actual system (ie. if SMP, how large is
+        * the cache, CPU freq. etc. All those help to determine when to
+        * wbinvd() */
+       WARN_ON_ONCE(!cache_size);
+       return !cache_size || num_pages < (cache_size >> 2);
+}
 #endif
 
 void

@@ -71,7 +85,7 @@ drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 {
 
 #if defined(CONFIG_X86)
-       if (cpu_has_clflush) {
+       if (cpu_has_clflush && drm_cache_should_clflush(num_pages)) {
                drm_cache_flush_clflush(pages, num_pages);
                return;
        }

@@ -104,7 +118,7 @@ void
 drm_clflush_sg(struct sg_table *st)
 {
 #if defined(CONFIG_X86)
-       if (cpu_has_clflush) {
+       if (cpu_has_clflush && drm_cache_should_clflush(st->nents)) {
                struct sg_page_iter sg_iter;
 
                mb();

Quoted 69 lines · Ben Widawsky

Any GEM driver which has very large objects and a slow CPU is subject to very long waits simply for clflushing incoherent objects. Generally, each individual object is not a problem, but if you have very large objects, or very many objects, the flushing begins to show up in profiles. Because on x86 we know the cache size, we can easily determine when an object will use all the cache, and forego iterating over each cacheline.

We need to be careful when using wbinvd. wbinvd() is itself potentially slow because it requires synchronizing the flush across all CPUs so they have a coherent view of memory. This can result in either stalling work being done on other CPUs, or this call itself stalling while waiting for a CPU to accept the interrupt. Also, wbinvd() also has the downside of invalidating all cachelines, so we don't want to use it unless we're sure we already own most of the cachelines.

The current algorithm is very naive. I think it can be tweaked more, and it would be good if someone else gave it some thought. I am pretty confident in i915, we can even skip the IPI in the execbuf path with minimal code change (or perhaps just some verifying of the existing code). It would be nice to hear what other developers who depend on this code think.

Cc Intel GFX <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/drm_cache.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_cache.c +16 -2

index d7797e8..6009c2d 100644
@@ -64,6 +64,20 @@ static void drm_cache_flush_clflush(struct page *pages[],
                drm_clflush_page(*pages++);
        mb();
 }
+
+static bool
+drm_cache_should_clflush(unsigned long num_pages)
+{
+       const int cache_size = boot_cpu_data.x86_cache_size;
+
+       /* For now the algorithm simply checks if the number of pages to be
+        * flushed is greater than the entire system cache. One could make the
+        * function more aware of the actual system (ie. if SMP, how large is
+        * the cache, CPU freq. etc. All those help to determine when to
+        * wbinvd() */
+       WARN_ON_ONCE(!cache_size);
+       return !cache_size || num_pages < (cache_size >> 2);
+}
 #endif
 
 void

@@ -71,7 +85,7 @@ drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 {
 
 #if defined(CONFIG_X86)
-       if (cpu_has_clflush) {
+       if (cpu_has_clflush && drm_cache_should_clflush(num_pages)) {
                drm_cache_flush_clflush(pages, num_pages);
                return;
        }

@@ -104,7 +118,7 @@ void
 drm_clflush_sg(struct sg_table *st)
 {
 #if defined(CONFIG_X86)
-       if (cpu_has_clflush) {
+       if (cpu_has_clflush && drm_cache_should_clflush(st->nents)) {
                struct sg_page_iter sg_iter;
 

drivers/gpu/drm/drm_cache.c void · line 145

Quoted 7 lines

drivers/gpu/drm/drm_cache.c +1 -1

                mb();

@@ -128,7 +142,7 @@ void
 drm_clflush_virt_range(void *addr, unsigned long length)
 {
 #if defined(CONFIG_X86)
-       if (cpu_has_clflush) {
+       if (cpu_has_clflush && drm_cache_should_clflush(length / PAGE_SIZE)) {

If length isn't a multiple of page size, isn't this ignoring the remainder? Should it be rounding length up to the next multiple of PAGE_SIZE, like ROUND_UP_TO?

Any GEM driver which has very large objects and a slow CPU is subject to very long waits simply for clflushing incoherent objects. Generally, each individual object is not a problem, but if you have very large objects, or very many objects, the flushing begins to show up in profiles. Because on x86 we know the cache size, we can easily determine when an object will use all the cache, and forego iterating over each cacheline.

We need to be careful when using wbinvd. wbinvd() is itself potentially slow because it requires synchronizing the flush across all CPUs so they have a coherent view of memory. This can result in either stalling work being done on other CPUs, or this call itself stalling while waiting for a CPU to accept the interrupt. Also, wbinvd() also has the downside of invalidating all cachelines, so we don't want to use it unless we're sure we already own most of the cachelines.

The current algorithm is very naive. I think it can be tweaked more, and it would be good if someone else gave it some thought. I am pretty confident in i915, we can even skip the IPI in the execbuf path with minimal code change (or perhaps just some verifying of the existing code). It would be nice to hear what other developers who depend on this code think.

Cc Intel GFX <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/drm_cache.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_cache.c +0 -0

index d7797e8..6009c2d 100644

One of the things wbinvd is considered evil for is that it blocks the CPU for an indeterminate amount of time - upsetting latency critcial aspects of the OS. For example, the x86/mm has similar code to use wbinvd for large clflushes that caused a bit of controversy with RT:

http://linux-kernel.2935.n7.nabble.com/PATCH-x86-Use-clflush-instead-of-wbinvd-whenever-possible-when-changing-mapping-td493751.html

and also the use of wbinvd in the nvidia driver has also been noted as evil by RT folks.

However as the wbinvd still exists, it can't be all that bad...

Quoted 5 lines · Chris Wilson

One of the things wbinvd is considered evil for is that it blocks the CPU for an indeterminate amount of time - upsetting latency critcial aspects of the OS. For example, the x86/mm has similar code to use wbinvd for large clflushes that caused a bit of controversy with RT:

Quoted 6 lines

http://linux-kernel.2935.n7.nabble.com/PATCH-x86-Use-clflush-instead-of-wbinvd-whenever-possible-when-changing-mapping-td493751.html

and also the use of wbinvd in the nvidia driver has also been noted as evil by RT folks.

However as the wbinvd still exists, it can't be all that bad...

Yeah there are definitely tradeoffs here. In this particular case, we're trying to flush out a ~140M object on every frame, which just seems silly.

There's definitely room for optimization in Mesa too; avoiding a mapping that marks a large bo as dirty would be good, but if we improve the kernel in this area, even sloppy apps and existing binaries will speed up.

Maybe we could apply this only on !llc systems or something? I wonder how much wbinvd performance varies across microarchitectures; maybe Thomas's issue isn't really relevant anymore (at least one can hope).

When digging into this, I found that an optimization to remove the IPI for wbinvd was clobbered during a merge; maybe that should be resurrected too. Surely a single, global wbinvd is sufficient; we don't need to do n_cpus^2 wbinvd + the associated invalidation bus signals here...

Alternately, we could insert some delays into this path just to make it extra clear to userspace that they really shouldn't be hitting this in the common case (and provide some additional interfaces to let them avoid it by allowing flushing and dirty management in userspace).

Jesse

If we're moving a bunch of buffers from the CPU domain to the GPU domain, and we've already blown out the entire cache via a wbinvd, there is nothing more to do.

With this and the previous patches, I am seeing a 3x FPS increase on a certain benchmark which uses a giant 2d array texture. Unless I missed something in the code, it should only effect non-LLC i915 platforms.

I haven't yet run any numbers for other benchmarks, nor have I attempted to check if various conformance tests still pass.

NOTE: As mentioned in the previous patch, if one can easily obtain the largest buffer and attempt to flush it first, the results would be even more desirable.

Cc DRI Development <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h            |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c            | 12 +++++-------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  8 +++++---
 drivers/gpu/drm/i915/intel_lrc.c           |  8 +++++---
 4 files changed, 17 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -1

index d68c75f..fdb92a3 100644
@@ -2642,7 +2642,8 @@ static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv)
 }
 
 void i915_gem_reset(struct drm_device *dev);
-bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
+enum drm_cache_flush
+i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init(struct drm_device *dev);
 int i915_gem_init_rings(struct drm_device *dev);

drivers/gpu/drm/i915/i915_gem.c +5 -7

index de241eb..3746738 100644
@@ -3608,7 +3608,7 @@ err_unpin:
 	return vma;
 }
 
-bool
+enum drm_cache_flush
 i915_gem_clflush_object(struct drm_i915_gem_object *obj,
 			bool force)
 {

@@ -3617,14 +3617,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
 	 * again at bind time.
 	 */
 	if (obj->pages == NULL)
-		return false;
+		return DRM_CACHE_FLUSH_NONE;
 
 	/*
 	 * Stolen memory is always coherent with the GPU as it is explicitly
 	 * marked as wc by the system, or the system is cache-coherent.
 	 */
 	if (obj->stolen || obj->phys_handle)
-		return false;
+		return DRM_CACHE_FLUSH_NONE;
 
 	/* If the GPU is snooping the contents of the CPU cache,
 	 * we do not need to manually clear the CPU cache lines.  However,

@@ -3635,12 +3635,10 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
 	 * tracking.
 	 */
 	if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
-		return false;
+		return DRM_CACHE_FLUSH_NONE;
 
 	trace_i915_gem_object_clflush(obj);
-	drm_clflush_sg(obj->pages);
-
-	return true;
+	return drm_clflush_sg(obj->pages);
 }
 
 /** Flushes the GTT write domain for the object if it's dirty. */

drivers/gpu/drm/i915/i915_gem_execbuffer.c +5 -3

index 0c25f62..e8eb9e9 100644
@@ -827,7 +827,7 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
 {
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
-	bool flush_chipset = false;
+	enum drm_cache_flush flush_chipset = DRM_CACHE_FLUSH_NONE;
 	int ret;
 
 	list_for_each_entry(vma, vmas, exec_list) {

@@ -836,8 +836,10 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
 		if (ret)
 			return ret;
 
-		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
-			flush_chipset |= i915_gem_clflush_object(obj, false);
+		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU &&
+		    flush_chipset != DRM_CACHE_FLUSH_WBINVD) {
+			flush_chipset = i915_gem_clflush_object(obj, false);
+		}
 
 		flush_domains |= obj->base.write_domain;
 	}

drivers/gpu/drm/i915/intel_lrc.c +5 -3

index 89b5577..a6c6ebd 100644
@@ -611,7 +611,7 @@ static int execlists_move_to_gpu(struct intel_ringbuffer *ringbuf,
 	struct intel_engine_cs *ring = ringbuf->ring;
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
-	bool flush_chipset = false;
+	enum drm_cache_flush flush_chipset = DRM_CACHE_FLUSH_NONE;
 	int ret;
 
 	list_for_each_entry(vma, vmas, exec_list) {

@@ -621,8 +621,10 @@ static int execlists_move_to_gpu(struct intel_ringbuffer *ringbuf,
 		if (ret)
 			return ret;
 
-		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
-			flush_chipset |= i915_gem_clflush_object(obj, false);
+		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU &&
+		    flush_chipset != DRM_CACHE_FLUSH_WBINVD) {
+			flush_chipset = i915_gem_clflush_object(obj, false);
+		}
 
 		flush_domains |= obj->base.write_domain;
 	}

Signature · 6 lines

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 8 lines · Ben Widawsky

If we're moving a bunch of buffers from the CPU domain to the GPU domain, and we've already blown out the entire cache via a wbinvd, there is nothing more to do.

With this and the previous patches, I am seeing a 3x FPS increase on a certain benchmark which uses a giant 2d array texture. Unless I missed something in the code, it should only effect non-LLC i915 platforms.

Quoted 5 lines

I haven't yet run any numbers for other benchmarks, nor have I attempted to check if various conformance tests still pass.

NOTE: As mentioned in the previous patch, if one can easily obtain the largest buffer and attempt to flush it first, the results would be even more desirable.

So even with that optimization if you only have tons of small buffers that need to be flushed you'd still take the clflush path for every single one.

How difficult would it to calculate the total size to be flushed first, and then make the clflush vs. wbinvd decision base on that?

Quoted 15 lines · Ville Syrjälä

Quoted 8 lines · Ben Widawsky

If we're moving a bunch of buffers from the CPU domain to the GPU domain, and we've already blown out the entire cache via a wbinvd, there is nothing more to do.

With this and the previous patches, I am seeing a 3x FPS increase on a certain benchmark which uses a giant 2d array texture. Unless I missed something in the code, it should only effect non-LLC i915 platforms.

Quoted 5 lines

I haven't yet run any numbers for other benchmarks, nor have I attempted to check if various conformance tests still pass.

NOTE: As mentioned in the previous patch, if one can easily obtain the largest buffer and attempt to flush it first, the results would be even more desirable.

Quoted 7 lines

So even with that optimization if you only have tons of small buffers that need to be flushed you'd still take the clflush path for every single one.

How difficult would it to calculate the total size to be flushed first, and then make the clflush vs. wbinvd decision base on that?

I'll write the patch and send it to Eero for test.

It's not hard, and I think that's a good idea as well. One reason I didn't put such code in this series is that moves away from a global DRM solution (and like I said in the cover-letter, I am fine with that). Implementing this, I think in the i915 code we'd just iterate through the BOs until we got to a certain threshold, then just call wbinvd() from i915 and not even both with drm_cache. You could also maybe try to shorcut if there are more than X buffers.

However, for what you describe, I think it might make more sense to let userspace specify an execbuf flag to do the wbinvd(). Userspace can trivially determine such info, it prevents having to iterate through the buffers an extra time in the kernel.

Quoted 24 lines · Ben Widawsky

Quoted 15 lines · Ville Syrjälä

Quoted 8 lines · Ben Widawsky

If we're moving a bunch of buffers from the CPU domain to the GPU domain, and we've already blown out the entire cache via a wbinvd, there is nothing more to do.

With this and the previous patches, I am seeing a 3x FPS increase on a certain benchmark which uses a giant 2d array texture. Unless I missed something in the code, it should only effect non-LLC i915 platforms.

Quoted 5 lines

I haven't yet run any numbers for other benchmarks, nor have I attempted to check if various conformance tests still pass.

NOTE: As mentioned in the previous patch, if one can easily obtain the largest buffer and attempt to flush it first, the results would be even more desirable.

Quoted 7 lines

So even with that optimization if you only have tons of small buffers that need to be flushed you'd still take the clflush path for every single one.

How difficult would it to calculate the total size to be flushed first, and then make the clflush vs. wbinvd decision base on that?

Quoted 8 lines

I'll write the patch and send it to Eero for test.

It's not hard, and I think that's a good idea as well. One reason I didn't put such code in this series is that moves away from a global DRM solution (and like I said in the cover-letter, I am fine with that). Implementing this, I think in the i915 code we'd just iterate through the BOs until we got to a certain threshold, then just call wbinvd() from i915 and not even both with drm_cache. You could also maybe try to shorcut if there are more than X buffers.

I don't mind an i915 specific solution (we have them already in many places). So will wait for the results of this experiments before merging more patches.

Quoted 34 lines · Daniel Vetter

Quoted 32 lines · Ben Widawsky

Quoted 15 lines · Ville Syrjälä

Quoted 8 lines · Ben Widawsky

If we're moving a bunch of buffers from the CPU domain to the GPU domain, and we've already blown out the entire cache via a wbinvd, there is nothing more to do.

With this and the previous patches, I am seeing a 3x FPS increase on a certain benchmark which uses a giant 2d array texture. Unless I missed something in the code, it should only effect non-LLC i915 platforms.

Quoted 5 lines

I haven't yet run any numbers for other benchmarks, nor have I attempted to check if various conformance tests still pass.

NOTE: As mentioned in the previous patch, if one can easily obtain the largest buffer and attempt to flush it first, the results would be even more desirable.

Quoted 7 lines

So even with that optimization if you only have tons of small buffers that need to be flushed you'd still take the clflush path for every single one.

How difficult would it to calculate the total size to be flushed first, and then make the clflush vs. wbinvd decision base on that?

I'll write the patch and send it to Eero for test.

It's not hard, and I think that's a good idea as well. One reason I didn't put such code in this series is that moves away from a global DRM solution (and like I said in the cover-letter, I am fine with that). Implementing this, I think in the i915 code we'd just iterate through the BOs until we got to a certain threshold, then just call wbinvd() from i915 and not even both with drm_cache. You could also maybe try to shorcut if there are more than X buffers.

Quoted 3 lines

I don't mind an i915 specific solution (we have them already in many places). So will wait for the results of this experiments before merging more patches.

I actually think an i915 specific solution is required, as the making drm_clflush_pages autoselect is likely to cause regressions on unaware drivers (e.g. anything that has a reservation loop in execbuf like i915).

I do think that execbuf regularly hitting clflush is something to be concerned about in userspace (even a wbinvd every execbuf is not going to be nice, even an mfence per execbuf shows up on profiles!), but we do have to occassionally flush large objects like framebuffers which probably would be faster using wbinvd.
-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Quoted 39 lines · Chris Wilson

Quoted 37 lines · Daniel Vetter

Quoted 24 lines · Ben Widawsky

Quoted 15 lines · Ville Syrjälä

Quoted 8 lines · Ben Widawsky

If we're moving a bunch of buffers from the CPU domain to the GPU domain, and we've already blown out the entire cache via a wbinvd, there is nothing more to do.

With this and the previous patches, I am seeing a 3x FPS increase on a certain benchmark which uses a giant 2d array texture. Unless I missed something in the code, it should only effect non-LLC i915 platforms.

Quoted 5 lines

I haven't yet run any numbers for other benchmarks, nor have I attempted to check if various conformance tests still pass.

NOTE: As mentioned in the previous patch, if one can easily obtain the largest buffer and attempt to flush it first, the results would be even more desirable.

Quoted 7 lines

So even with that optimization if you only have tons of small buffers that need to be flushed you'd still take the clflush path for every single one.

How difficult would it to calculate the total size to be flushed first, and then make the clflush vs. wbinvd decision base on that?

Quoted 8 lines

I'll write the patch and send it to Eero for test.

It's not hard, and I think that's a good idea as well. One reason I didn't put such code in this series is that moves away from a global DRM solution (and like I said in the cover-letter, I am fine with that). Implementing this, I think in the i915 code we'd just iterate through the BOs until we got to a certain threshold, then just call wbinvd() from i915 and not even both with drm_cache. You could also maybe try to shorcut if there are more than X buffers.

I don't mind an i915 specific solution (we have them already in many places). So will wait for the results of this experiments before merging more patches.

Quoted 3 lines

I actually think an i915 specific solution is required, as the making drm_clflush_pages autoselect is likely to cause regressions on unaware drivers (e.g. anything that has a reservation loop in execbuf like i915).

Assuming the stall is gone as Jesse said in the other thread, I can't envision a scenario where wbinvd would do worse on large objects.

Quoted 44 lines · Ben Widawsky

Quoted 42 lines · Chris Wilson

Quoted 34 lines · Daniel Vetter

Quoted 24 lines · Ben Widawsky

Quoted 15 lines · Ville Syrjälä

Quoted 8 lines · Ben Widawsky

If we're moving a bunch of buffers from the CPU domain to the GPU domain, and we've already blown out the entire cache via a wbinvd, there is nothing more to do.

With this and the previous patches, I am seeing a 3x FPS increase on a certain benchmark which uses a giant 2d array texture. Unless I missed something in the code, it should only effect non-LLC i915 platforms.

Quoted 5 lines

I haven't yet run any numbers for other benchmarks, nor have I attempted to check if various conformance tests still pass.

NOTE: As mentioned in the previous patch, if one can easily obtain the largest buffer and attempt to flush it first, the results would be even more desirable.

Quoted 7 lines

So even with that optimization if you only have tons of small buffers that need to be flushed you'd still take the clflush path for every single one.

How difficult would it to calculate the total size to be flushed first, and then make the clflush vs. wbinvd decision base on that?

Quoted 8 lines

I'll write the patch and send it to Eero for test.

It's not hard, and I think that's a good idea as well. One reason I didn't put such code in this series is that moves away from a global DRM solution (and like I said in the cover-letter, I am fine with that). Implementing this, I think in the i915 code we'd just iterate through the BOs until we got to a certain threshold, then just call wbinvd() from i915 and not even both with drm_cache. You could also maybe try to shorcut if there are more than X buffers.

Quoted 3 lines

I don't mind an i915 specific solution (we have them already in many places). So will wait for the results of this experiments before merging more patches.

I actually think an i915 specific solution is required, as the making drm_clflush_pages autoselect is likely to cause regressions on unaware drivers (e.g. anything that has a reservation loop in execbuf like i915).

Quoted 2 lines

Assuming the stall is gone as Jesse said in the other thread, I can't envision a scenario where wbinvd would do worse on large objects.

It is the multiple wbinvd performed at each execbuffer that is worrisome. -Chris

Quoted 19 lines · Chris Wilson

Quoted 18 lines · Ben Widawsky

Quoted 16 lines · Chris Wilson

Quoted 11 lines · Daniel Vetter

Quoted 6 lines · Ben Widawsky

It's not hard, and I think that's a good idea as well. One reason I didn't put such code in this series is that moves away from a global DRM solution (and like I said in the cover-letter, I am fine with that). Implementing this, I think in the i915 code we'd just iterate through the BOs until we got to a certain threshold, then just call wbinvd() from i915 and not even both with drm_cache. You could also maybe try to shorcut if there are more than X buffers.

I don't mind an i915 specific solution (we have them already in many places). So will wait for the results of this experiments before merging more patches.

I actually think an i915 specific solution is required, as the making drm_clflush_pages autoselect is likely to cause regressions on unaware drivers (e.g. anything that has a reservation loop in execbuf like i915).

Quoted 4 lines

Quoted 2 lines

Assuming the stall is gone as Jesse said in the other thread, I can't envision a scenario where wbinvd would do worse on large objects.

It is the multiple wbinvd performed at each execbuffer that is worrisome.

This patch attempts to avoid that by dropping all flushing after the first WBINVD.

Quoted 23 lines · Ben Widawsky

Quoted 22 lines · Chris Wilson

Quoted 20 lines · Ben Widawsky

Quoted 9 lines · Chris Wilson

Quoted 8 lines · Daniel Vetter

Quoted 6 lines · Ben Widawsky

It's not hard, and I think that's a good idea as well. One reason I didn't put such code in this series is that moves away from a global DRM solution (and like I said in the cover-letter, I am fine with that). Implementing this, I think in the i915 code we'd just iterate through the BOs until we got to a certain threshold, then just call wbinvd() from i915 and not even both with drm_cache. You could also maybe try to shorcut if there are more than X buffers.

Quoted 7 lines

Quoted 3 lines

I don't mind an i915 specific solution (we have them already in many places). So will wait for the results of this experiments before merging more patches.

I actually think an i915 specific solution is required, as the making drm_clflush_pages autoselect is likely to cause regressions on unaware drivers (e.g. anything that has a reservation loop in execbuf like i915).

Assuming the stall is gone as Jesse said in the other thread, I can't envision a scenario where wbinvd would do worse on large objects.

Quoted 4 lines

Quoted 1 line

It is the multiple wbinvd performed at each execbuffer that is worrisome.

This patch attempts to avoid that by dropping all flushing after the first WBINVD.

But you can't make the central change to drm_clflush_* without driver specific workarounds like this patch...