[PATCH 1/7] drm/i915: Track file_priv, not ctx in the ppgtt structure

[PATCH 1/7] drm/i915: Track file_priv, not ctx in the ppgtt structure

[Daniel Vetter]

Hardware contexts reference a ppgtt, not the other way round. And the
only user of this (in debugfs) actually only cares about which file
the ppgtt is associated with. So give it what it wants.

While at it give the ppgtt create function a proper name&place.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 22 +---------------------
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h     |  6 +++++-
 4 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9e737b771c40..3bf1d20c598b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -333,7 +333,7 @@ static int per_file_stats(int id, void *ptr, void *data)
 			}
 
 			ppgtt = container_of(vma->vm, struct i915_hw_ppgtt, base);
-			if (ppgtt->ctx && ppgtt->ctx->file_priv != stats->file_priv)
+			if (ppgtt->file_priv != stats->file_priv)
 				continue;
 
 			if (obj->ring) /* XXX per-vma statistic */
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 899c6a7a5920..3b8367aa8404 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -181,26 +181,6 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
 	return obj;
 }
 
-static struct i915_hw_ppgtt *
-create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx)
-{
-	struct i915_hw_ppgtt *ppgtt;
-	int ret;
-
-	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
-	if (!ppgtt)
-		return ERR_PTR(-ENOMEM);
-
-	ret = i915_ppgtt_init(dev, ppgtt);
-	if (ret) {
-		kfree(ppgtt);
-		return ERR_PTR(ret);
-	}
-
-	ppgtt->ctx = ctx;
-	return ppgtt;
-}
-
 static struct intel_context *
 __create_hw_context(struct drm_device *dev,
 		    struct drm_i915_file_private *file_priv)
@@ -287,7 +267,7 @@ i915_gem_create_context(struct drm_device *dev,
 	}
 
 	if (create_vm) {
-		struct i915_hw_ppgtt *ppgtt = create_vm_for_ctx(dev, ctx);
+		struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv);
 
 		if (IS_ERR_OR_NULL(ppgtt)) {
 			DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index baa94199239b..83ee41e5c1c7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1222,6 +1222,27 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 	return ret;
 }
 
+struct i915_hw_ppgtt *
+i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv)
+{
+	struct i915_hw_ppgtt *ppgtt;
+	int ret;
+
+	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
+	if (!ppgtt)
+		return ERR_PTR(-ENOMEM);
+
+	ret = i915_ppgtt_init(dev, ppgtt);
+	if (ret) {
+		kfree(ppgtt);
+		return ERR_PTR(ret);
+	}
+
+	ppgtt->file_priv = fpriv;
+
+	return ppgtt;
+}
+
 void  i915_ppgtt_release(struct kref *kref)
 {
 	struct i915_hw_ppgtt *ppgtt =
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 380e034c66f8..0b04ef6167f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -34,6 +34,8 @@
 #ifndef __I915_GEM_GTT_H__
 #define __I915_GEM_GTT_H__
 
+struct drm_i915_file_private;
+
 typedef uint32_t gen6_gtt_pte_t;
 typedef uint64_t gen8_gtt_pte_t;
 typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
@@ -258,7 +260,7 @@ struct i915_hw_ppgtt {
 		dma_addr_t *gen8_pt_dma_addr[4];
 	};
 
-	struct intel_context *ctx;
+	struct drm_i915_file_private *file_priv;
 
 	int (*enable)(struct i915_hw_ppgtt *ppgtt);
 	int (*switch_mm)(struct i915_hw_ppgtt *ppgtt,
@@ -276,6 +278,8 @@ bool intel_enable_ppgtt(struct drm_device *dev, bool full);
 
 int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
 void i915_ppgtt_release(struct kref *kref);
+struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev,
+					struct drm_i915_file_private *fpriv);
 static inline void i915_ppgtt_get(struct i915_hw_ppgtt *ppgtt)
 {
 	if (ppgtt)
-- 
1.9.3

[PATCH 2/7] drm/i915: Only refcount ppgtt if it actually is one

[Daniel Vetter]

This essentially unbreaks non-ppgtt operation where we'd scribble over
random memory.

While at it give the vm_to_ppgtt function a proper prefix and make it
a bit more paranoid.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h     | 10 +++++++++-
 drivers/gpu/drm/i915/i915_gem.c     |  3 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c |  3 ++-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c783cfe5515d..fd3a99c5a1bd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2460,6 +2460,15 @@ static inline bool i915_is_ggtt(struct i915_address_space *vm)
 	return vm == ggtt;
 }
 
+static inline struct i915_hw_ppgtt *
+i915_vm_to_ppgtt(struct i915_address_space *vm)
+{
+	WARN_ON(i915_is_ggtt(vm));
+
+	return container_of(vm, struct i915_hw_ppgtt, base);
+}
+
+
 static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
 {
 	return i915_gem_obj_bound(obj, obj_to_ggtt(obj));
@@ -2495,7 +2504,6 @@ void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
 
 /* i915_gem_context.c */
 #define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
-#define vm_to_ppgtt(vm) container_of(vm, struct i915_hw_ppgtt, base)
 int __must_check i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_reset(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b33a677b4b1e..c43ccfdf45a5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4508,7 +4508,8 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 
 	vm = vma->vm;
 
-	i915_ppgtt_put(vm_to_ppgtt(vm));
+	if (!i915_is_ggtt(vm))
+		i915_ppgtt_put(i915_vm_to_ppgtt(vm));
 
 	list_del(&vma->vma_link);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 83ee41e5c1c7..3753bf184865 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2204,7 +2204,8 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
 	if (!vma)
 		vma = __i915_gem_vma_create(obj, vm);
 
-	i915_ppgtt_get(vm_to_ppgtt(vm));
+	if (!i915_is_ggtt(vm))
+		i915_ppgtt_get(i915_vm_to_ppgtt(vm));
 
 	return vma;
 }
-- 
1.9.3

[PATCH 3/7] drm/i915: Add proper prefix to obj_to_ggtt

[Daniel Vetter]

Stuff in headers really aught to have this.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 11 ++++++-----
 drivers/gpu/drm/i915/i915_gem.c |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fd3a99c5a1bd..0762e19f9bc6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2451,7 +2451,7 @@ static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
 }
 
 /* Some GGTT VM helpers */
-#define obj_to_ggtt(obj) \
+#define i915_obj_to_ggtt(obj) \
 	(&((struct drm_i915_private *)(obj)->base.dev->dev_private)->gtt.base)
 static inline bool i915_is_ggtt(struct i915_address_space *vm)
 {
@@ -2471,19 +2471,19 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
 
 static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
 {
-	return i915_gem_obj_bound(obj, obj_to_ggtt(obj));
+	return i915_gem_obj_bound(obj, i915_obj_to_ggtt(obj));
 }
 
 static inline unsigned long
 i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
 {
-	return i915_gem_obj_offset(obj, obj_to_ggtt(obj));
+	return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
 }
 
 static inline unsigned long
 i915_gem_obj_ggtt_size(struct drm_i915_gem_object *obj)
 {
-	return i915_gem_obj_size(obj, obj_to_ggtt(obj));
+	return i915_gem_obj_size(obj, i915_obj_to_ggtt(obj));
 }
 
 static inline int __must_check
@@ -2491,7 +2491,8 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
 		      uint32_t alignment,
 		      unsigned flags)
 {
-	return i915_gem_object_pin(obj, obj_to_ggtt(obj), alignment, flags | PIN_GLOBAL);
+	return i915_gem_object_pin(obj, i915_obj_to_ggtt(obj),
+				   alignment, flags | PIN_GLOBAL);
 }
 
 static inline int
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c43ccfdf45a5..f4e57fe05c6a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5266,7 +5266,7 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 		return NULL;
 
 	vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link);
-	if (vma->vm != obj_to_ggtt(obj))
+	if (vma->vm != i915_obj_to_ggtt(obj))
 		return NULL;
 
 	return vma;
-- 
1.9.3

[PATCH 4/7] drm/i915: Allow i915_gem_setup_global_gtt to fail

[Daniel Vetter]

We already needs this just as a safety check in case the preallocation
reservation dance fails. But we definitely need this to be able to
move tha aliasing ppgtt setup back out of the context code to this
place, where it belongs.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c     |  7 ++++++-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 16 ++++++++++------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  4 ++--
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f4e57fe05c6a..d8399ee622b9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4762,7 +4762,12 @@ int i915_gem_init(struct drm_device *dev)
 			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
 	}
 
-	i915_gem_init_userptr(dev);
+	ret = i915_gem_init_userptr(dev);
+	if (ret) {
+		mutex_unlock(&dev->struct_mutex);
+		return ret;
+	}
+
 	i915_gem_init_global_gtt(dev);
 
 	ret = i915_gem_context_init(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 3753bf184865..4fa7807ed4d5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1709,10 +1709,10 @@ static void i915_gtt_color_adjust(struct drm_mm_node *node,
 	}
 }
 
-void i915_gem_setup_global_gtt(struct drm_device *dev,
-			       unsigned long start,
-			       unsigned long mappable_end,
-			       unsigned long end)
+int i915_gem_setup_global_gtt(struct drm_device *dev,
+			      unsigned long start,
+			      unsigned long mappable_end,
+			      unsigned long end)
 {
 	/* Let GEM Manage all of the aperture.
 	 *
@@ -1745,8 +1745,10 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
 
 		WARN_ON(i915_gem_obj_ggtt_bound(obj));
 		ret = drm_mm_reserve_node(&ggtt_vm->mm, &vma->node);
-		if (ret)
-			DRM_DEBUG_KMS("Reservation failed\n");
+		if (ret) {
+			DRM_DEBUG_KMS("Reservation failed: %i\n", ret);
+			return ret;
+		}
 		obj->has_global_gtt_mapping = 1;
 	}
 
@@ -1763,6 +1765,8 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
 
 	/* And finally clear the reserved guard page */
 	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
+
+	return 0;
 }
 
 void i915_gem_init_global_gtt(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 0b04ef6167f8..bea3541d5525 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -271,8 +271,8 @@ struct i915_hw_ppgtt {
 
 int i915_gem_gtt_init(struct drm_device *dev);
 void i915_gem_init_global_gtt(struct drm_device *dev);
-void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start,
-			       unsigned long mappable_end, unsigned long end);
+int i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start,
+			      unsigned long mappable_end, unsigned long end);
 
 bool intel_enable_ppgtt(struct drm_device *dev, bool full);
 
-- 
1.9.3

[PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter]

Stuffing this into the context setup code doesn't make a lot of sense.
Also reusing the real ppgtt setup code makes even less sense since the
aliasing ppgtt isn't a real address space. Leaving all that stuff
unitialized will make sure that we catch any abusers promptly.

