[PATCH 0/6] Prefer wbinvd() where appropriate

[PATCH 0/6] Prefer wbinvd() where appropriate

[Ben Widawsky]

A while back we had a workload which severely suffered from excessive
clflushing. I sent out a patch series which solved this generically in DRM core,
but that had the unfortunate side effect of possibly regressing non-intel
platforms.

I've re-spun the series to only take this shortcut when on an Intel platform.
This series is untested by me since we've now worked around the problem in a
different way within mesa. Jesse asked me to send this out, though I don't have
time to see if it's still useful (until my mesa patches are merged, it would be
useful, but afterwards, I do not know). I also didn't check that I rebased
things properly.

Do what you want with them...


Ben Widawsky (6):
  drm/i915: Remove the useless flush_chipset
  drm/i915: Pass eb_vmas to execbuffer implementations
  drm/i915: Opportunistically reduce flushing at execbuf
  drm/i915: Add debugfs knobs for wbinvd threshold
  drm/i915: Extract checking the necessity of flush
  drm/i915: obey wbinvd threshold in more places

 drivers/gpu/drm/i915/i915_debugfs.c        | 34 +++++++++++
 drivers/gpu/drm/i915/i915_drv.h            | 25 +++++++-
 drivers/gpu/drm/i915/i915_gem.c            | 96 ++++++++++++++++++++----------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 46 +++++++-------
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 13 +++-
 drivers/gpu/drm/i915/intel_lrc.c           | 21 ++++---
 drivers/gpu/drm/i915/intel_lrc.h           |  3 +-
 7 files changed, 172 insertions(+), 66 deletions(-)

-- 
2.3.0

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

[PATCH 1/6] drm/i915: Remove the useless flush_chipset

[Ben Widawsky]

flush_chipset makes no sense with execlists because the former is for strictly
prior to gen6, while the latter is for gen >= 8

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 70e449b..e727217 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -576,7 +576,6 @@ 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;
 	int ret;
 
 	list_for_each_entry(vma, vmas, exec_list) {
@@ -587,7 +586,7 @@ static int execlists_move_to_gpu(struct intel_ringbuffer *ringbuf,
 			return ret;
 
 		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
-			flush_chipset |= i915_gem_clflush_object(obj, false);
+			i915_gem_clflush_object(obj, false);
 
 		flush_domains |= obj->base.write_domain;
 	}
-- 
2.3.0

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

[PATCH 2/6] drm/i915: Pass eb_vmas to execbuffer implementations

[Ben Widawsky]

The role of eb_vmas continues to grow here as it becomes the proper
encapsulation for the data passed to the various execution function.

Next patch makes use of it... This patch was initially part of the next patch,
but got split out after I had found a bug that convinced me the two should be
separate.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h            | 13 +++++++++++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 26 ++++++++++----------------
 drivers/gpu/drm/i915/intel_lrc.c           |  8 +++++---
 drivers/gpu/drm/i915/intel_lrc.h           |  3 ++-
 4 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4179b90..90ff6aa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1641,6 +1641,15 @@ struct i915_workarounds {
 	u32 count;
 };
 
+struct eb_vmas {
+	struct list_head vmas;
+	int and;
+	union {
+		struct i915_vma *lut[0];
+		struct hlist_head buckets[0];
+	};
+};
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1896,7 +1905,7 @@ struct drm_i915_private {
 				  struct intel_engine_cs *ring,
 				  struct intel_context *ctx,
 				  struct drm_i915_gem_execbuffer2 *args,
-				  struct list_head *vmas,
+				  struct eb_vmas *eb,
 				  struct drm_i915_gem_object *batch_obj,
 				  u64 exec_start, u32 flags);
 		int (*init_rings)(struct drm_device *dev);
@@ -2626,7 +2635,7 @@ int i915_gem_ringbuffer_submission(struct drm_device *dev,
 				   struct intel_engine_cs *ring,
 				   struct intel_context *ctx,
 				   struct drm_i915_gem_execbuffer2 *args,
-				   struct list_head *vmas,
+				   struct eb_vmas *eb,
 				   struct drm_i915_gem_object *batch_obj,
 				   u64 exec_start, u32 flags);
 int i915_gem_execbuffer(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index b773368..13ed13e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -41,15 +41,6 @@
 
 #define BATCH_OFFSET_BIAS (256*1024)
 
-struct eb_vmas {
-	struct list_head vmas;
-	int and;
-	union {
-		struct i915_vma *lut[0];
-		struct hlist_head buckets[0];
-	};
-};
-
 static struct eb_vmas *
 eb_create(struct drm_i915_gem_execbuffer2 *args)
 {
@@ -617,10 +608,11 @@ eb_vma_misplaced(struct i915_vma *vma)
 
 static int
 i915_gem_execbuffer_reserve(struct intel_engine_cs *ring,
-			    struct list_head *vmas,
+			    struct eb_vmas *eb,
 			    bool *need_relocs)
 {
 	struct drm_i915_gem_object *obj;
+	struct list_head *vmas = &eb->vmas;
 	struct i915_vma *vma;
 	struct i915_address_space *vm;
 	struct list_head ordered_vmas;
@@ -803,7 +795,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 		goto err;
 
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, &need_relocs);
+	ret = i915_gem_execbuffer_reserve(ring, eb, &need_relocs);
 	if (ret)
 		goto err;
 
@@ -829,8 +821,9 @@ err:
 
 static int
 i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
-				struct list_head *vmas)
+				struct eb_vmas *eb)
 {
+	struct list_head *vmas = &eb->vmas;
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
 	bool flush_chipset = false;
@@ -1136,12 +1129,13 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 			       struct intel_engine_cs *ring,
 			       struct intel_context *ctx,
 			       struct drm_i915_gem_execbuffer2 *args,
-			       struct list_head *vmas,
+			       struct eb_vmas *eb,
 			       struct drm_i915_gem_object *batch_obj,
 			       u64 exec_start, u32 flags)
 {
 	struct drm_clip_rect *cliprects = NULL;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct list_head *vmas = &eb->vmas;
 	u64 exec_len;
 	int instp_mode;
 	u32 instp_mask;
@@ -1190,7 +1184,7 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 		}
 	}
 
-	ret = i915_gem_execbuffer_move_to_gpu(ring, vmas);
+	ret = i915_gem_execbuffer_move_to_gpu(ring, eb);
 	if (ret)
 		goto error;
 
@@ -1463,7 +1457,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, &need_relocs);
+	ret = i915_gem_execbuffer_reserve(ring, eb, &need_relocs);
 	if (ret)
 		goto err;
 
@@ -1527,7 +1521,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		exec_start += i915_gem_obj_offset(batch_obj, vm);
 
 	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
-				      &eb->vmas, batch_obj, exec_start, flags);
+				      eb, batch_obj, exec_start, flags);
 
 	/*
 	 * FIXME: We crucially rely upon the active tracking for the (ppgtt)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e727217..03741f9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -571,9 +571,10 @@ static int logical_ring_invalidate_all_caches(struct intel_ringbuffer *ringbuf,
 
 static int execlists_move_to_gpu(struct intel_ringbuffer *ringbuf,
 				 struct intel_context *ctx,
-				 struct list_head *vmas)
+				 struct eb_vmas *eb)
 {
 	struct intel_engine_cs *ring = ringbuf->ring;
+	struct list_head *vmas = &eb->vmas;
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
 	int ret;
@@ -621,12 +622,13 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
 			       struct intel_engine_cs *ring,
 			       struct intel_context *ctx,
 			       struct drm_i915_gem_execbuffer2 *args,
-			       struct list_head *vmas,
+			       struct eb_vmas *eb,
 			       struct drm_i915_gem_object *batch_obj,
 			       u64 exec_start, u32 flags)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
+	struct list_head *vmas = &eb->vmas;
 	int instp_mode;
 	u32 instp_mask;
 	int ret;
@@ -677,7 +679,7 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
 		return -EINVAL;
 	}
 
-	ret = execlists_move_to_gpu(ringbuf, ctx, vmas);
+	ret = execlists_move_to_gpu(ringbuf, ctx, eb);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 6f2d7da..661080bc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -80,11 +80,12 @@ void intel_lr_context_unpin(struct intel_engine_cs *ring,
 
 /* Execlists */
 int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
+struct eb_vmas;
 int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
 			       struct intel_engine_cs *ring,
 			       struct intel_context *ctx,
 			       struct drm_i915_gem_execbuffer2 *args,
-			       struct list_head *vmas,
+			       struct eb_vmas *eb,
 			       struct drm_i915_gem_object *batch_obj,
 			       u64 exec_start, u32 flags);
 u32 intel_execlists_ctx_id(struct drm_i915_gem_object *ctx_obj);
-- 
2.3.0

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

[PATCH 3/6] drm/i915: Opportunistically reduce flushing at execbuf

[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.

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

v2: Rewrite the patch to be i915 only
Obtain whether or not we wbinvd up front.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h            |  8 ++++++++
 drivers/gpu/drm/i915/i915_gem.c            | 11 +++++------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++++++++++++++++----
 drivers/gpu/drm/i915/intel_lrc.c           | 10 ++++++++--
 4 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 90ff6aa..5d2f62d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1643,6 +1643,7 @@ struct i915_workarounds {
 
 struct eb_vmas {
 	struct list_head vmas;
+	bool do_wbinvd;
 	int and;
 	union {
 		struct i915_vma *lut[0];
@@ -1913,6 +1914,8 @@ struct drm_i915_private {
 		void (*stop_ring)(struct intel_engine_cs *ring);
 	} gt;
 
+	size_t wbinvd_threshold;
+
 	uint32_t request_uniq;
 
 	/*
@@ -2810,6 +2813,11 @@ 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);
+static inline bool cpu_cache_is_coherent(struct drm_device *dev,
+					 enum i915_cache_level level)
+{
+	return HAS_LLC(dev) || level != I915_CACHE_NONE;
+}
 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);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fc81889..5bfb332 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -61,12 +61,6 @@ static int i915_gem_shrinker_oom(struct notifier_block *nb,
 				 void *ptr);
 static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
 
-static bool cpu_cache_is_coherent(struct drm_device *dev,
-				  enum i915_cache_level level)
-{
-	return HAS_LLC(dev) || level != I915_CACHE_NONE;
-}
-
 static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
 {
 	if (!cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
@@ -4878,6 +4872,11 @@ int i915_gem_init(struct drm_device *dev)
 		dev_priv->gt.stop_ring = intel_logical_ring_stop;
 	}
 
+	dev_priv->wbinvd_threshold = boot_cpu_data.x86_cache_size << 10;
+	/* Pick a high default in the unlikely case we got nothing */
+	if (!dev_priv->wbinvd_threshold)
+		dev_priv->wbinvd_threshold = (8 << 20);
+
 	ret = i915_gem_init_userptr(dev);
 	if (ret)
 		goto out_unlock;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 13ed13e..56f9268 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -50,7 +50,7 @@ eb_create(struct drm_i915_gem_execbuffer2 *args)
 		unsigned size = args->buffer_count;
 		size *= sizeof(struct i915_vma *);
 		size += sizeof(struct eb_vmas);
-		eb = kmalloc(size, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
+		eb = kzalloc(size, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 	}
 
 	if (eb == NULL) {
@@ -78,6 +78,7 @@ eb_reset(struct eb_vmas *eb)
 {
 	if (eb->and >= 0)
 		memset(eb->buckets, 0, (eb->and+1)*sizeof(struct hlist_head));
+	eb->do_wbinvd = false;
 }
 
 static int
@@ -154,6 +155,11 @@ eb_lookup_vmas(struct eb_vmas *eb,
 			hlist_add_head(&vma->exec_node,
 				       &eb->buckets[handle & eb->and]);
 		}
+
+		if (vma->node.size >= to_i915(obj->base.dev)->wbinvd_threshold &&
+		    obj->base.write_domain & I915_GEM_DOMAIN_CPU &&
+		    !cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
+			eb->do_wbinvd = true;
 		++i;
 	}
 
@@ -826,7 +832,7 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
 	struct list_head *vmas = &eb->vmas;
 	struct i915_vma *vma;
 	uint32_t flush_domains = 0;
-	bool flush_chipset = false;
+	bool flush_chipset = eb->do_wbinvd;
 	int ret;
 
 	list_for_each_entry(vma, vmas, exec_list) {
@@ -835,12 +841,18 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
 		if (ret)
 			return ret;
 
+		flush_domains |= obj->base.write_domain;
+
+		if (eb->do_wbinvd)
+			continue;
+
 		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
 			flush_chipset |= i915_gem_clflush_object(obj, false);
-
-		flush_domains |= obj->base.write_domain;
 	}
 
+	if (eb->do_wbinvd)
+		wbinvd();
+
 	if (flush_chipset)
 		i915_gem_chipset_flush(ring->dev);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 03741f9..16ca4a2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -586,12 +586,18 @@ static int execlists_move_to_gpu(struct intel_ringbuffer *ringbuf,
 		if (ret)
 			return ret;
 
+		flush_domains |= obj->base.write_domain;
+
+		if (eb->do_wbinvd)
+			continue;
+
 		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
 			i915_gem_clflush_object(obj, false);
-
-		flush_domains |= obj->base.write_domain;
 	}
 
+	if (eb->do_wbinvd)
+		wbinvd();
+
 	if (flush_domains & I915_GEM_DOMAIN_GTT)
 		wmb();
 
-- 
2.3.0

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

[PATCH 4/6] drm/i915: Add debugfs knobs for wbinvd threshold

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..e0fd3ba 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4316,6 +4316,39 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops,
 			i915_cache_sharing_get, i915_cache_sharing_set,
 			"%llu\n");
 
+static int i915_wbinvd_threshold_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+	*val = dev_priv->wbinvd_threshold;
+	mutex_unlock(&dev_priv->dev->struct_mutex);
+
+	return 0;
+}
+
+static int i915_wbinvd_threshold_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+	dev_priv->wbinvd_threshold = val;
+	mutex_unlock(&dev_priv->dev->struct_mutex);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_wbinvd_threshold_fops,
+			i915_wbinvd_threshold_get, i915_wbinvd_threshold_set,
+			"%llu\n");
 static int i915_forcewake_open(struct inode *inode, struct file *file)
 {
 	struct drm_device *dev = inode->i_private;
@@ -4450,6 +4483,7 @@ static const struct i915_debugfs_files {
 	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
 	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
+	{"i915_wbinvd_threshold", &i915_wbinvd_threshold_fops},
 };
 
 void intel_display_crc_init(struct drm_device *dev)
-- 
2.3.0

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

[PATCH 5/6] drm/i915: Extract checking the necessity of flush

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 53 +++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5bfb332..4d5a69d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -69,6 +69,35 @@ static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
 	return obj->pin_display;
 }
 
+static bool
+is_cpu_flush_required(struct drm_i915_gem_object *obj)
+{
+	/* If we don't have a page list set up, then we're not pinned
+	 * to GPU, and we can ignore the cache flush because it'll happen
+	 * again at bind time.
+	 */
+	if (obj->pages == NULL)
+		return false;
+
+	/*
+	 * 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;
+
+	/* If the GPU is snooping the contents of the CPU cache,
+	 * we do not need to manually clear the CPU cache lines.  However,
+	 * the caches are only snooped when the render cache is
+	 * flushed/invalidated.  As we always have to emit invalidations
+	 * and flushes when moving into and out of the RENDER domain, correct
+	 * snooping behaviour occurs naturally as the result of our domain
+	 * tracking.
+	 */
+	return !cpu_cache_is_coherent(obj->base.dev, obj->cache_level);
+}
+
+
 static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj)
 {
 	if (obj->tiling_mode)
@@ -3615,29 +3644,7 @@ bool
 i915_gem_clflush_object(struct drm_i915_gem_object *obj,
 			bool force)
 {
-	/* If we don't have a page list set up, then we're not pinned
-	 * to GPU, and we can ignore the cache flush because it'll happen
-	 * again at bind time.
-	 */
-	if (obj->pages == NULL)
-		return false;
-
-	/*
-	 * 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;
-
-	/* If the GPU is snooping the contents of the CPU cache,
-	 * we do not need to manually clear the CPU cache lines.  However,
-	 * the caches are only snooped when the render cache is
-	 * flushed/invalidated.  As we always have to emit invalidations
-	 * and flushes when moving into and out of the RENDER domain, correct
-	 * snooping behaviour occurs naturally as the result of our domain
-	 * tracking.
-	 */
-	if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
+	if (!force && !is_cpu_flush_required(obj)) {
 		obj->cache_dirty = true;
 		return false;
 	}
-- 
2.3.0

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

[PATCH 6/6] drm/i915: obey wbinvd threshold in more places

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     |  4 ++++
 drivers/gpu/drm/i915/i915_gem.c     | 32 ++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_gem_gtt.c | 13 ++++++++++---
 3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5d2f62d..dfecdfd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2818,6 +2818,10 @@ static inline bool cpu_cache_is_coherent(struct drm_device *dev,
 {
 	return HAS_LLC(dev) || level != I915_CACHE_NONE;
 }
+static inline bool i915_gem_obj_should_clflush(struct drm_i915_gem_object *obj)
+{
+	return obj->base.size >= to_i915(obj->base.dev)->wbinvd_threshold;
+}
 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);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4d5a69d..59be709 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -204,6 +204,7 @@ i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 	char *vaddr = obj->phys_handle->vaddr;
 	struct sg_table *st;
 	struct scatterlist *sg;
+	const bool do_wbinvd = i915_gem_obj_should_clflush(obj);
 	int i;
 
 	if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
@@ -219,12 +220,15 @@ i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 
 		src = kmap_atomic(page);
 		memcpy(vaddr, src, PAGE_SIZE);
-		drm_clflush_virt_range(vaddr, PAGE_SIZE);
+		if (!do_wbinvd)
+			drm_clflush_virt_range(vaddr, PAGE_SIZE);
 		kunmap_atomic(src);
 
 		page_cache_release(page);
 		vaddr += PAGE_SIZE;
 	}
+	if (do_wbinvd)
+		wbinvd();
 
 	i915_gem_chipset_flush(obj->base.dev);
 
@@ -252,6 +256,7 @@ i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 static void
 i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj)
 {
+	const bool do_wbinvd = i915_gem_obj_should_clflush(obj);
 	int ret;
 
 	BUG_ON(obj->madv == __I915_MADV_PURGED);
@@ -282,7 +287,8 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj)
 				continue;
 
 			dst = kmap_atomic(page);
-			drm_clflush_virt_range(vaddr, PAGE_SIZE);
+			if (!do_wbinvd)
+				drm_clflush_virt_range(vaddr, PAGE_SIZE);
 			memcpy(dst, vaddr, PAGE_SIZE);
 			kunmap_atomic(dst);
 
@@ -295,6 +301,9 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj)
 		obj->dirty = 0;
 	}
 
