[PATCH 1/6] drm/ttm: change fence_lock to inner lock

[PATCH 1/6] drm/ttm: change fence_lock to inner lock

[Maarten Lankhorst]

This requires changing the order in ttm_bo_cleanup_refs_or_queue to
take the reservation first, as there is otherwise no race free way to
take lru lock before fence_lock.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c           | 31 +++++++++++--------------------
 drivers/gpu/drm/ttm/ttm_execbuf_util.c |  4 ++--
 2 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 7426fe5..202fc20 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -500,27 +500,17 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 	struct ttm_bo_global *glob = bo->glob;
-	struct ttm_bo_driver *driver;
+	struct ttm_bo_driver *driver = bdev->driver;
 	void *sync_obj = NULL;
 	int put_count;
 	int ret;
 
+	spin_lock(&glob->lru_lock);
+	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
+
 	spin_lock(&bdev->fence_lock);
 	(void) ttm_bo_wait(bo, false, false, true);
-	if (!bo->sync_obj) {
-
-		spin_lock(&glob->lru_lock);
-
-		/**
-		 * Lock inversion between bo:reserve and bdev::fence_lock here,
-		 * but that's OK, since we're only trylocking.
-		 */
-
-		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
-
-		if (unlikely(ret == -EBUSY))
-			goto queue;
-
+	if (!ret && !bo->sync_obj) {
 		spin_unlock(&bdev->fence_lock);
 		put_count = ttm_bo_del_from_lru(bo);
 
@@ -530,18 +520,19 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 		ttm_bo_list_ref_sub(bo, put_count, true);
 
 		return;
-	} else {
-		spin_lock(&glob->lru_lock);
 	}
-queue:
-	driver = bdev->driver;
 	if (bo->sync_obj)
 		sync_obj = driver->sync_obj_ref(bo->sync_obj);
+	spin_unlock(&bdev->fence_lock);
+
+	if (!ret) {
+		atomic_set(&bo->reserved, 0);
+		wake_up_all(&bo->event_queue);
+	}
 
 	kref_get(&bo->list_kref);
 	list_add_tail(&bo->ddestroy, &bdev->ddestroy);
 	spin_unlock(&glob->lru_lock);
-	spin_unlock(&bdev->fence_lock);
 
 	if (sync_obj) {
 		driver->sync_obj_flush(sync_obj);
diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
index 1986d00..cd9e452 100644
--- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c
+++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
@@ -213,8 +213,8 @@ void ttm_eu_fence_buffer_objects(struct list_head *list, void *sync_obj)
 	driver = bdev->driver;
 	glob = bo->glob;
 
-	spin_lock(&bdev->fence_lock);
 	spin_lock(&glob->lru_lock);
+	spin_lock(&bdev->fence_lock);
 
 	list_for_each_entry(entry, list, head) {
 		bo = entry->bo;
@@ -223,8 +223,8 @@ void ttm_eu_fence_buffer_objects(struct list_head *list, void *sync_obj)
 		ttm_bo_unreserve_locked(bo);
 		entry->reserved = false;
 	}
-	spin_unlock(&glob->lru_lock);
 	spin_unlock(&bdev->fence_lock);
+	spin_unlock(&glob->lru_lock);
 
 	list_for_each_entry(entry, list, head) {
 		if (entry->old_sync_obj)
-- 
1.8.0

[PATCH 2/6] drm/radeon: allow move_notify to be called without reservation

[Maarten Lankhorst]

The few places that care should have those checks instead.
This allows destruction of bo backed memory without a reservation.
It's required for being able to rework the delayed destroy path,
as it is no longer guaranteed to hold a reservation before unlocking.

However any previous wait is still guaranteed to complete, and it's
one of the last things to be done before the buffer object is freed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
 drivers/gpu/drm/radeon/radeon_gart.c   | 1 -
 drivers/gpu/drm/radeon/radeon_object.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c
index 8690be7..6e24f84 100644
--- a/drivers/gpu/drm/radeon/radeon_gart.c
+++ b/drivers/gpu/drm/radeon/radeon_gart.c
@@ -1237,7 +1237,6 @@ void radeon_vm_bo_invalidate(struct radeon_device *rdev,
 {
 	struct radeon_bo_va *bo_va;
 
-	BUG_ON(!radeon_bo_is_reserved(bo));
 	list_for_each_entry(bo_va, &bo->va, bo_list) {
 		bo_va->valid = false;
 	}
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 7c4b4bb..69c8811 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -520,7 +520,7 @@ void radeon_bo_get_tiling_flags(struct radeon_bo *bo,
 int radeon_bo_check_tiling(struct radeon_bo *bo, bool has_moved,
 				bool force_drop)
 {
-	BUG_ON(!radeon_bo_is_reserved(bo));
+	BUG_ON(!radeon_bo_is_reserved(bo) && !force_drop);
 
 	if (!(bo->tiling_flags & RADEON_TILING_SURFACE))
 		return 0;
-- 
1.8.0

[PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Maarten Lankhorst]

By removing the unlocking of lru and retaking it immediately, a race is
removed where the bo is taken off the swap list or the lru list between
the unlock and relock. As such the cleanup_refs code can be simplified,
it will attempt to call ttm_bo_wait non-blockingly, and if it fails
it will drop the locks and perform a blocking wait, or return an error
if no_wait_gpu was set.

The need for looping is also eliminated, since swapout and evict_mem_first
will always follow the destruction path, so no new fence is allowed
to be attached. As far as I can see this may already have been the case,
but the unlocking / relocking required a complicated loop to deal with
re-reservation.

The downside is that ttm_bo_cleanup_memtype_use is no longer called with
reservation held, so drivers must be aware that move_notify with a null
parameter doesn't require a reservation.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 112 +++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 202fc20..02b275b 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -486,14 +486,6 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 		bo->ttm = NULL;
 	}
 	ttm_bo_mem_put(bo, &bo->mem);
-
-	atomic_set(&bo->reserved, 0);
-
-	/*
-	 * Make processes trying to reserve really pick it up.
-	 */
-	smp_mb__after_atomic_dec();
-	wake_up_all(&bo->event_queue);
 }
 
 static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
@@ -515,6 +507,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 		put_count = ttm_bo_del_from_lru(bo);
 
 		spin_unlock(&glob->lru_lock);
+		atomic_set(&bo->reserved, 0);
+		wake_up_all(&bo->event_queue);
+
 		ttm_bo_cleanup_memtype_use(bo);
 
 		ttm_bo_list_ref_sub(bo, put_count, true);
@@ -543,68 +538,72 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 }
 
 /**
- * function ttm_bo_cleanup_refs
+ * function ttm_bo_cleanup_refs_and_unlock
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
+ * Must be called with lru_lock and reservation held, this function
+ * will drop both before returning.
+ *
  * @interruptible         Any sleeps should occur interruptibly.
- * @no_wait_reserve       Never wait for reserve. Return -EBUSY instead.
  * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
  */
 
-static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
-			       bool interruptible,
-			       bool no_wait_reserve,
-			       bool no_wait_gpu)
+static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
+					  bool interruptible,
+					  bool no_wait_gpu)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
+	struct ttm_bo_driver *driver = bdev->driver;
 	struct ttm_bo_global *glob = bo->glob;
 	int put_count;
 	int ret = 0;
 
-retry:
 	spin_lock(&bdev->fence_lock);
-	ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
-	spin_unlock(&bdev->fence_lock);
+	ret = ttm_bo_wait(bo, false, false, true);
 
-	if (unlikely(ret != 0))
+	if (ret && no_wait_gpu) {
+		spin_unlock(&bdev->fence_lock);
+		atomic_set(&bo->reserved, 0);
+		wake_up_all(&bo->event_queue);
+		spin_unlock(&glob->lru_lock);
 		return ret;
+	} else if (ret) {
+		void *sync_obj;
 
-retry_reserve:
-	spin_lock(&glob->lru_lock);
-
-	if (unlikely(list_empty(&bo->ddestroy))) {
+		/**
+		 * Take a reference to the fence and unreserve,
+		 * at this point the buffer should be dead, so
+		 * no new sync objects can be attached.
+		 */
+		sync_obj = driver->sync_obj_ref(&bo->sync_obj);
+		spin_unlock(&bdev->fence_lock);
+		atomic_set(&bo->reserved, 0);
+		wake_up_all(&bo->event_queue);
 		spin_unlock(&glob->lru_lock);
-		return 0;
-	}
-
-	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
 
-	if (unlikely(ret == -EBUSY)) {
-		spin_unlock(&glob->lru_lock);
-		if (likely(!no_wait_reserve))
-			ret = ttm_bo_wait_unreserved(bo, interruptible);
-		if (unlikely(ret != 0))
+		ret = driver->sync_obj_wait(sync_obj, false, interruptible);
+		driver->sync_obj_unref(&sync_obj);
+		if (ret)
 			return ret;
 
-		goto retry_reserve;
-	}
-
-	BUG_ON(ret != 0);
+		/* remove sync_obj with ttm_bo_wait */
+		spin_lock(&bdev->fence_lock);
+		ret = ttm_bo_wait(bo, false, false, true);
+		spin_unlock(&bdev->fence_lock);
 
-	/**
-	 * We can re-check for sync object without taking
-	 * the bo::lock since setting the sync object requires
-	 * also bo::reserved. A busy object at this point may
-	 * be caused by another thread recently starting an accelerated
-	 * eviction.
-	 */
+		WARN_ON(ret);
 
-	if (unlikely(bo->sync_obj)) {
+		spin_lock(&glob->lru_lock);
+	} else {
+		spin_unlock(&bdev->fence_lock);
 		atomic_set(&bo->reserved, 0);
 		wake_up_all(&bo->event_queue);
+	}
+
+	if (unlikely(list_empty(&bo->ddestroy))) {
 		spin_unlock(&glob->lru_lock);
-		goto retry;
+		return 0;
 	}
 
 	put_count = ttm_bo_del_from_lru(bo);
@@ -647,9 +646,13 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 			kref_get(&nentry->list_kref);
 		}
 
-		spin_unlock(&glob->lru_lock);
-		ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
-					  !remove_all);
+		ret = ttm_bo_reserve_locked(entry, false, !remove_all, false, 0);
+		if (!ret)
+			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
+							     !remove_all);
+		else
+			spin_unlock(&glob->lru_lock);
+
 		kref_put(&entry->list_kref, ttm_bo_release_list);
 		entry = nentry;
 
@@ -803,9 +806,13 @@ retry:
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		spin_unlock(&glob->lru_lock);
-		ret = ttm_bo_cleanup_refs(bo, interruptible,
-					  no_wait_reserve, no_wait_gpu);
+		ret = ttm_bo_reserve_locked(bo, interruptible, no_wait_reserve, false, 0);
+		if (!ret)
+			ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
+							     no_wait_gpu);
+		else
+			spin_unlock(&glob->lru_lock);
+
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 
 		return ret;
@@ -1799,8 +1806,9 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 		kref_get(&bo->list_kref);
 
 		if (!list_empty(&bo->ddestroy)) {
-			spin_unlock(&glob->lru_lock);
-			(void) ttm_bo_cleanup_refs(bo, false, false, false);
+			ttm_bo_reserve_locked(bo, false, false, false, 0);
+			ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+
 			kref_put(&bo->list_kref, ttm_bo_release_list);
 			spin_lock(&glob->lru_lock);
 			continue;
-- 
1.8.0

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Thomas Hellstrom]

On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
> By removing the unlocking of lru and retaking it immediately, a race is
> removed where the bo is taken off the swap list or the lru list between
> the unlock and relock. As such the cleanup_refs code can be simplified,
> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
> it will drop the locks and perform a blocking wait, or return an error
> if no_wait_gpu was set.
>
> The need for looping is also eliminated, since swapout and evict_mem_first
> will always follow the destruction path, so no new fence is allowed
> to be attached. As far as I can see this may already have been the case,
> but the unlocking / relocking required a complicated loop to deal with
> re-reservation.
>
> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
> reservation held, so drivers must be aware that move_notify with a null
> parameter doesn't require a reservation.

Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
immediately clear from this patch.

>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 112 +++++++++++++++++++++++--------------------
>   1 file changed, 60 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 202fc20..02b275b 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -486,14 +486,6 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
>   		bo->ttm = NULL;
>   	}
>   	ttm_bo_mem_put(bo, &bo->mem);
> -
> -	atomic_set(&bo->reserved, 0);
> -
> -	/*
> -	 * Make processes trying to reserve really pick it up.
> -	 */
> -	smp_mb__after_atomic_dec();
> -	wake_up_all(&bo->event_queue);
>   }
>   
>   static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
> @@ -515,6 +507,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>   		put_count = ttm_bo_del_from_lru(bo);
>   
>   		spin_unlock(&glob->lru_lock);
> +		atomic_set(&bo->reserved, 0);
> +		wake_up_all(&bo->event_queue);
> +

I think (although I'm not 100% sure) that if we use atomic_set() to 
unreserve, and it's not followed by a spin_unlock(), we need to insert
a memory barrier, like is done above in the removed code, otherwise 
memory operations protected by reserve may be reordered until after 
reservation.

>   		ttm_bo_cleanup_memtype_use(bo);
>   
>   		ttm_bo_list_ref_sub(bo, put_count, true);
> @@ -543,68 +538,72 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>   }
>   
>   /**
> - * function ttm_bo_cleanup_refs
> + * function ttm_bo_cleanup_refs_and_unlock
>    * If bo idle, remove from delayed- and lru lists, and unref.
>    * If not idle, do nothing.
>    *
> + * Must be called with lru_lock and reservation held, this function
> + * will drop both before returning.
> + *
>    * @interruptible         Any sleeps should occur interruptibly.
> - * @no_wait_reserve       Never wait for reserve. Return -EBUSY instead.
>    * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
>    */
>   
> -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> -			       bool interruptible,
> -			       bool no_wait_reserve,
> -			       bool no_wait_gpu)
> +static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
> +					  bool interruptible,
> +					  bool no_wait_gpu)
>   {
>   	struct ttm_bo_device *bdev = bo->bdev;
> +	struct ttm_bo_driver *driver = bdev->driver;
>   	struct ttm_bo_global *glob = bo->glob;
>   	int put_count;
>   	int ret = 0;
>   
> -retry:
>   	spin_lock(&bdev->fence_lock);
> -	ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
> -	spin_unlock(&bdev->fence_lock);
> +	ret = ttm_bo_wait(bo, false, false, true);
>   
> -	if (unlikely(ret != 0))
> +	if (ret && no_wait_gpu) {
> +		spin_unlock(&bdev->fence_lock);
> +		atomic_set(&bo->reserved, 0);
> +		wake_up_all(&bo->event_queue);
> +		spin_unlock(&glob->lru_lock);
>   		return ret;
> +	} else if (ret) {
> +		void *sync_obj;
>   
> -retry_reserve:
> -	spin_lock(&glob->lru_lock);
> -
> -	if (unlikely(list_empty(&bo->ddestroy))) {
> +		/**
> +		 * Take a reference to the fence and unreserve,
> +		 * at this point the buffer should be dead, so
> +		 * no new sync objects can be attached.
> +		 */
> +		sync_obj = driver->sync_obj_ref(&bo->sync_obj);
> +		spin_unlock(&bdev->fence_lock);
> +		atomic_set(&bo->reserved, 0);
> +		wake_up_all(&bo->event_queue);
>   		spin_unlock(&glob->lru_lock);
> -		return 0;
> -	}
> -
> -	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
>   
> -	if (unlikely(ret == -EBUSY)) {
> -		spin_unlock(&glob->lru_lock);
> -		if (likely(!no_wait_reserve))
> -			ret = ttm_bo_wait_unreserved(bo, interruptible);
> -		if (unlikely(ret != 0))
> +		ret = driver->sync_obj_wait(sync_obj, false, interruptible);
> +		driver->sync_obj_unref(&sync_obj);
> +		if (ret)
>   			return ret;
>   
> -		goto retry_reserve;
> -	}
> -
> -	BUG_ON(ret != 0);
> +		/* remove sync_obj with ttm_bo_wait */
> +		spin_lock(&bdev->fence_lock);
> +		ret = ttm_bo_wait(bo, false, false, true);
> +		spin_unlock(&bdev->fence_lock);
>   
> -	/**
> -	 * We can re-check for sync object without taking
> -	 * the bo::lock since setting the sync object requires
> -	 * also bo::reserved. A busy object at this point may
> -	 * be caused by another thread recently starting an accelerated
> -	 * eviction.
> -	 */
> +		WARN_ON(ret);
>   
> -	if (unlikely(bo->sync_obj)) {
> +		spin_lock(&glob->lru_lock);
> +	} else {
> +		spin_unlock(&bdev->fence_lock);
>   		atomic_set(&bo->reserved, 0);
>   		wake_up_all(&bo->event_queue);
> +	}
> +
> +	if (unlikely(list_empty(&bo->ddestroy))) {
>   		spin_unlock(&glob->lru_lock);
> -		goto retry;
> +		return 0;
>   	}
>   
>   	put_count = ttm_bo_del_from_lru(bo);
> @@ -647,9 +646,13 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
>   			kref_get(&nentry->list_kref);
>   		}
>   
> -		spin_unlock(&glob->lru_lock);
> -		ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
> -					  !remove_all);
> +		ret = ttm_bo_reserve_locked(entry, false, !remove_all, false, 0);
> +		if (!ret)
> +			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
> +							     !remove_all);
> +		else
> +			spin_unlock(&glob->lru_lock);
> +
>   		kref_put(&entry->list_kref, ttm_bo_release_list);
>   		entry = nentry;
>   
> @@ -803,9 +806,13 @@ retry:
>   	kref_get(&bo->list_kref);
>   
>   	if (!list_empty(&bo->ddestroy)) {
> -		spin_unlock(&glob->lru_lock);
> -		ret = ttm_bo_cleanup_refs(bo, interruptible,
> -					  no_wait_reserve, no_wait_gpu);
> +		ret = ttm_bo_reserve_locked(bo, interruptible, no_wait_reserve, false, 0);
> +		if (!ret)
> +			ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
> +							     no_wait_gpu);
> +		else
> +			spin_unlock(&glob->lru_lock);
> +
>   		kref_put(&bo->list_kref, ttm_bo_release_list);
>   
>   		return ret;
> @@ -1799,8 +1806,9 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
>   		kref_get(&bo->list_kref);
>   
>   		if (!list_empty(&bo->ddestroy)) {
> -			spin_unlock(&glob->lru_lock);
> -			(void) ttm_bo_cleanup_refs(bo, false, false, false);
> +			ttm_bo_reserve_locked(bo, false, false, false, 0);
> +			ttm_bo_cleanup_refs_and_unlock(bo, false, false);
> +
>   			kref_put(&bo->list_kref, ttm_bo_release_list);
>   			spin_lock(&glob->lru_lock);
>   			continue;

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Maarten Lankhorst]

Op 28-11-12 12:54, Thomas Hellstrom schreef:
> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>> By removing the unlocking of lru and retaking it immediately, a race is
>> removed where the bo is taken off the swap list or the lru list between
>> the unlock and relock. As such the cleanup_refs code can be simplified,
>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>> it will drop the locks and perform a blocking wait, or return an error
>> if no_wait_gpu was set.
>>
>> The need for looping is also eliminated, since swapout and evict_mem_first
>> will always follow the destruction path, so no new fence is allowed
>> to be attached. As far as I can see this may already have been the case,
>> but the unlocking / relocking required a complicated loop to deal with
>> re-reservation.
>>
>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>> reservation held, so drivers must be aware that move_notify with a null
>> parameter doesn't require a reservation.
>
> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
> immediately clear from this patch.
Because we would hold the reservation while waiting and with the object still
on swap and lru lists still, that would defeat the whole purpose of keeping
the object on multiple lists, plus break current code that assumes bo's on the
those lists can always be reserved.

the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
I'm sure that would not be the case if the reservations were shared across
devices.

The alternatives are worse: doing a blocking reserve would rightfully anger lockdep,
and how do you want to handle tryreserve failing?

Since there is a path in which no reservation can be taken, to weed out bugs I felt it
would be better that any bugs that trigger from not holding a reservation would be
easier to find if it happens all the time, instead of only in some corner cases.

~Maarten

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Thomas Hellstrom]

On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>> By removing the unlocking of lru and retaking it immediately, a race is
>>> removed where the bo is taken off the swap list or the lru list between
>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>> it will drop the locks and perform a blocking wait, or return an error
>>> if no_wait_gpu was set.
>>>
>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>> will always follow the destruction path, so no new fence is allowed
>>> to be attached. As far as I can see this may already have been the case,
>>> but the unlocking / relocking required a complicated loop to deal with
>>> re-reservation.
>>>
>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>> reservation held, so drivers must be aware that move_notify with a null
>>> parameter doesn't require a reservation.
>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>> immediately clear from this patch.
> Because we would hold the reservation while waiting and with the object still
> on swap and lru lists still, that would defeat the whole purpose of keeping
> the object on multiple lists, plus break current code that assumes bo's on the
> those lists can always be reserved.
>
> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
> I'm sure that would not be the case if the reservations were shared across
> devices.
The evict path removes the BO from the LRU lists, drops the LRU lock but 
hangs on to the reservation,
and in case the wait goes wrong, re-adds the bo to the LRU lists and 
returns an error.

Isn't it possible to do the same in the !no_wait_gpu case?
/Thomas

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Maarten Lankhorst]

Op 28-11-12 14:21, Thomas Hellstrom schreef:
> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>> removed where the bo is taken off the swap list or the lru list between
>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>> it will drop the locks and perform a blocking wait, or return an error
>>>> if no_wait_gpu was set.
>>>>
>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>> will always follow the destruction path, so no new fence is allowed
>>>> to be attached. As far as I can see this may already have been the case,
>>>> but the unlocking / relocking required a complicated loop to deal with
>>>> re-reservation.
>>>>
>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>> reservation held, so drivers must be aware that move_notify with a null
>>>> parameter doesn't require a reservation.
>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>> immediately clear from this patch.
>> Because we would hold the reservation while waiting and with the object still
>> on swap and lru lists still, that would defeat the whole purpose of keeping
>> the object on multiple lists, plus break current code that assumes bo's on the
>> those lists can always be reserved.
>>
>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>> I'm sure that would not be the case if the reservations were shared across
>> devices.
> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
case, so not adding it back to the other lists is harmless.

But I thought it was a feature that it still appeared on the lists while not being reserved, since in that
case swapout and evict_first wouldn't need to hunt down another buffer to swap out or free, instead waiting
ddestroy wait to finish, with possibly multiple waiters.
> Isn't it possible to do the same in the !no_wait_gpu case?
> /Thomas
Sure, we wouldn't even need to re-add since only having the bo on the ddestroy list not catastrophical.
I was just worried because since that it is still a behavioral change.

I'm tempted to measure events about that though, and use perf events on how often contentions
occur, buffers get evicted and loaded back in again, etc..

Before we start worrying about any optimizations, we should worry about solid instrumentation first.

I think for example that evict_first and swapout could walk the ddestroy list first, all buffers that
can be reserved there will be tested if non-blocking wait succeeds or not. If it does succeed function
returns early, if not it unreserves it again without touching any list. This would cause ttm to prefer
non-blocking destruction of bo's before normal code runs, but without measurements it's no more
than a nice thought experiment.

~Maarten

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Thomas Hellstrom]

On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>>> removed where the bo is taken off the swap list or the lru list between
>>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>> it will drop the locks and perform a blocking wait, or return an error
>>>>> if no_wait_gpu was set.
>>>>>
>>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>>> will always follow the destruction path, so no new fence is allowed
>>>>> to be attached. As far as I can see this may already have been the case,
>>>>> but the unlocking / relocking required a complicated loop to deal with
>>>>> re-reservation.
>>>>>
>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>>> reservation held, so drivers must be aware that move_notify with a null
>>>>> parameter doesn't require a reservation.
>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>> immediately clear from this patch.
>>> Because we would hold the reservation while waiting and with the object still
>>> on swap and lru lists still, that would defeat the whole purpose of keeping
>>> the object on multiple lists, plus break current code that assumes bo's on the
>>> those lists can always be reserved.
>>>
>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>>> I'm sure that would not be the case if the reservations were shared across
>>> devices.
>> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
>> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
> If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
> leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
> case, so not adding it back to the other lists is harmless.
>
Well I'm a bit afraid that theoretically, other callers may have a bo 
reserved, while cleanup_refs_and_unlock
more or less runs the whole destroy path on that buffer. Sure, we have 
control over those other reservers,
but it may come back and bite us.

Also the wait might fail if a signal is hit, so it's definitely 
possible, and even likely in the case of the X server process.

Anyway, I prefer if we could try to keep the reservation across the 
ttm_cleanup_memtype_use  function, and as far
as I can tell, the only thing preventing that is the reservation release 
in the (!no_wait_gpu) path. So if we alter that to
do the same as the evict path I think without looking to deeply into the 
consequences that we should be safe.

Then we should be able to skip patch 2 as well.

/Thomas

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Maarten Lankhorst]

Op 28-11-12 15:23, Thomas Hellstrom schreef:
> On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
>> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>>>> removed where the bo is taken off the swap list or the lru list between
>>>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>>> it will drop the locks and perform a blocking wait, or return an error
>>>>>> if no_wait_gpu was set.
>>>>>>
>>>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>>>> will always follow the destruction path, so no new fence is allowed
>>>>>> to be attached. As far as I can see this may already have been the case,
>>>>>> but the unlocking / relocking required a complicated loop to deal with
>>>>>> re-reservation.
>>>>>>
>>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>>>> reservation held, so drivers must be aware that move_notify with a null
>>>>>> parameter doesn't require a reservation.
>>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>>> immediately clear from this patch.
>>>> Because we would hold the reservation while waiting and with the object still
>>>> on swap and lru lists still, that would defeat the whole purpose of keeping
>>>> the object on multiple lists, plus break current code that assumes bo's on the
>>>> those lists can always be reserved.
>>>>
>>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>>>> I'm sure that would not be the case if the reservations were shared across
>>>> devices.
>>> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
>>> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
>> If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
>> leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
>> case, so not adding it back to the other lists is harmless.
>>
> Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock
> more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers,
> but it may come back and bite us.
That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in
only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen
right away, it still happens before last list ref to the bo is dropped.

But it's your call, just choose the approach you want and I'll resubmit this. :-)

> Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.
>
> Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use  function, and as far
> as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to
> do the same as the evict path I think without looking to deeply into the consequences that we should be safe.
I think returning success early without removing off ddestroy list if re-reserving fails
with lru lock held would be better.

We completed the wait and attempt to reserve the bo, which failed. Without the lru
lock atomicity, this can't happen since the only places that would do it call this with
the lru lock held.

With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete
with remove_all set to true. And even if that happened the destruction code would run
*anyway* since we completed the waiting part already, it would just not necessarily be
run from this thread, but that guarantee didn't exist anyway.
> Then we should be able to skip patch 2 as well.
If my tryreserve approach sounds sane, second patch should still be skippable. :-)

~Maarten

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Thomas Hellstrom]

On 11/28/2012 03:46 PM, Maarten Lankhorst wrote:
> Op 28-11-12 15:23, Thomas Hellstrom schreef:
>> On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
>>> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>>>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>>>>> removed where the bo is taken off the swap list or the lru list between
>>>>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>>>> it will drop the locks and perform a blocking wait, or return an error
>>>>>>> if no_wait_gpu was set.
>>>>>>>
>>>>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>>>>> will always follow the destruction path, so no new fence is allowed
>>>>>>> to be attached. As far as I can see this may already have been the case,
>>>>>>> but the unlocking / relocking required a complicated loop to deal with
>>>>>>> re-reservation.
>>>>>>>
>>>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>>>>> reservation held, so drivers must be aware that move_notify with a null
>>>>>>> parameter doesn't require a reservation.
>>>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>>>> immediately clear from this patch.
>>>>> Because we would hold the reservation while waiting and with the object still
>>>>> on swap and lru lists still, that would defeat the whole purpose of keeping
>>>>> the object on multiple lists, plus break current code that assumes bo's on the
>>>>> those lists can always be reserved.
>>>>>
>>>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>>>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>>>>> I'm sure that would not be the case if the reservations were shared across
>>>>> devices.
>>>> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
>>>> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
>>> If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
>>> leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
>>> case, so not adding it back to the other lists is harmless.
>>>
>> Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock
>> more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers,
>> but it may come back and bite us.
> That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in
> only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen
> right away, it still happens before last list ref to the bo is dropped.
>
> But it's your call, just choose the approach you want and I'll resubmit this. :-)
>
>> Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.
>>
>> Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use  function, and as far
>> as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to
>> do the same as the evict path I think without looking to deeply into the consequences that we should be safe.
> I think returning success early without removing off ddestroy list if re-reserving fails
> with lru lock held would be better.
>
> We completed the wait and attempt to reserve the bo, which failed. Without the lru
> lock atomicity, this can't happen since the only places that would do it call this with
> the lru lock held.
>
> With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete
> with remove_all set to true. And even if that happened the destruction code would run
> *anyway* since we completed the waiting part already, it would just not necessarily be
> run from this thread, but that guarantee didn't exist anyway.
>> Then we should be able to skip patch 2 as well.
> If my tryreserve approach sounds sane, second patch should still be skippable. :-)

Sure, Lets go for that approach.

>
> ~Maarten
/Thomas

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Maarten Lankhorst]

Op 28-11-12 16:10, Thomas Hellstrom schreef:
> On 11/28/2012 03:46 PM, Maarten Lankhorst wrote:
>> Op 28-11-12 15:23, Thomas Hellstrom schreef:
>>> On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
>>>> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>>>>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>>>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>>>>>> removed where the bo is taken off the swap list or the lru list between
>>>>>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>>>>> it will drop the locks and perform a blocking wait, or return an error
>>>>>>>> if no_wait_gpu was set.
>>>>>>>>
>>>>>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>>>>>> will always follow the destruction path, so no new fence is allowed
>>>>>>>> to be attached. As far as I can see this may already have been the case,
>>>>>>>> but the unlocking / relocking required a complicated loop to deal with
>>>>>>>> re-reservation.
>>>>>>>>
>>>>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>>>>>> reservation held, so drivers must be aware that move_notify with a null
>>>>>>>> parameter doesn't require a reservation.
>>>>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>>>>> immediately clear from this patch.
>>>>>> Because we would hold the reservation while waiting and with the object still
>>>>>> on swap and lru lists still, that would defeat the whole purpose of keeping
>>>>>> the object on multiple lists, plus break current code that assumes bo's on the
>>>>>> those lists can always be reserved.
>>>>>>
>>>>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>>>>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>>>>>> I'm sure that would not be the case if the reservations were shared across
>>>>>> devices.
>>>>> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
>>>>> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
>>>> If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
>>>> leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
>>>> case, so not adding it back to the other lists is harmless.
>>>>
>>> Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock
>>> more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers,
>>> but it may come back and bite us.
>> That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in
>> only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen
>> right away, it still happens before last list ref to the bo is dropped.
>>
>> But it's your call, just choose the approach you want and I'll resubmit this. :-)
>>
>>> Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.
>>>
>>> Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use  function, and as far
>>> as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to
>>> do the same as the evict path I think without looking to deeply into the consequences that we should be safe.
>> I think returning success early without removing off ddestroy list if re-reserving fails
>> with lru lock held would be better.
>>
>> We completed the wait and attempt to reserve the bo, which failed. Without the lru
>> lock atomicity, this can't happen since the only places that would do it call this with
>> the lru lock held.
>>
>> With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete
>> with remove_all set to true. And even if that happened the destruction code would run
>> *anyway* since we completed the waiting part already, it would just not necessarily be
>> run from this thread, but that guarantee didn't exist anyway.
>>> Then we should be able to skip patch 2 as well.
>> If my tryreserve approach sounds sane, second patch should still be skippable. :-)
>
> Sure, Lets go for that approach.
Ok updated patch below,  you only need to check if you like the approach or not,
since I haven't tested it yet beyond compiling. Rest of series (minus patch 2)
should still apply without modification.

    drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2
    
    By removing the unlocking of lru and retaking it immediately, a race is
    removed where the bo is taken off the swap list or the lru list between
    the unlock and relock. As such the cleanup_refs code can be simplified,
    it will attempt to call ttm_bo_wait non-blockingly, and if it fails
    it will drop the locks and perform a blocking wait, or return an error
    if no_wait_gpu was set.
    
    The need for looping is also eliminated, since swapout and evict_mem_first
    will always follow the destruction path, no new fence is allowed
    to be attached. As far as I can see this may already have been the case,
    but the unlocking / relocking required a complicated loop to deal with
    re-reservation.
    
    Changes since v1:
     - Simplify no_wait_gpu case by folding it in with empty ddestroy.
     - Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 202fc20..e9f01fe 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 	ttm_bo_mem_put(bo, &bo->mem);
 
 	atomic_set(&bo->reserved, 0);
+	wake_up_all(&bo->event_queue);
 
 	/*
-	 * Make processes trying to reserve really pick it up.
+	 * Since the final reference to this bo may not be dropped by
+	 * the current task we have to put a memory barrier here to make
+	 * sure the changes done in this function are always visible.
+	 *
+	 * This function only needs protection against the final kref_put.
 	 */
-	smp_mb__after_atomic_dec();
-	wake_up_all(&bo->event_queue);
+	smp_mb__before_atomic_dec();
 }
 
 static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
@@ -543,68 +547,95 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 }
 
 /**
- * function ttm_bo_cleanup_refs
+ * function ttm_bo_cleanup_refs_and_unlock
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
+ * Must be called with lru_lock and reservation held, this function
+ * will drop both before returning.
+ *
  * @interruptible         Any sleeps should occur interruptibly.
- * @no_wait_reserve       Never wait for reserve. Return -EBUSY instead.
  * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
  */
 
-static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
-			       bool interruptible,
-			       bool no_wait_reserve,
-			       bool no_wait_gpu)
+static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
+					  bool interruptible,
+					  bool no_wait_gpu)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
+	struct ttm_bo_driver *driver = bdev->driver;
 	struct ttm_bo_global *glob = bo->glob;
 	int put_count;
-	int ret = 0;
+	int ret;
 
-retry:
 	spin_lock(&bdev->fence_lock);
-	ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
-	spin_unlock(&bdev->fence_lock);
+	ret = ttm_bo_wait(bo, false, false, true);
 
-	if (unlikely(ret != 0))
-		return ret;
+	if (ret && !no_wait_gpu) {
+		void *sync_obj;
 
-retry_reserve:
-	spin_lock(&glob->lru_lock);
+		/*
+		 * Take a reference to the fence and unreserve,
+		 * at this point the buffer should be dead, so
+		 * no new sync objects can be attached.
+		 */
+		sync_obj = driver->sync_obj_ref(&bo->sync_obj);
+		spin_unlock(&bdev->fence_lock);
 
-	if (unlikely(list_empty(&bo->ddestroy))) {
+		put_count = ttm_bo_del_from_lru(bo);
+
+		atomic_set(&bo->reserved, 0);
+		wake_up_all(&bo->event_queue);
 		spin_unlock(&glob->lru_lock);
-		return 0;
-	}
 
-	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
+		ttm_bo_list_ref_sub(bo, put_count, true);
 
-	if (unlikely(ret == -EBUSY)) {
-		spin_unlock(&glob->lru_lock);
-		if (likely(!no_wait_reserve))
-			ret = ttm_bo_wait_unreserved(bo, interruptible);
-		if (unlikely(ret != 0))
+		ret = driver->sync_obj_wait(sync_obj, false, interruptible);
+		driver->sync_obj_unref(&sync_obj);
+		if (ret) {
+			/*
+			 * Either the wait returned -ERESTARTSYS, or -EDEADLK
+			 * (radeon lockup) here. No effort is made to re-add
+			 * this bo to any lru list. Instead the bo only
+			 * appears on the delayed destroy list.
+			 */
 			return ret;
+		}
 
-		goto retry_reserve;
-	}
+		/*
+		 * remove sync_obj with ttm_bo_wait, the wait should be
+		 * finished, and no new wait object should have been added.
+		 */
+		spin_lock(&bdev->fence_lock);
+		ret = ttm_bo_wait(bo, false, false, true);
+		WARN_ON(ret);
+		spin_unlock(&bdev->fence_lock);
+		if (ret)
+			return ret;
 
-	BUG_ON(ret != 0);
+		spin_lock(&glob->lru_lock);
+		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
 
-	/**
-	 * We can re-check for sync object without taking
-	 * the bo::lock since setting the sync object requires
-	 * also bo::reserved. A busy object at this point may
-	 * be caused by another thread recently starting an accelerated
-	 * eviction.
-	 */
+		/*
+		 * We raced, and lost, someone else holds the reservation now,
+		 * and is probably busy in ttm_bo_cleanup_memtype_use.
+		 *
+		 * Even if it's not the case, because we finished waiting any
+		 * delayed destruction would succeed, so just return success
+		 * here.
+		 */
+		if (ret) {
+			spin_unlock(&glob->lru_lock);
+			return 0;
+		}
+	} else
+		spin_unlock(&bdev->fence_lock);
 
-	if (unlikely(bo->sync_obj)) {
+	if (ret || unlikely(list_empty(&bo->ddestroy))) {
 		atomic_set(&bo->reserved, 0);
 		wake_up_all(&bo->event_queue);
 		spin_unlock(&glob->lru_lock);
-		goto retry;
+		return ret;
 	}
 
 	put_count = ttm_bo_del_from_lru(bo);
@@ -647,9 +678,13 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 			kref_get(&nentry->list_kref);
 		}
 
-		spin_unlock(&glob->lru_lock);
-		ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
-					  !remove_all);
+		ret = ttm_bo_reserve_locked(entry, false, !remove_all, false, 0);
+		if (!ret)
+			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
+							     !remove_all);
+		else
+			spin_unlock(&glob->lru_lock);
+
 		kref_put(&entry->list_kref, ttm_bo_release_list);
 		entry = nentry;
 
@@ -803,9 +838,13 @@ retry:
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		spin_unlock(&glob->lru_lock);
-		ret = ttm_bo_cleanup_refs(bo, interruptible,
-					  no_wait_reserve, no_wait_gpu);
+		ret = ttm_bo_reserve_locked(bo, interruptible, no_wait_reserve, false, 0);
+		if (!ret)
+			ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
+							     no_wait_gpu);
+		else
+			spin_unlock(&glob->lru_lock);
+
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 
 		return ret;
@@ -1799,8 +1838,9 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 		kref_get(&bo->list_kref);
 
 		if (!list_empty(&bo->ddestroy)) {
-			spin_unlock(&glob->lru_lock);
-			(void) ttm_bo_cleanup_refs(bo, false, false, false);
+			ttm_bo_reserve_locked(bo, false, false, false, 0);
+			ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+
 			kref_put(&bo->list_kref, ttm_bo_release_list);
 			spin_lock(&glob->lru_lock);
 			continue;

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Thomas Hellstrom]

On 11/28/2012 07:32 PM, Maarten Lankhorst wrote:
> Op 28-11-12 16:10, Thomas Hellstrom schreef:
>> On 11/28/2012 03:46 PM, Maarten Lankhorst wrote:
>>> Op 28-11-12 15:23, Thomas Hellstrom schreef:
>>>> On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
>>>>> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>>>>>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>>>>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>>>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>>>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>>>>>>> removed where the bo is taken off the swap list or the lru list between
>>>>>>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>>>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>>>>>> it will drop the locks and perform a blocking wait, or return an error
>>>>>>>>> if no_wait_gpu was set.
>>>>>>>>>
>>>>>>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>>>>>>> will always follow the destruction path, so no new fence is allowed
>>>>>>>>> to be attached. As far as I can see this may already have been the case,
>>>>>>>>> but the unlocking / relocking required a complicated loop to deal with
>>>>>>>>> re-reservation.
>>>>>>>>>
>>>>>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>>>>>>> reservation held, so drivers must be aware that move_notify with a null
>>>>>>>>> parameter doesn't require a reservation.
>>>>>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>>>>>> immediately clear from this patch.
>>>>>>> Because we would hold the reservation while waiting and with the object still
>>>>>>> on swap and lru lists still, that would defeat the whole purpose of keeping
>>>>>>> the object on multiple lists, plus break current code that assumes bo's on the
>>>>>>> those lists can always be reserved.
>>>>>>>
>>>>>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>>>>>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>>>>>>> I'm sure that would not be the case if the reservations were shared across
>>>>>>> devices.
>>>>>> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
>>>>>> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
>>>>> If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
>>>>> leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
>>>>> case, so not adding it back to the other lists is harmless.
>>>>>
>>>> Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock
>>>> more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers,
>>>> but it may come back and bite us.
>>> That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in
>>> only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen
>>> right away, it still happens before last list ref to the bo is dropped.
>>>
>>> But it's your call, just choose the approach you want and I'll resubmit this. :-)
>>>
>>>> Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.
>>>>
>>>> Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use  function, and as far
>>>> as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to
>>>> do the same as the evict path I think without looking to deeply into the consequences that we should be safe.
>>> I think returning success early without removing off ddestroy list if re-reserving fails
>>> with lru lock held would be better.
>>>
>>> We completed the wait and attempt to reserve the bo, which failed. Without the lru
>>> lock atomicity, this can't happen since the only places that would do it call this with
>>> the lru lock held.
>>>
>>> With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete
>>> with remove_all set to true. And even if that happened the destruction code would run
>>> *anyway* since we completed the waiting part already, it would just not necessarily be
>>> run from this thread, but that guarantee didn't exist anyway.
>>>> Then we should be able to skip patch 2 as well.
>>> If my tryreserve approach sounds sane, second patch should still be skippable. :-)
>> Sure, Lets go for that approach.
> Ok updated patch below,  you only need to check if you like the approach or not,
> since I haven't tested it yet beyond compiling. Rest of series (minus patch 2)
> should still apply without modification.
>
>      drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2
>      
>      By removing the unlocking of lru and retaking it immediately, a race is
>      removed where the bo is taken off the swap list or the lru list between
>      the unlock and relock. As such the cleanup_refs code can be simplified,
>      it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>      it will drop the locks and perform a blocking wait, or return an error
>      if no_wait_gpu was set.
>      
>      The need for looping is also eliminated, since swapout and evict_mem_first
>      will always follow the destruction path, no new fence is allowed
>      to be attached. As far as I can see this may already have been the case,
>      but the unlocking / relocking required a complicated loop to deal with
>      re-reservation.
>      
>      Changes since v1:
>       - Simplify no_wait_gpu case by folding it in with empty ddestroy.
>       - Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
>      
>      Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 202fc20..e9f01fe 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
>   	ttm_bo_mem_put(bo, &bo->mem);
>   
>   	atomic_set(&bo->reserved, 0);
> +	wake_up_all(&bo->event_queue);
>   
>   	/*
> -	 * Make processes trying to reserve really pick it up.
> +	 * Since the final reference to this bo may not be dropped by
> +	 * the current task we have to put a memory barrier here to make
> +	 * sure the changes done in this function are always visible.
> +	 *
> +	 * This function only needs protection against the final kref_put.
>   	 */
> -	smp_mb__after_atomic_dec();
> -	wake_up_all(&bo->event_queue);
> +	smp_mb__before_atomic_dec();
>   }
>   
>   static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
> @@ -543,68 +547,95 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>   }
>   
>   /**
> - * function ttm_bo_cleanup_refs
> + * function ttm_bo_cleanup_refs_and_unlock
>    * If bo idle, remove from delayed- and lru lists, and unref.
>    * If not idle, do nothing.
>    *
> + * Must be called with lru_lock and reservation held, this function
> + * will drop both before returning.
> + *
>    * @interruptible         Any sleeps should occur interruptibly.
> - * @no_wait_reserve       Never wait for reserve. Return -EBUSY instead.
>    * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
>    */
>   
> -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> -			       bool interruptible,
> -			       bool no_wait_reserve,
> -			       bool no_wait_gpu)
> +static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
> +					  bool interruptible,
> +					  bool no_wait_gpu)
>   {
>   	struct ttm_bo_device *bdev = bo->bdev;
> +	struct ttm_bo_driver *driver = bdev->driver;
>   	struct ttm_bo_global *glob = bo->glob;
>   	int put_count;
> -	int ret = 0;
> +	int ret;
>   
> -retry:
>   	spin_lock(&bdev->fence_lock);
> -	ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
> -	spin_unlock(&bdev->fence_lock);
> +	ret = ttm_bo_wait(bo, false, false, true);
>   
> -	if (unlikely(ret != 0))
> -		return ret;
> +	if (ret && !no_wait_gpu) {
> +		void *sync_obj;
>   
> -retry_reserve:
> -	spin_lock(&glob->lru_lock);
> +		/*
> +		 * Take a reference to the fence and unreserve,
> +		 * at this point the buffer should be dead, so
> +		 * no new sync objects can be attached.
> +		 */
> +		sync_obj = driver->sync_obj_ref(&bo->sync_obj);
> +		spin_unlock(&bdev->fence_lock);
>   
> -	if (unlikely(list_empty(&bo->ddestroy))) {
> +		put_count = ttm_bo_del_from_lru(bo);
> +
> +		atomic_set(&bo->reserved, 0);
> +		wake_up_all(&bo->event_queue);
>   		spin_unlock(&glob->lru_lock);
> -		return 0;
> -	}
>   
> -	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
> +		ttm_bo_list_ref_sub(bo, put_count, true);
>   
> -	if (unlikely(ret == -EBUSY)) {
> -		spin_unlock(&glob->lru_lock);
> -		if (likely(!no_wait_reserve))
> -			ret = ttm_bo_wait_unreserved(bo, interruptible);
> -		if (unlikely(ret != 0))
> +		ret = driver->sync_obj_wait(sync_obj, false, interruptible);
> +		driver->sync_obj_unref(&sync_obj);
> +		if (ret) {
> +			/*
> +			 * Either the wait returned -ERESTARTSYS, or -EDEADLK
> +			 * (radeon lockup) here. No effort is made to re-add
> +			 * this bo to any lru list. Instead the bo only
> +			 * appears on the delayed destroy list.
> +			 */
>   			return ret;
> +		}
Actually, we *must* re-add the bo to LRU lists here, because otherwise 
when a driver needs
to evict a memory type completely, there's a large chance it will miss 
this bo.

So I think either we need to keep the reservation, or keep the bo on the 
LRU lists.

/Thomas

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Maarten Lankhorst]

Op 28-11-12 20:21, Thomas Hellstrom schreef:
> On 11/28/2012 07:32 PM, Maarten Lankhorst wrote:
>> Op 28-11-12 16:10, Thomas Hellstrom schreef:
>>> On 11/28/2012 03:46 PM, Maarten Lankhorst wrote:
>>>> Op 28-11-12 15:23, Thomas Hellstrom schreef:
>>>>> On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
>>>>>> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>>>>>>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>>>>>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>>>>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>>>>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>>>>>>>> removed where the bo is taken off the swap list or the lru list between
>>>>>>>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>>>>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>>>>>>> it will drop the locks and perform a blocking wait, or return an error
>>>>>>>>>> if no_wait_gpu was set.
>>>>>>>>>>
>>>>>>>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>>>>>>>> will always follow the destruction path, so no new fence is allowed
>>>>>>>>>> to be attached. As far as I can see this may already have been the case,
>>>>>>>>>> but the unlocking / relocking required a complicated loop to deal with
>>>>>>>>>> re-reservation.
>>>>>>>>>>
>>>>>>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>>>>>>>> reservation held, so drivers must be aware that move_notify with a null
>>>>>>>>>> parameter doesn't require a reservation.
>>>>>>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>>>>>>> immediately clear from this patch.
>>>>>>>> Because we would hold the reservation while waiting and with the object still
>>>>>>>> on swap and lru lists still, that would defeat the whole purpose of keeping
>>>>>>>> the object on multiple lists, plus break current code that assumes bo's on the
>>>>>>>> those lists can always be reserved.
>>>>>>>>
>>>>>>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>>>>>>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>>>>>>>> I'm sure that would not be the case if the reservations were shared across
>>>>>>>> devices.
>>>>>>> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
>>>>>>> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
>>>>>> If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
>>>>>> leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
>>>>>> case, so not adding it back to the other lists is harmless.
>>>>>>
>>>>> Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock
>>>>> more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers,
>>>>> but it may come back and bite us.
>>>> That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in
>>>> only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen
>>>> right away, it still happens before last list ref to the bo is dropped.
>>>>
>>>> But it's your call, just choose the approach you want and I'll resubmit this. :-)
>>>>
>>>>> Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.
>>>>>
>>>>> Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use  function, and as far
>>>>> as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to
>>>>> do the same as the evict path I think without looking to deeply into the consequences that we should be safe.
>>>> I think returning success early without removing off ddestroy list if re-reserving fails
>>>> with lru lock held would be better.
>>>>
>>>> We completed the wait and attempt to reserve the bo, which failed. Without the lru
>>>> lock atomicity, this can't happen since the only places that would do it call this with
>>>> the lru lock held.
>>>>
>>>> With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete
>>>> with remove_all set to true. And even if that happened the destruction code would run
>>>> *anyway* since we completed the waiting part already, it would just not necessarily be
>>>> run from this thread, but that guarantee didn't exist anyway.
>>>>> Then we should be able to skip patch 2 as well.
>>>> If my tryreserve approach sounds sane, second patch should still be skippable. :-)
>>> Sure, Lets go for that approach.
>> Ok updated patch below,  you only need to check if you like the approach or not,
>> since I haven't tested it yet beyond compiling. Rest of series (minus patch 2)
>> should still apply without modification.
>>
>>      drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2
>>           By removing the unlocking of lru and retaking it immediately, a race is
>>      removed where the bo is taken off the swap list or the lru list between
>>      the unlock and relock. As such the cleanup_refs code can be simplified,
>>      it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>      it will drop the locks and perform a blocking wait, or return an error
>>      if no_wait_gpu was set.
>>           The need for looping is also eliminated, since swapout and evict_mem_first
>>      will always follow the destruction path, no new fence is allowed
>>      to be attached. As far as I can see this may already have been the case,
>>      but the unlocking / relocking required a complicated loop to deal with
>>      re-reservation.
>>           Changes since v1:
>>       - Simplify no_wait_gpu case by folding it in with empty ddestroy.
>>       - Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
>>           Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 202fc20..e9f01fe 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
>>       ttm_bo_mem_put(bo, &bo->mem);
>>         atomic_set(&bo->reserved, 0);
>> +    wake_up_all(&bo->event_queue);
>>         /*
>> -     * Make processes trying to reserve really pick it up.
>> +     * Since the final reference to this bo may not be dropped by
>> +     * the current task we have to put a memory barrier here to make
>> +     * sure the changes done in this function are always visible.
>> +     *
>> +     * This function only needs protection against the final kref_put.
>>        */
>> -    smp_mb__after_atomic_dec();
>> -    wake_up_all(&bo->event_queue);
>> +    smp_mb__before_atomic_dec();
>>   }
>>     static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>> @@ -543,68 +547,95 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>>   }
>>     /**
>> - * function ttm_bo_cleanup_refs
>> + * function ttm_bo_cleanup_refs_and_unlock
>>    * If bo idle, remove from delayed- and lru lists, and unref.
>>    * If not idle, do nothing.
>>    *
>> + * Must be called with lru_lock and reservation held, this function
>> + * will drop both before returning.
>> + *
>>    * @interruptible         Any sleeps should occur interruptibly.
>> - * @no_wait_reserve       Never wait for reserve. Return -EBUSY instead.
>>    * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
>>    */
>>   -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
>> -                   bool interruptible,
>> -                   bool no_wait_reserve,
>> -                   bool no_wait_gpu)
>> +static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
>> +                      bool interruptible,
>> +                      bool no_wait_gpu)
>>   {
>>       struct ttm_bo_device *bdev = bo->bdev;
>> +    struct ttm_bo_driver *driver = bdev->driver;
>>       struct ttm_bo_global *glob = bo->glob;
>>       int put_count;
>> -    int ret = 0;
>> +    int ret;
>>   -retry:
>>       spin_lock(&bdev->fence_lock);
>> -    ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
>> -    spin_unlock(&bdev->fence_lock);
>> +    ret = ttm_bo_wait(bo, false, false, true);
>>   -    if (unlikely(ret != 0))
>> -        return ret;
>> +    if (ret && !no_wait_gpu) {
>> +        void *sync_obj;
>>   -retry_reserve:
>> -    spin_lock(&glob->lru_lock);
>> +        /*
>> +         * Take a reference to the fence and unreserve,
>> +         * at this point the buffer should be dead, so
>> +         * no new sync objects can be attached.
>> +         */
>> +        sync_obj = driver->sync_obj_ref(&bo->sync_obj);
>> +        spin_unlock(&bdev->fence_lock);
>>   -    if (unlikely(list_empty(&bo->ddestroy))) {
>> +        put_count = ttm_bo_del_from_lru(bo);
>> +
>> +        atomic_set(&bo->reserved, 0);
>> +        wake_up_all(&bo->event_queue);
>>           spin_unlock(&glob->lru_lock);
>> -        return 0;
>> -    }
>>   -    ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
>> +        ttm_bo_list_ref_sub(bo, put_count, true);
>>   -    if (unlikely(ret == -EBUSY)) {
>> -        spin_unlock(&glob->lru_lock);
>> -        if (likely(!no_wait_reserve))
>> -            ret = ttm_bo_wait_unreserved(bo, interruptible);
>> -        if (unlikely(ret != 0))
>> +        ret = driver->sync_obj_wait(sync_obj, false, interruptible);
>> +        driver->sync_obj_unref(&sync_obj);
>> +        if (ret) {
>> +            /*
>> +             * Either the wait returned -ERESTARTSYS, or -EDEADLK
>> +             * (radeon lockup) here. No effort is made to re-add
>> +             * this bo to any lru list. Instead the bo only
>> +             * appears on the delayed destroy list.
>> +             */
>>               return ret;
>> +        }
> Actually, we *must* re-add the bo to LRU lists here, because otherwise when a driver needs
> to evict a memory type completely, there's a large chance it will miss this bo.
>
> So I think either we need to keep the reservation, or keep the bo on the LRU lists.
The second option is what v1 did, except I never bothered to re-take the reservation. ;-)
It shouldn't cause troubles to leave it on the lru lists if we drop the the reservation,
we can keep handling re-reservation failure in the same way as in v2.

In that case would v3 be the same as v2 of this patch, except with those 2 lines from the
ret && !no_wait_gpu branch removed:

put_count = ttm_bo_del_from_lru(bo);
ttm_bo_list_ref_sub(bo, put_count, true);

And of course the comment after sync_obj_wait failure would no longer apply.

~Maarten

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Thomas Hellstrom]

On 11/28/2012 11:26 PM, Maarten Lankhorst wrote:
> Op 28-11-12 20:21, Thomas Hellstrom schreef:
>> On 11/28/2012 07:32 PM, Maarten Lankhorst wrote:
>>> Op 28-11-12 16:10, Thomas Hellstrom schreef:
>>>> On 11/28/2012 03:46 PM, Maarten Lankhorst wrote:
>>>>> Op 28-11-12 15:23, Thomas Hellstrom schreef:
>>>>>> On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
>>>>>>> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>>>>>>>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>>>>>>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>>>>>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>>>>>>>> By removing the unlocking of lru and retaking it immediately, a race is
>>>>>>>>>>> removed where the bo is taken off the swap list or the lru list between
>>>>>>>>>>> the unlock and relock. As such the cleanup_refs code can be simplified,
>>>>>>>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>>>>>>>> it will drop the locks and perform a blocking wait, or return an error
>>>>>>>>>>> if no_wait_gpu was set.
>>>>>>>>>>>
>>>>>>>>>>> The need for looping is also eliminated, since swapout and evict_mem_first
>>>>>>>>>>> will always follow the destruction path, so no new fence is allowed
>>>>>>>>>>> to be attached. As far as I can see this may already have been the case,
>>>>>>>>>>> but the unlocking / relocking required a complicated loop to deal with
>>>>>>>>>>> re-reservation.
>>>>>>>>>>>
>>>>>>>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called with
>>>>>>>>>>> reservation held, so drivers must be aware that move_notify with a null
>>>>>>>>>>> parameter doesn't require a reservation.
>>>>>>>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>>>>>>>> immediately clear from this patch.
>>>>>>>>> Because we would hold the reservation while waiting and with the object still
>>>>>>>>> on swap and lru lists still, that would defeat the whole purpose of keeping
>>>>>>>>> the object on multiple lists, plus break current code that assumes bo's on the
>>>>>>>>> those lists can always be reserved.
>>>>>>>>>
>>>>>>>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru lock, and
>>>>>>>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed now, but
>>>>>>>>> I'm sure that would not be the case if the reservations were shared across
>>>>>>>>> devices.
>>>>>>>> The evict path removes the BO from the LRU lists, drops the LRU lock but hangs on to the reservation,
>>>>>>>> and in case the wait goes wrong, re-adds the bo to the LRU lists and returns an error.
>>>>>>> If you really want to, we could hang on to the !no_wait_gpu path, wait shouldn't ever fail there, so I suppose
>>>>>>> leaving it off the lru lists and not re-add on any list in case of wait fail is fine. It's still on the ddestroy list in that
>>>>>>> case, so not adding it back to the other lists is harmless.
>>>>>>>
>>>>>> Well I'm a bit afraid that theoretically, other callers may have a bo reserved, while cleanup_refs_and_unlock
>>>>>> more or less runs the whole destroy path on that buffer. Sure, we have control over those other reservers,
>>>>>> but it may come back and bite us.
>>>>> That's why initially I moved all the destruction to ttm_bo_release_list, to have all destruction in
>>>>> only 1 place. But even now it's serialized with the lru lock, while the destruction may not happen
>>>>> right away, it still happens before last list ref to the bo is dropped.
>>>>>
>>>>> But it's your call, just choose the approach you want and I'll resubmit this. :-)
>>>>>
>>>>>> Also the wait might fail if a signal is hit, so it's definitely possible, and even likely in the case of the X server process.
>>>>>>
>>>>>> Anyway, I prefer if we could try to keep the reservation across the ttm_cleanup_memtype_use  function, and as far
>>>>>> as I can tell, the only thing preventing that is the reservation release in the (!no_wait_gpu) path. So if we alter that to
>>>>>> do the same as the evict path I think without looking to deeply into the consequences that we should be safe.
>>>>> I think returning success early without removing off ddestroy list if re-reserving fails
>>>>> with lru lock held would be better.
>>>>>
>>>>> We completed the wait and attempt to reserve the bo, which failed. Without the lru
>>>>> lock atomicity, this can't happen since the only places that would do it call this with
>>>>> the lru lock held.
>>>>>
>>>>> With the atomicity removal, the only place that could do this is ttm_bo_delayed_delete
>>>>> with remove_all set to true. And even if that happened the destruction code would run
>>>>> *anyway* since we completed the waiting part already, it would just not necessarily be
>>>>> run from this thread, but that guarantee didn't exist anyway.
>>>>>> Then we should be able to skip patch 2 as well.
>>>>> If my tryreserve approach sounds sane, second patch should still be skippable. :-)
>>>> Sure, Lets go for that approach.
>>> Ok updated patch below,  you only need to check if you like the approach or not,
>>> since I haven't tested it yet beyond compiling. Rest of series (minus patch 2)
>>> should still apply without modification.
>>>
>>>       drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2
>>>            By removing the unlocking of lru and retaking it immediately, a race is
>>>       removed where the bo is taken off the swap list or the lru list between
>>>       the unlock and relock. As such the cleanup_refs code can be simplified,
>>>       it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>       it will drop the locks and perform a blocking wait, or return an error
>>>       if no_wait_gpu was set.
>>>            The need for looping is also eliminated, since swapout and evict_mem_first
>>>       will always follow the destruction path, no new fence is allowed
>>>       to be attached. As far as I can see this may already have been the case,
>>>       but the unlocking / relocking required a complicated loop to deal with
>>>       re-reservation.
>>>            Changes since v1:
>>>        - Simplify no_wait_gpu case by folding it in with empty ddestroy.
>>>        - Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
>>>            Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>> index 202fc20..e9f01fe 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>> @@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
>>>        ttm_bo_mem_put(bo, &bo->mem);
>>>          atomic_set(&bo->reserved, 0);
>>> +    wake_up_all(&bo->event_queue);
>>>          /*
>>> -     * Make processes trying to reserve really pick it up.
>>> +     * Since the final reference to this bo may not be dropped by
>>> +     * the current task we have to put a memory barrier here to make
>>> +     * sure the changes done in this function are always visible.
>>> +     *
>>> +     * This function only needs protection against the final kref_put.
>>>         */
>>> -    smp_mb__after_atomic_dec();
>>> -    wake_up_all(&bo->event_queue);
>>> +    smp_mb__before_atomic_dec();
>>>    }
>>>      static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>>> @@ -543,68 +547,95 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>>>    }
>>>      /**
>>> - * function ttm_bo_cleanup_refs
>>> + * function ttm_bo_cleanup_refs_and_unlock
>>>     * If bo idle, remove from delayed- and lru lists, and unref.
>>>     * If not idle, do nothing.
>>>     *
>>> + * Must be called with lru_lock and reservation held, this function
>>> + * will drop both before returning.
>>> + *
>>>     * @interruptible         Any sleeps should occur interruptibly.
>>> - * @no_wait_reserve       Never wait for reserve. Return -EBUSY instead.
>>>     * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
>>>     */
>>>    -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
>>> -                   bool interruptible,
>>> -                   bool no_wait_reserve,
>>> -                   bool no_wait_gpu)
>>> +static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
>>> +                      bool interruptible,
>>> +                      bool no_wait_gpu)
>>>    {
>>>        struct ttm_bo_device *bdev = bo->bdev;
>>> +    struct ttm_bo_driver *driver = bdev->driver;
>>>        struct ttm_bo_global *glob = bo->glob;
>>>        int put_count;
>>> -    int ret = 0;
>>> +    int ret;
>>>    -retry:
>>>        spin_lock(&bdev->fence_lock);
>>> -    ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
>>> -    spin_unlock(&bdev->fence_lock);
>>> +    ret = ttm_bo_wait(bo, false, false, true);
>>>    -    if (unlikely(ret != 0))
>>> -        return ret;
>>> +    if (ret && !no_wait_gpu) {
>>> +        void *sync_obj;
>>>    -retry_reserve:
>>> -    spin_lock(&glob->lru_lock);
>>> +        /*
>>> +         * Take a reference to the fence and unreserve,
>>> +         * at this point the buffer should be dead, so
>>> +         * no new sync objects can be attached.
>>> +         */
>>> +        sync_obj = driver->sync_obj_ref(&bo->sync_obj);
>>> +        spin_unlock(&bdev->fence_lock);
>>>    -    if (unlikely(list_empty(&bo->ddestroy))) {
>>> +        put_count = ttm_bo_del_from_lru(bo);
>>> +
>>> +        atomic_set(&bo->reserved, 0);
>>> +        wake_up_all(&bo->event_queue);
>>>            spin_unlock(&glob->lru_lock);
>>> -        return 0;
>>> -    }
>>>    -    ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
>>> +        ttm_bo_list_ref_sub(bo, put_count, true);
>>>    -    if (unlikely(ret == -EBUSY)) {
>>> -        spin_unlock(&glob->lru_lock);
>>> -        if (likely(!no_wait_reserve))
>>> -            ret = ttm_bo_wait_unreserved(bo, interruptible);
>>> -        if (unlikely(ret != 0))
>>> +        ret = driver->sync_obj_wait(sync_obj, false, interruptible);
>>> +        driver->sync_obj_unref(&sync_obj);
>>> +        if (ret) {
>>> +            /*
>>> +             * Either the wait returned -ERESTARTSYS, or -EDEADLK
>>> +             * (radeon lockup) here. No effort is made to re-add
>>> +             * this bo to any lru list. Instead the bo only
>>> +             * appears on the delayed destroy list.
>>> +             */
>>>                return ret;
>>> +        }
>> Actually, we *must* re-add the bo to LRU lists here, because otherwise when a driver needs
>> to evict a memory type completely, there's a large chance it will miss this bo.
>>
>> So I think either we need to keep the reservation, or keep the bo on the LRU lists.
> The second option is what v1 did, except I never bothered to re-take the reservation. ;-)

Yes, I know ;)

> It shouldn't cause troubles to leave it on the lru lists if we drop the the reservation,
> we can keep handling re-reservation failure in the same way as in v2.
>
> In that case would v3 be the same as v2 of this patch, except with those 2 lines from the
> ret && !no_wait_gpu branch removed:
>
> put_count = ttm_bo_del_from_lru(bo);
> ttm_bo_list_ref_sub(bo, put_count, true);
>
> And of course the comment after sync_obj_wait failure would no longer apply.

Yes, sounds good, although note that if the tryreserve fails in the 
!no_wait_gpu case, and we give up
returning 0, that may cause a similar problem (ttm_bo_force_list_clean() 
not *ensuring* that a bo was removed, at least
not by the time the function completes, but if we make sure the 
while(!list_empty()) in ttm_bo_force_list_clean() remains,
that won't be a problem either.

/Thomas

>
> ~Maarten

[PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v3

[Maarten Lankhorst]

By removing the unlocking of lru and retaking it immediately, a race is
removed where the bo is taken off the swap list or the lru list between
the unlock and relock. As such the cleanup_refs code can be simplified,
it will attempt to call ttm_bo_wait non-blockingly, and if it fails
it will drop the locks and perform a blocking wait, or return an error
if no_wait_gpu was set.

The need for looping is also eliminated, since swapout and evict_mem_first
will always follow the destruction path, no new fence is allowed
to be attached. As far as I can see this may already have been the case,
but the unlocking / relocking required a complicated loop to deal with
re-reservation.

Changes since v1:
 - Simplify no_wait_gpu case by folding it in with empty ddestroy.
 - Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
Changes since v2:
 - Do not remove bo from lru list while waiting

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 202fc20..1beed25 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 	ttm_bo_mem_put(bo, &bo->mem);
 
 	atomic_set(&bo->reserved, 0);
+	wake_up_all(&bo->event_queue);
 
 	/*
-	 * Make processes trying to reserve really pick it up.
+	 * Since the final reference to this bo may not be dropped by
+	 * the current task we have to put a memory barrier here to make
+	 * sure the changes done in this function are always visible.
+	 *
+	 * This function only needs protection against the final kref_put.
 	 */
-	smp_mb__after_atomic_dec();
-	wake_up_all(&bo->event_queue);
+	smp_mb__before_atomic_dec();
 }
 
 static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
@@ -543,68 +547,84 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 }
 
 /**
- * function ttm_bo_cleanup_refs
+ * function ttm_bo_cleanup_refs_and_unlock
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
+ * Must be called with lru_lock and reservation held, this function
+ * will drop both before returning.
+ *
  * @interruptible         Any sleeps should occur interruptibly.
- * @no_wait_reserve       Never wait for reserve. Return -EBUSY instead.
  * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
  */
 
-static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
-			       bool interruptible,
-			       bool no_wait_reserve,
-			       bool no_wait_gpu)
+static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
+					  bool interruptible,
+					  bool no_wait_gpu)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
+	struct ttm_bo_driver *driver = bdev->driver;
 	struct ttm_bo_global *glob = bo->glob;
 	int put_count;
-	int ret = 0;
+	int ret;
 
-retry:
 	spin_lock(&bdev->fence_lock);
-	ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
-	spin_unlock(&bdev->fence_lock);
+	ret = ttm_bo_wait(bo, false, false, true);
 
-	if (unlikely(ret != 0))
-		return ret;
+	if (ret && !no_wait_gpu) {
+		void *sync_obj;
 
-retry_reserve:
-	spin_lock(&glob->lru_lock);
+		/*
+		 * Take a reference to the fence and unreserve,
+		 * at this point the buffer should be dead, so
+		 * no new sync objects can be attached.
+		 */
+		sync_obj = driver->sync_obj_ref(&bo->sync_obj);
+		spin_unlock(&bdev->fence_lock);
 
-	if (unlikely(list_empty(&bo->ddestroy))) {
+		atomic_set(&bo->reserved, 0);
+		wake_up_all(&bo->event_queue);
 		spin_unlock(&glob->lru_lock);
-		return 0;
-	}
 
-	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
-
-	if (unlikely(ret == -EBUSY)) {
-		spin_unlock(&glob->lru_lock);
-		if (likely(!no_wait_reserve))
-			ret = ttm_bo_wait_unreserved(bo, interruptible);
-		if (unlikely(ret != 0))
+		ret = driver->sync_obj_wait(sync_obj, false, interruptible);
+		driver->sync_obj_unref(&sync_obj);
+		if (ret)
 			return ret;
 
-		goto retry_reserve;
-	}
+		/*
+		 * remove sync_obj with ttm_bo_wait, the wait should be
+		 * finished, and no new wait object should have been added.
+		 */
+		spin_lock(&bdev->fence_lock);
+		ret = ttm_bo_wait(bo, false, false, true);
+		WARN_ON(ret);
+		spin_unlock(&bdev->fence_lock);
+		if (ret)
+			return ret;
 
-	BUG_ON(ret != 0);
+		spin_lock(&glob->lru_lock);
+		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
 
-	/**
-	 * We can re-check for sync object without taking
-	 * the bo::lock since setting the sync object requires
-	 * also bo::reserved. A busy object at this point may
-	 * be caused by another thread recently starting an accelerated
-	 * eviction.
-	 */
+		/*
+		 * We raced, and lost, someone else holds the reservation now,
+		 * and is probably busy in ttm_bo_cleanup_memtype_use.
+		 *
+		 * Even if it's not the case, because we finished waiting any
+		 * delayed destruction would succeed, so just return success
+		 * here.
+		 */
+		if (ret) {
+			spin_unlock(&glob->lru_lock);
+			return 0;
+		}
+	} else
+		spin_unlock(&bdev->fence_lock);
 
-	if (unlikely(bo->sync_obj)) {
+	if (ret || unlikely(list_empty(&bo->ddestroy))) {
 		atomic_set(&bo->reserved, 0);
 		wake_up_all(&bo->event_queue);
 		spin_unlock(&glob->lru_lock);
-		goto retry;
+		return ret;
 	}
 
 	put_count = ttm_bo_del_from_lru(bo);
@@ -647,9 +667,13 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 			kref_get(&nentry->list_kref);
 		}
 
-		spin_unlock(&glob->lru_lock);
-		ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
-					  !remove_all);
+		ret = ttm_bo_reserve_locked(entry, false, !remove_all, false, 0);
+		if (!ret)
+			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
+							     !remove_all);
+		else
+			spin_unlock(&glob->lru_lock);
+
 		kref_put(&entry->list_kref, ttm_bo_release_list);
 		entry = nentry;
 
@@ -803,9 +827,13 @@ retry:
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		spin_unlock(&glob->lru_lock);
-		ret = ttm_bo_cleanup_refs(bo, interruptible,
-					  no_wait_reserve, no_wait_gpu);
+		ret = ttm_bo_reserve_locked(bo, interruptible, no_wait_reserve, false, 0);
+		if (!ret)
+			ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
+							     no_wait_gpu);
+		else
+			spin_unlock(&glob->lru_lock);
+
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 
 		return ret;
@@ -1799,8 +1827,9 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 		kref_get(&bo->list_kref);
 
 		if (!list_empty(&bo->ddestroy)) {
-			spin_unlock(&glob->lru_lock);
-			(void) ttm_bo_cleanup_refs(bo, false, false, false);
+			ttm_bo_reserve_locked(bo, false, false, false, 0);
+			ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+
 			kref_put(&bo->list_kref, ttm_bo_release_list);
 			spin_lock(&glob->lru_lock);
 			continue;

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v3

[Thomas Hellstrom]

On 11/29/2012 12:36 PM, Maarten Lankhorst wrote:
> By removing the unlocking of lru and retaking it immediately, a race is
> removed where the bo is taken off the swap list or the lru list between
> the unlock and relock. As such the cleanup_refs code can be simplified,
> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
> it will drop the locks and perform a blocking wait, or return an error
> if no_wait_gpu was set.
>
> The need for looping is also eliminated, since swapout and evict_mem_first
> will always follow the destruction path, no new fence is allowed
> to be attached. As far as I can see this may already have been the case,
> but the unlocking / relocking required a complicated loop to deal with
> re-reservation.
>
> Changes since v1:
>   - Simplify no_wait_gpu case by folding it in with empty ddestroy.
>   - Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
> Changes since v2:
>   - Do not remove bo from lru list while waiting
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>

Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held

[Maarten Lankhorst]

Op 28-11-12 12:54, Thomas Hellstrom schreef:
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c | 112 +++++++++++++++++++++++--------------------
>>   1 file changed, 60 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 202fc20..02b275b 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -486,14 +486,6 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
>>           bo->ttm = NULL;
>>       }
>>       ttm_bo_mem_put(bo, &bo->mem);
>> -
>> -    atomic_set(&bo->reserved, 0);
>> -
>> -    /*
>> -     * Make processes trying to reserve really pick it up.
>> -     */
>> -    smp_mb__after_atomic_dec();
>> -    wake_up_all(&bo->event_queue);
>>   }
>>     static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>> @@ -515,6 +507,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>>           put_count = ttm_bo_del_from_lru(bo);
>>             spin_unlock(&glob->lru_lock);
>> +        atomic_set(&bo->reserved, 0);
>> +        wake_up_all(&bo->event_queue);
>> +
>
> I think (although I'm not 100% sure) that if we use atomic_set() to unreserve, and it's not followed by a spin_unlock(), we need to insert
> a memory barrier, like is done above in the removed code, otherwise memory operations protected by reserve may be reordered until after reservation.
Hm yeah, looks like ttm_bo_cleanup_memtype_use probably needs a smb_mb() at the end now. The original smp_mb__after_atomic_dec was a noop,
since the wake_up_all call takes a spinlock too.

Thanks for catching it, I'll await the reply to my other email then maybe reword, fix this and resend.

~Maarten

[PATCH 4/6] drm/ttm: cope with reserved buffers on swap list in ttm_bo_swapout, v2

[Maarten Lankhorst]

Replace the while loop with a simple for each loop, and only run the
delayed destroy cleanup if we can reserve the buffer first.

No race occurs, since lru lock is never dropped any more. An empty list
and a list full of unreservable buffers both cause -EBUSY to be returned,
which is identical to the previous situation.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 44 ++++++++++++++------------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 02b275b..74b296f 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1795,41 +1795,25 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	uint32_t swap_placement = (TTM_PL_FLAG_CACHED | TTM_PL_FLAG_SYSTEM);
 
 	spin_lock(&glob->lru_lock);
-	while (ret == -EBUSY) {
-		if (unlikely(list_empty(&glob->swap_lru))) {
-			spin_unlock(&glob->lru_lock);
-			return -EBUSY;
-		}
-
-		bo = list_first_entry(&glob->swap_lru,
-				      struct ttm_buffer_object, swap);
-		kref_get(&bo->list_kref);
-
-		if (!list_empty(&bo->ddestroy)) {
-			ttm_bo_reserve_locked(bo, false, false, false, 0);
-			ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+	list_for_each_entry(bo, &glob->swap_lru, swap) {
+		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
+		if (!ret)
+			break;
+	}
 
-			kref_put(&bo->list_kref, ttm_bo_release_list);
-			spin_lock(&glob->lru_lock);
-			continue;
-		}
+	if (ret) {
+		spin_unlock(&glob->lru_lock);
+		return ret;
+	}
 
-		/**
-		 * Reserve buffer. Since we unlock while sleeping, we need
-		 * to re-check that nobody removed us from the swap-list while
-		 * we slept.
-		 */
+	kref_get(&bo->list_kref);
 
-		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
-		if (unlikely(ret == -EBUSY)) {
-			spin_unlock(&glob->lru_lock);
-			ttm_bo_wait_unreserved(bo, false);
-			kref_put(&bo->list_kref, ttm_bo_release_list);
-			spin_lock(&glob->lru_lock);
-		}
+	if (!list_empty(&bo->ddestroy)) {
+		ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+		kref_put(&bo->list_kref, ttm_bo_release_list);
+		return ret;
 	}
 
-	BUG_ON(ret != 0);
 	put_count = ttm_bo_del_from_lru(bo);
 	spin_unlock(&glob->lru_lock);
 
-- 
1.8.0

Re: [PATCH 4/6] drm/ttm: cope with reserved buffers on swap list in ttm_bo_swapout, v2

[Thomas Hellstrom]

On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
> Replace the while loop with a simple for each loop, and only run the
> delayed destroy cleanup if we can reserve the buffer first.
>
> No race occurs, since lru lock is never dropped any more. An empty list
> and a list full of unreservable buffers both cause -EBUSY to be returned,
> which is identical to the previous situation.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 44 ++++++++++++++------------------------------
>   1 file changed, 14 insertions(+), 30 deletions(-)
>
For patch 4-6

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>

[PATCH 5/6] drm/ttm: cope with reserved buffers on lru list in ttm_mem_evict_first, v2

[Maarten Lankhorst]

Replace the goto loop with a simple for each loop, and only run the
delayed destroy cleanup if we can reserve the buffer first.

No race occurs, since lru lock is never dropped any more. An empty list
and a list full of unreservable buffers both cause -EBUSY to be returned,
which is identical to the previous situation, because previously buffers
on the lru list were always guaranteed to be reservable.

This should work since currently ttm guarantees items on the lru are
always reservable, and reserving items blockingly with some bo held
are enough to cause you to run into a deadlock.

Currently this is not a concern since removal off the lru list and
reservations are always done with atomically, but when this guarantee
no longer holds, we have to handle this situation or end up with
possible deadlocks.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 42 +++++++++++-------------------------------
 1 file changed, 11 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 74b296f..ef7b2ad 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -793,49 +793,29 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	struct ttm_bo_global *glob = bdev->glob;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
 	struct ttm_buffer_object *bo;
-	int ret, put_count = 0;
+	int ret = -EBUSY, put_count;
 
-retry:
 	spin_lock(&glob->lru_lock);
-	if (list_empty(&man->lru)) {
+	list_for_each_entry(bo, &man->lru, lru) {
+		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
+		if (!ret)
+			break;
+	}
+
+	if (ret) {
 		spin_unlock(&glob->lru_lock);
-		return -EBUSY;
+		return ret;
 	}
 
-	bo = list_first_entry(&man->lru, struct ttm_buffer_object, lru);
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_reserve_locked(bo, interruptible, no_wait_reserve, false, 0);
-		if (!ret)
-			ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
-							     no_wait_gpu);
-		else
-			spin_unlock(&glob->lru_lock);
-
+		ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
+						     no_wait_gpu);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
-
 		return ret;
 	}
 
-	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
-
-	if (unlikely(ret == -EBUSY)) {
-		spin_unlock(&glob->lru_lock);
-		if (likely(!no_wait_reserve))
-			ret = ttm_bo_wait_unreserved(bo, interruptible);
-
-		kref_put(&bo->list_kref, ttm_bo_release_list);
-
-		/**
-		 * We *need* to retry after releasing the lru lock.
-		 */
-
-		if (unlikely(ret != 0))
-			return ret;
-		goto retry;
-	}
-
 	put_count = ttm_bo_del_from_lru(bo);
 	spin_unlock(&glob->lru_lock);
 
-- 
1.8.0

[PATCH 6/6] drm/ttm: remove no_wait_reserve, v3

[Maarten Lankhorst]

All items on the lru list are always reservable, so this is a stupid
thing to keep. Not only that, it is used in a way which would
guarantee deadlocks if it were ever to be set to block on reserve.

This is a lot of churn, but mostly because of the removal of the
argument which can be nested arbitrarily deeply in many places.

No change of code in this patch except removal of the no_wait_reserve
argument, the previous patch removed the use of no_wait_reserve.

v2:
 - Warn if -EBUSY is returned on reservation, all objects on the list
   should be reservable. Adjusted patch slightly due to conflicts.
v3:
 - Focus on no_wait_reserve removal only.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
---
 drivers/gpu/drm/ast/ast_ttm.c            | 10 +++---
 drivers/gpu/drm/cirrus/cirrus_ttm.c      | 10 +++---
 drivers/gpu/drm/mgag200/mgag200_ttm.c    | 10 +++---
 drivers/gpu/drm/nouveau/nouveau_bo.c     | 55 +++++++++++++++-----------------
 drivers/gpu/drm/nouveau/nouveau_bo.h     |  2 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c    |  2 +-
 drivers/gpu/drm/radeon/radeon_object.c   |  8 ++---
 drivers/gpu/drm/radeon/radeon_ttm.c      | 31 +++++++++---------
 drivers/gpu/drm/ttm/ttm_bo.c             | 46 +++++++++++++-------------
 drivers/gpu/drm/ttm/ttm_bo_util.c        |  6 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c   | 13 ++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c  |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c |  2 +-
 include/drm/ttm/ttm_bo_api.h             |  3 +-
 include/drm/ttm/ttm_bo_driver.h          | 19 ++++-------
 15 files changed, 107 insertions(+), 114 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c
index 0a54f65..3602731 100644
--- a/drivers/gpu/drm/ast/ast_ttm.c
+++ b/drivers/gpu/drm/ast/ast_ttm.c
@@ -186,11 +186,11 @@ static void ast_ttm_io_mem_free(struct ttm_bo_device *bdev, struct ttm_mem_reg *
 
 static int ast_bo_move(struct ttm_buffer_object *bo,
 		       bool evict, bool interruptible,
-		       bool no_wait_reserve, bool no_wait_gpu,
+		       bool no_wait_gpu,
 		       struct ttm_mem_reg *new_mem)
 {
 	int r;
-	r = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, new_mem);
+	r = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, new_mem);
 	return r;
 }
 
@@ -383,7 +383,7 @@ int ast_bo_pin(struct ast_bo *bo, u32 pl_flag, u64 *gpu_addr)
 	ast_ttm_placement(bo, pl_flag);
 	for (i = 0; i < bo->placement.num_placement; i++)
 		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret)
 		return ret;
 
@@ -406,7 +406,7 @@ int ast_bo_unpin(struct ast_bo *bo)
 
 	for (i = 0; i < bo->placement.num_placement ; i++)
 		bo->placements[i] &= ~TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret)
 		return ret;
 
@@ -431,7 +431,7 @@ int ast_bo_push_sysram(struct ast_bo *bo)
 	for (i = 0; i < bo->placement.num_placement ; i++)
 		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
 
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret) {
 		DRM_ERROR("pushing to VRAM failed\n");
 		return ret;
diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c b/drivers/gpu/drm/cirrus/cirrus_ttm.c
index 90d7701..1413a26 100644
--- a/drivers/gpu/drm/cirrus/cirrus_ttm.c
+++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c
@@ -186,11 +186,11 @@ static void cirrus_ttm_io_mem_free(struct ttm_bo_device *bdev, struct ttm_mem_re
 
 static int cirrus_bo_move(struct ttm_buffer_object *bo,
 		       bool evict, bool interruptible,
-		       bool no_wait_reserve, bool no_wait_gpu,
+		       bool no_wait_gpu,
 		       struct ttm_mem_reg *new_mem)
 {
 	int r;
-	r = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, new_mem);
+	r = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, new_mem);
 	return r;
 }
 
@@ -388,7 +388,7 @@ int cirrus_bo_pin(struct cirrus_bo *bo, u32 pl_flag, u64 *gpu_addr)
 	cirrus_ttm_placement(bo, pl_flag);
 	for (i = 0; i < bo->placement.num_placement; i++)
 		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret)
 		return ret;
 
@@ -411,7 +411,7 @@ int cirrus_bo_unpin(struct cirrus_bo *bo)
 
 	for (i = 0; i < bo->placement.num_placement ; i++)
 		bo->placements[i] &= ~TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret)
 		return ret;
 
@@ -436,7 +436,7 @@ int cirrus_bo_push_sysram(struct cirrus_bo *bo)
 	for (i = 0; i < bo->placement.num_placement ; i++)
 		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
 
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret) {
 		DRM_ERROR("pushing to VRAM failed\n");
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 49d60a6..8fc9d92 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -186,11 +186,11 @@ static void mgag200_ttm_io_mem_free(struct ttm_bo_device *bdev, struct ttm_mem_r
 
 static int mgag200_bo_move(struct ttm_buffer_object *bo,
 		       bool evict, bool interruptible,
-		       bool no_wait_reserve, bool no_wait_gpu,
+		       bool no_wait_gpu,
 		       struct ttm_mem_reg *new_mem)
 {
 	int r;
-	r = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, new_mem);
+	r = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, new_mem);
 	return r;
 }
 
@@ -382,7 +382,7 @@ int mgag200_bo_pin(struct mgag200_bo *bo, u32 pl_flag, u64 *gpu_addr)
 	mgag200_ttm_placement(bo, pl_flag);
 	for (i = 0; i < bo->placement.num_placement; i++)
 		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret)
 		return ret;
 
@@ -405,7 +405,7 @@ int mgag200_bo_unpin(struct mgag200_bo *bo)
 
 	for (i = 0; i < bo->placement.num_placement ; i++)
 		bo->placements[i] &= ~TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret)
 		return ret;
 
@@ -430,7 +430,7 @@ int mgag200_bo_push_sysram(struct mgag200_bo *bo)
 	for (i = 0; i < bo->placement.num_placement ; i++)
 		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
 
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false, false);
+	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 	if (ret) {
 		DRM_ERROR("pushing to VRAM failed\n");
 		return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index eaad9cc..c2df9bb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -315,7 +315,7 @@ nouveau_bo_pin(struct nouveau_bo *nvbo, uint32_t memtype)
 
 	nouveau_bo_placement_set(nvbo, memtype, 0);
 
-	ret = nouveau_bo_validate(nvbo, false, false, false);
+	ret = nouveau_bo_validate(nvbo, false, false);
 	if (ret == 0) {
 		switch (bo->mem.mem_type) {
 		case TTM_PL_VRAM:
@@ -351,7 +351,7 @@ nouveau_bo_unpin(struct nouveau_bo *nvbo)
 
 	nouveau_bo_placement_set(nvbo, bo->mem.placement, 0);
 
-	ret = nouveau_bo_validate(nvbo, false, false, false);
+	ret = nouveau_bo_validate(nvbo, false, false);
 	if (ret == 0) {
 		switch (bo->mem.mem_type) {
 		case TTM_PL_VRAM:
@@ -392,12 +392,12 @@ nouveau_bo_unmap(struct nouveau_bo *nvbo)
 
 int
 nouveau_bo_validate(struct nouveau_bo *nvbo, bool interruptible,
-		    bool no_wait_reserve, bool no_wait_gpu)
+		    bool no_wait_gpu)
 {
 	int ret;
 
-	ret = ttm_bo_validate(&nvbo->bo, &nvbo->placement, interruptible,
-			      no_wait_reserve, no_wait_gpu);
+	ret = ttm_bo_validate(&nvbo->bo, &nvbo->placement,
+			      interruptible, no_wait_gpu);
 	if (ret)
 		return ret;
 
@@ -556,8 +556,7 @@ nouveau_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement *pl)
 static int
 nouveau_bo_move_accel_cleanup(struct nouveau_channel *chan,
 			      struct nouveau_bo *nvbo, bool evict,
-			      bool no_wait_reserve, bool no_wait_gpu,
-			      struct ttm_mem_reg *new_mem)
+			      bool no_wait_gpu, struct ttm_mem_reg *new_mem)
 {
 	struct nouveau_fence *fence = NULL;
 	int ret;
@@ -567,7 +566,7 @@ nouveau_bo_move_accel_cleanup(struct nouveau_channel *chan,
 		return ret;
 
 	ret = ttm_bo_move_accel_cleanup(&nvbo->bo, fence, evict,
-					no_wait_reserve, no_wait_gpu, new_mem);
+					no_wait_gpu, new_mem);
 	nouveau_fence_unref(&fence);
 	return ret;
 }
@@ -965,8 +964,7 @@ nouveau_vma_getmap(struct nouveau_channel *chan, struct nouveau_bo *nvbo,
 
 static int
 nouveau_bo_move_m2mf(struct ttm_buffer_object *bo, int evict, bool intr,
-		     bool no_wait_reserve, bool no_wait_gpu,
-		     struct ttm_mem_reg *new_mem)
+		     bool no_wait_gpu, struct ttm_mem_reg *new_mem)
 {
 	struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
 	struct nouveau_channel *chan = chan = drm->channel;
@@ -995,7 +993,6 @@ nouveau_bo_move_m2mf(struct ttm_buffer_object *bo, int evict, bool intr,
 	ret = drm->ttm.move(chan, bo, &bo->mem, new_mem);
 	if (ret == 0) {
 		ret = nouveau_bo_move_accel_cleanup(chan, nvbo, evict,
-						    no_wait_reserve,
 						    no_wait_gpu, new_mem);
 	}
 
@@ -1064,8 +1061,7 @@ nouveau_bo_move_init(struct nouveau_drm *drm)
 
 static int
 nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr,
-		      bool no_wait_reserve, bool no_wait_gpu,
-		      struct ttm_mem_reg *new_mem)
+		      bool no_wait_gpu, struct ttm_mem_reg *new_mem)
 {
 	u32 placement_memtype = TTM_PL_FLAG_TT | TTM_PL_MASK_CACHING;
 	struct ttm_placement placement;
@@ -1078,7 +1074,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr,
 
 	tmp_mem = *new_mem;
 	tmp_mem.mm_node = NULL;
-	ret = ttm_bo_mem_space(bo, &placement, &tmp_mem, intr, no_wait_reserve, no_wait_gpu);
+	ret = ttm_bo_mem_space(bo, &placement, &tmp_mem, intr, no_wait_gpu);
 	if (ret)
 		return ret;
 
@@ -1086,11 +1082,11 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr,
 	if (ret)
 		goto out;
 
-	ret = nouveau_bo_move_m2mf(bo, true, intr, no_wait_reserve, no_wait_gpu, &tmp_mem);
+	ret = nouveau_bo_move_m2mf(bo, true, intr, no_wait_gpu, &tmp_mem);
 	if (ret)
 		goto out;
 
-	ret = ttm_bo_move_ttm(bo, true, no_wait_reserve, no_wait_gpu, new_mem);
+	ret = ttm_bo_move_ttm(bo, true, no_wait_gpu, new_mem);
 out:
 	ttm_bo_mem_put(bo, &tmp_mem);
 	return ret;
@@ -1098,8 +1094,7 @@ out:
 
 static int
 nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr,
-		      bool no_wait_reserve, bool no_wait_gpu,
-		      struct ttm_mem_reg *new_mem)
+		      bool no_wait_gpu, struct ttm_mem_reg *new_mem)
 {
 	u32 placement_memtype = TTM_PL_FLAG_TT | TTM_PL_MASK_CACHING;
 	struct ttm_placement placement;
@@ -1112,15 +1107,15 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr,
 
 	tmp_mem = *new_mem;
 	tmp_mem.mm_node = NULL;
-	ret = ttm_bo_mem_space(bo, &placement, &tmp_mem, intr, no_wait_reserve, no_wait_gpu);
+	ret = ttm_bo_mem_space(bo, &placement, &tmp_mem, intr, no_wait_gpu);
 	if (ret)
 		return ret;
 
-	ret = ttm_bo_move_ttm(bo, true, no_wait_reserve, no_wait_gpu, &tmp_mem);
+	ret = ttm_bo_move_ttm(bo, true, no_wait_gpu, &tmp_mem);
 	if (ret)
 		goto out;
 
-	ret = nouveau_bo_move_m2mf(bo, true, intr, no_wait_reserve, no_wait_gpu, new_mem);
+	ret = nouveau_bo_move_m2mf(bo, true, intr, no_wait_gpu, new_mem);
 	if (ret)
 		goto out;
 
@@ -1195,8 +1190,7 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo,
 
 static int
 nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, bool intr,
-		bool no_wait_reserve, bool no_wait_gpu,
-		struct ttm_mem_reg *new_mem)
+		bool no_wait_gpu, struct ttm_mem_reg *new_mem)
 {
 	struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
 	struct nouveau_bo *nvbo = nouveau_bo(bo);
@@ -1220,23 +1214,26 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, bool intr,
 
 	/* CPU copy if we have no accelerated method available */
 	if (!drm->ttm.move) {
-		ret = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, new_mem);
+		ret = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, new_mem);
 		goto out;
 	}
 
 	/* Hardware assisted copy. */
 	if (new_mem->mem_type == TTM_PL_SYSTEM)
-		ret = nouveau_bo_move_flipd(bo, evict, intr, no_wait_reserve, no_wait_gpu, new_mem);
+		ret = nouveau_bo_move_flipd(bo, evict, intr,
+					    no_wait_gpu, new_mem);
 	else if (old_mem->mem_type == TTM_PL_SYSTEM)
-		ret = nouveau_bo_move_flips(bo, evict, intr, no_wait_reserve, no_wait_gpu, new_mem);
+		ret = nouveau_bo_move_flips(bo, evict, intr,
+					    no_wait_gpu, new_mem);
 	else
-		ret = nouveau_bo_move_m2mf(bo, evict, intr, no_wait_reserve, no_wait_gpu, new_mem);
+		ret = nouveau_bo_move_m2mf(bo, evict, intr,
+					   no_wait_gpu, new_mem);
 
 	if (!ret)
 		goto out;
 
 	/* Fallback to software copy. */
-	ret = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, new_mem);
+	ret = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, new_mem);
 
 out:
 	if (nv_device(drm->device)->card_type < NV_50) {
@@ -1343,7 +1340,7 @@ nouveau_ttm_fault_reserve_notify(struct ttm_buffer_object *bo)
 	nvbo->placement.fpfn = 0;
 	nvbo->placement.lpfn = mappable;
 	nouveau_bo_placement_set(nvbo, TTM_PL_FLAG_VRAM, 0);
-	return nouveau_bo_validate(nvbo, false, true, false);
+	return nouveau_bo_validate(nvbo, false, false);
 }
 
 static int
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h b/drivers/gpu/drm/nouveau/nouveau_bo.h
index 61b8980..689b59b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.h
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.h
@@ -76,7 +76,7 @@ u32  nouveau_bo_rd32(struct nouveau_bo *, unsigned index);
 void nouveau_bo_wr32(struct nouveau_bo *, unsigned index, u32 val);
 void nouveau_bo_fence(struct nouveau_bo *, struct nouveau_fence *);
 int  nouveau_bo_validate(struct nouveau_bo *, bool interruptible,
-			 bool no_wait_reserve, bool no_wait_gpu);
+			 bool no_wait_gpu);
 
 struct nouveau_vma *
 nouveau_bo_vma_find(struct nouveau_bo *, struct nouveau_vm *);
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 6d8391d..7b9364b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -434,7 +434,7 @@ validate_list(struct nouveau_channel *chan, struct list_head *list,
 			return ret;
 		}
 
-		ret = nouveau_bo_validate(nvbo, true, false, false);
+		ret = nouveau_bo_validate(nvbo, true, false);
 		if (unlikely(ret)) {
 			if (ret != -ERESTARTSYS)
 				NV_ERROR(drm, "fail ttm_validate\n");
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 69c8811..b97b7c6 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -240,7 +240,7 @@ int radeon_bo_pin_restricted(struct radeon_bo *bo, u32 domain, u64 max_offset,
 	}
 	for (i = 0; i < bo->placement.num_placement; i++)
 		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
-	r = ttm_bo_validate(&bo->tbo, &bo->placement, false, false, false);
+	r = ttm_bo_validate(&bo->tbo, &bo->placement, false, false);
 	if (likely(r == 0)) {
 		bo->pin_count = 1;
 		if (gpu_addr != NULL)
@@ -269,7 +269,7 @@ int radeon_bo_unpin(struct radeon_bo *bo)
 		return 0;
 	for (i = 0; i < bo->placement.num_placement; i++)
 		bo->placements[i] &= ~TTM_PL_FLAG_NO_EVICT;
-	r = ttm_bo_validate(&bo->tbo, &bo->placement, false, false, false);
+	r = ttm_bo_validate(&bo->tbo, &bo->placement, false, false);
 	if (unlikely(r != 0))
 		dev_err(bo->rdev->dev, "%p validate failed for unpin\n", bo);
 	return r;
@@ -355,7 +355,7 @@ int radeon_bo_list_validate(struct list_head *head)
 		retry:
 			radeon_ttm_placement_from_domain(bo, domain);
 			r = ttm_bo_validate(&bo->tbo, &bo->placement,
-						true, false, false);
+						true, false);
 			if (unlikely(r)) {
 				if (r != -ERESTARTSYS && domain == RADEON_GEM_DOMAIN_VRAM) {
 					domain |= RADEON_GEM_DOMAIN_GTT;
@@ -575,7 +575,7 @@ int radeon_bo_fault_reserve_notify(struct ttm_buffer_object *bo)
 			/* hurrah the memory is not visible ! */
 			radeon_ttm_placement_from_domain(rbo, RADEON_GEM_DOMAIN_VRAM);
 			rbo->placement.lpfn = rdev->mc.visible_vram_size >> PAGE_SHIFT;
-			r = ttm_bo_validate(bo, &rbo->placement, false, true, false);
+			r = ttm_bo_validate(bo, &rbo->placement, false, false);
 			if (unlikely(r != 0))
 				return r;
 			offset = bo->mem.start << PAGE_SHIFT;
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 563c8ed..1d8ff2f 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -216,7 +216,7 @@ static void radeon_move_null(struct ttm_buffer_object *bo,
 }
 
 static int radeon_move_blit(struct ttm_buffer_object *bo,
-			bool evict, int no_wait_reserve, bool no_wait_gpu,
+			bool evict, bool no_wait_gpu,
 			struct ttm_mem_reg *new_mem,
 			struct ttm_mem_reg *old_mem)
 {
@@ -266,14 +266,14 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
 			&fence);
 	/* FIXME: handle copy error */
 	r = ttm_bo_move_accel_cleanup(bo, (void *)fence,
-				      evict, no_wait_reserve, no_wait_gpu, new_mem);
+				      evict, no_wait_gpu, new_mem);
 	radeon_fence_unref(&fence);
 	return r;
 }
 
 static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
 				bool evict, bool interruptible,
-				bool no_wait_reserve, bool no_wait_gpu,
+				bool no_wait_gpu,
 				struct ttm_mem_reg *new_mem)
 {
 	struct radeon_device *rdev;
@@ -294,7 +294,7 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
 	placement.busy_placement = &placements;
 	placements = TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT;
 	r = ttm_bo_mem_space(bo, &placement, &tmp_mem,
-			     interruptible, no_wait_reserve, no_wait_gpu);
+			     interruptible, no_wait_gpu);
 	if (unlikely(r)) {
 		return r;
 	}
@@ -308,11 +308,11 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
-	r = radeon_move_blit(bo, true, no_wait_reserve, no_wait_gpu, &tmp_mem, old_mem);
+	r = radeon_move_blit(bo, true, no_wait_gpu, &tmp_mem, old_mem);
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
-	r = ttm_bo_move_ttm(bo, true, no_wait_reserve, no_wait_gpu, new_mem);
+	r = ttm_bo_move_ttm(bo, true, no_wait_gpu, new_mem);
 out_cleanup:
 	ttm_bo_mem_put(bo, &tmp_mem);
 	return r;
@@ -320,7 +320,7 @@ out_cleanup:
 
 static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
 				bool evict, bool interruptible,
-				bool no_wait_reserve, bool no_wait_gpu,
+				bool no_wait_gpu,
 				struct ttm_mem_reg *new_mem)
 {
 	struct radeon_device *rdev;
@@ -340,15 +340,16 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
 	placement.num_busy_placement = 1;
 	placement.busy_placement = &placements;
 	placements = TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT;
-	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, interruptible, no_wait_reserve, no_wait_gpu);
+	r = ttm_bo_mem_space(bo, &placement, &tmp_mem,
+			     interruptible, no_wait_gpu);
 	if (unlikely(r)) {
 		return r;
 	}
-	r = ttm_bo_move_ttm(bo, true, no_wait_reserve, no_wait_gpu, &tmp_mem);
+	r = ttm_bo_move_ttm(bo, true, no_wait_gpu, &tmp_mem);
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
-	r = radeon_move_blit(bo, true, no_wait_reserve, no_wait_gpu, new_mem, old_mem);
+	r = radeon_move_blit(bo, true, no_wait_gpu, new_mem, old_mem);
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
@@ -359,7 +360,7 @@ out_cleanup:
 
 static int radeon_bo_move(struct ttm_buffer_object *bo,
 			bool evict, bool interruptible,
-			bool no_wait_reserve, bool no_wait_gpu,
+			bool no_wait_gpu,
 			struct ttm_mem_reg *new_mem)
 {
 	struct radeon_device *rdev;
@@ -388,18 +389,18 @@ static int radeon_bo_move(struct ttm_buffer_object *bo,
 	if (old_mem->mem_type == TTM_PL_VRAM &&
 	    new_mem->mem_type == TTM_PL_SYSTEM) {
 		r = radeon_move_vram_ram(bo, evict, interruptible,
-					no_wait_reserve, no_wait_gpu, new_mem);
+					no_wait_gpu, new_mem);
 	} else if (old_mem->mem_type == TTM_PL_SYSTEM &&
 		   new_mem->mem_type == TTM_PL_VRAM) {
 		r = radeon_move_ram_vram(bo, evict, interruptible,
-					    no_wait_reserve, no_wait_gpu, new_mem);
+					    no_wait_gpu, new_mem);
 	} else {
-		r = radeon_move_blit(bo, evict, no_wait_reserve, no_wait_gpu, new_mem, old_mem);
+		r = radeon_move_blit(bo, evict, no_wait_gpu, new_mem, old_mem);
 	}
 
 	if (r) {
 memcpy:
-		r = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, new_mem);
+		r = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, new_mem);
 	}
 	return r;
 }
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index ef7b2ad..818d806 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -366,7 +366,7 @@ static int ttm_bo_add_ttm(struct ttm_buffer_object *bo, bool zero_alloc)
 static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
 				  struct ttm_mem_reg *mem,
 				  bool evict, bool interruptible,
-				  bool no_wait_reserve, bool no_wait_gpu)
+				  bool no_wait_gpu)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 	bool old_is_pci = ttm_mem_reg_is_pci(bdev, &bo->mem);
@@ -420,12 +420,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
 
 	if (!(old_man->flags & TTM_MEMTYPE_FLAG_FIXED) &&
 	    !(new_man->flags & TTM_MEMTYPE_FLAG_FIXED))
-		ret = ttm_bo_move_ttm(bo, evict, no_wait_reserve, no_wait_gpu, mem);
+		ret = ttm_bo_move_ttm(bo, evict, no_wait_gpu, mem);
 	else if (bdev->driver->move)
 		ret = bdev->driver->move(bo, evict, interruptible,
-					 no_wait_reserve, no_wait_gpu, mem);
+					 no_wait_gpu, mem);
 	else
-		ret = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, mem);
+		ret = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, mem);
 
 	if (ret) {
 		if (bdev->driver->move_notify) {
@@ -731,7 +731,7 @@ void ttm_bo_unlock_delayed_workqueue(struct ttm_bo_device *bdev, int resched)
 EXPORT_SYMBOL(ttm_bo_unlock_delayed_workqueue);
 
 static int ttm_bo_evict(struct ttm_buffer_object *bo, bool interruptible,
-			bool no_wait_reserve, bool no_wait_gpu)
+			bool no_wait_gpu)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 	struct ttm_mem_reg evict_mem;
@@ -762,7 +762,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, bool interruptible,
 	placement.num_busy_placement = 0;
 	bdev->driver->evict_flags(bo, &placement);
 	ret = ttm_bo_mem_space(bo, &placement, &evict_mem, interruptible,
-				no_wait_reserve, no_wait_gpu);
+				no_wait_gpu);
 	if (ret) {
 		if (ret != -ERESTARTSYS) {
 			pr_err("Failed to find memory space for buffer 0x%p eviction\n",
@@ -773,7 +773,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, bool interruptible,
 	}
 
 	ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, interruptible,
-				     no_wait_reserve, no_wait_gpu);
+				     no_wait_gpu);
 	if (ret) {
 		if (ret != -ERESTARTSYS)
 			pr_err("Buffer eviction failed\n");
@@ -787,7 +787,7 @@ out:
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 				uint32_t mem_type,
-				bool interruptible, bool no_wait_reserve,
+				bool interruptible,
 				bool no_wait_gpu)
 {
 	struct ttm_bo_global *glob = bdev->glob;
@@ -823,7 +823,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 
 	ttm_bo_list_ref_sub(bo, put_count, true);
 
-	ret = ttm_bo_evict(bo, interruptible, no_wait_reserve, no_wait_gpu);
+	ret = ttm_bo_evict(bo, interruptible, no_wait_gpu);
 	ttm_bo_unreserve(bo);
 
 	kref_put(&bo->list_kref, ttm_bo_release_list);
@@ -848,7 +848,6 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 					struct ttm_placement *placement,
 					struct ttm_mem_reg *mem,
 					bool interruptible,
-					bool no_wait_reserve,
 					bool no_wait_gpu)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
@@ -861,8 +860,8 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 			return ret;
 		if (mem->mm_node)
 			break;
-		ret = ttm_mem_evict_first(bdev, mem_type, interruptible,
-						no_wait_reserve, no_wait_gpu);
+		ret = ttm_mem_evict_first(bdev, mem_type,
+					  interruptible, no_wait_gpu);
 		if (unlikely(ret != 0))
 			return ret;
 	} while (1);
@@ -927,7 +926,7 @@ static bool ttm_bo_mt_compatible(struct ttm_mem_type_manager *man,
 int ttm_bo_mem_space(struct ttm_buffer_object *bo,
 			struct ttm_placement *placement,
 			struct ttm_mem_reg *mem,
-			bool interruptible, bool no_wait_reserve,
+			bool interruptible,
 			bool no_wait_gpu)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
@@ -1018,7 +1017,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
 		}
 
 		ret = ttm_bo_mem_force_space(bo, mem_type, placement, mem,
-						interruptible, no_wait_reserve, no_wait_gpu);
+						interruptible, no_wait_gpu);
 		if (ret == 0 && mem->mm_node) {
 			mem->placement = cur_flags;
 			return 0;
@@ -1033,7 +1032,7 @@ EXPORT_SYMBOL(ttm_bo_mem_space);
 
 int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
 			struct ttm_placement *placement,
-			bool interruptible, bool no_wait_reserve,
+			bool interruptible,
 			bool no_wait_gpu)
 {
 	int ret = 0;
@@ -1060,10 +1059,12 @@ int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
 	/*
 	 * Determine where to move the buffer.
 	 */
-	ret = ttm_bo_mem_space(bo, placement, &mem, interruptible, no_wait_reserve, no_wait_gpu);
+	ret = ttm_bo_mem_space(bo, placement, &mem,
+			       interruptible, no_wait_gpu);
 	if (ret)
 		goto out_unlock;
-	ret = ttm_bo_handle_move_mem(bo, &mem, false, interruptible, no_wait_reserve, no_wait_gpu);
+	ret = ttm_bo_handle_move_mem(bo, &mem, false,
+				     interruptible, no_wait_gpu);
 out_unlock:
 	if (ret && mem.mm_node)
 		ttm_bo_mem_put(bo, &mem);
@@ -1092,7 +1093,7 @@ static int ttm_bo_mem_compat(struct ttm_placement *placement,
 
 int ttm_bo_validate(struct ttm_buffer_object *bo,
 			struct ttm_placement *placement,
-			bool interruptible, bool no_wait_reserve,
+			bool interruptible,
 			bool no_wait_gpu)
 {
 	int ret;
@@ -1108,7 +1109,8 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
 	 */
 	ret = ttm_bo_mem_compat(placement, &bo->mem);
 	if (ret < 0) {
-		ret = ttm_bo_move_buffer(bo, placement, interruptible, no_wait_reserve, no_wait_gpu);
+		ret = ttm_bo_move_buffer(bo, placement, interruptible,
+					 no_wait_gpu);
 		if (ret)
 			return ret;
 	} else {
@@ -1221,7 +1223,7 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
 			goto out_err;
 	}
 
-	ret = ttm_bo_validate(bo, placement, interruptible, false, false);
+	ret = ttm_bo_validate(bo, placement, interruptible, false);
 	if (ret)
 		goto out_err;
 
@@ -1307,7 +1309,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	spin_lock(&glob->lru_lock);
 	while (!list_empty(&man->lru)) {
 		spin_unlock(&glob->lru_lock);
-		ret = ttm_mem_evict_first(bdev, mem_type, false, false, false);
+		ret = ttm_mem_evict_first(bdev, mem_type, false, false);
 		if (ret) {
 			if (allow_errors) {
 				return ret;
@@ -1819,7 +1821,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 		evict_mem.mem_type = TTM_PL_SYSTEM;
 
 		ret = ttm_bo_handle_move_mem(bo, &evict_mem, true,
-					     false, false, false);
+					     false, false);
 		if (unlikely(ret != 0))
 			goto out;
 	}
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index b9c4e51..9e9c5d2 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -43,7 +43,7 @@ void ttm_bo_free_old_node(struct ttm_buffer_object *bo)
 }
 
 int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
-		    bool evict, bool no_wait_reserve,
+		    bool evict,
 		    bool no_wait_gpu, struct ttm_mem_reg *new_mem)
 {
 	struct ttm_tt *ttm = bo->ttm;
@@ -314,7 +314,7 @@ static int ttm_copy_ttm_io_page(struct ttm_tt *ttm, void *dst,
 }
 
 int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
-		       bool evict, bool no_wait_reserve, bool no_wait_gpu,
+		       bool evict, bool no_wait_gpu,
 		       struct ttm_mem_reg *new_mem)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
@@ -611,7 +611,7 @@ EXPORT_SYMBOL(ttm_bo_kunmap);
 
 int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
 			      void *sync_obj,
-			      bool evict, bool no_wait_reserve,
+			      bool evict,
 			      bool no_wait_gpu,
 			      struct ttm_mem_reg *new_mem)
 {
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c
index 655d57f..7a9b3e3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c
@@ -66,7 +66,7 @@ int vmw_dmabuf_to_placement(struct vmw_private *dev_priv,
 	if (unlikely(ret != 0))
 		goto err;
 
-	ret = ttm_bo_validate(bo, placement, interruptible, false, false);
+	ret = ttm_bo_validate(bo, placement, interruptible, false);
 
 	ttm_bo_unreserve(bo);
 
@@ -123,7 +123,7 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv,
 	else
 		placement = &vmw_vram_gmr_placement;
 
-	ret = ttm_bo_validate(bo, placement, interruptible, false, false);
+	ret = ttm_bo_validate(bo, placement, interruptible, false);
 	if (likely(ret == 0) || ret == -ERESTARTSYS)
 		goto err_unreserve;
 
@@ -138,7 +138,7 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv,
 	else
 		placement = &vmw_vram_placement;
 
-	ret = ttm_bo_validate(bo, placement, interruptible, false, false);
+	ret = ttm_bo_validate(bo, placement, interruptible, false);
 
 err_unreserve:
 	ttm_bo_unreserve(bo);
@@ -223,10 +223,9 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv,
 	if (bo->mem.mem_type == TTM_PL_VRAM &&
 	    bo->mem.start < bo->num_pages &&
 	    bo->mem.start > 0)
-		(void) ttm_bo_validate(bo, &vmw_sys_placement, false,
-				       false, false);
+		(void) ttm_bo_validate(bo, &vmw_sys_placement, false, false);
 
-	ret = ttm_bo_validate(bo, &placement, interruptible, false, false);
+	ret = ttm_bo_validate(bo, &placement, interruptible, false);
 
 	/* For some reason we didn't up at the start of vram */
 	WARN_ON(ret == 0 && bo->offset != 0);
@@ -315,7 +314,7 @@ void vmw_bo_pin(struct ttm_buffer_object *bo, bool pin)
 	placement.num_placement = 1;
 	placement.placement = &pl_flags;
 
-	ret = ttm_bo_validate(bo, &placement, false, true, true);
+	ret = ttm_bo_validate(bo, &placement, false, true);
 
 	BUG_ON(ret != 0 || bo->mem.mem_type != old_mem_type);
 }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 534c967..394e647 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -1245,7 +1245,7 @@ static int vmw_validate_single_buffer(struct vmw_private *dev_priv,
 	 * used as a GMR, this will return -ENOMEM.
 	 */
 
-	ret = ttm_bo_validate(bo, &vmw_vram_gmr_placement, true, false, false);
+	ret = ttm_bo_validate(bo, &vmw_vram_gmr_placement, true, false);
 	if (likely(ret == 0 || ret == -ERESTARTSYS))
 		return ret;
 
@@ -1255,7 +1255,7 @@ static int vmw_validate_single_buffer(struct vmw_private *dev_priv,
 	 */
 
 	DRM_INFO("Falling through to VRAM.\n");
-	ret = ttm_bo_validate(bo, &vmw_vram_placement, true, false, false);
+	ret = ttm_bo_validate(bo, &vmw_vram_placement, true, false);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index 88b6f92..93dffe3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -998,7 +998,7 @@ int vmw_resource_check_buffer(struct vmw_resource *res,
 	backup_dirty = res->backup_dirty;
 	ret = ttm_bo_validate(&res->backup->base,
 			      res->func->backup_placement,
-			      true, false, false);
+			      true, false);
 
 	if (unlikely(ret != 0))
 		goto out_no_validate;
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index c6cae73..3cb5d84 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -337,7 +337,6 @@ extern int ttm_bo_wait(struct ttm_buffer_object *bo, bool lazy,
  * @bo: The buffer object.
  * @placement: Proposed placement for the buffer object.
  * @interruptible: Sleep interruptible if sleeping.
- * @no_wait_reserve: Return immediately if other buffers are busy.
  * @no_wait_gpu: Return immediately if the GPU is busy.
  *
  * Changes placement and caching policy of the buffer object
@@ -350,7 +349,7 @@ extern int ttm_bo_wait(struct ttm_buffer_object *bo, bool lazy,
  */
 extern int ttm_bo_validate(struct ttm_buffer_object *bo,
 				struct ttm_placement *placement,
-				bool interruptible, bool no_wait_reserve,
+				bool interruptible,
 				bool no_wait_gpu);
 
 /**
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index dd96442..e3a43a4 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -394,7 +394,7 @@ struct ttm_bo_driver {
 	 */
 	int (*move) (struct ttm_buffer_object *bo,
 		     bool evict, bool interruptible,
-		     bool no_wait_reserve, bool no_wait_gpu,
+		     bool no_wait_gpu,
 		     struct ttm_mem_reg *new_mem);
 
 	/**
@@ -703,7 +703,6 @@ extern bool ttm_mem_reg_is_pci(struct ttm_bo_device *bdev,
  * @proposed_placement: Proposed new placement for the buffer object.
  * @mem: A struct ttm_mem_reg.
  * @interruptible: Sleep interruptible when sliping.
- * @no_wait_reserve: Return immediately if other buffers are busy.
  * @no_wait_gpu: Return immediately if the GPU is busy.
  *
  * Allocate memory space for the buffer object pointed to by @bo, using
@@ -719,7 +718,7 @@ extern int ttm_bo_mem_space(struct ttm_buffer_object *bo,
 				struct ttm_placement *placement,
 				struct ttm_mem_reg *mem,
 				bool interruptible,
-				bool no_wait_reserve, bool no_wait_gpu);
+				bool no_wait_gpu);
 
 extern void ttm_bo_mem_put(struct ttm_buffer_object *bo,
 			   struct ttm_mem_reg *mem);
@@ -901,7 +900,6 @@ extern int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo,
  *
  * @bo: A pointer to a struct ttm_buffer_object.
  * @evict: 1: This is an eviction. Don't try to pipeline.
- * @no_wait_reserve: Return immediately if other buffers are busy.
  * @no_wait_gpu: Return immediately if the GPU is busy.
  * @new_mem: struct ttm_mem_reg indicating where to move.
  *
@@ -916,15 +914,14 @@ extern int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo,
  */
 
 extern int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
-			   bool evict, bool no_wait_reserve,
-			   bool no_wait_gpu, struct ttm_mem_reg *new_mem);
+			   bool evict, bool no_wait_gpu,
+			   struct ttm_mem_reg *new_mem);
 
 /**
  * ttm_bo_move_memcpy
  *
  * @bo: A pointer to a struct ttm_buffer_object.
  * @evict: 1: This is an eviction. Don't try to pipeline.
- * @no_wait_reserve: Return immediately if other buffers are busy.
  * @no_wait_gpu: Return immediately if the GPU is busy.
  * @new_mem: struct ttm_mem_reg indicating where to move.
  *
@@ -939,8 +936,8 @@ extern int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
  */
 
 extern int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
-			      bool evict, bool no_wait_reserve,
-			      bool no_wait_gpu, struct ttm_mem_reg *new_mem);
+			      bool evict, bool no_wait_gpu,
+			      struct ttm_mem_reg *new_mem);
 
 /**
  * ttm_bo_free_old_node
@@ -957,7 +954,6 @@ extern void ttm_bo_free_old_node(struct ttm_buffer_object *bo);
  * @bo: A pointer to a struct ttm_buffer_object.
  * @sync_obj: A sync object that signals when moving is complete.
  * @evict: This is an evict move. Don't return until the buffer is idle.
- * @no_wait_reserve: Return immediately if other buffers are busy.
  * @no_wait_gpu: Return immediately if the GPU is busy.
  * @new_mem: struct ttm_mem_reg indicating where to move.
  *
@@ -971,8 +967,7 @@ extern void ttm_bo_free_old_node(struct ttm_buffer_object *bo);
 
 extern int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
 				     void *sync_obj,
-				     bool evict, bool no_wait_reserve,
-				     bool no_wait_gpu,
+				     bool evict, bool no_wait_gpu,
 				     struct ttm_mem_reg *new_mem);
 /**
  * ttm_io_prot
-- 
1.8.0

Re: [PATCH 1/6] drm/ttm: change fence_lock to inner lock

[Thomas Hellstrom]

On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
> This requires changing the order in ttm_bo_cleanup_refs_or_queue to
> take the reservation first, as there is otherwise no race free way to
> take lru lock before fence_lock.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

Technically I think it would be possible to avoid reserving in the case 
the bo is still busy, and re-checking
for sync_obj presence once we have reserved (since we release the fence 
lock).

But that's an optimization. The important thing at this point is to get 
this right.

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>



> ---
>   drivers/gpu/drm/ttm/ttm_bo.c           | 31 +++++++++++--------------------
>   drivers/gpu/drm/ttm/ttm_execbuf_util.c |  4 ++--
>   2 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 7426fe5..202fc20 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -500,27 +500,17 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>   {
>   	struct ttm_bo_device *bdev = bo->bdev;
>   	struct ttm_bo_global *glob = bo->glob;
> -	struct ttm_bo_driver *driver;
> +	struct ttm_bo_driver *driver = bdev->driver;
>   	void *sync_obj = NULL;
>   	int put_count;
>   	int ret;
>   
> +	spin_lock(&glob->lru_lock);
> +	ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
> +
>   	spin_lock(&bdev->fence_lock);
>   	(void) ttm_bo_wait(bo, false, false, true);
> -	if (!bo->sync_obj) {
> -
> -		spin_lock(&glob->lru_lock);
> -
> -		/**
> -		 * Lock inversion between bo:reserve and bdev::fence_lock here,
> -		 * but that's OK, since we're only trylocking.
> -		 */
> -
> -		ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
> -
> -		if (unlikely(ret == -EBUSY))
> -			goto queue;
> -
> +	if (!ret && !bo->sync_obj) {
>   		spin_unlock(&bdev->fence_lock);
>   		put_count = ttm_bo_del_from_lru(bo);
>   
> @@ -530,18 +520,19 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>   		ttm_bo_list_ref_sub(bo, put_count, true);
>   
>   		return;
> -	} else {
> -		spin_lock(&glob->lru_lock);
>   	}
> -queue:
> -	driver = bdev->driver;
>   	if (bo->sync_obj)
>   		sync_obj = driver->sync_obj_ref(bo->sync_obj);
> +	spin_unlock(&bdev->fence_lock);
> +
> +	if (!ret) {
> +		atomic_set(&bo->reserved, 0);
> +		wake_up_all(&bo->event_queue);
> +	}
>   
>   	kref_get(&bo->list_kref);
>   	list_add_tail(&bo->ddestroy, &bdev->ddestroy);
>   	spin_unlock(&glob->lru_lock);
> -	spin_unlock(&bdev->fence_lock);
>   
>   	if (sync_obj) {
>   		driver->sync_obj_flush(sync_obj);
> diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
> index 1986d00..cd9e452 100644
> --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
> @@ -213,8 +213,8 @@ void ttm_eu_fence_buffer_objects(struct list_head *list, void *sync_obj)
>   	driver = bdev->driver;
>   	glob = bo->glob;
>   
> -	spin_lock(&bdev->fence_lock);
>   	spin_lock(&glob->lru_lock);
> +	spin_lock(&bdev->fence_lock);
>   
>   	list_for_each_entry(entry, list, head) {
>   		bo = entry->bo;
> @@ -223,8 +223,8 @@ void ttm_eu_fence_buffer_objects(struct list_head *list, void *sync_obj)
>   		ttm_bo_unreserve_locked(bo);
>   		entry->reserved = false;
>   	}
> -	spin_unlock(&glob->lru_lock);
>   	spin_unlock(&bdev->fence_lock);
> +	spin_unlock(&glob->lru_lock);
>   
>   	list_for_each_entry(entry, list, head) {
>   		if (entry->old_sync_obj)