RE: [PATCH 1/2] drm/ttm: cleanup ttm_mem_type_manager_func.get_node interface v2
>-Original Message- >From: dri-devel On Behalf Of >Christian König >Sent: Monday, June 29, 2020 8:22 AM >To: dri-devel@lists.freedesktop.org >Subject: [PATCH 1/2] drm/ttm: cleanup >ttm_mem_type_manager_func.get_node interface v2 > >Instead of signaling failure by setting the node pointer to >NULL do so by returning -ENOSPC. > >v2: add memset() to make sure that mem is always initialized. > >Signed-off-by: Christian König >--- > drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 +--- > drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 5 ++--- > drivers/gpu/drm/nouveau/nouveau_ttm.c | 8 > drivers/gpu/drm/ttm/ttm_bo.c | 12 ++-- > drivers/gpu/drm/ttm/ttm_bo_manager.c | 2 +- > drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 4 +--- > 6 files changed, 11 insertions(+), 24 deletions(-) > >diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >index 627104401e84..2baa80224fa4 100644 >--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c >@@ -229,7 +229,7 @@ static int amdgpu_gtt_mgr_new(struct >ttm_mem_type_manager *man, > if ((>mem == mem || tbo->mem.mem_type != TTM_PL_TT) && > atomic64_read(>available) < mem->num_pages) { > spin_unlock(>lock); >- return 0; >+ return -ENOSPC; > } > atomic64_sub(mem->num_pages, >available); > spin_unlock(>lock); >@@ -249,8 +249,6 @@ static int amdgpu_gtt_mgr_new(struct >ttm_mem_type_manager *man, > r = amdgpu_gtt_mgr_alloc(man, tbo, place, mem); > if (unlikely(r)) { > kfree(node); >- mem->mm_node = NULL; >- r = 0; I think that this one is still a problem. The code path sets: mem->mm_node = node; (about 3 lines above this chunk, and then calls amdgpu_gtt_mgr_malloc() If amdgpu_gtt_mgr_malloc() fails, mm_node is still set. I think you need to keep this mm_node = NULL setting, or the path needs to be modified to not set mm_node. > goto err_out; > } > } else { >diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c >b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c >index 128a667ed8fa..e8d1dd564006 100644 >--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c >+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c >@@ -336,8 +336,7 @@ static int amdgpu_vram_mgr_new(struct >ttm_mem_type_manager *man, > mem_bytes = (u64)mem->num_pages << PAGE_SHIFT; > if (atomic64_add_return(mem_bytes, >usage) > max_bytes) { > atomic64_sub(mem_bytes, >usage); >- mem->mm_node = NULL; >- return 0; >+ return -ENOSPC; > } > > if (place->flags & TTM_PL_FLAG_CONTIGUOUS) { >@@ -417,7 +416,7 @@ static int amdgpu_vram_mgr_new(struct >ttm_mem_type_manager *man, > atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage); > > kvfree(nodes); >- return r == -ENOSPC ? 0 : r; >+ return r; > } > > /** >diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c >b/drivers/gpu/drm/nouveau/nouveau_ttm.c >index 7ca0a2498532..e89ea052cf71 100644 >--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c >+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c >@@ -75,10 +75,6 @@ nouveau_vram_manager_new(struct >ttm_mem_type_manager *man, > ret = nouveau_mem_vram(reg, nvbo->contig, nvbo->page); > if (ret) { > nouveau_mem_del(reg); >- if (ret == -ENOSPC) { >- reg->mm_node = NULL; >- return 0; >- } > return ret; > } > >@@ -139,10 +135,6 @@ nv04_gart_manager_new(struct >ttm_mem_type_manager *man, > reg->num_pages << PAGE_SHIFT, >vma[0]); > if (ret) { > nouveau_mem_del(reg); >- if (ret == -ENOSPC) { >- reg->mm_node = NULL; >- return 0; >- } > return ret; > } > >diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c >index f78cfc76ad78..06b8bc0d8f23 100644 >--- a/drivers/gpu/drm/ttm/ttm_bo.c >+++ b/drivers/gpu/drm/ttm/ttm_bo.c >@@ -909,10 +909,10 @@ 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); >- if (unlikely(ret != 0)) >-
[PATCH 1/2] drm/ttm: cleanup ttm_mem_type_manager_func.get_node interface v2
Instead of signaling failure by setting the node pointer to NULL do so by returning -ENOSPC. v2: add memset() to make sure that mem is always initialized. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 +--- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 5 ++--- drivers/gpu/drm/nouveau/nouveau_ttm.c | 8 drivers/gpu/drm/ttm/ttm_bo.c | 12 ++-- drivers/gpu/drm/ttm/ttm_bo_manager.c | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 4 +--- 6 files changed, 11 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c index 627104401e84..2baa80224fa4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c @@ -229,7 +229,7 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager *man, if ((>mem == mem || tbo->mem.mem_type != TTM_PL_TT) && atomic64_read(>available) < mem->num_pages) { spin_unlock(>lock); - return 0; + return -ENOSPC; } atomic64_sub(mem->num_pages, >available); spin_unlock(>lock); @@ -249,8 +249,6 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager *man, r = amdgpu_gtt_mgr_alloc(man, tbo, place, mem); if (unlikely(r)) { kfree(node); - mem->mm_node = NULL; - r = 0; goto err_out; } } else { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c index 128a667ed8fa..e8d1dd564006 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c @@ -336,8 +336,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager *man, mem_bytes = (u64)mem->num_pages << PAGE_SHIFT; if (atomic64_add_return(mem_bytes, >usage) > max_bytes) { atomic64_sub(mem_bytes, >usage); - mem->mm_node = NULL; - return 0; + return -ENOSPC; } if (place->flags & TTM_PL_FLAG_CONTIGUOUS) { @@ -417,7 +416,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager *man, atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage); kvfree(nodes); - return r == -ENOSPC ? 0 : r; + return r; } /** diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c index 7ca0a2498532..e89ea052cf71 100644 --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c @@ -75,10 +75,6 @@ nouveau_vram_manager_new(struct ttm_mem_type_manager *man, ret = nouveau_mem_vram(reg, nvbo->contig, nvbo->page); if (ret) { nouveau_mem_del(reg); - if (ret == -ENOSPC) { - reg->mm_node = NULL; - return 0; - } return ret; } @@ -139,10 +135,6 @@ nv04_gart_manager_new(struct ttm_mem_type_manager *man, reg->num_pages << PAGE_SHIFT, >vma[0]); if (ret) { nouveau_mem_del(reg); - if (ret == -ENOSPC) { - reg->mm_node = NULL; - return 0; - } return ret; } diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index f78cfc76ad78..06b8bc0d8f23 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -909,10 +909,10 @@ 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); - if (unlikely(ret != 0)) - return ret; - if (mem->mm_node) + if (likely(!ret)) break; + if (unlikely(ret != -ENOSPC)) + return ret; ret = ttm_mem_evict_first(bdev, mem->mem_type, place, ctx, ticket); if (unlikely(ret != 0)) @@ -1056,12 +1056,11 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, man = >man[mem->mem_type]; ret = (*man->func->get_node)(man, bo, place, mem); + if (ret == -ENOSPC) + continue; if (unlikely(ret)) goto error; - if (!mem->mm_node) - continue; - ret = ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu); if (unlikely(ret)) { (*man->func->put_node)(man, mem); @@ -1121,6 +1120,7 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,