+	if (do_wbinvd && !ret)
+		wbinvd();
+
 	sg_free_table(obj->pages);
 	kfree(obj->pages);
 
@@ -396,7 +405,10 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
 			return -EFAULT;
 	}
 
-	drm_clflush_virt_range(vaddr, args->size);
+	if (args->size >= to_i915(obj->base.dev)->wbinvd_threshold)
+		wbinvd();
+	else
+		drm_clflush_virt_range(vaddr, args->size);
 	i915_gem_chipset_flush(dev);
 	return 0;
 }
@@ -647,6 +659,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
 	int obj_do_bit17_swizzling, page_do_bit17_swizzling;
 	int prefaulted = 0;
 	int needs_clflush = 0;
+	bool do_wbinvd = false;
 	struct sg_page_iter sg_iter;
 
 	user_data = to_user_ptr(args->data_ptr);
@@ -658,6 +671,9 @@ i915_gem_shmem_pread(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	if (needs_clflush && i915_gem_obj_should_clflush(obj))
+		do_wbinvd = true;
+
 	offset = args->offset;
 
 	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents,
@@ -714,6 +730,9 @@ next_page:
 	}
 
 out:
+	if (do_wbinvd && !ret)
+		wbinvd();
+
 	i915_gem_object_unpin_pages(obj);
 
 	return ret;
@@ -4061,7 +4080,12 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
 
 	/* Flush the CPU cache if it's still invalid. */
 	if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
-		i915_gem_clflush_object(obj, false);
+		struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+		if (is_cpu_flush_required(obj) &&
+		    obj->base.size >= dev_priv->wbinvd_threshold)
+			wbinvd();
+		else
+			i915_gem_clflush_object(obj, false);
 
 		obj->base.read_domains |= I915_GEM_DOMAIN_CPU;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 746f77f..13cc493 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -371,6 +371,9 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
 	unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
 	unsigned pte = start >> GEN8_PTE_SHIFT & GEN8_PTE_MASK;
 	struct sg_page_iter sg_iter;
+	const bool needs_flush = !HAS_LLC(ppgtt->base.dev);
+	bool do_wbinvd = needs_flush &&
+		pages->nents * PAGE_SIZE >= to_i915(vm->dev)->wbinvd_threshold;
 
 	pt_vaddr = NULL;
 
@@ -385,7 +388,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
 			gen8_pte_encode(sg_page_iter_dma_address(&sg_iter),
 					cache_level, true);
 		if (++pte == GEN8_PTES_PER_PAGE) {
-			if (!HAS_LLC(ppgtt->base.dev))
+			if (needs_flush && !do_wbinvd)
 				drm_clflush_virt_range(pt_vaddr, PAGE_SIZE);
 			kunmap_atomic(pt_vaddr);
 			pt_vaddr = NULL;
@@ -401,6 +404,9 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
 			drm_clflush_virt_range(pt_vaddr, PAGE_SIZE);
 		kunmap_atomic(pt_vaddr);
 	}
