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

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

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
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 9d3852c..fc562c6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1945,19 +1945,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.9.3



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

[PATCH v2 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)
v3:
- Do not bother with debug wording. (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     | 103 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_gem_gtt.c |  23 ++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h |   4 ++
 3 files changed, 92 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e8f6f4c..9717c9d 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,
@@ -3516,36 +3517,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);
@@ -4207,28 +4232,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 fc562c6..640584f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2847,3 +2847,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 v2 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 9717c9d..a020836 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 v2 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.

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)

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 | 45 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h | 15 +++++++++++--
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 640584f..ba28a67 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2762,6 +2762,46 @@ 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;
+	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)
 {
@@ -2775,6 +2815,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);
@@ -2864,6 +2907,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..96a9ef7 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,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.9.3



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

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

[Joonas Lahtinen]

Use partial view for huge BOs (bigger than the mappable aperture)
in fault handler so that they can be accessed without 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.

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 | 69 +++++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a020836..2f3fa0b 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,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);
-		int i;
+	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
+		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 +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.9.3



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

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

[Tvrtko Ursulin]

On 04/30/2015 12:19 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)
>
> 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     | 103 +++++++++++++++++++++++-------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c |  23 ++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |   4 ++
>   3 files changed, 92 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e8f6f4c..9717c9d 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,
> @@ -3516,36 +3517,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 do not like this almost identical branches - so I defer to Chris to 
okay that this is what he wanted.

>   	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);
> @@ -4207,28 +4232,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) {

I still don't get this. !ggtt_view means GLOBAL_BIND cannot be set, what 
am I missing? It wouldn't work if the condition was just the type check?

> +			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 fc562c6..640584f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2847,3 +2847,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);

It is a marginal point, but I wonder is size zero could be considered as 
a failure value and acted upon from the caller more gracefuly.

And in general I wonder if something like I915_BUG_ON which would 
re-route fops to fail-all and grab struct_mutex, or something, would 
maybe be an option.

Regards,

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

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

[Tvrtko Ursulin]

On 04/30/2015 12:20 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)
>
> 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 | 45 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h | 15 +++++++++++--
>   2 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 640584f..ba28a67 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2762,6 +2762,46 @@ 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;
> +	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)
>   {
> @@ -2775,6 +2815,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);
> @@ -2864,6 +2907,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..96a9ef7 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,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));

Still don't like this, would this be so bad:

if (a->type != PARTIAL)
	return a->type == b->type;
else
	return !memcmp(...)

?

Regards,

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

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

[Tvrtko Ursulin]

On 04/30/2015 12:21 PM, Joonas Lahtinen wrote:
>
> Use partial view for huge BOs (bigger than the mappable aperture)
> in fault handler so that they can be accessed without 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.
>
> 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 | 69 +++++++++++++++++++++++++++--------------
>   1 file changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a020836..2f3fa0b 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));

I think you don't need to memset since you assigned normal view to it.

> +		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);

Next fault outside this partial view will create a new vma? Is there 
something which would remove the old one for that case? Or they are just 
allowed to accumulate?

Regards,

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

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

[Tvrtko Ursulin]

On 04/30/2015 12:21 PM, Joonas Lahtinen wrote:
>
> Use partial view for huge BOs (bigger than the mappable aperture)
> in fault handler so that they can be accessed without 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.
>
> 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 | 69 +++++++++++++++++++++++++++--------------
>   1 file changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a020836..2f3fa0b 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,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);
> -		int i;
> +	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
> +		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);

If I read the diff correctly you don't have equivalent handling (as the 
normal view) for when the case when the pre-fault fails somewhere in the 
middle?

That means if that happens you'll call vm_insert_pfn on the whole range 
again - is that OK?

Regards,

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

Re: [PATCH v2 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: 6298
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  264/264              264/264
BYT                 -3              227/227              224/227
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_dummy_reloc_loop@render      FAIL(1)PASS(17)      TIMEOUT(1)PASS(1)
 BYT  igt@gem_pipe_control_store_loop@fresh-buffer      FAIL(1)TIMEOUT(9)PASS(9)      TIMEOUT(2)
*BYT  igt@gem_threaded_access_tiled      FAIL(1)PASS(6)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
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 v2 5/5] drm/i915: Use partial view in mmap fault handler

[Joonas Lahtinen]

On to, 2015-04-30 at 15:54 +0100, Tvrtko Ursulin wrote:
> On 04/30/2015 12:21 PM, Joonas Lahtinen wrote:
> >
> > Use partial view for huge BOs (bigger than the mappable aperture)
> > in fault handler so that they can be accessed without 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.
> >
> > 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 | 69 +++++++++++++++++++++++++++--------------
> >   1 file changed, 46 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index a020836..2f3fa0b 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,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);
> > -		int i;
> > +	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
> > +		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);
> 
> If I read the diff correctly you don't have equivalent handling (as the 
> normal view) for when the case when the pre-fault fails somewhere in the 
> middle?
> 

True so, the flag fault_mappable is used for the normal view to track
whether all pages were inserted and it makes sense to just insert the
faulted one. I just didn't want to add another flag to track the same
for each vma.

Regards, Joonas

> That means if that happens you'll call vm_insert_pfn on the whole range 
> again - is that OK?
> 
> Regards,
> 
> Tvrtko


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

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

[Tvrtko Ursulin]

On 05/04/2015 12:51 PM, Joonas Lahtinen wrote:
> On to, 2015-04-30 at 15:54 +0100, Tvrtko Ursulin wrote:
>> On 04/30/2015 12:21 PM, Joonas Lahtinen wrote:
>>>
>>> Use partial view for huge BOs (bigger than the mappable aperture)
>>> in fault handler so that they can be accessed without 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.
>>>
>>> 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 | 69 +++++++++++++++++++++++++++--------------
>>>    1 file changed, 46 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index a020836..2f3fa0b 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,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);
>>> -		int i;
>>> +	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
>>> +		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);
>>
>> If I read the diff correctly you don't have equivalent handling (as the
>> normal view) for when the case when the pre-fault fails somewhere in the
>> middle?
>>
>
> True so, the flag fault_mappable is used for the normal view to track
> whether all pages were inserted and it makes sense to just insert the
> faulted one. I just didn't want to add another flag to track the same
> for each vma.

But it is safe to do it multiple times?

Either way I would put a comment in explaining the difference between 
code paths.

Regards,

Tvrtko



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

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

[Daniel Vetter]

On Thu, Apr 30, 2015 at 01:16:30PM +0100, Tvrtko Ursulin wrote:
> On 04/30/2015 12:20 PM, Joonas Lahtinen wrote:
> >@@ -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));
> 
> Still don't like this, would this be so bad:
> 
> if (a->type != PARTIAL)
> 	return a->type == b->type;
> else
> 	return !memcmp(...)

Why do we even need this? memcmp implies comparing just one part of the
struct, doesn't it? Of course this means we need to clear it, but imo
that's the rtdt anyway.
-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 v2 5/5] drm/i915: Use partial view in mmap fault handler

[Joonas Lahtinen]

On ti, 2015-05-05 at 10:07 +0100, Tvrtko Ursulin wrote:
> On 05/04/2015 12:51 PM, Joonas Lahtinen wrote:
> > On to, 2015-04-30 at 15:54 +0100, Tvrtko Ursulin wrote:
> >> On 04/30/2015 12:21 PM, Joonas Lahtinen wrote:
> >>>
> >>> Use partial view for huge BOs (bigger than the mappable aperture)
> >>> in fault handler so that they can be accessed without 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.
> >>>
> >>> 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 | 69 +++++++++++++++++++++++++++--------------
> >>>    1 file changed, 46 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>> index a020836..2f3fa0b 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,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);
> >>> -		int i;
> >>> +	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
> >>> +		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);
> >>
> >> If I read the diff correctly you don't have equivalent handling (as the
> >> normal view) for when the case when the pre-fault fails somewhere in the
> >> middle?
> >>
> >
> > True so, the flag fault_mappable is used for the normal view to track
> > whether all pages were inserted and it makes sense to just insert the
> > faulted one. I just didn't want to add another flag to track the same
> > for each vma.
> 
> But it is safe to do it multiple times?
> 

Put a comment in there.

> Either way I would put a comment in explaining the difference between 
> code paths.
> 
> Regards,
> 
> Tvrtko
> 
> 
> 


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

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

[Joonas Lahtinen]

On ke, 2015-05-06 at 12:20 +0200, Daniel Vetter wrote:
> On Thu, Apr 30, 2015 at 01:16:30PM +0100, Tvrtko Ursulin wrote:
> > On 04/30/2015 12:20 PM, Joonas Lahtinen wrote:
> > >@@ -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));
> > 
> > Still don't like this, would this be so bad:
> > 
> > if (a->type != PARTIAL)
> > 	return a->type == b->type;
> > else
> > 	return !memcmp(...)
> 
> Why do we even need this? memcmp implies comparing just one part of the
> struct, doesn't it? Of course this means we need to clear it, but imo
> that's the rtdt anyway.

I only noticed this comment now (just sent v3 of the series out), I'm
fine with leaving it like it was in the revision 1, which means that the
last line is return !memcmp(..), so if the patch is otherwise OK, shall
I make a one more revision, or will you merge manually?

Regards, Joonas

> -Daniel


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

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

[Joonas Lahtinen]

On to, 2015-04-30 at 13:03 +0100, Tvrtko Ursulin wrote:
> On 04/30/2015 12:19 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)
> >
> > 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     | 103 +++++++++++++++++++++++-------------
> >   drivers/gpu/drm/i915/i915_gem_gtt.c |  23 ++++++++
> >   drivers/gpu/drm/i915/i915_gem_gtt.h |   4 ++
> >   3 files changed, 92 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index e8f6f4c..9717c9d 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,
> > @@ -3516,36 +3517,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 do not like this almost identical branches - so I defer to Chris to 
> okay that this is what he wanted.
> 

Confirmed in IRC that the above is according to what Chris meant.

> >   	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);
> > @@ -4207,28 +4232,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) {
> 
> I still don't get this. !ggtt_view means GLOBAL_BIND cannot be set, what 
> am I missing? It wouldn't work if the condition was just the type check?
> 

Clarified this.

> > +			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 fc562c6..640584f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2847,3 +2847,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);
> 
> It is a marginal point, but I wonder is size zero could be considered as 
> a failure value and acted upon from the caller more gracefuly.
> 
> And in general I wonder if something like I915_BUG_ON which would 
> re-route fops to fail-all and grab struct_mutex, or something, would 
> maybe be an option.
> 

I wouldn't like to overcomplicate the error handling for for imaginary
or promille use cases, so I think it is a good rule of thumb that bug on
if the kernel would OOPS anyway (which holds here).

Regards, Joonas

> Regards,
> 
> Tvrtko


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

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

[Daniel Vetter]

On Wed, May 06, 2015 at 02:40:48PM +0300, Joonas Lahtinen wrote:
> On ke, 2015-05-06 at 12:20 +0200, Daniel Vetter wrote:
> > On Thu, Apr 30, 2015 at 01:16:30PM +0100, Tvrtko Ursulin wrote:
> > > On 04/30/2015 12:20 PM, Joonas Lahtinen wrote:
> > > >@@ -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));
> > > 
> > > Still don't like this, would this be so bad:
> > > 
> > > if (a->type != PARTIAL)
> > > 	return a->type == b->type;
> > > else
> > > 	return !memcmp(...)
> > 
> > Why do we even need this? memcmp implies comparing just one part of the
> > struct, doesn't it? Of course this means we need to clear it, but imo
> > that's the rtdt anyway.
> 
> I only noticed this comment now (just sent v3 of the series out), I'm
> fine with leaving it like it was in the revision 1, which means that the
> last line is return !memcmp(..), so if the patch is otherwise OK, shall
> I make a one more revision, or will you merge manually?

Already pulled in, please do a follow-up patch.

Thanks, 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