ttm multihop split out v2

ttm multihop split out v2

[Dave Airlie]

This is the multhop patch split out per driver, and with the
changes Daniel requested.

Sorry it took a while, got distracted with other things.

Dave.


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

[PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Currently drivers get called to move a buffer, but if they have to
move it temporarily through another space (SYSTEM->VRAM via TT)
then they can end up with a lot of ttm->driver->ttm call stacks,
if the temprorary space moves requires eviction.

Instead of letting the driver do all the placement/space for the
temporary, allow it to report back (-EMULTIHOP) and a placement (hop)
to the move code, which will then do the temporary move, and the
correct placement move afterwards.

This removes a lot of code from drivers, at the expense of
adding some midlayering. I've some further ideas on how to turn
it inside out, but I think this is a good solution to the call
stack problems.

v2: separate out the driver patches, add WARN for getting
MULTHOP in paths we shouldn't (Daniel)

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |  3 +-
 drivers/gpu/drm/drm_gem_vram_helper.c      |  3 +-
 drivers/gpu/drm/nouveau/nouveau_bo.c       |  3 +-
 drivers/gpu/drm/qxl/qxl_ttm.c              |  3 +-
 drivers/gpu/drm/radeon/radeon_ttm.c        |  3 +-
 drivers/gpu/drm/ttm/ttm_bo.c               | 68 +++++++++++++++++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c |  3 +-
 include/drm/ttm/ttm_bo_driver.h            |  7 ++-
 8 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c01c060e4ac5..ce0d82802333 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -656,7 +656,8 @@ static bool amdgpu_mem_visible(struct amdgpu_device *adev,
  */
 static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 			  struct ttm_operation_ctx *ctx,
-			  struct ttm_resource *new_mem)
+			  struct ttm_resource *new_mem,
+			  struct ttm_place *hop)
 {
 	struct amdgpu_device *adev;
 	struct amdgpu_bo *abo;
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 16d68c04ea5d..2cec7b1482b8 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -964,7 +964,8 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
 static int bo_driver_move(struct ttm_buffer_object *bo,
 			  bool evict,
 			  struct ttm_operation_ctx *ctx,
-			  struct ttm_resource *new_mem)
+			  struct ttm_resource *new_mem,
+			  struct ttm_place *hop)
 {
 	struct drm_gem_vram_object *gbo;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 8133377d865d..fee07b9d19ed 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1023,7 +1023,8 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo,
 static int
 nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 		struct ttm_operation_ctx *ctx,
-		struct ttm_resource *new_reg)
+		struct ttm_resource *new_reg,
+		struct ttm_place *hop)
 {
 	struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
 	struct nouveau_bo *nvbo = nouveau_bo(bo);
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index a80d59634143..128c38c8a837 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -140,7 +140,8 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
 
 static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
 		       struct ttm_operation_ctx *ctx,
-		       struct ttm_resource *new_mem)
+		       struct ttm_resource *new_mem,
+		       struct ttm_place *hop)
 {
 	struct ttm_resource *old_mem = &bo->mem;
 	int ret;
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 95038ac3382e..29062dbea299 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -303,7 +303,8 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
 
 static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
 			  struct ttm_operation_ctx *ctx,
-			  struct ttm_resource *new_mem)
+			  struct ttm_resource *new_mem,
+			  struct ttm_place *hop)
 {
 	struct radeon_device *rdev;
 	struct radeon_bo *rbo;
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e2a124b3affb..9f840f2a7836 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -231,7 +231,8 @@ EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
 
 static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
 				  struct ttm_resource *mem, bool evict,
-				  struct ttm_operation_ctx *ctx)
+				  struct ttm_operation_ctx *ctx,
+				  struct ttm_place *hop)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 	struct ttm_resource_manager *old_man = ttm_manager_type(bdev, bo->mem.mem_type);
@@ -259,9 +260,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
 		}
 	}
 
-	ret = bdev->driver->move(bo, evict, ctx, mem);
-	if (ret)
+	ret = bdev->driver->move(bo, evict, ctx, mem, hop);
+	if (ret) {
+		if (ret == -EMULTIHOP)
+			return ret;
 		goto out_err;
+	}
 
 	ctx->bytes_moved += bo->num_pages << PAGE_SHIFT;
 	return 0;
@@ -566,6 +570,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
 	struct ttm_bo_device *bdev = bo->bdev;
 	struct ttm_resource evict_mem;
 	struct ttm_placement placement;
+	struct ttm_place hop = {};
 	int ret = 0;
 
 	dma_resv_assert_held(bo->base.resv);
@@ -596,8 +601,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
 		goto out;
 	}
 
-	ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx);
+	ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, &hop);
 	if (unlikely(ret)) {
+		WARN(ret == -EMULTIHOP, "Unexpected multihop in eviction - likely driver bug\n");
 		if (ret != -ERESTARTSYS)
 			pr_err("Buffer eviction failed\n");
 		ttm_resource_free(bo, &evict_mem);
@@ -936,11 +942,39 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
 }
 EXPORT_SYMBOL(ttm_bo_mem_space);
 
+static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo,
+				     struct ttm_resource *mem,
+				     struct ttm_operation_ctx *ctx,
+				     struct ttm_place *hop)
+{
+	struct ttm_placement hop_placement;
+	int ret;
+	struct ttm_resource hop_mem = *mem;
+
+	hop_mem.mm_node = NULL;
+	hop_mem.mem_type = TTM_PL_SYSTEM;
+	hop_mem.placement = 0;
+
+	hop_placement.num_placement = hop_placement.num_busy_placement = 1;
+	hop_placement.placement = hop_placement.busy_placement = hop;
+
+	/* find space in the bounce domain */
+	ret = ttm_bo_mem_space(bo, &hop_placement, &hop_mem, ctx);
+	if (ret)
+		return ret;
+	/* move to the bounce domain */
+	ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL);
+	if (ret)
+		return ret;
+	return 0;
+}
+
 static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
 			      struct ttm_placement *placement,
 			      struct ttm_operation_ctx *ctx)
 {
 	int ret = 0;
+	struct ttm_place hop = {};
 	struct ttm_resource mem;
 
 	dma_resv_assert_held(bo->base.resv);
@@ -954,12 +988,25 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
 
 	/*
 	 * Determine where to move the buffer.
+	 *
+	 * If driver determines move is going to need
+	 * an extra step then it will return -EMULTIHOP
+	 * and the buffer will be moved to the temporary
+	 * stop and the driver will be called to make
+	 * the second hop.
 	 */
+bounce:
 	ret = ttm_bo_mem_space(bo, placement, &mem, ctx);
 	if (ret)
-		goto out_unlock;
-	ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx);
-out_unlock:
+		return ret;
+	ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop);
+	if (ret == -EMULTIHOP) {
+		ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, &hop);
+		if (ret)
+			return ret;
+		/* try and move to final place now. */
+		goto bounce;
+	}
 	if (ret)
 		ttm_resource_free(bo, &mem);
 	return ret;
@@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
 	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
 		struct ttm_operation_ctx ctx = { false, false };
 		struct ttm_resource evict_mem;
+		struct ttm_place hop = {};
 
 		evict_mem = bo->mem;
 		evict_mem.mm_node = NULL;
 		evict_mem.placement = 0;
 		evict_mem.mem_type = TTM_PL_SYSTEM;
 
-		ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx);
-		if (unlikely(ret != 0))
+		ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, &hop);
+		if (unlikely(ret != 0)) {
+			WARN(ret == -EMULTIHOP, "Unexpected multihop in swaput - likely driver bug.\n");
 			goto out;
+		}
 	}
 
 	/**
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index 51f70bea41cc..6a04261ce760 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -695,7 +695,8 @@ static void vmw_swap_notify(struct ttm_buffer_object *bo)
 static int vmw_move(struct ttm_buffer_object *bo,
 		    bool evict,
 		    struct ttm_operation_ctx *ctx,
-		    struct ttm_resource *new_mem)
+		    struct ttm_resource *new_mem,
+		    struct ttm_place *hop)
 {
 	struct ttm_resource_manager *old_man = ttm_manager_type(bo->bdev, bo->mem.mem_type);
 	struct ttm_resource_manager *new_man = ttm_manager_type(bo->bdev, new_mem->mem_type);
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index da8208f43378..f02f7cf9ae90 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -121,6 +121,8 @@ struct ttm_bo_driver {
 	 * Return the bo flags for a buffer which is not mapped to the hardware.
 	 * These will be placed in proposed_flags so that when the move is
 	 * finished, they'll end up in bo->mem.flags
+	 * This should not cause multihop evictions, and the core will warn
+	 * if one is proposed.
 	 */
 
 	void (*evict_flags)(struct ttm_buffer_object *bo,
@@ -134,12 +136,15 @@ struct ttm_bo_driver {
 	 * the graphics address space
 	 * @ctx: context for this move with parameters
 	 * @new_mem: the new memory region receiving the buffer
+	 @ @hop: placement for driver directed intermediate hop
 	 *
 	 * Move a buffer between two memory regions.
+	 * Returns errno -EMULTIHOP if driver requests a hop
 	 */
 	int (*move)(struct ttm_buffer_object *bo, bool evict,
 		    struct ttm_operation_ctx *ctx,
-		    struct ttm_resource *new_mem);
+		    struct ttm_resource *new_mem,
+		    struct ttm_place *hop);
 
 	/**
 	 * struct ttm_bo_driver_member verify_access
-- 
2.27.0

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

[PATCH 2/4] drm/amdgpu/ttm: use multihop

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This removes the code to move resources directly between
SYSTEM and VRAM in favour of using the core ttm mulithop code.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 136 +++---------------------
 1 file changed, 13 insertions(+), 123 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index ce0d82802333..e1458d575aa9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -512,119 +512,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
 	return r;
 }
 
-/**
- * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer
- *
- * Called by amdgpu_bo_move().
- */
-static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
-				struct ttm_operation_ctx *ctx,
-				struct ttm_resource *new_mem)
-{
-	struct ttm_resource *old_mem = &bo->mem;
-	struct ttm_resource tmp_mem;
-	struct ttm_place placements;
-	struct ttm_placement placement;
-	int r;
-
-	/* create space/pages for new_mem in GTT space */
-	tmp_mem = *new_mem;
-	tmp_mem.mm_node = NULL;
-	placement.num_placement = 1;
-	placement.placement = &placements;
-	placement.num_busy_placement = 1;
-	placement.busy_placement = &placements;
-	placements.fpfn = 0;
-	placements.lpfn = 0;
-	placements.mem_type = TTM_PL_TT;
-	placements.flags = 0;
-	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
-	if (unlikely(r)) {
-		pr_err("Failed to find GTT space for blit from VRAM\n");
-		return r;
-	}
-
-	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
-	if (unlikely(r))
-		goto out_cleanup;
-
-	/* Bind the memory to the GTT space */
-	r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-
-	/* blit VRAM to GTT */
-	r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-
-	r = ttm_bo_wait_ctx(bo, ctx);
-	if (unlikely(r))
-		goto out_cleanup;
-
-	amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
-	ttm_resource_free(bo, &bo->mem);
-	ttm_bo_assign_mem(bo, new_mem);
-out_cleanup:
-	ttm_resource_free(bo, &tmp_mem);
-	return r;
-}
-
-/**
- * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM
- *
- * Called by amdgpu_bo_move().
- */
-static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
-				struct ttm_operation_ctx *ctx,
-				struct ttm_resource *new_mem)
-{
-	struct ttm_resource *old_mem = &bo->mem;
-	struct ttm_resource tmp_mem;
-	struct ttm_placement placement;
-	struct ttm_place placements;
-	int r;
-
-	/* make space in GTT for old_mem buffer */
-	tmp_mem = *new_mem;
-	tmp_mem.mm_node = NULL;
-	placement.num_placement = 1;
-	placement.placement = &placements;
-	placement.num_busy_placement = 1;
-	placement.busy_placement = &placements;
-	placements.fpfn = 0;
-	placements.lpfn = 0;
-	placements.mem_type = TTM_PL_TT;
-	placements.flags = 0;
-	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
-	if (unlikely(r)) {
-		pr_err("Failed to find GTT space for blit to VRAM\n");
-		return r;
-	}
-
-	/* move/bind old memory to GTT space */
-	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
-	if (unlikely(r))
-		return r;
-
-	r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-
-	ttm_bo_assign_mem(bo, &tmp_mem);
-	/* copy to VRAM */
-	r = amdgpu_move_blit(bo, evict, new_mem, old_mem);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-out_cleanup:
-	ttm_resource_free(bo, &tmp_mem);
-	return r;
-}
-
 /**
  * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy
  *
@@ -664,6 +551,17 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 	struct ttm_resource *old_mem = &bo->mem;
 	int r;
 
+	if ((old_mem->mem_type == TTM_PL_SYSTEM &&
+	     new_mem->mem_type == TTM_PL_VRAM) ||
+	    (old_mem->mem_type == TTM_PL_VRAM &&
+	     new_mem->mem_type == TTM_PL_SYSTEM)) {
+		hop->fpfn = 0;
+		hop->lpfn = 0;
+		hop->mem_type = TTM_PL_TT;
+		hop->flags = 0;
+		return -EMULTIHOP;
+	}
+
 	if (new_mem->mem_type == TTM_PL_TT) {
 		r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem);
 		if (r)
@@ -717,16 +615,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 		goto memcpy;
 	}
 
-	if (old_mem->mem_type == TTM_PL_VRAM &&
-	    new_mem->mem_type == TTM_PL_SYSTEM) {
-		r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem);
-	} else if (old_mem->mem_type == TTM_PL_SYSTEM &&
-		   new_mem->mem_type == TTM_PL_VRAM) {
-		r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem);
-	} else {
-		r = amdgpu_move_blit(bo, evict,
-				     new_mem, old_mem);
-	}
+	r = amdgpu_move_blit(bo, evict,
+			     new_mem, old_mem);
 
 	if (r) {
 memcpy:
-- 
2.27.0

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

[PATCH 3/4] drm/nouveau/ttm: use multihop

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This removes the code to move resources directly between
SYSTEM and VRAM in favour of using the core ttm mulithop code.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_bo.c | 112 ++++-----------------------
 1 file changed, 13 insertions(+), 99 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index fee07b9d19ed..2b7720f412c1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -861,96 +861,6 @@ nouveau_bo_move_init(struct nouveau_drm *drm)
 	NV_INFO(drm, "MM: using %s for buffer copies\n", name);
 }
 
-static int
-nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict,
-		      struct ttm_operation_ctx *ctx,
-		      struct ttm_resource *new_reg)
-{
-	struct ttm_place placement_memtype = {
-		.fpfn = 0,
-		.lpfn = 0,
-		.mem_type = TTM_PL_TT,
-		.flags = 0
-	};
-	struct ttm_placement placement;
-	struct ttm_resource tmp_reg;
-	int ret;
-
-	placement.num_placement = placement.num_busy_placement = 1;
-	placement.placement = placement.busy_placement = &placement_memtype;
-
-	tmp_reg = *new_reg;
-	tmp_reg.mm_node = NULL;
-	ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx);
-	if (ret)
-		return ret;
-
-	ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
-	if (ret)
-		goto out;
-
-	ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg);
-	if (ret)
-		goto out;
-
-	ret = nouveau_bo_move_m2mf(bo, true, ctx, &tmp_reg);
-	if (ret)
-		goto out;
-
-	ret = ttm_bo_wait_ctx(bo, ctx);
-	if (ret)
-		goto out;
-
-	nouveau_ttm_tt_unbind(bo->bdev, bo->ttm);
-	ttm_resource_free(bo, &bo->mem);
-	ttm_bo_assign_mem(bo, &tmp_reg);
-out:
-	ttm_resource_free(bo, &tmp_reg);
-	return ret;
-}
-
-static int
-nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict,
-		      struct ttm_operation_ctx *ctx,
-		      struct ttm_resource *new_reg)
-{
-	struct ttm_place placement_memtype = {
-		.fpfn = 0,
-		.lpfn = 0,
-		.mem_type = TTM_PL_TT,
-		.flags = 0
-	};
-	struct ttm_placement placement;
-	struct ttm_resource tmp_reg;
-	int ret;
-
-	placement.num_placement = placement.num_busy_placement = 1;
-	placement.placement = placement.busy_placement = &placement_memtype;
-
-	tmp_reg = *new_reg;
-	tmp_reg.mm_node = NULL;
-	ret = ttm_bo_mem_space(bo, &placement, &tmp_reg, ctx);
-	if (ret)
-		return ret;
-
-	ret = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
-	if (unlikely(ret != 0))
-		return ret;
-
-	ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_reg);
-	if (unlikely(ret != 0))
-		return ret;
-
-	ttm_bo_assign_mem(bo, &tmp_reg);
-	ret = nouveau_bo_move_m2mf(bo, true, ctx, new_reg);
-	if (ret)
-		goto out;
-
-out:
-	ttm_resource_free(bo, &tmp_reg);
-	return ret;
-}
-
 static void
 nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict,
 		     struct ttm_resource *new_reg)
@@ -1032,6 +942,17 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 	struct nouveau_drm_tile *new_tile = NULL;
 	int ret = 0;
 
+	if ((old_reg->mem_type == TTM_PL_SYSTEM &&
+	     new_reg->mem_type == TTM_PL_VRAM) ||
+	    (old_reg->mem_type == TTM_PL_VRAM &&
+	     new_reg->mem_type == TTM_PL_SYSTEM)) {
+		hop->fpfn = 0;
+		hop->lpfn = 0;
+		hop->mem_type = TTM_PL_TT;
+		hop->flags = 0;
+		return -EMULTIHOP;
+	}
+
 	if (new_reg->mem_type == TTM_PL_TT) {
 		ret = nouveau_ttm_tt_bind(bo->bdev, bo->ttm, new_reg);
 		if (ret)
@@ -1074,15 +995,8 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 
 	/* Hardware assisted copy. */
 	if (drm->ttm.move) {
-		if (new_reg->mem_type == TTM_PL_SYSTEM)
-			ret = nouveau_bo_move_flipd(bo, evict, ctx,
-						    new_reg);
-		else if (old_reg->mem_type == TTM_PL_SYSTEM)
-			ret = nouveau_bo_move_flips(bo, evict, ctx,
-						    new_reg);
-		else
-			ret = nouveau_bo_move_m2mf(bo, evict, ctx,
-						   new_reg);
+		ret = nouveau_bo_move_m2mf(bo, evict, ctx,
+					   new_reg);
 		if (!ret)
 			goto out;
 	}
-- 
2.27.0

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

[PATCH 4/4] drm/radeon/ttm: use multihop

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This removes the code to move resources directly between
SYSTEM and VRAM in favour of using the core ttm mulithop code.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 119 +++-------------------------
 1 file changed, 13 insertions(+), 106 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 29062dbea299..788655ebafdb 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -206,101 +206,6 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
 	return r;
 }
 
-static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
-				bool evict,
-				struct ttm_operation_ctx *ctx,
-				struct ttm_resource *new_mem)
-{
-	struct ttm_resource *old_mem = &bo->mem;
-	struct ttm_resource tmp_mem;
-	struct ttm_place placements;
-	struct ttm_placement placement;
-	int r;
-
-	tmp_mem = *new_mem;
-	tmp_mem.mm_node = NULL;
-	placement.num_placement = 1;
-	placement.placement = &placements;
-	placement.num_busy_placement = 1;
-	placement.busy_placement = &placements;
-	placements.fpfn = 0;
-	placements.lpfn = 0;
-	placements.mem_type = TTM_PL_TT;
-	placements.flags = 0;
-	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
-	if (unlikely(r)) {
-		return r;
-	}
-
-	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-
-	r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-	r = radeon_move_blit(bo, true, &tmp_mem, old_mem);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-	r = ttm_bo_wait_ctx(bo, ctx);
-	if (unlikely(r))
-		goto out_cleanup;
-
-	radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
-	ttm_resource_free(bo, &bo->mem);
-	ttm_bo_assign_mem(bo, new_mem);
-out_cleanup:
-	ttm_resource_free(bo, &tmp_mem);
-	return r;
-}
-
-static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
-				bool evict,
-				struct ttm_operation_ctx *ctx,
-				struct ttm_resource *new_mem)
-{
-	struct ttm_resource *old_mem = &bo->mem;
-	struct ttm_resource tmp_mem;
-	struct ttm_placement placement;
-	struct ttm_place placements;
-	int r;
-
-	tmp_mem = *new_mem;
-	tmp_mem.mm_node = NULL;
-	placement.num_placement = 1;
-	placement.placement = &placements;
-	placement.num_busy_placement = 1;
-	placement.busy_placement = &placements;
-	placements.fpfn = 0;
-	placements.lpfn = 0;
-	placements.mem_type = TTM_PL_TT;
-	placements.flags = 0;
-	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
-	if (unlikely(r)) {
-		return r;
-	}
-
-	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
-	if (unlikely(r))
-		goto out_cleanup;
-
-	r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem);
-	if (unlikely(r))
-		goto out_cleanup;
-
-	ttm_bo_assign_mem(bo, &tmp_mem);
-	r = radeon_move_blit(bo, true, new_mem, old_mem);
-	if (unlikely(r)) {
-		goto out_cleanup;
-	}
-out_cleanup:
-	ttm_resource_free(bo, &tmp_mem);
-	return r;
-}
-
 static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
 			  struct ttm_operation_ctx *ctx,
 			  struct ttm_resource *new_mem,
@@ -311,6 +216,17 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
 	struct ttm_resource *old_mem = &bo->mem;
 	int r;
 
+	if ((old_mem->mem_type == TTM_PL_SYSTEM &&
+	     new_mem->mem_type == TTM_PL_VRAM) ||
+	    (old_mem->mem_type == TTM_PL_VRAM &&
+	     new_mem->mem_type == TTM_PL_SYSTEM)) {
+		hop->fpfn = 0;
+		hop->lpfn = 0;
+		hop->mem_type = TTM_PL_TT;
+		hop->flags = 0;
+		return -EMULTIHOP;
+	}
+
 	if (new_mem->mem_type == TTM_PL_TT) {
 		r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem);
 		if (r)
@@ -351,17 +267,8 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
 		goto memcpy;
 	}
 
-	if (old_mem->mem_type == TTM_PL_VRAM &&
-	    new_mem->mem_type == TTM_PL_SYSTEM) {
-		r = radeon_move_vram_ram(bo, evict, ctx, 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, ctx, new_mem);
-	} else {
-		r = radeon_move_blit(bo, evict,
-				     new_mem, old_mem);
-	}
-
+	r = radeon_move_blit(bo, evict,
+			     new_mem, old_mem);
 	if (r) {
 memcpy:
 		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
-- 
2.27.0

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

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Ville Syrjälä]

On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
> Am 09.11.20 um 01:54 schrieb Dave Airlie:
> > @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
> >   	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
> >   		struct ttm_operation_ctx ctx = { false, false };
> >   		struct ttm_resource evict_mem;
> > +		struct ttm_place hop = {};
> 
> Please always use memset() if you want to zero initialize something in 
> the kernel, we had enough trouble with that.

What trouble is that? I've not heard of anything, and we use
={} quite extensively in drm land.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Christian König]

Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
>>>    	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
>>>    		struct ttm_operation_ctx ctx = { false, false };
>>>    		struct ttm_resource evict_mem;
>>> +		struct ttm_place hop = {};
>> Please always use memset() if you want to zero initialize something in
>> the kernel, we had enough trouble with that.
> What trouble is that? I've not heard of anything, and we use
> ={} quite extensively in drm land.

={} initializes only named fields, not padding.

The result is that for example when doing a hash or CRC of a structure 
you can come up with different results depending on the architecture 
and/or structure layout.

Another problem are information leaks from the kernel to userspace 
because of this.

Because of this Mesa for example strongly discourages using ={} for 
zeroing a structure.

Regards,
Christian.

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

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Ville Syrjälä]

On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
> > On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
> >> Am 09.11.20 um 01:54 schrieb Dave Airlie:
> >>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
> >>>    	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
> >>>    		struct ttm_operation_ctx ctx = { false, false };
> >>>    		struct ttm_resource evict_mem;
> >>> +		struct ttm_place hop = {};
> >> Please always use memset() if you want to zero initialize something in
> >> the kernel, we had enough trouble with that.
> > What trouble is that? I've not heard of anything, and we use
> > ={} quite extensively in drm land.
> 
> ={} initializes only named fields, not padding.

Has that actually happened?

> 
> The result is that for example when doing a hash or CRC of a structure 
> you can come up with different results depending on the architecture 
> and/or structure layout.
> 
> Another problem are information leaks from the kernel to userspace 
> because of this.
> 
> Because of this Mesa for example strongly discourages using ={} for 
> zeroing a structure.
> 
> Regards,
> Christian.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Christian König]

Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
>>>>>     	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
>>>>>     		struct ttm_operation_ctx ctx = { false, false };
>>>>>     		struct ttm_resource evict_mem;
>>>>> +		struct ttm_place hop = {};
>>>> Please always use memset() if you want to zero initialize something in
>>>> the kernel, we had enough trouble with that.
>>> What trouble is that? I've not heard of anything, and we use
>>> ={} quite extensively in drm land.
>> ={} initializes only named fields, not padding.
> Has that actually happened?

YES! Numerous times!

And the first few times it took us weeks to figure out what was actually 
happening.

Christian.

>
>> The result is that for example when doing a hash or CRC of a structure
>> you can come up with different results depending on the architecture
>> and/or structure layout.
>>
>> Another problem are information leaks from the kernel to userspace
>> because of this.
>>
>> Because of this Mesa for example strongly discourages using ={} for
>> zeroing a structure.
>>
>> Regards,
>> Christian.

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

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Ville Syrjälä]

On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
> > On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
> >> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
> >>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
> >>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
> >>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
> >>>>>     	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
> >>>>>     		struct ttm_operation_ctx ctx = { false, false };
> >>>>>     		struct ttm_resource evict_mem;
> >>>>> +		struct ttm_place hop = {};
> >>>> Please always use memset() if you want to zero initialize something in
> >>>> the kernel, we had enough trouble with that.
> >>> What trouble is that? I've not heard of anything, and we use
> >>> ={} quite extensively in drm land.
> >> ={} initializes only named fields, not padding.
> > Has that actually happened?
> 
> YES! Numerous times!

You wouldn't happen to have links/etc. to the discussion?
I'd like to check them out.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Christian König]

Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
> On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
>> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
>>> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
>>>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
>>>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
>>>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
>>>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
>>>>>>>      	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
>>>>>>>      		struct ttm_operation_ctx ctx = { false, false };
>>>>>>>      		struct ttm_resource evict_mem;
>>>>>>> +		struct ttm_place hop = {};
>>>>>> Please always use memset() if you want to zero initialize something in
>>>>>> the kernel, we had enough trouble with that.
>>>>> What trouble is that? I've not heard of anything, and we use
>>>>> ={} quite extensively in drm land.
>>>> ={} initializes only named fields, not padding.
>>> Has that actually happened?
>> YES! Numerous times!
> You wouldn't happen to have links/etc. to the discussion?
> I'd like to check them out.

Uff, that was years ago. Just google for something like "mesa memset 
hash", there was recently (~ the last year) another discussion because 
somebody ran into exactly that problem once more.

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

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Ville Syrjälä]

On Mon, Nov 09, 2020 at 09:48:04PM +0100, Christian König wrote:
> Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
> > On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
> >> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
> >>> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
> >>>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
> >>>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
> >>>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
> >>>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
> >>>>>>>      	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
> >>>>>>>      		struct ttm_operation_ctx ctx = { false, false };
> >>>>>>>      		struct ttm_resource evict_mem;
> >>>>>>> +		struct ttm_place hop = {};
> >>>>>> Please always use memset() if you want to zero initialize something in
> >>>>>> the kernel, we had enough trouble with that.
> >>>>> What trouble is that? I've not heard of anything, and we use
> >>>>> ={} quite extensively in drm land.
> >>>> ={} initializes only named fields, not padding.
> >>> Has that actually happened?
> >> YES! Numerous times!
> > You wouldn't happen to have links/etc. to the discussion?
> > I'd like to check them out.
> 
> Uff, that was years ago. Just google for something like "mesa memset 
> hash", there was recently (~ the last year) another discussion because 
> somebody ran into exactly that problem once more.

Found this:
https://gitlab.freedesktop.org/mesa/mesa/-/issues/2071
which does suprise me a bit. Though I suspect ={} might
behave differently since the compiler might treat it
more like a memset().

Also makes me wonder about padding in general, because IIRC
the standard allows padding to be clobbered even after
initialization whenever any member is getting written. So
I think technically there is no guaranteed way to clear
the padding unless you never store anything into the struct.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Dave Airlie]

On Tue, 10 Nov 2020 at 07:27, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Mon, Nov 09, 2020 at 09:48:04PM +0100, Christian König wrote:
> > Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
> > > On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
> > >> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
> > >>> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
> > >>>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
> > >>>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
> > >>>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
> > >>>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
> > >>>>>>>       if (bo->mem.mem_type != TTM_PL_SYSTEM) {
> > >>>>>>>               struct ttm_operation_ctx ctx = { false, false };
> > >>>>>>>               struct ttm_resource evict_mem;
> > >>>>>>> +             struct ttm_place hop = {};
> > >>>>>> Please always use memset() if you want to zero initialize something in
> > >>>>>> the kernel, we had enough trouble with that.
> > >>>>> What trouble is that? I've not heard of anything, and we use
> > >>>>> ={} quite extensively in drm land.
> > >>>> ={} initializes only named fields, not padding.
> > >>> Has that actually happened?
> > >> YES! Numerous times!
> > > You wouldn't happen to have links/etc. to the discussion?
> > > I'd like to check them out.
> >
> > Uff, that was years ago. Just google for something like "mesa memset
> > hash", there was recently (~ the last year) another discussion because
> > somebody ran into exactly that problem once more.
>
> Found this:
> https://gitlab.freedesktop.org/mesa/mesa/-/issues/2071
> which does suprise me a bit. Though I suspect ={} might
> behave differently since the compiler might treat it
> more like a memset().

It doesn't there's a bit of info out there on what happens, it really
only matters for structs we are passing to userspace, but it's
unlikely to matter here,
but I've changed this to memset in my local tree, because why not.

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

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Daniel Vetter]

On Mon, Nov 09, 2020 at 10:54:29AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Currently drivers get called to move a buffer, but if they have to
> move it temporarily through another space (SYSTEM->VRAM via TT)
> then they can end up with a lot of ttm->driver->ttm call stacks,
> if the temprorary space moves requires eviction.
> 
> Instead of letting the driver do all the placement/space for the
> temporary, allow it to report back (-EMULTIHOP) and a placement (hop)
> to the move code, which will then do the temporary move, and the
> correct placement move afterwards.
> 
> This removes a lot of code from drivers, at the expense of
> adding some midlayering. I've some further ideas on how to turn
> it inside out, but I think this is a good solution to the call
> stack problems.
> 
> v2: separate out the driver patches, add WARN for getting
> MULTHOP in paths we shouldn't (Daniel)
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |  3 +-
>  drivers/gpu/drm/drm_gem_vram_helper.c      |  3 +-
>  drivers/gpu/drm/nouveau/nouveau_bo.c       |  3 +-
>  drivers/gpu/drm/qxl/qxl_ttm.c              |  3 +-
>  drivers/gpu/drm/radeon/radeon_ttm.c        |  3 +-
>  drivers/gpu/drm/ttm/ttm_bo.c               | 68 +++++++++++++++++++---
>  drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c |  3 +-
>  include/drm/ttm/ttm_bo_driver.h            |  7 ++-
>  8 files changed, 77 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index c01c060e4ac5..ce0d82802333 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -656,7 +656,8 @@ static bool amdgpu_mem_visible(struct amdgpu_device *adev,
>   */
>  static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>  			  struct ttm_operation_ctx *ctx,
> -			  struct ttm_resource *new_mem)
> +			  struct ttm_resource *new_mem,
> +			  struct ttm_place *hop)
>  {
>  	struct amdgpu_device *adev;
>  	struct amdgpu_bo *abo;
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 16d68c04ea5d..2cec7b1482b8 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -964,7 +964,8 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
>  static int bo_driver_move(struct ttm_buffer_object *bo,
>  			  bool evict,
>  			  struct ttm_operation_ctx *ctx,
> -			  struct ttm_resource *new_mem)
> +			  struct ttm_resource *new_mem,
> +			  struct ttm_place *hop)
>  {
>  	struct drm_gem_vram_object *gbo;
>  
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 8133377d865d..fee07b9d19ed 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1023,7 +1023,8 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo,
>  static int
>  nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
>  		struct ttm_operation_ctx *ctx,
> -		struct ttm_resource *new_reg)
> +		struct ttm_resource *new_reg,
> +		struct ttm_place *hop)
>  {
>  	struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
>  	struct nouveau_bo *nvbo = nouveau_bo(bo);
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index a80d59634143..128c38c8a837 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -140,7 +140,8 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
>  
>  static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
>  		       struct ttm_operation_ctx *ctx,
> -		       struct ttm_resource *new_mem)
> +		       struct ttm_resource *new_mem,
> +		       struct ttm_place *hop)
>  {
>  	struct ttm_resource *old_mem = &bo->mem;
>  	int ret;
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 95038ac3382e..29062dbea299 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -303,7 +303,8 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
>  
>  static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>  			  struct ttm_operation_ctx *ctx,
> -			  struct ttm_resource *new_mem)
> +			  struct ttm_resource *new_mem,
> +			  struct ttm_place *hop)
>  {
>  	struct radeon_device *rdev;
>  	struct radeon_bo *rbo;
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index e2a124b3affb..9f840f2a7836 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -231,7 +231,8 @@ EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
>  
>  static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>  				  struct ttm_resource *mem, bool evict,
> -				  struct ttm_operation_ctx *ctx)
> +				  struct ttm_operation_ctx *ctx,
> +				  struct ttm_place *hop)
>  {
>  	struct ttm_bo_device *bdev = bo->bdev;
>  	struct ttm_resource_manager *old_man = ttm_manager_type(bdev, bo->mem.mem_type);
> @@ -259,9 +260,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>  		}
>  	}
>  
> -	ret = bdev->driver->move(bo, evict, ctx, mem);
> -	if (ret)
> +	ret = bdev->driver->move(bo, evict, ctx, mem, hop);
> +	if (ret) {
> +		if (ret == -EMULTIHOP)
> +			return ret;
>  		goto out_err;
> +	}
>  
>  	ctx->bytes_moved += bo->num_pages << PAGE_SHIFT;
>  	return 0;
> @@ -566,6 +570,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
>  	struct ttm_bo_device *bdev = bo->bdev;
>  	struct ttm_resource evict_mem;
>  	struct ttm_placement placement;
> +	struct ttm_place hop = {};
>  	int ret = 0;
>  
>  	dma_resv_assert_held(bo->base.resv);
> @@ -596,8 +601,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
>  		goto out;
>  	}
>  
> -	ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx);
> +	ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, &hop);
>  	if (unlikely(ret)) {
> +		WARN(ret == -EMULTIHOP, "Unexpected multihop in eviction - likely driver bug\n");
>  		if (ret != -ERESTARTSYS)
>  			pr_err("Buffer eviction failed\n");
>  		ttm_resource_free(bo, &evict_mem);
> @@ -936,11 +942,39 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>  }
>  EXPORT_SYMBOL(ttm_bo_mem_space);
>  
> +static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo,
> +				     struct ttm_resource *mem,
> +				     struct ttm_operation_ctx *ctx,
> +				     struct ttm_place *hop)
> +{
> +	struct ttm_placement hop_placement;
> +	int ret;
> +	struct ttm_resource hop_mem = *mem;
> +
> +	hop_mem.mm_node = NULL;
> +	hop_mem.mem_type = TTM_PL_SYSTEM;
> +	hop_mem.placement = 0;
> +
> +	hop_placement.num_placement = hop_placement.num_busy_placement = 1;
> +	hop_placement.placement = hop_placement.busy_placement = hop;
> +
> +	/* find space in the bounce domain */
> +	ret = ttm_bo_mem_space(bo, &hop_placement, &hop_mem, ctx);
> +	if (ret)
> +		return ret;
> +	/* move to the bounce domain */
> +	ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL);
> +	if (ret)
> +		return ret;
> +	return 0;
> +}
> +
>  static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
>  			      struct ttm_placement *placement,
>  			      struct ttm_operation_ctx *ctx)
>  {
>  	int ret = 0;
> +	struct ttm_place hop = {};
>  	struct ttm_resource mem;
>  
>  	dma_resv_assert_held(bo->base.resv);
> @@ -954,12 +988,25 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
>  
>  	/*
>  	 * Determine where to move the buffer.
> +	 *
> +	 * If driver determines move is going to need
> +	 * an extra step then it will return -EMULTIHOP
> +	 * and the buffer will be moved to the temporary
> +	 * stop and the driver will be called to make
> +	 * the second hop.
>  	 */
> +bounce:
>  	ret = ttm_bo_mem_space(bo, placement, &mem, ctx);
>  	if (ret)
> -		goto out_unlock;
> -	ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx);
> -out_unlock:
> +		return ret;
> +	ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop);
> +	if (ret == -EMULTIHOP) {
> +		ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, &hop);
> +		if (ret)
> +			return ret;
> +		/* try and move to final place now. */
> +		goto bounce;
> +	}
>  	if (ret)
>  		ttm_resource_free(bo, &mem);
>  	return ret;
> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
>  	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
>  		struct ttm_operation_ctx ctx = { false, false };
>  		struct ttm_resource evict_mem;
> +		struct ttm_place hop = {};
>  
>  		evict_mem = bo->mem;
>  		evict_mem.mm_node = NULL;
>  		evict_mem.placement = 0;
>  		evict_mem.mem_type = TTM_PL_SYSTEM;
>  
> -		ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx);
> -		if (unlikely(ret != 0))
> +		ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, &hop);
> +		if (unlikely(ret != 0)) {
> +			WARN(ret == -EMULTIHOP, "Unexpected multihop in swaput - likely driver bug.\n");
>  			goto out;
> +		}
>  	}
>  
>  	/**
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> index 51f70bea41cc..6a04261ce760 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> @@ -695,7 +695,8 @@ static void vmw_swap_notify(struct ttm_buffer_object *bo)
>  static int vmw_move(struct ttm_buffer_object *bo,
>  		    bool evict,
>  		    struct ttm_operation_ctx *ctx,
> -		    struct ttm_resource *new_mem)
> +		    struct ttm_resource *new_mem,
> +		    struct ttm_place *hop)
>  {
>  	struct ttm_resource_manager *old_man = ttm_manager_type(bo->bdev, bo->mem.mem_type);
>  	struct ttm_resource_manager *new_man = ttm_manager_type(bo->bdev, new_mem->mem_type);
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index da8208f43378..f02f7cf9ae90 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -121,6 +121,8 @@ struct ttm_bo_driver {
>  	 * Return the bo flags for a buffer which is not mapped to the hardware.
>  	 * These will be placed in proposed_flags so that when the move is
>  	 * finished, they'll end up in bo->mem.flags
> +	 * This should not cause multihop evictions, and the core will warn
> +	 * if one is proposed.
>  	 */
>  
>  	void (*evict_flags)(struct ttm_buffer_object *bo,
> @@ -134,12 +136,15 @@ struct ttm_bo_driver {
>  	 * the graphics address space
>  	 * @ctx: context for this move with parameters
>  	 * @new_mem: the new memory region receiving the buffer
> +	 @ @hop: placement for driver directed intermediate hop
>  	 *
>  	 * Move a buffer between two memory regions.
> +	 * Returns errno -EMULTIHOP if driver requests a hop
>  	 */
>  	int (*move)(struct ttm_buffer_object *bo, bool evict,
>  		    struct ttm_operation_ctx *ctx,
> -		    struct ttm_resource *new_mem);
> +		    struct ttm_resource *new_mem,
> +		    struct ttm_place *hop);

I think the warnings look all good, and everything else too.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

On the = {} bikeshed I frankly don't care, since for anything where this
matters vs memset you really should have a pad-less structure (by padding
the structure properly). So imo = {} is perfectly fine.
-Daniel

>  
>  	/**
>  	 * struct ttm_bo_driver_member verify_access
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/amdgpu/ttm: use multihop

[Daniel Vetter]

On Mon, Nov 09, 2020 at 10:54:30AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This removes the code to move resources directly between
> SYSTEM and VRAM in favour of using the core ttm mulithop code.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 136 +++---------------------
>  1 file changed, 13 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index ce0d82802333..e1458d575aa9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -512,119 +512,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>  	return r;
>  }
>  
> -/**
> - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer
> - *
> - * Called by amdgpu_bo_move().
> - */
> -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
> -				struct ttm_operation_ctx *ctx,
> -				struct ttm_resource *new_mem)
> -{
> -	struct ttm_resource *old_mem = &bo->mem;
> -	struct ttm_resource tmp_mem;
> -	struct ttm_place placements;
> -	struct ttm_placement placement;
> -	int r;
> -
> -	/* create space/pages for new_mem in GTT space */
> -	tmp_mem = *new_mem;
> -	tmp_mem.mm_node = NULL;
> -	placement.num_placement = 1;
> -	placement.placement = &placements;
> -	placement.num_busy_placement = 1;
> -	placement.busy_placement = &placements;
> -	placements.fpfn = 0;
> -	placements.lpfn = 0;
> -	placements.mem_type = TTM_PL_TT;
> -	placements.flags = 0;
> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -	if (unlikely(r)) {
> -		pr_err("Failed to find GTT space for blit from VRAM\n");
> -		return r;
> -	}
> -
> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -	if (unlikely(r))
> -		goto out_cleanup;
> -
> -	/* Bind the memory to the GTT space */
> -	r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -
> -	/* blit VRAM to GTT */
> -	r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -
> -	r = ttm_bo_wait_ctx(bo, ctx);
> -	if (unlikely(r))
> -		goto out_cleanup;
> -
> -	amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
> -	ttm_resource_free(bo, &bo->mem);
> -	ttm_bo_assign_mem(bo, new_mem);
> -out_cleanup:
> -	ttm_resource_free(bo, &tmp_mem);
> -	return r;
> -}
> -
> -/**
> - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM
> - *
> - * Called by amdgpu_bo_move().
> - */
> -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
> -				struct ttm_operation_ctx *ctx,
> -				struct ttm_resource *new_mem)
> -{
> -	struct ttm_resource *old_mem = &bo->mem;
> -	struct ttm_resource tmp_mem;
> -	struct ttm_placement placement;
> -	struct ttm_place placements;
> -	int r;
> -
> -	/* make space in GTT for old_mem buffer */
> -	tmp_mem = *new_mem;
> -	tmp_mem.mm_node = NULL;
> -	placement.num_placement = 1;
> -	placement.placement = &placements;
> -	placement.num_busy_placement = 1;
> -	placement.busy_placement = &placements;
> -	placements.fpfn = 0;
> -	placements.lpfn = 0;
> -	placements.mem_type = TTM_PL_TT;
> -	placements.flags = 0;
> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -	if (unlikely(r)) {
> -		pr_err("Failed to find GTT space for blit to VRAM\n");
> -		return r;
> -	}
> -
> -	/* move/bind old memory to GTT space */
> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -	if (unlikely(r))
> -		return r;
> -
> -	r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -
> -	ttm_bo_assign_mem(bo, &tmp_mem);
> -	/* copy to VRAM */
> -	r = amdgpu_move_blit(bo, evict, new_mem, old_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -out_cleanup:
> -	ttm_resource_free(bo, &tmp_mem);
> -	return r;
> -}
> -
>  /**
>   * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy
>   *
> @@ -664,6 +551,17 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>  	struct ttm_resource *old_mem = &bo->mem;
>  	int r;
>  
> +	if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> +	     new_mem->mem_type == TTM_PL_VRAM) ||
> +	    (old_mem->mem_type == TTM_PL_VRAM &&
> +	     new_mem->mem_type == TTM_PL_SYSTEM)) {
> +		hop->fpfn = 0;
> +		hop->lpfn = 0;
> +		hop->mem_type = TTM_PL_TT;
> +		hop->flags = 0;
> +		return -EMULTIHOP;

Helper for this might be neat, but total overkill. Like ttm_insert_tt_hop
and you pass the 2 other mem_types. Or maybe fully parametrized with
ttm_insert_hop:

	if (ttm_insert_tt_hope(old_mem, new_mem, TTM_PL_SYSTEM,
			TTM_PL_VRAM, hope))
		return -EMULTIPHOP;

Anyway real big bikeshed this one :-) Since Christian already checked the
details for the 3 driver patches from me just

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

on those three.
-Daniel

> +	}
> +
>  	if (new_mem->mem_type == TTM_PL_TT) {
>  		r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem);
>  		if (r)
> @@ -717,16 +615,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>  		goto memcpy;
>  	}
>  
> -	if (old_mem->mem_type == TTM_PL_VRAM &&
> -	    new_mem->mem_type == TTM_PL_SYSTEM) {
> -		r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem);
> -	} else if (old_mem->mem_type == TTM_PL_SYSTEM &&
> -		   new_mem->mem_type == TTM_PL_VRAM) {
> -		r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem);
> -	} else {
> -		r = amdgpu_move_blit(bo, evict,
> -				     new_mem, old_mem);
> -	}
> +	r = amdgpu_move_blit(bo, evict,
> +			     new_mem, old_mem);
>  
>  	if (r) {
>  memcpy:
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Ville Syrjälä]

On Tue, Nov 10, 2020 at 03:24:32PM +1000, Dave Airlie wrote:
> On Tue, 10 Nov 2020 at 07:27, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Mon, Nov 09, 2020 at 09:48:04PM +0100, Christian König wrote:
> > > Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
> > > > On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
> > > >> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
> > > >>> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
> > > >>>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
> > > >>>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
> > > >>>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
> > > >>>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
> > > >>>>>>>       if (bo->mem.mem_type != TTM_PL_SYSTEM) {
> > > >>>>>>>               struct ttm_operation_ctx ctx = { false, false };
> > > >>>>>>>               struct ttm_resource evict_mem;
> > > >>>>>>> +             struct ttm_place hop = {};
> > > >>>>>> Please always use memset() if you want to zero initialize something in
> > > >>>>>> the kernel, we had enough trouble with that.
> > > >>>>> What trouble is that? I've not heard of anything, and we use
> > > >>>>> ={} quite extensively in drm land.
> > > >>>> ={} initializes only named fields, not padding.
> > > >>> Has that actually happened?
> > > >> YES! Numerous times!
> > > > You wouldn't happen to have links/etc. to the discussion?
> > > > I'd like to check them out.
> > >
> > > Uff, that was years ago. Just google for something like "mesa memset
> > > hash", there was recently (~ the last year) another discussion because
> > > somebody ran into exactly that problem once more.
> >
> > Found this:
> > https://gitlab.freedesktop.org/mesa/mesa/-/issues/2071
> > which does suprise me a bit. Though I suspect ={} might
> > behave differently since the compiler might treat it
> > more like a memset().
> 
> It doesn't there's a bit of info out there on what happens, it really
> only matters for structs we are passing to userspace, but it's
> unlikely to matter here,
> but I've changed this to memset in my local tree, because why not.

Structs passed to userspace should really have no implicit padding.
One of those how to botch your ioctl things...

The main problems with memset() are:
- it's ugly
- people often get the sizeof wrong

I guess the latter could be alleviated with a cpp macro that
does the sizeof correctly for you.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Daniel Vetter]

On Tue, Nov 10, 2020 at 4:48 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Nov 10, 2020 at 03:24:32PM +1000, Dave Airlie wrote:
> > On Tue, 10 Nov 2020 at 07:27, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Mon, Nov 09, 2020 at 09:48:04PM +0100, Christian König wrote:
> > > > Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
> > > > > On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
> > > > >> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
> > > > >>> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
> > > > >>>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
> > > > >>>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
> > > > >>>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
> > > > >>>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
> > > > >>>>>>>       if (bo->mem.mem_type != TTM_PL_SYSTEM) {
> > > > >>>>>>>               struct ttm_operation_ctx ctx = { false, false };
> > > > >>>>>>>               struct ttm_resource evict_mem;
> > > > >>>>>>> +             struct ttm_place hop = {};
> > > > >>>>>> Please always use memset() if you want to zero initialize something in
> > > > >>>>>> the kernel, we had enough trouble with that.
> > > > >>>>> What trouble is that? I've not heard of anything, and we use
> > > > >>>>> ={} quite extensively in drm land.
> > > > >>>> ={} initializes only named fields, not padding.
> > > > >>> Has that actually happened?
> > > > >> YES! Numerous times!
> > > > > You wouldn't happen to have links/etc. to the discussion?
> > > > > I'd like to check them out.
> > > >
> > > > Uff, that was years ago. Just google for something like "mesa memset
> > > > hash", there was recently (~ the last year) another discussion because
> > > > somebody ran into exactly that problem once more.
> > >
> > > Found this:
> > > https://gitlab.freedesktop.org/mesa/mesa/-/issues/2071
> > > which does suprise me a bit. Though I suspect ={} might
> > > behave differently since the compiler might treat it
> > > more like a memset().
> >
> > It doesn't there's a bit of info out there on what happens, it really
> > only matters for structs we are passing to userspace, but it's
> > unlikely to matter here,
> > but I've changed this to memset in my local tree, because why not.
>
> Structs passed to userspace should really have no implicit padding.
> One of those how to botch your ioctl things...
>
> The main problems with memset() are:
> - it's ugly
> - people often get the sizeof wrong
>
> I guess the latter could be alleviated with a cpp macro that
> does the sizeof correctly for you.

Yeah imo not using = {} and instead insisting on memset is really bad
w/a for not padding your api structs properly. memset is only one
place where this goes horribly wrong.

I think for the gallium state tracker hasing issue memset is probably
ok ot use for these specifically, with a big comment explaining why.
And some code assurance that the memset is the same length you're
passing to the crc function and all that. But anywhere were it's more
(like anything related to api, which the gallium CSO hashing isnt) you
really want to have no implicit holes at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Christian König]

Am 09.11.20 um 01:54 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> Currently drivers get called to move a buffer, but if they have to
> move it temporarily through another space (SYSTEM->VRAM via TT)
> then they can end up with a lot of ttm->driver->ttm call stacks,
> if the temprorary space moves requires eviction.
>
> Instead of letting the driver do all the placement/space for the
> temporary, allow it to report back (-EMULTIHOP) and a placement (hop)
> to the move code, which will then do the temporary move, and the
> correct placement move afterwards.
>
> This removes a lot of code from drivers, at the expense of
> adding some midlayering. I've some further ideas on how to turn
> it inside out, but I think this is a good solution to the call
> stack problems.
>
> v2: separate out the driver patches, add WARN for getting
> MULTHOP in paths we shouldn't (Daniel)
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |  3 +-
>   drivers/gpu/drm/drm_gem_vram_helper.c      |  3 +-
>   drivers/gpu/drm/nouveau/nouveau_bo.c       |  3 +-
>   drivers/gpu/drm/qxl/qxl_ttm.c              |  3 +-
>   drivers/gpu/drm/radeon/radeon_ttm.c        |  3 +-
>   drivers/gpu/drm/ttm/ttm_bo.c               | 68 +++++++++++++++++++---
>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c |  3 +-
>   include/drm/ttm/ttm_bo_driver.h            |  7 ++-
>   8 files changed, 77 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index c01c060e4ac5..ce0d82802333 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -656,7 +656,8 @@ static bool amdgpu_mem_visible(struct amdgpu_device *adev,
>    */
>   static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>   			  struct ttm_operation_ctx *ctx,
> -			  struct ttm_resource *new_mem)
> +			  struct ttm_resource *new_mem,
> +			  struct ttm_place *hop)
>   {
>   	struct amdgpu_device *adev;
>   	struct amdgpu_bo *abo;
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 16d68c04ea5d..2cec7b1482b8 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -964,7 +964,8 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
>   static int bo_driver_move(struct ttm_buffer_object *bo,
>   			  bool evict,
>   			  struct ttm_operation_ctx *ctx,
> -			  struct ttm_resource *new_mem)
> +			  struct ttm_resource *new_mem,
> +			  struct ttm_place *hop)
>   {
>   	struct drm_gem_vram_object *gbo;
>   
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 8133377d865d..fee07b9d19ed 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1023,7 +1023,8 @@ nouveau_bo_vm_cleanup(struct ttm_buffer_object *bo,
>   static int
>   nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
>   		struct ttm_operation_ctx *ctx,
> -		struct ttm_resource *new_reg)
> +		struct ttm_resource *new_reg,
> +		struct ttm_place *hop)
>   {
>   	struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
>   	struct nouveau_bo *nvbo = nouveau_bo(bo);
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index a80d59634143..128c38c8a837 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -140,7 +140,8 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
>   
>   static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
>   		       struct ttm_operation_ctx *ctx,
> -		       struct ttm_resource *new_mem)
> +		       struct ttm_resource *new_mem,
> +		       struct ttm_place *hop)
>   {
>   	struct ttm_resource *old_mem = &bo->mem;
>   	int ret;
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 95038ac3382e..29062dbea299 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -303,7 +303,8 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
>   
>   static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>   			  struct ttm_operation_ctx *ctx,
> -			  struct ttm_resource *new_mem)
> +			  struct ttm_resource *new_mem,
> +			  struct ttm_place *hop)
>   {
>   	struct radeon_device *rdev;
>   	struct radeon_bo *rbo;
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index e2a124b3affb..9f840f2a7836 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -231,7 +231,8 @@ EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
>   
>   static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>   				  struct ttm_resource *mem, bool evict,
> -				  struct ttm_operation_ctx *ctx)
> +				  struct ttm_operation_ctx *ctx,
> +				  struct ttm_place *hop)
>   {
>   	struct ttm_bo_device *bdev = bo->bdev;
>   	struct ttm_resource_manager *old_man = ttm_manager_type(bdev, bo->mem.mem_type);
> @@ -259,9 +260,12 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>   		}
>   	}
>   
> -	ret = bdev->driver->move(bo, evict, ctx, mem);
> -	if (ret)
> +	ret = bdev->driver->move(bo, evict, ctx, mem, hop);
> +	if (ret) {
> +		if (ret == -EMULTIHOP)
> +			return ret;
>   		goto out_err;
> +	}
>   
>   	ctx->bytes_moved += bo->num_pages << PAGE_SHIFT;
>   	return 0;
> @@ -566,6 +570,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
>   	struct ttm_bo_device *bdev = bo->bdev;
>   	struct ttm_resource evict_mem;
>   	struct ttm_placement placement;
> +	struct ttm_place hop = {};
>   	int ret = 0;
>   
>   	dma_resv_assert_held(bo->base.resv);
> @@ -596,8 +601,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
>   		goto out;
>   	}
>   
> -	ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx);
> +	ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, &hop);
>   	if (unlikely(ret)) {
> +		WARN(ret == -EMULTIHOP, "Unexpected multihop in eviction - likely driver bug\n");
>   		if (ret != -ERESTARTSYS)
>   			pr_err("Buffer eviction failed\n");
>   		ttm_resource_free(bo, &evict_mem);
> @@ -936,11 +942,39 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>   }
>   EXPORT_SYMBOL(ttm_bo_mem_space);
>   
> +static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo,
> +				     struct ttm_resource *mem,
> +				     struct ttm_operation_ctx *ctx,
> +				     struct ttm_place *hop)
> +{
> +	struct ttm_placement hop_placement;
> +	int ret;
> +	struct ttm_resource hop_mem = *mem;
> +
> +	hop_mem.mm_node = NULL;
> +	hop_mem.mem_type = TTM_PL_SYSTEM;
> +	hop_mem.placement = 0;
> +
> +	hop_placement.num_placement = hop_placement.num_busy_placement = 1;
> +	hop_placement.placement = hop_placement.busy_placement = hop;
> +
> +	/* find space in the bounce domain */
> +	ret = ttm_bo_mem_space(bo, &hop_placement, &hop_mem, ctx);
> +	if (ret)
> +		return ret;
> +	/* move to the bounce domain */
> +	ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL);
> +	if (ret)
> +		return ret;
> +	return 0;
> +}
> +
>   static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
>   			      struct ttm_placement *placement,
>   			      struct ttm_operation_ctx *ctx)
>   {
>   	int ret = 0;
> +	struct ttm_place hop = {};
>   	struct ttm_resource mem;
>   
>   	dma_resv_assert_held(bo->base.resv);
> @@ -954,12 +988,25 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
>   
>   	/*
>   	 * Determine where to move the buffer.
> +	 *
> +	 * If driver determines move is going to need
> +	 * an extra step then it will return -EMULTIHOP
> +	 * and the buffer will be moved to the temporary
> +	 * stop and the driver will be called to make
> +	 * the second hop.
>   	 */
> +bounce:
>   	ret = ttm_bo_mem_space(bo, placement, &mem, ctx);
>   	if (ret)
> -		goto out_unlock;
> -	ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx);
> -out_unlock:
> +		return ret;
> +	ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop);
> +	if (ret == -EMULTIHOP) {
> +		ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, &hop);
> +		if (ret)
> +			return ret;
> +		/* try and move to final place now. */
> +		goto bounce;
> +	}
>   	if (ret)
>   		ttm_resource_free(bo, &mem);
>   	return ret;
> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
>   	if (bo->mem.mem_type != TTM_PL_SYSTEM) {
>   		struct ttm_operation_ctx ctx = { false, false };
>   		struct ttm_resource evict_mem;
> +		struct ttm_place hop = {};

Please always use memset() if you want to zero initialize something in 
the kernel, we had enough trouble with that.

Apart from that looks good to me,
Christian.

>   
>   		evict_mem = bo->mem;
>   		evict_mem.mm_node = NULL;
>   		evict_mem.placement = 0;
>   		evict_mem.mem_type = TTM_PL_SYSTEM;
>   
> -		ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx);
> -		if (unlikely(ret != 0))
> +		ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, &hop);
> +		if (unlikely(ret != 0)) {
> +			WARN(ret == -EMULTIHOP, "Unexpected multihop in swaput - likely driver bug.\n");
>   			goto out;
> +		}
>   	}
>   
>   	/**
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> index 51f70bea41cc..6a04261ce760 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> @@ -695,7 +695,8 @@ static void vmw_swap_notify(struct ttm_buffer_object *bo)
>   static int vmw_move(struct ttm_buffer_object *bo,
>   		    bool evict,
>   		    struct ttm_operation_ctx *ctx,
> -		    struct ttm_resource *new_mem)
> +		    struct ttm_resource *new_mem,
> +		    struct ttm_place *hop)
>   {
>   	struct ttm_resource_manager *old_man = ttm_manager_type(bo->bdev, bo->mem.mem_type);
>   	struct ttm_resource_manager *new_man = ttm_manager_type(bo->bdev, new_mem->mem_type);
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index da8208f43378..f02f7cf9ae90 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -121,6 +121,8 @@ struct ttm_bo_driver {
>   	 * Return the bo flags for a buffer which is not mapped to the hardware.
>   	 * These will be placed in proposed_flags so that when the move is
>   	 * finished, they'll end up in bo->mem.flags
> +	 * This should not cause multihop evictions, and the core will warn
> +	 * if one is proposed.
>   	 */
>   
>   	void (*evict_flags)(struct ttm_buffer_object *bo,
> @@ -134,12 +136,15 @@ struct ttm_bo_driver {
>   	 * the graphics address space
>   	 * @ctx: context for this move with parameters
>   	 * @new_mem: the new memory region receiving the buffer
> +	 @ @hop: placement for driver directed intermediate hop
>   	 *
>   	 * Move a buffer between two memory regions.
> +	 * Returns errno -EMULTIHOP if driver requests a hop
>   	 */
>   	int (*move)(struct ttm_buffer_object *bo, bool evict,
>   		    struct ttm_operation_ctx *ctx,
> -		    struct ttm_resource *new_mem);
> +		    struct ttm_resource *new_mem,
> +		    struct ttm_place *hop);
>   
>   	/**
>   	 * struct ttm_bo_driver_member verify_access

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

Re: [PATCH 2/4] drm/amdgpu/ttm: use multihop

[Christian König]

Am 09.11.20 um 01:54 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This removes the code to move resources directly between
> SYSTEM and VRAM in favour of using the core ttm mulithop code.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 136 +++---------------------
>   1 file changed, 13 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index ce0d82802333..e1458d575aa9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -512,119 +512,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>   	return r;
>   }
>   
> -/**
> - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer
> - *
> - * Called by amdgpu_bo_move().
> - */
> -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
> -				struct ttm_operation_ctx *ctx,
> -				struct ttm_resource *new_mem)
> -{
> -	struct ttm_resource *old_mem = &bo->mem;
> -	struct ttm_resource tmp_mem;
> -	struct ttm_place placements;
> -	struct ttm_placement placement;
> -	int r;
> -
> -	/* create space/pages for new_mem in GTT space */
> -	tmp_mem = *new_mem;
> -	tmp_mem.mm_node = NULL;
> -	placement.num_placement = 1;
> -	placement.placement = &placements;
> -	placement.num_busy_placement = 1;
> -	placement.busy_placement = &placements;
> -	placements.fpfn = 0;
> -	placements.lpfn = 0;
> -	placements.mem_type = TTM_PL_TT;
> -	placements.flags = 0;
> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -	if (unlikely(r)) {
> -		pr_err("Failed to find GTT space for blit from VRAM\n");
> -		return r;
> -	}
> -
> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -	if (unlikely(r))
> -		goto out_cleanup;
> -
> -	/* Bind the memory to the GTT space */
> -	r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -
> -	/* blit VRAM to GTT */
> -	r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -
> -	r = ttm_bo_wait_ctx(bo, ctx);
> -	if (unlikely(r))
> -		goto out_cleanup;
> -
> -	amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
> -	ttm_resource_free(bo, &bo->mem);
> -	ttm_bo_assign_mem(bo, new_mem);
> -out_cleanup:
> -	ttm_resource_free(bo, &tmp_mem);
> -	return r;
> -}
> -
> -/**
> - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM
> - *
> - * Called by amdgpu_bo_move().
> - */
> -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
> -				struct ttm_operation_ctx *ctx,
> -				struct ttm_resource *new_mem)
> -{
> -	struct ttm_resource *old_mem = &bo->mem;
> -	struct ttm_resource tmp_mem;
> -	struct ttm_placement placement;
> -	struct ttm_place placements;
> -	int r;
> -
> -	/* make space in GTT for old_mem buffer */
> -	tmp_mem = *new_mem;
> -	tmp_mem.mm_node = NULL;
> -	placement.num_placement = 1;
> -	placement.placement = &placements;
> -	placement.num_busy_placement = 1;
> -	placement.busy_placement = &placements;
> -	placements.fpfn = 0;
> -	placements.lpfn = 0;
> -	placements.mem_type = TTM_PL_TT;
> -	placements.flags = 0;
> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -	if (unlikely(r)) {
> -		pr_err("Failed to find GTT space for blit to VRAM\n");
> -		return r;
> -	}
> -
> -	/* move/bind old memory to GTT space */
> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -	if (unlikely(r))
> -		return r;
> -
> -	r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -
> -	ttm_bo_assign_mem(bo, &tmp_mem);
> -	/* copy to VRAM */
> -	r = amdgpu_move_blit(bo, evict, new_mem, old_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -out_cleanup:
> -	ttm_resource_free(bo, &tmp_mem);
> -	return r;
> -}
> -
>   /**
>    * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy
>    *
> @@ -664,6 +551,17 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>   	struct ttm_resource *old_mem = &bo->mem;
>   	int r;
>   
> +	if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> +	     new_mem->mem_type == TTM_PL_VRAM) ||
> +	    (old_mem->mem_type == TTM_PL_VRAM &&
> +	     new_mem->mem_type == TTM_PL_SYSTEM)) {
> +		hop->fpfn = 0;
> +		hop->lpfn = 0;
> +		hop->mem_type = TTM_PL_TT;
> +		hop->flags = 0;
> +		return -EMULTIHOP;
> +	}
> +
>   	if (new_mem->mem_type == TTM_PL_TT) {
>   		r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem);
>   		if (r)
> @@ -717,16 +615,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>   		goto memcpy;
>   	}
>   
> -	if (old_mem->mem_type == TTM_PL_VRAM &&
> -	    new_mem->mem_type == TTM_PL_SYSTEM) {
> -		r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem);
> -	} else if (old_mem->mem_type == TTM_PL_SYSTEM &&
> -		   new_mem->mem_type == TTM_PL_VRAM) {
> -		r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem);
> -	} else {
> -		r = amdgpu_move_blit(bo, evict,
> -				     new_mem, old_mem);
> -	}
> +	r = amdgpu_move_blit(bo, evict,
> +			     new_mem, old_mem);

Only a nit pick, but that can now probably be on one line.

Apart from that great cleanup. With the nits fixed the whole series is 
Reviewed-by: Christian König <christian.koenig@amd.com>

Regards,
Christian.

>   
>   	if (r) {
>   memcpy:

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

Re: [PATCH 4/4] drm/radeon/ttm: use multihop

[Thomas Zimmermann]

Hi

Am 09.11.20 um 01:54 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
> 
> This removes the code to move resources directly between
> SYSTEM and VRAM in favour of using the core ttm mulithop code.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/radeon/radeon_ttm.c | 119 +++-------------------------
>  1 file changed, 13 insertions(+), 106 deletions(-)

I got the following regression from that patch:

[   17.639429] ------------[ cut here ]------------
[   17.645322] Memory manager not clean during takedown.
[   17.650557] WARNING: CPU: 4 PID: 327 at drivers/gpu/drm/drm_mm.c:998
drm_mm_takedown+0x2e/0x40
[   17.659367] Modules linked in: hid_generic(E+) crct10dif_pclmul(E)
crc32_pclmul(E) ghash_clmulni_intel(E) radeon(E+) aesni_intel(E)
glue_helper(E) crypto_simd(E) drm_ttm_helper(E) cryptd(E) usbhid(E)
ttm(E) i915(E+) prime_numbers(E) w)
[   17.673721] hid-generic 0003:046A:0001.0001: input,hidraw0: USB HID
v1.00 Keyboard [HID 046a:0001] on usb-0000:00:14.0-7/input0
[   17.697411] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G
 E     5.10.0-rc3-1-default+ #639
[   17.718744] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
10/24/2018
[   17.718757] RIP: 0010:drm_mm_takedown+0x2e/0x40
[   17.718766] Code: 00 55 48 8d 6f 38 53 48 89 fb 48 89 ef e8 ba db 85
ff 48 8b 43 38 48 39 c5 75 03 5b 5d c3 48 c7 c7 40 ff b5 8e e8 b8 d8 62
00 <0f> 0b 5b 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 0f 1f 44 00
[   17.749995] RSP: 0018:ffffc900018d7790 EFLAGS: 00010282
[   17.755382] RAX: 0000000000000000 RBX: ffff8881490ad8a8 RCX:
0000000000000000
[   17.762713] RDX: 1ffff110f99fdd15 RSI: 0000000000000008 RDI:
fffff5200031aee8
[   17.762720] RBP: ffff8881490ad8e0 R08: 0000000000000001 R09:
ffff8887ccff80a7
[   17.762727] R10: ffffed10f99ff014 R11: 0000000000000001 R12:
0000000000000000
[   17.762734] R13: 0000000000000002 R14: ffff888158b40b58 R15:
ffff8881490ad998
[   17.762741] FS:  00007f15529ca940(0000) GS:ffff8887cce00000(0000)
knlGS:0000000000000000
[   17.762748] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   17.762754] CR2: 0000564e8a6283d8 CR3: 000000012aac6004 CR4:
00000000001706e0
[   17.762760] Call Trace:
[   17.762788]  ttm_range_man_fini+0x8b/0x150 [ttm]
[   17.762930]  radeon_ttm_fini+0xd1/0x210 [radeon]
[   17.763063]  radeon_bo_fini+0xf/0x60 [radeon]
[   17.763190]  si_fini+0x150/0x1d0 [radeon]
[   17.763313]  radeon_device_fini+0x61/0x177 [radeon]
[   17.763439]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
[   17.763564]  radeon_driver_load_kms+0x227/0x330 [radeon]
[   17.763593]  drm_dev_register+0x13b/0x2b0
[   17.763604]  ? drmm_add_final_kfree+0x46/0x60
[   17.763734]  radeon_pci_probe+0x19c/0x260 [radeon]
[   17.763854]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
[   17.763871]  local_pci_probe+0x74/0xc0
[   17.763893]  pci_call_probe+0xb7/0x1d0
[   17.763905]  ? pci_pm_suspend_noirq+0x440/0x440
[   17.763951]  pci_device_probe+0x102/0x140
[   17.763960]  ? driver_sysfs_add+0xe2/0x150
[   17.763978]  really_probe+0x185/0x680
[   17.764010]  driver_probe_device+0x13f/0x1d0
[   17.764032]  device_driver_attach+0x114/0x120
[   17.764048]  ? device_driver_attach+0x120/0x120
[   17.764058]  __driver_attach+0xb0/0x1a0
[   17.764076]  ? device_driver_attach+0x120/0x120
[   17.764083]  bus_for_each_dev+0xdd/0x120
[   17.764097]  ? subsys_dev_iter_exit+0x10/0x10
[   17.764133]  bus_add_driver+0x1fb/0x2e0
[   17.764161]  driver_register+0x103/0x180
[   17.764175]  ? 0xffffffffc102a000
[   17.764189]  do_one_initcall+0xbb/0x3a0
[   17.764204]  ? trace_event_raw_event_initcall_finish+0x120/0x120
[   17.764212]  ? mark_held_locks+0x23/0x90
[   17.764220]  ? lockdep_enabled+0x39/0x50
[   17.764231]  ? lock_is_held_type+0xb8/0xf0
[   17.764258]  ? rcu_read_lock_sched_held+0x3f/0x80
[   17.764269]  ? kasan_unpoison_shadow+0x33/0x40
[   17.764300]  do_init_module+0xfd/0x3c0
[   17.764327]  load_module+0xc04/0xc70
[   17.764359]  ? layout_and_allocate+0x260/0x260
[   17.764376]  ? kernel_read_file_from_fd+0x4b/0x90
[   17.764402]  __do_sys_finit_module+0xff/0x180
[   17.764415]  ? __ia32_sys_init_module+0x40/0x40
[   17.764508]  ? syscall_trace_enter.constprop.0+0x85/0x230
[   17.764531]  do_syscall_64+0x33/0x80
[   17.764543]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   17.764551] RIP: 0033:0x7f155355e799
[   17.764560] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
[   17.764566] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
0000000000000139
[   17.764579] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
00007f155355e799
[   17.764585] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
000000000000000f
[   17.764592] RBP: 0000000000020000 R08: 0000000000000000 R09:
000055c6d4417960
[   17.764598] R10: 000000000000000f R11: 0000000000000246 R12:
00007f155367d3a3
[   17.764605] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
000055c6d441c100
[   17.764670] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G
 E     5.10.0-rc3-1-default+ #639
[   17.764675] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
10/24/2018
[   17.764680] Call Trace:
[   17.764700]  dump_stack+0xae/0xe5
[   17.764716]  ? drm_mm_takedown+0x2e/0x40
[   17.764724]  __warn.cold+0x29/0x8a
[   17.764739]  ? drm_mm_takedown+0x2e/0x40
[   17.764755]  report_bug+0xcb/0xf0
[   17.764782]  handle_bug+0x38/0x90
[   17.764795]  exc_invalid_op+0x14/0x40
[   17.764809]  asm_exc_invalid_op+0x12/0x20
[   17.764816] RIP: 0010:drm_mm_takedown+0x2e/0x40
[   17.764824] Code: 00 55 48 8d 6f 38 53 48 89 fb 48 89 ef e8 ba db 85
ff 48 8b 43 38 48 39 c5 75 03 5b 5d c3 48 c7 c7 40 ff b5 8e e8 b8 d8 62
00 <0f> 0b 5b 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 0f 1f 44 00
[   17.764829] RSP: 0018:ffffc900018d7790 EFLAGS: 00010282
[   17.764839] RAX: 0000000000000000 RBX: ffff8881490ad8a8 RCX:
0000000000000000
[   17.764845] RDX: 1ffff110f99fdd15 RSI: 0000000000000008 RDI:
fffff5200031aee8
[   17.764851] RBP: ffff8881490ad8e0 R08: 0000000000000001 R09:
ffff8887ccff80a7
[   17.764856] R10: ffffed10f99ff014 R11: 0000000000000001 R12:
0000000000000000
[   17.764862] R13: 0000000000000002 R14: ffff888158b40b58 R15:
ffff8881490ad998
[   17.764939]  ttm_range_man_fini+0x8b/0x150 [ttm]
[   17.765071]  radeon_ttm_fini+0xd1/0x210 [radeon]
[   17.765198]  radeon_bo_fini+0xf/0x60 [radeon]
[   17.765322]  si_fini+0x150/0x1d0 [radeon]
[   17.765443]  radeon_device_fini+0x61/0x177 [radeon]
[   17.765563]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
[   17.765686]  radeon_driver_load_kms+0x227/0x330 [radeon]
[   17.765711]  drm_dev_register+0x13b/0x2b0
[   17.765722]  ? drmm_add_final_kfree+0x46/0x60
[   17.765849]  radeon_pci_probe+0x19c/0x260 [radeon]
[   17.765968]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
[   17.765984]  local_pci_probe+0x74/0xc0
[   17.766007]  pci_call_probe+0xb7/0x1d0
[   17.766020]  ? pci_pm_suspend_noirq+0x440/0x440
[   17.766065]  pci_device_probe+0x102/0x140
[   17.766073]  ? driver_sysfs_add+0xe2/0x150
[   17.766090]  really_probe+0x185/0x680
[   17.766121]  driver_probe_device+0x13f/0x1d0
[   17.766142]  device_driver_attach+0x114/0x120
[   17.766157]  ? device_driver_attach+0x120/0x120
[   17.766166]  __driver_attach+0xb0/0x1a0
[   17.766184]  ? device_driver_attach+0x120/0x120
[   17.766190]  bus_for_each_dev+0xdd/0x120
[   17.766202]  ? subsys_dev_iter_exit+0x10/0x10
[   17.766238]  bus_add_driver+0x1fb/0x2e0
[   17.766264]  driver_register+0x103/0x180
[   17.766278]  ? 0xffffffffc102a000
[   17.766291]  do_one_initcall+0xbb/0x3a0
[   17.766304]  ? trace_event_raw_event_initcall_finish+0x120/0x120
[   17.766312]  ? mark_held_locks+0x23/0x90
[   17.766319]  ? lockdep_enabled+0x39/0x50
[   17.766329]  ? lock_is_held_type+0xb8/0xf0
[   17.766356]  ? rcu_read_lock_sched_held+0x3f/0x80
[   17.766365]  ? kasan_unpoison_shadow+0x33/0x40
[   17.766392]  do_init_module+0xfd/0x3c0
[   17.766417]  load_module+0xc04/0xc70
[   17.766447]  ? layout_and_allocate+0x260/0x260
[   17.766463]  ? kernel_read_file_from_fd+0x4b/0x90
[   17.766488]  __do_sys_finit_module+0xff/0x180
[   17.766501]  ? __ia32_sys_init_module+0x40/0x40
[   17.766594]  ? syscall_trace_enter.constprop.0+0x85/0x230
[   17.766615]  do_syscall_64+0x33/0x80
[   17.766626]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   17.766634] RIP: 0033:0x7f155355e799
[   17.766641] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
[   17.766647] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
0000000000000139
[   17.766660] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
00007f155355e799
[   17.766666] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
000000000000000f
[   17.766672] RBP: 0000000000020000 R08: 0000000000000000 R09:
000055c6d4417960
[   17.766679] R10: 000000000000000f R11: 0000000000000246 R12:
00007f155367d3a3
[   17.766684] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
000055c6d441c100
[   17.766775] irq event stamp: 35403
[   17.766785] hardirqs last  enabled at (35409): [<ffffffff8d1b3bc1>]
console_trylock_spinning+0x1c1/0x1d0
[   17.766794] hardirqs last disabled at (35414): [<ffffffff8d1b3b70>]
console_trylock_spinning+0x170/0x1d0
[   17.766803] softirqs last  enabled at (35002): [<ffffffff8e6003dd>]
__do_softirq+0x3dd/0x554
[   17.766812] softirqs last disabled at (34989): [<ffffffff8e4010f2>]
asm_call_irq_on_stack+0x12/0x20
[   17.766818] ---[ end trace a1567ba1be224825 ]---
[   17.767050]
==================================================================
[   17.767110] BUG: KASAN: null-ptr-deref in
ttm_range_man_fini+0x35/0x150 [ttm]
[   17.767116] Write of size 1 at addr 0000000000000000 by task
systemd-udevd/327
[   17.767122]
[   17.767130] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G        W
 E     5.10.0-rc3-1-default+ #639
[   17.767135] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
10/24/2018
[   17.767141] Call Trace:
[   17.767161]  dump_stack+0xae/0xe5
[   17.767182]  ? ttm_range_man_fini+0x35/0x150 [ttm]
[   17.767193]  __kasan_report.cold+0x5/0x38
[   17.767225]  ? ttm_range_man_fini+0x35/0x150 [ttm]
[   17.767243]  kasan_report+0x3a/0x50
[   17.767262]  ttm_range_man_fini+0x35/0x150 [ttm]
[   17.767395]  radeon_ttm_fini+0xde/0x210 [radeon]
[   17.767525]  radeon_bo_fini+0xf/0x60 [radeon]
[   17.767651]  si_fini+0x150/0x1d0 [radeon]
[   17.767771]  radeon_device_fini+0x61/0x177 [radeon]
[   17.767893]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
[   17.768017]  radeon_driver_load_kms+0x227/0x330 [radeon]
[   17.768043]  drm_dev_register+0x13b/0x2b0
[   17.768054]  ? drmm_add_final_kfree+0x46/0x60
[   17.768182]  radeon_pci_probe+0x19c/0x260 [radeon]
[   17.768299]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
[   17.768315]  local_pci_probe+0x74/0xc0
[   17.768338]  pci_call_probe+0xb7/0x1d0
[   17.768351]  ? pci_pm_suspend_noirq+0x440/0x440
[   17.768397]  pci_device_probe+0x102/0x140
[   17.768404]  ? driver_sysfs_add+0xe2/0x150
[   17.768421]  really_probe+0x185/0x680
[   17.768452]  driver_probe_device+0x13f/0x1d0
[   17.768472]  device_driver_attach+0x114/0x120
[   17.768488]  ? device_driver_attach+0x120/0x120
[   17.768497]  __driver_attach+0xb0/0x1a0
[   17.768515]  ? device_driver_attach+0x120/0x120
[   17.768523]  bus_for_each_dev+0xdd/0x120
[   17.768536]  ? subsys_dev_iter_exit+0x10/0x10
[   17.768572]  bus_add_driver+0x1fb/0x2e0
[   17.768598]  driver_register+0x103/0x180
[   17.768611]  ? 0xffffffffc102a000
[   17.768625]  do_one_initcall+0xbb/0x3a0
[   17.768639]  ? trace_event_raw_event_initcall_finish+0x120/0x120
[   17.768646]  ? mark_held_locks+0x23/0x90
[   17.768654]  ? lockdep_enabled+0x39/0x50
[   17.768663]  ? lock_is_held_type+0xb8/0xf0
[   17.768689]  ? rcu_read_lock_sched_held+0x3f/0x80
[   17.768699]  ? kasan_unpoison_shadow+0x33/0x40
[   17.768728]  do_init_module+0xfd/0x3c0
[   17.768754]  load_module+0xc04/0xc70
[   17.768785]  ? layout_and_allocate+0x260/0x260
[   17.768800]  ? kernel_read_file_from_fd+0x4b/0x90
[   17.768825]  __do_sys_finit_module+0xff/0x180
[   17.768840]  ? __ia32_sys_init_module+0x40/0x40
[   17.768932]  ? syscall_trace_enter.constprop.0+0x85/0x230
[   17.768953]  do_syscall_64+0x33/0x80
[   17.768963]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   17.768971] RIP: 0033:0x7f155355e799
[   17.768979] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
[   17.768984] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
0000000000000139
[   17.768995] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
00007f155355e799
[   17.769001] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
000000000000000f
[   17.769008] RBP: 0000000000020000 R08: 0000000000000000 R09:
000055c6d4417960
[   17.769014] R10: 000000000000000f R11: 0000000000000246 R12:
00007f155367d3a3
[   17.769019] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
000055c6d441c100
[   17.769080]
==================================================================
[   17.769084] Disabling lock debugging due to kernel taint
[   17.772656] BUG: kernel NULL pointer dereference, address:
0000000000000000
[   17.786496] BTRFS: device fsid 293b6b08-509d-4de5-bde5-fc22f8707d6e
devid 1 transid 10671 /dev/sda3 scanned by systemd-udevd (312)
[   17.790614] #PF: supervisor write access in kernel mode
[   17.790616] #PF: error_code(0x0002) - not-present page
[   17.790619] PGD 0 P4D 0
[   18.843454] Oops: 0002 [#1] SMP KASAN PTI
[   18.843457] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G    B   W
 E     5.10.0-rc3-1-default+ #639
[   18.843458] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
10/24/2018
[   18.843468] RIP: 0010:ttm_range_man_fini+0x35/0x150 [ttm]
[   18.843472] Code: 4c 63 ee 41 54 55 49 8d 6d 18 53 4c 8d 34 ef 48 89
fb 4c 89 f7 48 83 ec 10 e8 57 07 86 cc 48 8b 2c eb 48 89 ef e8 0b 05 86
cc <c6> 45 00 00 48 89 ee 48 89 df e8 4c 05 00 00 41 89 c4 85 c0 74 12
[   18.890909] RSP: 0018:ffffc900018d77a8 EFLAGS: 00010282
[   18.890912] RAX: 0000000000000001 RBX: ffff888158b40a88 RCX:
dffffc0000000000
[   18.890913] RDX: 0000000000000007 RSI: 0000000000000004 RDI:
0000000000000297
[   18.890916] RBP: 0000000000000000 R08: 0000000000000000 R09:
ffffffff8f546ae3
[   18.917654] R10: fffffbfff1ea8d5c R11: 0000000000000001 R12:
ffff888158b40a88
[   18.917655] R13: 0000000000000001 R14: ffff888158b40b50 R15:
ffff88812e3e6490
[   18.917658] FS:  00007f15529ca940(0000) GS:ffff8887cce00000(0000)
knlGS:0000000000000000
[   18.917659] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   18.917660] CR2: 0000000000000000 CR3: 000000012aac6004 CR4:
00000000001706e0
[   18.917661] Call Trace:
[   18.917738]  radeon_ttm_fini+0xde/0x210 [radeon]
[   18.917802]  radeon_bo_fini+0xf/0x60 [radeon]
[   18.964662]  si_fini+0x150/0x1d0 [radeon]
[   18.964730]  radeon_device_fini+0x61/0x177 [radeon]
[   18.973659]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
[   18.973752]  radeon_driver_load_kms+0x227/0x330 [radeon]
[   18.984442]  drm_dev_register+0x13b/0x2b0
[   18.984445]  ? drmm_add_final_kfree+0x46/0x60
[   18.984505]  radeon_pci_probe+0x19c/0x260 [radeon]
[   18.997717]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
[   18.997723]  local_pci_probe+0x74/0xc0
[   19.007104]  pci_call_probe+0xb7/0x1d0
[   19.007107]  ? pci_pm_suspend_noirq+0x440/0x440
[   19.007112]  pci_device_probe+0x102/0x140
[   19.007117]  ? driver_sysfs_add+0xe2/0x150
[  OK     19.023596]  really_probe+0x185/0x680
[   19.023600]  driver_probe_device+0x13f/0x1d0
[   19.023612]  device_driver_attach+0x114/0x120
[   19.023615]  ? device_driver_attach+0x120/0x120
[   19.023617]  __driver_attach+0xb0/0x1a0
[   19.023619]  ? device_driver_attach+0x120/0x120
[   19.023623]  bus_for_each_dev+0xdd/0x120
0m] Found device[   19.054256]  ? subsys_dev_iter_exit+0x10/0x10
[   19.054260]  bus_add_driver+0x1fb/0x2e0
[   19.054264]  driver_register+0x103/0x180
[   19.054266]  ? 0xffffffffc102a000
[   19.054270]  do_one_initcall+0xbb/0x3a0
[   19.054273]  ? trace_event_raw_event_initcall_finish+0x120/0x120
[   19.054276]  ? mark_held_locks+0x23/0x90
[   19.054279]  ? lockdep_enabled+0x39/0x50
[   19.054282]  ? lock_is_held_type+0xb8/0xf0
[   19.054286]  ? rcu_read_lock_sched_held+0x3f/0x80
[   19.054288]  ? kasan_unpoison_shadow+0x33/0x40
 ST1000[   19.054292]  do_init_module+0xfd/0x3c0
[   19.054296]  load_module+0xc04/0xc70
DM003-1ER162 1   19.110979]  ? layout_and_allocate+0x260/0x260
[   19.110982]  ? kernel_read_file_from_fd+0x4b/0x90
[   19.110985]  __do_sys_finit_module+0xff/0x180
[   19.110988]  ? __ia32_sys_init_module+0x40/0x40
[   19.110995]  ? syscall_trace_enter.constprop.0+0x85/0x230
[   19.111001]  do_syscall_64+0x33/0x80
0m.
[   19.139555]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   19.139558] RIP: 0033:0x7f155355e799
[   19.139563] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
[   19.139566] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
0000000000000139
[   19.139571] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
00007f155355e799
[   19.139575] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
000000000000000f
[   19.189525] RBP: 0000000000020000 R08: 0000000000000000 R09:
000055c6d4417960
[   19.189526] R10: 000000000000000f R11: 0000000000000246 R12:
00007f155367d3a3
[   19.189528] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
000055c6d441c100
[   19.189532] Modules linked in: hid_generic(E) crct10dif_pclmul(E)
crc32_pclmul(E) ghash_clmulni_intel(E) radeon(E+) aesni_intel(E)
glue_helper(E) crypto_simd(E) drm_ttm_helper(E) cryptd(E) usbhid(E)
ttm(E) i915(E+) prime_numbers(E) wm)
[   19.211092] CR2: 0000000000000000
[   19.211136] ---[ end trace a1567ba1be224826 ]---
[   19.256281] RIP: 0010:ttm_range_man_fini+0x35/0x150 [ttm]
[   19.256284] Code: 4c 63 ee 41 54 55 49 8d 6d 18 53 4c 8d 34 ef 48 89
fb 4c 89 f7 48 83 ec 10 e8 57 07 86 cc 48 8b 2c eb 48 89 ef e8 0b 05 86
cc <c6> 45 00 00 48 89 ee 48 89 df e8 4c 05 00 00 41 89 c4 85 c0 74 12
[   19.256285] RSP: 0018:ffffc900018d77a8 EFLAGS: 00010282
[   19.256288] RAX: 0000000000000001 RBX: ffff888158b40a88 RCX:
dffffc0000000000
[   19.256289] RDX: 0000000000000007 RSI: 0000000000000004 RDI:
0000000000000297
[   19.256291] RBP: 0000000000000000 R08: 0000000000000000 R09:
ffffffff8f546ae3
[   19.256292] R10: fffffbfff1ea8d5c R11: 0000000000000001 R12:
ffff888158b40a88
[   19.256294] R13: 0000000000000001 R14: ffff888158b40b50 R15:
ffff88812e3e6490
[   19.256295] FS:  00007f15529ca940(0000) GS:ffff8887cce00000(0000)
knlGS:0000000000000000
[   19.256297] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   19.256298] CR2: 0000000000000000 CR3: 000000012aac6004 CR4:
00000000001706e0

The display remains dark after that. Reverting this patch restores
functionality.

Best regards
Thomas

> 
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 29062dbea299..788655ebafdb 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -206,101 +206,6 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
>  	return r;
>  }
>  
> -static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
> -				bool evict,
> -				struct ttm_operation_ctx *ctx,
> -				struct ttm_resource *new_mem)
> -{
> -	struct ttm_resource *old_mem = &bo->mem;
> -	struct ttm_resource tmp_mem;
> -	struct ttm_place placements;
> -	struct ttm_placement placement;
> -	int r;
> -
> -	tmp_mem = *new_mem;
> -	tmp_mem.mm_node = NULL;
> -	placement.num_placement = 1;
> -	placement.placement = &placements;
> -	placement.num_busy_placement = 1;
> -	placement.busy_placement = &placements;
> -	placements.fpfn = 0;
> -	placements.lpfn = 0;
> -	placements.mem_type = TTM_PL_TT;
> -	placements.flags = 0;
> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -	if (unlikely(r)) {
> -		return r;
> -	}
> -
> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -
> -	r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -	r = radeon_move_blit(bo, true, &tmp_mem, old_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -	r = ttm_bo_wait_ctx(bo, ctx);
> -	if (unlikely(r))
> -		goto out_cleanup;
> -
> -	radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
> -	ttm_resource_free(bo, &bo->mem);
> -	ttm_bo_assign_mem(bo, new_mem);
> -out_cleanup:
> -	ttm_resource_free(bo, &tmp_mem);
> -	return r;
> -}
> -
> -static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
> -				bool evict,
> -				struct ttm_operation_ctx *ctx,
> -				struct ttm_resource *new_mem)
> -{
> -	struct ttm_resource *old_mem = &bo->mem;
> -	struct ttm_resource tmp_mem;
> -	struct ttm_placement placement;
> -	struct ttm_place placements;
> -	int r;
> -
> -	tmp_mem = *new_mem;
> -	tmp_mem.mm_node = NULL;
> -	placement.num_placement = 1;
> -	placement.placement = &placements;
> -	placement.num_busy_placement = 1;
> -	placement.busy_placement = &placements;
> -	placements.fpfn = 0;
> -	placements.lpfn = 0;
> -	placements.mem_type = TTM_PL_TT;
> -	placements.flags = 0;
> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -	if (unlikely(r)) {
> -		return r;
> -	}
> -
> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -	if (unlikely(r))
> -		goto out_cleanup;
> -
> -	r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem);
> -	if (unlikely(r))
> -		goto out_cleanup;
> -
> -	ttm_bo_assign_mem(bo, &tmp_mem);
> -	r = radeon_move_blit(bo, true, new_mem, old_mem);
> -	if (unlikely(r)) {
> -		goto out_cleanup;
> -	}
> -out_cleanup:
> -	ttm_resource_free(bo, &tmp_mem);
> -	return r;
> -}
> -
>  static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>  			  struct ttm_operation_ctx *ctx,
>  			  struct ttm_resource *new_mem,
> @@ -311,6 +216,17 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>  	struct ttm_resource *old_mem = &bo->mem;
>  	int r;
>  
> +	if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> +	     new_mem->mem_type == TTM_PL_VRAM) ||
> +	    (old_mem->mem_type == TTM_PL_VRAM &&
> +	     new_mem->mem_type == TTM_PL_SYSTEM)) {
> +		hop->fpfn = 0;
> +		hop->lpfn = 0;
> +		hop->mem_type = TTM_PL_TT;
> +		hop->flags = 0;
> +		return -EMULTIHOP;
> +	}
> +
>  	if (new_mem->mem_type == TTM_PL_TT) {
>  		r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem);
>  		if (r)
> @@ -351,17 +267,8 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>  		goto memcpy;
>  	}
>  
> -	if (old_mem->mem_type == TTM_PL_VRAM &&
> -	    new_mem->mem_type == TTM_PL_SYSTEM) {
> -		r = radeon_move_vram_ram(bo, evict, ctx, 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, ctx, new_mem);
> -	} else {
> -		r = radeon_move_blit(bo, evict,
> -				     new_mem, old_mem);
> -	}
> -
> +	r = radeon_move_blit(bo, evict,
> +			     new_mem, old_mem);
>  	if (r) {
>  memcpy:
>  		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/radeon/ttm: use multihop

[Christian König]

Hi Dave,

amdgpu also blows up immediately, going to investigate now what's wrong 
here.

Christian.

Am 16.11.20 um 13:51 schrieb Thomas Zimmermann:
> Hi
>
> Am 09.11.20 um 01:54 schrieb Dave Airlie:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> This removes the code to move resources directly between
>> SYSTEM and VRAM in favour of using the core ttm mulithop code.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>>   drivers/gpu/drm/radeon/radeon_ttm.c | 119 +++-------------------------
>>   1 file changed, 13 insertions(+), 106 deletions(-)
> I got the following regression from that patch:
>
> [   17.639429] ------------[ cut here ]------------
> [   17.645322] Memory manager not clean during takedown.
> [   17.650557] WARNING: CPU: 4 PID: 327 at drivers/gpu/drm/drm_mm.c:998
> drm_mm_takedown+0x2e/0x40
> [   17.659367] Modules linked in: hid_generic(E+) crct10dif_pclmul(E)
> crc32_pclmul(E) ghash_clmulni_intel(E) radeon(E+) aesni_intel(E)
> glue_helper(E) crypto_simd(E) drm_ttm_helper(E) cryptd(E) usbhid(E)
> ttm(E) i915(E+) prime_numbers(E) w)
> [   17.673721] hid-generic 0003:046A:0001.0001: input,hidraw0: USB HID
> v1.00 Keyboard [HID 046a:0001] on usb-0000:00:14.0-7/input0
> [   17.697411] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G
>   E     5.10.0-rc3-1-default+ #639
> [   17.718744] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
> 10/24/2018
> [   17.718757] RIP: 0010:drm_mm_takedown+0x2e/0x40
> [   17.718766] Code: 00 55 48 8d 6f 38 53 48 89 fb 48 89 ef e8 ba db 85
> ff 48 8b 43 38 48 39 c5 75 03 5b 5d c3 48 c7 c7 40 ff b5 8e e8 b8 d8 62
> 00 <0f> 0b 5b 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 0f 1f 44 00
> [   17.749995] RSP: 0018:ffffc900018d7790 EFLAGS: 00010282
> [   17.755382] RAX: 0000000000000000 RBX: ffff8881490ad8a8 RCX:
> 0000000000000000
> [   17.762713] RDX: 1ffff110f99fdd15 RSI: 0000000000000008 RDI:
> fffff5200031aee8
> [   17.762720] RBP: ffff8881490ad8e0 R08: 0000000000000001 R09:
> ffff8887ccff80a7
> [   17.762727] R10: ffffed10f99ff014 R11: 0000000000000001 R12:
> 0000000000000000
> [   17.762734] R13: 0000000000000002 R14: ffff888158b40b58 R15:
> ffff8881490ad998
> [   17.762741] FS:  00007f15529ca940(0000) GS:ffff8887cce00000(0000)
> knlGS:0000000000000000
> [   17.762748] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   17.762754] CR2: 0000564e8a6283d8 CR3: 000000012aac6004 CR4:
> 00000000001706e0
> [   17.762760] Call Trace:
> [   17.762788]  ttm_range_man_fini+0x8b/0x150 [ttm]
> [   17.762930]  radeon_ttm_fini+0xd1/0x210 [radeon]
> [   17.763063]  radeon_bo_fini+0xf/0x60 [radeon]
> [   17.763190]  si_fini+0x150/0x1d0 [radeon]
> [   17.763313]  radeon_device_fini+0x61/0x177 [radeon]
> [   17.763439]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
> [   17.763564]  radeon_driver_load_kms+0x227/0x330 [radeon]
> [   17.763593]  drm_dev_register+0x13b/0x2b0
> [   17.763604]  ? drmm_add_final_kfree+0x46/0x60
> [   17.763734]  radeon_pci_probe+0x19c/0x260 [radeon]
> [   17.763854]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
> [   17.763871]  local_pci_probe+0x74/0xc0
> [   17.763893]  pci_call_probe+0xb7/0x1d0
> [   17.763905]  ? pci_pm_suspend_noirq+0x440/0x440
> [   17.763951]  pci_device_probe+0x102/0x140
> [   17.763960]  ? driver_sysfs_add+0xe2/0x150
> [   17.763978]  really_probe+0x185/0x680
> [   17.764010]  driver_probe_device+0x13f/0x1d0
> [   17.764032]  device_driver_attach+0x114/0x120
> [   17.764048]  ? device_driver_attach+0x120/0x120
> [   17.764058]  __driver_attach+0xb0/0x1a0
> [   17.764076]  ? device_driver_attach+0x120/0x120
> [   17.764083]  bus_for_each_dev+0xdd/0x120
> [   17.764097]  ? subsys_dev_iter_exit+0x10/0x10
> [   17.764133]  bus_add_driver+0x1fb/0x2e0
> [   17.764161]  driver_register+0x103/0x180
> [   17.764175]  ? 0xffffffffc102a000
> [   17.764189]  do_one_initcall+0xbb/0x3a0
> [   17.764204]  ? trace_event_raw_event_initcall_finish+0x120/0x120
> [   17.764212]  ? mark_held_locks+0x23/0x90
> [   17.764220]  ? lockdep_enabled+0x39/0x50
> [   17.764231]  ? lock_is_held_type+0xb8/0xf0
> [   17.764258]  ? rcu_read_lock_sched_held+0x3f/0x80
> [   17.764269]  ? kasan_unpoison_shadow+0x33/0x40
> [   17.764300]  do_init_module+0xfd/0x3c0
> [   17.764327]  load_module+0xc04/0xc70
> [   17.764359]  ? layout_and_allocate+0x260/0x260
> [   17.764376]  ? kernel_read_file_from_fd+0x4b/0x90
> [   17.764402]  __do_sys_finit_module+0xff/0x180
> [   17.764415]  ? __ia32_sys_init_module+0x40/0x40
> [   17.764508]  ? syscall_trace_enter.constprop.0+0x85/0x230
> [   17.764531]  do_syscall_64+0x33/0x80
> [   17.764543]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   17.764551] RIP: 0033:0x7f155355e799
> [   17.764560] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
> 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
> [   17.764566] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000139
> [   17.764579] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
> 00007f155355e799
> [   17.764585] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
> 000000000000000f
> [   17.764592] RBP: 0000000000020000 R08: 0000000000000000 R09:
> 000055c6d4417960
> [   17.764598] R10: 000000000000000f R11: 0000000000000246 R12:
> 00007f155367d3a3
> [   17.764605] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
> 000055c6d441c100
> [   17.764670] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G
>   E     5.10.0-rc3-1-default+ #639
> [   17.764675] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
> 10/24/2018
> [   17.764680] Call Trace:
> [   17.764700]  dump_stack+0xae/0xe5
> [   17.764716]  ? drm_mm_takedown+0x2e/0x40
> [   17.764724]  __warn.cold+0x29/0x8a
> [   17.764739]  ? drm_mm_takedown+0x2e/0x40
> [   17.764755]  report_bug+0xcb/0xf0
> [   17.764782]  handle_bug+0x38/0x90
> [   17.764795]  exc_invalid_op+0x14/0x40
> [   17.764809]  asm_exc_invalid_op+0x12/0x20
> [   17.764816] RIP: 0010:drm_mm_takedown+0x2e/0x40
> [   17.764824] Code: 00 55 48 8d 6f 38 53 48 89 fb 48 89 ef e8 ba db 85
> ff 48 8b 43 38 48 39 c5 75 03 5b 5d c3 48 c7 c7 40 ff b5 8e e8 b8 d8 62
> 00 <0f> 0b 5b 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 0f 1f 44 00
> [   17.764829] RSP: 0018:ffffc900018d7790 EFLAGS: 00010282
> [   17.764839] RAX: 0000000000000000 RBX: ffff8881490ad8a8 RCX:
> 0000000000000000
> [   17.764845] RDX: 1ffff110f99fdd15 RSI: 0000000000000008 RDI:
> fffff5200031aee8
> [   17.764851] RBP: ffff8881490ad8e0 R08: 0000000000000001 R09:
> ffff8887ccff80a7
> [   17.764856] R10: ffffed10f99ff014 R11: 0000000000000001 R12:
> 0000000000000000
> [   17.764862] R13: 0000000000000002 R14: ffff888158b40b58 R15:
> ffff8881490ad998
> [   17.764939]  ttm_range_man_fini+0x8b/0x150 [ttm]
> [   17.765071]  radeon_ttm_fini+0xd1/0x210 [radeon]
> [   17.765198]  radeon_bo_fini+0xf/0x60 [radeon]
> [   17.765322]  si_fini+0x150/0x1d0 [radeon]
> [   17.765443]  radeon_device_fini+0x61/0x177 [radeon]
> [   17.765563]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
> [   17.765686]  radeon_driver_load_kms+0x227/0x330 [radeon]
> [   17.765711]  drm_dev_register+0x13b/0x2b0
> [   17.765722]  ? drmm_add_final_kfree+0x46/0x60
> [   17.765849]  radeon_pci_probe+0x19c/0x260 [radeon]
> [   17.765968]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
> [   17.765984]  local_pci_probe+0x74/0xc0
> [   17.766007]  pci_call_probe+0xb7/0x1d0
> [   17.766020]  ? pci_pm_suspend_noirq+0x440/0x440
> [   17.766065]  pci_device_probe+0x102/0x140
> [   17.766073]  ? driver_sysfs_add+0xe2/0x150
> [   17.766090]  really_probe+0x185/0x680
> [   17.766121]  driver_probe_device+0x13f/0x1d0
> [   17.766142]  device_driver_attach+0x114/0x120
> [   17.766157]  ? device_driver_attach+0x120/0x120
> [   17.766166]  __driver_attach+0xb0/0x1a0
> [   17.766184]  ? device_driver_attach+0x120/0x120
> [   17.766190]  bus_for_each_dev+0xdd/0x120
> [   17.766202]  ? subsys_dev_iter_exit+0x10/0x10
> [   17.766238]  bus_add_driver+0x1fb/0x2e0
> [   17.766264]  driver_register+0x103/0x180
> [   17.766278]  ? 0xffffffffc102a000
> [   17.766291]  do_one_initcall+0xbb/0x3a0
> [   17.766304]  ? trace_event_raw_event_initcall_finish+0x120/0x120
> [   17.766312]  ? mark_held_locks+0x23/0x90
> [   17.766319]  ? lockdep_enabled+0x39/0x50
> [   17.766329]  ? lock_is_held_type+0xb8/0xf0
> [   17.766356]  ? rcu_read_lock_sched_held+0x3f/0x80
> [   17.766365]  ? kasan_unpoison_shadow+0x33/0x40
> [   17.766392]  do_init_module+0xfd/0x3c0
> [   17.766417]  load_module+0xc04/0xc70
> [   17.766447]  ? layout_and_allocate+0x260/0x260
> [   17.766463]  ? kernel_read_file_from_fd+0x4b/0x90
> [   17.766488]  __do_sys_finit_module+0xff/0x180
> [   17.766501]  ? __ia32_sys_init_module+0x40/0x40
> [   17.766594]  ? syscall_trace_enter.constprop.0+0x85/0x230
> [   17.766615]  do_syscall_64+0x33/0x80
> [   17.766626]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   17.766634] RIP: 0033:0x7f155355e799
> [   17.766641] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
> 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
> [   17.766647] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000139
> [   17.766660] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
> 00007f155355e799
> [   17.766666] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
> 000000000000000f
> [   17.766672] RBP: 0000000000020000 R08: 0000000000000000 R09:
> 000055c6d4417960
> [   17.766679] R10: 000000000000000f R11: 0000000000000246 R12:
> 00007f155367d3a3
> [   17.766684] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
> 000055c6d441c100
> [   17.766775] irq event stamp: 35403
> [   17.766785] hardirqs last  enabled at (35409): [<ffffffff8d1b3bc1>]
> console_trylock_spinning+0x1c1/0x1d0
> [   17.766794] hardirqs last disabled at (35414): [<ffffffff8d1b3b70>]
> console_trylock_spinning+0x170/0x1d0
> [   17.766803] softirqs last  enabled at (35002): [<ffffffff8e6003dd>]
> __do_softirq+0x3dd/0x554
> [   17.766812] softirqs last disabled at (34989): [<ffffffff8e4010f2>]
> asm_call_irq_on_stack+0x12/0x20
> [   17.766818] ---[ end trace a1567ba1be224825 ]---
> [   17.767050]
> ==================================================================
> [   17.767110] BUG: KASAN: null-ptr-deref in
> ttm_range_man_fini+0x35/0x150 [ttm]
> [   17.767116] Write of size 1 at addr 0000000000000000 by task
> systemd-udevd/327
> [   17.767122]
> [   17.767130] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G        W
>   E     5.10.0-rc3-1-default+ #639
> [   17.767135] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
> 10/24/2018
> [   17.767141] Call Trace:
> [   17.767161]  dump_stack+0xae/0xe5
> [   17.767182]  ? ttm_range_man_fini+0x35/0x150 [ttm]
> [   17.767193]  __kasan_report.cold+0x5/0x38
> [   17.767225]  ? ttm_range_man_fini+0x35/0x150 [ttm]
> [   17.767243]  kasan_report+0x3a/0x50
> [   17.767262]  ttm_range_man_fini+0x35/0x150 [ttm]
> [   17.767395]  radeon_ttm_fini+0xde/0x210 [radeon]
> [   17.767525]  radeon_bo_fini+0xf/0x60 [radeon]
> [   17.767651]  si_fini+0x150/0x1d0 [radeon]
> [   17.767771]  radeon_device_fini+0x61/0x177 [radeon]
> [   17.767893]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
> [   17.768017]  radeon_driver_load_kms+0x227/0x330 [radeon]
> [   17.768043]  drm_dev_register+0x13b/0x2b0
> [   17.768054]  ? drmm_add_final_kfree+0x46/0x60
> [   17.768182]  radeon_pci_probe+0x19c/0x260 [radeon]
> [   17.768299]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
> [   17.768315]  local_pci_probe+0x74/0xc0
> [   17.768338]  pci_call_probe+0xb7/0x1d0
> [   17.768351]  ? pci_pm_suspend_noirq+0x440/0x440
> [   17.768397]  pci_device_probe+0x102/0x140
> [   17.768404]  ? driver_sysfs_add+0xe2/0x150
> [   17.768421]  really_probe+0x185/0x680
> [   17.768452]  driver_probe_device+0x13f/0x1d0
> [   17.768472]  device_driver_attach+0x114/0x120
> [   17.768488]  ? device_driver_attach+0x120/0x120
> [   17.768497]  __driver_attach+0xb0/0x1a0
> [   17.768515]  ? device_driver_attach+0x120/0x120
> [   17.768523]  bus_for_each_dev+0xdd/0x120
> [   17.768536]  ? subsys_dev_iter_exit+0x10/0x10
> [   17.768572]  bus_add_driver+0x1fb/0x2e0
> [   17.768598]  driver_register+0x103/0x180
> [   17.768611]  ? 0xffffffffc102a000
> [   17.768625]  do_one_initcall+0xbb/0x3a0
> [   17.768639]  ? trace_event_raw_event_initcall_finish+0x120/0x120
> [   17.768646]  ? mark_held_locks+0x23/0x90
> [   17.768654]  ? lockdep_enabled+0x39/0x50
> [   17.768663]  ? lock_is_held_type+0xb8/0xf0
> [   17.768689]  ? rcu_read_lock_sched_held+0x3f/0x80
> [   17.768699]  ? kasan_unpoison_shadow+0x33/0x40
> [   17.768728]  do_init_module+0xfd/0x3c0
> [   17.768754]  load_module+0xc04/0xc70
> [   17.768785]  ? layout_and_allocate+0x260/0x260
> [   17.768800]  ? kernel_read_file_from_fd+0x4b/0x90
> [   17.768825]  __do_sys_finit_module+0xff/0x180
> [   17.768840]  ? __ia32_sys_init_module+0x40/0x40
> [   17.768932]  ? syscall_trace_enter.constprop.0+0x85/0x230
> [   17.768953]  do_syscall_64+0x33/0x80
> [   17.768963]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   17.768971] RIP: 0033:0x7f155355e799
> [   17.768979] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
> 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
> [   17.768984] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000139
> [   17.768995] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
> 00007f155355e799
> [   17.769001] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
> 000000000000000f
> [   17.769008] RBP: 0000000000020000 R08: 0000000000000000 R09:
> 000055c6d4417960
> [   17.769014] R10: 000000000000000f R11: 0000000000000246 R12:
> 00007f155367d3a3
> [   17.769019] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
> 000055c6d441c100
> [   17.769080]
> ==================================================================
> [   17.769084] Disabling lock debugging due to kernel taint
> [   17.772656] BUG: kernel NULL pointer dereference, address:
> 0000000000000000
> [   17.786496] BTRFS: device fsid 293b6b08-509d-4de5-bde5-fc22f8707d6e
> devid 1 transid 10671 /dev/sda3 scanned by systemd-udevd (312)
> [   17.790614] #PF: supervisor write access in kernel mode
> [   17.790616] #PF: error_code(0x0002) - not-present page
> [   17.790619] PGD 0 P4D 0
> [   18.843454] Oops: 0002 [#1] SMP KASAN PTI
> [   18.843457] CPU: 4 PID: 327 Comm: systemd-udevd Tainted: G    B   W
>   E     5.10.0-rc3-1-default+ #639
> [   18.843458] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24
> 10/24/2018
> [   18.843468] RIP: 0010:ttm_range_man_fini+0x35/0x150 [ttm]
> [   18.843472] Code: 4c 63 ee 41 54 55 49 8d 6d 18 53 4c 8d 34 ef 48 89
> fb 4c 89 f7 48 83 ec 10 e8 57 07 86 cc 48 8b 2c eb 48 89 ef e8 0b 05 86
> cc <c6> 45 00 00 48 89 ee 48 89 df e8 4c 05 00 00 41 89 c4 85 c0 74 12
> [   18.890909] RSP: 0018:ffffc900018d77a8 EFLAGS: 00010282
> [   18.890912] RAX: 0000000000000001 RBX: ffff888158b40a88 RCX:
> dffffc0000000000
> [   18.890913] RDX: 0000000000000007 RSI: 0000000000000004 RDI:
> 0000000000000297
> [   18.890916] RBP: 0000000000000000 R08: 0000000000000000 R09:
> ffffffff8f546ae3
> [   18.917654] R10: fffffbfff1ea8d5c R11: 0000000000000001 R12:
> ffff888158b40a88
> [   18.917655] R13: 0000000000000001 R14: ffff888158b40b50 R15:
> ffff88812e3e6490
> [   18.917658] FS:  00007f15529ca940(0000) GS:ffff8887cce00000(0000)
> knlGS:0000000000000000
> [   18.917659] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   18.917660] CR2: 0000000000000000 CR3: 000000012aac6004 CR4:
> 00000000001706e0
> [   18.917661] Call Trace:
> [   18.917738]  radeon_ttm_fini+0xde/0x210 [radeon]
> [   18.917802]  radeon_bo_fini+0xf/0x60 [radeon]
> [   18.964662]  si_fini+0x150/0x1d0 [radeon]
> [   18.964730]  radeon_device_fini+0x61/0x177 [radeon]
> [   18.973659]  radeon_driver_unload_kms+0x7a/0x130 [radeon]
> [   18.973752]  radeon_driver_load_kms+0x227/0x330 [radeon]
> [   18.984442]  drm_dev_register+0x13b/0x2b0
> [   18.984445]  ? drmm_add_final_kfree+0x46/0x60
> [   18.984505]  radeon_pci_probe+0x19c/0x260 [radeon]
> [   18.997717]  ? radeon_pmops_runtime_idle+0xe0/0xe0 [radeon]
> [   18.997723]  local_pci_probe+0x74/0xc0
> [   19.007104]  pci_call_probe+0xb7/0x1d0
> [   19.007107]  ? pci_pm_suspend_noirq+0x440/0x440
> [   19.007112]  pci_device_probe+0x102/0x140
> [   19.007117]  ? driver_sysfs_add+0xe2/0x150
> [  OK     19.023596]  really_probe+0x185/0x680
> [   19.023600]  driver_probe_device+0x13f/0x1d0
> [   19.023612]  device_driver_attach+0x114/0x120
> [   19.023615]  ? device_driver_attach+0x120/0x120
> [   19.023617]  __driver_attach+0xb0/0x1a0
> [   19.023619]  ? device_driver_attach+0x120/0x120
> [   19.023623]  bus_for_each_dev+0xdd/0x120
> 0m] Found device[   19.054256]  ? subsys_dev_iter_exit+0x10/0x10
> [   19.054260]  bus_add_driver+0x1fb/0x2e0
> [   19.054264]  driver_register+0x103/0x180
> [   19.054266]  ? 0xffffffffc102a000
> [   19.054270]  do_one_initcall+0xbb/0x3a0
> [   19.054273]  ? trace_event_raw_event_initcall_finish+0x120/0x120
> [   19.054276]  ? mark_held_locks+0x23/0x90
> [   19.054279]  ? lockdep_enabled+0x39/0x50
> [   19.054282]  ? lock_is_held_type+0xb8/0xf0
> [   19.054286]  ? rcu_read_lock_sched_held+0x3f/0x80
> [   19.054288]  ? kasan_unpoison_shadow+0x33/0x40
>   ST1000[   19.054292]  do_init_module+0xfd/0x3c0
> [   19.054296]  load_module+0xc04/0xc70
> DM003-1ER162 1   19.110979]  ? layout_and_allocate+0x260/0x260
> [   19.110982]  ? kernel_read_file_from_fd+0x4b/0x90
> [   19.110985]  __do_sys_finit_module+0xff/0x180
> [   19.110988]  ? __ia32_sys_init_module+0x40/0x40
> [   19.110995]  ? syscall_trace_enter.constprop.0+0x85/0x230
> [   19.111001]  do_syscall_64+0x33/0x80
> 0m.
> [   19.139555]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   19.139558] RIP: 0033:0x7f155355e799
> [   19.139563] Code: 48 8d 3d 3a bf 0c 00 0f 05 eb a5 66 0f 1f 44 00 00
> 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 76 0c 00 f7 d8 64 89 01 48
> [   19.139566] RSP: 002b:00007ffccda06428 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000139
> [   19.139571] RAX: ffffffffffffffda RBX: 000055c6d44158f0 RCX:
> 00007f155355e799
> [   19.139575] RDX: 0000000000000000 RSI: 00007f155367d3a3 RDI:
> 000000000000000f
> [   19.189525] RBP: 0000000000020000 R08: 0000000000000000 R09:
> 000055c6d4417960
> [   19.189526] R10: 000000000000000f R11: 0000000000000246 R12:
> 00007f155367d3a3
> [   19.189528] R13: 000055c6d4429ff0 R14: 0000000000000000 R15:
> 000055c6d441c100
> [   19.189532] Modules linked in: hid_generic(E) crct10dif_pclmul(E)
> crc32_pclmul(E) ghash_clmulni_intel(E) radeon(E+) aesni_intel(E)
> glue_helper(E) crypto_simd(E) drm_ttm_helper(E) cryptd(E) usbhid(E)
> ttm(E) i915(E+) prime_numbers(E) wm)
> [   19.211092] CR2: 0000000000000000
> [   19.211136] ---[ end trace a1567ba1be224826 ]---
> [   19.256281] RIP: 0010:ttm_range_man_fini+0x35/0x150 [ttm]
> [   19.256284] Code: 4c 63 ee 41 54 55 49 8d 6d 18 53 4c 8d 34 ef 48 89
> fb 4c 89 f7 48 83 ec 10 e8 57 07 86 cc 48 8b 2c eb 48 89 ef e8 0b 05 86
> cc <c6> 45 00 00 48 89 ee 48 89 df e8 4c 05 00 00 41 89 c4 85 c0 74 12
> [   19.256285] RSP: 0018:ffffc900018d77a8 EFLAGS: 00010282
> [   19.256288] RAX: 0000000000000001 RBX: ffff888158b40a88 RCX:
> dffffc0000000000
> [   19.256289] RDX: 0000000000000007 RSI: 0000000000000004 RDI:
> 0000000000000297
> [   19.256291] RBP: 0000000000000000 R08: 0000000000000000 R09:
> ffffffff8f546ae3
> [   19.256292] R10: fffffbfff1ea8d5c R11: 0000000000000001 R12:
> ffff888158b40a88
> [   19.256294] R13: 0000000000000001 R14: ffff888158b40b50 R15:
> ffff88812e3e6490
> [   19.256295] FS:  00007f15529ca940(0000) GS:ffff8887cce00000(0000)
> knlGS:0000000000000000
> [   19.256297] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   19.256298] CR2: 0000000000000000 CR3: 000000012aac6004 CR4:
> 00000000001706e0
>
> The display remains dark after that. Reverting this patch restores
> functionality.
>
> Best regards
> Thomas
>
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index 29062dbea299..788655ebafdb 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -206,101 +206,6 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
>>   	return r;
>>   }
>>   
>> -static int radeon_move_vram_ram(struct ttm_buffer_object *bo,
>> -				bool evict,
>> -				struct ttm_operation_ctx *ctx,
>> -				struct ttm_resource *new_mem)
>> -{
>> -	struct ttm_resource *old_mem = &bo->mem;
>> -	struct ttm_resource tmp_mem;
>> -	struct ttm_place placements;
>> -	struct ttm_placement placement;
>> -	int r;
>> -
>> -	tmp_mem = *new_mem;
>> -	tmp_mem.mm_node = NULL;
>> -	placement.num_placement = 1;
>> -	placement.placement = &placements;
>> -	placement.num_busy_placement = 1;
>> -	placement.busy_placement = &placements;
>> -	placements.fpfn = 0;
>> -	placements.lpfn = 0;
>> -	placements.mem_type = TTM_PL_TT;
>> -	placements.flags = 0;
>> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
>> -	if (unlikely(r)) {
>> -		return r;
>> -	}
>> -
>> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
>> -	if (unlikely(r)) {
>> -		goto out_cleanup;
>> -	}
>> -
>> -	r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem);
>> -	if (unlikely(r)) {
>> -		goto out_cleanup;
>> -	}
>> -	r = radeon_move_blit(bo, true, &tmp_mem, old_mem);
>> -	if (unlikely(r)) {
>> -		goto out_cleanup;
>> -	}
>> -	r = ttm_bo_wait_ctx(bo, ctx);
>> -	if (unlikely(r))
>> -		goto out_cleanup;
>> -
>> -	radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
>> -	ttm_resource_free(bo, &bo->mem);
>> -	ttm_bo_assign_mem(bo, new_mem);
>> -out_cleanup:
>> -	ttm_resource_free(bo, &tmp_mem);
>> -	return r;
>> -}
>> -
>> -static int radeon_move_ram_vram(struct ttm_buffer_object *bo,
>> -				bool evict,
>> -				struct ttm_operation_ctx *ctx,
>> -				struct ttm_resource *new_mem)
>> -{
>> -	struct ttm_resource *old_mem = &bo->mem;
>> -	struct ttm_resource tmp_mem;
>> -	struct ttm_placement placement;
>> -	struct ttm_place placements;
>> -	int r;
>> -
>> -	tmp_mem = *new_mem;
>> -	tmp_mem.mm_node = NULL;
>> -	placement.num_placement = 1;
>> -	placement.placement = &placements;
>> -	placement.num_busy_placement = 1;
>> -	placement.busy_placement = &placements;
>> -	placements.fpfn = 0;
>> -	placements.lpfn = 0;
>> -	placements.mem_type = TTM_PL_TT;
>> -	placements.flags = 0;
>> -	r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
>> -	if (unlikely(r)) {
>> -		return r;
>> -	}
>> -
>> -	r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
>> -	if (unlikely(r))
>> -		goto out_cleanup;
>> -
>> -	r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, &tmp_mem);
>> -	if (unlikely(r))
>> -		goto out_cleanup;
>> -
>> -	ttm_bo_assign_mem(bo, &tmp_mem);
>> -	r = radeon_move_blit(bo, true, new_mem, old_mem);
>> -	if (unlikely(r)) {
>> -		goto out_cleanup;
>> -	}
>> -out_cleanup:
>> -	ttm_resource_free(bo, &tmp_mem);
>> -	return r;
>> -}
>> -
>>   static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>>   			  struct ttm_operation_ctx *ctx,
>>   			  struct ttm_resource *new_mem,
>> @@ -311,6 +216,17 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>>   	struct ttm_resource *old_mem = &bo->mem;
>>   	int r;
>>   
>> +	if ((old_mem->mem_type == TTM_PL_SYSTEM &&
>> +	     new_mem->mem_type == TTM_PL_VRAM) ||
>> +	    (old_mem->mem_type == TTM_PL_VRAM &&
>> +	     new_mem->mem_type == TTM_PL_SYSTEM)) {
>> +		hop->fpfn = 0;
>> +		hop->lpfn = 0;
>> +		hop->mem_type = TTM_PL_TT;
>> +		hop->flags = 0;
>> +		return -EMULTIHOP;
>> +	}
>> +
>>   	if (new_mem->mem_type == TTM_PL_TT) {
>>   		r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem);
>>   		if (r)
>> @@ -351,17 +267,8 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>>   		goto memcpy;
>>   	}
>>   
>> -	if (old_mem->mem_type == TTM_PL_VRAM &&
>> -	    new_mem->mem_type == TTM_PL_SYSTEM) {
>> -		r = radeon_move_vram_ram(bo, evict, ctx, 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, ctx, new_mem);
>> -	} else {
>> -		r = radeon_move_blit(bo, evict,
>> -				     new_mem, old_mem);
>> -	}
>> -
>> +	r = radeon_move_blit(bo, evict,
>> +			     new_mem, old_mem);
>>   	if (r) {
>>   memcpy:
>>   		r = ttm_bo_move_memcpy(bo, ctx, new_mem);
>>

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

Re: [PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Christian König]

Am 10.11.20 um 18:11 schrieb Daniel Vetter:
> On Tue, Nov 10, 2020 at 4:48 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Tue, Nov 10, 2020 at 03:24:32PM +1000, Dave Airlie wrote:
>>> On Tue, 10 Nov 2020 at 07:27, Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>>> On Mon, Nov 09, 2020 at 09:48:04PM +0100, Christian König wrote:
>>>>> Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
>>>>>> On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
>>>>>>> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
>>>>>>>> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
>>>>>>>>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
>>>>>>>>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
>>>>>>>>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
>>>>>>>>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
>>>>>>>>>>>>        if (bo->mem.mem_type != TTM_PL_SYSTEM) {
>>>>>>>>>>>>                struct ttm_operation_ctx ctx = { false, false };
>>>>>>>>>>>>                struct ttm_resource evict_mem;
>>>>>>>>>>>> +             struct ttm_place hop = {};
>>>>>>>>>>> Please always use memset() if you want to zero initialize something in
>>>>>>>>>>> the kernel, we had enough trouble with that.
>>>>>>>>>> What trouble is that? I've not heard of anything, and we use
>>>>>>>>>> ={} quite extensively in drm land.
>>>>>>>>> ={} initializes only named fields, not padding.
>>>>>>>> Has that actually happened?
>>>>>>> YES! Numerous times!
>>>>>> You wouldn't happen to have links/etc. to the discussion?
>>>>>> I'd like to check them out.
>>>>> Uff, that was years ago. Just google for something like "mesa memset
>>>>> hash", there was recently (~ the last year) another discussion because
>>>>> somebody ran into exactly that problem once more.
>>>> Found this:
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2F-%2Fissues%2F2071&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C66be3d367f2b401b2e2808d8859ba4ee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637406250838085232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=X2z5zxsBvhXQvmd2oJzuN2YzDMGZnYUff6qxuJL%2BjLE%3D&amp;reserved=0
>>>> which does suprise me a bit. Though I suspect ={} might
>>>> behave differently since the compiler might treat it
>>>> more like a memset().
>>> It doesn't there's a bit of info out there on what happens, it really
>>> only matters for structs we are passing to userspace, but it's
>>> unlikely to matter here,
>>> but I've changed this to memset in my local tree, because why not.
>> Structs passed to userspace should really have no implicit padding.
>> One of those how to botch your ioctl things...
>>
>> The main problems with memset() are:
>> - it's ugly
>> - people often get the sizeof wrong
>>
>> I guess the latter could be alleviated with a cpp macro that
>> does the sizeof correctly for you.
> Yeah imo not using = {} and instead insisting on memset is really bad
> w/a for not padding your api structs properly. memset is only one
> place where this goes horribly wrong.

I'm not a fan of memset either, but there are more problems than just 
the padding in the UAPI.

I've seen at least the following in the wild:
1. UAPI information leak.
2. Hash and fingerprinting functions returning unstable results even 
when all meaningful members of a structure are initialized.
3. Valgrind/KASAN/Coverity randomly pointing this out as problem.
4. There is the discussion if ={} (not ISO C conform) or ={0] or even 
={{0}} should be used.

My preference would be to just teach compilers that not initializing 
padding in the kernel is a undesired behavior which should be avoided.

Regards
Christian.

>
> I think for the gallium state tracker hasing issue memset is probably
> ok ot use for these specifically, with a big comment explaining why.
> And some code assurance that the memset is the same length you're
> passing to the crc function and all that. But anywhere were it's more
> (like anything related to api, which the gallium CSO hashing isnt) you
> really want to have no implicit holes at all.
> -Daniel

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

Re: [PATCH 2/4] drm/amdgpu/ttm: use multihop

[Mike Lothian]

Hi

This patch is causing issues for me on both a Raven system and a Tonga
(PRIME) system

https://gitlab.freedesktop.org/drm/amd/-/issues/1405

It's only recently been merged into agd5f's tree - which is why I'm
only just noticing it

I realise this has now been submitted for rc1 - please can someone take a look

Thanks

Mike

On Mon, 9 Nov 2020 at 00:54, Dave Airlie <airlied@gmail.com> wrote:
>
> From: Dave Airlie <airlied@redhat.com>
>
> This removes the code to move resources directly between
> SYSTEM and VRAM in favour of using the core ttm mulithop code.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 136 +++---------------------
>  1 file changed, 13 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index ce0d82802333..e1458d575aa9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -512,119 +512,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>         return r;
>  }
>
> -/**
> - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer
> - *
> - * Called by amdgpu_bo_move().
> - */
> -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
> -                               struct ttm_operation_ctx *ctx,
> -                               struct ttm_resource *new_mem)
> -{
> -       struct ttm_resource *old_mem = &bo->mem;
> -       struct ttm_resource tmp_mem;
> -       struct ttm_place placements;
> -       struct ttm_placement placement;
> -       int r;
> -
> -       /* create space/pages for new_mem in GTT space */
> -       tmp_mem = *new_mem;
> -       tmp_mem.mm_node = NULL;
> -       placement.num_placement = 1;
> -       placement.placement = &placements;
> -       placement.num_busy_placement = 1;
> -       placement.busy_placement = &placements;
> -       placements.fpfn = 0;
> -       placements.lpfn = 0;
> -       placements.mem_type = TTM_PL_TT;
> -       placements.flags = 0;
> -       r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -       if (unlikely(r)) {
> -               pr_err("Failed to find GTT space for blit from VRAM\n");
> -               return r;
> -       }
> -
> -       r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -       if (unlikely(r))
> -               goto out_cleanup;
> -
> -       /* Bind the memory to the GTT space */
> -       r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> -       if (unlikely(r)) {
> -               goto out_cleanup;
> -       }
> -
> -       /* blit VRAM to GTT */
> -       r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem);
> -       if (unlikely(r)) {
> -               goto out_cleanup;
> -       }
> -
> -       r = ttm_bo_wait_ctx(bo, ctx);
> -       if (unlikely(r))
> -               goto out_cleanup;
> -
> -       amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
> -       ttm_resource_free(bo, &bo->mem);
> -       ttm_bo_assign_mem(bo, new_mem);
> -out_cleanup:
> -       ttm_resource_free(bo, &tmp_mem);
> -       return r;
> -}
> -
> -/**
> - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM
> - *
> - * Called by amdgpu_bo_move().
> - */
> -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
> -                               struct ttm_operation_ctx *ctx,
> -                               struct ttm_resource *new_mem)
> -{
> -       struct ttm_resource *old_mem = &bo->mem;
> -       struct ttm_resource tmp_mem;
> -       struct ttm_placement placement;
> -       struct ttm_place placements;
> -       int r;
> -
> -       /* make space in GTT for old_mem buffer */
> -       tmp_mem = *new_mem;
> -       tmp_mem.mm_node = NULL;
> -       placement.num_placement = 1;
> -       placement.placement = &placements;
> -       placement.num_busy_placement = 1;
> -       placement.busy_placement = &placements;
> -       placements.fpfn = 0;
> -       placements.lpfn = 0;
> -       placements.mem_type = TTM_PL_TT;
> -       placements.flags = 0;
> -       r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> -       if (unlikely(r)) {
> -               pr_err("Failed to find GTT space for blit to VRAM\n");
> -               return r;
> -       }
> -
> -       /* move/bind old memory to GTT space */
> -       r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> -       if (unlikely(r))
> -               return r;
> -
> -       r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> -       if (unlikely(r)) {
> -               goto out_cleanup;
> -       }
> -
> -       ttm_bo_assign_mem(bo, &tmp_mem);
> -       /* copy to VRAM */
> -       r = amdgpu_move_blit(bo, evict, new_mem, old_mem);
> -       if (unlikely(r)) {
> -               goto out_cleanup;
> -       }
> -out_cleanup:
> -       ttm_resource_free(bo, &tmp_mem);
> -       return r;
> -}
> -
>  /**
>   * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy
>   *
> @@ -664,6 +551,17 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>         struct ttm_resource *old_mem = &bo->mem;
>         int r;
>
> +       if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> +            new_mem->mem_type == TTM_PL_VRAM) ||
> +           (old_mem->mem_type == TTM_PL_VRAM &&
> +            new_mem->mem_type == TTM_PL_SYSTEM)) {
> +               hop->fpfn = 0;
> +               hop->lpfn = 0;
> +               hop->mem_type = TTM_PL_TT;
> +               hop->flags = 0;
> +               return -EMULTIHOP;
> +       }
> +
>         if (new_mem->mem_type == TTM_PL_TT) {
>                 r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem);
>                 if (r)
> @@ -717,16 +615,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>                 goto memcpy;
>         }
>
> -       if (old_mem->mem_type == TTM_PL_VRAM &&
> -           new_mem->mem_type == TTM_PL_SYSTEM) {
> -               r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem);
> -       } else if (old_mem->mem_type == TTM_PL_SYSTEM &&
> -                  new_mem->mem_type == TTM_PL_VRAM) {
> -               r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem);
> -       } else {
> -               r = amdgpu_move_blit(bo, evict,
> -                                    new_mem, old_mem);
> -       }
> +       r = amdgpu_move_blit(bo, evict,
> +                            new_mem, old_mem);
>
>         if (r) {
>  memcpy:
> --
> 2.27.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/amdgpu/ttm: use multihop

[Daniel Vetter]

On Sun, Dec 13, 2020 at 2:44 AM Mike Lothian <mike@fireburn.co.uk> wrote:
>
> Hi
>
> This patch is causing issues for me on both a Raven system and a Tonga
> (PRIME) system
>
> https://gitlab.freedesktop.org/drm/amd/-/issues/1405
>
> It's only recently been merged into agd5f's tree - which is why I'm
> only just noticing it
>
> I realise this has now been submitted for rc1 - please can someone take a look

Can you pls cherry-pick this one from drm-misc-next?

commit 9afdda82ee7f69b25cb5e6968e2d3d63e2313d12
Author: Christian König <christian.koenig@amd.com>
Date:   Wed Nov 25 15:32:23 2020 +0100

   drm/radeon: fix check order in radeon_bo_move

Christian, I think this fix should be cherry-picked to
drm-misc-next-fixes to make it into the merge window. It should have
been there from the start, but drm-misc-next-fixes wasn't ready yet I
think. Also adding Thomas to make sure the pull train goes out.
-Daniel

> Thanks
>
> Mike
>
> On Mon, 9 Nov 2020 at 00:54, Dave Airlie <airlied@gmail.com> wrote:
> >
> > From: Dave Airlie <airlied@redhat.com>
> >
> > This removes the code to move resources directly between
> > SYSTEM and VRAM in favour of using the core ttm mulithop code.
> >
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 136 +++---------------------
> >  1 file changed, 13 insertions(+), 123 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index ce0d82802333..e1458d575aa9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -512,119 +512,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
> >         return r;
> >  }
> >
> > -/**
> > - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer
> > - *
> > - * Called by amdgpu_bo_move().
> > - */
> > -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
> > -                               struct ttm_operation_ctx *ctx,
> > -                               struct ttm_resource *new_mem)
> > -{
> > -       struct ttm_resource *old_mem = &bo->mem;
> > -       struct ttm_resource tmp_mem;
> > -       struct ttm_place placements;
> > -       struct ttm_placement placement;
> > -       int r;
> > -
> > -       /* create space/pages for new_mem in GTT space */
> > -       tmp_mem = *new_mem;
> > -       tmp_mem.mm_node = NULL;
> > -       placement.num_placement = 1;
> > -       placement.placement = &placements;
> > -       placement.num_busy_placement = 1;
> > -       placement.busy_placement = &placements;
> > -       placements.fpfn = 0;
> > -       placements.lpfn = 0;
> > -       placements.mem_type = TTM_PL_TT;
> > -       placements.flags = 0;
> > -       r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> > -       if (unlikely(r)) {
> > -               pr_err("Failed to find GTT space for blit from VRAM\n");
> > -               return r;
> > -       }
> > -
> > -       r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> > -       if (unlikely(r))
> > -               goto out_cleanup;
> > -
> > -       /* Bind the memory to the GTT space */
> > -       r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> > -       if (unlikely(r)) {
> > -               goto out_cleanup;
> > -       }
> > -
> > -       /* blit VRAM to GTT */
> > -       r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem);
> > -       if (unlikely(r)) {
> > -               goto out_cleanup;
> > -       }
> > -
> > -       r = ttm_bo_wait_ctx(bo, ctx);
> > -       if (unlikely(r))
> > -               goto out_cleanup;
> > -
> > -       amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
> > -       ttm_resource_free(bo, &bo->mem);
> > -       ttm_bo_assign_mem(bo, new_mem);
> > -out_cleanup:
> > -       ttm_resource_free(bo, &tmp_mem);
> > -       return r;
> > -}
> > -
> > -/**
> > - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM
> > - *
> > - * Called by amdgpu_bo_move().
> > - */
> > -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
> > -                               struct ttm_operation_ctx *ctx,
> > -                               struct ttm_resource *new_mem)
> > -{
> > -       struct ttm_resource *old_mem = &bo->mem;
> > -       struct ttm_resource tmp_mem;
> > -       struct ttm_placement placement;
> > -       struct ttm_place placements;
> > -       int r;
> > -
> > -       /* make space in GTT for old_mem buffer */
> > -       tmp_mem = *new_mem;
> > -       tmp_mem.mm_node = NULL;
> > -       placement.num_placement = 1;
> > -       placement.placement = &placements;
> > -       placement.num_busy_placement = 1;
> > -       placement.busy_placement = &placements;
> > -       placements.fpfn = 0;
> > -       placements.lpfn = 0;
> > -       placements.mem_type = TTM_PL_TT;
> > -       placements.flags = 0;
> > -       r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> > -       if (unlikely(r)) {
> > -               pr_err("Failed to find GTT space for blit to VRAM\n");
> > -               return r;
> > -       }
> > -
> > -       /* move/bind old memory to GTT space */
> > -       r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> > -       if (unlikely(r))
> > -               return r;
> > -
> > -       r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> > -       if (unlikely(r)) {
> > -               goto out_cleanup;
> > -       }
> > -
> > -       ttm_bo_assign_mem(bo, &tmp_mem);
> > -       /* copy to VRAM */
> > -       r = amdgpu_move_blit(bo, evict, new_mem, old_mem);
> > -       if (unlikely(r)) {
> > -               goto out_cleanup;
> > -       }
> > -out_cleanup:
> > -       ttm_resource_free(bo, &tmp_mem);
> > -       return r;
> > -}
> > -
> >  /**
> >   * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy
> >   *
> > @@ -664,6 +551,17 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> >         struct ttm_resource *old_mem = &bo->mem;
> >         int r;
> >
> > +       if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> > +            new_mem->mem_type == TTM_PL_VRAM) ||
> > +           (old_mem->mem_type == TTM_PL_VRAM &&
> > +            new_mem->mem_type == TTM_PL_SYSTEM)) {
> > +               hop->fpfn = 0;
> > +               hop->lpfn = 0;
> > +               hop->mem_type = TTM_PL_TT;
> > +               hop->flags = 0;
> > +               return -EMULTIHOP;
> > +       }
> > +
> >         if (new_mem->mem_type == TTM_PL_TT) {
> >                 r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem);
> >                 if (r)
> > @@ -717,16 +615,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> >                 goto memcpy;
> >         }
> >
> > -       if (old_mem->mem_type == TTM_PL_VRAM &&
> > -           new_mem->mem_type == TTM_PL_SYSTEM) {
> > -               r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem);
> > -       } else if (old_mem->mem_type == TTM_PL_SYSTEM &&
> > -                  new_mem->mem_type == TTM_PL_VRAM) {
> > -               r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem);
> > -       } else {
> > -               r = amdgpu_move_blit(bo, evict,
> > -                                    new_mem, old_mem);
> > -       }
> > +       r = amdgpu_move_blit(bo, evict,
> > +                            new_mem, old_mem);
> >
> >         if (r) {
> >  memcpy:
> > --
> > 2.27.0
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/amdgpu/ttm: use multihop

[Alex Deucher]

On Mon, Dec 14, 2020 at 4:08 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Sun, Dec 13, 2020 at 2:44 AM Mike Lothian <mike@fireburn.co.uk> wrote:
> >
> > Hi
> >
> > This patch is causing issues for me on both a Raven system and a Tonga
> > (PRIME) system
> >
> > https://gitlab.freedesktop.org/drm/amd/-/issues/1405
> >
> > It's only recently been merged into agd5f's tree - which is why I'm
> > only just noticing it
> >
> > I realise this has now been submitted for rc1 - please can someone take a look
>
> Can you pls cherry-pick this one from drm-misc-next?
>
> commit 9afdda82ee7f69b25cb5e6968e2d3d63e2313d12
> Author: Christian König <christian.koenig@amd.com>
> Date:   Wed Nov 25 15:32:23 2020 +0100
>
>    drm/radeon: fix check order in radeon_bo_move
>
> Christian, I think this fix should be cherry-picked to
> drm-misc-next-fixes to make it into the merge window. It should have
> been there from the start, but drm-misc-next-fixes wasn't ready yet I
> think. Also adding Thomas to make sure the pull train goes out.

Looks like it's already in drm-misc-next-fixes:
https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-next-fixes&id=aefec40938e4a0e1214f9121520aba4d51697cd9

commit aefec40938e4a0e1214f9121520aba4d51697cd9
Author: Christian König <christian.koenig@amd.com>
Date:   Mon Nov 16 20:12:03 2020 +0100

    drm/amdgpu: fix check order in amdgpu_bo_move

    Reorder the code to fix checking if blitting is available.

    Signed-off-by: Christian König <christian.koenig@amd.com>
    Acked-by: Alex Deucher <alexander.deucher@amd.com>
    Link: https://patchwork.freedesktop.org/patch/401019/

Alex

> -Daniel
>
> > Thanks
> >
> > Mike
> >
> > On Mon, 9 Nov 2020 at 00:54, Dave Airlie <airlied@gmail.com> wrote:
> > >
> > > From: Dave Airlie <airlied@redhat.com>
> > >
> > > This removes the code to move resources directly between
> > > SYSTEM and VRAM in favour of using the core ttm mulithop code.
> > >
> > > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 136 +++---------------------
> > >  1 file changed, 13 insertions(+), 123 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > index ce0d82802333..e1458d575aa9 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > @@ -512,119 +512,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
> > >         return r;
> > >  }
> > >
> > > -/**
> > > - * amdgpu_move_vram_ram - Copy VRAM buffer to RAM buffer
> > > - *
> > > - * Called by amdgpu_bo_move().
> > > - */
> > > -static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict,
> > > -                               struct ttm_operation_ctx *ctx,
> > > -                               struct ttm_resource *new_mem)
> > > -{
> > > -       struct ttm_resource *old_mem = &bo->mem;
> > > -       struct ttm_resource tmp_mem;
> > > -       struct ttm_place placements;
> > > -       struct ttm_placement placement;
> > > -       int r;
> > > -
> > > -       /* create space/pages for new_mem in GTT space */
> > > -       tmp_mem = *new_mem;
> > > -       tmp_mem.mm_node = NULL;
> > > -       placement.num_placement = 1;
> > > -       placement.placement = &placements;
> > > -       placement.num_busy_placement = 1;
> > > -       placement.busy_placement = &placements;
> > > -       placements.fpfn = 0;
> > > -       placements.lpfn = 0;
> > > -       placements.mem_type = TTM_PL_TT;
> > > -       placements.flags = 0;
> > > -       r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> > > -       if (unlikely(r)) {
> > > -               pr_err("Failed to find GTT space for blit from VRAM\n");
> > > -               return r;
> > > -       }
> > > -
> > > -       r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> > > -       if (unlikely(r))
> > > -               goto out_cleanup;
> > > -
> > > -       /* Bind the memory to the GTT space */
> > > -       r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> > > -       if (unlikely(r)) {
> > > -               goto out_cleanup;
> > > -       }
> > > -
> > > -       /* blit VRAM to GTT */
> > > -       r = amdgpu_move_blit(bo, evict, &tmp_mem, old_mem);
> > > -       if (unlikely(r)) {
> > > -               goto out_cleanup;
> > > -       }
> > > -
> > > -       r = ttm_bo_wait_ctx(bo, ctx);
> > > -       if (unlikely(r))
> > > -               goto out_cleanup;
> > > -
> > > -       amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
> > > -       ttm_resource_free(bo, &bo->mem);
> > > -       ttm_bo_assign_mem(bo, new_mem);
> > > -out_cleanup:
> > > -       ttm_resource_free(bo, &tmp_mem);
> > > -       return r;
> > > -}
> > > -
> > > -/**
> > > - * amdgpu_move_ram_vram - Copy buffer from RAM to VRAM
> > > - *
> > > - * Called by amdgpu_bo_move().
> > > - */
> > > -static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict,
> > > -                               struct ttm_operation_ctx *ctx,
> > > -                               struct ttm_resource *new_mem)
> > > -{
> > > -       struct ttm_resource *old_mem = &bo->mem;
> > > -       struct ttm_resource tmp_mem;
> > > -       struct ttm_placement placement;
> > > -       struct ttm_place placements;
> > > -       int r;
> > > -
> > > -       /* make space in GTT for old_mem buffer */
> > > -       tmp_mem = *new_mem;
> > > -       tmp_mem.mm_node = NULL;
> > > -       placement.num_placement = 1;
> > > -       placement.placement = &placements;
> > > -       placement.num_busy_placement = 1;
> > > -       placement.busy_placement = &placements;
> > > -       placements.fpfn = 0;
> > > -       placements.lpfn = 0;
> > > -       placements.mem_type = TTM_PL_TT;
> > > -       placements.flags = 0;
> > > -       r = ttm_bo_mem_space(bo, &placement, &tmp_mem, ctx);
> > > -       if (unlikely(r)) {
> > > -               pr_err("Failed to find GTT space for blit to VRAM\n");
> > > -               return r;
> > > -       }
> > > -
> > > -       /* move/bind old memory to GTT space */
> > > -       r = ttm_tt_populate(bo->bdev, bo->ttm, ctx);
> > > -       if (unlikely(r))
> > > -               return r;
> > > -
> > > -       r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, &tmp_mem);
> > > -       if (unlikely(r)) {
> > > -               goto out_cleanup;
> > > -       }
> > > -
> > > -       ttm_bo_assign_mem(bo, &tmp_mem);
> > > -       /* copy to VRAM */
> > > -       r = amdgpu_move_blit(bo, evict, new_mem, old_mem);
> > > -       if (unlikely(r)) {
> > > -               goto out_cleanup;
> > > -       }
> > > -out_cleanup:
> > > -       ttm_resource_free(bo, &tmp_mem);
> > > -       return r;
> > > -}
> > > -
> > >  /**
> > >   * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy
> > >   *
> > > @@ -664,6 +551,17 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> > >         struct ttm_resource *old_mem = &bo->mem;
> > >         int r;
> > >
> > > +       if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> > > +            new_mem->mem_type == TTM_PL_VRAM) ||
> > > +           (old_mem->mem_type == TTM_PL_VRAM &&
> > > +            new_mem->mem_type == TTM_PL_SYSTEM)) {
> > > +               hop->fpfn = 0;
> > > +               hop->lpfn = 0;
> > > +               hop->mem_type = TTM_PL_TT;
> > > +               hop->flags = 0;
> > > +               return -EMULTIHOP;
> > > +       }
> > > +
> > >         if (new_mem->mem_type == TTM_PL_TT) {
> > >                 r = amdgpu_ttm_backend_bind(bo->bdev, bo->ttm, new_mem);
> > >                 if (r)
> > > @@ -717,16 +615,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> > >                 goto memcpy;
> > >         }
> > >
> > > -       if (old_mem->mem_type == TTM_PL_VRAM &&
> > > -           new_mem->mem_type == TTM_PL_SYSTEM) {
> > > -               r = amdgpu_move_vram_ram(bo, evict, ctx, new_mem);
> > > -       } else if (old_mem->mem_type == TTM_PL_SYSTEM &&
> > > -                  new_mem->mem_type == TTM_PL_VRAM) {
> > > -               r = amdgpu_move_ram_vram(bo, evict, ctx, new_mem);
> > > -       } else {
> > > -               r = amdgpu_move_blit(bo, evict,
> > > -                                    new_mem, old_mem);
> > > -       }
> > > +       r = amdgpu_move_blit(bo, evict,
> > > +                            new_mem, old_mem);
> > >
> > >         if (r) {
> > >  memcpy:
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel