Re: [PATCH 1/6] drm/amdgpu: rework VM state machine lock handling v2
Patch 1:
could you show the reserving VM?
The idea is that only the moved state is accessed without the VM being
reserved.
Because of this we can remove the locks when we a) are sure that the BO
isn't in the moved state and b) don't want to move it into the moved state.
on 2nd thought, perhaps going to reduce the movement for per vm bo for
performance increasing.
However, the move lru tail in amdgpu_vm_validate_pt_bos() looks change the per
vm bo order again.
IIRC, it expects relative stable order for per vm bo to get a better
performance.
What can happen is that a BO is not evicted because we need visible VRAM.
But the gross of all BOs should be evicted in the order they are
allocated and are then moved back into VRAM in the order they are
allocated etc...
That isn't 100% stable but still seems to be good enough.
Regards,
Christian.
Am 19.05.2018 um 10:00 schrieb Zhang, Jerry:
Patch 6:
I could read that code, but not sure the purpose.
on 2nd thought, perhaps going to reduce the movement for per vm bo for
performance increasing.
However, the move lru tail in amdgpu_vm_validate_pt_bos() looks change the per
vm bo order again.
IIRC, it expects relative stable order for per vm bo to get a better
performance.
Please confirm that.
Regards,
Jerry(Junwei Zhang)
From: amd-gfx on behalf of Zhang, Jerry
(Junwei)
Sent: Friday, May 18, 2018 5:55:31 PM
To: Christian König; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/amdgpu: rework VM state machine lock handling v2
2, 3, 4, 5 are
Reviewed-by: Junwei Zhang
Patch 1:
could you show the reserving VM?
Patch 6:
I could read that code, but not sure the purpose.
Jerry
On 05/17/2018 05:49 PM, Christian König wrote:
Only the moved state needs a separate spin lock protection. All other
states are protected by reserving the VM anyway.
v2: fix some more incorrect cases
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 66
+++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +--
2 files changed, 21 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1a8f4e0dd023..f0deedcaf1c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -119,9 +119,7 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base
*base,
* is currently evicted. add the bo to the evicted list to make sure it
* is validated on next vm use to avoid fault.
* */
- spin_lock(&vm->status_lock);
list_move_tail(&base->vm_status, &vm->evicted);
- spin_unlock(&vm->status_lock);
}
/**
@@ -228,7 +226,6 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
struct ttm_bo_global *glob = adev->mman.bdev.glob;
int r;
- spin_lock(&vm->status_lock);
while (!list_empty(&vm->evicted)) {
struct amdgpu_vm_bo_base *bo_base;
struct amdgpu_bo *bo;
@@ -236,10 +233,8 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
bo_base = list_first_entry(&vm->evicted,
struct amdgpu_vm_bo_base,
vm_status);
- spin_unlock(&vm->status_lock);
bo = bo_base->bo;
- BUG_ON(!bo);
if (bo->parent) {
r = validate(param, bo);
if (r)
@@ -259,13 +254,14 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
return r;
}
- spin_lock(&vm->status_lock);
- if (bo->tbo.type != ttm_bo_type_kernel)
+ if (bo->tbo.type != ttm_bo_type_kernel) {
+ spin_lock(&vm->moved_lock);
list_move(&bo_base->vm_status, &vm->moved);
- else
+ spin_unlock(&vm->moved_lock);
+ } else {
list_move(&bo_base->vm_status, &vm->relocated);
+ }
}
- spin_unlock(&vm->status_lock);
return 0;
}
@@ -279,13 +275,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
*/
bool amdgpu_vm_ready(struct amdgpu_vm *vm)
{
- bool ready;
-
- spin_lock(&vm->status_lock);
- ready = list_empty(&vm->evicted);
- spin_unlock(&vm->status_lock);
-
- return ready;
+ return list_empty(&vm->evicted);
}
/**
@@ -477,9 +467,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device
*adev,
pt->parent = amdgpu_bo_ref(parent->base.bo);
Re: [PATCH 1/6] drm/amdgpu: rework VM state machine lock handling v2
> Patch 6:
> I could read that code, but not sure the purpose.
on 2nd thought, perhaps going to reduce the movement for per vm bo for
performance increasing.
However, the move lru tail in amdgpu_vm_validate_pt_bos() looks change the per
vm bo order again.
IIRC, it expects relative stable order for per vm bo to get a better
performance.
Please confirm that.
Regards,
Jerry(Junwei Zhang)
From: amd-gfx on behalf of Zhang, Jerry
(Junwei)
Sent: Friday, May 18, 2018 5:55:31 PM
To: Christian König; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/amdgpu: rework VM state machine lock handling v2
2, 3, 4, 5 are
Reviewed-by: Junwei Zhang
Patch 1:
could you show the reserving VM?
Patch 6:
I could read that code, but not sure the purpose.
Jerry
On 05/17/2018 05:49 PM, Christian König wrote:
> Only the moved state needs a separate spin lock protection. All other
> states are protected by reserving the VM anyway.
>
> v2: fix some more incorrect cases
>
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 66
> +++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +--
> 2 files changed, 21 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 1a8f4e0dd023..f0deedcaf1c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -119,9 +119,7 @@ static void amdgpu_vm_bo_base_init(struct
> amdgpu_vm_bo_base *base,
>* is currently evicted. add the bo to the evicted list to make sure it
>* is validated on next vm use to avoid fault.
>* */
> - spin_lock(&vm->status_lock);
> list_move_tail(&base->vm_status, &vm->evicted);
> - spin_unlock(&vm->status_lock);
> }
>
> /**
> @@ -228,7 +226,6 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> struct ttm_bo_global *glob = adev->mman.bdev.glob;
> int r;
>
> - spin_lock(&vm->status_lock);
> while (!list_empty(&vm->evicted)) {
> struct amdgpu_vm_bo_base *bo_base;
> struct amdgpu_bo *bo;
> @@ -236,10 +233,8 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device
> *adev, struct amdgpu_vm *vm,
> bo_base = list_first_entry(&vm->evicted,
> struct amdgpu_vm_bo_base,
> vm_status);
> - spin_unlock(&vm->status_lock);
>
> bo = bo_base->bo;
> - BUG_ON(!bo);
> if (bo->parent) {
> r = validate(param, bo);
> if (r)
> @@ -259,13 +254,14 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device
> *adev, struct amdgpu_vm *vm,
> return r;
> }
>
> - spin_lock(&vm->status_lock);
> - if (bo->tbo.type != ttm_bo_type_kernel)
> + if (bo->tbo.type != ttm_bo_type_kernel) {
> + spin_lock(&vm->moved_lock);
> list_move(&bo_base->vm_status, &vm->moved);
> - else
> + spin_unlock(&vm->moved_lock);
> + } else {
> list_move(&bo_base->vm_status, &vm->relocated);
> + }
> }
> - spin_unlock(&vm->status_lock);
>
> return 0;
> }
> @@ -279,13 +275,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device
> *adev, struct amdgpu_vm *vm,
>*/
> bool amdgpu_vm_ready(struct amdgpu_vm *vm)
> {
> - bool ready;
> -
> - spin_lock(&vm->status_lock);
> - ready = list_empty(&vm->evicted);
> - spin_unlock(&vm->status_lock);
> -
> - return ready;
> + return list_empty(&vm->evicted);
> }
>
> /**
> @@ -477,9 +467,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device
> *adev,
> pt->parent = amdgpu_bo_ref(parent->base.bo);
>
> amdgpu_vm_bo_base_init(&entry->base, vm, pt);
> - spin_lock(&vm->status_lock);
> list_move(&entry->base.vm_status, &vm->relocated);
> - spin_unlock(&vm->status_lock);
> }
>
> if (level < AMDGPU_VM_PTB) {
> @@ -926,10 +914,8 @@ static void amdgpu_vm_invalidate_level(struct
> amdgpu_device *adev,
> if (!entry->base.bo)
>
Re: [PATCH 1/6] drm/amdgpu: rework VM state machine lock handling v2
2, 3, 4, 5 are
Reviewed-by: Junwei Zhang
Patch 1:
could you show the reserving VM?
Patch 6:
I could read that code, but not sure the purpose.
Jerry
On 05/17/2018 05:49 PM, Christian König wrote:
Only the moved state needs a separate spin lock protection. All other
states are protected by reserving the VM anyway.
v2: fix some more incorrect cases
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 66 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +--
2 files changed, 21 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1a8f4e0dd023..f0deedcaf1c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -119,9 +119,7 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base
*base,
* is currently evicted. add the bo to the evicted list to make sure it
* is validated on next vm use to avoid fault.
* */
- spin_lock(&vm->status_lock);
list_move_tail(&base->vm_status, &vm->evicted);
- spin_unlock(&vm->status_lock);
}
/**
@@ -228,7 +226,6 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
struct ttm_bo_global *glob = adev->mman.bdev.glob;
int r;
- spin_lock(&vm->status_lock);
while (!list_empty(&vm->evicted)) {
struct amdgpu_vm_bo_base *bo_base;
struct amdgpu_bo *bo;
@@ -236,10 +233,8 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
bo_base = list_first_entry(&vm->evicted,
struct amdgpu_vm_bo_base,
vm_status);
- spin_unlock(&vm->status_lock);
bo = bo_base->bo;
- BUG_ON(!bo);
if (bo->parent) {
r = validate(param, bo);
if (r)
@@ -259,13 +254,14 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
return r;
}
- spin_lock(&vm->status_lock);
- if (bo->tbo.type != ttm_bo_type_kernel)
+ if (bo->tbo.type != ttm_bo_type_kernel) {
+ spin_lock(&vm->moved_lock);
list_move(&bo_base->vm_status, &vm->moved);
- else
+ spin_unlock(&vm->moved_lock);
+ } else {
list_move(&bo_base->vm_status, &vm->relocated);
+ }
}
- spin_unlock(&vm->status_lock);
return 0;
}
@@ -279,13 +275,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
*/
bool amdgpu_vm_ready(struct amdgpu_vm *vm)
{
- bool ready;
-
- spin_lock(&vm->status_lock);
- ready = list_empty(&vm->evicted);
- spin_unlock(&vm->status_lock);
-
- return ready;
+ return list_empty(&vm->evicted);
}
/**
@@ -477,9 +467,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device
*adev,
pt->parent = amdgpu_bo_ref(parent->base.bo);
amdgpu_vm_bo_base_init(&entry->base, vm, pt);
- spin_lock(&vm->status_lock);
list_move(&entry->base.vm_status, &vm->relocated);
- spin_unlock(&vm->status_lock);
}
if (level < AMDGPU_VM_PTB) {
@@ -926,10 +914,8 @@ static void amdgpu_vm_invalidate_level(struct
amdgpu_device *adev,
if (!entry->base.bo)
continue;
- spin_lock(&vm->status_lock);
if (list_empty(&entry->base.vm_status))
list_add(&entry->base.vm_status, &vm->relocated);
- spin_unlock(&vm->status_lock);
amdgpu_vm_invalidate_level(adev, vm, entry, level + 1);
}
}
@@ -974,7 +960,6 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
params.func = amdgpu_vm_do_set_ptes;
}
- spin_lock(&vm->status_lock);
while (!list_empty(&vm->relocated)) {
struct amdgpu_vm_bo_base *bo_base, *parent;
struct amdgpu_vm_pt *pt, *entry;
@@ -984,13 +969,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device
*adev,
struct amdgpu_vm_bo_base,
vm_status);
list_del_init(&bo_base->vm_status);
- spin_unlock(&vm->status_lock);
bo = bo_base->bo->parent;
- if (!bo) {
- spin_lock(&vm->status_lock);
+ if (!bo)
continue;
- }
parent = list_first_entry(&bo->va, struct amdgpu_vm_bo_base,
[PATCH 1/6] drm/amdgpu: rework VM state machine lock handling v2
Only the moved state needs a separate spin lock protection. All other
states are protected by reserving the VM anyway.
v2: fix some more incorrect cases
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 66 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +--
2 files changed, 21 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1a8f4e0dd023..f0deedcaf1c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -119,9 +119,7 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base
*base,
* is currently evicted. add the bo to the evicted list to make sure it
* is validated on next vm use to avoid fault.
* */
- spin_lock(&vm->status_lock);
list_move_tail(&base->vm_status, &vm->evicted);
- spin_unlock(&vm->status_lock);
}
/**
@@ -228,7 +226,6 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
struct ttm_bo_global *glob = adev->mman.bdev.glob;
int r;
- spin_lock(&vm->status_lock);
while (!list_empty(&vm->evicted)) {
struct amdgpu_vm_bo_base *bo_base;
struct amdgpu_bo *bo;
@@ -236,10 +233,8 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
bo_base = list_first_entry(&vm->evicted,
struct amdgpu_vm_bo_base,
vm_status);
- spin_unlock(&vm->status_lock);
bo = bo_base->bo;
- BUG_ON(!bo);
if (bo->parent) {
r = validate(param, bo);
if (r)
@@ -259,13 +254,14 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
return r;
}
- spin_lock(&vm->status_lock);
- if (bo->tbo.type != ttm_bo_type_kernel)
+ if (bo->tbo.type != ttm_bo_type_kernel) {
+ spin_lock(&vm->moved_lock);
list_move(&bo_base->vm_status, &vm->moved);
- else
+ spin_unlock(&vm->moved_lock);
+ } else {
list_move(&bo_base->vm_status, &vm->relocated);
+ }
}
- spin_unlock(&vm->status_lock);
return 0;
}
@@ -279,13 +275,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
*/
bool amdgpu_vm_ready(struct amdgpu_vm *vm)
{
- bool ready;
-
- spin_lock(&vm->status_lock);
- ready = list_empty(&vm->evicted);
- spin_unlock(&vm->status_lock);
-
- return ready;
+ return list_empty(&vm->evicted);
}
/**
@@ -477,9 +467,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device
*adev,
pt->parent = amdgpu_bo_ref(parent->base.bo);
amdgpu_vm_bo_base_init(&entry->base, vm, pt);
- spin_lock(&vm->status_lock);
list_move(&entry->base.vm_status, &vm->relocated);
- spin_unlock(&vm->status_lock);
}
if (level < AMDGPU_VM_PTB) {
@@ -926,10 +914,8 @@ static void amdgpu_vm_invalidate_level(struct
amdgpu_device *adev,
if (!entry->base.bo)
continue;
- spin_lock(&vm->status_lock);
if (list_empty(&entry->base.vm_status))
list_add(&entry->base.vm_status, &vm->relocated);
- spin_unlock(&vm->status_lock);
amdgpu_vm_invalidate_level(adev, vm, entry, level + 1);
}
}
@@ -974,7 +960,6 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
params.func = amdgpu_vm_do_set_ptes;
}
- spin_lock(&vm->status_lock);
while (!list_empty(&vm->relocated)) {
struct amdgpu_vm_bo_base *bo_base, *parent;
struct amdgpu_vm_pt *pt, *entry;
@@ -984,13 +969,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device
*adev,
struct amdgpu_vm_bo_base,
vm_status);
list_del_init(&bo_base->vm_status);
- spin_unlock(&vm->status_lock);
bo = bo_base->bo->parent;
- if (!bo) {
- spin_lock(&vm->status_lock);
+ if (!bo)
continue;
- }
parent = list_first_entry(&bo->va, struct amdgpu_vm_bo_base,
bo_list);
@@ -999,12 +981,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device
*adev,
amdgpu_vm_update_pde(¶ms, vm, pt, entry);
- spin_lock(&v
