[PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Roger He]

on_alloc_stage: is this operation on allocation stage
resv: reservation bo used of this operation

Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
Signed-off-by: Roger He <Hongbo.He@amd.com>
---
 include/drm/ttm/ttm_bo_api.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 368eb02..25de597 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
  *
  * @interruptible: Sleep interruptible if sleeping.
  * @no_wait_gpu: Return immediately if the GPU is busy.
+ * @on_alloc_stage: is this operation on allocation stage
+ * @resv: resvation bo used
  *
  * Context for TTM operations like changing buffer placement or general memory
  * allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
 struct ttm_operation_ctx {
 	bool interruptible;
 	bool no_wait_gpu;
+	bool on_alloc_stage;
+	struct reservation_object *resv;
 	uint64_t bytes_moved;
 };
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] drm/ttm: when create a new ttm bo init on_alloc_stage and resv for ttm_operation_ctx

[Roger He]

Change-Id: I0c6571c2a64e6c5bdad80ccbcccb40eba1c20b4e
Signed-off-by: Roger He <Hongbo.He@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
 drivers/gpu/drm/ttm/ttm_bo.c               | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index dc0a8be..eb42782 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -327,7 +327,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
 			       uint64_t init_value,
 			       struct amdgpu_bo **bo_ptr)
 {
-	struct ttm_operation_ctx ctx = { !kernel, false };
+	struct ttm_operation_ctx ctx = { !kernel, false, true };
 	struct amdgpu_bo *bo;
 	enum ttm_bo_type type;
 	unsigned long page_align;
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 97c3da6..70b2673 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1208,6 +1208,7 @@ int ttm_bo_init_reserved(struct ttm_bo_device *bdev,
 		WARN_ON(!locked);
 	}
 
+	ctx->resv = resv;
 	if (likely(!ret))
 		ret = ttm_bo_validate(bo, placement, ctx);
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/ttm: use an ttm operation ctx for ttm_bo_move_xxx

[Roger He]

include ttm_bo_move_memcpy and ttm_bo_move_ttm

Change-Id: I160b2fe1da3200405810d0215c4521b5f0d3615a
Signed-off-by: Roger He <Hongbo.He@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++----
 drivers/gpu/drm/nouveau/nouveau_bo.c    | 7 +++----
 drivers/gpu/drm/qxl/qxl_ttm.c           | 3 +--
 drivers/gpu/drm/radeon/radeon_ttm.c     | 7 +++----
 drivers/gpu/drm/ttm/ttm_bo.c            | 6 ++----
 drivers/gpu/drm/ttm/ttm_bo_util.c       | 8 ++++----
 include/drm/ttm/ttm_bo_driver.h         | 4 ++--
 7 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 7db9556..c307a7d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -505,7 +505,7 @@ static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
-	r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, new_mem);
+	r = ttm_bo_move_ttm(bo, ctx, new_mem);
 out_cleanup:
 	ttm_bo_mem_put(bo, &tmp_mem);
 	return r;
@@ -536,7 +536,7 @@ static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
 	if (unlikely(r)) {
 		return r;
 	}
-	r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, &tmp_mem);
+	r = ttm_bo_move_ttm(bo, ctx, &tmp_mem);
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
@@ -597,8 +597,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 
 	if (r) {
 memcpy:
-		r = ttm_bo_move_memcpy(bo, ctx->interruptible,
-				       ctx->no_wait_gpu, new_mem);
+		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
 		if (r) {
 			return r;
 		}
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 949bf6b..6b6fb20 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1226,7 +1226,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr,
 	if (ret)
 		goto out;
 
-	ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, new_reg);
+	ret = ttm_bo_move_ttm(bo, &ctx, new_reg);
 out:
 	ttm_bo_mem_put(bo, &tmp_reg);
 	return ret;
@@ -1255,7 +1255,7 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr,
 	if (ret)
 		return ret;
 
-	ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, &tmp_reg);
+	ret = ttm_bo_move_ttm(bo, &ctx, &tmp_reg);
 	if (ret)
 		goto out;
 
@@ -1380,8 +1380,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 	/* Fallback to software copy. */
 	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
 	if (ret == 0)
-		ret = ttm_bo_move_memcpy(bo, ctx->interruptible,
-					 ctx->no_wait_gpu, new_reg);
+		ret = ttm_bo_move_memcpy(bo, ctx, new_reg);
 
 out:
 	if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) {
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index d866f32..78ce118 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -357,8 +357,7 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
 		qxl_move_null(bo, new_mem);
 		return 0;
 	}
-	return ttm_bo_move_memcpy(bo, ctx->interruptible, ctx->no_wait_gpu,
-				  new_mem);
+	return ttm_bo_move_memcpy(bo, ctx, new_mem);
 }
 
 static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 98e30d7..557fd79 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -347,7 +347,7 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
-	r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, new_mem);
+	r = ttm_bo_move_ttm(bo, &ctx, new_mem);
 out_cleanup:
 	ttm_bo_mem_put(bo, &tmp_mem);
 	return r;
@@ -380,7 +380,7 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
 	if (unlikely(r)) {
 		return r;
 	}
-	r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, &tmp_mem);
+	r = ttm_bo_move_ttm(bo, &ctx, &tmp_mem);
 	if (unlikely(r)) {
 		goto out_cleanup;
 	}
@@ -445,8 +445,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
 
 	if (r) {
 memcpy:
-		r = ttm_bo_move_memcpy(bo, ctx->interruptible,
-				       ctx->no_wait_gpu, new_mem);
+		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
 		if (r) {
 			return r;
 		}
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 70b2673..17fe8be 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -324,13 +324,11 @@ 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, ctx->interruptible,
-				      ctx->no_wait_gpu, mem);
+		ret = ttm_bo_move_ttm(bo, ctx, mem);
 	else if (bdev->driver->move)
 		ret = bdev->driver->move(bo, evict, ctx, mem);
 	else
-		ret = ttm_bo_move_memcpy(bo, ctx->interruptible,
-					 ctx->no_wait_gpu, mem);
+		ret = ttm_bo_move_memcpy(bo, ctx, mem);
 
 	if (ret) {
 		if (bdev->driver->move_notify) {
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index e7a519f..9237099 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -45,7 +45,7 @@ void ttm_bo_free_old_node(struct ttm_buffer_object *bo)
 }
 
 int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
-		    bool interruptible, bool no_wait_gpu,
+		   struct ttm_operation_ctx *ctx,
 		    struct ttm_mem_reg *new_mem)
 {
 	struct ttm_tt *ttm = bo->ttm;
@@ -53,7 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
 	int ret;
 
 	if (old_mem->mem_type != TTM_PL_SYSTEM) {
-		ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
+		ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
 
 		if (unlikely(ret != 0)) {
 			if (ret != -ERESTARTSYS)
@@ -329,7 +329,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 interruptible, bool no_wait_gpu,
+		       struct ttm_operation_ctx *ctx,
 		       struct ttm_mem_reg *new_mem)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
@@ -345,7 +345,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
 	unsigned long add = 0;
 	int dir;
 
-	ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
+	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
 	if (ret)
 		return ret;
 
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 6996d88..5115718 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -976,7 +976,7 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev,
  */
 
 int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
-		    bool interruptible, bool no_wait_gpu,
+		    struct ttm_operation_ctx *ctx,
 		    struct ttm_mem_reg *new_mem);
 
 /**
@@ -998,7 +998,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
  */
 
 int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
-		       bool interruptible, bool no_wait_gpu,
+		       struct ttm_operation_ctx *ctx,
 		       struct ttm_mem_reg *new_mem);
 
 /**
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock

[Roger He]

Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe
Signed-off-by: Roger He <Hongbo.He@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 17fe8be..eb8c568 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 								      place)) {
 				if (locked)
 					reservation_object_unlock(bo->resv);
+				locked = false;
 				continue;
 			}
 			break;
-- 
2.7.4

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

[PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

[Roger He]

Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6
Signed-off-by: Roger He <Hongbo.He@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index eb8c568..22b6ca5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		list_for_each_entry(bo, &man->lru[i], lru) {
-			if (bo->resv == resv) {
-				if (list_empty(&bo->ddestroy))
-					continue;
-			} else {
+			if (!ctx ||
+				!(ctx->on_alloc_stage &&
+				bo->resv == ctx->resv)) {
 				locked = reservation_object_trylock(bo->resv);
 				if (!locked)
 					continue;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm/ttm: remove parameter reservation since it is moved into ttm context

[Roger He]

Change-Id: I83ac6a77f24e14698aa386a497a262e24c5bbdb6
Signed-off-by: Roger He <Hongbo.He@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 22b6ca5..f138e95 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -707,7 +707,6 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
-			       struct reservation_object *resv,
 			       uint32_t mem_type,
 			       const struct ttm_place *place,
 			       struct ttm_operation_ctx *ctx)
@@ -834,7 +833,7 @@ 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, bo->resv, mem_type, place, ctx);
+		ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
 		if (unlikely(ret != 0))
 			return ret;
 	} while (1);
@@ -1332,8 +1331,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		while (!list_empty(&man->lru[i])) {
 			spin_unlock(&glob->lru_lock);
-			ret = ttm_mem_evict_first(bdev, NULL, mem_type,
-						  NULL, &ctx);
+			ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx);
 			if (ret)
 				return ret;
 			spin_lock(&glob->lru_lock);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Christian König]

Am 12.12.2017 um 10:33 schrieb Roger He:
> on_alloc_stage: is this operation on allocation stage
> resv: reservation bo used of this operation
>
> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
> Signed-off-by: Roger He <Hongbo.He@amd.com>
> ---
>   include/drm/ttm/ttm_bo_api.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 368eb02..25de597 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>    *
>    * @interruptible: Sleep interruptible if sleeping.
>    * @no_wait_gpu: Return immediately if the GPU is busy.
> + * @on_alloc_stage: is this operation on allocation stage
> + * @resv: resvation bo used
>    *
>    * Context for TTM operations like changing buffer placement or general memory
>    * allocation.
> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>   struct ttm_operation_ctx {
>   	bool interruptible;
>   	bool no_wait_gpu;
> +	bool on_alloc_stage;

The name describes how amdgpu want to use the flag and not what effect 
it has.

How about "allow_reserved_eviction"?

And also please update the documentation with something like:
* @allow_reserved_eviction: Allow eviction of reserved BOs.
* @resv: Reservation object to allow reserved evictions with.

Alternative we could put the ww_mutex context in here as Daniel 
suggested, but I think we should stick with what we have for now and 
make that change when we find another use case for this.

Christian.

> +	struct reservation_object *resv;
>   	uint64_t bytes_moved;
>   };
>   

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

Re: [PATCH 2/6] drm/ttm: when create a new ttm bo init on_alloc_stage and resv for ttm_operation_ctx

[Christian König]

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I0c6571c2a64e6c5bdad80ccbcccb40eba1c20b4e
> Signed-off-by: Roger He <Hongbo.He@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
>   drivers/gpu/drm/ttm/ttm_bo.c               | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index dc0a8be..eb42782 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -327,7 +327,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   			       uint64_t init_value,
>   			       struct amdgpu_bo **bo_ptr)
>   {
> -	struct ttm_operation_ctx ctx = { !kernel, false };
> +	struct ttm_operation_ctx ctx = { !kernel, false, true };
>   	struct amdgpu_bo *bo;
>   	enum ttm_bo_type type;
>   	unsigned long page_align;
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 97c3da6..70b2673 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -1208,6 +1208,7 @@ int ttm_bo_init_reserved(struct ttm_bo_device *bdev,
>   		WARN_ON(!locked);
>   	}
>   
> +	ctx->resv = resv;

That is something amdgpu specific, don't put that into TTM code.

Christian.

>   	if (likely(!ret))
>   		ret = ttm_bo_validate(bo, placement, ctx);
>   

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

Re: [PATCH 3/6] drm/ttm: use an ttm operation ctx for ttm_bo_move_xxx

[Christian König]

Am 12.12.2017 um 10:33 schrieb Roger He:
> include ttm_bo_move_memcpy and ttm_bo_move_ttm
>
> Change-Id: I160b2fe1da3200405810d0215c4521b5f0d3615a
> Signed-off-by: Roger He <Hongbo.He@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

But please separate that out and wait for a few days before committing, 
maybe some nouveau devs have objections.

Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++----
>   drivers/gpu/drm/nouveau/nouveau_bo.c    | 7 +++----
>   drivers/gpu/drm/qxl/qxl_ttm.c           | 3 +--
>   drivers/gpu/drm/radeon/radeon_ttm.c     | 7 +++----
>   drivers/gpu/drm/ttm/ttm_bo.c            | 6 ++----
>   drivers/gpu/drm/ttm/ttm_bo_util.c       | 8 ++++----
>   include/drm/ttm/ttm_bo_driver.h         | 4 ++--
>   7 files changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 7db9556..c307a7d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -505,7 +505,7 @@ static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> -	r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, new_mem);
> +	r = ttm_bo_move_ttm(bo, ctx, new_mem);
>   out_cleanup:
>   	ttm_bo_mem_put(bo, &tmp_mem);
>   	return r;
> @@ -536,7 +536,7 @@ static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
>   	if (unlikely(r)) {
>   		return r;
>   	}
> -	r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, &tmp_mem);
> +	r = ttm_bo_move_ttm(bo, ctx, &tmp_mem);
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> @@ -597,8 +597,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>   
>   	if (r) {
>   memcpy:
> -		r = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -				       ctx->no_wait_gpu, new_mem);
> +		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
>   		if (r) {
>   			return r;
>   		}
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 949bf6b..6b6fb20 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1226,7 +1226,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr,
>   	if (ret)
>   		goto out;
>   
> -	ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, new_reg);
> +	ret = ttm_bo_move_ttm(bo, &ctx, new_reg);
>   out:
>   	ttm_bo_mem_put(bo, &tmp_reg);
>   	return ret;
> @@ -1255,7 +1255,7 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr,
>   	if (ret)
>   		return ret;
>   
> -	ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, &tmp_reg);
> +	ret = ttm_bo_move_ttm(bo, &ctx, &tmp_reg);
>   	if (ret)
>   		goto out;
>   
> @@ -1380,8 +1380,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
>   	/* Fallback to software copy. */
>   	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
>   	if (ret == 0)
> -		ret = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -					 ctx->no_wait_gpu, new_reg);
> +		ret = ttm_bo_move_memcpy(bo, ctx, new_reg);
>   
>   out:
>   	if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) {
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index d866f32..78ce118 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -357,8 +357,7 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
>   		qxl_move_null(bo, new_mem);
>   		return 0;
>   	}
> -	return ttm_bo_move_memcpy(bo, ctx->interruptible, ctx->no_wait_gpu,
> -				  new_mem);
> +	return ttm_bo_move_memcpy(bo, ctx, new_mem);
>   }
>   
>   static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 98e30d7..557fd79 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -347,7 +347,7 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> -	r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, new_mem);
> +	r = ttm_bo_move_ttm(bo, &ctx, new_mem);
>   out_cleanup:
>   	ttm_bo_mem_put(bo, &tmp_mem);
>   	return r;
> @@ -380,7 +380,7 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
>   	if (unlikely(r)) {
>   		return r;
>   	}
> -	r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, &tmp_mem);
> +	r = ttm_bo_move_ttm(bo, &ctx, &tmp_mem);
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> @@ -445,8 +445,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>   
>   	if (r) {
>   memcpy:
> -		r = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -				       ctx->no_wait_gpu, new_mem);
> +		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
>   		if (r) {
>   			return r;
>   		}
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 70b2673..17fe8be 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -324,13 +324,11 @@ 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, ctx->interruptible,
> -				      ctx->no_wait_gpu, mem);
> +		ret = ttm_bo_move_ttm(bo, ctx, mem);
>   	else if (bdev->driver->move)
>   		ret = bdev->driver->move(bo, evict, ctx, mem);
>   	else
> -		ret = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -					 ctx->no_wait_gpu, mem);
> +		ret = ttm_bo_move_memcpy(bo, ctx, mem);
>   
>   	if (ret) {
>   		if (bdev->driver->move_notify) {
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index e7a519f..9237099 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -45,7 +45,7 @@ void ttm_bo_free_old_node(struct ttm_buffer_object *bo)
>   }
>   
>   int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
> -		    bool interruptible, bool no_wait_gpu,
> +		   struct ttm_operation_ctx *ctx,
>   		    struct ttm_mem_reg *new_mem)
>   {
>   	struct ttm_tt *ttm = bo->ttm;
> @@ -53,7 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
>   	int ret;
>   
>   	if (old_mem->mem_type != TTM_PL_SYSTEM) {
> -		ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
> +		ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
>   
>   		if (unlikely(ret != 0)) {
>   			if (ret != -ERESTARTSYS)
> @@ -329,7 +329,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 interruptible, bool no_wait_gpu,
> +		       struct ttm_operation_ctx *ctx,
>   		       struct ttm_mem_reg *new_mem)
>   {
>   	struct ttm_bo_device *bdev = bo->bdev;
> @@ -345,7 +345,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
>   	unsigned long add = 0;
>   	int dir;
>   
> -	ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
> +	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
>   	if (ret)
>   		return ret;
>   
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 6996d88..5115718 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -976,7 +976,7 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev,
>    */
>   
>   int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
> -		    bool interruptible, bool no_wait_gpu,
> +		    struct ttm_operation_ctx *ctx,
>   		    struct ttm_mem_reg *new_mem);
>   
>   /**
> @@ -998,7 +998,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
>    */
>   
>   int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> -		       bool interruptible, bool no_wait_gpu,
> +		       struct ttm_operation_ctx *ctx,
>   		       struct ttm_mem_reg *new_mem);
>   
>   /**

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock

[Christian König]

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe
> Signed-off-by: Roger He <Hongbo.He@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

That is a bug fix, isn't it? If yes maybe add CC:stable and commit it 
first before all other patches.

Christian.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 17fe8be..eb8c568 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   								      place)) {
>   				if (locked)
>   					reservation_object_unlock(bo->resv);
> +				locked = false;
>   				continue;
>   			}
>   			break;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

[Christian König]

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6
> Signed-off-by: Roger He <Hongbo.He@amd.com>

I would squash this one with patch #6.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index eb8c568..22b6ca5 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   	spin_lock(&glob->lru_lock);
>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   		list_for_each_entry(bo, &man->lru[i], lru) {
> -			if (bo->resv == resv) {
> -				if (list_empty(&bo->ddestroy))
> -					continue;
> -			} else {
> +			if (!ctx ||
> +				!(ctx->on_alloc_stage &&
> +				bo->resv == ctx->resv)) {

Coding style: The lines stating with "!(ctx" and "bo->resv" are to far 
to the right.

Additional to that I think ctx is mandatory and doesn't need a check 
(but might be wrong). If it isn't it's probably time to make it mandatory.

And I would use (ctx->on_alloc_stage || list_empty(&bo->ddestroy)) as 
check, we probably still want to be able to handle deleted BOs here 
during CS.

Christian.

>   				locked = reservation_object_trylock(bo->resv);
>   				if (!locked)
>   					continue;

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

RE: [PATCH 3/6] drm/ttm: use an ttm operation ctx for ttm_bo_move_xxx

[He, Roger]

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] 
Sent: Tuesday, December 12, 2017 6:34 PM
To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/6] drm/ttm: use an ttm operation ctx for ttm_bo_move_xxx

Am 12.12.2017 um 10:33 schrieb Roger He:
> include ttm_bo_move_memcpy and ttm_bo_move_ttm
>
> Change-Id: I160b2fe1da3200405810d0215c4521b5f0d3615a
> Signed-off-by: Roger He <Hongbo.He@amd.com>

	Reviewed-by: Christian König <christian.koenig@amd.com>

               But please separate that out and wait for a few days before committing, maybe some nouveau devs have objections.

Ok.

Thanks
Roger(Hongbo.He)

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++----
>   drivers/gpu/drm/nouveau/nouveau_bo.c    | 7 +++----
>   drivers/gpu/drm/qxl/qxl_ttm.c           | 3 +--
>   drivers/gpu/drm/radeon/radeon_ttm.c     | 7 +++----
>   drivers/gpu/drm/ttm/ttm_bo.c            | 6 ++----
>   drivers/gpu/drm/ttm/ttm_bo_util.c       | 8 ++++----
>   include/drm/ttm/ttm_bo_driver.h         | 4 ++--
>   7 files changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 7db9556..c307a7d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -505,7 +505,7 @@ static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> -	r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, new_mem);
> +	r = ttm_bo_move_ttm(bo, ctx, new_mem);
>   out_cleanup:
>   	ttm_bo_mem_put(bo, &tmp_mem);
>   	return r;
> @@ -536,7 +536,7 @@ static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
>   	if (unlikely(r)) {
>   		return r;
>   	}
> -	r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, &tmp_mem);
> +	r = ttm_bo_move_ttm(bo, ctx, &tmp_mem);
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> @@ -597,8 +597,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object 
> *bo, bool evict,
>   
>   	if (r) {
>   memcpy:
> -		r = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -				       ctx->no_wait_gpu, new_mem);
> +		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
>   		if (r) {
>   			return r;
>   		}
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c 
> b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 949bf6b..6b6fb20 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1226,7 +1226,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr,
>   	if (ret)
>   		goto out;
>   
> -	ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, new_reg);
> +	ret = ttm_bo_move_ttm(bo, &ctx, new_reg);
>   out:
>   	ttm_bo_mem_put(bo, &tmp_reg);
>   	return ret;
> @@ -1255,7 +1255,7 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr,
>   	if (ret)
>   		return ret;
>   
> -	ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, &tmp_reg);
> +	ret = ttm_bo_move_ttm(bo, &ctx, &tmp_reg);
>   	if (ret)
>   		goto out;
>   
> @@ -1380,8 +1380,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
>   	/* Fallback to software copy. */
>   	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
>   	if (ret == 0)
> -		ret = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -					 ctx->no_wait_gpu, new_reg);
> +		ret = ttm_bo_move_memcpy(bo, ctx, new_reg);
>   
>   out:
>   	if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) { 
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c 
> b/drivers/gpu/drm/qxl/qxl_ttm.c index d866f32..78ce118 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -357,8 +357,7 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
>   		qxl_move_null(bo, new_mem);
>   		return 0;
>   	}
> -	return ttm_bo_move_memcpy(bo, ctx->interruptible, ctx->no_wait_gpu,
> -				  new_mem);
> +	return ttm_bo_move_memcpy(bo, ctx, new_mem);
>   }
>   
>   static void qxl_bo_move_notify(struct ttm_buffer_object *bo, diff 
> --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 98e30d7..557fd79 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -347,7 +347,7 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> -	r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, new_mem);
> +	r = ttm_bo_move_ttm(bo, &ctx, new_mem);
>   out_cleanup:
>   	ttm_bo_mem_put(bo, &tmp_mem);
>   	return r;
> @@ -380,7 +380,7 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
>   	if (unlikely(r)) {
>   		return r;
>   	}
> -	r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, &tmp_mem);
> +	r = ttm_bo_move_ttm(bo, &ctx, &tmp_mem);
>   	if (unlikely(r)) {
>   		goto out_cleanup;
>   	}
> @@ -445,8 +445,7 @@ static int radeon_bo_move(struct ttm_buffer_object 
> *bo, bool evict,
>   
>   	if (r) {
>   memcpy:
> -		r = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -				       ctx->no_wait_gpu, new_mem);
> +		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
>   		if (r) {
>   			return r;
>   		}
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index 70b2673..17fe8be 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -324,13 +324,11 @@ 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, ctx->interruptible,
> -				      ctx->no_wait_gpu, mem);
> +		ret = ttm_bo_move_ttm(bo, ctx, mem);
>   	else if (bdev->driver->move)
>   		ret = bdev->driver->move(bo, evict, ctx, mem);
>   	else
> -		ret = ttm_bo_move_memcpy(bo, ctx->interruptible,
> -					 ctx->no_wait_gpu, mem);
> +		ret = ttm_bo_move_memcpy(bo, ctx, mem);
>   
>   	if (ret) {
>   		if (bdev->driver->move_notify) {
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
> b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index e7a519f..9237099 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -45,7 +45,7 @@ void ttm_bo_free_old_node(struct ttm_buffer_object *bo)
>   }
>   
>   int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
> -		    bool interruptible, bool no_wait_gpu,
> +		   struct ttm_operation_ctx *ctx,
>   		    struct ttm_mem_reg *new_mem)
>   {
>   	struct ttm_tt *ttm = bo->ttm;
> @@ -53,7 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
>   	int ret;
>   
>   	if (old_mem->mem_type != TTM_PL_SYSTEM) {
> -		ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
> +		ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
>   
>   		if (unlikely(ret != 0)) {
>   			if (ret != -ERESTARTSYS)
> @@ -329,7 +329,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 interruptible, bool no_wait_gpu,
> +		       struct ttm_operation_ctx *ctx,
>   		       struct ttm_mem_reg *new_mem)
>   {
>   	struct ttm_bo_device *bdev = bo->bdev; @@ -345,7 +345,7 @@ int 
> ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
>   	unsigned long add = 0;
>   	int dir;
>   
> -	ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
> +	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
>   	if (ret)
>   		return ret;
>   
> diff --git a/include/drm/ttm/ttm_bo_driver.h 
> b/include/drm/ttm/ttm_bo_driver.h index 6996d88..5115718 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -976,7 +976,7 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev,
>    */
>   
>   int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
> -		    bool interruptible, bool no_wait_gpu,
> +		    struct ttm_operation_ctx *ctx,
>   		    struct ttm_mem_reg *new_mem);
>   
>   /**
> @@ -998,7 +998,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
>    */
>   
>   int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> -		       bool interruptible, bool no_wait_gpu,
> +		       struct ttm_operation_ctx *ctx,
>   		       struct ttm_mem_reg *new_mem);
>   
>   /**

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

RE: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock

[He, Roger]

	That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.

Fortunately so far there is no issue directly resulted from that.

Thanks
Roger(Hongbo.He)

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] 
Sent: Tuesday, December 12, 2017 6:37 PM
To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe
> Signed-off-by: Roger He <Hongbo.He@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.

Christian.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index 17fe8be..eb8c568 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   								      place)) {
>   				if (locked)
>   					reservation_object_unlock(bo->resv);
> +				locked = false;
>   				continue;
>   			}
>   			break;

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

RE: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

[He, Roger]

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] 
Sent: Tuesday, December 12, 2017 6:40 PM
To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6
> Signed-off-by: Roger He <Hongbo.He@amd.com>

I would squash this one with patch #6.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index eb8c568..22b6ca5 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   	spin_lock(&glob->lru_lock);
>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   		list_for_each_entry(bo, &man->lru[i], lru) {
> -			if (bo->resv == resv) {
> -				if (list_empty(&bo->ddestroy))
> -					continue;
> -			} else {
> +			if (!ctx ||
> +				!(ctx->on_alloc_stage &&
> +				bo->resv == ctx->resv)) {

	Coding style: The lines stating with "!(ctx" and "bo->resv" are to far to the right.

	Additional to that I think ctx is mandatory and doesn't need a check (but might be wrong). If it isn't it's probably time to make it 	mandatory.

	And I would use (ctx->on_alloc_stage || list_empty(&bo->ddestroy)) as check, we probably still want to be able to handle 	deleted BOs here during CS.

Change that as below, I think it can cover the case you described. No matter the Bo is in deleted list or not.
if (!ctx->allow_reserved_eviction || bo->resv != ctx->resv) {
				locked = kcl_reservation_object_trylock(bo->resv);
				if (!locked)
					continue;
			}

Thanks
Roger(Hongbo.He)



Christian.

>   				locked = reservation_object_trylock(bo->resv);
>   				if (!locked)
>   					continue;

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

Re: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock

[Christian König]

Am 13.12.2017 um 03:06 schrieb He, Roger:
> 	That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.
>
> Fortunately so far there is no issue directly resulted from that.

Yeah, but that is irrelevant. Patches are classified as fix if they fix 
something, not if the bug was ever hit.

Not sure if the code is already upstream, but we should still make sure 
that we send this patch to the appropriate places.

Christian.

>
> Thanks
> Roger(Hongbo.He)
>
> -----Original Message-----
> From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com]
> Sent: Tuesday, December 12, 2017 6:37 PM
> To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock
>
> Am 12.12.2017 um 10:33 schrieb Roger He:
>> Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe
>> Signed-off-by: Roger He <Hongbo.He@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.
>
> Christian.
>
>> ---
>>    drivers/gpu/drm/ttm/ttm_bo.c | 1 +
>>    1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
>> b/drivers/gpu/drm/ttm/ttm_bo.c index 17fe8be..eb8c568 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>    								      place)) {
>>    				if (locked)
>>    					reservation_object_unlock(bo->resv);
>> +				locked = false;
>>    				continue;
>>    			}
>>    			break;

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

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Thomas Hellstrom]

Hi,

I think this series is quite poorly documented. We should have a log 
message explaining the purpose of the commit.
Also since it's not obvious what the series is attempting to achieve, 
please add a 0/X series header message..

/Thomas


On 12/12/2017 10:33 AM, Roger He wrote:
> on_alloc_stage: is this operation on allocation stage
> resv: reservation bo used of this operation
>
> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
> Signed-off-by: Roger He <Hongbo.He@amd.com>
> ---
>   include/drm/ttm/ttm_bo_api.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 368eb02..25de597 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>    *
>    * @interruptible: Sleep interruptible if sleeping.
>    * @no_wait_gpu: Return immediately if the GPU is busy.
> + * @on_alloc_stage: is this operation on allocation stage
> + * @resv: resvation bo used
>    *
>    * Context for TTM operations like changing buffer placement or general memory
>    * allocation.
> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>   struct ttm_operation_ctx {
>   	bool interruptible;
>   	bool no_wait_gpu;
> +	bool on_alloc_stage;
> +	struct reservation_object *resv;
>   	uint64_t bytes_moved;
>   };
>   


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Christian König]

Hi Thomas,

sorry for that. Noted on the rest of that series as well that we need to 
improve the commit messages. But this one somehow slipped through 
because I discussed this change previously internally with Roger.

That made the change completely logical for me, but without this context 
everybody else just thinks "Hui what?". Going to keep that in mind the 
next time.

But back to topic: This series allows BOs which share the same 
reservation object as the BO currently allocated/validated to be evicted 
even when they are reserved.

This is useful because amdgpu wants to use a single reservation object 
for almost all BOs of a process.

Regards,
Christian.

Am 13.12.2017 um 20:21 schrieb Thomas Hellstrom:
> Hi,
>
> I think this series is quite poorly documented. We should have a log 
> message explaining the purpose of the commit.
> Also since it's not obvious what the series is attempting to achieve, 
> please add a 0/X series header message..
>
> /Thomas
>
>
> On 12/12/2017 10:33 AM, Roger He wrote:
>> on_alloc_stage: is this operation on allocation stage
>> resv: reservation bo used of this operation
>>
>> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
>> Signed-off-by: Roger He <Hongbo.He@amd.com>
>> ---
>>   include/drm/ttm/ttm_bo_api.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>> index 368eb02..25de597 100644
>> --- a/include/drm/ttm/ttm_bo_api.h
>> +++ b/include/drm/ttm/ttm_bo_api.h
>> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>>    *
>>    * @interruptible: Sleep interruptible if sleeping.
>>    * @no_wait_gpu: Return immediately if the GPU is busy.
>> + * @on_alloc_stage: is this operation on allocation stage
>> + * @resv: resvation bo used
>>    *
>>    * Context for TTM operations like changing buffer placement or 
>> general memory
>>    * allocation.
>> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>>   struct ttm_operation_ctx {
>>       bool interruptible;
>>       bool no_wait_gpu;
>> +    bool on_alloc_stage;
>> +    struct reservation_object *resv;
>>       uint64_t bytes_moved;
>>   };
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Thomas Hellstrom]

Hi, Christian,

On 12/14/2017 09:40 AM, Christian König wrote:
> Hi Thomas,
>
> sorry for that. Noted on the rest of that series as well that we need 
> to improve the commit messages. But this one somehow slipped through 
> because I discussed this change previously internally with Roger.
>
> That made the change completely logical for me, but without this 
> context everybody else just thinks "Hui what?". Going to keep that in 
> mind the next time.
>
> But back to topic: This series allows BOs which share the same 
> reservation object as the BO currently allocated/validated to be 
> evicted even when they are reserved.
>
> This is useful because amdgpu wants to use a single reservation object 
> for almost all BOs of a process.

Yes, that indeed makes the whole thing more clear, and makes sense.

Out of interest, is the shared reservation object usage a speed 
optimization (avoiding the ww_mutex_locks at reservation time?)
or something else?

I guess that even if LRU lists might get crowded with unevictable BOs, 
iterating through those lists isn't really part of the fast path.
/Thomas

>
> Regards,
> Christian. 

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

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Christian König]

Am 14.12.2017 um 09:55 schrieb Thomas Hellstrom:
> Hi, Christian,
>
> On 12/14/2017 09:40 AM, Christian König wrote:
>> Hi Thomas,
>>
>> sorry for that. Noted on the rest of that series as well that we need 
>> to improve the commit messages. But this one somehow slipped through 
>> because I discussed this change previously internally with Roger.
>>
>> That made the change completely logical for me, but without this 
>> context everybody else just thinks "Hui what?". Going to keep that in 
>> mind the next time.
>>
>> But back to topic: This series allows BOs which share the same 
>> reservation object as the BO currently allocated/validated to be 
>> evicted even when they are reserved.
>>
>> This is useful because amdgpu wants to use a single reservation 
>> object for almost all BOs of a process.
>
> Yes, that indeed makes the whole thing more clear, and makes sense.
>
> Out of interest, is the shared reservation object usage a speed 
> optimization (avoiding the ww_mutex_locks at reservation time?)
> or something else?

Avoiding taking many ww_mutex_locks is one reason. The other major 
reason comes with GPU-VM page tables.

Just the same as CPU page tables multi level GPU page tables are 
allocated individually when needed. But during command submission we 
must make sure that all GPU page tables are validated and fences added 
to all of them.

Because of this we are using the reservation object of the root page 
directory (because that one is always allocated) as reservation object 
for all other page tables.

The effect is that you have to add the resulting fence of a command 
submission to only one reservation object and not a couple of hundreds 
or even thousands.

> I guess that even if LRU lists might get crowded with unevictable BOs, 
> iterating through those lists isn't really part of the fast path.

Yes, exactly. When we start to massively evict things performance goes 
down so much anyway that this extra cycling over the LRU doesn't hurt us 
much.

Christian.

> /Thomas
>
>>
>> Regards,
>> Christian. 
>

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

RE: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[He, Roger]

Hi Thomas:

Really sorry for that and will keep that in mind.
If necessary, next time I will send cover letter to provide more background and details.

	
Thanks
Roger(Hongbo.He)

-----Original Message-----
From: Thomas Hellstrom [mailto:thomas@shipmail.org] 
Sent: Thursday, December 14, 2017 3:21 AM
To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

Hi,

I think this series is quite poorly documented. We should have a log message explaining the purpose of the commit.
Also since it's not obvious what the series is attempting to achieve, please add a 0/X series header message..

/Thomas


On 12/12/2017 10:33 AM, Roger He wrote:
> on_alloc_stage: is this operation on allocation stage
> resv: reservation bo used of this operation
>
> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
> Signed-off-by: Roger He <Hongbo.He@amd.com>
> ---
>   include/drm/ttm/ttm_bo_api.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/ttm/ttm_bo_api.h 
> b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>    *
>    * @interruptible: Sleep interruptible if sleeping.
>    * @no_wait_gpu: Return immediately if the GPU is busy.
> + * @on_alloc_stage: is this operation on allocation stage
> + * @resv: resvation bo used
>    *
>    * Context for TTM operations like changing buffer placement or general memory
>    * allocation.
> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>   struct ttm_operation_ctx {
>   	bool interruptible;
>   	bool no_wait_gpu;
> +	bool on_alloc_stage;
> +	struct reservation_object *resv;
>   	uint64_t bytes_moved;
>   };
>   


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Daniel Vetter]

On Tue, Dec 12, 2017 at 10:34 AM Roger He <Hongbo.He@amd.com> wrote:
>
> on_alloc_stage: is this operation on allocation stage
> resv: reservation bo used of this operation
>
> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
> Signed-off-by: Roger He <Hongbo.He@amd.com>

Real commit message (the later patches using this are even sparser)
and/or proper kerneldoc would be massively appreciated in common code.
You guys have done a ton of stuff and special cases just for amdgpu,
and if someone doesn't at least know what you aimed to do it's pretty
much impossible to understand. I don't care much if this is the
standard in drivers, but common code really should bother to explain
what problem it's trying to solve.

(yes I think I figured out what it's doing, but I'm pretty sure most
dri-devel folks hacking on gem/cs/render stuff won't be so lucky)
-Daniel

> ---
>  include/drm/ttm/ttm_bo_api.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 368eb02..25de597 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>   *
>   * @interruptible: Sleep interruptible if sleeping.
>   * @no_wait_gpu: Return immediately if the GPU is busy.
> + * @on_alloc_stage: is this operation on allocation stage
> + * @resv: resvation bo used
>   *
>   * Context for TTM operations like changing buffer placement or general memory
>   * allocation.
> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>  struct ttm_operation_ctx {
>         bool interruptible;
>         bool no_wait_gpu;
> +       bool on_alloc_stage;
> +       struct reservation_object *resv;
>         uint64_t bytes_moved;
>  };
>
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx