From: Christian König christian.koenig@amd.com
If we import a BO with an external reservation object we don't reserve/unreserve it. So we never add it to the LRU causing a possible deny of service.
v2: fix typo in commit message
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 745e996..a98a5d5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1170,9 +1170,15 @@ int ttm_bo_init(struct ttm_bo_device *bdev, if (likely(!ret)) ret = ttm_bo_validate(bo, placement, interruptible, false);
- if (!resv) + if (!resv) { ttm_bo_unreserve(bo);
+ } else if (!(bo->mem.placement & TTM_PL_FLAG_NO_EVICT)) { + spin_lock(&bo->glob->lru_lock); + ttm_bo_add_to_lru(bo); + spin_unlock(&bo->glob->lru_lock); + } + if (unlikely(ret)) ttm_bo_unref(&bo);
From: Christian König christian.koenig@amd.com
It doesn't make any sense to try to swap out imported BOs.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index a98a5d5..3ea9a01 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -176,7 +176,7 @@ void ttm_bo_add_to_lru(struct ttm_buffer_object *bo) list_add_tail(&bo->lru, &man->lru); kref_get(&bo->list_kref);
- if (bo->ttm != NULL) { + if (bo->ttm && !(bo->ttm->page_flags & TTM_PAGE_FLAG_SG)) { list_add_tail(&bo->swap, &bo->glob->swap_lru); kref_get(&bo->list_kref); }
Reviewed-by: Thomas Hellstrom thellstrom@vmware.com
BTW, does Radeon touch the caching mode of imported buffers through TTM (like write-combining)? Do we need something similar for that?
/Thomas
On 01/11/2016 03:35 PM, Christian König wrote:
From: Christian König christian.koenig@amd.com
It doesn't make any sense to try to swap out imported BOs.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/ttm/ttm_bo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index a98a5d5..3ea9a01 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -176,7 +176,7 @@ void ttm_bo_add_to_lru(struct ttm_buffer_object *bo) list_add_tail(&bo->lru, &man->lru); kref_get(&bo->list_kref);
if (bo->ttm != NULL) {
}if (bo->ttm && !(bo->ttm->page_flags & TTM_PAGE_FLAG_SG)) { list_add_tail(&bo->swap, &bo->glob->swap_lru); kref_get(&bo->list_kref);
From: Christian König christian.koenig@amd.com
This allows the drivers to move a BO to the end of the LRU without removing and adding it again.
v2: Make it more robust, handle pinned and swapable BOs as well.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 21 +++++++++++++++++++++ include/drm/ttm/ttm_bo_api.h | 10 ++++++++++ 2 files changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 3ea9a01..4cbf265 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -228,6 +228,27 @@ void ttm_bo_del_sub_from_lru(struct ttm_buffer_object *bo) } EXPORT_SYMBOL(ttm_bo_del_sub_from_lru);
+void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo) +{ + struct ttm_bo_device *bdev = bo->bdev; + struct ttm_mem_type_manager *man; + + lockdep_assert_held(&bo->resv->lock.base); + + if (bo->mem.placement & TTM_PL_FLAG_NO_EVICT) { + list_del_init(&bo->swap); + list_del_init(&bo->lru); + + } else { + if (bo->ttm && !(bo->ttm->page_flags & TTM_PAGE_FLAG_SG)) + list_move_tail(&bo->swap, &bo->glob->swap_lru); + + man = &bdev->man[bo->mem.mem_type]; + list_move_tail(&bo->lru, &man->lru); + } +} +EXPORT_SYMBOL(ttm_bo_move_to_lru_tail); + /* * Call bo->mutex locked. */ diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index c768ddf..afae231 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -383,6 +383,16 @@ extern void ttm_bo_add_to_lru(struct ttm_buffer_object *bo); */ extern int ttm_bo_del_from_lru(struct ttm_buffer_object *bo);
+/** + * ttm_bo_move_to_lru_tail + * + * @bo: The buffer object. + * + * Move this BO to the tail of all lru lists used to lookup and reserve an + * object. This function must be called with struct ttm_bo_global::lru_lock + * held, and is used to make a BO less likely to be considered for eviction. + */ +extern void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo);
/** * ttm_bo_lock_delayed_workqueue
LGTM,
Although perhaps check that the LRU lock is held as well.
Reviewed-by: Thomas Hellstrom thellstrom@vmware.com
On 01/11/2016 03:35 PM, Christian König wrote:
From: Christian König christian.koenig@amd.com
This allows the drivers to move a BO to the end of the LRU without removing and adding it again.
v2: Make it more robust, handle pinned and swapable BOs as well.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/ttm/ttm_bo.c | 21 +++++++++++++++++++++ include/drm/ttm/ttm_bo_api.h | 10 ++++++++++ 2 files changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 3ea9a01..4cbf265 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -228,6 +228,27 @@ void ttm_bo_del_sub_from_lru(struct ttm_buffer_object *bo) } EXPORT_SYMBOL(ttm_bo_del_sub_from_lru);
+void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo) +{
- struct ttm_bo_device *bdev = bo->bdev;
- struct ttm_mem_type_manager *man;
- lockdep_assert_held(&bo->resv->lock.base);
- if (bo->mem.placement & TTM_PL_FLAG_NO_EVICT) {
list_del_init(&bo->swap);
list_del_init(&bo->lru);
- } else {
if (bo->ttm && !(bo->ttm->page_flags & TTM_PAGE_FLAG_SG))
list_move_tail(&bo->swap, &bo->glob->swap_lru);
man = &bdev->man[bo->mem.mem_type];
list_move_tail(&bo->lru, &man->lru);
- }
+} +EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
/*
- Call bo->mutex locked.
*/ diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index c768ddf..afae231 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -383,6 +383,16 @@ extern void ttm_bo_add_to_lru(struct ttm_buffer_object *bo); */ extern int ttm_bo_del_from_lru(struct ttm_buffer_object *bo);
+/**
- ttm_bo_move_to_lru_tail
- @bo: The buffer object.
- Move this BO to the tail of all lru lists used to lookup and reserve an
- object. This function must be called with struct ttm_bo_global::lru_lock
- held, and is used to make a BO less likely to be considered for eviction.
- */
+extern void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo);
/**
- ttm_bo_lock_delayed_workqueue
From: Christian König christian.koenig@amd.com
This makes it less likely to run into an ENOMEM because VM page tables are evicted last.
v2: move the BOs in the LRU tail after validation
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 7 ++++++- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 27 +++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 003959f..313b0cc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -987,6 +987,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, struct list_head *validated, struct amdgpu_bo_list_entry *entry); void amdgpu_vm_get_pt_bos(struct amdgpu_vm *vm, struct list_head *duplicates); +void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev, + struct amdgpu_vm *vm); int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring, struct amdgpu_sync *sync); void amdgpu_vm_flush(struct amdgpu_ring *ring, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index ce0254d..1fffc33 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -428,8 +428,10 @@ static int amdgpu_cs_parser_relocs(struct amdgpu_cs_parser *p) r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &duplicates);
error_validate: - if (r) + if (r) { + amdgpu_vm_move_pt_bos_in_lru(p->adev, &fpriv->vm); ttm_eu_backoff_reservation(&p->ticket, &p->validated); + }
error_reserve: if (need_mmap_lock) @@ -473,8 +475,11 @@ static int cmp_size_smaller_first(void *priv, struct list_head *a, **/ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error, bool backoff) { + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; unsigned i;
+ amdgpu_vm_move_pt_bos_in_lru(parser->adev, &fpriv->vm); + if (!error) { /* Sort the buffer list from the smallest to largest buffer, * which affects the order of buffers in the LRU list. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 8f7688e..aefc668 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -119,6 +119,33 @@ void amdgpu_vm_get_pt_bos(struct amdgpu_vm *vm, struct list_head *duplicates)
list_add(&entry->tv.head, duplicates); } + +} + +/** + * amdgpu_vm_move_pt_bos_in_lru - move the PT BOs to the LRU tail + * + * @adev: amdgpu device instance + * @vm: vm providing the BOs + * + * Move the PT BOs to the tail of the LRU. + */ +void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev, + struct amdgpu_vm *vm) +{ + struct ttm_bo_global *glob = adev->mman.bdev.glob; + unsigned i; + + spin_lock(&glob->lru_lock); + for (i = 0; i <= vm->max_pde_used; ++i) { + struct amdgpu_bo_list_entry *entry = &vm->page_tables[i].entry; + + if (!entry->robj) + continue; + + ttm_bo_move_to_lru_tail(&entry->robj->tbo); + } + spin_unlock(&glob->lru_lock); }
/**
From: Christian König christian.koenig@amd.com
Most VM BOs end up in the duplicates list, validate it first make -ENOMEM less likely.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Chunming Zhou David1.Zhou@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 1fffc33..6f89f8e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -421,11 +421,11 @@ static int amdgpu_cs_parser_relocs(struct amdgpu_cs_parser *p)
amdgpu_vm_get_pt_bos(&fpriv->vm, &duplicates);
- r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &p->validated); + r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &duplicates); if (r) goto error_validate;
- r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &duplicates); + r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &p->validated);
error_validate: if (r) {
On Mon, Jan 11, 2016 at 9:35 AM, Christian König deathsimple@vodafone.de wrote:
From: Christian König christian.koenig@amd.com
Most VM BOs end up in the duplicates list, validate it first make -ENOMEM less likely.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Chunming Zhou David1.Zhou@amd.com
Applied the series. thanks!
Alex
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 1fffc33..6f89f8e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -421,11 +421,11 @@ static int amdgpu_cs_parser_relocs(struct amdgpu_cs_parser *p)
amdgpu_vm_get_pt_bos(&fpriv->vm, &duplicates);
r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &p->validated);
r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &duplicates); if (r) goto error_validate;
r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &duplicates);
r = amdgpu_cs_list_validate(p->adev, &fpriv->vm, &p->validated);
error_validate: if (r) { -- 2.5.0
LGTM, minor nitpick below.
Reviewed-by: Thomas Hellstrom thellstrom@vmware.com
On 01/11/2016 03:35 PM, Christian König wrote:
From: Christian König christian.koenig@amd.com
If we import a BO with an external reservation object we don't reserve/unreserve it. So we never add it to the LRU causing a possible deny of service.
s/deny/denial/
v2: fix typo in commit message
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/ttm/ttm_bo.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 745e996..a98a5d5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1170,9 +1170,15 @@ int ttm_bo_init(struct ttm_bo_device *bdev, if (likely(!ret)) ret = ttm_bo_validate(bo, placement, interruptible, false);
- if (!resv)
if (!resv) { ttm_bo_unreserve(bo);
} else if (!(bo->mem.placement & TTM_PL_FLAG_NO_EVICT)) {
spin_lock(&bo->glob->lru_lock);
ttm_bo_add_to_lru(bo);
spin_unlock(&bo->glob->lru_lock);
}
if (unlikely(ret)) ttm_bo_unref(&bo);
dri-devel@lists.freedesktop.org