This is also a prep work to clean up the context->ppgtt link.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 31 +++++++++++++++++++++++++------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3b8367aa8404..7a455fcee3a7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
 			goto err_unpin;
 		} else
 			ctx->vm = &ppgtt->base;
-
-		/* This case is reserved for the global default context and
-		 * should only happen once. */
-		if (is_global_default_ctx) {
-			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
-				ret = -EEXIST;
-				goto err_unpin;
-			}
-
-			dev_priv->mm.aliasing_ppgtt = ppgtt;
-		}
 	} else if (USES_PPGTT(dev)) {
 		/* For platforms which only have aliasing PPGTT, we fake the
 		 * address space and refcounting. */
@@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
 		}
 	}
 
-	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
+	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context (error %ld)\n",
 			  PTR_ERR(ctx));
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4fa7807ed4d5..cb9bb13ff20a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1191,23 +1191,27 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	return 0;
 }
 
-int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret = 0;
 
 	ppgtt->base.dev = dev;
 	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
 
 	if (INTEL_INFO(dev)->gen < 8)
-		ret = gen6_ppgtt_init(ppgtt);
+		return gen6_ppgtt_init(ppgtt);
 	else if (IS_GEN8(dev))
-		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
+		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
 	else
 		BUG();
+}
+int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
 
+	ret = __hw_ppgtt_init(dev, ppgtt);
 	if (!ret) {
-		struct drm_i915_private *dev_priv = dev->dev_private;
 		kref_init(&ppgtt->ref);
 		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
 			    ppgtt->base.total);
@@ -1728,6 +1732,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 	struct drm_mm_node *entry;
 	struct drm_i915_gem_object *obj;
 	unsigned long hole_start, hole_end;
+	int ret;
 
 	BUG_ON(mappable_end > end);
 
@@ -1739,7 +1744,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 	/* Mark any preallocated objects as occupied */
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
-		int ret;
+
 		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
 			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
 
@@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 	/* And finally clear the reserved guard page */
 	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
 
+	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
+		struct i915_hw_ppgtt *ppgtt;
+
+		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
+		if (!ppgtt)
+			return -ENOMEM;
+
+		ret = __hw_ppgtt_init(dev, ppgtt);
+		if (!ret)
+			return ret;
+
+		dev_priv->mm.aliasing_ppgtt = ppgtt;
+	}
+
 	return 0;
 }
 
-- 
1.9.3

[PATCH 6/7] drm/i915: Only track real ppgtt for a context

[Daniel Vetter]

There's a bit a confusion since we track the global gtt,
the aliasing and real ppgtt in the ctx->vm pointer. And not
all callers really bother to check for the different cases and just
presume that it points to a real ppgtt.

Now looking closely we don't actually need ->vm to always point at an
address space - the only place that cares actually has fixup code
already to decide whether to look at the per-proces or the global
address space.

