| Message ID | 1533902159-14232-5-git-send-email-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ttm,amdgpu: Introduce LRU bulk move functionality | expand |
Am 10.08.2018 um 13:55 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. > > v2: move all BOs include idle, relocated, and moved list to the end of LRU and > put them 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 | 75 ++++++++++++++++++++++++++-------- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++ > 2 files changed, 63 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 9c84770..81fb1ac 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -268,6 +268,55 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, > } > > /** > + * amdgpu_vm_move_to_lru_tail - move one list of BOs to end of LRU > + * > + * @adev: amdgpu device pointer > + * @vm: vm providing the BOs > + * @list: the list that stored BOs > + * > + * Move one list of BOs to the end of LRU and update the positions. > + */ > +static void > +amdgpu_vm_move_to_lru_tail_by_list(struct amdgpu_device *adev, > + struct amdgpu_vm *vm, struct list_head *list) Am I blind or is adev not actually used in the function? > +{ > + struct amdgpu_vm_bo_base *bo_base, *tmp; > + > + list_for_each_entry_safe(bo_base, tmp, list, vm_status) { No need for the _safe variant as far as I can see. Apart from that this looks good to me. Christian. > + 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); > + } > +} > + > +/** > + * amdgpu_vm_move_to_lru_tail - move all BOs to the end of LRU > + * > + * @adev: amdgpu device pointer > + * @vm: vm providing the BOs > + * > + * Move all BOs to the end of LRU and remember their positions to put them > + * together. > + */ > +static void > +amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev, struct amdgpu_vm *vm) > +{ > + struct ttm_bo_global *glob = adev->mman.bdev.glob; > + > + spin_lock(&glob->lru_lock); > + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->idle); > + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->relocated); > + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->moved); > + spin_unlock(&glob->lru_lock); > +} > + > +/** > * amdgpu_vm_validate_pt_bos - validate the page table BOs > * > * @adev: amdgpu device pointer > @@ -286,6 +335,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 +345,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,18 +357,16 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, > } > } > > - spin_lock(&glob->lru_lock); > - list_for_each_entry(bo_base, &vm->idle, vm_status) { > - struct amdgpu_bo *bo = bo_base->bo; > + if (!validated) { > + spin_lock(&glob->lru_lock); > + ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move); > + spin_unlock(&glob->lru_lock); > + return 0; > + } > > - if (!bo->parent) > - continue; > + memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move)); > > - 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); > + amdgpu_vm_move_to_lru_tail(adev, vm); > > return r; > } > 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 Fri, Aug 10, 2018 at 10:18:24PM +0800, Koenig, Christian wrote: > Am 10.08.2018 um 13:55 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. > > > > v2: move all BOs include idle, relocated, and moved list to the end of LRU and > > put them 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 | 75 ++++++++++++++++++++++++++-------- > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 ++ > > 2 files changed, 63 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > index 9c84770..81fb1ac 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > @@ -268,6 +268,55 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, > > } > > > > /** > > + * amdgpu_vm_move_to_lru_tail - move one list of BOs to end of LRU > > + * > > + * @adev: amdgpu device pointer > > + * @vm: vm providing the BOs > > + * @list: the list that stored BOs > > + * > > + * Move one list of BOs to the end of LRU and update the positions. > > + */ > > +static void > > +amdgpu_vm_move_to_lru_tail_by_list(struct amdgpu_device *adev, > > + struct amdgpu_vm *vm, struct list_head *list) > > Am I blind or is adev not actually used in the function? > No, what you see is definitely real. ;-) That's my fault to add an unused adev int the function. Will remove it in next version. > > +{ > > + struct amdgpu_vm_bo_base *bo_base, *tmp; > > + > > + list_for_each_entry_safe(bo_base, tmp, list, vm_status) { > > No need for the _safe variant as far as I can see. > Yes, we won't move vm->vm_status to another list in the loop. Thanks, Ray > Apart from that this looks good to me. > > Christian. > > > + 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); > > + } > > +} > > + > > +/** > > + * amdgpu_vm_move_to_lru_tail - move all BOs to the end of LRU > > + * > > + * @adev: amdgpu device pointer > > + * @vm: vm providing the BOs > > + * > > + * Move all BOs to the end of LRU and remember their positions to put them > > + * together. > > + */ > > +static void > > +amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev, struct amdgpu_vm *vm) > > +{ > > + struct ttm_bo_global *glob = adev->mman.bdev.glob; > > + > > + spin_lock(&glob->lru_lock); > > + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->idle); > > + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->relocated); > > + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->moved); > > + spin_unlock(&glob->lru_lock); > > +} > > + > > +/** > > * amdgpu_vm_validate_pt_bos - validate the page table BOs > > * > > * @adev: amdgpu device pointer > > @@ -286,6 +335,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 +345,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,18 +357,16 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, > > } > > } > > > > - spin_lock(&glob->lru_lock); > > - list_for_each_entry(bo_base, &vm->idle, vm_status) { > > - struct amdgpu_bo *bo = bo_base->bo; > > + if (!validated) { > > + spin_lock(&glob->lru_lock); > > + ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move); > > + spin_unlock(&glob->lru_lock); > > + return 0; > > + } > > > > - if (!bo->parent) > > - continue; > > + memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move)); > > > > - 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); > > + amdgpu_vm_move_to_lru_tail(adev, vm); > > > > return r; > > } > > 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 { >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9c84770..81fb1ac 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -268,6 +268,55 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, } /** + * amdgpu_vm_move_to_lru_tail - move one list of BOs to end of LRU + * + * @adev: amdgpu device pointer + * @vm: vm providing the BOs + * @list: the list that stored BOs + * + * Move one list of BOs to the end of LRU and update the positions. + */ +static void +amdgpu_vm_move_to_lru_tail_by_list(struct amdgpu_device *adev, + struct amdgpu_vm *vm, struct list_head *list) +{ + struct amdgpu_vm_bo_base *bo_base, *tmp; + + list_for_each_entry_safe(bo_base, tmp, list, 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); + } +} + +/** + * amdgpu_vm_move_to_lru_tail - move all BOs to the end of LRU + * + * @adev: amdgpu device pointer + * @vm: vm providing the BOs + * + * Move all BOs to the end of LRU and remember their positions to put them + * together. + */ +static void +amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev, struct amdgpu_vm *vm) +{ + struct ttm_bo_global *glob = adev->mman.bdev.glob; + + spin_lock(&glob->lru_lock); + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->idle); + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->relocated); + amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->moved); + spin_unlock(&glob->lru_lock); +} + +/** * amdgpu_vm_validate_pt_bos - validate the page table BOs * * @adev: amdgpu device pointer @@ -286,6 +335,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 +345,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,18 +357,16 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm, } } - spin_lock(&glob->lru_lock); - list_for_each_entry(bo_base, &vm->idle, vm_status) { - struct amdgpu_bo *bo = bo_base->bo; + if (!validated) { + spin_lock(&glob->lru_lock); + ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move); + spin_unlock(&glob->lru_lock); + return 0; + } - if (!bo->parent) - continue; + memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move)); - 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); + amdgpu_vm_move_to_lru_tail(adev, vm); return r; } 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 {