Am 08.07.20 um 09:35 schrieb Chauhan, Madhav:

[AMD Public Use]

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Christian König Sent: Monday, July 6, 2020 11:18 PM To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: [PATCH 2/2] drm/amdgpu: stop allocating dummy GTT nodes

Now that TTM is fixed up we can finally stop that nonsense.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 104 ++++++-------------- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 18 +++- 2 files changed, 42 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c index 2c20d23d62d1..62cf4fbd803a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c @@ -150,60 +150,7 @@ static int amdgpu_gtt_mgr_fini(struct ttm_mem_type_manager *man) */ bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_mem_reg *mem) {

• struct amdgpu_gtt_node *node = mem->mm_node;
• return (node->node.start != AMDGPU_BO_INVALID_OFFSET);

-}

-/**

• • amdgpu_gtt_mgr_alloc - allocate new ranges
• • @man: TTM memory type manager
• • @tbo: TTM BO we need this range for
• • @place: placement flags and restrictions
• • @mem: the resulting mem object
• • Allocate the address space for a node.
• */

-static int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager *man,

• 		struct ttm_buffer_object *tbo,
  

• 		const struct ttm_place *place,
  

• 		struct ttm_mem_reg *mem)
  

-{

• struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev);
• struct amdgpu_gtt_mgr *mgr = man->priv;
• struct amdgpu_gtt_node *node = mem->mm_node;
• enum drm_mm_insert_mode mode;
• unsigned long fpfn, lpfn;
• int r;
• if (amdgpu_gtt_mgr_has_gart_addr(mem))

• return 0;
  

• if (place)

• fpfn = place->fpfn;
  

• else

• fpfn = 0;
  

• if (place && place->lpfn)

• lpfn = place->lpfn;
  

• else

• lpfn = adev->gart.num_cpu_pages;
  

• mode = DRM_MM_INSERT_BEST;
• if (place && place->flags & TTM_PL_FLAG_TOPDOWN)

• mode = DRM_MM_INSERT_HIGH;
  

• spin_lock(&mgr->lock);
• r = drm_mm_insert_node_in_range(&mgr->mm, &node->node, mem->num_pages,

• 			mem->page_alignment, 0, fpfn, lpfn,
  

• 			mode);
  

• spin_unlock(&mgr->lock);
• if (!r)

• mem->start = node->node.start;
  

• return r;

• return mem->mm_node != NULL; }

  /**

@@ -234,29 +181,37 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager *man, atomic64_sub(mem->num_pages, &mgr->available); spin_unlock(&mgr->lock);

• if (!place->lpfn) {

• mem->mm_node = NULL;
  

• mem->start = AMDGPU_BO_INVALID_OFFSET;
  

• return 0;
  

• }
• node = kzalloc(sizeof(*node), GFP_KERNEL); if (!node) { r = -ENOMEM; goto err_out; }

• node->node.start = AMDGPU_BO_INVALID_OFFSET;

• node->node.size = mem->num_pages; node->tbo = tbo;

• mem->mm_node = node;

• if (place->fpfn || place->lpfn || place->flags & TTM_PL_FLAG_TOPDOWN) {

• r = amdgpu_gtt_mgr_alloc(man, tbo, place, mem);
  

• if (unlikely(r)) {
  

• 	kfree(node);
  

• 	mem->mm_node = NULL;
  

• 	goto err_out;
  

• }
  

• } else {

• mem->start = node->node.start;
  

• }

• spin_lock(&mgr->lock);

• r = drm_mm_insert_node_in_range(&mgr->mm, &node->node, mem->num_pages,

• 			mem->page_alignment, 0, place->fpfn,
  

• 			place->lpfn, DRM_MM_INSERT_BEST);
  

• spin_unlock(&mgr->lock);

• if (unlikely(r))

• goto err_free;
  

• mem->mm_node = node;

• mem->start = node->node.start;

  return 0;

+err_free:

• kfree(node);
• err_out: atomic64_add(mem->num_pages, &mgr->available);

@@ -279,17 +234,14 @@ static void amdgpu_gtt_mgr_del(struct ttm_mem_type_manager *man, struct amdgpu_gtt_mgr *mgr = man->priv; struct amdgpu_gtt_node *node = mem->mm_node;

• if (!node)

• return;
  

• spin_lock(&mgr->lock);
• if (node->node.start != AMDGPU_BO_INVALID_OFFSET)

• if (node) {

• spin_lock(&mgr->lock);
  

  drm_mm_remove_node(&node->node);

• spin_unlock(&mgr->lock);
• atomic64_add(mem->num_pages, &mgr->available);

• spin_unlock(&mgr->lock);
  

• kfree(node);
  

• }

• kfree(node);
• mem->mm_node = NULL;

• atomic64_add(mem->num_pages, &mgr->available); }

Looks fine to me, nitpick: Should we update the documentation of amdgpu_gtt_mgr_new() which still says "Dummy"??

Yes, that's probably a good point.

Thanks, Christian.

Regards, Madhav

/** diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 38d2a7fb5698..50503f860fce 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -429,12 +429,22 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev, }

src_offset = src->offset;

• src_mm = amdgpu_find_mm_node(src->mem, &src_offset);
• src_node_size = (src_mm->size << PAGE_SHIFT) - src_offset;

• if (src->mem->mm_node) {

• src_mm = amdgpu_find_mm_node(src->mem, &src_offset);
  

• src_node_size = (src_mm->size << PAGE_SHIFT) - src_offset;
  

• } else {

• src_mm = NULL;
  

• src_node_size = ULLONG_MAX;
  

• }

  dst_offset = dst->offset;

• dst_mm = amdgpu_find_mm_node(dst->mem, &dst_offset);
• dst_node_size = (dst_mm->size << PAGE_SHIFT) - dst_offset;

• if (dst->mem->mm_node) {

• dst_mm = amdgpu_find_mm_node(dst->mem, &dst_offset);
  

• dst_node_size = (dst_mm->size << PAGE_SHIFT) - dst_offset;
  

• } else {

• dst_mm = NULL;
  

• dst_node_size = ULLONG_MAX;
  

• }

  mutex_lock(&adev->mman.gtt_window_lock);

-- 2.17.1

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

Am 07.07.20 um 21:16 schrieb Chauhan, Madhav:

[AMD Public Use]

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Christian König Sent: Monday, July 6, 2020 11:18 PM To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: [PATCH 1/2] drm/ttm: further cleanup ttm_mem_reg handling

Stop touching the backend private pointer alltogether and make sure we never put the same mem twice by.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/ttm/ttm_bo.c | 46 +++++++++++++++++++-------------- include/drm/ttm/ttm_bo_driver.h | 2 -- 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 0c13fe96c7e3..7be36b9996ed 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -312,7 +312,6 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, if (bdev->driver->move_notify) bdev->driver->move_notify(bo, evict, mem); bo->mem = *mem;

• 	mem->mm_node = NULL;
  goto moved;
  

  } }

@@ -616,7 +615,6 @@ static void ttm_bo_release(struct kref *kref) ttm_bo_cleanup_memtype_use(bo); dma_resv_unlock(bo->base.resv);

• BUG_ON(bo->mem.mm_node != NULL); atomic_dec(&ttm_bo_glob.bo_count); dma_fence_put(bo->moving); if (!ttm_bo_uses_embedded_gem_object(bo))

@@ -843,12 +841,29 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, return ret; }

+static int ttm_bo_mem_get(struct ttm_buffer_object *bo,

• 	  const struct ttm_place *place,
  

• 	  struct ttm_mem_reg *mem)
  

+{

• struct ttm_mem_type_manager *man = &bo->bdev->man[mem->mem_type];
• mem->mm_node = NULL;
• if (!man->func || !man->func->get_node)

• return 0;
  

• return man->func->get_node(man, bo, place, mem); }

Should not we export this as a symbol, so that it can be used similar to ttm_bo_mem_put in drm drivers??

No, it's a well known design problem of TTM that the exported counter part of ttm_bo_mem_put() is ttm_bo_mem_space().

I have it on my TODO list to fix this, but that needs a bigger change.

Regards, Christian.

Regards, Madhav

void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem) { struct ttm_mem_type_manager *man = &bo->bdev->man[mem->mem_type];

• if (mem->mm_node)

• (*man->func->put_node)(man, mem);
  

• if (!man->func || !man->func->put_node)

• return;
  

• man->func->put_node(man, mem);
• mem->mm_node = NULL;
• mem->mem_type = TTM_PL_SYSTEM; } EXPORT_SYMBOL(ttm_bo_mem_put);

@@ -902,7 +917,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,

ticket = dma_resv_locking_ctx(bo->base.resv); do {

• ret = (*man->func->get_node)(man, bo, place, mem);
  

• ret = ttm_bo_mem_get(bo, place, mem);
  

  if (likely(!ret)) break; if (unlikely(ret != -ENOSPC))

@@ -1032,7 +1047,6 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, if (unlikely(ret)) return ret;

• mem->mm_node = NULL; for (i = 0; i < placement->num_placement; ++i) { const struct ttm_place *place = &placement->placement[i]; struct ttm_mem_type_manager *man;

@@ -1044,20 +1058,16 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, goto error;

type_found = true;

• mem->mm_node = NULL;
  

• if (mem->mem_type == TTM_PL_SYSTEM)
  

• 	return 0;
  

• man = &bdev->man[mem->mem_type];
  

• ret = (*man->func->get_node)(man, bo, place, mem);
  

• ret = ttm_bo_mem_get(bo, place, mem);
  

  if (ret == -ENOSPC) continue; if (unlikely(ret)) goto error;

• man = &bdev->man[mem->mem_type];
  

  ret = ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu); if (unlikely(ret)) {

• 	(*man->func->put_node)(man, mem);
  

• 	ttm_bo_mem_put(bo, mem);
  if (ret == -EBUSY)
  	continue;
  
  

@@ -1076,12 +1086,8 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, goto error;

type_found = true;

• mem->mm_node = NULL;
  

• if (mem->mem_type == TTM_PL_SYSTEM)
  

• 	return 0;
  

• ret = ttm_bo_mem_force_space(bo, place, mem, ctx);

• if (ret == 0 && mem->mm_node)
  

• if (likely(!ret))
  return 0;
  
  

  if (ret && ret != -EBUSY)

@@ -1129,7 +1135,7 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, goto out_unlock; ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx); out_unlock:

• if (ret && mem.mm_node)

• if (ret) ttm_bo_mem_put(bo, &mem); return ret; }

@@ -1144,7 +1150,7 @@ static bool ttm_bo_places_compat(const struct ttm_place *places, for (i = 0; i < num_placement; i++) { const struct ttm_place *heap = &places[i];

• if (mem->mm_node && (mem->start < heap->fpfn ||
  

• if ((mem->start < heap->fpfn ||
  (heap->lpfn != 0 && (mem->start + mem->num_pages) > heap->lpfn)))
  continue;
  
  

diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index aa1f398c2ea7..732167cad130 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -564,8 +564,6 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx);

void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem); -void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,

• 	   struct ttm_mem_reg *mem);
  
  

  int ttm_bo_device_release(struct ttm_bo_device *bdev);

-- 2.17.1

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...