So switch to just tracking the ppgtt directly and ditch all the
extraneous code.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
 drivers/gpu/drm/i915/i915_drv.h            |  3 +--
 drivers/gpu/drm/i915/i915_gem_context.c    | 28 +++++++---------------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +++--
 drivers/gpu/drm/i915/i915_gpu_error.c      | 10 +++++++---
 5 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3bf1d20c598b..aaf07e230cb0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1774,7 +1774,7 @@ static int per_file_ctx(int id, void *ptr, void *data)
 {
 	struct intel_context *ctx = ptr;
 	struct seq_file *m = data;
-	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(ctx);
+	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
 
 	if (i915_gem_context_is_default(ctx))
 		seq_puts(m, "  default context:\n");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0762e19f9bc6..3230b08aff13 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -616,7 +616,7 @@ struct intel_context {
 	uint8_t remap_slice;
 	struct drm_i915_file_private *file_priv;
 	struct i915_ctx_hang_stats hang_stats;
-	struct i915_address_space *vm;
+	struct i915_hw_ppgtt *ppgtt;
 
 	struct {
 		struct drm_i915_gem_object *rcs_state;
@@ -2504,7 +2504,6 @@ i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
 void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
 
 /* i915_gem_context.c */
-#define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
 int __must_check i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_reset(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 7a455fcee3a7..c00e5d027774 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -136,15 +136,8 @@ void i915_gem_context_free(struct kref *ctx_ref)
 {
 	struct intel_context *ctx = container_of(ctx_ref,
 						   typeof(*ctx), ref);
-	struct i915_hw_ppgtt *ppgtt = NULL;
 
-	if (ctx->legacy_hw_ctx.rcs_state) {
-		/* We refcount even the aliasing PPGTT to keep the code symmetric */
-		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
-			ppgtt = ctx_to_ppgtt(ctx);
-	}
-
-	i915_ppgtt_put(ppgtt);
+	i915_ppgtt_put(ctx->ppgtt);
 	if (ctx->legacy_hw_ctx.rcs_state)
 		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
 	list_del(&ctx->link);
@@ -240,7 +233,6 @@ i915_gem_create_context(struct drm_device *dev,
 			bool create_vm)
 {
 	const bool is_global_default_ctx = file_priv == NULL;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_context *ctx;
 	int ret = 0;
 
@@ -274,15 +266,10 @@ i915_gem_create_context(struct drm_device *dev,
 					 PTR_ERR(ppgtt));
 			ret = PTR_ERR(ppgtt);
 			goto err_unpin;
-		} else
-			ctx->vm = &ppgtt->base;
-	} else if (USES_PPGTT(dev)) {
-		/* For platforms which only have aliasing PPGTT, we fake the
-		 * address space and refcounting. */
-		ctx->vm = &dev_priv->mm.aliasing_ppgtt->base;
-		i915_ppgtt_get(dev_priv->mm.aliasing_ppgtt);
-	} else
-		ctx->vm = &dev_priv->gtt.base;
+		}
+
+		ctx->ppgtt = ppgtt;
+	}
 
 	return ctx;
 
@@ -538,7 +525,6 @@ static int do_switch(struct intel_engine_cs *ring,
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct intel_context *from = ring->last_context;
-	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
 	int ret, i;
@@ -566,8 +552,8 @@ static int do_switch(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
-		ret = ppgtt->switch_mm(ppgtt, ring, false);
+	if (to->ppgtt) {
+		ret = to->ppgtt->switch_mm(to->ppgtt, ring, false);
 		if (ret)
 			goto unpin_out;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 60998fc4e5b2..54b3d8c8b228 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1318,8 +1318,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	i915_gem_context_reference(ctx);
 
-	vm = ctx->vm;
-	if (!USES_FULL_PPGTT(dev))
+	if (ctx->ppgtt)
+		vm = &ctx->ppgtt->base;
+	else
 		vm = &dev_priv->gtt.base;
 
 	eb = eb_create(args);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 0b3f69439451..06072120b15d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -958,6 +958,12 @@ static void i915_gem_record_rings(struct drm_device *dev,
 
 		request = i915_gem_find_active_request(ring);
 		if (request) {
+			struct i915_address_space *vm;
+
+			vm = request->ctx && request->ctx->ppgtt ?
+				&request->ctx->ppgtt->base :
+				&dev_priv->gtt.base;
+
 			/* We need to copy these to an anonymous buffer
 			 * as the simplest method to avoid being overwritten
 			 * by userspace.
@@ -965,9 +971,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 			error->ring[i].batchbuffer =
 				i915_error_object_create(dev_priv,
 							 request->batch_obj,
-							 request->ctx ?
-							 request->ctx->vm :
-							 &dev_priv->gtt.base);
+							 vm);
 
 			if (HAS_BROKEN_CS_TLB(dev_priv->dev) &&
 			    ring->scratch.obj)
-- 
1.9.3

[PATCH 7/7] drm/i915: Drop create_vm argument to i915_gem_create_context

[Daniel Vetter]

Now that all the flow is streamlined the rule is simple: We create
a new ppgtt for a new context when we have full ppgtt enabled.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index c00e5d027774..655ed6228aab 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -229,8 +229,7 @@ err_out:
  */
 static struct intel_context *
 i915_gem_create_context(struct drm_device *dev,
-			struct drm_i915_file_private *file_priv,
-			bool create_vm)
+			struct drm_i915_file_private *file_priv)
 {
 	const bool is_global_default_ctx = file_priv == NULL;
 	struct intel_context *ctx;
@@ -258,7 +257,7 @@ i915_gem_create_context(struct drm_device *dev,
 		}
 	}
 
-	if (create_vm) {
+	if (USES_FULL_PPGTT(dev)) {
 		struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv);
 
 		if (IS_ERR_OR_NULL(ppgtt)) {
@@ -337,7 +336,7 @@ int i915_gem_context_init(struct drm_device *dev)
 		}
 	}
 
-	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
+	ctx = i915_gem_create_context(dev, NULL);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context (error %ld)\n",
 			  PTR_ERR(ctx));
@@ -438,7 +437,7 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 	idr_init(&file_priv->context_idr);
 
 	mutex_lock(&dev->struct_mutex);
-	ctx = i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev));
+	ctx = i915_gem_create_context(dev, file_priv);
 	mutex_unlock(&dev->struct_mutex);
 
 	if (IS_ERR(ctx)) {
@@ -696,7 +695,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	ctx = i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev));
+	ctx = i915_gem_create_context(dev, file_priv);
 	mutex_unlock(&dev->struct_mutex);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
-- 
1.9.3

Re: [PATCH 6/7] drm/i915: Only track real ppgtt for a context

[Daniel Vetter]

On Wed, Jul 30, 2014 at 09:42:03PM +0200, Daniel Vetter wrote:
> There's a bit a confusion since we track the global gtt,
> the aliasing and real ppgtt in the ctx->vm pointer. And not
> all callers really bother to check for the different cases and just
> presume that it points to a real ppgtt.
> 
> Now looking closely we don't actually need ->vm to always point at an
> address space - the only place that cares actually has fixup code
> already to decide whether to look at the per-proces or the global
> address space.
> 
> So switch to just tracking the ppgtt directly and ditch all the
> extraneous code.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jesse looked up the Jira for this one for me, so

OTC-Jira: VIZ-3724

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h            |  3 +--
>  drivers/gpu/drm/i915/i915_gem_context.c    | 28 +++++++---------------------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +++--
>  drivers/gpu/drm/i915/i915_gpu_error.c      | 10 +++++++---
>  5 files changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3bf1d20c598b..aaf07e230cb0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1774,7 +1774,7 @@ static int per_file_ctx(int id, void *ptr, void *data)
>  {
>  	struct intel_context *ctx = ptr;
>  	struct seq_file *m = data;
> -	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(ctx);
> +	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
>  
>  	if (i915_gem_context_is_default(ctx))
>  		seq_puts(m, "  default context:\n");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0762e19f9bc6..3230b08aff13 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -616,7 +616,7 @@ struct intel_context {
>  	uint8_t remap_slice;
>  	struct drm_i915_file_private *file_priv;
>  	struct i915_ctx_hang_stats hang_stats;
> -	struct i915_address_space *vm;
> +	struct i915_hw_ppgtt *ppgtt;
>  
>  	struct {
>  		struct drm_i915_gem_object *rcs_state;
> @@ -2504,7 +2504,6 @@ i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
>  void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
>  
>  /* i915_gem_context.c */
> -#define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
>  int __must_check i915_gem_context_init(struct drm_device *dev);
>  void i915_gem_context_fini(struct drm_device *dev);
>  void i915_gem_context_reset(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 7a455fcee3a7..c00e5d027774 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -136,15 +136,8 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  {
>  	struct intel_context *ctx = container_of(ctx_ref,
>  						   typeof(*ctx), ref);
> -	struct i915_hw_ppgtt *ppgtt = NULL;
>  
> -	if (ctx->legacy_hw_ctx.rcs_state) {
> -		/* We refcount even the aliasing PPGTT to keep the code symmetric */
> -		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
> -			ppgtt = ctx_to_ppgtt(ctx);
> -	}
> -
> -	i915_ppgtt_put(ppgtt);
> +	i915_ppgtt_put(ctx->ppgtt);
>  	if (ctx->legacy_hw_ctx.rcs_state)
>  		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
>  	list_del(&ctx->link);
> @@ -240,7 +233,6 @@ i915_gem_create_context(struct drm_device *dev,
>  			bool create_vm)
>  {
>  	const bool is_global_default_ctx = file_priv == NULL;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_context *ctx;
>  	int ret = 0;
>  
> @@ -274,15 +266,10 @@ i915_gem_create_context(struct drm_device *dev,
>  					 PTR_ERR(ppgtt));
>  			ret = PTR_ERR(ppgtt);
>  			goto err_unpin;
> -		} else
> -			ctx->vm = &ppgtt->base;
> -	} else if (USES_PPGTT(dev)) {
> -		/* For platforms which only have aliasing PPGTT, we fake the
> -		 * address space and refcounting. */
> -		ctx->vm = &dev_priv->mm.aliasing_ppgtt->base;
> -		i915_ppgtt_get(dev_priv->mm.aliasing_ppgtt);
> -	} else
> -		ctx->vm = &dev_priv->gtt.base;
> +		}
> +
> +		ctx->ppgtt = ppgtt;
> +	}
>  
>  	return ctx;
>  
> @@ -538,7 +525,6 @@ static int do_switch(struct intel_engine_cs *ring,
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	struct intel_context *from = ring->last_context;
> -	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
>  	u32 hw_flags = 0;
>  	bool uninitialized = false;
>  	int ret, i;
> @@ -566,8 +552,8 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 */
>  	from = ring->last_context;
>  
> -	if (USES_FULL_PPGTT(ring->dev)) {
> -		ret = ppgtt->switch_mm(ppgtt, ring, false);
> +	if (to->ppgtt) {
> +		ret = to->ppgtt->switch_mm(to->ppgtt, ring, false);
>  		if (ret)
>  			goto unpin_out;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 60998fc4e5b2..54b3d8c8b228 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1318,8 +1318,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  
>  	i915_gem_context_reference(ctx);
>  
> -	vm = ctx->vm;
> -	if (!USES_FULL_PPGTT(dev))
> +	if (ctx->ppgtt)
> +		vm = &ctx->ppgtt->base;
> +	else
>  		vm = &dev_priv->gtt.base;
>  
>  	eb = eb_create(args);
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 0b3f69439451..06072120b15d 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -958,6 +958,12 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  
>  		request = i915_gem_find_active_request(ring);
>  		if (request) {
> +			struct i915_address_space *vm;
> +
> +			vm = request->ctx && request->ctx->ppgtt ?
> +				&request->ctx->ppgtt->base :
> +				&dev_priv->gtt.base;
> +
>  			/* We need to copy these to an anonymous buffer
>  			 * as the simplest method to avoid being overwritten
>  			 * by userspace.
> @@ -965,9 +971,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  			error->ring[i].batchbuffer =
>  				i915_error_object_create(dev_priv,
>  							 request->batch_obj,
> -							 request->ctx ?
> -							 request->ctx->vm :
> -							 &dev_priv->gtt.base);
> +							 vm);
>  
>  			if (HAS_BROKEN_CS_TLB(dev_priv->dev) &&
>  			    ring->scratch.obj)
> -- 
> 1.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Ben Widawsky]

<1386367941-7131-76-git-send-email-benjamin.widawsky@intel.com>

On Wed, Jul 30, 2014 at 09:42:02PM +0200, Daniel Vetter wrote:
> Stuffing this into the context setup code doesn't make a lot of sense.
> Also reusing the real ppgtt setup code makes even less sense since the
> aliasing ppgtt isn't a real address space. Leaving all that stuff
> unitialized will make sure that we catch any abusers promptly.
> 
> This is also a prep work to clean up the context->ppgtt link.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 31 +++++++++++++++++++++++++------
>  2 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3b8367aa8404..7a455fcee3a7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
>  			goto err_unpin;
>  		} else
>  			ctx->vm = &ppgtt->base;
> -
> -		/* This case is reserved for the global default context and
> -		 * should only happen once. */
> -		if (is_global_default_ctx) {
> -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> -				ret = -EEXIST;
> -				goto err_unpin;
> -			}
> -
> -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> -		}
>  	} else if (USES_PPGTT(dev)) {
>  		/* For platforms which only have aliasing PPGTT, we fake the
>  		 * address space and refcounting. */
> @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
>  		}
>  	}
>  
> -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> +	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
>  	if (IS_ERR(ctx)) {
>  		DRM_ERROR("Failed to create default global context (error %ld)\n",
>  			  PTR_ERR(ctx));
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4fa7807ed4d5..cb9bb13ff20a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1191,23 +1191,27 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	return 0;
>  }
>  
> -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> +int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret = 0;
>  
>  	ppgtt->base.dev = dev;
>  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
>  
>  	if (INTEL_INFO(dev)->gen < 8)
> -		ret = gen6_ppgtt_init(ppgtt);
> +		return gen6_ppgtt_init(ppgtt);
>  	else if (IS_GEN8(dev))
> -		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> +		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
>  	else
>  		BUG();
> +}
> +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;
>  
> +	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (!ret) {
> -		struct drm_i915_private *dev_priv = dev->dev_private;
>  		kref_init(&ppgtt->ref);
>  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
>  			    ppgtt->base.total);
> @@ -1728,6 +1732,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	struct drm_mm_node *entry;
>  	struct drm_i915_gem_object *obj;
>  	unsigned long hole_start, hole_end;
> +	int ret;
>  
>  	BUG_ON(mappable_end > end);
>  
> @@ -1739,7 +1744,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	/* Mark any preallocated objects as occupied */
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>  		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
> -		int ret;
> +
>  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
>  			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
>  
> @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	/* And finally clear the reserved guard page */
>  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
>  
> +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
> +		struct i915_hw_ppgtt *ppgtt;
> +
> +		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> +		if (!ppgtt)
> +			return -ENOMEM;
> +
> +		ret = __hw_ppgtt_init(dev, ppgtt);
> +		if (!ret)
> +			return ret;
> +
> +		dev_priv->mm.aliasing_ppgtt = ppgtt;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Ben Widawsky]

On Wed, Jul 30, 2014 at 08:46:11PM -0700, Ben Widawsky wrote:
> <1386367941-7131-76-git-send-email-benjamin.widawsky@intel.com>

and

<1386367941-7131-81-git-send-email-benjamin.widawsky@intel.com>

> 
> On Wed, Jul 30, 2014 at 09:42:02PM +0200, Daniel Vetter wrote:
> > Stuffing this into the context setup code doesn't make a lot of sense.
> > Also reusing the real ppgtt setup code makes even less sense since the
> > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > unitialized will make sure that we catch any abusers promptly.
> > 
> > This is also a prep work to clean up the context->ppgtt link.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
> >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 31 +++++++++++++++++++++++++------
> >  2 files changed, 26 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 3b8367aa8404..7a455fcee3a7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
> >  			goto err_unpin;
> >  		} else
> >  			ctx->vm = &ppgtt->base;
> > -
> > -		/* This case is reserved for the global default context and
> > -		 * should only happen once. */
> > -		if (is_global_default_ctx) {
> > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > -				ret = -EEXIST;
> > -				goto err_unpin;
> > -			}
> > -
> > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > -		}
> >  	} else if (USES_PPGTT(dev)) {
> >  		/* For platforms which only have aliasing PPGTT, we fake the
> >  		 * address space and refcounting. */
> > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
> >  		}
> >  	}
> >  
> > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > +	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
> >  	if (IS_ERR(ctx)) {
> >  		DRM_ERROR("Failed to create default global context (error %ld)\n",
> >  			  PTR_ERR(ctx));
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 4fa7807ed4d5..cb9bb13ff20a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1191,23 +1191,27 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> >  	return 0;
> >  }
> >  
> > -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > +int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int ret = 0;
> >  
> >  	ppgtt->base.dev = dev;
> >  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
> >  
> >  	if (INTEL_INFO(dev)->gen < 8)
> > -		ret = gen6_ppgtt_init(ppgtt);
> > +		return gen6_ppgtt_init(ppgtt);
> >  	else if (IS_GEN8(dev))
> > -		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> > +		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> >  	else
> >  		BUG();
> > +}
> > +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret = 0;
> >  
> > +	ret = __hw_ppgtt_init(dev, ppgtt);
> >  	if (!ret) {
> > -		struct drm_i915_private *dev_priv = dev->dev_private;
> >  		kref_init(&ppgtt->ref);
> >  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> >  			    ppgtt->base.total);
> > @@ -1728,6 +1732,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> >  	struct drm_mm_node *entry;
> >  	struct drm_i915_gem_object *obj;
> >  	unsigned long hole_start, hole_end;
> > +	int ret;
> >  
> >  	BUG_ON(mappable_end > end);
> >  
> > @@ -1739,7 +1744,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> >  	/* Mark any preallocated objects as occupied */
> >  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> >  		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
> > -		int ret;
> > +
> >  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
> >  			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
> >  
> > @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> >  	/* And finally clear the reserved guard page */
> >  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
> >  
> > +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
> > +		struct i915_hw_ppgtt *ppgtt;
> > +
> > +		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> > +		if (!ppgtt)
> > +			return -ENOMEM;
> > +
> > +		ret = __hw_ppgtt_init(dev, ppgtt);
> > +		if (!ret)
> > +			return ret;
> > +
> > +		dev_priv->mm.aliasing_ppgtt = ppgtt;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Ben Widawsky]

On Wed, Jul 30, 2014 at 08:47:03PM -0700, Ben Widawsky wrote:
> On Wed, Jul 30, 2014 at 08:46:11PM -0700, Ben Widawsky wrote:
> > <1386367941-7131-76-git-send-email-benjamin.widawsky@intel.com>
> 
> and
> 
> <1386367941-7131-81-git-send-email-benjamin.widawsky@intel.com>

Oops, was looking at the wrong this actually. Ignore please.

> 
> > 
> > On Wed, Jul 30, 2014 at 09:42:02PM +0200, Daniel Vetter wrote:
> > > Stuffing this into the context setup code doesn't make a lot of sense.
> > > Also reusing the real ppgtt setup code makes even less sense since the
> > > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > > unitialized will make sure that we catch any abusers promptly.
> > > 
> > > This is also a prep work to clean up the context->ppgtt link.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 31 +++++++++++++++++++++++++------
> > >  2 files changed, 26 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index 3b8367aa8404..7a455fcee3a7 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
> > >  			goto err_unpin;
> > >  		} else
> > >  			ctx->vm = &ppgtt->base;
> > > -
> > > -		/* This case is reserved for the global default context and
> > > -		 * should only happen once. */
> > > -		if (is_global_default_ctx) {
> > > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > > -				ret = -EEXIST;
> > > -				goto err_unpin;
> > > -			}
> > > -
> > > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > > -		}
> > >  	} else if (USES_PPGTT(dev)) {
> > >  		/* For platforms which only have aliasing PPGTT, we fake the
> > >  		 * address space and refcounting. */
> > > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
> > >  		}
> > >  	}
> > >  
> > > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > > +	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
> > >  	if (IS_ERR(ctx)) {
> > >  		DRM_ERROR("Failed to create default global context (error %ld)\n",
> > >  			  PTR_ERR(ctx));
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 4fa7807ed4d5..cb9bb13ff20a 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -1191,23 +1191,27 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> > >  	return 0;
> > >  }
> > >  
> > > -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > > +int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > -	int ret = 0;
> > >  
> > >  	ppgtt->base.dev = dev;
> > >  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
> > >  
> > >  	if (INTEL_INFO(dev)->gen < 8)
> > > -		ret = gen6_ppgtt_init(ppgtt);
> > > +		return gen6_ppgtt_init(ppgtt);
> > >  	else if (IS_GEN8(dev))
> > > -		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> > > +		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> > >  	else
> > >  		BUG();
> > > +}
> > > +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	int ret = 0;
> > >  
> > > +	ret = __hw_ppgtt_init(dev, ppgtt);
> > >  	if (!ret) {
> > > -		struct drm_i915_private *dev_priv = dev->dev_private;
> > >  		kref_init(&ppgtt->ref);
> > >  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> > >  			    ppgtt->base.total);
> > > @@ -1728,6 +1732,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> > >  	struct drm_mm_node *entry;
> > >  	struct drm_i915_gem_object *obj;
> > >  	unsigned long hole_start, hole_end;
> > > +	int ret;
> > >  
> > >  	BUG_ON(mappable_end > end);
> > >  
> > > @@ -1739,7 +1744,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> > >  	/* Mark any preallocated objects as occupied */
> > >  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> > >  		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
> > > -		int ret;
> > > +
> > >  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
> > >  			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
> > >  
> > > @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> > >  	/* And finally clear the reserved guard page */
> > >  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
> > >  
> > > +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
> > > +		struct i915_hw_ppgtt *ppgtt;
> > > +
> > > +		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> > > +		if (!ppgtt)
> > > +			return -ENOMEM;
> > > +
> > > +		ret = __hw_ppgtt_init(dev, ppgtt);
> > > +		if (!ret)
> > > +			return ret;
> > > +
> > > +		dev_priv->mm.aliasing_ppgtt = ppgtt;
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 1.9.3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ben Widawsky, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 4/7] drm/i915: Allow i915_gem_setup_global_gtt to fail

[Chris Wilson]

On Wed, Jul 30, 2014 at 09:42:01PM +0200, Daniel Vetter wrote:
> We already needs this just as a safety check in case the preallocation
> reservation dance fails. But we definitely need this to be able to
> move tha aliasing ppgtt setup back out of the context code to this
> place, where it belongs.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c     |  7 ++++++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 16 ++++++++++------
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  4 ++--
>  3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f4e57fe05c6a..d8399ee622b9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4762,7 +4762,12 @@ int i915_gem_init(struct drm_device *dev)
>  			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
>  	}
>  
> -	i915_gem_init_userptr(dev);
> +	ret = i915_gem_init_userptr(dev);
> +	if (ret) {

Just throw an error and keep on initialising. Optional features like
this and stolen, we can keep going - so we should drop the error code
return entirely for a better error message when we disable the feature.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/7] drm/i915: Only refcount ppgtt if it actually is one

[Chris Wilson]

On Wed, Jul 30, 2014 at 09:41:59PM +0200, Daniel Vetter wrote:
> This essentially unbreaks non-ppgtt operation where we'd scribble over
> random memory.
> 
> While at it give the vm_to_ppgtt function a proper prefix and make it
> a bit more paranoid.

Wrong direction. If you make ggtt/ppgtt more similar we can loose
having to write fragile code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/7] drm/i915: Only refcount ppgtt if it actually is one

[Daniel Vetter]

On Thu, Jul 31, 2014 at 8:52 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Jul 30, 2014 at 09:41:59PM +0200, Daniel Vetter wrote:
>> This essentially unbreaks non-ppgtt operation where we'd scribble over
>> random memory.
>>
>> While at it give the vm_to_ppgtt function a proper prefix and make it
>> a bit more paranoid.
>
> Wrong direction. If you make ggtt/ppgtt more similar we can loose
> having to write fragile code.

So moving the kref into the address space? I guess that would work
too, but I'm off for my extended w/e now ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/7] drm/i915: Only refcount ppgtt if it actually is one

[Daniel Vetter]

On Thu, Jul 31, 2014 at 9:39 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Thu, Jul 31, 2014 at 8:52 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Wed, Jul 30, 2014 at 09:41:59PM +0200, Daniel Vetter wrote:
>>> This essentially unbreaks non-ppgtt operation where we'd scribble over
>>> random memory.
>>>
>>> While at it give the vm_to_ppgtt function a proper prefix and make it
>>> a bit more paranoid.
>>
>> Wrong direction. If you make ggtt/ppgtt more similar we can loose
>> having to write fragile code.
>
> So moving the kref into the address space? I guess that would work
> too, but I'm off for my extended w/e now ;-)

Ok got bored and tried to implement this, but it doesn't work out too
well. It needs a fair bit of reorg so that we can handle the global
gtt and the ppgtt with the same refcounting scheme and clean them up
properly. I think as a stop-gap this one here is better and the
refcounting unification needs to be done on top.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter]

On Thu, Jul 31, 2014 at 5:47 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Wed, Jul 30, 2014 at 08:46:11PM -0700, Ben Widawsky wrote:
>> <1386367941-7131-76-git-send-email-benjamin.widawsky@intel.com>
>
> and
>
> <1386367941-7131-81-git-send-email-benjamin.widawsky@intel.com>

Both of these are merged, so I really don't follow what you're trying
to tell here. So this can't be the usual "I've written this long ago,
why was I ignored" complaint. Slightly more context instead of a bunch
of old msg-ids highly appreciated.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/7] drm/i915: Track file_priv, not ctx in the ppgtt structure

[Thierry, Michel]

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


> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Wednesday, July 30, 2014 8:42 PM
> To: Intel Graphics Development
> Cc: Daniel Vetter
> Subject: [Intel-gfx] [PATCH 1/7] drm/i915: Track file_priv, not ctx in the
ppgtt
> structure
> 
> Hardware contexts reference a ppgtt, not the other way round. And the
> only user of this (in debugfs) actually only cares about which file
> the ppgtt is associated with. So give it what it wants.
> 
> While at it give the ppgtt create function a proper name&place.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c | 22 +---------------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 21 +++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_gtt.h     |  6 +++++-
>  4 files changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9e737b771c40..3bf1d20c598b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -333,7 +333,7 @@ static int per_file_stats(int id, void *ptr, void
*data)
>  			}
> 
>  			ppgtt = container_of(vma->vm, struct
> i915_hw_ppgtt, base);
> -			if (ppgtt->ctx && ppgtt->ctx->file_priv != stats-
> >file_priv)
> +			if (ppgtt->file_priv != stats->file_priv)
>  				continue;
> 
>  			if (obj->ring) /* XXX per-vma statistic */
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 899c6a7a5920..3b8367aa8404 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -181,26 +181,6 @@ i915_gem_alloc_context_obj(struct drm_device
> *dev, size_t size)
>  	return obj;
>  }
> 
> -static struct i915_hw_ppgtt *
> -create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx)
> -{
> -	struct i915_hw_ppgtt *ppgtt;
> -	int ret;
> -
> -	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> -	if (!ppgtt)
> -		return ERR_PTR(-ENOMEM);
> -
> -	ret = i915_ppgtt_init(dev, ppgtt);
> -	if (ret) {
> -		kfree(ppgtt);
> -		return ERR_PTR(ret);
> -	}
> -
> -	ppgtt->ctx = ctx;
> -	return ppgtt;
> -}
> -
>  static struct intel_context *
>  __create_hw_context(struct drm_device *dev,
>  		    struct drm_i915_file_private *file_priv)
> @@ -287,7 +267,7 @@ i915_gem_create_context(struct drm_device *dev,
>  	}
> 
>  	if (create_vm) {
> -		struct i915_hw_ppgtt *ppgtt = create_vm_for_ctx(dev, ctx);
> +		struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev,
> file_priv);
> 
>  		if (IS_ERR_OR_NULL(ppgtt)) {
>  			DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index baa94199239b..83ee41e5c1c7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1222,6 +1222,27 @@ int i915_ppgtt_init(struct drm_device *dev, struct
> i915_hw_ppgtt *ppgtt)
>  	return ret;
>  }
> 
> +struct i915_hw_ppgtt *
> +i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private
> *fpriv)
> +{
> +	struct i915_hw_ppgtt *ppgtt;
> +	int ret;
> +
> +	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> +	if (!ppgtt)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = i915_ppgtt_init(dev, ppgtt);
> +	if (ret) {
> +		kfree(ppgtt);
> +		return ERR_PTR(ret);
> +	}
> +
> +	ppgtt->file_priv = fpriv;
> +
> +	return ppgtt;
> +}
> +
>  void  i915_ppgtt_release(struct kref *kref)
>  {
>  	struct i915_hw_ppgtt *ppgtt =
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 380e034c66f8..0b04ef6167f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -34,6 +34,8 @@
>  #ifndef __I915_GEM_GTT_H__
>  #define __I915_GEM_GTT_H__
> 
> +struct drm_i915_file_private;
> +
>  typedef uint32_t gen6_gtt_pte_t;
>  typedef uint64_t gen8_gtt_pte_t;
>  typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
> @@ -258,7 +260,7 @@ struct i915_hw_ppgtt {
>  		dma_addr_t *gen8_pt_dma_addr[4];
>  	};
> 
> -	struct intel_context *ctx;
> +	struct drm_i915_file_private *file_priv;
> 
>  	int (*enable)(struct i915_hw_ppgtt *ppgtt);
>  	int (*switch_mm)(struct i915_hw_ppgtt *ppgtt,
> @@ -276,6 +278,8 @@ bool intel_enable_ppgtt(struct drm_device *dev,
> bool full);
> 
>  int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
>  void i915_ppgtt_release(struct kref *kref);
> +struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev,
> +					struct drm_i915_file_private
*fpriv);
>  static inline void i915_ppgtt_get(struct i915_hw_ppgtt *ppgtt)
>  {
>  	if (ppgtt)
> --
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

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

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

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

Re: [PATCH 3/7] drm/i915: Add proper prefix to obj_to_ggtt

[Thierry, Michel]

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



> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Wednesday, July 30, 2014 8:42 PM
> To: Intel Graphics Development
> Cc: Daniel Vetter
> Subject: [Intel-gfx] [PATCH 3/7] drm/i915: Add proper prefix to
obj_to_ggtt
> 
> Stuff in headers really aught to have this.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 11 ++++++-----
>  drivers/gpu/drm/i915/i915_gem.c |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
Reviewed-by: Michel Thierry <michel.thierry@intel.com>

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

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

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

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Thierry, Michel]

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



> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Wednesday, July 30, 2014 8:42 PM
> To: Intel Graphics Development
> Cc: Daniel Vetter
> Subject: [Intel-gfx] [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt
as part
> of global gtt
> 
> Stuffing this into the context setup code doesn't make a lot of sense.
> Also reusing the real ppgtt setup code makes even less sense since the
> aliasing ppgtt isn't a real address space. Leaving all that stuff
> unitialized will make sure that we catch any abusers promptly.
> 
> This is also a prep work to clean up the context->ppgtt link.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 31
> +++++++++++++++++++++++++------
>  2 files changed, 26 insertions(+), 18 deletions(-)
> 
> @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct drm_device
> *dev,
>  	/* And finally clear the reserved guard page */
>  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
> 
> +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
> +		struct i915_hw_ppgtt *ppgtt;
> +
> +		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> +		if (!ppgtt)
> +			return -ENOMEM;
> +
> +		ret = __hw_ppgtt_init(dev, ppgtt);
> +		if (!ret)
__hw_ppgtt_init will return '0' if successful.
  
> +			return ret;
> +
> +		dev_priv->mm.aliasing_ppgtt = ppgtt;
> +	}
> +
>  	return 0;
>  }
> 
> --
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

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

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Ville Syrjälä]

On Wed, Jul 30, 2014 at 09:42:02PM +0200, Daniel Vetter wrote:
> Stuffing this into the context setup code doesn't make a lot of sense.
> Also reusing the real ppgtt setup code makes even less sense since the
> aliasing ppgtt isn't a real address space. Leaving all that stuff
> unitialized will make sure that we catch any abusers promptly.
> 
> This is also a prep work to clean up the context->ppgtt link.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 31 +++++++++++++++++++++++++------
>  2 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3b8367aa8404..7a455fcee3a7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
>  			goto err_unpin;
>  		} else
>  			ctx->vm = &ppgtt->base;
> -
> -		/* This case is reserved for the global default context and
> -		 * should only happen once. */
> -		if (is_global_default_ctx) {
> -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> -				ret = -EEXIST;
> -				goto err_unpin;
> -			}
> -
> -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> -		}
>  	} else if (USES_PPGTT(dev)) {
>  		/* For platforms which only have aliasing PPGTT, we fake the
>  		 * address space and refcounting. */
> @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
>  		}
>  	}
>  
> -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> +	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
>  	if (IS_ERR(ctx)) {
>  		DRM_ERROR("Failed to create default global context (error %ld)\n",
>  			  PTR_ERR(ctx));
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4fa7807ed4d5..cb9bb13ff20a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1191,23 +1191,27 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	return 0;
>  }
>  
> -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> +int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret = 0;
>  
>  	ppgtt->base.dev = dev;
>  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
>  
>  	if (INTEL_INFO(dev)->gen < 8)
> -		ret = gen6_ppgtt_init(ppgtt);
> +		return gen6_ppgtt_init(ppgtt);
>  	else if (IS_GEN8(dev))
> -		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> +		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
>  	else
>  		BUG();
> +}
> +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;
>  
> +	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (!ret) {
> -		struct drm_i915_private *dev_priv = dev->dev_private;
>  		kref_init(&ppgtt->ref);
>  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
>  			    ppgtt->base.total);
> @@ -1728,6 +1732,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	struct drm_mm_node *entry;
>  	struct drm_i915_gem_object *obj;
>  	unsigned long hole_start, hole_end;
> +	int ret;
>  
>  	BUG_ON(mappable_end > end);
>  
> @@ -1739,7 +1744,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	/* Mark any preallocated objects as occupied */
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>  		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
> -		int ret;
> +
>  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
>  			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
>  
> @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	/* And finally clear the reserved guard page */
>  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
>  
> +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {

Should that be !USES_FULL_PPGTT ?

> +		struct i915_hw_ppgtt *ppgtt;
> +
> +		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> +		if (!ppgtt)
> +			return -ENOMEM;
> +
> +		ret = __hw_ppgtt_init(dev, ppgtt);
> +		if (!ret)
> +			return ret;
> +
> +		dev_priv->mm.aliasing_ppgtt = ppgtt;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Thierry, Michel]

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



> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Ville Syrjälä
> Sent: Thursday, July 31, 2014 5:16 PM
> To: Daniel Vetter
> Cc: Intel Graphics Development
> Subject: Re: [Intel-gfx] [PATCH 5/7] drm/i915: Initialize the aliasing
ppgtt as
> part of global gtt
> 
> On Wed, Jul 30, 2014 at 09:42:02PM +0200, Daniel Vetter wrote:
> > Stuffing this into the context setup code doesn't make a lot of sense.
> > Also reusing the real ppgtt setup code makes even less sense since the
> > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > unitialized will make sure that we catch any abusers promptly.
> >
> > This is also a prep work to clean up the context->ppgtt link.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
> >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 31
> +++++++++++++++++++++++++------
> >  2 files changed, 26 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 3b8367aa8404..7a455fcee3a7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device
> *dev,
> >  			goto err_unpin;
> >  		} else
> >  			ctx->vm = &ppgtt->base;
> > -
> > -		/* This case is reserved for the global default context and
> > -		 * should only happen once. */
> > -		if (is_global_default_ctx) {
> > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > -				ret = -EEXIST;
> > -				goto err_unpin;
> > -			}
> > -
> > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > -		}
> >  	} else if (USES_PPGTT(dev)) {
> >  		/* For platforms which only have aliasing PPGTT, we fake the
> >  		 * address space and refcounting. */
> > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
> >  		}
> >  	}
> >
> > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > +	ctx = i915_gem_create_context(dev, NULL,
> USES_FULL_PPGTT(dev));
> >  	if (IS_ERR(ctx)) {
> >  		DRM_ERROR("Failed to create default global context (error
> %ld)\n",
> >  			  PTR_ERR(ctx));
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 4fa7807ed4d5..cb9bb13ff20a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1191,23 +1191,27 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt
> *ppgtt)
> >  	return 0;
> >  }
> >
> > -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt
*ppgtt)
> > +int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt
> *ppgtt)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int ret = 0;
> >
> >  	ppgtt->base.dev = dev;
> >  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
> >
> >  	if (INTEL_INFO(dev)->gen < 8)
> > -		ret = gen6_ppgtt_init(ppgtt);
> > +		return gen6_ppgtt_init(ppgtt);
> >  	else if (IS_GEN8(dev))
> > -		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> > +		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> >  	else
> >  		BUG();
> > +}
> > +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt
*ppgtt)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret = 0;
> >
> > +	ret = __hw_ppgtt_init(dev, ppgtt);
> >  	if (!ret) {
> > -		struct drm_i915_private *dev_priv = dev->dev_private;
> >  		kref_init(&ppgtt->ref);
> >  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> >  			    ppgtt->base.total);
> > @@ -1728,6 +1732,7 @@ int i915_gem_setup_global_gtt(struct
> drm_device *dev,
> >  	struct drm_mm_node *entry;
> >  	struct drm_i915_gem_object *obj;
> >  	unsigned long hole_start, hole_end;
> > +	int ret;
> >
> >  	BUG_ON(mappable_end > end);
> >
> > @@ -1739,7 +1744,7 @@ int i915_gem_setup_global_gtt(struct
> drm_device *dev,
> >  	/* Mark any preallocated objects as occupied */
> >  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> >  		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
> ggtt_vm);
> > -		int ret;
> > +
> >  		DRM_DEBUG_KMS("reserving preallocated space: %lx +
> %zx\n",
> >  			      i915_gem_obj_ggtt_offset(obj),
obj->base.size);
> >
> > @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct
> drm_device *dev,
> >  	/* And finally clear the reserved guard page */
> >  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
> >
> > +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
> 
> Should that be !USES_FULL_PPGTT ?
I think we need this for aliasing & full ppgtt (the global default ctx)...
so it should be USES_PPGTT.
> 
> > +		struct i915_hw_ppgtt *ppgtt;
> > +
> > +		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> > +		if (!ppgtt)
> > +			return -ENOMEM;
> > +
> > +		ret = __hw_ppgtt_init(dev, ppgtt);
> > +		if (!ret)
> > +			return ret;
> > +
> > +		dev_priv->mm.aliasing_ppgtt = ppgtt;
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

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

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter]

