[PATCH 1/2] drm/i915: Allow overlapping userptr objects

[PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Chris Wilson]

Whilst I strongly advise against doing so for the implicit coherency
issues between the multiple buffer objects accessing the same backing
store, it nevertheless is a valid use case, akin to mmaping the same
file multiple times.

The reason why we forbade it earlier was that our use of the interval
tree for fast invalidation upon vma changes excluded overlapping
objects. So in the case where the user wishes to create such pairs of
overlapping objects, we degrade the range invalidation to walkin the
linear list of objects associated with the mm.

v2: Compile for mmu-notifiers after tweaking

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Li, Victor Y" <victor.y.li@intel.com>
Cc: "Kelley, Sean V" <sean.v.kelley@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: Akash Goel <akash.goel@intel.com>
Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 139 ++++++++++++++++++++++++--------
 1 file changed, 104 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 21ea928..7c38f50 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -40,19 +40,87 @@ struct i915_mmu_notifier {
 	struct hlist_node node;
 	struct mmu_notifier mn;
 	struct rb_root objects;
+	struct list_head linear;
 	struct drm_device *dev;
 	struct mm_struct *mm;
 	struct work_struct work;
 	unsigned long count;
 	unsigned long serial;
+	bool is_linear;
 };
 
 struct i915_mmu_object {
 	struct i915_mmu_notifier *mmu;
 	struct interval_tree_node it;
+	struct list_head link;
 	struct drm_i915_gem_object *obj;
+	bool is_linear;
 };
 
+static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	unsigned long end;
+
+	mutex_lock(&dev->struct_mutex);
+	/* Cancel any active worker and force us to re-evaluate gup */
+	obj->userptr.work = NULL;
+
+	if (obj->pages != NULL) {
+		struct drm_i915_private *dev_priv = to_i915(dev);
+		struct i915_vma *vma, *tmp;
+		bool was_interruptible;
+
+		was_interruptible = dev_priv->mm.interruptible;
+		dev_priv->mm.interruptible = false;
+
+		list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
+			int ret = i915_vma_unbind(vma);
+			WARN_ON(ret && ret != -EIO);
+		}
+		WARN_ON(i915_gem_object_put_pages(obj));
+
+		dev_priv->mm.interruptible = was_interruptible;
+	}
+
+	end = obj->userptr.ptr + obj->base.size;
+
+	drm_gem_object_unreference(&obj->base);
+	mutex_unlock(&dev->struct_mutex);
+
+	return end;
+}
+
+static void invalidate_range__linear(struct i915_mmu_notifier *mn,
+				     struct mm_struct *mm,
+				     unsigned long start,
+				     unsigned long end)
+{
+	struct i915_mmu_object *mmu;
+	unsigned long serial;
+
+restart:
+	serial = mn->serial;
+	list_for_each_entry(mmu, &mn->linear, link) {
+		struct drm_i915_gem_object *obj;
+
+		if (mmu->it.last < start || mmu->it.start > end)
+			continue;
+
+		obj = mmu->obj;
+		drm_gem_object_reference(&obj->base);
+		spin_unlock(&mn->lock);
+
+		cancel_userptr(obj);
+
+		spin_lock(&mn->lock);
+		if (serial != mn->serial)
+			goto restart;
+	}
+
+	spin_unlock(&mn->lock);
+}
+
 static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 						       struct mm_struct *mm,
 						       unsigned long start,
@@ -60,16 +128,19 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 {
 	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
 	struct interval_tree_node *it = NULL;
+	unsigned long next = start;
 	unsigned long serial = 0;
 
 	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
-	while (start < end) {
+	while (next < end) {
 		struct drm_i915_gem_object *obj;
 
 		obj = NULL;
 		spin_lock(&mn->lock);
+		if (mn->is_linear)
+			return invalidate_range__linear(mn, mm, start, end);
 		if (serial == mn->serial)
-			it = interval_tree_iter_next(it, start, end);
+			it = interval_tree_iter_next(it, next, end);
 		else
 			it = interval_tree_iter_first(&mn->objects, start, end);
 		if (it != NULL) {
@@ -81,31 +152,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		if (obj == NULL)
 			return;
 
-		mutex_lock(&mn->dev->struct_mutex);
-		/* Cancel any active worker and force us to re-evaluate gup */
-		obj->userptr.work = NULL;
-
-		if (obj->pages != NULL) {
-			struct drm_i915_private *dev_priv = to_i915(mn->dev);
-			struct i915_vma *vma, *tmp;
-			bool was_interruptible;
-
-			was_interruptible = dev_priv->mm.interruptible;
-			dev_priv->mm.interruptible = false;
-
-			list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
-				int ret = i915_vma_unbind(vma);
-				WARN_ON(ret && ret != -EIO);
-			}
-			WARN_ON(i915_gem_object_put_pages(obj));
-
-			dev_priv->mm.interruptible = was_interruptible;
-		}
-
-		start = obj->userptr.ptr + obj->base.size;
-
-		drm_gem_object_unreference(&obj->base);
-		mutex_unlock(&mn->dev->struct_mutex);
+		next = cancel_userptr(obj);
 	}
 }
 
@@ -151,6 +198,8 @@ i915_mmu_notifier_get(struct drm_device *dev, struct mm_struct *mm)
 	mmu->objects = RB_ROOT;
 	mmu->count = 0;
 	mmu->serial = 0;
+	INIT_LIST_HEAD(&mmu->linear);
+	mmu->is_linear = false;
 
 	/* Protected by mmap_sem (write-lock) */
 	ret = __mmu_notifier_register(&mmu->mn, mm);
@@ -197,6 +246,17 @@ static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mmu)
 		mmu->serial = 1;
 }
 
+static bool i915_mmu_notifier_is_linear(struct i915_mmu_notifier *mmu)
+{
+	struct i915_mmu_object *mn;
+
+	list_for_each_entry(mn, &mmu->linear, link)
+		if (mn->is_linear)
+			return true;
+
+	return false;
+}
+
 static void
 i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
 		      struct i915_mmu_object *mn)
@@ -205,6 +265,9 @@ i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
 
 	spin_lock(&mmu->lock);
 	interval_tree_remove(&mn->it, &mmu->objects);
