* [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
@ 2021-06-02 10:09 Christian König
2021-06-02 10:09 ` [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes Christian König
` (12 more replies)
0 siblings, 13 replies; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
To improve the handling we want the establish the resource object as base
class for the backend allocations.
v2: add missing error handling
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 54 +++++++-------
drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +-
drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
drivers/gpu/drm/ttm/ttm_bo.c | 83 ++++++++--------------
drivers/gpu/drm/ttm/ttm_bo_util.c | 43 ++++++-----
drivers/gpu/drm/ttm/ttm_resource.c | 31 +++++---
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 2 +-
include/drm/ttm/ttm_bo_api.h | 1 -
include/drm/ttm/ttm_bo_driver.h | 10 ++-
include/drm/ttm/ttm_resource.h | 4 +-
11 files changed, 110 insertions(+), 126 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 03c6b63d1d54..59723c3d5826 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -362,14 +362,14 @@ int amdgpu_bo_create_kernel_at(struct amdgpu_device *adev,
if (cpu_addr)
amdgpu_bo_kunmap(*bo_ptr);
- ttm_resource_free(&(*bo_ptr)->tbo, (*bo_ptr)->tbo.resource);
+ ttm_resource_free(&(*bo_ptr)->tbo, &(*bo_ptr)->tbo.resource);
for (i = 0; i < (*bo_ptr)->placement.num_placement; ++i) {
(*bo_ptr)->placements[i].fpfn = offset >> PAGE_SHIFT;
(*bo_ptr)->placements[i].lpfn = (offset + size) >> PAGE_SHIFT;
}
r = ttm_bo_mem_space(&(*bo_ptr)->tbo, &(*bo_ptr)->placement,
- (*bo_ptr)->tbo.resource, &ctx);
+ &(*bo_ptr)->tbo.resource, &ctx);
if (r)
goto error;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 663aa7d2e2ea..69db89261650 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -491,7 +491,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
return r;
amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
- ttm_resource_free(bo, bo->resource);
+ ttm_resource_free(bo, &bo->resource);
ttm_bo_assign_mem(bo, new_mem);
goto out;
}
@@ -950,9 +950,9 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
struct ttm_operation_ctx ctx = { false, false };
struct amdgpu_ttm_tt *gtt = (void *)bo->ttm;
- struct ttm_resource tmp;
struct ttm_placement placement;
struct ttm_place placements;
+ struct ttm_resource *tmp;
uint64_t addr, flags;
int r;
@@ -962,37 +962,37 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
addr = amdgpu_gmc_agp_addr(bo);
if (addr != AMDGPU_BO_INVALID_OFFSET) {
bo->resource->start = addr >> PAGE_SHIFT;
- } else {
+ return 0;
+ }
- /* allocate GART space */
- placement.num_placement = 1;
- placement.placement = &placements;
- placement.num_busy_placement = 1;
- placement.busy_placement = &placements;
- placements.fpfn = 0;
- placements.lpfn = adev->gmc.gart_size >> PAGE_SHIFT;
- placements.mem_type = TTM_PL_TT;
- placements.flags = bo->resource->placement;
-
- r = ttm_bo_mem_space(bo, &placement, &tmp, &ctx);
- if (unlikely(r))
- return r;
+ /* allocate GART space */
+ placement.num_placement = 1;
+ placement.placement = &placements;
+ placement.num_busy_placement = 1;
+ placement.busy_placement = &placements;
+ placements.fpfn = 0;
+ placements.lpfn = adev->gmc.gart_size >> PAGE_SHIFT;
+ placements.mem_type = TTM_PL_TT;
+ placements.flags = bo->resource->placement;
- /* compute PTE flags for this buffer object */
- flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, &tmp);
+ r = ttm_bo_mem_space(bo, &placement, &tmp, &ctx);
+ if (unlikely(r))
+ return r;
- /* Bind pages */
- gtt->offset = (u64)tmp.start << PAGE_SHIFT;
- r = amdgpu_ttm_gart_bind(adev, bo, flags);
- if (unlikely(r)) {
- ttm_resource_free(bo, &tmp);
- return r;
- }
+ /* compute PTE flags for this buffer object */
+ flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, tmp);
- ttm_resource_free(bo, bo->resource);
- ttm_bo_assign_mem(bo, &tmp);
+ /* Bind pages */
+ gtt->offset = (u64)tmp->start << PAGE_SHIFT;
+ r = amdgpu_ttm_gart_bind(adev, bo, flags);
+ if (unlikely(r)) {
+ ttm_resource_free(bo, &tmp);
+ return r;
}
+ ttm_resource_free(bo, &bo->resource);
+ ttm_bo_assign_mem(bo, tmp);
+
return 0;
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index e688ca77483d..3a0d9b3bf991 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1009,7 +1009,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
if (old_reg->mem_type == TTM_PL_TT &&
new_reg->mem_type == TTM_PL_SYSTEM) {
nouveau_ttm_tt_unbind(bo->bdev, bo->ttm);
- ttm_resource_free(bo, bo->resource);
+ ttm_resource_free(bo, &bo->resource);
ttm_bo_assign_mem(bo, new_reg);
goto out;
}
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 2507c1741681..cdffa9b65108 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -229,7 +229,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
if (old_mem->mem_type == TTM_PL_TT &&
new_mem->mem_type == TTM_PL_SYSTEM) {
radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
- ttm_resource_free(bo, bo->resource);
+ ttm_resource_free(bo, &bo->resource);
ttm_bo_assign_mem(bo, new_mem);
goto out;
}
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 5a7ab4b35b2d..4ed56520b81d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -223,7 +223,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
bo->bdev->funcs->delete_mem_notify(bo);
ttm_bo_tt_destroy(bo);
- ttm_resource_free(bo, bo->resource);
+ ttm_resource_free(bo, &bo->resource);
}
static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
@@ -489,7 +489,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
struct ttm_operation_ctx *ctx)
{
struct ttm_device *bdev = bo->bdev;
- struct ttm_resource evict_mem;
+ struct ttm_resource *evict_mem;
struct ttm_placement placement;
struct ttm_place hop;
int ret = 0;
@@ -519,7 +519,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
goto out;
}
- ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, &hop);
+ 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)
@@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
*/
static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *mem,
+ struct ttm_resource **mem,
struct ttm_operation_ctx *ctx)
{
struct ttm_device *bdev = bo->bdev;
- struct ttm_resource_manager *man = ttm_manager_type(bdev, mem->mem_type);
+ struct ttm_resource_manager *man;
struct ww_acquire_ctx *ticket;
int ret;
+ man = ttm_manager_type(bdev, (*mem)->mem_type);
ticket = dma_resv_locking_ctx(bo->base.resv);
do {
ret = ttm_resource_alloc(bo, place, mem);
@@ -749,37 +750,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
return ret;
} while (1);
- return ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu);
-}
-
-/**
- * ttm_bo_mem_placement - check if placement is compatible
- * @bo: BO to find memory for
- * @place: where to search
- * @mem: the memory object to fill in
- *
- * Check if placement is compatible and fill in mem structure.
- * Returns -EBUSY if placement won't work or negative error code.
- * 0 when placement can be used.
- */
-static int ttm_bo_mem_placement(struct ttm_buffer_object *bo,
- const struct ttm_place *place,
- struct ttm_resource *mem)
-{
- struct ttm_device *bdev = bo->bdev;
- struct ttm_resource_manager *man;
-
- man = ttm_manager_type(bdev, place->mem_type);
- if (!man || !ttm_resource_manager_used(man))
- return -EBUSY;
-
- mem->mem_type = place->mem_type;
- mem->placement = place->flags;
-
- spin_lock(&bo->bdev->lru_lock);
- ttm_bo_move_to_lru_tail(bo, mem, NULL);
- spin_unlock(&bo->bdev->lru_lock);
- return 0;
+ return ttm_bo_add_move_fence(bo, man, *mem, ctx->no_wait_gpu);
}
/*
@@ -792,7 +763,7 @@ static int ttm_bo_mem_placement(struct ttm_buffer_object *bo,
*/
int ttm_bo_mem_space(struct ttm_buffer_object *bo,
struct ttm_placement *placement,
- struct ttm_resource *mem,
+ struct ttm_resource **mem,
struct ttm_operation_ctx *ctx)
{
struct ttm_device *bdev = bo->bdev;
@@ -807,8 +778,8 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
const struct ttm_place *place = &placement->placement[i];
struct ttm_resource_manager *man;
- ret = ttm_bo_mem_placement(bo, place, mem);
- if (ret)
+ man = ttm_manager_type(bdev, place->mem_type);
+ if (!man || !ttm_resource_manager_used(man))
continue;
type_found = true;
@@ -818,8 +789,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
if (unlikely(ret))
goto error;
- man = ttm_manager_type(bdev, mem->mem_type);
- ret = ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu);
+ ret = ttm_bo_add_move_fence(bo, man, *mem, ctx->no_wait_gpu);
if (unlikely(ret)) {
ttm_resource_free(bo, mem);
if (ret == -EBUSY)
@@ -832,9 +802,10 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
for (i = 0; i < placement->num_busy_placement; ++i) {
const struct ttm_place *place = &placement->busy_placement[i];
+ struct ttm_resource_manager *man;
- ret = ttm_bo_mem_placement(bo, place, mem);
- if (ret)
+ man = ttm_manager_type(bdev, place->mem_type);
+ if (!man || !ttm_resource_manager_used(man))
continue;
type_found = true;
@@ -861,12 +832,12 @@ 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_resource **mem,
struct ttm_operation_ctx *ctx,
struct ttm_place *hop)
{
struct ttm_placement hop_placement;
- struct ttm_resource hop_mem;
+ struct ttm_resource *hop_mem;
int ret;
hop_placement.num_placement = hop_placement.num_busy_placement = 1;
@@ -877,7 +848,7 @@ static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo,
if (ret)
return ret;
/* move to the bounce domain */
- ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL);
+ ret = ttm_bo_handle_move_mem(bo, hop_mem, false, ctx, NULL);
if (ret) {
ttm_resource_free(bo, &hop_mem);
return ret;
@@ -889,14 +860,12 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
struct ttm_placement *placement,
struct ttm_operation_ctx *ctx)
{
+ struct ttm_resource *mem;
struct ttm_place hop;
- struct ttm_resource mem;
int ret;
dma_resv_assert_held(bo->base.resv);
- memset(&hop, 0, sizeof(hop));
-
/*
* Determine where to move the buffer.
*
@@ -910,7 +879,7 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
if (ret)
return ret;
bounce:
- ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop);
+ 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)
@@ -1019,7 +988,7 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
{
static const struct ttm_place sys_mem = { .mem_type = TTM_PL_SYSTEM };
bool locked;
- int ret = 0;
+ int ret;
bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
@@ -1029,8 +998,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
bo->bdev = bdev;
bo->type = type;
bo->page_alignment = page_alignment;
- bo->resource = &bo->_mem;
- ttm_resource_alloc(bo, &sys_mem, bo->resource);
bo->moving = NULL;
bo->pin_count = 0;
bo->sg = sg;
@@ -1042,6 +1009,12 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
}
atomic_inc(&ttm_glob.bo_count);
+ ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
+ if (unlikely(ret)) {
+ ttm_bo_put(bo);
+ return ret;
+ }
+
/*
* For ttm_bo_type_device buffers, allocate
* address space from the device.
@@ -1170,7 +1143,7 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
*/
if (bo->resource->mem_type != TTM_PL_SYSTEM) {
struct ttm_operation_ctx ctx = { false, false };
- struct ttm_resource evict_mem;
+ struct ttm_resource *evict_mem;
struct ttm_place place, hop;
memset(&place, 0, sizeof(place));
@@ -1182,7 +1155,7 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
if (unlikely(ret))
goto out;
- ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, &hop);
+ 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/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index aedf02a31c70..1b326e70cb02 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -176,16 +176,17 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
struct ttm_operation_ctx *ctx,
struct ttm_resource *new_mem)
{
+ struct ttm_resource *old_mem = bo->resource;
struct ttm_device *bdev = bo->bdev;
- struct ttm_resource_manager *man = ttm_manager_type(bdev, new_mem->mem_type);
+ struct ttm_resource_manager *man;
struct ttm_tt *ttm = bo->ttm;
- struct ttm_resource *old_mem = bo->resource;
- struct ttm_resource old_copy = *old_mem;
void *old_iomap;
void *new_iomap;
int ret;
unsigned long i;
+ man = ttm_manager_type(bdev, new_mem->mem_type);
+
ret = ttm_bo_wait_ctx(bo, ctx);
if (ret)
return ret;
@@ -201,7 +202,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
* Single TTM move. NOP.
*/
if (old_iomap == NULL && new_iomap == NULL)
- goto out2;
+ goto out1;
/*
* Don't move nonexistent data. Clear destination instead.
@@ -210,7 +211,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
(ttm == NULL || (!ttm_tt_is_populated(ttm) &&
!(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)))) {
memset_io(new_iomap, 0, new_mem->num_pages*PAGE_SIZE);
- goto out2;
+ goto out1;
}
/*
@@ -235,27 +236,25 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
ret = ttm_copy_io_page(new_iomap, old_iomap, i);
}
if (ret)
- goto out1;
+ break;
}
mb();
-out2:
- old_copy = *old_mem;
+out1:
+ ttm_resource_iounmap(bdev, new_mem, new_iomap);
+out:
+ ttm_resource_iounmap(bdev, old_mem, old_iomap);
+
+ if (ret) {
+ ttm_resource_free(bo, &new_mem);
+ return ret;
+ }
+ ttm_resource_free(bo, &bo->resource);
ttm_bo_assign_mem(bo, new_mem);
if (!man->use_tt)
ttm_bo_tt_destroy(bo);
-out1:
- ttm_resource_iounmap(bdev, old_mem, new_iomap);
-out:
- ttm_resource_iounmap(bdev, &old_copy, old_iomap);
-
- /*
- * On error, keep the mm node!
- */
- if (!ret)
- ttm_resource_free(bo, &old_copy);
return ret;
}
EXPORT_SYMBOL(ttm_bo_move_memcpy);
@@ -566,7 +565,7 @@ static int ttm_bo_wait_free_node(struct ttm_buffer_object *bo,
if (!dst_use_tt)
ttm_bo_tt_destroy(bo);
- ttm_resource_free(bo, bo->resource);
+ ttm_resource_free(bo, &bo->resource);
return 0;
}
@@ -629,7 +628,7 @@ static void ttm_bo_move_pipeline_evict(struct ttm_buffer_object *bo,
}
spin_unlock(&from->move_lock);
- ttm_resource_free(bo, bo->resource);
+ ttm_resource_free(bo, &bo->resource);
dma_fence_put(bo->moving);
bo->moving = dma_fence_get(fence);
@@ -678,11 +677,11 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
if (ret)
ttm_bo_wait(bo, false, false);
- ttm_resource_alloc(bo, &sys_mem, bo->resource);
+ ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
bo->ttm = NULL;
dma_resv_unlock(&ghost->base._resv);
ttm_bo_put(ghost);
- return 0;
+ return ret;
}
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 59e2b7157e41..65451e1bc303 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -27,10 +27,16 @@
int ttm_resource_alloc(struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *res)
+ struct ttm_resource **res_ptr)
{
struct ttm_resource_manager *man =
ttm_manager_type(bo->bdev, place->mem_type);
+ struct ttm_resource *res;
+ int r;
+
+ res = kmalloc(sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
res->mm_node = NULL;
res->start = 0;
@@ -41,18 +47,27 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo,
res->bus.offset = 0;
res->bus.is_iomem = false;
res->bus.caching = ttm_cached;
+ r = man->func->alloc(man, bo, place, res);
+ if (r) {
+ kfree(res);
+ return r;
+ }
- return man->func->alloc(man, bo, place, res);
+ *res_ptr = res;
+ return 0;
}
-void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource *res)
+void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
{
- struct ttm_resource_manager *man =
- ttm_manager_type(bo->bdev, res->mem_type);
+ struct ttm_resource_manager *man;
- man->func->free(man, res);
- res->mm_node = NULL;
- res->mem_type = TTM_PL_SYSTEM;
+ if (!*res)
+ return;
+
+ man = ttm_manager_type(bo->bdev, (*res)->mem_type);
+ man->func->free(man, *res);
+ kfree(*res);
+ *res = NULL;
}
EXPORT_SYMBOL(ttm_resource_free);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index ed8563ef9a3b..bfcf31bf7e37 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -741,7 +741,7 @@ static int vmw_move(struct ttm_buffer_object *bo,
goto fail;
vmw_ttm_unbind(bo->bdev, bo->ttm);
- ttm_resource_free(bo, bo->resource);
+ ttm_resource_free(bo, &bo->resource);
ttm_bo_assign_mem(bo, new_mem);
return 0;
} else {
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 291a339a7e08..f681bbdbc698 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -137,7 +137,6 @@ struct ttm_buffer_object {
*/
struct ttm_resource *resource;
- struct ttm_resource _mem;
struct ttm_tt *ttm;
bool deleted;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 1a9ba0b13622..ead0ef7136c8 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -96,7 +96,7 @@ struct ttm_lru_bulk_move {
*/
int ttm_bo_mem_space(struct ttm_buffer_object *bo,
struct ttm_placement *placement,
- struct ttm_resource *mem,
+ struct ttm_resource **mem,
struct ttm_operation_ctx *ctx);
/**
@@ -188,8 +188,8 @@ ttm_bo_move_to_lru_tail_unlocked(struct ttm_buffer_object *bo)
static inline void ttm_bo_assign_mem(struct ttm_buffer_object *bo,
struct ttm_resource *new_mem)
{
- bo->_mem = *new_mem;
- new_mem->mm_node = NULL;
+ WARN_ON(bo->resource);
+ bo->resource = new_mem;
}
/**
@@ -202,9 +202,7 @@ static inline void ttm_bo_assign_mem(struct ttm_buffer_object *bo,
static inline void ttm_bo_move_null(struct ttm_buffer_object *bo,
struct ttm_resource *new_mem)
{
- struct ttm_resource *old_mem = bo->resource;
-
- WARN_ON(old_mem->mm_node != NULL);
+ ttm_resource_free(bo, &bo->resource);
ttm_bo_assign_mem(bo, new_mem);
}
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index 890b9d369519..c17c1a52070d 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -225,8 +225,8 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man)
int ttm_resource_alloc(struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *res);
-void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource *res);
+ struct ttm_resource **res);
+void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);
void ttm_resource_manager_init(struct ttm_resource_manager *man,
unsigned long p_size);
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-02 11:44 ` Thomas Hellström (Intel)
2021-06-02 10:09 ` [PATCH 03/10] drm/ttm: flip over the sys " Christian König
` (11 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Start with the range manager to make the resource object the base
class for the allocated nodes.
While at it cleanup a lot of the code around that.
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
drivers/gpu/drm/ttm/ttm_range_manager.c | 56 ++++++++++++++++++-------
drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
include/drm/ttm/ttm_bo_driver.h | 26 ------------
include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
include/drm/ttm/ttm_resource.h | 3 ++
10 files changed, 111 insertions(+), 50 deletions(-)
create mode 100644 include/drm/ttm/ttm_range_manager.h
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 69db89261650..df1f185faae9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -45,6 +45,7 @@
#include <drm/ttm/ttm_bo_api.h>
#include <drm/ttm/ttm_bo_driver.h>
#include <drm/ttm/ttm_placement.h>
+#include <drm/ttm/ttm_range_manager.h>
#include <drm/amdgpu_drm.h>
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 83e7258c7f90..17a4c5d47b6a 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -17,6 +17,8 @@
#include <drm/drm_prime.h>
#include <drm/drm_simple_kms_helper.h>
+#include <drm/ttm/ttm_range_manager.h>
+
static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
/**
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index 65430912ff72..b08b8efeefba 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -26,6 +26,8 @@
#include <linux/limits.h>
#include <linux/swiotlb.h>
+#include <drm/ttm/ttm_range_manager.h>
+
#include "nouveau_drv.h"
#include "nouveau_gem.h"
#include "nouveau_mem.h"
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 8aa87b8edb9c..19fd39d9a00c 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -32,6 +32,7 @@
#include <drm/ttm/ttm_bo_api.h>
#include <drm/ttm/ttm_bo_driver.h>
#include <drm/ttm/ttm_placement.h>
+#include <drm/ttm/ttm_range_manager.h>
#include "qxl_drv.h"
#include "qxl_object.h"
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index cdffa9b65108..ad2a5a791bba 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -45,6 +45,7 @@
#include <drm/ttm/ttm_bo_api.h>
#include <drm/ttm/ttm_bo_driver.h>
#include <drm/ttm/ttm_placement.h>
+#include <drm/ttm/ttm_range_manager.h>
#include "radeon_reg.h"
#include "radeon.h"
diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
index b9d5da6e6a81..ce5d07ca384c 100644
--- a/drivers/gpu/drm/ttm/ttm_range_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
@@ -29,12 +29,13 @@
* Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
*/
-#include <drm/ttm/ttm_bo_driver.h>
+#include <drm/ttm/ttm_device.h>
#include <drm/ttm/ttm_placement.h>
+#include <drm/ttm/ttm_range_manager.h>
+#include <drm/ttm/ttm_bo_api.h>
#include <drm/drm_mm.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
-#include <linux/module.h>
/*
* Currently we use a spinlock for the lock, but a mutex *may* be
@@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man,
struct ttm_resource *mem)
{
struct ttm_range_manager *rman = to_range_manager(man);
+ struct ttm_range_mgr_node *node;
struct drm_mm *mm = &rman->mm;
- struct drm_mm_node *node;
enum drm_mm_insert_mode mode;
unsigned long lpfn;
int ret;
@@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man,
if (!lpfn)
lpfn = man->size;
- node = kzalloc(sizeof(*node), GFP_KERNEL);
+ node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
if (!node)
return -ENOMEM;
@@ -78,17 +79,19 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man,
if (place->flags & TTM_PL_FLAG_TOPDOWN)
mode = DRM_MM_INSERT_HIGH;
+ ttm_resource_init(bo, place, &node->base);
+
spin_lock(&rman->lock);
- ret = drm_mm_insert_node_in_range(mm, node, mem->num_pages,
- bo->page_alignment, 0,
+ ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0],
+ mem->num_pages, bo->page_alignment, 0,
place->fpfn, lpfn, mode);
spin_unlock(&rman->lock);
if (unlikely(ret)) {
kfree(node);
} else {
- mem->mm_node = node;
- mem->start = node->start;
+ mem->mm_node = &node->mm_nodes[0];
+ mem->start = node->mm_nodes[0].start;
}
return ret;
@@ -98,15 +101,19 @@ static void ttm_range_man_free(struct ttm_resource_manager *man,
struct ttm_resource *mem)
{
struct ttm_range_manager *rman = to_range_manager(man);
+ struct ttm_range_mgr_node *node;
- if (mem->mm_node) {
- spin_lock(&rman->lock);
- drm_mm_remove_node(mem->mm_node);
- spin_unlock(&rman->lock);
+ if (!mem->mm_node)
+ return;
- kfree(mem->mm_node);
- mem->mm_node = NULL;
- }
+ node = to_ttm_range_mgr_node(mem);
+
+ spin_lock(&rman->lock);
+ drm_mm_remove_node(&node->mm_nodes[0]);
+ spin_unlock(&rman->lock);
+
+ kfree(node);
+ mem->mm_node = NULL;
}
static void ttm_range_man_debug(struct ttm_resource_manager *man,
@@ -125,6 +132,17 @@ static const struct ttm_resource_manager_func ttm_range_manager_func = {
.debug = ttm_range_man_debug
};
+/**
+ * ttm_range_man_init
+ *
+ * @bdev: ttm device
+ * @type: memory manager type
+ * @use_tt: if the memory manager uses tt
+ * @p_size: size of area to be managed in pages.
+ *
+ * Initialise a generic range manager for the selected memory type.
+ * The range manager is installed for this device in the type slot.
+ */
int ttm_range_man_init(struct ttm_device *bdev,
unsigned type, bool use_tt,
unsigned long p_size)
@@ -152,6 +170,14 @@ int ttm_range_man_init(struct ttm_device *bdev,
}
EXPORT_SYMBOL(ttm_range_man_init);
+/**
+ * ttm_range_man_fini
+ *
+ * @bdev: ttm device
+ * @type: memory manager type
+ *
+ * Remove the generic range manager from a slot and tear it down.
+ */
int ttm_range_man_fini(struct ttm_device *bdev,
unsigned type)
{
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 65451e1bc303..2a51ace17614 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -25,6 +25,22 @@
#include <drm/ttm/ttm_resource.h>
#include <drm/ttm/ttm_bo_driver.h>
+void ttm_resource_init(struct ttm_buffer_object *bo,
+ const struct ttm_place *place,
+ struct ttm_resource *res)
+{
+ res->mm_node = NULL;
+ res->start = 0;
+ res->num_pages = PFN_UP(bo->base.size);
+ res->mem_type = place->mem_type;
+ res->placement = place->flags;
+ res->bus.addr = NULL;
+ res->bus.offset = 0;
+ res->bus.is_iomem = false;
+ res->bus.caching = ttm_cached;
+}
+EXPORT_SYMBOL(ttm_resource_init);
+
int ttm_resource_alloc(struct ttm_buffer_object *bo,
const struct ttm_place *place,
struct ttm_resource **res_ptr)
@@ -38,15 +54,7 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo,
if (!res)
return -ENOMEM;
- res->mm_node = NULL;
- res->start = 0;
- res->num_pages = PFN_UP(bo->base.size);
- res->mem_type = place->mem_type;
- res->placement = place->flags;
- res->bus.addr = NULL;
- res->bus.offset = 0;
- res->bus.is_iomem = false;
- res->bus.caching = ttm_cached;
+ ttm_resource_init(bo, place, res);
r = man->func->alloc(man, bo, place, res);
if (r) {
kfree(res);
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index ead0ef7136c8..b266971c1974 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -304,30 +304,4 @@ int ttm_bo_tt_bind(struct ttm_buffer_object *bo, struct ttm_resource *mem);
*/
void ttm_bo_tt_destroy(struct ttm_buffer_object *bo);
-/**
- * ttm_range_man_init
- *
- * @bdev: ttm device
- * @type: memory manager type
- * @use_tt: if the memory manager uses tt
- * @p_size: size of area to be managed in pages.
- *
- * Initialise a generic range manager for the selected memory type.
- * The range manager is installed for this device in the type slot.
- */
-int ttm_range_man_init(struct ttm_device *bdev,
- unsigned type, bool use_tt,
- unsigned long p_size);
-
-/**
- * ttm_range_man_fini
- *
- * @bdev: ttm device
- * @type: memory manager type
- *
- * Remove the generic range manager from a slot and tear it down.
- */
-int ttm_range_man_fini(struct ttm_device *bdev,
- unsigned type);
-
#endif
diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h
new file mode 100644
index 000000000000..983f452ce54b
--- /dev/null
+++ b/include/drm/ttm/ttm_range_manager.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+
+#ifndef _TTM_RANGE_MANAGER_H_
+#define _TTM_RANGE_MANAGER_H_
+
+#include <drm/ttm/ttm_resource.h>
+#include <drm/drm_mm.h>
+
+/**
+ * struct ttm_range_mgr_node
+ *
+ * @base: base clase we extend
+ * @mm_nodes: MM nodes, usually 1
+ *
+ * Extending the ttm_resource object to manage an address space allocation with
+ * one or more drm_mm_nodes.
+ */
+struct ttm_range_mgr_node {
+ struct ttm_resource base;
+ struct drm_mm_node mm_nodes[];
+};
+
+/**
+ * to_ttm_range_mgr_node
+ *
+ * @res: the resource to upcast
+ *
+ * Upcast the ttm_resource object into a ttm_range_mgr_node object.
+ */
+static inline struct ttm_range_mgr_node *
+to_ttm_range_mgr_node(struct ttm_resource *res)
+{
+ return container_of(res->mm_node, struct ttm_range_mgr_node,
+ mm_nodes[0]);
+}
+
+int ttm_range_man_init(struct ttm_device *bdev,
+ unsigned type, bool use_tt,
+ unsigned long p_size);
+int ttm_range_man_fini(struct ttm_device *bdev,
+ unsigned type);
+
+#endif
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index c17c1a52070d..803e4875d779 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -223,6 +223,9 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man)
man->move = NULL;
}
+void ttm_resource_init(struct ttm_buffer_object *bo,
+ const struct ttm_place *place,
+ struct ttm_resource *res);
int ttm_resource_alloc(struct ttm_buffer_object *bo,
const struct ttm_place *place,
struct ttm_resource **res);
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 03/10] drm/ttm: flip over the sys manager to self allocated nodes
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
2021-06-02 10:09 ` [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-03 7:51 ` Matthew Auld
2021-06-02 10:09 ` [PATCH 04/10] drm/amdgpu: revert "drm/amdgpu: stop allocating dummy GTT nodes" Christian König
` (10 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Make sure to allocate a resource object here.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/ttm/ttm_sys_manager.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/ttm/ttm_sys_manager.c b/drivers/gpu/drm/ttm/ttm_sys_manager.c
index 474221e863d0..2b75f493c3c9 100644
--- a/drivers/gpu/drm/ttm/ttm_sys_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_sys_manager.c
@@ -3,6 +3,7 @@
#include <drm/ttm/ttm_resource.h>
#include <drm/ttm/ttm_device.h>
#include <drm/ttm/ttm_placement.h>
+#include <linux/slab.h>
#include "ttm_module.h"
@@ -11,12 +12,18 @@ static int ttm_sys_man_alloc(struct ttm_resource_manager *man,
const struct ttm_place *place,
struct ttm_resource *mem)
{
+ mem->mm_node = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem->mm_node)
+ return -ENOMEM;
+
+ ttm_resource_init(bo, place, mem->mm_node);
return 0;
}
static void ttm_sys_man_free(struct ttm_resource_manager *man,
struct ttm_resource *mem)
{
+ kfree(mem->mm_node);
}
static const struct ttm_resource_manager_func ttm_sys_manager_func = {
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 04/10] drm/amdgpu: revert "drm/amdgpu: stop allocating dummy GTT nodes"
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
2021-06-02 10:09 ` [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes Christian König
2021-06-02 10:09 ` [PATCH 03/10] drm/ttm: flip over the sys " Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-02 10:09 ` [PATCH 05/10] drm/amdkfd: use resource cursor in svm_migrate_copy_to_vram v2 Christian König
` (9 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
TTM is going to need this again since we are moving the resource
allocation into the backend.
Signed-off-by: Christian König <christian.koenig@amd.com>
Acked-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 68 ++++++++++++---------
1 file changed, 39 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index fac563fb6f0c..cd5c55cb38d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -24,16 +24,22 @@
#include "amdgpu.h"
+struct amdgpu_gtt_node {
+ struct drm_mm_node node;
+ struct ttm_buffer_object *tbo;
+};
+
static inline struct amdgpu_gtt_mgr *
to_gtt_mgr(struct ttm_resource_manager *man)
{
return container_of(man, struct amdgpu_gtt_mgr, manager);
}
-struct amdgpu_gtt_node {
- struct drm_mm_node node;
- struct ttm_buffer_object *tbo;
-};
+static inline struct amdgpu_gtt_node *
+to_amdgpu_gtt_node(struct ttm_resource *res)
+{
+ return container_of(res->mm_node, struct amdgpu_gtt_node, node);
+}
/**
* DOC: mem_info_gtt_total
@@ -99,7 +105,9 @@ const struct attribute_group amdgpu_gtt_mgr_attr_group = {
*/
bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_resource *mem)
{
- return mem->mm_node != NULL;
+ struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(mem);
+
+ return drm_mm_node_allocated(&node->node);
}
/**
@@ -130,12 +138,6 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_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;
@@ -143,19 +145,25 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
}
node->tbo = tbo;
+ if (place->lpfn) {
+ spin_lock(&mgr->lock);
+ r = drm_mm_insert_node_in_range(&mgr->mm, &node->node,
+ mem->num_pages,
+ tbo->page_alignment, 0,
+ place->fpfn, place->lpfn,
+ DRM_MM_INSERT_BEST);
+ spin_unlock(&mgr->lock);
+ if (unlikely(r))
+ goto err_free;
- spin_lock(&mgr->lock);
- r = drm_mm_insert_node_in_range(&mgr->mm, &node->node, mem->num_pages,
- tbo->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;
+ mem->start = node->node.start;
+ } else {
+ node->node.start = 0;
+ node->node.size = mem->num_pages;
+ mem->start = AMDGPU_BO_INVALID_OFFSET;
+ }
+ mem->mm_node = &node->node;
return 0;
err_free:
@@ -178,17 +186,19 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
static void amdgpu_gtt_mgr_del(struct ttm_resource_manager *man,
struct ttm_resource *mem)
{
+ struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(mem);
struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
- struct amdgpu_gtt_node *node = mem->mm_node;
- if (node) {
- spin_lock(&mgr->lock);
- drm_mm_remove_node(&node->node);
- spin_unlock(&mgr->lock);
- kfree(node);
- }
+ if (!node)
+ return;
+ spin_lock(&mgr->lock);
+ if (drm_mm_node_allocated(&node->node))
+ drm_mm_remove_node(&node->node);
+ spin_unlock(&mgr->lock);
atomic64_add(mem->num_pages, &mgr->available);
+
+ kfree(node);
}
/**
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 05/10] drm/amdkfd: use resource cursor in svm_migrate_copy_to_vram v2
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (2 preceding siblings ...)
2021-06-02 10:09 ` [PATCH 04/10] drm/amdgpu: revert "drm/amdgpu: stop allocating dummy GTT nodes" Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-03 9:44 ` Matthew Auld
2021-06-02 10:09 ` [PATCH 06/10] drm/amdgpu: switch the GTT backend to self alloc Christian König
` (8 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Access to the mm_node is now forbidden. So instead of hand wiring that
use the cursor functionality.
v2: fix handling as pointed out by Philip.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 68 ++++--------------------
1 file changed, 10 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index fd8f544f0de2..5ce8fa2ddab0 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -29,6 +29,7 @@
#include "amdgpu_object.h"
#include "amdgpu_vm.h"
#include "amdgpu_mn.h"
+#include "amdgpu_res_cursor.h"
#include "kfd_priv.h"
#include "kfd_svm.h"
#include "kfd_migrate.h"
@@ -205,34 +206,6 @@ svm_migrate_copy_done(struct amdgpu_device *adev, struct dma_fence *mfence)
return r;
}
-static uint64_t
-svm_migrate_node_physical_addr(struct amdgpu_device *adev,
- struct drm_mm_node **mm_node, uint64_t *offset)
-{
- struct drm_mm_node *node = *mm_node;
- uint64_t pos = *offset;
-
- if (node->start == AMDGPU_BO_INVALID_OFFSET) {
- pr_debug("drm node is not validated\n");
- return 0;
- }
-
- pr_debug("vram node start 0x%llx npages 0x%llx\n", node->start,
- node->size);
-
- if (pos >= node->size) {
- do {
- pos -= node->size;
- node++;
- } while (pos >= node->size);
-
- *mm_node = node;
- *offset = pos;
- }
-
- return (node->start + pos) << PAGE_SHIFT;
-}
-
unsigned long
svm_migrate_addr_to_pfn(struct amdgpu_device *adev, unsigned long addr)
{
@@ -297,11 +270,9 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
{
uint64_t npages = migrate->cpages;
struct device *dev = adev->dev;
- struct drm_mm_node *node;
+ struct amdgpu_res_cursor cursor;
dma_addr_t *src;
uint64_t *dst;
- uint64_t vram_addr;
- uint64_t offset;
uint64_t i, j;
int r;
@@ -317,19 +288,12 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
goto out;
}
- node = prange->ttm_res->mm_node;
- offset = prange->offset;
- vram_addr = svm_migrate_node_physical_addr(adev, &node, &offset);
- if (!vram_addr) {
- WARN_ONCE(1, "vram node address is 0\n");
- r = -ENOMEM;
- goto out;
- }
-
+ amdgpu_res_first(prange->ttm_res, prange->offset << PAGE_SHIFT,
+ npages << PAGE_SHIFT, &cursor);
for (i = j = 0; i < npages; i++) {
struct page *spage;
- dst[i] = vram_addr + (j << PAGE_SHIFT);
+ dst[i] = cursor.start + (j << PAGE_SHIFT);
migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]);
svm_migrate_get_vram_page(prange, migrate->dst[i]);
@@ -354,18 +318,10 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
mfence);
if (r)
goto out_free_vram_pages;
- offset += j;
- vram_addr = (node->start + offset) << PAGE_SHIFT;
+ amdgpu_res_next(&cursor, j << PAGE_SHIFT);
j = 0;
} else {
- offset++;
- vram_addr += PAGE_SIZE;
- }
- if (offset >= node->size) {
- node++;
- pr_debug("next node size 0x%llx\n", node->size);
- vram_addr = node->start << PAGE_SHIFT;
- offset = 0;
+ amdgpu_res_next(&cursor, PAGE_SIZE);
}
continue;
}
@@ -373,19 +329,15 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange,
pr_debug("dma mapping src to 0x%llx, page_to_pfn 0x%lx\n",
src[i] >> PAGE_SHIFT, page_to_pfn(spage));
- if (j + offset >= node->size - 1 && i < npages - 1) {
+ if (j << PAGE_SHIFT >= cursor.size - 1 && i < npages - 1) {
r = svm_migrate_copy_memory_gart(adev, src + i - j,
dst + i - j, j + 1,
FROM_RAM_TO_VRAM,
mfence);
if (r)
goto out_free_vram_pages;
-
- node++;
- pr_debug("next node size 0x%llx\n", node->size);
- vram_addr = node->start << PAGE_SHIFT;
- offset = 0;
- j = 0;
+ amdgpu_res_next(&cursor, (j + 1) * PAGE_SIZE);
+ j= 0;
} else {
j++;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 06/10] drm/amdgpu: switch the GTT backend to self alloc
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (3 preceding siblings ...)
2021-06-02 10:09 ` [PATCH 05/10] drm/amdkfd: use resource cursor in svm_migrate_copy_to_vram v2 Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-02 10:09 ` [PATCH 07/10] drm/amdgpu: switch the VRAM " Christian König
` (7 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Similar to the TTM range manager.
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 36 +++++++++++++--------
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index cd5c55cb38d1..29113f72bc39 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -22,11 +22,13 @@
* Authors: Christian König
*/
+#include <drm/ttm/ttm_range_manager.h>
+
#include "amdgpu.h"
struct amdgpu_gtt_node {
- struct drm_mm_node node;
struct ttm_buffer_object *tbo;
+ struct ttm_range_mgr_node base;
};
static inline struct amdgpu_gtt_mgr *
@@ -38,7 +40,8 @@ to_gtt_mgr(struct ttm_resource_manager *man)
static inline struct amdgpu_gtt_node *
to_amdgpu_gtt_node(struct ttm_resource *res)
{
- return container_of(res->mm_node, struct amdgpu_gtt_node, node);
+ return container_of(res->mm_node, struct amdgpu_gtt_node,
+ base.mm_nodes[0]);
}
/**
@@ -107,7 +110,7 @@ bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_resource *mem)
{
struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(mem);
- return drm_mm_node_allocated(&node->node);
+ return drm_mm_node_allocated(&node->base.mm_nodes[0]);
}
/**
@@ -138,16 +141,19 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
atomic64_sub(mem->num_pages, &mgr->available);
spin_unlock(&mgr->lock);
- node = kzalloc(sizeof(*node), GFP_KERNEL);
+ node = kzalloc(struct_size(node, base.mm_nodes, 1), GFP_KERNEL);
if (!node) {
r = -ENOMEM;
goto err_out;
}
node->tbo = tbo;
+ ttm_resource_init(tbo, place, &node->base.base);
+
if (place->lpfn) {
spin_lock(&mgr->lock);
- r = drm_mm_insert_node_in_range(&mgr->mm, &node->node,
+ r = drm_mm_insert_node_in_range(&mgr->mm,
+ &node->base.mm_nodes[0],
mem->num_pages,
tbo->page_alignment, 0,
place->fpfn, place->lpfn,
@@ -156,14 +162,14 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
if (unlikely(r))
goto err_free;
- mem->start = node->node.start;
+ mem->start = node->base.mm_nodes[0].start;
} else {
- node->node.start = 0;
- node->node.size = mem->num_pages;
+ node->base.mm_nodes[0].start = 0;
+ node->base.mm_nodes[0].size = mem->num_pages;
mem->start = AMDGPU_BO_INVALID_OFFSET;
}
- mem->mm_node = &node->node;
+ mem->mm_node = &node->base.mm_nodes[0];
return 0;
err_free:
@@ -186,15 +192,17 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
static void amdgpu_gtt_mgr_del(struct ttm_resource_manager *man,
struct ttm_resource *mem)
{
- struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(mem);
struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
+ struct amdgpu_gtt_node *node;
- if (!node)
+ if (!mem->mm_node)
return;
+ node = to_amdgpu_gtt_node(mem);
+
spin_lock(&mgr->lock);
- if (drm_mm_node_allocated(&node->node))
- drm_mm_remove_node(&node->node);
+ if (drm_mm_node_allocated(&node->base.mm_nodes[0]))
+ drm_mm_remove_node(&node->base.mm_nodes[0]);
spin_unlock(&mgr->lock);
atomic64_add(mem->num_pages, &mgr->available);
@@ -232,7 +240,7 @@ int amdgpu_gtt_mgr_recover(struct ttm_resource_manager *man)
spin_lock(&mgr->lock);
drm_mm_for_each_node(mm_node, &mgr->mm) {
- node = container_of(mm_node, struct amdgpu_gtt_node, node);
+ node = container_of(mm_node, typeof(*node), base.mm_nodes[0]);
r = amdgpu_ttm_recover_gart(node->tbo);
if (r)
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 07/10] drm/amdgpu: switch the VRAM backend to self alloc
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (4 preceding siblings ...)
2021-06-02 10:09 ` [PATCH 06/10] drm/amdgpu: switch the GTT backend to self alloc Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-02 10:09 ` [PATCH 08/10] drm/nouveau: switch the TTM backends " Christian König
` (6 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Similar to the TTM range manager.
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 51 ++++++++++++--------
1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 525cabe6c47a..5ebfaed37e47 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -23,6 +23,8 @@
*/
#include <linux/dma-mapping.h>
+#include <drm/ttm/ttm_range_manager.h>
+
#include "amdgpu.h"
#include "amdgpu_vm.h"
#include "amdgpu_res_cursor.h"
@@ -371,9 +373,9 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
struct amdgpu_vram_mgr *mgr = to_vram_mgr(man);
struct amdgpu_device *adev = to_amdgpu_device(mgr);
uint64_t vis_usage = 0, mem_bytes, max_bytes;
+ struct ttm_range_mgr_node *node;
struct drm_mm *mm = &mgr->mm;
enum drm_mm_insert_mode mode;
- struct drm_mm_node *nodes;
unsigned i;
int r;
@@ -388,8 +390,8 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
/* bail out quickly if there's likely not enough VRAM for this BO */
mem_bytes = (u64)mem->num_pages << PAGE_SHIFT;
if (atomic64_add_return(mem_bytes, &mgr->usage) > max_bytes) {
- atomic64_sub(mem_bytes, &mgr->usage);
- return -ENOSPC;
+ r = -ENOSPC;
+ goto error_sub;
}
if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
@@ -407,13 +409,15 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
num_nodes = DIV_ROUND_UP(mem->num_pages, pages_per_node);
}
- nodes = kvmalloc_array((uint32_t)num_nodes, sizeof(*nodes),
- GFP_KERNEL | __GFP_ZERO);
- if (!nodes) {
- atomic64_sub(mem_bytes, &mgr->usage);
- return -ENOMEM;
+ node = kvmalloc(struct_size(node, mm_nodes, num_nodes),
+ GFP_KERNEL | __GFP_ZERO);
+ if (!node) {
+ r = -ENOMEM;
+ goto error_sub;
}
+ ttm_resource_init(tbo, place, &node->base);
+
mode = DRM_MM_INSERT_BEST;
if (place->flags & TTM_PL_FLAG_TOPDOWN)
mode = DRM_MM_INSERT_HIGH;
@@ -432,8 +436,9 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
if (pages >= pages_per_node)
alignment = pages_per_node;
- r = drm_mm_insert_node_in_range(mm, &nodes[i], pages, alignment,
- 0, place->fpfn, lpfn, mode);
+ r = drm_mm_insert_node_in_range(mm, &node->mm_nodes[i], pages,
+ alignment, 0, place->fpfn,
+ lpfn, mode);
if (unlikely(r)) {
if (pages > pages_per_node) {
if (is_power_of_2(pages))
@@ -442,11 +447,11 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
pages = rounddown_pow_of_two(pages);
continue;
}
- goto error;
+ goto error_free;
}
- vis_usage += amdgpu_vram_mgr_vis_size(adev, &nodes[i]);
- amdgpu_vram_mgr_virt_start(mem, &nodes[i]);
+ vis_usage += amdgpu_vram_mgr_vis_size(adev, &node->mm_nodes[i]);
+ amdgpu_vram_mgr_virt_start(mem, &node->mm_nodes[i]);
pages_left -= pages;
++i;
@@ -459,16 +464,17 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
mem->placement |= TTM_PL_FLAG_CONTIGUOUS;
atomic64_add(vis_usage, &mgr->vis_usage);
- mem->mm_node = nodes;
+ mem->mm_node = &node->mm_nodes[0];
return 0;
-error:
+error_free:
while (i--)
- drm_mm_remove_node(&nodes[i]);
+ drm_mm_remove_node(&node->mm_nodes[i]);
spin_unlock(&mgr->lock);
- atomic64_sub(mem->num_pages << PAGE_SHIFT, &mgr->usage);
+ kvfree(node);
- kvfree(nodes);
+error_sub:
+ atomic64_sub(mem_bytes, &mgr->usage);
return r;
}
@@ -485,13 +491,17 @@ static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man,
{
struct amdgpu_vram_mgr *mgr = to_vram_mgr(man);
struct amdgpu_device *adev = to_amdgpu_device(mgr);
- struct drm_mm_node *nodes = mem->mm_node;
+ struct ttm_range_mgr_node *node;
uint64_t usage = 0, vis_usage = 0;
unsigned pages = mem->num_pages;
+ struct drm_mm_node *nodes;
if (!mem->mm_node)
return;
+ node = to_ttm_range_mgr_node(mem);
+ nodes = &node->mm_nodes[0];
+
spin_lock(&mgr->lock);
while (pages) {
pages -= nodes->size;
@@ -506,8 +516,7 @@ static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man,
atomic64_sub(usage, &mgr->usage);
atomic64_sub(vis_usage, &mgr->vis_usage);
- kvfree(mem->mm_node);
- mem->mm_node = NULL;
+ kvfree(node);
}
/**
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 08/10] drm/nouveau: switch the TTM backends to self alloc
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (5 preceding siblings ...)
2021-06-02 10:09 ` [PATCH 07/10] drm/amdgpu: switch the VRAM " Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-02 10:09 ` [PATCH 09/10] drm/vmwgfx: " Christian König
` (5 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Similar to the TTM range manager.
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/nouveau/nouveau_mem.h | 1 +
drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_mem.h b/drivers/gpu/drm/nouveau/nouveau_mem.h
index 7df3848e85aa..3a6a1be2ed52 100644
--- a/drivers/gpu/drm/nouveau/nouveau_mem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_mem.h
@@ -13,6 +13,7 @@ nouveau_mem(struct ttm_resource *reg)
}
struct nouveau_mem {
+ struct ttm_resource base;
struct nouveau_cli *cli;
u8 kind;
u8 comp;
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index b08b8efeefba..1ac2417effc0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -58,6 +58,8 @@ nouveau_vram_manager_new(struct ttm_resource_manager *man,
if (ret)
return ret;
+ ttm_resource_init(bo, place, reg->mm_node);
+
ret = nouveau_mem_vram(reg, nvbo->contig, nvbo->page);
if (ret) {
nouveau_mem_del(reg);
@@ -86,6 +88,7 @@ nouveau_gart_manager_new(struct ttm_resource_manager *man,
if (ret)
return ret;
+ ttm_resource_init(bo, place, reg->mm_node);
reg->start = 0;
return 0;
}
@@ -111,6 +114,7 @@ nv04_gart_manager_new(struct ttm_resource_manager *man,
if (ret)
return ret;
+ ttm_resource_init(bo, place, reg->mm_node);
ret = nvif_vmm_get(&mem->cli->vmm.vmm, PTES, false, 12, 0,
(long)reg->num_pages << PAGE_SHIFT, &mem->vma[0]);
if (ret) {
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 09/10] drm/vmwgfx: switch the TTM backends to self alloc
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (6 preceding siblings ...)
2021-06-02 10:09 ` [PATCH 08/10] drm/nouveau: switch the TTM backends " Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-02 10:09 ` [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2 Christian König
` (4 subsequent siblings)
12 siblings, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Similar to the TTM range manager.
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 18 +++++----
drivers/gpu/drm/vmwgfx/vmwgfx_thp.c | 37 ++++++++++---------
2 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
index 1774960d1b89..82a5e6489810 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
@@ -57,6 +57,12 @@ static int vmw_gmrid_man_get_node(struct ttm_resource_manager *man,
struct vmwgfx_gmrid_man *gman = to_gmrid_manager(man);
int id;
+ mem->mm_node = kmalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem->mm_node)
+ return -ENOMEM;
+
+ ttm_resource_init(bo, place, mem->mm_node);
+
id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
if (id < 0)
return id;
@@ -87,13 +93,11 @@ static void vmw_gmrid_man_put_node(struct ttm_resource_manager *man,
{
struct vmwgfx_gmrid_man *gman = to_gmrid_manager(man);
- if (mem->mm_node) {
- ida_free(&gman->gmr_ida, mem->start);
- spin_lock(&gman->lock);
- gman->used_gmr_pages -= mem->num_pages;
- spin_unlock(&gman->lock);
- mem->mm_node = NULL;
- }
+ ida_free(&gman->gmr_ida, mem->start);
+ spin_lock(&gman->lock);
+ gman->used_gmr_pages -= mem->num_pages;
+ spin_unlock(&gman->lock);
+ kfree(mem->mm_node);
}
static const struct ttm_resource_manager_func vmw_gmrid_manager_func;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c b/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c
index 5ccc35b3194c..8765835696ac 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c
@@ -7,6 +7,7 @@
#include "vmwgfx_drv.h"
#include <drm/ttm/ttm_bo_driver.h>
#include <drm/ttm/ttm_placement.h>
+#include <drm/ttm/ttm_range_manager.h>
/**
* struct vmw_thp_manager - Range manager implementing huge page alignment
@@ -54,16 +55,18 @@ static int vmw_thp_get_node(struct ttm_resource_manager *man,
{
struct vmw_thp_manager *rman = to_thp_manager(man);
struct drm_mm *mm = &rman->mm;
- struct drm_mm_node *node;
+ struct ttm_range_mgr_node *node;
unsigned long align_pages;
unsigned long lpfn;
enum drm_mm_insert_mode mode = DRM_MM_INSERT_BEST;
int ret;
- node = kzalloc(sizeof(*node), GFP_KERNEL);
+ node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
if (!node)
return -ENOMEM;
+ ttm_resource_init(bo, place, &node->base);
+
lpfn = place->lpfn;
if (!lpfn)
lpfn = man->size;
@@ -76,8 +79,9 @@ static int vmw_thp_get_node(struct ttm_resource_manager *man,
if (IS_ENABLED(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)) {
align_pages = (HPAGE_PUD_SIZE >> PAGE_SHIFT);
if (mem->num_pages >= align_pages) {
- ret = vmw_thp_insert_aligned(bo, mm, node, align_pages,
- place, mem, lpfn, mode);
+ ret = vmw_thp_insert_aligned(bo, mm, &node->mm_nodes[0],
+ align_pages, place, mem,
+ lpfn, mode);
if (!ret)
goto found_unlock;
}
@@ -85,14 +89,15 @@ static int vmw_thp_get_node(struct ttm_resource_manager *man,
align_pages = (HPAGE_PMD_SIZE >> PAGE_SHIFT);
if (mem->num_pages >= align_pages) {
- ret = vmw_thp_insert_aligned(bo, mm, node, align_pages, place,
- mem, lpfn, mode);
+ ret = vmw_thp_insert_aligned(bo, mm, &node->mm_nodes[0],
+ align_pages, place, mem, lpfn,
+ mode);
if (!ret)
goto found_unlock;
}
- ret = drm_mm_insert_node_in_range(mm, node, mem->num_pages,
- bo->page_alignment, 0,
+ ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0],
+ mem->num_pages, bo->page_alignment, 0,
place->fpfn, lpfn, mode);
found_unlock:
spin_unlock(&rman->lock);
@@ -100,8 +105,8 @@ static int vmw_thp_get_node(struct ttm_resource_manager *man,
if (unlikely(ret)) {
kfree(node);
} else {
- mem->mm_node = node;
- mem->start = node->start;
+ mem->mm_node = &node->mm_nodes[0];
+ mem->start = node->mm_nodes[0].start;
}
return ret;
@@ -113,15 +118,13 @@ static void vmw_thp_put_node(struct ttm_resource_manager *man,
struct ttm_resource *mem)
{
struct vmw_thp_manager *rman = to_thp_manager(man);
+ struct ttm_range_mgr_node * node = mem->mm_node;
- if (mem->mm_node) {
- spin_lock(&rman->lock);
- drm_mm_remove_node(mem->mm_node);
- spin_unlock(&rman->lock);
+ spin_lock(&rman->lock);
+ drm_mm_remove_node(&node->mm_nodes[0]);
+ spin_unlock(&rman->lock);
- kfree(mem->mm_node);
- mem->mm_node = NULL;
- }
+ kfree(node);
}
int vmw_thp_init(struct vmw_private *dev_priv)
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (7 preceding siblings ...)
2021-06-02 10:09 ` [PATCH 09/10] drm/vmwgfx: " Christian König
@ 2021-06-02 10:09 ` Christian König
2021-06-07 10:15 ` Thomas Hellström (Intel)
2021-06-03 8:45 ` [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Matthew Auld
` (3 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 10:09 UTC (permalink / raw)
To: matthew.auld, thomas_os, dri-devel
Instead of both driver and TTM allocating memory finalize embedding the
ttm_resource object as base into the driver backends.
v2: fix typo in vmwgfx grid mgr and double init in amdgpu_vram_mgr.c
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 44 ++++++--------
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
.../gpu/drm/amd/amdgpu/amdgpu_res_cursor.h | 5 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 60 +++++++++----------
drivers/gpu/drm/drm_gem_vram_helper.c | 3 +-
drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +--
drivers/gpu/drm/nouveau/nouveau_mem.c | 11 ++--
drivers/gpu/drm/nouveau/nouveau_mem.h | 14 ++---
drivers/gpu/drm/nouveau/nouveau_ttm.c | 32 +++++-----
drivers/gpu/drm/ttm/ttm_range_manager.c | 23 +++----
drivers/gpu/drm/ttm/ttm_resource.c | 18 +-----
drivers/gpu/drm/ttm/ttm_sys_manager.c | 12 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 24 ++++----
drivers/gpu/drm/vmwgfx/vmwgfx_thp.c | 27 ++++-----
include/drm/ttm/ttm_range_manager.h | 3 +-
include/drm/ttm/ttm_resource.h | 43 ++++++-------
16 files changed, 140 insertions(+), 189 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 29113f72bc39..194f9eecf89c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -40,8 +40,7 @@ to_gtt_mgr(struct ttm_resource_manager *man)
static inline struct amdgpu_gtt_node *
to_amdgpu_gtt_node(struct ttm_resource *res)
{
- return container_of(res->mm_node, struct amdgpu_gtt_node,
- base.mm_nodes[0]);
+ return container_of(res, struct amdgpu_gtt_node, base.base);
}
/**
@@ -102,13 +101,13 @@ const struct attribute_group amdgpu_gtt_mgr_attr_group = {
/**
* amdgpu_gtt_mgr_has_gart_addr - Check if mem has address space
*
- * @mem: the mem object to check
+ * @res: the mem object to check
*
* Check if a mem object has already address space allocated.
*/
-bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_resource *mem)
+bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_resource *res)
{
- struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(mem);
+ struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(res);
return drm_mm_node_allocated(&node->base.mm_nodes[0]);
}
@@ -126,19 +125,20 @@ bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_resource *mem)
static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
struct ttm_buffer_object *tbo,
const struct ttm_place *place,
- struct ttm_resource *mem)
+ struct ttm_resource **res)
{
struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
+ uint32_t num_pages = PFN_UP(tbo->base.size);
struct amdgpu_gtt_node *node;
int r;
spin_lock(&mgr->lock);
- if ((tbo->resource == mem || tbo->resource->mem_type != TTM_PL_TT) &&
- atomic64_read(&mgr->available) < mem->num_pages) {
+ if (tbo->resource && tbo->resource->mem_type != TTM_PL_TT &&
+ atomic64_read(&mgr->available) < num_pages) {
spin_unlock(&mgr->lock);
return -ENOSPC;
}
- atomic64_sub(mem->num_pages, &mgr->available);
+ atomic64_sub(num_pages, &mgr->available);
spin_unlock(&mgr->lock);
node = kzalloc(struct_size(node, base.mm_nodes, 1), GFP_KERNEL);
@@ -154,29 +154,28 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
spin_lock(&mgr->lock);
r = drm_mm_insert_node_in_range(&mgr->mm,
&node->base.mm_nodes[0],
- mem->num_pages,
- tbo->page_alignment, 0,
- place->fpfn, place->lpfn,
+ num_pages, tbo->page_alignment,
+ 0, place->fpfn, place->lpfn,
DRM_MM_INSERT_BEST);
spin_unlock(&mgr->lock);
if (unlikely(r))
goto err_free;
- mem->start = node->base.mm_nodes[0].start;
+ node->base.base.start = node->base.mm_nodes[0].start;
} else {
node->base.mm_nodes[0].start = 0;
- node->base.mm_nodes[0].size = mem->num_pages;
- mem->start = AMDGPU_BO_INVALID_OFFSET;
+ node->base.mm_nodes[0].size = node->base.base.num_pages;
+ node->base.base.start = AMDGPU_BO_INVALID_OFFSET;
}
- mem->mm_node = &node->base.mm_nodes[0];
+ *res = &node->base.base;
return 0;
err_free:
kfree(node);
err_out:
- atomic64_add(mem->num_pages, &mgr->available);
+ atomic64_add(num_pages, &mgr->available);
return r;
}
@@ -190,21 +189,16 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
* Free the allocated GTT again.
*/
static void amdgpu_gtt_mgr_del(struct ttm_resource_manager *man,
- struct ttm_resource *mem)
+ struct ttm_resource *res)
{
+ struct amdgpu_gtt_node *node = to_amdgpu_gtt_node(res);
struct amdgpu_gtt_mgr *mgr = to_gtt_mgr(man);
- struct amdgpu_gtt_node *node;
-
- if (!mem->mm_node)
- return;
-
- node = to_amdgpu_gtt_node(mem);
spin_lock(&mgr->lock);
if (drm_mm_node_allocated(&node->base.mm_nodes[0]))
drm_mm_remove_node(&node->base.mm_nodes[0]);
spin_unlock(&mgr->lock);
- atomic64_add(mem->num_pages, &mgr->available);
+ atomic64_add(res->num_pages, &mgr->available);
kfree(node);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 59723c3d5826..19c1384a133f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1296,7 +1296,7 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo)
if (bo->base.resv == &bo->base._resv)
amdgpu_amdkfd_remove_fence_on_pt_pd_bos(abo);
- if (bo->resource->mem_type != TTM_PL_VRAM || !bo->resource->mm_node ||
+ if (bo->resource->mem_type != TTM_PL_VRAM ||
!(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE))
return;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
index 40f2adf305bc..59e0fefb15aa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
@@ -28,6 +28,7 @@
#include <drm/drm_mm.h>
#include <drm/ttm/ttm_resource.h>
+#include <drm/ttm/ttm_range_manager.h>
/* state back for walking over vram_mgr and gtt_mgr allocations */
struct amdgpu_res_cursor {
@@ -53,7 +54,7 @@ static inline void amdgpu_res_first(struct ttm_resource *res,
{
struct drm_mm_node *node;
- if (!res || !res->mm_node) {
+ if (!res) {
cur->start = start;
cur->size = size;
cur->remaining = size;
@@ -63,7 +64,7 @@ static inline void amdgpu_res_first(struct ttm_resource *res,
BUG_ON(start + size > res->num_pages << PAGE_SHIFT);
- node = res->mm_node;
+ node = to_ttm_range_mgr_node(res)->mm_nodes;
while (start >= node->size << PAGE_SHIFT)
start -= node++->size << PAGE_SHIFT;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 5ebfaed37e47..9a6df02477ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -219,19 +219,20 @@ static u64 amdgpu_vram_mgr_vis_size(struct amdgpu_device *adev,
u64 amdgpu_vram_mgr_bo_visible_size(struct amdgpu_bo *bo)
{
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
- struct ttm_resource *mem = bo->tbo.resource;
- struct drm_mm_node *nodes = mem->mm_node;
- unsigned pages = mem->num_pages;
+ struct ttm_resource *res = bo->tbo.resource;
+ unsigned pages = res->num_pages;
+ struct drm_mm_node *mm;
u64 usage;
if (amdgpu_gmc_vram_full_visible(&adev->gmc))
return amdgpu_bo_size(bo);
- if (mem->start >= adev->gmc.visible_vram_size >> PAGE_SHIFT)
+ if (res->start >= adev->gmc.visible_vram_size >> PAGE_SHIFT)
return 0;
- for (usage = 0; nodes && pages; pages -= nodes->size, nodes++)
- usage += amdgpu_vram_mgr_vis_size(adev, nodes);
+ mm = &container_of(res, struct ttm_range_mgr_node, base)->mm_nodes[0];
+ for (usage = 0; pages; pages -= mm->size, mm++)
+ usage += amdgpu_vram_mgr_vis_size(adev, mm);
return usage;
}
@@ -367,7 +368,7 @@ static void amdgpu_vram_mgr_virt_start(struct ttm_resource *mem,
static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
struct ttm_buffer_object *tbo,
const struct ttm_place *place,
- struct ttm_resource *mem)
+ struct ttm_resource **res)
{
unsigned long lpfn, num_nodes, pages_per_node, pages_left, pages;
struct amdgpu_vram_mgr *mgr = to_vram_mgr(man);
@@ -388,7 +389,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
max_bytes -= AMDGPU_VM_RESERVED_VRAM;
/* bail out quickly if there's likely not enough VRAM for this BO */
- mem_bytes = (u64)mem->num_pages << PAGE_SHIFT;
+ mem_bytes = tbo->base.size;
if (atomic64_add_return(mem_bytes, &mgr->usage) > max_bytes) {
r = -ENOSPC;
goto error_sub;
@@ -406,7 +407,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
#endif
pages_per_node = max_t(uint32_t, pages_per_node,
tbo->page_alignment);
- num_nodes = DIV_ROUND_UP(mem->num_pages, pages_per_node);
+ num_nodes = DIV_ROUND_UP(PFN_UP(mem_bytes), pages_per_node);
}
node = kvmalloc(struct_size(node, mm_nodes, num_nodes),
@@ -422,8 +423,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
if (place->flags & TTM_PL_FLAG_TOPDOWN)
mode = DRM_MM_INSERT_HIGH;
- mem->start = 0;
- pages_left = mem->num_pages;
+ pages_left = node->base.num_pages;
/* Limit maximum size to 2GB due to SG table limitations */
pages = min(pages_left, 2UL << (30 - PAGE_SHIFT));
@@ -451,7 +451,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
}
vis_usage += amdgpu_vram_mgr_vis_size(adev, &node->mm_nodes[i]);
- amdgpu_vram_mgr_virt_start(mem, &node->mm_nodes[i]);
+ amdgpu_vram_mgr_virt_start(&node->base, &node->mm_nodes[i]);
pages_left -= pages;
++i;
@@ -461,10 +461,10 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
spin_unlock(&mgr->lock);
if (i == 1)
- mem->placement |= TTM_PL_FLAG_CONTIGUOUS;
+ node->base.placement |= TTM_PL_FLAG_CONTIGUOUS;
atomic64_add(vis_usage, &mgr->vis_usage);
- mem->mm_node = &node->mm_nodes[0];
+ *res = &node->base;
return 0;
error_free:
@@ -487,28 +487,22 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
* Free the allocated VRAM again.
*/
static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man,
- struct ttm_resource *mem)
+ struct ttm_resource *res)
{
+ struct ttm_range_mgr_node *node = to_ttm_range_mgr_node(res);
struct amdgpu_vram_mgr *mgr = to_vram_mgr(man);
struct amdgpu_device *adev = to_amdgpu_device(mgr);
- struct ttm_range_mgr_node *node;
uint64_t usage = 0, vis_usage = 0;
- unsigned pages = mem->num_pages;
- struct drm_mm_node *nodes;
-
- if (!mem->mm_node)
- return;
-
- node = to_ttm_range_mgr_node(mem);
- nodes = &node->mm_nodes[0];
+ unsigned i, pages;
spin_lock(&mgr->lock);
- while (pages) {
- pages -= nodes->size;
- drm_mm_remove_node(nodes);
- usage += nodes->size << PAGE_SHIFT;
- vis_usage += amdgpu_vram_mgr_vis_size(adev, nodes);
- ++nodes;
+ for (i = 0, pages = res->num_pages; pages;
+ pages -= node->mm_nodes[i].size, ++i) {
+ struct drm_mm_node *mm = &node->mm_nodes[i];
+
+ drm_mm_remove_node(mm);
+ usage += mm->size << PAGE_SHIFT;
+ vis_usage += amdgpu_vram_mgr_vis_size(adev, mm);
}
amdgpu_vram_mgr_do_reserve(man);
spin_unlock(&mgr->lock);
@@ -533,7 +527,7 @@ static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man,
* Allocate and fill a sg table from a VRAM allocation.
*/
int amdgpu_vram_mgr_alloc_sgt(struct amdgpu_device *adev,
- struct ttm_resource *mem,
+ struct ttm_resource *res,
u64 offset, u64 length,
struct device *dev,
enum dma_data_direction dir,
@@ -549,7 +543,7 @@ int amdgpu_vram_mgr_alloc_sgt(struct amdgpu_device *adev,
return -ENOMEM;
/* Determine the number of DRM_MM nodes to export */
- amdgpu_res_first(mem, offset, length, &cursor);
+ amdgpu_res_first(res, offset, length, &cursor);
while (cursor.remaining) {
num_entries++;
amdgpu_res_next(&cursor, cursor.size);
@@ -569,7 +563,7 @@ int amdgpu_vram_mgr_alloc_sgt(struct amdgpu_device *adev,
* and the number of bytes from it. Access the following
* DRM_MM node(s) if more buffer needs to exported
*/
- amdgpu_res_first(mem, offset, length, &cursor);
+ amdgpu_res_first(res, offset, length, &cursor);
for_each_sgtable_sg((*sgt), sg, i) {
phys_addr_t phys = cursor.start + adev->gmc.aper_base;
size_t size = cursor.size;
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 17a4c5d47b6a..2a1229b8364e 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -250,7 +250,8 @@ EXPORT_SYMBOL(drm_gem_vram_put);
static u64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo)
{
/* Keep TTM behavior for now, remove when drivers are audited */
- if (WARN_ON_ONCE(!gbo->bo.resource->mm_node))
+ if (WARN_ON_ONCE(!gbo->bo.resource ||
+ gbo->bo.resource->mem_type == TTM_PL_SYSTEM))
return 0;
return gbo->bo.resource->start;
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 3a0d9b3bf991..c3d20bc80022 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -918,12 +918,8 @@ static void nouveau_bo_move_ntfy(struct ttm_buffer_object *bo,
}
}
- if (new_reg) {
- if (new_reg->mm_node)
- nvbo->offset = (new_reg->start << PAGE_SHIFT);
- else
- nvbo->offset = 0;
- }
+ if (new_reg)
+ nvbo->offset = (new_reg->start << PAGE_SHIFT);
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_mem.c b/drivers/gpu/drm/nouveau/nouveau_mem.c
index a1049e9feee1..0de6549fb875 100644
--- a/drivers/gpu/drm/nouveau/nouveau_mem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_mem.c
@@ -178,25 +178,24 @@ void
nouveau_mem_del(struct ttm_resource *reg)
{
struct nouveau_mem *mem = nouveau_mem(reg);
- if (!mem)
- return;
+
nouveau_mem_fini(mem);
- kfree(reg->mm_node);
- reg->mm_node = NULL;
+ kfree(mem);
}
int
nouveau_mem_new(struct nouveau_cli *cli, u8 kind, u8 comp,
- struct ttm_resource *reg)
+ struct ttm_resource **res)
{
struct nouveau_mem *mem;
if (!(mem = kzalloc(sizeof(*mem), GFP_KERNEL)))
return -ENOMEM;
+
mem->cli = cli;
mem->kind = kind;
mem->comp = comp;
- reg->mm_node = mem;
+ *res = &mem->base;
return 0;
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_mem.h b/drivers/gpu/drm/nouveau/nouveau_mem.h
index 3a6a1be2ed52..2c01166a90f2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_mem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_mem.h
@@ -6,12 +6,6 @@ struct ttm_tt;
#include <nvif/mem.h>
#include <nvif/vmm.h>
-static inline struct nouveau_mem *
-nouveau_mem(struct ttm_resource *reg)
-{
- return reg->mm_node;
-}
-
struct nouveau_mem {
struct ttm_resource base;
struct nouveau_cli *cli;
@@ -21,8 +15,14 @@ struct nouveau_mem {
struct nvif_vma vma[2];
};
+static inline struct nouveau_mem *
+nouveau_mem(struct ttm_resource *reg)
+{
+ return container_of(reg, struct nouveau_mem, base);
+}
+
int nouveau_mem_new(struct nouveau_cli *, u8 kind, u8 comp,
- struct ttm_resource *);
+ struct ttm_resource **);
void nouveau_mem_del(struct ttm_resource *);
int nouveau_mem_vram(struct ttm_resource *, bool contig, u8 page);
int nouveau_mem_host(struct ttm_resource *, struct ttm_tt *);
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index 1ac2417effc0..f4c2e46b6fe1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -45,7 +45,7 @@ static int
nouveau_vram_manager_new(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *reg)
+ struct ttm_resource **res)
{
struct nouveau_bo *nvbo = nouveau_bo(bo);
struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
@@ -54,15 +54,15 @@ nouveau_vram_manager_new(struct ttm_resource_manager *man,
if (drm->client.device.info.ram_size == 0)
return -ENOMEM;
- ret = nouveau_mem_new(&drm->master, nvbo->kind, nvbo->comp, reg);
+ ret = nouveau_mem_new(&drm->master, nvbo->kind, nvbo->comp, res);
if (ret)
return ret;
- ttm_resource_init(bo, place, reg->mm_node);
+ ttm_resource_init(bo, place, *res);
- ret = nouveau_mem_vram(reg, nvbo->contig, nvbo->page);
+ ret = nouveau_mem_vram(*res, nvbo->contig, nvbo->page);
if (ret) {
- nouveau_mem_del(reg);
+ nouveau_mem_del(*res);
return ret;
}
@@ -78,18 +78,18 @@ static int
nouveau_gart_manager_new(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *reg)
+ struct ttm_resource **res)
{
struct nouveau_bo *nvbo = nouveau_bo(bo);
struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
int ret;
- ret = nouveau_mem_new(&drm->master, nvbo->kind, nvbo->comp, reg);
+ ret = nouveau_mem_new(&drm->master, nvbo->kind, nvbo->comp, res);
if (ret)
return ret;
- ttm_resource_init(bo, place, reg->mm_node);
- reg->start = 0;
+ ttm_resource_init(bo, place, *res);
+ (*res)->start = 0;
return 0;
}
@@ -102,27 +102,27 @@ static int
nv04_gart_manager_new(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *reg)
+ struct ttm_resource **res)
{
struct nouveau_bo *nvbo = nouveau_bo(bo);
struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
struct nouveau_mem *mem;
int ret;
- ret = nouveau_mem_new(&drm->master, nvbo->kind, nvbo->comp, reg);
- mem = nouveau_mem(reg);
+ ret = nouveau_mem_new(&drm->master, nvbo->kind, nvbo->comp, res);
if (ret)
return ret;
- ttm_resource_init(bo, place, reg->mm_node);
+ mem = nouveau_mem(*res);
+ ttm_resource_init(bo, place, *res);
ret = nvif_vmm_get(&mem->cli->vmm.vmm, PTES, false, 12, 0,
- (long)reg->num_pages << PAGE_SHIFT, &mem->vma[0]);
+ (long)(*res)->num_pages << PAGE_SHIFT, &mem->vma[0]);
if (ret) {
- nouveau_mem_del(reg);
+ nouveau_mem_del(*res);
return ret;
}
- reg->start = mem->vma[0].addr >> PAGE_SHIFT;
+ (*res)->start = mem->vma[0].addr >> PAGE_SHIFT;
return 0;
}
diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
index ce5d07ca384c..c32e1aee2481 100644
--- a/drivers/gpu/drm/ttm/ttm_range_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
@@ -58,7 +58,7 @@ to_range_manager(struct ttm_resource_manager *man)
static int ttm_range_man_alloc(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *mem)
+ struct ttm_resource **res)
{
struct ttm_range_manager *rman = to_range_manager(man);
struct ttm_range_mgr_node *node;
@@ -83,37 +83,30 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man,
spin_lock(&rman->lock);
ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0],
- mem->num_pages, bo->page_alignment, 0,
+ node->base.num_pages,
+ bo->page_alignment, 0,
place->fpfn, lpfn, mode);
spin_unlock(&rman->lock);
- if (unlikely(ret)) {
+ if (unlikely(ret))
kfree(node);
- } else {
- mem->mm_node = &node->mm_nodes[0];
- mem->start = node->mm_nodes[0].start;
- }
+ else
+ node->base.start = node->mm_nodes[0].start;
return ret;
}
static void ttm_range_man_free(struct ttm_resource_manager *man,
- struct ttm_resource *mem)
+ struct ttm_resource *res)
{
+ struct ttm_range_mgr_node *node = to_ttm_range_mgr_node(res);
struct ttm_range_manager *rman = to_range_manager(man);
- struct ttm_range_mgr_node *node;
-
- if (!mem->mm_node)
- return;
-
- node = to_ttm_range_mgr_node(mem);
spin_lock(&rman->lock);
drm_mm_remove_node(&node->mm_nodes[0]);
spin_unlock(&rman->lock);
kfree(node);
- mem->mm_node = NULL;
}
static void ttm_range_man_debug(struct ttm_resource_manager *man,
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 2a51ace17614..2a68145572cc 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -29,7 +29,6 @@ void ttm_resource_init(struct ttm_buffer_object *bo,
const struct ttm_place *place,
struct ttm_resource *res)
{
- res->mm_node = NULL;
res->start = 0;
res->num_pages = PFN_UP(bo->base.size);
res->mem_type = place->mem_type;
@@ -47,22 +46,8 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo,
{
struct ttm_resource_manager *man =
ttm_manager_type(bo->bdev, place->mem_type);
- struct ttm_resource *res;
- int r;
-
- res = kmalloc(sizeof(*res), GFP_KERNEL);
- if (!res)
- return -ENOMEM;
-
- ttm_resource_init(bo, place, res);
- r = man->func->alloc(man, bo, place, res);
- if (r) {
- kfree(res);
- return r;
- }
- *res_ptr = res;
- return 0;
+ return man->func->alloc(man, bo, place, res_ptr);
}
void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
@@ -74,7 +59,6 @@ void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
man = ttm_manager_type(bo->bdev, (*res)->mem_type);
man->func->free(man, *res);
- kfree(*res);
*res = NULL;
}
EXPORT_SYMBOL(ttm_resource_free);
diff --git a/drivers/gpu/drm/ttm/ttm_sys_manager.c b/drivers/gpu/drm/ttm/ttm_sys_manager.c
index 2b75f493c3c9..63aca52f75e1 100644
--- a/drivers/gpu/drm/ttm/ttm_sys_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_sys_manager.c
@@ -10,20 +10,20 @@
static int ttm_sys_man_alloc(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *mem)
+ struct ttm_resource **res)
{
- mem->mm_node = kzalloc(sizeof(*mem), GFP_KERNEL);
- if (!mem->mm_node)
+ *res = kzalloc(sizeof(**res), GFP_KERNEL);
+ if (!*res)
return -ENOMEM;
- ttm_resource_init(bo, place, mem->mm_node);
+ ttm_resource_init(bo, place, *res);
return 0;
}
static void ttm_sys_man_free(struct ttm_resource_manager *man,
- struct ttm_resource *mem)
+ struct ttm_resource *res)
{
- kfree(mem->mm_node);
+ kfree(res);
}
static const struct ttm_resource_manager_func ttm_sys_manager_func = {
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
index 82a5e6489810..28ceb749a733 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
@@ -52,16 +52,16 @@ static struct vmwgfx_gmrid_man *to_gmrid_manager(struct ttm_resource_manager *ma
static int vmw_gmrid_man_get_node(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *mem)
+ struct ttm_resource **res)
{
struct vmwgfx_gmrid_man *gman = to_gmrid_manager(man);
int id;
- mem->mm_node = kmalloc(sizeof(*mem), GFP_KERNEL);
- if (!mem->mm_node)
+ *res = kmalloc(sizeof(**res), GFP_KERNEL);
+ if (!*res)
return -ENOMEM;
- ttm_resource_init(bo, place, mem->mm_node);
+ ttm_resource_init(bo, place, *res);
id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
if (id < 0)
@@ -70,34 +70,34 @@ static int vmw_gmrid_man_get_node(struct ttm_resource_manager *man,
spin_lock(&gman->lock);
if (gman->max_gmr_pages > 0) {
- gman->used_gmr_pages += mem->num_pages;
+ gman->used_gmr_pages += (*res)->num_pages;
if (unlikely(gman->used_gmr_pages > gman->max_gmr_pages))
goto nospace;
}
- mem->mm_node = gman;
- mem->start = id;
+ (*res)->start = id;
spin_unlock(&gman->lock);
return 0;
nospace:
- gman->used_gmr_pages -= mem->num_pages;
+ gman->used_gmr_pages -= (*res)->num_pages;
spin_unlock(&gman->lock);
ida_free(&gman->gmr_ida, id);
+ kfree(*res);
return -ENOSPC;
}
static void vmw_gmrid_man_put_node(struct ttm_resource_manager *man,
- struct ttm_resource *mem)
+ struct ttm_resource *res)
{
struct vmwgfx_gmrid_man *gman = to_gmrid_manager(man);
- ida_free(&gman->gmr_ida, mem->start);
+ ida_free(&gman->gmr_ida, res->start);
spin_lock(&gman->lock);
- gman->used_gmr_pages -= mem->num_pages;
+ gman->used_gmr_pages -= res->num_pages;
spin_unlock(&gman->lock);
- kfree(mem->mm_node);
+ kfree(res);
}
static const struct ttm_resource_manager_func vmw_gmrid_manager_func;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c b/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c
index 8765835696ac..2a3d3468e4e0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_thp.c
@@ -51,7 +51,7 @@ static int vmw_thp_insert_aligned(struct ttm_buffer_object *bo,
static int vmw_thp_get_node(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *mem)
+ struct ttm_resource **res)
{
struct vmw_thp_manager *rman = to_thp_manager(man);
struct drm_mm *mm = &rman->mm;
@@ -78,26 +78,27 @@ static int vmw_thp_get_node(struct ttm_resource_manager *man,
spin_lock(&rman->lock);
if (IS_ENABLED(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)) {
align_pages = (HPAGE_PUD_SIZE >> PAGE_SHIFT);
- if (mem->num_pages >= align_pages) {
+ if (node->base.num_pages >= align_pages) {
ret = vmw_thp_insert_aligned(bo, mm, &node->mm_nodes[0],
- align_pages, place, mem,
- lpfn, mode);
+ align_pages, place,
+ &node->base, lpfn, mode);
if (!ret)
goto found_unlock;
}
}
align_pages = (HPAGE_PMD_SIZE >> PAGE_SHIFT);
- if (mem->num_pages >= align_pages) {
+ if (node->base.num_pages >= align_pages) {
ret = vmw_thp_insert_aligned(bo, mm, &node->mm_nodes[0],
- align_pages, place, mem, lpfn,
- mode);
+ align_pages, place, &node->base,
+ lpfn, mode);
if (!ret)
goto found_unlock;
}
ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0],
- mem->num_pages, bo->page_alignment, 0,
+ node->base.num_pages,
+ bo->page_alignment, 0,
place->fpfn, lpfn, mode);
found_unlock:
spin_unlock(&rman->lock);
@@ -105,20 +106,18 @@ static int vmw_thp_get_node(struct ttm_resource_manager *man,
if (unlikely(ret)) {
kfree(node);
} else {
- mem->mm_node = &node->mm_nodes[0];
- mem->start = node->mm_nodes[0].start;
+ node->base.start = node->mm_nodes[0].start;
+ *res = &node->base;
}
return ret;
}
-
-
static void vmw_thp_put_node(struct ttm_resource_manager *man,
- struct ttm_resource *mem)
+ struct ttm_resource *res)
{
+ struct ttm_range_mgr_node *node = to_ttm_range_mgr_node(res);
struct vmw_thp_manager *rman = to_thp_manager(man);
- struct ttm_range_mgr_node * node = mem->mm_node;
spin_lock(&rman->lock);
drm_mm_remove_node(&node->mm_nodes[0]);
diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h
index 983f452ce54b..22b6fa42ac20 100644
--- a/include/drm/ttm/ttm_range_manager.h
+++ b/include/drm/ttm/ttm_range_manager.h
@@ -30,8 +30,7 @@ struct ttm_range_mgr_node {
static inline struct ttm_range_mgr_node *
to_ttm_range_mgr_node(struct ttm_resource *res)
{
- return container_of(res->mm_node, struct ttm_range_mgr_node,
- mm_nodes[0]);
+ return container_of(res, struct ttm_range_mgr_node, base);
}
int ttm_range_man_init(struct ttm_device *bdev,
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index 803e4875d779..4abb95b9fd11 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -45,46 +45,38 @@ struct ttm_resource_manager_func {
*
* @man: Pointer to a memory type manager.
* @bo: Pointer to the buffer object we're allocating space for.
- * @placement: Placement details.
- * @flags: Additional placement flags.
- * @mem: Pointer to a struct ttm_resource to be filled in.
+ * @place: Placement details.
+ * @res: Resulting pointer to the ttm_resource.
*
* This function should allocate space in the memory type managed
- * by @man. Placement details if
- * applicable are given by @placement. If successful,
- * @mem::mm_node should be set to a non-null value, and
- * @mem::start should be set to a value identifying the beginning
+ * by @man. Placement details if applicable are given by @place. If
+ * successful, a filled in ttm_resource object should be returned in
+ * @res. @res::start should be set to a value identifying the beginning
* of the range allocated, and the function should return zero.
- * If the memory region accommodate the buffer object, @mem::mm_node
- * should be set to NULL, and the function should return 0.
+ * If the manager can't fulfill the request -ENOSPC should be returned.
* If a system error occurred, preventing the request to be fulfilled,
* the function should return a negative error code.
*
- * Note that @mem::mm_node will only be dereferenced by
- * struct ttm_resource_manager functions and optionally by the driver,
- * which has knowledge of the underlying type.
- *
- * This function may not be called from within atomic context, so
- * an implementation can and must use either a mutex or a spinlock to
- * protect any data structures managing the space.
+ * This function may not be called from within atomic context and needs
+ * to take care of its own locking to protect any data structures
+ * managing the space.
*/
int (*alloc)(struct ttm_resource_manager *man,
struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource *mem);
+ struct ttm_resource **res);
/**
* struct ttm_resource_manager_func member free
*
* @man: Pointer to a memory type manager.
- * @mem: Pointer to a struct ttm_resource to be filled in.
+ * @res: Pointer to a struct ttm_resource to be freed.
*
- * This function frees memory type resources previously allocated
- * and that are identified by @mem::mm_node and @mem::start. May not
- * be called from within atomic context.
+ * This function frees memory type resources previously allocated.
+ * May not be called from within atomic context.
*/
void (*free)(struct ttm_resource_manager *man,
- struct ttm_resource *mem);
+ struct ttm_resource *res);
/**
* struct ttm_resource_manager_func member debug
@@ -158,9 +150,9 @@ struct ttm_bus_placement {
/**
* struct ttm_resource
*
- * @mm_node: Memory manager node.
- * @size: Requested size of memory region.
- * @num_pages: Actual size of memory region in pages.
+ * @start: Start of the allocation.
+ * @num_pages: Actual size of resource in pages.
+ * @mem_type: Resource type of the allocation.
* @placement: Placement flags.
* @bus: Placement on io bus accessible to the CPU
*
@@ -168,7 +160,6 @@ struct ttm_bus_placement {
* buffer object.
*/
struct ttm_resource {
- void *mm_node;
unsigned long start;
unsigned long num_pages;
uint32_t mem_type;
--
2.25.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 10:09 ` [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes Christian König
@ 2021-06-02 11:44 ` Thomas Hellström (Intel)
2021-06-02 12:11 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-02 11:44 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/2/21 12:09 PM, Christian König wrote:
> Start with the range manager to make the resource object the base
> class for the allocated nodes.
>
> While at it cleanup a lot of the code around that.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
> drivers/gpu/drm/ttm/ttm_range_manager.c | 56 ++++++++++++++++++-------
> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
> include/drm/ttm/ttm_bo_driver.h | 26 ------------
> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
> include/drm/ttm/ttm_resource.h | 3 ++
> 10 files changed, 111 insertions(+), 50 deletions(-)
> create mode 100644 include/drm/ttm/ttm_range_manager.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 69db89261650..df1f185faae9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -45,6 +45,7 @@
> #include <drm/ttm/ttm_bo_api.h>
> #include <drm/ttm/ttm_bo_driver.h>
> #include <drm/ttm/ttm_placement.h>
> +#include <drm/ttm/ttm_range_manager.h>
>
> #include <drm/amdgpu_drm.h>
>
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 83e7258c7f90..17a4c5d47b6a 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -17,6 +17,8 @@
> #include <drm/drm_prime.h>
> #include <drm/drm_simple_kms_helper.h>
>
> +#include <drm/ttm/ttm_range_manager.h>
> +
> static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>
> /**
> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> index 65430912ff72..b08b8efeefba 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> @@ -26,6 +26,8 @@
> #include <linux/limits.h>
> #include <linux/swiotlb.h>
>
> +#include <drm/ttm/ttm_range_manager.h>
> +
> #include "nouveau_drv.h"
> #include "nouveau_gem.h"
> #include "nouveau_mem.h"
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index 8aa87b8edb9c..19fd39d9a00c 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -32,6 +32,7 @@
> #include <drm/ttm/ttm_bo_api.h>
> #include <drm/ttm/ttm_bo_driver.h>
> #include <drm/ttm/ttm_placement.h>
> +#include <drm/ttm/ttm_range_manager.h>
>
> #include "qxl_drv.h"
> #include "qxl_object.h"
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index cdffa9b65108..ad2a5a791bba 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -45,6 +45,7 @@
> #include <drm/ttm/ttm_bo_api.h>
> #include <drm/ttm/ttm_bo_driver.h>
> #include <drm/ttm/ttm_placement.h>
> +#include <drm/ttm/ttm_range_manager.h>
>
> #include "radeon_reg.h"
> #include "radeon.h"
> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
> index b9d5da6e6a81..ce5d07ca384c 100644
> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
> @@ -29,12 +29,13 @@
> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
> */
>
> -#include <drm/ttm/ttm_bo_driver.h>
> +#include <drm/ttm/ttm_device.h>
> #include <drm/ttm/ttm_placement.h>
> +#include <drm/ttm/ttm_range_manager.h>
> +#include <drm/ttm/ttm_bo_api.h>
> #include <drm/drm_mm.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> -#include <linux/module.h>
>
> /*
> * Currently we use a spinlock for the lock, but a mutex *may* be
> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man,
> struct ttm_resource *mem)
> {
> struct ttm_range_manager *rman = to_range_manager(man);
> + struct ttm_range_mgr_node *node;
> struct drm_mm *mm = &rman->mm;
> - struct drm_mm_node *node;
> enum drm_mm_insert_mode mode;
> unsigned long lpfn;
> int ret;
> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man,
> if (!lpfn)
> lpfn = man->size;
>
> - node = kzalloc(sizeof(*node), GFP_KERNEL);
> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
I'm still a bit confused about the situation where a driver wants to
attach private data to a struct ttm_resource without having to
re-implement its own range manager?
Could be cached sg-tables, list of GPU bindings etc. Wouldn't work with
the above unless we have a void *driver_private member on the struct
ttm_resource. Is that the plan going forward here? Or that the driver
actually does the re-implementation?
Thanks,
Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 11:44 ` Thomas Hellström (Intel)
@ 2021-06-02 12:11 ` Christian König
2021-06-02 12:33 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 12:11 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>
> On 6/2/21 12:09 PM, Christian König wrote:
>> Start with the range manager to make the resource object the base
>> class for the allocated nodes.
>>
>> While at it cleanup a lot of the code around that.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56 ++++++++++++++++++-------
>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
>> include/drm/ttm/ttm_resource.h | 3 ++
>> 10 files changed, 111 insertions(+), 50 deletions(-)
>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 69db89261650..df1f185faae9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -45,6 +45,7 @@
>> #include <drm/ttm/ttm_bo_api.h>
>> #include <drm/ttm/ttm_bo_driver.h>
>> #include <drm/ttm/ttm_placement.h>
>> +#include <drm/ttm/ttm_range_manager.h>
>> #include <drm/amdgpu_drm.h>
>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>> index 83e7258c7f90..17a4c5d47b6a 100644
>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>> @@ -17,6 +17,8 @@
>> #include <drm/drm_prime.h>
>> #include <drm/drm_simple_kms_helper.h>
>> +#include <drm/ttm/ttm_range_manager.h>
>> +
>> static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>> /**
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>> index 65430912ff72..b08b8efeefba 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>> @@ -26,6 +26,8 @@
>> #include <linux/limits.h>
>> #include <linux/swiotlb.h>
>> +#include <drm/ttm/ttm_range_manager.h>
>> +
>> #include "nouveau_drv.h"
>> #include "nouveau_gem.h"
>> #include "nouveau_mem.h"
>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>> index 8aa87b8edb9c..19fd39d9a00c 100644
>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>> @@ -32,6 +32,7 @@
>> #include <drm/ttm/ttm_bo_api.h>
>> #include <drm/ttm/ttm_bo_driver.h>
>> #include <drm/ttm/ttm_placement.h>
>> +#include <drm/ttm/ttm_range_manager.h>
>> #include "qxl_drv.h"
>> #include "qxl_object.h"
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index cdffa9b65108..ad2a5a791bba 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -45,6 +45,7 @@
>> #include <drm/ttm/ttm_bo_api.h>
>> #include <drm/ttm/ttm_bo_driver.h>
>> #include <drm/ttm/ttm_placement.h>
>> +#include <drm/ttm/ttm_range_manager.h>
>> #include "radeon_reg.h"
>> #include "radeon.h"
>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> index b9d5da6e6a81..ce5d07ca384c 100644
>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> @@ -29,12 +29,13 @@
>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>> */
>> -#include <drm/ttm/ttm_bo_driver.h>
>> +#include <drm/ttm/ttm_device.h>
>> #include <drm/ttm/ttm_placement.h>
>> +#include <drm/ttm/ttm_range_manager.h>
>> +#include <drm/ttm/ttm_bo_api.h>
>> #include <drm/drm_mm.h>
>> #include <linux/slab.h>
>> #include <linux/spinlock.h>
>> -#include <linux/module.h>
>> /*
>> * Currently we use a spinlock for the lock, but a mutex *may* be
>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>> ttm_resource_manager *man,
>> struct ttm_resource *mem)
>> {
>> struct ttm_range_manager *rman = to_range_manager(man);
>> + struct ttm_range_mgr_node *node;
>> struct drm_mm *mm = &rman->mm;
>> - struct drm_mm_node *node;
>> enum drm_mm_insert_mode mode;
>> unsigned long lpfn;
>> int ret;
>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>> ttm_resource_manager *man,
>> if (!lpfn)
>> lpfn = man->size;
>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>
> I'm still a bit confused about the situation where a driver wants to
> attach private data to a struct ttm_resource without having to
> re-implement its own range manager?
>
> Could be cached sg-tables, list of GPU bindings etc. Wouldn't work
> with the above unless we have a void *driver_private member on the
> struct ttm_resource. Is that the plan going forward here? Or that the
> driver actually does the re-implementation?
I don't really understand your concern here. The basic idea is that
drivers use ttm_resource as a base class for their own implementation.
See for example how nouveau does that:
struct nouveau_mem {
struct ttm_resource base;
struct nouveau_cli *cli;
u8 kind;
u8 comp;
struct nvif_mem mem;
struct nvif_vma vma[2];
};
The range manager is helping driver specific resource managers which
want to implement something drm_mm_nodes based. E.g. amdgpu_gtt_mgr and
amdgpu_vram_mgr, but it can also be used stand alone.
The ttm_range_mgr_node can then be used as base class for this
functionality. I already want to move some more code from
amdgpu_vram_mgr.c into the range manager, but that is just minor cleanup
work.
Regards,
Christian.
>
> Thanks,
>
> Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 12:11 ` Christian König
@ 2021-06-02 12:33 ` Thomas Hellström (Intel)
2021-06-02 13:07 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-02 12:33 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/2/21 2:11 PM, Christian König wrote:
> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>
>> On 6/2/21 12:09 PM, Christian König wrote:
>>> Start with the range manager to make the resource object the base
>>> class for the allocated nodes.
>>>
>>> While at it cleanup a lot of the code around that.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>> ++++++++++++++++++-------
>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
>>> include/drm/ttm/ttm_resource.h | 3 ++
>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 69db89261650..df1f185faae9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -45,6 +45,7 @@
>>> #include <drm/ttm/ttm_bo_api.h>
>>> #include <drm/ttm/ttm_bo_driver.h>
>>> #include <drm/ttm/ttm_placement.h>
>>> +#include <drm/ttm/ttm_range_manager.h>
>>> #include <drm/amdgpu_drm.h>
>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> @@ -17,6 +17,8 @@
>>> #include <drm/drm_prime.h>
>>> #include <drm/drm_simple_kms_helper.h>
>>> +#include <drm/ttm/ttm_range_manager.h>
>>> +
>>> static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>>> /**
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>> index 65430912ff72..b08b8efeefba 100644
>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>> @@ -26,6 +26,8 @@
>>> #include <linux/limits.h>
>>> #include <linux/swiotlb.h>
>>> +#include <drm/ttm/ttm_range_manager.h>
>>> +
>>> #include "nouveau_drv.h"
>>> #include "nouveau_gem.h"
>>> #include "nouveau_mem.h"
>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>> @@ -32,6 +32,7 @@
>>> #include <drm/ttm/ttm_bo_api.h>
>>> #include <drm/ttm/ttm_bo_driver.h>
>>> #include <drm/ttm/ttm_placement.h>
>>> +#include <drm/ttm/ttm_range_manager.h>
>>> #include "qxl_drv.h"
>>> #include "qxl_object.h"
>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>> index cdffa9b65108..ad2a5a791bba 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>> @@ -45,6 +45,7 @@
>>> #include <drm/ttm/ttm_bo_api.h>
>>> #include <drm/ttm/ttm_bo_driver.h>
>>> #include <drm/ttm/ttm_placement.h>
>>> +#include <drm/ttm/ttm_range_manager.h>
>>> #include "radeon_reg.h"
>>> #include "radeon.h"
>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> @@ -29,12 +29,13 @@
>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>> */
>>> -#include <drm/ttm/ttm_bo_driver.h>
>>> +#include <drm/ttm/ttm_device.h>
>>> #include <drm/ttm/ttm_placement.h>
>>> +#include <drm/ttm/ttm_range_manager.h>
>>> +#include <drm/ttm/ttm_bo_api.h>
>>> #include <drm/drm_mm.h>
>>> #include <linux/slab.h>
>>> #include <linux/spinlock.h>
>>> -#include <linux/module.h>
>>> /*
>>> * Currently we use a spinlock for the lock, but a mutex *may* be
>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>> ttm_resource_manager *man,
>>> struct ttm_resource *mem)
>>> {
>>> struct ttm_range_manager *rman = to_range_manager(man);
>>> + struct ttm_range_mgr_node *node;
>>> struct drm_mm *mm = &rman->mm;
>>> - struct drm_mm_node *node;
>>> enum drm_mm_insert_mode mode;
>>> unsigned long lpfn;
>>> int ret;
>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>> ttm_resource_manager *man,
>>> if (!lpfn)
>>> lpfn = man->size;
>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>
>> I'm still a bit confused about the situation where a driver wants to
>> attach private data to a struct ttm_resource without having to
>> re-implement its own range manager?
>>
>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't work
>> with the above unless we have a void *driver_private member on the
>> struct ttm_resource. Is that the plan going forward here? Or that the
>> driver actually does the re-implementation?
>
> I don't really understand your concern here. The basic idea is that
> drivers use ttm_resource as a base class for their own implementation.
>
> See for example how nouveau does that:
>
> struct nouveau_mem {
> struct ttm_resource base;
> struct nouveau_cli *cli;
> u8 kind;
> u8 comp;
> struct nvif_mem mem;
> struct nvif_vma vma[2];
> };
>
> The range manager is helping driver specific resource managers which
> want to implement something drm_mm_nodes based. E.g. amdgpu_gtt_mgr
> and amdgpu_vram_mgr, but it can also be used stand alone.
>
> The ttm_range_mgr_node can then be used as base class for this
> functionality. I already want to move some more code from
> amdgpu_vram_mgr.c into the range manager, but that is just minor
> cleanup work.
>
Sure but if you embed a ttm_range_mgr_node in your struct i915_resource,
and wanted to use the ttm range manager for it, it would allocate a
struct ttm_range_mgr_node rather than a struct i915_resource? Or am I
missing something?
/Thomas
> Regards,
> Christian.
>
>>
>> Thanks,
>>
>> Thomas
>>
>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 12:33 ` Thomas Hellström (Intel)
@ 2021-06-02 13:07 ` Christian König
2021-06-02 14:13 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 13:07 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 02.06.21 um 14:33 schrieb Thomas Hellström (Intel):
>
> On 6/2/21 2:11 PM, Christian König wrote:
>> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>>
>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>> Start with the range manager to make the resource object the base
>>>> class for the allocated nodes.
>>>>
>>>> While at it cleanup a lot of the code around that.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>>> ++++++++++++++++++-------
>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>>> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
>>>> include/drm/ttm/ttm_resource.h | 3 ++
>>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 69db89261650..df1f185faae9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -45,6 +45,7 @@
>>>> #include <drm/ttm/ttm_bo_api.h>
>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>> #include <drm/ttm/ttm_placement.h>
>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>> #include <drm/amdgpu_drm.h>
>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>> @@ -17,6 +17,8 @@
>>>> #include <drm/drm_prime.h>
>>>> #include <drm/drm_simple_kms_helper.h>
>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>> +
>>>> static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>>>> /**
>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>> index 65430912ff72..b08b8efeefba 100644
>>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>> @@ -26,6 +26,8 @@
>>>> #include <linux/limits.h>
>>>> #include <linux/swiotlb.h>
>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>> +
>>>> #include "nouveau_drv.h"
>>>> #include "nouveau_gem.h"
>>>> #include "nouveau_mem.h"
>>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>> @@ -32,6 +32,7 @@
>>>> #include <drm/ttm/ttm_bo_api.h>
>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>> #include <drm/ttm/ttm_placement.h>
>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>> #include "qxl_drv.h"
>>>> #include "qxl_object.h"
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> index cdffa9b65108..ad2a5a791bba 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> @@ -45,6 +45,7 @@
>>>> #include <drm/ttm/ttm_bo_api.h>
>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>> #include <drm/ttm/ttm_placement.h>
>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>> #include "radeon_reg.h"
>>>> #include "radeon.h"
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>> @@ -29,12 +29,13 @@
>>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>> */
>>>> -#include <drm/ttm/ttm_bo_driver.h>
>>>> +#include <drm/ttm/ttm_device.h>
>>>> #include <drm/ttm/ttm_placement.h>
>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>> +#include <drm/ttm/ttm_bo_api.h>
>>>> #include <drm/drm_mm.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/spinlock.h>
>>>> -#include <linux/module.h>
>>>> /*
>>>> * Currently we use a spinlock for the lock, but a mutex *may* be
>>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>>> ttm_resource_manager *man,
>>>> struct ttm_resource *mem)
>>>> {
>>>> struct ttm_range_manager *rman = to_range_manager(man);
>>>> + struct ttm_range_mgr_node *node;
>>>> struct drm_mm *mm = &rman->mm;
>>>> - struct drm_mm_node *node;
>>>> enum drm_mm_insert_mode mode;
>>>> unsigned long lpfn;
>>>> int ret;
>>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>>> ttm_resource_manager *man,
>>>> if (!lpfn)
>>>> lpfn = man->size;
>>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>>
>>> I'm still a bit confused about the situation where a driver wants
>>> to attach private data to a struct ttm_resource without having to
>>> re-implement its own range manager?
>>>
>>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't work
>>> with the above unless we have a void *driver_private member on the
>>> struct ttm_resource. Is that the plan going forward here? Or that
>>> the driver actually does the re-implementation?
>>
>> I don't really understand your concern here. The basic idea is that
>> drivers use ttm_resource as a base class for their own implementation.
>>
>> See for example how nouveau does that:
>>
>> struct nouveau_mem {
>> struct ttm_resource base;
>> struct nouveau_cli *cli;
>> u8 kind;
>> u8 comp;
>> struct nvif_mem mem;
>> struct nvif_vma vma[2];
>> };
>>
>> The range manager is helping driver specific resource managers which
>> want to implement something drm_mm_nodes based. E.g. amdgpu_gtt_mgr
>> and amdgpu_vram_mgr, but it can also be used stand alone.
>>
>> The ttm_range_mgr_node can then be used as base class for this
>> functionality. I already want to move some more code from
>> amdgpu_vram_mgr.c into the range manager, but that is just minor
>> cleanup work.
>>
> Sure but if you embed a ttm_range_mgr_node in your struct
> i915_resource, and wanted to use the ttm range manager for it, it
> would allocate a struct ttm_range_mgr_node rather than a struct
> i915_resource? Or am I missing something?
Yes, that's the general idea I'm targeting for. I'm just not fully there
yet.
The ttm range manager then provides functions to implement debugging
and/or the iterator for example while the driver specific parts only
implement stuff like special placement handling.
Christian.
>
> /Thomas
>
>
>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks,
>>>
>>> Thomas
>>>
>>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 13:07 ` Christian König
@ 2021-06-02 14:13 ` Thomas Hellström (Intel)
2021-06-02 14:17 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-02 14:13 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/2/21 3:07 PM, Christian König wrote:
>
>
> Am 02.06.21 um 14:33 schrieb Thomas Hellström (Intel):
>>
>> On 6/2/21 2:11 PM, Christian König wrote:
>>> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>>>
>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>> Start with the range manager to make the resource object the base
>>>>> class for the allocated nodes.
>>>>>
>>>>> While at it cleanup a lot of the code around that.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>>>> ++++++++++++++++++-------
>>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>>>> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
>>>>> include/drm/ttm/ttm_resource.h | 3 ++
>>>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> index 69db89261650..df1f185faae9 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> @@ -45,6 +45,7 @@
>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>> #include <drm/ttm/ttm_placement.h>
>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>> #include <drm/amdgpu_drm.h>
>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>> @@ -17,6 +17,8 @@
>>>>> #include <drm/drm_prime.h>
>>>>> #include <drm/drm_simple_kms_helper.h>
>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>> +
>>>>> static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
>>>>> /**
>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>> index 65430912ff72..b08b8efeefba 100644
>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>> @@ -26,6 +26,8 @@
>>>>> #include <linux/limits.h>
>>>>> #include <linux/swiotlb.h>
>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>> +
>>>>> #include "nouveau_drv.h"
>>>>> #include "nouveau_gem.h"
>>>>> #include "nouveau_mem.h"
>>>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>> @@ -32,6 +32,7 @@
>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>> #include <drm/ttm/ttm_placement.h>
>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>> #include "qxl_drv.h"
>>>>> #include "qxl_object.h"
>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>> index cdffa9b65108..ad2a5a791bba 100644
>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>> @@ -45,6 +45,7 @@
>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>> #include <drm/ttm/ttm_placement.h>
>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>> #include "radeon_reg.h"
>>>>> #include "radeon.h"
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>> @@ -29,12 +29,13 @@
>>>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>> */
>>>>> -#include <drm/ttm/ttm_bo_driver.h>
>>>>> +#include <drm/ttm/ttm_device.h>
>>>>> #include <drm/ttm/ttm_placement.h>
>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>> +#include <drm/ttm/ttm_bo_api.h>
>>>>> #include <drm/drm_mm.h>
>>>>> #include <linux/slab.h>
>>>>> #include <linux/spinlock.h>
>>>>> -#include <linux/module.h>
>>>>> /*
>>>>> * Currently we use a spinlock for the lock, but a mutex *may* be
>>>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>>>> ttm_resource_manager *man,
>>>>> struct ttm_resource *mem)
>>>>> {
>>>>> struct ttm_range_manager *rman = to_range_manager(man);
>>>>> + struct ttm_range_mgr_node *node;
>>>>> struct drm_mm *mm = &rman->mm;
>>>>> - struct drm_mm_node *node;
>>>>> enum drm_mm_insert_mode mode;
>>>>> unsigned long lpfn;
>>>>> int ret;
>>>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>>>> ttm_resource_manager *man,
>>>>> if (!lpfn)
>>>>> lpfn = man->size;
>>>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>>>
>>>> I'm still a bit confused about the situation where a driver wants
>>>> to attach private data to a struct ttm_resource without having to
>>>> re-implement its own range manager?
>>>>
>>>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't work
>>>> with the above unless we have a void *driver_private member on the
>>>> struct ttm_resource. Is that the plan going forward here? Or that
>>>> the driver actually does the re-implementation?
>>>
>>> I don't really understand your concern here. The basic idea is that
>>> drivers use ttm_resource as a base class for their own implementation.
>>>
>>> See for example how nouveau does that:
>>>
>>> struct nouveau_mem {
>>> struct ttm_resource base;
>>> struct nouveau_cli *cli;
>>> u8 kind;
>>> u8 comp;
>>> struct nvif_mem mem;
>>> struct nvif_vma vma[2];
>>> };
>>>
>>> The range manager is helping driver specific resource managers which
>>> want to implement something drm_mm_nodes based. E.g. amdgpu_gtt_mgr
>>> and amdgpu_vram_mgr, but it can also be used stand alone.
>>>
>>> The ttm_range_mgr_node can then be used as base class for this
>>> functionality. I already want to move some more code from
>>> amdgpu_vram_mgr.c into the range manager, but that is just minor
>>> cleanup work.
>>>
>> Sure but if you embed a ttm_range_mgr_node in your struct
>> i915_resource, and wanted to use the ttm range manager for it, it
>> would allocate a struct ttm_range_mgr_node rather than a struct
>> i915_resource? Or am I missing something?
>
> Yes, that's the general idea I'm targeting for. I'm just not fully
> there yet.
Hmm, I don't fully understand the reply, I described a buggy scenario
and you replied that's what we're targeting for?
I assume you mean we're going to get an init() method for the range
manager, and a destroy method for the struct ttm_resource?
Thanks,
Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 14:13 ` Thomas Hellström (Intel)
@ 2021-06-02 14:17 ` Christian König
2021-06-02 15:28 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 14:17 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 02.06.21 um 16:13 schrieb Thomas Hellström (Intel):
>
> On 6/2/21 3:07 PM, Christian König wrote:
>>
>>
>> Am 02.06.21 um 14:33 schrieb Thomas Hellström (Intel):
>>>
>>> On 6/2/21 2:11 PM, Christian König wrote:
>>>> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>>>>
>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>>> Start with the range manager to make the resource object the base
>>>>>> class for the allocated nodes.
>>>>>>
>>>>>> While at it cleanup a lot of the code around that.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>>>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>>>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>>>>> ++++++++++++++++++-------
>>>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>>>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>>>>> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
>>>>>> include/drm/ttm/ttm_resource.h | 3 ++
>>>>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>>>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> index 69db89261650..df1f185faae9 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> @@ -45,6 +45,7 @@
>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>> #include <drm/amdgpu_drm.h>
>>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>> @@ -17,6 +17,8 @@
>>>>>> #include <drm/drm_prime.h>
>>>>>> #include <drm/drm_simple_kms_helper.h>
>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>> +
>>>>>> static const struct drm_gem_object_funcs
>>>>>> drm_gem_vram_object_funcs;
>>>>>> /**
>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>> index 65430912ff72..b08b8efeefba 100644
>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>> @@ -26,6 +26,8 @@
>>>>>> #include <linux/limits.h>
>>>>>> #include <linux/swiotlb.h>
>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>> +
>>>>>> #include "nouveau_drv.h"
>>>>>> #include "nouveau_gem.h"
>>>>>> #include "nouveau_mem.h"
>>>>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>>>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>> @@ -32,6 +32,7 @@
>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>> #include "qxl_drv.h"
>>>>>> #include "qxl_object.h"
>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>> index cdffa9b65108..ad2a5a791bba 100644
>>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>> @@ -45,6 +45,7 @@
>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>> #include "radeon_reg.h"
>>>>>> #include "radeon.h"
>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>> @@ -29,12 +29,13 @@
>>>>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>>> */
>>>>>> -#include <drm/ttm/ttm_bo_driver.h>
>>>>>> +#include <drm/ttm/ttm_device.h>
>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>> +#include <drm/ttm/ttm_bo_api.h>
>>>>>> #include <drm/drm_mm.h>
>>>>>> #include <linux/slab.h>
>>>>>> #include <linux/spinlock.h>
>>>>>> -#include <linux/module.h>
>>>>>> /*
>>>>>> * Currently we use a spinlock for the lock, but a mutex *may* be
>>>>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>>>>> ttm_resource_manager *man,
>>>>>> struct ttm_resource *mem)
>>>>>> {
>>>>>> struct ttm_range_manager *rman = to_range_manager(man);
>>>>>> + struct ttm_range_mgr_node *node;
>>>>>> struct drm_mm *mm = &rman->mm;
>>>>>> - struct drm_mm_node *node;
>>>>>> enum drm_mm_insert_mode mode;
>>>>>> unsigned long lpfn;
>>>>>> int ret;
>>>>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>>>>> ttm_resource_manager *man,
>>>>>> if (!lpfn)
>>>>>> lpfn = man->size;
>>>>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>>>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>>>>
>>>>> I'm still a bit confused about the situation where a driver wants
>>>>> to attach private data to a struct ttm_resource without having to
>>>>> re-implement its own range manager?
>>>>>
>>>>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't work
>>>>> with the above unless we have a void *driver_private member on the
>>>>> struct ttm_resource. Is that the plan going forward here? Or that
>>>>> the driver actually does the re-implementation?
>>>>
>>>> I don't really understand your concern here. The basic idea is that
>>>> drivers use ttm_resource as a base class for their own implementation.
>>>>
>>>> See for example how nouveau does that:
>>>>
>>>> struct nouveau_mem {
>>>> struct ttm_resource base;
>>>> struct nouveau_cli *cli;
>>>> u8 kind;
>>>> u8 comp;
>>>> struct nvif_mem mem;
>>>> struct nvif_vma vma[2];
>>>> };
>>>>
>>>> The range manager is helping driver specific resource managers
>>>> which want to implement something drm_mm_nodes based. E.g.
>>>> amdgpu_gtt_mgr and amdgpu_vram_mgr, but it can also be used stand
>>>> alone.
>>>>
>>>> The ttm_range_mgr_node can then be used as base class for this
>>>> functionality. I already want to move some more code from
>>>> amdgpu_vram_mgr.c into the range manager, but that is just minor
>>>> cleanup work.
>>>>
>>> Sure but if you embed a ttm_range_mgr_node in your struct
>>> i915_resource, and wanted to use the ttm range manager for it, it
>>> would allocate a struct ttm_range_mgr_node rather than a struct
>>> i915_resource? Or am I missing something?
>>
>> Yes, that's the general idea I'm targeting for. I'm just not fully
>> there yet.
>
> Hmm, I don't fully understand the reply, I described a buggy scenario
> and you replied that's what we're targeting for?
Ok, I don't seem to understand what you mean here. What is buggy on that?
> I assume you mean we're going to get an init() method for the range
> manager, and a destroy method for the struct ttm_resource?
Well the ttm_range_manager is just another component implementing some
functionality by extending the ttm_resource object.
We currently don't have a destroy function for ttm_resource object
because that isn't necessary at the moment. But I'm probably going to
add one at some point.
Regards,
Christian.
>
> Thanks,
>
> Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 14:17 ` Christian König
@ 2021-06-02 15:28 ` Thomas Hellström (Intel)
2021-06-02 18:41 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-02 15:28 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
Hi!
On 6/2/21 4:17 PM, Christian König wrote:
> Am 02.06.21 um 16:13 schrieb Thomas Hellström (Intel):
>>
>> On 6/2/21 3:07 PM, Christian König wrote:
>>>
>>>
>>> Am 02.06.21 um 14:33 schrieb Thomas Hellström (Intel):
>>>>
>>>> On 6/2/21 2:11 PM, Christian König wrote:
>>>>> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>>>>>
>>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>>>> Start with the range manager to make the resource object the base
>>>>>>> class for the allocated nodes.
>>>>>>>
>>>>>>> While at it cleanup a lot of the code around that.
>>>>>>>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>>>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>>>>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>>>>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>>>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>>>>>> ++++++++++++++++++-------
>>>>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>>>>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>>>>>> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
>>>>>>> include/drm/ttm/ttm_resource.h | 3 ++
>>>>>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>>>>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>> index 69db89261650..df1f185faae9 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>> @@ -45,6 +45,7 @@
>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>> #include <drm/amdgpu_drm.h>
>>>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>> @@ -17,6 +17,8 @@
>>>>>>> #include <drm/drm_prime.h>
>>>>>>> #include <drm/drm_simple_kms_helper.h>
>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>> +
>>>>>>> static const struct drm_gem_object_funcs
>>>>>>> drm_gem_vram_object_funcs;
>>>>>>> /**
>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>> index 65430912ff72..b08b8efeefba 100644
>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>> @@ -26,6 +26,8 @@
>>>>>>> #include <linux/limits.h>
>>>>>>> #include <linux/swiotlb.h>
>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>> +
>>>>>>> #include "nouveau_drv.h"
>>>>>>> #include "nouveau_gem.h"
>>>>>>> #include "nouveau_mem.h"
>>>>>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>>>>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>> @@ -32,6 +32,7 @@
>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>> #include "qxl_drv.h"
>>>>>>> #include "qxl_object.h"
>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>> index cdffa9b65108..ad2a5a791bba 100644
>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>> @@ -45,6 +45,7 @@
>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>> #include "radeon_reg.h"
>>>>>>> #include "radeon.h"
>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>> @@ -29,12 +29,13 @@
>>>>>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>>>> */
>>>>>>> -#include <drm/ttm/ttm_bo_driver.h>
>>>>>>> +#include <drm/ttm/ttm_device.h>
>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>> +#include <drm/ttm/ttm_bo_api.h>
>>>>>>> #include <drm/drm_mm.h>
>>>>>>> #include <linux/slab.h>
>>>>>>> #include <linux/spinlock.h>
>>>>>>> -#include <linux/module.h>
>>>>>>> /*
>>>>>>> * Currently we use a spinlock for the lock, but a mutex *may* be
>>>>>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>>>>>> ttm_resource_manager *man,
>>>>>>> struct ttm_resource *mem)
>>>>>>> {
>>>>>>> struct ttm_range_manager *rman = to_range_manager(man);
>>>>>>> + struct ttm_range_mgr_node *node;
>>>>>>> struct drm_mm *mm = &rman->mm;
>>>>>>> - struct drm_mm_node *node;
>>>>>>> enum drm_mm_insert_mode mode;
>>>>>>> unsigned long lpfn;
>>>>>>> int ret;
>>>>>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>>>>>> ttm_resource_manager *man,
>>>>>>> if (!lpfn)
>>>>>>> lpfn = man->size;
>>>>>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>>>>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>>>>>
>>>>>> I'm still a bit confused about the situation where a driver
>>>>>> wants to attach private data to a struct ttm_resource without
>>>>>> having to re-implement its own range manager?
>>>>>>
>>>>>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't
>>>>>> work with the above unless we have a void *driver_private member
>>>>>> on the struct ttm_resource. Is that the plan going forward here?
>>>>>> Or that the driver actually does the re-implementation?
>>>>>
>>>>> I don't really understand your concern here. The basic idea is
>>>>> that drivers use ttm_resource as a base class for their own
>>>>> implementation.
>>>>>
>>>>> See for example how nouveau does that:
>>>>>
>>>>> struct nouveau_mem {
>>>>> struct ttm_resource base;
>>>>> struct nouveau_cli *cli;
>>>>> u8 kind;
>>>>> u8 comp;
>>>>> struct nvif_mem mem;
>>>>> struct nvif_vma vma[2];
>>>>> };
>>>>>
>>>>> The range manager is helping driver specific resource managers
>>>>> which want to implement something drm_mm_nodes based. E.g.
>>>>> amdgpu_gtt_mgr and amdgpu_vram_mgr, but it can also be used stand
>>>>> alone.
>>>>>
>>>>> The ttm_range_mgr_node can then be used as base class for this
>>>>> functionality. I already want to move some more code from
>>>>> amdgpu_vram_mgr.c into the range manager, but that is just minor
>>>>> cleanup work.
>>>>>
>>>> Sure but if you embed a ttm_range_mgr_node in your struct
>>>> i915_resource, and wanted to use the ttm range manager for it, it
>>>> would allocate a struct ttm_range_mgr_node rather than a struct
>>>> i915_resource? Or am I missing something?
>>>
>>> Yes, that's the general idea I'm targeting for. I'm just not fully
>>> there yet.
>>
>> Hmm, I don't fully understand the reply, I described a buggy scenario
>> and you replied that's what we're targeting for?
>
> Ok, I don't seem to understand what you mean here. What is buggy on that?
The buggy thing I'm trying to describe is a scenario where I want to
have a struct i915_ttm_resource which embeds a struct
ttm_range_mgr_node, but there is no way I can tell the generic ttm range
manager to allocate a struct i915_ttm_resource instead of a struct
ttm_range_mgr_node.
So what I want to be able to do: I have
struct i915_ttm_resource {
struct i915_gpu_bindings gpu_bindings;
struct ttm_range_mgr_node range_node;
};
Now I want to be able to share common code as much as possible and use
the generic ttm_range_manager here. How would I go about doing that with
the proposed changes?
Thanks,
Thomas
>
>> I assume you mean we're going to get an init() method for the range
>> manager, and a destroy method for the struct ttm_resource?
>
> Well the ttm_range_manager is just another component implementing some
> functionality by extending the ttm_resource object.
>
> We currently don't have a destroy function for ttm_resource object
> because that isn't necessary at the moment. But I'm probably going to
> add one at some point.
>
> Regards,
> Christian.
>
>>
>> Thanks,
>>
>> Thomas
>>
>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 15:28 ` Thomas Hellström (Intel)
@ 2021-06-02 18:41 ` Christian König
2021-06-02 18:52 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-02 18:41 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 02.06.21 um 17:28 schrieb Thomas Hellström (Intel):
> Hi!
>
> On 6/2/21 4:17 PM, Christian König wrote:
>> Am 02.06.21 um 16:13 schrieb Thomas Hellström (Intel):
>>>
>>> On 6/2/21 3:07 PM, Christian König wrote:
>>>>
>>>>
>>>> Am 02.06.21 um 14:33 schrieb Thomas Hellström (Intel):
>>>>>
>>>>> On 6/2/21 2:11 PM, Christian König wrote:
>>>>>> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>>>>>>
>>>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>>>>> Start with the range manager to make the resource object the base
>>>>>>>> class for the allocated nodes.
>>>>>>>>
>>>>>>>> While at it cleanup a lot of the code around that.
>>>>>>>>
>>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>>>>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>>>>>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>>>>>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>>>>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>>>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>>>>>>> ++++++++++++++++++-------
>>>>>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>>>>>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>>>>>>> include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++
>>>>>>>> include/drm/ttm/ttm_resource.h | 3 ++
>>>>>>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>>>>>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> index 69db89261650..df1f185faae9 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> @@ -45,6 +45,7 @@
>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>> #include <drm/amdgpu_drm.h>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>> @@ -17,6 +17,8 @@
>>>>>>>> #include <drm/drm_prime.h>
>>>>>>>> #include <drm/drm_simple_kms_helper.h>
>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>> +
>>>>>>>> static const struct drm_gem_object_funcs
>>>>>>>> drm_gem_vram_object_funcs;
>>>>>>>> /**
>>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>> index 65430912ff72..b08b8efeefba 100644
>>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>> @@ -26,6 +26,8 @@
>>>>>>>> #include <linux/limits.h>
>>>>>>>> #include <linux/swiotlb.h>
>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>> +
>>>>>>>> #include "nouveau_drv.h"
>>>>>>>> #include "nouveau_gem.h"
>>>>>>>> #include "nouveau_mem.h"
>>>>>>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>>>>>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>> @@ -32,6 +32,7 @@
>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>> #include "qxl_drv.h"
>>>>>>>> #include "qxl_object.h"
>>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>> index cdffa9b65108..ad2a5a791bba 100644
>>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>> @@ -45,6 +45,7 @@
>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>> #include "radeon_reg.h"
>>>>>>>> #include "radeon.h"
>>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>> @@ -29,12 +29,13 @@
>>>>>>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>>>>> */
>>>>>>>> -#include <drm/ttm/ttm_bo_driver.h>
>>>>>>>> +#include <drm/ttm/ttm_device.h>
>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>> +#include <drm/ttm/ttm_bo_api.h>
>>>>>>>> #include <drm/drm_mm.h>
>>>>>>>> #include <linux/slab.h>
>>>>>>>> #include <linux/spinlock.h>
>>>>>>>> -#include <linux/module.h>
>>>>>>>> /*
>>>>>>>> * Currently we use a spinlock for the lock, but a mutex
>>>>>>>> *may* be
>>>>>>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>>>>>>> ttm_resource_manager *man,
>>>>>>>> struct ttm_resource *mem)
>>>>>>>> {
>>>>>>>> struct ttm_range_manager *rman = to_range_manager(man);
>>>>>>>> + struct ttm_range_mgr_node *node;
>>>>>>>> struct drm_mm *mm = &rman->mm;
>>>>>>>> - struct drm_mm_node *node;
>>>>>>>> enum drm_mm_insert_mode mode;
>>>>>>>> unsigned long lpfn;
>>>>>>>> int ret;
>>>>>>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>>>>>>> ttm_resource_manager *man,
>>>>>>>> if (!lpfn)
>>>>>>>> lpfn = man->size;
>>>>>>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>>>>>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>>>>>>
>>>>>>> I'm still a bit confused about the situation where a driver
>>>>>>> wants to attach private data to a struct ttm_resource without
>>>>>>> having to re-implement its own range manager?
>>>>>>>
>>>>>>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't
>>>>>>> work with the above unless we have a void *driver_private member
>>>>>>> on the struct ttm_resource. Is that the plan going forward here?
>>>>>>> Or that the driver actually does the re-implementation?
>>>>>>
>>>>>> I don't really understand your concern here. The basic idea is
>>>>>> that drivers use ttm_resource as a base class for their own
>>>>>> implementation.
>>>>>>
>>>>>> See for example how nouveau does that:
>>>>>>
>>>>>> struct nouveau_mem {
>>>>>> struct ttm_resource base;
>>>>>> struct nouveau_cli *cli;
>>>>>> u8 kind;
>>>>>> u8 comp;
>>>>>> struct nvif_mem mem;
>>>>>> struct nvif_vma vma[2];
>>>>>> };
>>>>>>
>>>>>> The range manager is helping driver specific resource managers
>>>>>> which want to implement something drm_mm_nodes based. E.g.
>>>>>> amdgpu_gtt_mgr and amdgpu_vram_mgr, but it can also be used stand
>>>>>> alone.
>>>>>>
>>>>>> The ttm_range_mgr_node can then be used as base class for this
>>>>>> functionality. I already want to move some more code from
>>>>>> amdgpu_vram_mgr.c into the range manager, but that is just minor
>>>>>> cleanup work.
>>>>>>
>>>>> Sure but if you embed a ttm_range_mgr_node in your struct
>>>>> i915_resource, and wanted to use the ttm range manager for it, it
>>>>> would allocate a struct ttm_range_mgr_node rather than a struct
>>>>> i915_resource? Or am I missing something?
>>>>
>>>> Yes, that's the general idea I'm targeting for. I'm just not fully
>>>> there yet.
>>>
>>> Hmm, I don't fully understand the reply, I described a buggy
>>> scenario and you replied that's what we're targeting for?
>>
>> Ok, I don't seem to understand what you mean here. What is buggy on
>> that?
>
> The buggy thing I'm trying to describe is a scenario where I want to
> have a struct i915_ttm_resource which embeds a struct
> ttm_range_mgr_node, but there is no way I can tell the generic ttm
> range manager to allocate a struct i915_ttm_resource instead of a
> struct ttm_range_mgr_node.
>
> So what I want to be able to do: I have
>
> struct i915_ttm_resource {
> struct i915_gpu_bindings gpu_bindings;
> struct ttm_range_mgr_node range_node;
> };
>
> Now I want to be able to share common code as much as possible and use
> the generic ttm_range_manager here. How would I go about doing that
> with the proposed changes?
Ah, yes that is the part I haven't moved over yet. In other words that
is not possible yet.
Christian.
>
> Thanks,
>
> Thomas
>
>
>
>
>
>
>
>
>
>
>>
>>> I assume you mean we're going to get an init() method for the range
>>> manager, and a destroy method for the struct ttm_resource?
>>
>> Well the ttm_range_manager is just another component implementing
>> some functionality by extending the ttm_resource object.
>>
>> We currently don't have a destroy function for ttm_resource object
>> because that isn't necessary at the moment. But I'm probably going to
>> add one at some point.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks,
>>>
>>> Thomas
>>>
>>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 18:41 ` Christian König
@ 2021-06-02 18:52 ` Thomas Hellström (Intel)
2021-06-02 18:53 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-02 18:52 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/2/21 8:41 PM, Christian König wrote:
> Am 02.06.21 um 17:28 schrieb Thomas Hellström (Intel):
>> Hi!
>>
>> On 6/2/21 4:17 PM, Christian König wrote:
>>> Am 02.06.21 um 16:13 schrieb Thomas Hellström (Intel):
>>>>
>>>> On 6/2/21 3:07 PM, Christian König wrote:
>>>>>
>>>>>
>>>>> Am 02.06.21 um 14:33 schrieb Thomas Hellström (Intel):
>>>>>>
>>>>>> On 6/2/21 2:11 PM, Christian König wrote:
>>>>>>> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>>>>>>>
>>>>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>>>>>> Start with the range manager to make the resource object the base
>>>>>>>>> class for the allocated nodes.
>>>>>>>>>
>>>>>>>>> While at it cleanup a lot of the code around that.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>>>>>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>>>>>>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>>>>>>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>>>>>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>>>>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>>>>>>>> ++++++++++++++++++-------
>>>>>>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>>>>>>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>>>>>>>> include/drm/ttm/ttm_range_manager.h | 43
>>>>>>>>> +++++++++++++++++++
>>>>>>>>> include/drm/ttm/ttm_resource.h | 3 ++
>>>>>>>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>>>>>>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>> index 69db89261650..df1f185faae9 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>> @@ -45,6 +45,7 @@
>>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>> #include <drm/amdgpu_drm.h>
>>>>>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>>>>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>> @@ -17,6 +17,8 @@
>>>>>>>>> #include <drm/drm_prime.h>
>>>>>>>>> #include <drm/drm_simple_kms_helper.h>
>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>> +
>>>>>>>>> static const struct drm_gem_object_funcs
>>>>>>>>> drm_gem_vram_object_funcs;
>>>>>>>>> /**
>>>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>> index 65430912ff72..b08b8efeefba 100644
>>>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>> @@ -26,6 +26,8 @@
>>>>>>>>> #include <linux/limits.h>
>>>>>>>>> #include <linux/swiotlb.h>
>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>> +
>>>>>>>>> #include "nouveau_drv.h"
>>>>>>>>> #include "nouveau_gem.h"
>>>>>>>>> #include "nouveau_mem.h"
>>>>>>>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>>>>>>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>> @@ -32,6 +32,7 @@
>>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>> #include "qxl_drv.h"
>>>>>>>>> #include "qxl_object.h"
>>>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>> index cdffa9b65108..ad2a5a791bba 100644
>>>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>> @@ -45,6 +45,7 @@
>>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>> #include "radeon_reg.h"
>>>>>>>>> #include "radeon.h"
>>>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>> @@ -29,12 +29,13 @@
>>>>>>>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>>>>>> */
>>>>>>>>> -#include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>> +#include <drm/ttm/ttm_device.h>
>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>> +#include <drm/ttm/ttm_bo_api.h>
>>>>>>>>> #include <drm/drm_mm.h>
>>>>>>>>> #include <linux/slab.h>
>>>>>>>>> #include <linux/spinlock.h>
>>>>>>>>> -#include <linux/module.h>
>>>>>>>>> /*
>>>>>>>>> * Currently we use a spinlock for the lock, but a mutex
>>>>>>>>> *may* be
>>>>>>>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>>>>>>>> ttm_resource_manager *man,
>>>>>>>>> struct ttm_resource *mem)
>>>>>>>>> {
>>>>>>>>> struct ttm_range_manager *rman = to_range_manager(man);
>>>>>>>>> + struct ttm_range_mgr_node *node;
>>>>>>>>> struct drm_mm *mm = &rman->mm;
>>>>>>>>> - struct drm_mm_node *node;
>>>>>>>>> enum drm_mm_insert_mode mode;
>>>>>>>>> unsigned long lpfn;
>>>>>>>>> int ret;
>>>>>>>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>>>>>>>> ttm_resource_manager *man,
>>>>>>>>> if (!lpfn)
>>>>>>>>> lpfn = man->size;
>>>>>>>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>>>>>>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>>>>>>>
>>>>>>>> I'm still a bit confused about the situation where a driver
>>>>>>>> wants to attach private data to a struct ttm_resource without
>>>>>>>> having to re-implement its own range manager?
>>>>>>>>
>>>>>>>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't
>>>>>>>> work with the above unless we have a void *driver_private
>>>>>>>> member on the struct ttm_resource. Is that the plan going
>>>>>>>> forward here? Or that the driver actually does the
>>>>>>>> re-implementation?
>>>>>>>
>>>>>>> I don't really understand your concern here. The basic idea is
>>>>>>> that drivers use ttm_resource as a base class for their own
>>>>>>> implementation.
>>>>>>>
>>>>>>> See for example how nouveau does that:
>>>>>>>
>>>>>>> struct nouveau_mem {
>>>>>>> struct ttm_resource base;
>>>>>>> struct nouveau_cli *cli;
>>>>>>> u8 kind;
>>>>>>> u8 comp;
>>>>>>> struct nvif_mem mem;
>>>>>>> struct nvif_vma vma[2];
>>>>>>> };
>>>>>>>
>>>>>>> The range manager is helping driver specific resource managers
>>>>>>> which want to implement something drm_mm_nodes based. E.g.
>>>>>>> amdgpu_gtt_mgr and amdgpu_vram_mgr, but it can also be used
>>>>>>> stand alone.
>>>>>>>
>>>>>>> The ttm_range_mgr_node can then be used as base class for this
>>>>>>> functionality. I already want to move some more code from
>>>>>>> amdgpu_vram_mgr.c into the range manager, but that is just minor
>>>>>>> cleanup work.
>>>>>>>
>>>>>> Sure but if you embed a ttm_range_mgr_node in your struct
>>>>>> i915_resource, and wanted to use the ttm range manager for it, it
>>>>>> would allocate a struct ttm_range_mgr_node rather than a struct
>>>>>> i915_resource? Or am I missing something?
>>>>>
>>>>> Yes, that's the general idea I'm targeting for. I'm just not fully
>>>>> there yet.
>>>>
>>>> Hmm, I don't fully understand the reply, I described a buggy
>>>> scenario and you replied that's what we're targeting for?
>>>
>>> Ok, I don't seem to understand what you mean here. What is buggy on
>>> that?
>>
>> The buggy thing I'm trying to describe is a scenario where I want to
>> have a struct i915_ttm_resource which embeds a struct
>> ttm_range_mgr_node, but there is no way I can tell the generic ttm
>> range manager to allocate a struct i915_ttm_resource instead of a
>> struct ttm_range_mgr_node.
>>
>> So what I want to be able to do: I have
>>
>> struct i915_ttm_resource {
>> struct i915_gpu_bindings gpu_bindings;
>> struct ttm_range_mgr_node range_node;
>> };
>>
>> Now I want to be able to share common code as much as possible and
>> use the generic ttm_range_manager here. How would I go about doing
>> that with the proposed changes?
>
> Ah, yes that is the part I haven't moved over yet. In other words that
> is not possible yet.
OK, that "yet" sounds good. So this will be possible moving forward?
(Basically it's the overall design that's not completely clear to me
yet, not really the code itself)
Thanks,
Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes
2021-06-02 18:52 ` Thomas Hellström (Intel)
@ 2021-06-02 18:53 ` Christian König
0 siblings, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-02 18:53 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 02.06.21 um 20:52 schrieb Thomas Hellström (Intel):
>
> On 6/2/21 8:41 PM, Christian König wrote:
>> Am 02.06.21 um 17:28 schrieb Thomas Hellström (Intel):
>>> Hi!
>>>
>>> On 6/2/21 4:17 PM, Christian König wrote:
>>>> Am 02.06.21 um 16:13 schrieb Thomas Hellström (Intel):
>>>>>
>>>>> On 6/2/21 3:07 PM, Christian König wrote:
>>>>>>
>>>>>>
>>>>>> Am 02.06.21 um 14:33 schrieb Thomas Hellström (Intel):
>>>>>>>
>>>>>>> On 6/2/21 2:11 PM, Christian König wrote:
>>>>>>>> Am 02.06.21 um 13:44 schrieb Thomas Hellström (Intel):
>>>>>>>>>
>>>>>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>>>>>>> Start with the range manager to make the resource object the
>>>>>>>>>> base
>>>>>>>>>> class for the allocated nodes.
>>>>>>>>>>
>>>>>>>>>> While at it cleanup a lot of the code around that.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>>>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>>>>>>>>> ---
>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>>>>>>>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +
>>>>>>>>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +
>>>>>>>>>> drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
>>>>>>>>>> drivers/gpu/drm/radeon/radeon_ttm.c | 1 +
>>>>>>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 56
>>>>>>>>>> ++++++++++++++++++-------
>>>>>>>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++----
>>>>>>>>>> include/drm/ttm/ttm_bo_driver.h | 26 ------------
>>>>>>>>>> include/drm/ttm/ttm_range_manager.h | 43
>>>>>>>>>> +++++++++++++++++++
>>>>>>>>>> include/drm/ttm/ttm_resource.h | 3 ++
>>>>>>>>>> 10 files changed, 111 insertions(+), 50 deletions(-)
>>>>>>>>>> create mode 100644 include/drm/ttm/ttm_range_manager.h
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>>> index 69db89261650..df1f185faae9 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>>>> @@ -45,6 +45,7 @@
>>>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>>> #include <drm/amdgpu_drm.h>
>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>>> index 83e7258c7f90..17a4c5d47b6a 100644
>>>>>>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>>>>>>> @@ -17,6 +17,8 @@
>>>>>>>>>> #include <drm/drm_prime.h>
>>>>>>>>>> #include <drm/drm_simple_kms_helper.h>
>>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>>> +
>>>>>>>>>> static const struct drm_gem_object_funcs
>>>>>>>>>> drm_gem_vram_object_funcs;
>>>>>>>>>> /**
>>>>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>>> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>>> index 65430912ff72..b08b8efeefba 100644
>>>>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
>>>>>>>>>> @@ -26,6 +26,8 @@
>>>>>>>>>> #include <linux/limits.h>
>>>>>>>>>> #include <linux/swiotlb.h>
>>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>>> +
>>>>>>>>>> #include "nouveau_drv.h"
>>>>>>>>>> #include "nouveau_gem.h"
>>>>>>>>>> #include "nouveau_mem.h"
>>>>>>>>>> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>>> b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>>> index 8aa87b8edb9c..19fd39d9a00c 100644
>>>>>>>>>> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
>>>>>>>>>> @@ -32,6 +32,7 @@
>>>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>>> #include "qxl_drv.h"
>>>>>>>>>> #include "qxl_object.h"
>>>>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>>> index cdffa9b65108..ad2a5a791bba 100644
>>>>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>>>>>> @@ -45,6 +45,7 @@
>>>>>>>>>> #include <drm/ttm/ttm_bo_api.h>
>>>>>>>>>> #include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>>> #include "radeon_reg.h"
>>>>>>>>>> #include "radeon.h"
>>>>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>>> index b9d5da6e6a81..ce5d07ca384c 100644
>>>>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>>>>> @@ -29,12 +29,13 @@
>>>>>>>>>> * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>>>>>>> */
>>>>>>>>>> -#include <drm/ttm/ttm_bo_driver.h>
>>>>>>>>>> +#include <drm/ttm/ttm_device.h>
>>>>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>>>>> +#include <drm/ttm/ttm_range_manager.h>
>>>>>>>>>> +#include <drm/ttm/ttm_bo_api.h>
>>>>>>>>>> #include <drm/drm_mm.h>
>>>>>>>>>> #include <linux/slab.h>
>>>>>>>>>> #include <linux/spinlock.h>
>>>>>>>>>> -#include <linux/module.h>
>>>>>>>>>> /*
>>>>>>>>>> * Currently we use a spinlock for the lock, but a mutex
>>>>>>>>>> *may* be
>>>>>>>>>> @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct
>>>>>>>>>> ttm_resource_manager *man,
>>>>>>>>>> struct ttm_resource *mem)
>>>>>>>>>> {
>>>>>>>>>> struct ttm_range_manager *rman = to_range_manager(man);
>>>>>>>>>> + struct ttm_range_mgr_node *node;
>>>>>>>>>> struct drm_mm *mm = &rman->mm;
>>>>>>>>>> - struct drm_mm_node *node;
>>>>>>>>>> enum drm_mm_insert_mode mode;
>>>>>>>>>> unsigned long lpfn;
>>>>>>>>>> int ret;
>>>>>>>>>> @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct
>>>>>>>>>> ttm_resource_manager *man,
>>>>>>>>>> if (!lpfn)
>>>>>>>>>> lpfn = man->size;
>>>>>>>>>> - node = kzalloc(sizeof(*node), GFP_KERNEL);
>>>>>>>>>> + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL);
>>>>>>>>>
>>>>>>>>> I'm still a bit confused about the situation where a driver
>>>>>>>>> wants to attach private data to a struct ttm_resource without
>>>>>>>>> having to re-implement its own range manager?
>>>>>>>>>
>>>>>>>>> Could be cached sg-tables, list of GPU bindings etc. Wouldn't
>>>>>>>>> work with the above unless we have a void *driver_private
>>>>>>>>> member on the struct ttm_resource. Is that the plan going
>>>>>>>>> forward here? Or that the driver actually does the
>>>>>>>>> re-implementation?
>>>>>>>>
>>>>>>>> I don't really understand your concern here. The basic idea is
>>>>>>>> that drivers use ttm_resource as a base class for their own
>>>>>>>> implementation.
>>>>>>>>
>>>>>>>> See for example how nouveau does that:
>>>>>>>>
>>>>>>>> struct nouveau_mem {
>>>>>>>> struct ttm_resource base;
>>>>>>>> struct nouveau_cli *cli;
>>>>>>>> u8 kind;
>>>>>>>> u8 comp;
>>>>>>>> struct nvif_mem mem;
>>>>>>>> struct nvif_vma vma[2];
>>>>>>>> };
>>>>>>>>
>>>>>>>> The range manager is helping driver specific resource managers
>>>>>>>> which want to implement something drm_mm_nodes based. E.g.
>>>>>>>> amdgpu_gtt_mgr and amdgpu_vram_mgr, but it can also be used
>>>>>>>> stand alone.
>>>>>>>>
>>>>>>>> The ttm_range_mgr_node can then be used as base class for this
>>>>>>>> functionality. I already want to move some more code from
>>>>>>>> amdgpu_vram_mgr.c into the range manager, but that is just
>>>>>>>> minor cleanup work.
>>>>>>>>
>>>>>>> Sure but if you embed a ttm_range_mgr_node in your struct
>>>>>>> i915_resource, and wanted to use the ttm range manager for it,
>>>>>>> it would allocate a struct ttm_range_mgr_node rather than a
>>>>>>> struct i915_resource? Or am I missing something?
>>>>>>
>>>>>> Yes, that's the general idea I'm targeting for. I'm just not
>>>>>> fully there yet.
>>>>>
>>>>> Hmm, I don't fully understand the reply, I described a buggy
>>>>> scenario and you replied that's what we're targeting for?
>>>>
>>>> Ok, I don't seem to understand what you mean here. What is buggy on
>>>> that?
>>>
>>> The buggy thing I'm trying to describe is a scenario where I want to
>>> have a struct i915_ttm_resource which embeds a struct
>>> ttm_range_mgr_node, but there is no way I can tell the generic ttm
>>> range manager to allocate a struct i915_ttm_resource instead of a
>>> struct ttm_range_mgr_node.
>>>
>>> So what I want to be able to do: I have
>>>
>>> struct i915_ttm_resource {
>>> struct i915_gpu_bindings gpu_bindings;
>>> struct ttm_range_mgr_node range_node;
>>> };
>>>
>>> Now I want to be able to share common code as much as possible and
>>> use the generic ttm_range_manager here. How would I go about doing
>>> that with the proposed changes?
>>
>> Ah, yes that is the part I haven't moved over yet. In other words
>> that is not possible yet.
>
> OK, that "yet" sounds good. So this will be possible moving forward?
> (Basically it's the overall design that's not completely clear to me
> yet, not really the code itself)
Yes, absolutely.
Christian.
>
> Thanks,
>
> Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 03/10] drm/ttm: flip over the sys manager to self allocated nodes
2021-06-02 10:09 ` [PATCH 03/10] drm/ttm: flip over the sys " Christian König
@ 2021-06-03 7:51 ` Matthew Auld
0 siblings, 0 replies; 43+ messages in thread
From: Matthew Auld @ 2021-06-03 7:51 UTC (permalink / raw)
To: Christian König, thomas_os, dri-devel
On 02/06/2021 11:09, Christian König wrote:
> Make sure to allocate a resource object here.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (8 preceding siblings ...)
2021-06-02 10:09 ` [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2 Christian König
@ 2021-06-03 8:45 ` Matthew Auld
2021-06-04 11:54 ` Christian König
2021-06-04 9:33 ` Thomas Hellström (Intel)
` (2 subsequent siblings)
12 siblings, 1 reply; 43+ messages in thread
From: Matthew Auld @ 2021-06-03 8:45 UTC (permalink / raw)
To: Christian König, thomas_os, dri-devel
On 02/06/2021 11:09, Christian König wrote:
> To improve the handling we want the establish the resource object as base
> class for the backend allocations.
>
> v2: add missing error handling
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 54 +++++++-------
> drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +-
> drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 83 ++++++++--------------
> drivers/gpu/drm/ttm/ttm_bo_util.c | 43 ++++++-----
> drivers/gpu/drm/ttm/ttm_resource.c | 31 +++++---
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 2 +-
> include/drm/ttm/ttm_bo_api.h | 1 -
> include/drm/ttm/ttm_bo_driver.h | 10 ++-
> include/drm/ttm/ttm_resource.h | 4 +-
> 11 files changed, 110 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 03c6b63d1d54..59723c3d5826 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -362,14 +362,14 @@ int amdgpu_bo_create_kernel_at(struct amdgpu_device *adev,
> if (cpu_addr)
> amdgpu_bo_kunmap(*bo_ptr);
>
> - ttm_resource_free(&(*bo_ptr)->tbo, (*bo_ptr)->tbo.resource);
> + ttm_resource_free(&(*bo_ptr)->tbo, &(*bo_ptr)->tbo.resource);
>
> for (i = 0; i < (*bo_ptr)->placement.num_placement; ++i) {
> (*bo_ptr)->placements[i].fpfn = offset >> PAGE_SHIFT;
> (*bo_ptr)->placements[i].lpfn = (offset + size) >> PAGE_SHIFT;
> }
> r = ttm_bo_mem_space(&(*bo_ptr)->tbo, &(*bo_ptr)->placement,
> - (*bo_ptr)->tbo.resource, &ctx);
> + &(*bo_ptr)->tbo.resource, &ctx);
> if (r)
> goto error;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 663aa7d2e2ea..69db89261650 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -491,7 +491,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> return r;
>
> amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_mem);
> goto out;
> }
> @@ -950,9 +950,9 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
> struct ttm_operation_ctx ctx = { false, false };
> struct amdgpu_ttm_tt *gtt = (void *)bo->ttm;
> - struct ttm_resource tmp;
> struct ttm_placement placement;
> struct ttm_place placements;
> + struct ttm_resource *tmp;
> uint64_t addr, flags;
> int r;
>
> @@ -962,37 +962,37 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
> addr = amdgpu_gmc_agp_addr(bo);
> if (addr != AMDGPU_BO_INVALID_OFFSET) {
> bo->resource->start = addr >> PAGE_SHIFT;
> - } else {
> + return 0;
> + }
>
> - /* allocate GART space */
> - placement.num_placement = 1;
> - placement.placement = &placements;
> - placement.num_busy_placement = 1;
> - placement.busy_placement = &placements;
> - placements.fpfn = 0;
> - placements.lpfn = adev->gmc.gart_size >> PAGE_SHIFT;
> - placements.mem_type = TTM_PL_TT;
> - placements.flags = bo->resource->placement;
> -
> - r = ttm_bo_mem_space(bo, &placement, &tmp, &ctx);
> - if (unlikely(r))
> - return r;
> + /* allocate GART space */
> + placement.num_placement = 1;
> + placement.placement = &placements;
> + placement.num_busy_placement = 1;
> + placement.busy_placement = &placements;
> + placements.fpfn = 0;
> + placements.lpfn = adev->gmc.gart_size >> PAGE_SHIFT;
> + placements.mem_type = TTM_PL_TT;
> + placements.flags = bo->resource->placement;
>
> - /* compute PTE flags for this buffer object */
> - flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, &tmp);
> + r = ttm_bo_mem_space(bo, &placement, &tmp, &ctx);
> + if (unlikely(r))
> + return r;
>
> - /* Bind pages */
> - gtt->offset = (u64)tmp.start << PAGE_SHIFT;
> - r = amdgpu_ttm_gart_bind(adev, bo, flags);
> - if (unlikely(r)) {
> - ttm_resource_free(bo, &tmp);
> - return r;
> - }
> + /* compute PTE flags for this buffer object */
> + flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, tmp);
>
> - ttm_resource_free(bo, bo->resource);
> - ttm_bo_assign_mem(bo, &tmp);
> + /* Bind pages */
> + gtt->offset = (u64)tmp->start << PAGE_SHIFT;
> + r = amdgpu_ttm_gart_bind(adev, bo, flags);
> + if (unlikely(r)) {
> + ttm_resource_free(bo, &tmp);
> + return r;
> }
>
> + ttm_resource_free(bo, &bo->resource);
> + ttm_bo_assign_mem(bo, tmp);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index e688ca77483d..3a0d9b3bf991 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1009,7 +1009,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
> if (old_reg->mem_type == TTM_PL_TT &&
> new_reg->mem_type == TTM_PL_SYSTEM) {
> nouveau_ttm_tt_unbind(bo->bdev, bo->ttm);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_reg);
> goto out;
> }
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 2507c1741681..cdffa9b65108 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -229,7 +229,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
> if (old_mem->mem_type == TTM_PL_TT &&
> new_mem->mem_type == TTM_PL_SYSTEM) {
> radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_mem);
> goto out;
> }
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 5a7ab4b35b2d..4ed56520b81d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -223,7 +223,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
> bo->bdev->funcs->delete_mem_notify(bo);
>
> ttm_bo_tt_destroy(bo);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> }
>
> static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
> @@ -489,7 +489,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> struct ttm_operation_ctx *ctx)
> {
> struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource evict_mem;
> + struct ttm_resource *evict_mem;
> struct ttm_placement placement;
> struct ttm_place hop;
> int ret = 0;
> @@ -519,7 +519,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> goto out;
> }
>
> - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, &hop);
> + 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)
> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
> */
> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> - struct ttm_resource *mem,
> + struct ttm_resource **mem,
> struct ttm_operation_ctx *ctx)
> {
> struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource_manager *man = ttm_manager_type(bdev, mem->mem_type);
> + struct ttm_resource_manager *man;
> struct ww_acquire_ctx *ticket;
> int ret;
>
> + man = ttm_manager_type(bdev, (*mem)->mem_type);
> ticket = dma_resv_locking_ctx(bo->base.resv);
> do {
> ret = ttm_resource_alloc(bo, place, mem);
> @@ -749,37 +750,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
> return ret;
> } while (1);
>
> - return ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu);
> -}
> -
> -/**
> - * ttm_bo_mem_placement - check if placement is compatible
> - * @bo: BO to find memory for
> - * @place: where to search
> - * @mem: the memory object to fill in
> - *
> - * Check if placement is compatible and fill in mem structure.
> - * Returns -EBUSY if placement won't work or negative error code.
> - * 0 when placement can be used.
> - */
> -static int ttm_bo_mem_placement(struct ttm_buffer_object *bo,
> - const struct ttm_place *place,
> - struct ttm_resource *mem)
> -{
> - struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource_manager *man;
> -
> - man = ttm_manager_type(bdev, place->mem_type);
> - if (!man || !ttm_resource_manager_used(man))
> - return -EBUSY;
> -
> - mem->mem_type = place->mem_type;
> - mem->placement = place->flags;
> -
> - spin_lock(&bo->bdev->lru_lock);
> - ttm_bo_move_to_lru_tail(bo, mem, NULL);
> - spin_unlock(&bo->bdev->lru_lock);
Why do we drop the move_to_lru_tail here?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 05/10] drm/amdkfd: use resource cursor in svm_migrate_copy_to_vram v2
2021-06-02 10:09 ` [PATCH 05/10] drm/amdkfd: use resource cursor in svm_migrate_copy_to_vram v2 Christian König
@ 2021-06-03 9:44 ` Matthew Auld
0 siblings, 0 replies; 43+ messages in thread
From: Matthew Auld @ 2021-06-03 9:44 UTC (permalink / raw)
To: Christian König, thomas_os, dri-devel
On 02/06/2021 11:09, Christian König wrote:
> Access to the mm_node is now forbidden. So instead of hand wiring that
> use the cursor functionality.
>
> v2: fix handling as pointed out by Philip.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (9 preceding siblings ...)
2021-06-03 8:45 ` [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Matthew Auld
@ 2021-06-04 9:33 ` Thomas Hellström (Intel)
2021-06-07 16:40 ` Thomas Hellström (Intel)
2021-06-08 6:55 ` Thomas Hellström
12 siblings, 0 replies; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-04 9:33 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
Hi, Christian,
It looks like all patches in the series have been reviewed or acked by
Matthew,
and while still a little worried about the final outcome of embedding a
struct ttm_mem_resource, FWIW,
Acked-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
/Thomas
On 6/2/21 12:09 PM, Christian König wrote:
> To improve the handling we want the establish the resource object as base
> class for the backend allocations.
>
> v2: add missing error handling
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 54 +++++++-------
> drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +-
> drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 83 ++++++++--------------
> drivers/gpu/drm/ttm/ttm_bo_util.c | 43 ++++++-----
> drivers/gpu/drm/ttm/ttm_resource.c | 31 +++++---
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 2 +-
> include/drm/ttm/ttm_bo_api.h | 1 -
> include/drm/ttm/ttm_bo_driver.h | 10 ++-
> include/drm/ttm/ttm_resource.h | 4 +-
> 11 files changed, 110 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 03c6b63d1d54..59723c3d5826 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -362,14 +362,14 @@ int amdgpu_bo_create_kernel_at(struct amdgpu_device *adev,
> if (cpu_addr)
> amdgpu_bo_kunmap(*bo_ptr);
>
> - ttm_resource_free(&(*bo_ptr)->tbo, (*bo_ptr)->tbo.resource);
> + ttm_resource_free(&(*bo_ptr)->tbo, &(*bo_ptr)->tbo.resource);
>
> for (i = 0; i < (*bo_ptr)->placement.num_placement; ++i) {
> (*bo_ptr)->placements[i].fpfn = offset >> PAGE_SHIFT;
> (*bo_ptr)->placements[i].lpfn = (offset + size) >> PAGE_SHIFT;
> }
> r = ttm_bo_mem_space(&(*bo_ptr)->tbo, &(*bo_ptr)->placement,
> - (*bo_ptr)->tbo.resource, &ctx);
> + &(*bo_ptr)->tbo.resource, &ctx);
> if (r)
> goto error;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 663aa7d2e2ea..69db89261650 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -491,7 +491,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> return r;
>
> amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_mem);
> goto out;
> }
> @@ -950,9 +950,9 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
> struct ttm_operation_ctx ctx = { false, false };
> struct amdgpu_ttm_tt *gtt = (void *)bo->ttm;
> - struct ttm_resource tmp;
> struct ttm_placement placement;
> struct ttm_place placements;
> + struct ttm_resource *tmp;
> uint64_t addr, flags;
> int r;
>
> @@ -962,37 +962,37 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
> addr = amdgpu_gmc_agp_addr(bo);
> if (addr != AMDGPU_BO_INVALID_OFFSET) {
> bo->resource->start = addr >> PAGE_SHIFT;
> - } else {
> + return 0;
> + }
>
> - /* allocate GART space */
> - placement.num_placement = 1;
> - placement.placement = &placements;
> - placement.num_busy_placement = 1;
> - placement.busy_placement = &placements;
> - placements.fpfn = 0;
> - placements.lpfn = adev->gmc.gart_size >> PAGE_SHIFT;
> - placements.mem_type = TTM_PL_TT;
> - placements.flags = bo->resource->placement;
> -
> - r = ttm_bo_mem_space(bo, &placement, &tmp, &ctx);
> - if (unlikely(r))
> - return r;
> + /* allocate GART space */
> + placement.num_placement = 1;
> + placement.placement = &placements;
> + placement.num_busy_placement = 1;
> + placement.busy_placement = &placements;
> + placements.fpfn = 0;
> + placements.lpfn = adev->gmc.gart_size >> PAGE_SHIFT;
> + placements.mem_type = TTM_PL_TT;
> + placements.flags = bo->resource->placement;
>
> - /* compute PTE flags for this buffer object */
> - flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, &tmp);
> + r = ttm_bo_mem_space(bo, &placement, &tmp, &ctx);
> + if (unlikely(r))
> + return r;
>
> - /* Bind pages */
> - gtt->offset = (u64)tmp.start << PAGE_SHIFT;
> - r = amdgpu_ttm_gart_bind(adev, bo, flags);
> - if (unlikely(r)) {
> - ttm_resource_free(bo, &tmp);
> - return r;
> - }
> + /* compute PTE flags for this buffer object */
> + flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, tmp);
>
> - ttm_resource_free(bo, bo->resource);
> - ttm_bo_assign_mem(bo, &tmp);
> + /* Bind pages */
> + gtt->offset = (u64)tmp->start << PAGE_SHIFT;
> + r = amdgpu_ttm_gart_bind(adev, bo, flags);
> + if (unlikely(r)) {
> + ttm_resource_free(bo, &tmp);
> + return r;
> }
>
> + ttm_resource_free(bo, &bo->resource);
> + ttm_bo_assign_mem(bo, tmp);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index e688ca77483d..3a0d9b3bf991 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1009,7 +1009,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
> if (old_reg->mem_type == TTM_PL_TT &&
> new_reg->mem_type == TTM_PL_SYSTEM) {
> nouveau_ttm_tt_unbind(bo->bdev, bo->ttm);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_reg);
> goto out;
> }
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 2507c1741681..cdffa9b65108 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -229,7 +229,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
> if (old_mem->mem_type == TTM_PL_TT &&
> new_mem->mem_type == TTM_PL_SYSTEM) {
> radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_mem);
> goto out;
> }
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 5a7ab4b35b2d..4ed56520b81d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -223,7 +223,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
> bo->bdev->funcs->delete_mem_notify(bo);
>
> ttm_bo_tt_destroy(bo);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> }
>
> static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
> @@ -489,7 +489,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> struct ttm_operation_ctx *ctx)
> {
> struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource evict_mem;
> + struct ttm_resource *evict_mem;
> struct ttm_placement placement;
> struct ttm_place hop;
> int ret = 0;
> @@ -519,7 +519,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> goto out;
> }
>
> - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx, &hop);
> + 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)
> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
> */
> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> - struct ttm_resource *mem,
> + struct ttm_resource **mem,
> struct ttm_operation_ctx *ctx)
> {
> struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource_manager *man = ttm_manager_type(bdev, mem->mem_type);
> + struct ttm_resource_manager *man;
> struct ww_acquire_ctx *ticket;
> int ret;
>
> + man = ttm_manager_type(bdev, (*mem)->mem_type);
> ticket = dma_resv_locking_ctx(bo->base.resv);
> do {
> ret = ttm_resource_alloc(bo, place, mem);
> @@ -749,37 +750,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
> return ret;
> } while (1);
>
> - return ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu);
> -}
> -
> -/**
> - * ttm_bo_mem_placement - check if placement is compatible
> - * @bo: BO to find memory for
> - * @place: where to search
> - * @mem: the memory object to fill in
> - *
> - * Check if placement is compatible and fill in mem structure.
> - * Returns -EBUSY if placement won't work or negative error code.
> - * 0 when placement can be used.
> - */
> -static int ttm_bo_mem_placement(struct ttm_buffer_object *bo,
> - const struct ttm_place *place,
> - struct ttm_resource *mem)
> -{
> - struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource_manager *man;
> -
> - man = ttm_manager_type(bdev, place->mem_type);
> - if (!man || !ttm_resource_manager_used(man))
> - return -EBUSY;
> -
> - mem->mem_type = place->mem_type;
> - mem->placement = place->flags;
> -
> - spin_lock(&bo->bdev->lru_lock);
> - ttm_bo_move_to_lru_tail(bo, mem, NULL);
> - spin_unlock(&bo->bdev->lru_lock);
> - return 0;
> + return ttm_bo_add_move_fence(bo, man, *mem, ctx->no_wait_gpu);
> }
>
> /*
> @@ -792,7 +763,7 @@ static int ttm_bo_mem_placement(struct ttm_buffer_object *bo,
> */
> int ttm_bo_mem_space(struct ttm_buffer_object *bo,
> struct ttm_placement *placement,
> - struct ttm_resource *mem,
> + struct ttm_resource **mem,
> struct ttm_operation_ctx *ctx)
> {
> struct ttm_device *bdev = bo->bdev;
> @@ -807,8 +778,8 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
> const struct ttm_place *place = &placement->placement[i];
> struct ttm_resource_manager *man;
>
> - ret = ttm_bo_mem_placement(bo, place, mem);
> - if (ret)
> + man = ttm_manager_type(bdev, place->mem_type);
> + if (!man || !ttm_resource_manager_used(man))
> continue;
>
> type_found = true;
> @@ -818,8 +789,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
> if (unlikely(ret))
> goto error;
>
> - man = ttm_manager_type(bdev, mem->mem_type);
> - ret = ttm_bo_add_move_fence(bo, man, mem, ctx->no_wait_gpu);
> + ret = ttm_bo_add_move_fence(bo, man, *mem, ctx->no_wait_gpu);
> if (unlikely(ret)) {
> ttm_resource_free(bo, mem);
> if (ret == -EBUSY)
> @@ -832,9 +802,10 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>
> for (i = 0; i < placement->num_busy_placement; ++i) {
> const struct ttm_place *place = &placement->busy_placement[i];
> + struct ttm_resource_manager *man;
>
> - ret = ttm_bo_mem_placement(bo, place, mem);
> - if (ret)
> + man = ttm_manager_type(bdev, place->mem_type);
> + if (!man || !ttm_resource_manager_used(man))
> continue;
>
> type_found = true;
> @@ -861,12 +832,12 @@ 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_resource **mem,
> struct ttm_operation_ctx *ctx,
> struct ttm_place *hop)
> {
> struct ttm_placement hop_placement;
> - struct ttm_resource hop_mem;
> + struct ttm_resource *hop_mem;
> int ret;
>
> hop_placement.num_placement = hop_placement.num_busy_placement = 1;
> @@ -877,7 +848,7 @@ static int ttm_bo_bounce_temp_buffer(struct ttm_buffer_object *bo,
> if (ret)
> return ret;
> /* move to the bounce domain */
> - ret = ttm_bo_handle_move_mem(bo, &hop_mem, false, ctx, NULL);
> + ret = ttm_bo_handle_move_mem(bo, hop_mem, false, ctx, NULL);
> if (ret) {
> ttm_resource_free(bo, &hop_mem);
> return ret;
> @@ -889,14 +860,12 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
> struct ttm_placement *placement,
> struct ttm_operation_ctx *ctx)
> {
> + struct ttm_resource *mem;
> struct ttm_place hop;
> - struct ttm_resource mem;
> int ret;
>
> dma_resv_assert_held(bo->base.resv);
>
> - memset(&hop, 0, sizeof(hop));
> -
> /*
> * Determine where to move the buffer.
> *
> @@ -910,7 +879,7 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
> if (ret)
> return ret;
> bounce:
> - ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx, &hop);
> + 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)
> @@ -1019,7 +988,7 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
> {
> static const struct ttm_place sys_mem = { .mem_type = TTM_PL_SYSTEM };
> bool locked;
> - int ret = 0;
> + int ret;
>
> bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
>
> @@ -1029,8 +998,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
> bo->bdev = bdev;
> bo->type = type;
> bo->page_alignment = page_alignment;
> - bo->resource = &bo->_mem;
> - ttm_resource_alloc(bo, &sys_mem, bo->resource);
> bo->moving = NULL;
> bo->pin_count = 0;
> bo->sg = sg;
> @@ -1042,6 +1009,12 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
> }
> atomic_inc(&ttm_glob.bo_count);
>
> + ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
> + if (unlikely(ret)) {
> + ttm_bo_put(bo);
> + return ret;
> + }
> +
> /*
> * For ttm_bo_type_device buffers, allocate
> * address space from the device.
> @@ -1170,7 +1143,7 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
> */
> if (bo->resource->mem_type != TTM_PL_SYSTEM) {
> struct ttm_operation_ctx ctx = { false, false };
> - struct ttm_resource evict_mem;
> + struct ttm_resource *evict_mem;
> struct ttm_place place, hop;
>
> memset(&place, 0, sizeof(place));
> @@ -1182,7 +1155,7 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
> if (unlikely(ret))
> goto out;
>
> - ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, &ctx, &hop);
> + 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/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index aedf02a31c70..1b326e70cb02 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -176,16 +176,17 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> struct ttm_operation_ctx *ctx,
> struct ttm_resource *new_mem)
> {
> + struct ttm_resource *old_mem = bo->resource;
> struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource_manager *man = ttm_manager_type(bdev, new_mem->mem_type);
> + struct ttm_resource_manager *man;
> struct ttm_tt *ttm = bo->ttm;
> - struct ttm_resource *old_mem = bo->resource;
> - struct ttm_resource old_copy = *old_mem;
> void *old_iomap;
> void *new_iomap;
> int ret;
> unsigned long i;
>
> + man = ttm_manager_type(bdev, new_mem->mem_type);
> +
> ret = ttm_bo_wait_ctx(bo, ctx);
> if (ret)
> return ret;
> @@ -201,7 +202,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> * Single TTM move. NOP.
> */
> if (old_iomap == NULL && new_iomap == NULL)
> - goto out2;
> + goto out1;
>
> /*
> * Don't move nonexistent data. Clear destination instead.
> @@ -210,7 +211,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> (ttm == NULL || (!ttm_tt_is_populated(ttm) &&
> !(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)))) {
> memset_io(new_iomap, 0, new_mem->num_pages*PAGE_SIZE);
> - goto out2;
> + goto out1;
> }
>
> /*
> @@ -235,27 +236,25 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> ret = ttm_copy_io_page(new_iomap, old_iomap, i);
> }
> if (ret)
> - goto out1;
> + break;
> }
> mb();
> -out2:
> - old_copy = *old_mem;
> +out1:
> + ttm_resource_iounmap(bdev, new_mem, new_iomap);
> +out:
> + ttm_resource_iounmap(bdev, old_mem, old_iomap);
> +
> + if (ret) {
> + ttm_resource_free(bo, &new_mem);
> + return ret;
> + }
>
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_mem);
>
> if (!man->use_tt)
> ttm_bo_tt_destroy(bo);
>
> -out1:
> - ttm_resource_iounmap(bdev, old_mem, new_iomap);
> -out:
> - ttm_resource_iounmap(bdev, &old_copy, old_iomap);
> -
> - /*
> - * On error, keep the mm node!
> - */
> - if (!ret)
> - ttm_resource_free(bo, &old_copy);
> return ret;
> }
> EXPORT_SYMBOL(ttm_bo_move_memcpy);
> @@ -566,7 +565,7 @@ static int ttm_bo_wait_free_node(struct ttm_buffer_object *bo,
>
> if (!dst_use_tt)
> ttm_bo_tt_destroy(bo);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> return 0;
> }
>
> @@ -629,7 +628,7 @@ static void ttm_bo_move_pipeline_evict(struct ttm_buffer_object *bo,
> }
> spin_unlock(&from->move_lock);
>
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
>
> dma_fence_put(bo->moving);
> bo->moving = dma_fence_get(fence);
> @@ -678,11 +677,11 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
> if (ret)
> ttm_bo_wait(bo, false, false);
>
> - ttm_resource_alloc(bo, &sys_mem, bo->resource);
> + ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
> bo->ttm = NULL;
>
> dma_resv_unlock(&ghost->base._resv);
> ttm_bo_put(ghost);
>
> - return 0;
> + return ret;
> }
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 59e2b7157e41..65451e1bc303 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -27,10 +27,16 @@
>
> int ttm_resource_alloc(struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> - struct ttm_resource *res)
> + struct ttm_resource **res_ptr)
> {
> struct ttm_resource_manager *man =
> ttm_manager_type(bo->bdev, place->mem_type);
> + struct ttm_resource *res;
> + int r;
> +
> + res = kmalloc(sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
>
> res->mm_node = NULL;
> res->start = 0;
> @@ -41,18 +47,27 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo,
> res->bus.offset = 0;
> res->bus.is_iomem = false;
> res->bus.caching = ttm_cached;
> + r = man->func->alloc(man, bo, place, res);
> + if (r) {
> + kfree(res);
> + return r;
> + }
>
> - return man->func->alloc(man, bo, place, res);
> + *res_ptr = res;
> + return 0;
> }
>
> -void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource *res)
> +void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
> {
> - struct ttm_resource_manager *man =
> - ttm_manager_type(bo->bdev, res->mem_type);
> + struct ttm_resource_manager *man;
>
> - man->func->free(man, res);
> - res->mm_node = NULL;
> - res->mem_type = TTM_PL_SYSTEM;
> + if (!*res)
> + return;
> +
> + man = ttm_manager_type(bo->bdev, (*res)->mem_type);
> + man->func->free(man, *res);
> + kfree(*res);
> + *res = NULL;
> }
> EXPORT_SYMBOL(ttm_resource_free);
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> index ed8563ef9a3b..bfcf31bf7e37 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> @@ -741,7 +741,7 @@ static int vmw_move(struct ttm_buffer_object *bo,
> goto fail;
>
> vmw_ttm_unbind(bo->bdev, bo->ttm);
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_mem);
> return 0;
> } else {
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 291a339a7e08..f681bbdbc698 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -137,7 +137,6 @@ struct ttm_buffer_object {
> */
>
> struct ttm_resource *resource;
> - struct ttm_resource _mem;
> struct ttm_tt *ttm;
> bool deleted;
>
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 1a9ba0b13622..ead0ef7136c8 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -96,7 +96,7 @@ struct ttm_lru_bulk_move {
> */
> int ttm_bo_mem_space(struct ttm_buffer_object *bo,
> struct ttm_placement *placement,
> - struct ttm_resource *mem,
> + struct ttm_resource **mem,
> struct ttm_operation_ctx *ctx);
>
> /**
> @@ -188,8 +188,8 @@ ttm_bo_move_to_lru_tail_unlocked(struct ttm_buffer_object *bo)
> static inline void ttm_bo_assign_mem(struct ttm_buffer_object *bo,
> struct ttm_resource *new_mem)
> {
> - bo->_mem = *new_mem;
> - new_mem->mm_node = NULL;
> + WARN_ON(bo->resource);
> + bo->resource = new_mem;
> }
>
> /**
> @@ -202,9 +202,7 @@ static inline void ttm_bo_assign_mem(struct ttm_buffer_object *bo,
> static inline void ttm_bo_move_null(struct ttm_buffer_object *bo,
> struct ttm_resource *new_mem)
> {
> - struct ttm_resource *old_mem = bo->resource;
> -
> - WARN_ON(old_mem->mm_node != NULL);
> + ttm_resource_free(bo, &bo->resource);
> ttm_bo_assign_mem(bo, new_mem);
> }
>
> diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
> index 890b9d369519..c17c1a52070d 100644
> --- a/include/drm/ttm/ttm_resource.h
> +++ b/include/drm/ttm/ttm_resource.h
> @@ -225,8 +225,8 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man)
>
> int ttm_resource_alloc(struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> - struct ttm_resource *res);
> -void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource *res);
> + struct ttm_resource **res);
> +void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);
>
> void ttm_resource_manager_init(struct ttm_resource_manager *man,
> unsigned long p_size);
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-03 8:45 ` [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Matthew Auld
@ 2021-06-04 11:54 ` Christian König
0 siblings, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-04 11:54 UTC (permalink / raw)
To: Matthew Auld, thomas_os, dri-devel
Am 03.06.21 um 10:45 schrieb Matthew Auld:
> On 02/06/2021 11:09, Christian König wrote:
>> [SNIP]
>> -/**
>> - * ttm_bo_mem_placement - check if placement is compatible
>> - * @bo: BO to find memory for
>> - * @place: where to search
>> - * @mem: the memory object to fill in
>> - *
>> - * Check if placement is compatible and fill in mem structure.
>> - * Returns -EBUSY if placement won't work or negative error code.
>> - * 0 when placement can be used.
>> - */
>> -static int ttm_bo_mem_placement(struct ttm_buffer_object *bo,
>> - const struct ttm_place *place,
>> - struct ttm_resource *mem)
>> -{
>> - struct ttm_device *bdev = bo->bdev;
>> - struct ttm_resource_manager *man;
>> -
>> - man = ttm_manager_type(bdev, place->mem_type);
>> - if (!man || !ttm_resource_manager_used(man))
>> - return -EBUSY;
>> -
>> - mem->mem_type = place->mem_type;
>> - mem->placement = place->flags;
>> -
>> - spin_lock(&bo->bdev->lru_lock);
>> - ttm_bo_move_to_lru_tail(bo, mem, NULL);
>> - spin_unlock(&bo->bdev->lru_lock);
>
> Why do we drop the move_to_lru_tail here?
Ah, good point.
The move_to_lru_tail() was here to make sure we see the BO in the new
LRU instead of the old one before actually doing the move.
Since we haven't allocated the mem structure at this point that is no
longer possible, but I think it is ok to do this for now.
One motivation of doing this is to move the LRU handling into the
resource backend, so that tricks like those are not needed any more.
Regards,
Christian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2
2021-06-02 10:09 ` [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2 Christian König
@ 2021-06-07 10:15 ` Thomas Hellström (Intel)
2021-06-07 10:37 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-07 10:15 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/2/21 12:09 PM, Christian König wrote:
> Instead of both driver and TTM allocating memory finalize embedding the
> ttm_resource object as base into the driver backends.
>
> v2: fix typo in vmwgfx grid mgr and double init in amdgpu_vram_mgr.c
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 44 ++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
> .../gpu/drm/amd/amdgpu/amdgpu_res_cursor.h | 5 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 60 +++++++++----------
> drivers/gpu/drm/drm_gem_vram_helper.c | 3 +-
> drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +--
> drivers/gpu/drm/nouveau/nouveau_mem.c | 11 ++--
> drivers/gpu/drm/nouveau/nouveau_mem.h | 14 ++---
> drivers/gpu/drm/nouveau/nouveau_ttm.c | 32 +++++-----
> drivers/gpu/drm/ttm/ttm_range_manager.c | 23 +++----
> drivers/gpu/drm/ttm/ttm_resource.c | 18 +-----
> drivers/gpu/drm/ttm/ttm_sys_manager.c | 12 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 24 ++++----
> drivers/gpu/drm/vmwgfx/vmwgfx_thp.c | 27 ++++-----
> include/drm/ttm/ttm_range_manager.h | 3 +-
> include/drm/ttm/ttm_resource.h | 43 ++++++-------
> 16 files changed, 140 insertions(+), 189 deletions(-)
...
>
> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
> index ce5d07ca384c..c32e1aee2481 100644
> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
> @@ -58,7 +58,7 @@ to_range_manager(struct ttm_resource_manager *man)
> static int ttm_range_man_alloc(struct ttm_resource_manager *man,
> struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> - struct ttm_resource *mem)
> + struct ttm_resource **res)
> {
> struct ttm_range_manager *rman = to_range_manager(man);
> struct ttm_range_mgr_node *node;
> @@ -83,37 +83,30 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man,
>
> spin_lock(&rman->lock);
> ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0],
> - mem->num_pages, bo->page_alignment, 0,
> + node->base.num_pages,
> + bo->page_alignment, 0,
> place->fpfn, lpfn, mode);
> spin_unlock(&rman->lock);
>
> - if (unlikely(ret)) {
> + if (unlikely(ret))
> kfree(node);
> - } else {
> - mem->mm_node = &node->mm_nodes[0];
> - mem->start = node->mm_nodes[0].start;
> - }
> + else
> + node->base.start = node->mm_nodes[0].start;
>
> return ret;
> }
Looks like this patch forgets to assign *@res. Null pointer derefs when
testing i915.
BTW shouldn't we return the struct ttm_resource ptr here rather than
passing it as an argument?
/Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2
2021-06-07 10:15 ` Thomas Hellström (Intel)
@ 2021-06-07 10:37 ` Christian König
2021-06-07 10:44 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-07 10:37 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 07.06.21 um 12:15 schrieb Thomas Hellström (Intel):
>
> On 6/2/21 12:09 PM, Christian König wrote:
>> Instead of both driver and TTM allocating memory finalize embedding the
>> ttm_resource object as base into the driver backends.
>>
>> v2: fix typo in vmwgfx grid mgr and double init in amdgpu_vram_mgr.c
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 44 ++++++--------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
>> .../gpu/drm/amd/amdgpu/amdgpu_res_cursor.h | 5 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 60 +++++++++----------
>> drivers/gpu/drm/drm_gem_vram_helper.c | 3 +-
>> drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +--
>> drivers/gpu/drm/nouveau/nouveau_mem.c | 11 ++--
>> drivers/gpu/drm/nouveau/nouveau_mem.h | 14 ++---
>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 32 +++++-----
>> drivers/gpu/drm/ttm/ttm_range_manager.c | 23 +++----
>> drivers/gpu/drm/ttm/ttm_resource.c | 18 +-----
>> drivers/gpu/drm/ttm/ttm_sys_manager.c | 12 ++--
>> drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 24 ++++----
>> drivers/gpu/drm/vmwgfx/vmwgfx_thp.c | 27 ++++-----
>> include/drm/ttm/ttm_range_manager.h | 3 +-
>> include/drm/ttm/ttm_resource.h | 43 ++++++-------
>> 16 files changed, 140 insertions(+), 189 deletions(-)
> ...
>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> index ce5d07ca384c..c32e1aee2481 100644
>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> @@ -58,7 +58,7 @@ to_range_manager(struct ttm_resource_manager *man)
>> static int ttm_range_man_alloc(struct ttm_resource_manager *man,
>> struct ttm_buffer_object *bo,
>> const struct ttm_place *place,
>> - struct ttm_resource *mem)
>> + struct ttm_resource **res)
>> {
>> struct ttm_range_manager *rman = to_range_manager(man);
>> struct ttm_range_mgr_node *node;
>> @@ -83,37 +83,30 @@ static int ttm_range_man_alloc(struct
>> ttm_resource_manager *man,
>> spin_lock(&rman->lock);
>> ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0],
>> - mem->num_pages, bo->page_alignment, 0,
>> + node->base.num_pages,
>> + bo->page_alignment, 0,
>> place->fpfn, lpfn, mode);
>> spin_unlock(&rman->lock);
>> - if (unlikely(ret)) {
>> + if (unlikely(ret))
>> kfree(node);
>> - } else {
>> - mem->mm_node = &node->mm_nodes[0];
>> - mem->start = node->mm_nodes[0].start;
>> - }
>> + else
>> + node->base.start = node->mm_nodes[0].start;
>> return ret;
>> }
>
> Looks like this patch forgets to assign *@res. Null pointer derefs
> when testing i915.
I should really CC the Intel list for TTM patches as well. The CI system
should have spotted that.
>
> BTW shouldn't we return the struct ttm_resource ptr here rather than
> passing it as an argument?
Yeah, good idea.
Thanks for the report,
Christian.
>
> /Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2
2021-06-07 10:37 ` Christian König
@ 2021-06-07 10:44 ` Thomas Hellström (Intel)
0 siblings, 0 replies; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-07 10:44 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/7/21 12:37 PM, Christian König wrote:
> Am 07.06.21 um 12:15 schrieb Thomas Hellström (Intel):
>>
>> On 6/2/21 12:09 PM, Christian König wrote:
>>> Instead of both driver and TTM allocating memory finalize embedding the
>>> ttm_resource object as base into the driver backends.
>>>
>>> v2: fix typo in vmwgfx grid mgr and double init in amdgpu_vram_mgr.c
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 44 ++++++--------
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
>>> .../gpu/drm/amd/amdgpu/amdgpu_res_cursor.h | 5 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 60
>>> +++++++++----------
>>> drivers/gpu/drm/drm_gem_vram_helper.c | 3 +-
>>> drivers/gpu/drm/nouveau/nouveau_bo.c | 8 +--
>>> drivers/gpu/drm/nouveau/nouveau_mem.c | 11 ++--
>>> drivers/gpu/drm/nouveau/nouveau_mem.h | 14 ++---
>>> drivers/gpu/drm/nouveau/nouveau_ttm.c | 32 +++++-----
>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 23 +++----
>>> drivers/gpu/drm/ttm/ttm_resource.c | 18 +-----
>>> drivers/gpu/drm/ttm/ttm_sys_manager.c | 12 ++--
>>> drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 24 ++++----
>>> drivers/gpu/drm/vmwgfx/vmwgfx_thp.c | 27 ++++-----
>>> include/drm/ttm/ttm_range_manager.h | 3 +-
>>> include/drm/ttm/ttm_resource.h | 43 ++++++-------
>>> 16 files changed, 140 insertions(+), 189 deletions(-)
>> ...
>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> index ce5d07ca384c..c32e1aee2481 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> @@ -58,7 +58,7 @@ to_range_manager(struct ttm_resource_manager *man)
>>> static int ttm_range_man_alloc(struct ttm_resource_manager *man,
>>> struct ttm_buffer_object *bo,
>>> const struct ttm_place *place,
>>> - struct ttm_resource *mem)
>>> + struct ttm_resource **res)
>>> {
>>> struct ttm_range_manager *rman = to_range_manager(man);
>>> struct ttm_range_mgr_node *node;
>>> @@ -83,37 +83,30 @@ static int ttm_range_man_alloc(struct
>>> ttm_resource_manager *man,
>>> spin_lock(&rman->lock);
>>> ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0],
>>> - mem->num_pages, bo->page_alignment, 0,
>>> + node->base.num_pages,
>>> + bo->page_alignment, 0,
>>> place->fpfn, lpfn, mode);
>>> spin_unlock(&rman->lock);
>>> - if (unlikely(ret)) {
>>> + if (unlikely(ret))
>>> kfree(node);
>>> - } else {
>>> - mem->mm_node = &node->mm_nodes[0];
>>> - mem->start = node->mm_nodes[0].start;
>>> - }
>>> + else
>>> + node->base.start = node->mm_nodes[0].start;
>>> return ret;
>>> }
>>
>> Looks like this patch forgets to assign *@res. Null pointer derefs
>> when testing i915.
>
> I should really CC the Intel list for TTM patches as well. The CI
> system should have spotted that.
Unfortunately, the dg1 system is not participating in CI yet AFAICT, but
moving forward I think it's a good idea.
/Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (10 preceding siblings ...)
2021-06-04 9:33 ` Thomas Hellström (Intel)
@ 2021-06-07 16:40 ` Thomas Hellström (Intel)
2021-06-07 17:06 ` Thomas Hellström (Intel)
2021-06-07 17:10 ` Christian König
2021-06-08 6:55 ` Thomas Hellström
12 siblings, 2 replies; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-07 16:40 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/2/21 12:09 PM, Christian König wrote:
...
> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
> */
> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
> const struct ttm_place *place,
> - struct ttm_resource *mem,
> + struct ttm_resource **mem,
> struct ttm_operation_ctx *ctx)
> {
> struct ttm_device *bdev = bo->bdev;
> - struct ttm_resource_manager *man = ttm_manager_type(bdev, mem->mem_type);
> + struct ttm_resource_manager *man;
> struct ww_acquire_ctx *ticket;
> int ret;
>
> + man = ttm_manager_type(bdev, (*mem)->mem_type);
Isn't (*mem) uninitialized here? Should be place->mem_type? Eviction is
immediately sent to the bushes.
Got at least one additional NULL pointer deref to track down in the
eviction code, but could be a merge error of mine as well.
/Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-07 16:40 ` Thomas Hellström (Intel)
@ 2021-06-07 17:06 ` Thomas Hellström (Intel)
2021-06-07 17:54 ` Christian König
2021-06-07 17:10 ` Christian König
1 sibling, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-07 17:06 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/7/21 6:40 PM, Thomas Hellström (Intel) wrote:
>
> On 6/2/21 12:09 PM, Christian König wrote:
> ...
>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>> ttm_buffer_object *bo,
>> */
>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>> const struct ttm_place *place,
>> - struct ttm_resource *mem,
>> + struct ttm_resource **mem,
>> struct ttm_operation_ctx *ctx)
>> {
>> struct ttm_device *bdev = bo->bdev;
>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>> mem->mem_type);
>> + struct ttm_resource_manager *man;
>> struct ww_acquire_ctx *ticket;
>> int ret;
>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>
> Isn't (*mem) uninitialized here? Should be place->mem_type? Eviction
> is immediately sent to the bushes.
>
> Got at least one additional NULL pointer deref to track down in the
> eviction code, but could be a merge error of mine as well.
Actually this last one was probably due to a bad temporary fix of the
above one.
/Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-07 16:40 ` Thomas Hellström (Intel)
2021-06-07 17:06 ` Thomas Hellström (Intel)
@ 2021-06-07 17:10 ` Christian König
1 sibling, 0 replies; 43+ messages in thread
From: Christian König @ 2021-06-07 17:10 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 07.06.21 um 18:40 schrieb Thomas Hellström (Intel):
>
> On 6/2/21 12:09 PM, Christian König wrote:
> ...
>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>> ttm_buffer_object *bo,
>> */
>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>> const struct ttm_place *place,
>> - struct ttm_resource *mem,
>> + struct ttm_resource **mem,
>> struct ttm_operation_ctx *ctx)
>> {
>> struct ttm_device *bdev = bo->bdev;
>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>> mem->mem_type);
>> + struct ttm_resource_manager *man;
>> struct ww_acquire_ctx *ticket;
>> int ret;
>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>
> Isn't (*mem) uninitialized here? Should be place->mem_type? Eviction
> is immediately sent to the bushes.
Indeed and I'm like 99% sure that Nirmoy has pointed that out to me as well.
Looks like I missed to fix that one. We need more CI testing.
Thanks,
Christian.
>
> Got at least one additional NULL pointer deref to track down in the
> eviction code, but could be a merge error of mine as well.
>
> /Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-07 17:06 ` Thomas Hellström (Intel)
@ 2021-06-07 17:54 ` Christian König
2021-06-07 17:58 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-07 17:54 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 07.06.21 um 19:06 schrieb Thomas Hellström (Intel):
>
> On 6/7/21 6:40 PM, Thomas Hellström (Intel) wrote:
>>
>> On 6/2/21 12:09 PM, Christian König wrote:
>> ...
>>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>>> ttm_buffer_object *bo,
>>> */
>>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>> const struct ttm_place *place,
>>> - struct ttm_resource *mem,
>>> + struct ttm_resource **mem,
>>> struct ttm_operation_ctx *ctx)
>>> {
>>> struct ttm_device *bdev = bo->bdev;
>>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>>> mem->mem_type);
>>> + struct ttm_resource_manager *man;
>>> struct ww_acquire_ctx *ticket;
>>> int ret;
>>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>>
>> Isn't (*mem) uninitialized here? Should be place->mem_type? Eviction
>> is immediately sent to the bushes.
>>
>> Got at least one additional NULL pointer deref to track down in the
>> eviction code, but could be a merge error of mine as well.
>
> Actually this last one was probably due to a bad temporary fix of the
> above one.
I've found one more warning during my testing now. But that is just a
false positive.
Apart from that I haven't seen any other fallout, but fingers crossed.
Christian.
>
> /Thomas
>
>>
>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-07 17:54 ` Christian König
@ 2021-06-07 17:58 ` Thomas Hellström (Intel)
2021-06-07 17:59 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-07 17:58 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
On 6/7/21 7:54 PM, Christian König wrote:
>
>
> Am 07.06.21 um 19:06 schrieb Thomas Hellström (Intel):
>>
>> On 6/7/21 6:40 PM, Thomas Hellström (Intel) wrote:
>>>
>>> On 6/2/21 12:09 PM, Christian König wrote:
>>> ...
>>>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>>>> ttm_buffer_object *bo,
>>>> */
>>>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>>> const struct ttm_place *place,
>>>> - struct ttm_resource *mem,
>>>> + struct ttm_resource **mem,
>>>> struct ttm_operation_ctx *ctx)
>>>> {
>>>> struct ttm_device *bdev = bo->bdev;
>>>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>>>> mem->mem_type);
>>>> + struct ttm_resource_manager *man;
>>>> struct ww_acquire_ctx *ticket;
>>>> int ret;
>>>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>>>
>>> Isn't (*mem) uninitialized here? Should be place->mem_type? Eviction
>>> is immediately sent to the bushes.
>>>
>>> Got at least one additional NULL pointer deref to track down in the
>>> eviction code, but could be a merge error of mine as well.
>>
>> Actually this last one was probably due to a bad temporary fix of the
>> above one.
>
> I've found one more warning during my testing now. But that is just a
> false positive.
>
> Apart from that I haven't seen any other fallout, but fingers crossed.
vmwgfx doesn't seem to happy. It works AFAICT., but warns in vmw_move()
about ttm_bo_assign_mem() replacing an existing resource.
/Thomas
>
> Christian.
>
>>
>> /Thomas
>>
>>>
>>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-07 17:58 ` Thomas Hellström (Intel)
@ 2021-06-07 17:59 ` Christian König
2021-06-08 5:29 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-07 17:59 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 07.06.21 um 19:58 schrieb Thomas Hellström (Intel):
>
> On 6/7/21 7:54 PM, Christian König wrote:
>>
>>
>> Am 07.06.21 um 19:06 schrieb Thomas Hellström (Intel):
>>>
>>> On 6/7/21 6:40 PM, Thomas Hellström (Intel) wrote:
>>>>
>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>> ...
>>>>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>>>>> ttm_buffer_object *bo,
>>>>> */
>>>>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>>>> const struct ttm_place *place,
>>>>> - struct ttm_resource *mem,
>>>>> + struct ttm_resource **mem,
>>>>> struct ttm_operation_ctx *ctx)
>>>>> {
>>>>> struct ttm_device *bdev = bo->bdev;
>>>>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>>>>> mem->mem_type);
>>>>> + struct ttm_resource_manager *man;
>>>>> struct ww_acquire_ctx *ticket;
>>>>> int ret;
>>>>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>>>>
>>>> Isn't (*mem) uninitialized here? Should be place->mem_type?
>>>> Eviction is immediately sent to the bushes.
>>>>
>>>> Got at least one additional NULL pointer deref to track down in the
>>>> eviction code, but could be a merge error of mine as well.
>>>
>>> Actually this last one was probably due to a bad temporary fix of
>>> the above one.
>>
>> I've found one more warning during my testing now. But that is just a
>> false positive.
>>
>> Apart from that I haven't seen any other fallout, but fingers crossed.
>
> vmwgfx doesn't seem to happy. It works AFAICT., but warns in
> vmw_move() about ttm_bo_assign_mem() replacing an existing resource.
Yeah, that's the one I've just fixed. Patch is underway.
Christian.
>
> /Thomas
>
>
>
>>
>> Christian.
>>
>>>
>>> /Thomas
>>>
>>>>
>>>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-07 17:59 ` Christian König
@ 2021-06-08 5:29 ` Thomas Hellström (Intel)
2021-06-08 7:14 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-08 5:29 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
Hi,
On 6/7/21 7:59 PM, Christian König wrote:
>
>
> Am 07.06.21 um 19:58 schrieb Thomas Hellström (Intel):
>>
>> On 6/7/21 7:54 PM, Christian König wrote:
>>>
>>>
>>> Am 07.06.21 um 19:06 schrieb Thomas Hellström (Intel):
>>>>
>>>> On 6/7/21 6:40 PM, Thomas Hellström (Intel) wrote:
>>>>>
>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>> ...
>>>>>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>>>>>> ttm_buffer_object *bo,
>>>>>> */
>>>>>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>>>>> const struct ttm_place *place,
>>>>>> - struct ttm_resource *mem,
>>>>>> + struct ttm_resource **mem,
>>>>>> struct ttm_operation_ctx *ctx)
>>>>>> {
>>>>>> struct ttm_device *bdev = bo->bdev;
>>>>>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>>>>>> mem->mem_type);
>>>>>> + struct ttm_resource_manager *man;
>>>>>> struct ww_acquire_ctx *ticket;
>>>>>> int ret;
>>>>>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>>>>>
>>>>> Isn't (*mem) uninitialized here? Should be place->mem_type?
>>>>> Eviction is immediately sent to the bushes.
>>>>>
>>>>> Got at least one additional NULL pointer deref to track down in
>>>>> the eviction code, but could be a merge error of mine as well.
>>>>
>>>> Actually this last one was probably due to a bad temporary fix of
>>>> the above one.
>>>
>>> I've found one more warning during my testing now. But that is just
>>> a false positive.
>>>
>>> Apart from that I haven't seen any other fallout, but fingers crossed.
>>
>> vmwgfx doesn't seem to happy. It works AFAICT., but warns in
>> vmw_move() about ttm_bo_assign_mem() replacing an existing resource.
>
> Yeah, that's the one I've just fixed. Patch is underway.
If that's the move_to_ghost patch, I don't think that would fix the
vmwgfx issue, as IIRC vmwgfx ever uses ghost objects.
/Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
` (11 preceding siblings ...)
2021-06-07 16:40 ` Thomas Hellström (Intel)
@ 2021-06-08 6:55 ` Thomas Hellström
12 siblings, 0 replies; 43+ messages in thread
From: Thomas Hellström @ 2021-06-08 6:55 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
Hi,
On 6/2/21 12:09 PM, Christian König wrote:
> To improve the handling we want the establish the resource object as base
> class for the backend allocations.
>
> v2: add missing error handling
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 54 +++++++-------
> drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +-
> drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 83 ++++++++--------------
> drivers/gpu/drm/ttm/ttm_bo_util.c | 43 ++++++-----
> drivers/gpu/drm/ttm/ttm_resource.c | 31 +++++---
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 2 +-
> include/drm/ttm/ttm_bo_api.h | 1 -
> include/drm/ttm/ttm_bo_driver.h | 10 ++-
> include/drm/ttm/ttm_resource.h | 4 +-
> 11 files changed, 110 insertions(+), 126 deletions(-)
...
>
> @@ -629,7 +628,7 @@ static void ttm_bo_move_pipeline_evict(struct ttm_buffer_object *bo,
> }
> spin_unlock(&from->move_lock);
>
> - ttm_resource_free(bo, bo->resource);
> + ttm_resource_free(bo, &bo->resource);
>
> dma_fence_put(bo->moving);
> bo->moving = dma_fence_get(fence);
> @@ -678,11 +677,11 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
> if (ret)
> ttm_bo_wait(bo, false, false);
>
> - ttm_resource_alloc(bo, &sys_mem, bo->resource);
> + ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
> bo->ttm = NULL;
>
> dma_resv_unlock(&ghost->base._resv);
> ttm_bo_put(ghost);
>
> - return 0;
> + return ret;
Here we re-introduce a late point of failure, which I guess leaves the
bo in an undefined state? Same thing with my optimization for the idle
case. Needs fixing as soon as possible.
/Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-08 5:29 ` Thomas Hellström (Intel)
@ 2021-06-08 7:14 ` Christian König
2021-06-08 7:17 ` Thomas Hellström (Intel)
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-08 7:14 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel
Am 08.06.21 um 07:29 schrieb Thomas Hellström (Intel):
> Hi,
>
> On 6/7/21 7:59 PM, Christian König wrote:
>>
>>
>> Am 07.06.21 um 19:58 schrieb Thomas Hellström (Intel):
>>>
>>> On 6/7/21 7:54 PM, Christian König wrote:
>>>>
>>>>
>>>> Am 07.06.21 um 19:06 schrieb Thomas Hellström (Intel):
>>>>>
>>>>> On 6/7/21 6:40 PM, Thomas Hellström (Intel) wrote:
>>>>>>
>>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>>> ...
>>>>>>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>>>>>>> ttm_buffer_object *bo,
>>>>>>> */
>>>>>>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>>>>>> const struct ttm_place *place,
>>>>>>> - struct ttm_resource *mem,
>>>>>>> + struct ttm_resource **mem,
>>>>>>> struct ttm_operation_ctx *ctx)
>>>>>>> {
>>>>>>> struct ttm_device *bdev = bo->bdev;
>>>>>>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>>>>>>> mem->mem_type);
>>>>>>> + struct ttm_resource_manager *man;
>>>>>>> struct ww_acquire_ctx *ticket;
>>>>>>> int ret;
>>>>>>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>>>>>>
>>>>>> Isn't (*mem) uninitialized here? Should be place->mem_type?
>>>>>> Eviction is immediately sent to the bushes.
>>>>>>
>>>>>> Got at least one additional NULL pointer deref to track down in
>>>>>> the eviction code, but could be a merge error of mine as well.
>>>>>
>>>>> Actually this last one was probably due to a bad temporary fix of
>>>>> the above one.
>>>>
>>>> I've found one more warning during my testing now. But that is just
>>>> a false positive.
>>>>
>>>> Apart from that I haven't seen any other fallout, but fingers crossed.
>>>
>>> vmwgfx doesn't seem to happy. It works AFAICT., but warns in
>>> vmw_move() about ttm_bo_assign_mem() replacing an existing resource.
>>
>> Yeah, that's the one I've just fixed. Patch is underway.
>
> If that's the move_to_ghost patch, I don't think that would fix the
> vmwgfx issue, as IIRC vmwgfx ever uses ghost objects.
Mhm, could be that vmwgfx is hitting the same warning with a different
backtrace.
Do you have the log to double check?
Thanks,
Christian.
>
> /Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-08 7:14 ` Christian König
@ 2021-06-08 7:17 ` Thomas Hellström (Intel)
2021-06-08 7:21 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Thomas Hellström (Intel) @ 2021-06-08 7:17 UTC (permalink / raw)
To: Christian König, matthew.auld, dri-devel
Hi,
On 6/8/21 9:14 AM, Christian König wrote:
> Am 08.06.21 um 07:29 schrieb Thomas Hellström (Intel):
>> Hi,
>>
>> On 6/7/21 7:59 PM, Christian König wrote:
>>>
>>>
>>> Am 07.06.21 um 19:58 schrieb Thomas Hellström (Intel):
>>>>
>>>> On 6/7/21 7:54 PM, Christian König wrote:
>>>>>
>>>>>
>>>>> Am 07.06.21 um 19:06 schrieb Thomas Hellström (Intel):
>>>>>>
>>>>>> On 6/7/21 6:40 PM, Thomas Hellström (Intel) wrote:
>>>>>>>
>>>>>>> On 6/2/21 12:09 PM, Christian König wrote:
>>>>>>> ...
>>>>>>>> @@ -728,14 +728,15 @@ static int ttm_bo_add_move_fence(struct
>>>>>>>> ttm_buffer_object *bo,
>>>>>>>> */
>>>>>>>> static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>>>>>>> const struct ttm_place *place,
>>>>>>>> - struct ttm_resource *mem,
>>>>>>>> + struct ttm_resource **mem,
>>>>>>>> struct ttm_operation_ctx *ctx)
>>>>>>>> {
>>>>>>>> struct ttm_device *bdev = bo->bdev;
>>>>>>>> - struct ttm_resource_manager *man = ttm_manager_type(bdev,
>>>>>>>> mem->mem_type);
>>>>>>>> + struct ttm_resource_manager *man;
>>>>>>>> struct ww_acquire_ctx *ticket;
>>>>>>>> int ret;
>>>>>>>> + man = ttm_manager_type(bdev, (*mem)->mem_type);
>>>>>>>
>>>>>>> Isn't (*mem) uninitialized here? Should be place->mem_type?
>>>>>>> Eviction is immediately sent to the bushes.
>>>>>>>
>>>>>>> Got at least one additional NULL pointer deref to track down in
>>>>>>> the eviction code, but could be a merge error of mine as well.
>>>>>>
>>>>>> Actually this last one was probably due to a bad temporary fix of
>>>>>> the above one.
>>>>>
>>>>> I've found one more warning during my testing now. But that is
>>>>> just a false positive.
>>>>>
>>>>> Apart from that I haven't seen any other fallout, but fingers
>>>>> crossed.
>>>>
>>>> vmwgfx doesn't seem to happy. It works AFAICT., but warns in
>>>> vmw_move() about ttm_bo_assign_mem() replacing an existing resource.
>>>
>>> Yeah, that's the one I've just fixed. Patch is underway.
>>
>> If that's the move_to_ghost patch, I don't think that would fix the
>> vmwgfx issue, as IIRC vmwgfx ever uses ghost objects.
>
> Mhm, could be that vmwgfx is hitting the same warning with a different
> backtrace.
>
> Do you have the log to double check?
Unfortunately not, but IIRC it was directly from vmw_move().
/Thomas
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-08 7:17 ` Thomas Hellström (Intel)
@ 2021-06-08 7:21 ` Christian König
2021-06-08 9:38 ` Das, Nirmoy
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-08 7:21 UTC (permalink / raw)
To: Thomas Hellström (Intel), matthew.auld, dri-devel, Nirmoy Das
Am 08.06.21 um 09:17 schrieb Thomas Hellström (Intel):
> [SNIP]
>> Do you have the log to double check?
>
> Unfortunately not, but IIRC it was directly from vmw_move().
Nirmoy do you still have your vmwgfx test environment?
Thanks,
Christian.
>
> /Thomas
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-08 7:21 ` Christian König
@ 2021-06-08 9:38 ` Das, Nirmoy
2021-06-08 9:40 ` Das, Nirmoy
0 siblings, 1 reply; 43+ messages in thread
From: Das, Nirmoy @ 2021-06-08 9:38 UTC (permalink / raw)
To: Christian König, Thomas Hellström (Intel),
matthew.auld, dri-devel
On 6/8/2021 9:21 AM, Christian König wrote:
>
>
> Am 08.06.21 um 09:17 schrieb Thomas Hellström (Intel):
>> [SNIP]
>>> Do you have the log to double check?
>>
>> Unfortunately not, but IIRC it was directly from vmw_move().
>
> Nirmoy do you still have your vmwgfx test environment?
Yes!
>
> Thanks,
> Christian.
>
>>
>> /Thomas
>>
>>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-08 9:38 ` Das, Nirmoy
@ 2021-06-08 9:40 ` Das, Nirmoy
2021-06-08 9:42 ` Christian König
0 siblings, 1 reply; 43+ messages in thread
From: Das, Nirmoy @ 2021-06-08 9:40 UTC (permalink / raw)
To: Christian König, Thomas Hellström (Intel),
matthew.auld, dri-devel
On 6/8/2021 11:38 AM, Das, Nirmoy wrote:
>
> On 6/8/2021 9:21 AM, Christian König wrote:
>>
>>
>> Am 08.06.21 um 09:17 schrieb Thomas Hellström (Intel):
>>> [SNIP]
>>>> Do you have the log to double check?
>>>
>>> Unfortunately not, but IIRC it was directly from vmw_move().
>>
>> Nirmoy do you still have your vmwgfx test environment?
>
>
> Yes!
I will test this series on my vmwgfx setup.
>
>
>>
>> Thanks,
>> Christian.
>>
>>>
>>> /Thomas
>>>
>>>
>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-08 9:40 ` Das, Nirmoy
@ 2021-06-08 9:42 ` Christian König
2021-06-08 9:48 ` Das, Nirmoy
0 siblings, 1 reply; 43+ messages in thread
From: Christian König @ 2021-06-08 9:42 UTC (permalink / raw)
To: Das, Nirmoy, Thomas Hellström (Intel), matthew.auld, dri-devel
Am 08.06.21 um 11:40 schrieb Das, Nirmoy:
>
> On 6/8/2021 11:38 AM, Das, Nirmoy wrote:
>>
>> On 6/8/2021 9:21 AM, Christian König wrote:
>>>
>>>
>>> Am 08.06.21 um 09:17 schrieb Thomas Hellström (Intel):
>>>> [SNIP]
>>>>> Do you have the log to double check?
>>>>
>>>> Unfortunately not, but IIRC it was directly from vmw_move().
>>>
>>> Nirmoy do you still have your vmwgfx test environment?
>>
>>
>> Yes!
>
>
> I will test this series on my vmwgfx setup.
Since it is already pushed (and we fixed a bunch of its fallout) can you
please test drm-misc-next instead?
Thanks,
Christian.
>
>>
>>
>>>
>>> Thanks,
>>> Christian.
>>>
>>>>
>>>> /Thomas
>>>>
>>>>
>>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2
2021-06-08 9:42 ` Christian König
@ 2021-06-08 9:48 ` Das, Nirmoy
0 siblings, 0 replies; 43+ messages in thread
From: Das, Nirmoy @ 2021-06-08 9:48 UTC (permalink / raw)
To: Christian König, Thomas Hellström (Intel),
matthew.auld, dri-devel
On 6/8/2021 11:42 AM, Christian König wrote:
>
>
> Am 08.06.21 um 11:40 schrieb Das, Nirmoy:
>>
>> On 6/8/2021 11:38 AM, Das, Nirmoy wrote:
>>>
>>> On 6/8/2021 9:21 AM, Christian König wrote:
>>>>
>>>>
>>>> Am 08.06.21 um 09:17 schrieb Thomas Hellström (Intel):
>>>>> [SNIP]
>>>>>> Do you have the log to double check?
>>>>>
>>>>> Unfortunately not, but IIRC it was directly from vmw_move().
>>>>
>>>> Nirmoy do you still have your vmwgfx test environment?
>>>
>>>
>>> Yes!
>>
>>
>> I will test this series on my vmwgfx setup.
>
> Since it is already pushed (and we fixed a bunch of its fallout) can
> you please test drm-misc-next instead?
Sure!
Nirmoy
>
> Thanks,
> Christian.
>
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Christian.
>>>>
>>>>>
>>>>> /Thomas
>>>>>
>>>>>
>>>>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2021-06-08 9:48 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 10:09 [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Christian König
2021-06-02 10:09 ` [PATCH 02/10] drm/ttm: flip over the range manager to self allocated nodes Christian König
2021-06-02 11:44 ` Thomas Hellström (Intel)
2021-06-02 12:11 ` Christian König
2021-06-02 12:33 ` Thomas Hellström (Intel)
2021-06-02 13:07 ` Christian König
2021-06-02 14:13 ` Thomas Hellström (Intel)
2021-06-02 14:17 ` Christian König
2021-06-02 15:28 ` Thomas Hellström (Intel)
2021-06-02 18:41 ` Christian König
2021-06-02 18:52 ` Thomas Hellström (Intel)
2021-06-02 18:53 ` Christian König
2021-06-02 10:09 ` [PATCH 03/10] drm/ttm: flip over the sys " Christian König
2021-06-03 7:51 ` Matthew Auld
2021-06-02 10:09 ` [PATCH 04/10] drm/amdgpu: revert "drm/amdgpu: stop allocating dummy GTT nodes" Christian König
2021-06-02 10:09 ` [PATCH 05/10] drm/amdkfd: use resource cursor in svm_migrate_copy_to_vram v2 Christian König
2021-06-03 9:44 ` Matthew Auld
2021-06-02 10:09 ` [PATCH 06/10] drm/amdgpu: switch the GTT backend to self alloc Christian König
2021-06-02 10:09 ` [PATCH 07/10] drm/amdgpu: switch the VRAM " Christian König
2021-06-02 10:09 ` [PATCH 08/10] drm/nouveau: switch the TTM backends " Christian König
2021-06-02 10:09 ` [PATCH 09/10] drm/vmwgfx: " Christian König
2021-06-02 10:09 ` [PATCH 10/10] drm/ttm: flip the switch for driver allocated resources v2 Christian König
2021-06-07 10:15 ` Thomas Hellström (Intel)
2021-06-07 10:37 ` Christian König
2021-06-07 10:44 ` Thomas Hellström (Intel)
2021-06-03 8:45 ` [PATCH 01/10] drm/ttm: allocate resource object instead of embedding it v2 Matthew Auld
2021-06-04 11:54 ` Christian König
2021-06-04 9:33 ` Thomas Hellström (Intel)
2021-06-07 16:40 ` Thomas Hellström (Intel)
2021-06-07 17:06 ` Thomas Hellström (Intel)
2021-06-07 17:54 ` Christian König
2021-06-07 17:58 ` Thomas Hellström (Intel)
2021-06-07 17:59 ` Christian König
2021-06-08 5:29 ` Thomas Hellström (Intel)
2021-06-08 7:14 ` Christian König
2021-06-08 7:17 ` Thomas Hellström (Intel)
2021-06-08 7:21 ` Christian König
2021-06-08 9:38 ` Das, Nirmoy
2021-06-08 9:40 ` Das, Nirmoy
2021-06-08 9:42 ` Christian König
2021-06-08 9:48 ` Das, Nirmoy
2021-06-07 17:10 ` Christian König
2021-06-08 6:55 ` Thomas Hellström
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.