On Thu, Jul 31, 2014 at 07:15:52PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 30, 2014 at 09:42:02PM +0200, Daniel Vetter wrote:
> > Stuffing this into the context setup code doesn't make a lot of sense.
> > Also reusing the real ppgtt setup code makes even less sense since the
> > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > unitialized will make sure that we catch any abusers promptly.
> > 
> > This is also a prep work to clean up the context->ppgtt link.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +------------
> >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 31 +++++++++++++++++++++++++------
> >  2 files changed, 26 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 3b8367aa8404..7a455fcee3a7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
> >  			goto err_unpin;
> >  		} else
> >  			ctx->vm = &ppgtt->base;
> > -
> > -		/* This case is reserved for the global default context and
> > -		 * should only happen once. */
> > -		if (is_global_default_ctx) {
> > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > -				ret = -EEXIST;
> > -				goto err_unpin;
> > -			}
> > -
> > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > -		}
> >  	} else if (USES_PPGTT(dev)) {
> >  		/* For platforms which only have aliasing PPGTT, we fake the
> >  		 * address space and refcounting. */
> > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
> >  		}
> >  	}
> >  
> > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > +	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
> >  	if (IS_ERR(ctx)) {
> >  		DRM_ERROR("Failed to create default global context (error %ld)\n",
> >  			  PTR_ERR(ctx));
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 4fa7807ed4d5..cb9bb13ff20a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1191,23 +1191,27 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> >  	return 0;
> >  }
> >  
> > -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > +int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int ret = 0;
> >  
> >  	ppgtt->base.dev = dev;
> >  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
> >  
> >  	if (INTEL_INFO(dev)->gen < 8)
> > -		ret = gen6_ppgtt_init(ppgtt);
> > +		return gen6_ppgtt_init(ppgtt);
> >  	else if (IS_GEN8(dev))
> > -		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> > +		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
> >  	else
> >  		BUG();
> > +}
> > +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret = 0;
> >  
> > +	ret = __hw_ppgtt_init(dev, ppgtt);
> >  	if (!ret) {
> > -		struct drm_i915_private *dev_priv = dev->dev_private;
> >  		kref_init(&ppgtt->ref);
> >  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> >  			    ppgtt->base.total);
> > @@ -1728,6 +1732,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> >  	struct drm_mm_node *entry;
> >  	struct drm_i915_gem_object *obj;
> >  	unsigned long hole_start, hole_end;
> > +	int ret;
> >  
> >  	BUG_ON(mappable_end > end);
> >  
> > @@ -1739,7 +1744,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> >  	/* Mark any preallocated objects as occupied */
> >  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> >  		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
> > -		int ret;
> > +
> >  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
> >  			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
> >  
> > @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
> >  	/* And finally clear the reserved guard page */
> >  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
> >  
> > +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
> 
> Should that be !USES_FULL_PPGTT ?

Yup. And since I've also fumbled the check for __hw_ppgtt_init I really
wonder what exactly I've tested on my gen6 here ...

Time to hide and lick wounds I think ;-)
-Daniel

> 
> > +		struct i915_hw_ppgtt *ppgtt;
> > +
> > +		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> > +		if (!ppgtt)
> > +			return -ENOMEM;
> > +
> > +		ret = __hw_ppgtt_init(dev, ppgtt);
> > +		if (!ret)
> > +			return ret;
> > +
> > +		dev_priv->mm.aliasing_ppgtt = ppgtt;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter]

Stuffing this into the context setup code doesn't make a lot of sense.
Also reusing the real ppgtt setup code makes even less sense since the
aliasing ppgtt isn't a real address space. Leaving all that stuff
unitialized will make sure that we catch any abusers promptly.

This is also a prep work to clean up the context->ppgtt link.

v2: Fix up the logic fail, I've fumbled it so badly to completely
disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
the pde write into the gen6 init function, since otherwise it won't
work at all.

Cc: "Thierry, Michel" <michel.thierry@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 13 +---------
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 42 +++++++++++++++++++++++----------
 2 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3b8367aa8404..7a455fcee3a7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
 			goto err_unpin;
 		} else
 			ctx->vm = &ppgtt->base;
-
-		/* This case is reserved for the global default context and
-		 * should only happen once. */
-		if (is_global_default_ctx) {
-			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
-				ret = -EEXIST;
-				goto err_unpin;
-			}
-
-			dev_priv->mm.aliasing_ppgtt = ppgtt;
-		}
 	} else if (USES_PPGTT(dev)) {
 		/* For platforms which only have aliasing PPGTT, we fake the
 		 * address space and refcounting. */
@@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
 		}
 	}
 
-	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
+	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev));
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context (error %ld)\n",
 			  PTR_ERR(ctx));
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4fa7807ed4d5..a4c1740d79be 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1188,35 +1188,38 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 			 ppgtt->node.size >> 20,
 			 ppgtt->node.start / PAGE_SIZE);
 
+	gen6_write_pdes(ppgtt);
+	DRM_DEBUG("Adding PPGTT at offset %x\n",
+		  ppgtt->pd_offset << 10);
+
 	return 0;
 }
 
-int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret = 0;
 
 	ppgtt->base.dev = dev;
 	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
 
 	if (INTEL_INFO(dev)->gen < 8)
-		ret = gen6_ppgtt_init(ppgtt);
+		return gen6_ppgtt_init(ppgtt);
 	else if (IS_GEN8(dev))
-		ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
+		return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total);
 	else
 		BUG();
+}
+int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
 
-	if (!ret) {
-		struct drm_i915_private *dev_priv = dev->dev_private;
+	ret = __hw_ppgtt_init(dev, ppgtt);
+	if (ret == 0) {
 		kref_init(&ppgtt->ref);
 		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
 			    ppgtt->base.total);
 		i915_init_vm(dev_priv, &ppgtt->base);
-		if (INTEL_INFO(dev)->gen < 8) {
-			gen6_write_pdes(ppgtt);
-			DRM_DEBUG("Adding PPGTT at offset %x\n",
-				  ppgtt->pd_offset << 10);
-		}
 	}
 
 	return ret;
@@ -1728,6 +1731,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 	struct drm_mm_node *entry;
 	struct drm_i915_gem_object *obj;
 	unsigned long hole_start, hole_end;
+	int ret;
 
 	BUG_ON(mappable_end > end);
 
@@ -1739,7 +1743,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 	/* Mark any preallocated objects as occupied */
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
-		int ret;
+
 		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
 			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
 
@@ -1766,6 +1770,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 	/* And finally clear the reserved guard page */
 	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
 
+	if (HAS_ALIASING_PPGTT(dev) && !USES_FULL_PPGTT(dev)) {
+		struct i915_hw_ppgtt *ppgtt;
+
+		ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
+		if (!ppgtt)
+			return -ENOMEM;
+
+		ret = __hw_ppgtt_init(dev, ppgtt);
+		if (ret != 0)
+			return ret;
+
+		dev_priv->mm.aliasing_ppgtt = ppgtt;
+	}
+
 	return 0;
 }
 
-- 
1.9.3

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

[PATCH] drm/i915: Only track real ppgtt for a context

[Daniel Vetter]

There's a bit a confusion since we track the global gtt,
the aliasing and real ppgtt in the ctx->vm pointer. And not
all callers really bother to check for the different cases and just
presume that it points to a real ppgtt.

Now looking closely we don't actually need ->vm to always point at an
address space - the only place that cares actually has fixup code
already to decide whether to look at the per-proces or the global
address space.

So switch to just tracking the ppgtt directly and ditch all the
extraneous code.

v2: Fixup the ppgtt debugfs file to not oops on a NULL ctx->ppgtt.
Also drop the early exit - without aliasing ppgtt we want to dump all
the ppgtts of the contexts if we have full ppgtt.

Cc: "Thierry, Michel" <michel.thierry@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
OTC-Jira: VIZ-3724
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 11 ++++++++---
 drivers/gpu/drm/i915/i915_drv.h            |  3 +--
 drivers/gpu/drm/i915/i915_gem_context.c    | 28 +++++++---------------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +++--
 drivers/gpu/drm/i915/i915_gpu_error.c      | 10 +++++++---
 5 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3bf1d20c598b..2bd4beada1bd 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1774,7 +1774,13 @@ static int per_file_ctx(int id, void *ptr, void *data)
 {
 	struct intel_context *ctx = ptr;
 	struct seq_file *m = data;
-	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(ctx);
+	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
+
+	if (!ppgtt) {
+		seq_puts(m, "  no ppgtt for context %d\n",
+			 ctx->user_handle);
+		return 0;
+	}
 
 	if (i915_gem_context_is_default(ctx))
 		seq_puts(m, "  default context:\n");
@@ -1834,8 +1840,7 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 		seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset);
 
 		ppgtt->debug_dump(ppgtt, m);
-	} else
-		return;
+	}
 
 	list_for_each_entry_reverse(file, &dev->filelist, lhead) {
 		struct drm_i915_file_private *file_priv = file->driver_priv;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0762e19f9bc6..3230b08aff13 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -616,7 +616,7 @@ struct intel_context {
 	uint8_t remap_slice;
 	struct drm_i915_file_private *file_priv;
 	struct i915_ctx_hang_stats hang_stats;
-	struct i915_address_space *vm;
+	struct i915_hw_ppgtt *ppgtt;
 
 	struct {
 		struct drm_i915_gem_object *rcs_state;
@@ -2504,7 +2504,6 @@ i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
 void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
 
 /* i915_gem_context.c */
-#define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
 int __must_check i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_reset(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 7a455fcee3a7..c00e5d027774 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -136,15 +136,8 @@ void i915_gem_context_free(struct kref *ctx_ref)
 {
 	struct intel_context *ctx = container_of(ctx_ref,
 						   typeof(*ctx), ref);
-	struct i915_hw_ppgtt *ppgtt = NULL;
 
-	if (ctx->legacy_hw_ctx.rcs_state) {
-		/* We refcount even the aliasing PPGTT to keep the code symmetric */
-		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
-			ppgtt = ctx_to_ppgtt(ctx);
-	}
-
-	i915_ppgtt_put(ppgtt);
+	i915_ppgtt_put(ctx->ppgtt);
 	if (ctx->legacy_hw_ctx.rcs_state)
 		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
 	list_del(&ctx->link);
@@ -240,7 +233,6 @@ i915_gem_create_context(struct drm_device *dev,
 			bool create_vm)
 {
 	const bool is_global_default_ctx = file_priv == NULL;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_context *ctx;
 	int ret = 0;
 
@@ -274,15 +266,10 @@ i915_gem_create_context(struct drm_device *dev,
 					 PTR_ERR(ppgtt));
 			ret = PTR_ERR(ppgtt);
 			goto err_unpin;
-		} else
-			ctx->vm = &ppgtt->base;
-	} else if (USES_PPGTT(dev)) {
-		/* For platforms which only have aliasing PPGTT, we fake the
-		 * address space and refcounting. */
-		ctx->vm = &dev_priv->mm.aliasing_ppgtt->base;
-		i915_ppgtt_get(dev_priv->mm.aliasing_ppgtt);
-	} else
-		ctx->vm = &dev_priv->gtt.base;
+		}
+
+		ctx->ppgtt = ppgtt;
+	}
 
 	return ctx;
 
@@ -538,7 +525,6 @@ static int do_switch(struct intel_engine_cs *ring,
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct intel_context *from = ring->last_context;
-	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
 	int ret, i;
@@ -566,8 +552,8 @@ static int do_switch(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
-		ret = ppgtt->switch_mm(ppgtt, ring, false);
+	if (to->ppgtt) {
+		ret = to->ppgtt->switch_mm(to->ppgtt, ring, false);
 		if (ret)
 			goto unpin_out;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 60998fc4e5b2..54b3d8c8b228 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1318,8 +1318,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	i915_gem_context_reference(ctx);
 
-	vm = ctx->vm;
-	if (!USES_FULL_PPGTT(dev))
+	if (ctx->ppgtt)
+		vm = &ctx->ppgtt->base;
+	else
 		vm = &dev_priv->gtt.base;
 
 	eb = eb_create(args);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 0b3f69439451..06072120b15d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -958,6 +958,12 @@ static void i915_gem_record_rings(struct drm_device *dev,
 
 		request = i915_gem_find_active_request(ring);
 		if (request) {
+			struct i915_address_space *vm;
+
+			vm = request->ctx && request->ctx->ppgtt ?
+				&request->ctx->ppgtt->base :
+				&dev_priv->gtt.base;
+
 			/* We need to copy these to an anonymous buffer
 			 * as the simplest method to avoid being overwritten
 			 * by userspace.
@@ -965,9 +971,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 			error->ring[i].batchbuffer =
 				i915_error_object_create(dev_priv,
 							 request->batch_obj,
-							 request->ctx ?
-							 request->ctx->vm :
-							 &dev_priv->gtt.base);
+							 vm);
 
 			if (HAS_BROKEN_CS_TLB(dev_priv->dev) &&
 			    ring->scratch.obj)
-- 
1.9.3

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

Re: [PATCH 5/7] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter]

On Fri, Aug 01, 2014 at 09:54:09AM +0000, Thierry, Michel wrote:
> > > @@ -1766,6 +1771,20 @@ int i915_gem_setup_global_gtt(struct
> > drm_device *dev,
> > >  	/* And finally clear the reserved guard page */
> > >  	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
> > >
> > > +	if (HAS_ALIASING_PPGTT(dev) && USES_FULL_PPGTT(dev)) {
> > 
> > Should that be !USES_FULL_PPGTT ?
> I think we need this for aliasing & full ppgtt (the global default ctx)...
> so it should be USES_PPGTT.

We want to setup the aliasing ppgtt _only_ when required, so we don't
actually need it for USES_FULL_PPGTT. So Ville's right here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Only track real ppgtt for a context

[Thierry, Michel]

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



> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> Sent: Monday, August 04, 2014 3:21 PM
> To: Intel Graphics Development
> Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
> Subject: [PATCH] drm/i915: Only track real ppgtt for a context
> 
> There's a bit a confusion since we track the global gtt,
> the aliasing and real ppgtt in the ctx->vm pointer. And not
> all callers really bother to check for the different cases and just
> presume that it points to a real ppgtt.
> 
> Now looking closely we don't actually need ->vm to always point at an
> address space - the only place that cares actually has fixup code
> already to decide whether to look at the per-proces or the global
> address space.
> 
> So switch to just tracking the ppgtt directly and ditch all the
> extraneous code.
> 
> v2: Fixup the ppgtt debugfs file to not oops on a NULL ctx->ppgtt.
> Also drop the early exit - without aliasing ppgtt we want to dump all
> the ppgtts of the contexts if we have full ppgtt.
> 
> Cc: "Thierry, Michel" <michel.thierry@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> OTC-Jira: VIZ-3724
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        | 11 ++++++++---
>  drivers/gpu/drm/i915/i915_drv.h            |  3 +--
>  drivers/gpu/drm/i915/i915_gem_context.c    | 28 +++++++---------------------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +++--
>  drivers/gpu/drm/i915/i915_gpu_error.c      | 10 +++++++---
>  5 files changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3bf1d20c598b..2bd4beada1bd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1774,7 +1774,13 @@ static int per_file_ctx(int id, void *ptr, void *data)
>  {
>  	struct intel_context *ctx = ptr;
>  	struct seq_file *m = data;
> -	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(ctx);
> +	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
> +
> +	if (!ppgtt) {
> +		seq_puts(m, "  no ppgtt for context %d\n",
> +			 ctx->user_handle);
Should be seq_printf().

> +		return 0;
> +	}
> 
>  	if (i915_gem_context_is_default(ctx))
>  		seq_puts(m, "  default context:\n");
> @@ -1834,8 +1840,7 @@ static void gen6_ppgtt_info(struct seq_file *m,
> --
> 1.9.3

Apart from that, 

Reviewed-by: Michel Thierry <michel.thierry@intel.com>


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

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

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

[PATCH] drm/i915: Only track real ppgtt for a context

[Daniel Vetter]

There's a bit a confusion since we track the global gtt,
the aliasing and real ppgtt in the ctx->vm pointer. And not
all callers really bother to check for the different cases and just
presume that it points to a real ppgtt.

Now looking closely we don't actually need ->vm to always point at an
address space - the only place that cares actually has fixup code
already to decide whether to look at the per-proces or the global
address space.

So switch to just tracking the ppgtt directly and ditch all the
extraneous code.

v2: Fixup the ppgtt debugfs file to not oops on a NULL ctx->ppgtt.
Also drop the early exit - without aliasing ppgtt we want to dump all
the ppgtts of the contexts if we have full ppgtt.

v3: Actually git add the compile fix.

Reviewed-by: Michel Thierry <michel.thierry@intel.com>
Cc: "Thierry, Michel" <michel.thierry@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
OTC-Jira: VIZ-3724
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 11 ++++++++---
 drivers/gpu/drm/i915/i915_drv.h            |  3 +--
 drivers/gpu/drm/i915/i915_gem_context.c    | 28 +++++++---------------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +++--
 drivers/gpu/drm/i915/i915_gpu_error.c      | 10 +++++++---
 5 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3bf1d20c598b..7e8d94b53333 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1774,7 +1774,13 @@ static int per_file_ctx(int id, void *ptr, void *data)
 {
 	struct intel_context *ctx = ptr;
 	struct seq_file *m = data;
-	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(ctx);
+	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
+
+	if (!ppgtt) {
+		seq_printf(m, "  no ppgtt for context %d\n",
+			   ctx->user_handle);
+		return 0;
+	}
 
 	if (i915_gem_context_is_default(ctx))
 		seq_puts(m, "  default context:\n");
@@ -1834,8 +1840,7 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 		seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset);
 
 		ppgtt->debug_dump(ppgtt, m);
-	} else
-		return;
+	}
 
 	list_for_each_entry_reverse(file, &dev->filelist, lhead) {
 		struct drm_i915_file_private *file_priv = file->driver_priv;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0762e19f9bc6..3230b08aff13 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -616,7 +616,7 @@ struct intel_context {
 	uint8_t remap_slice;
 	struct drm_i915_file_private *file_priv;
 	struct i915_ctx_hang_stats hang_stats;
-	struct i915_address_space *vm;
+	struct i915_hw_ppgtt *ppgtt;
 
 	struct {
 		struct drm_i915_gem_object *rcs_state;
@@ -2504,7 +2504,6 @@ i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
 void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
 
 /* i915_gem_context.c */
-#define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
 int __must_check i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_reset(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 7a455fcee3a7..c00e5d027774 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -136,15 +136,8 @@ void i915_gem_context_free(struct kref *ctx_ref)
 {
 	struct intel_context *ctx = container_of(ctx_ref,
 						   typeof(*ctx), ref);
-	struct i915_hw_ppgtt *ppgtt = NULL;
 
-	if (ctx->legacy_hw_ctx.rcs_state) {
-		/* We refcount even the aliasing PPGTT to keep the code symmetric */
-		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
-			ppgtt = ctx_to_ppgtt(ctx);
-	}
-
-	i915_ppgtt_put(ppgtt);
+	i915_ppgtt_put(ctx->ppgtt);
 	if (ctx->legacy_hw_ctx.rcs_state)
 		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
 	list_del(&ctx->link);
@@ -240,7 +233,6 @@ i915_gem_create_context(struct drm_device *dev,
 			bool create_vm)
 {
 	const bool is_global_default_ctx = file_priv == NULL;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_context *ctx;
 	int ret = 0;
 
@@ -274,15 +266,10 @@ i915_gem_create_context(struct drm_device *dev,
 					 PTR_ERR(ppgtt));
 			ret = PTR_ERR(ppgtt);
 			goto err_unpin;
-		} else
-			ctx->vm = &ppgtt->base;
-	} else if (USES_PPGTT(dev)) {
-		/* For platforms which only have aliasing PPGTT, we fake the
-		 * address space and refcounting. */
-		ctx->vm = &dev_priv->mm.aliasing_ppgtt->base;
-		i915_ppgtt_get(dev_priv->mm.aliasing_ppgtt);
-	} else
-		ctx->vm = &dev_priv->gtt.base;
+		}
+
+		ctx->ppgtt = ppgtt;
+	}
 
 	return ctx;
 
@@ -538,7 +525,6 @@ static int do_switch(struct intel_engine_cs *ring,
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct intel_context *from = ring->last_context;
-	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
 	int ret, i;
@@ -566,8 +552,8 @@ static int do_switch(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
-		ret = ppgtt->switch_mm(ppgtt, ring, false);
+	if (to->ppgtt) {
+		ret = to->ppgtt->switch_mm(to->ppgtt, ring, false);
 		if (ret)
 			goto unpin_out;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 60998fc4e5b2..54b3d8c8b228 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1318,8 +1318,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	i915_gem_context_reference(ctx);
 
-	vm = ctx->vm;
-	if (!USES_FULL_PPGTT(dev))
+	if (ctx->ppgtt)
+		vm = &ctx->ppgtt->base;
+	else
 		vm = &dev_priv->gtt.base;
 
 	eb = eb_create(args);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 0b3f69439451..06072120b15d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -958,6 +958,12 @@ static void i915_gem_record_rings(struct drm_device *dev,
 
 		request = i915_gem_find_active_request(ring);
 		if (request) {
+			struct i915_address_space *vm;
+
+			vm = request->ctx && request->ctx->ppgtt ?
+				&request->ctx->ppgtt->base :
+				&dev_priv->gtt.base;
+
 			/* We need to copy these to an anonymous buffer
 			 * as the simplest method to avoid being overwritten
 			 * by userspace.
@@ -965,9 +971,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 			error->ring[i].batchbuffer =
 				i915_error_object_create(dev_priv,
 							 request->batch_obj,
-							 request->ctx ?
-							 request->ctx->vm :
-							 &dev_priv->gtt.base);
+							 vm);
 
 			if (HAS_BROKEN_CS_TLB(dev_priv->dev) &&
 			    ring->scratch.obj)
-- 
1.9.3

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

Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Thierry, Michel]

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



> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> Sent: Monday, August 04, 2014 3:19 PM
> To: Intel Graphics Development
> Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
> Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt
> 
> Stuffing this into the context setup code doesn't make a lot of sense.
> Also reusing the real ppgtt setup code makes even less sense since the
> aliasing ppgtt isn't a real address space. Leaving all that stuff
> unitialized will make sure that we catch any abusers promptly.
> 
> This is also a prep work to clean up the context->ppgtt link.
> 
> v2: Fix up the logic fail, I've fumbled it so badly to completely
> disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
> the pde write into the gen6 init function, since otherwise it won't
> work at all.
> 
> Cc: "Thierry, Michel" <michel.thierry@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 13 +---------
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 42 +++++++++++++++++++++++-
> ---------
>  2 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3b8367aa8404..7a455fcee3a7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
>  			goto err_unpin;
>  		} else
>  			ctx->vm = &ppgtt->base;
> -
> -		/* This case is reserved for the global default context and
> -		 * should only happen once. */
> -		if (is_global_default_ctx) {
> -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> -				ret = -EEXIST;
> -				goto err_unpin;
> -			}
> -
> -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> -		}

I expect some problems with full ppgtt & this (in some places, the driver is still making some decisions based on dev_priv->mm.aliasing_ppgtt, which now will be null). 
Should I address these problems in a subsequent patch?

>  	} else if (USES_PPGTT(dev)) {
>  		/* For platforms which only have aliasing PPGTT, we fake the
>  		 * address space and refcounting. */
> @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
>  		}
>  	}
> 
> -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> +	ctx = i915_gem_create_context(dev, NULL,
> USES_FULL_PPGTT(dev));
>  	if (IS_ERR(ctx)) {
>  		DRM_ERROR("Failed to create default global context (error
> %ld)\n",
>  			  PTR_ERR(ctx));
>  }
> 
> --
> 1.9.3


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

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

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

Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter]

On Wed, Aug 06, 2014 at 08:18:52AM +0000, Thierry, Michel wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> > Sent: Monday, August 04, 2014 3:19 PM
> > To: Intel Graphics Development
> > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
> > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt
> > 
> > Stuffing this into the context setup code doesn't make a lot of sense.
> > Also reusing the real ppgtt setup code makes even less sense since the
> > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > unitialized will make sure that we catch any abusers promptly.
> > 
> > This is also a prep work to clean up the context->ppgtt link.
> > 
> > v2: Fix up the logic fail, I've fumbled it so badly to completely
> > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
> > the pde write into the gen6 init function, since otherwise it won't
> > work at all.
> > 
> > Cc: "Thierry, Michel" <michel.thierry@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +---------
> >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 42 +++++++++++++++++++++++-
> > ---------
> >  2 files changed, 31 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 3b8367aa8404..7a455fcee3a7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev,
> >  			goto err_unpin;
> >  		} else
> >  			ctx->vm = &ppgtt->base;
> > -
> > -		/* This case is reserved for the global default context and
> > -		 * should only happen once. */
> > -		if (is_global_default_ctx) {
> > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > -				ret = -EEXIST;
> > -				goto err_unpin;
> > -			}
> > -
> > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > -		}
> 
> I expect some problems with full ppgtt & this (in some places, the
> driver is still making some decisions based on
> dev_priv->mm.aliasing_ppgtt, which now will be null).  Should I address
> these problems in a subsequent patch?

Oh, good catch. I've done a bit a review and found two cases:
- Driver unload code. Already fairly broken, made much worse by my series
  here. I've fixed that up yesterday and will resend the series with those
  additional patches and revised patches.
- cmd parser. I guess that should be a fixup patch on top. I'll also do
  that.

Have you spotted any other places?

Thanks, Daniel

> 
> >  	} else if (USES_PPGTT(dev)) {
> >  		/* For platforms which only have aliasing PPGTT, we fake the
> >  		 * address space and refcounting. */
> > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev)
> >  		}
> >  	}
> > 
> > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > +	ctx = i915_gem_create_context(dev, NULL,
> > USES_FULL_PPGTT(dev));
> >  	if (IS_ERR(ctx)) {
> >  		DRM_ERROR("Failed to create default global context (error
> > %ld)\n",
> >  			  PTR_ERR(ctx));
> >  }
> > 
> > --
> > 1.9.3
> 



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Thierry, Michel]

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



> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, August 06, 2014 9:31 AM
> To: Thierry, Michel
> Cc: Daniel Vetter; Intel Graphics Development
> Subject: Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
global gtt
> 
> On Wed, Aug 06, 2014 at 08:18:52AM +0000, Thierry, Michel wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> > > Sent: Monday, August 04, 2014 3:19 PM
> > > To: Intel Graphics Development
> > > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
> > > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
global gtt
> > >
> > > Stuffing this into the context setup code doesn't make a lot of sense.
> > > Also reusing the real ppgtt setup code makes even less sense since the
> > > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > > unitialized will make sure that we catch any abusers promptly.
> > >
> > > This is also a prep work to clean up the context->ppgtt link.
> > >
> > > v2: Fix up the logic fail, I've fumbled it so badly to completely
> > > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
> > > the pde write into the gen6 init function, since otherwise it won't
> > > work at all.
> > >
> > > Cc: "Thierry, Michel" <michel.thierry@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +---------
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 42
> +++++++++++++++++++++++-
> > > ---------
> > >  2 files changed, 31 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > > b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index 3b8367aa8404..7a455fcee3a7 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device
> *dev,
> > >  			goto err_unpin;
> > >  		} else
> > >  			ctx->vm = &ppgtt->base;
> > > -
> > > -		/* This case is reserved for the global default context and
> > > -		 * should only happen once. */
> > > -		if (is_global_default_ctx) {
> > > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > > -				ret = -EEXIST;
> > > -				goto err_unpin;
> > > -			}
> > > -
> > > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > > -		}
> >
> > I expect some problems with full ppgtt & this (in some places, the
> > driver is still making some decisions based on
> > dev_priv->mm.aliasing_ppgtt, which now will be null).  Should I address
> > these problems in a subsequent patch?
> 
> Oh, good catch. I've done a bit a review and found two cases:
> - Driver unload code. Already fairly broken, made much worse by my series
>   here. I've fixed that up yesterday and will resend the series with those
>   additional patches and revised patches.
> - cmd parser. I guess that should be a fixup patch on top. I'll also do
>   that.
> 
> Have you spotted any other places?
The other place is gen8_ring_dispatch_execbuffer, it won't set the ppgtt
flag in MI_BATCH_BUFFER_START.

> 
> Thanks, Daniel
> 
> >
> > >  	} else if (USES_PPGTT(dev)) {
> > >  		/* For platforms which only have aliasing PPGTT, we fake the
> > >  		 * address space and refcounting. */
> > > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device
> *dev)
> > >  		}
> > >  	}
> > >
> > > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > > +	ctx = i915_gem_create_context(dev, NULL,
> > > USES_FULL_PPGTT(dev));
> > >  	if (IS_ERR(ctx)) {
> > >  		DRM_ERROR("Failed to create default global context (error
> > > %ld)\n",
> > >  			  PTR_ERR(ctx));
> > >  }
> > >
> > > --
> > > 1.9.3
> >
> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

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

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

Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter]

On Wed, Aug 06, 2014 at 08:44:34AM +0000, Thierry, Michel wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Wednesday, August 06, 2014 9:31 AM
> > To: Thierry, Michel
> > Cc: Daniel Vetter; Intel Graphics Development
> > Subject: Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
> global gtt
> > 
> > On Wed, Aug 06, 2014 at 08:18:52AM +0000, Thierry, Michel wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> > > > Sent: Monday, August 04, 2014 3:19 PM
> > > > To: Intel Graphics Development
> > > > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
> > > > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
> global gtt
> > > >
> > > > Stuffing this into the context setup code doesn't make a lot of sense.
> > > > Also reusing the real ppgtt setup code makes even less sense since the
> > > > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > > > unitialized will make sure that we catch any abusers promptly.
> > > >
> > > > This is also a prep work to clean up the context->ppgtt link.
> > > >
> > > > v2: Fix up the logic fail, I've fumbled it so badly to completely
> > > > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
> > > > the pde write into the gen6 init function, since otherwise it won't
> > > > work at all.
> > > >
> > > > Cc: "Thierry, Michel" <michel.thierry@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +---------
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 42
> > +++++++++++++++++++++++-
> > > > ---------
> > > >  2 files changed, 31 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > > > b/drivers/gpu/drm/i915/i915_gem_context.c
> > > > index 3b8367aa8404..7a455fcee3a7 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device
> > *dev,
> > > >  			goto err_unpin;
> > > >  		} else
> > > >  			ctx->vm = &ppgtt->base;
> > > > -
> > > > -		/* This case is reserved for the global default context and
> > > > -		 * should only happen once. */
> > > > -		if (is_global_default_ctx) {
> > > > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > > > -				ret = -EEXIST;
> > > > -				goto err_unpin;
> > > > -			}
> > > > -
> > > > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > > > -		}
> > >
> > > I expect some problems with full ppgtt & this (in some places, the
> > > driver is still making some decisions based on
> > > dev_priv->mm.aliasing_ppgtt, which now will be null).  Should I address
> > > these problems in a subsequent patch?
> > 
> > Oh, good catch. I've done a bit a review and found two cases:
> > - Driver unload code. Already fairly broken, made much worse by my series
> >   here. I've fixed that up yesterday and will resend the series with those
> >   additional patches and revised patches.
> > - cmd parser. I guess that should be a fixup patch on top. I'll also do
> >   that.
> > 
> > Have you spotted any other places?
> The other place is gen8_ring_dispatch_execbuffer, it won't set the ppgtt
> flag in MI_BATCH_BUFFER_START.

Indeed. I've also found a few other really fishy places. Will follow up
with revised patches.
-Daniel

> 
> > 
> > Thanks, Daniel
> > 
> > >
> > > >  	} else if (USES_PPGTT(dev)) {
> > > >  		/* For platforms which only have aliasing PPGTT, we fake the
> > > >  		 * address space and refcounting. */
> > > > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device
> > *dev)
> > > >  		}
> > > >  	}
> > > >
> > > > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > > > +	ctx = i915_gem_create_context(dev, NULL,
> > > > USES_FULL_PPGTT(dev));
> > > >  	if (IS_ERR(ctx)) {
> > > >  		DRM_ERROR("Failed to create default global context (error
> > > > %ld)\n",
> > > >  			  PTR_ERR(ctx));
> > > >  }
> > > >
> > > > --
> > > > 1.9.3
> > >
> > 
> > 
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch