[PATCH v2 0/5] Partial GGTT views

[PATCH v2 0/5] Partial GGTT views

[Joonas Lahtinen]

This series adds partial GGTT views and uses them from the GTT mmap
fault handler for objects bigger than the whole aperture. This use case
was earlier early detected and rejected completely, so no previous code
paths are changed, only a new one is added. This allows to handle huge
objects (compared to aperture size) through mmap from user land.

And not only huge objects, but when objects are of regular size and
aperture is shrinked due to virtualization (like with vGT). XenGT is the
original and primary target for this initial partial GGTT view support,
where programs fail due to the GGTT being extremely small, but payloads
remain the same.

Tiling is not supported yet. The gem_mmap_gtt suite passes without
errors even when the mmap fault handler would be converted to
unconditionally use partial views.

Regards, Joonas

v2:
- Cleanups, and bump the activation treshold to only cover the code the
  code path that was previously early rejected.

Joonas Lahtinen (5):
  drm/i915: Do not clear mappings beyond VMA size
  drm/i915: Do not make assumptions on GGTT VMA sizes
  drm/i915: Consider object pinned if any VMA is pinned
  drm/i915: Add a partial GGTT view type
  drm/i915: Use partial view in mmap fault handler

 drivers/gpu/drm/i915/i915_gem.c     | 179 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_gem_gtt.c |  76 ++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_gtt.h |  19 +++-
 3 files changed, 204 insertions(+), 70 deletions(-)

-- 
1.9.3



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

[PATCH v3 1/4] 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)
v3:
- Do not bother with debug wording. (Tvrtko Ursulin)
v4:
- Clearer logic for calculating map_and_fenceable (Tvrtko Ursulin)

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c378f04..473e947 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3498,7 +3498,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,
@@ -3517,36 +3518,60 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 	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 object (view type=%u) alignment requested %u\n",
+			  ggtt_view ? ggtt_view->type : 0,
+			  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 an object (view type=%u) larger than the aperture: size=%u > %s aperture=%lu\n",
+			  ggtt_view ? ggtt_view->type : 0,
+			  size,
 			  flags & PIN_MAPPABLE ? "mappable" : "total",
 			  end);
 		return ERR_PTR(-E2BIG);
@@ -4208,7 +4233,8 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
-	if ((bound ^ vma->bound) & GLOBAL_BIND) {
+	if (ggtt_view && ggtt_view->type == I915_GGTT_VIEW_NORMAL &&
+	    (bound ^ vma->bound) & GLOBAL_BIND) {
 		bool mappable, fenceable;
 		u32 fence_size, fence_alignment;
 
@@ -4227,9 +4253,9 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 			    dev_priv->gtt.mappable_end);
 
 		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 8fee678..7f3c03a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2838,3 +2838,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.9.3



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

[PATCH v3 2/4] 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 473e947..7e8eafe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5253,13 +5253,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.9.3



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

[PATCH v3 3/4] 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.

v2:
- Retain error value from sg_alloc_table (Tvrtko Ursulin)
- Do not zero already zeroed variable (Tvrtko Ursulin)
- Use more common variable types for page size/offset (Tvrtko Ursulin)
v3:
- Only compare additional view parameters when need to (Tvrtko Ursulin)
v4:
- Do zero out the variable that needs to be (bug introduced in v2).

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
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 | 16 +++++++++++--
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7f3c03a..b058931 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2753,6 +2753,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 = -ENOMEM;
+
+	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(ret);
+}
+
 static int
 i915_get_ggtt_vma_pages(struct i915_vma *vma)
 {
@@ -2766,6 +2807,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);
@@ -2855,6 +2899,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..0d46dd2 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 {
+			unsigned long offset;
+			unsigned int size;
+		} partial;
+	} params;
+
 	struct sg_table *pages;
 
 	union {
@@ -495,7 +503,11 @@ 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;
+	if (a->type == I915_GGTT_VIEW_PARTIAL)
+		return !memcmp(&a->params, &b->params, sizeof(a->params));
+	return true;
 }
 
 size_t
-- 
1.9.3



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

[PATCH v3 4/4] 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.

v2:
- Only use partial views in the case where early rejection was
  previously done.
- Account variable type changes from previous reroll.
v3:
- Add a comment about overwriting existing page entries.
  (Tvrtko Ursulin)
- Whitespace fixes.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
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 | 73 ++++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7e8eafe..488ae13 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 the aperture. */
+	if (obj->base.size >= dev_priv->gtt.mappable_end) {
+		static const unsigned int 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(unsigned int,
+			      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,50 @@ 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);
-		int i;
+	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
+		/* Overriding existing pages in partial view does not cause
+		 * us any trouble as TLBs are still valid because the fault
+		 * is due to userspace losing part of the mapping or never
+		 * having accessed it before (at this partials' range).
+		 */
+		unsigned long base = vma->vm_start +
+				     (view.params.partial.offset << PAGE_SHIFT);
+		unsigned 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 +1931,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.9.3



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

Re: [PATCH v3 1/4] drm/i915: Do not make assumptions on GGTT VMA sizes

[Tvrtko Ursulin]

On 05/06/2015 12:33 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)
> v3:
> - Do not bother with debug wording. (Tvrtko Ursulin)
> v4:
> - Clearer logic for calculating map_and_fenceable (Tvrtko Ursulin)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c     | 74 +++++++++++++++++++++++++------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 23 ++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |  4 ++
>   3 files changed, 77 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c378f04..473e947 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3498,7 +3498,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,
> @@ -3517,36 +3518,60 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   	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;
> +	}

I don't like this duplication, but apparently Chris wants it like this 
and plans to change it all shortly AFAIR. In the view of that:

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 v3 3/4] drm/i915: Add a partial GGTT view type

[Tvrtko Ursulin]

On 05/06/2015 12:35 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.
>
> v2:
> - Retain error value from sg_alloc_table (Tvrtko Ursulin)
> - Do not zero already zeroed variable (Tvrtko Ursulin)
> - Use more common variable types for page size/offset (Tvrtko Ursulin)
> v3:
> - Only compare additional view parameters when need to (Tvrtko Ursulin)
> v4:
> - Do zero out the variable that needs to be (bug introduced in v2).

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 v3 4/4] drm/i915: Use partial view in mmap fault handler

[Tvrtko Ursulin]

On 05/06/2015 12:36 PM, 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.
>
> v2:
> - Only use partial views in the case where early rejection was
>    previously done.
> - Account variable type changes from previous reroll.
> v3:
> - Add a comment about overwriting existing page entries.
>    (Tvrtko Ursulin)
> - Whitespace fixes.

Again I wish two vm_insert_pfn loops could be unified by making partial 
view a special case of normal, but as this is another area where 
imminent change has been hinted, and it doesn't matter hugely anyway:

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 v3 2/4] drm/i915: Consider object pinned if any VMA is pinned

[Tvrtko Ursulin]

On 05/06/2015 12:34 PM, 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>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

Callers are inconsistent with what they want to know when calling this 
function which needs some follow-up work, but this is I think better in 
the short term.

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 v3 4/4] 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: 6329
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  342/342              342/342
BYT                                  286/286              286/286
BDW                                  321/321              321/321
-------------------------------------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 v3 3/4] drm/i915: Add a partial GGTT view type

[Daniel Vetter]

On Wed, May 06, 2015 at 01:24:18PM +0100, Tvrtko Ursulin wrote:
> 
> 
> On 05/06/2015 12:35 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.
> >
> >v2:
> >- Retain error value from sg_alloc_table (Tvrtko Ursulin)
> >- Do not zero already zeroed variable (Tvrtko Ursulin)
> >- Use more common variable types for page size/offset (Tvrtko Ursulin)
> >v3:
> >- Only compare additional view parameters when need to (Tvrtko Ursulin)
> >v4:
> >- Do zero out the variable that needs to be (bug introduced in v2).
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Pulled in entire series, thanks. Imo we have now an even bigger chaos
between i915_gem.c and i915_gem_gtt.c around vma handling. But as
discussed this is something we can clean up once the dust has settled a
bit I think. I guess we could extract an i915_gem_vma.c for all this code,
leaving only low-level gtt handling to i915_gem_gtt.c.
-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

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

[Chris Wilson]

On Wed, May 06, 2015 at 02:35:38PM +0300, Joonas Lahtinen wrote:
> +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 = -ENOMEM;
> +
> +	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;

This is a nasty bug, as is the converse where st->nents <
st->orig_nents.

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a7e39d4..115df10 100644
@@ -2890,7 +2900,7 @@ intel_partial_pages(const struct i915_ggtt_view *view,
                    struct drm_i915_gem_object *obj)
 {
        struct sg_table *st;
-       struct scatterlist *sg;
+       struct scatterlist *sg, *end;
        struct sg_page_iter obj_sg_iter;
        int ret = -ENOMEM;
 
@@ -2902,24 +2912,31 @@ intel_partial_pages(const struct i915_ggtt_view *view,
        if (ret)
                goto err_sg_alloc;
 
-       sg = st->sgl;
+       end = 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;
+               if (WARN_ON(st->nents >= view->params.partial.size)) {
+                       ret = -ENODEV;
+                       goto err_pages;
+               }
 
                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;
 
+               end = sg;
                sg = sg_next(sg);
                st->nents++;
        }
+       sg_mark_end(end);
 
        return st;
 
+err_pages:
+       sg_free_table(st);
 err_sg_alloc:
        kfree(st);
 err_st_alloc:

-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 v3 3/4] drm/i915: Add a partial GGTT view type

[Joonas Lahtinen]

Hi,

As discussed in IRC, this patch is not relevant. The interface is bit
misbehaving. CC'd Imre who agreed to go and change the interface to more
intuitive one.

Regards, Joonas

On ti, 2015-06-09 at 09:56 +0100, Chris Wilson wrote:
> On Wed, May 06, 2015 at 02:35:38PM +0300, Joonas Lahtinen wrote:
> > +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 = -ENOMEM;
> > +
> > +	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;
> 
> This is a nasty bug, as is the converse where st->nents <
> st->orig_nents.
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a7e39d4..115df10 100644
> @@ -2890,7 +2900,7 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>                     struct drm_i915_gem_object *obj)
>  {
>         struct sg_table *st;
> -       struct scatterlist *sg;
> +       struct scatterlist *sg, *end;
>         struct sg_page_iter obj_sg_iter;
>         int ret = -ENOMEM;
>  
> @@ -2902,24 +2912,31 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>         if (ret)
>                 goto err_sg_alloc;
>  
> -       sg = st->sgl;
> +       end = 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;
> +               if (WARN_ON(st->nents >= view->params.partial.size)) {
> +                       ret = -ENODEV;
> +                       goto err_pages;
> +               }
>  
>                 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;
>  
> +               end = sg;
>                 sg = sg_next(sg);
>                 st->nents++;
>         }
> +       sg_mark_end(end);
>  
>         return st;
>  
> +err_pages:
> +       sg_free_table(st);
>  err_sg_alloc:
>         kfree(st);
>  err_st_alloc:
> 
> -Chris
> 


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

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

[Chris Wilson]

On Wed, Jun 10, 2015 at 01:38:55PM +0300, Joonas Lahtinen wrote:
> Hi,
> 
> As discussed in IRC, this patch is not relevant. The interface is bit
> misbehaving. CC'd Imre who agreed to go and change the interface to more
> intuitive one.

partial.offset + partial.size is never checked for correctness, that
would be worthy of an assert during i915_vma_create to catch future
bugs.
-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