RE: [PATCH v3 4/6] drm/amdgpu: switch to amdgpu_bo_vm for vm code
[AMD Official Use Only]
Thanks Christian, I will resend with your suggestions included.
Nirmoy
-Original Message-
From: Koenig, Christian
Sent: Friday, May 28, 2021 10:01 AM
To: Das, Nirmoy ; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: Re: [PATCH v3 4/6] drm/amdgpu: switch to amdgpu_bo_vm for vm code
Am 27.05.21 um 13:53 schrieb Nirmoy Das:
> The subclass, amdgpu_bo_vm is intended for PT/PD BOs which are also
> shadowed, so switch to amdgpu_bo_vm BO for PT/PD BOs.
>
> v3: simplify code.
> check also if shadow bo exist, instead of checking only bo's type.
> v2: squash three related patches.
>
> Signed-off-by: Nirmoy Das
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 93 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 18 ++--
> 2 files changed, 68 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 6bc7566cc193..d723873df765 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -652,15 +652,15 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device
> *adev,
> spin_lock(>mman.bdev.lru_lock);
> list_for_each_entry(bo_base, >idle, vm_status) {
> struct amdgpu_bo *bo = bo_base->bo;
> + struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
>
> if (!bo->parent)
> continue;
>
> ttm_bo_move_to_lru_tail(>tbo, >tbo.mem,
> >lru_bulk_move);
> - if (bo->shadow)
> - ttm_bo_move_to_lru_tail(>shadow->tbo,
> - >shadow->tbo.mem,
> + if (shadow)
> + ttm_bo_move_to_lru_tail(>tbo, >tbo.mem,
> >lru_bulk_move);
> }
> spin_unlock(>mman.bdev.lru_lock);
> @@ -692,12 +692,13 @@ int amdgpu_vm_validate_pt_bos(struct
> amdgpu_device *adev, struct amdgpu_vm *vm,
>
> list_for_each_entry_safe(bo_base, tmp, >evicted, vm_status) {
> struct amdgpu_bo *bo = bo_base->bo;
> + struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
>
> r = validate(param, bo);
> if (r)
> return r;
> - if (bo->shadow) {
> - r = validate(param, bo->shadow);
> + if (shadow) {
> + r = validate(param, shadow);
> if (r)
> return r;
> }
> @@ -754,6 +755,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> unsigned level = adev->vm_manager.root_level;
> struct amdgpu_vm_update_params params;
> struct amdgpu_bo *ancestor = bo;
> + struct amdgpu_bo *shadow;
> unsigned entries, ats_entries;
> uint64_t addr;
> int r;
> @@ -793,9 +795,9 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> if (r)
> return r;
>
> - if (bo->shadow) {
> - r = ttm_bo_validate(>shadow->tbo, >shadow->placement,
> - );
> + shadow = amdgpu_bo_shadowed(bo);
> + if (shadow) {
> + r = ttm_bo_validate(>tbo, >placement, );
> if (r)
> return r;
> }
> @@ -863,14 +865,16 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device
> *adev,
>* @vm: requesting vm
>* @level: the page table level
>* @immediate: use a immediate update
> - * @bo: pointer to the buffer object pointer
> + * @vmbo: pointer to the buffer object pointer
>*/
> static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> int level, bool immediate,
> -struct amdgpu_bo **bo)
> +struct amdgpu_bo_vm **vmbo)
> {
> struct amdgpu_bo_param bp;
> + struct amdgpu_bo *bo;
> + struct dma_resv *resv;
> int r;
>
> memset(, 0, sizeof(bp));
> @@ -881,7 +885,7 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
> bp.domain = amdgpu_bo_get_preferred_pin_domain(adev, bp.domain);
> bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
> AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> - bp.bo_ptr_size = sizeof(struct amdgpu_bo);
> + bp.bo_ptr_size = sizeof(struct amdgpu_bo_vm);
> if (vm->use_cpu_for_update)
> bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>
Re: [PATCH v3 4/6] drm/amdgpu: switch to amdgpu_bo_vm for vm code
Am 27.05.21 um 13:53 schrieb Nirmoy Das:
The subclass, amdgpu_bo_vm is intended for PT/PD BOs which are also
shadowed, so switch to amdgpu_bo_vm BO for PT/PD BOs.
v3: simplify code.
check also if shadow bo exist, instead of checking only bo's type.
v2: squash three related patches.
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 93 +
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 18 ++--
2 files changed, 68 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 6bc7566cc193..d723873df765 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -652,15 +652,15 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device
*adev,
spin_lock(>mman.bdev.lru_lock);
list_for_each_entry(bo_base, >idle, vm_status) {
struct amdgpu_bo *bo = bo_base->bo;
+ struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
if (!bo->parent)
continue;
ttm_bo_move_to_lru_tail(>tbo, >tbo.mem,
>lru_bulk_move);
- if (bo->shadow)
- ttm_bo_move_to_lru_tail(>shadow->tbo,
- >shadow->tbo.mem,
+ if (shadow)
+ ttm_bo_move_to_lru_tail(>tbo, >tbo.mem,
>lru_bulk_move);
}
spin_unlock(>mman.bdev.lru_lock);
@@ -692,12 +692,13 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
list_for_each_entry_safe(bo_base, tmp, >evicted, vm_status) {
struct amdgpu_bo *bo = bo_base->bo;
+ struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
r = validate(param, bo);
if (r)
return r;
- if (bo->shadow) {
- r = validate(param, bo->shadow);
+ if (shadow) {
+ r = validate(param, shadow);
if (r)
return r;
}
@@ -754,6 +755,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
unsigned level = adev->vm_manager.root_level;
struct amdgpu_vm_update_params params;
struct amdgpu_bo *ancestor = bo;
+ struct amdgpu_bo *shadow;
unsigned entries, ats_entries;
uint64_t addr;
int r;
@@ -793,9 +795,9 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
if (r)
return r;
- if (bo->shadow) {
- r = ttm_bo_validate(>shadow->tbo, >shadow->placement,
- );
+ shadow = amdgpu_bo_shadowed(bo);
+ if (shadow) {
+ r = ttm_bo_validate(>tbo, >placement, );
if (r)
return r;
}
@@ -863,14 +865,16 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
* @vm: requesting vm
* @level: the page table level
* @immediate: use a immediate update
- * @bo: pointer to the buffer object pointer
+ * @vmbo: pointer to the buffer object pointer
*/
static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
int level, bool immediate,
- struct amdgpu_bo **bo)
+ struct amdgpu_bo_vm **vmbo)
{
struct amdgpu_bo_param bp;
+ struct amdgpu_bo *bo;
+ struct dma_resv *resv;
int r;
memset(, 0, sizeof(bp));
@@ -881,7 +885,7 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
bp.domain = amdgpu_bo_get_preferred_pin_domain(adev, bp.domain);
bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
AMDGPU_GEM_CREATE_CPU_GTT_USWC;
- bp.bo_ptr_size = sizeof(struct amdgpu_bo);
+ bp.bo_ptr_size = sizeof(struct amdgpu_bo_vm);
if (vm->use_cpu_for_update)
bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
@@ -890,26 +894,41 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
if (vm->root.base.bo)
bp.resv = vm->root.base.bo->tbo.base.resv;
- r = amdgpu_bo_create(adev, , bo);
+ r = amdgpu_bo_create_vm(adev, , vmbo);
if (r)
return r;
- if (vm->is_compute_context && (adev->flags & AMD_IS_APU))
+ bo = &(*vmbo)->bo;
+ if (vm->is_compute_context && (adev->flags & AMD_IS_APU)) {
+ (*vmbo)->shadow = NULL;
return 0;
+ }
if (!bp.resv)
- WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
+ WARN_ON(dma_resv_lock(bo->tbo.base.resv,
NULL));
- r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
+ resv = bp.resv;
+
[PATCH v3 4/6] drm/amdgpu: switch to amdgpu_bo_vm for vm code
The subclass, amdgpu_bo_vm is intended for PT/PD BOs which are also
shadowed, so switch to amdgpu_bo_vm BO for PT/PD BOs.
v3: simplify code.
check also if shadow bo exist, instead of checking only bo's type.
v2: squash three related patches.
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 93 +
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 18 ++--
2 files changed, 68 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 6bc7566cc193..d723873df765 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -652,15 +652,15 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device
*adev,
spin_lock(>mman.bdev.lru_lock);
list_for_each_entry(bo_base, >idle, vm_status) {
struct amdgpu_bo *bo = bo_base->bo;
+ struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
if (!bo->parent)
continue;
ttm_bo_move_to_lru_tail(>tbo, >tbo.mem,
>lru_bulk_move);
- if (bo->shadow)
- ttm_bo_move_to_lru_tail(>shadow->tbo,
- >shadow->tbo.mem,
+ if (shadow)
+ ttm_bo_move_to_lru_tail(>tbo, >tbo.mem,
>lru_bulk_move);
}
spin_unlock(>mman.bdev.lru_lock);
@@ -692,12 +692,13 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
list_for_each_entry_safe(bo_base, tmp, >evicted, vm_status) {
struct amdgpu_bo *bo = bo_base->bo;
+ struct amdgpu_bo *shadow = amdgpu_bo_shadowed(bo);
r = validate(param, bo);
if (r)
return r;
- if (bo->shadow) {
- r = validate(param, bo->shadow);
+ if (shadow) {
+ r = validate(param, shadow);
if (r)
return r;
}
@@ -754,6 +755,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
unsigned level = adev->vm_manager.root_level;
struct amdgpu_vm_update_params params;
struct amdgpu_bo *ancestor = bo;
+ struct amdgpu_bo *shadow;
unsigned entries, ats_entries;
uint64_t addr;
int r;
@@ -793,9 +795,9 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
if (r)
return r;
- if (bo->shadow) {
- r = ttm_bo_validate(>shadow->tbo, >shadow->placement,
- );
+ shadow = amdgpu_bo_shadowed(bo);
+ if (shadow) {
+ r = ttm_bo_validate(>tbo, >placement, );
if (r)
return r;
}
@@ -863,14 +865,16 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
* @vm: requesting vm
* @level: the page table level
* @immediate: use a immediate update
- * @bo: pointer to the buffer object pointer
+ * @vmbo: pointer to the buffer object pointer
*/
static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
int level, bool immediate,
- struct amdgpu_bo **bo)
+ struct amdgpu_bo_vm **vmbo)
{
struct amdgpu_bo_param bp;
+ struct amdgpu_bo *bo;
+ struct dma_resv *resv;
int r;
memset(, 0, sizeof(bp));
@@ -881,7 +885,7 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
bp.domain = amdgpu_bo_get_preferred_pin_domain(adev, bp.domain);
bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
AMDGPU_GEM_CREATE_CPU_GTT_USWC;
- bp.bo_ptr_size = sizeof(struct amdgpu_bo);
+ bp.bo_ptr_size = sizeof(struct amdgpu_bo_vm);
if (vm->use_cpu_for_update)
bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
@@ -890,26 +894,41 @@ static int amdgpu_vm_pt_create(struct amdgpu_device *adev,
if (vm->root.base.bo)
bp.resv = vm->root.base.bo->tbo.base.resv;
- r = amdgpu_bo_create(adev, , bo);
+ r = amdgpu_bo_create_vm(adev, , vmbo);
if (r)
return r;
- if (vm->is_compute_context && (adev->flags & AMD_IS_APU))
+ bo = &(*vmbo)->bo;
+ if (vm->is_compute_context && (adev->flags & AMD_IS_APU)) {
+ (*vmbo)->shadow = NULL;
return 0;
+ }
if (!bp.resv)
- WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
+ WARN_ON(dma_resv_lock(bo->tbo.base.resv,
NULL));
- r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
+ resv = bp.resv;
+ memset(, 0, sizeof(bp));
+ bp.size =