+	list_del(&mn->link);
+	if (mn->is_linear)
+		mmu->is_linear = i915_mmu_notifier_is_linear(mmu);
 	__i915_mmu_notifier_update_serial(mmu);
 	spin_unlock(&mmu->lock);
 
@@ -230,7 +293,6 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
 	 */
 	i915_gem_retire_requests(mmu->dev);
 
-	/* Disallow overlapping userptr objects */
 	spin_lock(&mmu->lock);
 	it = interval_tree_iter_first(&mmu->objects,
 				      mn->it.start, mn->it.last);
@@ -243,14 +305,22 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
 		 * to flush their object references upon which the object will
 		 * be removed from the interval-tree, or the the range is
 		 * still in use by another client and the overlap is invalid.
+		 *
+		 * If we do have an overlap, we cannot use the interval tree
+		 * for fast range invalidation.
 		 */
 
 		obj = container_of(it, struct i915_mmu_object, it)->obj;
-		ret = obj->userptr.workers ? -EAGAIN : -EINVAL;
-	} else {
+		if (!obj->userptr.workers)
+			mmu->is_linear = mn->is_linear = true;
+		else
+			ret = -EAGAIN;
+	} else
 		interval_tree_insert(&mn->it, &mmu->objects);
+
+	if (ret == 0) {
+		list_add(&mn->link, &mmu->linear);
 		__i915_mmu_notifier_update_serial(mmu);
-		ret = 0;
 	}
 	spin_unlock(&mmu->lock);
 	mutex_unlock(&mmu->dev->struct_mutex);
@@ -611,12 +681,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
  * We impose several restrictions upon the memory being mapped
  * into the GPU.
  * 1. It must be page aligned (both start/end addresses, i.e ptr and size).
- * 2. It cannot overlap any other userptr object in the same address space.
- * 3. It must be normal system memory, not a pointer into another map of IO
+ * 2. It must be normal system memory, not a pointer into another map of IO
  *    space (e.g. it must not be a GTT mmapping of another object).
- * 4. We only allow a bo as large as we could in theory map into the GTT,
+ * 3. We only allow a bo as large as we could in theory map into the GTT,
  *    that is we limit the size to the total size of the GTT.
- * 5. The bo is marked as being snoopable. The backing pages are left
+ * 4. The bo is marked as being snoopable. The backing pages are left
  *    accessible directly by the CPU, but reads and writes by the GPU may
  *    incur the cost of a snoop (unless you have an LLC architecture).
  *
-- 
1.9.1

[PATCH 2/2] drm/i915: Serialise userptr gup with mmu-notifier

[Chris Wilson]

Jerome Glisse pointed out that get_user_pages() does not synchronize
with concurrent invalidations of the VMA. As such if the backing vma is
changed whilst the pages for the object are being grabbed for use by the
GPU, we may end up with a random mixture of page references being held.
Worse still as the mmu-notifier will believe that the VMA invalidation
was complete and the old page references dropped.

In order to serialise gup with mmu-notifier, we use a seqlock to detect
when an invalidation has occurred in parallel to our gup and if so cancel
the gup. The detection is a little coarse, but hopefully we never see
contention here!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 108 +++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 7c38f50..8ed670f 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -46,6 +46,10 @@ struct i915_mmu_notifier {
 	struct work_struct work;
 	unsigned long count;
 	unsigned long serial;
+	unsigned long invalidate;
+	unsigned long invalidate_serial;
+	unsigned long invalidate_start;
+	unsigned long invalidate_end;
 	bool is_linear;
 };
 
@@ -131,6 +135,19 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 	unsigned long next = start;
 	unsigned long serial = 0;
 
+	spin_lock(&mn->lock);
+	if (mn->invalidate++ == 0) {
+		mn->invalidate_start = start;
+		mn->invalidate_end = end;
+	} else {
+		if (mn->invalidate_start > start)
+			mn->invalidate_start = start;
+		if (mn->invalidate_end < end)
+			mn->invalidate_end = end;
+	}
+	mn->invalidate_serial++;
+	spin_unlock(&mn->lock);
+
 	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
 	while (next < end) {
 		struct drm_i915_gem_object *obj;
@@ -156,8 +173,24 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 	}
 }
 
+static void i915_gem_userptr_mn_invalidate_range_end(struct mmu_notifier *_mn,
+						     struct mm_struct *mm,
+						     unsigned long start,
+						     unsigned long end)
+{
+	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
+
+	spin_lock(&mn->lock);
+	if (--mn->invalidate == 0) {
+		mn->invalidate_start = ~0UL;
+		mn->invalidate_end = 0UL;
+	}
+	spin_unlock(&mn->lock);
+}
+
 static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
 	.invalidate_range_start = i915_gem_userptr_mn_invalidate_range_start,
+	.invalidate_range_end = i915_gem_userptr_mn_invalidate_range_end,
 };
 
 static struct i915_mmu_notifier *
@@ -201,6 +234,10 @@ i915_mmu_notifier_get(struct drm_device *dev, struct mm_struct *mm)
 	INIT_LIST_HEAD(&mmu->linear);
 	mmu->is_linear = false;
 
+	mmu->invalidate = 0;
+	mmu->invalidate_start = ~0UL;
+	mmu->invalidate_end = 0UL;
+
 	/* Protected by mmap_sem (write-lock) */
 	ret = __mmu_notifier_register(&mmu->mn, mm);
 	if (ret) {
@@ -394,6 +431,44 @@ destroy_mmu:
 	return ret;
 }
 
+static bool
+i915_mmu_object_get(struct i915_mmu_object *mn, unsigned *serial)
+{
+	struct i915_mmu_notifier *mmu;
+	bool valid;
+
+	if (mn == NULL)
+		return true;
+
+	mmu = mn->mmu;
+
+	spin_lock(&mmu->lock);
+	valid = (mn->it.last <= mmu->invalidate_start ||
+		 mn->it.start >= mmu->invalidate_end);
+	*serial = mmu->invalidate_serial;
+	spin_unlock(&mmu->lock);
+
+	return valid;
+}
+
+static bool
+i915_mmu_object_put(struct i915_mmu_object *mn, unsigned serial)
+{
+	struct i915_mmu_notifier *mmu;
+	bool valid;
+
+	if (mn == NULL)
+		return true;
+
+	mmu = mn->mmu;
+
+	spin_lock(&mmu->lock);
+	valid = serial == mmu->invalidate_serial;
+	spin_unlock(&mmu->lock);
+
+	return valid;
+}
+
 #else
 
 static void
@@ -413,6 +488,20 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
 
 	return 0;
 }
+
+static bool
+i915_mmu_object_get(struct i915_mmu_object *mmu, unsigned *serial)
+{
+	*serial = 0;
+	return true;
+}
+
+static bool
+i915_mmu_object_put(struct i915_mmu_object *mmu, unsigned serial)
+{
+	return true;
+}
+
 #endif
 
 struct get_pages_work {
@@ -469,8 +558,14 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	struct drm_device *dev = obj->base.dev;
 	const int num_pages = obj->base.size >> PAGE_SHIFT;
 	struct page **pvec;
+	unsigned serial;
 	int pinned, ret;
 
+	if (!i915_mmu_object_get(obj->userptr.mn, &serial)) {
+		schedule_work(&work->work);
+		return;
+	}
+
 	ret = -ENOMEM;
 	pinned = 0;
 
@@ -497,7 +592,8 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	}
 
 	mutex_lock(&dev->struct_mutex);
-	if (obj->userptr.work != &work->work) {
+	if (obj->userptr.work != &work->work ||
+	    !i915_mmu_object_put(obj->userptr.mn, serial)) {
 		ret = 0;
 	} else if (pinned == num_pages) {
 		ret = st_set_pages(&obj->pages, pvec, num_pages);
@@ -522,8 +618,9 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 static int
 i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 {
-	const int num_pages = obj->base.size >> PAGE_SHIFT;
+	unsigned num_pages = obj->base.size >> PAGE_SHIFT;
 	struct page **pvec;
+	unsigned serial;
 	int pinned, ret;
 
 	/* If userspace should engineer that these pages are replaced in
@@ -545,7 +642,8 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 
 	pvec = NULL;
 	pinned = 0;
-	if (obj->userptr.mm == current->mm) {
+	if (obj->userptr.mm == current->mm &&
+	    i915_mmu_object_get(obj->userptr.mn, &serial)) {
 		pvec = kmalloc(num_pages*sizeof(struct page *),
 			       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
 		if (pvec == NULL) {
@@ -556,6 +654,10 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 
 		pinned = __get_user_pages_fast(obj->userptr.ptr, num_pages,
 					       !obj->userptr.read_only, pvec);
+
+		/* And check that the mmu remained valid */
+		if (!i915_mmu_object_put(obj->userptr.mn, serial))
+			num_pages = ~0;
 	}
 	if (pinned < num_pages) {
 		if (pinned < 0) {
-- 
1.9.1

Re: [PATCH 2/2] drm/i915: Serialise userptr gup with mmu-notifier

[Chris Wilson]

On Thu, Jul 10, 2014 at 10:21:44AM +0100, Chris Wilson wrote:
> Jerome Glisse pointed out that get_user_pages() does not synchronize
> with concurrent invalidations of the VMA. As such if the backing vma is
> changed whilst the pages for the object are being grabbed for use by the
> GPU, we may end up with a random mixture of page references being held.
> Worse still as the mmu-notifier will believe that the VMA invalidation
> was complete and the old page references dropped.
> 
> In order to serialise gup with mmu-notifier, we use a seqlock to detect
> when an invalidation has occurred in parallel to our gup and if so cancel
> the gup. The detection is a little coarse, but hopefully we never see
> contention here!

Hmm. This is bogus. I thought the gup_fast path was racy, but is
serialised correctly with invalidate-range.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Tvrtko Ursulin]

On 07/10/2014 10:21 AM, Chris Wilson wrote:
> Whilst I strongly advise against doing so for the implicit coherency
> issues between the multiple buffer objects accessing the same backing
> store, it nevertheless is a valid use case, akin to mmaping the same
> file multiple times.
>
> The reason why we forbade it earlier was that our use of the interval
> tree for fast invalidation upon vma changes excluded overlapping
> objects. So in the case where the user wishes to create such pairs of
> overlapping objects, we degrade the range invalidation to walkin the
> linear list of objects associated with the mm.
>
> v2: Compile for mmu-notifiers after tweaking

[snip]

> +static void invalidate_range__linear(struct i915_mmu_notifier *mn,
> +				     struct mm_struct *mm,
> +				     unsigned long start,
> +				     unsigned long end)
> +{
> +	struct i915_mmu_object *mmu;
> +	unsigned long serial;
> +
> +restart:
> +	serial = mn->serial;
> +	list_for_each_entry(mmu, &mn->linear, link) {
> +		struct drm_i915_gem_object *obj;
> +
> +		if (mmu->it.last < start || mmu->it.start > end)
> +			continue;
> +
> +		obj = mmu->obj;
> +		drm_gem_object_reference(&obj->base);
> +		spin_unlock(&mn->lock);
> +
> +		cancel_userptr(obj);
> +
> +		spin_lock(&mn->lock);
> +		if (serial != mn->serial)
> +			goto restart;
> +	}
> +
> +	spin_unlock(&mn->lock);
> +}
> +
>   static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   						       struct mm_struct *mm,
>   						       unsigned long start,
> @@ -60,16 +128,19 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   {
>   	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
>   	struct interval_tree_node *it = NULL;
> +	unsigned long next = start;
>   	unsigned long serial = 0;
>
>   	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
> -	while (start < end) {
> +	while (next < end) {
>   		struct drm_i915_gem_object *obj;
>
>   		obj = NULL;
>   		spin_lock(&mn->lock);
> +		if (mn->is_linear)
> +			return invalidate_range__linear(mn, mm, start, end);

Too bad that on first overlapping object the whole process goes into 
"slow mode". I wonder what would benchmarking say to that.

Perhaps we could still use interval tree but add another layer of 
indirection where ranges would be merged for overlapping objects and 
contain a linear list of them only there?

Tvrtko

Re: [PATCH 2/2] drm/i915: Serialise userptr gup with mmu-notifier

[Tvrtko Ursulin]

On 07/10/2014 10:21 AM, Chris Wilson wrote:
> Jerome Glisse pointed out that get_user_pages() does not synchronize
> with concurrent invalidations of the VMA. As such if the backing vma is
> changed whilst the pages for the object are being grabbed for use by the
> GPU, we may end up with a random mixture of page references being held.
> Worse still as the mmu-notifier will believe that the VMA invalidation
> was complete and the old page references dropped.
>
> In order to serialise gup with mmu-notifier, we use a seqlock to detect
> when an invalidation has occurred in parallel to our gup and if so cancel
> the gup. The detection is a little coarse, but hopefully we never see
> contention here!

Looks good to me, but as we discussed on IRC all get_user_pages users 
have this "problem" and I don't understand why it matters to us? How 
would someone hit/exploit the race? By one thread manically modifying 
mappings while another is creating userptr objects? Is there some other 
legitimate way of it happening?

Tvrtko

Re: [PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Chris Wilson]

On Thu, Jul 10, 2014 at 01:26:53PM +0100, Tvrtko Ursulin wrote:
> Too bad that on first overlapping object the whole process goes into
> "slow mode". I wonder what would benchmarking say to that.
> 
> Perhaps we could still use interval tree but add another layer of
> indirection where ranges would be merged for overlapping objects and
> contain a linear list of them only there?

I balked at the thought of having to do the merger of neighbouring
intervals when a new larger object is created.

Flipping between a simple linear list and the interval-tree is tricky
enough, so I'll wait until someone complains about the performance after
using an overlapping pair.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Daniel Vetter]

On Thu, Jul 10, 2014 at 10:21:43AM +0100, Chris Wilson wrote:
> Whilst I strongly advise against doing so for the implicit coherency
> issues between the multiple buffer objects accessing the same backing
> store, it nevertheless is a valid use case, akin to mmaping the same
> file multiple times.
> 
> The reason why we forbade it earlier was that our use of the interval
> tree for fast invalidation upon vma changes excluded overlapping
> objects. So in the case where the user wishes to create such pairs of
> overlapping objects, we degrade the range invalidation to walkin the
> linear list of objects associated with the mm.
> 
> v2: Compile for mmu-notifiers after tweaking
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Li, Victor Y" <victor.y.li@intel.com>
> Cc: "Kelley, Sean V" <sean.v.kelley@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com>

The commit message is a bit thin on why we need this. Sounds a bit like
userspace lost its marbles to me ...
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 139 ++++++++++++++++++++++++--------
>  1 file changed, 104 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 21ea928..7c38f50 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -40,19 +40,87 @@ struct i915_mmu_notifier {
>  	struct hlist_node node;
>  	struct mmu_notifier mn;
>  	struct rb_root objects;
> +	struct list_head linear;
>  	struct drm_device *dev;
>  	struct mm_struct *mm;
>  	struct work_struct work;
>  	unsigned long count;
>  	unsigned long serial;
> +	bool is_linear;
>  };
>  
>  struct i915_mmu_object {
>  	struct i915_mmu_notifier *mmu;
>  	struct interval_tree_node it;
> +	struct list_head link;
>  	struct drm_i915_gem_object *obj;
> +	bool is_linear;
>  };
>  
> +static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->base.dev;
> +	unsigned long end;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	/* Cancel any active worker and force us to re-evaluate gup */
> +	obj->userptr.work = NULL;
> +
> +	if (obj->pages != NULL) {
> +		struct drm_i915_private *dev_priv = to_i915(dev);
> +		struct i915_vma *vma, *tmp;
> +		bool was_interruptible;
> +
> +		was_interruptible = dev_priv->mm.interruptible;
> +		dev_priv->mm.interruptible = false;
> +
> +		list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
> +			int ret = i915_vma_unbind(vma);
> +			WARN_ON(ret && ret != -EIO);
> +		}
> +		WARN_ON(i915_gem_object_put_pages(obj));
> +
> +		dev_priv->mm.interruptible = was_interruptible;
> +	}
> +
> +	end = obj->userptr.ptr + obj->base.size;
> +
> +	drm_gem_object_unreference(&obj->base);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return end;
> +}
> +
> +static void invalidate_range__linear(struct i915_mmu_notifier *mn,
> +				     struct mm_struct *mm,
> +				     unsigned long start,
> +				     unsigned long end)
> +{
> +	struct i915_mmu_object *mmu;
> +	unsigned long serial;
> +
> +restart:
> +	serial = mn->serial;
> +	list_for_each_entry(mmu, &mn->linear, link) {
> +		struct drm_i915_gem_object *obj;
> +
> +		if (mmu->it.last < start || mmu->it.start > end)
> +			continue;
> +
> +		obj = mmu->obj;
> +		drm_gem_object_reference(&obj->base);
> +		spin_unlock(&mn->lock);
> +
> +		cancel_userptr(obj);
> +
> +		spin_lock(&mn->lock);
> +		if (serial != mn->serial)
> +			goto restart;
> +	}
> +
> +	spin_unlock(&mn->lock);
> +}
> +
>  static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  						       struct mm_struct *mm,
>  						       unsigned long start,
> @@ -60,16 +128,19 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  {
>  	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
>  	struct interval_tree_node *it = NULL;
> +	unsigned long next = start;
>  	unsigned long serial = 0;
>  
>  	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
> -	while (start < end) {
> +	while (next < end) {
>  		struct drm_i915_gem_object *obj;
>  
>  		obj = NULL;
>  		spin_lock(&mn->lock);
> +		if (mn->is_linear)
> +			return invalidate_range__linear(mn, mm, start, end);
>  		if (serial == mn->serial)
> -			it = interval_tree_iter_next(it, start, end);
> +			it = interval_tree_iter_next(it, next, end);
>  		else
>  			it = interval_tree_iter_first(&mn->objects, start, end);
>  		if (it != NULL) {
> @@ -81,31 +152,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  		if (obj == NULL)
>  			return;
>  
> -		mutex_lock(&mn->dev->struct_mutex);
> -		/* Cancel any active worker and force us to re-evaluate gup */
> -		obj->userptr.work = NULL;
> -
> -		if (obj->pages != NULL) {
> -			struct drm_i915_private *dev_priv = to_i915(mn->dev);
> -			struct i915_vma *vma, *tmp;
> -			bool was_interruptible;
> -
> -			was_interruptible = dev_priv->mm.interruptible;
> -			dev_priv->mm.interruptible = false;
> -
> -			list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
> -				int ret = i915_vma_unbind(vma);
> -				WARN_ON(ret && ret != -EIO);
> -			}
> -			WARN_ON(i915_gem_object_put_pages(obj));
> -
> -			dev_priv->mm.interruptible = was_interruptible;
> -		}
> -
> -		start = obj->userptr.ptr + obj->base.size;
> -
> -		drm_gem_object_unreference(&obj->base);
> -		mutex_unlock(&mn->dev->struct_mutex);
> +		next = cancel_userptr(obj);
>  	}
>  }
>  
> @@ -151,6 +198,8 @@ i915_mmu_notifier_get(struct drm_device *dev, struct mm_struct *mm)
>  	mmu->objects = RB_ROOT;
>  	mmu->count = 0;
>  	mmu->serial = 0;
> +	INIT_LIST_HEAD(&mmu->linear);
> +	mmu->is_linear = false;
>  
>  	/* Protected by mmap_sem (write-lock) */
>  	ret = __mmu_notifier_register(&mmu->mn, mm);
> @@ -197,6 +246,17 @@ static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mmu)
>  		mmu->serial = 1;
>  }
>  
> +static bool i915_mmu_notifier_is_linear(struct i915_mmu_notifier *mmu)
> +{
> +	struct i915_mmu_object *mn;
> +
> +	list_for_each_entry(mn, &mmu->linear, link)
> +		if (mn->is_linear)
> +			return true;
> +
> +	return false;
> +}
> +
>  static void
>  i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
>  		      struct i915_mmu_object *mn)
> @@ -205,6 +265,9 @@ i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
>  
>  	spin_lock(&mmu->lock);
>  	interval_tree_remove(&mn->it, &mmu->objects);
> +	list_del(&mn->link);
> +	if (mn->is_linear)
> +		mmu->is_linear = i915_mmu_notifier_is_linear(mmu);
>  	__i915_mmu_notifier_update_serial(mmu);
>  	spin_unlock(&mmu->lock);
>  
> @@ -230,7 +293,6 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
>  	 */
>  	i915_gem_retire_requests(mmu->dev);
>  
> -	/* Disallow overlapping userptr objects */
>  	spin_lock(&mmu->lock);
>  	it = interval_tree_iter_first(&mmu->objects,
>  				      mn->it.start, mn->it.last);
> @@ -243,14 +305,22 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
>  		 * to flush their object references upon which the object will
>  		 * be removed from the interval-tree, or the the range is
>  		 * still in use by another client and the overlap is invalid.
> +		 *
> +		 * If we do have an overlap, we cannot use the interval tree
> +		 * for fast range invalidation.
>  		 */
>  
>  		obj = container_of(it, struct i915_mmu_object, it)->obj;
> -		ret = obj->userptr.workers ? -EAGAIN : -EINVAL;
> -	} else {
> +		if (!obj->userptr.workers)
> +			mmu->is_linear = mn->is_linear = true;
> +		else
> +			ret = -EAGAIN;
> +	} else
>  		interval_tree_insert(&mn->it, &mmu->objects);
> +
> +	if (ret == 0) {
> +		list_add(&mn->link, &mmu->linear);
>  		__i915_mmu_notifier_update_serial(mmu);
> -		ret = 0;
>  	}
>  	spin_unlock(&mmu->lock);
>  	mutex_unlock(&mmu->dev->struct_mutex);
> @@ -611,12 +681,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>   * We impose several restrictions upon the memory being mapped
>   * into the GPU.
>   * 1. It must be page aligned (both start/end addresses, i.e ptr and size).
> - * 2. It cannot overlap any other userptr object in the same address space.
> - * 3. It must be normal system memory, not a pointer into another map of IO
> + * 2. It must be normal system memory, not a pointer into another map of IO
>   *    space (e.g. it must not be a GTT mmapping of another object).
> - * 4. We only allow a bo as large as we could in theory map into the GTT,
> + * 3. We only allow a bo as large as we could in theory map into the GTT,
>   *    that is we limit the size to the total size of the GTT.
> - * 5. The bo is marked as being snoopable. The backing pages are left
> + * 4. The bo is marked as being snoopable. The backing pages are left
>   *    accessible directly by the CPU, but reads and writes by the GPU may
>   *    incur the cost of a snoop (unless you have an LLC architecture).
>   *
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Chris Wilson]

Whilst I strongly advise against doing so for the implicit coherency
issues between the multiple buffer objects accessing the same backing
store, it nevertheless is a valid use case, akin to mmaping the same
file multiple times.

The reason why we forbade it earlier was that our use of the interval
tree for fast invalidation upon vma changes excluded overlapping
objects. So in the case where the user wishes to create such pairs of
overlapping objects, we degrade the range invalidation to walkin the
linear list of objects associated with the mm.

A situation where overlapping objects could arise is the lax implementation
of MIT-SHM Pixmaps in the xserver. A second situation is where the user
wishes to have different access modes to a region of memory (e.g. access
through a read-only userptr buffer and through a normal userptr buffer).

v2: Compile for mmu-notifiers after tweaking
v3: Rename is_linear/has_linear

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Li, Victor Y" <victor.y.li@intel.com>
Cc: "Kelley, Sean V" <sean.v.kelley@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: Akash Goel <akash.goel@intel.com>
Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 142 ++++++++++++++++++++++++--------
 1 file changed, 106 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index b41614d..74c45da 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -40,19 +40,87 @@ struct i915_mmu_notifier {
 	struct hlist_node node;
 	struct mmu_notifier mn;
 	struct rb_root objects;
+	struct list_head linear;
 	struct drm_device *dev;
 	struct mm_struct *mm;
 	struct work_struct work;
 	unsigned long count;
 	unsigned long serial;
+	bool has_linear;
 };
 
 struct i915_mmu_object {
 	struct i915_mmu_notifier *mmu;
 	struct interval_tree_node it;
+	struct list_head link;
 	struct drm_i915_gem_object *obj;
+	bool is_linear;
 };
 
+static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	unsigned long end;
+
+	mutex_lock(&dev->struct_mutex);
+	/* Cancel any active worker and force us to re-evaluate gup */
+	obj->userptr.work = NULL;
+
+	if (obj->pages != NULL) {
+		struct drm_i915_private *dev_priv = to_i915(dev);
+		struct i915_vma *vma, *tmp;
+		bool was_interruptible;
+
+		was_interruptible = dev_priv->mm.interruptible;
+		dev_priv->mm.interruptible = false;
+
+		list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
+			int ret = i915_vma_unbind(vma);
+			WARN_ON(ret && ret != -EIO);
+		}
+		WARN_ON(i915_gem_object_put_pages(obj));
+
+		dev_priv->mm.interruptible = was_interruptible;
+	}
+
+	end = obj->userptr.ptr + obj->base.size;
+
+	drm_gem_object_unreference(&obj->base);
+	mutex_unlock(&dev->struct_mutex);
+
+	return end;
+}
+
+static void invalidate_range__linear(struct i915_mmu_notifier *mn,
+				     struct mm_struct *mm,
+				     unsigned long start,
+				     unsigned long end)
+{
+	struct i915_mmu_object *mmu;
+	unsigned long serial;
+
+restart:
+	serial = mn->serial;
+	list_for_each_entry(mmu, &mn->linear, link) {
+		struct drm_i915_gem_object *obj;
+
+		if (mmu->it.last < start || mmu->it.start > end)
+			continue;
+
+		obj = mmu->obj;
+		drm_gem_object_reference(&obj->base);
+		spin_unlock(&mn->lock);
+
+		cancel_userptr(obj);
+
+		spin_lock(&mn->lock);
+		if (serial != mn->serial)
+			goto restart;
+	}
+
+	spin_unlock(&mn->lock);
+}
+
 static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 						       struct mm_struct *mm,
 						       unsigned long start,
@@ -60,16 +128,19 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 {
 	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
 	struct interval_tree_node *it = NULL;
+	unsigned long next = start;
 	unsigned long serial = 0;
 
 	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
-	while (start < end) {
+	while (next < end) {
 		struct drm_i915_gem_object *obj;
 
 		obj = NULL;
 		spin_lock(&mn->lock);
+		if (mn->has_linear)
+			return invalidate_range__linear(mn, mm, start, end);
 		if (serial == mn->serial)
-			it = interval_tree_iter_next(it, start, end);
+			it = interval_tree_iter_next(it, next, end);
 		else
 			it = interval_tree_iter_first(&mn->objects, start, end);
 		if (it != NULL) {
@@ -81,31 +152,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		if (obj == NULL)
 			return;
 
-		mutex_lock(&mn->dev->struct_mutex);
-		/* Cancel any active worker and force us to re-evaluate gup */
-		obj->userptr.work = NULL;
-
-		if (obj->pages != NULL) {
-			struct drm_i915_private *dev_priv = to_i915(mn->dev);
-			struct i915_vma *vma, *tmp;
-			bool was_interruptible;
-
-			was_interruptible = dev_priv->mm.interruptible;
-			dev_priv->mm.interruptible = false;
-
-			list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
-				int ret = i915_vma_unbind(vma);
-				WARN_ON(ret && ret != -EIO);
-			}
-			WARN_ON(i915_gem_object_put_pages(obj));
-
-			dev_priv->mm.interruptible = was_interruptible;
-		}
-
-		start = obj->userptr.ptr + obj->base.size;
-
-		drm_gem_object_unreference(&obj->base);
-		mutex_unlock(&mn->dev->struct_mutex);
+		next = cancel_userptr(obj);
 	}
 }
 
@@ -151,6 +198,8 @@ i915_mmu_notifier_get(struct drm_device *dev, struct mm_struct *mm)
 	mmu->objects = RB_ROOT;
 	mmu->count = 0;
 	mmu->serial = 1;
+	INIT_LIST_HEAD(&mmu->linear);
+	mmu->has_linear = false;
 
 	/* Protected by mmap_sem (write-lock) */
 	ret = __mmu_notifier_register(&mmu->mn, mm);
@@ -197,6 +246,17 @@ static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mmu)
 		mmu->serial = 1;
 }
 
+static bool i915_mmu_notifier_has_linear(struct i915_mmu_notifier *mmu)
+{
+	struct i915_mmu_object *mn;
+
+	list_for_each_entry(mn, &mmu->linear, link)
+		if (mn->is_linear)
+			return true;
+
+	return false;
+}
+
 static void
 i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
 		      struct i915_mmu_object *mn)
@@ -204,7 +264,11 @@ i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
 	lockdep_assert_held(&mmu->dev->struct_mutex);
 
 	spin_lock(&mmu->lock);
-	interval_tree_remove(&mn->it, &mmu->objects);
+	list_del(&mn->link);
+	if (mn->is_linear)
+		mmu->has_linear = i915_mmu_notifier_has_linear(mmu);
+	else
+		interval_tree_remove(&mn->it, &mmu->objects);
 	__i915_mmu_notifier_update_serial(mmu);
 	spin_unlock(&mmu->lock);
 
@@ -230,7 +294,6 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
 	 */
 	i915_gem_retire_requests(mmu->dev);
 
-	/* Disallow overlapping userptr objects */
 	spin_lock(&mmu->lock);
 	it = interval_tree_iter_first(&mmu->objects,
 				      mn->it.start, mn->it.last);
@@ -243,14 +306,22 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
 		 * to flush their object references upon which the object will
 		 * be removed from the interval-tree, or the the range is
 		 * still in use by another client and the overlap is invalid.
+		 *
+		 * If we do have an overlap, we cannot use the interval tree
+		 * for fast range invalidation.
 		 */
 
 		obj = container_of(it, struct i915_mmu_object, it)->obj;
-		ret = obj->userptr.workers ? -EAGAIN : -EINVAL;
-	} else {
+		if (!obj->userptr.workers)
+			mmu->has_linear = mn->is_linear = true;
+		else
+			ret = -EAGAIN;
+	} else
 		interval_tree_insert(&mn->it, &mmu->objects);
+
+	if (ret == 0) {
+		list_add(&mn->link, &mmu->linear);
 		__i915_mmu_notifier_update_serial(mmu);
-		ret = 0;
 	}
 	spin_unlock(&mmu->lock);
 	mutex_unlock(&mmu->dev->struct_mutex);
@@ -611,12 +682,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
  * We impose several restrictions upon the memory being mapped
  * into the GPU.
  * 1. It must be page aligned (both start/end addresses, i.e ptr and size).
- * 2. It cannot overlap any other userptr object in the same address space.
- * 3. It must be normal system memory, not a pointer into another map of IO
+ * 2. It must be normal system memory, not a pointer into another map of IO
  *    space (e.g. it must not be a GTT mmapping of another object).
- * 4. We only allow a bo as large as we could in theory map into the GTT,
+ * 3. We only allow a bo as large as we could in theory map into the GTT,
  *    that is we limit the size to the total size of the GTT.
- * 5. The bo is marked as being snoopable. The backing pages are left
+ * 4. The bo is marked as being snoopable. The backing pages are left
  *    accessible directly by the CPU, but reads and writes by the GPU may
  *    incur the cost of a snoop (unless you have an LLC architecture).
  *
-- 
1.9.1

Re: [PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Tvrtko Ursulin]

On 07/21/2014 01:21 PM, Chris Wilson wrote:
> Whilst I strongly advise against doing so for the implicit coherency
> issues between the multiple buffer objects accessing the same backing
> store, it nevertheless is a valid use case, akin to mmaping the same
> file multiple times.
>
> The reason why we forbade it earlier was that our use of the interval
> tree for fast invalidation upon vma changes excluded overlapping
> objects. So in the case where the user wishes to create such pairs of
> overlapping objects, we degrade the range invalidation to walkin the
> linear list of objects associated with the mm.
>
> A situation where overlapping objects could arise is the lax implementation
> of MIT-SHM Pixmaps in the xserver. A second situation is where the user
> wishes to have different access modes to a region of memory (e.g. access
> through a read-only userptr buffer and through a normal userptr buffer).
>
> v2: Compile for mmu-notifiers after tweaking
> v3: Rename is_linear/has_linear
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Li, Victor Y" <victor.y.li@intel.com>
> Cc: "Kelley, Sean V" <sean.v.kelley@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 142 ++++++++++++++++++++++++--------
>   1 file changed, 106 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index b41614d..74c45da 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -40,19 +40,87 @@ struct i915_mmu_notifier {
>   	struct hlist_node node;
>   	struct mmu_notifier mn;
>   	struct rb_root objects;
> +	struct list_head linear;
>   	struct drm_device *dev;
>   	struct mm_struct *mm;
>   	struct work_struct work;
>   	unsigned long count;
>   	unsigned long serial;
> +	bool has_linear;
>   };
>
>   struct i915_mmu_object {
>   	struct i915_mmu_notifier *mmu;
>   	struct interval_tree_node it;
> +	struct list_head link;
>   	struct drm_i915_gem_object *obj;
> +	bool is_linear;
>   };
>
> +static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->base.dev;
> +	unsigned long end;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	/* Cancel any active worker and force us to re-evaluate gup */
> +	obj->userptr.work = NULL;
> +
> +	if (obj->pages != NULL) {
> +		struct drm_i915_private *dev_priv = to_i915(dev);
> +		struct i915_vma *vma, *tmp;
> +		bool was_interruptible;
> +
> +		was_interruptible = dev_priv->mm.interruptible;
> +		dev_priv->mm.interruptible = false;
> +
> +		list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
> +			int ret = i915_vma_unbind(vma);
> +			WARN_ON(ret && ret != -EIO);
> +		}
> +		WARN_ON(i915_gem_object_put_pages(obj));
> +
> +		dev_priv->mm.interruptible = was_interruptible;
> +	}
> +
> +	end = obj->userptr.ptr + obj->base.size;
> +
> +	drm_gem_object_unreference(&obj->base);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return end;
> +}
> +
> +static void invalidate_range__linear(struct i915_mmu_notifier *mn,
> +				     struct mm_struct *mm,
> +				     unsigned long start,
> +				     unsigned long end)
> +{
> +	struct i915_mmu_object *mmu;
> +	unsigned long serial;
> +
> +restart:
> +	serial = mn->serial;
> +	list_for_each_entry(mmu, &mn->linear, link) {
> +		struct drm_i915_gem_object *obj;
> +
> +		if (mmu->it.last < start || mmu->it.start > end)
> +			continue;
> +
> +		obj = mmu->obj;
> +		drm_gem_object_reference(&obj->base);
> +		spin_unlock(&mn->lock);
> +
> +		cancel_userptr(obj);
> +
> +		spin_lock(&mn->lock);
> +		if (serial != mn->serial)
> +			goto restart;
> +	}
> +
> +	spin_unlock(&mn->lock);
> +}
> +
>   static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   						       struct mm_struct *mm,
>   						       unsigned long start,
> @@ -60,16 +128,19 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   {
>   	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
>   	struct interval_tree_node *it = NULL;
> +	unsigned long next = start;
>   	unsigned long serial = 0;
>
>   	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
> -	while (start < end) {
> +	while (next < end) {
>   		struct drm_i915_gem_object *obj;
>
>   		obj = NULL;
>   		spin_lock(&mn->lock);
> +		if (mn->has_linear)
> +			return invalidate_range__linear(mn, mm, start, end);
>   		if (serial == mn->serial)
> -			it = interval_tree_iter_next(it, start, end);
> +			it = interval_tree_iter_next(it, next, end);
>   		else
>   			it = interval_tree_iter_first(&mn->objects, start, end);
>   		if (it != NULL) {
> @@ -81,31 +152,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   		if (obj == NULL)
>   			return;
>
> -		mutex_lock(&mn->dev->struct_mutex);
> -		/* Cancel any active worker and force us to re-evaluate gup */
> -		obj->userptr.work = NULL;
> -
> -		if (obj->pages != NULL) {
> -			struct drm_i915_private *dev_priv = to_i915(mn->dev);
> -			struct i915_vma *vma, *tmp;
> -			bool was_interruptible;
> -
> -			was_interruptible = dev_priv->mm.interruptible;
> -			dev_priv->mm.interruptible = false;
> -
> -			list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
> -				int ret = i915_vma_unbind(vma);
> -				WARN_ON(ret && ret != -EIO);
> -			}
> -			WARN_ON(i915_gem_object_put_pages(obj));
> -
> -			dev_priv->mm.interruptible = was_interruptible;
> -		}
> -
> -		start = obj->userptr.ptr + obj->base.size;
> -
> -		drm_gem_object_unreference(&obj->base);
> -		mutex_unlock(&mn->dev->struct_mutex);
> +		next = cancel_userptr(obj);
>   	}
>   }
>
> @@ -151,6 +198,8 @@ i915_mmu_notifier_get(struct drm_device *dev, struct mm_struct *mm)
>   	mmu->objects = RB_ROOT;
>   	mmu->count = 0;
>   	mmu->serial = 1;
> +	INIT_LIST_HEAD(&mmu->linear);
> +	mmu->has_linear = false;
>
>   	/* Protected by mmap_sem (write-lock) */
>   	ret = __mmu_notifier_register(&mmu->mn, mm);
> @@ -197,6 +246,17 @@ static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mmu)
>   		mmu->serial = 1;
>   }
>
> +static bool i915_mmu_notifier_has_linear(struct i915_mmu_notifier *mmu)
> +{
> +	struct i915_mmu_object *mn;
> +
> +	list_for_each_entry(mn, &mmu->linear, link)
> +		if (mn->is_linear)
> +			return true;
> +
> +	return false;
> +}
> +
>   static void
>   i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
>   		      struct i915_mmu_object *mn)
> @@ -204,7 +264,11 @@ i915_mmu_notifier_del(struct i915_mmu_notifier *mmu,
>   	lockdep_assert_held(&mmu->dev->struct_mutex);
>
>   	spin_lock(&mmu->lock);
> -	interval_tree_remove(&mn->it, &mmu->objects);
> +	list_del(&mn->link);
> +	if (mn->is_linear)
> +		mmu->has_linear = i915_mmu_notifier_has_linear(mmu);
> +	else
> +		interval_tree_remove(&mn->it, &mmu->objects);
>   	__i915_mmu_notifier_update_serial(mmu);
>   	spin_unlock(&mmu->lock);
>
> @@ -230,7 +294,6 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
>   	 */
>   	i915_gem_retire_requests(mmu->dev);
>
> -	/* Disallow overlapping userptr objects */
>   	spin_lock(&mmu->lock);
>   	it = interval_tree_iter_first(&mmu->objects,
>   				      mn->it.start, mn->it.last);
> @@ -243,14 +306,22 @@ i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
>   		 * to flush their object references upon which the object will
>   		 * be removed from the interval-tree, or the the range is
>   		 * still in use by another client and the overlap is invalid.
> +		 *
> +		 * If we do have an overlap, we cannot use the interval tree
> +		 * for fast range invalidation.
>   		 */
>
>   		obj = container_of(it, struct i915_mmu_object, it)->obj;
> -		ret = obj->userptr.workers ? -EAGAIN : -EINVAL;
> -	} else {
> +		if (!obj->userptr.workers)
> +			mmu->has_linear = mn->is_linear = true;
> +		else
> +			ret = -EAGAIN;
> +	} else
>   		interval_tree_insert(&mn->it, &mmu->objects);
> +
> +	if (ret == 0) {
> +		list_add(&mn->link, &mmu->linear);
>   		__i915_mmu_notifier_update_serial(mmu);
> -		ret = 0;
>   	}
>   	spin_unlock(&mmu->lock);
>   	mutex_unlock(&mmu->dev->struct_mutex);
> @@ -611,12 +682,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>    * We impose several restrictions upon the memory being mapped
>    * into the GPU.
>    * 1. It must be page aligned (both start/end addresses, i.e ptr and size).
> - * 2. It cannot overlap any other userptr object in the same address space.
> - * 3. It must be normal system memory, not a pointer into another map of IO
> + * 2. It must be normal system memory, not a pointer into another map of IO
>    *    space (e.g. it must not be a GTT mmapping of another object).
> - * 4. We only allow a bo as large as we could in theory map into the GTT,
> + * 3. We only allow a bo as large as we could in theory map into the GTT,
>    *    that is we limit the size to the total size of the GTT.
> - * 5. The bo is marked as being snoopable. The backing pages are left
> + * 4. The bo is marked as being snoopable. The backing pages are left
>    *    accessible directly by the CPU, but reads and writes by the GPU may
>    *    incur the cost of a snoop (unless you have an LLC architecture).
>    *

Looks fine. Performance impact is potentially big as we discussed but I 
suppose we can leave that for later if an issue. So:

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

I think it would be good to add some more tests to cover the tracking 
"handover" between the interval tree and linear list to ensure 
invalidation still works correctly in non-trivial cases. Code looks 
correct in that respect but just in case. It is not a top priority so 
not sure when I'll find time to actually do it.

Tvrtko

Re: [PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Daniel Vetter]

On Wed, Jul 23, 2014 at 02:23:53PM +0100, Tvrtko Ursulin wrote:
> Looks fine. Performance impact is potentially big as we discussed but I
> suppose we can leave that for later if an issue. So:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Merged, thanks for patch and review.

> I think it would be good to add some more tests to cover the tracking
> "handover" between the interval tree and linear list to ensure invalidation
> still works correctly in non-trivial cases. Code looks correct in that
> respect but just in case. It is not a top priority so not sure when I'll
> find time to actually do it.

We don't yet have some tests with overlapping allocations? I think at
least some basic smoke test should be there ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Allow overlapping userptr objects

[Tvrtko Ursulin]

On 07/23/2014 03:34 PM, Daniel Vetter wrote:
> On Wed, Jul 23, 2014 at 02:23:53PM +0100, Tvrtko Ursulin wrote:
>> I think it would be good to add some more tests to cover the tracking
>> "handover" between the interval tree and linear list to ensure invalidation
>> still works correctly in non-trivial cases. Code looks correct in that
>> respect but just in case. It is not a top priority so not sure when I'll
>> find time to actually do it.
>
> We don't yet have some tests with overlapping allocations? I think at
> least some basic smoke test should be there ...

We do have basic overlapping tests. I was talking about adding some more 
to exercise specific kernel paths which this patch adds. It switches 
between using a linear list and an interval tree to track ranges so just 
to make sure state remains correct under such transitions.

But you also reminded me now, IGT needs to be told overalap is now 
allowed...

Tvrtko