public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns
@ 2016-04-03 17:14 Chris Wilson
  2016-04-03 17:14 ` [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Chris Wilson @ 2016-04-03 17:14 UTC (permalink / raw)
  To: intel-gfx

In order to ensure that all invalidations are completed before the
operation returns to userspace (i.e. before the mmap() syscall returns)
we need to flush the outstanding operations. To this end, we submit all
the per-object invalidation work to a private workqueue and then flush
the workqueue at the end of the function. We are allowed to block inside
invalidate_range_start, and struct_mutex is the inner lock so the worker
is not hiding any lock inversions. The advantage of using the workqueue
over direct calling of cancel_userptr() is that it allows us to
parallelise the potential waits and unpinning of large objects.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94699
Testcase: igt/gem_userptr_blits/sync-unmap-cycles
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 48 ++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 291a9393493d..92b39186b05a 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -49,6 +49,7 @@ struct i915_mmu_notifier {
 	struct hlist_node node;
 	struct mmu_notifier mn;
 	struct rb_root objects;
+	struct workqueue_struct *wq;
 };
 
 struct i915_mmu_object {
@@ -60,6 +61,40 @@ struct i915_mmu_object {
 	bool attached;
 };
 
+static void wait_rendering(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_gem_request *requests[I915_NUM_ENGINES];
+	unsigned reset_counter;
+	int i, n;
+
+	if (!obj->active)
+		return;
+
+	n = 0;
+	for (i = 0; i < I915_NUM_ENGINES; i++) {
+		struct drm_i915_gem_request *req;
+
+		req = obj->last_read_req[i];
+		if (req == NULL)
+			continue;
+
+		requests[n++] = i915_gem_request_reference(req);
+	}
+
+	reset_counter = atomic_read(&to_i915(dev)->gpu_error.reset_counter);
+	mutex_unlock(&dev->struct_mutex);
+
+	for (i = 0; i < n; i++)
+		__i915_wait_request(requests[i], reset_counter, false,
+				    NULL, NULL);
+
+	mutex_lock(&dev->struct_mutex);
+
+	for (i = 0; i < n; i++)
+		i915_gem_request_unreference(requests[i]);
+}
+
 static void cancel_userptr(struct work_struct *work)
 {
 	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
@@ -75,6 +110,8 @@ static void cancel_userptr(struct work_struct *work)
 		struct i915_vma *vma, *tmp;
 		bool was_interruptible;
 
+		wait_rendering(obj);
+
 		was_interruptible = dev_priv->mm.interruptible;
 		dev_priv->mm.interruptible = false;
 
@@ -140,7 +177,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		 */
 		mo = container_of(it, struct i915_mmu_object, it);
 		if (kref_get_unless_zero(&mo->obj->base.refcount))
-			schedule_work(&mo->work);
+			queue_work(mn->wq, &mo->work);
 
 		list_add(&mo->link, &cancelled);
 		it = interval_tree_iter_next(it, start, end);
@@ -148,6 +185,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 	list_for_each_entry(mo, &cancelled, link)
 		del_object(mo);
 	spin_unlock(&mn->lock);
+
+	flush_workqueue(mn->wq);
 }
 
 static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
@@ -167,10 +206,16 @@ i915_mmu_notifier_create(struct mm_struct *mm)
 	spin_lock_init(&mn->lock);
 	mn->mn.ops = &i915_gem_userptr_notifier;
 	mn->objects = RB_ROOT;
+	mn->wq = alloc_workqueue("i915-userptr-release", WQ_UNBOUND, 0);
+	if (mn->wq == NULL) {
+		kfree(mn);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	 /* Protected by mmap_sem (write-lock) */
 	ret = __mmu_notifier_register(&mn->mn, mm);
 	if (ret) {
+		destroy_workqueue(mn->wq);
 		kfree(mn);
 		return ERR_PTR(ret);
 	}
@@ -256,6 +301,7 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
 		return;
 
 	mmu_notifier_unregister(&mn->mn, mm);
+	destroy_workqueue(mn->wq);
 	kfree(mn);
 }
 
-- 
2.8.0.rc3

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages
  2016-04-03 17:14 [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Chris Wilson
@ 2016-04-03 17:14 ` Chris Wilson
  2016-04-03 21:11   ` Michał Winiarski
  2016-04-05 12:22   ` [Intel-gfx] " Tvrtko Ursulin
  2016-04-03 17:14 ` [PATCH 3/3] drm/i915: Store i915 pointer for userptr i915_mm_struct Chris Wilson
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 14+ messages in thread
From: Chris Wilson @ 2016-04-03 17:14 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson, Tvrtko Ursulin, Michał Winiarski, stable

Holding a reference to the containing task_struct is not sufficient to
prevent the mm_struct from being reaped under memory pressure. If this
happens whilst we are calling get_user_pages(), explosions errupt -
sometimes an immediate GPF, sometimes page flag corruption. To prevent
the target mm from being reaped as we are reading from it, acquire a
reference before we begin.

Testcase: igt/gem_shrink/*userptr
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 92b39186b05a..960bb37f458f 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -547,19 +547,24 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 
-		down_read(&mm->mmap_sem);
-		while (pinned < npages) {
-			ret = get_user_pages_remote(work->task, mm,
-					obj->userptr.ptr + pinned * PAGE_SIZE,
-					npages - pinned,
-					!obj->userptr.read_only, 0,
-					pvec + pinned, NULL);
-			if (ret < 0)
-				break;
-
-			pinned += ret;
+		ret = -EFAULT;
+		if (atomic_inc_not_zero(&mm->mm_users)) {
+			down_read(&mm->mmap_sem);
+			while (pinned < npages) {
+				ret = get_user_pages_remote
+					(work->task, mm,
+					 obj->userptr.ptr + pinned * PAGE_SIZE,
+					 npages - pinned,
+					 !obj->userptr.read_only, 0,
+					 pvec + pinned, NULL);
+				if (ret < 0)
+					break;
+
+				pinned += ret;
+			}
+			up_read(&mm->mmap_sem);
+			mmput(mm);
 		}
-		up_read(&mm->mmap_sem);
 	}
 
 	mutex_lock(&dev->struct_mutex);
-- 
2.8.0.rc3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] drm/i915: Store i915 pointer for userptr i915_mm_struct
  2016-04-03 17:14 [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Chris Wilson
  2016-04-03 17:14 ` [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
@ 2016-04-03 17:14 ` Chris Wilson
  2016-04-03 21:12   ` Michał Winiarski
  2016-04-04  6:27 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Patchwork
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2016-04-03 17:14 UTC (permalink / raw)
  To: intel-gfx

Since we only ever use the drm_i915_private from the stored
i915_mm_struct->dev, save some electons by storing the right
backpointer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 960bb37f458f..cb41919063d2 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -34,7 +34,7 @@
 
 struct i915_mm_struct {
 	struct mm_struct *mm;
-	struct drm_device *dev;
+	struct drm_i915_private *i915;
 	struct i915_mmu_notifier *mn;
 	struct hlist_node node;
 	struct kref kref;
@@ -250,13 +250,13 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 		return mn;
 
 	down_write(&mm->mm->mmap_sem);
-	mutex_lock(&to_i915(mm->dev)->mm_lock);
+	mutex_lock(&mm->i915->mm_lock);
 	if ((mn = mm->mn) == NULL) {
 		mn = i915_mmu_notifier_create(mm->mm);
 		if (!IS_ERR(mn))
 			mm->mn = mn;
 	}
-	mutex_unlock(&to_i915(mm->dev)->mm_lock);
+	mutex_unlock(&mm->i915->mm_lock);
 	up_write(&mm->mm->mmap_sem);
 
 	return mn;
@@ -373,7 +373,7 @@ i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
 		}
 
 		kref_init(&mm->kref);
-		mm->dev = obj->base.dev;
+		mm->i915 = to_i915(obj->base.dev);
 
 		mm->mm = current->mm;
 		atomic_inc(&current->mm->mm_count);
@@ -408,7 +408,7 @@ __i915_mm_struct_free(struct kref *kref)
 
 	/* Protected by dev_priv->mm_lock */
 	hash_del(&mm->node);
-	mutex_unlock(&to_i915(mm->dev)->mm_lock);
+	mutex_unlock(&mm->i915->mm_lock);
 
 	INIT_WORK(&mm->work, __i915_mm_struct_free__worker);
 	schedule_work(&mm->work);
-- 
2.8.0.rc3

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages
  2016-04-03 17:14 ` [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
@ 2016-04-03 21:11   ` Michał Winiarski
  2016-04-05 12:22   ` [Intel-gfx] " Tvrtko Ursulin
  1 sibling, 0 replies; 14+ messages in thread
From: Michał Winiarski @ 2016-04-03 21:11 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, stable

On Sun, Apr 03, 2016 at 06:14:06PM +0100, Chris Wilson wrote:
> Holding a reference to the containing task_struct is not sufficient to
> prevent the mm_struct from being reaped under memory pressure. If this
> happens whilst we are calling get_user_pages(), explosions errupt -
> sometimes an immediate GPF, sometimes page flag corruption. To prevent
> the target mm from being reaped as we are reading from it, acquire a
> reference before we begin.
> 
> Testcase: igt/gem_shrink/*userptr
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] drm/i915: Store i915 pointer for userptr i915_mm_struct
  2016-04-03 17:14 ` [PATCH 3/3] drm/i915: Store i915 pointer for userptr i915_mm_struct Chris Wilson
@ 2016-04-03 21:12   ` Michał Winiarski
  0 siblings, 0 replies; 14+ messages in thread
From: Michał Winiarski @ 2016-04-03 21:12 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Sun, Apr 03, 2016 at 06:14:07PM +0100, Chris Wilson wrote:
> Since we only ever use the drm_i915_private from the stored
> i915_mm_struct->dev, save some electons by storing the right
> backpointer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>

s/electons/electrons ;)

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns
  2016-04-03 17:14 [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Chris Wilson
  2016-04-03 17:14 ` [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
  2016-04-03 17:14 ` [PATCH 3/3] drm/i915: Store i915 pointer for userptr i915_mm_struct Chris Wilson
@ 2016-04-04  6:27 ` Patchwork
  2016-04-05 12:05 ` [PATCH 1/3] " Tvrtko Ursulin
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2016-04-04  6:27 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns
URL   : https://patchwork.freedesktop.org/series/5240/
State : success

== Summary ==

Series 5240v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5240/revisions/1/mbox/

Test drv_hangman:
        Subgroup error-state-basic:
                fail       -> PASS       (ilk-hp8440p)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:159  dwarn:0   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1783/

5633965ef152b136c22fe9858723be52116f8686 drm-intel-nightly: 2016y-04m-03d-14h-10m-49s UTC integration manifest
1b04d2337e24f073d462756f8a436a45a5e56d0a drm/i915: Store i915 pointer for userptr i915_mm_struct
76a09a33791a45a9b04c6cef416ff32dcc4df8d6 drm/i915/userptr: Hold mmref whilst calling get-user-pages
95f732b120f4073018e359adc26cff1194b2653d drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns
  2016-04-03 17:14 [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Chris Wilson
                   ` (2 preceding siblings ...)
  2016-04-04  6:27 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Patchwork
@ 2016-04-05 12:05 ` Tvrtko Ursulin
  2016-04-05 12:34   ` Chris Wilson
  2016-04-05 13:59 ` [PATCH v2 " Chris Wilson
  2016-04-05 16:57 ` ✗ Fi.CI.BAT: failure for series starting with [v2,3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct (rev4) Patchwork
  5 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-04-05 12:05 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 03/04/16 18:14, Chris Wilson wrote:
> In order to ensure that all invalidations are completed before the
> operation returns to userspace (i.e. before the mmap() syscall returns)
> we need to flush the outstanding operations. To this end, we submit all
> the per-object invalidation work to a private workqueue and then flush
> the workqueue at the end of the function. We are allowed to block inside
> invalidate_range_start, and struct_mutex is the inner lock so the worker
> is not hiding any lock inversions. The advantage of using the workqueue
> over direct calling of cancel_userptr() is that it allows us to
> parallelise the potential waits and unpinning of large objects.

There was no direct calling, but running it from a shared wq which is 
now private, per task->mm.

But it sounds so obvious now that the mmu notifier needs to wait for 
cancel_userptr workers to complete that I wonder how we missed that 
until now.

I would also spell out the two new bits explicitly in the commit: 
waiting for workers and waiting on rendering.

With some more description for the latter - why it is needed? 
i915_vma_unbind would be doing a flavour of waiting already.

Otherwise I did not find any issues with the code itself, just these 
high level questions.

Regards,

Tvrtko

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94699
> Testcase: igt/gem_userptr_blits/sync-unmap-cycles
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 48 ++++++++++++++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 291a9393493d..92b39186b05a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -49,6 +49,7 @@ struct i915_mmu_notifier {
>   	struct hlist_node node;
>   	struct mmu_notifier mn;
>   	struct rb_root objects;
> +	struct workqueue_struct *wq;
>   };
>
>   struct i915_mmu_object {
> @@ -60,6 +61,40 @@ struct i915_mmu_object {
>   	bool attached;
>   };
>
> +static void wait_rendering(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->base.dev;
> +	struct drm_i915_gem_request *requests[I915_NUM_ENGINES];
> +	unsigned reset_counter;
> +	int i, n;
> +
> +	if (!obj->active)
> +		return;
> +
> +	n = 0;
> +	for (i = 0; i < I915_NUM_ENGINES; i++) {
> +		struct drm_i915_gem_request *req;
> +
> +		req = obj->last_read_req[i];
> +		if (req == NULL)
> +			continue;
> +
> +		requests[n++] = i915_gem_request_reference(req);
> +	}
> +
> +	reset_counter = atomic_read(&to_i915(dev)->gpu_error.reset_counter);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	for (i = 0; i < n; i++)
> +		__i915_wait_request(requests[i], reset_counter, false,
> +				    NULL, NULL);
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	for (i = 0; i < n; i++)
> +		i915_gem_request_unreference(requests[i]);
> +}
> +
>   static void cancel_userptr(struct work_struct *work)
>   {
>   	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> @@ -75,6 +110,8 @@ static void cancel_userptr(struct work_struct *work)
>   		struct i915_vma *vma, *tmp;
>   		bool was_interruptible;
>
> +		wait_rendering(obj);
> +
>   		was_interruptible = dev_priv->mm.interruptible;
>   		dev_priv->mm.interruptible = false;
>
> @@ -140,7 +177,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   		 */
>   		mo = container_of(it, struct i915_mmu_object, it);
>   		if (kref_get_unless_zero(&mo->obj->base.refcount))
> -			schedule_work(&mo->work);
> +			queue_work(mn->wq, &mo->work);
>
>   		list_add(&mo->link, &cancelled);
>   		it = interval_tree_iter_next(it, start, end);
> @@ -148,6 +185,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   	list_for_each_entry(mo, &cancelled, link)
>   		del_object(mo);
>   	spin_unlock(&mn->lock);
> +
> +	flush_workqueue(mn->wq);
>   }
>
>   static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
> @@ -167,10 +206,16 @@ i915_mmu_notifier_create(struct mm_struct *mm)
>   	spin_lock_init(&mn->lock);
>   	mn->mn.ops = &i915_gem_userptr_notifier;
>   	mn->objects = RB_ROOT;
> +	mn->wq = alloc_workqueue("i915-userptr-release", WQ_UNBOUND, 0);
> +	if (mn->wq == NULL) {
> +		kfree(mn);
> +		return ERR_PTR(-ENOMEM);
> +	}
>
>   	 /* Protected by mmap_sem (write-lock) */
>   	ret = __mmu_notifier_register(&mn->mn, mm);
>   	if (ret) {
> +		destroy_workqueue(mn->wq);
>   		kfree(mn);
>   		return ERR_PTR(ret);
>   	}
> @@ -256,6 +301,7 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
>   		return;
>
>   	mmu_notifier_unregister(&mn->mn, mm);
> +	destroy_workqueue(mn->wq);
>   	kfree(mn);
>   }
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages
  2016-04-03 17:14 ` [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
  2016-04-03 21:11   ` Michał Winiarski
@ 2016-04-05 12:22   ` Tvrtko Ursulin
  1 sibling, 0 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-04-05 12:22 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 03/04/16 18:14, Chris Wilson wrote:
> Holding a reference to the containing task_struct is not sufficient to
> prevent the mm_struct from being reaped under memory pressure. If this
> happens whilst we are calling get_user_pages(), explosions errupt -
> sometimes an immediate GPF, sometimes page flag corruption. To prevent
> the target mm from being reaped as we are reading from it, acquire a
> reference before we begin.
>
> Testcase: igt/gem_shrink/*userptr
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++------------
>   1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 92b39186b05a..960bb37f458f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -547,19 +547,24 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	if (pvec != NULL) {
>   		struct mm_struct *mm = obj->userptr.mm->mm;
>
> -		down_read(&mm->mmap_sem);
> -		while (pinned < npages) {
> -			ret = get_user_pages_remote(work->task, mm,
> -					obj->userptr.ptr + pinned * PAGE_SIZE,
> -					npages - pinned,
> -					!obj->userptr.read_only, 0,
> -					pvec + pinned, NULL);
> -			if (ret < 0)
> -				break;
> -
> -			pinned += ret;
> +		ret = -EFAULT;
> +		if (atomic_inc_not_zero(&mm->mm_users)) {
> +			down_read(&mm->mmap_sem);
> +			while (pinned < npages) {
> +				ret = get_user_pages_remote
> +					(work->task, mm,
> +					 obj->userptr.ptr + pinned * PAGE_SIZE,
> +					 npages - pinned,
> +					 !obj->userptr.read_only, 0,
> +					 pvec + pinned, NULL);
> +				if (ret < 0)
> +					break;
> +
> +				pinned += ret;
> +			}
> +			up_read(&mm->mmap_sem);
> +			mmput(mm);
>   		}
> -		up_read(&mm->mmap_sem);
>   	}
>
>   	mutex_lock(&dev->struct_mutex);
>

Strange, doesn't this mean that the atomic_inc(&current->mm->mm_count) 
is not doing what we thought it would?

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns
  2016-04-05 12:05 ` [PATCH 1/3] " Tvrtko Ursulin
@ 2016-04-05 12:34   ` Chris Wilson
  0 siblings, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2016-04-05 12:34 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Tue, Apr 05, 2016 at 01:05:05PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/04/16 18:14, Chris Wilson wrote:
> >In order to ensure that all invalidations are completed before the
> >operation returns to userspace (i.e. before the mmap() syscall returns)
> >we need to flush the outstanding operations. To this end, we submit all
> >the per-object invalidation work to a private workqueue and then flush
> >the workqueue at the end of the function. We are allowed to block inside
> >invalidate_range_start, and struct_mutex is the inner lock so the worker
> >is not hiding any lock inversions. The advantage of using the workqueue
> >over direct calling of cancel_userptr() is that it allows us to
> >parallelise the potential waits and unpinning of large objects.
> 
> There was no direct calling, but running it from a shared wq which
> is now private, per task->mm.

I meant keep using a wq as opposed to changing the invalidate_range_begin to
call cancel_userptr() directly.

> But it sounds so obvious now that the mmu notifier needs to wait for
> cancel_userptr workers to complete that I wonder how we missed that
> until now.
> 
> I would also spell out the two new bits explicitly in the commit:
> waiting for workers and waiting on rendering.

Ok, deleted a couple of attempts already. Will get a cup of tea and try
and write something eloquent.

> With some more description for the latter - why it is needed?
> i915_vma_unbind would be doing a flavour of waiting already.

It allows us to drop the lock whilst waiting for outstanding rendering,
to try and prevent contention between munmap in one process and execbuf
in another. Later on we can reduce the lock here further.
-Chris

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns
  2016-04-03 17:14 [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Chris Wilson
                   ` (3 preceding siblings ...)
  2016-04-05 12:05 ` [PATCH 1/3] " Tvrtko Ursulin
@ 2016-04-05 13:59 ` Chris Wilson
  2016-04-05 14:00   ` [PATCH v2 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
                     ` (2 more replies)
  2016-04-05 16:57 ` ✗ Fi.CI.BAT: failure for series starting with [v2,3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct (rev4) Patchwork
  5 siblings, 3 replies; 14+ messages in thread
From: Chris Wilson @ 2016-04-05 13:59 UTC (permalink / raw)
  To: intel-gfx

In order to ensure that all invalidations are completed before the
operation returns to userspace (i.e. before the munmap() syscall returns)
we need to wait upon the outstanding operations.

We are allowed to block inside the invalidate_range_start callback, and
as struct_mutex is the inner lock with mmap_sem we can wait upon the
struct_mutex without provoking lockdep into warning about a deadlock.
However, we don't actually want to wait upon outstanding rendering
whilst holding the struct_mutex if we can help it otherwise we also
block other processes from submitting work to the GPU. So first we do a
wait without the lock and then when we reacquire the lock, we double
check that everything is ready for removing the invalidated pages.

Finally to wait upon the outstanding unpinning tasks, we create a
private workqueue as a means to conveniently wait upon all at once. The
drawback is that this workqueue is per-mm, so any threads concurrently
invalidating objects will wait upon each other. The advantage of using
the workqueue is that we can wait in parallel for completion of
rendering and unpinning of several objects (of particular importance if
the process terminates with a whole mm full of objects).

v2: Apply a cup of tea to the changelog.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94699
Testcase: igt/gem_userptr_blits/sync-unmap-cycles
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 48 ++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 291a9393493d..92b39186b05a 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -49,6 +49,7 @@ struct i915_mmu_notifier {
 	struct hlist_node node;
 	struct mmu_notifier mn;
 	struct rb_root objects;
+	struct workqueue_struct *wq;
 };
 
 struct i915_mmu_object {
@@ -60,6 +61,40 @@ struct i915_mmu_object {
 	bool attached;
 };
 
+static void wait_rendering(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_gem_request *requests[I915_NUM_ENGINES];
+	unsigned reset_counter;
+	int i, n;
+
+	if (!obj->active)
+		return;
+
+	n = 0;
+	for (i = 0; i < I915_NUM_ENGINES; i++) {
+		struct drm_i915_gem_request *req;
+
+		req = obj->last_read_req[i];
+		if (req == NULL)
+			continue;
+
+		requests[n++] = i915_gem_request_reference(req);
+	}
+
+	reset_counter = atomic_read(&to_i915(dev)->gpu_error.reset_counter);
+	mutex_unlock(&dev->struct_mutex);
+
+	for (i = 0; i < n; i++)
+		__i915_wait_request(requests[i], reset_counter, false,
+				    NULL, NULL);
+
+	mutex_lock(&dev->struct_mutex);
+
+	for (i = 0; i < n; i++)
+		i915_gem_request_unreference(requests[i]);
+}
+
 static void cancel_userptr(struct work_struct *work)
 {
 	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
@@ -75,6 +110,8 @@ static void cancel_userptr(struct work_struct *work)
 		struct i915_vma *vma, *tmp;
 		bool was_interruptible;
 
+		wait_rendering(obj);
+
 		was_interruptible = dev_priv->mm.interruptible;
 		dev_priv->mm.interruptible = false;
 
@@ -140,7 +177,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		 */
 		mo = container_of(it, struct i915_mmu_object, it);
 		if (kref_get_unless_zero(&mo->obj->base.refcount))
-			schedule_work(&mo->work);
+			queue_work(mn->wq, &mo->work);
 
 		list_add(&mo->link, &cancelled);
 		it = interval_tree_iter_next(it, start, end);
@@ -148,6 +185,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 	list_for_each_entry(mo, &cancelled, link)
 		del_object(mo);
 	spin_unlock(&mn->lock);
+
+	flush_workqueue(mn->wq);
 }
 
 static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
@@ -167,10 +206,16 @@ i915_mmu_notifier_create(struct mm_struct *mm)
 	spin_lock_init(&mn->lock);
 	mn->mn.ops = &i915_gem_userptr_notifier;
 	mn->objects = RB_ROOT;
+	mn->wq = alloc_workqueue("i915-userptr-release", WQ_UNBOUND, 0);
+	if (mn->wq == NULL) {
+		kfree(mn);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	 /* Protected by mmap_sem (write-lock) */
 	ret = __mmu_notifier_register(&mn->mn, mm);
 	if (ret) {
+		destroy_workqueue(mn->wq);
 		kfree(mn);
 		return ERR_PTR(ret);
 	}
@@ -256,6 +301,7 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
 		return;
 
 	mmu_notifier_unregister(&mn->mn, mm);
+	destroy_workqueue(mn->wq);
 	kfree(mn);
 }
 
-- 
2.8.0.rc3

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages
  2016-04-05 13:59 ` [PATCH v2 " Chris Wilson
@ 2016-04-05 14:00   ` Chris Wilson
  2016-04-05 14:00   ` [PATCH v2 3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct Chris Wilson
  2016-04-05 15:27   ` [PATCH v2 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Tvrtko Ursulin
  2 siblings, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2016-04-05 14:00 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable

Holding a reference to the containing task_struct is not sufficient to
prevent the mm_struct from being reaped under memory pressure. If this
happens whilst we are calling get_user_pages(), explosions erupt -
sometimes an immediate GPF, sometimes page flag corruption. To prevent
the target mm from being reaped as we are reading from it, acquire a
reference before we begin.

Testcase: igt/gem_shrink/*userptr
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: stable@vger.kernel.org
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 92b39186b05a..960bb37f458f 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -547,19 +547,24 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 
-		down_read(&mm->mmap_sem);
-		while (pinned < npages) {
-			ret = get_user_pages_remote(work->task, mm,
-					obj->userptr.ptr + pinned * PAGE_SIZE,
-					npages - pinned,
-					!obj->userptr.read_only, 0,
-					pvec + pinned, NULL);
-			if (ret < 0)
-				break;
-
-			pinned += ret;
+		ret = -EFAULT;
+		if (atomic_inc_not_zero(&mm->mm_users)) {
+			down_read(&mm->mmap_sem);
+			while (pinned < npages) {
+				ret = get_user_pages_remote
+					(work->task, mm,
+					 obj->userptr.ptr + pinned * PAGE_SIZE,
+					 npages - pinned,
+					 !obj->userptr.read_only, 0,
+					 pvec + pinned, NULL);
+				if (ret < 0)
+					break;
+
+				pinned += ret;
+			}
+			up_read(&mm->mmap_sem);
+			mmput(mm);
 		}
-		up_read(&mm->mmap_sem);
 	}
 
 	mutex_lock(&dev->struct_mutex);
-- 
2.8.0.rc3

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct
  2016-04-05 13:59 ` [PATCH v2 " Chris Wilson
  2016-04-05 14:00   ` [PATCH v2 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
@ 2016-04-05 14:00   ` Chris Wilson
  2016-04-05 15:27   ` [PATCH v2 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Tvrtko Ursulin
  2 siblings, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2016-04-05 14:00 UTC (permalink / raw)
  To: intel-gfx

Since we only ever use the drm_i915_private from the stored
i915_mm_struct->dev, save some electrons by storing the right
backpointer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 960bb37f458f..cb41919063d2 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -34,7 +34,7 @@
 
 struct i915_mm_struct {
 	struct mm_struct *mm;
-	struct drm_device *dev;
+	struct drm_i915_private *i915;
 	struct i915_mmu_notifier *mn;
 	struct hlist_node node;
 	struct kref kref;
@@ -250,13 +250,13 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm)
 		return mn;
 
 	down_write(&mm->mm->mmap_sem);
-	mutex_lock(&to_i915(mm->dev)->mm_lock);
+	mutex_lock(&mm->i915->mm_lock);
 	if ((mn = mm->mn) == NULL) {
 		mn = i915_mmu_notifier_create(mm->mm);
 		if (!IS_ERR(mn))
 			mm->mn = mn;
 	}
-	mutex_unlock(&to_i915(mm->dev)->mm_lock);
+	mutex_unlock(&mm->i915->mm_lock);
 	up_write(&mm->mm->mmap_sem);
 
 	return mn;
@@ -373,7 +373,7 @@ i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
 		}
 
 		kref_init(&mm->kref);
-		mm->dev = obj->base.dev;
+		mm->i915 = to_i915(obj->base.dev);
 
 		mm->mm = current->mm;
 		atomic_inc(&current->mm->mm_count);
@@ -408,7 +408,7 @@ __i915_mm_struct_free(struct kref *kref)
 
 	/* Protected by dev_priv->mm_lock */
 	hash_del(&mm->node);
-	mutex_unlock(&to_i915(mm->dev)->mm_lock);
+	mutex_unlock(&mm->i915->mm_lock);
 
 	INIT_WORK(&mm->work, __i915_mm_struct_free__worker);
 	schedule_work(&mm->work);
-- 
2.8.0.rc3

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns
  2016-04-05 13:59 ` [PATCH v2 " Chris Wilson
  2016-04-05 14:00   ` [PATCH v2 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
  2016-04-05 14:00   ` [PATCH v2 3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct Chris Wilson
@ 2016-04-05 15:27   ` Tvrtko Ursulin
  2 siblings, 0 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-04-05 15:27 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 05/04/16 14:59, Chris Wilson wrote:
> In order to ensure that all invalidations are completed before the
> operation returns to userspace (i.e. before the munmap() syscall returns)
> we need to wait upon the outstanding operations.
>
> We are allowed to block inside the invalidate_range_start callback, and
> as struct_mutex is the inner lock with mmap_sem we can wait upon the
> struct_mutex without provoking lockdep into warning about a deadlock.
> However, we don't actually want to wait upon outstanding rendering
> whilst holding the struct_mutex if we can help it otherwise we also
> block other processes from submitting work to the GPU. So first we do a
> wait without the lock and then when we reacquire the lock, we double
> check that everything is ready for removing the invalidated pages.
>
> Finally to wait upon the outstanding unpinning tasks, we create a
> private workqueue as a means to conveniently wait upon all at once. The
> drawback is that this workqueue is per-mm, so any threads concurrently
> invalidating objects will wait upon each other. The advantage of using
> the workqueue is that we can wait in parallel for completion of
> rendering and unpinning of several objects (of particular importance if
> the process terminates with a whole mm full of objects).
>
> v2: Apply a cup of tea to the changelog.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94699
> Testcase: igt/gem_userptr_blits/sync-unmap-cycles
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 48 ++++++++++++++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)

Looks OK to me.

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

Regards,

Tvrtko

> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 291a9393493d..92b39186b05a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -49,6 +49,7 @@ struct i915_mmu_notifier {
>   	struct hlist_node node;
>   	struct mmu_notifier mn;
>   	struct rb_root objects;
> +	struct workqueue_struct *wq;
>   };
>
>   struct i915_mmu_object {
> @@ -60,6 +61,40 @@ struct i915_mmu_object {
>   	bool attached;
>   };
>
> +static void wait_rendering(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->base.dev;
> +	struct drm_i915_gem_request *requests[I915_NUM_ENGINES];
> +	unsigned reset_counter;
> +	int i, n;
> +
> +	if (!obj->active)
> +		return;
> +
> +	n = 0;
> +	for (i = 0; i < I915_NUM_ENGINES; i++) {
> +		struct drm_i915_gem_request *req;
> +
> +		req = obj->last_read_req[i];
> +		if (req == NULL)
> +			continue;
> +
> +		requests[n++] = i915_gem_request_reference(req);
> +	}
> +
> +	reset_counter = atomic_read(&to_i915(dev)->gpu_error.reset_counter);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	for (i = 0; i < n; i++)
> +		__i915_wait_request(requests[i], reset_counter, false,
> +				    NULL, NULL);
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	for (i = 0; i < n; i++)
> +		i915_gem_request_unreference(requests[i]);
> +}
> +
>   static void cancel_userptr(struct work_struct *work)
>   {
>   	struct i915_mmu_object *mo = container_of(work, typeof(*mo), work);
> @@ -75,6 +110,8 @@ static void cancel_userptr(struct work_struct *work)
>   		struct i915_vma *vma, *tmp;
>   		bool was_interruptible;
>
> +		wait_rendering(obj);
> +
>   		was_interruptible = dev_priv->mm.interruptible;
>   		dev_priv->mm.interruptible = false;
>
> @@ -140,7 +177,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   		 */
>   		mo = container_of(it, struct i915_mmu_object, it);
>   		if (kref_get_unless_zero(&mo->obj->base.refcount))
> -			schedule_work(&mo->work);
> +			queue_work(mn->wq, &mo->work);
>
>   		list_add(&mo->link, &cancelled);
>   		it = interval_tree_iter_next(it, start, end);
> @@ -148,6 +185,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>   	list_for_each_entry(mo, &cancelled, link)
>   		del_object(mo);
>   	spin_unlock(&mn->lock);
> +
> +	flush_workqueue(mn->wq);
>   }
>
>   static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
> @@ -167,10 +206,16 @@ i915_mmu_notifier_create(struct mm_struct *mm)
>   	spin_lock_init(&mn->lock);
>   	mn->mn.ops = &i915_gem_userptr_notifier;
>   	mn->objects = RB_ROOT;
> +	mn->wq = alloc_workqueue("i915-userptr-release", WQ_UNBOUND, 0);
> +	if (mn->wq == NULL) {
> +		kfree(mn);
> +		return ERR_PTR(-ENOMEM);
> +	}
>
>   	 /* Protected by mmap_sem (write-lock) */
>   	ret = __mmu_notifier_register(&mn->mn, mm);
>   	if (ret) {
> +		destroy_workqueue(mn->wq);
>   		kfree(mn);
>   		return ERR_PTR(ret);
>   	}
> @@ -256,6 +301,7 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
>   		return;
>
>   	mmu_notifier_unregister(&mn->mn, mm);
> +	destroy_workqueue(mn->wq);
>   	kfree(mn);
>   }
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [v2,3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct (rev4)
  2016-04-03 17:14 [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Chris Wilson
                   ` (4 preceding siblings ...)
  2016-04-05 13:59 ` [PATCH v2 " Chris Wilson
@ 2016-04-05 16:57 ` Patchwork
  5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2016-04-05 16:57 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v2,3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct (rev4)
URL   : https://patchwork.freedesktop.org/series/5240/
State : failure

== Summary ==

Series 5240v4 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5240/revisions/4/mbox/

Test gem_mmap:
        Subgroup basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_mmap_gtt:
        Subgroup basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_sync:
        Subgroup basic-all:
                pass       -> DMESG-FAIL (bsw-nuc-2)
        Subgroup basic-bsd:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup basic-render:
                pass       -> DMESG-FAIL (ivb-t430s)
Test kms_addfb_basic:
        Subgroup bad-pitch-65536:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup unused-handle:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (bsw-nuc-2)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> DMESG-FAIL (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:151  dwarn:4   dfail:2   fail:1   skip:38 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:170  dwarn:0   dfail:1   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:161  dwarn:0   dfail:0   fail:2   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1807/

48d2a0497cd1d0630fbb59fb7871e4d678458307 drm-intel-nightly: 2016y-04m-05d-12h-30m-42s UTC integration manifest
cd06d64af3bce9a0e6c670c1557be52f8dae7b8d drm/i915/userptr: Hold mmref whilst calling get-user-pages
016e2d4be4df8ceddd6812915c3c855b228d696d drm/i915/userptr: Store i915 backpointer for i915_mm_struct

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-04-05 16:57 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-03 17:14 [PATCH 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Chris Wilson
2016-04-03 17:14 ` [PATCH 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
2016-04-03 21:11   ` Michał Winiarski
2016-04-05 12:22   ` [Intel-gfx] " Tvrtko Ursulin
2016-04-03 17:14 ` [PATCH 3/3] drm/i915: Store i915 pointer for userptr i915_mm_struct Chris Wilson
2016-04-03 21:12   ` Michał Winiarski
2016-04-04  6:27 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Patchwork
2016-04-05 12:05 ` [PATCH 1/3] " Tvrtko Ursulin
2016-04-05 12:34   ` Chris Wilson
2016-04-05 13:59 ` [PATCH v2 " Chris Wilson
2016-04-05 14:00   ` [PATCH v2 2/3] drm/i915/userptr: Hold mmref whilst calling get-user-pages Chris Wilson
2016-04-05 14:00   ` [PATCH v2 3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct Chris Wilson
2016-04-05 15:27   ` [PATCH v2 1/3] drm/i915/userptr: Flush cancellations before mmu-notifier invalidate returns Tvrtko Ursulin
2016-04-05 16:57 ` ✗ Fi.CI.BAT: failure for series starting with [v2,3/3] drm/i915/userptr: Store i915 backpointer for i915_mm_struct (rev4) Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox