on_alloc_stage: is this operation on allocation stage resv: reservation bo used of this operation
Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0 Signed-off-by: Roger He Hongbo.He@amd.com --- include/drm/ttm/ttm_bo_api.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj { * * @interruptible: Sleep interruptible if sleeping. * @no_wait_gpu: Return immediately if the GPU is busy. + * @on_alloc_stage: is this operation on allocation stage + * @resv: resvation bo used * * Context for TTM operations like changing buffer placement or general memory * allocation. @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj { struct ttm_operation_ctx { bool interruptible; bool no_wait_gpu; + bool on_alloc_stage; + struct reservation_object *resv; uint64_t bytes_moved; };
Change-Id: I0c6571c2a64e6c5bdad80ccbcccb40eba1c20b4e Signed-off-by: Roger He Hongbo.He@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +- drivers/gpu/drm/ttm/ttm_bo.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index dc0a8be..eb42782 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -327,7 +327,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, uint64_t init_value, struct amdgpu_bo **bo_ptr) { - struct ttm_operation_ctx ctx = { !kernel, false }; + struct ttm_operation_ctx ctx = { !kernel, false, true }; struct amdgpu_bo *bo; enum ttm_bo_type type; unsigned long page_align; diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 97c3da6..70b2673 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1208,6 +1208,7 @@ int ttm_bo_init_reserved(struct ttm_bo_device *bdev, WARN_ON(!locked); }
+ ctx->resv = resv; if (likely(!ret)) ret = ttm_bo_validate(bo, placement, ctx);
Am 12.12.2017 um 10:33 schrieb Roger He:
Change-Id: I0c6571c2a64e6c5bdad80ccbcccb40eba1c20b4e Signed-off-by: Roger He Hongbo.He@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +- drivers/gpu/drm/ttm/ttm_bo.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index dc0a8be..eb42782 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -327,7 +327,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, uint64_t init_value, struct amdgpu_bo **bo_ptr) {
- struct ttm_operation_ctx ctx = { !kernel, false };
- struct ttm_operation_ctx ctx = { !kernel, false, true }; struct amdgpu_bo *bo; enum ttm_bo_type type; unsigned long page_align;
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 97c3da6..70b2673 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1208,6 +1208,7 @@ int ttm_bo_init_reserved(struct ttm_bo_device *bdev, WARN_ON(!locked); }
- ctx->resv = resv;
That is something amdgpu specific, don't put that into TTM code.
Christian.
if (likely(!ret)) ret = ttm_bo_validate(bo, placement, ctx);
include ttm_bo_move_memcpy and ttm_bo_move_ttm
Change-Id: I160b2fe1da3200405810d0215c4521b5f0d3615a Signed-off-by: Roger He Hongbo.He@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++---- drivers/gpu/drm/nouveau/nouveau_bo.c | 7 +++---- drivers/gpu/drm/qxl/qxl_ttm.c | 3 +-- drivers/gpu/drm/radeon/radeon_ttm.c | 7 +++---- drivers/gpu/drm/ttm/ttm_bo.c | 6 ++---- drivers/gpu/drm/ttm/ttm_bo_util.c | 8 ++++---- include/drm/ttm/ttm_bo_driver.h | 4 ++-- 7 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 7db9556..c307a7d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -505,7 +505,7 @@ static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict, if (unlikely(r)) { goto out_cleanup; } - r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, new_mem); + r = ttm_bo_move_ttm(bo, ctx, new_mem); out_cleanup: ttm_bo_mem_put(bo, &tmp_mem); return r; @@ -536,7 +536,7 @@ static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict, if (unlikely(r)) { return r; } - r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, &tmp_mem); + r = ttm_bo_move_ttm(bo, ctx, &tmp_mem); if (unlikely(r)) { goto out_cleanup; } @@ -597,8 +597,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
if (r) { memcpy: - r = ttm_bo_move_memcpy(bo, ctx->interruptible, - ctx->no_wait_gpu, new_mem); + r = ttm_bo_move_memcpy(bo, ctx, new_mem); if (r) { return r; } diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 949bf6b..6b6fb20 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -1226,7 +1226,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr, if (ret) goto out;
- ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, new_reg); + ret = ttm_bo_move_ttm(bo, &ctx, new_reg); out: ttm_bo_mem_put(bo, &tmp_reg); return ret; @@ -1255,7 +1255,7 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr, if (ret) return ret;
- ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, &tmp_reg); + ret = ttm_bo_move_ttm(bo, &ctx, &tmp_reg); if (ret) goto out;
@@ -1380,8 +1380,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, /* Fallback to software copy. */ ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu); if (ret == 0) - ret = ttm_bo_move_memcpy(bo, ctx->interruptible, - ctx->no_wait_gpu, new_reg); + ret = ttm_bo_move_memcpy(bo, ctx, new_reg);
out: if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) { diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index d866f32..78ce118 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -357,8 +357,7 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, qxl_move_null(bo, new_mem); return 0; } - return ttm_bo_move_memcpy(bo, ctx->interruptible, ctx->no_wait_gpu, - new_mem); + return ttm_bo_move_memcpy(bo, ctx, new_mem); }
static void qxl_bo_move_notify(struct ttm_buffer_object *bo, diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 98e30d7..557fd79 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -347,7 +347,7 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo, if (unlikely(r)) { goto out_cleanup; } - r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, new_mem); + r = ttm_bo_move_ttm(bo, &ctx, new_mem); out_cleanup: ttm_bo_mem_put(bo, &tmp_mem); return r; @@ -380,7 +380,7 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo, if (unlikely(r)) { return r; } - r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, &tmp_mem); + r = ttm_bo_move_ttm(bo, &ctx, &tmp_mem); if (unlikely(r)) { goto out_cleanup; } @@ -445,8 +445,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
if (r) { memcpy: - r = ttm_bo_move_memcpy(bo, ctx->interruptible, - ctx->no_wait_gpu, new_mem); + r = ttm_bo_move_memcpy(bo, ctx, new_mem); if (r) { return r; } diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 70b2673..17fe8be 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -324,13 +324,11 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
if (!(old_man->flags & TTM_MEMTYPE_FLAG_FIXED) && !(new_man->flags & TTM_MEMTYPE_FLAG_FIXED)) - ret = ttm_bo_move_ttm(bo, ctx->interruptible, - ctx->no_wait_gpu, mem); + ret = ttm_bo_move_ttm(bo, ctx, mem); else if (bdev->driver->move) ret = bdev->driver->move(bo, evict, ctx, mem); else - ret = ttm_bo_move_memcpy(bo, ctx->interruptible, - ctx->no_wait_gpu, mem); + ret = ttm_bo_move_memcpy(bo, ctx, mem);
if (ret) { if (bdev->driver->move_notify) { diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index e7a519f..9237099 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -45,7 +45,7 @@ void ttm_bo_free_old_node(struct ttm_buffer_object *bo) }
int ttm_bo_move_ttm(struct ttm_buffer_object *bo, - bool interruptible, bool no_wait_gpu, + struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem) { struct ttm_tt *ttm = bo->ttm; @@ -53,7 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo, int ret;
if (old_mem->mem_type != TTM_PL_SYSTEM) { - ret = ttm_bo_wait(bo, interruptible, no_wait_gpu); + ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
if (unlikely(ret != 0)) { if (ret != -ERESTARTSYS) @@ -329,7 +329,7 @@ static int ttm_copy_ttm_io_page(struct ttm_tt *ttm, void *dst, }
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, - bool interruptible, bool no_wait_gpu, + struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem) { struct ttm_bo_device *bdev = bo->bdev; @@ -345,7 +345,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, unsigned long add = 0; int dir;
- ret = ttm_bo_wait(bo, interruptible, no_wait_gpu); + ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu); if (ret) return ret;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 6996d88..5115718 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -976,7 +976,7 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev, */
int ttm_bo_move_ttm(struct ttm_buffer_object *bo, - bool interruptible, bool no_wait_gpu, + struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem);
/** @@ -998,7 +998,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo, */
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, - bool interruptible, bool no_wait_gpu, + struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem);
/**
Am 12.12.2017 um 10:33 schrieb Roger He:
include ttm_bo_move_memcpy and ttm_bo_move_ttm
Change-Id: I160b2fe1da3200405810d0215c4521b5f0d3615a Signed-off-by: Roger He Hongbo.He@amd.com
Reviewed-by: Christian König christian.koenig@amd.com
But please separate that out and wait for a few days before committing, maybe some nouveau devs have objections.
Christian.
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++---- drivers/gpu/drm/nouveau/nouveau_bo.c | 7 +++---- drivers/gpu/drm/qxl/qxl_ttm.c | 3 +-- drivers/gpu/drm/radeon/radeon_ttm.c | 7 +++---- drivers/gpu/drm/ttm/ttm_bo.c | 6 ++---- drivers/gpu/drm/ttm/ttm_bo_util.c | 8 ++++---- include/drm/ttm/ttm_bo_driver.h | 4 ++-- 7 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 7db9556..c307a7d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -505,7 +505,7 @@ static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict, if (unlikely(r)) { goto out_cleanup; }
- r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, new_mem);
- r = ttm_bo_move_ttm(bo, ctx, new_mem); out_cleanup: ttm_bo_mem_put(bo, &tmp_mem); return r;
@@ -536,7 +536,7 @@ static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict, if (unlikely(r)) { return r; }
- r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, &tmp_mem);
- r = ttm_bo_move_ttm(bo, ctx, &tmp_mem); if (unlikely(r)) { goto out_cleanup; }
@@ -597,8 +597,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
if (r) { memcpy:
r = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, new_mem);
r = ttm_bo_move_memcpy(bo, ctx, new_mem);diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 949bf6b..6b6fb20 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -1226,7 +1226,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr, if (ret) goto out;
- ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, new_reg);
- ret = ttm_bo_move_ttm(bo, &ctx, new_reg); out: ttm_bo_mem_put(bo, &tmp_reg); return ret;
@@ -1255,7 +1255,7 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr, if (ret) return ret;
- ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, &tmp_reg);
- ret = ttm_bo_move_ttm(bo, &ctx, &tmp_reg); if (ret) goto out;
@@ -1380,8 +1380,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, /* Fallback to software copy. */ ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu); if (ret == 0)
ret = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, new_reg);
ret = ttm_bo_move_memcpy(bo, ctx, new_reg);out: if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) {
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index d866f32..78ce118 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -357,8 +357,7 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, qxl_move_null(bo, new_mem); return 0; }
- return ttm_bo_move_memcpy(bo, ctx->interruptible, ctx->no_wait_gpu,
new_mem);
return ttm_bo_move_memcpy(bo, ctx, new_mem); }
static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 98e30d7..557fd79 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -347,7 +347,7 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo, if (unlikely(r)) { goto out_cleanup; }
- r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, new_mem);
- r = ttm_bo_move_ttm(bo, &ctx, new_mem); out_cleanup: ttm_bo_mem_put(bo, &tmp_mem); return r;
@@ -380,7 +380,7 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo, if (unlikely(r)) { return r; }
- r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, &tmp_mem);
- r = ttm_bo_move_ttm(bo, &ctx, &tmp_mem); if (unlikely(r)) { goto out_cleanup; }
@@ -445,8 +445,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
if (r) { memcpy:
r = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, new_mem);
r = ttm_bo_move_memcpy(bo, ctx, new_mem);diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 70b2673..17fe8be 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -324,13 +324,11 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
if (!(old_man->flags & TTM_MEMTYPE_FLAG_FIXED) && !(new_man->flags & TTM_MEMTYPE_FLAG_FIXED))
ret = ttm_bo_move_ttm(bo, ctx->interruptible,ctx->no_wait_gpu, mem);
ret = ttm_bo_move_ttm(bo, ctx, mem);
ret = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, mem);
ret = ttm_bo_move_memcpy(bo, ctx, mem);if (ret) { if (bdev->driver->move_notify) {
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index e7a519f..9237099 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -45,7 +45,7 @@ void ttm_bo_free_old_node(struct ttm_buffer_object *bo) }
int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx,@@ -53,7 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo, int ret;
if (old_mem->mem_type != TTM_PL_SYSTEM) {
ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);if (unlikely(ret != 0)) { if (ret != -ERESTARTSYS)
@@ -329,7 +329,7 @@ static int ttm_copy_ttm_io_page(struct ttm_tt *ttm, void *dst, }
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem)@@ -345,7 +345,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, unsigned long add = 0; int dir;
- ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
- ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu); if (ret) return ret;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 6996d88..5115718 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -976,7 +976,7 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev, */
int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx,struct ttm_mem_reg *new_mem);
/**
@@ -998,7 +998,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo, */
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem);/**
-----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Tuesday, December 12, 2017 6:34 PM To: He, Roger Hongbo.He@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH 3/6] drm/ttm: use an ttm operation ctx for ttm_bo_move_xxx
Am 12.12.2017 um 10:33 schrieb Roger He:
include ttm_bo_move_memcpy and ttm_bo_move_ttm
Change-Id: I160b2fe1da3200405810d0215c4521b5f0d3615a Signed-off-by: Roger He Hongbo.He@amd.com
Reviewed-by: Christian König christian.koenig@amd.com
But please separate that out and wait for a few days before committing, maybe some nouveau devs have objections.
Ok.
Thanks Roger(Hongbo.He)
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++---- drivers/gpu/drm/nouveau/nouveau_bo.c | 7 +++---- drivers/gpu/drm/qxl/qxl_ttm.c | 3 +-- drivers/gpu/drm/radeon/radeon_ttm.c | 7 +++---- drivers/gpu/drm/ttm/ttm_bo.c | 6 ++---- drivers/gpu/drm/ttm/ttm_bo_util.c | 8 ++++---- include/drm/ttm/ttm_bo_driver.h | 4 ++-- 7 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 7db9556..c307a7d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -505,7 +505,7 @@ static int amdgpu_move_vram_ram(struct ttm_buffer_object *bo, bool evict, if (unlikely(r)) { goto out_cleanup; }
- r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, new_mem);
- r = ttm_bo_move_ttm(bo, ctx, new_mem); out_cleanup: ttm_bo_mem_put(bo, &tmp_mem); return r;
@@ -536,7 +536,7 @@ static int amdgpu_move_ram_vram(struct ttm_buffer_object *bo, bool evict, if (unlikely(r)) { return r; }
- r = ttm_bo_move_ttm(bo, ctx->interruptible, ctx->no_wait_gpu, &tmp_mem);
- r = ttm_bo_move_ttm(bo, ctx, &tmp_mem); if (unlikely(r)) { goto out_cleanup; }
@@ -597,8 +597,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
if (r) { memcpy:
r = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, new_mem);
r = ttm_bo_move_memcpy(bo, ctx, new_mem);diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 949bf6b..6b6fb20 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -1226,7 +1226,7 @@ nouveau_bo_move_flipd(struct ttm_buffer_object *bo, bool evict, bool intr, if (ret) goto out;
- ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, new_reg);
- ret = ttm_bo_move_ttm(bo, &ctx, new_reg); out: ttm_bo_mem_put(bo, &tmp_reg); return ret;
@@ -1255,7 +1255,7 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo, bool evict, bool intr, if (ret) return ret;
- ret = ttm_bo_move_ttm(bo, intr, no_wait_gpu, &tmp_reg);
- ret = ttm_bo_move_ttm(bo, &ctx, &tmp_reg); if (ret) goto out;
@@ -1380,8 +1380,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict, /* Fallback to software copy. */ ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu); if (ret == 0)
ret = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, new_reg);
ret = ttm_bo_move_memcpy(bo, ctx, new_reg);out: if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) {
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index d866f32..78ce118 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -357,8 +357,7 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict, qxl_move_null(bo, new_mem); return 0; }
- return ttm_bo_move_memcpy(bo, ctx->interruptible, ctx->no_wait_gpu,
new_mem);
return ttm_bo_move_memcpy(bo, ctx, new_mem); }
static void qxl_bo_move_notify(struct ttm_buffer_object *bo, diff
--git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 98e30d7..557fd79 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -347,7 +347,7 @@ static int radeon_move_vram_ram(struct ttm_buffer_object *bo, if (unlikely(r)) { goto out_cleanup; }
- r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, new_mem);
- r = ttm_bo_move_ttm(bo, &ctx, new_mem); out_cleanup: ttm_bo_mem_put(bo, &tmp_mem); return r;
@@ -380,7 +380,7 @@ static int radeon_move_ram_vram(struct ttm_buffer_object *bo, if (unlikely(r)) { return r; }
- r = ttm_bo_move_ttm(bo, interruptible, no_wait_gpu, &tmp_mem);
- r = ttm_bo_move_ttm(bo, &ctx, &tmp_mem); if (unlikely(r)) { goto out_cleanup; }
@@ -445,8 +445,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
if (r) { memcpy:
r = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, new_mem);
r = ttm_bo_move_memcpy(bo, ctx, new_mem);diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 70b2673..17fe8be 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -324,13 +324,11 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
if (!(old_man->flags & TTM_MEMTYPE_FLAG_FIXED) && !(new_man->flags & TTM_MEMTYPE_FLAG_FIXED))
ret = ttm_bo_move_ttm(bo, ctx->interruptible,ctx->no_wait_gpu, mem);
ret = ttm_bo_move_ttm(bo, ctx, mem);
ret = ttm_bo_move_memcpy(bo, ctx->interruptible,ctx->no_wait_gpu, mem);
ret = ttm_bo_move_memcpy(bo, ctx, mem);if (ret) { if (bdev->driver->move_notify) {
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index e7a519f..9237099 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -45,7 +45,7 @@ void ttm_bo_free_old_node(struct ttm_buffer_object *bo) }
int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx,@@ -53,7 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo, int ret;
if (old_mem->mem_type != TTM_PL_SYSTEM) {
ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);if (unlikely(ret != 0)) { if (ret != -ERESTARTSYS)
@@ -329,7 +329,7 @@ static int ttm_copy_ttm_io_page(struct ttm_tt *ttm, void *dst, }
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem)ttm_bo_move_memcpy(struct ttm_buffer_object *bo, unsigned long add = 0; int dir;
- ret = ttm_bo_wait(bo, interruptible, no_wait_gpu);
- ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu); if (ret) return ret;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 6996d88..5115718 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -976,7 +976,7 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev, */
int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx,struct ttm_mem_reg *new_mem);
/**
@@ -998,7 +998,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo, */
int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
bool interruptible, bool no_wait_gpu,
struct ttm_operation_ctx *ctx, struct ttm_mem_reg *new_mem);/**
Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe Signed-off-by: Roger He Hongbo.He@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 17fe8be..eb8c568 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, place)) { if (locked) reservation_object_unlock(bo->resv); + locked = false; continue; } break;
Am 12.12.2017 um 10:33 schrieb Roger He:
Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe Signed-off-by: Roger He Hongbo.He@amd.com
Reviewed-by: Christian König christian.koenig@amd.com
That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.
Christian.
drivers/gpu/drm/ttm/ttm_bo.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 17fe8be..eb8c568 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, place)) { if (locked) reservation_object_unlock(bo->resv);
locked = false; continue; } break;
That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.
Fortunately so far there is no issue directly resulted from that.
Thanks Roger(Hongbo.He)
-----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Tuesday, December 12, 2017 6:37 PM To: He, Roger Hongbo.He@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock
Am 12.12.2017 um 10:33 schrieb Roger He:
Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe Signed-off-by: Roger He Hongbo.He@amd.com
Reviewed-by: Christian König christian.koenig@amd.com
That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.
Christian.
drivers/gpu/drm/ttm/ttm_bo.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 17fe8be..eb8c568 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, place)) { if (locked) reservation_object_unlock(bo->resv);
locked = false; continue; } break;
Am 13.12.2017 um 03:06 schrieb He, Roger:
That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.
Fortunately so far there is no issue directly resulted from that.
Yeah, but that is irrelevant. Patches are classified as fix if they fix something, not if the bug was ever hit.
Not sure if the code is already upstream, but we should still make sure that we send this patch to the appropriate places.
Christian.
Thanks Roger(Hongbo.He)
-----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Tuesday, December 12, 2017 6:37 PM To: He, Roger Hongbo.He@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH 4/6] drm/ttm: init locked again to prevent incorrect unlock
Am 12.12.2017 um 10:33 schrieb Roger He:
Change-Id: I8db51d843955f5db14bb4bbff892eaedbd9f0abe Signed-off-by: Roger He Hongbo.He@amd.com
Reviewed-by: Christian König christian.koenig@amd.com
That is a bug fix, isn't it? If yes maybe add CC:stable and commit it first before all other patches.
Christian.
drivers/gpu/drm/ttm/ttm_bo.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 17fe8be..eb8c568 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -735,6 +735,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, place)) { if (locked) reservation_object_unlock(bo->resv);
locked = false; continue; } break;
Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6 Signed-off-by: Roger He Hongbo.He@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index eb8c568..22b6ca5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, spin_lock(&glob->lru_lock); for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { list_for_each_entry(bo, &man->lru[i], lru) { - if (bo->resv == resv) { - if (list_empty(&bo->ddestroy)) - continue; - } else { + if (!ctx || + !(ctx->on_alloc_stage && + bo->resv == ctx->resv)) { locked = reservation_object_trylock(bo->resv); if (!locked) continue;
Am 12.12.2017 um 10:33 schrieb Roger He:
Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6 Signed-off-by: Roger He Hongbo.He@amd.com
I would squash this one with patch #6.
drivers/gpu/drm/ttm/ttm_bo.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index eb8c568..22b6ca5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, spin_lock(&glob->lru_lock); for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { list_for_each_entry(bo, &man->lru[i], lru) {
if (bo->resv == resv) {if (list_empty(&bo->ddestroy))continue;} else {
if (!ctx ||!(ctx->on_alloc_stage &&bo->resv == ctx->resv)) {
Coding style: The lines stating with "!(ctx" and "bo->resv" are to far to the right.
Additional to that I think ctx is mandatory and doesn't need a check (but might be wrong). If it isn't it's probably time to make it mandatory.
And I would use (ctx->on_alloc_stage || list_empty(&bo->ddestroy)) as check, we probably still want to be able to handle deleted BOs here during CS.
Christian.
locked = reservation_object_trylock(bo->resv); if (!locked) continue;
-----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Tuesday, December 12, 2017 6:40 PM To: He, Roger Hongbo.He@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO
Am 12.12.2017 um 10:33 schrieb Roger He:
Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6 Signed-off-by: Roger He Hongbo.He@amd.com
I would squash this one with patch #6.
drivers/gpu/drm/ttm/ttm_bo.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index eb8c568..22b6ca5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, spin_lock(&glob->lru_lock); for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { list_for_each_entry(bo, &man->lru[i], lru) {
if (bo->resv == resv) {if (list_empty(&bo->ddestroy))continue;} else {
if (!ctx ||!(ctx->on_alloc_stage &&bo->resv == ctx->resv)) {
Coding style: The lines stating with "!(ctx" and "bo->resv" are to far to the right.
Additional to that I think ctx is mandatory and doesn't need a check (but might be wrong). If it isn't it's probably time to make it mandatory.
And I would use (ctx->on_alloc_stage || list_empty(&bo->ddestroy)) as check, we probably still want to be able to handle deleted BOs here during CS.
Change that as below, I think it can cover the case you described. No matter the Bo is in deleted list or not. if (!ctx->allow_reserved_eviction || bo->resv != ctx->resv) { locked = kcl_reservation_object_trylock(bo->resv); if (!locked) continue; }
Thanks Roger(Hongbo.He)
Christian.
locked = reservation_object_trylock(bo->resv); if (!locked) continue;
Change-Id: I83ac6a77f24e14698aa386a497a262e24c5bbdb6 Signed-off-by: Roger He Hongbo.He@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 22b6ca5..f138e95 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -707,7 +707,6 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, EXPORT_SYMBOL(ttm_bo_eviction_valuable);
static int ttm_mem_evict_first(struct ttm_bo_device *bdev, - struct reservation_object *resv, uint32_t mem_type, const struct ttm_place *place, struct ttm_operation_ctx *ctx) @@ -834,7 +833,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo, return ret; if (mem->mm_node) break; - ret = ttm_mem_evict_first(bdev, bo->resv, mem_type, place, ctx); + ret = ttm_mem_evict_first(bdev, mem_type, place, ctx); if (unlikely(ret != 0)) return ret; } while (1); @@ -1332,8 +1331,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev, for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { while (!list_empty(&man->lru[i])) { spin_unlock(&glob->lru_lock); - ret = ttm_mem_evict_first(bdev, NULL, mem_type, - NULL, &ctx); + ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx); if (ret) return ret; spin_lock(&glob->lru_lock);
Am 12.12.2017 um 10:33 schrieb Roger He:
on_alloc_stage: is this operation on allocation stage resv: reservation bo used of this operation
Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0 Signed-off-by: Roger He Hongbo.He@amd.com
include/drm/ttm/ttm_bo_api.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
- @interruptible: Sleep interruptible if sleeping.
- @no_wait_gpu: Return immediately if the GPU is busy.
- @on_alloc_stage: is this operation on allocation stage
- @resv: resvation bo used
- Context for TTM operations like changing buffer placement or general memory
- allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj { struct ttm_operation_ctx { bool interruptible; bool no_wait_gpu;
- bool on_alloc_stage;
The name describes how amdgpu want to use the flag and not what effect it has.
How about "allow_reserved_eviction"?
And also please update the documentation with something like: * @allow_reserved_eviction: Allow eviction of reserved BOs. * @resv: Reservation object to allow reserved evictions with.
Alternative we could put the ww_mutex context in here as Daniel suggested, but I think we should stick with what we have for now and make that change when we find another use case for this.
Christian.
- struct reservation_object *resv; uint64_t bytes_moved; };
Hi,
I think this series is quite poorly documented. We should have a log message explaining the purpose of the commit. Also since it's not obvious what the series is attempting to achieve, please add a 0/X series header message..
/Thomas
On 12/12/2017 10:33 AM, Roger He wrote:
on_alloc_stage: is this operation on allocation stage resv: reservation bo used of this operation
Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0 Signed-off-by: Roger He Hongbo.He@amd.com
include/drm/ttm/ttm_bo_api.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
- @interruptible: Sleep interruptible if sleeping.
- @no_wait_gpu: Return immediately if the GPU is busy.
- @on_alloc_stage: is this operation on allocation stage
- @resv: resvation bo used
- Context for TTM operations like changing buffer placement or general memory
- allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj { struct ttm_operation_ctx { bool interruptible; bool no_wait_gpu;
- bool on_alloc_stage;
- struct reservation_object *resv; uint64_t bytes_moved; };
Hi Thomas,
sorry for that. Noted on the rest of that series as well that we need to improve the commit messages. But this one somehow slipped through because I discussed this change previously internally with Roger.
That made the change completely logical for me, but without this context everybody else just thinks "Hui what?". Going to keep that in mind the next time.
But back to topic: This series allows BOs which share the same reservation object as the BO currently allocated/validated to be evicted even when they are reserved.
This is useful because amdgpu wants to use a single reservation object for almost all BOs of a process.
Regards, Christian.
Am 13.12.2017 um 20:21 schrieb Thomas Hellstrom:
Hi,
I think this series is quite poorly documented. We should have a log message explaining the purpose of the commit. Also since it's not obvious what the series is attempting to achieve, please add a 0/X series header message..
/Thomas
On 12/12/2017 10:33 AM, Roger He wrote:
on_alloc_stage: is this operation on allocation stage resv: reservation bo used of this operation
Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0 Signed-off-by: Roger He Hongbo.He@amd.com
include/drm/ttm/ttm_bo_api.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj { * * @interruptible: Sleep interruptible if sleeping. * @no_wait_gpu: Return immediately if the GPU is busy.
- @on_alloc_stage: is this operation on allocation stage
- @resv: resvation bo used
* * Context for TTM operations like changing buffer placement or general memory * allocation. @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj { struct ttm_operation_ctx { bool interruptible; bool no_wait_gpu; + bool on_alloc_stage; + struct reservation_object *resv; uint64_t bytes_moved; };
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Hi, Christian,
On 12/14/2017 09:40 AM, Christian König wrote:
Hi Thomas,
sorry for that. Noted on the rest of that series as well that we need to improve the commit messages. But this one somehow slipped through because I discussed this change previously internally with Roger.
That made the change completely logical for me, but without this context everybody else just thinks "Hui what?". Going to keep that in mind the next time.
But back to topic: This series allows BOs which share the same reservation object as the BO currently allocated/validated to be evicted even when they are reserved.
This is useful because amdgpu wants to use a single reservation object for almost all BOs of a process.
Yes, that indeed makes the whole thing more clear, and makes sense.
Out of interest, is the shared reservation object usage a speed optimization (avoiding the ww_mutex_locks at reservation time?) or something else?
I guess that even if LRU lists might get crowded with unevictable BOs, iterating through those lists isn't really part of the fast path. /Thomas
Regards, Christian.
Am 14.12.2017 um 09:55 schrieb Thomas Hellstrom:
Hi, Christian,
On 12/14/2017 09:40 AM, Christian König wrote:
Hi Thomas,
sorry for that. Noted on the rest of that series as well that we need to improve the commit messages. But this one somehow slipped through because I discussed this change previously internally with Roger.
That made the change completely logical for me, but without this context everybody else just thinks "Hui what?". Going to keep that in mind the next time.
But back to topic: This series allows BOs which share the same reservation object as the BO currently allocated/validated to be evicted even when they are reserved.
This is useful because amdgpu wants to use a single reservation object for almost all BOs of a process.
Yes, that indeed makes the whole thing more clear, and makes sense.
Out of interest, is the shared reservation object usage a speed optimization (avoiding the ww_mutex_locks at reservation time?) or something else?
Avoiding taking many ww_mutex_locks is one reason. The other major reason comes with GPU-VM page tables.
Just the same as CPU page tables multi level GPU page tables are allocated individually when needed. But during command submission we must make sure that all GPU page tables are validated and fences added to all of them.
Because of this we are using the reservation object of the root page directory (because that one is always allocated) as reservation object for all other page tables.
The effect is that you have to add the resulting fence of a command submission to only one reservation object and not a couple of hundreds or even thousands.
I guess that even if LRU lists might get crowded with unevictable BOs, iterating through those lists isn't really part of the fast path.
Yes, exactly. When we start to massively evict things performance goes down so much anyway that this extra cycling over the LRU doesn't hurt us much.
Christian.
/Thomas
Regards, Christian.
Hi Thomas:
Really sorry for that and will keep that in mind. If necessary, next time I will send cover letter to provide more background and details.
Thanks Roger(Hongbo.He)
-----Original Message----- From: Thomas Hellstrom [mailto:thomas@shipmail.org] Sent: Thursday, December 14, 2017 3:21 AM To: He, Roger Hongbo.He@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx
Hi,
I think this series is quite poorly documented. We should have a log message explaining the purpose of the commit. Also since it's not obvious what the series is attempting to achieve, please add a 0/X series header message..
/Thomas
On 12/12/2017 10:33 AM, Roger He wrote:
on_alloc_stage: is this operation on allocation stage resv: reservation bo used of this operation
Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0 Signed-off-by: Roger He Hongbo.He@amd.com
include/drm/ttm/ttm_bo_api.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
- @interruptible: Sleep interruptible if sleeping.
- @no_wait_gpu: Return immediately if the GPU is busy.
- @on_alloc_stage: is this operation on allocation stage
- @resv: resvation bo used
- Context for TTM operations like changing buffer placement or general memory
- allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj { struct ttm_operation_ctx { bool interruptible; bool no_wait_gpu;
- bool on_alloc_stage;
- struct reservation_object *resv; uint64_t bytes_moved; };
On Tue, Dec 12, 2017 at 10:34 AM Roger He Hongbo.He@amd.com wrote:
on_alloc_stage: is this operation on allocation stage resv: reservation bo used of this operation
Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0 Signed-off-by: Roger He Hongbo.He@amd.com
Real commit message (the later patches using this are even sparser) and/or proper kerneldoc would be massively appreciated in common code. You guys have done a ton of stuff and special cases just for amdgpu, and if someone doesn't at least know what you aimed to do it's pretty much impossible to understand. I don't care much if this is the standard in drivers, but common code really should bother to explain what problem it's trying to solve.
(yes I think I figured out what it's doing, but I'm pretty sure most dri-devel folks hacking on gem/cs/render stuff won't be so lucky) -Daniel
include/drm/ttm/ttm_bo_api.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
- @interruptible: Sleep interruptible if sleeping.
- @no_wait_gpu: Return immediately if the GPU is busy.
- @on_alloc_stage: is this operation on allocation stage
- @resv: resvation bo used
- Context for TTM operations like changing buffer placement or general memory
- allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj { struct ttm_operation_ctx { bool interruptible; bool no_wait_gpu;
bool on_alloc_stage;struct reservation_object *resv; uint64_t bytes_moved;};
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Christian König -
Christian König -
Daniel Vetter -
He, Roger -
Roger He
-
Thomas Hellstrom