| Message ID | 20200706174811.14755-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/ttm: further cleanup ttm_mem_reg handling | expand |
[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?? 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
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.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cmadhav.chauhan%40amd.com%7C886113da714f433267d308d821d4c4fe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296545014835325&sdata=bemDP4gp7%2BcWeCqxy99r71eUndax2ruO7FeCv30g2PA%3D&reserved=0
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); +} + 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);
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(-)