The idea and proposal is originally from Christian, and I continue to work to deliver it.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Thanks, Rui
Christian König (2): drm/ttm: add helper structures for bulk moves on lru list drm/ttm: revise ttm_bo_move_to_lru_tail to support bulk moves
Huang Rui (3): drm/ttm: add bulk move function on LRU drm/amdgpu: use bulk moves for efficient VM LRU handling drm/amdgpu: move PD/PT bos on LRU again
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 24 +++++++---- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++ drivers/gpu/drm/ttm/ttm_bo.c | 78 +++++++++++++++++++++++++++++++++- include/drm/ttm/ttm_bo_api.h | 16 ++++++- include/drm/ttm/ttm_bo_driver.h | 28 ++++++++++++ 5 files changed, 139 insertions(+), 11 deletions(-)
From: Christian König christian.koenig@amd.com
Add bulk move pos to store the pointer of first and last buffer object. The list in between will be bulk moved on lru list.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com --- include/drm/ttm/ttm_bo_driver.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 3234cc3..e4fee8e 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -491,6 +491,34 @@ struct ttm_bo_device { };
/** + * struct ttm_lru_bulk_move_pos + * + * @first: first BO in the bulk move range + * @last: last BO in the bulk move range + * + * Positions for a lru bulk move. + */ +struct ttm_lru_bulk_move_pos { + struct ttm_buffer_object *first; + struct ttm_buffer_object *last; +}; + +/** + * struct ttm_lru_bulk_move + * + * @tt: first/last lru entry for BOs in the TT domain + * @vram: first/last lru entry for BOs in the VRAM domain + * @swap: first/last lru entry for BOs on the swap list + * + * Helper structure for bulk moves on the LRU list. + */ +struct ttm_lru_bulk_move { + struct ttm_lru_bulk_move_pos tt[TTM_MAX_BO_PRIORITY]; + struct ttm_lru_bulk_move_pos vram[TTM_MAX_BO_PRIORITY]; + struct ttm_lru_bulk_move_pos swap[TTM_MAX_BO_PRIORITY]; +}; + +/** * ttm_flag_masked * * @old: Pointer to the result and original value.
From: Christian König christian.koenig@amd.com
When move a BO to the end of LRU, it need remember the BO positions. Make sure all moved bo in between "first" and "last". And they will be bulk moving together.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++---- drivers/gpu/drm/ttm/ttm_bo.c | 26 +++++++++++++++++++++++++- include/drm/ttm/ttm_bo_api.h | 6 +++++- 3 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 015613b..9c84770 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -297,9 +297,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, break;
spin_lock(&glob->lru_lock); - ttm_bo_move_to_lru_tail(&bo->tbo); + ttm_bo_move_to_lru_tail(&bo->tbo, NULL); if (bo->shadow) - ttm_bo_move_to_lru_tail(&bo->shadow->tbo); + ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL); spin_unlock(&glob->lru_lock); }
@@ -319,9 +319,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
- ttm_bo_move_to_lru_tail(&bo->tbo); + ttm_bo_move_to_lru_tail(&bo->tbo, NULL); if (bo->shadow) - ttm_bo_move_to_lru_tail(&bo->shadow->tbo); + ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL); } spin_unlock(&glob->lru_lock);
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 7c48472..7117b6b 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -214,12 +214,36 @@ 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) +static void ttm_bo_bulk_move_set_pos(struct ttm_lru_bulk_move_pos *pos, + struct ttm_buffer_object *bo) +{ + if (!pos->first) + pos->first = bo; + pos->last = bo; +} + +void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo, + struct ttm_lru_bulk_move *bulk) { reservation_object_assert_held(bo->resv);
ttm_bo_del_from_lru(bo); ttm_bo_add_to_lru(bo); + + if (bulk && !(bo->mem.placement & TTM_PL_FLAG_NO_EVICT)) { + switch (bo->mem.mem_type) { + case TTM_PL_TT: + ttm_bo_bulk_move_set_pos(&bulk->tt[bo->priority], bo); + break; + + case TTM_PL_VRAM: + ttm_bo_bulk_move_set_pos(&bulk->vram[bo->priority], bo); + break; + } + if (bo->ttm && !(bo->ttm->page_flags & + (TTM_PAGE_FLAG_SG | TTM_PAGE_FLAG_SWAPPED))) + ttm_bo_bulk_move_set_pos(&bulk->swap[bo->priority], bo); + } } EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index a01ba20..0d4eb81 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -51,6 +51,8 @@ struct ttm_placement;
struct ttm_place;
+struct ttm_lru_bulk_move; + /** * struct ttm_bus_placement * @@ -405,12 +407,14 @@ void ttm_bo_del_from_lru(struct ttm_buffer_object *bo); * ttm_bo_move_to_lru_tail * * @bo: The buffer object. + * @bulk: optional bulk move structure to remember BO positions * * 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. */ -void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo); +void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo, + struct ttm_lru_bulk_move *bulk);
/** * ttm_bo_lock_delayed_workqueue
This function allow us to bulk move a group of BOs to the tail of their LRU. The positions of group of BOs are stored on the (first, last) bulk_move_pos structure.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com --- drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/ttm/ttm_bo_api.h | 10 +++++++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 7117b6b..39d9d55 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -247,6 +247,58 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo, } EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
+static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos, + struct list_head *lru, bool is_swap) +{ + struct list_head entries, before; + struct list_head *list1, *list2; + + list1 = is_swap ? &pos->last->swap : &pos->last->lru; + list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev; + + list_cut_position(&entries, lru, list1); + list_cut_position(&before, &entries, list2); + list_splice(&before, lru); + list_splice_tail(&entries, lru); +} + +void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk) +{ + unsigned i; + + for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { + struct ttm_mem_type_manager *man; + + if (!bulk->tt[i].first) + continue; + + man = &bulk->tt[i].first->bdev->man[TTM_PL_TT]; + ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false); + } + + for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { + struct ttm_mem_type_manager *man; + + if (!bulk->vram[i].first) + continue; + + man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM]; + ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false); + } + + for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { + struct ttm_lru_bulk_move_pos *pos = &bulk->swap[i]; + struct list_head *lru; + + if (!pos->first) + continue; + + lru = &pos->first->bdev->glob->swap_lru[i]; + ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true); + } +} +EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail); + static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem, bool evict, struct ttm_operation_ctx *ctx) diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 0d4eb81..8c19470 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -417,6 +417,16 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo, struct ttm_lru_bulk_move *bulk);
/** + * ttm_bo_bulk_move_lru_tail + * + * @bulk: bulk move structure + * + * Bulk move BOs to the LRU tail, only valid to use when driver makes sure that + * BO order never changes. Should be called with ttm_bo_global::lru_lock held. + */ +void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk); + +/** * ttm_bo_lock_delayed_workqueue * * Prevent the delayed workqueue from running.
I continue to work for bulk moving that based on the proposal by Christian.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++++ 2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..eda0bb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -286,6 +286,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, { struct ttm_bo_global *glob = adev->mman.bdev.glob; struct amdgpu_vm_bo_base *bo_base, *tmp; + bool validated = false; int r = 0;
list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) { @@ -295,14 +296,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, r = validate(param, bo); if (r) break; - - spin_lock(&glob->lru_lock); - ttm_bo_move_to_lru_tail(&bo->tbo, NULL); - if (bo->shadow) - ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL); - spin_unlock(&glob->lru_lock); }
+ validated = true; if (bo->tbo.type != ttm_bo_type_kernel) { spin_lock(&vm->moved_lock); list_move(&bo_base->vm_status, &vm->moved); @@ -312,6 +308,15 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } }
+ if (!validated) { + spin_lock(&glob->lru_lock); + ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move); + spin_unlock(&glob->lru_lock); + return 0; + } + + memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move)); + spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo; @@ -319,9 +324,10 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
- ttm_bo_move_to_lru_tail(&bo->tbo, NULL); + ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) - ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL); + ttm_bo_move_to_lru_tail(&bo->shadow->tbo, + &vm->lru_bulk_move); } spin_unlock(&glob->lru_lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 67a15d4..92725ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -29,6 +29,7 @@ #include <linux/rbtree.h> #include <drm/gpu_scheduler.h> #include <drm/drm_file.h> +#include <drm/ttm/ttm_bo_driver.h>
#include "amdgpu_sync.h" #include "amdgpu_ring.h" @@ -226,6 +227,9 @@ struct amdgpu_vm {
/* Some basic info about the task */ struct amdgpu_task_info task_info; + + /* Store positions of group of BOs */ + struct ttm_lru_bulk_move lru_bulk_move; };
struct amdgpu_vm_manager {
Am 08.08.2018 um 11:59 schrieb Huang Rui:
I continue to work for bulk moving that based on the proposal by Christian.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++++ 2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..eda0bb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -286,6 +286,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, { struct ttm_bo_global *glob = adev->mman.bdev.glob; struct amdgpu_vm_bo_base *bo_base, *tmp;
bool validated = false; int r = 0;
list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {
@@ -295,14 +296,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, r = validate(param, bo); if (r) break;
spin_lock(&glob->lru_lock);ttm_bo_move_to_lru_tail(&bo->tbo, NULL);if (bo->shadow)ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);spin_unlock(&glob->lru_lock);
validated = true;@@ -312,6 +308,15 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } }
- if (!validated) {
spin_lock(&glob->lru_lock);ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);spin_unlock(&glob->lru_lock);return 0;- }
- memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
- spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo;
@@ -319,9 +324,10 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo,&vm->lru_bulk_move);
Here is still one problem: The BOs currently evicted are not yet in the idle state.
So this must be moved to the end of the state machine, probably best if we put that into a separate function.
Regards, Christian.
} spin_unlock(&glob->lru_lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 67a15d4..92725ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -29,6 +29,7 @@ #include <linux/rbtree.h> #include <drm/gpu_scheduler.h> #include <drm/drm_file.h> +#include <drm/ttm/ttm_bo_driver.h>
#include "amdgpu_sync.h" #include "amdgpu_ring.h" @@ -226,6 +227,9 @@ struct amdgpu_vm {
/* Some basic info about the task */ struct amdgpu_task_info task_info;
/* Store positions of group of BOs */
struct ttm_lru_bulk_move lru_bulk_move; };
struct amdgpu_vm_manager {
On Wed, Aug 08, 2018 at 06:47:49PM +0800, Christian König wrote:
Am 08.08.2018 um 11:59 schrieb Huang Rui:
I continue to work for bulk moving that based on the proposal by Christian.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++++ 2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..eda0bb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -286,6 +286,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, { struct ttm_bo_global *glob = adev->mman.bdev.glob; struct amdgpu_vm_bo_base *bo_base, *tmp;
bool validated = false; int r = 0;
list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {
@@ -295,14 +296,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, r = validate(param, bo); if (r) break;
spin_lock(&glob->lru_lock);ttm_bo_move_to_lru_tail(&bo->tbo, NULL);if (bo->shadow)ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);spin_unlock(&glob->lru_lock);
validated = true;@@ -312,6 +308,15 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } }
- if (!validated) {
spin_lock(&glob->lru_lock);ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);spin_unlock(&glob->lru_lock);return 0;- }
- memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
- spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo;
@@ -319,9 +324,10 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo,&vm->lru_bulk_move);Here is still one problem: The BOs currently evicted are not yet in the idle state.
So this must be moved to the end of the state machine, probably best if we put that into a separate function.
Thanks, so the evicted BOs should be behind the BOs in idle list that in the group between (first, last) of bulk_move_pos, right?
spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo;
if (!bo->parent) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) { struct amdgpu_bo *bo = bo_base->bo; if (!bo) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } spin_unlock(&glob->lru_lock);
Thanks, Ray
Regards, Christian.
} spin_unlock(&glob->lru_lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 67a15d4..92725ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -29,6 +29,7 @@ #include <linux/rbtree.h> #include <drm/gpu_scheduler.h> #include <drm/drm_file.h> +#include <drm/ttm/ttm_bo_driver.h>
#include "amdgpu_sync.h" #include "amdgpu_ring.h" @@ -226,6 +227,9 @@ struct amdgpu_vm {
/* Some basic info about the task */ struct amdgpu_task_info task_info;
/* Store positions of group of BOs */
struct ttm_lru_bulk_move lru_bulk_move; };
struct amdgpu_vm_manager {
Am 09.08.2018 um 08:18 schrieb Huang Rui:
On Wed, Aug 08, 2018 at 06:47:49PM +0800, Christian König wrote:
Am 08.08.2018 um 11:59 schrieb Huang Rui:
I continue to work for bulk moving that based on the proposal by Christian.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++++ 2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..eda0bb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -286,6 +286,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, { struct ttm_bo_global *glob = adev->mman.bdev.glob; struct amdgpu_vm_bo_base *bo_base, *tmp;
bool validated = false; int r = 0;
list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {
@@ -295,14 +296,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, r = validate(param, bo); if (r) break;
spin_lock(&glob->lru_lock);ttm_bo_move_to_lru_tail(&bo->tbo, NULL);if (bo->shadow)ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);spin_unlock(&glob->lru_lock); }
validated = true; if (bo->tbo.type != ttm_bo_type_kernel) { spin_lock(&vm->moved_lock); list_move(&bo_base->vm_status, &vm->moved);@@ -312,6 +308,15 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } }
- if (!validated) {
spin_lock(&glob->lru_lock);ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);spin_unlock(&glob->lru_lock);return 0;- }
- memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
- spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo;
@@ -319,9 +324,10 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow)
ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo,&vm->lru_bulk_move);Here is still one problem: The BOs currently evicted are not yet in the idle state.
So this must be moved to the end of the state machine, probably best if we put that into a separate function.
Thanks, so the evicted BOs should be behind the BOs in idle list that in the group between (first, last) of bulk_move_pos, right?
The order doesn't matter.
spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo; if (!bo->parent) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {
That won't work. The BOs get removed from the evicted list when they are validated.
You need to walk both the relocated as well as the moved list here.
Christian.
struct amdgpu_bo *bo = bo_base->bo; if (!bo) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } spin_unlock(&glob->lru_lock);Thanks, Ray
Regards, Christian.
} spin_unlock(&glob->lru_lock);diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 67a15d4..92725ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -29,6 +29,7 @@ #include <linux/rbtree.h> #include <drm/gpu_scheduler.h> #include <drm/drm_file.h> +#include <drm/ttm/ttm_bo_driver.h>
#include "amdgpu_sync.h" #include "amdgpu_ring.h" @@ -226,6 +227,9 @@ struct amdgpu_vm {
/* Some basic info about the task */ struct amdgpu_task_info task_info;
/* Store positions of group of BOs */
struct ttm_lru_bulk_move lru_bulk_move; };
struct amdgpu_vm_manager {
On Thu, Aug 09, 2018 at 03:18:55PM +0800, Koenig, Christian wrote:
Am 09.08.2018 um 08:18 schrieb Huang Rui:
On Wed, Aug 08, 2018 at 06:47:49PM +0800, Christian König wrote:
Am 08.08.2018 um 11:59 schrieb Huang Rui:
I continue to work for bulk moving that based on the proposal by Christian.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++++ 2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..eda0bb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -286,6 +286,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, { struct ttm_bo_global *glob = adev->mman.bdev.glob; struct amdgpu_vm_bo_base *bo_base, *tmp;
bool validated = false; int r = 0;
list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {
@@ -295,14 +296,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, r = validate(param, bo); if (r) break;
spin_lock(&glob->lru_lock);ttm_bo_move_to_lru_tail(&bo->tbo, NULL);if (bo->shadow)ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);spin_unlock(&glob->lru_lock); }
validated = true; if (bo->tbo.type != ttm_bo_type_kernel) { spin_lock(&vm->moved_lock); list_move(&bo_base->vm_status, &vm->moved);@@ -312,6 +308,15 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } }
- if (!validated) {
spin_lock(&glob->lru_lock);ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);spin_unlock(&glob->lru_lock);return 0;- }
- memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
- spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo;
@@ -319,9 +324,10 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow)
ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo,&vm->lru_bulk_move);Here is still one problem: The BOs currently evicted are not yet in the idle state.
So this must be moved to the end of the state machine, probably best if we put that into a separate function.
Thanks, so the evicted BOs should be behind the BOs in idle list that in the group between (first, last) of bulk_move_pos, right?
The order doesn't matter.
spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo; if (!bo->parent) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {That won't work. The BOs get removed from the evicted list when they are validated.
You need to walk both the relocated as well as the moved list here.
Is that because, at the start of the function, we already walk the evicted list, and some of BOs are validated and move to moved or relocated list, so here we need also walk both of them to make them together for bulk move?
Thanks, Ray
Christian.
struct amdgpu_bo *bo = bo_base->bo; if (!bo) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } spin_unlock(&glob->lru_lock);Thanks, Ray
Regards, Christian.
} spin_unlock(&glob->lru_lock);diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 67a15d4..92725ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -29,6 +29,7 @@ #include <linux/rbtree.h> #include <drm/gpu_scheduler.h> #include <drm/drm_file.h> +#include <drm/ttm/ttm_bo_driver.h>
#include "amdgpu_sync.h" #include "amdgpu_ring.h" @@ -226,6 +227,9 @@ struct amdgpu_vm {
/* Some basic info about the task */ struct amdgpu_task_info task_info;
/* Store positions of group of BOs */
struct ttm_lru_bulk_move lru_bulk_move; };
struct amdgpu_vm_manager {
Am 09.08.2018 um 14:25 schrieb Huang Rui:
On Thu, Aug 09, 2018 at 03:18:55PM +0800, Koenig, Christian wrote:
Am 09.08.2018 um 08:18 schrieb Huang Rui:
On Wed, Aug 08, 2018 at 06:47:49PM +0800, Christian König wrote:
Am 08.08.2018 um 11:59 schrieb Huang Rui:
I continue to work for bulk moving that based on the proposal by Christian.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++++ 2 files changed, 18 insertions(+), 8 deletions(-)diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..eda0bb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -286,6 +286,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, { struct ttm_bo_global *glob = adev->mman.bdev.glob; struct amdgpu_vm_bo_base *bo_base, *tmp;
bool validated = false; int r = 0;
list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {@@ -295,14 +296,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, r = validate(param, bo); if (r) break;
spin_lock(&glob->lru_lock);ttm_bo_move_to_lru_tail(&bo->tbo, NULL);if (bo->shadow)ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);spin_unlock(&glob->lru_lock); }
validated = true; if (bo->tbo.type != ttm_bo_type_kernel) { spin_lock(&vm->moved_lock); list_move(&bo_base->vm_status, &vm->moved);@@ -312,6 +308,15 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } }
- if (!validated) {
spin_lock(&glob->lru_lock);ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);spin_unlock(&glob->lru_lock);return 0;- }
- memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo;@@ -319,9 +324,10 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow)
ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo,&vm->lru_bulk_move);Here is still one problem: The BOs currently evicted are not yet in the idle state.
So this must be moved to the end of the state machine, probably best if we put that into a separate function.
Thanks, so the evicted BOs should be behind the BOs in idle list that in the group between (first, last) of bulk_move_pos, right?
The order doesn't matter.
spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo; if (!bo->parent) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {That won't work. The BOs get removed from the evicted list when they are validated.
You need to walk both the relocated as well as the moved list here.
Is that because, at the start of the function, we already walk the evicted list, and some of BOs are validated and move to moved or relocated list, so here we need also walk both of them to make them together for bulk move?
Yes, exactly.
Alternatively you can add a new function which is called after command submission is done to move everything on the LRU.
At this point all BOs should now be on the idle list again and you only need to walk that one.
Regards, Christian.
Thanks, Ray
Christian.
struct amdgpu_bo *bo = bo_base->bo; if (!bo) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } spin_unlock(&glob->lru_lock);Thanks, Ray
Regards, Christian.
} spin_unlock(&glob->lru_lock);diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 67a15d4..92725ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -29,6 +29,7 @@ #include <linux/rbtree.h> #include <drm/gpu_scheduler.h> #include <drm/drm_file.h> +#include <drm/ttm/ttm_bo_driver.h>
#include "amdgpu_sync.h" #include "amdgpu_ring.h"@@ -226,6 +227,9 @@ struct amdgpu_vm {
/* Some basic info about the task */ struct amdgpu_task_info task_info;
/* Store positions of group of BOs */
struct ttm_lru_bulk_move lru_bulk_move; };
struct amdgpu_vm_manager {
On Thu, Aug 09, 2018 at 08:27:10PM +0800, Koenig, Christian wrote:
Am 09.08.2018 um 14:25 schrieb Huang Rui:
On Thu, Aug 09, 2018 at 03:18:55PM +0800, Koenig, Christian wrote:
Am 09.08.2018 um 08:18 schrieb Huang Rui:
On Wed, Aug 08, 2018 at 06:47:49PM +0800, Christian König wrote:
Am 08.08.2018 um 11:59 schrieb Huang Rui:
I continue to work for bulk moving that based on the proposal by Christian.
Background: amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of them on the end of LRU list one by one. Thus, that cause so many BOs moved to the end of the LRU, and impact performance seriously.
Then Christian provided a workaround to not move PD/PT BOs on LRU with below patch: "drm/amdgpu: band aid validating VM PTs" Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU instead of one by one.
Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be validated we move all BOs together to the end of the LRU without dropping the lock for the LRU.
While doing so we note the beginning and end of this block in the LRU list.
Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do, we don't move every BO one by one, but instead cut the LRU list into pieces so that we bulk move everything to the end in just one operation.
Test data: +--------------+-----------------+-----------+---------------------------------------+ | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) | | |Principle(Vulkan)| | | +------------------------------------------------------------------------------------+ | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) | | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) | +------------------------------------------------------------------------------------+ | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) | |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)| |PT BOs on LRU)| | | | +------------------------------------------------------------------------------------+ | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) | | | | |0.214 ms(8K) 0.225 ms(16K) | +--------------+-----------------+-----------+---------------------------------------+
After test them with above three benchmarks include vulkan and opencl. We can see the visible improvement than original, and even better than original with workaround.
Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Huang Rui ray.huang@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 ++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++++ 2 files changed, 18 insertions(+), 8 deletions(-)diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..eda0bb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -286,6 +286,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, { struct ttm_bo_global *glob = adev->mman.bdev.glob; struct amdgpu_vm_bo_base *bo_base, *tmp;
bool validated = false; int r = 0;
list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {@@ -295,14 +296,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, r = validate(param, bo); if (r) break;
spin_lock(&glob->lru_lock);ttm_bo_move_to_lru_tail(&bo->tbo, NULL);if (bo->shadow)ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);spin_unlock(&glob->lru_lock); }
validated = true; if (bo->tbo.type != ttm_bo_type_kernel) { spin_lock(&vm->moved_lock); list_move(&bo_base->vm_status, &vm->moved);@@ -312,6 +308,15 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } }
- if (!validated) {
spin_lock(&glob->lru_lock);ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);spin_unlock(&glob->lru_lock);return 0;- }
- memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo;@@ -319,9 +324,10 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!bo->parent) continue;
ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow)
ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
ttm_bo_move_to_lru_tail(&bo->shadow->tbo,&vm->lru_bulk_move);Here is still one problem: The BOs currently evicted are not yet in the idle state.
So this must be moved to the end of the state machine, probably best if we put that into a separate function.
Thanks, so the evicted BOs should be behind the BOs in idle list that in the group between (first, last) of bulk_move_pos, right?
The order doesn't matter.
spin_lock(&glob->lru_lock); list_for_each_entry(bo_base, &vm->idle, vm_status) { struct amdgpu_bo *bo = bo_base->bo; if (!bo->parent) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {That won't work. The BOs get removed from the evicted list when they are validated.
You need to walk both the relocated as well as the moved list here.
Is that because, at the start of the function, we already walk the evicted list, and some of BOs are validated and move to moved or relocated list, so here we need also walk both of them to make them together for bulk move?
Yes, exactly.
Alternatively you can add a new function which is called after command submission is done to move everything on the LRU.
At this point all BOs should now be on the idle list again and you only need to walk that one.
Do you mean we move all BOs to idle list again in amdgpu_vm_bo_update of amdgpu_cs_ib_vm_chunk subsequently? But at that time, I see if the BO is evicted, it will be moved to evicted list, not to idle list again. Am I missed something?
Thanks, Ray
Regards, Christian.
Thanks, Ray
Christian.
struct amdgpu_bo *bo = bo_base->bo; if (!bo) continue; ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move); if (bo->shadow) ttm_bo_move_to_lru_tail(&bo->shadow->tbo, &vm->lru_bulk_move); } spin_unlock(&glob->lru_lock);Thanks, Ray
Regards, Christian.
} spin_unlock(&glob->lru_lock);diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 67a15d4..92725ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -29,6 +29,7 @@ #include <linux/rbtree.h> #include <drm/gpu_scheduler.h> #include <drm/drm_file.h> +#include <drm/ttm/ttm_bo_driver.h>
#include "amdgpu_sync.h" #include "amdgpu_ring.h"@@ -226,6 +227,9 @@ struct amdgpu_vm {
/* Some basic info about the task */ struct amdgpu_task_info task_info;
/* Store positions of group of BOs */
struct ttm_lru_bulk_move lru_bulk_move; };
struct amdgpu_vm_manager {
The new bulk moving functionality is ready, the overhead of moving PD/PT bos to LRU is fixed. So move them on LRU again.
Signed-off-by: Huang Rui ray.huang@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index eda0bb9..3550166 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -1090,7 +1090,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev, struct amdgpu_vm_bo_base, vm_status); bo_base->moved = false; - list_del_init(&bo_base->vm_status); + list_move(&bo_base->vm_status, &vm->idle);
bo = bo_base->bo->parent; if (!bo)
dri-devel@lists.freedesktop.org
-
Christian König -
Christian König -
Huang Rui