+
+	if (do_wbinvd)
+		wbinvd();
 }
 
 static void gen8_free_page_tables(struct page **pt_pages)
@@ -660,11 +666,12 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 			pd_vaddr[j] = gen8_pde_encode(ppgtt->base.dev, addr,
 						      I915_CACHE_LLC);
 		}
-		if (!HAS_LLC(ppgtt->base.dev))
-			drm_clflush_virt_range(pd_vaddr, PAGE_SIZE);
 		kunmap_atomic(pd_vaddr);
 	}
 
+	if (!HAS_LLC(ppgtt->base.dev))
+		wbinvd();
+
 	ppgtt->switch_mm = gen8_mm_switch;
 	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
 	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
-- 
2.3.0

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

Re: [PATCH 1/6] drm/i915: Remove the useless flush_chipset

[Chris Wilson]

On Mon, Feb 09, 2015 at 01:54:14PM -0800, Ben Widawsky wrote:
> flush_chipset makes no sense with execlists because the former is for strictly
> prior to gen6, while the latter is for gen >= 8

The whole split is insane. I'd rather keep the code as is and merge it
back together which was the idea behind requests.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: Pass eb_vmas to execbuffer implementations

[Chris Wilson]

On Mon, Feb 09, 2015 at 01:54:15PM -0800, Ben Widawsky wrote:
> The role of eb_vmas continues to grow here as it becomes the proper
> encapsulation for the data passed to the various execution function.
> 
> Next patch makes use of it... This patch was initially part of the next patch,
> but got split out after I had found a bug that convinced me the two should be
> separate.

Again. No. There should only be one execbuffer implementation.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: Opportunistically reduce flushing at execbuf

[Chris Wilson]

On Mon, Feb 09, 2015 at 01:54:16PM -0800, Ben Widawsky wrote:
> 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.

Out of curiosity, have you compared with how this performs with an
improved userspace? There are several techniques which userspace can do
that are much higher performance than either clflush or wbinvd.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: obey wbinvd threshold in more places

[Chris Wilson]

On Mon, Feb 09, 2015 at 01:54:19PM -0800, Ben Widawsky wrote:
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  4 ++++
>  drivers/gpu/drm/i915/i915_gem.c     | 32 ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 13 ++++++++++---
>  3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5d2f62d..dfecdfd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2818,6 +2818,10 @@ static inline bool cpu_cache_is_coherent(struct drm_device *dev,
>  {
>  	return HAS_LLC(dev) || level != I915_CACHE_NONE;
>  }
> +static inline bool i915_gem_obj_should_clflush(struct drm_i915_gem_object *obj)
> +{
> +	return obj->base.size >= to_i915(obj->base.dev)->wbinvd_threshold;
> +}

if (i915_gem_obj_should_clflush(obj)) wbinvd()?

Does wbinvd always have the same characteristic threshold, even coupled
with a second access (read or write) inside the TLB flushing of
kunmap_atomic. I would imagine that these workloads are dramatically
different to the replacement in execbuffer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: obey wbinvd threshold in more places

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5739
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  282/283              282/283
ILK                                  271/278              271/278
SNB              +2-22              340/346              320/346
IVB              +1-2              378/384              377/384
BYT                                  296/296              296/296
HSW              +4                 421/428              425/428
BDW                                  318/333              318/333
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_drv_suspend_debugfs-reader      DMESG_WARN(1, M37)      NO_RESULT(1, M37)
*ILK  igt_drv_suspend_fence-restore-tiled2untiled      DMESG_WARN(1, M37)      NO_RESULT(1, M37)
*ILK  igt_drv_suspend_fence-restore-untiled      DMESG_WARN(1, M37)      NO_RESULT(1, M37)
*ILK  igt_drv_suspend_forcewake      DMESG_WARN(1, M37)      NO_RESULT(1, M37)
*ILK  igt_gem_workarounds_suspend-resume      DMESG_WARN(1, M37)      INIT(1, M37)
 SNB  igt_kms_cursor_crc_cursor-size-change      NSPT(1, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_kms_flip_event_leak      NSPT(1, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_kms_flip_modeset-vs-vblank-race      DMESG_WARN(1, M22)PASS(1, M22)      PASS(1, M22)
 SNB  igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_kms_pipe_crc_basic_read-crc-pipe-A      DMESG_WARN(1, M22)PASS(6, M22)      PASS(1, M22)
 SNB  igt_kms_rotation_crc_primary-rotation      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_kms_rotation_crc_sprite-rotation      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_cursor      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_cursor-dpms      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_dpms-mode-unset-non-lpsp      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_dpms-non-lpsp      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_drm-resources-equal      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_fences      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_fences-dpms      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_gem-execbuf      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_gem-mmap-cpu      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_gem-mmap-gtt      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_gem-pread      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_i2c      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_modeset-non-lpsp      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_modeset-non-lpsp-stress-no-wait      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_pci-d3-state      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 SNB  igt_pm_rpm_rte      NSPT(2, M22)PASS(1, M22)      NSPT(1, M22)
 IVB  igt_gem_pwrite_pread_snooped-copy-performance      DMESG_WARN(1, M34)PASS(5, M34)      DMESG_WARN(1, M34)
 IVB  igt_gem_storedw_batches_loop_normal      DMESG_WARN(2, M34)PASS(2, M34)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_secure-dispatch      DMESG_WARN(1, M34)PASS(3, M34)      DMESG_WARN(1, M34)
 HSW  igt_gem_storedw_loop_blt      DMESG_WARN(3, M20)PASS(3, M20)      PASS(1, M20)
 HSW  igt_gem_storedw_loop_vebox      DMESG_WARN(3, M20)PASS(2, M20)      PASS(1, M20)
*HSW  igt_kms_flip_bo-too-big      BLACKLIST(1, M20)      PASS(1, M20)
*HSW  igt_kms_flip_bo-too-big-interruptible      BLACKLIST(1, M20)      PASS(1, M20)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx