[PATCH 1/5] drm/i915: Do not clear mappings beyond VMA size

[PATCH 1/5] drm/i915: Do not clear mappings beyond VMA size

[Joonas Lahtinen]

Do not to clear mappings outside the allocated VMA under any
circumstances. Only clear the smaller of VMA or object page count.

This is required to allow creating partial object VMAs which in
turn are needed for partial GGTT views.

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6fae6bd..ee9ff8e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1947,19 +1947,23 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
 	struct drm_device *dev = vma->vm->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj = vma->obj;
+	const uint64_t size = min_t(uint64_t,
+				    obj->base.size,
+				    vma->node.size);
 
 	if (vma->bound & GLOBAL_BIND) {
 		vma->vm->clear_range(vma->vm,
 				     vma->node.start,
-				     obj->base.size,
+				     size,
 				     true);
 	}
 
 	if (dev_priv->mm.aliasing_ppgtt && vma->bound & LOCAL_BIND) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
+
 		appgtt->base.clear_range(&appgtt->base,
 					 vma->node.start,
-					 obj->base.size,
+					 size,
 					 true);
 	}
 }
-- 
1.7.9.5



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

[PATCH 2/5] drm/i915: Do not make assumptions on GGTT VMA sizes

[Joonas Lahtinen]

GGTT VMA sizes might be smaller than the whole object size due to
different GGTT views.

v2:
- Separate GGTT view constraint calculations from normal view
  constraint calculations (Chris Wilson)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c     |  106 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_gem_gtt.c |   23 ++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h |    4 ++
 3 files changed, 95 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e8f6f4c..111ac8a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3497,7 +3497,8 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
 }
 
 /**
- * Finds free space in the GTT aperture and binds the object there.
+ * Finds free space in the GTT aperture and binds the object or a view of it
+ * there.
  */
 static struct i915_vma *
 i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
@@ -3513,39 +3514,66 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 		flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
 	unsigned long end =
 		flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
+	const bool is_object = !ggtt_view ||
+			       ggtt_view->type == I915_GGTT_VIEW_NORMAL;
 	struct i915_vma *vma;
 	int ret;
 
-	if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
-		return ERR_PTR(-EINVAL);
+	if (i915_is_ggtt(vm)) {
+		u32 view_size;
+
+		if (WARN_ON(!ggtt_view))
+			return ERR_PTR(-EINVAL);
 
-	fence_size = i915_gem_get_gtt_size(dev,
-					   obj->base.size,
-					   obj->tiling_mode);
-	fence_alignment = i915_gem_get_gtt_alignment(dev,
-						     obj->base.size,
-						     obj->tiling_mode, true);
-	unfenced_alignment =
-		i915_gem_get_gtt_alignment(dev,
-					   obj->base.size,
-					   obj->tiling_mode, false);
+		view_size = i915_ggtt_view_size(obj, ggtt_view);
+
+		fence_size = i915_gem_get_gtt_size(dev,
+						   view_size,
+						   obj->tiling_mode);
+		fence_alignment = i915_gem_get_gtt_alignment(dev,
+							     view_size,
+							     obj->tiling_mode,
+							     true);
+		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
+								view_size,
+								obj->tiling_mode,
+								false);
+		size = flags & PIN_MAPPABLE ? fence_size : view_size;
+	} else {
+		fence_size = i915_gem_get_gtt_size(dev,
+						   obj->base.size,
+						   obj->tiling_mode);
+		fence_alignment = i915_gem_get_gtt_alignment(dev,
+							     obj->base.size,
+							     obj->tiling_mode,
+							     true);
+		unfenced_alignment =
+			i915_gem_get_gtt_alignment(dev,
+						   obj->base.size,
+						   obj->tiling_mode,
+						   false);
+		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
+	}
 
 	if (alignment == 0)
 		alignment = flags & PIN_MAPPABLE ? fence_alignment :
 						unfenced_alignment;
 	if (flags & PIN_MAPPABLE && alignment & (fence_alignment - 1)) {
-		DRM_DEBUG("Invalid object alignment requested %u\n", alignment);
+		DRM_DEBUG("Invalid %s alignment requested %u\n",
+			  is_object ? "object" : "GGTT view",
+			  alignment);
 		return ERR_PTR(-EINVAL);
 	}
 
-	size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
-
-	/* If the object is bigger than the entire aperture, reject it early
-	 * before evicting everything in a vain attempt to find space.
+	/* If binding the object/GGTT view requires more space than the entire
+	 * aperture has, reject it early before evicting everything in a vain
+	 * attempt to find space.
 	 */
-	if (obj->base.size > end) {
-		DRM_DEBUG("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%lu\n",
-			  obj->base.size,
+	if (size > end) {
+		DRM_DEBUG("Attempting to bind %s (view type=%u) larger than the aperture: size=%u > %s aperture=%lu\n",
+			  is_object ? "an object" : "a GGTT view ",
+			  is_object ? 0 : ggtt_view->type,
+			  size,
 			  flags & PIN_MAPPABLE ? "mappable" : "total",
 			  end);
 		return ERR_PTR(-E2BIG);
@@ -4207,28 +4235,30 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
-	if ((bound ^ vma->bound) & GLOBAL_BIND) {
-		bool mappable, fenceable;
-		u32 fence_size, fence_alignment;
+	if (!ggtt_view || ggtt_view->type == I915_GGTT_VIEW_NORMAL) {
+		if ((bound ^ vma->bound) & GLOBAL_BIND) {
+			bool mappable, fenceable;
+			u32 fence_size, fence_alignment;
 
-		fence_size = i915_gem_get_gtt_size(obj->base.dev,
-						   obj->base.size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
-							     obj->base.size,
-							     obj->tiling_mode,
-							     true);
+			fence_size = i915_gem_get_gtt_size(obj->base.dev,
+							   obj->base.size,
+							   obj->tiling_mode);
+			fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
+								     obj->base.size,
+								     obj->tiling_mode,
+								     true);
 
-		fenceable = (vma->node.size == fence_size &&
-			     (vma->node.start & (fence_alignment - 1)) == 0);
+			fenceable = (vma->node.size == fence_size &&
+				     (vma->node.start & (fence_alignment - 1)) == 0);
 
-		mappable = (vma->node.start + fence_size <=
-			    dev_priv->gtt.mappable_end);
+			mappable = (vma->node.start + fence_size <=
+				    dev_priv->gtt.mappable_end);
 
-		obj->map_and_fenceable = mappable && fenceable;
-	}
+			obj->map_and_fenceable = mappable && fenceable;
+		}
 
-	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
+		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
+	}
 
 	vma->pin_count++;
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ee9ff8e..5babbd3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2842,3 +2842,26 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 
 	return 0;
 }
+
+/**
+ * i915_ggtt_view_size - Get the size of a GGTT view.
+ * @obj: Object the view is of.
+ * @view: The view in question.
+ *
+ * @return The size of the GGTT view in bytes.
+ */
+size_t
+i915_ggtt_view_size(struct drm_i915_gem_object *obj,
+		    const struct i915_ggtt_view *view)
+{
+	BUG_ON(!view);
+
+	if (view->type == I915_GGTT_VIEW_NORMAL ||
+	    view->type == I915_GGTT_VIEW_ROTATED) {
+		return obj->base.size;
+	} else {
+		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
+		return obj->base.size;
+	}
+}
+
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 4e6cac5..34b7cca 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -498,4 +498,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
 	return a->type == b->type;
 }
 
+size_t
+i915_ggtt_view_size(struct drm_i915_gem_object *obj,
+		    const struct i915_ggtt_view *view);
+
 #endif
-- 
1.7.9.5



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

[PATCH 3/5] drm/i915: Consider object pinned if any VMA is pinned

[Joonas Lahtinen]

Do not skip special GGTT views when considering whether an object
is pinned or not.

Wrong behaviour was introduced in;

commit ec7adb6ee79c8c9fe64d63ad638a31cd62e55515
Author: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Date:   Mon Mar 16 14:11:13 2015 +0200

    drm/i915: Do not use ggtt_view with (aliasing) PPGTT

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 111ac8a..c2c1819 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5256,13 +5256,10 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
 {
 	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		if (i915_is_ggtt(vma->vm) &&
-		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
-			continue;
+	list_for_each_entry(vma, &obj->vma_list, vma_link)
 		if (vma->pin_count > 0)
 			return true;
-	}
+
 	return false;
 }
 
-- 
1.7.9.5



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

[PATCH 4/5] drm/i915: Add a partial GGTT view type

[Joonas Lahtinen]

Partial view type allows manipulating parts of huge BOs through the GGTT,
which was not previously possible due to constraint that whole object had
to be mapped for any access to it through GGTT.

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |   46 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h |   15 ++++++++++--
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5babbd3..5937d3d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2764,6 +2764,47 @@ err_st_alloc:
 	return ERR_PTR(ret);
 }
 
+static struct sg_table *
+intel_partial_pages(const struct i915_ggtt_view *view,
+		    struct drm_i915_gem_object *obj)
+{
+	struct sg_table *st;
+	struct scatterlist *sg;
+	struct sg_page_iter obj_sg_iter;
+	int ret;
+
+	st = kmalloc(sizeof(*st), GFP_KERNEL);
+	if (!st)
+		goto err_st_alloc;
+
+	ret = sg_alloc_table(st, view->params.partial.size, GFP_KERNEL);
+	if (ret)
+		goto err_sg_alloc;
+
+	sg = st->sgl;
+	st->nents = 0;
+	for_each_sg_page(obj->pages->sgl, &obj_sg_iter, obj->pages->nents,
+		view->params.partial.offset)
+	{
+		if (st->nents >= view->params.partial.size)
+			break;
+
+		sg_set_page(sg, NULL, PAGE_SIZE, 0);
+		sg_dma_address(sg) = sg_page_iter_dma_address(&obj_sg_iter);
+		sg_dma_len(sg) = PAGE_SIZE;
+
+		sg = sg_next(sg);
+		st->nents++;
+	}
+
+	return st;
+
+err_sg_alloc:
+	kfree(st);
+err_st_alloc:
+	return ERR_PTR(-ENOMEM);
+}
+
 static int
 i915_get_ggtt_vma_pages(struct i915_vma *vma)
 {
@@ -2777,6 +2818,9 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 	else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
 		vma->ggtt_view.pages =
 			intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
+	else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL)
+		vma->ggtt_view.pages =
+			intel_partial_pages(&vma->ggtt_view, vma->obj);
 	else
 		WARN_ONCE(1, "GGTT view %u not implemented!\n",
 			  vma->ggtt_view.type);
@@ -2859,6 +2903,8 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 	if (view->type == I915_GGTT_VIEW_NORMAL ||
 	    view->type == I915_GGTT_VIEW_ROTATED) {
 		return obj->base.size;
+	} else if (view->type == I915_GGTT_VIEW_PARTIAL) {
+		return view->params.partial.size << PAGE_SHIFT;
 	} else {
 		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
 		return obj->base.size;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 34b7cca..ab1ad8a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -117,7 +117,8 @@ typedef uint64_t gen8_pde_t;
 
 enum i915_ggtt_view_type {
 	I915_GGTT_VIEW_NORMAL = 0,
-	I915_GGTT_VIEW_ROTATED
+	I915_GGTT_VIEW_ROTATED,
+	I915_GGTT_VIEW_PARTIAL,
 };
 
 struct intel_rotation_info {
@@ -130,6 +131,13 @@ struct intel_rotation_info {
 struct i915_ggtt_view {
 	enum i915_ggtt_view_type type;
 
+	union {
+		struct {
+			pgoff_t offset;
+			size_t size;
+		} partial;
+	} params;
+
 	struct sg_table *pages;
 
 	union {
@@ -495,7 +503,10 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
 	if (WARN_ON(!a || !b))
 		return false;
 
-	return a->type == b->type;
+	if (a->type != b->type)
+		return false;
+
+	return !memcmp(&a->params, &b->params, sizeof(a->params));
 }
 
 size_t
-- 
1.7.9.5



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

[PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Joonas Lahtinen]

Use partial view for huge BOs (bigger than half the mappable aperture)
in fault handler so that they can be accessed withough trying to make
room for them by evicting other objects.

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c2c1819..eb30cee 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_ggtt_view view = i915_ggtt_view_normal;
 	pgoff_t page_offset;
 	unsigned long pfn;
 	int ret = 0;
@@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		goto unlock;
 	}
 
-	/* Now bind it into the GTT if needed */
-	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+	/* Use a partial view if the object is bigger than half the aperture. */
+	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
+		static const size_t chunk_size = 256; // 1 MiB
+		memset(&view, 0, sizeof(view));
+		view.type = I915_GGTT_VIEW_PARTIAL;
+		view.params.partial.offset = rounddown(page_offset, chunk_size);
+		view.params.partial.size =
+			min_t(size_t,
+			      chunk_size,
+			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
+			      view.params.partial.offset);
+	}
+
+	/* Now pin it into the GTT if needed */
+	ret = i915_gem_object_ggtt_pin(obj, &view, 0, PIN_MAPPABLE);
 	if (ret)
 		goto unlock;
 
@@ -1681,30 +1695,44 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		goto unpin;
 
 	/* Finally, remap it using the new GTT offset */
-	pfn = dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj);
+	pfn = dev_priv->gtt.mappable_base +
+		i915_gem_obj_ggtt_offset_view(obj, &view);
 	pfn >>= PAGE_SHIFT;
 
-	if (!obj->fault_mappable) {
-		unsigned long size = min_t(unsigned long,
-					   vma->vm_end - vma->vm_start,
-					   obj->base.size);
+	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
+		unsigned long base = vma->vm_start +
+			(view.params.partial.offset << PAGE_SHIFT);
 		int i;
 
-		for (i = 0; i < size >> PAGE_SHIFT; i++) {
-			ret = vm_insert_pfn(vma,
-					    (unsigned long)vma->vm_start + i * PAGE_SIZE,
-					    pfn + i);
+		for (i = 0; i < view.params.partial.size; i++) {
+			ret = vm_insert_pfn(vma, base + i * PAGE_SIZE, pfn + i);
 			if (ret)
 				break;
 		}
-
 		obj->fault_mappable = true;
-	} else
-		ret = vm_insert_pfn(vma,
-				    (unsigned long)vmf->virtual_address,
-				    pfn + page_offset);
+	} else {
+		if (!obj->fault_mappable) {
+			unsigned long size = min_t(unsigned long,
+						   vma->vm_end - vma->vm_start,
+						   obj->base.size);
+			int i;
+
+			for (i = 0; i < size >> PAGE_SHIFT; i++) {
+				ret = vm_insert_pfn(vma,
+						    (unsigned long)vma->vm_start + i * PAGE_SIZE,
+						    pfn + i);
+				if (ret)
+					break;
+			}
+
+			obj->fault_mappable = true;
+		} else
+			ret = vm_insert_pfn(vma,
+					    (unsigned long)vmf->virtual_address,
+					    pfn + page_offset);
+	}
 unpin:
-	i915_gem_object_ggtt_unpin(obj);
+	i915_gem_object_ggtt_unpin_view(obj, &view);
 unlock:
 	mutex_unlock(&dev->struct_mutex);
 out:
@@ -1897,11 +1925,6 @@ i915_gem_mmap_gtt(struct drm_file *file,
 		goto unlock;
 	}
 
-	if (obj->base.size > dev_priv->gtt.mappable_end) {
-		ret = -E2BIG;
-		goto out;
-	}
-
 	if (obj->madv != I915_MADV_WILLNEED) {
 		DRM_DEBUG("Attempting to mmap a purgeable buffer\n");
 		ret = -EFAULT;
-- 
1.7.9.5



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

Re: [PATCH 3/5] drm/i915: Consider object pinned if any VMA is pinned

[Chris Wilson]

On Fri, Apr 24, 2015 at 03:09:39PM +0300, Joonas Lahtinen wrote:
> Do not skip special GGTT views when considering whether an object
> is pinned or not.
> 
> Wrong behaviour was introduced in;
> 
> commit ec7adb6ee79c8c9fe64d63ad638a31cd62e55515
> Author: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Date:   Mon Mar 16 14:11:13 2015 +0200
> 
>     drm/i915: Do not use ggtt_view with (aliasing) PPGTT
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

The function is obsolete. Please review the patches to remove the
callsite with more explicit vma management.
-Chris

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

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Chris Wilson]

On Fri, Apr 24, 2015 at 03:10:20PM +0300, Joonas Lahtinen wrote:
> Use partial view for huge BOs (bigger than half the mappable aperture)
> in fault handler so that they can be accessed withough trying to make
> room for them by evicting other objects.
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
>  1 file changed, 45 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c2c1819..eb30cee 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_ggtt_view view = i915_ggtt_view_normal;
>  	pgoff_t page_offset;
>  	unsigned long pfn;
>  	int ret = 0;
> @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  		goto unlock;
>  	}
>  
> -	/* Now bind it into the GTT if needed */
> -	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	/* Use a partial view if the object is bigger than half the aperture. */
> +	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
> +		static const size_t chunk_size = 256; // 1 MiB
> +		memset(&view, 0, sizeof(view));
> +		view.type = I915_GGTT_VIEW_PARTIAL;
> +		view.params.partial.offset = rounddown(page_offset, chunk_size);
> +		view.params.partial.size =
> +			min_t(size_t,
> +			      chunk_size,
> +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> +			      view.params.partial.offset);

This isn't what I was imagining.

I was expecting to see error handling inside the fault path if we could
not pin the object. This way we could handle fragmentation or display
objects pinned outside the aperture.
-Chris

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

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6260
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  318/318              318/318
IVB                                  341/341              341/341
BYT                                  287/287              287/287
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Joonas Lahtinen]

On pe, 2015-04-24 at 13:33 +0100, Chris Wilson wrote:
> On Fri, Apr 24, 2015 at 03:10:20PM +0300, Joonas Lahtinen wrote:
> > Use partial view for huge BOs (bigger than half the mappable aperture)
> > in fault handler so that they can be accessed withough trying to make
> > room for them by evicting other objects.
> > 
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
> >  1 file changed, 45 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index c2c1819..eb30cee 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> >  	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
> >  	struct drm_device *dev = obj->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct i915_ggtt_view view = i915_ggtt_view_normal;
> >  	pgoff_t page_offset;
> >  	unsigned long pfn;
> >  	int ret = 0;
> > @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> >  		goto unlock;
> >  	}
> >  
> > -	/* Now bind it into the GTT if needed */
> > -	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > +	/* Use a partial view if the object is bigger than half the aperture. */
> > +	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
> > +		static const size_t chunk_size = 256; // 1 MiB
> > +		memset(&view, 0, sizeof(view));
> > +		view.type = I915_GGTT_VIEW_PARTIAL;
> > +		view.params.partial.offset = rounddown(page_offset, chunk_size);
> > +		view.params.partial.size =
> > +			min_t(size_t,
> > +			      chunk_size,
> > +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> > +			      view.params.partial.offset);
> 
> This isn't what I was imagining.
> 
> I was expecting to see error handling inside the fault path if we could
> not pin the object. This way we could handle fragmentation or display
> objects pinned outside the aperture.

After discussion with Daniel, the idea was dropped due to high amount of
trashing which would occur if each object would be attempted to fit to
the mappable aperture for each fault to that object.

An alternate path could be added to mark an object partially_mappable if
mapping it normally fails, then further attempts could be skipped to
avoid the trashing. But it is still questionable if an object close to
the whole aperture size should be attempted to map normally, which will
cause huge trashing if there are plenty of objects in the mappable
aperture (that problem existed before, if the object was just short of
the aperture size, it was attempted to be mapped).

Regards, Joonas

PS. Sorry for late reply, it seems my original reply didn't get
delivered.

> -Chris
> 


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

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Chris Wilson]

On Mon, Apr 27, 2015 at 02:01:59PM +0300, Joonas Lahtinen wrote:
> On pe, 2015-04-24 at 13:33 +0100, Chris Wilson wrote:
> > On Fri, Apr 24, 2015 at 03:10:20PM +0300, Joonas Lahtinen wrote:
> > > Use partial view for huge BOs (bigger than half the mappable aperture)
> > > in fault handler so that they can be accessed withough trying to make
> > > room for them by evicting other objects.
> > > 
> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
> > >  1 file changed, 45 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index c2c1819..eb30cee 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > >  	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
> > >  	struct drm_device *dev = obj->base.dev;
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct i915_ggtt_view view = i915_ggtt_view_normal;
> > >  	pgoff_t page_offset;
> > >  	unsigned long pfn;
> > >  	int ret = 0;
> > > @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > >  		goto unlock;
> > >  	}
> > >  
> > > -	/* Now bind it into the GTT if needed */
> > > -	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > > +	/* Use a partial view if the object is bigger than half the aperture. */
> > > +	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
> > > +		static const size_t chunk_size = 256; // 1 MiB
> > > +		memset(&view, 0, sizeof(view));
> > > +		view.type = I915_GGTT_VIEW_PARTIAL;
> > > +		view.params.partial.offset = rounddown(page_offset, chunk_size);
> > > +		view.params.partial.size =
> > > +			min_t(size_t,
> > > +			      chunk_size,
> > > +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> > > +			      view.params.partial.offset);
> > 
> > This isn't what I was imagining.
> > 
> > I was expecting to see error handling inside the fault path if we could
> > not pin the object. This way we could handle fragmentation or display
> > objects pinned outside the aperture.
> 
> After discussion with Daniel, the idea was dropped due to high amount of
> trashing which would occur if each object would be attempted to fit to
> the mappable aperture for each fault to that object.

The point is that we fail to install a partial view for pinned objects
outside of the aperture. Or did I miss how you handle them?

I would suggest try first with a PIN_MAPPABLE | PIN_NOEVICT then. You
can limit thrashing by a vma->release callback to only zap the PTE
mapping into that vma. Also remember to destroy all of the partial vma
when we zap the mapping for an object.

> An alternate path could be added to mark an object partially_mappable if
> mapping it normally fails, then further attempts could be skipped to
> avoid the trashing. But it is still questionable if an object close to
> the whole aperture size should be attempted to map normally, which will
> cause huge trashing if there are plenty of objects in the mappable
> aperture (that problem existed before, if the object was just short of
> the aperture size, it was attempted to be mapped).

Just treat the aperture as first come first served, with perhaps a
reservation of the first 256k for transient partial vma. Fix thrashing
when we a usecase that falls foul of the page-fault-of-doom, but first
try to write one. I think with a mru list of individually faulted vma
thrashing will be limited (i.e. no worse than current mru eviction and
possibly better albeit at a higher pagefault cost).

Also I found a usecase for temporarily mapping invidual pages:
http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=bug90137&id=40e8a9cddb4a1e39a2fdcff4878b61c666e1a51e
-Chris

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

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Joonas Lahtinen]

On ma, 2015-04-27 at 12:21 +0100, Chris Wilson wrote:
> On Mon, Apr 27, 2015 at 02:01:59PM +0300, Joonas Lahtinen wrote:
> > On pe, 2015-04-24 at 13:33 +0100, Chris Wilson wrote:
> > > On Fri, Apr 24, 2015 at 03:10:20PM +0300, Joonas Lahtinen wrote:
> > > > Use partial view for huge BOs (bigger than half the mappable aperture)
> > > > in fault handler so that they can be accessed withough trying to make
> > > > room for them by evicting other objects.
> > > > 
> > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
> > > >  1 file changed, 45 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index c2c1819..eb30cee 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > >  	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
> > > >  	struct drm_device *dev = obj->base.dev;
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	struct i915_ggtt_view view = i915_ggtt_view_normal;
> > > >  	pgoff_t page_offset;
> > > >  	unsigned long pfn;
> > > >  	int ret = 0;
> > > > @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > >  		goto unlock;
> > > >  	}
> > > >  
> > > > -	/* Now bind it into the GTT if needed */
> > > > -	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > > > +	/* Use a partial view if the object is bigger than half the aperture. */
> > > > +	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
> > > > +		static const size_t chunk_size = 256; // 1 MiB
> > > > +		memset(&view, 0, sizeof(view));
> > > > +		view.type = I915_GGTT_VIEW_PARTIAL;
> > > > +		view.params.partial.offset = rounddown(page_offset, chunk_size);
> > > > +		view.params.partial.size =
> > > > +			min_t(size_t,
> > > > +			      chunk_size,
> > > > +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> > > > +			      view.params.partial.offset);
> > > 
> > > This isn't what I was imagining.
> > > 
> > > I was expecting to see error handling inside the fault path if we could
> > > not pin the object. This way we could handle fragmentation or display
> > > objects pinned outside the aperture.
> > 
> > After discussion with Daniel, the idea was dropped due to high amount of
> > trashing which would occur if each object would be attempted to fit to
> > the mappable aperture for each fault to that object.
> 
> The point is that we fail to install a partial view for pinned objects
> outside of the aperture. Or did I miss how you handle them?
> 

That is true.

By changing the comparison to be against full aperture size, then the
patch will only bring new functionality to handle the cases when the
object was actually impossible to map previously (and was early
rejected).

I'd prefer to have this version in first (with change of removing the /2
from aperture size comparison), to get some feedback from the XenGT team
about the usability of it (speed-wise).

I will extend the series with fence calculation for tield partial view,
so I would consider this in the rework. Does that sound good?

> I would suggest try first with a PIN_MAPPABLE | PIN_NOEVICT then. You
> can limit thrashing by a vma->release callback to only zap the PTE
> mapping into that vma. Also remember to destroy all of the partial vma
> when we zap the mapping for an object.
> 
> > An alternate path could be added to mark an object partially_mappable if
> > mapping it normally fails, then further attempts could be skipped to
> > avoid the trashing. But it is still questionable if an object close to
> > the whole aperture size should be attempted to map normally, which will
> > cause huge trashing if there are plenty of objects in the mappable
> > aperture (that problem existed before, if the object was just short of
> > the aperture size, it was attempted to be mapped).
> 
> Just treat the aperture as first come first served, with perhaps a
> reservation of the first 256k for transient partial vma. Fix thrashing
> when we a usecase that falls foul of the page-fault-of-doom, but first
> try to write one. I think with a mru list of individually faulted vma
> thrashing will be limited (i.e. no worse than current mru eviction and
> possibly better albeit at a higher pagefault cost).
> 

Ack.

> Also I found a usecase for temporarily mapping invidual pages:
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=bug90137&id=40e8a9cddb4a1e39a2fdcff4878b61c666e1a51e

I have not peeked into execbuffers much, but looks quite
straightforward. Could use the partial views code if desired. Not sure
how effective it is for single pages.

Regards, Joonas

> -Chris
> 


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

Re: [PATCH 3/5] drm/i915: Consider object pinned if any VMA is pinned

[Joonas Lahtinen]

On pe, 2015-04-24 at 13:29 +0100, Chris Wilson wrote:
> On Fri, Apr 24, 2015 at 03:09:39PM +0300, Joonas Lahtinen wrote:
> > Do not skip special GGTT views when considering whether an object
> > is pinned or not.
> > 
> > Wrong behaviour was introduced in;
> > 
> > commit ec7adb6ee79c8c9fe64d63ad638a31cd62e55515
> > Author: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Date:   Mon Mar 16 14:11:13 2015 +0200
> > 
> >     drm/i915: Do not use ggtt_view with (aliasing) PPGTT
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> The function is obsolete. Please review the patches to remove the
> callsite with more explicit vma management.

I do not call it myself at all, and I'm hesitant to change all the code
calling function just because I slipped code to function where it didn't
belong and now needs to be removed :P

I would leave this to the cleanup phase where all VMA related code was
agreed to be moved to its own file. I put it to my TODO list as it's not
related to the partial view series. Just to get this series out ASAP.

Regards, Joonas

> -Chris
> 


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

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Chris Wilson]

On Mon, Apr 27, 2015 at 03:12:01PM +0300, Joonas Lahtinen wrote:
> On ma, 2015-04-27 at 12:21 +0100, Chris Wilson wrote:
> > On Mon, Apr 27, 2015 at 02:01:59PM +0300, Joonas Lahtinen wrote:
> > > On pe, 2015-04-24 at 13:33 +0100, Chris Wilson wrote:
> > > > On Fri, Apr 24, 2015 at 03:10:20PM +0300, Joonas Lahtinen wrote:
> > > > > Use partial view for huge BOs (bigger than half the mappable aperture)
> > > > > in fault handler so that they can be accessed withough trying to make
> > > > > room for them by evicting other objects.
> > > > > 
> > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
> > > > >  1 file changed, 45 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > > index c2c1819..eb30cee 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > > >  	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
> > > > >  	struct drm_device *dev = obj->base.dev;
> > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > +	struct i915_ggtt_view view = i915_ggtt_view_normal;
> > > > >  	pgoff_t page_offset;
> > > > >  	unsigned long pfn;
> > > > >  	int ret = 0;
> > > > > @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > > >  		goto unlock;
> > > > >  	}
> > > > >  
> > > > > -	/* Now bind it into the GTT if needed */
> > > > > -	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > > > > +	/* Use a partial view if the object is bigger than half the aperture. */
> > > > > +	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
> > > > > +		static const size_t chunk_size = 256; // 1 MiB
> > > > > +		memset(&view, 0, sizeof(view));
> > > > > +		view.type = I915_GGTT_VIEW_PARTIAL;
> > > > > +		view.params.partial.offset = rounddown(page_offset, chunk_size);
> > > > > +		view.params.partial.size =
> > > > > +			min_t(size_t,
> > > > > +			      chunk_size,
> > > > > +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> > > > > +			      view.params.partial.offset);
> > > > 
> > > > This isn't what I was imagining.
> > > > 
> > > > I was expecting to see error handling inside the fault path if we could
> > > > not pin the object. This way we could handle fragmentation or display
> > > > objects pinned outside the aperture.
> > > 
> > > After discussion with Daniel, the idea was dropped due to high amount of
> > > trashing which would occur if each object would be attempted to fit to
> > > the mappable aperture for each fault to that object.
> > 
> > The point is that we fail to install a partial view for pinned objects
> > outside of the aperture. Or did I miss how you handle them?
> > 
> 
> That is true.
> 
> By changing the comparison to be against full aperture size, then the
> patch will only bring new functionality to handle the cases when the
> object was actually impossible to map previously (and was early
> rejected).
> 
> I'd prefer to have this version in first (with change of removing the /2
> from aperture size comparison), to get some feedback from the XenGT team
> about the usability of it (speed-wise).

No. Daniel rejected a change just because we didn't have this series,
only to find this series doesn't even provide the support Daniel
wanted...

I don't see this as a useful stepping point. The current users of large
objects do not map through the GTT.
-Chris

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

Re: [PATCH 1/5] drm/i915: Do not clear mappings beyond VMA size

[Tvrtko Ursulin]

On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
> Do not to clear mappings outside the allocated VMA under any
> circumstances. Only clear the smaller of VMA or object page count.
>
> This is required to allow creating partial object VMAs which in
> turn are needed for partial GGTT views.
>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c |    8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6fae6bd..ee9ff8e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1947,19 +1947,23 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
>   	struct drm_device *dev = vma->vm->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct drm_i915_gem_object *obj = vma->obj;
> +	const uint64_t size = min_t(uint64_t,
> +				    obj->base.size,
> +				    vma->node.size);
>
>   	if (vma->bound & GLOBAL_BIND) {
>   		vma->vm->clear_range(vma->vm,
>   				     vma->node.start,
> -				     obj->base.size,
> +				     size,
>   				     true);
>   	}
>
>   	if (dev_priv->mm.aliasing_ppgtt && vma->bound & LOCAL_BIND) {
>   		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> +
>   		appgtt->base.clear_range(&appgtt->base,
>   					 vma->node.start,
> -					 obj->base.size,
> +					 size,
>   					 true);
>   	}
>   }
>

Looks good to me,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Joonas Lahtinen]

On ma, 2015-04-27 at 13:25 +0100, Chris Wilson wrote:
> On Mon, Apr 27, 2015 at 03:12:01PM +0300, Joonas Lahtinen wrote:
> > On ma, 2015-04-27 at 12:21 +0100, Chris Wilson wrote:
> > > On Mon, Apr 27, 2015 at 02:01:59PM +0300, Joonas Lahtinen wrote:
> > > > On pe, 2015-04-24 at 13:33 +0100, Chris Wilson wrote:
> > > > > On Fri, Apr 24, 2015 at 03:10:20PM +0300, Joonas Lahtinen wrote:
> > > > > > Use partial view for huge BOs (bigger than half the mappable aperture)
> > > > > > in fault handler so that they can be accessed withough trying to make
> > > > > > room for them by evicting other objects.
> > > > > > 
> > > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
> > > > > >  1 file changed, 45 insertions(+), 22 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > index c2c1819..eb30cee 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > > > >  	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
> > > > > >  	struct drm_device *dev = obj->base.dev;
> > > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > +	struct i915_ggtt_view view = i915_ggtt_view_normal;
> > > > > >  	pgoff_t page_offset;
> > > > > >  	unsigned long pfn;
> > > > > >  	int ret = 0;
> > > > > > @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > > > >  		goto unlock;
> > > > > >  	}
> > > > > >  
> > > > > > -	/* Now bind it into the GTT if needed */
> > > > > > -	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > > > > > +	/* Use a partial view if the object is bigger than half the aperture. */
> > > > > > +	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
> > > > > > +		static const size_t chunk_size = 256; // 1 MiB
> > > > > > +		memset(&view, 0, sizeof(view));
> > > > > > +		view.type = I915_GGTT_VIEW_PARTIAL;
> > > > > > +		view.params.partial.offset = rounddown(page_offset, chunk_size);
> > > > > > +		view.params.partial.size =
> > > > > > +			min_t(size_t,
> > > > > > +			      chunk_size,
> > > > > > +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> > > > > > +			      view.params.partial.offset);
> > > > > 
> > > > > This isn't what I was imagining.
> > > > > 
> > > > > I was expecting to see error handling inside the fault path if we could
> > > > > not pin the object. This way we could handle fragmentation or display
> > > > > objects pinned outside the aperture.
> > > > 
> > > > After discussion with Daniel, the idea was dropped due to high amount of
> > > > trashing which would occur if each object would be attempted to fit to
> > > > the mappable aperture for each fault to that object.
> > > 
> > > The point is that we fail to install a partial view for pinned objects
> > > outside of the aperture. Or did I miss how you handle them?
> > > 
> > 
> > That is true.
> > 
> > By changing the comparison to be against full aperture size, then the
> > patch will only bring new functionality to handle the cases when the
> > object was actually impossible to map previously (and was early
> > rejected).
> > 
> > I'd prefer to have this version in first (with change of removing the /2
> > from aperture size comparison), to get some feedback from the XenGT team
> > about the usability of it (speed-wise).
> 
> No. Daniel rejected a change just because we didn't have this series,
> only to find this series doesn't even provide the support Daniel
> wanted...
> 

Which change is that? If we're talking about the back-up path for failed
normal mapping, I can of course add the code for it, it will not cause
any worse trashing than it has before for objects smaller than aperture
size. Fancier logic to avoid trashing can be added later.

> I don't see this as a useful stepping point. The current users of large
> objects do not map through the GTT.

In XenGT, when the mappable aperture size is decreased due to slicing of
the aperture for different guests, it's not about large objects but
small aperture. And that is the reason why the feature was initially
implemented, and they've already been waiting for the feature for quite
a while.

All the series was intended to was to provide this support, so they can
proceed with testing with small aperture sizes. Everything else is
considered extra, and it was in the initial description of the task (by
either Jesse or Daniel) that half aperture size is most likely when the
current code will start to fail in real use cases.

I do not see why all possible features need to be implemented first to
get a useful feature for the XenGT team in. After all, before adding the
partial views, anything bigger than the mappable aperture was simply
rejected in mmap. So I do not follow the logic why the code path for
this case can not be added.

If some other change was not merged due to waiting for the partial views
support with more features than the code path for objects bigger than
aperture size, not merging the current code only causes two features to
keep waiting instead of just one. And as the XenGT team has been waiting
for the feature for months before the feature was even started on, I do
think it is not very reasonable.

The next revision will be started on immediately after this first one
has been merged, and intention is to keep this first one simple. Let me
know whatever feature is awaiting for the next revision, and I can take
it into consideration to have the support for that use case ready as
first priority.

Regards, Joonas

> -Chris
> 


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

Re: [PATCH 2/5] drm/i915: Do not make assumptions on GGTT VMA sizes

[Tvrtko Ursulin]

Hi,

On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
>
> GGTT VMA sizes might be smaller than the whole object size due to
> different GGTT views.
>
> v2:
> - Separate GGTT view constraint calculations from normal view
>    constraint calculations (Chris Wilson)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c     |  106 ++++++++++++++++++++++-------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c |   23 ++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |    4 ++
>   3 files changed, 95 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e8f6f4c..111ac8a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3497,7 +3497,8 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
>   }
>
>   /**
> - * Finds free space in the GTT aperture and binds the object there.
> + * Finds free space in the GTT aperture and binds the object or a view of it
> + * there.
>    */
>   static struct i915_vma *
>   i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> @@ -3513,39 +3514,66 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   		flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
>   	unsigned long end =
>   		flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
> +	const bool is_object = !ggtt_view ||
> +			       ggtt_view->type == I915_GGTT_VIEW_NORMAL;
>   	struct i915_vma *vma;
>   	int ret;
>
> -	if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> -		return ERR_PTR(-EINVAL);
> +	if (i915_is_ggtt(vm)) {
> +		u32 view_size;
> +
> +		if (WARN_ON(!ggtt_view))
> +			return ERR_PTR(-EINVAL);
>
> -	fence_size = i915_gem_get_gtt_size(dev,
> -					   obj->base.size,
> -					   obj->tiling_mode);
> -	fence_alignment = i915_gem_get_gtt_alignment(dev,
> -						     obj->base.size,
> -						     obj->tiling_mode, true);
> -	unfenced_alignment =
> -		i915_gem_get_gtt_alignment(dev,
> -					   obj->base.size,
> -					   obj->tiling_mode, false);
> +		view_size = i915_ggtt_view_size(obj, ggtt_view);
> +
> +		fence_size = i915_gem_get_gtt_size(dev,
> +						   view_size,
> +						   obj->tiling_mode);
> +		fence_alignment = i915_gem_get_gtt_alignment(dev,
> +							     view_size,
> +							     obj->tiling_mode,
> +							     true);
> +		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
> +								view_size,
> +								obj->tiling_mode,
> +								false);
> +		size = flags & PIN_MAPPABLE ? fence_size : view_size;
> +	} else {
> +		fence_size = i915_gem_get_gtt_size(dev,
> +						   obj->base.size,
> +						   obj->tiling_mode);
> +		fence_alignment = i915_gem_get_gtt_alignment(dev,
> +							     obj->base.size,
> +							     obj->tiling_mode,
> +							     true);
> +		unfenced_alignment =
> +			i915_gem_get_gtt_alignment(dev,
> +						   obj->base.size,
> +						   obj->tiling_mode,
> +						   false);
> +		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> +	}

Looks like you could avoid duplicating the if/else below if you did 
something like:

view_size = ggtt_view ? i915_ggtt_view_size(...) : obj->base.size;

And then have only one instance of:

fenced_size = ... with view_size for size, etc..
unfenced_size..
*alignment = ...
size = ...

>
>   	if (alignment == 0)
>   		alignment = flags & PIN_MAPPABLE ? fence_alignment :
>   						unfenced_alignment;
>   	if (flags & PIN_MAPPABLE && alignment & (fence_alignment - 1)) {
> -		DRM_DEBUG("Invalid object alignment requested %u\n", alignment);
> +		DRM_DEBUG("Invalid %s alignment requested %u\n",
> +			  is_object ? "object" : "GGTT view",
> +			  alignment);
>   		return ERR_PTR(-EINVAL);
>   	}
>
> -	size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> -
> -	/* If the object is bigger than the entire aperture, reject it early
> -	 * before evicting everything in a vain attempt to find space.
> +	/* If binding the object/GGTT view requires more space than the entire
> +	 * aperture has, reject it early before evicting everything in a vain
> +	 * attempt to find space.
>   	 */
> -	if (obj->base.size > end) {
> -		DRM_DEBUG("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%lu\n",
> -			  obj->base.size,
> +	if (size > end) {
> +		DRM_DEBUG("Attempting to bind %s (view type=%u) larger than the aperture: size=%u > %s aperture=%lu\n",
> +			  is_object ? "an object" : "a GGTT view ",
> +			  is_object ? 0 : ggtt_view->type,
> +			  size,

I am not sure if this is_object thing here and above is worth it. Maybe 
just add ggtt_view->type? Should carry the same amount of info for 
debugging?

>   			  flags & PIN_MAPPABLE ? "mappable" : "total",
>   			  end);
>   		return ERR_PTR(-E2BIG);
> @@ -4207,28 +4235,30 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>   			return ret;
>   	}
>
> -	if ((bound ^ vma->bound) & GLOBAL_BIND) {
> -		bool mappable, fenceable;
> -		u32 fence_size, fence_alignment;
> +	if (!ggtt_view || ggtt_view->type == I915_GGTT_VIEW_NORMAL) {

Why this condition, and why "!ggtt_view" - that means PPGTT, no? GGTT 
and ggtt_view == NULL is not allowed by the WARN_ON earlier in this 
function.

> +		if ((bound ^ vma->bound) & GLOBAL_BIND) {
> +			bool mappable, fenceable;
> +			u32 fence_size, fence_alignment;
>
> -		fence_size = i915_gem_get_gtt_size(obj->base.dev,
> -						   obj->base.size,
> -						   obj->tiling_mode);
> -		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> -							     obj->base.size,
> -							     obj->tiling_mode,
> -							     true);
> +			fence_size = i915_gem_get_gtt_size(obj->base.dev,
> +							   obj->base.size,
> +							   obj->tiling_mode);
> +			fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> +								     obj->base.size,
> +								     obj->tiling_mode,
> +								     true);
>
> -		fenceable = (vma->node.size == fence_size &&
> -			     (vma->node.start & (fence_alignment - 1)) == 0);
> +			fenceable = (vma->node.size == fence_size &&
> +				     (vma->node.start & (fence_alignment - 1)) == 0);
>
> -		mappable = (vma->node.start + fence_size <=
> -			    dev_priv->gtt.mappable_end);
> +			mappable = (vma->node.start + fence_size <=
> +				    dev_priv->gtt.mappable_end);
>
> -		obj->map_and_fenceable = mappable && fenceable;
> -	}
> +			obj->map_and_fenceable = mappable && fenceable;
> +		}
>
> -	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> +		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> +	}
>
>   	vma->pin_count++;
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ee9ff8e..5babbd3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2842,3 +2842,26 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>
>   	return 0;
>   }
> +
> +/**
> + * i915_ggtt_view_size - Get the size of a GGTT view.
> + * @obj: Object the view is of.
> + * @view: The view in question.
> + *
> + * @return The size of the GGTT view in bytes.
> + */
> +size_t
> +i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> +		    const struct i915_ggtt_view *view)
> +{
> +	BUG_ON(!view);

WARN_ON and return object size maybe?

Once I mentioned, if we really wanted to stop the driver completely in 
some cases, should add something like I915_BUG_ON() and some flags at 
strategic places. Maybe it would be possible.

> +
> +	if (view->type == I915_GGTT_VIEW_NORMAL ||
> +	    view->type == I915_GGTT_VIEW_ROTATED) {
> +		return obj->base.size;
> +	} else {
> +		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
> +		return obj->base.size;
> +	}
> +}
> +
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 4e6cac5..34b7cca 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -498,4 +498,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
>   	return a->type == b->type;
>   }
>
> +size_t
> +i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> +		    const struct i915_ggtt_view *view);
> +
>   #endif
>

Regards,

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

Re: [PATCH 4/5] drm/i915: Add a partial GGTT view type

[Tvrtko Ursulin]

Hi,

On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
>
> Partial view type allows manipulating parts of huge BOs through the GGTT,
> which was not previously possible due to constraint that whole object had
> to be mapped for any access to it through GGTT.
>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c |   46 +++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |   15 ++++++++++--
>   2 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 5babbd3..5937d3d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2764,6 +2764,47 @@ err_st_alloc:
>   	return ERR_PTR(ret);
>   }
>
> +static struct sg_table *
> +intel_partial_pages(const struct i915_ggtt_view *view,
> +		    struct drm_i915_gem_object *obj)
> +{
> +	struct sg_table *st;
> +	struct scatterlist *sg;
> +	struct sg_page_iter obj_sg_iter;
> +	int ret;
> +
> +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		goto err_st_alloc;
> +
> +	ret = sg_alloc_table(st, view->params.partial.size, GFP_KERNEL);
> +	if (ret)
> +		goto err_sg_alloc;
> +
> +	sg = st->sgl;
> +	st->nents = 0;

sg_alloc_table configures the sg_table so not needed I think. Although I 
do see I am also doing it. :)

> +	for_each_sg_page(obj->pages->sgl, &obj_sg_iter, obj->pages->nents,
> +		view->params.partial.offset)
> +	{
> +		if (st->nents >= view->params.partial.size)
> +			break;
> +
> +		sg_set_page(sg, NULL, PAGE_SIZE, 0);
> +		sg_dma_address(sg) = sg_page_iter_dma_address(&obj_sg_iter);
> +		sg_dma_len(sg) = PAGE_SIZE;
> +
> +		sg = sg_next(sg);
> +		st->nents++;
> +	}

I suppose in this case (as opposed to rotated view) using 
sg_alloc_table_from_pages() could produce a more compact table. With the 
caveat of that it doesn't always work (see i915_gem_userptr.c/st_set_pages).

So maybe promote to driver public st_set_pages and call in on an array 
of pages?

> +
> +	return st;
> +
> +err_sg_alloc:
> +	kfree(st);

Here you lose ret from sg_alloc_table.

> +err_st_alloc:
> +	return ERR_PTR(-ENOMEM);
> +}
> +
>   static int
>   i915_get_ggtt_vma_pages(struct i915_vma *vma)
>   {
> @@ -2777,6 +2818,9 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>   	else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
>   		vma->ggtt_view.pages =
>   			intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
> +	else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL)
> +		vma->ggtt_view.pages =
> +			intel_partial_pages(&vma->ggtt_view, vma->obj);
>   	else
>   		WARN_ONCE(1, "GGTT view %u not implemented!\n",
>   			  vma->ggtt_view.type);
> @@ -2859,6 +2903,8 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   	if (view->type == I915_GGTT_VIEW_NORMAL ||
>   	    view->type == I915_GGTT_VIEW_ROTATED) {
>   		return obj->base.size;
> +	} else if (view->type == I915_GGTT_VIEW_PARTIAL) {
> +		return view->params.partial.size << PAGE_SHIFT;
>   	} else {
>   		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
>   		return obj->base.size;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 34b7cca..ab1ad8a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -117,7 +117,8 @@ typedef uint64_t gen8_pde_t;
>
>   enum i915_ggtt_view_type {
>   	I915_GGTT_VIEW_NORMAL = 0,
> -	I915_GGTT_VIEW_ROTATED
> +	I915_GGTT_VIEW_ROTATED,
> +	I915_GGTT_VIEW_PARTIAL,
>   };
>
>   struct intel_rotation_info {
> @@ -130,6 +131,13 @@ struct intel_rotation_info {
>   struct i915_ggtt_view {
>   	enum i915_ggtt_view_type type;
>
> +	union {
> +		struct {
> +			pgoff_t offset;
> +			size_t size;

Size is in pages right? Maybe it would be more self-documenting to use 
some basic type like unsigned int or long since size_t, to me at least, 
suggests bytes.

> +		} partial;
> +	} params;
> +
>   	struct sg_table *pages;
>
>   	union {
> @@ -495,7 +503,10 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
>   	if (WARN_ON(!a || !b))
>   		return false;
>
> -	return a->type == b->type;
> +	if (a->type != b->type)
> +		return false;
> +
> +	return !memcmp(&a->params, &b->params, sizeof(a->params));

So for rotated views it would still do memcmp. OK structure is zeroed on 
alloc, but it is pointless to do so.

Regards,

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

Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Chris Wilson]

On Mon, Apr 27, 2015 at 04:46:20PM +0300, Joonas Lahtinen wrote:
> In XenGT, when the mappable aperture size is decreased due to slicing of
> the aperture for different guests, it's not about large objects but
> small aperture. And that is the reason why the feature was initially
> implemented, and they've already been waiting for the feature for quite
> a while.

Sorry, but the quickest fix for that would have been to add the aperture
information to GET_APERTURE, like I have previously said.
-Chris

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

Re: [PATCH 2/5] drm/i915: Do not make assumptions on GGTT VMA sizes

[Joonas Lahtinen]

On ma, 2015-04-27 at 14:55 +0100, Tvrtko Ursulin wrote:
> Hi,
> 
> On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
> >
> > GGTT VMA sizes might be smaller than the whole object size due to
> > different GGTT views.
> >
> > v2:
> > - Separate GGTT view constraint calculations from normal view
> >    constraint calculations (Chris Wilson)
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c     |  106 ++++++++++++++++++++++-------------
> >   drivers/gpu/drm/i915/i915_gem_gtt.c |   23 ++++++++
> >   drivers/gpu/drm/i915/i915_gem_gtt.h |    4 ++
> >   3 files changed, 95 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index e8f6f4c..111ac8a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3497,7 +3497,8 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
> >   }
> >
> >   /**
> > - * Finds free space in the GTT aperture and binds the object there.
> > + * Finds free space in the GTT aperture and binds the object or a view of it
> > + * there.
> >    */
> >   static struct i915_vma *
> >   i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> > @@ -3513,39 +3514,66 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >   		flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
> >   	unsigned long end =
> >   		flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
> > +	const bool is_object = !ggtt_view ||
> > +			       ggtt_view->type == I915_GGTT_VIEW_NORMAL;
> >   	struct i915_vma *vma;
> >   	int ret;
> >
> > -	if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> > -		return ERR_PTR(-EINVAL);
> > +	if (i915_is_ggtt(vm)) {
> > +		u32 view_size;
> > +
> > +		if (WARN_ON(!ggtt_view))
> > +			return ERR_PTR(-EINVAL);
> >
> > -	fence_size = i915_gem_get_gtt_size(dev,
> > -					   obj->base.size,
> > -					   obj->tiling_mode);
> > -	fence_alignment = i915_gem_get_gtt_alignment(dev,
> > -						     obj->base.size,
> > -						     obj->tiling_mode, true);
> > -	unfenced_alignment =
> > -		i915_gem_get_gtt_alignment(dev,
> > -					   obj->base.size,
> > -					   obj->tiling_mode, false);
> > +		view_size = i915_ggtt_view_size(obj, ggtt_view);
> > +
> > +		fence_size = i915_gem_get_gtt_size(dev,
> > +						   view_size,
> > +						   obj->tiling_mode);
> > +		fence_alignment = i915_gem_get_gtt_alignment(dev,
> > +							     view_size,
> > +							     obj->tiling_mode,
> > +							     true);
> > +		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
> > +								view_size,
> > +								obj->tiling_mode,
> > +								false);
> > +		size = flags & PIN_MAPPABLE ? fence_size : view_size;
> > +	} else {
> > +		fence_size = i915_gem_get_gtt_size(dev,
> > +						   obj->base.size,
> > +						   obj->tiling_mode);
> > +		fence_alignment = i915_gem_get_gtt_alignment(dev,
> > +							     obj->base.size,
> > +							     obj->tiling_mode,
> > +							     true);
> > +		unfenced_alignment =
> > +			i915_gem_get_gtt_alignment(dev,
> > +						   obj->base.size,
> > +						   obj->tiling_mode,
> > +						   false);
> > +		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> > +	}
> 
> Looks like you could avoid duplicating the if/else below if you did 
> something like:
> 
> view_size = ggtt_view ? i915_ggtt_view_size(...) : obj->base.size;
> 
> And then have only one instance of:
> 
> fenced_size = ... with view_size for size, etc..
> unfenced_size..
> *alignment = ...
> size = ...
> 

It was originally like that, this v2 moved it to its own branch because
Chris informed me that there is work going on which adds more
constraints, and that optimizing common code should only be done after
the dust has settled.

> >
> >   	if (alignment == 0)
> >   		alignment = flags & PIN_MAPPABLE ? fence_alignment :
> >   						unfenced_alignment;
> >   	if (flags & PIN_MAPPABLE && alignment & (fence_alignment - 1)) {
> > -		DRM_DEBUG("Invalid object alignment requested %u\n", alignment);
> > +		DRM_DEBUG("Invalid %s alignment requested %u\n",
> > +			  is_object ? "object" : "GGTT view",
> > +			  alignment);
> >   		return ERR_PTR(-EINVAL);
> >   	}
> >
> > -	size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> > -
> > -	/* If the object is bigger than the entire aperture, reject it early
> > -	 * before evicting everything in a vain attempt to find space.
> > +	/* If binding the object/GGTT view requires more space than the entire
> > +	 * aperture has, reject it early before evicting everything in a vain
> > +	 * attempt to find space.
> >   	 */
> > -	if (obj->base.size > end) {
> > -		DRM_DEBUG("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%lu\n",
> > -			  obj->base.size,
> > +	if (size > end) {
> > +		DRM_DEBUG("Attempting to bind %s (view type=%u) larger than the aperture: size=%u > %s aperture=%lu\n",
> > +			  is_object ? "an object" : "a GGTT view ",
> > +			  is_object ? 0 : ggtt_view->type,
> > +			  size,
> 
> I am not sure if this is_object thing here and above is worth it. Maybe 
> just add ggtt_view->type? Should carry the same amount of info for 
> debugging?

True so.

> 
> >   			  flags & PIN_MAPPABLE ? "mappable" : "total",
> >   			  end);
> >   		return ERR_PTR(-E2BIG);
> > @@ -4207,28 +4235,30 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
> >   			return ret;
> >   	}
> >
> > -	if ((bound ^ vma->bound) & GLOBAL_BIND) {
> > -		bool mappable, fenceable;
> > -		u32 fence_size, fence_alignment;
> > +	if (!ggtt_view || ggtt_view->type == I915_GGTT_VIEW_NORMAL) {
> 
> Why this condition, and why "!ggtt_view" - that means PPGTT, no? GGTT 
> and ggtt_view == NULL is not allowed by the WARN_ON earlier in this 
> function.

This condition is there because the calculation applies to the whole
object flag 'map_and_fenceable'. And conditions when the whole object
are handled are when it's not in GGTT, or we're talking about normal
view in GGTT.

> 
> > +		if ((bound ^ vma->bound) & GLOBAL_BIND) {
> > +			bool mappable, fenceable;
> > +			u32 fence_size, fence_alignment;
> >
> > -		fence_size = i915_gem_get_gtt_size(obj->base.dev,
> > -						   obj->base.size,
> > -						   obj->tiling_mode);
> > -		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> > -							     obj->base.size,
> > -							     obj->tiling_mode,
> > -							     true);
> > +			fence_size = i915_gem_get_gtt_size(obj->base.dev,
> > +							   obj->base.size,
> > +							   obj->tiling_mode);
> > +			fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> > +								     obj->base.size,
> > +								     obj->tiling_mode,
> > +								     true);
> >
> > -		fenceable = (vma->node.size == fence_size &&
> > -			     (vma->node.start & (fence_alignment - 1)) == 0);
> > +			fenceable = (vma->node.size == fence_size &&
> > +				     (vma->node.start & (fence_alignment - 1)) == 0);
> >
> > -		mappable = (vma->node.start + fence_size <=
> > -			    dev_priv->gtt.mappable_end);
> > +			mappable = (vma->node.start + fence_size <=
> > +				    dev_priv->gtt.mappable_end);
> >
> > -		obj->map_and_fenceable = mappable && fenceable;
> > -	}
> > +			obj->map_and_fenceable = mappable && fenceable;
> > +		}
> >
> > -	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> > +		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> > +	}
> >
> >   	vma->pin_count++;
> >   	return 0;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index ee9ff8e..5babbd3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2842,3 +2842,26 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> >
> >   	return 0;
> >   }
> > +
> > +/**
> > + * i915_ggtt_view_size - Get the size of a GGTT view.
> > + * @obj: Object the view is of.
> > + * @view: The view in question.
> > + *
> > + * @return The size of the GGTT view in bytes.
> > + */
> > +size_t
> > +i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> > +		    const struct i915_ggtt_view *view)
> > +{
> > +	BUG_ON(!view);
> 
> WARN_ON and return object size maybe?
> 

There's no sense to call the this function with NULL pointer because it
is the main argument of the function, it'd OOPS in this case, so I
understood this would be the OK place to use it.

> Once I mentioned, if we really wanted to stop the driver completely in 
> some cases, should add something like I915_BUG_ON() and some flags at 
> strategic places. Maybe it would be possible.
> 
> > +
> > +	if (view->type == I915_GGTT_VIEW_NORMAL ||
> > +	    view->type == I915_GGTT_VIEW_ROTATED) {
> > +		return obj->base.size;
> > +	} else {
> > +		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
> > +		return obj->base.size;
> > +	}
> > +}
> > +
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index 4e6cac5..34b7cca 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -498,4 +498,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
> >   	return a->type == b->type;
> >   }
> >
> > +size_t
> > +i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> > +		    const struct i915_ggtt_view *view);
> > +
> >   #endif
> >
> 
> Regards,
> 
> Tvrtko


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

Re: [PATCH 4/5] drm/i915: Add a partial GGTT view type

[Tvrtko Ursulin]

On 04/27/2015 03:50 PM, Tvrtko Ursulin wrote:
>> +    for_each_sg_page(obj->pages->sgl, &obj_sg_iter, obj->pages->nents,
>> +        view->params.partial.offset)
>> +    {
>> +        if (st->nents >= view->params.partial.size)
>> +            break;
>> +
>> +        sg_set_page(sg, NULL, PAGE_SIZE, 0);
>> +        sg_dma_address(sg) = sg_page_iter_dma_address(&obj_sg_iter);
>> +        sg_dma_len(sg) = PAGE_SIZE;
>> +
>> +        sg = sg_next(sg);
>> +        st->nents++;
>> +    }
>
> I suppose in this case (as opposed to rotated view) using
> sg_alloc_table_from_pages() could produce a more compact table. With the
> caveat of that it doesn't always work (see
> i915_gem_userptr.c/st_set_pages).
>
> So maybe promote to driver public st_set_pages and call in on an array
> of pages?

Scratch this, on second thought it makes no sense.

Only if we had a smarter helper like sg_alloc_table_from_table_range() 
but no one cared about coalescing in the past.

Regards,

Tvrtko

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

Re: [PATCH 4/5] drm/i915: Add a partial GGTT view type

[Joonas Lahtinen]

On ma, 2015-04-27 at 15:50 +0100, Tvrtko Ursulin wrote:
> Hi,
> 
> On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
> >
> > Partial view type allows manipulating parts of huge BOs through the GGTT,
> > which was not previously possible due to constraint that whole object had
> > to be mapped for any access to it through GGTT.
> >
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_gtt.c |   46 +++++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/i915_gem_gtt.h |   15 ++++++++++--
> >   2 files changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 5babbd3..5937d3d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2764,6 +2764,47 @@ err_st_alloc:
> >   	return ERR_PTR(ret);
> >   }
> >
> > +static struct sg_table *
> > +intel_partial_pages(const struct i915_ggtt_view *view,
> > +		    struct drm_i915_gem_object *obj)
> > +{
> > +	struct sg_table *st;
> > +	struct scatterlist *sg;
> > +	struct sg_page_iter obj_sg_iter;
> > +	int ret;
> > +
> > +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> > +	if (!st)
> > +		goto err_st_alloc;
> > +
> > +	ret = sg_alloc_table(st, view->params.partial.size, GFP_KERNEL);
> > +	if (ret)
> > +		goto err_sg_alloc;
> > +
> > +	sg = st->sgl;
> > +	st->nents = 0;
> 
> sg_alloc_table configures the sg_table so not needed I think. Although I 
> do see I am also doing it. :)
> 

I initially stripped it w/r your code, but I was so desperate debugging
the for_each_sg_page interface I tried everything ;)

Removed it.

> > +	for_each_sg_page(obj->pages->sgl, &obj_sg_iter, obj->pages->nents,
> > +		view->params.partial.offset)
> > +	{
> > +		if (st->nents >= view->params.partial.size)
> > +			break;
> > +
> > +		sg_set_page(sg, NULL, PAGE_SIZE, 0);
> > +		sg_dma_address(sg) = sg_page_iter_dma_address(&obj_sg_iter);
> > +		sg_dma_len(sg) = PAGE_SIZE;
> > +
> > +		sg = sg_next(sg);
> > +		st->nents++;
> > +	}
> 
> I suppose in this case (as opposed to rotated view) using 
> sg_alloc_table_from_pages() could produce a more compact table. With the 
> caveat of that it doesn't always work (see i915_gem_userptr.c/st_set_pages).
> 
> So maybe promote to driver public st_set_pages and call in on an array 
> of pages?
> 

Disregarded regards to your later mail.

> > +
> > +	return st;
> > +
> > +err_sg_alloc:
> > +	kfree(st);
> 
> Here you lose ret from sg_alloc_table.
> 

Good catch.

> > +err_st_alloc:
> > +	return ERR_PTR(-ENOMEM);
> > +}
> > +
> >   static int
> >   i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >   {
> > @@ -2777,6 +2818,9 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >   	else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
> >   		vma->ggtt_view.pages =
> >   			intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
> > +	else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL)
> > +		vma->ggtt_view.pages =
> > +			intel_partial_pages(&vma->ggtt_view, vma->obj);
> >   	else
> >   		WARN_ONCE(1, "GGTT view %u not implemented!\n",
> >   			  vma->ggtt_view.type);
> > @@ -2859,6 +2903,8 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> >   	if (view->type == I915_GGTT_VIEW_NORMAL ||
> >   	    view->type == I915_GGTT_VIEW_ROTATED) {
> >   		return obj->base.size;
> > +	} else if (view->type == I915_GGTT_VIEW_PARTIAL) {
> > +		return view->params.partial.size << PAGE_SHIFT;
> >   	} else {
> >   		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
> >   		return obj->base.size;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index 34b7cca..ab1ad8a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -117,7 +117,8 @@ typedef uint64_t gen8_pde_t;
> >
> >   enum i915_ggtt_view_type {
> >   	I915_GGTT_VIEW_NORMAL = 0,
> > -	I915_GGTT_VIEW_ROTATED
> > +	I915_GGTT_VIEW_ROTATED,
> > +	I915_GGTT_VIEW_PARTIAL,
> >   };
> >
> >   struct intel_rotation_info {
> > @@ -130,6 +131,13 @@ struct intel_rotation_info {
> >   struct i915_ggtt_view {
> >   	enum i915_ggtt_view_type type;
> >
> > +	union {
> > +		struct {
> > +			pgoff_t offset;
> > +			size_t size;
> 
> Size is in pages right? Maybe it would be more self-documenting to use 
> some basic type like unsigned int or long since size_t, to me at least, 
> suggests bytes.
> 

Yeah, using unsigned long for offset and unsigned int for size, maps
more directly to the functions they're used with.

> > +		} partial;
> > +	} params;
> > +
> >   	struct sg_table *pages;
> >
> >   	union {
> > @@ -495,7 +503,10 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
> >   	if (WARN_ON(!a || !b))
> >   		return false;
> >
> > -	return a->type == b->type;
> > +	if (a->type != b->type)
> > +		return false;
> > +
> > +	return !memcmp(&a->params, &b->params, sizeof(a->params));
> 
> So for rotated views it would still do memcmp. OK structure is zeroed on 
> alloc, but it is pointless to do so.
> 

I'd rather not have special cases for each view type all around the
code, as I think even the current ones that there are should be reduced.

Regards, Joonas

> Regards,
> 
> Tvrtko


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

Re: [PATCH 1/5] drm/i915: Do not clear mappings beyond VMA size

[Daniel Vetter]

On Mon, Apr 27, 2015 at 01:55:15PM +0100, Tvrtko Ursulin wrote:
> 
> On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
> >Do not to clear mappings outside the allocated VMA under any
> >circumstances. Only clear the smaller of VMA or object page count.
> >
> >This is required to allow creating partial object VMAs which in
> >turn are needed for partial GGTT views.
> >
> >Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >index 6fae6bd..ee9ff8e 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >@@ -1947,19 +1947,23 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
> >  	struct drm_device *dev = vma->vm->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj = vma->obj;
> >+	const uint64_t size = min_t(uint64_t,
> >+				    obj->base.size,
> >+				    vma->node.size);
> >
> >  	if (vma->bound & GLOBAL_BIND) {
> >  		vma->vm->clear_range(vma->vm,
> >  				     vma->node.start,
> >-				     obj->base.size,
> >+				     size,
> >  				     true);
> >  	}
> >
> >  	if (dev_priv->mm.aliasing_ppgtt && vma->bound & LOCAL_BIND) {
> >  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> >+
> >  		appgtt->base.clear_range(&appgtt->base,
> >  					 vma->node.start,
> >-					 obj->base.size,
> >+					 size,
> >  					 true);
> >  	}
> >  }
> >
> 
> Looks good to me,
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx